[eslint-miner] fix(eslint-factory): recognize .addListener alias and req.on("response", cb) idiom in require-http-response-error-listener - #59679
Conversation
…se\", cb) idiom
require-http-response-error-listener (the only rule at "error" severity)
missed two standard Node.js EventEmitter/HTTP idioms:
- .addListener("error", ...) is a documented alias for .on(), but was not
recognized as satisfying the response-error-listener check.
- req.on("response", cb) — registering the response listener separately
from the inline request callback — caused getResponseCallback() to
return null, skipping the check entirely for that call site.
Both idioms are now detected: isErrorListenerCall() accepts
"on"/"once"/"addListener", and a new getResponseEventCallback() helper
recognizes the req.on("response", cb)/.once(...)/.addListener(...) idiom
(including the chained http.request(...).on("response", cb) form), scoped
to requests statically resolved to Node's http/https module.
Re-verified against the 4 known live call sites in actions/setup/js
(start_mcp_gateway.cjs, mount_mcp_as_cli.cjs, mcp_cli_bridge.cjs,
handle_agent_failure.cjs) — all remain true negatives (npm run
lint:setup-js still reports the rule at 0 errors).
Fixes #59646
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🔬 Test Quality Sentinel is analyzing test quality on this pull request... |
|
✅ Ponytail Reviewer completed successfully! Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
|
🔍 Design Decision Gate 🏗️ is checking for design decision records on this pull request... |
|
🧠 Matt Pocock Skills Reviewer is reviewing this pull request using Matt Pocock's engineering skills... |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail pass focused on simplification opportunities in changed lines.
net: -13 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #59679 · codex · gpt53codex · 4.59 AIC · ⌖ 3.17 AIC · ⊞ 13.9K
Comment /ponytail to run again
| if (callee.object.type !== AST_NODE_TYPES.Identifier || callee.object.name !== name) return false; | ||
| if (callee.property.type !== AST_NODE_TYPES.Identifier || (callee.property.name !== "on" && callee.property.name !== "once")) return false; | ||
| if (callee.property.type !== AST_NODE_TYPES.Identifier || !LISTENER_METHODS.has(callee.property.name)) return false; | ||
| const firstArg = call.arguments[0]; |
There was a problem hiding this comment.
L102: shrink: manual isFromHttpRequest boolean staging with two branches. Early-return by type and inline predicate assignment, same logic in fewer lines.
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
|
|
||
| /** Reports `callback` when its declared response parameter never gets an 'error' listener attached. */ |
There was a problem hiding this comment.
L174: yagni: separate responseName temp used once for lookup. Use param.name directly in find(...) to remove a dead local.
| return grandparent !== undefined && grandparent.type === AST_NODE_TYPES.CallExpression && grandparent.callee === parent && isErrorListenerCall(grandparent, responseName); | ||
| }); | ||
|
|
||
| if (!hasErrorListener) { |
There was a problem hiding this comment.
L191: shrink: guarded call split across temp + if. getResponseCallback(node) can be checked-and-called inline to cut ceremony.
There was a problem hiding this comment.
🟡 Changes recommended
Reassigned request bindings can produce false-positive errors on unrelated objects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extends the HTTP response error-listener ESLint rule to recognize additional Node.js listener idioms.
Changes:
- Supports
.addListener("error", ...). - Checks response handlers registered through request events.
- Adds focused rule tests.
File summaries
| File | Description |
|---|---|
require-http-response-error-listener.ts |
Adds listener detection and shared callback validation. |
require-http-response-error-listener.test.ts |
Tests new valid and invalid patterns. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if (def.type !== "Variable") return false; | ||
| const declarator = def.node as TSESTree.VariableDeclarator; | ||
| if (declarator.id.type !== AST_NODE_TYPES.Identifier) return false; | ||
| return declarator.init !== null && declarator.init !== undefined && declarator.init.type === AST_NODE_TYPES.CallExpression && isHttpRequestCall(declarator.init, sourceCode); |
|
@copilot PR sous-chef triage for #59679. Please address the current forward-progress blockers and then run the Open review feedback remains from Copilot review:
Failed checks from the candidate set:
Please push a fix and rerun the relevant checks, then use the
|
…reating them as HTTP requests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the review feedback in 7d1cbc7 (latest push): On the failed |
|
🎉 This pull request is included in a new release. Release: |
Summary
Closes a soundness gap in
require-http-response-error-listener— the only ESLint rule in this repo promoted to"error"severity — where two standard Node.js idioms caused the rule to silently skip its check:.addListener("error", ...)— a documented alias for.on()in Node's EventEmitter API — was not recognized as registering an error listener.req.on("response", cb)(registering the response handler as a separate listener on the request object, rather than passing it as the inline callback to.request()/.get()) causedgetResponseCallback()to returnnull, so the rule performed no check at all for that call site.Both are latent/ungrounded gaps (see #59646) — not currently causing false negatives on any live code in
actions/setup/js, but filed proactively because this rule's elevated severity makes any false negative here higher-impact, and both idioms are standard patterns likely to appear as the codebase grows.Changes
eslint-factory/src/rules/require-http-response-error-listener.ts:isErrorListenerCall()now accepts"on","once", and"addListener"(previously only"on"/"once").getResponseEventCallback()helper detects thereq.on("response", cb)idiom (and.once/.addListenervariants), including the chainedhttp.request(...).on("response", cb)form, scoped to requests statically resolved to Node'shttp/httpsmodule via the existingisHttpRequestCall()/isHttpRequestResultBinding()resolution logic.checkResponseCallback()helper so both the inline-callback and separate-listener code paths run the same error-listener check.docs.descriptionto document the new detection scope.eslint-factory/src/rules/require-http-response-error-listener.test.ts:.addListener("error", ...)satisfying the check (per acceptance criterion (a)).req.on("response", cb)with a missing'error'listener being flagged, and valid-case tests for the idiom with an'error'listener present — including the chained.on("response", ...).on("error", ...)form (per acceptance criteria (b) and (c)).Evidence / Validation
cd eslint-factory && npm install && npm run build— succeeds, no type errors.cd eslint-factory && npx vitest run— 675/675 tests pass (8/8 in the updated rule test file).cd eslint-factory && npm run lint:setup-js— 0 errors reported bygh-aw-custom/require-http-response-error-listener(63 pre-existing unrelated warnings from other rules remain untouched). Confirms the 4 known live call sites (start_mcp_gateway.cjs:560,mount_mcp_as_cli.cjs:245,mcp_cli_bridge.cjs:240,handle_agent_failure.cjs:1820) remain true negatives — no regression.Acceptance criteria (from #59646)
isErrorListenerCall()also recognizes.addListener("error", ...).req.on("response", callback)idiom and applies the same error-listener check to that callback's response parameter..addListener("error", ...)satisfying the check, (b)req.on("response", (res) => {...})with a missing'error'listener flagged, (c) same idiom with a present'error'listener not flagged.Fixes #59646