diff --git a/.changeset/proud-foxes-sprint.md b/.changeset/proud-foxes-sprint.md new file mode 100644 index 000000000..779891629 --- /dev/null +++ b/.changeset/proud-foxes-sprint.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2729 +--- +**An unreadable ROADMAP.md is no longer reported as a brand-new project** — a permission or I/O error reading `.planning/ROADMAP.md` used to return the same "phase not found" and `v1.0 / milestone` values as a project that simply has no roadmap yet, so workflows synthesized a blank phase or skipped requirement extraction with no signal. GSD now names the unreadable file on stderr while returning exactly what it returned before. A project that genuinely has no ROADMAP.md stays silent. (#1881) diff --git a/CONTEXT.md b/CONTEXT.md index e4417f647..c18eec2b2 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -116,7 +116,7 @@ Cross-seam principle (ADR-1411, epic #1411): context resolution — config loadi Diagnostic-output convention for the Resolution Provenance principle (ADR-1411 P3, #1416). Config-interpreting read verbs expose `Resolution { value, configured, reason, warnings }` (`src/resolution.cts`); agent-skills is the first adopter, where `value = { block, skills_count }` and `source`/`degraded` remain config-provenance extras outside the envelope. Other read verbs expose at least `warnings[]` (e.g. capability-state `{ runtimeConfigDir, capabilities, warnings? }`) without `configured`/`reason`, which are meaningful only for config-interpreting verbs. Mutation verbs expose `warnings[]` (advisory) PLUS `errors[]` (operation-not-applied), e.g. capability-writer `{ capabilities, warnings, errors }`. The shared seam across all shapes is `warnings: string[]`; a single generic `Resolution` across read+write verbs was rejected by the deletion test (`configured`/`reason` are meaningless for capability verbs; `errors[]` cannot fold into `warnings[]`) — ADR-1411 P3 amendment. Recurrence prevention is delivered by P4's CI guard (a configured input resolving empty must carry a `reason`), not by a shared envelope. A CI guard (`scripts/lint-resolution-provenance.cjs`, wired into `lint:ci`) enforces that every registered config-interpreting read verb keeps a `configured_empty`/`not_configured` contract test; the registry in that script is the registration point for future verbs (ADR-1411 P4 / #1417). ### Unusable Input Diagnostic Module -Leaf module owning the **out-of-band** half of ADR-1411's "corrupt is not absent" amendment (epic #1879). Where a read already returns a provenance envelope the cause is named in-band (`ConfigResolution.reason`, #1880); where a read returns a bare sentinel or a plausible default it cannot extend, the return value is preserved exactly and the cause is surfaced here instead. Interface: `UNUSABLE_REASON` (frozen reason enum — one entry per condition that has an emitting call site; adding a reason is three coordinated changes: enum + call site + the test locking `Object.keys(...).sort()`), `warnUnusableInput({reason, source?, content?}) → boolean` (returns whether this call actually wrote, so tests assert emission *counts* on a typed surface rather than scraping stderr), plus the `_resetUnusableInputWarningsForTests` / `_unusableInputWarningCountForTests` seams. Dedup key is `\0` — **both halves are load-bearing**: keying on the path alone would let a second, different fault on the same file go unreported, and keying on message prose would couple the guard to wording (ADR-1411 dedup clause). Path separators are deliberately **not** normalized: an earlier revision folded backslashes to `/` so two spellings of one Windows path would not double-report, but a backslash is a legal filename character on Linux and macOS, so that folding collapsed two genuinely distinct POSIX files onto one key and swallowed the second file's diagnostic. The trade is now one-directional — two spellings of one Windows path may report twice (noise), but two distinct files can never silence each other (lost signal), and ADR-1411 ranks the swallow the worse failure; ASCII control characters are stripped from the source before it is keyed or written, because the key separator is NUL (a crafted path could otherwise forge a collision) and because a path carrying ANSI escapes would replay into the operator's terminal. Callers with no path (in-memory content) fall back to a short content digest so *different* bad inputs still key differently. The diagnostic is **unconditional** — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since "an opt-in nobody sets is indistinguishable from the silence #1879 is about" — and **never throws**: a failed stderr write is swallowed so a degraded read is never escalated into a crash. First adopter is `extractFrontmatter` (#1882); `roadmap-parser` (#1881) and `planning-workspace`/`verify` (#1883) follow. Exists as a shared seam rather than a per-site copy because four sites need identical behavior and four hand-rolled copies is `DEFECT.GENERATIVE-FIX` by construction. Source of truth: `gsd-core/bin/lib/unusable-input.cjs` (generated from `src/unusable-input.cts`). Test anchor: `tests/unusable-input.test.cjs`. See Resolution Provenance, Config Loader Module. +Leaf module owning the **out-of-band** half of ADR-1411's "corrupt is not absent" amendment (epic #1879). Where a read already returns a provenance envelope the cause is named in-band (`ConfigResolution.reason`, #1880); where a read returns a bare sentinel or a plausible default it cannot extend, the return value is preserved exactly and the cause is surfaced here instead. Interface: `UNUSABLE_REASON` (frozen reason enum — one entry per condition that has an emitting call site; adding a reason is three coordinated changes: enum + call site + the test locking `Object.keys(...).sort()`), `warnUnusableInput({reason, source?, content?}) → boolean` (returns whether this call actually wrote, so tests assert emission *counts* on a typed surface rather than scraping stderr), plus the `_resetUnusableInputWarningsForTests` / `_unusableInputWarningCountForTests` seams. Dedup key is `\0` — **both halves are load-bearing**: keying on the path alone would let a second, different fault on the same file go unreported, and keying on message prose would couple the guard to wording (ADR-1411 dedup clause). Path separators are deliberately **not** normalized: an earlier revision folded backslashes to `/` so two spellings of one Windows path would not double-report, but a backslash is a legal filename character on Linux and macOS, so that folding collapsed two genuinely distinct POSIX files onto one key and swallowed the second file's diagnostic. The trade is now one-directional — two spellings of one Windows path may report twice (noise), but two distinct files can never silence each other (lost signal), and ADR-1411 ranks the swallow the worse failure; ASCII control characters are stripped from the source before it is keyed or written, because the key separator is NUL (a crafted path could otherwise forge a collision) and because a path carrying ANSI escapes would replay into the operator's terminal. Callers with no path (in-memory content) fall back to a short content digest so *different* bad inputs still key differently. The diagnostic is **unconditional** — a deliberate divergence from ADR-227's never-implemented `GSD_DEBUG` opt-in, since "an opt-in nobody sets is indistinguishable from the silence #1879 is about" — and **never throws**: a failed stderr write is swallowed so a degraded read is never escalated into a crash. Adopted by `extractFrontmatter` (#1882, `frontmatter_unterminated`) and by `getRoadmapPhaseInternal`/`getMilestoneInfo` (#1881, `roadmap_unreadable`); `planning-workspace`/`verify` (#1883) follow. #1881 detects on the errno alone: `platformReadSync` returns `null` for ENOENT and its callers convert that to an errno-less Error, so reporting unconditionally in those catches would flag every project without a ROADMAP.md as corrupt. Exists as a shared seam rather than a per-site copy because four sites need identical behavior and four hand-rolled copies is `DEFECT.GENERATIVE-FIX` by construction. Source of truth: `gsd-core/bin/lib/unusable-input.cjs` (generated from `src/unusable-input.cts`). Test anchor: `tests/unusable-input.test.cjs`. See Resolution Provenance, Config Loader Module. ### Worktree Safety Policy Module CJS Module owning worktree lifecycle safety policy for the GSD orchestration layer. Interface: `resolveWorktreeContext(cwd, deps) → WorktreeContext` (linked-worktree root mapping), `parseWorktreePorcelain(output) → WorktreeEntry[]` (porcelain parser, skips detached HEAD), `planWorktreePrune(repoRoot, opts, deps) → PrunePlan` (metadata-prune plan, never destructive by default), `executeWorktreePrunePlan(plan, deps) → PruneResult` (executes prune; degrades gracefully on git timeout), `listLinkedWorktreePaths(repoRoot, deps) → LinkedPathsResult`, `inspectWorktreeHealth(repoRoot, opts, deps) → HealthResult` (orphan + stale detection), `snapshotWorktreeInventory(repoRoot, opts, deps) → InventoryResult`, `planWorktreeWaveCleanup(repoRoot, manifest) → CleanupPlan` (manifest-scoped, fail-closed), `executeWorktreeWaveCleanupPlan(plan, deps) → CleanupResult`, `planWorktreeRecordAgent(manifestRaw, fields) → RecordAgentPlan` (write-strict per-agent manifest append; validates each field at write time via the same `normalizeCleanupManifestEntry` rules the reader enforces; fail-closed on a missing/garbled field or a duplicate `(worktree_path, branch)` the reader would dedup away), `cmdWorktreeRecordAgent(cwd, args, deps) → RecordAgentCmdResult` (thin deps-injectable IO wrapper for the `worktree record-agent` verb), `planWorktreeCreate(fields) → WorktreeCreatePlan` (write-strict `worktree create` planner — same missing-field-hint and `normalizeCleanupManifestEntry` validation as `planWorktreeRecordAgent`, pure/no-git), `executeWorktreeCreatePlan(plan, repoRoot, deps) → WorktreeCreateResult` (bounded `git rev-parse --verify` base check THEN `git worktree add -b `; fail-closed `base_unresolved`/`git_timeout`/`worktree_add_failed`; returns `cwd` — the working directory an executor spawn would use), `cmdWorktreeCreate(cwd, args, deps) → WorktreeCreateCmdResult` (CLI verb: plans, creates the worktree, then appends the manifest entry so it is immediately manageable by cleanup-wave/reap-orphans; dedupes by `(worktree_path, branch)`). #2584 ADR-1239 Codex-binding amendment, Phase 2: `worktree create` is the git-worktree-creation primitive for `dispatch.isolation: orchestrator-worktree` hosts — declared and testable but UNCONSUMED (no scheduler calls it yet; Phase 3 wires it). Source of truth: `gsd-core/bin/lib/worktree-safety.cjs`. Timeout path: all git subprocess calls are bounded; callers receive `ok:false, reason:'git_timed_out'` rather than a thrown exception. Test anchor: `tests/worktree-safety.test.cjs`. The `core.cjs` re-export spine was retired in epic #1267: this module absorbed the two thin compositional wrappers that squatted in Core — `resolveWorktreeRoot(cwd, deps)` (a projection over `resolveWorktreeContext`) and `pruneOrphanedWorktrees(...)` (sequences `planWorktreePrune` + `executeWorktreePrunePlan` with a timeout warning) — so callers reach this single worktree-lifecycle seam directly. `gitWorktreeInfoInternal` did NOT move here — worktree-info detection belongs to the Git Query Module. @@ -161,7 +161,7 @@ Canonical GFM table parsing + schema registry seam (`gsd-core/bin/lib/markdown-t Shared fail-loud `Result` and per-surface write-set contracts (`gsd-core/bin/lib/write-set.cjs`, generated from `src/write-set.cts`; ADR-2143 §5/§6, epic #2143). Pure, Node built-ins only, no I/O. Exports: `Result` (`{ok:true,value}\|{ok:false,reason}` — ADR-2143 §5 fail-loud parse shape, never a bare `null` a caller can mistake for "empty but fine"; the single source of truth `markdown-table.cjs` re-exports so its existing importers are unaffected; deliberately distinct from command-routing-hub's dispatch `Result` `{ok,data\|kind}`); `WriteOutcome` (`{surface: string, applied: boolean}` — one surface's outcome within a multi-surface write); `WriteSet` (`WriteOutcome[]`); `writeSetComplete(ws) → boolean` (true only when the set is non-empty AND every surface applied — ADR-2143 §6's "no OR-into-one-flag" rule: a command that mutates more than one surface must not collapse independent surface outcomes into a single boolean, the anti-pattern that let a checkbox-only partial write (#2140) report full success). `milestone.cts`'s `requirements mark-complete` handler is the first consumer: it reports a `write_set` (`checkbox`/`traceability` surfaces) and `write_set_complete` alongside its existing `updated`/`marked_complete`/`already_complete`/`not_found`/`table_unmatched` fields, which remain computed exactly as before — the write-set is additive, structured ADR-2143 documentation of the same per-surface facts #2140's tactical fix already exposed via `table_unmatched`. ### Roadmap Parser Module -Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `withPhaseSection`). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). +Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `withPhaseSection`). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`, and — since #1881 — `unusable-input` for the out-of-band diagnostic) — no `loadConfig`, no other core dependency. An unreadable ROADMAP.md is reported rather than collapsed into the same sentinel as a genuinely absent one; absence itself stays silent, and neither lookup gains a throw (the #2245 audit records that `src/state.cts` removed its defensive try/catch on the strength of `getMilestoneInfo` never throwing). Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). ### Core Utilities Module Module owning the shared low-level utility primitives extracted from Core: POSIX path normalization (`toPosixPath`), filesystem scanning (`detectSubRepos`, `readSubdirectories`, `getPhaseFileStats`, `pathExistsInternal`), and small pure helpers (`generateSlugInternal`, `extractOneLinerFromBody`, `filterPlanFiles`, `filterSummaryFiles`, `extractCanonicalPlanId`, `timeAgo`). Depends only on Node built-ins and already-leafed modules (`phase-id` for `comparePhaseNum`, `planning-workspace` for `findContextMdIn`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2c (#877) as the shared leaf that unblocks the phase-locator fs-search extraction (2d); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/core-utils.cjs` (generated from `src/core-utils.cts`). diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index 95585e6b5..f4843ddaa 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -33,6 +33,9 @@ const { import planningWorkspace = require('./planning-workspace.cjs'); const { planningDir } = planningWorkspace; import { platformReadSync } from './shell-command-projection.cjs'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import unusableInputMod = require('./unusable-input.cjs'); +const { UNUSABLE_REASON, warnUnusableInput } = unusableInputMod; import { tokenizeHeadings, stripTaggedBlocks, withSection } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; @@ -323,10 +326,18 @@ function getRoadmapPhaseInternal(cwd: string, phaseNum: unknown): RoadmapPhaseRe if (!phaseNum) return null; const normalizedPhase = stripProjectCodePrefix(phaseNum); if (/^999(?:\.|$)/.test(normalizedPhase)) return null; - const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); - if (!fs.existsSync(roadmapPath)) return null; + // Resolved INSIDE the try for the same reason as getMilestoneInfo below: planningDir + // throws a plain Error for an invalid GSD_WORKSTREAM/GSD_PROJECT segment, and resolving + // it outside let that escape uncaught, crashing every caller for a malformed workstream + // name. ADR-227 is explicit that throwing breaks pipeline continuity, and this read path + // has no reason to be the exception -- it already degrades to null for every other + // failure. Absence still returns null before any diagnostic, and when the path never + // resolved there is nothing to name. + let roadmapPath: string | undefined; try { + roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); + if (!fs.existsSync(roadmapPath)) return null; const roadmapRaw = platformReadSync(roadmapPath); if (roadmapRaw === null) throw new Error('missing'); const content = extractCurrentMilestone(roadmapRaw, cwd); @@ -352,11 +363,33 @@ function getRoadmapPhaseInternal(cwd: string, phaseNum: unknown): RoadmapPhaseRe } return null; - } catch { + } catch (err) { + // Absence already returned above via existsSync; anything caught here is a read fault + // or the synthetic missing-marker. The null is preserved exactly either way. + if (roadmapPath !== undefined) reportUnreadableRoadmap(err, roadmapPath); return null; } } +/** + * Report a ROADMAP.md that exists but could not be read (#1881, ADR-1411). + * + * The discriminator is the errno, and it matters in the SILENT direction. + * platformReadSync returns null for ENOENT and both callers convert that null into a + * synthetic Error carrying no code, which lands in the same catch as a real EACCES. + * Reporting unconditionally here would flag every project that has no ROADMAP.md yet -- + * every brand-new project -- as corrupt. A genuine read fault always carries an errno; + * absence never does. + * + * The parse itself is regex over text and cannot throw, so anything reaching a catch is + * either a read fault or that synthetic absence marker. Nothing else gets here. + */ +function reportUnreadableRoadmap(err: unknown, roadmapPath: string): void { + const code = (err as { code?: unknown } | null | undefined)?.code; + if (typeof code !== 'string') return; + warnUnusableInput({ reason: UNUSABLE_REASON.ROADMAP_UNREADABLE, source: roadmapPath }); +} + // ─── Milestone info lookup ──────────────────────────────────────────────────── interface MilestoneInfo { @@ -378,8 +411,16 @@ function stripLeadingDelimiter(s: string): string { } function getMilestoneInfo(cwd: string): MilestoneInfo { + // Declared here but RESOLVED INSIDE the try, so the catch can name the file without + // moving planningDir() out of the protected region. planningDir throws a plain Error + // for an invalid GSD_WORKSTREAM/GSD_PROJECT segment, and hoisting the call let that + // escape uncaught — breaking the invariant #2245 relies on, that this function never + // throws. When the path never resolved there is nothing to name, so the diagnostic is + // skipped and the default is returned exactly as before. + let roadmapPath: string | undefined; try { - const roadmap = platformReadSync(path.join(planningDir(cwd), 'ROADMAP.md')); + roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md'); + const roadmap = platformReadSync(roadmapPath); if (roadmap === null) throw new Error('missing'); let stateVersion: string | null = null; @@ -453,7 +494,12 @@ function getMilestoneInfo(cwd: string): MilestoneInfo { version: versionMatch ? versionMatch[0] : 'v1.0', name: 'milestone', }; - } catch { + } catch (err) { + // This function has no existsSync guard, so an absent ROADMAP arrives here too, as a + // synthetic Error with no errno. Only a real read fault is reported; the populated + // default is returned unchanged either way, and a plausible-looking default needs the + // diagnostic more than an empty sentinel does, not less (ADR-1411). + if (roadmapPath !== undefined) reportUnreadableRoadmap(err, roadmapPath); return { version: 'v1.0', name: 'milestone' }; } } diff --git a/src/unusable-input.cts b/src/unusable-input.cts index 7c74750b4..96c7f5af6 100644 --- a/src/unusable-input.cts +++ b/src/unusable-input.cts @@ -44,6 +44,11 @@ const UNUSABLE_REASON = Object.freeze({ * legitimately has no frontmatter. (#1882) */ FRONTMATTER_UNTERMINATED: 'frontmatter_unterminated', + /** + * A ROADMAP.md exists but could not be read (EACCES/EIO/…). Distinct from a project that + * simply has no ROADMAP yet: absence returns the same sentinel, silently. (#1881) + */ + ROADMAP_UNREADABLE: 'roadmap_unreadable', } as const); type UnusableReason = (typeof UNUSABLE_REASON)[keyof typeof UNUSABLE_REASON]; @@ -52,6 +57,8 @@ type UnusableReason = (typeof UNUSABLE_REASON)[keyof typeof UNUSABLE_REASON]; const REASON_PROSE: Readonly> = Object.freeze({ [UNUSABLE_REASON.FRONTMATTER_UNTERMINATED]: 'frontmatter opens with "---" but never closes; metadata was NOT applied', + [UNUSABLE_REASON.ROADMAP_UNREADABLE]: + 'ROADMAP.md exists but could not be read; phase and milestone lookups fell back to defaults', }); // ─── Dedup state ────────────────────────────────────────────────────────────── diff --git a/tests/mutation-workflow-base-ref.test.cjs b/tests/mutation-workflow-base-ref.test.cjs index 8418f807b..f60b761be 100644 --- a/tests/mutation-workflow-base-ref.test.cjs +++ b/tests/mutation-workflow-base-ref.test.cjs @@ -173,7 +173,15 @@ describe('#2452 CI gates: base-ref fetch must preserve ancestry', () => { git(origin, ['checkout', '--quiet', 'base']); for (let n = 1; n <= BASE_ADVANCE; n++) { fs.writeFileSync(path.join(origin, `base-${n}.txt`), `${n}\n`); - git(origin, ['add', '.']); + // Stage only the file this iteration created. `git add .` re-stages every + // file already in the tree, so across BASE_ADVANCE iterations it rehashes + // O(n²) blobs — roughly 1,800 stagings and 60 full index rewrites to add 60 + // one-line files. That churn is what this loop failed on in CI: the index + // ended up referencing a blob whose object write had not landed + // ("invalid object … for 'base-31.txt' / Error building trees") at commit 32 + // of 60. Staging the single new path is equivalent here — each commit adds + // exactly one file — and removes the redundant work entirely. + git(origin, ['add', `base-${n}.txt`]); git(origin, ['commit', '--quiet', '-m', `base advance ${n}`]); } diff --git a/tests/roadmap-parser.test.cjs b/tests/roadmap-parser.test.cjs index b0fbab575..86edc9c4b 100644 --- a/tests/roadmap-parser.test.cjs +++ b/tests/roadmap-parser.test.cjs @@ -2491,3 +2491,309 @@ describe('feat-3594: roadmap parser does not crash on ANY corpus fixture', () => }); }); } + +// ─── #1881: an unreadable ROADMAP is not an absent one ─────────────────────── +// +// getRoadmapPhaseInternal returns null for a read failure exactly as it does for +// "phase not found", and getMilestoneInfo returns {v1.0, milestone} — which reads +// as a brand-new project — for a read failure exactly as it does for "no ROADMAP +// yet". Per ADR-1411's "corrupt is not absent" amendment both return values are +// preserved and the cause is surfaced out of band instead. +// +// The discriminator is the errno, and it is load-bearing in the silent direction: +// getMilestoneInfo has no existsSync guard, so platformReadSync's null-for-ENOENT +// is converted to a synthetic Error('missing') that lands in the SAME catch as a +// real EACCES. Reporting unconditionally there would flag every project that has +// no ROADMAP.md — i.e. every brand-new project — as corrupt. Half of these cases +// exist to hold that line. +// +// Faults are injected by overriding platformReadSync and restoring in t.after(), +// never chmod 0o000: root bypasses mode bits, so a permission-based version would +// silently pass with zero coverage in root Docker/CI. + +describe('#1881 unreadable ROADMAP vs absent ROADMAP', () => { + const scp = require('../gsd-core/bin/lib/shell-command-projection.cjs'); + const { + UNUSABLE_REASON, + _resetUnusableInputWarningsForTests, + _unusableInputEmissionCountForTests, + } = require('../gsd-core/bin/lib/unusable-input.cjs'); + + const HEALTHY = [ + '# Roadmap', + '', + '## Milestone v2.3: Alpha', + '', + '### Phase 1: Alpha', + '**Goal**: ship', + '', + ].join('\n'); + + /** Write a project with the given ROADMAP content (or none when null). */ + function project(t, roadmap) { + const dir = createTempProject('gsd-1881-'); + t.after(() => cleanup(dir)); + const planning = path.join(dir, '.planning'); + fs.mkdirSync(planning, { recursive: true }); + if (roadmap !== null) fs.writeFileSync(path.join(planning, 'ROADMAP.md'), roadmap); + return dir; + } + + /** + * Make reads of files matching `match` fail with `err`, restoring in t.after(). + * Overrides the projection seam rather than the filesystem so the failure is + * deterministic on every platform and under root. + */ + function failReads(t, match, err) { + const original = scp.platformReadSync; + t.after(() => { + Object.defineProperty(scp, 'platformReadSync', { value: original, configurable: true }); + }); + Object.defineProperty(scp, 'platformReadSync', { + configurable: true, + value: (p) => { + if (match(String(p))) throw err; + return original(p); + }, + }); + } + + /** Silence stderr for the duration of `fn` and report how many diagnostics it wrote. */ + function emissionsDuring(fn) { + const before = _unusableInputEmissionCountForTests(); + const originalWrite = process.stderr.write; + process.stderr.write = () => true; + try { + fn(); + } finally { + process.stderr.write = originalWrite; + } + return _unusableInputEmissionCountForTests() - before; + } + + function eacces() { + const e = new Error('EACCES: permission denied'); + e.code = 'EACCES'; + return e; + } + + test('a healthy roadmap resolves a phase and stays silent', (t) => { + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + let result; + const emitted = emissionsDuring(() => { result = getRoadmapPhaseInternal(dir, '1'); }); + assert.strictEqual(result.found, true); + assert.strictEqual(emitted, 0); + }); + + test('a healthy roadmap resolves the milestone and stays silent', (t) => { + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + let info; + const emitted = emissionsDuring(() => { info = getMilestoneInfo(dir); }); + assert.deepStrictEqual(info, { version: 'v2.3', name: 'Alpha' }); + assert.strictEqual(emitted, 0); + }); + + test('an unreadable roadmap is reported on a phase lookup, and still returns null', (t) => { + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + failReads(t, (p) => p.endsWith('ROADMAP.md'), eacces()); + let result; + const emitted = emissionsDuring(() => { result = getRoadmapPhaseInternal(dir, '1'); }); + assert.strictEqual(result, null, 'the sentinel must be preserved exactly'); + assert.strictEqual(emitted, 1); + }); + + test('an unreadable roadmap is reported on a milestone lookup, and still returns the default', (t) => { + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + failReads(t, (p) => p.endsWith('ROADMAP.md'), eacces()); + let info; + const emitted = emissionsDuring(() => { info = getMilestoneInfo(dir); }); + assert.deepStrictEqual(info, { version: 'v1.0', name: 'milestone' }, + 'the plausible-looking default must be preserved exactly'); + assert.strictEqual(emitted, 1); + }); + + test('a project with NO roadmap stays silent — absence is not corruption', (t) => { + // The false-positive that would otherwise fire on every brand-new project. + _resetUnusableInputWarningsForTests(); + const dir = project(t, null); + let phase, info; + const emitted = emissionsDuring(() => { + phase = getRoadmapPhaseInternal(dir, '1'); + info = getMilestoneInfo(dir); + }); + assert.strictEqual(phase, null); + assert.deepStrictEqual(info, { version: 'v1.0', name: 'milestone' }); + assert.strictEqual(emitted, 0, 'a missing ROADMAP.md must never be reported as unreadable'); + }); + + test('a phase that is genuinely not in the roadmap stays silent', (t) => { + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + let result; + const emitted = emissionsDuring(() => { result = getRoadmapPhaseInternal(dir, '7'); }); + assert.strictEqual(result, null); + assert.strictEqual(emitted, 0); + }); + + test('roadmap content that parses to nothing stays silent', (t) => { + // The parse is regex over text and cannot throw, so unparseable content never + // reaches the catch. Content that yields nothing is absent information, not an + // unusable file. + _resetUnusableInputWarningsForTests(); + const dir = project(t, 'just prose, no headings at all\n'); + let phase, info; + const emitted = emissionsDuring(() => { + phase = getRoadmapPhaseInternal(dir, '1'); + info = getMilestoneInfo(dir); + }); + assert.strictEqual(phase, null); + assert.strictEqual(emitted, 0); + assert.ok(info, 'still returns a milestone default'); + }); + + test('an unreadable STATE.md alone stays silent — the inner catch is deliberate', (t) => { + // roadmap-parser.cts:394-400 records under the #2245 audit that STATE.md's + // `milestone:` field is an OPTIONAL enhancement whose failure legitimately falls + // back to ROADMAP-only heuristics. A diagnostic leaking from it would misreport + // that fallback as roadmap corruption. + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + fs.writeFileSync(path.join(dir, '.planning', 'STATE.md'), 'milestone: v2.3\n'); + failReads(t, (p) => p.endsWith('STATE.md'), eacces()); + let info; + const emitted = emissionsDuring(() => { info = getMilestoneInfo(dir); }); + assert.deepStrictEqual(info, { version: 'v2.3', name: 'Alpha' }); + assert.strictEqual(emitted, 0); + }); + + test('any errno is reported, not just EACCES', (t) => { + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + const eio = new Error('EIO: i/o error'); + eio.code = 'EIO'; + failReads(t, (p) => p.endsWith('ROADMAP.md'), eio); + const emitted = emissionsDuring(() => { getRoadmapPhaseInternal(dir, '1'); }); + assert.strictEqual(emitted, 1, 'the discriminator is "has an errno", not a whitelist'); + }); + + test('an error carrying no errno stays silent — that is the absence path', (t) => { + // platformReadSync returns null for ENOENT and the caller converts it to a + // synthetic Error with no .code. That error means "absent", not "unusable". + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + failReads(t, (p) => p.endsWith('ROADMAP.md'), new Error('missing')); + let result; + const emitted = emissionsDuring(() => { result = getRoadmapPhaseInternal(dir, '1'); }); + assert.strictEqual(result, null); + assert.strictEqual(emitted, 0); + }); + + test('a non-string errno is tolerated and stays silent', (t) => { + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + const weird = new Error('odd'); + weird.code = 42; + failReads(t, (p) => p.endsWith('ROADMAP.md'), weird); + let result; + const emitted = emissionsDuring(() => { result = getRoadmapPhaseInternal(dir, '1'); }); + assert.strictEqual(result, null); + assert.strictEqual(emitted, 0); + }); + + test('both lookups against one unreadable roadmap report once', (t) => { + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + failReads(t, (p) => p.endsWith('ROADMAP.md'), eacces()); + const emitted = emissionsDuring(() => { + getRoadmapPhaseInternal(dir, '1'); + getMilestoneInfo(dir); + }); + assert.strictEqual(emitted, 1, 'it is one unreadable file'); + }); + + test('a repeated lookup of the same unreadable roadmap reports once', (t) => { + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + failReads(t, (p) => p.endsWith('ROADMAP.md'), eacces()); + const emitted = emissionsDuring(() => { + getRoadmapPhaseInternal(dir, '1'); + getRoadmapPhaseInternal(dir, '1'); + }); + assert.strictEqual(emitted, 1); + }); + + test('two different unreadable roadmaps both report', (t) => { + _resetUnusableInputWarningsForTests(); + const a = project(t, HEALTHY); + const b = project(t, HEALTHY); + failReads(t, (p) => p.endsWith('ROADMAP.md'), eacces()); + const emitted = emissionsDuring(() => { + getRoadmapPhaseInternal(a, '1'); + getRoadmapPhaseInternal(b, '1'); + }); + assert.strictEqual(emitted, 2, 'a second file must never be suppressed by the first'); + }); + + test('getMilestoneInfo does not throw when the read fails', (t) => { + // ADR-1411 names this explicitly: src/state.cts removed a defensive try/catch + // around getMilestoneInfo under the #2245 audit because it "never throws". + // Introducing a throw here silently breaks that invariant. + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + failReads(t, () => true, eacces()); + let info; + emissionsDuring(() => { + assert.doesNotThrow(() => { info = getMilestoneInfo(dir); }); + }); + assert.deepStrictEqual(info, { version: 'v1.0', name: 'milestone' }); + }); + + test('getRoadmapPhaseInternal does not throw when the read fails', (t) => { + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + failReads(t, () => true, eacces()); + let result; + emissionsDuring(() => { + assert.doesNotThrow(() => { result = getRoadmapPhaseInternal(dir, '1'); }); + }); + assert.strictEqual(result, null); + }); + + test('neither lookup throws when the planning path itself cannot be resolved', (t) => { + // The regression this pair exists to catch. planningDir throws a plain Error for an + // invalid GSD_WORKSTREAM segment. Resolving it OUTSIDE the try -- which is what naming + // the file for the diagnostic naively required -- let that escape uncaught, crashing + // every caller of getMilestoneInfo for a workstream name containing a slash, and + // breaking the invariant #2245 relies on. The earlier 'does not throw' cases inject + // only through platformReadSync and so could never have caught it. + _resetUnusableInputWarningsForTests(); + const dir = project(t, HEALTHY); + const previous = process.env.GSD_WORKSTREAM; + t.after(() => { + if (previous === undefined) delete process.env.GSD_WORKSTREAM; + else process.env.GSD_WORKSTREAM = previous; + }); + process.env.GSD_WORKSTREAM = 'evil/../thing'; + let info, phase; + const emitted = emissionsDuring(() => { + assert.doesNotThrow(() => { info = getMilestoneInfo(dir); }); + assert.doesNotThrow(() => { phase = getRoadmapPhaseInternal(dir, '1'); }); + }); + assert.deepStrictEqual(info, { version: 'v1.0', name: 'milestone' }); + assert.strictEqual(phase, null); + assert.strictEqual(emitted, 0, + 'the path never resolved, so there is no file to name'); + }); + + test('the roadmap reason is present in the frozen vocabulary', () => { + // The full key-set lock lives once, in tests/unusable-input.test.cjs. Duplicating it + // here would mean two files to update every time a phase adds a reason, which defeats + // the point of a single coordinated change. This asserts only what #1881 owns. + assert.ok(Object.isFrozen(UNUSABLE_REASON)); + assert.strictEqual(UNUSABLE_REASON.ROADMAP_UNREADABLE, 'roadmap_unreadable'); + }); +}); diff --git a/tests/unusable-input.test.cjs b/tests/unusable-input.test.cjs index 84bb45df6..c2da7a9bf 100644 --- a/tests/unusable-input.test.cjs +++ b/tests/unusable-input.test.cjs @@ -70,7 +70,10 @@ describe('UNUSABLE_REASON', () => { assert.ok(Object.isFrozen(UNUSABLE_REASON), 'enum must be frozen'); // Locking the key set is what makes adding a reason three coordinated changes // (enum + call site + this assertion) instead of a silent widening. - assert.deepStrictEqual(Object.keys(UNUSABLE_REASON).sort(), ['FRONTMATTER_UNTERMINATED']); + assert.deepStrictEqual( + Object.keys(UNUSABLE_REASON).sort(), + ['FRONTMATTER_UNTERMINATED', 'ROADMAP_UNREADABLE'], + ); assert.strictEqual(UNUSABLE_REASON.FRONTMATTER_UNTERMINATED, 'frontmatter_unterminated'); });