From 4db185da741f6300135463d2bc7f4f2ef1e39a6c Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 13 Jun 2026 21:46:10 -0400 Subject: [PATCH] fix(#1161): make phase complete idempotent for roadmap completion dates (#1177) * fix(#1161): preserve existing roadmap completion date on repeat phase complete Guard the Completed cell write in cmdRoadmapUpdatePlanProgress so that a non-empty, non-placeholder date is never overwritten on repeat invocations. Also routes the date source through realClock.today() so GSD_NOW_MS pins the written date deterministically in tests (clock seam parity with the rest of the codebase). Regression tests added to 4-phase-complete-cjs-regression.test.cjs. Co-Authored-By: Claude Opus 4.8 * fix(#1161): preserve completion date in cmdPhaseComplete (real handler) + drive regression via phase complete CLI - Import realClock in phase.cts; switch cmdPhaseComplete from bare `new Date()` to `realClock.today()` so the clock seam is honoured in tests - Apply preserve-or-stamp guard to both 5-col (cells[4]) and 4-col (cells[3]) paths in cmdPhaseComplete: keep existing non-empty, non-dash date; only stamp today on first completion - Rewrite the #1161 describe block in 4-phase-complete-cjs-regression.test.cjs to drive the real handler via runGsdTools('phase complete 1') end-to-end; cases (a)/(b)/(c)/(d) all FAIL against unfixed build and pass post-fix Co-Authored-By: Claude Opus 4.8 * fix(#1161): preserve only date-shaped completion cells (self-heal garbage); correct test helper for 5-col Co-Authored-By: Claude Opus 4.8 * docs(#1161): add changeset fragment for completion-date idempotence Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .changeset/bold-otters-travel.md | 5 + src/phase.cts | 12 +- src/roadmap.cts | 17 +- .../4-phase-complete-cjs-regression.test.cjs | 291 +++++++++++++++++- 4 files changed, 317 insertions(+), 8 deletions(-) create mode 100644 .changeset/bold-otters-travel.md diff --git a/.changeset/bold-otters-travel.md b/.changeset/bold-otters-travel.md new file mode 100644 index 000000000..af08fc307 --- /dev/null +++ b/.changeset/bold-otters-travel.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 1177 +--- +**`phase complete` no longer rewrites an existing roadmap completion date** — repeat runs on an already-`Complete` phase preserve the recorded `YYYY-MM-DD` date (4- and 5-column layouts); empty/`-`/non-date cells are still stamped with the current date. diff --git a/src/phase.cts b/src/phase.cts index d8ea084a2..216612106 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -29,6 +29,7 @@ import stateMod = require('./state.cjs'); import { platformWriteSync, platformReadSync, platformEnsureDir } from './shell-command-projection.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; import { deriveProgressFromRoadmap, clampPercent } from './phase-lifecycle.cjs'; +import { realClock } from './clock.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- uat-predicate.cjs is an export= CommonJS module import uatPredicate = require('./uat-predicate.cjs'); const { evaluateUatPassed } = uatPredicate; @@ -1339,7 +1340,7 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); const statePath = path.join(planningDir(cwd), 'STATE.md'); const phasesDir = path.join(planningDir(cwd), 'phases'); - const today = new Date().toISOString().split('T')[0]; + const today = realClock.today(); const phaseInfoRaw = findPhaseInternal(cwd, phaseNum); if (!phaseInfoRaw) { @@ -1408,14 +1409,19 @@ function cmdPhaseComplete(cwd: string, phaseNum: string, raw: boolean): void { ); roadmapContent = roadmapContent.replace(tableRowPattern, (fullRow) => { const cells = fullRow.split('|').slice(1, -1); + const dateShape = /^\d{4}-\d{2}-\d{2}$/; if (cells.length === 5) { cells[2] = ` ${summaryCount}/${planCount} `; cells[3] = ' Complete '; - cells[4] = ` ${today} `; + // Preserve only a valid ISO date (#1161: idempotent; self-heal garbage) + const existingDate5 = cells[4].trim(); + cells[4] = dateShape.test(existingDate5) ? cells[4] : ` ${today} `; } else if (cells.length === 4) { cells[1] = ` ${summaryCount}/${planCount} `; cells[2] = ' Complete '; - cells[3] = ` ${today} `; + // Preserve only a valid ISO date (#1161: idempotent; self-heal garbage) + const existingDate4 = cells[3].trim(); + cells[3] = dateShape.test(existingDate4) ? cells[3] : ` ${today} `; } return '|' + cells.join('|') + '|'; }); diff --git a/src/roadmap.cts b/src/roadmap.cts index f167c424d..9a2fd744a 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -8,6 +8,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { realClock } from './clock.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import core = require('./core.cjs'); const { escapeRegex, normalizePhaseName, phaseMarkdownRegexSource, phaseMarkdownRegexSourceExact, output, error, findPhaseInternal, phaseTokenMatches } = core; @@ -470,7 +471,7 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und const isComplete = summaryCount >= planCount; const status = isComplete ? 'Complete' : summaryCount > 0 ? 'In Progress' : 'Planned'; - const today = new Date().toISOString().split('T')[0]; + const today = realClock.today(); if (!fs.existsSync(roadmapPath)) { output({ updated: false, reason: 'ROADMAP.md not found', plan_count: planCount, summary_count: summaryCount }, raw, 'no roadmap'); @@ -487,19 +488,27 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und `^(\\|\\s*${phasePattern}\\.?\\s[^|]*(?:\\|[^\\n]*))$`, 'im' ); - const dateField = isComplete ? ` ${today} ` : ' '; roadmapContent = roadmapContent.replace(tableRowPattern, (fullRow) => { const cells = fullRow.split('|').slice(1, -1); // drop leading/trailing empty from split + const dateShape = /^\d{4}-\d{2}-\d{2}$/; if (cells.length === 5) { // 5-col: Phase | Milestone | Plans | Status | Completed cells[2] = ` ${summaryCount}/${planCount} `; cells[3] = ` ${status.padEnd(11)}`; - cells[4] = dateField; + // Preserve only a valid ISO date (#1161: idempotent; self-heal garbage) + const existingDate5 = cells[4].trim(); + cells[4] = isComplete + ? (dateShape.test(existingDate5) ? cells[4] : ` ${today} `) + : ' '; } else if (cells.length === 4) { // 4-col: Phase | Plans | Status | Completed cells[1] = ` ${summaryCount}/${planCount} `; cells[2] = ` ${status.padEnd(11)}`; - cells[3] = dateField; + // Preserve only a valid ISO date (#1161: idempotent; self-heal garbage) + const existingDate4 = cells[3].trim(); + cells[3] = isComplete + ? (dateShape.test(existingDate4) ? cells[3] : ` ${today} `) + : ' '; } return '|' + cells.join('|') + '|'; }); diff --git a/tests/4-phase-complete-cjs-regression.test.cjs b/tests/4-phase-complete-cjs-regression.test.cjs index dc9771dda..a9d1e2d67 100644 --- a/tests/4-phase-complete-cjs-regression.test.cjs +++ b/tests/4-phase-complete-cjs-regression.test.cjs @@ -30,7 +30,7 @@ const fs = require('node:fs'); const path = require('node:path'); const os = require('node:os'); -const { cleanup } = require('./helpers.cjs'); +const { cleanup, runGsdTools } = require('./helpers.cjs'); // ── Load cmdPhaseComplete directly from phase.cjs (bypass the SDK router) ──── // phase-command-router.cjs delegates to SDK when available; we must test the @@ -358,6 +358,295 @@ describe('issue #4 (CJS): cmdPhaseComplete — idempotency (blind-increment bug) }); }); +// ───────────────────────────────────────────────────────────────────────────── +// Regressions: phase complete preserves completion date (#1161) +// Tests drive the REAL handler (cmdPhaseComplete) via the CLI entry point +// `runGsdTools('phase complete ')` so the fix in phase.cts is exercised +// end-to-end rather than hitting the roadmap.cjs helper in isolation. +// ───────────────────────────────────────────────────────────────────────────── + +/** Extract the Completed cell from the progress table row for a given phase number. + * The Completed column is always the LAST cell, regardless of whether the table is + * 4-col (Phase | Plans | Status | Completed) or 5-col (Phase | Milestone | Plans | Status | Completed). + */ +function extractCompletedCell(roadmapContent, phaseNum) { + // Match the full progress table row whose first cell starts with the phase number. + // Use [^|\n] to avoid crossing line boundaries. Capture everything up to the final '|'. + const re = new RegExp(`^(\\|\\s*${phaseNum}[^|\\n]*(?:\\|[^|\\n]*)*)\\|\\s*$`, 'm'); + const m = roadmapContent.match(re); + if (!m) return null; + // m[1] = '| 01. Foundation | 1/1 | Complete | 2026-01-01 ' + // Split on '|' → ['', ' 01. Foundation ', ' 1/1 ', ' Complete ', ' 2026-01-01 '] + // Drop the leading empty string and take the last element. + const cells = m[1].split('|').slice(1); // drop leading '' + return cells[cells.length - 1].trim(); +} + +/** + * Build a minimal 4-col ROADMAP project fixture whose Phase 01 row already has + * the Completed cell set to `existingDate` and Status `Complete`. + * The phase directory has plan+summary so `phase complete 1` can run. + * + * @param {string} existingDate - value in the Completed cell ('2026-01-01', '-', ' ', etc.) + * @param {boolean} [alreadyComplete] - if true the checkbox is already checked and status Complete + */ +function create4ColFixture(existingDate, alreadyComplete = true) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1161-4col-')); + const planDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planDir, 'phases'); + fs.mkdirSync(phasesDir, { recursive: true }); + + const checkbox = alreadyComplete ? '[x]' : '[ ]'; + const checkboxSuffix = alreadyComplete ? ' (completed 2026-01-01)' : ''; + const status = alreadyComplete ? 'Complete ' : 'Not started'; + + const roadmap = [ + '# Roadmap', + '', + `- ${checkbox} Phase 01: Foundation${checkboxSuffix}`, + '- [ ] Phase 02: API', + '', + '### Phase 01: Foundation', + '**Goal:** Build the foundation', + '**Plans:** 1/1 plans complete', + '', + '### Phase 02: API', + '**Goal:** Build the API', + '', + '## Progress', + '', + '| Phase | Plans Complete | Status | Completed |', + '|-------|----------------|--------|-----------|', + `| 01. Foundation | 1/1 | ${status} | ${existingDate} |`, + '| 02. API | 0/1 | Not started | - |', + '', + ].join('\n'); + fs.writeFileSync(path.join(planDir, 'ROADMAP.md'), roadmap); + + const state = [ + '# State', + '', + '**Current Phase:** 01', + '**Current Phase Name:** Foundation', + '**Status:** In progress', + '**Current Plan:** 01-01', + '**Last Activity:** 2025-01-01', + '**Last Activity Description:** Working on phase 1', + '**Completed Phases:** 0', + '**Total Phases:** 2', + '**Progress:** 0%', + '', + ].join('\n'); + fs.writeFileSync(path.join(planDir, 'STATE.md'), state); + + const phase01Dir = path.join(phasesDir, '01-foundation'); + fs.mkdirSync(phase01Dir, { recursive: true }); + fs.writeFileSync(path.join(phase01Dir, '01-01-PLAN.md'), '# Plan 1\nDo the work.\n'); + fs.writeFileSync(path.join(phase01Dir, '01-01-SUMMARY.md'), '# Summary 1\nDone.\n'); + + fs.mkdirSync(path.join(phasesDir, '02-api'), { recursive: true }); + + return tmpDir; +} + +/** + * Build a minimal 5-col ROADMAP project fixture (Phase | Milestone | Plans | Status | Completed). + * Phase 01 row already has Completed cell set to `existingDate`. + */ +function create5ColFixture(existingDate, alreadyComplete = true) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1161-5col-')); + const planDir = path.join(tmpDir, '.planning'); + const phasesDir = path.join(planDir, 'phases'); + fs.mkdirSync(phasesDir, { recursive: true }); + + const checkbox = alreadyComplete ? '[x]' : '[ ]'; + const checkboxSuffix = alreadyComplete ? ' (completed 2026-01-01)' : ''; + const status = alreadyComplete ? 'Complete ' : 'Not started'; + + const roadmap = [ + '# Roadmap', + '', + `- ${checkbox} Phase 01: Foundation${checkboxSuffix}`, + '- [ ] Phase 02: API', + '', + '### Phase 01: Foundation', + '**Goal:** Build the foundation', + '**Plans:** 1/1 plans complete', + '', + '### Phase 02: API', + '**Goal:** Build the API', + '', + '## Progress', + '', + '| Phase | Milestone | Plans | Status | Completed |', + '|-------|-----------|-------|--------|-----------|', + `| 01. Foundation | v1.0 | 1/1 | ${status} | ${existingDate} |`, + '| 02. API | v1.0 | 0/1 | Not started | - |', + '', + ].join('\n'); + fs.writeFileSync(path.join(planDir, 'ROADMAP.md'), roadmap); + + const state = [ + '# State', + '', + '**Current Phase:** 01', + '**Current Phase Name:** Foundation', + '**Status:** In progress', + '**Current Plan:** 01-01', + '**Last Activity:** 2025-01-01', + '**Last Activity Description:** Working on phase 1', + '**Completed Phases:** 0', + '**Total Phases:** 2', + '**Progress:** 0%', + '', + ].join('\n'); + fs.writeFileSync(path.join(planDir, 'STATE.md'), state); + + const phase01Dir = path.join(phasesDir, '01-foundation'); + fs.mkdirSync(phase01Dir, { recursive: true }); + fs.writeFileSync(path.join(phase01Dir, '01-01-PLAN.md'), '# Plan 1\nDo the work.\n'); + fs.writeFileSync(path.join(phase01Dir, '01-01-SUMMARY.md'), '# Summary 1\nDone.\n'); + + fs.mkdirSync(path.join(phasesDir, '02-api'), { recursive: true }); + + return tmpDir; +} + +// Fixed historical instant — will never collide with a real today() in CI. +const PINNED_MS_1161 = Date.parse('2021-03-22T10:00:00.000Z'); +const PINNED_DATE_1161 = '2021-03-22'; +// Env passed to runGsdTools to pin the clock in the subprocess SUT. +const PINNED_CLOCK_ENV = { + GSD_TEST_MODE: '1', + GSD_NOW_MS: String(PINNED_MS_1161), +}; + +describe('regressions: phase complete preserves completion date (#1161)', () => { + let tmpDir; + + afterEach(() => { + cleanup(tmpDir); + }); + + // ── (a) 4-col: already Complete with a date — repeat phase complete must NOT overwrite ── + + test('#1161 (a): 4-col ROADMAP — repeat `phase complete 1` preserves existing Completed date', () => { + // Arrange: Row is already Complete with '2026-01-01'. + tmpDir = create4ColFixture('2026-01-01', true); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + + // Act: run `phase complete 1` via the real CLI handler, clock pinned to PINNED_DATE. + const result = runGsdTools('phase complete 1', tmpDir, PINNED_CLOCK_ENV); + assert.ok(result.success, `phase complete failed: ${result.error || result.output}`); + + // Assert: Completed cell must still be '2026-01-01', NOT the pinned '2021-03-22'. + const after = fs.readFileSync(roadmapPath, 'utf8'); + const completedCell = extractCompletedCell(after, '01'); + assert.strictEqual( + completedCell, + '2026-01-01', + `#1161 (a) FAILED: repeat phase complete on 4-col table overwrote the existing date.\n` + + `Expected '2026-01-01', got '${completedCell}'.\n` + + `Pinned clock was '${PINNED_DATE_1161}' — if that appears the date was overwritten.\n\n` + + `ROADMAP after:\n${after}`, + ); + }); + + // ── (b) 5-col: already Complete with a date — repeat phase complete must NOT overwrite ── + + test('#1161 (b): 5-col ROADMAP — repeat `phase complete 1` preserves existing Completed date', () => { + // Arrange: 5-col table row is already Complete with '2026-01-01'. + tmpDir = create5ColFixture('2026-01-01', true); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + + // Act: run `phase complete 1` via the real CLI handler, clock pinned to PINNED_DATE. + const result = runGsdTools('phase complete 1', tmpDir, PINNED_CLOCK_ENV); + assert.ok(result.success, `phase complete failed: ${result.error || result.output}`); + + // Assert: Completed cell must still be '2026-01-01', NOT the pinned '2021-03-22'. + const after = fs.readFileSync(roadmapPath, 'utf8'); + const completedCell5 = extractCompletedCell(after, '01'); + assert.strictEqual( + completedCell5, + '2026-01-01', + `#1161 (b) FAILED: repeat phase complete on 5-col table overwrote the existing date.\n` + + `Expected '2026-01-01', got '${completedCell5}'.\n` + + `Pinned clock was '${PINNED_DATE_1161}' — if that appears the date was overwritten.\n\n` + + `ROADMAP after:\n${after}`, + ); + }); + + // ── (c) First-time completion (placeholder '-') must stamp the pinned date ── + + test('#1161 (c): 4-col ROADMAP — first `phase complete 1` (placeholder date) stamps pinned date', () => { + // Arrange: Row has '-' as Completed cell and is Not started (never completed). + tmpDir = create4ColFixture('-', false); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + + // Act: first-time phase complete. + const result = runGsdTools('phase complete 1', tmpDir, PINNED_CLOCK_ENV); + assert.ok(result.success, `phase complete failed: ${result.error || result.output}`); + + // Assert: Completed cell is now the pinned date. + const after = fs.readFileSync(roadmapPath, 'utf8'); + const completedCell = extractCompletedCell(after, '01'); + assert.strictEqual( + completedCell, + PINNED_DATE_1161, + `#1161 (c) FAILED: first-time completion should stamp '${PINNED_DATE_1161}', got '${completedCell}'.\n\n` + + `ROADMAP after:\n${after}`, + ); + }); + + // ── (d) Whitespace-only Completed cell is treated as empty and gets stamped ── + + test('#1161 (d): 4-col ROADMAP — whitespace-only Completed cell treated as empty, gets stamped', () => { + // Arrange: Row has ' ' (spaces) as Completed cell. + tmpDir = create4ColFixture(' ', false); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + + // Act: first-time phase complete. + const result = runGsdTools('phase complete 1', tmpDir, PINNED_CLOCK_ENV); + assert.ok(result.success, `phase complete failed: ${result.error || result.output}`); + + // Assert: Completed cell is now the pinned date (whitespace was treated as empty). + const after = fs.readFileSync(roadmapPath, 'utf8'); + const completedCell = extractCompletedCell(after, '01'); + assert.strictEqual( + completedCell, + PINNED_DATE_1161, + `#1161 (d) FAILED: whitespace-only Completed cell should be stamped '${PINNED_DATE_1161}', got '${completedCell}'.\n\n` + + `ROADMAP after:\n${after}`, + ); + }); + + // ── (e) Non-date garbage in Completed cell is self-healed and gets re-stamped ── + + test('#1161 (e): 5-col ROADMAP — non-date garbage Completed cell is self-healed and re-stamped', () => { + // Arrange: 5-col row is already Complete but the Completed cell contains 'TBD' + // (a non-date garbage value that the old guard would have preserved). + tmpDir = create5ColFixture('TBD', true); + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + + // Act: run `phase complete 1` via the real CLI handler, clock pinned to PINNED_DATE. + const result = runGsdTools('phase complete 1', tmpDir, PINNED_CLOCK_ENV); + assert.ok(result.success, `phase complete failed: ${result.error || result.output}`); + + // Assert: garbage 'TBD' must be replaced with the pinned date (self-heal). + // Pre-Fix 2: the old guard (existingDate && existingDate !== '-') would preserve 'TBD'. + // Post-Fix 2: the date-shape guard (/^\d{4}-\d{2}-\d{2}$/) rejects 'TBD' → re-stamps. + const after = fs.readFileSync(roadmapPath, 'utf8'); + const completedCell = extractCompletedCell(after, '01'); + assert.strictEqual( + completedCell, + PINNED_DATE_1161, + `#1161 (e) FAILED: non-date garbage 'TBD' in Completed cell should be self-healed to '${PINNED_DATE_1161}', got '${completedCell}'.\n` + + `Old guard (non-empty && !== '-') would preserve 'TBD'. New guard must require a date shape.\n\n` + + `ROADMAP after:\n${after}`, + ); + }); +}); + // ── T2: Progress percent must never exceed 100% ────────────────────────────── describe('issue #4 (CJS): cmdPhaseComplete — progress percent clamp', () => {