From c42353913337973bb519ee0983dd235747a52a3f Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 09:40:11 +0000 Subject: [PATCH] wave review-53cfb5bfe011: /ship pass 1 fixes - lib/ship-receipt-depth.ts: a merged resume (land after a true merge) no longer recomputes the review_exclude digest over the emptied range, so it lands instead of RECEIPT_INVALID (failing-first land test with review_exclude_paths active). - bin/gstack-review-dispatch render: a standalone targeted render under an active review_exclude_paths refuses and points at the pass manifest voices stage, instead of writing a brief with no selection that leaves verify dirty (failing-first test). - test/default-base.test.ts: #3795 expectations follow main R51 depth (light for an 80-line code diff; runtime_lines=80 still proves the real base). - test/review-dispatch.test.ts: the GST-897 fingerprint test renders from the clean checkout, like its neighbor, because CI rebuilds the tracked lib/diagram-render/dist/diagram-render.html and the reviewer diff ceiling refused the ROOT render. --- bin/gstack-review-dispatch | 1 + lib/ship-receipt-depth.ts | 4 ++-- test/default-base.test.ts | 14 +++++++------- test/review-dispatch.test.ts | 10 ++++++---- test/review-exclude.test.ts | 15 +++++++++++++++ test/ship-queue-land.test.ts | 24 ++++++++++++++++++++++-- 6 files changed, 53 insertions(+), 15 deletions(-) diff --git a/bin/gstack-review-dispatch b/bin/gstack-review-dispatch index b82c66c3b..45808e309 100755 --- a/bin/gstack-review-dispatch +++ b/bin/gstack-review-dispatch @@ -114,6 +114,7 @@ function render(): number { const mb = mergeBaseWith(base); const full = diffBase ? spawnSync('git', ['rev-parse', '--verify', '--quiet', `${diffBase}^{commit}`], { encoding: 'utf8', timeout: 30_000 }).stdout?.trim() : undefined; if (!full || full !== mb) throw new BriefError(`the targeted reader reads the whole diff on every pass: --diff-base must be the merge base with ${base} (${mb ?? 'unresolved'}), got ${diffBase ?? 'none'}`); + if (!flag('--disclosure') && ceilingContext(process.cwd(), base).state === 'active') throw new BriefError(`${base}'s review_exclude_paths is active, so the targeted reader renders in the pass manifest's voices stage (gstack-review-dispatch stage --manifest --stage voices), which carries the exclusion and its disclosure line`); } const checklists = flag('--checklists')?.split(',').map(s => s.trim()).filter(Boolean).map(c => (c.includes('/') ? c : path.join(ROOT, 'review/specialists', `${c}.md`))); for (const c of checklists ?? []) if (!fs.existsSync(c)) throw new BriefError(`checklist not found: ${c}`); diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 835e97ea7..6a7896572 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -243,7 +243,7 @@ export function receiptDepthVerdict(root: string, baseRef: string, r: ShipReceip return { ok: false, line, message: `the receipt ${why}, but head ${head.slice(0, 12)} needs ${need.depth} (${need.reasons.join(', ')}${errs ? `; ${errs}` : ''}); the receipt is unchanged; run /ship on ${head.slice(0, 12)}` }; } -/** bindReceipt plus the owner exceptions and the review depth: what READY and train build accept; the targeted line is reported, never refused on. A base that already contains the head refuses (BASE_CONTAINS_HEAD, GST-774) before any depth is computed, except for a PR GitHub already merged (`merged`: land resuming after a merge-only true merge), whose depth then reads the range with diff-scope's `--merged`. */ +/** bindReceipt plus the owner exceptions and the review depth: what READY and train build accept; the targeted line is reported, never refused on. A base that already contains the head refuses (BASE_CONTAINS_HEAD, GST-774) before any depth is computed, except for a PR GitHub already merged (`merged`: land resuming after a merge-only true merge), whose depth then reads the range with diff-scope's `--merged` and whose exclusion digest is not recomputed over that emptied range. */ export function bindShippedReceipt(root: string, baseRef: string, body: string, pr: string | undefined, accepted: readonly string[], merged = false): | { ok: true; receipt: ShipReceipt; head: string; line: string; depthLine: string; targetedLine: string } | { ok: false; code: ReceiptBindingCode | 'BASE_CONTAINS_HEAD' | 'QUEUE_RECEIPT_DEPTH_RAISED' | 'QUEUE_RECEIPT_EXCEPTION_INVALID' | 'RECEIPT_INVALID' | Extract['code']; message: string; line: string; depthLine?: string; targetedLine?: string } { @@ -256,7 +256,7 @@ export function bindShippedReceipt(root: string, baseRef: string, body: string, if (problems.length) return { ok: false, code: 'QUEUE_RECEIPT_EXCEPTION_INVALID', message: `the receipt's owner exception does not stand: ${problems.join('; ')}`, line: b.line }; const at = headInputs(root, baseRef, b.head, b.receipt, merged); if (at.x.state === 'refused') return { ok: false, code: at.x.code, message: at.x.lines.join('; '), line: b.line }; - const mismatch = excludeMismatch(at.x, b.receipt, b.head, baseRef); + const mismatch = merged ? null : excludeMismatch(at.x, b.receipt, b.head, baseRef); if (mismatch) return { ok: false, code: 'RECEIPT_INVALID', message: mismatch, line: b.line }; const d = receiptDepthVerdict(root, baseRef, b.receipt, b.head, at); const gateTagged = boundScan(b.receipt, b.head) ?? (gateScanNeeded(at.c.paths, at.c.root) ? null : 0); diff --git a/test/default-base.test.ts b/test/default-base.test.ts index 450c83a61..5c3fc12d6 100644 --- a/test/default-base.test.ts +++ b/test/default-base.test.ts @@ -128,14 +128,14 @@ describe('resolveBaseBranch skips an origin/HEAD that names the checked-out bran }); describe('the review depth ship-queue ready binds', () => { - // Value: protects=a READY on the bug checkout never binds a light depth for an 80-line diff; fails_when=the queue compares the - // PR head with itself and reports required=light runtime_lines=0; seam=bindShippedReceipt - test('origin/ refuses BASE_CONTAINS_HEAD; the resolved default base binds standard; the control binds the same', () => { + // Value: protects=a READY on the bug checkout binds the 80-line diff's real depth inputs (light under R51, runtime_lines=80); + // fails_when=the queue compares the PR head with itself and reports runtime_lines=0; seam=bindShippedReceipt + test('origin/ refuses BASE_CONTAINS_HEAD; the resolved default base binds runtime_lines=80; the control binds the same', () => { const body = path.join(fx.root, 'body.md'); const w = bin('gstack-ship-receipt', ['write', '--pr', '7', '--base', 'main', '--version', '1.0.0', '--session-kind', 'unattended', '--body', body], fx.clone); expect(w.status, w.err).toBe(0); const text = fs.readFileSync(body, 'utf8'); - expect(extractReceipt(text).receipt!.review_depth).toMatchObject({ depth: 'standard', runtime_lines: 80 }); + expect(extractReceipt(text).receipt!.review_depth).toMatchObject({ depth: 'light', runtime_lines: 80 }); const head = git(fx.clone, 'rev-parse', 'HEAD'); withEnv(fx.env, () => { const self = bindShippedReceipt(fx.clone, 'origin/pr-branch', text, '7', [head]); @@ -144,9 +144,9 @@ describe('the review depth ship-queue ready binds', () => { expect(self.code as string).toBe('BASE_CONTAINS_HEAD'); expect(self.message).toContain(head.slice(0, 12)); const real = bindShippedReceipt(fx.clone, `origin/${resolveBaseBranch(fx.clone)}`, text, '7', [head]); - expect(real.ok && real.depthLine).toContain('receipt=standard required=standard runtime_lines=80 class=code'); + expect(real.ok && real.depthLine).toContain('receipt=light required=light runtime_lines=80 class=code'); const control = bindShippedReceipt(fx.control, `origin/${resolveBaseBranch(fx.control)}`, text, '7', [head]); - expect(control.ok && control.depthLine).toContain('receipt=standard required=standard runtime_lines=80 class=code'); + expect(control.ok && control.depthLine).toContain('receipt=light required=light runtime_lines=80 class=code'); }); }, 120_000); }); @@ -161,7 +161,7 @@ describe('gstack-review-plan', () => { const depth = (out: string) => /^REVIEW_DEPTH: .*$/m.exec(out)?.[0]; const real = bin('gstack-review-plan', ['--base', 'main'], fx.clone); expect(real.status, real.err).toBe(0); - expect(depth(real.out)).toContain('REVIEW_DEPTH: standard runtime_lines=80'); + expect(depth(real.out)).toContain('REVIEW_DEPTH: light runtime_lines=80'); const control = bin('gstack-review-plan', ['--base', 'main'], fx.control); expect(depth(control.out)).toBe(depth(real.out)); git(fx.control, 'checkout', '-q', 'main'); diff --git a/test/review-dispatch.test.ts b/test/review-dispatch.test.ts index fe05dbc61..b92798558 100644 --- a/test/review-dispatch.test.ts +++ b/test/review-dispatch.test.ts @@ -782,16 +782,18 @@ describe('reading the REVIEWED_TREE line (GST-482 review fixes)', () => { test('the fingerprint step opens the brief and its command prints a line collect accepts; GST-850 gpt-6.1-sol replies stay refused (GST-897)', () => { const out = tmp(); const ledger = path.join(out, 'review-ledger.jsonl'); - const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: ROOT, encoding: 'utf8', timeout: 30_000 }).stdout.trim(); - const file = field(run(['render', '--role', 'outside-voice', '--out', out, '--diff-base', head]).stdout, 'file'); + const wt = cleanCheckout(); + const wtTop = fs.realpathSync(wt); + const head = spawnSync('git', ['rev-parse', 'HEAD'], { cwd: wt, encoding: 'utf8', timeout: 30_000 }).stdout.trim(); + const file = field(run(['render', '--role', 'outside-voice', '--out', out, '--diff-base', head], wt).stdout, 'file'); const brief = fs.readFileSync(file, 'utf8'); const first = brief.split('\n')[0]!; - expect(first).toStartWith(`First step, before anything else: run \`cd '${top}' && echo "REVIEWED_TREE: `); + expect(first).toStartWith(`First step, before anything else: run \`cd '${wtTop}' && echo "REVIEWED_TREE: `); expect(first).toContain('Copy that output line, not the command, as the first line of your final reply'); expect(brief.slice(brief.lastIndexOf('\n\n') + 2)).toContain('(the output, not the command)'); const command = /run `([^`]+)`/.exec(first)![1]!; const printed = spawnSync('bash', ['-c', command], { cwd: os.tmpdir(), encoding: 'utf8', timeout: 30_000 }).stdout.trim(); - expect(checkReviewedTree(printed + '\nNO FINDINGS', expected).ok).toBe(true); + expect(checkReviewedTree(printed + '\nNO FINDINGS', `${tree} ${wtTop}`).ok).toBe(true); run(['sent', '--ledger', ledger, '--role', 'outside-voice', '--sent', file]); for (const p of ['p1', 'p2']) { const reply = path.join(ROOT, `test/fixtures/review-wrong-tree/gpt-6.1-sol-gst850-${p}.out`); diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index b3cce7010..34e046931 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -123,6 +123,21 @@ describe('what review_exclude_paths excludes and keeps', () => { for (const b of briefs) expect(b).toContain(`(selection ${m.review_exclude.digest})`); }, 180_000); + // Value: protects=Step 11's standalone targeted render cannot record a brief with no selection beside the pass's briefs; + // fails_when=render --role targeted without --disclosure writes a rendered event with no exclude digest, so verify sees + // mixed digests and never prints VERIFY: clean; why_new=/ship review of the GST-508 wave (Red Team); seam=gstack-review-dispatch render + test('a standalone targeted render under an active exclusion refuses and leaves the ledger as the pass wrote it', () => { + const repo = census(); + const out = path.join(repo.root, 'pass'); + expect(runBin(repo, 'gstack-review-dispatch', ['pass', '--base', 'main', '--out', out, '--pass', 'p1', '--', '--gate-tagged', '0']).status).toBe(EXIT.ok); + const ledger = path.join(out, 'review-ledger.jsonl'); + const before = fs.readFileSync(ledger, 'utf8'); + const r = runBin(repo, 'gstack-review-dispatch', ['render', '--role', 'targeted', '--classes', 'money', '--base', 'main', '--diff-base', mergeBase(repo), '--out', out]); + expect(r.status).not.toBe(EXIT.ok); + expect(r.err).toContain("review_exclude_paths is active, so the targeted reader renders in the pass manifest's voices stage"); + expect(fs.readFileSync(ledger, 'utf8')).toBe(before); + }, 120_000); + // Value: protects=excluded receipts cannot dispatch the outside voice or the targeted money reader on lines no reader sees, // and a credential line inside a matched file keeps it; fails_when=scanNumbers or targetedClasses read excluded lines, or // a secret-bearing data file is excluded; why_new=task 9; seam=bin/gstack-review-plan; no_claim=pregate's credential scan diff --git a/test/ship-queue-land.test.ts b/test/ship-queue-land.test.ts index 806a0259f..e1ed4fea4 100644 --- a/test/ship-queue-land.test.ts +++ b/test/ship-queue-land.test.ts @@ -14,8 +14,8 @@ import { EXIT } from '../lib/headless-artifacts'; import { BASE_POLICY, cleanup, makeFixtureRepo, readFile, runBin, type FixtureRepo } from './helpers/restamp-fixture'; import { shippedBody } from './helpers/ship-queue-train-fixture'; import { resolveReleaseConflicts } from '../lib/ship-queue-land'; -import { receiptDepthAtHead } from '../lib/ship-receipt-depth'; -import { mergePolicy } from '../lib/ship-policy'; +import { receiptDepthAtHead, receiptExclude, receiptReviewAtHead } from '../lib/ship-receipt-depth'; +import { mergePolicy, REVIEW_EXCLUDE_KEY_VERSION } from '../lib/ship-policy'; import { persist, repoIdentity } from '../lib/ship-queue-train-record'; const ROOT = path.resolve(import.meta.dir, '..'); @@ -583,6 +583,26 @@ describe('land: one pass, base, lock and history', () => { expect(again.out).toMatch(/^REVIEW_DEPTH: receipt=light required=light /m); expect(again.out).toContain('LANDED: '); }, 90_000); + + // Value: protects=the same merged resume when review_exclude_paths excluded files from the PR's range; fails_when=the queue + // recomputes the exclusion over the merged (empty) range and refuses the receipt's digest as RECEIPT_INVALID; why_new=the + // case above has no review_exclude_paths; seam=none; no_claim=GitHub's merge-commit shape beyond two parents + test('merge-only: a merged resume lands when review_exclude_paths excluded files from the range', () => { + const fx = setup({ ...POLICY, history: 'merge-only', review_exclude_paths: ['data/**'], min_gstack_version: REVIEW_EXCLUDE_KEY_VERSION }); + const head = openPr(fx, '15', { edit: work => { fs.mkdirSync(path.join(work, 'data', 'a'), { recursive: true }); fs.writeFileSync(path.join(work, 'data', 'a', 'result.json'), '{"ok": true}\n'); } }); + fx.repo.git('checkout', '-q', '--detach', head); + const { depth, exclude } = receiptReviewAtHead(fx.repo.work, 'main', path.join(ROOT, 'bin'), head); + fx.repo.git('checkout', '-q', '--detach', 'origin/main'); + const recorded = receiptExclude(exclude); + expect(recorded?.excluded).toBe(1); + fx.write(d => { d.prs['15'].body = shippedBody('15', head, { gstack_version: REVIEW_EXCLUDE_KEY_VERSION, review_depth: depth, review_exclude: recorded }); }); + land(fx, ['--pr', '15']); + fx.write(d => { d.trueMerge = true; d.mergeErrorAfter = true; }); + expect(land(fx, ['--pr', '15']).err).toContain('HTTP 502'); + const again = land(fx, ['--pr', '15']); + expect(again.out + again.err).not.toMatch(/RECEIPT_INVALID|BASE_CONTAINS_HEAD|QUEUE_RECEIPT_DEPTH_RAISED/); + expect(again.out).toContain('LANDED: '); + }, 90_000); }); // /ship adversarial (Claude) and outside-voice (Codex) findings (GSTA-154 PR A): each case failed or was unreachable before its fix.