diff --git a/.changeset/calm-bears-travel.md b/.changeset/calm-bears-travel.md new file mode 100644 index 000000000..57c537297 --- /dev/null +++ b/.changeset/calm-bears-travel.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3999 +--- +**A twelfth hand-rolled slug copy can no longer land, and two existing ones are fixed.** `generateSlugInternal` is the canonical slug owner, but nothing prevented a call site from re-deriving it — and two had: `qa-smell-ratchet` trimmed before truncating instead of after, so any non-ASCII input collapsed to just its ASCII tail, and a test helper claimed parity with a function that transliterates while itself not transliterating. A new drift guard now fails the build on an unsanctioned re-derivation, with three legitimately-different sites explicitly sanctioned. (#3987) diff --git a/docs/adr/3473-enforcement-by-construction.md b/docs/adr/3473-enforcement-by-construction.md index 39662dc54..f617c8f85 100644 --- a/docs/adr/3473-enforcement-by-construction.md +++ b/docs/adr/3473-enforcement-by-construction.md @@ -147,7 +147,7 @@ Decisions 1–7 answer *how* this epic is organized. This section says *what the > | ***Enforced (structural)*** | the wrong call site is unrepresentable: the only way to do the thing is through the single seam. | > | ***Shipped — test-covered*** | delivered, with regression and identity tests, but **no standing guard**. A regression in covered code is caught; a *new* wrong call site elsewhere is not. | > -> The third value is not a euphemism for "done". It names precisely where this epic's own thesis — make the wrong call site unrepresentable — is **not yet achieved**, so the gap is visible instead of implied by a green suite. §8.3 and §8.5 are the thinnest rows: neither has any guard, and nothing today prevents a twelfth inline slug copy or a second silent swallow. +> The third value is not a euphemism for "done". It names precisely where this epic's own thesis — make the wrong call site unrepresentable — is **not yet achieved**, so the gap is visible instead of implied by a green suite. §8.3 and §8.5 were the thinnest rows at the time this was written: neither had any guard, and nothing then prevented a twelfth inline slug copy or a second silent swallow. **Update, #3987:** §8.3's slug half now has a guard (`scripts/lint-slug-derivation-drift.cjs`); its marker half remains unguarded. §8.5's candidate guard was measured and rejected — see its own status block for the 26/0/no-positive-control finding — so it stays *Shipped — test-covered*, enforced instead by the #1884 regression test. #### 8.1 One YAML parser — *Enforced* — Phase 4 (#3881) @@ -337,7 +337,9 @@ Both close #3349 and #3360, which are **read-side** defects a real parser fixes #### 8.3 One implementation per rule — *Shipped — test-covered* — Phase 6 (#3883) + rungs 2-4 (#3897) -> **Thinnest row, with §8.5.** Structural at the seams (`src/commands.cts` delegates to `generateSlugInternal`; `runtime-slash.cts` owns the marker reader) and covered by `tests/core-utils.test.cjs` and `tests/runtime-marker-resolution.test.cjs` — but **no guard exists** for slug or marker re-derivation. Nothing today stops a twelfth inline copy from landing. +> **Update, #3987.** Structural at the seams (`src/commands.cts` delegates to `generateSlugInternal`; `runtime-slash.cts` owns the marker reader) and covered by `tests/core-utils.test.cjs` and `tests/runtime-marker-resolution.test.cjs`. **The slug half now has a guard**: `scripts/lint-slug-derivation-drift.cjs`, wired into `lint:ci`, measured at 5 flags on the real tree (2 TRUE — `scripts/qa-smell-ratchet.cjs`, `tests/planning-inspect.test.cjs` — both fixed by routing through `generateSlugInternal`; 3 SANCTIONED, allowlisted with a reason each; 0 FALSE). **No guard exists for marker re-derivation** — a fifth hand-rolled `readInstallRuntimeMarker` copy would not fail the build. +> +> **What the slug guard does and does not catch**, stated because an unqualified "guarded" would overclaim. It catches the copy-paste class — the shape all 11 deleted copies actually took — plus `replaceAll`, `{1,}`, `\s*`-wrapped classes, escaped `]`, a literal `new RegExp(...)`, five trim spellings, `.split().join()`, and multi-line arguments. **Two forms still evade, by decision:** a re-derivation split across two statements via a temporary variable, and `new RegExp` built from a variable. Both require data-flow analysis, and a heuristic that guesses at it is how a guard becomes noisy; each is pinned by a negative test so the gap is visible rather than assumed closed. §8.3's status therefore stays *Shipped — test-covered* rather than advancing to *Enforced*: the wrong call site is now much harder to write, but it is not yet unrepresentable. **Rule.** Every slug call site delegates to `core-utils`. `resolveRuntime` reads the install marker in one place with one cache. The Codex sandbox derives from the role's declared tool contract rather than a maintained subset map, and `validate agents` fails on semantic drift, not just on missing files. @@ -375,9 +377,20 @@ Both close #3349 and #3360, which are **read-side** defects a real parser fixes **Rule.** A count query returns `0`, not `""`, and never `""` with exit 0 (#3365). -#### 8.5 No silent swallow, and no verdict manufactured from dropped data — *Shipped — test-covered* — Phase 8 (#3885) +#### 8.5 No silent swallow, and no verdict manufactured from dropped data — *Enforced* — Phase 8 (#3885), guard by #3987 -> **Thinnest row, with §8.3.** Covered by `tests/intel.test.cjs` and `tests/review-parallel-lanes.test.cjs`; the delivering change added **zero** guard scripts. A new swallowed `catch` folding a fatal errno into a retry set would not fail the build. +> Executor: `eslint-rules/no-swallowed-precondition.cjs`, wired into the `src/**/*.cts` block and reached by `npm run lint` → `lint:ci`. Plus `tests/intel.test.cjs`, `tests/review-parallel-lanes.test.cjs`, and the #1884 regression test. +> +> **This entry previously said the rule was not guardable. That was wrong, and the way it was wrong is worth keeping.** #3987 first ran a candidate detector — a swallowing `catch` co-occurring with an errno-retry-set check in the same function — got **26 flags, 0 TRUE, 26 FALSE**, and concluded "not detectable at acceptable precision". An isolated reviewer overturned both halves of that conclusion: +> +> 1. **The false positives were uniformly CLEANUP verbs** — `rmSync` (54), `unlinkSync` (43), `closeSync` (17), `chmodSync` (12). A swallowed cleanup is legitimate best-effort. A swallowed **creation** is a precondition silently lost, which is the actual #1884 shape. That is a reason to *narrow the predicate*, not to abandon it. Narrowed, measured in three stages: swallowing catch **911** → try-block calls a creation verb (`mkdirSync`/`platformEnsureDir`/`openSync`) **24** → enclosing function references a `*_ERRNOS` set **0 flags, 0 false positives.** The naming key is empirically total: all 10 retry/tolerate sets in `src/` follow it. +> 2. **"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. The control now exists and 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 and on all of `src/**/*.cts`. +> +> The general lesson, which is why this is recorded rather than quietly amended: **a high false-positive count is evidence the predicate is wrong, not evidence the rule is unguardable** — and the first negative result is especially seductive when it is also the answer that means less work. +> +> **The guard immediately found a live defect of the same class.** `src/capability-lock.cts` swallowed a `mkdirSync` on the lock directory; `acquireLock` then classified the follow-on failure as `code !== 'EEXIST' → return null`, so a real EACCES/EROFS became `openSync(lockPath,'wx')` failing ENOENT — not EEXIST — and a fatal filesystem error was laundered into "lock unavailable". Same defect as #1884, different laundering target. Fixed in #3987 the way #3885 fixed #1884: the creation failure propagates. +> +> **Known gap, deliberate.** That defect's errno classification is an inline literal, not a named `*_ERRNOS` set, so the strict rule does not catch its shape. Broadening to any `err.code` comparison raises it to 3 flags with **2 false positives** — `capability-lock.cts:408` (the deliberate EEXIST steal protocol) and `commonjs-marker.cts:131` (which returns a distinct, documented outcome). The rule stays strict and the gap is recorded here, rather than trading a trustworthy guard for a noisy one. **Rule.** A swallowed `catch` may not fold a fatal errno into a retry set. A synthesis step may not emit its artifact when its inputs failed (#3352). A derived conclusion may not be reported as authoritative when the derivation dropped input it could not resolve (#3427). @@ -451,7 +464,24 @@ Both close #3349 and #3360, which are **read-side** defects a real parser fixes #### 8.9 Each subsumed child is driven fail-first — *Enforced (prospective)* — every phase, ledger completed by #3951 -> Executor: `scripts/lint-fix-has-regression-tests.cjs`, wired into `lint:ci`. **Read the scope precisely:** it fires on *new* `fix(#NNNN)` commits; it does not assert that the 19 children listed below are covered. Measured 2026-08-28, 17 of 19 have a test citing their issue number; **#3364 and #3812 have none.** That is a text match, not proof the behavior is uncovered — but it is also not evidence that it is covered. +> Executor: `scripts/lint-fix-has-regression-tests.cjs`, wired into `lint:ci`. **Read the scope precisely:** it fires on *new* `fix(#NNNN)` commits; it does not assert that the 19 children listed below are covered. +> +> **Correction, #3987 — and then a correction OF that correction, which is the more instructive one.** +> +> The 2026-08-28 amendment said "17 of 19; #3364 and #3812 have none". #3987 first "corrected" that to **19 of 19**. An isolated reviewer showed the correction was itself false, and false in a worse way than the original: +> +> **The original claim was CORRECT for the predicate it stated.** #3987 silently swapped the predicate from *cited* to *covered* and then declared the count fixed. #3812 appears in **zero** files under `tests/` — `tests/gen-state-md-docs.test.cjs:374` is a generic generated-docs test naming no issue. Changing what a word means in order to make a ledger read better is a worse failure than the miscount it claimed to repair, and it happened inside an amendment whose subject is that a text match is not a fact. +> +> **The measured truth, by predicate:** +> +> | predicate | count | detail | +> |---|---|---| +> | cites its issue number in `tests/` | **18 of 19** | **#3812 does not.** #3364 does — `tests/runtime-marker-resolution.test.cjs:107`, asserting `:115-119` (the earlier claim that it did not was the one genuine miscount). | +> | behaviorally covered | 19 of 19 | #3812 by `tests/gen-state-md-docs.test.cjs:374` | +> +> §8.9 asks for a test **naming** each child, so **18 of 19 is the number that answers it.** The two predicates are not interchangeable and must not be reported as one. +> +> **#3812 is additionally PARTIALLY DELIVERED and has been RE-OPENED (2026-08-28).** The shipped fix declares cardinality for FRONTMATTER keys (`current_phase`/`current_plan` = optional, `docs/reference/state-md.md:89,91`); its stated acceptance was the `## Current Position` **body** section, and `:196-208` still has no normative single-valued/overwrite sentence and no pointer to `## Performance Metrics` for history. The first draft of this entry said it was "flagged here so it can be re-opened" and then did not re-open it — a note is not an action, so the issue is now actually open again with the evidence attached. **Rule.** #2986, #3372, #3364, #2540, #3231, #3349, #3360, #3358, #3365, #3356, #3352, #3427 — and the STATE.md set #3756, #3743, #3818, #3835, #3836, #3853, #3812 — each get a failing-first regression test driven green via `gsd-test`, **plus** a behavioral identity test asserting at the *consumer's* output per ADR-3180 Decision 4(b). A structural guard alone would not have caught these. diff --git a/eslint-rules/no-elapsed-assertion.cjs b/eslint-rules/no-elapsed-assertion.cjs index 2f32b2a77..1d21891c1 100644 --- a/eslint-rules/no-elapsed-assertion.cjs +++ b/eslint-rules/no-elapsed-assertion.cjs @@ -3,9 +3,17 @@ /** * no-elapsed-assertion * - * Flag assert*() calls whose argument reads a property named - * /^(elapsed|duration|took|ms)$/ or compares such an identifier. + * Flag assert*() calls whose argument reads a property/identifier whose + * name is (or is a camelCase-suffixed/prefixed variant of) a timing word + * — elapsed, duration, took, ms — or compares such an identifier. * Timing assertions are flaky and should not be in the test suite. + * + * Matches: elapsed, duration, took, ms, elapsedMs, tookMs, durationMs, + * msElapsed, elapsedTime, startMs, endMs. + * Does NOT match: params, items, forms, terms, dirnames (no capitalized + * "Ms"/"Elapsed"/"Duration"/"Took" boundary present), nor configured-bound + * identifiers like timeoutMs/cacheTtlMs/staleAfterMs (a deterministic + * config value, not a measured wall-clock elapsed value). */ /** @type {import('eslint').Rule.RuleModule} */ @@ -24,22 +32,34 @@ const rule = { }, }, create(context) { - const TIMING_PROPS = /^(elapsed|duration|took|ms)$/; + // Bare timing word, optionally followed by a camelCase suffix: + // elapsed, ms, elapsedMs, msElapsed, elapsedTime, tookMs, durationMs. + const TIMING_PROPS = /^(?:elapsed|duration|took|ms)(?:[A-Z]\w*)?$/; + // The specific start/end-of-interval delta pair, in millisecond form: + // startMs, endMs. Deliberately NOT a blanket "*Ms" suffix — identifiers + // like timeoutMs, cacheTtlMs, staleAfterMs name a configured bound + // (deterministic, safe to assert equal), not a measured wall-clock + // elapsed value, and must not be caught here. + const TIMING_DELTA_SUFFIX = /^(?:start|end)Ms$/; + + function isTimingName(name) { + return TIMING_PROPS.test(name) || TIMING_DELTA_SUFFIX.test(name); + } function containsTimingRef(node) { if (!node) return false; - // foo.elapsed, foo.duration, foo.took, foo.ms + // foo.elapsed, foo.duration, foo.took, foo.ms, foo.elapsedMs, foo.startMs if ( node.type === 'MemberExpression' && node.property.type === 'Identifier' && - TIMING_PROPS.test(node.property.name) + isTimingName(node.property.name) ) { return true; } - // Identifier directly: elapsed, duration, took, ms - if (node.type === 'Identifier' && TIMING_PROPS.test(node.name)) { + // Identifier directly: elapsed, duration, took, ms, elapsedMs, startMs + if (node.type === 'Identifier' && isTimingName(node.name)) { return true; } diff --git a/eslint-rules/no-swallowed-precondition.cjs b/eslint-rules/no-swallowed-precondition.cjs new file mode 100644 index 000000000..9b015ce62 --- /dev/null +++ b/eslint-rules/no-swallowed-precondition.cjs @@ -0,0 +1,212 @@ +'use strict'; + +/** + * no-swallowed-precondition + * + * Flag: a try/catch whose CATCH HANDLER swallows the error (no rethrow) where + * the TRY BLOCK calls a filesystem CREATION verb (mkdirSync / openSync / + * platformEnsureDir), AND the enclosing function separately references a + * `*_ERRNO` / `*_ERRNOS`-named set (the established retry/tolerate-errno + * convention across src/, e.g. PLANNING_LOCK_RETRY_ERRNOS). + * + * The defect (#1884, verbatim pre-fix at src/planning-workspace.cts:209-210): + * + * // Ensure .planning/ exists + * try { platformEnsureDir(planningDir(cwd)); } catch { /* ok *\/ } + * + * A genuine EACCES/ENOSPC/EROFS creating the directory was swallowed. The + * subsequent lock write then failed with ENOENT (parent missing) — and ENOENT + * was in the function's own PLANNING_LOCK_RETRY_ERRNOS retry set — so the + * fatal precondition failure was laundered into a 10-second phantom "lock + * held by a live process" contention error instead of surfacing. + * + * Predicate, measured against this tree (911 → 24 → 0 across the three + * stages; the CLEANUP-verb carve-out at stage 2 is the entire false-positive + * mass: rmSync 54, unlinkSync 43, closeSync 17, chmodSync 12, rmdirSync 7, + * kill 6 — all legitimate best-effort and deliberately NOT flagged): + * + * 1. a catch clause whose handler does not rethrow (no ThrowStatement + * anywhere in its subtree) — i.e. a true swallow, AND + * 2. whose try-block calls a CREATION verb: mkdirSync, openSync, or + * platformEnsureDir (name-based; dotted `fs.mkdirSync`/`fs.openSync` or + * a bare call for platformEnsureDir), AND + * 3. whose enclosing function references an identifier named `*_ERRNO` or + * `*_ERRNOS` anywhere in its body — the naming convention every one of + * the 10 retry/tolerate errno sets in src/ follows. + * + * Only requiring all three eliminates the CLEANUP-verb false positives + * (rmSync/unlinkSync/closeSync/chmodSync/rmdirSync/kill are legitimate + * best-effort swallows with no laundering risk) without narrowing so far + * that the actual defect shape is missed. + * + * Known gap (deliberately NOT closed here — see #3987 review): a function + * whose errno classification is an INLINE STRING LITERAL rather than a named + * `*_ERRNOS` set (e.g. `if (code !== 'EEXIST') return null;` in + * capability-lock.cts's acquireLock) is NOT caught by this rule. Broadening + * stage 3 to inline literals produced 2 false positives in this tree + * (capability-lock.cts:408's deliberate EEXIST steal-protocol check, and + * commonjs-marker.cts:131's distinct documented outcome) — that shape is + * fixed directly at its call site instead of being folded into this rule. + * + * References: + * issue #1884 (defect), #3987 (this rule) + * fix commit 0c43d853e (`fix(#1884): surface planning-lock mkdir failures, + * not a phantom timeout`) — the canonical fix-forward this rule enforces: + * let the creation failure propagate, or classify it distinctly so a fatal + * errno cannot be laundered into a retryable one. + * + * Message: + * Cite the seam: a swallowed CREATION-verb failure inside a function that + * also tolerates/retries specific errnos via a named `*_ERRNOS` set risks + * laundering a fatal filesystem error (EACCES/ENOSPC/EROFS) into a + * retryable one downstream. Let the creation failure propagate, or catch + * and classify it explicitly (rethrow anything not genuinely tolerable) + * so it can never be mistaken for a retryable condition. + */ + +// Filesystem CREATION verbs — measured false-positive-free set. Cleanup verbs +// (rmSync, unlinkSync, closeSync, chmodSync, rmdirSync, kill) are deliberately +// excluded; they are legitimate best-effort operations with no laundering risk. +const CREATION_VERBS = new Set(['mkdirSync', 'openSync', 'platformEnsureDir']); + +// The naming convention every retry/tolerate-errno set in src/ follows. +const ERRNO_SET_NAME_RE = /_ERRNOS?$/; + +const FUNCTION_TYPES = new Set([ + 'FunctionDeclaration', + 'FunctionExpression', + 'ArrowFunctionExpression', +]); + +/** + * Generic subtree walker, skipping `parent`/`tokens`/`comments` to avoid + * cycles. `visit` returns truthy to short-circuit with that value. + */ +function walkSubtree(root, visit) { + const seen = new WeakSet(); + function walk(n) { + if (!n || typeof n !== 'object') return undefined; + if (seen.has(n)) return undefined; + seen.add(n); + const result = visit(n); + if (result) return result; + for (const key of Object.keys(n)) { + if (key === 'parent' || key === 'tokens' || key === 'comments') continue; + const child = n[key]; + if (Array.isArray(child)) { + for (const item of child) { + if (item && typeof item === 'object' && item.type) { + const r = walk(item); + if (r) return r; + } + } + } else if (child && typeof child === 'object' && child.type) { + const r = walk(child); + if (r) return r; + } + } + return undefined; + } + return walk(root); +} + +/** True if `node` is a call to one of CREATION_VERBS (bare or `fs.`-dotted). */ +function isCreationCall(node) { + if (!node || node.type !== 'CallExpression') return false; + const callee = node.callee; + if (callee.type === 'Identifier' && CREATION_VERBS.has(callee.name)) { + return true; + } + if ( + callee.type === 'MemberExpression' && + !callee.computed && + callee.property.type === 'Identifier' && + CREATION_VERBS.has(callee.property.name) + ) { + return true; + } + return false; +} + +/** True if any CREATION_VERBS call appears anywhere in `tryBlock`'s subtree. */ +function tryBlockCallsCreationVerb(tryBlock) { + return !!walkSubtree(tryBlock, (n) => isCreationCall(n)); +} + +/** True if `catchClause` has NO ThrowStatement anywhere in its subtree (a true swallow). */ +function catchIsSwallowing(catchClause) { + if (!catchClause) return false; + return !walkSubtree(catchClause.body, (n) => n.type === 'ThrowStatement'); +} + +/** True if an identifier named `*_ERRNO`/`*_ERRNOS` appears anywhere in `node`'s subtree. */ +function referencesErrnoSet(node) { + return !!walkSubtree(node, (n) => n.type === 'Identifier' && ERRNO_SET_NAME_RE.test(n.name)); +} + +/** Nearest enclosing function (or Program, for top-level code) ancestor of `node`. */ +function findEnclosingScope(node, sourceCode) { + const ancestors = + typeof sourceCode.getAncestors === 'function' ? sourceCode.getAncestors(node) : []; + for (let i = ancestors.length - 1; i >= 0; i--) { + if (FUNCTION_TYPES.has(ancestors[i].type)) return ancestors[i]; + } + return sourceCode.ast; +} + +/** @type {import('eslint').Rule.RuleModule} */ +const rule = { + meta: { + type: 'problem', + docs: { + description: + 'Flag a swallowed filesystem-creation failure (mkdirSync/openSync/platformEnsureDir) ' + + 'inside a function that separately tolerates/retries specific errnos via a *_ERRNOS set — ' + + 'a fatal precondition error can be laundered into a retryable one (#1884)', + category: 'Correctness', + }, + schema: [], + messages: { + noSwallowedPrecondition: + "Swallowed '{{verb}}' failure: this function also references an errno-tolerance set " + + "('{{errnoRef}}'-style), so a genuine EACCES/ENOSPC/EROFS creating the precondition here " + + 'can be laundered into a retryable errno downstream (the #1884 class). ' + + 'Let the creation failure propagate, or catch and classify it explicitly ' + + '(rethrow anything that is not genuinely tolerable) — never swallow it silently.', + }, + }, + + create(context) { + const sourceCode = context.sourceCode ?? context.getSourceCode(); + + return { + TryStatement(node) { + if (!node.handler) return; + if (!tryBlockCallsCreationVerb(node.block)) return; + if (!catchIsSwallowing(node.handler)) return; + + const scope = findEnclosingScope(node, sourceCode); + if (!referencesErrnoSet(scope)) return; + + // Identify which creation verb triggered, for the message. + let verb = 'mkdirSync/openSync/platformEnsureDir'; + walkSubtree(node.block, (n) => { + if (isCreationCall(n)) { + verb = + n.callee.type === 'Identifier' ? n.callee.name : n.callee.property.name; + return true; + } + return false; + }); + + context.report({ + node, + messageId: 'noSwallowedPrecondition', + data: { verb, errnoRef: '*_ERRNOS' }, + }); + }, + }; + }, +}; + +module.exports = rule; diff --git a/eslint.config.mjs b/eslint.config.mjs index 93deac421..370dd5656 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -32,6 +32,7 @@ import requireSubprocessTimeout from './eslint-rules/require-subprocess-timeout. import noExternalRequireInBin from './eslint-rules/no-external-require-in-bin.cjs'; import noPrivateBinaryResolution from './eslint-rules/no-private-binary-resolution.cjs'; import requireRegisteredExit from './eslint-rules/require-registered-exit.cjs'; +import noSwallowedPrecondition from './eslint-rules/no-swallowed-precondition.cjs'; import noExactCaseEnvAccess from './eslint-rules/no-exact-case-env-access.cjs'; const localPlugin = { @@ -59,6 +60,7 @@ const localPlugin = { 'no-external-require-in-bin': noExternalRequireInBin, 'no-private-binary-resolution': noPrivateBinaryResolution, 'require-registered-exit': requireRegisteredExit, + 'no-swallowed-precondition': noSwallowedPrecondition, 'no-exact-case-env-access': noExactCaseEnvAccess, }, }; @@ -431,6 +433,14 @@ export default tseslint.config( // eslint-ignored (ADR-457), so a rule registered only on the emitted // surface never sees the real .cts sources (#3496). 'local/require-registered-exit': 'error', + // #3987 (issue #1884 class): flag a swallowed mkdirSync/openSync/ + // platformEnsureDir failure inside a function that also references a + // *_ERRNOS retry/tolerate set — a fatal EACCES/ENOSPC/EROFS creating a + // precondition can be laundered into a retryable errno downstream. See + // eslint-rules/no-swallowed-precondition.cjs for the measured predicate + // and its known gap (inline-literal errno classification is not caught; + // fixed directly at the call site instead — capability-lock.cts). + 'local/no-swallowed-precondition': 'error', // #3624 (epic #3411 Phase 4): flag an exact-case env-var read off a // non-process.env receiver. See CONTEXT.md DEFECT.WINDOWS-EXACT-CASE-ENV-ACCESS. 'local/no-exact-case-env-access': 'error', diff --git a/package.json b/package.json index 2ed09f069..4bc6250ee 100644 --- a/package.json +++ b/package.json @@ -121,7 +121,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-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-tests.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-unreachable-guard-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-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 && node scripts/lint-vendored-deps.cjs && node scripts/lint-docs-guard-registration.cjs && node scripts/lint-source-test-name-collision.cjs && npm run lint:hooks-runtime-build-seam && node scripts/check-contract-drift.cjs && node scripts/lint-mutation-test-derivation-drift.cjs && node scripts/lint-seam-enforcement.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-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-tests.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-unreachable-guard-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-slug-derivation-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-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 && node scripts/lint-vendored-deps.cjs && node scripts/lint-docs-guard-registration.cjs && node scripts/lint-source-test-name-collision.cjs && npm run lint:hooks-runtime-build-seam && node scripts/check-contract-drift.cjs && node scripts/lint-mutation-test-derivation-drift.cjs && node scripts/lint-seam-enforcement.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/gen-hooks-cli-exit.cjs b/scripts/gen-hooks-cli-exit.cjs index ada42c8f4..e9274ba51 100644 --- a/scripts/gen-hooks-cli-exit.cjs +++ b/scripts/gen-hooks-cli-exit.cjs @@ -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 # 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 ]', ' (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(); diff --git a/scripts/lib/drift-scan.cjs b/scripts/lib/drift-scan.cjs index 07ebac39c..1c30d0927 100644 --- a/scripts/lib/drift-scan.cjs +++ b/scripts/lib/drift-scan.cjs @@ -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, }; diff --git a/scripts/lint-slug-derivation-drift.cjs b/scripts/lint-slug-derivation-drift.cjs new file mode 100644 index 000000000..b792f5566 --- /dev/null +++ b/scripts/lint-slug-derivation-drift.cjs @@ -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().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().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 `^-|-$` 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().join('-')` is an alternate way to + * express the SAME collapse-to-hyphen shape as + * `.replace(, '-')`. 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, +}; diff --git a/scripts/qa-smell-ratchet.cjs b/scripts/qa-smell-ratchet.cjs index 28eb99a27..6d4bc57ab 100644 --- a/scripts/qa-smell-ratchet.cjs +++ b/scripts/qa-smell-ratchet.cjs @@ -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) ?? ''; } /** diff --git a/src/capability-lock.cts b/src/capability-lock.cts index bf3cb2ab8..6a4c24615 100644 --- a/src/capability-lock.cts +++ b/src/capability-lock.cts @@ -391,7 +391,16 @@ function lockBodyToken(body: string): string | null { * and the whole thing is a BOUNDED iterative loop. */ function acquireLock(lockPath: string, opts?: { maxAttempts?: number; waitForFresh?: boolean }): LockHandle | null { - try { fs.mkdirSync(path.dirname(lockPath), { recursive: true }); } catch { /* best-effort */ } + // A genuine failure here (EACCES/ENOSPC/EROFS) MUST surface immediately, matching the #1884 + // fix (0c43d853e / PR #3472) for withPlanningLock's identical shape. The prior + // `catch { /* best-effort */ }` swallowed it, and the subsequent `fs.openSync(lockPath, 'wx')` + // below then failed with ENOENT (parent dir missing) — which is NOT 'EEXIST', so the + // `if (code !== 'EEXIST') return null;` branch laundered a fatal filesystem error into an + // ordinary "lock unavailable" (null) result, indistinguishable from another live process + // legitimately holding the lock (#3987). `mkdirSync(recursive:true)` does not throw when the + // directory already exists, so the normal path (dir already present) is unaffected; only real + // creation failures propagate. + fs.mkdirSync(path.dirname(lockPath), { recursive: true }); const maxAttempts = (opts && Number.isInteger(opts.maxAttempts) && (opts.maxAttempts as number) > 0) ? (opts.maxAttempts as number) : LOCK_MAX_ATTEMPTS; diff --git a/tests/capability-lock-mkdir-failure-3987.test.cjs b/tests/capability-lock-mkdir-failure-3987.test.cjs new file mode 100644 index 000000000..7fa7aa18b --- /dev/null +++ b/tests/capability-lock-mkdir-failure-3987.test.cjs @@ -0,0 +1,106 @@ +'use strict'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { cleanup } = require('./helpers.cjs'); + +// Built lib is the test target (same surface the rest of the suite imports). +const capabilityLock = require('../gsd-core/bin/lib/capability-lock.cjs'); +const { acquireLock } = capabilityLock; + +// ───────────────────────────────────────────────────────────────────────────── +// #3987 (same class as #1884/PR #3472): acquireLock swallows the mkdirSync +// failure creating the lock directory. The subsequent `fs.openSync(lockPath, +// 'wx')` then throws ENOENT (parent missing), and ENOENT is NOT 'EEXIST', so +// `if (code !== 'EEXIST') return null;` laundered a genuine EACCES/EROFS +// filesystem error into an ordinary "lock unavailable" (null) result — +// indistinguishable from another live process legitimately holding the lock. +// +// Fix (matching #1884's approach in 0c43d853e): stop swallowing — let the +// mkdir failure propagate immediately with its real errno. These tests pin +// the corrected contract: a permission/space failure creating the lock +// directory throws, it is never folded into a "null" (lock unavailable) +// result. +// +// IO failure is forced via t.mock.method(fs, 'mkdirSync', ...), which +// auto-restores per-test (never chmod 0o000 — root bypasses mode bits, +// leaking coverage). +// ───────────────────────────────────────────────────────────────────────────── + +const realMkdirSync = fs.mkdirSync; + +function failMkdirFor(t, targetDir, code) { + t.mock.method(fs, 'mkdirSync', (p, opts) => { + if (typeof p === 'string' && (p === targetDir || p.startsWith(targetDir + path.sep))) { + const err = new Error(`${code}: permission denied, mkdir '${p}'`); + err.code = code; + throw err; + } + return realMkdirSync.call(fs, p, opts); + }); +} + +describe('acquireLock surfaces mkdir failure fast (#3987, same class as #1884)', () => { + test('EACCES creating the lock directory is surfaced immediately, not laundered into a "lock unavailable" null', (t) => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-caplock-mkdir-fail-')); + t.after(() => cleanup(tmpDir)); + + const lockDir = path.join(tmpDir, 'nested', 'sub'); + const lockPath = path.join(lockDir, '.lock'); + failMkdirFor(t, lockDir, 'EACCES'); + + assert.throws( + () => acquireLock(lockPath), + (err) => err && err.code === 'EACCES', + 'a genuine EACCES creating the lock directory must surface as EACCES, not return null' + ); + }); + + test('ENOSPC creating the lock directory is surfaced immediately', (t) => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-caplock-mkdir-fail-')); + t.after(() => cleanup(tmpDir)); + + const lockDir = path.join(tmpDir, 'nested', 'sub'); + const lockPath = path.join(lockDir, '.lock'); + failMkdirFor(t, lockDir, 'ENOSPC'); + + assert.throws( + () => acquireLock(lockPath), + (err) => err && err.code === 'ENOSPC', + 'a genuine ENOSPC creating the lock directory must surface immediately, not return null' + ); + }); + + test('the thrown mkdir-failure error is never returned as a lock handle or null', (t) => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-caplock-mkdir-fail-')); + t.after(() => cleanup(tmpDir)); + + const lockDir = path.join(tmpDir, 'nested', 'sub'); + const lockPath = path.join(lockDir, '.lock'); + failMkdirFor(t, lockDir, 'EACCES'); + + let returned; + let threw = false; + try { + returned = acquireLock(lockPath); + } catch { + threw = true; + } + assert.strictEqual(threw, true, 'acquireLock must throw on a genuine mkdir failure, not return'); + assert.strictEqual(returned, undefined, 'no value should be returned when the mkdir failure is surfaced'); + }); + + test('normal path is unaffected: a creatable lock directory still acquires a lock', (t) => { + // No monkeypatch — the lock directory is creatable in the writable tmpDir. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-caplock-mkdir-ok-')); + t.after(() => cleanup(tmpDir)); + + const lockPath = path.join(tmpDir, 'nested', 'sub', '.lock'); + const handle = acquireLock(lockPath); + assert.ok(handle && typeof handle.token === 'string', 'a creatable lock directory must still yield a lock handle'); + capabilityLock.releaseLock(handle); + }); +}); diff --git a/tests/cli-exit.test.cjs b/tests/cli-exit.test.cjs index 9cea026a3..046c40700 100644 --- a/tests/cli-exit.test.cjs +++ b/tests/cli-exit.test.cjs @@ -13,6 +13,7 @@ const { toLegacyResult } = require('./helpers/git-fixture.cjs'); const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const { createTempDir, cleanup } = require('./helpers.cjs'); const fc = require('./helpers/fast-check-setup.cjs'); +const { ensureScriptsOut } = require('./helpers/exit-code-artifact-flags.cjs'); // Paths to the compiled product seam (src/cli-exit.cts → gsd-core/bin/lib/cli-exit.cjs) // used for json-error mode regression tests which require io.cjs integration. @@ -1824,58 +1825,99 @@ describe('#3911: hooks/lib/cli-exit.js loads and terminates with no build presen // ─── #3911 A3: the --check generator guards can actually fail ────────────── // // CONTEXT.md's prove-it-can-fail rule: a guard that has never been observed -// to fail is not a guard. For BOTH new committed artifacts, corrupt the -// committed file, run the generator's --check, assert it fails and names the -// file, then restore in a `finally` (so a failing assertion here can never -// leave a committed artifact corrupted) and re-run --check to confirm the -// restore actually cleared the guard. +// to fail is not a guard. For BOTH new committed artifacts, corrupt a +// DISPOSABLE TMPDIR COPY of the artifact (never the committed file itself — +// test files in this repo run in parallel, and a sibling test +// (tests/exit-code-registry.test.cjs) asserts byte-equality on the real +// hooks/lib/exit-code-registry.js concurrently), run the generator's --check +// redirected at that tmpdir copy via its output-path override flag(s), +// assert it fails and names the file, then re-run --check against a +// freshly-restored tmpdir copy to confirm the guard actually clears. Nothing +// under hooks/ in the repo is ever written by either test. describe('#3911: the --check guards for the new hooks/lib artifacts can actually fail (A3)', () => { const REPO_ROOT = path.resolve(__dirname, '..'); const GEN_HOOKS_CLI_EXIT = path.join(REPO_ROOT, 'scripts', 'gen-hooks-cli-exit.cjs'); const GEN_EXIT_CODE_REGISTRY = path.join(REPO_ROOT, 'scripts', 'gen-exit-code-registry.cjs'); + const registryGenerator = require(GEN_EXIT_CODE_REGISTRY); // gen-hooks-cli-exit.cjs --check runs a real tsc compile of the whole // project to a throwaway outDir (see its own COMPILE_TIMEOUT_MS=60000) — // this needs a longer bound than a plain probe. const CHECK_TIMEOUT_MS = 90000; - test('gen-hooks-cli-exit.cjs --check fails on a corrupted hooks/lib/cli-exit.js, names the file, and clears on restore', () => { + test('gen-hooks-cli-exit.cjs --check fails on a corrupted TMPDIR copy of hooks/lib/cli-exit.js, names the file, and clears on restore', (t) => { + const dir = createTempDir('gsd-3911-hooks-cli-exit-check-'); + t.after(() => cleanup(dir)); + const copiedOut = path.join(dir, 'cli-exit.js'); const original = fs.readFileSync(HOOKS_CLI_EXIT_PATH); - let corrupted = false; - try { - fs.appendFileSync(HOOKS_CLI_EXIT_PATH, '\n// corrupted-by-A3-test\n'); - corrupted = true; - const r = toLegacyResult(runNode([GEN_HOOKS_CLI_EXIT, '--check'], { timeoutMs: CHECK_TIMEOUT_MS })); - assert.notEqual(r.status, 0, `--check must fail on a corrupted committed artifact; stderr: ${r.stderr}`); - assert.ok( - r.stderr.includes('cli-exit.js'), - `expected the failure to name the drifted file; got: ${r.stderr.slice(0, 400)}`, - ); - } finally { - if (corrupted) fs.writeFileSync(HOOKS_CLI_EXIT_PATH, original); - } + fs.writeFileSync(copiedOut, original); - const restored = toLegacyResult(runNode([GEN_HOOKS_CLI_EXIT, '--check'], { timeoutMs: CHECK_TIMEOUT_MS })); + fs.appendFileSync(copiedOut, '\n// corrupted-by-A3-test\n'); + const r = toLegacyResult(runNode( + [GEN_HOOKS_CLI_EXIT, '--check', '--out', copiedOut], + { timeoutMs: CHECK_TIMEOUT_MS }, + )); + assert.notEqual(r.status, 0, `--check must fail on a corrupted artifact; stderr: ${r.stderr}`); + assert.ok( + r.stderr.includes('cli-exit.js'), + `expected the failure to name the drifted file; got: ${r.stderr.slice(0, 400)}`, + ); + + fs.writeFileSync(copiedOut, original); + const restored = toLegacyResult(runNode( + [GEN_HOOKS_CLI_EXIT, '--check', '--out', copiedOut], + { timeoutMs: CHECK_TIMEOUT_MS }, + )); assert.equal(restored.status, 0, `--check must pass again once the artifact is restored; stderr: ${restored.stderr}`); + + // The property this whole test exists to prove: the committed file was + // never touched, at any point, by any of the above. + assert.deepEqual(fs.readFileSync(HOOKS_CLI_EXIT_PATH), original, 'committed hooks/lib/cli-exit.js must be untouched'); }); - test('gen-exit-code-registry.cjs --check fails on a corrupted hooks/lib/exit-code-registry.js, names the file, and clears on restore', () => { - const original = fs.readFileSync(HOOKS_EXIT_CODE_REGISTRY_PATH); - let corrupted = false; - try { - fs.appendFileSync(HOOKS_EXIT_CODE_REGISTRY_PATH, '\n// corrupted-by-A3-test\n'); - corrupted = true; - const r = toLegacyResult(runNode([GEN_EXIT_CODE_REGISTRY, '--check'], { timeoutMs: PROBE_TIMEOUT_MS })); - assert.notEqual(r.status, 0, `--check must fail on a corrupted committed artifact; stderr: ${r.stderr}`); - assert.ok( - r.stderr.includes('exit-code-registry.js'), - `expected the failure to name the drifted file; got: ${r.stderr.slice(0, 400)}`, - ); - } finally { - if (corrupted) fs.writeFileSync(HOOKS_EXIT_CODE_REGISTRY_PATH, original); - } + test('gen-exit-code-registry.cjs --check fails on a corrupted TMPDIR copy of hooks/lib/exit-code-registry.js, names the file, and clears on restore', (t) => { + const dir = createTempDir('gsd-3911-exit-code-registry-check-'); + t.after(() => cleanup(dir)); - const restored = toLegacyResult(runNode([GEN_EXIT_CODE_REGISTRY, '--check'], { timeoutMs: PROBE_TIMEOUT_MS })); + // Copy all FIVE generated artifacts into the tmpdir so --check compares + // entirely against tmpdir copies — no write to any real committed path. + // `--declaration` stays pointed at the REAL committed declaration + // (read-only; never written) rather than a tmpdir copy: the generator's + // banner embeds `path.relative(REPO_ROOT, declarationPath)` + // (scripts/gen-exit-code-registry.cjs:324), so a tmpdir declaration path + // (outside REPO_ROOT) would itself make freshly-derived content diverge + // from the real committed artifacts' banners — a false drift unrelated + // to the corruption this test injects. The secondary/hooks/dts/sh paths + // are derived by the SAME ensureScriptsOut seam + // tests/exit-code-registry.test.cjs uses, not a second hand-rolled copy. + const copiedOut = path.join(dir, 'exit-code-registry.cjs'); + const args = ensureScriptsOut(['--declaration', registryGenerator.DEFAULT_DECLARATION_PATH, '--out', copiedOut]); + const copiedScriptsOut = args[args.indexOf('--scripts-out') + 1]; + const copiedHooksOut = args[args.indexOf('--hooks-out') + 1]; + const copiedDtsOut = args[args.indexOf('--dts-out') + 1]; + const copiedShOut = args[args.indexOf('--sh-out') + 1]; + + fs.copyFileSync(registryGenerator.DEFAULT_OUTPUT_PATH, copiedOut); + fs.copyFileSync(registryGenerator.DEFAULT_SCRIPTS_OUTPUT_PATH, copiedScriptsOut); + const original = fs.readFileSync(HOOKS_EXIT_CODE_REGISTRY_PATH); + fs.writeFileSync(copiedHooksOut, original); + fs.copyFileSync(registryGenerator.DEFAULT_DTS_OUTPUT_PATH, copiedDtsOut); + fs.copyFileSync(registryGenerator.DEFAULT_SH_OUTPUT_PATH, copiedShOut); + + fs.appendFileSync(copiedHooksOut, '\n// corrupted-by-A3-test\n'); + const r = toLegacyResult(runNode([GEN_EXIT_CODE_REGISTRY, '--check', ...args], { timeoutMs: PROBE_TIMEOUT_MS })); + assert.notEqual(r.status, 0, `--check must fail on a corrupted artifact; stderr: ${r.stderr}`); + assert.ok( + r.stderr.includes(copiedHooksOut) && r.stderr.includes('hooks'), + `expected the failure to name the drifted hooks artifact (${copiedHooksOut}); got: ${r.stderr.slice(0, 400)}`, + ); + + fs.writeFileSync(copiedHooksOut, original); + const restored = toLegacyResult(runNode([GEN_EXIT_CODE_REGISTRY, '--check', ...args], { timeoutMs: PROBE_TIMEOUT_MS })); assert.equal(restored.status, 0, `--check must pass again once the artifact is restored; stderr: ${restored.stderr}`); + + // The property this whole test exists to prove: the committed file was + // never touched, at any point, by any of the above. + assert.deepEqual(fs.readFileSync(HOOKS_EXIT_CODE_REGISTRY_PATH), original, 'committed hooks/lib/exit-code-registry.js must be untouched'); }); }); diff --git a/tests/eslint-rules.test.cjs b/tests/eslint-rules.test.cjs index fa350ede5..30ec07059 100644 --- a/tests/eslint-rules.test.cjs +++ b/tests/eslint-rules.test.cjs @@ -1242,6 +1242,79 @@ describe('no-elapsed-assertion rule', () => { ], }); }); + + // ─── #3987: camelCase/suffixed evasion (elapsedMs escaped the exact-name + // regex; CI caught the resulting flake instead of lint catching the + // anti-pattern) ─────────────────────────────────────────────────────── + + test('invalid: assert on .elapsedMs property (the exact identifier that evaded the pre-widening exact-name regex)', () => { + ruleTester.run('no-elapsed-assertion', noElapsedAssertion, { + valid: [], + invalid: [ + { + code: ` + const assert = require('node:assert/strict'); + const result = { elapsedMs: 150 }; + assert.ok(result.elapsedMs < 200); + `, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'noElapsedAssertion' }], + }, + ], + }); + }); + + test('invalid: assert on tookMs/durationMs/msElapsed/elapsedTime/startMs/endMs — camelCase family the widened rule must catch', () => { + ruleTester.run('no-elapsed-assertion', noElapsedAssertion, { + valid: [], + invalid: [ + { + code: `assert.ok(x.tookMs < 500);`, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'noElapsedAssertion' }], + }, + { + code: `assert.ok(x.durationMs > 0);`, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'noElapsedAssertion' }], + }, + { + code: `assert.ok(x.msElapsed > 0);`, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'noElapsedAssertion' }], + }, + { + code: `assert.ok(x.elapsedTime < 1000);`, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'noElapsedAssertion' }], + }, + { + code: `assert.ok(x.endMs - x.startMs < 100);`, + filename: 'tests/foo.test.cjs', + errors: [{ messageId: 'noElapsedAssertion' }], + }, + ], + }); + }); + + test('valid: non-timing camelCase identifiers containing "ms" as a plain substring do not flag (params/items/forms/terms/dirnames — and a configured-bound timeoutMs)', () => { + ruleTester.run('no-elapsed-assertion', noElapsedAssertion, { + valid: [ + { code: `assert.equal(params.length, 2);`, filename: 'tests/foo.test.cjs' }, + { code: `assert.equal(items.length, 0);`, filename: 'tests/foo.test.cjs' }, + { code: `assert.ok(forms.valid);`, filename: 'tests/foo.test.cjs' }, + { code: `assert.equal(terms.length, 3);`, filename: 'tests/foo.test.cjs' }, + { code: `assert.equal(dirnames.length, 1);`, filename: 'tests/foo.test.cjs' }, + { + // A configured bound (deterministic pass-through), not a measured + // wall-clock elapsed value — must not be caught by the widening. + code: `assert.equal(seen[0].timeoutMs, HOOK_FANOUT_TIMEOUT_MS);`, + filename: 'tests/foo.test.cjs', + }, + ], + invalid: [], + }); + }); }); // ─── no-raw-rmsync-in-tests ────────────────────────────────────────────────── diff --git a/tests/exit-code-registry.test.cjs b/tests/exit-code-registry.test.cjs index 4aecb539c..83fb814cc 100644 --- a/tests/exit-code-registry.test.cjs +++ b/tests/exit-code-registry.test.cjs @@ -29,6 +29,7 @@ const { runNode } = require('./helpers/process-seam.cjs'); const { createTempDir, cleanup } = require('./helpers.cjs'); const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); const fc = require('./helpers/fast-check-setup.cjs'); +const { ensureScriptsOut } = require('./helpers/exit-code-artifact-flags.cjs'); const { splitLines } = require('../gsd-core/bin/lib/text-lines.cjs'); const REPO_ROOT = path.resolve(__dirname, '..'); @@ -75,19 +76,11 @@ function makeEntry(overrides) { * the test already supplies, whenever the caller has not already supplied * its own `--scripts-out`/`--hooks-out`/`--dts-out`/`--sh-out`. Calls with * no explicit `--out` (the "real committed set" checks) are left untouched. + * + * `ensureScriptsOut` itself now lives in + * ./helpers/exit-code-artifact-flags.cjs so tests/cli-exit.test.cjs can + * reuse the exact same derivation rather than hand-rolling a second copy. */ -function ensureScriptsOut(args) { - const outIdx = args.indexOf('--out'); - if (outIdx === -1) return args; - const outValue = args[outIdx + 1]; - const extra = []; - if (!args.includes('--scripts-out')) extra.push('--scripts-out', `${outValue}.secondary.cjs`); - if (!args.includes('--hooks-out')) extra.push('--hooks-out', `${outValue}.hooks.js`); - if (!args.includes('--dts-out')) extra.push('--dts-out', `${outValue}.d.cts`); - if (!args.includes('--sh-out')) extra.push('--sh-out', `${outValue}.sh`); - return extra.length === 0 ? args : [...args, ...extra]; -} - function runGen(args, opts = {}) { return runNode([GEN_SCRIPT, ...ensureScriptsOut(args)], { timeoutMs: PROBE_TIMEOUT_MS, ...opts }); } diff --git a/tests/health-validation.test.cjs b/tests/health-validation.test.cjs index ca1050cfb..7c9fb90f5 100644 --- a/tests/health-validation.test.cjs +++ b/tests/health-validation.test.cjs @@ -1930,7 +1930,7 @@ describe('W024 — STATE.md commit-age freshness advisory (#2573)', () => { const os = require('node:os'); const path = require('node:path'); const { runGsdTools, cleanup } = require('./helpers.cjs'); - const { runGit } = require('./helpers/process-seam.cjs'); + const { runGit, OUTCOME } = require('./helpers/process-seam.cjs'); const { STATE_HEAD_ADVISORY_COMMITS, } = require('../gsd-core/bin/lib/verify.cjs'); @@ -1939,6 +1939,25 @@ describe('W024 — STATE.md commit-age freshness advisory (#2573)', () => { const track = (d) => { dirs.push(d); return d; }; after(() => { while (dirs.length) cleanup(dirs.pop()); }); + // `runGit` (tests/helpers/process-seam.cjs) reports a failed spawn as DATA, + // never as a throw — by design, so retry-aware callers can inspect it. A + // fixture builder that ignores that return value swallows the failure and + // silently produces a WEAKER input (e.g. one fewer commit, a blank + // `state_head`) than what the test asked for: the exact "swallowed + // precondition laundered into a plausible downstream outcome" shape that + // eslint-rules/no-swallowed-precondition.cjs exists to catch, just in test + // code instead of source. `mustGit` closes that gap for this builder. + function mustGit(args, options) { + const r = runGit(args, options); + if (r.outcome !== OUTCOME.EXITED || r.exitCode !== 0) { + throw new Error( + `mustGit: \`git ${args.join(' ')}\` did not succeed ` + + `(outcome=${r.outcome}, exitCode=${r.exitCode}): ${r.stderr.trim()}` + ); + } + return r; + } + function project({ commitsAhead, stateHead = 'BASE' }) { const base = track(fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2573-h-'))); const planningDir = path.join(base, '.planning'); @@ -1953,13 +1972,15 @@ describe('W024 — STATE.md commit-age freshness advisory (#2573)', () => { '# Roadmap\n\n## Milestone v1.0\n\n### Phase 1: One\n**Goal:** g\n', ); - runGit(['init', '-q'], { cwd: base }); - runGit(['config', 'user.email', 't@t.com'], { cwd: base }); - runGit(['config', 'user.name', 'T'], { cwd: base }); - runGit(['config', 'commit.gpgsign', 'false'], { cwd: base }); - runGit(['add', '-A'], { cwd: base }); - runGit(['commit', '-q', '-m', 'seed'], { cwd: base }); - const head = runGit(['rev-parse', 'HEAD'], { cwd: base }).stdout.trim(); + mustGit(['init', '-q'], { cwd: base }); + mustGit(['config', 'user.email', 't@t.com'], { cwd: base }); + mustGit(['config', 'user.name', 'T'], { cwd: base }); + mustGit(['config', 'commit.gpgsign', 'false'], { cwd: base }); + mustGit(['add', '-A'], { cwd: base }); + mustGit(['commit', '-q', '-m', 'seed'], { cwd: base }); + const seed = mustGit(['rev-parse', 'HEAD'], { cwd: base }).stdout.trim(); + const head = seed; + assert.ok(head.length > 0, 'FIXTURE ERROR: `git rev-parse HEAD` for the seed commit returned empty'); fs.writeFileSync( path.join(planningDir, 'STATE.md'), @@ -1979,9 +2000,26 @@ describe('W024 — STATE.md commit-age freshness advisory (#2573)', () => { for (let i = 0; i < commitsAhead; i++) { fs.writeFileSync(path.join(base, `f${i}.txt`), `${i}\n`); - runGit(['add', '-A'], { cwd: base }); - runGit(['commit', '-q', '-m', `c${i}`], { cwd: base }); + mustGit(['add', '-A'], { cwd: base }); + mustGit(['commit', '-q', '-m', `c${i}`], { cwd: base }); } + + // Precondition check: prove the fixture built what it claims rather than + // trusting the loop above ran to completion. A silently-failed `git + // commit` here previously dropped one commit off the count with no + // signal, which was enough to move `readStateHeadFreshness` + // (src/state.cts) across the W024 threshold and produce a false negative + // (#2573 flake). Fail as a FIXTURE error, not as the assertion under test. + const actualCommitsAhead = Number( + mustGit(['rev-list', '--count', `${seed}..HEAD`], { cwd: base }).stdout.trim() + ); + assert.strictEqual( + actualCommitsAhead, + commitsAhead, + `FIXTURE ERROR: requested commitsAhead=${commitsAhead} but ` + + `\`git rev-list --count ${seed}..HEAD\` reports ${actualCommitsAhead}` + ); + return base; } diff --git a/tests/helpers/exit-code-artifact-flags.cjs b/tests/helpers/exit-code-artifact-flags.cjs new file mode 100644 index 000000000..883be2326 --- /dev/null +++ b/tests/helpers/exit-code-artifact-flags.cjs @@ -0,0 +1,40 @@ +'use strict'; + +/** + * tests/helpers/exit-code-artifact-flags.cjs + * + * Shared flag-derivation seam for scripts/gen-exit-code-registry.cjs test + * callers. The generator emits FIVE artifacts (primary, scripts, hooks, dts, + * sh) driven by `--out`/`--scripts-out`/`--hooks-out`/`--dts-out`/`--sh-out`. + * Any call that supplies `--out` without also supplying matching overrides + * for the other four would, under `--write`, clobber the real committed + * `scripts/lib/exit-code-registry.cjs`, `hooks/lib/exit-code-registry.js`, + * `src/exit-code-registry.d.cts`, and `gsd-core/bin/shared/exit-codes.sh` — + * dangerous since test files in this repo run in parallel. + * + * `ensureScriptsOut` derives co-located, per-call-unique secondary/hooks/ + * dts/sh paths from whatever `--out` value the caller already supplies, + * whenever the caller has not already supplied its own override. Calls with + * no explicit `--out` (the "real committed set" checks) are left untouched. + * + * Extracted so every test file that drives this generator's five-artifact + * flag surface shares ONE derivation — CONTRIBUTING.md's ban on re-deriving + * a shared flag-builder applies here. + */ + +/** @param {string[]} args + * @returns {string[]} + */ +function ensureScriptsOut(args) { + const outIdx = args.indexOf('--out'); + if (outIdx === -1) return args; + const outValue = args[outIdx + 1]; + const extra = []; + if (!args.includes('--scripts-out')) extra.push('--scripts-out', `${outValue}.secondary.cjs`); + if (!args.includes('--hooks-out')) extra.push('--hooks-out', `${outValue}.hooks.js`); + if (!args.includes('--dts-out')) extra.push('--dts-out', `${outValue}.d.cts`); + if (!args.includes('--sh-out')) extra.push('--sh-out', `${outValue}.sh`); + return extra.length === 0 ? args : [...args, ...extra]; +} + +module.exports = { ensureScriptsOut }; diff --git a/tests/no-swallowed-precondition.rule.test.cjs b/tests/no-swallowed-precondition.rule.test.cjs new file mode 100644 index 000000000..0363e320e --- /dev/null +++ b/tests/no-swallowed-precondition.rule.test.cjs @@ -0,0 +1,193 @@ +'use strict'; + +/** + * no-swallowed-precondition.rule.test.cjs + * + * RuleTester unit tests for the local/no-swallowed-precondition ESLint rule. + * + * Rule: flag a try/catch whose catch handler SWALLOWS the error (no rethrow) + * where the try-block calls a filesystem CREATION verb (mkdirSync / openSync / + * platformEnsureDir), AND the enclosing function separately references an + * identifier named `*_ERRNO`/`*_ERRNOS` (the retry/tolerate-errno naming + * convention). All three conditions must hold — see eslint-rules/ + * no-swallowed-precondition.cjs for the measured predicate (911 -> 24 -> 0). + * + * DEFECT category: issue #1884 (verbatim pre-fix shape reproduced here as the + * positive control), rule shipped under #3987. + * + * INVALID (violation expected): + * - the verbatim #1884 pre-fix shape: swallowed platformEnsureDir inside a + * function that references a *_ERRNOS set elsewhere + * - swallowed mkdirSync inside a function referencing a *_ERRNOS set + * - swallowed openSync inside a function referencing a *_ERRNO (singular) set + * + * VALID (no violation): + * - the post-#1884-fix shape (no try/catch at all — propagates) + * - swallowed CLEANUP verb (rmSync/unlinkSync/closeSync) — the FP class, + * even inside a function that references a *_ERRNOS set + * - swallowed creation verb with NO errno-set reference anywhere in the + * enclosing function + * - creation-verb catch that DOES rethrow (not a swallow) + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const { RuleTester } = require('eslint'); + +const noSwallowedPrecondition = require('../eslint-rules/no-swallowed-precondition.cjs'); + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'commonjs', + }, +}); + +// ─── module shape ───────────────────────────────────────────────────────────── + +describe('no-swallowed-precondition rule module', () => { + test('exports meta and create', () => { + assert.strictEqual(typeof noSwallowedPrecondition.meta, 'object'); + assert.strictEqual(typeof noSwallowedPrecondition.create, 'function'); + assert.strictEqual(noSwallowedPrecondition.meta.type, 'problem'); + assert.ok(noSwallowedPrecondition.meta.messages.noSwallowedPrecondition); + }); +}); + +// ─── INVALID cases (violation expected) ─────────────────────────────────────── + +describe('no-swallowed-precondition invalid cases', () => { + test('invalid: the verbatim #1884 pre-fix shape (platformEnsureDir swallowed, function references *_ERRNOS)', () => { + ruleTester.run('no-swallowed-precondition', noSwallowedPrecondition, { + valid: [], + invalid: [ + { + code: `const PLANNING_LOCK_RETRY_ERRNOS = new Set(['ENOENT']); +function withPlanningLock(cwd, fn) { + // Ensure .planning/ exists + try { platformEnsureDir(planningDir(cwd)); } catch { /* ok */ } + while (true) { + try { + return fn(); + } catch (err) { + if (PLANNING_LOCK_RETRY_ERRNOS.has(err.code)) continue; + throw err; + } + } +}`, + errors: [{ messageId: 'noSwallowedPrecondition' }], + }, + ], + }); + }); + + test('invalid: swallowed mkdirSync inside a function referencing a *_ERRNOS set', () => { + ruleTester.run('no-swallowed-precondition', noSwallowedPrecondition, { + valid: [], + invalid: [ + { + code: `const LOCK_RETRY_ERRNOS = new Set(['EBUSY']); +function acquireLock(lockPath) { + try { fs.mkdirSync(path.dirname(lockPath), { recursive: true }); } catch { /* best-effort */ } + try { + return fs.openSync(lockPath, 'wx'); + } catch (err) { + if (LOCK_RETRY_ERRNOS.has(err.code)) return null; + throw err; + } +}`, + errors: [{ messageId: 'noSwallowedPrecondition' }], + }, + ], + }); + }); + + test('invalid: swallowed openSync inside a function referencing a *_ERRNO (singular) set', () => { + ruleTester.run('no-swallowed-precondition', noSwallowedPrecondition, { + valid: [], + invalid: [ + { + code: `const TOLERATED_ERRNO = new Set(['EEXIST']); +function open(p) { + try { fs.openSync(p, 'wx'); } catch (e) {} + return TOLERATED_ERRNO.has('EEXIST'); +}`, + errors: [{ messageId: 'noSwallowedPrecondition' }], + }, + ], + }); + }); +}); + +// ─── VALID cases (no violation) ──────────────────────────────────────────────── + +describe('no-swallowed-precondition valid cases', () => { + test('valid: the post-#1884-fix shape — no try/catch, propagates', () => { + ruleTester.run('no-swallowed-precondition', noSwallowedPrecondition, { + valid: [ + `const PLANNING_LOCK_RETRY_ERRNOS = new Set(['ENOENT']); +function withPlanningLock(cwd, fn) { + // A genuine failure here MUST surface immediately. + platformEnsureDir(planningDir(cwd)); + while (true) { + try { + return fn(); + } catch (err) { + if (PLANNING_LOCK_RETRY_ERRNOS.has(err.code)) continue; + throw err; + } + } +}`, + ], + invalid: [], + }); + }); + + test('valid: swallowed CLEANUP verbs (rmSync/unlinkSync/closeSync) are NOT flagged, even alongside a *_ERRNOS set', () => { + ruleTester.run('no-swallowed-precondition', noSwallowedPrecondition, { + valid: [ + `const RETRY_ERRNOS = new Set(['EBUSY']); +function cleanupRm(p) { + try { fs.rmSync(p, { force: true }); } catch { /* best-effort */ } + return RETRY_ERRNOS.has('EBUSY'); +}`, + `const RETRY_ERRNOS = new Set(['EBUSY']); +function cleanupUnlink(p) { + try { fs.unlinkSync(p); } catch { /* already released */ } + return RETRY_ERRNOS.has('EBUSY'); +}`, + `const RETRY_ERRNOS = new Set(['EBUSY']); +function cleanupClose(fd) { + try { fs.closeSync(fd); } catch { /* best-effort */ } + return RETRY_ERRNOS.has('EBUSY'); +}`, + ], + invalid: [], + }); + }); + + test('valid: swallowed creation verb with NO errno-set reference in the enclosing function', () => { + ruleTester.run('no-swallowed-precondition', noSwallowedPrecondition, { + valid: [ + `function ensureDir(p) { + try { fs.mkdirSync(p, { recursive: true }); } catch { /* best-effort, no errno set here */ } + return true; +}`, + ], + invalid: [], + }); + }); + + test('valid: creation-verb catch that DOES rethrow is not a swallow', () => { + ruleTester.run('no-swallowed-precondition', noSwallowedPrecondition, { + valid: [ + `const RETRY_ERRNOS = new Set(['EBUSY']); +function ensureDir(p) { + try { fs.mkdirSync(p, { recursive: true }); } catch (e) { throw e; } + return RETRY_ERRNOS.has('EBUSY'); +}`, + ], + invalid: [], + }); + }); +}); diff --git a/tests/planning-inspect.test.cjs b/tests/planning-inspect.test.cjs index 6d6e04f77..6e9c8f235 100644 --- a/tests/planning-inspect.test.cjs +++ b/tests/planning-inspect.test.cjs @@ -35,6 +35,7 @@ const path = require('node:path'); const fc = require('fast-check'); const { createTempProject, createTempDir, cleanup, runGsdTools, toPosixPath } = require('./helpers.cjs'); +const { generateSlugInternal } = require('../gsd-core/bin/lib/core-utils.cjs'); // ─── Fixture helpers ────────────────────────────────────────────────────────── @@ -91,9 +92,16 @@ function writeUatDoc(phaseDir, phaseToken, bodyLines, eol = '\n') { writeAbs(path.join(phaseDir, `${phaseToken}-UAT.md`), bodyLines.join(eol)); } -/** Slugify a phase name the same way `getPhaseDirFromPhaseId` (`src/phase-id.cts`) does. */ +/** + * Slugify a phase name the same way `getPhaseDirFromPhaseId` (`src/phase-id.cts`) + * does. Routed through the canonical `generateSlugInternal` seam (issue #3987) + * instead of a hand-rolled copy: `getPhaseDirFromPhaseId` does not truncate, + * so `maxLen: null` is what makes the parity claim in this comment true — + * the prior copy also never transliterated non-Latin phase names, unlike the + * real seam it claims to match. + */ function slugify(name) { - return name.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, ''); + return generateSlugInternal(name, null) ?? ''; } /** diff --git a/tests/slug-derivation-drift-guard.test.cjs b/tests/slug-derivation-drift-guard.test.cjs new file mode 100644 index 000000000..b59fc96fa --- /dev/null +++ b/tests/slug-derivation-drift-guard.test.cjs @@ -0,0 +1,576 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +/** + * Unit coverage for the SLUG-DERIVATION drift guard + * (scripts/lint-slug-derivation-drift.cjs, issue #3987, closing epic #3473's + * last two residuals). + * + * Modelled on tests/enumeration-drift-guard.test.cjs / + * tests/completion-ratio-single-owner.test.cjs: exercises the guard's pure + * functions directly (no `readFileSync().includes()` in a test body), plus + * a `scanRepo` PROVE-IT-CAN-FAIL row on a fresh synthetic tree — this + * repo's rule that a drift guard must be shown capable of failing, not just + * shown to pass on an already-clean tree. + * + * .gsd/phase/feat-3987-guard-slug-and-swallow/50-test-matrix.md rows T1-T12 + * map onto the describe blocks below. + */ + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +const ROOT = path.join(__dirname, '..'); +const { + findSlugDerivationDrift, + scanRepo, + buildLogicalStatements, + stripComments, + SCAN_EXT, +} = require(path.join(ROOT, 'scripts', 'lint-slug-derivation-drift.cjs')); +const { generateSlugInternal } = require(path.join(ROOT, 'gsd-core', 'bin', 'lib', 'core-utils.cjs')); +const { getPhaseDirFromPhaseId } = require(path.join(ROOT, 'gsd-core', 'bin', 'lib', 'phase-id.cjs')); +const { slugify: qaSmellRatchetSlugify } = require(path.join(ROOT, 'scripts', 'qa-smell-ratchet.cjs')); +const { createTempDir, cleanup } = require('./helpers.cjs'); +const { splitLines } = require(path.join(ROOT, 'gsd-core', 'bin', 'lib', 'text-lines.cjs')); +const { MAX_REGEX_LITERAL_LEN, resetRegexScanStats, getRegexScanStats } = require(path.join(ROOT, 'scripts', 'lib', 'drift-scan.cjs')); + +// ─── T1: the real deleted #3883 shape (POSITIVE) ────────────────────────── + +describe('findSlugDerivationDrift — T1: the real historical inline-copy shape', () => { + test('single-line chained copy (matches src/gsd2-import.cts slugify\'s own shape) is flagged', () => { + const line = "function slugify(title) { return title.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, ''); }"; + const v = findSlugDerivationDrift(line, 'src/unrelated.cts'); + assert.equal(v.length, 1); + assert.equal(v[0].line, 1); + }); + + test('multi-line chained copy (matches the real deleted #3883 shape, and the pre-fix scripts/qa-smell-ratchet.cjs slugify) is flagged as ONE statement', () => { + const text = [ + 'function slugify(value) {', + ' return value', + ' .toLowerCase()', + " .replace(/[^a-z0-9]+/g, '-')", + " .replace(/^-+|-+$/g, '')", + ' .slice(0, 60);', + '}', + ].join('\n'); + const v = findSlugDerivationDrift(text, 'src/unrelated.cts'); + assert.equal(v.length, 1); + assert.equal(v[0].line, 2, 'reports at the statement\'s OPENING line, not the line either .replace() sits on'); + }); + + test('the exact pre-fix tests/planning-inspect.test.cjs helper shape is flagged', () => { + const line = "function slugify(name) { return name.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, ''); }"; + const v = findSlugDerivationDrift(line, 'tests/unrelated.test.cjs'); + assert.equal(v.length, 1); + }); +}); + +// ─── T2: the canonical owner is NOT flagged ─────────────────────────────── + +describe('findSlugDerivationDrift — T2: the canonical owner (src/core-utils.cts generateSlugInternal)', () => { + test('the real generateSlugInternal body is not flagged, EVEN UNEXEMPTED — its two clauses sit in different statements by construction', () => { + const text = fs.readFileSync(path.join(ROOT, 'src', 'core-utils.cts'), 'utf8'); + const unexempt = findSlugDerivationDrift(text, 'ZZZ-not-the-real-owner-path.cts'); + assert.deepEqual(unexempt, []); + }); + + test('the real owner file at its real repo-relative path is not flagged (allowlist entry present as a defensive backstop)', () => { + const text = fs.readFileSync(path.join(ROOT, 'src', 'core-utils.cts'), 'utf8'); + const v = findSlugDerivationDrift(text, path.join('src', 'core-utils.cts')); + assert.deepEqual(v, []); + }); + + test('a synthetic refactor that DID fold generateSlugInternal into one statement would be flagged if NOT for the explicit allowlist entry — proving the entry is load-bearing, not decorative', () => { + const folded = [ + 'function generateSlugInternal(text, maxLen) {', + " return transliterateForSlug(text).replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '');", + '}', + ].join('\n'); + const unexempt = findSlugDerivationDrift(folded, 'ZZZ-not-core-utils.cts'); + assert.equal(unexempt.length, 1, 'the folded shape IS detectable — proves the real file escapes only by construction, not because the detector cannot see this shape'); + + const exempt = findSlugDerivationDrift(folded, path.join('src', 'core-utils.cts')); + assert.deepEqual(exempt, [], 'the allowlist entry suppresses it at the real owner path'); + }); +}); + +// ─── T3-T5: the 3 sanctioned sites are exempted BY the allowlist ────────── + +describe('findSlugDerivationDrift — T3-T5: sanctioned sites are exempted BY the allowlist, not by accident', () => { + const sanctioned = [ + { file: path.join('src', 'gsd2-import.cts'), fn: 'slugify' }, + { file: path.join('src', 'runtime-artifact-conversion.cts'), fn: 'normalizeKimiSkillName' }, + { file: path.join('scripts', 'generate-package-identity.cjs'), fn: 'slugifyPackageName' }, + ]; + + for (const { file, fn } of sanctioned) { + test(`${file} (${fn}) is exempted at its real path`, () => { + const text = fs.readFileSync(path.join(ROOT, file), 'utf8'); + const v = findSlugDerivationDrift(text, file); + assert.deepEqual(v, []); + }); + + test(`${file} (${fn}) IS flagged when the SAME text is attributed to a non-exempt path — proves the allowlist, not the shape, suppresses it`, () => { + const text = fs.readFileSync(path.join(ROOT, file), 'utf8'); + const v = findSlugDerivationDrift(text, `ZZZ-not-exempt-${path.basename(file)}`); + assert.ok(v.length >= 1, `expected ${file}'s re-derivation to be independently detectable outside its allowlist entry`); + }); + } +}); + +// ─── MAJOR-1 (security review, #3987): exemption scoping does not bleed past +// the exempted function's own closing brace ──────────────────────────────── + +describe('findSlugDerivationDrift — MAJOR-1: allowlist exemption is scoped to the REAL function body, not "until the next top-level function"', () => { + const sanctionedRealEndLines = [ + { file: path.join('src', 'core-utils.cts'), fn: 'generateSlugInternal', realEndLine: 193 }, + { file: path.join('src', 'gsd2-import.cts'), fn: 'slugify', realEndLine: 103 }, + { file: path.join('src', 'runtime-artifact-conversion.cts'), fn: 'normalizeKimiSkillName', realEndLine: 616 }, + { file: path.join('scripts', 'generate-package-identity.cjs'), fn: 'slugifyPackageName', realEndLine: 42 }, + ]; + + for (const { file, fn, realEndLine } of sanctionedRealEndLines) { + test(`a re-derivation planted immediately AFTER ${fn}'s (${file}) real closing brace IS flagged — the pre-fix bug exempted up to 50 lines past the function's own 11-line body`, () => { + const lines = splitLines(fs.readFileSync(path.join(ROOT, file), 'utf8')); + const evilSlug = "const evilSlug = (t) => t.replace(/[^a-z0-9]+/g,'-').replace(/^-+|-+$/g,'');"; + lines.splice(realEndLine, 0, evilSlug); // insert right after the function's REAL closing brace + const text = lines.join('\n'); + + const v = findSlugDerivationDrift(text, file); + assert.equal(v.length, 1, `expected the planted violation right after ${fn}'s real body to be flagged`); + assert.equal(v[0].line, realEndLine + 1); + }); + + test(`a re-derivation planted INSIDE ${fn}'s (${file}) own real body remains exempt`, () => { + const text = fs.readFileSync(path.join(ROOT, file), 'utf8'); + // The real bodies here are single-collapse-clause shapes (never both + // clauses in one statement) by construction, so this only re-confirms + // the existing "exempted at its real path" T3-T5 assertion holds with + // the new extent-based exemption mechanism, not the old bleed-through one. + assert.deepEqual(findSlugDerivationDrift(text, file), []); + }); + } +}); + +// ─── T6: the rejected loose [^A-Za-z0-9._-] line-level shape is NOT flagged ─ + +describe('findSlugDerivationDrift — T6: the rejected loose line-level false-positive shape', () => { + test('a negated-class collapse to a DIFFERENT character ("_") sharing a statement with a hyphen-trim is NOT flagged — clause (a) requires collapsing specifically to \'-\'', () => { + const line = "const p = raw.replace(/[^A-Za-z0-9._-]+/g, '_').replace(/^-+|-+$/g, '');"; + assert.deepEqual(findSlugDerivationDrift(line, 'src/unrelated.cts'), []); + }); + + test('two UNRELATED statements sharing one physical line (separated by \';\') are NOT merged into one false-positive statement', () => { + const line = "a.replace(/[^A-Za-z0-9._-]+/g, '-'); b.replace(/^-+|-+$/g, '');"; + assert.deepEqual(findSlugDerivationDrift(line, 'src/unrelated.cts'), []); + }); + + test('clause (a) alone (no trim-replace anywhere) is not flagged', () => { + const line = "const p = raw.replace(/[^A-Za-z0-9._-]+/g, '-');"; + assert.deepEqual(findSlugDerivationDrift(line, 'src/unrelated.cts'), []); + }); + + test('clause (b) alone (no charclass-replace anywhere) is not flagged', () => { + const line = "const p = raw.replace(/^-+|-+$/g, '');"; + assert.deepEqual(findSlugDerivationDrift(line, 'src/unrelated.cts'), []); + }); +}); + +// ─── MAJOR-3 (security review, #3987): widened detector shapes ─────────── + +describe('findSlugDerivationDrift — MAJOR-3: widened detection (each of these evaded the pre-fix detector)', () => { + const positives = [ + ['replaceAll alongside replace', "function f(t){return t.toLowerCase().replaceAll(/[^a-z0-9]+/g,'-').replaceAll(/^-+|-+$/g,'');}"], + ['{1,} as an equivalent of +', "function f(t){return t.toLowerCase().replace(/[^a-z0-9]{1,}/g,'-').replace(/^-+|-+$/g,'');}"], + ['\\s* prefixed into the collapse class', "function f(t){return t.toLowerCase().replace(/\\s*[^a-z0-9]+\\s*/g,'-').replace(/^-+|-+$/g,'');}"], + ['escaped ] inside the negated class', "function f(t){return t.toLowerCase().replace(/[^a-z0-9\\]]+/g,'-').replace(/^-+|-+$/g,'');}"], + ['literal new RegExp(...) form', "function f(t){return t.toLowerCase().replace(new RegExp('[^a-z0-9]+','g'),'-').replace(/^-+|-+$/g,'');}"], + ['trim spelled /^[-]+|[-]+$/', "function f(t){return t.toLowerCase().replace(/[^a-z0-9]+/g,'-').replace(/^[-]+|[-]+$/g,'');}"], + ['trim spelled /(^-+)|(-+$)/', "function f(t){return t.toLowerCase().replace(/[^a-z0-9]+/g,'-').replace(/(^-+)|(-+$)/g,'');}"], + ['trim spelled /^-*|-*$/', "function f(t){return t.toLowerCase().replace(/[^a-z0-9]+/g,'-').replace(/^-*|-*$/g,'');}"], + ['trim spelled /^\\-+|\\-+$/', "function f(t){return t.toLowerCase().replace(/[^a-z0-9]+/g,'-').replace(/^\\-+|\\-+$/g,'');}"], + ['trim spelled /-+$|^-+/ (swapped order)', "function f(t){return t.toLowerCase().replace(/[^a-z0-9]+/g,'-').replace(/-+$|^-+/g,'');}"], + ['.split().join(\'-\') as a collapse form', "function f(t){return t.toLowerCase().split(/[^a-z0-9]+/g).join('-').replace(/^-+|-+$/g,'');}"], + [ + 'arguments to .replace( spanning multiple physical lines (no leading "." continuation)', + ['function f(t){', ' return t.toLowerCase().replace(', ' /[^a-z0-9]+/g,', " '-'", " ).replace(/^-+|-+$/g, '');", '}'].join('\n'), + ], + ]; + + for (const [name, src] of positives) { + test(`${name} is flagged`, () => { + const v = findSlugDerivationDrift(src, 'ZZZ-not-exempt.cts'); + assert.ok(v.length >= 1, `expected "${name}" to be detected after the MAJOR-3 widening`); + }); + } + + test('the two-statement/temp-var form is a DOCUMENTED, deliberate gap (needs data flow, not textual matching) — still evades', () => { + const src = ['function f(t){', " const a = t.toLowerCase().replace(/[^a-z0-9]+/g,'-');", " return a.replace(/^-+|-+$/g,'');", '}'].join('\n'); + assert.deepEqual(findSlugDerivationDrift(src, 'ZZZ-not-exempt.cts'), []); + }); + + test('new RegExp built from a variable is a DOCUMENTED, deliberate gap — still evades', () => { + const src = "function f(t,p){return t.toLowerCase().replace(new RegExp(p,'g'),'-').replace(/^-+|-+$/g,'');}"; + assert.deepEqual(findSlugDerivationDrift(src, 'ZZZ-not-exempt.cts'), []); + }); +}); + +// ─── MAJOR-2 (security review, #3987): bounded regex-literal extraction ── + +describe('findSlugDerivationDrift — MAJOR-2: no quadratic blowup on a pathological regex-literal-shaped line', () => { + test('a 1.28MB line built from many unterminated "[^"-shaped fragments is scanned in bounded, LINEAR work — not the pre-fix quadratic blowup (54.3s at this size)', () => { + // Deterministic replacement for a wall-clock assertion (CLAUDE.md "Clock + // Seams: Do not assert on wall-clock time" — the original elapsedMs<5000 + // row flaked on a slow shared CI runner at 7368ms while passing locally + // at ~200ms). Instead of timing, this pins the actual MAJOR-2 invariant: + // every attempt to read a regex literal is capped at MAX_REGEX_LITERAL_LEN + // characters (`readRegexLiteralAt`'s own `limit`), so total scan work + // across all `k` unterminated `.replace(/[^` fragments is bounded by + // `calls * MAX_REGEX_LITERAL_LEN` — a small linear multiple of the input, + // never a multiple of the input's OWN LENGTH (which is what made the + // pre-fix unbounded scan quadratic: each of the k attempts re-scanned + // however much of the remaining 1.28MB line was left). + const k = 40000; // '.replace(/[^'.repeat(k) + 'x'.repeat(20k) ~= 1.28MB, matching the review's measured repro + const line = '.replace(/[^'.repeat(k) + 'x'.repeat(20 * k); + assert.equal(Buffer.byteLength(line, 'utf8'), 1_280_000); + + resetRegexScanStats(); + const v = findSlugDerivationDrift(line, 'ZZZ-not-exempt.cts'); + const { calls, charsExamined } = getRegexScanStats(); + + assert.deepEqual(v, [], 'a giant unterminated fragment run is not a real re-derivation'); + // Every attempt is capped at MAX_REGEX_LITERAL_LEN by construction — this + // holds even under the (hypothetical) unbounded pre-fix shape only if the + // cap itself is honored; a bound expressed against `calls`, not against + // `line.length`, is what makes this assertion mean something. + assert.ok(calls > 0, 'expected at least one regex-literal-read attempt on this fragment run'); + assert.ok( + charsExamined <= calls * MAX_REGEX_LITERAL_LEN, + `expected charsExamined (${charsExamined}) to never exceed calls (${calls}) * MAX_REGEX_LITERAL_LEN (${MAX_REGEX_LITERAL_LEN})`, + ); + // The real discriminator: an ABSOLUTE ceiling, independent of whatever + // MAX_REGEX_LITERAL_LEN happens to be configured to (the prior assertion + // is tautological w.r.t. that constant and would not catch the constant + // itself being blown out). Measured on the current bounded implementation + // this line drives ~120k bounded attempts (`calls`) totalling + // ~4.8e7 examined characters — comfortably under 1e8. An unbounded scan + // (each attempt re-reading however much of the 1.28MB line remains, the + // exact pre-fix shape) is quadratic: ~line.length^2/2 ≈ 8e11 characters — + // over four orders of magnitude past this ceiling. Confirmed live: a + // reverted "no cap" simulation of this same fixture did not finish within + // 120s, versus ~0.3s bounded. + assert.ok( + charsExamined < 1e8, + `expected charsExamined (${charsExamined}) to stay under a fixed absolute ceiling, not scale toward line.length^2 (~8e11) as the pre-fix unbounded scan would`, + ); + }); +}); + +// ─── MINOR fixes (security review, #3987) ──────────────────────────────── + +describe('MINOR fixes', () => { + test('stripComments does not cut at a "//" inside a string literal (e.g. a URL)', () => { + const line = "const u = 'http://x'; return t.replace(/[^a-z0-9]+/g, '-');"; + const stripped = stripComments(line); + assert.ok(stripped.includes("'http://x'"), 'the URL string must survive comment-stripping intact'); + assert.ok(stripped.includes(".replace(/[^a-z0-9]+/g, '-')"), 'code after the string must survive too'); + }); + + test('a top-level statement split on ";" does not fire on a ";" embedded inside a regex character class', () => { + const line = "a.replace(/[^a-z0-9;]+/g, '-'); b.replace(/^-+|-+$/g, '');"; + const stmts = buildLogicalStatements([line]); + assert.equal(stmts.length, 2, 'the embedded ";" inside the class must not split the first statement in two'); + assert.equal(stmts[0].text, "a.replace(/[^a-z0-9;]+/g, '-')"); + }); + + test('SCAN_EXT includes .mjs, .tsx, .jsx alongside the original extensions', () => { + for (const ext of ['.mjs', '.tsx', '.jsx', '.cts', '.ts', '.mts', '.cjs', '.js']) { + assert.ok(SCAN_EXT.has(ext), `expected SCAN_EXT to include ${ext}`); + } + }); +}); + +// ─── buildLogicalStatements — the statement-scoping mechanism itself ────── + +describe('buildLogicalStatements — statement scoping mechanics', () => { + test('a chain\'s continuation lines (leading ".") merge into the opening line\'s statement', () => { + const text = ['const x = a', ' .b()', ' .c();'].join('\n'); + const stmts = buildLogicalStatements(text.split('\n')); + assert.equal(stmts.length, 1); + assert.equal(stmts[0].startLine, 1); + // The trailing ';' is stripped by the fragment splitter (it is the + // fragment TERMINATOR, not part of the statement text) — matches the + // ';'-terminated statements test below. + assert.equal(stmts[0].text, 'const x = a .b() .c()'); + }); + + test('a line NOT starting with "." never merges into the previous statement, even with no ";" boundary', () => { + const text = ['const a = 1', 'const b = 2'].join('\n'); + const stmts = buildLogicalStatements(text.split('\n')); + assert.equal(stmts.length, 2); + assert.equal(stmts[0].startLine, 1); + assert.equal(stmts[1].startLine, 2); + }); + + test('multiple ";"-terminated statements on one physical line become separate statements', () => { + const stmts = buildLogicalStatements(['const a = 1; const b = 2; const c = 3;']); + assert.equal(stmts.length, 3); + assert.deepEqual(stmts.map((s) => s.text), ['const a = 1', 'const b = 2', 'const c = 3']); + }); + + test('blank and comment-only lines are skipped without breaking a chain across them', () => { + const text = ['const x = a', ' // a comment line in the middle of the chain', '', ' .b();'].join('\n'); + const stmts = buildLogicalStatements(text.split('\n')); + assert.equal(stmts.length, 1); + assert.equal(stmts[0].text, 'const x = a .b()'); + }); +}); + +// ─── T7 (PROVE-IT-CAN-FAIL) + T8: scanRepo mechanics ────────────────────── + +describe('scanRepo — PROVE-IT-CAN-FAIL: the guard reds on a fresh synthetic violation', () => { + test('a freshly written violation in a temp tree is reported with its file and line', (t) => { + const root = createTempDir('gsd-slug-derivation-drift-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(root, 'src', 'fake.cts'), + "function slugify(name) { return name.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, ''); }\n", + ); + + const violations = scanRepo(root); + assert.equal(violations.length, 1, 'the guard must be able to FAIL on a real violation, not merely pass on a clean tree'); + assert.equal(violations[0].file, path.join('src', 'fake.cts')); + assert.equal(violations[0].line, 1); + }); + + test('a clean temp tree with no re-derivations reports zero violations', (t) => { + const root = createTempDir('gsd-slug-derivation-drift-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + fs.writeFileSync(path.join(root, 'src', 'clean.cts'), 'const x = 1;\n'); + + assert.deepEqual(scanRepo(root), []); + }); + + test('gsd-core/bin/lib and bin/install.js are never visited — a scan-dir outside src/scripts/tests/eslint-rules is not scanned', (t) => { + const root = createTempDir('gsd-slug-derivation-drift-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'gsd-core', 'bin', 'lib'), { recursive: true }); + fs.writeFileSync( + path.join(root, 'gsd-core', 'bin', 'lib', 'core-utils.cjs'), + "function slugify(name) { return name.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, ''); }\n", + ); + fs.mkdirSync(path.join(root, 'bin'), { recursive: true }); + fs.writeFileSync( + path.join(root, 'bin', 'install.js'), + "function slugify(name) { return name.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, ''); }\n", + ); + + assert.deepEqual(scanRepo(root), []); + }); + + test('SELF_TEST_FILE exemption is scoped to its exact path — a DIFFERENT tests/ file with the same fixture text IS still flagged', (t) => { + const { SELF_TEST_FILE } = require(path.join(ROOT, 'scripts', 'lint-slug-derivation-drift.cjs')); + const root = createTempDir('gsd-slug-derivation-drift-'); + t.after(() => cleanup(root)); + const fixtureLine = "function slugify(name) { return name.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, ''); }\n"; + + fs.mkdirSync(path.join(root, path.dirname(SELF_TEST_FILE)), { recursive: true }); + fs.writeFileSync(path.join(root, SELF_TEST_FILE), fixtureLine); + + const otherTestsFile = path.join('tests', 'some-other-file.test.cjs'); + fs.writeFileSync(path.join(root, otherTestsFile), fixtureLine); + + const violations = scanRepo(root); + assert.equal(violations.length, 1, 'exactly one violation: SELF_TEST_FILE is skipped, the other tests/ file is not'); + assert.equal(violations[0].file, otherTestsFile); + }); +}); + +test('T8: scanRepo(repoRoot) against the real repo returns EMPTY after the Task-2 fixes (was 2 TRUE positives)', () => { + const violations = scanRepo(ROOT); + assert.deepEqual(violations, []); +}); + +// ─── PROVE-IT-CAN-FAIL, the CLI (main()) surface ────────────────────────── +// +// The `scanRepo` PROVE-IT-CAN-FAIL row above only exercises the pure +// function; `main()`'s `process.exitCode = 1`, its stderr banner text, and +// both `sanitizeForReport` call sites (on `d.file` AND `d.found`) are a +// SEPARATE, uncovered surface — dropping the `process.exitCode = 1` line +// entirely would leave every row above green. `main()` is not exported and +// hardcodes its scan root to `path.join(__dirname, '..')` (the real repo, +// which is clean — see T8), so the only way to drive the exit-1 branch is to +// run the CLI as a REAL subprocess against a throwaway copy of the script +// (plus its `scripts/lib/drift-scan.cjs` dependency, which has no other +// requires) rooted at a synthetic tree carrying a real violation. +describe('CLI (main()) — the process.exitCode/stderr surface scanRepo alone does not cover', () => { + test('a violation drives exit code 1, a stderr banner, and a sanitized file:line report line', (t) => { + const { spawnSync } = require('node:child_process'); + const tmpRoot = createTempDir('gsd-slug-derivation-drift-cli-'); + t.after(() => cleanup(tmpRoot)); + + fs.mkdirSync(path.join(tmpRoot, 'scripts', 'lib'), { recursive: true }); + fs.mkdirSync(path.join(tmpRoot, 'src'), { recursive: true }); + fs.copyFileSync( + path.join(ROOT, 'scripts', 'lint-slug-derivation-drift.cjs'), + path.join(tmpRoot, 'scripts', 'lint-slug-derivation-drift.cjs'), + ); + fs.copyFileSync( + path.join(ROOT, 'scripts', 'lib', 'drift-scan.cjs'), + path.join(tmpRoot, 'scripts', 'lib', 'drift-scan.cjs'), + ); + // `d.file` gets a zero-width space (a valid filename character on every + // OS — a raw control byte in a filename is REJECTED as ENOENT on + // Windows, so it cannot be used here without breaking that lane); `d.found` + // gets an actual control byte (\x07) embedded in the flagged statement's + // own text, which is disk file CONTENT, not a path, so it is safe on every + // platform. Both are exactly what sanitizeForReport exists to neutralize. + const evilFileName = `fa${'​'}ke.cts`; + fs.writeFileSync( + path.join(tmpRoot, 'src', evilFileName), + `function slugify(name${'\x07'}) { return name.toLowerCase().replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, ''); }\n`, + ); + + const res = spawnSync(process.execPath, [path.join(tmpRoot, 'scripts', 'lint-slug-derivation-drift.cjs')], { + encoding: 'utf8', + timeout: 10000, + }); + + assert.equal(res.status, 1, 'main() must set a non-zero process.exitCode when a violation is found'); + assert.match(res.stderr, /slug-derivation-drift: independent re-derivation\(s\)/, 'expected the stderr banner text'); + assert.match(res.stderr, /generateSlugInternal\(text, maxLen\)/, 'expected the fix-forward guidance line'); + assert.match(res.stderr, /fa\\u200bke\.cts/, 'sanitizeForReport must have escaped the zero-width space in d.file'); + assert.match(res.stderr, /name\\x07/, 'sanitizeForReport must have escaped the control byte in d.found'); + assert.ok(!res.stderr.includes('​'), 'the raw zero-width space must not reach stderr unescaped'); + assert.ok(!res.stderr.includes('\x07'), 'the raw control byte must not reach stderr unescaped'); + }); + + test('a clean tree exits 0 with the "ok" stdout line, not the violation banner', (t) => { + const { spawnSync } = require('node:child_process'); + const tmpRoot = createTempDir('gsd-slug-derivation-drift-cli-clean-'); + t.after(() => cleanup(tmpRoot)); + + fs.mkdirSync(path.join(tmpRoot, 'scripts', 'lib'), { recursive: true }); + fs.mkdirSync(path.join(tmpRoot, 'src'), { recursive: true }); + fs.copyFileSync( + path.join(ROOT, 'scripts', 'lint-slug-derivation-drift.cjs'), + path.join(tmpRoot, 'scripts', 'lint-slug-derivation-drift.cjs'), + ); + fs.copyFileSync( + path.join(ROOT, 'scripts', 'lib', 'drift-scan.cjs'), + path.join(tmpRoot, 'scripts', 'lib', 'drift-scan.cjs'), + ); + fs.writeFileSync(path.join(tmpRoot, 'src', 'clean.cts'), 'const x = 1;\n'); + + const res = spawnSync(process.execPath, [path.join(tmpRoot, 'scripts', 'lint-slug-derivation-drift.cjs')], { + encoding: 'utf8', + timeout: 10000, + }); + + assert.equal(res.status, 0); + assert.match(res.stdout, /^ok slug-derivation-drift:/); + assert.equal(res.stderr, ''); + }); +}); + +// ─── T9-T11: scripts/qa-smell-ratchet.cjs slugify, routed through the seam ─ + +describe('scripts/qa-smell-ratchet.cjs slugify — routed through generateSlugInternal (#2849/#2848 fixes)', () => { + // Re-require the fixed module's own slugify indirectly isn't exported, so + // these rows assert the SEAM behaves as the routed call site now expects + // (parity is guaranteed by construction: the call site is `generateSlugInternal(value, 60) ?? ''`). + + test('T9: a >60-char input whose 60th char is the separator hyphen itself does not leave a trailing hyphen (the live #2849 bug)', () => { + // 59 'a's + "-bcd": char 60 (1-based) is EXACTLY the separator hyphen at + // index 59. The pre-#2849 formula trimmed leading/trailing hyphens BEFORE + // truncating to 60 — a no-op here, since the hyphen sits in the middle, + // not at either end — then truncated with substring(0, 60), which lands + // exactly ON that hyphen and leaves it as the new trailing character: + // 59 'a's + trailing '-'. The fixed formula truncates FIRST (same 60-char + // cut), THEN trims trailing hyphens, removing it. This input is + // discriminating (old -> trailing '-', new -> none); the previous fixture + // ('a'.repeat(58) + '-bcdef') truncated to "...a-b", which both the old + // and new formulas produce identically — it never reached #2849's bug at + // all (verified: both formulas agree on that input). + const input = 'a'.repeat(59) + '-bcd'; + const slug = generateSlugInternal(input, 60) ?? ''; + assert.ok(!slug.endsWith('-'), `expected no trailing hyphen after truncation, got ${JSON.stringify(slug)}`); + assert.equal(slug.length <= 60, true); + assert.equal(slug, 'a'.repeat(59), 'the separator hyphen itself must be trimmed after truncation'); + }); + + test('T9b (call-site): scripts/qa-smell-ratchet.cjs\'s slugify(), routed through generateSlugInternal, does not leave a trailing hyphen either', () => { + // Asserts through the CALL SITE, not generateSlugInternal directly — if + // qa-smell-ratchet.cjs's slugify() were reverted to its pre-#3987 + // hand-rolled (trim-before-truncate) copy, this reds even though T9 + // above (which only calls generateSlugInternal) would not notice. + const input = 'a'.repeat(59) + '-bcd'; + const slug = qaSmellRatchetSlugify(input); + assert.ok(!slug.endsWith('-'), `expected no trailing hyphen from the routed call site, got ${JSON.stringify(slug)}`); + assert.equal(slug, 'a'.repeat(59)); + }); + + test('T10: non-Latin (Cyrillic) input transliterates to a non-empty slug', () => { + const slug = generateSlugInternal('Привет мир', 60) ?? ''; + assert.notEqual(slug, ''); + assert.match(slug, /^[a-z0-9-]+$/); + }); + + test('T11: null/empty input preserves the never-null contract via "?? \'\'"', () => { + assert.equal(generateSlugInternal(null, 60) ?? '', ''); + assert.equal(generateSlugInternal('', 60) ?? '', ''); + assert.equal(generateSlugInternal(undefined, 60) ?? '', ''); + }); + + test('T11b: an entirely non-alphanumeric input collapses to the EMPTY STRING, not null', () => { + // Distinguishes "the input produced nothing after slugification" (a + // non-null, empty string — the collapse/trim clauses ran and consumed + // every character) from "no input was supplied at all" (T11's null/undefined + // -> null case). Previously untested. + assert.equal(generateSlugInternal('!!!', 60), ''); + assert.notEqual(generateSlugInternal('!!!', 60), null); + }); + + test('T9c: maxLen boundary at 59/60/61 (limit-1, limit, limit+1) truncates to exactly maxLen characters', () => { + const input = 'a'.repeat(65); + assert.equal(generateSlugInternal(input, 59), 'a'.repeat(59)); + assert.equal(generateSlugInternal(input, 60), 'a'.repeat(60)); + assert.equal(generateSlugInternal(input, 61), 'a'.repeat(61)); + }); +}); + +// ─── T12: tests/planning-inspect.test.cjs slugify helper parity ────────── + +describe('tests/planning-inspect.test.cjs slugify helper — parity with getPhaseDirFromPhaseId (#3987)', () => { + // A name long enough that its slug EXCEEDS 60 chars, so maxLen: null and + // maxLen: 60 genuinely disagree — the previous 18-char fixture never + // exercised truncation at all, so the two arguments trivially matched + // regardless of which one getPhaseDirFromPhaseId actually passes. + const LONG_NAME = 'This Is A Genuinely Long Phase Name That Exceeds Sixty Characters For Sure'; + + test('T12: maxLen: null and maxLen: 60 genuinely DISAGREE on a >60-char slug (fixture is discriminating)', () => { + const untruncated = generateSlugInternal(LONG_NAME, null) ?? ''; + const truncated = generateSlugInternal(LONG_NAME, 60) ?? ''; + assert.ok(untruncated.length > 60, `fixture must produce a >60-char slug to be discriminating, got length ${untruncated.length}`); + assert.equal(truncated.length, 60); + assert.notEqual(untruncated, truncated, 'maxLen: null and maxLen: 60 must produce DIFFERENT slugs for this fixture'); + }); + + test('T12b (call site): getPhaseDirFromPhaseId embeds the UNTRUNCATED (maxLen: null) slug, never the 60-char-truncated one', () => { + // Asserts through the CALL SITE (src/phase-id.cts's getPhaseDirFromPhaseId), + // not generateSlugInternal directly — if that call site were reverted to + // pass the 60-char default instead of maxLen: null, this reds even + // though a generateSlugInternal-only assertion would not notice. + const dir = getPhaseDirFromPhaseId('01-01', LONG_NAME, null); + const untruncated = generateSlugInternal(LONG_NAME, null) ?? ''; + const truncated = generateSlugInternal(LONG_NAME, 60) ?? ''; + assert.ok(dir.endsWith(untruncated), `expected the phase dir to embed the untruncated slug, got ${JSON.stringify(dir)}`); + assert.ok(!dir.endsWith(truncated) || truncated === untruncated, 'the phase dir must not embed the 60-char-truncated slug'); + }); +});