From 1e4a85e4b58de73a26229225f4785b1034bf33ce Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 07:21:34 +0000 Subject: [PATCH] fix(ship): base merges, adjacent base edits, test-only and audit-requested fixes no longer force a second review loop (GST-407, GST-384, GST-391) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-docs-candidate | 33 ++- bin/gstack-doctor-components.sh | 1 + bin/gstack-ship-receipt | 36 ++- changelog.d/gst-407-ship-rereview-triggers.md | 10 + docs/ship-policy.md | 3 +- docs/troubleshooting.md | 20 ++ lib/result-codes-release.ts | 5 + lib/ship-patch.ts | 163 +++++++++++ lib/ship-receipt.ts | 12 +- lib/ship-release.ts | 109 +++++-- ship/DRIVER.md | 16 +- ship/DRIVER.md.tmpl | 16 +- ship/SKILL.md | 4 +- ship/SKILL.md.tmpl | 4 +- ship/sections/documentation.md | 4 +- ship/sections/documentation.md.tmpl | 4 +- test/ship-rereview-triggers.test.ts | 272 ++++++++++++++++++ .../ship/gst-407-ship-rereview-triggers.json | 4 + 18 files changed, 659 insertions(+), 57 deletions(-) create mode 100644 changelog.d/gst-407-ship-rereview-triggers.md create mode 100644 lib/ship-patch.ts create mode 100644 test/ship-rereview-triggers.test.ts create mode 100644 test/size-ledger/ship/gst-407-ship-rereview-triggers.json diff --git a/bin/gstack-docs-candidate b/bin/gstack-docs-candidate index 7208f13a4..511372c17 100755 --- a/bin/gstack-docs-candidate +++ b/bin/gstack-docs-candidate @@ -6,6 +6,7 @@ * gstack-docs-candidate snapshot --out --audit-id --mode edit|read-only * --base [--select ]... [--docs ]... * gstack-docs-candidate compare + * gstack-docs-candidate attempt --state * gstack-docs-candidate [snapshot|compare] --help * * snapshot saves, NUL-safely and without writing to the repository: base SHA, @@ -20,6 +21,13 @@ * the index changed, which recorded paths' content changed or disappeared, and * which paths are newly dirty or untracked. It never decides whether a change * was permitted; the caller checks those paths against the child's report. + * + * attempt reads one audit child's output and says whether the launch counts + * against Step 14.5's attempt budget. A launch the child's install check refused + * (an `INSTALL: fail` row, its last GSTACK_RESULT line a document-release + * refusal, and no audit JSON) never audited, so it is not counted, once per + * invocation (--state, the invocation's record); a second refusal is counted + * and stops for the install fix (GST-391, gbrain #8084). */ import { spawnSync } from 'child_process'; import * as fs from 'fs'; @@ -83,7 +91,8 @@ function one(args: string[], name: string): string { } const USAGE = `usage: gstack-docs-candidate snapshot --out --audit-id --mode edit|read-only --base [--select ]... [--docs ]... - gstack-docs-candidate compare `; + gstack-docs-candidate compare + gstack-docs-candidate attempt --state `; /** Each field compare needs that the record lacks or types wrongly; empty for a valid record. */ function recordProblems(record: any): string[] { @@ -98,7 +107,7 @@ function recordProblems(record: any): string[] { function main(argv: string[]): void { const [command, ...args] = argv; const topHelp = (command === '--help' || command === '-h') && !args.length; - const commandHelp = (command === 'snapshot' || command === 'compare') && args.length === 1 && args[0] === '--help'; + const commandHelp = (command === 'snapshot' || command === 'compare' || command === 'attempt') && args.length === 1 && args[0] === '--help'; if (topHelp || commandHelp) { console.log(USAGE); return; @@ -160,6 +169,26 @@ function main(argv: string[]): void { })); return; } + if (command === 'attempt') { + const [file, ...rest] = args; + if (!file || file.startsWith('--')) throw new Error('attempt takes the child output file, then --state '); + const state = one(rest, '--state'); + const lines = fs.readFileSync(file, 'utf8').split('\n').map(l => l.trim()).filter(Boolean); + const last = lines.filter(l => l.startsWith('GSTACK_RESULT: ')).pop(); + const install = lines.find(l => /^INSTALL: fail\b/.test(l)); + const audited = lines.some(l => { try { const v = JSON.parse(l); return !!v && typeof v === 'object' && 'schema_version' in v; } catch { return false; } }); + if (!last || !/^GSTACK_RESULT: skill=document-release status=refused\b/.test(last) || !install || audited) { console.log('ATTEMPT: counted'); return; } + let seen = 0; + try { seen = Number(JSON.parse(fs.readFileSync(state, 'utf8')).not_counted) || 0; } catch { seen = 0; } + if (seen >= 1) { + console.log('ATTEMPT: counted (a second install refusal in this invocation)'); + console.log(`STOP: run the install fix (${install}) and confirm its gstack: ok trailer before another launch`); + process.exit(3); + } + fs.writeFileSync(state, JSON.stringify({ not_counted: seen + 1 }) + '\n', { mode: 0o600 }); + console.log(`ATTEMPT: not-counted (install refused: ${install})`); + return; + } throw new Error(USAGE); } diff --git a/bin/gstack-doctor-components.sh b/bin/gstack-doctor-components.sh index bc777e96e..a69aefeb2 100644 --- a/bin/gstack-doctor-components.sh +++ b/bin/gstack-doctor-components.sh @@ -312,6 +312,7 @@ gstack_result_anchor() { REVIEW_LOOP_EXCEPTION_INVALID) printf '%s' 'review-loop-exception-invalid' ;; RECEIPT_CARRY_TREE_MISMATCH) printf '%s' 'receipt-carry-tree-mismatch' ;; RECEIPT_CARRY_PRODUCT_CHANGED) printf '%s' 'receipt-carry-product-changed' ;; + RECEIPT_CARRY_SCOPED_REVIEW) printf '%s' 'receipt-carry-scoped-review' ;; DOCS_AUDIT_BLOCKED) printf '%s' 'docs-audit-blocked' ;; RECEIPT_PROSE_MISMATCH) printf '%s' 'receipt-prose-mismatch' ;; RECEIPT_CARRY_PREGATE_STALE) printf '%s' 'receipt-carry-pregate-stale' ;; diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 59c64628e..11c1d9866 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -44,6 +44,7 @@ import { receiptIssuesFrom, receiptSummaryLine, renderCensus, renderReceiptBlock, validateReceipt, type CensusPr, } from '../lib/ship-receipt'; import { checkDocsCaps, docsCapLine, docsExceptionLine, parseReceiptDocs } from '../lib/ship-receipt-docs'; +import { comparePath, contextPacket, readScopedReview, touchedPaths } from '../lib/ship-patch'; import { deeper, depthBelow, exceptionProblems, parseReviewLoopException, receiptDepthAtHead, reviewLoopExceptionLine } from '../lib/ship-receipt-depth'; import { checkReceiptProse, proseLine } from '../lib/ship-receipt-prose'; import { resolveStateRoot } from '../lib/state-root'; @@ -488,7 +489,7 @@ function cmdCarry(rest: string[], json: boolean): number { if (!repoRoot) { process.stderr.write('gstack-ship-receipt carry: run it inside the git checkout whose head carries the receipt\n'); return EXIT.usage; } const head = (flag(rest, '--head') ?? git(['rev-parse', 'HEAD'], repoRoot))?.toLowerCase(); if (!head || !/^[0-9a-f]{40,64}$/.test(head)) { process.stderr.write('gstack-ship-receipt carry: cannot resolve HEAD\n'); return EXIT.usage; } - const refused = (code: 'QUEUE_RECEIPT_MISSING' | 'RECEIPT_CARRY_REFUSED' | 'RECEIPT_CARRY_PRODUCT_CHANGED' | 'RECEIPT_CARRY_TREE_MISMATCH' | 'RECEIPT_GIT_TOO_OLD' | 'RECEIPT_CARRY_PREGATE_STALE', message: string) => { + const refused = (code: 'QUEUE_RECEIPT_MISSING' | 'RECEIPT_CARRY_REFUSED' | 'RECEIPT_CARRY_PRODUCT_CHANGED' | 'RECEIPT_CARRY_SCOPED_REVIEW' | 'RECEIPT_CARRY_TREE_MISMATCH' | 'RECEIPT_GIT_TOO_OLD' | 'RECEIPT_CARRY_PREGATE_STALE', message: string) => { process.stderr.write(resultLineWithDocs('gstack-ship-receipt carry', code, message) + '\n'); process.stdout.write(json ? JSON.stringify({ skill: 'ship', status: 'refused', run: '-', code, message }) + '\n' : `CARRY: refused (${code})\nGSTACK_RESULT: skill=ship status=refused run=-\n`); return EXIT.refused; @@ -582,25 +583,44 @@ function cmdCarry(rest: string[], json: boolean): number { } const current = retarget ? headPatch : productHead; const changedProduct: string[] = []; + const contextPaths: string[] = []; if (retarget && reviewed !== current) changedProduct.push(`HEAD's product diff against ${base} is not the one reviewed against ${reviewedBase} (the parent has not fully landed on ${base}, or the branch changed since)`); else if (productFrom !== productHead && !retarget) { - // planCarry names at most CARRY_NAMED_PATHS differing paths, so the per-path comparison (two patch-ids each) stops there, - // and it walks 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. - const touched = new Set([...(git(['diff', '--name-only', fromBase, from], repoRoot) ?? '').split('\n'), ...(git(['diff', '--name-only', headBase, head], repoRoot) ?? '').split('\n')].filter(Boolean)); + // 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 + // (lib/ship-patch.ts) needs a scoped review instead of the loop. + const sides = { reviewedBase: fromBase, reviewed: from, currentBase: headBase, current: head }; + const touched = new Set([...(touchedPaths(repoRoot, fromBase, from) ?? []), ...(touchedPaths(repoRoot, headBase, head) ?? [])]); for (const p of [...touched].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; - if (asId(patchId(repoRoot, fromBase, from, [p])) !== asId(patchId(repoRoot, headBase, head, [p]))) changedProduct.push(p); + const verdict = comparePath(repoRoot, sides, p); + if (verdict === 'changed') changedProduct.push(p); + else if (verdict === 'context') contextPaths.push(p); } - if (!changedProduct.length) changedProduct.push('(a product path outside the branch\'s touched files; compare the two base diffs)'); + if (!changedProduct.length && !contextPaths.length) changedProduct.push('(a product path outside the branch\'s touched files; compare the two base diffs)'); + } + let scopedReview: string | undefined; + if (!changedProduct.length && contextPaths.length) { + const record = flag(rest, '--scoped-review'); + if (!record) { + const gitPath = git(['rev-parse', '--git-path', `gstack/scoped-review-${head.slice(0, 12)}.md`], repoRoot); + const packet = path.resolve(repoRoot, gitPath ?? `.git/gstack/scoped-review-${head.slice(0, 12)}.md`); + fs.mkdirSync(path.dirname(packet), { recursive: true }); + fs.writeFileSync(packet, contextPacket(repoRoot, { reviewedBase: fromBase, reviewed: from, currentBase: headBase, current: head }, contextPaths)); + return refused('RECEIPT_CARRY_SCOPED_REVIEW', `base context moved next to the reviewed hunks in ${contextPaths.join(', ')}: one fix-check reviewer reads ${packet}, then rerun carry with --scoped-review ({"head":"${head}","paths":[…],"reviewer":"","verdict":"clean"}); a blocking finding means /ship on ${head.slice(0, 12)}`); + } + const review = readScopedReview(record, { head }, contextPaths); + if (!review.ok) return refused('RECEIPT_CARRY_REFUSED', `--scoped-review: ${review.message}`); + scopedReview = `base context moved (scoped review by ${review.review.reviewer}: ${contextPaths.join(', ')})`; } const mergeCommits = (git(['rev-list', '--merges', '--count', `${from}..${head}`], repoRoot) ?? '0').trim(); // Evidence for the record, never the decision: a slow range-diff is reported unavailable rather than waited for. const rangeRaw = spawnSync('git', ['range-diff', '--no-color', `${fromBase}..${from}`, `${headBase}..${head}`], { cwd: repoRoot, encoding: 'utf8', timeout: 15_000, maxBuffer: 64 * 1024 * 1024 }); const rangeDiff = rangeRaw.status === 0 ? (rangeRaw.stdout.trim().split('\n').slice(0, 20).join('\n') || '(empty: identical ranges)') : `unavailable: ${(rangeRaw.stderr ?? '').trim().split('\n')[0] || (rangeRaw.signal ? `range-diff killed by ${rangeRaw.signal} after 15 s` : 'range-diff failed')}`; - plan = planCarry({ from, head, changed, productPatchId: { from: reviewed as string, head: current as string }, testPatchId: { from: testFrom, head: testHead }, changedProduct, mergeCommits: Number(mergeCommits) || 0, rangeDiff, ...(retarget ? { retargetedFrom: reviewedBase } : {}) }); + plan = planCarry({ from, head, changed, productPatchId: { from: reviewed as string, head: current as string }, testPatchId: { from: testFrom, head: testHead }, changedProduct, mergeCommits: Number(mergeCommits) || 0, rangeDiff, ...(retarget ? { retargetedFrom: reviewedBase } : {}), ...(scopedReview ? { scopedReview } : {}) }); } if (!plan.ok) return refused(plan.code, plan.message); // Eligibility is not publication: a receipt that recorded a pregate is rebound only with a pregate current for HEAD, diff --git a/changelog.d/gst-407-ship-rereview-triggers.md b/changelog.d/gst-407-ship-rereview-triggers.md new file mode 100644 index 000000000..54b69d459 --- /dev/null +++ b/changelog.d/gst-407-ship-rereview-triggers.md @@ -0,0 +1,10 @@ +**/ship no longer pays for a second review loop when only main moved, a test file changed, or the docs audit's own requested fix landed.** Each of these used to count as a product change: a base merge after review (`ROUTE: … behavior=86` on gbrain #7739), main rewording the line next to a reviewed hunk (#3631, founderstar#441), a test-only fix (gbrain #8084), and a one-line index link the audit asked for (gbrain #8087). Each one cost another ~$19 loop or an owner's documentation exception. A real product change still sends the PR back through the full loop (GST-407, GST-384, GST-391). + +### Changed +- **`gstack-ship-release finish --reviewed-base `** (the merge base /ship now saves with the reviewed tree) compares the branch's own patch per path. Paths only the base moved count as `base=`. Test code files with no gate edit (no skipped or removed test, snapshot or relaxed gate value) count as `tests=`, and the lanes rerun them. Without the flag the route is unchanged and prints `base=unknown`. +- **Adjacent base edits get a scoped review, not the full loop.** When the branch's +/- lines are the reviewed ones in the same places and only context lines the base edited differ, `finish` stops with `context=N` and `gstack-ship-receipt carry` with `RECEIPT_CARRY_SCOPED_REVIEW`. Both write a packet with just those hunks. After one fix-check reviewer comes back clean, `--context-reviewed ` or `carry --scoped-review ` continues, and the carry reason names the reviewer. The same lines moved somewhere else are still a product change. +- **Docs audit freshness** ignores test code and paths only the base moved. `--docs-requested --docs-result ` carries a fix the audit's own result asked for when its blockers or decisions name that exact path, the edit is at most 5 lines, and the doc caps still pass. +- **`gstack-docs-candidate attempt --state `** says whether an audit launch counts against Step 14.5's budget. A launch the child's install check refused is not counted, once per invocation. A second refusal stops for the install fix. + +### Tests +- `test/ship-rereview-triggers.test.ts` reproduces each trigger and runs the negative controls: a product edit, a changed conflict resolution, the same line moved to another place (with and without a base change), a skipped test, and product files named `café.ts` and `[id].ts`. diff --git a/docs/ship-policy.md b/docs/ship-policy.md index 0cf827ca5..7bc9019ec 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1046,7 +1046,8 @@ receipt when the base moves: | What changed since /ship | The receipt | |---|---| | a merge of the base, product diff unchanged (tests, release metadata, registered outputs the pregate's regen check proved) | carries; no review reruns | -| a merge of the base whose resolution changed product code | a fresh /ship (`RECEIPT_CARRY_PRODUCT_CHANGED`) | +| a merge of the base that edited lines next to the branch's unchanged hunks (the branch's +/- lines identical and in the same places) | carries after one fix-check reviewer reads the hunks and comes back clean (`RECEIPT_CARRY_SCOPED_REVIEW`, then `carry --scoped-review `) | +| a merge of the base whose resolution changed product code, or moved the branch's lines elsewhere | a fresh /ship (`RECEIPT_CARRY_PRODUCT_CHANGED`) | | a rebase or force-push onto a tree the receipt covers, product diff against the new base unchanged | carries (`carry.ancestry: rewritten`) | | any other rebase or force-push | a fresh /ship (`RECEIPT_CARRY_TREE_MISMATCH`) | | a stacked PR whose parent landed (squash or merge), retargeted to the base with the base merged in, HEAD's product diff exactly the one reviewed against the parent branch | carries (reason `stack parent landed`) | diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index e5119eb6c..5a8c0550d 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -5050,6 +5050,26 @@ landed on the base, which the `carry` object in the carried receipt records with the product patch-id and the range-diff evidence ([unattended.md](unattended.md#the-terminal-line)). + +### `RECEIPT_CARRY_SCOPED_REVIEW` + +**Meaning.** Every product path the branch touches has the reviewed +/- lines +in the same places, but the base edited context lines next to some of them, so +the 3-line patches differ (GST-384: main reworded the line above the PR's +hunk). The merge may have spliced the two, as in a JSON entry where the branch +changed one key and the base the next, so the receipt does not carry silently. +The refusal names the paths and a packet file with the hunks that differ and +the base's own change. The same lines moved somewhere else are +`RECEIPT_CARRY_PRODUCT_CHANGED`, not this code. + +**Fix.** Dispatch one fix-check reviewer +([review-policy.md](review-policy.md)) on the packet. When it comes back clean, +write `{"head":"","paths":[...],"reviewer":"","verdict":"clean"}` +and rerun `gstack-ship-receipt carry ... --scoped-review `; the +carry reason records the reviewer and the paths. A blocking finding means /ship +on the new head. `gstack-ship-release finish` stops the same way with +`context=N` and takes `--context-reviewed ` bound to the tree. + ### `RECEIPT_PROSE_MISMATCH` diff --git a/lib/result-codes-release.ts b/lib/result-codes-release.ts index 96a2f66ea..6c18c3e3e 100644 --- a/lib/result-codes-release.ts +++ b/lib/result-codes-release.ts @@ -284,6 +284,11 @@ export const RELEASE_RESULT_CODES = { summary: 'the product diff against the base changed between the receipt\'s head and HEAD (a product file differs beyond a base merge, a test-only fix, release metadata or a regenerated output), so the reviewed verdict does not cover HEAD', fix: 'run /ship on the new head; the receipt carries only when every changed path is a test, release metadata, a registered output the pregate regen check proved, or the base merge, and the product patch-id is unchanged', }, + RECEIPT_CARRY_SCOPED_REVIEW: { + anchor: 'receipt-carry-scoped-review', + summary: 'the branch\'s own +/- lines are the reviewed ones, but the base edited context next to them, so the merge may have spliced the two (GST-384); the receipt carries only after a scoped review of those hunks', + fix: 'dispatch one fix-check reviewer on the packet the refusal names, write its clean verdict as { head, paths, reviewer, verdict } and rerun carry with --scoped-review ; a blocking finding means /ship on the new head', + }, DOCS_AUDIT_BLOCKED: { anchor: 'docs-audit-blocked', summary: 'the documentation audit (/ship Step 14.5) is blocked: gstack-ship-receipt docs-caps found a declared doc cap over, or gstack-ship-receipt write was given a blocked audit with no owner exception and refused to record the ship as audited', diff --git a/lib/ship-patch.ts b/lib/ship-patch.ts new file mode 100644 index 000000000..643f8daac --- /dev/null +++ b/lib/ship-patch.ts @@ -0,0 +1,163 @@ +/** + * ship-patch: whether the branch's own change to a path is the one a review saw, when the base + * moved under it (GST-407, GST-384). /ship's finish route and the receipt carry both ask it. + * + * Each side is a branch patch: reviewedBase -> reviewed (what the review read) and + * currentBase -> current (what ships), where each base is that side's merge base with the + * target branch. A path is + * - same: the 3-line-context patch-ids match (a base merge only shifted line numbers); + * - context: the +/- lines match hunk for hunk, and each hunk sits where the base's own + * change (reviewedBase -> currentBase) puts it, so only context lines the base edited differ. + * The merge may still have spliced the two (a JSON entry where the branch changed one key and + * the base the next), so a context path needs a scoped review of its hunks, never a silent pass; + * - changed: anything else, including the same lines moved to another place, and any path whose + * patches cannot be read (an empty patch on both sides of a path the branch touches). + * Paths reach git as literal pathspecs from NUL-separated listings, so quoted names and glob + * characters cannot widen or empty a comparison. + */ +import { spawnSync } from 'node:child_process'; +import * as fs from 'node:fs'; +import { EMPTY_PROJECT_CONFIG, scanCandidate } from './gate-diff/index'; +import { parseUnifiedDiff } from './gate-diff/git'; +import { CARRY_TEST_CODE_GLOBS } from './ship-receipt'; +import { matchesAny } from './ship-policy'; + +export interface PatchSides { reviewedBase: string; reviewed: string; currentBase: string; current: string } +export type PathVerdict = 'same' | 'context' | 'changed'; + +function git(repoRoot: string, args: string[], input?: string): string | null { + const r = spawnSync('git', ['-c', 'core.quotepath=off', ...args], { cwd: repoRoot, encoding: 'utf8', input, timeout: 120_000, maxBuffer: 256 * 1024 * 1024 }); + return r.status === 0 ? r.stdout : null; +} + +/** Every path the diff from..to touches (renames as a delete and an add); null when git cannot diff them. */ +export function touchedPaths(repoRoot: string, from: string, to: string): Set | null { + const out = git(repoRoot, ['diff', '-z', '--name-only', '--no-renames', from, to]); + return out === null ? null : new Set(out.split('\0').filter(Boolean)); +} + +function pathDiff(repoRoot: string, from: string, to: string, rel: string, context: number): string | null { + return git(repoRoot, ['--literal-pathspecs', 'diff', '--no-color', '--no-ext-diff', '--no-renames', '--binary', '--full-index', `-U${context}`, from, to, '--', rel]); +} + +function patchId(repoRoot: string, diff: string): string | null { + if (!diff.trim()) return ''; + const out = git(repoRoot, ['patch-id', '--verbatim'], diff); + return out === null ? null : out.trim().split(/\s+/)[0] ?? ''; +} + +interface ZeroHunk { oldStart: number; oldCount: number; newStart: number; newCount: number; body: string } + +/** The hunks of a zero-context diff; null when it has none to read (binary, mode-only) but is not empty. */ +function zeroHunks(diff: string): ZeroHunk[] | null { + const hunks: ZeroHunk[] = []; + const lines = diff.split('\n'); + for (let i = 0; i < lines.length; i++) { + const m = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(lines[i]!); + if (!m) continue; + const h: ZeroHunk = { oldStart: Number(m[1]), oldCount: m[2] === undefined ? 1 : Number(m[2]), newStart: Number(m[3]), newCount: m[4] === undefined ? 1 : Number(m[4]), body: '' }; + const body: string[] = []; + let left = h.oldCount + h.newCount; + while (left > 0 || lines[i + 1]?.startsWith('\\')) { + const line = lines[++i]; + if (line === undefined) return null; + if (!line.startsWith('\\')) left--; + body.push(line); + } + h.body = body.join('\n'); + hunks.push(h); + } + return hunks.length || !diff.trim() ? hunks : null; +} + +/** Where a line of the reviewed base sits in the current base, or null when the base changed it. */ +function mapper(base: ZeroHunk[]): (line: number) => number | null { + return line => { + let shift = 0; + for (const h of base) { + if (h.oldCount > 0 && line >= h.oldStart && line < h.oldStart + h.oldCount) return null; + const before = h.oldCount > 0 ? h.oldStart + h.oldCount - 1 < line : h.oldStart < line; + if (before) shift += h.newCount - h.oldCount; + } + return line + shift; + }; +} + +/** Whether a branch hunk at the reviewed base's position lands where the current branch hunk sits. */ +function sameSpot(r: ZeroHunk, c: ZeroHunk, map: (line: number) => number | null): boolean { + if (r.oldCount > 0) { + for (let k = 0; k < r.oldCount; k++) if (map(r.oldStart + k) !== c.oldStart + k) return false; + return true; + } + let below = r.oldStart; + while (below > 0 && map(below) === null) below--; + let above = r.oldStart + 1; + for (let guard = 0; map(above) === null && guard < 100_000; guard++) above++; + const low = below > 0 ? map(below)! : 0; + return c.oldStart >= low && c.oldStart <= map(above)! - 1; +} + +/** The verdict for one path the branch touches on either side (module header). */ +export function comparePath(repoRoot: string, s: PatchSides, rel: string): PathVerdict { + const r3 = pathDiff(repoRoot, s.reviewedBase, s.reviewed, rel, 3); + const c3 = pathDiff(repoRoot, s.currentBase, s.current, rel, 3); + if (r3 === null || c3 === null || (!r3.trim() && !c3.trim())) return 'changed'; + const ids = [patchId(repoRoot, r3), patchId(repoRoot, c3)]; + if (ids[0] === null || ids[1] === null) return 'changed'; + if (ids[0] === ids[1]) return 'same'; + const r0 = zeroHunks(pathDiff(repoRoot, s.reviewedBase, s.reviewed, rel, 0) ?? ''); + const c0 = zeroHunks(pathDiff(repoRoot, s.currentBase, s.current, rel, 0) ?? ''); + const b0 = zeroHunks(pathDiff(repoRoot, s.reviewedBase, s.currentBase, rel, 0) ?? ''); + if (!r0?.length || !c0 || !b0 || r0.length !== c0.length || r0.some((h, i) => h.body !== c0[i]!.body)) return 'changed'; + const map = mapper(b0); + return r0.every((h, i) => sameSpot(h, c0[i]!, map)) ? 'context' : 'changed'; +} + +function hunkTexts(diff: string): string[] { + return diff.split(/^(?=@@ )/m).slice(1).map(h => h.replace(/^@@ [^@]*@@/, '@@')); +} + +/** The scoped-review packet: per context path, the hunks that differ between the two branch patches and the base's own change. */ +export function contextPacket(repoRoot: string, s: PatchSides, paths: readonly string[]): string { + const fence = (label: string, body: string) => `${label}\n\`\`\`diff\n${body.split('\n').slice(0, 200).join('\n').trimEnd()}\n\`\`\`\n`; + const parts = [`# Scoped review: base context moved next to the reviewed hunks\n\nreviewed ${s.reviewed.slice(0, 12)} against ${s.reviewedBase.slice(0, 12)}; current ${s.current.slice(0, 12)} against ${s.currentBase.slice(0, 12)}. The branch's +/- lines are unchanged; read whether the merge kept their meaning next to the base's edit. Report clean, or findings at the blocking floor.\n`]; + for (const rel of paths) { + const r = hunkTexts(pathDiff(repoRoot, s.reviewedBase, s.reviewed, rel, 3) ?? ''); + const c = hunkTexts(pathDiff(repoRoot, s.currentBase, s.current, rel, 3) ?? ''); + parts.push(`## ${rel}\n`, fence('Reviewed hunks', r.filter(h => !c.includes(h)).join('')), fence('Current hunks', c.filter(h => !r.includes(h)).join('')), + fence("The base's change", hunkTexts(pathDiff(repoRoot, s.reviewedBase, s.currentBase, rel, 3) ?? '').join(''))); + } + return parts.join('\n'); +} + +/** A test code file (`*.test.*`, `*_test.go`, …) outside shared helpers and fixtures: a change to it reruns tests, not the review loop. */ +export function isTestCode(rel: string): boolean { + return matchesAny(CARRY_TEST_CODE_GLOBS, rel) && !/(^|\/)(helpers|fixtures|__fixtures__|__snapshots__)\//.test(rel); +} + +/** Test paths whose change from..to carries a gate edit (lib/gate-diff RH tags: a removed or skipped test, a snapshot, a relaxed gate value). */ +export function gateEditedPaths(repoRoot: string, from: string, to: string, paths: readonly string[]): Set | null { + if (!paths.length) return new Set(); + const diff = git(repoRoot, ['--literal-pathspecs', 'diff', '--no-color', '--no-ext-diff', '--no-renames', from, to, '--', ...paths]); + if (diff === null) return null; + const scan = scanCandidate({ files: parseUnifiedDiff(diff), commits: [], project: EMPTY_PROJECT_CONFIG, specIsTestPath: true, candidate: 'route', artifact: '' }); + return new Set(scan.hunks.filter(h => h.tags.length > 0).map(h => h.path)); +} + +export interface ScopedReview { paths: string[]; reviewer: string; verdict: 'clean'; head?: string; tree?: string } + +/** A clean scoped review that covers every context path at this head or tree, or why it does not stand. */ +export function readScopedReview(file: string, bind: { head?: string; tree?: string }, paths: readonly string[]): { ok: true; review: ScopedReview } | { ok: false; message: string } { + let v: any; + try { v = JSON.parse(fs.readFileSync(file, 'utf8')); } catch (e) { return { ok: false, message: `${file} is not readable JSON (${(e as Error).message.split('\n')[0]})` }; } + if (!v || typeof v !== 'object' || Array.isArray(v)) return { ok: false, message: `${file} must be { paths, reviewer, verdict, head|tree }` }; + if (v.verdict !== 'clean') return { ok: false, message: `the scoped review is ${JSON.stringify(v.verdict)}, not clean: fix its findings and run the full loop` }; + if (typeof v.reviewer !== 'string' || !v.reviewer.trim()) return { ok: false, message: 'the scoped review names no reviewer' }; + for (const key of ['head', 'tree'] as const) { + if (bind[key] && v[key] !== bind[key]) return { ok: false, message: `the scoped review is for ${key} ${String(v[key] ?? 'none').slice(0, 12)}, not ${bind[key]!.slice(0, 12)}` }; + } + const covered = Array.isArray(v.paths) ? v.paths.filter((p: unknown) => typeof p === 'string') : []; + const missing = paths.filter(p => !covered.includes(p)); + if (missing.length) return { ok: false, message: `the scoped review does not cover ${missing.slice(0, 5).join(', ')}` }; + return { ok: true, review: { paths: covered, reviewer: v.reviewer.trim(), verdict: 'clean', ...(v.head ? { head: v.head } : {}), ...(v.tree ? { tree: v.tree } : {}) } }; +} diff --git a/lib/ship-receipt.ts b/lib/ship-receipt.ts index 6cc75afa4..392be1103 100644 --- a/lib/ship-receipt.ts +++ b/lib/ship-receipt.ts @@ -145,11 +145,11 @@ export function receiptSummaryLine(r: ShipReceipt): string { * such as `openapi.spec.yaml` is product. */ const TEST_CODE_EXTENSIONS = ['ts', 'tsx', 'js', 'jsx', 'mjs', 'cjs', 'mts', 'cts', 'rb', 'py', 'go', 'rs', 'java', 'kt', 'swift', 'dart', 'php', 'cs']; -export const CARRY_TEST_GLOBS: readonly string[] = [ - 'test/**', 'tests/**', 'spec/**', '__tests__/**', 'e2e/**', 'cypress/**', '**/__tests__/**', +export const CARRY_TEST_CODE_GLOBS: readonly string[] = [ ...TEST_CODE_EXTENSIONS.flatMap(ext => [`**/*.test.${ext}`, `**/*.spec.${ext}`]), '**/*_test.go', '**/test_*.py', '**/*_test.py', '**/*_spec.rb', '**/*Test.java', '**/*Tests.swift', ]; +export const CARRY_TEST_GLOBS: readonly string[] = ['test/**', 'tests/**', 'spec/**', '__tests__/**', 'e2e/**', 'cypress/**', '**/__tests__/**', ...CARRY_TEST_CODE_GLOBS]; export const CARRY_METADATA_GLOBS: readonly string[] = ['VERSION', 'CHANGELOG.md', 'package.json', 'agents-digest/**']; /** How many differing product paths a RECEIPT_CARRY_PRODUCT_CHANGED refusal names, and how far the per-path patch-id walk goes. */ export const CARRY_NAMED_PATHS = 20; @@ -170,6 +170,8 @@ export interface CarryPlanInput { rangeDiff: string; /** The base the receipt was reviewed against when it differs from the carry's (a stack parent that landed, #3570): productPatchId.from is then the recorded diff against it. */ retargetedFrom?: string; + /** A clean scoped review of product paths whose only difference is base context next to unchanged hunks (lib/ship-patch.ts, GST-384): the carry reason it adds. */ + scopedReview?: string; } export interface CarryRecord { from: string; reason: string; product_patch_id: string; changed_paths: string[]; merge_commits: number; range_diff: string; covers: string[]; reruns: string[]; trees?: string[]; ancestry?: 'linear' | 'rewritten' } export type CarryPlan = { ok: true; carry: CarryRecord } | { ok: false; code: 'RECEIPT_CARRY_PRODUCT_CHANGED' | 'RECEIPT_CARRY_REFUSED' | 'RECEIPT_CARRY_TREE_MISMATCH'; message: string }; @@ -231,15 +233,17 @@ export function classifyCarryPath(rel: string, extra: { metadata?: readonly stri * against the base has the same patch-id at both heads, which is what a * conflict-only base merge, a test-only fix or a release-metadata change * leaves behind. A product patch that differs is RECEIPT_CARRY_PRODUCT_CHANGED, - * whatever else changed. + * whatever else changed, unless every difference is base context next to unchanged hunks + * and a scoped review of those hunks came back clean (`scopedReview`). */ export function planCarry(input: CarryPlanInput): CarryPlan { - if (input.productPatchId.from !== input.productPatchId.head) { + if (input.productPatchId.from !== input.productPatchId.head && (input.changedProduct.length || !input.scopedReview)) { const named = input.changedProduct.slice(0, CARRY_NAMED_PATHS); return { ok: false, code: 'RECEIPT_CARRY_PRODUCT_CHANGED', message: `product diff changed between ${input.from.slice(0, 12)} and ${input.head.slice(0, 12)}: ${named.join(', ') || 'patch-id differs'}${input.changedProduct.length > named.length ? ` (+${input.changedProduct.length - named.length} more)` : ''}` }; } const reasons: string[] = input.retargetedFrom ? [`stack parent ${input.retargetedFrom} landed`] : []; if (input.mergeCommits > 0) reasons.push(input.mergeCommits === 1 ? 'base merge' : `${input.mergeCommits} base merges`); + if (input.scopedReview && input.productPatchId.from !== input.productPatchId.head) reasons.push(input.scopedReview); if (input.testPatchId.from !== input.testPatchId.head) reasons.push('test-only change'); if (input.changed.some(c => c.class === 'metadata')) reasons.push('release metadata'); if (input.changed.some(c => c.class === 'generated')) reasons.push('regenerated outputs'); diff --git a/lib/ship-release.ts b/lib/ship-release.ts index 9e379303d..28a2b9a94 100644 --- a/lib/ship-release.ts +++ b/lib/ship-release.ts @@ -12,6 +12,7 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; import { classifyPath } from './review-depth'; +import { comparePath, contextPacket, gateEditedPaths, isTestCode, readScopedReview, touchedPaths, type PatchSides } from './ship-patch'; import { resolveStateRoot } from './state-root'; import { bumpVersion, cmpVersion, fmtVersion, parseVersion, versionWidth, type Bump } from './version-source'; import { bin, evidenceExempt, firstLine, flagReader, fragmentPath, gh, git, hostedOrigin, laneLine, lanes, releasePaths, Report, runLanes, sh, shipPolicy, short } from './ship-bin-common'; @@ -21,10 +22,12 @@ export const RELEASE_HELP = `gstack-ship-release — /ship Steps 12 and 15-17 (s version --base [--bump micro|patch|minor|major] [--version ] [--keep-current] [--accept-incomplete] [--why ] [--answer ] Step 12: classify, pick the queue-aware version, write it with --regen-digest --changelog, log the decision; --keep-current (or --version ) keeps VERSION; under bump ask a session no person answers needs --answer before a bump is written - finish --base --reviewed-tree --docs-post [--lane