Store one current verdict per requirement

[tylerwillis]

Closes #21.

Implements the approved REQ-012 behavioral contract: stable per-requirement verdict files with internal content hashes, authoritative freshness checks, safe legacy transition notices, explicit prune migration, and isolated concurrent recording. It also marks verdict JSON as linguist-generated for GitHub presentation.

No review coverage or gate is weakened; superseded evidence remains in Git history.

Specification artifact: panopticon://tasks/d56f87dccf71410790e22e3c09e5f099/artifacts/REQ-012-stable-verdict-files.md

Focused BUILDING verification completed for the feature, CLI, maintenance, validation, file-scoped IDs, and incremental check behavior. Full-suite and CI proof follow in TESTING.

[tylerwillis]

claude-fable-5 — Round 1 (verified)

• Harness: claude
• Requested model: claude-fable-5
• Verified responding model: claude-fable-5
• Verification source: claude-json:modelUsage
• Reviewed commit: c8214d909d86ff74d1eb5dade9b22585d12efe43
• Review round: 1

---

All verification is done — build clean, self-check exit 0 (with ~200 migration notices), 176/176 tests pass. Here is the review.

---

Review: main...c8214d9 — Stable verdict files (REQ-012)

Summary of what I verified

The diff implements REQ-012 faithfully in code: writeVerdict now records at <requirement-id>.json, validates the review-ID/requirement-ID/hash relationship before writing, and deletes that requirement's legacy files only (src/verdict.ts:275–298). scanVerdicts classifies stable vs. legacy by filename, keeps malformed stable paths authoritative (stableFiles carries entries even when the record is invalid), and excludes legacy records from the evidence map whenever a stable path exists — so verdictViolations (src/review.ts:87–108) genuinely cannot fall back to a legacy pass past a malformed, stale, or failing stable record. Prune's retain-stable / recover-from-legacy / remove-legacy ordering matches REQ-012.6, including keeping a current stable fail over a conflicting legacy pass. I built the tool and ran it on its own repo (exit 0) and ran the full suite (18 files, 176 tests, all pass). The new 777-line test file exercises real CLI processes, byte-level record checks, and the exact hash stream — it is unusually honest test material.

MUST-FIX findings

MF-1 — REQ-012.6 contradicts REQ-006.2.2, and the diff leaves both in force.
specs/REQ-006-maintenance.md:30 still says "2119 prune MUST leave verdicts that match current review targets untouched," while specs/REQ-012-stable-verdict-files.md:69 says "2119 prune MUST remove every legacy verdict file." For a well-formed current legacy pass, the new pruneVerdicts (src/verdict.ts:231–268) deletes that file and rewrites the record at the stable path — mandated by REQ-012.6.1/.2, forbidden by a plain reading of REQ-006.2.2. The diff reconciled the equivalent conflict for REQ-003.7.3 by removing it, but did not touch REQ-006.2. Notably, the REQ-006.2.2-annotated test in tests/maintenance.test.ts was reworked to exercise only a current stable verdict (asserting bytes and mtime unchanged), which sidesteps exactly the contradictory case, so the gate can't surface the conflict. Fix in spec text: rephrase REQ-006.2.2 around the record rather than the file (its motivating clause — a passing check still passes after prune — does hold), or tombstone it the way REQ-003.7.3 was.

MF-2 — The branch ships its own repo mid-migration; every 2119 check now prints ~200 nag lines.
At c8214d9, node dist/cli.js check exits 0 but emits roughly 200 migration: … run \2119 prune`notices: nearly all pre-existing verdicts (REQ-001 through REQ-011, allself-supplied-evidence.) are still legacy-named, including superseded duplicates (five REQ-003.8.2---style stale files elsewhere, e.g. REQ-004.1.7--* ×6), and even 7 of the 9 new REQ-012 verdicts (.2119/verdicts/REQ-012.1.1--ee54df994395.jsonetc., recorded at 07:33 before the writer changed). Only re-reviewed requirements were converted in c8214d9. The PR introducing stable verdicts leaves the tool telling every subsequent developer to run the command the PR just built, and buries thecheck: PASSsummary under the notice wall. Run2119 prune` and commit the result (a mechanical, reviewable rename diff, exactly what the feature promises), or state explicitly in the PR why the transition state is retained.

MF-3 — REQ-003.7.3 was removed in the final test:-labeled commit immediately after a reviewer FAILed it.
specs/REQ-003-judgment-reviews.md:79 becomes "REQUIREMENT REMOVED" only in c8214d9 ("test: strengthen verdict migration boundaries") — not in the spec commit 1873720 that defined the superseding contract. The committed .2119/verdicts/REQ-003.7.3.json records a fail dated 15:52 the same day ("The test accepts .json despite the exact .json requirement…"), which stops gating solely because removed requirements are skipped (src/review.ts:40; ignoring tombstoned verdicts is itself sanctioned by REQ-003.5's rule at specs/REQ-003-judgment-reviews.md:45). Substantively the removal is correct — "filename is exactly <reviewId>.json" is flatly incompatible with REQ-012.1.1, and the filename-integrity property survives in validateVerdict (both kinds) with explicit wrong-name tests. But resolving a recorded FAIL by deleting the requirement inside a commit labeled as test work is the exact pattern this tool exists to make visible. Move the removal into the spec commit (or record the supersession rationale in REQ-012's overview), and let prune clear the orphaned fail record (folds into MF-2).

Suggestions

S-1 — Stale comment and dead branch in the audit-cleanup change (src/review.ts:139–141, 150; src/cli.ts:113–119).
The comment "Audit files stay while their verdict is still a current pass (REQ-003.6.3)" is now only true under preserveAudits; plain 2119 review deletes all .audit.md files. Moreover, when preserveAudits is true, every kept audit is immediately rewritten by generateAuditInstructions for the same passing set — the keep branch is unobservable, so deleting all audits unconditionally in generateInstructions would be simpler and behaviorally identical. Also note this is a REQ-006.1 behavior change riding in a REQ-012 branch; it's defensible (the directory now truly "reflects exactly the pending set," and REQ-006.1.x were honestly re-verdicted), but it deserved a mention in a commit message.

S-2 — The REQ-012.2.1 concurrency test's overlap assertion is vacuous (tests/stable-verdict-files.test.ts:182–183).
expect(child.exitCode).toBeNull() immediately after spawn is always true — the child hasn't started. Two real CLI processes do run concurrently and both records are byte-checked, which satisfies the requirement's literal "launched without waiting" wording, but the test cannot detect a genuine race in writeVerdict's write → readdir → unlink sequence if timing serializes the processes. If stronger evidence is wanted, loop the pair or gate both children on a shared start signal. (I traced the cleanup loop: it only unlinks files whose parsed requirement ID equals its own, so cross-requirement interference isn't possible through this code path — the current design is sound.)

S-3 — Migration-notice labeling edge cases (src/check.ts:96–97; src/verdict.ts:196–209).
With reviews: false, currentReviewIds is empty, so every legacy verdict is labeled "superseded" even when it matches current content; likewise true orphans (requirements with no current target) are labeled "superseded legacy verdict" with a stable "destination" that prune will never create — prune removes them instead. Cosmetic, but the notice promises a migration that won't happen.

S-4 — Duplicated stable-name regex.
SAFE_REQUIREMENT_ID (src/verdict.ts:81) and the inline /^[A-Za-z0-9.-]+$/ in src/changed.ts:377 must stay in lockstep, or check --changed and scanVerdicts will disagree on which files are stable verdicts. Export the constant from verdict.ts.

There are no must-fix findings in the implementation logic itself — I probed the fail-closed paths (malformed stable + valid current legacy, stale-stable overwrite during prune, crash window between stable write and legacy cleanup, prune of stale stable fails, junk filenames like REQ-x--BADHASH.json classifying as malformed stable) and all behave correctly and are covered by tests.

Verdict

Request changes. The code and tests are correct, thorough, and honest — the fail-closed stable-authority semantics are exactly what REQ-012 specifies, and the test material is strong. The blockers are all at the spec/repo-state layer: an unreconciled MUST-level contradiction with REQ-006.2.2 (MF-1), an incomplete self-migration that floods every check run (MF-2), and a spec removal whose placement and trigger undercut the project's own honesty story (MF-3). All three are cheap to fix and none requires reworking the implementation.

[tylerwillis]

gpt-5.6-sol — Round 1 (verified)

• Harness: codex
• Requested model: gpt-5.6-sol
• Verified responding model: gpt-5.6-sol
• Verification source: codex-rollout:turn_context.payload.model
• Reviewed commit: c8214d909d86ff74d1eb5dade9b22585d12efe43
• Review round: 1

---

Must-fix findings: None.

Suggestions: None.

Verdict: APPROVE. The final diff conforms to REQ-012, preserves compatibility with existing requirements, and the annotated tests honestly exercise the specified behavior and negative cases.

[tylerwillis]

claude-fable-5 — Round 2 (verified)

• Harness: claude
• Requested model: claude-fable-5
• Verified responding model: claude-fable-5
• Verification source: claude-json:modelUsage
• Reviewed commit: fca9bae42f2e449923c8b66aabb2a813889c2f1f
• Review round: 2

---

Round-two re-review: main...fca9bae (stable verdict files, REQ-012)

I read the full final diff (spec, source, all six test files, and the committed .2119/verdicts churn), rebuilt, and verified: npm test passes (18 files, 176 tests) and the tool's own gate 2119 check passes at the reviewed commit. The final commit fca9bae is confirmed to be CI-only in effect: a 15-second timeout on the prune-matrix test (which runs ~20 CLI subprocesses and takes ~2.3s locally, so headroom is justified) plus the three re-recorded verdicts that edit forced. REQ-012 treated as approved per instructions.

Correctness verification (adversarial pass, key paths)

• Stable-path authority (REQ-012.4.1): verdictViolations (src/review.ts:85-107) short-circuits on any stable file — malformed stable falls through to the scan's fail-closed violation and never consults legacy; stale and current-fail each produce violations then continue. scanVerdicts (src/verdict.ts:164-181) additionally excludes legacy verdicts from the verdicts map whenever a stable path exists, so no downstream consumer can fall back either. Tests exercise the bypass attempts directly (stable-verdict-files.test.ts:443-492).
• Legacy transition (REQ-012.5.1): superseded legacy passes can't satisfy the gate (map keyed by reviewId; lookup is by current ID), malformed legacy is always loud, and notices are stdout-only/non-failing with source→destination named. Confirmed on the real repo: exit 0 with 158 notices.
• Prune (REQ-012.6, REQ-006.2.2): the migrate-then-sweep in pruneVerdicts (src/verdict.ts:231-259) is correct in the corners I probed — a migrated destination overwriting a stale/malformed stable is protected from the sweep by the keep set; a kept stable is untouched byte-for-byte (the test even asserts preserved mtime, maintenance.test.ts); migration serializes the parsed record, which retains any extra fields, satisfying the reworded REQ-006.2.2 "fields preserved". Invalid legacy is swept, not migrated, with each action listed.
• Writer supersession + concurrency (REQ-012.1.1/2.1): writeVerdict only unlinks legacy files whose parsed requirementId matches its own, so concurrent recordings for distinct requirements cannot collide; the test uses two genuinely overlapping CLI processes.
• Spec honesty: removing REQ-003.7.3 via the tombstone convention (REQUIREMENT REMOVED, consistent with REQ-003.3.2's own vocabulary) is the right call — it directly contradicted stable paths — and record-level validation still enforces filename/requirementId/reviewId/hash agreement per kind (src/verdict.ts:184-187). The rewordings of REQ-006.2.2 and the review-packet comment about audit files match the actual new behavior; the always-delete-then-regenerate audit flow is consistent with REQ-006.1.1's "directory reflects exactly the pending set" and REQ-003.6.3's "without modifying any existing verdict" (tests assert verdict byte-equality across audit generation).

MUST-FIX

None. I could not construct a state in which the gate passes when it shouldn't, loses evidence prune must keep, or contradicts the amended spec text.

SUGGESTIONS

1. Repo left in transitional state — 158 migration notices on every full 2119 check (.2119/verdicts/, e.g. REQ-001.1.1--3e8f3130284f.json and ~157 siblings). The branch migrates only the requirements whose verdicts were re-recorded during development; everything else stays legacy, so the tool's own output is a wall of "run 2119 prune" lines. Notably, four verdicts introduced by this diff for the feature itself — REQ-012.1.1--ee54df994395.json, REQ-012.3.1--*, REQ-012.3.2--*, REQ-012.4.1--* — are committed at legacy paths even though the new writer records at stable paths. Running 2119 prune once and committing the canonical tree would dogfood REQ-012.6 and silence the noise.
2. Overwritten stale stable record is not listed as removed (src/verdict.ts:236-248). When migration recovers legacy evidence over an existing stale/malformed stable file, only the migrated line is printed; the destroyed prior record at the destination is implicit. REQ-012.6.2's "list each conversion or removal" is arguably satisfied, but an explicit line would make the audit trail complete.
3. Dangerous-by-default parameter stableFiles: Map<string, VerdictFile> = new Map() on verdictViolations (src/review.ts:83). There is exactly one caller and it passes the map, but the default silently disables stable-path authority for any future caller — worth making it required.
4. Dead normalization in test helper (tests/self-supplied-evidence.test.ts, normalizedTask): the regex stripping ## Additional review criteria up to ## Recording your verdict can no longer match, since the custom section now precedes ## Your task and the helper only ever receives the post-## Your task slice. Harmless, but it implies a template shape that no longer exists.
5. Cosmetic: legacyMigrationNotices labels legacy records for requirements with no current review target as "superseded" and tells the user to prune-migrate them, when prune will actually delete them; and the auditRequested variable in src/cli.ts:113 is a single-use rename with no behavioral effect.

Verdict

APPROVE. The implementation matches the amended spec faithfully, fails closed everywhere I attacked it, the new 824-line test file earns its keep (real CLI processes, real concurrency, near-miss counterexamples for every validation conjunct), and the round-two delta is exactly the advertised CI-only timeout adjustment plus its forced verdict re-recordings. The suggestions above are polish, not blockers — item 1 is the one I'd most encourage addressing before or shortly after merge.

[tylerwillis]

gpt-5.6-sol — Round 2 (verified)

• Harness: codex
• Requested model: gpt-5.6-sol
• Verified responding model: gpt-5.6-sol
• Verification source: codex-rollout:turn_context.payload.model
• Reviewed commit: fca9bae42f2e449923c8b66aabb2a813889c2f1f
• Review round: 2

---

MUST-FIX: None.

SUGGESTION: None.

VERDICT: APPROVE. The complete final diff is correct, scoped to stable verdict storage and its migration behavior, and the REQ-012 tests honestly exercise the specified outcomes. The round-two timeout-only adjustment is appropriate and does not weaken assertions.

[tylerwillis]

Final dual-review triage

Final reviewed commit: fca9bae42f2e449923c8b66aabb2a813889c2f1f
Review rounds used: 2/2
Final verified reviews: Claude 5169614361; Codex 5169630453

Both final reviewers approve with no MUST-FIX findings. The accepted round-one fixes were implemented, re-judged where evidence hashes changed, and re-run through the full local and CI gates.

Round 1 findings

• MF-1 — REQ-006.2.2 contradicted explicit legacy migration: ACCEPTED. Reworded the requirement around preserving all fields of current evidence during canonical-path migration, annotated the migration test with REQ-006.2.2, and added pass/fail, malformed-field, post-prune gate, deletion, cardinality, and reporting cases.
• MF-2 — migrate every legacy verdict in this feature PR: REJECTED AS A BLOCKER / DEFERRED. REQ-012 deliberately makes migration explicit and keeps check non-mutating. Automatically rewriting the entire repository would contradict that transition choice. A deliberate dogfood cleanup remains worthwhile and is recommended below.
• MF-3 — REQ-003.7.3 removal lacked an explicit supersession rationale: ACCEPTED. REQ-012 now records that the old review-ID filename rule directly conflicts with stable paths and that record-level filename/ID/hash validation remains enforced.
• S-1 — audit cleanup contained a dead preservation branch and stale comment: ACCEPTED. Plain review now deletes audit packets unconditionally; explicit audit requests regenerate them afterward, and the comment describes that behavior.
• S-2 — overlapping-process test contained vacuous immediate exitCode assertions: ACCEPTED. Removed those assertions; the test still launches both real CLI processes before awaiting either and validates both complete outputs.
• S-3 — migration labels are misleading for disabled reviews/orphans: DEFERRED. This is diagnostic polish outside the approved transition states; included in the recommendation below.
• S-4 — stable requirement-ID regex was duplicated: ACCEPTED. Exported and reused one SAFE_REQUIREMENT_ID definition.
• Codex round 1: no findings.

Final round 2 findings

• No MUST-FIX findings from either reviewer. Both approve the final diff.
• S-1 — repository remains partially migrated and emits migration notices: DEFERRED. Same disposition as round-one MF-2; intentional transition behavior, but recommended as a follow-up dogfood cleanup.
• S-2 — overwritten stale stable destination is implicit in the migration action: REJECTED AS A DEFECT / DEFERRED AS DIAGNOSTIC POLISH. The conversion action names source and destination and satisfies the contract, but more explicit replacement output could help operators.
• S-3 — stableFiles has a permissive default parameter: DEFERRED. The sole current caller supplies it and final stable authority is correct; making the invariant mandatory is worthwhile API hardening.
• S-4 — dead custom-section normalization regex in a test helper: REJECTED. It is harmless test-helper cleanup with no behavioral or honesty impact and does not justify another diff/review cycle.
• S-5a — orphan legacy notices say “superseded”: DEFERRED. Combined with the diagnostic recommendation below.
• S-5b — auditRequested is single-use: REJECTED. The name keeps the audit-generation condition consistent and readable; it is not a defect.
• Codex round 2: no findings.

Additional non-blocking suggestions observed before the final CI-only round-two retry

• Missing-verdict diagnostics point at a legacy-style review-ID path: DEFERRED. A stable requirement path would be more actionable; included below.
• Stable verdict writes are not atomic against concurrent readers: DEFERRED. Distinct-requirement writer isolation is correct and satisfies REQ-012.2.1; temp-write-plus-rename is worthwhile hardening.
• A test remains annotated to tombstoned REQ-003.7.3: DEFERRED. The test is valuable and still runs, but its annotation can be re-homed during repository evidence cleanup.

Verification after accepted fixes

• npm test: 18 files, 176 tests passed.
• Repository gate under pipefail: 0 violations, 0 uncovered, 0 failing reviews, 0 stale reviews.
• PR CI: passed on fca9bae.
• Worktree: clean.

Suggested placeholder issues

These are recommendations for the operator to endorse, reject, or edit at the PR approval gate. No issue should be filed merely because it appears here.

Complete the repository’s dogfood verdict migration

Run the explicit 2119 prune migration in a dedicated reviewable change, verify the selected current evidence, remove the remaining legacy duplicates, and re-home the REQ-003.7.3 test annotation to a live filename/record-validation requirement. This was deferred because REQ-012 intentionally makes migration explicit and a repository-wide evidence rewrite would obscure the feature implementation diff.

Improve migration and missing-evidence diagnostics

Distinguish current, superseded, orphaned, and reviews-disabled legacy states; describe overwritten stable destinations explicitly during recovery; and point missing-verdict violations at the canonical <requirementId>.json path. The gate is already correct and fail-closed, so this is deferred operator-facing clarity rather than a correctness blocker.

Harden verdict I/O and internal authority APIs

Use temp-file-plus-rename for stable verdict writes/migrations to avoid torn reads, and make the stable-file authority map a required verdictViolations argument so future callers cannot silently disable the invariant. Current callers and the specified distinct-requirement concurrency behavior are correct; this is forward-looking robustness.