ci: require a single aggregate gate job per workflow#12453
Merged
Conversation
Code Review by Qodo
1. TS CI success bypass
|
📝 WalkthroughWalkthroughBoth ChangesUnified Aggregate Success Gates for CI Workflows
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
PR Summary by QodoFix required Rust CI Test checks by splitting matrix into per-OS jobs WalkthroughsDescription• Replace the Rust Test matrix with three static per-OS required-check jobs • Extract shared Rust test steps into a reusable composite action • Ensure non-Rust PRs report required contexts as skipped (not missing) Diagramgraph TD
A[".github/workflows/pacquet-ci.yml"] --> B["changes job (rust flag)"] --> C["test-ubuntu job"] --> F["rust-test composite action"] --> G[("ci-performance-rust-<os> artifact")]
B --> D["test-windows job"] --> F
B --> E["test-macos job"] --> F
H{{"Branch protection: required checks"}} --> C
H --> D
H --> E
High-Level AssessmentThe following are alternative approaches to this PR: 1. Keep matrix; move gating from job `if:` to step-level `if:`
2. Add three lightweight wrapper jobs for required contexts
Recommendation: The chosen approach (three static per-OS jobs + a shared composite action) is the most robust way to satisfy branch-protection required check naming in GitHub Actions. It guarantees the exact required contexts exist whether the jobs run or are skipped, while keeping the implementation DRY via the composite action. File ChangesOther (2)
|
Comment on lines
+84
to
+86
| - uses: ./.github/actions/rust-test | ||
| with: | ||
| save-cache: ${{ github.ref_name == 'main' }} | ||
| os: ubuntu-latest |
There was a problem hiding this comment.
1. Missing rust-test path filter 🐞 Bug ☼ Reliability
The changes job’s paths-filter omits .github/actions/rust-test/**, so PRs that only modify the new rust-test composite action will leave needs.changes.outputs.rust false and skip all Rust CI jobs. This allows changes to the shared test runner logic to merge without actually executing the tests/artifact staging behavior it defines.
Agent Prompt
### Issue description
The Rust change-detection filter in `.github/workflows/pacquet-ci.yml` does not include the newly introduced composite action directory `.github/actions/rust-test/**`. As a result, PRs that only change that action will not be treated as “Rust changes”, and all Rust CI jobs (including the required Test jobs) will be skipped.
### Issue Context
The workflow now runs tests via `uses: ./.github/actions/rust-test`, but the detector only watches `rustup` and `binstall` action directories.
### Fix Focus Areas
- .github/workflows/pacquet-ci.yml[47-64]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
No longer applicable — this PR was reworked to use a single Rust CI / Success aggregate gate instead of per-OS jobs, so the .github/actions/rust-test composite action was removed. The change-detection paths-filter is unchanged from main.
Written by an agent (Claude Code, claude-opus-4-8).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/actions/rust-test/action.yml (1)
100-114: 💤 Low valueHeredoc produces leading whitespace in metadata.json.
The indented heredoc content will include leading spaces in the generated JSON file. While JSON parsers tolerate this, using
<<-EOFwith tab indentation (or unindenting the content) would produce cleaner output.🤖 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/actions/rust-test/action.yml around lines 100 - 114, The heredoc that generates the metadata.json file includes leading whitespace due to indentation, which produces non-standard JSON formatting. Replace the current `<<EOF` heredoc syntax with `<<-EOF` to enable tab-stripping mode, then adjust the indentation of the JSON content lines to use tabs instead of spaces, which will be stripped automatically. Alternatively, unindent all the JSON content lines to remove the leading whitespace while keeping the heredoc delimiter unindented.
🤖 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.
Nitpick comments:
In @.github/actions/rust-test/action.yml:
- Around line 100-114: The heredoc that generates the metadata.json file
includes leading whitespace due to indentation, which produces non-standard JSON
formatting. Replace the current `<<EOF` heredoc syntax with `<<-EOF` to enable
tab-stripping mode, then adjust the indentation of the JSON content lines to use
tabs instead of spaces, which will be stripped automatically. Alternatively,
unindent all the JSON content lines to remove the leading whitespace while
keeping the heredoc delimiter unindented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88132de0-f75f-4ab6-a369-58e6db65900a
📒 Files selected for processing (2)
.github/actions/rust-test/action.yml.github/workflows/pacquet-ci.yml
3a6e0c9 to
ae4bfe5
Compare
|
Code review by qodo was updated up to the latest commit ae4bfe5 |
Comment on lines
+94
to
+105
| success: | ||
| name: TS CI / Success | ||
| if: ${{ always() }} | ||
| needs: | ||
| - compile-and-lint | ||
| - test-smoke | ||
| - test | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Fail if any dependency failed or was cancelled | ||
| if: ${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') }} | ||
| run: exit 1 |
There was a problem hiding this comment.
1. Ts ci success bypass 🐞 Bug ☼ Reliability
TS CI / Success always runs and treats skipped dependencies as a pass, so for same-repo pull_request events where compile-and-lint is skipped, it can report success even though no TS compile/lint/tests ran. If branch protection is updated to require only this context (per the new comment), same-repo PRs can be merged without waiting for the push-based CI run that actually executes the test matrix.
Agent Prompt
## Issue description
The new aggregate job `TS CI / Success` runs unconditionally (`if: always()`) and only fails when dependencies are `failure`/`cancelled`. On same-repo `pull_request` events, `compile-and-lint` is intentionally skipped, which causes `test-smoke` and `test` to be skipped as well; the aggregate job still reports success, creating a potential merge-gate bypass if this context is used as the only required check.
## Issue Context
`compile-and-lint` already has an `if:` guard to avoid duplicate same-repo PR runs; the aggregate job should follow the same guard (or otherwise ensure it does not produce a passing required check when the actual CI did not run).
## Fix Focus Areas
- .github/workflows/ci.yml[9-14]
- .github/workflows/ci.yml[88-105]
### Concrete fix
Update the `success` job to use the same `if:` condition as `compile-and-lint` (while still using `always()` semantics for dependency failures within that scope), e.g.:
- `if: ${{ always() && (github.event_name == 'push' || github.event.pull_request.head.repo.full_name != github.repository || github.head_ref == 'chore/update-lockfile') }}`
This ensures same-repo PR events don’t get a green `TS CI / Success` without running the underlying TS jobs, while push events still produce the required context.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Member
Author
There was a problem hiding this comment.
Good catch — this is a real bypass and I've removed the TS CI / Success gate entirely (force-pushed). TS CI runs on both push and pull_request, and on same-repo PRs the pull_request run skips compile-and-lint (cascading to the whole matrix), so a single gate would report success-via-skip. A gate is a late check, so it also can't provide the early in_progress blocking signal that TS CI's granular required contexts give during the real push run.
TS CI never had the original problem this PR fixes, so it keeps its granular required contexts unchanged. The aggregate gate is now applied only to Rust CI, which is safe there because push is restricted to main — a PR branch produces a single run with no concurrent run to shadow it.
Written by an agent (Claude Code, claude-opus-4-8).
ae4bfe5 to
76e6b68
Compare
|
Code review by qodo was updated up to the latest commit 76e6b68 |
Branch protection required the individual Rust CI and TS CI jobs as
status checks. That is brittle: most Rust jobs skip on PRs that touch no
Rust files, and a matrix job skipped at the job level never expands its
'${{ matrix.* }}' name — GitHub reports one check under the literal
unexpanded name, so the required per-OS contexts never appear and any
non-Rust PR blocks forever waiting for status that is never reported.
Add one static-named aggregate gate per workflow ('Rust CI / Success'
and 'TS CI / Success') that runs with 'if: always()' and fails only when
a dependency actually failed or was cancelled; skipped dependencies
count as a pass. Branch protection can then require just these two
contexts, decoupling them from the matrix shapes.
TS CI runs on both push and pull_request, and on same-repo PRs the
pull_request run skips every job for deduplication. A naive gate would
report a green 'TS CI / Success' on that skipped run and let a PR merge
before the real push run finished. The gate's name is therefore
'TS CI / Success' only on runs that do real work, and a different name
on the skipped duplicate run — using 'compile-and-lint''s 'if:' verbatim,
which the name MUST stay in sync with. Rust CI needs no such guard: its
push trigger is restricted to main, so a PR produces a single run.
Requires a branch-protection update: replace the per-job Rust CI / TS CI
contexts with 'Rust CI / Success' and 'TS CI / Success'.
76e6b68 to
22aa26a
Compare
|
Code review by qodo was updated up to the latest commit 22aa26a |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
17-17: ⚖️ Poor tradeoffCondition duplication creates a synchronization risk.
The condition on line 106 must exactly match line 17 to preserve the race-condition safety: if they diverge, a skipped duplicate run could report as
TS CI / Successbefore the real run finishes, satisfying branch protection prematurely.The "KEEP IN SYNC" comments mitigate this, but manual synchronization is fragile. GitHub Actions doesn't offer good primitives (env, variables, anchors) for sharing job-level expressions, so eliminating the duplication is difficult.
Consider adding a CI linting step or a custom action that verifies these conditions remain identical, or document this as a known limitation in a broader CONTRIBUTING guide.
Also applies to: 106-106
🤖 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/ci.yml at line 17, The workflow has a duplicated condition expression on line 17 and line 106 that must remain perfectly synchronized to prevent race-condition safety issues where a skipped duplicate run could report success prematurely. To fix this, implement one of the following approaches: either add a CI linting step or custom action that verifies both conditions on line 17 and line 106 remain identical (failing the build if they diverge), or document this synchronization requirement as a known limitation in a CONTRIBUTING guide with clear instructions for maintainers on how to update both locations together whenever changes are needed. The goal is to move from relying on informal "KEEP IN SYNC" comments to an automated or well-documented approach that prevents accidental divergence.
🤖 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.
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 17: The workflow has a duplicated condition expression on line 17 and
line 106 that must remain perfectly synchronized to prevent race-condition
safety issues where a skipped duplicate run could report success prematurely. To
fix this, implement one of the following approaches: either add a CI linting
step or custom action that verifies both conditions on line 17 and line 106
remain identical (failing the build if they diverge), or document this
synchronization requirement as a known limitation in a CONTRIBUTING guide with
clear instructions for maintainers on how to update both locations together
whenever changes are needed. The goal is to move from relying on informal "KEEP
IN SYNC" comments to an automated or well-documented approach that prevents
accidental divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e6cae91-5264-4fdb-bf0f-817281155016
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/pacquet-ci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pacquet-ci.yml
This was referenced Jun 17, 2026
This was referenced Jun 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PRs that touch no Rust files (e.g. #12287) get stuck as not mergeable, blocked on required checks that are never reported.
Branch protection requires the individual Rust CI Test jobs:
Rust CI / Test / ubuntuRust CI / Test / windowsRust CI / Test / macosThese come from a single matrix job gated to skip on non-Rust PRs. When a matrix job is skipped at the job level, GitHub does not expand the matrix — it reports a single check under the literal, unexpanded name:
So the three required per-OS contexts never appear, and the PR waits on them forever.
Fix
Add one static-named aggregate gate per workflow — the canonical pattern for required matrix checks:
Rust CI / Success—needsevery Rust job (changes,test,clippy,doc,typos,deny,format,dylint).TS CI / Success—needscompile-and-lint,test-smoke,test.Each runs with
if: ${{ always() }}so it reports under a static name in every state, and fails only when a dependency actually failed or was cancelled (contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')). Skipped dependencies count as a pass — a skipped job means "not applicable to this PR". No third-party action is used, keeping the merge gate's supply-chain surface minimal.The TS CI same-repo bypass, and how the gate avoids it
TS CI runs on both
pushandpull_request. On a same-repo PR thepull_requestrun skipscompile-and-lint(the dedupif:guard), cascading skips to the whole matrix. A naive gate would report a greenTS CI / Successon that skipped run and satisfy branch protection before the realpushrun finished — letting a PR merge without CI actually running.To prevent this, the gate's name is
TS CI / Successonly on runs that do real work, and a different name (TS CI / Success (skipped duplicate)) on the same-repopull_requestrun that gets deduplicated:The condition is
compile-and-lint'sif:verbatim, so the requiredTS CI / Successcontext is emitted only by real runs (same-repopush, forkpull_request,chore/update-lockfilePRs,mainpush). Forks keep working (their real run is thepull_requestevent). The two spots carry a comment noting the name and theif:must stay in sync.Rust CI needs no such guard: its
pushtrigger is restricted tomain, so a PR produces a single run with no concurrent run to shadow it.Required follow-up (branch protection)
One-time change on
main, landed in lockstep with the merge. Replace the per-job contexts:Rust CI / Clippy,Rust CI / Dylint,Rust CI / Doc,Rust CI / Spell Check,Rust CI / Test / ubuntu,Rust CI / Test / windows,Rust CI / Test / macosTS CI / Compile & Lint,TS CI / Test / windows / Node 22,TS CI / Test / ubuntu / Node 22,TS CI / Test / ubuntu / Node 24,TS CI / Test / ubuntu / Node 26with just:
Rust CI / SuccessTS CI / SuccessNote: the
Successgates also fold in jobs that were not previously required individually (Cargo Deny,Format), so a failure in those will now block merge too.Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit