diff --git a/.changeset/humble-sloths-forage.md b/.changeset/humble-sloths-forage.md new file mode 100644 index 000000000..6a7470f6f --- /dev/null +++ b/.changeset/humble-sloths-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3081 +--- +**`milestone complete` no longer silently disarms its unstarted-phase guard when STATE.md's `milestone:` field drifts** — the guard now runs whenever the ROADMAP can be scoped for the requested version (independent of STATE), and a STATE mismatch emits a WARNING naming both values instead of skipping the scan. (#2946) diff --git a/docs/CLI-TOOLS.md b/docs/CLI-TOOLS.md index 4004b109a..7d430e493 100644 --- a/docs/CLI-TOOLS.md +++ b/docs/CLI-TOOLS.md @@ -533,13 +533,25 @@ if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi ```bash # Archive milestone -node gsd-tools.cjs milestone complete [--name ] [--no-archive-phases] +node gsd-tools.cjs milestone complete [--name ] [--no-archive-phases] [--force] [--dry-run] # Mark requirements as complete node gsd-tools.cjs requirements mark-complete # Accepts: REQ-01,REQ-02 or REQ-01 REQ-02 or [REQ-01, REQ-02] ``` +**`milestone complete` flags** + +| Flag | Description | +|------|-------------| +| `` | Milestone version label to archive (e.g. `v1.0`). | +| `--name ` | Display name for the MILESTONES.md entry. Defaults to ``. | +| `--no-archive-phases` | Leave phase directories in place instead of moving them into `.planning/milestones/-phases/`. | +| `--force` | Override the unstarted-phase guard (see below). | +| `--dry-run` | Print the archive plan (roadmap, requirements, phases to move) without mutating anything. | + +**Unstarted-phase guard.** Before archiving, the command scans the ROADMAP scoped for `` and refuses if any `### Phase N:` heading in that slice has no matching phase directory on disk (`disk_status: no_directory`). Phase 0 (pre-milestone) and Phase 999 (backlog) sentinels are excluded. The guard runs whenever `--force` is absent, independent of `STATE.md`'s `milestone:` field — if that field is present but does not match ``, a WARNING naming both values is emitted to stderr and the scan still runs (#2946). Pass `--force` to override. + --- ## Agent Skills diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index acf384afc..774a7bc14 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -472,6 +472,8 @@ If any category is non-empty you are prompted with `[R] Resolve` / `[A] Acknowle > **Note:** the `deferred-items.md` category is the per-phase SCOPE BOUNDARY log a phase agent writes when it finds a defect it should not fix. It is a different artifact from the `## Deferred Items` section `[A]` writes into `STATE.md`, which records what you acknowledged at close. +> **Unstarted-phase guard.** Archiving refuses if the milestone's ROADMAP still lists a phase with no phase directory on disk — `Cannot mark milestone complete: ROADMAP lists N unstarted phase(s)`. If a phase was intentionally deferred or merged without a directory, run `gsd-tools milestone complete --force` (the `/gsd-complete-milestone` workflow runs the underlying command without `--force`, so use the CLI directly to override). A `STATE.md` `milestone:` value that does not match `` prints a WARNING and still runs the guard (#2946). + --- ### `/gsd-milestone-summary` diff --git a/src/milestone.cts b/src/milestone.cts index 6ce333999..e9babe30e 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -527,13 +527,25 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo // Guard: prevent marking complete when ROADMAP still lists phases that have // no directory on disk (disk_status: no_directory). This catches the case // where the active milestone was erroneously marked complete before phases - // were even started. Only fires when STATE.md confirms the current milestone - // version matches what is being completed — no false positives on fresh - // projects where phases haven't been scaffolded yet. + // were even started. The scan scopes the ROADMAP via the `version` argument + // (getMilestonePhaseFilter / extractCurrentMilestone above) and runs whenever + // --force is absent — a fresh project with no `### Phase N:` headings in the + // scoped slice yields an empty `noDirectoryPhases` and the guard is a no-op, + // so no STATE match is required to avoid false positives. // Pass --force to override this guard. + // + // #2946: the scan used to be nested inside `if (stateVersion && stateVersion + // === version)`, which silently disarmed the guard whenever STATE.md's + // `milestone:` field was desynced or absent — functionally an implicit + // --force on a one-way-door operation (ROADMAP/REQUIREMENTS archived, phase + // directories MOVED). The STATE field is not the source of truth for which + // phases belong to this milestone; the ROADMAP scoping is. The scan now runs + // unconditionally, and a present-but-mismatched STATE field emits a WARNING + // so the suspicious condition is visible rather than silent. if (!options.force) { try { - // Only guard when STATE.md's milestone field matches the version being completed. + // Read STATE.md's milestone field only to detect a suspicious mismatch; + // it no longer gates the scan. (#2946) let stateVersion: string | null = null; try { const stateRaw = fs.existsSync(statePath) ? fs.readFileSync(statePath, 'utf-8') : null; @@ -542,57 +554,75 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo if (milestoneMatch) stateVersion = milestoneMatch[1].trim(); } } catch { - /* skip */ + /* skip — stateVersion stays null, scan still runs */ + } + if (stateVersion !== null && stateVersion !== version) { + // #2946: emit a WARNING so the suspicious STATE mismatch is visible + // rather than silently disarming the guard. Plain-text diagnostic on + // stderr, matching the existing [gsd-tools] WARNING convention + // (state.cts). A missing STATE.md `milestone:` field is not warned + // here — the scan still runs, and "no milestone declared" is a normal + // state for a fresh project, not a suspicious drift. + // + // `stateVersion` comes from a user-controlled file (STATE.md) and is + // not validated like the CLI `version` arg (ARCHIVE_VERSION_LABEL_RE). + // Sanitize before interpolating into stderr so ANSI escapes / control + // chars / secret-looking strings cannot be echoed verbatim into a CI + // log or terminal (CONTRIBUTING.md security: secret-looking values in + // stderr). `version` is already constrained to [A-Za-z0-9._-]. + const safeStateVersion = stateVersion.replace(/[\x00-\x1f\x7f]/g, '?').slice(0, 80); + process.stderr.write( + `[gsd-tools] WARNING: STATE.md milestone: "${safeStateVersion}" ≠ requested "${version}" — ` + + `running the unstarted-phase guard against the ROADMAP scoped for "${version}" anyway.\n`, + ); } - if (stateVersion && stateVersion === version) { - const roadmapContent = fs.readFileSync(roadmapPath, 'utf-8'); - const scopedContent = extractCurrentMilestone(roadmapContent, cwd); - // #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE). - const phasePattern = new RegExp(`#{2,4}\\s*Phase\\s+(${PHASE_NUMBER_TOKEN_SOURCE})(?:\\s*\\([^)\\n]{0,200}\\))?\\s*:\\s*([^\\n]+)`, 'gi'); - const noDirectoryPhases: string[] = []; - let pm: RegExpExecArray | null; - const phaseDirEntries = ((): string[] => { - try { - return fs - .readdirSync(phasesDir, { withFileTypes: true }) - .filter((e) => e.isDirectory()) - .map((e) => e.name); - } catch { - return []; - } - })(); - while ((pm = phasePattern.exec(scopedContent)) !== null) { - const phaseNum = pm[1]; - // Phase 0 (pre-milestone) and Phase 999 (backlog) are sentinels, not - // real phases — they legitimately have no directory and must not block - // milestone completion. Mirrors the engine-wide sentinel convention - // (phase-id getMilestoneFromPhaseId, roadmap-command-router SENTINELS, - // the #1445 /^999/ progress filters). (#1580) - const major = parseInt(phaseNum, 10); - if (major === 0 || major === 999) continue; - const normalized = normalizePhaseName(phaseNum); - // A phase has disk_status: 'no_directory' when no phase directory - // with a matching token exists on disk. Use the same phaseTokenMatches - // helper that roadmap.analyze uses to avoid false positives on decimal - // (2.1) and letter-suffix (12A) phase IDs. - const hasDirectory = phaseDirEntries.some((d) => phaseTokenMatches(d, normalized)); - if (!hasDirectory) { - noDirectoryPhases.push(phaseNum); - } + const roadmapContent = fs.readFileSync(roadmapPath, 'utf-8'); + const scopedContent = extractCurrentMilestone(roadmapContent, cwd); + // #1729: `(?:\s*\([^)\n]{0,200}\))?` tolerates a pre-colon ( ) tag (literal mirror of OPTIONAL_PHASE_TAG_SOURCE). + const phasePattern = new RegExp(`#{2,4}\\s*Phase\\s+(${PHASE_NUMBER_TOKEN_SOURCE})(?:\\s*\\([^)\\n]{0,200}\\))?\\s*:\\s*([^\\n]+)`, 'gi'); + const noDirectoryPhases: string[] = []; + let pm: RegExpExecArray | null; + const phaseDirEntries = ((): string[] => { + try { + return fs + .readdirSync(phasesDir, { withFileTypes: true }) + .filter((e) => e.isDirectory()) + .map((e) => e.name); + } catch { + return []; } - if (noDirectoryPhases.length > 0) { - error( - `Cannot mark milestone complete: ROADMAP lists ${noDirectoryPhases.length} unstarted phase(s) ` + - `(e.g. Phase ${noDirectoryPhases[0]}). Re-run with --force to override.`, - ); + })(); + while ((pm = phasePattern.exec(scopedContent)) !== null) { + const phaseNum = pm[1]; + // Phase 0 (pre-milestone) and Phase 999 (backlog) are sentinels, not + // real phases — they legitimately have no directory and must not block + // milestone completion. Mirrors the engine-wide sentinel convention + // (phase-id getMilestoneFromPhaseId, roadmap-command-router SENTINELS, + // the #1445 /^999/ progress filters). (#1580) + const major = parseInt(phaseNum, 10); + if (major === 0 || major === 999) continue; + const normalized = normalizePhaseName(phaseNum); + // A phase has disk_status: 'no_directory' when no phase directory + // with a matching token exists on disk. Use the same phaseTokenMatches + // helper that roadmap.analyze uses to avoid false positives on decimal + // (2.1) and letter-suffix (12A) phase IDs. + const hasDirectory = phaseDirEntries.some((d) => phaseTokenMatches(d, normalized)); + if (!hasDirectory) { + noDirectoryPhases.push(phaseNum); } } + if (noDirectoryPhases.length > 0) { + error( + `Cannot mark milestone complete: ROADMAP lists ${noDirectoryPhases.length} unstarted phase(s) ` + + `(e.g. Phase ${noDirectoryPhases[0]}). Re-run with --force to override.`, + ); + } } catch (e) { // If the error came from our guard, re-throw it; otherwise skip silently. const message = e instanceof Error ? e.message : String(e); if (message && message.startsWith('Cannot mark milestone complete:')) throw e; - // Phase scan failed or STATE version mismatch — allow completion to proceed. + // Phase scan failed (e.g. ROADMAP unreadable) — allow completion to proceed. } } diff --git a/tests/milestone-archive.test.cjs b/tests/milestone-archive.test.cjs index 706477a60..ca7c4f187 100644 --- a/tests/milestone-archive.test.cjs +++ b/tests/milestone-archive.test.cjs @@ -58,6 +58,10 @@ describe('bug #2684: milestone.complete forwards version to phases.archive', () path.join(tmpDir, '.planning', 'ROADMAP.md'), `# Roadmap\n\n### Phase 1: Foundation\n**Goal:** Setup\n`, ); + // #2946: the unstarted-phase guard now runs whenever --force is absent + // (independent of STATE.md). This test exercises version-forwarding, not + // the guard, so give Phase 1 a real directory so the scan is satisfied. + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-foundation'), { recursive: true }); const result = runSdkQuery(['milestone.complete', 'v2.5'], tmpDir); assert.ok(result.success, `Command failed: ${result.error}`); diff --git a/tests/milestone.test.cjs b/tests/milestone.test.cjs index 342714ae7..32dc11ddb 100644 --- a/tests/milestone.test.cjs +++ b/tests/milestone.test.cjs @@ -1514,7 +1514,7 @@ describe('milestone complete explicit version scope (#3043)', () => { ); fs.writeFileSync(path.join(tmpDir, '.planning', 'REQUIREMENTS.md'), '# Requirements\n'); - for (const [dir, liner] of [['103.old', 'old milestone A'], ['104.old', 'old milestone B'], ['108.new', 'new milestone']]) { + for (const [dir, liner] of [['103-old', 'old milestone A'], ['104-old', 'old milestone B'], ['108-new', 'new milestone']]) { const p = path.join(tmpDir, '.planning', 'phases', dir); fs.mkdirSync(p, { recursive: true }); fs.writeFileSync(path.join(p, 'SUMMARY.md'), `one-liner: ${liner}\n\n## Summary\n${liner.split(' ')[0]}\n`); @@ -1762,6 +1762,205 @@ describe('bug-978: milestone complete --force overrides unstarted-phase guard', }); } +// ──────────────────────────────────────────────────────────────────────── +// bug #2946: milestone complete unstarted-phase guard fails open on STATE desync +// ──────────────────────────────────────────────────────────────────────── +// +// The guard that refuses to archive a milestone while the ROADMAP still lists +// unstarted phases used to nest its entire ROADMAP scan inside +// `if (stateVersion && stateVersion === version)`. Any STATE.md `milestone:` +// value that did not exactly string-equal the version argument — a desynced +// value, or no `milestone:` field at all — skipped the scan with no warning, +// functionally equivalent to an implicit `--force`. The operation the guard +// fronts is a one-way door: ROADMAP.md and REQUIREMENTS.md are archived and +// phase directories are MOVED into `.planning/milestones/-phases/`. +// +// The scan was already driven by the `version` argument through +// getMilestonePhaseFilter / extractCurrentMilestone; the STATE match was a +// redundant second gate that shadowed and broke it. After the fix the scan +// runs whenever `--force` is absent and the ROADMAP can be scoped for the +// version; a present-but-mismatched STATE.md `milestone:` field additionally +// emits a WARNING naming both values. + +describe('bug #2946: unstarted-phase guard runs independent of STATE.md milestone field', () => { + const { test, beforeEach, afterEach } = require('node:test'); + const assert = require('node:assert/strict'); + const fs = require('fs'); + const path = require('path'); + const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); + + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject('gsd-bug-2946-'); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + /** + * Build a fixture where the guard MUST fire if it runs: + * - ROADMAP.md scopes for `version` (heading includes the version literal) + * and lists a `### Phase 2:` heading with no on-disk phase directory. + * - STATE.md `milestone:` field is shaped by `stateMode`: + * 'sync' → milestone: + * 'desync' → milestone: -closing + * 'absent' → no milestone: line in frontmatter + * 'no-file' → STATE.md not written at all + * The unstarted phase is a REAL phase number (Phase 0 / 999 are sentinels + * excluded by the scan, #1580, so they would not fire it). + */ + function makeFixture(tmpDir, version, stateMode) { + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap ${version}\n\n### Phase 2: Real Work\n**Goal:** Not started\n`, + ); + if (stateMode === 'no-file') return; + let frontmatter; + if (stateMode === 'sync') frontmatter = `milestone: ${version}\n`; + else if (stateMode === 'desync') frontmatter = `milestone: ${version}-closing\n`; + else frontmatter = ''; // 'absent' + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `---\n${frontmatter}---\n# State\n\n**Status:** In progress\n**Last Activity:** 2025-01-01\n**Last Activity Description:** Working\n`, + ); + } + + test('without --force the guard fires even when STATE.md milestone: desyncs from the requested version', () => { + makeFixture(tmpDir, 'v1.0', 'desync'); + const result = runGsdTools( + ['milestone', 'complete', 'v1.0', '--name', 'Regression Test', '--dry-run'], + tmpDir, + ); + assert.strictEqual(result.success, false, 'guard must fire on STATE desync, not fail open'); + assert.ok( + result.error.includes('Re-run with --force to override'), + `expected guard error message; got: ${result.error}`, + ); + }); + + test('without --force the guard fires even when STATE.md has no milestone: field', () => { + makeFixture(tmpDir, 'v1.0', 'absent'); + const result = runGsdTools( + ['milestone', 'complete', 'v1.0', '--name', 'Regression Test', '--dry-run'], + tmpDir, + ); + assert.strictEqual(result.success, false, 'guard must fire when milestone: field is absent'); + assert.ok( + result.error.includes('Re-run with --force to override'), + `expected guard error message; got: ${result.error}`, + ); + }); + + test('without --force the guard fires even when STATE.md does not exist', () => { + makeFixture(tmpDir, 'v1.0', 'no-file'); + const result = runGsdTools( + ['milestone', 'complete', 'v1.0', '--name', 'Regression Test', '--dry-run'], + tmpDir, + ); + assert.strictEqual(result.success, false, 'guard must fire when STATE.md is missing entirely'); + assert.ok( + result.error.includes('Re-run with --force to override'), + `expected guard error message; got: ${result.error}`, + ); + }); + + test('with --force the guard is bypassed even when STATE.md milestone: desyncs', () => { + makeFixture(tmpDir, 'v1.0', 'desync'); + const result = runGsdTools( + ['milestone', 'complete', 'v1.0', '--name', 'Regression Test', '--dry-run', '--force'], + tmpDir, + ); + assert.ok( + result.success, + `--force must override the guard on the desync path too; got: ${result.error}`, + ); + const out = JSON.parse(result.output); + assert.strictEqual(out.dry_run, true, 'preview should run past the guard with --force'); + }); + + test('a STATE milestone mismatch emits a warning naming both versions', () => { + makeFixture(tmpDir, 'v1.0', 'desync'); + const result = runGsdTools( + ['milestone', 'complete', 'v1.0', '--name', 'Regression Test', '--dry-run'], + tmpDir, + ); + // Guard fires (failure path) — the WARNING is written to stderr before + // the `Error:` line, so it appears in result.error. Assert on the stable + // operator-facing tokens (the WARNING marker and both version literals + // the operator must see), not the surrounding prose — the prose is a + // human formatter and may be reworded. + assert.strictEqual(result.success, false); + assert.ok( + result.error.includes('WARNING:'), + `expected a WARNING marker on stderr; got: ${result.error}`, + ); + assert.ok( + result.error.includes('v1.0-closing') && result.error.includes('v1.0'), + `warning should name both the STATE value and the requested version; got: ${result.error}`, + ); + }); + + test('no WARNING is emitted when STATE.md milestone: field is absent (fresh project is not suspicious drift)', () => { + // The scan still runs and fires (covered by the absent-field test above), + // but a missing milestone: declaration is a normal fresh-project state, + // not a mismatch — so no WARNING should accompany it. + makeFixture(tmpDir, 'v1.0', 'absent'); + const result = runGsdTools( + ['milestone', 'complete', 'v1.0', '--name', 'Regression Test', '--dry-run'], + tmpDir, + ); + assert.strictEqual(result.success, false, 'guard must still fire on absent field'); + assert.ok( + !result.error.includes('WARNING:'), + `no WARNING expected for an absent milestone: field; got: ${result.error}`, + ); + }); + + test('guard is a no-op when the scoped ROADMAP has no Phase headings (fresh project)', () => { + // STATE in sync, ROADMAP scopes for v1.0 but lists NO phase headings → + // scan yields zero unstarted phases, guard must not fire. + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap v1.0\n\nThis milestone has no phases yet.\n`, + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `---\nmilestone: v1.0\n---\n# State\n\n**Status:** In progress\n`, + ); + const result = runGsdTools( + ['milestone', 'complete', 'v1.0', '--name', 'Fresh', '--dry-run'], + tmpDir, + ); + assert.ok( + result.success, + `guard must be a no-op when the scoped slice has no phase headings; got: ${result.error}`, + ); + }); + + test('Phase 0 and Phase 999 sentinels do not fire the unstarted-phase guard', () => { + // STATE absent (the strictest case for the new guard). ROADMAP has only + // sentinel phases with no directories — they must be skipped (#1580). + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + `# Roadmap v1.0\n\n### Phase 0: Pre-milestone\n**Goal:** Setup\n\n### Phase 999: Backlog\n**Goal:** Later\n`, + ); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + `---\n---\n# State\n\n**Status:** In progress\n`, + ); + const result = runGsdTools( + ['milestone', 'complete', 'v1.0', '--name', 'Sentinel', '--dry-run'], + tmpDir, + ); + assert.ok( + result.success, + `Phase 0 / 999 sentinels must not fire the guard; got: ${result.error}`, + ); + }); +}); + // ──────────────────────────────────────────────────────────────────────── // Folded from tests/bug-2660-one-liner-extraction.test.cjs — consolidation epic #1969 (B3 #1972) // ────────────────────────────────────────────────────────────────────────