From f482012002f3875445c4984193fc8dc7beddc654 Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 08:39:06 +0000 Subject: [PATCH] fix(ship): verify accepts an output as a review only when its reply names the render's tree (GST-541 red team) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/review-ledger.ts | 42 +++++++++++++++++++----------------- test/review-dispatch.test.ts | 11 +++++++++- 2 files changed, 32 insertions(+), 21 deletions(-) diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index f33fddf12..d9de23553 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -236,18 +236,6 @@ export function recordJudgment(ledger: string, merged: MergedLine[], judged: Jud return res; } -/** - * Whether a recorded output was a review: it has findings, or its file (unchanged since collect) carries the reader's clean marker, so - * a ledger written before collect recorded the field is judged by the same rule; with no readable file, collect's own review field. - */ -function reviewed(o: LedgerEvent): boolean { - if (o.tree_ok === false || unavailableOutput(o)) return false; - if (Array.isArray(o.ids) && o.ids.length) return true; - let text: string | undefined; - try { text = typeof o.file === 'string' ? fs.readFileSync(o.file, 'utf8') : undefined; } catch { /* unreadable */ } - 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 { @@ -256,6 +244,23 @@ function unchangedOutput(o: LedgerEvent): string | undefined { } catch { return undefined; } } +/** 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 treeHolds = (render: LedgerEvent | undefined, reply: string | undefined) => !render || !('tree' in render) + || (reply !== undefined && checkReviewedTree(reply, typeof render.tree === 'string' ? render.tree : null).ok); + +/** + * Whether a recorded output was a review: its unchanged reply names its render's tree, and it has findings or carries the reader's + * clean marker, so a ledger written before collect recorded the field is judged by the same rule; with no readable file, collect's own + * review field. + */ +function reviewed(o: LedgerEvent, render?: LedgerEvent): boolean { + if (o.tree_ok === false || unavailableOutput(o)) return false; + const text = unchangedOutput(o); + if (!treeHolds(render, text)) return false; + if (Array.isArray(o.ids) && o.ids.length) return true; + return text !== undefined ? hasCleanMarker(String(o.reader), text) : o.review === true; +} + /** 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[]) : [])); @@ -293,7 +298,7 @@ export function verify(ledger: string): { rows: VerifyRow[]; clean: boolean; bri voided += epVoided; const lastMatch = [...sent].reverse().find(({ e }) => e.sha256 === r.sha256); const outputs = lastMatch ? inside.filter(({ e, i }) => e.event === 'output' && i > lastMatch.i) : []; - const collected = outputs.some(({ e }) => reviewed(e)); + const collected = outputs.some(({ e }) => reviewed(e, r)); const unavailable = !collected && outputs.some(({ e }) => unavailableOutput(e)); const status = !sent.length ? 'NOT_SENT' : mismatched ? 'MISMATCH' : collected ? 'MATCH' : unavailable ? 'UNAVAILABLE' : 'NOT_COLLECTED'; const tampered = fileNow && fileNow !== r.sha256 ? ' file_changed_since_render' : ''; @@ -310,10 +315,7 @@ 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[] = []; @@ -332,17 +334,17 @@ export function verify(ledger: string): { rows: VerifyRow[]; clean: boolean; bri 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; 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 (parsed.length !== ids.length && !(o.tree_ok === false && !ids.length && !treeHolds(render, text))) { status = 'MISMATCH'; notes.push(`merged_count_differs=${parsed.length}/${ids.length}`); } } } - if (status === 'MATCH' && !reviewed(o)) { + if (status === 'MATCH' && !reviewed(o, render)) { // 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) && treeHolds(unchangedOutput(e))); + const review = later.findIndex(e => e.event === 'output' && e.reader === reader && reviewed(e, render)); 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'); } - else if (o.tree_ok === false) { status = 'NOT_REVIEWED'; notes.push(`wrong_tree reported=${JSON.stringify(o.reported_tree)} expected=${JSON.stringify(o.tree)}`); } + else if (o.tree_ok === false || !treeHolds(render, text)) { status = 'NOT_REVIEWED'; notes.push(`wrong_tree reported=${JSON.stringify(o.reported_tree)} expected=${JSON.stringify(o.tree)}`); } else { status = 'NOT_REVIEWED'; notes.push('no_finding_no_clean_marker'); } } rows.push({ kind: 'OUTPUT', reader: p?.label ?? reader, status, detail: `sha=${short(o.sha256)} findings=${ids.length}${notes.length ? ' ' + notes.join(' ') : ''}` }); diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index 3d1cbdb6b..3640a6909 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -741,7 +741,7 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change // 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, or an output with no recorded file passes on its review field (fix-checks at 721539c, 56ed6e3 and 50dc73c: 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', () => { + test('an output event edited to look refused, or a refused one edited to look accepted, 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) => { @@ -788,6 +788,15 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change 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]); + const elsewhere = pass(); + run(['sent', '--ledger', elsewhere.ledger, '--role', 'specialist', '--name', 'testing', '--sent', elsewhere.file]); + const refused = run(['collect', '--ledger', elsewhere.ledger, '--role', 'specialist', '--name', 'testing', '--output', elsewhere.w('else.out', `REVIEWED_TREE: ${'b'.repeat(40)} /elsewhere\nNO FINDINGS\n`), '--merged', elsewhere.merged]); + expect(refused.stdout.startsWith('COLLECT_WRONG_TREE: reader=specialist/testing')).toBe(true); + fs.writeFileSync(elsewhere.ledger, readJsonl>(elsewhere.ledger).map(e => JSON.stringify(e.event === 'output' ? { ...e, tree_ok: true, review: true, reported_tree: e.tree } : e)).join('\n') + '\n'); + fs.writeFileSync(elsewhere.merged, ''); + expect(run(['judge', '--ledger', elsewhere.ledger, '--merged', elsewhere.merged, '--judged', elsewhere.w('judged.jsonl', '')]).status).toBe(0); + const e = run(['verify', '--ledger', elsewhere.ledger]); + expect([e.status, /^OUTPUT specialist\/testing NOT_REVIEWED .*wrong_tree/m.test(e.stdout), e.stdout.includes('VERIFY: dirty')]).toEqual([1, true, true]); }); });