[eslint-miner] feat(eslint): add no-json-stringify-error rule#43523
Conversation
JSON.stringify(errorVariable) silently produces {} for Error objects
because message, stack, name, code, and other useful properties are
non-enumerable. This is a real footgun in catch blocks and .catch()
callbacks: callers expect serialized diagnostic info but get an empty
object.
Evidence from actions/setup/js corpus:
- expired_entity_cleanup_helpers.cjs:134 — JSON.stringify(error, null, 2)
inside a catch block passed to core.error(); produces {} silently.
The rule flags JSON.stringify() calls where the first argument is an
identifiable caught-error variable (CatchClause param or .catch()
callback param). It offers a suggestion to replace with getErrorMessage().
Valid patterns NOT flagged:
- JSON.stringify({ message: err.message, stack: err.stack }) — explicit
- JSON.stringify(nonErrorVar) inside catch — different variable name
- JSON.stringify(err) outside any catch or .catch() scope
- Bare catch {} without a binding (no trackable variable name)
Validated with:
- npm run build (tsc, zero errors)
- npm run lint:setup-js (1 real hit, 0 false positives)
- vitest: all 140 tests pass (11 files)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43523 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
|
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a new gh-aw-custom/no-json-stringify-error ESLint rule to catch the common footgun where JSON.stringify(err) inside catch / .catch() silently logs {} for Error objects, and wires it into the plugin and default config for *.cjs files.
Changes:
- Implement
no-json-stringify-errorrule with a suggestion to replaceJSON.stringify(err)withgetErrorMessage(err). - Add Vitest/RuleTester coverage for valid/invalid scenarios across
catchand.catch()contexts. - Register and enable the rule in the ESLint plugin and
eslint-factoryconfig.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/no-json-stringify-error.ts | New rule implementation detecting JSON.stringify() on caught error identifiers and offering a suggestion fix. |
| eslint-factory/src/rules/no-json-stringify-error.test.ts | New tests exercising rule behavior for catch blocks and promise .catch() callbacks. |
| eslint-factory/src/index.ts | Registers the new rule in the exported plugin rules map. |
| eslint-factory/eslint.config.cjs | Enables the new rule as "warn" for *.cjs files. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Low
| } else { | ||
| // Non-.catch() function: sentinel to avoid false positives from shadowed names | ||
| scopeStack.push({ varName: "", isSentinel: true }); | ||
| } |
| messages: { | ||
| jsonStringifyError: | ||
| "JSON.stringify({{errorVar}}) produces {} for Error objects — Error properties (message, stack, etc.) are non-enumerable. Use getErrorMessage({{errorVar}}) or serialize explicitly: JSON.stringify({ message: {{errorVar}}.message, stack: {{errorVar}}.stack }).", | ||
| useGetErrorMessage: "Replace with getErrorMessage({{errorVar}}) for safe error message extraction.", | ||
| }, |
| valid: [ | ||
| `const err = { a: 1 }; try { f(); } catch (err) { } JSON.stringify(err);`, | ||
| ], |
|
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: no-json-stringify-error ESLint rule
Overall: The rule addresses a real, well-documented footgun — JSON.stringify(error) silently producing {}. The implementation, test coverage, and PR description are high quality. One correctness bug needs to be fixed before merging.
🔴 Blocking: getCaughtVarNames ignores sentinel boundaries (false positives)
The sentinel mechanism is designed to prevent outer catch variable names from leaking into unrelated function scopes (e.g. a forEach callback). But getCaughtVarNames() iterates all stack entries and simply skips sentinel entries — it does not stop collection when it encounters a sentinel. This means a variable name registered in an outer catch scope leaks through the sentinel into an inner non-.catch() callback.
Reproduction:
try { f(); } catch (err) {
[1, 2].forEach(function(err) { JSON.stringify(err); }); // ← will be false-positively flagged
}Stack at the JSON.stringify call:
{ varName: "err", isSentinel: false }(outer catch){ varName: "", isSentinel: true }(forEach sentinel)
getCaughtVarNames returns {"err"} because it doesn't break at the sentinel. The comment says this design is meant to avoid false positives, but the implementation doesn't deliver that.
Fix — scan from top of stack, stop at the first sentinel:
function getCaughtVarNames(): Set<string> {
const names = new Set<string>();
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;
}i️ Non-blocking: .then(null, rejectionHandler) gap
isCatchCallback only matches .catch(fn). The second-argument form .then(null, fn) is not detected. Low priority but worth a follow-up.
✅ Strengths
- Rule type (
problem),hasSuggestions, and message text are appropriate - 11 test cases cover the main valid/invalid patterns well
- The suggestion fixer correctly scopes the fix to the whole
JSON.stringify(...)call expression - Registration in
index.tsandeslint.config.cjsis clean and alphabetically ordered
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 109 AIC · ⌖ 6.31 AIC · ⊞ 4.9K
| function getCaughtVarNames(): Set<string> { | ||
| const names = new Set<string>(); | ||
| for (const scope of scopeStack) { | ||
| if (!scope.isSentinel && scope.varName) { |
There was a problem hiding this comment.
Bug: sentinel boundaries are not respected — outer catch vars leak through
The current loop iterates all entries in scopeStack and only skips the sentinel entry itself (!scope.isSentinel). It does not stop at the topmost sentinel, so names from outer catch scopes remain visible inside non-.catch() callbacks.
Consider this valid (non-error) pattern that will be false-positively flagged:
try { f(); } catch (err) {
[1, 2].forEach(function(err) { JSON.stringify(err); });
}Stack when JSON.stringify is reached:
{ varName: "err", isSentinel: false }— the catch variable{ varName: "", isSentinel: true }— the forEach sentinel
getCaughtVarNames still collects "err" from entry 1, causing a spurious flag.
The fix: scan from the top of the stack and break at the first sentinel:
function getCaughtVarNames(): Set<string> {
const names = new Set<string>();
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;
}Add matching test cases:
valid: [
`try { f(); } catch (err) { [1].forEach(function(err) { JSON.stringify(err); }); }`,
`try { f(); } catch (err) { [1].forEach(err => JSON.stringify(err)); }`,
]@copilot please address this.
| 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; |
There was a problem hiding this comment.
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 ruleThis 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.
Review Summary 🧠Applied 🔴 Blocking: scope-stack leak causing false positives
try { f(); } catch (err) {
items.map(err => JSON.stringify(err)); // false positive — inner err is not the caught Error
}Fix: stop collection when the first sentinel is encountered (see inline comment on line 43 of Also add a corresponding test case to the valid set (see inline comment on line 58 of the test file). 🟡 Non-blocking improvements
@copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on one correctness issue in the scope-stack logic and several improvements to test structure and message consistency.
📋 Key Themes & Highlights
Key Issues
- Correctness (scope leak):
getCaughtVarNames()iterates the entire scope stack without stopping at sentinels, so a non-.catch()callback inside a catch block can produce false positives when its parameter name matches the outer catch variable. This is the only blocking issue. - Missing edge-case test: No test covers a non-
.catch()callback inside a catch block with a same-named parameter — exactly the scenario that would catch the scope leak. - Message consistency: The
useGetErrorMessagesuggestion message should call out the import requirement, matching the wording in the siblingprefer-get-error-messagerule. - Test structure: Tests are split across 11
RuleTester.run()calls rather than consolidated into one — departs from the established codebase pattern.
Positive Highlights
- ✅ Solid real-world motivation with a confirmed live hit (0 false positives in
actions/setup/js) - ✅ Well-designed scope stack with sentinels and correct push/pop pairing on all AST entry/exit events
- ✅ Good range of valid and invalid test cases covering CJS/ESM, bare catch, nested catch, and promise callbacks
- ✅ Suggestion-based auto-fix is the right UX choice — avoids silent, potentially broken rewrites
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 116.3 AIC · ⌖ 10.6 AIC · ⊞ 6.6K
Comment /matt to run again
| function getCaughtVarNames(): Set<string> { | ||
| const names = new Set<string>(); | ||
| for (const scope of scopeStack) { | ||
| if (!scope.isSentinel && scope.varName) { |
There was a problem hiding this comment.
[/tdd] getCaughtVarNames() does not stop at sentinels — outer catch variable names can leak through non-.catch() function boundaries, potentially causing false positives when a regular function parameter shadows a catch variable name.
💡 False positive scenario and fix
Consider this pattern (non-.catch() callback inside a catch):
try { f(); } catch (err) {
items.map(err => JSON.stringify(err));
// ^^^ map callback parameter, not a caught Error
}The sentinel pushed by the arrow function is iterated over (because isSentinel skips it) but does NOT stop the outer err from being collected. getCaughtVarNames iterates the entire stack, so the outer err is returned and the JSON.stringify(err) inside the map callback is flagged as an error, which is a false positive.
Suggested fix — stop collection at the innermost sentinel:
function getCaughtVarNames(): Set<string> {
const names = new Set<string>();
for (let i = scopeStack.length - 1; i >= 0; i--) {
const scope = scopeStack[i];
if (scope.isSentinel) break; // new function scope: stop, don't see outer frames
if (scope.varName) names.add(scope.varName);
}
return names;
}This does trade off catching closured-captured errors in non-catch callbacks (e.g. setTimeout(() => JSON.stringify(err), 0)), which is an acceptable tradeoff to avoid false positives.
@copilot please address this.
| valid: [ | ||
| `const err = { a: 1 }; try { f(); } catch (err) { } JSON.stringify(err);`, | ||
| ], | ||
| invalid: [], |
There was a problem hiding this comment.
[/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.
| const callee = node.callee; | ||
| if (callee.type !== AST_NODE_TYPES.MemberExpression) return; | ||
| if (callee.computed) return; | ||
| const obj = callee.object; |
There was a problem hiding this comment.
[/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.
| description: "Disallow JSON.stringify() on caught error variables — Error properties (message, stack, etc.) are non-enumerable and produce {} silently", | ||
| }, | ||
| schema: [], | ||
| messages: { |
There was a problem hiding this comment.
[/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.
| fix(fixer) { | ||
| return fixer.replaceText(node, `getErrorMessage(${errorVar})`); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
[/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.
|
|
||
| it("invalid: JSON.stringify(error, null, 2) in catch block is flagged", () => { | ||
| cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { | ||
| valid: [], |
There was a problem hiding this comment.
[/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.
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the unresolved review feedback in Re-ran local validation on the updated branch: Latest known CI on the current head shows |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Great work on this one! 🎉 The The PR is well-structured: the rule implementation, tests, plugin registration, and config wiring all land together, the test coverage is thorough (7 valid + 4 invalid scenarios), the body includes a live evidence hit from This looks ready for review. ✅
|
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new ESLint rule
gh-aw-custom/no-json-stringify-errorto theeslint-factoryplugin. The rule flagsJSON.stringify()calls whose first argument is a caught error variable, becauseErrorproperties (message,stack, etc.) are non-enumerable and silently produce{}when serialised withJSON.stringify.Changes
eslint-factory/src/rules/no-json-stringify-error.tseslint-factory/src/rules/no-json-stringify-error.test.tseslint-factory/src/index.tsnoJsonStringifyErrorRuleeslint-factory/eslint.config.cjs"warn"severityRule Behaviour
Flagged —
JSON.stringifyon a caught error variable in atry/catchblock or a.catch()inline callback (arrow or function expression):Not flagged — non-error variables, explicit property serialisation, scope shadowing, bare catch without binding, named
.catch()handler references:Auto-fix suggestion — replaces
JSON.stringify(errVar)withgetErrorMessage(errVar)(fromerror_helpers.cjs).Implementation Details
@typescript-eslint/utils(ESLintUtils,TSESTree,AST_NODE_TYPES).scopeStack: ErrorScope[]. Each entry holdsvarNameandisSentinel..catch()functions (and.catch()callbacks with destructured or absent params) to prevent false positives from shadowed names.getCaughtVarNames()walks the stack inward-to-outward and stops at the first sentinel, so inner function scopes cannot see outer catch variables.isCatchCallback()matches only inline callbacks passed as the first argument to a.catch()member call; named handler references are out of scope.try/catchblocks are supported — each clause pushes its own scope entry independently.Commits
6e527830ffeat(eslint): add no-json-stringify-error rulea47a44b05fix eslint no-json-stringify-error scope handling15eb2ac7cchore: clarify no-json-stringify-error guidanceb8354dc77docs: clarify no-json-stringify-error scope scan