Tighten `require-spawnsync-error-check` for destructuring and guard semantics by Copilot · Pull Request #45419 · github/gh-aw

Copilot · 2026-07-14T09:24:03Z

require-spawnsync-error-check previously missed spawnSync destructuring patterns and treated any .error read as compliant, which allowed non-handling usages (e.g. logging only) to bypass the rule. This update expands coverage for destructured bindings and enforces that .error participates in actual guard/handling control flow.

Rule coverage: destructured spawnSync bindings
- Add analysis for object-pattern declarators initialized from spawnSync(...).
- Report when error is not bound (e.g. { status }).
- Accept destructuring only when bound error is used in a guarding/handling position.
Rule soundness: require guarding .error usage
- Replace “any .error read” logic with guard-aware detection.
- Treat as valid only when .error participates in control flow (condition tests, logical guards, throw/return paths), not passive reads.
Scope explicitly documented
- Clarify in rule docs that current analysis targets variable declarator initializers.
- Explicitly mark AssignmentExpression (result = spawnSync(...)) and inline chains (spawnSync(...).status) as out of scope.
Regression cases captured in rule tests
- Add cases for:
  - logging-only .error read (invalid),
  - destructured without error (invalid),
  - destructured with guarded error (valid),
  - guarded .error plus additional logging (valid).

// now invalid
const { status } = spawnSync(cmd, args);
if (status !== 0) throw new Error(`exit ${status}`);

// valid
const { status, error } = spawnSync(cmd, args);
if (error) throw error;
if (status !== 0) throw new Error(`exit ${status}`);

github-actions · 2026-07-14T09:37:08Z

Hey @Copilot 👋 — thanks for picking up the require-spawnsync-error-check coverage gaps! Fixing the two weaknesses (destructured result escape + non-guarding .error reads) is a well-scoped and clearly on-topic improvement to the eslint-factory linter.

A couple of things to address before this is ready for review:

No changes yet — the PR currently has 0 additions and 0 deletions. Once the implementation lands, reviewers will be able to dig in.
Add tests — the eslint-factory rules rely on test fixtures/cases to verify both valid and invalid patterns. Make sure the new logic for destructured-result tracking and non-guarding .error reads is covered by rule test cases (valid + invalid examples).

Once the diff is in, here's a prompt you can assign to your coding agent to ensure coverage:

In the eslint-factory require-spawnsync-error-check rule, add test cases for the two new scenarios:
1. Destructured/assigned result that escapes the check (e.g. `const { error } = spawnSync(...); use(error)` without guarding) — should be flagged.
2. Any `.error` read that is not used as a guard (e.g. `console.log(result.error)`) — should be flagged.
3. Counterpart valid cases where `.error` is properly guarded — should pass.
Ensure both the rule implementation and test cases are committed.

Generated by ✅ Contribution Check · 112.9 AIC · ⌖ 10.7 AIC · ⊞ 6.2K · ◷

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot

Pull request overview

Tightens spawnSync linting by recognizing destructured error bindings and requiring guard-aware error usage.

Changes:

Adds destructured binding and scope analysis.
Distinguishes guarding error checks from passive reads.
Adds regression tests and documents unsupported forms.

Show a summary per file

File	Description
`eslint-factory/src/rules/require-spawnsync-error-check.ts`	Implements guard and destructuring analysis.
`eslint-factory/src/rules/require-spawnsync-error-check.test.ts`	Adds valid and invalid regression cases.

Review details

Tip

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

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

+      continue;
+    }
+
+    break;


+    if (parent.type === AST_NODE_TYPES.LogicalExpression && ((parent.operator === "&&" && (parent.left === current || parent.right === current)) || (parent.operator === "||" && parent.left === current))) {
+      return true;


+    if (property.key.type !== AST_NODE_TYPES.Identifier || property.key.name !== "error") {
+      continue;


+    if (parent.type === AST_NODE_TYPES.LogicalExpression && ((parent.operator === "&&" && (parent.left === current || parent.right === current)) || (parent.operator === "||" && parent.left === current))) {
+      return true;


github-actions · 2026-07-14T10:17:48Z

✅ Test Quality Sentinel completed test quality analysis.

No test files detected in pre-fetched data (test-diff.txt empty). Test Quality Sentinel analysis skipped per workflow Step 1.

github-actions · 2026-07-14T10:17:51Z

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

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

github-actions · 2026-07-14T10:17:58Z

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

github-actions · 2026-07-14T10:18:27Z

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

github-actions

Review: `require-spawnsync-error-check` tightening

The destructuring coverage and guard-aware detection are solid improvements. One blocking correctness bug found.

Blocking: && right-hand operand is not a guarding position (see inline comment on line 33). condition && result.error is a passive read that would incorrectly pass the rule. Both && and || should only treat parent.left === current as guarding.

Non-blocking: No test for renamed destructuring ({ error: err }) which getErrorBindingNames explicitly supports — worth adding a valid-case test to prevent regression.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 32.1 AIC · ⌖ 5.5 AIC · ⊞ 4.8K

github-actions · 2026-07-14T10:24:55Z

+      return true;
+    }
+
+    if (parent.type === AST_NODE_TYPES.LogicalExpression && ((parent.operator === "&&" && (parent.left === current || parent.right === current)) || (parent.operator === "||" && parent.left === current))) {


Bug: && right-hand operand is incorrectly treated as a guarding position.

In condition && result.error, the result.error on the right is a passive read — it only executes when condition is already truthy. It does not guard against the error. Only the left operand of && establishes a guard (e.g. result.error && doSomething()).

This allows the following to pass the rule when it should be flagged:

const result = spawnSync(cmd, args); if (result.status !== 0 && result.error) log(result.error); // right side — not a guard

Fix: change (parent.left === current || parent.right === current) to just parent.left === current:

// Both && and || only guard on the left operand if (parent.type === AST_NODE_TYPES.LogicalExpression && parent.left === current) { return true; }

@copilot please address this.

github-actions

Request changes: four correctness gaps in guard-walk logic that cause false negatives (valid guards rejected) and false positives (non-guards accepted).

The existing comments detail each issue; see inline threads for specifics. Summary:

Comparison-based guards rejected — if (result.error !== undefined) is not recognized because BinaryExpression parents are not traversed.
|| right operand prematurely rejected — if (status !== 0 || result.error) reports false positives.
String-literal destructuring keys not recognized — const { "error": spawnError } = spawnSync(...) is reported as missing a check.
Logical-expression operands accepted unconditionally — passive reads like result.error && result.error.message satisfy the rule without any guard context.

All four affect the correctness of the lint rule's intent and need to be fixed before merging.

🔎 Code quality review by PR Code Quality Reviewer · 88.8 AIC · ⌖ 4.33 AIC · ⊞ 5.4K
_{Comment /review to run again}

github-actions

Skills-Based Review 🧠

Applied /diagnosing-bugs and /tdd — requesting changes on one correctness issue and two clarity gaps.

📋 Key Themes & Highlights

Key Issues

&& right-operand false negative (line 33, isGuardingErrorUsage): treating the right side of && as a guard means cond && result.error (standalone expression) would pass the check even though result.error is never acted on. Only the left side of && should be a guard terminus; the right side should continue walking up.
Redundant type guard (line 155): ref.identifier.type === AST_NODE_TYPES.Identifier is always true for scope references — remove it for clarity.
Test coverage gap (test line 27): the case of logging before a guard is not in the valid suite, leaving an implicit gap in the spec.

Positive Highlights

✅ Guard-aware detection (isGuardingErrorUsage) is a solid improvement over the previous "any .error read" heuristic.
✅ Destructuring path (getErrorBindingNames) handles both { error } and { error = default } cleanly.
✅ Scope is explicitly documented in the rule description — great for future contributors.
✅ New regression tests directly mirror the PR description examples.

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

github-actions · 2026-07-14T10:27:13Z

+      return true;
+    }
+
+    if (parent.type === AST_NODE_TYPES.LogicalExpression && ((parent.operator === "&&" && (parent.left === current || parent.right === current)) || (parent.operator === "||" && parent.left === current))) {


[/diagnosing-bugs] The && right-operand branch is a false negative: someCondition && result.error would satisfy the guard check even though result.error is only read when someCondition is truthy — no throw/return occurs.

💡 Details and suggested fix

The current check:

parent.operator === "&&" && (parent.left === current || parent.right === current)

treats the right side of && as a guard. For handleError() && result.error (standalone expression), isGuardingErrorUsage returns true even though nothing acts on the error.

Restricting && to the left operand (like || does) or continuing to walk up to the enclosing conditional removes this false negative:

parent.operator === "&&" && parent.left === current // or parent.operator === "||" && parent.left === current

The right side of && is already guarded by the enclosing if/while parent via the loop — no early return is needed for it.

@copilot please address this.

github-actions · 2026-07-14T10:27:14Z

+      return true;
+    }
+
+    if (parent.type === AST_NODE_TYPES.LogicalExpression && ((parent.operator === "&&" && (parent.left === current || parent.right === current)) || (parent.operator === "||" && parent.left === current))) {


[/tdd] Missing test: ?? (nullish coalescing) is not covered — result.error ?? fallback would not satisfy isGuardingErrorUsage since NullishCoalescing is absent from the operator checks, but there is no test case documenting this as intentionally out-of-scope.

💡 Suggested test case or comment

Either add an invalid test case:

{ code: `const result = spawnSync("git", ["status"]); const e = result.error ?? null;`, errors: [{ messageId: "missingErrorCheck" }] }

Or add a comment in isGuardingErrorUsage explicitly noting ?? is not treated as a guard (the operator does not imply control flow diversion).

@copilot please address this.

github-actions · 2026-07-14T10:27:14Z

+          const hasGuardingErrorCheck = errorBindingNames.some(varName => {
+            const variable = findVariableByName(sourceCode, node, varName);
+            if (!variable) return false;
+            return variable.references.some(ref => ref.identifier.type === AST_NODE_TYPES.Identifier && isGuardingErrorUsage(ref.identifier));


[/diagnosing-bugs] ref.identifier.type === AST_NODE_TYPES.Identifier is always true for an ESLint scope reference — Reference.identifier is always an Identifier node. This guard is a no-op and only adds noise.

💡 Simplification

return variable.references.some(ref => isGuardingErrorUsage(ref.identifier));

@copilot please address this.

github-actions · 2026-07-14T10:32:47Z

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 100/100 — Excellent

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

📊 Metrics (1 test)

Metric	Value
Analyzed	1 (JavaScript/Vitest)
✅ Design	1 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	1 (100%)
Duplicate clusters	0
Inflation	No (0.14:1 ratio)
🚨 Violations	0

Verdict

✅ PASS. 0% implementation tests (threshold: 30%). The new test case precisely targets guard-aware semantics: verifying that logging result.error is acceptable when the variable is properly guarded. This boundary condition is important for the rule's correctness and represents high-value design coverage.

Analysis:

Test added: Line ~27 of require-spawnsync-error-check.test.ts
What it verifies: ESLint rule correctly accepts code that logs result.error after proper guarding
Design value: High — Prevents regression where guarded logging incorrectly triggers errors
Type: Positive assertion via RuleTester (code should NOT produce lint error)
Test inflation: Healthy (16 test lines / 111 production lines = 0.14:1; threshold is 2:1)

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 · 95.6 AIC · ⌖ 10.3 AIC · ⊞ 6.8K · ◷
_{Comment /review to run again}

github-actions

✅ Test Quality Sentinel: 100/100 — Excellent. 0% implementation tests (threshold: 30%). The test case verifies important guard-aware semantics and represents high-value design coverage.

pelikhan · 2026-07-14T11:04:08Z

@copilot run pr-finisher skill

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

gh-aw-bot · 2026-07-14T11:18:34Z

@copilot please run the pr-finisher skill, address the unresolved review feedback, and rerun checks. Prioritize these unresolved threads first (newest first):

github-actions — Tighten require-spawnsync-error-check for destructuring and guard semantics #45419 (comment)
Copilot — Tighten require-spawnsync-error-check for destructuring and guard semantics #45419 (comment)
Copilot — Tighten require-spawnsync-error-check for destructuring and guard semantics #45419 (comment)
Copilot — Tighten require-spawnsync-error-check for destructuring and guard semantics #45419 (comment)
Copilot — Tighten require-spawnsync-error-check for destructuring and guard semantics #45419 (comment)
The PR already has blocking change requests, so please push a follow-up commit that resolves those comments and then rerun the checks.

Generated by 👨‍🍳 PR Sous Chef · 8.61 AIC · ⌖ 8.24 AIC · ⊞ 5.1K · ◷
_{Comment /souschef to run again}

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot · 2026-07-14T11:37:53Z

@copilot run pr-finisher skill

Addressed the in-scope rule feedback in cffa6a3 and ec86d47. The guard walker now traverses comparison expressions, handles right-hand || guards without accepting passive logical reads, recognizes string-literal destructuring keys, and adds the requested regression coverage.

Initial plan

9949b4a

Copilot AI assigned Copilot and pelikhan Jul 14, 2026

Copilot started work on behalf of pelikhan July 14, 2026 09:24 View session

Copilot AI linked an issue Jul 14, 2026 that may be closed by this pull request

eslint-factory: require-spawnsync-error-check — destructured/assigned result escapes rule; any .error read (even non-guarding) s [Content truncated due to length] #45387

Closed

github-actions Bot mentioned this pull request Jul 14, 2026

[Contribution Check Report] Contribution Check — 2026-07-14 #45356

Open

Copilot AI and others added 2 commits July 14, 2026 09:42

fix eslint require-spawnsync-error-check for destructuring and guarding

54ccb81

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

refine spawnsync guard semantics and tests

ca26474

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Fix coverage gaps in require-spawnsync-error-check rule~~ Tighten require-spawnsync-error-check for destructuring and guard semantics Jul 14, 2026

Copilot finished work on behalf of pelikhan July 14, 2026 09:55

Copilot AI requested a review from pelikhan July 14, 2026 09:55

pelikhan marked this pull request as ready for review July 14, 2026 10:06

Copilot AI review requested due to automatic review settings July 14, 2026 10:06

Copilot started reviewing on behalf of pelikhan July 14, 2026 10:07 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

github-actions Bot requested changes Jul 14, 2026

View reviewed changes

github-actions Bot approved these changes Jul 14, 2026

View reviewed changes

github-actions Bot mentioned this pull request Jul 14, 2026

[aw] Test Quality Sentinel reported incomplete result #45444

Open

Copilot started work on behalf of pelikhan July 14, 2026 11:04 View session

chore: start pr finisher pass

7e09c14

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

github-actions Bot mentioned this pull request Jul 14, 2026

[pr-sous-chef] Run report — 4 nudged, 3 skipped #45345

Open

Copilot AI and others added 2 commits July 14, 2026 11:29

fix spawnSync guard-walk edge cases

cffa6a3

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

clarify spawnSync guard handling

c094827

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Copilot finished work on behalf of pelikhan July 14, 2026 11:38

Copilot started work on behalf of gh-aw-bot July 14, 2026 11:38 View session

pelikhan merged commit 8a3e458 into main Jul 14, 2026
7 of 8 checks passed

pelikhan deleted the copilot/fix-eslint-factory-error-check branch July 14, 2026 11:43

Copilot stopped work on behalf of gh-aw-bot due to an error July 14, 2026 11:43
The session was cancelled by the user.

Copilot AI requested a review from gh-aw-bot July 14, 2026 11:43

		if (parent.type === AST_NODE_TYPES.LogicalExpression && ((parent.operator === "&&" && (parent.left === current \|\| parent.right === current)) \|\| (parent.operator === "\|\|" && parent.left === current))) {
		return true;

		if (property.key.type !== AST_NODE_TYPES.Identifier \|\| property.key.name !== "error") {
		continue;

Uh oh!

Conversation

Copilot AI commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

github-actions Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review: require-spawnsync-error-check tightening

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Skills-Based Review 🧠

Key Issues

Positive Highlights

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 14, 2026

🧪 Test Quality Sentinel Report

Verdict

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pelikhan commented Jul 14, 2026

Uh oh!

gh-aw-bot commented Jul 14, 2026

Uh oh!

Copilot AI commented Jul 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Copilot AI commented Jul 14, 2026 •

edited

Loading

github-actions Bot commented Jul 14, 2026 •

edited

Loading

github-actions Bot commented Jul 14, 2026 •

edited

Loading

github-actions Bot commented Jul 14, 2026 •

edited

Loading

github-actions Bot commented Jul 14, 2026 •

edited

Loading

Review: `require-spawnsync-error-check` tightening