diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index c3e2460ca..96f7630a3 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -48,7 +48,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, PACKET_SECTION_CAP, portedEntry, readScopedReview, touchedPaths } from '../lib/ship-patch'; +import { comparePath, contextPacket, PACKET_SECTION_CAP, readScopedReview, touchedPaths } from '../lib/ship-patch'; import { deeper, depthBelow, exceptionProblems, parseReviewLoopException, parseReviewTargeted, receiptExclude, receiptReviewAtHead, receiptTargetedAtHead, reviewLoopExceptionLine, targetedClassesSince } from '../lib/ship-receipt-depth'; import { checkReceiptProse, proseLine } from '../lib/ship-receipt-prose'; import { resolveStateRoot } from '../lib/state-root'; @@ -538,7 +538,10 @@ function productSpec(repoRoot: string, rules: ProductRules, from: string, to: st // A stamp file the branch changed on neither side beyond a stamp against that side's base (back at the base's content // after a changelog-fragment conversion, or only its version moved) holds none of the branch's product: whatever else // differs between the two heads came from the base (GST-1210). - const ownStampOnly = (p: string) => !!branch?.touched.has(p) && stamped(p) && branch.sides.every(([a, b]) => (git(['rev-parse', '--verify', '--quiet', `${a}:${p}`], repoRoot) ?? 'absent') === (git(['rev-parse', '--verify', '--quiet', `${b}:${p}`], repoRoot) ?? 'absent') || classify(p, a, b) === 'metadata'); + // The same `ls-tree` entry (mode and object id, empty when absent) is an unchanged side; a failed read never is. + const entryAt = (ref: string, p: string) => git(['ls-tree', ref, '--', p], repoRoot); + const sameEntry = (a: string, b: string, p: string) => { const x = entryAt(a, p); return x !== undefined && x === entryAt(b, p); }; + const ownStampOnly = (p: string) => !!branch?.touched.has(p) && stamped(p) && branch.sides.every(([a, b]) => sameEntry(a, b, p) || classify(p, a, b) === 'metadata'); const changed = changedRaw.split('\n').filter(Boolean).map(p => ({ path: p, class: ownStampOnly(p) ? 'metadata' as const : classify(p, from, to) })); const excludeGlobs = (globs: readonly string[]) => globs.map(g => `:(exclude,glob)${g}`); const demoted = !branch ? changed.filter(c => c.class === 'product' && matchesAny(rules.metadata, c.path)).map(c => c.path) @@ -699,7 +702,6 @@ function cmdCarry(rest: string[], json: boolean): number { const changedProduct: string[] = []; const contextPaths: string[] = []; const stampOnly: string[] = []; - const ported: 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 walk stops there, and it reads only the paths the @@ -719,11 +721,10 @@ function cmdCarry(rest: string[], json: boolean): number { if (cls === 'metadata' && spec.ownRows.includes(p) && !movedBase.has(p)) stampOnly.push(p); if (cls !== 'product') continue; const verdict = comparePath(repoRoot, sides, p); - if (verdict === 'changed' && portedEntry(repoRoot, sides, p, movedBase)) ported.push(p); - else if (verdict === 'changed') changedProduct.push(p); + if (verdict === 'changed') changedProduct.push(p); else if (verdict === 'context') contextPaths.push(p); } - const left = [...contextPaths, ...stampOnly, ...ported]; + const left = [...contextPaths, ...stampOnly]; if (!changedProduct.length && !left.length) changedProduct.push('(a product path outside the branch\'s touched files; compare the two base diffs)'); else if (!changedProduct.length) { const rest = [...spec.product, ...left.map(p => `:(exclude,literal)${p}`)]; @@ -762,7 +763,7 @@ function cmdCarry(rest: string[], json: boolean): number { // 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', `${rangeBase}..${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 } : {}), ...(stackParent ? { stackParent } : {}), ...(scopedReview ? { scopedReview } : {}), ...(stampedRows.length ? { stampedRows } : {}), ...(!changedProduct.length && ported.length ? { portedEntries: ported } : {}) }); + 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 } : {}) }); } 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/capy-gst1210-carry-stamp-reverts.md b/changelog.d/capy-gst1210-carry-stamp-reverts.md index c24dcd56b..569eb9d5d 100644 --- a/changelog.d/capy-gst1210-carry-stamp-reverts.md +++ b/changelog.d/capy-gst1210-carry-stamp-reverts.md @@ -1 +1 @@ -- **A /ship receipt now survives two edits that change no reviewed code (GST-1210).** `gstack-ship-receipt carry` used to refuse with `RECEIPT_CARRY_PRODUCT_CHANGED` when a PR moved to a changelog fragment after review, putting `package.json` and the other stamp files back at main's content, if main had also changed `package.json` beyond its version (HARN-1, harness-evals#206). A stamp file the branch changes by no more than a stamp against each head's base now carries as release metadata. It also refused when a merge from main copied main's repin of one harness-pin entry, byte for byte, into the branch's one-file-per-entry pin file (#3903). That now carries as `base entries ported byte for byte`. A non-stamp `package.json` change, a logic edit, or a pin that differs from main's entry still refuses. +- **A /ship receipt now survives a changelog-fragment conversion (GST-1210).** When a PR moved to a `changelog.d` fragment after review, `VERSION`, `package.json` and `CHANGELOG.md` went back to main's content, and if main had also changed `package.json` beyond its version, `gstack-ship-receipt carry` read that as the branch's change and refused with `RECEIPT_CARRY_PRODUCT_CHANGED` (HARN-1, harness-evals#206). A stamp file the branch changes by no more than a stamp against each head's base now carries as release metadata. A non-stamp `package.json` change, a logic edit, or a stamp file whose entry can't be read still refuses. diff --git a/docs/ship-policy.md b/docs/ship-policy.md index cbf200b9d..abc075469 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1506,7 +1506,6 @@ receipt when the base moves: | a stacked PR whose receipt was written against the base, after its parent landed and the base was merged in, HEAD's product diff exactly the reviewed range from the parent's head | carries (reason `stack parent landed`); base context next to the child's hunks is `RECEIPT_CARRY_SCOPED_REVIEW` | | a stamp on the branch's own rows in a stamp file the branch also changed as product: every changed line at a branch-added position, the file's mode unchanged, the base not touching it, the rest of the product diff unchanged | carries (reason `a stamp on the branch's own rows ()`) | | a stamp file the branch changes, against each head's base, by no more than a stamp (back at the base's content after a changelog-fragment conversion, or only its version field moved), whatever else the base changed in it | carries (reason `release metadata`, GST-1210) | -| a one-line `*.json` pin file the branch added, named `…/.json`, whose change since the reviewed head is byte for byte the base's own edit of entry `` in a JSON map line of a base file (a merge that ports a base repin into a one-file-per-entry split) | carries (reason `base entries ported byte for byte ()`, GST-1210); a pin that differs from the base's entry, or sits in another entry's file, refuses | **Stacked PRs (#3570).** A child PR's receipt is written against its parent branch, so its `base` names that branch and its `product_patch_id` is the diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 882aea8c4..24cc0ec57 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -5996,8 +5996,7 @@ landed on the base, a stamp on the branch's own rows in a stamp file it also changed as product (every changed line at a branch-added position, the mode unchanged, the base not touching the file), a stamp file the branch changes by no more than a stamp against each head's base (a changelog-fragment conversion -that puts `package.json` back at the base's content), a one-entry pin file -whose change is byte for byte the base's edit of that entry, or base context next to unchanged +that puts `package.json` back at the base's content), or base context next to unchanged hunks after a clean scoped review ([`RECEIPT_CARRY_SCOPED_REVIEW`](#receipt-carry-scoped-review)), 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)). diff --git a/lib/ship-patch.ts b/lib/ship-patch.ts index 242ba4157..d069dc5cc 100644 --- a/lib/ship-patch.ts +++ b/lib/ship-patch.ts @@ -120,37 +120,6 @@ export function comparePath(repoRoot: string, s: PatchSides, rel: string): PathV return ids[0] === ids[1] ? 'same' : 'context'; } -/** - * The base's JSON map file a one-entry pin file ports (GST-1210, #3903 at f0613db8): `rel` is a one-line `*.json` file the - * branch added (absent at both bases, a regular file at both heads) named `…/.json`, whose whole text at each head is - * byte for byte the value of entry `` in one JSON map line of a base file `baseMoved` lists: the old value at the - * reviewed base, the new one at the current base. The branch then holds the base's own edit of that entry and nothing of - * its own. Exactly one entry of that map may name the file. Null for anything else, so a pin that differs from the base's - * entry stays a product change. - */ -export function portedEntry(repoRoot: string, s: PatchSides, rel: string, baseMoved: Iterable): string | null { - if (!rel.endsWith('.json')) return null; - const blob = (ref: string, p: string) => git(repoRoot, ['show', `${ref}:${p}`]); - const regular = (ref: string) => /^100644 blob /.test(git(repoRoot, ['ls-tree', ref, '--', rel]) ?? ''); - const value = (text: string | null) => (text !== null && /^[^\n]+\n$/.test(text) ? text.slice(0, -1) : null); - const before = value(blob(s.reviewed, rel)), after = value(blob(s.current, rel)); - if (before === null || after === null || before === after || !regular(s.reviewed) || !regular(s.current)) return null; - if (blob(s.reviewedBase, rel) !== null || blob(s.currentBase, rel) !== null) return null; - const entries = (text: string | null) => new Set((text ?? '').split('\n').map(l => l.trim().replace(/,$/, ''))); - for (const file of baseMoved) { - if (file === rel || !file.endsWith('.json')) continue; - const named = [...entries(blob(s.currentBase, file))].flatMap(line => { - const m = /^("(?:[^"\\]|\\.)*"): (.*)$/.exec(line); - let key: unknown; - try { key = m ? JSON.parse(m[1]!) : null; } catch { return []; } - return typeof key === 'string' && key && rel.endsWith(`/${key}.json`) ? [{ quoted: m![1]!, value: m![2]! }] : []; - }); - // One entry of the map names the pin file; two (`retry.js` and `lib/retry.js` for `…/lib/retry.js.json`) is no port. - if (named.length === 1 && named[0]!.value === after && entries(blob(s.reviewedBase, file)).has(`${named[0]!.quoted}: ${before}`)) return file; - } - return null; -} - function hunkTexts(diff: string): string[] { return diff.split(/^(?=@@ )/m).slice(1).map(h => h.replace(/^@@ [^@]*@@/, '@@')); } diff --git a/lib/ship-receipt.ts b/lib/ship-receipt.ts index 3130cbea8..bbb145358 100644 --- a/lib/ship-receipt.ts +++ b/lib/ship-receipt.ts @@ -184,8 +184,6 @@ export interface CarryPlanInput { scopedReview?: string; /** Stamp files the branch changed as product whose only change since the reviewed head is a stamp on the branch's own rows, the base untouched, and without which the two product patches match (GST-980). */ stampedRows?: string[]; - /** Branch-added one-entry pin files whose change since the reviewed head is, byte for byte, the base's own edit of that entry in a base JSON map (`portedEntry`, GST-1210). */ - portedEntries?: 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 }; @@ -254,7 +252,7 @@ export function classifyCarryPath(rel: string, extra: { metadata?: readonly stri * branch's own rows in a stamp file the branch also changed as product (`stampedRows`, GST-980). */ export function planCarry(input: CarryPlanInput): CarryPlan { - if (input.productPatchId.from !== input.productPatchId.head && (input.changedProduct.length || (!input.scopedReview && !input.stampedRows?.length && !input.portedEntries?.length))) { + if (input.productPatchId.from !== input.productPatchId.head && (input.changedProduct.length || (!input.scopedReview && !input.stampedRows?.length))) { 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)` : ''}` }; } @@ -263,7 +261,6 @@ export function planCarry(input: CarryPlanInput): CarryPlan { 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.stampedRows?.length && input.productPatchId.from !== input.productPatchId.head) reasons.push(`a stamp on the branch's own rows (${input.stampedRows.join(', ')})`); - if (input.portedEntries?.length && input.productPatchId.from !== input.productPatchId.head) reasons.push(`base entries ported byte for byte (${input.portedEntries.join(', ')})`); 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/test/ship-receipt-carry-fragment-conversion.test.ts b/test/ship-receipt-carry-fragment-conversion.test.ts index 5a0ff6f55..c1897e5a9 100644 --- a/test/ship-receipt-carry-fragment-conversion.test.ts +++ b/test/ship-receipt-carry-fragment-conversion.test.ts @@ -25,7 +25,7 @@ const RETRY4 = "exports.MAX_RETRIES = 4;\nexports.TIMEOUT_MS = 5000;\nexports.re * adopts changelog fragments and (`mainScripts`) changes package.json beyond its version. The branch merges main and * converts: `converted` is the package.json it keeps (main's by default), plus a fragment and any `extra` files. */ -function converted(repo: FixtureRepo, opts: { mainScripts?: Record; keep?: (main: string) => string; extra?: Record } = {}): { reviewed: string; body: string } { +function converted(repo: FixtureRepo, opts: { mainScripts?: Record; keep?: (main: string) => string; extra?: Record; executable?: boolean } = {}): { reviewed: string; body: string } { repo.git('checkout', '-q', 'main'); commit(repo, { VERSION: '1.0.0\n', 'CHANGELOG.md': '# Changelog\n\n## 1.0.0\n- base\n', '.gstack/ship-policy.json': POLICY(false) }, 'main: stamp files'); repo.git('push', '-q', 'origin', 'main'); @@ -45,6 +45,7 @@ function converted(repo: FixtureRepo, opts: { mainScripts?: Record m))(mainPkg)); write(repo.work, 'changelog.d/four-retries.md', '- four retries\n'); for (const [rel, text] of Object.entries(opts.extra ?? {})) write(repo.work, rel, text); + if (opts.executable) fs.chmodSync(path.join(repo.work, 'package.json'), 0o755); repo.git('add', '-A'); repo.git('commit', '-q', '--no-edit', '-m', 'convert to a changelog fragment'); return { reviewed, body }; @@ -87,6 +88,17 @@ describe('carry across a changelog-fragment conversion (GST-1210)', () => { expect(out.err).toContain('package.json'); }, 90_000); + // Value: protects=fail closed on a stamp file whose content is the base's but whose mode the branch flipped (a mode is + // not a stamp); fails_when=an unchanged side is read from the object id alone; seam=none; no_claim=gh pr edit + test('a package.json back at the base\'s content with a flipped mode still refuses', () => { + const repo = makeFixtureRepo(); repos.push(repo); + const { body } = converted(repo, { mainScripts: { lint: 'node lint.js' }, executable: true }); + const out = carry(repo, body); + expect(out.status).toBe(EXIT.refused); + expect(out.err).toContain('RECEIPT_CARRY_PRODUCT_CHANGED'); + expect(out.err).toContain('package.json'); + }, 90_000); + // Value: protects=fail closed on a logic edit beside the reviewed one riding the conversion; fails_when=the stamp-file // rule widens to the whole diff; seam=none; no_claim=gh pr edit test('a logic edit next to the reviewed constant still refuses', () => { diff --git a/test/ship-receipt-carry-ported-pin.test.ts b/test/ship-receipt-carry-ported-pin.test.ts index d27856bfa..7e80a18ab 100644 --- a/test/ship-receipt-carry-ported-pin.test.ts +++ b/test/ship-receipt-carry-ported-pin.test.ts @@ -1,19 +1,17 @@ /** - * gstack-ship-receipt carry across a base merge that ports one harness-pin entry (GST-1210, GSTA-202 #3903 at f0613db8): - * the branch splits the base's JSON map of pins into one file per entry, and a later merge from the base copies the base's - * edit of one entry into its pin file, byte for byte. That file holds the base's change and nothing of the branch's, so - * the receipt carries; a pin that differs from the base's entry, or lands in another entry's file, still refuses. + * gstack-ship-receipt carry never guesses which base entry a branch-added pin file holds (GST-1210, GSTA-202 #3903 at + * f0613db8): the branch splits the base's JSON map of pins into one file per entry, and a pin file that changes after review + * is a product change whatever the base did to a same-named entry. Inferring the mapping from names and values let an + * unrelated base edit carry an unreviewed change (GSTA-36 ruling A on #3937), so these controls keep every such pin refused. */ import { afterEach, describe, expect, test } from 'bun:test'; import * as fs from 'node:fs'; import * as path from 'node:path'; import { EXIT } from '../lib/headless-artifacts'; -import { extractReceipt } from '../lib/ship-receipt'; import { cleanup, commit, makeFixtureRepo, runBin, type FixtureRepo } from './helpers/pregate-fixture'; const repos: FixtureRepo[] = []; afterEach(() => { while (repos.length) cleanup(repos.pop()!); }); -const receiptIn = (body: string) => extractReceipt(fs.readFileSync(body, 'utf8')).receipt!; const carry = (repo: FixtureRepo, body: string) => runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']); const RETRY = '{"blob":"aaa","since":6,"nonBehavioral":"first pin"}'; const UTIL = '{"blob":"bbb","since":6}'; @@ -41,20 +39,6 @@ function ported(repo: FixtureRepo, ports: Record, maps: [string, } describe('carry across a ported harness-pin entry (GST-1210)', () => { - // Value: protects=a branch that splits harness pins into one file per entry keeps its receipt when a base merge ports - // the base's repin byte for byte (#3903 at f0613db8 was refused); fails_when=carry reads the ported pin as the branch's - // product change; seam=none; no_claim=gh pr edit (--body stands in) - test('a pin file equal to the base\'s entry byte for byte carries', () => { - const repo = makeFixtureRepo(); repos.push(repo); - const { reviewed, body } = ported(repo, { [PIN]: `${RETRY_MAIN}\n` }); - const head = repo.git('rev-parse', 'HEAD'); - const out = carry(repo, body); - expect(out.err).not.toContain('RECEIPT_CARRY'); - expect(out.status).toBe(EXIT.ok); - expect(out.out).toContain(`CARRY: carried from ${reviewed.slice(0, 12)} to ${head.slice(0, 12)}`); - expect(receiptIn(body).carry!.reason).toContain(`base entries ported byte for byte (${PIN})`); - }, 90_000); - // Value: protects=fail closed on a pin the branch changed beyond the base's entry (an unreviewed repin); // fails_when=any pin file change carries; seam=none; no_claim=gh pr edit test('a pin entry that differs from the base\'s still refuses', () => {