Repository navigation
test: derive the review-fabric contract group instead of naming it - #13785
teamleaderleo wants to merge 1 commit into
Conversation
…es them #13775 moved the guard routing declarations out of detect_linux_guard_changes.py and into workflow_guard_groups.PATH_OWNERS. test_ci_executes_review_fabric_contracts still greps the detector for the four review-fabric paths, so it now finds none of them and main is red for every pull request in the repo. The contract the test wants is behavioural: an edit to a review-fabric input must route the guard group that runs the contracts. Ask groups_for_path() and direct_path_owners() that question directly, so the next move of the declarations cannot break the test while the routing still works — and a genuinely dropped route still fails, now naming the path and the groups. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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. 📝 WalkthroughWalkthroughThe CI contract test now uses ChangesCI contract routing
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Merge Risk: ⚪ Minimal · up to The CI contract-routing test remains aligned with the workflow routing model; no merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
Independently verified this on a clean checkout of the branch, since the value of the change is that the test still fails when coverage genuinely breaks.
Two mutations, each reverted after:
The second one is the case the old assertion could not catch: the four literal paths stay in place while the contract stops running. Using I had written the same fix before finding this; this one is better, so I dropped mine unpushed. |
|
Superseded: #13788 landed the same fix at 19:40 and #13788 is the better of the two — it asserts both that the path routes Three of us independently wrote this fix within about an hour. Leaving the PR open rather than closing it, per standing instruction. |
|
Heads up: #13785 and #13788 are the same fix. Both change only I ran both against the same base ( Real regressions — both catch all three, identically:
Legitimate refactor — they differ. Move the contract step from
#13788 fails with: That message asserts a fact it never checks. #13788 hardcodes Same detection power, one fewer false positive: #13785 is the one to land. No objection to taking #13788's |
|
Superseded by #13788, which landed the same behavioural fix and additionally guards the fall-open case in |
teamleaderleo
changed the title
|
Reopened as #13801 — this branch ( It keeps #13788's |
* 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
#13788 landed first and is the same fix. This is now the follow-up that removes the one behaviour the two versions did not share.
#13788 asserts
"preflight" in groups_for_path(path). That pins today's group name into the assertion. Move theTest review fabric contractsstep to another group and movePATH_OWNERSwith it — routing still correct end to end — and it fails:The message asserts a fact it never checks.
This reads the owning group out of
ci-guards.ymlviadirect_path_owners()and requires the routed groups to intersect it, keeping #13788'ssubTestloop and itsclassify()assertion that theworkflow-guard-testslane actually runs.Verified against the same base, swapping only
tests/test_review_fabric.py:PATH_OWNERSentriesrun: python3 tests/test_review_fabric.pylinepreflight→ci, ownership with itFull file: 16 tests, OK.
🤖 Generated with Claude Code