From 4bf3b02bec8898a32d7fc597e2f90956a35338f9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 12 Apr 2026 15:52:33 -0400 Subject: [PATCH] fix: add phase add-batch command to prevent duplicate phase numbers on parallel invocations (#2165) (#2170) Parallel `phase add` invocations each read disk state before any write completes, causing all processes to calculate the same next phase number and produce duplicate directories and ROADMAP entries. The new `add-batch` subcommand accepts a JSON array of phase descriptions and performs all directory creation and ROADMAP appends within a single `withPlanningLock()` call, incrementing `maxPhase` within the lock for each entry. This guarantees sequential numbering regardless of call concurrency patterns. Closes #2165 Co-authored-by: Claude Sonnet 4.6 --- get-shit-done/bin/gsd-tools.cjs | 12 ++++- get-shit-done/bin/lib/phase.cjs | 71 ++++++++++++++++++++++++++ tests/phase.test.cjs | 89 +++++++++++++++++++++++++++++++++ 3 files changed, 171 insertions(+), 1 deletion(-) diff --git a/get-shit-done/bin/gsd-tools.cjs b/get-shit-done/bin/gsd-tools.cjs index 396b630e4..5d34852f2 100755 --- a/get-shit-done/bin/gsd-tools.cjs +++ b/get-shit-done/bin/gsd-tools.cjs @@ -714,6 +714,16 @@ async function runCommand(command, args, cwd, raw, defaultValue) { } } phase.cmdPhaseAdd(cwd, descArgs.join(' '), raw, customId); + } else if (subcommand === 'add-batch') { + // Accepts JSON array of descriptions via --descriptions '[...]' or positional args + const descFlagIdx = args.indexOf('--descriptions'); + let descriptions; + if (descFlagIdx !== -1 && args[descFlagIdx + 1]) { + try { descriptions = JSON.parse(args[descFlagIdx + 1]); } catch (e) { error('--descriptions must be a JSON array'); } + } else { + descriptions = args.slice(2).filter(a => a !== '--raw'); + } + phase.cmdPhaseAddBatch(cwd, descriptions, raw); } else if (subcommand === 'insert') { phase.cmdPhaseInsert(cwd, args[2], args.slice(3).join(' '), raw); } else if (subcommand === 'remove') { @@ -722,7 +732,7 @@ async function runCommand(command, args, cwd, raw, defaultValue) { } else if (subcommand === 'complete') { phase.cmdPhaseComplete(cwd, args[2], raw); } else { - error('Unknown phase subcommand. Available: next-decimal, add, insert, remove, complete'); + error('Unknown phase subcommand. Available: next-decimal, add, add-batch, insert, remove, complete'); } break; } diff --git a/get-shit-done/bin/lib/phase.cjs b/get-shit-done/bin/lib/phase.cjs index 9bdf70e8c..ea1b92060 100644 --- a/get-shit-done/bin/lib/phase.cjs +++ b/get-shit-done/bin/lib/phase.cjs @@ -408,6 +408,76 @@ function cmdPhaseAdd(cwd, description, raw, customId) { output(result, raw, result.padded); } +function cmdPhaseAddBatch(cwd, descriptions, raw) { + if (!Array.isArray(descriptions) || descriptions.length === 0) { + error('descriptions array required for phase add-batch'); + } + const config = loadConfig(cwd); + const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); + if (!fs.existsSync(roadmapPath)) { error('ROADMAP.md not found'); } + const projectCode = config.project_code || ''; + const prefix = projectCode ? `${projectCode}-` : ''; + + const results = withPlanningLock(cwd, () => { + let rawContent = fs.readFileSync(roadmapPath, 'utf-8'); + const content = extractCurrentMilestone(rawContent, cwd); + let maxPhase = 0; + if (config.phase_naming !== 'custom') { + const phasePattern = /#{2,4}\s*Phase\s+(\d+)[A-Z]?(?:\.\d+)*:/gi; + let m; + while ((m = phasePattern.exec(content)) !== null) { + const num = parseInt(m[1], 10); + if (num >= 999) continue; + if (num > maxPhase) maxPhase = num; + } + const phasesOnDisk = path.join(planningDir(cwd), 'phases'); + if (fs.existsSync(phasesOnDisk)) { + const dirNumPattern = /^(?:[A-Z][A-Z0-9]*-)?(\d+)-/; + for (const entry of fs.readdirSync(phasesOnDisk)) { + const match = entry.match(dirNumPattern); + if (!match) continue; + const num = parseInt(match[1], 10); + if (num >= 999) continue; + if (num > maxPhase) maxPhase = num; + } + } + } + const added = []; + for (const description of descriptions) { + const slug = generateSlugInternal(description); + let newPhaseId, dirName; + if (config.phase_naming === 'custom') { + newPhaseId = slug.toUpperCase().replace(/-/g, '-'); + dirName = `${prefix}${newPhaseId}-${slug}`; + } else { + maxPhase += 1; + newPhaseId = maxPhase; + dirName = `${prefix}${String(newPhaseId).padStart(2, '0')}-${slug}`; + } + const dirPath = path.join(planningDir(cwd), 'phases', dirName); + fs.mkdirSync(dirPath, { recursive: true }); + fs.writeFileSync(path.join(dirPath, '.gitkeep'), ''); + const dependsOn = config.phase_naming === 'custom' ? '' : `\n**Depends on:** Phase ${typeof newPhaseId === 'number' ? newPhaseId - 1 : 'TBD'}`; + const phaseEntry = `\n### Phase ${newPhaseId}: ${description}\n\n**Goal:** [To be planned]\n**Requirements**: TBD${dependsOn}\n**Plans:** 0 plans\n\nPlans:\n- [ ] TBD (run /gsd-plan-phase ${newPhaseId} to break down)\n`; + const lastSeparator = rawContent.lastIndexOf('\n---'); + rawContent = lastSeparator > 0 + ? rawContent.slice(0, lastSeparator) + phaseEntry + rawContent.slice(lastSeparator) + : rawContent + phaseEntry; + added.push({ + phase_number: typeof newPhaseId === 'number' ? newPhaseId : String(newPhaseId), + padded: typeof newPhaseId === 'number' ? String(newPhaseId).padStart(2, '0') : String(newPhaseId), + name: description, + slug, + directory: toPosixPath(path.join(path.relative(cwd, planningDir(cwd)), 'phases', dirName)), + naming_mode: config.phase_naming, + }); + } + atomicWriteFileSync(roadmapPath, rawContent); + return added; + }); + output({ phases: results, count: results.length }, raw); +} + function cmdPhaseInsert(cwd, afterPhase, description, raw) { if (!afterPhase || !description) { error('after-phase and description required for phase insert'); @@ -979,6 +1049,7 @@ module.exports = { cmdFindPhase, cmdPhasePlanIndex, cmdPhaseAdd, + cmdPhaseAddBatch, cmdPhaseInsert, cmdPhaseRemove, cmdPhaseComplete, diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index b23b31cad..92cfa3f58 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -891,6 +891,95 @@ describe('phase add with project_code', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// phase add-batch command (#2165) +// ───────────────────────────────────────────────────────────────────────────── + +describe('phase add-batch command (#2165)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + [ + '# Roadmap v1.0', + '', + '### Phase 1: Foundation', + '**Goal:** Setup', + '', + '---', + '', + ].join('\n') + ); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('adds multiple phases with sequential numbers in a single call', () => { + // Use array form to avoid shell quoting issues with JSON args + const result = runGsdTools(['phase', 'add-batch', '--descriptions', '["Alpha","Beta","Gamma"]'], tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual(output.count, 3, 'should report 3 phases added'); + assert.strictEqual(output.phases[0].phase_number, 2); + assert.strictEqual(output.phases[1].phase_number, 3); + assert.strictEqual(output.phases[2].phase_number, 4); + + assert.ok(fs.existsSync(path.join(tmpDir, '.planning', 'phases', '02-alpha')), '02-alpha dir must exist'); + assert.ok(fs.existsSync(path.join(tmpDir, '.planning', 'phases', '03-beta')), '03-beta dir must exist'); + assert.ok(fs.existsSync(path.join(tmpDir, '.planning', 'phases', '04-gamma')), '04-gamma dir must exist'); + + const roadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.ok(roadmap.includes('### Phase 2: Alpha'), 'roadmap should include Phase 2'); + assert.ok(roadmap.includes('### Phase 3: Beta'), 'roadmap should include Phase 3'); + assert.ok(roadmap.includes('### Phase 4: Gamma'), 'roadmap should include Phase 4'); + }); + + test('no duplicate phase numbers when multiple add-batch calls are made sequentially', () => { + // Regression for #2165: parallel `phase add` invocations produced duplicates + // because each read disk state before any write landed. add-batch serializes + // the entire batch under a single lock so the next call sees the updated state. + const r1 = runGsdTools(['phase', 'add-batch', '--descriptions', '["Wave-One-A","Wave-One-B"]'], tmpDir); + assert.ok(r1.success, `First batch failed: ${r1.error}`); + + const r2 = runGsdTools(['phase', 'add-batch', '--descriptions', '["Wave-Two-A","Wave-Two-B"]'], tmpDir); + assert.ok(r2.success, `Second batch failed: ${r2.error}`); + + const out1 = JSON.parse(r1.output); + const out2 = JSON.parse(r2.output); + const allNums = [...out1.phases, ...out2.phases].map(p => p.phase_number); + const unique = new Set(allNums); + assert.strictEqual(unique.size, allNums.length, `Duplicate phase numbers detected: ${allNums}`); + + // Directories must all exist and be unique + const dirs = fs.readdirSync(path.join(tmpDir, '.planning', 'phases')); + assert.strictEqual(dirs.length, 4, `Expected 4 phase dirs, got: ${dirs}`); + }); + + test('each phase directory contains a .gitkeep file', () => { + const result = runGsdTools(['phase', 'add-batch', '--descriptions', '["Setup","Build"]'], tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + assert.ok( + fs.existsSync(path.join(tmpDir, '.planning', 'phases', '02-setup', '.gitkeep')), + '.gitkeep must exist in 02-setup' + ); + assert.ok( + fs.existsSync(path.join(tmpDir, '.planning', 'phases', '03-build', '.gitkeep')), + '.gitkeep must exist in 03-build' + ); + }); + + test('returns error for empty descriptions array', () => { + const result = runGsdTools(['phase', 'add-batch', '--descriptions', '[]'], tmpDir); + assert.ok(!result.success, 'should fail on empty array'); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // phase insert command // ─────────────────────────────────────────────────────────────────────────────