d98b55562c
* enhance(#3910): move the last src/ terminators onto the seam Phase 6 bans the raw terminator by construction, which it cannot do while violations stand. A census found 12 sites the rule would flag; nine of the ten unsanctioned ones were owned by no phase of the epic at all — a coverage hole in the decomposition, since P0-P2 are infra, P3 the gate modules, P4 the scanners, P5 the fragments, P7 the hooks, P8 io.cts, and P6 itself only adds the rule. `src/**/*.cts` now holds exactly 2 raw exits, both inside `terminateNow`, the single sanctioned site. `io.cts`'s `error()` is the interesting one. It was first called substantive on "dozens of callers, contract risk" — asserted, not measured, and the measurement refuted it: 289 call sites, zero inside a try whose catch would swallow a throw. The real obstacle was structural instead: `terminateNow` cannot emit exit 1, because ADR-3889 §1 makes 0 and 1 unallocatable and `nameForExitCode(1)` throws. So the only route is `ExitError` under `runMain`, which sets exitCode and writes stderr only when the error carries a user message — keeping the existing stderr write and throwing a message-less ExitError is observably identical. That census was still too narrow, and running the CLI proved it. It asked whether the CALL sits in a try/catch; the two regressions that surfaced were interceptors elsewhere on the stack: - `command-routing-hub.cts`'s `dispatch()` swallowed the ExitError into a HandlerFailure, so the caller emitted a duplicated, wrong stderr line on every Hub-routed path. It now rethrows ExitError explicitly — the same shape `gsd-tools.cjs` already used at two dispatch sites, so this follows an established idiom rather than inventing one. - the profile-pipeline router's deliberately un-awaited `.catch(e => error(...))` turned an ExitError rejection into an uncaught exception; it now mirrors runMain's handling. `edge-probe` and `ui-consideration-probe` gained `runMain` wrappers because probe-core's new throwing default would otherwise have escaped them. A follow-up sweep of every dispatcher — 19 command routers, the Hub, the gsd-tools dispatch seams — found no further swallowing catch. The admitted bound: ~1260 non-rethrowing catches repo-wide were scanned structurally but not individually classified. Both real regressions were found by execution, not by reading, so the suite is the detector that matters here. `gsd-tools.cjs:253` stays a raw exit deliberately: it is the ensureRuntimeBuild bootstrap, which runs before cli-exit is required, so the seam does not yet exist. It needs a second allowlist entry, which means #3910's "single allowlist entry" criterion is unachievable as written. Verification runs on the remote runner. Refs #3910 * enhance(#3910): ban the raw terminator by construction Adds local/require-registered-exit and registers it on all four globs: src/**/*.cts, scripts/**/*.cjs, hooks/**/*.js, gsd-core/bin/**/*.cjs. Registering on the .cts glob is load-bearing, not redundant — the emitted .cjs mirrors are globally eslint-ignored, so a rule registered only on the emitted globs is blind to the sources. That is the #3496 lesson, and it is how the previous guard became invisible: n/no-process-exit was 'error' in one block yet fired zero times on all three surfaces that mattered. The dead n/no-process-exit: 'off' block for hooks is deleted in the same PR. Phase 7 migrated every hook, so the exemption now protects nothing. Two allowlist entries, not the one #3910 anticipated. terminateNow's body is detected STRUCTURALLY — a process.exit lexically inside a function of that name — rather than by a path and line number that rots. The second is gsd-tools.cjs's ensureRuntimeBuild bootstrap, an inline disable with its reason at the call site: it runs before ./lib/cli-exit.cjs is required, so the seam does not exist yet and no migration is possible. #3910's 'single allowlist entry' criterion is therefore unachievable as written, and is amended with the measurement rather than quietly missed. The rule is proven able to FAIL, per glob: four positive controls, one for each registered glob. A guard that cannot be shown to fire is not a guard. Four matching negative controls pin process.exitCode as never-flagged — conflating it with process.exit is what inflated this epic's original census 2x. An allowlist case and a near-miss (same shape, different function name) fix the structural detection in place. Verification runs on the remote runner. Refs #3910 * fix(#3910): stop the detached catch from throwing, and scope the allowlist Review findings, one of them a regression the previous fix introduced. _handlePipelineRejection called error() from inside a DETACHED .catch(). error() now throws, so that throw became an unhandled promise rejection — and on Node >=15 with --unhandled-rejections=throw, Node dumps a raw stack trace with absolute paths on top of the clean Error: line. That was impossible before this branch, because process.exit(1) terminated synchronously before any rejection machinery could observe it. The handler now writes byte-identical stderr itself, in both plain and --json-errors form, and sets exitCode in place. This was the THIRD interceptor found, and like the first two it surfaced by running the CLI rather than by reading code. The rule's terminateNow allowlist had no path constraint, so any function anywhere named terminateNow across all four globs inherited it. It now requires the structural nesting check AND a cli-exit.cts basename — still no line numbers to rot. The four per-glob positive controls only varied a filename inside RuleTester, which never resolves eslint.config.mjs. Since the rule is filename-agnostic, all four exercised identical logic and none proved the rule was WIRED — this epic's own failure mode. A registration test now asserts the rule resolves for a real path in each glob, and it is proven able to fail: removing one glob's registration flips the resolved value from [2] to undefined. Three evasions the rule cannot catch (computed member, aliasing, .call/.apply) are documented in its header and pinned by tests, labelled as known limits rather than endorsed, so a future change that starts catching them is a deliberate diff. Refs #3910 * docs(#3910): document the raw-terminator ban Reference and Explanation via a new docs/features fragment (FEATURES.md is generated from it, not hand-edited). How-To: docs/how-to/resolve-a-raw-terminator-finding.md, indexed from docs/README.md — a contributor whose code trips the rule picks among three replacements by surface (runMain/ExitError for a CLI path, terminateNow for a hook, process.exitCode where the process should drain), and needs to know why process.exitCode is correct and never flagged, since conflating the two is what inflated this epic's original census 2x. The page also names the three patterns the rule cannot catch and says plainly that using one to dodge it is a review finding, not a fix — documenting them without that sentence would read as a sanctioned workaround. docs/INVENTORY.md deliberately untouched: eslint-rules/ is not a tracked family in the manifest (verified — a regen produced a zero diff), so a hand-written row would desync the table from the family it claims to belong to. Refs #3910 * fix(#3910): a catch that sniffs the message swallows an ExitError The remote run returned 41 failures, and one of them was a live production regression rather than a test artifact. `cmdMilestoneComplete`'s unstarted-phase guard re-threw only when `e.message.startsWith('Cannot mark milestone complete:')`. `error()` used to `process.exit(1)`, uncatchable, so the guard always fired. It now throws an ExitError carrying no message, the string test fails, and the ExitError was silently swallowed — the guard stopped blocking milestone completion entirely. Proven against the real CLI: pre-fix, a milestone with an unstarted phase archived at exit 0; post-fix it is blocked at exit 1 with the intended message. That is a guard that silently stopped guarding, which is this epic's thesis appearing inside the phase meant to enforce it. Worth stating plainly: an earlier census DID examine this site, saw a `throw e`, and classified it as rethrowing. It was wrong — the rethrow is conditional, and a conditional rethrow on an inspected message is indistinguishable from an unconditional one unless you read the predicate. So the class was swept rather than patched where it was tripped over. An AST census of every CatchClause across src/, gsd-core/bin/ and scripts/ found 38 conditional rethrows. Two more had the same defect and are fixed the same way: `config.cts`'s `'No config.json'` sniff and `gsd-tools.cjs`'s `e.name === 'WindowsError'`. The remaining 25 are provably unreachable — every one wraps a bare fs, YAML, manifest-require or git-exec primitive that cannot throw ExitError — and two were scanner false positives, both explained. Each fix is an unconditional `instanceof ExitError` rethrow placed BEFORE any inspection, matching the idiom command-routing-hub and gsd-tools already used. Residual bound, stated rather than implied: zero known-reachable unfixed sites, contingent only on error() never later being called inside one of those 25 primitive try blocks. The remaining failures were harness artifacts, and the harnesses were corrected to the new contract rather than the assertions weakened. Tests that mocked `process.exit` to observe termination now catch ExitError and assert its code; tests parsing stderr as a single JSON object still assert exactly that, with their ad-hoc `node -e` scripts wrapped in runMain so it is true. milestone and phase-resolution-parity needed no test change — they were correctly written against the real bug and are what caught it. Verification runs on the remote runner. Refs #3910 * chore(#3910): backfill the changeset PR number Also reframes the fragment to lead with the user-visible change — the milestone guard blocking again — rather than the narrowest of the three fixes. Refs #3910 --------- Co-authored-by: sim <sim@local>
128 lines
7.1 KiB
Markdown
128 lines
7.1 KiB
Markdown
# How to resolve a raw-terminator finding
|
|
|
|
`npm run lint` (or `npx eslint .`) reported `local/require-registered-exit`. That rule bans a
|
|
bare `process.exit(...)` call everywhere it matters — the seam it protects is the whole point of
|
|
[ADR-3889](../adr/3889-process-exit-contract.md): every process termination is projected through
|
|
one of two registered terminators, so "nothing fails with success" cannot reopen through a new
|
|
call site that bypasses them.
|
|
|
|
This page covers what the rule reports, which of the three replacements applies to your surface,
|
|
the two existing allowlist entries and why a third should not be added casually, and the
|
|
documented evasions the rule cannot catch today.
|
|
|
|
## Read a finding
|
|
|
|
```
|
|
error 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. local/require-registered-exit
|
|
```
|
|
|
|
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 flag `process.exitCode = N`: that is an assignment, never a `CallExpression`, and is
|
|
the correct pattern (see below).
|
|
|
|
## Pick the right replacement
|
|
|
|
Three outcomes exist. Pick by asking what the surrounding code actually needs, not by pattern-
|
|
matching on which file you happen to be in.
|
|
|
|
### 1. A CLI entrypoint — throw `ExitError`, let `runMain` project the code
|
|
|
|
If the call is deep inside a command's execution path and needs to unwind the stack and stop,
|
|
throw instead of exiting directly:
|
|
|
|
```js
|
|
throw new ExitError(1, 'a short reason');
|
|
```
|
|
|
|
`runMain` (`src/cli-exit.cts`) is the single place that catches an `ExitError` and turns it into
|
|
the process's actual exit code — it wraps every CLI entrypoint, so the throw always has somewhere
|
|
to land. This is exactly the migration `src/io.cts`'s `error()` went through: it used to call
|
|
`process.exit(1)` directly (uncatchable, stderr output unchanged), and now throws `ExitError(1)`
|
|
instead — stderr is byte-identical, only the control-flow shape changed.
|
|
|
|
**A throw only reaches `runMain` if nothing between the throw site and `runMain` swallows it.**
|
|
This branch's own migration needed three interceptor fixes for exactly that reason:
|
|
`command-routing-hub`'s `dispatch()` now rethrows an `ExitError` instead of catching it as a
|
|
generic failure, and the profile-pipeline router's detached `.catch()` no longer calls `error()`
|
|
(which would throw again from inside a `.catch()`, going nowhere) — it writes stderr and sets
|
|
`exitCode` in place instead. If you introduce a new `try`/`catch` or `.catch()` between your throw
|
|
site and `runMain`, check that it rethrows `ExitError` rather than absorbing it.
|
|
|
|
### 2. A hook that must write-then-terminate immediately — `terminateNow`
|
|
|
|
Enforcement hooks (`hooks/**/*.js`) run once per invocation and need to write their JSON response
|
|
and stop in the same breath — there is no `runMain` wrapper to unwind into. Use `terminateNow`
|
|
from `src/cli-exit.cts` (or its generated hook-side copy, `hooks/lib/cli-exit.js`). It is the
|
|
**only** sanctioned direct terminator in the codebase, and the only place exit code 2 (the
|
|
hook-protocol deny) may be produced (ADR-3889 §3).
|
|
|
|
If you are declaring a hook's fail-open/fail-closed policy rather than calling `terminateNow`
|
|
directly, see [Declare a hook's crash policy](declare-a-hook-crash-policy.md) — `allow()`/
|
|
`deny()`/`crash()` in `hooks/lib/hook-exit.js` are the higher-level vocabulary built on top of
|
|
this seam.
|
|
|
|
### 3. Nothing needs an immediate hard exit — `process.exitCode`
|
|
|
|
If the process should simply drain and exit non-zero once its event loop empties — no forced
|
|
unwind, no immediate write-then-die — set `process.exitCode` and return normally:
|
|
|
|
```js
|
|
process.exitCode = 1;
|
|
return;
|
|
```
|
|
|
|
This is never flagged: it is an assignment (`MemberExpression` target), never a `CallExpression`,
|
|
so the rule's `CallExpression`-only selector excludes it structurally. It is also the pattern
|
|
`runMain` itself uses once an `ExitError` is caught — projecting the code onto `process.exitCode`
|
|
rather than calling `process.exit()` a second time.
|
|
|
|
## The two allowlist entries — and why a third needs a real reason
|
|
|
|
Exactly two call sites in the repo are exempt, both for structural reasons, not convenience:
|
|
|
|
1. **`terminateNow`'s own body**, in `src/cli-exit.cts` — detected structurally: any
|
|
`process.exit()` call lexically nested inside a function declaration or expression named
|
|
`terminateNow`, *and* the file's basename is `cli-exit.cts`. The basename check matters on its
|
|
own: without it, any function named `terminateNow` anywhere in the repo would silently inherit
|
|
the allowlist.
|
|
2. **`gsd-core/bin/gsd-tools.cjs`'s `ensureRuntimeBuild` bootstrap-failure path**, via an inline
|
|
`// eslint-disable-next-line local/require-registered-exit` carrying a reason. This one call
|
|
runs *before* `./lib/cli-exit.cjs` is even required — the registered-exit seam has not been
|
|
loaded yet at that point in the process's lifetime, so there is nothing to route through.
|
|
|
|
Do not add a third entry to make an inconvenient finding go away. If you believe you have a
|
|
genuine third case — code that runs before any exit seam is loadable, the same way
|
|
`ensureRuntimeBuild` does — that is a structural claim about your file's position in the process
|
|
lifecycle, not a style preference, and it belongs in chat as a blocking question before you land
|
|
an `eslint-disable` comment.
|
|
|
|
## Known evasions — using one is a review finding, not a fix
|
|
|
|
The rule matches the literal `process.exit(...)` shape with no scope or flow analysis, so three
|
|
shapes are not caught today (and are pinned by tests in `tests/eslint-rules.test.cjs` so a future
|
|
widening is a visible decision, not a silent one):
|
|
|
|
- `process['exit'](0)` — computed member access defeats the property-name check.
|
|
- `const e = process.exit; e(1);` — aliasing the function to a local binding before calling it;
|
|
by the call site, the callee is a plain identifier, not a `MemberExpression` on `process`.
|
|
- `process.exit.call(null, 1)` / `process.exit.apply(null, [1])` — the outer call's callee is
|
|
`process.exit.call`, not `process.exit` itself.
|
|
|
|
These are documented gaps, not sanctioned escape hatches. Writing code in one of these shapes to
|
|
route around the rule reopens exactly the defect class ADR-3889 exists to close, and is a review
|
|
finding — fix the underlying call the same way any other raw terminator would be fixed, do not
|
|
rely on the lint being unable to see it.
|
|
|
|
## Related
|
|
|
|
- [ADR-3889](../adr/3889-process-exit-contract.md) — the exit-code registry and the terminator
|
|
contract this rule enforces
|
|
- [Declare a hook's crash policy](declare-a-hook-crash-policy.md) — the higher-level
|
|
`allow()`/`deny()`/`crash()` vocabulary hooks use on top of `terminateNow`
|
|
- [Resolve ESLint coverage findings](resolve-eslint-coverage-findings.md) — sibling "the lint gate
|
|
surfaced something, here is what to do with it" page
|