refactor(proxy): rewrite server tools around the declared tool type - #163
Merged
Merged
Conversation
Classification matched a server tool by name, and `normalizeToolName` strips case and separators — so `WebSearch`, the ordinary function Claude Code declares and resolves itself, compared equal to `web_search`, the provider-executed server tool. The proxy answered Claude Code's own calls, the `tool_use` block it was waiting for never arrived, and the turn ended with an answer invented from memory. Only the declared type can tell them apart. Anthropic sends a dated type (`web_search_20250305`), the Responses API sends `web_search_preview`, and a client's own function is declared as `function` or with no type at all. Classification now reads the type and never the name, and both translators preserve it through to the request that goes upstream. That removes the ambiguity the loop existed to paper over. A request either declares a server tool — in which case one search runs and upstream is asked once more, without the tools, to write the answer — or it does not, and every tool call goes back to the client untouched. So `web-search-loop.ts` and the `server-tool/` module go away, replaced by `server-tools/`, and each route drives the turn itself instead of it hanging off the shared chat chokepoint. The private `x-codebuddy2api-server-tool` marker goes with them: it was the mechanism that carried the conflated identity, and nothing reads it any more. Also excludes `.worktrees/**` from vitest, which was collecting a whole stale copy of the suite out of every git worktree.
Raises changed-branch coverage to 91.75%, past the 90% floor. `rewriteServerTools` can only be reached once declarations have been found, which already guarantees a non-empty array, so its nullable return was unreachable — it now takes `unknown[]` and the caller drops the guard. The unused `readJsonResponse` helper goes too.
![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: aa808206e4
ℹ️ 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".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #163 +/- ##
==========================================
- Coverage 95.73% 94.81% -0.93%
==========================================
Files 70 67 -3
Lines 6708 6436 -272
Branches 1927 1864 -63
==========================================
- Hits 6422 6102 -320
- Misses 286 334 +48
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Server tools are now answered by a genuine loop, bounded by the `max_uses`
the client declared, instead of a fixed two-call turn:
while model requests web_search:
execute search
append tool result
call chat again
The loop is over *server* tools only. Claude Code's own `WebSearch` is an
ordinary client function and a call to it is never picked up — that is the
distinction the whole feature turns on, and the reason classification keys
off the declared type rather than the name.
- Translation carries the whole declaration through, so `max_uses` reaches
the loop (it was dropped before, leaving the budget pinned at 5).
- The budget is per tool kind: a fetch's allowance no longer caps searches.
- Calls are clamped to the budget before running, and the transcript only
carries calls that were actually executed, so no assistant message ever
promises a result it does not have.
- A server tool nothing here can run is withdrawn rather than offered;
offering it handed the client a `tool_use` it had no handler for.
- Every exit reports the searches already run and the usage accrued,
including when a later hop fails.
- On Responses the preamble lands ahead of the searches, and the stream
carries one id, with the answer — not the preamble — as text deltas.
The `web_fetch` end-to-end probe raced a real local fetch behind the mocked endpoint call — `local-fetch` uses `node:http`, so the stub the setup file installs on `globalThis.fetch` did not apply and the test resolved a real hostname. That is why it passed locally and failed on CI, and it bought nothing: the branches it targeted are not in this change's diff, so patch coverage is unmoved. `test:ci` now also runs the default reporter alongside the JUnit one. The JUnit reporter writes failures to a file rather than stdout, which is how a red CI run ended up with no explanation in the log.
Owner
Author
|
@codex review |
Owner
Author
|
@codex Please validate whether the flow complies with Anthropic's server-side tool specification. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ddf9604ebb
ℹ️ 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".
Addresses the Codex review findings on the PR.
Prose written *between* two searches was dropped. A single preamble can
only hold the first hop's, so a turn like
text → search → text → search → answer lost the second passage entirely.
The turn now reports one segment per hop — the prose that preceded it
plus the calls it made — and every renderer interleaves them the way
Anthropic does, instead of gathering prose and blocks by kind.
Also from the review:
- Streamed preambles went out whole inside `content_block_start`. Streaming
consumers read generated text from `text_delta` / `thinking_delta`, so a
block that starts already complete reaches no delta callback at all.
They are now emitted as an empty start, a delta, then a stop.
- A streamed `server_tool_use` carried the full input *and* streamed it
again as an `input_json_delta`. Anthropic builds streamed input from
deltas alone, so the block now opens with `input: {}`.
- Withdrawing a server tool nothing here can run left a forced
`tool_choice` naming it, which an upstream validating the two together
can reject instead of letting the model answer. A forced choice is now
dropped whenever the tool it names is not on offer.
The contract these routes are audited against: the three-phase flow, and the distinction between `WebSearch` (Claude Code's own client tool, which the proxy must never pick up) and `web_search_*` (a provider-executed tool the proxy must run, in a loop bounded by `max_uses`, hiding the internal chat function). Kept out of the VitePress navigation: it is an internal design document rather than user documentation, and the guide tree is localised per language. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nning Conformance fixes against the design spec in docs/design/claude-code-websearch-flow.md. - Terminate the loop when a hop can afford nothing. `spent` tested "every kind exhausted" while affordability is per kind, so with both search and fetch runnable, a model that kept asking for the exhausted one ran a hop that executed nothing: the transcript never advanced and the loop re-issued an identical billed request forever. - Stop handing the internal search function to the client on the failure paths. Both error returns passed the payload through verbatim, so a `web_search` tool_call could reach the client as a `tool_use` for a tool it never declared and has no handler for. Every return now keeps only the calls the client can answer. - Read the closing call's body through the same guard every other hop uses. An unparseable body threw out of the turn, discarding every search already run and billed. - Reconcile `tool_choice` against the tools actually offered, on the first hop and on the closing call: a pin can no longer name a withdrawn server tool, and a pin the client set on one of its own tools survives the withdrawal. - Judge name collisions per server-tool kind, so a clash on the fetch name no longer withholds a search the client also declared. Responses path: - Accumulate image-loop iteration segments instead of keeping the first, which dropped every search after the first along with the prose that preceded it. - Skip call items the proxy mints when replaying a transcript; each was becoming an empty user turn, one per search the previous turn ran. Tests cover each fix, plus the three gaps the spec leans on hardest: both tools declared in one request with the model calling both, the query coming from the model's arguments rather than the prompt, and the Anthropic translator preserving a declared server tool's type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two defects on the Responses path, both confirmed against the real handler
rather than inferred.
A hosted-tool `tool_choice` was rejected outright: `{type: 'web_search_preview'}`
matches none of the three shapes the validator accepts, so pinning search
returned 400. Past validation it would have failed anyway — the choice was
forwarded unchanged as a type the chat upstream has never seen, while the
declaration itself had been rewritten into an ordinary function. The pin is
what makes the model emit a query instead of answering from memory, so it is
now rewritten to name the function the proxy actually injects. With no backend
configured the pin is dropped rather than naming a withdrawn tool, and the
request answers from memory instead of failing.
The image-generation loop replayed each round without the turn's searches. The
turn builds its continuation against a transcript it keeps to itself, so the
replayed request asked the model to continue from input in which the search it
had just run did not exist:
round 1: [user, assistant[web_search], tool]
round 2: [user, assistant[image_generation], tool] <- search gone
The turn now hands back the messages it appended, on the same out-of-band
channel it already used for executions, and the loop splices them in ahead of
its own message.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A client that hung up mid-turn was charged for the rest of the budget it declared. The loop had no way to know, so every remaining search ran, each followed by an upstream round trip, and the closing answer was written for nobody. The turn now takes an `AbortSignal` and stops at the next checkpoint — between hops, before executing, and before the closing call — handing back 499 (client closed request) with the searches already run still attached, as on every other non-success path. It does not abort mid-hop: an upstream call already in flight cannot be recalled more cheaply than letting it finish. `rebuildResponse` now labels what it returns as JSON rather than inheriting the upstream's content-type. Every caller hands it a `JSON.stringify(...)` result, so an upstream that answered a `stream: false` hop with SSE left a body claiming to be an event stream. Responses path: - Hosted-tool `tool_choice` now covers the tools this adapter actually serves. `image_generation` is executed here, so pinning it 400'd; it names the injected function instead. A pin on a declared-but-unimplemented hosted type is dropped rather than rejected, so the request is served without the tool instead of failing. Pinning a tool the request never declared still 400s. - `output_text` carries `url_citation` annotations for the search results the model actually quoted. The indices are measured, never guessed: a search knows the titles and URLs but nothing about where they land in an answer that did not exist yet, so a result whose URL never appears in the text gets no annotation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
The bug
Claude Code declares
WebSearchas an ordinary function tool and resolves it itself. The proxy classified server tools by name, andnormalizeToolNamestrips case and separators — soWebSearchandweb_searchcompared equal:When the model returned a
WebSearchtool call, the proxy executed the search itself and answered it. Claude Code never received thetool_useblock it was waiting for, so the search it asked for never happened and the turn ended with an answer invented from memory.The fix
Only the declared type distinguishes them. Anthropic sends
web_search_20250305, Responses sendsweb_search_preview, and a client's own function is declared asfunctionor with no type. Classification now reads the type and never the name, and both translators preserve it through to the request that goes upstream.The flow, as the spec describes it
Phase 1 — the main request. Claude Code declares
WebSearch. The proxy forwards it as an ordinary function and returns an Anthropictool_use. It does not search — Claude Code resolves it itself.Phase 2 — the side request. Claude Code sends a second, independent request carrying the server type. Now the proxy searches, and it does so with a real loop:
bounded by the
max_usesthe client declared. The loop is over server tools only — a call to the client's ownWebSearchis never picked up.Phase 3 — the continuation. Claude Code wraps the answer as a
WebSearchtool_resultand continues; the proxy does not re-enter the server-tool path.What went away
web-search-loop.ts(924 lines), theserver-tool/module, and thecodebuddy/server-tools.tsstream probe — replaced byserver-tools/(~700 lines). The privatex-codebuddy2api-server-toolmarker went with them: it carried the conflated identity, and nothing reads it now.Net −9,295 lines.
Verification
bun run lint,format:check,typecheck,build— cleantest:patch-branches— 90.74%Reviewed by four agents per round, across four rounds
Each round dispatched 2–4 agents to audit the code against the 26-chapter spec and report deviations; findings were fixed and re-audited until clean. Worth knowing what that found, because several defects were ones I introduced while fixing earlier ones:
tool_callsin the transcript, so upstream rejected the next hop after the searches were already billedfindIndexpicked the preamble as the answer to stream, so the real answer got no deltaspinnedResponseId, thenemitOpeningEvents), producing two response ids and then tworesponse.createdeventsTest coverage note
An early audit found most tests bypassed the real translation path — they fed hand-built declarations straight into
rewriteServerTools, somapAnthropicToolsToChatwas never exercised and a translator bug that droppedmax_useswas invisible. Tests now go throughhandleMessagesRequest/handleResponsesRequestwith the spec's actual payload shape, includingmax_usesandtool_choice.Also
vitest.config.tsnow excludes.worktrees/**— a git worktree is a full checkout carrying its own copy of the suite (and, once built, a second under.next/standalone), which was being collected and run against the current tree.