Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions eslint-factory/eslint.config.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ module.exports = [
},
rules: {
"gh-aw-custom/no-core-setoutput-non-string": "warn",
"gh-aw-custom/no-json-stringify-error": "warn",
"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",
Expand Down
2 changes: 2 additions & 0 deletions eslint-factory/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { noCoreSetOutputNonStringRule } from "./rules/no-core-setoutput-non-string";
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";
Expand All @@ -15,6 +16,7 @@ const plugin = {
},
rules: {
"no-core-setoutput-non-string": noCoreSetOutputNonStringRule,
"no-json-stringify-error": noJsonStringifyErrorRule,
"no-unsafe-catch-error-property": noUnsafeCatchErrorPropertyRule,
"no-unsafe-promise-catch-error-property": noUnsafePromiseCatchErrorPropertyRule,
"prefer-get-error-message": preferGetErrorMessageRule,
Expand Down
233 changes: 233 additions & 0 deletions eslint-factory/src/rules/no-json-stringify-error.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,233 @@
import { RuleTester } from "eslint";
import { describe, it } from "vitest";
import { noJsonStringifyErrorRule } from "./no-json-stringify-error";

const cjsRuleTester = new RuleTester({
languageOptions: {
ecmaVersion: 2022,
sourceType: "commonjs",
},
});

const esmRuleTester = new RuleTester({
languageOptions: {
ecmaVersion: 2022,
sourceType: "module",
},
});

describe("no-json-stringify-error", () => {
it("valid: JSON.stringify on a non-caught variable is not flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [`const obj = { a: 1 }; JSON.stringify(obj);`, `JSON.stringify({ message: "hello" });`, `JSON.stringify("a string");`, `const data = fetchData(); JSON.stringify(data);`],
invalid: [],
});
});

it("valid: JSON.stringify on a non-error variable inside a catch block is not flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [`try { f(); } catch (err) { const data = { a: 1 }; JSON.stringify(data); }`, `try { f(); } catch (err) { JSON.stringify(someOtherVar); }`, `try { f(); } catch (err) { JSON.stringify({ message: err.message }); }`],
invalid: [],
});
});

it("valid: JSON.stringify with explicit error properties is not flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [`try { f(); } catch (err) { JSON.stringify({ message: err.message, stack: err.stack }); }`, `try { f(); } catch (err) { JSON.stringify({ error: String(err) }); }`],
invalid: [],
});
});

it("valid: JSON.stringify on catch param that shadows outer scope is not flagged outside the catch", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [
`const err = { a: 1 }; try { f(); } catch (err) { } JSON.stringify(err);`,
`try { f(); } catch (err) { [1].forEach(function(err) { JSON.stringify(err); }); }`,
`try { f(); } catch (err) { items.map(err => JSON.stringify(err)); }`,
],
invalid: [],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Missing a test for the shadowing false-positive scenario: a non-.catch() callback (e.g. Array.map) inside a catch block where the callback parameter has the same name as the caught error.

💡 Suggested test to add to the valid set
// valid: non-.catch() callback inside a catch that shadows the catch param
`try { f(); } catch (err) { items.map(err => JSON.stringify(err)); }`,

This exercises the boundary between sentinel and non-sentinel scope frames and would fail if getCaughtVarNames leaks outer catch names through a sentinel entry.

@copilot please address this.

});
});

it("valid: JSON.stringify on promise .catch() non-error param is not flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [`p.catch(function(err) { JSON.stringify(otherVar); })`, `p.catch(err => JSON.stringify(notErr))`],
invalid: [],
});
});

it("valid: bare catch {} without binding is not flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [`try { f(); } catch { JSON.stringify(someObj); }`],
invalid: [],
});
});

it("invalid: JSON.stringify(err) in catch block is flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `try { f(); } catch (err) { core.error(JSON.stringify(err)); }`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "err" },
output: `try { f(); } catch (err) { core.error(getErrorMessage(err)); }`,
},
],
},
],
},
],
});
});

it("invalid: JSON.stringify(error, null, 2) in catch block is flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Each test case creates a new RuleTester.run() call inside its own it() block. This is structurally fine, but it departs from the pattern used in all existing rule test files (e.g. no-unsafe-catch-error-property.test.ts), which call ruleTester.run() once with a combined valid/invalid array. The current structure makes it harder to see the full contract at a glance and causes the rule name to be repeated 11 times.

💡 Suggestion

Consolidate into a single cjsRuleTester.run() call and a single esmRuleTester.run() call, with all valid/invalid cases gathered together. Use comments to group the cases:

cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
  valid: [
    // non-caught variables
    `const obj = { a: 1 }; JSON.stringify(obj);`,
    // catch-block non-error
    `try { f(); } catch (err) { JSON.stringify(someOtherVar); }`,
    // ... etc
  ],
  invalid: [
    // catch block
    { code: `try { f(); } catch (err) { core.error(JSON.stringify(err)); }`, errors: [...] },
    // ... etc
  ],
});

This aligns the test structure with the established codebase convention and makes the rule's full input/output contract immediately scannable.

@copilot please address this.

invalid: [
{
code: `try { f(); } catch (error) { core.error(\`details: \${JSON.stringify(error, null, 2)}\`); }`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "error" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "error" },
output: `try { f(); } catch (error) { core.error(\`details: \${getErrorMessage(error)}\`); }`,
},
],
},
],
},
],
});
});

it("invalid: JSON.stringify(err) in promise .catch() arrow callback is flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `p.catch(err => core.error(JSON.stringify(err)));`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "err" },
output: `p.catch(err => core.error(getErrorMessage(err)));`,
},
],
},
],
},
],
});
});

it("invalid: JSON.stringify(err) in promise .catch() function callback is flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `p.catch(function(err) { core.error(JSON.stringify(err)); });`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "err" },
output: `p.catch(function(err) { core.error(getErrorMessage(err)); });`,
},
],
},
],
},
],
});
});

it("invalid: nested catch — each catch variable is tracked independently", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `try { f(); } catch (outer) { try { g(); } catch (inner) { } core.error(JSON.stringify(outer)); }`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "outer" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "outer" },
output: `try { f(); } catch (outer) { try { g(); } catch (inner) { } core.error(getErrorMessage(outer)); }`,
},
],
},
],
},
],
});
});

it("invalid: works with ES module syntax", () => {
esmRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `try { fetch(url); } catch (e) { console.error(JSON.stringify(e)); }`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "e" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "e" },
output: `try { fetch(url); } catch (e) { console.error(getErrorMessage(e)); }`,
},
],
},
],
},
],
});
});

it("invalid: inner catch variable also flagged when outer has JSON.stringify", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `try { f(); } catch (outer) { try { g(); } catch (inner) { core.error(JSON.stringify(inner)); } }`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "inner" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "inner" },
output: `try { f(); } catch (outer) { try { g(); } catch (inner) { core.error(getErrorMessage(inner)); } }`,
},
],
},
],
},
],
});
});
});
131 changes: 131 additions & 0 deletions eslint-factory/src/rules/no-json-stringify-error.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
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}`);

interface ErrorScope {
varName: string;
isSentinel: boolean;
}

/**
* Returns true when the function is passed directly as the first argument to a
* `.catch()` call. Named-reference handlers (for example `p.catch(handler)`)
* are intentionally out of scope.
*/
function isCatchCallback(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;
return prop.type === AST_NODE_TYPES.Identifier && prop.name === "catch" && parent.arguments[0] === node;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: also check .then(null, callback) and .finally() patterns

isCatchCallback currently only checks for .catch(handler). In practice, rejected promise handlers also appear as the second argument to .then():

p.then(null, function(err) { JSON.stringify(err); }); // not caught by current rule

This is a lower-priority gap — the current scope catches the most common pattern — but worth noting for future completeness. Adding a check for prop.name === "then" && parent.arguments[1] === node would cover it.

@copilot please address this.

}

export const noJsonStringifyErrorRule = createRule({
name: "no-json-stringify-error",
meta: {
type: "problem",
hasSuggestions: true,
docs: {
description: "Disallow JSON.stringify() on caught error variables — Error properties (message, stack, etc.) are non-enumerable and produce {} silently",
},
schema: [],
messages: {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] isCatchCallback only matches calls where the .catch method is on the immediate parent CallExpression and the arrow/function is argument index 0. This correctly handles p.catch(err => ...) but misses p.catch(handler) where handler is a reference. That's an intentional limitation — but it also won't catch Promise.all([...]).catch(err => ...) since the callee is still a MemberExpression with property catch. Is the property name check prop.name === "catch" meant to match any object's .catch() (including custom thenables)? If so, this is intentional and should be documented.

💡 Suggestion

Add a short JSDoc comment to isCatchCallback explaining its intentional scope:

/**
 * Returns true if the function is passed directly as the first argument to any
 * `.catch()` method call (Promise.catch or custom thenables). Named-reference
 * handlers (p.catch(handler)) are out of scope.
 */
function isCatchCallback(...): boolean {

This makes the tradeoff explicit for future maintainers.

@copilot please address this.

jsonStringifyError:
"JSON.stringify({{errorVar}}) produces {} for Error objects — Error properties (message, stack, etc.) are non-enumerable. Prefer getErrorMessage({{errorVar}}) from error_helpers.cjs or explicitly serialize a guarded value after narrowing it.",
useGetErrorMessage: "Replace with getErrorMessage({{errorVar}}) — ensure getErrorMessage is imported from error_helpers.cjs.",
},
},
defaultOptions: [],
create(context) {
// Stack tracking caught error variable names.
// Each scope entry holds varName (empty string for sentinel) and isSentinel flag.
const scopeStack: ErrorScope[] = [];

function getCaughtVarNames(): Set<string> {
const names = new Set<string>();
// Walk from the innermost active scope outward and stop at the first
// sentinel so non-.catch() callbacks cannot see shadowed catch vars.
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 (isCatchCallback(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 {
// No-param or destructuring: push sentinel so outer frames are not affected
scopeStack.push({ varName: "", isSentinel: true });
}
} else {
// Non-.catch() function: sentinel to avoid false positives from shadowed names
scopeStack.push({ varName: "", isSentinel: true });
}
Comment on lines +66 to +69
}

function exitFunction(): void {
scopeStack.pop();
}

return {
// Track catch clause parameters
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();
},

// Track .catch() callback parameters
ArrowFunctionExpression: enterFunction,
"ArrowFunctionExpression:exit": exitFunction,
FunctionExpression: enterFunction,
"FunctionExpression:exit": exitFunction,

// Detect JSON.stringify(caughtErrorVar, ...)
CallExpression(node) {
const caughtNames = getCaughtVarNames();
if (caughtNames.size === 0) return;

const callee = node.callee;
if (callee.type !== AST_NODE_TYPES.MemberExpression) return;
if (callee.computed) return;
const obj = callee.object;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The CallExpression check if (scopeStack.length === 0) return is a fast-path guard, but it will also skip checks when a .catch() callback has a sentinel on top (e.g. the callback had no parameter). The real guard should be getCaughtVarNames().size === 0 only — and since that already follows, the length === 0 guard is redundant noise rather than a meaningful optimisation.

💡 Suggestion

Remove the scopeStack.length === 0 guard — the subsequent caughtNames.size === 0 already short-circuits correctly. Or, at minimum, add a comment explaining why the length guard is meaningful despite the sentinel case.

// before: two guards; the first is redundant because getCaughtVarNames handles empty stacks
CallExpression(node) {
  if (scopeStack.length === 0) return;         // <- remove or document
  const caughtNames = getCaughtVarNames();
  if (caughtNames.size === 0) return;
  ...
}

@copilot please address this.

const prop = callee.property;
if (obj.type !== AST_NODE_TYPES.Identifier || obj.name !== "JSON") return;
if (prop.type !== AST_NODE_TYPES.Identifier || prop.name !== "stringify") return;

const firstArg = node.arguments[0];
if (!firstArg || firstArg.type !== AST_NODE_TYPES.Identifier) return;
if (!caughtNames.has(firstArg.name)) return;

const errorVar = firstArg.name;
context.report({
node,
messageId: "jsonStringifyError",
data: { errorVar },
suggest: [
{
messageId: "useGetErrorMessage" as const,
data: { errorVar },
fix(fixer) {
return fixer.replaceText(node, `getErrorMessage(${errorVar})`);
},
},

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The auto-fix suggestion replaces JSON.stringify(err) with getErrorMessage(err) but does not import getErrorMessage. If the consumer file does not already import it, the suggested fix produces broken code. The sibling rule prefer-get-error-message handles this the same way (no import insertion), but it's worth calling out in the suggestion message text to avoid confusion.

💡 Suggestion

Update useGetErrorMessage message to make the import requirement explicit:

useGetErrorMessage: "Replace with getErrorMessage({{errorVar}}) — ensure getErrorMessage is imported from error_helpers.cjs before applying.",

This matches the wording used in the existing prefer-get-error-message rule and sets consistent expectations.

@copilot please address this.

],
});
},
};
},
});
Loading