feat: verify stage in tm-review-changes with spec-format item transforms

[sv-tmueller]

Closes #406

Part of batch #417 and of #344 (package P1).

What changed

• Spec format: new items_transform field naming a reducer from a closed set that each renderer implements. First entry: must_fix_deduped (flatten worker findings, keep must-fix, dedup on file + line + problem). The cap stays the one generic step and runs after the transform. An unknown name throws in both renderers, so a host without the reducer fails loudly instead of fanning out over unreduced items.
• items_cap bug: both renderers looked up args["args.areas"], so the cap was always the default. Fixed (strip the args. prefix, coerce like MAX_AREAS).
• tm-review-changes: new verify stage between review and consolidate, worker tier, one adversarial verifier per must-fix finding (schema { confirmed, note }), capped by args.maxVerify (default 12). Consolidate gets confirmed, refuted and unverified findings as separate inputs.
• The script owns the report's refuted and unverified arrays and strips refuted findings from mustFix (finalizeReport), and sets the verdict to approve when that strip empties mustFix, so this does not depend on model behavior.
• A dead verifier (null result) means unverified, not refuted. Findings past the cap are also unverified. Both go to the critic and are listed in the report.
• Docs: adapter-interface.md, README node label, tm-review-changes skill description, team-guide-rationale.md worked-example line.
• Version bump 2.8.1 to 2.9.0, after docs: add headless claude -p checklist to tm-ab-test #420 and feat: token report covers workflow subagents and adds a per-agent wall-clock table #421 merged first (merge commit 93bb0b1).

Report-level overflow AC (Claude Code runtime only)

The AC "findings past the cap appear in the report" is met by the Claude Code runtime (tm-review-changes.js), the only renderer that builds consolidate inputs from runtime data. Hermes stubs those slots for every workflow, and the Codex renderer builds no consolidate prompt from stage results. In the Hermes and Codex renderers, overflow is a log line, like the existing stub-fallback log.

Live run (lead, after tester PASS at 5741657)

• Run: one tm-review-changes run on this PR's own diff, 2026-10-01, run id wf_613b6f09-7e3, args: { base: "origin/main" }, with the lead checkout detached at 57416579 for the run and HEAD checked unchanged afterward (batch Batch: measurement-tools #417 Decision 5).
• Load adaptation: the Workflow runtime refused this branch's .claude/workflows/tm-review-changes.js by path ("export const meta = { name, description, phases } must be the FIRST statement in the script"), the same computed-meta refusal A/B arm: live ultracode on Opus 5.5 on a replayed merged issue #405 recorded. The run used a scratch copy with meta evaluated once and written as a literal first statement; diff against the branch file shows no other change (batch Batch: measurement-tools #417 Decision 2).
• Result: verdict approve, 0 must-fix, 1 should-fix, 6 nits, 3 dismissed. 8 agents (7 Sonnet reviewers, 1 Opus critic); all 7 dimensions reported, 2 with zero findings.
• Confirmed count: 0. Refuted count: 0 (unverified: 0). The reviewers raised no must-fix finding, so the verify stage did not fire: no verify:* dispatch, which is the D10 behavior for zero must-fix. The verify path is exercised by the tests on this PR, not by this live run.
• Cost: $1.76 at list price (Opus critic $0.63, 7 Sonnet 5.5 reviewers $1.13). Source: token-report.mjs from PR feat: token report covers workflow subagents and adds a per-agent wall-clock table #421's branch at a8e2393 (unmerged, run read-only) against a scratch copy holding only this workflow's 8 transcripts. That script's price table has no claude-sonnet-5-5 entry, so the Sonnet figure is priced by hand at the published Sonnet 5.5 list rates (https://platform.claude.com/docs/en/about-claude/pricing, retrieved 2026-10-01: $2 input, $2.50 5m cache write, $4 1h cache write, $0.20 cache read, $10 output per MTok). Output tokens are that script's chars/4 estimate and exclude thinking, so the figure undercounts.
• Wall-clock: 195,901 ms (3 min 16 s), measured: the workflow's own task-notification duration_ms. On this diff that is well under the 600 s headless background-task ceiling; a run with must-fix findings adds the verify phase.
• Should-fix the run raised: finalizeReport removes refuted findings from report.mustFix but keeps the critic's verdict, so a report can read mustFix: [] with verdict changes-requested. The strip also matches exact file + line + problem, so a critic that rewords a refuted finding slips past it. The run's suggested fix: derive the verdict from the final mustFix length, with a test where the critic echoes only refuted findings. Fixed in 69800c2 (reviewer fix round 1/3): the verdict is recomputed to approve when only refuted findings were removed. The reworded-finding half stays out of scope (D2 specifies an exact key).
• Nits the run raised: the critic-facing REPORT_SCHEMA still offers refuted and unverified; the "Bounded by construction" comment reflow; the workflow's own maxVerify coercion is tested only with 1; maxVerify is not in the invoke comment or SKILL.md; the verify prompt does not mark the finding text as untrusted data; ITEM_TRANSFORMS is duplicated across the two renderers (guarded by the parametrized test).

Verification

npm test (467 tests at 93bb0b1, 0 failures) and node scripts/check-version-bump.mjs origin/main HEAD both pass locally.

New tests: item-transforms.test.mjs (both renderers, filter, dedup, cap, overflow log, unknown name throws), review-changes-verify.test.mjs (refuted, over-cap and dead-verifier handling end to end), hermes-verify-stage.test.mjs, plus additions to hermes-adapter, helpers, prompts-sync and render-path tests. The items_cap test in hermes-adapter.test.mjs failed on main (expected 1 dispatch, got 2).

Notes for the reviewer

• The verify stage maps to the fact-checker role in both renderers (per the sub-plan). On Hermes that role's prompt prepends the fact-checker report format, while the stage schema is { confirmed, note }. The task text and output_schema ask for the latter; worth a look in a live Hermes run.
• Adding the Verify phase and verify:* dispatches lengthens the run; see the wall-clock figure above.

🤖 Generated with Claude Code

[sv-tmueller]

Tester report (initial test, batch #417)

VERDICT: PASS
COMMIT: 5741657
FINDINGS: none
Checks run: npm test exit 0 (429 pass, 0 fail). node scripts/check-version-bump.mjs origin/main HEAD exit 0 (2.7.0 -> 2.8.0).
Forwarded items:

1. Mutation re-run in a scratch copy under the scratchpad, not the repo. A dead verifier counted as refuted failed 2 tests (dead verifier, non-boolean confirmed). Overflow not tracked in unverified failed 1 (the cap test). A disabled refuted-strip in finalizeReport failed 1 (the refuted-absent-from-mustFix test). The tests pin D1, the overflow AC and the strip.
2. Must-fix reaches consolidate only through mustFixDeduped, then confirmed, refuted or unverified. raw excludes must-fix, so no double path. Dedup duplicates share a key, so none are lost. A same-key should-fix from another dimension stays in raw, which is acceptable.
3. The throw before items_source resolves breaks no path. item-transforms.test.mjs covers the stub fallback, and a Hermes DRY_RUN of tm-review-changes runs review, verify and consolidate cleanly.
4. The team-guide-rationale.md line 153 edit is accurate and is the same drift D12 targets. No other stale "reviewers + one critic" text remains outside the dated codebase map.
5. Not demonstrated. The Hermes renderer does no parsing: delegateTaskLeaf passes only the schema name string as output_schema and returns the host result as is. The fact-checker VERDICT/CLAIMS prompt could conflict with {confirmed, note}, but the shipped code never mis-parses. It is a live-host risk only (see UNTESTED CLAIMS).
6. The regex pins the mapping text in both renderers. hermes-verify-stage.test.mjs also checks behaviorally that the verify goal starts with the fact-checker role prompt on Hermes. Adequate.
   Attacks, all clean:

• The items_cap test fails on main's renderers (items_cap "args.areas" resolves..., 1 fail) and passes on the branch.
• In both renderers, cap 0, -1, 2.5 and "3" fall back to the default 12. Cap 3 gives 3. An undefined args is safe.
• Null and malformed reports are skipped.
• Delimiter-collision keys (a:1/2 vs a/1:2) stay distinct.
• An unknown transform throws, including prototype keys such as toString.
• The unprefixed items_cap form still works.
• Zero must-fix findings skips verify and logs it. A null parallel() slot becomes unverified.
• criticWithFallback and safeRef are untouched, and the byte-identical tests pass.
• All seven surfaces in the issue's table changed together.
  Minor, not findings:
• Dedup treats line 1 and "1" as different keys, and undefined and null as the same.
• The maxVerify arg is not mentioned in the workflow's invoke comment or the skill docs.
• Hermes ignores verify results, because consolidate slots are stubbed there (D7, as planned).
  UNTESTED CLAIMS: the live /tm-review-changes run on this PR's diff (confirmed count, refuted count, cost, wall-clock in the PR body; the lead's step). A real Hermes host returning a parseable {confirmed, note} from the fact-checker role is untested, since the tests stub delegate_task. Codex spawn behaviour for verify is also untested, because it cannot be stubbed.
  LESSONS: a worktree-isolated tester cannot run heredoc or node -e scripts. Write scratch scripts with printf '%s\n' '...' > <scratch path> and run the file.

[sv-tmueller]

Reviewer report (batch #417)

VERDICT: APPROVE
STAGE: quality
FINDINGS:

1. .claude/workflows/tm-review-changes.js:118-124, should-fix. finalizeReport removes refuted findings from mustFix but keeps the critic's verdict. If the critic ignores the prompt and returns only refuted findings, the report reads mustFix: [] with verdict: 'changes-requested'. This does not defeat the AC: review-changes-verify.test.mjs shows the finding gone from mustFix and present in refuted. It cannot turn a real must-fix into approve either. The wrong outcome is a false block, with the refuted appendix visible to the reader. So it does not block. But the verdict, the field humans and the tm-ab-test report template read, still depends on what the model does, which is what D2 set out to remove. Required change: inside if (Array.isArray(report.mustFix)), after the filter, add if (out.mustFix.length === 0 && report.mustFix.length > 0) out.verdict = 'approve'. Unverified findings are never removed, so D1 still holds. Add one sentence to the comment above the function saying the verdict is recomputed when only refuted findings were removed. Add two finalizeReport cases to helpers.test.mjs: (a) { verdict: 'changes-requested', mustFix: [gone] } with gone refuted gives verdict === 'approve' and an empty mustFix; (b) the same input plus a kept finding keeps 'changes-requested'. The live run's other half (a reworded refuted finding slips past) is not a finding. D2 specifies matching on exact file + line + problem, and fuzzy matching would be new scope.
   Evidence: sub-plan D2 "Without this, the AC ... can only be checked against model behavior"; the consolidate prompt says "verdict is changes-requested if any remain, approve otherwise".
2. .claude/workflows/tm-review-changes.js:80, nit. The invoke comment does not mention the new maxVerify arg. Change it to Workflow({ name: 'tm-review-changes', args: { base: 'origin/main', maxVerify: 12 } }).
   Evidence: the MAX_AREAS precedent the issue cites lists its cap in the invoke comment, tm-map-codebase.js:84 args: { path: 'src', areas: 24 }.
3. .claude/workflows/tm-review-changes.js:72-73, nit. The rewrap of the "Bounded by construction" comment leaves a short line, "// and effort are pinned per". Rewrap the paragraph to even line lengths.
   Evidence: the diff hunk at @@ -53,9 +66,11 @@.
4. .claude/workflows/tm-review-changes.js:323, nit. .filter((f) => f.severity !== 'must-fix') throws on a null entry in findings, while mustFixDeduped skips null entries (!f). Use .filter((f) => f && f.severity !== 'must-fix') so the two paths treat malformed worker output the same way.
   Evidence: tm-review-changes.js:101 if (!f || f.severity !== 'must-fix') continue.
5. PR feat: verify stage in tm-review-changes with spec-format item transforms #419 body, "Notes for the reviewer", nit. It still says "see the wall-clock placeholder above", but the figure is now filled in. Change it to "see the wall-clock figure above".
   Evidence: PR body, last bullet before the attribution line.
6. PR feat: verify stage in tm-review-changes with spec-format item transforms #419 body, "What changed", nit. The "Docs:" bullet leaves out docs/team-guide-rationale.md. That edit is in scope: it fixes the same "reviewers plus one critic" drift as D12, and CLAUDE.md says "the doc is corrected in the same PR". Append ", team-guide-rationale.md worked-example line" to that bullet.
   Evidence: the diff touches docs/team-guide-rationale.md:153; the Docs bullet lists only adapter-interface.md, README and SKILL.md.
   CHECKS: n/a
   LESSONS: A sub-plan that moves ownership of a list field from the model to the script should also name the fields derived from it (here the verdict), or the gap only shows up in review after the live run.

Lead note: nits 5 and 6 are PR-body text, not part of the diff; the lead applied both verbatim to the PR body. Findings 1-4 are left for the human review.

[sv-tmueller]

Reviewer fix round 1/3 (owner-requested before merge, batch #417): finding 1 (should-fix) applied in 69800c2 with the reviewer's exact change and two finalizeReport tests. The lead then merged origin/main in 93bb0b1 (PRs #422, #421, #420 landed) and resolved plugin.json to 2.9.0. Re-test and re-review run on 93bb0b1.

[sv-tmueller]

Tester report, re-test after review fix round 1/3 (batch #417)

VERDICT: PASS
COMMIT: 93bb0b1
FINDINGS: none
UNTESTED CLAIMS: none. Observation, not a finding: the new finalizeReport branch has no test for "no mustFix array" or "empty mustFix with changes-requested". I checked both by hand, see below.
Evidence per check:

1. git diff 57416579 69800c2 --stat shows only .claude/workflows/tm-review-changes.js (+3/-1, one line plus one comment sentence) and .claude/workflows/__tests__/helpers.test.mjs (+15, the two cases). I ran the post-fix helpers.test.mjs against a scratch copy of .claude/workflows with the pre-fix tm-review-changes.js (from 5741657). Case (a) fails there (not ok 2 - recomputes the verdict to approve when only refuted findings were removed) and case (b) passes. Totals were pass 57, fail 1.
2. I called finalizeReport on the real source from /tmp/attack.mjs.

• Refuted-only gives approve and an empty mustFix.
• Refuted plus an unverified or confirmed finding that stays in mustFix gives changes-requested.
• Only unverified or only confirmed gives changes-requested.
• No mustFix array gives changes-requested, with mustFix undefined.
• Empty mustFix with changes-requested, nothing refuted, keeps changes-requested.
• Empty mustFix with a non-matching refuted entry keeps changes-requested.
• A refuted entry that is not in mustFix keeps changes-requested.
• mustFix: null keeps changes-requested.
  Only an all-refuted strip flips the verdict.

3. git diff origin/main 93bb0b1 --name-only matches the gh pr view 419 file list exactly (17 files). git grep -nE '^(<{7}|={7}|>{7})' finds no conflict markers. origin/main is an ancestor of HEAD. git diff 57416579 93bb0b1 shows the extra files (tm-ab-test, token-report, team-guide, lead-effort docs and fixtures) are merged-in main content from docs: add headless claude -p checklist to tm-ab-test #420, feat: token report covers workflow subagents and adds a per-agent wall-clock table #421 and docs: tidy lead-effort note and report nits after batch 403 #422, plus plugin.json. Those files are not in the PR's own diff against origin/main.
4. safeRef hashes to a2a88696df4a in all three workflows (tm-map-codebase, tm-review-codebase, tm-review-changes). criticWithFallback hashes to bcac7b502a35 in all three.
5. npm test exits 0 (467 pass, 0 fail). node scripts/check-version-bump.mjs origin/main HEAD exits 0 ("plugin.json version changed (2.8.1 -> 2.9.0)").

[sv-tmueller]

Reviewer report, re-review after fix round 1/3 (batch #417)

VERDICT: APPROVE
STAGE: quality
FINDINGS:

1. .claude/workflows/tests/helpers.test.mjs:561, nit. No test covers the report.mustFix.length > 0 guard at tm-review-changes.js:124. If the guard were deleted, every test would still pass. The guard is what stops the script from overriding a critic verdict when the script removed nothing. The two inputs that reach it, { mustFix: [] } with no verdict and an approve verdict, never assert a changes-requested result. Required change: after the "keeps changes-requested when a non-refuted finding remains" case, add test('leaves the verdict alone when nothing was refuted', () => { const out = finalizeReport({ verdict: 'changes-requested', mustFix: [] }, [], []); assert.equal(out.verdict, 'changes-requested') }).
   Evidence: the tester's re-test observation ("no test for ... empty mustFix with changes-requested"). helpers.test.mjs:539-597 has no such assertion.
2. PR feat: verify stage in tm-review-changes with spec-format item transforms #419 body, nit. Four lines went stale after 69800c2 and 93bb0b1. Required change: (a) "What changed" bullet 2: append ", and sets the verdict to approve when that strip empties mustFix". (b) "Version bump 2.7.0 to 2.8.0. P1 (Token report: include workflow subagents and add a per-agent wall-clock table #416) and P2 also bump it; the conflict is resolved at merge time." becomes "Version bump 2.8.1 to 2.9.0, after docs: add headless claude -p checklist to tm-ab-test #420 and feat: token report covers workflow subagents and adds a per-agent wall-clock table #421 merged first (merge commit 93bb0b1)." (c) Live-run bullet "Should-fix the run raised": append " Fixed in 69800c2 (reviewer fix round 1/3): the verdict is recomputed to approve when only refuted findings were removed. The reworded-finding half stays out of scope (D2 specifies an exact key)." (d) "Verification": "429 tests" becomes "467 tests at 93bb0b1".
   Evidence: the PR body line "Version bump 2.7.0 to 2.8.0" against git diff origin/main 93bb0b1 -- .claude/.claude-plugin/plugin.json (2.8.1 -> 2.9.0). The tester re-test reported 467 pass.
   CHECKS: n/a
   LESSONS: When the lead merges main to settle a version-bump conflict, refresh the PR body's version line and check counts in the same step, or the human merge gate reads stale numbers.

Lead note: nit 2 (PR body) applied verbatim by the lead before merge, with (a) placed in the finalizeReport bullet it describes. Nit 1 (one extra guard test) is left as a follow-on; the tester checked that case by hand at 93bb0b1 (verdict unchanged).