diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 930b8b48e..b9b956945 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -294,7 +294,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/changelog.d/capy-gst-1593-targeted-main-merge.md b/changelog.d/capy-gst-1593-targeted-main-merge.md new file mode 100644 index 000000000..5fe1ac148 --- /dev/null +++ b/changelog.d/capy-gst-1593-targeted-main-merge.md @@ -0,0 +1 @@ +- **A main merge after the targeted read no longer voids the record.** `gstack-ship-receipt write --review-targeted` refused with "the product changed after the read" whenever main brought in money- or secret-tagged files, though the PR's product patch was unchanged, because the classes changed since the read were computed across the merge commit (harness-evals #293, #292, #225). Write now computes those classes, and the plain-docs carry, over the PR's own change, meaning the paths whose part of the PR's diff against its merge base differs between the read and HEAD. A branch-side edit to money, secret, auth or gate code after the read, or a merge resolution that changes the PR's own hunks, still refuses. (GST-1593) diff --git a/docs/review-policy.md b/docs/review-policy.md index d8bf3e620..d82049b9e 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -489,7 +489,10 @@ renders its brief; `verify --targeted-out .gstack/tmp/targeted.json` writes the the reader got its brief verbatim and its reply was collected on a committed diff, whatever happened to HEAD after, and it names the commit it read (`head`). `write` binds it only while that commit is an ancestor of HEAD, its product patch is the receipt's, and nothing changed -after it carries a targeted class, with the gate scan run over that range: any other product change +after it carries a targeted class, with the gate scan run over that change. What changed after the +read is the PR's own change, on `product_patch_id`'s basis: the paths whose part of the PR's diff +against its merge base differs between the read and HEAD, so files only a base merge brought in never +count, while a branch edit or a merge resolution that changes the PR's own hunks does (GST-1593). Any other product change after the read refuses with `RECEIPT_INVALID`, and so does a test edit the gate scan tags (a skipped case). A docs-only change after the read (TODOS.md included) carries the record only when it changes no binary file, adds no numeric claim, touches no results, prereg or eval path, diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 2911d4c32..66c613a38 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3502,7 +3502,9 @@ or when `--review-targeted` names a read commit that is not an ancestor of HEAD, or after which a change carries a targeted class (money, gate, auth or secret code changed after the targeted reader read it, a test edit the gate scan tags included), or whose product patch is not the receipt's and the change -since it is more than plain docs. A docs-only change after the read carries the +since it is more than plain docs. The change since the read is the PR's own: a +base merge after the read that leaves the PR's diff alone never voids the record, +whatever files it brought in (GST-1593). A docs-only change after the read carries the record only when it changes no binary file, adds no numeric claim, touches no results, prereg or eval path, and names no money, security, secret or auth topic; any other docs change needs the reader again (GST-1195). With `--body`, write also refuses, writing diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 5e86b6d23..fa105169a 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -479,16 +479,87 @@ 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: 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, and so does a path whose mode at `head` is not the replayed one. The commit carries the messages of every + * commit since `from` that origin/ does not hold, side-branch merges included (a CI skip marker is gate class). 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 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; + 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 [wOld, wMode, wBase, wNew] = w?.split(' ') ?? [], [nOld, nMode, nBase, nNew] = n?.split(' ') ?? []; + const shas = [wNew, wBase, nBase, nNew]; + if (wBase === nBase || nMode !== (wOld === `:${wMode}` ? nOld?.slice(1) : wMode) || 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]) : ''; + const messages = run(['log', '--format=%B', `${from}..${head}`, ...(baseSha.ok ? [`^${baseSha.out}`] : [])]); + 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', '-m', messages.trim() || '(no messages)'], env)?.trim()) || null; + } finally { for (const s of ['index', '0', '1', '2']) fs.rmSync(`${tmp}.${s}`, { 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 +569,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..52a341d5a 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,152 @@ 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 classesSince = (repo: FixtureRepo, read: string) => { + const saved = Object.fromEntries(Object.keys(repo.env).map(k => [k, process.env[k]])); + Object.assign(process.env, repo.env); + try { return targetedClassesSince(repo.work, read, repo.git('rev-parse', 'HEAD'), 'main'); } finally { + 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(); + 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(classesSince(repo, head)).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(classesSince(repo, head)).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(classesSince(repo, head)).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(classesSince(repo, head)).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)}`); + }); + + // Value: protects=with no main merge, a branch commit after the read that moves the PR's money line to another block with the + // 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 repo = makeFixtureRepo({ prefix: 'gst1593-relocate-' }); + 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'); + 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'); + }); + + // Value: protects=a CI skip marker on a side-branch commit merged with --no-ff after the read stays gate class; fails_when=the + // built commit carries only first-parent messages; seam=targetedClassesSince + test('a side-branch commit whose message skips CI, merged with --no-ff after the read, is refused', () => { + const { repo, head } = moneyBranch(); + repo.git('checkout', '-q', '-b', 'side'); + commit(repo, 'test/charge.test.ts', 'import { test } from "bun:test";\ntest("cap", () => {});\n', '[skip ci] add a cap case'); + repo.git('checkout', '-q', 'feat'); + repo.git('merge', '-q', '--no-ff', '-m', 'merge side', 'side'); + expect(classesSince(repo, head)).toContain('gate'); + }); + + // Value: protects=a mode-only change after the read, on a file a main merge also changed, still voids the record; fails_when=the + // replay compares blob bytes and ignores the mode; seam=targetedClassesSince + test('after a main merge to the PR\'s own file, a mode-only change is refused', () => { + const repo = makeFixtureRepo({ prefix: 'gst1593-mode-' }); + 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'); + repo.git('update-index', '--chmod=+x', 'src/charge.ts'); + repo.git('commit', '-q', '-m', 'make charge executable'); + expect(classesSince(repo, read)).toContain('money'); + }); +});