eslint-factory: remove useless rethrow from require-json-parse-try-catch suggestion

[Copilot]

The require-json-parse-try-catch suggestion previously auto-generated catch (err) { throw err; }, which adds no handling and can trigger no-useless-catch. This change updates the emitted fix to produce a meaningful catch body and aligns all rule fixtures with the new output.

• Suggestion output quality

  • Replaced the no-op rethrow catch body in require-json-parse-try-catch autofix output.
  • New emitted catch block includes:
    • an explicit // TODO: handle parse failure for this code path.
    • contextual rethrow via new Error("Failed to parse JSON: " + ...)
  • Refactored rule code to build suggestion text via a dedicated helper for readability.

• Rule fixture updates

  • Updated require-json-parse-try-catch.test.ts expected suggestion outputs across all affected cases.
  • Includes top-level statement wrapping and deferred-callback nested cases (on, then, setTimeout, Promise executor, nextTick).

try {
  const data = JSON.parse(rawInput);
} catch (err) {
  // TODO: handle parse failure for this code path.
  throw new Error(
    "Failed to parse JSON: " + (err instanceof Error ? err.message : String(err)),
  );
}

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills...

[github-actions]

✅ 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 (0 additions detected in default business logic dirs).

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[Copilot]

Pull request overview

Updates the require-json-parse-try-catch ESLint rule in eslint-factory to emit a non-trivial try/catch suggestion (avoiding a no-op throw err; catch body that can trigger no-useless-catch) and refreshes the rule’s fixtures to match the new suggestion output.

Changes:

• Introduced a buildTryCatchSuggestion() helper to centralize and simplify suggestion string construction.
• Updated the rule’s autofix suggestion to include a TODO comment and a more descriptive rethrow (throw new Error("Failed to parse JSON: " + ...)).
• Updated all affected test fixture expected outputs to match the new suggestion text across top-level and deferred-callback cases.

Show a summary per file

File	Description
eslint-factory/src/rules/require-json-parse-try-catch.ts	Refactors suggestion construction and replaces the catch-body output to avoid a useless rethrow.
eslint-factory/src/rules/require-json-parse-try-catch.test.ts	Updates expected suggestion outputs for all impacted cases to align with the new emitted catch block.

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: 1
• Review effort level: Low

[pelikhan]

@copilot run pr-finisher skill

[github-actions]

🤖 PR Triage

Field	Value
Category	bug
Risk	🟢 Low
Score	30 / 100
Action	⏸ defer
Batch	draft-small (with #42542, #42535)

Score breakdown: Impact 15/50 · Urgency 5/30 · Quality 10/20

Rationale: DRAFT fix for misleading require-json-parse-try-catch ESLint autofix output (no-op rethrow). Small, clean change. Mark ready when CI passes; part of the draft-small batch.

Generated by 🔧 PR Triage Agent · 83.9 AIC · ⌖ 17.1 AIC · ⊞ 1.6K · ◷

[github-actions]

Non-blocking observations

The core change is correct — replacing the no-op rethrow with a meaningful error message is a genuine improvement, and extracting buildTryCatchSuggestion is a clean refactor. Two medium issues worth addressing before this pattern proliferates across codebases that copy the generated suggestion.

Issues found

1. Generated Error drops the original SyntaxError (medium) — require-json-parse-try-catch.ts line 67

new Error("Failed to parse JSON: " + ...) loses err as .cause. Anyone debugging a parse failure in production gets only a generic message; the actual SyntaxError (with position info and its own stack) is gone. Fix: pass { cause: err } as the second argument.

2. Indentation path is completely untested (medium) — require-json-parse-try-catch.test.ts

Every fixture uses a top-level statement, so indent is always "". The ${indent} prefixes in buildTryCatchSuggestion are never exercised. A single test with JSON.parse inside an if body would cover this path.

🔎 Code quality review by PR Code Quality Reviewer · 43.4 AIC · ⌖ 7.49 AIC · ⊞ 1.6K
_{Comment /review to run again}

[github-actions]

Generated Error silently drops the original SyntaxError, making parse failures harder to debug.

💡 Suggested fix

The emitted catch body wraps err into a new Error but never passes it as cause, discarding the original SyntaxError stack trace. Anyone who applies this suggestion and later needs to diagnose why JSON failed to parse (e.g., unexpected token at position X) has no way to recover that information from the rethrown error.

Add { cause: err } as the second argument to new Error() in the generated string:

`${indent}  throw new Error(`,
`${indent}    "Failed to parse JSON: " + (err instanceof Error ? err.message : String(err)),`,
`${indent}    { cause: err },`,   // <-- add this line in the generated output
`${indent}  );`,

This preserves the original SyntaxError as error.cause, which debuggers and structured logging can surface. Error.cause is available in Node ≥ 16.9 and all modern browsers.

[github-actions]

No test covers an indented JSON.parse call — the indent path in buildTryCatchSuggestion is untested.

💡 What to add

Every test fixture here uses a top-level statement where indent computes to "". The indentation extraction in the rule:

const indent = stmtLine.match(/^(\s*)/)?.[1] ?? "";

...and the ${indent} prefixes in buildTryCatchSuggestion are therefore never exercised by any test. If a future refactor breaks indentation for nested calls (e.g., JSON.parse inside an if body), all existing tests would still pass.

Add an invalid fixture that wraps JSON.parse inside an indented block — something like:

{
  code: `if (cond) {\n  JSON.parse(raw);\n}`,
  // expect indented try/catch in suggestion output
}

This guards the indent !== "" path end-to-end.

[github-actions]

Review: eslint-factory — remove useless rethrow from require-json-parse-try-catch suggestion

Overall: ✅ The fix is correct and a clear improvement. Replacing the no-op throw err with a contextual wrapper eliminates the no-useless-catch lint trigger and gives consumers meaningful scaffolding to build on. The buildTryCatchSuggestion helper is clean and all 11 test fixtures are consistently updated.

One non-blocking suggestion (see inline comment): The generated new Error(...) discards the original error's stack trace. Adding { cause: err } would preserve the original for debugging, with no compatibility cost at Node 16.9+.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 28.5 AIC · ⌖ 8.17 AIC · ⊞ 4.9K

[github-actions]

The generated throw new Error(...) discards the original error's stack trace, which can make debugging parse failures harder.

Consider emitting { cause: err } to retain the original context:

`${indent}  throw new Error(`,
`${indent}    "Failed to parse JSON: " + (err instanceof Error ? err.message : String(err)),`,
`${indent}    { cause: err },`,
`${indent}  );`,

This produces auto-generated scaffolding like:

catch (err) {
  // TODO: handle parse failure for this code path.
  throw new Error(
    "Failed to parse JSON: " + (err instanceof Error ? err.message : String(err)),
    { cause: err },
  );
}

Error({ cause }) is supported in Node 16.9+ and is available in all environments this rule targets. Since the fix is auto-generated and applied broadly, preserving the original error as cause improves debuggability for all consumers who apply it. @copilot please address this.

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 95/100 — Excellent

Analyzed 5 modified test block(s) in a TypeScript/vitest test file: all 5 are design tests verifying behavioral contracts; 0 guideline violations, 1 pre-existing expectation anomaly flagged.

📊 Metrics & Test Classification (5 tests analyzed)

Metric	Value
New/modified tests analyzed	5
✅ Design tests (behavioral contracts)	5 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	5 (100%)
Duplicate test clusters	0
Test inflation detected	No (ratio ≈ 0.79:1)
🚨 Coding-guideline violations	0

Note: The changed test file is require-json-parse-try-catch.test.ts (TypeScript/vitest). This format was not detected by the pre-fetch step (which targets *.test.cjs/*.test.js). It is treated as a JavaScript/vitest test for scoring purposes.

Test (it() block)	File	Classification	Issues Detected
invalid: bare JSON.parse reports requireTryCatch	.test.ts:39	✅ Design	—
suggestion: wraps enclosing statement in try/catch (ESM)	.test.ts:73	✅ Design	—
suggestion output is syntactically valid JavaScript	.test.ts:98	✅ Design	⚠️ Suspected wrong expected indentation (see below)
invalid: computed JSON["parse"] access is flagged	.test.ts:120	✅ Design	—
invalid: JSON.parse in deferred callback	.test.ts:168	✅ Design	—

Go: 0; JavaScript/TypeScript: 5 (*.test.ts via vitest).

Test inflation: Test file: +11/−11 lines (net 0); production file: +14/−1 lines. Ratio: 0.79:1. ✅ Not inflated.

⚠️ Flagged Tests (1 issue)

suggestion output is syntactically valid JavaScript (.test.ts:98) — Design / pre-existing anomaly.

The expected output: string contains \n } catch (err) { (two leading spaces before }) rather than \n} catch (err) {. For a top-level input with no leading whitespace (const result = JSON.parse(text);), buildTryCatchSuggestion should produce } catch (no spaces) because indent would be "". This expectation was already present before this PR and was carried forward unchanged; the PR correctly updated only the catch-body content.

Risk: If this it() block is actively executed by vitest, it would fail. Worth verifying that pnpm test passes in the eslint-factory/ workspace with this change.

Suggested fix:

// Change:
output: `try {\n  const result = JSON.parse(text);\n  } catch (err) {\n ...`
// To:
output: `try {\n  const result = JSON.parse(text);\n} catch (err) {\n ...`

Verdict

✅ Check passed. 0% implementation tests (threshold: 30%). The PR correctly updates 11 output: fixture strings across 5 it() blocks to reflect the new meaningful catch block (throw new Error(...) replacing the useless throw err). All test changes verify user-visible auto-fix behavior — these are design tests. No Go mock violations. No missing build tags. One pre-existing expected-output anomaly was preserved rather than fixed; this should be addressed in a follow-up.

References:

• §28468632381

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 77.1 AIC · ⌖ 27.2 AIC · ⊞ 7K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 95/100. 0% implementation tests (threshold: 30%). All 5 modified test blocks verify behavioral contracts (the auto-fix suggestion output). No Go mock violations. No missing build tags. One pre-existing indentation anomaly flagged in the comment above.

[Copilot]

@copilot run pr-finisher skill

Done in d41e2b4: preserved the original JSON.parse error as cause, added coverage for the indented suggestion path, and ran the PR-finisher validation pass.

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.1