fix(server-tools): leave a client-declared web_fetch to the client - #153
Merged
Merged
Conversation
The Responses API has no `web_fetch` server tool — only `web_search`. A
client that declares one as a plain function owns it and resolves it
itself. But the fetch branch of `replaceServerTools` took the call over
whenever a backend was configured, before it ever asked whether the
declaration was client-owned:
if (isWebFetchTool(tool)) {
if (fetchEnabled && fetchProvider) { ...take over... }
if (!isServerDeclaredFetchTool(tool)) { return [tool]; }
So choosing `codebuddy` or `codebuddy2api` silently disabled a tool the
client had declared and depended on — the client never saw the call come
back. Only the `passthrough` default kept it working, which is why this
went unnoticed.
Reorder so the client-owned check comes first, matching what the search
branch already does. The backend setting chooses who runs the *proxy's*
tool; it is not a licence to take the client's.
Verified end to end on the Responses route: a Codex-declared `web_fetch`
is now handed back unresolved, with the proxy neither fetching the page
nor re-asking upstream.
Two existing tests encoded the old behaviour and are updated:
- `takes over a client-declared web_fetch function when a backend is set`
asserted the takeover directly. It now asserts the loop declines to
touch the request, which is the behaviour it was named for.
- Two `runOnce` cases built the proxy's own definition by hand instead of
going through the Responses translator, so it arrived unmarked and
looked client-owned. They now use `translateResponsesToolsToChat`, which
is what actually marks an Anthropic-typed declaration as server-side.
![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: 7de71a3e22
ℹ️ 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #153 +/- ##
=======================================
Coverage 95.41% 95.42%
=======================================
Files 36 36
Lines 6349 6355 +6
Branches 1813 1813
=======================================
+ Hits 6058 6064 +6
Misses 291 291
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
The declaration-level guard kept a client's `web_fetch` out of the
proxy's hands, but the synchronous loop still classified calls by name
and backend alone:
(Boolean(fetchProvider) && isWebFetchToolCall(toolCall))
So a request carrying both a server-declared `web_search` and a
client-owned `web_fetch` — search makes the loop run, which activates
the fetch backend — had the client's fetch executed instead of handed
back. Ownership is decided when the declarations are rewritten, and the
streaming paths already consulted it via `ownedNames`; only this path
did not.
Route it through `isLocalServerToolCall` like the streaming paths, and
thread `ownedNames` into the loop scope.
Verified by inspection at the decision point — `ownedNames` is
`["websearch"]`, so `web_search` is executed and `web_fetch` is
returned — and by a test that fails (two upstream rounds instead of
one) when the classification is reverted.
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.
Problem
The Responses API has no
web_fetchserver tool — onlyweb_search. A client that declares one as a plain function owns it and resolves it itself.But the fetch branch of
replaceServerToolstook the call over whenever a backend was configured, before asking whether the declaration was client-owned:So choosing
codebuddyorcodebuddy2apisilently disabled a tool the client had declared and depended on — the client never saw the call come back. Only thepassthroughdefault kept it working, which is why this went unnoticed.Observed on the Responses route with
CODEBUDDY_WEB_FETCH_BACKEND=codebuddyand a Codex-declared{type:"function", name:"web_fetch"}:web_fetch_20250910Fix
Reorder so the client-owned check comes first, matching what the search branch already does. The backend setting chooses who runs the proxy's tool; it is not a licence to take the client's.
Verified end to end on the Responses route: the call is now handed back unresolved, with the proxy neither fetching the page nor re-asking upstream.
Tests
web_fetchis handed back even with an executable backend — asserts no fetch, no second upstream call, noopen_page. Confirmed red before the fix (pageFetches0 → 5).tool_callsreach the client.takes over a client-declared web_fetch function when a backend is setasserted the takeover directly; it now asserts the loop declines to touch the request.runOncecases built the proxy's own definition by hand instead of going through the Responses translator, so it arrived unmarked and looked client-owned. They now usetranslateResponsesToolsToChat, which is what actually marks an Anthropic-typed declaration as server-side.All 677 tests pass; typecheck, eslint, and prettier are clean. Changed-branch coverage is 100%.
Note
This also applies to
/v1/chat/completions, which is fine — a client-owned tool is client-owned on either route.