fix(eslint-factory): extend no-caught-error-interpolation to cover EventEmitter .on('error') handlers - #49768
Conversation
…entEmitter .on('error') handlers
- Add isInlineEventErrorHandler() helper that matches .on/'once'/.addListener with 'error' event
- Update isCaughtErrorVariableDef() to recognize first param of inline EventEmitter error listeners
- Update rule description to mention EventEmitter scope
- Add 5 new tests: valid (non-error events, named listener), invalid (.on/.once/.addListener + mcp_server_core.cjs repro)
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
Automated triage — see full report issue for details. Structured data: {
"action": "defer",
"category": "bug",
"pr_number": 49768,
"risk": "low"
}
|
|
🧠 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. |
|
✅ 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 in default business logic dirs). |
There was a problem hiding this comment.
Pull request overview
Extends the ESLint rule to detect unsafe interpolation in inline EventEmitter error handlers.
Changes:
- Recognizes
.on(),.once(), and.addListener()error handlers. - Documents the expanded detection scope.
- Adds positive and negative RuleTester coverage.
Show a summary per file
| File | Description |
|---|---|
no-caught-error-interpolation.ts |
Adds EventEmitter error-handler detection. |
no-caught-error-interpolation.test.ts |
Tests supported methods and exclusions. |
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: 2
- Review effort level: Balanced
| if (isInlineRejectionHandler(fnNode)) return true; | ||
| if (isInlineEventErrorHandler(fnNode)) { | ||
| // Only the first parameter receives the error object from an 'error' event | ||
| return fnNode.params[0] === def.name; |
| it("invalid: bare .on('error', ...) listener variable is flagged", () => { | ||
| cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, { | ||
| valid: [], | ||
| invalid: [ | ||
| { | ||
| code: `emitter.on('error', err => { log(\`event error: \${err}\`); });`, |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: no-caught-error-interpolation EventEmitter extension
This is a clean, well-scoped extension. The implementation is correct and test coverage is thorough.
Key design points verified:
isInlineEventErrorHandlermirrorsisInlineRejectionHandlerwith identical guard orderingfnNode.params[0] === def.namereference equality correctly gates flagging to the first (error) parameter onlycallee.computedguard correctly excludes bracket notation calls — conservative and consistent with existing pattern- Named function reference valid case is explicitly tested and correctly excluded (inline-only restriction)
- All three EventEmitter variants (
.on,.once,.addListener) have invalid test cases
No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.7 AIC · ⌖ 13.8 AIC · ⊞ 5.4K
There was a problem hiding this comment.
Verdict: Request changes
The new EventEmitter detection is a purely syntactic heuristic (method name on/once/addListener + literal "error") with no type information, so it will false-positive on any non-EventEmitter .on('error', fn) call (jQuery, custom pub/sub, etc.), and it silently fails to flag destructured error parameters — both are unaddressed and untested.
Themes
- False positives:
isInlineEventErrorHandlercan't distinguish NodeEventEmitterfrom unrelated.on('error', ...)APIs where the callback argument isn't anError. This should at minimum be documented as an accepted tradeoff with a covering test, or narrowed with type info. - False negatives:
fnNode.params[0] === def.namedoesn't handle destructured first parameters (.on('error', ({message}) => ...)), which silently fall through unflagged with no test to confirm this is intentional. - Test additions themselves are solid and cover the main
.on/.once/.addListener+ named-function-exclusion cases well.
🔎 Code quality review by PR Code Quality Reviewer · auto · 23.3 AIC · ⌖ 4.69 AIC · ⊞ 7.9K
Comment /review to run again
| const callee = parent.callee; | ||
| if (callee.type !== AST_NODE_TYPES.MemberExpression || callee.computed) return false; | ||
| const prop = callee.property; | ||
| if (prop.type !== AST_NODE_TYPES.Identifier) return false; |
There was a problem hiding this comment.
This heuristic matches any object's .on/.once/.addListener('error', fn) call, not just Node EventEmitters, which will produce false positives on non-Error-passing APIs.
💡 Why this matters and how to narrow it
isInlineEventErrorHandler only checks the method name and that the first argument is the string literal "error" — it has no way to distinguish a real Node EventEmitter from jQuery-style .on('error', fn) (jQuery passes a jQuery.Event, not an Error), RxJS-like observables, or arbitrary user-defined pub/sub classes with an on method. Unlike .catch/.then, which are effectively unambiguous promise-shaped names, on/once/addListener are extremely common generic method names across unrelated APIs.
Concretely, this will now force String(err)/getErrorMessage(err) wrapping on code like:
$(elem).on('error', (event) => log(`img failed: ${event}`)); // event is not an ErrorConsider either:
- Restricting to call sites where the receiver is a known
EventEmitter-typed variable (requires type information viaESLintUtils.RuleCreator+ parserServices), or - At minimum documenting this as a known limitation/tradeoff in the rule's
docs.description, and adding a test case demonstrating the accepted false-positive behavior so it's an intentional, reviewed tradeoff rather than an unverified assumption.
| if (isInlineRejectionHandler(fnNode)) return true; | ||
| if (isInlineEventErrorHandler(fnNode)) { | ||
| // Only the first parameter receives the error object from an 'error' event | ||
| return fnNode.params[0] === def.name; |
There was a problem hiding this comment.
fnNode.params[0] === def.name silently fails to flag destructured error parameters, e.g. .on('error', ({message}) => ...), leaving that case untested and its behavior unverified.
💡 Why this matters
When the first parameter is a destructuring pattern (ObjectPattern/ArrayPattern), def.name refers to the Identifier bound inside the pattern, not the pattern node itself, so it never strictly equals fnNode.params[0]. The function returns false in that branch, meaning destructured error bindings from an .on('error', ...) handler are never flagged — a silent false negative rather than a crash, but it's an edge case with no test coverage, so there's no evidence this fallthrough is the intended behavior versus an overlooked gap.
Add a test case such as:
emitter.on('error', ({message}) => log(`err: ${message}`));to confirm/document whether destructured members should be in scope for this rule, and to guard against regressions.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (5 tests)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnosing-bugs — changes look correct, with two minor test-coverage gaps flagged inline.
📋 Key Themes & Highlights
Key Themes
- Missing edge-case test: The
fnNode.params[0] === def.nameguard (suppressing over-flagging of extra handler params) has no test to protect it from future regressions. - Missing FunctionExpression test: The implementation covers both
ArrowFunctionExpressionandFunctionExpression, but only arrow-function cases appear in the new tests.
Positive Highlights
- ✅ Clean structural mirror of the existing
isInlineRejectionHandler— consistent, navigable pattern - ✅ Grounded repro from
mcp_server_core.cjs:1052included as a test fixture — excellent practice - ✅ Valid-case tests (non-error events, hoisted function refs) explicitly document the intended scope boundaries
- ✅
callee.computedcheck correctly excludesemitter['on']('error', ...)dynamic-property forms - ✅ Docs description updated to list the new scope
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 29.5 AIC · ⌖ 12 AIC · ⊞ 7.1K
Comment /matt to run again
| // Only the first parameter receives the error object from an 'error' event | ||
| return fnNode.params[0] === def.name; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
[/tdd] The fnNode.params[0] === def.name guard (lines 75–78) suppresses false-positives on extra parameters, but there is no test verifying this branch holds.
💡 Suggested test
Add a valid case to confirm that extra params in an error handler are not flagged:
it("valid: second param in .on('error', ...) is not flagged", () => {
cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, {
valid: [
`emitter.on('error', (err, context) => { log(\`ctx: \${context}\`); });`,
],
invalid: [],
});
});Without this, a future refactor could silently drop the guard and start over-flagging context.
@copilot please address this.
| }); | ||
| }); | ||
|
|
||
| it("valid: non-'error' EventEmitter event name is not flagged", () => { |
There was a problem hiding this comment.
[/tdd] The test for the named/hoisted function reference passes only because the rule never sees the inner template literal (it's inside a separate function declaration, not inside the .on callback). Consider adding a case where a FunctionExpression (not arrow) is used inline to confirm both callback forms are covered:
💡 Suggested test for inline FunctionExpression
it("invalid: inline function expression in .on('error', ...) is flagged", () => {
cjsRuleTester.run("no-caught-error-interpolation", noCaughtErrorInterpolationRule, {
valid: [],
invalid: [
{
code: `emitter.on('error', function(err) { log(\`error: \${err}\`); });`,
errors: [{ messageId: "bareErrorInterpolation", data: { errorVar: "err" } }],
},
],
});
});The implementation handles FunctionExpression via the union type, but no test exercises it.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
no-caught-error-interpolationonly recognized try/catch and promise.catch/.thenrejection handlers as "caught error" scopes, missing the structurally identical EventEmitter pattern —emitter.on('error', err => \...${err}...`)— where Node guarantees the callback receives anError` object.Changes
no-caught-error-interpolation.tsisInlineEventErrorHandler(): mirrorsisInlineRejectionHandler()but matches.on/.once/.addListenercalls where the first argument is the string literal"error"and the function is the second argumentisCaughtErrorVariableDef()to call the new helper, gating onfnNode.params[0] === def.nameso only the first parameter (the error argument) is flagged — not any additional paramsdocs.descriptionto list EventEmitter handlers as a detected scopeno-caught-error-interpolation.test.ts— 5 new cases:'error'event names (.on('data', ...)) are not flagged.on('error', onError)is not flagged (inline-only restriction, now explicitly documented).on('error', ...),.once('error', ...),.addListener('error', ...)— all flagged withString(err)suggestionGrounded repro (
mcp_server_core.cjs:1052) is now a confirmed true positive:.on('error', err => ...)handler parameters #49720