feat(review-loops): make review findings the load-bearing artifact (#162) - #169
Conversation
…oops to structured findings
Type REVIEW_SCHEMA.issues.items with severity/summary/recommendation (one
field looser than CHALLENGE_SCHEMA.findings: recommendation stays optional),
keeping `issues` out of REVIEW_SCHEMA.required and `id` out of the schema
entirely. Add normalizeFindings(raw, source) to turn a reviewer's `issues`
array into engine-assigned {id, severity, summary, recommendation} findings,
returning null when the reviewer omitted the key so callers fall back to
today's prose path byte-for-byte. Add findingsBlock() as the single renderer
feeding every fix stage: null -> today's prose, non-empty -> the id-prefixed
work list plus prose kept as context, empty array -> an explicit
no-findings line with the prose still rendered underneath.
Wire the quality loop and test loop: a changes_requested review whose
`issues` normalizes to a present, empty array is now treated as
nothing-to-fix (returns approved/{ok:true}, increments the new
ctx.metrics.findings_empty_exits counter, skips the fix stage) instead of
degrading the loop toward a fix stage with nothing to fix. Fix prompts are
now fed from findingsBlock() and ask implementers to prefix each
fixes_applied entry with the id of the finding it resolves. Reviewer prompts
gain wording tying the `issues` array to the verdict.
Adds normalizeFindings/findingsBlock unit tests, and both a present-empty-
array scenario and its mirror image (issues omitted entirely, which must
NOT be treated as empty) to the quality-loop and test-loop suites, plus a
regression proving the existing issues:['x'] fixtures still trigger their
fix stage with the new rendered finding line.
Part of #162.
… line runQualityLoop and runTestLoop repeated the same three-line "every concern goes in `issues`" instruction verbatim. Extract it to ISSUES_ASK, matching the existing HANDOFF_ASK/COMMIT_SHA_ASK shared-prompt-line pattern, so the two call sites can't drift. Synced via lint-engine.js --fix per the LOCKSTEP-EDIT rule. Part of #162.
Type the pr-review merge gate (reviewAndMerge) into the same normalizeFindings()/findingsBlock() machinery task 1 gave the internal quality/test loops (issue #162): - Add ISSUES_ASK to the spec-review and code-review prompts, tying the verdict to a non-empty `issues` array the same way quality review and test validation already do. - Normalize each reviewer's issues under a distinct source ('spec-i'+iter, 'code-i'+iter) before concatenating for recordGateOutcome, since both reviews land in one tally via parallel() and model-chosen/shared ids would collide. - Add nothingToFix(r, f): true when a reviewer approved outright or requested changes while naming zero findings. prReviewClean (both approved) stays the only path that sets approved = true. When both reviewers have nothing to fix but the pair isn't clean, break WITHOUT approving, tallied as 'carried-unresolved' (computeGateYield hardcodes exactly four disposition keys) and counted in ctx.metrics.findings_empty_exits, landing on the existing needs_human. - Add a haltReason local so fail()'s message is accurate for both the cap-reached and the early-empty-exit paths. - Feed pr-fix from findingsBlock per reviewer (with the prose comments kept below as context) and add the fixes_applied id-prefix instruction. - Rewrite the recordGateOutcome doc comment: 'accepted' no longer claims to be the same condition that ends the loop, 'carried-unresolved' now covers both the iteration-cap and the empty-exit path, and the stale "severity stays zero" NOTE is replaced since REVIEW_SCHEMA.issues is now typed and gate_findings['pr-review'].severity reports real counts. Adds 7 integration tests to tests/pr-review-gate.test.js covering the empty-exit break, the mixed empty/real-findings case, the omitted-issues fallback, both null-reviewer death paths, and non-zero severity tallying. node scripts/lint-engine.js --fix keeps .claude/workflows/ticketmill.js in lockstep.
… fix stages The "prefix each fixes_applied entry with the id it resolves" instruction was copy-pasted verbatim (only the example id differed) into the quality-fix, test-quality-fix, and pr-fix prompts added by issue #162. Extract it into fixesAppliedIdAsk(example), matching the existing ISSUES_ASK/COMMIT_SHA_ASK/ HANDOFF_ASK shared-prompt-line convention.
Add docs/architecture/gate-hygiene.md covering the typed REVIEW_SCHEMA.issues shape (one field looser than CHALLENGE_SCHEMA.findings), the engine-assigned id scheme, the null-vs-empty findings distinction, the three loop predicates and why the merge gate's differs, the empty-findings exit landing on needs_human as carried-unresolved, the findings_empty_exits counter, and the fixes_applied id echo. Documents the baseline shift in computeGateYield's escaped-defect signal now that gate_findings['pr-review'].count carries real data. Add the gate-hygiene.md row to index.md's file map (authored text, outside the provenance fixture's 6-line tracked segment). Precisely reword docs/architecture/CLAUDE.md:3 to distinguish the ten split-derived pages from pages added since, leaving the other two "ten files" references (:49, :83, both describing the provenance fixture's still-exactly-ten scope) untouched. Add a gate-hygiene.md row to the "What lives here" table and a pointer to the now-stale, hash-frozen metrics.md:81-84 passage this page supersedes. Copy CLAUDE.md over AGENTS.md per the freeze pair's parity note. docs/architecture/pipeline.md, metrics.md, and failure-semantics.md are untouched — their prose is hash-frozen by tests/architecture-provenance.test.js. Refs #162
…empty-array branch Every existing fixture with an approved reviewer also carried issues: [], which independently satisfies nothingToFix's second branch, so no test could tell `r.result === 'approved' ||` apart from `f.length === 0`. Add direct unit tests for nothingToFix() (mirroring the normalizeFindings/findingsBlock units) plus a reviewAndMerge() integration case pairing an approval that carries a non-empty issues array with a changes_requested/issues:[] partner, asserting bothNothingToFix still halts early as carried-unresolved without running pr-fix. Verified by temporarily deleting the approved-branch clause: all three new assertions fail against the mutant and pass against HEAD.
Spec Review (Iteration 1)Verdict: approved Verified against issue #162's acceptance criteria and the actual diff (
All acceptance criteria from issue #162 are met by the diff. |
Code Review (Iteration 1)Verdict: changes requested (1 blocking, 2 minor, 1 deferred note) Baseline is clean: Blocking1. The quality-loop and test-loop empty-findings exits are silent verification skips — no Both branches take a reviewer verdict of So an issue whose quality gate said "changes requested" can complete, merge into the batch branch, The merge gate is fine as written — Fix direction: push a line at both internal-loop exits, e.g. Note on scope: this is not a re-litigation of the adjudicated predicate. The settled decision Minor2. The comment reads "shared verbatim by every REVIEW_SCHEMA reviewer prompt (quality review, test 3.
Deferred note (out of scope for this PR)4. Severity vocabulary mismatch between the new enum and this repo's reviewer persona. |
…view Code review on PR #169 flagged that the quality-loop and test-loop empty-findings exits (a changes_requested verdict with zero structured issues, converted to a clean pass) were silent — recorded only in ctx.metrics and a pushDecision() note, neither of which reaches the batch PR body. Push a VERIFY_SKIPS entry at both exits, the same mechanism already used for capped challenges, the skipped test loop, and the skipped browser check. The pr-review merge gate's equivalent exit was already visible via needs_human and is untouched. Also: ISSUES_ASK's doc comment now names all four REVIEW_SCHEMA prompts that consume it (was missing spec review and code review), and findingsBlock() falls back to fallbackLabel (the reviewer's summary) instead of dropping straight to "(none)" when a non-null findings array pairs with empty comments, restoring the fallback chain the pre-#162 prompt had.
PR Review Fix (iteration 1)Commit c957604
|
Spec Review (Iteration 2)Verdict: approved Re-verified against issue #162's acceptance criteria and the full diff (
All acceptance criteria from issue #162 remain met by the diff at HEAD. |
Code Review (Iteration 2)Verdict: changes requested (0 blocking, 2 minor) Baseline re-verified in the worktree at Both iteration-1 fixes land correctly and are not re-flagged:
Re-checked the parts most likely to break and found them sound: Minor1.
That is not what the pre-#162 engine did. The same call site already passed
The passage this page claims to supersede got this right and says so explicitly Two places inherit the same false premise and should move with it:
Fix direction: reword the three passages to "already populated, but only incidentally — now 2. The pr-fix prompt wraps each reviewer in Nothing tells the fix agent where the spec block ends and the code block begins except the id Not re-flaggedThe severity-vocabulary mismatch between |
…adings Code review iteration 2 (PR #169) found two minor issues: gate-hygiene.md claimed gate_findings['pr-review'].count carried no real signal before #162, but the merge gate already fed real counts into it whenever a reviewer put a concern in issues rather than comments — reword the three affected passages (gate-hygiene.md, ticketmill.js's recordGateOutcome NOTE, the CLAUDE.md/AGENTS.md freeze-pair note) to "already populated, but only incidentally — now guaranteed and schema-backed". Also demote findingsBlock()'s internal headings from ## to ### so they nest under the pr-fix prompt's own "## Spec review" / "## Code review" wrapper headings instead of colliding with them.
PR Review Fix (iteration 2)Commit e2f70f0
|
Spec Review (Iteration 3)Verdict: approved Re-verified against issue #162's acceptance criteria and the full diff at HEAD (
All acceptance criteria from issue #162 are met by the diff at HEAD. |
Code Review (Iteration 3)Verdict: approved (0 blocking, 1 minor, 2 non-blocking notes) Baseline re-verified in the worktree at Both iteration-2 findings land and are not re-flagged:
New-code re-checks, all sound: Minor1.
That is the iteration-2 premise, unrevised: it says the The contradiction is load-bearing rather than cosmetic, because
and that same note says two lines earlier that the sentence "was already inaccurate when it Fix direction: reword While in that paragraph: Notes (non-blocking, outside this gate)2. 3. Em-dash convention drift in the new prose. Not re-flaggedThe severity-vocabulary mismatch between Why approvedZero blocking and zero functional findings. Every finding above is documentation consistency, none |
The quality loop and test loop empty-findings exits (a changes_requested verdict with zero structured issues, treated as clean) were silent before a follow-up code review fixed it: they now push a VERIFY_SKIPS entry, the same mechanism already used for capped challenges and skipped test loops, so they surface in the batch PR's Verification Gaps section. gate-hygiene.md covered the findings_empty_exits counter but not this visibility change; add it to the same section. Refs #162
Tech docs reviewThe engine change and the docs for it ( I checked the three commits made after that docs commit (
Verified against Commit: |
Implementation CompleteBranch Reviews passed:
Deferred Suggestions for Follow-up
|
1 participant
Closes #162
Summary
Makes structured review findings the load-bearing artifact for the three review/fix loops instead of reviewer prose.
REVIEW_SCHEMA.issuesis now typed with the sameseverity/summary/optional-recommendationshapeCHALLENGE_SCHEMA.findingsalready used (one field looser:recommendationis optional), with engine-assigned stable ids. The quality loop, test loop, and pr-review merge gate now key their exit predicates and fix-stage prompts off this structured array rather than offresult === 'approved'alone.Key decisions
normalizeFindings(raw, source)andfindingsBlock(findings, comments, fallbackLabel)helpers next torecordGateOutcome.normalizeFindingsreturnsnullwhen a reviewer omitsissuesentirely, preserving today's prose-only behavior for non-conforming reviewers rather than throwing or silently treating omission as empty.REVIEW_SCHEMA.issues.itemsrequiresseverityandsummary; severity is coerced tocritical/major/minor/unspecified.issuesstays out ofREVIEW_SCHEMA.required, andidstays out of the schema (the engine assigns ids assource-N).REVIEW_SCHEMA-producing prompts (spec review, code review, quality review, test validation) now state explicitly that every concern belongs inissues, that prose-only concerns incommentswill not be fixed, and that no findings means an empty array.findingsBlock(), with reviewer prose kept below as context only; fixes_applied entries are asked to echo the resolved finding's id.approvedOR a present-and-emptyissuesarray is treated as a clean exit (fix stage skipped,findings_empty_exitsmetric incremented).reviewAndMerge) keepsprReviewClean(both reviewers approved) as the only setter ofapproved = true. When both reviewers have nothing to fix but the gate isn't clean, the loop breaks without approving, landing on the existingneeds_humanpath, tallied ascarried-unresolved(no new disposition string introduced).recordGateOutcomedoc comment notinggate_findings['pr-review'].severitynow reports real counts instead of always zero.docs/architecture/gate-hygiene.mdcovering the new finding shape, engine-assigned ids, the absent-vs-empty distinction, the three loop predicates, and thefindings_empty_exitscounter, plus repaired thedocs/architectureCLAUDE.md/AGENTS.md freeze pair and index.md file-map entry.node scripts/lint-engine.js --fixafter engine-path edits to keep the lockstep copy ofworkflows/ticketmill.jsin sync per the profile's LOCKSTEP-EDIT RULE.Token usage (approximate, this issue only): 424435 output tokens