feat(eslint-factory): add require-execfilesync-try-catch rule - #47251
Conversation
Cover execFileSync with the same try/catch requirement as execSync. Both methods have identical throw-on-failure semantics (ChildProcessError on non-zero exit or signal termination). Changes: - Add require-execfilesync-try-catch.ts rule (mirrors require-execsync-try-catch) - Add require-execfilesync-try-catch.test.ts with 8 test cases - Register new rule in src/index.ts - Enable rule as "warn" in eslint.config.cjs - Add table entry and docs section for both execSync and execFileSync rules in README (require-execsync-try-catch was previously undocumented) - Note that execFile (async) is intentionally out of scope Closes #47211 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds lint coverage requiring child_process.execFileSync calls to use protective try/catch.
Changes:
- Adds and registers the new rule.
- Covers CommonJS, ESM, namespace, and alias bindings.
- Adds tests, configuration, and documentation.
Show a summary per file
| File | Description |
|---|---|
require-execfilesync-try-catch.ts |
Implements detection and suggestions. |
require-execfilesync-try-catch.test.ts |
Tests supported call forms. |
src/index.ts |
Registers the rule. |
README.md |
Documents synchronous execution rules. |
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 (2)
eslint-factory/src/rules/require-execfilesync-try-catch.ts:155
- The rule drops its advertised suggestion whenever
findEnclosingStatementreturnsnull. This happens for both target violations inbuild_checkout_manifest.cjs: each call is the concise body of an arrow function inside a variable declaration, and the shared helper only recognizes expression/return statements. Those calls are reported withsuggest: [], so the main corpus examples cannot receive the promised try/catch rewrite. Add safe handling for concise arrow bodies (converting the body to a block with a protected return), with a regression test for this corpus shape.
suggest: stmt
eslint-factory/README.md:428
- Node does not expose a
ChildProcessErrorclass forexecFileSync; the API throws anErrorcarrying the synchronous child-process result. Referring to a nonexistent type makes the documentation inaccurate.
Why: `execFileSync` has identical throw-on-failure semantics to `execSync` — it throws a `ChildProcessError` when the child process exits with a non-zero status code or is killed by a signal. An unhandled throw crashes the action without surfacing a useful diagnostic.
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Medium
| { | ||
| code: `const { execFileSync: run } = require("child_process"); run("git", ["status"]);`, |
| docs: { | ||
| description: | ||
| "Require execFileSync calls in actions/setup/js scripts to be wrapped in try/catch. " + | ||
| "execFileSync throws a ChildProcessError when the child process exits with a non-zero status code or is killed by a signal; " + |
|
|
||
| Require `execSync` calls sourced from `child_process` to be wrapped in `try/catch`. | ||
|
|
||
| Why: `execSync` throws a `ChildProcessError` when the child process exits with a non-zero status code or is killed by a signal. An unhandled throw crashes the action without surfacing a useful diagnostic. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #47251 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The rule implementation is functionally correct — binding detection, scope walking, and try-block guard all look solid. Two issues flagged by pre-existing inline comments should be fixed before merge:
ChildProcessErrordoes not exist in Node.js (rule message, docs) —execFileSyncthrows a plainErrorwith child-process result fields attached. Using a nonexistent type name misleads users.- Alias test gap — the member-expression alias path (
const run = cp.execFileSync; run(...)) is implemented but untested.
Neither prevents the rule from working at runtime, but both should be addressed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 15.7 AIC · ⌖ 5.57 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (5 tests)
Test Coverage Breakdown
Strengths✅ High behavioral coverage: All 5 tests verify user-visible contracts (error detection, auto-fix suggestions) Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on correctness, test coverage, and duplication.
📋 Key Themes & Highlights
Blocking Issues
- Incorrect error type (×2): README and rule message reference
ChildProcessError, which does not exist in Node's public API. Node throws anErrorwith child-process result fields augmented on it. This affects docs and the auto-suggested catch handler in the rule message. - Code duplication: The new
require-execfilesync-try-catch.tsis a near-verbatim copy ofrequire-execsync-try-catch.ts. The shared binding-resolution helpers should be extracted totry-catch-rule-utils.tsto prevent silent divergence. - Missing test: The member-expression alias path (
const run = cp.execFileSync; run(...)) is documented but has noinvalidtest case. - Misleading test label: The test "aliased execFileSync" only covers destructuring aliasing, not the distinct member-expression alias form.
Positive Highlights
- ✅ The rule correctly handles all four documented binding forms (destructured, namespace, aliased, ESM)
- ✅ Good use of the existing
buildTryCatchSuggestion/findEnclosingStatementutilities - ✅ Clean integration: wired into
index.tsandeslint.config.cjscorrectly
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 34.5 AIC · ⌖ 4.83 AIC · ⊞ 6.7K
Comment /matt to run again
| @@ -0,0 +1,180 @@ | |||
| import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils"; | |||
There was a problem hiding this comment.
[/codebase-design] This file is nearly identical to require-execsync-try-catch.ts — only the function name (execFileSync vs execSync) differs across ~180 lines. The binding-resolution helpers (isRequireChildProcess, isChildProcessImportBinding, isChildProcessObjectBinding) are copy-pasted verbatim and will silently diverge.
💡 Suggested approach
Extract a generic factory into try-catch-rule-utils.ts:
export function createChildProcessSyncRule(fnName: string, ruleName: string): Rule.RuleModule { ... }Then both rules become one-liners:
export const requireExecFileSyncTryCatchRule = createChildProcessSyncRule('execFileSync', 'require-execfilesync-try-catch');This eliminates two maintenance surfaces where fixes/improvements to binding resolution must be applied twice.
@copilot please address this.
| output: `import { execFileSync } from "child_process"; try {\n execFileSync("git", ["status"]);\n} catch (err) {\n // TODO: handle execFileSync failure (non-zero exit / signal termination).\n throw new Error(\n "execFileSync failed: " + (err instanceof Error ? err.message : String(err)),\n { cause: err },\n );\n}`, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
[/tdd] The test title says "aliased execFileSync without try/catch is flagged" but only covers destructuring-alias (const { execFileSync: run } = require(...)). The implementation also handles the member-expression alias path (const run = cp.execFileSync) documented in both the README and source — that path has no invalid test.
💡 Suggested test to add
it('invalid: member-expression alias without try/catch is flagged', () => {
cjsRuleTester.run('require-execfilesync-try-catch', requireExecFileSyncTryCatchRule, {
valid: [],
invalid: [
{
code: `const cp = require('child_process'); const run = cp.execFileSync; run('git', ['status']);`,
errors: [{ messageId: 'requireTryCatch' }],
},
],
});
});Without this, a regression in isChildProcessObjectBinding for the alias path would go undetected.
@copilot please address this.
There was a problem hiding this comment.
Request Changes
The rule logic and intent are correct, but three issues need fixing before merge.
Blocking issues (2 medium + pre-existing comments)
-
Code duplication (new comment, line 10 of rule file):
isRequireChildProcess,isChildProcessImportBinding, andisChildProcessObjectBindingare verbatim copies fromrequire-execsync-try-catch.ts. These must be extracted to shared utils — bug fixes to one file will silently not apply to the other. -
Missing
node:child_processinvalid test (new comment, test line 120): Invalid-case tests only coverchild_process, notnode:child_process. A regression in specifier handling for thenode:prefix in the flagging path would go undetected. -
ChildProcessErroris a non-existent type (existing comments): Both the rule description and the README refer toChildProcessError, which Node.js does not expose publicly.execFileSyncthrows a plainErrorwith augmented fields (.status,.signal,.stderr,.stdout). Fix the wording. -
Member-expression alias test is missing (existing comment):
const run = cp.execFileSync; run(...)is documented as a supported detection form but has no test coverage.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 39.1 AIC · ⌖ 4.78 AIC · ⊞ 5.6K
Comment /review to run again
|
|
||
| type SourceCodeScope = ReturnType<TSESLint.SourceCode["getScope"]>; | ||
|
|
||
| function isRequireChildProcess(node: TSESTree.Node | null | undefined): boolean { |
There was a problem hiding this comment.
Entire rule body duplicated from require-execsync-try-catch.ts — any bug fix to shared helpers will only apply to whichever file is edited, silently leaving the other broken.
💡 Details
isRequireChildProcess, isChildProcessImportBinding, and isChildProcessObjectBinding are verbatim copies of the same helpers in require-execsync-try-catch.ts. Consequences:
- Adding a new child process specifier, fixing a scope-walk bug, or changing the alias-detection logic must be done twice or one rule silently regresses.
- These helpers are pure utility functions with no rule-specific behavior — they belong in
try-catch-rule-utils.ts(or a dedicatedchild-process-utils.ts) and should be imported by both rules.
The two rules differ only in the target function name (execSync vs execFileSync) and their diagnostic messages. All detection logic should be shared.
| ], | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Missing invalid-case test for node:child_process namespace form — a regression in the node: prefix path for namespace requires would go undetected.
💡 Details
The valid-case tests include node:child_process (lines 113, 129), but the invalid cases only test child_process. There is no test asserting that:
const cp = require('node:child_process');
cp.execFileSync('git', ['status']); // should be flaggedAdd at least one invalid case using the node: specifier with namespace access to ensure CHILD_PROCESS_SPECIFIERS is applied symmetrically for the flagging path, not just the pass-through path.
|
@copilot please run the Unresolved reviews:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved review feedback in commit Updates include:
I also reran local checks ( CI on this new HEAD is stale from the agent push and needs a maintainer re-trigger. |
|
@copilot merge main and recompile |
…y-cover-execfilesync # Conflicts: # eslint-factory/eslint.config.cjs # eslint-factory/src/index.ts Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in |
|
@copilot please run the Unresolved reviews:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in |
|
🎉 This pull request is included in a new release. Release: |
execFileSynchas identical throw-on-failure semantics toexecSync(throwsChildProcessErroron non-zero exit or signal), but was not covered by any lint rule — leaving unprotected call sites able to crash actions silently.Changes
require-execfilesync-try-catch— flags unprotectedexecFileSynccalls sourced fromchild_process/node:child_process; emits a suggestion that wraps the enclosing statement intry/catchwith a typed rethrowcp.execFileSync), aliased (const run = cp.execFileSync), ESM importsrc/index.tsand enabled as"warn"ineslint.config.cjsrequire-execsync-try-catch; explicitly documents thatexecFile(async, callback-based) is out of scope