From 77dcdda534c4a474bbd674e4963e84d287a38ba9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 2 Sep 2026 08:12:57 -0400 Subject: [PATCH] enhance(#4014): an unreadable directory must not report as an empty one (#4163) * test(#4014): add failing-first coverage for unreadable-vs-empty directory scope (epic #3473 B4) * fix(#4014): an unreadable directory must not report as an empty one (epic #3473 B4) * test(#4014): update hardcoded generateSlugInternal closing-brace line after import shift src/core-utils.cts's new #4014 import block shifted every subsequent line by 6, moving generateSlugInternal's real closing brace from line 193 to 199. tests/slug-derivation-drift-guard.test.cjs's MAJOR-1 fixture hardcodes that line number to plant a synthetic violation immediately after the function's real body; the guard script itself locates the boundary dynamically via brace-matching and needed no change. * docs(#4014): document the unreadable-directory scope signal and add changeset * docs(#4014): backfill changeset PR number to #4163 * test(#4014): kill pre-existing core-utils.cjs mutation-score gap, unrelated to this issue's diff --------- Co-authored-by: sim --- .changeset/lively-birds-click.md | 5 + docs/FEATURES.md | 47 ++++++ .../unreadable-directory-scope-signal.md | 47 ++++++ src/core-utils.cts | 46 ++++-- src/gap-checker.cts | 42 ++++-- src/init.cts | 37 +++++ src/planning-workspace.cts | 64 ++++++--- src/roadmap.cts | 57 ++++++-- tests/core-utils.test.cjs | 136 ++++++++++++++++++ tests/gap-checker.property.test.cjs | 131 +++++++++++++++++ tests/init.test.cjs | 15 ++ tests/planning-workspace.test.cjs | 110 ++++++++------ tests/roadmap.test.cjs | 47 ++++++ tests/slug-derivation-drift-guard.test.cjs | 2 +- 14 files changed, 693 insertions(+), 93 deletions(-) create mode 100644 .changeset/lively-birds-click.md create mode 100644 docs/features/unreadable-directory-scope-signal.md diff --git a/.changeset/lively-birds-click.md b/.changeset/lively-birds-click.md new file mode 100644 index 000000000..7c9ca3e79 --- /dev/null +++ b/.changeset/lively-birds-click.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 4163 +--- +**`roadmap analyze`, `gap-checker`, and `init`'s JSON output now distinguish an unreadable phase directory from a genuinely empty one** — a new `context_scope`/`phase_dir_scope` field (`'complete'` or `'unreadable'`) sits alongside the existing `has_context`/`context_read_error` fields, so a permission or I/O failure reading a phase directory is no longer indistinguishable from a phase that simply has no context file yet. `init manager`'s previously-silent read failure (a bare empty catch) now surfaces the same signal. (#4014) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 88eadc2a9..41d818277 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -206,6 +206,7 @@ - [gsd-tools Declares Outcomes, Pinned at v1](#3912-gsd-tools-declares-outcomes-pinned-at-v1) - [Reachable Lint Rules and a Non-Destructive Quick-Task Append](#3951-reachable-lint-rules-and-a-non-destructive-quick-task-append) - [Per-Task External-Tracker Content-Resolution Seam](#3970-per-task-external-tracker-content-resolution-seam) + - [Unreadable-Directory Scope Signal](#4014-unreadable-directory-scope-signal) --- @@ -4196,6 +4197,52 @@ for how `execute:task` differs from the twelve prose-dispatched points. --- +### 4014. Unreadable-Directory Scope Signal + +**Purpose:** ADR-3473 §8.4 ("failure is a value") applies to filesystem +listings, not only command argv. `#3885` (B5) gave `roadmap analyze`, +`gap-checker`, and `init`'s JSON bundles a `context_read_error` / +`phase_dir_read_error` string naming an unreadable phase directory — but the +underlying `has_context` / `hasContext` boolean stayed `false` either way, +so a consumer branching on that boolean alone still cannot tell "genuinely +no context file" from "could not read the directory at all." This closes +that gap with a typed signal, reusing ADR-3180's existing frozen `SCOPE` +enum rather than a new vocabulary. + +**`findContextMdIn` (`src/planning-workspace.cts`) now reports its own +scope.** Called with a directory path, it returns `{ file, files, scope }` +instead of a bare filename-or-null, and never throws — an unreadable +directory reports `scope: 'unreadable'` (previously it threw, forcing every +caller to hand-roll its own `try`/`catch`); a genuinely absent directory +(`ENOENT`) reports `scope: 'complete'`, the same "real empty" answer as +today. The array-input call form (an already-read listing) is unchanged. + +**Five downstream call sites gain an additive `scope` field, none renamed +or removed:** `roadmap analyze`'s `AnalyzePhase.context_scope`, +`gap-checker`'s `phase_dir_scope`, and `init`'s `context_scope` on all three +JSON bundles (`init plan-phase`, `init phase-op`, `init manager`) — +including `cmdInitManager`, whose own read failure previously vanished into +a bare empty `catch {}` with no signal of any kind. `getPhaseFileStats` +(`src/core-utils.cts`) — the shared listing owner behind `roadmap analyze` +and `init`'s `has_context` — no longer lets its own failed read get masked +by an unrelated, already-successful `scanPhasePlans` scope on the same +phase directory. + +**Known limits:** +- `context_read_error` / `phase_dir_read_error`'s message text is now a + fixed "Could not read phase directory ``" rather than embedding the + underlying OS errno text — `findContextMdIn`'s directory-string form + reports only the `SCOPE` discriminator, not the raw caught error. The + field's presence and type are unchanged; only its message detail is + coarser than before #4014. +- `init.cts`'s three call sites call `findContextMdIn` for the scope signal + and then still run their own, pre-existing `fs.readdirSync` on the same + path for the rest of their output — an intentional, additive-only choice + to avoid altering already-complex failure control-flow at those sites, + not a performance optimization. + +--- + _Generated by `scripts/gen-features.cjs` — add a fragment under `docs/features/` and run `--write`._ diff --git a/docs/features/unreadable-directory-scope-signal.md b/docs/features/unreadable-directory-scope-signal.md new file mode 100644 index 000000000..f6e232f96 --- /dev/null +++ b/docs/features/unreadable-directory-scope-signal.md @@ -0,0 +1,47 @@ +--- +id: 4014 +title: Unreadable-Directory Scope Signal +group: v1.7.0 Features +--- + +**Purpose:** ADR-3473 §8.4 ("failure is a value") applies to filesystem +listings, not only command argv. `#3885` (B5) gave `roadmap analyze`, +`gap-checker`, and `init`'s JSON bundles a `context_read_error` / +`phase_dir_read_error` string naming an unreadable phase directory — but the +underlying `has_context` / `hasContext` boolean stayed `false` either way, +so a consumer branching on that boolean alone still cannot tell "genuinely +no context file" from "could not read the directory at all." This closes +that gap with a typed signal, reusing ADR-3180's existing frozen `SCOPE` +enum rather than a new vocabulary. + +**`findContextMdIn` (`src/planning-workspace.cts`) now reports its own +scope.** Called with a directory path, it returns `{ file, files, scope }` +instead of a bare filename-or-null, and never throws — an unreadable +directory reports `scope: 'unreadable'` (previously it threw, forcing every +caller to hand-roll its own `try`/`catch`); a genuinely absent directory +(`ENOENT`) reports `scope: 'complete'`, the same "real empty" answer as +today. The array-input call form (an already-read listing) is unchanged. + +**Five downstream call sites gain an additive `scope` field, none renamed +or removed:** `roadmap analyze`'s `AnalyzePhase.context_scope`, +`gap-checker`'s `phase_dir_scope`, and `init`'s `context_scope` on all three +JSON bundles (`init plan-phase`, `init phase-op`, `init manager`) — +including `cmdInitManager`, whose own read failure previously vanished into +a bare empty `catch {}` with no signal of any kind. `getPhaseFileStats` +(`src/core-utils.cts`) — the shared listing owner behind `roadmap analyze` +and `init`'s `has_context` — no longer lets its own failed read get masked +by an unrelated, already-successful `scanPhasePlans` scope on the same +phase directory. + +**Known limits:** +- `context_read_error` / `phase_dir_read_error`'s message text is now a + fixed "Could not read phase directory ``" rather than embedding the + underlying OS errno text — `findContextMdIn`'s directory-string form + reports only the `SCOPE` discriminator, not the raw caught error. The + field's presence and type are unchanged; only its message detail is + coarser than before #4014. +- `init.cts`'s three call sites call `findContextMdIn` for the scope signal + and then still run their own, pre-existing `fs.readdirSync` on the same + path for the rest of their output — an intentional, additive-only choice + to avoid altering already-complex failure control-flow at those sites, + not a performance optimization. diff --git a/src/core-utils.cts b/src/core-utils.cts index f72f354cf..b261ec075 100644 --- a/src/core-utils.cts +++ b/src/core-utils.cts @@ -40,6 +40,12 @@ import phaseIdModule = require('./phase-id.cjs'); import planningWorkspace = require('./planning-workspace.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports import shellCommandProjection = require('./shell-command-projection.cjs'); +// planning-scope.cjs is a leaf module (no imports of its own — see its file +// header), so importing it here directly cannot introduce a cycle. +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; +type Scope = planningScopeMod.Scope; // ─── Line-ending normalization ───────────────────────────────────────────────── @@ -250,14 +256,14 @@ interface PhaseFileStats { hasContext: boolean; hasVerification: boolean; hasReviews: boolean; - scope: string; + scope: Scope; } // Minimal shape this module needs from plan-scan.cjs's scanPhasePlans result. interface PlanScanResultShape { planFiles: string[]; summaryFiles: string[]; - scope: string; + scope: Scope; } /** @@ -291,16 +297,40 @@ interface PlanScanResultShape { * Degrades on an unreadable directory instead of throwing: empty arrays, * every flag false, scope UNREADABLE (mirroring scanPhasePlans's own * degrade path). + * + * #4014 (epic #3473 B4-unreadable): this function's OWN readdirSync used to + * catch its own failure and then return `scope: scan.scope` — the scope of + * the UNRELATED, already-successful scanPhasePlans call — silently dropping + * that THIS read failed. The own-readdirSync is now routed through + * findContextMdIn's directory-string form (which never throws and reports + * its own SCOPE.UNREADABLE), and the two independently-scoped answers are + * combined into the worse of the two below — never letting a successful + * scan.scope mask this function's own failed read. + * + * The combination is written out as a two-value comparison rather than + * calling planning-snapshot.cts's `worstScope` directly: that module + * requires core-utils.cjs (for `findOrphanSummaries`) at its own top level, + * so importing it back here would create a tight, direct + * core-utils.cjs <-> planning-snapshot.cjs cycle — a materially different + * (and riskier) shape than this file's existing, documented lazy-access + * cyclic partners (planning-workspace.cjs, phase-id.cjs), which + * planning-snapshot.cts sits *above* in the dependency graph, not beside. + * `findContextMdIn`'s directory-string form can only ever report + * SCOPE.COMPLETE or SCOPE.UNREADABLE (a single readdirSync is binary — see + * its own doc comment), so per planning-snapshot.cts's SCOPE_SEVERITY + * ordering (UNREADABLE is maximal), `worstScope(scan.scope, ownScope)` is + * exactly `ownScope === UNREADABLE ? UNREADABLE : scan.scope` — the + * expression below is that identity, not a re-derivation of the ordering. */ function getPhaseFileStats(phaseDir: string): PhaseFileStats { // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-unsafe-assignment const scanPhasePlans: (dir: string) => PlanScanResultShape = require('./plan-scan.cjs'); const scan = scanPhasePlans(phaseDir); - let files: string[]; - try { - files = fs.readdirSync(phaseDir); - } catch { + const { files, scope: ownScope } = planningWorkspace.findContextMdIn(phaseDir); + const scope: Scope = ownScope === SCOPE.UNREADABLE ? SCOPE.UNREADABLE : scan.scope; + + if (ownScope === SCOPE.UNREADABLE) { return { plans: scan.planFiles, summaries: scan.summaryFiles, @@ -308,7 +338,7 @@ function getPhaseFileStats(phaseDir: string): PhaseFileStats { hasContext: false, hasVerification: false, hasReviews: false, - scope: scan.scope, + scope, }; } @@ -321,7 +351,7 @@ function getPhaseFileStats(phaseDir: string): PhaseFileStats { hasContext: planningWorkspace.findContextMdIn(scopedFiles) !== null, hasVerification: scopedFiles.some(f => f.endsWith('-VERIFICATION.md') || f === 'VERIFICATION.md'), hasReviews: scopedFiles.some(f => f.endsWith('-REVIEWS.md') || f === 'REVIEWS.md'), - scope: scan.scope, + scope, }; } diff --git a/src/gap-checker.cts b/src/gap-checker.cts index 8abf1406a..62c803ed0 100644 --- a/src/gap-checker.cts +++ b/src/gap-checker.cts @@ -33,6 +33,10 @@ const { scanPhasePlans } = planScanMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseIdMod = require('./phase-id.cjs'); const { scopeToPhase } = phaseIdMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; +type Scope = planningScopeMod.Scope; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -73,8 +77,16 @@ interface GapResult { * attempted) or was read successfully. A message naming the phase directory * when it EXISTS but `readdirSync` failed (EACCES/EIO/...) — never * collapsed to the same `[]` an absent directory produces. + * + * #4014 (epic #3473 B4): kept additive — `phase_dir_scope` below is the + * new, typed sibling signal; this field is now derived from it rather than + * owning its own readdirSync. */ phase_dir_read_error: string | null; + /** #4014 (epic #3473 B4): the SCOPE this phase dir's listing resolved to — + * SCOPE.UNREADABLE distinguishes a real read failure from a genuinely + * empty/absent phase dir (SCOPE.COMPLETE). */ + phase_dir_scope: Scope; } interface RunGapAnalysisOptions { @@ -347,6 +359,7 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp summary: 'workflow.post_planning_gaps disabled — skipping post-planning gap analysis', counts: { total: 0, covered: 0, uncovered: 0 }, phase_dir_read_error: null, + phase_dir_scope: SCOPE.COMPLETE, }; } @@ -372,17 +385,20 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp // Read the phase directory once; reuse the listing for both context detection // and plan-file enumeration (avoids redundant readdirSync calls). - let phaseDirFiles: string[] = []; - // #3885 (ADR-3473 §8.5): the existsSync guard above already means a catch - // here is NEVER "genuinely absent" (ENOENT) — this directory exists, so any - // failure to list it is a real read error (EACCES/EIO/...) and must be - // named, not folded into the same `[]` an absent directory produces. - let phaseDirReadError: string | null = null; - try { - if (fs.existsSync(absPhaseDir)) phaseDirFiles = fs.readdirSync(absPhaseDir); - } catch (err) { - phaseDirReadError = `Could not read phase directory ${formatDiagnosticToken(absPhaseDir)}: ${formatDiagnosticToken((err as Error)?.message ?? String(err))}`; - } + // + // #4014 (epic #3473 B4): findContextMdIn's directory-string form now owns + // the listing + ENOENT-vs-other discrimination, retiring the local + // existsSync-guarded readdirSync try/catch — ENOENT (genuinely absent) + // and a successful-but-empty read both resolve to SCOPE.COMPLETE with an + // empty listing, exactly like the existsSync guard's short-circuit did. + const { files: phaseDirFiles, scope: phaseDirScope } = findContextMdIn(absPhaseDir); + // #3885 (ADR-3473 §8.5): `phaseDirReadError` stays additive for the + // shipped `phase_dir_read_error` JSON field — derived from `phaseDirScope` + // rather than from its own caught error, since findContextMdIn's + // directory-string form reports SCOPE, not the raw errno message. + const phaseDirReadError = phaseDirScope === SCOPE.UNREADABLE + ? `Could not read phase directory ${formatDiagnosticToken(absPhaseDir)}` + : null; // #3511-class: scope the raw listing to this phase dir before the // phase-numbered -CONTEXT.md predicate. `phaseDirFiles` itself stays raw — @@ -450,6 +466,7 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp summary: coverageSummary + '; extracted 0 of N — possible format mismatch', counts: { total: rows.length, covered, uncovered }, phase_dir_read_error: phaseDirReadError, + phase_dir_scope: phaseDirScope, }; } return { @@ -459,6 +476,7 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp summary: 'extracted 0 of N — possible format mismatch', counts: { total: 0, covered: 0, uncovered: 0 }, phase_dir_read_error: phaseDirReadError, + phase_dir_scope: phaseDirScope, }; } @@ -477,6 +495,7 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp summary: 'no requirements or decisions to check', counts: { total: 0, covered: 0, uncovered: 0 }, phase_dir_read_error: phaseDirReadError, + phase_dir_scope: phaseDirScope, }; } @@ -498,6 +517,7 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp summary, counts: { total: rows.length, covered, uncovered }, phase_dir_read_error: phaseDirReadError, + phase_dir_scope: phaseDirScope, }; } diff --git a/src/init.cts b/src/init.cts index db9004c4a..e9f04835b 100644 --- a/src/init.cts +++ b/src/init.cts @@ -31,6 +31,10 @@ import phaseId = require('./phase-id.cjs'); import worktreeSafety = require('./worktree-safety.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-workspace.cjs is an export= CommonJS module import planningWorkspace = require('./planning-workspace.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; +type Scope = planningScopeMod.Scope; import { maskIfSecret } from './secrets.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- plan-scan.cjs is an export= CommonJS module import scanPhasePlans = require('./plan-scan.cjs'); @@ -1131,6 +1135,11 @@ function cmdInitPlanPhase( has_research: phaseInfo?.['has_research'] || false, has_context: phaseInfo?.['has_context'] || false, + // #4014 (epic #3473 B4-unreadable): additive scope signal adjacent to + // has_context — SCOPE.COMPLETE by default (no phase directory to read is + // a genuine, not-unreadable answer), overwritten below to whatever + // findContextMdIn(phaseDirFull) reports once a directory is known. + context_scope: SCOPE.COMPLETE, has_reviews: phaseInfo?.['has_reviews'] || false, has_plans: ((phaseInfo?.['plans'] as unknown[] | undefined)?.length || 0) > 0, plan_count: (phaseInfo?.['plans'] as unknown[] | undefined)?.length || 0, @@ -1149,6 +1158,13 @@ function cmdInitPlanPhase( if (phaseInfo?.['directory']) { const phaseDirFull = path.join(cwd, phaseInfo['directory'] as string); + // #4014 (epic #3473 B4-unreadable): findContextMdIn's directory-string + // form never throws, so this can record the real scope BEFORE the + // pre-existing `fs.readdirSync(phaseDirFull)` immediately below + // (unchanged) throws on the same unreadable directory and is caught + // exactly as before — additive only, the failure control-flow for + // context_path/research_path/etc. is untouched. + result['context_scope'] = findContextMdIn(phaseDirFull).scope; try { const files = fs.readdirSync(phaseDirFull); const phaseDirName = path.basename(phaseDirFull); @@ -2038,6 +2054,9 @@ function cmdInitPhaseOp(cwd: string, phase: string, raw: boolean): void { has_research: phaseInfo?.['has_research'] || false, has_context: phaseInfo?.['has_context'] || false, + // #4014 (epic #3473 B4-unreadable): see the parallel field in + // cmdInitPlanPhase — additive scope signal adjacent to has_context. + context_scope: SCOPE.COMPLETE, has_plans: ((phaseInfo?.['plans'] as unknown[] | undefined)?.length || 0) > 0, has_verification: phaseInfo?.['has_verification'] || false, has_reviews: phaseInfo?.['has_reviews'] || false, @@ -2055,6 +2074,9 @@ function cmdInitPhaseOp(cwd: string, phase: string, raw: boolean): void { if (phaseInfo?.['directory']) { const phaseDirFull = path.join(cwd, phaseInfo['directory'] as string); + // #4014 (epic #3473 B4-unreadable): see the parallel site in + // cmdInitPlanPhase — additive only, failure control-flow below unchanged. + result['context_scope'] = findContextMdIn(phaseDirFull).scope; try { const files = fs.readdirSync(phaseDirFull); const phaseDirName = path.basename(phaseDirFull); @@ -2414,6 +2436,9 @@ function cmdInitManager(cwd: string, raw: boolean): void { let hasResearch = false; let lastActivity: string | null = null; let isActive = false; + // #4014 (epic #3473 B4-unreadable): default COMPLETE — no directory at + // all (dirMatch not found) is a genuine, not-unreadable answer. + let contextScope: Scope = SCOPE.COMPLETE; let completion = buildPhaseCompletionProjection( cwd, phaseNum, @@ -2433,6 +2458,17 @@ function cmdInitManager(cwd: string, raw: boolean): void { if (dirMatch) { const fullDir = path.join(phasesDir, dirMatch); const phaseDirRel = toPosixPath(path.relative(cwd, fullDir)); + // #4014 (epic #3473 B4-unreadable): this whole block used to swallow + // ANY readdirSync failure below into the bare `catch { /* empty */ }` + // at the bottom — an unreadable phase directory reported the exact + // same `has_context: false` / `disk_status: 'no_directory'` as a + // directory that never existed. findContextMdIn's directory-string + // form never throws, so it can record the real scope (COMPLETE vs + // UNREADABLE) here, BEFORE the pre-existing `fs.readdirSync(fullDir)` + // immediately below (unchanged) throws on the exact same unreadable + // directory and is caught exactly as before — this line is additive + // only, the failure control-flow is untouched. + contextScope = findContextMdIn(fullDir).scope; const phaseFiles = fs.readdirSync(fullDir); planCount = listPhasePlanFiles(fullDir).length; summaryCount = listPhaseSummaryFiles(fullDir).length; @@ -2510,6 +2546,7 @@ function cmdInitManager(cwd: string, raw: boolean): void { ...completion, last_activity: lastActivity, is_active: isActive, + context_scope: contextScope, }); } diff --git a/src/planning-workspace.cts b/src/planning-workspace.cts index b2a0149f9..33fb125de 100644 --- a/src/planning-workspace.cts +++ b/src/planning-workspace.cts @@ -19,6 +19,10 @@ import { platformEnsureDir, retryRenameSync } from './shell-command-projection.c import { realClock } from './clock.cjs'; import type { Clock } from './clock.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; +type Scope = planningScopeMod.Scope; +// eslint-disable-next-line @typescript-eslint/no-require-imports import activeWorkstreamStore = require('./active-workstream-store.cjs'); const { createSharedPointerAdapter, @@ -569,32 +573,56 @@ function describeUnresolvedWorkstreamReason(reason: 'invalid_name' | 'missing_wo * form (`CONTEXT.md`) and the padded-prefix convention (`NN-CONTEXT.md`, * `NN.N-CONTEXT.md`, etc.) used by gsd-discuss-phase output. * - * Returns the filename (not the full path) of the first match, or null if - * no CONTEXT.md exists in the directory. - * * Canonical dual-form predicate extracted here to eliminate the 5-site * duplication that previously existed across init.cjs, roadmap.cjs, * core.cjs, gap-checker.cjs (#3739). * - * @param absDirOrFiles - Absolute path to the phase directory, - * OR an already-read files array (avoids a redundant readdirSync at call sites - * that already hold a directory listing). + * Two call shapes, two return shapes (#4014, epic #3473 B4-unreadable): + * + * - Array-input form (`files: string[]`, an already-read directory listing — + * avoids a redundant readdirSync at call sites that already hold one, and + * lets a caller pass an already phase-scoped listing): UNCHANGED — + * returns the matched filename or `null`, never throws (there is no I/O + * to fail on an in-memory array). + * - Directory-string form (`absDir: string`): performs the `readdirSync` + * itself and returns `{ file, files, scope }` — `file`/`files` are the + * match and the raw listing, `scope` is `SCOPE.COMPLETE` on a successful + * read (including ENOENT, which is a genuine "nothing there yet" answer, + * not a failure) or `SCOPE.UNREADABLE` on any other read error + * (EACCES/EIO/…). This form never throws — a caller that used to see an + * exception on an unreadable directory now sees `scope: SCOPE.UNREADABLE` + * instead, so an unreadable phase dir is reported distinctly from a + * genuinely empty one rather than being silently indistinguishable from + * it (#1883's original defect this closes at the source). */ -function findContextMdIn(absDirOrFiles: string | string[]): string | null { - try { - const files = Array.isArray(absDirOrFiles) - ? absDirOrFiles - : fs.readdirSync(absDirOrFiles); +function findContextMdIn(files: string[]): string | null; +function findContextMdIn(absDir: string): { file: string | null; files: string[]; scope: Scope }; +function findContextMdIn( + absDirOrFiles: string | string[], +): string | null | { file: string | null; files: string[]; scope: Scope } { + const matchIn = (files: string[]): string | null => { if (files.includes('CONTEXT.md')) return 'CONTEXT.md'; return files.find((f: string) => f.endsWith('-CONTEXT.md')) ?? null; + }; + + if (Array.isArray(absDirOrFiles)) { + return matchIn(absDirOrFiles); + } + + try { + const files = fs.readdirSync(absDirOrFiles); + return { file: matchIn(files), files, scope: SCOPE.COMPLETE }; } catch (err) { - // #1883: distinguish genuine absence from a permission/I-O failure. ENOENT - // ("nothing there") keeps the long-standing null contract the callers rely - // on; every other error (EACCES, EIO, …) is a real read failure that must - // propagate — otherwise an unreadable phase dir is silently reported as - // "no CONTEXT.md" and the discuss/plan gates wrongly skip context. - if ((err as NodeJS.ErrnoException).code === 'ENOENT') return null; - throw err; + // #1883 / #4014: distinguish genuine absence from a permission/I-O + // failure. ENOENT ("nothing there") keeps the long-standing "real empty" + // contract callers rely on; every other error (EACCES, EIO, …) is a real + // read failure — reported as SCOPE.UNREADABLE rather than thrown, so a + // caller no longer needs its own try/catch to keep an unreadable phase + // dir from being silently reported the same as "no CONTEXT.md". + if ((err as NodeJS.ErrnoException).code === 'ENOENT') { + return { file: null, files: [], scope: SCOPE.COMPLETE }; + } + return { file: null, files: [], scope: SCOPE.UNREADABLE }; } } diff --git a/src/roadmap.cts b/src/roadmap.cts index ae6206b69..afea0625e 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -65,8 +65,19 @@ interface PhasePlansAndSummaries { * readdirSync failed for any other reason (EACCES/EIO/...), so an * unreadable directory is never silently reported the same as one that was * successfully read and genuinely has no CONTEXT.md. + * + * #4014 (epic #3473 B4): kept — `AnalyzePhase.context_read_error` (the + * shipped, tested `roadmap analyze` JSON field this feeds) is an existing + * consumer, so this field stays additive rather than being retired. `scope` + * below is the new, typed sibling signal; this field is now derived from + * it rather than owning its own readdirSync. */ contextReadError: string | null; + /** #4014 (epic #3473 B4): the `SCOPE` this phase dir's listing resolved to + * — `SCOPE.UNREADABLE` distinguishes a real read failure from a + * genuinely empty/absent phase dir (`SCOPE.COMPLETE`), which + * `contextReadError`/`hasContext` alone cannot. */ + scope: Scope; } interface PhaseSearchResult { @@ -126,21 +137,20 @@ function countPhasePlansAndSummaries(phaseDir: string): PhasePlansAndSummaries { const { planCount, summaryCount } = scanPhasePlans(phaseDir); // hasContext and hasResearch are not plan-scan concerns — read the directory // once and share the listing for all non-plan metadata that cmdRoadmapAnalyze needs. - let phaseFiles: string[] = []; - // #3885 (ADR-3473 §8.5): distinguish "genuinely absent" (ENOENT) from - // "could not read" (EACCES/EIO/...) — the collapse of both to an empty - // listing is exactly the defect class this item closes. Mirrors - // core-utils.cts's getPhaseFileStats / phase-locator.cts's - // listMilestonePhaseDirs SCOPE.UNREADABLE discriminator. - let contextReadError: string | null = null; - try { - phaseFiles = fs.readdirSync(phaseDir); - } catch (err) { - const code = (err as NodeJS.ErrnoException)?.code; - if (code !== 'ENOENT') { - contextReadError = `Could not read phase directory ${formatDiagnosticToken(phaseDir)}: ${formatDiagnosticToken((err as Error)?.message ?? String(err))}`; - } - } + // + // #4014 (epic #3473 B4): the listing + unreadable-vs-empty discrimination + // is now owned by findContextMdIn's directory-string form, retiring this + // function's own readdirSync try/catch (mirrors core-utils.cts's + // getPhaseFileStats / phase-locator.cts's listMilestonePhaseDirs + // SCOPE.UNREADABLE discriminator). + const { files: phaseFiles, scope } = findContextMdIn(phaseDir); + // #3885 (ADR-3473 §8.5): `contextReadError` stays additive for the shipped + // `AnalyzePhase.context_read_error` JSON field — derived from `scope` + // rather than from its own caught error, since findContextMdIn's + // directory-string form reports SCOPE, not the raw errno message. + const contextReadError = scope === SCOPE.UNREADABLE + ? `Could not read phase directory ${formatDiagnosticToken(phaseDir)}` + : null; // #3511: scope the raw listing to this phase dir before the // phase-numbered-artifact predicates (hasContext/hasResearch) — planCount/ // summaryCount above stay on scanPhasePlans's own unscoped listing since a @@ -153,6 +163,7 @@ function countPhasePlansAndSummaries(phaseDir: string): PhasePlansAndSummaries { hasContext: findContextMdIn(scopedFiles) !== null, hasResearch: scopedFiles.some(f => f.endsWith('-RESEARCH.md') || f === 'RESEARCH.md'), contextReadError, + scope, }; } @@ -393,6 +404,11 @@ type AnalyzePhase = { roadmap_complete: boolean; /** #3885 (ADR-3473 §8.5): see PhasePlansAndSummaries.contextReadError. */ context_read_error: string | null; + /** #4014 (epic #3473 B4): see PhasePlansAndSummaries.scope. Additive + * sibling of context_read_error — SCOPE.UNREADABLE for the same read + * failure context_read_error names, SCOPE.COMPLETE otherwise (including a + * genuinely absent/no_directory phase). */ + context_scope: Scope; }; type AnalyzePhaseCollection = { @@ -496,6 +512,10 @@ function collectAnalyzePhases( // hit a non-ENOENT error — no directory at all is `disk_status: // 'no_directory'`, a real (if uninteresting) answer, not a read error. let contextReadError: string | null = null; + // #4014 (epic #3473 B4): additive sibling — SCOPE.COMPLETE by default + // (no directory at all is a genuine, not-unreadable answer), overwritten + // below only when dirMatch resolves. + let contextScope: Scope = SCOPE.COMPLETE; // DEAD catch removed (#2245 audit): matchPhaseDirs(...) is a pure // array lookup on an already-resolved string array, and @@ -528,6 +548,7 @@ function collectAnalyzePhases( hasContext = counts.hasContext; hasResearch = counts.hasResearch; contextReadError = counts.contextReadError; + contextScope = counts.scope; // ADR-3180 §7.4 (issue #3186, disk-strict, #3168 fix): route "is this // phase complete" through the canonical owner (`isPhaseComplete`), @@ -575,6 +596,7 @@ function collectAnalyzePhases( disk_status: diskStatus, roadmap_complete: roadmapComplete, context_read_error: contextReadError, + context_scope: contextScope, }); } @@ -595,6 +617,9 @@ function collectAnalyzePhases( let tHasContext = false; let tHasResearch = false; let tContextReadError: string | null = null; + // #4014 (epic #3473 B4): additive sibling, same default rule as the + // heading-declared branch above. + let tContextScope: Scope = SCOPE.COMPLETE; if (dirMatchA) { const counts = countPhasePlansAndSummaries(path.join(phasesDir, dirMatchA)); tPlanCount = counts.planCount; @@ -606,6 +631,7 @@ function collectAnalyzePhases( // existsSync (which cannot itself distinguish EACCES from absent), but // an unreadable phase directory is still surfaced via the sibling call. tContextReadError = counts.contextReadError; + tContextScope = counts.scope; } phases.push({ number: tr.id, @@ -620,6 +646,7 @@ function collectAnalyzePhases( disk_status: dirMatchA ? 'ok' : 'no_directory', roadmap_complete: false, context_read_error: tContextReadError, + context_scope: tContextScope, }); } return { phases, detailKeys }; diff --git a/tests/core-utils.test.cjs b/tests/core-utils.test.cjs index 2a257b696..8f08a7783 100644 --- a/tests/core-utils.test.cjs +++ b/tests/core-utils.test.cjs @@ -556,6 +556,55 @@ describe('getPhaseFileStats', () => { assert.strictEqual(stats.hasReviews, false); }); + // ─── #4014 (epic #3473 B4-unreadable) matrix rows 10-11 ─────────────────── + // + // getPhaseFileStats used to catch its OWN readdirSync failure and return + // `scope: scan.scope` — the scope of the UNRELATED, already-successful + // scanPhasePlans call — silently dropping that THIS function's own read + // failed. Both scanPhasePlans and getPhaseFileStats's own read call + // `fs.readdirSync(phaseDir)` on the identical path, in that order (scan + // first) — so the failure is injected on the SECOND call to that exact + // path only, letting scanPhasePlans succeed (scope complete) while + // getPhaseFileStats's own read fails, reproducing the exact swallowing bug + // named in the issue. No chmod 0o000 — root bypasses mode bits (silent + // zero coverage in root CI); restored via t.mock's auto-restore. + test('#4014 matrix row 10: own readdirSync failure is not masked by an already-successful scan.scope', (t) => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cu-test-')); + fs.writeFileSync(path.join(tmpDir, '01-PLAN.md'), '# Plan\n'); + const target = path.resolve(tmpDir); + const origReaddirSync = fs.readdirSync.bind(fs); + let callsOnTarget = 0; + t.mock.method(fs, 'readdirSync', (p, ...rest) => { + if (path.resolve(String(p)) === target) { + callsOnTarget += 1; + // 1st call = scanPhasePlans's own read: let it succeed (scope complete). + if (callsOnTarget === 1) return origReaddirSync(p, ...rest); + // 2nd call = getPhaseFileStats's own read (via findContextMdIn): fail it. + const err = new Error(`EACCES: simulated failure, scandir '${p}'`); + err.code = 'EACCES'; + throw err; + } + return origReaddirSync(p, ...rest); + }); + + const stats = coreUtils.getPhaseFileStats(tmpDir); + assert.strictEqual(stats.scope, SCOPE.UNREADABLE, + `own readdirSync failure must win over the unrelated already-successful scan.scope; got: ${stats.scope}`); + // #3183's pre-existing plans/summaries-untouched contract: unaffected + // by this function's own (unrelated) read outcome. + assert.deepEqual(stats.plans, ['01-PLAN.md']); + }); + + test('#4014 matrix row 11: both reads succeed — scope complete, flags computed as before (must not regress)', () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cu-test-')); + fs.writeFileSync(path.join(tmpDir, '01-PLAN.md'), '# Plan\n'); + fs.writeFileSync(path.join(tmpDir, 'CONTEXT.md'), '# context\n'); + const stats = coreUtils.getPhaseFileStats(tmpDir); + assert.strictEqual(stats.scope, SCOPE.COMPLETE); + assert.strictEqual(stats.hasContext, true); + assert.deepEqual(stats.plans, ['01-PLAN.md']); + }); + test('#3183 row 7 regression: nested plans/PLAN-01.md ONLY is reported (used to report 0)', () => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cu-test-')); fs.mkdirSync(path.join(tmpDir, 'plans')); @@ -777,6 +826,28 @@ describe('extractCanonicalPlanId', () => { assert.strictEqual(coreUtils.extractCanonicalPlanId('100-01-extra-slug-PLAN.md'), '100-01'); assert.strictEqual(coreUtils.extractCanonicalPlanId('01-02-PLAN.md'), '01-02'); }); + + // ─── #4014 mutation-gap: the three chained suffix strips are each anchored + // ($) to the true end of the filename. An unanchored variant would instead + // strip the FIRST mid-string occurrence of the pattern, corrupting a + // filename that merely CONTAINS one of these substrings earlier on. + + test('mutation-gap: mid-string ".md" is left alone — only the trailing .md is stripped', () => { + // "foo.mdx-01-PLAN.md": the trailing "-PLAN.md" is stripped by the first + // regex; the base "foo.mdx-01" does NOT end in ".md" (it ends in "mdx" + // before "-01"), so the third (bare .md$) regex must NOT touch the ".md" + // that happens to sit inside "foo.mdx". An unanchored /\.md/i would strip + // it there instead, corrupting the base to "foox-01". + assert.strictEqual(coreUtils.extractCanonicalPlanId('foo.mdx-01-PLAN.md'), 'foo.mdx-01'); + }); + + test('mutation-gap: mid-string "-SUMMARY.md" is left alone — only the trailing form is stripped', () => { + // "foo-SUMMARY.md-01-PLAN.md": trailing "-PLAN.md" is stripped by the + // first regex, leaving "foo-SUMMARY.md-01" — which does NOT end in + // "-SUMMARY.md". An unanchored /-SUMMARY\.md/i would still strip the + // mid-string occurrence, corrupting the base to "foo-01". + assert.strictEqual(coreUtils.extractCanonicalPlanId('foo-SUMMARY.md-01-PLAN.md'), 'foo-SUMMARY.md-01'); + }); }); // ─── countMatchedSummaries (#1988) ─────────────────────────────────────────── @@ -843,6 +914,71 @@ describe('countMatchedSummaries — stray non-plan summaries excluded (#1988)', }); }); +// ─── #4014 mutation-gap: summaryCandidates / findUnsummarizedPlans / +// findOrphanSummaries — same "Stryker core-utils shard runs only THIS file" +// reasoning as countMatchedSummaries above. These three share the private +// summaryCandidates helper but, unlike countMatchedSummaries, had no direct +// coverage inside core-utils.test.cjs itself (their existing coverage lives +// in plan-count-single-owner.test.cjs / summary-status-blocked-3345.test.cjs, +// neither of which the core-utils Stryker shard executes). + +describe('summaryCandidates / findUnsummarizedPlans / findOrphanSummaries — #4014 mutation-gap', () => { + const { countMatchedSummaries, findUnsummarizedPlans, findOrphanSummaries } = coreUtils; + + test('mutation-gap: mid-string ".md" in the plan filename does not corrupt the base', () => { + // "my.mdx-file-PLAN.md": summaryCandidates' own `base` strip is anchored + // (/\.md$/i) so only the true trailing ".md" is removed, leaving + // "my.mdx-file-PLAN" — the marker-swap candidate is then + // "my.mdx-file-SUMMARY.md". An unanchored strip would instead corrupt the + // base by stripping the ".md" that sits inside "my.mdx" first. + assert.strictEqual( + countMatchedSummaries(['my.mdx-file-PLAN.md'], ['my.mdx-file-SUMMARY.md']), + 1, + ); + assert.deepEqual(findUnsummarizedPlans(['my.mdx-file-PLAN.md'], ['my.mdx-file-SUMMARY.md']), []); + }); + + test('mutation-gap: legacy extended form -PLAN- matches ONLY via the --SUMMARY candidate', () => { + // "14-PLAN-01.md" also generates a marker-swap candidate + // ("14-SUMMARY-01.md") and a stem-suffix candidate + // ("14-PLAN-01-SUMMARY.md"), but the summary file used here + // ("14-01-SUMMARY.md") matches NEITHER of those — it can only match via + // the `extended` candidate on the summaryCandidates line that special- + // cases `^(\d+)-PLAN-(\d+)`. Isolates that candidate from the other two. + assert.strictEqual(countMatchedSummaries(['14-PLAN-01.md'], ['14-01-SUMMARY.md']), 1); + assert.deepEqual(findUnsummarizedPlans(['14-PLAN-01.md'], ['14-01-SUMMARY.md']), []); + }); + + test('mutation-gap: #3183 canonical-id candidate fires only when it differs from the PLAN-stripped stem', () => { + // "68-01-scaffolding-PLAN.md": extractCanonicalPlanId pairs "68"+"01" into + // "68-01", which differs from the plain PLAN-stripped stem + // "68-01-scaffolding" — so the `canonicalId !== planStem` guard fires and + // pushes the "68-01-SUMMARY.md" candidate. The summary here matches ONLY + // through that candidate (not the marker-swap or stem-suffix forms), so + // flipping the guard's equality in either direction breaks this match. + assert.strictEqual( + countMatchedSummaries(['68-01-scaffolding-PLAN.md'], ['68-01-SUMMARY.md']), + 1, + ); + assert.deepEqual(findUnsummarizedPlans(['68-01-scaffolding-PLAN.md'], ['68-01-SUMMARY.md']), []); + }); + + test('mutation-gap: findUnsummarizedPlans on a MIXED set returns exactly the unsummarized subset', () => { + // An all-matched or all-unmatched fixture can't distinguish a `.filter()` + // that was mutated to always-true/always-false/identity from a correctly + // behaving one — only a mixed set, checked by exact array identity, can. + const plans = ['01-01-PLAN.md', '01-02-PLAN.md', '01-03-PLAN.md']; + const summaries = ['01-01-SUMMARY.md', '01-03-SUMMARY.md']; // 01-02 has none + assert.deepEqual(findUnsummarizedPlans(plans, summaries), ['01-02-PLAN.md']); + }); + + test('mutation-gap: findOrphanSummaries on a MIXED set returns exactly the unclaimed subset', () => { + const plans = ['01-01-PLAN.md', '01-02-PLAN.md']; + const summaries = ['01-01-SUMMARY.md', '01-02-SUMMARY.md', '01-STRAY-SUMMARY.md']; + assert.deepEqual(findOrphanSummaries(plans, summaries), ['01-STRAY-SUMMARY.md']); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // #3883 (ADR-3473 §8.3) — one slug implementation per rule // diff --git a/tests/gap-checker.property.test.cjs b/tests/gap-checker.property.test.cjs index 9b2487c31..635e84b1b 100644 --- a/tests/gap-checker.property.test.cjs +++ b/tests/gap-checker.property.test.cjs @@ -901,5 +901,136 @@ describe('#3885 (ADR-3473 §8.5): runGapAnalysis distinguishes unreadable from a cleanup(tmpDir); } }); + + // #4014 (epic #3473 B4-unreadable) matrix row 12: runGapAnalysis now + // consumes `scope` from findContextMdIn(phaseDir) and surfaces it as the + // additive `phase_dir_scope` field — the typed SCOPE-enum sibling of the + // pre-existing `phase_dir_read_error` string field asserted above. + test('#4014 matrix row 12: unreadable phase dir reports phase_dir_scope:unreadable', (t) => { + setup(); + try { + injectReaddirFailure(t, phaseDir, 'EACCES'); + const result = runGapAnalysis(tmpDir, phaseDir); + assert.strictEqual(result.phase_dir_scope, 'unreadable', + `an unreadable phase directory must report phase_dir_scope 'unreadable'; got: ${result.phase_dir_scope}`); + } finally { + cleanup(tmpDir); + } + }); + + test('#4014: readable phase dir reports phase_dir_scope:complete (must not regress)', () => { + setup(); + try { + const result = runGapAnalysis(tmpDir, phaseDir); + assert.strictEqual(result.phase_dir_scope, 'complete'); + } finally { + cleanup(tmpDir); + } + }); + + test('#4014: genuinely absent phase dir reports phase_dir_scope:complete (boundary — not unreadable)', () => { + tmpDir = createTempProject(); + const missingPhaseDir = path.join(tmpDir, '.planning', 'phases', '09-nonexistent'); + try { + const result = runGapAnalysis(tmpDir, missingPhaseDir); + assert.strictEqual(result.phase_dir_scope, 'complete', + `a genuinely absent phase directory must report phase_dir_scope 'complete', not 'unreadable'; got: ${result.phase_dir_scope}`); + } finally { + cleanup(tmpDir); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// #4014 (epic #3473 B4-unreadable) matrix row 15 — independence: the SAME +// injected-unreadable phase directory must be reported distinguishably from +// an empty one, using the SAME SCOPE.UNREADABLE value, across all three CLI +// surfaces this issue touches: `roadmap analyze` (roadmap.cts), gap-checker +// (gap-checker.cts), and one `init` bundle (init.cts). All three surfaces are +// exercised in-process (each module's exported function called directly, +// mirroring the identical direct-call style already used above and in +// init.test.cjs's #3885 block) against one shared fixture, so a single +// `fs.readdirSync` monkeypatch on the same phase directory drives all three. +// ───────────────────────────────────────────────────────────────────────────── +describe('#4014 matrix row 15: unreadable-vs-empty identity is consistent across roadmap/gap-checker/init', () => { + const fs = require('fs'); + const path = require('path'); + const { createTempProject, cleanup } = require('./helpers.cjs'); + const { runGapAnalysis } = require('../gsd-core/bin/lib/gap-checker.cjs'); + const roadmapLib = require('../gsd-core/bin/lib/roadmap.cjs'); + const initMod = require('../gsd-core/bin/lib/init.cjs'); + + function injectReaddirFailure(t, targetPath, code) { + const resolved = path.resolve(targetPath); + const origReaddirSync = fs.readdirSync.bind(fs); + t.mock.method(fs, 'readdirSync', (p, ...rest) => { + if (path.resolve(String(p)) === resolved) { + const err = new Error(`${code}: simulated failure, scandir '${p}'`); + err.code = code; + throw err; + } + return origReaddirSync(p, ...rest); + }); + } + + // `output()` writes via `fs.writeSync(1, ...)`, bypassing console.log — see + // init.test.cjs's captureFd1 for the identical rationale/pattern. + function captureFd1(run) { + const chunks = []; + const origWriteSync = fs.writeSync; + fs.writeSync = function patchedWriteSync(fd, data, offset, length) { + if (fd !== 1) return origWriteSync.apply(fs, arguments); + const buf = Buffer.isBuffer(data) ? data : Buffer.from(data); + const start = offset ?? 0; + const len = length ?? (buf.length - start); + chunks.push(Buffer.from(buf.subarray(start, start + len))); + return len; + }; + try { + run(); + } finally { + fs.writeSync = origWriteSync; + } + return JSON.parse(Buffer.concat(chunks).toString('utf8')); + } + + test('the same unreadable phase directory reports SCOPE.UNREADABLE consistently on all three surfaces', (t) => { + const tmpDir = createTempProject(); + try { + const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.md'), '# Plan\n'); + fs.writeFileSync( + path.join(tmpDir, '.planning', 'ROADMAP.md'), + '# Roadmap\n\n### Phase 3: API\n**Goal:** Build API\n', + ); + + injectReaddirFailure(t, phaseDir, 'EACCES'); + + // Surface 1: gap-checker. + const gapResult = runGapAnalysis(tmpDir, phaseDir); + assert.strictEqual(gapResult.phase_dir_scope, 'unreadable', + `gap-checker surface: got phase_dir_scope=${gapResult.phase_dir_scope}`); + + // Surface 2: roadmap analyze. + const analyzeOutput = captureFd1(() => roadmapLib.cmdRoadmapAnalyze(tmpDir, false)); + const phase3 = analyzeOutput.phases.find((p) => p.number === '3'); + assert.ok(phase3, `phase 3 must appear in analyze output; got: ${JSON.stringify(analyzeOutput.phases)}`); + assert.strictEqual(phase3.context_scope, 'unreadable', + `roadmap analyze surface: got context_scope=${phase3.context_scope}`); + + // Surface 3: one init bundle (cmdInitPlanPhase). + const initOutput = captureFd1(() => initMod.cmdInitPlanPhase(tmpDir, '03', false)); + assert.strictEqual(initOutput.context_scope, 'unreadable', + `init surface: got context_scope=${initOutput.context_scope}`); + + // All three used the SAME typed SCOPE.UNREADABLE value — no ad-hoc + // per-surface string vocabulary. + assert.strictEqual(gapResult.phase_dir_scope, phase3.context_scope); + assert.strictEqual(phase3.context_scope, initOutput.context_scope); + } finally { + cleanup(tmpDir); + } + }); }); diff --git a/tests/init.test.cjs b/tests/init.test.cjs index f9b36bc47..0a248774d 100644 --- a/tests/init.test.cjs +++ b/tests/init.test.cjs @@ -3413,11 +3413,17 @@ describe('#3885 (ADR-3473 §8.5): init callers distinguish unreadable from absen const phaseDirAbs = () => path.join(projectDir, '.planning', 'phases', '03-api'); describe('cmdInitPlanPhase', () => { + // #4014 (epic #3473 B4-unreadable) matrix row 14: a readable, genuinely + // context-less phase dir reports context_scope 'complete' — the + // additive scope signal adjacent to has_context. test('readablePhaseDirReportsNoReadError (MUST STAY GREEN)', (t) => { const output = captureFd1(t, () => initMod.cmdInitPlanPhase(projectDir, '03', false)); assert.strictEqual(output.context_read_error ?? null, null); + assert.strictEqual(output.context_scope, 'complete'); }); + // #4014 matrix row 13: has_context stays false AND context_scope + // distinguishes this from genuine absence. test('unreadablePhaseDirIsNotReportedAsAbsent', (t) => { injectReaddirFailure(t, phaseDirAbs(), 'EACCES'); const output = captureFd1(t, () => initMod.cmdInitPlanPhase(projectDir, '03', false)); @@ -3425,6 +3431,9 @@ describe('#3885 (ADR-3473 §8.5): init callers distinguish unreadable from absen `an unreadable phase directory must be reported, not silently absent; got: ${JSON.stringify(output.context_read_error)}`); assert.ok(output.context_read_error.includes('03-api'), `the reported error must name the discarded input (the phase directory); got: ${output.context_read_error}`); + assert.strictEqual(output.has_context, false); + assert.strictEqual(output.context_scope, 'unreadable', + `an unreadable phase directory must report context_scope 'unreadable', distinct from a genuinely empty one; got: ${output.context_scope}`); }); test('raceConditionEnoentStaysAGenuineSilentDegrade (MUST STAY GREEN)', (t) => { @@ -3436,11 +3445,14 @@ describe('#3885 (ADR-3473 §8.5): init callers distinguish unreadable from absen }); describe('cmdInitPhaseOp', () => { + // #4014 matrix row 14 (second surface). test('readablePhaseDirReportsNoReadError (MUST STAY GREEN)', (t) => { const output = captureFd1(t, () => initMod.cmdInitPhaseOp(projectDir, '03', false)); assert.strictEqual(output.context_read_error ?? null, null); + assert.strictEqual(output.context_scope, 'complete'); }); + // #4014 matrix row 13 (second surface). test('unreadablePhaseDirIsNotReportedAsAbsent', (t) => { injectReaddirFailure(t, phaseDirAbs(), 'EACCES'); const output = captureFd1(t, () => initMod.cmdInitPhaseOp(projectDir, '03', false)); @@ -3448,6 +3460,9 @@ describe('#3885 (ADR-3473 §8.5): init callers distinguish unreadable from absen `an unreadable phase directory must be reported, not silently absent; got: ${JSON.stringify(output.context_read_error)}`); assert.ok(output.context_read_error.includes('03-api'), `the reported error must name the discarded input (the phase directory); got: ${output.context_read_error}`); + assert.strictEqual(output.has_context, false); + assert.strictEqual(output.context_scope, 'unreadable', + `an unreadable phase directory must report context_scope 'unreadable', distinct from a genuinely empty one; got: ${output.context_scope}`); }); test('raceConditionEnoentStaysAGenuineSilentDegrade (MUST STAY GREEN)', (t) => { diff --git a/tests/planning-workspace.test.cjs b/tests/planning-workspace.test.cjs index 67501aca7..4de0e71f7 100644 --- a/tests/planning-workspace.test.cjs +++ b/tests/planning-workspace.test.cjs @@ -499,32 +499,42 @@ describe('bug #3739 — gap-analysis padded-prefix CONTEXT.md', () => { }); // ── Test 5: findContextMdIn helper unit test ───────────────────────────── + // + // #4014 (epic #3473 B4-unreadable) matrix rows 1-3: the directory-string + // form now returns `{ file, files, scope }` instead of a bare string/null + // — `.file` carries what these tests used to assert directly on the + // return value. - test('findContextMdIn helper returns padded filename when present', () => { + test('findContextMdIn helper returns padded filename when present (matrix row 2)', () => { const { findContextMdIn } = require('../gsd-core/bin/lib/planning-workspace.cjs'); // Write 01-CONTEXT.md into the phase dir (already created in beforeEach) fs.writeFileSync(path.join(phaseDir, '01-CONTEXT.md'), '# context\n'); - const found = findContextMdIn(phaseDir); - assert.strictEqual(found, '01-CONTEXT.md', + const result = findContextMdIn(phaseDir); + assert.strictEqual(result.file, '01-CONTEXT.md', 'findContextMdIn must return the padded-prefix filename'); + assert.ok(result.files.includes('01-CONTEXT.md'), 'files must include the raw listing'); + assert.strictEqual(result.scope, 'complete'); }); - test('findContextMdIn helper returns bare filename when only bare form exists', () => { + test('findContextMdIn helper returns bare filename when only bare form exists (matrix row 1)', () => { const { findContextMdIn } = require('../gsd-core/bin/lib/planning-workspace.cjs'); fs.writeFileSync(path.join(phaseDir, 'CONTEXT.md'), '# context\n'); - const found = findContextMdIn(phaseDir); - assert.strictEqual(found, 'CONTEXT.md', + const result = findContextMdIn(phaseDir); + assert.strictEqual(result.file, 'CONTEXT.md', 'findContextMdIn must return CONTEXT.md for bare form'); + assert.strictEqual(result.scope, 'complete'); }); - test('findContextMdIn helper returns null when no CONTEXT.md exists', () => { + test('findContextMdIn helper returns null file when no CONTEXT.md exists (matrix row 3)', () => { const { findContextMdIn } = require('../gsd-core/bin/lib/planning-workspace.cjs'); // phaseDir exists but is empty (no CONTEXT.md) - const found = findContextMdIn(phaseDir); - assert.strictEqual(found, null, - 'findContextMdIn must return null when no CONTEXT.md exists'); + const result = findContextMdIn(phaseDir); + assert.strictEqual(result.file, null, + 'findContextMdIn must return a null file when no CONTEXT.md exists'); + assert.strictEqual(result.scope, 'complete', + 'a successfully-read, genuinely context-less directory is scope complete, not unreadable'); }); // ── Test 5b: findContextMdIn accepts pre-read files array (avoids double readdirSync) ── @@ -551,8 +561,8 @@ describe('bug #3739 — gap-analysis padded-prefix CONTEXT.md', () => { fs.writeFileSync(path.join(phaseDir, 'CONTEXT.md'), '# bare context\n'); fs.writeFileSync(path.join(phaseDir, '01-CONTEXT.md'), '# padded context\n'); - const found = findContextMdIn(phaseDir); - assert.strictEqual(found, 'CONTEXT.md', + const result = findContextMdIn(phaseDir); + assert.strictEqual(result.file, 'CONTEXT.md', 'findContextMdIn must return bare CONTEXT.md when both forms exist — matches pre-refactor gap-checker behavior'); }); @@ -583,13 +593,25 @@ describe('bug #3739 — gap-analysis padded-prefix CONTEXT.md', () => { }); } -// ─── bug #1883: findContextMdIn must not swallow permission/I-O errors ─────── -// A catch-all `catch { return null }` conflated a genuine ENOENT ("nothing there") -// with an EACCES/EIO failure ("can't read this"), so an unreadable phase dir was -// silently reported as "no CONTEXT.md" — discuss/plan gates then wrongly believed -// context had never been gathered. The narrowed catch re-throws every non-ENOENT -// error and keeps returning null only for genuine absence. -describe('bug #1883 — findContextMdIn distinguishes a permission error from emptiness', () => { +// ─── bug #1883 / #4014: findContextMdIn distinguishes unreadable from absent ── +// A catch-all `catch { return null }` originally conflated a genuine ENOENT +// ("nothing there") with an EACCES/EIO failure ("can't read this"), so an +// unreadable phase dir was silently reported as "no CONTEXT.md" — discuss/ +// plan gates then wrongly believed context had never been gathered. #1883's +// fix re-threw every non-ENOENT error to surface the distinction to a caller +// via try/catch. +// +// #4014 (epic #3473 B4-unreadable) changes the SHAPE of that fix: throwing +// pushed every one of findContextMdIn's callers into hand-rolling their own +// try/catch (five call sites, independently — the exact duplication this +// epic's B4 item closes). The directory-string form now never throws — +// ENOENT and a successful read both resolve to `scope: SCOPE.COMPLETE` +// (ENOENT is a genuine "nothing there yet" answer, not a failure — ADR-3180's +// existing COMPLETE-with-zero-items contract), and every other read error +// resolves to `scope: SCOPE.UNREADABLE` instead of propagating an exception. +// Matrix rows 4 and 5 below are the identity pair this item exists to keep +// distinguishable: both share `file: null`, but only row 5's `scope` differs. +describe('bug #1883 / #4014 — findContextMdIn distinguishes unreadable from absent', () => { const { findContextMdIn } = require('../gsd-core/bin/lib/planning-workspace.cjs'); // Helper: build a Node-style error with a `code`, matching what fs.readdirSync throws. @@ -601,48 +623,56 @@ describe('bug #1883 — findContextMdIn distinguishes a permission error from em return err; } - test('findContextMdIn re-throws a permission (EACCES) error instead of swallowing it as null', (t) => { + // Matrix row 5 — negative, the defect this issue fixes: a directory that + // exists but cannot be read must be reported as scope 'unreadable', not + // silently collapsed to the same shape row 4 (genuine absence) produces. + test('findContextMdIn reports scope unreadable on a permission (EACCES) error instead of throwing (matrix row 5)', (t) => { // No chmod 0o000 — root bypasses mode bits (silent zero coverage in root CI). // t.mock auto-restores after the test. t.mock.method(fs, 'readdirSync', () => { throw fsError('EACCES'); }); - assert.throws( - () => findContextMdIn('/denied/phase-dir'), - (err) => err.code === 'EACCES', - 'an unreadable dir must propagate EACCES, not return null as if empty', - ); + const result = findContextMdIn('/denied/phase-dir'); + assert.deepStrictEqual(result, { file: null, files: [], scope: 'unreadable' }, + 'an unreadable dir must report scope unreadable, not throw and not look like an absent dir'); }); - test('findContextMdIn re-throws any non-ENOENT error (EIO)', (t) => { + test('findContextMdIn reports scope unreadable on any non-ENOENT error (EIO)', (t) => { t.mock.method(fs, 'readdirSync', () => { throw fsError('EIO'); }); - assert.throws( - () => findContextMdIn('/io-failure/phase-dir'), - (err) => err.code === 'EIO', - 'every non-ENOENT error must propagate — the narrowed catch only keeps ENOENT', - ); + const result = findContextMdIn('/io-failure/phase-dir'); + assert.deepStrictEqual(result, { file: null, files: [], scope: 'unreadable' }, + 'every non-ENOENT error must resolve to scope unreadable — the narrowed catch only keeps ENOENT as scope complete'); }); - test('findContextMdIn returns null for an absent dir (ENOENT) — empty path unchanged', () => { + // Matrix row 4 — boundary, the "real empty" case: a directory that + // genuinely does not exist must NOT become unreadable. + test('findContextMdIn reports scope complete with a null file for an absent dir (ENOENT) (matrix row 4)', () => { // A path that genuinely does not exist yields ENOENT from the real OS call. const absent = path.join(os.tmpdir(), 'gsd-1883-does-not-exist-' + process.pid); - assert.strictEqual(findContextMdIn(absent), null, - 'an absent dir (ENOENT) must still return null — Hyrum: empty path unchanged'); + assert.deepStrictEqual(findContextMdIn(absent), { file: null, files: [], scope: 'complete' }, + 'an absent dir (ENOENT) must report scope complete with a null file — Hyrum: the pre-#4014 null-on-ENOENT contract is unchanged in substance, just re-shaped'); }); - test('findContextMdIn finds bare CONTEXT.md', (t) => { + test('findContextMdIn finds bare CONTEXT.md (matrix row 1)', (t) => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1883-bare-')); t.after(() => { cleanup(tmp); }); fs.writeFileSync(path.join(tmp, 'CONTEXT.md'), '# bare\n'); - assert.strictEqual(findContextMdIn(tmp), 'CONTEXT.md'); + const result = findContextMdIn(tmp); + assert.strictEqual(result.file, 'CONTEXT.md'); + assert.strictEqual(result.scope, 'complete'); }); - test('findContextMdIn finds padded NN-CONTEXT.md', (t) => { + test('findContextMdIn finds padded NN-CONTEXT.md (matrix row 2)', (t) => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1883-padded-')); t.after(() => { cleanup(tmp); }); fs.writeFileSync(path.join(tmp, '01-CONTEXT.md'), '# padded\n'); - assert.strictEqual(findContextMdIn(tmp), '01-CONTEXT.md'); + const result = findContextMdIn(tmp); + assert.strictEqual(result.file, '01-CONTEXT.md'); + assert.strictEqual(result.scope, 'complete'); }); - test('findContextMdIn matches against a pre-read files array without touching fs', (t) => { + // Matrix row 6 — the array-input form must not regress: unchanged + // string|null return, no object wrapping, and it must never touch fs (the + // whole point of accepting a pre-read listing). + test('findContextMdIn matches against a pre-read files array without touching fs (matrix row 6)', (t) => { // The array path never calls readdirSync — exercised by getPhaseFileStats, // countPhasePlansAndSummaries, runGapAnalysis. Must keep working untouched. t.mock.method(fs, 'readdirSync', () => { throw new Error('array path must not call fs'); }); @@ -651,7 +681,7 @@ describe('bug #1883 — findContextMdIn distinguishes a permission error from em assert.strictEqual(findContextMdIn(['02-CONTEXT.md']), '02-CONTEXT.md', 'array path matches padded form'); assert.strictEqual(findContextMdIn(['PLAN.md']), null, - 'array path returns null when no match'); + 'array path returns null when no match — no object wrapping for the array form'); }); }); diff --git a/tests/roadmap.test.cjs b/tests/roadmap.test.cjs index 405744d37..957065f3d 100644 --- a/tests/roadmap.test.cjs +++ b/tests/roadmap.test.cjs @@ -4312,6 +4312,53 @@ describe('#3885 (ADR-3473 §8.5): countPhasePlansAndSummaries distinguishes unre `ENOENT must be treated as genuinely absent, not reported as an error; got: ${phase.context_read_error}`, ); }); + + // ─── #4014 (epic #3473 B4-unreadable) matrix rows 7-9 ─────────────────── + // + // `countPhasePlansAndSummaries` (not exported — driven through + // cmdRoadmapAnalyze, the same style as T61/T62/T64 above) gains a + // `context_scope` field on its result, surfaced on `AnalyzePhase` as + // `context_scope` — the typed SCOPE-enum sibling of the existing + // `context_read_error` string field. + + test('#4014 matrix row 7: readable phase dir with CONTEXT.md reports has_context:true, context_scope:complete', () => { + const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, 'CONTEXT.md'), '# context\n'); + + const output = runAnalyzeCapturingStdout(tmpDir); + const phase = findPhase3(output); + assert.strictEqual(phase.has_context, true); + assert.strictEqual(phase.context_scope, 'complete'); + assert.strictEqual(phase.context_read_error ?? null, null); + }); + + test('#4014 matrix row 8: unreadable phase dir reports context_scope:unreadable, distinct from a genuinely empty phase dir', () => { + const phaseDir = path.join(tmpDir, '.planning', 'phases', '03-api'); + fs.mkdirSync(phaseDir, { recursive: true }); + fs.writeFileSync(path.join(phaseDir, '03-01-PLAN.md'), '---\nwave: 1\n---\n## Task 1\n'); + + const restore = injectReaddirFailure(phaseDir, 'EACCES'); + let output; + try { + output = runAnalyzeCapturingStdout(tmpDir); + } finally { + restore(); + } + const phase = findPhase3(output); + assert.strictEqual(phase.has_context, false); + assert.strictEqual(phase.context_scope, 'unreadable', + `an unreadable phase directory must report context_scope 'unreadable', distinct from a genuinely empty one; got: ${phase.context_scope}`); + }); + + test('#4014 matrix row 9: genuinely absent phase dir reports context_scope:complete (boundary — not unreadable)', () => { + // No phase directory created at all — matches T64's ENOENT-shaped absence. + const output = runAnalyzeCapturingStdout(tmpDir); + const phase = findPhase3(output); + assert.strictEqual(phase.has_context, false); + assert.strictEqual(phase.context_scope, 'complete', + `a genuinely absent phase directory must report context_scope 'complete', not 'unreadable'; got: ${phase.context_scope}`); + }); }); // ═══════════════════════════════════════════════════════════════════════════ diff --git a/tests/slug-derivation-drift-guard.test.cjs b/tests/slug-derivation-drift-guard.test.cjs index b59fc96fa..837dc8eac 100644 --- a/tests/slug-derivation-drift-guard.test.cjs +++ b/tests/slug-derivation-drift-guard.test.cjs @@ -127,7 +127,7 @@ describe('findSlugDerivationDrift — T3-T5: sanctioned sites are exempted BY th describe('findSlugDerivationDrift — MAJOR-1: allowlist exemption is scoped to the REAL function body, not "until the next top-level function"', () => { const sanctionedRealEndLines = [ - { file: path.join('src', 'core-utils.cts'), fn: 'generateSlugInternal', realEndLine: 193 }, + { file: path.join('src', 'core-utils.cts'), fn: 'generateSlugInternal', realEndLine: 199 }, { file: path.join('src', 'gsd2-import.cts'), fn: 'slugify', realEndLine: 103 }, { file: path.join('src', 'runtime-artifact-conversion.cts'), fn: 'normalizeKimiSkillName', realEndLine: 616 }, { file: path.join('scripts', 'generate-package-identity.cjs'), fn: 'slugifyPackageName', realEndLine: 42 },