* fix(#3914): retire n/no-process-exit where its successor governs
Epic #3889 criterion 5 — no phase closes with a guard added and its
predecessor left standing — is violated in the tree by the epic that wrote it.
local/require-registered-exit was registered on gsd-core/bin/**/*.cjs and
scripts/**/*.cjs, while n/no-process-exit stayed 'error' over a nine-glob block
covering those same two. Only the hooks 'off' exemption ever came down; the
predecessor's registration never did. Both rules have been enforcing the same
property on the same surfaces since P6.
Narrowed, not deleted. Seven of those nine globs have NO successor —
eslint-rules/, bin/lib/, pi/, examples/, vscode/, .kilo/, .opencode/ — so
deleting the rule outright would silently drop enforcement on all seven. That
is the inversion this epic has already hit three times: removing a coarse guard
because a narrower one exists somewhere it does not reach. Flat config is
last-match-wins and both successor blocks come after the nine-glob block, so
'n/no-process-exit': 'off' in exactly those two retires the predecessor
precisely where the successor governs and nowhere else.
The successor is strictly more precise: it permits process.exit only inside
terminateNow in cli-exit.cts, the single sanctioned terminator (ADR-3889 §3),
where n/no-process-exit permits none and would flag terminateNow's own
generated copy.
Asserted at the consumer's altitude via ESLint.calculateConfigForFile on real
paths, with the positive control that matters: n/no-process-exit is still
'error' on six of the seven successor-less globs, so a future edit that turns
this into a blanket disable goes red. bin/lib/ has no file in this checkout and
is reported as untested rather than given an invented path. Severity is
normalized across the string/numeric/array forms the API can return, and the
normalized value asserted — not truthiness.
Verified by running calculateConfigForFile myself on both superseded globs and
four controls before trusting the test.
Found and fixed inline: the change made an eslint-disable directive at
gsd-tools.cjs:257 partially unused, which --max-warnings 0 rejects; narrowed to
the one rule still in force.
Verification runs on the remote runner.
Refs #3914
* docs(#3914): the epic added three guards, it did not remove one
The audit reconciled the epic ledger against what actually landed. The net is
+3, not -1: four lint:generated-sync --check arms (gen-scripts-cli-exit,
gen-hooks-cli-exit, gen-exit-code-registry, gen-exit-code-docs) plus one rule,
against two retirements.
An epic whose thesis was consolidation ended with a larger guard surface than
it started with. The additions are each defensible; the claim that the total
fell was never true.
Two of the three prior errors in this amendment are mine. It said "Net -1 by
count" above terms reading -1 -1 +1 +1 +1, which sums to +1 — an arithmetic
error in the paragraph directly below the sentence arguing that an ADR about
honest accounting must not pad its own ledger. And the term list omitted two of
the four --check arms, which is what turns that +1 into the real +3.
Recorded rather than quietly rewritten. This ledger has now been wrong three
times — the original -2, the -1 that replaced it, and #3914's own table, which
states -1 above terms summing to 0 — and a written claim nobody checked against
the thing it describes is the exact failure this epic exists to close.
Refs #3914
* fix(#3914): make the successor actually supersede before retiring the predecessor
An isolated security review found that the previous commit turned off a guard
that was still doing work. Reproduced by executing both rules against a
fixture, not inferred:
const exit = 'exit';
process[exit](1);
n/no-process-exit flags it; local/require-registered-exit did not, because it
early-returned on callee.computed. So retiring the predecessor on
gsd-core/bin/**/*.cjs and scripts/**/*.cjs un-guarded that shape on precisely
the two globs this epic's exit contract cares most about.
This is the third time in this epic I have removed a coarse guard on the claim
that a narrower one covered it, without checking construct-level parity — after
the allowlist key-to-prefix-to-exact-membership sequence and the band
ranges-to-categories one. The rule is the same every time: a narrower guard
supersedes a coarser one only where it demonstrably reaches at least as far,
and "demonstrably" means executing both against the constructs, not reading
either.
The successor now resolves computed property access for the statically
determinable cases — a string Literal, and an Identifier bound once to a string
Literal, resolved through scope — and leaves genuinely dynamic properties
alone so the rule does not over-fire. Measured after the fix: plain
process.exit flagged, process['exit']() flagged, process[exit]() flagged,
process[globalThis.k]() not flagged. That makes it a strict superset of the
predecessor on these globs, since process['exit']() was caught by NEITHER rule
before.
The second finding is worse than the first, because it was reasoning rather
than oversight. My justification comment claimed n/no-process-exit "would flag
terminateNow's own generated copy here". It would not — that file is in the
global ignore list, so neither rule ever lints it. There was no conflict to
resolve; I wrote a rationale I had not checked, in a change whose entire
subject is written claims nobody verified. Both comment blocks now state the
real basis.
The tests that should have caught this asserted only rule SEVERITY per glob and
never construct REACH, which is exactly how a coverage hole passed. A parity
matrix now pins all five shapes, including a RED/GREEN regression pin against
an inlined reproduction of the pre-fix rule — inlined rather than loaded from
HEAD, because HEAD resolves to the fixed commit under the remote runner and
would silently stop testing anything.
Verification runs on the remote runner.
Refs #3914
* fix(#3914): the two exit rules are complementary — keep both
Reverts this branch's retirement of n/no-process-exit. The premise was wrong
twice, and the second review proved the change itself was wrong.
I claimed local/require-registered-exit was a strict superset on
gsd-core/bin/**/*.cjs and scripts/**/*.cjs. Measured, successor vs predecessor:
function f(exit) { process[exit](1); } 0 vs 1
let exit='exit'; exit='exit'; process[exit]() 0 vs 1
const { exit } = ...; process[exit](1) 0 vs 1
plus for-of bindings, let-then-assign, var redeclaration, catch params, and an
undeclared global named exit. The predecessor matches any identifier NAMED
exit however it is bound; the successor resolves only a string literal or a
single-write const. It never was a superset — I asserted the relationship after
fixing one construct and did not re-check the rest.
The justification was independently false: all three generated cli-exit copies
are in the global ignore list, so n/no-process-exit was never flagging
terminateNow. There was no conflict to resolve. I wrote a rationale I had not
verified, in the phase whose subject is written claims nobody checked.
So criterion 5 does not apply to this pair. They are not predecessor and
successor — they are complementary, each catching constructs the other misses.
The epic's criterion assumed a replacement relationship that does not exist
here, and retiring either rule loses real coverage. The ADR ledger now says so
with the measured shapes.
What survives is the genuine improvement: the computed-property strengthening.
local/require-registered-exit now catches process['exit'](1) and optional-chain
terminators like process?.[k]?.(1), which NEITHER rule caught before, while
correctly ignoring a genuinely dynamic property so it does not over-fire.
The parity tests are rewritten to assert what is true rather than what I wanted
to be true: a bidirectional matrix where each rule is shown catching shapes the
other misses. The previous matrix tested only the four shapes where the
successor wins, which is precisely why the regression shipped — a test set
selected to confirm the thesis.
Also corrected: a stale ADR sentence claiming a third wrong ledger version that
does not exist (the table it described now reads +3 over terms summing to +3),
and a changeset whose stated motivation was the false generated-copy conflict.
Verification runs on the remote runner.
Refs #3914
* fix(#3914): the exemption term was a no-op — the net is +4
Fourth correction to this ledger, and a fourth error of the same kind.
Every version counted removing the n/no-process-exit 'off' entry from the hooks
block as -1. Measured: calculateConfigForFile returns undefined for that rule on
hooks/**. It was never registered there, and no broader block sets it globally,
so the 'off' entry overrode nothing and removing it changed no enforcement at
all. A no-op removal, not a guard removal — the same category error as counting
baseline acknowledgement entries: a thing that is not a guard, in guard units.
It is misattributed too; that block came down in d98b55562 (#3910), already on
next before this branch existed.
So the epic added FOUR guards, not three.
This surfaced from a test of mine that overclaimed. I asserted n/no-process-exit
was error on "all nine CommonJS/hook globs" — but hooks is not one of the nine,
and the rule resolves to undefined there. Fixing the test to match reality is
what exposed the ledger term, which is the argument for tests that assert
identity rather than a comfortable shape.
The hooks state is now pinned explicitly rather than glossed: n/no-process-exit
unregistered, local/require-registered-exit error. It is mildly surprising and
therefore worth a test.
Also updates a pre-existing test that documented the old name-based-only
boundary as intentional. The computed-property strengthening deliberately moves
that boundary — process['exit'](0) was caught by NEITHER rule before — so the
test now asserts the new contract and cites the ADR, rather than being left to
fail or the rule weakened to satisfy it. A contract change should read as
deliberate in the test that pins it.
Verification runs on the remote runner.
Refs #3914
* chore(#3914): backfill changeset pr number to 4018
---------
Co-authored-by: sim <sim@local>
203 lines
8.9 KiB
JavaScript
203 lines
8.9 KiB
JavaScript
'use strict';
|
|
|
|
const path = require('node:path');
|
|
|
|
/**
|
|
* require-registered-exit
|
|
*
|
|
* Issue #3910 (epic #3889 Phase 6): "the raw terminator is banned by
|
|
* construction." A raw `process.exit(...)` call bypasses the registered
|
|
* exit-contract machinery in src/cli-exit.cts (ExitError/runMain for a CLI
|
|
* path, terminateNow for a hook) — the whole point of ADR-3889 is that
|
|
* EVERY process termination is projected through one of those two seams, so
|
|
* a bare `process.exit()` re-opens exactly the "nothing fails with success"
|
|
* defect class the epic exists to close.
|
|
*
|
|
* Flags: any `CallExpression` whose callee is a `MemberExpression` with
|
|
* `object.name === 'process'` and `property.name === 'exit'` — i.e.
|
|
* `process.exit(...)`.
|
|
*
|
|
* Does NOT flag `process.exitCode = N` — that assignment is the CORRECT
|
|
* drain-then-exit pattern `runMain` itself uses, and conflating the two is
|
|
* what inflated this epic's original raw-`process.exit()` census 2x (a
|
|
* `MemberExpression` assignment target is never a `CallExpression`, so this
|
|
* rule's `CallExpression`-only selector already excludes it structurally;
|
|
* see the negative-control tests in tests/eslint-rules.test.cjs).
|
|
*
|
|
* ── Allowlist (exactly two sites, repo-wide) ────────────────────────────────
|
|
*
|
|
* 1. The body of `terminateNow` in src/cli-exit.cts — the single sanctioned
|
|
* terminator (ADR-3889 §3: write-then-terminate, the only place exit code
|
|
* 2 — the hook-protocol deny — may be produced). Detected STRUCTURALLY
|
|
* below (any process.exit() call lexically nested inside a function
|
|
* declaration/expression named `terminateNow`, AND the file's basename is
|
|
* `cli-exit.cts`), not by a path+line number, which rots the instant the
|
|
* function grows or moves. The basename constraint is required in
|
|
* addition to the name check: without it, any function named
|
|
* `terminateNow` anywhere in the repo would silently inherit the
|
|
* allowlist, widening the rule's trust boundary to a name that can be
|
|
* typo'd or copy-pasted into an unrelated module.
|
|
*
|
|
* 2. gsd-core/bin/gsd-tools.cjs's `ensureRuntimeBuild` bootstrap-failure path
|
|
* (see its own inline `// eslint-disable-next-line local/require-registered-exit`
|
|
* comment). That call runs BEFORE `./lib/cli-exit.cjs` is even required —
|
|
* the registered-exit seam does not exist yet at that point in the
|
|
* process's lifetime, so there is nothing to route through. An inline
|
|
* disable directive WITH a reason comment at that one call site was
|
|
* chosen over a hardcoded path in this rule for the same reason
|
|
* terminateNow's allowlisting is structural rather than path-based: a
|
|
* path-keyed allowlist here would silently stop protecting the file the
|
|
* moment its bootstrap code moved, while an inline directive travels with
|
|
* 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) ────────────────────
|
|
*
|
|
* Even with the computed-property resolution above, the rule does NOT do
|
|
* general binding/flow analysis, so it still cannot catch:
|
|
*
|
|
* - `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`.
|
|
* - `process.exit.call(null, 1)` / `process.exit.apply(null, [1])` —
|
|
* invoking `process.exit` indirectly via Function.prototype.call/apply;
|
|
* the outer CallExpression's callee is `process.exit.call`, not
|
|
* `process.exit` itself.
|
|
*
|
|
* 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.
|
|
*/
|
|
|
|
/**
|
|
* True if `node` (a CallExpression) is lexically nested inside a function
|
|
* declaration or function expression named `name`, walking up the ESLint
|
|
* `.parent` chain. Used to allowlist the terminateNow body structurally —
|
|
* see the module doc comment above.
|
|
*/
|
|
function isInsideFunctionNamed(node, name) {
|
|
let current = node.parent;
|
|
while (current) {
|
|
if (
|
|
(current.type === 'FunctionDeclaration' || current.type === 'FunctionExpression') &&
|
|
current.id &&
|
|
current.id.type === 'Identifier' &&
|
|
current.id.name === name
|
|
) {
|
|
return true;
|
|
}
|
|
current = current.parent;
|
|
}
|
|
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: {
|
|
type: 'problem',
|
|
docs: {
|
|
description:
|
|
'Disallow raw process.exit() outside terminateNow — route CLI paths through runMain/ExitError, hooks through terminateNow, and drain-only exits through process.exitCode',
|
|
category: 'Best Practices',
|
|
},
|
|
schema: [],
|
|
messages: {
|
|
rawProcessExit:
|
|
'Raw process.exit() is banned outside terminateNow (ADR-3889). Use runMain/ExitError '
|
|
+ '(src/cli-exit.cts) for a CLI entrypoint, terminateNow (src/cli-exit.cts) for a hook that '
|
|
+ 'must write-then-terminate immediately, or set process.exitCode and let the process drain '
|
|
+ 'naturally when nothing needs an immediate hard exit.',
|
|
},
|
|
},
|
|
create(context) {
|
|
return {
|
|
CallExpression(node) {
|
|
const callee = node.callee;
|
|
if (callee.type !== 'MemberExpression') return;
|
|
if (callee.object.type !== 'Identifier' || callee.object.name !== 'process') 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;
|
|
|
|
context.report({ node, messageId: 'rawProcessExit' });
|
|
},
|
|
};
|
|
},
|
|
};
|
|
|
|
module.exports = rule;
|