From c56c0068df0c94731550c33219bfc4d07917a1fb Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 12:22:13 +0000 Subject: [PATCH] fix: /ship review pass 1: notes-only floor, scoped carry order, named temp and diff refusals - A notes-only pass is recorded clean only when every finding is fixed, or noted or deferred below the blocking floor (never CRITICAL or blocking), by its logger-stamped action, and the findings account for the claimed issue and critical counts (GST-430). - The preflight scoped carry route commits and pushes the base-merge fix before writing the packet (GST-442). - patchId refuses by name on an unwritable TMPDIR; the tree receipt names why its diff failed (GST-448). - Step 16 stage 3 spells out --docs-requested with its --docs-result (GST-444). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-receipt | 3 ++- lib/headless-artifacts.ts | 8 +++++++- lib/review-evidence.ts | 11 +++++++---- lib/ship-preflight.ts | 2 +- lib/tree-receipt.ts | 8 ++++++-- ship/SKILL.md | 2 +- ship/SKILL.md.tmpl | 2 +- test/fixtures/golden/agy-ship-SKILL.md | 2 +- test/fixtures/golden/claude-ship-SKILL.md | 2 +- test/fixtures/golden/codex-ship-SKILL.md | 2 +- test/fixtures/golden/factory-ship-SKILL.md | 2 +- test/review-notes-only-pass.test.ts | 4 ++++ test/ship-receipt-carry-large-diff.test.ts | 17 +++++++++++++++++ test/ship-rereview-triggers.test.ts | 2 +- .../ship/gst-430-notes-only-pass.json | 4 ++-- test/train-gate.test.ts | 17 +++++++++++++++++ 16 files changed, 70 insertions(+), 18 deletions(-) diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 87f941121..3447320e4 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -391,7 +391,8 @@ function writeRunRecord(a: { receipt: ShipReceipt; head: string; pr: string | un */ function patchId(repoRoot: string, from: string, to: string, pathspec: string[]): string | { error: string } { const firstLine = (s: string | null | undefined, fallback: string) => (s ?? '').trim().split('\n')[0]?.slice(0, 200) || fallback; - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-patch-id-')); + let dir: string; + try { dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-patch-id-')); } catch (e) { return { error: `cannot create a temp file under ${os.tmpdir()} (${firstLine((e as Error).message, 'mkdtemp failed')}); set TMPDIR to a writable directory` }; } const file = path.join(dir, 'diff.patch'); let fd: number | undefined; try { diff --git a/lib/headless-artifacts.ts b/lib/headless-artifacts.ts index d045bab9e..a0e5ae46d 100644 --- a/lib/headless-artifacts.ts +++ b/lib/headless-artifacts.ts @@ -548,7 +548,13 @@ export interface DerivedReviewStatus { export function isOpenFinding(row: Record): boolean { if (typeof row.disposition === 'string') return !RESOLVED.includes(row.disposition); if (typeof row.status === 'string') return !['resolved', 'fixed', 'closed'].includes(row.status); - return !['fixed', 'auto-fixed', 'deferred', 'noted'].includes(row.action as string); + return isOpenByAction(row); +} +/** A finding's `action` alone: fixed and auto-fixed resolve it; deferred and noted resolve only a non-blocking, non-CRITICAL one (review policy's floor). */ +export function isOpenByAction(row: Record): boolean { + if (row.action === 'fixed' || row.action === 'auto-fixed') return false; + if (row.action === 'deferred' || row.action === 'noted') return row.blocking === true || isCriticalFinding(row); + return true; } export function isCriticalFinding(row: Record): boolean { return typeof row.severity === 'string' && row.severity.toLowerCase() === 'critical'; diff --git a/lib/review-evidence.ts b/lib/review-evidence.ts index ce8dbd13a..dae8ccc7d 100644 --- a/lib/review-evidence.ts +++ b/lib/review-evidence.ts @@ -2,7 +2,7 @@ import { createHash } from 'node:crypto'; import { spawnSync } from 'node:child_process'; import { closeSync, constants, fstatSync, lstatSync, mkdirSync, openSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; -import { deriveReviewStatus, isOpenFinding, readJsonl } from './headless-artifacts'; +import { deriveReviewStatus, isCriticalFinding, isOpenByAction, readJsonl } from './headless-artifacts'; import { resultLine, type ResultCodeName } from './result-codes'; const DIFF_REVIEWS = new Set(['review', 'review-fix-check', 'adversarial-review', 'codex-review', 'design-review-lite', 'ship']); @@ -271,10 +271,13 @@ export function bindReview(rec: Record, token: string, env = proces if (env.GSTACK_STAMP_TREE) rec.tree = env.GSTACK_STAMP_TREE; if (env.GSTACK_STAMP_DIRTY) rec.dirty = env.GSTACK_STAMP_DIRTY === 'true'; stampGateFindings(rec.findings, env.GSTACK_STAMP_SESSION_KIND); - // A /ship pass whose findings were all fixed, deferred or noted left no open defect (review-army item 6), so a - // claimed issues_found is recorded clean with zero counts and Step 11.5 can bind it (GST-430). + // A /ship pass whose findings were all fixed, or deferred or noted below the floor, left no open defect (review-army + // item 6), so a claimed issues_found is recorded clean with zero counts and Step 11.5 can bind it (GST-430). Only the + // logger-stamped action counts (never a caller's status), and the findings must account for every claimed issue. if (!findingsPath && (rec.skill === 'review' || rec.skill === 'review-fix-check') && rec.status === 'issues_found' && rec.completed === true && - Array.isArray(rec.findings) && rec.findings.length > 0 && rec.findings.every(f => record(f) && typeof f.action === 'string' && !isOpenFinding(f))) { + Array.isArray(rec.findings) && rec.findings.length > 0 && rec.findings.length >= Number(rec.issues_found ?? 0) && + rec.findings.filter(f => record(f) && isCriticalFinding(f)).length >= Number(rec.critical ?? 0) && + rec.findings.every(f => record(f) && typeof f.action === 'string' && !isOpenByAction(f))) { Object.assign(rec, { status: 'clean', issues_found: 0, critical: 0, informational: 0 }); } if (!DIFF_REVIEWS.has(rec.skill)) { diff --git a/lib/ship-preflight.ts b/lib/ship-preflight.ts index 604a19345..d87924ea5 100644 --- a/lib/ship-preflight.ts +++ b/lib/ship-preflight.ts @@ -51,7 +51,7 @@ function integrate(cwd: string, base: string, r: Report): boolean { return false; } if (line === 'CARRY: refused (RECEIPT_CARRY_SCOPED_REVIEW)') { - r.next(`carry route (Step 1.4): the base edited context next to reviewed hunks: gstack-ship-receipt carry --pr ${n} --base ${base} writes the packet; one fix-check reviewer reads it; fix what the base merge broke, gstack-pregate --allow-repo-commands, then carry again with --scoped-review --pregate .gstack/tmp/pregate.json; a blocking finding runs Steps 2-16`); + r.next(`carry route (Step 1.4): the base edited context next to reviewed hunks: fix what the base merge broke (CHANGELOG, size caps), commit and push; then gstack-ship-receipt carry --pr ${n} --base ${base} writes the packet for that head; one fix-check reviewer reads it; gstack-pregate --allow-repo-commands, then carry again with --scoped-review --pregate .gstack/tmp/pregate.json; a blocking finding runs Steps 2-16`); return false; } } else r.add('PR: none open'); diff --git a/lib/tree-receipt.ts b/lib/tree-receipt.ts index 6caf2ab0d..f419ace96 100644 --- a/lib/tree-receipt.ts +++ b/lib/tree-receipt.ts @@ -170,8 +170,12 @@ export function buildReceipt(o: ReceiptOptions): TreeReceipt | { error: string; const versionRel = o.versionRel ?? ('rel' in cv ? cv.rel : 'rel' in gv ? gv.rel : 'VERSION'); const oldV = 'version' in gv ? gv.version : null; const newV = 'version' in cv ? cv.version : null; - const diffText = git(o.repoRoot, ['diff', '--unified=0', '--no-color', '--no-ext-diff', gatedHead, currentHead]); - if (diffText === null) return { error: `git diff ${gatedHead.slice(0, 12)} ${currentHead.slice(0, 12)} failed, so the changed files cannot be classified (GST-448)`, code: 'EVIDENCE_IDENTITY_UNKNOWN' }; + const d = spawnSync('git', ['diff', '--unified=0', '--no-color', '--no-ext-diff', gatedHead, currentHead], { cwd: o.repoRoot, encoding: 'utf8', timeout: 120_000, maxBuffer: 256 * 1024 * 1024 }); + if (d.status !== 0) { + const why = (d.error as NodeJS.ErrnoException | undefined)?.code ?? (d.signal ? `killed by ${d.signal}` : (d.stderr ?? '').trim().split('\n')[0]?.slice(0, 200) || `exit ${d.status}`); + return { error: `git diff ${gatedHead.slice(0, 12)} ${currentHead.slice(0, 12)} failed (${why}), so the changed files cannot be classified (GST-448)`, code: 'EVIDENCE_IDENTITY_UNKNOWN' }; + } + const diffText = d.stdout.replace(/\n$/, ''); const files = classifyFiles(o.repoRoot, parseUnifiedDiff(diffText), gatedHead, currentHead, o.policy, versionRel, oldV, newV); const residual = files.filter(f => f.verdict === 'changed').map(f => f.path); const stampFiles = files.filter(f => f.verdict === 'stamp').map(f => f.path); diff --git a/ship/SKILL.md b/ship/SKILL.md index 70fa1d6a5..9bfbc08f2 100644 --- a/ship/SKILL.md +++ b/ship/SKILL.md @@ -719,7 +719,7 @@ audit or risk decision never qualifies. Report changed inputs, blockers and attempts used: -- **A fix the audit's result named:** rerun finish with `--docs-requested`. +- **A fix the audit's result named:** rerun finish with `--docs-requested --docs-result `. - **An attempt remains, with changed inputs or an available repair:** insert `14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count. Validate the outcome before Step 15, then restart Step 16 stage 1. diff --git a/ship/SKILL.md.tmpl b/ship/SKILL.md.tmpl index 8a98cf78a..6baea48df 100644 --- a/ship/SKILL.md.tmpl +++ b/ship/SKILL.md.tmpl @@ -440,7 +440,7 @@ audit or risk decision never qualifies. Report changed inputs, blockers and attempts used: -- **A fix the audit's result named:** rerun finish with `--docs-requested`. +- **A fix the audit's result named:** rerun finish with `--docs-requested --docs-result `. - **An attempt remains, with changed inputs or an available repair:** insert `14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count. Validate the outcome before Step 15, then restart Step 16 stage 1. diff --git a/test/fixtures/golden/agy-ship-SKILL.md b/test/fixtures/golden/agy-ship-SKILL.md index aea68e7a3..2f2398284 100644 --- a/test/fixtures/golden/agy-ship-SKILL.md +++ b/test/fixtures/golden/agy-ship-SKILL.md @@ -719,7 +719,7 @@ audit or risk decision never qualifies. Report changed inputs, blockers and attempts used: -- **A fix the audit's result named:** rerun finish with `--docs-requested`. +- **A fix the audit's result named:** rerun finish with `--docs-requested --docs-result `. - **An attempt remains, with changed inputs or an available repair:** insert `14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count. Validate the outcome before Step 15, then restart Step 16 stage 1. diff --git a/test/fixtures/golden/claude-ship-SKILL.md b/test/fixtures/golden/claude-ship-SKILL.md index 70fa1d6a5..9bfbc08f2 100644 --- a/test/fixtures/golden/claude-ship-SKILL.md +++ b/test/fixtures/golden/claude-ship-SKILL.md @@ -719,7 +719,7 @@ audit or risk decision never qualifies. Report changed inputs, blockers and attempts used: -- **A fix the audit's result named:** rerun finish with `--docs-requested`. +- **A fix the audit's result named:** rerun finish with `--docs-requested --docs-result `. - **An attempt remains, with changed inputs or an available repair:** insert `14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count. Validate the outcome before Step 15, then restart Step 16 stage 1. diff --git a/test/fixtures/golden/codex-ship-SKILL.md b/test/fixtures/golden/codex-ship-SKILL.md index 9c4a3e659..9bafcfc8b 100644 --- a/test/fixtures/golden/codex-ship-SKILL.md +++ b/test/fixtures/golden/codex-ship-SKILL.md @@ -721,7 +721,7 @@ audit or risk decision never qualifies. Report changed inputs, blockers and attempts used: -- **A fix the audit's result named:** rerun finish with `--docs-requested`. +- **A fix the audit's result named:** rerun finish with `--docs-requested --docs-result `. - **An attempt remains, with changed inputs or an available repair:** insert `14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count. Validate the outcome before Step 15, then restart Step 16 stage 1. diff --git a/test/fixtures/golden/factory-ship-SKILL.md b/test/fixtures/golden/factory-ship-SKILL.md index e693b7f92..338b3f1c9 100644 --- a/test/fixtures/golden/factory-ship-SKILL.md +++ b/test/fixtures/golden/factory-ship-SKILL.md @@ -723,7 +723,7 @@ audit or risk decision never qualifies. Report changed inputs, blockers and attempts used: -- **A fix the audit's result named:** rerun finish with `--docs-requested`. +- **A fix the audit's result named:** rerun finish with `--docs-requested --docs-result `. - **An attempt remains, with changed inputs or an available repair:** insert `14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count. Validate the outcome before Step 15, then restart Step 16 stage 1. diff --git a/test/review-notes-only-pass.test.ts b/test/review-notes-only-pass.test.ts index ca851e2ee..da4021bcb 100644 --- a/test/review-notes-only-pass.test.ts +++ b/test/review-notes-only-pass.test.ts @@ -57,6 +57,10 @@ describe('a notes-only /ship review pass (GST-430)', () => { ['gate open', pass([note(1, 'noted'), note(2, 'kept', { gate: 'RH1', gate_id: 'g', reason: 'r' })])], ['no action', pass([note(1, 'noted'), { fingerprint: 'lib/a.ts:9:testing', severity: 'CRITICAL' }])], ['incomplete', pass([note(1, 'noted')], { completed: false })], + ['caller status over a stamped-open gate', pass([note(1, 'noted'), note(2, 'kept', { gate: 'RH1', gate_id: 'g', reason: 'r', severity: 'CRITICAL', status: 'resolved' })])], + ['critical noted', pass([note(1, 'noted'), note(2, 'noted', { severity: 'CRITICAL' })])], + ['blocking deferred', pass([note(1, 'noted'), note(2, 'deferred', { blocking: true, issue: null })])], + ['counts beyond the findings', pass([note(1, 'noted')], { issues_found: 2, critical: 1 })], ]; for (const [name, row] of cases) { expect(run([row]).code).toBe(0); diff --git a/test/ship-receipt-carry-large-diff.test.ts b/test/ship-receipt-carry-large-diff.test.ts index 567a5ab68..ae134f9e1 100644 --- a/test/ship-receipt-carry-large-diff.test.ts +++ b/test/ship-receipt-carry-large-diff.test.ts @@ -50,4 +50,21 @@ describe('carry with a large product diff (GST-448)', () => { const out = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']); expect([out.status, out.out.split('\n').find(l => l.startsWith('CARRY:'))]).toEqual([EXIT.refused, 'CARRY: refused (RECEIPT_CARRY_PRODUCT_CHANGED)']); }); + + // Value: protects=an unwritable TMPDIR is a named refusal with its repair, never a stack trace that preflight reads as + // 'CARRY: check failed'; fails_when=patchId creates its temp file outside its error handling; seam=TMPDIR; no_claim=none + test('an unwritable TMPDIR refuses the carry by name', () => { + const repo = makeFixtureRepo(); repos.push(repo); + commit(repo, receipts(3, 'base'), 'results: 3 run receipts'); + const body = path.join(repo.root, 'body.md'); + expect(runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '9', '--base', 'main', '--version', '1.0.0', '--session-kind', 'unattended', '--body', body]).status).toBe(EXIT.ok); + repo.git('checkout', '-q', 'main'); + write(repo.work, 'docs/README.md', '# docs moved on main\n'); + repo.git('add', '-A'); repo.git('commit', '-qm', 'main moves on'); repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-q', 'feature'); + repo.git('merge', '-q', '--no-edit', 'origin/main'); + const out = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main'], { TMPDIR: path.join(repo.root, 'no-such-dir') }); + expect([out.status, out.out.split('\n').find(l => l.startsWith('CARRY:'))]).toEqual([EXIT.refused, 'CARRY: refused (RECEIPT_CARRY_REFUSED)']); + expect(out.err).toContain('set TMPDIR to a writable directory'); + }); }); diff --git a/test/ship-rereview-triggers.test.ts b/test/ship-rereview-triggers.test.ts index 4c213b9f7..f605caa92 100644 --- a/test/ship-rereview-triggers.test.ts +++ b/test/ship-rereview-triggers.test.ts @@ -501,7 +501,7 @@ describe('preflight carry route for an adjacent base edit (GST-442)', () => { f.gh(`case "$1 $2" in "pr list") echo 9 ;; "pr view") cat '${view}' ;; *) exit 1 ;; esac`); const r = f.run(PREFLIGHT, ['--base', 'main']); expect(lineOf(r.stdout, 'CARRY')).toBe('CARRY: refused (RECEIPT_CARRY_SCOPED_REVIEW)'); - expect(r.stdout).toMatch(/^NEXT: carry route \(Step 1\.4\): .*one fix-check reviewer.*--scoped-review /m); + expect(r.stdout).toMatch(/^NEXT: carry route \(Step 1\.4\): .*fix what the base merge broke.*commit and push.*writes the packet.*one fix-check reviewer.*--scoped-review /m); expect(fs.existsSync(path.join(f.work, '.git', 'gstack', `scoped-review-${head.slice(0, 12)}.md`))).toBe(false); const wrote = f.run(path.join(BIN, 'gstack-ship-receipt'), ['carry', '--pr', '9', '--base', 'main']); expect([wrote.status, fs.existsSync(path.join(f.work, '.git', 'gstack', `scoped-review-${head.slice(0, 12)}.md`))]).toEqual([EXIT.refused, true]); diff --git a/test/size-ledger/ship/gst-430-notes-only-pass.json b/test/size-ledger/ship/gst-430-notes-only-pass.json index 2268860d0..fd91f3258 100644 --- a/test/size-ledger/ship/gst-430-notes-only-pass.json +++ b/test/size-ledger/ship/gst-430-notes-only-pass.json @@ -1,4 +1,4 @@ { - "bytes": -22, - "why": "GST-442/GST-444: Step 1.4 follows a scoped-review NEXT; Step 16 stage 2 and 3 route finish scoped-review STOP and --docs-requested; trims in Steps 3, 12 and 16 keep the skill under its cap" + "bytes": 6, + "why": "GST-442/GST-444: Step 1.4 follows a scoped-review NEXT; Step 16 stage 2 and 3 route finish scoped-review STOP and --docs-requested with its --docs-result; trims in Steps 3, 12 and 16 keep the skill under its cap" } diff --git a/test/train-gate.test.ts b/test/train-gate.test.ts index 32757a46d..17702d1f0 100644 --- a/test/train-gate.test.ts +++ b/test/train-gate.test.ts @@ -224,6 +224,23 @@ describe('prefix trees modulo stamps', () => { expect(stampFreeTree(git, 'f'.repeat(40), repo.work, policy)).toHaveProperty('error'); }); + // Value: protects=a gated-to-current diff git cannot produce refuses with its cause instead of reading as 'no changes' + // (GST-448); fails_when=buildReceipt treats a failed diff as an empty one or drops why it failed; seam=a missing blob; no_claim=none + test('a diff git cannot produce refuses the receipt with EVIDENCE_IDENTITY_UNKNOWN and git\'s reason', () => { + const policy = loadPolicyFromHead(repo.work).policy; + repo.git('checkout', '-q', '-B', 'lost-blob', sha.t1!); + fs.writeFileSync(path.join(repo.work, 'lost.txt'), 'a blob this test deletes from the object store\n'); + repo.git('add', '-A'); repo.git('commit', '-q', '-m', 'add a blob'); + const head = repo.git('rev-parse', 'HEAD'); + const blobId = repo.git('rev-parse', `${head}:lost.txt`); + const gitDir = repo.git('rev-parse', '--absolute-git-dir'); + fs.rmSync(path.join(gitDir, 'objects', blobId.slice(0, 2), blobId.slice(2))); + const r = buildReceipt({ repoRoot: repo.work, gated: sha.t1!, current: head, policy, policyLabel: 'fixture' }) as { error?: string; code?: string }; + expect(r.code).toBe('EVIDENCE_IDENTITY_UNKNOWN'); + expect(r.error).toMatch(/^git diff [0-9a-f]{12} [0-9a-f]{12} failed \(.+\), so the changed files cannot be classified/); + repo.git('checkout', '-q', '-f', 'squash'); + }); + // #3512: a stamp path carrying a tentative `*_KEY_VERSION = ''` (gstack's own convention since #3511) digests the // same before and after the slot stamp moves it, so train-backstop finds the record that covers main; a base-version mention // on a line the base already carried digests the same at the prefix (where it equals VERSION) and on the stamped main.