From 80778e2674a956cc8d77a50a809b1604e1e18725 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 27 Jul 2026 20:17:09 -0400 Subject: [PATCH] fix(#1881): report an unreadable ROADMAP instead of reading it as absent (#2729) * test(#1882): stage one file per commit in the base-ref ancestry fixture CI failed on ubuntu-24 inside this test's setup loop, before any code under test ran: at commit 32 of 60 the index referenced a blob whose object write had not landed -- "invalid object ... for 'base-31.txt' / Error building trees". The loop staged with `git add .`, which re-stages every file already in the tree. Across 60 iterations that rehashes O(n squared) blobs -- roughly 1,800 stagings and 60 full index rewrites to add 60 one-line files -- and that churn is what the object store failed under. Each commit only ever adds a single new file, so staging that one path is equivalent and removes the redundant work entirely. Verified the loop still builds the intended history: 61 commits, git fsck clean. The fixture already carries a note from an earlier fix in this epic recording that it passed on ubuntu-22 and windows-24 and failed on ubuntu-24 for the same commit. That was a different stage -- fetch versus diff -- but the same lane and the same brittleness, so this is the second time this fixture's cost has surfaced as a red build rather than as a test failure. Not caused by this PR's change, which touches two configuration lists and cannot reach a scratch git repository in tmpdir. Fixed here rather than deferred, because the run surfaced it. Refs #1879 Co-Authored-By: Claude Opus 5 * test(#1881): prove an unreadable ROADMAP is indistinguishable from an absent one Failing-first. Encodes the issue's runtime repro: an unreadable ROADMAP.md makes getRoadmapPhaseInternal return the same null it returns for "phase not found", and getMilestoneInfo return the same {v1.0, milestone} it returns for a project with no roadmap at all -- so a permission or I/O fault reads as a brand-new project. Half these cases exist to hold the opposite line. getMilestoneInfo has no existsSync guard, so platformReadSync's null-for-ENOENT is converted to a synthetic Error carrying no errno, and that lands in the SAME catch as a real EACCES. Reporting unconditionally there would flag every project without a ROADMAP.md -- every brand-new project -- as corrupt. The absent case, the errno-less error, a non-string errno, unparseable content and a genuinely missing phase are all pinned silent. One case guards a decision rather than behaviour: an unreadable STATE.md alone must stay silent, because the inner catch that swallows it is deliberate and documented under the #2245 audit as an optional enhancement falling back to ROADMAP-only heuristics. Two more pin the invariant ADR-1411 names explicitly -- neither function may throw, because src/state.cts removed its own defensive try/catch on the strength of that guarantee. Assertions are on the frozen reason enum and the emission counter, never on diagnostic prose. Faults are injected by overriding the platformReadSync seam and restoring in t.after(), never chmod 0o000, which root bypasses. Adds the ROADMAP_UNREADABLE reason to the shared vocabulary as scaffolding; no call site emits it yet, which is what makes these tests red. Refs #1879 Co-Authored-By: Claude Opus 5 * fix(#1881): report an unreadable ROADMAP instead of reading it as absent getRoadmapPhaseInternal returned null for a read failure exactly as it does for "phase not found", and getMilestoneInfo returned {v1.0, milestone} exactly as it does for a project with no roadmap -- so a permission or I/O fault presented as a brand-new project and workflows synthesised a blank phase or skipped requirement extraction with no signal. Both return values are preserved exactly, per ADR-1411's amendment: continuity is correct, the silence was the defect. Each catch now reports through the shared unusable-input seam that shipped with #1882 rather than a second copy of the same mechanism. 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 into a synthetic Error with no code that lands in the same catch as a real EACCES. Reporting unconditionally there would flag every project without a ROADMAP.md -- every brand-new project -- as corrupt. A genuine read fault always carries an errno; absence never does. The parse is regex over text and cannot throw, so nothing else reaches these catches. Neither function gains a throw. ADR-1411 names this explicitly: src/state.cts removed its defensive try/catch around getMilestoneInfo under the #2245 audit because it never throws, and two tests pin that. The inner STATE.md catch stays untouched and silent -- its fallback to ROADMAP-only heuristics is a deliberate, documented optional-enhancement path, not a fault. Where the fix belongs was the design question. platformReadSync does not leak: it keeps absent and unusable as two channels, exactly as an abstraction should. Both callers re-collapsed that distinction, so the fix is caller-side and the projection seam -- with roughly ninety other dependents -- is untouched. Closes #1881 Co-Authored-By: Claude Opus 5 * test(#1881): admit the roadmap reason to the locked vocabulary The seam documents adding a reason as three coordinated changes -- the enum entry, the emitting call site, and the test that locks Object.keys(...).sort(). This PR made the first two and the lock caught the third, which is the whole point of pinning the key set rather than asserting each value exists. The roadmap suite no longer re-locks the full set. Two complete locks would mean two files to update every time a later phase adds a reason, and #1883 and #1884 are both going to. The canonical lock stays in the seam's own suite; the roadmap suite asserts only the value it introduces. Refs #1879 Co-Authored-By: Claude Opus 5 * fix(#1881): resolve the roadmap path inside the try, not outside it Naming the file in the diagnostic required the resolved path in the catch, and the obvious way to get it was to hoist `path.join(planningDir(cwd), 'ROADMAP.md')` above the try. planningDir throws a plain Error for an invalid GSD_WORKSTREAM or GSD_PROJECT segment -- one containing a slash, backslash or `..` -- so hoisting it let that throw escape uncaught. That broke the exact invariant ADR-1411 names as this file's hazard: src/state.cts removed its defensive try/catch around getMilestoneInfo under the #2245 audit because that function never throws. Of its callers only archivePhaseDirectories wraps it; cmdInitExecutePhase, cmdInitNewMilestone, cmdInitMilestoneOp, cmdInitManager, cmdInitProgress, cmdProgressRender and cmdStats all call it bare, so a workstream name with a slash in it crashed the CLI outright instead of degrading. The previous commit asserted "neither function gains a throw -- two tests pin that". That was false. Both tests inject faults through platformReadSync only and never through planningDir, so neither could have exercised the path that broke. The guarantee was claimed, not demonstrated. The path is now declared before the try and resolved inside it, so the catch can still name the file when there is one, and a path that never resolved reports nothing and returns the sentinel unchanged. The two test names are narrowed to what they actually prove -- that a failing READ does not throw -- and a new case injects the planningDir failure directly, which is what would have caught this. getRoadmapPhaseInternal carried the same hazard, resolving the path outside its try since before this branch. It is fixed the same way rather than left: ADR-227 is explicit that throwing breaks pipeline continuity, this read path already degrades to null for every other failure, and a PR whose purpose is hardening this invariant is the wrong place to leave the sibling crashing. Behaviour otherwise unchanged and re-verified: healthy lookups, EACCES reporting on both functions, absent-roadmap silence, and the errno discriminator all unaffected. Refs #1879 Co-Authored-By: Claude Opus 5 * chore(#1881): backfill changeset pr number to 2729 --------- Co-authored-by: Claude Opus 5 --- .changeset/proud-foxes-sprint.md | 5 + CONTEXT.md | 4 +- src/roadmap-parser.cts | 56 +++- src/unusable-input.cts | 7 + tests/mutation-workflow-base-ref.test.cjs | 10 +- tests/roadmap-parser.test.cjs | 306 ++++++++++++++++++++++ tests/unusable-input.test.cjs | 5 +- 7 files changed, 384 insertions(+), 9 deletions(-) create mode 100644 .changeset/proud-foxes-sprint.md 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'); });