Make review audits automation-friendly

[tylerwillis]

Summary

• specify successful exit semantics for audit-only review runs
• preserve non-zero status when required judgment reviews are pending
• cover both boundaries with fresh-context-reviewed CLI tests

Plan

Approved task artifact: plan.md (panopticon://tasks/4ead402c475d4ca69af851de9455dd68/artifacts/plan.md)

Validation so far

• npm run build
• npx vitest run tests/rigor.test.ts
• node dist/cli.js check

Full local and CI gates run in the TESTING stage.

[tylerwillis]

Dual Review — Fable 5 (Round 1)

Review complete. I read the branch diff (3 commits vs main: spec + tests, the one-line CLI fix, and a verdict refresh), the governing spec, and the surrounding implementation (src/cli.ts, src/review.ts, src/hash.ts, src/annotations.ts, test helpers). Note on process: the sandbox blocked node/npx, so I could not execute the test suite or 2119 check; everything below is from static verification, including confirming dist/cli.js was rebuilt and contains the change.

MUST-FIX

None found. I actively looked for the failure modes this kind of change usually has, and each checks out:

• Spec conformance. The single changed line (src/cli.ts:129, process.exit(1) → break) sits in the branch reached only when tasks.length === 0 && auditTasks.length > 0, so audit-only runs now exit 0 (REQ-009.1.1). The pending-reviews path still ends in the untouched process.exit(1) at src/cli.ts:145, and audits are generated before that branch, satisfying REQ-009.1.2. The audit: always config path converges into the same auditTasks variable before the branch, so it's covered too.
• No collateral behavior change. The REQ-006.1 stale-instruction cleanup runs unconditionally inside generateInstructions (src/review.ts:122-127), before any exit branching — unaffected. Natural exit (falling out of the switch) is already the established pattern for the adjacent "all current" branch and for pass/init, and the readline interface is only created on a TTY and closed, so nothing holds the event loop open in CI/test runs.
• Verdict bookkeeping is consistent. REQ-003.2.2 is the only requirement whose review hash covers src/**, and it was correctly re-keyed after the cli.ts edit (commit 4daf9e7). The new REQ-009 verdicts' evidence (spec text + test annotation blocks) hasn't changed since they were recorded. I also checked the annotation-block hashing (src/annotations.ts:59-60): blocks run from an annotation line to the line before the next annotation, and the new tests were inserted at a block boundary, so neighboring requirements (e.g. REQ-004.1.10) were not silently re-keyed. That said, CI running 2119 check remains the authoritative confirmation, since I couldn't execute it.

SPEC/TEST HONESTY

Both new tests are genuine — they drive the real CLI binary and would fail if their requirement were violated:

• The REQ-009.1.1 test asserts status === 0 after a real review --audit run; under the pre-fix code that path exited 1, so this test fails on main. It also pins its precondition (audit file exists on disk, "adversarial audit(s) generated" in stdout), so it can't pass vacuously via the "all verdicts current, nothing generated" branch.
• The REQ-009.1.2 test constructs a genuine coexistence state (one passed requirement, one pending), asserts the audit file was actually written, and asserts status === 1 — a regression guard for the direction the fix could have overshot. No mocking anywhere; both tests exercise dist/cli.js end to end.

One honesty caveat worth stating rather than flagging: the REQ-009.1.2 test also passes on main (that behavior predates the fix). That's correct for a regression guard, just don't read the pair as "two tests the fix turned green."

SIMPLIFY

Nothing to flag. The production change is exactly one line, reuses the existing branch structure, and touches nothing beyond what the spec requires — this is the minimal implementation. The two tasks.length === 0 branches could technically merge now that both end in break, but they carry distinct user-facing messages and merging would trade clarity for three lines; not worth it.

SUGGESTIONS (ranked)

1. SUGGESTION — commit ordering (trivial effort, only matters pre-merge). Commit 9f56364 ships the REQ-009.1.1 test (and its passing verdict) before the fix lands in d893dc6, so the test suite is red at that intermediate commit — a git bisect hazard. Squashing the first two commits, or reordering fix-before-test-with-spec, would keep every commit green.
2. SUGGESTION — pin the audit-only branch message (5 minutes). The REQ-009.1.1 test could additionally assert stdout contains "no pending reviews; dispatch the audits above", tying the exit-0 to the specific branch the spec targets rather than inferring it from the audit-generation message. Marginal, since the current assertions already exclude the other exit-0 branch.
3. SUGGESTION — audit: always coverage (10 minutes, low value). The spec's motivation names audit: always automation, but neither test exercises the config-driven path. Because flag and config converge into one auditTasks computation before the exit decision, such a test would mostly re-test the same branch — add only if you want the config wiring itself pinned.

Overall: a tight, minimal change that does what REQ-009 asks, with honest end-to-end tests on both sides of the exit-status boundary.

[tylerwillis]

Dual Review — Sol 5.6 (Round 1)

1. MUST-FIX

None. The control-flow change satisfies both requirements:

• Audit-only runs reach the tasks.length === 0 branch and terminate naturally with status 0.
• Runs with pending judgment reviews retain the existing status 1 path.

2. SIMPLIFY

No actionable simplification. Replacing process.exit(1) with break is a minimal, localized change and avoids touching unrelated review-generation logic.

The REQ-003.2.2 verdict replacement is incidental churn, but appears necessary because modifying tests/rigor.test.ts changed that test’s review hash.

3. SPEC/TEST HONESTY

Both tests are honest:

• REQ-009.1.1: The real CLI is invoked, an actual passing verdict is recorded, an audit file must be generated, and status 0 is asserted. The test fails against the previous implementation because it returned status 1.
• REQ-009.1.2: The fixture contains one passed requirement and one pending requirement. It verifies both audit generation and status 1. An implementation that indiscriminately changed audit runs to exit 0 would fail this test.

The tests do not mock the relevant control flow and directly exercise each requirement’s observable criterion.

4. SUGGESTIONS

None. The implementation and coverage are appropriately scoped.

[tylerwillis]

Dual-review triage — Round 1

MUST-FIX

• Fable 5: none.
• Sol 5.6: none.

No must-fix was accepted, so no code change or second review round is required.

Fable 5 suggestions

1. REJECT — squash/reorder to keep every intermediate commit green. The two-commit split intentionally preserves the 2119 workflow boundary: approved spec + honest tests/verdicts first, implementation second. A red pre-implementation test is expected in SPECIFYING, and the final branch/gates are green.
2. REJECT — assert the full audit-only branch message. REQ-009 specifies exit status, not stable wording. The test already establishes the relevant branch behavior through a real passing verdict, actual audit-file creation, audit-generation output, and status 0; pinning prose would over-specify the CLI.
3. REJECT — duplicate REQ-009 through audit: always. Existing REQ-003.6.4 coverage already proves the config-driven path generates audits, while both flag and config feed the same auditTasks value before the exit branch. Adding the full cross-product would not materially improve defect detection for this small change.

Sol 5.6 suggestions

• None.

Final state

• Local suite: 113/113 passed.
• node dist/cli.js check: PASS, 0 violations/uncovered/stale.
• GitHub Actions: green on 4daf9e7abc39fd34bc1865369ed1cc1aa329d756.
• Surviving findings: none.