[eslint-miner] feat(eslint): add require-error-cause-in-rethrow rule - #43758
Conversation
Adds a new ESLint rule that detects `new Error()` calls inside catch
blocks that reference the caught variable (directly or via
getErrorMessage()) but omit `{ cause: caughtVar }` as the second
argument.
## Evidence
Scanning actions/setup/js found 25 violations across 18 production
files. Representative examples:
- apply_samples.cjs:67 — `throw new Error(`${ERR_PARSE}: ...${getErrorMessage(err)}`)`
- safe_output_manifest.cjs — three file-write helpers swallow original fs errors
- handle_agent_failure.cjs, route_slash_command.cjs, merge_remote_agent_github_folder.cjs
All of these create a new Error with the message from the caught
error but discard the original stack trace, making debugging harder
in production action runs.
## Rule details
- **Name**: `require-error-cause-in-rethrow`
- **Type**: problem
- **Auto-fixable**: hasSuggestions (appends `, { cause: err }`)
- **Scope**: only `new Error()` (not subclasses), only inside catch
blocks, only when the caught variable appears in the message arg
- **False-positive risk**: very low — requires both a catch binding AND
the variable to appear in the Error message
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #43758 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds a new custom ESLint rule to the eslint-factory plugin to warn when new Error(...) is created in a catch block with a message that references the caught error, but omits { cause: caughtVar }, which would otherwise preserve the original error chain/stack.
Changes:
- Introduces
require-error-cause-in-rethrowrule implementation with an autofix suggestion. - Adds a new rule test suite validating expected valid/invalid patterns and suggestion output.
- Registers the rule in the plugin entrypoint and enables it (warn) in the eslint-factory config.
Show a summary per file
| File | Description |
|---|---|
| eslint-factory/src/rules/require-error-cause-in-rethrow.ts | Implements the new rule logic + suggestion fixer. |
| eslint-factory/src/rules/require-error-cause-in-rethrow.test.ts | Adds unit tests for the new rule behavior and suggestion output. |
| eslint-factory/src/index.ts | Exposes the new rule via the plugin’s rules export. |
| eslint-factory/eslint.config.cjs | Enables the new rule at warn severity. |
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: 6
- Review effort level: Low
| function expressionReferencesCatchVar(node: TSESTree.Expression, varName: string): boolean { | ||
| if (node.type === AST_NODE_TYPES.Identifier && node.name === varName) return true; | ||
| if (node.type === AST_NODE_TYPES.CallExpression) { |
There was a problem hiding this comment.
Addressed in 8d48654: expressionReferencesCatchVar now handles MemberExpression so patterns like ${err.message} are detected, with regression coverage added.
| missingCause: | ||
| "`new Error(...)` inside catch({{catchVar}}) references {{catchVar}} but omits `{ cause: {{catchVar}} }` — the original stack trace will be lost. " + | ||
| "Add `{ cause: {{catchVar}} }` as the second argument.", |
There was a problem hiding this comment.
Addressed in 8d48654: updated diagnostic wording to catch ({{catchVar}}).
| NewExpression(node) { | ||
| // Only flag `new Error(...)` — not subclasses like `new TypeError(...)`. |
There was a problem hiding this comment.
Addressed in 8d48654: the rule now only reports throw new Error(...) by requiring the NewExpression parent to be a ThrowStatement.
| for (let i = ancestors.length - 1; i >= 0; i--) { | ||
| const a = ancestors[i]; | ||
| if (a.type === AST_NODE_TYPES.FunctionDeclaration || a.type === AST_NODE_TYPES.FunctionExpression) { | ||
| // Crossed a function boundary — outer catch no longer in scope for execution | ||
| return false; | ||
| } | ||
| if (a.type === AST_NODE_TYPES.CatchClause) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Addressed in 8d48654: ArrowFunctionExpression is now treated as a deferred execution boundary in isInsideCatchBody.
| // Direct catch var reference with cause | ||
| `try { doSomething(); } catch (err) { throw new Error(\`Outer: \${err.message}\`, { cause: err }); }`, | ||
| // Nested function inside catch — should NOT flag (deferred execution boundary) | ||
| `try { doSomething(); } catch (err) { const fn = function() { throw new Error(\`msg: \${getErrorMessage(err)}\`); }; }`, | ||
| ], |
There was a problem hiding this comment.
Addressed in 8d48654: added valid regression cases for nested arrow functions and non-throw new Error(...) inside catch.
| { | ||
| // Direct reference to catch var in message (not via getErrorMessage) | ||
| code: `try { doSomething(); } catch (err) { throw new Error(\`Failed: \${err}\`); }`, | ||
| errors: [ |
There was a problem hiding this comment.
Addressed in 8d48654: added an invalid test case for member-access references (${err.message}) and verified suggestion output.
🧪 Test Quality Sentinel Report
📊 Metrics (3 tests)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Test Quality Sentinel: 77/100. 33.3% implementation tests (threshold: 30%). The "uses the correct docs URL" test asserts a string constant with no behavioral contract — removing or converting it to a pattern assertion would drop the ratio to 0% and make this excellent. Review flagged tests in the comment above.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on correctness gaps and test coverage.
📋 Key Themes & Findings
Key Themes
- Correctness gaps (3 issues):
expressionReferencesCatchVarmissesMemberExpression(err.messagepatterns),hasCausePropertyrejects valid non-identity cause expressions, andArrowFunctionExpressionis not treated as an execution boundary despite the comment saying it should be. - Fixer fragility: Text-slicing the options object to insert
causebreaks on multiline objects and trailing commas. - Test coverage gaps: Arrow-function boundary and Error-subclass-without-cause cases are undocumented.
Positive Highlights
- ✅ Evidence-driven motivation — 25 real violations across 18 production files
- ✅ Low false-positive design: dual guard (catch binding + catch var in message)
- ✅ Good deferred-execution awareness for
FunctionDeclaration/FunctionExpression - ✅ Well-structured test suite with valid/invalid cases and suggestion output verification
- ✅ Clean integration into
index.tsandeslint.config.cjs
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 94 AIC · ⌖ 6.64 AIC · ⊞ 6.7K
Comment /matt to run again
| // protects the node (deferred execution). | ||
| for (let i = ancestors.length - 1; i >= 0; i--) { | ||
| const a = ancestors[i]; | ||
| if (a.type === AST_NODE_TYPES.FunctionDeclaration || a.type === AST_NODE_TYPES.FunctionExpression) { |
There was a problem hiding this comment.
[/codebase-design] ArrowFunctionExpression is missing from the execution-boundary barrier, but the comment above (line 100–108) states _non-arrow
There was a problem hiding this comment.
Addressed in 8d48654: arrow functions are now included in execution-boundary checks to avoid deferred-callback false positives.
| if (secondArg.type === AST_NODE_TYPES.ObjectExpression) { | ||
| // Add cause property to the existing object | ||
| const existingText = sourceCode.getText(secondArg); | ||
| // Strip braces and trim |
There was a problem hiding this comment.
[/tdd] The fixer uses raw string slicing (existingText.slice(1, -1).trim()) to reconstruct the options object, which breaks for multiline objects, trailing commas, or objects with inline comments.
💡 Example breakage and suggested fix
For a real-world multiline input:
throw new Error(`msg: ${err}`, {
// retries exceeded
code: 500,
});The fixer produces: { cause: err, // retries exceeded\n code: 500, } — a trailing comma and embedded comment will likely cause a parse error.
A node-level insertion is safer:
const firstProp = secondArg.properties[0];
if (firstProp) {
return fixer.insertTextBefore(firstProp, `cause: ${catchVarName}, `);
} else {
return fixer.replaceText(secondArg, `{ cause: ${catchVarName} }`);
}Please also add a test with a multiline options object to prevent regressions.
@copilot please address this.
There was a problem hiding this comment.
Addressed in bddffaa: replaced slice-based options reconstruction with insertion-based fixer edits for existing object literals.
| const isKeyNamed = (key.type === AST_NODE_TYPES.Identifier && key.name === "cause") || (key.type === AST_NODE_TYPES.Literal && key.value === "cause"); | ||
| if (!isKeyNamed) return false; | ||
| const value = prop.value; | ||
| return value.type === AST_NODE_TYPES.Identifier && value.name === catchVarName; |
There was a problem hiding this comment.
[/tdd] hasCauseProperty only returns true when the cause value is exactly the catch variable identifier — { cause: err }. It misses valid and common patterns like { cause: new Error('wrap', { cause: err }) } or { cause: someOtherVar }, both of which legitimately preserve error chains. The rule would flag these as violations when they should be accepted.
💡 Suggested relaxation
Consider accepting any non-null expression as the cause value (not just the exact catch variable), since any cause is better than none:
function hasCauseProperty(optionsArg: TSESTree.Expression): boolean {
if (optionsArg.type !== AST_NODE_TYPES.ObjectExpression) return false;
return optionsArg.properties.some(prop => {
if (prop.type !== AST_NODE_TYPES.Property) return false;
const key = prop.key;
return (
(key.type === AST_NODE_TYPES.Identifier && key.name === 'cause') ||
(key.type === AST_NODE_TYPES.Literal && key.value === 'cause')
);
});
}Add a test case like throw new Error(${err}, { cause: wrapError(err) }) to document this behaviour.
@copilot please address this.
There was a problem hiding this comment.
Addressed in bddffaa: hasCauseProperty now treats any explicit cause property as satisfying the rule, including wrapped expressions.
| const callee = node.callee; | ||
| if (callee.type !== AST_NODE_TYPES.Identifier || callee.name !== "Error") return; | ||
|
|
||
| const frame = innermostCatch(); |
There was a problem hiding this comment.
[/codebase-design] innermostCatch() is called at line 150 but isInsideCatchBody() (line 152) independently re-walks the ancestor chain to find the same catch clause. Both calls read catchStack and the ancestor tree but neither one is the sole source of truth. This double-check creates confusion: if the two disagree (e.g. due to a bug), the error silently disappears.
💡 Suggested simplification
Since isInsideCatchBody already validates that we're inside a catch clause, innermostCatch is redundant for the guard and its result (catchVarName) can be derived from the ancestor check directly. Or, make isInsideCatchBody return the catch variable name (or null), removing the need for the separate stack entirely:
function catchVarForNode(node: TSESTree.Node): string | null {
const ancestors = sourceCode.getAncestors(node);
for (let i = ancestors.length - 1; i >= 0; i--) {
const a = ancestors[i];
if (a.type === AST_NODE_TYPES.FunctionDeclaration || a.type === AST_NODE_TYPES.FunctionExpression) return null;
if (a.type === AST_NODE_TYPES.CatchClause) {
const param = a.param;
return param?.type === AST_NODE_TYPES.Identifier ? param.name : null;
}
}
return null;
}This eliminates the stack entirely, making the rule stateless and easier to reason about.
@copilot please address this.
There was a problem hiding this comment.
No functional change for this note. I kept the current approach for minimal scope while fixing the correctness issues raised in this PR.
| // Direct catch var reference with cause | ||
| `try { doSomething(); } catch (err) { throw new Error(\`Outer: \${err.message}\`, { cause: err }); }`, | ||
| // Nested function inside catch — should NOT flag (deferred execution boundary) | ||
| `try { doSomething(); } catch (err) { const fn = function() { throw new Error(\`msg: \${getErrorMessage(err)}\`); }; }`, |
There was a problem hiding this comment.
[/tdd] The test that validates the nested-function boundary only covers FunctionExpression — it does not test ArrowFunctionExpression, which the implementation does NOT treat as a boundary. Without a test for arrow functions, the current (potentially surprising) behaviour — that catch (err) { const fn = () => { throw new Error(${err}) }; } IS flagged — is undocumented and unverified.
💡 Suggested additions
Add explicit valid/invalid test cases for arrow functions to pin the intended behaviour:
// Should this be valid (deferred, not in-scope) or invalid (flagged)?
`try { doSomething(); } catch (err) { const fn = () => { throw new Error(`msg: ${getErrorMessage(err)}`); }; }`The test will both document the decision and prevent future regressions if the boundary logic changes.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 8d48654: added a dedicated valid case for nested ArrowFunctionExpression inside catch.
| // Template literal with cause | ||
| `try { doSomething(); } catch (err) { throw new Error(\`Failed: \${getErrorMessage(err)}\`, { cause: err }); }`, | ||
| // Cause with extra properties | ||
| `try { doSomething(); } catch (err) { throw new Error("msg: " + getErrorMessage(err), { cause: err, extra: 1 }); }`, |
There was a problem hiding this comment.
[/tdd] The valid cases don't include a test for new TypeError(...) (subclass) without a cause, which is a notable gap — the rule description says it only flags new Error(), not subclasses. Without an explicit invalid subclass test that stays valid (no flag), a future change that accidentally extends the rule to subclasses would pass unnoticed.
💡 Suggested addition
Add to the valid array:
// Error subclass WITHOUT cause — rule must NOT flag this
`try { doSomething(); } catch (err) { throw new TypeError(`Failed: ${getErrorMessage(err)}`); }`,This pins the intentional narrowing to Error only.
@copilot please address this.
There was a problem hiding this comment.
Addressed in bddffaa: added a valid case for new TypeError(...) without cause to lock rule scope to new Error(...) only.
| */ | ||
| function expressionReferencesCatchVar(node: TSESTree.Expression, varName: string): boolean { | ||
| if (node.type === AST_NODE_TYPES.Identifier && node.name === varName) return true; | ||
| if (node.type === AST_NODE_TYPES.CallExpression) { |
There was a problem hiding this comment.
[/codebase-design] expressionReferencesCatchVar recurses into CallExpression arguments and template literals, but misses MemberExpression — so err.message, err.code, or err.stack in the error message are NOT detected. For example, throw new Error('Prefix: ' + err.message) inside a catch block would NOT be flagged, even though err is clearly referenced.
💡 Suggested addition
Add a MemberExpression case:
if (node.type === AST_NODE_TYPES.MemberExpression) {
const obj = node.object;
if (obj.type !== AST_NODE_TYPES.PrivateIdentifier && expressionReferencesCatchVar(obj, varName)) return true;
}And add tests:
// Should be flagged (err.message in message, no cause)
`try { doSomething(); } catch (err) { throw new Error('Prefix: ' + err.message); }`
// Should pass (err.message in message, with cause)
`try { doSomething(); } catch (err) { throw new Error('Prefix: ' + err.message, { cause: err }); }`@copilot please address this.
There was a problem hiding this comment.
Addressed in 8d48654: member-access references (err.message, computed members) are now traversed and detected.
There was a problem hiding this comment.
REQUEST_CHANGES — 3 correctness bugs and 3 design issues must be fixed before merge
The rule idea is solid and the test structure is good, but the current implementation has correctness problems that will produce both false positives (valid code flagged) and false negatives (real violations missed) in production.
🔴 Blocking issues (3)
-
Arrow functions not blocked as execution barriers (line 119) —
isInsideCatchBodyexits early forFunctionDeclaration/FunctionExpressionbut notArrowFunctionExpression. Arrow callbacks inside a catch are deferred — the catch may already have unwound before they run. The block comment explicitly promises this won't be flagged, but the code does the opposite. -
err.messageanderr.toString()silently missed (line 29) —expressionReferencesCatchVardoesn't handleMemberExpression. The single most common real-world rethrow pattern (new Error(err.message)) never triggers the rule. -
hasCausePropertyrejects wrapped cause expressions (line 66) — Only a bare identifier is accepted as thecausevalue. Code like{ cause: ensureError(err) }or{ cause: err as Error }is treated as missing cause and falsely flagged.
🟡 Non-blocking but worth fixing (3)
-
Block comment describes non-existent sentinel mechanism (line 104) — misleads future maintainers.
-
Fixer uses
slice(1,-1)string manipulation (line 185) — collapses multi-line object formatting and may produce trailing-comma violations. -
Nested catches use wrong frame for variable lookup (line 150) —
innermostCatch()returns the innermost frame, but the rule should check whether any active catch frame's variable appears in the message.
🔎 Code quality review by PR Code Quality Reviewer · 128.8 AIC · ⌖ 9.06 AIC · ⊞ 5.5K
Comment /review to run again
| // protects the node (deferred execution). | ||
| for (let i = ancestors.length - 1; i >= 0; i--) { | ||
| const a = ancestors[i]; | ||
| if (a.type === AST_NODE_TYPES.FunctionDeclaration || a.type === AST_NODE_TYPES.FunctionExpression) { |
There was a problem hiding this comment.
Arrow functions are NOT blocked by this check — false positives for deferred callbacks.
isInsideCatchBody only exits early for FunctionDeclaration and FunctionExpression. It never checks ArrowFunctionExpression. The block comment above (line 104) explicitly promises arrow functions inside a catch won't be flagged ("arrow functions that are not immediately-invoked are deferred"), but the code does the opposite — this produces a false positive:
try { doSomething(); } catch (err) {
const fn = () => { throw new Error(`msg: ${err}`); };
// ^^^^ incorrectly flagged — arrow fn is deferred execution
}The test on line 35 of .test.ts only covers function(), not () =>, so this gap is untested.
💡 Suggested fix
Add ArrowFunctionExpression to the bailout in isInsideCatchBody:
if (
a.type === AST_NODE_TYPES.FunctionDeclaration ||
a.type === AST_NODE_TYPES.FunctionExpression ||
a.type === AST_NODE_TYPES.ArrowFunctionExpression
) {
return false;
}Add a corresponding valid test case to the test file:
// Arrow function inside catch — deferred, should NOT flag
`try { doSomething(); } catch (err) { const fn = () => { throw new Error(\`msg: \${err}\`); }; }`,There was a problem hiding this comment.
Addressed in 8d48654: arrow functions are now treated as deferred boundaries, so deferred callback throws are not flagged.
| const isKeyNamed = (key.type === AST_NODE_TYPES.Identifier && key.name === "cause") || (key.type === AST_NODE_TYPES.Literal && key.value === "cause"); | ||
| if (!isKeyNamed) return false; | ||
| const value = prop.value; | ||
| return value.type === AST_NODE_TYPES.Identifier && value.name === catchVarName; |
There was a problem hiding this comment.
hasCauseProperty rejects any cause value that isn't a bare identifier — false positives.
The check requires prop.value.type === Identifier && prop.value.name === catchVarName. Any non-trivial cause expression is rejected and the rule reports a violation on code that already has a cause:
try { doSomething(); } catch (err) {
// All of these incorrectly trigger the rule:
throw new Error(msg, { cause: ensureError(err) }); // ← CallExpression
throw new Error(msg, { cause: err instanceof Error ? err : new Error(String(err)) }); // ← ConditionalExpression
throw new Error(msg, { cause: err as Error }); // ← TSAsExpression
}This is a meaningful false positive for TypeScript codebases that assert or wrap the caught value before chaining.
💡 Suggested fix
Relax the value check to accept any expression that contains a reference to the catch variable, using the same expressionReferencesCatchVar helper:
function hasCauseProperty(optionsArg: TSESTree.Expression, catchVarName: string): boolean {
if (optionsArg.type !== AST_NODE_TYPES.ObjectExpression) return false;
return optionsArg.properties.some(prop => {
if (prop.type !== AST_NODE_TYPES.Property) return false;
const key = prop.key;
const isKeyNamed =
(key.type === AST_NODE_TYPES.Identifier && key.name === "cause") ||
(key.type === AST_NODE_TYPES.Literal && key.value === "cause");
if (!isKeyNamed) return false;
// Accept any value that references the catch var (not just bare identifier)
const value = prop.value;
if (value.type === AST_NODE_TYPES.Identifier && value.name === catchVarName) return true;
if (value.type !== AST_NODE_TYPES.PrivateIdentifier) {
return expressionReferencesCatchVar(value as TSESTree.Expression, catchVarName);
}
return false;
});
}There was a problem hiding this comment.
Addressed in bddffaa: explicit cause presence now passes regardless of value expression, covering wrapped-cause patterns.
| * Returns true if `node` is syntactically inside the try/catch clause | ||
| * referenced by the current `catchStack` top-frame — i.e., we have not | ||
| * crossed an intervening function boundary that would create a new execution | ||
| * context (arrow functions that are *not* immediately-invoked are deferred). |
There was a problem hiding this comment.
The block comment describes an implementation that doesn't exist.
Lines 104–109 say the rule "inserts a barrier sentinel into catchStack whenever we enter a non-arrow function body". No such insertion ever happens — the visitor only registers CatchClause, CatchClause:exit, and NewExpression. The actual mechanism (ancestor walking in isInsideCatchBody) is never explained.
This is misleading for future maintainers: they'll look for non-existent sentinel pushes in the visitor, miss the real logic, and likely introduce bugs or skip the ancestor-walk when refactoring.
💡 Suggested fix
Replace the comment with an accurate description:
/**
* Returns true if `node` is directly inside a catch clause (not separated
* by a function boundary that would defer execution). Specifically:
* - walks ancestors from innermost to outermost
* - returns false if a FunctionDeclaration, FunctionExpression, or
* ArrowFunctionExpression is encountered before a CatchClause
* - returns true once a CatchClause ancestor is found
*/There was a problem hiding this comment.
Addressed in 8372478: updated the isInsideCatchBody comment to match the actual ancestor-walk implementation.
| if (args.length >= 2) { | ||
| const secondArg = args[1]; | ||
| if (secondArg.type === "SpreadElement") return null; | ||
| if (secondArg.type === AST_NODE_TYPES.ObjectExpression) { |
There was a problem hiding this comment.
Fixer's slice(1,-1) string-slices the object literal — corrupts multi-line formatting and trailing commas.
The suggestion fixer extracts object contents by slicing braces from raw source text, then concatenates inline. This:
- Collapses all whitespace and newlines in multi-property objects, producing a noisy suggestion diff.
- If the original object has a trailing comma (
{ code: 500, }), the output is{ cause: err, code: 500, }— valid JS/TS but may triggercomma-danglelinting rules. - Inline comments inside the object (
{ /* tracking */ code: 500 }) are preserved literally but may produce unexpected output.
// original
new Error(msg, {
code: 500, // tracking
})
// suggested fix produces
new Error(msg, { cause: err, code: 500, // tracking
})💡 Suggested fix
Use the AST fixer API to insert only the cause property, rather than reconstructing the object as a string. Insert before the first existing property:
if (secondArg.type === AST_NODE_TYPES.ObjectExpression) {
if (secondArg.properties.length === 0) {
// empty object: replace entire token range between braces
return fixer.replaceText(secondArg, `{ cause: ${catchVarName} }`);
}
// Insert before the first property
const firstProp = secondArg.properties[0];
return fixer.insertTextBefore(firstProp, `cause: ${catchVarName}, `);
}There was a problem hiding this comment.
Addressed in bddffaa: removed object text slicing and switched to insertion-based edits in the existing options object.
| const callee = node.callee; | ||
| if (callee.type !== AST_NODE_TYPES.Identifier || callee.name !== "Error") return; | ||
|
|
||
| const frame = innermostCatch(); |
There was a problem hiding this comment.
Nested catches with different variable names cause the rule to miss outer-variable references.
innermostCatch() returns the top frame of catchStack — the innermost catch. When a new Error inside an inner catch references the outer catch's variable, innermostCatch() returns the inner frame, and expressionReferencesCatchVar tests against the wrong name:
try {
doSomething();
} catch (outerErr) {
try {
doOther();
} catch (innerErr) {
// innermostCatch() returns { varName: 'innerErr' }
// expressionReferencesCatchVar checks for 'innerErr'
// but message references outerErr — violation is silently missed
throw new Error(`outer: ${getErrorMessage(outerErr)}, inner: ${getErrorMessage(innerErr)}`);
}
}💡 Impact and fix notes
The ancestor-walk in isInsideCatchBody already correctly identifies which CatchClause ancestor is active. The right fix is to derive the catch variable name from the ancestor walk rather than from catchStack. Alternatively, walk catchStack to find the frame whose varName appears in the message expression, but this could report against the wrong frame.
The simplest correct approach: use sourceCode.getAncestors(node) to find the innermost CatchClause ancestor, extract its param.name, and use that — making catchStack unnecessary for the message-check logic entirely.
There was a problem hiding this comment.
No change for this item. The rule intentionally uses the innermost catch binding to keep diagnostics tied to the active catch variable and avoid cross-catch ambiguity.
There was a problem hiding this comment.
Review: require-error-cause-in-rethrow ESLint rule
Good rule concept — catches a real and recurring bug (silent stack-trace loss). The core detection logic, test coverage, and rule registration are all solid. A few issues need addressing before merge:
Blocking issues (3):
-
hasCausePropertyis too narrow (line 70) — it only acceptscauseequal to the exact catch variable identifier, so common wrapping idioms like{ cause: new Error(err.message) }still trigger the rule. This will produce false-positive warnings in real code. -
Arrow functions not treated as execution barriers (line 113) — deferred arrow functions (e.g. inside
setTimeout(() => { throw new Error(msg); })) are incorrectly flagged because onlyFunctionDeclaration/FunctionExpressionblock the catch walk. A test case for this boundary is also missing. -
Unsafe text-splice in the suggestion fixer (line 154) —
existingText.slice(1, -1)is fragile: trailing commas, embedded comments, or non-standard spacing can corrupt the emitted fix. Use AST-level property insertion instead.
Non-blocking:
- The JSDoc on
isInsideCatchBody(line 108) describes a 'catchStack barrier' approach that was never implemented; update the comment to match the actual ancestor-traversal logic.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 109.1 AIC · ⌖ 5.27 AIC · ⊞ 4.9K
| * | ||
| * We track this by inserting a "barrier" sentinel into catchStack whenever we | ||
| * enter a non-arrow function body, so that the innermost catch frame above | ||
| * the barrier is invisible to nodes inside the nested function. |
There was a problem hiding this comment.
The JSDoc comment on isInsideCatchBody describes a 'barrier sentinel inserted into catchStack' approach, but the actual implementation walks sourceCode.getAncestors(node) instead — no sentinel is ever pushed. The comment is misleading and should be updated to describe the ancestor-traversal approach actually used. @copilot please address this.
There was a problem hiding this comment.
Addressed in 8372478: revised the JSDoc so it no longer references a sentinel mechanism and now reflects ancestor traversal.
| function isInsideCatchBody(node: TSESTree.Node): boolean { | ||
| const frame = innermostCatch(); | ||
| if (!frame) return false; | ||
| const ancestors = sourceCode.getAncestors(node); |
There was a problem hiding this comment.
Arrow functions are not treated as barriers here, but they can still cause false-positives. Consider catch (err) { setTimeout(() => { throw new Error(msg ${err}); }, 0); } — the arrow function is deferred and the original catch is already done by the time it runs, yet the rule would flag it. The test suite even documents a 'valid' case for const fn = function() {} but omits an equivalent arrow-function case. Consider also treating ArrowFunctionExpression as a barrier (or add a test that documents the current intentional behavior). @copilot please address this.
There was a problem hiding this comment.
Addressed in 8d48654: arrow functions are now treated as function boundaries, preventing deferred-execution false positives.
| if (!frame || !frame.varName) return; | ||
| if (!isInsideCatchBody(node)) return; | ||
|
|
||
| const catchVarName = frame.varName; |
There was a problem hiding this comment.
The suggestion fixer strips slice(1, -1) off the existing object text and prepends cause. This breaks if the source object has trailing whitespace, comments, or a trailing comma: e.g. { code: 500, /* note */ } becomes { cause: err, code: 500, /* note */ } (double space). More critically, it produces an invalid AST if the existing text contains a leading/trailing comment like /* @type {Opts} */{ code: 500 }. Use AST-level manipulation (insert a new Property node at position 0 of the object) instead of raw text splicing. @copilot please address this.
There was a problem hiding this comment.
Addressed in bddffaa: suggestion generation no longer uses slice(1, -1) and now inserts cause safely into object literals.
| }); | ||
| } | ||
|
|
||
| export const requireErrorCauseInRethrowRule = createRule({ |
There was a problem hiding this comment.
hasCauseProperty only accepts a cause referencing the exact catch variable. This means new Error(msg, { cause: new Error(err.message) }) — a common wrapping idiom — is still flagged as a violation even though the developer deliberately created a new cause chain. This will generate false-positive warnings in production code. Consider widening the check to: any cause property is present (regardless of value), or document this deliberate narrowness in a comment so reviewers understand the trade-off. @copilot please address this.
There was a problem hiding this comment.
Addressed in bddffaa: hasCauseProperty now accepts any explicit cause property value, including wrapped expressions.
🤖 PR Triage
Score breakdown: Impact 22 + Urgency 14 + Quality 13 Rationale: New ESLint rule
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I ran the PR finisher flow and pushed the follow-up fixes in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds a new custom ESLint rule
require-error-cause-in-rethrowto thegh-aw-customplugin. The rule enforces thatnew Error(msg)thrown inside acatchblock includes{ cause: err }when the message already references the caught variable — preventing silent loss of the original stack trace and error chain.Changes
eslint-factory/src/rules/require-error-cause-in-rethrow.ts(new)@typescript-eslint/utils(ESLintUtils.RuleCreator).throw new Error(msg)inside acatch (err)block when:err), via member access (err.message), or viagetErrorMessage(err).causeproperty is present.catchStackto track nested catch clauses; stops traversal at function/arrow-function boundaries to avoid false positives in deferred callbacks.new Error(...)— not subclasses (TypeError,RangeError, etc.).hasSuggestions: true):, { cause: err }after the last argument.cause: prependscause: err,to the existing properties.null(no fix).eslint-factory/src/rules/require-error-cause-in-rethrow.test.ts(new)RuleTestertest suite (12 valid cases, 6 invalid cases).catch {}, error subclasses, nested function/arrow-function boundaries, existingcausewith wrapped expression.error), second argument object missingcause, direct catch var reference, member access reference — each asserting the suggestion output.eslint-factory/src/index.tsrequireErrorCauseInRethrowRulein the plugin's rules map.eslint-factory/eslint.config.cjs"warn"severity in the shared config.Commits
47aa68ea9feat(eslint): add require-error-cause-in-rethrow rule8d4865431fix(eslint): tighten require-error-cause-in-rethrow detection1fc752a57chore(eslint): address follow-up review nitsbddffaa72fix(eslint): handle cause options and safer fixer insertion8372478e7chore(eslint): refine cause docs and fixer insertion guard