From 4e8b1de50303dca54667abf5aaff09303d4a3694 Mon Sep 17 00:00:00 2001 From: garrytan Date: Tue, 3 Nov 2026 11:37:36 +0000 Subject: [PATCH] fix(ship-receipt): never leave out a path with a compared path beneath it (review fix, GST-1214) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- bin/gstack-ship-receipt | 5 ++++- test/ship-receipt-carry-squash.test.ts | 18 ++++++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/bin/gstack-ship-receipt b/bin/gstack-ship-receipt index 64a7b9b70..3d1a90e27 100755 --- a/bin/gstack-ship-receipt +++ b/bin/gstack-ship-receipt @@ -670,7 +670,10 @@ function cmdCarry(rest: string[], json: boolean): number { if (!listed[0] || !listed[1] || !listed[2]) return `git could not list the paths the branch touched against ${base}; run /ship on ${head.slice(0, 12)}`; // From a stack parent, a path whose reviewed content is the base's (an earlier base merge brought it in) is base // context, not reviewed product, unless HEAD changed it against the base (GST-1214). - const baseMerged = rangeBase === fromBase ? [] : [...listed[0]].filter(p => !listed[2]!.has(p) && !listed[1]!.has(p)); + // A pathspec also names everything beneath a path, so a path with any compared path under it (a file the base turned + // into a directory) stays in. + const compared = [...listed[0], ...listed[1]]; + const baseMerged = rangeBase === fromBase ? [] : [...listed[0]].filter(p => !listed[2]!.has(p) && !listed[1]!.has(p) && !compared.some(q => q.startsWith(`${p}/`))); const touched = new Set([...listed[0], ...listed[1]].filter(p => !baseMerged.includes(p))); const found = productSpec(repoRoot, rules, from, head, { touched, sides: [[rangeBase, from], [headBase, head]], own: branchSides(repoRoot, baseRef, from, '') }); if (!found) return `git diff ${from.slice(0, 12)} ${head.slice(0, 12)} failed`; diff --git a/test/ship-receipt-carry-squash.test.ts b/test/ship-receipt-carry-squash.test.ts index 34474216f..20db3a387 100644 --- a/test/ship-receipt-carry-squash.test.ts +++ b/test/ship-receipt-carry-squash.test.ts @@ -243,7 +243,7 @@ describe('carry after a stack parent squash-merges, receipt written against main describe('carry after a stack parent lands, the child having merged main before its receipt (GST-1214)', () => { /** main <- parent (lib/util.js) <- feature (lib/retry.js), which merges main (wiki/) once before its receipt is written against main. */ - function mergedStack(repo: FixtureRepo): { parent: string; reviewed: string; body: string } { + function mergedStack(repo: FixtureRepo, mainFiles: Record = { 'wiki/a.md': '# a\n', 'wiki/b.md': '# b\n' }): { parent: string; reviewed: string; body: string } { repo.git('checkout', '-q', '-b', 'parent'); const parent = commit(repo, { 'lib/util.js': SUB }, 'parent: sub'); repo.git('push', '-q', 'origin', 'parent'); @@ -251,7 +251,7 @@ describe('carry after a stack parent lands, the child having merged main before repo.git('reset', '-q', '--hard', parent); commit(repo, { 'lib/retry.js': RETRY4 }, 'child: four retries'); repo.git('checkout', '-q', 'main'); - commit(repo, { 'wiki/a.md': '# a\n', 'wiki/b.md': '# b\n' }, 'main: wiki'); + commit(repo, mainFiles, 'main: wiki'); repo.git('push', '-q', 'origin', 'main'); repo.git('checkout', '-q', 'feature'); repo.git('merge', '-q', '--no-edit', 'origin/main'); @@ -290,4 +290,18 @@ describe('carry after a stack parent lands, the child having merged main before expect(out.err).toContain('wiki/a.md'); expect(receiptIn(body).head).toBe(reviewed); }, 90_000); + + // Value: protects=fail closed when the earlier base merge turned a file into a directory: leaving out the old file must not + // hide an edit beneath it (a pathspec names a path and everything under it); fails_when=the exclusion covers a changed + // descendant; seam=none; no_claim=- + test('a base merge that replaced a file with a directory never hides an edit beneath it', () => { + const repo = makeFixtureRepo({ baseFiles: { services: 'old\n' } }); repos.push(repo); + const { reviewed, body } = mergedStack(repo, { services: null, 'services/worker.js': 'exports.allowed = false;\n' }); + landOnMain(repo, { 'lib/util.js': SUB }); + commit(repo, { 'services/worker.js': 'exports.allowed = true;\n' }, 'child: allow'); + const out = carry(repo, body); + expect(out.status).toBe(EXIT.refused); + expect(out.out).toContain('CARRY: refused (RECEIPT_CARRY_PRODUCT_CHANGED)'); + expect(receiptIn(body).head).toBe(reviewed); + }, 90_000); });