* test: reproduce phase.add dry-run + flag validation gaps (#3226) Add failing tests for: - --dry-run silently absorbed into description (symptom A) - Unknown --flag should return validation error (symptom C) - ### Phase N: ROADMAP heading scan verification (symptom B) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(sdk): phase.add honors --dry-run; rejects unknown flags (#3226) - Add flag parser to phaseAdd: strip recognized flags (--dry-run) from args before positional parsing so they never silently become description or customId values - --dry-run computes the next phase number and roadmap_entry string but skips mkdir, writeFile, and readModifyWriteRoadmapMd; returns { dry_run: true, roadmap_entry } alongside normal fields - Any unrecognized --flag throws a Validation GSDError naming the flag - ROADMAP ### Phase N: heading scan for numbering (symptom B) was already correct; verified with new regression test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * changeset: pr=3246 for #3226 * fix(sdk): phase.add scans disk AND roadmap (union, not fallback) Address CodeRabbit finding: the conditional `if (maxPhase === 0)` guard around the filesystem scan meant that if ROADMAP had any phases but disk was ahead (e.g. ROADMAP max=10, dirs include 12-*), phase.add would pick 11 and collide with the existing directory. Remove the guard: always scan on-disk phase directories and take the max across both ROADMAP and filesystem (union semantics). All 57 phase-lifecycle tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: reproduce phase.add concurrent ID collision (CR finding) Two concurrent phase.add calls against the same project observe maxPhase before the lock is held, producing duplicate phase IDs. Adds a Promise.all regression test that asserts both calls succeed with distinct phase numbers {11, 12}. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(sdk): compute phase number under roadmap lock (CR finding) Move maxPhase/newPhaseId/dirName computation inside the readModifyWriteRoadmapMd callback so the entire read → compute → write cycle is serialised under the lock. Previously, two concurrent phase.add calls could both observe maxPhase=N before either acquired the lock, then both write with phase ID N+1 — producing duplicate IDs. In dry-run mode (no write, no race) the computation still happens outside the lock to avoid unnecessary contention. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
5
.changeset/nimble-deer-chatter.md
Normal file
5
.changeset/nimble-deer-chatter.md
Normal file
@@ -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.
|
||||
@@ -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<string, unknown>;
|
||||
|
||||
// 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<string, unknown>;
|
||||
|
||||
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<string, unknown>;
|
||||
|
||||
// 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<string, unknown>).phase_number as number;
|
||||
const n2 = (r2.data as Record<string, unknown>).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 ─────────────────────────────────────────────────────
|
||||
|
||||
@@ -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<string, unknown> = {
|
||||
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 };
|
||||
};
|
||||
|
||||
|
||||
Reference in New Issue
Block a user