diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 9107d62a9..c9d231caa 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -31,6 +31,7 @@ */ import { spawnSync } from 'node:child_process'; import * as fs from 'node:fs'; +import * as os from 'node:os'; import * as path from 'node:path'; import { EXIT, exitTableHelp, type ShipReceipt } from '../lib/headless-artifacts'; import { resultLine, resultLineWithDocs } from '../lib/result-codes'; @@ -116,7 +117,7 @@ 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 }); + const r = spawnSync('git', args, { cwd, encoding: 'utf8', timeout: 30_000, maxBuffer: 256 * 1024 * 1024 }); return r.status === 0 ? r.stdout.trim() : undefined; } function gh(args: string[]): { ok: boolean; out: string; err: string } { @@ -390,12 +391,20 @@ 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.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}`)}` }; - return id.stdout.trim().split(/\s+/)[0] ?? ''; + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-patch-id-')); + const file = path.join(dir, 'diff.patch'); + let fd: number | undefined; + try { + const diff = spawnSync('git', ['diff', '--no-color', '--no-ext-diff', '--full-index', `--output=${file}`, from, to, '--', ...pathspec], { cwd: repoRoot, encoding: 'utf8', timeout: 120_000 }); + 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}`)}` }; + fd = fs.openSync(file, 'r'); + const id = spawnSync('git', ['patch-id', '--verbatim'], { cwd: repoRoot, stdio: [fd, 'pipe', 'pipe'], encoding: 'utf8', timeout: 120_000 }); + if (id.status !== 0) return { error: `git patch-id --verbatim: ${firstLine(id.stderr, id.signal ? `killed by ${id.signal}` : `exit ${id.status}`)}` }; + return id.stdout.trim().split(/\s+/)[0] ?? ''; + } finally { + if (fd !== undefined) fs.closeSync(fd); + fs.rmSync(dir, { recursive: true, force: true }); + } } /** `git patch-id --verbatim` exists from git 2.39 (docs/ship-policy.md#receipt-carry); an older git is a named refusal, never a silent fallback to the whitespace-stripping default. */ @@ -597,13 +606,15 @@ function cmdCarry(rest: string[], json: boolean): number { else if (productFrom !== productHead && !retarget) { // planCarry names at most CARRY_NAMED_PATHS differing paths, so the walk stops there, and it reads only the paths the // branch itself touched against the base at either head: a base merge brings in every path the base changed, none of - // which can differ between the two base diffs. A path whose only difference is base context next to unchanged hunks + // which can differ between the two base diffs. A touched path with the same blob at both heads and at both bases has + // the same patch, so only paths that moved on one side are compared (GST-448: 6000 touched paths). A path whose only difference is base context next to unchanged hunks // (lib/ship-patch.ts) needs a scoped review instead of the loop. const sides = { reviewedBase: fromBase, reviewed: from, currentBase: headBase, current: head }; - const listed = [touchedPaths(repoRoot, fromBase, from), touchedPaths(repoRoot, headBase, head)]; - if (!listed[0] || !listed[1]) return refused('RECEIPT_CARRY_REFUSED', `git could not list the paths the branch touched against ${base}; run /ship on ${head.slice(0, 12)}`); - const touched = new Set([...listed[0], ...listed[1]]); - for (const p of [...touched].sort()) { + const listed = [touchedPaths(repoRoot, fromBase, from), touchedPaths(repoRoot, headBase, head), touchedPaths(repoRoot, from, head), touchedPaths(repoRoot, fromBase, headBase)]; + if (listed.some(l => !l)) return refused('RECEIPT_CARRY_REFUSED', `git could not list the paths the branch touched against ${base}; run /ship on ${head.slice(0, 12)}`); + const [touchedFrom, touchedHead, movedHead, movedBase] = listed as Set[]; + const touched = new Set([...touchedFrom, ...touchedHead]); + for (const p of [...touched].filter(p => movedHead.has(p) || movedBase.has(p)).sort()) { if (changedProduct.length >= CARRY_NAMED_PATHS) break; const cls = changed.find(c => c.path === p)?.class ?? classifyCarryPath(p, rules.extra); if (cls !== 'product') continue; diff --git a/changelog.d/gst-430-notes-only-pass.md b/changelog.d/gst-430-notes-only-pass.md index 82b586fc8..c7ad2ced2 100644 --- a/changelog.d/gst-430-notes-only-pass.md +++ b/changelog.d/gst-430-notes-only-pass.md @@ -1,9 +1,14 @@ **A /ship review pass that only left notes now binds at Step 11.5.** A pass whose findings were all fixed, deferred or noted has no open defect, but a driver could still log it as `issues_found`. Step 11.5 then refused it, and the only way out was an extra rebind pass (CAPY-2, qm#2837). When the tree changes during a pass, the refusal now names the start and end trees and the cheapest repair (GST-430). +**Receipt carry and write handle results PRs with thousands of files (GST-448).** A PR that adds about 8,300 run receipts (harness-evals#147) refused every carry with `git could not compute HEAD's product patch-id: git diff … failed`, and every base move cost it about 3 hours of /ship. The name-only listing ran past a 1 MB output buffer. + ### Fixed +- **`gstack-ship-receipt carry` and `write`** read git's path listings with a 256 MB buffer instead of the 1 MB default, so a large PR's `product_patch_id` is recorded and carried. The product patch-id streams `git diff` through a file into `git patch-id --verbatim` and no longer holds the diff in memory. The per-path walk compares only touched paths that changed at either head or either base, so a 6,000-path PR refuses or carries in seconds. +- **`gstack-tree-receipt`** fails closed (`EVIDENCE_IDENTITY_UNKNOWN`) when git cannot produce the diff between the gated and current heads. Before, it read a failed diff as no changes. - **`gstack-review-log`** records a completed `review` or `review-fix-check` row claimed as `issues_found` as `clean` with zero counts when every finding's action is `fixed`, `auto-fixed`, `deferred` or `noted`. A skipped finding, a gate finding left open, a finding with no action, or an incomplete pass keeps the claimed status. - **Findings files** (`--findings`) count `auto-fixed`, `deferred` and `noted` actions as resolved, the same as `fixed`. - **`review_freshness`** for a pass whose tree moved now reads `content changed during review: the tree moved from to while ran …`, followed by the repair: keep the checkout frozen until every reviewer returns, then rerun on the frozen tree (docs/troubleshooting.md#review-content-changed). ### Tests +- `test/ship-receipt-carry-large-diff.test.ts`: a PR that adds 6,000 receipts under a long directory name writes a receipt with its product patch-id and carries across a base merge. Negative control: one rescored receipt still refuses with `RECEIPT_CARRY_PRODUCT_CHANGED`. - `test/review-notes-only-pass.test.ts`: a pass with three notes is recorded clean and reads CURRENT. Negative controls: a skipped finding, an open gate finding, a finding with no action, and an incomplete pass keep `issues_found`. Also checks the reason a pass gets when its tree moved. diff --git a/lib/tree-receipt.ts b/lib/tree-receipt.ts index de94d0004..6caf2ab0d 100644 --- a/lib/tree-receipt.ts +++ b/lib/tree-receipt.ts @@ -170,7 +170,8 @@ 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, so the changed files cannot be classified (GST-448)`, 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); diff --git a/test/ship-receipt-carry-large-diff.test.ts b/test/ship-receipt-carry-large-diff.test.ts new file mode 100644 index 000000000..567a5ab68 --- /dev/null +++ b/test/ship-receipt-carry-large-diff.test.ts @@ -0,0 +1,53 @@ +/** + * gstack-ship-receipt carry on a PR whose own diff is large (GST-448, harness-evals#147): a results PR that adds thousands + * of run-receipt files has a name-only listing and a product diff over the child-process output buffer, so carry refused + * with "git could not compute HEAD's product patch-id: git diff … failed" after every base move. + */ +import { afterEach, describe, expect, setDefaultTimeout, test } from 'bun:test'; +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 { cleanup, commit, makeFixtureRepo, runBin, write, type FixtureRepo } from './helpers/pregate-fixture'; + +setDefaultTimeout(120_000); +const repos: FixtureRepo[] = []; +afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); +const DIR = `results/${'harness-eval-run-receipts-'.repeat(8)}`; +/** `count` run receipts under a long directory name: about 1.4 MB of paths and 2 MB of diff, past the 1 MB default buffer. */ +const receipts = (count: number, tag: string) => Object.fromEntries(Array.from({ length: count }, (_, i) => [`${DIR}/${tag}-run-${String(i).padStart(5, '0')}.json`, `{"run":${i},"tag":"${tag}","score":0.${i % 10}}\n`])); + +describe('carry with a large product diff (GST-448)', () => { + // Value: protects=a results PR keeps its receipt across a base move when its own diff is megabytes of added files + // (harness-evals#147 paid ~3h of /ship per base move); fails_when=carry buffers git's name-only listing or the diff in + // a bounded child-process buffer and refuses; seam=none; no_claim=gh pr edit (--body stands in) + test('a base merge carries the receipt of a PR that adds 6000 files', () => { + const repo = makeFixtureRepo(); repos.push(repo); + commit(repo, receipts(6000, 'base'), 'results: 6000 run receipts'); + const body = path.join(repo.root, 'body.md'); + const w = runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '9', '--base', 'main', '--version', '1.0.0', '--session-kind', 'unattended', '--body', body]); + expect(w.status).toBe(EXIT.ok); + const reviewed = extractReceipt(fs.readFileSync(body, 'utf8')).receipt!.product_patch_id; + expect(reviewed).toMatch(/^[0-9a-f]{40}$/); + 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']); + expect([out.status, out.out.split('\n').find(l => l.startsWith('CARRY:')), out.err.split('\n').find(l => l.includes('RECEIPT_CARRY'))]).toEqual([EXIT.ok, expect.stringMatching(/^CARRY: carried/), undefined]); + expect(extractReceipt(fs.readFileSync(body, 'utf8')).receipt!.product_patch_id).toBe(reviewed); + }); + + // Value: protects=negative control: the large-diff path still refuses when the PR's own receipts change after review; + // fails_when=the streamed patch-id reads as unchanged for a changed diff; seam=none; no_claim=none + test('one changed receipt in the large diff still refuses the carry', () => { + const repo = makeFixtureRepo(); repos.push(repo); + commit(repo, receipts(6000, 'base'), 'results: 6000 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); + commit(repo, { [`${DIR}/base-run-00042.json`]: '{"run":42,"tag":"base","score":0.9}\n' }, 'results: rescore run 42'); + 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)']); + }); +});