From c4624834f218182a8a40e9ea48ca112c64f47d1b Mon Sep 17 00:00:00 2001 From: garrytan Date: Fri, 6 Nov 2026 07:47:51 +0000 Subject: [PATCH] fix: pre-landing review fixes (GST-1593) ownChangeSince: a path whose base content main also changed now moves unless replaying main's change onto the read's content (git merge-file) gives HEAD's content exactly, so relocating the PR's money hunk to an identical context after a main merge voids the targeted record; position-free hunk comparison is gone. The built commit carries the branch's first-parent messages since the read, so a [skip ci] commit stays RH-14 gate class. Two red-first tests cover both. Review-Finding: v1/outside-voice#1 reader=outside-voice Review-Finding: v1/adversarial-native#1 reader=adversarial-native Review-Finding: v1/outside-voice#2 reader=outside-voice Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/ship-receipt-depth.ts | 67 +++++++++++++++++------------ test/review-targeted-record.test.ts | 29 ++++++++++++- 2 files changed, 66 insertions(+), 30 deletions(-) diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index a351fe642..5f6846891 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -482,9 +482,11 @@ export function bindShippedReceipt(root: string, baseRef: string, body: string, * 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, and where the base - * content did not move any change to the PR's side moves it, so a hunk relocated to an identical context counts. With no resolvable - * merge base every path that changed from `from` to `head` moves. Null when git cannot build it. + * content (GST-1593); a branch edit, or a merge resolution that changes the PR's own hunks, moves the path: a path whose base + * content main also changed moves unless replaying main's change onto `from`'s content (git merge-file) gives `head`'s exactly, + * so a hunk relocated to an identical context counts. With no resolvable merge base every path that changed from `from` to + * `head` moves. The commit carries the branch's first-parent messages since `from` (a CI skip marker is gate class). Null when + * git cannot build it. */ function ownChangeSince(root: string, base: string, from: string, head: string): string | null { const run = (args: string[], env?: NodeJS.ProcessEnv, input?: string) => { @@ -499,40 +501,49 @@ function ownChangeSince(root: string, base: string, from: string, head: string): 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 blob = (sha: string) => { + const r = spawnSync('git', ['cat-file', 'blob', sha], { cwd: root, timeout: 60_000, maxBuffer: 256 * 1024 * 1024 }); + return r.status === 0 ? r.stdout : null; + }; + const gitDir = git(root, ['rev-parse', '--absolute-git-dir']); + if (!gitDir.ok) return null; + const tmp = path.join(gitDir.out, `gstack-since-${process.pid}-${randomBytes(4).toString('hex')}`); + const index = `${tmp}.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' }; 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 => { - const w = was.get(p), n = now.get(p); - if (w === n) return false; - if (!w || !n || w.split(' ')[2] === n.split(' ')[2]) return true; - 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 { + 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 => { + const w = was.get(p), n = now.get(p); + if (w === n) return false; + const [, , wBase, wNew] = w?.split(' ') ?? [], [, , nBase, nNew] = n?.split(' ') ?? []; + const shas = [wNew, wBase, nBase, nNew]; + if (wBase === nBase || shas.some(s => !s || /^0+$/.test(s))) return true; + const blobs = shas.map(s => blob(s!)); + if (blobs.some(b => !b)) return true; + const files = blobs.slice(0, 3).map((b, i) => { fs.writeFileSync(`${tmp}.${i}`, b!); return `${tmp}.${i}`; }); + const merged = spawnSync('git', ['merge-file', '-p', '-q', ...files], { cwd: root, timeout: 60_000, maxBuffer: 256 * 1024 * 1024 }); + return merged.status !== 0 || !merged.stdout.equals(blobs[3]!); + }); + } else { + const all = raw(from, head); + if (!all) return null; + moved = [...all.keys()]; + } const listed = moved.length ? run(['--literal-pathspecs', 'ls-tree', '-z', '--full-tree', head, '--', ...moved]) : ''; - if (listed === null) return null; + const messages = run(['log', '--first-parent', '--format=%B', `${from}..${head}`]); + if (listed === null || messages === 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 }); } + return (tree && run(['commit-tree', '--no-gpg-sign', tree, '-p', from, '-m', 'gstack-ship-receipt: the PR\'s own change since the targeted read', '-m', messages.trim() || '(no messages)'], env)?.trim()) || null; + } finally { for (const s of ['index', '0', '1', '2']) fs.rmSync(`${tmp}.${s}`, { force: true }); } } /** diff --git a/test/review-targeted-record.test.ts b/test/review-targeted-record.test.ts index 4d0205dd8..c558ff0f7 100644 --- a/test/review-targeted-record.test.ts +++ b/test/review-targeted-record.test.ts @@ -309,6 +309,8 @@ describe('a main merge after the read keeps the record when the PR\'s own change for (const [k, v] of Object.entries(saved)) { if (v === undefined) delete process.env[k]; else process.env[k] = v; } } }; + const block = (cond: string, cap: boolean) => [` if (${cond}) {`, ' a();', ' b();', ' c();', ...(cap ? [' const spend_cap_usd = 5000;'] : []), ' d();', ' e();', ' f();', ' }']; + const charge = (one: boolean, two: boolean, header = '// charge') => [header, 'export function charge(amount: number) {', ...block('amount > 0', one), ...block('amount < 0', two), '}', ''].join('\n'); 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(); @@ -378,8 +380,6 @@ describe('a main merge after the read keeps the record when the PR\'s own change // same context and function header still voids the record (position-free hunks compared equal); fails_when=a path whose // base content did not move is judged by its line-number-free hunks; seam=targetedClassesSince test('no main merge: a branch edit that relocates the PR\'s money hunk to an identical context is refused', () => { - const block = (cond: string, cap: boolean) => [` if (${cond}) {`, ' a();', ' b();', ' c();', ...(cap ? [' const spend_cap_usd = 5000;'] : []), ' d();', ' e();', ' f();', ' }']; - const charge = (one: boolean, two: boolean) => ['export function charge(amount: number) {', ...block('amount > 0', one), ...block('amount < 0', two), '}', ''].join('\n'); const repo = makeFixtureRepo({ prefix: 'gst1593-relocate-' }); repos.push(repo); commit(repo, 'src/charge.ts', charge(false, false), 'main: charge'); @@ -389,4 +389,29 @@ describe('a main merge after the read keeps the record when the PR\'s own change commit(repo, 'src/charge.ts', charge(false, true), 'move the cap to the second block after the read'); expect(classesSince(repo, read)).toContain('money'); }); + + // Value: protects=a main merge that also changed the PR's file replays cleanly (the record binds) yet a later relocation of the + // PR's money hunk to an identical context still voids it; fails_when=position-free hunks decide a path whose base moved, or the + // replay over-refuses a clean main change; seam=targetedClassesSince + test('a main merge to the PR\'s own file binds; relocating the money hunk after it is refused', () => { + const repo = makeFixtureRepo({ prefix: 'gst1593-relocate-merge-' }); + repos.push(repo); + commit(repo, 'src/charge.ts', charge(false, false), 'main: charge'); + repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-q', '-b', 'feat'); + const read = commit(repo, 'src/charge.ts', charge(true, false), 'cap in the first block'); + mainCommit(repo, { 'src/charge.ts': charge(false, false, '// charge: amounts in cents') }, 'main: header'); + repo.git('merge', '-q', '--no-edit', 'origin/main'); + expect(classesSince(repo, read)).toEqual([]); + commit(repo, 'src/charge.ts', charge(false, true, '// charge: amounts in cents'), 'move the cap to the second block after the merge'); + expect(classesSince(repo, read)).toContain('money'); + }); + + // Value: protects=a CI skip marker in a branch commit after the read stays gate class (RH-14) though the scan reads a built + // commit; fails_when=the built commit drops the branch's messages; seam=targetedClassesSince + test('a branch commit after the read whose message skips CI is refused', () => { + const { repo, head } = moneyBranch(); + commit(repo, 'test/charge.test.ts', 'import { test } from "bun:test";\ntest("cap", () => {});\n', '[skip ci] add a cap case'); + expect(classesSince(repo, head)).toContain('gate'); + }); });