Repository navigation
Give every PR exactly one Summary check - #787
Conversation
|
Warning Review limit reached
Next review available in: 46 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
4abf23e to
d927aa0
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4abf23edb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| paths: | ||
| - 'scripts/claude-assistant/**' |
There was a problem hiding this comment.
GitHub's path-filter semantics run a paths workflow when at least one changed file matches, whereas paths-ignore skips CI only when every changed file matches. A PR changing both src/... and README.md therefore starts this fallback alongside ci.yml, producing two Summary checks from the same app; GitHub explicitly warns that duplicate job names across workflows create ambiguous required-check results and can block merging. Make the fallback mutually exclusive with real CI, or replace both with one always-triggered aggregator.
AGENTS.md reference: AGENTS.md:L188-L198
Useful? React with 👍 / 👎.
| - name: Gate satisfied without the matrix | ||
| run: echo "Every changed file matches ci.yml paths-ignore; the VM matrix does not apply to this PR." |
There was a problem hiding this comment.
For a PR changing only .github/workflows/claude.yml, ci.yml is skipped and this required Summary succeeds unconditionally even when the newly added actionlint job fails. Since the stated follow-up is to require Summary, branch protection can accept that green gate without proving the check intended for these ignored files passed; make this Summary depend on the applicable lint work, or expose an always-present required aggregate that includes it.
AGENTS.md reference: AGENTS.md:L188-L198
Useful? React with 👍 / 👎.
ci.yml's pull_request trigger carried a paths-ignore list. A PR whose every changed file matched it produced no check runs at all - not a skipped Summary, nothing - and GitHub reported it CLEAN. PR #785, a two-file actions/setup-node bump touching only claude*.yml, was mechanically mergeable that way, with nothing configured to stop it. Same claim-confusion as the base-branch filter that let PR #752 in: "no failing checks" and "the checks ran" are different claims. Skipping now happens per job, not per workflow: - The pull_request trigger has no path filter, so Summary exists on every PR and can be made a required check on main. Required checks also reject direct pushes whose commits lack them. - A `changes` job decides whether the VM matrix can say anything about the diff, and lint, packaging, fc-mock, host, host-root and container are gated on it. Summary already treats a skipped `needs` as non-failure and a failed one as failure, so the gate needs no new logic. Non-pull_request events and an empty file list both fail toward running everything. - actionlint (binary checksum-pinned to the published v1.7.10 release value) lints every workflow file and is in summary.needs. It lives here rather than in claude-lint.yml so that Summary gates it: for a PR touching only claude*.yml, which skips the matrix, it is the one gating job that still runs and can fail. An earlier version of this change added a second workflow, also named CI, that emitted its own Summary for the ignored paths. Review found that structurally cannot work: `paths` fires when ANY file matches while `paths-ignore` skips only when EVERY file matches, so a PR touching both src/ and a doc emits two Summary check runs and makes the required-check result ambiguous - and that fallback Summary passed unconditionally, including when actionlint failed. Both findings are now regression tests. Tested (red first, then green): - Reinstating paths-ignore, ungating lint, and dropping actionlint from summary.needs fails all three new guards: FAIL every_pull_request_produces_exactly_one_summary FAIL expensive_jobs_are_gated_on_the_changes_job FAIL workflow_changes_get_a_relevant_gated_check Restoring the change: 10 tests run: 10 passed, 0 skipped - actionlint -shellcheck= clean on all workflow files - cargo fmt --check clean
ejc3
changed the title
d927aa0 to
6980e7b
Compare
A PR whose every changed file matches ci.yml's
pull_requestpaths-ignoreproduces no check runs at all — not a skipped Summary, nothing — and GitHub reports itCLEAN. PR #785 (a two-fileactions/setup-nodebump touching only.github/workflows/claude*.yml) reached exactly that state: mechanically mergeable on the strength of checks that never ran. Same claim-confusion as PR #752's base-branch filter: "no failing checks" and "the checks ran" are different statements.What this adds
ci-docs-summary.yml— the documented companion pattern for skipped-but-required checks: same workflow name (CI), same job name (Summary), withpathsequal to ci.yml'spaths-ignore. Every PR now produces aSummarycheck run, which makesSummarysafe to set as a required status check onmain(follow-up, after this merges). Required checks also reject direct pushes whose commits lack passing checks — closing the door on accidental pushes to main generally.claude-lint.yml— now triggers on.github/workflows/claude*.ymltoo, and gains anactionlintjob (binary download checksum-pinned to the published v1.7.10 release value, no third-party action) linting every workflow file. A claude-workflow edit finally gets a check that can fail for a YAML defect. shellcheck is off pending cleanup of the pre-existing SC2086/SC2129 findings in claude.yml's run blocks; actionlint's own checks (expressions, action inputs, needs refs, runner labels) apply.test_ci_workflow_coverage.rs, in the file's existing style: the companion'spathsmust equal ci.yml'spaths-ignoreentry-for-entry (drift reopens the hole for exactly the drifted path), and claude-lint must keep coveringclaude*.ymlwith a real linter job.Test evidence
RED (before the workflow changes, tests only):
GREEN (with the workflows in place):
actionlint -shellcheck=(v1.7.10) passes on all nine workflow files, including the two edited here; the same invocation was verified green against #785's head, so the rebased #785 will pass its new check.Refs #786 (the "#785 fail-closed" and review-integrity items).