Repository navigation
Conversation
When an MCP server keeps rejecting every refreshed token, each tools/call fails with a 401 but the server status stays connected, so the UI gives no signal and the client keeps refreshing on every call (issue anomalyco#53942). callTool now detects auth failures (an SDK UnauthorizedError, or a plain Error whose message carries HTTP 401 / Unauthorized) and stops the server, marks it needs_auth, and publishes StatusChanged, so the rejected tools stop being surfaced and the refresh loop breaks. Re-auth from /mcps restores it; credential reconnects already restart needs_auth servers. Fixes anomalyco#53942
Replace the regex match on error message text with a precise check: only a real HTTP 401 (SdkHttpError.status === 401) or the SDK's UnauthorizedError surfaces as MCP.NeedsAuthError and takes a server into needs_auth. Messages from other APIs that merely echo "401"/"unauthorized" no longer tear a server down. Teardown now runs under the server lock and only stops a still live connection, and the tool call error is no longer double-wrapped.
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This works. I ran OpenCode from source against a local MCP server that answers tools/call with 401. On the base, the call fails and /mcps still says Connected. With this PR, the call reports that authentication is required and /mcps shows Sign in required. mcp.test.ts and the packages/core typecheck pass, apart from one test that fails on the base too. The SDK throws the issue's "401 after re-authentication" error as an SdkHttpError with status 401, which the new check catches.
Public API: unchanged.
What I'd fix before merging:
- The handler doesn't check that the 401 came from the connection that's live now, and it sets the status outside the lock (see inline).
- The two "does not mark needs_auth" tests add little. Detection now looks at the HTTP status, so the "body echoes 401" test can't catch a regression. I'd keep just the 401 test so the change stays small.
| Effect.gen(function* () { | ||
| // Teardown serializes with other lifecycle ops and only stops a server that is still live, | ||
| // mirroring recover(), in case the connection was replaced or stopped while the call ran. | ||
| yield* Effect.gen(function* () { |
There was a problem hiding this comment.
This doesn't check that the 401 came from the connection that's live now:
target.entry.client == nullonly checks that some client exists. If the server reconnected while this call was running (for example, the user just re-authenticated andreconnectstarted a fresh connection), a late 401 from the old connection stops the new one.- The
needs_authassignment and theStatusChangedpublish below run outside the lock, and they also run when the guard returned early. That can overwritedisabled, or a newerconnectedstatus.
I'd capture the connection the call ran on and do everything inside the lock only when entry.client === connection: stop the server, set the status and publish. That's the same check recover() and whenLive() use. Note that recovering() may swap the connection after a session expiry, so the live connection isn't necessarily the one you started with.
…tion Capture the connection the call ran on and, on a NeedsAuthError, stop the server, set the status and publish only while holding the lock and when entry.client is still that exact connection. This mirrors recover()/whenLive() so a late 401 from a connection that was replaced (e.g. during re-auth) can't tear down the newer connection or overwrite a newer disabled/connected status.
|
Addressed the review feedback: the MCP auth handling now captures the connection the call actually ran on and, on a NeedsAuthError, only stops the server / sets needs_auth status / publishes while holding the lock and when |
|
Would love to see this merged. I think this blocks Google Workspace MCPs which i think is a fairly high priority set of MCPs. |
Adapted from anomalyco#55013 to the 2.2.7 connection module. A real HTTP 401 or SDK UnauthorizedError during a tool call surfaces as NeedsAuthError; the server is stopped and marked needs_auth (only if still live on the connection the call ran on), so the UI offers a login instead of every call failing (anomalyco#53942). Errors that merely mention 401 do not count.
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Closes #53942
Type of change
What does this PR do?
When an MCP server keeps rejecting every refreshed token after a failed OAuth refresh, each
tools/callfails with HTTP 401 but the server status stays "Connected" — no signal to re-auth, and it keeps refreshing on every call.This detects auth failures precisely instead of matching error message text:
packages/core/src/mcp/client.ts— a real HTTP 401 (SdkHttpError.status === 401) or the SDK'sUnauthorizedErrorsurfaces as the existingMCP.NeedsAuthError. Errors from other APIs that merely echo "401" no longer count as auth failures.packages/core/src/mcp/index.ts—callToolcatchesMCP.NeedsAuthErrorand, while holding the server lock and only when the connection is still the live one, stops the server, marks itneeds_auth, and publishesStatusChangedso/mcpsand the UI show it. The error is no longer double-wrapped.Re-authing from
/mcps(or a credential change via the existing reconnect path) restarts the server and restores the tools.How did you verify your code works?
packages/coretypecheck clean.mcp.test.ts.mcp.test.tsfailures reproduce identically on unmodifiedv2(container-environment timing).Screenshots / recordings
N/A — backend change.
Checklist
Added by the opencode bot (AI-generated).
Related: #53804, #49773