From e94348fa545caa3981acd98c07270ebe56720e42 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 28 Jul 2026 09:44:27 +0000 Subject: [PATCH 1/2] feat(eslint): add prefer-get-error-message-over-string rule Adds a new custom ESLint rule that flags String(err) interpolations inside template literals when the value is a caught error variable (try/catch binding or inline .catch()/.then() rejection handler) and getErrorMessage is already resolvable in the current scope. Motivation: many actions/setup/js files already import getErrorMessage from error_helpers.cjs and use it consistently at most call sites, yet still call String(err) at other call sites in the same file. String(err) on an Error produces the redundant "Error: message" prefix and skips HTML error-page sanitization that getErrorMessage provides. This rule catches that same-file inconsistency (93 real hits found in actions/setup/js when run against the current codebase, 0 false positives observed in review). The rule intentionally stays quiet when getErrorMessage is not resolvable in scope, to avoid suggesting an import that isn't already in use nearby (kept as suggestion, not auto-fix, matching the pattern used by no-caught-error-interpolation and prefer-get-error-message). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- eslint-factory/eslint.config.cjs | 1 + eslint-factory/src/index.ts | 2 + ...efer-get-error-message-over-string.test.ts | 84 ++++++++++ .../prefer-get-error-message-over-string.ts | 144 ++++++++++++++++++ 4 files changed, 231 insertions(+) create mode 100644 eslint-factory/src/rules/prefer-get-error-message-over-string.test.ts create mode 100644 eslint-factory/src/rules/prefer-get-error-message-over-string.ts diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index 0cb81fe8524..dc397768ce5 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -20,6 +20,7 @@ module.exports = [ "gh-aw-custom/no-unsafe-catch-error-property": "warn", "gh-aw-custom/no-unsafe-promise-catch-error-property": "warn", "gh-aw-custom/prefer-get-error-message": "warn", + "gh-aw-custom/prefer-get-error-message-over-string": "warn", "gh-aw-custom/prefer-number-isnan": "warn", "gh-aw-custom/require-async-entrypoint-catch": "warn", "gh-aw-custom/require-await-core-summary-write": "warn", diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index e5b276bf65a..7bfcf7daaca 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -6,6 +6,7 @@ import { noJsonStringifyErrorRule } from "./rules/no-json-stringify-error"; import { noUnsafeCatchErrorPropertyRule } from "./rules/no-unsafe-catch-error-property"; import { noUnsafePromiseCatchErrorPropertyRule } from "./rules/no-unsafe-promise-catch-error-property"; import { preferGetErrorMessageRule } from "./rules/prefer-get-error-message"; +import { preferGetErrorMessageOverStringRule } from "./rules/prefer-get-error-message-over-string"; import { preferNumberIsNanRule } from "./rules/prefer-number-isnan"; import { requireAsyncEntrypointCatchRule } from "./rules/require-async-entrypoint-catch"; import { requireAwaitCoreSummaryWriteRule } from "./rules/require-await-core-summary-write"; @@ -45,6 +46,7 @@ const plugin = { "no-unsafe-catch-error-property": noUnsafeCatchErrorPropertyRule, "no-unsafe-promise-catch-error-property": noUnsafePromiseCatchErrorPropertyRule, "prefer-get-error-message": preferGetErrorMessageRule, + "prefer-get-error-message-over-string": preferGetErrorMessageOverStringRule, "prefer-number-isnan": preferNumberIsNanRule, "require-async-entrypoint-catch": requireAsyncEntrypointCatchRule, "require-await-core-summary-write": requireAwaitCoreSummaryWriteRule, diff --git a/eslint-factory/src/rules/prefer-get-error-message-over-string.test.ts b/eslint-factory/src/rules/prefer-get-error-message-over-string.test.ts new file mode 100644 index 00000000000..78a1a5f1003 --- /dev/null +++ b/eslint-factory/src/rules/prefer-get-error-message-over-string.test.ts @@ -0,0 +1,84 @@ +import { RuleTester } from "eslint"; +import { describe, it } from "vitest"; +import { preferGetErrorMessageOverStringRule } from "./prefer-get-error-message-over-string"; + +const cjsRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "commonjs", + }, +}); + +describe("prefer-get-error-message-over-string", () => { + it("valid: String(err) not flagged when getErrorMessage is unavailable", () => { + cjsRuleTester.run("prefer-get-error-message-over-string", preferGetErrorMessageOverStringRule, { + valid: [`try { f(); } catch (err) { console.log(\`failed: \${String(err)}\`); }`, `p.catch(err => \`error: \${String(err)}\`);`], + invalid: [], + }); + }); + + it("valid: non-caught variable String() interpolation is not flagged", () => { + cjsRuleTester.run("prefer-get-error-message-over-string", preferGetErrorMessageOverStringRule, { + valid: [`const { getErrorMessage } = require("./error_helpers.cjs"); const name = "world"; const s = \`Hello \${String(name)}\`;`], + invalid: [], + }); + }); + + it("valid: getErrorMessage(err) usage is not flagged", () => { + cjsRuleTester.run("prefer-get-error-message-over-string", preferGetErrorMessageOverStringRule, { + valid: [`const { getErrorMessage } = require("./error_helpers.cjs"); try { f(); } catch (err) { console.log(\`failed: \${getErrorMessage(err)}\`); }`], + invalid: [], + }); + }); + + it("invalid: String(err) flagged in catch block when getErrorMessage is imported", () => { + cjsRuleTester.run("prefer-get-error-message-over-string", preferGetErrorMessageOverStringRule, { + valid: [], + invalid: [ + { + code: `const { getErrorMessage } = require("./error_helpers.cjs"); try { f(); } catch (err) { throw new Error(\`Failed: \${String(err)}\`, { cause: err }); }`, + errors: [ + { + messageId: "preferGetErrorMessage", + suggestions: [ + { + messageId: "replaceWithGetErrorMessage", + output: `const { getErrorMessage } = require("./error_helpers.cjs"); try { f(); } catch (err) { throw new Error(\`Failed: \${getErrorMessage(err)}\`, { cause: err }); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: String(err) flagged in .catch(fn) rejection handler when getErrorMessage is imported", () => { + cjsRuleTester.run("prefer-get-error-message-over-string", preferGetErrorMessageOverStringRule, { + valid: [], + invalid: [ + { + code: `const { getErrorMessage } = require("./error_helpers.cjs"); p.catch(err => log(\`failed: \${String(err)}\`));`, + errors: [ + { + messageId: "preferGetErrorMessage", + suggestions: [ + { + messageId: "replaceWithGetErrorMessage", + output: `const { getErrorMessage } = require("./error_helpers.cjs"); p.catch(err => log(\`failed: \${getErrorMessage(err)}\`));`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("valid: String(err) in a tagged template is not flagged", () => { + cjsRuleTester.run("prefer-get-error-message-over-string", preferGetErrorMessageOverStringRule, { + valid: [`const { getErrorMessage } = require("./error_helpers.cjs"); try { f(); } catch (err) { tag\`failed: \${String(err)}\`; }`], + invalid: [], + }); + }); +}); diff --git a/eslint-factory/src/rules/prefer-get-error-message-over-string.ts b/eslint-factory/src/rules/prefer-get-error-message-over-string.ts new file mode 100644 index 00000000000..809d27b8e35 --- /dev/null +++ b/eslint-factory/src/rules/prefer-get-error-message-over-string.ts @@ -0,0 +1,144 @@ +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}`); + +/** + * Returns true when the function node is an inline rejection handler passed to + * a promise method (.catch(fn) or .then(onFulfilled, onRejected)). + */ +function isInlineRejectionHandler(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): boolean { + const parent = node.parent; + if (!parent || parent.type !== AST_NODE_TYPES.CallExpression) return false; + const callee = parent.callee; + if (callee.type !== AST_NODE_TYPES.MemberExpression || callee.computed) return false; + const prop = callee.property; + if (prop.type !== AST_NODE_TYPES.Identifier) return false; + if (prop.name === "catch" && parent.arguments[0] === node) return true; + if (prop.name === "then" && parent.arguments[1] === node) return true; + return false; +} + +/** + * Returns true when the variable definition represents a caught error binding: + * - A try/catch clause with a simple identifier param (not destructured). + * - A parameter of an inline promise rejection handler (.catch(fn) / + * .then(_, fn)). + */ +function isCaughtErrorVariableDef(def: TSESLint.Scope.Definition): boolean { + if (def.type === "CatchClause") { + const catchNode = def.node as TSESTree.CatchClause; + return catchNode.param?.type === AST_NODE_TYPES.Identifier; + } + + if (def.type === "Parameter") { + const fn = def.node as TSESTree.Node; + if (fn.type !== AST_NODE_TYPES.ArrowFunctionExpression && fn.type !== AST_NODE_TYPES.FunctionExpression) { + return false; + } + return isInlineRejectionHandler(fn as TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression); + } + + return false; +} + +/** + * Returns the argument name when `node` is a `String()` call + * expression, or null otherwise. + */ +function getStringCallArgName(node: TSESTree.CallExpressionArgument): string | null { + if (node.type !== AST_NODE_TYPES.CallExpression) return null; + if (node.callee.type !== AST_NODE_TYPES.Identifier || node.callee.name !== "String") return null; + if (node.arguments.length !== 1) return null; + const arg = node.arguments[0]; + return arg.type === AST_NODE_TYPES.Identifier ? arg.name : null; +} + +export const preferGetErrorMessageOverStringRule = createRule({ + name: "prefer-get-error-message-over-string", + meta: { + type: "suggestion", + hasSuggestions: true, + docs: { + description: + "Prefer getErrorMessage(err) over String(err) when interpolating a caught error into a template literal, when getErrorMessage is already resolvable in scope. " + + "String(err) on an Error produces the redundant 'Error: message' prefix and does not sanitize GitHub's HTML error-page responses, " + + "while getErrorMessage(err) handles both correctly. Several actions/setup/js files already import getErrorMessage " + + "elsewhere yet still call String(err) at other call sites in the same file — this rule catches that inconsistency.", + }, + schema: [], + messages: { + preferGetErrorMessage: "Use getErrorMessage({{errorVar}}) instead of String({{errorVar}}) — getErrorMessage is already available in this scope and produces a cleaner, sanitized message for caught errors.", + replaceWithGetErrorMessage: "Replace String({{errorVar}}) with getErrorMessage({{errorVar}}).", + }, + }, + defaultOptions: [], + create(context) { + const sourceCode = context.sourceCode; + type Scope = ReturnType; + + function isDefinitionAvailableAtNode(definition: TSESLint.Scope.Definition, node: TSESTree.Node): boolean { + if (definition.type === "ImportBinding" || definition.type === "FunctionName") { + return true; + } + const definitionNode = definition.name ?? definition.node; + if (!definitionNode?.range || !node.range) return false; + return definitionNode.range[0] < node.range[0]; + } + + function hasResolvableLocalBinding(node: TSESTree.Node, name: string): boolean { + let scope: Scope | null = sourceCode.getScope(node); + while (scope) { + const variable = scope.set.get(name); + if (variable && variable.defs.some(def => isDefinitionAvailableAtNode(def, node))) { + return true; + } + scope = scope.upper; + } + return false; + } + + function resolvesToCaughtErrorVariable(expr: TSESTree.Identifier): boolean { + let scope: Scope | null = sourceCode.getScope(expr); + while (scope) { + const v = scope.set.get(expr.name); + if (v) { + return v.defs.some(isCaughtErrorVariableDef); + } + scope = scope.upper; + } + return false; + } + + return { + TemplateLiteral(node) { + // Tagged templates pass values to the tag function as-is; not string-coerced. + if (node.parent?.type === AST_NODE_TYPES.TaggedTemplateExpression) return; + + for (const expr of node.expressions) { + const errorVar = getStringCallArgName(expr); + if (!errorVar) continue; + if (expr.type !== AST_NODE_TYPES.CallExpression) continue; + const argNode = expr.arguments[0]; + if (argNode.type !== AST_NODE_TYPES.Identifier) continue; + if (!resolvesToCaughtErrorVariable(argNode)) continue; + if (!hasResolvableLocalBinding(node, "getErrorMessage")) continue; + + context.report({ + node: expr, + messageId: "preferGetErrorMessage", + data: { errorVar }, + suggest: [ + { + messageId: "replaceWithGetErrorMessage", + data: { errorVar }, + fix(fixer) { + return fixer.replaceText(expr, `getErrorMessage(${errorVar})`); + }, + }, + ], + }); + } + }, + }; + }, +}); From e93183931c44529fd72f48239817eb505e21e30c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:40:58 +0000 Subject: [PATCH 2/2] fix(eslint): restrict rejection handler check to first parameter only Add `isFirstParameterOf` helper that uses range containment so that non-first parameters (e.g. metadata in `.catch((err, metadata) => ...)`) are not treated as the rejection reason. Promise rejection callbacks only forward the reason as their first argument. Fixes the false-positive reported in review: #copilot-pull-request-reviewer Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- ...efer-get-error-message-over-string.test.ts | 7 +++++ .../prefer-get-error-message-over-string.ts | 27 ++++++++++++++++--- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/eslint-factory/src/rules/prefer-get-error-message-over-string.test.ts b/eslint-factory/src/rules/prefer-get-error-message-over-string.test.ts index 78a1a5f1003..4210daea901 100644 --- a/eslint-factory/src/rules/prefer-get-error-message-over-string.test.ts +++ b/eslint-factory/src/rules/prefer-get-error-message-over-string.test.ts @@ -81,4 +81,11 @@ describe("prefer-get-error-message-over-string", () => { invalid: [], }); }); + + it("valid: String(metadata) for a non-first rejection handler parameter is not flagged", () => { + cjsRuleTester.run("prefer-get-error-message-over-string", preferGetErrorMessageOverStringRule, { + valid: [`const { getErrorMessage } = require("./error_helpers.cjs"); p.catch((err, metadata) => log(\`info: \${String(metadata)}\`));`], + invalid: [], + }); + }); }); diff --git a/eslint-factory/src/rules/prefer-get-error-message-over-string.ts b/eslint-factory/src/rules/prefer-get-error-message-over-string.ts index 809d27b8e35..5dc5dcdeead 100644 --- a/eslint-factory/src/rules/prefer-get-error-message-over-string.ts +++ b/eslint-factory/src/rules/prefer-get-error-message-over-string.ts @@ -18,11 +18,30 @@ function isInlineRejectionHandler(node: TSESTree.ArrowFunctionExpression | TSEST return false; } +/** + * Returns true when the given identifier is the first formal parameter of fn, + * or is nested within it (e.g. the bound name of an assignment pattern or rest + * element at the first parameter position). + * Promise rejection callbacks forward the rejection reason only as their first + * argument; additional parameters are unrelated values and must not be flagged. + */ +function isFirstParameterOf(fn: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression, paramNameNode: TSESTree.Node): boolean { + if (fn.params.length === 0) return false; + const firstParam = fn.params[0]; + // Fast path: simple identifier param — the nodes are the same object. + if (firstParam === paramNameNode) return true; + // For assignment patterns (err = default) or rest params (...err) the + // identifier is nested inside the first param node; use range containment. + if (!paramNameNode.range || !firstParam.range) return false; + return paramNameNode.range[0] >= firstParam.range[0] && paramNameNode.range[1] <= firstParam.range[1]; +} + /** * Returns true when the variable definition represents a caught error binding: * - A try/catch clause with a simple identifier param (not destructured). - * - A parameter of an inline promise rejection handler (.catch(fn) / - * .then(_, fn)). + * - The *first* parameter of an inline promise rejection handler (.catch(fn) / + * .then(_, fn)). Only the first argument receives the rejection reason; + * additional parameters (e.g. metadata) are not error values. */ function isCaughtErrorVariableDef(def: TSESLint.Scope.Definition): boolean { if (def.type === "CatchClause") { @@ -35,7 +54,9 @@ function isCaughtErrorVariableDef(def: TSESLint.Scope.Definition): boolean { if (fn.type !== AST_NODE_TYPES.ArrowFunctionExpression && fn.type !== AST_NODE_TYPES.FunctionExpression) { return false; } - return isInlineRejectionHandler(fn as TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression); + const fnNode = fn as TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression; + if (!isInlineRejectionHandler(fnNode)) return false; + return isFirstParameterOf(fnNode, def.name); } return false;