Repository navigation
fix(core): keep the connection up when a request POST is HTTP-refused - #2777
Merged
Merged
Conversation
The SDK's StreamableHTTP _send reports a failed POST through onerror and then rethrows it to the sender. InspectorClient's onerror errored the whole connection, so on the Node transport a refused subscriptions/listen took the session down before #2722's refused stream status could show, and the TUI's 'Listen stream: refused by the server' was unreachable. A deterministic HTTP refusal (the #2722 classifier: ClientHttpNotImplemented with 400/403/404/405, a code the SDK throws only from its POST path) now leaves the status alone; the request's rejection already surfaces it. Everything else reported through onerror still errors the connection. Closes #2776 Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Signed-off-by: cliffhall <[email protected]>
There was a problem hiding this comment.
🟢 Approval recommended
The narrow guard matches the requested behavior and has focused regression coverage with no unresolved findings.
No open issues 🎉
What changed in this PR
Keeps Node transport sessions connected when a request POST receives a deterministic HTTP refusal, allowing subscription refusal notices to remain visible.
Changes:
- Reuses the existing refusal classifier without changing handling of other transport errors.
- Adds unit and integration assertions for connection status and refusal reporting.
| File | Description |
|---|---|
| core/mcp/inspectorClient.ts | Ignores request-level HTTP refusals in the connection error handler. |
| clients/web/src/test/integration/mcp/inspectorClient-subscriptions-era.test.ts | Verifies refused subscriptions leave real sessions connected. |
| clients/web/src/test/core/mcp/inspectorClient-peer-handler-timing.test.ts | Tests refusal statuses and continued handling of fatal errors. |
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2776
Problem
The SDK's
StreamableHTTPClientTransport._sendreports a failed POST throughonerrorand then rethrows it to the sender.InspectorClient'sbaseTransport.onerrortreated every report as a failed connection and set the status toerror.So on the Node transport, any request the server refused with a bare HTTP status took the whole session down, even though the request's own rejection already surfaces it. For #2722 that meant the TUI's new
Listen stream: refused by the servercould not be reached: the connect-time listen 403 errored the client before the Subscriptions tab could show. PR #2730 called this out as pre-existing and out of scope, and the v2.12.0 smoke (#2764) recorded it as partial.Change
core/mcp/inspectorClient.ts:onerrorreturns early, leaving the status alone, whensubscriptionListenRefusal(error)classifies the error as a deterministic HTTP refusal: anSdkHttpErrorwithClientHttpNotImplementedand status 400, 403, 404 or 405. The SDK throws that code only from its POST path, so the error is always also delivered to the sender as a rejection. Everything else reported throughonerrorstill errors the connection, as before: an SSE GET stream failing, a dropped connection, a 5xx, an auth failure. The web client is unaffected, because its remote transport doesn't route send failures throughonerror.Tests
inspectorClient-peer-handler-timing.test.ts, newtransport onerror (#2776)block):onerrorwith a POST refusal at 400, 403, 404 or 405 keeps the clientconnectedand dispatches noerrorevent. A 503 POST failure, an auth failure and a plain transportErrorstill seterrorand dispatch it.inspectorClient-subscriptions-era.test.ts, against the real refusing server): the bare-403 subscribe and the connect-time listen cases now also assertgetStatus()isconnected, and the connect-time case asserts the stored refusal. The comment above the stubbed block is updated.Verified by hand
TUI in a real pty (160×50) against
subscriptions-listen-refused-http.jsonon the modern era, keysc(connect) thenu(Subscriptions):● connected✗ errorResources (3) ▶ Subscriptions (0) … Protocol (4)Resources (0)Not subscribed/Listen stream: refused by the serverError / Error POSTing to endpoint: ForbiddenProtocol (4)(nothing re-sent)npm run local:gate: green (exit 0).🤖 Generated with Claude Code