diff --git a/.changeset/lively-mice-wave.md b/.changeset/lively-mice-wave.md new file mode 100644 index 000000000..e778c94fe --- /dev/null +++ b/.changeset/lively-mice-wave.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3893 +--- +**Sentinel phases no longer skew estimation calibration.** Backlog and icebox phase directories (milestones 0 and 999) were counted as completed phases when rebuilding the calibration factor, so a single one could switch calibration on from phantom evidence and two could corrupt the factor outright. (#3882) diff --git a/CONTEXT.md b/CONTEXT.md index 513f5e582..87888bf7e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -30,7 +30,9 @@ Module owning the canonical phase-verification status projection shared by phase Module owning the deterministic resolvability probe over PLAN.md `` verify commands (#2401), plus the prior-phase command harvest that feeds the planner. `extractAutomatedCommands(planText)` pulls every `…` body with its owning ``, in document order, via a ReDoS-safe stop-at-next-open task pattern (shape mirrors `PLAN_TASK_BLOCK_RE` in `verify.cjs`) and a monotonic span pointer; non-string input yields `[]`. `resolveVerifyCommandTarget(command, {projectRoot, declaredPaths})` is a **RECOGNIZER, not a shell interpreter** (deliberate, per Greenspun): it grounds exactly two forms — a folded leading `cd ` chain and `npm --prefix ` — and any path carrying `$`, a backtick, `*`, `?`, `~`, or a newline returns `unresolvable`/`dynamic_path` at WARNING severity, never BLOCKER. Status is a closed 5-atom enum (`ok`/`broken`/`unresolvable`/`not_applicable`/`pending_creation`) and severity a closed 3-atom enum (`blocker`/`warning`/`none`); `broken` is only ever `missing_dir` or `no_manifest`, while `script_missing`/`manifest_unreadable`/`outside_root` stay advisory on an `ok` status. A target an earlier task in the same phase declares (`` or the `## Artifacts this phase produces` section) is `pending_creation`, never a blocker — without that, every greenfield phase would red. A bare ancestor climb (`cd ../..`, every segment `..`) short-circuits to `outside_root` without touching the filesystem, because the checker's root and a parallel executor worktree's root differ; a climb naming a concrete sibling (`cd ../../frontend` — the exact #2401 shape) still names something checkable and is probed normally. **The module never executes command text** (`fs.statSync`/`existsSync`/`readFileSync`/`readdirSync` only — PLAN.md is model-authored untrusted input) and deliberately exposes **no `suggestion` field**: prescribing a replacement path is the failure being fixed, not the fix. `probePhaseVerifyCommands({phaseDir, projectRoot})` backs `gsd-tools check verify-command-paths ` (routed in `check-command-router.cjs`), degrading to a populated `readError` rather than throwing — an empty `commands` with a non-empty `readError` means *could not look*, not *nothing to report*. `harvestPriorVerifyCommands({planningDir, beforePhase, limit=20, lookback=3})` walks descending phase dirs for the nearest prior phase with any command, deduped first-seen and capped, and is emitted as `init.plan-phase`'s `prior_verify_commands` **ungated by `context_window`** — the `>= 500000` enrichment gate is exactly what starved the planner at 200k. **Failing-direction probe (#3172), the module's second concern.** Every runnable `` must carry a `` sibling naming what output constitutes failure; a command with no expressible failure mode is not an acceptance test. `extractFailingDirections(planText)` recovers `` and `` in ONE document-order pass (a single backreferenced alternation — two independent scans would discard the relative positions the pairing walk needs) over the SAME text units as `extractAutomatedCommands` (each `` body, then the task-stripped remainder), sharing that function's `MAX_BLOCK_WALK` guard, `MISSING_SENTINEL_RE` and task grammar rather than copying them (`DEFECT.GENERATIVE-FIX-DIVERGENCE`); pairing never crosses a task boundary. Each `` binds to the nearest PRECEDING ``, FIRST-WINS — a redundant second statement for one command is ignored and adds no row, and a statement preceding every command is an `orphan` WARNING, never a blocker. `resolveFailingDirection(command, statement)` is the single verdict implementation: status is a closed 6-atom enum (`ok`/`missing`/`empty`/`placeholder`/`sentinel`/`orphan`) over the same 3-atom severity enum, and check ORDER is load-bearing — empty command first, then the `MISSING` Wave-0 sentinel (exempt at `none` even when a statement IS present, because it is not runnable and without that exemption every greenfield phase would red), then missing/empty/placeholder. The placeholder set (`tbd`/`todo`/`n/a`/`na`/`none`/`unknown`/`tba`/`?`/`-`) matches the WHOLE trimmed value case-insensitively, never a substring: `"TBD in the harness output"` is real prose and passes. **PRESENCE only** — whether a statement names the RIGHT signal stays plan-checker judgment at WARNING, so every BLOCKER is deterministic and reproducible. The probe never prescribes a statement, for the same reason it never prescribes a path: a prescribed one is copied verbatim and carries zero information. `probePhaseFailingDirections({phaseDir})` backs `gsd-tools check verify-failure-directions `, degrading to a populated `readError` with top-level `status: 'unresolvable'` — an empty `commands` with a non-empty `readError` means *could not look*, not *nothing to report*. The #2401 path-probe surface is deliberately NOT overloaded (its 5-atom status and counts are unchanged by a missing statement) so `docs/how-to/resolve-verify-command-path-findings.md` stays true. Source of truth: `gsd-core/bin/lib/verify-command-grounding.cjs` (generated from `src/verify-command-grounding.cts`). Design: `.gsd/phase/feat-3172-stated-failing-direction/40-design.md`. ### Phase Locator Module -Module owning phase-directory search and location: active-phase discovery against the `.planning/phases/` tree (`searchPhaseInDir`, `findPhaseInternal`) and archived-phase-dir enumeration (`getArchivedPhaseDirs`), matching phase ids/tokens against the filesystem. Depends only on leaf modules (`phase-id` for token/name matching, `core-utils` for fs-scan/path helpers, `planning-workspace` for `planningDir`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2d (#881); 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-locator.cjs` (generated from `src/phase-locator.cts`). Since #2830, `searchPhaseInDir` also parses each plan's `depends_on` and each completed plan's SUMMARY `status` and calls Plan Dependency Graph Module's `computeHaltPropagation` to populate `halted_plans`/`blocked_by`/`runnable_plans` — additive fields; `incomplete_plans` keeps its pre-#2830 meaning unchanged. Since #3185 (ADR-3180 Decision 1, Phase 3), the module also owns `listMilestonePhaseDirs(phasesDir, { cwd, ws, versionOverride, phaseIdConvention })`, the single canonical owner of milestone-scoped phase-directory enumeration: it applies the current milestone's `ROADMAP.md` window (via `getMilestonePhaseFilter`) and then the canonical `isSentinelPhaseId` sentinel filter, in that order, over the raw `phasesDir` directory listing. It returns `{ value: string[], scope }`, where `scope` is the `SCOPE` enum from `src/planning-scope.cts` (`complete`/`truncated`/`unscoped`/`unreadable`), so a caller can distinguish a genuinely empty milestone from an enumeration that could not be scoped. Consumed by `query progress`, `stats`, and the bare `phases list`, all of which need "which phases belong to this milestone." `phases list --phase` and `--include-archived` (lookup/archive questions) read the unscoped physical directory set and do not call this owner. `phases clear` and `milestone complete`'s phase-archival move call `isSentinelPhaseId` directly instead — they must sweep every non-sentinel phase directory regardless of milestone window, so they take the sentinel filter without this owner's window scoping. +Module owning phase-directory search and location: active-phase discovery against the `.planning/phases/` tree (`searchPhaseInDir`, `findPhaseInternal`) and archived-phase-dir enumeration (`getArchivedPhaseDirs`), matching phase ids/tokens against the filesystem. Depends only on leaf modules (`phase-id` for token/name matching, `core-utils` for fs-scan/path helpers, `planning-workspace` for `planningDir`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2d (#881); 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-locator.cjs` (generated from `src/phase-locator.cts`). Since #2830, `searchPhaseInDir` also parses each plan's `depends_on` and each completed plan's SUMMARY `status` and calls Plan Dependency Graph Module's `computeHaltPropagation` to populate `halted_plans`/`blocked_by`/`runnable_plans` — additive fields; `incomplete_plans` keeps its pre-#2830 meaning unchanged. Since #3185 (ADR-3180 Decision 1, Phase 3), the module also owns `listMilestonePhaseDirs(phasesDir, { cwd, ws, versionOverride, phaseIdConvention })`, the single canonical owner of milestone-scoped phase-directory enumeration: it applies the current milestone's `ROADMAP.md` window (via `getMilestonePhaseFilter`) and then the canonical `isSentinelPhaseId` sentinel filter, in that order, over the raw `phasesDir` directory listing. It returns `{ value: string[], scope }`, where `scope` is the `SCOPE` enum from `src/planning-scope.cts` (`complete`/`truncated`/`unscoped`/`unreadable`), so a caller can distinguish a genuinely empty milestone from an enumeration that could not be scoped. Consumed by `query progress`, `stats`, and the bare `phases list`, all of which need "which phases belong to this milestone." `phases list --phase` and `--include-archived` (lookup/archive questions) read the unscoped physical directory set and do not call this owner. `phases clear` and `milestone complete`'s phase-archival move call `isSentinelPhaseId` directly instead — they must sweep every non-sentinel phase directory regardless of milestone window, so they take the sentinel filter without this owner's window scoping. **Note that combination is obtainable from the owner itself**: called with no `cwd`, `listMilestonePhaseDirs` applies no window (`inWindow = () => true`) while still refusing sentinels unconditionally — "sentinels are never milestone phases" — so an all-milestones, sentinel-free enumeration needs no separate implementation. That is the call `collectCalibrationSamples` was missing. + +Since #3882 (ADR-3473 §8.2) the module also owns **`listAllPhaseDirs(phasesDir, { includeSentinels, phaseIdConvention })`** — the OTHER axis, the physical directory set with sentinel inclusion **stated rather than implied**. `includeSentinels` is required with no default, so sentinel inclusion cannot be obtained by omission; §8.2's *"a caller that wants sentinels asks for them explicitly"* is enforced at compile time, not documented against. It mirrors the owner's absent-vs-unreadable handling and returns the same `{ value, scope }` shape, so the discriminator survives. This is what the lookup-index callers (`cmdRoadmapAnalyze`'s `_phaseDirNames`, `cmdInitMilestoneOp`'s `diskPhaseDirs`) now call instead of hand-rolling a `readdirSync`, and what `buildAllPhaseDirNamesField` (Planning Snapshot Module) delegates to — that function was a near-duplicate of this combination differing only in sort order, and now re-applies its own lexicographic sort over the owner's result because W007's output order is observable. Exactly one `readdirSync` over the phases directory remains across the two modules. ### Plan Dependency Graph Module Module owning the single halt-propagation engine over a plan's `depends_on` DAG (#2830). **Domain term: _halted_** — a plan that reached a designed stop (a gate failure, a spike concluding without expanding, or any other intentional non-completion) and wrote a SUMMARY recording that fact via `status: halted` in its frontmatter, as opposed to `status: complete` (ordinary finish) or no SUMMARY at all (not yet attempted). **Domain term: _blocked_** — a plan whose `depends_on` chain reaches a halted plan, directly or transitively; distinct from merely _incomplete_ (no SUMMARY yet) — an ordinary in-progress/not-yet-started dependency does not block. `computeHaltPropagation(nodes: {id, resolvedDependsOn, halted}[])` performs exactly one Kahn's-algorithm topological pass and returns `{order, visited, blockedBy}`, where `blockedBy` maps a plan id to the de-duplicated set of halted plan ids transitively upstream of it (diamond-safe, any depth). This is the SHARED engine both of the two independent "which plans are incomplete" readers call — `phase.cts`'s wave-grouping (`cmdPhasePlanIndex`) and `phase-locator.cts`'s phase-location primitive (`searchPhaseInDir`) — so the two-implementation divergence that caused #2830 (one parsed `depends_on` for waves only, the other never parsed it at all) cannot recur: each caller resolves its own raw `depends_on` tokens to canonical ids before calling in, but the graph traversal itself exists in exactly one place. Pure — no I/O, no config; each caller does its own file reads (a plan's frontmatter, a completed plan's SUMMARY `status`) and fails open (treats an unreadable/malformed file as "not halted"/"no deps") rather than throwing. Source of truth: `gsd-core/bin/lib/plan-dependency-graph.cjs` (generated from `src/plan-dependency-graph.cts`). diff --git a/docs/json-errors.md b/docs/json-errors.md index 496a6f1a6..eecc376f2 100644 --- a/docs/json-errors.md +++ b/docs/json-errors.md @@ -200,6 +200,12 @@ text (unstable). | `phase_not_found` | Phase directory lookup returns no match | | `summary_no_planning` | Summary operation when no `.planning/` directory exists | +### Estimate errors + +| Code | When emitted | +|------|-------------| +| `estimate_phases_unreadable` | `estimate-calibrate` when `.planning/phases/` exists but could not be read (EACCES/EIO) — refused rather than silently rebuilding calibration from a phantom empty sample set (#3882, ADR-3473 §8.5) | + ### Graphify errors | Code | When emitted | diff --git a/scripts/lint-phase-enumeration-drift.cjs b/scripts/lint-phase-enumeration-drift.cjs index 924d1dfa8..0c14308fe 100644 --- a/scripts/lint-phase-enumeration-drift.cjs +++ b/scripts/lint-phase-enumeration-drift.cjs @@ -62,11 +62,22 @@ * outstanding WORK wants the live set"): a LOOKUP, DIAGNOSTIC or ARCHIVAL * pass wants the physical set; only "which phases belong to this milestone" * wants the scoped set. - * - `src/roadmap.cts` `cmdRoadmapAnalyze`: builds `_phaseDirNames` as a - * heading->directory LOOKUP INDEX, not a milestone enumeration. It must - * see the PHYSICAL set so a heading already scoped by - * `extractCurrentMilestoneScoped` can find its directory; filtering it - * through the owner would scope the same set twice. + * - `src/roadmap.cts` `cmdRoadmapAnalyze` — MIGRATED (#3882, ADR-3473 §8.2): + * `_phaseDirNames`, a heading->directory LOOKUP INDEX (not a milestone + * enumeration; must see the PHYSICAL set so a heading already scoped by + * `extractCurrentMilestoneScoped` can find its directory), now calls + * `listAllPhaseDirs(phasesDir, { includeSentinels: true })` instead of a + * hand-rolled `readdirSync` — no longer exempted, since there is nothing + * left in this function for either detector to catch. + * - `src/init.cts` `cmdInitMilestoneOp`'s `diskPhaseDirs` — MIGRATED (#3882, + * ADR-3473 §8.2): the same heading->directory LOOKUP INDEX shape as + * `cmdRoadmapAnalyze`'s `_phaseDirNames` above, now calls + * `listAllPhaseDirs(phasesDir, { includeSentinels: true })`. Its sibling + * readdirSync (the no-ROADMAP-headings-found fallback) was already routed + * through `listMilestonePhaseDirs` before this phase and remains so — no + * longer exempted; the function's other former exemption reason + * (`detectHasPriorPhases`/`detectUiPhaseActive`, unrelated call sites in + * the same file) is unaffected. * - `src/verify.cts` `cmdValidateHealth`: a project-wide HEALTH-CHECK sweep * (config drift, phase-directory naming, duplicate-directory collisions, * unsummarized-plan detection) — same "sweep everything, report gaps" @@ -280,9 +291,8 @@ const OWNER_FILES = new Set([ // `lint-milestone-window-drift.cjs`'s FUNCTION_SCOPED_EXEMPTIONS mechanism. // See the header comment for the full written reason behind each entry. const FUNCTION_SCOPED_EXEMPTIONS = new Map([ - [path.join('src', 'roadmap.cts'), new Set(['cmdRoadmapAnalyze'])], [path.join('src', 'verify.cts'), new Set(['cmdValidateHealth', 'cmdVerifySchemaDrift'])], - [path.join('src', 'init.cts'), new Set(['detectHasPriorPhases', 'detectUiPhaseActive', 'cmdInitMilestoneOp'])], + [path.join('src', 'init.cts'), new Set(['detectHasPriorPhases', 'detectUiPhaseActive'])], [path.join('src', 'milestone.cts'), new Set(['archivePhaseDirectories', 'cmdMilestoneComplete', 'cmdPhasesClear'])], [path.join('src', 'phase.cts'), new Set(['cmdPhasesList', 'cmdPhaseNextDecimal', 'cmdPhasePlanIndex', 'cmdPhaseInsert', 'renameDecimalPhases', 'renameIntegerPhases'])], [path.join('src', 'audit.cts'), new Set(['listAuditPhaseTargets'])], diff --git a/src/estimate-cli.cts b/src/estimate-cli.cts index 3f3645c9e..aba05bc31 100644 --- a/src/estimate-cli.cts +++ b/src/estimate-cli.cts @@ -28,10 +28,16 @@ import estimation = require('./phase-estimation.cjs'); import planningWorkspace = require('./planning-workspace.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- config-loader.cjs is an export= CommonJS module import configLoader = require('./config-loader.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports -- phase-locator.cjs is an export= CommonJS module +import phaseLocator = require('./phase-locator.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-scope.cjs is an export= CommonJS module +import planningScopeMod = require('./planning-scope.cjs'); const { output, error, ERROR_REASON } = io; const { planningDir } = planningWorkspace; const { CONFIG_DEFAULTS } = configLoader; +const { listMilestonePhaseDirs } = phaseLocator; +const { SCOPE } = planningScopeMod; // WIN-1 parity (DEFECT.WINDOWS-FS-OPS): on Windows a concurrent reader, indexer, // or AV scanner can transiently hold the rename target open. Retry the transient @@ -195,6 +201,22 @@ export function cmdEstimateCheck(cwd: string, args: string[], raw: boolean): voi }, raw); } +/** + * Thrown by `collectCalibrationSamples` when the phases directory EXISTS but + * could not be read (EACCES/EIO, `scope: SCOPE.UNREADABLE`). #3882/ADR-3473 + * §8.5: that is a NON-answer, not "zero completed phases" — silently + * returning `[]` here would make an unreadable directory output-identical to + * a genuinely empty one, which is exactly the defect class §8.5 exists to + * end. `cmdEstimateCalibrate` catches this and refuses the rebuild instead + * of persisting a phantom empty calibration. + */ +export class PhasesUnreadableError extends Error { + constructor(public readonly phasesRoot: string) { + super(`phases directory exists but could not be read: ${phasesRoot}`); + this.name = 'PhasesUnreadableError'; + } +} + /** * Pair each completed phase's PLAN estimate with its SUMMARY actuals. * @@ -202,17 +224,25 @@ export function cmdEstimateCheck(cwd: string, args: string[], raw: boolean): voi * A plan with no `estimate` block, a summary with no `actuals`, or a malformed * value is skipped rather than guessed — a fabricated sample would silently * steer every future estimate. + * + * #3882 (ADR-3473 §8.2): this used to hand-roll a raw `readdirSync` over the + * phases directory, treating every directory (including sentinel phases — + * milestone 0 / 999, SENTINEL_RANGES) as a completed phase. A sentinel's + * PLAN/SUMMARY pair then silently contributed a phantom sample to the + * CALIBRATION FACTOR applied to every future estimate. Routed through the + * canonical owner (`listMilestonePhaseDirs`, `src/phase-locator.cts`) called + * with NO `cwd` — that combination is already "all milestone windows, + * sentinels excluded", exactly what this caller needs; no new API was + * required for this half (see the design doc's §3, corrected after + * measurement). It also inherits the `scope` discriminator for free, so an + * unreadable phases directory is no longer indistinguishable from a real + * empty one. */ export function collectCalibrationSamples(cwd: string): estimation.CalibrationSample[] { const phasesRoot = path.join(planningDir(cwd), 'phases'); - let phases: string[]; - try { - phases = fs.readdirSync(phasesRoot, { withFileTypes: true }) - .filter((d) => d.isDirectory()) - .map((d) => d.name) - .sort(); - } catch { - return []; + const { value: phases, scope } = listMilestonePhaseDirs(phasesRoot); + if (scope === SCOPE.UNREADABLE) { + throw new PhasesUnreadableError(phasesRoot); } const readBlock = (file: string, key: string): Record | null => { @@ -279,7 +309,19 @@ export function collectCalibrationSamples(cwd: string): estimation.CalibrationSa * next estimate reads the result. */ export function cmdEstimateCalibrate(cwd: string, _args: string[], raw: boolean): void { - const samples = collectCalibrationSamples(cwd); + let samples: estimation.CalibrationSample[]; + try { + samples = collectCalibrationSamples(cwd); + } catch (err) { + if (err instanceof PhasesUnreadableError) { + // #3882/ADR-3473 §8.5: refuse rather than silently rebuild the + // calibration document from a phantom empty sample set — an + // unreadable phases directory must never look output-identical to a + // project with genuinely zero completed phases. + error(err.message, ERROR_REASON.ESTIMATE_PHASES_UNREADABLE); + } + throw err; + } const calibration = estimation.computeCalibration(samples); const target = path.join(planningDir(cwd), CALIBRATION_FILENAME); diff --git a/src/init.cts b/src/init.cts index 993ce518b..2fecc20f8 100644 --- a/src/init.cts +++ b/src/init.cts @@ -84,7 +84,7 @@ const { harvestPriorVerifyCommands } = verifyCommandGrounding; const { output, error, ERROR_REASON } = io; const { loadConfig, loadConfigResolved } = configLoader; const { resolveModelInternal, resolveGranularityInternal, assertValidGranularityOverride } = modelResolver; -const { findPhaseInternal, listMilestonePhaseDirs } = phaseLocator; +const { findPhaseInternal, listMilestonePhaseDirs, listAllPhaseDirs } = phaseLocator; const { getRoadmapPhaseInternal, getMilestoneInfo, @@ -2184,17 +2184,22 @@ function cmdInitMilestoneOp(cwd: string, raw: boolean): void { const m = tok.match(/^(\d+)([A-Z]?(?:\.\d+)*)$/); return m ? String(parseInt(m[1], 10)) + m[2] : tok; }; + // #3882 (ADR-3473 §8.2): this used to hand-roll a readdirSync over the + // phases directory (a heading->directory LOOKUP INDEX, same role as + // cmdRoadmapAnalyze's `_phaseDirNames` — `roadmapPhaseNumbers` above is + // already scoped/sentinel-excluded, so this map must see the PHYSICAL set + // to resolve each heading's phase number to its actual directory name; + // scoping it again would look up inside an already-scoped set for no + // benefit). Routed through the named "physical set, sentinels included" + // axis instead: every `num` looked up below came from `roadmapPhaseNumbers` + // (sentinels already excluded there), so a sentinel entry surviving in + // this map is never read — inclusion is output-invariant, this only + // removes the re-derivation. const diskPhaseDirs = new Map(); - try { - const entries = fs.readdirSync(phasesDir, { withFileTypes: true }); - for (const e of entries) { - if (!e.isDirectory()) continue; - const m = stripProjectCodePrefix(e.name).match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})`)); - if (!m) continue; - diskPhaseDirs.set(canonicalizePhase(m[1]), e.name); - } - } catch { - /* intentionally empty */ + for (const name of listAllPhaseDirs(phasesDir, { includeSentinels: true }).value) { + const m = stripProjectCodePrefix(name).match(new RegExp(`^(${PHASE_NUMBER_TOKEN_SOURCE})`)); + if (!m) continue; + diskPhaseDirs.set(canonicalizePhase(m[1]), name); } if (roadmapPhaseNumbers.length > 0) { diff --git a/src/io.cts b/src/io.cts index f575fe883..bcc6c4227 100644 --- a/src/io.cts +++ b/src/io.cts @@ -201,6 +201,10 @@ const ERROR_REASON = Object.freeze({ // graphify GRAPHIFY_NO_GRAPH: 'graphify_no_graph', GRAPHIFY_INVALID_QUERY: 'graphify_invalid_query', + // estimate-calibrate (#3882, ADR-3473 §8.2): the phases directory exists + // but could not be read — a NON-answer, distinct from a project that + // genuinely has zero completed phases yet. + ESTIMATE_PHASES_UNREADABLE: 'estimate_phases_unreadable', // hooks HOOKS_OPT_OUT: 'hooks_opt_out', // commit-docs-guard (#3588) diff --git a/src/phase-locator.cts b/src/phase-locator.cts index c83483ca8..dcc1c3c56 100644 --- a/src/phase-locator.cts +++ b/src/phase-locator.cts @@ -409,6 +409,55 @@ function listMilestonePhaseDirs( return { value, scope }; } +/** + * #3882 (ADR-3473 §8.2, issue #3882): the single owner of "the PHYSICAL set + * of phase directories on disk, entirely un-windowed" — the OTHER axis + * `listMilestonePhaseDirs` above deliberately does not offer. That owner + * refuses sentinels UNCONDITIONALLY (see its own doc comment); it has no way + * to say "physical set, sentinels included". That is exactly what an + * archival, lookup-index, or health-sweep caller needs — e.g. a heading -> + * directory lookup index that must resolve a directory regardless of + * milestone window (`cmdRoadmapAnalyze`'s `_phaseDirNames`, + * `cmdInitMilestoneOp`'s `diskPhaseDirs`) — which is why those callers used + * to hand-roll a `readdirSync` instead of calling either owner. + * + * `includeSentinels` is REQUIRED, with no default value. #3882/ADR-3473 §8.2: + * "a caller that wants sentinels asks for them explicitly" — obtaining + * sentinel-inclusion by silent omission is exactly the defect class this + * axis exists to close, so the call site is refused at COMPILE TIME without + * it, not merely documented against it here. + * + * Mirrors `listMilestonePhaseDirs`'s own absent/unreadable handling + * (ADR-3180 Decision 2): an ABSENT `phasesDir` is a real empty (a project + * with no phase directories yet), `scope: SCOPE.COMPLETE`; a `phasesDir` + * that EXISTS but cannot be read is a NON-answer, `scope: SCOPE.UNREADABLE` + * — a caller must not treat that empty list as "this project has no + * phases." + */ +function listAllPhaseDirs( + phasesDir: string, + opts: { includeSentinels: boolean; phaseIdConvention?: string | null }, +): { value: string[]; scope: Scope } { + const { includeSentinels, phaseIdConvention = null } = opts; + + if (!fs.existsSync(phasesDir)) return { value: [], scope: SCOPE.COMPLETE }; + + let names: string[]; + try { + names = fs.readdirSync(phasesDir, { withFileTypes: true }) + .filter((e) => e.isDirectory()) + .map((e) => e.name); + } catch { + return { value: [], scope: SCOPE.UNREADABLE }; + } + + const value = names + .filter((name) => includeSentinels || !isSentinelPhaseId(name, phaseIdConvention ?? undefined)) + .sort((a, b) => comparePhaseNum(a, b)); + + return { value, scope: SCOPE.COMPLETE }; +} + function getArchivedPhaseDirs(cwd: string): ArchivedPhaseDir[] { // #2855: same workstream-scoped resolution as findPhaseInternal above, via // the shared listArchiveVersionDirs helper. `phase.list --include-archived` @@ -437,4 +486,5 @@ export = { findPhaseInternal, getArchivedPhaseDirs, listMilestonePhaseDirs, + listAllPhaseDirs, }; diff --git a/src/planning-snapshot.cts b/src/planning-snapshot.cts index 66c52e780..1de9c8155 100644 --- a/src/planning-snapshot.cts +++ b/src/planning-snapshot.cts @@ -26,7 +26,7 @@ import roadmapParserMod = require('./roadmap-parser.cjs'); const { getMilestoneInfo, extractCurrentMilestone } = roadmapParserMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseLocatorMod = require('./phase-locator.cjs'); -const { listMilestonePhaseDirs } = phaseLocatorMod; +const { listMilestonePhaseDirs, listAllPhaseDirs } = phaseLocatorMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import verificationMod = require('./verification.cjs'); const { isPhaseComplete } = verificationMod; @@ -838,19 +838,25 @@ function buildPlanningRootFilesField(cwd: string): { value: string[]; scope: Sco * `phaseDirs` cannot). An absent `phases/` root is a real empty, not a * failure (mirrors `listMilestonePhaseDirs`'s own treatment); a present but * unreadable root degrades to `UNREADABLE` with an empty list. + * + * #3882 (ADR-3473 §8.3): delegates the actual disk scan to + * `listAllPhaseDirs(phasesDir, {includeSentinels: true})` — that function is + * the sole owner of "readdirSync the phases/ root, map to dir names, handle + * absent-vs-unreadable"; this field is one more consumer of that scan, not a + * second implementation of it. The two functions previously duplicated the + * same readdirSync + filter + map + absent/unreadable handling, which is + * exactly the defect class ADR-3473 §8.3 forbids. + * + * The RE-SORT below is deliberate, not leftover duplication: + * `listAllPhaseDirs` orders its `value` by `comparePhaseNum` (numeric phase + * order — its own documented contract), but `allPhaseDirNames`'s existing, + * externally-observable order is plain lexicographic `.sort()`, and W007's + * consumers depend on that order today. Re-sorting here preserves that + * contract without forking the underlying scan. */ function buildAllPhaseDirNamesField(phasesDir: string): { value: string[]; scope: Scope } { - if (!fs.existsSync(phasesDir)) return { value: [], scope: SCOPE.COMPLETE }; - try { - const value = fs - .readdirSync(phasesDir, { withFileTypes: true }) - .filter((e) => e.isDirectory()) - .map((e) => e.name) - .sort(); - return { value, scope: SCOPE.COMPLETE }; - } catch { - return { value: [], scope: SCOPE.UNREADABLE }; - } + const { value, scope } = listAllPhaseDirs(phasesDir, { includeSentinels: true }); + return { value: value.slice().sort(), scope }; } /** diff --git a/src/roadmap.cts b/src/roadmap.cts index ca4ae69ef..d34d9ca12 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -19,7 +19,7 @@ import phaseIdMod = require('./phase-id.cjs'); const { normalizePhaseName, phaseMarkdownRegexSource, matchPhaseDirs, stripProjectCodePrefix, OPTIONAL_PHASE_TAG_SOURCE, roadmapPhaseLookupSources, isSentinelPhaseId, scopeToPhase } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseLocatorMod = require('./phase-locator.cjs'); -const { findPhaseInternal, listMilestonePhaseDirs } = phaseLocatorMod; +const { findPhaseInternal, listMilestonePhaseDirs, listAllPhaseDirs } = phaseLocatorMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningScopeMod = require('./planning-scope.cjs'); const { SCOPE } = planningScopeMod; @@ -534,18 +534,17 @@ function cmdRoadmapAnalyze(cwd: string, raw: boolean): void { const phasesDir = planningPaths(cwd).phases; // Build phase directory lookup once (O(1) readdir instead of O(N) per phase) - // #3185 exemption (documented reason, not a file allowlist — ADR-3180 - // Decision 4a): this is a heading->directory LOOKUP INDEX, not a milestone - // enumeration. It must see the PHYSICAL set so a heading already scoped by - // extractCurrentMilestoneScoped above can find its directory; filtering it - // through listMilestonePhaseDirs would scope the same set twice. - const _phaseDirNames = (() => { - try { - return fs.readdirSync(phasesDir, { withFileTypes: true }) - .filter(e => e.isDirectory()) - .map(e => e.name); - } catch { return []; } - })(); + // #3185 exemption reason (ADR-3180 Decision 4a): this is a heading->directory + // LOOKUP INDEX, not a milestone enumeration. It must see the PHYSICAL set so + // a heading already scoped by extractCurrentMilestoneScoped above can find + // its directory; filtering it through listMilestonePhaseDirs would scope + // the same set twice. #3882 (ADR-3473 §8.2): routed through the named + // "physical set, sentinels included" axis instead of a hand-rolled + // readdirSync — every heading matched below already excludes sentinel + // phase numbers via isSentinelPhaseId before it ever consults this list + // (collectAnalyzePhases), so a sentinel directory's presence here is + // output-invariant; this only removes the re-derivation, not the reason. + const _phaseDirNames = listAllPhaseDirs(phasesDir, { includeSentinels: true }).value; // Scan the scoped milestone window for phase-detail headings and enrich each // with its on-disk status. Extracted into `collectAnalyzePhases` (#3165) so diff --git a/tests/estimate-calibrate.test.cjs b/tests/estimate-calibrate.test.cjs index 61139146e..fe37b95be 100644 --- a/tests/estimate-calibrate.test.cjs +++ b/tests/estimate-calibrate.test.cjs @@ -23,6 +23,11 @@ const path = require('node:path'); const { createTempProject, cleanup, runGsdTools } = require('./helpers.cjs'); const est = require('../gsd-core/bin/lib/phase-estimation.cjs'); +const estimateCli = require('../gsd-core/bin/lib/estimate-cli.cjs'); +// io.cjs owns error()/ERROR_REASON/JSON-error-mode — driven directly here so +// H1 below can assert a typed `reason`, mirroring tests/config-get-default.test.cjs's +// established in-process CLI-error-path pattern. +const io = require('../gsd-core/bin/lib/io.cjs'); /** Write a phase dir containing a PLAN with an estimate and a SUMMARY with actuals. */ function writePhase(tmpDir, phaseDir, { estTokens, actTokens, tasks = 3, commits = 4 }) { @@ -364,3 +369,252 @@ describe('multi-plan phases pair per plan, not per phase', () => { assert.equal(out.applied, true); }); }); + +// ─── sentinel phases must not skew calibration (#3882, ADR-3473 §8.2) ────── +// +// collectCalibrationSamples enumerates .planning/phases with a raw readdirSync +// and treats every directory as a completed phase — it never routes through +// the phase-directory owner and never applies isSentinelPhaseId, so a sentinel +// directory (milestone 0 or 999 — SENTINEL_RANGES in src/phase-id.cts) whose +// PLAN/SUMMARY pair carries an estimate/actuals block contributes a phantom +// calibration sample. +// +// Governing constraint (.gsd/phase/feat-3882-enumerations/50-test-matrix.md +// row A1): computeCalibration is MEDIAN-based, so a single outlier sample does +// not move a 3-sample factor at all — asserting "the factor is unchanged" against +// exactly one sentinel would pass on the broken code for the wrong reason. The +// real, measured damage is the MIN_CALIBRATION_SAMPLES threshold crossing +// (applied flips false -> true, confidence low -> med on phantom evidence) and, +// once two sentinels are present, actual factor corruption. Every assertion +// below compares the WHOLE computed CalibrationResult object between a +// sentinel-free project and its sentinel-injected twin, reached the same way +// production reaches it (collectCalibrationSamples -> computeCalibration, the +// exact pair cmdEstimateCalibrate calls). +describe('sentinel phases must not skew calibration (#3882)', () => { + // Two genuine phases with DIFFERING ratios (1x and 2x), so A3 can pin each + // one's own unchanged value rather than two indistinguishable duplicates. + const REAL_PHASES = [ + ['01-alpha', 1000, 1000], + ['02-beta', 2000, 4000], + ]; + // A deliberately extreme, fabricated ratio (50x) — the shape of what a + // sentinel's PLAN/SUMMARY pair would carry. + const SENTINEL_SAMPLE = { estTokens: 1000, actTokens: 50000 }; + + function buildRealOnlyProject() { + const tmpDir = createTempProject(); + for (const [dir, estTokens, actTokens] of REAL_PHASES) { + writePhase(tmpDir, dir, { estTokens, actTokens }); + } + return tmpDir; + } + + /** Reached the way production reaches it — see cmdEstimateCalibrate above. */ + function calibrationResultFor(tmpDir) { + return est.computeCalibration(estimateCli.collectCalibrationSamples(tmpDir)); + } + + test('A1a: sentinelPhaseDoesNotActivateCalibration', (t) => { + const realOnly = buildRealOnlyProject(); + t.after(() => cleanup(realOnly)); + const withSentinel = buildRealOnlyProject(); + t.after(() => cleanup(withSentinel)); + // milestone 999 — reserved icebox sentinel range (SENTINEL_RANGES). + writePhase(withSentinel, '999-icebox', SENTINEL_SAMPLE); + + const realOnlyResult = calibrationResultFor(realOnly); + const withSentinelResult = calibrationResultFor(withSentinel); + + assert.deepEqual( + withSentinelResult, realOnlyResult, + 'a single sentinel phase must not change the computed calibration at all — ' + + `real-only=${JSON.stringify(realOnlyResult)} with-sentinel=${JSON.stringify(withSentinelResult)}`, + ); + }); + + test('A1b: sentinelPhasesDoNotCorruptTheFactor', (t) => { + const realOnly = buildRealOnlyProject(); + t.after(() => cleanup(realOnly)); + const withTwoSentinels = buildRealOnlyProject(); + t.after(() => cleanup(withTwoSentinels)); + // milestone 999 and milestone 0 — both reserved sentinel ranges. + writePhase(withTwoSentinels, '999-icebox', SENTINEL_SAMPLE); + writePhase(withTwoSentinels, '0-backlog', SENTINEL_SAMPLE); + + const realOnlyResult = calibrationResultFor(realOnly); + const withSentinelsResult = calibrationResultFor(withTwoSentinels); + + assert.deepEqual( + withSentinelsResult, realOnlyResult, + 'sentinel phases must not corrupt the calibration factor — ' + + `real-only=${JSON.stringify(realOnlyResult)} with-sentinels=${JSON.stringify(withSentinelsResult)}`, + ); + }); + + test('A2: sentinelPhaseContributesNoCalibrationSample', (t) => { + const withTwoSentinels = buildRealOnlyProject(); + t.after(() => cleanup(withTwoSentinels)); + writePhase(withTwoSentinels, '999-icebox', SENTINEL_SAMPLE); + writePhase(withTwoSentinels, '0-backlog', SENTINEL_SAMPLE); + + const samples = estimateCli.collectCalibrationSamples(withTwoSentinels); + const sentinelHits = samples.filter( + (s) => s.estimateTokens === SENTINEL_SAMPLE.estTokens && s.actualTokens === SENTINEL_SAMPLE.actTokens, + ); + assert.equal( + sentinelHits.length, 0, + `the sentinel phases' sample must be absent from the returned list; got ${JSON.stringify(samples)}`, + ); + }); + + test('A3: realPhasesStillContribute', (t) => { + const withTwoSentinels = buildRealOnlyProject(); + t.after(() => cleanup(withTwoSentinels)); + writePhase(withTwoSentinels, '999-icebox', SENTINEL_SAMPLE); + writePhase(withTwoSentinels, '0-backlog', SENTINEL_SAMPLE); + + const samples = estimateCli.collectCalibrationSamples(withTwoSentinels); + const realSamples = samples.filter( + (s) => !(s.estimateTokens === SENTINEL_SAMPLE.estTokens && s.actualTokens === SENTINEL_SAMPLE.actTokens), + ); + assert.deepEqual( + realSamples.sort((a, b) => a.estimateTokens - b.estimateTokens), + [ + { estimateTokens: 1000, actualTokens: 1000 }, + { estimateTokens: 2000, actualTokens: 4000 }, + ], + 'the two genuine phases must still contribute their own, unchanged samples', + ); + }); +}); + +// ─── PhasesUnreadableError / estimate_phases_unreadable (#3882, ADR-3473 §8.5, +// review finding #2) ───────────────────────────────────────────────────── +// +// `collectCalibrationSamples` now routes through `listMilestonePhaseDirs` +// (the #3882 fix above), which means an unreadable phases directory is no +// longer output-identical to a genuinely empty one — it surfaces as +// `scope: SCOPE.UNREADABLE`. `cmdEstimateCalibrate` converts that into a +// refusal (`PhasesUnreadableError`, `process.exit(1)`, +// `ERROR_REASON.ESTIMATE_PHASES_UNREADABLE`) instead of silently persisting +// a phantom empty calibration document — a real behavior change (previously +// silent-empty), disclosed and tested here rather than left as an +// undocumented side effect of the routing fix. +// +// H1 drives the real `cmdEstimateCalibrate` IN-PROCESS: `runGsdTools` spawns +// a real subprocess, and neither `fs.readdirSync` monkeypatching nor +// `process.exit` interception crosses that process boundary. The pattern +// below is the two established repo idioms composed, not invented: the +// process.exit-interception + `--json-errors`-mode capture from +// tests/config-get-default.test.cjs, and the `fs.readdirSync` method +// monkeypatch (never chmod, which root/CI bypasses — CLAUDE.md's +// cross-platform IO-failure-injection rule) already used in +// tests/phase-locator.test.cjs for `listAllPhaseDirs`'s own unreadable case. +describe('unreadable phases directory refuses calibration (#3882, ADR-3473 §8.5)', () => { + class _ExitSignal extends Error { + constructor(code, message) { + super(message ?? `process.exit(${code})`); + this.code = code; + } + } + + /** Runs cmdEstimateCalibrate in-process with process.exit + stderr(fd 2) captured. */ + function runCalibrateExpectError(tmpDir) { + const origExit = process.exit; + const origWriteSync = fs.writeSync; + io.setJsonErrorMode(true); + let exitCount = 0; + let exitCode; + let stderr = ''; + fs.writeSync = (fd, ...rest) => { + if (fd !== 2) return origWriteSync.call(fs, fd, ...rest); + const [data, offset = 0, length] = rest; + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset, offset + (length ?? data.length - offset)).toString('utf8') + : String(data); + stderr += chunk; + return Buffer.byteLength(chunk); + }; + const lastError = () => { + const parts = stderr.split('\n').filter(Boolean); + try { return JSON.parse(parts[parts.length - 1]); } catch { return {}; } + }; + process.exit = (code) => { + exitCount++; + exitCode = code; + throw new _ExitSignal(code, lastError().message); + }; + try { + estimateCli.cmdEstimateCalibrate(tmpDir, [], false); + } catch (e) { + if (!(e instanceof _ExitSignal)) throw e; + } finally { + process.exit = origExit; + fs.writeSync = origWriteSync; + io.setJsonErrorMode(false); + } + assert.ok(exitCode !== 0 && exitCode !== undefined, 'expected a non-zero exit code'); + assert.equal(exitCount, 1, 'error() must fire exactly once (production process.exit terminates)'); + return { status: exitCode, ...lastError() }; + } + + test('H1: unreadable phases directory exits non-zero with estimate_phases_unreadable', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + + const originalReaddirSync = fs.readdirSync; + fs.readdirSync = (...args) => { + if (args[0] === phasesDir) { + const err = new Error('EACCES: permission denied, scandir'); + err.code = 'EACCES'; + throw err; + } + return originalReaddirSync.apply(fs, args); + }; + let result; + try { + result = runCalibrateExpectError(tmpDir); + } finally { + fs.readdirSync = originalReaddirSync; + } + + assert.equal(result.status, 1); + assert.equal(result.reason, io.ERROR_REASON.ESTIMATE_PHASES_UNREADABLE); + assert.ok( + !fs.existsSync(path.join(tmpDir, '.planning', 'estimation-calibration.json')), + 'refusing the rebuild must not persist a phantom empty calibration document', + ); + }); + + test('H2: a genuinely-empty phases directory still succeeds with empty calibration (boundary the H1 guard must not cross)', (t) => { + // Real CLI as a subprocess (runGsdTools) — this is the boundary case: + // .planning/phases/ EXISTS and is READABLE, but has zero entries. Must + // succeed, not be caught by the unreadable-directory refusal above. + // Overlaps the pre-existing "no phases at all is a clean no-op" test + // above; kept as its own named row because it pins THIS boundary + // specifically (see 50-test-matrix.md row H2), not incidentally. + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + + const r = runGsdTools('query estimate-calibrate', tmpDir); + assert.ok(r.success, `a readable, genuinely-empty phases dir must succeed: ${r.error}`); + const out = JSON.parse(r.output); + assert.equal(out.sample_count, 0); + assert.equal(out.applied, false); + }); + + test('H3: a normal project with real phases is unaffected by the unreadable-dir guard', (t) => { + const tmpDir = createTempProject(); + t.after(() => cleanup(tmpDir)); + writePhase(tmpDir, '01-a', { estTokens: 100, actTokens: 200 }); + writePhase(tmpDir, '02-b', { estTokens: 100, actTokens: 200 }); + writePhase(tmpDir, '03-c', { estTokens: 100, actTokens: 200 }); + + const r = runGsdTools('query estimate-calibrate', tmpDir); + assert.ok(r.success, `a normal readable project must not be affected by the guard: ${r.error}`); + const out = JSON.parse(r.output); + assert.equal(out.sample_count, 3); + assert.equal(out.applied, true); + }); +}); diff --git a/tests/phase-locator.test.cjs b/tests/phase-locator.test.cjs index 4dc42dec0..ecabaa228 100644 --- a/tests/phase-locator.test.cjs +++ b/tests/phase-locator.test.cjs @@ -27,8 +27,9 @@ const fc = require('./helpers/fast-check-setup.cjs'); const phaseLocator = require('../gsd-core/bin/lib/phase-locator.cjs'); const planDependencyGraph = require('../gsd-core/bin/lib/plan-dependency-graph.cjs'); const { - runGsdTools, createTempProject, cleanup, isolateWorkstreamEnv, restoreWorkstreamEnv, + runGsdTools, createTempProject, createTempDir, cleanup, isolateWorkstreamEnv, restoreWorkstreamEnv, } = require('./helpers.cjs'); +const driftGuard = require('../scripts/lint-phase-enumeration-drift.cjs'); // ─── findPhaseInternal — basic active-phase lookup ──────────────────────────── @@ -1416,3 +1417,415 @@ describe('#2855: getArchivedPhaseDirs does not leak cross-workstream archived ph }); } + +// ═══════════════════════════════════════════════════════════════════════════ +// #3882 (ADR-3473 §8.2) — listAllPhaseDirs, the parity guarantee on +// listMilestonePhaseDirs, sentinel-range boundaries, migrated-call-site +// invariance, and the lint-phase-enumeration-drift.cjs guard's fail-capable +// proof. `tests/estimate-calibrate.test.cjs` (rows A1a/A1b/A2/A3) covers the +// actual calibration defect this issue fixes; this section covers rows +// B1-B4, C1, D1-D3, E1/E2, F1-F3 from +// .gsd/phase/feat-3882-enumerations/50-test-matrix.md. +// +// Per that doc's §G test-hermeticity rule: every fixture here builds its own +// temp project via helpers.cjs and removes it through `cleanup()` — no raw +// `fs.rmSync` (`local/no-raw-rmsync-in-tests`), and no fixture keyed to a +// real repo path. +// ═══════════════════════════════════════════════════════════════════════════ + +/** Build `/.planning/phases/` for each of `names`. Returns the phases dir. */ +function build3882PhasesFixture(names) { + const tmp = createTempDir('gsd-3882-'); + const phasesDir = path.join(tmp, '.planning', 'phases'); + fs.mkdirSync(phasesDir, { recursive: true }); + for (const name of names) fs.mkdirSync(path.join(phasesDir, name)); + return { tmp, phasesDir }; +} + +// ─── B. The new API — both axes, stated ──────────────────────────────────── + +describe('listAllPhaseDirs (#3882, ADR-3473 §8.2)', () => { + test('B1: returns the physical set — every phase directory on disk, across milestone windows', (t) => { + const { tmp, phasesDir } = build3882PhasesFixture(['01-one', '02-two', '03A-suffix', '04.1-decimal']); + t.after(() => cleanup(tmp)); + + const result = phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: false }); + assert.deepEqual(result, { value: ['01-one', '02-two', '03A-suffix', '04.1-decimal'], scope: 'complete' }); + }); + + test('B2: includeSentinels cannot be obtained by omission — compile-time only, pin each explicit runtime value', (t) => { + // TypeScript refuses `listAllPhaseDirs(phasesDir, {})` and + // `listAllPhaseDirs(phasesDir)` at the BUILD step (`opts.includeSentinels` + // has no `?` and no default) — `npm run build:lib` / `npx tsc --noEmit` + // are the actual compile-time gate for that contract; there is no + // runtime shape for "omitted" to assert against here (the compiled + // `.cjs` has no type information left to inspect). This row instead pins + // the two EXPLICIT values so their divergence (the entire point of the + // axis) is asserted, not merely documented. + const { tmp, phasesDir } = build3882PhasesFixture(['01-real', '999-icebox']); + t.after(() => cleanup(tmp)); + + const withSentinels = phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: true }).value; + const withoutSentinels = phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: false }).value; + assert.notDeepEqual(withSentinels, withoutSentinels, + 'includeSentinels must change the returned set — a no-op boolean would defeat the whole axis'); + }); + + test('B3: includeSentinels:false — physical set minus sentinels, the combination collectCalibrationSamples needs', (t) => { + const { tmp, phasesDir } = build3882PhasesFixture(['01-real', '02-real', '0-backlog', '999-icebox']); + t.after(() => cleanup(tmp)); + + const result = phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: false }); + assert.deepEqual(result, { value: ['01-real', '02-real'], scope: 'complete' }); + }); + + test('B4: includeSentinels:true — the archival/lookup/diagnostic intent, sentinels kept, order asserted', (t) => { + const { tmp, phasesDir } = build3882PhasesFixture(['01-real', '02-real', '0-backlog', '999-icebox']); + t.after(() => cleanup(tmp)); + + const result = phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: true }); + // Order asserted directly (comparePhaseNum: 0 < 1 < 2 < 999) — a prior + // version of this row `.sort()`ed both sides, which discarded the order + // assertion `listAllPhaseDirs`'s own documented `comparePhaseNum` sort + // contract makes available. + assert.deepEqual(result, { value: ['0-backlog', '01-real', '02-real', '999-icebox'], scope: 'complete' }); + }); + + test('absent phases dir is a real empty (scope: complete), not a failure', (t) => { + const tmp = createTempDir('gsd-3882-'); + t.after(() => cleanup(tmp)); + const phasesDir = path.join(tmp, '.planning', 'phases'); // never created + + assert.deepEqual(phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: true }), { value: [], scope: 'complete' }); + assert.deepEqual(phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: false }), { value: [], scope: 'complete' }); + }); + + test('an existing-but-unreadable phases dir is scope: unreadable, not a silent empty (#8.5)', () => { + const tmp = createTempDir('gsd-3882-'); + const phasesDir = path.join(tmp, '.planning', 'phases'); + fs.mkdirSync(phasesDir, { recursive: true }); + + // #3882/CLAUDE.md IO-failure-injection rule: mode-bit tricks are + // unreliable under root/CI (root bypasses 0o000 with zero coverage) and, + // worse here, leave the directory in a state `cleanup()`'s recursive + // rmSync cannot always tear down deterministically. Inject the failure + // deterministically via method monkeypatching on `fs.readdirSync` + // instead — restored, and the real directory removed via `cleanup()`, + // in a single `finally` so no failure mode can leak a broken permission + // bit or a raw `rmSync` call into this test. + const originalReaddirSync = fs.readdirSync; + fs.readdirSync = (...args) => { + if (args[0] === phasesDir) { + const err = new Error('EACCES: permission denied, scandir'); + err.code = 'EACCES'; + throw err; + } + return originalReaddirSync.apply(fs, args); + }; + try { + const result = phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: true }); + assert.deepEqual(result, { value: [], scope: 'unreadable' }); + } finally { + fs.readdirSync = originalReaddirSync; + cleanup(tmp); + } + }); +}); + +// ─── C. Parity — listMilestonePhaseDirs is UNCHANGED (highest-risk row) ─── + +describe('listMilestonePhaseDirs output is unchanged (#3882 row C1)', () => { + test('C1: byte-identical to a verbatim expectation captured from origin/next\'s built lib', (t) => { + // Captured by building origin/next (832dcbb7513d0e00bfe31c072c48751bb16e88cf) + // in an isolated worktree (`npm ci` -> build:lib's own `prepare` script) + // and calling `listMilestonePhaseDirs(phasesDir)` (no `cwd`) against this + // EXACT fixture set, BEFORE any change in this diff — never re-derived + // from the new code (#3427 anti-fixture-trap discipline). + const EXPECTED_FROM_NEXT = { value: ['01-one', '02-two', '03A-suffix', '04.1-decimal'], scope: 'complete' }; + + const { tmp, phasesDir } = build3882PhasesFixture([ + '01-one', '02-two', '03A-suffix', '04.1-decimal', '0-backlog', '999-icebox', + ]); + t.after(() => cleanup(tmp)); + + const actual = phaseLocator.listMilestonePhaseDirs(phasesDir); + assert.deepEqual(actual, EXPECTED_FROM_NEXT, + `listMilestonePhaseDirs must be byte-identical to origin/next's behavior for the 16 callers depending on it; got ${JSON.stringify(actual)}`); + }); +}); + +// ─── D. Boundaries — the sentinel range, limit-1/limit/limit+1 ──────────── + +describe('sentinel-range boundaries, driven through the new API (#3882 rows D1-D3)', () => { + test('D1: lower edge — 0 is the sentinel; 1 (just above) is kept. (there is no "just below" 0 for a phase id)', (t) => { + const { tmp, phasesDir } = build3882PhasesFixture(['0-backlog', '01-real']); + t.after(() => cleanup(tmp)); + + const result = phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: false }); + assert.deepEqual(result.value, ['01-real']); + }); + + test('D2: upper edge — 998 (limit-1, kept), 999 (limit, sentinel, dropped), 1000 (limit+1, kept)', (t) => { + const { tmp, phasesDir } = build3882PhasesFixture(['998-real', '999-icebox', '1000-real']); + t.after(() => cleanup(tmp)); + + const result = phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: false }); + assert.deepEqual(result.value, ['998-real', '1000-real']); + }); + + test('D3: decimal/letter-suffix continuations at the sentinel-range edges — pin the ACTUAL (conservative) reading, not a re-derived one', (t) => { + // #1324's bracket-shaped continuations (e.g. a decimal sub-phase of 0 or + // 999, or a letter-suffixed one) are string-indistinguishable from a + // sentinel by a naive prefix test. isSentinelPhaseId's own documented + // residual ambiguity must not be silently resolved in passing here. + // + // Review fix: the prior version of this row derived `expectedKept` by + // calling isSentinelPhaseId itself, which can only fail if the call is + // deleted outright — a proxy for "the call happened", not for "the call + // returns the right thing". Verified by direct execution + // (`isSentinelPhaseId('0.1-decimal-of-backlog') === true`, + // `isSentinelPhaseId('999A-suffix-of-icebox') === true`, + // `isSentinelPhaseId('01-real') === false`) and hardcoded below. + // + // The prior fixture was ALSO trivial in a second way: both boundary-shaped + // entries land on the "dropped" side, so `01-real` — the one surviving + // entry — is not itself boundary-shaped, and the row could not catch a + // regression that started wrongly KEEPING a continuation. Two genuinely + // non-sentinel continuation-shaped dirs are added at the SAME edges + // (`1000.1-decimal-real` just above the upper sentinel bound, decimal + // continuation; `01A-real-suffix` a letter-suffixed ordinary phase) so + // the row asserts both directions: continuation-of-a-sentinel dropped, + // continuation-of-a-real-phase kept. + const { tmp, phasesDir } = build3882PhasesFixture([ + '0.1-decimal-of-backlog', '999A-suffix-of-icebox', '01-real', + '1000.1-decimal-real', '01A-real-suffix', + ]); + t.after(() => cleanup(tmp)); + + const withoutSentinels = phaseLocator.listAllPhaseDirs(phasesDir, { includeSentinels: false }).value; + assert.deepEqual( + withoutSentinels.slice().sort(), + ['01-real', '01A-real-suffix', '1000.1-decimal-real'].sort(), + ); + }); +}); + +// ─── E. Migrated call sites ───────────────────────────────────────────── + +describe('migrated exemptions behave identically (#3882 rows E1/E2)', () => { + /** Capture whatever a synchronous fn writes to `fd` via fs.writeSync, without touching the real fd. */ + function captureFdWrite(fd, fn) { + const orig = fs.writeSync; + let captured = Buffer.alloc(0); + fs.writeSync = (writeFd, ...rest) => { + if (writeFd !== fd) return orig.call(fs, writeFd, ...rest); + const [data, offset = 0, length] = rest; + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset, offset + (length ?? data.length - offset)) + : Buffer.from(String(data), 'utf8'); + captured = Buffer.concat([captured, chunk]); + return chunk.length; + }; + try { + fn(); + } finally { + fs.writeSync = orig; + } + return captured.toString('utf-8'); + } + + /** + * Run `subject(tmpDir, false)` in-process with `fs.readdirSync` forced to a + * FIXED order for the exact `phasesDir` path, and return its parsed JSON + * output. `runGsdTools` spawns a real subprocess, which neither + * `fs.readdirSync` monkeypatching nor stdout capture can cross — this + * drives the actual shipped command function directly instead, mirroring + * tests/config-get-default.test.cjs's established in-process CLI pattern. + */ + function runWithForcedReaddirOrder(tmpDir, phasesDir, order, subject) { + const originalReaddirSync = fs.readdirSync; + fs.readdirSync = (...args) => { + if (args[0] === phasesDir && args[1] && args[1].withFileTypes) { + return order.map((name) => ({ name, isDirectory: () => true, isFile: () => false })); + } + return originalReaddirSync.apply(fs, args); + }; + try { + return JSON.parse(captureFdWrite(1, () => subject(tmpDir, false))); + } finally { + fs.readdirSync = originalReaddirSync; + } + } + + /** Build a ROADMAP.md + two colliding phase-01 directories (only one carries a SUMMARY). */ + function buildCollidingPhaseFixture(prefix) { + const tmpDir = createTempProject(prefix); + fs.writeFileSync(path.join(tmpDir, '.planning', 'ROADMAP.md'), [ + '# ROADMAP', '', '## Milestone v1.0', '', '## Phase 01: Real Phase', '**Goal:** ship it', '', + ].join('\n')); + const phasesDir = path.join(tmpDir, '.planning', 'phases'); + const realDir = path.join(phasesDir, '01-real'); + const dupDir = path.join(phasesDir, '01-duplicate'); + fs.mkdirSync(realDir, { recursive: true }); + fs.mkdirSync(dupDir, { recursive: true }); + // Only '01-real' carries a matched PLAN/SUMMARY pair — the observable + // that reveals which of the two colliding directories each caller + // actually selected. countMatchedSummaries (core-utils.cts) only counts a + // SUMMARY that pairs with an existing PLAN, so both files are required. + fs.writeFileSync(path.join(realDir, '01-PLAN.md'), '---\nphase: 01-real\n---\nplan\n'); + fs.writeFileSync(path.join(realDir, '01-SUMMARY.md'), '---\nphase: 01-real\n---\ndone\n'); + return { tmpDir, phasesDir }; + } + + test('E1: colliding phase numbers (comparePhaseNum returns 0 for 01-real/01-duplicate) — cmdRoadmapAnalyze first-wins vs cmdInitMilestoneOp last-wins, SAME tie order, OPPOSITE selection', () => { + // #3882: both migrated call sites route through the SAME owner + // (listAllPhaseDirs) and the SAME comparePhaseNum sort, but the sort is + // STABLE and comparePhaseNum returns 0 for a genuinely colliding phase + // number, so real disks' unspecified readdirSync order decides the tie. + // Fault-inject (monkeypatch — never chmod, which root/CI bypasses) a + // FIXED order so the tie is deterministic, then assert each caller's own + // documented selection rule against it: `matches[0]` "TAKE" (phase-id.cts + // ~985 — cmdRoadmapAnalyze reads a phase ONCE per heading to decorate a + // row it already emits, first match wins) vs `diskPhaseDirs.set()` + // (src/init.cts ~2198 — later entries in iteration order overwrite + // earlier ones in the Map, so the LAST directory wins). Same input order, + // opposite winners — exactly the risk a raw-fs -> comparePhaseNum reorder + // creates and the one this row exists to catch. + const roadmap = require('../gsd-core/bin/lib/roadmap.cjs'); + const init = require('../gsd-core/bin/lib/init.cjs'); + const { tmpDir, phasesDir } = buildCollidingPhaseFixture('gsd-3882-e1-collide-'); + try { + // Forced order: '01-duplicate' (no summary) BEFORE '01-real' (has one). + const order = ['01-duplicate', '01-real']; + const analyzeOut = runWithForcedReaddirOrder(tmpDir, phasesDir, order, roadmap.cmdRoadmapAnalyze); + const initOut = runWithForcedReaddirOrder(tmpDir, phasesDir, order, init.cmdInitMilestoneOp); + + const analyzedPhase = analyzeOut.phases.find((p) => p.number === '01'); + assert.ok(analyzedPhase, 'phase 01 must resolve despite the collision'); + assert.equal(analyzedPhase.summary_count, 0, + 'cmdRoadmapAnalyze must select the FIRST directory in tie order (01-duplicate, no summary) — matches[0] first-wins'); + + assert.equal(initOut.completed_phases, 1, + 'cmdInitMilestoneOp must select the LAST directory in tie order (01-real, has a summary) — Map.set last-wins, ' + + 'the OPPOSITE selection from cmdRoadmapAnalyze given the identical input order'); + } finally { + cleanup(tmpDir); + } + }); + + test('E1a: reversing the forced tie order reverses BOTH callers\' selections — confirms the effect tracks the collision, not a fixed name preference', () => { + const roadmap = require('../gsd-core/bin/lib/roadmap.cjs'); + const init = require('../gsd-core/bin/lib/init.cjs'); + const { tmpDir, phasesDir } = buildCollidingPhaseFixture('gsd-3882-e1a-collide-'); + try { + // Reversed order: '01-real' (has summary) BEFORE '01-duplicate' (none). + const order = ['01-real', '01-duplicate']; + const analyzeOut = runWithForcedReaddirOrder(tmpDir, phasesDir, order, roadmap.cmdRoadmapAnalyze); + const initOut = runWithForcedReaddirOrder(tmpDir, phasesDir, order, init.cmdInitMilestoneOp); + + const analyzedPhase = analyzeOut.phases.find((p) => p.number === '01'); + assert.equal(analyzedPhase.summary_count, 1, + 'first-wins now selects 01-real (first in the reversed order) — has a summary'); + assert.equal(initOut.completed_phases, 0, + 'last-wins now selects 01-duplicate (last in the reversed order) — no summary'); + } finally { + cleanup(tmpDir); + } + }); + + test('E1b (regression pin, original row): cmdRoadmapAnalyze\'s heading->directory lookup is unaffected by a sentinel directory\'s presence', () => { + // #3882: migrated `_phaseDirNames` (src/roadmap.cts) from a hand-rolled + // readdirSync to listAllPhaseDirs({includeSentinels:true}). Its own + // written exemption reason establishes WHY sentinel-inclusion is + // output-invariant here: every heading is tested with isSentinelPhaseId + // BEFORE this list is ever consulted (collectAnalyzePhases), so a + // sentinel directory can never be the value looked up. Assert that + // invariance directly, mirroring A1a/A1b's differential shape. + const tmpDir = createTempProject('gsd-3882-e1-'); + try { + const roadmapPath = path.join(tmpDir, '.planning', 'ROADMAP.md'); + fs.writeFileSync(roadmapPath, [ + '# ROADMAP', + '', + '## Milestone v1.0', + '', + '## Phase 01: Real Phase', + '**Goal:** ship it', + '', + ].join('\n')); + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '01-real'), { recursive: true }); + + const before = JSON.parse(runGsdTools('query roadmap.analyze', tmpDir).output); + + fs.mkdirSync(path.join(tmpDir, '.planning', 'phases', '999-icebox'), { recursive: true }); + const after = JSON.parse(runGsdTools('query roadmap.analyze', tmpDir).output); + + assert.deepEqual(after.phases, before.phases, + 'adding a sentinel phase directory must not change the resolved phase list'); + assert.deepEqual(after.missing_details, before.missing_details); + } finally { + cleanup(tmpDir); + } + }); + + test('E2: unmigrated exemptions are still guarded by detector 1', () => { + // Retiring coverage for a call site that did NOT move would be a silent + // regression. Spot-check the exemptions this phase deliberately left in + // place (see the guard's own header comment for the written reasons). + const stillExempt = [ + [path.join('src', 'milestone.cts'), 'archivePhaseDirectories'], + [path.join('src', 'milestone.cts'), 'cmdPhasesClear'], + [path.join('src', 'verify.cts'), 'cmdValidateHealth'], + [path.join('src', 'verify.cts'), 'cmdVerifySchemaDrift'], + [path.join('src', 'init.cts'), 'detectHasPriorPhases'], + [path.join('src', 'init.cts'), 'detectUiPhaseActive'], + ]; + for (const [file, fn] of stillExempt) { + const exempt = driftGuard.FUNCTION_SCOPED_EXEMPTIONS.get(file); + assert.ok(exempt && exempt.has(fn), `${file} ${fn} must still carry a function-scoped exemption`); + } + // And the migrated ones must NOT still be listed (removing coverage + // silently would be one failure mode; leaving a stale, now-pointless + // exemption around would be a different but real one — this pins both). + const roadmapExempt = driftGuard.FUNCTION_SCOPED_EXEMPTIONS.get(path.join('src', 'roadmap.cts')); + assert.ok(!roadmapExempt || !roadmapExempt.has('cmdRoadmapAnalyze'), + 'cmdRoadmapAnalyze must no longer carry an exemption — it no longer hand-rolls a readdirSync'); + const initExempt = driftGuard.FUNCTION_SCOPED_EXEMPTIONS.get(path.join('src', 'init.cts')); + assert.ok(!initExempt || !initExempt.has('cmdInitMilestoneOp'), + 'cmdInitMilestoneOp must no longer carry an exemption — its diskPhaseDirs lookup no longer hand-rolls a readdirSync'); + }); +}); + +// ─── F. The guard — both detectors, proven fail-capable ────────────────── + +describe('lint-phase-enumeration-drift.cjs guard (#3882 rows F1-F3)', () => { + test('F1: detector 2 (sentinel literal) still fires on a bare 999 outside phase-id.cts', () => { + const planted = "if (phaseNum === 999) return true;\n"; + const violations = driftGuard.findPhaseEnumerationDrift(planted, path.join('src', 'not-an-owner.cts')); + assert.equal(violations.length, 1); + assert.equal(violations[0].line, 1); + }); + + test('F1b: detector 2 still fires on a bare SENTINEL_RANGES reference outside phase-id.cts', () => { + const planted = 'const x = SENTINEL_RANGES.includes(n);\n'; + const violations = driftGuard.findPhaseEnumerationDrift(planted, path.join('src', 'not-an-owner.cts')); + assert.equal(violations.length, 1); + }); + + test('F2: detector 1 (enumeration) still fires on a hand-rolled readdirSync outside the owner', () => { + const planted = "const x = fs.readdirSync(phasesDir, { withFileTypes: true });\n"; + const violations = driftGuard.findPhaseEnumerationDrift(planted, path.join('src', 'not-an-owner.cts')); + assert.equal(violations.length, 1); + }); + + test('F3: guardCanFail — both detectors are demonstrated failing above (not merely asserted passing), and the real tree is currently clean', () => { + const root = path.join(__dirname, '..'); + const violations = driftGuard.scanRepo(root); + assert.deepEqual(violations, [], 'the real tree must currently pass — F1/F1b/F2 above already proved the detectors CAN fail on a planted violation'); + }); + + test('the owner files themselves are exempt by construction', () => { + assert.ok(driftGuard.OWNER_FILES.has(path.join('src', 'phase-locator.cts'))); + assert.ok(driftGuard.OWNER_FILES.has(path.join('src', 'phase-id.cts'))); + }); +}); diff --git a/tests/planning-snapshot.test.cjs b/tests/planning-snapshot.test.cjs index 8740f89d6..8409b3839 100644 --- a/tests/planning-snapshot.test.cjs +++ b/tests/planning-snapshot.test.cjs @@ -1114,6 +1114,36 @@ describe('allPhaseDirNames field (Phase 11, #3309 — health-diagnostic-rules/ro const snap = buildPlanningSnapshot(cwd); assert.deepStrictEqual(snap.allPhaseDirNames, { value: [], scope: SCOPE.UNREADABLE }); }); + + test('#3882 §3.1: byte-identical to a HARDCODED literal, order INCLUDED — mixed sentinel/decimal/letter-suffix/colliding fixture', (t) => { + // 40-design.md §3.1 claimed this proof existed ("proven byte-identical — + // order included") before it did; the only existing assertion (above, + // `.slice().sort()`) is order-BLIND, so it can only ever pass regardless + // of what order buildAllPhaseDirNamesField returns. This row is what + // makes that claim true: order asserted, expectation hardcoded (never + // re-derived from listAllPhaseDirs or buildAllPhaseDirNamesField + // themselves), over a fixture mixing every axis the delegation could get + // wrong — sentinel (0-/999-), decimal (04.1-), letter-suffix (03A-), and + // a genuinely COLLIDING phase-number pair (01-real / 01-duplicate, both + // phase "01" — the shape where a raw-fs -> comparePhaseNum reorder could + // silently change output order). + const cwd = createTempDir('gsd-3882-apdn-order-'); + t.after(() => cleanup(cwd)); + writeRoadmap(cwd, ['## v1.0 Current 🚧', '', '### Phase 1: Foo'].join('\n')); + const names = ['01-real', '01-duplicate', '999-icebox', '0-backlog', '04.1-decimal', '03A-suffix']; + for (const name of names) { + fs.mkdirSync(path.join(planningDirOf(cwd), 'phases', name), { recursive: true }); + } + + const snap = buildPlanningSnapshot(cwd); + // Hardcoded literal — plain JS default Array.prototype.sort() (UTF-16 + // code-unit order, a language guarantee) applied to the fixture list by + // hand, not by calling into the code under test. + assert.deepStrictEqual(snap.allPhaseDirNames, { + value: ['0-backlog', '01-duplicate', '01-real', '03A-suffix', '04.1-decimal', '999-icebox'], + scope: SCOPE.COMPLETE, + }); + }); }); // ═════════════════════════════════════════════════════════════════════════