Gate CI test jobs via change scanning instead of workflow-level paths-ignore#8979
Gate CI test jobs via change scanning instead of workflow-level paths-ignore#8979SID-6921 wants to merge 2 commits into
Conversation
Signed-off-by: SID <nandasiddhardha@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChanges
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Trigger
participant changes
participant static-checks
participant min-dep
participant full-dep
participant packaging
Trigger->>changes: start workflow
changes->>changes: inspect changed files and set run_tests
changes->>static-checks: run_tests output
changes->>min-dep: run_tests output
changes->>full-dep: run_tests output
changes->>packaging: run_tests output
Related Issues: None specified. Related PRs: None specified. Suggested labels: ci, github-actions Suggested reviewers: CI maintainers Poem 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the main CI workflow to avoid workflow-level paths-ignore (which can prevent required checks from registering), and instead introduces a “changes” pre-job that decides whether to run heavy CI based on whether any non-documentation files changed.
Changes:
- Remove workflow-level
paths-ignorefrom.github/workflows/cicd_tests.ymlso checks always register on PRs. - Add a
changesjob that computes arun_testsoutput by diffing changed files and treatingdocs/**,*.md, and*.rstas docs-only. - Gate heavy jobs (
static-checks,min-dep,full-dep,packaging) onneeds.changes.outputs.run_tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ "${{ github.event_name }}" == "pull_request" ]]; then | ||
| base_ref="origin/${{ github.base_ref }}" | ||
| git fetch origin "${{ github.base_ref }}" --depth=1 | ||
| changed_files=$(git diff --name-only "$base_ref"..."${{ github.sha }}") |
| - uses: actions/checkout@v6 | ||
| with: | ||
| fetch-depth: 0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/cicd_tests.yml:
- Around line 52-54: The read-only checkout in the workflow still persists Git
credentials, which is unnecessary for this job. Update the actions/checkout step
in the CI workflow to explicitly disable credential persistence by setting
persist-credentials to false alongside fetch-depth, so the checkout does not
store the token in git config.
- Around line 47-51: The changes job in the workflow is missing an explicit
permissions block, so it inherits broader default GITHUB_TOKEN access than
needed. Add a least-privilege permissions setting to the changes job itself,
keeping only the read access required for diffing repository files. Update the
job definition near the changes and steps keys so the permissions are scoped
directly to that job.
- Around line 61-72: The PR diff in the workflow step is interpolating
github.base_ref directly into the shell script, which triggers
template-injection concerns. Update the changed-files logic to pass
github.base_ref into the step via env and read it as a shell variable inside the
script instead of embedding it in base_ref assignment and git fetch. Keep the
existing behavior in the pull_request branch, but ensure both the base_ref
calculation and fetch call use the env-backed variable rather than a GitHub
expression.
- Around line 58-84: The changes-detection step in cicd_tests.yml can fail
closed under set -euo pipefail when git fetch or git diff errors, which prevents
the changes job from setting a safe default. Update the shell logic around the
changed_files computation so any diff-detection failure in the current changes
job path falls back to run_tests=true instead of exiting, and add a timeout to
the fetch/diff operation; keep the fix localized to the workflow step that uses
github.event_name, git fetch, git diff, and the run_tests output.
🪄 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.yaml
Review profile: CHILL
Plan: Pro
Run ID: ab8d4120-9cd5-4085-878c-b868b0a749c5
📒 Files selected for processing (1)
.github/workflows/cicd_tests.yml
| run: | | ||
| set -euo pipefail | ||
|
|
||
| if [[ "${{ github.event_name }}" == "pull_request" ]]; then | ||
| base_ref="origin/${{ github.base_ref }}" | ||
| git fetch origin "${{ github.base_ref }}" --depth=1 | ||
| changed_files=$(git diff --name-only "$base_ref"..."${{ github.sha }}") | ||
| else | ||
| before="${{ github.event.before }}" | ||
| if [[ -z "$before" || "$before" == "0000000000000000000000000000000000000000" ]]; then | ||
| echo "run_tests=true" >> "$GITHUB_OUTPUT" | ||
| exit 0 | ||
| fi | ||
| changed_files=$(git diff --name-only "$before"..."${{ github.sha }}") | ||
| fi | ||
|
|
||
| run_tests=false | ||
| while IFS= read -r file; do | ||
| [[ -z "$file" ]] && continue | ||
| if [[ "$file" == docs/* || "$file" == *.md || "$file" == *.rst ]]; then | ||
| continue | ||
| fi | ||
| run_tests=true | ||
| break | ||
| done <<< "$changed_files" | ||
|
|
||
| echo "run_tests=$run_tests" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
git ls-files .github/workflows && echo '---' && sed -n '1,220p' .github/workflows/cicd_tests.ymlRepository: Project-MONAI/MONAI
Length of output: 8407
🌐 Web query:
GitHub Actions skipped job required status check branch protection treated as success or failure
💡 Result:
GitHub treats a skipped job as a successful status for the purposes of branch protection [1][2][3]. When a job is skipped—for example, due to a conditional if expression—it reports its status as "Success," meaning it will not block a pull request from merging even if that job is configured as a required status check [1][2][4]. However, the behavior differs depending on why the job was skipped: 1. Job-level conditionals: If a job within a workflow is skipped because its if condition evaluated to false, it is treated as a success [5][2]. This can lead to unintended results if you rely on that job to validate your code [6]. 2. Workflow-level filtering: If an entire workflow is skipped due to path filtering, branch filtering, or a commit message, the associated checks will remain in a "Pending" state [7][2][8]. Because they never report a status, they will block the pull request from merging if they are configured as required [7][2][8]. 3. Dependency chains: If a job is skipped because a prerequisite job failed, it is also treated as a success [6][2]. To prevent this, you can use the always() conditional expression in your job's if field to ensure the job runs (and reports a failure) even if its dependencies failed [6][2][8]. For required checks that depend on conditions, it is generally recommended to avoid using path or branch filtering if the workflow is required for merging [8]. Instead, consider using the always() function to ensure that downstream jobs explicitly report a failure status if required upstream tasks do not succeed [6][2].
Citations:
- 1: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/about-status-checks
- 2: https://github.com/github/docs/blob/main/content/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks.md
- 3: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches
- 4: https://docs.github.com/articles/about-status-checks
- 5: https://stackoverflow.com/questions/77996177/how-can-i-handle-a-required-check-that-isnt-always-triggered
- 6: Required status check job is skipped when one of the dependent job fails actions/runner#2566
- 7: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks
- 8: https://docs.github.com/en/enterprise-server@3.17/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks
🏁 Script executed:
rg -n "needs: changes|timeout-minutes:|if: needs\.changes\.outputs\.run_tests" .github/workflows/cicd_tests.ymlRepository: Project-MONAI/MONAI
Length of output: 520
Fail open when diff detection breaks .github/workflows/cicd_tests.yml:58-84
git fetch/git diff under set -euo pipefail can fail the changes job, which skips every needs: changes test job. Skipped required checks pass, so a transient diff error can let untested changes merge. Default run_tests=true on diff failures and add a timeout.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 47-84: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 62-62: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[error] 63-63: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/cicd_tests.yml around lines 58 - 84, The changes-detection
step in cicd_tests.yml can fail closed under set -euo pipefail when git fetch or
git diff errors, which prevents the changes job from setting a safe default.
Update the shell logic around the changed_files computation so any
diff-detection failure in the current changes job path falls back to
run_tests=true instead of exiting, and add a timeout to the fetch/diff
operation; keep the fix localized to the workflow step that uses
github.event_name, git fetch, git diff, and the run_tests output.
…ars for template injection safety, fail-open on diff errors Signed-off-by: SID <nandasiddhardha@gmail.com>
Summary
eeds.changes.outputs.run_tests
Why
Workflow-level paths-ignore can prevent required checks from appearing at all for doc-only PRs, which blocks mergeability. With this change, checks are always created, and heavy jobs are skipped only when changes are docs-only.
Change Detection Logic
Related: #8971