feat(eslint-factory): add require-execfilesync-try-catch rule #47251
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| import { RuleTester } from "eslint"; | ||
| import { describe, it } from "vitest"; | ||
| import { requireExecFileSyncTryCatchRule } from "./require-execfilesync-try-catch"; | ||
|
|
||
| const cjsRuleTester = new RuleTester({ | ||
| languageOptions: { | ||
| ecmaVersion: 2022, | ||
| sourceType: "commonjs", | ||
| }, | ||
| }); | ||
|
|
||
| const esmRuleTester = new RuleTester({ | ||
| languageOptions: { | ||
| ecmaVersion: 2022, | ||
| sourceType: "module", | ||
| }, | ||
| }); | ||
|
|
||
| describe("require-execfilesync-try-catch", () => { | ||
| it("valid: execFileSync inside try block passes (CommonJS, destructured)", () => { | ||
| cjsRuleTester.run("require-execfilesync-try-catch", requireExecFileSyncTryCatchRule, { | ||
| valid: [ | ||
| `const { execFileSync } = require("child_process"); try { execFileSync("git", ["status"]); } catch (e) {}`, | ||
| `const { execFileSync } = require("node:child_process"); try { execFileSync("git", ["status"]); } catch (e) {}`, | ||
| `const { execFileSync } = require("child_process"); function f() { try { execFileSync("git", ["status"]); } catch (e) {} }`, | ||
| ], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("valid: execFileSync inside try block passes (CommonJS, namespace)", () => { | ||
| cjsRuleTester.run("require-execfilesync-try-catch", requireExecFileSyncTryCatchRule, { | ||
| valid: [`const cp = require("child_process"); try { cp.execFileSync("git", ["status"]); } catch (e) {}`], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("valid: execFileSync inside try block passes (ES module)", () => { | ||
| esmRuleTester.run("require-execfilesync-try-catch", requireExecFileSyncTryCatchRule, { | ||
| valid: [`import { execFileSync } from "child_process"; try { execFileSync("git", ["status"]); } catch (e) {}`, `import { execFileSync } from "node:child_process"; try { execFileSync("git", ["status"]); } catch (e) {}`], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("valid: execFileSync from non-child_process module is ignored", () => { | ||
| cjsRuleTester.run("require-execfilesync-try-catch", requireExecFileSyncTryCatchRule, { | ||
| valid: [ | ||
| // execFileSync from an unrelated module — should not be flagged | ||
| `const { execFileSync } = require("some-other-lib"); execFileSync("git", ["status"]);`, | ||
| // bare execFileSync without any require — should not be flagged | ||
| `execFileSync("git", ["status"]);`, | ||
| // member call on unrelated object | ||
| `mockChild.execFileSync("git", ["status"]);`, | ||
| ], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: execFileSync without try/catch (CommonJS, destructured)", () => { | ||
| cjsRuleTester.run("require-execfilesync-try-catch", requireExecFileSyncTryCatchRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `const { execFileSync } = require("child_process"); execFileSync("git", ["status"]);`, | ||
| errors: [ | ||
| { | ||
| messageId: "requireTryCatch", | ||
| suggestions: [ | ||
| { | ||
| messageId: "wrapInTryCatch", | ||
| output: `const { execFileSync } = require("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}`, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: execFileSync without try/catch (CommonJS, namespace)", () => { | ||
| cjsRuleTester.run("require-execfilesync-try-catch", requireExecFileSyncTryCatchRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `const cp = require("child_process"); cp.execFileSync("git", ["status"]);`, | ||
| errors: [ | ||
| { | ||
| messageId: "requireTryCatch", | ||
| suggestions: [ | ||
| { | ||
| messageId: "wrapInTryCatch", | ||
| output: `const cp = require("child_process"); try {\n cp.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}`, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| code: `const cp = require("node:child_process"); cp.execFileSync("git", ["status"]);`, | ||
| errors: [ | ||
| { | ||
| messageId: "requireTryCatch", | ||
| suggestions: [ | ||
| { | ||
| messageId: "wrapInTryCatch", | ||
| output: `const cp = require("node:child_process"); try {\n cp.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}`, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: execFileSync without try/catch (ES module)", () => { | ||
| esmRuleTester.run("require-execfilesync-try-catch", requireExecFileSyncTryCatchRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `import { execFileSync } from "child_process"; execFileSync("git", ["status"]);`, | ||
| errors: [ | ||
| { | ||
| messageId: "requireTryCatch", | ||
| suggestions: [ | ||
| { | ||
| messageId: "wrapInTryCatch", | ||
| 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}`, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Missing invalid-case test for 💡 DetailsThe valid-case tests include const cp = require('node:child_process');
cp.execFileSync('git', ['status']); // should be flaggedAdd at least one invalid case using the |
||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: destructured alias of execFileSync without try/catch is flagged", () => { | ||
| cjsRuleTester.run("require-execfilesync-try-catch", requireExecFileSyncTryCatchRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `const { execFileSync: run } = require("child_process"); run("git", ["status"]);`, | ||
|
Comment on lines
+143
to
+144
|
||
| errors: [ | ||
| { | ||
| messageId: "requireTryCatch", | ||
| suggestions: [ | ||
| { | ||
| messageId: "wrapInTryCatch", | ||
| output: `const { execFileSync: run } = require("child_process"); try {\n run("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}`, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: member-expression alias of execFileSync 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", | ||
| suggestions: [ | ||
| { | ||
| messageId: "wrapInTryCatch", | ||
| output: `const cp = require("child_process"); const run = cp.execFileSync; try {\n run("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}`, | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils"; | ||
|
Contributor
There was a problem hiding this comment. [/codebase-design] This file is nearly identical to 💡 Suggested approachExtract a generic factory into 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. |
||
| import { buildTryCatchSuggestion, findEnclosingStatement, isChildProcessImportBinding, isChildProcessObjectBinding, isInsideTryBlock, isRequireChildProcess } from "./try-catch-rule-utils"; | ||
|
|
||
| const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); | ||
|
|
||
| type SourceCodeScope = ReturnType<TSESLint.SourceCode["getScope"]>; | ||
|
|
||
| /** | ||
| * Walks the scope chain to decide whether `identifierName` resolves to | ||
| * `execFileSync` from `child_process`. | ||
| */ | ||
| function isExecFileSyncBinding(identifierName: string, scopeNode: TSESTree.Node, sourceCode: TSESLint.SourceCode): boolean { | ||
| let scope: SourceCodeScope | null = sourceCode.getScope(scopeNode); | ||
| while (scope) { | ||
| const variable = scope.set.get(identifierName); | ||
| if (variable && variable.defs.length > 0) { | ||
| for (const def of variable.defs) { | ||
| // ESM: import { execFileSync } from "child_process" | ||
| if (isChildProcessImportBinding(def) && def.node.type === AST_NODE_TYPES.ImportSpecifier) { | ||
| const specifier = def.node as TSESTree.ImportSpecifier; | ||
| const importedName = specifier.imported.type === AST_NODE_TYPES.Identifier ? specifier.imported.name : null; | ||
| if (importedName === "execFileSync") return true; | ||
| } | ||
| // CJS: const { execFileSync } = require("child_process") | ||
| if (def.type === "Variable") { | ||
| const declarator = def.node as TSESTree.VariableDeclarator; | ||
| if (declarator.id.type === AST_NODE_TYPES.ObjectPattern && isRequireChildProcess(declarator.init)) { | ||
| for (const prop of declarator.id.properties) { | ||
| if (prop.type !== AST_NODE_TYPES.Property) continue; | ||
| if (prop.key.type !== AST_NODE_TYPES.Identifier || prop.key.name !== "execFileSync") continue; | ||
| const boundName = prop.value.type === AST_NODE_TYPES.Identifier ? prop.value.name : null; | ||
| if (boundName === identifierName) return true; | ||
| } | ||
| } | ||
| // const execFileSync = childProcess.execFileSync | ||
| if (declarator.id.type === AST_NODE_TYPES.Identifier && declarator.init?.type === AST_NODE_TYPES.MemberExpression) { | ||
| const init = declarator.init; | ||
| if ( | ||
| !init.computed && | ||
| init.object.type === AST_NODE_TYPES.Identifier && | ||
| isChildProcessObjectBinding(init.object.name, init.object, sourceCode) && | ||
| init.property.type === AST_NODE_TYPES.Identifier && | ||
| init.property.name === "execFileSync" | ||
| ) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| scope = scope.upper; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Returns true if the CallExpression is an `execFileSync(...)` call sourced from | ||
| * the `child_process` module. | ||
| */ | ||
| function isExecFileSyncCall(node: TSESTree.CallExpression, sourceCode: TSESLint.SourceCode): boolean { | ||
| const callee = node.callee; | ||
|
|
||
| // execFileSync(...) — destructured or aliased | ||
| if (callee.type === AST_NODE_TYPES.Identifier) { | ||
| return isExecFileSyncBinding(callee.name, callee, sourceCode); | ||
| } | ||
|
|
||
| // childProcess.execFileSync(...) or cp.execFileSync(...) | ||
| if (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 === "execFileSync") { | ||
| return isChildProcessObjectBinding(callee.object.name, callee.object, sourceCode); | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| export const requireExecFileSyncTryCatchRule = createRule({ | ||
| name: "require-execfilesync-try-catch", | ||
| meta: { | ||
| type: "problem", | ||
| hasSuggestions: true, | ||
| docs: { | ||
| description: | ||
| "Require execFileSync calls in actions/setup/js scripts to be wrapped in try/catch. " + | ||
| "execFileSync throws an Error containing child-process result fields 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.", | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| requireTryCatch: "Wrap execFileSync({{arg}}) in try/catch — execFileSync throws when the process exits non-zero or is killed by a signal, " + "and will crash the action if the error is unhandled.", | ||
| wrapInTryCatch: "Wrap in try { ... } catch { ... } and re-throw with { cause: err } to preserve context.", | ||
| }, | ||
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
|
|
||
| return { | ||
| CallExpression(node) { | ||
| if (!isExecFileSyncCall(node, sourceCode)) return; | ||
| if (isInsideTryBlock(sourceCode, node)) return; | ||
|
|
||
| const argText = node.arguments.length > 0 ? sourceCode.getText(node.arguments[0]) : ""; | ||
| const stmt = findEnclosingStatement(sourceCode, node); | ||
|
|
||
| context.report({ | ||
| node, | ||
| messageId: "requireTryCatch", | ||
| data: { arg: argText }, | ||
| suggest: stmt | ||
| ? [ | ||
| { | ||
| messageId: "wrapInTryCatch", | ||
| fix(fixer) { | ||
| const stmtText = sourceCode.getText(stmt); | ||
| const startLine = stmt.loc?.start.line; | ||
| const stmtLine = startLine !== undefined ? (sourceCode.lines[startLine - 1] ?? "") : ""; | ||
| const indent = stmtLine.match(/^(\s*)/)?.[1] ?? ""; | ||
| return fixer.replaceText( | ||
| stmt, | ||
| buildTryCatchSuggestion(stmtText, { | ||
| indent, | ||
| todoComment: "TODO: handle execFileSync failure (non-zero exit / signal termination).", | ||
| errorPrefix: "execFileSync failed: ", | ||
| }) | ||
| ); | ||
| }, | ||
| }, | ||
| ] | ||
| : [], | ||
| }); | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/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
Without this, a regression in
isChildProcessObjectBindingfor the alias path would go undetected.@copilot please address this.