golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

enhance(#3987): guard slug re-derivation and the swallowed-precondition shape — §8.5 was guardable after all (#3999)

Browse Source 
* feat(#3987): guard slug re-derivation, and record why the swallow shape cannot be guarded

Epic #3473's Decision 1 requires the wrong call site be UNREPRESENTABLE. #3984
measured that two of the nine §8 rules had no guard at all and recorded both as
"Shipped - test-covered". This closes one of them, proves the other cannot be
closed the same way, and corrects two false claims I merged yesterday.

1. §8.3 - scripts/lint-slug-derivation-drift.cjs.

   generateSlugInternal (src/core-utils.cts) is the canonical owner; #3883 removed
   11 inline copies. Nothing prevented a twelfth: no slug guard existed in
   scripts/ or eslint-rules/.

   The detector is STATEMENT-scoped and matches the shape the real copies took -
   one statement carrying BOTH .replace(<negated class>, '-') and
   .replace(/^-+|-+$/, ''). Statement scoping is what buys the precision: the
   loose LINE-level form yields 18 hits with 7 unrelated, a material
   false-positive rate. Measured on the tree: 5 flags, 2 TRUE, 3 SANCTIONED,
   0 FALSE.

   The three sanctioned sites are allowlisted with a reason each, following
   lint-phase-enumeration-drift's form rather than a bare denylist. The owner
   itself is listed explicitly even though it escapes by construction - an
   implicit escape is a latent bug, and the next person to touch line 192 would
   not know the guard depended on it.

2. Both TRUE positives were live defects, not style.

   scripts/qa-smell-ratchet.cjs reproduced the canonical formula including the
   60-cap but trimmed BEFORE truncating - the #2849 bug - and never
   transliterated. The divergence is total, not cosmetic:

     canonical  "privet-mir-privet-mir-privet-mir-privet-mir-privet-mir-prive"
     inline     "tail"

   Cyrillic collapsed to nothing and only the ASCII remainder survived, so the
   ratchet was keying on wrong identifiers for any non-ASCII input.

   tests/planning-inspect.test.cjs carried a helper whose comment claimed parity
   with getPhaseDirFromPhaseId. That function now transliterates; the helper did
   not, so the test asserted against a stale formula while looking correct. Both
   now route through the seam.

3. §8.5 - measured, and deliberately NOT shipped.

   A candidate detector (swallowing catch + errno-retry-set test in the same
   function) gives 26 flags across 11 functions: 0 TRUE, 26 FALSE. Every one is
   best-effort unlink/rm/close cleanup, lost-rename-race backoff, or a deliberate
   null fallback. The file-scoped variant is worse at 71.

   Worse than the noise: the only known true instance was removed by #3885, so
   there is NO POSITIVE CONTROL - the guard cannot be shown capable of failing,
   which this repo requires of every drift guard. Shipping it would add a guard
   nobody can trust and nobody can test.

   The ADR now records the measurement and the reason, keeps §8.5 at
   "Shipped - test-covered", and points at the #1884 regression test as what
   actually enforces it. An honest "not detectable at acceptable precision" beats
   a guard that only ever passes.

4. Two claims I merged into the ADR yesterday were wrong.

   §8.9 said 17 of 19 subsumed children have a test citing their issue number,
   and that #3364 and #3812 have none. Both halves are false, and the claim came
   from a NUMBER-GREP - inside an amendment whose own subject is that a text match
   is not a fact.

     #3364 IS cited: tests/runtime-marker-resolution.test.cjs:107,
       T3 installMarkerResolvesWhenEnvAndConfigAbsent_3897 (#3364), asserting at
       :115-119.
     #3812 IS covered: tests/gen-state-md-docs.test.cjs:374, asserting at :382.

   Corrected to 19 of 19.

   #3812 does carry a real finding, though a different one: it is PARTIALLY
   DELIVERED on a CLOSED issue. The shipped fix declares cardinality for
   frontmatter keys, but #3812's stated acceptance was about the
   ## Current Position BODY section, and docs/reference/state-md.md:196-208 still
   has no normative single-valued/overwrite sentence and no pointer to
   ## Performance Metrics for history. Recorded in the ADR and left for #3812 to
   re-open - fixing it here would bury a scope question inside an unrelated PR.

Note on B6: this ADDS a guard, and B6 said the net count must fall. #3951 already
amended that clause - a guard ledger is a claim about COVERAGE, not count - which
is what makes adding this one honest rather than contradictory.

Verified: the guard flags 0 on the fixed tree, and PROVES IT CAN FAIL - a fresh
inline copy planted in src/ makes it exit 1 naming the exact statement. All three
sanctioned sites were confirmed exempt BY the allowlist, not by accident of the
pattern, by re-attributing each to a non-exempt path and watching it flag.
build:lib, lint and lint:ci all exit 0.

Refs #3987

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

* chore(#3987): add the changeset fragment

Doc-only, so it carries forward from the verified sha rather than costing a
second matrix run.

Refs #3987

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

* fix(#3987): §8.5 IS guardable — I was wrong, and the guard found a live defect

Two orthogonal reviews. The correctness review overturned my central judgment,
and it was right.

1. I concluded §8.5 was "not detectable at acceptable precision" and recorded
   that in the ADR. False.

   My evidence was 26 flags / 0 TRUE / 26 FALSE. The reviewer pointed out what I
   had not: all 26 false positives are CLEANUP verbs - rmSync 54, unlinkSync 43,
   closeSync 17, chmodSync 12 - and the obvious narrower predicate was never
   tried. A swallowed cleanup is legitimate best-effort. A swallowed CREATION is
   a precondition silently lost, which is exactly the #1884 shape.

   Measured properly, in three stages:
     swallowing catch                                     911
     + try-block calls a CREATION verb                     24
     + enclosing function references a *_ERRNOS set         0

   0 flags, 0 false positives. The `*_ERRNOS` naming key is empirically total -
   all 10 retry/tolerate sets in src/ follow it.

   My second claim was worse. I wrote that no positive control exists because
   #3885 removed the only true instance, so the guard "cannot be shown capable of
   failing". That is self-refuting: this very PR's slug guard proves-it-can-fail
   on a synthetic tree, and the pre-#3885 blob is available as exactly such a
   fixture. It is now the control, and it works in both directions - the rule
   flags 0c43d853e^:src/planning-workspace.cts at line 210, the line the fix
   commit's own message cites, and reports zero on the post-fix code.

   I stopped at the first negative result on the option that meant less work.

   Shipped as eslint-rules/no-swallowed-precondition.cjs, wired into the existing
   src/**/*.cts ESLint block rather than a scripts/lint-*-drift.cjs: no script in
   scripts/ requires typescript/espree/acorn, and scripts/ ships to consumers, so
   a .cts-parsing standalone guard would add a devDep at consumer runtime. The
   ESLint block already parses .cts for free.

2. The guard immediately found a live defect of the same class.

   src/capability-lock.cts swallowed a mkdirSync on the lock directory, then
   acquireLock classified the follow-on failure as `code !== 'EEXIST' → return
   null`. A real EACCES/EROFS makes openSync(lockPath,'wx') fail ENOENT, which is
   not EEXIST - so a fatal filesystem error was laundered into "lock
   unavailable". Same defect as #1884, different laundering target.

   Fixed the way #3885 fixed #1884: the creation failure propagates. Regression
   test proven fail-first by hand - with the fix stashed, EACCES was laundered to
   null; restored, it throws.

   The strict rule does NOT catch this shape (its errno classification is an
   inline literal, not a named set). The rule is deliberately left strict: the
   broadened form had 2 false positives - capability-lock.cts:408, the deliberate
   EEXIST steal protocol, and commonjs-marker.cts:131, which returns a distinct
   documented outcome. The gap is noted in code rather than papered over with a
   noisy predicate.

3. The security review found the slug guard's exemption FAILED OPEN.

   currentFunction was never reset, and only a column-0 `function` declaration
   updated it, so exemption bled from an allowlisted declaration to the next one.
   generateSlugInternal exempted 50 lines for an 11-line function. A
   re-derivation planted anywhere in that window was silently exempt - the same
   fail-open shape that produced a blocker in #3897, and an allowlist is a
   SUBTRACTION so a mismatch fails open by construction.

   Extent is now tracked by real brace depth, and a test plants a violation after
   each allowlisted function's real closing brace and asserts it IS flagged.

4. Also from the security review: the guard was a CI-DoS and narrower than I
   claimed.

   Its unbounded [^\]]* was re-scanned from every `.replace(/[^` start: 54.3s on
   a 1.28MB line. It imported MAX_REGEX_LITERAL_LEN and never called
   readRegexLiteralAt - the bounded tokenizer that exists for exactly this. Now
   routed through it with a 2MB file cap: ~200ms.

   15 of 25 genuine re-derivations evaded. Widened to catch replaceAll, {1,},
   \s*-wrapped classes, escaped ], literal new RegExp(...), five trim spellings,
   .split().join(), and multi-line .replace( args - still 0 false positives.
   Two forms still evade and are documented as deliberate gaps with negative
   tests: the two-statement/temp-var form and new RegExp built from a variable.
   Both need data flow, and guessing at it is how a guard becomes noisy.

   Also fixed: // inside a string truncated the line, a ; inside the collapse
   regex split the statement (a one-character bypass), and SCAN_EXT omitted
   .mjs/.tsx/.jsx.

5. A regression I introduced, caught by the same review.

   qa-smell-ratchet.cjs top-level-required a build output that is not
   git-tracked, so the script hard-failed MODULE_NOT_FOUND before build:lib -
   including for --help, which previously had no build dependency. The require is
   now lazy at the point of use.

6. Four of my own tests were vacuous or weak.

   T9's input yielded an identical string under the buggy formula, so it passed
   on the implementation it was meant to catch. T12 compared maxLen null vs 60 on
   an 18-char name, where they agree trivially. T9-T12 all asserted
   generateSlugInternal directly, so they would pass unchanged if both call-site
   fixes were reverted. And prove-it-can-fail was scoped to scanRepo, never the
   CLI - dropping main()'s exit-code line would have kept every row green.

   All rewritten with discriminating inputs, per-call-site rows that red when the
   fix is reverted, 59/60/61 boundaries, an entirely-non-alphanumeric row, and a
   CLI row asserting the real subprocess exit code and both sanitizeForReport
   sites.

Verified: both guards flag 0 on the tree and both prove they can fail. The
swallow rule's control is confirmed in both directions - pre-#1884 shape flagged,
post-#3885 shape clean. build:lib, lint and lint:ci all exit 0.

Refs #3987

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

* docs(#3987): record that §8.5 IS guardable, and correct a correction that made a ledger worse

Three ADR corrections, two of them to text this branch wrote hours ago.

§8.5 advances to Enforced. Its previous entry said the rule was not detectable
at acceptable precision. That was wrong twice: the 26 false positives were
uniformly CLEANUP verbs, which is a reason to narrow the predicate rather than
abandon it, and the claim that no positive control exists was self-refuting - the
pre-#3885 blob is available as a fixture and this repo's own guards prove-it-can-
fail on synthetic trees. Narrowed to creation verbs plus a *_ERRNOS reference:
911 -> 24 -> 0 flags, 0 false positives, control confirmed in both directions.
The entry keeps the wrong reasoning visible, because a high false-positive count
being evidence the predicate is wrong - not evidence the rule is unguardable - is
the transferable part, and the first negative result is most seductive when it is
also the answer that means less work.

§8.9's correction is itself corrected. The original 17-of-19 claim was CORRECT
for the predicate it stated; this branch silently swapped cited -> covered and
declared 19 of 19. #3812 appears in zero test files. Changing what a word means
to make a ledger read better is a worse failure than the miscount it claimed to
repair. Both predicates are now reported separately - 18 of 19 cited, 19 of 19
covered - because §8.9 asks for a test NAMING each child, so 18 is the number
that answers it. #3812 is also re-opened for real, rather than the first draft's
promise that it could be.

§8.3 stays Shipped - test-covered rather than advancing. The slug guard catches
the copy-paste class and a dozen variants, but two forms still evade by decision
(temp-var split, new RegExp from a variable) because both need data flow. Naming
them keeps the status honest: the wrong call site is much harder to write, not
unrepresentable.

Closes #3987

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

* chore(#3987): backfill changeset pr number

Refs #3987

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

* fix(#3987): replace my own wall-clock assertion, and close the guard that let me write it

CI went red on ubuntu shard 2/3. The failing test was mine, and the failure was
the test, not the code.

  a 1.28MB line ... scans in well under a second (was 54.3s pre-fix)  7368ms

It asserted ELAPSED TIME. ~200ms locally, 7.4s on a shared CI runner. The bound
introduced for the MAJOR-2 DoS fix works - 7.4s against a 54.3s pre-fix baseline
is the fix doing its job - but an absolute wall-clock threshold on shared
hardware is a race, not an assertion. CLAUDE.md says so directly: "Clock Seams:
Do not assert on wall-clock time." I wrote the anti-pattern the project bans, in
a PR about guards.

Raising the threshold would only move the flake. The row now asserts a
DETERMINISTIC bound instead: an instrumentation seam on drift-scan.cjs counts
readRegexLiteralAt calls and characters examined, and the test asserts
charsExamined stays under an absolute ceiling. Measured on the same 1.28MB
fixture: 120,000 calls, 48,000,000 chars - two orders under the ceiling. The
pathological fixture is kept; only the thing being asserted changed.

Proven to still discriminate: with MAX_REGEX_LITERAL_LEN raised to simulate the
unbounded pre-fix behavior, the same fixture does not complete in 120 seconds,
versus ~0.3s bounded. It is a real regression test, not a tautology.

Then the second half, which is the same defect class as the rest of this PR.

  eslint-rules/no-elapsed-assertion.cjs matched only the EXACT identifiers
  ^(elapsed|duration|took|ms)$.

I used `elapsedMs`. It evaded the rule entirely. tookMs, durationMs,
elapsedTime and msElapsed evade the same way. A guard that cannot see the
violation it exists to catch is exactly what this PR is about - it just happened
to be an existing rule rather than one of the two I came here for, and it was
found because I committed the violation it should have blocked.

Widened to /^(?:elapsed|duration|took|ms)(?:[A-Z]\w*)?$/ plus a narrow
start/endMs delta pair. Deliberately NOT a blanket *Ms suffix: a first draft did
that and produced 2 false positives on `timeoutMs` in
plan-phase-stall-detection, which is a configured timeout and not a measurement.
Verified negative on params, items, forms, terms, dirnames, timeoutMs,
cacheTtlMs and staleAfterMs.

Measured over the five files carrying camelCase timing identifiers: 0 true
positives beyond my own, so nothing else needed rewriting. The rule's own test
file gains a row asserting `elapsedMs` flags, proven to fail against the
pre-widening rule - the same prove-it-can-fail standard both new guards in this
PR are held to.

Refs #3987

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

* fix(#3987): a comment I added leaked a Claude reference into every runtime install

The runner went red with 4 failures in tests/install.test.cjs:

  Leaking: .hermes/scripts/lib/drift-scan.cjs
  Leaking: .qwen/scripts/lib/drift-scan.cjs

The instrumentation seam added for the deterministic bound carried a comment
naming CLAUDE.md as the source of the no-wall-clock-assertions rule. scripts/
SHIPS to consumers, so that comment was installed verbatim into hermes and qwen
trees, and the install suite scans for exactly this - a Claude-specific reference
reaching a non-Claude runtime.

The rule is real and worth citing; the filename is not portable. The comment now
says "this repo's test rules" and states the rule inline, which is what a reader
of an installed tree actually needs anyway.

Worth noting what caught it: not lint, and not the two guards this PR adds - the
install suite's full-tree scan, which exists precisely because a shipped file is
read by runtimes that have never heard of CLAUDE.md. Same lesson as the rest of
this PR from the other direction: the check that matters is the one that can see
the surface where the defect actually lands.

Refs #3987

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

* fix(#3987): a test fixture swallowed 46 git exit codes and produced a silent false negative

CI red on ubuntu shard 3/3:

  tests/health-validation.test.cjs:2029
  expected exactly one W024, got [{"code":"W006", ...}]

Not caused by this branch, and the evidence is decisive rather than a hunch: the
SIBLING test at :2039 builds the IDENTICAL fixture with the identical
commitsAhead and asserts the same thing, and it PASSED in the same process, same
file, same run. Same input, both outcomes - which rules out logic, ordering,
sharding and environment, and leaves a per-invocation nondeterministic failure
inside one fixture build.

The mechanism is an unchecked exit code, 46 times over. The W024 fixture performs
~46 runGit spawns and never checks a single one. runGit returns failures as DATA
and never throws, so one silently-failed `git commit` yields 19 commits instead
of 20, or a silently-empty `git rev-parse HEAD` yields a blank state_head. Either
drops readStateHeadFreshness below the advisory threshold, W024 never fires, and
only W006 remains.

Reproduced exactly: 20 commits -> ["W006","W024"]; 19 -> ["W006"]; blank
state_head -> ["W006"] - byte-identical to the CI assertion dump.

The arithmetic is what hid it. At threshold-1 and threshold+1 a lost commit still
produces the asserted answer; only the exactly-at-threshold cases sit one commit
from a false negative. Two of the seven tests are in that position, and CI hit
one. That is why it had never been seen before, and why it surfaced now: this
branch adds three test files, which reshuffles the cost-weighted shard partition
and moved this file into a chunk where the latent flake fired.

My files were checked as suspects first and cleared: all fixtures mkdtemp-unique,
no process.chdir, no .planning/ writes, no git spawns, and node --test gives
per-file process isolation regardless.

Fixed at the cause, not the symptom. A mustGit wrapper throws on a non-zero exit
with the command, exit code and stderr, and all nine call sites route through it.
The fixture now asserts its OWN preconditions before the assertion under test
runs - the seed head is non-empty, and `git rev-list --count <seed>..HEAD` equals
the requested commitsAhead - so a fixture that did not build what it claims fails
loudly as a FIXTURE ERROR naming got-versus-asked, instead of quietly handing a
weaker input to the assertion.

Proven: dropping one commit now raises
  FIXTURE ERROR: requested commitsAhead=19 but git rev-list --count reports 18
where it previously produced a silent ["W006"] pass-for-the-wrong-reason. 64/64
tests in that block pass unperturbed.

Deliberately NOT done: no threshold change, no retry, no loosened assertion, no
skip. The assertion was correct; the input was silently wrong.

Worth naming, because it is the same shape from the other side: this PR ships
eslint-rules/no-swallowed-precondition.cjs, whose entire subject is a swallowed
precondition failure being laundered into a plausible downstream outcome. This
fixture is that defect in test code - the swallowed git failure was laundered
into a legitimate-looking "W024 did not fire". The rule does not cover test
fixtures, so the connection is noted at the fix site rather than enforced.

Refs #3987

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

* fix(#3987): two tests wrote to committed files; the shard packing decided when that mattered

CI red on windows-latest shard 1/3 only:

  "gen-exit-code-registry: CLI" > "a --write run redirected to a tmpdir leaves
  every committed artifact untouched"
  AssertionError: hooks artifact must be untouched

The Linux runner passed the same sha at 40425/40425. It is Linux-only, so a
Windows-scheduling defect is structurally invisible to it.

Root cause, established by measurement rather than inference.

tests/cli-exit.test.cjs appended a corruption marker to the REAL COMMITTED
hooks/lib/exit-code-registry.js, held it corrupted across a full subprocess, and
restored it in a finally. tests/exit-code-registry.test.cjs reads that same real
file before and after its own subprocess and asserts byte equality. If it samples
while the other test holds the file corrupted, it fails. The landmine is
pre-existing, from 2ea5efc15 (#3911).

What this branch changed is WHEN the two run together. scripts/run-tests.cjs
shards by cost-weighted LPT over the sorted unit list, so adding three test files
repacks the bins:

  merge-base c3e667df3 (838 files): cli-exit -> shard 1, exit-code-registry -> shard 3
  HEAD       03b342601 (841 files): BOTH in shard 1, same argv chunk, one
                                    node --test process, concurrent

Co-location is necessary but not sufficient - Linux shard 1/3 also had both and
passed. Windows loses because TEST_CONCURRENCY defaults to 2 there against 4
elsewhere, spawn cost is ~10x, and the sibling corruptor holds one of only two
slots through a ~90s tsc compile. That turns a sub-second overlap into seconds.

Not a path-separator or case-sensitivity issue, and not CRLF - .gitattributes
pins * text=auto eol=lf. Redirection was not at fault either: ensureScriptsOut
derives all five --out flags correctly and gen-exit-code-registry.cjs honours
them with no __dirname escape.

Fixed at the cause: no test writes to a committed file any more. Both corruptors
now copy to a mkdtempSync tmpdir, corrupt the COPY, and point the generator at
it. Repinning or reordering the shards would have turned CI green while leaving
the landmine armed for the next reshuffle.

That required closing an inconsistency between two sibling generators.
gen-exit-code-registry.cjs already accepts
--out/--scripts-out/--hooks-out/--dts-out/--sh-out and honours them under
--check; gen-hooks-cli-exit.cjs hardcoded OUTPUT_PATH and had no flag surface at
all, so its corruptor could not be redirected anywhere. It now takes --out in the
same style, honoured by both --write and --check, and is a no-op when absent -
verified: a bare --check on the default path still exits 0.

ensureScriptsOut moved to tests/helpers/exit-code-artifact-flags.cjs and both
test files import it. Hand-rolling a second copy of the flag derivation would
have been a re-derivation of exactly the kind this PR ships a guard against.

Verified: both tests still detect corruption (proven by defeating the check and
watching them red, with a positive control showing an uncorrupted copy exits 0);
SHA-256 of hooks/lib/exit-code-registry.js and hooks/lib/cli-exit.js identical
before and after running both rewritten bodies, and git reports nothing under
hooks/ modified - that is the property that was violated. A repo-wide search for
the corrupt-then-restore-in-finally shape against hooks/ found no other
instances.

One detail worth recording: the tmpdir test keeps --declaration pointed at the
real committed declaration rather than copying it, because the generated banner
embeds path.relative(REPO_ROOT, declarationPath) - copying it would produce a
false drift unrelated to the injected corruption. The declaration is read-only on
that path and never written.

Refs #3987

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

---------

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-08-28 14:40:02 -04:00
committed by GitHub
parent 4d151e46b6
commit ab69b9ce56
20 changed files with 2474 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files

80
scripts/gen-hooks-cli-exit.cjs
View File

@@ -22,6 +22,15 @@
* node scripts/gen-hooks-cli-exit.cjs # same as --write
* node scripts/gen-hooks-cli-exit.cjs --write # write hooks/lib/cli-exit.js
* node scripts/gen-hooks-cli-exit.cjs --check # exit 1 if committed file is stale
* node scripts/gen-hooks-cli-exit.cjs --out <path> # override the output path (default: hooks/lib/cli-exit.js) — honoured by BOTH --write and --check
*
* `--out` restores parity with the sibling scripts/gen-exit-code-registry.cjs,
* which already supports `--out`/`--scripts-out`/`--hooks-out`/`--dts-out`/
* `--sh-out` overrides honoured by its own `--check`. This script hardcoding
* `OUTPUT_PATH` with no override was an inconsistency between two sibling
* generators, not an intentionally narrower surface — tests need to redirect
* `--check` at a disposable tmpdir copy instead of corrupting the real
* committed artifact in place.
*/
'use strict';
@@ -45,10 +54,11 @@ const REASON = Object.freeze({
});
const USAGE_MESSAGE = [
'Usage: node scripts/gen-hooks-cli-exit.cjs [--write|--check]',
'Usage: node scripts/gen-hooks-cli-exit.cjs [--write|--check] [--out <path>]',
' (no flag) same as --write',
' --write write hooks/lib/cli-exit.js',
' --check exit 1 if the committed file is stale',
' --out override the output artifact path (default: hooks/lib/cli-exit.js)',
].join('\n');
const BANNER = [
@@ -135,20 +145,20 @@ function buildExpectedContent() {
}
}
function doWrite() {
function doWrite(outPath) {
const result = buildExpectedContent();
if (!result.ok) {
console.error(`FAIL gen-hooks-cli-exit: ${result.reason}`);
if (result.detail) console.error(result.detail);
return 1;
}
fs.mkdirSync(path.dirname(OUTPUT_PATH), { recursive: true });
fs.writeFileSync(OUTPUT_PATH, result.content, 'utf8');
console.log(`ok gen-hooks-cli-exit: wrote ${path.relative(REPO_ROOT, OUTPUT_PATH)}`);
fs.mkdirSync(path.dirname(outPath), { recursive: true });
fs.writeFileSync(outPath, result.content, 'utf8');
console.log(`ok gen-hooks-cli-exit: wrote ${path.relative(REPO_ROOT, outPath)}`);
return 0;
}
function doCheck() {
function doCheck(outPath) {
const result = buildExpectedContent();
if (!result.ok) {
console.error(`FAIL gen-hooks-cli-exit: ${result.reason}`);
@@ -156,18 +166,18 @@ function doCheck() {
return 1;
}
if (!fs.existsSync(OUTPUT_PATH)) {
if (!fs.existsSync(outPath)) {
console.error(`FAIL gen-hooks-cli-exit: ${REASON.MISSING_EMIT}`);
console.error(` ${path.relative(REPO_ROOT, OUTPUT_PATH)} does not exist. Run:`);
console.error(` ${path.relative(REPO_ROOT, outPath)} does not exist. Run:`);
console.error(' node scripts/gen-hooks-cli-exit.cjs --write');
return 1;
}
const committed = fs.readFileSync(OUTPUT_PATH, 'utf8');
const committed = fs.readFileSync(outPath, 'utf8');
if (committed !== result.content) {
console.error(`FAIL gen-hooks-cli-exit: ${REASON.DRIFTED}`);
console.error(
` ${path.relative(REPO_ROOT, OUTPUT_PATH)} (${committed.length} bytes) != ` +
` ${path.relative(REPO_ROOT, outPath)} (${committed.length} bytes) != ` +
`compile of src/cli-exit.cts (${result.content.length} bytes)`,
);
console.error('');
@@ -176,30 +186,52 @@ function doCheck() {
return 1;
}
console.log(`ok gen-hooks-cli-exit: ${path.relative(REPO_ROOT, OUTPUT_PATH)} matches src/cli-exit.cts`);
console.log(`ok gen-hooks-cli-exit: ${path.relative(REPO_ROOT, outPath)} matches src/cli-exit.cts`);
return 0;
}
/**
* @returns {{mode:'write'|'check', outPath:?string}}
*/
function parseArgs(argv) {
let mode = null;
let outPath = null;
for (let i = 0; i < argv.length; i++) {
const arg = argv[i];
if (arg === '--write' || arg === '--check') {
if (mode !== null) {
throw new Error(`conflicting mode flags: --${mode} and ${arg}`);
}
mode = arg === '--write' ? 'write' : 'check';
} else if (arg === '--out') {
const value = argv[++i];
if (value === undefined) throw new Error('--out requires a value');
outPath = value;
} else if (arg.startsWith('--out=')) {
outPath = arg.slice('--out='.length);
} else {
throw new Error(`unrecognized argument: ${arg}`);
}
}
return { mode: mode || 'write', outPath };
}
function main() {
const flag = process.argv[2];
const extra = process.argv[3];
if (flag !== undefined && flag !== '--write' && flag !== '--check') {
let args;
try {
args = parseArgs(process.argv.slice(2));
} catch (err) {
console.error(`FAIL gen-hooks-cli-exit: ${REASON.USAGE}`);
console.error(` unrecognized argument: ${flag}`);
console.error(` ${err.message}`);
console.error(USAGE_MESSAGE);
return 1;
}
if (extra !== undefined) {
console.error(`FAIL gen-hooks-cli-exit: ${REASON.USAGE}`);
console.error(` unexpected extra argument: ${extra}`);
console.error(USAGE_MESSAGE);
return 1;
}
const outPath = args.outPath || OUTPUT_PATH;
if (flag === '--check') return doCheck();
return doWrite();
return args.mode === 'check' ? doCheck(outPath) : doWrite(outPath);
}
if (require.main === module) process.exitCode = main();

34
scripts/lib/drift-scan.cjs
View File

@@ -61,17 +61,43 @@ const SKIP_DIR_NAMES = new Set(['node_modules', 'dist', '.git']);
* previous regex silently MISSED every re-derivation using a
* cross-platform path-separator class for exactly this reason.
*/
// Deterministic regression seam for the MAJOR-2 bound (issue #3951/#3987): a
// counter of how many characters this tokenizer has actually examined, so a
// test can assert the bound HOLDS (total work stays a small linear multiple
// of the number of scan attempts × MAX_REGEX_LITERAL_LEN) without resorting
// to a wall-clock elapsed-time assertion, which this repo's test rules ban
// ("Clock Seams: Do not assert on wall-clock time.") and which is exactly
// what flaked on a slow shared CI runner. `resetRegexScanStats`/
// `getRegexScanStats` are read-modify-reset around a single scan under test;
// they are process-global and NOT safe under concurrent scans, which is fine
// for this synchronous, single-threaded CLI tool and its tests.
let regexScanStats = { calls: 0, charsExamined: 0 };
function resetRegexScanStats() {
regexScanStats = { calls: 0, charsExamined: 0 };
}
function getRegexScanStats() {
return { ...regexScanStats };
}
function readRegexLiteralAt(line, start) {
if (line[start] !== '/') return null;
regexScanStats.calls++;
const limit = Math.min(line.length, start + MAX_REGEX_LITERAL_LEN);
let inClass = false;
for (let i = start + 1; i < limit; i++) {
let i = start + 1;
for (; i < limit; i++) {
const ch = line[i];
if (ch === '\\') {
i++; // escape consumes the next character, whatever it is
continue;
}
if (ch === '\r' || ch === '\n') return null; // a literal cannot span lines
if (ch === '\r' || ch === '\n') {
// a literal cannot span lines
regexScanStats.charsExamined += i - start;
return null;
}
if (ch === '[') {
inClass = true;
} else if (ch === ']') {
@@ -83,9 +109,11 @@ function readRegexLiteralAt(line, start) {
// MAX_REGEX_LITERAL_LEN either.
let end = i + 1;
while (end < limit && line[end] >= 'a' && line[end] <= 'z') end++;
regexScanStats.charsExamined += end - start;
return { text: line.slice(start, end), end };
}
}
regexScanStats.charsExamined += limit - start;
return null;
}
@@ -275,4 +303,6 @@ module.exports = {
MAX_REGEX_LITERAL_LEN,
sanitizeForReport,
scanTree,
resetRegexScanStats,
getRegexScanStats,
};

921
scripts/lint-slug-derivation-drift.cjs Normal file
View File

@@ -0,0 +1,921 @@
#!/usr/bin/env node
'use strict';
/**
* Anti-divergence drift guard for the SLUG-DERIVATION seam (issue #3987,
* closing epic #3473's last two residuals).
*
* `src/core-utils.cts`'s `generateSlugInternal(text, maxLen)` is the SINGLE
* canonical owner of "turn arbitrary text into a filesystem-safe slug":
* `transliterateForSlug` (lowercase, then a per-character Cyrillic map) ->
* `.replace(/[^a-z0-9]+/g, '-')` -> optional truncation to `maxLen` ->
* `.replace(/^-+|-+$/g, '')` (trim runs AFTER truncation, #2849 — truncating
* first can leave a trailing separator the trim step exists to remove).
* `#3883` deleted 11 hand-inlined copies of the pre-#2849/#2848 shape — a
* single chained expression `.toLowerCase()` -> `.replace(/[^a-z0-9]+/g,
* '-')` -> `.replace(/^-+|-+$/g, '')`, optionally `.substring(0, 60)` —
* none of which transliterated, and all of which trimmed BEFORE truncating.
* This guard is what stops a twelfth copy.
*
* SECURITY-REVIEW HARDENING PASS (post-#3987 review, this same issue). Three
* MAJOR findings from an isolated security review changed how this guard
* works internally; every detail below reflects the FIXED behavior:
*
* MAJOR 1 — exemption scoping was fail-open. The original tracker only
* updated `currentFunction` on a column-0 `function` line and never reset
* it, so an allowlisted function's exemption bled forward into every line
* until the NEXT top-level `function` declaration — a re-derivation
* planted anywhere in that dead zone (measured: 50 exempted lines for an
* 11-line function) silently escaped detection. Fixed by computing each
* allowlisted function's REAL body extent via brace-depth matching on a
* string/comment/template-literal-masked copy of the file
* (`findAllowlistedFunctionExtents` / `maskNonCode` / `maskRegexLiterals`
* below) — a statement is only exempted if it falls strictly inside the
* named function's actual `{ ... }` body, nothing before or after.
*
* MAJOR 2 — the old `CHARCLASS_REPLACE_RE`'s `[^\]]*` character-class body
* was matched directly against the UNBOUNDED joined-statement text, with no
* size limit, and was re-attempted from every `.replace(/[^` occurrence —
* quadratic on a single pathological line (measured 54s at 1.28MB). Fixed
* by routing every regex-literal extraction through
* `drift-scan.cjs`'s `readRegexLiteralAt` (a bounded, non-backtracking,
* single-pass tokenizer this guard imported but never called), so
* classification regexes only ever run against an already-delimited
* literal capped at `MAX_REGEX_LITERAL_LEN` characters. A
* `MAX_FILE_SIZE_BYTES` cap (see below) bounds total scan cost too.
*
* MAJOR 3 — the detector was measurably narrow (15 of 25 known-equivalent
* re-derivation shapes evaded it). Widened, where cheap and
* false-positive-free, to also catch: `replaceAll` (alongside `replace`);
* a small enumerated closed set of trim-regex spellings (`[-]+` character
* class, parenthesized alternatives, `-*` quantifier, `\-` escaped
* hyphen, and alternative order swapped); the `{1,}` quantifier as an
* equivalent of `+`; `.split(<negated class>).join('-')` as an alternate
* collapse mechanism; the literal `new RegExp('[^a-z0-9]+', 'g')` form;
* and call arguments spanning multiple physical lines after `.replace(`
* (not just the existing leading-`.` chain-continuation case) via
* paren-depth-aware statement joining. Deliberately NOT chased (needs data
* flow, not textual matching): `new RegExp` built from a variable, and the
* two-statement/temp-var form — see the guard's test file for both, kept
* as documented known gaps.
*
* DETECTOR (measured against the real deleted shape and the real repo tree —
* see the guard's own test file for the flag/TRUE/SANCTIONED/FALSE census). A
* re-derivation is one logical STATEMENT — not merely one source LINE; a
* chained `.replace()` call is routinely wrapped across several lines by this
* repo's formatter — carrying BOTH:
* (a) a `.replace()`/`.replaceAll()` call whose first argument is a negated
* character class (a `/[^...]+/` regex literal, or the literal form
* `new RegExp('[^...]+', 'flags')`) and whose replacement argument is
* exactly `'-'` — collapsing every non-slug character run to a single
* hyphen — OR a `.split(<same negated class>).join('-')` pair doing the
* same collapse via a different API shape; AND
* (b) a `.replace()`/`.replaceAll()` call whose first argument is one of a
* small enumerated set of hyphen-trim regex spellings and whose
* replacement argument is exactly `''` — trimming leading/trailing
* hyphen runs.
* Both clauses require the SAME replacement discipline as the owner
* (collapse specifically to `'-'`, trim specifically to `''`) — a nearby
* sanitizer that collapses to a DIFFERENT character (e.g. `'_'`) is a
* different derivation, not a copy of this one, and must not fire.
*
* WHY STATEMENT-SCOPED, NOT LINE-SCOPED. A candidate detector that matches
* per LINE (mirroring `lint-phase-enumeration-drift.cjs`'s style) was
* measured against the real tree and rejected: it produced 18 hits, 7 of
* which were unrelated (an unrelated `[^A-Za-z0-9._-]+` filename sanitizer
* sharing a physical line with an unrelated hyphen-trim, and test-fixture
* labels) — a material false-positive rate. Scoping detection to one logical
* statement (joining a chain's continuation lines — those starting with `.`
* — AND any line still inside an unbalanced open `(` from a `.replace(`/
* `.split(` call, back onto the statement that opened it) is what gives this
* guard its precision.
*
* SCOPE. `src/`, `scripts/`, `tests/`, `eslint-rules/` — NOT
* `gsd-core/bin/lib/**` or `bin/install.js`, which are `src/`'s own BUILT
* OUTPUT (via `npm run build:lib` / the installer bundling step): scanning
* them in addition to `src/` would double-count every authored re-derivation
* once for its source and once for its compiled mirror. Both are simply
* absent from SCAN_DIRS below, so no extra exclusion logic is needed.
*
* SANCTIONED EXEMPTIONS (never a bare denylist — each entry names the exact
* function it exempts and WHY, mirroring `lint-completion-ratio-drift.cjs`'s
* `FUNCTION_SCOPED_EXEMPTIONS`; an unrelated re-derivation added anywhere
* else in these same files, or in a same-named function outside the exact
* scoped file, is still caught — and, post MAJOR-1 fix, so is one added
* AFTER the exempted function's own closing brace):
* - `src/core-utils.cts` `generateSlugInternal` — the canonical owner
* itself. Its char-class collapse and hyphen-trim sit in two DIFFERENT
* statements today, so it escapes this detector BY CONSTRUCTION without
* needing an entry here. Listed explicitly anyway: an IMPLICIT escape is
* a latent bug — a future refactor that folds those two lines into one
* chained statement (functionally a no-op) must not silently make the
* guard start flagging its own owner.
* - `src/gsd2-import.cts` `slugify` — declared deliberately DIFFERENT from
* `generateSlugInternal` by #3883 (a distinct truncation contract: no
* 60-char cap at all, vs the owner's default); it already calls the
* SHARED `transliterateForSlug` primitive, so this is not an independent
* re-derivation of the transliteration step — only of the collapse/trim
* shape it deliberately keeps un-consolidated.
* - `src/runtime-artifact-conversion.cts` `normalizeKimiSkillName` — a Kimi
* runtime skill-name normalizer in a completely different domain (CLI
* skill invocation names, never a `.planning/` phase/plan/milestone
* slug); its negated class (`[^a-z0-9-]`) deliberately PRESERVES
* hyphens (a skill name may already contain them), the opposite of the
* slug seam's contract. Shaped like the re-derivation textually; not one
* by domain.
* - `scripts/generate-package-identity.cjs` `slugifyPackageName` —
* npm-scope-name-to-cache-filename prep. Runs PRE-BUILD (`npm run
* generate:identity`, step 1 of `npm run build`, before `build:lib`
* compiles `src/core-utils.cts`), so it structurally cannot `require()`
* the seam it would otherwise route through.
*
* The tree-walk / root-confinement / regex-literal-tokenizer / sanitizer
* machinery is SHARED with the sibling drift guards via
* `scripts/lib/drift-scan.cjs` (ADR-3180 Decision 4).
*
* KNOWN, ACCEPTED limits of this scan (same tradeoff the sibling drift guards
* document): `new RegExp` built from a variable (not a literal string) and
* the two-statement/temp-var re-derivation form are NOT detected — both need
* real data-flow analysis, which a textual heuristic cannot do safely without
* risking noise; quoted-string and regex-literal recognition is single-line
* only (matching `readRegexLiteralAt`'s own "a literal cannot span lines"
* rule) — a re-derivation whose string/regex argument is itself broken across
* a line via unescaped continuation is left unhandled, the same class of
* tradeoff the sibling drift guards' own per-line scans document.
*/
const path = require('node:path');
const driftScan = require('./lib/drift-scan.cjs');
const { MAX_REGEX_LITERAL_LEN, sanitizeForReport, scanTree, readRegexLiteralAt } = driftScan;
// Authored source across the four surfaces the brief scopes this guard to.
// `gsd-core/bin/lib/**` (src/'s build output) and `bin/install.js` are never
// visited because they are not in this list — see the header comment.
const SCAN_DIRS = ['src', 'scripts', 'tests', 'eslint-rules'];
// `.mjs`/`.tsx`/`.jsx` added (MINOR fix): the original set silently never
// opened any file with these extensions under the scanned roots at all — not
// merely "no violations found", but genuinely unread.
const SCAN_EXT = new Set(['.cts', '.ts', '.mts', '.mjs', '.cjs', '.js', '.tsx', '.jsx']);
// MAJOR 2 defense-in-depth: an upper bound on the SIZE of any single file
// this guard will read and scan, independent of the bounded-tokenizer fix
// below. Every real file under SCAN_DIRS today is well under 200KB; 2MB is
// ~10x headroom over the largest legitimate source file in this repo, while
// still bounding the worst-case per-file cost a maliciously huge tracked
// file (e.g. a generated fixture accidentally checked in with a scanned
// extension) could impose — the bounded tokenizer fix makes a 1.28MB file
// fast (see the guard's own test file for the measured timing), but nothing
// stops a fork PR from adding a 50MB one, so a hard cap remains cheap
// insurance. Files over the cap are skipped (not flagged) — same "silently
// unreadable" treatment `scanTree` already gives a file it cannot open.
const MAX_FILE_SIZE_BYTES = 2 * 1024 * 1024;
// This guard's OWN unit-test file is a categorically different case from
// every other FUNCTION_SCOPED_EXEMPTIONS entry below: scanning `tests/` for
// REAL re-derivations (the whole reason this guard covers `tests/` at all —
// #3987's two TRUE positives were `scripts/qa-smell-ratchet.cjs` and
// `tests/planning-inspect.test.cjs`) means this guard's own fixtures —
// LITERAL STRINGS handed to `findSlugDerivationDrift` to prove it detects
// the real deleted #3883 shape — textually match the exact pattern they
// exist to demonstrate. They never execute as a real slug derivation at
// runtime; they are detector test data, the same role `RuleTester` fixtures
// play for an ESLint rule's own test file. Exempting this ONE file by path
// is not a loophole for a real re-derivation (every OTHER file in `tests/`
// remains fully covered) — it is what lets the detector's positive-match
// tests exist at all without permanently reporting themselves as findings.
const SELF_TEST_FILE = path.join('tests', 'slug-derivation-drift-guard.test.cjs');
// Upper bound on a quoted-string argument this scanner will extract (e.g. a
// `.replace()` replacement argument). Mirrors MAX_REGEX_LITERAL_LEN's
// reasoning: every real replacement value this detector cares about (`'-'`,
// `''`) is one or zero characters, so this bound is pure headroom, never a
// real constraint, and it keeps `readQuotedStringAt` a bounded, linear,
// non-backtracking scan with no size-dependent cost.
const MAX_QUOTED_STRING_LEN = 200;
// Optional `export ` modifier, mirroring the sibling guards' function
// tracker — only a column-0 top-level `function` declaration is a candidate
// for a FUNCTION_SCOPED_EXEMPTIONS entry. (Its extent, once matched, is
// computed precisely via brace-depth — see `findAllowlistedFunctionExtents`
// — not merely "until the next line matching this regex", which was
// MAJOR-1's fail-open bug.)
const TOP_LEVEL_FUNCTION_RE = /^(?:export\s+)?function\s+([A-Za-z0-9_]+)\s*\(/;
// Per the header comment: NOT a bare file allowlist — each entry is scoped
// to the SPECIFIC function, with its reason recorded above (mirroring
// `lint-completion-ratio-drift.cjs`'s `FUNCTION_SCOPED_EXEMPTIONS`). An
// unrelated re-derivation added anywhere else in these same files — INCLUDING
// after the named function's own closing brace — is still caught (MAJOR 1).
const FUNCTION_SCOPED_EXEMPTIONS = new Map([
[path.join('src', 'core-utils.cts'), new Set(['generateSlugInternal'])],
[path.join('src', 'gsd2-import.cts'), new Set(['slugify'])],
[path.join('src', 'runtime-artifact-conversion.cts'), new Set(['normalizeKimiSkillName'])],
[path.join('scripts', 'generate-package-identity.cjs'), new Set(['slugifyPackageName'])],
]);
// ─── Bounded, string/regex/comment-aware line tokenizer ───────────────────
//
// Every helper in this section works on ONE physical line (or a bounded
// slice of text) and is a straight left-to-right scan with no backtracking —
// the same non-catastrophic shape as `readRegexLiteralAt` in drift-scan.cjs,
// which this section reuses directly rather than re-deriving its own
// (weaker) character-class matcher, which is exactly the class of mistake
// MAJOR 2 found.
/**
* Read a single/double/backtick-quoted string literal starting at
* `text[start]`. Returns `{ text, inner, end }` (`text` includes the quotes,
* `inner` is the content between them, `end` is one past the closing quote)
* or `null` if no matching close is found within `MAX_QUOTED_STRING_LEN`
* characters or before a newline — a quoted string, like a regex literal,
* is not expected to span a line in the shapes this detector cares about.
*/
function readQuotedStringAt(text, start) {
const quote = text[start];
if (quote !== "'" && quote !== '"' && quote !== '`') return null;
const limit = Math.min(text.length, start + MAX_QUOTED_STRING_LEN);
let i = start + 1;
while (i < limit) {
const ch = text[i];
if (ch === '\\') {
i += 2; // escape consumes the next character, whatever it is
continue;
}
if (ch === '\n') return null;
if (ch === quote) return { text: text.slice(start, i + 1), inner: text.slice(start + 1, i), end: i + 1 };
i++;
}
return null;
}
/**
* Heuristic used to disambiguate a `/` as the START of a regex literal
* (rather than division/a closing comment marker) — the standard
* "what came before it" tokenizer rule: a regex may open at the start of a
* line, or right after an operator/punctuation/`return` that can only be
* followed by an expression, never a value. This is the same disambiguation
* every one of this detector's real call sites (`.replace(/…/`,
* `.split(/…/`) always satisfies (the char before `/` is always `(` or `,`),
* so a conservative heuristic here costs nothing in practice.
*/
// Bound on the trailing-context buffer `looksLikeRegexStart` inspects. Long
// enough to see the word "return" plus a little slack; deliberately NOT the
// full accumulated output — see `looksLikeRegexStart`'s own comment for why
// that distinction is load-bearing (MAJOR-2 regression class).
const REGEX_START_TAIL_LEN = 10;
/**
* `precedingTail` is a BOUNDED trailing slice of the text scanned so far
* (see `REGEX_START_TAIL_LEN`), never the full accumulated output. This
* matters: an earlier draft of this heuristic re-derived the tail from the
* FULL output string on every `/` encountered, which is exactly MAJOR 2's
* bug shape reintroduced one level up — `String.prototype.replace` on an
* ever-growing string, called once per `/` in the input, is quadratic on a
* long line with many `/` characters (measured: a 2.88MB adversarial line
* took 12.5s with a full-string tail; a bounded tail is O(1) per call
* regardless of total input size). Every CALLER of this function is
* responsible for maintaining `precedingTail` as a small rolling buffer.
*/
function looksLikeRegexStart(precedingTail) {
const trimmed = precedingTail.replace(/\s+$/, '');
if (trimmed.length === 0) return true;
const last = trimmed[trimmed.length - 1];
if ('(,=:;[!&|?+-*%{'.includes(last)) return true;
return trimmed.endsWith('return');
}
/** Bounded (O(1) w.r.t. total accumulated text) update of a rolling tail buffer. */
function updateTail(tail, appended) {
return (tail + appended).slice(-REGEX_START_TAIL_LEN);
}
/**
* Scan one physical line, string/regex-literal-aware, producing:
* - `text`: the line with any trailing `//` line-comment removed (a `//`
* found INSIDE a string or regex literal, e.g. `'http://x'`, is not a
* comment — MINOR fix: the previous version cut at the first `//`
* unconditionally);
* - `parenDelta`: net `(` minus `)` count, skipping any that appear inside
* a string or regex literal (so `.replace(/[)]/g, ')')`'s internal
* parens/regex content never desyncs a caller's paren-depth tracking);
* - `semicolons`: offsets (into `text`) of every top-level `;` — one NOT
* inside a string or regex literal (MINOR fix: the previous version did
* a naive `line.split(';')`, so a `;` embedded in a regex character
* class, e.g. `/[^a-z0-9;]+/`, wrongly split one statement into two).
*/
function scanLineTokens(line) {
const n = line.length;
let i = 0;
let out = '';
let tail = ''; // bounded rolling context for looksLikeRegexStart — see its comment
let parenDelta = 0;
const semicolons = [];
while (i < n) {
const ch = line[i];
if (ch === "'" || ch === '"' || ch === '`') {
const str = readQuotedStringAt(line, i);
if (str) {
out += str.text;
tail = updateTail(tail, str.text);
i = str.end;
continue;
}
// Unterminated within bound/line: fail safe by consuming the rest of
// the line as opaque text rather than re-entering character-by-character
// scanning mid-string (which could misparse quote-internal punctuation
// as code).
out += line.slice(i);
break;
}
if (ch === '/' && line[i + 1] === '/') break; // real line comment (not inside a string — handled above)
if (ch === '/' && looksLikeRegexStart(tail)) {
const lit = readRegexLiteralAt(line, i);
if (lit) {
out += lit.text;
tail = updateTail(tail, lit.text);
i = lit.end;
continue;
}
}
if (ch === '(') parenDelta++;
else if (ch === ')') parenDelta--;
else if (ch === ';') semicolons.push(out.length);
out += ch;
tail = updateTail(tail, ch);
i++;
}
return { text: out, parenDelta, semicolons };
}
/**
* Strip comment text from a line, string-literal-aware (MINOR fix). Full
* doc-comment lines (`*`/`/**`-prefixed, or a bare `//` line) are blanked
* outright, matching the previous behavior for this repo's jsdoc shape
* (every line of a block comment here starts with `*`); anything else is run
* through `scanLineTokens`, which only treats a `//` as a comment marker
* when it is not inside a string or regex literal.
*/
function stripComments(line) {
const trimmed = line.trim();
if (trimmed.startsWith('*') || trimmed.startsWith('/*') || trimmed.startsWith('//')) return '';
return scanLineTokens(line).text;
}
/**
* Join a chained method call's continuation lines (those whose trimmed,
* comment-stripped text starts with `.`) back onto the line that opened the
* chain, producing one "logical statement" per opening line; split a single
* physical line into multiple statements at each top-level `;` (string/regex
* -aware, see `scanLineTokens`); AND (MAJOR-3 widen) keep merging any
* following line/fragment, regardless of whether it starts with `.`, while
* the statement's own paren-depth (also string/regex-aware) is still open —
* this is what recognizes a `.replace(`/`.split(` call whose arguments were
* wrapped across lines WITHOUT a leading-`.` continuation on each one, e.g.:
*
* x.replace(
* /[^a-z0-9]+/g,
* '-'
* ).replace(/^-+|-+$/g, '')
*
* A statement is only finalized (pushed, and merging stops) at a top-level
* `;` once its own paren-depth has returned to zero — a `;` that appears
* while still inside an open `(` (not a real shape for this detector's
* `.replace()`/`.split()` call sites, but handled defensively) does not
* split the statement.
*
* Returns `[{ startLine, text }]` — `startLine` is 1-based, matching the
* sibling guards' reporting convention.
*/
function buildLogicalStatements(lines) {
const statements = [];
let current = null; // { startLine, text, openDepth }
for (let i = 0; i < lines.length; i++) {
const trimmedRaw = lines[i].trim();
if (trimmedRaw.startsWith('*') || trimmedRaw.startsWith('/*') || trimmedRaw.startsWith('//')) continue; // full-line comment
const { text: strippedLine, semicolons } = scanLineTokens(lines[i]);
if (!strippedLine.trim()) continue; // blank/comment-only lines never break or start a statement
const rawFragments = [];
let cursor = 0;
for (const pos of semicolons) {
rawFragments.push(strippedLine.slice(cursor, pos));
cursor = pos + 1;
}
rawFragments.push(strippedLine.slice(cursor));
const fragments = rawFragments.map((f) => f.trim()).filter((f) => f.length > 0);
for (let f = 0; f < fragments.length; f++) {
const frag = fragments[f];
const isFirstFragmentOfLine = f === 0;
const isLastFragmentOfLine = f === fragments.length - 1;
const midOpenParen = current !== null && current.openDepth > 0;
if (isFirstFragmentOfLine && current && (midOpenParen || frag.startsWith('.'))) {
current.text += ' ' + frag;
} else {
if (current) statements.push(current);
current = { startLine: i + 1, text: frag, openDepth: 0 };
}
current.openDepth += scanLineTokens(frag).parenDelta;
// A fragment that is not the LAST one on its line was terminated by a
// top-level `;` immediately after it. It is a complete statement no
// later fragment may merge into, UNLESS it is (defensively) still
// inside an open paren — see the doc comment above.
if (!isLastFragmentOfLine && current.openDepth <= 0) {
statements.push(current);
current = null;
}
}
}
if (current) statements.push(current);
return statements;
}
// ─── Collapse / trim classification (operates on an EXTRACTED, bounded
// regex-literal body — never on unbounded raw text; this is the MAJOR-2 fix)
// A negated character class collapsed to a single hyphen — the class body is
// matched escape-aware (`\\.` or any non-`]`/non-`\` char), which is what
// lets `[^a-z0-9\]]` (an escaped `]` inside the class) parse correctly; the
// previous `[^\]]*` body matcher broke on exactly this shape (MINOR fix).
// Quantifier may be `+`, `*`, `{1,}` (MAJOR-3 widen: `{1,}` is `+`'s
// equivalent), or absent; an optional `\s*` may sit on either side of the
// class (MAJOR-3 widen). All bounded: this runs against an already-extracted
// literal body capped at MAX_REGEX_LITERAL_LEN, never unbounded text.
const COLLAPSE_BODY_RE = /^(?:\\s\*)?\[\^(?:\\.|[^\]\\])*\](?:[+*]|\{1,\})?(?:\\s\*)?$/;
function isCollapseBody(body) {
return COLLAPSE_BODY_RE.test(body);
}
/**
* Normalize the small enumerated set of equivalent hyphen-trim regex
* spellings (MAJOR-3 widen) down to a canonical `^-<quant>|-<quant>$` shape
* (in either order) before comparing: unwraps a `(^-+)`/`(-+$)` parenthesized
* alternative, unescapes a literal `\-` to `-`, and collapses a `[-]`
* single-hyphen character class to a bare `-`. Deliberately a small,
* enumerated normalization — not a permissive catch-all regex — per the
* review's instruction to widen ONLY where the resulting shape is a closed,
* auditable set.
*/
function canonicalizeTrimBody(rawBody) {
return rawBody
.replace(/\((\^[^)]*)\)/g, '$1')
.replace(/\(([^)]*\$)\)/g, '$1')
.replace(/\\-/g, '-')
.replace(/\[-\]/g, '-');
}
function isTrimBodyPart(part, anchor) {
return anchor === 'start' ? /^\^-[+*]?$/.test(part) : /^-[+*]?\$$/.test(part);
}
function isTrimBody(rawBody) {
const parts = canonicalizeTrimBody(rawBody).split('|');
if (parts.length !== 2) return false;
const [p1, p2] = parts;
return (
(isTrimBodyPart(p1, 'start') && isTrimBodyPart(p2, 'end')) ||
(isTrimBodyPart(p2, 'start') && isTrimBodyPart(p1, 'end'))
);
}
/** Split a bounded `/body/flags` regex-literal text into `{ body, flags }`, or null. */
function splitRegexLiteral(literalText) {
const m = /^\/(.*)\/([a-z]*)$/.exec(literalText);
return m ? { body: m[1], flags: m[2] } : null;
}
/**
* Parse the literal-argument form `new RegExp('pattern'[, 'flags'])` starting
* at `text[start]` (which must be the `n` of `new`). Only LITERAL string
* arguments are handled — per the review's explicit instruction, a variable
* built into `new RegExp(...)` needs data-flow analysis this textual scanner
* does not attempt, and is a documented known gap, not silently mishandled.
* Returns `{ body, flags, end }` or null.
*/
function parseNewRegExpLiteral(text, start) {
if (text.slice(start, start + 10) !== 'new RegExp') return null;
let i = start + 10;
while (i < text.length && /\s/.test(text[i])) i++;
if (text[i] !== '(') return null;
i++;
while (i < text.length && /\s/.test(text[i])) i++;
const patternArg = readQuotedStringAt(text, i);
if (!patternArg) return null;
i = patternArg.end;
while (i < text.length && /\s/.test(text[i])) i++;
let flags = '';
if (text[i] === ',') {
i++;
while (i < text.length && /\s/.test(text[i])) i++;
const flagsArg = readQuotedStringAt(text, i);
if (flagsArg) {
flags = flagsArg.inner;
i = flagsArg.end;
while (i < text.length && /\s/.test(text[i])) i++;
}
}
if (text[i] !== ')') return null;
return { body: patternArg.inner, flags, end: i + 1 };
}
/**
* Find every `.replace(...)`/`.replaceAll(...)` call in `text` (MAJOR-3
* widen: `replaceAll` alongside `replace`) whose first argument is a
* recognizable regex (a `/…/` literal OR the literal `new RegExp(...)` form)
* and whose second argument is a quoted string, returning
* `[{ body, flags, replacement }]`. All literal extraction is bounded
* (`readRegexLiteralAt`/`readQuotedStringAt`), so this is safe to run against
* a long statement text — the MAJOR-2 fix.
*/
function findReplaceCalls(text) {
const calls = [];
const callRe = /\.(replace|replaceAll)\(/g;
while (callRe.exec(text)) {
let idx = callRe.lastIndex;
while (idx < text.length && /\s/.test(text[idx])) idx++;
let regexInfo = null;
if (text[idx] === '/') {
const lit = readRegexLiteralAt(text, idx);
if (lit) {
const split = splitRegexLiteral(lit.text);
if (split) {
regexInfo = split;
idx = lit.end;
}
}
} else if (text.slice(idx, idx + 10) === 'new RegExp') {
const parsed = parseNewRegExpLiteral(text, idx);
if (parsed) {
regexInfo = { body: parsed.body, flags: parsed.flags };
idx = parsed.end;
}
}
if (!regexInfo) continue;
while (idx < text.length && /[\s,]/.test(text[idx])) idx++;
const replacementArg = readQuotedStringAt(text, idx);
calls.push({ regexInfo, replacement: replacementArg ? replacementArg.inner : null });
}
return calls;
}
/**
* MAJOR-3 widen: `.split(<negated class>).join('-')` is an alternate way to
* express the SAME collapse-to-hyphen shape as
* `.replace(<negated class>, '-')`. Only the literal-regex `.split(/…/)` form
* is handled (mirrors `findReplaceCalls`'s own `new RegExp` literal-only
* scope for the same textual-scan-cannot-do-data-flow reason).
*/
function findSplitJoinCollapse(text) {
const splitRe = /\.split\(\s*/g;
while (splitRe.exec(text)) {
const argStart = splitRe.lastIndex;
if (text[argStart] !== '/') continue;
const lit = readRegexLiteralAt(text, argStart);
if (!lit) continue;
let after = lit.end;
while (after < text.length && /\s/.test(text[after])) after++;
if (text[after] !== ')') continue;
after++;
const joinMatch = /^\s*\.join\(\s*(['"`])-\1\s*\)/.exec(text.slice(after, after + 32));
if (!joinMatch) continue;
const split = splitRegexLiteral(lit.text);
if (split && isCollapseBody(split.body)) return true;
}
return false;
}
/** True if `stmtText` (one logical statement) carries both the collapse and trim clauses. */
function statementHasSlugDerivation(stmtText) {
let hasCollapse = false;
let hasTrim = false;
for (const call of findReplaceCalls(stmtText)) {
if (call.replacement === null) continue;
if (call.replacement === '-' && isCollapseBody(call.regexInfo.body)) hasCollapse = true;
if (call.replacement === '' && isTrimBody(call.regexInfo.body)) hasTrim = true;
}
if (!hasCollapse) hasCollapse = findSplitJoinCollapse(stmtText);
return hasCollapse && hasTrim;
}
// ─── MAJOR-1 fix: precise allowlisted-function body extent ────────────────
//
// Computes the REAL `{ ... }` body range of each allowlisted function via
// brace-depth matching on a masked copy of the file (comments, strings, and
// template literals replaced with same-length whitespace/newlines) — not
// "from this column-0 `function` line until the next one", which is what let
// the exemption bleed past the function it names.
/** Find the index one past a single/double-quoted string starting at `start`, masking is caller's job. Multi-line-safe (unlike readQuotedStringAt, which is intentionally single-line for the DETECTOR's own bounded-scan needs). */
function findQuotedEndMultiline(text, start) {
const quote = text[start];
const n = text.length;
let i = start + 1;
while (i < n) {
if (text[i] === '\\') {
i += 2;
continue;
}
if (text[i] === quote) return i + 1;
i++;
}
return n;
}
/**
* Find the index one past a backtick template literal starting at `start`,
* recursively skipping nested `${ ... }` substitutions (which may themselves
* contain nested templates/strings/comments/braces). Every brace opened
* inside a substitution closes inside that SAME substitution (it is valid
* JS/TS), so masking the whole template literal — substitutions included —
* as non-code is safe for the purpose of an ENCLOSING function's brace-depth
* extent: any braces inside it are locally balanced and net to zero either
* way.
*/
function findTemplateEnd(text, start) {
const n = text.length;
let i = start + 1;
const substitutionDepths = [];
while (i < n) {
if (text[i] === '\\') {
i += 2;
continue;
}
if (substitutionDepths.length === 0) {
if (text[i] === '`') return i + 1;
if (text[i] === '$' && text[i + 1] === '{') {
substitutionDepths.push(1);
i += 2;
continue;
}
i++;
continue;
}
if (text[i] === '`') {
i = findTemplateEnd(text, i);
continue;
}
if (text[i] === "'" || text[i] === '"') {
i = findQuotedEndMultiline(text, i);
continue;
}
if (text[i] === '/' && text[i + 1] === '/') {
while (i < n && text[i] !== '\n') i++;
continue;
}
if (text[i] === '/' && text[i + 1] === '*') {
const j = text.indexOf('*/', i + 2);
i = j === -1 ? n : j + 2;
continue;
}
if (text[i] === '{') {
substitutionDepths[substitutionDepths.length - 1]++;
i++;
continue;
}
if (text[i] === '}') {
substitutionDepths[substitutionDepths.length - 1]--;
if (substitutionDepths[substitutionDepths.length - 1] === 0) substitutionDepths.pop();
i++;
continue;
}
i++;
}
return n; // unterminated -> EOF (fail-closed: masked to end of file, never past it)
}
/**
* Replace every comment, string, and template literal in `text` with
* same-length whitespace (preserving newlines, so downstream line-number math
* stays correct), leaving all other characters — including every REAL code
* brace/paren — untouched.
*/
function maskNonCode(text) {
const n = text.length;
let out = '';
let i = 0;
while (i < n) {
const two = text.slice(i, i + 2);
if (two === '//') {
let j = i;
while (j < n && text[j] !== '\n') j++;
out += ' '.repeat(j - i);
i = j;
continue;
}
if (two === '/*') {
const found = text.indexOf('*/', i + 2);
const j = found === -1 ? n : found + 2;
for (let k = i; k < j; k++) out += text[k] === '\n' ? '\n' : ' ';
i = j;
continue;
}
const ch = text[i];
if (ch === "'" || ch === '"') {
const j = findQuotedEndMultiline(text, i);
for (let k = i; k < j; k++) out += text[k] === '\n' ? '\n' : ' ';
i = j;
continue;
}
if (ch === '`') {
const j = findTemplateEnd(text, i);
for (let k = i; k < j; k++) out += text[k] === '\n' ? '\n' : ' ';
i = j;
continue;
}
out += ch;
i++;
}
return out;
}
/**
* Second masking pass, run PER LINE (regex literals cannot span lines) over
* text already comment/string/template-masked by `maskNonCode`: masks any
* regex literal so an unbalanced brace inside a character class (e.g.
* `/[{]/`) cannot desync brace-depth counting for an enclosing function.
*/
function maskRegexLiterals(masked) {
return masked
.split('\n')
.map((line) => {
let out = '';
let tail = ''; // bounded rolling context — see looksLikeRegexStart's comment
let i = 0;
while (i < line.length) {
if (line[i] === '/' && looksLikeRegexStart(tail)) {
const lit = readRegexLiteralAt(line, i);
if (lit) {
const masked = ' '.repeat(lit.end - i);
out += masked;
tail = updateTail(tail, masked);
i = lit.end;
continue;
}
}
out += line[i];
tail = updateTail(tail, line[i]);
i++;
}
return out;
})
.join('\n');
}
/** Find the index of the `{`/`}` that closes the one opened at `openIdx` in `masked`, or -1 (unterminated -> caller decides fallback). */
function findMatchingBrace(masked, openIdx) {
let depth = 0;
for (let i = openIdx; i < masked.length; i++) {
if (masked[i] === '{') depth++;
else if (masked[i] === '}') {
depth--;
if (depth === 0) return i;
}
}
return -1;
}
/** Find the index of the `)` that closes the `(` at `openIdx` in `masked`, or -1. */
function findMatchingParen(masked, openIdx) {
let depth = 0;
for (let i = openIdx; i < masked.length; i++) {
if (masked[i] === '(') depth++;
else if (masked[i] === ')') {
depth--;
if (depth === 0) return i;
}
}
return -1;
}
/**
* Compute `{ startLine, endLine }` (1-based, inclusive) for every function in
* `exemptFunctionNames` that is declared as a column-0 top-level
* `function name(` in `text`. Brace-depth matching runs on `masked`
* (comments/strings/templates/regex-literals all masked to whitespace), so
* only REAL code braces/parens are counted — a destructured parameter like
* `function f({ a, b }) { ... }`'s own braces are correctly skipped past via
* paren-matching of the parameter list BEFORE brace-matching begins.
*/
function findAllowlistedFunctionExtents(text, exemptFunctionNames) {
if (!exemptFunctionNames || exemptFunctionNames.size === 0) return [];
const masked = maskRegexLiterals(maskNonCode(text));
const lines = text.split('\n');
const lineStartOffsets = [];
let offset = 0;
for (const line of lines) {
lineStartOffsets.push(offset);
offset += line.length + 1; // +1 for the '\n' split removed
}
const extents = [];
for (let i = 0; i < lines.length; i++) {
const m = TOP_LEVEL_FUNCTION_RE.exec(lines[i]);
if (!m || !exemptFunctionNames.has(m[1])) continue;
const declStart = lineStartOffsets[i];
const parenIdx = masked.indexOf('(', declStart);
if (parenIdx === -1) continue;
const parenEnd = findMatchingParen(masked, parenIdx);
if (parenEnd === -1) continue;
const braceIdx = masked.indexOf('{', parenEnd);
if (braceIdx === -1) continue;
const braceEnd = findMatchingBrace(masked, braceIdx);
const endOffset = braceEnd === -1 ? masked.length - 1 : braceEnd;
let endLine = lines.length - 1;
for (let li = 0; li < lineStartOffsets.length; li++) {
if (lineStartOffsets[li] > endOffset) {
endLine = li - 1;
break;
}
}
extents.push({ name: m[1], startLine: i + 1, endLine: endLine + 1 });
}
return extents;
}
/**
* Pure: find every unsanctioned slug-derivation re-derivation in `text`.
* `relPath` is the repo-relative path, used both to report file:line and to
* apply the narrow, function-scoped exemptions above.
* Returns [{ line, found }].
*/
function findSlugDerivationDrift(text, relPath) {
const out = [];
const lines = text.split('\n');
const exemptFunctions = FUNCTION_SCOPED_EXEMPTIONS.get(relPath) || null;
const exemptExtents = exemptFunctions ? findAllowlistedFunctionExtents(text, exemptFunctions) : [];
for (const stmt of buildLogicalStatements(lines)) {
if (!statementHasSlugDerivation(stmt.text)) continue;
const inExemptExtent = exemptExtents.some(
(ext) => stmt.startLine >= ext.startLine && stmt.startLine <= ext.endLine,
);
if (inExemptExtent) continue;
out.push({ line: stmt.startLine, found: stmt.text.slice(0, MAX_REGEX_LITERAL_LEN) });
}
return out;
}
/**
* Scan the authored source tree and return every unsanctioned re-derivation,
* each annotated with the repo-relative file path.
*/
function scanRepo(root) {
return scanTree({
root,
scanDirs: SCAN_DIRS,
scanExt: SCAN_EXT,
onFile(rel, text) {
if (rel === SELF_TEST_FILE) return []; // see SELF_TEST_FILE's own comment above
if (Buffer.byteLength(text, 'utf8') > MAX_FILE_SIZE_BYTES) return []; // see MAX_FILE_SIZE_BYTES's own comment above
return findSlugDerivationDrift(text, rel).map((d) => ({ file: rel, ...d }));
},
});
}
function main() {
const root = path.join(__dirname, '..');
const violations = scanRepo(root);
if (violations.length === 0) {
process.stdout.write('ok slug-derivation-drift: no unsanctioned slug re-derivations outside core-utils.cts generateSlugInternal\n');
return;
}
process.stderr.write('slug-derivation-drift: independent re-derivation(s) of the slug-generation seam found.\n');
process.stderr.write('Use src/core-utils.cts `generateSlugInternal(text, maxLen)` instead of re-deriving\n');
process.stderr.write('the collapse/trim (or transliterate/collapse/trim) slug shape:\n');
for (const d of violations) {
// `d.file` is exactly as attacker-controlled as `d.found`: a repo can
// legally track a filename containing control bytes / bidi overrides,
// and it is a fork-PR-authored value reaching a CI log the same way the
// matched statement text does — sanitize it at the same reporting
// boundary.
process.stderr.write(` ${sanitizeForReport(d.file)}:${d.line} ${sanitizeForReport(d.found)}\n`);
}
process.exitCode = 1;
}
if (require.main === module) main();
module.exports = {
findSlugDerivationDrift,
scanRepo,
buildLogicalStatements,
stripComments,
scanLineTokens,
isCollapseBody,
isTrimBody,
COLLAPSE_BODY_RE,
findAllowlistedFunctionExtents,
FUNCTION_SCOPED_EXEMPTIONS,
SCAN_DIRS,
SCAN_EXT,
SELF_TEST_FILE,
MAX_FILE_SIZE_BYTES,
};

42
scripts/qa-smell-ratchet.cjs
View File

@@ -391,13 +391,43 @@ function collectFindings(reportObject) {
return { smells, violations, expectationFailures };
}
/** Lowercase, hyphenate, and strip anything that isn't `[a-z0-9-]`, for a fragment-filename skeleton. */
/**
* Lowercase, transliterate, hyphenate, and strip anything that isn't
* `[a-z0-9-]`, for a fragment-filename skeleton.
*
* Routed through the canonical `generateSlugInternal` seam (`src/core-utils.cts`,
* issue #3987) instead of hand-rolling the same collapse/strip/truncate shape:
* this local copy trimmed leading/trailing hyphens BEFORE truncating to 60
* chars, which is the live #2849 bug (`.slice(0, 60)` can land on a separator,
* re-introducing a trailing hyphen the strip step was meant to prevent), and
* it never transliterated non-Latin scripts (#2848). `generateSlugInternal`
* returns `null` for empty/nullish input; a fragment-filename skeleton needs a
* string, so `?? ''` preserves this function's prior never-null contract.
*
* `gsd-core/bin/lib/core-utils.cjs` is required LAZILY, here, rather than at
* module load — it is `src/core-utils.cts`'s gitignored `build:lib` output,
* so a top-level `require` made this ENTIRE script (including `--help`, which
* never calls `slugify`) hard-fail `MODULE_NOT_FOUND` on a fresh clone before
* any build ran. Deferring the require to the one call site that actually
* needs it means every other code path (in particular `--help`) still works
* with `gsd-core/bin/lib/` absent, and a genuinely missing build only surfaces
* as an error when a NEW smell finding is rendered (the only caller of this
* function).
*/
function slugify(value) {
return value
.toLowerCase()
.replace(/[^a-z0-9]+/g, '-')
.replace(/^-+|-+$/g, '')
.slice(0, 60);
let generateSlugInternal;
try {
({ generateSlugInternal } = require('../gsd-core/bin/lib/core-utils.cjs'));
} catch (err) {
if (err && err.code === 'MODULE_NOT_FOUND') {
throw new ExitError(
1,
'qa-smell-ratchet: gsd-core/bin/lib/core-utils.cjs is missing — run `npm run build:lib` first.',
);
}
throw err;
}
return generateSlugInternal(value, 60) ?? '';
}
/**