From 3bb74826869d83ad01268f4fd0467810a31b502d Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sun, 26 Jul 2026 09:43:37 +0000 Subject: [PATCH 1/4] feat(eslint): add no-caught-error-interpolation rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new custom ESLint rule that detects bare caught-error variable interpolation in template literals (e.g. `...${err}...`). Problem: Directly interpolating a caught error variable in a template literal is unreliable: - For Error objects: produces redundant 'Error: message' prefix - For non-Error throws: produces useless '[object Object]' The rule flags bare Identifier expressions in template literals when the variable is a caught error param (try/catch, .catch(), or .then rejection handler). It suggests using getErrorMessage(err) if available or String(err) as an import-free alternative. Real instances found and flagged: - actions/setup/js/copilot_sdk_driver.cjs:87 - actions/setup/js/copilot_harness.cjs:850 Not flagged (intentionally excluded): - err.message, err.stack, err.toString() — covered by no-unsafe-catch-error-property - String(err), getErrorMessage(err) — already correct forms - Outer catch vars inside non-rejection function callbacks (false-positive guard) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- eslint-factory/eslint.config.cjs | 1 + eslint-factory/src/index.ts | 2 + .../no-caught-error-interpolation.test.ts | 241 ++++++++++++++++++ .../rules/no-caught-error-interpolation.ts | 171 +++++++++++++ 4 files changed, 415 insertions(+) create mode 100644 eslint-factory/src/rules/no-caught-error-interpolation.test.ts create mode 100644 eslint-factory/src/rules/no-caught-error-interpolation.ts diff --git a/eslint-factory/eslint.config.cjs b/eslint-factory/eslint.config.cjs index 63ce614345b..2b60553305f 100644 --- a/eslint-factory/eslint.config.cjs +++ b/eslint-factory/eslint.config.cjs @@ -41,6 +41,7 @@ module.exports = [ "gh-aw-custom/require-fs-io-try-catch": "warn", "gh-aw-custom/no-setfailed-then-exit-zero": "warn", "gh-aw-custom/no-err-stack-then-string-fallback": "warn", + "gh-aw-custom/no-caught-error-interpolation": "warn", "gh-aw-custom/require-fetch-try-catch": "warn", }, }, diff --git a/eslint-factory/src/index.ts b/eslint-factory/src/index.ts index 59114b0244c..fe84d35ab48 100644 --- a/eslint-factory/src/index.ts +++ b/eslint-factory/src/index.ts @@ -27,6 +27,7 @@ import { requireExecFileSyncTryCatchRule } from "./rules/require-execfilesync-tr import { requireFsIoTryCatchRule } from "./rules/require-fs-io-try-catch"; import { noSetFailedThenExitZeroRule } from "./rules/no-setfailed-then-exit-zero"; import { noErrStackThenStringFallbackRule } from "./rules/no-err-stack-then-string-fallback"; +import { noCaughtErrorInterpolationRule } from "./rules/no-caught-error-interpolation"; import { requireFetchTryCatchRule } from "./rules/require-fetch-try-catch"; const plugin = { @@ -64,6 +65,7 @@ const plugin = { "require-fs-io-try-catch": requireFsIoTryCatchRule, "no-setfailed-then-exit-zero": noSetFailedThenExitZeroRule, "no-err-stack-then-string-fallback": noErrStackThenStringFallbackRule, + "no-caught-error-interpolation": noCaughtErrorInterpolationRule, "require-fetch-try-catch": requireFetchTryCatchRule, }, }; diff --git a/eslint-factory/src/rules/no-caught-error-interpolation.test.ts b/eslint-factory/src/rules/no-caught-error-interpolation.test.ts new file mode 100644 index 00000000000..f5b8b8e3cca --- /dev/null +++ b/eslint-factory/src/rules/no-caught-error-interpolation.test.ts @@ -0,0 +1,241 @@ +import { RuleTester } from "eslint"; +import { describe, it } from "vitest"; +import { noCaughtErrorInterpolationRule } from "./no-caught-error-interpolation"; + +const cjsRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "commonjs", + }, +}); + +const esmRuleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: "module", + }, +}); + +describe("no-caught-error-interpolation", () => { + it("valid: non-caught variable interpolation is not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [ + `const err = new Error("test"); const msg = \`Error: \${err}\`;`, + `const name = "world"; const s = \`Hello \${name}\`;`, + `fetch(url).then(r => \`status: \${r}\`);`, + ], + invalid: [], + }); + }); + + it("valid: getErrorMessage(err) interpolation is not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [ + `try { f(); } catch (err) { console.log(\`failed: \${getErrorMessage(err)}\`); }`, + `try { f(); } catch (err) { core.setFailed(\`Error: \${getErrorMessage(err)}\`); }`, + `p.catch(err => { log(\`failed: \${getErrorMessage(err)}\`); });`, + ], + invalid: [], + }); + }); + + it("valid: String(err) interpolation is not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [ + `try { f(); } catch (err) { console.log(\`failed: \${String(err)}\`); }`, + `p.catch(err => \`error: \${String(err)}\`);`, + ], + invalid: [], + }); + }); + + it("valid: err.message interpolation is not flagged (covered by no-unsafe-catch-error-property)", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [ + `try { f(); } catch (err) { console.log(\`failed: \${err.message}\`); }`, + `try { f(); } catch (err) { console.log(\`stack: \${err.stack}\`); }`, + ], + invalid: [], + }); + }); + + it("valid: err.toString() interpolation is not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [`try { f(); } catch (err) { console.log(\`failed: \${err.toString()}\`); }`], + invalid: [], + }); + }); + + it("valid: outer catch variable not accessible inside nested non-rejection function", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [ + // The setTimeout callback is a sentinel: outer 'err' is not accessible + `try { f(); } catch (err) { setTimeout(function() { log(\`msg: \${err}\`); }, 0); }`, + ], + invalid: [], + }); + }); + + it("invalid: bare catch variable in template literal is flagged (try/catch)", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (err) { console.log(\`failed: \${err}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { console.log(\`failed: \${String(err)}\`); }`, + }, + ], + }, + ], + }, + { + code: `try { f(); } catch (error) { core.setFailed(\`Action failed: \${error}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "error" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "error" }, + output: `try { f(); } catch (error) { core.setFailed(\`Action failed: \${String(error)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: bare catch variable flagged with getErrorMessage suggestion when in scope", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `const { getErrorMessage } = require("./error_helpers.cjs"); try { f(); } catch (err) { console.log(\`failed: \${err}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "err" }, + output: `const { getErrorMessage } = require("./error_helpers.cjs"); try { f(); } catch (err) { console.log(\`failed: \${getErrorMessage(err)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: bare .catch() rejection handler variable is flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `fetch(url).catch(err => { log(\`network error: \${err}\`); });`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `fetch(url).catch(err => { log(\`network error: \${String(err)}\`); });`, + }, + ], + }, + ], + }, + { + code: `p.then(null, err => { log(\`rejected: \${err}\`); });`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `p.then(null, err => { log(\`rejected: \${String(err)}\`); });`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: multiple bare error interpolations in same template are all flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (err) { log(\`got \${err} at step, original: \${err}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { log(\`got \${String(err)} at step, original: \${err}\`); }`, + }, + ], + }, + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { log(\`got \${err} at step, original: \${String(err)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: ESM import context is also flagged", () => { + esmRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (e) { throw new Error(\`wrapper: \${e}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "e" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "e" }, + output: `try { f(); } catch (e) { throw new Error(\`wrapper: \${String(e)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); +}); diff --git a/eslint-factory/src/rules/no-caught-error-interpolation.ts b/eslint-factory/src/rules/no-caught-error-interpolation.ts new file mode 100644 index 00000000000..ae7e6fb66f4 --- /dev/null +++ b/eslint-factory/src/rules/no-caught-error-interpolation.ts @@ -0,0 +1,171 @@ +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}`); + +interface ErrorScope { + varName: string; + isSentinel: boolean; +} + +/** + * 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 `node` is a `TemplateLiteral` expression inside a + * `TemplateLiteral` expression. Used to identify `${someVar}` directly + * interpolated (as opposed to `${someVar.message}` or `${fn(someVar)}`). + * + * A "bare interpolation" is a `TSESTree.TemplateElement` expression where the + * expression is exactly an `Identifier` — no member access, no call, no + * unary/binary operation, no nullish coercion. + */ +function isBareIdentifierExpression(node: TSESTree.Expression): node is TSESTree.Identifier { + return node.type === AST_NODE_TYPES.Identifier; +} + +export const noCaughtErrorInterpolationRule = createRule({ + name: "no-caught-error-interpolation", + meta: { + type: "suggestion", + hasSuggestions: true, + docs: { + description: + "Disallow directly interpolating a caught error variable in a template literal (e.g. `${err}`). " + + "For Error objects this produces the redundant 'Error: message' prefix; for non-Error throws (plain objects, strings, etc.) " + + "it silently produces '[object Object]' or another useless string. " + + "Use getErrorMessage(err) for consistent, safe formatting, or String(err) when getErrorMessage is unavailable. " + + "Detected scopes: try/catch bindings, .catch(fn) inline callbacks, and .then(onFulfilled, onRejected) inline rejection handlers.", + }, + schema: [], + messages: { + bareErrorInterpolation: + "Directly interpolating caught error '{{errorVar}}' in a template literal is unsafe — " + + "for Error objects it produces 'Error: message' (redundant prefix); for non-Error throws it produces '[object Object]'. " + + "Use ${getErrorMessage({{errorVar}})} if it is available, or ${String({{errorVar}})} as an import-free alternative.", + useGetErrorMessage: "Replace \\${{{errorVar}}} with \\${getErrorMessage({{errorVar}})} — ensure getErrorMessage is imported from error_helpers.cjs.", + useStringFallback: "Replace \\${{{errorVar}}} with \\${String({{errorVar}})} — getErrorMessage is not in scope; String() is a safe import-free alternative.", + }, + }, + defaultOptions: [], + create(context) { + const sourceCode = context.sourceCode; + type SourceCodeScope = ReturnType; + + const scopeStack: ErrorScope[] = []; + + function getCaughtVarNames(): Set { + const names = new Set(); + for (let i = scopeStack.length - 1; i >= 0; i--) { + const scope = scopeStack[i]; + if (scope.isSentinel) break; + if (scope.varName) names.add(scope.varName); + } + return names; + } + + function enterFunction(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): void { + if (isInlineRejectionHandler(node)) { + const params = node.params; + if (params.length === 1 && params[0].type === AST_NODE_TYPES.Identifier) { + scopeStack.push({ varName: params[0].name, isSentinel: false }); + } else { + scopeStack.push({ varName: "", isSentinel: true }); + } + } else { + scopeStack.push({ varName: "", isSentinel: true }); + } + } + + function exitFunction(): void { + scopeStack.pop(); + } + + 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; + return definitionNode.range[0] < node.range[0]; + } + + function hasResolvableLocalBinding(node: TSESTree.Node, name: string): boolean { + let scope: SourceCodeScope | 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; + } + + return { + CatchClause(node) { + const param = node.param; + if (!param || param.type !== AST_NODE_TYPES.Identifier) { + scopeStack.push({ varName: "", isSentinel: true }); + } else { + scopeStack.push({ varName: param.name, isSentinel: false }); + } + }, + "CatchClause:exit"() { + scopeStack.pop(); + }, + + ArrowFunctionExpression: enterFunction, + "ArrowFunctionExpression:exit": exitFunction, + FunctionExpression: enterFunction, + "FunctionExpression:exit": exitFunction, + + TemplateLiteral(node) { + const caughtNames = getCaughtVarNames(); + if (caughtNames.size === 0) return; + + for (const expr of node.expressions) { + if (!isBareIdentifierExpression(expr)) continue; + if (!caughtNames.has(expr.name)) continue; + + const errorVar = expr.name; + const getErrorMessageAvailable = hasResolvableLocalBinding(node, "getErrorMessage"); + + context.report({ + node: expr, + messageId: "bareErrorInterpolation", + data: { errorVar }, + suggest: [ + getErrorMessageAvailable + ? ({ + messageId: "useGetErrorMessage" as const, + data: { errorVar }, + fix(fixer) { + return fixer.replaceText(expr, `getErrorMessage(${errorVar})`); + }, + } as const) + : ({ + messageId: "useStringFallback" as const, + data: { errorVar }, + fix(fixer) { + return fixer.replaceText(expr, `String(${errorVar})`); + }, + } as const), + ], + }); + } + }, + }; + }, +}); From 0b7f3f1f93d210547f5a00759737c4d54700fe73 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 26 Jul 2026 14:08:58 +0000 Subject: [PATCH 2/4] fix(eslint): address review feedback on no-caught-error-interpolation rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace stack-based scope tracking with ESLint scope analysis so closures retain access to outer catch bindings (fixes false negatives from non-rejection function sentinels) - Skip TaggedTemplateExpression nodes — tag functions receive raw values, not string-coerced output - Fix isBareIdentifierExpression JSDoc to accurately describe the function - Add isCaughtErrorVariableDef helper that correctly identifies catch clause bindings (simple identifier only, not destructured params) and inline rejection handler params via scope analysis - Guard definitionNode.range being undefined in isDefinitionAvailableAtNode - Remove FunctionDeclaration / FunctionExpression / ArrowFunctionExpression visitors — no longer needed with scope-based approach - Add tests: tagged template (valid), zero-param .catch() (valid), destructured catch params (valid), closures flagging outer catch var (invalid), ESM import getErrorMessage suggestion path (invalid) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .../no-caught-error-interpolation.test.ts | 105 +++++++++++++--- .../rules/no-caught-error-interpolation.ts | 117 ++++++++---------- 2 files changed, 143 insertions(+), 79 deletions(-) diff --git a/eslint-factory/src/rules/no-caught-error-interpolation.test.ts b/eslint-factory/src/rules/no-caught-error-interpolation.test.ts index f5b8b8e3cca..08a8f03e86a 100644 --- a/eslint-factory/src/rules/no-caught-error-interpolation.test.ts +++ b/eslint-factory/src/rules/no-caught-error-interpolation.test.ts @@ -19,11 +19,7 @@ const esmRuleTester = new RuleTester({ describe("no-caught-error-interpolation", () => { it("valid: non-caught variable interpolation is not flagged", () => { cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { - valid: [ - `const err = new Error("test"); const msg = \`Error: \${err}\`;`, - `const name = "world"; const s = \`Hello \${name}\`;`, - `fetch(url).then(r => \`status: \${r}\`);`, - ], + valid: [`const err = new Error("test"); const msg = \`Error: \${err}\`;`, `const name = "world"; const s = \`Hello \${name}\`;`, `fetch(url).then(r => \`status: \${r}\`);`], invalid: [], }); }); @@ -41,20 +37,14 @@ describe("no-caught-error-interpolation", () => { it("valid: String(err) interpolation is not flagged", () => { cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { - valid: [ - `try { f(); } catch (err) { console.log(\`failed: \${String(err)}\`); }`, - `p.catch(err => \`error: \${String(err)}\`);`, - ], + valid: [`try { f(); } catch (err) { console.log(\`failed: \${String(err)}\`); }`, `p.catch(err => \`error: \${String(err)}\`);`], invalid: [], }); }); it("valid: err.message interpolation is not flagged (covered by no-unsafe-catch-error-property)", () => { cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { - valid: [ - `try { f(); } catch (err) { console.log(\`failed: \${err.message}\`); }`, - `try { f(); } catch (err) { console.log(\`stack: \${err.stack}\`); }`, - ], + valid: [`try { f(); } catch (err) { console.log(\`failed: \${err.message}\`); }`, `try { f(); } catch (err) { console.log(\`stack: \${err.stack}\`); }`], invalid: [], }); }); @@ -66,11 +56,30 @@ describe("no-caught-error-interpolation", () => { }); }); - it("valid: outer catch variable not accessible inside nested non-rejection function", () => { + it("valid: tagged template expression is not flagged (tag receives raw values)", () => { cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { valid: [ - // The setTimeout callback is a sentinel: outer 'err' is not accessible - `try { f(); } catch (err) { setTimeout(function() { log(\`msg: \${err}\`); }, 0); }`, + // Tagged templates pass values to the tag as-is; no unsafe coercion occurs + `try { f(); } catch (err) { tag\`msg: \${err}\`; }`, + `p.catch(err => format\`error: \${err}\`);`, + ], + invalid: [], + }); + }); + + it("valid: .catch() with zero params is not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [`fetch(url).catch(() => { log("caught"); });`], + invalid: [], + }); + }); + + it("valid: destructured catch param elements are not flagged", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [ + // `message` is already a string extracted from the error object + `try { f(); } catch ({ message }) { log(\`failed: \${message}\`); }`, + `try { f(); } catch ({ message, code }) { log(\`\${code}: \${message}\`); }`, ], invalid: [], }); @@ -215,6 +224,70 @@ describe("no-caught-error-interpolation", () => { }); }); + it("invalid: closure inside catch body still flags outer catch variable", () => { + cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `try { f(); } catch (err) { setTimeout(function() { log(\`msg: \${err}\`); }, 0); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { setTimeout(function() { log(\`msg: \${String(err)}\`); }, 0); }`, + }, + ], + }, + ], + }, + { + code: `try { f(); } catch (err) { [1].forEach(x => { log(\`\${err}\`); }); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { [1].forEach(x => { log(\`\${String(err)}\`); }); }`, + }, + ], + }, + ], + }, + ], + }); + }); + + it("invalid: ESM import of getErrorMessage triggers getErrorMessage suggestion", () => { + esmRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { + valid: [], + invalid: [ + { + code: `import { getErrorMessage } from "./error_helpers.js"; try { f(); } catch (err) { log(\`failed: \${err}\`); }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useGetErrorMessage", + data: { errorVar: "err" }, + output: `import { getErrorMessage } from "./error_helpers.js"; try { f(); } catch (err) { log(\`failed: \${getErrorMessage(err)}\`); }`, + }, + ], + }, + ], + }, + ], + }); + }); + it("invalid: ESM import context is also flagged", () => { esmRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { valid: [], diff --git a/eslint-factory/src/rules/no-caught-error-interpolation.ts b/eslint-factory/src/rules/no-caught-error-interpolation.ts index ae7e6fb66f4..4319540c442 100644 --- a/eslint-factory/src/rules/no-caught-error-interpolation.ts +++ b/eslint-factory/src/rules/no-caught-error-interpolation.ts @@ -2,11 +2,6 @@ import { AST_NODE_TYPES, ESLintUtils, TSESLint, TSESTree } from "@typescript-esl const createRule = ESLintUtils.RuleCreator(name => `https://github.com/github/gh-aw/tree/main/eslint-factory#${name}`); -interface ErrorScope { - varName: string; - isSentinel: boolean; -} - /** * Returns true when the function node is an inline rejection handler passed to * a promise method (.catch(fn) or .then(onFulfilled, onRejected)). @@ -24,18 +19,43 @@ function isInlineRejectionHandler(node: TSESTree.ArrowFunctionExpression | TSEST } /** - * Returns true when `node` is a `TemplateLiteral` expression inside a - * `TemplateLiteral` expression. Used to identify `${someVar}` directly - * interpolated (as opposed to `${someVar.message}` or `${fn(someVar)}`). - * - * A "bare interpolation" is a `TSESTree.TemplateElement` expression where the - * expression is exactly an `Identifier` — no member access, no call, no - * unary/binary operation, no nullish coercion. + * Returns true when `node` is a bare `Identifier` expression — no member + * access, no call, no unary/binary operation, no nullish coercion. Used to + * identify direct `${someVar}` interpolations as opposed to safe forms such + * as `${someVar.message}` or `${fn(someVar)}`. */ function isBareIdentifierExpression(node: TSESTree.Expression): node is TSESTree.Identifier { return node.type === AST_NODE_TYPES.Identifier; } +/** + * Returns true when the variable definition represents a caught error binding + * that should not be interpolated directly: + * - 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 { + // try/catch binding — only flag simple identifier params, not destructured ones + // (e.g. `catch ({ message })` introduces string properties, not raw error objects) + if (def.type === "CatchClause") { + const catchNode = def.node as TSESTree.CatchClause; + return catchNode.param?.type === AST_NODE_TYPES.Identifier; + } + + // Inline rejection handler parameter (.catch(err => ...) / .then(_, err => ...)) + // def.node is the function node for Parameter definitions + 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; +} + export const noCaughtErrorInterpolationRule = createRule({ name: "no-caught-error-interpolation", meta: { @@ -62,47 +82,19 @@ export const noCaughtErrorInterpolationRule = createRule({ defaultOptions: [], create(context) { const sourceCode = context.sourceCode; - type SourceCodeScope = ReturnType; - - const scopeStack: ErrorScope[] = []; - - function getCaughtVarNames(): Set { - const names = new Set(); - for (let i = scopeStack.length - 1; i >= 0; i--) { - const scope = scopeStack[i]; - if (scope.isSentinel) break; - if (scope.varName) names.add(scope.varName); - } - return names; - } - - function enterFunction(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): void { - if (isInlineRejectionHandler(node)) { - const params = node.params; - if (params.length === 1 && params[0].type === AST_NODE_TYPES.Identifier) { - scopeStack.push({ varName: params[0].name, isSentinel: false }); - } else { - scopeStack.push({ varName: "", isSentinel: true }); - } - } else { - scopeStack.push({ varName: "", isSentinel: true }); - } - } - - function exitFunction(): void { - scopeStack.pop(); - } + 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: SourceCodeScope | null = sourceCode.getScope(node); + let scope: Scope | null = sourceCode.getScope(node); while (scope) { const variable = scope.set.get(name); if (variable && variable.defs.some(def => isDefinitionAvailableAtNode(def, node))) { @@ -114,30 +106,29 @@ export const noCaughtErrorInterpolationRule = createRule({ } return { - CatchClause(node) { - const param = node.param; - if (!param || param.type !== AST_NODE_TYPES.Identifier) { - scopeStack.push({ varName: "", isSentinel: true }); - } else { - scopeStack.push({ varName: param.name, isSentinel: false }); - } - }, - "CatchClause:exit"() { - scopeStack.pop(); - }, - - ArrowFunctionExpression: enterFunction, - "ArrowFunctionExpression:exit": exitFunction, - FunctionExpression: enterFunction, - "FunctionExpression:exit": exitFunction, - TemplateLiteral(node) { - const caughtNames = getCaughtVarNames(); - if (caughtNames.size === 0) return; + // Tagged templates pass values to the tag function as-is; they are not + // string-coerced by interpolation, so flagging them would be incorrect. + if (node.parent?.type === AST_NODE_TYPES.TaggedTemplateExpression) return; for (const expr of node.expressions) { if (!isBareIdentifierExpression(expr)) continue; - if (!caughtNames.has(expr.name)) continue; + + // Resolve the identifier through the full scope chain. This correctly + // handles closures: an identifier inside a nested function can still + // resolve to an outer catch binding. + let scope: Scope | null = sourceCode.getScope(expr); + let variable: TSESLint.Scope.Variable | null = null; + while (scope) { + const v = scope.set.get(expr.name); + if (v) { + variable = v; + break; + } + scope = scope.upper; + } + + if (!variable || !variable.defs.some(isCaughtErrorVariableDef)) continue; const errorVar = expr.name; const getErrorMessageAvailable = hasResolvableLocalBinding(node, "getErrorMessage"); From f149cafd8d5f75e1aee405652af1f1b11bcd9829 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 26 Jul 2026 14:10:35 +0000 Subject: [PATCH 3/4] fix: clarify range guard comment in isDefinitionAvailableAtNode Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- eslint-factory/src/rules/no-caught-error-interpolation.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/eslint-factory/src/rules/no-caught-error-interpolation.ts b/eslint-factory/src/rules/no-caught-error-interpolation.ts index 4319540c442..9d7c6c3a6b7 100644 --- a/eslint-factory/src/rules/no-caught-error-interpolation.ts +++ b/eslint-factory/src/rules/no-caught-error-interpolation.ts @@ -89,6 +89,7 @@ export const noCaughtErrorInterpolationRule = createRule({ return true; } const definitionNode = definition.name ?? definition.node; + // range is absent on synthetic/virtual nodes; treat as unavailable rather than throwing if (!definitionNode?.range || !node.range) return false; return definitionNode.range[0] < node.range[0]; } From 02d39b5de590d6d5537a5d444518d93674c3f0d4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 26 Jul 2026 15:10:39 +0000 Subject: [PATCH 4/4] test(eslint): add FunctionDeclaration inside catch coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FunctionDeclaration nodes inside catch blocks correctly resolve to the catch binding via scope analysis — err is still the raw caught error. Add a test case to document this as an invalid (flagged) pattern. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .../no-caught-error-interpolation.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/eslint-factory/src/rules/no-caught-error-interpolation.test.ts b/eslint-factory/src/rules/no-caught-error-interpolation.test.ts index 08a8f03e86a..71b3e538089 100644 --- a/eslint-factory/src/rules/no-caught-error-interpolation.test.ts +++ b/eslint-factory/src/rules/no-caught-error-interpolation.test.ts @@ -260,6 +260,24 @@ describe("no-caught-error-interpolation", () => { }, ], }, + { + // FunctionDeclaration inside catch — err resolves to the catch binding, + // so the interpolation is unsafe and must be flagged + code: `try { f(); } catch (err) { function helper() { return \`msg: \${err}\`; } }`, + errors: [ + { + messageId: "bareErrorInterpolation", + data: { errorVar: "err" }, + suggestions: [ + { + messageId: "useStringFallback", + data: { errorVar: "err" }, + output: `try { f(); } catch (err) { function helper() { return \`msg: \${String(err)}\`; } }`, + }, + ], + }, + ], + }, ], }); });