[eslint-miner] feat(eslint): add require-spawnsync-error-check rule

[github-actions]

Summary

Adds a new ESLint rule require-spawnsync-error-check to the gh-aw-custom plugin. The rule enforces that callers of spawnSync (bare or namespaced via childProcess / child_process) check the .error property on the result, not only .status.

Motivation: When spawnSync cannot spawn the child process (e.g. ENOENT, ETIMEDOUT), result.status is null and result.error holds the actual Error. Checking only result.status silently swallows spawn-level failures or emits a misleading "exit null" diagnostic.

Changes

File	Change
eslint-factory/src/rules/require-spawnsync-error-check.ts	New rule implementation (103 lines)
eslint-factory/src/rules/require-spawnsync-error-check.test.ts	Vitest + RuleTester tests (64 lines)
eslint-factory/src/index.ts	Import and register new rule in plugin
eslint-factory/eslint.config.cjs	Enable rule at warn severity

Rule behaviour

• Matched call forms: spawnSync(...), childProcess.spawnSync(...), child_process.spawnSync(...)
• Trigger: result variable assigned from a matched call that never has .error read anywhere in scope
• Message ID: missingErrorCheck
• Config severity: warn
• Schema options: none

Valid (no warning):

const result = spawnSync("git", ["status"]);
if (result.error) throw result.error;
if (result.status !== 0) throw new Error("failed");

Invalid (warning emitted):

const result = spawnSync("git", ["status"]);
if (result.status !== 0) throw new Error("failed"); // result.error never read

Test coverage

• Correct docs URL assertion
• Valid: bare spawnSync, namespaced variants, indirect property read via intermediate variable
• Invalid: status-only check, stdout access without error check, namespaced variants without error check
• Non-spawnSync calls (execSync, bare identifier reference) are not flagged

Impact

Purely additive. No existing rules modified. No breaking changes.

Generated by PR Description Updater for #45246 · 34.6 AIC · ⌖ 4.53 AIC · ⊞ 4.7K · ◷

[github-actions]

⚠️ PR Code Quality Reviewer failed to deliver outputs during code quality review.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

No ADR enforcement needed: PR #45246 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[Copilot]

Pull request overview

Adds an ESLint rule detecting unchecked spawnSync() errors in setup scripts.

Changes:

• Implements and tests the new rule.
• Registers and enables it as a warning.

Show a summary per file

File	Description
eslint-factory/src/rules/require-spawnsync-error-check.ts	Implements detection logic.
eslint-factory/src/rules/require-spawnsync-error-check.test.ts	Adds rule tests.
eslint-factory/src/index.ts	Registers the rule.
eslint-factory/eslint.config.cjs	Enables the rule.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Medium

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 82/100 — Excellent

Analyzed 4 test(s): 3 design, 1 implementation, 0 violation(s).

📊 Metrics (4 tests)

Metric	Value
Analyzed	4 (Go: 0, JS: 4)
✅ Design	3 (75%)
⚠️ Implementation	1 (25%)
Edge/error coverage	3 (75%)
Duplicate clusters	0
Inflation	NO (0.62:1)
🚨 Violations	0

Test	File	Classification	Issues
"uses the correct docs URL"	require-spawnsync-error-check.test.ts:13	implementation	Metadata test; low value but acceptable
"valid: result.error is checked alongside result.status"	require-spawnsync-error-check.test.ts:19	design	✅ 4 valid scenarios with comprehensive coverage
"invalid: only result.status checked, result.error never read"	require-spawnsync-error-check.test.ts:35	design	✅ 3 invalid cases with error assertions
"valid: non-spawnSync call is ignored"	require-spawnsync-error-check.test.ts:55	design	✅ Edge case: correctly ignores non-targets

Verdict

✅ Passed. 25% implementation tests (threshold: 30%). No coding violations. The test suite demonstrates strong behavioral contract coverage with 3 of 4 tests validating core rule behavior: proper flagging of violations, acceptance of valid code, and edge case handling. The single metadata test is low-value but acceptable for an ESLint rule. Test inflation ratio (0.62:1) is healthy.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 15.5 AIC · ⌖ 9.98 AIC · ⊞ 6.8K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 82/100. 25% implementation tests (threshold: 30%). No violations. Strong behavioral coverage with 3 of 4 tests validating core rule behavior.

[github-actions]

Review: require-spawnsync-error-check ESLint rule

The rule is well-motivated, correctly scoped, and the implementation is clean. Tests cover the main valid/invalid patterns and I confirmed zero false positives on existing compliant files.

One non-blocking gap flagged inline: the .error detection only covers member-expression access (result.error) and misses the destructuring pattern (const { error } = result), which would produce a false positive. See inline comment for the fix approach.

Everything else (scope walking, namespace matching, message clarity, test coverage) looks good.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 19.8 AIC · ⌖ 5.03 AIC · ⊞ 4.8K

[github-actions]

The .error check only detects result.error member-expression accesses, but misses the destructuring pattern:

const result = spawnSync("git", ["status"]);
const { error } = result;   // result.error IS read — but the rule flags this as a violation
if (error) throw error;

This produces a false positive. Consider also detecting VariableDeclarator nodes whose id is an ObjectPattern that destructures error from the spawnSync result variable, and marking those as satisfying the check.

@copilot please address this.

[github-actions]

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 34.8 AIC · ⌖ 4.6 AIC · ⊞ 6.6K
_{Comment /matt to run again}

[github-actions]

[/tdd] False positive risk: destructuring const { error } = result is a valid error-check pattern but won't be detected here, causing a spurious warning on correct code.

💡 Suggested fix

The current check only recognises result.error MemberExpression access. Add a second branch that detects references where the parent is an ObjectPattern destructuring the result variable:

// also match: const { error } = result
const hasDestructureErrorCheck = variable.references.some(ref => {
  const id = ref.identifier;
  const parent = id.parent;
  return (
    parent !== undefined &&
    parent.type === AST_NODE_TYPES.VariableDeclarator &&
    parent.init === id &&
    parent.id.type === AST_NODE_TYPES.ObjectPattern &&
    parent.id.properties.some(
      p =>
        p.type === AST_NODE_TYPES.Property &&
        p.key.type === AST_NODE_TYPES.Identifier &&
        p.key.name === "error",
    )
  );
});

Combine with hasErrorCheck using ||.

Add a corresponding test case:

`const result = spawnSync("git", ["status"]); const { error } = result; if (error) throw error;`

@copilot please address this.

[github-actions]

[/tdd] The rule only tracks simple const result = spawnSync(...) bindings. Re-assignment (result = spawnSync(...)) or let result; result = spawnSync(...) patterns are silently ignored, missing real violations.

💡 Suggested fix or test

Either document the limitation in the rule description/schema, or add an AssignmentExpression visitor. At minimum, add a test asserting the known gap so future contributors are aware:

// Known gap – no error reported for assignment (non-declarator) pattern
`let result; result = spawnSync("git", ["status"]); if (result.status !== 0) throw new Error("failed");`

@copilot please address this.

[github-actions]

[/grill-with-docs] CHILD_PROCESS_OBJECTS is a hard-coded set of aliases. Dynamic imports (const cp = require('child_process') with any alias) are not detected, which is a gap worth documenting.

💡 Suggestion

Add a comment and/or a doc note in the rule's description clarifying that the rule only covers the two well-known aliases (childProcess, child_process) and bare spawnSync calls — not arbitrary require-assigned aliases. This sets correct expectations and prevents confusion when new aliases appear.

@copilot please address this.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.9