From 1b53e50c469ee3ebb47671e1dd5266db7a60ed53 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 18:05:44 +0000 Subject: [PATCH] fix(ship): verify re-derives each merged row by index (GST-541) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-541-verify-binding.md | 7 ++++++ docs/review-policy.md | 5 +++- lib/review-ledger.ts | 12 ++++++++- test/review-dispatch.test.ts | 35 ++++++++++++++++++++++++++- 4 files changed, 56 insertions(+), 3 deletions(-) create mode 100644 changelog.d/gst-541-verify-binding.md diff --git a/changelog.d/gst-541-verify-binding.md b/changelog.d/gst-541-verify-binding.md new file mode 100644 index 000000000..97b17dd72 --- /dev/null +++ b/changelog.d/gst-541-verify-binding.md @@ -0,0 +1,7 @@ +**`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. + +### Tests +- `test/review-dispatch.test.ts`: a forged label and a summary copied onto a blocker's id both verify dirty. Both verified clean at main c830c9b. + +### Docs +- `docs/review-policy.md` (verify) describes the per-row check. diff --git a/docs/review-policy.md b/docs/review-policy.md index 301d187e5..a16968f30 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -498,7 +498,10 @@ ones, and the arm lost blockers. `bin/gstack-review-dispatch` closes both ends: missing coverage leaves the review incomplete. A mismatched send re-sent byte-exact before anything was collected is void and counted (`voided=N` on the verdict line); one that produced an output stays `MISMATCH`. Then output hashes - against the merged lines they produced, and the judgment rerun against the + against the merged lines they produced, and each merged line against the finding collect + parses at that id's index from the unchanged output file, label and raw text byte for byte + (`merged_line_differs` on the OUTPUT and JUDGE rows otherwise, GST-541), so an edited + merged.jsonl cannot lower a finding or swap its text, and the judgment 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 --out --pass -- ` runs one pass from a diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index 0d945d0c4..29e4e64f5 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -247,6 +247,7 @@ export function verify(ledger: string): { rows: VerifyRow[]; clean: boolean; bri return { at, next, reader, label: nth > 1 ? `${reader}@${nth}` : reader }; }); let voided = 0; + const differs: string[] = []; for (const p of episodes) { const r = events[p.at]!; const inside = events.map((e, i) => ({ e, i })).filter(({ e, i }) => i > p.at && i < p.next && e.reader === p.reader); @@ -279,12 +280,19 @@ export function verify(ledger: string): { rows: VerifyRow[]; clean: boolean; bri const lastSent = p ? events.slice(p.at + 1, i).filter(e => e.event === 'sent' && e.reader === reader).at(-1) : undefined; if (!p) { status = 'MISMATCH'; notes.push('brief_not_rendered'); } else if (!lastSent || lastSent.sha256 !== events[p.at]!.sha256) { status = 'MISMATCH'; notes.push('brief_not_sent_verbatim'); } - if (typeof o.file === 'string' && fs.existsSync(o.file) && promptHash(fs.readFileSync(o.file, 'utf8')) !== o.sha256) { status = 'MISMATCH'; notes.push('output_file_changed'); } + const text = typeof o.file === 'string' && fs.existsSync(o.file) ? fs.readFileSync(o.file, 'utf8') : undefined; + 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[] = []; try { lines = typeof o.merged === 'string' ? readJsonl(o.merged).filter(l => ids.includes(l.id)) : []; } catch { notes.push('merged_unreadable'); status = 'MISMATCH'; } if (lines.length !== ids.length) { status = 'MISMATCH'; notes.push(`merged_lines=${lines.length}/${ids.length}`); } if (lines.some(l => l.output_sha256 !== o.sha256)) { status = 'MISMATCH'; notes.push('merged_line_from_other_output'); } + // Each merged row must be the finding collect parses at that id's index from the unchanged output, label and raw text byte for byte. + if (text !== undefined && promptHash(text) === o.sha256 && !unavailableOutput(o)) { + const parsed = parseFindings(reader, text); + const off = ids.filter((id, k) => { const m = lines.find(l => l.id === id); return !m || parsed[k]?.label !== m.label || parsed[k]?.raw !== m.raw; }); + if (off.length || parsed.length !== ids.length) { status = 'MISMATCH'; notes.push(`merged_line_differs=${JSON.stringify(off)}`); differs.push(...off); } + } 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) : []; @@ -312,6 +320,8 @@ export function verify(ledger: string): { rows: VerifyRow[]; clean: boolean; bri rerun = judge(readJsonl(String(v.merged)), readJsonl(String(v.judged))); } catch { notes.push('judged_or_merged_unreadable'); } if (rerun && !rerun.ok) notes.push(`rerun_rejects dropped=${JSON.stringify(rerun.dropped)} downgraded=${JSON.stringify(rerun.downgraded)}`); + const forged = differs.filter(id => judgedIds.has(id)); + if (forged.length) notes.push(`merged_line_differs=${JSON.stringify(forged)}`); const status = !v.ok ? 'REJECTED' : notes.length ? 'MISMATCH' : 'OK'; rows.push({ kind: 'JUDGE', reader: '-', status, detail: `dropped=${JSON.stringify(v.dropped)} downgraded=${JSON.stringify(v.downgraded)} added=${v.added} promoted=${v.promoted} folded=${v.folded}${notes.length ? ' ' + notes.join(' ') : ''}` }); } else if (events.some(e => e.event === 'output')) rows.push({ kind: 'JUDGE', reader: '-', status: 'MISSING', detail: 'outputs were collected but no judged set was checked' }); diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index b76de4a38..4e509ebad 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -4,7 +4,7 @@ import * as fs from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { BriefError, promptHash, renderBrief, sectionTexts } from '../lib/review-brief'; -import { collect, judge, parseFindings, type MergedLine } from '../lib/review-ledger'; +import { collect, judge, parseFindings, readJsonl, type MergedLine } from '../lib/review-ledger'; const ROOT = path.resolve(import.meta.dir, '..'); const BIN = path.join(ROOT, 'bin', 'gstack-review-dispatch'); @@ -475,4 +475,37 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change const v = run(['verify', '--ledger', ledger]); expect([v.status, v.stdout.trim()]).toEqual([3, "VERIFY: no brief records (this run's ledger kept no prompts; dispatched-vs-rendered cannot be checked)"]); }); + + // Value: protects=verify re-derives every merged row from the reader's unchanged output by its index, so a merged.jsonl edited + // after collect cannot lower a finding or swap a blocker's text for a summary and still verify clean; fails_when=a forged label + // or a copied raw line on a real blocker's id verifies clean (GST-541: both did at main c830c9b); why_new=verify compared only + // output hashes and line counts; seam=verify; no_claim=that an output collected without a recorded file can be re-derived + test('verify re-derives each merged row by index: a forged label or a summary copied onto a blocker id is dirty', () => { + const blocker = '{"severity":"CRITICAL","path":"log.ts","line":3,"category":"security","summary":"bearer token written to a public log","specialist":"testing"}'; + const summary = '{"severity":"INFORMATIONAL","path":"a.ts","line":9,"category":"style","summary":"I found nothing CRITICAL.","specialist":"testing"}'; + const setup = () => { + const p = pass(); + run(['sent', '--ledger', p.ledger, '--role', 'specialist', '--name', 'testing', '--sent', p.file]); + const out = p.w('spec.out', blocker + '\n' + summary + '\n'); + expect(run(['collect', '--ledger', p.ledger, '--role', 'specialist', '--name', 'testing', '--output', out, '--merged', p.merged]).status).toBe(0); + return p; + }; + const judgeAll = (p: ReturnType, severity: string) => run(['judge', '--ledger', p.ledger, '--merged', p.merged, '--judged', p.w('judged.jsonl', + readJsonl(p.merged).map(m => JSON.stringify({ id: m.id, severity: m.id.endsWith('#1') ? severity : 'INFORMATIONAL' })).join('\n') + '\n')]); + const clean = setup(); + expect(judgeAll(clean, 'CRITICAL').status).toBe(0); + expect(run(['verify', '--ledger', clean.ledger]).stdout).toContain('VERIFY: clean'); + const forge = (p: ReturnType, edit: (rows: MergedLine[]) => void) => { + const rows = readJsonl(p.merged); + edit(rows); + fs.writeFileSync(p.merged, rows.map(r => JSON.stringify(r)).join('\n') + '\n'); + expect(judgeAll(p, 'INFORMATIONAL').stdout.trim()).toBe('JUDGMENT: ok added=0 promoted=0 folded=0'); + return run(['verify', '--ledger', p.ledger]); + }; + const label = forge(setup(), rows => { rows[0]!.label = 'INFORMATIONAL'; }); + expect([label.status, label.stdout]).toEqual([1, expect.stringContaining('merged_line_differs=["specialist/testing#1"]')]); + const copied = forge(setup(), rows => { rows[0]!.label = rows[1]!.label; rows[0]!.raw = rows[1]!.raw; }); + expect([copied.status, copied.stdout]).toEqual([1, expect.stringContaining('merged_line_differs=["specialist/testing#1"]')]); + expect(copied.stdout).toMatch(/^JUDGE - MISMATCH .*merged_line_differs/m); + }); });