Tighten require-spawnsync-error-check for destructuring and guard semantics

.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`

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" }],
+        },
       ],
     });
   });

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<TSESLint.SourceCode["getScope"]>;
+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<TSESLint.SourceCode>, node: TSESTree.Node, varName: string): ScopeVariable | undefined {
+  let scope: ReturnType<typeof sourceCode.getScope> | 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<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 (!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" });
         }
       },