fix(ship): verify re-derives a tree refusal from the reply before it skips the count check or voids an output (GST-541 fix-check 2)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

changelog.d/gst-541-verify-binding.md

@@ -1,10 +1,11 @@
-**`gstack-review-dispatch verify` now catches a merged findings file edited after collect.** Verify used to compare only output hashes and line counts. So a `merged.jsonl` row whose label was lowered, or whose text was replaced with another line's summary, passed with `VERIFY: clean` once the judge agreed with the edited row. Verify now re-parses each reader's unchanged output and checks every merged row against the finding at its index, label and raw text byte for byte. A mismatch prints `merged_line_differs` on the OUTPUT and JUDGE rows, and verify is dirty. A missing output file (even for a reply with no findings), a judgment run on a copy of the merged file, and merged rows that collect never wrote are dirty too.
+**`gstack-review-dispatch verify` now catches a merged findings file edited after collect.** Verify used to compare only output hashes and line counts. So a `merged.jsonl` row whose label was lowered, or whose text was replaced with another line's summary, passed with `VERIFY: clean` once the judge agreed with the edited row. Verify now re-parses each reader's unchanged output and checks every merged row against the finding at its index, label and raw text byte for byte. A mismatch prints `merged_line_differs` on the OUTPUT and JUDGE rows, and verify is dirty. A missing output file (even for a reply with no findings), a judgment run on a copy of the merged file, merged rows that collect never wrote, and an output whose finding count no longer matches its recorded ids are dirty too. A reply refused for naming the wrong checkout is still allowed to hold findings collect never merged, but only when verify re-checks its tree line and finds it wrong.
 
 ### Known
 - Deleting a whole output event together with its merged rows still verifies clean, because the ledger keeps no anchor for it. It is listed for the structured-findings plan.
 
 ### Tests
 - `test/review-dispatch.test.ts`: a forged label, a summary copied onto a blocker's id, a deleted output file, a judgment on a forged copy and an extra merged row all verify dirty. Each verified clean at main c830c9b.
+- `test/review-dispatch.test.ts`: an output event edited to look like a wrong-tree refusal (no ids, `review: false`, `tree_ok: false`) verifies dirty, even after a re-sent brief is answered. A real wrong-tree reply stays `NOT_REVIEWED` until a re-sent brief gets a review, then verifies clean.
 
 ### Docs
 - `docs/review-policy.md` (verify) describes the per-row check.

docs/review-policy.md

@@ -504,8 +504,12 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends:
    merged.jsonl cannot lower a finding or swap its text. A recorded output file that is missing
    (`output_file_missing`), a judgment run on a merged file collect did not write
    (`judge_merged_not_collected`) and merged rows collect never recorded
-   (`merged_rows_not_collected`) are dirty too. Deleting a whole output event with its rows is
-   not caught yet, because the ledger has no anchor for it, and the judgment rerun against the
+   (`merged_rows_not_collected`) are dirty too, and so is an output whose re-parsed findings
+   outnumber or undercount the ids collect recorded (`merged_count_differs`). The one exception
+   is a reply collect refused for its tree: verify re-checks the reply's `REVIEWED_TREE` line
+   against the render's tree, and such an output stays `NOT_REVIEWED` until a re-sent brief is
+   answered by a reply that passes the same check. Deleting a whole output event with its rows is
+   not caught yet, because the ledger has no anchor for it. The judgment is then rerun against the
    recorded judged set, covering every collected id. A ledger from a run that kept no
    prompts prints `VERIFY: no brief records` (exit 3), never clean.
 6. `pass --base <b> --out <dir> --pass <id> -- <plan flags>` runs one pass from a

lib/review-ledger.ts

@@ -16,7 +16,7 @@
  */
 import * as fs from 'node:fs';
 import * as path from 'node:path';
-import { isReviewedTreeLine, promptHash, type TreeCheck } from './review-brief';
+import { checkReviewedTree, isReviewedTreeLine, promptHash, type TreeCheck } from './review-brief';
 
 export interface LedgerEvent { event: 'rendered' | 'sent' | 'output' | 'judgment' | 'judge' | 'program'; [k: string]: unknown }
 export const readerKey = (role: string, name?: string) => (name ? `${role}/${name}` : role);
@@ -234,6 +234,14 @@ function reviewed(o: LedgerEvent): boolean {
   return text !== undefined && promptHash(text) === o.sha256 ? hasCleanMarker(String(o.reader), text) : o.review === true;
 }
 
+/** A recorded output's file text while it still hashes to what collect recorded. */
+function unchangedOutput(o: LedgerEvent): string | undefined {
+  try {
+    const text = typeof o.file === 'string' ? fs.readFileSync(o.file, 'utf8') : undefined;
+    return text !== undefined && promptHash(text) === o.sha256 ? text : undefined;
+  } catch { return undefined; }
+}
+
 /** Every id collect recorded, in order. */
 const collectedIds = (events: LedgerEvent[]) => events.filter(e => e.event === 'output').flatMap(e => (Array.isArray(e.ids) ? (e.ids as string[]) : []));
 
@@ -288,6 +296,10 @@ export function verify(ledger: string): { rows: VerifyRow[]; clean: boolean; bri
     else if (!lastSent || lastSent.sha256 !== events[p.at]!.sha256) { status = 'MISMATCH'; notes.push('brief_not_sent_verbatim'); }
     let text: string | undefined;
     try { text = typeof o.file === 'string' ? fs.readFileSync(o.file, 'utf8') : undefined; } catch { /* missing or unreadable */ }
+    // Whether a reply names the tree its render recorded (GST-482), checked on the reply itself, never on an output event's editable fields.
+    const render = p ? events[p.at]! : undefined;
+    const treeHolds = (reply: string | undefined) => !render || !('tree' in render)
+      || (reply !== undefined && checkReviewedTree(reply, typeof render.tree === 'string' ? render.tree : null).ok);
     if (text !== undefined && promptHash(text) !== o.sha256) { status = 'MISMATCH'; notes.push('output_file_changed'); }
     const ids = Array.isArray(o.ids) ? (o.ids as string[]) : [];
     let lines: MergedLine[] = [];
@@ -305,14 +317,14 @@ export function verify(ledger: string): { rows: VerifyRow[]; clean: boolean; bri
       if (unavailableOutput(o) && parsed.length) { status = 'MISMATCH'; notes.push('unavailable_output_has_findings'); }
       else if (!unavailableOutput(o)) {
         if (off.length) { status = 'MISMATCH'; notes.push(`merged_line_differs=${JSON.stringify(off)}`); differs.push(...off); }
-        // A reply collect refused for its tree (GST-482) records no ids whatever it holds; tree_ok false always verifies NOT_REVIEWED.
-        if (parsed.length !== ids.length && !(o.tree_ok === false && !ids.length)) { status = 'MISMATCH'; notes.push(`merged_count_differs=${parsed.length}/${ids.length}`); }
+        // A reply collect refused for its tree (GST-482) records no ids whatever it holds; the refusal is re-derived from the reply.
+        if (parsed.length !== ids.length && !(o.tree_ok === false && !ids.length && !treeHolds(text))) { status = 'MISMATCH'; notes.push(`merged_count_differs=${parsed.length}/${ids.length}`); }
       }
     }
     if (status === 'MATCH' && !reviewed(o)) {
       // A non-review the same dispatch later replaced with a review (the brief re-sent verbatim, then answered) is void, counted like a voided send.
       const later = p ? events.slice(i + 1, p.next) : [];
-      const review = later.findIndex(e => e.event === 'output' && e.reader === reader && reviewed(e));
+      const review = later.findIndex(e => e.event === 'output' && e.reader === reader && reviewed(e) && treeHolds(unchangedOutput(e)));
       const replaced = review > 0 && later.slice(0, review).some(e => e.event === 'sent' && e.reader === reader && e.sha256 === events[p!.at]!.sha256);
       if (unavailableOutput(o)) { status = 'UNAVAILABLE'; notes.push(`reason=${JSON.stringify(o.unavailable)}`); }
       else if (replaced) { status = 'VOIDED'; voided++; notes.push('replaced_by_a_review'); }

test/review-dispatch.test.ts

@@ -540,37 +540,54 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change
     const extra = forge(setup(), rows => { rows[0]!.label = 'INFORMATIONAL'; rows.push({ ...rows[1]!, id: 'specialist/testing#9' }); });
     expect([extra.status, extra.stdout.includes('merged_rows_not_collected=["specialist/testing#9"]')]).toEqual([1, true]);
   });
-  // Value: protects=an output event edited to read like a refused reply (no ids, review false) cannot drop its findings and still verify
-  //   clean; fails_when=verify skips the count check on review:false alone, so a clean marker keeps the output MATCH (fix-check at 721539c:
-  //   VERIFY: clean); why_new=the GST-482 count exemption trusted collect's review field; seam=verify; no_claim=that a deleted output
-  //   event is caught (#3719)
-  test('an output event edited to no ids and review false is dirty; a real wrong-tree reply with findings is NOT_REVIEWED, no count mismatch', () => {
+  // Value: protects=an output event edited to read like a refused reply (no ids, review false, tree_ok false) cannot drop its findings
+  //   and still verify clean, while a reply collect really refused for its tree stays NOT_REVIEWED until a re-sent brief is answered;
+  //   fails_when=verify skips the count check on the editable review or tree_ok field, so a clean marker keeps the output MATCH or a
+  //   later review voids it (fix-checks at 721539c and 56ed6e3: VERIFY: clean); why_new=the GST-482 count exemption trusted collect's
+  //   fields; seam=verify; no_claim=that a deleted output event is caught (#3719)
+  test('an output event edited to look refused is dirty; a real wrong-tree reply is NOT_REVIEWED, then voided by a review', () => {
     const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: ROOT, encoding: 'utf8', timeout: 30_000 }).stdout.trim();
+    const findings = '1. **CRITICAL**: auth.ts:3 leaks a token\nBlocks: yes\n\nRecommendation: request changes\n';
     const voice = (reply: string) => {
       const out = tmp();
       const ledger = path.join(out, 'review-ledger.jsonl');
       const merged = path.join(out, 'merged.jsonl');
-      run(['sent', '--ledger', ledger, '--role', 'outside-voice', '--sent', field(run(['render', '--role', 'outside-voice', '--out', out, '--diff-base', head]).stdout, 'file')]);
-      const file = path.join(out, 'ov.out');
-      fs.writeFileSync(file, reply);
-      const c = run(['collect', '--ledger', ledger, '--role', 'outside-voice', '--output', file, '--merged', merged]);
-      const judgeNone = () => run(['judge', '--ledger', ledger, '--merged', merged, '--judged', path.join(out, 'judged.jsonl')]);
-      fs.writeFileSync(path.join(out, 'judged.jsonl'), '');
-      return { ledger, merged, c, judgeNone };
+      const brief = field(run(['render', '--role', 'outside-voice', '--out', out, '--diff-base', head]).stdout, 'file');
+      const send = () => run(['sent', '--ledger', ledger, '--role', 'outside-voice', '--sent', brief]);
+      let n = 0;
+      const take = (text: string) => { const f = path.join(out, `ov${++n}.out`); fs.writeFileSync(f, text); return run(['collect', '--ledger', ledger, '--role', 'outside-voice', '--output', f, '--merged', merged]); };
+      send();
+      const c = take(reply);
+      const forge = (edit: Record<string, unknown>) => {
+        fs.writeFileSync(ledger, readJsonl<Record<string, unknown>>(ledger).map(e => JSON.stringify(e.event === 'output' ? { ...e, ...edit } : e)).join('\n') + '\n');
+        fs.writeFileSync(merged, '');
+      };
+      const verify = () => {
+        fs.writeFileSync(path.join(out, 'judged.jsonl'), '');
+        expect(run(['judge', '--ledger', ledger, '--merged', merged, '--judged', path.join(out, 'judged.jsonl')]).status).toBe(0);
+        return run(['verify', '--ledger', ledger]);
+      };
+      return { c, send, take, forge, verify };
     };
-    const findings = '1. **CRITICAL**: auth.ts:3 leaks a token\nBlocks: yes\n\nRecommendation: request changes\n';
-    const forged = voice(findings);
-    expect(forged.c.status).toBe(0);
-    fs.writeFileSync(forged.ledger, readJsonl<Record<string, unknown>>(forged.ledger).map(e => JSON.stringify(e.event === 'output' ? { ...e, ids: [], review: false } : e)).join('\n') + '\n');
-    fs.writeFileSync(forged.merged, '');
-    expect(forged.judgeNone().status).toBe(0);
-    const v = run(['verify', '--ledger', forged.ledger]);
+    const noIds = voice(findings);
+    expect(noIds.c.status).toBe(0);
+    noIds.forge({ ids: [], review: false });
+    const v = noIds.verify();
     expect([v.status, v.stdout.includes('merged_count_differs=1/0'), v.stdout.includes('VERIFY: dirty')]).toEqual([1, true, true]);
+    const flagged = voice(findings);
+    flagged.forge({ ids: [], review: false, tree_ok: false });
+    flagged.send();
+    expect(flagged.take('Recommendation: approve\n').status).toBe(0);
+    const f = flagged.verify();
+    expect([f.status, f.stdout.includes('merged_count_differs=1/0'), f.stdout.includes('VERIFY: dirty')]).toEqual([1, true, true]);
     const wrong = voice(`REVIEWED_TREE: ${'b'.repeat(40)} /elsewhere\n` + findings);
     expect([wrong.c.status, wrong.c.stdout.startsWith('COLLECT_WRONG_TREE: reader=outside-voice')]).toEqual([1, true]);
-    expect(wrong.judgeNone().status).toBe(0);
-    const w = run(['verify', '--ledger', wrong.ledger]);
+    const w = wrong.verify();
     expect([w.status, /^OUTPUT outside-voice NOT_REVIEWED .*wrong_tree/m.test(w.stdout), w.stdout.includes('merged_count_differs')]).toEqual([1, true, false]);
+    wrong.send();
+    expect(wrong.take('Recommendation: approve\n').status).toBe(0);
+    const r = wrong.verify();
+    expect([r.status, r.stdout.includes('replaced_by_a_review'), r.stdout.includes('VERIFY: clean briefs=1 outputs=2 voided=1')]).toEqual([0, true, true]);
   });
 });