fix(mcp): route call-time ErrNoToken through the auth gate (closes #376) - #377
Conversation
The delegated-consent auth gate (#330) was consulted only for resolveClient (connection-establish) errors. But per-request-auth transports (transport_http attaches the bearer on every Send, including tools/call) 403 at CALL time, not at initialize — so a type=user server whose user has no grant surfaced ErrNoToken from client.CallTool, which the gate never inspected. The call failed hard with reason=no_token, no mcp_auth_required event fired, and the platform consent flow had nothing to trigger on (field: Atlassian agent, forge#376). Execute now wraps the whole resolve→call sequence in the gate: an ErrNoToken from either half parks via authGate.Await and, on a granted resume, retries resolve→call. Same bounded, one-shot semantics as before; the diagnostic phase prefix is irrelevant now since both paths are covered. Tests: call-time no-token parks + retries to success; gate give-up still fails as no_token; a non-auth CallTool error never parks; nil gate still surfaces ErrNoToken.
initializ-mk
left a comment
There was a problem hiding this comment.
Reviewed the full resolve→call rework against the branch source — correct, minimal, well-tested. Approve-worthy. This is a genuine bug fix and the root-cause analysis in the description matches the code.
The bug is real, the diagnosis is right
The auth gate (#330) previously wrapped only resolveClient — the establish half. But transport_http attaches the bearer per request (authFn runs inside Send on every frame, including tools/call), so a streamable-HTTP server that initializes fine and only 403s on the actual call surfaces mcp.ErrNoToken from client.CallTool — which the old code reached only after the gate check. Hard fail, reason=no_token, no park, no mcp_auth_required. The "no initialize:/connect: withPhase prefix" tell is a legitimate proof it bypassed establish.
The fix is structurally sound
- One-shot semantics preserved — the gate branch is a plain
if, not a loop, so a park happens at most once; a secondErrNoTokenon retry falls straight through to emit+return asno_token. - Retry re-resolves intentionally — on a granted resume
resolveAndCallre-runsresolveClientso the per-subject pool picks up the now-granted connection, then calls. For the fixed-client case (resolver == nil) the re-resolve just returnsm.client— harmless. - Retry is safe against double-execution —
ErrNoTokenis a 403, so the firstCallTooldid no work before failing; re-issuing can't double-apply a write. Strictly improves on before, where a call-time no-token never retried at all. - Establish-time path unchanged — net behavior identical to the old code.
Tests cover all four branches
CallTimeNoToken_Parks, GateGivesUp, CallTimeError_NoSpuriousPark, NoGate_NoTokenSurfaces. The gateStub.onAwait hook flipping the mock client to success cleanly models "grant now exists." Interface signature matches (Await(ctx, server string) error). CI fully green.
Non-blocking observations (no fix required)
res.Contenton a(nil, nil)return would nil-deref inflattenContent— but this is pre-existing (the old code had the identical shape) and real MCP clients never return(nil, nil). Noted only for completeness.- No test asserts the "retry still returns
ErrNoToken" grant-race case doesn't double-park — the non-looping structure makes that obvious, so it's a nicety, not a gap.
Good fix. LGTM.
| } | ||
|
|
||
| res, err := resolveAndCall() | ||
| if err != nil && m.authGate != nil && errors.Is(err, mcp.ErrNoToken) { |
There was a problem hiding this comment.
This is the crux of the fix and it's correct. Gating resolveAndCall (resolve→call) rather than just resolveClient means an ErrNoToken from either half now parks: the establish case (unchanged) and the call-time case (newly covered — the #376 regression, where per-request-auth transports 403 on the tools/call frame).
One-shot semantics are intact — this is a single if, not a loop, so a grant-race that still yields ErrNoToken on the retry (line 229) falls through to emit+return as no_token rather than parking twice. And the retry re-running resolveClient is the right call: a per-subject pool needs to re-pick the now-granted connection.
Closes #376.
Problem
A delegated (type=user) MCP agent whose user has no grant failed the tool call hard instead of parking on the auth gate — no
mcp_auth_required, task completes, LLM narrates the raw error:The auth gate (#330) was consulted only for
resolveClient(establish) errors inmcp_tool.go::Execute. Buttransport_httpattaches the bearer per request (authFnruns insideSend, every frame includingtools/call), so a streamable-HTTP MCP server that initializes fine and only 403s on the actual call surfacesErrNoTokenfromclient.CallTool— which the gate never inspected. (Diagnostic tell: the error carried noinitialize:/connect:withPhaseprefix, proving it bypassed establish.)Fix
Executenow wraps the whole resolve→call sequence: anErrNoTokenfrom either the establish or the CallTool half parks viaauthGate.Await, and on a granted resume retries resolve→call. Same bounded, one-shot gate semantics as before — the establish-time behavior is unchanged, the call-time path is newly covered.Tests
CallTimeNoToken_Parks— the MCP auth gate bypassed when ErrNoToken surfaces at tool-call time (per-request auth), not connection-establish #376 regression: CallTool 403 → gate consulted once → retry succeeds after consentCallTimeNoToken_GateGivesUp— Await error still fails asno_token, no extra callCallTimeError_NoSpuriousPark— a non-auth CallTool error never touches the gateNoGate_NoTokenSurfaces— nil gate still surfacesErrNoTokengo test ./...green (forge-core), gofmt + golangci-lint clean.Downstream
Restores the clean park + auto-resume path for the platform consent flow (initializ/agent-builder emits/consumes
mcp_auth_required; initializ/console-next#67 surfaces it). The console's proactive "connect your account" banner (#67) was a partial mitigation; this is the server-side root fix.