Repository navigation
test: assert review fabric routing by behaviour, not by detector source - #13800
teamleaderleo wants to merge 1 commit into
Conversation
`test_ci_executes_review_fabric_contracts` grepped scripts/ci/detect_linux_guard_changes.py for each review fabric contract path as a literal string. #13775 derived the Linux guard route inputs from ci-guards.yml instead of listing them, so the literals went away and the test has failed on main ever since. main's last ci.yml run predates that merge, so nothing reported it; it surfaces on any pull request whose diff routes to the preflight group. The routing itself is correct, and narrower than before: each of the four contract paths reaches exactly the preflight group, which is the group that runs this file. Assert that instead, and read the owning group out of ci-guards.yml rather than naming it, so a step moving between groups keeps the test honest instead of breaking it. Verified the assertion still bites: pointing the policy path at another group fails with "reaches ['release-ios'], none of which runs the review fabric tests". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 16 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)
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 |
|
All contributors have signed the CLA ✍️ ✅ |
|
This is the third PR on The group-derivation idea here is right and matches #13801. The difference is what the assertion is made against, and it costs a regression.
Measured on base
Dropping ownership is the regression this test exists to catch, so that first row is the whole job. #13801 asserts against Suggest closing this in favour of #13801. The comment here — "Assert what the router does, not how it is written" — is better than what #13801 carries, and worth lifting across. |
* 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>
1 participant
main is red on this guard right now
python3 tests/test_review_fabric.pyfails on a clean checkout oforigin/mainat 5a848ea:test_ci_executes_review_fabric_contractsreadscripts/ci/detect_linux_guard_changes.pyand asserted each review fabric contract path appeared in it as a literal string. #13775 (5c07ba3) derived the Linux guard route inputs fromci-guards.ymlinstead of listing them, so those literals are gone.It went unreported because main's most recent
ci.ymlrun is atf3d204a46, which predates #13775. It is not harmless, though: it surfaces on any pull request whose diff routes to thepreflightgroup, which includes anything touching a workflow, script or test.The routing is fine — the assertion was not
#13775 made routing narrower, not broken. Each contract path reaches exactly the one group that runs these tests:
linux_guard_tests.github/review-fabric-policy.jsonpreflight.github/review-fabric.mdpreflight.github/scripts/review_fabric.pypreflighttests/test_review_fabric.pypreflight, +1So the test now asserts that: for each contract path, the router selects the guard job and selects a group that runs this file. The owning group is read out of
ci-guards.ymlviadirect_path_ownersrather than hardcoded, so a step moving between groups keeps the test honest instead of breaking it — the failure mode this PR is fixing.Verification
origin/main, passes here.PATH_OWNERSfails with:AssertionError: frozenset() is not true : .github/review-fabric-policy.json reaches ['release-ios'], none of which runs the review fabric tests(perturbation reverted; the diff is one file).
test_check_ghostty_zig_workflows.py(nobashlexinstalled locally) andtest_ghostty_zig_version_sync.sh(ghostty submodule not checked out), both of which fail identically on cleanorigin/main.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes
test_ci_executes_review_fabric_contracts, which failed on main once the Linux guard router started deriving its inputs fromci-guards.ymlinstead of listing each contract path as a literal in the detector source.ci-guards.yml, so moving a step between groups updates the contract instead of breaking the test.Written for commit 1f2ea4c. Summary will update on new commits.