fix(agents): warn when addMcpServer falls back to the default callback URL#1719
fix(agents): warn when addMcpServer falls back to the default callback URL#1719mattzcarey wants to merge 2 commits into
Conversation
…k URL The sendIdentityOnConnect:false guard throws for an explicit callbackHost without callbackPath, but every other path (sendIdentityOnConnect:true, or a derived callbackHost) silently built the default callback URL. That URL embeds the instance name and only works when /agents/* is routed through routeAgentRequest, so a misrouted Worker hangs OAuth in AUTHENTICATING with no signal. Warn whenever the default URL is used. Fixes #1378
🦋 Changeset detectedLatest commit: 53fd1ad The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // The sendIdentityOnConnect:false guard above throws for this case; | ||
| // every other path that lands here gets a warning instead — the | ||
| // default URL leaks the instance name and only works when the Worker | ||
| // routes the agents prefix through routeAgentRequest (#1378). |
There was a problem hiding this comment.
🚩 Comment incorrectly claims sendIdentityOnConnect:false guard covers all paths to this branch
The comment at packages/agents/src/index.ts:10785 states "The sendIdentityOnConnect:false guard above throws for this case" — but this is only true when resolvedCallbackHost was explicitly provided by the caller. When the host is auto-derived from the request/connection at packages/agents/src/index.ts:10767-10776, the guard at packages/agents/src/index.ts:10754-10764 has already been passed (because resolvedCallbackHost was still undefined at that point). This means: with sendIdentityOnConnect:false, no explicit callbackHost, and a host auto-derived from the current request, the code reaches this else branch and only warns — the instance name still appears in the default callback URL despite the agent opting out of identity exposure. The underlying enforcement gap is pre-existing (the old ternary had the same control flow), but the new comment gives a false sense that it's fully handled. Consider either extending the guard to also check after auto-derivation, or correcting the comment to note this gap.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — the comment over-claimed. Corrected in 53fd1ad: the throw only covers sendIdentityOnConnect: false with an explicit callbackHost; an auto-derived host bypasses it regardless of the option and now lands in the warn branch (which is the behavioral improvement this PR adds — that path was previously silent). Left the enforcement gap as a warn rather than extending the throw to derived hosts, since that would break currently-working setups that do route /agents/* correctly; a maintainer could escalate it in the next major (#844).
agents
@cloudflare/ai-chat
@cloudflare/codemode
create-think
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
The throw only covers sendIdentityOnConnect:false with an explicit callbackHost; an auto-derived host bypasses it regardless of the option and lands in the warn branch. Say so instead of over-claiming.
1 participant
Note
This is a Fable experiment (model-authored PR). Please close if it adds no value.
Fixes #1378
Problem
addMcpServerhas a guard that throws whensendIdentityOnConnect: false+ explicitcallbackHost+ nocallbackPath. But the guard is bypassed on two paths:sendIdentityOnConnect: true(the case in the issue — an option about WS identity broadcasting silently disabling callback-path safety), andcallbackHostauto-derived from the current request/connection (derivation happens after the guard).On both paths the default callback URL (
…/{agentsPrefix}/{kebab(class)}/{this.name}/callback) is silently generated and persisted. If the Worker doesn't route/agents/*throughrouteAgentRequest(e.g. narrowedrun_worker_first+ SPA assets, as inexamples/assistant), the OAuth redirect never reaches the DO and the MCP server hangs inAUTHENTICATINGwith zero server-side signal.Fix
Per the issue's least-disruptive option (and Matt's call):
console.warnwhenever the default callback URL is constructed because nocallbackPathwas provided. The warning names the server, prints the exact fallback URL, explains the routing requirement and the failure mode, and says what to do (callbackPath+getAgentByName).The existing
sendIdentityOnConnect: falsethrow is unchanged — this only adds a signal to the paths that previously proceeded silently. No behavioral change to URL construction.Tests
Two new tests in
packages/agents/src/tests/mcp/add-mcp-server.test.ts, via warn-capturing probe methods on the existingTestHttpMcpDedupAgent(defaultsendIdentityOnConnect: true, mockedconnectToServer):callbackPath→ warning fires, includes server name, the full default URL (instance name visible), and thecallbackPathremediationcallbackPath→ no warningFull MCP suite: 492 passed (26 files). Typecheck/oxfmt/oxlint clean.
Notes for maintainers
/agents/*correctly (the default URL is fine there). Escalating to a throw in the next major could be paired with breaking changes for next major #844.addMcpServercall (server registration time), not per request, so it can't spam logs.agentspatch).