The record grammar: gates per paragraph, Gate resolved only, per-comment verdicts, IDs match the milestone - #275
Conversation
There was a problem hiding this comment.
Request changes.
Finding: the PR adds the new requirement-ID mismatch behavior to milestone evidence
and status, but the tests only cover the shared parser, the close helper, and
the status note string. The behavior promised by M13-R6 is that close and evidence
refuse REQUIREMENT_ID_MISMATCH, while status reports the same condition and
continues. The changed call sites in internal/cli/evidence.go and
internal/cli/status.go need verb/report-level tests that would fail if those
call sites were removed.
Verification run:
go test ./...passed.go vet ./...passed.gofmt -l .reported files under the requested localgomodcache; tracked Go
files are gofmt-clean viagit ls-files '*.go' | xargs gofmt -l.go build -o ./gh-codecrew ./cmd/codecrewpassed../gh-codecrew milestone evidence 13passed read-only../gh-codecrew milestone close 13 --dry-runrefusedOPEN_TASKSbefore the
requirement gate, writing nothing../gh-codecrew statusprinted the live board successfully../gh-codecrew milestone evidence 12still refusedNOT_FOUND, matching the
PR's stated out-of-scope boundary.
Record checks:
- Reviewer App config id matched
gh api /apps/radiusred-checky --jq .id. - Reviewer identity differs from the PR author.
- Task plan existed before the first commit.
- Decision comments were recorded on #260 before the commits.
- PR body contains
Closes #260. - Commit subjects are conventional, lowercase after type, within 100 characters,
and all commit messages reference(#260).
… comment (#260) Three of M13-R6's four rule changes, all in the tracker: what recorded text means, tightened for protocol 2.0. **Gate raised:** is now read per paragraph, anywhere in a comment, exactly as the record labels are — a gate raised as a comment's second paragraph used to be invisible, so GATE_UNRECORDED never fired for it. Resolution is per gate and only by a **Gate resolved:** record: it answers every gate raised before it and still open, so one comment may still answer several questions, and never one raised after it; a bare **Decision:** on another subject no longer clears every gate on the issue, which is what SPEC §8 always said. UnresolvedGates returns one entry per unresolved gate. ParseVerdicts reads a comment through StripCode and emits at most one verdict per requirement ID per comment — the first — so the caller's last-write-wins tally now means "the latest comment carrying a verdict for the ID wins, first match inside it", and a verdict quoted in a code span or a fence is content, as a URL in code already was. stripCode and its helpers move from internal/cli/evidence.go to the tracker package as StripCode: one implementation, so the record-reading rule and the citation-reading rule cannot drift apart again. MismatchedRequirementIDs lands beside RequirementIDs for the fourth rule; its callers follow in the next commit. Table tests for every rule, including the M5 corpus: the project's one hand-raised gate (#68) and its answer are not reclassified. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
…own (#260) M13-R6's fourth rule at its three call sites. SPEC §4 says a requirement ID is M<milestone>-R<k>; until now nothing checked it, so a milestone could declare M12-R3 under M13 and the close would count a verdict against a requirement it does not own. milestone close folds the check into its existing "requirements declared" gate — the gate list and the dry run keep their shape — and milestone evidence checks it before walking the record, both refusing the new REQUIREMENT_ID_MISMATCH with every offending ID and the milestone read in the detail, because the fix is a hand edit of the issue body. status prints the same condition as a line and carries on: it reports the board rather than gating it, and a mismatch on one milestone must not hide the others' tasks. extractURLs now calls tracker.StripCode, the shared implementation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
…roduction (#260) SPEC §4 states the placement rule for the whole label family, gates included, the per-comment verdict supersession rule with code as content, and that the requirement-ID grammar is enforced rather than conventional. §8 says only **Gate resolved:** resolves and that resolution is per gate. The §6 rows follow: task finish names its two gate refusals, milestone close its new refusal and the per-comment tally, milestone evidence the ID check before the walk, and a new every-verb row carries REQUIREMENT_ID_MISMATCH. roles/qa.md asks for one verdict per requirement ID per comment and says an earlier verdict may be quoted inside a code span or a fence — now the safe place to put one. roles/implementer.md says a gate may be raised in any paragraph and that only a **Gate resolved:** reply answers it. The introduction's refusal-code list gains the code under the verbs that raise it, thirty-two to thirty-three with the README's count, and the CHANGELOG entry names each rule change as a protocol-2.0 break. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
…l sites (#260) Checky's finding on PR #275: M13-R6 promises that milestone close and milestone evidence refuse REQUIREMENT_ID_MISMATCH while status reports the same condition and carries on, but only the shared parser, the close helper and the note string were covered — the two new call sites could have been deleted with every test still green. evidenceFake grows a body per milestone and records the walk, so the verb's two tests read through milestoneEvidenceReport: a milestone whose body carries a foreign ID refuses with the code, names the offending ID and not its own, prints the requirements note first and — the point — never reaches the walk; a milestone whose IDs are its own passes the check and walks. statusFake gains an overridable body, and status gets the mirror pair: a mismatch prints the line naming the code and the report still runs to the tasks and the gate list below it; a well-formed section prints no line at all. Both fail if their call site is removed, which is what they are for. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
14f08ee to
f40750c
Compare
|
Addressed in f40750c (rebased onto b48ffb6, You are right that the two new call sites were untested:
Mutation-checked both: with the Two things from the rebase worth your eye:
|
There was a problem hiding this comment.
Approved. Round-two finding 5125242306 is closed: the evidence and status REQUIREMENT_ID_MISMATCH call sites now have verb/report-level tests, and a compile-safe mutation that disables those branches makes TestMilestoneEvidenceRefusesMismatchedRequirementID and TestStatusReportsRequirementIDMismatch fail for the missing behavior. Rebase check is clean: CHANGELOG keeps both entries, and SPEC's milestone evidence row carries both #262's closed-milestone wording and the ID-grammar sentence.
Verification: go test ./..., go vet ./..., gofmt -l ., and go build -o ./gh-codecrew ./cmd/codecrew all passed with local caches. Read-only/dry-run smoke checks ran for status, milestone evidence 13, milestone close 13 --dry-run, and task finish 260 --dry-run. Record checks passed: plan and Decisions precede the first commit, PR body has Closes #260, commits are conventional and reference (#260), and the reviewer App identity differs from the PR author.
What this does
M13-R6 in full: four rules about what recorded text means, tightened for protocol 2.0. Every one of them reclassifies text already written on GitHub, which is why they ride a major rather than a minor. Tracker-only in code apart from the three verbs' call sites; it touches no path, so it is independent of #255.
1. A gate is read per paragraph.
**Gate raised:**now counts wherever**Decision:**does — any paragraph, anywhere in a comment — instead of only when it opens the comment body. A gate raised as a comment's second paragraph used to not exist totask finish, soGATE_UNRECORDEDnever fired for it (Claude finding 6a). The gate labels also accept the parenthetical qualifier the rest of the family may carry, andExtractRecordsandUnresolvedGatesnow share oneparagraphs()splitter, so there is one placement rule and one implementation of it.2. Only a
**Gate resolved:**record resolves a gate, and per gate. The single trailing "last resolution index" is gone. Comments are read in order, paragraph by paragraph: a**Gate raised:**paragraph opens a gate, a**Gate resolved:**paragraph closes every gate opened before it and still open — one comment may still answer several questions — and never one opened after it. A bare**Decision:**on any subject resolves nothing, which is SPEC §8's wording (Claude finding 6b).UnresolvedGatesreturns one entry per unresolved gate, so a comment raising two unanswered gates counts twice. Thecc:needs-decisionlabel remains the hard block. The rule is stated precisely in the Decision on #260.3. Verdicts per comment, code stripped.
ParseVerdictsreads a comment throughStripCodeand emits at most one verdict per requirement ID per comment — the first.milestone close's existing last-write-wins tally over that stream therefore now means exactly "the latest comment carrying a verdict for the ID wins, and within it the first match counts", so a QA comment may quote the verdict it supersedes without superseding itself, and a verdict inside a code span or a fence is content, as a URL in code already was (Claude finding 7).stripCodeand its helpers move frominternal/cli/evidence.gotointernal/tracker/markdown.goastracker.StripCode— one implementation, so the record-reading rule and the citation-reading rule cannot drift apart again (Decision).4. A requirement ID carries its milestone's own number. New
tracker.MismatchedRequirementIDs(body, n)and the new refusalREQUIREMENT_ID_MISMATCH, naming every offending ID and the milestone read.milestone closechecks it inside its existing "requirements declared" gate (the gate list and the dry run keep their shape) before any verdict is counted against a foreign requirement;milestone evidencechecks it before walking the record;statusprints it as a line and carries on, because it reports the board rather than gating it (Decision). SPEC §4's ID grammar was conventional until now (Codex finding 5).milestone newalready writes correct IDs and--dry-runis unaffected.Docs
SPEC §4 (the placement rule covers the gate labels; the per-comment verdict rule with code as content; the ID grammar is enforced), §8 (only
**Gate resolved:**resolves, per gate), and the §6 rows fortask finish,milestone closeandmilestone evidence, plus a new every-verb row for the ID check.roles/qa.mdasks for one verdict per ID per comment and points at the fence as the safe place to quote an earlier one;roles/implementer.mdsays a gate may be raised in any paragraph and only a**Gate resolved:**reply answers it.docs/introduction.md's refusal-code list gains the code under the verbs that raise it, thirty-two → thirty-three, with the README's count. CHANGELOG:### The record grammar, tightened.Tests
Table tests in the tracker package for each rule: a gate raised as a second paragraph is unresolved and resolvable; a bare Decision after a gate leaves it unresolved; a Deviation resolves nothing; one
Gate resolvedclears every earlier open gate; two gates in one comment are two gates; a label mid-line is not a gate; a verdict in a fence and in a code span is content; two verdicts for one ID in one comment take the first; a later comment supersedes; the close's tally over that stream; matching and mismatched requirement-ID sets, including that a prefix (M1-R1underM13) is a mismatch and that IDs outside## Requirementsare not requirements;StripCodedirectly.internal/clicovers the new refusal's code and detail, and the status note.Fixtures. No
testdata/fixture encoded the old behaviour, so none needed changing: the M5 corpus carries the project's one hand-raised gate (#68) and its**Gate resolved:**answer, which stays resolved.TestUnresolvedGatesM5Corpusnow asserts that over the live corpus so a future loosening is caught. The one test that did encode the old rule —TestUnresolvedGates's "gate then decision counts" case — is rewritten as its opposite.Live history. Checked: no open issue in the hub carries a
**Gate raised:**record (the search hits are prose in titles and capture bodies), so nothing open is newly blocked; closed issues are re-read only bymilestone evidence. Smoke test against the live hub, read-only:milestone evidence 13prints the same output before and after the change (requirements counted: M13-R1 … (8)/all 0 cited links resolve across 10 issues);milestone evidence 12refusesNOT_FOUNDbefore and after, M12 being closed — that is M13-R9's job, not this task's.For the reviewer
StripCode, records and gates are not. Reasoning and the rejected alternatives are in the Decision.unsatisfiedvsnot satisfied(the second half of Claude finding 7) is out of scope here, as the task says. It is worth its own requirement:unsatisfiedcurrently reads as "no verdict at all" and surfaces asVERDICT_MISSING, which is the most misleading refusal in the set.Satisfies M13-R6.
Closes #260
🤖 Generated with Claude Code
https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr