diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 72145f10bda..aee174f47cd 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -22,6 +22,7 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/charts-trending.md` - `.github/aw/charts.md` - `.github/aw/cli-commands.md` +- `.github/aw/configure-agentic-engine.md` - `.github/aw/context.md` - `.github/aw/create-agentic-workflow-trigger-details.md` - `.github/aw/create-agentic-workflow.md` diff --git a/eslint-factory/src/rules/require-spawnsync-error-check.test.ts b/eslint-factory/src/rules/require-spawnsync-error-check.test.ts index d0fb03aa83c..4d3ce09a60c 100644 --- a/eslint-factory/src/rules/require-spawnsync-error-check.test.ts +++ b/eslint-factory/src/rules/require-spawnsync-error-check.test.ts @@ -23,8 +23,20 @@ describe("require-spawnsync-error-check", () => { `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;`, + // logging is fine when there is also a guard + `const result = spawnSync("git", ["status"]); if (result.error) throw result.error; core.info(String(result.error));`, + // logging before a later guard is still valid + `const result = spawnSync("git", ["status"]); core.debug(String(result.error)); if (result.error) throw result.error;`, + // destructured binding includes error and guards on it + `const { status, error } = spawnSync("zip", ["-v"]); if (error) throw error; if (status !== 0) throw new Error("x");`, + // renamed destructuring bindings are supported + `const { error: spawnError } = spawnSync("zip", ["-v"]); if (spawnError !== undefined) throw spawnError;`, + // string-literal keys are supported when they bind error + `const { "error": spawnError } = spawnSync("zip", ["-v"]); if (spawnError) throw spawnError;`, + // comparison-based guards should count + `const result = spawnSync("git", ["status"]); if (result.error !== undefined) throw result.error;`, + // result.error on the right side of || can still guard when the full expression is the test + `const result = spawnSync("git", ["status"]); if (result.status !== 0 || result.error) throw result.error;`, ], invalid: [], }); @@ -46,6 +58,34 @@ describe("require-spawnsync-error-check", () => { code: `const result = child_process.spawnSync("curl", ["--version"]); return result.stdout;`, errors: [{ messageId: "missingErrorCheck" }], }, + { + code: `const result = spawnSync("git", ["status"]); core.info(String(result.error)); if (result.status !== 0) throw new Error("failed");`, + errors: [{ messageId: "missingErrorCheck" }], + }, + { + code: `const { status } = spawnSync("git", ["status"]); if (status !== 0) throw new Error("failed");`, + errors: [{ messageId: "missingErrorCheck" }], + }, + { + code: `const { status, error } = spawnSync("git", ["status"]); core.info(String(error)); if (status !== 0) throw new Error("failed");`, + errors: [{ messageId: "missingErrorCheck" }], + }, + { + code: `const result = spawnSync("git", ["status"]); const cached = result.error && result.error.message; return cached;`, + errors: [{ messageId: "missingErrorCheck" }], + }, + { + code: `const result = spawnSync("git", ["status"]); if (result.status !== 0 && result.error) core.info(String(result.error));`, + errors: [{ messageId: "missingErrorCheck" }], + }, + { + code: `const result = spawnSync("git", ["status"]); const fallback = result.error || new Error("fallback"); throw fallback;`, + errors: [{ messageId: "missingErrorCheck" }], + }, + { + code: `const result = spawnSync("git", ["status"]); const maybeError = result.error ?? null; return maybeError;`, + errors: [{ messageId: "missingErrorCheck" }], + }, ], }); }); diff --git a/eslint-factory/src/rules/require-spawnsync-error-check.ts b/eslint-factory/src/rules/require-spawnsync-error-check.ts index 727b87382c9..dc84d7373cd 100644 --- a/eslint-factory/src/rules/require-spawnsync-error-check.ts +++ b/eslint-factory/src/rules/require-spawnsync-error-check.ts @@ -1,4 +1,4 @@ -import { AST_NODE_TYPES, ESLintUtils, TSESTree } from "@typescript-eslint/utils"; +import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-eslint/utils"; const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); @@ -7,6 +7,101 @@ const SPAWNSYNC_NAME = "spawnSync"; // Known namespace aliases for the child_process module. const CHILD_PROCESS_OBJECTS = new Set(["childProcess", "child_process"]); +const CONDITIONAL_TEST_PARENTS = new Set([AST_NODE_TYPES.IfStatement, AST_NODE_TYPES.WhileStatement, AST_NODE_TYPES.DoWhileStatement, AST_NODE_TYPES.ForStatement]); + +function isConditionalTestParent(node: TSESTree.Node): node is TSESTree.IfStatement | TSESTree.WhileStatement | TSESTree.DoWhileStatement | TSESTree.ForStatement { + return CONDITIONAL_TEST_PARENTS.has(node.type); +} + +type ScopeType = ReturnType; +type ScopeVariable = ScopeType["variables"][number]; + +function isGuardingErrorUsage(node: TSESTree.Expression): boolean { + let current: TSESTree.Node = node; + + while (current.parent) { + const parent: TSESTree.Node = current.parent; + + if (isConditionalTestParent(parent) && parent.test === current) { + return true; + } + + if (parent.type === AST_NODE_TYPES.ConditionalExpression && parent.test === current) { + return true; + } + + if (parent.type === AST_NODE_TYPES.LogicalExpression) { + // Guarding intent depends on how the full logical expression is used: + // climb through the left operand of either operator and the right operand of `||`, + // but reject the right operand of `&&` because it executes conditionally and + // does not establish an independent guard. + if (parent.left === current || (parent.operator === "||" && parent.right === current)) { + current = parent; + continue; + } + + break; + } + + if ((parent.type === AST_NODE_TYPES.ThrowStatement || parent.type === AST_NODE_TYPES.ReturnStatement) && parent.argument === current) { + return true; + } + + if (parent.type === AST_NODE_TYPES.UnaryExpression && parent.operator === "!" && parent.argument === current) { + current = parent; + continue; + } + + if (parent.type === AST_NODE_TYPES.BinaryExpression && (parent.left === current || parent.right === current)) { + current = parent; + continue; + } + + break; + } + + return false; +} + +function findVariableByName(sourceCode: Readonly, node: TSESTree.Node, varName: string): ScopeVariable | undefined { + let scope: ReturnType | null = sourceCode.getScope(node); + while (scope) { + const variable = scope.set.get(varName); + if (variable) return variable; + scope = scope.upper; + } + return undefined; +} + +function isErrorKey(node: TSESTree.PropertyName): boolean { + return (node.type === AST_NODE_TYPES.Identifier && node.name === "error") || (node.type === AST_NODE_TYPES.Literal && node.value === "error"); +} + +function getErrorBindingNames(node: TSESTree.ObjectPattern): string[] { + const names: string[] = []; + + for (const property of node.properties) { + // Intentionally only support static `error` keys (`{ error }` / `{ error: err }`). + // Computed keys are ignored. + if (property.type !== AST_NODE_TYPES.Property || property.computed) { + continue; + } + + if (!isErrorKey(property.key)) { + continue; + } + + const value = property.value; + if (value.type === AST_NODE_TYPES.Identifier) { + names.push(value.name); + } else if (value.type === AST_NODE_TYPES.AssignmentPattern && value.left.type === AST_NODE_TYPES.Identifier) { + // Support `{ error = defaultValue }` by tracking the bound identifier name. + names.push(value.left.name); + } + } + + return names; +} /** * Returns true when the expression is a call to spawnSync (either bare or namespaced). @@ -45,7 +140,8 @@ export const requireSpawnSyncErrorCheckRule = createRule({ 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.", + "checking only result.status silently swallows spawn-level failures or reports a misleading 'exit null' message. " + + "Scope: this rule checks variable declarator initializers (including object destructuring) and does not analyze AssignmentExpression forms (`result = spawnSync(...)`) or inline chains (`spawnSync(...).status`).", }, schema: [], messages: { @@ -64,30 +160,47 @@ export const requireSpawnSyncErrorCheckRule = createRule({ 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; + if (node.id.type === AST_NODE_TYPES.ObjectPattern) { + const errorBindingNames = getErrorBindingNames(node.id); + if (errorBindingNames.length === 0) { + context.report({ node: node.init, messageId: "missingErrorCheck" }); + return; + } - const varName = node.id.name; + const hasGuardingErrorCheck = errorBindingNames.some(varName => { + const variable = findVariableByName(sourceCode, node, varName); + if (!variable) return false; + return variable.references.some(ref => ref.identifier.type === AST_NODE_TYPES.Identifier && isGuardingErrorUsage(ref.identifier)); + }); - // 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 (!hasGuardingErrorCheck) { + context.report({ node: node.init, messageId: "missingErrorCheck" }); + } + return; } + // Only handle simple identifier bindings: const result = spawnSync(...) + if (node.id.type !== AST_NODE_TYPES.Identifier) return; + + const variable = findVariableByName(sourceCode, node, node.id.name); if (!variable) return; - // Check whether any reference to this variable reads the .error property. - const hasErrorCheck = variable.references.some(ref => { + // Check whether any reference to this variable uses .error in a guarding position. + const hasGuardingErrorCheck = 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"; + 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" && + isGuardingErrorUsage(parent) + ); }); - if (!hasErrorCheck) { + if (!hasGuardingErrorCheck) { context.report({ node: node.init, messageId: "missingErrorCheck" }); } },