diff --git a/.changeset/nimble-deer-chatter.md b/.changeset/nimble-deer-chatter.md new file mode 100644 index 000000000..68b7af945 --- /dev/null +++ b/.changeset/nimble-deer-chatter.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3246 +--- +**`gsd-sdk query phase.add --dry-run` is now honored** — previously absorbed into the description text and writing real files. Unknown `--flag` arguments now return a validation error instead of silent fallthrough. diff --git a/sdk/src/query/phase-lifecycle.test.ts b/sdk/src/query/phase-lifecycle.test.ts index 3f2863ac5..92eb01222 100644 --- a/sdk/src/query/phase-lifecycle.test.ts +++ b/sdk/src/query/phase-lifecycle.test.ts @@ -453,6 +453,144 @@ describe('phaseAdd', () => { // Should detect CK-45 and CK-46, so new phase = 47 expect(data.phase_number).toBe(47); }); + + // ── Symptom A: --dry-run flag (#3226) ───────────────────────────────── + + it('--dry-run returns JSON result without creating any files or modifying ROADMAP', async () => { + const { phaseAdd } = await import('./phase-lifecycle.js'); + await setupTestProject(tmpDir, { + phases: ['09-foundation', '10-read-only-queries'], + }); + + const roadmapBefore = await readFile(join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + const result = await phaseAdd(['Dry Run Phase', '--dry-run'], tmpDir); + const data = result.data as Record; + + // Result must include the computed fields + expect(data.phase_number).toBe(11); + expect(data.padded).toBe('11'); + expect(data.name).toBe('Dry Run Phase'); + expect(data.slug).toBe('dry-run-phase'); + expect(data.dry_run).toBe(true); + expect(typeof data.roadmap_entry).toBe('string'); + expect((data.roadmap_entry as string)).toContain('### Phase 11: Dry Run Phase'); + + // ROADMAP.md must be unchanged + const roadmapAfter = await readFile(join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + expect(roadmapAfter).toBe(roadmapBefore); + + // No new phase directory must have been created + const phasesDir = join(tmpDir, '.planning', 'phases'); + const entries = await readdir(phasesDir, { withFileTypes: true }); + const newDir = entries.find(e => e.isDirectory() && e.name.includes('11-dry-run-phase')); + expect(newDir).toBeUndefined(); + }); + + it('--dry-run works when flag appears after customId position', async () => { + const { phaseAdd } = await import('./phase-lifecycle.js'); + await setupTestProject(tmpDir, { + phases: ['09-foundation', '10-read-only-queries'], + }); + + const roadmapBefore = await readFile(join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + // description + --dry-run — no customId; flag must not be mistaken for customId + const result = await phaseAdd(['My Feature', '--dry-run'], tmpDir); + const data = result.data as Record; + + expect(data.dry_run).toBe(true); + expect(data.phase_number).toBe(11); + + // ROADMAP must still be untouched + const roadmapAfter = await readFile(join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + expect(roadmapAfter).toBe(roadmapBefore); + }); + + // ── Symptom C: unknown flag rejection (#3226) ────────────────────────── + + it('rejects unknown --flags with a validation error naming the flag', async () => { + const { phaseAdd } = await import('./phase-lifecycle.js'); + await setupTestProject(tmpDir); + + await expect(phaseAdd(['My Feature', '--bogus-flag'], tmpDir)).rejects.toThrow('--bogus-flag'); + }); + + it('rejects any unknown --flag even when mixed with dry-run', async () => { + const { phaseAdd } = await import('./phase-lifecycle.js'); + await setupTestProject(tmpDir); + + await expect(phaseAdd(['Desc', '--dry-run', '--unknown'], tmpDir)).rejects.toThrow('--unknown'); + }); + + // ── Symptom B: ROADMAP heading scan counts ### Phase N: (#3226 verify) ─ + + it('scans ### Phase N: headings in ROADMAP when no on-disk dirs exist (B already fixed)', async () => { + const { phaseAdd } = await import('./phase-lifecycle.js'); + + const roadmap = [ + '# Roadmap', + '', + '## Current Milestone: v5.0', + '', + '### Phase 5: Foundation', + '', + '**Goal:** Build foundation', + '**Plans:** 0 plans', + '', + ].join('\n'); + + await setupTestProject(tmpDir, { + roadmap, + state: MINIMAL_STATE, + phases: [], // no on-disk dirs — must rely on ROADMAP scan + }); + + const result = await phaseAdd(['Next Phase'], tmpDir); + const data = result.data as Record; + + // Must detect Phase 5 from ### heading → next = 6, not 1 + expect(data.phase_number).toBe(6); + }); + + // ── Concurrent phase.add: no duplicate IDs (CR finding) ──────────────── + it('concurrent phase.add calls produce distinct sequential phase numbers', async () => { + const { phaseAdd } = await import('./phase-lifecycle.js'); + await setupTestProject(tmpDir, { + phases: ['09-foundation', '10-read-only-queries'], + }); + + // Fire two phase.add calls simultaneously. If computation happens outside + // the lock both will observe maxPhase=10 and claim newPhaseId=11 — collision. + const [r1, r2] = await Promise.all([ + phaseAdd(['Concurrent Alpha'], tmpDir), + phaseAdd(['Concurrent Beta'], tmpDir), + ]); + + const n1 = (r1.data as Record).phase_number as number; + const n2 = (r2.data as Record).phase_number as number; + + // Both must succeed and produce DIFFERENT numbers + expect(n1).not.toBe(n2); + + // The pair must be {11, 12} — no gaps, no duplicates + const sorted = [n1, n2].sort((a, b) => a - b); + expect(sorted).toEqual([11, 12]); + + // ROADMAP.md must contain exactly one entry for each phase + const roadmap = await readFile(join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + const phase11Count = (roadmap.match(/### Phase 11:/g) || []).length; + const phase12Count = (roadmap.match(/### Phase 12:/g) || []).length; + expect(phase11Count).toBe(1); + expect(phase12Count).toBe(1); + + // Both phase directories must exist on disk + const phasesDir = join(tmpDir, '.planning', 'phases'); + const entries = await readdir(phasesDir, { withFileTypes: true }); + const dirs = entries.filter(e => e.isDirectory()).map(e => e.name); + const has11 = dirs.some(d => d.startsWith('11-')); + const has12 = dirs.some(d => d.startsWith('12-')); + expect(has11).toBe(true); + expect(has12).toBe(true); + }); }); // ─── phaseAddBatch ───────────────────────────────────────────────────── diff --git a/sdk/src/query/phase-lifecycle.ts b/sdk/src/query/phase-lifecycle.ts index a9a3e5c5c..b07bbe196 100644 --- a/sdk/src/query/phase-lifecycle.ts +++ b/sdk/src/query/phase-lifecycle.ts @@ -189,12 +189,36 @@ export async function readModifyWriteRoadmapMd( * Creates a new phase directory with .gitkeep, appends a phase section * to ROADMAP.md before the last "---" separator. * - * @param args - args[0]: description (required), args[1]: customId (optional) + * @param args - description (required), optional customId, optional --dry-run flag. + * Recognized flags: --dry-run (compute result without writing to disk). + * Any other --flag argument is rejected with a validation error. * @param projectDir - Project root directory * @returns QueryResult with { phase_number, padded, name, slug, directory, naming_mode } + * In --dry-run mode also includes { dry_run: true, roadmap_entry: string } */ export const phaseAdd: QueryHandler = async (args, projectDir, workstream) => { - const description = args[0]; + // ── Flag parsing ──────────────────────────────────────────────────────── + // Separate recognized flags from positional args. Any unrecognized --flag + // is rejected immediately so it is never silently absorbed into positional slots. + const RECOGNIZED_FLAGS = new Set(['--dry-run']); + let dryRun = false; + const positional: string[] = []; + + for (const arg of args) { + if (arg.startsWith('--')) { + if (!RECOGNIZED_FLAGS.has(arg)) { + throw new GSDError( + `Unknown flag ${arg} for phase.add`, + ErrorClassification.Validation, + ); + } + if (arg === '--dry-run') dryRun = true; + } else { + positional.push(arg); + } + } + + const description = positional[0]; if (!description) { throw new GSDError('description required for phase add', ErrorClassification.Validation); } @@ -207,27 +231,32 @@ export const phaseAdd: QueryHandler = async (args, projectDir, workstream) => { } catch { /* use defaults */ } const slug = generateSlugInternal(description); - const customId = args[1] || null; + // positional[1] is the optional customId — flags are already stripped + const customId = positional[1] || null; // Optional project code prefix (e.g., 'CK' -> 'CK-01-foundation') const projectCode = (config.project_code as string) || ''; assertSafeProjectCode(projectCode); const prefix = projectCode ? `${projectCode}-` : ''; - let newPhaseId: number | string = ''; - let dirName = ''; + // ── Helper: compute newPhaseId / dirName / computedPhaseEntry from raw ROADMAP content ── + // Extracted as a local async function so it can be called both inside the + // roadmap lock (non-dry-run) and outside (dry-run, where no write occurs and + // there is no race condition to guard against). + const computePhaseFields = async (rawRoadmapContent: string) => { + const milestoneContent = await extractCurrentMilestone(rawRoadmapContent, projectDir); - await readModifyWriteRoadmapMd(projectDir, async (rawContent) => { - const content = await extractCurrentMilestone(rawContent, projectDir); + let resolvedPhaseId: number | string = ''; + let resolvedDirName = ''; if (customId || config.phase_naming === 'custom') { // Custom phase naming - newPhaseId = customId || slug.toUpperCase().replace(/-/g, '_'); - if (!newPhaseId) { + resolvedPhaseId = customId || slug.toUpperCase().replace(/-/g, '_'); + if (!resolvedPhaseId) { throw new GSDError('--id required when phase_naming is "custom"', ErrorClassification.Validation); } - assertSafePhaseDirName(String(newPhaseId), 'custom phase id'); - dirName = `${prefix}${newPhaseId}-${slug}`; + assertSafePhaseDirName(String(resolvedPhaseId), 'custom phase id'); + resolvedDirName = `${prefix}${resolvedPhaseId}-${slug}`; } else { // Sequential mode: find highest integer phase number (in current milestone only) // Skip 999.x backlog phases — they live outside the active sequence @@ -235,67 +264,94 @@ export const phaseAdd: QueryHandler = async (args, projectDir, workstream) => { const phasePattern = /(?:^|\n)\s*(?:[-*]\s*(?:\[[x ]\]\s*)?|#{2,4}\s*|\*{1,2}\s*)Phase\s+(\d+)[A-Z]?(?:\.\d+)*:/gi; let maxPhase = 0; let m: RegExpExecArray | null; - while ((m = phasePattern.exec(content)) !== null) { + while ((m = phasePattern.exec(milestoneContent)) !== null) { const num = parseInt(m[1], 10); if (num >= 999) continue; // backlog phases use 999.x numbering if (num > maxPhase) maxPhase = num; } - // Belt-and-suspenders: if ROADMAP scan found nothing, fall back to scanning - // .planning/phases/ directory names as the canonical source of truth - if (maxPhase === 0) { - const phasesDir = planningPaths(projectDir, workstream).phases; - try { - const entries = await readdir(phasesDir, { withFileTypes: true }); - for (const entry of entries) { - if (!entry.isDirectory()) continue; - const dirMatch = /^(?:[A-Z][A-Z0-9]*-)?(\d+)[A-Z]?(?:\.\d+)*-/i.exec(entry.name); - if (dirMatch) { - const num = parseInt(dirMatch[1], 10); - if (num >= 999) continue; - if (num > maxPhase) maxPhase = num; - } - } - } catch { - // phases dir may not exist yet — leave maxPhase as 0 + // Also scan on-disk phase directories (union semantics) — ROADMAP and disk + // may diverge temporarily (e.g. a previous run created the dir but didn't + // update ROADMAP). Taking the max of both sources prevents collisions. + const phasesDir = planningPaths(projectDir, workstream).phases; + try { + const entries = await readdir(phasesDir, { withFileTypes: true }); + for (const entry of entries) { + if (!entry.isDirectory()) continue; + const dirMatch = /^(?:[A-Z][A-Z0-9]*-)?(\d+)[A-Z]?(?:\.\d+)*-/i.exec(entry.name); + if (!dirMatch) continue; + const num = parseInt(dirMatch[1], 10); + if (num >= 999) continue; + if (num > maxPhase) maxPhase = num; } + } catch { + // phases dir may not exist yet — leave maxPhase as 0 } - newPhaseId = maxPhase + 1; - const paddedNum = String(newPhaseId).padStart(2, '0'); - dirName = `${prefix}${paddedNum}-${slug}`; + resolvedPhaseId = maxPhase + 1; + const paddedNum = String(resolvedPhaseId).padStart(2, '0'); + resolvedDirName = `${prefix}${paddedNum}-${slug}`; } - assertSafePhaseDirName(dirName); + assertSafePhaseDirName(resolvedDirName); - const dirPath = join(planningPaths(projectDir, workstream).phases, dirName); + if (!resolvedDirName) { + throw new GSDError('Phase directory name was not computed', ErrorClassification.Execution); + } + if (resolvedPhaseId === '') { + throw new GSDError('Phase ID was not computed', ErrorClassification.Execution); + } - // Create directory with .gitkeep so git tracks empty folders - await mkdir(dirPath, { recursive: true }); - await writeFile(join(dirPath, '.gitkeep'), '', 'utf-8'); - - // Build phase entry 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`; + : `\n**Depends on:** Phase ${typeof resolvedPhaseId === 'number' ? resolvedPhaseId - 1 : 'TBD'}`; + const resolvedEntry = `\n### Phase ${resolvedPhaseId}: ${description}\n\n**Goal:** [To be planned]\n**Requirements**: TBD${dependsOn}\n**Plans:** 0 plans\n\nPlans:\n- [ ] TBD (run /gsd-plan-phase ${resolvedPhaseId} to break down)\n`; - // Find insertion point: before last "---" or at end - const lastSeparator = rawContent.lastIndexOf('\n---'); - if (lastSeparator > 0) { - return rawContent.slice(0, lastSeparator) + phaseEntry + rawContent.slice(lastSeparator); - } - return rawContent + phaseEntry; - }, workstream); + return { resolvedPhaseId, resolvedDirName, resolvedEntry }; + }; - if (!dirName) { - throw new GSDError('Phase directory name was not computed', ErrorClassification.Execution); - } - if (newPhaseId === '') { - throw new GSDError('Phase ID was not computed', ErrorClassification.Execution); + let newPhaseId: number | string = ''; + let dirName = ''; + let computedPhaseEntry = ''; + + if (dryRun) { + // Dry-run: no write, no race condition — compute outside the lock. + const roadmapPath = planningPaths(projectDir, workstream).roadmap; + let rawRoadmapContent = ''; + try { + rawRoadmapContent = await readFile(roadmapPath, 'utf-8'); + } catch { /* ROADMAP.md may not exist yet */ } + + const { resolvedPhaseId, resolvedDirName, resolvedEntry } = await computePhaseFields(rawRoadmapContent); + newPhaseId = resolvedPhaseId; + dirName = resolvedDirName; + computedPhaseEntry = resolvedEntry; + } else { + // Real write path: hold the roadmap lock across the entire read → compute → write + // cycle so that two concurrent phase.add calls cannot both observe the same + // maxPhase and produce duplicate phase IDs. + await readModifyWriteRoadmapMd(projectDir, async (roadmapRaw) => { + const { resolvedPhaseId, resolvedDirName, resolvedEntry } = await computePhaseFields(roadmapRaw); + newPhaseId = resolvedPhaseId; + dirName = resolvedDirName; + computedPhaseEntry = resolvedEntry; + + const dirPath = join(planningPaths(projectDir, workstream).phases, dirName); + + // Create directory with .gitkeep so git tracks empty folders + await mkdir(dirPath, { recursive: true }); + await writeFile(join(dirPath, '.gitkeep'), '', 'utf-8'); + + // Find insertion point: before last "---" or at end + const lastSeparator = roadmapRaw.lastIndexOf('\n---'); + if (lastSeparator > 0) { + return roadmapRaw.slice(0, lastSeparator) + computedPhaseEntry + roadmapRaw.slice(lastSeparator); + } + return roadmapRaw + computedPhaseEntry; + }, workstream); } - const result = { + const result: Record = { phase_number: typeof newPhaseId === 'number' ? newPhaseId : String(newPhaseId), padded: typeof newPhaseId === 'number' ? String(newPhaseId).padStart(2, '0') : String(newPhaseId), name: description, @@ -304,6 +360,11 @@ export const phaseAdd: QueryHandler = async (args, projectDir, workstream) => { naming_mode: config.phase_naming || 'sequential', }; + if (dryRun) { + result.dry_run = true; + result.roadmap_entry = computedPhaseEntry; + } + return { data: result }; };