Skip to content

eslint-factory: extend no-json-stringify-error to cover .then(_, onRejected) and pin scope boundaries #43724

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 6 commits into from
Jul 6, 2026
+143 −9
Merged

eslint-factory: extend no-json-stringify-error to cover .then(_, onRejected) and pin scope boundaries #43724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
182d65e
Initial plan
Copilot Jul 6, 2026
ad87461
chore: initial plan for no-json-stringify-error scope extension
Copilot Jul 6, 2026
eea6eb7
feat(eslint-factory): extend no-json-stringify-error to cover .then(_…
Copilot Jul 6, 2026
befce33
Merge branch 'main' into copilot/eslint-factory-fix-no-json-stringify…
pelikhan Jul 6, 2026
f6e9018
Merge branch 'main' into copilot/eslint-factory-fix-no-json-stringify…
github-actions[bot] Jul 6, 2026
c916cb0
Address eslint-factory review feedback
Copilot Jul 6, 2026
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
19 changes: 19 additions & 0 deletions eslint-factory/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,25 @@ This project hosts custom ESLint linters for `/actions/setup/js`.

## Rules

### `no-json-stringify-error`

Disallow `JSON.stringify()` on caught error variables. `Error` properties (`message`, `stack`, etc.) are non-enumerable, so `JSON.stringify(err)` silently produces `{}`.

**Detected scopes:**
- `try { } catch (err) { }` — catch-clause bindings.
- `p.catch(err => ...)` — inline arrow or function callbacks passed as the first argument to `.catch()`.
- `p.then(onFulfilled, err => ...)` — inline rejection handlers passed as the **second** argument to `.then()`, which are semantically equivalent to `.catch()`.

**Out of scope:** named-reference handlers such as `p.catch(handler)` or `p.then(ok, handler)` — the rule does not follow references across files or scopes.

Flagged forms:
- `JSON.stringify(err)` where `err` is a catch-clause or inline rejection-handler parameter.
- `JSON.stringify(err, null, 2)` (with replacer/space arguments).

Safe alternatives:
- `getErrorMessage(err)` from `error_helpers.cjs` (auto-suggested fix).
- `JSON.stringify({ message: err.message, stack: err.stack })` — explicitly serializing safe string properties.

### `prefer-number-isnan`

Prefer `Number.isNaN()` over global `isNaN()` to avoid silent coercion of non-numeric inputs.
Expand Down
101 changes: 101 additions & 0 deletions eslint-factory/src/rules/no-json-stringify-error.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,91 @@ describe("no-json-stringify-error", () => {
});
});

it("valid: p.catch(handler) named-reference is not flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [`const handler = err => JSON.stringify(err); p.catch(handler);`],
invalid: [],
});
});

it("valid: p.then(ok, handler) named-reference rejection handler is not flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [`function onRejected(err) { JSON.stringify(err); } p.then(ok, onRejected);`],
invalid: [],
});
});

it("invalid: JSON.stringify(err) in .then() second-argument rejection handler (arrow) is flagged", () => {

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The two .then() invalid tests use cjsRuleTester only — there is no ESM-mode counterpart, unlike the existing try/catch and .catch() tests which exercise both testers. If the ESM parser resolves method calls differently (e.g., in strict mode modules) the gap would go undetected.

💡 Suggested addition

Add a minimal ESM-mode invalid test alongside the existing it("invalid: works with ES module syntax") block:

it("invalid: JSON.stringify(err) in .then() rejection handler works with ES module syntax", () => {
  esmRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
    valid: [],
    invalid: [
      {
        code: `p.then(null, err => console.error(JSON.stringify(err)));`,
        errors: [{ messageId: "jsonStringifyError", data: { errorVar: "err" } }],
      },
    ],
  });
});

@copilot please address this.

cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `p.then(result => result, err => core.error(JSON.stringify(err)));`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "err" },
output: `p.then(result => result, err => core.error(getErrorMessage(err)));`,
},
],
},
],
},
{
code: `p.then(null, err => core.error(JSON.stringify(err, null, 2)));`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "err" },
output: `p.then(null, err => core.error(getErrorMessage(err)));`,
},
],
},
],
},
],
});
});

it("invalid: JSON.stringify(err) in .then() second-argument rejection handler (function) is flagged", () => {
cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing test: JSON.stringify(err, null, 2) with extra args in .then() rejection handler — the replacer/space-argument variant is already covered for try/catch, but not for the new .then() path.

💡 Suggested fix

Add a case inside the existing .then() invalid test block (or a separate small test):

{
  code: `p.then(null, err => core.error(JSON.stringify(err, null, 2)));`,
  errors: [{
    messageId: "jsonStringifyError",
    data: { errorVar: "err" },
    suggestions: [{ messageId: "useGetErrorMessage", data: { errorVar: "err" }, output: `p.then(null, err => core.error(getErrorMessage(err)));` }],
  }],
},

The rule targets JSON.stringify by matching any call where the first argument is the error variable, so extra arguments are already handled correctly — but without a test, that assumption is unverified and future refactors targeting arguments.length checks have no safety net.

valid: [],
invalid: [
{
code: `p.then(null, function(err) { core.error(JSON.stringify(err)); });`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "err" },
output: `p.then(null, function(err) { core.error(getErrorMessage(err)); });`,
},
],
},
],
},
],
});
});

it("valid: first argument of .then() (onFulfilled) is not treated as a rejection handler", () => {

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] The onFulfilled non-flagging test uses a single parameter named result — it does not cover the case where the first-arg callback happens to use err as a parameter name (e.g., p.then(err => JSON.stringify(err))). This is the most likely false-positive scenario and should be a dedicated test.

💡 Suggested test 
it("valid: p.then(err => JSON.stringify(err)) — first argument is onFulfilled, not flagged", () => {
  cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
    valid: [`p.then(err => core.info(JSON.stringify(err)));`],
    invalid: [],
  });
});

Without this, a future refactor that accidentally checks argument index >= 1 instead of === 1 would go undetected.

@copilot please address this.

cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [`p.then(result => JSON.stringify(result));`, `p.then(function(result) { JSON.stringify(result); });`, `p.then(err => core.info(JSON.stringify(err)));`],
invalid: [],
});

@github-actions github-actions Bot Jul 6, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing ESM-mode test for .then(_, fn) rejection handler — the new detection path is validated only under CJS, leaving ESM coverage absent while try/catch already has an esmRuleTester case.

💡 Suggested fix

Add an ESM invalid case after the existing CJS tests:

it("invalid: JSON.stringify(err) in .then() rejection handler is flagged (ESM)", () => {
  esmRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
    valid: [],
    invalid: [
      {
        code: `p.then(ok, err => core.error(JSON.stringify(err)));`,
        errors: [{ messageId: "jsonStringifyError", data: { errorVar: "err" }, suggestions: [{ messageId: "useGetErrorMessage", data: { errorVar: "err" }, output: `p.then(ok, err => core.error(getErrorMessage(err)));` }] }],
      },
    ],
  });
});

Parser configuration differences between module and script mode can affect identifier binding. The suite already has esmRuleTester coverage for try/catch and .catch() — this new .then() path deserves the same treatment. Without it, a regression in ESM mode would pass CI silently.

});

it("invalid: works with ES module syntax", () => {
esmRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
Expand All @@ -203,6 +288,22 @@ describe("no-json-stringify-error", () => {
},
],
},
{
code: `p.then(ok, err => console.error(JSON.stringify(err)));`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [
{
messageId: "useGetErrorMessage",
data: { errorVar: "err" },
output: `p.then(ok, err => console.error(getErrorMessage(err)));`,
},
],
},
],
},
],
});
});
Expand Down
32 changes: 23 additions & 9 deletions eslint-factory/src/rules/no-json-stringify-error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,28 @@ interface ErrorScope {
}

/**
* 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.
* Returns true when the function node is an inline rejection handler passed to
* a promise method:
* - First argument of `.catch(fn)` — the canonical rejection handler.
* - Second argument of `.then(onFulfilled, onRejected)` — the rejection-handler
* position, analogous to `.catch(fn)`.
*
* Named-reference handlers (for example `p.catch(handler)`) are intentionally
* out of scope because the rule cannot statically follow the reference to
* inspect its parameter list or body.
*/
function isCatchCallback(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): boolean {
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;
return prop.type === AST_NODE_TYPES.Identifier && prop.name === "catch" && parent.arguments[0] === node;
if (prop.type !== AST_NODE_TYPES.Identifier) return false;
// .catch(fn) — first argument
if (prop.name === "catch" && parent.arguments[0] === node) return true;
// .then(onFulfilled, onRejected) — second argument is the rejection handler
if (prop.name === "then" && parent.arguments[1] === node) return true;
return false;
}

export const noJsonStringifyErrorRule = createRule({
Expand All @@ -27,7 +38,10 @@ export const noJsonStringifyErrorRule = createRule({
type: "problem",
hasSuggestions: true,
docs: {
description: "Disallow JSON.stringify() on caught error variables — Error properties (message, stack, etc.) are non-enumerable and produce {} silently",
description:
"Disallow JSON.stringify() on caught error variables — Error properties (message, stack, etc.) are non-enumerable and produce {} silently. " +
"Detected scopes: try/catch bindings, .catch(fn) inline callbacks, and .then(onFulfilled, onRejected) inline rejection handlers. " +
"Named-reference handlers (e.g. p.catch(handler)) are out of scope because the rule cannot statically follow the reference.",
},
schema: [],
messages: {
Expand Down Expand Up @@ -55,7 +69,7 @@ export const noJsonStringifyErrorRule = createRule({
}

function enterFunction(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): void {
if (isCatchCallback(node)) {
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 });
Expand All @@ -64,7 +78,7 @@ export const noJsonStringifyErrorRule = createRule({
scopeStack.push({ varName: "", isSentinel: true });
}
} else {
// Non-.catch() function: sentinel to avoid false positives from shadowed names
// Non-rejection-handler function: sentinel to avoid false positives from shadowed names
scopeStack.push({ varName: "", isSentinel: true });
}
}
Expand All @@ -87,7 +101,7 @@ export const noJsonStringifyErrorRule = createRule({
scopeStack.pop();
},

// Track .catch() callback parameters
// Track inline rejection-handler parameters from .catch(fn) and .then(_, fn)
ArrowFunctionExpression: enterFunction,
"ArrowFunctionExpression:exit": exitFunction,
FunctionExpression: enterFunction,
Expand Down
Loading