test: check review-fabric routing through the router, not its source text by teamleaderleo · Pull Request #13788 · manaflow-ai/cmux

teamleaderleo · 2026-09-22T23:32:25Z

Summary by cubic

Fixes the review-fabric routing contract test, which failed on a clean main and blocked several CI status checks, after #13775 changed how guard routes are derived. The test used to grep the router source for literal path strings; it now asks the router for the routing decision directly.

Asserts each path routes to the linux_guard_tests lane and is explicitly owned by the preflight group.
Checks ownership via groups_for_path instead of classify_test_groups because the latter falls open to every group for unowned paths.

^{Written for commit 17c4f1f. Summary will update on new commits.}

coderabbitai · 2026-09-22T23:32:32Z

Warning

Review limit reached

Next included review available in 30 minutes.

Check out review usage here.

View limit details

Limit 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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c6690822-c4d8-453e-8158-8b4aa38f6ed9

📥 Commits

Reviewing files that changed from the base of the PR and between 95e843a and e170af7.

📒 Files selected for processing (1)

tests/test_review_fabric.py

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

…text test_ci_executes_review_fabric_contracts grepped scripts/ci/detect_linux_guard_changes.py for each review-fabric path as a literal string. manaflow-ai#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>

github-actions · 2026-09-22T23:32:36Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

teamleaderleo · 2026-09-22T23:38:24Z

hm? did we not originally write a pr description here?

teamleaderleo · 2026-09-22T23:41:32Z

Heads up: #13785 and #13788 are the same fix. Both change only tests/test_review_fabric.py, and both rewrite test_ci_executes_review_fabric_contracts — the assertion #13775 invalidated by making guard routes derived instead of literal. Whichever lands second will conflict.

I ran both against the same base (95e843a26) in a worktree, swapping only the test file.

Real regressions — both catch all three, identically:

Scenario	#13785	#13788
R1 drop the review-fabric `PATH_OWNERS` entries	caught	caught
R2 stop running `python3 tests/test_review_fabric.py` in `ci-guards.yml`	caught	caught
R3 reroute a path to the wrong group	caught	caught

Legitimate refactor — they differ. Move the contract step from preflight to ci and move PATH_OWNERS with it, so routing stays correct end to end:

	result
#13785	PASS — correct
#13788	FAIL — false positive

#13788 fails with:

AssertionError: 'preflight' not found in ('ci',) : editing .github/review-fabric-policy.json
must select the preflight guard group, which is where tests/test_review_fabric.py runs

That message asserts a fact it never checks. #13788 hardcodes "preflight"; #13785 derives the owning group from direct_path_owners(workflow), so it keeps testing the real invariant — the paths route to whichever group actually runs the contracts — instead of pinning today's group name.

Same detection power, one fewer false positive: #13785 is the one to land. No objection to taking #13788's subTest loop or its explanatory comment on top of it.

#13788 landed the same fix as this branch and asserts `"preflight" in groups_for_path(path)`. That pins today's group name into the assertion: move the `Test review fabric contracts` step to another group and move `PATH_OWNERS` with it — routing still correct end to end — and the test fails with `'preflight' not found in ('ci',) : ... must select the preflight guard group, which is where tests/test_review_fabric.py runs`. The message asserts a fact it never checks. Read the owning group out of ci-guards.yml with `direct_path_owners()` and require the routed groups to intersect it, keeping #13788's subTest loop and its `classify()` assertion that the lane itself runs. Verified against the same base, swapping only this file: dropping the review-fabric `PATH_OWNERS` entries, replacing the `run:` line, and rerouting a path to the wrong group each still fail; the group-move refactor above now passes where #13788 fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…13801) #13788 landed the same fix as this branch and asserts `"preflight" in groups_for_path(path)`. That pins today's group name into the assertion: move the `Test review fabric contracts` step to another group and move `PATH_OWNERS` with it — routing still correct end to end — and the test fails with `'preflight' not found in ('ci',) : ... must select the preflight guard group, which is where tests/test_review_fabric.py runs`. The message asserts a fact it never checks. Read the owning group out of ci-guards.yml with `direct_path_owners()` and require the routed groups to intersect it, keeping #13788's subTest loop and its `classify()` assertion that the lane itself runs. Verified against the same base, swapping only this file: dropping the review-fabric `PATH_OWNERS` entries, replacing the `run:` line, and rerouting a path to the wrong group each still fail; the group-move refactor above now passes where #13788 fails it. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

…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: 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>

* docs: tell agent sessions how not to duplicate each other Several agent sessions work this repo at once and cannot see each other. Nothing in CLAUDE.md says so, and the resulting waste is now measurable. On 2026-09-22 a shared observable -- main going red on test_ci_executes_review_fabric_contracts -- reached every session at once. Each diagnosed it independently and opened a PR: #13785, #13788, #13800, #13801 and #13802, five PRs on one test function in twenty-one minutes, two of them five seconds apart. One landed. The reviewer attention spent on the other four is the cost this section exists to avoid. Two failures showed up repeatedly and are written down here because neither is guessable: Sessions share one GitHub account, so `author` and `mergedBy` name the account and never the actor. Three separate claims about which session did what were made from those fields today, all wrong, and two were relayed to the user before being retracted. GitHub keeps serving `mergeable` and `mergeStateStatus` on closed and merged pull requests, where they are stale. Reading CONFLICTING off an already merged PR sent a session to resolve a conflict that did not exist, twice. The last paragraph guards the opposite error. #13754 and #13797 changed exactly the same two files, fixed different bugs, and both merged, so an overlap scan keyed on file paths would have proposed closing a good PR. Composing them locally and running the shared test is what distinguishes the cases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: give sessions a callsign to sign their work with The section above tells sessions how not to collide. It does not give them a way to say who they were, and that gap produced its own failures today: three claims about which session opened, merged or reviewed something, every one of them read off `author` or `mergedBy`, every one wrong, two relayed to the user before being retracted. Those fields name the shared push account. Nothing in the repository answers "which session did this", so sessions inferred it from timing and were wrong. A callsign in a commit trailer answers it directly. Stated as attribution and not authority, deliberately. The Stensibly product model is explicit that callsigns, names, branches and prior activity never substitute for current authority evidence, and a self-assigned name two sessions can pick independently is exactly the kind of identity that must not gate an action. It records who acted. It grants nothing. This commit signs itself, which is the whole convention. Callsign: Teakettle 🫖 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: correct the callsign section against the live registry The previous commit invented a convention. There is already a working one, and checking it showed the invented version wrong in three ways. `teamleaderleo/stensibly` #454 is a live registrar: a `github-actions[bot]` workflow that accepts `/callsign reserve`, answers in seconds with a `callsign-receipt/v0` carrying an accepted generation and a 24h lease, and releases on request. Its worker quickstart is `docs/callsign-registry-dogfood.md` in that repo. This section now points there instead of describing a parallel scheme. I reserved through it rather than trusting the document, and each correction below is something the receipt disproved: The sigil is derived from the callsign by the registrar, not chosen by the worker. Reserving `Teakettle` returned `💾`, not the emoji the previous commit had picked for itself and put in its own trailer. Names are leased. Collision keys are compared without case or separators, so `Rook`, `rook` and `r-o_o k` are one name. The previous commit said collisions were expected and tolerable, which is true of the derived sigil and false of the name. A generation may be shown only from an accepted receipt, with `pending` or `unregistered` as the honest fallback. The previous commit had no notion of a generation at all. The sign-off format follows the registry's: `— <Callsign> g<generation> <sigil>`, not a bare name and emoji. Attribution and not authority is unchanged and now cites its owner: `teamleaderleo/quarry` #1103 tracks the defect that a callsign in comment text is marker text rather than an authenticated principal. Callsign: Teakettle g1 💾 Run: run_cmux_ci_delineation_20260922_01 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: check local worktrees and recent remote branches --------- 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>

069f1ce ci: streamline app-host test consumers (manaflow-ai#13427) cfe69ec Merge pull request manaflow-ai#13885 from teamleaderleo/docs/full-ci-explicit-scope 25a6378 docs: require explicit broad-suite intent for full-ci 0a6c362 Merge pull request manaflow-ai#13749 from manaflow-ai/ci/app-host-product-fast-transport 3094a95 ci: overlap local Rust helper builds with nightly Swift compilation (manaflow-ai#13874) ea4b13b CI: run Claude wrapper regressions without compiling the app (manaflow-ai#13869) 8f1491b test: bound SSH fish helper pipe draining (manaflow-ai#13408) 9667375 Merge pull request manaflow-ai#13826 from manaflow-ai/12284-claude-wrapper-subcommands 40b0cd7 ci: scope trusted web complexity before Bun setup (manaflow-ai#13598) 0f0d4a7 ci(iOS): build simulator test product once (manaflow-ai#13600) 0f28e77 Merge main settings runtime repair for native validation 819adce Merge branch 'main' of https://github.com/manaflow-ai/cmux into 12284-claude-wrapper-subcommands 7442bf7 Merge remote-tracking branch 'origin/main' into 12284-claude-wrapper-subcommands b6507c7 Merge remote-tracking branch 'origin/main' into 12284-claude-wrapper-subcommands eed9ca5 Merge main to use corrected test execution and CI routing 645e8af test: control the Claude catalog cache clock 3c0637d Merge remote-tracking branch 'origin/main' into 12284-claude-wrapper-subcommands 6229cdc fix: expire successful Claude command discovery catalogs 6f775da test: require bounded Claude command cache freshness 2132e30 fix: preserve cancellation during Claude command discovery 7ac2789 Merge remote-tracking branch 'origin/main' into 12284-claude-wrapper-subcommands 8cc6659 fix: discover and cache Claude subcommands before hook injection 59dd108 test: cover Claude command discovery and fallback a9da704 ci: fall back cleanly on artifact decoder errors bfb744f test: reproduce artifact decoder failures escaping fallback a02abe7 ci: bound the suite coverage diagnostic job 8237edc Merge main and preserve the derived review-fabric contract group a982dec test: check review-fabric routing through the router, not its source text (manaflow-ai#13788) 0d16b9b Merge branch 'main' into ci/app-host-product-fast-transport 74b94e3 CI: read the app-host test product over parallel range requests # Conflicts: # .github/workflows/ci-artifact-transport.yml # .github/workflows/ci-guards.yml # .github/workflows/ci-macos.yml # .github/workflows/ci.yml # .github/workflows/nightly.yml # .github/workflows/test-ios.yml # .github/workflows/web-complexity-trusted.yml

teamleaderleo force-pushed the ci/fix-review-fabric-route-contract branch from 17c4f1f to e170af7 Compare September 22, 2026 23:32

teamleaderleo merged commit d7f6648 into manaflow-ai:main Sep 22, 2026
31 of 32 checks passed

teamleaderleo mentioned this pull request Sep 22, 2026

test: derive the review-fabric contract group instead of naming it #13785

Closed

teamleaderleo deleted the ci/fix-review-fabric-route-contract branch September 23, 2026 11:22

Navigation Menu

Repository navigation

test: check review-fabric routing through the router, not its source text - #13788

test: check review-fabric routing through the router, not its source text#13788
teamleaderleo merged 1 commit into
manaflow-ai:mainfrom
teamleaderleo:ci/fix-review-fabric-route-contract

teamleaderleo commented Sep 22, 2026 •

edited by cubic-dev-ai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 22, 2026 •

edited

Loading

Review limit reached

Uh oh!

github-actions Bot commented Sep 22, 2026

Uh oh!

teamleaderleo commented Sep 22, 2026

Uh oh!

Uh oh!

teamleaderleo commented Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

teamleaderleo commented Sep 22, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by cubic

Uh oh!

coderabbitai Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

github-actions Bot commented Sep 22, 2026

Uh oh!

teamleaderleo commented Sep 22, 2026

Uh oh!

Uh oh!

teamleaderleo commented Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

teamleaderleo commented Sep 22, 2026 •

edited by cubic-dev-ai Bot

Loading

coderabbitai Bot commented Sep 22, 2026 •

edited

Loading