From 1b02200e20385ae38f3453c4b7e6a2c540de538b Mon Sep 17 00:00:00 2001 From: garrytan Date: Mon, 2 Nov 2026 20:40:49 +0000 Subject: [PATCH] fix: pre-landing review fixes (carry keeps a write --version receipt's version without a version source; a placeholder fill composes with a stamp move; a bare {version} template and a blank placeholder take no placeholder) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-receipt | 4 ++-- lib/ship-policy.ts | 2 +- lib/stamp-only.ts | 16 ++++++++++------ test/gstack-ship-policy.test.ts | 1 + test/gstack-ship-receipt-docs.test.ts | 19 +++++++++++++++++++ test/tree-receipt-stamp-sensitive.test.ts | 4 ++++ 6 files changed, 37 insertions(+), 9 deletions(-) diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index ed17a062b..056669aae 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -749,8 +749,8 @@ function cmdCarry(rest: string[], json: boolean): number { } // The rebound product_patch_id is HEAD's diff against the carry's base, so a retargeted receipt names that base from now on. // A receipt that records a version names HEAD's, read as write reads it: a restamp after a base merge moves it (GST-1030). - const version = receipt.version === undefined ? undefined : receiptVersion(repoRoot, head, base); - if (receipt.version !== undefined && version === undefined) return refused('RECEIPT_CARRY_REFUSED', `HEAD ${head.slice(0, 12)} has no readable release version, so the carried receipt cannot name one; fix the version source, then rerun carry`); + // A head with no version source keeps the recorded one (write --version named it). + const version = receipt.version === undefined ? undefined : (receiptVersion(repoRoot, head, base) ?? receipt.version); const rebound = { ...receipt, ...(version !== undefined ? { version } : {}), ...(retarget ? { base: base.replace(/^origin\//, '') } : {}), ...(pregateField ?? {}), ...depthField }; const carried = carryReceipt(rebound, head, plan.carry, { tree: headTree, fromTree, productPatchId: headPatch }); const errors = validateReceipt(carried); diff --git a/lib/ship-policy.ts b/lib/ship-policy.ts index 044c75d49..8c56c275d 100644 --- a/lib/ship-policy.ts +++ b/lib/ship-policy.ts @@ -352,7 +352,7 @@ function placeholderErrors(value: unknown, out: PolicyError[]): void { value.forEach((p, i) => { const at = `stamp_placeholders[${i}]`; if (typeof p !== 'string') { out.push({ code: 'POLICY_INVALID', path: at, message: 'must be a string' }); return; } - if (!p || p.length > MAX_PLACEHOLDER || /[\0\r\n]/.test(p)) out.push({ code: 'POLICY_CONTAINMENT', path: at, message: `must be one non-empty line of at most ${MAX_PLACEHOLDER} characters` }); + if (!p.trim() || p.length > MAX_PLACEHOLDER || /[\0\r\n]/.test(p)) out.push({ code: 'POLICY_CONTAINMENT', path: at, message: `must be one non-empty line of at most ${MAX_PLACEHOLDER} characters` }); else if (p.includes(VERSION_SLOT)) out.push({ code: 'POLICY_INVALID', path: at, message: `${JSON.stringify(p)} must not contain ${VERSION_SLOT}` }); else if (parseVersion(p)) out.push({ code: 'POLICY_INVALID', path: at, message: `${JSON.stringify(p)} is a version, not a placeholder` }); }); diff --git a/lib/stamp-only.ts b/lib/stamp-only.ts index a5f0b3179..53b2b5231 100644 --- a/lib/stamp-only.ts +++ b/lib/stamp-only.ts @@ -30,14 +30,17 @@ import { cmpVersion, parseVersion, versionWidth } from './version-source'; * branch line may change: the stage moves nothing else then. On such a branch * line a declared placeholder (`stamp_placeholders`, GST-1026: gbrain's * `since: 'TBD'`) may also become the new version, but only inside a - * `stamp_patterns` template, never a built-in form: the line rewritten that way - * must be the added line, with at least one rewrite. + * `stamp_patterns` template with literal text around its slot, never a built-in + * form: with at least one such rewrite, the line must then be the added line, + * or one whose remaining stamp-form moves pass the fork-version bound. */ export function stampOnlyLine(removed: string, added: string, oldV: string, newV: string, templates: readonly string[] = [], side?: BranchSide, at?: number, placeholders: readonly string[] = []): boolean { if (removed === added) return false; if (oldV !== newV && stampOnlyWithin(removed, added, oldV, newV, templates)) return true; if (!side || at === undefined || !side.at.has(at) || !side.added.has(removed)) return false; - return stampOnlyWithin(removed, added, side.fork, newV, templates) || placeholderOnly(removed, added, newV, templates, placeholders); + if (stampOnlyWithin(removed, added, side.fork, newV, templates)) return true; + const filled = fillPlaceholders(removed, newV, templates, placeholders); + return filled.count > 0 && (filled.text === added || stampOnlyWithin(filled.text, added, side.fork, newV, templates)); } /** @@ -70,18 +73,19 @@ export function branchSides(repoRoot: string, baseRef: string, head: string, fal }; } -/** Each declared placeholder rewritten to `newV` inside each template (a bare edge of the slot at a word boundary): the added line, with at least one rewrite. */ -function placeholderOnly(removed: string, added: string, newV: string, templates: readonly string[], placeholders: readonly string[]): boolean { +/** Each declared placeholder rewritten to `newV` inside each template with literal text around its slot (a bare edge at a word boundary). */ +function fillPlaceholders(removed: string, newV: string, templates: readonly string[], placeholders: readonly string[]): { text: string; count: number } { let text = removed; let count = 0; for (const t of templates) { const [pre, post] = t.split(VERSION_SLOT) as [string, string]; + if (!pre && !post) continue; for (const p of placeholders) { const re = new RegExp(`${escapeRegExp(pre)}${pre ? '' : '(? { count++; return pre + newV + post; }); } } - return count > 0 && text === added; + return { text, count }; } function stampOnlyWithin(removed: string, added: string, oldV: string, newV: string, templates: readonly string[]): boolean { diff --git a/test/gstack-ship-policy.test.ts b/test/gstack-ship-policy.test.ts index 66016e1a9..e290256a8 100644 --- a/test/gstack-ship-policy.test.ts +++ b/test/gstack-ship-policy.test.ts @@ -82,6 +82,7 @@ describe('validatePolicy (schema + containment, pure)', () => { expect(at(['x'.repeat(32)])).toEqual([]); expect(at(['TB\nD'])).toEqual(['POLICY_CONTAINMENT:stamp_placeholders[0]']); expect(at([''])).toEqual(['POLICY_CONTAINMENT:stamp_placeholders[0]']); + expect(at([' '])).toEqual(['POLICY_CONTAINMENT:stamp_placeholders[0]']); expect(at(['{version}'])).toEqual(['POLICY_INVALID:stamp_placeholders[0]']); expect(at(['next-{version}'])).toEqual(['POLICY_INVALID:stamp_placeholders[0]']); expect(at(['1.2.3.0'])).toEqual(['POLICY_INVALID:stamp_placeholders[0]']); diff --git a/test/gstack-ship-receipt-docs.test.ts b/test/gstack-ship-receipt-docs.test.ts index 167aafb20..03213266c 100644 --- a/test/gstack-ship-receipt-docs.test.ts +++ b/test/gstack-ship-receipt-docs.test.ts @@ -53,6 +53,25 @@ describe('E18: a repository without a version source or CHANGELOG', () => { expect(JSON.parse(fs.readFileSync(path.join(out, 'ship-run.json'), 'utf8')).version).toBe('none'); }); + // Value: protects=a receipt write --version named in a repository with no version source keeps that version through a + // carry (GST-1030 review); fails_when=the carry refuses because HEAD has no readable version, or drops the version; + // seam=bin/gstack-ship-receipt cmdCarry + test('a carry keeps the version write --version named when HEAD has no version source (GST-1030)', () => { + const repo = fixture(); + for (const f of ['VERSION', 'package.json', 'package-lock.json']) fs.rmSync(path.join(repo.work, f)); + repo.git('add', '-A'); repo.git('commit', '-qm', 'changelog only'); repo.git('push', '-q', 'origin', 'main'); + makePr(repo, 'pr-unversioned'); + const body = path.join(repo.root, 'body.md'); + fs.writeFileSync(body, '## Summary\n'); + expect(runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '3', '--base', 'main', '--session-kind', 'x', '--version', '2.0.0', '--body', body]).status).toBe(EXIT.ok); + fs.mkdirSync(path.join(repo.work, 'test'), { recursive: true }); + fs.writeFileSync(path.join(repo.work, 'test', 'a.test.ts'), 'test a\n'); + repo.git('add', '-A'); repo.git('commit', '-qm', 'a test-only change'); + const carried = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']); + expect(carried.out, carried.err).toContain('CARRY: carried'); + expect(extractReceipt(fs.readFileSync(body, 'utf8')).receipt?.version).toBe('2.0.0'); + }); + test('a repository with a version source keeps its version, and one with only a CHANGELOG records no version rather than "none"', () => { const repo = fixture(); const w = runBin(repo, 'gstack-ship-receipt', ['write', '--base', 'main', '--session-kind', 'x', '--json']); diff --git a/test/tree-receipt-stamp-sensitive.test.ts b/test/tree-receipt-stamp-sensitive.test.ts index 848704e63..033f4ea5b 100644 --- a/test/tree-receipt-stamp-sensitive.test.ts +++ b/test/tree-receipt-stamp-sensitive.test.ts @@ -145,6 +145,10 @@ describe('tree receipt: stamps, stamp-sensitive gates and negative fixtures', () expect(line(' { since: TBD, id: mine },', ' { since: 0.60.553.0, id: mine },')).toBe(false); expect(line(row, " { since: '0.60.553.0', id: 'yours' },")).toBe(false); expect(line(row, " { since: '0.60.554.0', id: 'mine' },")).toBe(false); + const both = " { since: 'TBD', note: 'Added in v0.60.552.0' },"; + expect(line(both, " { since: '0.60.553.0', note: 'Added in v0.60.553.0' },")).toBe(true); + expect(line(both, " { since: '0.60.553.0', note: 'Added in v0.60.551.0' },")).toBe(false); + expect(line(' // TBD: tidy', ' // 0.60.553.0: tidy', { templates: ['{version}'] })).toBe(false); }); test('negative fixture: a dependency pin equal to the old version is `changed`, not a stamp, even inside stamp_paths', () => {