From b30df5c9d6c2bf4cd33033a2f4e87d9f38f40f7d Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 19:31:09 +0000 Subject: [PATCH] fix(ship): an output with no readable recorded file is dirty even with no ids (GST-541 fix-check 3) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-541-verify-binding.md | 4 ++-- lib/review-ledger.ts | 4 ++-- test/review-dispatch.test.ts | 10 +++++++--- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/changelog.d/gst-541-verify-binding.md b/changelog.d/gst-541-verify-binding.md index 0a8148497..1182c4751 100644 --- a/changelog.d/gst-541-verify-binding.md +++ b/changelog.d/gst-541-verify-binding.md @@ -1,11 +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, 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. +**`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 or an output event with no file recorded (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. +- `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, and so does one edited to drop its file and ids. 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. diff --git a/lib/review-ledger.ts b/lib/review-ledger.ts index b33c57329..fdda655d4 100644 --- a/lib/review-ledger.ts +++ b/lib/review-ledger.ts @@ -307,11 +307,11 @@ export function verify(ledger: string): { rows: VerifyRow[]; clean: boolean; bri 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 && (ids.length || typeof o.file === 'string')) { + if (text === undefined) { status = 'MISMATCH'; notes.push(typeof o.file === 'string' ? 'output_file_missing' : 'output_file_unrecorded'); differs.push(...ids); - } else if (text !== undefined && promptHash(text) === o.sha256) { + } else if (promptHash(text) === o.sha256) { 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 (unavailableOutput(o) && parsed.length) { status = 'MISMATCH'; notes.push('unavailable_output_has_findings'); } diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index 7d59830a7..b7422df11 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -540,10 +540,10 @@ 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, 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; + // Value: protects=an output event edited to read like a refused reply (no ids, review false, tree_ok false) or to drop its file + // 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 + // 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', () => { const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: ROOT, encoding: 'utf8', timeout: 30_000 }).stdout.trim(); @@ -574,6 +574,10 @@ describe('gstack-review-dispatch sent, collect, judge and verify: nothing change 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 unfiled = voice(findings); + unfiled.forge({ file: null, ids: [] }); + const u = unfiled.verify(); + expect([u.status, u.stdout.includes('output_file_unrecorded'), u.stdout.includes('VERIFY: dirty')]).toEqual([1, true, true]); const flagged = voice(findings); flagged.forge({ ids: [], review: false, tree_ok: false }); flagged.send();