From 82057955d835411afc2cb4d79e59c119b3bb2fe0 Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 18:31:25 +0000 Subject: [PATCH] fix(ship-receipt): carry across a base merge whose path listing passes 1 MiB (GST-552) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-receipt | 32 ++++++++---- .../gst-552-receipt-carry-maxbuffer.md | 5 ++ lib/ship-receipt-depth.ts | 18 ++++++- lib/tree-receipt.ts | 8 +-- test/ship-receipt-carry-large-merge.test.ts | 52 +++++++++++++++++++ 5 files changed, 100 insertions(+), 15 deletions(-) create mode 100644 changelog.d/gst-552-receipt-carry-maxbuffer.md create mode 100644 test/ship-receipt-carry-large-merge.test.ts diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 3abb2714a..86ac0345e 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -45,7 +45,7 @@ import { receiptIssuesFrom, receiptSummaryLine, renderCensus, renderReceiptBlock, validateReceipt, type CensusPr, } from '../lib/ship-receipt'; import { checkDocsCaps, docsCapLine, docsExceptionLine, parseReceiptDocs } from '../lib/ship-receipt-docs'; -import { deeper, depthBelow, exceptionProblems, parseReviewLoopException, parseReviewTargeted, receiptDepthAtHead, reviewLoopExceptionLine } from '../lib/ship-receipt-depth'; +import { deeper, depthBelow, exceptionProblems, GitRunError, parseReviewLoopException, parseReviewTargeted, receiptDepthAtHead, receiptGit, reviewLoopExceptionLine } from '../lib/ship-receipt-depth'; import { checkReceiptProse, proseLine } from '../lib/ship-receipt-prose'; import { resolveStateRoot } from '../lib/state-root'; import { readPause, receiptBaseRed } from '../lib/base-red'; @@ -118,8 +118,8 @@ function jsonArg(raw: string | undefined): Record | undefined { return parsed; } function git(args: string[], cwd?: string): string | undefined { - const r = spawnSync('git', args, { cwd, encoding: 'utf8', timeout: 30_000 }); - return r.status === 0 ? r.stdout.trim() : undefined; + const r = receiptGit(cwd, args, 30_000); + return r.ok ? r.out.trim() : undefined; } function gh(args: string[]): { ok: boolean; out: string; err: string } { const r = spawnSync('gh', args, { encoding: 'utf8', timeout: 120_000, maxBuffer: 64 * 1024 * 1024 }); @@ -210,7 +210,7 @@ function cmdWrite(rest: string[], json: boolean): number { let loop: unknown; try { loop = jsonArg(flag(rest, '--review-loop')); } catch {} if (!parseReviewLoop(loop)?.fix_checks) return refuse('--review-loop-exception needs --review-loop with fix_checks >= 1: the stop it accepts follows a fix-check'); const fc = loopException.exception.fix_check.head; - if (spawnSync('git', ['merge-base', '--is-ancestor', fc, head], { cwd: repoRoot }).status !== 0) return refuse(`fix_check.head ${fc.slice(0, 12)} is not ${head.slice(0, 12)} or an ancestor of it`); + if (!receiptGit(repoRoot, ['merge-base', '--is-ancestor', fc, head], 30_000).ok) return refuse(`fix_check.head ${fc.slice(0, 12)} is not ${head.slice(0, 12)} or an ancestor of it`); const line = reviewLoopExceptionLine(loopException.exception); if (bodyFile && !bodyText.split(/\r?\n/).some(l => l.trim() === line)) return refuse(`${bodyFile} does not carry the exception line verbatim: ${JSON.stringify(line)}`); } @@ -399,10 +399,10 @@ 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 diff = spawnSync('git', ['diff', '--no-color', '--no-ext-diff', '--full-index', from, to, '--', ...pathspec], { cwd: repoRoot, encoding: 'utf8', timeout: 120_000, maxBuffer: 256 * 1024 * 1024 }); - if (diff.status !== 0) return { error: `git diff ${from.slice(0, 12)} ${to.slice(0, 12)}: ${firstLine(diff.stderr, diff.signal ? `killed by ${diff.signal}` : `exit ${diff.status}`)}` }; + if (diff.status !== 0) return { error: `git diff ${from.slice(0, 12)} ${to.slice(0, 12)}: ${firstLine(diff.error?.message ?? diff.stderr, diff.signal ? `killed by ${diff.signal}` : `exit ${diff.status}`)}` }; if (!diff.stdout.trim()) return ''; const id = spawnSync('git', ['patch-id', '--verbatim'], { cwd: repoRoot, input: diff.stdout, encoding: 'utf8', timeout: 120_000, maxBuffer: 64 * 1024 * 1024 }); - if (id.status !== 0) return { error: `git patch-id --verbatim: ${firstLine(id.stderr, id.signal ? `killed by ${id.signal}` : `exit ${id.status}`)}` }; + if (id.status !== 0) return { error: `git patch-id --verbatim: ${firstLine(id.error?.message ?? id.stderr, id.signal ? `killed by ${id.signal}` : `exit ${id.status}`)}` }; return id.stdout.trim().split(/\s+/)[0] ?? ''; } @@ -463,7 +463,7 @@ function productRules(repoRoot: string, base: string, outputs: readonly string[] function productSpec(repoRoot: string, rules: ProductRules, from: string, to: string): { changed: Array<{ path: string; class: ReturnType }>; product: string[]; tests: string[] } | undefined { const changedRaw = git(['diff', '--name-only', from, to], repoRoot); if (changedRaw === undefined) return undefined; - const blobAt = (ref: string, p: string): string | null => { const r = spawnSync('git', ['show', `${ref}:${p}`], { cwd: repoRoot, encoding: 'utf8', timeout: 30_000, maxBuffer: 16 * 1024 * 1024 }); return r.status === 0 ? r.stdout : null; }; + const blobAt = (ref: string, p: string): string | null => { const r = receiptGit(repoRoot, ['show', `${ref}:${p}`], 30_000); return r.ok ? r.out : null; }; const versionAt = (ref: string): string | null => { const file = blobAt(ref, 'VERSION'); if (file !== null) return file.trim() || null; @@ -554,7 +554,7 @@ function cmdCarry(rest: string[], json: boolean): number { const dirty = status.stdout.split('\n').filter(Boolean).map(l => l.slice(3).replace(/^.* -> /, '')); if (dirty.length) return refused('RECEIPT_CARRY_REFUSED', `the working tree has uncommitted changes (${dirty.slice(0, 5).join(', ')}${dirty.length > 5 ? ` +${dirty.length - 5} more` : ''}); commit them and carry at the new head, or run /ship`); const fromKnown = git(['cat-file', '-e', `${from}^{commit}`], repoRoot) !== undefined; - const ancestor = fromKnown && spawnSync('git', ['merge-base', '--is-ancestor', from, head], { cwd: repoRoot, encoding: 'utf8', timeout: 30_000 }).status === 0; + const ancestor = fromKnown && receiptGit(repoRoot, ['merge-base', '--is-ancestor', from, head], 30_000).ok; const fromTree = fromKnown ? git(['rev-parse', `${from}^{tree}`], repoRoot) : undefined; const covered = [...(receipt.head_tree ? [receipt.head_tree] : fromTree ? [fromTree] : []), ...(receipt.carry?.trees ?? [])]; if (!ancestor && !covered.length) return refused('RECEIPT_CARRY_REFUSED', `the receipt's head ${from.slice(0, 12)} is not an ancestor of HEAD ${head.slice(0, 12)} (rebased or force-pushed), and the receipt records no tree to compare; merge the base in next time, and run /ship now`); @@ -636,7 +636,7 @@ function cmdCarry(rest: string[], json: boolean): number { return refused('RECEIPT_CARRY_PREGATE_STALE', `the receipt records a pregate for ${receipt.head.slice(0, 12)}; run gstack-pregate at HEAD ${head.slice(0, 12)} and pass --pregate `); } let depthField: Partial = {}; - const isAncestor = (commit: string) => !ancestor || spawnSync('git', ['merge-base', '--is-ancestor', commit, head], { cwd: repoRoot, timeout: 30_000 }).status === 0; + const isAncestor = (commit: string) => !ancestor || receiptGit(repoRoot, ['merge-base', '--is-ancestor', commit, head], 30_000).ok; const problems = exceptionProblems(body, receipt, isAncestor); if (problems.length) return refused('RECEIPT_CARRY_REFUSED', `the receipt's owner exception does not stand: ${problems.join('; ')}; run /ship on ${head.slice(0, 12)}`); if (receipt.review_depth || receipt.gstack_version) { @@ -750,4 +750,16 @@ function main(argv: string[]): number { return EXIT.usage; } -process.exitCode = main(process.argv.slice(2)); +/** A git answer the receipt could not read (output past the buffer, a timeout) refuses by name instead of reading as "no" (GST-552). */ +function mainRefusingGitErrors(argv: string[]): number { + try { return main(argv); } catch (e) { + if (!(e instanceof GitRunError)) throw e; + const [cmd, ...rest] = argv; + const code = cmd === 'carry' ? 'RECEIPT_CARRY_REFUSED' : 'RECEIPT_INVALID'; + process.stderr.write(resultLineWithDocs(`gstack-ship-receipt ${cmd}`, code, e.message) + '\n'); + if (cmd === 'carry') process.stdout.write(rest.includes('--json') ? JSON.stringify({ skill: 'ship', status: 'refused', run: '-', code, message: e.message }) + '\n' : `CARRY: refused (${code})\nGSTACK_RESULT: skill=ship status=refused run=-\n`); + return EXIT.refused; + } +} + +process.exitCode = mainRefusingGitErrors(process.argv.slice(2)); diff --git a/changelog.d/gst-552-receipt-carry-maxbuffer.md b/changelog.d/gst-552-receipt-carry-maxbuffer.md new file mode 100644 index 000000000..731ab82d6 --- /dev/null +++ b/changelog.d/gst-552-receipt-carry-maxbuffer.md @@ -0,0 +1,5 @@ +**`gstack-ship-receipt carry` works again across a base merge that adds thousands of files.** After harness-evals#147 merged a base that added about 8,300 files, every carry across that merge was refused with "git diff … failed". The carry lists the paths that changed between the reviewed head and HEAD, and that list was 1.37 MB. The git calls ran with the default 1 MiB output limit, so git was stopped mid-listing (ENOBUFS) and the carry read it as a failed diff. + +### Fixed +- **`bin/gstack-ship-receipt`, `lib/ship-receipt-depth.ts`**: the receipt's git calls go through one helper, `receiptGit`, with a 256 MiB output limit (the same one `lib/review-collect.ts` uses). When git's output still passes that limit or the call times out, the command now refuses by name (`RECEIPT_CARRY_REFUSED` for carry) and says the buffer was exceeded, instead of treating the missing answer as "no". The `--is-ancestor` check in `write --review-loop-exception` now has a 30-second timeout. +- **`lib/tree-receipt.ts`**: the gated-to-current diff runs with a 256 MiB limit, and a failed diff is an `EVIDENCE_IDENTITY_UNKNOWN` error instead of an empty diff that classified no changed files. diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 15d4bdbbf..e863d9f74 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -35,9 +35,23 @@ const sha = (v: unknown): v is string => typeof v === 'string' && /^[0-9a-f]{40} const BIN_DIR = path.join(import.meta.dir, '..', 'bin'); const short = (s: string | null | undefined) => (s ? s.slice(0, 12) : 'none'); +/** The receipt's git output cap: a base merge can list thousands of paths (harness-evals#147: 1.37 MB of names), past spawnSync's 1 MiB default. */ +export const GIT_MAX_BUFFER = 256 * 1024 * 1024; +/** A git run whose answer could not be read (output past the buffer, a timeout): a named refusal, never a quiet "no" (GST-552). */ +export class GitRunError extends Error {} +/** git for the receipt: `ok` is git's own verdict; a spawn error (ENOBUFS, a timeout) throws GitRunError. */ +export function receiptGit(cwd: string | undefined, args: string[], timeout = 60_000, maxBuffer = GIT_MAX_BUFFER): { ok: boolean; out: string } { + const r = spawnSync('git', args, { cwd, encoding: 'utf8', timeout, maxBuffer }); + if (r.error || (r.status === null && r.signal)) { + const code = (r.error as NodeJS.ErrnoException | undefined)?.code; + const why = code === 'ENOBUFS' ? `its output exceeded the ${maxBuffer / (1024 * 1024)} MiB buffer` : code === 'ETIMEDOUT' || r.signal ? `it was stopped after ${timeout / 1000} s (${r.error?.message ?? r.signal})` : r.error!.message; + throw new GitRunError(`git ${args.slice(0, 2).join(' ')} could not be read: ${why}`); + } + return { ok: r.status === 0, out: r.stdout ?? '' }; +} function git(root: string, args: string[]): { ok: boolean; out: string } { - const r = spawnSync('git', args, { cwd: root, encoding: 'utf8', timeout: 60_000 }); - return { ok: r.status === 0, out: (r.stdout ?? '').trim() }; + const r = receiptGit(root, args); + return { ok: r.ok, out: r.out.trim() }; } /** gstack-diff-scope on the committed range `base`...`head` (`--head`): the same signals at write, carry and the queue. */ diff --git a/lib/tree-receipt.ts b/lib/tree-receipt.ts index de94d0004..b4ba02da4 100644 --- a/lib/tree-receipt.ts +++ b/lib/tree-receipt.ts @@ -42,7 +42,7 @@ export interface TreeReceipt { } function git(cwd: string, args: string[]): string | null { - const r = spawnSync('git', args, { cwd, encoding: 'utf8', timeout: 60_000, maxBuffer: 64 * 1024 * 1024 }); + const r = spawnSync('git', args, { cwd, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024 }); return r.status === 0 ? (r.stdout ?? '').replace(/\n$/, '') : null; } function blob(cwd: string, treeish: string, rel: string): string | null { @@ -170,11 +170,13 @@ 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]) ?? ''; + 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 (past the 256 MiB buffer, a timeout, or a missing object), so the change between the gated and current trees cannot be classified`, code: 'EVIDENCE_IDENTITY_UNKNOWN' }; 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); - const stampDiff = stampFiles.length ? (git(o.repoRoot, ['diff', '--no-color', '--no-ext-diff', gatedHead, currentHead, '--', ...stampFiles]) ?? '') : ''; + const stampDiff = stampFiles.length ? git(o.repoRoot, ['diff', '--no-color', '--no-ext-diff', gatedHead, currentHead, '--', ...stampFiles]) : ''; + if (stampDiff === null) return { error: `git diff of the stamp files ${gatedHead.slice(0, 12)} ${currentHead.slice(0, 12)} failed, so the stamp cannot be shown`, code: 'EVIDENCE_IDENTITY_UNKNOWN' }; const predecessor = resolvePredecessor(o); const current = executionIdentity(o.repoRoot, currentHead, o.env); diff --git a/test/ship-receipt-carry-large-merge.test.ts b/test/ship-receipt-carry-large-merge.test.ts new file mode 100644 index 000000000..8f8a84733 --- /dev/null +++ b/test/ship-receipt-carry-large-merge.test.ts @@ -0,0 +1,52 @@ +/** + * gstack-ship-receipt carry across a base merge whose path listing passes 1 MiB (GST-552, harness-evals#147: + * about 8,300 added files, a 1.37 MB `git diff --name-only`). spawnSync's default 1 MiB maxBuffer made that + * listing fail with ENOBUFS, so every carry across the merge was refused as "git diff ... failed". + */ +import { afterEach, describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import { EXIT } from '../lib/headless-artifacts'; +import { extractReceipt } from '../lib/ship-receipt'; +import { GitRunError, receiptGit } from '../lib/ship-receipt-depth'; +import { cleanup, commit, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/pregate-fixture'; + +const repos: FixtureRepo[] = []; +afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); + +/** 4,600 empty files under long paths: about 1.15 MB of `git diff --name-only`, one shared blob. */ +function addManyFiles(work: string): void { + const dir = path.join(work, 'vendor', 'x'.repeat(200)); + fs.mkdirSync(dir, { recursive: true }); + for (let i = 0; i < 4600; i++) fs.writeFileSync(path.join(dir, `${'f'.repeat(30)}-${i}.txt`), ''); +} + +describe('carry across a base merge with a listing over 1 MiB (GST-552)', () => { + // Value: protects=a receipt carries across a huge base merge (harness-evals#147 refused every carry with ENOBUFS); + // fails_when=the carry's git listings run with the 1 MiB default buffer; seam=none; no_claim=gh pr edit (--body stands in) + test('the product-unchanged branch carries, and a listing past the buffer is a named error, never a false', () => { + const repo = makeFixtureRepo(); repos.push(repo); + const reviewed = commit(repo, { 'lib/util.js': "exports.add = (a, b) => a + b + 0;\n" }); + const body = path.join(repo.root, 'body.md'); + const w = runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '7', '--base', 'main', '--version', '1.0.0', '--session-kind', 'unattended', '--body', body]); + expect(w.status).toBe(EXIT.ok); + repo.git('checkout', '-q', 'main'); + addManyFiles(repo.work); + repo.git('add', '-A'); repo.git('commit', '-qm', 'main: vendor drop'); repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-q', 'feature'); + repo.git('merge', '-q', '--no-edit', 'origin/main'); + const head = repo.git('rev-parse', 'HEAD'); + const listing = spawnSync('git', ['diff', '--name-only', reviewed, head], { cwd: repo.work, env: { ...process.env, ...repo.env }, encoding: 'utf8', timeout: 30_000, maxBuffer: 64 * 1024 * 1024 }); + expect(listing.stdout.length).toBeGreaterThan(1024 * 1024); + expect(() => receiptGit(repo.work, ['diff', '--name-only', reviewed, head], 30_000, 1024 * 1024)).toThrow(GitRunError); + expect(() => receiptGit(repo.work, ['diff', '--name-only', reviewed, head], 30_000, 1024 * 1024)).toThrow('git diff --name-only could not be read: its output exceeded the 1 MiB buffer'); + expect(receiptGit(repo.work, ['merge-base', '--is-ancestor', head, reviewed]).ok).toBe(false); + + const out = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']); + expect(out.err).not.toContain('failed'); + expect(out.status).toBe(EXIT.ok); + expect(out.out).toContain(`CARRY: carried from ${reviewed.slice(0, 12)} to ${head.slice(0, 12)}`); + expect(extractReceipt(fs.readFileSync(body, 'utf8')).receipt!.head).toBe(head); + }, 90_000); +});