enhance(#3882): stop sentinel phases skewing estimation calibration (#3893)

* 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 <sim@local>
2026-08-26 15:03:12 -04:00
parent 832dcbb751
commit a638ca4332
13 changed files with 880 additions and 54 deletions
--- 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<string, unknown> | 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);
--- 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<string, string>();
-  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) {
--- 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)
--- 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,
 };
--- 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 };
 }

 /**
--- 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