fix(anthropic): carry server-tool hop metadata beside the response - #157
Conversation
![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: 7fc9e8882a
ℹ️ 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".
| executions: results.map((result) => result.execution), | ||
| reasoning: '', | ||
| text: '', |
There was a problem hiding this comment.
When the first upstream response contains both a locally executed server tool and a client-owned tool, priorTurns is empty because withIntermediateTurns returns no turns when there are no earlier hops. This newly appended turn then carries only the execution, even if the current message contains text or reasoning. Because the nonempty metadata makes mapOpenAIResponseToAnthropic render exclusively from turns, that current prose is silently omitted from the Anthropic response. Populate this turn with the current iteration's text and reasoning rather than empty strings.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
+ Coverage 95.47% 95.48% +0.01%
==========================================
Files 37 37
Lines 6633 6650 +17
Branches 1910 1912 +2
==========================================
+ Hits 6333 6350 +17
Misses 300 300
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
#152 laid non-streaming server-tool blocks out per hop, but put the grouping it needed on the chat-completion payload itself. That payload is what an OpenAI-protocol client receives, so three things went wrong. ## 1. Hop metadata leaked into chat-completions responses `withIntermediateTurns` attached `turns` to the payload, and `Response.json` serialized it. `app/v1/chat/completions/route.ts` returns `proxyChatCompletions` unchanged, so `/v1/chat/completions` exposed a non-protocol `turns` field holding internal tool inputs and results — strict validators can reject it, and every client received the tool data twice. Carry the grouping out of band through a WeakMap, the way `serverToolExecutions` already is: `attachServerToolTurns` / `getServerToolTurns`. The Anthropic adapter reads it off the response instead of the payload. ## 2. Mixed server/client-tool turns lost the grouping When a later hop mixed a local server tool with a client-owned tool, `withIntermediateTurns` built the turns but the mixed branch took only `.message` off its result and never grouped the current hop. With no hop metadata the mapper flattened all reasoning and text ahead of all server-tool blocks — the exact ordering bug #152 set out to fix. Preserve the prior turns and add the current hop's calls to its entry. ## 3. A first hop that mixed tools dropped its own prose The mixed branch built the current hop with empty text and reasoning. When that hop was also the first, `withIntermediateTurns` had no earlier hops to fold and so returned no turns, and a block renderer renders purely from `turns` once it is non-empty — so everything the model said in that hop disappeared from the Anthropic response. `withIntermediateTurns` already builds this hop as its closing entry, so the calls are added to that entry rather than appended as a new one, which would repeat the prose. Only when there are no earlier hops is a fresh entry built, seeded with this iteration's text and reasoning. ## Verification - Three regression tests, one per issue. Each was confirmed to fail with its fix reverted and pass with it applied, so they guard the behaviour rather than the implementation. - `withIntermediateTurns` and `buildMixedTurnPayload` now return `{ payload, turns }`, and `ServerToolLoopResult.turns` is required rather than optional, so every return path states its grouping explicitly instead of relying on a `?? []` fallback. - 733 tests pass; typecheck, eslint, prettier, and build are clean. Coverage 94.67% statements. Rebased onto #156, which exports `withIntermediateTurns` to the image-generation loop; those call sites now take `.payload`.
7fc9e88 to
f1a4105
Compare
|
Addressed the third review comment (prose dropped in a first mixed-tool hop), and rebased onto The bug confirmed and fixed. With the empty strings restored, a first hop carrying both a local tool and a client-owned tool produced One correction to the suggested remedy, in case it matters elsewhere: populate a newly appended turn with the current iteration's text and reasoning and that prose duplicates on later hops. The fix therefore adds the calls to that closing entry rather than appending a new one, and only builds a fresh entry when there are no earlier hops, seeded with this iteration's text and reasoning. Tests. Three, one per issue, each verified to fail with its fix reverted and pass with it applied:
Also in this update: rebased onto #156, which exports 733 tests pass; typecheck, eslint, prettier, and build are clean. Coverage 94.67% statements, patch branch coverage 100% (8/8). |
1 participant
Follows up on #152, addressing both Codex review comments from that PR.
#152 laid non-streaming server-tool blocks out per hop, but put the per-hop grouping it needed on the chat-completion payload itself. That payload is what an OpenAI-protocol client receives, so two things went wrong.
1. Hop metadata leaked into chat-completions responses
withIntermediateTurnsattachedturnsto the payload, andResponse.jsonserialized it.app/v1/chat/completions/route.tsreturnsproxyChatCompletionsunchanged, so/v1/chat/completionsexposed a non-protocolturnsfield containing internal tool inputs and results — strict validators can reject it, and every client received the tool data twice.Fixed by carrying the grouping out of band through a
WeakMap, the wayserverToolExecutionsalready is:attachServerToolTurns/getServerToolTurns. The Anthropic adapter reads it off the response instead of the payload.2. Mixed server/client-tool turns lost the grouping
When a later hop mixed a local server tool with a client-owned tool,
withIntermediateTurnsbuilt the turns but the mixed branch took only.messageoff its result, and never grouped the current hop's calls. The response had no hop metadata, sobuildAnthropicTurnBlockswas never used and the mapper flattened all reasoning and text ahead of all server-tool blocks — the exact ordering bug #152 set out to fix. Reproduced by a search-only hop followed by a hop mixing search withclient_tool.Fixed by preserving the prior turns and appending the current hop's execution group when constructing the mixed response.
Verification
withIntermediateTurnsandbuildMixedTurnPayloadnow return{ payload, turns }, andServerToolLoopResult.turnsis required rather than optional, so every return path states its grouping explicitly instead of relying on a?? []fallback.Non-streaming remains the affected path; the streaming path already emits the correct layout and is untouched.