diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index f34a858db29..1ed4be34c47 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -28,6 +28,7 @@ module.exports = [ "gh-aw-custom/require-json-parse-try-catch": "warn", "gh-aw-custom/require-parseInt-radix": "warn", "gh-aw-custom/require-return-after-core-setfailed": "warn", + "gh-aw-custom/require-spawnsync-error-check": "warn", }, }, { diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index 262850479fd..de4443d3d8a 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -14,6 +14,7 @@ import { requireJsonParseTryCatchRule } from "./rules/require-json-parse-try-cat import { requireErrorCauseInRethrowRule } from "./rules/require-error-cause-in-rethrow"; import { requireParseIntRadixRule } from "./rules/require-parseInt-radix"; import { requireReturnAfterCoreSetFailedRule } from "./rules/require-return-after-core-setfailed"; +import { requireSpawnSyncErrorCheckRule } from "./rules/require-spawnsync-error-check"; const plugin = { meta: { @@ -37,6 +38,7 @@ const plugin = { "require-json-parse-try-catch": requireJsonParseTryCatchRule, "require-parseInt-radix": requireParseIntRadixRule, "require-return-after-core-setfailed": requireReturnAfterCoreSetFailedRule, + "require-spawnsync-error-check": requireSpawnSyncErrorCheckRule, }, }; diff --git a/eslint-factory/src/rules/require-spawnsync-error-check.test.ts b/eslint-factory/src/rules/require-spawnsync-error-check.test.ts new file mode 100644 index 00000000000..075a2fbc7ed --- /dev/null +++ b/eslint-factory/src/rules/require-spawnsync-error-check.test.ts @@ -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: [], + }); + }); +}); diff --git a/eslint-factory/src/rules/require-spawnsync-error-check.ts b/eslint-factory/src/rules/require-spawnsync-error-check.ts new file mode 100644 index 00000000000..ee7f7b658f1 --- /dev/null +++ b/eslint-factory/src/rules/require-spawnsync-error-check.ts @@ -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; + } + + 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; + + const varName = node.id.name; + + // Locate the variable in scope (may be in an outer scope). + let scope: ReturnType | 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 && + parent.type === AST_NODE_TYPES.MemberExpression && + !parent.computed && + parent.object === id && + parent.property.type === AST_NODE_TYPES.Identifier && + parent.property.name === "error" + ); + }); + + if (!hasErrorCheck) { + context.report({ node: node.init, messageId: "missingErrorCheck" }); + } + }, + }; + }, +});