From 63e2b7ac2c1139e18aa0d261fa5b4748884623a2 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 16 Aug 2026 10:36:49 -0700 Subject: [PATCH] fix(ship): review fix loop stays in one invocation, bounded at 3 cycles (#2391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-landing review committed its fixes, then STOPPED and told the user to run /ship again — 5-10 manual invocations on a branch with a few auto-fixable findings, violating /ship's fully-automated contract. There is no user decision between those invocations; each rerun just repeats the workflow until a review pass produces no fixes. ship/sections/review-army.md.tmpl item 7 now makes the loop explicit: after committing fixes, re-run the test suite (Step 5) and this review (Step 9 items 2-6) in the SAME invocation, repeating until one full pass applies zero fixes, then continue to Step 12. Bounded at 3 fix cycles — a review that will not converge STOPs with a report of which findings keep reappearing (a genuine blocker), never with a rerun request. test/ship-review-loop.test.ts asserts no rendered ship surface (section + all three host goldens) carries the STOP-and-rerun shape and that the bounded loop language renders. Fixes #2391 Co-Authored-By: Claude Fable 5 --- ship/sections/review-army.md | 3 +- ship/sections/review-army.md.tmpl | 3 +- test/fixtures/golden/codex-ship-SKILL.md | 3 +- test/fixtures/golden/factory-ship-SKILL.md | 3 +- test/ship-review-loop.test.ts | 49 ++++++++++++++++++++++ 5 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 test/ship-review-loop.test.ts diff --git a/ship/sections/review-army.md b/ship/sections/review-army.md index a840119f2..245433056 100644 --- a/ship/sections/review-army.md +++ b/ship/sections/review-army.md @@ -383,7 +383,8 @@ Output a summary header: `Pre-Landing Review: N issues (X critical, Y informatio - If 3 or fewer ASK items, you may use individual AskUserQuestion calls instead 7. **After all fixes (auto + user-approved):** - - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **STOP** and tell the user to run `/ship` again to re-test. + - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **stay in this invocation and loop**: re-run the test suite (Step 5) on the fixed code, then re-run this review (Step 9 items 2-6) against the updated diff. Repeat until one full pass applies ZERO fixes — tests green and review clean — then continue to Step 12. NEVER stop to tell the user to run `/ship` again; a fix-and-rerun cycle has no user decision in it, and stopping there breaks the fully-automated contract (#2391). + - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. - If no fixes applied (all ASK items skipped, or no issues found): continue to Step 12. 8. Output summary: `Pre-Landing Review: N issues — M auto-fixed, K asked (J fixed, L skipped)` diff --git a/ship/sections/review-army.md.tmpl b/ship/sections/review-army.md.tmpl index e55db627e..541531355 100644 --- a/ship/sections/review-army.md.tmpl +++ b/ship/sections/review-army.md.tmpl @@ -33,7 +33,8 @@ Review the diff for structural issues that tests don't catch. - If 3 or fewer ASK items, you may use individual AskUserQuestion calls instead 7. **After all fixes (auto + user-approved):** - - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **STOP** and tell the user to run `/ship` again to re-test. + - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **stay in this invocation and loop**: re-run the test suite (Step 5) on the fixed code, then re-run this review (Step 9 items 2-6) against the updated diff. Repeat until one full pass applies ZERO fixes — tests green and review clean — then continue to Step 12. NEVER stop to tell the user to run `/ship` again; a fix-and-rerun cycle has no user decision in it, and stopping there breaks the fully-automated contract (#2391). + - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. - If no fixes applied (all ASK items skipped, or no issues found): continue to Step 12. 8. Output summary: `Pre-Landing Review: N issues — M auto-fixed, K asked (J fixed, L skipped)` diff --git a/test/fixtures/golden/codex-ship-SKILL.md b/test/fixtures/golden/codex-ship-SKILL.md index 79d179610..7edb0701c 100644 --- a/test/fixtures/golden/codex-ship-SKILL.md +++ b/test/fixtures/golden/codex-ship-SKILL.md @@ -2148,7 +2148,8 @@ Output a summary header: `Pre-Landing Review: N issues (X critical, Y informatio - If 3 or fewer ASK items, you may use individual AskUserQuestion calls instead 7. **After all fixes (auto + user-approved):** - - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **STOP** and tell the user to run `/ship` again to re-test. + - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **stay in this invocation and loop**: re-run the test suite (Step 5) on the fixed code, then re-run this review (Step 9 items 2-6) against the updated diff. Repeat until one full pass applies ZERO fixes — tests green and review clean — then continue to Step 12. NEVER stop to tell the user to run `/ship` again; a fix-and-rerun cycle has no user decision in it, and stopping there breaks the fully-automated contract (#2391). + - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. - If no fixes applied (all ASK items skipped, or no issues found): continue to Step 12. 8. Output summary: `Pre-Landing Review: N issues — M auto-fixed, K asked (J fixed, L skipped)` diff --git a/test/fixtures/golden/factory-ship-SKILL.md b/test/fixtures/golden/factory-ship-SKILL.md index ac4d091ce..de86452e0 100644 --- a/test/fixtures/golden/factory-ship-SKILL.md +++ b/test/fixtures/golden/factory-ship-SKILL.md @@ -2402,7 +2402,8 @@ Output a summary header: `Pre-Landing Review: N issues (X critical, Y informatio - If 3 or fewer ASK items, you may use individual AskUserQuestion calls instead 7. **After all fixes (auto + user-approved):** - - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **STOP** and tell the user to run `/ship` again to re-test. + - If ANY fixes were applied: commit fixed files by name (`git add && git commit -m "fix: pre-landing review fixes"`), then **stay in this invocation and loop**: re-run the test suite (Step 5) on the fixed code, then re-run this review (Step 9 items 2-6) against the updated diff. Repeat until one full pass applies ZERO fixes — tests green and review clean — then continue to Step 12. NEVER stop to tell the user to run `/ship` again; a fix-and-rerun cycle has no user decision in it, and stopping there breaks the fully-automated contract (#2391). + - **Bound: 3 fix cycles.** If the 3rd cycle still applies fixes, STOP and report which findings keep reappearing — a review that won't converge is a genuine blocker worth human eyes, not a re-run request. - If no fixes applied (all ASK items skipped, or no issues found): continue to Step 12. 8. Output summary: `Pre-Landing Review: N issues — M auto-fixed, K asked (J fixed, L skipped)` diff --git a/test/ship-review-loop.test.ts b/test/ship-review-loop.test.ts new file mode 100644 index 000000000..88d919cb5 --- /dev/null +++ b/test/ship-review-loop.test.ts @@ -0,0 +1,49 @@ +/** + * /ship review fix loop stays in one invocation (#2391). + * + * The pre-landing review used to commit its fixes, STOP, and tell the user + * to run /ship again — 5-10 manual invocations on a branch with a few + * auto-fixable findings, violating the skill's fully-automated contract. + * The rendered section must instruct a bounded in-invocation loop + * (re-test, re-review, max 3 fix cycles) and must never terminate an + * AUTO-FIX result with a rerun request. + */ +import { describe, test, expect } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; + +const ROOT = path.join(import.meta.dir, '..'); + +const RENDERED_SITES = [ + path.join(ROOT, 'ship', 'sections', 'review-army.md'), + path.join(ROOT, 'test', 'fixtures', 'golden', 'claude-ship-SKILL.md'), + path.join(ROOT, 'test', 'fixtures', 'golden', 'codex-ship-SKILL.md'), + path.join(ROOT, 'test', 'fixtures', 'golden', 'factory-ship-SKILL.md'), +]; + +describe('/ship review fix loop (#2391)', () => { + test('no rendered ship surface instructs a STOP-and-rerun after fixes', () => { + // The pre-fix instruction: "then **STOP** and tell the user to run + // `/ship` again". The fixed text mentions the phrase only inside a + // NEVER-do-this prohibition, so match the imperative STOP shape. + const rerunRequest = /\*\*STOP\*\*[^\n]*run `\/ship` again/; + for (const file of RENDERED_SITES) { + const content = fs.readFileSync(file, 'utf-8'); + expect(rerunRequest.test(content)).toBe(false); + } + }); + + test('rendered section instructs the bounded in-invocation loop', () => { + const content = fs.readFileSync(RENDERED_SITES[0], 'utf-8'); + expect(content).toContain('stay in this invocation and loop'); + expect(content).toContain('3 fix cycles'); + // The loop re-runs tests AND the review, and only a converged pass continues. + expect(content).toContain('re-run the test suite (Step 5)'); + expect(content).toContain('re-run this review (Step 9 items 2-6)'); + }); + + test('the non-convergence stop is a blocker report, not a rerun request', () => { + const content = fs.readFileSync(RENDERED_SITES[0], 'utf-8'); + expect(content).toContain('report which findings keep reappearing'); + }); +});