Summary
Two related weaknesses in require-spawnsync-error-check let real spawn-failure gaps slip through.
File: eslint-factory/src/rules/require-spawnsync-error-check.ts
1. Coverage FN — only const <id> = spawnSync(...) is analyzed
The rule bails unless the binding is a plain identifier (L68: if (node.id.type !== AST_NODE_TYPES.Identifier) return). These idiomatic shapes escape entirely:
const { status, stdout } = spawnSync(cmd, args); // no `error` bound -> cannot check it -> rule never fires
let result; result = spawnSync(cmd, args); // AssignmentExpression, not a VariableDeclarator init
if (spawnSync(cmd, args).status !== 0) { ... } // no binding at allThe first case is the most dangerous: destructuring only status means .error is unbindable, which is exactly the failure mode the rule targets (spawn error -> status === null), yet the rule stays silent.
Grounding: every live site today uses const result = spawnSync(...) — git_helpers.cjs:87 guards if (result.error) (true negative ✓); apply_samples.cjs:97, artifact_client.cjs:163/170/183/316, and send_otlp_span.cjs:823 check only .status/.signal (correctly flagged true positives). The destructuring FN is therefore latent, but it is a common refactor that would silently disable the check.
2. Soundness — any .error read satisfies the rule, even a non-guarding one
hasErrorCheck (L84-88) treats any reference that reads .error as satisfying the rule. A read that does not act on the failure still silences it:
const result = spawnSync(cmd, args);
core.warning(`spawn stderr: ${result.error}`); // logs error but does NOT handle it
if (result.status !== 0) throw new Error(`exit ${result.status}`); // still reports misleading "exit null"The rule's own remediation text implies a guard (if (result.error) { throw result.error; }), but the check accepts a bare logging read.
Acceptance criteria
- Destructured result bindings are analyzed:
const { status } = spawnSync(...) with no error property and no other .error handling is reported; const { status, error } = spawnSync(...) that guards on error is valid.
- Decide and document scope for
result = spawnSync() (assignment) and inline spawnSync().status: either handle them or add an explicit out-of-scope note in the rule doc comment — no silent gaps.
- Tighten
hasErrorCheck so the .error read must occur in a guarding position (an if/ternary test, a &&/|| operand, or a throw/return driven by it) rather than any read. A bare core.info(result.error) with no guard should still report.
- Regression: preserve existing true positives (
apply_samples.cjs:97, artifact_client.cjs, send_otlp_span.cjs:823) and the true negative git_helpers.cjs:87 (if (result.error) { ... throw }). Add unit tests for: guarded if (result.error) throw (valid), logging-only read (invalid), destructured-without-error (invalid), destructured-with-error-guard (valid).
Generated by 🤖 ESLint Refiner · 360.4 AIC · ⌖ 12.5 AIC · ⊞ 4.6K · ◷
Summary
Two related weaknesses in
require-spawnsync-error-checklet real spawn-failure gaps slip through.File:
eslint-factory/src/rules/require-spawnsync-error-check.ts1. Coverage FN — only
const <id> = spawnSync(...)is analyzedThe rule bails unless the binding is a plain identifier (L68:
if (node.id.type !== AST_NODE_TYPES.Identifier) return). These idiomatic shapes escape entirely:The first case is the most dangerous: destructuring only
statusmeans.erroris unbindable, which is exactly the failure mode the rule targets (spawn error ->status === null), yet the rule stays silent.Grounding: every live site today uses
const result = spawnSync(...)—git_helpers.cjs:87guardsif (result.error)(true negative ✓);apply_samples.cjs:97,artifact_client.cjs:163/170/183/316, andsend_otlp_span.cjs:823check only.status/.signal(correctly flagged true positives). The destructuring FN is therefore latent, but it is a common refactor that would silently disable the check.2. Soundness — any
.errorread satisfies the rule, even a non-guarding onehasErrorCheck(L84-88) treats any reference that reads.erroras satisfying the rule. A read that does not act on the failure still silences it:The rule's own remediation text implies a guard (
if (result.error) { throw result.error; }), but the check accepts a bare logging read.Acceptance criteria
const { status } = spawnSync(...)with noerrorproperty and no other.errorhandling is reported;const { status, error } = spawnSync(...)that guards onerroris valid.result = spawnSync()(assignment) and inlinespawnSync().status: either handle them or add an explicit out-of-scope note in the rule doc comment — no silent gaps.hasErrorCheckso the.errorread must occur in a guarding position (anif/ternary test, a&&/||operand, or a throw/return driven by it) rather than any read. A barecore.info(result.error)with no guard should still report.apply_samples.cjs:97,artifact_client.cjs,send_otlp_span.cjs:823) and the true negativegit_helpers.cjs:87(if (result.error) { ... throw }). Add unit tests for: guardedif (result.error) throw(valid), logging-only read (invalid), destructured-without-error (invalid), destructured-with-error-guard (valid).