From b5676b59c083ec732f23a8e19fc5c2dd42742bf0 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 11 May 2026 21:13:07 -0400 Subject: [PATCH] fix: accept flag-first syntax for phase remove (#3416) * fix(phase): parse remove --force regardless of position * chore(changeset): add fragment for phase-remove flag fix * fix(phase): fail when phase.remove target is missing --- .../3409-phase-remove-force-position.md | 5 ++++ .../bin/lib/phase-command-router.cjs | 20 ++++++++++++- sdk/src/query/phase-lifecycle.test.ts | 29 +++++++++++++++++++ sdk/src/query/phase-lifecycle.ts | 29 +++++++++++++++---- tests/phase.test.cjs | 24 +++++++++++++++ 5 files changed, 100 insertions(+), 7 deletions(-) create mode 100644 .changeset/3409-phase-remove-force-position.md diff --git a/.changeset/3409-phase-remove-force-position.md b/.changeset/3409-phase-remove-force-position.md new file mode 100644 index 000000000..445b246ca --- /dev/null +++ b/.changeset/3409-phase-remove-force-position.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3416 +--- +**`phase remove --force ` now works correctly across both CLI and SDK query paths (#3409)** — flag parsing no longer treats `--force` as the phase id when the flag appears before the positional argument, preventing false success responses and unintended `STATE.md` phase-count drift. diff --git a/get-shit-done/bin/lib/phase-command-router.cjs b/get-shit-done/bin/lib/phase-command-router.cjs index 2bedc18fa..14a6eeaad 100644 --- a/get-shit-done/bin/lib/phase-command-router.cjs +++ b/get-shit-done/bin/lib/phase-command-router.cjs @@ -66,7 +66,25 @@ function routePhaseCommand({ phase, args, cwd, raw, error }) { } phase.cmdPhaseInsert(cwd, args[2], args.slice(3).join(' '), raw); }, - remove: () => phase.cmdPhaseRemove(cwd, args[2], { force: args.includes('--force') }, raw), + remove: () => { + const removeArgs = args.slice(2).filter(token => token !== '--raw'); + let forceFlag = false; + const positional = []; + for (const token of removeArgs) { + if (token === '--force') { + forceFlag = true; + continue; + } + if (token.startsWith('--')) { + error(`phase remove does not support ${token}`); + } + positional.push(token); + } + if (positional.length > 1) { + error('phase remove accepts exactly one phase number'); + } + phase.cmdPhaseRemove(cwd, positional[0], { force: forceFlag }, raw); + }, complete: () => phase.cmdPhaseComplete(cwd, args[2], raw), }, }); diff --git a/sdk/src/query/phase-lifecycle.test.ts b/sdk/src/query/phase-lifecycle.test.ts index 2746e010f..9f7def534 100644 --- a/sdk/src/query/phase-lifecycle.test.ts +++ b/sdk/src/query/phase-lifecycle.test.ts @@ -972,6 +972,22 @@ describe('phaseRemove', () => { expect(data.directory_deleted).toBeTruthy(); }); + it('bug-3409: accepts --force before phase id', async () => { + const { phaseRemove } = await import('./phase-lifecycle.js'); + const phasesDir = join(tmpDir, '.planning', 'phases'); + await setupTestProject(tmpDir, { + roadmap: ROADMAP_FOR_REMOVE, + state: STATE_FOR_REMOVE, + phases: ['05-auth', '06-dashboard', '07-api'], + }); + await writeFile(join(phasesDir, '06-dashboard', '06-01-SUMMARY.md'), 'summary', 'utf-8'); + + const result = await phaseRemove(['--force', '6'], tmpDir); + const data = result.data as Record; + expect(data.removed).toBe('6'); + expect(data.directory_deleted).toBeTruthy(); + }); + it('throws GSDError when ROADMAP.md is missing', async () => { const { phaseRemove } = await import('./phase-lifecycle.js'); // Set up without ROADMAP.md @@ -994,6 +1010,19 @@ describe('phaseRemove', () => { await expect(phaseRemove([], tmpDir)).rejects.toThrow('phase number required'); }); + it('throws GSDError when target phase does not exist and does not mutate STATE.md', async () => { + const { phaseRemove } = await import('./phase-lifecycle.js'); + await setupTestProject(tmpDir, { + roadmap: ROADMAP_FOR_REMOVE, + state: STATE_FOR_REMOVE, + phases: ['05-auth', '06-dashboard', '07-api'], + }); + + await expect(phaseRemove(['99'], tmpDir)).rejects.toThrow('Phase 99 not found'); + const stateContent = await readFile(join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + expect(stateContent).toMatch(/total_phases:\s*7/); + }); + it('updates ROADMAP.md by removing phase section and renumbering', async () => { const { phaseRemove } = await import('./phase-lifecycle.js'); await setupTestProject(tmpDir, { diff --git a/sdk/src/query/phase-lifecycle.ts b/sdk/src/query/phase-lifecycle.ts index 8920acfca..1180e4593 100644 --- a/sdk/src/query/phase-lifecycle.ts +++ b/sdk/src/query/phase-lifecycle.ts @@ -842,7 +842,24 @@ async function updateRoadmapAfterPhaseRemoval( * @returns QueryResult with { removed, directory_deleted, renamed_directories, renamed_files, roadmap_updated, state_updated } */ export const phaseRemove: QueryHandler = async (args, projectDir, workstream) => { - const targetPhase = args[0]; + let force = false; + const positional: string[] = []; + for (const token of args) { + if (token === '--force') { + force = true; + continue; + } + if (token.startsWith('--')) { + throw new GSDError(`phase remove does not support ${token}`, ErrorClassification.Validation); + } + positional.push(token); + } + + if (positional.length > 1) { + throw new GSDError('phase remove accepts exactly one phase number', ErrorClassification.Validation); + } + + const targetPhase = positional[0]; if (!targetPhase) { throw new GSDError('phase number required for phase remove', ErrorClassification.Validation); } @@ -857,15 +874,17 @@ export const phaseRemove: QueryHandler = async (args, projectDir, workstream) => const normalized = normalizePhaseName(targetPhase); const isDecimal = targetPhase.includes('.'); - const force = args[1] === '--force'; // Find target directory const entries = await readdir(phasesDir, { withFileTypes: true }); const dirs = entries.filter(e => e.isDirectory()).map(e => e.name); const targetDir = dirs.find(d => phaseTokenMatches(d, normalized)) ?? null; + if (!targetDir) { + throw new GSDError(`Phase ${targetPhase} not found`, ErrorClassification.Validation); + } // Guard against removing executed work - if (targetDir && !force) { + if (!force) { const files = await readdir(join(phasesDir, targetDir)); const summaries = files.filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); if (summaries.length > 0) { @@ -877,9 +896,7 @@ export const phaseRemove: QueryHandler = async (args, projectDir, workstream) => } // Delete directory - if (targetDir) { - await rm(join(phasesDir, targetDir), { recursive: true, force: true }); - } + await rm(join(phasesDir, targetDir), { recursive: true, force: true }); // Renumber subsequent phases on disk let renamedDirs: Array<{ from: string; to: string }> = []; diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index fac4c2ed2..84541896b 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -1491,6 +1491,30 @@ describe('phase remove command', () => { assert.ok(forceResult.success, `Force remove failed: ${forceResult.error}`); }); + test('bug-3409: supports --force before phase id', () => { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap\n### Phase 1: A\n**Goal:** A\n### Phase 2: B\n**Goal:** B\n` + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `# State\n\n**Current Phase:** 1\n**Total Phases:** 2\n` + ); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-a'), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-b'), { recursive: true }); + + const result = runGsdTools('phase remove --force 2', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.removed, '2'); + assert.strictEqual(output.directory_deleted, '02-b'); + assert.ok(!fs.existsSync(path.join(tmpDir, '.planning', 'phases', '02-b'))); + + const state = fs.readFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'utf-8'); + assert.ok(state.includes('**Total Phases:** 1'), 'total phases should be decremented after real removal'); + }); + test('removes decimal phase and renumbers siblings', () => { fs.writeFileSync( path.join(tmpDir, '.planning', 'ROADMAP.md'),