64 Commits

Author	SHA1	Message	Date
Jakub Zych	fe3ed06691	chore: clear dead test and allowlist leftovers of dropped runtimes	2026-10-06 20:35:12 +02:00
Jakub Zych	792139b5ed	chore: sweep Kimi mentions from comments and notes	2026-10-06 20:19:17 +02:00
Jakub Zych	a9a7a328e6	refactor: hard-fork GSD -> MSD (Make Software Done) ... Mechanical rename produced by scripts/msd-rename.cjs: gsd/Gsd/GSD -> msd/Msd/MSD across contents and paths, upstream package/repo coordinates -> @golem15/msd-core and golem15com/msd-core. Deep links into upstream history, sibling upstream packages, the GSD-2 import feature, CHANGELOG.md and .changeset/ are kept as-is. Hand edits on top: MSD block-letter banner and logos, LICENSE copyright line, package/plugin identity, regenerated lockfile, install-tree fixtures, derived registries and benchmark baseline; migration checksum baseline re-locked (MSD keeps its own install state, so no install had applied the old sums); sort-order and regex-escaped expectations in tests adjusted.	2026-10-06 01:47:40 +02:00
Tom Boucher	ccb39aec15	test(#4528): migrate final seam-dispatch batch and retire the timeout-literal allowlist (#4684) ... Batch 17 of 17 — the terminal batch — in the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/cjs-command-router-adapter.test.cjs, tests/dispatcher.test.cjs, tests/run-tests-temp-root.test.cjs, and tests/shell-command-projection-dispatch.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. No src/bin file touched, no numeric value changed anywhere. Eslint ground truth (9 sites) matches the issue's own stated count exactly for the first time in this epic — no drift to disclose. Reuses PROBE_TIMEOUT_MS (1 site) and QUICK_SPAWN_TIMEOUT_MS (1 site). Adds four file-local constants for shapes with no existing match: RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS and RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS (run-tests-temp-root.test.cjs, distinguishing a `node -e` isolated function call from a real end-to-end spawn of the test runner itself, despite each coinciding numerically with an unrelated existing constant), and EXEC_TOOL_OPTION_PASSTHROUGH_TIMEOUT_MS and DISPATCH_FORCED_TIMEOUT_MS (shell-command-projection-dispatch.test.cjs — a mocked-spawnSync pass-through fixture and a deliberately-forced real timeout, neither a real subprocess bound in the usual sense). Terminal-batch cleanup: deletes eslint-rules/no-adhoc-timeout-literal.allowlist.json entirely, drops its require and the allowlist option from eslint.config.mjs's local/no-adhoc-timeout-literal registration (now a bare 'error', mirroring local/no-unbounded-spawn's own already-terminal configuration in the same file), and updates TESTING-STANDARDS.md's enforcement note to match — a stale pointer to the deleted file caught by review, fixed inline. A full-repo eslint run with no cache confirms zero violations anywhere in the tree under the now allowlist-free rule. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-13 03:44:51 -04:00
Tom Boucher	a841575037	test(#4527): migrate planning/review-lane batch to named timeout constants (#4680) ... Batch 16 of 17 in the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in 9 test files with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. No src/bin file touched, no numeric value changed anywhere. Ground truth via eslint found 48 sites, not the issue's stated 38 — tests/code-review.test.cjs alone has 11, not 1 (a 10-site undercount, the largest single-file drift in this epic). All 11 are migrated. Reuses PROBE_TIMEOUT_MS (17 sites across assumption-delta.test.cjs and code-review.test.cjs), LOOP_HOOK_POINT_CLI_TIMEOUT_MS (1 site), QUICK_SPAWN_TIMEOUT_MS (1 site). Adds a new shared constant, HTTP_REACHABLE_PROBE_TIMEOUT_FIXTURE_MS, promoted because two files (reviewer-manifest-body.test.cjs, reviewer-trust-disclosure.test.cjs) independently arrived at the same probe-fixture value across 3 sites. File-local constants elsewhere for values not shared across files: FALLOW_AUDIT_TIMEOUT_MS (code-review-pipeline-regression.test.cjs); a 5-constant set covering runBashScript's own override/forced-timeout/ boundary-triple tests (plan-phase-stall-detection.test.cjs); 9 lane-specific NATIVE_TIMEOUT_MS constants, one per shipped reviewer CLI tool, even where 4 lanes coincidentally share a value (review-lane-invocation.test.cjs); and a 6-constant set covering reviewer-manifest-body.test.cjs's own probe-kind fixtures and its separate, unrelated boundary/invalid set that coincidentally overlaps in shape (not value) with plan-phase-stall-detection.test.cjs's triple. Fixes applied inline from review: GENERATOR_SCRIPT_TIMEOUT_MS was misapplied to 3 sites in plan-review-convergence.test.cjs whose actual shape (nested bash -> node -> gsd-tools.cjs chain) doesn't match that constant's documented direct-spawn class, confirmed against its own cited precedent files — replaced with a new file-local constant naming the correct class, same value. Two stray un-renamed literals in review-lane-invocation.test.cjs (missed by eslint's object-literal-only detection) were investigated and left as disclosed literals with an explanatory comment rather than force-fit onto an unrelated lane's constant, since they belong to a distinct config-override code path. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-13 02:35:23 -04:00
Tom Boucher	2a5d919a03	test(#4526): migrate gate/predicate evaluators batch to named timeout constants (#4679) ... Batch 15 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/check-predicate.test.cjs, tests/gate-predicate-evaluator.test.cjs, tests/policy-160-route0-resume.test.cjs, tests/phase6-capstone-conformance.test.cjs, and tests/prohibition-enforcement.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 5 files from the rule's allowlist. Ground truth via eslint found 17 sites, not the issue's stated 16 -- prohibition-enforcement.test.cjs has 2 sites, not 1 -- disclosed in the PR body. Reuses QUICK_SPAWN_TIMEOUT_MS and LOOP_HOOK_POINT_CLI_TIMEOUT_MS across 1 file; no new shared constants needed. Adds file-local constants for a real bounded-shell subprocess class (including one deliberately non-generous value to force a timeout), a predicate's own declarative timeout FIELD as fixture/validation data (including a deliberately-invalid zero/negative pair proving rejection), a heavier gsd-tools.cjs check CLI dispatch tier, and two distinct deliberately-short enforcement bounds forcing a fast hang-timeout path. No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-13 01:22:08 -04:00
Tom Boucher	0cee0eee47	test(#4525): migrate statusline/teams batch to named timeout constants (#4677) ... Batch 14 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/gsd-statusline.test.cjs and tests/teams-status.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 2 files from the rule's allowlist. Issue #4525 cautions against forcing either file onto PROBE_TIMEOUT_MS without checking, since both invoke a "long-lived status renderer." Checking each file's actual production handler separately found the caution applies to one file and not the other: gsd-statusline.test.cjs's 7 sites all spawn hooks/gsd-statusline.js, which does real rendering work (context-window, git, teams state) -- two new file-local constants (STATUSLINE_HOOK_TIMEOUT_MS=4000, 6 sites; STATUSLINE_HOOK_GIT_SHIM_TIMEOUT_MS =5000, 1 site for a heavier git-shim test). teams-status.test.cjs's 5 sites spawn `gsd-tools.cjs query teams-status`, whose handler (gsd-core/bin/lib/teams-status.cjs's cmdTeamsStatus) is a lightweight env-truthiness check with no rendering or fan-out -- genuinely matching PROBE_TIMEOUT_MS's class, confirmed by reading the handler directly rather than assumed from the matching value. No new shared-helper constant is needed this batch. No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-13 00:23:50 -04:00
Tom Boucher	56b706a07a	test(#4524): migrate task-content resolution batch to named timeout constants (#4675) ... Batch 13 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeoutMs object-literal property in tests/task-content-resolution.test.cjs, tests/task-command-router-resolve-content.test.cjs, and tests/task-content-resolver-grammar-parity.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 3 files from the rule's allowlist. Every one of the 13 sites describes the same field -- a task-content- resolver manifest's invoke.timeoutMs -- as fixture/validation data; none is a real subprocess spawn timeout, verified by tracing each site to a pure function, a garbage-shape rejection path, or a fully-injected fake exec function. Adds one new shared constant, TASK_RESOLVER_INVOKE_TIMEOUT_MS, used by 2 files in this batch (crossing the promotion bar). Adds file-local constants for a value used by only 1 file, plus three deliberately-invalid values (zero, negative, non-integer) inside one findResolver garbage-shapes test proving the validator rejects a malformed manifest regardless of which way its timeout is invalid. Also fixes a review-caught defect outside the mechanical rule's own scope: a bare-literal duplicate of the new shared constant inside a ResolverTimeoutError assertion (a call argument, not an object-literal property, so the lint rule never flagged it) -- renamed in this same PR per the no-deferrals rule. No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-12 23:21:21 -04:00
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
Tom Boucher	d6788a6805	test(#4523): migrate config/env/locking/perf batch to named timeout constants (#4673) ... Batch 12 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/check-env.test.cjs, tests/config-get-default.test.cjs, tests/federated-config.test.cjs, tests/gsd-check-update-worker-platform-gate.test.cjs, tests/gsd-mcp-server-bin.test.cjs, tests/health-validation.test.cjs, tests/locking-bugs-1909-1916-1925-1927.test.cjs, tests/perf-316-state-lock-buffer-alloc.test.cjs, tests/perf-317-context-monitor-fs.test.cjs, and tests/pi-config-dir-env-override.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 10 files from the rule's allowlist. Ground truth via eslint matched the issue's stated 26 sites across 10 files exactly. Reuses PROBE_TIMEOUT_MS, GENERATOR_SCRIPT_TIMEOUT_MS, GSD_TOOLS_CLI_MODERATE_TIMEOUT_MS, and INSTALL_TIMEOUT_MS across 4 files. No new shared constants needed -- every recurring value across files was independently verified to be a genuinely different operation class, per this migration's standing rule that numeric coincidence is never identity. Adds 11 new file-local constants, three of which are not real subprocess timeouts at all (a config-merge fixture value, and two node:test per-test timeout options bounding ReDoS/lock-retry regression backstops). Per the issue's explicit mandate, gsd-check-update-worker-platform-gate.test.cjs now imports (read-only) NPM_VIEW_TIMEOUT_MS from gsd-core/bin/check-latest-version.cjs for disclosure -- this file and that production module once independently guessed the same 15000ms value, causing the PR #4428 Windows double-SIGKILL collision. No site in this file's current bare literals actually wraps a live npm-view call needing margin arithmetic, so the import documents the historical relationship honestly rather than fabricating a computation. No src/bin file edited (only a read-only import added), no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-12 21:33:26 -04:00
Tom Boucher	c99d7bb2be	test(#4522): migrate core CLI/domain state batch to named timeout constants (#4662) ... Batch 11 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/state-document.test.cjs, tests/phase.test.cjs, tests/commands.test.cjs, tests/pattern.test.cjs, tests/adr-612-bracket-coherence.test.cjs, tests/adr-612-bracket-read-tolerance.test.cjs, tests/milestone-lock.test.cjs, tests/init.test.cjs, tests/state-todos-render.test.cjs, tests/quick-batch.test.cjs, tests/graphify.test.cjs, and tests/effort-surface-axis.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 12 files from the rule's allowlist. Ground truth via eslint found 25 sites, not the issue's stated 24 (phase.test.cjs has 5, not 4) -- disclosed in the PR body. Reuses PROBE_TIMEOUT_MS, GIT_TIMEOUT_MS, and LOOP_HOOK_POINT_CLI_TIMEOUT_MS across 8 files. Adds two new shared constants to tests/helpers/timeouts.cjs (each independently arrived at by 2 files in this batch, crossing the promotion bar): PATHOLOGICAL_INPUT_TEST_TIMEOUT_MS (node:test's own per-test timeout option, not a subprocess bound) and GSD_TOOLS_CLI_MODERATE_TIMEOUT_MS (a single gsd-tools.cjs CLI subcommand spawn, distinct tier from PROBE_TIMEOUT_MS/LOOP_HOOK_POINT_CLI_TIMEOUT_MS). Adds 3 file-local constants for values used by only 1 file in this batch. No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-12 09:12:31 -04:00
Tom Boucher	aad96e0b5f	test(#4521): migrate capability subsystem batch to named timeout constants (#4627) ... Batch 10 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/adr857-core-without-capabilities.test.cjs, tests/capability-cli.test.cjs, tests/capability-probe-fallback.test.cjs, tests/capability-state.test.cjs, tests/capability-trust.test.cjs, tests/capability-validator-task-content-resolver.test.cjs, and tests/capability-writer.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 7 files from the rule's allowlist. Reuses the existing PROBE_TIMEOUT_MS constant at 8 sites across 3 files. Adds 7 new file-local constants (no promotion to the shared helper needed this batch -- every new class is confined to exactly one file, below the two-file promotion bar): GSD_TOOLS_CLI_TIMEOUT_MS, FRAGMENT_PROBE_SNIPPET_TIMEOUT_MS, INSTALLED_RUNTIME_CLI_TIMEOUT_MS, FIXTURE_MCP_SERVER_TIMEOUT_VALUE, TASK_RESOLVER_FIXTURE_TIMEOUT_MS, TASK_RESOLVER_TIMEOUT_CEILING_MS, and TASK_RESOLVER_TIMEOUT_CEILING_PLUS_ONE_MS (the last two forming a boundary-coverage limit/limit+1 pair). No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-11 09:55:55 -04:00
sim	2eef8ada4f	test(#4520): migrate generators/doc-gates/attribution batch to named timeout constants ... Batch 9 of 17 in the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in 14 files with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes the 14 files from the rule's allowlist. The issue's own guess ("all run a scripts/*.cjs generator or lint script once, BUILD_TIMEOUT_MS class") needed two corrections found by reading every site directly. First, BUILD_TIMEOUT_MS's own doc comment scopes it specifically to scripts/build-hooks.js, which none of this batch's generator/lint-script sites run — a new shared constant, GENERATOR_SCRIPT_TIMEOUT_MS, covers the class instead. Second, no-pending-3212-markers.test.cjs's single site spawns `git ls-files` directly, not a scripts/*.cjs script at all — routed to a second new shared constant, REAL_REPO_GIT_TIMEOUT_MS, promoted once emitted-attribution.test.cjs's own git-plumbing sites were found sharing the same class and value. Isolated Standards-axis review caught a further misclassification: one of REAL_REPO_GIT_TIMEOUT_MS's three emitted-attribution.test.cjs sites actually builds a fresh throwaway temp repo (createTempDir + git init), contradicting that constant's own real-repo-tree-only scope. Fixed with a new file-local FRESH_FIXTURE_GIT_TIMEOUT_MS holding the exact pre-existing value under an honest name, rather than reusing the shared GIT_FIXTURE_TIMEOUT_MS (which would have doubled the bound). emitted-attribution.test.cjs also gets two more file-local constants: HEAVY_REAL_TREE_TEST_TIMEOUT_MS (node:test's own per-test timeout option, not a spawn bound) and BUILD_HOOKS_UNDER_LOAD_TIMEOUT_MS (the same build-hooks.js script as the shared norm, at 4x its bound inside the suite's heaviest test). emitted-ack-trailer.test.cjs gets IMPOSSIBLY_SHORT_GIT_TIMEOUT_MS — the one value in this migration that is deliberately tiny (20ms), used to force a timeout in a negative test, not generous headroom. No src/bin file touched, no numeric value changed anywhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-10 19:22:04 -04:00
sim	150acd78c1	test(#4519): migrate security-scanner batch to named timeout constants ... Batch 8 of 17 in the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/secret-scan-lint.security.test.cjs, tests/security-scan.security.test.cjs, tests/security-prompt-injection.security.test.cjs, tests/prompt-injection-scan.security.test.cjs, tests/read-injection-scanner.security.test.cjs, tests/read-injection-scanner.property.test.cjs, and tests/security.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes the 7 files from the rule's allowlist. The issue guessed this batch "most likely needs its own named SCAN_TIMEOUT_MS." Reading every one of the 19 call sites directly found a more specific picture: 8 sites across 3 files scan exactly one small temp fixture file and match the existing QUICK_SPAWN_TIMEOUT_MS class exactly (reused, no new constant). Two new shared constants cover genuinely distinct classes that happen to coincide in value: SCAN_USAGE_ERROR_TIMEOUT_MS (a bash scan script given missing arguments) and MALFORMED_INPUT_HOOK_TIMEOUT_MS (a Node hook fed malformed JSON) -- kept as separate names per this migration's standing rule that numeric coincidence is never identity. Three file-local constants cover a real multi-file directory scan, a property-fuzzing safety net, and a path-traversal hook test, each with its own pre-existing rationale preserved. No bound is lowered or raised anywhere in this batch, honoring the issue's explicit caution that security-scan timing margins deserve extra scrutiny. No src/bin file touched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-10 17:09:23 -04:00
sim	6d6e3eea73	test(#4518): migrate loop/hook-point e2e batch to named timeout constants ... Batch 7 of 17 in the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/loop-render-hooks.test.cjs, tests/loop-walk.qa.test.cjs, tests/loop-hooks-empty-points-e2e.test.cjs, tests/loop-hooks-ship-pre-e2e.test.cjs, tests/loop-hooks-verify-post-e2e.test.cjs, tests/check-gap-analysis-plan-post-e2e.test.cjs, tests/check-tdd-review-checkpoint-e2e.test.cjs, tests/execute-wave-post-gate-pipeline-e2e.test.cjs, tests/plan-pre-hook-e2e.test.cjs, and tests/qa/tdd-walk.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes the 10 files from the rule's allowlist. Promotes a new shared class norm to tests/helpers/timeouts.cjs, LOOP_HOOK_POINT_CLI_TIMEOUT_MS: 7 files independently arrived at the same value for a single gsd-tools.cjs CLI subcommand invocation with no confirmed subprocess fan-out. Reuses the existing PROBE_TIMEOUT_MS for loop-render-hooks.test.cjs's 11 sites (same class, exact value match). Adds 4 file-local constants for values that share a class with the new norm or an existing one but diverge in pre-existing value, or that numerically coincide with an unrelated existing constant without matching its actual operation. Isolated Standards-axis review caught that the new shared constant's doc comment exhaustively enumerated 3 verb families while a 7th genuine site (an `init new-project` invocation) also correctly belonged to the class; fixed by rewording the comment to state the class definition (call shape) first and list all 4 representative verbs, explicitly illustrative rather than exhaustive. No value changed, no site's classification changed. No src/bin file touched, no numeric value changed anywhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-10 15:50:52 -04:00
Tom Boucher	138e70d734	test(#4517): migrate hook/guard invocation batch to named timeout constants (#4608) ... Batch 6 of 17 in the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/read-guard.test.cjs, tests/feat-2483-review-claude-mds-guard.test.cjs, tests/gsd-write-guard.test.cjs, tests/gsd-secret-read-guard.test.cjs, and tests/hooks-crash-policy.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes the 5 files from the rule's allowlist. Reuses existing tests/helpers/timeouts.cjs class norms where the shape matches (QUICK_SPAWN_TIMEOUT_MS x2, PROBE_TIMEOUT_MS x1). Adds two file-local constants for classes not shared across files: READ_GUARD_HOOK_TIMEOUT_MS (read-guard.test.cjs's 4 sites, a tighter no-fan-out bound than QUICK_SPAWN_TIMEOUT_MS with no bench data to widen it) and FIXTURE_PROBE_CAPABILITY_TIMEOUT_MS (fixture data, not a real spawn timeout). The fifth site (feat-2483-review-claude-mds-guard.test.cjs's review-lane invoke spawn) was initially classified onto HOOK_FANOUT_TIMEOUT_MS; isolated Spec-axis review caught that this call is one nested spawn, not the multi-spawn git-hook fan-out shape that constant's own doc comment defines. Corrected to a new file-local REVIEW_LANE_INVOKE_TIMEOUT_MS holding the exact pre-existing 60000ms value under an honest name, rather than narrowing to PROBE_TIMEOUT_MS with no bench justification. No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-10 15:36:43 -04:00
Tom Boucher	557a0b2876	test(#4516): migrate per-runtime install/upgrade adapters to named timeout constants (#4582) ... Batch 5 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property across tests/augment-upgrades.test.cjs, tests/shared-hooks-dir-resolution.test.cjs, tests/antigravity-upgrades.test.cjs, tests/cursor-hooks.test.cjs, tests/cursor-hook-workspace-roots.test.cjs, tests/gemini-runtime-removed.test.cjs, tests/kilo-upgrades.test.cjs, tests/kimi-upgrades.test.cjs, tests/kimi-variant-disambiguation.test.cjs, tests/opencode-plugin-adapter.test.cjs, tests/windsurf-hooks-bridge.test.cjs, tests/effort-sync-installed-runtime.test.cjs, and tests/hooks-commonjs-marker.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 13 files from the rule's allowlist. Adds one new shared constant to tests/helpers/timeouts.cjs for spawning a single already-staged hook script directly (a heavier class than the existing quick-spawn norm), shared across three files in this batch. One new file-local constant covers a property-test driver process distinct from any existing class. Every other site reuses an existing shared norm. No src/bin file touched, no numeric timeout value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-10 14:11:26 -04:00
Tom Boucher	9770258558	chore(#4590): add no-rendered-text-length-assert ESLint rule (#4595) ... * test(#4590): add no-rendered-text-length-assert ESLint rule Enforces ADR-456's typed-surface mandate for one specific bug shape: a test assertion whose pass/fail depends on the length/substring content of a template literal that interpolates an OS-derived path (os.tmpdir(), os.homedir(), path.join/resolve/..., or a PATH_RETURNING_FNS resolver). Because macOS's default tmpdir prefix is longer than Linux's, such an assertion can pass on one runner and fail on another -- the defect class behind #4421's incident (git show 4e75b836e9), already fixed there by pinning to a typed field per ADR-456 Sec(c) before this rule existed to catch a recurrence. Two repo-wide sweeps against the real tests/ tree narrowed the rule to a sound scope: an initial design that traced call arguments (to approximate the historical incident's cross-file render-function shape) produced false positives on ordinary fs.readFileSync(path.join(...)) + assert.match patterns; a second design that matched any bare direct path-returning call produced 45 false positives on path suffix/prefix/non-emptiness checks. The shipped rule matches only a path-returning expression interpolated into a template literal, directly or via one identifier hop -- disclosed in the rule's own "Known boundaries" as not covering the literal cross-file incident shape, which would require tracing into a callee's body. Phase 1 of epic #4589 (CI test-matrix Linux-primary migration) -- Phase 2's safety argument depends on this class of OS-dependent test assertion being enforced going forward, not merely fixed once. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4590): address code-review findings on no-rendered-text-length-assert Reletter the "Known boundaries" doc-comment list (a)-(e), fixing a gap left by an earlier edit pass and every stale cross-reference to it. Collapse isDirectPathTaint/isTaintedInterpolation's duplicated TemplateLiteral-walk into one recursive relationship (isTaintedInterpolation now delegates a nested-template-literal case back to isDirectPathTaint instead of re-implementing the .some() traversal) -- behavior unchanged, confirmed by re-running the repo-wide sweep (still zero false positives). Found by the Standards-axis /code-review pass on this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-09 21:55:51 -04:00
Tom Boucher	30468f16fe	test(#4515): migrate installer-runs batch to named timeout constants (#4575) ... * test(#4515): migrate installer-runs batch to named timeout constants Batch 4 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property across tests/install.test.cjs, tests/install-minimal-hooks.test.cjs, tests/fragment-single-edit-propagation.install.test.cjs, tests/install-regressions.test.cjs, tests/install-runtime-artifacts.test.cjs, tests/npm-integrity-gate.test.cjs, tests/faulty-deps.test.cjs, tests/release-tarball-smoke.install.test.cjs, tests/install-write-confinement.test.cjs, tests/plugin-manifest.test.cjs, and tests/packaging-shipped-scripts-require-only-shipped.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 11 files from the rule's allowlist. Adds one new shared constant to tests/helpers/timeouts.cjs for a fixture-JSON value (in seconds, not ms) mimicking a Claude Code settings.json hook-entry's own timeout field, shared across two files in this batch. Every other new constant is file-local, each carrying a comment explaining why its call site is a distinct operational class from the existing shared norms even where its digits numerically coincide with one. No src/bin file touched, no numeric timeout value changed anywhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: combine gsd-worktree-path-guard's git rev-parse calls to cut subprocess count under CI load Discovered while verifying PR #4575 (a full test (windows-latest) failure in an unrelated test, tests/kilo-upgrades.test.cjs's worktree-path-guard rejection test). Root cause: this hook's up-to-4 sequential git spawns (git-dir, branch, worktree-toplevel, file-toplevel), invoked inside a native-plugin's own 8000ms-capped subprocess wrapper, left zero margin for node/git startup overhead — the guard is deliberately fail-open on any timeout, so CI-load-induced latency in this chain silently downgrades a security block into an allow. Combines the first three git rev-parse calls (git-dir, branch via --abbrev-ref, worktree-toplevel) into ONE spawn instead of three, using git's documented multi-flag rev-parse output (one line per flag, in order) — verified empirically including the detached-HEAD edge case. No timeout value changed; this reduces subprocess COUNT, not any tolerance. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: add changeset fragment for the worktree-path-guard fix Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-09 11:45:44 -04:00
sim	ab0405ad22	test(#4514): migrate git-adjacent workflow checks to named timeout constants ... Batch 3 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/ci-rebase-check.test.cjs, tests/gsd-validate-commit-crash-policy.test.cjs, tests/pr-branch-planning-filter.test.cjs, tests/reapply-verify-hunks.test.cjs, tests/ship-notes-wedged-pr.test.cjs, tests/slug-derivation-drift-guard.test.cjs, and tests/worktree-safety.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes the 7 files from the rule's allowlist. Mints a new shared class norm, QUICK_SPAWN_TIMEOUT_MS (10000ms), in tests/helpers/timeouts.cjs: 5 sites across 4 of this batch's files had independently arrived at the same value for the same shape (a cheap, trivial subprocess/hook invocation with no real git/network/fan-out work). No src/bin file touched, no numeric value changed anywhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-09 07:05:48 -04:00
sim	95e6a58fd4	test(#4513): migrate git plumbing batch to named timeout constants ... Batch 2 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/git-base-branch.test.cjs, tests/commit-files-pathspec.test.cjs, and tests/git-fixture.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes the 3 files from the rule's allowlist. This batch introduces a violation shape not seen in Batch 1: several sites are pinned-value test assertions verifying the EXACT timeout production code hardcodes (not bounds on this suite's own subprocess calls). Named as four separate constants even where values coincide, so the tests keep catching independent production drift instead of silently tolerating it. No src/bin file touched, no numeric value changed anywhere -- verified site-by-site by two independent isolated review passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-08 19:56:08 -04:00
sim	ff5c5782ee	test(#4512): migrate process-seam batch to named timeout constants ... Batch 1 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/process-seam.test.cjs, tests/helpers-process-isolation.test.cjs, tests/run-with-timeout.test.cjs, and tests/helpers.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes the 4 files from the rule's allowlist. Adds SEAM_DEFAULT_TIMEOUT_MS to tests/helpers/timeouts.cjs (shared across 2 batch files, mirroring process-seam.cjs's own un-exported default). File-local constants elsewhere for values not shared across files or not a bench-derived class norm. No src/bin file touched, no numeric value changed anywhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-08 19:38:41 -04:00
Tom Boucher	f15887ebb1	feat(#4446): ban ad hoc timeout literals in tests, ship with full legacy allowlist (#4449) ... Nothing enforced CONTRIBUTING.md's own stated preference ("A non-literal value is trusted — that is the shape you should be writing") for timeout values in tests. local/no-unbounded-spawn requires SOME bound but allows a bare literal; no-magic-sleep-in-tests and no-elapsed-assertion cover different anti-patterns entirely. This is exactly how PR #4428's Windows CI incident happened: two independently-guessed 15000ms literals (one in production's check-latest- version.cjs, one in this suite's own worker test) collided exactly and raced two SIGKILLs against each other. New rule local/no-adhoc-timeout-literal (eslint-rules/no-adhoc-timeout- literal.cjs) flags a resolvable numeric timeout/timeoutMs literal; an Identifier or MemberExpression value is trusted. No marker-comment escape — the fix is always to extract a named constant, which is trivial. Ships with a full legacy allowlist (126 files, 352 violations, generated by running the rule with an empty allowlist against tests/) so it can go live at error severity without breaking CI, mirroring how no-unbounded-spawn itself was rolled out. Migration is tracked separately in #4445 (this PR does not close it — only #4446, introducing the gate itself). Documents the policy and the compliant shapes in TESTING-STANDARDS.md and TEST-EXAMPLES.md. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-06 21:23:39 -04:00
Tom Boucher	1fe85cd43e	chore(#4244): ESLint rules for the #4220 Windows dirname-walk / TMPDIR-triad bug class (#4246) ... * fix(#4244): repoint TEMP/TMP alongside TMPDIR and fix the sweepProtectSet fixed-point walk Repo-wide sweep (ahead of adding lint rules for these exact bug classes) found both incident patterns still live and unfixed on `next`: - scripts/run-tests.cjs's sweepProtectSet walk stopped on `cur !== runTempRoot && cur.length > 1` — a POSIX-only sentinel. win32 dirname('D:\') is a fixed point (length 3, never satisfies `> 1`... wait, it does satisfy length>1), so a selected file living outside runTempRoot (the common case) spins the walk forever on Windows. Extracted a pure, exported computeSweepProtectSet helper that terminates on dirname(cur) === cur instead, with in-process RuleTester-style coverage for both win32 and posix paths. - tests/run-tests-temp-root.test.cjs's own #4020 regression test set only TMPDIR on its runNode(...) child env. Node's os.tmpdir() never reads TMPDIR on Windows (only TEMP, then TMP), so the redirect silently no-oped there — masked because Windows CI died in the dirname-walk hang above before ever reaching this test. - tests/config-schema.property.test.cjs's fallow config-set test had the same TMPDIR-only pattern, direct process.env assignment this time, restored in its own finally block. Origin: #4220 and its shared root cause #4020. * feat(#4244): require-full-tmpdir-triad and no-unbounded-dirname-walk ESLint rules Two custom local ESLint rules catch the #4220 / #4020 Windows CI hang bug class at author time, joining the ADR-1703 DEFECT.WINDOWS-TEST-PORTABILITY catalog. Neither eslint-plugin-unicorn nor eslint-plugin-n has a rule for either shape. - local/require-full-tmpdir-triad: flags a TMPDIR environment override (direct process.env.TMPDIR assignment, or a TMPDIR property in a spawn-like call's env: object literal) not accompanied by TEMP and TMP in the same scope. Node's os.tmpdir() never reads TMPDIR on Windows. Registered on tests/**/*.cjs, matching the require-userprofile-with-home precedent. - local/no-unbounded-dirname-walk: flags a while/do-while loop reassigning from dirname() with no fixed-point termination guard (dirname(cur) !== cur, or path.parse(cur).root). path.dirname() is a no-op at the platform root, but the value differs by platform (win32 'D:\' is length 3, posix '/' is length 1), so a POSIX-shaped length/equality bound never fires on Windows. Registered on BOTH tests/**/*.cjs and scripts/**/*.cjs — the real #4020 bug lived in scripts/run-tests.cjs, not tests/. Both rules join the zero-escape-hatch discipline already established for this catalog (no bespoke comment marker; PROTECTED_RULES in tests/portability-rule-disable-ban.test.cjs independently bans eslint-disable of either). ADR-1703 and its two companion contributing docs get an amendment documenting the mechanism, code examples, and the repo-wide sweep (three live instances found and fixed in the prior commit; no others found). CI test-scope selection updated so an edit to either rule or to scripts/run-tests.cjs re-runs the right suites. * fix(#4244): no-unbounded-dirname-walk must analyze a single-condition loop test too checkWhile bailed out early unless node.test was a LogicalExpression, so a single-condition loop -- while (cur !== root) { cur = dirname(cur); } -- was silently skipped and never reported. That is the EXACT minimal shape of the original #4020/#4220 bug, and it is literally the shape used by this rule's own shipped RuleTester fixtures (the "equality-only bound" invalid cases), which were failing (0 errors reported, 1 expected) until this fix -- confirmed by running RuleTester directly against both fixtures, not just via a passing test-runner exit code. The conjunct-collection helper already handled a non-LogicalExpression test correctly (it pushes a single node as the sole conjunct); only the early-return gate needed to stop requiring a compound && / || test. Verified: RuleTester run directly against both previously-broken fixtures plus two new sanity cases (a guarded single-condition loop stays valid; an unrelated single-condition loop stays silent), and a fresh `npx eslint .` across the whole repo remains clean (no other single-condition dirname-walk shape exists in the tree). * fix(#4244): require-full-tmpdir-triad must recognize a destructured child_process call isSpawnLikeCallee only recognized a MemberExpression callee (child_process.spawnSync(...)) or a bare identifier in ENV_LOCAL_HELPER_NAMES (runNode). A destructured import called bare -- const { spawnSync } = require('child_process'); spawnSync(...) -- has an Identifier callee named "spawnSync", which matched neither branch, so the whole env-literal check was skipped. gsd-test caught this: both "invalid: child_process.spawnSync with TMPDIR-only env" cases in tests/require-full-tmpdir-triad.rule.test.cjs were failing (0 errors reported, 1 expected). Widened the bare-identifier branch to also match any of the known ENV_CHILD_PROCESS_METHODS names, matched by name only -- the same lightweight convention this repo's other eslint-rules/*.cjs use (e.g. no-hardcoded-tmp.cjs's isFsMethodCall), not full import data-flow tracing. Verified: RuleTester run directly against all 11 cases in tests/require-full-tmpdir-triad.rule.test.cjs (not just the two that were failing), all pass; a fresh npx eslint . and npm run lint:ci across the whole repo remain clean. * fix(#4244): correct a stale escape-hatch reference in a test comment The comment on the "length comparison against another expression's length" case referenced a "// allow-dirname-walk marker" that doesn't exist -- the rule has zero comment-based escape hatches by design (ADR-1703), and an earlier draft's marker mechanism was removed before this branch's first commit. Spec-axis review caught the stale reference. No behavior change; comment-only. * chore(#4244): backfill changeset PR number (pr:0 -> pr:4246) --------- Co-authored-by: sim <sim@local>	2026-09-03 14:14:09 -04:00
Tom Boucher	2131fe13f3	enhance(#3464): exec() detection widening, citation-debt cleanup — Phase 8 (#4171) ... * feat(#3464): widen no-source-grep to detect regex.exec() on tracked text Adds an execCall kind alongside the existing regexTest detection -- regex.exec(tracked) was invisible to the rule while regex.test(tracked) was already caught, despite both reading a source-derived string through a regex. Measured: 4 previously-invisible sites across 2 files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#3464): migrate 4 sites newly flagged by the exec() widening docs-hooks-table-parity.test.cjs's three regex-extraction loops are site-scoped marked (source-text-is-the-product) -- the dynamic preToolEvent/postToolEvent dialect branching they mirror is explicitly documented as not statically parseable, so a literal-pattern mirror is the practical minimum-cost check. no-bare-gsd-tools-command-position.test.cjs's readRouterVerbs() now requires HOST_COMMAND_ROUTERS directly instead of regex-walking gsd-tools.cjs's source text -- the same accessor three other suites already use. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3464): pay down 6 grandfathered uncited allow-test-rule markers Two were genuinely load-bearing (suppressing a real detected violation) and just needed a citation added -- phase6-capstone-conformance.test.cjs, runtime-name-policy.test.cjs, both now (#3464). Four were dead-weight file-header markers suppressing nothing -- each file's real effective sites are covered by separate, already-cited markers elsewhere in the same file. Deleted outright rather than cited, per Phase 1's own precedent (remove non-load-bearing markers instead of grandfathering them forever) -- codex-config.test.cjs (two copies), gsd-check-update-worker-platform-gate.test.cjs, orphaned-hooks.test.cjs, settings-jsonc.test.cjs. allowlist.json: 134 -> 128 entries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#3464): re-baseline effective-exemption ceiling to 84 The exec() widening's 3 newly-marked sites are now suppressed and counted; ceiling rises 81 -> 84, the exact measured high-water mark. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3464): correct citation and restore a wrongly-deleted marker Two review corrections, both found by the orthogonal review pass: - docs-hooks-table-parity.test.cjs's 3 new exec() markers cited #3464 (mechanically "the phase that widened the rule") when the file's own established, correct reference is #3839 (the issue this whole test exists to enforce, already cited in its file header) -- fixed to match. - gsd-check-update-worker-platform-gate.test.cjs's deleted file-header marker was NOT dead weight: its codeOnly() helper wraps readFileSync and is called inline as an assert argument, a genuine source-grep pattern on real .cjs/.js source that the rule cannot currently see (helper-function indirection is a distinct blind spot from anything Phase 7/8 measured) -- CONTRIBUTING.md is explicit that "unverified" is not the same as "vestigial." Restored, site-scoped this time (directly above codeOnly(), not as an inert file-header comment) and cited (#3103, the issue the file's own docstring already references). codex-config.test.cjs's two deletions and orphaned-hooks.test.cjs's / settings-jsonc.test.cjs's deletions were independently re-verified and stand: their flagged lines read generated .toml/.json OUTPUT, not source, or have no residual pattern at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-02 08:11:23 -04:00
Tom Boucher	f16ff7d1b3	enhance(#3545): widen no-source-grep with fold+hooks, migrate 76 sites (#4161) ... * feat(#3545): widen no-source-grep with one-hop path-fold and hooks dir Resolve a readFileSync() path argument that is a bare Identifier one hop back to its VariableDeclarator initializer before classification, and recognize `hooks` as a source directory alongside bin/lib/gsd-core/src. Measured (epic #3464 phase 7): fold+hooks together newly flag 76 unsuppressed sites across 18 files that were previously invisible to identifier-indirected or hooks/-rooted source reads. Neither widening alone is sufficient — hooks-only surfaces 0 new sites, confirming #3520's prior finding that the identifier-indirection gap must close first. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#3545): migrate 76 sites newly flagged by the fold+hooks widening Per-site classification: rewrite behaviorally (require() the real module, assert on its actual exported behavior) wherever the read was a proxy for code behavior; add a site-scoped `// allow-test-rule: <reason> (#3545)` marker only where the raw source text genuinely is the product under test (codex-config.test.cjs's adapter-header-contract checks, install.js structural-wiring guards with no exported symbol, AST-parse fixture inputs, etc.) — each marker cites an existing repo-sanctioned category from CONTRIBUTING.md's allow-test-rule exception table. Also converts two try/finally test bodies (introduced during this same migration) to the required t.after() cleanup pattern per CONTRIBUTING.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#3545): re-baseline effective-exemption ceiling to 81 The fold+hooks widening's own newly-detected sites are now suppressed by site-scoped markers, moving them from invisible into the tightly-ratcheted effective-exemption count. Ceiling rises from 10 to 81 (the exact measured high-water mark, grace unchanged at 2) — a deliberate, measured re-baseline per the widening working as intended, not an ordinary ceiling bump. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3545): use canonical allow-test-rule category tokens 4 markers added during migration cited an issue ref correctly but didn't use one of CONTRIBUTING.md's seven recognized category tokens, unlike every other marker in this change. Cosmetic only — same suppression lines, same effective/live counts (81/81, 0 live). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3545): correct stale phase-artifact path in test comment Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-01 21:40:38 -04:00
Tom Boucher	80de48c319	enhance(#3914): every phase records a truthful guard ledger (#4018) ... * fix(#3914): retire n/no-process-exit where its successor governs Epic #3889 criterion 5 — no phase closes with a guard added and its predecessor left standing — is violated in the tree by the epic that wrote it. local/require-registered-exit was registered on gsd-core/bin/**/*.cjs and scripts/**/*.cjs, while n/no-process-exit stayed 'error' over a nine-glob block covering those same two. Only the hooks 'off' exemption ever came down; the predecessor's registration never did. Both rules have been enforcing the same property on the same surfaces since P6. Narrowed, not deleted. Seven of those nine globs have NO successor — eslint-rules/, bin/lib/, pi/, examples/, vscode/, .kilo/, .opencode/ — so deleting the rule outright would silently drop enforcement on all seven. That is the inversion this epic has already hit three times: removing a coarse guard because a narrower one exists somewhere it does not reach. Flat config is last-match-wins and both successor blocks come after the nine-glob block, so 'n/no-process-exit': 'off' in exactly those two retires the predecessor precisely where the successor governs and nowhere else. The successor is strictly more precise: it permits process.exit only inside terminateNow in cli-exit.cts, the single sanctioned terminator (ADR-3889 §3), where n/no-process-exit permits none and would flag terminateNow's own generated copy. Asserted at the consumer's altitude via ESLint.calculateConfigForFile on real paths, with the positive control that matters: n/no-process-exit is still 'error' on six of the seven successor-less globs, so a future edit that turns this into a blanket disable goes red. bin/lib/ has no file in this checkout and is reported as untested rather than given an invented path. Severity is normalized across the string/numeric/array forms the API can return, and the normalized value asserted — not truthiness. Verified by running calculateConfigForFile myself on both superseded globs and four controls before trusting the test. Found and fixed inline: the change made an eslint-disable directive at gsd-tools.cjs:257 partially unused, which --max-warnings 0 rejects; narrowed to the one rule still in force. Verification runs on the remote runner. Refs #3914 * docs(#3914): the epic added three guards, it did not remove one The audit reconciled the epic ledger against what actually landed. The net is +3, not -1: four lint:generated-sync --check arms (gen-scripts-cli-exit, gen-hooks-cli-exit, gen-exit-code-registry, gen-exit-code-docs) plus one rule, against two retirements. An epic whose thesis was consolidation ended with a larger guard surface than it started with. The additions are each defensible; the claim that the total fell was never true. Two of the three prior errors in this amendment are mine. It said "Net -1 by count" above terms reading -1 -1 +1 +1 +1, which sums to +1 — an arithmetic error in the paragraph directly below the sentence arguing that an ADR about honest accounting must not pad its own ledger. And the term list omitted two of the four --check arms, which is what turns that +1 into the real +3. Recorded rather than quietly rewritten. This ledger has now been wrong three times — the original -2, the -1 that replaced it, and #3914's own table, which states -1 above terms summing to 0 — and a written claim nobody checked against the thing it describes is the exact failure this epic exists to close. Refs #3914 * fix(#3914): make the successor actually supersede before retiring the predecessor An isolated security review found that the previous commit turned off a guard that was still doing work. Reproduced by executing both rules against a fixture, not inferred: const exit = 'exit'; process[exit](1); n/no-process-exit flags it; local/require-registered-exit did not, because it early-returned on callee.computed. So retiring the predecessor on gsd-core/bin/**/*.cjs and scripts/**/*.cjs un-guarded that shape on precisely the two globs this epic's exit contract cares most about. This is the third time in this epic I have removed a coarse guard on the claim that a narrower one covered it, without checking construct-level parity — after the allowlist key-to-prefix-to-exact-membership sequence and the band ranges-to-categories one. The rule is the same every time: a narrower guard supersedes a coarser one only where it demonstrably reaches at least as far, and "demonstrably" means executing both against the constructs, not reading either. The successor now resolves computed property access for the statically determinable cases — a string Literal, and an Identifier bound once to a string Literal, resolved through scope — and leaves genuinely dynamic properties alone so the rule does not over-fire. Measured after the fix: plain process.exit flagged, process['exit']() flagged, process[exit]() flagged, process[globalThis.k]() not flagged. That makes it a strict superset of the predecessor on these globs, since process['exit']() was caught by NEITHER rule before. The second finding is worse than the first, because it was reasoning rather than oversight. My justification comment claimed n/no-process-exit "would flag terminateNow's own generated copy here". It would not — that file is in the global ignore list, so neither rule ever lints it. There was no conflict to resolve; I wrote a rationale I had not checked, in a change whose entire subject is written claims nobody verified. Both comment blocks now state the real basis. The tests that should have caught this asserted only rule SEVERITY per glob and never construct REACH, which is exactly how a coverage hole passed. A parity matrix now pins all five shapes, including a RED/GREEN regression pin against an inlined reproduction of the pre-fix rule — inlined rather than loaded from HEAD, because HEAD resolves to the fixed commit under the remote runner and would silently stop testing anything. Verification runs on the remote runner. Refs #3914 * fix(#3914): the two exit rules are complementary — keep both Reverts this branch's retirement of n/no-process-exit. The premise was wrong twice, and the second review proved the change itself was wrong. I claimed local/require-registered-exit was a strict superset on gsd-core/bin/**/*.cjs and scripts/**/*.cjs. Measured, successor vs predecessor: function f(exit) { process[exit](1); } 0 vs 1 let exit='exit'; exit='exit'; process[exit]() 0 vs 1 const { exit } = ...; process[exit](1) 0 vs 1 plus for-of bindings, let-then-assign, var redeclaration, catch params, and an undeclared global named exit. The predecessor matches any identifier NAMED exit however it is bound; the successor resolves only a string literal or a single-write const. It never was a superset — I asserted the relationship after fixing one construct and did not re-check the rest. The justification was independently false: all three generated cli-exit copies are in the global ignore list, so n/no-process-exit was never flagging terminateNow. There was no conflict to resolve. I wrote a rationale I had not verified, in the phase whose subject is written claims nobody checked. So criterion 5 does not apply to this pair. They are not predecessor and successor — they are complementary, each catching constructs the other misses. The epic's criterion assumed a replacement relationship that does not exist here, and retiring either rule loses real coverage. The ADR ledger now says so with the measured shapes. What survives is the genuine improvement: the computed-property strengthening. local/require-registered-exit now catches process['exit'](1) and optional-chain terminators like process?.[k]?.(1), which NEITHER rule caught before, while correctly ignoring a genuinely dynamic property so it does not over-fire. The parity tests are rewritten to assert what is true rather than what I wanted to be true: a bidirectional matrix where each rule is shown catching shapes the other misses. The previous matrix tested only the four shapes where the successor wins, which is precisely why the regression shipped — a test set selected to confirm the thesis. Also corrected: a stale ADR sentence claiming a third wrong ledger version that does not exist (the table it described now reads +3 over terms summing to +3), and a changeset whose stated motivation was the false generated-copy conflict. Verification runs on the remote runner. Refs #3914 * fix(#3914): the exemption term was a no-op — the net is +4 Fourth correction to this ledger, and a fourth error of the same kind. Every version counted removing the n/no-process-exit 'off' entry from the hooks block as -1. Measured: calculateConfigForFile returns undefined for that rule on hooks/**. It was never registered there, and no broader block sets it globally, so the 'off' entry overrode nothing and removing it changed no enforcement at all. A no-op removal, not a guard removal — the same category error as counting baseline acknowledgement entries: a thing that is not a guard, in guard units. It is misattributed too; that block came down in d98b55562 (#3910), already on next before this branch existed. So the epic added FOUR guards, not three. This surfaced from a test of mine that overclaimed. I asserted n/no-process-exit was error on "all nine CommonJS/hook globs" — but hooks is not one of the nine, and the rule resolves to undefined there. Fixing the test to match reality is what exposed the ledger term, which is the argument for tests that assert identity rather than a comfortable shape. The hooks state is now pinned explicitly rather than glossed: n/no-process-exit unregistered, local/require-registered-exit error. It is mildly surprising and therefore worth a test. Also updates a pre-existing test that documented the old name-based-only boundary as intentional. The computed-property strengthening deliberately moves that boundary — process['exit'](0) was caught by NEITHER rule before — so the test now asserts the new contract and cites the ADR, rather than being left to fail or the rule weakened to satisfy it. A contract change should read as deliberate in the test that pins it. Verification runs on the remote runner. Refs #3914 * chore(#3914): backfill changeset pr number to 4018 --------- Co-authored-by: sim <sim@local>	2026-08-29 00:53:33 -04:00
Tom Boucher	ab69b9ce56	enhance(#3987): guard slug re-derivation and the swallowed-precondition shape — §8.5 was guardable after all (#3999) ... * feat(#3987): guard slug re-derivation, and record why the swallow shape cannot be guarded Epic #3473's Decision 1 requires the wrong call site be UNREPRESENTABLE. #3984 measured that two of the nine §8 rules had no guard at all and recorded both as "Shipped - test-covered". This closes one of them, proves the other cannot be closed the same way, and corrects two false claims I merged yesterday. 1. §8.3 - scripts/lint-slug-derivation-drift.cjs. generateSlugInternal (src/core-utils.cts) is the canonical owner; #3883 removed 11 inline copies. Nothing prevented a twelfth: no slug guard existed in scripts/ or eslint-rules/. The detector is STATEMENT-scoped and matches the shape the real copies took - one statement carrying BOTH .replace(<negated class>, '-') and .replace(/^-+|-+$/, ''). Statement scoping is what buys the precision: the loose LINE-level form yields 18 hits with 7 unrelated, a material false-positive rate. Measured on the tree: 5 flags, 2 TRUE, 3 SANCTIONED, 0 FALSE. The three sanctioned sites are allowlisted with a reason each, following lint-phase-enumeration-drift's form rather than a bare denylist. The owner itself is listed explicitly even though it escapes by construction - an implicit escape is a latent bug, and the next person to touch line 192 would not know the guard depended on it. 2. Both TRUE positives were live defects, not style. scripts/qa-smell-ratchet.cjs reproduced the canonical formula including the 60-cap but trimmed BEFORE truncating - the #2849 bug - and never transliterated. The divergence is total, not cosmetic: canonical "privet-mir-privet-mir-privet-mir-privet-mir-privet-mir-prive" inline "tail" Cyrillic collapsed to nothing and only the ASCII remainder survived, so the ratchet was keying on wrong identifiers for any non-ASCII input. tests/planning-inspect.test.cjs carried a helper whose comment claimed parity with getPhaseDirFromPhaseId. That function now transliterates; the helper did not, so the test asserted against a stale formula while looking correct. Both now route through the seam. 3. §8.5 - measured, and deliberately NOT shipped. A candidate detector (swallowing catch + errno-retry-set test in the same function) gives 26 flags across 11 functions: 0 TRUE, 26 FALSE. Every one is best-effort unlink/rm/close cleanup, lost-rename-race backoff, or a deliberate null fallback. The file-scoped variant is worse at 71. Worse than the noise: the only known true instance was removed by #3885, so there is NO POSITIVE CONTROL - the guard cannot be shown capable of failing, which this repo requires of every drift guard. Shipping it would add a guard nobody can trust and nobody can test. The ADR now records the measurement and the reason, keeps §8.5 at "Shipped - test-covered", and points at the #1884 regression test as what actually enforces it. An honest "not detectable at acceptable precision" beats a guard that only ever passes. 4. Two claims I merged into the ADR yesterday were wrong. §8.9 said 17 of 19 subsumed children have a test citing their issue number, and that #3364 and #3812 have none. Both halves are false, and the claim came from a NUMBER-GREP - inside an amendment whose own subject is that a text match is not a fact. #3364 IS cited: tests/runtime-marker-resolution.test.cjs:107, T3 installMarkerResolvesWhenEnvAndConfigAbsent_3897 (#3364), asserting at :115-119. #3812 IS covered: tests/gen-state-md-docs.test.cjs:374, asserting at :382. Corrected to 19 of 19. #3812 does carry a real finding, though a different one: it is PARTIALLY DELIVERED on a CLOSED issue. The shipped fix declares cardinality for frontmatter keys, but #3812's stated acceptance was about the ## Current Position BODY section, and docs/reference/state-md.md:196-208 still has no normative single-valued/overwrite sentence and no pointer to ## Performance Metrics for history. Recorded in the ADR and left for #3812 to re-open - fixing it here would bury a scope question inside an unrelated PR. Note on B6: this ADDS a guard, and B6 said the net count must fall. #3951 already amended that clause - a guard ledger is a claim about COVERAGE, not count - which is what makes adding this one honest rather than contradictory. Verified: the guard flags 0 on the fixed tree, and PROVES IT CAN FAIL - a fresh inline copy planted in src/ makes it exit 1 naming the exact statement. All three sanctioned sites were confirmed exempt BY the allowlist, not by accident of the pattern, by re-attributing each to a non-exempt path and watching it flag. build:lib, lint and lint:ci all exit 0. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3987): add the changeset fragment Doc-only, so it carries forward from the verified sha rather than costing a second matrix run. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): §8.5 IS guardable — I was wrong, and the guard found a live defect Two orthogonal reviews. The correctness review overturned my central judgment, and it was right. 1. I concluded §8.5 was "not detectable at acceptable precision" and recorded that in the ADR. False. My evidence was 26 flags / 0 TRUE / 26 FALSE. The reviewer pointed out what I had not: all 26 false positives are CLEANUP verbs - rmSync 54, unlinkSync 43, closeSync 17, chmodSync 12 - and the obvious narrower predicate was never tried. A swallowed cleanup is legitimate best-effort. A swallowed CREATION is a precondition silently lost, which is exactly the #1884 shape. Measured properly, in three stages: swallowing catch 911 + try-block calls a CREATION verb 24 + enclosing function references a *_ERRNOS set 0 0 flags, 0 false positives. The `*_ERRNOS` naming key is empirically total - all 10 retry/tolerate sets in src/ follow it. My second claim was worse. I wrote that no positive control exists because #3885 removed the only true instance, so the guard "cannot be shown capable of failing". That is self-refuting: this very PR's slug guard proves-it-can-fail on a synthetic tree, and the pre-#3885 blob is available as exactly such a fixture. It is now the control, and it works in both directions - the rule flags 0c43d853e^:src/planning-workspace.cts at line 210, the line the fix commit's own message cites, and reports zero on the post-fix code. I stopped at the first negative result on the option that meant less work. Shipped as eslint-rules/no-swallowed-precondition.cjs, wired into the existing src/**/*.cts ESLint block rather than a scripts/lint-*-drift.cjs: no script in scripts/ requires typescript/espree/acorn, and scripts/ ships to consumers, so a .cts-parsing standalone guard would add a devDep at consumer runtime. The ESLint block already parses .cts for free. 2. The guard immediately found a live defect of the same class. src/capability-lock.cts swallowed a mkdirSync on the lock directory, then acquireLock classified the follow-on failure as `code !== 'EEXIST' → return null`. A real EACCES/EROFS makes openSync(lockPath,'wx') fail ENOENT, which is not EEXIST - so a fatal filesystem error was laundered into "lock unavailable". Same defect as #1884, different laundering target. Fixed the way #3885 fixed #1884: the creation failure propagates. Regression test proven fail-first by hand - with the fix stashed, EACCES was laundered to null; restored, it throws. The strict rule does NOT catch this shape (its errno classification is an inline literal, not a named set). The rule is deliberately left strict: the broadened form had 2 false positives - capability-lock.cts:408, the deliberate EEXIST steal protocol, and commonjs-marker.cts:131, which returns a distinct documented outcome. The gap is noted in code rather than papered over with a noisy predicate. 3. The security review found the slug guard's exemption FAILED OPEN. currentFunction was never reset, and only a column-0 `function` declaration updated it, so exemption bled from an allowlisted declaration to the next one. generateSlugInternal exempted 50 lines for an 11-line function. A re-derivation planted anywhere in that window was silently exempt - the same fail-open shape that produced a blocker in #3897, and an allowlist is a SUBTRACTION so a mismatch fails open by construction. Extent is now tracked by real brace depth, and a test plants a violation after each allowlisted function's real closing brace and asserts it IS flagged. 4. Also from the security review: the guard was a CI-DoS and narrower than I claimed. Its unbounded [^\]]* was re-scanned from every `.replace(/[^` start: 54.3s on a 1.28MB line. It imported MAX_REGEX_LITERAL_LEN and never called readRegexLiteralAt - the bounded tokenizer that exists for exactly this. Now routed through it with a 2MB file cap: ~200ms. 15 of 25 genuine re-derivations evaded. Widened to catch replaceAll, {1,}, \s*-wrapped classes, escaped ], literal new RegExp(...), five trim spellings, .split().join(), and multi-line .replace( args - still 0 false positives. Two forms still evade and are documented as deliberate gaps with negative tests: the two-statement/temp-var form and new RegExp built from a variable. Both need data flow, and guessing at it is how a guard becomes noisy. Also fixed: // inside a string truncated the line, a ; inside the collapse regex split the statement (a one-character bypass), and SCAN_EXT omitted .mjs/.tsx/.jsx. 5. A regression I introduced, caught by the same review. qa-smell-ratchet.cjs top-level-required a build output that is not git-tracked, so the script hard-failed MODULE_NOT_FOUND before build:lib - including for --help, which previously had no build dependency. The require is now lazy at the point of use. 6. Four of my own tests were vacuous or weak. T9's input yielded an identical string under the buggy formula, so it passed on the implementation it was meant to catch. T12 compared maxLen null vs 60 on an 18-char name, where they agree trivially. T9-T12 all asserted generateSlugInternal directly, so they would pass unchanged if both call-site fixes were reverted. And prove-it-can-fail was scoped to scanRepo, never the CLI - dropping main()'s exit-code line would have kept every row green. All rewritten with discriminating inputs, per-call-site rows that red when the fix is reverted, 59/60/61 boundaries, an entirely-non-alphanumeric row, and a CLI row asserting the real subprocess exit code and both sanitizeForReport sites. Verified: both guards flag 0 on the tree and both prove they can fail. The swallow rule's control is confirmed in both directions - pre-#1884 shape flagged, post-#3885 shape clean. build:lib, lint and lint:ci all exit 0. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3987): record that §8.5 IS guardable, and correct a correction that made a ledger worse Three ADR corrections, two of them to text this branch wrote hours ago. §8.5 advances to Enforced. Its previous entry said the rule was not detectable at acceptable precision. That was wrong twice: the 26 false positives were uniformly CLEANUP verbs, which is a reason to narrow the predicate rather than abandon it, and the claim that no positive control exists was self-refuting - the pre-#3885 blob is available as a fixture and this repo's own guards prove-it-can- fail on synthetic trees. Narrowed to creation verbs plus a *_ERRNOS reference: 911 -> 24 -> 0 flags, 0 false positives, control confirmed in both directions. The entry keeps the wrong reasoning visible, because a high false-positive count being evidence the predicate is wrong - not evidence the rule is unguardable - is the transferable part, and the first negative result is most seductive when it is also the answer that means less work. §8.9's correction is itself corrected. The original 17-of-19 claim was CORRECT for the predicate it stated; this branch silently swapped cited -> covered and declared 19 of 19. #3812 appears in zero test files. Changing what a word means to make a ledger read better is a worse failure than the miscount it claimed to repair. Both predicates are now reported separately - 18 of 19 cited, 19 of 19 covered - because §8.9 asks for a test NAMING each child, so 18 is the number that answers it. #3812 is also re-opened for real, rather than the first draft's promise that it could be. §8.3 stays Shipped - test-covered rather than advancing. The slug guard catches the copy-paste class and a dozen variants, but two forms still evade by decision (temp-var split, new RegExp from a variable) because both need data flow. Naming them keeps the status honest: the wrong call site is much harder to write, not unrepresentable. Closes #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3987): backfill changeset pr number Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): replace my own wall-clock assertion, and close the guard that let me write it CI went red on ubuntu shard 2/3. The failing test was mine, and the failure was the test, not the code. a 1.28MB line ... scans in well under a second (was 54.3s pre-fix) 7368ms It asserted ELAPSED TIME. ~200ms locally, 7.4s on a shared CI runner. The bound introduced for the MAJOR-2 DoS fix works - 7.4s against a 54.3s pre-fix baseline is the fix doing its job - but an absolute wall-clock threshold on shared hardware is a race, not an assertion. CLAUDE.md says so directly: "Clock Seams: Do not assert on wall-clock time." I wrote the anti-pattern the project bans, in a PR about guards. Raising the threshold would only move the flake. The row now asserts a DETERMINISTIC bound instead: an instrumentation seam on drift-scan.cjs counts readRegexLiteralAt calls and characters examined, and the test asserts charsExamined stays under an absolute ceiling. Measured on the same 1.28MB fixture: 120,000 calls, 48,000,000 chars - two orders under the ceiling. The pathological fixture is kept; only the thing being asserted changed. Proven to still discriminate: with MAX_REGEX_LITERAL_LEN raised to simulate the unbounded pre-fix behavior, the same fixture does not complete in 120 seconds, versus ~0.3s bounded. It is a real regression test, not a tautology. Then the second half, which is the same defect class as the rest of this PR. eslint-rules/no-elapsed-assertion.cjs matched only the EXACT identifiers ^(elapsed|duration|took|ms)$. I used `elapsedMs`. It evaded the rule entirely. tookMs, durationMs, elapsedTime and msElapsed evade the same way. A guard that cannot see the violation it exists to catch is exactly what this PR is about - it just happened to be an existing rule rather than one of the two I came here for, and it was found because I committed the violation it should have blocked. Widened to /^(?:elapsed|duration|took|ms)(?:[A-Z]\w*)?$/ plus a narrow start/endMs delta pair. Deliberately NOT a blanket *Ms suffix: a first draft did that and produced 2 false positives on `timeoutMs` in plan-phase-stall-detection, which is a configured timeout and not a measurement. Verified negative on params, items, forms, terms, dirnames, timeoutMs, cacheTtlMs and staleAfterMs. Measured over the five files carrying camelCase timing identifiers: 0 true positives beyond my own, so nothing else needed rewriting. The rule's own test file gains a row asserting `elapsedMs` flags, proven to fail against the pre-widening rule - the same prove-it-can-fail standard both new guards in this PR are held to. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): a comment I added leaked a Claude reference into every runtime install The runner went red with 4 failures in tests/install.test.cjs: Leaking: .hermes/scripts/lib/drift-scan.cjs Leaking: .qwen/scripts/lib/drift-scan.cjs The instrumentation seam added for the deterministic bound carried a comment naming CLAUDE.md as the source of the no-wall-clock-assertions rule. scripts/ SHIPS to consumers, so that comment was installed verbatim into hermes and qwen trees, and the install suite scans for exactly this - a Claude-specific reference reaching a non-Claude runtime. The rule is real and worth citing; the filename is not portable. The comment now says "this repo's test rules" and states the rule inline, which is what a reader of an installed tree actually needs anyway. Worth noting what caught it: not lint, and not the two guards this PR adds - the install suite's full-tree scan, which exists precisely because a shipped file is read by runtimes that have never heard of CLAUDE.md. Same lesson as the rest of this PR from the other direction: the check that matters is the one that can see the surface where the defect actually lands. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): a test fixture swallowed 46 git exit codes and produced a silent false negative CI red on ubuntu shard 3/3: tests/health-validation.test.cjs:2029 expected exactly one W024, got [{"code":"W006", ...}] Not caused by this branch, and the evidence is decisive rather than a hunch: the SIBLING test at :2039 builds the IDENTICAL fixture with the identical commitsAhead and asserts the same thing, and it PASSED in the same process, same file, same run. Same input, both outcomes - which rules out logic, ordering, sharding and environment, and leaves a per-invocation nondeterministic failure inside one fixture build. The mechanism is an unchecked exit code, 46 times over. The W024 fixture performs ~46 runGit spawns and never checks a single one. runGit returns failures as DATA and never throws, so one silently-failed `git commit` yields 19 commits instead of 20, or a silently-empty `git rev-parse HEAD` yields a blank state_head. Either drops readStateHeadFreshness below the advisory threshold, W024 never fires, and only W006 remains. Reproduced exactly: 20 commits -> ["W006","W024"]; 19 -> ["W006"]; blank state_head -> ["W006"] - byte-identical to the CI assertion dump. The arithmetic is what hid it. At threshold-1 and threshold+1 a lost commit still produces the asserted answer; only the exactly-at-threshold cases sit one commit from a false negative. Two of the seven tests are in that position, and CI hit one. That is why it had never been seen before, and why it surfaced now: this branch adds three test files, which reshuffles the cost-weighted shard partition and moved this file into a chunk where the latent flake fired. My files were checked as suspects first and cleared: all fixtures mkdtemp-unique, no process.chdir, no .planning/ writes, no git spawns, and node --test gives per-file process isolation regardless. Fixed at the cause, not the symptom. A mustGit wrapper throws on a non-zero exit with the command, exit code and stderr, and all nine call sites route through it. The fixture now asserts its OWN preconditions before the assertion under test runs - the seed head is non-empty, and `git rev-list --count <seed>..HEAD` equals the requested commitsAhead - so a fixture that did not build what it claims fails loudly as a FIXTURE ERROR naming got-versus-asked, instead of quietly handing a weaker input to the assertion. Proven: dropping one commit now raises FIXTURE ERROR: requested commitsAhead=19 but git rev-list --count reports 18 where it previously produced a silent ["W006"] pass-for-the-wrong-reason. 64/64 tests in that block pass unperturbed. Deliberately NOT done: no threshold change, no retry, no loosened assertion, no skip. The assertion was correct; the input was silently wrong. Worth naming, because it is the same shape from the other side: this PR ships eslint-rules/no-swallowed-precondition.cjs, whose entire subject is a swallowed precondition failure being laundered into a plausible downstream outcome. This fixture is that defect in test code - the swallowed git failure was laundered into a legitimate-looking "W024 did not fire". The rule does not cover test fixtures, so the connection is noted at the fix site rather than enforced. Refs #3987 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3987): two tests wrote to committed files; the shard packing decided when that mattered CI red on windows-latest shard 1/3 only: "gen-exit-code-registry: CLI" > "a --write run redirected to a tmpdir leaves every committed artifact untouched" AssertionError: hooks artifact must be untouched The Linux runner passed the same sha at 40425/40425. It is Linux-only, so a Windows-scheduling defect is structurally invisible to it. Root cause, established by measurement rather than inference. tests/cli-exit.test.cjs appended a corruption marker to the REAL COMMITTED hooks/lib/exit-code-registry.js, held it corrupted across a full subprocess, and restored it in a finally. tests/exit-code-registry.test.cjs reads that same real file before and after its own subprocess and asserts byte equality. If it samples while the other test holds the file corrupted, it fails. The landmine is pre-existing, from 2ea5efc15 (#3911). What this branch changed is WHEN the two run together. scripts/run-tests.cjs shards by cost-weighted LPT over the sorted unit list, so adding three test files repacks the bins: merge-base c3e667df3 (838 files): cli-exit -> shard 1, exit-code-registry -> shard 3 HEAD 03b342601 (841 files): BOTH in shard 1, same argv chunk, one node --test process, concurrent Co-location is necessary but not sufficient - Linux shard 1/3 also had both and passed. Windows loses because TEST_CONCURRENCY defaults to 2 there against 4 elsewhere, spawn cost is ~10x, and the sibling corruptor holds one of only two slots through a ~90s tsc compile. That turns a sub-second overlap into seconds. Not a path-separator or case-sensitivity issue, and not CRLF - .gitattributes pins * text=auto eol=lf. Redirection was not at fault either: ensureScriptsOut derives all five --out flags correctly and gen-exit-code-registry.cjs honours them with no __dirname escape. Fixed at the cause: no test writes to a committed file any more. Both corruptors now copy to a mkdtempSync tmpdir, corrupt the COPY, and point the generator at it. Repinning or reordering the shards would have turned CI green while leaving the landmine armed for the next reshuffle. That required closing an inconsistency between two sibling generators. gen-exit-code-registry.cjs already accepts --out/--scripts-out/--hooks-out/--dts-out/--sh-out and honours them under --check; gen-hooks-cli-exit.cjs hardcoded OUTPUT_PATH and had no flag surface at all, so its corruptor could not be redirected anywhere. It now takes --out in the same style, honoured by both --write and --check, and is a no-op when absent - verified: a bare --check on the default path still exits 0. ensureScriptsOut moved to tests/helpers/exit-code-artifact-flags.cjs and both test files import it. Hand-rolling a second copy of the flag derivation would have been a re-derivation of exactly the kind this PR ships a guard against. Verified: both tests still detect corruption (proven by defeating the check and watching them red, with a positive control showing an uncorrupted copy exits 0); SHA-256 of hooks/lib/exit-code-registry.js and hooks/lib/cli-exit.js identical before and after running both rewritten bodies, and git reports nothing under hooks/ modified - that is the property that was violated. A repo-wide search for the corrupt-then-restore-in-finally shape against hooks/ found no other instances. One detail worth recording: the tmpdir test keeps --declaration pointed at the real committed declaration rather than copying it, because the generated banner embeds path.relative(REPO_ROOT, declarationPath) - copying it would produce a false drift unrelated to the injected corruption. The declaration is read-only on that path and never written. Refs #3987 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-08-28 14:40:02 -04:00
Tom Boucher	3a6c0412a9	enhance(#3624): local/no-exact-case-env-access — ratchet ADR-1703 onto production env reads (epic #3411 Phase 4) (#3976) ... * enhance(#3624): local/no-exact-case-env-access — ratchet ADR-1703 onto production env reads (epic #3411 Phase 4) Extends ADR-1703's portability rule catalog with a second production-runtime rule: it flags an exact-case read of a Windows case-varying environment variable (PATH, PATHEXT, ComSpec, USERPROFILE, TEMP, TMP, APPDATA) off any receiver that is not process.env itself, matched via an env-shaped-receiver check to avoid colliding with ordinary `.path`-named properties elsewhere in the tree. Exports the seam's private `_envGet` as `envGet` so the rule's remediation message names a real helper, and fixes the one pre-existing violation the tightened rule found (`src/runtime-hooks-surface.cts`'s `env.APPDATA` read). Closes #3624 * fix(#3624): extractStaticName recognizes non-computed Literal destructuring keys; add missing accessor-call test case Review findings from the code-review + isolated-adversarial passes: - extractStaticName only matched non-computed Identifier keys, so a destructuring like `const { 'PATH': v } = opts.env;` (the issue's own I8 acceptance case) silently evaded the rule. Widened to accept a Literal key regardless of computed, which is safe for MemberExpression too (its non-computed property is always an Identifier by grammar). - Added the missing RuleTester valid case for "a case-insensitive accessor call" (envGet(env, 'PATH')) from the issue's Done-when checklist. * docs: backfill changeset PR number for #3624 (PR #3976) --------- Co-authored-by: sim <sim@local>	2026-08-28 08:49:03 -04:00
Tom Boucher	d98b55562c	enhance(#3910): the raw terminator is banned by construction (#3980) ... * enhance(#3910): move the last src/ terminators onto the seam Phase 6 bans the raw terminator by construction, which it cannot do while violations stand. A census found 12 sites the rule would flag; nine of the ten unsanctioned ones were owned by no phase of the epic at all — a coverage hole in the decomposition, since P0-P2 are infra, P3 the gate modules, P4 the scanners, P5 the fragments, P7 the hooks, P8 io.cts, and P6 itself only adds the rule. `src/**/*.cts` now holds exactly 2 raw exits, both inside `terminateNow`, the single sanctioned site. `io.cts`'s `error()` is the interesting one. It was first called substantive on "dozens of callers, contract risk" — asserted, not measured, and the measurement refuted it: 289 call sites, zero inside a try whose catch would swallow a throw. The real obstacle was structural instead: `terminateNow` cannot emit exit 1, because ADR-3889 §1 makes 0 and 1 unallocatable and `nameForExitCode(1)` throws. So the only route is `ExitError` under `runMain`, which sets exitCode and writes stderr only when the error carries a user message — keeping the existing stderr write and throwing a message-less ExitError is observably identical. That census was still too narrow, and running the CLI proved it. It asked whether the CALL sits in a try/catch; the two regressions that surfaced were interceptors elsewhere on the stack: - `command-routing-hub.cts`'s `dispatch()` swallowed the ExitError into a HandlerFailure, so the caller emitted a duplicated, wrong stderr line on every Hub-routed path. It now rethrows ExitError explicitly — the same shape `gsd-tools.cjs` already used at two dispatch sites, so this follows an established idiom rather than inventing one. - the profile-pipeline router's deliberately un-awaited `.catch(e => error(...))` turned an ExitError rejection into an uncaught exception; it now mirrors runMain's handling. `edge-probe` and `ui-consideration-probe` gained `runMain` wrappers because probe-core's new throwing default would otherwise have escaped them. A follow-up sweep of every dispatcher — 19 command routers, the Hub, the gsd-tools dispatch seams — found no further swallowing catch. The admitted bound: ~1260 non-rethrowing catches repo-wide were scanned structurally but not individually classified. Both real regressions were found by execution, not by reading, so the suite is the detector that matters here. `gsd-tools.cjs:253` stays a raw exit deliberately: it is the ensureRuntimeBuild bootstrap, which runs before cli-exit is required, so the seam does not yet exist. It needs a second allowlist entry, which means #3910's "single allowlist entry" criterion is unachievable as written. Verification runs on the remote runner. Refs #3910 * enhance(#3910): ban the raw terminator by construction Adds local/require-registered-exit and registers it on all four globs: src/**/*.cts, scripts/**/*.cjs, hooks/**/*.js, gsd-core/bin/**/*.cjs. Registering on the .cts glob is load-bearing, not redundant — the emitted .cjs mirrors are globally eslint-ignored, so a rule registered only on the emitted globs is blind to the sources. That is the #3496 lesson, and it is how the previous guard became invisible: n/no-process-exit was 'error' in one block yet fired zero times on all three surfaces that mattered. The dead n/no-process-exit: 'off' block for hooks is deleted in the same PR. Phase 7 migrated every hook, so the exemption now protects nothing. Two allowlist entries, not the one #3910 anticipated. terminateNow's body is detected STRUCTURALLY — a process.exit lexically inside a function of that name — rather than by a path and line number that rots. The second is gsd-tools.cjs's ensureRuntimeBuild bootstrap, an inline disable with its reason at the call site: it runs before ./lib/cli-exit.cjs is required, so the seam does not exist yet and no migration is possible. #3910's 'single allowlist entry' criterion is therefore unachievable as written, and is amended with the measurement rather than quietly missed. The rule is proven able to FAIL, per glob: four positive controls, one for each registered glob. A guard that cannot be shown to fire is not a guard. Four matching negative controls pin process.exitCode as never-flagged — conflating it with process.exit is what inflated this epic's original census 2x. An allowlist case and a near-miss (same shape, different function name) fix the structural detection in place. Verification runs on the remote runner. Refs #3910 * fix(#3910): stop the detached catch from throwing, and scope the allowlist Review findings, one of them a regression the previous fix introduced. _handlePipelineRejection called error() from inside a DETACHED .catch(). error() now throws, so that throw became an unhandled promise rejection — and on Node >=15 with --unhandled-rejections=throw, Node dumps a raw stack trace with absolute paths on top of the clean Error: line. That was impossible before this branch, because process.exit(1) terminated synchronously before any rejection machinery could observe it. The handler now writes byte-identical stderr itself, in both plain and --json-errors form, and sets exitCode in place. This was the THIRD interceptor found, and like the first two it surfaced by running the CLI rather than by reading code. The rule's terminateNow allowlist had no path constraint, so any function anywhere named terminateNow across all four globs inherited it. It now requires the structural nesting check AND a cli-exit.cts basename — still no line numbers to rot. The four per-glob positive controls only varied a filename inside RuleTester, which never resolves eslint.config.mjs. Since the rule is filename-agnostic, all four exercised identical logic and none proved the rule was WIRED — this epic's own failure mode. A registration test now asserts the rule resolves for a real path in each glob, and it is proven able to fail: removing one glob's registration flips the resolved value from [2] to undefined. Three evasions the rule cannot catch (computed member, aliasing, .call/.apply) are documented in its header and pinned by tests, labelled as known limits rather than endorsed, so a future change that starts catching them is a deliberate diff. Refs #3910 * docs(#3910): document the raw-terminator ban Reference and Explanation via a new docs/features fragment (FEATURES.md is generated from it, not hand-edited). How-To: docs/how-to/resolve-a-raw-terminator-finding.md, indexed from docs/README.md — a contributor whose code trips the rule picks among three replacements by surface (runMain/ExitError for a CLI path, terminateNow for a hook, process.exitCode where the process should drain), and needs to know why process.exitCode is correct and never flagged, since conflating the two is what inflated this epic's original census 2x. The page also names the three patterns the rule cannot catch and says plainly that using one to dodge it is a review finding, not a fix — documenting them without that sentence would read as a sanctioned workaround. docs/INVENTORY.md deliberately untouched: eslint-rules/ is not a tracked family in the manifest (verified — a regen produced a zero diff), so a hand-written row would desync the table from the family it claims to belong to. Refs #3910 * fix(#3910): a catch that sniffs the message swallows an ExitError The remote run returned 41 failures, and one of them was a live production regression rather than a test artifact. `cmdMilestoneComplete`'s unstarted-phase guard re-threw only when `e.message.startsWith('Cannot mark milestone complete:')`. `error()` used to `process.exit(1)`, uncatchable, so the guard always fired. It now throws an ExitError carrying no message, the string test fails, and the ExitError was silently swallowed — the guard stopped blocking milestone completion entirely. Proven against the real CLI: pre-fix, a milestone with an unstarted phase archived at exit 0; post-fix it is blocked at exit 1 with the intended message. That is a guard that silently stopped guarding, which is this epic's thesis appearing inside the phase meant to enforce it. Worth stating plainly: an earlier census DID examine this site, saw a `throw e`, and classified it as rethrowing. It was wrong — the rethrow is conditional, and a conditional rethrow on an inspected message is indistinguishable from an unconditional one unless you read the predicate. So the class was swept rather than patched where it was tripped over. An AST census of every CatchClause across src/, gsd-core/bin/ and scripts/ found 38 conditional rethrows. Two more had the same defect and are fixed the same way: `config.cts`'s `'No config.json'` sniff and `gsd-tools.cjs`'s `e.name === 'WindowsError'`. The remaining 25 are provably unreachable — every one wraps a bare fs, YAML, manifest-require or git-exec primitive that cannot throw ExitError — and two were scanner false positives, both explained. Each fix is an unconditional `instanceof ExitError` rethrow placed BEFORE any inspection, matching the idiom command-routing-hub and gsd-tools already used. Residual bound, stated rather than implied: zero known-reachable unfixed sites, contingent only on error() never later being called inside one of those 25 primitive try blocks. The remaining failures were harness artifacts, and the harnesses were corrected to the new contract rather than the assertions weakened. Tests that mocked `process.exit` to observe termination now catch ExitError and assert its code; tests parsing stderr as a single JSON object still assert exactly that, with their ad-hoc `node -e` scripts wrapped in runMain so it is true. milestone and phase-resolution-parity needed no test change — they were correctly written against the real bug and are what caught it. Verification runs on the remote runner. Refs #3910 * chore(#3910): backfill the changeset PR number Also reframes the fragment to lead with the user-visible change — the milestone guard blocking again — rather than the narrowest of the three fixes. Refs #3910 --------- Co-authored-by: sim <sim@local>	2026-08-28 03:15:39 -04:00
Tom Boucher	15af0f5536	enhance(#3951): B6+B7 — widen two unreachable lint rules and make the guard ledger true (#3965) ... * fix(#3951): two lint rules that could not reach the code they govern B6 names two widenings. Measuring them first turned up a defect the criterion did not know about, and refuted the reason it gave for one of them. 1. no-adhoc-markdown-parsing self-gates on its own filename. Lines 107-110 short-circuit create() to {} unless the path matches /(?:^|\/)src\/[^/]+\.cts$/. B6 says to widen the files: glob in eslint.config.mjs - but doing only that ships an INERT rule, because the gate still returns {} for every new path. Both halves have to change, and the gate is the load-bearing one. That same regex hides a live hole: [^/]+ is FLAT-ONLY, so it requires the file to sit directly in src/. The registered glob is src/**/*.cts, which includes subdirectories. 28 .cts files - health-diagnostic-rules/ (10), installer-migrations/ (11), observability/ (3), host-integration-adapters/ (2), vendor/ (2) - are inside the registered glob and silently skipped. Measured with the gate neutralized: 0 violations there today. The hole is hiding nothing right now, and is fixed anyway, because "no violations today" is not a property that keeps holding. The fix is not invented: require-subprocess-timeout.cjs:196 already carries the correct form of this guard, /(?:^|\/)src\/.*\.cts$/ with .*, one directory over. Checked the other 21 rules for the same bug - no-adhoc-regex-escape and no-private-binary-resolution short-circuit only to exempt their own seam file, which is the right shape, and no-crlf-fragile-split has no filename gate at all. This bug is unique to the one rule. 2. no-adhoc-regex-escape could not see the shape that actually occurs. Line 396 gated the whole UNSAFE-NEW-REGEXP arm on arg.type === 'Identifier'. Every check below it - the _SOURCE provenance check, the isSoleReturnOfOwnParameter shape - lives inside that branch, so new RegExp(obj['key']) and new RegExp(cfg.pattern) were never examined at all. Runtime data arrives as a property access far more often than as a bare identifier, which is exactly why this rule never fired on the #3477 ReDoS. Widened to MemberExpression, measured by AST walk across all five registered blocks rather than by grep. 27 sites, zero TSAsExpression: 18 safe new RegExp(X.source, flags) -> exempted, keyed strictly on the PROPERTY being `source`, never on the object. Keying on the object would wave through X.anything and buy nothing. B6 estimated ~10; that was an undercount. 3 _SOURCE-suffixed constants reached through a required module namespace (phaseId.BRACKET_PHASE_TOKEN_SOURCE) -> the same provenance-exempt class the rule already recognizes for bare identifiers, extended to reach them. Without this the widening produces 3 false flags. 6 real findings -> marked, each a test extracting a pattern from a shipped file at test time, where the runtime contract IS the product. Deliberately the NARROW MemberExpression form. The rule's own isSoleReturnOfOwnParameter doc comment records that an earlier broad "any non-literal identifier" heuristic produced ~25 false positives and was rejected; a re-run of the census after this change flags exactly the 6 above and nothing else. Verified by execution, not by reading: the gate now accepts src/<subdir>/x.cts, still accepts flat src/x.cts, and still exempts paths outside src/ - each pinned by a test proven to fail against the old regex. build:lib, lint and lint:ci all exit 0. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3951): give no-adhoc-markdown-parsing its reach, and fix the 80 parses it finds The rule self-gates on filename AND is registered on one glob, so widening either half alone is inert. Both move here: the gate now accepts tests/**/*.cjs and scripts/**/*.cjs alongside src/**/*.cts, and eslint.config.mjs registers it on the same two. A test pins that the gate and the registration AGREE, in both directions. The original defect was a gate narrower than its registration; the failure mode of this fix is a gate wider than its registration. Both are silent, so the test asserts the pair rather than either half. 80 violations across 43 files, all in tests/, zero in scripts/. 70 are routed through the existing seams - scanFencedBlocks, collectSection, stripFencedCode, tokenizeHeadings from markdown-sectionizer; splitTableRow, parseMarkdownTable, findTableWithColumns from markdown-table. Headerless STATE.md tables use splitTableRow per line, because parseMarkdownTable needs a real delimiter row. 10 are suppressed, 12.5%, well under the third that would have meant the rule is mis-scoped for tests/ rather than the tests carrying debt. Each names its reason: three regression guards (#3873 / bug-#21) are deliberately independent of the generator's own fence handling, and routing them through the seam would have them test the generator against itself; one is a negative-text probe that extracts nothing; six are a shell-pipe-to-jq detector whose regex coincidentally matches the table fingerprint and is not markdown parsing at all. All ten sit in tests whose subject is .md content, which is normally a reason to prefer the seam. The marker used is allow-adhoc-markdown, distinct from no-source-grep's allow-test-rule, and lint:ci's lint-allow-test-rule-refs reports the same 280/280 unverified count as before - checked rather than assumed, because those two markers are easy to conflate. The widening earned its keep immediately: it found a test that passed for the wrong reason. tests/config-field-docs.test.cjs asserted notEqual(<cell>, '600') against the TYPE column instead of the DEFAULT column. notEqual('number', '600') is true forever, so the guard against workflow.subagent_timeout regressing to the old seconds default could never fire. docs/CONFIGURATION.md:434 is `| workflow.subagent_timeout | number | 300000 | ... |`, so the default is cell index 2; the assertion is now row-scoped through splitTableRow and reads 300000. That is the argument for the widening in one case: the violation was invisible to lint, the suite was green, and the assertion was vacuous. A rule that cannot reach a file cannot tell you the file is lying. Not fixed here, and recorded rather than assumed: #3426/#3239 are NOT reachable by this widening. tests/package-legitimacy-gate.test.cjs yields zero violations even with the gate bypassed - its hand-rolled scans are real, but built from line filters and split('|') rather than the regex-literal fingerprints this rule detects. They need new detectors. The epic assumed a wider glob would catch them. build:lib, lint and lint:ci all exit 0; the post-fix census across tests/** and scripts/** is 0 violations. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3951): B7 — and #3356's defects were still live in the code B7 asks that each closed child be driven fail-first with a behavioral identity test at the CONSUMER's output. Four of eleven children had no test citing their issue number. Auditing them by BEHAVIOR rather than by number-grep changed the answer for three of the four. #3364 and #2540 — traceability only. Both were implemented by #3941 and their consumer-output tests exist and were shown failing-first; neither cited its originating issue, so an audit that greps for the number reports them uncovered. Tagged the specific asserting test in each file, following the citation form those files already use. #3372 — covered, but only at helper level, and the triage narrowed it. Of the four commands the issue names, only estimate-cli's collectCalibrationSamples actually enumerates phase dirs from disk; smart-entry, audit and roadmap-upgrade derive from ROADMAP/body text and never reach the sentinel path, so they are benign by construction and were left alone rather than "fixed" into churn. The existing #3882 rows asserted the helper's return value. Added a consumer-output test driving `query estimate-calibrate` and asserting sample_count and the persisted document. RED proof: reverted collectCalibrationSamples to a raw readdirSync and ran the real CLI - sample_count 3, sentinel leaked; restored - sample_count 2. #3356 — NOT covered, and BOTH halves of the defect were still live in source. The issue is closed; the bug was not fixed. Fixed here rather than writing tests that document a bug as correct. Defect 1, the contradicted row. quick.md:627 claimed `quick-tasks-append` performs "the equivalent write" to the Step 7c row. It did not: the `#` cell was a positional ordinal and `Directory` read `—`, because the route had no way to receive a quick id or task directory. Added OPTIONAL `--quick-id` / `--slug` / `--directory`. A caller with neither - fast.md, the original #2133 caller - omits them and gets the byte-identical prior row, so nothing existing changes. A caller that HAS a real id and directory now gets the canonical row quick.md:632 renders. The false-equivalence sentence itself is corrected rather than left to mislead the next reader. Defect 2, the forced re-derive. The route called readModifyWriteStateMd with no options, so a body-only append to the Quick Tasks table triggered a full re-derive of the disk-derived progress.* frontmatter. Every other body-only writer passes { resync: false } - src/state.cts's own docstring prescribes it - and this route was the lone outlier. RED proof: reverted the option, seeded a project with 2 real phase dirs and a curated total_phases of 25, ran quick-tasks-append; total_phases collapsed to 2. Restored; it stayed 25. That second one is the shape this epic exists to close: a silent write that replaces curated state with a re-derivation nobody asked for, exit 0 throughout. build:lib, lint and lint:ci all exit 0. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3951): amend B6's ledger to what was measured, and document the new flags The ADR gains a ledger amendment in its own correction style - the sixth wrong premise it records, found the same way as the other five, by measuring before building. B6 says the net guard count must fall. It rose: 62 -> 69, +7, measured from the epic's filing commit to origin/next. The attribution is the point, though. Five of the seven came from PRs unrelated to this epic, one was added by a phase of it, and the epic did retire something sub-file - #3884 removed a detector with an explicit "net: -1 detector, 0 added" ledger. Every named casualty is load-bearing, two already carry retractions in this same document, and a sweep of all 22 rules plus every scripts/lint-* found no provably dead guard. There is no honest way to make the count fall; forcing it would trade coverage for a number, which is the Goodhart outcome Decision 6 exists to prevent. The amendment also records that B6's own prescribed fix for one widening was inert. no-adhoc-markdown-parsing self-gates on its filename, so widening only the files: glob - which is what the criterion says to do - ships a rule that still returns {} for every new path. And #3426/#3239 are not reachable by that widening at all; their scans use line filters and split('|'), not the regex fingerprints the rule detects. The roster row tracked them against the wrong mechanism. Three roster rows updated from aspiration to fact: the two widenings are DONE with their measured counts, and lint-phase-enumeration-drift is marked RETAINED rather than "expected casualty - verify before retiring", because Phase 5 verified it and kept it. The rule Decision 6 should carry forward is stated plainly: a guard ledger is a claim about COVERAGE, not about COUNT. "Net count must fall" is measurable and wrong. "Every guard is reachable, and each retirement names what makes its defect unrepresentable" is the property that was actually wanted. CLI-TOOLS.md documents the optional --quick-id/--slug/--directory flags and says plainly that omitting them keeps the pre-#3356 row byte-identical, plus that the append no longer re-derives progress frontmatter. New features fragment (id 3951); FEATURES.md regenerated rather than hand-edited. Changeset is Changed, pr:0 pending backfill. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3951): correct four rows that pinned the lint rule's old narrow reach The remote suite came back RED with 5 failures, all in tests/eslint-rules.test.cjs. They are stale tests, not a regression: four rows assert that no-adhoc-markdown-parsing is inert outside src/*.cts, which is exactly the contract this deliverable changes. Confirmed by reading rather than inferred from the names - the row at :1981 used filename: 'tests/some.test.cjs' and filename: 'scripts/helper.cjs', the two roots the rule now covers on purpose. Worth recording WHY local gates missed this. npm run lint and lint:ci were green, and the touched test files passed standalone. Lint only reports violations in real files; these rows assert the rule's REACH using synthetic RuleTester filenames, so nothing but the full suite could see them. Local green on a rule change says nothing about the rule's own tests. Each row is rewritten with BOTH halves rather than flipped from valid to invalid: - the same fingerprint under tests/ or scripts/ is now flagged, with the right messageId - the negative space is preserved - the same fingerprint under a path outside all three roots (gsd-core/bin/lib/foo.cjs) is still NOT flagged The second half is the one that matters. Without it the rule has no boundary and nothing would catch an over-wide gate later, which is the mirror image of the bug this deliverable just fixed. Each row is renamed to state the current contract; the old names said "non-src/*.cts ... is not flagged" and would have been actively misleading once the bodies changed. Proven to test the widening rather than restate it: every flagged half was run against HEAD~2's pre-widening rule and does NOT fire there, then against the current rule and does. 12/12 on that probe; the full file is 178/178. Swept for the same staleness elsewhere and found none. require-subprocess-timeout's own "inert outside src/*.cts" row is untouched - that rule's gate was not widened here - and no-adhoc-regex-escape's test file already carries correctly-targeted rows. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3951): acknowledge the quick.md growth the attribution guard reported The full suite came back RED with one failure, and it is mine: 1 file(s) grew without an acknowledgment: quick.md grew 364 bytes gsd-core/workflows/quick.md is runtime-loaded emitted content, so correcting its false 'performs the equivalent write' claim trips emitted-attribution by construction. This is the acknowledgment, not a workaround - there is nothing to regenerate. The fragment names ONE path, which is the only one the guard reported. The four spent acknowledgments it also listed (audit-uat, plan-phase, progress, review) belong to other fragments whose ripple the base already absorbs; they are inert, not failures, and are deliberately NOT copied here - naming paths I did not change would make this record false in the other direction. Byte figure corrected before committing: the guard reported 37220 -> 37584 (+364), but origin/next has since moved and quick.md is 37232 there now, so the measured delta is +352. The reason text says so and names the base as a moving figure rather than pinning a number that is already stale. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3951): move the quick.md growth ack to a trailer, delete the obsolete fragment The acknowledgment mechanism changed under this branch. Merging next brought in the redesign - it also deleted .github/workflows/ack-fragment-sweep.yml, which was in the merge status and which I did not register at the time - and the guard now says so directly: Add a trailer to a commit in this PR (never a new file). Emitted-Drift-Ack-Growth: quick.md - <why this growth is deliberate> So tests/emitted-drift-acks/3951-quick-append-equivalence.json is obsolete on arrival. A fragment file is no longer read by anything, and leaving it would be a dead record that looks like an active one. It is deleted here rather than kept "just in case". The byte figure moved again with the merge: 37232 -> 37596, +364. The earlier fragment said +352, measured before the merge auto-merged quick.md itself. The trailer carries no number, which is the better design - the figure was stale twice in two attempts. Refs #3951 Emitted-Drift-Ack-Growth: quick.md — #3356/#3951 replaces a false claim with an accurate one. Line 627 said the `quick-tasks-append` shortcut "performs the equivalent write" to the Step 7c row rendered above it; it did not, and that was the documented half of #3356 — with no quick id or task directory the route emitted a positional ordinal in `#` and an em-dash in `Directory`, a visibly different row. The corrected sentence has to carry three facts the original elided: what the shortcut actually writes when it has neither input, that this is honest behavior for its real caller (`fast.md`, which has neither), and how a caller with both now gets the byte-identical canonical row via the new optional `--quick-id`/`--slug`/`--directory` flags. Prose is the product here — an executing agent reads this line to decide whether the shortcut is safe for its case, and a shorter correction would either drop the flags (leaving the reader unable to act on the fix) or drop the limitation (recreating the false claim in gentler words). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3951): backfill changeset pr number Refs #3951 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-08-27 23:10:49 -04:00
Tom Boucher	9d65cd5404	fix(#3664): warn when config-dir targets a foreign-agent destination (#3794) ... * test(#3664): failing-first rows for the config-dir foreign-agent warning * fix(#3664): warn when config-dir targets a foreign-agent destination * test(#3664): fold the foreign-agent warning rows into the install-regressions suite * fix(#3664): close review findings — kimi-agents kind, gsd.md ownership, e2e gate * test(#3664): sync boolean call sites and the path-vocab registries * chore(#3664): backfill changeset pr number * test(#3663): skip the posix case-pin on win32 where folding is the fix --------- Co-authored-by: sim <sim@local>	2026-08-24 02:45:28 -04:00
Tom Boucher	2972da4c9d	enhance(#3619): ratchet the platform seam with local/no-private-binary-resolution (epic #3411 Phase 3) (#3636) ... * chore(#3619): ratchet the platform seam with local/no-private-binary-resolution Epic #3411 Phase 3, the ratchet. Scope revised with maintainer approval and recorded on the issue: the epic's literal ask was a rule rejecting a bare-name spawn outside the seam. Surveyed at ac1b6d679, ~30 such sites exist and none is a defect — git, gh and npm ship native .exe that CreateProcess resolves unaided, and the rest are POSIX-only tools. ADR-1703 rules 2 and 3 forbid grandfathering and escape hatches, so a literal rule would be unsuppressable and would force rewriting 30 correct calls. The epic's actual thesis was four private RESOLVERS, not four bare spawns. So the rule flags re-implementing resolution: reading PATHEXT in any casing from any object, and a hardcoded list carrying two or more of .exe/.cmd/.bat/.com — precisely the shapes fallow-runner's candidateNames and gsd-tools' PATHEXT string had before Phases 1 and 2 deleted them. Three boundaries were arrived at rather than assumed: two-or-more a single .endsWith('.cmd') is a classification, not a candidate set; runtime-hooks-surface derives .cmd shim paths that way boundary-aware a naive substring test flags .execute and .compacting, caught on src/host-integration.cts before it could become a false positive nobody could suppress suffix-anchored the seam exemption matches src/shell-command-projection.cts exactly; a substring match would also exempt the dispatch test file. Case I9 pins it. PATH scans are deliberately NOT flagged — membership checks (bin/install.js) are indistinguishable from resolution scans, and an unsound rule in a zero-escape-hatch architecture is worse than no rule. To make the ratchet strict with no carve-out, resolveExecutableBinary gained pathOverride: search THIS PATH, read everything else including PATHEXT from the ambient environment. resolveFallowBinary now supplies its own search path without hand-threading PATHEXT, which would itself have been a private read. The three alternatives were all worse: exempting the file is grandfathering, exempting the AST shape is a carve-out every future caller must replicate, and dropping the pass-through would silently ignore a user's real PATHEXT — buying a lint rule with a correctness regression. eslint-rules/** is outside the rule's globs rather than exempted, because portability-vocab.cjs owns the extension set. scripts/**/*.cjs got its own block so that exclusion does not leave a hole in the ratchet. Started green with nothing suppressed. Proven able to fail: a fixture with both signals reports two errors. Refs #3411 * fix(#3619): close the PATHEXT destructuring evasion and correct two overclaims Adversarial review found a trivial evasion of the rule's primary signal: the visitor only handled MemberExpression, so const { PATHEXT } = process.env const { PATHEXT: exts } = process.env const { Pathext } = opts.env were all unflagged. That is a common idiom, not an exotic bypass. An ObjectPattern visitor now catches it in every form — renamed, any casing, any receiver, string keys — while leaving a computed key alone, since it is not statically decidable. I10-I13 pin the invalid forms and V9/V10 pin PATH and the computed key. Two overclaims corrected, both mine: Standards review proved the docs were factually wrong. Both the ADR amendment and the CONTEXT.md entry asserted that tests/shell-command-projection-dispatch.test.cjs is still linted by this rule. It is not — the rule's surface is src, gsd-core/bin, scripts and hooks, and tests/** is deliberately outside it because test setup legitimately assigns process.env.PATHEXT (fallow-runner's P3 does exactly that). The suffix-vs-substring distinction is therefore proven by RuleTester case I9 feeding a synthetic filename, NOT by real coverage of that file. Both documents now say so. The rule's own docstring claimed the seam exemption matches the seam path 'exactly'. It is a suffix match, so a nested foo/src/shell-command-projection.cts would also be exempt. Suffix matching is kept — it is how sibling rules resolve paths and the nested case does not exist — but the docstring now states the boundary rather than overstating the precision. The evasion fix was verified by executing eslint against both destructuring forms in scripts/, not by inspection. Probe: 31/31. Refs #3411 * chore(#3619): backfill changeset pr number 3636 --------- Co-authored-by: sim <sim@local>	2026-08-18 16:40:17 -04:00
Tom Boucher	bc557f6876	chore(#3520): ratchet on effective exemptions, track unverified separately (#3529) ... Phase 5 of #3464, following #3465, #3466, #3502 and #3508. Those cut the ceiling 305 -> 278, made the rule accurate, and ended file-wide amnesty. This one fixes the number itself. scripts/lint-allow-test-rule-refs.cjs counted FILES CONTAINING MARKER TEXT. Only 5 of those files carry a marker that actually suppresses a violation the rule detects, across 10 sites. The ratcheted number was ~98% noise, which is exactly why bumping it was frictionless: the metric was never coupled to the thing it claimed to govern. That is the whole complaint this epic opened with, stated precisely. Verified directly rather than assumed: only eslint-rules/no-source-grep.cjs functionally honors the marker. Four other rule files mention allow-test-rule in prose only, and no-raw-rmsync-in-tests.cjs:24 explicitly states it does not apply. So the large count was not legitimately large because several rules share the annotation. Now two numbers, only the first ratcheted: EFFECTIVE EXEMPTIONS -- markers that actually suppress a detected violation. 10 sites across 5 files. Tightly ratcheted in both directions, as before: over the ceiling fails, and slack beyond grace fails. UNVERIFIED MARKERS -- marker-bearing files with no detectable violation. 273 files. Reported and given a loose ceiling so the pool cannot silently balloon, but deliberately NOT tightly ratcheted, because shrinking it is a rule-coverage problem and not a delete-the-markers problem. A file with at least one effective site counts as effective and is not also counted as unverified; the two numbers never double-count. Reporting ONLY the effective count was considered and rejected. It would say five files and look excellent while being falsely reassuring, because "no detectable violation" is not "no violation". This phase's own measurement found two genuine source-greps that are unsuppressed AND undetected -- tests/security-prompt-injection.security.test.cjs:852 and tests/check-update-config-dir.test.cjs:91 -- each reading a real shipped file and text-searching it, invisible only because the path is bound to a separate const the rule never resolves back to its literal. Markers guarding that class count as zero-effective and would look vestigial. Trading a number that is too big and meaningless for one that is too small and falsely reassuring is not progress, so the script prints the known-limit caveat alongside the numbers and the two undetected violations are filed separately rather than lost. Single source of truth is structural, not a matter of discipline. The script does not re-implement detection or the site-scoped adjacency predicate -- that is the generative-fix-divergence class this repo has shipped before. The rule now exports MAX_MARKER_LOOKAHEAD_LINES, MARKER_COMMENT_RE, collectMarkerAndCommentLines and isSuppressedAt (extracted verbatim, no logic change), plus a default-off neutralizeSuppression option so the counter can enumerate every site through the real rule via ESLint's Linter API and then classify each with the rule's own predicate, replicating reportUnlessSuppressed's search-line-OR-read-line check exactly. Default rule behavior is byte-identical: tests/eslint-rules.test.cjs passes 168/168 unchanged. A parity test asserts the script's suppressed/not verdict equals the rule's own report/no-report outcome for every site in a fixture corpus. A real silent-failure bug surfaced and was fixed while building this: ESLint's flat-config Linter reports "No matching configuration" and returns ZERO messages for any filename resolving outside its cwd. That would have quietly misclassified every sandboxed test fixture as having no violations -- a test suite that passes while asserting nothing. Fixed by anchoring the Linter to the tests dir, with a defensive throw if it ever recurs. Two earlier claims of mine are corrected by this phase's measurement. Widening the source-dir allowlist to include hooks/ -- the "fifth blind spot" recorded in #3508 -- rescues ZERO sites; it is real in principle and has no practical effect, because the hooks reads that exist are missed for other reasons (.sh extension, identifier-indirection, dynamic filenames). And the #3508 correction that attributed those reads to the hooks/ gap rather than to variable indirection was itself incomplete: both are independently sufficient, so fixing either alone changes nothing. I accepted the reviewer's causal claim as uncritically as I had made my own. The unverified count is 273, not the ~289 in the phase design doc. That is legitimate drift -- the baseline was measured at fba7c9032 and other merged work has since removed markers. Left as measured rather than adjusted to match the document. Adversarial review found a BLOCKER in the first revision and it is fixed here. The counter walked only tests/**/*.test.cjs, but the rule is registered on tests/**/*.cjs -- every .cjs, not just test files -- plus scripts/**, eslint-rules/**, bin/lib/**, pi/**, examples/**, gsd-core/bin/** and three plugin globs. 35 non-.test.cjs files under tests/ were never walked, and tests/helpers/live-command-registry.cjs:1 carries a real marker that appeared in NEITHER reported number. A counter that undercounts is worse than the meaningless one it replaces, because it will be trusted. The scan set is now derived programmatically: the script dynamically imports eslint.config.mjs and extracts the `files` globs from every config block that enables local/no-source-grep. There is no hardcoded list to drift, which is the same divergence class the parity test already guards. Two further defects surfaced while fixing it. The silent-clean catch around linter.verify() was worse than reported -- ESLint signals a parse error by returning a message with fatal:true rather than throwing, so the original catch would not even have fired for the common case. Both paths now throw with the file path. That silent swallow was actively hiding a broken fixture in this suite's own tests: 'no marker here\n' is not valid JS and the case only "passed" because the parse failure was discarded. Fixed. And once the scan widened, the raw-text marker scanner started matching this tooling's own doc comments and RuleTester fixture strings, so marker extraction moved to AST comment nodes. That corrected three long-standing FALSE POSITIVES: allowlist entries for tests/eslint-rules.test.cjs that were never real markers, only fixture payload. Allowlist 137 -> 135: three false positives pruned, one real entry added for live-command-registry. The headline numbers are coincidentally unchanged (10/10 effective across 5 files, 273/280 unverified) but the composition is corrected: one real file gained, one phantom dropped. Verified by direct diff rather than inferred from the totals matching. The first remote run of this branch came back RED with 23 failures, all one cause, and it is fixed here. Classification drove ESLint's flat-config Linter, which resolves configuration relative to a cwd and refuses to lint any file outside it. The test harness writes fixtures into an OS temp dir, so every sandbox row hit "No matching configuration found" and tripped the defensive throw. It surfaced only on the container because the repo lives at /work there and the fixtures at /tmp, making the mismatch unmissable; a local run had reported the suite green, which it was not. Fixed by not depending on config *resolution* at all: the script now builds an eslintrc-format Linter and registers the rule directly with defineRule, since it already knows exactly which rule and options it wants. That removes the cwd-ancestor constraint entirely and makes repo files and out-of-tree fixtures classify identically. Verified out-of-tree explicitly, not just in-repo, because the local temp path shape is what hid the bug the first time. Parse failures still throw loudly -- that behavior is required and tested. Classifications are unchanged for real repo files (10/10 effective across 5, 273/280 unverified, 0 live), which is the check that the config swap did not quietly alter results. The second remote run cut the failures from 23 to 2, and the survivors were a genuinely different and more interesting defect: tests/packaging-shipped-scripts-require-only-shipped.test.cjs caught that scripts/ SHIPS in the published package while eslint-rules/ does not, so importing the rule for single-source-of-truth would MODULE_NOT_FOUND in a real consumer's install. That test statically extracts require() calls, so hiding the import inside a function would have dodged the check without fixing the problem. Resolved along the grain of existing convention rather than by weakening anything: package.json already excludes several repo-internal lint gates from the shipped set via `!scripts/...`, including `!scripts/lint-no-adhoc-regex-escape.cjs`, which is the same situation. This gate is CI-only and has no meaning in a consumer install, so it joins them. Verified with `npm pack --dry-run` that the .cjs is genuinely absent from the tarball (its inert JSON config files remain, and carry no requires). The third remote run failed on shard 3/3 across all three OSes with exit code NULL and empty output -- the spawned gate was killed by a timeout, not failing an assertion. Cause: replacing a raw text scan with a full ESLint Linter pass over every file in every registered glob took the gate from 0.57s to ~7-12s, and GitHub's runners are slower than the bench that had just passed it green. Fixed by narrowing the work rather than raising the timeout to hide it. Both numbers the gate computes are properties of files that CONTAIN a marker, so only those (~294) need linting; the repo-wide byte walk that finds them stays, since that was the undercount fix. Live violations in files carrying NO marker are already enforced by npm run lint over exactly these globs, so re-detecting them here was redundant. 2.87s now, from ~7s measured locally. That narrowing changes what one reported number means, so the wording changed with it: "live violations in marker-bearing files: 0 (unmarked files are enforced separately by npm run lint)". A number that quietly covers less than it reads is the failure this whole epic is about, so it is stated rather than left implicit, and the test row that asserted the old broader behavior was split -- an unmarked live violation now passes this gate (and is caught by eslint), while a live violation in a marker-bearing file still fails it. The repo-baseline test's timeout was also raised to 30s with a comment, since a gate that legitimately takes ~3s must not sit at a timeout close to its own runtime. Every other row keeps the shorter sandbox-scoped timeout. Closes #3520 Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-15 00:33:31 -04:00
Tom Boucher	c66b010052	chore(#3508): site-scoped allow-test-rule suppression (#3512) ... Phase 4 of #3464, following #3465, #3466 and #3502. Those cut the ceiling 305 -> 278 and made the rule accurate. This closes the remaining structural weakness: suppression was FILE-WIDE, so a single justified exemption silently absolved every other source-grep in that file, forever, including ones added later by someone else. hasAllowAnnotation did comments.some(...) over the whole file and returned {} early. A marker is now checked per report: a violation is suppressed only by a marker on its own line, or on a line above it with nothing but blank lines and other comments in between, bounded by MAX_MARKER_LOOKAHEAD_LINES = 8. The bound is comment-purity rather than raw distance, and that distinction is load-bearing: an intervening line of real code (a `test(...)` opener, say) ends the window even when the marker is physically close. Chosen from the actual placements in the affected files rather than picked a priori -- the repo's convention puts several lines of prose rationale between the marker and the code, so a tighter rule would have invalidated legitimate existing markers and forced churn for no correctness gain. Measured before writing any code, by running the real rule with the suppression check neutralized across all 1194 files its globs match: 14 violation sites in 8 files, and ZERO in files carrying no marker -- so the green build was legitimate, and the entire migration surface was those 14. 11 sites were mechanical: an existing marker already stated the right reason, it just sat too far away. Those were relocated to their call sites with the original #NNN citations preserved. Three were orphans -- the file's markers were about an entirely different concern and nobody had ever justified these reads. All three are fixed BEHAVIORALLY, with no new markers: install-minimal-hooks.test.cjs:975 asserted src.includes('gsd-update-check') && src.includes('replace(') against bin/install.js. It now calls the exported stripStaleGsdHookBlocks() on a legacy TOML fixture and asserts the actual stripped output. This is the case this phase was opened around: it could be added with no review friction and stay invisible indefinitely under file-wide amnesty. config.test.cjs:1917 regex-tested src/init.cts for detectGitCreateTag. It now drives `init complete-milestone` and asserts the git_create_tag field. config-schema.property.test.cjs:1107 did the same for detectFallowConfig; it now drives `init code-review` and asserts fallow_enabled. Each was proven RED against a broken production file and GREEN against the real one, with src/init.cts and bin/install.js confirmed byte-identical afterwards. Marker lines in the 8 files went 20 -> 24, against a filed expectation of "must not increase" (projected 14). That projection was wrong and is corrected on #3508 rather than met by deletion. It assumed every existing marker was a distant blanket that site-scoping would consolidate. Some are already site-adjacent and guard real source-greps the rule CANNOT detect -- verified in install-minimal-hooks.test.cjs:2686-2757, where seven markers each sit directly above a readFileSync(reloadScript) + .includes() pair reading hooks/gsd-config-reload.js. Removing them to hit a number would have repeated the Phase 1 mistake: deleting markers on "the rule doesn't fire" evidence when the rule provably cannot see the violation. An earlier revision of this commit message attributed that invisibility to the #3502 dynamic-path blind spot, on the grounds that reloadScript is a variable. Adversarial review caught that as a false causal claim and it is corrected here. looksLikeSourcePath's hasSourceDir regex is /['"](?:bin|lib|gsd-core|src)['"]/i, and those reads target hooks/ -- so a fully literal path.join(ROOT,'hooks','gsd-config-reload.js') is equally invisible. The variable indirection is irrelevant. This is a FIFTH, distinct blind spot: the source-dir allowlist omits hooks/, which is a real shipped production directory (eslint.config.mjs registers its own rule block for hooks/**/*.js). Recorded in 40-design.md Known limits and left for a follow-on phase -- widening the allowlist is unmeasured, and measuring before widening is the discipline #3502 established. The conclusion was right; the stated mechanism was not, and asserting an unverified cause is the error being corrected. The honest metric is not fewer markers. It is that every marker now sits adjacent to the specific read it justifies instead of absolving a whole file. Site-scoping turns one blanket marker covering N sites into N site markers by design; the count rising is the mechanism working. A second review finding is fixed here too. Suppression originally keyed only off the text-search line, so a marker placed directly above the readFileSync() call -- the intuitive place to annotate "this read is fine" -- did NOT suppress when the search sat on the following line, because the read's own assignment line breaks comment-purity. It failed safe (a loud error, never silent suppression), but it was a trap contributors would hit, and it contradicted this change's own claim that the placement rule would not force churn. A violation is now suppressed by a marker adjacent to EITHER the search site or the originating read. The violation is fundamentally the read+search pair, so annotating either half is legitimate, and it stays strictly site-scoped -- the decisive isolation row still holds. 17 RuleTester rows cover the new semantics. The decisive one asserts that a marker adjacent to one violation does NOT suppress an unrelated violation elsewhere in the same file -- exactly 1 error, reported at the second site. Teeth-checked by reverting the predicate to file-wide, confirming that row and two others flip pass->fail, then restoring. Two pre-existing RuleTester cases that asserted the old file-wide semantics were corrected. Compatibility held where it matters: 277 marker-bearing files have no detectable violation at all, and site-scoping makes their markers no-ops rather than errors. All stay green, untouched. Ceiling unchanged at 278; lint-allow-test-rule-refs reports 278/278. Deliberately not done here, and recorded for the follow-on phase: the same measurement found only 8 of 285 marker-bearing files contain a detectable violation. That suggests a large honest ceiling drop, but "the rule doesn't fire" is the unsound oracle that forced the Phase 1 revert of 295 files, and the rule still has documented blind spots -- as install-minimal-hooks itself demonstrates above. It needs two independent signals agreeing, which is only credible now that the rule is accurate. With suppression site-scoped, "an effective exemption" is finally well-defined, which is what makes re-pointing the ratchet at effective exemptions -- rather than at marker-text presence -- the natural next step. Closes #3508 Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-14 19:02:14 -04:00
Tom Boucher	e57e3b5d6e	chore(#3502): widen no-source-grep, close four measured blind spots (#3505) ... Phase 3 of #3464, following #3465 and #3466. Those phases cut the exemption ceiling 305 -> 278 by removing vestigial markers and rewriting real source-greps behaviorally. This one addresses why the ratchet was weak in the first place: it counts markers, and markers correlated only loosely with violations, because the rule's implementation was far narrower than its intent. Four gaps, each measured against 833 files under tests/ before any code was written: A TEXT_METHODS omitted matchAll, split and replace, and never handled the regex-side form re.test(tracked) / /lit/.test(tracked) where the tracked value is the ARGUMENT rather than the callee object. B The extension test was /\.(?:cjs|js|ts)/, which does not match .cts, .mts or .mjs. Under ADR-457 this repo's production modules live in src/**/*.cts, so the rule has been structurally blind to the entire TypeScript source surface since that migration. Highest-value fix here. C Tracking stopped at one hop, so an intermediate transform (const b = strip(a); b.match(...)) escaped. D Variables were tracked by NAME in a flat Set<string>, with no scope resolution, so a name reused across describe/test blocks was conflated. D removes one verified false positive where an outer `const src = readFileSync(...)` was cross-attributed to a shadowed arrow-function parameter of the same name. Deliberately NOT implemented: flagging reads whose path cannot be statically resolved. Measured at 4255 sites across 255 files, 66 of them newly red, with a 6-of-6 false-positive rate in spot-checking -- every sampled site read a markdown workflow or fixture doc through a path variable, not JS source. The heuristic "no JS extension literal present" inverts to "not a source file" in this codebase. Shipping it would have manufactured exactly the marker-spam dynamic this epic exists to stop. The principled version needs real static resolution (constant-folding path.join and template literals) and is left to a future phase. The widening surfaced two genuinely-invisible violations, handled on their merits rather than uniformly: tests/verifier-behavior-unverified.test.cjs read src/verification.cts and regexed it for the VERIFIER_STATUSES array. Fixed BEHAVIORALLY with no marker: that constant is already exported, so the test now asserts the real runtime value -- strictly stronger, and immune to source formatting. Mutation-checked: injecting present_behavior_unverified into the exported array turns it red, restoring turns it green. tests/adr-index-gate.test.cjs scans src/plan-drift-guard.cts for docs/adr/<name>.md citations and asserts each cited ADR exists. Those citations live in COMMENTS, erased at compile time: no exported value, no runtime observable, and making it behavioral would mean contorting production code into exporting its own documentation citations. Irreducible, so it takes one marker, cited to #3502, stating exactly why. Documenting a real exemption beats leaving the violation invisible, which was the status quo. Two defects in this branch's own work, both found by review and fixed here rather than shipped: FALSE POSITIVE (adversarial review). Hop propagation walked every Identifier in a declarator init and treated any reference to a tracked variable as derivation, regardless of whether the derived VALUE still carried source text. So `const len = raw.length; /^\d+$/.test(len)` was reported as a source-grep. Propagation is now value-shape aware: it follows identity, string-returning string methods, split/join, template embedding, string concatenation, conditional branches and call arguments; it stops at .length, numeric methods (indexOf/search/charCodeAt), boolean methods (includes/startsWith/test), comparisons, negation, typeof, and Number/parseInt/Boolean coercions. Unrecognized shapes still propagate -- the conservative default for a linter is a rarer false positive over a silent false negative, and that choice is documented inline. Seven RuleTester rows now cover this axis, which was previously untested. SUPER-QUADRATIC SCAN (security review). resolveVariable() resolved each identifier with two linear Array.find passes over scope.references and scope.variables, once per identifier walked -- O(vars-in-scope) per lookup. On a synthetic single-scope file of N consts each referencing ~20 priors: 4.76s at N=3000 and 24.32s at N=6000 (~5.1x for 2x N). Replaced with a Map<IdentifierNode, Variable> built once per file, lazily, from the scope manager: 0.19s and 0.37s for the same inputs (~1.95x for 2x N, linear). Semantics unchanged. No measurable effect on the real repo either way, but this is exactly the bug class ADR-3212 / local/no-unbounded-quantifier exists to catch, and this repo has a prior incident where a rule written to catch complexity bugs shipped with one of its own. The per-widening measurement had a flaw worth recording: each widening was measured in isolation, so a site needing TWO at once appeared in neither column, and the UNION column was dominated by the rejected dynamic-path noise and never inspected for interactions. adr-index-gate needs both B (.cts) and A (.matchAll) and was missed for exactly that reason. Real newly-red count was 2, not the 1 predicted. Corrected on #3502 rather than quietly amended. Marker-bearing files 277 -> 278 against an unchanged ceiling of 278. The ceiling is NOT raised: the sole addition is the cited irreducible exemption. 27 RuleTester rows cover the change. The valid rows carry as much weight as the invalid ones -- shadowed same-name bindings, sibling block scopes, .md and .json literal reads, dynamic path variables, non-textual derivations, reads never text-searched, and require() of a .cjs must all stay valid. A widening that flagged those would be worse than the status quo, because it would push contributors toward adding markers to silence noise. Known limit, documented rather than papered over: a tracked value round-tripped through an array or object literal and read back via destructuring is still not tracked. Pre-existing, not introduced here, and deliberately not widened for -- closing it means tracking member identity, with its own false-positive surface. Closes #3502 Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-14 17:19:24 -04:00
Tom Boucher	895d9df96d	fix(#3477): run untrusted key_links patterns on a linear-time engine (#3496) ... `cmdVerifyKeyLinks` compiled `must_haves.key_links[].pattern` from plan frontmatter with `new RegExp()` and tested it against whole file contents, so a nested-quantifier pattern such as `(a+)+$` hung `verify-phase` indefinitely (CWE-1333). JavaScript has no regex-execution timeout. Untrusted patterns now run on RE2 (re2js), whose match time is linear in input length — the class is closed by the engine, not by a heuristic screen. The screen lost in the ADR-0174 consolidation was deliberately NOT restored: it never worked, since `(a|a)*$`, `((a+))+$`, `(a+){2,}$` and `(a{1,3})+$` all evade it. A refused pattern's matcher returns false for every input, so it cannot report a match no matter what the caller does. The engine is vendored at gsd-core/bin/lib/vendor/re2js.cjs because gsd-core/bin/** is copied into installed trees with no node_modules; runtime dependencies are unchanged. New ESLint rule local/no-external-require-in-bin enforces that invariant, which had been documented in a comment since the #3024/#2071 bug class and enforced nowhere. Backreferences and look-around are unsupported by RE2 by construction — disclosed in a Changed changeset. Closes #3477 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-14 14:34:36 -04:00
Tom Boucher	69e7afd0c7	chore(#3212): bounded quantifiers over document content — prohibition with teeth — Phase 4 (#3441) ... * feat(#3415): ship local/no-unbounded-quantifier, burn down ReDoS class Phase 4 of epic #3212 (ADR-3212 §5/§7, the final phase). New rule flags an unbounded */+/{n,} quantifier over a broad character class ([\s\S], dotAll ., or a 1-2-unit negated class like [^\n]/[^)\n] — the exact #2128-fixed shape) applied to a regex whose match target is data-flow-traced to readFileSync content. eslint-rules/lib/readfilesync-trace.cjs extracts the data-flow tracer shared with no-crlf-fragile-split (Phase 2) rather than a second copy — no-crlf-fragile-split refactored onto it with zero behavior change, parity-tested. Real triage, not 798 mechanical edits: the ADR's census (2026-08-08) screened every unbounded quantifier in the tree unscoped. Correctly scoped to readFileSync-derived content (matching Phase 2's own G2/G3 scoping), the rule found 162 real hits across two detection waves — the second wave (93) surfaced only after a genuine off-by-one bug in this rule's own first draft was caught while writing its RuleTester tests and fixed (the bug silently missed every directly-quantified [\s\S]* with no gap before the quantifier — exactly the class this rule exists to catch). 3 hits landed in production src/ (commands.cts, milestone.cts, roadmap.cts) and were each empirically timed against adversarial input (matching #2128's own measured-not-assumed precedent) — all confirmed linear-time/benign, left unbounded with a measured-evidence comment rather than mechanically bounded. The remaining 159 are test-file fixture parsing (test-author-controlled, fixed-size content, not adversarial input) — each suppressed with a specific, non-generic reason. Zero functional behavior changed anywhere in this diff. tests/no-pending-3212-markers.test.cjs locks the epic's own closing invariant (ADR §7: "assert zero pending #3212 markers remain") — ground truth confirmed trivially true today (no phase left any such marker behind), now regression-locked going forward. Design: .gsd/phase/chore-3415-prohibition-with-teeth/40-design.md Test matrix: .gsd/phase/chore-3415-prohibition-with-teeth/50-test-matrix.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3415): correct rule category mislabel, add CI test-scope entry An orthogonal Standards-axis review found eslint-rules/no-unbounded-quantifier.cjs mistakenly carried meta.docs.category: 'Portability', copied from a sibling rule without realizing what that implied: docs/contributing/cross-platform- portability-rules.md governs an ADR-1703 rule family under a hard "zero escape hatches" contract (tests/portability-rule-disable-ban.test.cjs's PROTECTED_RULES bans eslint-disable for those rules entirely). This rule is not part of that family — it's ADR-3212 (ReDoS/CWE-1333), a different epic — and its eslint-disable-next-line suppressions (159 of them, added earlier this same phase after empirical benign-verification) are an intentional, correct design, not a bypass. Corrected to category: 'Best Practices', matching the actual precedent (no-adhoc-regex-escape.cjs, Phase 1 of the same epic, which is also correctly outside PROTECTED_RULES), and the rule's own docstring now states this explicitly so a future reader doesn't have to re-derive it. Also registers a new scripts/ci-test-scope.cjs bucket so editing this rule or the shared eslint-rules/lib/readfilesync-trace.cjs helper re-runs their own test suites under targeted CI selection — was previously unregistered and invisible to that fast-path (this PR's own gsd-test checkpoint runs the full suite regardless, so this only affects future narrowly-scoped PRs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3415): bound no-unbounded-quantifier's own scanner (CWE-1333, ironic) Security review found the rule meant to catch algorithmic-complexity bugs had one of its own: hasUnboundedBroadQuantifier's negated-class inner scan walked from each `[^` occurrence to the next `]` (or EOF) with no bound, while the outer loop only ever advanced by one character — O(n²) total work on a pattern with many unclosed `[^` runs. Runs unconditionally inside checkPattern on any `new RegExp('literal string')` argument in any linted file, before the (cheap) readFileSync data-flow gate — so a single crafted string literal, no valid regex syntax required, could make `npm run lint` / CI hang. Empirically confirmed both the bug and the fix: pre-fix, n=4000/8000/ 16000/32000 chars took 30.8/115.6/463.8/1874.3ms (~4x work per 2x n, quadratic); extrapolated, the 300000-char repro from the finding would run ~165s. Post-fix (bail the inner scan once units exceeds the rule's own 1-2-unit scope, rather than continuing to hunt for a closing `]`), the same 300000-char input runs in 8.7ms via the real rule module, independently reconfirmed at 18ms via a fresh Linter.verify() call. New regression row in tests/no-unbounded-quantifier.rule.test.cjs asserts the RuleTester run on a 50000-char adversarial pattern completes and returns a defined result — no wall-clock assertion (CLAUDE.md Clock Seams / local/no-elapsed-assertion). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3415): triage 3 new sites, re-raise ceiling after upstream batch next merged 12 more PRs during this PR's review. Two consequences: - tests/edit-phase.test.cjs (fix #3262, unrelated) added 3 new content.match(/<tag>([\s\S]*?)<\/tag>/) reads of this repo's own workflow .md content — the same Class A pattern as the ~159 sites already triaged elsewhere in this PR. Suppressed with the same established reason. - lint-allow-test-rule-refs' ratchet ceiling needed re-raising again (301 -> 303) for the same reason as the two prior bumps: organic growth from unrelated, already-reviewed PRs landing concurrently, not a defect in this branch's own diff. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-08-14 10:02:28 -04:00
Tom Boucher	0624c5da6f	chore(#3212): src/text-lines.cts is the sole owner of line-terminator handling — Phase 2 (#3420) ... * test(#3413): failing-first suite for the line-terminator seam Phase 2 of epic #3212 (ADR-3212 §3/§6/§7). Tests only — src/text-lines.cts does not exist yet, so tests/text-lines.test.cjs fails with MODULE_NOT_FOUND at its require line, which is the intended RED. The frontmatter.test.cjs additions drive #3360 (confirmed-bug) fail-first: parseMustHavesBlock currently returns [] for every must_haves block on a CRLF-authored plan file, because \r is its own LineTerminator in ECMAScript and two /m-anchored \s* patterns can absorb it, inflating a captured indent by one character and tripping the "not nested under must_haves" guard. Verified locally against the current (unfixed) compiled module: both the direct repro and the silent-exit "blank line before must_haves:" variant return [] today. A parity property test (crlf vs lf must deep-equal for every block name) matches a pattern this maintainer has required repeatedly for prior CRLF fixes in this codebase (Cortex-recorded, verify_intent=held). The no-crlf-fragile-split.rule.test.cjs additions lock the eslint rule's future fix-hint text (pointing at splitLines()) and its self-reference non-violation (the seam's own correct \r?\n split must never flag itself). Design: .gsd/phase/chore-3413-text-lines-seam/40-design.md Test matrix: .gsd/phase/chore-3413-text-lines-seam/50-test-matrix.md * chore(#3413): src/text-lines.cts owns line-terminator handling Phase 2 of epic #3212 (ADR-3212 §3/§6/§7). Adds splitLines/normalizeEol/ detectEol/joinLines and migrates frontmatter.cts onto it. parseMustHavesBlock (#3360, confirmed-bug) returned [] for every must_haves block on a CRLF plan file. Root cause: \r is its own LineTerminator in ECMAScript, so under /m two \s*-anchored indentation lookups could match at the position INSIDE a \r\n pair and absorb the terminator, inflating the captured indent by one character and tripping the "not nested under must_haves" guard. Two silent exits, one with a diagnostic and one without (a blank line before must_haves: hits the silent path). Fixed by converting both lookups from a whole-string /m match to split-then-scan — splitLines first, then a per-line, non-/m match — the same structural pattern parseYamlRegion (30 lines away in the same file) already used safely. Nothing downstream of the two lookups changed; blockLines is now sliced from the already-split array instead of re-splitting a substring, but its contents are unchanged for LF input, and the per-line dash/kv parsing loop is untouched. A parity property test (CRLF and LF plans parse to identical must_haves for every block name) matches a pattern this maintainer has required repeatedly for prior CRLF fixes in this file's neighborhood (Cortex: 7 recorded decisions, verify_intent -> held). frontmatter.cts's other .split(/\r?\n/) call sites (parseYamlRegion, isFrontmatterShaped, sliceTopLevelFrontmatterSegments, spliceFrontmatter) are rerouted onto splitLines — a literal 1:1 substitution, zero behavior change, since splitLines IS that same regex plus a type guard. The 4 scripts/normalizeLineEndings copies (gen-registry, gen-loop-host- contract, gen-capability-registry, gen-context-index) are deleted and rerouted onto normalizeEol, which strips a bare unpaired \r exactly like the deleted copies did (not just \r\n pairs) -- verified against each script's own --check mode against its real generated output. local/no-crlf-fragile-split widens from tests/ to src/**/*.cts, with its fix-hint message now naming splitLines() instead of the raw regex -- the prohibition finally has a primitive to point at. Detection logic unchanged in this phase (deliberate scope limit, see design doc Known limits: the rule doesn't yet recognize safeReadFile/platformReadSync as a content source, and has no detector for the \s-adjacent-to-anchor shape that is #3360's actual mechanism -- the CLASS is converged by the direct fix + regression test regardless). joinLines/detectEol are NOT wired into frontmatter.cts's own write path (cmdFrontmatterSet/Merge -> platformWriteSync) -- verified that platformWriteSync already, unconditionally converts CRLF->LF on every .md write today as a pre-existing policy owned by a different module, and ADR-3212's backward-compatibility clause rules out a file-format change in any phase. Stated explicitly in Known limits rather than left for a reader to discover. Six-gate ripple: .gitignore, eslint.config.mjs (src/**/*.cts block), docs/INVENTORY.md + INVENTORY-MANIFEST.json (regenerated), CONTEXT.md glossary (Text Lines Module, mirroring Phase 1's Pattern Module entry). Design: .gsd/phase/chore-3413-text-lines-seam/40-design.md Test matrix: .gsd/phase/chore-3413-text-lines-seam/50-test-matrix.md * fix(#3413): fix 13 pre-existing CRLF-fragile splits the widened rule found Widening local/no-crlf-fragile-split from tests/ to src/**/*.cts (the previous commit) immediately surfaced 13 real, pre-existing violations across 10 files -- undetected until now because the rule never scanned src/. This is the exact defect class ADR-3212 exists to close, playing out again one phase after Phase 1 hit the same shape ("the new lint rule -- once live -- found 27 more"). Per CLAUDE.md's no-defer rule, fixed inline rather than deferred or suppressed; there is no established suppression convention for this rule in src/ and inventing one now would undermine the point of widening it. audit.cts, broken-windows.cts, core-utils.cts, init.cts, milestone.cts, phase.cts (x3), profile-output.cts, roadmap.cts (x2): bare-\n splits or regex character classes widened to \r?\n / [^\r\n], each following the same pattern already established migrating frontmatter.cts. phase-estimation.cts: `\r?(?:\n|$)` restructured to `(?:\r?\n|\r?$)` -- already semantically CRLF-safe, but the rule's lexical scanner doesn't recognize \r? guarding a group (only \r? immediately before a literal \n). Verified the two forms are equivalent across all four EOL/EOF cases before restructuring, not assumed. roadmap-upgrade.cts needed two coupled sites, not the one flagged line: computeMigrationPlan and applyMigration must agree on line representation for the lines[edit.lineIndex] === edit.from equality check to hold, and the write-back needed joinLines + detectEol -- a plain lines.join('\n') was silently flattening a CRLF ROADMAP.md to LF wholesale on every migration. This is the first real production consumer of joinLines/detectEol in this epic (frontmatter.cts's own write path doesn't use them -- see the previous commit's Known limits). Fixing the 13 flagged sites surfaced 4 more adjacent same-shape sites the rule doesn't track (.search() and new RegExp(dynamicString) aren't in its tracked call/construction set). Investigated each empirically -- hand-tracing this exact bug class already produced one wrong conclusion earlier in this phase (a detectEol design-doc arithmetic error), so these were verified with real CRLF fixtures rather than reasoned about on paper: - audit.cts (scanTodos): REAL bug, fixed. `bodyMatch.trim().split ('\n')[0]` leaked a trailing \r into a user-visible todo summary on CRLF input -- .trim() only strips the string's outer edges, not a \r sitting mid-string before the first bare \n. Now splitLines(...) [0]. - phase.cts (cmdPhaseInsert, bullet-style branch): REAL bug, fixed. [^\n]* in targetBulletPattern swallowed a line's trailing \r on CRLF input, shifting the computed insert position to land INSIDE the \r\n pair; combined with a hardcoded '\n' bullet separator, a CRLF ROADMAP.md ended up with a mixed CRLF/LF result after an insert. Fixed with two coupled changes (either alone still corrupts, verified both ways): [^\r\n]* in the pattern, and the new bullet's leading terminator now comes from detectEol(rawContent). - roadmap.cts (cmdRoadmapAnnotateDependencies phase-boundary scan): investigated, genuinely safe, left untouched. The .search(/\n#{2,4} .../) boundary-finder and the [^\n]*-based heading match were empirically verified on a 3-phase CRLF fixture -- the only stray \r ends up at the tail of an intermediate phaseSection string that is only ever used for .test()-based idempotency checks, never for an exact-match comparison or written back to disk. No corruption on round-trip. Every fix re-verified: npm run build:lib clean, npx eslint 'src/**/*.cts' --no-cache reports 0 problems (was 13), and each fixed function's existing LF-input tests were spot-checked unchanged. * fix(#3413): apply orthogonal review findings Two isolated review engines (correctness + security) ran against the full diff and found three majors, one real security issue, and several disclosure-worthy minors. All fixed or explicitly disclosed with evidence; nothing deferred. MAJOR — detectEol's tie-break contradicted its own documented contract. Code returned '\n' on a 1:1 crlf/bare-LF tie; every doc (design doc, CONTEXT.md, the function's own comment) says ties resolve to '\r\n'. The existing test masked this by reusing the same tie fixture the buggy code happened to satisfy, rather than a genuine LF-majority case. Root cause: an Edit attempted earlier in this phase to fix this exact arithmetic error was blocked by the tier guard, and a later dispatch was incorrectly told it had already landed. Fixed: condition is now crlfCount >= bareLfCount; the test fixture corrected to a genuine 2:1 majority, with a new explicit tie-case test. MAJOR — phase.cts's cmdPhaseInsert built an EOL-aware bulletEntry via detectEol(rawContent), justified by a comment claiming a hardcoded '\n' corrupts a CRLF ROADMAP.md. False: this write goes through platformWriteSync, whose normalizeContent/_normalizeMd unconditionally converts CRLF->LF for any .md target — the templating was inert dead code, erased before the file is ever written. Reverted to hardcoded '\n', comment corrected to state the true reasoning. The separate [^\n]* -> [^\r\n]* widening one function up (a real splice-position fix, independent of final EOL) was kept. MAJOR — roadmap-upgrade.cts's stated rationale for switching onto splitLines/joinLines was wrong (both functions always agreed on line representation, before and after — the claimed equality-check risk never existed), and the change it justified introduced a real regression: forcing every line onto one dominant terminator silently rewrites untouched lines' EOL on a mixed-CRLF/LF ROADMAP.md. This write path uses raw fs.writeFileSync, not platformWriteSync, so unlike the phase.cts case above the regression is genuinely live. Fixing this took two attempts. The first attempt (revert to split('\n')/join('\n') plus a suppression comment) was correctly blocked by an agent that discovered local/no-crlf-fragile-split is a PROTECTED_RULES entry in tests/portability-rule-disable-ban.test.cjs — a hard, out-of-band, ADR-1703-governed guardrail banning any eslint-disable of this rule anywhere in src/**/*.cts. That agent also detected and correctly disregarded an injected instruction that appeared in tool output during a git operation, per this session's untrusted-content policy. The actual fix: computeMigrationPlan reverted to roadmapContent.split('\n') (confirmed lint-clean — the rule's data-flow tracking only follows a variable's initializer, and this one is declared empty then reassigned in a try block). applyMigration's write-back now splices edits against the ORIGINAL content string via indexOf('\n', pos) boundary-walking instead of a full split/rejoin, so every untouched character — including every line's own terminator — is copied byte-for-byte. A capture-group split (/(\r\n|\n)/, preserving terminators inline) was tried first and empirically confirmed to still trip the rule before this approach was chosen instead. MINOR (security) — roadmap.cts's cmdRoadmapAnnotateDependencies used the STRING form of String#replace, so $&, $`, $', $1-$9 inside must_haves.truths content (author-controlled) were interpreted as replacement directives, splicing unrelated ROADMAP.md text into the result. Fixed with the function-replacement form, which is never pattern-interpreted. Verified before/after with the reviewer's exact repro. Also disclosed rather than silently left: test matrix row 31 (four planned CRLF-materialized regression tests) was never implemented as separate files — corrected to record the actual verification (a manual --check run plus incidental existing coverage via each script's normalizeLineEndings: normalizeEol alias). parseMustHavesBlock's LF behavior was claimed byte-for-byte unchanged but the old yaml.indexOf(blockMatch[0]) substring search could match an unrelated earlier occurrence of the header text (e.g. inside a quoted value) — the split-then-scan fix incidentally also closes this, a strict improvement now recorded in the design doc rather than left implicit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3413): checkpoint 2 red — missing eslint ignore entry, RuleTester config error Checkpoint 2 came back red with 5 failures on the reviewed sha, both gaps genuinely undetectable by any local gate. eslint.config.mjs was missing the 'gsd-core/bin/lib/text-lines.cjs' ignores-list entry (ADR-457: generated .cjs artifacts are excluded from direct type-aware linting). Phase 1's sibling entry (pattern.cjs) sits two lines above it and was the exact precedent read while researching the six-gate ripple for this module -- missed anyway. Caught by tests/repo-invariants.test.cjs's bin/lib coverage-tracking test, which only runs on the remote suite. tests/no-crlf-fragile-split.rule.test.cjs's row-32 case specified both `messageId` and `message` on the same RuleTester error assertion -- ESLint's RuleTester rejects that combination outright. This existed since the test was first authored and was never caught locally: `npx eslint` only lints the file's syntax, it does not execute RuleTester, and local `node --test` is hard-blocked in this repo -- the assertion had never actually RUN before this checkpoint. It was even present in checkpoint 1's failure list, listed there as one of the "expected RED" tests; I matched it against my expected-failures list by test NAME only and never inspected the actual failure detail closely enough to notice it was failing for the wrong reason (a RuleTester config error, not the intended message-text mismatch). Fixed by keeping `message` (the exact-text assertion the test exists to make) and dropping `messageId`. Verified the crlfFragileSplit message string in eslint-rules/no-crlf-fragile-split.cjs matches this assertion character-for-character, and swept every other invalid case in the file for the same double-specification bug (none found -- all pre-existing cases use messageId alone). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3413): add Fixed changeset for the #3360 CRLF parsing fix The sole user-visible effect of this phase. No breaking-change label or Changed fragment needed — ADR-3212's Backward Compatibility section names the Node floor (Phase 1, already shipped) as the epic's only breaking change; Phase 2 has none. * chore(#3413): backfill changeset pr number to 3420 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-13 20:27:48 -04:00
Tom Boucher	dc3c81e93d	chore(#3212): src/pattern.cts is the sole owner of runtime-value regex construction — Phase 1 (#3416) ... * test(#3412): failing-first suite for the pattern-construction seam Phase 1 of epic #3212 (ADR-3212 §1/§2/§7). Tests only — src/pattern.cts and eslint-rules/no-adhoc-regex-escape.cjs do not exist yet, so both suites fail with MODULE_NOT_FOUND, which is the intended RED. Locks the measured behavior rather than the assumed behavior: RegExp.escape hex-escapes the leading character of nearly every string ("abc" -> "\x61bc"), so the suite asserts match-equivalence against an inlined historical oracle (the implementation being deleted) rather than byte-equivalence of pattern text — 200 seeded fast-check runs plus a fixed corpus, 0 mismatches. Also locks the latent character-class range bug this phase fixes as a side effect: a hyphen-bearing value interpolated into [...] currently forms a real range and matches an unintended character; post-migration it must not. * chore(#3412): src/pattern.cts owns runtime-value regex construction Phase 1 of epic #3212 (ADR-3212 §1/§2/§6/§7). Adds the pattern seam delegating to the built-in RegExp.escape, deletes every hand-rolled copy, and raises the Node floor to the Active LTS line. The census was low, three times over. ADR-3212 counted 10 copies; a graph query found 12; the new lint rule — once live — found 27 more. The difference is that the census counted named helper FUNCTIONS while the rule counts the escape SHAPE, so inline .replace(<class>, '\$&') copies were never in scope. ADR §1's actual requirement is that no module outside the seam escapes a value for regex use, so all of them are, and CLAUDE.md's no-defer rule makes them this change's work. Fourth consecutive epic here whose copy count was low — the argument for ADR-3180 Amendment 3's "state N found by the guard" rule. Also corrected mid-implementation: the survey reported phase-id.cts's escapeRegex had 0 external importers. It had 8 production importers, making its removal a public-surface change to an ADR-2121-owned module and requiring an update to that ADR's locked-surface test. Blast radius revised Medium-High -> High. RegExp.escape is match-equivalent but NOT text-equivalent: it hex-escapes the leading char of nearly every string ("abc" -> "\x61bc"). Equivalence is proven by a seeded fast-check property test against the deleted implementation as oracle. It also fixes a latent bug: a hyphen-bearing value interpolated into a character class previously formed a real range and matched an unintended character. Node floor 22 -> 24 (RegExp.escape is Node 24+), across engines, .nvmrc, package-lock, 9 CI matrix entries, and 5 docs. The aggregate `required-tests` context is unchanged and no job was added or removed, so branch protection cannot be orphaned by the dropped lanes. Enforced by eslint-rules/no-adhoc-regex-escape.cjs (shape-matched, with structural provenance for reviewed pattern-fragment constants rather than a name heuristic) plus a whole-tree companion guard covering the directories ESLint's globs miss. * fix(#3412): close the _SOURCE guard evasion, correct two false claims Three findings from the orthogonal review pass, all fixed. 1. The ESLint rule's `_SOURCE` provenance fallback was pure identifier- name matching with no binding check, so `new RegExp(userInput_SOURCE)` — a function parameter — sailed past the guard. That is the same rename-evasion class issue #3410 documents, reopened by the very fallback meant to complement the structural check. Now bound to the identifier's actual binding kind: import, require-derived const, or module-scope const; parameters, `let`/`var`, and unresolvable bindings fail closed. Four RuleTester cases cover the evasion and prove the legitimate cross-module case still passes. 2. src/pattern.cts's own header carried the stale pre-correction counts (12 copies / 17 call sites) while CONTEXT.md and the design doc carried the corrected ones (~39 / ~44) — a self-contradiction inside the PR whose entire purpose is deleting divergent copies. Rewritten, preserving the durable lesson: a named-function census cannot see inline copies; only a shape-matching guard can. 3. The claim that all deleted copies threw TypeError on non-string was false. phase-id.cts's copy — the one with 8 external importers — did String(value).replace(...) and never threw. The seam's locked signature does not coerce, so this is a real, now-disclosed behavior change rather than the pure preservation the tests asserted. Audited all 32 invocations across the 8 importers and 6 in-file callers: every one is safe by construction (upstream truthy guard or a string-producing derivation), verified by runtime probe against the compiled modules rather than by TS compilation, which cannot see a runtime undefined. Corrected the false claim in both the test comment and the design doc, and added it to Known limits. * docs(#3412): add Changed changeset for the Node 24 floor The only user-visible break in this phase. The escape-behavior change is internal and match-equivalent, so it carries no user-facing note. * fix(#3412): resolve the seam's require graph in script fixtures and packaging Checkpoint 2 came back red with 90 failures on the node24 lane. Three distinct defects, all introduced by routing scripts/ through the new pattern seam, none reproducible by any local gate: 1. ~82 failures — tests/adr-index-gate.test.cjs and tests/removed-but-needed-lint.test.cjs copy a scripts/*.cjs into an mkdtemp fixture and spawn it there (necessary: those scripts resolve their scan root from __dirname/.., so running the real script would scan the real repo). Each harness hand-listed the dependencies to copy alongside. Adding require('../gsd-core/bin/lib/pattern.cjs') to gen-adr-index.cjs made both lists silently incomplete -> MODULE_NOT_FOUND, plus 17 downstream 'did not emit parseable JSON' failures from the same crash. Fixed as a class, not an instance: new tests/helpers/copy-script- fixture.cjs walks a script's transitive static relative-require graph and copies it, so dependencies are derived and never re-declared. It throws (naming the unbuilt artifact) instead of letting the child die with a bare MODULE_NOT_FOUND. Verified for all four seam-consuming scripts: gen-adr-index, lint-removed-but-needed, gen-loop-host- contract, sync-runtime-launcher. 2. 2 failures — scripts/ ships wholesale but eslint-rules/ does not, so the new scripts/lint-no-adhoc-regex-escape.cjs would be MODULE_NOT_FOUND in a published install (#2858 guard). Excluded from the tarball, matching the existing precedent for gen-emitted- baseline.cjs, which is excluded for the identical reason, and locked with a test modeled on that one. Confirmed against a real npm pack: 890 files, 0 from eslint-rules/, and gsd-core/bin/lib/pattern.cjs present (so the other four scripts' requires are legitimate). 3. 6 failures — tests/phase-id.test.cjs asserted the literal escaped source text ('0*29', 'PROJ-42'). RegExp.escape is match-equivalent to the retired hand-rolled escaper but NOT text-equivalent: it hex- escapes the leading character and all hyphens ('0*\x329', '\x50ROJ\x2d42'). Verified NOT a behavior change — 576 match decisions across all three real interpolation prefixes, zero divergence. Those tests now compile each source into the same heading regex src/roadmap.cts's searchPhaseInContent builds and assert what matches and what does not, including the 'i'-flag canonicalization the hex escape has to preserve. Re-pinning the new literals would have rebuilt the same brittleness one layer down. Adds a test for the property the escape exists for: a dot in '1.2' must not act as a wildcard. Also shares one definition of 'a require' between the packaging guard and the fixture copier, so the two cannot disagree about what they scan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3412): refuse to copy a fixture dependency outside the fixture root copyScriptWithDeps resolved each relative require and joined the repo-relative result onto fixtureRoot. A require resolving OUTSIDE the repo yields a '../'-prefixed relative path, so path.join climbed out of the fixture and wrote into the surrounding temp dir (verified: repoRoot=/repo + depAbs=/etc/passwd wrote /tmp/etc/passwd). No script in the tree does this today, so this closes an available escape rather than an active one. Refuses via the existing unresolved- require path so the failure names the offending specifier. Covered by a negative proof that the guard fires and that nothing lands outside the fixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3412): parse requires instead of pattern-matching them; restore the foreign-prefix contract Applies all findings from the second orthogonal review round, re-run because real code changed after round 1. HIGH (security) — extractRequires stripped BLOCK comments before LINE comments, so a '//' comment containing '/*' opened a phantom block comment, and a '//' inside a string literal truncated the line. Both hid real requires: 'const u="http://x"; require("./real.cjs")' returned [], and four real requires in gsd-core/bin/gsd-tools.cjs were invisible. Replaced with a real AST parse via espree. This is ADR-3212's own Decision 4 — tokenizer-first for stateful grammars — applied to the case it describes; comment/string/regex nesting is exactly such a grammar, which is why the regex version was wrong. The function was moved byte-identical out of the #2858 packaging guard, so the bug PRE-DATES this branch and has been a live blind spot there: a shipped script could have required an unshipped path undetected. Fixing it makes that guard strictly stronger than on next. espree is promoted from a transitive eslint dependency to an explicit devDependency rather than relying on hoisting. The script parse attempt sets ecmaFeatures.globalReturn because Node wraps CommonJS bodies in a function, making a top-level return legal — scripts/check-coverage-gate .cjs relies on it, and without the flag the guard throws on a file it is supposed to scan. Verified 0 unparseable across all 324 .cjs/.js under scripts/, bin/, and gsd-core/bin/, and 0 new violations against a real npm pack, so the exact extractor does not newly fail the guard. MEDIUM (security) — the repo-containment check guarded dependencies but not the entry path. One escapesContainment predicate now guards both. LOW (security) — containment was lexical while fs follows symlinks, and a directory symlink could mint a fresh dedupe key per level. realpath now resolves both repoRoot and each dependency before the decision, and the realpath-derived path is the dedupe key. Destination layout still uses the original repo-relative path, so copied trees are unchanged. MAJOR (standards) — the round-1 behavioral rewrite of phase-id tests lost the foreign-prefix contract: every assertion was satisfied by an impl returning [A-Z]+\x2d42, i.e. ANY project code — the exact #3599 bug class the exact-source prevents. The literal assertions it replaced were catching this. Now asserts the compiled regex REJECTS a different prefix with the same number. MAJOR (standards) — the test hand-duplicated production's heading regex with no parity guard (CLAUDE.md's 'Generative Fix Divergence'). Removed the parallel surface instead of policing it: src/roadmap.cts exports buildPhaseHeadingRegex, searchPhaseInContent calls it, the test imports it. Byte-identical .source and .flags verified for both escaped forms. MINOR — '..foo' no longer false-flagged as an escape; the inverted spurious-vs-missing doc claim corrected; the dead allow-test-rule header removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3412): backfill changeset pr number to 3416 * fix(#3412): make the escape guard's own regex linear, reword an injection-scan collision Two CI failures on PR #3416, both in code this branch added. CodeQL js/redos (high) — REPLACE_CALL_RE's outer alternation let a bracket run be consumed EITHER by the character-class branch OR one character at a time by the trailing catch-all, so a failing match explored both parses of every pair. Measured on the real regex: n=26 -> 204ms, n=28 -> 791ms, n=30 -> 3475ms, a clean 2^n. This script scans repo source, so a file with a long bracket run after '.replace(/' would hang CI outright — a guard against undisciplined pattern construction was itself the worst pattern in the diff. Fixed the way ADR-3212 already prescribes: the catch-all branch now excludes '[' and ']' so a bracket can only be consumed by the class branch (this is what makes it linear), and every quantifier is bounded (the locked bounded-quantifiers decision) as a second line of defense. Now 0ms at n=2000. Disclosed coverage tradeoff, recorded at the constant: a regex literal with a BARE unescaped ']' outside a class is no longer matched by this backstop. No census shape has that form, and the AST rule remains the primary detector. Verified the guard did not go blind doing it: a real census-shape violation is still reported, and an allow-adhoc-regex-escape suppression comment is still honored. Regression test drives the exported findViolations on a 2000-repetition adversarial input and asserts the RESULT. It makes no wall-clock assertion — elapsed-time tests are forbidden — so a regression surfaces as a harness timeout, which is the correct signal. Prompt injection scan — 'must not act as a regex wildcard' in a test comment matched the scanner's jailbreak pattern act\s+as\s+(a|an|if| my). Reworded to 'behave as'. Deliberately NOT allowlisted: silencing a whole test file over one phrase would blunt the scanner permanently, and the comment has nothing to do with injection. Neither failure was reachable from the remote runner — CodeQL and the injection scan are not in that matrix, so the sha it passed was green and still wrong. 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-08-13 16:19:57 -04:00
Tom Boucher	bcf7b04864	chore(#2896): convert CONTEXT.md prose defect registry into enforced gates (#3325) ... * chore(#2896): convert CONTEXT.md prose defect registry into enforced gates Squashes the prior 4-commit sequence and fixes defects found while resuming this branch: 5 orphaned/corrupted DEFECT fragment lines left by an earlier botched edit, 17 "Source of truth: Memtrace `find_symbol`" placeholders that had destroyed real file-path citations, and 3 DEFECT.GENERATIVE-* entries merged into one RULESET.GENERATIVE-FIX predicate (policy, not an unenforced defect) to satisfy the zero DEFECT.<NAME>.<field>= acceptance criterion. Six mechanizable defects get real gates: DEFECT.UNBOUNDED-SUBPROCESS (eslint-rules/require-subprocess-timeout.cjs), DEFECT.CANARY-VERSION-LEAK (scripts/lint-canary-version-leak.cjs + version-gate.yml), DEFECT.CHANGESET-PR-FIELD-DRIFT (findPrFieldDrift in changeset/lint.cjs), DEFECT.FRONTMATTER-SCALAR-BROAD-GREP, DEFECT.REMOVED-BUT-NEEDED, and DEFECT.DEFAULT-FLIP-DOCUMENTATION (new lint scripts, wired into lint:ci). Already-enforced and unenforceable prose entries are deleted; the gate is the record. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#2896): route the new lint tests' subprocess calls through the bounded process-seam helper The 4 new test files for this PR's lint checks called cp.spawnSync/ execFileSync directly with no timeout, tripping this repo's own existing local/no-unbounded-spawn ESLint rule. Route every one through runNode/gitOrThrow (tests/helpers/process-seam.cjs, tests/helpers/git-fixture.cjs) instead, matching the pattern already used elsewhere in the suite (e.g. tests/changeset-lint.test.cjs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: register claude-orchestration.cjs and regenerate stale generated indexes Pre-existing drift on next, unrelated to this PR's own change, surfaced by running lint:ci as part of verifying #2896: two cli_modules (claude-orchestration.cjs, write-set.cjs) landed without a manifest regen, and CONTEXT.md's own edits in this PR staled its two generated indexes. Adds the missing docs/INVENTORY.md row for claude-orchestration.cjs (write-set.cjs already had one — only its manifest entry was stale) and regenerates docs/INVENTORY-MANIFEST.json, docs/CONTEXT-INDEX.json, and examples/dynamic-context-management/CONTEXT-INDEX.json. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2896): default-flip-documentation lint's local fallback base was main, not next Found in review: every other base-ref fallback in this repo (see scripts/changeset/lint.cjs's DEFAULT_BASE, #2988) defaults to `next`, the integration branch every PR actually targets — `main` is the release branch. This script's local fallback (used only when GITHUB_BASE_REF is unset, i.e. never in CI, but potentially on a local or direct invocation) diffed against the wrong ref. No test exercised the unset-env-var path, so it shipped unnoticed; every e2e test sets GITHUB_BASE_REF explicitly and is unaffected by this fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2896): stale eslint comment, overclaiming CONTEXT.md wording, and an incompletely-regenerated manifest Found by the isolated Standards code-review pass: - eslint.config.mjs's require-subprocess-timeout comment said "'warn' for now... flip to 'error' once migrated" while the rule already shipped as 'error' with all 8 sites migrated in the same commit — described a state that never existed. - The CONTEXT.md pointer block claimed the rule's bounded call sites "never throw", but roadmap-upgrade.cts's pre-mutation clean-tree check correctly still throws on failure (it gates a destructive real-run migration; degrading to "assume clean" would risk clobbering uncommitted work) — softened the claim to describe both shapes accurately instead of overclaiming one. - docs/INVENTORY-MANIFEST.json's claude-orchestration.cjs/write-set.cjs entries from the prior "fix: register claude-orchestration.cjs..." commit didn't actually land — re-running the generator now includes them; lint:generated-sync is green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#2896): backfill changeset pr field with the real PR number Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2896): normalize buildCorpus file paths to POSIX in lint-removed-but-needed Windows CI caught it: path.relative(root, abs) returns backslash- separated paths on Windows, but findSurvivingReferences's package-lock special case does file.startsWith('.github/workflows') — a forward- slash literal. On Windows the check silently never matched, so tests/removed-but-needed-lint.test.cjs's real-defect-shape fixture got exit 0 instead of the expected exit 1. Normalize at the production source (RULESET.CONTENT-PATH-NORMALIZATION) rather than the test side. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-08-10 12:55:52 -04:00
Tom Boucher	c28134ab39	fix(#3271): delete 25 duplicated folded test suites and fix three runner defects found doing it (#3285) ... * test(#3271): guard against a folded suite appearing twice in one host Adds local/no-duplicate-fold-marker, an AST rule that reports the second and every subsequent `folded:<name>` marker in a host file, plus RuleTester cases and a tree-wide regression assertion. Failing-first on purpose: the rule is registered at error and the 25 duplicated regions are still present, so eslint and the new tree-wide test are RED. The deletions land in the next commit. The marker key is the whitespace-delimited token after `folded:` — not the issue's `[a-z0-9-]*` slice, which truncates at `.` and false-positives on tests/model-resolver.test.cjs where feat-443-effort-fast-mode.integration and feat-443-effort-fast-mode are two distinct folded suites. Refs #3271 * fix(#3271): delete 25 duplicated folded suites from three install hosts Three consolidated install suites each carried a verbatim second copy of a contiguous run of #1969 B1 folded blocks. Byte-identical, constant offset, and green — each duplicated block registered and ran twice on every lane. tests/install.test.cjs 5981-9937 (3957 lines, 18 blocks) tests/install-minimal-hooks.test.cjs 2734-4015 (1282 lines, 5 blocks) tests/install-write-confinement.test.cjs 1754-2321 ( 568 lines, 2 blocks) Introduced by 6d072435d (#1975 re-applying #1970's hunks on a tree that already had them, 2026-07-03) — one stale-base re-application, three files, one commit. Verified by marker-count bisect: 1 at 4f779eda4 and 0cc7a1a42, 2 from 6d072435d onward. The later copy is deleted in each case, so every file returns to what its authoring batch produced and blame on the surviving lines stays accurate. local/no-duplicate-fold-marker, red on the previous commit, is now green. tests/model-resolver.test.cjs is untouched: the issue lists it, but its two blocks are folded from two different files and are not identical. It is a false positive of the issue's own grep, whose `[a-z0-9-]*` key truncates at `.`. Fixes #3271 * test(#3271): property-test marker identity and pin the alias non-goal Three review findings, all fixed inline: 1. foldMarkerOf is a parser and carried no fast-check property test. Raised independently by the /code-review standards axis and the isolated adversarial pass; the file already establishes the fc.property-driving-ruleTester idiom for a sibling rule. Added, two arms over markers generated from [a-z0-9-._]: the same marker twice always reports exactly once against firstLine 1, and two distinct markers never collide. The alphabet includes `.` on purpose — an implementation keyed on the issue's [a-z0-9-]* slice passes arm 1 and fails arm 2, which is exactly the model-resolver false positive. 2. meta.docs.category was the novel value 'Test hygiene'; all 16 sibling local rules use 'Best Practices', 'Portability' or 'Reliability'. Now 'Best Practices'. 3. A call through a further alias (const d = __foldDescribe) was unreported and undocumented — accidental rather than deliberate. It is now the fourth entry in the rule's documented non-goals, with the reason, and pinned by a valid RuleTester case so it cannot drift silently. Refs #3271 * test(#3271): name the step and elapsed time when a baseline build fails buildBaselineAtRef runs four bounded steps and, when one exceeded its bound, threw a bare "spawnSync ETIMEDOUT" naming neither the step nor how long anything took. Diagnosing one real failure took four separate experiments to recover information the throw already had. Each step is now timed, and any throw carries the breakdown: which step failed, its elapsed time, the timings of every step that completed before it, all three bounds, and the tail of the child's captured stdout/stderr. The failure message is deliberately the carrier. On the remote runner the captured output field comes back empty in failures.json while error and stack survive verbatim, so the message is the only channel that reaches a reader of a remote verdict. Refs #3271 * fix(#3271): size the baseline generator bound for the machine it runs on Instrumentation from a real remote-runner failure gave the breakdown: git-worktree-add=15.1s npm-run-build-lib=19.8s gen-emitted-baseline=FAILED@300.1s Steps 1 and 2 are comfortable. Only the generator exceeds its bound, and it is not hung — it needs more than 300s there. Measured ladder for that step: ~22s idle in a container, ~39s end-to-end in a clean container, ~142s with 8 CPU burners on 8 cores, and >300s under the real suite. Its cost is 19 sequential installer spawns, and spawn latency is exactly where a container degrades worst (3.9x slower than host, against 1.1x for file IO) — which is why a CPU-only load test did not reproduce it and why four earlier hypotheses (container slowness, network, shallow clone, CPU contention) all measured clean. The 300s bound was sized on an idle machine for a step that never runs on one. Under the remote runner the on-disk baseline cache is structurally absent — CI restores it via actions/cache keyed on github.event.pull_request.base.sha, a key that exists only inside GitHub Actions — so this slow path runs on every remote verification. The result: this gate has passed 0 times in 754 runs, failing 80 times and never once executing successfully. Raised to the 600000ms ceiling that local/no-unbounded-spawn treats as the largest meaningful bound; the other two bounds are untouched. This makes the gate RUN, which is the point: the alternative considered and rejected was degrading the timeout to a skip, and that was measured to turn the suite green with the gate silently not running at all. The real remedy is making the cache reachable from the remote runner so the in-job build returns to being the rare fallback ADR-2719 §5 describes. That is a gsd-test-runner change, not one this repo can make. Refs #3271 * fix(#3271): tolerate an overlay source that vanishes mid-walk Observed on the remote runner, three runs across three different branches: ENOENT: no such file or directory, link '/work/hooks/dist/gsd-config-reload.js' -> '/tmp/gsd-2930-overlay-6nOZay/hooks/dist/gsd-config-reload.js' buildOverlayRepo enumerates names with readdirSync and then acts on each one, so statSync, copyFileSync and linkSync all sit in a TOCTOU window. hooks/dist is regenerated by an ATOMIC REPLACE (scripts/build-hooks.js unlinks and renames), so any concurrently running test that rebuilds hooks retires a just-listed name mid-walk and the overlay dies on it. linkOrCopyFile already tolerated EXDEV and EPERM; ENOENT went straight through. On ENOENT the source is now re-examined ONCE rather than slept on. An atomic rename is a single syscall, so by the time the failure surfaces the successor is either already in place (the retry succeeds) or the path has genuinely left the tree, in which case there is nothing to mirror and the leaf is skipped. No sleep and no spin: a timing-based wait here would be the very flake being fixed. Every other errno still propagates untouched, so a real permission or IO fault stays a hard failure. Five tests hold the boundary: gone-for-good skips without retrying, mid-replace retries exactly once and places the file, EACCES still throws, a real linkSync ENOENT is injected by monkeypatching fs and restoring it in a finally (never a mode-bit trick, which root bypasses), and isMissingPath accepts only ENOENT. Refs #3271 * fix(#3271): order the timeout ladder inward-out and lock it Two review blockers, both real. The generator bound had been raised to 600000ms — exactly the whole-chunk timeout in scripts/run-tests.cjs:973. A step bound equal to the chunk ceiling loses the race: the chunk is killed first and the failure arrives as an opaque "no failed step" kill, so the per-step diagnostic added a commit earlier was built and then made unreachable in the same change. Separately the #2767 test declared a per-test timeout of 300000ms, BELOW the inner bound it was meant to permit, so it could still die at the exact 300s ceiling this was supposed to lift — via node:test's timeout rather than spawnSync's. Its sibling declared 900000ms, above the chunk ceiling, which is the same opaque-kill hazard from the other direction. The three bounds only produce a useful failure if they fire inward-out, so they now do: step 360s, per-test 480s, chunk 600s. 360s is ~3x the passing observation (91.6s / 115.8s) and 20% above the censored 300.1s timeout, while leaving 240s of chunk headroom for every other file sharing it. Four tests lock the ordering, including a drift guard on the exported values — without it, editing a call site's literal timeout would leave the ordering assertions passing while the real ladder inverted. Also from review: - err.gsdBaselineStep and err.gsdBaselineTimings were written and never read anywhere in the tree; only the rewritten message is consumed. Removed rather than kept as speculative surface. - buildOverlayRepo discarded placeVanishableLeaf's boolean at both call sites, so a vanished leaf left the overlay with no accounting at all. It now collects the skipped paths and warns once. Not thrown: a source that left the tree really is not part of the snapshot, and throwing would reintroduce the crash the tolerance removes — but silence would let a dropped leaf resurface later as an unrelated missing-file assertion. - The instrumentation commit shipped no test. One now drives a real failure and asserts the message names the step, its elapsed time, and the bounds. Refs #3271 * chore(#3271): backfill the changeset PR number * fix(#3271): bound a hook fan-out as its own class, not as a bare probe CI failure on PR #3285, job full test (windows-latest, 22, shard 2/3) — every other lane green, including windows-latest node 24 across all three shards: not ok 1 - blocks push when any to-be-pushed commit matches local blocked regex error: bash .githooks\pre-push failed — outcome=timed_out exitCode=null stderr= duration_ms: 15040.2168 A bound, not a hang: the test supplies stdin via input:, so the hook is not blocked reading its ref list, and the duration lands exactly on the 15000ms bound. The site used PROBE_TIMEOUT_MS, which tests/helpers/timeouts.cjs documents as "a single short CLI query or node -e probe against a temp fixture". This is not that. It spawns bash running .githooks/pre-push, and the hook then invokes a MOCK git that is itself a bash script, so one runHook is roughly four Git Bash spawns. On Windows each is Defender-scanned and the first hook test in a file pays cold start on top. That module's own docstring warns against precisely this: a call site that differs from its class must not be forced onto a shared value that does not describe it. HOOK_FANOUT_TIMEOUT_MS is that missing class — 60000ms, 4x the bound that failed and half INSTALL_TIMEOUT_MS, which is the right order: a hook fan-out is much lighter than a full installer run and far heavier than reading back a version string. Two tests lock the ordering against both neighbours, including one asserting real margin over the censored 15040ms observation, since a bound that merely matched what was measured would be the same defect again. Scoped deliberately: the other ~360 runHook sites keep their current bounds. This adds the norm and applies it where a real failure demonstrated the need, rather than sweeping a value across sites with no evidence for any of them. Refs #3271 --------- Co-authored-by: sim <sim@local>	2026-08-09 23:41:44 -04:00
Tom Boucher	0c413bbc9c	chore(#3059): close the ESLint glob-coverage escape and guard it (#3277) ... * chore(#3059): close the ESLint glob-coverage escape and guard it 62 tracked source files matched no `files:` glob in eslint.config.mjs, so ESLint skipped them entirely while `eslint .` still exited 0 — including all 26 files under hooks/, the enforcement machinery itself. Covers 56 of them (eslint-rules/, hooks/, bin/lib/, pi/, examples/, vscode/, the plugin shims, root *.mjs) and allowlists the 6 deliberate must-not-compile brand-typing fixtures with a recorded reason each. hooks/** is covered with n/no-process-exit deliberately off: a hook's whole contract is its exit code, several exits are load-bearing stdin-timeout guards where nothing else terminates the process, and ADR-0012/0174 scope the no-process-exit convention to the Command Routing Hub. bin/lib/ui-safety-gate.cjs is dual-mode, so it keeps the rule live and takes two targeted disables in its require.main===module tail instead. Adds scripts/lint-eslint-glob-coverage.cjs + a node:test drift guard so the class cannot regrow: allowlist entries require a non-empty reason, the list ratchets down only (a stale entry fails), and a tracked-count floor means a broken `git ls-files` fails rather than reporting clean. Closes #3059 * chore(#3059): apply review findings — correct the changeset count, add parser properties Isolated adversarial review, confirmed by rebuilding a byte-for-byte replica of the pre-change eslint.config.mjs: the changeset claimed 44 previously- unlinted files. The real figure is 56 (56 covered + 6 allowlisted = 62). That was user-facing CHANGELOG text and was wrong; corrected, along with three consequential figures in the design record. CLAUDE.md requires a fast-check property test for parsers and budget limits, and listTrackedSourceFiles is a parser. The standards review called this "satisfied in spirit"; it is not. Adds three properties driving the real exported parser through an injected execFile: extension totality/soundness including a trailing terminator, backslash-normalization totality, and CRLF/LF equivalence — the invariant the repo's recurring CRLF defect class breaks. Also de-duplicates the anchor rows onto one shared resolver, kept deliberately independent of the guard's own resolveFileCoverage so an anchor still fails if that resolution regresses, and records in the guard's header why the bin/install.js family is NOT allowlisted: it resolves to 2 rules under ADR-1703, so an entry would trip the allowlist_stale ratchet. * fix(#3059): make the coverage guard's git call container-safe The remote runner reported the guard degrading to `git_failed` on both Node lanes: fatal: detected dubious ownership in repository at '/work' The runner executes in a container where the repo is owned by a different UID, so git refuses to operate on it. The guard's degraded-verdict path worked exactly as designed — it reported the failure instead of throwing or falsely reporting clean — but a guard that cannot run in CI is not a gate. `git ls-files` is now invoked as `git -c safe.directory=* ls-files`. `-c` scopes the override to the single invocation and mutates no config file, and the wildcard is appropriate because this command only enumerates tracked paths in the repository it is already executing inside. Adds a regression test that captures the argv through the injected execFile seam and asserts `-c safe.directory=*` precedes `ls-files`, so the container case is pinned behaviorally rather than by reading the script's source. * chore(#3059): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3277 exists. --------- Co-authored-by: sim <sim@local>	2026-08-09 19:35:34 -04:00
Tom Boucher	9faacc0c15	test(#3148): bound the long tail and delete the unbounded-spawn allowlist (#3192) ... * test(#3148): bound the long tail and delete the allowlist Migrates the final 170 unbounded sync spawn sites across 49 files, then removes the allowlist entirely. local/no-unbounded-spawn now runs with no exemption surface across tests/**: there is no file to add a name to. drift-detection's throw-native git() helper routes to gitOrThrow -- bare runGit would have taken 16 call sites quiet on failure. commands.test.cjs has two independently-scoped runGsdTools/runCli helpers, one already bounded and one not; they are kept distinct rather than unified, the same trap as the two same-named git() helpers in Wave 1. runNpm's bound was erasable. Its options spread callerOptions after the defaults, so an explicit timeout:undefined silently dropped the 180000ms bound -- the rule flagged it and was right; it was not a false positive. Fixed by destructuring with a default, with a test that fails when the default is removed. Two sites stay on a raw spawn with an explicit timeout because the seam cannot express them: one needs shell:true for npm.cmd on Windows, one redirects stdout to a real fd. Both are the rule's own documented second option, not an escape from it. Closure verified rather than asserted: the derivation scan reports 0 unbounded spawn helpers and 0 unbounded direct git call sites, and a temporary file carrying an unbounded spawn still errors with the allowlist gone. Closes #3064. * test(#3148): close a hole in the guard's own eslint-disable ban The ban listed only the top level of tests/, so it was blind to 37 .cjs files under tests/helpers, qa, observability, fixtures and dispatch. With the allowlist deleted this test is the sole remaining way to detect someone silencing the rule inline, so the gap was load-bearing: a nested file could carry an unbounded spawn plus an eslint-disable and pass everything. Proven before and after. A probe planted under tests/helpers with both was invisible to the guard and clean under eslint; after making the listing recursive the guard fails on it. The scanned set goes from 771 files to 808. Pre-existing since the guard shipped, but this wave is what promoted it to sole defense, so it is fixed here rather than filed. Also converts the last hand-rolled throw check to throwIfFailed and the last re-derived legacy shape to compose toLegacyResult, which makes the epic's none-remain claim true rather than nearly true. toLegacyResult itself is not widened -- eight callers depend on its shape and one consumer does not justify changing a shared contract. * fix(#3148): correct seam incoherence at the bound and a slow review-lane error path Two real failures from the remote runner, both fixed at the cause. The seam could return outcome TIMED_OUT together with exitCode 0. At the exact bound spawnSync reports ETIMEDOUT while the child has already exited with a real status, and toSeamResult classified on the error code while passing status straight through -- an incoherent pair its own boundary test was written to catch, and did. A status that is not null is direct evidence the child exited on its own, so it now decides the outcome before the error-code branches run. process-seam.cjs was deliberately untouched by every earlier wave; this is a defect in the module itself, kept surgical, with a unit test that fails against the old logic. review-lane with an unknown subcommand fell through to its usage error only after loading the capability registry and building a per-lane plan, which spawns one child process per lane -- up to twelve. The error path took ~1288ms instead of ~119ms, and under bench load it outran a caller's spawn timeout and was killed before writing anything, which is the empty stdout and stderr CI saw. It now fails fast before any of that work begins. This is the epic's first production change. It is user-facing, so it carries a changeset rather than a no-changelog label. * test(#3148): replace a real-race timeout test with a deterministic one E9 raced git rev-parse against a 1ms bound and assumed git always lost. On a warm container git finishes first, spawnSync returns status 0 with no error at all, the seam correctly classifies EXITED, and gitOrThrow correctly does not throw -- so the test failed on both lanes. A probe confirms a genuine timeout always carries status null, so this was never the seam misbehaving. Raising the bound would only lengthen the odds, which is the same defect with better luck. The test now drives gitOrThrow against a stubbed runGit that returns a synthetic TIMED_OUT result, so it asserts exactly what it always meant to -- that a timeout propagates as a throw -- with no timing dependence. Five consecutive runs are identical where the old one varied. I wrote this test in Wave 0; it is a real-race test by construction and CLAUDE.md says to replace those rather than re-run them. * chore(#3148): backfill changeset PR number 3192 --------- Co-authored-by: sim <sim@local>	2026-08-07 21:03:50 -04:00
Tom Boucher	cbd180c5cd	test(#3147): bound the lint/changeset/docs cluster onto the process seam (#3181) ... * test(#3147): bound the lint/changeset/docs cluster onto the process seam Migrates 69 unbounded sync spawn sites across 24 files. Allowlist 73 to 49. Two shared helpers move: tests/helpers/graphify.cjs (6 importing suites) and tests/fixtures/index.cjs, whose three quoted-argument shell strings became single argv elements rather than whitespace splits. changeset-lint's throw-native git() helper routes to gitOrThrow; migrating it to bare runGit would have silently swallowed a failure that is loud today. ingest-docs goes the other way -- its catch never rethrew, it degraded failure into data every call site asserts on, so throwIfFailed would have thrown where the original returned. The design doc said otherwise and was corrected. tsconfig-noemit runs a real tsc --noEmit and takes a bespoke 180000ms per the ensure-runtime-build precedent, not the 30000ms build-hooks norm -- that norm is for a file copy, and sizing against a label rather than the work is the same error in the opposite direction. * test(#3147): add toLegacyResult and settle review findings The seam exposed a throwing adapter (throwIfFailed) but no non-throwing one, so eight files independently re-derived the same unwrap back to the legacy {status, stdout, stderr} shape. That is the third time this epic produced N copies of one mechanism -- seven throw wrappers in Wave 1, fifty-two timeout constants in Wave 2, eight result adapters here. The pattern is that whenever the seam does not expose a mechanism, every suite re-derives it. toLegacyResult now sits beside throwIfFailed, with its own tests. Two sites are deliberately NOT converted: changeset-cli's runRender and runRenderIn return {status, report, stderr} from parsed JSON and never a raw stdout, so they are a different shape family. lint-legacy-dir-name keeps its local GUARD_TIMEOUT_MS: 30000 matches the build norm numerically but bounds a lint probe, not hooks bundling, and importing it would encode a coincidence as a relationship. --------- Co-authored-by: sim <sim@local>	2026-08-07 15:45:51 -04:00
Tom Boucher	3fac6e629f	test(#3145): bound the installer/runtime cluster onto the process seam (#3176) ... * test(#3145): bound the installer/runtime cluster onto the process seam Migrates 156 unbounded sync spawn sites across 47 files. Allowlist 120 to 73. Timeouts are sized from evidence already in the tree rather than a house default, because this wave spawns installers rather than git plumbing and an undersized bound does not catch a hang -- it manufactures CI flake, which is worse, since a flake gets re-run instead of investigated. install.test.cjs records a real spawnSync ETIMEDOUT at a 60000ms cap on a loaded bench while another lane passed the same commit in 12.7s, so full installs are bound at 120000ms against that recorded incident. Also adds an auditable escape to the guard's timeout ceiling. The 600000ms cap was set in #3143 from partial evidence, but fragment-single-edit- propagation carries a documented, load-tested 900000ms bound on a run that chains a full build plus eight generators -- the guard would have rejected a correct timeout the moment that file left the allowlist. A value above the ceiling is now permitted only with an inline allow-spawn-timeout-ceiling marker carrying a non-empty reason. It raises the ceiling; it never waives the requirement for a bound, which is asserted directly. install-shared.cjs keeps its hand-rolled assert rather than routing through throwIfFailed: its message embeds both streams, and throwIfFailed carries only a trimmed stderr. The message now also names the outcome, so a bounded timeout reads as such across its 38 importers instead of as expected null to equal 0. * test(#3145): extract class-norm timeouts and correct the build-hooks sizing A pre-PR review found 52 copies of four class-norm timeout constants across this wave. These are not per-suite fixture bindings -- they are shared facts about how long a class of subprocess takes, derived from a recorded bench incident. That norm already moved once (60000 to 120000 after a real ETIMEDOUT), and 52 copies would have drifted the next time it moved. Extracts tests/helpers/timeouts.cjs, where each norm is justified once, and converts the copies. A site that genuinely differs -- a real tsc compile, or regen:derived -- keeps its own local constant with its own justification. Also corrects a misclassification: scripts/build-hooks.js was sized as a build at 120000 in twelve places and 60000 in another, but it compiles and bundles nothing. Its own header says no bundling needed; it copies pre-built files and syntax-checks them with vm. Three different values bounded one script; now there is one. * test(#3145): fix red CI — lint self-match and a Windows chunk overrun Two failures on PR 3176. lint-allow-test-rule-refs read a RuleTester fixture as a real exemption. The fixture exists to prove an unrelated marker does NOT suppress the rule, so it carries that marker's literal text as test data. Split via concatenation, the same idiom no-unbounded-spawn-allowlist.test.cjs already uses for its own self-match problem. The explanatory comment needed the same treatment. The Windows shard 3/3 chunk was killed at its 600000ms budget. Output stopped seven minutes before the kill, so this was an overrun rather than a slow chunk: regenDerivedPropagatesSingleFragmentEditWithNoSecondSourceSurface runs regen:derived bounded at 900000ms, which is larger than the whole chunk budget, so the chunk killer always fires first and it can never complete there. Both the test and that bound predate this change; modifying the file pulled it into the Windows targeted set and exposed it. Skipped on Windows with the reason recorded; the Linux lanes cover it. The 900000 bound and its ceiling marker are unchanged -- they are correct. * test(#3145): refresh the stale test-timings cost table The Windows shard was killed at its 600000ms per-chunk budget. run-tests.cjs packs chunks by measured duration from tests/test-timings.json, and an unknown file falls back to the table's median weight -- advisory by design, but it silently underweights exactly the files that matter. Four of the failing chunk's 22 files were absent from the table, including the two heaviest: fragment-single-edit-propagation.install.test.cjs at 230s (it runs regen:derived) and agent-fragments-emission.install.test.cjs at 79s. Both were weighted as average, so the chunk's total weight read 53.68 against a budget of 60 and the packer produced a single chunk. Regenerated from a passing full-suite run, per the remedy the script itself documents. 700 to 770 entries, 70 added, 0 dropped -- verified, since gen-test-timings.cjs replaces the table wholesale rather than merging. Proven against the real packer: the same 22 files now weigh 103.91 and split into two chunks. No logic, budget, or timeout was changed; raising a budget to make a red gate pass is not a fix. --------- Co-authored-by: sim <sim@local>	2026-08-07 15:18:18 -04:00
Tom Boucher	1d208e5af6	test(#3144): bound the git/worktree cluster onto the process seam (#3152) ... * test(#3144): bound the git/worktree cluster onto the process seam Migrates 180 unbounded sync spawn sites across 19 files. Every previously unbounded call now carries an explicit timeout with a comment giving the number and why. The migration is not a callee swap. execSync and execFileSync throw on a non-zero exit and the seam never does, so each site was classified first: sites that rely on the throw route to gitOrThrow, and sites that already read .status to detect an EXPECTED non-zero -- an intended cherry-pick conflict, a rev-parse outside a repo driving a skip -- route to the never-throwing runGit instead, which would otherwise throw on exactly the exit being probed for. Two same-named git() helpers in worktree-cleanup.test.cjs have different return contracts, one trimmed and one raw; both are preserved rather than unified. Collapses five hand-rolled throw wrappers onto one throwIfFailed in git-fixture.cjs, which gitOrThrow now also uses so the shape cannot drift. Allowlist drops 139 to 120; BASELINE lowered to match. * test(#3144): fix pre-PR review findings Documents throwIfFailed in the CONTEXT.md glossary and CONTRIBUTING.md -- it became the shared throw mechanism without either doc naming it. Routes the sixth and seventh hand-rolled copies of the throw shape through throwIfFailed (worktree-baseref-install, worktree-safety-reap); the first consolidation missed both. Converts ci-rebase-check's 8 fixture-setup calls from unchecked runGit to gitOrThrow so a failed setup step aborts where it fails rather than surfacing later as a confusing failure against the wrong subject. Adds 12 direct unit tests for throwIfFailed, which until now was only exercised transitively. Splits verify.test.cjs's non-git grep/sed bound off GIT_TIMEOUT_MS. --------- Co-authored-by: sim <sim@local>	2026-08-07 10:58:34 -04:00
Tom Boucher	2afe17bbdb	test(#3143): add the no-unbounded-spawn guard and throw-preserving git fixture (#3150) ... * test(#3143): add no-unbounded-spawn guard and throw-preserving git fixture Adds the ESLint rule local/no-unbounded-spawn, wired into the tests/**/*.cjs block, plus an allowlist that only ratchets down: a listed file with zero violations reports its own entry as stale. The rule resolves renamed destructures and chained requires rather than matching literal callee names -- both forms exist in the suite today and a name-only matcher leaves them permanently invisible. It resolves an options object held in a single-write const, which is what keeps process-seam.cjs, the bounded reference implementation, from flagging itself. timeout: 0 and anything above the 600000ms ceiling are rejected as only nominally bounded. Adds tests/helpers/git-fixture.cjs so a migrated execSync call site keeps its throw-on-non-zero contract; process-seam.cjs is unchanged. * test(#3143): prove the allowlist guards can actually fail Extracts the D4/D6/D7/D8 checks into pure helpers and drives each against a synthetic fixture carrying an injected violation. Without this the suite only proved that today's clean data passes, which a deleted check would also satisfy. * fix(#3143): close two ceiling and alias escapes found in review Nested arithmetic bypassed the ceiling entirely: the numeric evaluator only resolved a flat literal, so `timeout: 60 * 60 * 1000` (3600000ms, six times the ceiling) fell through to trusted and reported nothing. The evaluator now recurses through arithmetic and unary signs with a depth cap. Alias resolution was traversal-order dependent, not scope dependent: a call textually above its own require destructure saw an empty alias map and reported clean. The map is now built in a Program pre-pass. Also: an explicit timeoutMs:undefined no longer overwrites the git fixture default via spread, adds the missing seam-routed rule test, and de-duplicates the repeated try/catch in the fixture tests. --------- Co-authored-by: sim <sim@local>	2026-08-07 09:43:36 -04:00
Tom Boucher	4bd6fb066b	chore(#2880): close ADR-2143 deployment misses — table-regex fingerprint + state-document seam migration (#2889) ... * chore(#2880): close ADR-2143 seam misses — widen table-regex fingerprint, migrate state-document onto the seam The no-adhoc-markdown-parsing rule matched only a negated class whose sole member was a pipe ([^|]), so the stricter and more common [^|\n] spelling evaded it entirely -- src/state-document.cts hand-rolled exactly that shape and linted clean. Widen the fingerprint to any negated class excluding a pipe, which is the ADR-2143 section 7 prohibition as written. With the rule fixed, state-document.cts goes red. Replace tableRowPattern with locateFieldRow: a line scan using the markdown-table seam's splitTableRow for cell semantics, returning the value cell's byte range, and splice that range instead of running a whole-document content.replace. An edit now physically cannot cross a row boundary (section 4). Behavior is frozen -- stateReplaceField has 79 dependents across 5 command processes. Characterization tests lock all 14 table-branch rows plus CRLF, extract round-trip and the withFallback caller shape; a fast-check property asserts every non-target line stays byte-identical. Refs #2880, epic #2143 * fix(#2880): address adversarial review — lone-CR rows, field-name padding, quadratic scan, over-broad fingerprint Isolated adversarial review found four defects in the first commit. 1. locateFieldRow split lines on \n only. JS treats a lone \r as a line terminator, so the regex it replaced matched rows separated by bare CR. "| Phase | 3 |\r| Other | 9 |" returned 3 before and null after. Now CR, LF and CRLF are all terminators, byte offsets unchanged. 2. The field name was normalised with trim().toLowerCase(). The old regex embedded it verbatim, so its whitespace had to be absorbed by the row's own padding -- and because the group is a literal-character match rather than a whitespace class, a tab-padded cell does not accept a space-padded name. Replaced with an offset-aligned search reproducing the original backtracking exactly. 3. The widened fingerprint regex had two unbounded [^\]]* around an optional and ran quadratically over every regex source in every linted file: 256000 chars took 23 seconds. Replaced with a single-pass scanner that never rescans; the same input is now ~1ms. 4. The fingerprint also matched non-table idioms such as [^\s|] and [^"|]. Narrowed to a class excluding the pipe plus only \n, \r or \t. Differential fuzz against origin/next: 20000 cases, 0 mismatches. Refs #2880, epic #2143 * test(#2880): drop wall-clock assertion from the ReDoS regression guard local/no-elapsed-assertion flagged the elapsed-time check, and CLAUDE.md bans timing assertions outright as flaky. The 256000-char input stays as the regression guard for the quadratic scan; correctness of the verdict is what is asserted. If the quadratic path returns, the test stops completing and surfaces as a suite timeout rather than a silent pass. Also adds the changeset fragment for #2880. Refs #2880 * fix(#2880): spec-correct case folding, property tests, naming Code-review findings. The field-name comparison used toLowerCase(). The regex it replaced used /i WITHOUT /u, and ECMAScript Canonicalize deliberately does not fold a non-ASCII character onto an ASCII one -- KELVIN SIGN U+212A matched ASCII K where the old code returned null. Replaced with spec-correct Canonicalize, including the multi-character uppercase case (eszett -> SS), which a naive uppercase comparison also gets wrong. Added the fast-check property tests CLAUDE.md requires for parsers: one for the negated-class scanner, one for the field-name fold semantics, each against an independent reference implementation. Both reference impls failed on first run against real bugs, so neither property is vacuous. Renamed p2/p3 to name the exactly-three-pipes invariant, and reduced a duplicated comment to a cross-reference. Differential fuzz vs origin/next: 20000 runs, 0 mismatches, with the harness proven to discriminate the KELVIN case. Refs #2880 * chore(#2880): backfill changeset PR number (#2889) * docs(#2890): correct the local ESLint plugin path in CONTEXT.md CONTEXT.md named the local AST-rule plugin directory as scripts/eslint-rules/, which does not exist. The real location is eslint-rules/ at the repo root -- what eslint.config.mjs actually imports -- and CONTEXT.md's own later entry already says so explicitly, so the file disagreed with itself. Found by a line-by-line audit of all 1036 lines against the live graph; this was the only confirmed inaccuracy. Closes #2890 --------- Co-authored-by: Test <test@example.com>	2026-07-30 19:55:03 -04:00
Tom Boucher	7cbfb2819f	fix(#2764): extend no-path-literal-in-assert to membership/substring checks (catches the #2728 Windows CI shape) (#2879) ... * test(#2764): no-path-literal-in-assert must flag membership/substring checks over path-returners The equality-only visitor missed .includes/.indexOf/.startsWith/.endsWith over a path-returning receiver (directly or via a .map() hop) — these pass lint and fail on Windows. Add RuleTester invalid cases for each shape + valid cases for the suppressions (POSIX-normalized receiver, non-path receiver, no-slash arg). * fix(#2764): extend no-path-literal-in-assert to membership/substring checks over path-returners Add a third CallExpression shape: .includes/.indexOf/.startsWith/.endsWith/.match over a receiver that traces to a path-returning call (directly, or through ONE .map(f => path-…) hop — the #2728 shape that passed lint and failed Windows CI). Reuse isPathReturningCall/isPosixSlashStringLiteral/isPosixNormalizerCall; respect the normalizer + Windows-excluded suppressions. The rule is scoped to tests/**/*.test.cjs. * chore(#2764): changeset fragment * test+docs(#2764): add .match test coverage, Windows-excluded membership case, fix doc accuracy (review findings) - MAJOR: .match was supported but untested — add invalid (.match with slash) + valid (.match no-slash) RuleTester cases. - MINOR: Windows-excluded membership case was claimed by the matrix but absent — add a platform-guard valid case in membership form. - MINOR: .map() hop comment said 'ONE' but recursion allows nesting — reword to 'recursively'. - Add known-boundary (d) note for the membership/.match shape. - Fix a premature comment-close from a literal */ in the glob example. * chore(#2764): backfill changeset PR number (2879) --------- Co-authored-by: Test <test@example.com>	2026-07-30 15:26:43 -04:00