From 80de48c319cbbf378c1ab11280d199ddcefb105e Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 29 Aug 2026 00:53:33 -0400 Subject: [PATCH] enhance(#3914): every phase records a truthful guard ledger (#4018) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#3914): retire n/no-process-exit where its successor governs Epic #3889 criterion 5 — no phase closes with a guard added and its predecessor left standing — is violated in the tree by the epic that wrote it. local/require-registered-exit was registered on gsd-core/bin/**/*.cjs and scripts/**/*.cjs, while n/no-process-exit stayed 'error' over a nine-glob block covering those same two. Only the hooks 'off' exemption ever came down; the predecessor's registration never did. Both rules have been enforcing the same property on the same surfaces since P6. Narrowed, not deleted. Seven of those nine globs have NO successor — eslint-rules/, bin/lib/, pi/, examples/, vscode/, .kilo/, .opencode/ — so deleting the rule outright would silently drop enforcement on all seven. That is the inversion this epic has already hit three times: removing a coarse guard because a narrower one exists somewhere it does not reach. Flat config is last-match-wins and both successor blocks come after the nine-glob block, so 'n/no-process-exit': 'off' in exactly those two retires the predecessor precisely where the successor governs and nowhere else. The successor is strictly more precise: it permits process.exit only inside terminateNow in cli-exit.cts, the single sanctioned terminator (ADR-3889 §3), where n/no-process-exit permits none and would flag terminateNow's own generated copy. Asserted at the consumer's altitude via ESLint.calculateConfigForFile on real paths, with the positive control that matters: n/no-process-exit is still 'error' on six of the seven successor-less globs, so a future edit that turns this into a blanket disable goes red. bin/lib/ has no file in this checkout and is reported as untested rather than given an invented path. Severity is normalized across the string/numeric/array forms the API can return, and the normalized value asserted — not truthiness. Verified by running calculateConfigForFile myself on both superseded globs and four controls before trusting the test. Found and fixed inline: the change made an eslint-disable directive at gsd-tools.cjs:257 partially unused, which --max-warnings 0 rejects; narrowed to the one rule still in force. Verification runs on the remote runner. Refs #3914 * docs(#3914): the epic added three guards, it did not remove one The audit reconciled the epic ledger against what actually landed. The net is +3, not -1: four lint:generated-sync --check arms (gen-scripts-cli-exit, gen-hooks-cli-exit, gen-exit-code-registry, gen-exit-code-docs) plus one rule, against two retirements. An epic whose thesis was consolidation ended with a larger guard surface than it started with. The additions are each defensible; the claim that the total fell was never true. Two of the three prior errors in this amendment are mine. It said "Net -1 by count" above terms reading -1 -1 +1 +1 +1, which sums to +1 — an arithmetic error in the paragraph directly below the sentence arguing that an ADR about honest accounting must not pad its own ledger. And the term list omitted two of the four --check arms, which is what turns that +1 into the real +3. Recorded rather than quietly rewritten. This ledger has now been wrong three times — the original -2, the -1 that replaced it, and #3914's own table, which states -1 above terms summing to 0 — and a written claim nobody checked against the thing it describes is the exact failure this epic exists to close. Refs #3914 * fix(#3914): make the successor actually supersede before retiring the predecessor An isolated security review found that the previous commit turned off a guard that was still doing work. Reproduced by executing both rules against a fixture, not inferred: const exit = 'exit'; process[exit](1); n/no-process-exit flags it; local/require-registered-exit did not, because it early-returned on callee.computed. So retiring the predecessor on gsd-core/bin/**/*.cjs and scripts/**/*.cjs un-guarded that shape on precisely the two globs this epic's exit contract cares most about. This is the third time in this epic I have removed a coarse guard on the claim that a narrower one covered it, without checking construct-level parity — after the allowlist key-to-prefix-to-exact-membership sequence and the band ranges-to-categories one. The rule is the same every time: a narrower guard supersedes a coarser one only where it demonstrably reaches at least as far, and "demonstrably" means executing both against the constructs, not reading either. The successor now resolves computed property access for the statically determinable cases — a string Literal, and an Identifier bound once to a string Literal, resolved through scope — and leaves genuinely dynamic properties alone so the rule does not over-fire. Measured after the fix: plain process.exit flagged, process['exit']() flagged, process[exit]() flagged, process[globalThis.k]() not flagged. That makes it a strict superset of the predecessor on these globs, since process['exit']() was caught by NEITHER rule before. The second finding is worse than the first, because it was reasoning rather than oversight. My justification comment claimed n/no-process-exit "would flag terminateNow's own generated copy here". It would not — that file is in the global ignore list, so neither rule ever lints it. There was no conflict to resolve; I wrote a rationale I had not checked, in a change whose entire subject is written claims nobody verified. Both comment blocks now state the real basis. The tests that should have caught this asserted only rule SEVERITY per glob and never construct REACH, which is exactly how a coverage hole passed. A parity matrix now pins all five shapes, including a RED/GREEN regression pin against an inlined reproduction of the pre-fix rule — inlined rather than loaded from HEAD, because HEAD resolves to the fixed commit under the remote runner and would silently stop testing anything. Verification runs on the remote runner. Refs #3914 * fix(#3914): the two exit rules are complementary — keep both Reverts this branch's retirement of n/no-process-exit. The premise was wrong twice, and the second review proved the change itself was wrong. I claimed local/require-registered-exit was a strict superset on gsd-core/bin/**/*.cjs and scripts/**/*.cjs. Measured, successor vs predecessor: function f(exit) { process[exit](1); } 0 vs 1 let exit='exit'; exit='exit'; process[exit]() 0 vs 1 const { exit } = ...; process[exit](1) 0 vs 1 plus for-of bindings, let-then-assign, var redeclaration, catch params, and an undeclared global named exit. The predecessor matches any identifier NAMED exit however it is bound; the successor resolves only a string literal or a single-write const. It never was a superset — I asserted the relationship after fixing one construct and did not re-check the rest. The justification was independently false: all three generated cli-exit copies are in the global ignore list, so n/no-process-exit was never flagging terminateNow. There was no conflict to resolve. I wrote a rationale I had not verified, in the phase whose subject is written claims nobody checked. So criterion 5 does not apply to this pair. They are not predecessor and successor — they are complementary, each catching constructs the other misses. The epic's criterion assumed a replacement relationship that does not exist here, and retiring either rule loses real coverage. The ADR ledger now says so with the measured shapes. What survives is the genuine improvement: the computed-property strengthening. local/require-registered-exit now catches process['exit'](1) and optional-chain terminators like process?.[k]?.(1), which NEITHER rule caught before, while correctly ignoring a genuinely dynamic property so it does not over-fire. The parity tests are rewritten to assert what is true rather than what I wanted to be true: a bidirectional matrix where each rule is shown catching shapes the other misses. The previous matrix tested only the four shapes where the successor wins, which is precisely why the regression shipped — a test set selected to confirm the thesis. Also corrected: a stale ADR sentence claiming a third wrong ledger version that does not exist (the table it described now reads +3 over terms summing to +3), and a changeset whose stated motivation was the false generated-copy conflict. Verification runs on the remote runner. Refs #3914 * fix(#3914): the exemption term was a no-op — the net is +4 Fourth correction to this ledger, and a fourth error of the same kind. Every version counted removing the n/no-process-exit 'off' entry from the hooks block as -1. Measured: calculateConfigForFile returns undefined for that rule on hooks/**. It was never registered there, and no broader block sets it globally, so the 'off' entry overrode nothing and removing it changed no enforcement at all. A no-op removal, not a guard removal — the same category error as counting baseline acknowledgement entries: a thing that is not a guard, in guard units. It is misattributed too; that block came down in d98b55562 (#3910), already on next before this branch existed. So the epic added FOUR guards, not three. This surfaced from a test of mine that overclaimed. I asserted n/no-process-exit was error on "all nine CommonJS/hook globs" — but hooks is not one of the nine, and the rule resolves to undefined there. Fixing the test to match reality is what exposed the ledger term, which is the argument for tests that assert identity rather than a comfortable shape. The hooks state is now pinned explicitly rather than glossed: n/no-process-exit unregistered, local/require-registered-exit error. It is mildly surprising and therefore worth a test. Also updates a pre-existing test that documented the old name-based-only boundary as intentional. The computed-property strengthening deliberately moves that boundary — process['exit'](0) was caught by NEITHER rule before — so the test now asserts the new contract and cites the ADR, rather than being left to fail or the rule weakened to satisfy it. A contract change should read as deliberate in the test that pins it. Verification runs on the remote runner. Refs #3914 * chore(#3914): backfill changeset pr number to 4018 --------- Co-authored-by: sim --- .changeset/noble-lemurs-sprint.md | 5 + docs/adr/3889-process-exit-contract.md | 64 ++++- eslint-rules/require-registered-exit.cjs | 96 ++++++-- gsd-core/bin/gsd-tools.cjs | 4 + ...ocs-guard-registration.exempt-baseline.cjs | 5 +- tests/eslint-rules.test.cjs | 223 +++++++++++++++++- 6 files changed, 369 insertions(+), 28 deletions(-) create mode 100644 .changeset/noble-lemurs-sprint.md diff --git a/.changeset/noble-lemurs-sprint.md b/.changeset/noble-lemurs-sprint.md new file mode 100644 index 000000000..4cae89f21 --- /dev/null +++ b/.changeset/noble-lemurs-sprint.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4018 +--- +**`local/require-registered-exit` now catches computed and optional-chained `process.exit()` calls.** The rule previously missed `process['exit']()` and `process?.[k]?.()` forms where the property name is a statically resolvable string, letting a raw terminator slip past the ADR-3889 registered-exit contract. It now resolves a computed property to a string literal (directly, or through a single never-reassigned string-literal-initialized binding) and flags those forms too. `n/no-process-exit` remains registered everywhere it already was — the two rules are complementary, not predecessor/successor, so neither is retired. (#3914) diff --git a/docs/adr/3889-process-exit-contract.md b/docs/adr/3889-process-exit-contract.md index 5810b1204..e69e8b4c4 100644 --- a/docs/adr/3889-process-exit-contract.md +++ b/docs/adr/3889-process-exit-contract.md @@ -296,13 +296,50 @@ and it is the property ADR-2980's declined Option 3 lacked. > it was **promoted from SMELL to VIOLATION**, so it can now fail a build, which it never could > before (`runOracles`'s `get failed()` returns `violations` only). > -> **Measured ledger for the epic as delivered:** −1 oracle (`soft-error-exit-zero`), −1 -> `n/no-process-exit: 'off'` exemption block, +1 rule (`local/require-registered-exit`), +1 registry -> `--check`, +1 generated-docs `--check` (`docs/reference/exit-codes.md`). One further guard changed -> strength rather than count: `untyped-success` SMELL → VIOLATION. Two mis-scoped oracles were -> corrected in passing (`routing-validity`, `value-hygiene`) — see #3913. +> **Measured ledger for the epic as delivered** (corrected again by #3914's audit — the version +> first written here was wrong twice over, see below): > -> **Net −1 by count**, not −4. +> | | | +> |---|--:| +> | −1 oracle (`soft-error-exit-zero`) | −1 | +> | ~~−1 `n/no-process-exit: 'off'` hooks exemption block~~ — **0, see below** | 0 | +> | +1 rule (`local/require-registered-exit`) | +1 | +> | +4 `lint:generated-sync --check` arms: `gen-scripts-cli-exit`, `gen-hooks-cli-exit`, `gen-exit-code-registry`, `gen-exit-code-docs` | +4 | +> | **Net** | **+4** | +> +> **The exemption-block term was a fourth error, corrected here.** Every prior version of this ledger +> counted removing the `n/no-process-exit: 'off'` entry from the hooks block as **−1**. Measured: +> `n/no-process-exit` is **not registered at all** on `hooks/**` — `ESLint.calculateConfigForFile` +> returns `undefined` for it there, not `error`. No broader block sets it globally. So the `'off'` +> entry was overriding nothing, and removing it changed no enforcement whatsoever. It is a **no-op +> removal, not a guard removal**, and counting it as −1 is the same category error as counting +> baseline acknowledgement entries: a thing that is not a guard, in guard units. +> +> It is also **misattributed** — that block came down in `d98b55562` (#3910), already on `next` +> before #3914 existed. +> +> **The epic ADDED four guards. It did not remove one.** That is the honest result, and it is worth +> stating without softening: an epic whose thesis is consolidation ended with a larger guard surface +> than it started with. The additions are defensible individually — a rule and four drift checks that +> did not exist — but "net −1" was never true. +> +> One further guard changed strength rather than count: `untyped-success` SMELL → VIOLATION. Two +> mis-scoped oracles were corrected in passing (`routing-validity`, `value-hygiene`) — see #3913. +> +> **Two corrections to what this paragraph previously claimed, both mine:** +> +> 1. It said **"Net −1 by count"** above a term list reading `−1 −1 +1 +1 +1`. That sums to **+1**. +> A plain arithmetic error, in the paragraph immediately below the sentence arguing that an ADR +> about honest accounting must not pad its own ledger. +> 2. The term list also **omitted two of the four `--check` arms** (`gen-scripts-cli-exit` from P0 +> and `gen-hooks-cli-exit` from P7), which is what turns +1 into the real +4. +> +> Recorded rather than quietly rewritten, because the failure this epic exists to close is a written +> claim nobody checked against the thing it describes — and this ledger has now been wrong four +> times, three of them mine: the original "Net −2"; the "Net −1" that replaced it above a term list +> summing to +1; and the "Net +3" that replaced that, stated before the exemption-block term above +> was re-checked and found to be a no-op removal rather than a −1. Each of the four was found by +> someone checking the ledger against the tree, not by re-reading the ledger itself. > > The −4 first written here counted the five pruned `smell-baseline.json` entries in the same units > as oracles and lint rules. They are not guards — they are *acknowledgements* that a guard fired. @@ -322,6 +359,21 @@ and it is the property ADR-2980's declined Option 3 lacked. > `KIND.PROSE` count reached 0 because the sole prose-producing step was changed to > `smart-entry --json`. The promotion is real and the guard now fails a build — but what it > currently guards is that no *new* prose-only step appears, not that an existing one is caught. +> +> **The epic's criterion 5 ("retire the predecessor guard the new rule supersedes") does not apply +> to `n/no-process-exit` and `local/require-registered-exit`.** #3914 originally turned +> `n/no-process-exit` off on `gsd-core/bin/**/*.cjs` and `scripts/**/*.cjs`, on the premise that +> `local/require-registered-exit` was a strict superset there. It is not: the two rules are +> **complementary**, each catching constructs the other misses. Measured directly (successor vs. +> predecessor flag counts): `process['exit'](1)` and `process?.[k]?.(1)` are successor-only (a +> string-literal or optional-chained computed property the predecessor's Identifier-only property +> match never reaches); a function parameter, a destructured binding, a reassign-to-the-same-value +> binding, a `for`-of loop variable, a `var` redeclaration, a catch param, and an undeclared global +> — all literally named `exit` — are predecessor-only (the predecessor's esquery selector matches on +> the AST node's own `.name`, not a resolved value, while the successor only resolves a computed +> property through a single never-reassigned string-literal initializer). Retiring either rule on +> that shared surface loses real coverage. Criterion 5 assumed a replacement relationship that does +> not exist for this pair; both rules remain registered everywhere they were before. ## Revisit if diff --git a/eslint-rules/require-registered-exit.cjs b/eslint-rules/require-registered-exit.cjs index c639f04e6..a3830a91b 100644 --- a/eslint-rules/require-registered-exit.cjs +++ b/eslint-rules/require-registered-exit.cjs @@ -51,15 +51,30 @@ const path = require('node:path'); * the call site and fails loudly (an unused-disable lint error) if the * surrounding code changes such that it is no longer needed. * + * ── Computed member access — `process['exit']()` / `process[x]()` ───────── + * + * A `MemberExpression` callee on `process` is followed whether or not it is + * computed. For a computed property the property name is resolved via + * `resolveComputedPropertyName` below: + * + * - A string `Literal` property (`process['exit'](0)`) resolves directly. + * - An `Identifier` property (`process[exit](0)`) resolves ONLY when it is + * statically determinable: the identifier must bind to exactly one + * variable declaration in scope, that declaration's initializer must be + * a string `Literal`, and the variable must have at most one write + * reference (its own initializer — i.e. never reassigned). This closes + * `const exit = 'exit'; process[exit](1);`. + * + * A genuinely dynamic computed property (a runtime value, a function call, a + * reassigned binding, or an identifier with no resolvable single-literal + * definition) resolves to `null` and is deliberately NOT flagged — the rule + * never guesses at a property name it cannot prove. + * * ── Known limits (documented, deliberately out of scope) ──────────────────── * - * The rule matches a literal `CallExpression` shaped exactly like - * `process.exit(...)` (a non-computed MemberExpression on an Identifier - * named `process` with a property named `exit`). It does NOT do scope/flow - * analysis, so it cannot catch: + * Even with the computed-property resolution above, the rule does NOT do + * general binding/flow analysis, so it still cannot catch: * - * - `process['exit'](0)` — computed member access (same identifier, but - * `callee.computed` is true so the property-name check never runs). * - `const e = process.exit; e(1);` — aliasing the function reference to a * local binding before calling it; by the time the alias is called, the * callee is a plain Identifier, not a MemberExpression on `process`. @@ -68,14 +83,13 @@ const path = require('node:path'); * the outer CallExpression's callee is `process.exit.call`, not * `process.exit` itself. * - * Catching these would require binding/scope-aware analysis (tracking that a - * local variable or a `.call`/`.apply` receiver resolves back to - * `process.exit`), which is a materially different and more expensive class - * of rule. Out of scope for this issue. See the pinning tests in - * tests/eslint-rules.test.cjs ("KNOWN LIMIT (pinned, not endorsed)") that - * assert these are NOT flagged today — if a future change starts catching - * one of them, those tests will fail loudly instead of the change silently - * altering the rule's reach. + * Catching these would require a materially different and more expensive + * class of analysis (tracking that a local variable or a `.call`/`.apply` + * receiver resolves back to `process.exit`). Out of scope for this issue. + * See the pinning tests in tests/eslint-rules.test.cjs ("KNOWN LIMIT (pinned, + * not endorsed)") that assert these are NOT flagged today — if a future + * change starts catching one of them, those tests will fail loudly instead + * of the change silently altering the rule's reach. */ /** @@ -100,6 +114,47 @@ function isInsideFunctionNamed(node, name) { return false; } +/** + * Resolves the property name of a computed `MemberExpression` property node + * to a string, or returns `null` when it cannot be statically determined. + * See the module doc comment ("Computed member access") for the resolution + * rules. `callNode` is used to anchor scope lookup for an Identifier + * property. + */ +function resolveComputedPropertyName(propertyNode, callNode, context) { + if (propertyNode.type === 'Literal' && typeof propertyNode.value === 'string') { + return propertyNode.value; + } + if (propertyNode.type === 'Identifier') { + const scope = + context.sourceCode && typeof context.sourceCode.getScope === 'function' + ? context.sourceCode.getScope(callNode) + : context.getScope(); + let cur = scope; + while (cur) { + const variable = cur.variables.find((v) => v.name === propertyNode.name); + if (variable) { + if (variable.defs.length !== 1) return null; + const def = variable.defs[0]; + if ( + def.type !== 'Variable' || + !def.node.init || + def.node.init.type !== 'Literal' || + typeof def.node.init.value !== 'string' + ) { + return null; + } + const writeRefs = variable.references.filter((r) => r.isWrite()); + if (writeRefs.length > 1) return null; + return def.node.init.value; + } + cur = cur.upper; + } + return null; + } + return null; +} + /** @type {import('eslint').Rule.RuleModule} */ const rule = { meta: { @@ -122,9 +177,18 @@ const rule = { return { CallExpression(node) { const callee = node.callee; - if (callee.type !== 'MemberExpression' || callee.computed) return; + if (callee.type !== 'MemberExpression') return; if (callee.object.type !== 'Identifier' || callee.object.name !== 'process') return; - if (callee.property.type !== 'Identifier' || callee.property.name !== 'exit') return; + + let propertyName; + if (!callee.computed) { + if (callee.property.type !== 'Identifier') return; + propertyName = callee.property.name; + } else { + propertyName = resolveComputedPropertyName(callee.property, node, context); + if (propertyName === null) return; + } + if (propertyName !== 'exit') return; const filename = context.filename ?? context.getFilename(); if (path.basename(filename) === 'cli-exit.cts' && isInsideFunctionNamed(node, 'terminateNow')) return; diff --git a/gsd-core/bin/gsd-tools.cjs b/gsd-core/bin/gsd-tools.cjs index d3e076371..fd79997d9 100755 --- a/gsd-core/bin/gsd-tools.cjs +++ b/gsd-core/bin/gsd-tools.cjs @@ -254,6 +254,10 @@ try { // at this point in the process's lifetime — there is nothing to route // through. This is the second (and only other) sanctioned allowlist entry // for local/require-registered-exit, alongside terminateNow's own body. + // #3914: n/no-process-exit and local/require-registered-exit are + // complementary, not predecessor/successor (see eslint.config.mjs and + // docs/adr/3889-process-exit-contract.md) — both remain 'error' on this + // glob, so both need a disable directive here. // eslint-disable-next-line n/no-process-exit, local/require-registered-exit process.exit(1); } diff --git a/scripts/lint-docs-guard-registration.exempt-baseline.cjs b/scripts/lint-docs-guard-registration.exempt-baseline.cjs index 859837e99..7b7dee9f0 100644 --- a/scripts/lint-docs-guard-registration.exempt-baseline.cjs +++ b/scripts/lint-docs-guard-registration.exempt-baseline.cjs @@ -139,7 +139,10 @@ const DOCS_GUARD_EXEMPT_DOCS_PATHS = { 'declarative-reference-antigravity.test.cjs': ['docs/cli/features'], 'declarative-reference-zcode.test.cjs': ['docs/reference/host-integration-capability-matrix.md'], 'emitted-attribution.test.cjs': ['docs/README.md', 'docs/tests', 'docs/tests/helpers/install-shared.cjs'], - 'eslint-rules.test.cjs': ['docs/readme.md'], + // #3914: cites docs/adr/3889-process-exit-contract.md ~:350-362 in an + // explanatory comment describing the intentional computed-property + // boundary move; the file never reads that (or any) docs/ file. + 'eslint-rules.test.cjs': ['docs/adr/3889-process-exit-contract.md', 'docs/readme.md'], 'estimate-calibrate.test.cjs': [ 'docs/adr', 'docs/adr/2629-phase-effort-estimation-calibration.md', 'docs/reference', 'docs/reference/planning-artifacts.md', diff --git a/tests/eslint-rules.test.cjs b/tests/eslint-rules.test.cjs index 30ec07059..269a12ed1 100644 --- a/tests/eslint-rules.test.cjs +++ b/tests/eslint-rules.test.cjs @@ -19,9 +19,10 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); -const { RuleTester, ESLint } = require('eslint'); +const { RuleTester, ESLint, Linter } = require('eslint'); const path = require('node:path'); const fc = require('fast-check'); +const pluginN = require('eslint-plugin-n'); const noSourceGrep = require('../eslint-rules/no-source-grep.cjs'); const noMagicSleepInTests = require('../eslint-rules/no-magic-sleep-in-tests.cjs'); @@ -3439,12 +3440,24 @@ describe('require-registered-exit rule', () => { }); }); - test('valid: computed member access process["exit"](0) is not flagged (documented boundary, name-based matching only)', () => { + // #3914 (epic #3889 Phase 7 follow-up) moved this boundary on purpose: see + // docs/adr/3889-process-exit-contract.md ~:350-362. Previously + // process['exit'](0) was caught by NEITHER this rule (name-based matching + // only) nor n/no-process-exit's Identifier-only property match, so it was + // a genuine, silent evasion. The rule now resolves a string-literal + // computed property the same as a dotted one, closing that gap. This is + // NOT a weakening — the old "not flagged" behavior documented below is + // superseded and should never be restored to make this test pass again. + test('invalid: computed member access process["exit"](0) IS flagged (boundary moved by #3914, ADR-3889)', () => { ruleTester.run('require-registered-exit', requireRegisteredExit, { - valid: [ - { code: `process['exit'](0);`, filename: 'src/some-module.cts' }, + valid: [], + invalid: [ + { + code: `process['exit'](0);`, + filename: 'src/some-module.cts', + errors: [{ messageId: 'rawProcessExit' }], + }, ], - invalid: [], }); }); @@ -3558,4 +3571,204 @@ describe('require-registered-exit rule', () => { ); } }); + + // ── #3914 (epic #3889 criterion 5, CORRECTED): the two rules are + // COMPLEMENTARY, not predecessor/successor ────────────────────────────── + // + // An isolated review, plus live measurement (see the parity matrix below), + // proved the original #3914 retirement of n/no-process-exit on + // gsd-core/bin/**/*.cjs and scripts/**/*.cjs was WRONG: local/require- + // registered-exit is NOT a strict superset there. n/no-process-exit's + // esquery selector `[property.name="exit"]` matches ANY MemberExpression + // property node whose own AST `.name` reads `exit`, computed or not, + // regardless of whether the value is statically resolvable — so it catches + // a function parameter, a destructured binding, a for-of loop variable, a + // reassign-to-the-same-string, a `var` redeclaration, a catch param, or an + // undeclared global, all named `exit`, none of which + // local/require-registered-exit's narrower (declaration-must-resolve-to-a- + // single-string-literal) analysis reaches. Conversely, the successor + // catches a string-literal computed property (`process['exit']()`) and an + // optional-chained computed property, neither of which the predecessor's + // Identifier-only property match reaches. Both rules therefore remain + // 'error' everywhere they were already registered; this section documents + // and pins that complementary relationship instead of a supersession that + // does not exist. + // + // n/no-process-exit resolves to a bare string OR an array whose first + // element is severity (possibly numeric 0/1/2) depending on how ESLint + // merges the flat config; normalize before asserting. + function normalizeSeverity(entry) { + const raw = Array.isArray(entry) ? entry[0] : entry; + if (raw === 'off' || raw === 0) return 'off'; + if (raw === 'warn' || raw === 1) return 'warn'; + if (raw === 'error' || raw === 2) return 'error'; + return raw; + } + + // n/no-process-exit is registered ('error') on exactly the nine globs of + // the shared CommonJS block (eslint.config.mjs ~:476-486): gsd-core/bin/**/*.cjs, + // scripts/**/*.cjs, eslint-rules/**/*.cjs, bin/lib/**/*.cjs, pi/**/*.cjs, + // examples/**/*.cjs, vscode/*.js, .kilo/plugins/*.js, .opencode/plugins/*.js. + // hooks/**/*.js is NOT one of them — the hooks block (eslint.config.mjs + // ~:594-619) registers the `n` plugin (for n/no-path-concat) but never sets + // the n/no-process-exit rule key, so it resolves to undefined there, not + // 'off' and not 'error'. Asserting 'error' for a hooks path is a false + // claim about the rule's actual reach; see the companion test below, which + // pins that undefined resolution explicitly. + // bin/lib/**/*.cjs is skipped here: it is a build-time-generated directory + // with no file checked into this repo, so there is no real representative + // path to resolve config for. Eight of the nine globs are exercised. + test('n/no-process-exit is error on eight of its nine CommonJS globs (bin/lib/** has no checked-in file; no supersession)', async () => { + const REPO_ROOT = path.join(__dirname, '..'); + const eslint = new ESLint({ cwd: REPO_ROOT }); + const allPaths = [ + path.join(REPO_ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs'), + path.join(REPO_ROOT, 'scripts', 'affected-tests-lib.cjs'), + path.join(REPO_ROOT, 'eslint-rules', 'no-source-grep.cjs'), + path.join(REPO_ROOT, 'pi', 'gsd.cjs'), + path.join(REPO_ROOT, 'examples', 'dynamic-context-management', 'demo.cjs'), + path.join(REPO_ROOT, 'vscode', 'extension.js'), + path.join(REPO_ROOT, '.kilo', 'plugins', 'gsd-core.js'), + path.join(REPO_ROOT, '.opencode', 'plugins', 'gsd-core.js'), + ]; + for (const p of allPaths) { + const config = await eslint.calculateConfigForFile(p); + assert.strictEqual( + normalizeSeverity(config.rules['n/no-process-exit']), + 'error', + `expected n/no-process-exit to be error for ${path.relative(REPO_ROOT, p)}`, + ); + } + }); + + // hooks/**/*.js is NOT among n/no-process-exit's registered globs (see + // above): the rule resolves to undefined there, while local/require- + // registered-exit — the successor rule for this surface — is 'error'. + // This pins the real, slightly surprising state (an unregistered rule, + // not an 'off' rule) rather than papering over it with a false 'error' + // claim. + test('on hooks/** n/no-process-exit is unregistered (undefined) while local/require-registered-exit is error', async () => { + const REPO_ROOT = path.join(__dirname, '..'); + const eslint = new ESLint({ cwd: REPO_ROOT }); + const p = path.join(REPO_ROOT, 'hooks', 'gsd-check-update.js'); + const config = await eslint.calculateConfigForFile(p); + assert.strictEqual( + config.rules['n/no-process-exit'], + undefined, + `expected n/no-process-exit to be unregistered for ${path.relative(REPO_ROOT, p)}`, + ); + assert.strictEqual( + normalizeSeverity(config.rules['local/require-registered-exit']), + 'error', + `expected local/require-registered-exit to be error for ${path.relative(REPO_ROOT, p)}`, + ); + }); + + test('local/require-registered-exit is error on all four of its globs', async () => { + const REPO_ROOT = path.join(__dirname, '..'); + const eslint = new ESLint({ cwd: REPO_ROOT }); + const registeredPaths = [ + path.join(REPO_ROOT, 'src', 'cli-exit.cts'), + path.join(REPO_ROOT, 'scripts', 'affected-tests-lib.cjs'), + path.join(REPO_ROOT, 'hooks', 'gsd-check-update.js'), + path.join(REPO_ROOT, 'gsd-core', 'bin', 'gsd-tools.cjs'), + ]; + for (const p of registeredPaths) { + const config = await eslint.calculateConfigForFile(p); + assert.strictEqual( + normalizeSeverity(config.rules['local/require-registered-exit']), + 'error', + `expected local/require-registered-exit to be error for ${path.relative(REPO_ROOT, p)}`, + ); + } + }); + + // ── #3914 (corrected): bidirectional construct-parity matrix ──────────── + // + // The severity-registration tests above prove both rules are 'error' on + // the shared globs — they don't prove the two rules' AST reach relative to + // each other. This matrix lints each measured shape through BOTH rules + // directly (via `Linter`) and asserts the true, bidirectional relationship: + // each rule catches constructs the other misses; neither over-fires on a + // genuinely dynamic property. + describe('construct parity with n/no-process-exit (complementary, not predecessor/successor)', () => { + const FILES_GLOB = ['**/*.js', '**/*.cjs', '**/*.cts']; + + function lintLocal(ruleModule, code, filename) { + const linter = new Linter(); + const config = { + files: FILES_GLOB, + languageOptions: { ecmaVersion: 2022, sourceType: 'commonjs' }, + plugins: { local: { rules: { 'require-registered-exit': ruleModule } } }, + rules: { 'local/require-registered-exit': 'error' }, + }; + return linter.verify(code, config, { filename }); + } + + function lintPredecessor(code, filename) { + const linter = new Linter(); + const config = { + files: FILES_GLOB, + languageOptions: { ecmaVersion: 2022, sourceType: 'commonjs' }, + plugins: { n: pluginN }, + rules: { 'n/no-process-exit': 'error' }, + }; + return linter.verify(code, config, { filename }); + } + + test('process.exit(1) — BOTH rules flag (plain member access)', () => { + const code = 'process.exit(1);'; + assert.strictEqual(lintPredecessor(code, 'x.cjs').length, 1); + assert.strictEqual(lintLocal(requireRegisteredExit, code, 'x.cjs').length, 1); + }); + + test("process['exit'](1) — successor-ONLY (string-literal computed property; predecessor's Identifier-only property match cannot see it)", () => { + const code = "process['exit'](1);"; + assert.strictEqual(lintPredecessor(code, 'x.cjs').length, 0); + assert.strictEqual(lintLocal(requireRegisteredExit, code, 'x.cjs').length, 1); + }); + + test('process?.[k]?.(1) with k statically "exit" — successor-ONLY (optional-chained computed property)', () => { + const code = "const k = 'exit'; process?.[k]?.(1);"; + assert.strictEqual(lintPredecessor(code, 'x.cjs').length, 0); + assert.strictEqual(lintLocal(requireRegisteredExit, code, 'x.cjs').length, 1); + }); + + test('function f(exit) { process[exit](1); } — predecessor-ONLY (parameter named exit; not a resolvable literal binding)', () => { + const code = 'function f(exit) { process[exit](1); }'; + assert.strictEqual(lintPredecessor(code, 'x.cjs').length, 1); + assert.strictEqual(lintLocal(requireRegisteredExit, code, 'x.cjs').length, 0); + }); + + test("let exit='exit'; exit='exit'; process[exit]() — predecessor-ONLY (reassigned-to-same-value binding disqualifies the successor's single-write check)", () => { + const code = "let exit = 'exit'; exit = 'exit'; process[exit]();"; + assert.strictEqual(lintPredecessor(code, 'x.cjs').length, 1); + assert.strictEqual(lintLocal(requireRegisteredExit, code, 'x.cjs').length, 0); + }); + + test('const { exit } = obj; process[exit](1) — predecessor-ONLY (destructured binding; no string-literal initializer to resolve)', () => { + const code = "const { exit } = require('x'); process[exit](1);"; + assert.strictEqual(lintPredecessor(code, 'x.cjs').length, 1); + assert.strictEqual(lintLocal(requireRegisteredExit, code, 'x.cjs').length, 0); + }); + + test('process[someRuntimeValue](1) with a genuinely dynamic value — NEITHER rule flags (no over-firing)', () => { + const code = + 'function pick(v) { return v; } ' + + 'const someRuntimeValue = pick("exit"); ' + + 'process[someRuntimeValue](1);'; + assert.strictEqual(lintPredecessor(code, 'x.cjs').length, 0); + assert.strictEqual(lintLocal(requireRegisteredExit, code, 'x.cjs').length, 0); + }); + + test('a sanctioned process.exit() inside terminateNow in cli-exit.cts is still not flagged by the successor (allowlist unaffected)', () => { + const code = 'function terminateNow(outcome, payload) {\n process.exit(2);\n}'; + assert.strictEqual(lintLocal(requireRegisteredExit, code, 'src/cli-exit.cts').length, 0); + }); + + test('a sanctioned process.exit() inside terminateNow is still flagged by the predecessor (why both directives are needed at that call site)', () => { + const code = 'function terminateNow(outcome, payload) {\n process.exit(2);\n}'; + assert.strictEqual(lintPredecessor(code, 'src/cli-exit.cts').length, 1); + }); + }); });