Repository navigation
ci: bound the suite coverage gate - #13820
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 7 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesCI timeout configuration
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No concrete merge-blocking risk remains in this workflow-only change. 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
* ci: bound the suite coverage gate * ci: align timeout placement with in-flight branch fixes
* ci: bound the suite coverage gate * ci: align timeout placement with in-flight branch fixes
* ci: bound the suite coverage gate * ci: align timeout placement with in-flight branch fixes
* test: require push and pull_request path filters to agree A workflow that filters both events writes its input list twice, and the two copies can disagree without either pull request seeing it. Two already have: ci-artifact-transport.yml guards nine paths on pull requests and not on push, and cmux-skill-contract.yml omits a test its own job runs. web-complexity.yml diverges on purpose -- a pull request must not be able to queue the job that runs its own edit -- so it is declared in EXEMPTIONS with a reason rather than left looking like a typo. An exemption whose workflow no longer diverges is an error, so it cannot go stale. The guard lives in testbox-broker-guard.yml, which deliberately carries no path filter of its own: a parity check over path filters cannot be gated by one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: guard the same files on main that pull requests guard ci-artifact-transport.yml listed 21 paths under pull_request and 12 under push, so nine files were checked on pull requests and unguarded on main. Four of the nine are test files the job itself executes, and tests/test_ci_selective_layer_wiring.py reads .github/workflows/ci-macos.yml and asserts on its contents -- so a push to main that broke the selective layer wiring ran nothing. The push list is the wrong one: it omits inputs the job demonstrably reads. cmux-skill-contract.yml had the same shape at one path: its job runs tests/test_cmux_settings_jsonc.py, which only the pull_request filter named. Both push lists now match their pull_request lists, and tests/test_ci_workflow_path_filter_parity.py fails if they drift apart again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: check review-fabric routing through the router, not its source text (#13788) test_ci_executes_review_fabric_contracts grepped scripts/ci/detect_linux_guard_changes.py for each review-fabric path as a literal string. #13775 made the guard routes derived from PATH_OWNERS and from ci-guards.yml's run: lines, so those literals are no longer in the file and the test fails on a clean main -- taking preflight, Guard status, linux-preflight, tests and ci-status down with it on every pull request routing that lane. The routing is intact; only the check was stale. Assert the behaviour instead: the path must route linux_guard_tests, and groups_for_path must explicitly own it with preflight. That second assertion uses groups_for_path rather than classify_test_groups because classify_test_groups falls open to every group for an unknown path, and so would keep passing if ownership were dropped. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * test: preserve negated workflow path filter ordering * ci: bound the suite coverage gate (#13820) * ci: bound the suite coverage gate * ci: align timeout placement with in-flight branch fixes --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* test(ci): reconcile the required-check tuple against GitHub
REQUIRED_CHECKS mirrors a list that lives in a repository ruleset, which
is not in this tree. Nothing today compares the two, so an admin adding a
required check leaves the tuple stale and every guard still green, while
pull requests wait on a context no workflow produces.
These cases cover the reconciliation that does not exist yet, over fixture
payloads so the guard lane stays offline. Most of them pin the fail-closed
rule: an unreadable, empty or unrecognised payload must raise rather than
report agreement, because a reconciliation that passes when it cannot read
its source is the bug being fixed.
Red until scripts/ci/required_status_checks.py lands.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* ci: reconcile the required checks on main against GitHub
The required status checks live in the `main` ruleset. REQUIRED_CHECKS was a
hand-kept copy of that list, and nothing compared the two: an admin could add
a required check and every guard in the tree stayed green while each pull
request waited on a context no workflow produces, with no red check to name
the cause.
scripts/ci/required_status_checks.py now owns the copy, and
.github/workflows/required-checks-drift.yml asks GitHub every six hours
whether it is still true. It checks two directions, because they fail
separately:
- the live contexts against REQUIRED_CHECKS, in both directions;
- each required context against what actually reported on the last ten
merged pull request heads, which catches a renamed producing job. There
the settings and the tuple still agree, and both name a phantom.
The reconciliation reads `repos/:owner/:repo/rules/branches/main`, not
`branches/main/protection`. docs/ci/derived-not-declared.md recorded this row
as blocked on a token with `administration: read` that PR CI must not hold;
that is true of the protection endpoint and not of the rulesets endpoint,
which returns the same contexts to an ordinary repository read and, on this
public repository, to no token at all. The workflow holds `contents: read`.
Every unreadable, empty or unrecognised response fails the job. A guard that
reports success when it could not read its source is the bug this fixes, so
it is not a shape the new code is allowed to take.
tests/test_ci_merge_queue_required_checks.py now imports the tuple instead of
declaring a second one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test: check review-fabric routing through the router, not its source text (#13788)
test_ci_executes_review_fabric_contracts grepped
scripts/ci/detect_linux_guard_changes.py for each review-fabric path as a
literal string. #13775 made the guard routes derived from PATH_OWNERS and from
ci-guards.yml's run: lines, so those literals are no longer in the file and the
test fails on a clean main -- taking preflight, Guard status, linux-preflight,
tests and ci-status down with it on every pull request routing that lane.
The routing is intact; only the check was stale. Assert the behaviour instead:
the path must route linux_guard_tests, and groups_for_path must explicitly own
it with preflight. That second assertion uses groups_for_path rather than
classify_test_groups because classify_test_groups falls open to every group for
an unknown path, and so would keep passing if ownership were dropped.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* ci: bound the suite coverage gate (#13820)
* ci: bound the suite coverage gate
* ci: align timeout placement with in-flight branch fixes
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ile, not display name (#13793) * test: pin workflow_run triggers to the files they name `on.workflow_run.workflows:` matches a workflow's display `name:`. Renaming a workflow for clarity stops every consumer from triggering, and nothing turns red: the consumer simply never runs again. Four workflows depend on a name this way, and `merge-group-fail-fast.yml` is the expensive one -- when it stops firing, a doomed merge group runs its macOS jobs to the end and every queue entry behind it waits. This test requires each consumer to declare the workflow file it means, derives the expected name from that file, and requires the fail-fast watcher to confirm `github.event.workflow_run.path` before it cancels anything. It fails until the next commit adds those declarations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: identify the fail-fast watcher's source workflow by file Each workflow_run consumer now declares the file whose display name its `workflows:` filter spells out, so the pairing is written down in terms that do not change when someone renames a workflow for clarity. `merge-group-fail-fast.yml` also checks that identity at run time. The workflow_run object carries `path` next to the presentation `name`, and display names are not unique, so the watcher confirms it was started by merge-group-policy-checks.yml before it cancels a CI run. The CI run it then watches was already located by workflow file rather than by name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: check review-fabric routing through the router, not its source text (#13788) test_ci_executes_review_fabric_contracts grepped scripts/ci/detect_linux_guard_changes.py for each review-fabric path as a literal string. #13775 made the guard routes derived from PATH_OWNERS and from ci-guards.yml's run: lines, so those literals are no longer in the file and the test fails on a clean main -- taking preflight, Guard status, linux-preflight, tests and ci-status down with it on every pull request routing that lane. The routing is intact; only the check was stale. Assert the behaviour instead: the path must route linux_guard_tests, and groups_for_path must explicitly own it with preflight. That second assertion uses groups_for_path rather than classify_test_groups because classify_test_groups falls open to every group for an unknown path, and so would keep passing if ownership were dropped. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * ci: bound the suite coverage gate (#13820) * ci: bound the suite coverage gate * ci: align timeout placement with in-flight branch fixes --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The new
suite-coveragejob has no timeout, so the required CI guard rejects every merged PR test tree containing it. Add a five-minute bound to this short policy check.Validation: ran
tests/test_ci_required_checks_are_bounded.pyagainst main83a6294a1d; it failed specifically onsuite-coverage. The same guard passes with this one-line change. This unblocks the guard failure observed on #13795 without changing test scheduling.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds a five-minute timeout to the
suite-coverageCI job so the required CI guard no longer rejects PRs running it. This unblocks the guard failure on #13795 without changing test scheduling.Written for commit 5942f76. Summary will update on new commits.
Summary by CodeRabbit