Repository navigation
fix(client): fall back to initialize when the discover probe gets a completed non-modern answer - #2571
fix(client): fall back to initialize when the discover probe gets a completed non-modern answer#2571claude[bot] wants to merge 6 commits into
Code review found 1 important issue
Found 5 candidates, confirmed 6. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 5 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | packages/client/src/client/versionNegotiation.ts:410-430 |
Pin-mode/modern-only rejections lose all provenance for the new legacy inflows: a 503 or broken 2xx body now throws 'did |
| 🟡 Nit | docs/protocol-versions.md:104 |
Categorical completed-exchange rule isn't backed for direct JSON answers that produce no matching reply (empty batch [], |
| 🟡 Nit | packages/client/src/client/streamableHttp.ts:1129-1143 |
202 gate matches post-connect Client.discover(), not just the probe — breaks the 'negotiation-phase only' EraNegotiation |
Annotations
Check warning on line 430 in packages/client/src/client/versionNegotiation.ts
claude / Claude Code Review
Pin-mode/modern-only rejections lose all provenance for the new legacy inflows: a 503 or broken 2xx body now throws 'did not offer pinned protocol version' with no status/body/cause
In pin mode and for modern-only clients, the outcome families this PR newly routes into the legacy verdict — non-auth 5xx and every invalid-reply shape — now reject with the fixed `did not offer pinned protocol version ... via server/discover` / `gave no modern evidence` message carrying no status, body, or cause: a transient 503 mid-deploy is indistinguishable from a server that genuinely lacks the pinned revision, and `docs/troubleshooting.md` maps that message tail to "drop the pin or use aut
Check warning on line 104 in docs/protocol-versions.md
claude / Claude Code Review
Categorical completed-exchange rule isn't backed for direct JSON answers that produce no matching reply (empty batch [], schema-valid reply with wrong id) — these still burn the full probe timeout
The categorical completed-exchange rule this PR adds here ("classified by what it answers, however broken... Any completed non-auth HTTP exchange whose direct (non-SSE) answer is not a valid modern reply is legacy evidence") isn't backed by the implementation for two direct `application/json` shapes: a 200 body of `[]` (zero messages produced, `_send` resolves, no stamp fires) and a schema-valid reply with a non-matching id (e.g. `id: 1` substituted for the probe's string id — the sibling of the
Check warning on line 1143 in packages/client/src/client/streamableHttp.ts
claude / Claude Code Review
202 gate matches post-connect Client.discover(), not just the probe — breaks the 'negotiation-phase only' EraNegotiationFailed contract
The new 202 gate checks only `message.method === 'server/discover'`, so it matches not just the connect-time probe but also the public post-connect `Client.discover()` API — a 202 to a mid-session `discover()` on an already-established modern connection now rejects with `SdkError(EraNegotiationFailed)`, contradicting both the inline comment ("Scoped to the probe request") and the sdkErrors.ts docstring this PR edits ("Negotiation-phase only: this code is never used once an era is established").