[eslint-miner] eslint: add require-execsync-try-catch rule by github-actions[bot] · Pull Request #46789 · github/gh-aw

github-actions · 2026-07-20T09:54:40Z

Summary

Adds a new ESLint rule gh-aw-custom/require-execsync-try-catch to the eslint-factory plugin. The rule flags any call to execSync from child_process (or node:child_process) that is not wrapped in a try/catch block. An unhandled execSync throw crashes the action without surfacing a useful diagnostic.

Changes

File	Type	Description
`eslint-factory/src/rules/require-execsync-try-catch.ts`	added	Rule implementation using scope-walking to resolve `execSync` bindings across CJS destructured, CJS namespace, ESM named imports, and aliased forms
`eslint-factory/src/rules/require-execsync-try-catch.test.ts`	added	Vitest/ESLint `RuleTester` suite covering valid and invalid patterns for all import styles, aliases, and non-`child_process` sources
`eslint-factory/src/index.ts`	modified	Imports and registers the new rule in the plugin
`eslint-factory/eslint.config.cjs`	modified	Enables `gh-aw-custom/require-execsync-try-catch` at `warn` severity

Rule behaviour

Reports any execSync(...) call from child_process/node:child_process not enclosed in a try block.
Ignores calls where the binding resolves to a different module, or where no binding can be resolved (bare identifiers, mock objects).

Suggests an auto-fix that wraps the enclosing statement in:

try {
  execSync(...);
} catch (err) {
  // TODO: handle execSync failure (non-zero exit / signal termination).
  throw new Error("execSync failed: " + (err instanceof Error ? err.message : String(err)), { cause: err });
}

Impact

Non-breaking. Severity is warn; existing code is not blocked.
Scope resolution handles destructuring, namespace imports, aliased bindings, and node: protocol specifiers.

Generated by PR Description Updater for #46789 · 24.4 AIC · ⌖ 4.51 AIC · ⊞ 4.8K · ◷

execSync from child_process throws a ChildProcessError when the child process exits non-zero or is killed by a signal. An unhandled throw crashes the action step with no useful diagnostic. Evidence in actions/setup/js: - start_mcp_gateway.cjs:824 — bare execSync(`node "${converterPath}"`) without a surrounding try/catch (confirmed by lint:setup-js producing one new warning for this exact line) The rule: - Detects execSync sourced from child_process / node:child_process via destructured require, namespace require, or ESM import - Skips calls already inside a try block - Offers a hasSuggestions fixer to wrap the statement in try/catch with { cause: err } re-throw - Zero false positives: execSync from unrelated modules and bare (unresolved) identifiers are ignored Tests: 8 new unit tests (valid + invalid cases); all 306 tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-07-20T09:57:22Z

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

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

github-actions · 2026-07-20T09:57:27Z

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

github-actions · 2026-07-20T09:57:30Z

✅ Test Quality Sentinel completed test quality analysis.

github-actions · 2026-07-20T09:57:37Z

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

Copilot

Pull request overview

Adds an ESLint rule requiring child_process.execSync calls to be protected by try/catch.

Changes:

Implements binding-aware detection and remediation suggestions.
Adds RuleTester coverage.
Registers and enables the rule.

Show a summary per file

File	Description
`eslint-factory/src/rules/require-execsync-try-catch.ts`	Implements the rule.
`eslint-factory/src/rules/require-execsync-try-catch.test.ts`	Tests supported call patterns.
`eslint-factory/src/index.ts`	Exports the rule.
`eslint-factory/eslint.config.cjs`	Enables the rule as a warning.

Review details

Tip

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

Comments suppressed due to low confidence (1)

eslint-factory/src/rules/require-execsync-try-catch.ts:120

String-literal member access such as cp["execSync"](...) is semantically the same namespace call, but !callee.computed makes it invisible to this rule. The shared fs resolver already handles this shape in try-catch-rule-utils.ts:138-145; resolve both identifier and computed string-literal properties here and cover the case in tests.

    callee.type === AST_NODE_TYPES.MemberExpression &&
    !callee.computed &&
    callee.object.type === AST_NODE_TYPES.Identifier &&
    callee.property.type === AST_NODE_TYPES.Identifier &&
    callee.property.name === "execSync"

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

+        if (isChildProcessImportBinding(def) && def.node.type === AST_NODE_TYPES.ImportNamespaceSpecifier) {
+          return true;


+        // Still flag it even in deferred callbacks — execSync in async callbacks is still risky.
+        void withinDeferredBoundary;


+    docs: {
+      description:
+        "Require execSync calls in actions/setup/js scripts to be wrapped in try/catch. " +
+        "execSync throws a ChildProcessError when the child process exits with a non-zero status code or is killed by a signal; " +


github-actions

Review: require-execsync-try-catch

The rule logic, scope analysis, and test coverage are solid. One blocking issue:

Dead code must be removed — the withinDeferredBoundary ancestor-walk in CallExpression is computed then immediately discarded (void withinDeferredBoundary). Per the inline comment this is intentional ("Still flag it even in deferred callbacks"), making the entire block a no-op that burns CPU on every flagged call site and leaves misleading code behind. Remove it before merging.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 22 AIC · ⌖ 4.46 AIC · ⊞ 5K

github-actions · 2026-07-20T10:02:08Z

+        const stmt = findEnclosingStatement(sourceCode, node);
+
+        context.report({
+          node,


Dead code: withinDeferredBoundary is computed but never used.

The block below computes withinDeferredBoundary by walking all ancestors, then immediately discards the result via void withinDeferredBoundary. The accompanying comment says "Still flag it even in deferred callbacks", which means this whole block is a no-op — it adds CPU overhead on every matched call site without affecting behaviour.

Remove the unused variable and its ancestor-walk entirely:

// DELETE all of this: const ancestors = sourceCode.getAncestors(node); let withinDeferredBoundary = false; for (let i = ancestors.length - 1; i >= 0; i--) { if (isDeferredCallback(ancestors[i])) { withinDeferredBoundary = true; break; } } void withinDeferredBoundary;

If there is future intent to skip flagging in deferred callbacks, track that as a follow-up issue rather than leaving inert code here.

@copilot please address this.

github-actions

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

github-actions · 2026-07-20T10:02:38Z

+          }
+        }
+        // Still flag it even in deferred callbacks — execSync in async callbacks is still risky.
+        void withinDeferredBoundary;


[/codebase-design] Dead code: the withinDeferredBoundary block (lines 159–168) is computed then immediately suppressed with void — it has no effect on rule behaviour.

💡 Suggestion

The inline comment says "Still flag it even in deferred callbacks", so the intent is clear, but the code is misleading noise. Either:

Remove the block entirely (preferred — the behaviour is already correct without it):

// Remove lines 159–168; isInsideTryBlock already handles the try-protection check.

Or add a single explanatory comment where isInsideTryBlock is called:

// execSync inside deferred callbacks is still flagged — the outer try does not protect it. if (isInsideTryBlock(sourceCode, node)) return;

Dead void-suppressed variables indicate unfinished refactoring and may confuse future maintainers.

@copilot please address this.

github-actions · 2026-07-20T10:02:38Z

+      ],
+    });
+  });
+});


[/tdd] Missing test: ESM namespace import (import * as cp from "child_process") is not covered — the rule handles ImportNamespaceSpecifier in isChildProcessObjectBinding but there is no test confirming it works.

💡 Suggested test case

it("invalid: cp.execSync without try/catch (ESM namespace import)", () => { esmRuleTester.run("require-execsync-try-catch", requireExecSyncTryCatchRule, { valid: [ `import * as cp from "child_process"; try { cp.execSync("ls"); } catch (e) {}`, ], invalid: [ { code: `import * as cp from "child_process"; cp.execSync("ls");`, errors: [{ messageId: "requireTryCatch" }], }, ], }); });

Without this test, a regression in the namespace import path would go undetected.

@copilot please address this.

github-actions · 2026-07-20T10:03:10Z

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 89/100 — Excellent

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

📊 Metrics (8 tests)

Metric	Value
Analyzed	8 (JS: 8)
✅ Design	8 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	5 (62.5%)
Duplicate clusters	0
Inflation	No (0.74:1)
🚨 Violations	0

Test	File	Classification	Issues
valid: execSync inside try block (CommonJS, destructured)	require-execsync-try-catch.test.ts:20–29	Design	—
valid: execSync inside try block (CommonJS, namespace)	require-execsync-try-catch.test.ts:31–36	Design	—
valid: execSync inside try block (ES module)	require-execsync-try-catch.test.ts:38–46	Design	—
valid: execSync from non-child_process module is ignored	require-execsync-try-catch.test.ts:48–60	Design (edge case)	—
invalid: execSync without try/catch (CommonJS, destructured)	require-execsync-try-catch.test.ts:62–82	Design	—
invalid: execSync without try/catch (CommonJS, namespace)	require-execsync-try-catch.test.ts:84–104	Design	—
invalid: execSync without try/catch (ES module)	require-execsync-try-catch.test.ts:106–126	Design	—
invalid: aliased execSync without try/catch is flagged	require-execsync-try-catch.test.ts:128–148	Design (edge case)	—

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). All tests verify user-visible rule behavior with comprehensive coverage of import patterns, scoping, and error detection across CommonJS and ES modules.

Strengths:

✅ All 8 tests are design-level behavioral contracts, not implementation details
✅ Critical edge cases covered: non-matching modules, aliased imports, namespace access patterns
✅ Both valid (false-positive prevention) and invalid (error detection) paths tested
✅ ESLint RuleTester framework used appropriately with verified fixer suggestions
✅ No test inflation: 0.74:1 ratio (149 test lines vs 202 production lines)
✅ Clear, descriptive test names documenting what is verified

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

github-actions

✅ Test Quality Sentinel: 89/100 Excellent. 0% implementation tests (threshold: 30%). All 8 tests verify design-level behavioral contracts with comprehensive edge case coverage for binding resolution and error detection across CommonJS and ES modules.

github-actions · 2026-07-20T11:42:41Z

Commit pushed: ccbf9b8

Generated by PR Sous Chef · 11.7 AIC · ⌖ 13.6 AIC · ⊞ 6.9K

…7a27177dae80

gh-aw-bot · 2026-07-20T11:42:48Z

@copilot please run the pr-finisher skill, investigate the failed checks below, address any unresolved review feedback, and rerun checks once the branch is up to date.

Failed checks:

safe_outputs

Warning

Firewall blocked 1 domain

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

pi.dev

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

network:
  allowed:
    - defaults
    - "pi.dev"

See Network Configuration for more information.

Generated by 👨‍🍳 PR Sous Chef · 11.7 AIC · ⌖ 13.6 AIC · ⊞ 6.9K · ◷
_{Comment /souschef to run again}

github-actions · 2026-07-20T18:04:16Z

🎉 This pull request is included in a new release.

Release: v0.82.14

github-actions Bot added automation cookie Issue Monster Loves Cookies! eslint labels Jul 20, 2026

pelikhan marked this pull request as ready for review July 20, 2026 09:56

Copilot AI review requested due to automatic review settings July 20, 2026 09:56

Copilot started reviewing on behalf of pelikhan July 20, 2026 09:57 View session

Copilot AI reviewed Jul 20, 2026

View reviewed changes

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

[aw] No-Op Runs #43499

Closed

github-actions Bot commented Jul 20, 2026

View reviewed changes

style: format code

ccbf9b8

Merge branch 'main' into eslint-miner/require-execsync-try-catch-c1e5…

32d43f4

…7a27177dae80

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

[pr-sous-chef] Run report — 0 nudged, 3 skipped #46687

Closed

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

pelikhan merged commit cffcb58 into main Jul 20, 2026
1 check failed

pelikhan deleted the eslint-miner/require-execsync-try-catch-c1e57a27177dae80 branch July 20, 2026 11:48

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

Copilot AI requested a review from gh-aw-bot July 20, 2026 11:48

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

Smoke Test: MAI-Code-1-Flash - 29740355007 #46812

Closed

		if (isChildProcessImportBinding(def) && def.node.type === AST_NODE_TYPES.ImportNamespaceSpecifier) {
		return true;

		// Still flag it even in deferred callbacks — execSync in async callbacks is still risky.
		void withinDeferredBoundary;

Uh oh!

Conversation

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

Uh oh!

Summary

Changes

Rule behaviour

Impact

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review: require-execsync-try-catch

Uh oh!

github-actions Bot Jul 20, 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 Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 20, 2026

🧪 Test Quality Sentinel Report

Verdict

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 20, 2026

Uh oh!

gh-aw-bot commented Jul 20, 2026

Uh oh!

Uh oh!

github-actions Bot commented Jul 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jul 20, 2026 •

edited

Loading

github-actions Bot commented Jul 20, 2026 •

edited

Loading

github-actions Bot commented Jul 20, 2026 •

edited

Loading

github-actions Bot commented Jul 20, 2026 •

edited

Loading

github-actions Bot commented Jul 20, 2026 •

edited

Loading