eslint-factory: extend no-json-stringify-error to cover .then(_, onRejected) and pin scope boundaries#43724
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…, onRejected) and pin scope boundaries Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43724 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
There was a problem hiding this comment.
Pull request overview
Extends the eslint-factory no-json-stringify-error rule so it also detects JSON.stringify(err) inside inline Promise rejection handlers passed as the second argument to .then(onFulfilled, onRejected), and documents/tests the intended scope boundaries (inline handlers only; named references out of scope). The PR also includes widespread .lock.yml churn that appears to be mechanical (YAML block chomping / trailing-newline normalization).
Changes:
- Update rule scope detection to treat
.then(_, onRejected)inline callbacks as “catch-like” error scopes. - Add new RuleTester cases covering
.then(_, onRejected)(arrow + function) and verifying named-reference handlers remain unflagged. - Document the rule in
eslint-factory/README.mdand update the rule’sdocs.description; regenerate multiple workflow.lock.ymlfiles.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/no-json-stringify-error.ts | Extends catch-scope detection to include .then(_, onRejected) inline rejection handlers; updates rule docs text. |
| eslint-factory/src/rules/no-json-stringify-error.test.ts | Adds test cases for .then(_, onRejected) detection and named-reference exclusions. |
| eslint-factory/README.md | Documents no-json-stringify-error rule behavior/scope alongside existing rules. |
| .github/workflows/workflow-skill-extractor.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in embedded script. |
| .github/workflows/workflow-health-manager.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/weekly-blog-post-writer.lock.yml | Mechanical .lock.yml regeneration (`run: |
| .github/workflows/unbloat-docs.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/uk-ai-operational-resilience.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/test-quality-sentinel.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/step-name-alignment.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/static-analysis-report.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/stale-repo-identifier.lock.yml | Mechanical .lock.yml regeneration (`run: |
| .github/workflows/spec-extractor.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/schema-consistency-checker.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/repository-quality-improver.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/release.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/prompt-clustering-analysis.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/pr-description-caveman.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/objective-impact-report.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in embedded script. |
| .github/workflows/mergefest.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/mattpocock-skills-reviewer.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/linter-miner.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/lint-monster.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/issue-arborist.lock.yml | Mechanical .lock.yml regeneration (`run: |
| .github/workflows/impeccable-skills-reviewer.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/gpclean.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/glossary-maintainer.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/eslint-monster.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/docs-noob-tester.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/designer-drift-audit.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/design-decision-gate.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/dependabot-burner.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in embedded script. |
| .github/workflows/delight.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/dataflow-pr-discussion-dataset.lock.yml | Mechanical .lock.yml regeneration (`run: |
| .github/workflows/daily-spdd-spec-planner.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/daily-security-observability.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/daily-multi-device-docs-tester.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/daily-community-attribution.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/daily-ambient-context-optimizer.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in embedded script. |
| .github/workflows/copilot-pr-nlp-analysis.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/copilot-centralization-drilldown.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/contribution-check.lock.yml | Mechanical .lock.yml regeneration (`run: |
| .github/workflows/code-simplifier.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/cli-consistency-checker.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/ci-doctor.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/aw-failure-investigator.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/auto-triage-issues.lock.yml | Mechanical .lock.yml regeneration (`run: |
| .github/workflows/architecture-guardian.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/agentic-token-trend-audit.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/agentic-token-optimizer.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
| .github/workflows/agentic-token-audit.lock.yml | Mechanical .lock.yml regeneration / newline-chomping normalization in run: scripts. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 51/51 changed files
- Comments generated: 2
- Review effort level: Low
| * - Second argument of `.then(onFulfilled, onRejected)` — semantically | ||
| * equivalent to `.catch(onRejected)`. |
| * 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 without cross-file analysis. |
![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 — .then(_, onRejected) extension
The implementation is correct and well-scoped.
isCatchCallback logic (lines 21–33):
.catch(fn)at argument index 0 ✓.then(_, fn)at argument index 1 ✓- Computed member access excluded (
callee.computedguard) ✓ - Named references excluded by AST node type — only
ArrowFunctionExpression/FunctionExpressionnodes reachenterFunction, sop.then(ok, handler)wherehandleris an Identifier is never matched ✓
Sentinel stack discipline remains intact — non-rejection-handler functions still push a sentinel, blocking outer catch-var names from leaking into unrelated scopes.
Tests (5 new cases):
- Both arrow and function-expression inline handlers flagged ✓
p.then(result => ...)first argument not flagged ✓- Named-reference variants for both
.catchand.thenpinned as valid ✓
README correctly enumerates all three detected scopes and states the named-reference out-of-scope boundary.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 46.2 AIC · ⌖ 7.41 AIC · ⊞ 4.9K
🧪 Test Quality Sentinel Report✅ Test Quality Score: 82/100 — Excellent
📊 Metrics (18 tests)
📝 Notes on inflation flagInflation (3.45:1): The 69:20 added-line ratio exceeds the 2:1 threshold mechanically, but this is structurally expected for ESLint Arrow/function pairs: Testing both Verdict
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.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — solid change with a few targeted improvements available.
📋 Key Themes & Highlights
Key Themes
- Naming drift:
isCatchCallbacknow covers.then()rejection handlers too; the name no longer matches the semantics and is a future maintenance hazard. - Test coverage gap: The two new
invalidtests only exercise CJS mode. The existing.catch()andtry/catchinvalid cases also have ESM counterparts — the new.then()cases should too. - False-positive boundary: The
onFulfilledtest uses aresultparameter. The highest-risk false-positive is a first-arg callback that useserras its param name — this isn't tested. - Incidental test correctness: The named-reference tests pass because
FunctionDeclarationnodes are not tracked — not because cross-file reference resolution is skipped. The test description is misleading.
Positive Highlights
- ✅ Precise AST logic:
parent.arguments[1] === nodecorrectly pins to the second argument without relying on index arithmetic. - ✅ Updated JSDoc on
isCatchCallbackaccurately describes both detection cases and the out-of-scope rationale. - ✅ README now explicitly enumerates all three scopes and the
out of scopecaveat — great for rule consumers. - ✅ Five new test cases cover both arrow and function-expression syntax plus both named-reference exclusions.
- ✅ The
docs.descriptionupdate keeps the rule self-documenting inside the ESLint metadata.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 88.6 AIC · ⌖ 8.5 AIC · ⊞ 6.7K
Comment /matt to run again
| * out of scope because the rule cannot statically follow the reference to | ||
| * inspect its parameter list without cross-file analysis. | ||
| */ | ||
| function isCatchCallback(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): boolean { |
There was a problem hiding this comment.
[/grill-with-docs] isCatchCallback is now a misnomer — the function also returns true for .then() second-arg rejection handlers, but its name implies only .catch(). Future maintainers may assume the sentinel does not cover .then() and widen it incorrectly.
💡 Suggested rename
Consider renaming to isInlineRejectionHandler (or isRejectionHandlerCallback) to match the expanded semantics described in the JSDoc:
function isInlineRejectionHandler(
node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression
): boolean {The callers (enterFunction) and the comment on line 104 would need updating to match.
@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("invalid: JSON.stringify(err) in .then() second-argument rejection handler (arrow) is flagged", () => { |
There was a problem hiding this comment.
[/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.
| }); | ||
| }); | ||
|
|
||
| it("valid: first argument of .then() (onFulfilled) is not treated as a rejection handler", () => { |
There was a problem hiding this comment.
[/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.
| }); | ||
| }); | ||
|
|
||
| it("valid: p.catch(handler) named-reference is not flagged (static cross-reference out of scope)", () => { |
There was a problem hiding this comment.
[/tdd] The named-reference exclusion test (p.catch(handler)) places JSON.stringify(err) inside handler, so the rule would flag the standalone function body. The test only passes because p.catch(handler) itself is not inside a rejection scope — but the test comment says it's about cross-reference analysis. The test would be clearer if handler is defined outside any .catch() call and the JSON.stringify call is confirmed untouched.
💡 Observation
This is a readability/specification issue, not a bug. The current test function handler(err) { JSON.stringify(err); } p.catch(handler); passes because handler is a FunctionDeclaration (not FunctionExpression/ArrowFunctionExpression), so enterFunction is never triggered for it at all. The test description says "static cross-reference out of scope" but the actual mechanism is that FunctionDeclaration nodes are not tracked. A comment or rename clarifying this would prevent confusion.
@copilot please address this.
There was a problem hiding this comment.
Two test coverage gaps and a stale function name block this merge.
The rule logic itself is correct — the .then(_, onRejected) detection in isCatchCallback is a clean, minimal change and the sentinel-stack scoping is unaffected. The concerns below are about test confidence and maintainability debt introduced alongside the logic change.
Blocking findings
-
Missing ESM test for the new
.then()path (high): All five new invalid/valid cases usecjsRuleTester. The suite already has anesmRuleTesterpath fortry/catchand.catch(). Module vs. script parser differences can mask rule failures, and a regression in ESM mode would pass CI silently. -
isCatchCallbackname is now wrong (medium): The function covers both.catch(fn)and.then(_, fn), but the name only sayscatch. Maintainers reading the.then()branch will spend time verifying the coverage is intentional rather than a copy-paste mistake. Rename toisInlineRejectionHandler(or similar) and update the companion comment at the AST visitor registration site.
Non-blocking observations
-
Missing
JSON.stringify(err, null, 2)test for the.then()path (medium): The extra-args variant is covered fortry/catchbut not for the new.then()detection. The rule logic already handles it correctly, but a dedicated test would pin the assumption against future refactors. -
parent.arguments[1]has no explicit bounds guard (low):undefined === nodeis alwaysfalse, so this is safe today. Butparent.arguments.length > 1 && parent.arguments[1] === nodemakes the intent explicit and guards against future pattern-changes. -
Stale comment in
enterFunctionelse-branch still reads// Non-.catch() function— update toNon-rejection-handler functionto stay in sync with the expanded detection scope.
🔎 Code quality review by PR Code Quality Reviewer · 101.6 AIC · ⌖ 5.72 AIC · ⊞ 5.4K
Comment /review to run again
| cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { | ||
| valid: [`p.then(result => JSON.stringify(result));`, `p.then(function(result) { JSON.stringify(result); });`], | ||
| invalid: [], | ||
| }); |
There was a problem hiding this comment.
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.
| * out of scope because the rule cannot statically follow the reference to | ||
| * inspect its parameter list without cross-file analysis. | ||
| */ | ||
| function isCatchCallback(node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression): boolean { |
There was a problem hiding this comment.
isCatchCallback is now a misleading name — the function detects both .catch(fn) and .then(_, fn) rejection handlers, so the name actively lies to maintainers about its scope.
💡 Suggested fix
Rename to isInlineRejectionHandler (and update the companion comment at line 101):
function isInlineRejectionHandler(
node: TSESTree.ArrowFunctionExpression | TSESTree.FunctionExpression
): boolean {
...
}Also update the comment at the AST visitor registration site:
// Track inline rejection-handler parameters (.catch(fn) and .then(onFulfilled, onRejected))
ArrowFunctionExpression: enterFunction,A maintainer looking at the .then() branch and seeing isCatchCallback will waste time deciding whether the coverage is intentional — a good function name eliminates that question entirely.
| }); | ||
|
|
||
| it("invalid: JSON.stringify(err) in .then() second-argument rejection handler (function) is flagged", () => { | ||
| cjsRuleTester.run("no-json-stringify-error", noJsonStringifyErrorRule, { |
There was a problem hiding this comment.
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.
|
@copilot please run the
|
PR Triage
CHANGES_REQUESTED by reviewer: missing ESM test coverage for .then(_, onRejected) and isCatchCallback rename needed. Blocked on reviewer feedback; 48 lock file changes add noise. Defer until agent addresses review comments.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in c916cb0. The follow-up updates rename the rejection-handler helper, tighten the inline-scope wording, and add the missing |
The
no-json-stringify-errorrule silently skippedJSON.stringify(err)inside.then(onFulfilled, onRejected)rejection handlers — semantically identical to.catch(err => ...)but undetected. The named-reference exclusion (p.catch(handler)) also lacked a pinning test and was undocumented.Rule change
isCatchCallback: now also returnstruefor the second argument of.then()calls (the rejection handler), in addition to the first argument of.catch().docs.description: updated to enumerate the three detected scopes (try/catch,.catch(fn),.then(_, fn)) and state explicitly that named-reference handlers are out of scope because cross-file static analysis is required.Tests (5 new)
.then(ok, err => JSON.stringify(err))arrow inline.then(null, function(err) { JSON.stringify(err); })function inline.then()first argument (onFulfilled)p.catch(handler)named referencep.then(ok, handler)named-reference rejection handlerREADME
Added
### no-json-stringify-errorsection toeslint-factory/README.md(rule was previously undocumented alongsideprefer-number-isnan).pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/28776499241