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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* fix(#1161): preserve only date-shaped completion cells (self-heal garbage); correct test helper for 5-col

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* docs(#1161): add changeset fragment for completion-date idempotence

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-06-13 21:46:10 -04:00
committed by GitHub
parent ae8bb707bc
commit 4db185da74
4 changed files with 317 additions and 8 deletions

View File

@@ -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.

View File

@@ -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('|') + '|';
});

View File

@@ -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('|') + '|';
});

View File

@@ -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 <N>')` 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', () => {