Repository navigation
fix(client): make streamable HTTP auth awaits abortable by requestSignal - #2644
claude[bot] wants to merge 8 commits into
Code review found 11 potential issues
Found 9 candidates, confirmed 11. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 7 |
| 🟣 Pre-existing | 4 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | packages/client/src/client/streamableHttp.ts:670-676 |
[quality] New catch guards bypass the isAborted() helper the same diff introduces |
| 🟡 Nit | .changeset/abortable-auth-awaits.md:1-5 |
[quality] Changeset omits the bundled resumption-observer behavior fix |
| 🟡 Nit | packages/client/src/client/auth.ts:58-65 |
New UnauthorizedContext.signal extension point: type not exported, prose docs not updated |
| 🟡 Nit | packages/client/src/client/streamableHttp.ts:619-623 |
[quality] Combined-signal ternary duplicated verbatim in _startOrAuthSse and _send |
| 🟡 Nit | packages/client/src/client/streamableHttp.ts:1076-1084 |
Resumption-branch .catch: double onerror on genuine failures, and onRequestStreamEnd never fires on resume-GET failure |
| 🟣 Pre-existing | packages/client/src/client/streamableHttp.ts:613-623 |
anySignal fallback pins one listener per completed request on the transport-lifetime signal |
Annotations
Check warning on line 676 in packages/client/src/client/streamableHttp.ts
claude / Claude Code Review
[quality] New catch guards bypass the isAborted() helper the same diff introduces
The two new `onUnauthorized` catch guards (here and the `_send` sibling at ~line 1169) write the inline `signal?.aborted === true` check that the `isAborted()` helper this same diff introduces was created to replace — its JSDoc cites exactly this check's post-await narrowing hazard, and `_commonHeaders` already uses it. Mechanical fix: replace both inline checks with `isAborted(signal)` so one idiom lands in the diff and the helper's narrowing protection covers all four new abort-check sites.
Check warning on line 5 in .changeset/abortable-auth-awaits.md
claude / Claude Code Review
[quality] Changeset omits the bundled resumption-observer behavior fix
The changeset only describes the abortable-auth work, but the final commit (2d4fbe7) also bundles a second user-visible behavior fix: the resumption branch of `_send` now threads `onresumptiontoken`/`onRequestStreamEnd` into the resumed GET, so consumers' `onresumptiontoken` callbacks start firing on resume-via-`send()` where they previously never did. Add a sentence to `.changeset/abortable-auth-awaits.md` (or a second changeset) covering the resumption-observer fix so the release notes don't s
Check warning on line 65 in packages/client/src/client/auth.ts
claude / Claude Code Review
New UnauthorizedContext.signal extension point: type not exported, prose docs not updated
The new `UnauthorizedContext.signal` extension point is invisible to the provider authors who need to adopt it: (1) `UnauthorizedContext` is not exported from `@modelcontextprotocol/client` (unlike the sibling context types `OAuthClientInformationContext` / `CrossAppAccessContext`), so consumers can't write a standalone typed `onUnauthorized` implementation; (2) the prose docs that teach `onUnauthorized` (`docs/clients/machine-auth.md:43` and the `AuthProvider` section of `docs/migration/upgrade
Check warning on line 623 in packages/client/src/client/streamableHttp.ts
claude / Claude Code Review
[quality] Combined-signal ternary duplicated verbatim in _startOrAuthSse and _send
The five-line combined-signal computation (transportSignal + the anySignal ternary) is now duplicated verbatim in _startOrAuthSse (lines 619-623) and _send (lines 1093-1097) — and this PR had to move both copies in lockstep to hoist them above their _commonHeaders calls. Consider extracting a small private helper, e.g. _combinedSignal(requestSignal?: AbortSignal): AbortSignal | undefined, called from both sites (and terminateSession as the degenerate no-requestSignal case) so the GET and POST pa
Check warning on line 1084 in packages/client/src/client/streamableHttp.ts
claude / Claude Code Review
Resumption-branch .catch: double onerror on genuine failures, and onRequestStreamEnd never fires on resume-GET failure
Genuine (non-abort) failures escaping the fire-and-forget `_startOrAuthSse` in this resumption-branch `.catch` (and the 202/initialized sibling at ~line 1259) fire `onerror` twice — `_startOrAuthSse`'s internal catch already emits before rethrowing — and on the resume path `options?.onRequestStreamEnd?.()` is never invoked when the resume GET fails outright (fetch rejection, non-401/403/405 HTTP error, exhausted auth retry), so the per-request stream dead-ends silently despite the observer threa
Check notice on line 623 in packages/client/src/client/streamableHttp.ts
claude / Claude Code Review
anySignal fallback pins one listener per completed request on the transport-lifetime signal
Pre-existing issue (not introduced by this PR, which only relocates the call sites): the non-native `anySignal` fallback (Node 20.0–20.2, where `AbortSignal.any` is absent) removes its listeners only from `cleanup()`, which runs solely when one of the input signals aborts — so on the common path where a request completes normally, the `onA` listener (whose closure pins the per-request signal and controller) stays on the long-lived transport signal until `close()`, accumulating one listener per c