feat(no-json-stringify-error): scope-aware suggestions + detail-preserving second suggestion#43748
Conversation
…-preserving form
- Add scope check: when `getErrorMessage` is not in scope, fall back to
`String(errorVar)` (useStringFallback) instead of an unresolved identifier
- Add second detail-preserving suggestion that emits
`\`\${getErrorMessage(errorVar)}\\n\${errorVar.stack ?? ""}\`` (or
`String()` variant when out of scope) so callers who wanted null,2-style
detail retain the stack trace
- Add new message IDs: useStringFallback, useDetailPreservingForm,
useDetailPreservingFormFallback
- Update all existing invalid test cases to reflect the new two-suggestion shape
- Add three new test cases: in-scope getErrorMessage path (CJS require),
in-scope path inside template literal, and explicit String() fallback
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…serving suggestion Replace template-literal approach with string concatenation to avoid nested template literals when the flagged call is already inside a template expression. The detail-preserving suggestions now produce: getErrorMessage(err) + "\n" + (err.stack ?? "") (in-scope path) String(err) + "\n" + (err.stack ?? "") (fallback path) Both forms are valid in any syntactic context — standalone arguments, template expressions, and ternaries — without requiring template nesting. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR updates the no-json-stringify-error ESLint rule to emit scope-aware suggestions: it now prefers getErrorMessage(err) only when getErrorMessage is resolvable in the current scope chain, and otherwise suggests an import-free fallback (String(err)). It also adds a second “detail-preserving” suggestion intended to retain stack information while avoiding nested template literals.
Changes:
- Add scope-chain inspection to choose between
getErrorMessage(err)andString(err)for the first suggestion. - Add an always-emitted second suggestion that preserves additional detail (stack).
- Update existing tests and add new cases for in-scope CommonJS patterns and fallback behavior.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/no-json-stringify-error.ts | Adds scope-aware suggestion selection and introduces a second, detail-preserving suggestion. |
| eslint-factory/src/rules/no-json-stringify-error.test.ts | Updates expected suggestions/outputs and adds new test cases for in-scope detection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Low
| const getErrorMessageInScope = (() => { | ||
| let scope: ReturnType<typeof context.sourceCode.getScope> | null = context.sourceCode.getScope(node); | ||
| while (scope) { | ||
| if (scope.variables.some(v => v.name === "getErrorMessage")) return true; | ||
| scope = scope.upper; | ||
| } | ||
| return false; | ||
| })(); |
| useDetailPreservingForm: 'Replace with getErrorMessage({{errorVar}}) + "\\n" + ({{errorVar}}.stack ?? "") to preserve the stack trace — ensure getErrorMessage is imported from error_helpers.cjs.', | ||
| useDetailPreservingFormFallback: 'Replace with String({{errorVar}}) + "\\n" + ({{errorVar}}.stack ?? "") to preserve the stack trace.', |
| messageId: "useDetailPreservingForm" as const, | ||
| data: { errorVar }, | ||
| fix(fixer) { | ||
| return fixer.replaceText(node, `getErrorMessage(${errorVar}) + "\\n" + (${errorVar}.stack ?? "")`); |
| messageId: "useDetailPreservingFormFallback" as const, | ||
| data: { errorVar }, | ||
| fix(fixer) { | ||
| return fixer.replaceText(node, `String(${errorVar}) + "\\n" + (${errorVar}.stack ?? "")`); |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics (3 tests)
Note: The pre-fetch test-detection scripts (targeting
Verdict
Inflation note: The 3.6× ratio (164 added test lines vs 46 production lines) is above the 2:1 threshold, but is structurally expected for ESLint Test breakdown:
References: §28782440159 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.
Skills-Based Review 🧠
Applied /tdd, /codebase-design, and /grill-with-docs — COMMENT with 4 targeted suggestions.
📋 Key Themes & Highlights
Key Themes
- Missing ESM in-scope test: All three new scope-aware tests use
cjsRuleTester. The ESMimportpath is exercised nowhere, leaving a gap where a regression in ESM scope detection would go unnoticed. (See inline comment on line 391 of the test file.) - Scope check precision:
scope.variables.some(v => v.name === "getErrorMessage")matches any declaration of that name — includinglet getErrorMessage = nullor a forward-declared-but-uninitialized var — potentially surfacinguseGetErrorMessagewhen the call would fail at runtime. Addingv.defs.length > 0would tighten this. (See inline comment on line 136 of the rule file.) - Undocumented local-scope behavior: A
const getErrorMessage = ...declared inside the catch body would be found by the upward scope walk. Whether that's intentional is undocumented by a test. (See inline comment on line 133.) - Primary error message inconsistency: The
jsonStringifyErrormessage hardcodes "Prefer getErrorMessage" even when the rule now choosesString()as the first suggestion. (See inline comment on line 52.)
Positive Highlights
- ✅ Scope-chain traversal is clean and idiomatic — using
context.sourceCode.getScope(node)withscope.upperis exactly the right approach for ESLint scope analysis. - ✅ String concatenation instead of template literals for the detail-preserving suggestion is a good defensive choice — avoids nested template literal issues when the flagged expression is already inside a
${...}. - ✅ 11 existing tests updated to match new behavior; the dual-suggestion structure is consistent and clear across all cases.
- ✅ Three new test cases cleanly document the two new code paths (in-scope CJS, destructured require, explicit no-scope fallback).
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 82.4 AIC · ⌖ 5.93 AIC · ⊞ 6.7K
Comment /matt to run again
| }); | ||
|
|
||
| it("invalid: when getErrorMessage is in scope, suggests getErrorMessage and detail-preserving form", () => { | ||
| cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { |
There was a problem hiding this comment.
[/tdd] No ESM in-scope test: all three new test cases use cjsRuleTester. The ESM import path (import { getErrorMessage } from ...) is untested — scope analysis for import declarations differs from CJS require() and is worth an explicit esmRuleTester case.
💡 Suggested test addition
it("invalid: when getErrorMessage is imported via ESM, first suggestion uses getErrorMessage", () => {
esmRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `import { getErrorMessage } from "./error_helpers"; try { f(); } catch (err) { core.error(JSON.stringify(err)); }`,
errors: [{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [{
messageId: "useGetErrorMessage",
data: { errorVar: "err" },
output: `import { getErrorMessage } from "./error_helpers"; try { f(); } catch (err) { core.error(getErrorMessage(err)); }`,
}],
}],
},
],
});
});Without this, a regression in ESM scope detection would go unnoticed.
@copilot please address this.
| const errorVar = firstArg.name; | ||
|
|
||
| // Check whether getErrorMessage is resolvable in the current scope chain. | ||
| const getErrorMessageInScope = (() => { |
There was a problem hiding this comment.
[/tdd] No test for getErrorMessage declared inside the catch block itself. The scope walk starts at context.sourceCode.getScope(node) (the CallExpression) and walks upward, which means a local const getErrorMessage = ... inside the catch body would be found. That's probably the right behavior, but there's no test confirming it — or ruling it out.
💡 Why this matters
Consider:
try { f(); } catch (err) {
const getErrorMessage = customHelper;
core.error(JSON.stringify(err)); // would this pick useGetErrorMessage or useStringFallback?
}Add a test to document the intended behavior (accept useGetErrorMessage here, or document why it shouldn't).
@copilot please address this.
| const getErrorMessageInScope = (() => { | ||
| let scope: ReturnType<typeof context.sourceCode.getScope> | null = context.sourceCode.getScope(node); | ||
| while (scope) { | ||
| if (scope.variables.some(v => v.name === "getErrorMessage")) return true; |
There was a problem hiding this comment.
[/codebase-design] scope.variables.some(v => v.name === "getErrorMessage") matches any variable named getErrorMessage — including uninitialized, re-assigned, or completely unrelated variables like let getErrorMessage = null. This could cause the rule to emit useGetErrorMessage when calling getErrorMessage(err) would throw at runtime.
💡 Consider checking for at least one write reference (i.e., an actual binding)
// More precise: require at least one write reference so that uninitialized declarations don't count
if (scope.variables.some(v =>
v.name === "getErrorMessage" && v.defs.length > 0
)) return true;v.defs.length > 0 ensures the variable has an actual definition (import, const, function) rather than only being an identifier reference.
@copilot please address this.
| "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.", | ||
| useStringFallback: "Replace with String({{errorVar}}) — getErrorMessage is not in scope; String() is a safe import-free alternative.", | ||
| useDetailPreservingForm: 'Replace with getErrorMessage({{errorVar}}) + "\\n" + ({{errorVar}}.stack ?? "") to preserve the stack trace — ensure getErrorMessage is imported from error_helpers.cjs.', |
There was a problem hiding this comment.
[/grill-with-docs] The jsonStringifyError primary message still says "Prefer getErrorMessage({{errorVar}})" unconditionally, even though the rule now sometimes suggests String() as the first option. The message misleads users in the no-scope case.
💡 Suggested fix
Consider splitting into two message variants, or make the primary message scope-neutral:
jsonStringifyError:
"JSON.stringify({{errorVar}}) produces {} for Error objects — Error properties are non-enumerable. " +
"Use the suggestions below to replace it with a safe alternative.",This avoids confusing users who don't have getErrorMessage imported.
@copilot please address this.
There was a problem hiding this comment.
Review: no-json-stringify-error scope-aware suggestions
The scope-walk logic and dual-suggestion approach are well-structured and the bulk of test coverage is solid. Two actionable issues found:
1. Diagnostic message contradicts the fallback suggestion (blocking)
When getErrorMessage is not in scope, the rule still emits the jsonStringifyError messageId whose text reads "Prefer getErrorMessage({{errorVar}}) from error_helpers.cjs". The first suggestion then proposes String(err) — directly contradicting the message body. The main error message should be updated to scope-neutral wording, or a second messageId (e.g. jsonStringifyErrorNoHelper) should be registered and emitted when the fallback path is taken. See inline comment on no-json-stringify-error.ts line 49.
2. Missing ESM import scope test (non-blocking but recommended)
All three new scope-aware test cases use cjsRuleTester with require() patterns. The scope walk reads scope.variables, which also covers ES module import { getErrorMessage } from '...' bindings — but that code path has no test. A regression in ESM import detection would be invisible without one. See inline comment on the new test block in no-json-stringify-error.test.ts.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 82.1 AIC · ⌖ 6.15 AIC · ⊞ 4.9K
| @@ -48,6 +48,9 @@ export const noJsonStringifyErrorRule = createRule({ | |||
| 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.", | |||
There was a problem hiding this comment.
The jsonStringifyError diagnostic message always says "Prefer getErrorMessage({{errorVar}}) from error_helpers.cjs" regardless of whether getErrorMessage is in scope. When the scope check yields false, the first suggestion becomes String(err) — but the diagnostic still points the developer at getErrorMessage. The two signals disagree.
Consider a scope-neutral phrasing, e.g.:
"JSON.stringify({{errorVar}}) produces {} for Error objects — ... Use getErrorMessage({{errorVar}}) if it is imported, or String({{errorVar}}) as a safe import-free alternative."Alternatively, register a second messageId (e.g. jsonStringifyErrorNoHelper) and emit it when getErrorMessageInScope is false.
@copilot please address this.
| }); | ||
|
|
||
| it("invalid: when getErrorMessage is in scope, suggests getErrorMessage and detail-preserving form", () => { | ||
| cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { |
There was a problem hiding this comment.
The three new "in scope" test cases use cjsRuleTester with CJS require() patterns. The scope-walk (context.sourceCode.getScope(node).variables) should also detect ES module import { getErrorMessage } from "..." bindings — but there is no test covering this path.
Adding a test with esmRuleTester:
it("invalid: when getErrorMessage is imported (ESM), suggests getErrorMessage", () => {
esmRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, {
valid: [],
invalid: [
{
code: `import { getErrorMessage } from "./error_helpers.js"; try { f(); } catch (err) { core.error(JSON.stringify(err)); }`,
errors: [
{
messageId: "jsonStringifyError",
data: { errorVar: "err" },
suggestions: [
{ messageId: "useGetErrorMessage", data: { errorVar: "err" },
output: `import { getErrorMessage } from "./error_helpers.js"; try { f(); } catch (err) { core.error(getErrorMessage(err)); }` },
{ messageId: "useDetailPreservingForm", data: { errorVar: "err" },
output: \`import { getErrorMessage } from "./error_helpers.js"; try { f(); } catch (err) { core.error(getErrorMessage(err) + "\\n" + (err.stack ?? "")); }\` },
],
},
],
},
],
});
});Without this, a regression in ESM scope detection would go unnoticed.
@copilot please address this.
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness bugs in the fixer plus four test coverage gaps.
### Blocking issues (must fix before merge)
1. Fixer missing parentheses (high) — The detail-preserving fixers replace the raw JSON.stringify(err) node with getErrorMessage(err) + "\n" + (err.stack ?? "") without wrapping in parentheses. When the original call is chained (.slice(), .trim(), etc.) the replacement silently changes runtime semantics — the chained method attaches only to (err.stack ?? "") instead of the whole expression. Both useDetailPreservingForm and useDetailPreservingFormFallback fixers are affected.
2. TDZ false positive (high) — scope.variables is populated by ESLint for all const/let bindings in a block regardless of textual order. If getErrorMessage is declared after the JSON.stringify(err) call in the same block, the scope walk returns true and the rule suggests getErrorMessage(err), which throws a ReferenceError at runtime due to the temporal dead zone. A range check against node.range[0] is needed.
### Non-blocking issues (should fix)
3. Name-only scope detection (medium) — Any binding named getErrorMessage — a local function, test mock, different library — triggers the useGetErrorMessage path and tells the developer to "ensure getErrorMessage is imported from error_helpers.cjs", which is incorrect when a different getErrorMessage is already present.
4. Detail-preserving suggestion duplicates the message (medium) — String(err) + "\n" + (err.stack ?? "") produces the error message twice on most engines, since err.stack already starts with "Error: message". (err.stack ?? String(err)) is a cleaner idiom.
5. Missing ESM in-scope test (medium) — esmRuleTester only covers the fallback path. An ESM import test is needed to confirm the scope walk traverses module scope correctly.
6. Missing promise-handler in-scope test (medium) — No test verifies that getErrorMessage is correctly detected when the call site is inside a .catch()/.then() handler, where the custom sentinel-frame mechanism and the ESLint scope API interact.
🔎 Code quality review by PR Code Quality Reviewer · 126.1 AIC · ⌖ 5.99 AIC · ⊞ 5.5K
Comment /review to run again
Comments that could not be inline-anchored
eslint-factory/src/rules/no-json-stringify-error.ts:169
Fixer output missing parentheses breaks operator precedence when JSON.stringify(err) is part of a chained call — applying suggestion 2 silently produces wrong code.
<details>
<summary>💡 Explanation and fix</summary>
fixer.replaceText(node, ...) replaces only the JSON.stringify(err) CallExpression node. If the original code chains a method on it:
console.log(JSON.stringify(err).slice(0, 200))the replacement emits:
// WRONG — .slice(0, 200) applies only to (err.…
</details>
<details><summary>eslint-factory/src/rules/no-json-stringify-error.ts:134</summary>
**TDZ false positive: `const`/`let getErrorMessage` declared after the call site still triggers `useGetErrorMessage`** — the suggestion emits code that throws `ReferenceError` at runtime.
<details>
<summary>💡 Explanation and fix</summary>
ESLint's `scope.variables` contains **all** `const`/`let` bindings for a block scope, regardless of their textual position. If user code looks like:
```js
try { f(); } catch (err) {
core.error(JSON.stringify(err)); // flagged here
const getErrorMessage…
</details>
<details><summary>eslint-factory/src/rules/no-json-stringify-error.ts:136</summary>
**Scope check matches any identifier named `getErrorMessage`, not just the `error_helpers.cjs` import** — a local function, test mock, or different library triggers the `useGetErrorMessage` path with a misleading message.
<details>
<summary>💡 Explanation</summary>
The check `v.name === "getErrorMessage"` is purely name-based. Consider:
```js
function getErrorMessage(e) { return e.code; } // totally unrelated helper
try { f(); } catch (err) {
core.error(JSON.stringify(err));
}The rul…
eslint-factory/src/rules/no-json-stringify-error.ts:176
String(err) + "\\n" + (err.stack ?? "") duplicates the error message — the suggested fix produces noisier output than the original intent.
<details>
<summary>💡 Detail</summary>
String(err) on a standard Error produces "Error: the message". On most JS engines err.stack starts with the same "Error: the message" line before the frame list:
Error: the message
at f (file.js:1:5)
at ...
So the generated code:
console.error(String(err) + "\n" + (err.stack …
</details>
<details><summary>eslint-factory/src/rules/no-json-stringify-error.test.ts:312</summary>
**No ESM test with `getErrorMessage` in scope** — the `esmRuleTester` block only covers the fallback path, leaving the scope-detection logic untested for ES module files.
<details>
<summary>💡 Detail</summary>
All three new tests that exercise `useGetErrorMessage` / `useDetailPreservingForm` use `cjsRuleTester` with a `require()` call to introduce the binding. ESM `import` declarations produce a different scope structure in ESLint (module-level scope vs. program scope). A test like:
```ts
es…
</details>
<details><summary>eslint-factory/src/rules/no-json-stringify-error.test.ts:390</summary>
**No test for `getErrorMessage`-in-scope inside a `.catch()` or `.then()` rejection handler** — the scope-detection logic's interaction with the custom sentinel-frame stack is untested for promise handlers.
<details>
<summary>💡 Detail</summary>
The rule uses its own `scopeStack` with sentinels to track caught error variables, while the new scope-detection feature uses ESLint's built-in `context.sourceCode.getScope(node)` API. These are two separate mechanisms. Inside an inline `.catch(err =>…
</details>
The
no-json-stringify-errorrule had a single suggestion (getErrorMessage(err)) that was always emitted regardless of whethergetErrorMessagewas imported, and it discarded the developer's evident intent to capture more than just.message.Changes
Scope-aware first suggestion
context.sourceCode.getScope(node)up the scope chain to check ifgetErrorMessageis declared in the fileuseGetErrorMessage:getErrorMessage(err)(existing behavior)useStringFallback:String(err)(safe, no import required)Detail-preserving second suggestion (always emitted)
useDetailPreservingForm(whengetErrorMessagein scope):getErrorMessage(err) + "\n" + (err.stack ?? "")useDetailPreservingFormFallback(fallback):String(err) + "\n" + (err.stack ?? "")${...}expressionExample — the live site pattern from the corpus:
Tests
useStringFallback+useDetailPreservingFormFallbacksince test snippets don't importgetErrorMessage)