Shims removed; the exit-code contract and the refusal-code table in SPEC (#261) - #282
Conversation
…rates (#261) The pre-1.0 form of the same conventions was accepted by a 1.x binary with a note to update the field. Under 2.0 it is two majors back and its repo is on the 1.x layout, so it meets the migration refusal every older pointer meets — which is the truth about it. The missing-`codecrew:` rule is kept, not inverted: the pointer now lives at `.codecrew/config.yml`, so the file's own path is proof of the layout it speaks, and a 1.x repo refuses LAYOUT_LEGACY long before this check runs. BREAKING CHANGE: a pointer saying codecrew: "0.1" is refused and moved forward with `gh codecrew migrate` rather than accepted with a note. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
… names every seat (#261) `holder()` resolved `coordinator` to `~` when a declared routing table had no such row, because the row arrived after 1.0 hubs had scaffolded their tables. `init` scaffolds it now and `gh codecrew migrate` adds it to a 1.x table, so the shim's only remaining effect was to infer a holder for a seat the table does not name. A table declaring nothing at all is untouched: every seat is the operator's, coordinator included. BREAKING CHANGE: a routing table that declares roles but no `coordinator` row now errors on `role coordinator` as it does for any other missing role; `gh codecrew migrate` writes the row. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
…ses NOT_OWNER (#261) `StartedBy` fell back to the task's first assignee "for tasks started before the record existed", which gave every assigned-but-never-started task an implicit owner for the ownership gate. Deleted with the other 1.0 shims, and the `Task` argument it was the only reader of goes with it. The consequence lands in the gate: an empty owner used to wave `sameSeat` through, so with no fallback an unstarted task would have been finishable by anyone. It refuses instead, and the detail names `task start` rather than an owner that does not exist. `--bypass` still overrides, with a record that says there was no start rather than naming a phantom starter. BREAKING CHANGE: `task finish` on a task with no `**Started by**` record refuses NOT_OWNER; run `gh codecrew task start <ref>` first. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
… table (#261) §6 said only that a blocked gate "exits nonzero". It now says what the contract is: 0 on success, 1 on every failure of any kind, no exit-code taxonomy within this major — a finer status would break every caller already asserting on 1 the day it arrived — and the `refused[CODE]: detail` line on stderr is the machine channel, the code the branch point and the detail prose nothing should parse. The `task finish` row also gains `--operator-confirm`, which was in the usage string and load-bearing for the solo tier but missing from its signature. §10 gains the catalogue: all forty-two codes, the verbs that raise each and one line of meaning, under the stability promise already living there. §5's version prose records that the "0.1" acceptance is gone and why the missing-field rule is kept. `docs/introduction.md` carried a second catalogue of the same codes and now points at the table; the README bullet follows. A test holds the table, the count SPEC states and the `refuse("CODE"` sites in `internal/cli/` to the same set, so a verb cannot add a code without its row. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
…own (#261) Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
There was a problem hiding this comment.
Verdict: request changes on radiusred/gh-codecrew PR #282.
Finding:
internal/cli/context.go:32still says"0.1" and a missing field proceed with a note on stderr. That describes the shim this PR removes. The comment should say only a missing field proceeds with a note; a"0.1"pointer now refuses as an older major and namesgh codecrew migrate.
Verification:
- Read reviewer contract with
gh codecrew roles show reviewer. - Read
.codecrew/roles/implementer.mdandCONTRIBUTING.md. - Inspected the diff before the PR body, then read task #261 and milestone #254 records and decisions.
- Confirmed local App ID
4719924matchesgh api /apps/radiusred-checky --jq .id, and differs from PR authorapp/radiusred-cody. - Compared production
refuse("CODE"literals ininternal/cli/against SPEC §10: 42 source codes, 42 SPEC rows, no differences. Test-only placeholders wereCODEandX. - Ran
go test ./...,go vet ./...,gofmt -l ., andgo build -o ./gh-codecrew ./cmd/codecrewwith caches under<local path>. - Exercised
./gh-codecrew task finish 261 --dry-run; it exited 1 withrefused[NOT_OWNER]and wrote nothing.
… acceptance (#261) The doc comment still said `"0.1" and a missing field proceed with a note`, which is the shim this task removes: a 0.1 pointer is an older major now and refuses with the detail naming `gh codecrew migrate`. A missing field is the only version the check proceeds on (checky's finding on PR #282). The same sweep found three test fixtures still carrying `codecrew: "0.1"` as arbitrary pointer content — `teamCtx`, and the two `routeRole` shapes whose version line the test asserts survives the surgery. None of them exercises the version check, so none was failing, but a fixture is an example, and the example was of a pointer this binary refuses. They say `"2.0"` now. The 0.1 pointers that remain are the ones that mean it: `Compatible` and `CompatibleHub` refusing it as two majors back, `loadConfig` refusing it PROTOCOL_MISMATCH, and `migrate` refusing it MIGRATION_UNSUPPORTED. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
|
Fixed in 1f18fd1 — thanks, that comment was exactly the shim's own description surviving the shim.
I grepped the rest of the tree for the acceptance in every wording I could think of ( The Rebased on |
There was a problem hiding this comment.
Verdict: approve radiusred/gh-codecrew PR #282 (round two).
Round one's single finding is addressed. internal/cli/context.go:32 no longer describes the deleted acceptance: a "0.1" pointer is now called out as an older major refused PROTOCOL_MISMATCH with the detail that names gh codecrew migrate, and only a missing field proceeds with a note. That matches config.Compatible — olderMajor("0.1", "2.0") is true, so the migrate branch is the one a 0.1 pointer takes — and it matches the prose already in internal/config/config.go and SPEC §5.
Verification (round two, d13dffe..1f18fd1, base still 5fbf14f):
- The diff is three files and nothing else: the
loadConfigcomment, plus thecodecrew: "0.1"→"2.0"sweep in fixtures that carried the version as arbitrary content —teamCtxininternal/cli/context_test.go, andnestedYML,inlineYMLand the spoke fixture inTestRouteRoleErrorsininternal/cli/identity_test.go. - The fixture change does not weaken
TestRouteRoleNestedShape. Its keep-list still asserts the whole linecodecrew: "2.0" # protocol version— value and trailing comment — survivesrouteRole's line surgery, androuteRoleneither reads nor branches on the version (it re-parses withconfig.Parse, which does not run the compatibility check), so"0.1"and"2.0"are equivalent inputs there. TheTestRouteRoleErrorsspoke fixture still fails for the reason it was written for: no routing table. - The tests that assert the removal are untouched and still present:
internal/cli/context_test.go:115(0.1refused, detail "predates the protocol"),internal/config/config_test.goTestCompatibleRefuses01, andinternal/cli/migrate_test.go:483. Agit grepfor0.1leaves only those deliberate cases, historical milestone records, and unrelated loopback addresses. go test ./...,go vet ./...,gofmt -l .andgo build -o ./gh-codecrew ./cmd/codecreware clean at1f18fd1. Both PR checks pass.- Record unchanged and intact:
Closes #261in the PR body, both Decision comments linked from it, the Unreleased CHANGELOG entry ending(#261). All six commits since the base are conventional, lowercase after the type, ≤100 characters (longest 93), each referencing(#261), and authored asradiusred-cody[bot]. - Identity: the local credential stub's App id matches
gh api /apps/radiusred-checky --jq .id(they agree), and it differs from the PR authorradiusred-cody[bot].
The substance — the three shim removals, the 42-row SPEC §10 table against the refuse("CODE" sites, the exit-code contract, and a read-only dry run — was verified in round one and is unchanged by this push.
Nit, non-blocking: in internal/cli/context.go:34, "A missing field is the only version the check proceeds on, with a note on stderr" is strictly true only of the versions that produce a note — a pointer on the same major proceeds too, silently. If the comment is touched again, "the only mismatch the check proceeds on" would close the gap.
M13-R7 in full: the three 1.0 compatibility shims deleted, and the machine contract an orchestrator reads written into the SPEC.
Closes #261
No backlog capture is adopted by M13-R7 — the requirement names none, so this PR closes the task alone.
The shims
config.Compatible's"0.1"acceptance. A 1.x binary took the pre-1.0 form of the same conventions with a note to update the field. It is two majors back under 2.0, and its repo is on the 1.x layout, so it now meets the samegh codecrew migraterefusal every older pointer meets — which is the truth about it.A pointer with no
codecrew:field is still assumed current, with a note. The Claude scan's finding 1 proposed inverting that; the Decision comment on #261 records why it is kept instead. Short version: the rule was risky while the pointer sat at the repository root, where the same file could belong to either major. Under 2.0 the file's own path is proof of the layout — a 1.x repo has no.codecrew/config.ymland refusesLAYOUT_LEGACYbeforeCompatibleis ever reached — so inverting would refuse the one pointer shape that cannot be lying: a hand-written two-line spoke pointer.The
coordinatorspecial case inholder(). It resolved the seat to~when a declared table had no such row, because the row arrived after 1.0 hubs had scaffolded their tables.initscaffolds it andmigrate(#256) writes it into a 1.x table, so the case's only remaining effect was to infer a holder for a seat the table does not name. It is now the same error every other missing role is. A table that declares nothing at all is unaffected: every seat, coordinator included, is the operator's.The first-assignee fallback in
StartedBy. It gave every assigned-but-never-started task an implicit owner for the ownership gate. Deleted, and theTaskargument it was the only reader of goes with it (StartedBy(comments []Comment) string).Deleting it alone would have made the gate weaker, which is worth the reviewer's attention:
sameSeatanswered true for an empty owner, so a task with no start record would have been finishable by anyone. So the empty-owner case moved out ofsameSeatand into the gate, where it refusesNOT_OWNERwith its own detail — "nothing records a start on o/r#7 … run gh codecrew task start 7 … (an assignee is not a start record)".--bypassstill overrides it, and its record says there was no start rather than naming an owner that does not exist.SPEC
§6 — the exit-code contract. The section closed with "Verbs exit nonzero with a machine-readable reason". It now states the contract:
0on success,1on every failure of any kind, and no exit-code taxonomy within this major, because a status finer than "this did not happen" would break every caller already asserting on1the day it arrived. Therefused[CODE]: detailline on stderr is the machine channel — the code is the branch point and stable, the detail is prose for a human that nothing should parse, anote:line is advisory, and output a caller consumes goes to stdout.§6 — the
task finishrow gains--operator-confirm(the operator's note on this issue: it was in the usage string and load-bearing for the solo tier but missing from the signature), and its ownership prose loses the assignee fallback.§10 — the refusal-code table. All forty-two: the code, the verbs that raise it, one line of meaning, sitting under the stability promise that already lived in §10. The count is now stated once, in SPEC.
§5 records that the
"0.1"acceptance is gone and why the missing-field rule is kept.One catalogue, and a test that keeps it honest
docs/introduction.mdcarried a second full catalogue of the same forty-two codes; three tasks in this milestone alone (#256, #259, this one) had to write each new code twice in two registers. Its## Refusal codessection now carries what only it was saying — the shape of a refusal, what the code and the detail are each for, that the fix is in the detail line — and points at SPEC §10 for the table. The README bullet follows. The Decision comment on #261 records the trade-off and what was rejected.internal/cli/refusals_test.goholds three things to the same set: the codes in SPEC §10's table, the count SPEC states, and therefuse("CODE"sites ininternal/cli/. A verb that adds a code cannot merge without its row, and a row cannot outlive its code. Verified by mutation — deleting theNO_PRrow fails the test naming it.Requirements
M13-R7.
Tests
go test ./...,go vet ./...,gofmt -l .clean. Each removal ships with a test that the old behaviour now refuses:TestCompatibleRefuses01andTestCompatibleMissingFieldIsAssumedCurrent(config),TestCoordinatorAbsentFromDeclaredTableRefuses(role),TestStartedByIgnoresAssignees(tracker) andTestPlanFinishRefusesATaskNobodyStarted(the gate end of it, including the--bypassrecord's wording), plus the invertedsameSeatcase andTestRefusalCodesMatchTheSpecTable.Decisions recorded
codecrew:field: kept, not invertedNo deviations, and no ask-the-human points were reached.
🤖 Generated with Claude Code
https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr