Make review findings the load-bearing artifact

[aaddrick]

Problem

Review findings are collected, counted, and then discarded. grep -n "\.issues" workflows/ticketmill.js finds exactly one consumer in the pipeline: the recordGateOutcome tally at :4669.

What the fix stages actually receive is reviewer prose:

• runQualityLoop :2962 passes String(rev.comments || rev.summary || 'No comments')
• reviewAndMerge :4680-4681 passes String(spec.comments || spec.summary) and String(code.comments || code.summary)
• the test loop :3484 passes String(v.comments || v.summary || 'Fix test quality issues')

And loop control never reads findings either:

• :2954 if (rev.result === 'approved')
• :3477 if (v.result === 'approved')
• :4667 spec.result === 'approved' && code.result === 'approved'

REVIEW_SCHEMA.issues at :638 is { type: 'array', items: {} }, untyped, with no severity field. That is why gate_findings['pr-review'].severity reports zeros across the board, documented as expected at :2021-2027.

Two consequences. Nothing downstream can address a specific finding, because findings have no identity. And a gate cannot terminate on findings being resolved, only on a reviewer changing its own verdict string.

Change

1. Type REVIEW_SCHEMA.issues with the shape CHALLENGE_SCHEMA.findings already uses at :599: objects with severity (critical/major/minor), summary, and an optional recommendation. Give each finding a stable id within its gate iteration so later stages can reference one.
2. Ask for it in the prompts that produce a REVIEW_SCHEMA result: spec review :4613, code review :4633, quality review :2934, test validation :3452.
3. Feed the three fix stages from the structured array instead of the prose blob. Keep rendering comments as context, but the findings array becomes the input of record.
4. Move the three loop predicates to "approved, or zero surviving findings."

Acceptance criteria

• REVIEW_SCHEMA.issues is typed and the four producing prompts ask for it.
• All three fix stages receive the structured findings; comments still appears as context.
• All three loop predicates accept a clean pass on either condition.
• gate_findings['pr-review'].severity reports real counts in a live run, and the note at :2021-2027 is updated to match.
• A reviewer that returns changes_requested with an empty findings array still terminates the loop rather than running a fix stage with nothing to fix.
• A reviewer that omits issues entirely degrades to today's behaviour rather than throwing. recordGateOutcome already tolerates a missing severity.
• node --test stays green. Extend tests/pr-review-gate.test.js, tests/quality-loop.test.js, tests/test-loop.test.js, and tests/gate-findings.test.js.

Why this one leads

Every other structural improvement in this set needs finding identity. A fix stage cannot rebut a specific finding if it received a prose blob, and a re-raised finding cannot be detected across gates if findings are not addressable. It also removes the severity blind spot at the merge gate, which is currently the only gate whose findings carry no severity at all.

Out of scope

No suppression or filtering of findings. This issue only makes findings load-bearing. Whether volume needs reducing is a separate question that needs data this change produces.

Documentation target

Document in CHANGELOG.md and in a new page docs/architecture/gate-hygiene.md (create it), adding its row to docs/architecture/index.md's file map.

Do NOT edit docs/architecture/pipeline.md, metrics.md, or failure-semantics.md. Their moved prose is hash-frozen by tests/architecture-provenance.test.js, and since the tech-docs stage runs at :4707, after the last test-loop execution, a frozen-prose edit surfaces only as a red CI check on the batch PR. New pages are authored text and are explicitly allowed; see docs/architecture/CLAUDE.md.

[aaddrick]

Ticketmill Claimed

batch: Batch_2026-07-27_225225
run: 2026-07-27-gate-hygiene-t1
host: tanngrisnir
started: 1785207408
Queued in an active ticketmill run — will be processed even if work has not visibly started (concurrency queue). Claims older than 12h are stale.

[aaddrick]

Starting Automated Processing

Batch branch: Batch_2026-07-27_225225 (integration target)

Pipeline:

1. Research
2. Evaluate + Contrarian
3. Plan + Contrarian
4. Tasks with quality loops
5. Tests
6. Docs
7. PR
8. Review
9. Merge

[aaddrick]

Evaluation: Best Path

Complexity: complex. Not for the size of the diff, which is small, but for what it moves: the merge gate can now end without a reviewer having said "approved". That is control flow, it is the last gate before a squash-merge, and four live agent prompts change shape in the same pass.

Approach

One helper, four schema lines, four prompt edits, three predicate edits, and a comment rewrite.

1. normalizeFindings(raw, source), next to recordGateOutcome.

null                      when `raw` is not an array (reviewer omitted `issues`)
[{id, severity, summary, recommendation}, ...]   otherwise, one entry per input entry

Two properties carry the whole design.

Null is not empty. null means "this reviewer gave us no structured findings, use the prose" and keeps today's behaviour exactly. [] means "this reviewer looked and found nothing" and terminates the loop. Acceptance criteria 5 and 6 read as a contradiction until you notice they are asking for two different values, and JavaScript can tell undefined from [].

Arity is preserved. Every raw entry maps to exactly one finding. A malformed entry keeps its slot with severity: 'unspecified' (which recordGateOutcome already ignores) and summary: String(entry) for a bare string. Normalization that filtered junk would let a garbage array read as zero findings and approve a PR, and it would make gate_findings.count disagree with what the reviewer actually emitted.

Ids are engine-assigned, source + '-' + (i + 1), e.g. code-i2-3. Not model-supplied: spec review and code review run in parallel() at the merge gate and would both name their first finding F1, and the harness's scripted agents need determinism. The issue asks for a stable per-iteration id, and this gives one that is stable and unique across the two reviewers, which a model-chosen id is not.

2. Schema. issues.items gets required: ['severity', 'summary'] with severity as the same three-value enum CHALLENGE_SCHEMA.findings uses, plus optional recommendation. issues itself stays out of REVIEW_SCHEMA.required, for the reason already written above CHALLENGE_SCHEMA at :592: models drop the array under long output and control flow has to survive that. No id in the schema, since the engine assigns it and inviting a value we overwrite is worse than not asking.

3. Prompts. The four producing sites get the sentence the contrarian prompts already carry at :3846 and :4218, sharpened: every concern goes in the issues array, a concern that appears only in comments will not be fixed, and no findings means an empty array. This is the load-bearing mitigation for change 4, not a nicety.

4. Fix stages. A shared findingsBlock() renders - [id] [severity] summary -> recommendation, the same line shape as :4259. It goes first, the prose comments follows under a context heading. When findings is null, the prompt is byte-identical to today's.

5. Predicates. clean = result === 'approved' || (findings !== null && findings.length === 0), applied per reviewer at the merge gate and ANDed, so a spec reviewer with two findings still blocks a code reviewer with none. The pr-review tally switches to the normalized arrays, which is what makes severity report real counts.

Where a loop exits on the empty-array branch rather than a real approval, push a deferred note. A PR that merges over a changes_requested verdict should be visible to the human reading the batch PR, not silent.

6. Housekeeping. Rewrite :2021-2027. New docs/architecture/gate-hygiene.md plus its row in index.md's file map (authored text, editable). Nothing else under docs/architecture/. CHANGELOG.md, plugin.json to 0.1.42, and node scripts/lint-engine.js --fix in the same commit per the profile's LOCKSTEP-EDIT RULE, which is the sanctioned way the .claude/workflows/ copy moves.

Tests. Empty array terminates each of the three loops (script no fix stage; the responder throws on an unscripted key, which is the proof). Omitted issues still runs the fix stage, which is the regression guard on today's behaviour. The fix prompt contains the id, severity and summary, and still contains the prose. gate_findings['pr-review'].severity reports a real mixed-severity count across both reviewers. A malformed entry does not throw and keeps its slot.

Risks

1. The gate genuinely gets weaker. A reviewer writing complaints in prose and returning issues: [] now merges. Prompt wording is the only defence, and prompt wording is not testable. First live run is the real check.
2. A tightened schema can kill a reviewer. An item-level enum violation invalidates the whole result; two failed attempts at the merge gate is needs_human. Keeping issues optional and requirements to two fields is the hedge.
3. The severity acceptance criterion cannot be closed by node --test. Unit tests prove the plumbing; only a live run proves reviewers fill it.
4. Reviewers may inflate the array now that it is the thing that counts. Volume is explicitly out of scope here, but this change is what produces the data to judge it.
5. Lockstep drift if --fix is skipped.

Alternatives rejected

• Model-supplied ids: collide across the two parallel merge-gate reviewers, and are not stable across iterations anyway.
• Predicate on zero critical/major (mirroring the contrarian gates): silently stops fixing minor findings. Bigger weakening than asked for.
• Treat missing issues as []: terminates on the single most common reviewer degradation, disabling all three fix stages.
• Reuse CHALLENGE_SCHEMA and rename issues to findings: churns four prompts and recordGateOutcome for a cosmetic win.
• Drop comments from fix prompts: it is the only rationale carrier when the array is absent, and the issue requires keeping it.
• Filtering normalization: a garbage array would read as clean and approve.
• Per-finding rebuttal records now: that is the next issue. This one only has to make findings addressable.

[aaddrick]

Contrarian: Approach Challenge (Iteration 1)

Verdict: needs_rework — one major finding. Everything else is caveat-level, and the housekeeping half of the plan verified clean.

Strengths (steel-man)

The null-vs-[] insight is the right read of the issue. Acceptance criteria 5 and 6 genuinely do look contradictory until you notice they name two different values, and building the whole design on that distinction is correct. Arity preservation is the right call for the same reason the evaluation gives: a filtering normalizer would let a garbage array read as zero findings at the merge gate. Engine-assigned ids are right, and the collision argument holds — spec review and code review really do run through parallel() at :4610, so two model-chosen F1s would land in the same tally.

I checked the housekeeping claims and they hold:

• docs/architecture/index.md's provenance segment is firstLineOutput: "# Ticketmill Architecture", lines: 6 — the title and lede only. The file map starts at line 7 and is outside every hash in tests/architecture-provenance.test.js. Adding a row is safe.
• tests/readme-docs-census.test.js filters on /^docs\/[^/]+\.md$/, so a file at docs/architecture/gate-hygiene.md is not in its census set.
• Two-hop reachability holds: docs/index.md:28 links architecture/index.md, which will link gate-hygiene.md.
• .claude-plugin/plugin.json is at 0.1.41, so 0.1.42 is the right bump, and scripts/lint-engine.js --fix writes source over copy for both lockstep pairs, as the profile rule at .claude/ticketmill.json:16 requires.

Ledger evidence also supports the change more strongly than the evaluation claims. logs/ticketmill/runs/2026-07-26-docs.json records pr-review severity {critical:0, major:1, minor:2} on a count of 5, and tier4-94.json records minor:1 on a count of 3. Reviewers are already volunteering severity inside the untyped array. The note at :2021-2027 is not merely about to become stale — it is already wrong.

Findings

[Major] The empty-findings escape hatch turns a changes_requested verdict into an autonomous squash-merge, and the new prompt sentence raises the trigger rate rather than lowering it.

• Assumption challenged: that AC 3's "clean pass on either condition" applies to the merge gate the same way it applies to the two internal loops, and that prompt wording is an adequate defence.
• Failure scenario: a spec reviewer writes three objections into comments, and — obeying the new instruction "no findings means an empty array" — emits issues: [] alongside result: 'changes_requested'. The new predicate reads clean, approved = true at :4671, and the run proceeds to gh pr merge --squash --delete-branch at :4780. Today the identical result runs a pr-fix stage on the prose and re-reviews. The null-vs-[] hinge only protects a reviewer that omits the key, which is precisely the habit the new prompt sentence trains away. The design's safety mechanism and its prompt change point in opposite directions.
• Impact: the last gate before an autonomous merge can be overridden by the engine on a verdict that explicitly asked for changes. The evaluation names this as risk 1 and mitigates it with a ctx.deferred note, which is post-hoc narration on an already-merged PR.
• Recommendation: split the mapping instead of applying one predicate three times. Keep the permissive form in runQualityLoop and the test loop — they are internal and the merge gate still backstops them. In reviewAndMerge, treat changes_requested plus an empty findings array as loop termination without approved = true, landing on the existing needs_human path at :4700. AC 5 requires only that the loop terminate "rather than running a fix stage with nothing to fix"; it does not require a merge, and needs_human is strictly safer than both today's behaviour and the proposal.

[Minor] Tallying the empty-findings exit as disposition: 'accepted' makes gate yield report the opposite of what happened, and a new disposition string would be silently dropped.

• Failure scenario: computeGateYield at :5317-5320 hardcodes exactly four disposition names, and :5356 computes ratio = accepted / (accepted + dismissed). An exit that merged over a changes_requested verdict counted as accepted inflates the one metric meant to measure gate quality — in the issue whose subject is gate hygiene. Introducing a fifth name instead (say accepted-no-findings) without touching computeGateYield drops that iteration from every bucket, so the gate looks like it never ran.
• Recommendation: if a distinct disposition is used, wire it into computeGateYield's buckets and the df44b9d report renderer in the same commit. Also widen the comment rewrite: the approach names only :2021-2027, but the accepted definition at :1994-1999 ("both spec and code review returned 'approved'") and carried-unresolved at :2004-2007 go stale under the new predicate too.

[Minor] A ctx.deferred note on the empty-array exit becomes an auto-filed GitHub issue.

• Failure scenario: the merge stage at :4789 instructs the agent to scan "the deferred suggestions below" and "create one GitHub issue per distinct actionable item," and the same block is rendered into the Implementation Complete comment at :4781. runQualityLoop runs once per task and again per pr-fix iteration, so this can fire several times in one issue and spawn near-duplicate follow-ups into a backlog this repo mills autonomously.
• Recommendation: carry the signal as a metric plus a pushDecision timeline entry, which is where gate narrative already lives, rather than through ctx.deferred. If it must go in ctx.deferred, word it as an observation with no action verb so the follow-up scan does not match it.

[Minor] "The responder throws on an unscripted key, which is the proof" is not how stage() behaves.

• Failure scenario: stage() at :1776-1782 catches every agent throw, logs it, retries STAGE_TRIES times, then calls recordAgentDeath() and returns null. A fix stage that wrongly runs therefore surfaces as a degrade or a needs_human, not as the thrown assertion, and enough of them trip the global STOP breaker at :1665 — which would make the failure read as "halted," not "the fix stage ran."
• Recommendation: assert positively. Have the scripted responder record every label it sees, then assert the fix label is absent and assert the loop's own return value ('approved', {ok: true}, status: 'merged'). Keep the throw as a backstop, not as the proof.

[Minor] The severity: 'unspecified' coercion is probably unreachable in production, and the tests covering it prove less than they appear to.

• Failure scenario: the schema is passed to the real agent runtime at :1777, so an out-of-enum severity is a validation failure that yields a retry and then a null result — not a value normalizeFindings ever sees. The vm harness's scripted agents bypass validation entirely, so a "malformed entry keeps its slot" test exercises the helper and nothing else. The real merge-gate risk from tightening the item schema is a reviewer that returns null twice.
• Recommendation: keep the coercion as defence-in-depth and say so in the comment, but add the test that covers the actual risk: a null from either merge-gate reviewer still reaches needs_human at :4653 and never merges. Note that CHALLENGE_SCHEMA at :599 already requires three fields per item and survives in production, so the enum itself is low-risk — the hedging in the evaluation is aimed at the wrong hazard.

Verdict

needs_rework, on the strength of the first finding alone. The fix is small — one predicate placement at reviewAndMerge rather than three identical ones — and the rest of the approach can proceed unchanged.

[aaddrick]

Revised Evaluation (after contrarian iteration 1)

All five findings verified against the tree. All five confirmed; all five adopted. The central design (null for absent, [] for present-and-empty) is unchanged. What changes is that the empty-array predicate is no longer applied uniformly at all three gates.

[major] The permissive predicate at the merge gate — CONFIRMED, adopted

Verified: :4671 if (prReviewClean) { approved = true; break } leads to the browser re-check, tech docs, and the squash-merge stage. Making prReviewClean true on changes_requested + issues: [] is an autonomous merge on a reviewer that asked for changes. :4701 if (!approved) return fail(ctx, 'needs_human', 'pr-review', ...) is the existing termination that AC 5 is already satisfied by. AC 5 asks that the loop terminate rather than run a fix stage with nothing to fix; it does not ask for a merge.

The predicate is split rather than shared:

• runQualityLoop :2954 and the test loop :3477 keep the permissive form — approved-equivalent, exiting 'approved' / { ok: true }. Both are backstopped by the merge gate, and in the quality loop specifically a non-approved break returns 'degraded', which increments quality_degrades and feeds the halt window at :2982-2988. Tallying "reviewer had nothing to fix" as a degrade would trip a halt on a clean state.
• reviewAndMerge gets a distinct exit. Per reviewer, nothingToFix(r, f) = r.result === 'approved' || (f !== null && f.length === 0). If both reviewers have nothing to fix but prReviewClean is false, the loop breaks WITHOUT setting approved, landing on :4701. One reviewer's real findings still send the iteration into the fix stage, so the AND is preserved in both directions.
• :4701's reason string currently reads "not approved after N iterations", which would be false on an early break. A small haltReason local carries the accurate text for each of the two paths into the one fail() call.

Second half of the finding also confirmed and adopted: the proposed prompt sentence ("no findings means an empty array") manufactures the state it is being defended against. Replacement wording ties the array to the verdict instead of to the count:

Every concern you want fixed goes in issues, one entry per concern, with severity, summary, and a recommendation. A concern that appears only in comments will not be fixed. If you return changes_requested, issues must be non-empty — if you have nothing that must be fixed, return approved.

That makes changes_requested + [] an incoherent output the prompt actively discourages, rather than one it invites. The empty-array branch stays as the safety net for reviewers that produce it anyway, which is what AC 5 asks for.

[minor] Gate-yield disposition — CONFIRMED, adopted, and resolved without a new string

Verified at :5316-5320: computeGateYield reads exactly four disposition keys (accepted, dismissed, re-litigated, carried-unresolved) and the df44b9d renderer at :5378 prints exactly those columns. A fifth string would be tallied by recordGateOutcome and then silently dropped by both.

Rather than add a string and wire it through two more places, the empty-findings exit records 'carried-unresolved' — which is what actually happened: the gate did not settle cleanly and the PR is left open for a human. Same outcome, same column, no renderer change, no drop.

Comment rewrite widened past the severity note as the finding asks. Three edits at :1994-2027:

• accepted at :1996-1999 currently says "for pr-review, both spec and code review returned 'approved' (the same condition that ends the loop)". The parenthetical goes stale — the loop now has a second exit. Reworded to state that accepted requires both approvals, and that it is no longer the only way the loop ends.
• carried-unresolved at :2001-2007 currently says "the gate's iteration cap was reached without a clean pass". Widened to "the gate did not reach a clean pass — either the iteration cap was reached, or both reviewers requested changes while naming no findings to fix", both landing on the same needs_human at :4701.
• The severity NOTE at :2021-2027 is rewritten as planned: REVIEW_SCHEMA.issues is typed, the prompts ask for severity, and gate_findings['pr-review'].severity now carries real counts.

[minor] ctx.deferred on the empty-array exit — CONFIRMED, adopted

Verified: ctx.deferred is rendered into deferredBlock at :4756 and fed to the merge stage's step 5 at :4788-4791, which instructs "Create one GitHub issue per distinct actionable item". A note pushed once per quality-loop scope plus once per test iteration plus once at the merge gate is several auto-filed issues per run. Dropped entirely.

Replaced with:

• ctx.metrics.findings_empty_exits, a new counter on the metrics literal at :4846. It rides the existing metrics: ctx.metrics serialization at :2734 and :4811 with no serializer change, and it is deliberately NOT added to FRICTION_WEIGHTS at :2383, so the friction score is byte-identical to today's for any run where the branch never fires.
• One pushDecision(ctx, 'Gate: no findings to fix', ...) at each firing site, which is where gate narrative already lives (:3141, :3368, :3391, :4221) and which surfaces in timeline(ctx) without filing anything.

[minor] The "responder throws" test proof — CONFIRMED, adopted

Verified at :1776-1782: stage() wraps the agent() call in a try/catch, logs the throw, retries to STAGE_TRIES, then recordAgentDeath() and returns null. A throw from installScriptedResponder is an agent death, not a test failure. It fails in the direction that flatters the test: in runQualityLoop a null review sets degraded = true and breaks at :2950 — the loop exits without running a fix stage, which is exactly the assertion, reached for exactly the wrong reason.

Tests assert positively instead. The scripted responder pushes every stage key it is asked for onto a seen array before dispatching. Each empty-array scenario then asserts:

• seen does not contain the fix key (quality-fix-<prefix>-i1, test-quality-fix-i1, pr-fix-i1), and for the merge-gate scenario does not contain merge either;
• the loop's own return value: runQualityLoop returns 'approved', the test loop returns { ok: true }, and reviewAndMerge returns status: 'needs_human' with stage: 'pr-review' and no status: 'merged';
• ctx.metrics.findings_empty_exits incremented, and gate_findings['pr-review'].disposition['carried-unresolved'] is 1 for the merge-gate case.

The throw stays in the responder as a backstop for unscripted keys. It is no longer cited as the proof of anything.

[minor] The 'unspecified' coercion and the real hazard — CONFIRMED, adopted

Verified at :599: CHALLENGE_SCHEMA.findings.items requires all three of severity, summary, recommendation and runs in production against every contrarian gate. The enum is proven, so the hedging in the prior evaluation was aimed at a hazard that does not exist. The coercion stays as defence-in-depth against a reviewer whose issues never went through item validation, and its source comment says exactly that rather than implying it is a live path. Its unit test moves to tests/gate-findings.test.js as a normalizeFindings unit test and stops being presented as integration coverage.

The real merge-gate hazard gets the test instead. Verified at :4656: if (!spec || !code) return fail(ctx, 'needs_human', 'pr-review', ...). Two new scenarios in tests/pr-review-gate.test.js — a null spec reviewer, and a null code reviewer — each asserting needs_human, stage: 'pr-review', and that merge never appears in seen. That is the path a schema tightening could plausibly start exercising, and it is currently untested at this call site.

On item required: ['severity', 'summary'] plus optional recommendation, deliberately one field looser than CHALLENGE_SCHEMA. A finding missing its recommendation still renders a usable fix-stage line; a finding missing its summary does not. issues itself stays out of REVIEW_SCHEMA.required, and id stays out of the schema entirely — the engine assigns it.

Unchanged from the prior evaluation

normalizeFindings(raw, source) returning null for an omitted issues and a one-entry-per-input array otherwise, with engine-assigned source + '-' + (i+1) ids. The shared findingsBlock() renderer reusing the - [severity] summary -> recommendation line shape from :4259 with the id prefixed, prose comments retained below under a context heading, and a byte-identical-to-today prompt when findings is null. recordGateOutcome(ctx, 'pr-review', ...) fed the normalized arrays (a null normalizes to [] for the tally, matching today's spec.issues || []). Housekeeping: docs/architecture/gate-hygiene.md plus its row in docs/architecture/index.md's file map (segment 0 covers only the first six lines, above the table), CHANGELOG.md, .claude-plugin/plugin.json 0.1.41 -> 0.1.42, and node scripts/lint-engine.js --fix in the same commit.

[aaddrick]

Contrarian: Approach Challenge (Iteration 2)

Verdict: sound_with_caveats — the iteration-1 major is closed, verified against the tree. Four minor findings remain, three of them about text (a doc inventory, a stale rationale, a distorted metric) and one about a test-harness key the plan doesn't mention.

Marker check: every comment on this issue carries <!-- ticketmill aaddrick/ticketmill#162 -->. Nothing misfiled, nothing deleted.

Strengths (steel-man)

The major finding was adopted rather than argued around, and the shape it landed in is the right one. I re-checked the control flow it depends on:

• reviewAndMerge's loop header is for (let iter = 1; iter <= MAX_PR_REVIEW_ITERATIONS && !approved; iter++), and the only exits after it are if (!approved) return fail(ctx, 'needs_human', 'pr-review', ...) at :4701. A break that does not set approved therefore cannot reach the browser re-check, tech docs, or gh pr merge --squash. The proposed exit does what it claims.
• AC 5 forecloses the softer alternative I would otherwise have proposed (fall back to today's prose fix path on iteration 1 rather than escalating): it requires the loop to terminate rather than run a fix stage with nothing to fix. needs_human is the only termination available at that gate. The revision is not over-correcting.
• The prompt rewrite genuinely inverts the trap. "If you return changes_requested, issues must be non-empty — if you have nothing that must be fixed, return approved" makes the dangerous state incoherent instead of instructing reviewers into it.

Two more checks that came back clean and are worth recording because they were the obvious places for this to break:

• No orphan schema site. REVIEW_SCHEMA is referenced at exactly four call sites (:2951, :3474, :4630, :4651) — the same four the plan rewrites. :4481 uses TASK_REVIEW_SCHEMA, which has no issues at all. Tightening issues.items cannot strand a prompt that wasn't updated.
• No existing test fixture flips. Every scripted changes_requested in the suite carries a non-empty issues (pr-review-gate.test.js:107,145, quality-loop.test.js:65,144, test-loop.test.js:106). The last three use issues: ['x'] — bare strings — so normalizeFindings' string branch is exercised by the existing suite rather than only by a new unit test, and those loops keep running their fix stages exactly as they do today.

Findings

[Minor] The freeze pair docs/architecture/AGENTS.md / CLAUDE.md carries its own file inventory, and the plan explicitly excludes it.

• Assumption challenged: that index.md's file map is the only inventory a new file under docs/architecture/ has to appear in. The revised plan says "Nothing else under docs/architecture/".
• Failure scenario: AGENTS.md opens with "These ten files used to be one file", carries a "What lives here" table with a row per page, and says the provenance test "only knows about the ten files". Adding gate-hygiene.md makes all three stale. Worse, tests/docs-links.test.js:246-265 asserts every tracked <dir>/AGENTS.md is byte-identical to its sibling CLAUDE.md, so an implementer who notices the staleness and fixes only AGENTS.md turns node --test red.
• Impact: either a stale directory guide that the next tech-docs agent reads as authoritative, or a red suite from a half-applied fix.
• Recommendation: add the row to both halves of the pair and reword "ten files", finishing with cp docs/architecture/CLAUDE.md docs/architecture/AGENTS.md as that file's own parity note instructs. Or drop the new file entirely and append the section to docs/architecture/metrics.md, already scoped to "Friction and churn, rework tax, gate yield, and outcome grading" — appended authored text sits outside every tracked segment, so provenance is unaffected either way.

[Minor] The escaped-defect signal is defined on raw finding count, and this change is designed to raise exactly that count on one side of the comparison.

• Assumption challenged: that making pr-review findings truthful only improves the observability this issue exists to serve.
• Failure scenario: computeGateYield computes escaped = escapeCount > 0 && earlyCount === 0 && !earlyOnlyDismissed at :5341, where escapeCount is gate_findings['pr-review'].count — all findings, any severity. The four rewritten prompts explicitly instruct reviewers to put every concern in issues. Across the five run records in logs/ticketmill/runs/, 8 units carry gate_findings; escaped fired 0 times, and 6 of the 8 recorded pr-review count 0. Two of those units (Enrich ctx with retained changed-files, friction fields, gate findings, and a completeness score #87, Proactive token cost estimator + budget guard (stop before the account ceiling, not after) #97) also recorded zero approach/plan findings — so a single post-change code-review nit on a unit like those flips it into the "Escaped defects" table, in the very report whose comment calls that count "the one signal in this engine with plausible coupling to what actually ships".
• Impact: the first post-change batch report reads as an escaped-defect regression that is entirely an artifact of the reviewers finally filling in the array.
• Recommendation: cheapest correct fix is now available for the first time precisely because of this change — compute escapeCount from severity.critical + severity.major rather than count. If that is out of scope, say so in gate-hygiene.md in as many words, so the next reader of a Gate Yield table knows the baseline moved.

[Minor] The stated reason for keeping the permissive predicate in runQualityLoop is not true of the code, and the test the plan proposes asserts a value nothing consumes.

• Assumption challenged: "a non-approved break returns 'degraded', which increments quality_degrades and feeds the halt window at :2982-2988, so tallying a clean state as a degrade would trip a halt."
• Failure scenario: degraded is set only by a dead/errored review, simplify, or fix stage (:2927, :2951, :2973). A plain break leaves it false, so ctx.degrades.push(degraded) pushes false and if (degraded) ctx.metrics.quality_degrades++ never fires — the halt window is untouched. And runQualityLoop's return value is read at only two sites (:4528, :4698), both if (q === 'halted'). Permissive and strict are behaviorally identical here; the only observable difference is a string no production code reads.
• Impact: the false rationale goes into a source comment and gate-hygiene.md and is copied forward as fact by the next person reasoning about the halt window. Separately, "runQualityLoop returns 'approved'" is the weakest of the three proposed assertions.
• Recommendation: keep the permissive form — 'approved' is the honest label for "the reviewer had nothing to fix" — but rest the justification on the leg that holds (both internal loops are backstopped by the merge gate, which is where the strict exit now lives). In the quality-loop test, assert the fix key is absent from seen and that ctx.metrics.quality_degrades is still 0, and treat the return string as a bonus.

[Minor] ctx.metrics.findings_empty_exits has a second home the plan doesn't name, and missing it makes the new assertions fail as NaN.

• Assumption challenged: that adding the counter to the metrics literal at :4846 is the whole change, since it rides the existing serialization at :2734/:4811.
• Failure scenario: tests/harness.js:82-89 hand-duplicates that literal as freshMetrics(), and harness.makeCtx builds every fixture ctx from it. Add the key to the engine only and every harness-driven ctx lacks it, so ctx.metrics.findings_empty_exits++ evaluates undefined + 1 = NaN — in exactly the tests the plan writes to prove the counter. tests/run-record.test.js:71 also derives its required-key list from freshMetrics(), so the parity is load-bearing beyond this one counter.
• Impact: self-announcing (the suite goes red immediately), but it costs a debug cycle in a run that is already spending its budget on control-flow tests.
• Recommendation: add findings_empty_exits: 0 to tests/harness.js's freshMetrics() in the same commit as the engine literal, and keep the two ordered identically so the duplication stays visibly a mirror.

Verdict

sound_with_caveats. The empty-findings exit at the merge gate no longer merges, the schema tightening has no unprompted call site, and no existing fixture changes meaning. The four items above are text and test-harness hygiene; none of them blocks starting.

[aaddrick]

Implementation Plan

Three tasks. Two engine tasks split by gate so no helper lands unused, then one docs task.

Task 1 builds the foundations and wires the two internal loops. REVIEW_SCHEMA.issues.items gets a real shape, normalizeFindings() and findingsBlock() land next to recordGateOutcome, and the quality loop and test loop start reading the array instead of the prose. Task 2 wires the merge gate, where an empty findings array terminates the loop onto needs_human rather than merging, and rewrites the stale disposition comment. Task 3 writes docs/architecture/gate-hygiene.md and repairs the two file inventories a new page under that directory makes stale.

CHANGELOG.md and .claude-plugin/plugin.json are left to the release stage, which this repo's profile has configured.

Full plan

The hinge the whole design rests on

issues absent and issues: [] are different values and get different treatment. Absent means the reviewer never opted in, so the engine falls back to today's prose path byte-for-byte. Present-and-empty means the reviewer looked and found nothing, which is a signal worth acting on. normalizeFindings returns null for the first and an array for the second, and every predicate below reads that difference.

Task 1 — schema, helpers, and the two internal loops

Files: workflows/ticketmill.js, tests/harness.js, tests/gate-findings.test.js, tests/quality-loop.test.js, tests/test-loop.test.js.

1. Type REVIEW_SCHEMA.issues.items at :637-638:

   issues: { type: 'array', items: { type: 'object', required: ['severity', 'summary'], properties: {
     severity: { enum: ['critical', 'major', 'minor'] }, summary: { type: 'string' }, recommendation: { type: 'string' },
   } } },

   Deliberately one field looser than CHALLENGE_SCHEMA.findings at :599: a finding missing its recommendation still renders a usable fix-stage line, a finding missing its summary does not. issues stays OUT of REVIEW_SCHEMA.required, and id stays out of the schema entirely, since the engine assigns it.

2. Add normalizeFindings(raw, source) next to recordGateOutcome (~:2028). Returns null when raw is not an array. Otherwise arity-preserving, one output entry per input entry: id = source + '-' + (i + 1), severity coerced to critical/major/minor or 'unspecified', summary and recommendation carried through, a bare-string entry becoming { summary: String(entry) }. Arity is preserved rather than filtered so a garbage array cannot read as zero findings at the merge gate. The 'unspecified' branch is defence-in-depth against an array that never went through item validation, and its source comment says so rather than implying it is a live production path.

3. Add findingsBlock(findings, comments, fallbackLabel), a shared fix-stage renderer. When findings is null it emits exactly what the site emits today (String(rev.comments || rev.summary || '<fallback>')) so the omit path is a byte-identical prompt. Otherwise it reuses the line shape already at :4259, with the id prefixed:

   - [code-i2-3] [major] summary text -> recommendation text
   

   and keeps the prose comments below it under a context heading, marked as context rather than as the work list.

4. Add findings_empty_exits: 0 to the metrics literal at :4846 and to freshMetrics() in tests/harness.js:82-89, in the same position in both. The harness hand-mirrors that literal and tests/run-record.test.js:71 derives its required-key list from it, so a one-sided add makes ctx.metrics.findings_empty_exits++ evaluate to NaN in exactly the new tests. Do NOT add it to FRICTION_WEIGHTS at :2384: the friction score must stay byte-identical for any run where the branch never fires.

5. Quality review prompt at :2934-2952 and test validation prompt at :3452-3473 both gain the same closing wording, which ties the array to the verdict instead of to the count:

   Every concern you want fixed goes in issues, one entry per concern, with severity, summary, and a recommendation. A concern that appears only in comments will not be fixed. If you return changes_requested, issues must be non-empty. If you have nothing that must be fixed, return approved.

6. runQualityLoop: normalize rev.issues with a per-iteration source, then

   if (rev.result === 'approved' || (f !== null && f.length === 0)) { approved = true; ... break }

   On the empty-array leg only, increment ctx.metrics.findings_empty_exits and pushDecision(ctx, 'Gate: no findings to fix', ...). The permissive form is right here because this loop is backstopped by the merge gate, where the strict exit lives. Feed the quality-fix stage at :2957-2962 from findingsBlock.

7. Test loop at :3477: same predicate, returning { ok: true }; test-quality-fix at :3479-3484 fed from findingsBlock.

8. Tests.

   • tests/gate-findings.test.js: normalizeFindings units covering omitted -> null, [] -> [], id assignment across a multi-entry array, bare-string entry, out-of-enum severity -> 'unspecified', arity preserved for a malformed entry. findingsBlock units covering the null -> today's-prose path and the rendered-line shape.
   • tests/quality-loop.test.js: a reviewer returning changes_requested with issues: [] asserts positively, not by throwing. The scripted responder records every stage key it is asked for; assert quality-fix-task-1-i1 is absent from that list, runQualityLoop returns 'approved', ctx.metrics.quality_degrades is still 0, and findings_empty_exits is 1.
   • tests/test-loop.test.js: the same scenario asserting { ok: true }, no test-quality-fix-i1 key, and the counter.
   • Regression in both: the existing issues: ['x'] fixtures still run their fix stage, and the fix prompt now contains the rendered finding line.

9. Run node scripts/lint-engine.js --fix in the same commit (the profile's LOCKSTEP-EDIT RULE; --fix always writes source over copy, never the reverse), then the full test_command.

Task 2 — the merge gate

Files: workflows/ticketmill.js, tests/pr-review-gate.test.js.

1. Spec review prompt at :4613-4629 and code review prompt at :4633-4650 get the identical wording from task 1 step 5.

2. Normalize both reviewers' arrays with distinct sources ('spec-i' + iter, 'code-i' + iter). Ids must not collide: the two reviews run through parallel() at :4610 and land in one tally.

3. recordGateOutcome(ctx, 'pr-review', ...) is fed the normalized arrays, with null flattened to [] so the tally matches today's spec.issues || [].

4. Per reviewer, nothingToFix(r, f) = r.result === 'approved' || (f !== null && f.length === 0). prReviewClean keeps its current meaning, both reviewers approved, and remains the ONLY path that sets approved = true. When both reviewers have nothing to fix but prReviewClean is false, record the iteration as 'carried-unresolved', increment findings_empty_exits, pushDecision, and break WITHOUT setting approved, landing on the existing needs_human at :4701. One reviewer with real findings still sends the iteration into the fix stage, so the AND is preserved in both directions. A haltReason local carries accurate text for each of the two paths into that one fail() call, since "not approved after N iterations" is false on an early break.

5. Feed pr-fix at :4675-4681 from findingsBlock per reviewer, keeping the two prose blocks below as context.

6. Rewrite the doc comment at :1994-2027, three edits:

   • accepted (:1996-1999): drop "the same condition that ends the loop", since the loop now has a second exit; state that accepted still requires both approvals.
   • carried-unresolved (:2001-2007): widen to "the gate did not reach a clean pass, either the iteration cap was reached, or both reviewers requested changes while naming no findings to fix", both landing on the same needs_human.
   • the severity NOTE (:2021-2027): rewrite. REVIEW_SCHEMA.issues is typed, the four prompts ask for severity, and gate_findings['pr-review'].severity now carries real counts.

7. Tests in tests/pr-review-gate.test.js:

   • both reviewers changes_requested with issues: [] -> needs_human with stage: 'pr-review', and neither pr-fix-i1 nor merge appears in the recorded stage keys; gate_findings['pr-review'].disposition['carried-unresolved'] is 1.
   • one reviewer empty, the other with real findings -> pr-fix-i1 still runs.
   • a null spec reviewer, and separately a null code reviewer -> needs_human, stage: 'pr-review', merge never runs. This is the path a schema tightening could plausibly start exercising and it is untested at this call site today.
   • a typed issues array with mixed severities -> gate_findings['pr-review'].severity reports real non-zero counts, which is the acceptance criterion the old NOTE described as permanently zero.

8. node scripts/lint-engine.js --fix, then the full test_command.

Task 3 — documentation

Files: docs/architecture/gate-hygiene.md (new), docs/architecture/index.md, docs/architecture/CLAUDE.md, docs/architecture/AGENTS.md.

1. New page docs/architecture/gate-hygiene.md: what a finding is now, the typed issues shape and why it is one field looser than CHALLENGE_SCHEMA, the engine-assigned id scheme, the absent-vs-empty distinction, the three predicates and why the merge gate's differs from the two internal ones, the empty-findings exit landing on needs_human and tallying carried-unresolved, and findings_empty_exits.

2. One paragraph on the observability baseline moving. computeGateYield derives escaped from gate_findings['pr-review'].count at :5341, all findings at any severity. Prompts that now ask reviewers to put every concern in the array raise that count on one side of the comparison, so the first post-change batch report can show escaped defects where earlier runs showed none. That is a baseline shift, not a regression. Changing the definition to severity.critical + severity.major is out of scope for this issue and belongs in a follow-up.

3. Add the gate-hygiene.md row to the file map in docs/architecture/index.md. The provenance segment for that file covers only its first six lines, above the table, so appending a row is outside every tracked hash.

4. Repair the freeze pair. docs/architecture/CLAUDE.md says "These ten files" twice and carries a "What lives here" table with a row per page. Edit CLAUDE.md, then cp docs/architecture/CLAUDE.md docs/architecture/AGENTS.md, which is the direction that file's own parity note prescribes. tests/docs-links.test.js:246-265 asserts the two are byte-identical, so a half-applied fix turns the suite red.

5. Do NOT edit pipeline.md, metrics.md, or failure-semantics.md. Their prose is hash-frozen by tests/architecture-provenance.test.js and the tech-docs stage runs after the last test-loop execution, so a frozen-prose edit surfaces only as a red check on the batch PR.

6. Run node --test to exercise docs-links, architecture-provenance, and readme-docs-census.

Carried caveats

• Whether four rewritten prompts actually produce well-formed issues arrays is not statically checkable. Only a live run settles it. The null fallback means the failure mode is "behaves like today", not "throws".
• The escaped-defect projection came from the five run records currently in logs/ticketmill/runs/, eight units carrying gate_findings. The direction of the effect is certain, the magnitude is not.
• The lockstep copy at .claude/workflows/ticketmill.js is engine-owned. It is touched only by node scripts/lint-engine.js --fix, never edited by hand, and only because the profile's LOCKSTEP-EDIT RULE makes the mechanical sync mandatory for any engine edit.

[aaddrick]

Task List

• 1. Schema, helpers, and the two internal loops (ticketmill-implementer) — type REVIEW_SCHEMA.issues.items at :637-638 (required: ['severity','summary'], severity enum, optional recommendation; issues stays out of required, id stays out of the schema); add normalizeFindings(raw, source) and findingsBlock() next to recordGateOutcome; add findings_empty_exits: 0 to the metrics literal at :4846 AND to freshMetrics() in tests/harness.js:82-89, not to FRICTION_WEIGHTS; add the verdict-tied issues wording to the quality-review (:2934) and test-validation (:3452) prompts; make both loop predicates approved OR (findings !== null && findings.length === 0), incrementing the counter and pushDecision on the empty leg; feed quality-fix (:2957) and test-quality-fix (:3479) from findingsBlock with comments kept as context. Tests: normalizeFindings/findingsBlock units in tests/gate-findings.test.js, plus positive-assertion empty-array scenarios in tests/quality-loop.test.js and tests/test-loop.test.js (fix key absent from the recorded stage keys, return value, quality_degrades still 0, counter at 1) and a regression that existing issues: ['x'] fixtures still run their fix stage. Run node scripts/lint-engine.js --fix in the same commit, then the full test_command.

• 2. The merge gate (ticketmill-implementer) — add the same wording to the spec-review (:4613) and code-review (:4633) prompts; normalize both arrays with distinct sources so the parallel() pair cannot collide ids; feed recordGateOutcome(ctx, 'pr-review', ...) the normalized arrays; add per-reviewer nothingToFix(r, f) while keeping prReviewClean (both approved) as the ONLY setter of approved = true; when both reviewers have nothing to fix but the gate is not clean, record 'carried-unresolved', bump the counter, pushDecision, and break WITHOUT approved, landing on the existing needs_human at :4701 via a haltReason local that carries accurate text for both paths; feed pr-fix (:4675) from findingsBlock; rewrite the doc comment at :1994-2027 in three places (accepted, carried-unresolved, and the stale severity NOTE). Tests in tests/pr-review-gate.test.js: both-empty -> needs_human/stage: 'pr-review' with no pr-fix-i1 and no merge; one-empty-one-real -> pr-fix-i1 still runs; a null spec reviewer and a null code reviewer each -> needs_human and no merge; a mixed-severity array -> real non-zero gate_findings['pr-review'].severity. Run node scripts/lint-engine.js --fix, then the full test_command.

• 3. Documentation (ticketmill-implementer) — write docs/architecture/gate-hygiene.md covering the typed finding shape, the engine-assigned ids, absent-vs-empty, the three predicates and why the merge gate's differs, the needs_human exit and its carried-unresolved tally, and findings_empty_exits; include a paragraph stating that pr-review finding counts step up after this change, so the escaped-defect baseline in gate yield (computeGateYield, :5341, keyed on raw count) moves, and that redefining it on severity.critical + severity.major is a follow-up. Add its row to the file map in docs/architecture/index.md (below the six-line provenance segment). Repair the freeze pair: edit docs/architecture/CLAUDE.md ("ten files" in two places, plus a "What lives here" row), then cp docs/architecture/CLAUDE.md docs/architecture/AGENTS.md per that file's own parity note, since tests/docs-links.test.js:246-265 asserts byte-identity. Do NOT touch pipeline.md, metrics.md, or failure-semantics.md (hash-frozen by tests/architecture-provenance.test.js). Run node --test.

CHANGELOG.md and .claude-plugin/plugin.json are not tasks: the release stage handles both.

[aaddrick]

Contrarian: Plan Stress Test (Iteration 1)

Verdict: needs_rework on one major test-coverage finding. The decomposition, ordering and assignments are right; four minor findings are documentation accuracy and prompt hygiene.

Marker check: every comment on this issue carries <!-- ticketmill aaddrick/ticketmill#162 -->. Nothing misfiled, nothing deleted.

Strengths (steel-man)

Splitting by gate rather than by layer is the correct call and it is the one that makes the ordering safe: task 1 lands normalizeFindings/findingsBlock with two consumers already wired, task 2 adds the third, so no helper sits dead in the tree between commits. Nothing in task 2 can run before task 1, and the plan sequences them that way.

I re-checked the claims the plan is built on, and the ones that matter hold:

• REVIEW_SCHEMA.issues is { type: 'array', items: {} } at :637-638, and stage() at :1765-1786 passes schema straight to agent() with no engine-side validation. The vm harness's scripted responder does not validate either. So tightening items cannot break an existing fixture.
• No existing fixture flips meaning. I grepped every scripted changes_requested in the suite: pr-review-gate.test.js:107,145 carry typed objects, quality-loop.test.js:65,144 and test-loop.test.js:106 carry issues: ['x']. Not one of them is changes_requested with issues: [], so the new empty-array leg is unreachable from today's fixtures.
• APPROVED_REVIEW at pr-review-gate.test.js:62 is approved plus issues: [], which is exactly the case prReviewClean must keep owning. Under the plan it still sets approved = true, since prReviewClean remains the only setter.
• The metrics literal at :4846 is the only one in the engine, tests/harness.js:82-89 is its only hand mirror, and no test couples FRICTION_WEIGHTS to the metrics key set. The two-sided add the plan specifies is both necessary and sufficient.
• docs/architecture/index.md's file map uses real markdown links, not code spans, so the new row genuinely creates the second hop from docs/index.md and docs-links.test.js:171 stays green. readme-docs-census.test.js filters ^docs/[^/]+\.md$, so a file one level down is out of its census. architecture-split.test.js only asserts the fixture's ten files exist on disk, so an eleventh file is invisible to it.

Findings

[Major] No loop-level test covers the omitted-issues path, and the one plausible implementation slip is invisible to every test the plan does specify.

• Assumption challenged: that a normalizeFindings(undefined) -> null unit test plus a findingsBlock(null) unit test together prove acceptance criterion 7 at the three call sites.
• Failure scenario: the natural call is normalizeFindings(rev.issues, source). The natural slip, in a codebase where (spec.issues || []) is the existing idiom two hundred lines away at :4669, is normalizeFindings(rev.issues || [], source). That collapses absent into present-and-empty, and every planned test still passes: the unit test calls the helper directly and never sees the || []; every loop-level fixture in the suite has issues present, so none of them exercises absence; the new empty-array scenarios assert the fix stage is skipped, which is what the slip produces too. What ships is all three fix loops exiting approved on the first review from any reviewer that drops the key, and CHALLENGE_SCHEMA's own comment at :592-594 records that this model demonstrably drops the findings array under long output.
• Impact: silent, and it fails in the flattering direction. Quality loops return 'approved', the test loop returns {ok: true}, PRs stop getting fixed, and the run record shows a clean pass. This is precisely the inverse of what the issue is for.
• Recommendation: add the mirror-image scenario to each of the three test sites the plan already touches. A scripted changes_requested response with issues omitted (not [], not ['x']), asserting the fix key IS present in the recorded stage keys and findings_empty_exits is still 0. Three scenarios, same responder scaffolding as the empty-array ones, and they are the only thing that pins the null-versus-empty hinge the entire design rests on.

[Minor] docs/architecture/metrics.md:82-85 states the claim this issue falsifies, is hash-frozen, and the plan leaves it standing with no pointer.

• Assumption challenged: that the stale severity claim lives only in the source comment at :2021-2027.
• Failure scenario: metrics.md:82-85 reads "REVIEW_SCHEMA's issues field has no severity ... so gate_findings['pr-review'].severity stays zero across the board". After task 1 that is false, and it is the published version of the note task 2 rewrites. I resolved the fixture against the file: metrics.md's single segment starts at line 5 and covers 329 lines against a 332-line body, so the segment runs past the last line on disk and relies on the provenance test's trailing-blank padding. That means it cannot be edited and cannot be safely appended to either, so the alternative floated at approach-challenge iteration 2 ("append the section to metrics.md") would have turned the suite red. The plan was right to create a new file; it just stops short of neutralizing the old text.
• Impact: a shipped architecture page asserts the opposite of shipped behavior. Worse, the tech-docs stage at :4709-4724 tells a doc writer to "create/update docs" for exactly this diff, runs after the last test-loop execution, and is non-fatal, so a doc writer who notices the contradiction and corrects it surfaces only as a red architecture-provenance check on the batch PR.
• Recommendation: one paragraph in gate-hygiene.md naming metrics.md's gate-findings paragraph as frozen provenance prose that this page supersedes and that must not be edited, and one line to the same effect in the CLAUDE.md/AGENTS.md freeze pair task 3 is already opening. That converts an undocumented landmine into the guard the tech-docs agent reads before touching the directory.

[Minor] The "reword ten files" instruction is wrong for two of the three occurrences and misses the third.

• Assumption challenged: that docs/architecture/CLAUDE.md says "ten files" in two places and that all of them go stale when an eleventh file lands.
• Failure scenario: the file says "ten" at :3, :48 and :81, not two places. Line :48 ("the full reconstruction across all ten files") and line :81 ("the provenance test only knows about the ten files") are statements about tests/fixtures/architecture-split.json, which still tracks exactly ten outputs after this change. Rewording those to "eleven" makes CLAUDE.md contradict the fixture and misdescribe the test in the one document written to stop people breaking that test. Only :3 ("These ten files used to be one file") is actually stale, and :48 is not in the plan's inventory at all.
• Impact: a mechanical find-and-replace by the implementer produces a directory guide that is wrong about its own invariant, mirrored byte-for-byte into AGENTS.md.
• Recommendation: reword :3 only, to distinguish the ten split-derived pages from pages added since, leave :48 and :81 at ten, and mark the new "What lives here" row as authored text outside the provenance fixture (the same way the AGENTS.md/CLAUDE.md row already is).

[Minor] Nothing ever reads a finding id back, so "addressable findings" lands as decoration.

• Assumption challenged: that rendering - [code-i2-3] [major] summary -> recommendation into the fix prompt is what makes findings addressable.
• Failure scenario: the id appears in exactly one place, the fix prompt, and no prompt asks the fix agent to reference it. FIX_SCHEMA.fixes_applied is {type:'array', items:{type:'string'}} and stays free prose, so after the fix stage there is no record tying any applied fix to any finding, and the next iteration's reviewer has nothing to check a claimed fix against. Acceptance criterion 1 is met literally (a stable per-iteration id exists) while the artifact stays as unaddressable as it is today.
• Impact: the follow-up issue that wants per-finding rebuttal records inherits an id scheme with zero call sites and no data to migrate.
• Recommendation: one sentence in the three fix prompts asking the implementer to prefix each fixes_applied entry with the finding id it resolves. No schema change, no control flow, and it makes the ids load-bearing in the run record on the first live run rather than the second.

[Minor] findingsBlock's behavior for a present-but-empty array is unspecified at the one site that can reach it without exiting.

• Assumption challenged: that [] always terminates a loop, so the renderer only ever sees null or a non-empty array.
• Failure scenario: at pr-fix, one reviewer returning [] while the other returns real findings does not terminate (the AND is preserved, correctly), so findingsBlock is called with an empty array. The plan specifies the null branch byte-for-byte and the non-empty branch by line shape, and says nothing about []. An implementer who renders it as an empty work list with the prose demoted under a "context" heading hands the fix agent a reviewer whose prose objections now read as optional background, where today String(spec.comments || spec.summary || 'approved') is the whole instruction.
• Impact: a quiet fidelity loss in the fix prompt at the merge gate, in the direction of fixing less.
• Recommendation: specify it in task 1: [] renders one explicit line saying the reviewer named no structured findings, and never suppresses or demotes that reviewer's prose block.

Verdict

needs_rework, on the major alone. The fix is three test scenarios in tasks 1 and 2 that reuse scaffolding those tasks already build; nothing about the decomposition, ordering, or assignments needs to move.

[aaddrick]

Revised Implementation Plan (iteration 1)

The plan contrarian returned needs_rework with one major and four minor findings. I verified all five against the worktree; all five are adopted. No adjudicated decision is overturned — the shape stays three sequential ticketmill-implementer tasks split by gate, which the contrarian explicitly endorsed.

What the major finding actually catches

The whole design hinges on null (reviewer omitted issues) meaning something different from [] (reviewer named none). Every scripted reviewer in tests/quality-loop.test.js, tests/test-loop.test.js and tests/pr-review-gate.test.js today supplies issues explicitly — [], ['x'], or a typed array. There is not one omitted-issues response anywhere in the three files.

The planned unit test normalizeFindings(undefined) -> null proves the helper right, but the plausible slip is at the call site: normalizeFindings(rev.issues || [], source) collapses absent into empty before the helper ever sees it. That slip passes the unit test, passes every existing fixture, and silently turns all three fix loops off — a changes_requested review with no issues key would return approved and skip straight past the fix stage. The fix is three mirror-image loop-level scenarios, one per gate, asserting the fix stage key IS present and findings_empty_exits is still 0.

The four minor findings, verified

Finding	Verification
metrics.md states the falsified claim, and it is frozen	Lines 81-84 read "REVIEW_SCHEMA's issues field has no severity ... so gate_findings['pr-review'].severity stays zero across the board". The fixture's only segment for that file starts at line 5 and runs 329 lines, so 81-84 sits inside the tracked range. Unfixable in place; it needs a pointer instead.
"reword ten files" is wrong twice	docs/architecture/CLAUDE.md says ten at :3 ("These ten files used to be one file"), :48 ("reconstruction across all ten files") and :81 ("only knows about the ten files"). Only :3 counts directory contents; :48 and :81 count split-derived files and stay correct at ten.
Finding ids read by nothing	FIX_SCHEMA at :646 already carries fixes_applied: {type:'array', items:{type:'string'}}. A prompt sentence makes the ids load-bearing with zero schema change.
findingsBlock([]) unspecified	Reachable at the pr-fix site when one reviewer is empty and the other has findings — the exact scenario task 2's own test list already includes.

Full revised plan (3 tasks)

Task 1 — Schema, helpers, and the two internal loops

In workflows/ticketmill.js:

• Type REVIEW_SCHEMA.issues.items at :637-638 as {type:'object', required:['severity','summary'], properties:{severity:{enum:['critical','major','minor']}, summary:{type:'string'}, recommendation:{type:'string'}}} — deliberately one field looser than CHALLENGE_SCHEMA.findings at :599. Keep issues OUT of REVIEW_SCHEMA.required; keep id out of the schema entirely, since the engine assigns it.
• Add normalizeFindings(raw, source) next to recordGateOutcome (~:2028): null when raw is not an array (the reviewer omitted the key), otherwise arity-preserving one entry per input with id = source + '-' + (i+1), severity coerced to critical/major/minor or 'unspecified', summary and recommendation carried through, and a bare-string entry becoming {summary: String(entry)}. The source comment labels the 'unspecified' coercion as defence-in-depth against an array that never went through item validation, not as a live production path.
• Add findingsBlock(findings, comments, fallbackLabel):
  • findings === null -> emit exactly what the site emits today (String(rev.comments || rev.summary || '<fallback>')), so the omit path produces a byte-identical prompt.
  • findings.length > 0 -> reuse the line shape at :4259 with the id prefixed (- [code-i2-3] [major] summary -> recommendation), and keep prose comments below under a heading marked as context, not as the work list.
  • findings.length === 0 (new, finding 5) -> emit one explicit line stating that reviewer named no structured findings, and still render its prose block below under the same context heading. An empty array never suppresses or demotes prose. Only the pr-fix site reaches this branch; the two internal loops exit before rendering.
• Add findings_empty_exits: 0 to the metrics literal at :4846 AND to freshMetrics() in tests/harness.js:82-89, same position. The harness hand-mirrors that literal and tests/run-record.test.js:71 derives its required-key list from it, so a one-sided add makes the counter NaN. Do NOT add it to FRICTION_WEIGHTS at :2384.
• Prompt wording added to the quality-review prompt (:2934-2952) and the test-validation prompt (:3452-3473): every concern you want fixed goes in issues, one entry per concern, with severity, summary and a recommendation; a concern appearing only in comments will not be fixed; if you return changes_requested, issues must be non-empty; if you have nothing that must be fixed, return approved.
• Predicates at :2954 and :3477 become rev.result === 'approved' || (f !== null && f.length === 0), returning 'approved' / {ok:true}. On the empty-array leg ONLY: increment ctx.metrics.findings_empty_exits and pushDecision(ctx, 'Gate: no findings to fix', ...).
• Feed quality-fix (:2957-2962) and test-quality-fix (:3479-3484) from findingsBlock. Add to both fix prompts (finding 4) a sentence asking the implementer to prefix each fixes_applied entry with the id of the finding it resolves, e.g. [quality-task-1-i1-2] tightened the null guard. No schema change needed — FIX_SCHEMA:646 already accepts free-form strings — and it puts the ids in the run record on the first live run.

Tests:

• tests/gate-findings.test.js unit coverage for both helpers: omitted -> null; [] -> []; id assignment across a multi-entry array; a bare-string entry; an out-of-enum severity -> 'unspecified'; arity preserved for a malformed entry; null -> today's prose byte for byte; the rendered line shape; and [] -> the explicit no-findings line with the prose block still present.
• tests/quality-loop.test.js and tests/test-loop.test.js, empty-array scenario: the scripted responder records every stage key it is asked for, and the test asserts the fix key (quality-fix-task-1-i1 / test-quality-fix-i1) is ABSENT from that list, the return value is 'approved' / {ok:true}, ctx.metrics.quality_degrades is still 0, and findings_empty_exits is 1. Do not rely on a responder throw as proof — stage() at :1776-1782 catches throws into a retry and an agent death.
• New (finding 1) — the same two files get the mirror-image scenario: a scripted changes_requested response with the issues key OMITTED, not [] and not ['x'], asserting the fix stage key IS present in the recorded keys, the loop behaves exactly as it does on main, and findings_empty_exits is still 0. Reuses the empty-array scaffolding. This is the only test that pins the null-versus-empty hinge at the call site; a rev.issues || [] slip passes everything else in the suite.
• Regression: the existing issues:['x'] fixtures still run their fix stage, and the fix prompt now contains the rendered finding line.

Finish with node scripts/lint-engine.js --fix in the same commit (profile LOCKSTEP-EDIT RULE; --fix always writes source over copy, never hand-edit the installed copy), then the full test_command.

Task 2 — The merge gate

In workflows/ticketmill.js:

• Add the identical verdict-tied issues wording from task 1 to the spec-review prompt (:4613-4629) and the code-review prompt (:4633-4650).
• Normalize each reviewer's issues with DISTINCT sources ('spec-i' + iter, 'code-i' + iter) — the two reviews run through parallel() at :4610 and land in one tally, so shared ids would collide.
• Feed recordGateOutcome(ctx, 'pr-review', ...) the normalized arrays, flattening null to [] so the tally matches today's spec.issues || [].
• Add per-reviewer nothingToFix(r, f) = r.result === 'approved' || (f !== null && f.length === 0). prReviewClean keeps its current meaning (both reviewers approved) and MUST remain the only path that sets approved = true — a permissive predicate here would let a changes_requested verdict reach gh pr merge --squash.
• When both reviewers have nothing to fix but prReviewClean is false: record the iteration as 'carried-unresolved' (not a new disposition string — computeGateYield hardcodes exactly four keys at :5316-5320 and the renderer prints exactly those columns), increment findings_empty_exits, pushDecision, and break WITHOUT setting approved, landing on the existing needs_human at :4701. One reviewer with real findings still sends the iteration into the fix stage, so the AND holds in both directions.
• Add a haltReason local carrying accurate text for each of the two paths into the single fail() call at :4701 — "not approved after N iterations" is false on an early break.
• Feed pr-fix (:4675-4681) from findingsBlock per reviewer, both prose blocks below as context. Add the same fixes_applied id-prefix sentence from task 1.
• Rewrite the doc comment at :1994-2027 in three places: accepted (:1996-1999) drops "the same condition that ends the loop", since the loop now has a second exit, while still requiring both approvals; carried-unresolved (:2001-2007) widens to "the gate did not reach a clean pass — either the iteration cap was reached, or both reviewers requested changes while naming no findings to fix", both landing on the same needs_human; the severity NOTE (:2021-2027) is rewritten because REVIEW_SCHEMA.issues is now typed, the four prompts ask for severity, and gate_findings['pr-review'].severity now carries real counts.

Tests in tests/pr-review-gate.test.js:

• Both reviewers changes_requested with issues: [] -> needs_human with stage pr-review, neither pr-fix-i1 nor merge in the recorded stage keys, gate_findings['pr-review'].disposition['carried-unresolved'] === 1.
• One reviewer empty and the other with real findings -> pr-fix-i1 still runs, and its prompt carries both the rendered finding line and the empty reviewer's explicit no-findings line plus prose (the finding-5 branch).
• New (finding 1) — both reviewers changes_requested with issues OMITTED -> pr-fix-i1 DOES run, merge does not, findings_empty_exits is 0. Third leg of the null-versus-empty pin.
• A null spec reviewer, and separately a null code reviewer -> needs_human, stage pr-review, merge never runs. That is the :4656 path a schema tightening could start exercising, untested at this call site today.
• A typed mixed-severity array -> gate_findings['pr-review'].severity reports real non-zero counts, the acceptance criterion the old NOTE called permanently zero.

Finish with node scripts/lint-engine.js --fix and the full test_command.

Task 3 — Documentation

Create docs/architecture/gate-hygiene.md covering: what a review finding is now; the typed REVIEW_SCHEMA.issues shape and why it is one field looser than CHALLENGE_SCHEMA; the engine-assigned id scheme and why the engine assigns rather than the model; the absent-versus-empty distinction (null falls back to the prose path, [] is a real signal); the three loop predicates and why the merge gate's differs from the two internal ones; the empty-findings exit landing on needs_human and tallying carried-unresolved; the findings_empty_exits counter; and that fix stages are asked to echo finding ids back through fixes_applied.

Include a paragraph stating that computeGateYield derives escaped at :5342-5343 from gate_findings['pr-review'].count — all findings at any severity — so prompts that now ask reviewers to put every concern in the array raise that count on one side of the comparison, and the first post-change batch report can show escaped defects where earlier runs showed none. That is a baseline shift, not a regression; redefining the signal on severity.critical + severity.major is a follow-up, out of scope here.

New (finding 2) — add a short provenance paragraph naming docs/architecture/metrics.md:81-84, the "Completing the gate findings tally" passage ending "gate_findings['pr-review'].severity stays zero across the board", as hash-frozen prose describing the state of the world before this issue. It is superseded by this page and MUST NOT be edited: it sits inside the fixture's single tracked segment for that file, which starts at line 5 and runs 329 lines, so any correction turns tests/architecture-provenance.test.js red. Add one line to the same effect in the CLAUDE.md / AGENTS.md freeze pair, which is what a doc-writing agent reads first. That converts an undocumented landmine into a guard.

Add the gate-hygiene.md row to the file map in docs/architecture/index.md — the provenance segment for that file covers only its first six lines (title and lede), so appending a table row is outside every tracked hash.

Revised (finding 3) — the freeze-pair edit is precise, not a find and replace:

• docs/architecture/CLAUDE.md:3, "These ten files used to be one file", is the only occurrence that counts directory contents. Reword it to distinguish the ten split-derived pages from pages added since, so the sentence stays true as the directory grows.
• :48 ("reconstruction across all ten files") and :81 ("only knows about the ten files and line ranges recorded when the split happened") both describe the provenance fixture's scope, which is still exactly ten files. Leave both at ten — changing them would make the directory guide wrong about its own invariant.
• Add the gate-hygiene.md row to the "What lives here" table at :59-73, marked as authored text outside the provenance fixture, the way the AGENTS.md, CLAUDE.md row at :73 already is.
• Then run cp docs/architecture/CLAUDE.md docs/architecture/AGENTS.md, the direction that file's own parity note at :9-12 prescribes, because tests/docs-links.test.js:246-265 asserts the two are byte-identical and a half-applied fix turns node --test red.

Do NOT edit docs/architecture/pipeline.md, metrics.md, or failure-semantics.md — their prose is hash-frozen by tests/architecture-provenance.test.js, and the tech-docs stage runs after the last test-loop execution, so a frozen-prose edit surfaces only as a red check on the batch PR.

Run node --test to exercise docs-links, architecture-provenance, and readme-docs-census.

Deliberately not tasks

CHANGELOG.md and .claude-plugin/plugin.json — the profile has a release stage configured and handles both.

[aaddrick]

Revised Task List (iteration 1)

• 1. Schema, helpers, and the two internal loops (ticketmill-implementer) — type REVIEW_SCHEMA.issues.items; add normalizeFindings() and findingsBlock() (including the explicit empty-array branch); add findings_empty_exits to both the engine metrics literal and tests/harness.js freshMetrics(); add the verdict-tied issues wording to the quality-review and test-validation prompts; flip the quality and test loop predicates to "approved OR present-and-empty"; feed both fix stages from the structured array and ask them to prefix fixes_applied entries with finding ids. Tests: helper unit coverage, empty-array loop scenarios, and the omitted-issues mirror scenarios that pin the null-versus-empty hinge at the call site. Ends with node scripts/lint-engine.js --fix and the full test command.
• 2. The merge gate (ticketmill-implementer) — verdict-tied issues wording in the spec and code review prompts; per-reviewer normalization with distinct sources; nothingToFix() with prReviewClean still the only setter of approved = true; the both-empty exit breaking without approval onto the existing needs_human, tallied carried-unresolved; haltReason for accurate failure text; pr-fix fed from findingsBlock with the id-echo sentence; the :1994-2027 doc comment rewritten in three places. Tests include the omitted-issues merge-gate leg and the mixed-severity non-zero tally. Ends with node scripts/lint-engine.js --fix and the full test command.
• 3. Documentation (ticketmill-implementer) — write docs/architecture/gate-hygiene.md (findings model, predicates, counter, escaped-defect baseline shift, and the frozen-prose pointer at metrics.md:81-84); add its index.md file-map row; repair the freeze pair with a precise edit to CLAUDE.md:3 only, leaving :48 and :81 at ten, plus the new table row marked as authored text, then cp to AGENTS.md. Run node --test.