fix(server-tools): replay spent upstream bodies as real error statuses - #149
orangeboyChen wants to merge 4 commits into
Conversation
A turn that ran a server tool lost everything the model wrote between searches, and delivered its final answer only after it was complete. The execution loop fed each iteration's message back into the transcript but never emitted it, so a multi-hop turn surfaced only the tool calls and the last answer. Anthropic returns the text between searches as content blocks of the same turn, and there is no other point at which that text can reach the client. The final iteration was also requested buffered, which held the whole answer back before replaying it in fixed-size slices. It now streams, the same way the first iteration already did: text is forwarded as it arrives and only tool-call frames are held, since a server tool still has to be answered locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tion Three defects in the streamed search loop: Streamed iterations had their text emitted twice — once by the probe as it forwarded deltas, and again from the accumulated content when the turn continued. The synthesized emission now applies only to buffered iterations, whose payload never reached the client. The probe's reader was never registered with the stream's cancellation path, so a client disconnect could not interrupt a read already parked on a stalled upstream; the loop only noticed once upstream produced another chunk. The reader is now handed to the caller, which cancels it. A non-streaming turn mixing a local web tool with a client-owned tool folded the current iteration's text in twice: the mixed payload already carries it, and the accumulated turns included it too. Only completed iterations are accumulated now. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An upstream error response was read twice on the server-tool path. The loop consumed the body to check whether the model asked for a search, then handed the same Response back; the route layer read it again to build the client's answer and threw "Body already used", so a rate limit surfaced as a 500 instead of its real status. Clone before reading in both loop iterations and rebuild the response from the drained body on failure, preserving non-JSON error details. The Anthropic streaming bridge also emitted a generic api_error at HTTP 200, hiding the upstream status from clients that key their retry decisions on rate_limit_error. It now maps the upstream status through the same status-to-type table the non-streaming path uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Superseded by #150, which is based directly on main instead of carrying the 3 commits from fix/server-tool-loop-streaming. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f15c2c494
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (nextRemainingCalls.length) { | ||
| finalPayload = buildMixedTurnPayload({ | ||
| message, | ||
| payload, | ||
| payload: { |
There was a problem hiding this comment.
When a follow-up iteration contains both a locally executable tool and a client-owned tool, its text and reasoning have already been sent by probeServerToolStream for SSE responses, or explicitly emitted at lines 1237–1257 for buffered responses. Building finalPayload from the same message and then passing it to synthesizeChatCompletionStream emits that content a second time, so clients receive duplicated prose/reasoning. Defer the earlier emission or synthesize only the findings and outstanding tool calls in this branch.
Useful? React with 👍 / 👎.
| if (cancelled) return; | ||
| activeReader = null; | ||
| usage = sumUsage(usage, context.usage); |
There was a problem hiding this comment.
If one streamed iteration reports usage and a later iteration omits it while requesting another local tool, context.usage still contains the prior iteration's value because probeServerToolStream only overwrites it when a chunk supplies usage. Summing that retained value here counts the earlier request again, and the inflated total is later emitted when the loop produces a synthesized mixed or fallback response. Reset the context usage before each probe or sum the current probe's independently initialized usage.
Useful? React with 👍 / 👎.
| message: withIntermediateTurns({ | ||
| payload, | ||
| reasonings: intermediateReasonings, | ||
| texts: intermediateTexts, | ||
| }).choices?.[0]?.message, | ||
| payload, |
There was a problem hiding this comment.
For a multi-hop non-streaming turn that ends with both a server-tool call and a client-owned call, withIntermediateTurns adds earlier reasoning to the temporary message, but buildMixedTurnPayload only reads that argument's content; its output message is copied from the separate, original payload. Consequently earlier intermediateReasonings are discarded even though earlier text is retained. Pass the folded payload itself or copy the folded reasoning into the returned message.
Useful? React with 👍 / 👎.
| if (!response.ok || payload.error) { | ||
| return { body: loopBody, executions, response }; | ||
| return { | ||
| body: loopBody, | ||
| executions, | ||
| response: await buildServerToolFailureResponse(response), |
There was a problem hiding this comment.
When upstream returns HTTP 200 with a JSON error payload, this branch recognizes a failure but rebuilds it with the unchanged 200 status. In proxyChatCompletions, a streaming caller therefore treats the response as successful and feeds it to synthesizeChatCompletionStream; because an error payload has no choices, the client receives an empty successful stream instead of the upstream error. Convert payload-level failures to a non-2xx status or ensure they bypass success-stream synthesis.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
==========================================
- Coverage 95.38% 95.33% -0.05%
==========================================
Files 36 36
Lines 6239 6349 +110
Branches 1778 1812 +34
==========================================
+ Hits 5951 6053 +102
- Misses 288 296 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
Problem
An upstream rate limit surfaced to clients as
status_code=500, Body already usedinstead of the real 429:Cause
The
route: "/v1/chat/completions"in that log isfetchChatCompletion's internal label, so the 429 came from inside the server-tool loop, not a plain chat request.executeWebSearchLoopread the upstream body to check whether the model asked for a search, then returned that sameResponseobject. AResponsebody can only be consumed once, so the route layer's second read — the one that builds the answer the client sees — threwBody is unusable: Body has already been read. The 500 replaced the real status.Verified against the unpatched tree: HTTP 500
{"type":"error","error":{"type":"api_error","message":"Body is unusable: Body has already been read"}}.Fix
web-search-loop.ts— clone before reading in both loop iterations, and rebuild the response from the drained body on failure, so both reads work. The body is replayed verbatim rather than re-serialized, so an error detail that isn't valid JSON still reaches the client intact.content-length/content-encodingare dropped since the bytes are re-emitted, not re-encoded.anthropic.ts— the streaming bridge emitted a genericapi_errorat HTTP 200, hiding the upstream status from clients that key retry decisions onrate_limit_error. It now maps the upstream status through the same status→type table the non-streaming path already used (extracted asanthropicErrorType).Result
api_errorrate_limit_error+ upstream detailapi_errorrate_limit_error+ upstream detailBody already usedThe streaming HTTP status stays 200 because the Anthropic SSE envelope is already committed by the time the failure arrives — the error type now carries the information instead.
Verification
3 new regression tests in
tests/server/upstream-error-response.test.ts; all 3 fail on the unpatched tree and pass with the fix.bun run lint/format:check/typecheck/build— passbun run test:ci— 667 tests pass, 94.46% statementsbun run test:patch-branches— 92.80% (≥ 90% required)🤖 Generated with Claude Code