Only run cargo check when rust code is changed#7572
Only run cargo check when rust code is changed#7572youknowone merged 1 commit intoRustPython:mainfrom
cargo check when rust code is changed#7572Conversation
📝 WalkthroughWalkthroughAdded a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 49-55: The merge-base step (id: merge_base) currently uses git
merge-base HEAD "origin/${BASE_REF}" which yields HEAD on push-to-main and makes
diffs empty; update the logic to use github.event.before as the base when
github.event_name == 'push' (fall back to origin/${BASE_REF} for pull_request)
so the computed sha reflects the previous commit for pushes; also fix the diff
pathspec that currently reads ':.github/workflows/ci.yaml' to be an exclusion by
changing it to ':!.github/workflows/ci.yaml' so the workflow file is excluded
like the other directories (adjust any step that evaluates rust_code or
cargo_check to use the corrected merge_base and pathspec).
- Around line 60-65: The git diff pathspec list in the conditional uses
':.github/workflows/ci.yaml' which incorrectly includes ci.yaml instead of
excluding it; update the pathspec in the git diff command (the list passed to
git diff --quiet "${MERGE_BASE}...HEAD") to use ':!.github/workflows/ci.yaml' so
ci.yaml is excluded like the other patterns (e.g., ':!Lib/**', ':!scripts/**').
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a8df71a2-7a77-4122-a124-5c01de5f42b0
📒 Files selected for processing (1)
.github/workflows/ci.yaml
| - name: Determine merge base | ||
| id: merge_base | ||
| run: | | ||
| sha=$(git merge-base HEAD "origin/${BASE_REF}") | ||
| echo "sha=${sha}" >> "$GITHUB_OUTPUT" | ||
| env: | ||
| BASE_REF: ${{ github.event.pull_request.base.ref || 'main' }} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verifies the push-to-main behavior that makes merge-base equal HEAD.
main_head="$(git rev-parse origin/main)"
mb="$(git merge-base "$main_head" origin/main)"
echo "origin/main HEAD: $main_head"
echo "merge-base(origin/main, origin/main): $mb"
if [ "$main_head" = "$mb" ]; then
echo "Confirmed: for push-to-main, merge-base equals HEAD, so diff HEAD...HEAD is empty."
fiRepository: RustPython/RustPython
Length of output: 285
🏁 Script executed:
cat -n .github/workflows/ci.yaml | sed -n '49,72p'Repository: RustPython/RustPython
Length of output: 1011
🏁 Script executed:
cat -n .github/workflows/ci.yaml | sed -n '37,75p'Repository: RustPython/RustPython
Length of output: 1595
🏁 Script executed:
cat -n .github/workflows/ci.yaml | sed -n '152,160p'Repository: RustPython/RustPython
Length of output: 377
Fix merge-base logic for push events and correct pathspec in diff filter; currently cargo_check never runs and change detection is broken.
Two issues prevent the change detection from working:
-
Push to main always skips
cargo_check: At line 52,git merge-base HEAD "origin/${BASE_REF}"resolves toHEADon push to main, making the diff empty andrust_codealways false. Usegithub.event.beforefor push events instead. -
Pathspec at line 64 is incorrect:
':.github/workflows/ci.yaml'includes only the workflow file (inverted logic). Should be':!.github/workflows/ci.yaml'to exclude it like the other directories.
Proposed fixes
For the merge-base step (lines 49-55):
- name: Determine merge base
id: merge_base
run: |
- sha=$(git merge-base HEAD "origin/${BASE_REF}")
+ if [ "${GITHUB_EVENT_NAME}" = "push" ] && [ -n "${BEFORE_SHA}" ]; then
+ sha="${BEFORE_SHA}"
+ else
+ sha=$(git merge-base HEAD "origin/${BASE_REF}")
+ fi
echo "sha=${sha}" >> "$GITHUB_OUTPUT"
env:
BASE_REF: ${{ github.event.pull_request.base.ref || 'main' }}
+ BEFORE_SHA: ${{ github.event.before }}For the pathspec (line 64):
if git diff --quiet "${MERGE_BASE}...HEAD" -- \
':!Lib/**' \
':!scripts/**' \
':!extra_tests/**' \
- ':.github/workflows/ci.yaml' \
+ ':!.github/workflows/ci.yaml' \📝 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: Determine merge base | |
| id: merge_base | |
| run: | | |
| sha=$(git merge-base HEAD "origin/${BASE_REF}") | |
| echo "sha=${sha}" >> "$GITHUB_OUTPUT" | |
| env: | |
| BASE_REF: ${{ github.event.pull_request.base.ref || 'main' }} | |
| - name: Determine merge base | |
| id: merge_base | |
| run: | | |
| if [ "${GITHUB_EVENT_NAME}" = "push" ] && [ -n "${BEFORE_SHA}" ]; then | |
| sha="${BEFORE_SHA}" | |
| else | |
| sha=$(git merge-base HEAD "origin/${BASE_REF}") | |
| fi | |
| echo "sha=${sha}" >> "$GITHUB_OUTPUT" | |
| env: | |
| BASE_REF: ${{ github.event.pull_request.base.ref || 'main' }} | |
| BEFORE_SHA: ${{ github.event.before }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yaml around lines 49 - 55, The merge-base step (id:
merge_base) currently uses git merge-base HEAD "origin/${BASE_REF}" which yields
HEAD on push-to-main and makes diffs empty; update the logic to use
github.event.before as the base when github.event_name == 'push' (fall back to
origin/${BASE_REF} for pull_request) so the computed sha reflects the previous
commit for pushes; also fix the diff pathspec that currently reads
':.github/workflows/ci.yaml' to be an exclusion by changing it to
':!.github/workflows/ci.yaml' so the workflow file is excluded like the other
directories (adjust any step that evaluates rust_code or cargo_check to use the
corrected merge_base and pathspec).
| if git diff --quiet "${MERGE_BASE}...HEAD" -- \ | ||
| ':!Lib/**' \ | ||
| ':!scripts/**' \ | ||
| ':!extra_tests/**' \ | ||
| ':.github/workflows/ci.yaml' \ | ||
| ; then |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Current expression (with typo) matches:"
git ls-files -- ':!Lib/**' ':!scripts/**' ':!extra_tests/**' ':.github/workflows/ci.yaml' | sed -n '1,20p'
echo
echo "Fixed expression matches:"
git ls-files -- ':!Lib/**' ':!scripts/**' ':!extra_tests/**' ':!.github/workflows/ci.yaml' | sed -n '1,20p'
echo
echo "Expected: first output is narrowly scoped; second output includes repo files excluding the specified paths."Repository: RustPython/RustPython
Length of output: 774
🏁 Script executed:
cat -n .github/workflows/ci.yaml | head -100Repository: RustPython/RustPython
Length of output: 4096
🏁 Script executed:
grep -n "rust_code" .github/workflows/ci.yamlRepository: RustPython/RustPython
Length of output: 222
🏁 Script executed:
sed -n '150,165p' .github/workflows/ci.yamlRepository: RustPython/RustPython
Length of output: 530
Fix pathspec typo at line 64 (:! is required for exclusion).
':.github/workflows/ci.yaml' is a positive pathspec that only includes the ci.yaml file. This breaks the change detection logic—when Rust code changes but ci.yaml doesn't, the check incorrectly reports no changes, causing the cargo check job to be skipped. It should be ':!.github/workflows/ci.yaml' to exclude ci.yaml from the set of files being checked.
One-line fix
- ':.github/workflows/ci.yaml' \
+ ':!.github/workflows/ci.yaml' \📝 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.
| if git diff --quiet "${MERGE_BASE}...HEAD" -- \ | |
| ':!Lib/**' \ | |
| ':!scripts/**' \ | |
| ':!extra_tests/**' \ | |
| ':.github/workflows/ci.yaml' \ | |
| ; then | |
| if git diff --quiet "${MERGE_BASE}...HEAD" -- \ | |
| ':!Lib/**' \ | |
| ':!scripts/**' \ | |
| ':!extra_tests/**' \ | |
| ':!.github/workflows/ci.yaml' \ | |
| ; then |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yaml around lines 60 - 65, The git diff pathspec list
in the conditional uses ':.github/workflows/ci.yaml' which incorrectly includes
ci.yaml instead of excluding it; update the pathspec in the git diff command
(the list passed to git diff --quiet "${MERGE_BASE}...HEAD") to use
':!.github/workflows/ci.yaml' so ci.yaml is excluded like the other patterns
(e.g., ':!Lib/**', ':!scripts/**').
Shamelessly stole the idea from https://github.com/astral-sh/ruff/blob/99d97bd72f1934ac2af93e52468c10ef1c7a1a4e/.github/workflows/ci.yaml
Summary by CodeRabbit
Release Notes
Note: This is an internal infrastructure improvement with no user-facing impact.