From 057376d24da78aa24b867b03c82c1042c2e6bedf Mon Sep 17 00:00:00 2001 From: garrytan Date: Fri, 6 Nov 2026 07:08:51 +0000 Subject: [PATCH] ship-receipt write: the targeted record survives a main merge; classes since the read cover the PR's own change (GST-1593) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-receipt | 2 +- lib/ship-receipt-depth.ts | 82 ++++++++++++++++++++++++----- test/review-targeted-record.test.ts | 76 +++++++++++++++++++++++++- 3 files changed, 146 insertions(+), 14 deletions(-) diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index f3dde839e..88a29648e 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -290,7 +290,7 @@ function cmdWrite(rest: string[], json: boolean): number { const product = reviewedProduct(repoRoot, base, head, pregateFile); let targeted: ReturnType | undefined; if (flag(rest, '--review-targeted') !== undefined) { - try { targeted = parseReviewTargeted(jsonArg(flag(rest, '--review-targeted')), product.product_patch_id ?? 'none', read => reviewedProduct(repoRoot, base, read, pregateFile).product_patch_id, read => targetedClassesSince(repoRoot, read, headSha ?? head), read => plainDocsSince(repoRoot, read, headSha ?? head)); } catch (e: any) { targeted = { ok: false, message: `--review-targeted: ${e.message}` }; } + try { targeted = parseReviewTargeted(jsonArg(flag(rest, '--review-targeted')), product.product_patch_id ?? 'none', read => reviewedProduct(repoRoot, base, read, pregateFile).product_patch_id, read => targetedClassesSince(repoRoot, read, headSha ?? head, base), read => plainDocsSince(repoRoot, read, headSha ?? head, base)); } catch (e: any) { targeted = { ok: false, message: `--review-targeted: ${e.message}` }; } } if (targeted && !targeted.ok) { process.stderr.write(resultLine('gstack-ship-receipt write', 'RECEIPT_INVALID', `${targeted.message}; nothing written`) + '\n'); return EXIT.fail; } const approval = approvalForReceipt({ repoRoot, base, head, citationText: [bodyText, ...flags(rest, '--approval')].join('\n'), runDirs: flags(rest, '--approval-run') }); diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 5e86b6d23..b9bb37230 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -479,16 +479,72 @@ export function bindShippedReceipt(root: string, baseRef: string, body: string, } /** - * The targeted classes of what changed from `from` to the checked-out `head`, with gstack-gate-diff run over that range whenever - * a non-docs path changed (a test edit that skips a case is gate class); null when `from` is not an ancestor of `head`, the gate - * scan did not run, or a class input is unreadable. + * A commit on `from` that holds what the PR's own change did after `from`: every path whose part of the PR's diff against its + * merge base with origin/ (product_patch_id's basis, with tests and metadata kept) differs between `from` and `head`, at + * `head`'s content. A main merge moves the merge base with the files it brings, so a file only main changed keeps `from`'s + * content (GST-1593); a branch edit, or a merge resolution that changes the PR's own hunks, moves the path. With no resolvable + * merge base every path that changed from `from` to `head` moves. Null when git cannot build it. */ -export function targetedClassesSince(root: string, from: string, head: string): readonly TargetedClass[] | null { +function ownChangeSince(root: string, base: string, from: string, head: string): string | null { + const run = (args: string[], env?: NodeJS.ProcessEnv, input?: string) => { + const r = spawnSync('git', args, { cwd: root, encoding: 'utf8', env, input, timeout: 60_000, maxBuffer: 256 * 1024 * 1024 }); + return r.status === 0 ? r.stdout : null; + }; + const raw = (a: string, b: string) => { + const out = run(['diff', '--raw', '-z', '--no-renames', '--full-index', a, b]); + if (out === null) return null; + const t = out.split('\0'); + const entries = new Map(); + for (let i = 0; i + 1 < t.length; i += 2) entries.set(t[i + 1]!, t[i]!); + return entries; + }; + const hunks = (a: string, b: string, p: string) => run(['--literal-pathspecs', 'diff', '--no-color', '--no-ext-diff', '--no-renames', '--binary', a, b, '--', p]) + ?.split('\n').filter(l => !l.startsWith('index ')).map(l => l.replace(/^@@ -\d+(?:,\d+)? \+\d+(?:,\d+)? @@/, '@@')).join('\n') ?? null; + const baseSha = baseCommit(root, `origin/${base.replace(/^origin\//, '')}`); + const mbFrom = baseSha.ok ? git(root, ['merge-base', baseSha.out, from]) : baseSha; + const mbHead = baseSha.ok ? git(root, ['merge-base', baseSha.out, head]) : baseSha; + let moved: string[]; + if (mbFrom.ok && mbHead.ok) { + const was = raw(mbFrom.out, from), now = raw(mbHead.out, head); + if (!was || !now) return null; + moved = [...new Set([...was.keys(), ...now.keys()])].filter(p => { + if (was.get(p) === now.get(p)) return false; + const before = hunks(mbFrom.out, from, p); + return before === null || before !== hunks(mbHead.out, head, p); + }); + } else { + const all = raw(from, head); + if (!all) return null; + moved = [...all.keys()]; + } + const gitDir = git(root, ['rev-parse', '--absolute-git-dir']); + if (!gitDir.ok) return null; + const index = path.join(gitDir.out, `gstack-since-${process.pid}-${randomBytes(4).toString('hex')}.index`); + const env = { ...process.env, GIT_INDEX_FILE: index, GIT_AUTHOR_NAME: 'gstack-ship-receipt', GIT_AUTHOR_EMAIL: 'ship-receipt@gstack.local', GIT_COMMITTER_NAME: 'gstack-ship-receipt', GIT_COMMITTER_EMAIL: 'ship-receipt@gstack.local', GIT_AUTHOR_DATE: '2000-01-01T00:00:00Z', GIT_COMMITTER_DATE: '2000-01-01T00:00:00Z' }; + try { + const listed = moved.length ? run(['--literal-pathspecs', 'ls-tree', '-z', '--full-tree', head, '--', ...moved]) : ''; + if (listed === null) return null; + const atHead = new Map(listed.split('\0').filter(Boolean).map(l => [l.slice(l.indexOf('\t') + 1), l] as const)); + const entries = moved.map(p => `${atHead.get(p) ?? `0 ${'0'.repeat(from.length)}\t${p}`}\0`).join(''); + if (run(['read-tree', from], env) === null || run(['update-index', '-z', '--index-info'], env, entries) === null) return null; + const tree = run(['write-tree'], env)?.trim(); + return (tree && run(['commit-tree', '--no-gpg-sign', tree, '-p', from, '-m', 'gstack-ship-receipt: the PR\'s own change since the targeted read'], env)?.trim()) || null; + } finally { fs.rmSync(index, { force: true }); } +} + +/** + * The targeted classes of what the PR's own change did from `from` to `head` against origin/ (ownChangeSince), with + * gstack-gate-diff run over that change whenever a non-docs path moved (a test edit that skips a case is gate class); null when + * `from` is not an ancestor of `head`, the change cannot be built, the gate scan did not run, or a class input is unreadable. + */ +export function targetedClassesSince(root: string, from: string, head: string, base: string): readonly TargetedClass[] | null { if (!git(root, ['merge-base', '--is-ancestor', from, head]).ok) return null; - const at = headInputs(root, from, head, undefined, true); + const since = ownChangeSince(root, base, from, head); + if (!since) return null; + const at = headInputs(root, from, since, undefined, true); let gateTagged: number | null = 0; if (gateScanNeeded(at.c.paths, at.c.root)) { - const g = runScriptSync(path.join(BIN_DIR, 'gstack-gate-diff'), [from], { cwd: root, encoding: 'utf8', timeout: 120_000, maxBuffer: 16 * 1024 * 1024 }); + const g = runScriptSync(path.join(BIN_DIR, 'gstack-gate-diff'), ['--commit', since], { cwd: root, encoding: 'utf8', timeout: 120_000, maxBuffer: 16 * 1024 * 1024 }); const m = /GATE_SUMMARY: .*?tagged=\{([^}]*)\}/.exec(g.stdout ?? ''); if (g.status !== 0 || !m) return null; gateTagged = m[1]!.split(',').reduce((n, kv) => n + (Number(kv.split(':')[1]) || 0), 0); @@ -498,14 +554,16 @@ export function targetedClassesSince(root: string, from: string, head: string): } /** - * Whether what changed from `from` to the checked-out `head` is plain docs, which a targeted record carries across (GST-1195): - * every path docs class, R65's claim rule (platformFits: no numeric claim, no results, prereg or eval path), and no money, - * security, secret or auth wording in the changed lines (the risk matchers plus DOCS_CLAIM_WORDS, since the matchers spare prose). - * False when `from` is not an ancestor or the diff is unreadable. + * Whether what the PR's own change did from `from` to `head` against origin/ (ownChangeSince) is plain docs, which a + * targeted record carries across (GST-1195): every path docs class, R65's claim rule (platformFits: no numeric claim, no results, + * prereg or eval path), and no money, security, secret or auth wording in the changed lines (the risk matchers plus + * DOCS_CLAIM_WORDS, since the matchers spare prose). False when `from` is not an ancestor or the change is unreadable. */ -export function plainDocsSince(root: string, from: string, head: string): boolean { +export function plainDocsSince(root: string, from: string, head: string, base: string): boolean { if (!git(root, ['merge-base', '--is-ancestor', from, head]).ok) return false; - const c = collectCommit(root, from, head); + const since = ownChangeSince(root, base, from, head); + if (!since) return false; + const c = collectCommit(root, from, since); if (c.errors.length || !c.paths.length || !c.paths.every(p => classifyPath(p, c.root) === 'docs') || c.perPath.some(p => p.binary)) return false; const lines = c.paths.flatMap(p => [...(c.added?.[p] ?? []), ...(c.removed?.[p] ?? [])]); if (lines.some(l => l.toLowerCase().split(/[^a-z0-9]+/).some(t => DOCS_CLAIM_WORDS.has(t) || AUTH_PATH_PREFIXES.some(a => t.startsWith(a))))) return false; diff --git a/test/review-targeted-record.test.ts b/test/review-targeted-record.test.ts index 3eeb5909f..a936d99b3 100644 --- a/test/review-targeted-record.test.ts +++ b/test/review-targeted-record.test.ts @@ -12,7 +12,7 @@ import { EXIT } from '../lib/headless-artifacts'; import { checkoutTree } from '../lib/review-brief'; import { TARGETED_RULES, targetedBriefSha } from '../lib/review-targeted'; import { extractReceipt, placeReceiptFirst } from '../lib/ship-receipt'; -import { receiptDepthAtHead } from '../lib/ship-receipt-depth'; +import { receiptDepthAtHead, targetedClassesSince } from '../lib/ship-receipt-depth'; import { cleanup, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/restamp-fixture'; setDefaultTimeout(180_000); @@ -293,3 +293,77 @@ describe('/ship\'s receipt writes pass the targeted record (item 3, #3853)', () expect(expand()).toEqual(['--review-targeted', `@${recordPath}`]); }); }); + +describe('a main merge after the read keeps the record when the PR\'s own change is unchanged (GST-1593, harness-evals #293)', () => { + const mainCommit = (repo: FixtureRepo, files: Record, msg: string) => { + repo.git('checkout', '-q', 'main'); + for (const [file, text] of Object.entries(files)) commit(repo, file, text, msg); + repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-q', 'feat'); + repo.git('fetch', '-q', 'origin'); + }; + const mainBrings = { 'lib/budget-ledger.ts': 'export const spend_cap_usd = 40;\n', 'lib/secrets.ts': 'export const key = process.env.OPENAI_API_KEY;\n' }; + const readAndMerge = () => { + const { repo, base, head } = moneyBranch(); + const record = path.join(repo.root, 'targeted.json'); + expect(verify(repo, targetedRead(repo, base, 'h1', 'NO FINDINGS'), record).status).toBe(0); + mainCommit(repo, mainBrings, 'main: budget ledger and secrets'); + return { repo, head, record }; + }; + + // Value: protects=a main merge that brings money- and secret-tagged files leaves the PR's product patch unchanged, so the + // record binds (HARN-1 on harness-evals #293, #292, #225: targetedClassesSince diffed across the merge commit and voided it); + // fails_when=the classes since the read count files only main brought in; seam=gstack-ship-receipt write --review-targeted + test('read, then a main merge that brings money and secret files: write binds', () => { + const { repo, head, record } = readAndMerge(); + repo.git('merge', '-q', '--no-edit', 'origin/main'); + const bound = write(repo, ['--review-targeted', `@${record}`]); + expect(bound.status, bound.err).toBe(EXIT.ok); + expect(JSON.parse(bound.out).review_targeted).toMatchObject({ classes: ['money'], status: 'done' }); + expect(targetedClassesSince(repo.work, head, repo.git('rev-parse', 'HEAD'), 'main')).toEqual([]); + }); + + // Value: protects=a branch-side money edit or skipped test after the read still voids the record across a main merge; + // fails_when=the PR-own basis drops the branch's own change; seam=targetedClassesSince and write --review-targeted + test('after a main merge, a branch-side money edit and a branch-side skipped test are refused', () => { + const { repo, head, record } = readAndMerge(); + repo.git('merge', '-q', '--no-edit', 'origin/main'); + const merged = repo.git('rev-parse', 'HEAD'); + commit(repo, 'src/charge.ts', 'export const spend_cap_usd = 50;\n', 'money edit after the merge'); + expect(targetedClassesSince(repo.work, head, repo.git('rev-parse', 'HEAD'), 'main')).toContain('money'); + const money = write(repo, ['--review-targeted', `@${record}`]); + expect(money.status).toBe(EXIT.fail); + expect(money.err).toContain(`the targeted reader read ${head.slice(0, 12)}`); + + repo.git('reset', '-q', '--hard', merged); + commit(repo, 'test/charge.test.ts', 'import { test, expect } from "bun:test";\ntest.skip("cap", () => { expect(5).toBe(5); });\n', 'skip a case after the merge'); + expect(targetedClassesSince(repo.work, head, repo.git('rev-parse', 'HEAD'), 'main')).toContain('gate'); + const skipped = write(repo, ['--review-targeted', `@${record}`]); + expect(skipped.status).toBe(EXIT.fail); + expect(skipped.err).toContain(`the targeted reader read ${head.slice(0, 12)}`); + }); + + // Value: protects=a plain docs commit after the read still carries the record when a main merge also brought money and secret + // files (GST-1195's docs carry over the same PR-own basis); fails_when=plainDocsSince reads main's files as the branch's; + // seam=gstack-ship-receipt write --review-targeted + test('after a main merge, a plain docs commit binds', () => { + const { repo, record } = readAndMerge(); + repo.git('merge', '-q', '--no-edit', 'origin/main'); + commit(repo, 'docs/run-order.md', '# Run order\n\nRead the overview first, then the walkthrough.\n', 'Step 14.5 docs'); + const docs = write(repo, ['--review-targeted', `@${record}`]); + expect(docs.status, docs.err).toBe(EXIT.ok); + }); + + // Value: protects=a merge whose resolution changes the PR's own hunks is a change the reader never read; fails_when=the merge + // commit's own edit to the PR's file is credited to main; seam=targetedClassesSince and write --review-targeted + test('a main merge that also changes the PR\'s own hunks is refused', () => { + const { repo, head, record } = readAndMerge(); + repo.git('merge', '-q', '--no-commit', '--no-ff', 'origin/main'); + fs.writeFileSync(path.join(repo.work, 'src', 'charge.ts'), 'export const spend_cap_usd = 9;\n'); + repo.git('add', '-A'); repo.git('commit', '-q', '--no-edit'); + expect(targetedClassesSince(repo.work, head, repo.git('rev-parse', 'HEAD'), 'main')).toEqual(['money']); + const evil = write(repo, ['--review-targeted', `@${record}`]); + expect(evil.status).toBe(EXIT.fail); + expect(evil.err).toContain(`the targeted reader read ${head.slice(0, 12)}`); + }); +});