From dc3c81e93dbd9e03185841b22d0b4c320c055a80 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 13 Aug 2026 16:19:57 -0400 Subject: [PATCH] =?UTF-8?q?chore(#3212):=20src/pattern.cts=20is=20the=20so?= =?UTF-8?q?le=20owner=20of=20runtime-value=20regex=20construction=20?= =?UTF-8?q?=E2=80=94=20Phase=201=20(#3416)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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(, '\$&') 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 * 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 * 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 * 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 --------- Co-authored-by: sim Co-authored-by: Claude Opus 5 --- .changeset/calm-lemurs-sing.md | 5 + .github/ISSUE_TEMPLATE/chore.yml | 2 +- .github/workflows/install-smoke.yml | 19 +- .github/workflows/test.yml | 41 +- .gitignore | 1 + .nvmrc | 2 +- CONTEXT.md | 3 + CONTRIBUTING.md | 9 +- bin/install.js | 5 +- docs/INVENTORY-MANIFEST.json | 1 + docs/INVENTORY.md | 1 + docs/TESTING-SUITES.md | 36 +- docs/branch-protection.md | 5 +- docs/contributing/bootstrap.md | 4 +- eslint-rules/no-adhoc-regex-escape.cjs | 442 ++++++++++++++++++ eslint.config.mjs | 23 + package-lock.json | 3 +- package.json | 6 +- scripts/ci-test-scope.cjs | 4 +- scripts/gen-adr-index.cjs | 20 +- scripts/gen-loop-host-contract.cjs | 8 +- scripts/lint-completion-predicate-drift.cjs | 3 +- scripts/lint-no-adhoc-regex-escape.cjs | 236 ++++++++++ scripts/lint-removed-but-needed.cjs | 5 +- scripts/run-tests.cjs | 9 +- scripts/sync-runtime-launcher.cjs | 6 +- src/api-coverage.cts | 5 +- src/assumption-delta.cts | 5 +- src/gap-checker.cts | 4 +- src/init.cts | 3 +- src/markdown-sectionizer.cts | 4 +- src/milestone.cts | 3 +- src/pattern.cts | 50 ++ src/phase-id.cts | 17 +- src/phase.cts | 2 +- src/roadmap-parser.cts | 5 +- src/roadmap.cts | 23 +- src/runtime-artifact-conversion.cts | 5 +- src/shell-command-projection.cts | 3 +- src/state-document.cts | 6 +- src/state-transition.cts | 4 +- src/state.cts | 3 +- tests/adr-index-gate.test.cjs | 15 +- tests/api-coverage-gate-e2e.test.cjs | 3 +- tests/capability-lifecycle.test.cjs | 3 +- tests/check-env.test.cjs | 3 +- tests/code-review-agent-skills.test.cjs | 6 +- tests/code-review.test.cjs | 3 +- tests/codex-config.test.cjs | 15 +- tests/config-schema.property.test.cjs | 3 +- tests/cursor-hook-workspace-roots.test.cjs | 3 +- tests/docs-parity-live-registry.test.cjs | 9 +- tests/emitted-attribution.test.cjs | 7 +- tests/eslint-no-adhoc-regex-escape.test.cjs | 306 ++++++++++++ tests/helpers/copy-script-fixture.cjs | 265 +++++++++++ tests/helpers/install-shared.cjs | 4 +- tests/install-runtime-artifacts.test.cjs | 3 +- tests/no-unbounded-spawn-allowlist.test.cjs | 2 +- ...pped-scripts-require-only-shipped.test.cjs | 32 +- tests/pattern.test.cjs | 204 ++++++++ tests/phase-id-drift-guard.test.cjs | 9 +- tests/phase-id.test.cjs | 152 +++--- tests/phase.test.cjs | 3 +- tests/phase6-capstone-conformance.test.cjs | 5 +- tests/product-name-purity.test.cjs | 3 +- tests/removed-but-needed-lint.test.cjs | 9 +- tests/reversibility-tagging.test.cjs | 3 +- tests/runtime-launcher-parity.test.cjs | 6 +- tests/workstream-name-policy.test.cjs | 3 +- tests/worktree-safety.test.cjs | 3 +- tsconfig.build.json | 2 +- 71 files changed, 1839 insertions(+), 286 deletions(-) create mode 100644 .changeset/calm-lemurs-sing.md create mode 100644 eslint-rules/no-adhoc-regex-escape.cjs create mode 100644 scripts/lint-no-adhoc-regex-escape.cjs create mode 100644 src/pattern.cts create mode 100644 tests/eslint-no-adhoc-regex-escape.test.cjs create mode 100644 tests/helpers/copy-script-fixture.cjs create mode 100644 tests/pattern.test.cjs diff --git a/.changeset/calm-lemurs-sing.md b/.changeset/calm-lemurs-sing.md new file mode 100644 index 000000000..a38c2d1c7 --- /dev/null +++ b/.changeset/calm-lemurs-sing.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3416 +--- +**GSD now requires Node 24 or newer** — the `engines.node` floor moves from 22 to 24, and the Node 22 test lane is retired. Node 22 entered Maintenance LTS and this project tracks the Active LTS line; the change is what lets regex escaping delegate to the built-in `RegExp.escape` instead of a hand-rolled implementation. If you are on Node 22, upgrade before updating GSD. diff --git a/.github/ISSUE_TEMPLATE/chore.yml b/.github/ISSUE_TEMPLATE/chore.yml index 1bb45cfc4..ce3ecf642 100644 --- a/.github/ISSUE_TEMPLATE/chore.yml +++ b/.github/ISSUE_TEMPLATE/chore.yml @@ -89,7 +89,7 @@ body: placeholder: | - [ ] All test files use `node:assert/strict` - [ ] Zero `try/finally` cleanup blocks in test lifecycle code - - [ ] CI green on all matrix entries (Node 22/24, Ubuntu/macOS/Windows) + - [ ] CI green on all matrix entries (Node 24, Ubuntu/macOS/Windows) - [ ] No change to user-facing behavior validations: required: true diff --git a/.github/workflows/install-smoke.yml b/.github/workflows/install-smoke.yml index 0b62bd48c..a487873ef 100644 --- a/.github/workflows/install-smoke.yml +++ b/.github/workflows/install-smoke.yml @@ -9,7 +9,7 @@ name: Install Smoke # installing from an unpacked local directory, so any stale tsc output lacking # execute bits will be caught by the unpacked job before release. # -# - PRs: path-filtered, minimal runner (ubuntu + Node LTS) for fast signal. +# - PRs: path-filtered, minimal runner (ubuntu + Node 24) for fast signal. # - Push to release branches / main: full matrix. # - workflow_call: invoked from release.yml as a pre-publish gate. @@ -65,16 +65,15 @@ jobs: strategy: fail-fast: false matrix: - # PRs run the minimal path (ubuntu + LTS). Pushes / release branches - # and workflow_call add macOS + Node 24 coverage. + # PRs run the minimal path (ubuntu + Node 24, the engines.node floor). + # Pushes / release branches and workflow_call add macOS coverage. + # (Node floor is 24 everywhere now, so the prior "ubuntu + Node 24, + # full_only" row is identical to this minimal row and was dropped — + # it would otherwise re-run the same os/node combo twice on push.) include: - - os: ubuntu-latest - node-version: 22 - full_only: false - shell: bash - os: ubuntu-latest node-version: 24 - full_only: true + full_only: false shell: bash - os: macos-latest node-version: 24 @@ -218,10 +217,10 @@ jobs: if: github.event_name == 'pull_request' run: node scripts/ci-rebase-check.cjs - - name: Set up Node.js 22 + - name: Set up Node.js 24 uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6.3.0 with: - node-version: 22 + node-version: 24 cache: 'npm' - name: Install root deps diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ccaa5a59e..dbbf3ac33 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -158,10 +158,13 @@ jobs: fail-fast: false matrix: include: - # Required default PR lanes: fast signal, minimum runtime, primary runtime, - # and one Windows shell/path lane. The non-primary OS/runtime lanes run - # scoped tests from scripts/ci-test-scope.cjs; Ubuntu/Node 24 runs the - # broader default suite. + # Required default PR lanes: fast signal (targeted scope), primary + # runtime (full scope, sharded), and one Windows shell/path lane. The + # Node floor is 24 (engines.node >=24.0.0), so the fast-signal lane + # runs the same Node version as the full-scope lanes; only the test + # selection (scope: targeted vs full) differs. The non-primary + # OS/runtime lanes run scoped tests from scripts/ci-test-scope.cjs; + # Ubuntu/Node 24 runs the broader default suite. # # #2952: the `scope: full` lane is SHARDED three ways. It was the only # unsharded lane in this file, and the whole unit suite under c8 on one @@ -180,7 +183,7 @@ jobs: # install-heavy suites. Its shards use the same `--shard i/n` # flag on scripts/run-tests.cjs, applied AFTER scope selection. - os: ubuntu-latest - node-version: 22 + node-version: 24 scope: targeted - os: ubuntu-latest node-version: 24 @@ -364,10 +367,10 @@ jobs: # CI stays green. base.sha is fixed for the life of the run. CI_REBASE_BASE_SHA: ${{ github.event.pull_request.base.sha }} run: node scripts/ci-rebase-check.cjs - - name: Set up Node.js 22 + - name: Set up Node.js 24 uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6.3.0 with: - node-version: 22 + node-version: 24 cache: 'npm' - name: Environment check run: npm run check:env @@ -401,8 +404,10 @@ jobs: # cliff. The cap stays at 20m as a generous backstop; a healthy shard now # finishes in roughly a third of the old single-lane wall-clock. # - # The matrix is the cross-product of 3 OS/node legs × 3 shards = 9 jobs, - # enumerated explicitly as `include:` rows. (A base `shard: [1,2,3]` + # The matrix is the cross-product of 2 OS/node legs × 3 shards = 6 jobs + # (windows-latest/24, macos-latest/24 — the Node floor is 24, so there is + # no separate node-22 leg to cross-product against), enumerated explicitly + # as `include:` rows. (A base `shard: [1,2,3]` # dimension would NOT cross-product against `include` legs — include rows # sharing no key with the base matrix are appended as standalone combos — # and a NESTED `leg.os` key is not resolvable by the H1 shell-policy linter @@ -433,29 +438,17 @@ jobs: matrix: include: - os: windows-latest - node-version: 22 + node-version: 24 shell: pwsh shard: 1 - os: windows-latest - node-version: 22 + node-version: 24 shell: pwsh shard: 2 - os: windows-latest - node-version: 22 + node-version: 24 shell: pwsh shard: 3 - - os: macos-latest - node-version: 22 - shell: 'zsh {0}' - shard: 1 - - os: macos-latest - node-version: 22 - shell: 'zsh {0}' - shard: 2 - - os: macos-latest - node-version: 22 - shell: 'zsh {0}' - shard: 3 - os: macos-latest node-version: 24 shell: 'zsh {0}' diff --git a/.gitignore b/.gitignore index 8770a8ed8..cb90eb946 100644 --- a/.gitignore +++ b/.gitignore @@ -196,6 +196,7 @@ build/ /gsd-core/bin/lib/planning-workspace.cjs /gsd-core/bin/lib/planning-scope.cjs /gsd-core/bin/lib/planning-snapshot.cjs +/gsd-core/bin/lib/pattern.cjs /gsd-core/bin/lib/health-diagnostic-types.cjs /gsd-core/bin/lib/health-diagnostic.cjs /gsd-core/bin/lib/health-diagnostic-rules/root-existence.cjs diff --git a/.nvmrc b/.nvmrc index 2bd5a0a98..a45fd52cc 100644 --- a/.nvmrc +++ b/.nvmrc @@ -1 +1 @@ -22 +24 diff --git a/CONTEXT.md b/CONTEXT.md index fa98510f7..d5ecd0d49 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -103,6 +103,9 @@ Module owning legacy-key normalization, defaults merge, and explicit on-disk mig ### Planning Scope Module Leaf module owning the frozen `SCOPE` discriminator (`COMPLETE` / `TRUNCATED` / `UNSCOPED` / `UNREADABLE`) that every consolidated `.planning/` semantic derivation returns alongside its payload, per ADR-3180 Decision 2. It exists to make one distinction representable: `COMPLETE` with zero items is a REAL answer (a phase genuinely has no plans; a milestone genuinely has no phases yet), while the other three with zero items are NON-answers — the derivation could not see all of its input. Before it, those two cases were output-identical, which is the failure class epic #3180 removes: a truncated milestone window returned `phase_count: 0` with no error, indistinguishable from a freshly-declared milestone. It is a frozen enum rather than a message string because `CONTRIBUTING.md` bans raw-text matching on outputs and requires a typed IR, so callers branch on `result.scope === SCOPE.TRUNCATED`. Pure and import-free — the bottom of the dependency graph, so any consumer can depend on it without a cycle (mirrors the Phase Id Module's leaf position). Source of truth: `gsd-core/bin/lib/planning-scope.cjs` (generated from `src/planning-scope.cts`). The contract is PROVISIONAL: #3183 is its first real implementation, and ADR-3180 requires the ADR be amended before Phase 2 rather than the contract worked around, if it does not fit. +### Pattern Module +Leaf module owning the construction of a regex from a **runtime value**, per ADR-3212 §1 (epic #3212 Phase 1, #3412). Exposes `escapeRegex(value) → string` and `literalPattern(value, flags?) → RegExp`. `escapeRegex` **delegates to the built-in `RegExp.escape`** (TC39 Stage 4, ES2026, Node 24+) — it is deliberately NOT an implementation, which is the whole point: the ~39 hand-rolled copies it replaces existed because every author re-derived the metacharacter set, and TC39 standardized the primitive precisely because userland versions "miss edge cases." `escapeRegex` is the primary export (the large majority of call sites build a regex *source string* and interpolate it into a larger pattern); `literalPattern` is the minority convenience for the `new RegExp(escapeRegex(v))` shape. Pure and import-free — a leaf, so any consumer can depend on it without a cycle (mirrors the Planning Scope and Phase Id modules' position). **Behavioral note, measured not assumed:** `RegExp.escape` is a *superset* escaper and produces different pattern SOURCE TEXT than the hand-rolled class did — it hex-escapes the leading character of nearly every string (`"abc"` → `"\x61bc"`), plus `-`, space, `/`, and control chars. It is **match-equivalent** (verified by a seeded `fast-check` property test against the deleted implementation as oracle, plus a fixed corpus), so no consumer's matching behavior changes; but anything asserting on pattern text rather than match results does. It also **fixes a latent bug as a side effect**: a hyphen-bearing value interpolated into a character class previously formed a real range (`[a-z]` built from an escaped `"a-z"` matched `"m"`), and no longer does. Enforced by `eslint-rules/no-adhoc-regex-escape.cjs` (fires on the `.replace(, '\\$&')` shape anywhere outside this module, matching on shape rather than exact bytes, and on `new RegExp` built from an unescaped runtime value; exempts reviewed pattern-fragment constants such as `PHASE_NUMBER_TOKEN_SOURCE` by structural provenance — a module-scope const with a static initializer — not by name alone) plus `scripts/lint-no-adhoc-regex-escape.cjs`, a whole-tree companion covering directories ESLint's globs miss. Requires `engines.node >= 24.0.0`; a seam test asserts `typeof RegExp.escape === 'function'` so the floor and the capability cannot silently diverge. Source of truth: `gsd-core/bin/lib/pattern.cjs` (generated from `src/pattern.cts`). Design: `.gsd/phase/chore-3412-pattern-seam/40-design.md`. + ### Planning Snapshot Module Module owning the parsed projection of `.planning/` that a diagnostic rule may read, per ADR-3180 §8.1 (Decision 8, Phase 10, #3308). `buildPlanningSnapshot(cwd) → PlanningSnapshot` is composed EXCLUSIVELY from the already-consolidated §7 owners — `getMilestoneInfo` (Roadmap Parser Module), `listMilestonePhaseDirs` (Phase Locator Module), `isPhaseComplete` (Verification Module), `scanPhasePlans` (Plan Scan Module), `stateFieldValue`/`stateCurrentPositionSlice` (STATE.md Document Module), `planningPaths` (Planning Workspace Module) — and introduces no new semantic derivation of its own. `PlanningSnapshot` exposes `milestone`/`phaseDirs`/`phases`/`currentPhaseLabel`, each a `{value, scope}` pair per the Planning Scope Module's frozen `SCOPE` enum; `phases` additionally carries a `PhaseSnapshot[]` (`dir`, `complete`, `verificationStatus`, `planCount`, `summaryCount`, `scope`). The one new piece of logic this module adds is `worstScope(...scopes) → Scope`, a pure severity-ordered combinator (`UNREADABLE` > `UNSCOPED` > `TRUNCATED` > `COMPLETE`) that folds several independently-scoped owner answers about the same phase directory into one composite signal — NOT a re-derivation of any owner (each owner's own algorithm is untouched; only their already-computed `scope` verdicts are combined), but new coordination logic no single owner has the visibility to express. Every exposed field carries PARSED values only, never raw document text — this is structural, not advisory: a diagnostic rule given only the parsed value cannot re-derive a field's location the way `#3162`'s three inert `Current Phase` literal-search predicates did. Read failures on STATE.md (exists-but-unreadable, distinct from absent) are reported via the Unusable Input Diagnostic Module's `warnUnusableInput(UNUSABLE_REASON.STATE_UNREADABLE)`. Guarded by `scripts/lint-planning-snapshot-bypass-drift.cjs` (ratcheted per Decision 4(e), scoped to `DIAGNOSTIC_RULE_FUNCTIONS` — currently `cmdValidateHealth` in `src/verify.cts` only, acknowledging its existing raw `.planning/` reads as debt owned by Phase 11, #3309, which migrates it onto this snapshot). Source of truth: `gsd-core/bin/lib/planning-snapshot.cjs` (generated from `src/planning-snapshot.cts`). Design: `.gsd/phase/refactor-3308-planning-snapshot-parsed-projection/40-design.md`. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f852debbb..1e62d1287 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -195,7 +195,7 @@ Contributor requirements (summary): - **One concern per PR** — bug fixes, enhancements, and features must be separate PRs - **No drive-by formatting** — don't reformat code unrelated to your change - **Don't bundle test-fixture updates into `docs:` or unrelated commits** — when a production change makes an existing test assertion stale, the test correction MUST land as its own `test:` (or `fix:`) commit, not bundled into a `docs:` commit that also updates the explanation. The release-sdk hotfix cherry-pick filter routes by commit-subject prefix (`fix:`, `chore:`, `test:`); a test-fixture correction packed under a `docs:` prefix is invisible to the picker and ships a half-state to the hotfix branch — production code changed, test assertion stale. v1.42.3 hit this exact mode (#3621). The fix is upstream: keep the test-fixture commit separate. -- **CI must pass** — all configured matrix jobs must be green. Node 22 remains the compatibility floor; Node 24 is the primary target; Node 26 compatibility must be preserved for code and tests even when a Node 26 CI lane is not yet available. +- **CI must pass** — all configured matrix jobs must be green. Node 24 is the compatibility floor and primary target; Node 26 compatibility must be preserved for code and tests even when a Node 26 CI lane is not yet available. - **Scope matches the approved issue** — if your PR does more than what the issue describes, the extra changes will be asked to be removed or moved to a new issue ## CHANGELOG Entries — Drop a Fragment @@ -871,17 +871,16 @@ For everything else, if a test reaches for `.includes()` / `.startsWith()` / `as ### Node.js Version Compatibility -**Node 22 is the minimum supported version.** Node 24 is the primary CI target. Node 26 is the forward-compatibility target: do not add tests or production code that depend on deprecated behavior likely to fail there. +**Node 24 is the minimum supported version.** Node 24 is also the primary CI target. Node 26 is the forward-compatibility target: do not add tests or production code that depend on deprecated behavior likely to fail there. | Version | Status | |---------|--------| -| **Node 22** | Minimum required — Active LTS until October 2026, Maintenance LTS until April 2027 | -| **Node 24** | Primary CI target — current Active LTS, all tests must pass | +| **Node 24** | Minimum required and primary CI target — Active LTS, all tests must pass | | Node 26 | Forward-compatible target — avoid deprecated APIs and exact runtime-error prose | Do not use: - Deprecated APIs -- APIs not available in Node 22 +- APIs not available in Node 24 Safe to use: - `node:test` — stable since Node 18, fully featured in 24 diff --git a/bin/install.js b/bin/install.js index a392c169b..7a9bfbb29 100755 --- a/bin/install.js +++ b/bin/install.js @@ -59,6 +59,7 @@ const { composeWorkflow } = require('../gsd-core/bin/lib/workflow-fragments.cjs' // MCP catalog, src/mcp-catalog.cts) — see the comment at its call site below. const { shouldCompose } = require('../gsd-core/bin/lib/mcp-catalog.cjs'); const runtimeArtifactConversion = require('../gsd-core/bin/lib/runtime-artifact-conversion.cjs'); +const { escapeRegex: escapeRegExp } = require('../gsd-core/bin/lib/pattern.cjs'); // #2544: the CommonJS marker's single source of truth. classifyMarker() backs // BOTH ensureCommonJsMarker() (install) and removeCommonJsMarker() (uninstall), // so the write side can no longer clobber a package.json the remove side would @@ -1970,10 +1971,6 @@ function buildKiloAgentPermissionBlock(claudeTools) { return lines; } -function escapeRegExp(value) { - return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - function replaceRelativePathReference(content, fromPath, toPath) { const escapedPath = escapeRegExp(fromPath); return content.replace( diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index c70d7f328..ff55391dc 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -438,6 +438,7 @@ "onboard-projection.cjs", "package-identity.cjs", "package-legitimacy.cjs", + "pattern.cjs", "phase-command-router.cjs", "phase-estimation.cjs", "phase-id.cjs", diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 10081190e..981cb8aa9 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -540,6 +540,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`. | `model-resolver.cjs` | Model/effort resolution policy — resolves model, tier, granularity, effort, and fast-mode for an agent from config + model profiles/catalog (extracted from `core.cjs`, ADR-857) | | `package-identity.cjs` | Generated single source for GSD's published-package coordinates (npm name, bin name, repo slug, changelog URL, manual-install command), derived from package.json; read by the update worker, `check-latest-version`, and installer (#498) | | `package-legitimacy.cjs` | Registry-API package legitimacy verdicts (OK/SUS/SLOP) from npm/PyPI/crates, slopcheck optional | +| `pattern.cjs` | The pattern-construction seam — `escapeRegex` (delegates to the built-in `RegExp.escape`) and `literalPattern`; sole owner of building a `RegExp` from a runtime value (ADR-3212 §1, epic #3212 Phase 1, #3412) | | `phase-command-router.cjs` | Thin CJS subcommand router adapter for `gsd-tools phase` | | `phase-estimation.cjs` | Pure phase-effort estimation — `estimate`/`actuals` schema parse+render, smart-zone budget classification, and estimate-vs-actual calibration (median ratio, clamped, sample-gated). Confidence is derived from calibration sample count, never self-rated (ADR-2629) | | `phase-id.cjs` | Pure phase-id parsing/matching helpers — normalize, token match, milestone/phase-dir id parsing, phase-markdown regex builders (extracted from `core.cjs`, ADR-857) | diff --git a/docs/TESTING-SUITES.md b/docs/TESTING-SUITES.md index d11fa580f..0eb209629 100644 --- a/docs/TESTING-SUITES.md +++ b/docs/TESTING-SUITES.md @@ -307,33 +307,35 @@ either way — it is never the same as clean. The `Tests` workflow runs every PR through a scoped gate generated by `scripts/ci-test-scope.cjs`. -| Lane | Node 22 | Node 24 | -|---|---|---| -| `ubuntu-latest` | scoped tests | unit + integration + security | -| `windows-latest` | — | scoped Windows/path/shell tests | -| `macos-latest` | full parity when required | full parity when required | +All lanes run on **Node 24** — the `engines.node` floor (`>=24.0.0`) and the +only supported runtime. + +| Lane | Scope | +|---|---| +| `ubuntu-latest` (scoped) | scoped tests — fast PR signal | +| `ubuntu-latest` (full, sharded) | unit + integration + security | +| `windows-latest` | scoped Windows/path/shell tests | +| `macos-latest` | full parity when required | -- **Node 22** is the `engines.node` floor (`>=22.0.0`) — must stay green. -- **Node 24** is the default development lane. - **Scoped tests** are selected from the changed paths, plus a small CLI/package smoke set. They are for confidence on the affected surface, not for counting tests. The default PR gate runs the broad `unit` (under the c8 coverage gate), `integration`, and `security` suites once on Ubuntu / Node 24, scoped tests on -Ubuntu / Node 22, and scoped tests on Windows / Node 24. "Scoped" means the -diff-selected list from the rule table — not the full suite and not a fixed -smoke set (the fixed smoke list is only the empty-selection fallback). The -Windows lane's list is the Windows-sensitive subset of the selection, plus -**every changed test file, unconditionally** (the #494 invariant, narrowed): a -modified test is exercised on the divergent OS before merge at per-file cost, -without paying for the three full parity lanes. +a second Ubuntu / Node 24 lane, and scoped tests on Windows / Node 24. +"Scoped" means the diff-selected list from the rule table — not the full suite +and not a fixed smoke set (the fixed smoke list is only the empty-selection +fallback). The Windows lane's list is the Windows-sensitive subset of the +selection, plus **every changed test file, unconditionally** (the #494 +invariant, narrowed): a modified test is exercised on the divergent OS before +merge at per-file cost, without paying for the three full parity lanes. PRs touching workflow, package, test-runner, install, release, or Windows-sensitive surfaces also run the full parity matrix on macOS and the older Windows runtime, plus `install` and `slow` on the primary Ubuntu lane. Everything (including the full parity matrix) runs on every push to `next`, -which covers the residual macOS / Windows-Node-22 cross-product for scoped PRs. +which covers the residual macOS / Windows cross-product for scoped PRs. Coverage runs inside the Ubuntu / Node 24 full lane (not a separate job — that duplicated the entire unit run) and stays single-lane because multiplying @@ -391,8 +393,8 @@ event stream from a `gsd-test` run: ```bash node scripts/gen-test-timings.cjs \ - ~/.local/state/gsd-test/runs//test-events-linux-node22.jsonl \ - ~/.local/state/gsd-test/runs//test-events-linux-node24.jsonl + ~/.local/state/gsd-test/runs//test-events-linux-node24.jsonl \ + ~/.local/state/gsd-test/runs//test-events-macos-node24.jsonl ``` Pass every lane you have. A file's recorded time is the **max** across the diff --git a/docs/branch-protection.md b/docs/branch-protection.md index 24201721f..11a047db9 100644 --- a/docs/branch-protection.md +++ b/docs/branch-protection.md @@ -74,7 +74,7 @@ branch-protection context is the single aggregate `Required tests` job. Doc-only PRs run the lightweight lint and aggregate jobs only. Code-touching PRs run the default required matrix: -- Ubuntu / Node 22 scoped tests +- Ubuntu / Node 24 scoped tests - Ubuntu / Node 24 unit, integration, and security suites - Windows / Node 24 scoped Windows/path/shell tests - Ubuntu / Node 24 coverage @@ -83,8 +83,7 @@ PRs that touch workflow, package, test-runner, install, release, or Windows sensitive surfaces also run install/slow on the primary Ubuntu lane and the full parity matrix: -- Windows / Node 22 -- macOS / Node 22 +- Windows / Node 24 - macOS / Node 24 ### Canonical code-paths list diff --git a/docs/contributing/bootstrap.md b/docs/contributing/bootstrap.md index b28252cb7..1c2cb29c6 100644 --- a/docs/contributing/bootstrap.md +++ b/docs/contributing/bootstrap.md @@ -83,7 +83,7 @@ This runs `scripts/check-env.cjs` and reports pass/fail for each check: | Check | What it verifies | |---|---| -| `node-version` | Active Node satisfies `engines.node` (`>=22.0.0`) | +| `node-version` | Active Node satisfies `engines.node` (`>=24.0.0`) | | `npm-version` | Active npm satisfies `engines.npm` (`>=10.0.0`) | | `lockfile-present` | `package-lock.json` exists at root | | `lockfile-sync` | `npm ci --dry-run` exits 0 (lockfile matches installed state) | @@ -104,7 +104,7 @@ npm run check:env -- --json ## Troubleshooting -### `node-version` FAIL — Node X does NOT satisfy `>=22.0.0` +### `node-version` FAIL — Node X does NOT satisfy `>=24.0.0` **Cause:** The system Node is too old, or the version manager hasn't activated the correct version. diff --git a/eslint-rules/no-adhoc-regex-escape.cjs b/eslint-rules/no-adhoc-regex-escape.cjs new file mode 100644 index 000000000..a6953d09b --- /dev/null +++ b/eslint-rules/no-adhoc-regex-escape.cjs @@ -0,0 +1,442 @@ +'use strict'; + +/** + * no-adhoc-regex-escape + * + * ADR-3212 (lexical-seam-consolidation) §7 / #3412 Phase 1: + * `src/pattern.cts` is the single seam for regex-metacharacter escaping + * (`escapeRegex` / `literalPattern`, delegating to the built-in + * `RegExp.escape`). This rule flags the two ways a 13th hand-rolled copy of + * that concern can be reintroduced: + * + * 1. ADHOC-REPLACE — an inline `.replace(, '\$&')` call anywhere outside + * `src/pattern.cts`. Matches on the character-class + * MEMBER SET (the 14 regex metacharacters + * `. * + ? ^ $ { } ( ) | [ ] \`), not on exact byte + * order or escaping style — a reordered or + * differently-escaped class with the same member set + * is the same shape and still fires (design doc + * row 25). + * + * 2. UNSAFE-NEW-REGEXP — `new RegExp()` where `` + * is a genuinely dynamic, unescaped runtime value + * (a function parameter, or a `let`/`var` binding, or + * a `const` whose initializer is not a static + * string/template) with no seam routing. + * + * Negative space (design doc "Not-corruption"; test-matrix rows 26-28) — + * deliberately NOT flagged: + * + * - `src/pattern.cts` itself — the seam owns this shape. + * - A regex literal that merely CONTAINS the escape-class characters as + * DATA (e.g. `/^[.*+?]$/`) with no `.replace(..., '\$&')` shape present. + * - `new RegExp()` where `` is a REVIEWED PATTERN + * FRAGMENT. Provenance is accepted under EITHER of two conditions + * (strongest first): + * (a) STRUCTURAL — the identifier resolves (via scope walk) to a + * `const` declaration whose initializer is a string Literal or a + * TemplateLiteral with zero `${...}` expressions. This cannot be + * evaded by renaming the identifier (#3410's guard-evasion class: + * a rule keyed only on an identifier's NAME is defeated by a + * rename; a rule keyed on the const/static-initializer SHAPE is + * not). + * (b) NAMING CONVENTION (fallback) — the identifier ends in `_SOURCE` + * AND resolves (via scope walk) to one of: an ES `import` binding, + * or a `require(...)`-derived module-scope `const` (either + * `const { X_SOURCE } = require('./mod.cjs')` or + * `const X_SOURCE = require('./mod.cjs').X_SOURCE`). Kept for the + * cross-module case where the initializer itself is not in scope + * to inspect (condition (a) cannot see into another file), e.g. an + * imported `PHASE_NUMBER_TOKEN_SOURCE`. (a) is the real check; (b) + * only covers what (a) structurally cannot see — and is bound to + * the identifier's ACTUAL BINDING KIND, not just its spelling: a + * function parameter, a `let`/`var`, a reassigned binding, or an + * unresolvable identifier is NEVER exempted by naming alone, no + * matter what it's called. A naming-only check (no scope/binding + * verification) is exactly #3410's guard-evasion class — a rule + * keyed on spelling is defeated by picking a matching name for a + * genuinely dynamic value (e.g. a function parameter named + * `userInput_SOURCE`). If scope analysis cannot resolve the + * identifier's binding at all, this fails CLOSED (flags it) — + * an unresolvable binding is not evidence of safety. + * A `new RegExp()` argument that already routes through + * the seam (`escapeRegex(...)` / `literalPattern(...)`, by name or as a + * `pattern.escapeRegex(...)` member call) is also treated as safe — that + * IS the fix, not a violation. + * + * Per-finding exemption: add // allow-adhoc-regex-escape: as a + * trailing comment on the same source line, OR as a standalone comment on + * the line immediately preceding the flagged node (mirrors + * no-adhoc-markdown-parsing's `// allow-adhoc-markdown:` convention). + */ + +// The 14 regex metacharacters the census copies' char class escapes. +const ESCAPE_ALL_METACHARS_SET = new Set(['.', '*', '+', '?', '^', '$', '{', '}', '(', ')', '|', '[', ']', '\\']); + +// The self-reference replacement string every copy uses: '\$&' (3 chars: +// backslash, dollar, ampersand). +const SELF_REFERENCE_REPLACEMENT = '\\$&'; + +const SOURCE_SUFFIX_RE = /_SOURCE$/; + +/** + * Parse the first top-level (unescaped) `[...]` character class out of a + * regex source string and return its member-character set, or `null` if no + * class is present / it is unterminated. Single-pass, escape-aware — mirrors + * the style of no-adhoc-markdown-parsing's hasQualifyingNegatedPipeClass + * (walk once, honor `\` escapes, never backtrack into the class body). + */ +function parseCharClassMembers(src) { + let i = 0; + while (i < src.length) { + const ch = src[i]; + if (ch === '\\') { + i += 2; + continue; + } + if (ch === '[') break; + i += 1; + } + if (i >= src.length) return null; + + let j = i + 1; + let negated = false; + if (src[j] === '^') { + negated = true; + j += 1; + } + + const members = new Set(); + while (j < src.length) { + const c = src[j]; + if (c === '\\') { + const next = src[j + 1]; + if (next === undefined) return null; // unterminated escape + members.add(next); + j += 2; + continue; + } + if (c === ']') { + return { members, negated }; + } + members.add(c); + j += 1; + } + return null; // unterminated class +} + +/** + * Does `src` (a regex literal's `.pattern`) contain, as its first char + * class, EXACTLY the escape-all-metachars member set — regardless of + * ordering or which members are backslash-escaped (row 25: shape, not + * bytes)? + */ +function isEscapeAllMetacharsClassSource(src) { + const parsed = parseCharClassMembers(src || ''); + if (!parsed || parsed.negated) return false; + if (parsed.members.size !== ESCAPE_ALL_METACHARS_SET.size) return false; + for (const m of ESCAPE_ALL_METACHARS_SET) { + if (!parsed.members.has(m)) return false; + } + return true; +} + +/** Is `node` the regex literal `.replace()` first-argument shape? */ +function isEscapeAllMetacharsRegexLiteral(node) { + return node.type === 'Literal' && !!node.regex && isEscapeAllMetacharsClassSource(node.regex.pattern); +} + +/** Is `node` the string literal `'\$&'` self-reference replacement? */ +function isSelfReferenceReplacement(node) { + return node.type === 'Literal' && typeof node.value === 'string' && node.value === SELF_REFERENCE_REPLACEMENT; +} + +/** + * Walk up the scope chain from `scope` looking for `identifierName`'s + * declaration. Returns `{ def, kind }` where `kind` is 'param' for a + * function-parameter binding, 'import' for an ES `import` binding, or the + * VariableDeclaration `kind` ('const'/'let'/'var') for a variable binding — + * or `null` if unresolved in any enclosing scope reachable from here (fails + * closed: callers must treat `null` as "not provably safe"). + */ +function resolveBinding(identifierName, scope) { + let s = scope; + while (s) { + const variable = s.variables.find((v) => v.name === identifierName); + if (variable) { + const def = variable.defs && variable.defs[0]; + if (!def) return null; + if (def.type === 'Parameter') return { def, kind: 'param' }; + if (def.type === 'ImportBinding') return { def, kind: 'import' }; + if ( + def.node + && def.node.type === 'VariableDeclarator' + && def.parent + && def.parent.type === 'VariableDeclaration' + ) { + return { def, kind: def.parent.kind }; + } + return null; + } + s = s.upper; + } + return null; +} + +/** Is `node` a `require(...)` call expression? */ +function isRequireCallExpression(node) { + return !!node && node.type === 'CallExpression' + && node.callee && node.callee.type === 'Identifier' && node.callee.name === 'require'; +} + +/** + * Provenance condition (b), require branch — does `identifierName` resolve + * to a module-scope `const` whose initializer is EITHER `require(...)` + * itself (the `const { X_SOURCE } = require('./mod.cjs')` destructure shape) + * OR a non-computed member access on a `require(...)` call (the + * `const X_SOURCE = require('./mod.cjs').X_SOURCE` shape)? + */ +function resolvesToRequireDerivedConstBinding(identifierName, scope) { + const binding = resolveBinding(identifierName, scope); + if (!binding || binding.kind !== 'const') return false; + const init = binding.def.node.init; + if (!init) return false; + if (isRequireCallExpression(init)) return true; + if (init.type === 'MemberExpression' && !init.computed && isRequireCallExpression(init.object)) return true; + return false; +} + +/** Provenance condition (b), import branch — does `identifierName` resolve to an ES `import` binding? */ +function resolvesToImportBinding(identifierName, scope) { + const binding = resolveBinding(identifierName, scope); + return !!binding && binding.kind === 'import'; +} + +/** + * Provenance condition (a) — STRUCTURAL: does `identifierName` resolve to a + * `const` whose initializer is a static string Literal or a TemplateLiteral + * with no `${...}` expressions? + */ +function resolvesToStaticConstFragment(identifierName, scope) { + const binding = resolveBinding(identifierName, scope); + if (!binding || binding.kind !== 'const') return false; + const init = binding.def.node.init; + if (!init) return false; + if (init.type === 'Literal' && typeof init.value === 'string') return true; + if (init.type === 'TemplateLiteral' && (!init.expressions || init.expressions.length === 0)) return true; + return false; +} + +/** + * UNSAFE-NEW-REGEXP is deliberately narrow. An earlier version of this rule + * flagged ANY `new RegExp()` whose identifier resolved to a + * function parameter, a `let`/`var`, or a `const` with a non-static + * initializer — e.g. `const src = someHelper(x); new RegExp(src);`. A first + * whole-repo run proved that heuristic unusable: it fired ~25 times across + * `tests/**` on code with no relation to the escape-helper concern — + * `for (const level of [...]) { ...new RegExp(level)... }` (a fixed literal + * array), `patterns.filter(p => new RegExp(p).test(...))` (`p` is itself an + * already-regex-shaped source string, not literal text needing escaping), + * `const src = phaseId.phaseMarkdownRegexSource(x); new RegExp(src)` (a + * value produced by a function that ALREADY routes through escapeRegex + * internally, just not through a literal `new RegExp(escapeRegex(...))` + * one-liner). A syntax-only rule cannot distinguish "raw literal text that + * needed escaping and didn't get it" from "an already-valid regex-source + * string produced elsewhere" for an arbitrary non-literal expression — so it + * does not try to for the general case. + * + * What IS a reliable, false-positive-free signal is the row-29 shape + * itself: a function whose ENTIRE body is `return new RegExp()` (or an arrow function's implicit-return equivalent) — i.e. a + * hand-rolled re-implementation of `literalPattern()` that skips the + * escaping step. That shape cannot arise from "value already computed + * upstream and merely re-used" (there is no upstream — the parameter IS the + * function's only input), so it is exactly ADR §7's "new RegExp() built + * from a non-literal without routing through the seam" with no other + * plausible reading. + */ +function isSoleReturnOfOwnParameter(newExprNode, argIdentifier) { + let fn = null; + + const parent = newExprNode.parent; + if (!parent) return false; + + if (parent.type === 'ReturnStatement' && parent.argument === newExprNode) { + const block = parent.parent; + if ( + block + && block.type === 'BlockStatement' + && block.body.length === 1 + && block.parent + && (block.parent.type === 'FunctionDeclaration' + || block.parent.type === 'FunctionExpression' + || block.parent.type === 'ArrowFunctionExpression') + && block.parent.body === block + ) { + fn = block.parent; + } + } + else if (parent.type === 'ArrowFunctionExpression' && parent.body === newExprNode) { + fn = parent; + } + + if (!fn) return false; + return (fn.params || []).some((p) => p.type === 'Identifier' && p.name === argIdentifier.name); +} + +function isReviewedPatternFragmentIdentifier(identifierName, scope) { + if (resolvesToStaticConstFragment(identifierName, scope)) return true; // (a) structural + if (SOURCE_SUFFIX_RE.test(identifierName)) { + // (b) naming fallback — only trusted when the identifier's ACTUAL + // BINDING is an import or a require()-derived module-scope const. + // A function parameter, a let/var, a reassigned binding, or an + // unresolvable identifier is never exempted by spelling alone (#3410 + // guard-evasion class) — fails closed via resolveBinding returning null. + if (resolvesToImportBinding(identifierName, scope)) return true; + if (resolvesToRequireDerivedConstBinding(identifierName, scope)) return true; + return false; + } + return false; +} + +/** Does `node` route through the seam (`escapeRegex(...)` / `literalPattern(...)`)? */ +function isSeamRoutedCall(node) { + if (node.type !== 'CallExpression') return false; + const callee = node.callee; + if (!callee) return false; + if (callee.type === 'Identifier') { + return callee.name === 'escapeRegex' || callee.name === 'literalPattern'; + } + if (callee.type === 'MemberExpression' && !callee.computed && callee.property) { + return callee.property.name === 'escapeRegex' || callee.property.name === 'literalPattern'; + } + return false; +} + +/** @type {import('eslint').Rule.RuleModule} */ +const rule = { + meta: { + type: 'problem', + docs: { + description: + 'Disallow a hand-rolled regex-metacharacter-escape helper or an unrouted new RegExp() from a runtime value outside src/pattern.cts — import escapeRegex()/literalPattern() from the seam instead.', + category: 'Best Practices', + }, + schema: [], + messages: { + adhocRegexEscape: + 'Ad-hoc regex-metacharacter-escape detected (.replace() against the escape-all-metachars character class, with the \'\\$&\' self-reference replacement). Import escapeRegex() from ./pattern (src/pattern.cts) instead. Suppress with: // allow-adhoc-regex-escape: ', + unsafeNewRegExp: + 'new RegExp() built from a runtime value with no escaping and no seam routing. Route it through escapeRegex()/literalPattern() from ./pattern (src/pattern.cts) first. Suppress with: // allow-adhoc-regex-escape: ', + }, + }, + + create(context) { + const filename = context.getFilename ? context.getFilename() : context.filename; + const normalizedFilename = filename.replace(/\\/g, '/'); + // The seam owns this shape — it is not a violation of itself. + if (/(?:^|\/)src\/pattern\.cts$/.test(normalizedFilename)) { + return {}; + } + + const sourceCode = context.getSourceCode ? context.getSourceCode() : context.sourceCode; + + function isAllowed(node) { + const nodeStartLine = node.loc.start.line; + const allComments = sourceCode.getAllComments(); + return allComments.some((c) => { + if (!/allow-adhoc-regex-escape:\s*\S/.test(c.value)) return false; + return c.loc.start.line === nodeStartLine || c.loc.start.line === nodeStartLine - 1; + }); + } + + return { + // ── ADHOC-REPLACE: .replace(, '\$&') ────────────── + CallExpression(node) { + const callee = node.callee; + if (callee && callee.type === 'MemberExpression' && !callee.computed + && callee.property && callee.property.name === 'replace') { + const args = node.arguments || []; + const patternArg = args[0]; + const replacementArg = args[1]; + if ( + patternArg + && replacementArg + && isEscapeAllMetacharsRegexLiteral(patternArg) + && isSelfReferenceReplacement(replacementArg) + ) { + if (!isAllowed(node)) { + context.report({ node, messageId: 'adhocRegexEscape' }); + } + } + return; + } + + // ── UNSAFE-NEW-REGEXP is checked on NewExpression below; nothing + // else to do for other CallExpressions here. + }, + + // ── UNSAFE-NEW-REGEXP: new RegExp() ────── + NewExpression(node) { + if (!node.callee || node.callee.type !== 'Identifier' || node.callee.name !== 'RegExp') return; + const arg = node.arguments && node.arguments[0]; + if (!arg) return; + + // Static text (string literal / no-interpolation template literal) — + // fully known at lint time, not a runtime value. Never flagged. + if (arg.type === 'Literal' && typeof arg.value === 'string') return; + if (arg.type === 'TemplateLiteral' && (!arg.expressions || arg.expressions.length === 0)) return; + + // Already routed through the seam — that IS the fix. + if (isSeamRoutedCall(arg)) return; + + if (arg.type === 'Identifier') { + const scope = context.getScope ? context.getScope() : sourceCode.getScope(node); + if (isReviewedPatternFragmentIdentifier(arg.name, scope)) { + return; + } + + // The `_SOURCE` suffix exists ONLY to claim provenance exemption + // (b). An identifier that carries the suffix but did NOT resolve + // to a legitimate import/require-derived binding above (or is + // unresolvable at all) IS the #3410-class evasion this check + // exists to close — flag it unconditionally, independent of the + // narrower sole-return-of-parameter shape below. This cannot + // reproduce the ~25 unrelated false positives an earlier, broader + // "any non-literal identifier" heuristic produced (see + // isSoleReturnOfOwnParameter's doc comment): those identifiers + // never carried the `_SOURCE` naming convention in the first + // place, so this branch never reaches them. + if (SOURCE_SUFFIX_RE.test(arg.name)) { + if (!isAllowed(node)) { + context.report({ node, messageId: 'unsafeNewRegExp' }); + } + return; + } + + // Narrow, false-positive-free shape only — see + // isSoleReturnOfOwnParameter's doc comment for why the broader + // "any non-literal identifier" heuristic was rejected. + if (isSoleReturnOfOwnParameter(node, arg)) { + if (!isAllowed(node)) { + context.report({ node, messageId: 'unsafeNewRegExp' }); + } + } + } + }, + }; + }, +}; + +// Exposed for reuse by scripts/lint-no-adhoc-regex-escape.cjs (the whole-tree +// parity backstop for directories/extensions this rule's eslint.config.mjs +// wiring does not reach, e.g. tests/fixtures/**/*.cts). ESLint itself only +// reads `.create`/`.meta` off a required rule module, so these extra +// properties are inert to the linter and exist purely as a single source of +// truth for the shape-detection logic (avoids a second, drifting copy of the +// character-class parser — the exact class of bug this rule exists to end). +rule.isEscapeAllMetacharsClassSource = isEscapeAllMetacharsClassSource; +rule.SELF_REFERENCE_REPLACEMENT = SELF_REFERENCE_REPLACEMENT; + +module.exports = rule; diff --git a/eslint.config.mjs b/eslint.config.mjs index 2ecb346d3..11bb01aa8 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -15,6 +15,7 @@ import noElapsedAssertion from './eslint-rules/no-elapsed-assertion.cjs'; import noRawRmsyncInTests from './eslint-rules/no-raw-rmsync-in-tests.cjs'; import noTautologicalAssert from './eslint-rules/no-tautological-assert.cjs'; import noAdhocMarkdownParsing from './eslint-rules/no-adhoc-markdown-parsing.cjs'; +import noAdhocRegexEscape from './eslint-rules/no-adhoc-regex-escape.cjs'; import noPathLiteralInAssert from './eslint-rules/no-path-literal-in-assert.cjs'; import noPosixModeBitAssert from './eslint-rules/no-posix-mode-bit-assert.cjs'; import noUnguardedNonportableExec from './eslint-rules/no-unguarded-nonportable-exec.cjs'; @@ -36,6 +37,7 @@ const localPlugin = { 'no-raw-rmsync-in-tests': noRawRmsyncInTests, 'no-tautological-assert': noTautologicalAssert, 'no-adhoc-markdown-parsing': noAdhocMarkdownParsing, + 'no-adhoc-regex-escape': noAdhocRegexEscape, 'no-path-literal-in-assert': noPathLiteralInAssert, 'no-posix-mode-bit-assert': noPosixModeBitAssert, 'no-unguarded-nonportable-exec': noUnguardedNonportableExec, @@ -135,6 +137,7 @@ export default tseslint.config( 'gsd-core/bin/lib/configuration.cjs', 'gsd-core/bin/lib/state-document.cjs', 'gsd-core/bin/lib/planning-snapshot.cjs', + 'gsd-core/bin/lib/pattern.cjs', 'gsd-core/bin/lib/health-diagnostic-types.cjs', 'gsd-core/bin/lib/health-diagnostic.cjs', 'gsd-core/bin/lib/health-diagnostic-rules/root-existence.cjs', @@ -306,6 +309,11 @@ export default tseslint.config( // ADR-1372 T7: enforce use of the markdown-sectionizer seam; grandfather // pre-migration sites with // allow-adhoc-markdown: 'local/no-adhoc-markdown-parsing': 'error', + // ADR-3212 Phase 1 (#3412): enforce the pattern-construction seam + // (src/pattern.cts's escapeRegex/literalPattern) — flags a re-inlined + // escape-all-metachars .replace() helper or an unrouted new RegExp() + // from a runtime value. + 'local/no-adhoc-regex-escape': 'error', // ADR-1703 Phase 5: flag path-returning calls interpolated into content // (markdown @-references, workflow files, generated docs) without POSIX // normalization. Promoted to 'error' after precision review (path.basename @@ -347,6 +355,10 @@ export default tseslint.config( rules: { 'local/normalize-path-in-content': 'error', 'local/require-fs-op-fallback': 'error', + // ADR-3212 Phase 1 (#3412): pattern-construction seam prohibition — + // scripts/build-hooks.js is a .js file, so it falls outside the + // scripts/**/*.cjs glob below and needs it registered here too. + 'local/no-adhoc-regex-escape': 'error', }, }, @@ -398,6 +410,9 @@ export default tseslint.config( // Promoted to error (#3313) — a fresh non-cached `npx eslint .` run found // zero live violations of this rule in this glob at promotion time. 'local/no-source-grep': 'error', + // ADR-3212 Phase 1 (#3412): pattern-construction seam prohibition — + // see the src/**/*.cts block above for detail. + 'local/no-adhoc-regex-escape': 'error', }, }, @@ -414,6 +429,8 @@ export default tseslint.config( 'no-empty': ['warn', { allowEmptyCatch: true }], 'no-useless-escape': 'warn', 'n/no-path-concat': 'error', + // ADR-3212 Phase 1 (#3412): pattern-construction seam prohibition. + 'local/no-adhoc-regex-escape': 'error', // n/no-process-exit is deliberately OFF for hooks ONLY. // // A hook is a standalone process whose ENTIRE contract is its exit code: the @@ -493,6 +510,12 @@ export default tseslint.config( // Ban a consolidation-epic folded suite appearing twice in one host file (#3271). // A second copy runs the same tests twice on every lane and drifts silently. 'local/no-duplicate-fold-marker': 'error', + // ADR-3212 Phase 1 (#3412): pattern-construction seam prohibition — + // see the src/**/*.cts block above for detail. The historical oracle + // inlined in tests/pattern.test.cjs is exempted per-finding with + // // allow-adhoc-regex-escape: comments (design doc Notes: "not a 13th + // production copy"). + 'local/no-adhoc-regex-escape': 'error', // Ban raw setTimeout sync + elapsed/duration-style assertions via no-restricted-syntax 'no-restricted-syntax': [ 'error', diff --git a/package-lock.json b/package-lock.json index 47b165dcc..e1400b351 100644 --- a/package-lock.json +++ b/package-lock.json @@ -26,6 +26,7 @@ "eslint": "^9.39.4", "eslint-plugin-n": "^17.24.0", "eslint-plugin-no-only-tests": "^3.4.0", + "espree": "^10.4.0", "fast-check": "^4.8.0", "globals": "^16.5.0", "js-yaml": "^4.3.1", @@ -33,7 +34,7 @@ "typescript-eslint": "^8.60.0" }, "engines": { - "node": ">=22.0.0", + "node": ">=24.0.0", "npm": ">=10.0.0" }, "optionalDependencies": { diff --git a/package.json b/package.json index 2b921edf2..42fe58be2 100644 --- a/package.json +++ b/package.json @@ -27,6 +27,7 @@ "!scripts/run-tests.cjs", "!scripts/affected-tests-lib.cjs", "!scripts/run-affected-tests.cjs", + "!scripts/lint-no-adhoc-regex-escape.cjs", "pi", "vscode" ], @@ -54,7 +55,7 @@ "access": "public" }, "engines": { - "node": ">=22.0.0", + "node": ">=24.0.0", "npm": ">=10.0.0" }, "dependencies": { @@ -69,6 +70,7 @@ "eslint": "^9.39.4", "eslint-plugin-n": "^17.24.0", "eslint-plugin-no-only-tests": "^3.4.0", + "espree": "^10.4.0", "fast-check": "^4.8.0", "globals": "^16.5.0", "js-yaml": "^4.3.1", @@ -114,7 +116,7 @@ "lint:table-schema-drift": "node scripts/lint-table-schema-drift.cjs", "lint:frontmatter-scalar-broad-grep": "node scripts/lint-frontmatter-scalar-broad-grep.cjs", "lint:removed-but-needed": "node scripts/lint-removed-but-needed.cjs", - "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs", + "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs", "lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs", "lint:regression-names": "node scripts/lint-regression-test-names.cjs", "lint:descriptions": "node scripts/lint-descriptions.cjs", diff --git a/scripts/ci-test-scope.cjs b/scripts/ci-test-scope.cjs index 48823c128..da1e130ac 100644 --- a/scripts/ci-test-scope.cjs +++ b/scripts/ci-test-scope.cjs @@ -474,8 +474,8 @@ function classify(files) { // the scoped windows lane instead of triggering the three full parity // lanes. (full_matrix fired on 15/15 sampled PRs because test-driven // PRs always touch tests/, costing ~25 runner-minutes each.) Changed - // tests already run on ubuntu-22 and ubuntu-24 via targeted_tests; the - // residual macOS / windows-node-22 cross-product is covered by the full + // tests already run on the two ubuntu-24 lanes via targeted_tests; the + // residual macOS / windows cross-product is covered by the full // matrix on every push to next. windows.add(file); } diff --git a/scripts/gen-adr-index.cjs b/scripts/gen-adr-index.cjs index 7947f8399..3429c36f0 100644 --- a/scripts/gen-adr-index.cjs +++ b/scripts/gen-adr-index.cjs @@ -30,6 +30,7 @@ const fs = require('node:fs'); const path = require('node:path'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const { escapeRegex: escapeRegExp } = require('../gsd-core/bin/lib/pattern.cjs'); const ROOT = path.resolve(__dirname, '..'); const ADR_DIR = path.join(ROOT, 'docs', 'adr'); @@ -95,19 +96,12 @@ const REASON = Object.freeze({ * added to `STATUSES` now covers the bracket for free, and the corpus's * parity test iterates the real exported array rather than a copy. */ -/** - * Escape a string for literal inclusion inside a dynamic RegExp alternation. - * Defence-in-depth, not a live-bug fix: `STATUSES` is a static array literal - * today, so nothing in it can currently carry a regex metacharacter. But - * nothing enforces that it STAYS static — if a future change ever derives it - * from external input (a config file, a corpus scan), an unescaped `join('|')` - * would let a status token break out of the alternation it is meant to be one - * branch of. - */ -function escapeRegExp(s) { - return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - +// Escaped for defence-in-depth, not a live-bug fix: `STATUSES` is a static +// array literal today, so nothing in it can currently carry a regex +// metacharacter. But nothing enforces that it STAYS static — if a future +// change ever derives it from external input (a config file, a corpus scan), +// an unescaped `join('|')` would let a status token break out of the +// alternation it is meant to be one branch of. const STATUS_BRACKET_RE = new RegExp(String.raw`\s*\[(${STATUSES.map(escapeRegExp).join('|')})\]\s*$`, 'i'); /** diff --git a/scripts/gen-loop-host-contract.cjs b/scripts/gen-loop-host-contract.cjs index 9d7bed19c..012d1ad72 100644 --- a/scripts/gen-loop-host-contract.cjs +++ b/scripts/gen-loop-host-contract.cjs @@ -20,6 +20,7 @@ const fs = require('node:fs'); const path = require('node:path'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const { escapeRegex: escapeRegExp } = require('../gsd-core/bin/lib/pattern.cjs'); const ROOT = path.resolve(__dirname, '..'); const WORKFLOWS_DIR = path.join(ROOT, 'gsd-core', 'workflows'); @@ -191,13 +192,6 @@ function parseLoopHostBlock(content, fileName) { * @param {string} fileName For error messages * @returns {string[]} Array of error strings; empty = OK */ -/** - * Escape a string for literal use in a RegExp. - */ -function escapeRegExp(s) { - return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - function crossCheckRoles(content, agentRoles, fileName) { const errors = []; for (const role of agentRoles) { diff --git a/scripts/lint-completion-predicate-drift.cjs b/scripts/lint-completion-predicate-drift.cjs index e37512427..b9fac4bf7 100644 --- a/scripts/lint-completion-predicate-drift.cjs +++ b/scripts/lint-completion-predicate-drift.cjs @@ -159,6 +159,7 @@ const path = require('node:path'); const driftScan = require('./lib/drift-scan.cjs'); const { MAX_REGEX_LITERAL_LEN, sanitizeForReport, scanTree } = driftScan; +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); // ─── SHARED TOKENIZER + FUNCTION ATTRIBUTION (mirrors lint-state-field-drift.cjs) ── // @@ -696,7 +697,7 @@ function findScanPhasePlansCompletedReadDrift(text, relPath, exemptFunctions) { for (const [fnKey, varNames] of varsByFn) { if (fnKey !== FN_KEY_MODULE && exemptFunctions && exemptFunctions.has(fnKey)) continue; for (const varName of varNames) { - const escaped = varName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const escaped = escapeRegex(varName); const readRe = new RegExp(`\\b${escaped}\\.completed\\b`); for (let i = 0; i < lines.length; i++) { const fnHere = innermostAt[i] || FN_KEY_MODULE; diff --git a/scripts/lint-no-adhoc-regex-escape.cjs b/scripts/lint-no-adhoc-regex-escape.cjs new file mode 100644 index 000000000..76dfc8803 --- /dev/null +++ b/scripts/lint-no-adhoc-regex-escape.cjs @@ -0,0 +1,236 @@ +#!/usr/bin/env node +'use strict'; + +/** + * lint-no-adhoc-regex-escape.cjs — the whole-tree parity backstop for + * `local/no-adhoc-regex-escape` (ADR-3212 §7 / #3412 Phase 1; test-matrix + * row 30: "a 13th copy of the escape body anywhere in the tree" must fail + * CI, not review). + * + * ## Why this exists alongside the ESLint rule + * + * `eslint.config.mjs` covers `src/**\/*.cts`, `scripts/**\/*.cjs`, + * `tests/**\/*.cjs`, `hooks/**\/*.js`+`*.cjs`, and a short explicit list of + * top-level `.js` files — but NOT every extension in every directory. A + * whole-tree file-inventory check found `tests/fixtures/brand-typing/*.cts` + * (6 files): `.cts` under `tests/` matches neither the `src/**\/*.cts` glob + * (wrong directory) nor the `tests/**\/*.cjs` glob (wrong extension), so the + * AST rule never sees them. A hand-rolled escape helper planted in a + * directory/extension combination outside every `eslint.config.mjs` glob + * would pass `npx eslint .` cleanly. This script closes that gap with a + * plain-text, whole-repo-tree scan (mirrors scripts/lint-removed-but-needed.cjs's + * SCAN_ROOTS + walk() + ExitError structure) instead of another eslint.config.mjs + * glob entry, so growing the covered surface never depends on remembering to + * add a new files: [...] block for the next unanticipated extension. + * + * ## What this checks + * + * Walks the ENTIRE repository tree (skipping node_modules/.git/dist/coverage/ + * .worktrees/.claude, mirroring eslint.config.mjs's global ignores), and for + * every source-like file (.cjs/.js/.mjs/.cts/.ts), text-scans for a + * `.replace(, )` call whose regex + * literal's character class matches the escape-all-metachars member set + * (reusing `eslint-rules/no-adhoc-regex-escape.cjs`'s + * `isEscapeAllMetacharsClassSource` — single source of truth for the shape, + * per CLAUDE.md's Generative Fix Divergence guidance) and whose replacement + * string is `'\$&'`. `src/pattern.cts` (the seam) and any line carrying a + * trailing/preceding `// allow-adhoc-regex-escape: ` comment are + * exempt, mirroring the ESLint rule's own suppression convention. + * + * A plain regex/text scan (not a full parser) is a deliberate, disclosed + * trade-off: it only needs to close the residual gap outside the AST rule's + * glob coverage, not replace it — the AST rule remains the primary, + * precision enforcement for everything it does cover. + */ + +const fs = require('node:fs'); +const path = require('node:path'); +const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const { isEscapeAllMetacharsClassSource, SELF_REFERENCE_REPLACEMENT } = require('../eslint-rules/no-adhoc-regex-escape.cjs'); + +const ROOT = path.join(__dirname, '..'); + +const SKIP_DIRS = new Set(['node_modules', '.git', 'dist', 'coverage', '.worktrees', '.claude']); +const SOURCE_EXT = new Set(['.cjs', '.js', '.mjs', '.cts', '.ts']); + +// The seam owns this shape — not a violation of itself. +const SEAM_FILE_RE = /(?:^|\/)src\/pattern\.cts$/; + +// NOTE: bin/install.js was considered for a blanket "generated bundle, +// downstream mirror" exemption (the same reasoning that excuses +// gsd-core/bin/lib/**/*.cjs below) but REJECTED after inspection — +// bin/install.js:1974 carries a genuine, hand-written `function +// escapeRegExp(value) { return value.replace(...); }` of its own (real +// executable code, not an embedded string copy of another file's content). +// It is therefore a real 13th-plus copy and deliberately left un-exempted; +// see this script's own findings for it. +const GENERATED_BUNDLE_FILES = new Set(); + +// The RuleTester fixture corpus for the ESLint rule itself. Row 24/25's +// `code:` fixtures deliberately contain the flagged shape as STRING DATA +// (inside String.raw / template literals) to prove the rule fires — this is +// not runnable production code, and the AST-based ESLint rule correctly does +// not fire on it either (it never becomes a real `.replace()` CallExpression +// in this file's own AST). This text-only scanner cannot make that +// distinction, so the file is exempted explicitly. Per the dispatch brief, +// this test file is a fixed contract and is not edited to route around this. +const RULE_FIXTURE_FILES = new Set(['tests/eslint-no-adhoc-regex-escape.test.cjs']); + +// Two suppression conventions are honored, matching what actually appears in +// this repo: the rule's own per-finding `// allow-adhoc-regex-escape: ` +// marker (mirrors no-adhoc-markdown-parsing's convention), and a native +// `// eslint-disable-next-line local/no-adhoc-regex-escape -- ` +// directive (what tests/pattern.test.cjs's historical-oracle exemptions use — +// real ESLint already honors this natively; this text-only scanner has to +// recognize it explicitly since it does not run through ESLint's engine). +const ALLOW_COMMENT_RE = /allow-adhoc-regex-escape:\s*\S/; +const ESLINT_DISABLE_RE = /eslint-disable(?:-next-line|-line)?\b[^\n]*\bno-adhoc-regex-escape\b/; + +function isSuppressedByComment(line) { + return ALLOW_COMMENT_RE.test(line) || ESLINT_DISABLE_RE.test(line); +} + +/** + * Is `rel` (repo-relative, POSIX-slash) a tsc-generated `.cjs` mirror of a + * `src/**\/*.cts` source (ADR-457)? Dynamic (checks the filesystem for the + * sibling `.cts`) rather than a hardcoded list, so it tracks whichever `.cts` + * modules exist without needing its own upkeep as the seam migration lands. + * @param {string} rel + * @param {string} root + */ +function isGeneratedLibMirror(rel, root) { + if (!rel.startsWith('gsd-core/bin/lib/') || !rel.endsWith('.cjs')) return false; + const candidateSrc = `src/${rel.slice('gsd-core/bin/lib/'.length, -'.cjs'.length)}.cts`; + return fs.existsSync(path.join(root, candidateSrc)); +} + +// A permissive, single-pass extraction of `.replace(, +// )` call text. Deliberately simple (no nested-nesting / +// multi-line-argument support) — this is a coverage backstop for the AST +// rule, not a replacement for it; every real census copy is a single-line +// `.replace(/[...]/g, '\$&')` call. +// +// ReDoS fix (#3412, CodeQL js/redos): the original outer alternation let a +// `[...]` run be consumed EITHER by the character-class branch OR one +// character at a time by the trailing catch-all branch, so on a failing +// match the engine explored both parses of every bracket pair — exponential +// (measured: n=26 -> 204ms, n=28 -> 791ms, n=30 -> 3475ms, ~2^n). Two +// independent fixes, both required, neither alone sufficient long-term: +// (a) the trailing branch is now `[^/\\\n[\]]` — it excludes `[` and `]`, +// so a bracket can only ever be consumed by the character-class +// branch. This removes the ambiguity and makes matching linear. +// Consequence: a regex literal containing a BARE unescaped `]` +// outside a character class is no longer matched by this backstop — +// acceptable, since this is a coverage backstop for the AST rule, not +// a replacement for it, and no census shape has that form. +// (b) every `*` is now a bounded quantifier (ADR-3212's locked +// bounded-quantifiers decision) — a second line of defense that caps +// worst-case work even if the grammar above is later loosened. +const REPLACE_CALL_RE = /\.replace\(\s{0,20}\/((?:\\.|\[(?:\\.|[^\]\\]){0,200}\]|[^/\\\n[\]]){1,400})\/([a-z]{0,10})\s{0,20},\s{0,20}(['"`])((?:\\.|(?!\3)[^\\]){0,400})\3\s{0,20}\)/g; + +function unescapeSimpleStringLiteral(raw) { + // Handles the two-char escapes this specific replacement string ever uses + // (\\ and \$); good enough for the '\$&' shape this script looks for. + return raw.replace(/\\(.)/g, '$1'); +} + +function walk(dir) { + const out = []; + let entries; + try { + entries = fs.readdirSync(dir, { withFileTypes: true }); + } + catch { + return out; + } + for (const entry of entries) { + if (SKIP_DIRS.has(entry.name)) continue; + const full = path.join(dir, entry.name); + if (entry.isDirectory()) out.push(...walk(full)); + else if (entry.isFile() && SOURCE_EXT.has(path.extname(entry.name))) out.push(full); + } + return out; +} + +/** + * Pure: does `content` (the text of a single file) contain the + * escape-all-metachars `.replace(, '\$&')` shape on a line that is + * NOT exempted by an `allow-adhoc-regex-escape:` comment? + * @param {string} content + * @returns {{ line: number }[]} + */ +function findViolations(content) { + const violations = []; + const lines = content.split('\n'); + let match; + REPLACE_CALL_RE.lastIndex = 0; + while ((match = REPLACE_CALL_RE.exec(content)) !== null) { + const [, classSource, , , replacementRaw] = match; + if (!isEscapeAllMetacharsClassSource(`[${classSource}]`)) continue; + if (unescapeSimpleStringLiteral(replacementRaw) !== SELF_REFERENCE_REPLACEMENT) continue; + + const upToMatch = content.slice(0, match.index); + const line = upToMatch.split('\n').length; + const sameLine = lines[line - 1] || ''; + const prevLine = lines[line - 2] || ''; + if (isSuppressedByComment(sameLine) || isSuppressedByComment(prevLine)) continue; + + violations.push({ line }); + } + return violations; +} + +/** + * Pure: scan the whole tree rooted at `root` and return every violation. + * @param {string} root + * @returns {{ file: string, line: number }[]} + */ +function scan(root) { + const violations = []; + for (const abs of walk(root)) { + const rel = path.relative(root, abs).replace(/\\/g, '/'); + if (SEAM_FILE_RE.test(rel)) continue; + if (GENERATED_BUNDLE_FILES.has(rel)) continue; + if (RULE_FIXTURE_FILES.has(rel)) continue; + if (isGeneratedLibMirror(rel, root)) continue; + let content; + try { + content = fs.readFileSync(abs, 'utf8'); + } + catch { + continue; // unreadable (broken symlink, binary) — skip + } + for (const v of findViolations(content)) { + violations.push({ file: rel, line: v.line }); + } + } + return violations; +} + +function main() { + const violations = scan(ROOT); + if (violations.length > 0) { + const detail = violations.map((v) => ` ${v.file}:${v.line}`).join('\n'); + throw new ExitError( + 1, + 'lint-no-adhoc-regex-escape: a hand-rolled regex-metacharacter-escape\n' + + '.replace(, \'\\$&\') copy was found outside\n' + + 'src/pattern.cts (ADR-3212 §7 / #3412, test-matrix row 30 — parity\n' + + 'assertion). Import escapeRegex() from src/pattern.cts instead, or\n' + + 'suppress a genuine non-production exception with a trailing\n' + + '// allow-adhoc-regex-escape: comment:\n' + + detail, + ); + } + console.log('ok lint-no-adhoc-regex-escape: no hand-rolled escape-metachars copy found outside src/pattern.cts'); +} + +module.exports = { + findViolations, + scan, + walk, + SOURCE_EXT, + SKIP_DIRS, +}; + +if (require.main === module) runMain(main); diff --git a/scripts/lint-removed-but-needed.cjs b/scripts/lint-removed-but-needed.cjs index 1bd165426..4c1c20e07 100644 --- a/scripts/lint-removed-but-needed.cjs +++ b/scripts/lint-removed-but-needed.cjs @@ -33,6 +33,7 @@ const fs = require('node:fs'); const path = require('node:path'); const cp = require('node:child_process'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const ROOT = path.join(__dirname, '..'); const SCAN_ROOTS = ['.github/workflows', 'gsd-core', 'docs']; @@ -42,10 +43,6 @@ const EXTRA_FILES = ['package.json']; // never carry a meaningful basename reference, and is often large. const SKIP_EXT = new Set(['.png', '.jpg', '.jpeg', '.gif', '.ico', '.woff', '.woff2', '.ttf', '.zip']); -function escapeRegex(s) { - return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - /** * Pure: does `content` contain a literal reference to `basename`, delimited * by non-identifier/non-path characters on both sides (so "foo.json" doesn't diff --git a/scripts/run-tests.cjs b/scripts/run-tests.cjs index 10fc76205..476ad6bf0 100644 --- a/scripts/run-tests.cjs +++ b/scripts/run-tests.cjs @@ -949,9 +949,12 @@ function main() { // makes a chunk's `node --test` child hang ~150s on Windows AFTER its last test // prints; two such stalls push the windows full lane past its 20m cap and the // job is CANCELLED with no failed step — a false-negative gate (#1051, recurrence - // of #869). --test-force-exit (Node >=22; engines requires >=22.0.0) exits the - // runner once all tests finish regardless of lingering handles. The leaking - // tests are also fixed at the source; this is the defensive backstop. + // of #869). --test-force-exit (available since Node 22; engines.node now + // requires >=24.0.0, so it is always available here — the nodeMajor check + // below is kept as a floor-independent CLI-flag-availability guard, not a + // statement of this repo's supported version) exits the runner once all + // tests finish regardless of lingering handles. The leaking tests are also + // fixed at the source; this is the defensive backstop. // RUN_TESTS_NO_FORCE_EXIT=1 disables it (used by the harness regression test to // observe the pre-fix hang). const nodeMajor = Number(process.versions.node.split('.')[0]); diff --git a/scripts/sync-runtime-launcher.cjs b/scripts/sync-runtime-launcher.cjs index dbc38a2b4..ebf5047b4 100644 --- a/scripts/sync-runtime-launcher.cjs +++ b/scripts/sync-runtime-launcher.cjs @@ -19,6 +19,8 @@ const fs = require('node:fs'); const path = require('node:path'); +const { escapeRegex: escapeRegExp } = require('../gsd-core/bin/lib/pattern.cjs'); + const WORKFLOWS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows'); const AGENTS_DIR = path.join(__dirname, '..', 'agents'); const SNIPPET_FILE = path.join(WORKFLOWS_DIR, '_runtime-launcher.snippet.sh'); @@ -373,10 +375,6 @@ function transformFile(content, preamble) { return outputLines.join('\n'); } -function escapeRegExp(str) { - return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - /** * A file "delegates to the shared resolver" when it pulls the canonical gsd_run * preamble in from gsd-core/references/gsd-run-resolver.md via an @-include diff --git a/src/api-coverage.cts b/src/api-coverage.cts index 4afb4bd74..961ae82fb 100644 --- a/src/api-coverage.cts +++ b/src/api-coverage.cts @@ -57,6 +57,7 @@ */ import { stripFencedCode, scanInlineCodeSpans, extractFencedBlock } from './markdown-sectionizer.cjs'; +import { escapeRegex } from './pattern.cjs'; // ─── Integration-signal vocabulary ──────────────────────────────────────────── @@ -170,10 +171,6 @@ function resolveTerms(terms?: Partial): ApiCoverageTermSet { return { verbs: merge('verbs'), nouns: merge('nouns') }; } -function escapeRegex(s: string): string { - return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - function makeSnippet(line: string, anchor: string): string { const cleaned = line.replace(/\s+/g, ' ').trim(); if (cleaned.length <= 120) return cleaned; diff --git a/src/assumption-delta.cts b/src/assumption-delta.cts index 8cae79677..ba1a93965 100644 --- a/src/assumption-delta.cts +++ b/src/assumption-delta.cts @@ -34,6 +34,7 @@ */ import { stripFencedCode } from './markdown-sectionizer.cjs'; +import { escapeRegex } from './pattern.cjs'; export type AssumptionDeltaKind = 'pluralization' | 'optional' | 'chosen'; @@ -210,10 +211,6 @@ export function detectAssumptionDelta( return { detected: signals.length > 0, signals, terms: effective }; } -function escapeRegex(s: string): string { - return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - // ── CLI entry point ────────────────────────────────────────────────────────── // Reads phase-section text from STDIN (not argv) to avoid OS ARG_MAX limits. // Invoked by workflow bash as: echo "$PHASE_SECTION" | node .../assumption-delta.cjs [--json] diff --git a/src/gap-checker.cts b/src/gap-checker.cts index 0cedde9e2..0db0771f1 100644 --- a/src/gap-checker.cts +++ b/src/gap-checker.cts @@ -21,9 +21,7 @@ import path from 'node:path'; // eslint-disable-next-line @typescript-eslint/no-require-imports import io = require('./io.cjs'); const { output, error } = io; -// eslint-disable-next-line @typescript-eslint/no-require-imports -import phaseId = require('./phase-id.cjs'); -const { escapeRegex } = phaseId; +import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspace = require('./planning-workspace.cjs'); const { planningPaths, planningDir, findContextMdIn } = planningWorkspace; diff --git a/src/init.cts b/src/init.cts index 2aebea796..d64f62183 100644 --- a/src/init.cts +++ b/src/init.cts @@ -11,6 +11,7 @@ import path from 'node:path'; import os from 'node:os'; import { execGit, platformWriteSync, platformReadSync, toNativePath, posixNormalize } from './shell-command-projection.cjs'; import { realClock } from './clock.cjs'; +import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- io.cjs is an export= CommonJS module import io = require('./io.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- config-loader.cjs is an export= CommonJS module @@ -88,7 +89,7 @@ const { extractCurrentMilestone, } = roadmapParser; const { pathExistsInternal, generateSlugInternal, toPosixPath } = coreUtils; -const { escapeRegex, normalizePhaseName, matchPhaseDirs, stripProjectCodePrefix, PHASE_NUMBER_TOKEN_SOURCE, isForeignPrefixedPhaseQuery, isSentinelPhaseId } = phaseId; +const { normalizePhaseName, matchPhaseDirs, stripProjectCodePrefix, PHASE_NUMBER_TOKEN_SOURCE, isForeignPrefixedPhaseQuery, isSentinelPhaseId } = phaseId; const { pruneOrphanedWorktrees } = worktreeSafety; const { diff --git a/src/markdown-sectionizer.cts b/src/markdown-sectionizer.cts index 3bf4e8b04..edf5ca427 100644 --- a/src/markdown-sectionizer.cts +++ b/src/markdown-sectionizer.cts @@ -10,6 +10,8 @@ * ADR-457 build-at-publish: compiled by tsc to gsd-core/bin/lib/markdown-sectionizer.cjs. */ +import { escapeRegex } from './pattern.cjs'; + // ─── Types ──────────────────────────────────────────────────────────────────── /** Result of stripping fenced code blocks from markdown content. */ @@ -940,7 +942,7 @@ export function extractTaggedBlocks(content: string, tagName: string, allowAttri * open>` marks the ACTIVE milestone and must be preserved, not stripped (#557). */ function taggedBlockPattern(tagName: string, flags: string, allowAttributes: boolean): RegExp { - const esc = tagName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const esc = escapeRegex(tagName); const open = allowAttributes ? `<${esc}(?:\\s[^>]{0,1000})?>` : `<${esc}>`; const boundary = allowAttributes ? `<${esc}[\\s>]` : `<${esc}>`; return new RegExp(`${open}((?:(?!${boundary})[\\s\\S])*?)`, flags); diff --git a/src/milestone.cts b/src/milestone.cts index 56aa7cf31..bd02444ba 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -26,7 +26,8 @@ import ioMod = require('./io.cjs'); const { output, error } = ioMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseIdMod = require('./phase-id.cjs'); -const { escapeRegex, normalizePhaseName, matchPhaseDirs, PHASE_NUMBER_TOKEN_SOURCE, isSentinelPhaseId } = phaseIdMod; +const { normalizePhaseName, matchPhaseDirs, PHASE_NUMBER_TOKEN_SOURCE, isSentinelPhaseId } = phaseIdMod; +import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import roadmapParserMod = require('./roadmap-parser.cjs'); const { diff --git a/src/pattern.cts b/src/pattern.cts new file mode 100644 index 000000000..de1067fd2 --- /dev/null +++ b/src/pattern.cts @@ -0,0 +1,50 @@ +/** + * pattern.cts — the pattern-construction seam (ADR-3212 §1, epic #3212 Phase 1, + * #3412). + * + * Source in src/pattern.cts, compiled to gsd-core/bin/lib/pattern.cjs + * (gitignored), per the repo's ADR-457 build-at-publish convention. + * + * Sole owner of building a `RegExp` from a runtime value. `escapeRegex` + * delegates to the built-in `RegExp.escape` (ES2026 / Node 24+) rather than + * hand-rolling yet another copy of the metacharacter-escape helper this + * module replaces — see ADR §1 ("No module outside the seam escapes a value + * for regex use"). + * + * Counts, corrected during implementation (design doc "Ground truth" #1; + * .gsd/phase/chore-3412-pattern-seam/40-design.md): the ADR's census counted + * ~12 *named* helper functions. The shape-based lint guard + * (eslint-rules/no-adhoc-regex-escape.cjs), which matches the escape-class + * SHAPE wherever it appears rather than named-function bodies, found 27 + * additional inline copies that census-by-name could not see — **~39 escape + * sites total**. Call sites follow the same pattern: 17 were surveyed + * directly, but `src/phase-id.cts`'s `escapeRegex` turned out to have 8 + * external production importers the pre-implementation survey missed, plus + * the 27 guard-found inline sites also call into the seam — **~44 call + * sites total**. The lesson worth keeping: a named-function census + * structurally cannot see an inline `.replace(...)` copy or an + * externally-imported symbol; only a shape-based guard (or a direct + * importer graph query) does. + * + * `escapeRegex` is the PRIMARY export: the large majority of call sites + * build a *source string* (alternation via `.map(escapeRegex).join('|')`, or + * template/concat interpolation into a larger pattern) rather than a + * standalone literal match. `literalPattern` is the minority convenience + * wrapper for the remaining "match this value literally" shape — not the + * dominant one (design doc "Ground truth" #2). + * + * Behavior-preserving for MATCH RESULTS, not for pattern TEXT: `RegExp.escape` + * hex-escapes the leading character of nearly every non-empty input (and `-`, + * space, and control characters throughout), so the escaped source string + * differs from the twelve deleted copies' output for almost every value. + * Match behavior against that source is unaffected — verified by the + * migration-equivalence property sweep in tests/pattern.test.cjs (rows 15-17). + */ + +export function escapeRegex(value: string): string { + return RegExp.escape(value); +} + +export function literalPattern(value: string, flags?: string): RegExp { + return new RegExp(escapeRegex(value), flags); +} diff --git a/src/phase-id.cts b/src/phase-id.cts index 28ecc4b17..08f6a9aea 100644 --- a/src/phase-id.cts +++ b/src/phase-id.cts @@ -7,14 +7,14 @@ * boundary moved. The core.cjs re-export spine was retired in epic #1267; * callers import phase-id helpers from phase-id.cjs directly. * - * Dependencies: none (pure string/regex, no Node built-ins required). + * Dependencies: + * - ./pattern.cjs (escapeRegex — #3212 Phase 1 seam; this module is no + * longer the owner of pattern-escaping, only a consumer) */ -// ─── Phase-id helpers ───────────────────────────────────────────────────────── +import { escapeRegex } from './pattern.cjs'; -function escapeRegex(value: unknown): string { - return String(value).replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} +// ─── Phase-id helpers ───────────────────────────────────────────────────────── // project_code values start with an uppercase letter (e.g. PROJ, APP_CODE); // leading underscores are not valid project codes per .planning/config.json. @@ -437,7 +437,11 @@ function phaseMarkdownRegexSource(phaseNum: unknown): string { // Plain numeric phase: 1, 01, 12A, 12.1 const match = stripped.match(/^0*(\d+)([A-Z])?((?:\.\d+)*)$/i); - if (!match) return escapeRegex(phaseNum); + // #3212: escapeRegex now requires a string (the seam owns coercion policy, + // not this module) — String(...) here preserves this function's own + // pre-existing `unknown` acceptance for callers that pass a non-string + // phaseNum through to this fallback branch. + if (!match) return escapeRegex(String(phaseNum)); const integer = match[1].replace(/^0+/, '') || '0'; const letter = match[2] ? escapeRegex(match[2]) : ''; @@ -924,7 +928,6 @@ function roadmapPhaseLookupSources(phaseNum: unknown): string[] { } export = { - escapeRegex, OPTIONAL_PROJECT_CODE_PREFIX_SOURCE, OPTIONAL_PHASE_TAG_SOURCE, PHASE_NUMBER_TOKEN_SOURCE, diff --git a/src/phase.cts b/src/phase.cts index 05b33855a..d0d66cd8f 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -38,7 +38,6 @@ const { // eslint-disable-next-line @typescript-eslint/no-require-imports -- phase-id.cjs is an export= CommonJS module import phaseIdMod = require('./phase-id.cjs'); const { - escapeRegex, normalizePhaseName, phaseMarkdownRegexSource, comparePhaseNum, @@ -48,6 +47,7 @@ const { OPTIONAL_PHASE_TAG_SOURCE, PHASE_NUMBER_TOKEN_SOURCE, } = phaseIdMod; +import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- phase-locator.cjs is an export= CommonJS module import phaseLocatorMod = require('./phase-locator.cjs'); const { findPhaseInternal, getArchivedPhaseDirs, listMilestonePhaseDirs } = phaseLocatorMod; diff --git a/src/roadmap-parser.cts b/src/roadmap-parser.cts index a76521314..bddd251d4 100644 --- a/src/roadmap-parser.cts +++ b/src/roadmap-parser.cts @@ -10,7 +10,8 @@ * * Dependencies (leaf modules only — no loadConfig): * - node:fs / node:path (stdlib) - * - ./phase-id.cjs (escapeRegex, phaseMarkdownRegexSource) + * - ./phase-id.cjs (phaseMarkdownRegexSource) + * - ./pattern.cjs (escapeRegex — #3212 Phase 1 seam) * - ./planning-workspace.cjs (planningDir) * - ./shell-command-projection.cjs (platformReadSync) * - ./markdown-sectionizer.cjs (tokenizeHeadings, stripTaggedBlocks, withSection, collectSection) @@ -19,10 +20,10 @@ import fs from 'node:fs'; import path from 'node:path'; +import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseIdModule = require('./phase-id.cjs'); const { - escapeRegex, phaseMarkdownRegexSource, stripProjectCodePrefix, OPTIONAL_PHASE_TAG_SOURCE, diff --git a/src/roadmap.cts b/src/roadmap.cts index 1050522e9..65d1291f6 100644 --- a/src/roadmap.cts +++ b/src/roadmap.cts @@ -9,12 +9,13 @@ import fs from 'node:fs'; import path from 'node:path'; import { realClock } from './clock.cjs'; +import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output, error } = ioMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseIdMod = require('./phase-id.cjs'); -const { escapeRegex, normalizePhaseName, phaseMarkdownRegexSource, matchPhaseDirs, stripProjectCodePrefix, OPTIONAL_PHASE_TAG_SOURCE, roadmapPhaseLookupSources, isSentinelPhaseId } = phaseIdMod; +const { normalizePhaseName, phaseMarkdownRegexSource, matchPhaseDirs, stripProjectCodePrefix, OPTIONAL_PHASE_TAG_SOURCE, roadmapPhaseLookupSources, isSentinelPhaseId } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseLocatorMod = require('./phase-locator.cjs'); const { findPhaseInternal, listMilestonePhaseDirs } = phaseLocatorMod; @@ -124,17 +125,26 @@ function countPhasePlansAndSummaries(phaseDir: string): PhasePlansAndSummaries { // ─── searchPhaseInContent ───────────────────────────────────────────────────── +/** + * Build the phase-heading regex used by `searchPhaseInContent` for a given + * pre-escaped phase source. Extracted (#3412) so tests can assert against the + * exact production pattern instead of hand-duplicating it. + * #1729: OPTIONAL_PHASE_TAG_SOURCE after the number tolerates a pre-colon ( ) tag. + */ +function buildPhaseHeadingRegex(escapedPhase: string): RegExp { + return new RegExp( + `^(?:\\[[^\\]]{1,200}\\]\\s*)?Phase\\s+${escapedPhase}${OPTIONAL_PHASE_TAG_SOURCE}:\\s*(.+)$`, + 'i' + ); +} + /** * Search for a phase header (and its section) within the given content string. * Returns a result object if found (either a full match or a malformed_roadmap * checklist-only match), or null if the phase is not present at all. */ function searchPhaseInContent(content: string, escapedPhase: string, phaseNum: string): PhaseSearchResult | null { - // #1729: OPTIONAL_PHASE_TAG_SOURCE after the number tolerates a pre-colon ( ) tag. - const headingPattern = new RegExp( - `^(?:\\[[^\\]]{1,200}\\]\\s*)?Phase\\s+${escapedPhase}${OPTIONAL_PHASE_TAG_SOURCE}:\\s*(.+)$`, - 'i' - ); + const headingPattern = buildPhaseHeadingRegex(escapedPhase); const headings = tokenizeHeadings(content); const headingIndex = headings.findIndex((heading) => headingPattern.test(heading.text)); const headerMatch = headingIndex === -1 ? null : headings[headingIndex].text.match(headingPattern); @@ -1084,4 +1094,5 @@ export = { cmdRoadmapAnalyze, cmdRoadmapUpdatePlanProgress, cmdRoadmapAnnotateDependencies, + buildPhaseHeadingRegex, }; diff --git a/src/runtime-artifact-conversion.cts b/src/runtime-artifact-conversion.cts index 442537a42..8ee05dc62 100644 --- a/src/runtime-artifact-conversion.cts +++ b/src/runtime-artifact-conversion.cts @@ -26,6 +26,7 @@ const { getDirName } = runtimeNamePolicy; import capabilityRegistry = require('./capability-registry.cjs'); import hostIntegration = require('./host-integration.cjs'); import { posixNormalize } from './shell-command-projection.cjs'; +import { escapeRegex as escapeRegExp } from './pattern.cjs'; // #2870: install-scope.cts is a leaf-tier sibling (imports only // runtime-homes.cjs + node builtins, never this module) — no cycle. See the // isGlobal sites below for why the boolean projection is centralized here too. @@ -292,10 +293,6 @@ function buildKiloAgentPermissionBlock(claudeTools) { return lines; } -function escapeRegExp(value) { - return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - function replaceRelativePathReference(content, fromPath, toPath) { const escapedPath = escapeRegExp(fromPath); return content.replace( diff --git a/src/shell-command-projection.cts b/src/shell-command-projection.cts index cf0f8ec6b..5b7558b57 100644 --- a/src/shell-command-projection.cts +++ b/src/shell-command-projection.cts @@ -17,6 +17,7 @@ import fs from 'node:fs'; // can intercept calls from this seam — destructured imports capture references // at load time and become un-mockable. import childProcess from 'node:child_process'; +import { escapeRegex } from './pattern.cjs'; /** * Convert a filesystem path to POSIX form (forward slashes) by translating the @@ -297,7 +298,7 @@ export function isManagedHookCommand(commandText: unknown, opts: { surface?: str } for (const basename of managedBasenames) { - const escapedBasename = basename.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const escapedBasename = escapeRegex(basename); const pattern = new RegExp(`(^|[\\\\/\\s"'` + '`' + `])${escapedBasename}(?=$|[\\s"'` + '`' + `])`); if (pattern.test(normalizedCommand)) return true; } diff --git a/src/state-document.cts b/src/state-document.cts index 192c89c97..a47aa9f8b 100644 --- a/src/state-document.cts +++ b/src/state-document.cts @@ -11,16 +11,12 @@ import { splitTableRow } from './markdown-table.cjs'; import { clampPercentFromFraction } from './phase-lifecycle.cjs'; import { collectSection } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; +import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-scope.cjs is an export= CommonJS module import planningScopeMod = require('./planning-scope.cjs'); const { SCOPE } = planningScopeMod; type Scope = planningScopeMod.Scope; -// Internal helpers -function escapeRegex(str: string): string { - return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - function toFiniteNumber(value: unknown): number | null { const number = Number(value); return Number.isFinite(number) ? number : null; diff --git a/src/state-transition.cts b/src/state-transition.cts index 859c64182..8f370e414 100644 --- a/src/state-transition.cts +++ b/src/state-transition.cts @@ -21,11 +21,9 @@ import { KNOWN_TEMPLATE_DEFAULTS } from './state-document.cjs'; import { tokenizeHeadings } from './markdown-sectionizer.cjs'; import type { HeadingToken } from './markdown-sectionizer.cjs'; import { deriveProgressFromRoadmap, clampPercent } from './phase-lifecycle.cjs'; -// eslint-disable-next-line @typescript-eslint/no-require-imports -import phaseIdMod = require('./phase-id.cjs'); +import { escapeRegex } from './pattern.cjs'; const { extractFrontmatter, reconstructFrontmatter, stripFrontmatter } = frontmatter; -const { escapeRegex } = phaseIdMod; // Stop predicate for section-body slicing: a level-2+ heading ends the section. const STOP_H2_PLUS = (lv: number): boolean => lv >= 2; diff --git a/src/state.cts b/src/state.cts index fafffbb2d..7f52f562b 100644 --- a/src/state.cts +++ b/src/state.cts @@ -8,6 +8,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { escapeRegex } from './pattern.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); const { output, error } = ioMod; @@ -16,7 +17,7 @@ import configLoaderMod = require('./config-loader.cjs'); const { loadConfig } = configLoaderMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import phaseIdMod = require('./phase-id.cjs'); -const { escapeRegex, parsePhaseFromProse, PHASE_NUMBER_TOKEN_SOURCE, phaseKeyFromToken, phaseKeyFromDir, isSentinelPhaseId } = phaseIdMod; +const { parsePhaseFromProse, PHASE_NUMBER_TOKEN_SOURCE, phaseKeyFromToken, phaseKeyFromDir, isSentinelPhaseId } = phaseIdMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import roadmapParserMod = require('./roadmap-parser.cjs'); const { getMilestoneInfo, extractCurrentMilestone, isMilestoneBoundedInRoadmap, hasMilestoneSectioning } = roadmapParserMod; diff --git a/tests/adr-index-gate.test.cjs b/tests/adr-index-gate.test.cjs index d35e09664..077a68304 100644 --- a/tests/adr-index-gate.test.cjs +++ b/tests/adr-index-gate.test.cjs @@ -16,6 +16,7 @@ const path = require('node:path'); const { spawnSync } = require('node:child_process'); const { createTempDir, cleanup } = require('./helpers.cjs'); +const { copyScriptWithDeps } = require('./helpers/copy-script-fixture.cjs'); const REPO_ROOT = path.resolve(__dirname, '..'); const SCRIPT_REL = path.join('scripts', 'gen-adr-index.cjs'); @@ -25,22 +26,18 @@ const END = ''; /** * Build a throwaway repo whose docs/adr/ contains exactly `files`, and whose - * scripts/ holds a copy of the generator + its cli-exit dependency. A unique - * mkdtemp per call keeps parallel tests from colliding, and the dir is removed - * via `t.after()` so a failing assertion cannot leak it. + * scripts/ holds a copy of the generator together with its transitive + * relative-require graph. A unique mkdtemp per call keeps parallel tests from + * colliding, and the dir is removed via `t.after()` so a failing assertion + * cannot leak it. */ function makeRepo(t, files) { // helpers.cleanup (not raw fs.rmSync) carries the Windows-EBUSY retry budget. const root = createTempDir('gsd-adr-index-'); t.after(() => cleanup(root)); fs.mkdirSync(path.join(root, 'docs', 'adr'), { recursive: true }); - fs.mkdirSync(path.join(root, 'scripts', 'lib'), { recursive: true }); - fs.copyFileSync(path.join(REPO_ROOT, SCRIPT_REL), path.join(root, SCRIPT_REL)); - fs.copyFileSync( - path.join(REPO_ROOT, 'scripts', 'lib', 'cli-exit.cjs'), - path.join(root, 'scripts', 'lib', 'cli-exit.cjs'), - ); + copyScriptWithDeps(REPO_ROOT, root, SCRIPT_REL); for (const [name, body] of Object.entries(files)) { fs.writeFileSync(path.join(root, 'docs', 'adr', name), body); diff --git a/tests/api-coverage-gate-e2e.test.cjs b/tests/api-coverage-gate-e2e.test.cjs index 6dee682f0..4cb629afb 100644 --- a/tests/api-coverage-gate-e2e.test.cjs +++ b/tests/api-coverage-gate-e2e.test.cjs @@ -26,6 +26,7 @@ const { runNode, OUTCOME } = require('./helpers/process-seam.cjs'); // file (#2365 review): readPhaseScope is the pure phase-scope reader behind the // gate. Those tests monkeypatch fs rather than drive a subprocess. const { readPhaseScope } = require('../gsd-core/bin/lib/check-command-router.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const TOOLS_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'gsd-tools.cjs'); @@ -342,7 +343,7 @@ describe('readPhaseScope — fail-closed on a real read failure (#2365 review)', tmpDir = makeProject({ api_coverage_gate: true }); const phaseDir = makePhaseDir(tmpDir, '01-pay'); writePlan(phaseDir, '01-PLAN.md', '# Plan\nIntegrate the Stripe API.'); - const res = withFsThrow('readdirSync', new RegExp(phaseDir.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '$'), 'EACCES', () => + const res = withFsThrow('readdirSync', new RegExp(escapeRegex(phaseDir) + '$'), 'EACCES', () => readPhaseScope(tmpDir, phaseDir, '01')); assert.ok(res.readError, 'an unreadable phase directory must set readError, not read as empty'); }); diff --git a/tests/capability-lifecycle.test.cjs b/tests/capability-lifecycle.test.cjs index 28ce73138..1045effe2 100644 --- a/tests/capability-lifecycle.test.cjs +++ b/tests/capability-lifecycle.test.cjs @@ -20,6 +20,7 @@ const path = require('node:path'); const { cleanup } = require('./helpers.cjs'); const lifecycle = require('../gsd-core/bin/lib/capability-lifecycle.cjs'); const ledgerMod = require('../gsd-core/bin/lib/capability-ledger.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const { CAP_MARKER } = lifecycle; // --------------------------------------------------------------------------- @@ -3218,7 +3219,7 @@ test('IC-05/WIN-2: a consent-store write failure leaves the install status:insta assert.ok(fs.existsSync(path.join(dir, '.gsd', 'capabilities', 'rofs-cap', 'capability.json')), 'the bundle is committed on disk'); assert.match(buf, /capability consent:/i, 'a consent diagnostic was written to stderr'); assert.match(buf, /could not write the consent record/i, 'the warning explains the write failure'); - assert.match(buf, new RegExp(home.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')), 'the warning names the consent store path'); + assert.match(buf, new RegExp(escapeRegex(home)), 'the warning names the consent store path'); }); // --------------------------------------------------------------------------- diff --git a/tests/check-env.test.cjs b/tests/check-env.test.cjs index 0d585fc37..7582cc75e 100644 --- a/tests/check-env.test.cjs +++ b/tests/check-env.test.cjs @@ -222,7 +222,8 @@ describe('check-env.cjs', () => { } assert.equal(typeof parsed.pass, 'boolean', 'Live repo JSON must have boolean pass'); assert.ok(Array.isArray(parsed.checks), 'Live repo JSON must have checks array'); - // Node version check must be present and pass (Node >=22 is installed) + // Node version check must be present and pass (Node >=24 is installed, + // per package.json engines.node) const nodeCheck = parsed.checks.find((c) => c.name === 'node-version'); assert.ok(nodeCheck, 'node-version check must be present in live repo output'); assert.equal(nodeCheck.status, 'pass', `node-version should pass on live repo, got: ${nodeCheck.status} — ${nodeCheck.message}`); diff --git a/tests/code-review-agent-skills.test.cjs b/tests/code-review-agent-skills.test.cjs index a6e211c17..ebff77ae2 100644 --- a/tests/code-review-agent-skills.test.cjs +++ b/tests/code-review-agent-skills.test.cjs @@ -16,6 +16,8 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +const { escapeRegex: escapeRe } = require('../gsd-core/bin/lib/pattern.cjs'); + const WORKFLOWS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows'); // Workflow file -> EVERY subagent type it spawns and must inject skills for. @@ -29,10 +31,6 @@ const REVIEW_FAMILY = [ { file: 'eval-review.md', agentTypes: ['gsd-eval-auditor'] }, ]; -function escapeRe(s) { - return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - const cases = REVIEW_FAMILY.flatMap(({ file, agentTypes }) => agentTypes.map((agentType) => ({ file, agentType })), ); diff --git a/tests/code-review.test.cjs b/tests/code-review.test.cjs index dcdf77758..91215cd01 100644 --- a/tests/code-review.test.cjs +++ b/tests/code-review.test.cjs @@ -26,6 +26,7 @@ const fs = require('fs'); const path = require('path'); const os = require('os'); const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); // --- Test Environment Setup --- @@ -747,7 +748,7 @@ describe('bug-2839: /gsd-code-review-fix cleanup is transactional', () => { // (`rm -f .../.review-fix-recovery-pending.json`) or a shell-variable form // referring to the previously-declared `sentinel` variable // (`rm -f "$sentinel"` / `rm -f "${sentinel}"`). - const escapedName = SENTINEL_NAME.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const escapedName = escapeRegex(SENTINEL_NAME); const sentinelRemovalRe = new RegExp( `(rm\\s+(?:-f\\s+)?[^\\n]*(?:${escapedName}|\\$\\{?sentinel\\}?)|unlink[^\\n]*(?:${escapedName}|\\$\\{?sentinel\\}?))` ); diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index 702367a33..235ba83e4 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -23,6 +23,7 @@ const { throwIfFailed } = require('./helpers/git-fixture.cjs'); const { cleanup } = require('./helpers.cjs'); const fc = require('fast-check'); const { CLAUDE_AGENT_ALIASES } = require('../gsd-core/bin/lib/model-resolver.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); // #3241 — the intended new home for CLAUDE_AGENT_ALIASES + isAnthropicFlavoredModel // (see .gsd/phase/feat-3241-codex-omit-model-by-default/40-design.md "The seam // decision"). Neither export exists on model-catalog.cjs yet; requiring the @@ -1721,7 +1722,7 @@ describe('mergeCodexConfig', () => { assert.ok(!content.includes('Updated description'), 'no per-agent description in config.toml'); assert.ok(!content.includes('[agents.gsd-planner]'), 'no agent role table (canonical source is the standalone TOML)'); // Verify no duplicate markers - const markerCount = (content.match(new RegExp(GSD_CODEX_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g')) || []).length; + const markerCount = (content.match(new RegExp(escapeRegex(GSD_CODEX_MARKER), 'g')) || []).length; assert.strictEqual(markerCount, 1, 'exactly one marker'); }); @@ -1775,7 +1776,7 @@ describe('mergeCodexConfig', () => { // GSD section (stripLeakedGsdCodexSections) and the fresh block never // re-adds one (#2406) — zero role tables should remain anywhere. const gsdStructCount = (content.match(/^\[agents\.gsd-executor\]\s*$/gm) || []).length; - const markerCount = (content.match(new RegExp(GSD_CODEX_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g')) || []).length; + const markerCount = (content.match(new RegExp(escapeRegex(GSD_CODEX_MARKER), 'g')) || []).length; assert.ok(content.includes('[model]'), 'preserves user content'); assert.ok(content.includes('[agents.custom-agent]'), 'preserves non-GSD agent section'); @@ -1808,7 +1809,7 @@ describe('mergeCodexConfig', () => { assert.ok(content.includes('other_feature = true'), 'preserves user feature keys'); assert.ok(!content.includes('[agents.gsd-executor]'), 'no agent role table (canonical source is the standalone TOML)'); // Verify no duplicate markers - const markerCount = (content.match(new RegExp(GSD_CODEX_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g')) || []).length; + const markerCount = (content.match(new RegExp(escapeRegex(GSD_CODEX_MARKER), 'g')) || []).length; assert.strictEqual(markerCount, 1, 'exactly one marker'); }); @@ -2128,7 +2129,7 @@ describe('mergeCodexConfig trailing-content preservation (#2940)', () => { assert.ok(content.includes('[model]'), 'user [model] section preserved after re-merge'); assert.ok(content.includes('name = "gpt-5.4"'), 'user model value preserved verbatim'); assert.ok(content.includes(GSD_CODEX_MARKER), 'GSD marker still present'); - const markerCount = (content.match(new RegExp(GSD_CODEX_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g')) || []).length; + const markerCount = (content.match(new RegExp(escapeRegex(GSD_CODEX_MARKER), 'g')) || []).length; assert.strictEqual(markerCount, 1, 'exactly one marker (no duplication)'); assert.ok(content.includes('max_depth ='), 'GSD-managed [agents] block regenerated'); }); @@ -2237,7 +2238,7 @@ describe('mergeCodexConfig trailing-content preservation (#2940)', () => { const content = fs.readFileSync(configPath, 'utf8'); assert.ok(content.includes('[profiles.work]'), 'content before marker preserved'); assert.ok(content.includes('[mcp_servers.github]'), 'content after marker preserved'); - const markerCount = (content.match(new RegExp(GSD_CODEX_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g')) || []).length; + const markerCount = (content.match(new RegExp(escapeRegex(GSD_CODEX_MARKER), 'g')) || []).length; assert.strictEqual(markerCount, 1, 'exactly one marker'); }); @@ -4712,7 +4713,7 @@ describe('#2760 fix 4 — Write-failure rollback (atomic write + snapshot restor const preInstallBytes = fs.readFileSync(path.join(codexHome, 'config.toml')); const configPath = path.join(codexHome, 'config.toml'); - const tempPattern = new RegExp('^' + configPath.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '\\.tmp-'); + const tempPattern = new RegExp('^' + escapeRegex(configPath) + '\\.tmp-'); // Stub: allow writes to atomic temp files (which renameSync overwrites // the target, never truncating it directly) but throw on any direct @@ -4778,7 +4779,7 @@ describe('#2760 fix 4 — Write-failure rollback (atomic write + snapshot restor const preInstallBytes = fs.readFileSync(path.join(codexHome, 'config.toml')); const configPath = path.join(codexHome, 'config.toml'); - const tempPattern = new RegExp('^' + configPath.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '\\.tmp-'); + const tempPattern = new RegExp('^' + escapeRegex(configPath) + '\\.tmp-'); // Stub: fault writes targeting the atomic temp file (the pre-rename branch // of atomicWriteFileSync). Other writes (agent .toml files in CODEX_HOME) diff --git a/tests/config-schema.property.test.cjs b/tests/config-schema.property.test.cjs index 403b33e96..3b7767853 100644 --- a/tests/config-schema.property.test.cjs +++ b/tests/config-schema.property.test.cjs @@ -31,6 +31,7 @@ const { VALID_CONFIG_KEYS, RUNTIME_STATE_KEYS, } = require('../gsd-core/bin/lib/config-schema.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); describe('config-schema: isValidConfigKey properties', () => { // (a) Never throws on any input @@ -487,7 +488,7 @@ const SECTION_HEADERS = ['Planning', 'Execution', 'Docs & Output', 'Features', ' function hasPathLike(block, field) { const parts = field.split('.'); if (parts.length === 1) return block.includes(parts[0]); - const escaped = parts.map((p) => p.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')); + const escaped = parts.map((p) => escapeRegex(p)); const pattern = new RegExp(escaped.join('[\\s\\S]{0,600}'), 'i'); return pattern.test(block); } diff --git a/tests/cursor-hook-workspace-roots.test.cjs b/tests/cursor-hook-workspace-roots.test.cjs index 089421772..6d45e5eb3 100644 --- a/tests/cursor-hook-workspace-roots.test.cjs +++ b/tests/cursor-hook-workspace-roots.test.cjs @@ -32,6 +32,7 @@ const path = require('node:path'); const { execFileSync } = require('node:child_process'); const { createTempDir, cleanup } = require('./helpers.cjs'); const { runHook: runHookSeam } = require('./helpers/process-seam.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const HOOKS = path.join(__dirname, '..', 'hooks'); const SESSION_START = path.join(HOOKS, 'gsd-cursor-session-start.js'); @@ -85,7 +86,7 @@ describe('#2587: cursor hooks resolve the workspace from workspace_roots, not cw }); assert.match( out.additional_context || '', - new RegExp(MSG_PRESENT_FRAGMENT.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')), + new RegExp(escapeRegex(MSG_PRESENT_FRAGMENT)), 'must report STATE.md present when workspace_roots points at the project', ); } finally { diff --git a/tests/docs-parity-live-registry.test.cjs b/tests/docs-parity-live-registry.test.cjs index 85ed92d32..d8a8d331a 100644 --- a/tests/docs-parity-live-registry.test.cjs +++ b/tests/docs-parity-live-registry.test.cjs @@ -231,9 +231,12 @@ function extractCommandTokens(content) { /** * Walk a directory and return all .md files recursively. - * Uses hand-rolled DFS for Node 20 compat (Node 22+ recursive readdirSync is - * not available in all CI matrix entries). Surfaces permission-denied errors - * as structured warnings (PRED.k302) rather than silently skipping. + * Uses hand-rolled DFS rather than `fs.readdirSync(dir, { recursive: true })` + * — the recursive option is available on every CI matrix entry now that the + * Node floor is >=24.0.0 (it landed in Node 20.1), but this walker predates + * that bump and was not rewritten as part of it; unchanged, not stale. + * Surfaces permission-denied errors as structured warnings (PRED.k302) rather + * than silently skipping. */ function listMdFiles(dir) { if (!fs.existsSync(dir)) return []; diff --git a/tests/emitted-attribution.test.cjs b/tests/emitted-attribution.test.cjs index f352d1ae9..14121d3ef 100644 --- a/tests/emitted-attribution.test.cjs +++ b/tests/emitted-attribution.test.cjs @@ -41,6 +41,7 @@ const fc = require('fast-check'); const { cleanup, createTempDir } = require('./helpers.cjs'); const { BUILD_SCRIPT, buildParityManifest, buildInstallTree, PKG_VERSION } = require('./helpers/install-shared.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const { resolveChangedPaths, resolveBase, @@ -891,7 +892,7 @@ test('listFragmentFiles: exactly MAX_ACK_FRAGMENTS entries passes, one over fail assert.throws( () => listFragmentFiles(dir), (err) => { - assert.match(err.message, new RegExp(dir.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))); + assert.match(err.message, new RegExp(escapeRegex(dir))); assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS + 1))); assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS))); return true; @@ -1233,7 +1234,7 @@ test('listAckFragmentFiles: exactly MAX_ACK_FRAGMENTS entries passes, one over f assert.throws( () => listAckFragmentFiles(dir), (err) => { - assert.match(err.message, new RegExp(dir.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))); + assert.match(err.message, new RegExp(escapeRegex(dir))); assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS + 1))); assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS))); return true; @@ -1262,7 +1263,7 @@ test('listAckFragmentFilesAtRef: exactly MAX_ACK_FRAGMENTS entries passes, one o assert.throws( () => listAckFragmentFilesAtRef(SHA_A, { run: overCap }), (err) => { - assert.match(err.message, new RegExp(`${ACK_DIR_REPO_PATH}/ at "${SHA_A}"`.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))); + assert.match(err.message, new RegExp(escapeRegex(`${ACK_DIR_REPO_PATH}/ at "${SHA_A}"`))); assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS + 1))); assert.match(err.message, new RegExp(String(MAX_ACK_FRAGMENTS))); return true; diff --git a/tests/eslint-no-adhoc-regex-escape.test.cjs b/tests/eslint-no-adhoc-regex-escape.test.cjs new file mode 100644 index 000000000..ebf038d8a --- /dev/null +++ b/tests/eslint-no-adhoc-regex-escape.test.cjs @@ -0,0 +1,306 @@ +'use strict'; + +/** + * RuleTester unit tests for `local/no-adhoc-regex-escape` + * (#3212 Phase 1, #3412). + * + * Design: .gsd/phase/chore-3412-pattern-seam/40-design.md (Negative space) + * Test matrix: .gsd/phase/chore-3412-pattern-seam/50-test-matrix.md — section 5, rows 24-29 + * ADR: docs/adr/3212-lexical-seam-consolidation.md §7 + * + * TDD RED: `eslint-rules/no-adhoc-regex-escape.cjs` does not exist yet — + * this file's require() throws MODULE_NOT_FOUND until the implementing phase + * adds it. That is the intended starting state. + * + * RuleTester setup (languageOptions, sourceType, filename conventions) mirrors + * tests/eslint-rules.test.cjs's `no-adhoc-markdown-parsing` suite, which is + * this repo's other src/*.cts-scoped structural-shape rule. + * + * Contract this test file locks for the not-yet-written rule (per ADR §7 and + * the design doc's Negative space section): + * - FIRES on an inline `.replace(, '\$&')` + * shape outside src/pattern.cts, matching the SHAPE (character-class + * membership set) not exact byte order (row 25). + * - Does NOT fire inside src/pattern.cts itself — the seam is the owner + * (row 26). + * - Does NOT fire on `new RegExp()` when the identifier is a + * `*_SOURCE`-suffixed constant — the design's named provenance marker for + * "deliberate, reviewed pattern fragments" (row 27). + * - Does NOT fire on a regex literal that merely contains the escape class + * as DATA, with no `.replace(..., '\$&')` shape present (row 28). + * - FIRES on `new RegExp()` when the identifier is a plain + * runtime value with no `_SOURCE` provenance marker and no routing + * through escapeRegex/literalPattern — ADR §7's "new RegExp() built from + * a non-literal without routing through the seam" (row 29). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const { RuleTester } = require('eslint'); + +const noAdhocRegexEscape = require('../eslint-rules/no-adhoc-regex-escape.cjs'); +const { findViolations } = require('../scripts/lint-no-adhoc-regex-escape.cjs'); + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'commonjs', + }, +}); + +// Separate instance for the ES-module `import` binding case (row 27b) — the +// default instance above uses sourceType 'commonjs', which cannot parse +// `import` statements. +const moduleRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'module', + }, +}); + +describe('no-adhoc-regex-escape rule', () => { + test('rule module exports a create function', () => { + assert.strictEqual(typeof noAdhocRegexEscape.create, 'function'); + }); + + // ── row 24: invalid — a new inline escape-all-metachars .replace() ──────── + + test('row 24 invalid: a new inline .replace(/[.*+?^${}()|[\\]\\\\]/g, \'\\\\$&\') outside the seam', () => { + ruleTester.run('no-adhoc-regex-escape', noAdhocRegexEscape, { + valid: [], + invalid: [ + { + // NOT String.raw: the code-under-test contains a literal `${` (part + // of the escape-all-metachars character class), which String.raw + // would still interpret as a template-literal substitution marker. + // A normal (non-raw) template literal is used instead, with `\$` + // and doubled backslashes to produce the exact literal source text. + code: ` + function esc(s) { + return s.replace(/[.*+?^\${}()|[\\]\\\\]/g, '\\\\$&'); + } + `, + filename: 'src/some-module.cts', + errors: [{ messageId: 'adhocRegexEscape' }], + }, + ], + }); + }); + + // ── row 25: invalid — reordered/differently-escaped char class, same shape ─ + + test('row 25 invalid: same escape shape with a REORDERED character class still fires (matches shape, not exact bytes)', () => { + // Same 14-member set { . * + ? ^ $ { } ( ) | [ ] \ } as row 24, but written + // in a different order and with a different (still valid) escaping style. + // If the rule only string-matched the exact literal bytes of the census + // copies, this would slip through — it must not. + ruleTester.run('no-adhoc-regex-escape', noAdhocRegexEscape, { + valid: [], + invalid: [ + { + code: String.raw` + function escapeForRegex(value) { + return value.replace(/[\^$.|?*+()[\]{}\\]/g, '\\$&'); + } + `, + filename: 'src/another-module.cts', + errors: [{ messageId: 'adhocRegexEscape' }], + }, + ], + }); + }); + + // ── row 26: valid — the seam file itself is exempt ───────────────────────── + + test('row 26 valid: src/pattern.cts itself is exempt (the seam owns this shape)', () => { + ruleTester.run('no-adhoc-regex-escape', noAdhocRegexEscape, { + valid: [ + { + // Not String.raw — see the row-24 comment above (`${` needs escaping + // in a non-raw template literal). + code: ` + function escapeRegex(value) { + return value.replace(/[.*+?^\${}()|[\\]\\\\]/g, '\\\\$&'); + } + `, + filename: 'src/pattern.cts', + }, + ], + invalid: [], + }); + }); + + // ── row 27: valid (negative space) — new RegExp from an exported *_SOURCE ── + + test('row 27 valid: new RegExp built from an exported *_SOURCE constant is NOT flagged (critical negative-space case)', () => { + ruleTester.run('no-adhoc-regex-escape', noAdhocRegexEscape, { + valid: [ + { + // A deliberate, reviewed pattern fragment (design doc Negative space: + // PHASE_NUMBER_TOKEN_SOURCE, MILESTONE_HEADING_LINE_SOURCE, etc.) — + // its metacharacters are load-bearing and must not be treated as an + // unescaped runtime value. + code: String.raw` + const PHASE_NUMBER_TOKEN_SOURCE = '\\d+[A-Z]?'; + const re = new RegExp(PHASE_NUMBER_TOKEN_SOURCE); + `, + filename: 'src/phase-id.cts', + }, + { + code: String.raw` + const MILESTONE_HEADING_LINE_SOURCE = '^##\\s+Milestone\\s'; + const re = new RegExp(MILESTONE_HEADING_LINE_SOURCE, 'm'); + `, + filename: 'src/roadmap-parser.cts', + }, + ], + invalid: [], + }); + }); + + // ── row 28: valid (negative space) — regex literal containing the class as data ─ + + test('row 28 valid: a regex literal that merely CONTAINS [.*+?] as data is NOT flagged', () => { + // A legitimate validator matching one of the metacharacters as literal + // DATA — no .replace(..., '\$&') shape present, so this must not fire. + ruleTester.run('no-adhoc-regex-escape', noAdhocRegexEscape, { + valid: [ + { + code: String.raw`const isMetachar = /^[.*+?]$/.test(char);`, + filename: 'src/some-validator.cts', + }, + ], + invalid: [], + }); + }); + + // ── #3412 Standards-review Finding 1: _SOURCE naming-only evasion closed ── + // A rule that trusts the `_SOURCE` suffix on spelling alone (no + // scope/binding check) is exactly #3410's guard-evasion class: naming a + // genuinely dynamic value with a matching suffix defeats it. These + // cases lock that the fallback is now bound to the identifier's ACTUAL + // BINDING KIND (import / require()-derived const), not its name. + + test('Finding 1 invalid: a function-parameter identifier named *_SOURCE is NOT exempted by naming alone', () => { + ruleTester.run('no-adhoc-regex-escape', noAdhocRegexEscape, { + valid: [], + invalid: [ + { + // The evasion: naming a plain, unescaped function parameter with + // the `_SOURCE` suffix used to sail past condition (b) because + // that check was pure identifier-name regex matching with no + // scope/binding verification. It must fire now. + code: String.raw` + function f(userInput_SOURCE) { + return new RegExp(userInput_SOURCE); + } + `, + filename: 'src/some-module.cts', + errors: [{ messageId: 'unsafeNewRegExp' }], + }, + ], + }); + }); + + test('Finding 1 invalid: a reassigned let identifier named *_SOURCE is NOT exempted by naming alone', () => { + ruleTester.run('no-adhoc-regex-escape', noAdhocRegexEscape, { + valid: [], + invalid: [ + { + // A `let` binding holding genuinely dynamic content (not a + // static const, not an import, not a require()-derived const) — + // the suffix alone must not exempt it. + code: String.raw` + let mutable_SOURCE = getUserInput(); + const re = new RegExp(mutable_SOURCE); + `, + filename: 'src/some-module.cts', + errors: [{ messageId: 'unsafeNewRegExp' }], + }, + ], + }); + }); + + test('Finding 1 valid: a require()-derived *_SOURCE const (destructured) still passes', () => { + // The legitimate cross-module case condition (b) exists to cover: + // `const { X_SOURCE } = require('./mod.cjs')`. Proves the tightened + // check did not regress this repo's dominant CJS import shape. + ruleTester.run('no-adhoc-regex-escape', noAdhocRegexEscape, { + valid: [ + { + code: String.raw` + const { PHASE_NUMBER_TOKEN_SOURCE } = require('./phase-id.cjs'); + const re = new RegExp(PHASE_NUMBER_TOKEN_SOURCE); + `, + filename: 'src/roadmap-parser.cts', + }, + { + // The other require()-derived shape condition (b) covers: + // `const X_SOURCE = require('./mod.cjs').X_SOURCE`. + code: String.raw` + const PHASE_NUMBER_TOKEN_SOURCE = require('./phase-id.cjs').PHASE_NUMBER_TOKEN_SOURCE; + const re = new RegExp(PHASE_NUMBER_TOKEN_SOURCE); + `, + filename: 'src/roadmap-parser.cts', + }, + ], + invalid: [], + }); + }); + + test('Finding 1 valid: an ES `import`-derived *_SOURCE binding still passes', () => { + moduleRuleTester.run('no-adhoc-regex-escape', noAdhocRegexEscape, { + valid: [ + { + code: String.raw` + import { PHASE_NUMBER_TOKEN_SOURCE } from './phase-id.cts'; + const re = new RegExp(PHASE_NUMBER_TOKEN_SOURCE); + `, + filename: 'src/roadmap-parser.cts', + }, + ], + invalid: [], + }); + }); + + // ── row 29: invalid — new RegExp from a genuinely unescaped runtime value ── + + test('row 29 invalid: new RegExp from a genuinely unescaped runtime value fires', () => { + // A plain, non-`_SOURCE`-suffixed parameter interpolated straight into + // new RegExp() with no seam routing (no escapeRegex/literalPattern call + // anywhere on it) — ADR §7: "flags new RegExp() built from a non-literal + // without routing through the seam." This is the minimal-pair inverse of + // row 27: same `new RegExp()` shape, but the identifier + // carries no `_SOURCE` provenance marker. + ruleTester.run('no-adhoc-regex-escape', noAdhocRegexEscape, { + valid: [], + invalid: [ + { + code: String.raw` + function buildLiteralMatcher(userSuppliedValue) { + return new RegExp(userSuppliedValue); + } + `, + filename: 'src/some-module.cts', + errors: [{ messageId: 'unsafeNewRegExp' }], + }, + ], + }); + }); + + // ── ReDoS regression — scripts/lint-no-adhoc-regex-escape.cjs's own regex ─ + + test('an adversarial [] run after .replace(/ terminates instead of backtracking exponentially (#3412)', () => { + // Pre-fix, REPLACE_CALL_RE's outer alternation let a `[...]` run be + // consumed either by the character-class branch or one char at a time + // by the catch-all branch, so a failing match explored both parses of + // every bracket pair: measured n=30 -> 3475ms (~2^n growth per +2). This + // adversarial input never closes the `.replace(/` call (no trailing + // `, '...')`), forcing the engine all the way through backtracking on a + // pre-fix regex. n=2000 is comfortably past where the pre-fix regex + // would already be unusable; a linear-time regex resolves it instantly. + const adversarial = '.replace(/' + '[]'.repeat(2000) + 'X'; + const result = findViolations(adversarial); + assert.deepEqual(result, []); + }); +}); diff --git a/tests/helpers/copy-script-fixture.cjs b/tests/helpers/copy-script-fixture.cjs new file mode 100644 index 000000000..5b0cdb8df --- /dev/null +++ b/tests/helpers/copy-script-fixture.cjs @@ -0,0 +1,265 @@ +'use strict'; + +/** + * Copy a repo script into a throwaway fixture tree ALONG WITH its transitive + * relative-require dependencies, preserving repo-relative layout. + * + * Several suites drive a `scripts/*.cjs` end-to-end by copying it into an + * mkdtemp fixture and spawning it there — necessary because those scripts + * resolve their scan root from `path.join(__dirname, '..')`, so running the + * REAL script would scan the real repo instead of the fixture (see + * tests/removed-but-needed-lint.test.cjs's copyScriptInto for the original + * statement of that constraint). + * + * Each such harness used to hand-list the script's dependencies + * (`fs.copyFileSync(... 'scripts/lib/cli-exit.cjs' ...)`). That made every new + * require in a covered script a silent, duplicated edit across N harnesses, + * and the failure mode was a MODULE_NOT_FOUND that only ever appeared in CI. + * #3412 collected the bill: adding one require of the new pattern seam to + * gen-adr-index.cjs broke 82 tests across two suites that had each hand-copied + * a now-incomplete dependency list. + * + * Walking the require graph instead means a script's dependencies are derived, + * never re-declared, so this class cannot recur. + */ + +const fs = require('node:fs'); +const path = require('node:path'); +const espree = require('espree'); + +/** Relative specifiers — the only kind that resolves inside the fixture tree. */ +const RELATIVE_SPEC_RE = /^\.{1,2}[\\/]/; + +/** + * Parse attempts tried in order by `extractRequires`. `globalReturn: true` is + * required on the `script` attempt because Node wraps every CommonJS module + * body in an implicit function, which makes a top-level `return` legal there + * even though it is not legal in a bare ECMAScript Program — `espree` without + * the flag rejects it with "'return' outside of function". A real shipped + * script relies on exactly this (`scripts/check-coverage-gate.cjs` has a + * top-level `return`), so dropping the flag silently breaks the #2858 + * packaging guard that scans it. Do not remove this without re-verifying + * every shipped `scripts/**`, `bin/**`, and `gsd-core/bin/**` .cjs/.js file + * still parses. + */ +const PARSE_ATTEMPTS = [ + { ecmaVersion: 'latest', sourceType: 'script', ecmaFeatures: { globalReturn: true } }, + { ecmaVersion: 'latest', sourceType: 'module' }, +]; + +/** + * Extract every static `require('...')` string-literal call from a CJS or ESM + * source, via a real AST parse rather than pattern-matching. Commented-out, + * string-embedded, and template-literal occurrences are correctly ignored; + * `foo.require('x')` (a method call, not a bare identifier callee) is + * correctly ignored; dynamic `require(variable)` remains out of scope — it + * cannot be resolved statically, and in a script that ships it would itself + * be a red flag (see tests/packaging-shipped-scripts-require-only-shipped. + * test.cjs, which consumes this same extractor so the two guards cannot + * disagree about what "a require" is). + * + * Currently a spurious hit (a require-shaped call this function reports that + * the source does not actually reach) is not merely harmless: an unresolvable + * specifier makes `copyScriptWithDeps` throw. With a real parser this is + * largely moot — there is no pattern-matching left to produce a false + * positive from comment/string/regex content. + * + * @param {string} source + * @returns {string[]} specifiers, in source order, duplicates included + */ +function extractRequires(source) { + let ast = null; + for (const options of PARSE_ATTEMPTS) { + try { + ast = espree.parse(source, options); + break; + } catch { + /* try next */ + } + } + if (ast === null) { + throw new Error('extractRequires: source did not parse as script or module'); + } + const found = []; + (function walk(node) { + if (node === null || typeof node !== 'object') return; + if (Array.isArray(node)) { + node.forEach(walk); + return; + } + if ( + node.type === 'CallExpression' && + node.callee && + node.callee.type === 'Identifier' && + node.callee.name === 'require' && + node.arguments.length === 1 && + node.arguments[0].type === 'Literal' && + typeof node.arguments[0].value === 'string' + ) { + found.push(node.arguments[0].value); + } + for (const key of Object.keys(node)) walk(node[key]); + })(ast); + return found; +} + +/** + * Resolve a relative require specifier to a real file, applying Node's CJS + * extension/index candidates. + * + * @param {string} fromAbsFile absolute path of the requiring file + * @param {string} spec the relative specifier + * @returns {string|null} absolute path of the resolved file, or null + */ +function resolveRelativeRequire(fromAbsFile, spec) { + const base = path.resolve(path.dirname(fromAbsFile), spec); + const candidates = [ + base, + `${base}.cjs`, + `${base}.js`, + `${base}.json`, + path.join(base, 'index.cjs'), + path.join(base, 'index.js'), + ]; + for (const candidate of candidates) { + if (fs.existsSync(candidate) && fs.statSync(candidate).isFile()) return candidate; + } + return null; +} + +/** + * True when `rel` (a `path.relative(base, target)` result) climbs outside + * `base` — i.e. `target` is not contained within `base`. Guards against the + * `'..foo'.startsWith('..')` false positive (a real sibling directory named + * `..foo` is NOT an escape) by requiring either an exact `..` or a + * `..`-prefixed relative path, using the platform separator since + * `path.relative` returns platform-native separators. + * + * @param {string} rel + * @returns {boolean} + */ +function escapesContainment(rel) { + return rel === '..' || rel.startsWith(`..${path.sep}`) || path.isAbsolute(rel); +} + +/** + * Copy `scriptRel` (repo-relative, e.g. `scripts/gen-adr-index.cjs`) and every + * file reachable from it through static relative requires into `fixtureRoot`, + * at the same repo-relative paths. Bare specifiers (`node:fs`, npm packages) + * are left alone — they resolve from the real installation. + * + * Both the entry script and every discovered dependency are validated against + * the repo boundary before being copied (symlinks resolved via + * `fs.realpathSync` first, since the containment check is otherwise lexical + * while `copyFileSync`/`readFileSync` follow links — a repo-committed symlink + * pointing outside the repo must not smuggle its target's contents in). The + * ORIGINAL repo-relative path (not the realpath-derived one) is preserved for + * the destination location, so the copied layout is unchanged; the + * realpath-derived repo-relative path is used as the dedupe key so a + * directory-symlink cycle cannot mint a new key per level. + * + * Throws when a relative require does not resolve on disk, or when the entry + * or a dependency resolves outside the repo. Unresolved-require failures are + * nearly always an unbuilt artifact (`gsd-core/bin/lib/*.cjs` requires + * `npm run build:lib`), and failing here names the cause instead of letting + * the spawned subprocess die with a bare MODULE_NOT_FOUND. + * + * @param {string} repoRoot absolute path to the repo root + * @param {string} fixtureRoot absolute path to the temp fixture root + * @param {string} scriptRel repo-relative path of the script to copy + * @returns {string} absolute path of the copied script inside `fixtureRoot` + */ +function copyScriptWithDeps(repoRoot, fixtureRoot, scriptRel) { + const toPosix = (p) => p.split(path.sep).join('/'); + const entry = toPosix(scriptRel); + const copied = new Set(); + const unresolved = []; + + let realRepoRoot; + try { + realRepoRoot = fs.realpathSync(repoRoot); + } catch (err) { + throw new Error(`copyScriptWithDeps: could not resolve repoRoot ${repoRoot}: ${err.message}`); + } + + /** + * @param {string} abs absolute path (not yet realpath-resolved) + * @param {string} label human-readable label for error messages + * @returns {{ dedupeKey: string } | null} null when unresolvable or escaping + */ + function checkContainment(abs, label) { + let real; + try { + real = fs.realpathSync(abs); + } catch (err) { + unresolved.push(`${label} could not be resolved on disk: ${err.message}`); + return null; + } + const dedupeKey = toPosix(path.relative(realRepoRoot, real)); + if (escapesContainment(dedupeKey)) { + unresolved.push(`${label} resolves outside the repo (${real}) — refusing to copy outside the fixture`); + return null; + } + return { dedupeKey }; + } + + /** @param {string} rel repo-relative posix path (ORIGINAL, pre-realpath) */ + function copyOne(rel) { + const abs = path.join(repoRoot, rel); + if (!fs.existsSync(abs)) { + if (!copied.has(rel)) { + copied.add(rel); + unresolved.push(`${rel} (does not exist in the repo)`); + } + return; + } + + const containment = checkContainment(abs, rel); + if (!containment) return; + if (copied.has(containment.dedupeKey)) return; + copied.add(containment.dedupeKey); + + const dest = path.join(fixtureRoot, rel); + fs.mkdirSync(path.dirname(dest), { recursive: true }); + fs.copyFileSync(abs, dest); + + for (const spec of extractRequires(fs.readFileSync(abs, 'utf-8'))) { + if (!RELATIVE_SPEC_RE.test(spec)) continue; + const depAbs = resolveRelativeRequire(abs, spec); + if (!depAbs) { + unresolved.push(`require('${spec}') from ${rel}`); + continue; + } + const depRel = toPosix(path.relative(repoRoot, depAbs)); + copyOne(depRel); + } + } + + // F2: the entry path bypasses the sandbox guard the same way a dependency + // could — validate it against the same containment rule before copying. + const entryAbs = path.join(repoRoot, entry); + if (!fs.existsSync(entryAbs)) { + throw new Error(`copyScriptWithDeps: entry script does not exist in the repo: ${entry}`); + } + const entryContainment = checkContainment(entryAbs, `entry script ${entry}`); + if (!entryContainment) { + throw new Error( + `copyScriptWithDeps: entry script ${entry} resolves outside the repo — refusing to copy outside the fixture`, + ); + } + + copyOne(entry); + + if (unresolved.length > 0) { + throw new Error( + `copyScriptWithDeps(${entry}) could not resolve ${unresolved.length} ` + + `relative require(s); the fixture would fail with MODULE_NOT_FOUND:\n` + + unresolved.map((u) => ` - ${u}`).join('\n') + + `\nIf these are compiled artifacts under gsd-core/bin/lib/, run \`npm run build:lib\`.`, + ); + } + + return path.join(fixtureRoot, scriptRel); +} + +module.exports = { extractRequires, resolveRelativeRequire, copyScriptWithDeps }; diff --git a/tests/helpers/install-shared.cjs b/tests/helpers/install-shared.cjs index 75cfbc211..299e7f548 100644 --- a/tests/helpers/install-shared.cjs +++ b/tests/helpers/install-shared.cjs @@ -24,6 +24,7 @@ const { runNode } = require('./process-seam.cjs'); const { resolveRuntimeArtifactLayout, } = require('../../gsd-core/bin/lib/runtime-artifact-layout.cjs'); +const { escapeRegex: escapeRegExp } = require('../../gsd-core/bin/lib/pattern.cjs'); const INSTALL_SCRIPT = path.join(__dirname, '..', '..', 'bin', 'install.js'); const MANIFEST_NAME = 'gsd-file-manifest.json'; @@ -227,9 +228,6 @@ function stripAnsi(str) { // A version string can itself contain regex metacharacters (`.`, and — via // prerelease/build metadata — `-`/`+`), so it must be escaped before being spliced // into a RegExp source, or e.g. the `.` in "1.9.0" would match ANY character. -function escapeRegExp(str) { - return str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} // Loosely semver-shaped: leading `MAJOR.MINOR.PATCH`, optional `-prerelease` and/or // `+build` metadata (e.g. `1.9.0`, `1.9.0-rc.1`, `1.9.0+abc`). Deliberately loose diff --git a/tests/install-runtime-artifacts.test.cjs b/tests/install-runtime-artifacts.test.cjs index 96ba6a6e7..21e81efb2 100644 --- a/tests/install-runtime-artifacts.test.cjs +++ b/tests/install-runtime-artifacts.test.cjs @@ -42,6 +42,7 @@ const { } = require('../gsd-core/bin/lib/runtime-artifact-layout.cjs'); const { applySurface } = require('../gsd-core/bin/lib/surface.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const { loadSkillsManifest, @@ -433,7 +434,7 @@ describe('installOpencodeFamilySkills — emits skills//SKILL.md (#784)', ); // Regression guard for the prefix-overlap double-rewrite (e.g. kilo-alt-alt). assert.ok( - !new RegExp(`${defaultBase.replace(/[\\.*+?^${}()|[\]]/g, '\\$&')}-[^/\\s]*-`).test(body), + !new RegExp(`${escapeRegex(defaultBase)}-[^/\\s]*-`).test(body), `${skillName}: must not contain a doubled config-dir suffix`, ); } diff --git a/tests/no-unbounded-spawn-allowlist.test.cjs b/tests/no-unbounded-spawn-allowlist.test.cjs index 78f14aae1..61023d872 100644 --- a/tests/no-unbounded-spawn-allowlist.test.cjs +++ b/tests/no-unbounded-spawn-allowlist.test.cjs @@ -49,7 +49,7 @@ const REPO_ROOT = path.join(__dirname, '..'); * Recursively list every `.cjs` file under `tests/`, including subdirectories * (`tests/helpers/`, `tests/qa/`, `tests/observability/`, `tests/fixtures/`, * `tests/dispatch/`, etc.). `fs.readdirSync(dir, { recursive: true, - * withFileTypes: true })` is available on the repo's Node floor (>=22.0.0 per + * withFileTypes: true })` is available on the repo's Node floor (>=24.0.0 per * package.json `engines`; the option landed in Node 20.1). Each returned * `Dirent` carries `parentPath` — its containing directory, which for a * nested entry is the subdirectory, not `TESTS_DIR` — so the joined path is diff --git a/tests/packaging-shipped-scripts-require-only-shipped.test.cjs b/tests/packaging-shipped-scripts-require-only-shipped.test.cjs index 774a3d4df..fa329a6ad 100644 --- a/tests/packaging-shipped-scripts-require-only-shipped.test.cjs +++ b/tests/packaging-shipped-scripts-require-only-shipped.test.cjs @@ -21,6 +21,8 @@ const { execFileSync } = require('node:child_process'); const REPO_ROOT = path.join(__dirname, '..'); +const { extractRequires } = require('./helpers/copy-script-fixture.cjs'); + /** * Resolve the tarball file list via `npm pack --dry-run --json`. * This is the ACTUAL set of files that ship — not a hardcoded list — so adding @@ -47,27 +49,6 @@ function resolveTarballFiles() { return new Set(parsed[0].files.map((f) => f.path.replace(/\\/g, '/'))); } -/** - * Extract all require('...') string-literal calls from a .cjs source. - * Only static string-literal requires are checked — dynamic require(variable) - * is out of scope (and would itself be a red flag in a shipped script). - */ -function extractRequires(source) { - const requires = []; - // Strip block comments (/* ... */) and inline line comments (// ...) before - // matching, so a require() appearing in a doc comment or fenced code block - // inside a /* */ does not produce a false positive. - const stripped = source - .replace(/\/\*[\s\S]*?\*\//g, '') - .replace(/\/\/.*$/gm, ''); - const requireRe = /\brequire\s*\(\s*['"]([^'"]+)['"]\s*\)/g; - let m; - while ((m = requireRe.exec(stripped)) !== null) { - requires.push(m[1]); - } - return requires; -} - /** * Classify a require specifier from a given file path: * - 'builtin' — node: prefix or a Node builtin (fs, path, etc.) @@ -198,6 +179,15 @@ describe('#2858 — shipped scripts require only shipped paths', () => { ); }); + test('lint-no-adhoc-regex-escape.cjs does NOT ship (repo-only CI tooling)', () => { + // This script requires ../eslint-rules/ which does not ship. It is + // repo-only CI tooling. It must be excluded from the npm tarball (#3412). + assert.ok( + !shippedFiles.has('scripts/lint-no-adhoc-regex-escape.cjs'), + 'scripts/lint-no-adhoc-regex-escape.cjs must NOT ship — it requires ../eslint-rules/ which does not ship (#3412)', + ); + }); + test('a script requiring a sibling in the same shipped dir resolves as shipped (positive case)', () => { // Sanity: scripts/lib/cli-exit.cjs ships and is required by shipped scripts. // This confirms the guard's positive path works — a valid intra-shipped require diff --git a/tests/pattern.test.cjs b/tests/pattern.test.cjs new file mode 100644 index 000000000..ccdbd6852 --- /dev/null +++ b/tests/pattern.test.cjs @@ -0,0 +1,204 @@ +'use strict'; + +/** + * Tests for `src/pattern.cts` — the pattern-construction seam (#3212 Phase 1, #3412). + * + * Design: .gsd/phase/chore-3412-pattern-seam/40-design.md + * Test matrix: .gsd/phase/chore-3412-pattern-seam/50-test-matrix.md + * ADR: docs/adr/3212-lexical-seam-consolidation.md §1, §2, §7 + * + * TDD RED: `src/pattern.cts` does not exist yet — this file's + * `require('../gsd-core/bin/lib/pattern.cjs')` throws MODULE_NOT_FOUND until + * the implementing phase adds it. That is the intended starting state (mirrors + * tests/planning-snapshot.test.cjs's RED convention). + * + * Covers test-matrix sections 1 (escapeRegex, rows 1-10), 2 (literalPattern, + * rows 11-14), and 3 (migration equivalence, rows 15-17). + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +// Seeded fast-check convention: require the shared setup helper (NOT +// 'fast-check' directly) so numRuns/seed are configured globally before any +// fc.assert() call — mirrors tests/frontmatter.property.test.cjs and every +// other *.property.test.cjs file. seed: 42, overridable via GSD_FC_SEED. +const fc = require('./helpers/fast-check-setup.cjs'); + +const { escapeRegex, literalPattern } = require('../gsd-core/bin/lib/pattern.cjs'); + +// ─── Section 1: escapeRegex — rows 1-10 ─────────────────────────────────── + +describe('escapeRegex', () => { + test('row 1: "" returns ""', () => { + assert.strictEqual(escapeRegex(''), ''); + }); + + test('row 2: ordinary chars only ("_x") are unchanged', () => { + assert.strictEqual(escapeRegex('_x'), '_x'); + }); + + test('row 3: each metacharacter is escaped and the result matches its own literal form', () => { + const metachars = ['.', '*', '+', '?', '^', '$', '{', '}', '(', ')', '|', '[', ']', '\\']; + for (const ch of metachars) { + const escaped = escapeRegex(ch); + const re = new RegExp(escaped); + assert.ok(re.test(ch), `escapeRegex(${JSON.stringify(ch)}) -> ${JSON.stringify(escaped)} must match its own literal char`); + } + }); + + test('row 4: leading ASCII letter locks the MEASURED hex-escape ("abc" -> "\\x61bc", Node v26.5.1)', () => { + assert.strictEqual(escapeRegex('abc'), '\\x61bc'); + }); + + test('row 5: leading digit locks the MEASURED hex-escape ("5abc" -> "\\x35abc", Node v26.5.1)', () => { + assert.strictEqual(escapeRegex('5abc'), '\\x35abc'); + }); + + test('row 6: hyphen anywhere locks the MEASURED hex-escape ("a-b" -> "\\x61\\x2db", Node v26.5.1)', () => { + assert.strictEqual(escapeRegex('a-b'), '\\x61\\x2db'); + }); + + test('row 7: whitespace / newline / control chars are escaped and the result still constructs', () => { + const values = [' ', '\n', '\t', '\r', 'a\nb', 'a\tb\rc']; + for (const v of values) { + const escaped = escapeRegex(v); + assert.doesNotThrow(() => new RegExp(escaped)); + assert.ok(new RegExp(escaped).test(v)); + } + }); + + // #3212 Spec-axis review correction: this throw behavior matches MOST of the + // deleted copies' `.replace` throw, but NOT ALL of them — `src/phase-id.cts`'s + // original `escapeRegex(value: unknown)` did `String(value).replace(...)` and + // never threw (`escapeRegex(42) === '42'`, `escapeRegex(null) === 'null'`, per + // the deleted `tests/phase-id.test.cjs` assertions). That is a deliberate, + // disclosed behavior change for phase-id's former callers: the seam's locked + // signature (`escapeRegex(value: string): string`, ADR §1) does not coerce, so + // a non-string reaching it now throws instead of silently stringifying. + // Mitigated by an explicit `String(...)` wrap at the one former phase-id call + // site that genuinely needed it (phaseMarkdownRegexSource's fallback branch, + // src/phase-id.cts) — the seam's throw itself is intentional and stays. + test('row 8: non-string input throws TypeError (matches 11 of the 12 deleted copies\' .replace throw — phase-id.cts\'s copy coerced instead, see comment above)', () => { + for (const bad of [null, undefined, 42, {}, [], true]) { + assert.throws(() => escapeRegex(bad), TypeError, `escapeRegex(${JSON.stringify(bad)}) must throw TypeError`); + } + }); + + test('row 9: a 10,000-char value does not throw and completes without catastrophic time', () => { + const big = 'a-b.c*d'.repeat(1429); // ~10,001 chars + let escaped; + assert.doesNotThrow(() => { + escaped = escapeRegex(big); + }); + assert.doesNotThrow(() => new RegExp(escaped)); + }); + + test('row 10: RegExp.escape availability — the ADR §2 floor-vs-capability assertion', () => { + assert.strictEqual(typeof RegExp.escape, 'function'); + }); +}); + +// ─── Section 2: literalPattern — rows 11-14 ─────────────────────────────── + +describe('literalPattern', () => { + test('row 11: value + no flags produces a RegExp matching the value literally', () => { + const re = literalPattern('abc'); + assert.ok(re instanceof RegExp); + assert.ok(re.test('abc')); + }); + + test('row 12: value + valid flags ("gi") applies the flags', () => { + const re = literalPattern('abc', 'gi'); + assert.strictEqual(re.flags, 'gi'); + assert.ok(re.test('ABC')); + }); + + test('row 13: invalid flags ("qq") throws SyntaxError from RegExp', () => { + assert.throws(() => literalPattern('abc', 'qq'), SyntaxError); + }); + + test('row 14: metacharacter-heavy value matches literally, not by its metacharacter interpretation', () => { + const re = literalPattern('a.b*c'); + assert.ok(re.test('a.b*c'), 'must match the literal string'); + // If '.' and '*' were left live, this would ALSO match (any-char + zero-or-more) + assert.ok(!re.test('aXbYYYc'), 'must NOT match the metacharacter interpretation'); + }); +}); + +// ─── Section 3: migration equivalence — rows 15-17 (load-bearing) ───────── + +describe('migration equivalence (row-9 sweep)', () => { + test('row 15: property — hand oracle and escapeRegex are match-equivalent for all (s, probe) pairs (seeded)', () => { + // `hand` is the historical oracle: the deleted pre-migration implementation, + // byte-identical across all twelve copies before this seam existed (design + // doc "Ground truth" #1; ADR §7 Class 1 census). It is inlined here ONLY as + // a test oracle for this equivalence sweep — this is NOT a 13th production + // copy of the escape helper (design doc Notes: "not a 13th production copy"). + // eslint-disable-next-line local/no-adhoc-regex-escape -- historical oracle for the row-15 equivalence sweep, not a production copy (#3412) + const hand = (s) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + + fc.assert( + fc.property( + fc.string({ maxLength: 200 }), + fc.string({ maxLength: 50 }), + (s, probe) => { + const handResult = new RegExp(hand(s)).test(probe); + const seamResult = new RegExp(escapeRegex(s)).test(probe); + assert.strictEqual( + seamResult, + handResult, + `match divergence for s=${JSON.stringify(s)} probe=${JSON.stringify(probe)}` + ); + } + ) + ); + }); + + test('row 16: the real corpus — phase tokens, milestone versions, STATE field names, runtime ids are match-equivalent', () => { + // eslint-disable-next-line local/no-adhoc-regex-escape -- historical oracle for the row-16 equivalence sweep, not a production copy (#3412) + const hand = (s) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const corpus = [ + '1A', + 'PROJ-05', + '999.1', + 'v1.2', + '**Current Phase:**', + 'claude-code', + 'gemini-cli', + ]; + const probes = ['1A', 'PROJ-05', '999.1', 'v1.2', '**Current Phase:**', 'claude-code', 'gemini-cli', 'no-match-here']; + for (const s of corpus) { + for (const probe of probes) { + const handResult = new RegExp(hand(s)).test(probe); + const seamResult = new RegExp(escapeRegex(s)).test(probe); + assert.strictEqual( + seamResult, + handResult, + `corpus divergence for s=${JSON.stringify(s)} probe=${JSON.stringify(probe)}` + ); + } + } + }); + + test('row 17: latent-bug fix — a hyphen interpolated into a character class no longer forms a range', () => { + // Pre-migration (hand-rolled, verified): new RegExp('[' + hand('a-z') + ']').test('m') === true + // — the unescaped '-' formed an unintended a-through-z RANGE that happened + // to match 'm'. This is design row 10's confirmed latent bug. + // eslint-disable-next-line local/no-adhoc-regex-escape -- historical oracle for the row-17 latent-bug check, not a production copy (#3412) + const hand = (s) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + assert.strictEqual( + new RegExp('[' + hand('a-z') + ']').test('m'), + true, + 'sanity check: pre-migration hand-rolled behavior was true (verified)' + ); + + // Post-migration (the seam): the fix. 'a', '-', 'z' are all escaped as + // discrete literal characters, so '[...]' no longer forms a range and 'm' + // (interior to the would-be range) must NOT match. + assert.strictEqual( + new RegExp('[' + escapeRegex('a-z') + ']').test('m'), + false, + 'deliberate fix, not an accident: the seam must not let a value form a character-class range' + ); + }); +}); diff --git a/tests/phase-id-drift-guard.test.cjs b/tests/phase-id-drift-guard.test.cjs index 960483dac..149cbf39b 100644 --- a/tests/phase-id-drift-guard.test.cjs +++ b/tests/phase-id-drift-guard.test.cjs @@ -32,8 +32,15 @@ const phaseId = require(path.join(ROOT, 'gsd-core', 'bin', 'lib', 'phase-id.cjs' // The locked canonical surface (ADR-2121 Decision 1/2; PHASE_NUMBER_TOKEN_SOURCE // added in Phase 4). Every name is exported by phase-id.cjs; the identity guard // forbids any other module from re-exporting a divergent copy of one. +// #3212 Phase 1: `escapeRegex` moved off this locked surface entirely — it is +// no longer owned (or re-exported) by phase-id.cjs, it is owned by the +// pattern-construction seam (src/pattern.cts / gsd-core/bin/lib/pattern.cjs). +// Dropped from CANONICAL rather than kept: phase-id.cjs no longer exports the +// name at all, so `name in phaseId` below would fail if it stayed listed, and +// the identity-guard test's job (no consumer re-exports a DIVERGENT copy) is +// now the pattern seam's own single-owner property, not phase-id's. const CANONICAL = [ - 'escapeRegex', 'OPTIONAL_PROJECT_CODE_PREFIX_SOURCE', 'OPTIONAL_PHASE_TAG_SOURCE', + 'OPTIONAL_PROJECT_CODE_PREFIX_SOURCE', 'OPTIONAL_PHASE_TAG_SOURCE', 'PHASE_NUMBER_TOKEN_SOURCE', 'stripProjectCodePrefix', 'normalizePhaseName', 'getMilestoneFromPhaseId', 'getPhaseDirFromPhaseId', 'phaseMarkdownRegexSource', 'phaseMarkdownRegexSourceExact', 'comparePhaseNum', 'extractPhaseToken', diff --git a/tests/phase-id.test.cjs b/tests/phase-id.test.cjs index 07d53025a..273cc7843 100644 --- a/tests/phase-id.test.cjs +++ b/tests/phase-id.test.cjs @@ -2,7 +2,6 @@ * Tests for src/phase-id.cts (compiled to gsd-core/bin/lib/phase-id.cjs). * * Verifies behavioural contracts of the extracted pure phase-id helpers: - * - escapeRegex * - normalizePhaseName * - comparePhaseNum * - extractPhaseToken @@ -22,53 +21,27 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); const phaseId = require('../gsd-core/bin/lib/phase-id.cjs'); +const { buildPhaseHeadingRegex: headingRegex } = require('../gsd-core/bin/lib/roadmap.cjs'); const fc = require('fast-check'); -// ─── escapeRegex ───────────────────────────────────────────────────────────── +// escapeRegex moved off phase-id.cjs entirely in #3212 Phase 1 (#3412): it is +// now owned by the pattern-construction seam (src/pattern.cts, tests in +// tests/pattern.test.cjs) and phase-id.cjs no longer exports it — see +// tests/phase-id-drift-guard.test.cjs's CANONICAL list, which drops it for +// the same reason. -describe('escapeRegex', () => { - test('escapes all regex special characters', () => { - assert.strictEqual(phaseId.escapeRegex('.'), '\\.'); - assert.strictEqual(phaseId.escapeRegex('*'), '\\*'); - assert.strictEqual(phaseId.escapeRegex('+'), '\\+'); - assert.strictEqual(phaseId.escapeRegex('?'), '\\?'); - assert.strictEqual(phaseId.escapeRegex('^'), '\\^'); - assert.strictEqual(phaseId.escapeRegex('$'), '\\$'); - assert.strictEqual(phaseId.escapeRegex('{'), '\\{'); - assert.strictEqual(phaseId.escapeRegex('}'), '\\}'); - assert.strictEqual(phaseId.escapeRegex('('), '\\('); - assert.strictEqual(phaseId.escapeRegex(')'), '\\)'); - assert.strictEqual(phaseId.escapeRegex('|'), '\\|'); - assert.strictEqual(phaseId.escapeRegex('['), '\\['); - assert.strictEqual(phaseId.escapeRegex(']'), '\\]'); - assert.strictEqual(phaseId.escapeRegex('\\'), '\\\\'); - }); - - test('leaves alphanumeric and hyphen characters unescaped', () => { - assert.strictEqual(phaseId.escapeRegex('abc'), 'abc'); - assert.strictEqual(phaseId.escapeRegex('01-02'), '01-02'); - assert.strictEqual(phaseId.escapeRegex('v1.0'), 'v1\\.0'); - }); - - test('coerces non-string values via String()', () => { - assert.strictEqual(phaseId.escapeRegex(42), '42'); - assert.strictEqual(phaseId.escapeRegex(null), 'null'); - assert.strictEqual(phaseId.escapeRegex(undefined), 'undefined'); - }); - - test('adversarial: path-traversal-like inputs are treated as literals', () => { - const result = phaseId.escapeRegex('../../../etc/passwd'); - // The dots get escaped; slashes and alphanumeric pass through unchanged - assert.strictEqual(result, '\\.\\./\\.\\./\\.\\./etc/passwd'); - // The result forms a valid regex (no throws) - assert.doesNotThrow(() => new RegExp(result)); - }); - - test('unicode passthrough', () => { - assert.strictEqual(phaseId.escapeRegex('Phase Name'), 'Phase Name'); - assert.strictEqual(phaseId.escapeRegex('中文'), '中文'); - }); -}); +// ─── #3412 shared test helper ───────────────────────────────────────────────── +// +// phase-id.cts now delegates regex escaping to RegExp.escape via src/pattern.cts. +// RegExp.escape is MATCH-equivalent to the retired hand-rolled escaper but not +// TEXT-equivalent (it hex-escapes the first character and all hyphens), so any +// test that pinned the literal source text (e.g. `'0*29'`, `'PROJ-42'`) is +// brittle to that internal encoding, not to actual behavior. `headingRegex` +// (imported above as `buildPhaseHeadingRegex` from gsd-core/bin/lib/roadmap.cjs) +// is the SAME production function src/roadmap.cts's searchPhaseInContent uses to +// build its heading regex — not a hand-duplicated copy — so tests assert what +// matches and what doesn't — the real contract — rather than the escaper's +// spelling, with no parity gap against the production pattern. // ─── normalizePhaseName ─────────────────────────────────────────────────────── @@ -387,20 +360,50 @@ describe('phaseMarkdownRegexSource', () => { const re = new RegExp(src); assert.ok(!re.test('3X1'), 'unescaped dot would match any char — must be escaped'); }); + + test('regex metacharacters in a phase id are neutralized, not interpreted (#3412)', () => { + // The property RegExp.escape exists for: a literal metacharacter in the + // phase id (the dot in "1.2") must not behave as a regex wildcard once the + // source is compiled into the same heading regex production uses. + const src = phaseId.phaseMarkdownRegexSource('1.2'); + const re = headingRegex(src); + assert.ok(re.test('Phase 1.2: Title')); + assert.ok(!re.test('Phase 1X2: Title')); + }); }); // ─── phaseMarkdownRegexSourceExact ──────────────────────────────────────────── describe('phaseMarkdownRegexSourceExact', () => { test('returns escaped form for project-code-prefixed IDs', () => { - const result = phaseId.phaseMarkdownRegexSourceExact('PROJ-42'); - // hyphen is not a regex special char so it passes through unescaped - assert.strictEqual(result, 'PROJ-42'); - // The result is a valid regex source - assert.doesNotThrow(() => new RegExp(result)); - assert.strictEqual(phaseId.phaseMarkdownRegexSourceExact('MANIFOLD-117'), 'MANIFOLD-117'); - assert.strictEqual(phaseId.phaseMarkdownRegexSourceExact('APP1-117'), 'APP1-117'); - assert.strictEqual(phaseId.phaseMarkdownRegexSourceExact('APP_1-117'), 'APP_1-117'); + // Source text is RegExp.escape's business (#3412) — the actual contract + // is a non-null, compilable source that matches its own prefixed heading + // and rejects the bare-numeric heading. + for (const [id, bareHeadingNum, foreignPrefix] of [ + ['PROJ-42', '42', 'OTHER'], + ['AB-29', '29', 'CK'], + ['MANIFOLD-117', '117', 'OTHER'], + ['APP1-117', '117', 'OTHER'], + ['APP_1-117', '117', 'OTHER'], + ]) { + const result = phaseId.phaseMarkdownRegexSourceExact(id); + assert.ok(result !== null, id); + assert.doesNotThrow(() => new RegExp(result)); + const re = headingRegex(result); + assert.ok(re.test(`Phase ${id}: Title`), `${id} must match its own heading`); + assert.ok(!re.test(`Phase ${bareHeadingNum}: Title`), `${id} must not match the bare-numeric heading`); + // #3599 regression: the exact source must be tied to the FULL prefixed + // id, not just its trailing number — a different prefix with the same + // number must not match (an impl returning `[A-Z]+-42` would pass every + // assertion above but fail this one). + assert.ok( + !re.test(`Phase ${foreignPrefix}-${bareHeadingNum}: Title`), + `${id} must not match a foreign-prefixed heading with the same number`, + ); + // Case-insensitivity (the 'i' flag) — canonicalizes the same way a + // literal would, despite the hex escape (#3412). + assert.ok(re.test(`phase ${id.toLowerCase()}: title`)); + } }); test('returns null for non-prefixed IDs', () => { @@ -657,24 +660,53 @@ describe('isForeignPrefixedPhaseQuery', () => { // ─── roadmapPhaseLookupSources (#2121, owned here after the move) ───────────── describe('roadmapPhaseLookupSources', () => { - const PREFIX_TOLERANT = `${phaseId.OPTIONAL_PROJECT_CODE_PREFIX_SOURCE}0*29`; - test('a bare numeric query yields the numeric then prefix-tolerant sources', () => { const sources = phaseId.roadmapPhaseLookupSources('29'); - assert.deepEqual(sources, ['0*29', PREFIX_TOLERANT]); + assert.equal(sources.length, 2); + // Source text is RegExp.escape's business (#3412) — pin matching + // behavior instead: source[0] matches the bare and zero-padded heading + // but not a project-code-prefixed one; source[1] additionally tolerates + // the prefix. + const re0 = headingRegex(sources[0]); + const re1 = headingRegex(sources[1]); + assert.ok(re0.test('Phase 29: Title')); + assert.ok(re0.test('Phase 029: Title')); + assert.ok(!re0.test('Phase CK-29: Title')); + assert.ok(re1.test('Phase CK-29: Title')); + assert.ok(re1.test('Phase 29: Title')); }); test('the bare numeric source precedes the prefix-tolerant fallback', () => { const sources = phaseId.roadmapPhaseLookupSources('29'); - assert.ok(sources.indexOf('0*29') < sources.indexOf(PREFIX_TOLERANT)); + // Behavioral ordering (#3412): the source that rejects a project-code + // prefix must appear before the source that accepts one. + const bareIdx = sources.findIndex((s) => !headingRegex(s).test('Phase CK-29: Title')); + const prefixTolerantIdx = sources.findIndex((s) => headingRegex(s).test('Phase CK-29: Title')); + assert.notEqual(bareIdx, -1); + assert.notEqual(prefixTolerantIdx, -1); + assert.ok(bareIdx < prefixTolerantIdx); }); test('a project-code-prefixed query adds the exact source first (3 sources)', () => { const sources = phaseId.roadmapPhaseLookupSources('AB-29'); assert.equal(sources.length, 3); - assert.equal(sources[0], 'AB-29'); - assert.ok(sources.includes('0*29')); - assert.ok(sources.includes(PREFIX_TOLERANT)); + // source[0] is the EXACT source (#3599): matches its own prefixed + // heading and must NOT match the bare numeric heading — that ordering + // is the whole point of #3599. Source text itself is RegExp.escape's + // business (#3412). + const reExact = headingRegex(sources[0]); + assert.ok(reExact.test('Phase AB-29: Title')); + assert.ok(!reExact.test('Phase 29: Title')); + // #3599 regression: the exact source is tied to the FULL prefix, not just + // the trailing number — a foreign prefix with the same number must not match. + assert.ok(!reExact.test('Phase CK-29: Title')); + // Case-insensitivity (the 'i' flag) — canonicalizes the same way a + // literal would, despite the hex escape (#3412). + assert.ok(reExact.test('phase ab-29: title')); + // The remaining two sources behave as the numeric/prefix-tolerant pair. + const rest = sources.slice(1).map(headingRegex); + assert.ok(rest.some((re) => re.test('Phase 29: Title') && !re.test('Phase CK-29: Title'))); + assert.ok(rest.some((re) => re.test('Phase CK-29: Title'))); }); test('zero-padding is tolerated: 029 resolves the same sources as 29', () => { diff --git a/tests/phase.test.cjs b/tests/phase.test.cjs index 01f4d62cd..c92c05d41 100644 --- a/tests/phase.test.cjs +++ b/tests/phase.test.cjs @@ -8558,6 +8558,7 @@ const { cleanup, runGsdTools } = require('./helpers.cjs'); // phase-command-router.cjs delegates to SDK when available; we must test the // CJS implementation directly since that is where the bug lives. const phaseModule = require('../gsd-core/bin/lib/phase.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const { cmdPhaseComplete } = phaseModule; function writePassedVerificationFile(phaseDir, phase = '01') { @@ -8698,7 +8699,7 @@ function roadmapCompletionSnapshot(roadmapContent) { } function extractField(stateContent, fieldName) { - const escaped = fieldName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const escaped = escapeRegex(fieldName); const boldMatch = stateContent.match(new RegExp(`\\*\\*${escaped}:\\*\\*[ \\t]*(.+)`, 'i')); if (boldMatch) return boldMatch[1].trim(); const plainMatch = stateContent.match(new RegExp(`^${escaped}:[ \\t]*(.+)`, 'im')); diff --git a/tests/phase6-capstone-conformance.test.cjs b/tests/phase6-capstone-conformance.test.cjs index 32d3cd471..08af76cf4 100644 --- a/tests/phase6-capstone-conformance.test.cjs +++ b/tests/phase6-capstone-conformance.test.cjs @@ -21,15 +21,12 @@ const CORE_SUBSTRATE_TERMS = [ const registry = require('../gsd-core/bin/lib/capability-registry.cjs'); const { isCentralConfigKey } = require('../gsd-core/bin/lib/config-schema.cjs'); +const { escapeRegex: escapeRegExp } = require('../gsd-core/bin/lib/pattern.cjs'); function readRepoFile(relativePath) { return fs.readFileSync(path.join(ROOT, relativePath), 'utf8'); } -function escapeRegExp(value) { - return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); -} - function activeWhenKeys() { const keys = new Set(); for (const cap of Object.values(registry.capabilities)) { diff --git a/tests/product-name-purity.test.cjs b/tests/product-name-purity.test.cjs index d834d1541..4ed6c5963 100644 --- a/tests/product-name-purity.test.cjs +++ b/tests/product-name-purity.test.cjs @@ -15,6 +15,7 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const ROOT = path.join(__dirname, '..'); @@ -48,7 +49,7 @@ function findProductParentheticals(content) { for (const product of PRODUCTS) { // Match "ProductName (something)" but not "ProductName (v1.2.3)" (version refs are ok) const pattern = new RegExp( - product.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + + escapeRegex(product) + '\\s*\\([^)]*(?!v?\\d+\\.\\d)[^)]*\\)', 'g' ); diff --git a/tests/removed-but-needed-lint.test.cjs b/tests/removed-but-needed-lint.test.cjs index 04e74ae8d..932df7323 100644 --- a/tests/removed-but-needed-lint.test.cjs +++ b/tests/removed-but-needed-lint.test.cjs @@ -23,6 +23,7 @@ const { referencesBasename, referencesNpmLockfileDependency, findSurvivingRefere const { cleanup } = require('./helpers.cjs'); const { runNode } = require('./helpers/process-seam.cjs'); const { gitOrThrow } = require('./helpers/git-fixture.cjs'); +const { copyScriptWithDeps } = require('./helpers/copy-script-fixture.cjs'); describe('removed-but-needed lint: referencesBasename (pure)', () => { test('a plain filename reference in prose is found', () => { @@ -158,13 +159,7 @@ function buildTempRepo(tmpDir, baseFiles, prFiles) { * @returns {string} path to the copied script inside tmpDir/scripts/ */ function copyScriptInto(tmpDir) { - const scriptsDir = path.join(tmpDir, 'scripts'); - const libDir = path.join(scriptsDir, 'lib'); - fs.mkdirSync(libDir, { recursive: true }); - const scriptCopy = path.join(scriptsDir, 'lint-removed-but-needed.cjs'); - fs.copyFileSync(LINT_SCRIPT, scriptCopy); - fs.copyFileSync(path.join(ROOT, 'scripts', 'lib', 'cli-exit.cjs'), path.join(libDir, 'cli-exit.cjs')); - return scriptCopy; + return copyScriptWithDeps(ROOT, tmpDir, path.relative(ROOT, LINT_SCRIPT)); } describe('removed-but-needed lint: main() end-to-end wiring', () => { diff --git a/tests/reversibility-tagging.test.cjs b/tests/reversibility-tagging.test.cjs index f4e6ddb4b..1245248d0 100644 --- a/tests/reversibility-tagging.test.cjs +++ b/tests/reversibility-tagging.test.cjs @@ -13,6 +13,7 @@ const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); const { runGsdTools, createTempProject, cleanup } = require('./helpers.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const ROOT = path.resolve(__dirname, '..'); const PLANNER = path.join(ROOT, 'agents', 'gsd-planner.md'); @@ -47,7 +48,7 @@ function namesRating(text, rating) { // js/incomplete-sanitization (CodeQL, high). `-` needs no escaping outside a // character class, so the previous `-`-only replace was both incomplete and // unnecessary. - const escaped = rating.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const escaped = escapeRegex(rating); return new RegExp(`\\b${escaped}\\b`).test(text); } diff --git a/tests/runtime-launcher-parity.test.cjs b/tests/runtime-launcher-parity.test.cjs index 45ff4bc89..cbbb5cc50 100644 --- a/tests/runtime-launcher-parity.test.cjs +++ b/tests/runtime-launcher-parity.test.cjs @@ -32,6 +32,7 @@ const os = require('node:os'); const { runHook: runHookSeam } = require('./helpers/process-seam.cjs'); const { throwIfFailed } = require('./helpers/git-fixture.cjs'); const { cleanup } = require('./helpers.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const WORKFLOWS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows'); const AGENTS_DIR = path.join(__dirname, '..', 'agents'); @@ -87,7 +88,7 @@ function extractShellBlocks(content) { blockLang = (fenceOpen[2] || '').toLowerCase(); blockLines = []; // Closing pattern: same indent prefix + ``` - closingPattern = new RegExp('^' + blockIndent.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '```\\s*$'); + closingPattern = new RegExp('^' + escapeRegex(blockIndent) + '```\\s*$'); continue; } } else { @@ -1027,6 +1028,7 @@ const os = require('node:os'); const { runHook: runHookSeam } = require('./helpers/process-seam.cjs'); const { throwIfFailed } = require('./helpers/git-fixture.cjs'); const { cleanup } = require('./helpers.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); const WORKFLOWS_DIR = path.join(__dirname, '..', 'gsd-core', 'workflows'); const SNIPPET_FILE = path.join(WORKFLOWS_DIR, '_runtime-launcher.snippet.sh'); @@ -1104,7 +1106,7 @@ function extractShellBlocks(content) { blockIndent = fenceOpen[1]; blockLang = (fenceOpen[2] || '').toLowerCase(); blockLines = []; - closingPattern = new RegExp('^' + blockIndent.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '```\\s*$'); + closingPattern = new RegExp('^' + escapeRegex(blockIndent) + '```\\s*$'); continue; } } else { diff --git a/tests/workstream-name-policy.test.cjs b/tests/workstream-name-policy.test.cjs index a0273c5f3..46429fe26 100644 --- a/tests/workstream-name-policy.test.cjs +++ b/tests/workstream-name-policy.test.cjs @@ -8,6 +8,7 @@ const { isValidActiveWorkstreamName, INVALID_ACTIVE_WORKSTREAM_NAME_MESSAGE, } = require('../gsd-core/bin/lib/workstream-name-policy.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); describe('workstream-name-policy', () => { test('normalizeWorkstreamNameInput trims and nulls empty input', () => { @@ -39,7 +40,7 @@ describe('workstream-name-policy', () => { assert.equal(assertValidActiveWorkstreamName(' alpha '), 'alpha'); assert.throws( () => assertValidActiveWorkstreamName('alpha/beta'), - new RegExp(INVALID_ACTIVE_WORKSTREAM_NAME_MESSAGE.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) + new RegExp(escapeRegex(INVALID_ACTIVE_WORKSTREAM_NAME_MESSAGE)) ); }); diff --git a/tests/worktree-safety.test.cjs b/tests/worktree-safety.test.cjs index c3076bf66..e6ba62e0d 100644 --- a/tests/worktree-safety.test.cjs +++ b/tests/worktree-safety.test.cjs @@ -23,6 +23,7 @@ const fc = require('fast-check'); const { createTempDir, cleanup } = require('./helpers.cjs'); const { createFixture } = require('./fixtures/index.cjs'); const { makeFaultyGit } = require('./helpers/faulty-deps.cjs'); +const { escapeRegex } = require('../gsd-core/bin/lib/pattern.cjs'); // 30000ms: this file's single named bound for every migrated subprocess call // below (git plumbing on small mkdtemp fixtures, gsd-tools.cjs/hook CLI runs, @@ -974,7 +975,7 @@ describe('planWorktreeRecordAgent', () => { }); assert.equal(plan.reason, 'missing_field'); for (const flag of ['--agent-id', '--path', '--branch', '--base']) { - assert.match(plan.hint, new RegExp(flag.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))); + assert.match(plan.hint, new RegExp(escapeRegex(flag))); } }); diff --git a/tsconfig.build.json b/tsconfig.build.json index 51f927db2..75a0590cb 100644 --- a/tsconfig.build.json +++ b/tsconfig.build.json @@ -6,7 +6,7 @@ "module": "nodenext", "moduleResolution": "nodenext", "target": "ES2022", - "lib": ["ES2022"], + "lib": ["ES2022", "ES2025.RegExp"], "types": ["node"], "strict": true, "declaration": false,