Files
msd-core/scripts/lint-source-test-name-collision.cjs
Tom Boucher bbdf7e8e84 chore(#4654): add local/no-unconfined-path-join and drain it to zero — Phase 4 of #4636 (#4674)
* chore(#4654): add local/no-unconfined-path-join and drain it to zero

Phase 4 of epic #4636 — the ratchet, and the phase that makes the epic hold.

THE MEASUREMENT THAT RESHAPED THE PHASE. An AST census (the repo's own parser,
not grep) found what the epic never enumerated: ADR-4650 named seven containment
implementations; `src/` alone held roughly 24 more hand-rolled gates across ~13
files, several guarding a write or an `fs.rmSync`. Two verified by reading rather
than pattern-matching — `research-store.cts` comments its own as "ensure the
resolved file path stays inside the store dir" immediately before a write, and
`capability-lifecycle.cts` gates `fs.rmSync` with one.

So the epic's Done-when "one containment predicate, used at every site" was FALSE
when Phase 3 reported it satisfied. It is true now: the rule is clean across
src/, scripts/, gsd-core/bin/ and hooks/ with an EMPTY allowlist.

WHY NOT THE RULE THE ISSUE PROPOSED. #4654 proposed flagging `path.join` whose
first argument is a managed root and whose later arguments derive from argv. That
is a taint analysis over 2046 call sites, in ESLint, without type information;
"derives from argv" is not locally decidable. Any approximation either floods or
is trivially evaded, and a rule that fires on hundreds of correct sites earns an
allowlist of hundreds — the opposite of a ratchet. What is actually duplicated is
the COMPARISON, not the join, and that has one recognizable shape.

  Arm 1  X.startsWith(Y + sep)            the hand-rolled containment idiom
  Arm 2  a containment predicate called as a bare statement, answer discarded

Arm 2 is the issue's "asserts the result was narrowed, not merely that a helper
was called". Its example `validatePath(x, root).resolved` is already
structurally impossible — Phase 3 un-exported `validatePath` — so the remaining
expressible failure is ignoring the answer, which is the defect that recurred
five times in this epic. The census found exactly one live instance
(`milestone.cts:1643`); it now returns the proven `ContainedPath` so consumers
stop re-deriving the path the comment above it was extracted to stop them
re-deriving.

The rule deliberately does NOT try to catch validate-one-path-use-another where
the answer is used but a different variable flows onward. That needs flow
analysis; the branded `ContainedPath` from Phase 3 is the defense there, and the
two are complementary.

PER-SITE FAMILY CHOICE, NOT A DEFAULT. Phase 3's lesson binds: collapsing a
lexical site onto the realpath family broke four tests and was caught only by the
matrix. Every migrated site was triaged individually. The six
installer-migrations tree-walks and the six capability-lifecycle gates take the
LEXICAL family because their operands are already realpath-resolved and they
deliberately treat the final component as a link; boundary sites take realpath.

TWO SITES WITH AN INVERTED CONTRACT, which a mechanical swap would have broken.
`installer-migrations.cts:127` and `runtime-artifact-install-plan.cts:144` REJECT
`target === root` by contract, while the canonical comparison ACCEPTS it. Swapped
naively, a migration could `rmdir` the user's config root and a third-party
descriptor could write at configHome itself. Both keep `=== root` as an explicit
additional arm alongside the predicate call — the predicate decides containment,
the call site keeps its own extra condition (ADR-4650 decision 6).

ONE DUPLICATE DELETED OUTRIGHT: `planning-inspect.cts`'s `isWithinRoot` was
byte-identical to `isContainedIn` and said so in its own docstring.
`isContainedIn` is now exported for callers that have already resolved both
operands and need only the comparison, with a doc note that a caller which has
NOT resolved them must use a full predicate instead.

THE MARKER, AND WHY IT IS NOT THE ALLOWLIST. Nine sites are justified holdouts and
carry `// allow-handrolled-containment: <reason>` with a mandatory, reviewable
reason. Two justifications: (a) not a containment decision — an ancestor-walk loop
condition, sub-repo grouping, worktree identity matching, declared-path coverage;
(b) it IS containment but the canonical predicate is unreachable —
`capability-validator.cjs` is a committed pre-build `.cjs` and the compiled
`security.cjs` is untracked build output, so requiring it would break a fresh
clone. `scripts/lib/drift-scan.cjs` runs under `lint:ci` with the same exposure.
The marker was renamed from `allow-lexical-prefix-match` mid-phase because that
name asserted only (a) and would have stated something false at the (b) sites.

A marker suppresses BEFORE the violation counter increments, so a file whose
every occurrence is marked still reports `staleAllowlistEntry` — otherwise a
drained entry lingers and silently re-permits the site later.

DEMONSTRATED RED, per #4654: a hand-rolled copy reintroduced into a real `src/`
file made `npm run lint` fail with the rule's full guidance message; removing it
returned the tree to clean. Both halves recorded — red alone proves nothing,
since a rule red for an unrelated reason looks identical.

DISCLOSED: `defaultRequireFromInstallRoot` (gsd-tools.cjs) previously carried two
distinct rejection messages and two manual realpath calls; routing it through
`tryWithinRoot` collapses them to one message, and a missing module now surfaces
as MODULE_NOT_FOUND rather than ENOENT. No test asserts either message. The
security property is preserved and slightly strengthened — the candidate is
realpathed and containment re-checked, and the dangling-symlink oracle closure
comes along with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* docs(#4654): record the containment ratchet in CONTEXT.md and the security model

Both entries previously described the seam without the thing that keeps it a
seam. They now state what the rule bans, and — more usefully for whoever reads
this next — what it deliberately does NOT attempt: deciding per path.join call
whether an argument came from user input. That question is not locally
decidable, and an approximation across ~2000 join sites would earn an exemption
list of hundreds, which is the opposite of a ratchet.

Also records the marker's two legitimate justifications and that its reason is
mandatory, so the escape stays reviewable rather than becoming a mute button.

Glossary gate 270 refs exit 0; install-tree goldens and CONTEXT-INDEX.json
regenerated and confirmed byte-identical rather than assumed — which also
confirms eslint-rules/ is not a shipped path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#4654): close review findings and the two matrix failures

MATRIX FAILURE 1 — a collapsed message broke a negative-proof test, and my
evidence for collapsing it was wrong. I searched tests/ for the literal string
"resolves outside its install root", found nothing, and reported that no test
asserted it. The test matches a REGEX SUBSTRING, /outside its install root/, so
the literal search missed it. What broke was "NEGATIVE PROOF: a symlinked module
pointing OUTSIDE the install root is not loaded" — the test guarding the exact
property I claimed was preserved. defaultRequireFromInstallRoot now does both
checks again with both messages byte-identical, each routed through the
canonical predicate, which is better than the original since that hand-rolled
both comparisons.

MATRIX FAILURE 2 — shipped migrations are checksum-locked, and a marker cannot
serve there. migrationChecksum hashes plan.toString(), which INCLUDES comments,
so a suppression marker inside a plan body drifts the baseline exactly as an
edit does. Measured: with markers in place, two of the four still differed from
their committed checksums. The four shipped bodies are now byte-identical to
next, and the rule's config excludes those four paths BY NAME rather than by a
directory wildcard, so a NEW migration is still covered. Six containment
comparisons stay un-ratcheted there; that gap is recorded in the rule's Known
gaps, in CONTEXT.md and in the security model rather than left implicit.
Justification (c) is removed from the marker's documented reasons, because a
marker was proven unable to express it.

ADVERSARIAL REVIEW — the sharpest finding was that the rule banned the CORRECT
shape while permitting the incorrect one: startsWith(root) with no separator is
the genuinely unsafe form, since it accepts a sibling such as root-evil, and my
own test blessed it as valid. Flagging every bare startsWith would swamp the
rule, so that stays a STATED gap rather than a silent one. Closed for real: the
template-literal spelling, which the census never saw because it only inspected
plus-concatenation — that surfaced TWELVE more sites, now triaged and migrated.
A separator reached through a const alias is now resolved via scope analysis.
And isContainedIn, exported in Phase 3, was missing from the discarded-result
set, so a bare no-op call went unflagged on the one function the epic funnels
through.

SECURITY REVIEW — the marker could over-suppress two ways: a block comment
worked identically to a line comment, and one marker silently covered every
violation sharing its line. It now requires a Line comment positioned after the
flagged node ends, so it anchors to the node it trails. Four sites had dropped
an unreachable-but-deliberate equality rejection against the root; each is
restored as the call site's own arm. eslint.config.mjs still documented the OLD
marker token, which my rename missed — it would have sent the next author in
circles.

A FALSE GREEN, recorded because it nearly stuck: lint:ci reported exit 0 from a
stale eslint cache while twelve real violations existed. Every lint check here
now clears the cache first.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* fix(#4654): anchor a suppression marker to the violation it actually trails

The matrix caught this; my own test caught it, on its first execution. The case
"two violations on one line: trailing marker suppresses only the one it trails"
expected 1 error and got 0 — both were suppressed.

ROOT CAUSE: the anchoring accepted any Line comment on the node's line whose
range started at or after the node's end. A trailing marker at the END of a line
sits after EVERY node on that line, so that condition held for all of them.
"After the node" does not identify WHICH node the marker trails. The fix reads
as correct and is not.

FIX: deferred reporting. Violations accumulate during traversal instead of being
reported immediately; at Program:exit each marker claims exactly ONE pending
violation — the one on its line whose end is nearest before the marker begins —
and every unclaimed violation is then counted and reported. One marker, one
suppression. An earlier violation sharing the line is still reported, which is
the property the security review asked for and the previous attempt only
appeared to deliver.

The counter now increments at flush time rather than during traversal, so a
suppressed occurrence still does not keep an allowlist entry alive.

AND A TOOL THAT SHOULD HAVE EXISTED BEFORE THE FIRST MATRIX RUN. `node --test`
is hard-blocked here, so this rule's test file could only ever be executed on
the remote matrix — which is why a broken anchoring shipped into a run. ESLint's
programmatic Linter API is not a test runner, and exercising the rule through it
verifies every case locally in seconds. All 24 now pass locally, including the
two-on-one-line case that failed remotely. That loop should have been built
before the rule was first sent to the matrix rather than after it failed twice.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* chore(#4654): backfill PR 4674 into the changeset and complete 70-docs.json

The phase gate requires enablementSequence and the Diataxis quadrants; 70-docs
now carries both, with the how-to quadrant skipped for a stated reason rather
than an empty field. The audience for this deliverable is a contributor who
trips the rule, and the task-oriented guidance reaches them in the ESLint
message itself — which names the correct predicate, says how to choose between
the realpath and lexical families, cites the Phase 3 regression caused by
choosing wrong, and gives the marker syntax. A docs/how-to page would be a
second, driftable copy read by nobody at the moment of failure.

enablementSequence is recorded as what it actually is: a VERIFICATION sequence,
not an enablement one. The rule is never off, so there is no off-to-on
transition to describe.

scripts/lint-docs-required.cjs now passes (ok_docs_updated) — it could not
evaluate against the mandated pr:0 placeholder.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
2026-09-12 22:17:46 -04:00

242 lines
10 KiB
JavaScript

#!/usr/bin/env node
'use strict';
/**
* lint-source-test-name-collision.cjs — no SOURCE file may carry a basename
* that matches Node's built-in test-file collection patterns.
*
* ## Why (the incident)
*
* `src/test-home-guard.cts` was a SOURCE module (a runtime guard, not a
* test) whose filename happened to match Node's `test-*` collection
* convention. `scripts/run-tests.cjs` — what local `npm test` and GitHub CI
* use — globs only `tests/**\/*.test.cjs`, so CI never saw it. But the
* REMOTE test runner (the push gate; see CLAUDE.md's `gsd-test` section)
* collects test files the way `node --test` does by default, across the
* whole tree, so it picked the file up and executed it AS a test, where it
* exited 1. Net effect: `next` was green on GitHub CI and red on the push
* gate, blocking every push repo-wide. Proven by a control run on the
* unmodified `next` tip 622f43353: `37199 passed / 1 failed`,
* `throw · src/test-home-guard.cts`. The file has since been renamed to
* `src/real-home-guard.cts` in this branch; this lint exists so a source
* file can never silently re-acquire a collectable name again.
*
* ## What "collectable" means
*
* Node's test runner (`node --test`, and by extension the remote runner
* that dispatches this repo's push-gate suite) collects any file whose path
* matches, by default:
*
* **\/*.test.?(c|m)js **\/*-test.?(c|m)js **\/*_test.?(c|m)js
* **\/test-*.?(c|m)js **\/test.?(c|m)js **\/test/**
*
* Node also resolves `.ts`/`.cts`/`.mts` through the same collector once
* type-stripping is active (unflagged since Node 23.6; this repo's
* `engines.node` floor is >=24) — which is exactly how a `.cts` file ended
* up collected in the incident above. This lint therefore checks each of
* the six extensions `js`, `cjs`, `mjs`, `ts`, `cts`, `mts` against each
* basename-shaped pattern, plus a path-based check for any file living
* inside a directory literally named `test` (not `tests` — this repo's own
* test directory is deliberately outside the scanned source dirs, see
* below).
*
* ## Scanned (source/shipped) directories
*
* src/, scripts/, hooks/, bin/, gsd-core/bin/ (excluding
* gsd-core/bin/lib/**, see below), eslint-rules/
*
* ## Exempted
*
* - tests/ — files there are SUPPOSED to match; that is the point.
* - node_modules/, .git/ — never source we own.
* - gsd-core/bin/lib/** — build output generated from src/*.cts by
* `npm run build:lib` (tsc), and gitignored (verified: every file under
* it, including the incident's own post-fix
* `gsd-core/bin/lib/real-home-guard.cjs`, is listed in .gitignore). A
* generated file inherits its source's basename 1:1, so scanning it
* would double-report the exact same defect `src/` already caught —
* noise, not signal. `gsd-core/bin/shared/*.json` is data, not code,
* but is harmlessly included since it never matches a JS/TS extension.
*
* Exported pure(ish) function `checkSourceTestNameCollisions({ dirs, root })`
* so tests can drive it against synthetic fixture directories; also runnable
* as a CLI against the real tree
* (`node scripts/lint-source-test-name-collision.cjs`).
*/
const fs = require('fs');
const path = require('path');
const { ExitError, runMain } = require('./lib/cli-exit.cjs');
// The six extensions Node's test runner collects, per the incident: the
// documented `?(c|m)js` set (js, cjs, mjs) plus the TypeScript-loader
// equivalents (ts, cts, mts) that the same collector resolves once
// type-stripping is active (unflagged since Node 23.6; this repo's
// engines.node floor is >=24, per package.json).
const COLLECTED_EXTENSIONS = ['js', 'cjs', 'mjs', 'ts', 'cts', 'mts'];
const EXT_ALT = COLLECTED_EXTENSIONS.join('|');
// Basename-shaped patterns, translated 1:1 from Node's documented defaults:
// **/*.test.?(c|m)js **/*-test.?(c|m)js **/*_test.?(c|m)js
// **/test-*.?(c|m)js **/test.?(c|m)js
// (the sixth default, **/test/**, is a path-shaped check — see
// isUnderLiteralTestDir below, not a basename regex.)
const BASENAME_PATTERNS = [
{ name: '*.test.EXT', re: new RegExp(`\\.test\\.(?:${EXT_ALT})$`) },
{ name: '*-test.EXT', re: new RegExp(`-test\\.(?:${EXT_ALT})$`) },
{ name: '*_test.EXT', re: new RegExp(`_test\\.(?:${EXT_ALT})$`) },
{ name: 'test-*.EXT', re: new RegExp(`^test-.*\\.(?:${EXT_ALT})$`) },
{ name: 'test.EXT', re: new RegExp(`^test\\.(?:${EXT_ALT})$`) },
];
/**
* Source/shipped directories this guard checks, relative to repo root.
* Confirmed against the repo layout: src/, scripts/, hooks/, bin/,
* gsd-core/bin/, eslint-rules/ all ship first-party source or shipped
* tooling; nothing else at the top level carries executable source outside
* tests/.
*/
const DEFAULT_SCAN_DIRS = ['src', 'scripts', 'hooks', 'bin', 'gsd-core/bin', 'eslint-rules'];
// Directories to never descend into anywhere in the tree.
const ALWAYS_EXCLUDE_DIR_NAMES = new Set(['node_modules', '.git']);
// Relative dir prefixes (POSIX-joined, relative to repo root) that are
// generated build output and must not be scanned — see the module doc for
// why gsd-core/bin/lib is excluded (it 1:1-inherits src/*.cts basenames, so
// scanning it double-reports the same defect src/ already catches).
const GENERATED_OUTPUT_PREFIXES = ['gsd-core/bin/lib'];
function toPosix(p) {
return p.split(path.sep).join('/');
}
function isGeneratedOutput(relPath) {
const posixRel = toPosix(relPath);
return GENERATED_OUTPUT_PREFIXES.some(
(prefix) => posixRel === prefix || posixRel.startsWith(`${prefix}/`) // allow-handrolled-containment: scan-exclusion membership test against a fixed generated-output prefix list, not a filesystem root-confinement gate
);
}
/**
* Node's **\/test/** default: any file living inside a directory literally
* named `test` (singular) anywhere in its path. Deliberately does NOT match
* `tests/` (plural) — this repo's real test directory is a sibling of the
* scanned source dirs, not nested inside one, and is never itself scanned.
*/
function isUnderLiteralTestDir(relPath) {
return toPosix(relPath).split('/').slice(0, -1).includes('test');
}
function matchingBasenamePatterns(basename) {
return BASENAME_PATTERNS.filter((p) => p.re.test(basename)).map((p) => p.name);
}
function walk(dir, root, out) {
let entries;
try {
entries = fs.readdirSync(dir, { withFileTypes: true });
} catch (err) {
// A scan dir that cannot be read must never be silently treated as
// "zero files, zero violations" — that would be a green check that
// guarded nothing (same class of bug as an empty registry elsewhere in
// this repo's lints).
out.unreadable.push({ dir: path.relative(root, dir) || dir, error: err.message });
return;
}
for (const entry of entries) {
const full = path.join(dir, entry.name);
const rel = path.relative(root, full);
if (entry.isDirectory()) {
if (ALWAYS_EXCLUDE_DIR_NAMES.has(entry.name)) continue;
if (isGeneratedOutput(rel)) continue;
walk(full, root, out);
} else if (entry.isFile()) {
out.scanned.push(rel);
}
}
}
/**
* @param {{ dirs?: string[], root: string }} opts
* `dirs` — scan dirs relative to `root` (defaults to DEFAULT_SCAN_DIRS).
* `root` — the directory `dirs` are resolved against (repo root for the
* real CLI run; a synthetic fixture root in tests).
* @returns {{ ok: boolean, violations: Array<{file:string, patterns:string[], reason:string}>, scanned: string[] }}
*/
function checkSourceTestNameCollisions({ dirs = DEFAULT_SCAN_DIRS, root }) {
const violations = [];
const out = { scanned: [], unreadable: [] };
for (const dir of dirs) {
const full = path.join(root, dir);
if (!fs.existsSync(full)) continue; // a configured dir that doesn't exist is not this lint's problem
walk(full, root, out);
}
if (out.unreadable.length > 0) {
return {
ok: false,
violations: out.unreadable.map((u) => ({
file: u.dir,
patterns: [],
reason: `cannot read scan directory ${u.dir}: ${u.error} — a collision guard that cannot ` +
'read its own input must fail, never silently report zero violations',
})),
scanned: out.scanned,
};
}
for (const rel of out.scanned) {
const basename = path.basename(rel);
const patterns = matchingBasenamePatterns(basename);
const underTestDir = isUnderLiteralTestDir(rel);
if (patterns.length === 0 && !underTestDir) continue;
const matched = underTestDir ? [...patterns, '**/test/**'] : patterns;
violations.push({
file: toPosix(rel),
patterns: matched,
reason:
`basename matches Node's test-collection pattern(s) [${matched.join(', ')}] — the remote ` +
'test runner (the push gate) collects files by this convention and executes them AS ' +
'tests, while GitHub CI (scripts/run-tests.cjs) globs only tests/**/*.test.cjs and never ' +
'sees it; the failure then appears ONLY at the push gate (incident: src/test-home-guard.cts, ' +
'control run on next tip 622f43353: 37199 passed / 1 failed, throw · src/test-home-guard.cts). ' +
'Remedy: rename the file out of the pattern; do NOT add a runner-side exclusion.',
});
}
violations.sort((a, b) => a.file.localeCompare(b.file));
return { ok: violations.length === 0, violations, scanned: out.scanned };
}
module.exports = {
checkSourceTestNameCollisions,
COLLECTED_EXTENSIONS,
DEFAULT_SCAN_DIRS,
GENERATED_OUTPUT_PREFIXES,
isUnderLiteralTestDir,
matchingBasenamePatterns,
};
function main() {
const ROOT = path.join(__dirname, '..');
const result = checkSourceTestNameCollisions({ root: ROOT });
if (!result.ok) {
process.stderr.write(
`lint-source-test-name-collision: ${result.violations.length} violation(s) among ${result.scanned.length} scanned file(s)\n\n`
);
for (const v of result.violations) {
process.stderr.write(` ${v.file}\n ${v.reason}\n`);
}
throw new ExitError(1);
}
console.log(`ok lint-source-test-name-collision: ${result.scanned.length} file(s) scanned, 0 violations`);
}
if (require.main === module) runMain(main);