test(#3143): add the no-unbounded-spawn guard and throw-preserving git fixture (#3150)

* test(#3143): add no-unbounded-spawn guard and throw-preserving git fixture

Adds the ESLint rule local/no-unbounded-spawn, wired into the tests/**/*.cjs
block, plus an allowlist that only ratchets down: a listed file with zero
violations reports its own entry as stale.

The rule resolves renamed destructures and chained requires rather than
matching literal callee names -- both forms exist in the suite today and a
name-only matcher leaves them permanently invisible. It resolves an options
object held in a single-write const, which is what keeps process-seam.cjs,
the bounded reference implementation, from flagging itself.

timeout: 0 and anything above the 600000ms ceiling are rejected as only
nominally bounded.

Adds tests/helpers/git-fixture.cjs so a migrated execSync call site keeps
its throw-on-non-zero contract; process-seam.cjs is unchanged.

* test(#3143): prove the allowlist guards can actually fail

Extracts the D4/D6/D7/D8 checks into pure helpers and drives each against a
synthetic fixture carrying an injected violation. Without this the suite only
proved that today's clean data passes, which a deleted check would also
satisfy.

* fix(#3143): close two ceiling and alias escapes found in review

Nested arithmetic bypassed the ceiling entirely: the numeric evaluator only
resolved a flat literal, so `timeout: 60 * 60 * 1000` (3600000ms, six times
the ceiling) fell through to trusted and reported nothing. The evaluator now
recurses through arithmetic and unary signs with a depth cap.

Alias resolution was traversal-order dependent, not scope dependent: a call
textually above its own require destructure saw an empty alias map and
reported clean. The map is now built in a Program pre-pass.

Also: an explicit timeoutMs:undefined no longer overwrites the git fixture
default via spread, adds the missing seam-routed rule test, and de-duplicates
the repeated try/catch in the fixture tests.

---------

Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
2026-08-07 09:43:36 -04:00
committed by GitHub
parent 4b66bf4560
commit 2afe17bbdb
9 changed files with 1502 additions and 0 deletions

View File

@@ -417,6 +417,57 @@ bench OOM. Inject faults in-process through a module's `deps` parameter instead.
Per-suite wrappers are still expected and encouraged: bind your fixture (cwd, env, payload) in a
local helper and delegate the spawn to the seam.
#### When you want git to *throw*: `gitOrThrow`
`runGit` never throws — that is the whole point of it. But `execSync` and `execFileSync` **do**
throw on a non-zero exit, and a lot of fixture setup relies on that: `git commit` failing should
stop the test right there, not hand back an empty string that produces a baffling assertion failure
twenty lines later.
For that case use `tests/helpers/git-fixture.cjs`:
```javascript
const { gitOrThrow } = require('./helpers/git-fixture.cjs');
gitOrThrow(['init', '-b', 'main'], { cwd: dir });
gitOrThrow(['commit', '-m', 'seed'], { cwd: dir }); // throws if git exits non-zero
const branch = gitOrThrow(['rev-parse', '--abbrev-ref', 'HEAD'], { cwd: dir }).trim();
```
It returns `stdout` as a **string** on success. On any non-`EXITED` outcome, or a non-zero exit, it
throws an `Error` carrying `status`, `exitCode`, `stdout`, `stderr`, `signal`, `timedOut` and
`outcome` as own properties. `status` and `exitCode` are deliberate aliases: `status` is what the
legacy `execSync` idiom reads (`catch (err) { assert.equal(err.status, 1) }`), so a migrated call
site keeps working.
| You want | Use |
|---|---|
| Every outcome as data; you branch on `outcome` | `runGit` |
| Fixture setup that must abort loudly on failure | `gitOrThrow` |
`process-seam.cjs` itself is untouched by this — it still never throws.
#### The lint rule that enforces it
`local/no-unbounded-spawn` (`eslint-rules/no-unbounded-spawn.cjs`) fails any `spawnSync`,
`execFileSync` or `execSync` under `tests/` that is not timeout-bounded. It resolves renamed
destructures (`const { execSync: exec } = require('node:child_process')`) and chained requires
(`require('node:child_process').execSync(...)`), so renaming your way around it does not work.
Two things it deliberately rejects, because both look bounded and are not:
- `timeout: 0` — Node reads zero as *no timeout*.
- `timeout: 999999999` — anything above the 600000 ms ceiling is effectively unbounded. Size the
number to what the command actually runs and say why in a comment.
A non-literal value (`timeout: GIT_TIMEOUT_MS`) is trusted — that is the shape you should be
writing.
`eslint-rules/no-unbounded-spawn.allowlist.json` grandfathers files that predate the rule. It only
ratchets **down**: once a file is clean, the rule reports its allowlist line as stale and you delete
it. Never add an entry, and never reach for `eslint-disable` on this rule — a test asserts that no
such comment exists.
### Test Structure
```javascript