From 76dd22deedcebee45f1b33540438a9f14ff97150 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 22 May 2026 14:54:27 -0400 Subject: [PATCH] fix(3774): treat 999 as exact sentinel in phase-lifecycle-policy (#93) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(3774): treat 999 as exact sentinel, not lower bound, in phase-lifecycle-policy scanSequentialMaxPhaseFromMilestone and scanSequentialMaxPhaseFromDirs used `num >= 999` to skip the backlog lane, but this incorrectly excluded every phase ≥ 1000, causing computeNextSequentialPhaseId to return 1 for projects using canonical phase IDs in the 1000+ range. Change both guards to `num === 999` so only the backlog sentinel is skipped. Adds regression test: project with phases 1000–1500 must produce 1501, not 1. Co-Authored-By: Claude Sonnet 4.6 * chore: add changeset for fix #3792 (phase.add returns 1 on 1000+ projects) Co-Authored-By: Claude Sonnet 4.6 * fix(3774): address review — fix 4 CJS scanner twins + tighten regression test Addresses gsd-code-reviewer BLOCKER (4 CJS scanner twins in phase.cjs:610,624,688,698 still carried >= 999, reachable via GSD_WORKSTREAM / absent SDK build) and MAJOR (regression test couldn't distinguish === 999 from === 1000 — added [999, 1000] fixture asserting result === 1001). Decrement helpers at :893, :922, :930, :936 left unchanged — intentional 999-lane protection per dual-review analysis. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Claude Sonnet 4.6 --- .changeset/bold-orcas-howl.md | 5 ++ get-shit-done/bin/lib/phase.cjs | 8 +- sdk/src/query/phase-lifecycle-policy.ts | 6 +- sdk/src/query/phase-lifecycle.test.ts | 101 +++++++++++++++++++++++- tests/phase.test.cjs | 56 +++++++++++++ 5 files changed, 168 insertions(+), 8 deletions(-) create mode 100644 .changeset/bold-orcas-howl.md diff --git a/.changeset/bold-orcas-howl.md b/.changeset/bold-orcas-howl.md new file mode 100644 index 000000000..a38617bff --- /dev/null +++ b/.changeset/bold-orcas-howl.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3792 +--- +phase.add no longer returns 1 on projects using canonical phase IDs 1000 or higher. diff --git a/get-shit-done/bin/lib/phase.cjs b/get-shit-done/bin/lib/phase.cjs index f197ac9ba..664aea800 100644 --- a/get-shit-done/bin/lib/phase.cjs +++ b/get-shit-done/bin/lib/phase.cjs @@ -607,7 +607,7 @@ function cmdPhaseAdd(cwd, description, raw, customId) { let m; while ((m = phasePattern.exec(content)) !== null) { const num = parseInt(m[1], 10); - if (num >= 999) continue; // backlog phases use 999.x numbering + if (num === 999) continue; // backlog phases use 999.x numbering if (num > maxPhase) maxPhase = num; } @@ -621,7 +621,7 @@ function cmdPhaseAdd(cwd, description, raw, customId) { const match = entry.match(dirNumPattern); if (!match) continue; const num = parseInt(match[1], 10); - if (num >= 999) continue; // skip backlog orphans + if (num === 999) continue; // skip backlog orphans if (num > maxPhase) maxPhase = num; } } @@ -685,7 +685,7 @@ function cmdPhaseAddBatch(cwd, descriptions, raw) { let m; while ((m = phasePattern.exec(content)) !== null) { const num = parseInt(m[1], 10); - if (num >= 999) continue; + if (num === 999) continue; if (num > maxPhase) maxPhase = num; } const phasesOnDisk = path.join(planningDir(cwd), 'phases'); @@ -695,7 +695,7 @@ function cmdPhaseAddBatch(cwd, descriptions, raw) { const match = entry.match(dirNumPattern); if (!match) continue; const num = parseInt(match[1], 10); - if (num >= 999) continue; + if (num === 999) continue; if (num > maxPhase) maxPhase = num; } } diff --git a/sdk/src/query/phase-lifecycle-policy.ts b/sdk/src/query/phase-lifecycle-policy.ts index b891cabce..4232d27e5 100644 --- a/sdk/src/query/phase-lifecycle-policy.ts +++ b/sdk/src/query/phase-lifecycle-policy.ts @@ -60,7 +60,7 @@ export function extractOneLinerFromBody(content: string): string | null { /** * Scan highest sequential phase number in milestone content. - * Skips backlog lanes (`999.x`). + * Skips exactly the backlog sentinel (`999`); phases 1000+ are valid canonical IDs. */ export function scanSequentialMaxPhaseFromMilestone(milestoneContent: string): number { const phasePattern = /(?:^|\n)\s*(?:[-*]\s*(?:\[[x ]\]\s*)?|#{2,4}\s*|\*{1,2}\s*)Phase\s+(\d+)[A-Z]?(?:\.\d+)*:/gi; @@ -68,7 +68,7 @@ export function scanSequentialMaxPhaseFromMilestone(milestoneContent: string): n let m: RegExpExecArray | null; while ((m = phasePattern.exec(milestoneContent)) !== null) { const num = parseInt(m[1], 10); - if (num >= 999) continue; + if (num === 999) continue; if (num > maxPhase) maxPhase = num; } return maxPhase; @@ -85,7 +85,7 @@ export function scanSequentialMaxPhaseFromDirs(dirNames: string[]): number { const match = dirNumPattern.exec(dirName); if (!match) continue; const num = parseInt(match[1], 10); - if (num >= 999) continue; + if (num === 999) continue; if (num > maxPhase) maxPhase = num; } return maxPhase; diff --git a/sdk/src/query/phase-lifecycle.test.ts b/sdk/src/query/phase-lifecycle.test.ts index a79faee3e..44f8221b5 100644 --- a/sdk/src/query/phase-lifecycle.test.ts +++ b/sdk/src/query/phase-lifecycle.test.ts @@ -325,7 +325,7 @@ describe('phaseAdd', () => { expect(roadmap).toContain('**Goal:** [To be planned]'); }); - it('skips phases >= 999 when calculating next number (backlog exclusion)', async () => { + it('skips exactly phase 999 (backlog sentinel) when calculating next number', async () => { const { phaseAdd } = await import('./phase-lifecycle.js'); const roadmapWith999 = MINIMAL_ROADMAP.replace( '---\n*Last updated', @@ -339,6 +339,105 @@ describe('phaseAdd', () => { expect(data.phase_number).toBe(11); }); + it('returns correct next phase id for projects using 1000+ canonical phase numbers (regression #3774)', async () => { + // Bug: scanSequentialMaxPhaseFromMilestone and scanSequentialMaxPhaseFromDirs + // used `num >= 999` instead of `num === 999`, causing every phase ≥ 1000 to be + // excluded from the max-scan. computeNextSequentialPhaseId returned 1 (0+1) + // instead of 1501 for a project whose highest phase is 1500. + const { phaseAdd } = await import('./phase-lifecycle.js'); + + const roadmapWith1000Plus = [ + '# Roadmap', + '', + '## Current Milestone: v10.0 Large Project', + '', + '### Phase 1000: Foundation', + '', + '**Goal:** Foundation', + '**Requirements**: TBD', + '**Plans:** 1 plans', + '', + 'Plans:', + '- [x] 1000-01 (Foundation setup)', + '', + '### Phase 1500: Latest', + '', + '**Goal:** Latest', + '**Requirements**: TBD', + '**Depends on:** Phase 1499', + '**Plans:** 1 plans', + '', + 'Plans:', + '- [x] 1500-01 (Latest step)', + '', + '---', + '*Last updated: 2026-05-20*', + '', + ].join('\n'); + + const phases = [ + '1000-foundation', + '1100-alpha', + '1200-beta', + '1300-gamma', + '1400-delta', + '1500-latest', + ]; + + await setupTestProject(tmpDir, { roadmap: roadmapWith1000Plus, phases }); + + const result = await phaseAdd(['Next After 1500'], tmpDir); + const data = result.data as Record; + // Must be 1501, not 1 (the pre-fix bug value) + expect(data.phase_number).toBe(1501); + }); + + it('distinguishes === 999 guard from === 1000 off-by-one: [999, 1000] fixture must yield 1001 (regression #3774)', async () => { + // Distinguishing fixture: phases [999, 1000] on disk + in ROADMAP. + // With correct guard (=== 999): skips 999, keeps 1000 as max → next = 1001 ✓ + // With off-by-one guard (=== 1000): skips 1000, keeps 999 → max = 999 but + // 999 is itself skipped by the equality guard (999 === 999 is true when + // the guard fires), so actually keeps nothing → max = 0 → next = 1 ✗. + // Either way, the result diverges from 1001, catching the regression. + const { phaseAdd } = await import('./phase-lifecycle.js'); + + const roadmapWith999and1000 = [ + '# Roadmap', + '', + '## Current Milestone: v1.0', + '', + '### Phase 999: Backlog', + '', + '**Goal:** Backlog sentinel', + '**Plans:** 0 plans', + '', + '### Phase 1000: First Four-Digit Phase', + '', + '**Goal:** First canonical phase above backlog sentinel', + '**Requirements**: TBD', + '**Plans:** 1 plans', + '', + 'Plans:', + '- [x] 1000-01 (initial work)', + '', + '---', + '*Last updated: 2026-05-21*', + '', + ].join('\n'); + + const phases = [ + '999-backlog', + '1000-first-four-digit', + ]; + + await setupTestProject(tmpDir, { roadmap: roadmapWith999and1000, phases }); + + const result = await phaseAdd(['After One Thousand'], tmpDir); + const data = result.data as Record; + // Must be 1001: skips 999 (backlog sentinel), keeps 1000 as max, adds 1 + expect(data.phase_number).toBe(1001); + }); + it('throws GSDError with Validation for empty description', async () => { const { phaseAdd } = await import('./phase-lifecycle.js'); await setupTestProject(tmpDir); diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index d2780b05e..0f2aee81b 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -932,6 +932,62 @@ describe('phase add command', () => { 'directory should be 04-dashboard, not 1000-dashboard' ); }); + + test('CJS scanner [999, 1000] fixture: skips exactly 999 and returns 1001 (regression #3774)', () => { + // Locks the BLOCKER fix in phase.cjs: guards at :610, :624, :688, :698 must + // use === 999 (not >= 999). With >= 999, phase 1000 is excluded from the + // max-scan and the result collapses back toward 1 instead of 1001. + // + // GSD_WORKSTREAM=ws1 forces the CJS fallback in phase-command-router.cjs. + // When GSD_WORKSTREAM is set, planningDir resolves to + // .planning/workstreams// — so ROADMAP.md and phases/ live there. + const ws = 'ws1'; + const planningBase = path.join(tmpDir, '.planning', 'workstreams', ws); + fs.mkdirSync(path.join(planningBase, 'phases'), { recursive: true }); + + fs.writeFileSync( + path.join(planningBase, 'ROADMAP.md'), + [ + '# Roadmap', + '', + '## Current Milestone: v1.0', + '', + '### Phase 999: Backlog', + '', + '**Goal:** Backlog sentinel', + '**Plans:** 0 plans', + '', + '### Phase 1000: First Four-Digit Phase', + '', + '**Goal:** First canonical phase above backlog sentinel', + '**Requirements**: TBD', + '**Plans:** 1 plans', + '', + 'Plans:', + '- [x] 1000-01 (initial work)', + '', + '---', + '*Last updated: 2026-05-21*', + '', + ].join('\n') + ); + + // Create matching phase directories on disk (inside the workstream planning dir) + fs.mkdirSync(path.join(planningBase, 'phases', '999-backlog'), { recursive: true }); + fs.mkdirSync(path.join(planningBase, 'phases', '1000-first-four-digit'), { recursive: true }); + + const result = runGsdTools('phase add After One Thousand', tmpDir, { GSD_WORKSTREAM: ws }); + assert.ok(result.success, `CJS phase add failed: ${result.error}`); + + const output = JSON.parse(result.output); + // Must be 1001: skips 999 (backlog sentinel), keeps 1000, adds 1. + // With the old >= 999 guard: phase 1000 is excluded → max stays 0 → result = 1. + assert.strictEqual(output.phase_number, 1001, 'CJS scanner must return 1001, not 1 (regression #3774)'); + assert.ok( + fs.existsSync(path.join(planningBase, 'phases', '1001-after-one-thousand')), + 'directory should be 1001-after-one-thousand' + ); + }); }); // ─────────────────────────────────────────────────────────────────────────────