Clarify commit message from pr-auto-commit #6706
Conversation
📝 WalkthroughWalkthroughUpdates the GitHub Actions PR auto-commit workflow to configure git, run formatting/check steps that may conditionally commit, refactors change detection to compare HEAD with the PR head SHA, and records committed commands; also reorders imports in Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Actions Runner
participant Git as Git CLI
participant Formatter as Ruff/cargo fmt
participant Script as generate_opcode_metadata.py
participant GitHub as GitHub API
Runner->>Git: configure user.name/email
Runner->>Git: checkout PR head
Runner->>Formatter: run cargo fmt
Formatter-->>Runner: formatted files
Runner->>Git: check for changes vs PR head SHA
alt changes detected
Runner->>Git: commit formatting changes
end
Runner->>Formatter: run ruff format
Formatter-->>Runner: formatted Python
Runner->>Git: check & commit if changed
Runner->>Formatter: run ruff import-sorting check
Formatter-->>Runner: check result
Runner->>Git: commit changes if any
Runner->>Script: run generate_opcode_metadata.py
Script-->>Runner: generated files
Runner->>Git: commit generated files if any
Runner->>GitHub: push to PR head ref
Runner->>GitHub: post PR comment listing committed commands
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Code has been automatically formatted The code in this PR has been formatted using git pull origin clarify-commit-message |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-auto-commit.yaml (1)
100-107: Update the PR comment to reflect all auto-formatting steps.The comment message only mentions
cargo fmt --all, but the workflow now potentially applies multiple types of changes (cargo fmt, ruff format, ruff check import sorting, and generate_opcode_metadata.py). This could mislead contributors about what was actually changed.💬 Proposed fix for the PR comment message
message: | **Code has been automatically formatted** - The code in this PR has been formatted using `cargo fmt --all`. + The code in this PR has been automatically formatted and/or updated. + This may include: `cargo fmt --all`, `ruff format`, import sorting, and generated code updates. Please pull the latest changes before pushing again: ```bash git pull origin ${{ github.event.pull_request.head.ref }}
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pr-auto-commit.yamlscripts/fix_test.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Follow PEP 8 style for custom Python code
Use ruff for linting Python code
Files:
scripts/fix_test.py
**/*test*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*test*.py: NEVER comment out or delete any test code lines except for removing@unittest.expectedFailuredecorators and upper TODO comments
NEVER modify test assertions, test logic, or test data in test files
When a test cannot pass due to missing language features, keep it as expectedFailure and document the reason; do not comment it out
The only acceptable modifications to test files are: (1) Removing@unittest.expectedFailuredecorators and the upper TODO comments when tests actually pass, and (2) Adding@unittest.expectedFailuredecorators when tests cannot be fixed
Files:
scripts/fix_test.py
🧠 Learnings (5)
📓 Common learnings
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 5932
File: .github/workflows/comment-commands.yml:18-24
Timestamp: 2025-07-10T10:08:43.330Z
Learning: In GitHub Actions workflows for the RustPython project, the maintainer ShaharNaveh prefers to keep workflows simple and doesn't mind if steps fail when the desired state is already achieved (e.g., user already assigned to an issue). Avoid suggesting complex error handling for edge cases they don't consider problematic.
📚 Learning: 2025-08-30T14:40:05.858Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*test*.py : The only acceptable modifications to test files are: (1) Removing `unittest.expectedFailure` decorators and the upper TODO comments when tests actually pass, and (2) Adding `unittest.expectedFailure` decorators when tests cannot be fixed
Applied to files:
scripts/fix_test.py
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Follow the default rustfmt code style by running `cargo fmt` to format Rust code
Applied to files:
.github/workflows/pr-auto-commit.yaml
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Always run clippy to lint code (`cargo clippy`) before completing tasks and fix any warnings or lints introduced by changes
Applied to files:
.github/workflows/pr-auto-commit.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (8)
scripts/fix_test.py (1)
30-30: LGTM! Import sorting aligns with the new workflow.This alphabetical reordering of imports is consistent with the
ruff check --select I --fixstep introduced in the pr-auto-commit workflow..github/workflows/pr-auto-commit.yaml (7)
37-40: LGTM! Explicit git configuration improves clarity.Adding this dedicated step ensures consistent committer identity for all auto-commits and makes the workflow more maintainable.
46-49: LGTM! Conditional commit achieves the PR objective.The workflow now only creates a "cargo fmt" commit when formatting actually makes changes, addressing the core issue described in the PR.
57-63: LGTM! Separate commit for ruff format improves clarity.This separate step with its own conditional commit allows distinguishing between formatting changes and import sorting changes in the commit history.
65-71: LGTM! Import sorting as a separate step provides clarity.The separate commit for import sorting makes it clear what type of auto-formatting was applied, and explains the changes in scripts/fix_test.py.
73-79: LGTM! Separate commit for code generation is appropriate.The distinct "Auto-generate" prefix in the commit message clearly differentiates generated code changes from formatting changes.
84-84: LGTM! Improved change detection logic.Comparing HEAD to the original PR head SHA correctly detects whether any of the conditional formatting/generation steps created commits, which is more robust than checking the working directory state.
90-93: LGTM! Step name accurately reflects its purpose.The renamed step correctly indicates it only pushes commits made by previous steps, not creating new commits itself.
There was a problem hiding this comment.
Can you please adjust the message at line 103?
There was a problem hiding this comment.
Oh, I missed it. I fixed it! Thanks for letting me know it 🙏🏻
61bc77e to
a5a6ca1
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/pr-auto-commit.yaml:
- Around line 95-99: The git push step uses an unquoted expression `${{
github.event.pull_request.head.ref }}` which actionlint flags for possible
script injection; fix by passing the PR ref via an environment variable and
quoting it in the push command: set an env key (e.g., PR_REF) to `${{
github.event.pull_request.head.ref }}` on the "Push formatting changes" step and
replace the direct expression in the run command with a quoted reference to that
env var (e.g., git push origin HEAD:"${PR_REF}"), ensuring the ref is properly
quoted to prevent shell injection.
- Around line 77-85: The "Run generate_opcode_metadata.py" step runs a
PR-controlled script and can exfiltrate AUTO_COMMIT_PAT for forked PRs; guard
that step so it does not run for forked repositories by adding a condition that
checks the PR head repo is not a fork (e.g., use
github.event.pull_request.head.repo.fork == false) before executing python
scripts/generate_opcode_metadata.py and the git commit logic, leaving the step
intact for non-fork PRs only.
- Around line 37-42: The workflow currently uses echo "" to initialize
/tmp/committed_commands.txt which writes a leading blank line; replace that echo
call with a true truncation/no-op redirection so the file is empty without a
newline (e.g., use a plain shell redirection or printf with an empty string) and
keep the file name /tmp/committed_commands.txt unchanged.
- Around line 43-85: The commit steps currently stage with git add -u (which
ignores untracked files) but gate on git status --porcelain (which sees
untracked), causing git commit to fail when only new files exist; for each of
the four commit blocks (the cargo fmt, ruff format, ruff check --select I --fix,
and generate_opcode_metadata.py blocks) replace git add -u with git add -A so
untracked files are included before committing, ensuring git commit succeeds
when new files were created.
🧹 Nitpick comments (1)
.github/workflows/pr-auto-commit.yaml (1)
86-94:has_changesdetection is OK if all changes are committed; consider asserting a clean working tree.
This check only detects new commits (HEAD change). If anything leaves the tree dirty, the workflow will silently skip pushing.Suggestion: fail fast if the working tree isn’t clean before computing
has_changes.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pr-auto-commit.yamlscripts/fix_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/fix_test.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 5932
File: .github/workflows/comment-commands.yml:18-24
Timestamp: 2025-07-10T10:08:43.330Z
Learning: In GitHub Actions workflows for the RustPython project, the maintainer ShaharNaveh prefers to keep workflows simple and doesn't mind if steps fail when the desired state is already achieved (e.g., user already assigned to an issue). Avoid suggesting complex error handling for edge cases they don't consider problematic.
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to **/*.rs : Follow the default rustfmt code style by running `cargo fmt` to format Rust code
Applied to files:
.github/workflows/pr-auto-commit.yaml
🪛 actionlint (1.7.10)
.github/workflows/pr-auto-commit.yaml
97-97: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (1)
.github/workflows/pr-auto-commit.yaml (1)
100-107: Nice: PR comment now reflects what actually got committed.
The recorded command list makes the message accurate and reduces confusion.Also applies to: 113-117
| - name: Configure git | ||
| run: | | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| echo "" > /tmp/committed_commands.txt | ||
There was a problem hiding this comment.
Avoid writing a leading blank line to committed_commands.txt.
echo "" > ... creates an initial empty line, which will show up in the PR comment output.
Proposed fix
- name: Configure git
run: |
git config user.name "github-actions[bot]"
git config user.email "github-actions[bot]@users.noreply.github.com"
- echo "" > /tmp/committed_commands.txt
+ : > /tmp/committed_commands.txt🤖 Prompt for AI Agents
In @.github/workflows/pr-auto-commit.yaml around lines 37 - 42, The workflow
currently uses echo "" to initialize /tmp/committed_commands.txt which writes a
leading blank line; replace that echo call with a true truncation/no-op
redirection so the file is empty without a newline (e.g., use a plain shell
redirection or printf with an empty string) and keep the file name
/tmp/committed_commands.txt unchanged.
| - name: Run cargo fmt | ||
| run: | | ||
| echo "Running cargo fmt --all on PR #${{ github.event.pull_request.number }}" | ||
| cargo fmt --all | ||
| if [ -n "$(git status --porcelain)" ]; then | ||
| git add -u | ||
| git commit -m "Auto-format: cargo fmt --all" | ||
| echo "- \`cargo fmt --all\`" >> /tmp/committed_commands.txt | ||
| fi | ||
| - name: Install ruff | ||
| uses: astral-sh/ruff-action@57714a7c8a2e59f32539362ba31877a1957dded1 # v3.5.1 | ||
| with: | ||
| version: "0.14.9" | ||
| args: "--version" | ||
|
|
||
| - run: ruff format | ||
| - run: ruff check --select I --fix | ||
| - run: python scripts/generate_opcode_metadata.py | ||
| - name: Run ruff format | ||
| run: | | ||
| ruff format | ||
| if [ -n "$(git status --porcelain)" ]; then | ||
| git add -u | ||
| git commit -m "Auto-format: ruff format" | ||
| echo "- \`ruff format\`" >> /tmp/committed_commands.txt | ||
| fi | ||
| - name: Configure git | ||
| - name: Run ruff check import sorting | ||
| run: | | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| ruff check --select I --fix | ||
| if [ -n "$(git status --porcelain)" ]; then | ||
| git add -u | ||
| git commit -m "Auto-format: ruff check --select I --fix" | ||
| echo "- \`ruff check --select I --fix\`" >> /tmp/committed_commands.txt | ||
| fi | ||
| - name: Run generate_opcode_metadata.py | ||
| run: | | ||
| python scripts/generate_opcode_metadata.py | ||
| if [ -n "$(git status --porcelain)" ]; then | ||
| git add -u | ||
| git commit -m "Auto-generate: generate_opcode_metadata.py" | ||
| echo "- \`python scripts/generate_opcode_metadata.py\`" >> /tmp/committed_commands.txt | ||
| fi | ||
There was a problem hiding this comment.
Fix conditional-commit staging: git add -u can make git commit fail.
You gate on git status --porcelain (which includes untracked files), but then stage with git add -u (which excludes untracked). If the only changes are new files, git commit fails and the job aborts.
Proposed fix (apply to all 4 commit blocks)
- name: Run cargo fmt
run: |
echo "Running cargo fmt --all on PR #${{ github.event.pull_request.number }}"
cargo fmt --all
if [ -n "$(git status --porcelain)" ]; then
- git add -u
- git commit -m "Auto-format: cargo fmt --all"
+ git add -A
+ if ! git diff --cached --quiet; then
+ git commit -m "Auto-format: cargo fmt --all"
+ fi
echo "- \`cargo fmt --all\`" >> /tmp/committed_commands.txt
fi - name: Run ruff format
run: |
ruff format
if [ -n "$(git status --porcelain)" ]; then
- git add -u
- git commit -m "Auto-format: ruff format"
+ git add -A
+ if ! git diff --cached --quiet; then
+ git commit -m "Auto-format: ruff format"
+ fi
echo "- \`ruff format\`" >> /tmp/committed_commands.txt
fi - name: Run ruff check import sorting
run: |
ruff check --select I --fix
if [ -n "$(git status --porcelain)" ]; then
- git add -u
- git commit -m "Auto-format: ruff check --select I --fix"
+ git add -A
+ if ! git diff --cached --quiet; then
+ git commit -m "Auto-format: ruff check --select I --fix"
+ fi
echo "- \`ruff check --select I --fix\`" >> /tmp/committed_commands.txt
fi - name: Run generate_opcode_metadata.py
run: |
python scripts/generate_opcode_metadata.py
if [ -n "$(git status --porcelain)" ]; then
- git add -u
- git commit -m "Auto-generate: generate_opcode_metadata.py"
+ git add -A
+ if ! git diff --cached --quiet; then
+ git commit -m "Auto-generate: generate_opcode_metadata.py"
+ fi
echo "- \`python scripts/generate_opcode_metadata.py\`" >> /tmp/committed_commands.txt
fi🤖 Prompt for AI Agents
In @.github/workflows/pr-auto-commit.yaml around lines 43 - 85, The commit steps
currently stage with git add -u (which ignores untracked files) but gate on git
status --porcelain (which sees untracked), causing git commit to fail when only
new files exist; for each of the four commit blocks (the cargo fmt, ruff format,
ruff check --select I --fix, and generate_opcode_metadata.py blocks) replace git
add -u with git add -A so untracked files are included before committing,
ensuring git commit succeeds when new files were created.
| - name: Run generate_opcode_metadata.py | ||
| run: | | ||
| python scripts/generate_opcode_metadata.py | ||
| if [ -n "$(git status --porcelain)" ]; then | ||
| git add -u | ||
| git commit -m "Auto-generate: generate_opcode_metadata.py" | ||
| echo "- \`python scripts/generate_opcode_metadata.py\`" >> /tmp/committed_commands.txt | ||
| fi | ||
There was a problem hiding this comment.
Critical: pull_request_target + running a PR-controlled Python script can exfiltrate AUTO_COMMIT_PAT.
Because you check out the PR’s head SHA and then run python scripts/generate_opcode_metadata.py, a fork PR can modify that script and execute arbitrary code with access to secrets.
Minimal mitigation (keeps the workflow simple): skip this step for fork PRs.
Proposed fix
- name: Run generate_opcode_metadata.py
+ if: ${{ github.event.pull_request.head.repo.full_name == github.repository }}
run: |
python scripts/generate_opcode_metadata.py
if [ -n "$(git status --porcelain)" ]; then
git add -u
git commit -m "Auto-generate: generate_opcode_metadata.py"
echo "- \`python scripts/generate_opcode_metadata.py\`" >> /tmp/committed_commands.txt
fi🤖 Prompt for AI Agents
In @.github/workflows/pr-auto-commit.yaml around lines 77 - 85, The "Run
generate_opcode_metadata.py" step runs a PR-controlled script and can exfiltrate
AUTO_COMMIT_PAT for forked PRs; guard that step so it does not run for forked
repositories by adding a condition that checks the PR head repo is not a fork
(e.g., use github.event.pull_request.head.repo.fork == false) before executing
python scripts/generate_opcode_metadata.py and the git commit logic, leaving the
step intact for non-fork PRs only.
| - name: Push formatting changes | ||
| if: steps.check-changes.outputs.has_changes == 'true' | ||
| run: | | ||
| git add -u | ||
| git commit -m "Auto-format: cargo fmt --all" | ||
| git push origin HEAD:${{ github.event.pull_request.head.ref }} | ||
There was a problem hiding this comment.
Fix potential script-injection via ${{ github.event.pull_request.head.ref }} (actionlint finding).
Pass via env and quote the ref in the git push.
Proposed fix
- name: Push formatting changes
if: steps.check-changes.outputs.has_changes == 'true'
+ env:
+ PR_HEAD_REF: ${{ github.event.pull_request.head.ref }}
run: |
- git push origin HEAD:${{ github.event.pull_request.head.ref }}
+ git push origin "HEAD:refs/heads/${PR_HEAD_REF}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Push formatting changes | |
| if: steps.check-changes.outputs.has_changes == 'true' | |
| run: | | |
| git add -u | |
| git commit -m "Auto-format: cargo fmt --all" | |
| git push origin HEAD:${{ github.event.pull_request.head.ref }} | |
| - name: Push formatting changes | |
| if: steps.check-changes.outputs.has_changes == 'true' | |
| env: | |
| PR_HEAD_REF: ${{ github.event.pull_request.head.ref }} | |
| run: | | |
| git push origin "HEAD:refs/heads/${PR_HEAD_REF}" | |
🧰 Tools
🪛 actionlint (1.7.10)
97-97: "github.event.pull_request.head.ref" is potentially untrusted. avoid using it directly in inline scripts. instead, pass it through an environment variable. see https://docs.github.com/en/actions/reference/security/secure-use#good-practices-for-mitigating-script-injection-attacks for more details
(expression)
🤖 Prompt for AI Agents
In @.github/workflows/pr-auto-commit.yaml around lines 95 - 99, The git push
step uses an unquoted expression `${{ github.event.pull_request.head.ref }}`
which actionlint flags for possible script injection; fix by passing the PR ref
via an environment variable and quoting it in the push command: set an env key
(e.g., PR_REF) to `${{ github.event.pull_request.head.ref }}` on the "Push
formatting changes" step and replace the direct expression in the run command
with a quoted reference to that env var (e.g., git push origin
HEAD:"${PR_REF}"), ensuring the ref is properly quoted to prevent shell
injection.
* Clarify commit message from pr-auto-commit * Auto-format: cargo fmt --all --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
The existing pr-auto-commit workflow did not perform separate commits, so even though
cargo fmt --allwasn't actually executed, the commit message remained asAuto-format: cargo fmt --all, creating some potential confusion. This improves that issue.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.