From 8da48e22ae32f99e001374e34db312af0c14ce44 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 14 Jun 2026 17:52:15 -0400 Subject: [PATCH] fix(#1229): count bullet-only phases so phase.add stops reusing an existing phase number (#1249) * fix(#1229): count bullet-only phases + guard against number collision in phase.add Before this fix, the phase.add number scan only checked ### Phase N: section headers and on-disk phases/N-* directories. A phase that existed only as a roadmap bullet (e.g. "- [ ] **Phase 11: ...**") was invisible to both scans, causing phase.add to silently assign a duplicate number. Fix: add a bullet-entry regex scan (all checkbox variants: [ ], [x], [~], with or without ** bold markers) to the set-based phase-number collection in cmdPhaseAdd. Also added a post-compute collision guard that advances the candidate past any already-used number. Regression tests added to tests/phase.test.cjs (bug #1229 describe block): bullet-only collision, [x]/[~] variants, plain-bullet, and baseline preservation. Co-Authored-By: Claude Sonnet 4.6 * chore(#1229): add Fixed changeset fragment --------- Co-authored-by: Claude Sonnet 4.6 --- .changeset/eager-voles-howl.md | 5 + src/phase.cts | 36 +++- tests/phase.test.cjs | 325 +++++++++++++++++++++++++++++++++ 3 files changed, 358 insertions(+), 8 deletions(-) create mode 100644 .changeset/eager-voles-howl.md diff --git a/.changeset/eager-voles-howl.md b/.changeset/eager-voles-howl.md new file mode 100644 index 000000000..45acc4e6a --- /dev/null +++ b/.changeset/eager-voles-howl.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1249 +--- +**`phase add` no longer reuses an existing phase number when that phase exists only as a roadmap bullet** — the next-number scan now counts phases listed only as `- [ ] **Phase N: ...**` bullets (all checkbox variants, with or without a title), in addition to `### Phase N:` section headers and on-disk phase directories, so a bullet-only phase is no longer shadowed and `phase add` appends after the highest used number. diff --git a/src/phase.cts b/src/phase.cts index a0cb328f5..122afcbf0 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -702,15 +702,31 @@ function cmdPhaseAdd(cwd: string, description: string, raw: boolean, customId?: if (!_newPhaseId) error('--id required when phase_naming is "custom"'); _dirName = `${prefix}${_newPhaseId}-${slug}`; } else { - const phasePattern = /#{2,4}\s*Phase\s+(\d+)[A-Z]?(?:\.\d+)*:/gi; - let maxPhase = 0; + // Collect all phase numbers visible in the current-milestone content. + // Three sources are scanned so that a phase in ANY representation + // (section header, roadmap bullet, or on-disk directory) is counted: + + // 1) Section headers: ### Phase N: / ## Phase N: / #### Phase N: + const headerPattern = /#{2,4}\s*Phase\s+(\d+)[A-Z]?(?:\.\d+)*:/gi; + // 2) Roadmap bullet entries: - [ ] **Phase N: ...** (all checkbox variants) + // The lookahead accepts colon, decimal-dot, whitespace, bold-close asterisk, + // or end-of-line so titleless forms ("- [ ] **Phase 11**", "- [ ] Phase 11") + // are counted and cannot collide with a freshly-added phase. (#1229) + const bulletPattern = /^[ \t]*-[ \t]*\[[^\]]*\][ \t]*\*{0,2}Phase[ \t]+(\d+)(?=[:.\s*]|$)/gim; + + const usedPhaseNums = new Set(); let m: RegExpExecArray | null; - while ((m = phasePattern.exec(content)) !== null) { + + while ((m = headerPattern.exec(content)) !== null) { const num = parseInt(m[1], 10); - if (num === 999) continue; - if (num > maxPhase) maxPhase = num; + if (num !== 999) usedPhaseNums.add(num); + } + while ((m = bulletPattern.exec(content)) !== null) { + const num = parseInt(m[1], 10); + if (num !== 999) usedPhaseNums.add(num); } + // 3) On-disk phase directories (e.g. phases/11-foo/ with no header yet) const phasesOnDisk = path.join(planningDir(cwd), 'phases'); if (fs.existsSync(phasesOnDisk)) { const dirNumPattern = /^(?:[A-Z][A-Z0-9]*-)?(\d+)-/; @@ -718,12 +734,16 @@ function cmdPhaseAdd(cwd: string, description: string, raw: boolean, customId?: const match = entry.match(dirNumPattern); if (!match) continue; const num = parseInt(match[1], 10); - if (num === 999) continue; - if (num > maxPhase) maxPhase = num; + if (num !== 999) usedPhaseNums.add(num); } } - _newPhaseId = maxPhase + 1; + // phase.add appends after the highest *used* number. Collecting numbers from + // section headers, roadmap bullets, AND on-disk dirs above is what prevents the + // #1229 collision (a bullet-only Phase N is now counted), so max+1 cannot reuse + // an existing number. + const maxUsed = usedPhaseNums.size > 0 ? Math.max(...usedPhaseNums) : 0; + _newPhaseId = maxUsed + 1; const paddedNum = String(_newPhaseId).padStart(2, '0'); _dirName = `${prefix}${paddedNum}-${slug}`; } diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 49b0875ea..bc7492ad2 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -4892,3 +4892,328 @@ describe('bug-3287 — init plan-phase exposes expected_phase_dir with project_c }); }); } + +// ───────────────────────────────────────────────────────────────────────────── +// bug #1229: phase.add bullet-only phase collision +// ───────────────────────────────────────────────────────────────────────────── + +// Count canonical Phase N entries in roadmap content (header or bullet form). +// Only counts "### Phase N:" headers and "- [ ] **Phase N:" bullet entries. +// Does NOT count references like "**Depends on:** Phase N". +function countBug1229PhaseNumber(roadmapContent, n) { + let count = 0; + const headerRe = new RegExp('^#{2,4}\\s*Phase\\s+' + n + '[A-Z]?(?:\\.\\d+)*:', 'gim'); + const bulletRe = new RegExp( + '^[ \\t]*-[ \\t]*\\[[^\\]]*\\][ \\t]*\\*{0,2}Phase[ \\t]+' + n + '(?=[:.\\ \\t*]|$)', + 'gim', + ); + const headerMatches = roadmapContent.match(headerRe); + const bulletMatches = roadmapContent.match(bulletRe); + if (headerMatches) count += headerMatches.length; + if (bulletMatches) count += bulletMatches.length; + return count; +} + +describe('bug #1229: phase.add must count bullet-only phases to avoid number collision', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('bullet-only Phase 11 is counted: next add gets Phase 12, not 11', () => { + // ROADMAP has Phases 1-3 as full sections and Phase 11 as bullet-only. + // Before the fix, maxPhase resolved to 3 (header scan) and phase.add + // silently produced Phase 4, then on a second add would produce Phase 11 + // — or if headers went to 10, it would produce Phase 11 colliding with + // the existing bullet. + const roadmap = [ + '# Roadmap v1.0', + '', + '## Phases', + '', + '- [ ] **Phase 1: Foundation**', + '- [ ] **Phase 2: Core**', + '- [x] **Phase 3: Done**', + '- [ ] **Phase 11: Communications / Zoho Sync**', + '', + '### Phase 1: Foundation', + '', + '**Goal:** Build foundations', + '', + '### Phase 2: Core', + '', + '**Goal:** Core work', + '', + '### Phase 3: Done', + '', + '**Goal:** Completed work', + '', + '---', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + + // Create disk dirs for phases 1, 2, 3 (not 11 -- that is bullet-only) + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-foundation'), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '02-core'), { recursive: true }); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '03-done'), { recursive: true }); + + const result = runGsdTools('phase add New Feature', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 12, + `Expected phase 12 (bullet-only Phase 11 must be counted), got ${output.phase_number}`, + ); + + // Verify no duplicate Phase 11 written + const updatedRoadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + const phase11Count = countBug1229PhaseNumber(updatedRoadmap, 11); + assert.ok( + phase11Count === 1, + `ROADMAP must have exactly 1 occurrence of Phase 11 (no duplicate), found ${phase11Count}`, + ); + + // Verify Phase 12 was written + assert.ok( + updatedRoadmap.includes('### Phase 12:'), + 'ROADMAP must contain new ### Phase 12: entry', + ); + + // Verify directory was created at 12, not 11 + assert.ok( + fs.existsSync(path.join(tmpDir, '.planning', 'phases', '12-new-feature')), + 'phases/12-new-feature directory must be created', + ); + assert.ok( + !fs.existsSync(path.join(tmpDir, '.planning', 'phases', '11-new-feature')), + 'phases/11-new-feature must NOT be created (collision guard)', + ); + }); + + test('[x] checkbox variant bullet phase is counted', () => { + // Phase 5 exists only as a [x] bullet (completed, no dir, no header) + const roadmap = [ + '# Roadmap v1.0', + '', + '### Phase 1: Foundation', + '', + '**Goal:** Setup', + '', + '### Phase 2: API', + '', + '**Goal:** Build', + '', + '### Phase 3: UI', + '', + '**Goal:** Interfaces', + '', + '### Phase 4: Deploy', + '', + '**Goal:** Ship it', + '', + '- [x] **Phase 5: Post-launch Cleanup**', + '', + '---', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + + const result = runGsdTools('phase add Follow-up Work', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 6, + `Expected phase 6 ([x] bullet-only Phase 5 must be counted), got ${output.phase_number}`, + ); + }); + + test('[~] checkbox variant bullet phase is counted', () => { + // Phase 7 exists only as a [~] bullet (in-progress, no dir, no header) + const roadmap = [ + '# Roadmap v1.0', + '', + '### Phase 1: Foundation', + '', + '**Goal:** Setup', + '', + '- [~] **Phase 7: Partial Work**', + '', + '---', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + + const result = runGsdTools('phase add Next Phase', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 8, + `Expected phase 8 ([~] bullet-only Phase 7 must be counted), got ${output.phase_number}`, + ); + }); + + test('baseline: no bullet-only phases -- existing behavior preserved', () => { + const roadmap = [ + '# Roadmap v1.0', + '', + '### Phase 1: Foundation', + '', + '**Goal:** Setup', + '', + '### Phase 2: API', + '', + '**Goal:** Build', + '', + '---', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + + const result = runGsdTools('phase add Third Phase', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 3, + `Expected phase 3 (normal sequential add), got ${output.phase_number}`, + ); + }); + + test('bullet without ** bold markers is counted', () => { + // Phase 6 as plain bullet without ** markdown bold + const roadmap = [ + '# Roadmap v1.0', + '', + '### Phase 1: Foundation', + '', + '**Goal:** Setup', + '', + '### Phase 2: Core', + '', + '**Goal:** Core', + '', + '- [ ] Phase 6: Plain bullet no bold', + '', + '---', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + + const result = runGsdTools('phase add Another Phase', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 7, + `Expected phase 7 (plain-bullet Phase 6 must be counted), got ${output.phase_number}`, + ); + }); + + test('titleless bold bullet "- [ ] **Phase 11**" is counted: next add gets Phase 12', () => { + // Regression for the adversarial-review finding: the original bulletPattern + // required a colon or whitespace after the digits, so "- [ ] **Phase 11**" + // (bold-close immediately after the number) was silently skipped and phase.add + // would assign Phase 11 again — the exact collision class bug #1229 fixes. + const roadmap = [ + '# Roadmap v1.0', + '', + '### Phase 1: Foundation', + '', + '**Goal:** Setup', + '', + '### Phase 2: Core', + '', + '**Goal:** Core', + '', + '- [ ] **Phase 11**', + '', + '---', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + + const result = runGsdTools('phase add Titleless Bold Follow-up', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 12, + `Expected phase 12 (titleless bold bullet "**Phase 11**" must be counted), got ${output.phase_number}`, + ); + + const updatedRoadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.ok( + updatedRoadmap.includes('### Phase 12:'), + 'ROADMAP must contain new ### Phase 12: entry', + ); + assert.ok( + fs.existsSync(path.join(tmpDir, '.planning', 'phases', '12-titleless-bold-follow-up')), + 'phases/12-titleless-bold-follow-up directory must be created', + ); + assert.ok( + !fs.existsSync(path.join(tmpDir, '.planning', 'phases', '11-titleless-bold-follow-up')), + 'phases/11-titleless-bold-follow-up must NOT be created (collision guard)', + ); + }); + + test('EOL bullet "- [ ] Phase 11" (no title, no bold) is counted: next add gets Phase 12', () => { + // Regression: "- [ ] Phase 11" at end-of-line was not matched by the original + // pattern whose trailing [:\s] requires at least one character after the digits. + const roadmap = [ + '# Roadmap v1.0', + '', + '### Phase 1: Foundation', + '', + '**Goal:** Setup', + '', + '### Phase 2: Core', + '', + '**Goal:** Core', + '', + '- [ ] Phase 11', + '', + '---', + ].join('\n'); + + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), roadmap); + + const result = runGsdTools('phase add EOL Follow-up', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const output = JSON.parse(result.output); + assert.strictEqual( + output.phase_number, + 12, + `Expected phase 12 (EOL bullet "Phase 11" must be counted), got ${output.phase_number}`, + ); + + const updatedRoadmap = fs.readFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), 'utf-8'); + assert.ok( + updatedRoadmap.includes('### Phase 12:'), + 'ROADMAP must contain new ### Phase 12: entry', + ); + assert.ok( + fs.existsSync(path.join(tmpDir, '.planning', 'phases', '12-eol-follow-up')), + 'phases/12-eol-follow-up directory must be created', + ); + assert.ok( + !fs.existsSync(path.join(tmpDir, '.planning', 'phases', '11-eol-follow-up')), + 'phases/11-eol-follow-up must NOT be created (collision guard)', + ); + }); +});