Clarify commit message from pr-auto-commit - #6706
Conversation
Sorry, something went wrong.
📝 Walkthrough
WalkthroughUpdates 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 scripts/fix_test.py. 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)
✏️ 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using cargo fmt --all. git pull origin clarify-commit-message |
Sorry, something went wrong.
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 messagemessage: | **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 }}
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 29bb8b4 and 61bc77e.
📒 Files selected for processing (2)- .github/workflows/pr-auto-commit.yaml
- scripts/fix_test.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
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*test*.py: NEVER comment out or delete any test code lines except for removing @unittest.expectedFailure decorators 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.expectedFailure decorators and the upper TODO comments when tests actually pass, and (2) Adding @unittest.expectedFailure decorators when tests cannot be fixed
Files:
- scripts/fix_test.py
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.
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
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
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
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
- 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
scripts/fix_test.py (1).github/workflows/pr-auto-commit.yaml (7)30-30: LGTM! Import sorting aligns with the new workflow.
This alphabetical reordering of imports is consistent with the ruff check --select I --fix step introduced in the pr-auto-commit workflow.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Can you please adjust the message at line 103?
Sorry, something went wrong.
There was a problem hiding this comment.
Oh, I missed it. I fixed it! Thanks for letting me know it 🙏🏻
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agentsIn @.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.
.github/workflows/pr-auto-commit.yaml (1)📜 Review details86-94: has_changes detection 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.
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 61bc77e and 0ec2add.
📒 Files selected for processing (2)- .github/workflows/pr-auto-commit.yaml
- scripts/fix_test.py
- scripts/fix_test.py
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.
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
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
.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
Sorry, something went wrong.
| - 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.
⚠️ Potential issue | 🟡 Minor
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.
- 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.txtIn @.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.
Sorry, something went wrong.
| - 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.
⚠️ Potential issue | 🟠 Major
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.
- 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
fiIn @.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.
Sorry, something went wrong.
| - 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.
⚠️ Potential issue | 🔴 Critical
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
fiIn @.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.
Sorry, something went wrong.
| - 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.
⚠️ Potential issue | 🟠 Major
Fix potential script-injection via ${{ github.event.pull_request.head.ref }} (actionlint finding).
Pass via env and quote the ref in the git push.
- 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}"‼️ 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}" | |
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 AgentsIn @.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.
Sorry, something went wrong.
ShaharNaveh
left a comment
There was a problem hiding this comment.
looks great!
Sorry, something went wrong.
youknowone
left a comment
There was a problem hiding this comment.
Thank you!
Sorry, something went wrong.
* 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 --all wasn't actually executed, the commit message remained as Auto-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.