From ca367035c0350600425eaed7f9532d6cd8eea569 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 6 Nov 2026 15:37:33 -0800 Subject: [PATCH] fix: the receipt and the review plan agree on stamp and docs carry for review_targeted (GST-1689) (#4099) * fix: the receipt and the review plan agree on stamp and docs carry for review_targeted (GST-1689) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: pre-landing review fixes A restamp after a base merge now carries the targeted record: the CHANGELOG is read against the read commit's merged with the base's, so a sibling release's section is not counted as the PR's docs change. Adds the stamps-only and base-merge restamp cases. Review-Finding: p1/red-team#1 reader=red-team Review-Finding: p1/specialist-batched#1 reader=testing Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/capy-gst-1689-stamp-docs-carry.md | 1 + docs/review-policy.md | 21 ++- docs/troubleshooting.md | 4 +- lib/review-targeted-delta.ts | 24 +-- lib/ship-receipt-depth.ts | 116 +++++++++++--- test/review-targeted-stamp-carry.test.ts | 150 ++++++++++++++++++ 6 files changed, 278 insertions(+), 38 deletions(-) create mode 100644 changelog.d/capy-gst-1689-stamp-docs-carry.md create mode 100644 test/review-targeted-stamp-carry.test.ts diff --git a/changelog.d/capy-gst-1689-stamp-docs-carry.md b/changelog.d/capy-gst-1689-stamp-docs-carry.md new file mode 100644 index 000000000..e9dfdacc3 --- /dev/null +++ b/changelog.d/capy-gst-1689-stamp-docs-carry.md @@ -0,0 +1 @@ +- **A restamp plus a plain docs edit after the targeted read no longer forces a third review round.** `gstack-ship-receipt write` and `gstack-review-plan` now decide "the product changed after the read" with one classifier: a path whose change is a release stamp under the base policy's stamp rule (`stamp_files`, `stamp_paths`, `stamp_patterns`, the version file, manifests, lockfiles and the CHANGELOG re-heading or fragment fold) always carries the targeted record, and docs and CHANGELOG edits carry under the existing plain-docs rule, which still refuses a numeric, money, security or other claim. Before, /ship's restamp plus its docs audit made write refuse the record while the plan printed `NEXT: done`, so only `--post-review` (adversarial plus the outside voice) could finish (GBRA-345, gbrain #8727). A code change after the read still drops the record, and a docs claim beside a restamp now gets the one targeted reader instead of a dead end. (GST-1689) diff --git a/docs/review-policy.md b/docs/review-policy.md index 44097d6e2..499b25e18 100644 --- a/docs/review-policy.md +++ b/docs/review-policy.md @@ -505,7 +505,11 @@ read is the PR's own change, on `product_patch_id`'s basis: the paths whose part 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 +skipped case). A release stamp after the read always carries the record: a path whose change +is a stamp under the base policy's stamp rule (`stamp_files`, `stamp_paths`, `stamp_patterns`, +the version file, manifests, lockfiles and the CHANGELOG re-heading or fragment fold, the rule +the tree receipt uses) is left out of that change (GST-1689). 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, and names no money, security, secret or auth topic (GST-1195). Every verify removes `--targeted-out` before it writes a record, so an earlier run's record never reaches @@ -532,13 +536,18 @@ change only when that change is plain docs, so the receipt asked for the targeted reader on HEAD. But the plan printed `done`, so nothing could dispatch it (harness-evals #225). Now, when the loop is done, pass `--range-base ` and `--targeted-ran `. If every path whose branch patch changed since -then is docs class and not binary, and the change is not plain docs, the plan +targeted_patch_id it read>`. If every path the PR's own change moved since +then, release stamps aside, is docs class and not binary, and the change is +not plain docs, the plan prints `NEXT: targeted`, one `DISPATCH: targeted` for the head's classes and `TARGETED_DELTA: range_base= paths= classes=` (`targeted_delta` in `--json`). The patch-id check proves that range_base is what the reader read. The branch patch is compared on each side's own -merge base, so a base merge alone adds no path. +merge base, so a base merge alone adds no path. The plan classifies the +change with the same function `write` uses (`docsChangeSince` in +`lib/ship-receipt-depth.ts`), so a restamp plus a plain docs-audit edit is +`done` in the plan and carried by the receipt, and a docs claim beside a +restamp gets the one reader (GST-1689). The reader reads the whole merge-base diff at HEAD, like every targeted reader. It isn't scoped to the docs change, because `render` reads no other @@ -552,7 +561,7 @@ among its plan lines. After `verify --targeted-out`, pass the new Every other case stays `done` and prints `SKIP: targeted-only read ()`. That covers a code or binary change with no `--grant` (the granted -case is below), which takes the review loop as before; plain docs, which the record carries; a range_base the +case is below), which takes the review loop as before; plain docs and release stamps, which the record carries; a range_base the `--targeted-ran` patch doesn't match; and uncommitted edits. `--force targeted` refuses (exit 2) and names these flags, because the targeted reader is no specialist (`lib/review-targeted-delta.ts`, @@ -667,7 +676,7 @@ Findings bind by id first (GST-1168). A row finding that came from a dispatched | `review_loop.rounds` | passes plus fix-checks (R51 caps it at 2; the queue's `REVIEW_DEPTH:` line prints it) | | `review_loop.dispatches` | reviewer dispatches by role: `specialist`, `red-team`, `adversarial-native`, `outside-voice`, `fix-check`, `targeted` | | `review_loop.est_input_tokens` | prompt plus diff bytes the dispatches read, divided by 4: a relative estimate for comparing runs, not a bill | -| `review_targeted` | the [targeted pass](#targeted-pass-on-money-gate-auth-and-secret-code): the `classes` it covered, `rules`, `brief_sha`, the receipt's own `product_patch_id`, `status` (`done` only with its brief sent verbatim and its output collected on a committed diff, else `failed`; `write` refuses a record when a change after its read commit carries a targeted class or the read commit is not an ancestor, or when its product patch differs and the change is more than plain docs (no binary file and no numeric, results, prereg, eval, money, security, secret or auth claim; GST-1195), and refuses to write without a record when the head needs a class), `findings` and `raised_by`; a carry keeps it while the product patch is unchanged | +| `review_targeted` | the [targeted pass](#targeted-pass-on-money-gate-auth-and-secret-code): the `classes` it covered, `rules`, `brief_sha`, the receipt's own `product_patch_id`, `status` (`done` only with its brief sent verbatim and its output collected on a committed diff, else `failed`; `write` refuses a record when a change after its read commit carries a targeted class or the read commit is not an ancestor, or when its product patch differs and the change is more than release stamps and plain docs (no binary file and no numeric, results, prereg, eval, money, security, secret or auth claim; GST-1195, GST-1689), and refuses to write without a record when the head needs a class), `findings` and `raised_by`; a carry keeps it while the product patch is unchanged | Each review row may carry `dispatches: [{role, effort, prompt_bytes, diff_bytes, status}]`, and each finding `raised_by` (the reviewer that raised it, diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index c2df46508..5876b9a68 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3568,7 +3568,9 @@ 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. 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 +whatever files it brought in (GST-1593). A release stamp after the read (a path whose +change is a stamp under the base policy's stamp rule) always carries the record +(GST-1689). 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/review-targeted-delta.ts b/lib/review-targeted-delta.ts index 14a25c0b5..81367bbec 100644 --- a/lib/review-targeted-delta.ts +++ b/lib/review-targeted-delta.ts @@ -1,20 +1,20 @@ /** * review-targeted-delta — the targeted-only read after a finished review loop (GST-1598, * docs/review-policy.md#targeted-only-read-after-the-loop). A targeted record names the commit its reader read, and - * gstack-ship-receipt write carries it across a later docs change only when that change is plain docs (plainDocsSince, - * GST-1195). A docs claim added after the loop finished (a changelog fragment with dollar figures, harness-evals #225) + * gstack-ship-receipt write carries it across a later change only when that change is release stamps and plain docs + * (plainDocsSince, GST-1195); this module classifies the change with the same docsChangeSince, so the two agree + * (GST-1689). A docs claim added after the loop finished (a changelog fragment with dollar figures, harness-evals #225) * left no exit: the plan printed `done`, so nothing dispatched the reader the receipt asks for. bin/gstack-review-plan * asks this module when the loop is done and the head has targeted classes the passed --targeted-ran no longer covers. * It dispatches one targeted reader on HEAD only when range_base is the commit that read happened on (its targeted - * patch-id is --targeted-ran), every path whose branch patch changed since is docs class and not binary, and the change - * is not plain docs. With the owner's review_loop_grant for a stop head inside range_base..HEAD (`granted`, checked by the - * plan before it gets here), a code change since the read takes the same one reader (GST-1607, harness-evals #271: the + * patch-id is --targeted-ran), every path the PR's own change moved since, release stamps aside, is docs class and not + * binary, and that change is not plain docs. With the owner's review_loop_grant for a stop head inside range_base..HEAD + * (`granted`, checked by the plan before it gets here), a code change since the read takes the same one reader (GST-1607, harness-evals #271: the * granted fix round moved the product patch the record names). Anything else keeps `done` with the reason. */ import { spawnSync } from 'node:child_process'; -import { classifyPath } from './review-depth'; import { comparePath, touchedPaths } from './ship-patch'; -import { plainDocsSince } from './ship-receipt-depth'; +import { docsChangeSince } from './ship-receipt-depth'; export type TargetedDelta = { state: 'dispatch'; range_base: string; paths: string[] } | { state: 'skip'; reason: string }; @@ -41,9 +41,9 @@ export function targetedDelta(root: string, base: string, rangeBase: string, ran const sides = { reviewedBase: readBase, reviewed: read, currentBase: headBase, current: head }; const paths = [...new Set([...before, ...now])].filter(p => !['same', 'context'].includes(comparePath(root, sides, p))).sort(); if (!paths.length) return skip(`the branch patch is unchanged since range_base ${read.slice(0, 12)}`); - const binary = new Set((git(['diff', '--numstat', '-z', '--no-renames', headBase, head]) ?? '').split('\0').filter(l => l.startsWith('-\t-\t')).map(l => l.slice(4))); - const other = paths.filter(p => classifyPath(p, root) !== 'docs' || binary.has(p)); - if (other.length && !granted) return skip(`the change since range_base ${read.slice(0, 12)} is not docs-only (${other.join(', ')}), so it takes the review loop`); - if (plainDocsSince(root, read, head, base)) return skip(`the change since range_base ${read.slice(0, 12)} is plain docs, which the targeted record carries (GST-1195)`); - return { state: 'dispatch', range_base: read, paths }; + const change = docsChangeSince(root, read, head, base); + if (!change) return skip(`git could not build the PR's own change since range_base ${read.slice(0, 12)}`); + if (change.verdict === 'code' && !granted) return skip(`the change since range_base ${read.slice(0, 12)} is not docs-only (${change.paths.join(', ')}), so it takes the review loop`); + if (change.verdict === 'plain') return skip(`the change since range_base ${read.slice(0, 12)} is ${change.paths.length ? 'plain docs' : 'release stamps'}${change.paths.length && change.stamps.length ? ' and release stamps' : ''}, which the targeted record carries (GST-1195, GST-1689)`); + return { state: 'dispatch', range_base: read, paths: [...change.paths] }; } diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index dc7c32e31..3d7c931eb 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -33,6 +33,10 @@ import { platformReviewAt } from './review-platform'; import { reviewPolicy, type ReviewPolicy } from './ship-policy-review'; import { TARGETED_CLASSES, classList, targetedClasses, type TargetedClass } from './review-targeted'; import { runScriptSync } from './script-invocation'; +import { parseUnifiedDiff, versionAtRef } from './restamp'; +import { branchSides } from './stamp-only'; +import { classifyFiles } from './tree-receipt'; +import { mergeChangelogs } from './changelog-check'; import { bindReceipt, type ReceiptBindingCode } from './ship-receipt'; import { carriedAudits, docsExceptionLine, parseReceiptDocs, type CarriedAudit } from './ship-receipt-docs'; @@ -245,7 +249,7 @@ export function parseReviewTargeted(value: unknown, productPatchId: string, prod if (typeof v.findings !== 'number' || !Number.isInteger(v.findings) || v.findings < 0) return bad('review_targeted.findings must be an integer >= 0'); if (typeof v.head !== 'string' || !/^[0-9a-f]{40,64}$/.test(v.head)) return bad('review_targeted.head must be the commit the targeted reader read (rerun gstack-review-dispatch verify --targeted-out with this gstack)'); const read = productPatchOf(v.head); - if ((read !== productPatchId && !plainDocs?.(v.head)) || (classesSince && classesSince(v.head)?.length !== 0)) return bad(`the targeted reader read ${v.head.slice(0, 12)}, whose product patch ${read?.slice(0, 12) ?? 'cannot be read'} is not this receipt's ${productPatchId.slice(0, 12)}: the product changed after the read (only a docs change with no numeric, results, prereg, eval, money or security claim carries the record), so run the targeted reader on HEAD and verify again`); + if ((read !== productPatchId && !plainDocs?.(v.head)) || (classesSince && classesSince(v.head)?.length !== 0)) return bad(`the targeted reader read ${v.head.slice(0, 12)}, whose product patch ${read?.slice(0, 12) ?? 'cannot be read'} is not this receipt's ${productPatchId.slice(0, 12)}: the product changed after the read (only release stamps and a docs change with no numeric, results, prereg, eval, money or security claim carry the record), so run the targeted reader on HEAD and verify again`); const classes = TARGETED_CLASSES.filter(c => (v.classes as string[]).includes(c)); return { ok: true, targeted: { classes, rules: v.rules as string, brief_sha: v.brief_sha, product_patch_id: productPatchId, status: v.status as ReviewTargeted['status'], findings: v.findings, raised_by: (v.raised_by as string).trim() } }; } @@ -516,16 +520,17 @@ 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 + * merge base with origin/ (the local when origin's does not resolve, as write reads it; 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. + * commit since `from` that origin/ does not hold, side-branch merges included (a CI skip marker is gate class). A path in + * `keep` keeps `from`'s content. Null when git cannot build it. */ -function ownChangeSince(root: string, base: string, from: string, head: string): string | null { +function ownChangeSince(root: string, base: string, from: string, head: string, keep: readonly 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; @@ -547,7 +552,8 @@ function ownChangeSince(root: string, base: string, from: string, head: string): 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 baseAt = writeBase(root, base).sha; + const baseSha = baseAt ? { ok: true, out: baseAt } : { ok: false, out: '' }; 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 { @@ -572,6 +578,7 @@ function ownChangeSince(root: string, base: string, from: string, head: string): if (!all) return null; moved = [...all.keys()]; } + moved = moved.filter(p => !keep.includes(p)); 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; @@ -590,7 +597,7 @@ function ownChangeSince(root: string, base: string, from: string, head: string): */ 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 since = ownChangeSince(root, base, from, head); + const since = productChangeSince(root, base, from, head)?.since; if (!since) return null; const at = headInputs(root, from, since, undefined, true); let gateTagged: number | null = 0; @@ -605,20 +612,91 @@ export function targetedClassesSince(root: string, from: string, head: string, b } /** - * 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. + * What the PR's own change from `from` to `head` against origin/ (ownChangeSince) is to a targeted record: the one + * classifier gstack-ship-receipt write (plainDocsSince) and gstack-review-plan's targeted-only read (lib/review-targeted-delta.ts) + * share, so the receipt never refuses a change the plan calls done (GST-1689). `stamps` are the paths whose change is a release + * stamp under the base policy's stamp rule (lib/tree-receipt.ts classifyFiles: stamp_files, stamp_paths and stamp_patterns, the + * version file, manifests, lockfiles and the CHANGELOG re-heading or fragment fold), which always carry; `paths` are the rest. + * The rest is `plain` when it is plain docs (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); `claim` when it is docs that fail that rule; `code` when + * any path is not docs class or is binary. Null when `from` is not an ancestor or the change is unreadable. */ -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 since = ownChangeSince(root, base, from, head); - if (!since) return false; +export interface DocsChange { verdict: 'plain' | 'claim' | 'code'; stamps: readonly string[]; paths: readonly string[] } +export function docsChangeSince(root: string, from: string, head: string, base: string): DocsChange | null { + if (!git(root, ['merge-base', '--is-ancestor', from, head]).ok) return null; + const change = productChangeSince(root, base, from, head); + if (!change) return null; + const { since, stamps } = change; 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; - return platformFits('docs', c.paths, 0, scanNumbers(c.added ?? {})) && matchRisk({ codePaths: c.paths, added: c.added, removed: c.removed, scopeAuth: true }).size === 0; + if (c.errors.length || !(c.paths.length || stamps.length)) return null; + const paths = c.paths; + const verdict = (v: DocsChange['verdict']): DocsChange => ({ verdict: v, stamps, paths }); + if (!paths.length) return verdict('plain'); + if (!paths.every(p => classifyPath(p, c.root) === 'docs') || c.perPath.some(p => p.binary && paths.includes(p.path))) return verdict('code'); + const added = Object.fromEntries(paths.map(p => [p, c.added?.[p] ?? []])); + const removed = Object.fromEntries(paths.map(p => [p, c.removed?.[p] ?? []])); + const lines = paths.flatMap(p => [...added[p]!, ...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 verdict('claim'); + return verdict(platformFits('docs', paths, 0, scanNumbers(added)) && matchRisk({ codePaths: paths, added, removed, scopeAuth: true }).size === 0 ? 'plain' : 'claim'); +} +/** Whether the PR's own change since `from` carries a targeted record: release stamps and plain docs only (docsChangeSince). */ +export const plainDocsSince = (root: string, from: string, head: string, base: string): boolean => docsChangeSince(root, from, head, base)?.verdict === 'plain'; + +/** + * ownChangeSince less its release stamps: `since` holds the PR's own change after `from` with every path whose change is a + * release stamp under the base policy (`stamps`, stampPathsSince) kept at `from`, the change the targeted classes and the docs + * rule read, since a stamp moves only versions and always carries a targeted record (GST-1689). Null when git cannot build it. + */ +function productChangeSince(root: string, base: string, from: string, head: string): { since: string; stamps: string[] } | null { + const own = ownChangeSince(root, base, from, head); + if (!own) return null; + const stamps = stampPathsSince(root, base, from, own, head); + if (!stamps.length) return { since: own, stamps }; + const since = ownChangeSince(root, base, from, head, stamps); + return since ? { since, stamps } : null; +} + +/** + * The paths whose change from `from` to `since` is a release stamp under the base policy (classifyFiles); none when the policy, + * either version or the diff cannot be read. When origin/ changed the CHANGELOG between `from`'s merge base and `head`'s, + * the CHANGELOG is read against `from`'s merged with the base's (mergeChangelogs, as a base merge resolves it), so the released + * sections a base merge brings are not counted as the PR's own change; an unmergeable pair keeps `from`'s and fails closed. + */ +function stampPathsSince(root: string, base: string, from: string, since: string, head: string): string[] { + const { ref, sha } = writeBase(root, base); + const load = sha ? loadPolicyAt(root, sha, ref) : null; + if (!sha || !load || isUnavailable(load) || load.errors.length) return []; + const was = versionAtRef(root, from), now = versionAtRef(root, since); + if (!('version' in was) || !('version' in now)) return []; + const gated = (load.policy.changelog && baseMergedChangelogCommit(root, load.policy.changelog, sha, from, head)) || from; + const d = spawnSync('git', ['diff', '--unified=0', '--no-color', '--no-ext-diff', '--no-renames', gated, since], { cwd: root, encoding: 'utf8', timeout: 60_000, maxBuffer: 256 * 1024 * 1024 }); + if (d.status !== 0) return []; + return classifyFiles(root, parseUnifiedDiff(d.stdout.replace(/\n$/, '')), gated, since, load.policy, now.rel, was.version, now.version, branchSides(root, ref, from, was.version)).filter(f => f.verdict === 'stamp').map(f => f.path); +} + +/** A commit on `from` whose CHANGELOG is `from`'s merged with the base's at `head`'s merge base; null when the base did not change it since `from`'s merge base or the pair does not merge. */ +function baseMergedChangelogCommit(root: string, rel: string, baseSha: 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 mbFrom = git(root, ['merge-base', baseSha, from]), mbHead = git(root, ['merge-base', baseSha, head]); + if (!mbFrom.ok || !mbHead.ok || mbFrom.out === mbHead.out) return null; + const ours = run(['show', `${from}:${rel}`]), theirs = run(['show', `${mbHead.out}:${rel}`]); + if (ours === null || theirs === null || theirs === run(['show', `${mbFrom.out}:${rel}`])) return null; + const merged = mergeChangelogs(ours, theirs); + const gitDir = git(root, ['rev-parse', '--absolute-git-dir']); + if (merged === null || !gitDir.ok) return null; + const index = path.join(gitDir.out, `gstack-changelog-${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 mode = run(['--literal-pathspecs', 'ls-tree', from, '--', rel])?.split(' ')[0]; + const blob = run(['hash-object', '-w', '--stdin'], undefined, merged)?.trim(); + if (!mode || !blob || run(['read-tree', from], env) === null || run(['update-index', '--cacheinfo', `${mode},${blob},${rel}`], env) === 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 CHANGELOG with the base merged'], env)?.trim()) || null; + } finally { fs.rmSync(index, { force: true }); } } /** Words that make a docs line a money, security, secret or auth claim for plainDocsSince: deliberately broad, since a false hit only reruns the reader. */ const DOCS_CLAIM_WORDS = new Set([...MONEY_PATH_TOKENS, ...AUTH_PATH_TOKENS, 'cap', 'caps', 'capped', 'usd', 'dollar', 'dollars', 'paid', 'pay', 'payment', 'payments', 'refund', 'refunds', diff --git a/test/review-targeted-stamp-carry.test.ts b/test/review-targeted-stamp-carry.test.ts new file mode 100644 index 000000000..0e8da18b1 --- /dev/null +++ b/test/review-targeted-stamp-carry.test.ts @@ -0,0 +1,150 @@ +/** + * GST-1689 (GBRA-345, gbrain #8727): gstack-ship-receipt write and gstack-review-plan classify the change after a targeted + * read with one classifier (docsChangeSince). A /ship restamp (VERSION, manifests, CHANGELOG re-heading, stamp_patterns lines) + * plus a plain docs-audit edit carries the record at write, and the plan agrees with NEXT: done; a code change drops it, and a + * docs edit that adds a numeric claim on a money-class PR drops it with the plan dispatching the one targeted reader. + */ +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 { checkoutTree } from '../lib/review-brief'; +import { BASE_POLICY, cleanup, makeFixtureRepo, mergeMain, restamp, runBin, type FixtureRepo } from './helpers/restamp-fixture'; + +setDefaultTimeout(180_000); +const repos: FixtureRepo[] = []; +afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); + +const commit = (repo: FixtureRepo, files: Record, msg: string) => { + for (const [f, t] of Object.entries(files)) { fs.mkdirSync(path.dirname(path.join(repo.work, f)), { recursive: true }); fs.writeFileSync(path.join(repo.work, f), t); } + repo.git('add', '-A'); repo.git('commit', '-q', '-m', msg); + return repo.git('rev-parse', 'HEAD'); +}; + +/** + * A money-class branch with an [Unreleased] CHANGELOG entry and a stamp_patterns line in a code file, read clean by the + * targeted reader through the real dispatch steps: the commit it read, the record verify wrote and the targeted patch-id. + */ +function readBranch(): { repo: FixtureRepo; read: string; record: string; ran: string } { + const repo = makeFixtureRepo({ prefix: 'gst1689-', policy: { ...BASE_POLICY, stamp_paths: ['docs/**/*.md', 'src/meta.ts'], stamp_patterns: ["CHARGE_SINCE = '{version}'"] } }); + repos.push(repo); + repo.git('checkout', '-q', '-b', 'feat'); + const changelog = fs.readFileSync(path.join(repo.work, 'CHANGELOG.md'), 'utf8').replace('# Changelog\n', '# Changelog\n\n## [Unreleased]\n\n- Adds a per-run spend cap.\n'); + const read = commit(repo, { 'src/charge.ts': 'export const spend_cap_usd = 5;\n', 'src/meta.ts': "export const CHARGE_SINCE = '1.2.4.0';\n", 'CHANGELOG.md': changelog }, 'charge'); + const out = path.join(repo.root, 'read'); + const ledger = path.join(out, 'review-ledger.jsonl'); + const render = runBin(repo, 'gstack-review-dispatch', ['render', '--role', 'targeted', '--classes', 'money', '--diff-base', repo.git('merge-base', 'origin/main', 'HEAD'), '--out', out]); + expect(render.status, render.err).toBe(0); + expect(runBin(repo, 'gstack-review-dispatch', ['sent', '--ledger', ledger, '--role', 'targeted', '--sent', /file=(\S+)/.exec(render.out)![1]!]).status).toBe(0); + fs.writeFileSync(path.join(out, 'targeted.out'), `REVIEWED_TREE: ${checkoutTree(repo.work)}\nNO FINDINGS\n`); + expect(runBin(repo, 'gstack-review-dispatch', ['collect', '--ledger', ledger, '--role', 'targeted', '--output', path.join(out, 'targeted.out'), '--merged', path.join(out, 'merged.jsonl')]).status).toBe(0); + fs.writeFileSync(path.join(out, 'judged.jsonl'), ''); + expect(runBin(repo, 'gstack-review-dispatch', ['judge', '--ledger', ledger, '--merged', path.join(out, 'merged.jsonl'), '--judged', path.join(out, 'judged.jsonl')]).status).toBe(0); + const record = path.join(repo.root, 'targeted.json'); + const v = runBin(repo, 'gstack-review-dispatch', ['verify', '--ledger', ledger, '--targeted-out', record]); + expect(v.out).toContain('TARGETED_RECORD: classes=money status=done findings=0 '); + const plan = JSON.parse(runBin(repo, 'gstack-review-plan', ['--base', 'main', '--gate-tagged', '0', '--json']).out); + expect(plan.targeted.classes).toEqual(['money']); + return { repo, read, record, ran: plan.targeted.targeted_patch_id }; +} + +/** /ship Step 12's restamp to 1.2.5.0, committed: VERSION, manifests, the CHANGELOG re-heading and the stamp_patterns line. */ +function restampTo(repo: FixtureRepo, version = '1.2.5.0'): void { + const r = restamp(repo, ['--version', version]); + expect(r.status, r.err).toBe(EXIT.ok); + repo.git('add', '-A'); repo.git('commit', '-q', '-m', `restamp ${version}`); + expect(fs.readFileSync(path.join(repo.work, 'src', 'meta.ts'), 'utf8')).toBe(`export const CHARGE_SINCE = '${version}';\n`); +} + +const write = (repo: FixtureRepo, record: string) => runBin(repo, 'gstack-ship-receipt', ['write', '--base', 'main', '--session-kind', 'unattended', '--review-targeted', `@${record}`, '--json']); +/** The plan after a clean fix-check, with the documented recovery flags: the commit the reader read and its targeted patch-id. */ +const planAfter = (repo: FixtureRepo, read: string, ran: string) => runBin(repo, 'gstack-review-plan', ['--base', 'main', '--gate-tagged', '0', '--pass', '1', '--last', 'fix-check', '--fixed-below', '--range-base', read, '--targeted-ran', ran]); +const dispatches = (out: string) => out.split('\n').filter(l => l.startsWith('DISPATCH: ')); + +describe('GST-1689: the receipt and the review plan agree on what carries a targeted record', () => { + // Value: protects=a restamp (VERSION, package.json, lockfile, CHANGELOG re-heading, a stamp_patterns line in a code file) plus a + // plain docs-audit edit after a clean targeted read carries the record at write, as the plan already said (GBRA-345: write refused, + // the plan printed NEXT: done, and only a 3-reader third round could finish); fails_when=write refuses the record or the plan + // dispatches a reader; seam=gstack-ship-receipt write --review-targeted and gstack-review-plan --range-base --targeted-ran; + // no_claim=what the reader finds + test('restamp plus a plain docs-audit edit: write binds the record and the plan is done', () => { + const { repo, read, record, ran } = readBranch(); + restampTo(repo); + commit(repo, { 'docs/run-order.md': '# Run order\n\nRead the overview first, then the walkthrough.\n' }, 'Step 14.5 docs audit'); + const w = write(repo, record); + expect(w.status, w.err).toBe(EXIT.ok); + expect(JSON.parse(w.out).review_targeted).toMatchObject({ classes: ['money'], status: 'done' }); + const p = planAfter(repo, read, ran); + expect(p.status, p.err).toBe(0); + expect(p.out).toContain('NEXT: done'); + expect(dispatches(p.out)).toEqual([]); + expect(p.out).toContain('is plain docs and release stamps, which the targeted record carries'); + }); + + // Value: protects=a restamp alone after the read is release stamps, which carry the record, with the plan's stamps-only reason; + // fails_when=an empty docs remainder stops counting as plain; seam=gstack-ship-receipt write and the plan's skip reason + test('restamp alone: write binds the record and the plan is done', () => { + const { repo, read, record, ran } = readBranch(); + restampTo(repo); + const w = write(repo, record); + expect(w.status, w.err).toBe(EXIT.ok); + expect(JSON.parse(w.out).review_targeted).toMatchObject({ classes: ['money'], status: 'done' }); + const p = planAfter(repo, read, ran); + expect(p.out).toContain('NEXT: done'); + expect(dispatches(p.out)).toEqual([]); + expect(p.out).toContain('is release stamps, which the targeted record carries'); + }); + + // Value: protects=a slot restamp after a base merge brought a sibling release whose CHANGELOG entry has a numeric line still + // carries the record: main's released section is not the PR's docs change; fails_when=the CHANGELOG is classified against the + // pre-merge read, so write refuses and the plan dispatches the reader; seam=gstack-ship-receipt write and gstack-review-plan + test('base merge with a sibling release, then restamp: write binds the record and the plan is done', () => { + const { repo, read, record, ran } = readBranch(); + repo.git('checkout', '-q', 'main'); + const cl = fs.readFileSync(path.join(repo.work, 'CHANGELOG.md'), 'utf8').replace('# Changelog\n', '# Changelog\n\n## [1.2.5.0] - 2026-10-20\n\n- Cuts startup time by 40%.\n'); + commit(repo, { 'CHANGELOG.md': cl, VERSION: '1.2.5.0\n' }, 'sibling release'); + repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-q', 'feat'); + mergeMain(repo); + restampTo(repo, '1.2.6.0'); + const w = write(repo, record); + expect(w.status, w.err).toBe(EXIT.ok); + expect(JSON.parse(w.out).review_targeted).toMatchObject({ classes: ['money'], status: 'done' }); + const p = planAfter(repo, read, ran); + expect(p.out).toContain('NEXT: done'); + expect(dispatches(p.out)).toEqual([]); + expect(p.out).toContain('is release stamps, which the targeted record carries'); + }); + + // Value: protects=a one-line code change after the read still drops the record, release stamps beside it or not; fails_when=the + // stamp carry swallows a code edit; seam=gstack-ship-receipt write --review-targeted and the plan's skip reason + test('restamp plus a one-line code change: write refuses and the plan sends it to the review loop', () => { + const { repo, read, record, ran } = readBranch(); + restampTo(repo); + commit(repo, { 'src/charge.ts': 'export const spend_cap_usd = 50;\n' }, 'code after the read'); + const w = write(repo, record); + expect(w.status).toBe(EXIT.fail); + expect(w.err).toContain(`the targeted reader read ${read.slice(0, 12)}`); + const p = planAfter(repo, read, ran); + expect(p.status, p.err).toBe(0); + expect(dispatches(p.out)).toEqual([]); + expect(p.out).toContain('is not docs-only (src/charge.ts), so it takes the review loop'); + }); + + // Value: protects=on a money-class PR a docs edit that adds a numeric claim still drops the record as strictly as before, and the + // plan dispatches the one targeted reader the receipt asks for rather than printing done; fails_when=the stamp carry lets a + // claim through, or the plan and the receipt disagree; seam=gstack-ship-receipt write --review-targeted and gstack-review-plan + test('restamp plus a docs edit with a numeric claim on a money-class PR: write refuses and the plan dispatches the targeted reader', () => { + const { repo, read, record, ran } = readBranch(); + restampTo(repo); + commit(repo, { 'docs/run-order.md': '# Run order\n\nEach run now finishes 40% faster.\n' }, 'docs claim'); + const w = write(repo, record); + expect(w.status).toBe(EXIT.fail); + expect(w.err).toContain(`the targeted reader read ${read.slice(0, 12)}`); + const p = planAfter(repo, read, ran); + expect(p.status, p.err).toBe(0); + expect(p.out).toContain('NEXT: targeted'); + expect(dispatches(p.out)).toEqual(['DISPATCH: targeted targeted effort=full classes=money']); + expect(p.out).toContain(`TARGETED_DELTA: range_base=${read} paths=docs/run-order.md classes=money`); + }); +});