fix(mcp): authenticate() takes a token, so a remove during its listing wins

The already-authorized path listed tools and committed the client with no
check, so a server removed meanwhile came back connected. It now claims a
token first, like `finishAuth`, and closes its client if a newer call owns
the server.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0172qrhMa5TQgETASi5hxMqD

Author: ralphstodomingo

packages/opencode/src/mcp/index.ts

@@ -1410,6 +1410,12 @@ export const layer = Layer.effect(
     })
 
     const authenticate = Effect.fn("MCP.authenticate")(function* (mcpName: string) {
+      // altimate_change start — see connect: a disconnect or remove issued while the
+      // already-authorized path lists tools is the later call, and wins. The browser path
+      // hands over to finishAuth, which takes its own token.
+      const seq = ++lifecycleSeq
+      const token = claim(yield* InstanceState.get(state), mcpName, seq)
+      // altimate_change end
       const result = yield* startAuth(mcpName)
       if (!result.authorizationUrl) {
         const client = "client" in result ? result.client : undefined
@@ -1431,7 +1437,15 @@ export const layer = Layer.effect(
         }
 
         const s = yield* InstanceState.get(state)
+        // A newer call owns the server, and may own the OAuth state too: close this client,
+        // leave the rest alone. Checked again after the clear, which suspends.
+        const superseded = Effect.fnUntraced(function* () {
+          yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore)
+          return s.status[mcpName] ?? ({ status: "disabled" } satisfies Status)
+        })
+        if (!isCurrent(s, mcpName, token)) return yield* superseded()
         yield* auth.clearOAuthState(mcpName)
+        if (!isCurrent(s, mcpName, token)) return yield* superseded()
         return yield* storeClient(s, mcpName, client, listing.tools, listing.meta, mcpConfig.timeout)
         // altimate_change end
       }

packages/opencode/test/mcp/oauth-auto-connect.test.ts

@@ -1,5 +1,5 @@
 import { expect, mock, beforeEach } from "bun:test"
-import { Effect, Layer } from "effect"
+import { Effect, Fiber, Layer } from "effect"
 import { testEffect } from "../lib/effect"
 
 // Mock UnauthorizedError to match the SDK's class
@@ -23,6 +23,10 @@ let simulateAuthFlow = true
 let connectSucceedsImmediately = false
 let serverCapabilities: { tools?: object; resources?: object } = { tools: {} }
 let listToolsCalls = 0
+// altimate_change start — hold the tool listing open so a lifecycle call can land mid-authenticate
+let listToolsGate: { taken: () => void; release: Promise<void> } | undefined
+let closedClients = 0
+// altimate_change end
 
 // Mock the transport constructors to simulate OAuth auto-auth on 401
 void mock.module("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({
@@ -101,14 +105,26 @@ void mock.module("@modelcontextprotocol/sdk/client/index.js", () => ({
 
     async listTools() {
       listToolsCalls++
+      // altimate_change start — see listToolsGate
+      const gate = listToolsGate
+      listToolsGate = undefined
+      if (gate) {
+        gate.taken()
+        await gate.release
+      }
+      // altimate_change end
       return { tools: [{ name: "test_tool", inputSchema: { type: "object", properties: {} } }] }
     }
 
     async listResources() {
       return { resources: [{ name: "docs", uri: "docs://readme" }] }
     }
 
-    async close() {}
+    // altimate_change start — see listToolsGate
+    async close() {
+      closedClients++
+    }
+    // altimate_change end
   },
 }))
 
@@ -123,6 +139,10 @@ beforeEach(() => {
   connectSucceedsImmediately = false
   serverCapabilities = { tools: {} }
   listToolsCalls = 0
+  // altimate_change start — see listToolsGate
+  listToolsGate = undefined
+  closedClients = 0
+  // altimate_change end
 })
 
 // Import modules after mocking
@@ -274,3 +294,35 @@ mcpTest.instance(
     ),
   { config: config("test-oauth-resources") },
 )
+
+// altimate_change start — the already-authorized path of authenticate() commits a client after
+// its own async listing; a remove that lands during the listing is the later call, and wins. (review)
+mcpTest.instance(
+  "a server removed while authenticate() lists its tools stays removed, and the late client is closed",
+  () =>
+    MCP.Service.use((mcp) =>
+      Effect.gen(function* () {
+        yield* mcp.add("test-oauth-removed", { type: "remote", url: "https://example.com/mcp" })
+
+        simulateAuthFlow = false
+        connectSucceedsImmediately = true
+        let taken!: () => void
+        const wasTaken = new Promise<void>((resolve) => (taken = resolve))
+        let release!: () => void
+        listToolsGate = { taken, release: new Promise<void>((resolve) => (release = resolve)) }
+        const closedBefore = closedClients
+
+        const authenticating = yield* Effect.forkChild(mcp.authenticate("test-oauth-removed"))
+        yield* Effect.promise(() => wasTaken)
+        yield* mcp.remove("test-oauth-removed")
+        release()
+        yield* Fiber.join(authenticating)
+
+        expect((yield* mcp.status())["test-oauth-removed"]?.status).not.toBe("connected")
+        expect((yield* mcp.clients())["test-oauth-removed"]).toBeUndefined()
+        expect(closedClients).toBe(closedBefore + 1)
+      }),
+    ),
+  { config: config("test-oauth-removed") },
+)
+// altimate_change end