From 2e2b8ba4a7b586f6d1eb41ff1f11a7d37b08b88a Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 9 Aug 2026 17:08:46 -0400 Subject: [PATCH] enhance(#2704): resolve documentation links and compare H1 status brackets in the ADR gate (#3266) * test(#2704): failing-first coverage for ADR link resolution and H1 status brackets Binds the gate to two assertions it does not yet make: every relative markdown link under docs/adr/ must resolve, and an H1 trailing status bracket must agree with the Status: field instead of being silently stripped. Covers all 51 rows of the phase test matrix across two altitudes - the pure extractLinks/maskCode IR for fence and inline-code-span boundaries, hostile input and the fast-check totality properties, and the real CLI verdict for the end-to-end classes. Includes the DEFECT.GENERATIVE-FIX parity test that iterates the exported STATUSES array so a sixth status is covered the day it is added. * feat(#2704): resolve ADR documentation links and compare H1 status brackets The ADR gate validated naming, relation symmetry and index freshness but never resolved a link target, and it stripped an ADR's trailing H1 status bracket for display rather than comparing it against that ADR's own Status: field. Both classes were structurally invisible: #2691 found five dangling references by manual audit roughly a year after they were introduced, one of which reached the published npm payload, while CI reported green throughout. Both are now assertions on the same --check path, using only node:fs and node:path - no dependency and no subprocess. Fenced blocks and inline code spans are masked before scanning, because markdown does not render a link inside code. That is not a policy choice: the corpus contains exactly two such sequences today and both are ordinary JavaScript. Masking preserves length and column positions so findings still name a real line. Resolution is case-exact on every platform - a link that resolves only through macOS or Windows case-folding still 404s on github.com and still fails the Linux lane - and a destination resolving outside the repository is reported before any filesystem call is made. Also single-sources two duplicated surfaces this change would otherwise have extended: the H1 bracket vocabulary (a second hand-written copy of STATUSES with nothing asserting agreement, a DEFECT.GENERATIVE-FIX instance) and the docs/adr directory traversal. Two tests added by #2691 that reimplemented link resolution and bracket comparison inside the test file are removed for the same reason; the corpus assertion is now made by running the real gate against the real corpus. * fix(#2704): reject symlinks that leave the repository and linearize code masking Four defects from the isolated adversarial security review, plus one it noted. BLOCKER - a symlink defeated path containment. path.relative(ROOT, abs) is purely lexical, but the case-exact walk then calls readdirSync, which follows symlinks at the OS level: a contributor-committed docs/adr/x -> /etc together with a link through it passed containment and listed the real external directory, and a wrong-case probe echoed a real external filename through the "Did you mean" hint into publicly-readable fork-PR logs. Every segment is now lstat'd before descent; a symlink is realpathed and re-checked against realpath(ROOT) - realpath on both sides, so a root under /var does not produce false escapes - and an escape emits no hint and reads nothing further. The same rule now governs which FILES are read: an ADR entry that is a symlink out of the repository is excluded and reported rather than parsed, closing the vector this change had widened by newly reading README.md, naming-violation files, and full bodies rather than only header fields. MAJOR - inline-span masking rescanned the line remainder per backtick run, roughly O(n^1.6) on adversarial input: 1.76s for an 800KB line. Rewritten as a single linear pass pairing runs through forward-only per-length cursors. Same input now takes 3.31ms, with behavior unchanged. MINOR - an unreadable or broken entry threw, and the generic handler wrote a raw stack trace carrying absolute CI paths to stderr. The scan is now fault-tolerant and reports excluded entries as ordinary violations. The status vocabulary is escaped before being interpolated into a dynamic RegExp - defence-in-depth, not a live bug. The containment predicate had reached three hand-written copies while fixing this; it is now the single escapesRoot() helper used by all four call sites. * feat(#2704): add a --json report so the gate's tests assert on typed values Maintainer-directed addition. CONTRIBUTING.md's "Prohibited: Raw Text Matching on Test Outputs" requires that a system under test producing text also expose a structured intermediate representation, and that tests assert on that IR rather than on rendered prose. This gate had no such surface, so its verdict tests matched on stderr. --json runs exactly the same validation as --check and writes a report to stdout with the same exit code, following the frozen-REASON-enum pattern already used by verify-reapply-patches.cjs. Every violation carries a stable reason code plus the fields a consumer needs, so nothing has to pattern-match an error message. Adding a reason stays three coordinated changes - the enum, the emitting site, and the test locking Object.keys(REASON).sort(). The human output is unchanged, deliberately: a large pre-existing suite asserts on it and migrating that is not this PR's concern. Verified by running the pre-change and post-change scripts against an identical violating corpus and diffing their stderr - character-for-character identical. This PR's own verdict tests now assert on parsed --json. Absence checks improve the most: "no bracket violation" is now a reason-code predicate rather than a negative regex over prose, which could pass for the wrong reason. The security assertions were strengthened rather than translated - no leaked filename may appear in ANY field of the serialized report. Unknown flags are now rejected instead of silently falling through to printing the index. * test(#2704): fix the status-parity fixture and guard hooks/dist before overlay builds Two failures from the matrix run of 79b29909. The status-parity fixture was mine. It built, per status token, an ADR whose H1 bracket and Status field both carried that token - but Superseded carries an obligation beyond the bracket: it must name its successor as a file link and be symmetric with it. The fixture declared a bare Superseded, tripped that unrelated invariant, and the test reported a bracket-parity failure for a reason that had nothing to do with bracket parity. The fixture now satisfies each token's own obligations in both the agreeing and contradicting corpora, derived from the status actually declared rather than special-cased on one name, so a future token carrying obligations is handled rather than silently skipped. The second failure was not mine but is fixed here rather than deferred. mcp-catalog-parity.install.test.cjs hardlinks hooks/dist/* while building its overlay, but hooks/dist is a gitignored build artifact produced only by build:hooks. The suite had no guard, so it passed only when some other suite happened to build it first - an execution-order dependency, which is why it failed on node22 and passed on node24 for identical code. install.test.cjs already documents this exact hazard and guards it. Six behaviorally identical copies of that guard existed across three files. Rather than add a seventh, they are now one canonical tests/helpers/hooks-dist.cjs - idempotent and bounded by the shared BUILD_TIMEOUT_MS class norm - which is the same single-sourcing this PR applies to the ADR gate itself. * docs(#2704): add a how-to for contributors the ADR gate rejects The reference and explanation quadrants were covered by Lifecycle rules 5 and 6, but the task-oriented one was thin: a contributor meets this gate because it failed on their PR, under pressure, and the rules told them what is checked without telling them what to do about it. Adds the command to reproduce the CI failure locally and a message-to-remedy table covering every reason code that can be hit - unresolved target, wrong case with the did-you-mean hint, repository escape, symlinked ADR file, bracket contradiction - plus the backtick escape hatch for illustrative links and the caveat that indented code blocks are not skipped. The table is itself written in backticked inline code, so the gate skips it: the escape hatch demonstrated on the page that documents it. * chore(#2704): backfill changeset PR number pr:0 placeholder replaced with the real PR number now that #3266 exists. --------- Co-authored-by: sim --- .changeset/patient-foxes-click.md | 5 + docs/adr/README.md | 66 ++ scripts/gen-adr-index.cjs | 754 ++++++++++++++- tests/adr-index-gate.test.cjs | 1072 +++++++++++++++++++-- tests/helpers/hooks-dist.cjs | 35 + tests/install-minimal-hooks.test.cjs | 70 +- tests/install.test.cjs | 36 +- tests/mcp-catalog-parity.install.test.cjs | 13 +- 8 files changed, 1860 insertions(+), 191 deletions(-) create mode 100644 .changeset/patient-foxes-click.md create mode 100644 tests/helpers/hooks-dist.cjs diff --git a/.changeset/patient-foxes-click.md b/.changeset/patient-foxes-click.md new file mode 100644 index 000000000..bb37d775f --- /dev/null +++ b/.changeset/patient-foxes-click.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3266 +--- +**The ADR gate now resolves documentation links and checks H1 status brackets** — a link in `docs/adr/` that pointed nowhere, and an H1 whose trailing `[Status]` bracket contradicted its own `Status:` field, both passed CI green; readers and agents following those citations hit dead ends the build had already blessed. `gen-adr-index.cjs --check` now fails on either, naming the file, the line, and the unresolved target. Links inside fenced or inline code are left alone, and resolution is case-exact on every platform. A new `--json` flag reports the same findings as a structured document with stable `reason` codes, so tooling never has to pattern-match an error message. (#2704) diff --git a/docs/adr/README.md b/docs/adr/README.md index e210d6678..59258dd1c 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -84,6 +84,55 @@ Only an `Accepted` ADR is owed the back-link. A `Proposed` ADR's claim is **pros An H1 of `# ADR-0175: …` in a file named `218-*.md` is a rename that never finished. The id in the title must match the filename's prefix. +### 5. A trailing H1 status bracket must agree with the `Status` field + +Many ADRs restate their status in the H1 — `# ADR-1610: … [Accepted]`. That bracket is the first thing a reader sees, and the index strips it when rendering the title, so a stale one used to be invisible to everyone but the reader it misled. + +If the H1 ends in a bracket holding a status token, it must name the **same** status as the `Status` field. Comparison is case-insensitive and against the parsed *token*, so `[Superseded]` agrees with `Status: Superseded by [ADR-0174](0174-retire-gsd-sdk-package-boundary.md) (2026-05-23)`. + +A trailing bracket that is **not** a status token — `[Draft]`, `[WIP]` — is treated as part of the title and left alone. If you want a bracket the gate ignores, do not spell it like a status. + +### 6. Every relative link resolves + +A link whose target does not exist on disk fails the check, naming the file, the line, and the unresolved target. This covers every markdown file in this directory, including this README and any file whose name breaks the convention above. + +| Written as | Treated as | +|---|---| +| `[t](900-beta.md)`, `[t](../prd/)` | resolved — a directory counts | +| `[t](900-beta.md#section)` | the **file** is resolved; the `#fragment` is not checked | +| `[t](https://…)`, `[t](mailto:…)`, `[t](//host/x)` | out of scope — absolute destinations are never fetched | +| `[t](#lifecycle-rules)` | out of scope — a same-document anchor is not a file reference | +| `[t](/docs/adr/x.md)` | resolved against the repository root, as GitHub does | +| a link inside a ``` fence or `` `backticks` `` | **not a link** — markdown does not render one there, so it is never resolved | +| `[text][ref]` reference-style, ``, bare autolinks | not supported; write an inline link | + +Two consequences worth stating outright: + +- **Case matters, on every platform.** `[t](0001-Alpha.md)` pointing at `0001-alpha.md` fails even on macOS and Windows, because it 404s on github.com and reds the Linux CI lane. The failure names the entry it found so the fix is obvious. +- **A link to a generated or ignored path fails.** Nothing here consults `.gitignore`; the question is only whether a reader following the link lands somewhere. Cite the hand-authored source rather than the build artifact. + +### If the gate rejects something you wrote + +Reproduce it locally first — it is the same command CI runs, and it names the file, the line, and the target: + +```bash +node scripts/gen-adr-index.cjs --check +``` + +Then work from the `reason`: + +| What it says | What to do | +|---|---| +| `does not resolve — no such file or directory at …` | Fix the path. It is relative to `docs/adr/`, so a sibling ADR is just `900-slug.md`. If the target genuinely does not exist yet, drop the link rather than leaving it pointing nowhere. | +| `…Did you mean X? — link targets are case-sensitive on github.com` | Match the on-disk name exactly. Your machine may open the file regardless; github.com and the Linux CI lane will not. | +| `escapes the repository` | The path resolves outside the repo. Link something inside it, or use an absolute URL — those are out of scope and never checked. | +| `is a symlink that escapes the repository` | An ADR file itself is a symlink pointing outside the repo. Commit a real file. | +| `H1 status bracket […] contradicts the Status field (…)` | Update whichever of the two is stale so they agree. The `Status` field is authoritative; the bracket is a restatement for the reader. | + +**A link that is an example, not a destination, belongs in backticks.** The gate skips fenced blocks and inline code entirely, because markdown does not render a link there. That is the escape hatch for illustrative syntax — the table above is written that way, which is why it does not fail this check. An indented code block (four spaces) is *not* skipped; use backticks. + +To consume the result from a script rather than by eye, use `--json` (below) and branch on each violation's stable `reason` code. + ### Ratifying a stale `Proposed` A stale `Proposed` is not cosmetic: it tells contributors and agents that live architecture is an unbuilt idea. Fix it — but on evidence, not vibes. @@ -110,10 +159,27 @@ A stale `Proposed` is not cosmetic: it tells contributors and agents that live a node scripts/gen-adr-index.cjs # print the index node scripts/gen-adr-index.cjs --write # regenerate it into this file node scripts/gen-adr-index.cjs --check # CI: fail if stale or invalid +node scripts/gen-adr-index.cjs --json # same checks, machine-readable report ``` After adding an ADR, or changing any ADR's status or relations, run `--write` and commit the result. `npm run lint:generated-sync` runs `--check` in CI, so a missing or stale row fails the build rather than rotting silently. +`--json` runs the same validation as `--check` and writes a report to stdout instead of prose to stderr, with the same exit code. Each violation carries a stable `reason` code, so a tool consuming this never has to pattern-match an error message: + +```json +{ + "ok": false, + "adrCount": 76, + "indexStale": false, + "violations": [ + { "file": "2704-example.md", "line": 41, "reason": "link_unresolved", + "target": "reference/x.md", "resolved": "docs/adr/reference/x.md" } + ] +} +``` + +An unrecognized flag is rejected rather than ignored. + This replaces a hand-maintained table that had drifted to **40 of 65 ADRs** — the entire capability family and EoS itself were missing from it, which is precisely why the ADRs a reader most needed were the ones they could not find. ## Index diff --git a/scripts/gen-adr-index.cjs b/scripts/gen-adr-index.cjs index aaec6c116..7947f8399 100644 --- a/scripts/gen-adr-index.cjs +++ b/scripts/gen-adr-index.cjs @@ -23,6 +23,7 @@ * node scripts/gen-adr-index.cjs # print the index to stdout * node scripts/gen-adr-index.cjs --write # rewrite the index in README.md * node scripts/gen-adr-index.cjs --check # exit 1 if stale or invalid + * node scripts/gen-adr-index.cjs --json # --check semantics; JSON report on stdout */ const fs = require('node:fs'); @@ -54,6 +55,61 @@ const END_MARKER = ''; */ const STATUSES = ['Accepted', 'Proposed', 'Superseded', 'Legacy', 'Retired']; +/** + * Stable reason codes for every lifecycle violation this gate can emit. + * Tests assert via `assert.equal(record.reason, REASON.X)` (or `.some(...)` + * over the `--json` `violations` array) rather than regex-matching stderr + * prose — see CONTRIBUTING.md "Prohibited: Raw Text Matching on Test + * Outputs" and the worked example in `bin/verify-reapply-patches.cjs`. + * + * Adding a reason is a deliberate three-part change: a new entry here, the + * emitting `add(...)` call site, and the corpus test that locks + * `Object.keys(REASON).sort()` — so a new violation class cannot ship + * without its own typed identity. + */ +const REASON = Object.freeze({ + FILENAME_INVALID: 'filename_invalid', + STATUS_MISSING: 'status_missing', + STATUS_INVALID: 'status_invalid', + STATUS_BRACKET_MISMATCH: 'status_bracket_mismatch', + ID_MISMATCH: 'id_mismatch', + SUPERSEDED_NO_SUCCESSOR: 'superseded_no_successor', + SUPERSEDED_BARE_ID: 'superseded_bare_id', + RELATION_LINK_MISSING: 'relation_link_missing', + RELATION_BARE_ID_MISSING: 'relation_bare_id_missing', + RELATION_BARE_ID_UNLINKED: 'relation_bare_id_unlinked', + RELATION_ASYMMETRIC: 'relation_asymmetric', + LINK_UNRESOLVED: 'link_unresolved', + LINK_ESCAPES_REPO: 'link_escapes_repo', + LINK_ESCAPES_REPO_SYMLINK: 'link_escapes_repo_symlink', + DIRENT_UNREADABLE: 'dirent_unreadable', + DIRENT_ESCAPES_REPO_SYMLINK: 'dirent_escapes_repo_symlink', +}); + +/** + * The H1 trailing-bracket vocabulary, derived from `STATUSES` — not a second + * hand-written literal. Before this PR, `parseAdr`'s title strip carried its + * own copy of these five tokens, and nothing asserted the two lists agreed: + * a textbook `DEFECT.GENERATIVE-FIX` instance (a generated surface and its + * hand-authored source drifting apart with no parity check). A 6th status + * added to `STATUSES` now covers the bracket for free, and the corpus's + * parity test iterates the real exported array rather than a copy. + */ +/** + * Escape a string for literal inclusion inside a dynamic RegExp alternation. + * Defence-in-depth, not a live-bug fix: `STATUSES` is a static array literal + * today, so nothing in it can currently carry a regex metacharacter. But + * nothing enforces that it STAYS static — if a future change ever derives it + * from external input (a config file, a corpus scan), an unescaped `join('|')` + * would let a status token break out of the alternation it is meant to be one + * branch of. + */ +function escapeRegExp(s) { + return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +const STATUS_BRACKET_RE = new RegExp(String.raw`\s*\[(${STATUSES.map(escapeRegExp).join('|')})\]\s*$`, 'i'); + /** * Header fields that assert a lifecycle relation. * @@ -113,14 +169,116 @@ function canonicalId(raw) { /** The documented filename shape: `-.md`. */ const ADR_FILENAME_RE = /^[0-9]+-[a-z0-9-]+\.md$/; -function adrFiles() { - return fs - .readdirSync(ADR_DIR) - .filter((f) => f.endsWith('.md') && f !== 'README.md') - .filter((f) => fs.statSync(path.join(ADR_DIR, f)).isFile()) +/** + * Whole-segment containment test: true if `abs` is NOT inside `root`. + * + * The SINGLE copy of this predicate. Before this PR it was hand-written + * inline in two places (the lexical pre-stat check in `validateLinks` and the + * post-realpath escape check in `existsCaseExact`) with no shared name; a + * third copy for `markdownFilesInAdrDir`'s own symlink check would have made + * three. All three call sites now share this one function. + * + * `rel.startsWith('..')` alone would also match an in-repo path whose first + * segment merely BEGINS with two dots (`..hidden.md`), and a false "escapes + * the repository" on a valid path is a worse failure than a miss — hence the + * whole-segment `rel === '..' || rel.startsWith('..' + sep)` form. + */ +function escapesRoot(abs, root) { + const rel = path.relative(root, abs); + return rel === '..' || rel.startsWith(`..${path.sep}`) || path.isAbsolute(rel); +} + +/** + * `fs.realpathSync(ROOT)`, tolerant of an unreadable/vanished ROOT (degrades + * to the lexical ROOT itself rather than throwing — callers still get a + * usable comparison root, just without symlink-normalization on hosts where + * ROOT itself sits under a symlinked ancestor, e.g. macOS's /var -> /private/var). + */ +function realRootOrFallback() { + try { + return fs.realpathSync(ROOT); + } catch { + return ROOT; + } +} + +/** + * Whether `joined` (a `*.md` dirent directly under docs/adr/) should be + * treated as an ADR file: a regular file, or a symlink that resolves to a + * regular file WITHOUT leaving the repository. Applies the same rule to which + * FILES are read as `validateLinks` already applies to which link TARGETS + * resolve — a symlink escaping the repo is never followed and its content is + * never touched (no `statSync`/`readFileSync` past the `lstatSync`/ + * `realpathSync` calls below), because `parseAdr` and `validateLinks` both + * read the FULL body of every accepted file and echo fragments into stderr. + */ +function isAcceptedAdrEntry(joined, realRoot) { + let lst; + try { + lst = fs.lstatSync(joined); + } catch { + return false; // vanished / unreadable + } + if (lst.isFile()) return true; + if (!lst.isSymbolicLink()) return false; + + let real; + try { + real = fs.realpathSync(joined); + } catch { + return false; // broken symlink + } + if (escapesRoot(real, realRoot)) return false; // escapes the repository + + try { + return fs.statSync(real).isFile(); + } catch { + return false; // vanished between realpath and stat (TOCTOU) + } +} + +/** + * Every markdown file directly under docs/adr/, README.md included. + * + * Single source of the traversal rule: `adrFiles()` is this minus README.md + * (which is the index, not an ADR), and the link-resolution pass is this + * unfiltered (the generated index can point nowhere too). Two hand-copied + * readdir filters would drift the moment either grew a rule — the exact + * DEFECT.GENERATIVE-FIX shape this gate now enforces against the corpus. + */ +function markdownFilesInAdrDir() { + let entries; + try { + entries = fs.readdirSync(ADR_DIR); + } catch { + // An unreadable docs/adr/ itself degrades to "no files" here — the caller + // (validate/validateLinks) surfaces the real problem elsewhere; this + // traversal helper must never throw a raw fs error up into `runMain`, + // which would print `err.stack` (absolute host paths) to public CI logs. + entries = []; + } + const realRoot = realRootOrFallback(); + return entries + .filter((f) => f.endsWith('.md')) + .filter((f) => { + try { + return isAcceptedAdrEntry(path.join(ADR_DIR, f), realRoot); + } catch { + // A `*.md` dirent that cannot be classified — most commonly a broken + // symlink — is excluded here rather than crashing the caller. It is + // not silently dropped from the gate: `validateLinks` diffs this + // filtered list against the raw `readdirSync` listing and reports + // the exclusion as its own violation, naming the file. + return false; + } + }) .sort(); } +function adrFiles() { + return markdownFilesInAdrDir().filter((f) => f !== 'README.md'); +} + /** * Split the directory into files this tool can parse and files it cannot. * @@ -192,6 +350,409 @@ function bareAdrRefs(text) { return out; } +/** + * Mask code (fenced blocks and inline spans) so link resolution never reads a + * `[…](…)` sequence that markdown does not render as a link. The corpus has + * two real examples of this: `mod[entry.router]({ args, cwd, raw, error })` + * inside a ``` fence, and `` `require(module)[router]()` `` inline — both + * ordinary JavaScript, neither a link. + * + * The output is the SAME LENGTH as the input, with every masked character + * replaced by a single space and every newline left untouched — so a finding + * computed against the masked text still names the correct 1-indexed line + * (Kernighan's Law: keep the debug surface honest rather than deleting text). + */ +function maskCode(text) { + // Capturing split keeps the line terminators as their own array elements + // (even indices are line content, odd indices are the terminator that + // followed), so the rebuild below never has to guess LF vs CRLF. + const parts = String(text).split(/(\r?\n)/); + + // null outside a fence; otherwise the marker char ('`' or '~') and the + // length of the run that opened it — both are load-bearing for closing: + // only the SAME char with a run length >= the opener's closes the fence. + let fence = null; + + for (let i = 0; i < parts.length; i += 2) { + const line = parts[i]; + + if (fence) { + // Whichever way this line resolves, it is code: the closing fence line + // is still a fence delimiter, not prose. + const closeRe = fence.char === '`' ? /^ {0,3}(`{3,})\s*$/ : /^ {0,3}(~{3,})\s*$/; + const close = line.match(closeRe); + parts[i] = ' '.repeat(line.length); + if (close && close[1].length >= fence.len) fence = null; + continue; + } + + const open = line.match(/^ {0,3}(`{3,}|~{3,})/); + if (open) { + fence = { char: open[1][0], len: open[1].length }; + parts[i] = ' '.repeat(line.length); + continue; + } + + parts[i] = maskInlineCodeSpans(line); + } + + return parts.join(''); +} + +/** + * Mask backtick-delimited inline code spans within a single line (fences are + * handled by the caller, per-line, before this runs — a span never crosses a + * newline). CommonMark's rule: a run of N backticks opens a span, closed by + * the NEXT run of exactly N backticks; a run of any other length in between + * is part of the span's content, not a delimiter. An opening run with no + * matching close is literal text, not a span. + * + * LINEAR, not the naive per-opener rescan this replaced: the old + * implementation, for every backtick run, rescanned the entire remainder of + * the line looking for a same-length closer. A line of strictly-ascending- + * length backtick runs (nothing ever closes) forced a near-full rescan per + * run — measured ~O(n^1.6) and unbounded (34ms/50KB -> 220ms/200KB -> + * 1.76s/800KB on adversarial input). This version scans the line ONCE to + * collect every backtick run as `{start, end, len}`, then walks that run + * list left to right with a per-length cursor (`byLen`/`cursor` below) that + * only ever advances forward — so finding "the next run of equal length" is + * amortized O(1) per step and the whole pass is O(line length). + * + * Behavior is identical to the rescan version for every input: once an + * opener at run `r` is paired with the next same-length run `r'`, every run + * strictly between them is consumed as span content and is never + * reconsidered as its own delimiter — exactly what the old code did by + * jumping `i` straight to the close and never revisiting the interior. + */ +function maskInlineCodeSpans(line) { + const runs = []; + let i = 0; + while (i < line.length) { + if (line[i] !== '`') { + i += 1; + continue; + } + const start = i; + while (i < line.length && line[i] === '`') i += 1; + runs.push({ start, end: i, len: i - start }); + } + if (runs.length === 0) return line; + + // Every run's index, grouped by length, in left-to-right order (already + // sorted — `runs` was built in scan order). + const byLen = new Map(); + for (let idx = 0; idx < runs.length; idx += 1) { + const len = runs[idx].len; + if (!byLen.has(len)) byLen.set(len, []); + byLen.get(len).push(idx); + } + const cursor = new Map(); // len -> next unexamined index into byLen.get(len) + + const spans = []; // [start, end) ranges to mask, in order, non-overlapping + let r = 0; + while (r < runs.length) { + const len = runs[r].len; + const candidates = byLen.get(len); + let c = cursor.get(len) || 0; + // Skip past any candidate at or before `r`: `r` itself, or an index + // already consumed as interior content of an earlier matched span (a run + // inside a completed span is never revisited — same as the rescan + // version never re-examining a delimiter it has already masked over). + while (c < candidates.length && candidates[c] <= r) c += 1; + if (c < candidates.length) { + const closeIdx = candidates[c]; + spans.push([runs[r].start, runs[closeIdx].end]); + cursor.set(len, c + 1); + r = closeIdx + 1; + } else { + cursor.set(len, c); + r += 1; // no closer of equal length anywhere ahead — literal text + } + } + + let out = ''; + let pos = 0; + for (const [s, e] of spans) { + out += line.slice(pos, s) + ' '.repeat(e - s); + pos = e; + } + out += line.slice(pos); + return out; +} + +/** + * Every inline `[text](dest)` / `![alt](dest)` link or image in `text`, with + * code masked out first so a code-shaped bracket/paren sequence is never + * misread as a link (see `maskCode`). + * + * `[^\][\n]*` for the link-text class deliberately excludes BOTH bracket + * characters, not just `]` — so `[see [1]](x.md)` does not match (nested + * brackets are out of the inline-links-only scope this gate supports) and a + * regex character class in prose like `[A-Z][A-Z0-9_]` cannot be misread as + * one either. Reference-style links (`[text][ref]`) are correspondingly not + * supported: the corpus has zero reference definitions to resolve against. + * + * Returns `{ line, target }` per match — `line` is 1-indexed, `target` is the + * RAW parenthesized capture, untrimmed and unresolved; callers normalize. + */ +function extractLinks(text) { + const masked = maskCode(String(text)); + const lines = masked.split(/\r?\n/); + const out = []; + const re = /!?\[[^\][\n]*\]\(([^()\n]*)\)/g; + for (let i = 0; i < lines.length; i += 1) { + re.lastIndex = 0; + let m; + while ((m = re.exec(lines[i])) !== null) { + out.push({ line: i + 1, target: m[1] }); + } + } + return out; +} + +/** + * Case-exact existence of `abs` (which MUST already be verified inside ROOT + * by the caller — LEXICALLY, via `path.relative`). Walks each path segment + * against a cached, real `readdirSync` listing of its parent rather than + * calling `fs.existsSync(abs)` directly: existsSync resolves through the OS's + * case-folding rules, which pass on macOS/Windows for a link that 404s on + * github.com and reds the Linux CI lane — every platform must agree, so + * resolution never trusts the filesystem's own case sensitivity (or lack + * of it). + * + * SYMLINK ESCAPE (the reason this function is more than a readdir loop): the + * caller's containment check is purely lexical string math on `abs` — it + * proves nothing about what is actually ON DISK at each segment. But + * `readdirSync` FOLLOWS symlinks at the OS level while walking further down a + * path. A contributor can commit `docs/adr/x -> /etc` (a symlink; Linux CI + * lanes, including fork PRs, preserve symlinks) plus an ADR linking + * `[t](x/passwd)`: the caller's lexical check sees `docs/adr/x/passwd`, which + * LOOKS repo-internal, and this walk would then list the real external + * directory — and the "Did you mean X?" hint below is built from exactly that + * listing, so a wrong-case probe (`[t](x/PASSWD)`) would echo a real filename + * from OUTSIDE the repo into PUBLIC CI LOGS on a fork PR. So every segment is + * lstat'd, and a symlink is realpath'd and re-checked against the REAL root, + * BEFORE this walk ever descends into or reads what it points at. + * + * `dirCache` is a Map|null> (null = unreadable), + * built and owned by the caller so repeated links into the same directory + * cost one `readdirSync` total, not one per link. + * + * `realRoot` is `fs.realpathSync(ROOT)`, computed ONCE by the caller (never + * per-segment/per-link here) and passed in — ROOT itself may sit under a + * symlinked path (macOS's `/var` -> `/private/var`), so comparing a + * realpath'd descendant against a non-realpath'd ROOT would misclassify every + * legitimate path on such a host as an escape. + * + * Returns `{ exists, hint, escaped }`. `escaped: true` means a symlink + * resolved outside `realRoot`; in that case `exists` is `false` and `hint` is + * ALWAYS `null` — the caller must report a distinct "escapes" message and + * never fall back to the generic "does not resolve" wording or a hint, both + * of which would leak into the escape's own disclosure hazard. + */ +function existsCaseExact(abs, dirCache, realRoot) { + const rel = path.relative(ROOT, abs); + if (rel === '') return { exists: true, hint: null, escaped: false }; // ROOT itself + + const segments = rel.split(path.sep); + let dir = ROOT; + for (const seg of segments) { + let entries = dirCache.get(dir); + if (entries === undefined) { + try { + entries = new Set(fs.readdirSync(dir)); + } catch { + entries = null; + } + dirCache.set(dir, entries); + } + if (!entries || !entries.has(seg)) { + const hint = entries ? [...entries].find((e) => e.toLowerCase() === seg.toLowerCase()) : null; + return { exists: false, hint: hint || null, escaped: false }; + } + + const joined = path.join(dir, seg); + + // `entries.has(seg)` above proved a directory ENTRY named `seg` exists — + // it says nothing about what that entry IS. Check before descending. + let lst; + try { + lst = fs.lstatSync(joined); + } catch { + // Vanished between readdir and lstat (TOCTOU race) — degrade to "does + // not resolve", never throw. + return { exists: false, hint: null, escaped: false }; + } + + if (lst.isSymbolicLink()) { + let real; + try { + real = fs.realpathSync(joined); + } catch { + // Broken symlink — degrade to "does not resolve", never throw. + return { exists: false, hint: null, escaped: false }; + } + if (escapesRoot(real, realRoot)) { + // No further readdirSync down this path, and no hint: both would + // disclose facts about a directory outside the repo. + return { exists: false, hint: null, escaped: true }; + } + dir = real; // resolves inside the repo — continue the walk from there. + continue; + } + + dir = joined; + } + return { exists: true, hint: null, escaped: false }; +} + +/** + * The link-resolution pass: every inline link/image target in every `*.md` + * file directly under `docs/adr/` — INCLUDING README.md (the generated index + * can point nowhere too) and files that fail the naming convention (their + * naming violation is reported separately by `partitionAdrFiles`, but a + * reader still follows their links). Non-recursive, matching `adrFiles()`. + * + * Errors are reported through the same `add(file, msg)` channel `validate` + * uses elsewhere, keeping the `${file}: ${msg}` prefix uniform — but the + * "file" half of that prefix is `${file}:${line}` here, so the emitted line + * reads `:: ` (a literal colon immediately before the line + * number, compiler-diagnostic style) rather than `: : `. + */ +function validateLinks(add) { + const files = markdownFilesInAdrDir(); + + // Computed ONCE per pass, never per-link/per-dirent: see existsCaseExact's + // doc comment for why comparing against the REAL root (not the lexical + // ROOT constant) is required to avoid false escapes when the repo checkout + // itself sits under a symlinked ancestor (e.g. macOS's /var -> /private/var). + const realRoot = realRootOrFallback(); + + // Report any `*.md` dirent that `markdownFilesInAdrDir` silently excluded — + // because it could not be stat'd (e.g. a broken symlink) OR because it IS a + // symlink that resolves outside the repository — so it surfaces as a gate + // finding instead of quietly vanishing from the index. Reading the + // directory again here (rather than threading a second return value + // through `markdownFilesInAdrDir`) keeps that function's contract + // (`string[]`) simple for its other callers. Wrapped in try/catch for the + // same reason as inside `markdownFilesInAdrDir`: an unreadable ADR_DIR + // degrades to "nothing more to report" here, never a crash. + let dirents; + try { + dirents = fs.readdirSync(ADR_DIR); + } catch { + dirents = []; + } + const included = new Set(files); + const BROKEN_MSG = 'could not be read (broken symlink?) and was excluded from the index. Remove it or fix its target.'; + for (const f of dirents) { + if (!f.endsWith('.md') || included.has(f)) continue; + const joined = path.join(ADR_DIR, f); + let lst; + try { + lst = fs.lstatSync(joined); + } catch { + add(f, BROKEN_MSG, { reason: REASON.DIRENT_UNREADABLE, line: null }); + continue; + } + if (!lst.isSymbolicLink()) { + // Not a symlink and still excluded — some other legitimate reason + // (e.g. it's a directory literally named `*.md`), not unreadable. + continue; + } + let real; + try { + real = fs.realpathSync(joined); + } catch { + add(f, BROKEN_MSG, { reason: REASON.DIRENT_UNREADABLE, line: null }); + continue; + } + if (escapesRoot(real, realRoot)) { + // Distinct message from the broken-symlink one above, and — same + // discipline as the link-target escape below — no path or hint from + // outside the repo is ever included: only the in-repo dirent name. + add( + f, + 'is a symlink that escapes the repository and was excluded from the index. Point it at a file inside docs/adr/, or remove it.', + { reason: REASON.DIRENT_ESCAPES_REPO_SYMLINK, line: null }, + ); + continue; + } + // Resolves inside the repo but is not a regular file (e.g. a symlink to + // a directory) — a legitimate exclusion, not a disclosure hazard. + } + + const dirCache = new Map(); + + for (const file of files) { + const text = fs.readFileSync(path.join(ADR_DIR, file), 'utf8'); + + for (const { line, target: rawTarget } of extractLinks(text)) { + let t = String(rawTarget).trim(); + + if (t.startsWith('<') && t.endsWith('>')) { + t = t.slice(1, -1).trim(); + } else { + // A link title: `dest "Title"` / `dest 'Title'`. Drop it, keep dest. + const titled = t.match(/^(\S+)\s+(?:"[^"]*"|'[^']*')$/); + if (titled) t = titled[1]; + } + + if (t === '' || t.startsWith('#') || t.startsWith('//') || /^[a-z][a-z0-9+.-]*:/i.test(t)) { + continue; // empty, same-document anchor, protocol-relative, or any URI scheme — out of scope + } + + t = t.split('#')[0]; + if (t === '') continue; // was only a fragment + + try { + t = decodeURIComponent(t); + } catch { + // Malformed escape (e.g. "%zz"): resolve the raw, non-decoded text + // rather than throwing — an unresolvable literal is still reportable. + } + + const abs = t.startsWith('/') ? path.resolve(ROOT, t.slice(1)) : path.resolve(ADR_DIR, t); + + // Containment BEFORE any filesystem call: never `stat` outside ROOT. + const rel = path.relative(ROOT, abs); + if (escapesRoot(abs, ROOT)) { + add(`${file}:${line}`, `link "${rawTarget}" escapes the repository. Link a path inside the repo.`, { + reason: REASON.LINK_ESCAPES_REPO, + file, + line, + target: rawTarget, + }); + continue; + } + + const { exists, hint, escaped } = existsCaseExact(abs, dirCache, realRoot); + if (escaped) { + // A symlink under the (lexically in-repo) target path resolves + // outside the repository. Distinct message from the generic + // "does not resolve" below, and — deliberately — no hint: the hint + // itself would be the disclosure (see existsCaseExact's doc comment). + add(`${file}:${line}`, `link "${rawTarget}" escapes the repository via a symlink. Link a path inside the repo.`, { + reason: REASON.LINK_ESCAPES_REPO_SYMLINK, + file, + line, + target: rawTarget, + }); + continue; + } + if (!exists) { + const relFromRoot = rel.split(path.sep).join('/'); + const hintSuffix = hint ? ` Did you mean ${hint}? — link targets are case-sensitive on github.com.` : ''; + add( + `${file}:${line}`, + `link "${rawTarget}" does not resolve — no such file or directory at ${relFromRoot}.${hintSuffix}`, + { reason: REASON.LINK_UNRESOLVED, file, line, target: rawTarget, resolved: relFromRoot }, + ); + } + } + } +} + function parseAdr(file) { const full = path.join(ADR_DIR, file); const text = fs.readFileSync(full, 'utf8'); @@ -205,12 +766,19 @@ function parseAdr(file) { const displayId = rawId; const h1 = (lines.find((l) => /^#\s/.test(l)) || '').replace(/^#\s+/, '').trim(); + + // Capture the trailing status bracket against the RAW h1, before the title + // strip below discards it. This is the value the H1-vs-Status comparison in + // `validate` checks against — the strip alone throws the information away. + const bracketMatch = h1.match(STATUS_BRACKET_RE); + const bracketStatus = bracketMatch ? bracketMatch[1] : null; + // Title as displayed: drop a leading "ADR-123 — " / "ADR-123: " prefix and a // trailing "[Proposed]"-style status bracket, both of which the index renders // from structured fields instead. const title = h1 .replace(/^ADR[-\s]?0*\d+\s*(?:[—:-]\s*)?/i, '') - .replace(/\s*\[(?:Proposed|Accepted|Superseded|Legacy|Retired)\]\s*$/i, '') + .replace(STATUS_BRACKET_RE, '') .trim(); const declaredIdMatch = h1.match(/^ADR[-\s]?0*(\d+)\b/i); @@ -252,7 +820,7 @@ function parseAdr(file) { relations[kind].out.push({ field: `## ${kind === 'supersedes' ? 'Supersedes' : 'Subsumes'} section`, value: '', links: sections[kind], bare: [] }); } - return { file, fileId, displayId, title, declaredId, statusRaw, statusToken, relations, text }; + return { file, fileId, displayId, title, declaredId, statusRaw, statusToken, bracketStatus, relations, text }; } function buildCorpus() { @@ -269,26 +837,64 @@ function buildCorpus() { function validate({ adrs, byFile, byId, nonConforming }) { const errors = []; - const add = (file, msg) => errors.push(`${file}: ${msg}`); + const violations = []; + // `record` carries the STRUCTURED half of every violation — `reason` plus + // whatever typed fields a `--json` consumer needs (line, target, resolved, + // expected/actual, status, …) — kept alongside, never instead of, the + // human `${file}: ${msg}` line: a large pre-existing test suite asserts on + // that string verbatim and is out of scope to migrate. `record` may supply + // its own `file` (spread AFTER the outer `file`) so link-violation records + // carry the plain filename as a field distinct from the human message's + // `:` prefix string. + const add = (file, msg, record) => { + errors.push(`${file}: ${msg}`); + violations.push({ file, ...record }); + }; for (const f of nonConforming) { add( f, 'filename does not match the `-.md` convention, so it cannot appear in the index. ' + 'Rename it (see docs/adr/README.md "Naming Convention"), or move it out of docs/adr/ if it is not an ADR.', + { reason: REASON.FILENAME_INVALID, line: null }, ); } for (const a of adrs) { if (!a.statusToken) { - add(a.file, 'no `- **Status:** ` field found in the header block.'); + add(a.file, 'no `- **Status:** ` field found in the header block.', { + reason: REASON.STATUS_MISSING, + line: null, + }); continue; } if (!STATUSES.includes(a.statusToken)) { - add(a.file, `status "${a.statusToken}" is not one of ${STATUSES.join(' | ')} (full line: "${a.statusRaw}").`); + add(a.file, `status "${a.statusToken}" is not one of ${STATUSES.join(' | ')} (full line: "${a.statusRaw}").`, { + reason: REASON.STATUS_INVALID, + line: null, + status: a.statusToken, + }); + } else if ( + // Only compare when the status token is itself valid — an already-invalid + // token gets its own report above, and piling a bracket-disagreement + // message on top of it would be a second complaint about the same defect. + a.bracketStatus && + a.bracketStatus.toLowerCase() !== a.statusToken.toLowerCase() + ) { + add( + a.file, + `H1 status bracket [${a.bracketStatus}] contradicts the Status field (${a.statusToken}). ` + + 'Update the H1 bracket (or the Status field) so they agree — a stale bracket is the first thing a reader sees.', + { reason: REASON.STATUS_BRACKET_MISMATCH, line: null, expected: a.statusToken, actual: a.bracketStatus }, + ); } if (a.declaredId && a.declaredId !== a.fileId) { - add(a.file, `H1 declares ADR-${a.declaredId} but the filename says ${a.fileId}. The id must match the filename.`); + add(a.file, `H1 declares ADR-${a.declaredId} but the filename says ${a.fileId}. The id must match the filename.`, { + reason: REASON.ID_MISMATCH, + line: null, + expected: a.fileId, + actual: a.declaredId, + }); } // A Superseded ADR must point at its successor by FILE LINK. @@ -296,13 +902,19 @@ function validate({ adrs, byFile, byId, nonConforming }) { const links = a.relations.supersedes.in.flatMap((r) => r.links); if (links.length === 0) { const bare = a.relations.supersedes.in.flatMap((r) => r.bare); - add( - a.file, - bare.length - ? `status is Superseded and mentions ADR-${bare.join('/')} but not as a markdown link to the file. ` + - 'Bare ids are ambiguous (ADR-0010 and ADR-0011 each resolve to multiple files) — link the target file.' - : 'status is Superseded but names no successor. Write `Superseded by [ADR-N](N-slug.md)`.', - ); + if (bare.length) { + add( + a.file, + `status is Superseded and mentions ADR-${bare.join('/')} but not as a markdown link to the file. ` + + 'Bare ids are ambiguous (ADR-0010 and ADR-0011 each resolve to multiple files) — link the target file.', + { reason: REASON.SUPERSEDED_BARE_ID, line: null, bare }, + ); + } else { + add(a.file, 'status is Superseded but names no successor. Write `Superseded by [ADR-N](N-slug.md)`.', { + reason: REASON.SUPERSEDED_NO_SUCCESSOR, + line: null, + }); + } } } @@ -311,7 +923,14 @@ function validate({ adrs, byFile, byId, nonConforming }) { for (const dir of ['out', 'in']) { for (const rel of a.relations[kind][dir]) { for (const l of rel.links) { - if (!byFile.has(l)) add(a.file, `"${rel.field}" links "${l}", which does not exist in docs/adr/.`); + if (!byFile.has(l)) { + add(a.file, `"${rel.field}" links "${l}", which does not exist in docs/adr/.`, { + reason: REASON.RELATION_LINK_MISSING, + line: null, + field: rel.field, + target: l, + }); + } } // The synthetic relation lifted out of the Status line is already covered by // the dedicated Superseded check above; reporting it again just duplicates. @@ -328,13 +947,24 @@ function validate({ adrs, byFile, byId, nonConforming }) { if (linkedIds.has(b)) continue; const candidates = byId.get(b) || []; if (candidates.length === 0) { - add(a.file, `"${rel.field}" names ADR-${b}, which does not exist in docs/adr/. If it is an ISSUE number, write "#${b}" — not "ADR-${b}".`); + add( + a.file, + `"${rel.field}" names ADR-${b}, which does not exist in docs/adr/. If it is an ISSUE number, write "#${b}" — not "ADR-${b}".`, + { reason: REASON.RELATION_BARE_ID_MISSING, line: null, field: rel.field, target: b }, + ); } else { add( a.file, `"${rel.field}" names ADR-${b} without a file link` + (candidates.length > 1 ? ` (ambiguous — resolves to ${candidates.length} files: ${candidates.map((c) => c.file).join(', ')})` : '') + '. Link the target file so the relation is checkable.', + { + reason: REASON.RELATION_BARE_ID_UNLINKED, + line: null, + field: rel.field, + target: b, + candidates: candidates.map((c) => c.file), + }, ); } } @@ -376,13 +1006,19 @@ function validate({ adrs, byFile, byId, nonConforming }) { target, `${a.file} declares ${claim}, but this ADR does not record it. ` + `Add \`- **${needed}:** [ADR-${a.displayId}](${a.file})\` so a reader of THIS file learns the decision moved on.`, + { reason: REASON.RELATION_ASYMMETRIC, line: null, source: a.file, kind, neededField: needed }, ); } } } } - return errors; + // Link resolution reads the directory directly rather than the parsed + // `adrs` list — it must ALSO cover README.md and naming-violation files, + // neither of which is in `adrs` (see `validateLinks`'s own doc comment). + validateLinks(add); + + return { errors, violations }; } const GROUPS = [ @@ -498,13 +1134,52 @@ function spliceIntoReadme(readme, index) { return readme.slice(0, start) + index + readme.slice(end + END_MARKER.length); } +/** + * Parse CLI flags from `argv` (already sliced to just the flags, i.e. + * `process.argv.slice(2)`). Supports `--write`, `--check`, `--json` in any + * order. FAIL-CLOSED on an unrecognized flag: silently falling through to + * the no-flags "print the index" behavior would mask a typo (e.g. + * `--jsno`) as a clean run instead of failing loudly, so any argument that + * is not one of the three recognized flags throws `ExitError(1, …)` naming + * the offender rather than being ignored. + */ +function parseArgs(argv) { + const opts = { write: false, check: false, json: false }; + for (const arg of argv) { + if (arg === '--write') opts.write = true; + else if (arg === '--check') opts.check = true; + else if (arg === '--json') opts.json = true; + else throw new ExitError(1, `unknown flag: ${arg}\nRecognized flags: --write, --check, --json.`); + } + return opts; +} + function main() { - const [, , flag] = process.argv; + const { write, check, json } = parseArgs(process.argv.slice(2)); const corpus = buildCorpus(); - const errors = validate(corpus); + const { errors, violations } = validate(corpus); + const index = renderIndex(corpus); - if (errors.length > 0 && flag !== '--write') { + if (json) { + // `--json` implies `--check` semantics (`--check --json` is identical to + // `--json` alone) but emits a single JSON document to stdout instead of + // the human stderr report, and writes nothing to stderr at all. Unlike + // the human `--check` path below — which short-circuits on lifecycle + // violations and never even reads README.md to check staleness — the + // JSON report always computes BOTH facts (`violations` and + // `indexStale`) independently, since a consumer parsing the document + // needs the complete picture in one shot rather than one violation + // class masking the other. + const readme = fs.readFileSync(README_PATH, 'utf8'); + const expected = spliceIntoReadme(readme, index); + const indexStale = expected !== readme; + const ok = violations.length === 0 && !indexStale; + process.stdout.write(JSON.stringify({ ok, adrCount: corpus.adrs.length, indexStale, violations }) + '\n'); + return ok ? 0 : 1; + } + + if (errors.length > 0 && !write) { process.stderr.write( `docs/adr/ has ${errors.length} lifecycle violation(s).\n` + 'See docs/adr/README.md "Lifecycle rules" for the contract.\n\n', @@ -514,9 +1189,19 @@ function main() { throw new ExitError(1); } - const index = renderIndex(corpus); - - if (flag === '--check') { + // `--write` takes precedence over a co-supplied `--check`: neither + // combination is part of this CLI's documented contract (the flags exist + // to be used one at a time, or as `--check --json`), so this is an + // arbitrary-but-safe tiebreak rather than a specified behavior. + if (write) { + const readme = fs.readFileSync(README_PATH, 'utf8'); + fs.writeFileSync(README_PATH, spliceIntoReadme(readme, index)); + process.stdout.write(`Wrote ADR index into ${README_PATH} (${corpus.adrs.length} ADRs).\n`); + if (errors.length > 0) { + process.stderr.write(`\n${errors.length} lifecycle violation(s) remain — --check will fail:\n\n`); + for (const e of errors) process.stderr.write(` ✗ ${e}\n`); + } + } else if (check) { const readme = fs.readFileSync(README_PATH, 'utf8'); const expected = spliceIntoReadme(readme, index); if (expected !== readme) { @@ -526,17 +1211,14 @@ function main() { throw new ExitError(1); } process.stdout.write(`docs/adr/README.md index is up to date (${corpus.adrs.length} ADRs).\n`); - } else if (flag === '--write') { - const readme = fs.readFileSync(README_PATH, 'utf8'); - fs.writeFileSync(README_PATH, spliceIntoReadme(readme, index)); - process.stdout.write(`Wrote ADR index into ${README_PATH} (${corpus.adrs.length} ADRs).\n`); - if (errors.length > 0) { - process.stderr.write(`\n${errors.length} lifecycle violation(s) remain — --check will fail:\n\n`); - for (const e of errors) process.stderr.write(` ✗ ${e}\n`); - } } else { process.stdout.write(index + '\n'); } } -runMain(main); +// Guarded: `require`-ing this module (the test suite imports STATUSES and +// the pure scanner directly) must not also run the generator as a side +// effect of loading it. +if (require.main === module) runMain(main); + +module.exports = { STATUSES, REASON, extractLinks, maskCode }; diff --git a/tests/adr-index-gate.test.cjs b/tests/adr-index-gate.test.cjs index 760120f58..d35e09664 100644 --- a/tests/adr-index-gate.test.cjs +++ b/tests/adr-index-gate.test.cjs @@ -69,6 +69,27 @@ function run(root, args = []) { return { status: res.status, stdout: res.stdout || '', stderr: res.stderr || '' }; } +/** + * Run the generator with `--json` and parse its stdout into the structured + * report (see gen-adr-index.cjs's `--json` doc comment for the shape). Per + * CONTRIBUTING.md's "Prohibited: Raw Text Matching on Test Outputs" (and the + * `bin/verify-reapply-patches.cjs` worked example this PR follows), gate + * assertions bind to this typed report instead of regexing stderr prose. + * Asserts the parse succeeded with a useful message on failure — a crash + * that corrupts stdout (or leaves it empty) fails loudly here instead of + * throwing an opaque `JSON.parse` SyntaxError deep inside a test body. + */ +function runJson(root, args = []) { + const r = run(root, ['--json', ...args]); + let report; + try { + report = JSON.parse(r.stdout); + } catch (err) { + assert.fail(`--json did not emit parseable JSON on stdout (status ${r.status}): ${err.message}\nstdout: ${r.stdout}\nstderr: ${r.stderr}`); + } + return { status: r.status, report }; +} + const adr = (title, fields) => `# ${title}\n\n${fields.map((f) => `- ${f}`).join('\n')}\n\n## Context\n\nBody.\n`; test('a clean corpus generates an index and --check passes', (t) => { @@ -650,61 +671,20 @@ test('insert-only holds for titles carrying markdown/HTML hazards', (t) => { // reverting the repair re-reds them. const ADR_DIR = path.join(REPO_ROOT, 'docs', 'adr'); -const STATUS_TOKENS = ['Accepted', 'Proposed', 'Superseded', 'Legacy', 'Retired']; -function adrMarkdownFiles() { - return fs.readdirSync(ADR_DIR).filter((f) => f.endsWith('.md')); -} - -test('every relative markdown link in docs/adr/ resolves to a file that exists', () => { - const dangling = []; - for (const file of adrMarkdownFiles()) { - const body = fs.readFileSync(path.join(ADR_DIR, file), 'utf8'); - for (const match of body.matchAll(/\]\(([^)#:\s]+\.md)(?:#[^)]*)?\)/g)) { - const target = match[1]; - if (!fs.existsSync(path.resolve(ADR_DIR, target))) { - dangling.push(`${file} -> ${target}`); - } - } - } - assert.deepEqual( - dangling, - [], - `dangling relative links in docs/adr/ (a link written as reference/x.md from inside docs/adr/ resolves to the nonexistent docs/adr/reference/):\n${dangling.join('\n')}`, - ); -}); - -test('no ADR H1 status bracket contradicts its Status field', () => { - // The index generator strips a trailing "[Proposed]"-style bracket for - // display instead of comparing it, so a stale bracket is invisible to the - // gate while still being the first thing a reader sees. - const mismatches = []; - for (const file of adrMarkdownFiles()) { - if (file === 'README.md') continue; - const lines = fs.readFileSync(path.join(ADR_DIR, file), 'utf8').split(/\r?\n/); - const heading = lines.find((l) => /^#\s/.test(l)) || ''; - const bracket = heading.match(/\[(Proposed|Accepted|Superseded|Legacy|Retired)\]\s*$/i); - if (!bracket) continue; - const statusLine = lines.find((l) => /^\s*[-*]?\s*\*\*Status/.test(l)) || ''; - // Resolve by earliest position in the line, not by STATUS_TOKENS order: a - // Status field like "Superseded by ADR-X (was Accepted ...)" mentions two - // tokens, and array order would pick 'Accepted' and report a false mismatch - // against a correct [Superseded] bracket. - let token; - let tokenAt = Infinity; - for (const s of STATUS_TOKENS) { - const at = statusLine.search(new RegExp(`\\b${s}\\b`, 'i')); - if (at !== -1 && at < tokenAt) { - tokenAt = at; - token = s; - } - } - if (token && token.toLowerCase() !== bracket[1].toLowerCase()) { - mismatches.push(`${file}: H1 says [${bracket[1]}], Status field says ${token}`); - } - } - assert.deepEqual(mismatches, [], `H1 bracket contradicts Status:\n${mismatches.join('\n')}`); -}); +// The two corpus-level checks that used to live here — "every relative +// markdown link in docs/adr/ resolves" and "no ADR H1 status bracket +// contradicts its Status field" — were themselves second implementations of +// the rule scripts/gen-adr-index.cjs now enforces for real: exactly the +// `DEFECT.GENERATIVE-FIX` shape (a check and its parallel copy, nothing +// asserting agreement) this PR (#2704) exists to remove. The corpus +// assertion is now made by `test('the real corpus passes the new +// assertions')` below, which is strictly stronger — it runs the shipping +// `--check` code path instead of a parallel regex copy that could silently +// drift from it. One deliberate behavioral difference: the old link-check +// test flagged a dangling `.md` link even inside a fenced code block, where +// the real gate treats fenced (and inline) code as code — markdown does not +// render a link there, so masking it out is correct, not a regression. test('the ADR path cited by src/plan-drift-guard.cts exists', () => { // This module is compiled into the published payload, so a wrong citation @@ -816,3 +796,987 @@ describe('#2705: legacy ADR range single-sourced and accurate', () => { ); }); }); + +// ─── #2704: link resolution + H1 status bracket vs Status: field ─────────── +// +// Failing-first: the production surface below does not exist yet. +// scripts/gen-adr-index.cjs will gain: +// module.exports = { STATUSES, REASON, extractLinks, maskCode } +// extractLinks(text) -> [{ line, target }] (1-indexed line, RAW dest text) +// maskCode(text) -> same-length string; code masked to ' ', newlines kept +// `if (require.main === module) runMain(main);` (today it runs unconditionally) +// `--json` -> { ok, adrCount, indexStale, violations: [{file, reason, ...}] } +// on stdout, gate-verdict tests below bind to this typed report via +// `runJson`, not stderr prose (REASON is the frozen enum of violation kinds). +// +// Two altitudes, per .gsd/phase/feat-2704-adr-gate-link-resolution/50-test-matrix.md: +// IR altitude — require the real script from REPO_ROOT and assert on +// extractLinks/maskCode's typed return values. +// Gate-verdict altitude — run(root, ['--check']) and assert on exit status + +// stderr, following this file's established idiom. +// +// MESSAGE-FORMAT CONTRACT these tests bind the CLI to (there was no contract +// before this PR, so this file establishes one): a dangling/escaping link +// violation is reported as `:: ...` — filename, a literal colon, +// the 1-indexed line number, then prose naming the raw target text. A +// repository-escaping target gets a message containing "escap...", distinct +// from the generic "does not resolve" wording used for an ordinary dangling +// target (row 27) — proof the escape guard runs before any generic resolution +// attempt. An H1-bracket-vs-Status disagreement names the file and BOTH +// tokens and uses the word "bracket", so rows 46/47 can assert its ABSENCE +// precisely when a different, pre-existing error already owns the report. +// +// fast-check is confirmed present in package.json devDependencies (^4.8.0); +// property tests below pin { seed: 2704, numRuns: 200 } per-call so a failure +// replays deterministically regardless of this suite's global fc default. +const { + STATUSES, + REASON, + extractLinks, + maskCode, +} = require(path.join(REPO_ROOT, SCRIPT_REL)); +const fc = require('./helpers/fast-check-setup.cjs'); + +/** + * Build an ADR with header fields plus explicit prose body lines after + * '## Context'. Array `.join('\n')` only — fence/inline-span detection is + * column-sensitive, so an indented template literal would silently corrupt + * the boundary rows (15-20). + */ +function adrBody(title, fields, bodyLines) { + return [ + `# ${title}`, + '', + ...fields.map((f) => `- ${f}`), + '', + '## Context', + '', + ...bodyLines, + '', + ].join('\n'); +} + +// ── Link resolution — gate-verdict altitude ───────────────────────────────── + +test('a resolving relative link passes', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [Beta](900-beta.md) for context.']), + '900-beta.md': adr('Beta', ['**Status:** Accepted']), + }); + assert.equal(run(root, ['--write']).status, 0); + const res = run(root, ['--check']); + assert.equal(res.status, 0, `a link to an existing sibling must resolve: ${res.stderr}`); +}); + +test('a dangling relative link fails and names file, line and target', (t) => { + const root = makeRepo(t, { + // Lines: 1 '# Alpha', 2 '', 3 '- **Status:** Accepted', 4 '', 5 '## Context', + // 6 '', 7 'Body text.', 8 the link line, 9 trailing ''. + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['Body text.', 'See [Ghost](ghost.md) for context.']), + }); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1, 'a dangling relative link must fail --check'); + assert.ok( + report.violations.some( + (v) => v.reason === REASON.LINK_UNRESOLVED && v.file === '0001-alpha.md' && v.line === 8 && v.target === 'ghost.md', + ), + `expected a link_unresolved violation for 0001-alpha.md:8 target ghost.md; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('a directory target counts as resolved', (t) => { + const okRoot = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See the [PRD folder](../prd/) for background.']), + }); + fs.mkdirSync(path.join(okRoot, 'docs', 'prd'), { recursive: true }); + assert.equal(run(okRoot, ['--write']).status, 0); + assert.equal(run(okRoot, ['--check']).status, 0, 'an existing directory target must resolve'); + + const missingRoot = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See the [PRD folder](../prd/) for background.']), + }); + // No docs/prd/ created here — the directory target does not exist. + const { status, report } = runJson(missingRoot, ['--check']); + assert.equal(status, 1, 'a nonexistent directory target must fail'); + assert.ok( + report.violations.some((v) => v.reason === REASON.LINK_UNRESOLVED && v.target === '../prd/'), + `expected a link_unresolved violation for target ../prd/; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('absolute destinations are out of scope', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], [ + 'See [docs](https://example.com/nonexistent) and [http](http://example.com/x)', + 'and [mail](mailto:nobody@example.com) and [proto-rel](//example.com/x).', + ]), + }); + assert.equal(run(root, ['--write']).status, 0); + const res = run(root, ['--check']); + assert.equal(res.status, 0, `URI-scheme/protocol-relative destinations must be skipped: ${res.stderr}`); +}); + +test('a same-document anchor is not a file reference', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [context](#context) above.']), + }); + assert.equal(run(root, ['--write']).status, 0); + assert.equal(run(root, ['--check']).status, 0); +}); + +test('a fragment is stripped before resolution', (t) => { + const okRoot = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [Beta](900-beta.md#context) for detail.']), + '900-beta.md': adr('Beta', ['**Status:** Accepted']), + }); + assert.equal(run(okRoot, ['--write']).status, 0); + assert.equal(run(okRoot, ['--check']).status, 0, 'the file exists — only the fragment is unresolved (and ignored)'); + + const missingRoot = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [Ghost](ghost.md#context) for detail.']), + }); + const { status, report } = runJson(missingRoot, ['--check']); + assert.equal(status, 1, 'the file does not exist, fragment or not'); + assert.ok( + report.violations.some((v) => v.reason === REASON.LINK_UNRESOLVED && v.target === 'ghost.md#context'), + `expected a link_unresolved violation naming ghost.md; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('an empty destination is skipped', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], [ + 'Empty: [t]().', + 'Whitespace-only: [t]( ).', + ]), + }); + assert.equal(run(root, ['--write']).status, 0); + const res = run(root, ['--check']); + assert.equal(res.status, 0, `empty/whitespace-only destinations must never resolve to the ADR dir: ${res.stderr}`); +}); + +test('a link inside a fenced code block is code, not a link', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], [ + '```', + 'See [Ghost](ghost.md) inside a backtick fence.', + '```', + '', + '~~~', + 'See [Ghost2](ghost2.md) inside a tilde fence.', + '~~~', + ]), + }); + assert.equal(run(root, ['--write']).status, 0); + const res = run(root, ['--check']); + assert.equal(res.status, 0, `links inside fenced code must not be checked: ${res.stderr}`); +}); + +test('a link inside an inline code span is code, not a link', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['Inline: `[Ghost](ghost.md)` is code, not a link.']), + }); + assert.equal(run(root, ['--write']).status, 0); + const res = run(root, ['--check']); + assert.equal(res.status, 0, `a link inside an inline code span must not be checked: ${res.stderr}`); +}); + +test('the two code shapes present in the real corpus produce no findings', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], [ + '```text', + 'mod[entry.router]({ args, cwd, raw, error })', + '```', + '', + 'Inline: `require(module)[router]()` explained here.', + ]), + }); + assert.equal(run(root, ['--write']).status, 0); + const res = run(root, ['--check']); + assert.equal(res.status, 0, `the real corpus's bracket-after-identifier shapes must not be misread as links: ${res.stderr}`); +}); + +test('a dangling image target fails', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['![diagram](missing.png)']), + }); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1, 'an image with a dangling target must fail like any link'); + assert.ok( + report.violations.some((v) => v.reason === REASON.LINK_UNRESOLVED && v.target === 'missing.png'), + `expected a link_unresolved violation for missing.png; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('angle-bracket and titled destinations', (t) => { + // 'ref/a b.md' lives under a subdirectory: readdirSync(ADR_DIR) is + // non-recursive, so this never trips the `-.md` naming check. + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], [ + 'See [Beta]() for detail.', + 'See [Gamma](900-gamma.md "The Gamma decision") too.', + ]), + '900-gamma.md': adr('Gamma', ['**Status:** Accepted']), + }); + fs.mkdirSync(path.join(root, 'docs', 'adr', 'ref'), { recursive: true }); + fs.writeFileSync(path.join(root, 'docs', 'adr', 'ref', 'a b.md'), '# scratch\n'); + assert.equal(run(root, ['--write']).status, 0); + const res = run(root, ['--check']); + assert.equal(res.status, 0, `angle-bracket and titled destinations must resolve: ${res.stderr}`); +}); + +test('percent-encoded destinations decode', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], [ + 'See [Beta](ref/a%20b.md) for detail.', + 'See [Ghost](ref/a%zz.md) too.', + ]), + }); + fs.mkdirSync(path.join(root, 'docs', 'adr', 'ref'), { recursive: true }); + fs.writeFileSync(path.join(root, 'docs', 'adr', 'ref', 'a b.md'), '# scratch\n'); + // A malformed percent-escape must not crash the process: `runJson` asserts + // the parse succeeded, which itself proves stdout carried a real JSON + // document rather than a stack trace from an uncaught TypeError/URIError. + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1, `a%20b.md must decode and resolve; a%zz.md is malformed and must not resolve: ${JSON.stringify(report)}`); + assert.ok( + !report.violations.some((v) => v.target === 'ref/a%20b.md'), + 'the percent-encoded-but-valid target must not itself be reported', + ); + assert.ok( + report.violations.some((v) => v.reason === REASON.LINK_UNRESOLVED && v.target === 'ref/a%zz.md'), + `the malformed escape must be reported using its raw text; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('a root-relative destination resolves against the repo root', (t) => { + // '/docs/prd/plan.md' exists ONLY relative to the repo root — resolving it + // relative to docs/adr/ instead (docs/adr/docs/prd/plan.md) would not exist, + // so this discriminates the two interpretations rather than only proving + // "some" interpretation works. + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [Repo file](/docs/prd/plan.md).']), + }); + fs.mkdirSync(path.join(root, 'docs', 'prd'), { recursive: true }); + fs.writeFileSync(path.join(root, 'docs', 'prd', 'plan.md'), '# plan\n'); + assert.equal(run(root, ['--write']).status, 0); + const res = run(root, ['--check']); + assert.equal(res.status, 0, `a root-relative destination must resolve against the repo root: ${res.stderr}`); +}); + +test('a destination escaping the repository is rejected without touching the filesystem', (t) => { + // IR altitude first: extractLinks must capture the raw traversal target + // verbatim — no early resolution/mangling before the CLI-level escape + // guard gets a chance to reject it. + const source = 'See [Ghost](../../../../../etc/passwd) for context.\n'; + assert.deepEqual(extractLinks(source), [{ line: 1, target: '../../../../../etc/passwd' }]); + + // Gate-verdict altitude: the CLI must reject the escape outright. + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [Ghost](../../../../../etc/passwd) for context.']), + }); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1, 'a link escaping the repository root must fail'); + assert.ok( + report.violations.some((v) => v.reason === REASON.LINK_ESCAPES_REPO && v.target === '../../../../../etc/passwd'), + `expected a link_escapes_repo violation for the traversal target; got ${JSON.stringify(report.violations)}`, + ); + // The escape-vs-unresolved discrimination is now two distinct reason + // codes, not two prose patterns — proof the escape guard runs BEFORE any + // generic resolution attempt. + assert.ok( + !report.violations.some((v) => v.reason === REASON.LINK_UNRESOLVED), + 'a repo-escaping target must get link_escapes_repo, never the generic link_unresolved reason', + ); +}); + +test('a repo-root path whose first segment starts with two dots is not an escape', (t) => { + // `path.relative(ROOT, abs).startsWith('..')` alone would ALSO match an + // in-repo path whose first segment merely begins with two dots — a + // legitimate root-level file named `..hidden.md`. docs/adr/ is two + // segments below ROOT, so '../..' walks docs/adr -> docs -> ROOT, landing + // squarely inside the repo: path.relative(ROOT, ROOT/'..hidden.md') is + // exactly '..hidden.md', which starts with '..' but does not escape. + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [Hidden](../../..hidden.md) for context.']), + }); + fs.writeFileSync(path.join(root, '..hidden.md'), '# hidden\n'); + assert.equal( + path.relative(root, path.join(root, '..hidden.md')), + '..hidden.md', + 'fixture sanity check: the resolved relative path must literally start with two dots', + ); + assert.equal(run(root, ['--write']).status, 0); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 0, `a same-segment-prefix path must not be misclassified as escaping: ${JSON.stringify(report)}`); + assert.ok(report.ok, 'a same-segment-prefix path must produce a clean report, not an escape violation'); +}); + +test('resolution is case-exact', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adr('Alpha', ['**Status:** Accepted']), + '900-beta.md': adrBody('Beta', ['**Status:** Accepted'], ['See [Alpha](0001-ALPHA.md) for detail.']), + }); + // Deliberately no platform guard: case-exactness must hold identically on + // every OS, including case-insensitive filesystems (macOS default, Windows), + // where a naive fs.existsSync(...) would silently resolve and hide this. + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1, 'a case-mismatched target must fail on every platform, not just case-sensitive ones'); + assert.ok( + report.violations.some((v) => v.reason === REASON.LINK_UNRESOLVED && v.target === '0001-ALPHA.md'), + `expected a link_unresolved violation for the case-mismatched target; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('every occurrence is reported, not just the first', (t) => { + const root = makeRepo(t, { + // Lines: 1 '# Alpha' .. 6 '', 7 First, 8 Between, 9 Second, 10 trailing ''. + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], [ + 'First: [Ghost](ghost.md).', + 'Between.', + 'Second: [Ghost again](ghost.md).', + ]), + }); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1); + const ghostFindings = report.violations.filter((v) => v.reason === REASON.LINK_UNRESOLVED && v.target === 'ghost.md'); + assert.ok(ghostFindings.length >= 2, `both dangling occurrences must be reported; violations:\n${JSON.stringify(report.violations)}`); + assert.ok(ghostFindings.some((v) => v.line === 7), 'first occurrence must report its own line'); + assert.ok(ghostFindings.some((v) => v.line === 9), 'second occurrence must report its own line'); +}); + +test('only the unresolvable link on a mixed line is reported', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [Beta](900-beta.md) and [Ghost](ghost.md) together.']), + '900-beta.md': adr('Beta', ['**Status:** Accepted']), + }); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1); + assert.ok( + !report.violations.some((v) => v.target === '900-beta.md'), + 'the resolving link must not be reported', + ); + assert.ok( + report.violations.some((v) => v.reason === REASON.LINK_UNRESOLVED && v.target === 'ghost.md' && v.line === 7), + `the unresolvable link must be reported on its own line; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('CRLF input yields the same findings as LF', () => { + const lfLines = ['# Alpha', '', 'See [Ghost](ghost.md) here.', '', 'More [Also](also.md) text.']; + const lfLinks = extractLinks(lfLines.join('\n')); + const crlfLinks = extractLinks(lfLines.join('\r\n')); + assert.deepEqual(crlfLinks, lfLinks, 'CRLF and LF twins must yield identical {line, target} findings'); +}); + +test('a dangling link in README.md is caught', (t) => { + const root = makeRepo(t, { '0001-alpha.md': adr('Alpha', ['**Status:** Accepted']) }); + // makeRepo writes its own bare README.md — this test supplies real prose so + // it can carry a dangling link. + fs.writeFileSync( + path.join(root, 'docs', 'adr', 'README.md'), + ['# ADRs', '', 'See [the process doc](process.md) for how ADRs are written.', '', '## Index', '', START, END, ''].join('\n'), + ); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1, 'a dangling link in README.md must be caught'); + assert.ok( + report.violations.some((v) => v.reason === REASON.LINK_UNRESOLVED && v.file === 'README.md' && v.target === 'process.md'), + `expected a link_unresolved violation naming README.md's dangling link; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('a non-conforming filename is still link-checked', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adr('Alpha', ['**Status:** Accepted']), + 'notes.md': ['# Scratch notes', '', 'Not an ADR, but see [Ghost](ghost.md) anyway.', ''].join('\n'), + }); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1); + assert.ok( + report.violations.some((v) => v.reason === REASON.FILENAME_INVALID && v.file === 'notes.md'), + `the existing naming violation must still be reported; got ${JSON.stringify(report.violations)}`, + ); + assert.ok( + report.violations.some((v) => v.reason === REASON.LINK_UNRESOLVED && v.file === 'notes.md' && v.target === 'ghost.md'), + `the dangling link must ALSO be reported; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('the real corpus passes the new assertions', () => { + const res = run(REPO_ROOT, ['--check']); + assert.equal(res.status, 0, `docs/adr/ must satisfy the link-resolution and bracket-parity gates:\n${res.stderr}`); +}); + +// ── Link resolution — IR altitude (fence/inline-span boundaries, hostile input) ── + +test('fence marker length 2/3/4', () => { + // 2-backtick run: NOT a fence — a dangling link after it must still be found. + const two = ['``', 'text', '``', '[Ghost](ghost.md)'].join('\n'); + assert.deepEqual(extractLinks(two), [{ line: 4, target: 'ghost.md' }], '2 backticks do not open a fence'); + + // 3-backtick run: IS a fence — its contents (including a link) are masked. + const three = ['```', '[Ghost](ghost.md)', '```'].join('\n'); + assert.deepEqual(extractLinks(three), [], '3 backticks open a real fence'); + + // 4-backtick run closed by only 3: still open — a shorter run cannot close it. + const four = ['````', '[Ghost](ghost.md)', '```', 'still inside the fence', '[Ghost2](ghost2.md)', '````'].join('\n'); + assert.deepEqual(extractLinks(four), [], 'a 4-run fence is not closed by a 3-run'); +}); + +test('a fence closes only on its own marker kind', () => { + const mixed = [ + '```', + '[Ghost](ghost.md)', + '~~~', + 'still fenced — ~~~ does not close a ``` fence', + '[Ghost2](ghost2.md)', + '```', + '[After](after.md)', + ].join('\n'); + assert.deepEqual(extractLinks(mixed), [{ line: 7, target: 'after.md' }], 'only the real close (```) ends the fence'); +}); + +test('an unterminated fence swallows the remainder without crashing', () => { + const text = ['```', '[Ghost](ghost.md)', 'never closed', '[Ghost2](ghost2.md)'].join('\n'); + let links; + assert.doesNotThrow(() => { links = extractLinks(text); }); + assert.deepEqual(links, [], 'an unterminated fence masks the rest of the file — no findings, no crash'); +}); + +test('inline code spans close on an equal backtick run', () => { + const oneRun = 'a `[Ghost](ghost.md)` b [Real](real.md)'; + assert.deepEqual(extractLinks(oneRun), [{ line: 1, target: 'real.md' }], 'a 1-backtick span closes on the next 1-run'); + + const twoRun = 'a ``[Ghost](ghost.md)`` b [Real](real.md)'; + assert.deepEqual(extractLinks(twoRun), [{ line: 1, target: 'real.md' }], 'a 2-backtick span closes on the next 2-run'); +}); + +test('link text with nested brackets is skipped, not misreported', () => { + let links; + assert.doesNotThrow(() => { links = extractLinks('[see [1]](x.md)'); }); + assert.deepEqual(links, [], 'nested brackets in link text are out of the inline-links-only scope'); +}); + +test('regex character classes in prose are not links', () => { + const text = 'Use `[A-Z][A-Z0-9_]` for constants and [a-z0-9][a-z0-9-] for slugs.'; + assert.deepEqual(extractLinks(text), [], 'bracket-adjacent-bracket regex-class prose must not be read as markdown links'); +}); + +test('an empty ADR file produces no link findings', (t) => { + // IR altitude: no text at all yields no links (not even a crash). + assert.deepEqual(extractLinks(''), []); + + // Gate-verdict altitude: the existing "no Status field" rule still fires for + // a 0-byte file, but no spurious link-resolution finding piggybacks on it. + const root = makeRepo(t, { '0001-alpha.md': '' }); + const res = run(root, ['--check']); + assert.equal(res.status, 1); + assert.match(res.stderr, /no `- \*\*Status:\*\* ` field/); + assert.doesNotMatch(res.stderr, /does not resolve|escapes the repository/, 'a 0-byte file must not also report a link finding'); +}); + +test('property: extractLinks is total and reports in-range lines', () => { + fc.assert( + fc.property( + fc.oneof( + fc.string({ maxLength: 300 }), + fc.string({ unit: 'grapheme-composite', maxLength: 300 }), + fc.string({ unit: 'binary', maxLength: 300 }), + ), + (text) => { + let links; + assert.doesNotThrow(() => { links = extractLinks(text); }, `extractLinks threw on: ${JSON.stringify(text).slice(0, 120)}`); + const lineCount = text.split(/\r?\n/).length; + for (const { line } of links) { + assert.ok(line >= 1 && line <= lineCount, `line ${line} out of range [1, ${lineCount}]`); + } + }, + ), + { seed: 2704, numRuns: 200 }, + ); +}); + +test('property: masking preserves length and line structure', () => { + fc.assert( + fc.property( + fc.oneof( + fc.string({ maxLength: 300 }), + fc.string({ unit: 'grapheme-composite', maxLength: 300 }), + fc.string({ unit: 'binary', maxLength: 300 }), + ), + (text) => { + let masked; + assert.doesNotThrow(() => { masked = maskCode(text); }, `maskCode threw on: ${JSON.stringify(text).slice(0, 120)}`); + assert.equal(masked.length, text.length, 'masked output must be the same length as input'); + for (let i = 0; i < text.length; i++) { + if (text[i] === '\n') assert.equal(masked[i], '\n', `newline at index ${i} must survive masking`); + } + }, + ), + { seed: 2704, numRuns: 200 }, + ); +}); + +// ── H1 status bracket vs Status: field — gate-verdict altitude ────────────── + +test('an agreeing H1 bracket passes and is still stripped from the title', (t) => { + const root = makeRepo(t, { '0001-alpha.md': adr('Title one [Accepted]', ['**Status:** Accepted']) }); + assert.equal(run(root, ['--write']).status, 0); + const check = run(root, ['--check']); + assert.equal(check.status, 0, `an agreeing bracket must pass: ${check.stderr}`); + const readme = fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8'); + assert.doesNotMatch(readme, /\[Accepted\]/, 'the bracket must not survive into the rendered title'); + assert.match(readme, /Title one/); +}); + +test('an H1 bracket contradicting Status fails and names both', (t) => { + const root = makeRepo(t, { '0001-alpha.md': adr('Title one [Proposed]', ['**Status:** Accepted']) }); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1); + assert.ok( + report.violations.some( + (v) => + v.reason === REASON.STATUS_BRACKET_MISMATCH && + v.file === '0001-alpha.md' && + v.actual === 'Proposed' && + v.expected === 'Accepted', + ), + `expected a status_bracket_mismatch violation naming both tokens; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('bracket comparison is case-insensitive', (t) => { + const root = makeRepo(t, { '0001-alpha.md': adr('Title one [proposed]', ['**Status:** Proposed']) }); + assert.equal(run(root, ['--write']).status, 0); + assert.equal(run(root, ['--check']).status, 0, 'a differently-cased but agreeing bracket must pass'); +}); + +test('a bracket agreeing with a prose-carrying Status passes', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adr('Title one [Superseded]', ['**Status:** Superseded by [ADR-900](900-beta.md)']), + '900-beta.md': adr('Title two', ['**Status:** Accepted', '**Supersedes:** [ADR-0001](0001-alpha.md)']), + }); + assert.equal(run(root, ['--write']).status, 0); + const check = run(root, ['--check']); + assert.equal(check.status, 0, `a bracket agreeing with the parsed status TOKEN must pass: ${check.stderr}`); +}); + +test('a non-status trailing bracket is title text, not a claim', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adr('Title one [Draft]', ['**Status:** Accepted']), + '900-beta.md': adr('Title two [ADR-0001](0001-alpha.md)', ['**Status:** Accepted']), + }); + assert.equal(run(root, ['--write']).status, 0); + const check = run(root, ['--check']); + assert.equal(check.status, 0, `a non-vocabulary bracket and a link-shaped H1 suffix are both title text: ${check.stderr}`); + const readme = fs.readFileSync(path.join(root, 'docs', 'adr', 'README.md'), 'utf8'); + assert.match(readme, /\[Draft\]/, '[Draft] is title text and must survive into the rendered title'); +}); + +test('a missing Status field does not also report bracket disagreement', (t) => { + const root = makeRepo(t, { '0001-alpha.md': '# Title one [Accepted]\n\nNo header fields.\n\n## Context\n\nBody.\n' }); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1); + assert.ok( + report.violations.some((v) => v.reason === REASON.STATUS_MISSING && v.file === '0001-alpha.md'), + `expected a status_missing violation; got ${JSON.stringify(report.violations)}`, + ); + assert.ok( + !report.violations.some((v) => v.reason === REASON.STATUS_BRACKET_MISMATCH), + 'a missing Status field must not ALSO get a status_bracket_mismatch violation', + ); +}); + +test('an invalid Status token is not also reported as a bracket disagreement', (t) => { + const root = makeRepo(t, { '0001-alpha.md': adr('Title one [Accepted]', ['**Status:** Draft']) }); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1); + assert.ok( + report.violations.some((v) => v.reason === REASON.STATUS_INVALID && v.status === 'Draft'), + `expected a status_invalid violation naming "Draft"; got ${JSON.stringify(report.violations)}`, + ); + assert.ok( + !report.violations.some((v) => v.reason === REASON.STATUS_BRACKET_MISMATCH), + 'an invalid status token must not ALSO get a status_bracket_mismatch violation', + ); +}); + +test('an ADR with no H1 is skipped', (t) => { + const root = makeRepo(t, { '0001-alpha.md': '- **Status:** Accepted\n\n## Context\n\nBody with no heading line at all.\n' }); + assert.equal(run(root, ['--write']).status, 0); + const res = run(root, ['--check']); + assert.equal(res.status, 0, `no H1 means no bracket to compare — must not error: ${res.stderr}`); +}); + +test('both real H1 spellings are compared', (t) => { + const root = makeRepo(t, { + '1143-one.md': '# ADR-1143: Title one [Proposed]\n\n- **Status:** Accepted\n\n## Context\n\nBody.\n', + '1606-two.md': '# ADR 1606: title two [Proposed]\n\n- **Status:** Accepted\n\n## Context\n\nBody.\n', + }); + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1); + assert.ok( + report.violations.some((v) => v.reason === REASON.STATUS_BRACKET_MISMATCH && v.file === '1143-one.md'), + `expected a status_bracket_mismatch violation for 1143-one.md; got ${JSON.stringify(report.violations)}`, + ); + assert.ok( + report.violations.some((v) => v.reason === REASON.STATUS_BRACKET_MISMATCH && v.file === '1606-two.md'), + `expected a status_bracket_mismatch violation for 1606-two.md; got ${JSON.stringify(report.violations)}`, + ); +}); + +/** + * Some status tokens carry obligations beyond "the H1 bracket must agree + * with the Status field" — e.g. `Superseded` also requires a successor + * named as a markdown link, symmetrically recorded on both sides (see + * gen-adr-index.cjs's dedicated Superseded check). The bracket-parity test + * below wants to exercise ONLY bracket-vs-Status agreement, so whatever + * status a fixture DECLARES must independently satisfy that status's own + * obligations — otherwise the corpus fails for an unrelated, pre-existing + * reason and the test reports the wrong defect (exactly what happened here: + * a bare "Superseded" Status field with no successor tripped the + * "names no successor" check before bracket comparison ever mattered). + * + * Keyed by status token, not hardcoded into the test body, so a FUTURE + * token with its own obligation is forced through this same seam instead of + * silently reusing the bare-token fixture and reporting a misleading + * failure. + */ +function statusObligations(token) { + if (token === 'Superseded') { + return { + statusField: 'Superseded by [ADR-900](900-beta.md)', + companions: { + '900-beta.md': adr('Beta', ['**Status:** Accepted', '**Supersedes:** [ADR-0001](0001-alpha.md)']), + }, + }; + } + return { statusField: token, companions: {} }; +} + +test('parity: every status in the vocabulary is recognized as a bracket', (t) => { + // DEFECT.GENERATIVE-FIX guard: this iterates the REAL exported STATUSES + // array instead of a hand-copied literal, so a 6th status added to the + // vocabulary is covered by this test the day it lands, not the next time + // someone remembers to update a parallel hardcoded list here. + assert.ok(Array.isArray(STATUSES) && STATUSES.length > 0, 'STATUSES must be a real exported, non-empty array'); + for (const token of STATUSES) { + const own = statusObligations(token); + const agreeing = makeRepo(t, { + '0001-alpha.md': adr(`Title one [${token}]`, [`**Status:** ${own.statusField}`]), + ...own.companions, + }); + assert.equal(run(agreeing, ['--write']).status, 0, `token "${token}": --write must succeed`); + const agreeingCheck = run(agreeing, ['--check']); + assert.equal(agreeingCheck.status, 0, `token "${token}": an agreeing bracket must pass: ${agreeingCheck.stderr}`); + + const other = STATUSES.find((s) => s !== token); + // The contradicting fixture DECLARES `other` in the Status field, so it + // is `other`'s obligations (not `token`'s) that the corpus must satisfy. + const otherObligations = statusObligations(other); + const contradicting = makeRepo(t, { + '0001-alpha.md': adr(`Title one [${token}]`, [`**Status:** ${otherObligations.statusField}`]), + ...otherObligations.companions, + }); + assert.equal(run(contradicting, ['--write']).status, 0, `token "${token}" vs "${other}": --write must succeed even with violations`); + const { status: contradictingStatus, report: contradictingReport } = runJson(contradicting, ['--check']); + assert.equal(contradictingStatus, 1, `token "${token}" vs "${other}": a contradicting bracket must fail`); + assert.ok( + contradictingReport.violations.some( + (v) => v.reason === REASON.STATUS_BRACKET_MISMATCH && v.actual === token && v.expected === other, + ), + `token "${token}" vs "${other}": expected a status_bracket_mismatch violation with actual="${token}" expected="${other}"; got ${JSON.stringify(contradictingReport.violations)}`, + ); + } +}); + +test('the generated index is byte-identical to the pre-change output', (t) => { + // Build the SAME corpus twice — once with an agreeing bracket in the H1, + // once without — and assert the rendered README region is identical either + // way. The bracket is compared, not rendered: adding the comparison must + // not move a single byte of the generated index. + const withBracket = makeRepo(t, { '0001-alpha.md': adr('Title one [Accepted]', ['**Status:** Accepted']) }); + assert.equal(run(withBracket, ['--write']).status, 0); + const readmeWith = fs.readFileSync(path.join(withBracket, 'docs', 'adr', 'README.md'), 'utf8'); + + const withoutBracket = makeRepo(t, { '0001-alpha.md': adr('Title one', ['**Status:** Accepted']) }); + assert.equal(run(withoutBracket, ['--write']).status, 0); + const readmeWithout = fs.readFileSync(path.join(withoutBracket, 'docs', 'adr', 'README.md'), 'utf8'); + + assert.equal(readmeWith, readmeWithout, 'an agreeing H1 bracket must not change a single byte of the generated index'); +}); + +// ─── Isolated adversarial security review, #2704 follow-up ───────────────── +// +// F1/F2 (BLOCKER, live PoC): `validateLinks` checks containment LEXICALLY +// (`path.relative(ROOT, abs)`), which is correct as far as it goes. But +// `existsCaseExact` then walks path segments via `readdirSync`, which +// FOLLOWS symlinks at the OS level. A contributor can commit +// `docs/adr/x -> /etc/somewhere-outside` plus an ADR linking +// `[t](x/passwd)`: the lexical check sees `docs/adr/x/passwd` (looks +// repo-internal), and the walk then lists the real external directory — and +// a wrong-case probe echoes a real filename from OUTSIDE the repo into +// PUBLIC CI LOGS on a fork PR via the "Did you mean X?" hint. Proven live by +// the reviewer. +// +// F4 (MINOR): an unreadable `*.md` dirent (broken symlink) previously threw +// `ENOENT` out of `markdownFilesInAdrDir`'s `statSync`, and `runMain` wrote +// the raw `err.stack` — absolute host filesystem paths — to public fork-PR +// CI logs. +// +// F3 (MAJOR): `maskInlineCodeSpans`'s correctness under adversarial input is +// covered separately below; see that test's comment. + +describe('symlink escape guard (F1/F2)', () => { + /** + * fs.symlinkSync can fail with EPERM on a platform/host that forbids + * unprivileged symlink creation (notably Windows without Developer Mode or + * admin rights). `t.skip()` degrades cleanly there; a bare `return` would + * silently report a PASS in node:test and hide the gap this guard exists + * to close. + */ + function trySymlink(t, target, linkPath, type) { + try { + fs.symlinkSync(target, linkPath, type); + return true; + } catch (err) { + if (err && err.code === 'EPERM') { + t.skip('cannot create symlinks on this platform (EPERM)'); + return false; + } + throw err; + } + } + + test('a link traversing a symlink out of the repository is rejected', (t) => { + const outside = createTempDir('gsd-adr-index-outside-'); + t.after(() => cleanup(outside)); + fs.writeFileSync(path.join(outside, 'secret.txt'), 'do not leak me\n'); + + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [t](x/secret.txt) for context.']), + }); + if (!trySymlink(t, outside, path.join(root, 'docs', 'adr', 'x'), 'dir')) return; + + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1, 'a link traversing an out-of-repo symlink must fail --check'); + assert.ok( + report.violations.some((v) => v.reason === REASON.LINK_ESCAPES_REPO_SYMLINK && v.target === 'x/secret.txt'), + `expected a link_escapes_repo_symlink violation, not an ordinary dangling link; got ${JSON.stringify(report.violations)}`, + ); + assert.ok( + !report.violations.some((v) => v.reason === REASON.LINK_UNRESOLVED), + 'a symlink escape must get its own reason code, never the generic link_unresolved one', + ); + }); + + test('a symlink out of the repository never leaks a filename hint', (t) => { + const outside = createTempDir('gsd-adr-index-outside-'); + t.after(() => cleanup(outside)); + fs.writeFileSync(path.join(outside, 'secret.txt'), 'do not leak me\n'); + + const root = makeRepo(t, { + // Wrong case: on a naive implementation this would trigger a + // "Did you mean secret.txt?" hint built from the OUTSIDE directory's + // real listing — the disclosure this test guards against. + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [t](x/SECRET.TXT) for context.']), + }); + if (!trySymlink(t, outside, path.join(root, 'docs', 'adr', 'x'), 'dir')) return; + + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1); + // A stronger guarantee than the old stderr-prose check: no field of any + // violation record — not just a hand-picked message string — may carry + // the sentinel filename from OUTSIDE the repository. The ADR's own link + // target is deliberately case-DIFFERENT ('x/SECRET.TXT') from the real + // outside file ('secret.txt'), so this exact-case search cannot + // false-positive on the requested-target text, only on a genuine leak + // (e.g. a "Did you mean secret.txt?" hint built from the real listing). + assert.equal( + JSON.stringify(report).includes('secret.txt'), + false, + `the real external filename must never be echoed into the report: ${JSON.stringify(report)}`, + ); + }); + + test('a symlink that stays inside the repository still resolves', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adrBody('Alpha', ['**Status:** Accepted'], ['See [t](inside/target.md) for context.']), + }); + const insideTarget = path.join(root, 'internal-target'); + fs.mkdirSync(insideTarget, { recursive: true }); + fs.writeFileSync(path.join(insideTarget, 'target.md'), '# scratch\n'); + if (!trySymlink(t, insideTarget, path.join(root, 'docs', 'adr', 'inside'), 'dir')) return; + + assert.equal(run(root, ['--write']).status, 0); + const res = run(root, ['--check']); + assert.equal(res.status, 0, `an in-repo symlink must not be misclassified as an escape: ${res.stderr}`); + }); + + // Same containment rule this describe block enforces for link TARGETS + // (`x/secret.txt` above) applies to the ADR FILES themselves: + // `markdownFilesInAdrDir` used `fs.statSync`, which follows symlinks, so a + // `docs/adr/*.md` symlinked out of the repo was accepted as an ADR and had + // its full body read by `parseAdr` and scanned for links by + // `validateLinks` — echoing fragments of an arbitrary outside file into + // public stderr on a fork PR. + + test('an ADR file that is a symlink out of the repository is not read', (t) => { + const outside = createTempDir('gsd-adr-index-outside-'); + t.after(() => cleanup(outside)); + fs.writeFileSync( + path.join(outside, 'evil-source.md'), + adrBody('Evil', ['**Status:** Accepted'], ['SENTINEL-OUTSIDE-CONTENT', 'See [x](sentinel-target.md) for context.']), + ); + + const root = makeRepo(t, {}); + if ( + !trySymlink(t, path.join(outside, 'evil-source.md'), path.join(root, 'docs', 'adr', '0002-evil.md'), 'file') + ) { + return; + } + + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1, 'an ADR file symlinked out of the repository must fail the gate'); + assert.ok( + report.violations.some((v) => v.reason === REASON.DIRENT_ESCAPES_REPO_SYMLINK && v.file === '0002-evil.md'), + `expected a dirent_escapes_repo_symlink violation naming 0002-evil.md, distinct from the broken/unreadable reason; got ${JSON.stringify(report.violations)}`, + ); + assert.equal( + JSON.stringify(report).includes('SENTINEL-OUTSIDE-CONTENT'), + false, + 'must never read or echo content from the file outside the repository', + ); + assert.equal( + JSON.stringify(report).includes('sentinel-target.md'), + false, + 'must never echo a link target found only inside the unread outside file', + ); + }); + + test('an ADR file that is a symlink inside the repository is still read', (t) => { + const root = makeRepo(t, { + '0001-alpha.md': adr('Alpha module', ['**Status:** Accepted']), + }); + if ( + !trySymlink( + t, + path.join(root, 'docs', 'adr', '0001-alpha.md'), + path.join(root, 'docs', 'adr', '0003-alias.md'), + 'file', + ) + ) { + return; + } + + const write = run(root, ['--write']); + assert.equal(write.status, 0, `a legitimate in-repo symlinked ADR file must still be read: ${write.stderr}`); + }); +}); + +test('a broken symlink under docs/adr is reported, not a crash (F4)', (t) => { + const root = makeRepo(t, { '0001-alpha.md': adr('Alpha', ['**Status:** Accepted']) }); + try { + fs.symlinkSync(path.join(root, 'does-not-exist.md'), path.join(root, 'docs', 'adr', 'ghost.md'), 'file'); + } catch (err) { + if (err && err.code === 'EPERM') { t.skip('cannot create symlinks on this platform (EPERM)'); return; } + throw err; + } + + // runJson itself asserts stdout parses as JSON: a raw stack trace or a + // surfaced fs error (TypeError/ENOENT) would either corrupt stdout or + // leave it empty, so the parse succeeding is already proof this is a + // typed gate violation, not a crash — no separate stderr pattern needed. + const { status, report } = runJson(root, ['--check']); + assert.equal(status, 1, 'a broken symlink under docs/adr must fail the gate, not crash it'); + assert.ok( + report.violations.some((v) => v.reason === REASON.DIRENT_UNREADABLE && v.file === 'ghost.md'), + `expected a dirent_unreadable violation naming ghost.md; got ${JSON.stringify(report.violations)}`, + ); +}); + +test('masking an adversarial backtick line stays fast (F3)', () => { + // ~2000 backtick runs of strictly ascending length, none of which ever + // closes (every length is unique) — the exact shape that forced the old + // per-opener rescan implementation into near-quadratic time (measured + // 34ms/50KB -> 220ms/200KB -> 1.76s/800KB, unbounded). This test asserts + // correctness, not timing (this repo forbids wall-clock assertions in + // tests) — the linear rewrite's speed is verified separately, out of band. + const N = 2000; + let hostile = ''; + for (let n = 1; n <= N; n += 1) hostile += '`'.repeat(n) + 'x'; + + const text = `Intro line.\n${hostile}\nSee [Real](real.md) after the pathological section.\n`; + + let masked; + assert.doesNotThrow(() => { masked = maskCode(text); }, 'masking the adversarial line must not throw'); + assert.equal(masked.length, text.length, 'masked output must be the same length as input'); + for (let i = 0; i < text.length; i += 1) { + if (text[i] === '\n') assert.equal(masked[i], '\n', `newline at index ${i} must survive masking`); + } + + const links = extractLinks(text); + assert.deepEqual( + links.map((l) => l.target), + ['real.md'], + 'the link after the pathological backtick section must still be found', + ); +}); + +// ── --json structured output surface ──────────────────────────────────── + +test('the REASON enum is locked', () => { + // Adding a violation class is a deliberate three-part change: a new entry + // here, its emitting `add(...)` call site, and this list growing to match + // — never a silent addition that a `--json` consumer discovers by surprise. + assert.deepEqual(Object.keys(REASON).sort(), [ + 'DIRENT_ESCAPES_REPO_SYMLINK', + 'DIRENT_UNREADABLE', + 'FILENAME_INVALID', + 'ID_MISMATCH', + 'LINK_ESCAPES_REPO', + 'LINK_ESCAPES_REPO_SYMLINK', + 'LINK_UNRESOLVED', + 'RELATION_ASYMMETRIC', + 'RELATION_BARE_ID_MISSING', + 'RELATION_BARE_ID_UNLINKED', + 'RELATION_LINK_MISSING', + 'STATUS_BRACKET_MISMATCH', + 'STATUS_INVALID', + 'STATUS_MISSING', + 'SUPERSEDED_BARE_ID', + 'SUPERSEDED_NO_SUCCESSOR', + ]); +}); + +test('an unknown flag is rejected', (t) => { + const root = makeRepo(t, { '0001-alpha.md': adr('Alpha', ['**Status:** Accepted']) }); + const res = run(root, ['--bogus']); + assert.equal(res.status, 1, 'an unrecognized flag must fail closed, not silently fall through'); + assert.match(res.stderr, /unknown flag: --bogus/); + assert.equal(res.stdout, '', 'the index must NOT be printed when an unrecognized flag is supplied'); +}); + +test('--json emits nothing on stderr and a parseable report on stdout', (t) => { + const clean = makeRepo(t, { '0001-alpha.md': adr('Alpha', ['**Status:** Accepted']) }); + assert.equal(run(clean, ['--write']).status, 0); + const cleanRun = run(clean, ['--json']); + assert.equal(cleanRun.status, 0, `a clean corpus must exit 0 under --json: ${cleanRun.stderr}`); + assert.equal(cleanRun.stderr, '', '--json must write nothing to stderr on a clean corpus'); + const cleanReport = JSON.parse(cleanRun.stdout); + assert.deepEqual(cleanReport, { ok: true, adrCount: 1, indexStale: false, violations: [] }); + + const violating = makeRepo(t, { '0001-alpha.md': adr('Alpha', ['**Status:** Draft']) }); + const violatingRun = run(violating, ['--json']); + assert.equal(violatingRun.status, 1, `a violating corpus must exit 1 under --json: ${violatingRun.stdout}`); + assert.equal(violatingRun.stderr, '', '--json must write nothing to stderr on a violating corpus'); + const violatingReport = JSON.parse(violatingRun.stdout); + assert.equal(violatingReport.ok, false); + assert.ok(violatingReport.violations.some((v) => v.reason === REASON.STATUS_INVALID && v.status === 'Draft')); + + // `--check --json` is identical to `--json` alone. + const combined = run(violating, ['--check', '--json']); + assert.equal(combined.status, 1); + assert.equal(combined.stderr, ''); + assert.deepEqual(JSON.parse(combined.stdout), violatingReport); +}); diff --git a/tests/helpers/hooks-dist.cjs b/tests/helpers/hooks-dist.cjs new file mode 100644 index 000000000..29f37fd52 --- /dev/null +++ b/tests/helpers/hooks-dist.cjs @@ -0,0 +1,35 @@ +'use strict'; + +/** + * Ensure hooks/dist is populated before any suite that reads it. + * hooks/dist/ is gitignored and only produced by `npm run build:hooks`. + * In CI the scoped/windows test jobs do NOT run build:hooks before running + * tests, so the first test that needs hooks/dist would fail. This mirrors + * the pattern used in bug-3357-codex-legacy-hooks-json-migration.test.cjs. + * + * Idempotent: only rebuilds when the directory is absent or empty of .js + * files. Extracted from six behaviorally-identical copies that had + * accumulated across tests/install.test.cjs (x2) and + * tests/install-minimal-hooks.test.cjs (x4) — see #2704's Failure B, where a + * seventh suite (tests/mcp-catalog-parity.install.test.cjs) needed the same + * guard but had no copy of its own, and so failed only on lanes where no + * other suite happened to build hooks/dist first. + */ + +const fs = require('node:fs'); +const path = require('node:path'); +const { runNode } = require('./process-seam.cjs'); +const { throwIfFailed } = require('./git-fixture.cjs'); +const { BUILD_TIMEOUT_MS } = require('./timeouts.cjs'); + +const REPO_ROOT = path.resolve(__dirname, '..', '..'); +const HOOKS_DIST_DIR = path.join(REPO_ROOT, 'hooks', 'dist'); +const BUILD_HOOKS_SCRIPT = path.join(REPO_ROOT, 'scripts', 'build-hooks.js'); + +function ensureHooksDist() { + if (!fs.existsSync(HOOKS_DIST_DIR) || fs.readdirSync(HOOKS_DIST_DIR).filter((f) => f.endsWith('.js')).length === 0) { + throwIfFailed(runNode([BUILD_HOOKS_SCRIPT], { timeoutMs: BUILD_TIMEOUT_MS }), `node ${BUILD_HOOKS_SCRIPT}`); + } +} + +module.exports = { ensureHooksDist, HOOKS_DIST_DIR, BUILD_HOOKS_SCRIPT }; diff --git a/tests/install-minimal-hooks.test.cjs b/tests/install-minimal-hooks.test.cjs index 36cba1f67..2d6771167 100644 --- a/tests/install-minimal-hooks.test.cjs +++ b/tests/install-minimal-hooks.test.cjs @@ -1237,22 +1237,9 @@ const { test, describe, before } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); -const { runNode } = require('./helpers/process-seam.cjs'); -const { throwIfFailed } = require('./helpers/git-fixture.cjs'); +const { ensureHooksDist } = require('./helpers/hooks-dist.cjs'); const REPO_ROOT = path.resolve(__dirname, '..'); -const HOOKS_DIST_DIR = path.join(REPO_ROOT, 'hooks', 'dist'); -const BUILD_HOOKS_SCRIPT = path.join(REPO_ROOT, 'scripts', 'build-hooks.js'); - -// #3145: class-norm timeout, not a per-suite value — see helpers/timeouts.cjs. -const { BUILD_TIMEOUT_MS: BUILD_HOOKS_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); - -/** Idempotently ensure hooks/dist contains built .js files. */ -function ensureHooksDist() { - if (!fs.existsSync(HOOKS_DIST_DIR) || fs.readdirSync(HOOKS_DIST_DIR).filter(f => f.endsWith('.js')).length === 0) { - throwIfFailed(runNode([BUILD_HOOKS_SCRIPT], { timeoutMs: BUILD_HOOKS_TIMEOUT_MS }), `node ${BUILD_HOOKS_SCRIPT}`); - } -} before(() => { ensureHooksDist(); @@ -1680,8 +1667,7 @@ const { test, describe, before, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); -const { runNode } = require('./helpers/process-seam.cjs'); -const { throwIfFailed } = require('./helpers/git-fixture.cjs'); +const { ensureHooksDist } = require('./helpers/hooks-dist.cjs'); const { install } = require('../bin/install.js'); const { createTempDir, cleanup } = require('./helpers.cjs'); @@ -1694,23 +1680,6 @@ const { createTempDir, cleanup } = require('./helpers.cjs'); // every "expected AfterTool/PostToolUse/BeforeTool/PreToolUse hooks" assertion // fails. This mirrors the pattern in bug-376-claude-js-hook-gsd-rewriter.test.cjs. -const REPO_ROOT = path.resolve(__dirname, '..'); -const HOOKS_DIST_DIR = path.join(REPO_ROOT, 'hooks', 'dist'); -const BUILD_HOOKS_SCRIPT = path.join(REPO_ROOT, 'scripts', 'build-hooks.js'); - -// #3145: class-norm timeout, not a per-suite value — see helpers/timeouts.cjs. -const { BUILD_TIMEOUT_MS: BUILD_HOOKS_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); - -/** - * Idempotently ensure hooks/dist contains built .js files. - * Runs build-hooks.js only when the directory is absent or empty of .js files. - */ -function ensureHooksDist() { - if (!fs.existsSync(HOOKS_DIST_DIR) || fs.readdirSync(HOOKS_DIST_DIR).filter(f => f.endsWith('.js')).length === 0) { - throwIfFailed(runNode([BUILD_HOOKS_SCRIPT], { timeoutMs: BUILD_HOOKS_TIMEOUT_MS }), `node ${BUILD_HOOKS_SCRIPT}`); - } -} - before(() => { ensureHooksDist(); }); @@ -2996,22 +2965,9 @@ const { test, describe, before } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); -const { runNode } = require('./helpers/process-seam.cjs'); -const { throwIfFailed } = require('./helpers/git-fixture.cjs'); +const { ensureHooksDist } = require('./helpers/hooks-dist.cjs'); const REPO_ROOT = path.resolve(__dirname, '..'); -const HOOKS_DIST_DIR = path.join(REPO_ROOT, 'hooks', 'dist'); -const BUILD_HOOKS_SCRIPT = path.join(REPO_ROOT, 'scripts', 'build-hooks.js'); - -// #3145: class-norm timeout, not a per-suite value — see helpers/timeouts.cjs. -const { BUILD_TIMEOUT_MS: BUILD_HOOKS_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); - -/** Idempotently ensure hooks/dist contains built .js files. */ -function ensureHooksDist() { - if (!fs.existsSync(HOOKS_DIST_DIR) || fs.readdirSync(HOOKS_DIST_DIR).filter(f => f.endsWith('.js')).length === 0) { - throwIfFailed(runNode([BUILD_HOOKS_SCRIPT], { timeoutMs: BUILD_HOOKS_TIMEOUT_MS }), `node ${BUILD_HOOKS_SCRIPT}`); - } -} before(() => { ensureHooksDist(); @@ -3439,8 +3395,7 @@ const { test, describe, before, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); -const { runNode } = require('./helpers/process-seam.cjs'); -const { throwIfFailed } = require('./helpers/git-fixture.cjs'); +const { ensureHooksDist } = require('./helpers/hooks-dist.cjs'); const { install } = require('../bin/install.js'); const { createTempDir, cleanup } = require('./helpers.cjs'); @@ -3453,23 +3408,6 @@ const { createTempDir, cleanup } = require('./helpers.cjs'); // every "expected AfterTool/PostToolUse/BeforeTool/PreToolUse hooks" assertion // fails. This mirrors the pattern in bug-376-claude-js-hook-gsd-rewriter.test.cjs. -const REPO_ROOT = path.resolve(__dirname, '..'); -const HOOKS_DIST_DIR = path.join(REPO_ROOT, 'hooks', 'dist'); -const BUILD_HOOKS_SCRIPT = path.join(REPO_ROOT, 'scripts', 'build-hooks.js'); - -// #3145: class-norm timeout, not a per-suite value — see helpers/timeouts.cjs. -const { BUILD_TIMEOUT_MS: BUILD_HOOKS_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); - -/** - * Idempotently ensure hooks/dist contains built .js files. - * Runs build-hooks.js only when the directory is absent or empty of .js files. - */ -function ensureHooksDist() { - if (!fs.existsSync(HOOKS_DIST_DIR) || fs.readdirSync(HOOKS_DIST_DIR).filter(f => f.endsWith('.js')).length === 0) { - throwIfFailed(runNode([BUILD_HOOKS_SCRIPT], { timeoutMs: BUILD_HOOKS_TIMEOUT_MS }), `node ${BUILD_HOOKS_SCRIPT}`); - } -} - before(() => { ensureHooksDist(); }); diff --git a/tests/install.test.cjs b/tests/install.test.cjs index ff1e8d9a1..ac203bc8f 100644 --- a/tests/install.test.cjs +++ b/tests/install.test.cjs @@ -5502,32 +5502,16 @@ const path = require('node:path'); const { runNode } = require('./helpers/process-seam.cjs'); const { throwIfFailed } = require('./helpers/git-fixture.cjs'); const { cleanup } = require('./helpers.cjs'); +const { ensureHooksDist, HOOKS_DIST_DIR } = require('./helpers/hooks-dist.cjs'); const REPO_ROOT = path.resolve(__dirname, '..'); const INSTALL_PATH = path.join(REPO_ROOT, 'bin', 'install.js'); -const HOOKS_DIST_DIR = path.join(REPO_ROOT, 'hooks', 'dist'); -const BUILD_HOOKS_SCRIPT = path.join(REPO_ROOT, 'scripts', 'build-hooks.js'); // #3145: class-norm timeouts, not per-suite values — see helpers/timeouts.cjs. const { - BUILD_TIMEOUT_MS: BUILD_HOOKS_TIMEOUT_MS, INSTALL_TIMEOUT_MS, } = require('./helpers/timeouts.cjs'); -/** - * Ensure hooks/dist is populated before any suite that reads it. - * hooks/dist/ is gitignored and only produced by `npm run build:hooks`. - * In CI the scoped/windows test jobs do NOT run build:hooks before running - * tests, so the first test that needs hooks/dist would fail. This mirrors - * the pattern used in bug-3357-codex-legacy-hooks-json-migration.test.cjs. - */ -function ensureHooksDist() { - if (!fs.existsSync(HOOKS_DIST_DIR) || fs.readdirSync(HOOKS_DIST_DIR).filter(f => f.endsWith('.js')).length === 0) { - const r = runNode([BUILD_HOOKS_SCRIPT], { timeoutMs: BUILD_HOOKS_TIMEOUT_MS }); - throwIfFailed(r, `node ${BUILD_HOOKS_SCRIPT}`); - } -} - // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- @@ -9567,32 +9551,16 @@ const path = require('node:path'); const { runNode } = require('./helpers/process-seam.cjs'); const { throwIfFailed } = require('./helpers/git-fixture.cjs'); const { cleanup } = require('./helpers.cjs'); +const { ensureHooksDist, HOOKS_DIST_DIR } = require('./helpers/hooks-dist.cjs'); const REPO_ROOT = path.resolve(__dirname, '..'); const INSTALL_PATH = path.join(REPO_ROOT, 'bin', 'install.js'); -const HOOKS_DIST_DIR = path.join(REPO_ROOT, 'hooks', 'dist'); -const BUILD_HOOKS_SCRIPT = path.join(REPO_ROOT, 'scripts', 'build-hooks.js'); // #3145: class-norm timeouts, not per-suite values — see helpers/timeouts.cjs. const { - BUILD_TIMEOUT_MS: BUILD_HOOKS_TIMEOUT_MS, INSTALL_TIMEOUT_MS, } = require('./helpers/timeouts.cjs'); -/** - * Ensure hooks/dist is populated before any suite that reads it. - * hooks/dist/ is gitignored and only produced by `npm run build:hooks`. - * In CI the scoped/windows test jobs do NOT run build:hooks before running - * tests, so the first test that needs hooks/dist would fail. This mirrors - * the pattern used in bug-3357-codex-legacy-hooks-json-migration.test.cjs. - */ -function ensureHooksDist() { - if (!fs.existsSync(HOOKS_DIST_DIR) || fs.readdirSync(HOOKS_DIST_DIR).filter(f => f.endsWith('.js')).length === 0) { - const r = runNode([BUILD_HOOKS_SCRIPT], { timeoutMs: BUILD_HOOKS_TIMEOUT_MS }); - throwIfFailed(r, `node ${BUILD_HOOKS_SCRIPT}`); - } -} - // --------------------------------------------------------------------------- // Helpers // --------------------------------------------------------------------------- diff --git a/tests/mcp-catalog-parity.install.test.cjs b/tests/mcp-catalog-parity.install.test.cjs index 7e515b825..1056c1da1 100644 --- a/tests/mcp-catalog-parity.install.test.cjs +++ b/tests/mcp-catalog-parity.install.test.cjs @@ -55,7 +55,7 @@ * checks, and keeps this already-slow suite bounded. */ -const { describe, test } = require('node:test'); +const { describe, test, before } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const os = require('node:os'); @@ -65,6 +65,7 @@ const { runNode } = require('./helpers/process-seam.cjs'); const { cleanup } = require('./helpers.cjs'); const { runMinimalInstall, installerEnv } = require('./helpers/install-shared.cjs'); const { buildOverlayRepo } = require('./helpers/overlay-repo.cjs'); +const { ensureHooksDist } = require('./helpers/hooks-dist.cjs'); const { buildCatalog, readResource, shouldCompose } = require('../gsd-core/bin/lib/mcp-catalog.cjs'); @@ -74,6 +75,16 @@ const MARKER_TOKEN = 'gsd:section'; // #3145: class-norm timeout, not a per-suite value — see helpers/timeouts.cjs. const { INSTALL_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +// hooks/dist/ is gitignored and only produced by `npm run build:hooks`. This +// suite's real spawned install (runMinimalInstall) and overlay builds +// (buildOverlayRepo, which hard-links every leaf under REPO_ROOT including +// hooks/dist/) both need it populated. In CI the scoped/windows jobs do not +// run build:hooks first, so — absent this guard — the suite only passes when +// some OTHER suite happened to build hooks/dist first (see #2704 Failure B). +before(() => { + ensureHooksDist(); +}); + /** Recursively collect `.md` file paths under `absDir`, relative to `REPO_ROOT`, POSIX-normalized. */ function collectMarkdownFiles(absDir, out = []) { let entries;