diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 50c73537b..f3dde839e 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -50,7 +50,7 @@ import { } from '../lib/ship-receipt'; import { checkDocsCaps, docsCapLine, docsExceptionLine, parseReceiptDocs } from '../lib/ship-receipt-docs'; import { comparePath, contextPacket, PACKET_SECTION_CAP, readScopedReview, touchedPaths } from '../lib/ship-patch'; -import { planRecheck, readRecheck, recheckPacket } from '../lib/ship-recheck'; +import { planRecheck, readRecheck, recheckPacket, recheckRecordTemplate } from '../lib/ship-recheck'; import { carryExcludeBinds, deeper, depthBelow, exceptionHead, exceptionLoopProblem, exceptionProblems, parseReviewLoopException, parseReviewTargeted, plainDocsSince, platformReceiptDepth, receiptExclude, receiptReviewAtHead, receiptTargetedAtHead, reviewLoopExceptionLine, targetedClassesSince, writeExcludeBinds } from '../lib/ship-receipt-depth'; import { RISK_CLASSES, type RiskClass } from '../lib/review-risk'; import { checkReceiptProse, proseLine } from '../lib/ship-receipt-prose'; @@ -843,21 +843,24 @@ function cmdCarry(rest: string[], json: boolean): number { const fullRange = rangeRaw.status === 0 && inter?.status === 0 ? `${rangeRaw.stdout.trim()}\n${inter.stdout.trim()}` : null; const numstat = named.length ? spawnSync('git', ['diff', '--numstat', from, head, '--', ...named.map(p => `:(literal)${p}`)], { cwd: repoRoot, encoding: 'utf8', timeout: 30_000, maxBuffer: 16 * 1024 * 1024 }) : null; const binary = numstat?.status !== 0 || (numstat?.stdout ?? '').split('\n').some(l => l.startsWith('-\t-\t')); - const r = planRecheck({ code: 'RECEIPT_CARRY_PRODUCT_CHANGED', ancestor, retarget, rangeDiff: fullRange, unnamedPaths: named.length !== changedProduct.length, legacy: !receipt.review_depth, walkCut, contextPaths: contextPaths.length, binary }); + // Moved base context composes into the same packet, bound to range_base (GST-1561). + const context = contextPaths.length ? contextPacket(repoRoot, { reviewedBase: rangeBase, reviewed: from, currentBase: headBase, current: head }, contextPaths) : undefined; + const bound = context ? { rangeBase, context: context.text } : {}; + const r = planRecheck({ code: 'RECEIPT_CARRY_PRODUCT_CHANGED', ancestor, retarget, rangeDiff: fullRange, unnamedPaths: named.length !== changedProduct.length, legacy: !receipt.review_depth, walkCut, contextPaths: contextPaths.length, context, binary }); const record = flag(rest, '--recheck'); if (!r.ok) process.stderr.write(`RECHECK: not eligible (${r.reason}); run /ship on ${head.slice(0, 12)}\n`); else if (record) { - const read = readRecheck(record, { from, head, sha256: r.sha256 }); + const read = readRecheck(record, { from, head, sha256: r.sha256, ...bound }); if (!read.ok) return refused('RECEIPT_CARRY_REFUSED', `--recheck: ${read.message}`); - recheck = `scoped re-check by ${read.record.reader} (range-diff ${r.lines} lines)`; + recheck = `scoped re-check by ${read.record.reader} (range-diff ${r.lines} lines${context ? `; base context moved in ${contextPaths.join(', ')}; range_base ${rangeBase.slice(0, 12)}` : ''})`; } else { let where = 'not written (dry run)'; if (!rest.includes('--dry-run')) { const packet = path.resolve(repoRoot, git(['rev-parse', '--git-path', `gstack/recheck-${head.slice(0, 12)}.md`], repoRoot) ?? `.git/gstack/recheck-${head.slice(0, 12)}.md`); - try { fs.mkdirSync(path.dirname(packet), { recursive: true }); fs.writeFileSync(packet, recheckPacket({ from, head, plan: r, rangeDiff: fullRange! })); where = packet; } + try { fs.mkdirSync(path.dirname(packet), { recursive: true }); fs.writeFileSync(packet, recheckPacket({ from, head, plan: r, rangeDiff: fullRange!, ...bound })); where = packet; } catch (e) { where = `unwritable: ${(e as Error).message.split('\n')[0]}`; } } - process.stderr.write(`RECHECK: eligible range_diff_lines=${r.lines} range_diff_sha256=${r.sha256} packet=${where}; one reader reads it, then carry --recheck ({"from":"${from}","head":"${head}","range_diff_sha256":"${r.sha256}","reader":"","verdict":"clean"}); a blocking finding means /ship on ${head.slice(0, 12)}\n`); + process.stderr.write(`RECHECK: eligible range_diff_lines=${r.lines} range_diff_sha256=${r.sha256} packet=${where}; one reader reads it, then carry --recheck (${recheckRecordTemplate({ from, head, sha256: r.sha256, ...bound })}); a blocking finding means /ship on ${head.slice(0, 12)}\n`); } } 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 } : {}), ...(stackParent ? { stackParent } : {}), ...(scopedReview ? { scopedReview } : {}), ...(stampedRows.length ? { stampedRows } : {}), ...(recheck ? { recheck } : {}) }); diff --git a/changelog.d/capy-gst-1561-carry-mixed.md b/changelog.d/capy-gst-1561-carry-mixed.md new file mode 100644 index 000000000..0c65627df --- /dev/null +++ b/changelog.d/capy-gst-1561-carry-mixed.md @@ -0,0 +1 @@ +- **A receipt carry with both a changed path and a moved-context path gets one scoped re-check instead of deadlocking.** founderstar#563 merged a base that took `SAVE_VERSION := 83` under the branch's reviewed `:= 84` and also moved a block next to a reviewed hunk. Carry called the constant file changed, J1's re-check refused because context had moved, and the scoped review opened only when nothing had changed, so every rules-number PR needed a fresh /ship. Now a hunk whose added lines are byte-identical and whose removed lines differ only by the base's edit of the lines the branch replaces counts as a context path, so carry and finish put it in the scoped-review packet for one reader and never carry it silently. When real changed paths and context paths come together, carry writes one packet: the range-diff, the changed paths' diff, then the context hunks. The existing 400-line and 64 KB caps count the whole packet, `range_diff_sha256` binds all of it, and the reader's record names `range_base`, which the carry reason logs. Past the caps, carry refuses as before. A changed path alone, or context paths alone, keep their routes with unchanged output, and a branch-side change to an added line still needs review. (GST-1561) diff --git a/docs/ship-policy.md b/docs/ship-policy.md index fe029a4ed..6352f93c5 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1933,12 +1933,39 @@ until one fix-check reviewer clears the packet; a working tree with uncommitted changes is [`RECEIPT_CARRY_REFUSED`](troubleshooting.md#receipt-carry-refused). A `RECEIPT_CARRY_PRODUCT_CHANGED` on linear history against the reviewed base whose range-diff, plus the diff of the changed product paths, is at most 400 -lines and 64 KB, with no binary path, no moved base context and a receipt that +lines and 64 KB, with no binary path and a receipt that records its review depth, also prints `RECHECK: eligible … packet=`: one reader on that packet and `carry --recheck ` rebind the receipt instead of a full second /ship, unless HEAD needs a deeper review depth or another risk class ([scoped re-check](troubleshooting.md#receipt-carry-recheck), J1). +**Changed and context paths together (GST-1561).** When the base also moved +context next to reviewed hunks in other paths, the scoped review and J1 compose +into one bounded re-check instead of refusing each other (founderstar#563 hit +this on every rules-number PR). The one packet holds the range-diff and the +changed paths' diff, then the scoped-review sections of the context paths. The +400-line and 64 KB caps count the whole packet, and `range_diff_sha256` binds +all of it. The record also names `range_base`, the commit the reviewed range +starts from (the fork point, or a landed stack parent), and the carry reason +logs the context paths and that `range_base`. A context section over 200 lines, +or a packet over the caps, prints `RECHECK: not eligible ()` and stays +`RECEIPT_CARRY_PRODUCT_CHANGED`. A changed path alone, or context paths alone, +keep the routes above, and their output is unchanged. + +The same scoped review also covers a hunk whose only difference from the +reviewed one is the base's edit of the lines the branch replaces. The base took +`SAVE_VERSION := 83` while the branch's `:= 84` line, reviewed against `82`, +stays byte-identical. The branch's added lines must match byte for byte, and +the lines just outside the reviewed hunk's removed range must be lines the base +kept, landing just outside the current hunk's range (`lib/ship-patch.ts` +`comparePath`). The path then counts as a context path. Carry and finish put it +in the scoped-review packet, and the reader checks that the branch's value +still holds next to the base's. It never carries without a reader. A hunk whose +added lines changed, that now also removes a line the base never touched, or +whose range holds a line the base added (a base hunk at or next to the replaced +lines that adds more lines than it removes) stays a changed path. A [mechanical retake](#mechanical-retake) does not apply +here, since the branch retook nothing. + **Rewritten history.** `write` records `head_tree` (the reviewed head's tree) and `product_patch_id` (its product patch against the base), and every carry rebinds both to the new head and keeps the earlier trees in `carry.trees`. diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 7ccc0bfa2..27fdf4e05 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -6464,7 +6464,9 @@ the range-diff evidence ([unattended.md](unattended.md#the-terminal-line)). **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 +hunk). A hunk whose added lines are unchanged and whose removed lines differ only +by the base's edit of the lines the branch replaces counts too (GST-1561: the base +took `SAVE_VERSION := 83` under the branch's `:= 84`). 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 @@ -6506,15 +6508,21 @@ against the current one is at most 400 lines (`RECHECK_RANGE_DIFF_CAP` in range_diff_lines=N range_diff_sha256= packet=`: one reader on that range-diff can clear it instead of a full second /ship. A rebased branch (`RECEIPT_CARRY_TREE_MISMATCH`), a retargeted stack, a receipt with no -`review_depth`, more than 20 changed product paths, a binary path, base -context moved next to reviewed hunks, an empty or unreadable range-diff and a +`review_depth`, more than 20 changed product paths, a binary path, a moved +base context section over 200 lines, an empty or unreadable range-diff and a packet over 400 lines or 64 KB print `RECHECK: not eligible ()` instead. +When base context also moved next to reviewed hunks in other paths, their +scoped-review sections join the same packet (GST-1561): the caps count the +whole packet, `range_diff_sha256` binds all of it, and the RECHECK line's record +adds `"range_base"`. **Fix.** Dispatch one reader on the packet (`.git/gstack/recheck-.md`; `--dry-run` writes none). When it comes back clean, write `{"from":"","head":"","range_diff_sha256":"","reader":"","verdict":"clean"}` -and rerun `gstack-ship-receipt carry ... --recheck `. The carry -reason records the reader and the range-diff size. Carry still refuses when +(plus `"range_base"` when the line's record names one; a record without it then +refuses) and rerun `gstack-ship-receipt carry ... --recheck `. The carry +reason records the reader and the range-diff size, and with moved context the +context paths and `range_base`. Carry still refuses when HEAD needs a deeper review depth or a different risk class than the receipt recorded; that, or a blocking finding, means /ship on the new head. In /ship Step 1, `gstack-ship-preflight` prints this as `NEXT: carry route (Step 1.4, J1)`. diff --git a/lib/ship-patch.ts b/lib/ship-patch.ts index 9512d0a26..8b986a0ff 100644 --- a/lib/ship-patch.ts +++ b/lib/ship-patch.ts @@ -9,7 +9,10 @@ * base merge only shifted line numbers; patch-ids ignore positions, so a block moved to an identical * context elsewhere is changed); * - 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. + * change (reviewedBase -> currentBase) puts it, so only context lines the base edited differ; + * or a hunk's added lines match byte for byte and its removed lines are the base's own edit of + * the lines the reviewed hunk replaced (founderstar#563: the base took SAVE_VERSION 83 under the + * branch's 84, GST-1561), which the scoped review reads too. * 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, a file-mode change, and any path whose @@ -122,10 +125,26 @@ export function comparePath(repoRoot: string, s: PatchSides, rel: string): PathV const c0 = zeroHunks(pathDiff(repoRoot, s.currentBase, s.current, rel, 0) ?? ''); if (ids[0] === ids[1] && r0 === null) return 'same'; 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'; + if (!r0?.length || !c0 || !b0 || r0.length !== c0.length) return 'changed'; const map = mapper(b0); - if (!r0.every((h, i) => sameSpot(h, c0[i]!, map))) return 'changed'; - return ids[0] === ids[1] ? 'same' : 'context'; + if (!r0.every((h, i) => h.body === c0[i]!.body ? sameSpot(h, c0[i]!, map) : baseEditOnly(h, c0[i]!, map, b0))) return 'changed'; + return ids[0] === ids[1] && r0.every((h, i) => h.body === c0[i]!.body) ? 'same' : 'context'; +} + +/** + * Whether a hunk whose body changed since review differs only by the base's edit of the lines it replaces: the added + * lines are the reviewed ones byte for byte, and the lines just outside the reviewed hunk's removed range are lines the + * base kept, landing just outside the current hunk's range, so the current removed lines are the base's version of the + * reviewed ones. A hunk that now removes another line, or adds anything else, is not, and neither is one whose range + * holds a line the base added before, inside or after the reviewed lines (a base hunk there that adds more lines than + * it removes): dropping it deletes base code rather than taking the base's edit. + */ +function baseEditOnly(r: ZeroHunk, c: ZeroHunk, map: (line: number) => number | null, base: ZeroHunk[]): boolean { + const added = (h: ZeroHunk) => h.body.split('\n').filter((l, i, all) => l.startsWith('+') || (l.startsWith('\\') && all[i - 1]?.startsWith('+'))).join('\n'); + const start = (h: ZeroHunk) => (h.oldCount > 0 ? h.oldStart : h.oldStart + 1); + const end = start(r) + r.oldCount; + if (base.some(b => b.newCount > b.oldCount && b.oldStart >= start(r) - 1 && b.oldStart < end)) return false; + return r.oldCount > 0 && added(r) === added(c) && map(start(r) - 1) === start(c) - 1 && map(end) === start(c) + c.oldCount; } /** @@ -178,7 +197,7 @@ export function contextPacket(repoRoot: string, s: PatchSides, paths: readonly s const late = Object.keys(bounded).sort(); const sides = `reviewed ${s.reviewed.slice(0, 12)} against ${s.reviewedBase.slice(0, 12)}; current ${s.current.slice(0, 12)} against ${s.currentBase.slice(0, 12)}.`; const parts = [paths.length || !late.length - ? `# Scoped review: base context moved next to the reviewed hunks\n\n${sides} 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` + ? `# Scoped review: base context moved next to the reviewed hunks\n\n${sides} The branch's added lines are unchanged, and a removed line differs only where the base edited the line the branch replaces; read whether the merge kept their meaning next to the base's edit (a version or number the base also took). Report clean, or findings at the blocking floor.\n` : `# Scoped review: edits that landed after the review bound\n\n${sides}\n`]; if (late.length) parts.push(`Bounded edits since review in ${late.map(p => `${p} (${bounded[p]})`).join(', ')}: each landed after the review bound, and its kind was proven from the diff; this one read replaces another review round (docs/review-policy.md#bounded-post-review-edits). Read every line under "Changed since review" as gate code. Audit prose is policy text agents follow: flag any change to what a gate, cap, reviewer, approval or exception does. A lanes edit maps files to tests: flag a glob that marks files covered by tests that do not exercise them. A bookkeeping row is an exemption or a limit: flag a row that moves an exemption onto code it should not cover, or raises a limit without a reason in the diff. Read them through the targeted brief at the end for money, gate, auth and secret wording. Report clean, or findings at the blocking floor.\n`); if (tests.length) parts.push(`Test code changed since review in ${tests.join(', ')}: read whether each test still checks what it did (no weakened or bypassed assertion, early return, new exemption, skip or mock). Report clean, or findings at the blocking floor.\n`); diff --git a/lib/ship-recheck.ts b/lib/ship-recheck.ts index db1a85c43..37a14814e 100644 --- a/lib/ship-recheck.ts +++ b/lib/ship-recheck.ts @@ -8,9 +8,14 @@ * Qualifies: RECEIPT_CARRY_PRODUCT_CHANGED on linear history (the receipt's head is an ancestor of HEAD) against the same base. * Does not qualify: RECEIPT_CARRY_TREE_MISMATCH (rewritten history: the reviewed commits are gone, so there is no range to * diff), a retargeted stack (the reviewed range was against another base), a legacy receipt with no review_depth, more - * changed product paths than the carry walk names, a path it cannot name, a binary path, base context moved next to reviewed hunks, - * an unreadable or empty range-diff, a packet over RECHECK_RANGE_DIFF_CAP lines or RECHECK_PACKET_BYTES bytes, and every RECEIPT_CARRY_REFUSED / RECEIPT_CARRY_PREGATE_STALE (those need their inputs - * repaired, not a review). + * changed product paths than the carry walk names, a path it cannot name, a binary path, base context moved next to reviewed hunks + * without its scoped-review sections (or with one over PACKET_SECTION_CAP), an unreadable or empty range-diff, a packet over + * RECHECK_RANGE_DIFF_CAP lines or RECHECK_PACKET_BYTES bytes, and every RECEIPT_CARRY_REFUSED / RECEIPT_CARRY_PREGATE_STALE (those need + * their inputs repaired, not a review). + * + * Moved base context composes (GST-1561, founderstar#563): the context paths' scoped-review sections (lib/ship-patch.ts contextPacket) + * join the range-diff in the one packet, the caps count both, the verdict binds both, and the record also names range_base, the + * commit the reviewed range starts from. */ import { createHash } from 'node:crypto'; import * as fs from 'node:fs'; @@ -27,8 +32,10 @@ export type RecheckInput = { binary?: boolean; /** The changed-path walk stopped at CARRY_NAMED_PATHS, so later changed product paths would be missing from the packet. */ walkCut?: boolean; - /** Paths whose only difference is base context next to reviewed hunks: they need RECEIPT_CARRY_SCOPED_REVIEW's packet, not this one. */ + /** Paths whose only difference is base context next to reviewed hunks: the packet needs their scoped-review sections (`context`). */ contextPaths?: number; + /** contextPacket's sections for those paths, and every path with a section over PACKET_SECTION_CAP. */ + context?: { text: string; over: readonly string[] }; }; export type RecheckPlan = { ok: true; lines: number; sha256: string } | { ok: false; reason: string }; @@ -43,39 +50,47 @@ export function planRecheck(i: RecheckInput): RecheckPlan { if (i.walkCut) return { ok: false, reason: 'more changed product paths than the walk names, so the packet would miss some' }; if (i.unnamedPaths) return { ok: false, reason: 'a changed product path could not be named, so the packet would miss it' }; if (i.binary) return { ok: false, reason: 'a changed product path is binary, so the packet cannot show its contents' }; - if (i.contextPaths) return { ok: false, reason: 'base context also moved next to reviewed hunks, which needs the scoped review of those hunks' }; + if (i.contextPaths && !i.context) return { ok: false, reason: 'base context also moved next to reviewed hunks, which needs the scoped review of those hunks' }; + if (i.context?.over.length) return { ok: false, reason: `base context next to reviewed hunks in ${i.context.over.join(', ')} is over the scoped review's section cap` }; if (i.rangeDiff === null) return { ok: false, reason: 'git range-diff failed or timed out' }; - const text = i.rangeDiff.trim(); - if (!text) return { ok: false, reason: 'git range-diff is empty, so it shows nothing for a reader to check' }; + if (!i.rangeDiff.trim()) return { ok: false, reason: 'git range-diff is empty, so it shows nothing for a reader to check' }; + const text = packetMaterial(i.rangeDiff, i.context?.text); const lines = text.split('\n').length; - if (lines > RECHECK_RANGE_DIFF_CAP) return { ok: false, reason: `git range-diff is ${lines} lines, over the scoped re-check cap of ${RECHECK_RANGE_DIFF_CAP}` }; + if (lines > RECHECK_RANGE_DIFF_CAP) return { ok: false, reason: `${i.context ? 'the packet' : 'git range-diff'} is ${lines} lines, over the scoped re-check cap of ${RECHECK_RANGE_DIFF_CAP}` }; const bytes = Buffer.byteLength(text); if (bytes > RECHECK_PACKET_BYTES) return { ok: false, reason: `the packet is ${bytes} bytes, over the scoped re-check cap of ${RECHECK_PACKET_BYTES}` }; return { ok: true, lines, sha256: sha(text) }; } -/** The packet the one reader reads: what to judge, the record to write, then the range-diff verbatim. */ -export function recheckPacket(o: { from: string; head: string; plan: Extract; rangeDiff: string }): string { +/** What range_diff_sha256 binds: the range-diff, then any moved base context's scoped-review sections. */ +const packetMaterial = (rangeDiff: string, context?: string) => (context ? `${rangeDiff.trim()}\n\n${context.trim()}` : rangeDiff.trim()); + +/** The clean record a reader writes; range_base only when base context composes into the packet. */ +export const recheckRecordTemplate = (o: { from: string; head: string; sha256: string; rangeBase?: string }) => + JSON.stringify({ from: o.from, head: o.head, ...(o.rangeBase ? { range_base: o.rangeBase } : {}), range_diff_sha256: o.sha256, reader: '', verdict: 'clean' }); + +/** The packet the one reader reads: what to judge, the record to write, then the range-diff verbatim and any moved base context. */ +export function recheckPacket(o: { from: string; head: string; plan: Extract; rangeDiff: string; context?: string; rangeBase?: string }): string { return [ `# Scoped re-check: the product patch changed since the reviewed head\n`, - `reviewed ${o.from}; current ${o.head}. Below is \`git range-diff\` of the reviewed range against the current one, then the diff of the changed product paths between the two heads (${o.plan.lines} lines).`, + `reviewed ${o.from}${o.context ? ` (range_base ${o.rangeBase})` : ''}; current ${o.head}. Below is \`git range-diff\` of the reviewed range against the current one, then the diff of the changed product paths between the two heads${o.context ? ', then the hunks where base context moved next to the reviewed ones' : ''} (${o.plan.lines} lines).`, 'Review only what changed between the two ranges, against the blocking floor: would fail a test, break production, or lose data or secrets.', - `Reply NO FINDINGS, or one finding per line with path:line, Severity and Blocks. A clean reply is recorded as {"from":"${o.from}","head":"${o.head}","range_diff_sha256":"${o.plan.sha256}","reader":"","verdict":"clean"}.`, + `Reply NO FINDINGS, or one finding per line with path:line, Severity and Blocks. A clean reply is recorded as ${recheckRecordTemplate({ ...o, sha256: o.plan.sha256 })}.`, 'A blocking finding means the full /ship on the current head.\n', - '```diff', o.rangeDiff.trim(), '```', '', + '```diff', o.rangeDiff.trim(), '```', '', ...(o.context ? [o.context.trim(), ''] : []), ].join('\n'); } -export interface RecheckRecord { from: string; head: string; range_diff_sha256: string; reader: string; verdict: 'clean' } +export interface RecheckRecord { from: string; head: string; range_base?: string; range_diff_sha256: string; reader: string; verdict: 'clean' } /** A clean re-check record bound to this carry's from, head and range-diff, or why it does not stand. */ -export function readRecheck(file: string, bind: { from: string; head: string; sha256: string }): { ok: true; record: RecheckRecord } | { ok: false; message: string } { +export function readRecheck(file: string, bind: { from: string; head: string; sha256: string; rangeBase?: string }): { ok: true; record: RecheckRecord } | { ok: false; message: string } { let v: Record; 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') return { ok: false, message: `${file} is not a JSON object` }; if (v.verdict !== 'clean') return { ok: false, message: `the re-check is ${JSON.stringify(v.verdict)}, not clean: run /ship on ${bind.head.slice(0, 12)}` }; if (typeof v.reader !== 'string' || !v.reader.trim()) return { ok: false, message: 'the re-check names no reader' }; - for (const [k, want] of [['from', bind.from], ['head', bind.head], ['range_diff_sha256', bind.sha256]] as const) { + for (const [k, want] of [['from', bind.from], ['head', bind.head], ['range_diff_sha256', bind.sha256], ...(bind.rangeBase ? [['range_base', bind.rangeBase] as const] : [])] as const) { if (v[k] !== want) return { ok: false, message: `the re-check is for ${k} ${String(v[k] ?? 'none').slice(0, 12)}, not ${want.slice(0, 12)}` }; } return { ok: true, record: v as unknown as RecheckRecord }; diff --git a/scripts/lib/windows-curation.ts b/scripts/lib/windows-curation.ts index 857c5b016..90c191455 100644 --- a/scripts/lib/windows-curation.ts +++ b/scripts/lib/windows-curation.ts @@ -411,6 +411,10 @@ export const KNOWN_WINDOWS_INCOMPATIBLE: Array<{ file: string; reason: string }> file: 'test/ship-mechanical-retake.test.ts', reason: 'drives bin/gstack-ship-release, gstack-review-plan, gstack-ship-receipt, gstack-docs-candidate and gstack-wtree (shebangs) through test/helpers/ship-bin-fixture.ts like test/ship-finish-stack-parent.test.ts; windows-latest fails every case at the first bin spawn (windows-scoped-shard, run 42156939158)', }, + { + file: 'test/ship-receipt-carry-mixed.test.ts', + reason: 'drives bin/gstack-ship-receipt, gstack-ship-release, gstack-docs-candidate and gstack-wtree (shebangs) through test/helpers/ship-bin-fixture.ts like test/ship-mechanical-retake.test.ts; windows-latest fails every case at the first bin spawn (windows-scoped-shard, run 42167292675)', + }, ]; // Force-include overrides: files a WINDOWS_FRAGILE_PATTERNS regex excludes for diff --git a/test/ship-receipt-carry-mixed.test.ts b/test/ship-receipt-carry-mixed.test.ts new file mode 100644 index 000000000..627a7f5aa --- /dev/null +++ b/test/ship-receipt-carry-mixed.test.ts @@ -0,0 +1,202 @@ +/** + * GST-1561 (founderstar#563): a receipt carry whose diff holds both a changed product path and a context path had no + * bounded route. RECEIPT_CARRY_SCOPED_REVIEW opened only when no path changed, and J1's scoped re-check refused whenever + * base context moved, so a base merge that bumped a constant the branch also replaces (SAVE_VERSION := 82 became 83 on + * the base; the branch's own `:= 84` line is byte-identical) next to a moved base block forced a full /ship every time. + * Here: a hunk whose only difference is a base-side removed line takes the scoped review with the context paths (finish + * and carry), a changed path plus context paths take one composed re-check packet bound to range_base, past the size cap + * it refuses as before, and a branch-side change to an added line still needs review. + */ +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 { comparePath } from '../lib/ship-patch'; +import { extractReceipt } from '../lib/ship-receipt'; +import { RECHECK_RANGE_DIFF_CAP } from '../lib/ship-recheck'; +import { lineOf, RELEASE, shipFixture, type ShipFx } from './helpers/ship-bin-fixture'; + +setDefaultTimeout(120_000); +const BIN = path.dirname(RELEASE); +const RECEIPT = path.join(BIN, 'gstack-ship-receipt'); +const LANE = 'tests=printf " 3 pass\\n 0 fail\\nRan 3 tests across 1 file. [5.00ms]\\n"'; +const READER = 'anthropic/claude-opus-5-5'; +const ships: ShipFx[] = []; +afterEach(() => { while (ships.length) ships.pop()!.cleanup(); }); + +const GAME = 'scripts/core/game.gd'; +const VESTING = 'scripts/core/vesting.gd'; +const SCORE = 'scripts/core/score.gd'; +const game = (save: number) => ['extends Node', '', `const SAVE_VERSION := ${save}`, '', 'func _ready():', '\tload_save()', '', 'func load_save():', '\tpass', ''].join('\n') + '\n'; +const vesting = (edit: Record = {}) => Array.from({ length: 12 }, (_, i) => edit[i + 1] ?? `var v${i + 1} := ${i + 1}`).join('\n') + '\n'; +const BRANCH_VESTING = { 6: 'func was_vesting_clear(): return granted and not revoked' }; +const BASE_VESTING = { 4: 'var v4 := 40 # moved by the base' }; +const UNATTENDED = { GSTACK_SESSION_KIND: 'unattended' }; + +/** A branch reviewed at its first commit (receipt written by gstack-ship-receipt write, so it records review_depth). */ +function reviewedBranch(base: Record, feat: Record) { + const f = shipFixture(base, feat); ships.push(f); + const from = f.git('rev-parse', 'HEAD').stdout.trim(); + const reviewedTree = f.run(path.join(BIN, 'gstack-wtree'), []).stdout.trim(); + const reviewedBase = f.git('merge-base', 'origin/main', 'HEAD').stdout.trim(); + const body = path.join(f.root, 'body.md'); + fs.writeFileSync(body, '## Summary\n'); + const w = f.run(RECEIPT, ['write', '--pr', '5', '--base', 'main', '--body', body], UNATTENDED); + expect([w.status, w.stderr.slice(0, 400)]).toEqual([EXIT.ok, expect.any(String)]); + expect(extractReceipt(fs.readFileSync(body, 'utf8')).receipt!.review_depth).toBeDefined(); + const carry = (extra: string[] = []) => f.run(RECEIPT, ['carry', '--body', body, '--base', 'main', ...extra], UNATTENDED); + return { f, from, reviewedTree, reviewedBase, body, carry }; +} + +/** Merge origin/main; a conflict takes `resolved` and commits the merge. */ +function mergeMain(f: ShipFx, resolved: Record) { + f.git('fetch', '-q', 'origin'); + if (f.git('merge', '-q', '--no-edit', 'origin/main').status !== 0) { f.write(resolved); f.commit('merge main'); } +} + +/** founderstar#563: the base took SAVE_VERSION 83 under the branch's 84, and moved a block next to the branch's vesting hunk. */ +function founderstar(resolvedSave = 84, baseBlock = 0) { + const x = reviewedBranch({ [GAME]: game(82), [VESTING]: vesting(), [SCORE]: 'var score := 1\n' }, { [GAME]: game(84), [VESTING]: vesting(BRANCH_VESTING) }); + const moved = { 4: [BASE_VESTING[4], ...Array.from({ length: baseBlock }, (_, i) => `var base${i} := ${i}`)].join('\n') }; + x.f.advanceBase({ [GAME]: game(83), [VESTING]: vesting(moved) }); + mergeMain(x.f, { [GAME]: game(resolvedSave), [VESTING]: vesting({ ...moved, ...BRANCH_VESTING }) }); + const head = x.f.git('rev-parse', 'HEAD').stdout.trim(); + const sides = { reviewedBase: x.reviewedBase, reviewed: x.from, currentBase: x.f.git('merge-base', 'origin/main', 'HEAD').stdout.trim(), current: head }; + return { ...x, head, sides }; +} + +const scopedPacket = (err: string) => /one fix-check reviewer reads (\S+), then rerun carry/.exec(err)?.[1]; +const eligible = (err: string) => /^RECHECK: eligible range_diff_lines=(\d+) range_diff_sha256=([0-9a-f]{64}) packet=(\S+);/m.exec(err); +const recordAt = (f: ShipFx, name: string, v: object) => { const file = path.join(f.root, name); fs.writeFileSync(file, JSON.stringify(v)); return file; }; + +describe('a base-side removed line next to byte-identical added lines (GST-1561 ask 2)', () => { + test('comparePath routes it with the context paths; a branch-side added-line change stays changed', () => { + const same = founderstar(); + expect(comparePath(same.f.work, same.sides, GAME)).toBe('context'); + expect(comparePath(same.f.work, same.sides, VESTING)).toBe('context'); + const moved = founderstar(85); + expect(comparePath(moved.f.work, moved.sides, GAME)).toBe('changed'); + }); + + test('a branch that now also removes a line the base never touched stays changed', () => { + const x = reviewedBranch({ [GAME]: game(82) }, { [GAME]: game(84) }); + x.f.advanceBase({ [GAME]: game(83) }); + mergeMain(x.f, { [GAME]: game(84).replace('\tload_save()\n', '') }); + const head = x.f.git('rev-parse', 'HEAD').stdout.trim(); + const sides = { reviewedBase: x.reviewedBase, reviewed: x.from, currentBase: x.f.git('merge-base', 'origin/main', 'HEAD').stdout.trim(), current: head }; + expect(comparePath(x.f.work, sides, GAME)).toBe('changed'); + }); + + for (const [where, inserted] of [['before', game(82).replace('const SAVE', 'const BASE_ADD := 1\nconst SAVE')], ['after', game(82).replace(':= 82\n', ':= 82\nconst BASE_ADD := 1\n')], ['beside its own edit of', game(83).replace(':= 83\n', ':= 83\nconst BASE_ADD := 1\n')]] as const) { + test(`a merge that drops a line the base inserted ${where} the replaced line stays changed`, () => { + const x = reviewedBranch({ [GAME]: game(82) }, { [GAME]: game(84) }); + x.f.advanceBase({ [GAME]: inserted }); + mergeMain(x.f, { [GAME]: game(84) }); + if (fs.readFileSync(path.join(x.f.work, GAME), 'utf8') !== game(84)) { x.f.write({ [GAME]: game(84) }); x.f.commit('drop the base line'); } + const head = x.f.git('rev-parse', 'HEAD').stdout.trim(); + const sides = { reviewedBase: x.reviewedBase, reviewed: x.from, currentBase: x.f.git('merge-base', 'origin/main', 'HEAD').stdout.trim(), current: head }; + expect(comparePath(x.f.work, sides, GAME)).toBe('changed'); + }); + } +}); + +describe('carry: the founderstar#563 shape takes one scoped reader, then binds', () => { + test('one packet holds the constant hunk and the moved context block; a clean record carries', () => { + const { f, head, carry, body } = founderstar(); + const ask = carry(); + expect([ask.status, ask.stdout.trim().split('\n')[0]], ask.stderr).toEqual([EXIT.refused, 'CARRY: refused (RECEIPT_CARRY_SCOPED_REVIEW)']); + const packet = scopedPacket(ask.stderr); + expect(packet, ask.stderr).toBeDefined(); + const text = fs.readFileSync(packet!, 'utf8'); + for (const s of ['-const SAVE_VERSION := 82', '-const SAVE_VERSION := 83', '+const SAVE_VERSION := 84', 'was_vesting_clear', 'moved by the base']) expect(text).toContain(s); + const partial = recordAt(f, 'partial.json', { head, paths: [VESTING], reviewer: READER, verdict: 'clean' }); + expect(carry(['--scoped-review', partial]).stderr).toContain(`the scoped review does not cover ${GAME}`); + const ok = carry(['--scoped-review', recordAt(f, 'scoped.json', { head, paths: [GAME, VESTING], reviewer: READER, verdict: 'clean' })]); + expect([ok.status, ok.stderr.slice(0, 600)]).toEqual([EXIT.ok, expect.any(String)]); + const carried = extractReceipt(fs.readFileSync(body, 'utf8')).receipt!; + expect(carried.head).toBe(head); + expect(carried.carry!.reason).toContain(`scoped review by ${READER}`); + expect(carried.carry!.reason).toContain(GAME); + }); + + test('a branch-side change to the added line still forces review (never the scoped context route)', () => { + const { carry } = founderstar(85); + const r = carry(); + expect(r.status).toBe(EXIT.refused); + expect(r.stdout).toContain('RECEIPT_CARRY_PRODUCT_CHANGED'); + expect(r.stderr).toContain(GAME); + expect(r.stdout).not.toContain('RECEIPT_CARRY_SCOPED_REVIEW'); + }); +}); + +describe('carry: a changed path plus context paths compose into one bounded re-check (GST-1561 ask 1)', () => { + const composed = (baseBlock = 0) => { + const x = founderstar(84, baseBlock); + x.f.write({ [SCORE]: 'var score0 := 2\n' }); x.f.commit('fix: score'); + return { ...x, head: x.f.git('rev-parse', 'HEAD').stdout.trim() }; + }; + + test('one packet holds the range-diff and the context hunks, bound to range_base; a clean record carries', () => { + const { f, from, head, reviewedBase, carry, body } = composed(); + const ask = carry(); + expect([ask.status, ask.stdout.trim().split('\n')[0]], ask.stderr).toEqual([EXIT.refused, 'CARRY: refused (RECEIPT_CARRY_PRODUCT_CHANGED)']); + const m = eligible(ask.stderr); + expect(m, ask.stderr).not.toBeNull(); + const rangeBase = /"range_base":"([0-9a-f]{40,64})"/.exec(ask.stderr)?.[1]; + expect(rangeBase).toBe(reviewedBase); + const text = fs.readFileSync(m![3]!, 'utf8'); + for (const s of ['var score0 := 2', 'was_vesting_clear', 'moved by the base', 'const SAVE_VERSION := 83', `range_base ${reviewedBase}`]) expect(text).toContain(s); + const record = { from, head, range_base: rangeBase, range_diff_sha256: m![2], reader: READER, verdict: 'clean' }; + const noBase = carry(['--recheck', recordAt(f, 'nobase.json', { ...record, range_base: undefined })]); + expect([noBase.status, noBase.stdout.includes('RECEIPT_CARRY_REFUSED'), noBase.stderr.includes('range_base')]).toEqual([EXIT.refused, true, true]); + const ok = carry(['--recheck', recordAt(f, 'recheck.json', record)]); + expect([ok.status, ok.stderr.slice(0, 600)]).toEqual([EXIT.ok, expect.any(String)]); + expect(ok.stdout).toContain(`scoped re-check by ${READER}`); + const reason = extractReceipt(fs.readFileSync(body, 'utf8')).receipt!.carry!.reason; + expect(reason).toContain(`scoped re-check by ${READER}`); + expect(reason).toContain(VESTING); + expect(reason).toContain(`range_base ${reviewedBase.slice(0, 12)}`); + }); + + test('past the size cap it refuses as before: RECEIPT_CARRY_PRODUCT_CHANGED, re-check not eligible', () => { + const { carry } = composed(RECHECK_RANGE_DIFF_CAP); + const r = carry(); + expect([r.status, r.stdout.trim().split('\n')[0]]).toEqual([EXIT.refused, 'CARRY: refused (RECEIPT_CARRY_PRODUCT_CHANGED)']); + expect(r.stderr).toMatch(/RECHECK: not eligible \(the packet is \d+ lines, over the scoped re-check cap/); + expect(eligible(r.stderr)).toBeNull(); + }); + + test('changed-only keeps J1 as it is; context-only keeps the scoped review', () => { + const changed = reviewedBranch({ [SCORE]: 'var score := 1\n' }, { [SCORE]: 'var score := 2\n' }); + changed.f.write({ [SCORE]: 'var score := 3\n' }); changed.f.commit('fix: score 3'); + const c = changed.carry(); + const m = eligible(c.stderr); + expect(m, c.stderr).not.toBeNull(); + expect(c.stderr).not.toContain('"range_base"'); + expect(fs.readFileSync(m![3]!, 'utf8')).not.toContain('# Scoped review'); + + const context = reviewedBranch({ [VESTING]: vesting() }, { [VESTING]: vesting(BRANCH_VESTING) }); + context.f.advanceBase({ [VESTING]: vesting(BASE_VESTING) }); + mergeMain(context.f, { [VESTING]: vesting({ ...BASE_VESTING, ...BRANCH_VESTING }) }); + const s = context.carry(); + expect([s.status, s.stdout.trim().split('\n')[0]]).toEqual([EXIT.refused, 'CARRY: refused (RECEIPT_CARRY_SCOPED_REVIEW)']); + expect(s.stderr).not.toContain('RECHECK:'); + }); +}); + +describe('finish: the founderstar#563 shape takes the scoped reader too', () => { + test('ROUTE sends nothing to behavior, and one clean scoped record lets finish go on', () => { + const x = founderstar(); + const post = path.join(x.f.root, 'post.json'); + expect(x.f.run(path.join(BIN, 'gstack-docs-candidate'), ['snapshot', '--out', post, '--audit-id', 'a1', '--mode', 'edit', '--base', 'main', '--docs', 'README.md']).status).toBe(0); + const finish = (extra: string[] = []) => x.f.run(RELEASE, ['finish', '--base', 'main', '--reviewed-tree', x.reviewedTree, '--docs-post', post, '--lane', LANE, '--reviewed-base', x.reviewedBase, ...extra]); + const ask = finish(); + expect([ask.status, lineOf(ask.stdout, 'ROUTE')], ask.stdout).toEqual([3, expect.stringMatching(/ behavior=0 .* context=[12]$/)]); + const packet = /reads (\S+\.md)[ ,]/.exec(ask.stdout)?.[1]; + expect(fs.readFileSync(packet!, 'utf8')).toContain('moved by the base'); + const tree = x.f.run(path.join(BIN, 'gstack-wtree'), []).stdout.trim(); + const ok = finish(['--context-reviewed', recordAt(x.f, 'ctx.json', { tree, paths: [GAME, VESTING], reviewer: READER, verdict: 'clean' })]); + expect(lineOf(ok.stdout, 'SCOPED_REVIEW'), ok.stdout).toStartWith(`SCOPED_REVIEW: clean by ${READER} (`); + expect(ok.stdout).not.toContain('STOP: behavior'); + }); +}); diff --git a/test/ship-recheck.test.ts b/test/ship-recheck.test.ts index 487a61486..d5cc8a33b 100644 --- a/test/ship-recheck.test.ts +++ b/test/ship-recheck.test.ts @@ -2,8 +2,8 @@ * J1 (GSTA-258 L4): a scoped re-check after a refused receipt carry. Each refusal reason that does or does not qualify, * the record binding, and the carry end to end: a bounded product change prints RECHECK: eligible and writes the packet, * a clean record bound to that packet carries the receipt with the reader named, and a changed risk class, a legacy - * receipt, more paths than the walk names and moved base context never take the re-check (the /ship p1 red-team and - * security findings). + * receipt and more paths than the walk names never take the re-check (the /ship p1 red-team and security findings). + * Moved base context joins the packet instead (GST-1561; test/ship-receipt-carry-mixed.test.ts). */ import { afterEach, describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; @@ -32,7 +32,8 @@ describe('planRecheck: which refusals qualify', () => { ['legacy receipt', { ...OK, legacy: true }, 'no review_depth'], ['walk cut at the named-path limit', { ...OK, walkCut: true }, 'more changed product paths'], ['unnamed product path', { ...OK, unnamedPaths: true }, 'could not be named'], - ['base context moved', { ...OK, contextPaths: 1 }, 'base context'], + ['base context moved without its sections', { ...OK, contextPaths: 1 }, 'base context'], + ['base context over its section cap', { ...OK, contextPaths: 1, context: { text: '## src/c.ts', over: ['src/c.ts'] } }, 'section cap'], ['binary product path', { ...OK, binary: true }, 'binary'], ['wide packet over the byte cap', { ...OK, rangeDiff: 'x'.repeat(RECHECK_PACKET_BYTES + 1) }, 'bytes'], ['range-diff failed', { ...OK, rangeDiff: null }, 'failed or timed out'], @@ -138,7 +139,7 @@ describe('carry with a scoped re-check (CLI)', () => { expect(runBin(bin.repo, 'gstack-ship-receipt', ['carry', '--body', bin.body], bin.env).err).toContain('RECHECK: not eligible (a changed product path is binary'); }, 90_000); - test('base context moved next to a reviewed hunk plus a product edit elsewhere is not eligible (red-team p1 repro)', () => { + test('base context moved next to a reviewed hunk plus a product edit elsewhere composes the context hunks into the packet (red-team p1 repro, GST-1561)', () => { const lines = (n: number, edits: Record = {}) => Array.from({ length: 10 }, (_, i) => edits[i + 1] ?? `line ${i + 1}`).join('\n') + '\n'; const repo = makeFixtureRepo({ prefix: 'ship-recheck-ctx-' }); repos.push(repo); fs.mkdirSync(path.join(repo.work, 'src'), { recursive: true }); @@ -161,6 +162,11 @@ describe('carry with a scoped re-check (CLI)', () => { repo.git('commit', '-qam', 'fix: app'); const r = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body], env); expect(r.status).toBe(EXIT.refused); - expect(r.err).toContain('RECHECK: not eligible (base context also moved'); + const m = eligible(r.err); + expect(m, r.err).not.toBeNull(); + const packet = fs.readFileSync(m![3]!, 'utf8'); + expect(packet).toContain('export const a = 2;'); + expect(packet).toContain('## src/c.ts'); + expect(packet).toContain('main line 4'); }, 60_000); });