Update CI auto-formate #6237
Update CI auto-formate #6237
Conversation
Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>
WalkthroughCI workflows updated: rustfmt removed from the lint job; a new PR-targeted auto-format workflow runs Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PR as Pull Request
participant GH as GitHub Actions
participant Core as Core Jobs (tests, clippy)
participant AF as Auto-format Job
participant Git as Git (commit/push)
participant Comment as PR Commenter
PR->>GH: open/update (pull_request_target)
GH->>Core: run tests & clippy (rustfmt removed)
Core-->>GH: status results
GH->>AF: start auto-format (if head commit != "[skip ci]" and required checks passed)
AF->>AF: setup toolchain (rustfmt), run `cargo fmt --all`
alt formatting changes detected
AF->>Git: commit with "[skip ci]" and push to PR branch
Git-->>AF: push success
AF->>Comment: post PR comment with auto-format details
else no changes
AF-->>GH: exit (already formatted)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
|
I don't know why this failing but I tested on my fork with the help of different account too and its working. |
Co-authored-by: fanninpm <luxverdans@sbcglobal.net>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/ci.yaml (2)
452-462: Job dependency chain creates significant delay before formatting feedback.The job depends on 7 prior jobs, which means formatting comments won't appear until all of those jobs finish. Consider whether this is intentional (to avoid conflicts) or whether an earlier checkpoint would be preferable.
481-493: Usegit add -Ato capture all changes (including untracked files).Line 487 uses
git add -u, which only stages modifications to already-tracked files. Whilecargo fmttypically doesn't create new files, usinggit add -Aorgit add .is more defensive:- git add -u + git add -AAdditionally, consider checking the git push exit code to ensure the push succeeded before setting
formatted=true.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yaml(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Format Rust code with the default rustfmt style (run `cargo fmt`)
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Run clippy and fix any warnings or lints introduced by your changes
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Run clippy and fix any warnings or lints introduced by your changes
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Format Rust code with the default rustfmt style (run `cargo fmt`)
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to extra_tests/**/*.py : Use ruff to lint custom Python code
Applied to files:
.github/workflows/ci.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). (11)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (4)
.github/workflows/ci.yaml (4)
309-316: Lint job updates look good.Removing rustfmt from the lint step and dedicating it to the new auto_format_commit job is a clean separation of concerns.
465-475: Checkout and Rust setup are correct.Line 469 correctly uses
${{ github.event.pull_request.head.ref || github.ref_name }}to handle both PR and push events—good alignment with the past suggestion from fanninpm.
476-480: Cargo fmt command is standard and correct.
495-501: PR comment logic is sound.The conditional on line 496 correctly gates the comment to PR events, and the message is clear.
| pull-requests: write | ||
| name: Auto-format code | ||
| runs-on: ubuntu-latest | ||
| if: ${{ !contains(github.event.head_commit.message, '[skip ci]') }} |
There was a problem hiding this comment.
🧩 Analysis chain
Critical: Condition check fails for pull_request events.
github.event.head_commit is only available on push events, not on pull_request events. For PRs (which are triggered on lines 4–5), this field will be null, causing contains() to behave unpredictably. The job may run unconditionally on PR events.
This is why the author reports CI passing on their fork but failing here—depending on how GitHub Actions handles contains(null, ...), the condition may be silently broken for PRs.
For pull_request events, you need to check the base branch's latest commit or use an alternative approach. Replace the condition with:
- if: ${{ !contains(github.event.head_commit.message, '[skip ci]') }}
+ if: ${{ github.event_name != 'pull_request' || !contains(github.event.pull_request.head.sha, 'skip-not-applicable') }}Or, if you want to reliably check the commit message for PRs, you'll need to fetch it explicitly in a step. For now, a safer fallback is:
+ if: ${{ github.event_name == 'push' || github.event_name == 'pull_request' }}(This allows the job to run on both; the git check inside the step will prevent unnecessary commits.)
Alternatively, consider moving the [skip ci] check inside the "Commit and push" step (lines 481–493) as a regular bash conditional, where you always have access to the commit message.
I'll verify the review comment by examining the workflow file and checking GitHub Actions documentation.
[/run_scripts]
[/web_search]
🏁 Script executed:
#!/bin/bash
# Inspect the workflow file around line 459
head -n 470 .github/workflows/ci.yaml | tail -n 30Length of output: 1166
🏁 Script executed:
#!/bin/bash
# Show the workflow on/trigger section to see which events activate it
head -n 20 .github/workflows/ci.yamlLength of output: 666
🏁 Script executed:
#!/bin/bash
# Find all jobs and their conditionals to understand context
rg -nP "^\s+if:|^\s+jobs:|^\s+[a-z_]+:" .github/workflows/ci.yaml | head -n 50Length of output: 2131
🌐 Web query:
GitHub Actions github.event.head_commit pull_request vs push events
💡 Result:
Short answer
- github.event.head_commit is provided by the push webhook/payload (use in on: push). It contains the head commit object for that push. [1]
- The pull_request event does NOT include github.event.head_commit. For pull requests use github.event.pull_request.head (e.g. github.event.pull_request.head.sha, github.event.pull_request.head.ref) or inspect github.event.pull_request.commits via the API. The pull_request run context is different (it may run in a merge/merge-commit context vs. the head branch depending on the event type). Use pull_request_target if you need the workflow to run in the base context. [1][2][3]
Examples
- Push: commit message => ${{ github.event.head_commit.message }}. [1][3]
- Pull request: head SHA => ${{ github.event.pull_request.head.sha }}; head ref => ${{ github.event.pull_request.head.ref }}. Fetch commits/changes with the REST API or checkout more history if needed. [1][3]
References
[1] GitHub Actions events (push/pull_request) docs.
[2] pull_request_target vs pull_request notes (security / context).
[3] Community/StackOverflow discussion showing head_commit available on push but not on pull_request.
Critical: github.event.head_commit is unavailable on pull_request events and will cause the condition to fail.
The workflow triggers on both push and pull_request events (lines 4–5), but line 459 unconditionally accesses github.event.head_commit.message, which only exists on push events, not pull_request events. On PRs, this field is null, causing contains() to behave unpredictably—the job will likely run unconditionally regardless of any [skip ci] marker.
Use the same pattern as other jobs in this workflow (lines 111, 176): add a conditional that checks the event type and uses the appropriate context field. For PRs, use github.event.pull_request.head.sha or other PR-specific fields, or check labels instead. For example:
- if: ${{ !contains(github.event.head_commit.message, '[skip ci]') }}
+ if: ${{ github.event_name == 'push' || github.event_name != 'push' }}Or safely move the commit message check into the bash step where you always have access to the local git history.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/ci.yaml around line 459: the job condition currently reads
`contains(github.event.head_commit.message, '[skip ci]')` which fails for
pull_request events because github.event.head_commit is only present on push
events; change the condition to handle both event types by checking
github.event_name first and using the correct field for each (for push use
github.event.head_commit.message, for pull_request use a PR-specific field such
as github.event.pull_request.title or body or check PR labels), or alternatively
remove the expression and perform the skip-detection inside a shell step that
reads the commit/PR message via git, ensuring the job no longer directly
accesses head_commit when the event is pull_request.
|
@coderabbitai Why does CI fail? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8609 🏁 Script executed: Length of output: 23955 @fanninpm The CI is failing because of how the Root CauseThe error in the logs shows: This happens because:
Why It Worked on the ForkWhen @YashSuthar983 tested this on their own fork, it worked because:
Additional Problem: Write PermissionsEven if the checkout issue were fixed, there's a security restriction: GitHub Actions from forked PRs don't have write permissions to the base repository by default. The workflow tries to push formatting changes back to the PR branch, which would fail for forked PRs. SolutionsFor this auto-format workflow to work properly with forked PRs, you have a few options:
The simplest fix would be option #2 - only auto-format for PRs from branches in the main repository, not from forks. 🧠 Learnings used |
|
@fanninpm i am still confused as when i tested with my different account where i fork my fork then make a pr ,there it works . |
Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>
.github/workflows/ci.yaml
Outdated
| uses: actions/checkout@v5 | ||
| with: | ||
| fetch-depth: 0 | ||
| ref: ${{ github.event.pull_request.head.ref || github.ref_name }} |
There was a problem hiding this comment.
| ref: ${{ github.event.pull_request.head.ref || github.ref_name }} |
@coderabbitai @YashSuthar983 is reporting that this doesn't work on their fork. Why is that?
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Actions push to fork pull request branch permissions
💡 Result:
Short answer: By default you cannot push to a pull request branch coming from a fork because GitHub gives workflows triggered by pull_request from forks a read-only GITHUB_TOKEN and does not expose repository secrets. To push to a forked-PR branch you must use an explicit token/approach with elevated permissions or change repo/enterprise fork policies. Key options:
-
Default behavior: pull requests from forks get a read-only GITHUB_TOKEN and no secrets, so workflows cannot commit/push to the target repo or push back to the fork. [1][2][3]
-
Use pull_request_target (runs in the base repo context) plus a stored token (repo-scoped PAT or other secret) to perform writes — but be careful: pull_request_target runs with base repo code/secrets and can introduce security risks (validate inputs!). [4][3]
-
Use a personal access token (PAT) or GitHub App token (stored as a secret) for git push operations (or for actions like peter-evans/create-pull-request with push-to-fork). That token must have repo:contents/workflows write scopes as needed; store it in Secrets and use it instead of GITHUB_TOKEN. [4][1]
-
Enterprise/repo settings: in private/internal repos admins can allow “Send write tokens to workflows from pull requests” or “Send secrets to workflows from pull requests” so fork PR workflows can get write GITHUB_TOKEN or secrets — this is controlled at enterprise/org/repo Actions settings. [2][5]
Useful docs/readme:
- GitHub Actions security / GITHUB_TOKEN restrictions for fork PRs. [3][5]
- create-pull-request action (supports push-to-fork and PAT usage). [4]
- GitHub repo/enterprise Actions settings (approval, send-write-tokens, send-secrets). [2]
References
[1] Synacktiv write-up on GITHUB_TOKEN restrictions for PRs from forks.
[2] GitHub Docs — Managing Actions settings for a repository (fork PR policies, send write tokens/secrets).
[3] GitHub Security Lab — pull_request vs pull_request_target and risks.
[4] peter-evans/create-pull-request action docs (push-to-fork, token options).
@fanninpm The workflow works on @YashSuthar983's fork but not for pull requests from forks to this repository due to GitHub Actions security restrictions:
Why it fails for fork PRs:
When a pull request comes from a fork, GitHub Actions runs with a read-only GITHUB_TOKEN and does not expose repository secrets for security reasons. This prevents the workflow from pushing commits back to the fork's branch.
Why it worked on their fork:
When they tested on their own fork, the PR was within the same repository (not a cross-repository fork PR), so the workflow had write permissions and could push successfully.
The fundamental issue:
The auto_format_commit job tries to git push formatting changes (line 489), which requires write permissions that fork PRs don't have by default.
Potential solutions:
-
Skip fork PRs (simplest, safest):
if: ${{ !contains(github.event.head_commit.message, '[skip ci]') && github.event.pull_request.head.repo.full_name == github.repository }}
-
Use
pull_request_targetwith a PAT (security risk):
Switch topull_request_targettrigger and use a Personal Access Token stored in secrets, but this has security implications as it runs untrusted code with elevated privileges. -
Post a comment instead: For fork PRs, skip the push and just post a comment asking the contributor to run
cargo fmt --alllocally.
Given the RustPython project context, I'd recommend option 1 (skip fork PRs) since most contributors can run cargo fmt locally, and it avoids the security risks of elevated permissions.
🧠 Learnings used
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-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Format Rust code with the default rustfmt style (run `cargo fmt`)
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Run clippy and fix any warnings or lints introduced by your changes
There was a problem hiding this comment.
hy see
let my fork be A and my friends fork which is fork of A is B.
so i make a pr from B to A , there auto-format works.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
let my fork be A and my friends fork which is fork of A is B.
so i make a pr from B to A , there auto-format works.
Let's say your fork A already has branch A:a, and your friend's fork B has branch B:a. You're telling GitHub Actions to look for a branch a, and it finds A:a and runs the action there. Is this what you intended?
There was a problem hiding this comment.
not exactly i want to checkout the head of pr branch then make a commit on top of it as i read that we have permission to a commit on a pr if contributor checked the allow changes from maintainer .
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/ci.yaml (1)
459-459: CRITICAL: Condition fails on pull_request events.Line 459 accesses
github.event.head_commit.message, which is only available onpushevents. The workflow triggers on bothpushandpull_request(lines 2–5), so this condition is broken for PR events. For PRs,github.event.head_commitisnull, causingcontains(null, '[skip ci]')to behave unpredictably—the job will likely run unconditionally.This matches the root cause identified in the previous review: the workflow works on your fork (where you have permissions) but fails here because the condition doesn't distinguish event types. Per previous review recommendations, the simplest fix is to skip auto-format for fork PRs by checking the event type:
- if: ${{ !contains(github.event.head_commit.message, '[skip ci]') }} + if: ${{ github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) }}This skips the job for fork PRs (where you don't have write permissions anyway) and checks the commit message only on push events where the context is available.
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
459-459: Align skip condition with other jobs.Other jobs in this workflow use
!contains(github.event.pull_request.labels.*.name, 'skip:ci')to determine whether to run (lines 111, 176, 237, etc.). The new job uses a different mechanism: commit message matching. For consistency and predictability, consider aligning the auto-format job to use the same label-based skip pattern already established throughout the workflow:- if: ${{ !contains(github.event.head_commit.message, '[skip ci]') }} + if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip:ci') }}This ensures that all jobs respond to the same
skip:cilabel, making the workflow behavior uniform and easier to reason about.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ci.yaml(2 hunks)
🧰 Additional context used
🧠 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.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Format Rust code with the default rustfmt style (run `cargo fmt`)
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Run clippy and fix any warnings or lints introduced by your changes
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Format Rust code with the default rustfmt style (run `cargo fmt`)
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-07-10T10:08:43.330Z
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.
Applied to files:
.github/workflows/ci.yaml
📚 Learning: 2025-09-11T05:06:59.971Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to extra_tests/**/*.py : Use ruff to lint custom Python code
Applied to files:
.github/workflows/ci.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). (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (1)
.github/workflows/ci.yaml (1)
309-316: Lint job refactor looks good.Removing rustfmt from the lint job and moving it to a separate auto-format job is a clean separation of concerns. The lint job retains clippy for static analysis, which is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr-auto-commit.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Format Rust code with the default rustfmt style (run `cargo fmt`)
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Run clippy and fix any warnings or lints introduced by your changes
🪛 actionlint (1.7.8)
.github/workflows/pr-auto-commit.yaml
69-69: "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). (11)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (2)
.github/workflows/pr-auto-commit.yaml (2)
78-92: Well-designed PR comment notification.The PR comment step is appropriately gated to only run when formatting changes are detected, and the message clearly explains what happened and how to pull the latest changes. This is good UX.
37-46: ****The
poseidon/wait-for-status-checksaction waits until all watched checks succeed (or are skipped) or one fails, meaning it validates success and exits with failure if any check fails. The workflow will not proceed past that step if checks don't pass. The echo at lines 45–46 merely confirms that the validation passed; no additional verification is needed.Likely an incorrect or invalid review comment.
| auto_format: | ||
| if: | | ||
| !contains(github.event.pull_request.labels.*.name, 'skip:ci') && | ||
| !contains(github.event.pull_request.head.sha, '[skip ci]') | ||
| permissions: | ||
| contents: write | ||
| pull-requests: write | ||
| checks: read | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 60 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Add guard to skip auto-format for forked PRs.
Per the PR comments and static analysis, this workflow fails when triggered by PRs from forked repositories. GitHub Actions running in the base repository cannot push to a fork's branch, even with pull_request_target and write permissions. The recommended fix is to skip this job for forked PRs.
Add a check to the condition to verify the PR originates from the base repository:
auto_format:
if: |
!contains(github.event.pull_request.labels.*.name, 'skip:ci') &&
!contains(github.event.pull_request.head.sha, '[skip ci]') &&
+ github.event.pull_request.head.repo.full_name == github.repositoryThis ensures the job only runs for PRs against the main repository, not forked PRs.
🏁 Script executed:
#!/bin/bash
# Check if file exists and examine lines 16-26 plus surrounding context
if [ -f ".github/workflows/pr-auto-commit.yaml" ]; then
echo "=== File exists, showing lines 1-35 ==="
head -35 .github/workflows/pr-auto-commit.yaml
else
echo "=== File does not exist ==="
find . -name "*pr-auto-commit*" -type f 2>/dev/null || echo "No matching files found"
fiLength of output: 924
🏁 Script executed:
#!/bin/bash
# Search for any existing references to github.repository or fork-related conditions
if [ -f ".github/workflows/pr-auto-commit.yaml" ]; then
echo "=== Searching for fork/repository guards ==="
grep -n "github.repository\|\.repo\.full_name\|forked" .github/workflows/pr-auto-commit.yaml || echo "No fork guards found"
echo ""
echo "=== Full condition section ==="
sed -n '16,26p' .github/workflows/pr-auto-commit.yaml
fiLength of output: 452
🏁 Script executed:
#!/bin/bash
# Check if pull_request_target is used (which would require fork handling)
if [ -f ".github/workflows/pr-auto-commit.yaml" ]; then
echo "=== Checking trigger event ==="
grep -n "on:\|pull_request" .github/workflows/pr-auto-commit.yaml | head -10
fiLength of output: 597
Add guard to skip auto-format for forked PRs.
The workflow uses pull_request_target to gain write permissions but then checks out and pushes to github.event.pull_request.head.repo.full_name (the fork's repository). For forked PRs, GitHub Actions in the base repository cannot push to the fork's branch, causing the workflow to fail.
Add a check to the condition to verify the PR originates from the base repository:
auto_format:
if: |
!contains(github.event.pull_request.labels.*.name, 'skip:ci') &&
!contains(github.event.pull_request.head.sha, '[skip ci]') &&
+ github.event.pull_request.head.repo.full_name == github.repositoryThis ensures the job only runs for PRs against the main repository, not forked PRs.
🤖 Prompt for AI Agents
.github/workflows/pr-auto-commit.yaml around lines 16-26: the job uses
pull_request_target and attempts to push to the PR head, which will fail for
forked PRs because the base repo cannot push to a fork; update the job's if
condition to also require the PR head repository to be the same as the base
repository (e.g. add a check that github.event.pull_request.head.repo.full_name
== github.repository or github.event.pull_request.head.repo.owner.login ==
github.repository_owner) and combine it with the existing checks using && so the
auto_format job only runs for PRs originating in the base repository.
| - name: Commit and push formatting changes | ||
| if: steps.check_changes.outputs.has_changes == 'true' | ||
| run: | | ||
| git config user.name "github-actions[bot]" | ||
| git config user.email "github-actions[bot]@users.noreply.github.com" | ||
| git add -u | ||
| git commit -m "Auto-format code [skip ci]" | ||
| git push origin HEAD:${{ github.event.pull_request.head.ref }} |
There was a problem hiding this comment.
Mitigate script injection risk: pass head.ref through environment variable.
Line 69 is flagged by actionlint for potential script injection. The github.event.pull_request.head.ref is directly interpolated into an inline script, which violates GitHub Actions security best practices. An untrusted PR ref could be crafted to inject arbitrary shell commands.
Pass the ref through an environment variable instead:
- name: Commit and push formatting changes
if: steps.check_changes.outputs.has_changes == 'true'
+ env:
+ HEAD_REF: ${{ github.event.pull_request.head.ref }}
run: |
git config user.name "github-actions[bot]"
git config user.email "github-actions[bot]@users.noreply.github.com"
git add -u
git commit -m "Auto-format code [skip ci]"
- git push origin HEAD:${{ github.event.pull_request.head.ref }}
+ git push origin HEAD:"$HEAD_REF"See GitHub's security guidance.
📝 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: Commit and push formatting changes | |
| if: steps.check_changes.outputs.has_changes == 'true' | |
| run: | | |
| git config user.name "github-actions[bot]" | |
| git config user.email "github-actions[bot]@users.noreply.github.com" | |
| git add -u | |
| git commit -m "Auto-format code [skip ci]" | |
| git push origin HEAD:${{ github.event.pull_request.head.ref }} | |
| - name: Commit and push formatting changes | |
| if: steps.check_changes.outputs.has_changes == 'true' | |
| env: | |
| HEAD_REF: ${{ github.event.pull_request.head.ref }} | |
| run: | | |
| git config user.name "github-actions[bot]" | |
| git config user.email "github-actions[bot]@users.noreply.github.com" | |
| git add -u | |
| git commit -m "Auto-format code [skip ci]" | |
| git push origin HEAD:"$HEAD_REF" |
🧰 Tools
🪛 actionlint (1.7.8)
69-69: "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
.github/workflows/pr-auto-commit.yaml around lines 67 to 76: the workflow
interpolates github.event.pull_request.head.ref directly into the inline shell
script causing actionlint/script-injection risk; set the PR head ref into an
environment variable (e.g., REF="${{ github.event.pull_request.head.ref }}") in
the step’s env and then use that env variable inside the run script (e.g., git
push origin HEAD:"$REF"), ensuring you reference the variable rather than
expanding the expression in the shell to mitigate injection.
|
@fanninpm @youknowone now i have done diff approach here i used pull_request_target as this has more permissions then workflow_run . tested in pr |
Signed-off-by: Yash Suthar <yashsuthar983@gmail.com>
40a778d to
012693f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/pr-auto-commit.yaml (2)
16-19:⚠️ Critical: Add fork guard to prevent workflow failure on forked PRs.The job condition lacks a check to ensure the PR originates from the base repository. When this workflow runs on PRs from forked repositories, GitHub Actions in the base repo cannot push to the fork's branch, causing the workflow to fail. This is why the workflow succeeds in the author's fork but fails in the main repository.
Add a guard to skip auto-format for forked PRs:
auto_format: if: | !contains(github.event.pull_request.labels.*.name, 'skip:ci') && !contains(github.event.pull_request.head.sha, '[skip ci]') && + github.event.pull_request.head.repo.full_name == github.repositoryThis ensures the job only runs for PRs opened against the main repository, not forked PRs.
67-76:⚠️ Major: Mitigate script injection risk by passing ref through environment variable.Line 76 directly interpolates
github.event.pull_request.head.refinto the inline shell script. An untrusted PR ref could be crafted to inject arbitrary shell commands. Per GitHub's security guidance, pass the ref through an environment variable instead:- name: Commit and push formatting changes if: steps.check_changes.outputs.has_changes == 'true' + env: + HEAD_REF: ${{ github.event.pull_request.head.ref }} run: | git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com" git add -u git commit -m "Auto-format code [skip ci]" - git push origin HEAD:${{ github.event.pull_request.head.ref }} + git push origin HEAD:"$HEAD_REF"
🧹 Nitpick comments (1)
.github/workflows/pr-auto-commit.yaml (1)
78-95: Minor: Consistent script variable handling in PR comment message.Line 94 also interpolates
${{ github.event.pull_request.head.ref }}directly. While this is in a message/documentation context (lower risk than the git push command), apply the same mitigation for consistency and to establish a pattern:- name: Comment on PR if: steps.check_changes.outputs.has_changes == 'true' + env: + HEAD_REF: ${{ github.event.pull_request.head.ref }} uses: marocchino/sticky-pull-request-comment@v2 with: number: ${{ github.event.pull_request.number }} message: | **Code has been automatically formatted** The code in this PR has been formatted using `cargo fmt`. The changes have been committed with `[skip ci]` to avoid triggering another CI run. **Triggered by commit:** `${{ github.event.pull_request.head.sha }}` **Last formatted:** ${{ github.event.pull_request.updated_at }} You may need to pull the latest changes before pushing again: ```bash - git pull origin ${{ github.event.pull_request.head.ref }} + git pull origin "$HEAD_REF" ```
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/pr-auto-commit.yaml(1 hunks)
🧰 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.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Format Rust code with the default rustfmt style (run `cargo fmt`)
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-09-11T05:06:59.971Z
Learning: Applies to **/*.rs : Run clippy and fix any warnings or lints introduced by your changes
📚 Learning: 2025-07-10T10:08:43.330Z
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.
Applied to files:
.github/workflows/pr-auto-commit.yaml
🪛 actionlint (1.7.8)
.github/workflows/pr-auto-commit.yaml
69-69: "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). (10)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
|
I'm a bit concerned about the formatting failure, but let's give it another try. |
|
Great, it looks working well! https://github.com/RustPython/RustPython/actions/runs/19235980885/job/54985900338?pr=6234 |
|
Thank you so much! 👍 |
|
okay, not good in this case: https://github.com/RustPython/RustPython/actions/runs/19236037205/job/54989070706?pr=6234 |
|
@youknowone i guess its failing because fork using old auto-commit job, |
After running all all test i have added a another which check the rust format and if its not correct it make a commit with
[skip ci] which tell github workflow to ignore this commit.
@youknowone now this should work. i tested again on my fork with different account for testing.
issue :#6204
Summary by CodeRabbit