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>
This commit is contained in:
garrytanandcapy-ai[bot] committed 2026-11-02 20:40:49 +00:00
1 parent ead29adf42
commit 1b02200e20
6 files changed
+37 -9

No files matched your search

+2 -2
View File
@@ -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);
+1 -1
View File
@@ -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` });
});
+10 -6
View File
@@ -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 ? '' : '(?<![A-Za-z0-9_.])'}${escapeRegExp(p)}${post ? '' : '(?![A-Za-z0-9_.])'}${escapeRegExp(post)}`, 'g');
text = text.replace(re, () => { 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 {
+1
View File
@@ -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]']);
+19
View File
@@ -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']);
@@ -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', () => {