-
Notifications
You must be signed in to change notification settings - Fork 454
[eslint-miner] feat(eslint): add require-spawnsync-error-check rule #45246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| import { RuleTester } from "eslint"; | ||
| import { describe, expect, it } from "vitest"; | ||
| import { requireSpawnSyncErrorCheckRule } from "./require-spawnsync-error-check"; | ||
|
|
||
| const cjsRuleTester = new RuleTester({ | ||
| languageOptions: { | ||
| ecmaVersion: 2022, | ||
| sourceType: "commonjs", | ||
| }, | ||
| }); | ||
|
|
||
| describe("require-spawnsync-error-check", () => { | ||
| it("uses the correct docs URL", () => { | ||
| expect(requireSpawnSyncErrorCheckRule.meta.docs.url).toBe( | ||
| "https://github.com/github/gh-aw/tree/main/eslint-factory#require-spawnsync-error-check", | ||
| ); | ||
| }); | ||
|
|
||
| it("valid: result.error is checked alongside result.status", () => { | ||
| cjsRuleTester.run("require-spawnsync-error-check", requireSpawnSyncErrorCheckRule, { | ||
| valid: [ | ||
| // bare spawnSync, checks result.error | ||
| `const result = spawnSync("git", ["status"]); if (result.error) throw result.error; if (result.status !== 0) throw new Error("failed");`, | ||
| // namespaced childProcess.spawnSync, checks result.error | ||
| `const result = childProcess.spawnSync("git", ["status"]); if (result.error) throw result.error;`, | ||
| // child_process.spawnSync, checks result.error | ||
| `const result = child_process.spawnSync("curl", ["-v"]); if (result.error) { throw result.error; } if (result.status !== 0) throw new Error("x");`, | ||
| // result is accessed via .error destructuring equivalent — property access | ||
| `const r = spawnSync("zip", ["-v"]); const e = r.error; if (e) throw e;`, | ||
| ], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
|
|
||
| it("invalid: only result.status checked, result.error never read", () => { | ||
| cjsRuleTester.run("require-spawnsync-error-check", requireSpawnSyncErrorCheckRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `const result = spawnSync("git", ["status"]); if (result.status !== 0) throw new Error("failed");`, | ||
| errors: [{ messageId: "missingErrorCheck" }], | ||
| }, | ||
| { | ||
| code: `const result = childProcess.spawnSync("zip", ["-v"], { stdio: "ignore" }); if (result.status !== 0) throw new Error("zip not found");`, | ||
| errors: [{ messageId: "missingErrorCheck" }], | ||
| }, | ||
| { | ||
| code: `const result = child_process.spawnSync("curl", ["--version"]); return result.stdout;`, | ||
| errors: [{ messageId: "missingErrorCheck" }], | ||
| }, | ||
| ], | ||
| }); | ||
| }); | ||
|
|
||
| it("valid: non-spawnSync call is ignored", () => { | ||
| cjsRuleTester.run("require-spawnsync-error-check", requireSpawnSyncErrorCheckRule, { | ||
| valid: [ | ||
| `const result = execSync("git status"); if (!result) throw new Error("failed");`, | ||
| `const result = spawnSync; result.toString();`, | ||
| ], | ||
| invalid: [], | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| import { AST_NODE_TYPES, ESLintUtils, TSESTree } from "@typescript-eslint/utils"; | ||
|
|
||
| const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); | ||
|
|
||
| // Unqualified function name used when spawnSync is destructured from child_process. | ||
| const SPAWNSYNC_NAME = "spawnSync"; | ||
|
|
||
| // Known namespace aliases for the child_process module. | ||
| const CHILD_PROCESS_OBJECTS = new Set(["childProcess", "child_process"]); | ||
|
|
||
| /** | ||
| * Returns true when the expression is a call to spawnSync (either bare or namespaced). | ||
| * Matched forms: | ||
| * spawnSync(cmd, args, opts) | ||
| * childProcess.spawnSync(cmd, args, opts) | ||
| * child_process.spawnSync(cmd, args, opts) | ||
| */ | ||
| function isSpawnSyncCall(node: TSESTree.Expression): boolean { | ||
| if (node.type !== AST_NODE_TYPES.CallExpression) return false; | ||
| const callee = node.callee; | ||
|
|
||
| if (callee.type === AST_NODE_TYPES.Identifier && callee.name === SPAWNSYNC_NAME) { | ||
| return true; | ||
|
Comment on lines
+22
to
+23
|
||
| } | ||
|
|
||
| if ( | ||
| callee.type === AST_NODE_TYPES.MemberExpression && | ||
| !callee.computed && | ||
| callee.object.type === AST_NODE_TYPES.Identifier && | ||
| CHILD_PROCESS_OBJECTS.has(callee.object.name) && | ||
| callee.property.type === AST_NODE_TYPES.Identifier && | ||
| callee.property.name === SPAWNSYNC_NAME | ||
| ) { | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| export const requireSpawnSyncErrorCheckRule = createRule({ | ||
| name: "require-spawnsync-error-check", | ||
| meta: { | ||
| type: "problem", | ||
| docs: { | ||
| description: | ||
| "Require spawnSync result variables in actions/setup/js scripts to check result.error in addition to result.status. " + | ||
| "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 reports a misleading 'exit null' message.", | ||
| }, | ||
| schema: [], | ||
| messages: { | ||
| missingErrorCheck: | ||
| "spawnSync result must have its .error property checked. " + | ||
| "When the child process cannot be spawned (e.g. ENOENT, ETIMEDOUT), result.status is null and result.error contains the cause — " + | ||
| "checking only result.status produces a misleading diagnostic. Add: if (result.error) { throw result.error; }", | ||
| }, | ||
| }, | ||
| defaultOptions: [], | ||
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
|
|
||
| return { | ||
| VariableDeclarator(node: TSESTree.VariableDeclarator) { | ||
| if (!node.init) return; | ||
| if (!isSpawnSyncCall(node.init)) return; | ||
|
|
||
| // Only handle simple identifier bindings: const result = spawnSync(...) | ||
| if (node.id.type !== AST_NODE_TYPES.Identifier) return; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The rule only tracks simple 💡 Suggested fix or testEither document the limitation in the rule description/schema, or add an // 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. |
||
|
|
||
| const varName = node.id.name; | ||
|
|
||
| // Locate the variable in scope (may be in an outer scope). | ||
| let scope: ReturnType<typeof sourceCode.getScope> | null = sourceCode.getScope(node); | ||
| let variable: (typeof scope)["variables"][number] | undefined; | ||
| while (scope) { | ||
| variable = scope.set.get(varName); | ||
| if (variable) break; | ||
| scope = scope.upper; | ||
| } | ||
|
|
||
| if (!variable) return; | ||
|
|
||
| // Check whether any reference to this variable reads the .error property. | ||
| const hasErrorCheck = variable.references.some(ref => { | ||
|
|
||
| const id = ref.identifier; | ||
| const parent = id.parent; | ||
| return ( | ||
| parent !== undefined && | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 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 @copilot please address this. |
||
| parent.type === AST_NODE_TYPES.MemberExpression && | ||
| !parent.computed && | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] False positive risk: destructuring 💡 Suggested fixThe current check only recognises // 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 Add a corresponding test case: `const result = spawnSync("git", ["status"]); const { error } = result; if (error) throw error;`@copilot please address this. |
||
| parent.object === id && | ||
| parent.property.type === AST_NODE_TYPES.Identifier && | ||
| parent.property.name === "error" | ||
| ); | ||
| }); | ||
|
|
||
| if (!hasErrorCheck) { | ||
| context.report({ node: node.init, messageId: "missingErrorCheck" }); | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
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.
[/grill-with-docs]
CHILD_PROCESS_OBJECTSis 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
descriptionclarifying that the rule only covers the two well-known aliases (childProcess,child_process) and barespawnSynccalls — not arbitraryrequire-assigned aliases. This sets correct expectations and prevents confusion when new aliases appear.@copilot please address this.