diff --git a/.changeset/2562-workstream-progress-milestone-scoping.md b/.changeset/2562-workstream-progress-milestone-scoping.md new file mode 100644 index 000000000..bff498ee4 --- /dev/null +++ b/.changeset/2562-workstream-progress-milestone-scoping.md @@ -0,0 +1,17 @@ +--- +type: Fixed +pr: 2588 +--- +**`workstream progress` / `workstream status` / `workstream list` no longer report a workstream's CURRENT milestone as "milestone complete" / 100% while phases in that milestone are unstarted, in progress, or failing verification.** Three coupled defects in the shared inventory derivation are fixed. (1) The shipped signal was project-lifetime rather than milestone-scoped — `workstreamMilestoneShipped()` returned true if ANY `*-ROADMAP.md` snapshot existed or `SHIPPED` appeared anywhere in `ROADMAP.md`, and since every previously shipped milestone leaves a permanent collapsed `✅ … SHIPPED` block, any workstream that had ever shipped was pinned to "milestone complete" forever (an over-correction from #1913). It now requires the CURRENT version's archived `milestones/-ROADMAP.md` snapshot, or the current milestone's own ROADMAP line marked shipped; `REQUIREMENTS` snapshots are deliberately not accepted because they can be written at milestone start. (2) The completion percentage silently excluded phases declared for the current milestone but never scaffolded, while completed PRIOR-milestone phase directories inflated the numerator — both numerator and denominator are now scoped to the current milestone, whose phase set is read from the ROADMAP `## Progress` table (which lists phases with no directory) via the canonical `findTableWithColumns` parser, with the current version taken from the workstream `STATE.md` `milestone:` field rather than ROADMAP in-progress markers, which can be stale. (3) Phase completeness ignored the verification verdict — a phase with `SUMMARY` count ≥ `PLAN` count now counts as `in_progress` rather than `complete` when its verdict is an explicit failing one (`gaps_found`/`human_needed`); `missing`/`unknown`/`stale` are intentionally untouched so verifier-disabled projects do not regress to never-complete. + +Two further denominator gaps are closed. A phase declared as a `## Progress` table row with **no `### Phase N` heading** was dropped by the heading-only count *even when other headings existed* (the regex counts 1 for a "1 heading + 1 table-only" roadmap), and milestone scoping could not cover it because a flat Progress table carries no per-phase milestone attribution — so greenfield and single-milestone projects kept the faulty count. When scoping cannot engage, the denominator is now the union of the Progress table's declared phase numbers and the phase directories, so neither source can shrink it. Separately, a sub-phase directory inserted mid-milestone (`30.1-…` under a table-declared phase 30) has no table row of its own and previously had no milestone attribution at all; it now inherits its parent phase's milestone and joins BOTH sides of the calculation — numerator-only would let `completed_phases` exceed a denominator that never counted it and cap back to 100%, reintroducing the reported defect. Attribution is one-directional (a sub-phase counts only when its parent is in the current milestone), so a follow-up created in a later milestone under an older parent is excluded rather than misattributed. + +Membership and the denominator are derived from a single canonical phase-key surface, promoted to the phase-id owner module as `phaseKeyFromToken` / `phaseKeyFromDir` / `phaseKeyFromProse` / `parentPhaseKey` (previously a private pair in `state.cts`). Deriving one side of a comparison with a bespoke regex was itself a way to reproduce this issue: a padded `| 01. … |` table row never matched a `1-slug` directory, and a project-code-prefixed `PROJ-05-…` directory matched nothing at all — each silently zeroing or pinning the rollup while `phases[]` reported the opposite. Directory membership additionally consults `getMilestonePhaseFilter`, the module that owns milestone-phase filtering, which now accepts a workstream name so its `planningDir` resolution can target `.planning/workstreams//` (a loop over workstreams cannot express that through `GSD_WORKSTREAM`) and exposes `versionScoped` so its phase count is never mistaken for a current-milestone denominator on an unversioned roadmap. "Milestone shipped" detection likewise moved to that module as `isMilestoneShippedInRoadmap`: heading and `` lines only — a bullet such as `- [x] 03-01: ship the v2.0 login endpoint ✅` is prose about a phase, not a milestone verdict — with the version token boundary-matched so a shipped `v2.0.1` heading cannot close `v2.0`. A ROADMAP row whose Milestone cell is blank or malformed now stays in the denominator instead of vanishing from both sides, and a stale directory colliding on phase number with a current one (Bug #2445's scenario) counts once; the Builder asserts `completed_phases <= denominator` and throws rather than letting `Math.min` round a contradiction up to 100%. `getMilestonePhaseFilter` still applies its own internal phase-id normaliser for directory matching rather than routing through `phase-id.cts`; the two signals are OR'd, so a divergence can only widen membership, never narrow it — but they remain two normalisers, not one. + +A **declared-but-empty current milestone** is scoped rather than treated as unscoped. `STATE.md`'s `milestone:` field updates the moment `/gsd-new-milestone` writes the heading, while the `## Progress` table and phase sections land later; in that window nothing attributes a phase to the current milestone, scoping switched off entirely, and the fallback counted the project's whole phase history as both numerator and denominator — reporting 100% for a milestone with no work done, the same symptom by a different route. Three witnesses now distinguish that state, each covering a ROADMAP shape the others miss: `getMilestonePhaseFilter` gained `versionSectionFound` (the milestone's section exists but declares no phases — `versionScoped` cannot answer this, because a located-but-empty section falls through to the zero-count pass-all degrade that resets it), the existing `missingExplicitVersion` (a versioned roadmap with no section for this version), and a Progress table attributing every row to another milestone. A ROADMAP that attributes no versions anywhere matches none of them — its rows parse unattributed and stay in the current milestone — so free-form legacy projects keep their whole-roadmap count instead of regressing to 0%. Within an empty milestone, membership inverts: a phase directory belongs unless another milestone's row claims it, so a phase scaffolded before the roadmap catches up is counted rather than dropped from both sides. Scoping is now stated by the caller (`milestoneScoped`) instead of inferred from `currentMilestonePhaseCount > 0`, which could not represent "scoped and legitimately zero-phase". + +**`status` is cross-validated against the milestone's own artifacts, not asserted from the shipped marker alone.** Scoping the marker to the current milestone stopped a PRIOR milestone pinning `status` to "milestone complete", but the marker was still echoed as fact for the current one — so a single payload could report `status: "milestone complete"` beside `progress_percent: 67`, which is this issue's own symptom reached through `status`. The two shipped signals are now distinguished and cross-checked at different strengths, because one check cannot serve both. A `heading` signal (an operator-typed `✅ SHIPPED` in the LIVE roadmap) is refused when the milestone's completion ratio is short, which also catches phases declared but never scaffolded. A `snapshot` signal (`milestones/-ROADMAP.md`) is not gated on that ratio alone: the `milestone complete` run that writes it also moves the milestone's phase directories into `milestones/-phases/` while copying — never truncating — the live ROADMAP, so a CLEAN archive reads 0/N by construction and a bare ratio gate would strip "milestone complete" from every archived milestone in every project. But a phase directory still present under `phases/` means the archive is not clean — a phase was added or reopened after it, reachable because `milestone complete` does not advance `STATE.md`'s `milestone:` field — and once that is true the ratio is meaningful again, so the snapshot check is the conjunction of the two. The `legacy` project-lifetime fallback is ungated by signal, as before. The cross-check as a whole engages only when milestone scoping is active, for ALL three signals and not just `legacy`: with scoping off the denominator is the whole-roadmap count and membership is everything, so there is no current-milestone artifact set to check a current-milestone claim against. When a marker is refused, the `STATE.md` field is not accepted as a fallback claim of completion either — in this window it commonly asserts the same thing — so against contradicting artifacts neither source can report the milestone complete. + +Greenfield roadmaps without a versioned Progress table, and projects whose current milestone version cannot be determined, keep the previous behaviour. (#2562) + +**Behaviour change for consumers of the inventory JSON:** `roadmap_phase_count`, `completed_phases` and `progress_percent` now describe the workstream's CURRENT milestone rather than its lifetime, and there is no schema signal marking the change. Anything reading those fields — including `getOtherActiveWorkstreamInventories`, which filters completed workstreams out of the active list — sees real movement: a post-v1.0 workstream that reported `milestone complete` / 100% will now report its actual in-flight progress. The inventory also gains `milestone_shipped_unverified`: true when a shipped marker fired for the current milestone but its artifacts contradicted it. It is distinct from `status_conflict`, which continues to report only the derived-vs-`STATE.md`-field disagreement. `workstream list`, `workstream status` and `workstream progress` all project the new field, so a refused marker is visible at the CLI rather than collapsing silently into a fallback `status`. diff --git a/CONTEXT.md b/CONTEXT.md index bf0a3ba4a..c855867fa 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -15,7 +15,7 @@ Module owning `milestone complete` (archive roadmap/requirements/phases, build M Module that composes Dispatch Policy Module, Query Execution Policy Module, and per-stage handlers (input-validation, plan, execution, result-builder, formatting, error-mapping, observability) into the end-to-end pipeline that produces a `QueryDispatchResult`. The SDK-era pipeline collapsed onto the Command Routing Hub per ADR-0174; current dispatch seam: `gsd-core/bin/lib/command-routing-hub.cjs` (see Command Routing Hub below). ### Phase Id Module -Module owning the pure phase-id parsing and matching helpers: phase-name normalization, phase-token extraction/matching, milestone- and phase-dir id parsing, phase-markdown regex builders, and the ADR-612 bracket phase-id round-trip grammar (`escapeRegex`, `normalizePhaseName`, `comparePhaseNum`, `extractPhaseToken`, `phaseTokenMatches`, `phaseMarkdownRegexSource`/`phaseMarkdownRegexSourceExact`, `getMilestoneFromPhaseId`, `getPhaseDirFromPhaseId`, `parsePhaseId`/`renderPhaseId`/`toDir` over the `PhaseId` type, `isSentinelPhaseId`/`SENTINEL_RANGES`, and the `BRACKET_PHASE_TOKEN_SOURCE`/`PHASE_HEADING_PREFIX_SRC` grammar sources). Pure string/regex — no I/O, no config, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2a (#865) as the cycle-free leaf that unblocks the roadmap-parser and phase-locator extractions; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/phase-id.cjs` (generated from `src/phase-id.cts`). +Module owning the pure phase-id parsing and matching helpers: phase-name normalization, phase-token extraction/matching, milestone- and phase-dir id parsing, phase-markdown regex builders, and the ADR-612 bracket phase-id round-trip grammar (`escapeRegex`, `normalizePhaseName`, `comparePhaseNum`, `extractPhaseToken`, `phaseTokenMatches`, `phaseMarkdownRegexSource`/`phaseMarkdownRegexSourceExact`, `getMilestoneFromPhaseId`, `getPhaseDirFromPhaseId`, `parsePhaseId`/`renderPhaseId`/`toDir` over the `PhaseId` type, `isSentinelPhaseId`/`SENTINEL_RANGES`, and the `BRACKET_PHASE_TOKEN_SOURCE`/`PHASE_HEADING_PREFIX_SRC` grammar sources). Also owns the canonical phase KEY surface (#2562) — `phaseKeyFromToken`/`phaseKeyFromDir`/`phaseKeyFromProse`/`parentPhaseKey` — the padding-, case- and project-code-insensitive identity used whenever two independently-derived phase references (a ROADMAP table cell and a phase directory, say) are compared; deriving one side of such a comparison with a bespoke regex is what silently zeroed a rollup in #2562. Pure string/regex — no I/O, no config, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2a (#865) as the cycle-free leaf that unblocks the roadmap-parser and phase-locator extractions; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/phase-id.cjs` (generated from `src/phase-id.cts`). ### Phase Lifecycle Module Module owning phase create, rename, complete, remove, list, and plan-index operations, plus phase-dir prefix validation, STATE.md staleness detection, and auto-prune behaviour. Entry point: `gsd-core/bin/lib/phase.cjs` (CJS surface). Typed phase events: `GSDPhaseStartEvent`, `GSDPhaseStepStartEvent`, `GSDPhaseStepCompleteEvent`, `GSDPhaseCompleteEvent`. (The SDK native-query surface, the `types.ts` event definitions, `phase-runner.ts`, and `phase-prompt.ts` were retired with the SDK package per ADR-0174.) @@ -103,6 +103,8 @@ Module owning `.planning` path resolution, active workstream pointer policy (`se ### Workstream Inventory Module Module owning workstream directory discovery, per-workstream state projection, phase/plan/summary counting, roadmap-declared phase count, active marker projection, and active-workstream collision inputs. Command handlers render list/status/progress outputs from this inventory instead of rescanning `.planning/workstreams/*` directly. Source of truth for the pure projection is `gsd-core/bin/lib/workstream-inventory-builder.cjs` (a Builder Module); the Reader Adapter `gsd-core/bin/lib/workstream-inventory.cjs` collects filesystem inputs and delegates projection to the Builder. +Completion is scoped to the workstream's CURRENT milestone (#2562): `roadmap_phase_count`, `completed_phases` and `progress_percent` describe that one milestone, not the workstream's lifetime, and `status: "milestone complete"` is derived from the current milestone's own close artifacts (an archived `milestones/-ROADMAP.md` snapshot, or `isMilestoneShippedInRoadmap` on its own ROADMAP heading) rather than any project-lifetime shipped marker. That marker is a claim, not a verdict: it is cross-validated against the milestone's own artifacts, and the two signals are checked at different strengths because one check cannot serve both. A live-ROADMAP `heading` marker is refused when the completion ratio is short. An archived `snapshot` is not ratio-gated alone — `milestone complete` moves the milestone's phase directories into `milestones/-phases/` while copying, never truncating, the live ROADMAP, so a CLEAN archive reads 0/N by construction — and is refused on the conjunction of a still-live in-milestone directory (the archive is not clean; a phase was added or reopened after it, reachable because `milestone complete` does not advance `STATE.md`'s `milestone:` field) AND a short completion ratio. The `legacy` project-lifetime fallback is ungated by signal. The cross-check as a whole engages only under milestone scoping, for all three signals: unscoped, the denominator is the whole-roadmap count and membership is everything, so there is no current-milestone artifact set to check against. A refused marker does not fall through to a `STATE.md` field asserting the same thing, and the refusal is reported as `milestone_shipped_unverified`, projected by `workstream list`/`status`/`progress` so it is visible at the CLI — distinct from `status_conflict`, which reports only derived-vs-field disagreement. Membership and the denominator are derived from one canonical key surface — the phase-id owner module's `phaseKeyFromDir`/`phaseKeyFromProse` — with directory membership additionally consulting the Roadmap Parser Module's `getMilestonePhaseFilter` when that filter is genuinely version-scoped (that filter keeps its own internal normaliser, OR'd in, so a divergence can only widen membership); the Builder enforces `completed_phases <= denominator` and throws on violation rather than capping the percentage. A current milestone that is DECLARED but not yet populated is scoped, not unscoped: `versionSectionFound` / `missingExplicitVersion` / a Progress table attributing every row elsewhere each witness that state, and without them scoping switched off and the whole-lifetime fallback reported a milestone with no work done as 100%. Within such a milestone, membership inverts — a directory belongs unless another milestone's row claims it — so a phase scaffolded ahead of the ROADMAP is counted rather than hidden. Scoping is stated by the Reader (`milestoneScoped`) rather than inferred from `currentMilestonePhaseCount > 0`, which cannot represent a scoped, legitimately zero-phase milestone. A ROADMAP that attributes no versions at all matches none of the witnesses, so free-form legacy projects keep their whole-roadmap count. The Reader passes the workstream name to `getMilestonePhaseFilter`/`extractCurrentMilestone` so their `planningDir` resolution targets `.planning/workstreams//`, which a loop over workstreams cannot express through `GSD_WORKSTREAM`. + ### Project-Root Resolution Module Module owning project-root resolution from any starting directory. Walks the ancestor chain (bounded by `FIND_PROJECT_ROOT_MAX_DEPTH = 10`) applying five heuristics in order: (0) own `.planning/` guard (#1362), (1) parent `.planning/config.json` `sub_repos` traversal, (2) legacy `multiRepo: true` boolean + ancestor `.git`, (3) `.git` heuristic with parent `.planning/`, (4) nearest-ancestor `.planning/` walk-up (#1414, epic #1411) — a last-resort second walk (same depth bound, stops at `os.homedir()`) that anchors a plain descendant subdirectory of a single-repo project to its nearest ancestor `.planning/` instead of degrading to defaults; ordered after (1)–(3) so `sub_repos`/`multiRepo` resolution always wins (the Resolution Provenance deterministic-anchoring rule). Returns `startDir` when no ancestor qualifies. Sync `node:fs` I/O. Source of truth: `gsd-core/bin/lib/project-root.cjs`; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. @@ -173,7 +175,7 @@ Canonical GFM table parsing + schema registry seam (`gsd-core/bin/lib/markdown-t Shared fail-loud `Result` and per-surface write-set contracts (`gsd-core/bin/lib/write-set.cjs`, generated from `src/write-set.cts`; ADR-2143 §5/§6, epic #2143). Pure, Node built-ins only, no I/O. Exports: `Result` (`{ok:true,value}\|{ok:false,reason}` — ADR-2143 §5 fail-loud parse shape, never a bare `null` a caller can mistake for "empty but fine"; the single source of truth `markdown-table.cjs` re-exports so its existing importers are unaffected; deliberately distinct from command-routing-hub's dispatch `Result` `{ok,data\|kind}`); `WriteOutcome` (`{surface: string, applied: boolean}` — one surface's outcome within a multi-surface write); `WriteSet` (`WriteOutcome[]`); `writeSetComplete(ws) → boolean` (true only when the set is non-empty AND every surface applied — ADR-2143 §6's "no OR-into-one-flag" rule: a command that mutates more than one surface must not collapse independent surface outcomes into a single boolean, the anti-pattern that let a checkbox-only partial write (#2140) report full success). `milestone.cts`'s `requirements mark-complete` handler is the first consumer: it reports a `write_set` (`checkbox`/`traceability` surfaces) and `write_set_complete` alongside its existing `updated`/`marked_complete`/`already_complete`/`not_found`/`table_unmatched` fields, which remain computed exactly as before — the write-set is additive, structured ADR-2143 documentation of the same per-surface facts #2140's tactical fix already exposed via `table_unmatched`. ### Roadmap Parser Module -Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `withPhaseSection`). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`, and — since #1881 — `unusable-input` for the out-of-band diagnostic) — no `loadConfig`, no other core dependency. An unreadable ROADMAP.md is reported rather than collapsed into the same sentinel as a genuinely absent one; absence itself stays silent, and neither lookup gains a throw (the #2245 audit records that `src/state.cts` removed its defensive try/catch on the strength of `getMilestoneInfo` never throwing). Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). +Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `isMilestoneShippedInRoadmap`, `withPhaseSection`). Milestone shipped/active heading classification is owned here (#2562): `isMilestoneShippedInRoadmap(content, version)` answers "does the ROADMAP mark THIS milestone shipped" from heading and `` lines only — never a bullet that merely names the version — with the version token boundary-matched so `v2.0` does not match inside `v2.0.1`. `extractCurrentMilestone` and `getMilestonePhaseFilter` take an optional trailing workstream name so their `planningDir` resolution targets `.planning/workstreams//`; omitted, it resolves exactly as before (including the `GSD_WORKSTREAM` fallback). `getMilestonePhaseFilter` exposes `versionScoped`, true only when the returned phase set really is one milestone's — consumers must not read `phaseCount` as a current-milestone denominator otherwise — and `versionSectionFound`, true whenever the requested version's section was located at all. The two differ precisely for a located-but-EMPTY section: it falls through to the zero-count pass-all degrade, which resets `versionScoped` to false, leaving `versionSectionFound` the only surviving evidence that the milestone exists rather than being absent. `missingExplicitVersion` covers the complementary shape (versioned roadmap, no section for this version). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`, and — since #1881 — `unusable-input` for the out-of-band diagnostic) — no `loadConfig`, no other core dependency. An unreadable ROADMAP.md is reported rather than collapsed into the same sentinel as a genuinely absent one; absence itself stays silent, and neither lookup gains a throw (the #2245 audit records that `src/state.cts` removed its defensive try/catch on the strength of `getMilestoneInfo` never throwing). Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). ### Core Utilities Module Module owning the shared low-level utility primitives extracted from Core: POSIX path normalization (`toPosixPath`), filesystem scanning (`detectSubRepos`, `readSubdirectories`, `getPhaseFileStats`, `pathExistsInternal`), and small pure helpers (`generateSlugInternal`, `extractOneLinerFromBody`, `filterPlanFiles`, `filterSummaryFiles`, `extractCanonicalPlanId`, `timeAgo`). Depends only on Node built-ins and already-leafed modules (`phase-id` for `comparePhaseNum`, `planning-workspace` for `findContextMdIn`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2c (#877) as the shared leaf that unblocks the phase-locator fs-search extraction (2d); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/core-utils.cjs` (generated from `src/core-utils.cts`). diff --git a/src/phase-id.cts b/src/phase-id.cts index 67d6e102d..a8e43a875 100644 --- a/src/phase-id.cts +++ b/src/phase-id.cts @@ -568,6 +568,69 @@ function phaseTokenMatches(dirName: string, normalized: string): boolean { return false; } +// ─── Canonical phase KEY surface (#2562) ───────────────────────────────────── +// +// A phase "key" is the padding-, case- and project-code-insensitive identity of +// a phase, for use as a Map/Set key when two independently-derived phase +// references (a ROADMAP table cell and a phase directory name, say) must be +// compared. Promoted here from a local pair in state.cts (#2445) so every +// consumer derives BOTH sides of a comparison from the SAME function — deriving +// one side with a bespoke regex is the #2562 defect class (a `01` table cell +// never matching a `1-slug` directory, silently zeroing a rollup). + +/** + * Canonical key for an already-extracted phase TOKEN (`"5"`, `"05"`, `"005"`, + * `"12A"`, `"30.1"`, `"PROJ-05"`). Padding- and case-insensitive: every + * spelling of a number collapses to one key. + * + * Leading zeros are stripped per hyphen-separated segment BEFORE + * `normalizePhaseName` pads to the 2-digit convention. Padding alone is not a + * normalisation — `padStart(2)` is a no-op once the input is already ≥2 + * characters, so `5` yielded `05` while `005` stayed `005` and the two never + * compared equal. The strip is deliberately confined to this key surface: + * `normalizePhaseName` itself is a RENDERING function whose verbatim treatment + * of wide IDs (`001.10`) is relied on by plan-ID capture and wave assignment. + * Arithmetic is avoided (`parseInt` would lose precision on a long digit run). + */ +function phaseKeyFromToken(token: unknown): string { + const stripped = String(token) + .split('-') + .map(segment => segment.replace(/^0+(?=\d)/, '')) + .join('-'); + return normalizePhaseName(stripped).toUpperCase(); +} + +/** + * Canonical key for a phase DIRECTORY name (`"05-schedule-8"` → `"05"`, + * `"PROJ-5-x"` → `"05"`, `"30.1-follow-up"` → `"30.1"`). + */ +function phaseKeyFromDir(dirName: string): string { + return phaseKeyFromToken(extractPhaseToken(dirName)); +} + +/** + * Canonical key for a phase referenced in PROSE — a ROADMAP `## Progress` table + * cell (`"30. Schedule 8 rollout"`, `"**05.1 Follow-up**"`) or a STATE.md + * `Phase:` value. Markdown emphasis is stripped first so a bolded cell is not + * mistaken for a non-phase. Returns null when the value does not BEGIN with a + * phase token (`parsePhaseFromProse` anchoring, #2111). + */ +function phaseKeyFromProse(value: string | null | undefined): string | null { + if (value == null) return null; + const { phase } = parsePhaseFromProse(String(value).replace(/[*_`~]/g, '')); + return phase === null ? null : phaseKeyFromToken(phase); +} + +/** + * The PARENT phase key of a sub-phase key (`"30.1"` → `"30"`), or null for a + * top-level phase. A sub-phase directory inserted mid-milestone frequently has + * no ROADMAP row of its own and inherits its parent's milestone (#2562). + */ +function parentPhaseKey(key: string): string | null { + const dot = key.indexOf('.'); + return dot === -1 ? null : key.slice(0, dot); +} + // ─── #2121 canonical surface (ADR-2121) ────────────────────────────────────── /** @@ -711,6 +774,10 @@ export = { comparePhaseNum, extractPhaseToken, phaseTokenMatches, + phaseKeyFromToken, + phaseKeyFromDir, + phaseKeyFromProse, + parentPhaseKey, parsePhaseFromProse, stripConfiguredProjectCodePrefix, isForeignPrefixedPhaseQuery, diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index e80643d59..2e79aaebf 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -41,6 +41,18 @@ import type { HeadingToken } from './markdown-sectionizer.cjs'; // ─── Roadmap milestone scoping ─────────────────────────────────────────────── +/** + * Markers that classify a MILESTONE HEADING (or ``) as closed/shipped + * versus still active. Hoisted to module scope in #2562 — three call sites + * (`extractCurrentMilestone`, `currentMilestoneRawRanges`, + * `isMilestoneShippedInRoadmap`) previously kept byte-identical copies. + */ +const MILESTONE_CLOSED_MARKER_PATTERN = /\b(?:CLOSED|ARCHIVED|ABANDONED|SHIPPED|FAILED)\b|✅|🗄/i; +const MILESTONE_ACTIVE_MARKER_PATTERN = /\b(?:STARTED|ACTIVE|WIP)\b|in\s+progress|🚧|🔄/i; +function isClosedMilestoneHeading(headingText: string): boolean { + return MILESTONE_CLOSED_MARKER_PATTERN.test(headingText) && !MILESTONE_ACTIVE_MARKER_PATTERN.test(headingText); +} + /** * Strip shipped milestone content wrapped in
blocks. */ @@ -49,14 +61,58 @@ function stripShippedMilestones(content: string): string { } /** - * Extract the current milestone section from ROADMAP.md by positive lookup. + * #2562: is the milestone `version` marked SHIPPED by the ROADMAP itself? + * + * Scoped deliberately narrowly, because a false positive here reproduces the + * exact symptom #2562 reports ("milestone complete" while phases are unstarted): + * + * - Only a MILESTONE HEADING (`^#{1,3}` that is not a `Phase N:` heading) or a + * `` line can carry the signal. A bullet or checklist item that + * merely NAMES the version (`- [x] 03-01: ship the v2.0 login endpoint ✅`) + * is prose about a phase, not a milestone verdict, and is ignored. + * - The version token is boundary-matched with `(?![\w.-])` (mirrors the #730 + * sub-milestone boundary at `extractCurrentMilestone`), so `v2.0` does not + * match inside `v2.0.1` — `\b` alone would, since `.` is a non-word char. + * - Shipped/active classification reuses the same marker patterns the milestone + * sectioniser uses, so an in-progress marker on the line always wins. + * + * Both patterns are anchored and use only complementary character classes + * (`[^\n]`, `[^<]`, `[^>]`) with no overlapping alternation, so matching stays + * linear in the ROADMAP's length — an untrusted ROADMAP cannot drive backtracking. */ -function extractCurrentMilestone(content: string, cwd?: string): string { +function isMilestoneShippedInRoadmap(content: string, version: string): boolean { + const boundedVersion = `${escapeRegex(version)}(?![\\w.-])`; + const candidates = [ + // A milestone heading: `## v2.0 Launch — ✅ SHIPPED`. + new RegExp(`^#{1,3}[^\\S\\n]+(?!Phase\\s+\\S)[^\\n]*${boundedVersion}[^\\n]*$`, 'gmi'), + // A collapsed shipped block's own summary: `✅ v2.0 … SHIPPED`. + new RegExp(`]*>[^<]*${boundedVersion}[^<]*<\\/summary>`, 'gi'), + ]; + for (const pattern of candidates) { + for (const match of content.matchAll(pattern)) { + if (isClosedMilestoneHeading(match[0])) return true; + } + } + return false; +} + +/** + * Extract the current milestone section from ROADMAP.md by positive lookup. + * + * @param content - ROADMAP.md content. + * @param cwd - Project working directory, used to read the companion STATE.md + * for the current `milestone:` version. + * @param ws - #2562: workstream name, so the companion STATE.md is read from + * `.planning/workstreams//` instead of the project root. Omitted (the + * default) preserves the prior `planningDir(cwd)` resolution exactly, + * including its `GSD_WORKSTREAM` env fallback. + */ +function extractCurrentMilestone(content: string, cwd?: string, ws?: string | null): string { if (!cwd) return stripShippedMilestones(content); let version: string | null = null; try { - const statePath = path.join(planningDir(cwd), 'STATE.md'); + const statePath = path.join(planningDir(cwd, ws), 'STATE.md'); const stateRaw = platformReadSync(statePath); if (stateRaw !== null) { const milestoneMatch = stateRaw.match(/^milestone:\s*(.+)/m); @@ -113,9 +169,7 @@ function extractCurrentMilestone(content: string, cwd?: string): string { const allMatches = headingMatches; - const closedMarkerPattern = /\b(?:CLOSED|ARCHIVED|ABANDONED|SHIPPED|FAILED)\b|✅|🗄/i; - const activeMarkerPattern = /\b(?:STARTED|ACTIVE|WIP)\b|in\s+progress|🚧|🔄/i; - const isClosed = (h: string) => closedMarkerPattern.test(h) && !activeMarkerPattern.test(h); + const isClosed = isClosedMilestoneHeading; const firstMatch = allMatches[0]; const selected = allMatches.find((m) => !isClosed(m[1])) || firstMatch; @@ -519,6 +573,26 @@ function getMilestoneInfo(cwd: string): MilestoneInfo { type MilestonePhaseFilter = ((dirName: string) => boolean) & { phaseCount: number; missingExplicitVersion: boolean; + /** + * #2562: true only when `versionOverride` was supplied AND a matching + * milestone section was located, i.e. the phase set really is scoped to that + * one milestone. False for the whole-roadmap (unversioned) shape, where + * `phaseCount` spans the project's lifetime and must NOT be read as a + * current-milestone denominator. + */ + versionScoped: boolean; + /** + * #2562: true when `versionOverride`'s milestone section was LOCATED in the + * ROADMAP, independent of whether it turned out to declare any phases. + * `versionScoped` cannot answer that question — a located-but-empty section + * falls through to the zero-count pass-all filter below, which resets + * `versionScoped` to false, making "milestone absent" and "milestone present + * but not yet populated" indistinguishable. They are not the same state: the + * second is a real, empty current milestone, and a caller that treats it as + * "unscoped" silently reports the project's whole phase history as if it were + * the current milestone's. + */ + versionSectionFound: boolean; }; /** @@ -533,15 +607,23 @@ type MilestonePhaseFilter = ((dirName: string) => boolean) & { * free-form ROADMAPs that lack versioned milestone headings. When absent or * any other value, the warning is suppressed — legacy/default projects must * never see spurious warnings. + * @param ws - #2562: workstream name, so the ROADMAP/STATE pair is read from + * `.planning/workstreams//` instead of the project root. Required by any + * caller that iterates workstreams (it cannot set `GSD_WORKSTREAM` per + * iteration). Omitted (the default) preserves the prior `planningDir(cwd)` + * resolution exactly, including its `GSD_WORKSTREAM` env fallback — every + * pre-#2562 call site is unaffected. */ -function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, phaseIdConvention?: string | null): MilestonePhaseFilter { +function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, phaseIdConvention?: string | null, ws?: string | null): MilestonePhaseFilter { const milestonePhaseNums = new Set(); let missingExplicitVersion = false; + let versionScoped = false; + let versionSectionFound = false; try { - const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); + const roadmapPath = path.join(planningDir(cwd, ws), 'ROADMAP.md'); const roadmapContent = platformReadSync(roadmapPath); if (roadmapContent === null) throw new Error('missing'); - let roadmap = extractCurrentMilestone(roadmapContent, cwd); + let roadmap = extractCurrentMilestone(roadmapContent, cwd, ws); const hasVersionedMilestonesGlobal = /^#{1,3}\s+.*v\d+\.\d+/mi.test(roadmapContent); const hasPhaseHeadings = /#{2,4}\s*(?:\[[^\]]{1,200}\]\s*)?Phase\s+[\w]/i.test(roadmapContent); @@ -578,6 +660,8 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p missingExplicitVersion = true; } } else { + versionScoped = true; + versionSectionFound = true; const sectionStart = sectionMatch.index!; const headingLevel = (sectionMatch[1].match(/^(#{1,3})\s/) ?? ['', '#'])[1].length; const afterHeading = sectionStart + sectionMatch[0].length; @@ -632,17 +716,25 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p const passAll = (() => true) as unknown as MilestonePhaseFilter; passAll.phaseCount = 0; passAll.missingExplicitVersion = missingExplicitVersion; + passAll.versionScoped = false; + // #2562: preserved through the pass-all degrade precisely BECAUSE + // `versionScoped` is reset here — this is the only surviving evidence that + // the current milestone exists in the ROADMAP and simply has no phases yet. + passAll.versionSectionFound = versionSectionFound; return passAll; } - const normalized = new Set( - [...milestonePhaseNums].map(n => n.split('-').map(seg => (seg.replace(/^0+(?=\d)/, '') || '0')).join('-').toLowerCase()) - ); - function normalizePhaseIdSegments(id: string): string { return id.split('-').map(seg => seg.replace(/^0+(?=\d)/, '') || '0').join('-'); } + // #2562: derive BOTH sides of every membership comparison from + // normalizePhaseIdSegments. This set previously inlined a byte-identical + // second copy of that logic — the drift-prone shape this issue is about. + const normalized = new Set( + [...milestonePhaseNums].map(n => normalizePhaseIdSegments(n).toLowerCase()) + ); + const roadmapUsesHyphenedIds = [...normalized].some(n => n.includes('-')); // #2043: milestone-prefixed sub-phase components must be zero-padded — so a // single-digit slug word after the phase @@ -672,6 +764,8 @@ function getMilestonePhaseFilter(cwd: string, versionOverride?: string | null, p } (isDirInMilestone as MilestonePhaseFilter).phaseCount = milestonePhaseNums.size; (isDirInMilestone as MilestonePhaseFilter).missingExplicitVersion = missingExplicitVersion; + (isDirInMilestone as MilestonePhaseFilter).versionScoped = versionScoped; + (isDirInMilestone as MilestonePhaseFilter).versionSectionFound = versionSectionFound; return isDirInMilestone as MilestonePhaseFilter; } @@ -717,9 +811,7 @@ function currentMilestoneRawRanges( const headingMatches = [...content.matchAll(sectionPattern)]; if (headingMatches.length === 0) return null; - const closedMarkerPattern = /\b(?:CLOSED|ARCHIVED|ABANDONED|SHIPPED|FAILED)\b|✅|🗄/i; - const activeMarkerPattern = /\b(?:STARTED|ACTIVE|WIP)\b|in\s+progress|🚧|🔄/i; - const isClosed = (h: string) => closedMarkerPattern.test(h) && !activeMarkerPattern.test(h); + const isClosed = isClosedMilestoneHeading; const firstMatch = headingMatches[0]; const selected = headingMatches.find((m) => !isClosed(m[1])) || firstMatch; const sectionStart = selected.index ?? 0; @@ -764,6 +856,7 @@ function currentMilestoneRawRanges( export = { stripShippedMilestones, extractCurrentMilestone, + isMilestoneShippedInRoadmap, replaceInCurrentMilestone, getRoadmapPhaseInternal, getMilestoneInfo, diff --git a/src/state.cts b/src/state.cts index c6da78f46..67fa6769d 100644 --- a/src/state.cts +++ b/src/state.cts @@ -16,7 +16,7 @@ import configLoaderMod = require('./config-loader.cjs'); const { loadConfig } = configLoaderMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseIdMod = require('./phase-id.cjs'); -const { escapeRegex, normalizePhaseName, extractPhaseToken, parsePhaseFromProse, PHASE_NUMBER_TOKEN_SOURCE } = phaseIdMod; +const { escapeRegex, parsePhaseFromProse, PHASE_NUMBER_TOKEN_SOURCE, phaseKeyFromToken, phaseKeyFromDir } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import roadmapParserMod = require('./roadmap-parser.cjs'); const { getMilestoneInfo, getMilestonePhaseFilter, extractCurrentMilestone } = roadmapParserMod; @@ -1526,25 +1526,11 @@ function cmdStateSnapshot(cwd: string, raw: boolean): void { // ─── State Frontmatter Sync ────────────────────────────────────────────────── -/** - * Canonical key for matching a ROADMAP phase token against an on-disk phase - * directory: normalizePhaseName collapses padding/case, strips the project-code - * prefix, and handles decimals/letter-suffixes/milestone-prefixed IDs, so - * "Phase 4"/"Phase 04"/dir "04-delta" and "Phase PROJ-42"/dir "PROJ-42-foo" - * each map to one key. For a directory, extract its phase token first. - * - * Stripping the project-code prefix is GSD's canonical phase identity (a - * project_code is a display prefix; normalizePhaseName / phaseTokenMatches treat - * `CK-01` and `01` as the same phase, which is what lets a prefixed dir match a - * bare ROADMAP token). A consistent project uses one scheme, so a bare numeric - * and a same-suffix project-code phase never coexist in one milestone. - */ -function phaseKeyFromToken(token: string): string { - return normalizePhaseName(token).toUpperCase(); -} -function phaseKeyFromDir(dir: string): string { - return phaseKeyFromToken(extractPhaseToken(dir)); -} +// `phaseKeyFromToken` / `phaseKeyFromDir` — the canonical key for matching a +// ROADMAP phase token against an on-disk phase directory — moved to the +// phase-id owner module in #2562 so every consumer derives BOTH sides of a +// phase comparison from the same function (see phase-id.cts). Imported at the +// top of this file; call sites below are unchanged. /** * Extract the set of retired/folded phase keys from a ROADMAP milestone scope diff --git a/src/workstream-inventory-builder.cts b/src/workstream-inventory-builder.cts index c89386df8..0baef82b0 100644 --- a/src/workstream-inventory-builder.cts +++ b/src/workstream-inventory-builder.cts @@ -15,6 +15,15 @@ function toPosixPath(p: string): string { return p.split('\\').join('/'); } +/** + * #2562: verification verdicts that DISQUALIFY a phase from `complete`, even + * when its SUMMARY count meets its PLAN count. Deliberately scoped to the two + * EXPLICIT failing verdicts the verifier emits — `missing`/`unknown` (verifier + * off / not yet run) and `stale` (mtime-derived, #2348) are intentionally left + * untouched so verifier-disabled projects do not regress to never-complete. + */ +const FAILING_VERIFICATION_STATUSES = new Set(['gaps_found', 'human_needed']); + export function isCompletedInventory(status: unknown): boolean { const s = (typeof status === 'string' ? status @@ -29,6 +38,28 @@ export interface PhaseFilesCount { directory: string; planCount: number; summaryCount: number; + /** + * #2562: the directory's canonical phase key (`phaseKeyFromDir`). Two stale + * same-numbered directories (`05-x` alongside `5-x-old` — Bug #2445's + * scenario) share a key and must count ONCE in the rollup, or the numerator + * outgrows a denominator that counts distinct phases. Absent → the directory + * name is its own key (no de-duplication). + */ + phaseKey?: string; + /** #2562: directory mtime, the Bug #2445 tie-break when two directories share a phase key. */ + mtimeMs?: number; + /** + * #2562: whether this phase directory belongs to the CURRENT milestone. + * Only meaningful when milestone scoping is active (see + * `currentMilestonePhaseCount`); undefined/true otherwise. + */ + inMilestone?: boolean; + /** + * #2562: the phase's `*-VERIFICATION.md` verdict (`readVerificationStatus`). + * A phase with SUMMARY count ≥ PLAN count but a failing verdict + * (`gaps_found`/`human_needed`) must NOT count as complete. + */ + verificationStatus?: string; } export interface PhaseStatus { @@ -50,6 +81,20 @@ export interface StateProjection { last_activity: string | null | undefined; } +/** + * Which authoritative signal claimed the current milestone is shipped. + * + * - `snapshot` — `milestones/-ROADMAP.md` exists. Written by + * `milestone complete`; the strongest signal a tool can produce. + * - `heading` — the LIVE ROADMAP carries a shipped marker for the current + * milestone. Operator-typed prose; the weakest signal. + * - `legacy` — the over-broad project-lifetime fallback used only when the + * current milestone version cannot be determined (#1913 protection for + * malformed/legacy projects). + * - `null` — no shipped signal. + */ +export type MilestoneShippedSignal = 'snapshot' | 'heading' | 'legacy' | null; + export interface BuildWorkstreamInventoryInputs { name: string; projectDir: string; @@ -67,7 +112,35 @@ export interface BuildWorkstreamInventoryInputs { * "milestone complete" regardless of the mutable STATE.md `Status` field, * so a stale field can never report a shipped workstream as executing (#1913). */ - milestoneShipped: boolean; + milestoneShipped?: boolean; + /** + * #2562 review: WHICH shipped signal fired, so the builder can cross-validate + * it against the milestone's own artifacts at the right strength. The two + * signals are not interchangeable — see the `shippedContradicted` block below. + * Supersedes the `milestoneShipped` boolean; a caller passing only the boolean + * is treated as `'legacy'` (ungated), preserving pre-review behavior. + */ + milestoneShippedSignal?: MilestoneShippedSignal; + /** + * #2562: number of phases the CURRENT milestone declares in the ROADMAP + * `## Progress` table (including phases declared but never scaffolded). When + * > 0, milestone scoping is active: only phases whose directory belongs to + * the current milestone (`PhaseFilesCount.inMilestone`) feed the completion + * rollup, and this value — not `roadmapPhaseCount` — is the denominator, so + * completed prior-milestone phases can never inflate the percentage to 100. + * 0 disables scoping and preserves the legacy `roadmapPhaseCount` behavior. + */ + currentMilestonePhaseCount?: number; + /** + * #2562: whether milestone scoping is active, stated by the caller rather than + * inferred from `currentMilestonePhaseCount > 0`. The two are not equivalent: + * a current milestone that is declared but not yet populated is scoped AND has + * zero phases, and inferring from the count alone reads it as "unscoped" — + * which silently falls back to counting the project's entire phase history as + * the current milestone's, reporting 100% for a milestone with no work done. + * Defaults to the count-derived value so pre-#2562 callers are unaffected. + */ + milestoneScoped?: boolean; } export interface WorkstreamInventory { @@ -76,10 +149,25 @@ export interface WorkstreamInventory { active: boolean; files: WorkstreamFilesExist; status: string; - /** Whether `status` was derived from an authoritative signal ("derived") or taken verbatim from the STATE.md field ("field"). */ + /** + * Whether `status` was derived ("derived") or taken verbatim from the STATE.md + * field ("field"). `"derived"` covers TWO cases and does NOT imply + * `status === 'milestone complete'`: a shipped signal fired and was accepted, + * OR a shipped signal was refused by the artifacts and the STATE field claimed + * completion anyway, so `status` was derived DOWN to `'in_progress'`. Check + * `milestone_shipped_unverified` to tell them apart. + */ status_source: 'field' | 'derived'; /** True when the derived status disagrees with the STATE.md `Status` field (the field is stale). */ status_conflict: boolean; + /** + * #2562 review: a shipped signal fired for the current milestone but the + * milestone's own artifacts contradict it, so the claim was NOT trusted. + * Distinct from `status_conflict`, which reports the derived-vs-STATE-field + * disagreement. Without this the rejection would be silent: `status` falls + * back to the field and no output says a shipped marker was seen and refused. + */ + milestone_shipped_unverified: boolean; current_phase: string | null | undefined; last_activity: string | null | undefined; phases: PhaseStatus[]; @@ -103,46 +191,170 @@ export function buildWorkstreamInventory(inputs: BuildWorkstreamInventoryInputs) stateProjection, filesExist, milestoneShipped, + milestoneShippedSignal, + currentMilestonePhaseCount = 0, + milestoneScoped, } = inputs; + // A caller that passes only the legacy boolean states THAT a signal fired but + // not which one. Treat it as `legacy` — the ungated strength — so pre-review + // callers keep their exact behavior rather than silently acquiring a new gate. + const shippedSignal: MilestoneShippedSignal = + milestoneShippedSignal !== undefined ? milestoneShippedSignal : (milestoneShipped ? 'legacy' : null); + const milestoneShippedResolved = shippedSignal !== null; + + // #2562: when scoping is active, prior-milestone phase directories are + // excluded from the completion rollup and the denominator. The caller states + // this; the count-derived default is the pre-#2562 fallback for callers that + // do not, and cannot represent a scoped-but-empty current milestone. + const scoped = milestoneScoped ?? currentMilestonePhaseCount > 0; + // Index counts by directory for O(1) lookup during sort/iteration - const countsMap = new Map(); + const countsMap = new Map(); for (const entry of phaseFilesCounts) { - countsMap.set(entry.directory, { planCount: entry.planCount, summaryCount: entry.summaryCount }); + countsMap.set(entry.directory, entry); } + // #2562 / Bug #2445: pick ONE directory per phase key for the rollup. Stale + // same-numbered directories left over from a prior milestone would otherwise + // each add to the numerator while the denominator counts distinct phases — + // pushing completed_phases past it, where the old `Math.min` cap silently + // rounded the result up to 100% and hid an unstarted phase. Newest-on-disk + // wins, mirroring state.cts's #2445 de-duplication. + const rollupDirByKey = new Map(); + for (const dir of [...phaseDirNames].sort()) { + const entry = countsMap.get(dir); + if (scoped && entry?.inMilestone === false) continue; + const key = entry?.phaseKey ?? dir; + const incumbent = rollupDirByKey.get(key); + if (incumbent === undefined) { + rollupDirByKey.set(key, dir); + continue; + } + const incumbentMtime = countsMap.get(incumbent)?.mtimeMs ?? 0; + if ((entry?.mtimeMs ?? 0) > incumbentMtime) rollupDirByKey.set(key, dir); + } + const rollupDirs = new Set(rollupDirByKey.values()); + const phases: PhaseStatus[] = []; let completedPhases = 0; let totalPlans = 0; let completedPlans = 0; + // #2562 review: in-milestone phase directories still present under `phases/`, + // whatever their status. A CLEAN archive has none — `milestone complete` moves + // them all out — so this counts exactly the phases that outlived the archive, + // which is what distinguishes "archived" from "archived, then reopened". + // Deliberately NOT "…and unfinished": a complete live dir beside a declared + // but never-scaffolded phase is a dirty archive too, and the dirless phase has + // no directory to inspect. + let liveInMilestonePhases = 0; for (const dir of [...phaseDirNames].sort()) { - const counts = countsMap.get(dir) ?? { planCount: 0, summaryCount: 0 }; + const counts = countsMap.get(dir); + const planCount = counts?.planCount ?? 0; + const summaryCount = counts?.summaryCount ?? 0; + // #2562: SUMMARY≥PLAN parity is necessary but not sufficient — a phase whose + // verification verdict is an explicit failing one is still in progress. + const verificationStatus = counts?.verificationStatus ?? 'missing'; + const summariesMeetPlans = summaryCount >= planCount && planCount > 0; const status: 'complete' | 'in_progress' | 'pending' = - counts.summaryCount >= counts.planCount && counts.planCount > 0 + summariesMeetPlans && !FAILING_VERIFICATION_STATUSES.has(verificationStatus) ? 'complete' - : counts.planCount > 0 + : planCount > 0 ? 'in_progress' : 'pending'; - totalPlans += counts.planCount; - completedPlans += Math.min(counts.summaryCount, counts.planCount); - if (status === 'complete') completedPhases++; + // #2562: only current-milestone phases feed the rollup when scoping is on, + // and only one directory per phase key (see rollupDirs above). + const countsTowardMilestone = (!scoped || counts?.inMilestone !== false) && rollupDirs.has(dir); + if (countsTowardMilestone) { + totalPlans += planCount; + completedPlans += Math.min(summaryCount, planCount); + if (status === 'complete') completedPhases++; + liveInMilestonePhases++; + } phases.push({ directory: dir, status, - plan_count: counts.planCount, - summary_count: counts.summaryCount, + plan_count: planCount, + summary_count: summaryCount, }); } + // #2562: the denominator is the current milestone's declared phase count when + // scoping is active (catches phases declared but never scaffolded), else the + // legacy whole-roadmap heading count. + const effectivePhaseCount = scoped ? currentMilestonePhaseCount : roadmapPhaseCount; + + // #2562 invariant: the numerator counts de-duplicated in-milestone phase keys + // and the denominator counts the union of those keys with the roadmap's own + // declarations, so the numerator can never exceed it. Raising the numerator + // above the denominator means the two sides were derived in different key + // spaces — the defect class this issue is about. The old `Math.min(100, …)` + // capped that away and reported 100%; this makes it fail loudly instead. + if (scoped && completedPhases > effectivePhaseCount) { + throw new Error( + `workstream inventory invariant violated for "${name}": completed_phases (${completedPhases}) ` + + `exceeds the current-milestone denominator (${effectivePhaseCount}). The completion numerator and ` + + `denominator were derived in different phase-key spaces.` + ); + } + // #1913: derive status from authoritative shipped signals rather than trusting // the mutable STATE.md `Status` field. When a shipped signal is present, the // workstream is "milestone complete" regardless of a stale field value. + // + // #2562 review: a shipped signal is a CLAIM, and a claim its own milestone's + // artifacts contradict must not be echoed as fact — the defect class this issue + // is about reaches `status`, not just `progress_percent`. The two signals need + // DIFFERENT cross-checks; one check for both regresses the commonest shape: + // + // - `heading` — operator-typed marker in the LIVE roadmap. Nothing has been + // archived, so every phase the milestone declares should be on disk and + // complete. Gate on the full ratio, which also catches the + // declared-but-never-scaffolded phases that have no directory to inspect. + // - `snapshot` — `milestones/-ROADMAP.md`. The `milestone complete` + // run that writes it also MOVES the milestone's phase directories into + // `milestones/-phases/` (milestone.cts:783-790) while COPYING — + // never truncating — the live ROADMAP (:700-702), so its Progress rows + // survive. A CLEAN archive therefore reads 0/N by construction, and gating + // it on the ratio alone would strip `milestone complete` from every + // archived milestone. But a live in-milestone directory means the archive + // is NOT clean — a phase was added or reopened after it, reachable because + // `milestone complete` does not advance STATE's `milestone:` field + // (state-transition.cts:83, :1335); only `/gsd-new-milestone` does (:1224). + // Once any in-milestone directory is live the ratio IS meaningful again, so + // the check is the conjunction. Requiring the live directory to itself be + // unfinished was too narrow: it let a complete live dir alongside a + // declared-but-unscaffolded phase reproduce the reported symptom, since a + // dirless phase has nothing to inspect. + // + // The whole cross-check is scoped-only, and NOT because of the signal: when + // scoping is off, `effectivePhaseCount` is the whole-roadmap count and + // membership is everything, so there is no current-milestone artifact set to + // check a current-milestone claim against. `legacy` is additionally ungated by + // signal — it is the fallback for an unknown milestone version, which is + // exactly when scoping cannot engage either. const fieldStatus = stateProjection.status; - const useDerived = milestoneShipped; - const status = useDerived ? 'milestone complete' : fieldStatus; - const status_source: 'field' | 'derived' = useDerived ? 'derived' : 'field'; - const status_conflict = useDerived && !isCompletedInventory(fieldStatus); + const shippedContradicted = scoped && ( + shippedSignal === 'heading' + ? completedPhases < effectivePhaseCount + : shippedSignal === 'snapshot' + ? liveInMilestonePhases > 0 && completedPhases < effectivePhaseCount + : false + ); + const useDerived = milestoneShippedResolved && !shippedContradicted; + // Refusing the claim does not make the STATE field a safe fallback: it is + // operator-written and in this window it commonly ALSO reads "milestone + // complete", which would re-report the refused claim through the other door. + // Against contradicting artifacts, NEITHER source may assert completion. + const artifactOverride = shippedContradicted && isCompletedInventory(fieldStatus); + const status = useDerived + ? 'milestone complete' + : artifactOverride + ? 'in_progress' + : fieldStatus; + const status_source: 'field' | 'derived' = useDerived || artifactOverride ? 'derived' : 'field'; + const status_conflict = (useDerived && !isCompletedInventory(fieldStatus)) || artifactOverride; return { name, @@ -156,17 +368,22 @@ export function buildWorkstreamInventory(inputs: BuildWorkstreamInventoryInputs) status, status_source, status_conflict, + milestone_shipped_unverified: shippedContradicted, current_phase: stateProjection.current_phase, last_activity: stateProjection.last_activity, phases, phase_count: phases.length, completed_phases: completedPhases, - roadmap_phase_count: roadmapPhaseCount, + roadmap_phase_count: effectivePhaseCount, total_plans: totalPlans, completed_plans: completedPlans, + // The `Math.min` cap is unreachable under milestone scoping (the invariant + // above throws first) and survives only for the legacy unscoped path, where + // the denominator is a roadmap heading count that a caller cannot guarantee + // bounds the numerator. progress_percent: - roadmapPhaseCount > 0 - ? Math.min(100, Math.round((completedPhases / roadmapPhaseCount) * 100)) + effectivePhaseCount > 0 + ? Math.min(100, Math.round((completedPhases / effectivePhaseCount) * 100)) : 0, }; } diff --git a/src/workstream-inventory.cts b/src/workstream-inventory.cts index 97b0d8e7e..08dbed24b 100644 --- a/src/workstream-inventory.cts +++ b/src/workstream-inventory.cts @@ -24,8 +24,18 @@ import planScan = require('./plan-scan.cjs'); import planningWorkspace = require('./planning-workspace.cjs'); const { planningPaths, planningRoot, getActiveWorkstream } = planningWorkspace; import { stateExtractField } from './state-document.cjs'; +import { findTableWithColumns } from './markdown-table.cjs'; +// eslint-disable-next-line @typescript-eslint/no-require-imports -- verification.cjs is an export= CommonJS module +import verificationMod = require('./verification.cjs'); +const { readVerificationStatus } = verificationMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports -- phase-id.cjs is an export= CommonJS module +import phaseIdMod = require('./phase-id.cjs'); +const { phaseKeyFromDir, phaseKeyFromProse, parentPhaseKey } = phaseIdMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports -- roadmap-parser.cjs is an export= CommonJS module +import roadmapParserMod = require('./roadmap-parser.cjs'); +const { getMilestonePhaseFilter, isMilestoneShippedInRoadmap } = roadmapParserMod; import { buildWorkstreamInventory, isCompletedInventory } from './workstream-inventory-builder.cjs'; -import type { WorkstreamInventory, StateProjection } from './workstream-inventory-builder.cjs'; +import type { WorkstreamInventory, StateProjection, MilestoneShippedSignal } from './workstream-inventory-builder.cjs'; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -62,6 +72,128 @@ function countRoadmapPhases(roadmapPath: string, fallbackCount: number): number } } +interface RoadmapProgressRow { + /** Canonical phase key (`phaseKeyFromProse`) — the SAME key space as `phaseKeyFromDir`. */ + key: string; + /** `vX.Y` when the row attributes the phase to a milestone; null when the cell is absent, blank or malformed. */ + version: string | null; +} + +/** + * #2562: parse the ROADMAP `## Progress` table into canonical phase keys with + * their milestone attribution, e.g. `| 30. Name | v10.0 | 1/3 | … |` → + * `{ key: '30', version: 'v10.0' }`. The table is the authoritative per-phase + * milestone attribution and — crucially — lists phases declared but never + * scaffolded (no directory), which a directory-only scan misses. `Plans + * Complete` is present in BOTH RoadmapProgress variants, so this matches the + * flat (no Milestone column → every `version` null) and milestone-grouped + * shapes alike. + * + * Row keys come from `phaseKeyFromProse`, the same owner-module derivation + * `phaseKeyFromDir` uses for directories, so a `| 01. … |` row and a `1-slug` + * directory cannot land in different key spaces (the padding-asymmetry defect). + */ +function parseRoadmapProgressRows(roadmapPath: string): RoadmapProgressRow[] { + let content: string; + try { + content = fs.readFileSync(roadmapPath, 'utf-8'); + } catch { + return []; /* no roadmap */ + } + // Milestone-ATTRIBUTING shape first. Both shapes carry `Plans Complete`, so + // probing that column first would pick a flat table appearing earlier in the + // document over a milestone-grouped one later — every row would come back + // unattributed and be treated as current-milestone, silently over-including. + const table = findTableWithColumns(content, ['Phase', 'Milestone']) + ?? findTableWithColumns(content, ['Phase', 'Plans Complete']); + if (!table) return []; + const rows: RoadmapProgressRow[] = []; + for (const row of table.rows) { + const key = phaseKeyFromProse(row['Phase']); + if (key === null) continue; + const cell = (row['Milestone'] ?? '').trim(); + rows.push({ key, version: /^v\d+(?:\.\d+)+$/.test(cell) ? cell : null }); + } + return rows; +} + +/** + * #2562: the workstream's CURRENT milestone version, read from the STATE.md + * `milestone:` frontmatter field (the reliable per-workstream signal — the + * ROADMAP's own in-progress markers can be stale, e.g. a lingering 🚧 on an + * already-shipped milestone). Falls back to the ROADMAP in-progress heading + * marker only when STATE has no field. + */ +function readCurrentMilestoneVersion(statePath: string, roadmapPath: string): string | null { + try { + const m = fs.readFileSync(statePath, 'utf-8').match(/^milestone:\s*["']?(v\d+(?:\.\d+)+)["']?/m); + if (m) return m[1]; + } catch { + /* no state */ + } + try { + const rm = fs.readFileSync(roadmapPath, 'utf-8').match(/(?:🚧|🔄)\s*\*\*(v\d+(?:\.\d+)+)\b/); + if (rm) return rm[1]; + } catch { + /* no roadmap */ + } + return null; +} + +/** + * #2562: does the CURRENT milestone's own ROADMAP heading carry a shipped + * marker? Delegated to `roadmap-parser`, the module that owns milestone-heading + * classification: heading/`` lines only (never a bullet that merely + * names the version), version-token boundary-matched so `v2.0` does not match + * inside `v2.0.1`, and in-progress markers win. Scoped to the current version, + * so a prior milestone's collapsed `
✅ … SHIPPED` + * block can never mark the current milestone complete. + */ +function currentMilestoneHeadingShipped(roadmapPath: string, version: string): boolean { + try { + return isMilestoneShippedInRoadmap(fs.readFileSync(roadmapPath, 'utf-8'), version); + } catch { + return false; /* no roadmap */ + } +} + +/** + * Legacy pre-#2562 shipped detection: ANY archived milestone snapshot OR a + * SHIPPED marker anywhere in the ROADMAP. Over-broad (project-lifetime, not + * milestone-scoped) — retained ONLY as the fallback when the current milestone + * version cannot be determined (malformed/legacy STATE.md with no `milestone:` + * field), so those projects keep #1913's stale-field protection. + */ +function legacyMilestoneShipped(roadmapPath: string, planningBase: string): boolean { + try { + const milestonesDir = path.join(planningBase, 'milestones'); + for (const entry of fs.readdirSync(milestonesDir, { withFileTypes: true })) { + if (entry.isFile() && /-ROADMAP\.md$/i.test(entry.name)) return true; + } + } catch { + /* no milestones archive dir */ + } + try { + if (/SHIPPED/i.test(fs.readFileSync(roadmapPath, 'utf-8'))) return true; + } catch { + /* no roadmap */ + } + return false; +} + +/** + * #2562: directory mtime, used only to break a duplicate-phase-key tie in the + * rollup (keep the more recently touched directory — the Bug #2445 rule). 0 on + * a stat failure, which loses the tie rather than throwing. + */ +function phaseDirMtime(phaseDir: string): number { + try { + return fs.statSync(phaseDir).mtimeMs; + } catch { + return 0; + } +} + function countPhaseFiles(phaseDir: string): PhaseFileCounts { const scan = planScan(phaseDir); return { planCount: scan.planCount, summaryCount: scan.summaryCount }; @@ -85,27 +217,39 @@ function readStateProjection(statePath: string): StateProjection { } /** - * #1913: detect an authoritative shipped signal for a workstream so the - * inventory status is never trusted from the mutable STATE.md `Status` field - * alone. Returns true when EITHER an archived milestone snapshot is present - * under `/milestones/` OR the workstream ROADMAP carries a - * SHIPPED marker — both are hard to desync, unlike the hand-maintained field. + * #1913 + #2562: detect an authoritative shipped signal for a workstream's + * CURRENT milestone, so the inventory status is never trusted from the mutable + * STATE.md `Status` field alone (#1913) yet is never pinned to "milestone + * complete" by a PRIOR milestone's shipped marker (#2562). + * + * When the current milestone version is known, the signal is scoped to it: + * an archived snapshot `milestones/-ROADMAP.md` (the canonical + * "milestone shipped" artifact) OR the current milestone's own ROADMAP line + * marked shipped. When the version cannot be determined, we fall back to the + * over-broad legacy detection to preserve #1913's protection for those + * (malformed/legacy) projects. + * + * Returns WHICH signal fired, not merely that one did. The two differ in how + * much they can be trusted and therefore in how the builder cross-validates + * them against the milestone's own artifacts — see the `shippedContradicted` + * block in `workstream-inventory-builder.cts`. Collapsing them to a boolean is + * what forced a single completeness check to serve two incompatible shapes. */ -function workstreamMilestoneShipped(roadmapPath: string, planningBase: string): boolean { - try { - const milestonesDir = path.join(planningBase, 'milestones'); - for (const entry of fs.readdirSync(milestonesDir, { withFileTypes: true })) { - if (entry.isFile() && /-ROADMAP\.md$/i.test(entry.name)) return true; - } - } catch { - /* no milestones archive dir */ +function workstreamShippedSignal( + roadmapPath: string, + planningBase: string, + currentVersion: string | null, +): MilestoneShippedSignal { + if (!currentVersion) { + return legacyMilestoneShipped(roadmapPath, planningBase) ? 'legacy' : null; } - try { - if (/SHIPPED/i.test(fs.readFileSync(roadmapPath, 'utf-8'))) return true; - } catch { - /* no roadmap */ - } - return false; + // Canonical shipped artifact: the archived ROADMAP snapshot of the CURRENT + // milestone (`vX.Y-ROADMAP.md`), written at milestone close. REQUIREMENTS + // snapshots are intentionally NOT accepted — they can be written at milestone + // START (requirements-locked), so they do not imply shipped. + const snapshot = path.join(planningBase, 'milestones', `${currentVersion}-ROADMAP.md`); + if (fs.existsSync(snapshot)) return 'snapshot'; + return currentMilestoneHeadingShipped(roadmapPath, currentVersion) ? 'heading' : null; } function sortWorkstreamInventories(inventories: WorkstreamInventory[], activeWorkstreamName: string | null): WorkstreamInventory[] { @@ -127,12 +271,139 @@ function inspectWorkstream(cwd: string, name: string, options: InspectWorkstream const p = planningPaths(cwd, name); const phaseDirNames = readSubdirectories(p.phases); - // Collect per-phase file counts + // #2562: scope progress to the CURRENT milestone. Membership and the + // denominator are derived in ONE key space (`phaseKeyFromDir` / + // `phaseKeyFromProse`, both from the phase-id owner module) so the two sides + // of the rollup cannot disagree. + const currentVersion = readCurrentMilestoneVersion(p.state, p.roadmap); + const progressRows = parseRoadmapProgressRows(p.roadmap); + + // Phase keys the ROADMAP attributes to the current milestone. A row whose + // Milestone cell is blank or malformed is INCLUDED rather than dropped: a + // phase we cannot attribute must still be visible to the rollup. Dropping it + // from both sides was the silent-deletion defect — it let an unstarted phase + // vanish and the percentage round to 100. Over-inclusive-never-under is the + // degrade direction this codebase already commits to for unparseable roadmap + // input (see the getMilestonePhaseFilter catch in roadmap-parser.cts). + const currentMilestoneKeys = new Set(); + if (currentVersion) { + for (const row of progressRows) { + if (row.version === null || row.version === currentVersion) currentMilestoneKeys.add(row.key); + } + } + + // Roadmap-heading membership, from the module that OWNS milestone-phase + // filtering. Consulted only when it is genuinely scoped to a single milestone + // (`versionScoped`); the unversioned whole-roadmap shape spans the project's + // lifetime and would re-admit prior-milestone phases — the very defect here. + const headingFilter = getMilestonePhaseFilter(cwd, currentVersion, null, name); + const headingScoped = headingFilter.versionScoped && headingFilter.phaseCount > 0; + + // Phase keys the ROADMAP attributes to some OTHER milestone. A row carrying an + // explicit version that is not the current one is a positive claim by a prior + // (or future) milestone — the only reliable evidence that a phase does NOT + // belong to the current one. + const claimedElsewhere = new Set(); + for (const row of progressRows) { + if (row.version !== null && row.version !== currentVersion) claimedElsewhere.add(row.key); + } + + // #2562: the current milestone is DECLARED but nothing attributes a phase to + // it yet — the window right after `/gsd-new-milestone`, where STATE.md's + // `milestone:` field updates the moment the heading lands but the Progress + // table and phase sections have not caught up. + // + // Treating that as "unscoped" was a hole in the original fix: scoping switched + // off entirely and the fallback below counted the project's ENTIRE phase + // history as both numerator and denominator, so a workstream whose current + // milestone had zero phases done reported 100% off its predecessors' work. + // That is the very symptom #2562 reports, reached by a different route. + // + // Three independent signals witness it; ANY of them is enough, and each covers + // a ROADMAP shape the others miss: + // - `versionSectionFound` — the milestone's own section exists but declares + // no phases (heading-only ROADMAPs, and the common `## v3.0` stub). + // - `missingExplicitVersion` — the ROADMAP versions its milestones but has + // no section for this one at all. + // - a Progress table that attributes every row elsewhere (`claimedElsewhere` + // non-empty while `currentMilestoneKeys` is empty). + // A ROADMAP that attributes NO versions anywhere matches none of them: its + // rows parse with `version: null`, land in `currentMilestoneKeys`, and never + // reach here. That is deliberate — for a free-form legacy project the + // whole-roadmap count IS the current milestone, and `readCurrentMilestoneVersion` + // hands back a non-null version for almost every project, so keying off + // `currentVersion` alone would regress every one of them to 0%. + const currentMilestoneDeclaredEmpty = + currentVersion !== null && + currentMilestoneKeys.size === 0 && + !headingScoped && + (headingFilter.versionSectionFound || headingFilter.missingExplicitVersion || claimedElsewhere.size > 0); + + // A dir-only phase joins the current milestone when the roadmap names it, or + // when it is a sub-phase (`30.1-…`) of a phase the roadmap names — sub-phases + // inserted mid-milestone rarely get a row of their own. Membership feeds BOTH + // the numerator and (via `milestoneKeys` below) the denominator, so a member + // can never exceed the denominator that counts it. + const scoped = currentMilestoneKeys.size > 0 || headingScoped || currentMilestoneDeclaredEmpty; + const isDirInCurrentMilestone = (dir: string): boolean => { + if (!scoped) return true; + const key = phaseKeyFromDir(dir); + if (currentMilestoneKeys.has(key)) return true; + const parent = parentPhaseKey(key); + if (parent !== null && currentMilestoneKeys.has(parent)) return true; + // An empty current milestone has no roadmap declarations to match against, + // so membership inverts: a directory belongs UNLESS another milestone claims + // it. A phase scaffolded before the roadmap caught up would otherwise vanish + // from both sides of the rollup — under-reporting, the direction this + // codebase never degrades in. + if (currentMilestoneDeclaredEmpty) { + const parentKey = parentPhaseKey(key); + return !claimedElsewhere.has(key) && (parentKey === null || !claimedElsewhere.has(parentKey)); + } + return headingScoped && headingFilter(dir); + }; + + // Collect per-phase file counts (+ canonical key, milestone membership, + // verification verdict). `phaseKey` lets the builder de-duplicate stale + // same-numbered directories (Bug #2445's scenario) in the rollup. const phaseFilesCounts = phaseDirNames.map(dir => { - const counts = countPhaseFiles(path.join(p.phases, dir)); - return { directory: dir, planCount: counts.planCount, summaryCount: counts.summaryCount }; + const phaseDir = path.join(p.phases, dir); + const counts = countPhaseFiles(phaseDir); + return { + directory: dir, + phaseKey: phaseKeyFromDir(dir), + mtimeMs: phaseDirMtime(phaseDir), + planCount: counts.planCount, + summaryCount: counts.summaryCount, + inMilestone: isDirInCurrentMilestone(dir), + verificationStatus: readVerificationStatus(phaseDir).status, + }; }); + // The denominator is the union of what the roadmap DECLARES for the current + // milestone (including never-scaffolded phases) and the keys of the member + // directories (including dir-only sub-phases). One key space, so + // `completed_phases <= denominator` holds by construction rather than by a + // `Math.min` cap that hid the inconsistency. + const milestoneKeys = new Set(currentMilestoneKeys); + for (const entry of phaseFilesCounts) { + if (entry.inMilestone) milestoneKeys.add(entry.phaseKey); + } + const currentMilestonePhaseCount = scoped + ? Math.max(milestoneKeys.size, headingScoped ? headingFilter.phaseCount : 0) + : 0; + + // Unscoped fallback: the denominator must STILL count phases the ROADMAP + // declares in its Progress table but never scaffolded — the heading-only + // count drops them, even when other headings exist. Union the declared rows + // with the phase directories so neither source can silently shrink it. + let fallbackPhaseCount = countRoadmapPhases(p.roadmap, phaseDirNames.length); + if (!scoped && progressRows.length > 0) { + const union = new Set(progressRows.map(row => row.key)); + for (const entry of phaseFilesCounts) union.add(entry.phaseKey); + fallbackPhaseCount = union.size; + } + return buildWorkstreamInventory({ name, projectDir: cwd, @@ -140,14 +411,19 @@ function inspectWorkstream(cwd: string, name: string, options: InspectWorkstream phaseDirNames, activeWorkstreamName: activeWorkstreamName ?? '', phaseFilesCounts, - roadmapPhaseCount: countRoadmapPhases(p.roadmap, phaseDirNames.length), + roadmapPhaseCount: fallbackPhaseCount, + currentMilestonePhaseCount, + // Stated, not inferred from the count: a declared-but-empty current + // milestone is legitimately scoped AND legitimately zero-phase, and the + // builder cannot tell those apart from `currentMilestonePhaseCount` alone. + milestoneScoped: scoped, stateProjection: readStateProjection(p.state), filesExist: { roadmap: fs.existsSync(p.roadmap), state: fs.existsSync(p.state), requirements: fs.existsSync(p.requirements), }, - milestoneShipped: workstreamMilestoneShipped(p.roadmap, p.planning), + milestoneShippedSignal: workstreamShippedSignal(p.roadmap, p.planning, currentVersion), }); } diff --git a/src/workstream.cts b/src/workstream.cts index 437377adc..dba182022 100644 --- a/src/workstream.cts +++ b/src/workstream.cts @@ -235,6 +235,10 @@ function cmdWorkstreamList(cwd: string, raw: boolean): void { has_roadmap: ws.files.roadmap, has_state: ws.files.state, status: ws.status, + // #2562: a refused shipped marker must reach the surface. Projecting + // `status` without it renders the refusal invisible at the CLI, which is + // the silent-collapse defect this issue is about. + milestone_shipped_unverified: ws.milestone_shipped_unverified, current_phase: ws.current_phase, phase_count: ws.phase_count, completed_phases: ws.completed_phases, @@ -272,6 +276,7 @@ function cmdWorkstreamStatus(cwd: string, name: string | null | undefined, raw: phase_count: inv.phase_count, completed_phases: inv.completed_phases, status: inv.status, + milestone_shipped_unverified: inv.milestone_shipped_unverified, current_phase: inv.current_phase, last_activity: inv.last_activity, }, raw, undefined); @@ -386,6 +391,7 @@ function cmdWorkstreamProgress(cwd: string, raw: boolean): void { name: ws.name, active: ws.active, status: ws.status, + milestone_shipped_unverified: ws.milestone_shipped_unverified, current_phase: ws.current_phase ?? null, phases: `${ws.completed_phases}/${ws.roadmap_phase_count}`, plans: `${ws.completed_plans}/${ws.total_plans}`, diff --git a/tests/phase-id.test.cjs b/tests/phase-id.test.cjs index 150e6c529..fb329ca22 100644 --- a/tests/phase-id.test.cjs +++ b/tests/phase-id.test.cjs @@ -975,3 +975,58 @@ describe('#2736 prose name precedence — properties', () => { } }); }); + +// ─── phase-key derivations (#2562) ─────────────────────────────────────────── + +// #2562: the whole point of these living here is that a ROADMAP table cell and +// a phase DIRECTORY must land in ONE key space. Modules that derived their own +// regex for this is what let a `| 01. … |` row miss a `1-slug` directory, so +// the contract is unit-tested at the owner module, not only through consumers. +describe('phaseKeyFrom* — one key space for directories and prose', () => { + test('every zero-padding spelling of a directory collapses to one key', () => { + for (const dir of ['5-a', '05-a', 'PROJ-5-a', 'PROJ-05-a']) { + assert.strictEqual(phaseId.phaseKeyFromDir(dir), '05', dir); + } + }); + + test('every zero-padding spelling in prose collapses to the same key', () => { + for (const prose of ['5. A', '05. A', '**5. A**', '`05. A`']) { + assert.strictEqual(phaseId.phaseKeyFromProse(prose), '05', prose); + } + }); + + test('a padded table cell and an unpadded directory produce the SAME key', () => { + assert.strictEqual(phaseId.phaseKeyFromProse('01. Setup'), phaseId.phaseKeyFromDir('1-setup')); + assert.strictEqual(phaseId.phaseKeyFromProse('30. Rollout'), phaseId.phaseKeyFromDir('030-rollout')); + }); + + test('sub-phase keys keep their decimal segment', () => { + assert.strictEqual(phaseId.phaseKeyFromDir('30.1-follow-up'), '30.1'); + assert.strictEqual(phaseId.phaseKeyFromProse('**05.1 Follow-up**'), '05.1'); + }); + + test('prose that does not begin with a phase token is null, not a bogus key', () => { + assert.strictEqual(phaseId.phaseKeyFromProse('Not a phase'), null); + assert.strictEqual(phaseId.phaseKeyFromProse(null), null); + assert.strictEqual(phaseId.phaseKeyFromProse(undefined), null); + }); + + test('parentPhaseKey resolves a sub-phase to its parent and a top-level to null', () => { + assert.strictEqual(phaseId.parentPhaseKey('30.1'), '30'); + assert.strictEqual(phaseId.parentPhaseKey('05.12'), '05'); + assert.strictEqual(phaseId.parentPhaseKey('30'), null); + }); + + test('property: padding a directory number never changes its key', () => { + fc.assert( + fc.property( + fc.integer({ min: 1, max: 99 }), + fc.integer({ min: 0, max: 3 }), + (num, pad) => { + const padded = String(num).padStart(String(num).length + pad, '0'); + return phaseId.phaseKeyFromDir(`${padded}-slug`) === phaseId.phaseKeyFromDir(`${num}-slug`); + }, + ), + ); + }); +}); diff --git a/tests/roadmap-parser.test.cjs b/tests/roadmap-parser.test.cjs index 86edc9c4b..ea6b03cda 100644 --- a/tests/roadmap-parser.test.cjs +++ b/tests/roadmap-parser.test.cjs @@ -32,6 +32,7 @@ const { getRoadmapPhaseInternal, getMilestoneInfo, getMilestonePhaseFilter, + isMilestoneShippedInRoadmap, withPhaseSection, } = roadmapParser; @@ -479,6 +480,38 @@ describe('roadmap-parser: getMilestoneInfo #2135 — milestone_name clobber', () }); }); +// ─── isMilestoneShippedInRoadmap ────────────────────────────────────────────── + +// #2562: this module owns milestone-heading classification, so its own shipped +// detection is unit-tested here rather than only through the workstream +// inventory that consumes it. +describe('roadmap-parser: isMilestoneShippedInRoadmap', () => { + test('a shipped marker on the milestone heading counts', () => { + assert.strictEqual(isMilestoneShippedInRoadmap('## v2.0 Launch — ✅ SHIPPED\n', 'v2.0'), true); + }); + + test('a collapsed shipped marker counts', () => { + const roadmap = '
✅ v2.0 Launch — SHIPPED\n\ncontent\n
\n'; + assert.strictEqual(isMilestoneShippedInRoadmap(roadmap, 'v2.0'), true); + }); + + test('a bullet merely naming the version does NOT count', () => { + assert.strictEqual(isMilestoneShippedInRoadmap('- ✅ v2.0 Launch — SHIPPED\n', 'v2.0'), false); + }); + + test('version tokens are boundary-matched (v2.0.1 is not v2.0)', () => { + assert.strictEqual(isMilestoneShippedInRoadmap('## v2.0.1 Patch — ✅ SHIPPED\n', 'v2.0'), false); + }); + + test('an in-progress marker on the heading beats a shipped one', () => { + assert.strictEqual(isMilestoneShippedInRoadmap('## 🚧 v2.0 Launch — ✅ SHIPPED\n', 'v2.0'), false); + }); + + test('another milestone being shipped says nothing about this one', () => { + assert.strictEqual(isMilestoneShippedInRoadmap('## v1.0 Old — ✅ SHIPPED\n', 'v2.0'), false); + }); +}); + // ─── getMilestonePhaseFilter ────────────────────────────────────────────────── describe('roadmap-parser: getMilestonePhaseFilter', () => { @@ -493,6 +526,46 @@ describe('roadmap-parser: getMilestonePhaseFilter', () => { assert.strictEqual(filter('anything'), true); }); + // #2562 added a trailing optional `ws` param. Every pre-existing call site in + // the codebase passes 1–3 args, so what has to hold is that omitting the 4th + // is INDISTINGUISHABLE from the prior resolution — including its + // `GSD_WORKSTREAM` env fallback. Characterises the legacy call surface + // directly; it does not stand in for coverage of the individual callers. + test('#2562: omitting the ws param preserves the prior path resolution exactly', () => { + const ROADMAP = ['## v1.0: Launch', '### Phase 1: Setup', '**Goal:** setup'].join('\n'); + writeRoadmap(tmpDir, ROADMAP); + + const omitted = getMilestonePhaseFilter(tmpDir); + const explicitUndefined = getMilestonePhaseFilter(tmpDir, undefined, undefined, undefined); + const explicitNull = getMilestonePhaseFilter(tmpDir, null, null, null); + + for (const [label, filter] of [['omitted', omitted], ['undefined', explicitUndefined], ['null', explicitNull]]) { + assert.strictEqual(filter.phaseCount, 1, `${label}: same phase count`); + assert.strictEqual(filter('01-setup'), true, `${label}: same membership`); + assert.strictEqual(filter('02-other'), false, `${label}: same exclusion`); + assert.strictEqual(typeof filter.versionScoped, 'boolean', `${label}: new flag is present, not undefined`); + } + }); + + test('#2562: the GSD_WORKSTREAM env fallback still resolves when ws is omitted', () => { + const wsRoadmap = ['## v1.0: WS', '### Phase 7: Only', '**Goal:** only'].join('\n'); + const wsDir = path.join(tmpDir, '.planning', 'workstreams', 'alpha'); + fs.mkdirSync(wsDir, { recursive: true }); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), wsRoadmap); + writeRoadmap(tmpDir, ['## v1.0: Root', '### Phase 1: Setup', '**Goal:** setup'].join('\n')); + + const previous = process.env.GSD_WORKSTREAM; + process.env.GSD_WORKSTREAM = 'alpha'; + try { + const filter = getMilestonePhaseFilter(tmpDir); + assert.strictEqual(filter('07-only'), true, 'env fallback must still reach the workstream roadmap'); + assert.strictEqual(filter('01-setup'), false, 'and must not read the root roadmap'); + } finally { + if (previous === undefined) delete process.env.GSD_WORKSTREAM; + else process.env.GSD_WORKSTREAM = previous; + } + }); + test('basic milestone phase filter — matches dirs by phase number', () => { writeRoadmap(tmpDir, [ '## v1.0: Launch', diff --git a/tests/workstream-inventory.test.cjs b/tests/workstream-inventory.test.cjs index d2d5fd56b..6312bc077 100644 --- a/tests/workstream-inventory.test.cjs +++ b/tests/workstream-inventory.test.cjs @@ -13,6 +13,9 @@ const { cleanup } = require('./helpers.cjs'); const { createFixture, seedWorkstream } = require('./fixtures/index.cjs'); const { buildWorkstreamInventory, isCompletedInventory } = require('../gsd-core/bin/lib/workstream-inventory-builder.cjs'); const { inspectWorkstream } = require('../gsd-core/bin/lib/workstream-inventory.cjs'); +const { VERIFIER_STATUSES } = require('../gsd-core/bin/lib/verification.cjs'); +const { phaseKeyFromDir, phaseKeyFromProse, phaseKeyFromToken } = require('../gsd-core/bin/lib/phase-id.cjs'); +const fc = require('fast-check'); const STALE_STATE = 'status: executing\n'; const IN_PROGRESS_ROADMAP = @@ -124,3 +127,882 @@ describe('isCompletedInventory — ADR-2207 status lifecycle', () => { assert.ok(!isCompletedInventory('Executing')); }); }); + +// #2562: progress/status must be DERIVED from on-disk artifacts scoped to the +// CURRENT milestone — never a project-lifetime "ever shipped anything" signal, +// never a denominator that silently drops declared-but-unscaffolded phases, and +// never counting a phase with a failing VERIFICATION verdict as complete. +describe('#2562 — progress/status scoped to the current milestone (derived from artifacts)', () => { + let tmpDir; + before(() => { tmpDir = createFixture(); }); + after(() => cleanup(tmpDir)); + + const BUILDER_BASE = { + projectDir: '/tmp/ws-proj', + workstreamDir: '/tmp/ws-proj/.planning/workstreams/ws', + activeWorkstreamName: '', + stateProjection: { status: 'executing', current_phase: null, last_activity: null }, + filesExist: { roadmap: true, state: true, requirements: true }, + milestoneShipped: false, + }; + + // ── Defect 3: verification-gated completeness (builder unit) ───────────────── + test('builder: SUMMARY≥PLAN but a human_needed verdict is NOT complete', () => { + const inv = buildWorkstreamInventory({ + ...BUILDER_BASE, + name: 'ws', + phaseDirNames: ['1-a', '2-b'], + phaseFilesCounts: [ + { directory: '1-a', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, + { directory: '2-b', planCount: 4, summaryCount: 4, inMilestone: true, verificationStatus: 'human_needed' }, + ], + roadmapPhaseCount: 2, + currentMilestonePhaseCount: 2, + }); + assert.equal(inv.phases.find(p => p.directory === '2-b').status, 'in_progress'); + assert.equal(inv.completed_phases, 1); + assert.equal(inv.progress_percent, 50); + }); + + test('builder: missing/unknown verdict still counts complete (no verifier-off regression)', () => { + const inv = buildWorkstreamInventory({ + ...BUILDER_BASE, + name: 'ws', + phaseDirNames: ['1-a'], + phaseFilesCounts: [ + { directory: '1-a', planCount: 2, summaryCount: 2, inMilestone: true, verificationStatus: 'missing' }, + ], + roadmapPhaseCount: 1, + currentMilestonePhaseCount: 1, + }); + assert.equal(inv.phases[0].status, 'complete'); + assert.equal(inv.progress_percent, 100); + }); + + // ── Defect 2: denominator includes declared-but-unscaffolded phases (builder) ─ + test('builder: current-milestone denominator counts a phase with no directory', () => { + const inv = buildWorkstreamInventory({ + ...BUILDER_BASE, + name: 'ws', + phaseDirNames: ['1-a', '2-b'], // phase 3 declared for the milestone but never scaffolded + phaseFilesCounts: [ + { directory: '1-a', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, + { directory: '2-b', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, + ], + roadmapPhaseCount: 2, + currentMilestonePhaseCount: 3, + }); + assert.equal(inv.roadmap_phase_count, 3); + assert.equal(inv.completed_phases, 2); + assert.equal(inv.progress_percent, 67, 'the dirless third phase keeps this below 100'); + }); + + // ── Defect 1: prior-milestone phases must not inflate the numerator (builder) ─ + test('builder: completed prior-milestone dirs are excluded from the current rollup', () => { + const inv = buildWorkstreamInventory({ + ...BUILDER_BASE, + name: 'ws', + phaseDirNames: ['1-old', '2-cur'], + phaseFilesCounts: [ + { directory: '1-old', planCount: 3, summaryCount: 3, inMilestone: false, verificationStatus: 'passed' }, + { directory: '2-cur', planCount: 2, summaryCount: 0, inMilestone: true, verificationStatus: 'missing' }, + ], + roadmapPhaseCount: 2, + currentMilestonePhaseCount: 1, + }); + assert.equal(inv.completed_phases, 0); + assert.equal(inv.total_plans, 2, 'only the current-milestone directory contributes plans'); + assert.equal(inv.progress_percent, 0); + }); + + // ── inspectWorkstream integration (all three defects, end-to-end) ──────────── + function writeWsPhase(wsDir, slug, { plans = 0, summaries = 0, verification } = {}) { + const dir = path.join(wsDir, 'phases', slug); + fs.mkdirSync(dir, { recursive: true }); + for (let i = 1; i <= plans; i++) fs.writeFileSync(path.join(dir, `0${i}-PLAN.md`), '# plan\n'); + for (let i = 1; i <= summaries; i++) fs.writeFileSync(path.join(dir, `0${i}-SUMMARY.md`), '# summary\n'); + if (verification) fs.writeFileSync(path.join(dir, '01-VERIFICATION.md'), `---\nstatus: ${verification}\n---\n`); + } + + const MS_STATE = 'milestone: v2.0\nstatus: executing\n'; + // Milestone-grouped Progress table: v2.0 declares phases 3,4,5; 1,2 are shipped v1.0. + const MS_ROADMAP = [ + '# Roadmap', '', '## Progress', '', + '| Phase | Milestone | Plans | Status | Done |', + '| --- | --- | --- | --- | --- |', + '| 1. Old A | v1.0 | 2/2 | Complete | - |', + '| 2. Old B | v1.0 | 2/2 | Complete | - |', + '| 3. New A | v2.0 | 1/1 | Complete | - |', + '| 4. New B | v2.0 | 0/1 | In Progress | - |', + '| 5. New C | v2.0 | 0/1 | Not started | - |', + '', + ].join('\n'); + + test('inspectWorkstream: prior-milestone dirs + a dirless current phase → not complete, not 100%', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-scope' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), MS_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), MS_ROADMAP); + writeWsPhase(wsDir, '1-old-a', { plans: 2, summaries: 2, verification: 'passed' }); + writeWsPhase(wsDir, '2-old-b', { plans: 2, summaries: 2, verification: 'passed' }); + writeWsPhase(wsDir, '3-new-a', { plans: 1, summaries: 1, verification: 'passed' }); + writeWsPhase(wsDir, '4-new-b', { plans: 1, summaries: 0 }); // in progress; phase 5 has NO dir + + const inv = inspectWorkstream(tmpDir, 'ws-scope', { active: null }); + assert.ok(inv); + assert.equal(inv.roadmap_phase_count, 3, 'denominator = v2.0 phases {3,4,5}, incl. dirless 5'); + assert.equal(inv.completed_phases, 1, 'only phase 3; shipped v1.0 phases 1,2 excluded'); + assert.equal(inv.progress_percent, 33); + assert.notEqual(inv.status, 'milestone complete'); + assert.equal(inv.status, 'executing'); + }); + + // Reporter's minimal fixture (issue #2562): a FLAT Progress table (no Milestone + // column, so milestone scoping cannot engage) where phase 2 is declared as a + // table row only — no `### Phase 2` heading, no directory. The heading-only + // count sees just phase 1 and silently drops phase 2 from the denominator. + test('inspectWorkstream: flat Progress table — a table-only phase still counts in the denominator', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-flat' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), 'status: executing\n'); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ + '# Roadmap', '', '## Phases', '', '### Phase 1: Foo', '**Goal:** foo', '', + '## Progress', '', + '| Phase | Plans Complete | Status | Completed |', + '| --- | --- | --- | --- |', + '| 1. Foo | 1/1 | Complete | - |', + '| 2. Bar | 0/1 | Not started | - |', + '', + ].join('\n')); + writeWsPhase(wsDir, '1-foo', { plans: 1, summaries: 1, verification: 'gaps_found' }); + + const inv = inspectWorkstream(tmpDir, 'ws-flat', { active: null }); + assert.ok(inv); + assert.equal(inv.roadmap_phase_count, 2, 'table-only phase 2 must not vanish from the denominator'); + assert.equal(inv.phases[0].status, 'in_progress', 'gaps_found verdict is not complete'); + assert.equal(inv.completed_phases, 0); + assert.equal(inv.progress_percent, 0); + }); + + // A sub-phase inserted mid-milestone (`3.1-…`) has no Progress-table row. It + // inherits its parent's milestone and must land on BOTH sides of the rollup: + // numerator-only would let completed_phases exceed the denominator and cap + // back to 100%, reintroducing the very defect this issue is about. + test('inspectWorkstream: a dir-only sub-phase counts in BOTH numerator and denominator', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-subphase' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), MS_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), MS_ROADMAP); + // v2.0 declares 3,4,5. All three complete, PLUS a dir-only 3.1 still in progress. + writeWsPhase(wsDir, '3-new-a', { plans: 1, summaries: 1, verification: 'passed' }); + writeWsPhase(wsDir, '3.1-inserted', { plans: 2, summaries: 0 }); + writeWsPhase(wsDir, '4-new-b', { plans: 1, summaries: 1, verification: 'passed' }); + writeWsPhase(wsDir, '5-new-c', { plans: 1, summaries: 1, verification: 'passed' }); + + const inv = inspectWorkstream(tmpDir, 'ws-subphase', { active: null }); + assert.ok(inv); + assert.equal(inv.roadmap_phase_count, 4, 'denominator = declared {3,4,5} + inherited 3.1'); + assert.equal(inv.completed_phases, 3); + assert.equal(inv.progress_percent, 75, 'the in-progress sub-phase must hold this below 100'); + assert.equal(inv.total_plans, 5, 'the sub-phase contributes its plans too'); + }); + + test('inspectWorkstream: a PRIOR-version snapshot does not mark the current milestone complete', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-prior-snap' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), MS_STATE); // current milestone = v2.0 + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), MS_ROADMAP); + fs.mkdirSync(path.join(wsDir, 'milestones'), { recursive: true }); + fs.writeFileSync(path.join(wsDir, 'milestones', 'v1.0-ROADMAP.md'), '# v1.0 archived\n'); + writeWsPhase(wsDir, '3-new-a', { plans: 1, summaries: 1, verification: 'passed' }); + + const inv = inspectWorkstream(tmpDir, 'ws-prior-snap', { active: null }); + assert.ok(inv); + assert.equal(inv.status, 'executing', 'v1.0 snapshot must not mark v2.0 complete'); + assert.equal(inv.status_source, 'field'); + }); + + test('inspectWorkstream: the CURRENT-version snapshot marks the milestone complete', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-cur-snap' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), MS_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), MS_ROADMAP); + fs.mkdirSync(path.join(wsDir, 'milestones'), { recursive: true }); + fs.writeFileSync(path.join(wsDir, 'milestones', 'v2.0-ROADMAP.md'), '# v2.0 archived\n'); + + const inv = inspectWorkstream(tmpDir, 'ws-cur-snap', { active: null }); + assert.ok(inv); + assert.equal(inv.status, 'milestone complete'); + assert.equal(inv.status_source, 'derived'); + }); +}); + +// #2562 review round 2 — every one of these is a DISTINCT way to reproduce this +// issue's own symptom ("milestone complete"/100% while phases are incomplete), +// introduced by deriving the two sides of the rollup from different phase-key +// derivations rather than from the phase-id owner module. Each test reddens when +// only its own fix is reverted. +describe('#2562 — milestone scoping boundaries (one phase-key derivation)', () => { + let tmpDir; + before(() => { tmpDir = createFixture(); }); + after(() => cleanup(tmpDir)); + + function writePhase(wsDir, slug, { plans = 0, summaries = 0, verification } = {}) { + const dir = path.join(wsDir, 'phases', slug); + fs.mkdirSync(dir, { recursive: true }); + for (let i = 1; i <= plans; i++) fs.writeFileSync(path.join(dir, `0${i}-PLAN.md`), '# plan\n'); + for (let i = 1; i <= summaries; i++) fs.writeFileSync(path.join(dir, `0${i}-SUMMARY.md`), '# summary\n'); + if (verification) fs.writeFileSync(path.join(dir, '01-VERIFICATION.md'), `---\nstatus: ${verification}\n---\n`); + } + + function roadmapWithRows(rows) { + return [ + '# Roadmap', '', '## Progress', '', + '| Phase | Milestone | Plans Complete | Status | Completed |', + '| --- | --- | --- | --- | --- |', + ...rows, + '', + ].join('\n'); + } + + const V2_STATE = 'milestone: v2.0\nstatus: executing\n'; + + // A zero-padded roadmap table against zero-padded directories. Deriving the + // table key with one regex and the directory key with another put `01` and `1` + // in different key spaces: NOTHING matched, every phase fell out of the + // milestone, and the rollup reported 0% while listing both phases complete. + test('zero-padded table rows match zero-padded directories', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-padded' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), roadmapWithRows([ + '| 01. Alpha | v2.0 | 1/1 | Complete | - |', + '| 02. Beta | v2.0 | 1/1 | Complete | - |', + ])); + writePhase(wsDir, '01-alpha', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '02-beta', { plans: 1, summaries: 1, verification: 'passed' }); + + const inv = inspectWorkstream(tmpDir, 'ws-padded', { active: null }); + assert.ok(inv); + assert.equal(inv.roadmap_phase_count, 2); + assert.equal(inv.completed_phases, 2, 'padded dirs must match padded table rows'); + assert.equal(inv.progress_percent, 100); + assert.deepEqual(inv.phases.map(p => p.status), ['complete', 'complete'], + 'phases[] and the rollup must agree'); + }); + + // The mirror image: an UNPADDED table against PADDED directories. Padding is a + // presentation choice on either side; one key function makes it irrelevant. + test('unpadded table rows match padded directories (and vice versa)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-mixed-pad' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), roadmapWithRows([ + '| 1. Alpha | v2.0 | 1/1 | Complete | - |', + '| 2. Beta | v2.0 | 0/1 | In Progress | - |', + ])); + writePhase(wsDir, '01-alpha', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '02-beta', { plans: 1, summaries: 0 }); + + const inv = inspectWorkstream(tmpDir, 'ws-mixed-pad', { active: null }); + assert.ok(inv); + assert.equal(inv.roadmap_phase_count, 2, 'the two phases must not double-count as four'); + assert.equal(inv.completed_phases, 1); + assert.equal(inv.progress_percent, 50); + }); + + // A project-code-prefixed directory (`PROJ-05-…`). The bespoke `^0*(\d+…)` + // directory parser yielded null for these, excluding EVERY directory from the + // milestone and pinning the workstream at 0% forever. + test('project-code-prefixed directories are scoped, not excluded', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-projcode' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), roadmapWithRows([ + '| PROJ-01. Shipped | v1.0 | 1/1 | Complete | - |', + '| PROJ-05. Alpha | v2.0 | 1/1 | Complete | - |', + '| PROJ-06. Beta | v2.0 | 0/1 | In Progress | - |', + ])); + // The prior-milestone directory is the discriminator: a phase key that never + // resolves collapses scoping to the whole roadmap, and PROJ-01 sneaks into + // the current rollup as a third completed phase. + writePhase(wsDir, 'PROJ-01-shipped', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, 'PROJ-05-alpha', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, 'PROJ-06-beta', { plans: 1, summaries: 0 }); + + const inv = inspectWorkstream(tmpDir, 'ws-projcode', { active: null }); + assert.ok(inv); + assert.equal(inv.roadmap_phase_count, 2, 'denominator = v2.0 phases only'); + assert.equal(inv.completed_phases, 1, 'prefixed dirs must scope, not be excluded outright'); + assert.equal(inv.progress_percent, 50); + }); + + // The load-bearing invariant of the whole fix, stated directly: however a + // roadmap decorates a phase reference (padding, project code, markdown + // emphasis, a `Phase ` label, trailing prose), the key it yields must equal + // the key its own directory yields. Every blocker above is an instance of + // this property failing. + test('property: a table cell and its directory always yield the same phase key', () => { + // Padding and project code are decoration and vary INDEPENDENTLY on the two + // sides — that independence is the point. Comparing an identically-decorated + // token against itself would pass vacuously. + fc.assert(fc.property( + fc.integer({ min: 1, max: 400 }), + fc.option(fc.integer({ min: 1, max: 99 }), { nil: null }), + fc.constantFrom('', '0', '00'), + fc.constantFrom('', '0', '00'), + fc.constantFrom('', 'PROJ-', 'CK-', 'MEM-'), + fc.constantFrom('', 'PROJ-', 'CK-', 'MEM-'), + fc.constantFrom('', '**', '`'), + fc.constantFrom('', 'Phase '), + fc.stringMatching(/^[a-z][a-z-]{0,20}$/), + (num, sub, cellPad, dirPad, cellCode, dirCode, emphasis, label, slug) => { + const suffix = sub === null ? '' : `.${sub}`; + const cell = `${emphasis}${label}${cellCode}${cellPad}${num}${suffix}. Some Name${emphasis}`; + const dir = `${dirCode}${dirPad}${num}${suffix}-${slug}`; + assert.equal(phaseKeyFromProse(cell), phaseKeyFromDir(dir), + `cell ${JSON.stringify(cell)} and dir ${JSON.stringify(dir)} must share a key`); + }, + ), { numRuns: 1000 }); + }); + + // A blank Milestone cell must not silently delete the phase from BOTH sides of + // the calculation — that is how an unstarted phase vanished and the remaining + // completed one rounded the workstream to 100%. + test('a blank Milestone cell keeps the phase in the denominator', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-blank-cell' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), roadmapWithRows([ + '| 1. Alpha | v2.0 | 1/1 | Complete | - |', + '| 2. Beta | | 0/1 | Not started | - |', + '| 3. Gamma | TBD | 0/1 | Not started | - |', + ])); + writePhase(wsDir, '1-alpha', { plans: 1, summaries: 1, verification: 'passed' }); + + const inv = inspectWorkstream(tmpDir, 'ws-blank-cell', { active: null }); + assert.ok(inv); + assert.equal(inv.roadmap_phase_count, 3, 'unattributable rows degrade over-inclusively'); + assert.equal(inv.completed_phases, 1); + assert.equal(inv.progress_percent, 33); + assert.notEqual(inv.progress_percent, 100); + }); + + // A bullet that merely NAMES the current version with a checkmark is prose + // about a phase, not a milestone verdict. Reading it as a shipped signal + // reproduces this issue's exact symptom. + test('a checkmarked bullet naming the version does NOT mark the milestone shipped', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-bullet-tick' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ + roadmapWithRows([ + '| 1. Alpha | v2.0 | 1/1 | Complete | - |', + '| 2. Beta | v2.0 | 0/1 | In Progress | - |', + ]), + '## Plans', + '- [x] 03-01: Ship the v2.0 login endpoint ✅', + '', + ].join('\n')); + writePhase(wsDir, '1-alpha', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '2-beta', { plans: 1, summaries: 0 }); + + const inv = inspectWorkstream(tmpDir, 'ws-bullet-tick', { active: null }); + assert.ok(inv); + assert.equal(inv.status, 'executing'); + assert.equal(inv.status_source, 'field'); + assert.equal(inv.progress_percent, 50); + }); + + // `\b` does not bound a version token: `.` is a non-word character, so a naive + // `\bv2\.0\b` matches inside `v2.0.1`. A shipped SIBLING patch release must not + // close the current milestone. + test('a shipped v2.0.1 heading does NOT mark v2.0 shipped', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-version-boundary' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ + roadmapWithRows(['| 1. Alpha | v2.0 | 0/1 | In Progress | - |']), + '## v2.0.1 Patch — ✅ SHIPPED', + '', + ].join('\n')); + writePhase(wsDir, '1-alpha', { plans: 1, summaries: 0 }); + + const inv = inspectWorkstream(tmpDir, 'ws-version-boundary', { active: null }); + assert.ok(inv); + assert.equal(inv.status, 'executing'); + assert.equal(inv.progress_percent, 0); + }); + + // `phaseKeyFromToken` strips leading zeros per hyphen-separated segment before + // `normalizePhaseName` runs — which is BEFORE that function strips the + // project-code prefix. lint-phase-id-drift exempts phase-id.cts by design, so + // it is silent here by construction; these cases are the coverage instead. + test('key derivation is symmetric across project codes and hyphenated ids', () => { + for (const [token, dir] of [ + ['CK-01', 'CK-01-x'], + ['CK-1', 'CK-001-x'], // padding differs across the prefix + ['M1-2', 'M1-2-x'], + ['P0.3-2', 'P0.3-2-x'], // letter-prefixed leading segment, preserved verbatim + ['01-02', '01-02-x'], + ]) { + assert.equal(phaseKeyFromToken(token), phaseKeyFromDir(dir), + `token ${token} and dir ${dir} must share a key`); + } + // Known, PRE-EXISTING asymmetry, pinned so it is not "fixed" by accident: + // in a DIRECTORY a single-digit segment after the phase number is a slug + // word, not a sub-phase (#2043/#2232 — `extractPhaseToken`), so `M1-46-6-rs` + // is phase 46. A ROADMAP token `M1-46-6` has no slug and is phase 46-06. + // Unchanged by #2562: both sides behaved this way before. + assert.equal(phaseKeyFromToken('M1-46-6'), '46-06'); + assert.equal(phaseKeyFromDir('M1-46-6-rs-x'), '46'); + }); + + // The Builder's invariant throw is a contract assertion for external callers. + // `listWorkstreamInventories` loops every workstream with no try/catch, so a + // reachable throw would take down `workstream list`/`status`/`progress` for + // ALL workstreams — this pins that the real reader cannot construct one, with + // every adversarial shape at once. + test('inspectWorkstream cannot trip the Builder invariant', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-adversarial' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), roadmapWithRows([ + '| 1. Prior | v1.0 | 2/2 | Complete | - |', + '| 01. Dupe | v2.0 | 1/1 | Complete | - |', + '| 2. Dirless | v2.0 | 0/1 | Not started | - |', + '| 3. Unattributed | | 0/1 | Not started | - |', + '| PROJ-04. Prefixed | v2.0 | 1/1 | Complete | - |', + ])); + writePhase(wsDir, '1-prior', { plans: 2, summaries: 2, verification: 'passed' }); + writePhase(wsDir, '01-dupe', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '1-dupe-stale', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '001-dupe-staler', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, 'PROJ-04-prefixed', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '3.1-inserted', { plans: 1, summaries: 0 }); + + let inv; + assert.doesNotThrow(() => { inv = inspectWorkstream(tmpDir, 'ws-adversarial', { active: null }); }); + assert.ok(inv); + assert.ok(inv.completed_phases <= inv.roadmap_phase_count, + `numerator ${inv.completed_phases} must not exceed denominator ${inv.roadmap_phase_count}`); + assert.ok(inv.progress_percent < 100, 'incomplete phases must keep this below 100'); + }); + + // A flat table earlier in the document must not shadow the milestone-grouped + // table that carries the attribution: every row would come back unattributed, + // be treated as current-milestone, and silently re-admit prior phases. + test('a milestone-grouped table wins over an earlier flat table', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-two-tables' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ + '# Roadmap', '', '## Summary', '', + '| Phase | Plans Complete | Status | Completed |', + '| --- | --- | --- | --- |', + '| 1. Prior | 2/2 | Complete | - |', + '| 2. Alpha | 1/1 | Complete | - |', + '| 3. Beta | 0/1 | Not started | - |', + '', '## Progress', '', + '| Phase | Milestone | Plans Complete | Status | Completed |', + '| --- | --- | --- | --- | --- |', + '| 1. Prior | v1.0 | 2/2 | Complete | - |', + '| 2. Alpha | v2.0 | 1/1 | Complete | - |', + '| 3. Beta | v2.0 | 0/1 | Not started | - |', + '', + ].join('\n')); + writePhase(wsDir, '1-prior', { plans: 2, summaries: 2, verification: 'passed' }); + writePhase(wsDir, '2-alpha', { plans: 1, summaries: 1, verification: 'passed' }); + + const inv = inspectWorkstream(tmpDir, 'ws-two-tables', { active: null }); + assert.ok(inv); + assert.equal(inv.roadmap_phase_count, 2, 'denominator = v2.0 phases {2,3}'); + assert.equal(inv.completed_phases, 1, 'the shipped v1.0 phase must stay out'); + assert.equal(inv.progress_percent, 50); + }); + + // An in-progress marker on the milestone heading always wins over a checkmark + // elsewhere on the same line — the active-wins rule the sectioniser applies. + test('an in-progress marker on the milestone heading beats a checkmark', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-active-wins' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ + roadmapWithRows(['| 1. Alpha | v2.0 | 0/1 | In Progress | - |']), + '## v2.0 Launch — 🚧 IN PROGRESS (phase 1 scaffolded ✅)', + '', + ].join('\n')); + writePhase(wsDir, '1-alpha', { plans: 1, summaries: 0 }); + + const inv = inspectWorkstream(tmpDir, 'ws-active-wins', { active: null }); + assert.ok(inv); + assert.equal(inv.status, 'executing'); + assert.equal(inv.status_source, 'field'); + }); + + // The current milestone's OWN shipped heading is still honoured — the boundary + // fix must not cost the signal it exists to carry. + test('the current milestone\'s own shipped heading still marks it complete', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-own-heading' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ + roadmapWithRows(['| 1. Alpha | v2.0 | 1/1 | Complete | - |']), + '## v2.0 Launch — ✅ SHIPPED', + '', + ].join('\n')); + writePhase(wsDir, '1-alpha', { plans: 1, summaries: 1, verification: 'passed' }); + + const inv = inspectWorkstream(tmpDir, 'ws-own-heading', { active: null }); + assert.ok(inv); + assert.equal(inv.status, 'milestone complete'); + assert.equal(inv.status_source, 'derived'); + }); + + // Bug #2445's scenario: a stale directory colliding on phase number with a + // current one. Counting the numerator per-DIRECTORY while the denominator + // counts distinct PHASES pushed completed_phases past the denominator, where + // the old Math.min cap reported 100% and hid the unstarted phase. + test('a stale same-numbered directory does not double-count the numerator', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-dupe-dir' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), roadmapWithRows([ + '| 1. Alpha | v2.0 | 1/1 | Complete | - |', + '| 2. Beta | v2.0 | 0/1 | Not started | - |', + ])); + writePhase(wsDir, '1-alpha', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '01-alpha-old', { plans: 1, summaries: 1, verification: 'passed' }); + + const inv = inspectWorkstream(tmpDir, 'ws-dupe-dir', { active: null }); + assert.ok(inv); + assert.equal(inv.roadmap_phase_count, 2); + assert.equal(inv.completed_phases, 1, 'two dirs, one phase'); + assert.equal(inv.progress_percent, 50, 'the unstarted phase 2 must stay visible'); + }); + + // The builder is the pure seam: a caller that hands it an inconsistent pair + // must fail loudly rather than have Math.min round the contradiction to 100%. + test('builder: a numerator above the denominator throws instead of capping to 100%', () => { + assert.throws(() => buildWorkstreamInventory({ + name: 'ws', + projectDir: '/tmp/ws-proj', + workstreamDir: '/tmp/ws-proj/.planning/workstreams/ws', + activeWorkstreamName: '', + stateProjection: { status: 'executing', current_phase: null, last_activity: null }, + filesExist: { roadmap: true, state: true, requirements: true }, + milestoneShipped: false, + phaseDirNames: ['1-a', '2-b', '3-c'], + phaseFilesCounts: [ + { directory: '1-a', phaseKey: '01', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, + { directory: '2-b', phaseKey: '02', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, + { directory: '3-c', phaseKey: '03', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: 'passed' }, + ], + roadmapPhaseCount: 3, + currentMilestonePhaseCount: 2, + }), /invariant violated/); + }); + + // The builder hand-lists the verdicts that disqualify a phase from `complete`. + // Pin it to the verifier's own vocabulary so a new emitted status cannot land + // without a decision here. + test('parity: every verifier status other than passed blocks completeness', () => { + const nonPassing = VERIFIER_STATUSES.filter(s => s !== 'passed'); + assert.ok(nonPassing.length > 0, 'guard: the verifier must emit a non-passing status'); + for (const status of nonPassing) { + const inv = buildWorkstreamInventory({ + name: 'ws', + projectDir: '/tmp/ws-proj', + workstreamDir: '/tmp/ws-proj/.planning/workstreams/ws', + activeWorkstreamName: '', + stateProjection: { status: 'executing', current_phase: null, last_activity: null }, + filesExist: { roadmap: true, state: true, requirements: true }, + milestoneShipped: false, + phaseDirNames: ['1-a'], + phaseFilesCounts: [ + { directory: '1-a', phaseKey: '01', planCount: 1, summaryCount: 1, inMilestone: true, verificationStatus: status }, + ], + roadmapPhaseCount: 1, + currentMilestonePhaseCount: 1, + }); + assert.equal(inv.phases[0].status, 'in_progress', `verifier status "${status}" must not count complete`); + } + }); + + // The scoping reads the WORKSTREAM's ROADMAP/STATE pair, not the project root's. + // getMilestonePhaseFilter resolves via planningDir(cwd, ws); without the ws + // argument it falls back to GSD_WORKSTREAM, which a loop over workstreams + // cannot set per iteration. + test('scoping reads the workstream ROADMAP, not the project-root ROADMAP', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-not-root' }); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), [ + '# Root Roadmap', '', '## v2.0 Root — ✅ SHIPPED', '', '### Phase 9: Root only', '**Goal:** root', '', + ].join('\n')); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), 'milestone: v2.0\nstatus: milestone complete\n'); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), roadmapWithRows([ + '| 1. Alpha | v2.0 | 0/1 | In Progress | - |', + ])); + writePhase(wsDir, '1-alpha', { plans: 1, summaries: 0 }); + + const inv = inspectWorkstream(tmpDir, 'ws-not-root', { active: null }); + assert.ok(inv); + assert.equal(inv.status, 'executing', 'the ROOT roadmap\'s shipped v2.0 must not leak in'); + assert.equal(inv.roadmap_phase_count, 1, 'root-only phase 9 must not join the denominator'); + assert.equal(inv.progress_percent, 0); + }); + + // ─── The declared-but-empty current milestone ────────────────────────────── + // + // STATE.md's `milestone:` field updates the moment `/gsd-new-milestone` writes + // the heading; the Progress table and phase sections land later. In that + // window nothing attributes a phase to the current milestone. Scoping used to + // switch OFF there, and the fallback counted the project's ENTIRE phase + // history as both numerator and denominator — so a milestone with no work + // done reported 100% off its predecessors'. #2562's own symptom, other route. + // + // Three ROADMAP shapes reach it and each needs its own witness; the fourth is + // the legacy shape that must NOT be caught. + const V3_STATE = 'milestone: v3.0\nstatus: executing\n'; + const PRIOR_ROWS = [ + '| 1. Alpha | v1.0 | 1/1 | Complete | - |', + '| 2. Beta | v2.0 | 1/1 | Complete | - |', + ]; + + function seedTwoShippedPhases(name, roadmap) { + const wsDir = seedWorkstream(tmpDir, { name }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V3_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), roadmap); + writePhase(wsDir, '1-alpha', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '2-beta', { plans: 1, summaries: 1, verification: 'passed' }); + return wsDir; + } + + // The common shape: `## v3.0` exists but has no phases under it yet. The + // milestone filter DOES locate the section (setting versionScoped), then the + // zero-phase pass-all degrade resets versionScoped to false — erasing the only + // evidence that the milestone exists. versionSectionFound survives that reset. + test('a located-but-empty current milestone reports 0%, not its predecessors\' 100%', () => { + const wsDir = seedTwoShippedPhases('ws-empty-section', [ + '# Roadmap', '', + '## v1.0', '', '### Phase 1: Alpha', '', + '## v2.0', '', '### Phase 2: Beta', '', + '## v3.0 — Next', '', + roadmapWithRows(PRIOR_ROWS), + ].join('\n')); + assert.ok(fs.existsSync(wsDir)); + + const inv = inspectWorkstream(tmpDir, 'ws-empty-section', { active: null }); + assert.ok(inv); + assert.equal(inv.completed_phases, 0, 'v1.0/v2.0 phases must not count toward v3.0'); + assert.equal(inv.roadmap_phase_count, 0, 'v3.0 declares no phases'); + assert.equal(inv.progress_percent, 0, 'an unstarted milestone must never report 100%'); + }); + + // No v3.0 section at all, but the ROADMAP versions its other milestones — so + // the filter reports missingExplicitVersion rather than a located section. + test('a current milestone absent from a versioned roadmap reports 0%', () => { + seedTwoShippedPhases('ws-absent-section', [ + '# Roadmap', '', + '## v1.0', '', '### Phase 1: Alpha', '', + '## v2.0', '', '### Phase 2: Beta', '', + roadmapWithRows(PRIOR_ROWS), + ].join('\n')); + + const inv = inspectWorkstream(tmpDir, 'ws-absent-section', { active: null }); + assert.ok(inv); + assert.equal(inv.completed_phases, 0); + assert.equal(inv.progress_percent, 0); + }); + + // Unversioned phase headings, but the Progress table attributes every row to + // another milestone. Neither filter flag fires; the table is the only witness. + test('a progress table attributing every row elsewhere reports 0%', () => { + seedTwoShippedPhases('ws-rows-elsewhere', [ + '# Roadmap', '', + '### Phase 1: Alpha', '', '### Phase 2: Beta', '', + roadmapWithRows(PRIOR_ROWS), + ].join('\n')); + + const inv = inspectWorkstream(tmpDir, 'ws-rows-elsewhere', { active: null }); + assert.ok(inv); + assert.equal(inv.completed_phases, 0); + assert.equal(inv.progress_percent, 0); + }); + + // The boundary. A free-form ROADMAP attributes no versions at all: its rows + // parse unattributed, land in the current milestone, and must keep reporting + // 100%. readCurrentMilestoneVersion hands back a non-null version for nearly + // every project, so scoping on `currentVersion` alone would zero these out. + test('a legacy roadmap with no version attribution keeps its whole-roadmap count', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-legacy-freeform' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V3_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ + '# Roadmap', '', '### Phase 1: Alpha', '', '### Phase 2: Beta', '', + '## Progress', '', + '| Phase | Plans Complete | Status |', + '| --- | --- | --- |', + '| 1. Alpha | 1/1 | Complete |', + '| 2. Beta | 1/1 | Complete |', '', + ].join('\n')); + writePhase(wsDir, '1-alpha', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '2-beta', { plans: 1, summaries: 1, verification: 'passed' }); + + const inv = inspectWorkstream(tmpDir, 'ws-legacy-freeform', { active: null }); + assert.ok(inv); + assert.equal(inv.completed_phases, 2, 'unattributed phases belong to the current milestone'); + assert.equal(inv.progress_percent, 100, 'legacy free-form projects must not regress to 0%'); + }); + + // Degrade direction: over-inclusive, never under. A phase scaffolded before + // the ROADMAP caught up is claimed by NO milestone, so the empty current one + // adopts it rather than dropping it from both sides of the rollup — hiding + // real work would be the same class of defect as inventing it. + test('a phase scaffolded before the roadmap catches up joins the empty milestone', () => { + const wsDir = seedTwoShippedPhases('ws-scaffold-first', [ + '# Roadmap', '', + '## v1.0', '', '### Phase 1: Alpha', '', + '## v2.0', '', '### Phase 2: Beta', '', + '## v3.0 — Next', '', + roadmapWithRows(PRIOR_ROWS), + ].join('\n')); + writePhase(wsDir, '3-gamma', { plans: 1, summaries: 0 }); + + const inv = inspectWorkstream(tmpDir, 'ws-scaffold-first', { active: null }); + assert.ok(inv); + assert.equal(inv.roadmap_phase_count, 1, 'the unclaimed phase 3 is v3.0\'s, and its only one'); + assert.equal(inv.completed_phases, 0, 'it is started, not finished'); + assert.equal(inv.progress_percent, 0); + }); + + // The invariant throw at the builder fires on `completedPhases > denominator`. + // A scoped milestone with a zero denominator sits one bad exclusion away from + // crashing `workstream list` on every freshly-declared milestone — a worse + // failure than a wrong percentage. Pin that it stays a number. + test('a zero-denominator scoped milestone does not trip the rollup invariant', () => { + seedTwoShippedPhases('ws-zero-denominator', [ + '# Roadmap', '', '## v3.0 — Next', '', roadmapWithRows(PRIOR_ROWS), + ].join('\n')); + + assert.doesNotThrow(() => inspectWorkstream(tmpDir, 'ws-zero-denominator', { active: null })); + const inv = inspectWorkstream(tmpDir, 'ws-zero-denominator', { active: null }); + assert.equal(inv.progress_percent, 0); + assert.equal(inv.phases.length, 2, 'the phases themselves stay listed, they just do not count'); + }); +}); + +// #2562 review round 4 — `status` was the one field still asserted from an +// un-cross-validated shipped marker: `progress_percent` derived from artifacts +// while `status` echoed the marker, so the two could contradict each other in a +// single payload. That IS this issue's symptom, reached through `status`. +// +// The cross-check differs by signal strength, and using one check for both +// regresses the archived case — see the builder comment. These four pin both +// halves plus the window where the STATE field re-asserts the refused claim. +describe('#2562 — a shipped marker its own artifacts contradict is not asserted as status', () => { + let tmpDir; + before(() => { tmpDir = createFixture(); }); + after(() => cleanup(tmpDir)); + + const V2_STATE = 'milestone: v2.0\nstatus: executing\n'; + const V2_SHIPPED_STATE = 'milestone: v2.0\nstatus: milestone complete\n'; + // v2.0 declares phases 3 and 4. Rows survive archiving: `milestone complete` + // COPIES ROADMAP.md to the snapshot (milestone.cts:671-674) and never + // truncates the live file. + const V2_ROADMAP = shipped => [ + '# Roadmap', '', + `## Milestone v2.0 — Two${shipped ? ' — ✅ SHIPPED' : ''}`, '', + '## Progress', '', + '| Phase | Milestone | Plans | Status | Done |', + '| --- | --- | --- | --- | --- |', + '| 3. New A | v2.0 | 1/1 | Complete | - |', + '| 4. New B | v2.0 | 0/1 | In Progress | - |', + '', + ].join('\n'); + + function writePhase(wsDir, slug, { plans = 0, summaries = 0, verification } = {}) { + const dir = path.join(wsDir, 'phases', slug); + fs.mkdirSync(dir, { recursive: true }); + for (let i = 1; i <= plans; i++) fs.writeFileSync(path.join(dir, `0${i}-PLAN.md`), '# plan\n'); + for (let i = 1; i <= summaries; i++) fs.writeFileSync(path.join(dir, `0${i}-SUMMARY.md`), '# summary\n'); + if (verification) fs.writeFileSync(path.join(dir, '01-VERIFICATION.md'), `---\nstatus: ${verification}\n---\n`); + } + + function writeSnapshot(wsDir) { + fs.mkdirSync(path.join(wsDir, 'milestones'), { recursive: true }); + fs.writeFileSync(path.join(wsDir, 'milestones', 'v2.0-ROADMAP.md'), '# v2.0 archived\n'); + } + + test('a live-ROADMAP SHIPPED heading is refused while the milestone is incomplete', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-heading-lies' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), V2_ROADMAP(true)); + writePhase(wsDir, '3-new-a', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '4-new-b', { plans: 1, summaries: 0 }); + + const inv = inspectWorkstream(tmpDir, 'ws-heading-lies', { active: null }); + assert.ok(inv); + assert.notEqual(inv.status, 'milestone complete', 'phase 4 is unfinished — the heading is a claim, not a fact'); + assert.equal(inv.milestone_shipped_unverified, true); + assert.equal(inv.progress_percent, 50, 'status and percent must agree'); + }); + + // Nothing on disk contradicts the archive: `milestone complete` MOVES phase + // dirs into `milestones/v2.0-phases/` (milestone.cts:755-762) while leaving + // the Progress rows, so a correctly-archived milestone reads 0/2 by + // construction. Gating this on the completeness ratio — the obvious single + // fix — reddens here and strips `milestone complete` from every archived + // milestone in every project. + test('an archived snapshot survives its phase dirs being moved out (no ratio gate)', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-archived-clean' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), V2_ROADMAP(false)); + writeSnapshot(wsDir); // phases/ is empty — the dirs are under milestones/v2.0-phases/ + + const inv = inspectWorkstream(tmpDir, 'ws-archived-clean', { active: null }); + assert.ok(inv); + assert.equal(inv.status, 'milestone complete'); + assert.equal(inv.status_source, 'derived'); + assert.equal(inv.milestone_shipped_unverified, false); + }); + + // The narrow predicate "a live dir that is itself unfinished" was NOT enough: + // here the live dir is COMPLETE and the unfinished phase 2 is declared with no + // directory, so nothing is live-and-unfinished and the marker sailed through, + // reproducing the reported symptom verbatim (`milestone complete` beside 50%). + // What makes the ratio meaningful again is simply that the archive is dirty — + // any in-milestone directory outliving it — so the check is the conjunction. + test('an archived snapshot is refused when a COMPLETE live dir sits beside a dirless phase', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-archived-dirty' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), V2_ROADMAP(false)); + writeSnapshot(wsDir); + writePhase(wsDir, '3-new-a', { plans: 1, summaries: 1, verification: 'passed' }); // complete, still live + // phase 4 is declared in the Progress table with NO directory + + const inv = inspectWorkstream(tmpDir, 'ws-archived-dirty', { active: null }); + assert.ok(inv); + assert.equal(inv.completed_phases, 1); + assert.equal(inv.roadmap_phase_count, 2, 'the dirless phase 4 stays in the denominator'); + assert.equal(inv.progress_percent, 50); + assert.notEqual(inv.status, 'milestone complete', 'status must not contradict the percentage'); + assert.equal(inv.milestone_shipped_unverified, true); + }); + + // `milestone complete` does not advance STATE's `milestone:` field + // (state-transition.cts:1335 writes status/last_activity only; :1224 is the + // separate new-milestone path), so a phase can be added or reopened while the + // shipped version is still current. That live dir DOES contradict the archive. + test('an archived snapshot is refused once a phase is reopened under it', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-archived-reopened' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), V2_ROADMAP(false)); + writeSnapshot(wsDir); + writePhase(wsDir, '4-new-b', { plans: 2, summaries: 1 }); // reopened after the archive + + const inv = inspectWorkstream(tmpDir, 'ws-archived-reopened', { active: null }); + assert.ok(inv); + assert.notEqual(inv.status, 'milestone complete'); + assert.equal(inv.milestone_shipped_unverified, true); + }); + + // The refusal must not leak back in through the STATE field, which is + // operator-written and in this window commonly says the same thing. + test('a refused marker is not re-asserted by a STATE field claiming the same', () => { + const wsDir = seedWorkstream(tmpDir, { name: 'ws-field-echo' }); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), V2_SHIPPED_STATE); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), V2_ROADMAP(true)); + writePhase(wsDir, '3-new-a', { plans: 1, summaries: 1, verification: 'passed' }); + writePhase(wsDir, '4-new-b', { plans: 1, summaries: 0 }); + + const inv = inspectWorkstream(tmpDir, 'ws-field-echo', { active: null }); + assert.ok(inv); + assert.equal(isCompletedInventory(inv.status), false, 'neither source may assert completion here'); + assert.equal(inv.status_conflict, true, 'the field disagrees with the artifacts'); + assert.equal(inv.milestone_shipped_unverified, true); + }); +}); diff --git a/tests/workstream.test.cjs b/tests/workstream.test.cjs index 3e18e7e67..ed1620fcf 100644 --- a/tests/workstream.test.cjs +++ b/tests/workstream.test.cjs @@ -845,3 +845,89 @@ describe('path traversal rejection', () => { } }); }); + +// #2562: the inventory can refuse a shipped marker its artifacts contradict, but +// a refusal that no command projects is the same silent collapse the issue is +// about — the operator sees a fallback `status` and nothing saying a marker was +// seen and rejected. These assert at the CLI, the surface that was missing it, +// not at the builder that already had the field. +describe('#2562 — a refused shipped marker reaches every workstream command', () => { + let tmpDir; + + before(() => { + tmpDir = createFixture(); + const wsDir = path.join(tmpDir, '.planning', 'workstreams', 'dirty-archive'); + // A milestone that was archived (snapshot present) and then reopened: phase 1 + // is complete and still on disk, phase 2 is declared with no directory. The + // archive is therefore not clean AND the ratio is short. + fs.mkdirSync(path.join(wsDir, 'phases', '1-foo'), { recursive: true }); + fs.writeFileSync(path.join(wsDir, 'phases', '1-foo', '01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(wsDir, 'phases', '1-foo', '01-SUMMARY.md'), '# Summary\n'); + fs.writeFileSync(path.join(wsDir, 'STATE.md'), 'milestone: v2.0\nstatus: executing\n'); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ + '# Roadmap', '', '## Milestone v2.0 — Two', '', '## Progress', '', + '| Phase | Milestone | Plans | Status | Done |', + '| --- | --- | --- | --- | --- |', + '| 1. Foo | v2.0 | 1/1 | Complete | - |', + '| 2. Bar | v2.0 | 0/1 | Not started | - |', '', + ].join('\n')); + fs.mkdirSync(path.join(wsDir, 'milestones'), { recursive: true }); + fs.writeFileSync(path.join(wsDir, 'milestones', 'v2.0-ROADMAP.md'), '# v2.0 archived\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'active-workstream'), 'dirty-archive\n'); + }); + + after(() => cleanup(tmpDir)); + + test('workstream progress projects the refusal beside the percentage', () => { + const result = runGsdTools(['workstream', 'progress', '--raw'], tmpDir); + assert.ok(result.success, `progress failed: ${result.error}`); + const ws = JSON.parse(result.output).workstreams.find(w => w.name === 'dirty-archive'); + assert.ok(ws, 'workstream missing from progress output'); + assert.strictEqual(ws.milestone_shipped_unverified, true); + assert.notStrictEqual(ws.status, 'milestone complete', 'status must not contradict the percentage'); + assert.strictEqual(ws.progress_percent, 50); + }); + + test('workstream status projects the refusal', () => { + const result = runGsdTools(['workstream', 'status', 'dirty-archive', '--raw'], tmpDir); + assert.ok(result.success, `status failed: ${result.error}`); + const data = JSON.parse(result.output); + assert.strictEqual(data.found, true); + assert.strictEqual(data.milestone_shipped_unverified, true); + }); + + test('workstream list projects the refusal', () => { + const result = runGsdTools(['workstream', 'list', '--raw'], tmpDir); + assert.ok(result.success, `list failed: ${result.error}`); + const ws = JSON.parse(result.output).workstreams.find(w => w.name === 'dirty-archive'); + assert.ok(ws, 'workstream missing from list output'); + assert.strictEqual(ws.milestone_shipped_unverified, true); + }); + + test('a clean archive reports no refusal at the CLI', () => { + const isolatedDir = createFixture(); + try { + const wsDir = path.join(isolatedDir, '.planning', 'workstreams', 'clean-archive'); + fs.mkdirSync(path.join(wsDir, 'phases'), { recursive: true }); // dirs moved out by the archive + fs.writeFileSync(path.join(wsDir, 'STATE.md'), 'milestone: v2.0\nstatus: executing\n'); + fs.writeFileSync(path.join(wsDir, 'ROADMAP.md'), [ + '# Roadmap', '', '## Milestone v2.0 — Two', '', '## Progress', '', + '| Phase | Milestone | Plans | Status | Done |', + '| --- | --- | --- | --- | --- |', + '| 1. Foo | v2.0 | 1/1 | Complete | - |', + '| 2. Bar | v2.0 | 1/1 | Complete | - |', '', + ].join('\n')); + fs.mkdirSync(path.join(wsDir, 'milestones'), { recursive: true }); + fs.writeFileSync(path.join(wsDir, 'milestones', 'v2.0-ROADMAP.md'), '# v2.0 archived\n'); + + const result = runGsdTools(['workstream', 'progress', '--raw'], isolatedDir); + assert.ok(result.success, `progress failed: ${result.error}`); + const ws = JSON.parse(result.output).workstreams.find(w => w.name === 'clean-archive'); + assert.ok(ws, 'workstream missing from progress output'); + assert.strictEqual(ws.milestone_shipped_unverified, false); + assert.strictEqual(ws.status, 'milestone complete'); + } finally { + cleanup(isolatedDir); + } + }); +});