Files
msd-core/src/unusable-input.cts
Tom Boucher 80778e2674 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

* chore(#1881): backfill changeset pr number to 2729

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-07-27 20:17:09 -04:00

244 lines
12 KiB
TypeScript

/**
* Unusable Input Diagnostic — the out-of-band half of ADR-1411's
* "corrupt is not absent" amendment (epic #1879).
*
* ADR-1411 splits the amendment's mechanism in two. Where a read already returns a
* provenance envelope, the cause is named *in-band* — that is `ConfigResolution.reason`
* (#1880, shipped). 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 *out-of-band*,
* as a deduplicated diagnostic on stderr. This module owns that second mechanism.
*
* It exists as a shared seam rather than a pattern copied per site because four call sites
* across four modules need identical behaviour (#1882 frontmatter, #1881 roadmap-parser,
* #1883 planning-workspace/verify, #1884 planning lock). Four hand-rolled copies of one
* behaviour is `DEFECT.GENERATIVE-FIX` by construction; one seam with a frozen reason set
* is the documented cure.
*
* Two contracts this module must not break:
*
* - **Unconditional.** ADR-1411 diverges deliberately from ADR-227's never-implemented
* `GSD_DEBUG` opt-in: "an opt-in nobody sets is indistinguishable from the silence
* #1879 is about". There is no config gate here, by design.
* - **Never throws.** Callers are leaf readers that promised a total function. A failed
* stderr write (closed stream, EPIPE) must not turn a silent degradation into a crash.
*/
import crypto from 'node:crypto';
// ─── Reason vocabulary ────────────────────────────────────────────────────────
/**
* Frozen so tests assert a typed surface instead of diagnostic prose
* (CONTRIBUTING.md — Prohibited: Raw Text Matching on Test Outputs).
*
* Adding a reason is three coordinated changes, matching the repo's `REASON`-enum
* convention: the entry here, the emitting call site, and the test that locks
* `Object.keys(UNUSABLE_REASON).sort()`. Each epic-#1879 phase adds only its own —
* pre-declaring the later phases' reasons would be speculative generality and would
* leave values no call site emits.
*/
const UNUSABLE_REASON = Object.freeze({
/**
* A file opened a `---` frontmatter fence at byte 0, carried at least one parseable
* key, and never closed the fence — a truncated or half-written file, NOT a file that
* 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];
/** One human-readable clause per reason. Prose lives here, never in a test assertion. */
const REASON_PROSE: Readonly<Record<UnusableReason, string>> = 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 ──────────────────────────────────────────────────────────────
/**
* Process-lifetime dedup set. Mirrors `config-loader.cjs`'s `_warnedUnknownConfigKeys`
* guard, which ADR-1411 names as the precedent to reuse.
*/
const _warnedUnusableInputs = new Set<string>();
/**
* Count of diagnostics actually WRITTEN, which is not the same as the size of the dedup set:
* one emission records every key the input could later be identified by, so set size counts
* identities while this counts events. Tests assert on this because the behavioural claim is
* "how many diagnostics did the operator see", not "how many keys are interned".
*/
let _unusableInputEmissions = 0;
/**
* ASCII control characters (including NUL) are stripped from any path before it is used
* as a key component or written to a terminal. Two reasons, both real:
*
* - the key separator is NUL, so a `sourcePath` containing NUL could otherwise forge a
* collision with a different (path, reason) pair and suppress a genuine second failure;
* - a path carrying ANSI escapes would be replayed verbatim into the operator's terminal.
*/
const CONTROL_CHARS = /[\u0000-\u001F\u007F]/g;
/**
* Strip control characters. Deliberately does NOT normalize path separators.
*
* An earlier revision folded backslashes to `/` unconditionally, reasoning that `C:\a\b.md`
* and `C:/a/b.md` are one file and should not report twice. That is true on Windows, and
* false — destructively — everywhere else: `\` is a legal filename character on Linux and
* macOS, so `/repo/weird\name/PLAN.md` and `/repo/weird/name/PLAN.md` are two genuinely
* different files that collapsed to one key, and the second one's diagnostic was silently
* swallowed. ADR-1411 forbids exactly that ("keying too coarsely suppresses a genuine second
* failure in a different file"), and this repo targets Linux/macOS/Windows alike.
*
* The trade is now explicit and one-directional: two spellings of one Windows path may
* report twice (mild noise), but two distinct files can never silence each other (lost
* signal). Dropping a real diagnostic is the strictly worse failure.
*/
function sanitizeSource(source: string): string {
return source.replace(CONTROL_CHARS, '');
}
/**
* Identify the offending input. A path is preferred because it is what an operator can act
* on. When the caller has only an in-memory string, fall back to a short content digest so
* that *different* bad inputs still produce *different* keys.
*
* The leading `p`/`d` tag is what keeps the two namespaces disjoint. Without it a caller
* whose file is literally named `<unnamed:8efa5269728e7271>` would key identically to a
* path-less caller whose content happens to hash to that digest — no brute force required,
* since the digest of any predictable content (a shared template, known boilerplate) can
* simply be computed and used as a filename to pre-seed suppression. Because control
* characters — including NUL — are stripped from `source`, a caller-supplied path can never
* contain the separator and so can never forge a key in the other namespace either.
*
* The digest is computed only on the emission path, which is rare, so it never costs
* anything on a healthy read.
*/
function sourceKey(source?: string, content?: string): string {
if (typeof source === 'string' && source.trim() !== '') {
return `p\u0000${sanitizeSource(source)}`;
}
const digest = crypto.createHash('sha256').update(content ?? '').digest('hex').slice(0, 16);
return `d\u0000${digest}`;
}
/** Human-facing name for the offending input, derived from the same key. */
function displaySource(key: string): string {
return key.startsWith('p\u0000') ? key.slice(2) : `<unnamed:${key.slice(2)}>`;
}
// ─── Emission ─────────────────────────────────────────────────────────────────
interface WarnUnusableInputArgs {
/** Which unusable-input condition fired. */
reason: UnusableReason;
/** Resolved path of the offending file, when the caller has one. */
source?: string;
/** Raw content, used only to derive a dedup key when `source` is absent. */
content?: string;
}
/**
* Emit a deduplicated diagnostic naming an input that exists but cannot be used.
*
* The key is `<normalized source>\0<reason>`. ADR-1411 requires the resolved path AND the
* distinguishing cause — keying on the path alone would let a second, different fault on
* the same file go unreported; keying on the message prose would couple the guard to
* wording.
*
* @returns `true` when this call actually wrote a diagnostic, `false` when it was
* deduplicated. Returning the decision is what lets tests assert emission *counts* on a
* typed surface rather than scraping stderr.
*/
function warnUnusableInput({ reason, source, content }: WarnUnusableInputArgs): boolean {
// Defensive: an unknown reason must not emit a diagnostic with `undefined` in it.
const prose = Object.prototype.hasOwnProperty.call(REASON_PROSE, reason)
? REASON_PROSE[reason]
: null;
if (prose === null) return false;
// The guarantee, stated precisely, because it is not symmetric:
//
// * a file reported BY NAME is reported at most once, and
// * an anonymous re-parse of content already reported by name stays silent, and
// * two DIFFERENT files always both report, even when their truncated bytes are identical.
//
// The asymmetry is the anonymous-FIRST ordering (a path-less parse, then a named parse of the
// same content), which emits twice. That is a deliberate limit, not an oversight. A path-less
// caller cannot identify its file, so suppressing the later named report would also suppress a
// genuine second failure in a DIFFERENT file whenever two files share byte-identical truncated
// content — the over-coarse keying ADR-1411 explicitly forbids. Between a duplicate line and a
// swallowed diagnostic the ADR ranks the swallow worse, so the duplicate is accepted; and the
// second line is the more useful of the two, because it carries the filename.
//
// Mechanically: check ONLY the key matching what this caller actually knows, but record every
// key the input could later be identified by.
const identity = sourceKey(source, content);
const keys = [`${identity}\u0000${reason}`];
if (typeof content === 'string' && identity.startsWith('p\u0000')) {
keys.push(`${sourceKey(undefined, content)}\u0000${reason}`);
}
if (_warnedUnusableInputs.has(keys[0])) return false;
for (const k of keys) _warnedUnusableInputs.add(k);
try {
process.stderr.write(`gsd: warning — ${displaySource(identity)}: ${prose}. (#1879)\n`);
// Counted only after a write that actually completed. Incrementing before the try counted
// attempts, so on a broken stderr the counter claimed a diagnostic had reached the operator
// when nothing had — a seam documented as "written" reporting something else.
_unusableInputEmissions += 1;
} catch {
/* a closed or broken stderr must never escalate a degraded read into a crash */
}
return true;
}
// ─── Test seams ───────────────────────────────────────────────────────────────
/**
* Clear the dedup state between cases.
*
* This exists because the set is process-global: without it, the second test to use a key
* silently observes the first test's suppression. #2674 is the cautionary precedent — a
* reset helper that cleared two of three sets was a silent no-op for the very suite that
* existed to test it, and the cases only passed because each happened to pick a key no
* other case reused.
*/
function _resetUnusableInputWarningsForTests(): void {
_warnedUnusableInputs.clear();
_unusableInputEmissions = 0;
}
/** Number of diagnostics written — the typed surface tests assert on instead of stderr prose. */
function _unusableInputEmissionCountForTests(): number {
return _unusableInputEmissions;
}
/** Size of the dedup set (identities interned, not events). Retained for key-shape assertions. */
function _unusableInputWarningCountForTests(): number {
return _warnedUnusableInputs.size;
}
/** Test seam: the sanitized form of a source, so control-char handling is asserted on a
* returned value instead of by scraping what reached stderr. */
function _sanitizeSourceForTests(source: string): string {
return sanitizeSource(source);
}
export = {
UNUSABLE_REASON,
_sanitizeSourceForTests,
warnUnusableInput,
_resetUnusableInputWarningsForTests,
_unusableInputWarningCountForTests,
_unusableInputEmissionCountForTests,
};