From a638ca4332f430d5abcaa316102ae173afc8c90e Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 26 Aug 2026 15:03:12 -0400 Subject: [PATCH] enhance(#3882): stop sentinel phases skewing estimation calibration (#3893) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#3882): failing-first rows for sentinel phases skewing calibration Adds A1a/A1b/A2/A3 to tests/estimate-calibrate.test.cjs, the module's existing test file, rather than a new bug-NNNN file. collectCalibrationSamples (src/estimate-cli.cts:206) does a raw readdirSync over .planning/phases and never applies isSentinelPhaseId, so a sentinel phase (milestone 0 or 999) carrying a PLAN estimate / SUMMARY actuals pair contributes a phantom calibration sample. computeCalibration is median-based, so a single 50x outlier among three samples leaves the factor unmoved — asserting "the factor is unchanged" against one sentinel would pass on the broken code for the wrong reason. Each row instead asserts the WHOLE computed CalibrationResult object (factor, applied, confidence, sampleCount, clamped) for a sentinel-free project against its sentinel-injected twin: - A1a: one sentinel flips applied false->true and confidence low->med on phantom evidence (calibration switches on with zero real signal). - A1b: two sentinels corrupt the factor itself (1 -> 3, clamped false->true). - A2: the sentinel's own sample is verified absent from the returned list. - A3: the two genuine phases still contribute their own unchanged samples (regression pin — stops A1/A2 passing by filtering everything). Verified RED on today's code (node tests/estimate-calibrate.test.cjs): A1a/A1b/A2 fail with the exact differing objects; A3 and all pre-existing rows in the file remain green (no collateral). Refs #3882 * feat(#3882): route phase enumeration through its owner and name the sentinel axis Task 1: collectCalibrationSamples (src/estimate-cli.cts) hand-rolled a raw readdirSync over .planning/phases, treating every directory (including sentinel phases, milestone 0/999) as a completed phase and feeding phantom PLAN/SUMMARY samples into the estimation calibration factor. Routed through the existing owner, listMilestonePhaseDirs(phasesRoot) with no cwd -- already 'all milestones, sentinels excluded', exactly the combination this caller needs; no new API was required for this half. It now also surfaces the scope discriminator: an unreadable phases directory throws PhasesUnreadableError instead of silently returning zero samples, and cmdEstimateCalibrate reports it via a new ERROR_REASON.ESTIMATE_PHASES_UNREADABLE instead of persisting a phantom empty calibration document. Task 2: added listAllPhaseDirs(phasesDir, { includeSentinels }) to src/phase-locator.cts -- the one genuinely missing axis: 'physical set, sentinels INCLUDED'. includeSentinels has no default and is required, so a call site cannot obtain sentinel-inclusion by omission (compile-time refusal, not just documentation). Mirrors listMilestonePhaseDirs's absent/unreadable scope handling. Task 3: migrated the two exemptions whose written reason maps cleanly onto 'physical set, sentinels included' -- cmdRoadmapAnalyze's _phaseDirNames (src/roadmap.cts) and cmdInitMilestoneOp's diskPhaseDirs (src/init.cts), both heading->directory lookup indexes. Left the rest: archivePhaseDirectories's own body has no readdirSync to migrate (its callers already resolve dirs before calling it, and both current callers deliberately EXCLUDE sentinels -- migrating it would be an unauthorized behavior change, not an API swap); cmdValidateHealth's exemption is vestigial (its actual physical-set sweep already lives in planning-snapshot.cts's buildAllPhaseDirNamesField, a pre-existing near-duplicate of the new axis, flagged as a finding, not restructured); cmdPhasesClear/cmdMilestoneComplete/cmdVerifySchemaDrift/detectHasPriorPhases/detectUiPhaseActive want a different combination (sentinels excluded, or a single-phase lookup) and are unaffected. Task 4: detector 2 (sentinel literal) is untouched and retained. Removed exemption entries only for the two migrated call sites; every other function-scoped exemption is preserved. Guard exits 0. Refs #3882 * refactor(#3882): delegate the snapshot phase-dir scan to its owner buildAllPhaseDirNamesField duplicated listAllPhaseDirs's own readdirSync + directory-filter + absent/unreadable handling — the 'one implementation per rule' defect ADR-3473 SS8.3 names, introduced by this branch's own #3882 work. Delegate to listAllPhaseDirs and re-apply the field's existing lexicographic sort on top, since W007's observable order must not change. Refs #3882 * docs(#3882): document the sentinel axis and the enumeration consolidation Records listAllPhaseDirs in the Phase Locator glossary entry, and the fact that the owner already answers the all-milestones sentinel-free question when called without a cwd -- the call collectCalibrationSamples was missing. Also notes that buildAllPhaseDirNamesField now delegates rather than carrying a second readdir, and that exactly one readdirSync over the phases directory remains across the two modules. Refs #3882 * test(#3882): close review findings — real order proof, unreadable coverage, collision fixtures Refs #3882 * chore(#3882): backfill changeset PR number Refs #3882 --------- Co-authored-by: sim --- .changeset/lively-mice-wave.md | 5 + CONTEXT.md | 4 +- docs/json-errors.md | 6 + scripts/lint-phase-enumeration-drift.cjs | 24 +- src/estimate-cli.cts | 60 +++- src/init.cts | 27 +- src/io.cts | 4 + src/phase-locator.cts | 50 +++ src/planning-snapshot.cts | 30 +- src/roadmap.cts | 25 +- tests/estimate-calibrate.test.cjs | 254 ++++++++++++++ tests/phase-locator.test.cjs | 415 ++++++++++++++++++++++- tests/planning-snapshot.test.cjs | 30 ++ 13 files changed, 880 insertions(+), 54 deletions(-) create mode 100644 .changeset/lively-mice-wave.md 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, + }); + }); }); // ═════════════════════════════════════════════════════════════════════════