From dcf78364cae2b0b8045bf2b53ce0a4a8289ad4cc Mon Sep 17 00:00:00 2001 From: Dominic Cooney Date: Thu, 18 Jun 2026 20:56:48 -0400 Subject: [PATCH] fix(vscode): reliable MCP OAuth on the SDK extension (ENG-2108, CLINE-2304) (#11529) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(vscode): store MCP OAuth in shared settings file like the CLI (ENG-2108) VSCode stored MCP OAuth tokens in a single mcpOAuthSecrets secrets blob keyed by sha256(name:url), while the CLI/SDK store per-server oauth state in cline_mcp_settings.json. The two never interoperated (CLI auth was invisible to VSCode), and VSCode's read-whole-blob/write-whole-blob through StateManager's non-refreshing cache meant concurrent windows clobbered each other's tokens. - Store MCP OAuth state in the shared settings file in @cline/core's format. - Reads are fresh from disk; writes are scoped read-modify-write of one server's oauth key via updateMcpServerOAuthState (now atomic temp+rename). - Replace the vscode:// callback flow with HTTP-based token collection via authorizeMcpServerOAuth (same local loopback flow the CLI uses). - Reconnect an unauthenticated server when its tokens appear (e.g. CLI auth). - One-time migration of legacy mcpOAuthSecrets tokens into the shared file. - Remove McpOAuthRedirectResolver, mcpOAuthFlow, completeOAuth, and the mcp-auth URI callback route. * feat(vscode): add --instances/--random-port to MCP OAuth test server Lets you start several independent test servers, each on its own OS-assigned random port, so you can add multiple streamableHttp MCP servers to Cline at once and exercise concurrent OAuth flows. baseUrl now reflects the actually bound port so discovery metadata and redirect URIs stay correct under random ports. * fix(vscode): stop MCP OAuth handshake writes from livelocking the settings watcher (ENG-2108) Now that codeVerifier/clientInformation live in the shared settings file, the MCP SDK's per-connect-attempt saveCodeVerifier() writes were tripping the settings watcher, which re-entered updateServerConnections -> connectToServer -> another write, looping forever. It was especially bad with two+ unauthenticated servers, where each server's verifier churn re-triggered the other (visible as a flickering, ever-changing codeVerifier nonce). The watcher now compares a connection-relevant fingerprint (full per-server config minus the oauth block, plus a boolean for whether an access token exists) and skips writes that only churn OAuth-handshake fields. A token appearing/disappearing still changes the fingerprint, so CLI/other-window authorization continues to trigger a reconnect via serverGainedOAuthTokens. * feat(vscode): print paste-ready MCP settings fragment from OAuth test server On startup the test server now emits an mcpServers JSON fragment (nested transport shape, matching cline_mcp_settings.json) alongside the banner, so you can paste it straight into the settings file instead of hand-writing it. With --instances the entries get distinct names (oauth-test-1, ...), each carrying its actual bound port. * fix(vscode): atomic MCP settings writes + fingerprint gate; drop timer guards (CLINE-2097) Deleting one MCP server could empty the whole list. Root cause: settings writes were non-atomic (fs.writeFile), so chokidar (and any other process) could read a transient empty/torn file mid-write and reconcile to zero servers. The previous fix only masked this with a per-process isUpdatingClineSettings boolean cleared on a 300ms timer — it did nothing for the CLI or other windows and was racy. Replace both timer guards (isUpdatingClineSettings, isUpdatingFromRemoteConfig) with two deterministic, process-agnostic mechanisms: - writeSettingsFile(): atomic temp-file + rename for every settings write, so any reader always sees a complete file. Holds for any number of concurrent writers (CLI, multiple windows, SDK OAuth handshake). - content fingerprint: the watcher reconciles only when the connection-relevant view changed. writeSettingsFile pre-seeds the fingerprint so our own write is a no-op, while a genuine change from any other process is still processed. Because reconcile is idempotent and reads are never torn, a missed suppression is at worst a redundant reconnect, never data loss. All RPC writers (toggle disabled, autoApprove x2, timeout, add, delete) and the remote-config sync now go through writeSettingsFile. Removes all setTimeout(., 300) flag juggling. * feat(vscode): add a non-guessable 'frozzle' tool to the MCP OAuth test server The MCP OAuth test server now serves tools/list + tools/call exposing a 'frozzle' tool whose output cannot be derived without calling it (reverse the string and swap each letter's case, wrapped in guillemets). This gives an eval a reliable end-to-end signal that the OAuth-authenticated MCP round-trip really happened: a correct 'frozzle ' answer can't be hallucinated. The transform is easy to verify at a glance and invertible. Adds frozzle.test.ts. * fix(sdk): drop lingering OAuth callback sockets on close so deny->approve re-auth works (ENG-2108) The local OAuth callback server's close() called Server.close(), which only stops accepting new connections and lets existing keep-alive sockets linger. The browser / global-fetch connection pool keeps such a socket to the fixed callback port (1456) alive. So after the user denied an MCP OAuth request and retried, the retry's approve callback could be delivered over the pooled socket to the FIRST (already-settled) server. That server's settle() was a no-op, so waitForCallback() never resolved, finishAuth()/token exchange never ran, and no token was saved — the server stayed unauthenticated (the deny->approve repro). Call server.closeAllConnections() in close() so no pooled socket outlives the server. Adds a regression test driving a keep-alive agent across close(). * fix(vscode): actually reconnect MCP server when toggled back on (ENG-2108) toggleServerDisabledRPC only flipped the in-memory disabled flag and set status to 'connecting', but never rebuilt the connection. A disabled server's connection has no live transport/client, so re-enabling left it stuck on the yellow 'connecting' indicator forever and never re-advertised its tools to the agent. Tear down and rebuild the connection through deleteConnection + connectToServer (which opens a real transport when enabled, or a disconnected stub when disabled), then notifyWebviewOfServerChanges so the SDK session's tool list is refreshed. OAuth state is preserved (deleteConnection doesn't clear it). Adds McpHub.toggleServerDisabledRPC.test.ts. * fix(vscode): reload MCP tools silently without chat spam (ENG-2108) Restarting the SDK session to pick up MCP tool changes appended visible chat messages ('MCP tools changed - reloading...' and 'MCP tools reloaded successfully...') plus a completion_result banner. Toggling several servers piled up many of these. Tool reloading should be transparent. Emit only the session status transitions (running -> idle) via emitSessionEvents([], ...) instead of appendAndEmit, so no chat messages or completion banner are shown. Genuine reload failures still surface an error message. Updates sdk-mcp-coordinator.test.ts accordingly. * docs(mcp): clean up comments to describe current behavior Revise comments across the MCP OAuth and settings code to document the code as it stands, dropping references to prior implementations, task IDs, and before/after narration. Also reflow the auth-server regression test to the repository's formatter. No behavior change. * fix(vscode): atomic fallback write in remote MCP sync; document sync OAuth I/O Make the no-McpHub branch of syncRemoteMcpServersToSettings write via an atomic temp-file + rename so a concurrent reader never observes a torn or empty settings file, matching every other settings write. Document why the OAuth state read-modify-write in McpOAuthManager is synchronous: it serializes this process's shared-file updates without a Promise queue, which we prefer over async I/O for reliability of the cross-process settings file. * fix(mcp): serialize settings read-modify-writes * docs(vscode): clarify MCP settings create race * fix(vscode): create MCP settings atomically * fix(cli): keep clearing missing MCP OAuth state a no-op * fix(vscode): avoid yielding while holding MCP settings lock (#11596) * fix(mcp): async lock acquisition for VSCode MCP settings/OAuth writes Add updateMcpSettingsFile/updateMcpServerOAuthStateAsync to @cline/core that yield the event loop while acquiring the cross-process settings lock instead of blocking it with Atomics.wait. The critical section stays synchronous and the mutator stays pure, so the lock is never held across an await and serialization is preserved without an in-process queue. Route the VSCode extension host's OAuth state writes (McpOAuthManager) through the async variant so a connection-time OAuth callback can no longer freeze the extension host event loop or deadlock against an in-flight updateMcpSettingsFile whose lock-releasing continuation needs the loop. Unify the sync and async acquisition paths on a shared reentrancy guard (activeLocks) so a nested settings update on the same file fails fast instead of self-deadlocking. Tests: contended async serialization asserting zero Atomics.wait calls, async stale-lock reclaim, reentrancy fail-fast, and uncontended run+release. * fix(mcp): bootstrap missing settings file inside the lock; tidy docs Creating the MCP settings file now happens in one place: the locked read-modify-write helpers. A missing file reads as an empty settings object, so the first write to a fresh path (e.g. a fresh-install `cline mcp add`) creates it inside the lock instead of throwing ENOENT. The SDK (updateMcpSettingsFile / updateMcpSettingsFileSync) and the VSCode lock helper share this contract, so callers no longer need to pre-create the file. Add regression tests for the SDK, the CLI wizard addServer(), and the VSCode helper on a missing path. Also flag the synchronous SDK entry points (updateMcpSettingsFileSync, updateMcpServerOAuthState) as preferring their async siblings, with a TODO to delete them once all callers migrate, and tighten the lock-helper doc comments to describe current behavior. * fix(vscode): finish npm->bun migration in dev tooling, tasks, and docs The npm->bun migration (#11632) updated package scripts, .vscodeignore and .vscode-test.mjs but left a trail of npm/npx/node invocations in editor configs, dev scripts, and docs. Following the breadcrumbs from 'npm run protos': - .vscode/launch.json: standalone-core debug uses 'bun .ts' (was npx tsx); Open Storybook uses 'bun run' (was npm run). - .vscode/tasks.json: all task commands use 'bun run' (was npm run). - scripts/run-extension-host.sh and .claude/hooks/claude-code-for-web-setup.sh: 'bun run' (was npm run). - debug-harness/server.ts: shebang 'bun'; build steps use 'bun run protos', 'bun esbuild.mjs', 'bunx vite build' (were npm/node/npx). - dev script shebangs (test-hostbridge-server, test-standalone-core-api-server, testing-platform-orchestrator, interactive-playwright): '#!/usr/bin/env bun' (was npx tsx). - WebviewProvider HMR hint, e2e README, copilot-instructions, PR template, mcp-oauth-test-server docs, generate-state-proto message, tsconfig.test comment, state-keys test comment: bun. Left untouched (correct per .clinerules/bun-and-node): Node-runtime invocations (node build.mjs), prebuild-install --target=, vsce, 'npm install -g cline' (user CLI install), and App.stories.tsx mock chat fixtures. --------- Co-authored-by: Saoud Rizwan <7799382+saoudrizwan@users.noreply.github.com> Co-authored-by: Cline Agent --- .claude/hooks/claude-code-for-web-setup.sh | 4 +- .github/copilot-instructions.md | 12 +- .github/pull_request_template.md | 2 +- .vscode/launch.json | 7 +- .vscode/tasks.json | 20 +- apps/cli/src/wizards/mcp/settings.test.ts | 11 + apps/cli/src/wizards/mcp/settings.ts | 134 ++-- apps/cline-hub/src/server/mcp.ts | 60 +- apps/examples/desktop-app/sidecar/commands.ts | 58 +- apps/vscode/scripts/generate-state-proto.mjs | 2 +- apps/vscode/scripts/interactive-playwright.ts | 6 +- apps/vscode/scripts/publish-nightly.mjs | 16 +- apps/vscode/scripts/run-extension-host.sh | 10 +- apps/vscode/scripts/test-hostbridge-server.ts | 2 +- .../test-standalone-core-api-server.ts | 8 +- .../scripts/testing-platform-orchestrator.ts | 10 +- .../__tests__/syncRemoteMcpServers.test.ts | 14 +- apps/vscode/src/core/storage/disk.ts | 15 +- .../remote-config/syncRemoteMcpServers.ts | 92 ++- .../src/core/webview/WebviewProvider.ts | 2 +- apps/vscode/src/dev/commands/tasks.ts | 31 +- apps/vscode/src/dev/debug-harness/server.ts | 8 +- .../src/dev/mcp-oauth-test-server/README.md | 24 +- .../__tests__/frozzle.test.ts | 32 + .../src/dev/mcp-oauth-test-server/server.ts | 251 +++++++- apps/vscode/src/sdk/SdkController.ts | 9 - apps/vscode/src/sdk/auth-service.ts | 8 - .../src/sdk/sdk-mcp-coordinator.test.ts | 40 +- apps/vscode/src/sdk/sdk-mcp-coordinator.ts | 30 +- apps/vscode/src/services/mcp/McpHub.ts | 570 ++++++++++-------- .../src/services/mcp/McpOAuthManager.ts | 562 +++++++---------- .../services/mcp/McpOAuthRedirectResolver.ts | 129 ---- .../__tests__/McpHub.deleteServerRPC.test.ts | 89 ++- .../McpHub.toggleServerDisabledRPC.test.ts | 140 +++++ .../McpOAuthRedirectResolver.test.ts | 280 --------- .../mcp/__tests__/mcpOAuthFlow.test.ts | 34 -- .../mcp/__tests__/settingsLock.test.ts | 53 ++ apps/vscode/src/services/mcp/mcpOAuthFlow.ts | 39 -- apps/vscode/src/services/mcp/settingsLock.ts | 228 +++++++ .../src/services/uri/SharedUriHandler.ts | 14 - .../storage/__tests__/state-keys.test.ts | 2 +- apps/vscode/src/test/e2e/README.md | 16 +- apps/vscode/src/utils/mcpAuth.ts | 15 +- apps/vscode/tsconfig.test.json | 2 +- apps/vscode/vitest.config.ts | 2 + sdk/packages/core/src/auth/server.test.ts | 76 +++ sdk/packages/core/src/auth/server.ts | 9 + .../src/extensions/mcp/config-loader.test.ts | 250 +++++++- .../core/src/extensions/mcp/config-loader.ts | 489 ++++++++++++--- sdk/packages/core/src/extensions/mcp/index.ts | 8 + sdk/packages/core/src/extensions/mcp/oauth.ts | 3 +- sdk/packages/core/src/index.ts | 8 + 52 files changed, 2378 insertions(+), 1558 deletions(-) create mode 100644 apps/vscode/src/dev/mcp-oauth-test-server/__tests__/frozzle.test.ts delete mode 100644 apps/vscode/src/services/mcp/McpOAuthRedirectResolver.ts create mode 100644 apps/vscode/src/services/mcp/__tests__/McpHub.toggleServerDisabledRPC.test.ts delete mode 100644 apps/vscode/src/services/mcp/__tests__/McpOAuthRedirectResolver.test.ts delete mode 100644 apps/vscode/src/services/mcp/__tests__/mcpOAuthFlow.test.ts create mode 100644 apps/vscode/src/services/mcp/__tests__/settingsLock.test.ts delete mode 100644 apps/vscode/src/services/mcp/mcpOAuthFlow.ts create mode 100644 apps/vscode/src/services/mcp/settingsLock.ts diff --git a/.claude/hooks/claude-code-for-web-setup.sh b/.claude/hooks/claude-code-for-web-setup.sh index 38cb4298e1..66750f51c6 100755 --- a/.claude/hooks/claude-code-for-web-setup.sh +++ b/.claude/hooks/claude-code-for-web-setup.sh @@ -41,11 +41,11 @@ fi # Install project dependencies echo "Installing dependencies..." -npm run install:all +bun run install:all # Generate gRPC/protobuf types (required for TypeScript) echo "Generating proto types..." -npm run protos +bun run protos echo "" echo "Session setup complete!" diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 21289c46af..0bd72a52b3 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -9,14 +9,14 @@ This is a VS Code extension. Read `.clinerules/general.md` for tribal knowledge - **MCP**: `src/services/mcp/McpHub.ts`. ## Build & Test (Critical — non-obvious commands) -- **Build**: `npm run compile` — NOT `npm run build`. -- **Watch**: `npm run watch` (extension + webview). -- **Protos**: `npm run protos` — run **immediately** after any `.proto` change. Generates into `src/shared/proto/`, `src/generated/`. -- **Tests**: `npm run test:unit`. After prompt/tool changes: `UPDATE_SNAPSHOTS=true npm run test:unit`. +- **Build**: `bun run compile` — NOT `bun run build`. +- **Watch**: `bun run watch` (extension + webview). +- **Protos**: `bun run protos` — run **immediately** after any `.proto` change. Generates into `src/shared/proto/`, `src/generated/`. +- **Tests**: `bun run test:unit`. After prompt/tool changes: `UPDATE_SNAPSHOTS=true bun run test:unit`. ## Protobuf RPC Workflow (4 steps) 1. **Define** in `proto/cline/*.proto`. Naming: `PascalCaseService`, `camelCase` RPCs, `PascalCase` Messages. Use `common.proto` shared types for simple data. -2. **Generate**: `npm run protos`. +2. **Generate**: `bun run protos`. 3. **Backend handler**: `src/core/controller//`. 4. **Frontend call**: `UiServiceClient.myMethod(Request.create({...}))`. - Adding enums (e.g. `ClineSay`) → also update `src/shared/proto-conversions/cline-message.ts`. @@ -38,7 +38,7 @@ For Responses API providers: add to `isNextGenModelProvider()` in `src/utils/mod 4. Whitelist in `src/core/prompts/system-prompt/variants/*/config.ts` for each model family. 5. Handler in `src/core/task/tools/handlers/`, wire in `ToolExecutor.ts`. 6. If tool has UI: add `ClineSay` enum in proto → `ExtensionMessage.ts` → `cline-message.ts` → `ChatRow.tsx`. -7. Regenerate snapshots: `UPDATE_SNAPSHOTS=true npm run test:unit`. +7. Regenerate snapshots: `UPDATE_SNAPSHOTS=true bun run test:unit`. ## Modifying System Prompt Modular: `components/` (shared) + `variants/` (model-specific) + `templates/` (`{{PLACEHOLDER}}`). Variants override components via `componentOverrides` in `config.ts` or custom `template.ts`. XS variant is heavily condensed inline. Always regenerate snapshots after changes. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 2f969b5921..73c151a6d2 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -59,7 +59,7 @@ We're not looking for exhaustive documentation - just evidence that you've thoug - [ ] Changes are limited to a single feature, bugfix or chore (split larger changes into separate PRs) -- [ ] Tests are passing (`npm test`) and code is formatted and linted (`npm run format && npm run lint`) +- [ ] Tests are passing (`bun test`) and code is formatted and linted (`bun run format && bun run lint`) - [ ] I have reviewed [contributor guidelines](https://github.com/cline/cline/blob/main/CONTRIBUTING.md) ### Screenshots diff --git a/.vscode/launch.json b/.vscode/launch.json index 747c784eb4..0e5f7cf0a3 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -126,10 +126,7 @@ "${workspaceFolder}/apps/vscode/dist-standalone/**/*.js" ], "preLaunchTask": "compile-standalone", - "runtimeExecutable": "npx", - "runtimeArgs": [ - "tsx" - ], + "runtimeExecutable": "bun", "program": "scripts/test-standalone-core-api-server.ts", "envFile": "${workspaceFolder}/apps/vscode/.env", "env": { @@ -183,7 +180,7 @@ "name": "Open Storybook", "type": "node", "request": "launch", - "runtimeExecutable": "npm", + "runtimeExecutable": "bun", "runtimeArgs": [ "run", "storybook" diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 6c04e2a186..31e4e7f07b 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -6,7 +6,7 @@ { "label": "compile-standalone", "type": "shell", - "command": "npm run compile-standalone", + "command": "bun run compile-standalone", "group": "build", "problemMatcher": [], "presentation": { @@ -19,7 +19,7 @@ { "label": "npm: protos", "type": "shell", - "command": "npm run protos", + "command": "bun run protos", "problemMatcher": [], "isBackground": false, "presentation": { @@ -65,7 +65,7 @@ }, { "type": "shell", - "command": "npm run build:webview", + "command": "bun run build:webview", "group": "build", "problemMatcher": [], "isBackground": true, @@ -86,7 +86,7 @@ }, { "type": "shell", - "command": "npm run build:webview:test", + "command": "bun run build:webview:test", "group": "build", "problemMatcher": [], "isBackground": true, @@ -108,7 +108,7 @@ }, { "type": "shell", - "command": "npm run dev:webview", + "command": "bun run dev:webview", "group": "build", "problemMatcher": [ { @@ -145,7 +145,7 @@ }, { "type": "shell", - "command": "npm run watch:esbuild", + "command": "bun run watch:esbuild", "group": "build", "problemMatcher": { "pattern": [ @@ -184,7 +184,7 @@ }, { "type": "shell", - "command": "npm run watch:esbuild:test", + "command": "bun run watch:esbuild:test", "group": "build", "problemMatcher": { "pattern": [ @@ -224,7 +224,7 @@ }, { "type": "shell", - "command": "npm run watch:tsc", + "command": "bun run watch:tsc", "group": "build", "problemMatcher": "$tsc-watch", "isBackground": true, @@ -242,7 +242,7 @@ }, { "type": "shell", - "command": "npm run watch-tests", + "command": "bun run watch-tests", "label": "npm: watch-tests", "problemMatcher": "$tsc-watch", "isBackground": true, @@ -282,7 +282,7 @@ }, { "type": "shell", - "command": "npm run storybook", + "command": "bun run storybook", "group": "build", "problemMatcher": [], "isBackground": false, diff --git a/apps/cli/src/wizards/mcp/settings.test.ts b/apps/cli/src/wizards/mcp/settings.test.ts index 2dfd28f89c..e4facfcb83 100644 --- a/apps/cli/src/wizards/mcp/settings.test.ts +++ b/apps/cli/src/wizards/mcp/settings.test.ts @@ -57,6 +57,17 @@ describe("MCP wizard settings", () => { expect(Object.keys(parsed.mcpServers ?? {})).toEqual(["added"]); }); + it("creates the settings file when adding a server to a missing path", async () => { + const settingsPath = await useTempSettingsPath(); + + addServer("added", { type: "stdio", command: "npx", args: ["server"] }); + + const parsed = JSON.parse(await readFile(settingsPath, "utf8")) as { + mcpServers?: Record; + }; + expect(Object.keys(parsed.mcpServers ?? {})).toEqual(["added"]); + }); + it("parses quoted stdio command arguments", () => { expect( parseStdioCommand('npx -y "@scope/server name" --root "my dir"'), diff --git a/apps/cli/src/wizards/mcp/settings.ts b/apps/cli/src/wizards/mcp/settings.ts index f4582c5f72..5b31a48488 100644 --- a/apps/cli/src/wizards/mcp/settings.ts +++ b/apps/cli/src/wizards/mcp/settings.ts @@ -1,8 +1,9 @@ -import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; -import { dirname } from "node:path"; +import { existsSync, readFileSync } from "node:fs"; import { type McpServerOAuthState, + McpSettingsUpdateSkippedError, resolveDefaultMcpSettingsPath, + updateMcpSettingsFileSync, } from "@cline/core"; export interface McpServerEntry { @@ -56,28 +57,6 @@ export function loadServers(): McpServerEntry[] { } } -function readRawSettings(): Record { - const path = getSettingsPath(); - if (!existsSync(path)) return {}; - try { - const raw = readFileSync(path, "utf-8"); - const parsed = JSON.parse(raw); - return parsed && typeof parsed === "object" && !Array.isArray(parsed) - ? (parsed as Record) - : {}; - } catch { - return {}; - } -} - -function readRawServers(): Record { - const settings = readRawSettings(); - const servers = settings.mcpServers; - return servers && typeof servers === "object" && !Array.isArray(servers) - ? { ...(servers as Record) } - : {}; -} - function getOwnServerRecord( servers: Record, name: string, @@ -92,62 +71,85 @@ function getOwnServerRecord( return value as Record; } -function writeServers(servers: Record): void { - const path = getSettingsPath(); - const settings = readRawSettings(); - mkdirSync(dirname(path), { recursive: true }); - writeFileSync( - path, - `${JSON.stringify({ ...settings, mcpServers: servers }, null, 2)}\n`, - ); +/** + * Mutate the MCP settings file through @cline/core's locked read-update-write + * helper. The mutator must be synchronous and pure; the helper may call it more + * than once to verify deterministic output. Throw McpSettingsUpdateSkippedError + * for normal no-op cases instead of returning a boolean that callers can ignore. + */ +function mutateServers(mutate: (servers: Record) => void): void { + updateMcpSettingsFileSync(getSettingsPath(), (settings) => { + const serversValue = settings.mcpServers; + const servers = serversValue && typeof serversValue === "object" && !Array.isArray(serversValue) + ? { ...(serversValue as Record) } + : {}; + mutate(servers); + settings.mcpServers = servers; + }); } export function addServer(name: string, transport: McpTransport): void { - const servers = readRawServers(); - servers[name] = { transport }; - writeServers(servers); + mutateServers((servers) => { + servers[name] = { transport }; + }); } export function removeServer(name: string): boolean { - const servers = readRawServers(); - if (!(name in servers)) return false; - delete servers[name]; - writeServers(servers); - return true; + try { + mutateServers((servers) => { + if (!(name in servers)) { + throw new McpSettingsUpdateSkippedError(`MCP server not found: ${name}`); + } + delete servers[name]; + }); + return true; + } catch (error) { + if (error instanceof McpSettingsUpdateSkippedError) { + return false; + } + throw error; + } } export function updateServer(name: string, transport: McpTransport): void { - const servers = readRawServers(); - const existing = - servers[name] && typeof servers[name] === "object" - ? (servers[name] as Record) - : {}; - servers[name] = { ...existing, transport }; - writeServers(servers); + mutateServers((servers) => { + const existing = + servers[name] && typeof servers[name] === "object" + ? (servers[name] as Record) + : {}; + servers[name] = { ...existing, transport }; + }); } export function clearServerOAuth(name: string): void { - const servers = readRawServers(); - const existing = getOwnServerRecord(servers, name); - if (!existing) { - return; + try { + mutateServers((servers) => { + const existing = getOwnServerRecord(servers, name); + if (!existing) { + throw new McpSettingsUpdateSkippedError(`MCP server not found: ${name}`); + } + delete existing.oauth; + servers[name] = existing; + }); + } catch (error) { + if (error instanceof McpSettingsUpdateSkippedError) { + return; + } + throw error; } - delete existing.oauth; - servers[name] = existing; - writeServers(servers); } export function toggleServer(name: string, disabled: boolean): void { - const servers = readRawServers(); - const existing = - servers[name] && typeof servers[name] === "object" - ? (servers[name] as Record) - : {}; - if (disabled) { - existing.disabled = true; - } else { - delete existing.disabled; - } - servers[name] = existing; - writeServers(servers); + mutateServers((servers) => { + const existing = + servers[name] && typeof servers[name] === "object" + ? (servers[name] as Record) + : {}; + if (disabled) { + existing.disabled = true; + } else { + delete existing.disabled; + } + servers[name] = existing; + }); } diff --git a/apps/cline-hub/src/server/mcp.ts b/apps/cline-hub/src/server/mcp.ts index c51dfefcc2..b20892d8f5 100644 --- a/apps/cline-hub/src/server/mcp.ts +++ b/apps/cline-hub/src/server/mcp.ts @@ -1,5 +1,5 @@ -import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; -import { dirname } from "node:path"; +import { existsSync, readFileSync } from "node:fs"; +import { updateMcpSettingsFileSync } from "@cline/core"; import { resolveMcpSettingsPath } from "@cline/shared/storage"; import type { JsonRecord } from "./types"; @@ -65,9 +65,9 @@ export function readMcpServersResponse(): JsonRecord { } export function writeMcpServersMap(servers: JsonRecord): void { - const path = resolveMcpSettingsPath(); - mkdirSync(dirname(path), { recursive: true }); - writeFileSync(path, `${JSON.stringify({ mcpServers: servers }, null, 2)}\n`); + updateMcpSettingsFileSync(resolveMcpSettingsPath(), (settings) => { + settings.mcpServers = servers; + }); } export function ensureMcpSettingsFile(): string { @@ -78,23 +78,21 @@ export function ensureMcpSettingsFile(): string { return path; } -function readServersMap(): { path: string; servers: JsonRecord } { - const path = ensureMcpSettingsFile(); - const parsed = JSON.parse(readFileSync(path, "utf8")) as JsonRecord; - return { path, servers: (parsed.mcpServers as JsonRecord | undefined) ?? {} }; -} - export function setMcpServerDisabled( name: string, disabled: boolean, ): JsonRecord { - const { servers } = readServersMap(); - const current = servers[name]; - if (!current || typeof current !== "object") { - throw new Error(`unknown MCP server: ${name}`); - } - servers[name] = { ...(current as JsonRecord), disabled }; - writeMcpServersMap(servers); + // Hold the cross-process lock across read-modify-write so a concurrent writer + // (the extension, the CLI) cannot clobber this change. + updateMcpSettingsFileSync(resolveMcpSettingsPath(), (settings) => { + const servers = ((settings.mcpServers as JsonRecord | undefined) ?? {}) as JsonRecord; + const current = servers[name]; + if (!current || typeof current !== "object") { + throw new Error(`unknown MCP server: ${name}`); + } + servers[name] = { ...(current as JsonRecord), disabled }; + settings.mcpServers = servers; + }); return readMcpServersResponse(); } @@ -127,19 +125,27 @@ export function upsertMcpServer(input: JsonRecord): JsonRecord { }, disabled: input.disabled === true, }; - const { servers } = readServersMap(); - if (previousName && previousName !== name) { - delete servers[previousName]; - } - servers[name] = next; - writeMcpServersMap(servers); + // Hold the cross-process lock across read-modify-write so a concurrent writer + // cannot clobber this upsert. + updateMcpSettingsFileSync(resolveMcpSettingsPath(), (settings) => { + const servers = ((settings.mcpServers as JsonRecord | undefined) ?? {}) as JsonRecord; + if (previousName && previousName !== name) { + delete servers[previousName]; + } + servers[name] = next; + settings.mcpServers = servers; + }); return readMcpServersResponse(); } export function deleteMcpServer(name: string): JsonRecord { if (!name) throw new Error("server name is required"); - const { servers } = readServersMap(); - delete servers[name]; - writeMcpServersMap(servers); + // Hold the cross-process lock across read-modify-write so a concurrent writer + // cannot resurrect the deleted server from a stale snapshot. + updateMcpSettingsFileSync(resolveMcpSettingsPath(), (settings) => { + const servers = ((settings.mcpServers as JsonRecord | undefined) ?? {}) as JsonRecord; + delete servers[name]; + settings.mcpServers = servers; + }); return readMcpServersResponse(); } diff --git a/apps/examples/desktop-app/sidecar/commands.ts b/apps/examples/desktop-app/sidecar/commands.ts index 12697a8eff..36915ca2da 100644 --- a/apps/examples/desktop-app/sidecar/commands.ts +++ b/apps/examples/desktop-app/sidecar/commands.ts @@ -1,11 +1,9 @@ import { execFileSync, spawn } from "node:child_process"; import { existsSync, - mkdirSync, readdirSync, readFileSync, rmSync, - writeFileSync, } from "node:fs"; import { basename, dirname, extname, join } from "node:path"; import type { @@ -45,6 +43,7 @@ import { setDisabledPlugin, setDisabledTools, toggleDisabledTool, + updateMcpSettingsFileSync, } from "@cline/core"; import { getClineEnvironmentConfig } from "@cline/shared"; import { broadcastEvent, resolveSidecarAskQuestion } from "./context"; @@ -143,9 +142,9 @@ function readMcpServersResponse(): JsonRecord { } function writeMcpServersMap(servers: JsonRecord): void { - const path = resolveMcpSettingsPath(); - mkdirSync(dirname(path), { recursive: true }); - writeFileSync(path, `${JSON.stringify({ mcpServers: servers }, null, 2)}\n`); + updateMcpSettingsFileSync(resolveMcpSettingsPath(), (settings) => { + settings.mcpServers = servers; + }); } function ensureMcpSettingsFile(): string { @@ -1043,18 +1042,19 @@ export async function handleCommand( } if (command === "set_mcp_server_disabled") { const path = ensureMcpSettingsFile(); - const parsed = JSON.parse(readFileSync(path, "utf8")) as JsonRecord; - const servers = (parsed.mcpServers as JsonRecord | undefined) ?? {}; - const name = String(args?.name ?? "").trim(); - const current = servers[name]; - if (!current || typeof current !== "object") { - throw new Error(`unknown MCP server: ${name}`); - } - servers[name] = { - ...(current as JsonRecord), - disabled: Boolean(args?.disabled), - }; - writeMcpServersMap(servers); + updateMcpSettingsFileSync(path, (settings) => { + const servers = ((settings.mcpServers as JsonRecord | undefined) ?? {}) as JsonRecord; + const name = String(args?.name ?? "").trim(); + const current = servers[name]; + if (!current || typeof current !== "object") { + throw new Error(`unknown MCP server: ${name}`); + } + servers[name] = { + ...(current as JsonRecord), + disabled: Boolean(args?.disabled), + }; + settings.mcpServers = servers; + }); return readMcpServersResponse(); } if (command === "upsert_mcp_server") { @@ -1093,21 +1093,23 @@ export async function handleCommand( metadata: input.metadata, }; const path = ensureMcpSettingsFile(); - const parsed = JSON.parse(readFileSync(path, "utf8")) as JsonRecord; - const servers = (parsed.mcpServers as JsonRecord | undefined) ?? {}; - if (previousName && previousName !== name) { - delete servers[previousName]; - } - servers[name] = next; - writeMcpServersMap(servers); + updateMcpSettingsFileSync(path, (settings) => { + const servers = ((settings.mcpServers as JsonRecord | undefined) ?? {}) as JsonRecord; + if (previousName && previousName !== name) { + delete servers[previousName]; + } + servers[name] = next; + settings.mcpServers = servers; + }); return readMcpServersResponse(); } if (command === "delete_mcp_server") { const path = ensureMcpSettingsFile(); - const parsed = JSON.parse(readFileSync(path, "utf8")) as JsonRecord; - const servers = (parsed.mcpServers as JsonRecord | undefined) ?? {}; - delete servers[String(args?.name ?? "")]; - writeMcpServersMap(servers); + updateMcpSettingsFileSync(path, (settings) => { + const servers = ((settings.mcpServers as JsonRecord | undefined) ?? {}) as JsonRecord; + delete servers[String(args?.name ?? "")]; + settings.mcpServers = servers; + }); return readMcpServersResponse(); } if (command === "ensure_mcp_settings_file") { diff --git a/apps/vscode/scripts/generate-state-proto.mjs b/apps/vscode/scripts/generate-state-proto.mjs index b5ed1d0120..4386327c3b 100644 --- a/apps/vscode/scripts/generate-state-proto.mjs +++ b/apps/vscode/scripts/generate-state-proto.mjs @@ -453,7 +453,7 @@ async function main() { await fs.writeFile(STATE_PROTO_PATH, protoContent) console.log(`Updated ${STATE_PROTO_PATH}`) - console.log("\nGeneration complete! Run 'npm run protos' to regenerate TypeScript from protos.") + console.log("\nGeneration complete! Run 'bun run protos' to regenerate TypeScript from protos.") } main().catch((error) => { diff --git a/apps/vscode/scripts/interactive-playwright.ts b/apps/vscode/scripts/interactive-playwright.ts index 98d7f7878b..1da9c63295 100644 --- a/apps/vscode/scripts/interactive-playwright.ts +++ b/apps/vscode/scripts/interactive-playwright.ts @@ -1,4 +1,4 @@ -#!/usr/bin/env npx tsx +#!/usr/bin/env bun /** * Interactive Playwright launcher for the Cline VS Code extension. @@ -15,10 +15,10 @@ * * Usage: * 1. (Optional) Build and install the e2e extension: - * npm run test:e2e:build + * bun run test:e2e:build * * 2. From the repo root, start the interactive session: - * npm run test:e2e:ui + * bun run test:e2e:ui * * 3. VS Code will launch with the Cline extension loaded and gRPC recording enabled. * diff --git a/apps/vscode/scripts/publish-nightly.mjs b/apps/vscode/scripts/publish-nightly.mjs index 89852214a3..f7b45cd68b 100755 --- a/apps/vscode/scripts/publish-nightly.mjs +++ b/apps/vscode/scripts/publish-nightly.mjs @@ -35,9 +35,9 @@ * least as often as the scheduled release nightly runs. * * Usage: - * npm run publish:marketplace:nightly # release channel - * npm run publish:marketplace:nightly -- --pre-release # pre-release channel - * npm run publish:marketplace:nightly -- --dry-run # package only + * bun run publish:marketplace:nightly # release channel + * bun run publish:marketplace:nightly -- --pre-release # pre-release channel + * bun run publish:marketplace:nightly -- --dry-run # package only * * Environment variables: * VSCE_PAT - Personal Access Token for VS Code Marketplace @@ -582,7 +582,7 @@ if (showHelp) { Nightly publish script for VS Code extension Usage: - npm run publish:marketplace:nightly [options] + bun run publish:marketplace:nightly [options] Options: --pre-release Publish to the pre-release channel of cline-nightly. @@ -596,10 +596,10 @@ Environment variables: OVSX_PAT Personal Access Token for OpenVSX Registry Examples: - npm run publish:marketplace:nightly # Release channel publish - npm run publish:marketplace:nightly -- --pre-release # Pre-release channel publish - npm run publish:marketplace:nightly -- --dry-run # Package only - VSCE_PAT="token" npm run publish:marketplace:nightly # Publish to VS Code only + bun run publish:marketplace:nightly # Release channel publish + bun run publish:marketplace:nightly -- --pre-release # Pre-release channel publish + bun run publish:marketplace:nightly -- --dry-run # Package only + VSCE_PAT="token" bun run publish:marketplace:nightly # Publish to VS Code only `) process.exit(0) } diff --git a/apps/vscode/scripts/run-extension-host.sh b/apps/vscode/scripts/run-extension-host.sh index 7864e79bc3..6fe9142ec0 100755 --- a/apps/vscode/scripts/run-extension-host.sh +++ b/apps/vscode/scripts/run-extension-host.sh @@ -18,11 +18,11 @@ fi # Step 1: Build protos (everything depends on this) echo "Building protos..." -npm run protos || { echo "Protos build failed"; exit 1; } +bun run protos || { echo "Protos build failed"; exit 1; } # Step 2: Build webview once echo "Building webview..." -npm run build:webview || { echo "Webview build failed"; exit 1; } +bun run build:webview || { echo "Webview build failed"; exit 1; } # Step 3: Kill existing session if one is running tmux kill-session -t "$SESSION" 2>/dev/null @@ -44,9 +44,9 @@ tmux select-layout -t "$SESSION" even-horizontal # Ctrl+C kills the whole session tmux bind-key -T root C-c kill-session -tmux send-keys -t "$SESSION:0.0" "npm run watch:esbuild" Enter -tmux send-keys -t "$SESSION:0.1" "npm run watch:tsc" Enter -tmux send-keys -t "$SESSION:0.2" "npm run dev:webview" Enter +tmux send-keys -t "$SESSION:0.0" "bun run watch:esbuild" Enter +tmux send-keys -t "$SESSION:0.1" "bun run watch:tsc" Enter +tmux send-keys -t "$SESSION:0.2" "bun run dev:webview" Enter tmux send-keys -t "$SESSION:0.3" "while [ ! -f '$WORKSPACE/dist/extension.js' ]; do sleep 0.5; done && echo 'Launching Extension Host...' && code --extensionDevelopmentPath='$WORKSPACE' --disable-workspace-trust --disable-extension saoudrizwan.claude-dev --disable-extension saoudrizwan.cline-nightly '$WORKSPACE' && echo 'Extension Host launched.'" Enter # Attach to the session diff --git a/apps/vscode/scripts/test-hostbridge-server.ts b/apps/vscode/scripts/test-hostbridge-server.ts index 12ea575b4b..2ab040ead2 100755 --- a/apps/vscode/scripts/test-hostbridge-server.ts +++ b/apps/vscode/scripts/test-hostbridge-server.ts @@ -1,4 +1,4 @@ -#!/usr/bin/env npx tsx +#!/usr/bin/env bun import * as grpc from "@grpc/grpc-js" import { ReflectionService } from "@grpc/reflection" import * as health from "grpc-health-check" diff --git a/apps/vscode/scripts/test-standalone-core-api-server.ts b/apps/vscode/scripts/test-standalone-core-api-server.ts index 8c5e88b3c5..09b2bec945 100644 --- a/apps/vscode/scripts/test-standalone-core-api-server.ts +++ b/apps/vscode/scripts/test-standalone-core-api-server.ts @@ -1,4 +1,4 @@ -#!/usr/bin/env npx tsx +#!/usr/bin/env bun /** * Simple Cline gRPC Server @@ -7,8 +7,8 @@ * without requiring the full installation, while automatically mocking all external services. Simply run: * * # One-time setup (generates protobuf files) - * npm run compile-standalone - * npm run test:sca-server + * bun run compile-standalone + * bun run test:sca-server * * The following components are started automatically: * 1. HostBridge test server @@ -66,7 +66,7 @@ async function main(): Promise { console.error(" CLINE_DIST_DIR - Override distribution directory") console.error(" CLINE_CORE_FILE - Override core file name") console.error("") - console.error("To build the standalone version, run: npm run compile-standalone") + console.error("To build the standalone version, run: bun run compile-standalone") process.exit(1) } diff --git a/apps/vscode/scripts/testing-platform-orchestrator.ts b/apps/vscode/scripts/testing-platform-orchestrator.ts index ac71e501d3..a92df0a3ad 100644 --- a/apps/vscode/scripts/testing-platform-orchestrator.ts +++ b/apps/vscode/scripts/testing-platform-orchestrator.ts @@ -1,15 +1,15 @@ -#!/usr/bin/env npx tsx +#!/usr/bin/env bun /** * Test Orchestrator * * Automates server lifecycle for running spec files against the standalone server. * * Prerequisites: - * Build standalone first: `npm run compile-standalone` + * Build standalone first: `bun run compile-standalone` * * Usage: - * - Single file: `npm run test:tp-orchestrator path/to/spec.json` - * - All specs dir: `npm run test:tp-orchestrator tests/specs` + * - Single file: `bun run test:tp-orchestrator path/to/spec.json` + * - All specs dir: `bun run test:tp-orchestrator tests/specs` * * Flags: * --server-logs Show server logs (hidden by default) @@ -209,7 +209,7 @@ async function main() { if (!inputPath) { console.error( - "Usage: npx tsx scripts/testing-platform-orchestrator.ts [--count=N] [--server-logs] [--fix] [--coverage]", + "Usage: bun scripts/testing-platform-orchestrator.ts [--count=N] [--server-logs] [--fix] [--coverage]", ) process.exit(1) } diff --git a/apps/vscode/src/core/storage/__tests__/syncRemoteMcpServers.test.ts b/apps/vscode/src/core/storage/__tests__/syncRemoteMcpServers.test.ts index 2f3dd21380..ad0814a78e 100644 --- a/apps/vscode/src/core/storage/__tests__/syncRemoteMcpServers.test.ts +++ b/apps/vscode/src/core/storage/__tests__/syncRemoteMcpServers.test.ts @@ -234,21 +234,19 @@ describe("syncRemoteMcpServersToSettings", () => { result.mcpServers["my-server"].remoteConfigured.should.equal(true) }) - it("should set McpHub isUpdatingFromRemoteConfig flag during write", async () => { + it("should record the post-write McpHub fingerprint when an McpHub is provided", async () => { await writeSettings({}) const mockMcpHub = { - setIsUpdatingFromRemoteConfig: sandbox.stub(), + recordSettingsFingerprint: sandbox.stub(), } await syncRemoteMcpServersToSettings([{ name: "test", url: "https://test.com" }], tempDir, mockMcpHub as any) - mockMcpHub.setIsUpdatingFromRemoteConfig.calledWith(true).should.be.true() - mockMcpHub.setIsUpdatingFromRemoteConfig.calledWith(false).should.be.true() - - const calls = mockMcpHub.setIsUpdatingFromRemoteConfig.getCalls() - calls[0].args[0].should.equal(true) - calls[1].args[0].should.equal(false) + mockMcpHub.recordSettingsFingerprint.calledOnce.should.be.true() + const result = await readSettings() + result.mcpServers["test"].url.should.equal("https://test.com") + result.mcpServers["test"].remoteConfigured.should.equal(true) }) }) }) diff --git a/apps/vscode/src/core/storage/disk.ts b/apps/vscode/src/core/storage/disk.ts index 1b88fa3989..e35dbd3293 100644 --- a/apps/vscode/src/core/storage/disk.ts +++ b/apps/vscode/src/core/storage/disk.ts @@ -176,9 +176,18 @@ export async function ensureSettingsDirectoryExists(): Promise { */ export async function getMcpSettingsFilePath(settingsDirectoryPath: string): Promise { const mcpSettingsFilePath = path.join(settingsDirectoryPath, GlobalFileNames.mcpSettings) - const fileExists = await fileExistsAtPath(mcpSettingsFilePath) - if (!fileExists) { - await fs.writeFile(mcpSettingsFilePath, JSON.stringify({ mcpServers: {} }, null, 2)) + const tempPath = `${mcpSettingsFilePath}.tmp.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}` + try { + await fs.writeFile(tempPath, JSON.stringify({ mcpServers: {} }, null, 2), { encoding: "utf8", flag: "wx" }) + // Hard-linking publishes the fully-written temp file without overwriting an + // existing settings file. EEXIST means another process won the create race. + await fs.link(tempPath, mcpSettingsFilePath) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "EEXIST") { + throw error + } + } finally { + await fs.unlink(tempPath).catch(() => {}) } return mcpSettingsFilePath } diff --git a/apps/vscode/src/core/storage/remote-config/syncRemoteMcpServers.ts b/apps/vscode/src/core/storage/remote-config/syncRemoteMcpServers.ts index f1369e808c..7fa4635053 100644 --- a/apps/vscode/src/core/storage/remote-config/syncRemoteMcpServers.ts +++ b/apps/vscode/src/core/storage/remote-config/syncRemoteMcpServers.ts @@ -1,7 +1,7 @@ import { getMcpSettingsFilePath } from "@core/storage/disk" import { RemoteMCPServer } from "@shared/remote-config/schema" -import * as fs from "fs/promises" import type { McpHub } from "@/services/mcp/McpHub" +import { updateMcpSettingsFile } from "@/services/mcp/settingsLock" import { Logger } from "@/shared/services/Logger" /** @@ -30,64 +30,54 @@ export async function syncRemoteMcpServersToSettings( // Get or create the MCP settings file const settingsPath = await getMcpSettingsFilePath(settingsDirectoryPath) - // Read current settings - const content = await fs.readFile(settingsPath, "utf-8") - const config = JSON.parse(content) + // Hold the cross-process lock across the whole read-modify-write so a + // concurrent writer (CLI, another window, an OAuth handshake) cannot drop + // this sync's changes from a stale snapshot. Only writers need the lock; + // reads rely on atomic rename to always see a complete file. + const config = await updateMcpSettingsFile(settingsPath, (current) => { + const config = current as Record + const servers = config.mcpServers as Record - // Ensure mcpServers object exists - if (!config.mcpServers || typeof config.mcpServers !== "object") { - config.mcpServers = {} - } - - // Remove servers marked as remoteConfigured that are no longer in the new remote config list. - // This uses the persistent `remoteConfigured` marker in the settings file instead of - // in-memory state, so it works correctly across extension restarts. - for (const [serverName, serverConfig] of Object.entries(config.mcpServers)) { - const server = serverConfig as Record - if (server.remoteConfigured === true) { - const stillInRemoteConfig = remoteMCPServers.some( - (remoteServer) => remoteServer.name === serverName && remoteServer.url === server.url, - ) - if (!stillInRemoteConfig) { - delete config.mcpServers[serverName] + // Remove servers marked as remoteConfigured that are no longer in the new remote config list. + // This uses the persistent `remoteConfigured` marker in the settings file instead of + // in-memory state, so it works correctly across extension restarts. + for (const [serverName, serverConfig] of Object.entries(servers)) { + const server = serverConfig as Record + if (server.remoteConfigured === true) { + const stillInRemoteConfig = remoteMCPServers.some( + (remoteServer) => remoteServer.name === serverName && remoteServer.url === server.url, + ) + if (!stillInRemoteConfig) { + delete servers[serverName] + } } } - } - // Add/update servers from new remote config - for (const server of remoteMCPServers) { - // Check if server with same name and URL already exists to skip duplicates - const existingServer = config.mcpServers[server.name] - if (existingServer && existingServer.url === server.url) { - if (!existingServer.remoteConfigured) { - existingServer.remoteConfigured = true + // Add/update servers from new remote config + for (const server of remoteMCPServers) { + // Check if server with same name and URL already exists to skip duplicates + const existingServer = servers[server.name] + if (existingServer && existingServer.url === server.url) { + if (!existingServer.remoteConfigured) { + existingServer.remoteConfigured = true + } + continue } - continue - } - // Add or update the server with remoteConfigured marker - config.mcpServers[server.name] = { - url: server.url, - type: "streamableHttp", - disabled: false, - autoApprove: [], - remoteConfigured: true, + // Add or update the server with remoteConfigured marker + servers[server.name] = { + url: server.url, + type: "streamableHttp", + disabled: false, + autoApprove: [], + remoteConfigured: true, + } } - } - - // Set flag to prevent watcher from triggering + config.mcpServers = servers + return config + }) if (mcpHub) { - mcpHub.setIsUpdatingFromRemoteConfig(true) - } - - try { - // Write back to file - await fs.writeFile(settingsPath, JSON.stringify(config, null, 2)) - } finally { - // Always clear flag, even if write fails - if (mcpHub) { - mcpHub.setIsUpdatingFromRemoteConfig(false) - } + mcpHub.recordSettingsFingerprint(config.mcpServers) } } catch (error) { Logger.error("[RemoteConfig] Failed to sync remote MCP servers:", error) diff --git a/apps/vscode/src/core/webview/WebviewProvider.ts b/apps/vscode/src/core/webview/WebviewProvider.ts index 19335afb39..4ee187fd10 100644 --- a/apps/vscode/src/core/webview/WebviewProvider.ts +++ b/apps/vscode/src/core/webview/WebviewProvider.ts @@ -168,7 +168,7 @@ export abstract class WebviewProvider { HostProvider.window.showMessage({ type: ShowMessageType.ERROR, message: - "Cline: Local webview dev server is not running, HMR will not work. Please run 'npm run dev:webview' before launching the extension to enable HMR. Using bundled assets.", + "Cline: Local webview dev server is not running, HMR will not work. Please run 'bun run dev:webview' before launching the extension to enable HMR. Using bundled assets.", }) } diff --git a/apps/vscode/src/dev/commands/tasks.ts b/apps/vscode/src/dev/commands/tasks.ts index 543b74b24c..68996fdfa2 100644 --- a/apps/vscode/src/dev/commands/tasks.ts +++ b/apps/vscode/src/dev/commands/tasks.ts @@ -16,27 +16,28 @@ export function registerTaskCommands(controller: Controller): vscode.Disposable[ return [ vscode.commands.registerCommand("cline.dev.expireMcpOAuthTokens", async () => { try { - const stateManager = controller.stateManager - const secretsJson = stateManager.getSecretKey("mcpOAuthSecrets") - - if (!secretsJson) { - vscode.window.showInformationMessage("No MCP OAuth secrets found - no servers are authenticated") - return - } - - const secrets = JSON.parse(secretsJson) + // OAuth tokens live in the shared MCP settings file (per-server + // `oauth.tokens`). Invalidate each access_token so the next request + // gets a 401 and the MCP SDK exercises the refresh_token flow. + const settingsPath = await controller.mcpHub.getMcpSettingsFilePath() + const content = JSON.parse(await fs.readFile(settingsPath, "utf-8")) + const servers = (content?.mcpServers ?? {}) as Record let expiredCount = 0 - // Set all tokens_saved_at to 2 hours ago (past expiration) - for (const hash in secrets) { - if (secrets[hash].tokens_saved_at) { - secrets[hash].tokens_saved_at = Date.now() - 2 * 60 * 60 * 1000 // 2 hours ago + for (const [name, server] of Object.entries(servers)) { + if (server?.oauth?.tokens?.access_token) { + server.oauth.tokens.access_token = "expired-by-dev-command" expiredCount++ - Logger.log(`[Dev] Expired tokens for hash: ${hash}`) + Logger.log(`[Dev] Invalidated access token for server: ${name}`) } } - stateManager.setSecret("mcpOAuthSecrets", JSON.stringify(secrets)) + if (expiredCount === 0) { + vscode.window.showInformationMessage("No MCP OAuth tokens found - no servers are authenticated") + return + } + + await fs.writeFile(settingsPath, JSON.stringify(content, null, 2)) const action = await vscode.window.showInformationMessage( `Expired ${expiredCount} MCP OAuth token(s). Reload window to test token refresh flow.`, diff --git a/apps/vscode/src/dev/debug-harness/server.ts b/apps/vscode/src/dev/debug-harness/server.ts index e8e3c741ac..6e707dcbb2 100644 --- a/apps/vscode/src/dev/debug-harness/server.ts +++ b/apps/vscode/src/dev/debug-harness/server.ts @@ -1,4 +1,4 @@ -#!/usr/bin/env npx tsx +#!/usr/bin/env bun /** * Debug Harness Server @@ -331,10 +331,10 @@ class DebugHarness { if (!opts.skipBuild && !SKIP_BUILD) { log("Building extension (unminified, with sourcemaps)...") const execOpts: ExecSyncOptions = { cwd: PROJECT_ROOT, stdio: "inherit", env: { ...process.env, IS_DEV: "true" } } - execSync("npm run protos", execOpts) - execSync("node esbuild.mjs", execOpts) + execSync("bun run protos", execOpts) + execSync("bun esbuild.mjs", execOpts) log("Building webview (unminified, with inline sourcemaps)...") - execSync("cd webview-ui && npx vite build -- --dev-build", execOpts) + execSync("cd webview-ui && bunx vite build -- --dev-build", execOpts) } // Verify build output exists diff --git a/apps/vscode/src/dev/mcp-oauth-test-server/README.md b/apps/vscode/src/dev/mcp-oauth-test-server/README.md index e3d01333d9..a53f0631eb 100644 --- a/apps/vscode/src/dev/mcp-oauth-test-server/README.md +++ b/apps/vscode/src/dev/mcp-oauth-test-server/README.md @@ -37,7 +37,11 @@ bun run dev:mcp-oauth-test-server -- --verbose bun src/dev/mcp-oauth-test-server/server.ts --verbose ``` -Then in Cline, add an MCP server (StreamableHTTP) pointing at: +On startup the server prints a **paste-ready `mcpServers` JSON fragment** (in +the nested `transport` shape used by `cline_mcp_settings.json`) in addition to +the banner — merge it under `mcpServers` in +`~/.cline/data/settings/cline_mcp_settings.json`. Or add an MCP server +(StreamableHTTP) in Cline by hand, pointing at: ``` http://127.0.0.1:7777/mcp @@ -50,7 +54,9 @@ can click **Approve** or **Deny**. | Flag | Description | |------|-------------| -| `--port ` | Port to listen on (default `7777`, env `MCP_OAUTH_TEST_PORT`) | +| `--port ` | Port to listen on (default `7777`, env `MCP_OAUTH_TEST_PORT`). `0` = OS-assigned random port. | +| `--random-port` | Bind an OS-assigned random free port instead of `--port` | +| `--instances ` | Start N independent servers, each on its own random port (implies `--random-port`). Use to add several MCP servers to Cline at once. | | `--auto-approve` | Skip consent; always approve | | `--auto-deny` | Skip consent; always deny (simulate "Deny" click) | | `--code-ttl ` | Authorization-code lifetime (default `600000`). Set small to force expiry. | @@ -58,6 +64,20 @@ can click **Approve** or **Deny**. | `--verbose`, `-v` | Log every request | | `--help`, `-h` | Show help | +## Adding multiple servers at once + +Each instance binds its own random port and prints its `/mcp` endpoint. Add +each one to Cline as a separate StreamableHTTP server to exercise concurrent +OAuth flows / multiple authenticated servers: + +```bash +bun src/dev/mcp-oauth-test-server/server.ts --instances 3 --verbose +``` + +Because OAuth state is keyed by **server name** in `cline_mcp_settings.json`, +each Cline server entry gets its own independent tokens — even if two point at +the same URL. + ## Reproducing specific bugs **"OAuth state expired" race** — make the user take longer than Cline's diff --git a/apps/vscode/src/dev/mcp-oauth-test-server/__tests__/frozzle.test.ts b/apps/vscode/src/dev/mcp-oauth-test-server/__tests__/frozzle.test.ts new file mode 100644 index 0000000000..d956037b0a --- /dev/null +++ b/apps/vscode/src/dev/mcp-oauth-test-server/__tests__/frozzle.test.ts @@ -0,0 +1,32 @@ +import { describe, it } from "mocha" +import "should" +import { frozzle } from "../server" + +// The `frozzle` tool's value is that its output can't be guessed without +// calling it; these tests pin the exact transform so evals and the server stay +// in sync. If you change frozzle(), update any eval fixtures that assert on it. +describe("frozzle", () => { + it("reverses the string, swaps case, and wraps in guillemets", () => { + frozzle("Hello").should.equal("«OLLEh»") + }) + + it("leaves digits and spaces in place (only letter case is swapped)", () => { + frozzle("Frozzle Me 123").should.equal("«321 Em ELZZORf»") + }) + + it("handles empty input", () => { + frozzle("").should.equal("«»") + }) + + it("swaps each letter's case independently", () => { + frozzle("MixedCase").should.equal("«ESAcDEXIm»") + }) + + it("is invertible: re-frozzling the inner content restores the original", () => { + // frozzle = reverse + swapCase, both self-inverse, so applying the same + // transform to the unwrapped result returns the original input. + const original = "AbCdef 99" + const inner = frozzle(original).slice(1, -1) // strip « » + frozzle(inner).should.equal(`«${original}»`) + }) +}) diff --git a/apps/vscode/src/dev/mcp-oauth-test-server/server.ts b/apps/vscode/src/dev/mcp-oauth-test-server/server.ts index fbc7df1d57..3cc4ef8161 100644 --- a/apps/vscode/src/dev/mcp-oauth-test-server/server.ts +++ b/apps/vscode/src/dev/mcp-oauth-test-server/server.ts @@ -23,7 +23,11 @@ * * 2. MCP StreamableHTTP resource server: * POST /mcp (returns 401 + WWW-Authenticate until authed, - * then a minimal initialize response) + * then initialize + a `frozzle` tool) + * + * The `frozzle` tool exists so an eval can prove the OAuth'd MCP round-trip + * actually happened: its output is not derivable without calling the tool, so + * a correct "frozzle " answer can't be hallucinated. See frozzle(). * * The endpoint shapes match what `@modelcontextprotocol/sdk` v1.25.x discovers * (see node_modules/@modelcontextprotocol/sdk/dist/esm/client/auth.js). @@ -45,7 +49,7 @@ * * Run interactively: * cd apps/vscode - * npx tsx src/dev/mcp-oauth-test-server/server.ts --verbose + * bun src/dev/mcp-oauth-test-server/server.ts --verbose * * Then add an MCP server to Cline pointing at: * http://127.0.0.1:7777/mcp (type: streamableHttp) @@ -68,6 +72,18 @@ interface TestServerOptions { codeTtlMs: number slowAuthorizeMs: number verbose: boolean + /** + * Number of independent server instances to start. When > 1, each instance + * binds its own OS-assigned random port (the fixed `--port` can't be shared), + * so you can add several distinct MCP servers to Cline at once and exercise + * concurrent OAuth flows. + */ + instances: number + /** + * Bind an OS-assigned random free port instead of the fixed `--port`. + * Implied when `--instances` > 1. Also enabled by passing `--port 0`. + */ + randomPort: boolean } function parseArgs(argv: string[]): TestServerOptions { @@ -79,6 +95,8 @@ function parseArgs(argv: string[]): TestServerOptions { codeTtlMs: 10 * 60 * 1000, slowAuthorizeMs: 0, verbose: false, + instances: 1, + randomPort: false, } for (let i = 0; i < argv.length; i++) { const arg = argv[i] @@ -98,6 +116,13 @@ function parseArgs(argv: string[]): TestServerOptions { case "--slow-authorize": opts.slowAuthorizeMs = Number(argv[++i]) break + case "--instances": + opts.instances = Number(argv[++i]) + break + case "--random-port": + case "--random-ports": + opts.randomPort = true + break case "--verbose": case "-v": opts.verbose = true @@ -115,22 +140,45 @@ function parseArgs(argv: string[]): TestServerOptions { console.error("Cannot set both --auto-approve and --auto-deny") process.exit(1) } + if (!Number.isInteger(opts.instances) || opts.instances < 1) { + console.error(`--instances must be a positive integer (got ${opts.instances})`) + process.exit(1) + } + // `--port 0` is a conventional request for an OS-assigned random port. + if (opts.port === 0) { + opts.randomPort = true + } + // Multiple instances can't share one fixed port, so each gets a random one. + if (opts.instances > 1) { + opts.randomPort = true + } return opts } function printUsageAndExit(code = 0): never { console.log(`MCP OAuth Test Server -Usage: npx tsx src/dev/mcp-oauth-test-server/server.ts [options] +Usage: bun src/dev/mcp-oauth-test-server/server.ts [options] Options: - --port Port to listen on (default 7777) + --port Port to listen on (default 7777; 0 = OS-assigned random) + --random-port Bind an OS-assigned random free port instead of --port + --instances Start N independent servers, each on its own random + port (implies --random-port). Use to add several MCP + servers to Cline at once. --auto-approve Always approve authorization (no consent screen) --auto-deny Always deny authorization (simulate "Deny" click) --code-ttl Authorization code lifetime (default 600000) --slow-authorize Delay /authorize response by --verbose, -v Log every request --help, -h Show this help + +Examples: + # Single server on the default fixed port + bun src/dev/mcp-oauth-test-server/server.ts --verbose + + # Three servers on random ports, to test adding multiple at once + bun src/dev/mcp-oauth-test-server/server.ts --instances 3 --verbose `) process.exit(code) } @@ -172,13 +220,31 @@ class TestServer { private readonly authCodes = new Map() private readonly refreshTokens = new Map() private server: http.Server | null = null + /** + * The port actually bound. Differs from `opts.port` when `randomPort` is + * set (the OS assigns it), and is the value every absolute URL we emit + * (discovery metadata, redirect targets, the /mcp resource id) must use — + * otherwise the SDK's redirect_uri / resource checks fail. + */ + private boundPort = 0 constructor(opts: TestServerOptions) { this.opts = opts + this.boundPort = opts.port } private get baseUrl(): string { - return `http://${this.opts.host}:${this.opts.port}` + return `http://${this.opts.host}:${this.boundPort}` + } + + /** The port this server is actually listening on (resolved after start). */ + get port(): number { + return this.boundPort + } + + /** The MCP StreamableHTTP endpoint clients should connect to. */ + get mcpEndpoint(): string { + return `${this.baseUrl}/mcp` } private log(...args: unknown[]): void { @@ -187,21 +253,35 @@ class TestServer { } } - start(): void { - this.server = http.createServer((req, res) => { - this.handleRequest(req, res).catch((err) => { - console.error("[mcp-oauth-test] Unhandled error:", err) - if (!res.headersSent) { - this.json(res, 500, { error: "server_error", error_description: String(err) }) + /** + * Start listening. Resolves once bound, so callers can read `.port` + * (important when binding an OS-assigned random port). + */ + start(): Promise { + return new Promise((resolve, reject) => { + this.server = http.createServer((req, res) => { + this.handleRequest(req, res).catch((err) => { + console.error("[mcp-oauth-test] Unhandled error:", err) + if (!res.headersSent) { + this.json(res, 500, { error: "server_error", error_description: String(err) }) + } + }) + }) + this.server.once("error", reject) + // `randomPort` → listen on 0 so the OS assigns a free port. + const listenPort = this.opts.randomPort ? 0 : this.opts.port + this.server.listen(listenPort, this.opts.host, () => { + const address = this.server?.address() + if (address && typeof address === "object") { + this.boundPort = address.port } + console.log(`MCP OAuth Test Server listening on ${this.baseUrl}`) + console.log(` MCP endpoint: ${this.baseUrl}/mcp (type: streamableHttp)`) + console.log(` Authorize page: ${this.baseUrl}/authorize`) + const mode = this.opts.autoApprove ? "auto-approve" : this.opts.autoDeny ? "auto-deny" : "interactive consent" + console.log(` Mode: ${mode}, code TTL: ${this.opts.codeTtlMs}ms`) + resolve() }) - }) - this.server.listen(this.opts.port, this.opts.host, () => { - console.log(`MCP OAuth Test Server listening on ${this.baseUrl}`) - console.log(` MCP endpoint: ${this.baseUrl}/mcp (type: streamableHttp)`) - console.log(` Authorize page: ${this.baseUrl}/authorize`) - const mode = this.opts.autoApprove ? "auto-approve" : this.opts.autoDeny ? "auto-deny" : "interactive consent" - console.log(` Mode: ${mode}, code TTL: ${this.opts.codeTtlMs}ms`) }) } @@ -500,6 +580,62 @@ class TestServer { }, }) } + + // Advertise the `frozzle` tool. Its description deliberately does NOT + // reveal what frozzling does, so a model cannot fabricate the result — + // the only way to produce a correct answer is to actually call the tool + // over MCP. This makes it a reliable end-to-end signal that the OAuth'd + // MCP connection works (vs. the model hallucinating an answer). + if (body?.method === "tools/list") { + return this.json(res, 200, { + jsonrpc: "2.0", + id, + result: { + tools: [ + { + name: "frozzle", + description: + "Frozzle the given text and return its frozzled form. " + + "The frozzling transform is defined solely by this server; " + + "there is no way to compute the result without calling this tool.", + inputSchema: { + type: "object", + properties: { + text: { type: "string", description: "The text to frozzle." }, + }, + required: ["text"], + }, + }, + ], + }, + }) + } + + if (body?.method === "tools/call") { + const params = (body?.params ?? {}) as { name?: string; arguments?: Record } + if (params.name === "frozzle") { + const text = typeof params.arguments?.text === "string" ? params.arguments.text : "" + const frozzled = frozzle(text) + this.log(`frozzle(${JSON.stringify(text)}) -> ${JSON.stringify(frozzled)}`) + return this.json(res, 200, { + jsonrpc: "2.0", + id, + result: { + content: [{ type: "text", text: frozzled }], + }, + }) + } + // Unknown tool. + return this.json(res, 200, { + jsonrpc: "2.0", + id, + result: { + isError: true, + content: [{ type: "text", text: `Unknown tool: ${String(params.name)}` }], + }, + }) + } + // Any other method: empty-ish OK so the SDK doesn't error out. return this.json(res, 200, { jsonrpc: "2.0", id, result: {} }) } @@ -604,6 +740,37 @@ function base64UrlSha256(input: string): string { return crypto.createHash("sha256").update(input).digest("base64url") } +/** + * The "frozzle" transform exposed by the test server's MCP `frozzle` tool. + * + * The point of frozzling is that it is arbitrary and non-obvious: a model + * cannot guess or compute the result without actually calling the tool over + * the (OAuth-authenticated) MCP connection. So when an eval asks the agent to + * "frozzle " and checks the answer, a correct result is proof the MCP + * round-trip really happened — not a hallucination. + * + * It is nonetheless deterministic, easy to verify at a glance, and invertible: + * reverse the string and swap the case of each letter (upper<->lower), then + * wrap in « » markers. e.g. frozzle("Hello") === "«OLLEh»". + */ +export function frozzle(text: string): string { + const swapped = [...text] + .reverse() + .map((ch) => { + const lower = ch.toLowerCase() + const upper = ch.toUpperCase() + if (ch === lower && ch !== upper) { + return upper // lowercase -> uppercase + } + if (ch === upper && ch !== lower) { + return lower // uppercase -> lowercase + } + return ch // non-letters unchanged + }) + .join("") + return `«${swapped}»` +} + function asString(value: unknown): string | undefined { return typeof value === "string" ? value : undefined } @@ -620,18 +787,58 @@ function delay(ms: number): Promise { // Entry point // --------------------------------------------------------------------------- -export { parseArgs, TestServer, type TestServerOptions } +export { buildSettingsFragment, parseArgs, TestServer, type TestServerOptions } + +/** + * Build a paste-ready `mcpServers` fragment for cline_mcp_settings.json from a + * set of running test-server endpoints. + * + * Uses the nested `transport` shape the CLI/SDK writes (and the extension also + * accepts), so the output can be dropped straight into the settings file. When + * there are multiple endpoints the names are suffixed (`oauth-test-1`, …) since + * OAuth state is keyed by server name — distinct names get independent tokens. + */ +function buildSettingsFragment(endpoints: string[]): string { + const mcpServers: Record = {} + endpoints.forEach((endpoint, index) => { + const name = endpoints.length === 1 ? "oauth-test" : `oauth-test-${index + 1}` + mcpServers[name] = { + transport: { + type: "streamableHttp", + url: endpoint, + }, + } + }) + return JSON.stringify({ mcpServers }, null, 2) +} // Only auto-start when run directly (so this module can be imported by the // debug harness later without spawning a server). const isMain = process.argv[1] && /mcp-oauth-test-server[/\\]server\.(ts|js)$/.test(process.argv[1]) if (isMain) { const opts = parseArgs(process.argv.slice(2)) - const server = new TestServer(opts) - server.start() + const servers: TestServer[] = [] + + void (async () => { + for (let i = 0; i < opts.instances; i++) { + const server = new TestServer(opts) + await server.start() + servers.push(server) + } + if (opts.instances > 1) { + console.log(`\nStarted ${opts.instances} MCP OAuth test servers.`) + } + // Emit a paste-ready settings fragment so you don't have to hand-write + // the JSON (handy with random ports / multiple instances). + console.log("\nPaste into ~/.cline/data/settings/cline_mcp_settings.json (merge under mcpServers):\n") + console.log(buildSettingsFragment(servers.map((server) => server.mcpEndpoint))) + })() + process.on("SIGINT", () => { console.log("\nShutting down...") - server.stop() + for (const server of servers) { + server.stop() + } process.exit(0) }) } diff --git a/apps/vscode/src/sdk/SdkController.ts b/apps/vscode/src/sdk/SdkController.ts index a7f4be77f5..0d4b276347 100644 --- a/apps/vscode/src/sdk/SdkController.ts +++ b/apps/vscode/src/sdk/SdkController.ts @@ -1181,15 +1181,6 @@ export class Controller { await this.postStateToWebview() } - async handleMcpOAuthCallback(serverHash: string, code: string, state: string | null): Promise { - try { - await this.mcpHub.completeOAuth(serverHash, code, state) - await this.postStateToWebview() - } catch (error) { - Logger.error("Failed to complete MCP OAuth:", error) - } - } - // ---- Provider auth callbacks ---- private persistProviderApiKeyFromState(provider: string): void { diff --git a/apps/vscode/src/sdk/auth-service.ts b/apps/vscode/src/sdk/auth-service.ts index 88cf7d1588..957482fc13 100644 --- a/apps/vscode/src/sdk/auth-service.ts +++ b/apps/vscode/src/sdk/auth-service.ts @@ -822,14 +822,6 @@ export class AuthService { Logger.warn("[SdkAuthService] handleOcaAuthCallback called — OCA uses SDK callback server") } - /** - * Handle MCP OAuth callback. - */ - async handleMcpOAuthCallback(_serverHash: string, _code: string, _state: string | null): Promise { - // MCP OAuth callbacks are not yet handled by the SDK adapter. - Logger.warn("[SdkAuthService] handleMcpOAuthCallback not yet implemented") - } - // ---- Restore auth on startup ---- /** diff --git a/apps/vscode/src/sdk/sdk-mcp-coordinator.test.ts b/apps/vscode/src/sdk/sdk-mcp-coordinator.test.ts index c4583bfa0c..fee94f43dc 100644 --- a/apps/vscode/src/sdk/sdk-mcp-coordinator.test.ts +++ b/apps/vscode/src/sdk/sdk-mcp-coordinator.test.ts @@ -44,16 +44,12 @@ describe("SdkMcpCoordinator", () => { coordinator.handleToolListChanged() await vi.waitFor(() => expect(options.sessions.replaceActiveSession).toHaveBeenCalledOnce()) - expect(options.messages.appendAndEmit).toHaveBeenCalledWith( - [ - expect.objectContaining({ - type: "say", - say: "info", - text: "MCP tools changed - reloading tools for this session...", - }), - ], - { type: "status", payload: { sessionId: "old-session", status: "running" } }, - ) + // Reloading tools is silent: only a status transition is emitted, no chat message. + expect(options.messages.emitSessionEvents).toHaveBeenCalledWith([], { + type: "status", + payload: { sessionId: "old-session", status: "running" }, + }) + expect(options.messages.appendAndEmit).not.toHaveBeenCalled() }) it("rebuilds the active session with the current mode and preserved messages", async () => { @@ -73,17 +69,13 @@ describe("SdkMcpCoordinator", () => { initialMessages: [{ role: "user", content: "hello" }], disposeReason: "mcpToolRestart", }) - expect(options.messages.appendAndEmit).toHaveBeenLastCalledWith( - [ - expect.objectContaining({ - type: "say", - say: "info", - text: "MCP tools reloaded successfully. You can continue your conversation.", - }), - expect.objectContaining({ type: "ask", ask: "completion_result" }), - ], - { type: "status", payload: { sessionId: "new-session", status: "idle" } }, - ) + // Success is silent: only a status transition back to idle, no chat + // message or completion banner. + expect(options.messages.emitSessionEvents).toHaveBeenCalledWith([], { + type: "status", + payload: { sessionId: "new-session", status: "idle" }, + }) + expect(options.messages.appendAndEmit).not.toHaveBeenCalled() expect(options.postStateToWebview).toHaveBeenCalledOnce() }) @@ -128,6 +120,7 @@ function makeCoordinator(input: Partial = {}) { }, messages: { appendAndEmit: vi.fn(), + emitSessionEvents: vi.fn(), }, sessionConfigBuilder: { build: vi.fn().mockResolvedValue(config), @@ -142,7 +135,10 @@ function makeCoordinator(input: Partial = {}) { getActiveSession: ReturnType replaceActiveSession: ReturnType } - messages: SdkMcpCoordinatorOptions["messages"] & { appendAndEmit: ReturnType } + messages: SdkMcpCoordinatorOptions["messages"] & { + appendAndEmit: ReturnType + emitSessionEvents: ReturnType + } sessionConfigBuilder: SdkMcpCoordinatorOptions["sessionConfigBuilder"] & { build: ReturnType } getWorkspaceRoot: ReturnType loadInitialMessages: ReturnType diff --git a/apps/vscode/src/sdk/sdk-mcp-coordinator.ts b/apps/vscode/src/sdk/sdk-mcp-coordinator.ts index 4fc2869b8c..5fa2f56e01 100644 --- a/apps/vscode/src/sdk/sdk-mcp-coordinator.ts +++ b/apps/vscode/src/sdk/sdk-mcp-coordinator.ts @@ -75,14 +75,10 @@ export class SdkMcpCoordinator { Logger.log(`[SdkController] Restarting session ${oldSessionId} for MCP tool changes`) - const infoMessage: ClineMessage = { - ts: Date.now(), - type: "say", - say: "info", - text: "MCP tools changed - reloading tools for this session...", - partial: false, - } - this.options.messages.appendAndEmit([infoMessage], { + // Reloading tools is a silent, behind-the-scenes operation — it should + // "just work" without spamming the chat. Emit only the status transition + // (no chat message), so toggling several servers doesn't pile up notices. + this.options.messages.emitSessionEvents([], { type: "status", payload: { sessionId: oldSessionId, status: "running" }, }) @@ -112,21 +108,9 @@ export class SdkMcpCoordinator { ) } - const successMessage: ClineMessage = { - ts: Date.now(), - type: "say", - say: "info", - text: "MCP tools reloaded successfully. You can continue your conversation.", - partial: false, - } - const completionAsk: ClineMessage = { - ts: successMessage.ts + 1, - type: "ask", - ask: "completion_result", - text: "", - partial: false, - } - this.options.messages.appendAndEmit([successMessage, completionAsk], { + // Silently return the session to idle — no "reloaded successfully" + // chat message or completion banner. The reload is transparent. + this.options.messages.emitSessionEvents([], { type: "status", payload: { sessionId: startResult.sessionId, status: "idle" }, }) diff --git a/apps/vscode/src/services/mcp/McpHub.ts b/apps/vscode/src/services/mcp/McpHub.ts index a0a3d0e757..03e0037324 100644 --- a/apps/vscode/src/services/mcp/McpHub.ts +++ b/apps/vscode/src/services/mcp/McpHub.ts @@ -41,10 +41,10 @@ import { fetch } from "@/shared/net" import { ShowMessageType } from "@/shared/proto/host/window" import { Logger } from "@/shared/services/Logger" import { expandEnvironmentVariables } from "@/utils/envExpansion" -import { getServerAuthHash } from "@/utils/mcpAuth" import type { TelemetryService } from "../telemetry/TelemetryService" import { DEFAULT_REQUEST_TIMEOUT_MS } from "./constants" import { McpOAuthManager } from "./McpOAuthManager" +import { updateMcpSettingsFile } from "./settingsLock" import { StreamableHttpReconnectHandler } from "./StreamableHttpReconnectHandler" import { BaseConfigSchema, McpSettingsSchema, ServerConfigSchema } from "./schemas" import type { McpConnection, McpServerConfig, Transport } from "./types" @@ -60,23 +60,27 @@ export class McpHub { connections: McpConnection[] = [] isConnecting = false /** - * Flag to skip file watcher processing when we're updating Cline-specific settings - * (autoApprove, timeout) that don't require an MCP server restart. + * Fingerprint of the connection-relevant view of the settings file as of the + * watcher's last reconciliation. * - * The file watcher has a 100ms stabilityThreshold before firing "change" events. - * When we update settings, we set this flag to true, write the file, then clear - * the flag after 300ms. This ensures the flag is still true when the delayed - * file watcher event fires, so we can skip redundant processing. + * The settings-file watcher uses this single, process-agnostic check to + * decide whether a file change needs action: it recomputes the fingerprint + * and skips when it's unchanged. Because the fingerprint is keyed on file + * CONTENT rather than on who wrote it: + * - writes that change nothing connection-relevant (e.g. OAuth + * codeVerifier/clientInformation churn during a handshake) are no-ops, + * which prevents a self-perpetuating watcher → reconnect → write loop; + * - a write from any other process (CLI, another window) that does change + * something is processed normally; + * - whether an access token is present is part of the fingerprint, so an + * authorization completed elsewhere still triggers a reconnect via + * serverGainedOAuthTokens. * - * Timeline: - * 0ms: flag = true, write file - * ~100ms: file watcher fires "change" → sees flag=true → skips - * 300ms: flag = false (ready for external file changes) + * Combined with atomic writes, a reader can never + * observe a torn/empty file mid-write, so the worst case is a redundant + * reconciliation rather than dropping the server list. */ - private isUpdatingClineSettings = false - - // Track when remote config is updating to prevent unnecessary watcher triggers - private isUpdatingFromRemoteConfig = false + private lastConnectionFingerprint?: string /** * Map of unique keys to each connected server names @@ -116,7 +120,7 @@ export class McpHub { this.getSettingsDirectoryPath = getSettingsDirectoryPath this.clientVersion = clientVersion this.telemetryService = telemetryService - this.mcpOAuthManager = new McpOAuthManager() + this.mcpOAuthManager = new McpOAuthManager(() => this.getMcpSettingsFilePath()) this.watchMcpSettingsFile() this.initializeMcpServers() } @@ -163,18 +167,20 @@ export class McpHub { } /** - * Sets the flag to indicate remote config is updating - * Used to prevent watcher from triggering on remote config writes + * Record the post-write connection fingerprint so this window's watcher treats + * its own write as a no-op. This does not write the settings file. */ - setIsUpdatingFromRemoteConfig(value: boolean): void { - this.isUpdatingFromRemoteConfig = value + recordSettingsFingerprint(servers: Record): void { + this.lastConnectionFingerprint = this.computeConnectionFingerprint(servers) } - /** - * Gets whether remote config is currently updating - */ - getIsUpdatingFromRemoteConfig(): boolean { - return this.isUpdatingFromRemoteConfig + private async readPostWriteMcpSettings(): Promise> { + const settings = await this.readAndValidateMcpSettingsFile() + if (!settings) { + throw new Error("Failed to read or validate MCP settings after write") + } + this.recordSettingsFingerprint(settings.mcpServers as Record) + return settings } private async readAndValidateMcpSettingsFile(): Promise | undefined> { @@ -283,17 +289,21 @@ export class McpHub { }) this.settingsWatcher.on("change", async () => { - // Skip if remote config is currently updating to prevent unnecessary reconnections - if (this.isUpdatingFromRemoteConfig) { - return - } - // Skip processing if we're updating Cline-specific settings (autoApprove, timeout) - if (this.isUpdatingClineSettings) { - return - } - const settings = await this.readAndValidateMcpSettingsFile() if (settings) { + // Skip when nothing connection-relevant changed. This covers our own + // writes (callers pre-seed the fingerprint) as well as + // OAuth-handshake churn from the SDK (codeVerifier/clientInformation + // rewrites on every connect attempt for unauthenticated servers). A + // write from the CLI or another window that genuinely changes a + // server, or a token appearing/disappearing, produces a different + // fingerprint and is processed normally. + const fingerprint = this.computeConnectionFingerprint(settings.mcpServers as Record) + if (fingerprint === this.lastConnectionFingerprint) { + return + } + this.lastConnectionFingerprint = fingerprint + try { // Re-add any remotely configured servers that were manually removed from the file const remoteServers = StateManager.get().getRemoteConfigSettings().remoteMCPServers @@ -312,10 +322,25 @@ export class McpHub { } } if (fileNeedsUpdate) { - this.isUpdatingFromRemoteConfig = true const settingsPath = await getMcpSettingsFilePathHelper(await this.getSettingsDirectoryPath()) - await fs.writeFile(settingsPath, JSON.stringify({ mcpServers: settings.mcpServers }, null, 2)) - this.isUpdatingFromRemoteConfig = false + const fresh = await updateMcpSettingsFile(settingsPath, (current) => { + const servers = current.mcpServers as Record + for (const rs of remoteServers) { + if (!servers[rs.name]) { + servers[rs.name] = { + url: rs.url, + type: "streamableHttp", + disabled: false, + autoApprove: [], + remoteConfigured: true, + } + } + } + current.mcpServers = servers + return current + }) + this.recordSettingsFingerprint(fresh.mcpServers as Record) + settings.mcpServers = fresh.mcpServers as any } } await this.updateServerConnections(settings.mcpServers) @@ -333,6 +358,11 @@ export class McpHub { private async initializeMcpServers(): Promise { const settings = await this.readAndValidateMcpSettingsFile() if (settings) { + // Seed the watcher's baseline so the first post-startup write is + // compared against the current connection-relevant state, not undefined. + this.lastConnectionFingerprint = this.computeConnectionFingerprint( + settings.mcpServers as Record, + ) await this.updateServerConnections(settings.mcpServers) } } @@ -899,8 +929,12 @@ export class McpHub { } catch (error) { Logger.error(`Failed to connect to new MCP server ${name}:`, error) } - } else if (this.configsRequireRestart(JSON.parse(currentConnection.server.config), config)) { - // Existing server with changed connection config (excludes Cline-specific settings) + } else if ( + this.configsRequireRestart(JSON.parse(currentConnection.server.config), config) || + this.serverGainedOAuthTokens(currentConnection, config) + ) { + // Existing server with changed connection config (excludes Cline-specific settings), + // or an unauthenticated server whose OAuth tokens just appeared (e.g. CLI authorized it) try { if (config.type === "stdio") { this.setupFileWatcher(name, config) @@ -968,8 +1002,13 @@ export class McpHub { } catch (error) { Logger.error(`Failed to connect to new MCP server ${name}:`, error) } - } else if (this.configsRequireRestart(JSON.parse(currentConnection.server.config), config)) { - // Existing server with changed connection config (excludes Cline-specific settings) + } else if ( + this.configsRequireRestart(JSON.parse(currentConnection.server.config), config) || + this.serverGainedOAuthTokens(currentConnection, config) + ) { + // Existing server with changed connection config (excludes Cline-specific settings), + // or an unauthenticated server whose OAuth tokens just appeared in the settings + // file (e.g. the CLI or another window completed authorization for it) try { // Set status to "connecting" and notify webview before restart (same pattern as restartConnection) currentConnection.server.status = "connecting" @@ -1027,27 +1066,77 @@ export class McpHub { * ## Adding new Cline-specific settings: * When adding a new setting that doesn't require server restart: * 1. Add it to the destructuring below to exclude from comparison - * 2. Add it to `isUpdatingClineSettings` flag usage in the update function + * 2. Add it to computeConnectionFingerprint() if a change to it should (or + * should not) wake the settings watcher * 3. Update in-memory state (e.g., `connection.server.config`) in the update function * 4. Update the schema in `src/services/mcp/schemas.ts` if needed */ private configsRequireRestart(oldConfig: McpServerConfig, newConfig: McpServerConfig): boolean { - // Exclude Cline-specific settings from comparison (add new ones here) + // Exclude Cline-specific settings from comparison (add new ones here). + // `oauth` and `metadata` are also excluded: the server's oauth block is + // rewritten on every token save/refresh (by this process, the CLI, or + // another window), and restarting on each refresh would churn the + // connection. Token changes are picked up separately, by + // serverGainedOAuthTokens in updateServerConnections. const { autoApprove: _oldAutoApprove, timeout: _oldTimeout, remoteConfigured: _oldRemoteConfigured, + oauth: _oldOauth, + metadata: _oldMetadata, ...oldConnectionConfig - } = oldConfig + } = oldConfig as McpServerConfig & { oauth?: unknown; metadata?: unknown } const { autoApprove: _newAutoApprove, timeout: _newTimeout, remoteConfigured: _newRemoteConfigured, + oauth: _newOauth, + metadata: _newMetadata, ...newConnectionConfig - } = newConfig + } = newConfig as McpServerConfig & { oauth?: unknown; metadata?: unknown } return !deepEqual(oldConnectionConfig, newConnectionConfig) } + /** + * True when an unauthenticated server's settings entry now carries an access + * token — e.g. the CLI or another window completed OAuth for it. The settings + * watcher uses this to reconnect the server so it picks up the credentials. + */ + private serverGainedOAuthTokens(connection: McpConnection, newConfig: McpServerConfig): boolean { + if (connection.server.oauthAuthStatus !== "unauthenticated") { + return false + } + const oauth = (newConfig as McpServerConfig & { oauth?: { tokens?: { access_token?: unknown } } }).oauth + return typeof oauth?.tokens?.access_token === "string" && oauth.tokens.access_token.length > 0 + } + + /** + * Builds a fingerprint of only the parts of the settings file that affect + * how connections are managed (see lastConnectionFingerprint). Per server it + * captures the full config minus the `oauth` block, plus a single boolean for + * whether a usable access token exists. + * + * Excluding the rest of the `oauth` block means OAuth-handshake churn + * (codeVerifier, clientInformation, discoveryState, lastError), which the MCP + * SDK rewrites on every connect attempt, does not change the fingerprint. The + * access-token boolean is included so that an authorization completing + * elsewhere (token appears or disappears) does change it. + */ + private computeConnectionFingerprint(mcpServers: Record): string { + const normalized: Record = {} + for (const name of Object.keys(mcpServers).sort()) { + const { oauth, ...connectionConfig } = mcpServers[name] as McpServerConfig & { + oauth?: { tokens?: { access_token?: unknown } } + } + const accessToken = oauth?.tokens?.access_token + normalized[name] = { + config: connectionConfig, + hasToken: typeof accessToken === "string" && accessToken.length > 0, + } + } + return JSON.stringify(normalized) + } + private setupFileWatcher(name: string, config: Extract) { const filePath = config.args?.find((arg: string) => arg.includes("build/index.js")) if (filePath) { @@ -1194,33 +1283,42 @@ export class McpHub { // Public methods for server management public async toggleServerDisabledRPC(serverName: string, disabled: boolean): Promise { + this.isConnecting = true try { - const config = await this.readAndValidateMcpSettingsFile() - if (!config) { - throw new Error("Failed to read or validate MCP settings") - } + // Hold the cross-process lock across read-modify-write so a concurrent + // writer (CLI, OAuth handshake, another window) cannot clobber this + // toggle. Connection rebuild stays OUTSIDE the lock: connectToServer can + // trigger SDK OAuth writes that take the same (non-reentrant) lock. + const settingsPath = await getMcpSettingsFilePathHelper(await this.getSettingsDirectoryPath()) + await updateMcpSettingsFile(settingsPath, (validated) => { + const servers = validated.mcpServers as Record - if (config.mcpServers[serverName]) { - config.mcpServers[serverName].disabled = disabled - - const settingsPath = await getMcpSettingsFilePathHelper(await this.getSettingsDirectoryPath()) - await fs.writeFile(settingsPath, JSON.stringify(config, null, 2)) - - const connection = this.connections.find((conn) => conn.server.name === serverName) - if (connection) { - connection.server.disabled = disabled - // When enabling a server, set status to "connecting" so UI shows yellow indicator - if (!disabled) { - connection.server.status = "connecting" - connection.server.error = "" - } + if (!servers[serverName]) { + throw new Error(`Server "${serverName}" not found in MCP configuration`) } - const serverOrder = Object.keys(config.mcpServers || {}) - return this.getSortedMcpServers(serverOrder) - } - Logger.error(`Server "${serverName}" not found in MCP configuration`) - throw new Error(`Server "${serverName}" not found in MCP configuration`) + servers[serverName].disabled = disabled + validated.mcpServers = servers + return validated + }) + const config = await this.readPostWriteMcpSettings() + + // Rebuild the connection so the toggle takes effect. A disabled + // server's connection is a stub with no live transport/client, so the + // toggle must route through connectToServer(), which opens a real + // transport when enabled or creates a disconnected stub when disabled. + // deleteConnection preserves OAuth state. + const mcpServers = config.mcpServers as Record + const newConfig = mcpServers[serverName] + await this.deleteConnection(serverName) + await this.connectToServer(serverName, newConfig, "rpc") + + // Refresh the SDK session's tool list to reflect the server + // appearing or disappearing. + await this.notifyWebviewOfServerChanges() + + const serverOrder = Object.keys(config.mcpServers || {}) + return this.getSortedMcpServers(serverOrder) } catch (error) { Logger.error("Failed to update server disabled state:", error) if (error instanceof Error) { @@ -1231,6 +1329,8 @@ export class McpHub { message: `Failed to update server state: ${error instanceof Error ? error.message : String(error)}`, }) throw error + } finally { + this.isConnecting = false } } @@ -1378,32 +1478,30 @@ export class McpHub { * @returns Array of updated MCP servers */ async toggleToolAutoApproveRPC(serverName: string, toolNames: string[], shouldAllow: boolean): Promise { - // Set flag to prevent file watcher from triggering during our update - this.isUpdatingClineSettings = true try { const settingsPath = await getMcpSettingsFilePathHelper(await this.getSettingsDirectoryPath()) - const content = await fs.readFile(settingsPath, "utf-8") - const config = JSON.parse(content) - - // Initialize autoApprove if it doesn't exist - if (!config.mcpServers[serverName].autoApprove) { - config.mcpServers[serverName].autoApprove = [] - } - - const autoApprove = config.mcpServers[serverName].autoApprove - for (const toolName of toolNames) { - const toolIndex = autoApprove.indexOf(toolName) - - if (shouldAllow && toolIndex === -1) { - // Add tool to autoApprove list - autoApprove.push(toolName) - } else if (!shouldAllow && toolIndex !== -1) { - // Remove tool from autoApprove list - autoApprove.splice(toolIndex, 1) + const { config, autoApprove } = await updateMcpSettingsFile(settingsPath, (parsed) => { + // Initialize autoApprove if it doesn't exist + const servers = parsed.mcpServers as Record + if (!servers[serverName].autoApprove) { + servers[serverName].autoApprove = [] } - } - await fs.writeFile(settingsPath, JSON.stringify(config, null, 2)) + const approve = servers[serverName].autoApprove + for (const toolName of toolNames) { + const toolIndex = approve.indexOf(toolName) + + if (shouldAllow && toolIndex === -1) { + // Add tool to autoApprove list + approve.push(toolName) + } else if (!shouldAllow && toolIndex !== -1) { + // Remove tool from autoApprove list + approve.splice(toolIndex, 1) + } + } + return { config: parsed, autoApprove: approve } + }) + this.recordSettingsFingerprint(config.mcpServers as Record) // Update the tools list to reflect the change const connection = this.connections.find((conn) => conn.server.name === serverName) @@ -1421,42 +1519,34 @@ export class McpHub { } catch (error) { Logger.error("Failed to update autoApprove settings:", error) throw error // Re-throw to ensure the error is properly handled - } finally { - // Clear flag after a delay to ensure file watcher event has been processed - // The file watcher has a 100ms stabilityThreshold, so we wait a bit longer - setTimeout(() => { - this.isUpdatingClineSettings = false - }, 300) } } async toggleToolAutoApprove(serverName: string, toolNames: string[], shouldAllow: boolean): Promise { - // Set flag to prevent file watcher from triggering during our update - this.isUpdatingClineSettings = true try { const settingsPath = await getMcpSettingsFilePathHelper(await this.getSettingsDirectoryPath()) - const content = await fs.readFile(settingsPath, "utf-8") - const config = JSON.parse(content) - - // Initialize autoApprove if it doesn't exist - if (!config.mcpServers[serverName].autoApprove) { - config.mcpServers[serverName].autoApprove = [] - } - - const autoApprove = config.mcpServers[serverName].autoApprove - for (const toolName of toolNames) { - const toolIndex = autoApprove.indexOf(toolName) - - if (shouldAllow && toolIndex === -1) { - // Add tool to autoApprove list - autoApprove.push(toolName) - } else if (!shouldAllow && toolIndex !== -1) { - // Remove tool from autoApprove list - autoApprove.splice(toolIndex, 1) + const { autoApprove, mcpServers } = await updateMcpSettingsFile(settingsPath, (config) => { + // Initialize autoApprove if it doesn't exist + const servers = config.mcpServers as Record + if (!servers[serverName].autoApprove) { + servers[serverName].autoApprove = [] } - } - await fs.writeFile(settingsPath, JSON.stringify(config, null, 2)) + const approve = servers[serverName].autoApprove + for (const toolName of toolNames) { + const toolIndex = approve.indexOf(toolName) + + if (shouldAllow && toolIndex === -1) { + // Add tool to autoApprove list + approve.push(toolName) + } else if (!shouldAllow && toolIndex !== -1) { + // Remove tool from autoApprove list + approve.splice(toolIndex, 1) + } + } + return { autoApprove: approve as string[], mcpServers: servers as Record } + }) + this.recordSettingsFingerprint(mcpServers) // Update the tools list to reflect the change const connection = this.connections.find((conn) => conn.server.name === serverName) @@ -1475,62 +1565,47 @@ export class McpHub { message: "Failed to update autoApprove settings", }) throw error // Re-throw to ensure the error is properly handled - } finally { - // Clear flag after a delay to ensure file watcher event has been processed - setTimeout(() => { - this.isUpdatingClineSettings = false - }, 300) } } public async addRemoteServer(serverName: string, serverUrl: string, transportType = "streamableHttp"): Promise { - // Set flag to prevent file watcher from triggering during our update - this.isUpdatingClineSettings = true try { - const settings = await this.readAndValidateMcpSettingsFile() - if (!settings) { - throw new Error("Failed to read MCP settings") - } - - if (settings.mcpServers[serverName]) { - throw new Error(`An MCP server with the name "${serverName}" already exists`) - } - - const serverConfig = { - url: serverUrl, - type: transportType, - disabled: false, - autoApprove: [], - } - - // Expand environment variables for validation - const expandedConfig = expandEnvironmentVariables(serverConfig) - - const urlValidation = z.string().url().safeParse(expandedConfig.url) - if (!urlValidation.success) { - throw new Error(`Invalid server URL: ${expandedConfig.url}. Please provide a valid URL.`) - } - - const parsedConfig = ServerConfigSchema.parse(expandedConfig) - - settings.mcpServers[serverName] = parsedConfig const settingsPath = await getMcpSettingsFilePathHelper(await this.getSettingsDirectoryPath()) + await updateMcpSettingsFile(settingsPath, (current) => { + const servers = current.mcpServers as Record + if (servers[serverName]) { + throw new Error(`An MCP server with the name "${serverName}" already exists`) + } - // We don't write the zod-transformed version to the file. - // The above parse() call adds the transportType field to the server config - // It would be fine if this was written, but we don't want to clutter up the file with internal details + const serverConfig = { + url: serverUrl, + type: transportType, + disabled: false, + autoApprove: [], + } - // ToDo: We could benefit from input / output types reflecting the non-transformed / transformed versions - await fs.writeFile( - settingsPath, - JSON.stringify( - { - mcpServers: { ...settings.mcpServers, [serverName]: serverConfig }, - }, - null, - 2, - ), - ) + // Expand environment variables for validation + const expandedConfig = expandEnvironmentVariables(serverConfig) + + const urlValidation = z.string().url().safeParse(expandedConfig.url) + if (!urlValidation.success) { + throw new Error(`Invalid server URL: ${expandedConfig.url}. Please provide a valid URL.`) + } + + const parsedConfig = ServerConfigSchema.parse(expandedConfig) + + servers[serverName] = parsedConfig + + // We don't write the zod-transformed version to the file. + // The above parse() call adds the transportType field to the server config + // It would be fine if this was written, but we don't want to clutter up the file with internal details + + // ToDo: We could benefit from input / output types reflecting the non-transformed / transformed versions + const serversToWrite = { ...servers, [serverName]: serverConfig } + current.mcpServers = serversToWrite + return current + }) + const settings = await this.readPostWriteMcpSettings() await this.updateServerConnectionsRPC(settings.mcpServers as Record) @@ -1539,11 +1614,6 @@ export class McpHub { } catch (error) { Logger.error("Failed to add remote MCP server:", error) throw error - } finally { - // Clear flag after a delay to ensure file watcher event has been processed - setTimeout(() => { - this.isUpdatingClineSettings = false - }, 300) } } @@ -1553,46 +1623,37 @@ export class McpHub { * @returns Array of remaining MCP servers */ public async deleteServerRPC(serverName: string): Promise { - // Set flag to prevent file watcher from triggering during our update - this.isUpdatingClineSettings = true try { // Clear OAuth data BEFORE removing from config (while we still have the connection/URL) await this.clearOAuthForConnection(serverName) const settingsPath = await getMcpSettingsFilePathHelper(await this.getSettingsDirectoryPath()) - const content = await fs.readFile(settingsPath, "utf-8") - const config = JSON.parse(content) - if (!config.mcpServers || typeof config.mcpServers !== "object") { - config.mcpServers = {} - } + await updateMcpSettingsFile(settingsPath, (parsed) => { + const servers = parsed.mcpServers as Record - if (config.mcpServers[serverName]) { - delete config.mcpServers[serverName] - const updatedConfig = { - mcpServers: config.mcpServers, + if (!servers[serverName]) { + throw new Error(`${serverName} not found in MCP configuration`) } - await fs.writeFile(settingsPath, JSON.stringify(updatedConfig, null, 2)) - await this.updateServerConnectionsRPC(config.mcpServers) - // Get the servers in their correct order from settings - const serverOrder = Object.keys(config.mcpServers || {}) - return this.getSortedMcpServers(serverOrder) - } - throw new Error(`${serverName} not found in MCP configuration`) + delete servers[serverName] + parsed.mcpServers = servers + return parsed + }) + const config = await this.readPostWriteMcpSettings() + const mcpServers = config.mcpServers as Record + + await this.updateServerConnectionsRPC(mcpServers) + + // Get the servers in their correct order from settings + const serverOrder = Object.keys(mcpServers || {}) + return this.getSortedMcpServers(serverOrder) } catch (error) { Logger.error(`Failed to delete MCP server: ${error instanceof Error ? error.message : String(error)}`) throw error - } finally { - // Clear flag after a delay to ensure file watcher event has been processed - setTimeout(() => { - this.isUpdatingClineSettings = false - }, 300) } } public async updateServerTimeoutRPC(serverName: string, timeout: number): Promise { - // Set flag to prevent file watcher from triggering during our update - this.isUpdatingClineSettings = true try { // Validate timeout against schema const setConfigResult = BaseConfigSchema.shape.timeout.safeParse(timeout) @@ -1601,19 +1662,22 @@ export class McpHub { } const settingsPath = await getMcpSettingsFilePathHelper(await this.getSettingsDirectoryPath()) - const content = await fs.readFile(settingsPath, "utf-8") - const config = JSON.parse(content) + await updateMcpSettingsFile(settingsPath, (parsed) => { + const servers = parsed.mcpServers as Record - if (!config.mcpServers?.[serverName]) { - throw new Error(`Server "${serverName}" not found in settings`) - } + if (!servers[serverName]) { + throw new Error(`Server "${serverName}" not found in settings`) + } - config.mcpServers[serverName] = { - ...config.mcpServers[serverName], - timeout, - } + servers[serverName] = { + ...servers[serverName], + timeout, + } - await fs.writeFile(settingsPath, JSON.stringify(config, null, 2)) + parsed.mcpServers = servers + return parsed + }) + const config = await this.readPostWriteMcpSettings() // Update in-memory config to reflect the new timeout const connection = this.connections.find((conn) => conn.server.name === serverName) @@ -1635,11 +1699,6 @@ export class McpHub { message: `Failed to update server timeout: ${error instanceof Error ? error.message : String(error)}`, }) throw error - } finally { - // Clear flag after a delay to ensure file watcher event has been processed - setTimeout(() => { - this.isUpdatingClineSettings = false - }, 300) } } @@ -1776,8 +1835,16 @@ export class McpHub { } /** - * Initiates OAuth flow for a server - * Opens browser to authorization URL + * Runs the complete OAuth flow for a server when the user clicks + * "Authenticate". + * + * The interactive flow is HTTP-based token collection (the same flow the CLI + * uses): a local loopback callback server is bound, the browser is opened to + * the authorization URL, and the code is exchanged in-process with the OAuth + * state validated against the value generated for this flow. Tokens are + * written to the shared MCP settings file, so the CLI and other windows see + * them immediately. On success the connection is restarted so the transport + * picks up the fresh tokens. */ async initiateOAuth(serverName: string): Promise { const connection = this.connections.find((conn) => conn.server.name === serverName) @@ -1785,58 +1852,35 @@ export class McpHub { throw new Error(`No connection found for server: ${serverName}`) } - // Extract serverUrl from config - const config = JSON.parse(connection.server.config) - const serverUrl = config.url - if (!serverUrl) { - throw new Error(`No URL found in config for server: ${serverName}`) - } - - // Start OAuth flow - opens the SDK-generated authorization URL in browser - await this.mcpOAuthManager.startOAuthFlow(serverName, serverUrl) - } - - /** - * Completes OAuth flow after callback - * Validates state, calls finishAuth, and reconnects - */ - async completeOAuth(serverHash: string, code: string, state: string | null): Promise { - // Find the connection by matching the server hash - const connection = this.connections.find((conn) => { - const config = JSON.parse(conn.server.config) - if (config.url) { - const hash = getServerAuthHash(conn.server.name, config.url) - return hash === serverHash - } - return false - }) - - if (!connection) { - throw new Error(`No connection found for server hash: ${serverHash}`) - } - - // Validate state for CSRF protection (if provided) - if (state && !this.mcpOAuthManager.validateAndClearState(serverHash, state)) { - throw new Error("Invalid OAuth state - possible CSRF attack") - } - - // Call finishAuth on the transport - SDK handles token exchange - // finishAuth is only available on SSE and StreamableHTTP transports - if (connection.transport instanceof SSEClientTransport || connection.transport instanceof StreamableHTTPClientTransport) { - await connection.transport.finishAuth(code) - } else { - throw new Error("OAuth is only supported for SSE and HTTP transports") - } - - Logger.log(`[McpOAuth] Authentication completed for ${connection.server.name}`) - - // Update server status - connection.server.oauthAuthStatus = "authenticated" - connection.server.oauthRequired = true + // Show "pending" in the UI while the user is off in the browser + connection.server.oauthAuthStatus = "pending" connection.server.error = "" + await this.notifyWebviewOfServerChanges() - // Restart connection to complete setup with authenticated transport - await this.restartConnection(connection.server.name) + try { + // Blocks until tokens are exchanged and written to the settings file + await this.mcpOAuthManager.startOAuthFlow(serverName) + } catch (error) { + const current = this.connections.find((conn) => conn.server.name === serverName) + if (current) { + current.server.oauthAuthStatus = "unauthenticated" + this.appendErrorMessage(current, error instanceof Error ? error.message : String(error)) + } + await this.notifyWebviewOfServerChanges() + throw error + } + + Logger.log(`[McpOAuth] Authentication completed for ${serverName}`) + + const authedConnection = this.connections.find((conn) => conn.server.name === serverName) + if (authedConnection) { + authedConnection.server.oauthAuthStatus = "authenticated" + authedConnection.server.oauthRequired = true + authedConnection.server.error = "" + } + + // Restart connection so the transport authenticates with the new tokens + await this.restartConnection(serverName) } async dispose(): Promise { diff --git a/apps/vscode/src/services/mcp/McpOAuthManager.ts b/apps/vscode/src/services/mcp/McpOAuthManager.ts index ea51651481..91eca35c46 100644 --- a/apps/vscode/src/services/mcp/McpOAuthManager.ts +++ b/apps/vscode/src/services/mcp/McpOAuthManager.ts @@ -1,325 +1,193 @@ +// MCP OAuth state is stored in the shared MCP settings file +// (~/.cline/data/settings/cline_mcp_settings.json) under each server's `oauth` +// key, in the format @cline/core (CLI, JetBrains) reads and writes: +// +// { "mcpServers": { "linear": { "transport": {...}, "oauth": { "tokens": {...}, ... } } } } +// +// This shared file is the single source of truth, which keeps the extension, +// the CLI, and multiple extension windows interoperable: +// - Writes scope to ONE server's `oauth` key via @cline/core's +// updateMcpServerOAuthStateAsync, which re-reads the file under a +// cross-process lock and replaces it atomically (temp + rename), so +// concurrent writers never clobber other servers or the whole file. Lock +// acquisition yields the extension host event loop rather than blocking it. +// - Reads come fresh from disk, so a token authorized by the CLI or another +// window is picked up without restarting. +// - The interactive authorization flow is HTTP-based token collection via +// @cline/core's authorizeMcpServerOAuth, which binds a local loopback +// callback server — the same flow the CLI uses. + +import { + authorizeMcpServerOAuth, + getMcpServerOAuthState, + type McpServerOAuthState, + updateMcpServerOAuthStateAsync, +} from "@cline/core" import { StateManager } from "@core/storage/StateManager" import { OAuthClientProvider } from "@modelcontextprotocol/sdk/client/auth.js" -import type { OAuthClientInformationFull, OAuthClientMetadata, OAuthTokens } from "@modelcontextprotocol/sdk/shared/auth.js" +import type { OAuthClientInformationMixed, OAuthClientMetadata, OAuthTokens } from "@modelcontextprotocol/sdk/shared/auth.js" import crypto from "crypto" -import { HostProvider } from "@/hosts/host-provider" +import { fetch } from "@/shared/net" import { Logger } from "@/shared/services/Logger" import { openExternal } from "@/utils/env" -import { getMcpServerCallbackPath, getServerAuthHash } from "@/utils/mcpAuth" -import { McpOAuthRedirectResolver } from "./McpOAuthRedirectResolver" -import { shouldStartNewOAuthFlow } from "./mcpOAuthFlow" +import { getServerAuthHash } from "@/utils/mcpAuth" /** - * OAuth `state` lifetime, measured from when a flow starts. - * - * Used to decide whether an in-progress flow is still fresh (so a repeated - * `redirectToAuthorization()` keeps it rather than starting a new one) and to - * expire a state during callback validation. - * - * Users typically complete OAuth within seconds/minutes of clicking - * "Authenticate"; 10 minutes leaves room to create an account, while still - * guaranteeing a stale flow eventually expires so the system makes progress. + * Fallback redirect URL advertised in client metadata for connection-time + * providers. Matches @cline/core's DEFAULT_HTTP_MCP_REDIRECT_URL — the actual + * redirect URL used during an interactive flow is chosen by + * authorizeMcpServerOAuth when it binds its local callback server. */ -const MCP_OAUTH_STATE_EXPIRY_MS = 10 * 60 * 1000 // 10 minutes +const DEFAULT_HTTP_MCP_REDIRECT_URL = "http://127.0.0.1:1456/mcp/oauth/callback" /** - * Structure for all OAuth data stored in the single mcpOAuthSecrets JSON + * Ports the local OAuth callback server may bind. The first three match the + * @cline/core defaults; extras tolerate concurrent flows from other Cline + * processes (CLI, another extension window) holding a port. */ -interface McpOAuthSecrets { - [serverHash: string]: { - tokens?: OAuthTokens - tokens_saved_at?: number - client_info?: OAuthClientInformationFull - redirect_url_at_registration?: string - code_verifier?: string - oauth_state?: string - oauth_state_timestamp?: number - pending_auth_url?: string - // PKCE verifier captured when the pending flow was created. The SDK calls - // saveCodeVerifier() with a new verifier on every connect attempt; when an - // in-progress flow is kept, this verifier is restored so it stays paired - // with the code_challenge baked into pending_auth_url. - pending_code_verifier?: string - } -} +const MCP_OAUTH_CALLBACK_PORTS = [1456, 1457, 1458, 1459, 1460, 1461] + +/** How long the interactive flow waits for the browser callback. */ +const MCP_OAUTH_FLOW_TIMEOUT_MS = 10 * 60 * 1000 // 10 minutes /** - * Helper to read OAuth secrets from storage + * Read a server's OAuth state fresh from the shared settings file. + * Never throws — a missing/unreadable file simply means "no state". */ -function getMcpOAuthSecrets(): McpOAuthSecrets { - const stateManager = StateManager.get() - const secretsJson = stateManager.getSecretKey("mcpOAuthSecrets") - if (!secretsJson) { - return {} - } +function readOAuthState(serverName: string, settingsPath: string): McpServerOAuthState { try { - return JSON.parse(secretsJson) as McpOAuthSecrets - } catch (error) { - Logger.error("[McpOAuth] Failed to parse MCP OAuth secrets:", error) + return getMcpServerOAuthState(serverName, { filePath: settingsPath }) ?? {} + } catch { return {} } } /** - * Helper to save OAuth secrets to storage + * Scoped write of one server's OAuth state. The updater runs against the + * server's current state under a cross-process lock and returns the next state; + * concurrent writers are never clobbered wholesale. Lock acquisition yields the + * event loop. Failures are logged but non-fatal, so a missing server entry + * (e.g. deleted in another window) does not crash the MCP SDK provider callbacks + * that invoke this mid-connection. */ -function saveMcpOAuthSecrets(secrets: McpOAuthSecrets): void { - const stateManager = StateManager.get() - stateManager.setSecret("mcpOAuthSecrets", JSON.stringify(secrets)) +async function patchOAuthState( + serverName: string, + settingsPath: string, + updater: (current: McpServerOAuthState) => McpServerOAuthState, +): Promise { + try { + await updateMcpServerOAuthStateAsync(serverName, updater, { filePath: settingsPath }) + } catch (error) { + Logger.warn(`[McpOAuth] Failed to persist OAuth state for ${serverName}: ${error}`) + } } /** - * Implementation of OAuthClientProvider for Cline - * Manages OAuth state and token storage for a single MCP server + * Implementation of OAuthClientProvider for connection-time auth. + * + * This provider is attached to SSE/StreamableHTTP transports so the MCP SDK + * can read tokens (and auto-refresh them with the stored refresh_token). It + * reads/writes the shared settings file in @cline/core's format. + * + * Note: `redirectToAuthorization` here is a no-op signal — connection attempts + * never open a browser. The interactive flow (Authenticate button) goes + * through McpOAuthManager.startOAuthFlow → authorizeMcpServerOAuth, which + * runs its own provider with a live local callback server. */ class ClineOAuthClientProvider implements OAuthClientProvider { - private serverName: string - private serverUrl: string - private _redirectUrl: string - private isRegistrationValid: boolean - private serverHash: string - - constructor(serverName: string, serverUrl: string) { - this.serverName = serverName - this.serverUrl = serverUrl - this.serverHash = getServerAuthHash(serverName, serverUrl) - - // Redirect URL and registration validity will be set when initialize() is called - this._redirectUrl = "" - this.isRegistrationValid = false - } - - async initialize(): Promise { - // Get the full callback URL with the MCP server-specific path, - // attempting to reuse the previously-registered port to preserve the OAuth client registration. - const callbackPath = getMcpServerCallbackPath(this.serverName, this.serverUrl) - const secrets = getMcpOAuthSecrets() - const savedRedirectUrl = secrets[this.serverHash]?.redirect_url_at_registration - - const resolution = await McpOAuthRedirectResolver.resolve( - savedRedirectUrl, - callbackPath, - HostProvider.get().getCallbackUrl, - ) - - this._redirectUrl = resolution.redirectUrl - this.isRegistrationValid = resolution.isRegistrationValid - } + constructor( + private readonly serverName: string, + private readonly settingsPath: string, + ) {} get redirectUrl(): string { - return this._redirectUrl + const state = readOAuthState(this.serverName, this.settingsPath) + return state.redirectUrl ?? DEFAULT_HTTP_MCP_REDIRECT_URL } get clientMetadata(): OAuthClientMetadata { return { - redirect_uris: [this._redirectUrl], + redirect_uris: [this.redirectUrl], token_endpoint_auth_method: "none", grant_types: ["authorization_code", "refresh_token"], response_types: ["code"], client_name: "Cline", - client_uri: "https://cline.bot", - software_id: "cline", } } state(): string { - // State is managed through storage, not instance variable - return crypto.randomBytes(32).toString("hex") + return crypto.randomUUID() } - async clientInformation(): Promise { - // If the redirect URL has changed since the client was registered - // (e.g., different port bound, platform migration), the saved client_info - // is stale — the OAuth server will reject requests with the old redirect_uri. - // Return undefined to force the SDK to re-register a new client. - if (!this.isRegistrationValid) { - const secrets = getMcpOAuthSecrets() - if (secrets[this.serverHash]?.client_info) { - Logger.log(`[McpOAuth] Discarding stale client registration for ${this.serverName} — redirect URL changed`) - // Clear the stale client_info and tokens (tokens are bound to the old client_id) - delete secrets[this.serverHash].client_info - delete secrets[this.serverHash].redirect_url_at_registration - delete secrets[this.serverHash].tokens - delete secrets[this.serverHash].tokens_saved_at - saveMcpOAuthSecrets(secrets) - } - return undefined - } - - const secrets = getMcpOAuthSecrets() - return secrets[this.serverHash]?.client_info + async clientInformation(): Promise { + const state = readOAuthState(this.serverName, this.settingsPath) + return state.clientInformation as OAuthClientInformationMixed | undefined } - async saveClientInformation(clientInformation: OAuthClientInformationFull): Promise { - const secrets = getMcpOAuthSecrets() - if (!secrets[this.serverHash]) { - secrets[this.serverHash] = {} - } - secrets[this.serverHash].client_info = clientInformation - // Track the redirect URL used for this registration so we can detect - // when it changes and force re-registration (see clientInformation()) - secrets[this.serverHash].redirect_url_at_registration = this._redirectUrl - saveMcpOAuthSecrets(secrets) - - // After a successful registration, the current redirect URL is now the registered one - this.isRegistrationValid = true + async saveClientInformation(clientInformation: OAuthClientInformationMixed): Promise { + await patchOAuthState(this.serverName, this.settingsPath, (current) => ({ + ...current, + clientInformation: clientInformation as Record, + })) } async tokens(): Promise { - // Called by the SDK to check if we have valid tokens - // This is called: - // - During connection setup (to check if auth is needed) - // - Before each request (to include Authorization header) - // - During token refresh (to get refresh_token) - // - // IMPORTANT: We should return expired tokens if they have a refresh_token - // The SDK will automatically attempt to refresh them - // Only return undefined if we have NO tokens at all - - const secrets = getMcpOAuthSecrets() - const serverData = secrets[this.serverHash] - - if (!serverData?.tokens) { - return undefined - } - - // Check token expiration if timestamp exists - if (serverData.tokens_saved_at && serverData.tokens.expires_in) { - const expiresInMs = serverData.tokens.expires_in * 1000 - - if (serverData.tokens_saved_at + expiresInMs < Date.now()) { - // Token is expired - // If we have a refresh_token, return the expired tokens so SDK can refresh - // Otherwise return undefined to trigger full re-authentication - if (serverData.tokens.refresh_token) { - Logger.log(`[McpOAuth] Token expired for ${this.serverName}, will attempt refresh`) - return serverData.tokens - } - return undefined - } - } - - return serverData.tokens + // Always read fresh from disk: tokens may have just been written by the + // CLI, another extension window, or the interactive authorize flow. + // Expired access tokens are still returned when a refresh_token exists — + // the MCP SDK refreshes them automatically and calls saveTokens(). + const state = readOAuthState(this.serverName, this.settingsPath) + return state.tokens as OAuthTokens | undefined } async saveTokens(tokens: OAuthTokens): Promise { - // Called by the SDK after successful token exchange - // Flow: finishAuth(code) → SDK's auth() → exchangeAuthorization() → THIS METHOD - // Stores tokens in the single mcpOAuthSecrets JSON for persistence + // Called by the SDK after a successful token exchange or refresh. Logger.log(`[McpOAuth] Tokens saved for ${this.serverName}`) - - const secrets = getMcpOAuthSecrets() - if (!secrets[this.serverHash]) { - secrets[this.serverHash] = {} - } - - secrets[this.serverHash].tokens = tokens - secrets[this.serverHash].tokens_saved_at = Date.now() - saveMcpOAuthSecrets(secrets) + const lastAuthenticatedAt = Date.now() + await patchOAuthState(this.serverName, this.settingsPath, (current) => ({ + ...current, + tokens: tokens as Record, + lastError: undefined, + lastAuthenticatedAt, + })) } - async redirectToAuthorization(authorizationUrl: URL): Promise { - // ======================================================================== - // IMPORTANT: This is called AUTOMATICALLY by the MCP SDK during connection - // ======================================================================== - // - // Flow: - // 1. Extension loads → McpHub.connectToServer() is called - // 2. authProvider.tokens() returns undefined (no tokens yet) - // 3. client.connect(transport) is called - // 4. SDK detects no tokens → calls auth(provider, {serverUrl}) - // 5. SDK internally calls startAuthorization() to generate OAuth URL - // 6. SDK calls THIS METHOD with the generated authorizationUrl - // 7. SDK throws UnauthorizedError (caught by McpHub to show "Authenticate" button) - // - // Our Strategy: - // - Store the auth URL and state but DON'T open browser yet - // - Wait for user to explicitly click "Authenticate" button - // - When clicked, retrieve stored URL and open browser (see startOAuthFlow()) - // - // This prevents automatic browser popups on every extension load! - // ======================================================================== - - // Guard: Check if we already have valid tokens - // If tokens exist, this method shouldn't be called (SDK would use them) - // But if it is called, don't overwrite existing auth state - const existingTokens = await this.tokens() - if (existingTokens && existingTokens.access_token) { - Logger.warn(`[McpOAuth] Preserving existing tokens for ${this.serverName}`) - return - } - - const secrets = getMcpOAuthSecrets() - if (!secrets[this.serverHash]) { - secrets[this.serverHash] = {} - } - - // The SDK calls this on every connection attempt, and a single server can be - // reconnected repeatedly (settings watcher, reconnect handler, restart). The - // authorization URL of an in-progress flow may already be open in the user's - // browser, so an in-progress, still-fresh flow is kept rather than replaced — - // otherwise the stored state would diverge from the URL the user completes and - // the callback would fail validation. - const existing = secrets[this.serverHash] - const hasInProgressFlow = Boolean(existing.oauth_state && existing.pending_auth_url) - if ( - hasInProgressFlow && - !shouldStartNewOAuthFlow({ - existingFlowStartedAt: existing.oauth_state_timestamp, - now: Date.now(), - ttlMs: MCP_OAUTH_STATE_EXPIRY_MS, - }) - ) { - // The SDK just called saveCodeVerifier() with a fresh verifier for this - // (now-discarded) attempt. Restore the verifier that pairs with the kept - // flow's pending_auth_url, otherwise token exchange would fail PKCE. - if (existing.pending_code_verifier) { - secrets[this.serverHash].code_verifier = existing.pending_code_verifier - saveMcpOAuthSecrets(secrets) - } - Logger.log(`[McpOAuth] Keeping in-progress OAuth flow for ${this.serverName}`) - return - } - - // Generate and add state parameter for CSRF protection - const state = crypto.randomBytes(32).toString("hex") - authorizationUrl.searchParams.set("state", state) - - // Save state, timestamp, and the complete auth URL for use when the user - // clicks "Authenticate". Capture the verifier the SDK just saved (it pairs - // with this URL's code_challenge) so it can be restored if a later attempt - // keeps this flow. - secrets[this.serverHash].oauth_state = state - secrets[this.serverHash].oauth_state_timestamp = Date.now() - secrets[this.serverHash].pending_auth_url = authorizationUrl.toString() - secrets[this.serverHash].pending_code_verifier = secrets[this.serverHash].code_verifier - saveMcpOAuthSecrets(secrets) - - Logger.log(`[McpOAuth] OAuth required for ${this.serverName} - user must click Authenticate button`) + async redirectToAuthorization(_authorizationUrl: URL): Promise { + // Intentionally do nothing. The SDK calls this during a connection + // attempt when the server requires auth; it then throws + // UnauthorizedError, which McpHub catches to show the "Authenticate" + // button. The actual browser flow runs in startOAuthFlow(), with a + // dedicated provider whose local callback server is actually listening. + Logger.log(`[McpOAuth] OAuth required for ${this.serverName} - user must click Authenticate`) } async saveCodeVerifier(codeVerifier: string): Promise { - // Called by SDK when starting authorization flow (PKCE) - // The verifier is used later in finishAuth() to exchange the auth code for tokens - const secrets = getMcpOAuthSecrets() - if (!secrets[this.serverHash]) { - secrets[this.serverHash] = {} - } - secrets[this.serverHash].code_verifier = codeVerifier - saveMcpOAuthSecrets(secrets) + await patchOAuthState(this.serverName, this.settingsPath, (current) => ({ + ...current, + codeVerifier, + })) } async codeVerifier(): Promise { - // Called by SDK during finishAuth() to retrieve the PKCE verifier - // Used to prove that the same client that started auth is finishing it - const secrets = getMcpOAuthSecrets() - const verifier = secrets[this.serverHash]?.code_verifier - - if (!verifier) { + const state = readOAuthState(this.serverName, this.settingsPath) + if (!state.codeVerifier) { throw new Error(`No code verifier found for ${this.serverName}`) } + return state.codeVerifier + } - return verifier + async invalidateCredentials(scope: "all" | "client" | "tokens" | "verifier"): Promise { + await patchOAuthState(this.serverName, this.settingsPath, (current) => { + if (scope === "all") { + return { lastError: current.lastError, redirectUrl: current.redirectUrl } + } + return { + ...current, + ...(scope === "client" ? { clientInformation: undefined } : {}), + ...(scope === "tokens" ? { tokens: undefined, lastAuthenticatedAt: undefined } : {}), + ...(scope === "verifier" ? { codeVerifier: undefined } : {}), + } + }) } /** @@ -329,116 +197,126 @@ class ClineOAuthClientProvider implements OAuthClientProvider { const tokens = await this.tokens() return Boolean(tokens && tokens.access_token) } - - /** - * Get the server hash for this provider - */ - getServerHash(): string { - return this.serverHash - } } /** - * Manages OAuth authentication for MCP servers - * Creates and manages OAuthClientProvider instances and handles token storage + * Manages OAuth authentication for MCP servers. + * + * Creates connection-time OAuthClientProvider instances (token reads/refresh + * writes against the shared settings file) and runs the interactive + * HTTP-callback authorization flow via @cline/core. */ export class McpOAuthManager { private providers: Map = new Map() + /** Serializes interactive flows per server so double-clicks don't race. */ + private activeFlows: Map> = new Map() + + constructor(private readonly getSettingsPath: () => Promise) {} /** - * Gets or creates an OAuthClientProvider for a server - * Note: This is async now because we need to initialize the redirect URL + * Gets or creates an OAuthClientProvider for a server. */ async getOrCreateProvider(serverName: string, serverUrl: string): Promise { const key = `${serverName}:${serverUrl}` - if (this.providers.has(key)) { - return this.providers.get(key)! + const existing = this.providers.get(key) + if (existing) { + return existing } - - // Create provider - const provider = new ClineOAuthClientProvider(serverName, serverUrl) - await provider.initialize() // Sets the redirect URL + // Import tokens from the legacy `mcpOAuthSecrets` store into the shared + // settings file before the first read, if any are present. + await this.migrateLegacySecrets(serverName, serverUrl) + const provider = new ClineOAuthClientProvider(serverName, await this.getSettingsPath()) this.providers.set(key, provider) return provider } /** - * Validates and clears stored OAuth state using hash-based lookup + * Runs the interactive OAuth flow when the user clicks "Authenticate". + * + * Delegates to @cline/core's authorizeMcpServerOAuth (the exact flow the + * CLI uses): binds a local loopback callback server, performs discovery and + * client registration, opens the browser, validates the returned state + * in-process, exchanges the code, and writes tokens to the shared settings + * file. Resolves when tokens are saved (or rejects on timeout/denial). */ - validateAndClearState(serverHash: string, state: string): boolean { - const secrets = getMcpOAuthSecrets() - const serverData = secrets[serverHash] - - if (!serverData?.oauth_state) { - Logger.error(`No stored state found for server hash: ${serverHash}`) - return false + async startOAuthFlow(serverName: string): Promise { + const inFlight = this.activeFlows.get(serverName) + if (inFlight) { + Logger.log(`[McpOAuth] OAuth flow already in progress for ${serverName}`) + return inFlight } - // Check if state has expired - if (serverData.oauth_state_timestamp) { - if (Date.now() - serverData.oauth_state_timestamp > MCP_OAUTH_STATE_EXPIRY_MS) { - Logger.error(`OAuth state expired for server hash: ${serverHash}`) - // Clear expired state - delete serverData.oauth_state - delete serverData.oauth_state_timestamp - saveMcpOAuthSecrets(secrets) - return false - } - } + const flow = (async () => { + const settingsPath = await this.getSettingsPath() + const result = await authorizeMcpServerOAuth({ + serverName, + filePath: settingsPath, + clientName: "Cline", + fetch, + openUrl: (url) => openExternal(url), + callbackPorts: MCP_OAUTH_CALLBACK_PORTS, + timeoutMs: MCP_OAUTH_FLOW_TIMEOUT_MS, + }) + Logger.log(`[McpOAuth] ${result.message}`) + })() - // Validate state matches - const isValid = serverData.oauth_state === state - - // Clear state after validation - delete serverData.oauth_state - delete serverData.oauth_state_timestamp - saveMcpOAuthSecrets(secrets) - - return isValid - } - - /** - * Opens the browser to the stored OAuth URL when user clicks "Authenticate" - * - * This retrieves the authorization URL that was stored by redirectToAuthorization() - * when the SDK auto-detected that OAuth was needed during connection. - * - * Flow: - * 1. User sees "Authenticate" button (because UnauthorizedError was caught) - * 2. User clicks button → UI calls authenticateMcpServer RPC - * 3. Controller calls mcpHub.initiateOAuth() - * 4. McpHub calls THIS METHOD - * 5. We retrieve the stored auth URL (generated by SDK, includes state) - * 6. We open browser to that URL - * 7. User authorizes → callback with code → completeOAuth() - */ - async startOAuthFlow(serverName: string, serverUrl: string): Promise { - const serverHash = getServerAuthHash(serverName, serverUrl) - const secrets = getMcpOAuthSecrets() - const storedAuthUrl = secrets[serverHash]?.pending_auth_url - - if (storedAuthUrl) { - // Use the URL that the SDK generated (with state already added in redirectToAuthorization) - await openExternal(storedAuthUrl) - } else { - // Fallback: if no stored URL, the SDK hasn't been triggered yet - // This could happen if the server was just added but connection hasn't been attempted - throw new Error(`No pending authorization URL found for ${serverName}. Please try restarting the server first.`) + this.activeFlows.set(serverName, flow) + try { + await flow + } finally { + this.activeFlows.delete(serverName) } } /** - * Clears all OAuth data for a server (used when server is deleted) + * Clears all OAuth data for a server (used when server is deleted). + * Tokens live in the server's own settings entry, so deleting the entry + * removes them; this also drops the cached provider and proactively + * clears the oauth block in case the entry itself is kept. */ async clearServerAuth(serverName: string, serverUrl: string): Promise { - const key = `${serverName}:${serverUrl}` - const serverHash = getServerAuthHash(serverName, serverUrl) + this.providers.delete(`${serverName}:${serverUrl}`) + await patchOAuthState(serverName, await this.getSettingsPath(), () => ({})) + } - this.providers.delete(key) + /** + * One-time migration of tokens from the legacy `mcpOAuthSecrets` secrets + * blob into the shared settings file. Runs per server at connection time; + * file-based state always wins (never overwrite newer shared state). + */ + private async migrateLegacySecrets(serverName: string, serverUrl: string): Promise { + try { + const secretsJson = StateManager.get().getSecretKey("mcpOAuthSecrets") + if (!secretsJson) { + return + } + const secrets = JSON.parse(secretsJson) as Record< + string, + { tokens?: OAuthTokens; tokens_saved_at?: number; client_info?: Record } + > + const serverHash = getServerAuthHash(serverName, serverUrl) + const legacy = secrets[serverHash] + if (!legacy?.tokens?.access_token) { + return + } - // Clear all OAuth-related data for this server - const secrets = getMcpOAuthSecrets() - delete secrets[serverHash] - saveMcpOAuthSecrets(secrets) + const settingsPath = await this.getSettingsPath() + const current = readOAuthState(serverName, settingsPath) + if (!current.tokens) { + Logger.log(`[McpOAuth] Migrating legacy OAuth tokens for ${serverName} to shared settings file`) + await patchOAuthState(serverName, settingsPath, (state) => ({ + ...state, + tokens: legacy.tokens as unknown as Record, + clientInformation: state.clientInformation ?? legacy.client_info, + lastAuthenticatedAt: legacy.tokens_saved_at ?? Date.now(), + })) + } + + // Drop the migrated entry so this only happens once per server. + delete secrets[serverHash] + StateManager.get().setSecret("mcpOAuthSecrets", Object.keys(secrets).length ? JSON.stringify(secrets) : undefined) + } catch (error) { + Logger.warn(`[McpOAuth] Legacy OAuth migration failed for ${serverName}: ${error}`) + } } } diff --git a/apps/vscode/src/services/mcp/McpOAuthRedirectResolver.ts b/apps/vscode/src/services/mcp/McpOAuthRedirectResolver.ts deleted file mode 100644 index 45221786f7..0000000000 --- a/apps/vscode/src/services/mcp/McpOAuthRedirectResolver.ts +++ /dev/null @@ -1,129 +0,0 @@ -import { Logger } from "@/shared/services/Logger" - -/** - * Result of resolving an OAuth redirect URL for an MCP server. - */ -export interface RedirectUrlResolution { - /** The resolved redirect URL to use for OAuth */ - redirectUrl: string - /** Whether the previously saved client registration can be reused */ - isRegistrationValid: boolean -} - -/** - * Function type for obtaining a callback URL. - * @param path - The callback path (e.g., /mcp-auth/callback/{hash}) - * @param preferredPort - Optional port to try binding first (ignored by non-loopback providers like VSCode desktop) - */ -export type GetCallbackUrlFn = (path: string, preferredPort?: number) => Promise - -/** - * Pure logic for MCP OAuth redirect URL resolution. - * - * Solves the problem where a dynamically-registered OAuth client_id becomes - * stale when the local callback server port changes between sessions. - * OAuth servers (like Linear) reject authorization requests where the - * redirect_uri doesn't match the URI registered for the client_id. - * - * Strategy: - * 1. If we have a saved redirect URL from a previous registration, extract the port - * 2. Ask the callback URL provider to try that port first - * 3. If we get the same URL back → existing registration is valid - * 4. If port was unavailable or URL differs → force re-registration - * - * This handles all combinations: - * - Standalone/JetBrains/CLI (http://127.0.0.1:{port}/...) — dynamic port, can become stale - * - VSCode Desktop (vscode://extension-id/...) — stable, no port - * - VSCode Web (https://codespace.github.dev/...) — stable per-codespace - * - Legacy state (no saved redirect URL) — conservative: assume stale - * - Cross-platform migration (VSCode → JetBrains) — detect scheme change - */ -export class McpOAuthRedirectResolver { - /** - * Extract the port number from an http://127.0.0.1:{port}/... URL. - * Returns undefined for non-loopback URLs (vscode://, https://, etc.) - * or URLs that don't match the expected loopback pattern. - */ - static extractLoopbackPort(url: string): number | undefined { - if (!McpOAuthRedirectResolver.isLoopbackUrl(url)) { - return undefined - } - - try { - const parsed = new URL(url) - const port = Number.parseInt(parsed.port, 10) - return Number.isNaN(port) || port <= 0 || port > 65535 ? undefined : port - } catch { - return undefined - } - } - - /** - * Determines if a redirect URL is an http://127.0.0.1 loopback URL - * (i.e., the type that uses dynamic ports and can become stale). - */ - static isLoopbackUrl(url: string): boolean { - try { - const parsed = new URL(url) - return parsed.protocol === "http:" && parsed.hostname === "127.0.0.1" - } catch { - return false - } - } - - /** - * Determines if two redirect URLs are compatible for OAuth client reuse. - * - * Rules: - * - If savedUrl is undefined (legacy state, no tracking), return false (conservative: - * we don't know what URL was registered, so we force re-registration to be safe) - * - If both are identical strings → compatible - * - Otherwise → incompatible (different port, different scheme, different platform) - */ - static isRedirectCompatible(savedRedirectUrl: string | undefined, currentRedirectUrl: string): boolean { - if (savedRedirectUrl === undefined) { - return false - } - return savedRedirectUrl === currentRedirectUrl - } - - /** - * Resolves the redirect URL, attempting to preserve existing client registrations. - * - * For loopback URLs (standalone/JetBrains/CLI): extracts the previously-used port - * and asks the callback URL provider to try binding it first. If the same URL is - * obtained, the existing registration remains valid. - * - * For scheme URLs (VSCode desktop): no port to prefer, returns URL directly. - * - * For legacy state (no saved URL): gets a fresh URL, marks registration as invalid - * so the SDK will re-register with the new redirect_uri. - * - * @param savedRedirectUrl - The redirect URL from a previous registration (may be undefined for legacy state) - * @param callbackPath - The OAuth callback path (e.g., /mcp-auth/callback/{hash}) - * @param getCallbackUrl - Function to get a callback URL, optionally with a preferred port - */ - static async resolve( - savedRedirectUrl: string | undefined, - callbackPath: string, - getCallbackUrl: GetCallbackUrlFn, - ): Promise { - // Determine if we have a preferred port to try - const preferredPort = - savedRedirectUrl !== undefined ? McpOAuthRedirectResolver.extractLoopbackPort(savedRedirectUrl) : undefined - - // Get the callback URL, passing the preferred port if we have one - const redirectUrl = await getCallbackUrl(callbackPath, preferredPort) - - // Check if the resolved URL matches the saved one - const isRegistrationValid = McpOAuthRedirectResolver.isRedirectCompatible(savedRedirectUrl, redirectUrl) - - if (savedRedirectUrl !== undefined && !isRegistrationValid) { - Logger.log( - `[McpOAuthRedirectResolver] Redirect URL changed: saved="${savedRedirectUrl}" current="${redirectUrl}" — client re-registration required`, - ) - } - - return { redirectUrl, isRegistrationValid } - } -} diff --git a/apps/vscode/src/services/mcp/__tests__/McpHub.deleteServerRPC.test.ts b/apps/vscode/src/services/mcp/__tests__/McpHub.deleteServerRPC.test.ts index ea5c103c2c..b9f1b1dd8c 100644 --- a/apps/vscode/src/services/mcp/__tests__/McpHub.deleteServerRPC.test.ts +++ b/apps/vscode/src/services/mcp/__tests__/McpHub.deleteServerRPC.test.ts @@ -25,6 +25,24 @@ mock.module("@/core/storage/disk", diskMock) mock.module("fs/promises", fsPromisesMock) mock.module("node:fs/promises", fsPromisesMock) +// The settings write goes through settingsLock.ts, which writes the file with +// synchronous `node:fs` (temp file + rename), not `fs/promises`. Wrap +// writeFileSync/renameSync at the module level so a test can observe that the +// real settings path is only ever produced by an atomic rename — never written +// in place. Both default to the real implementation so the lock and the write +// still hit disk. +const actualNodeFs = await import("node:fs") +// Capture the genuine sync writers as values BEFORE mock.module overrides +// `node:fs`, so the pass-through spies do not recurse into themselves. +const realWriteFileSync = actualNodeFs.writeFileSync +const realRenameSync = actualNodeFs.renameSync +const writeFileSyncSpy: sinon.SinonStub = sinon.stub() +const renameSyncSpy: sinon.SinonStub = sinon.stub() +const nodeFsNamespace = { ...actualNodeFs, writeFileSync: writeFileSyncSpy, renameSync: renameSyncSpy } +const nodeFsMock = () => ({ ...nodeFsNamespace, default: nodeFsNamespace }) +mock.module("fs", nodeFsMock) +mock.module("node:fs", nodeFsMock) + import { McpHub } from "../McpHub" // Regression tests for McpHub.deleteServerRPC(): deleting one server must not @@ -71,10 +89,16 @@ describe("McpHub.deleteServerRPC", () => { // it to observe behavior. writeFileStub.reset() writeFileStub.callsFake((...args: unknown[]) => (realWriteFile as (...a: unknown[]) => Promise)(...args)) + // node:fs writeFileSync/renameSync default to the real implementation so the + // settings lock and atomic write still hit disk; individual tests wrap them + // to observe the write path. + writeFileSyncSpy.reset() + writeFileSyncSpy.callsFake((...args: unknown[]) => (realWriteFileSync as (...a: unknown[]) => void)(...args)) + renameSyncSpy.reset() + renameSyncSpy.callsFake((...args: unknown[]) => (realRenameSync as (...a: unknown[]) => void)(...args)) hub = Object.create(McpHub.prototype) as McpHub ;(hub as any).getSettingsDirectoryPath = async () => tempDir - ;(hub as any).isUpdatingClineSettings = false ;(hub as any).connections = [makeConnection("alpha"), makeConnection("beta")] // clearOAuthForConnection touches the OAuth manager; stub it out. sandbox.stub(hub as any, "clearOAuthForConnection").resolves() @@ -116,32 +140,56 @@ describe("McpHub.deleteServerRPC", () => { Object.keys(persisted.mcpServers).should.deepEqual(["beta"]) }) - it("guards the write with isUpdatingClineSettings so the watcher skips its own event", async () => { - const clock = sandbox.useFakeTimers() + it("writes atomically (temp file + rename), never truncating the real file", async () => { await writeSettings({ alpha: { type: "stdio", command: "a" }, beta: { type: "stdio", command: "b" } }) - // Capture the flag at the moment the settings file is written. Wrap the - // module-level writeFile stub (mock.module) rather than sinon-stubbing the - // ESM `fs/promises` namespace, which bun forbids. - let flagDuringWrite: boolean | undefined - writeFileStub.callsFake((...args: unknown[]) => { - flagDuringWrite = (hub as any).isUpdatingClineSettings - return (realWriteFile as (...a: unknown[]) => Promise)(...args) + // A reader must never observe the real settings file in a truncated or + // empty state, since a client that reads it mid-write could conclude + // there are no servers. settingsLock.ts writes synchronously via node:fs: + // it writes the new contents to a temp file, then renames that temp file + // onto the real settings path. So writeFileSync must only ever target a + // path other than the settings file, and the real path only changes via + // renameSync. Wrap the module-level node:fs spies (mock.module) — the + // write goes through node:fs, not fs/promises. + const writeSyncTargets: string[] = [] + writeFileSyncSpy.callsFake((...args: unknown[]) => { + writeSyncTargets.push(String(args[0])) + return (realWriteFileSync as (...a: unknown[]) => void)(...args) + }) + const renameTargets: string[] = [] + renameSyncSpy.callsFake((...args: unknown[]) => { + renameTargets.push(String(args[1])) + return (realRenameSync as (...a: unknown[]) => void)(...args) }) await hub.deleteServerRPC("alpha") - // True during the write and still true immediately after (cleared on a timer). - flagDuringWrite!.should.be.true() - ;(hub as any).isUpdatingClineSettings.should.be.true() - - // The flag is cleared on a 300ms timer so external edits resume. - clock.tick(300) - ;(hub as any).isUpdatingClineSettings.should.be.false() + // Some writeFileSync call produced the new settings, but never in place. + writeSyncTargets.length.should.be.greaterThan(0) + writeSyncTargets.every((target) => target !== settingsPath).should.be.true() + // The real settings path only ever appears as a rename destination. + renameTargets.includes(settingsPath).should.be.true() + // The final file is complete and correct. + const persisted = JSON.parse(await fs.readFile(settingsPath, "utf-8")) + Object.keys(persisted.mcpServers).should.deepEqual(["beta"]) }) - it("throws and still clears the guard when the server is not found", async () => { - const clock = sandbox.useFakeTimers() + it("pre-seeds the connection fingerprint so the watcher skips its own write", async () => { + await writeSettings({ alpha: { type: "stdio", command: "a" }, beta: { type: "stdio", command: "b" } }) + + await hub.deleteServerRPC("alpha") + + // After the write, lastConnectionFingerprint reflects the just-written, + // schema-validated content, so the watcher's "change" event for our own + // write is a no-op. Read the file back through the same validating reader + // the implementation uses so the expected fingerprint includes the schema + // defaults (autoApprove, timeout) the impl seeds. + const validated = await (hub as any).readAndValidateMcpSettingsFile() + const expected = (hub as any).computeConnectionFingerprint(validated.mcpServers) + ;(hub as any).lastConnectionFingerprint.should.equal(expected) + }) + + it("throws when the server is not found", async () => { await writeSettings({ beta: { type: "stdio", command: "b" } }) let threw: Error | undefined @@ -152,8 +200,5 @@ describe("McpHub.deleteServerRPC", () => { } ;(threw === undefined).should.be.false() threw!.message.should.match(/not found in MCP configuration/) - - clock.tick(300) - ;(hub as any).isUpdatingClineSettings.should.be.false() }) }) diff --git a/apps/vscode/src/services/mcp/__tests__/McpHub.toggleServerDisabledRPC.test.ts b/apps/vscode/src/services/mcp/__tests__/McpHub.toggleServerDisabledRPC.test.ts new file mode 100644 index 0000000000..a4d5d1aee4 --- /dev/null +++ b/apps/vscode/src/services/mcp/__tests__/McpHub.toggleServerDisabledRPC.test.ts @@ -0,0 +1,140 @@ +import { afterEach, beforeEach, describe, it } from "mocha" +import "should" +import * as diskModule from "@core/storage/disk" +import fs from "fs/promises" +import os from "os" +import path from "path" +import sinon from "sinon" +import { HostProvider } from "@/hosts/host-provider" +import { McpHub } from "../McpHub" + +// Tests for McpHub.toggleServerDisabledRPC(): toggling a server off then on +// must rebuild the connection rather than only flip an in-memory flag, since a +// disabled server's connection is a stub with no live transport/client. Tests +// bypass the constructor's watcher via Object.create(McpHub.prototype), +// matching the sibling McpHub tests. + +type FakeConnection = { + server: { name: string; config: string; status: string; disabled: boolean } + client: Record | null + transport: Record | null +} + +function makeConnection(name: string, disabled: boolean): FakeConnection { + return { + server: { + name, + config: JSON.stringify({ type: "stdio", command: "test", timeout: 60, disabled }), + status: disabled ? "disconnected" : "connected", + disabled, + }, + client: disabled ? null : {}, + transport: disabled ? null : {}, + } +} + +describe("McpHub.toggleServerDisabledRPC", () => { + let sandbox: sinon.SinonSandbox + let tempDir: string + let settingsPath: string + let hub: McpHub + let connectArgs: Array<{ name: string; disabled: boolean }> + let notifyCount: number + + const writeSettings = async (mcpServers: Record) => { + await fs.writeFile(settingsPath, JSON.stringify({ mcpServers }, null, 2)) + } + + beforeEach(async () => { + sandbox = sinon.createSandbox() + tempDir = path.join(os.tmpdir(), `mcp-toggle-test-${Date.now()}-${Math.random().toString(36).slice(2)}`) + await fs.mkdir(tempDir, { recursive: true }) + settingsPath = path.join(tempDir, "cline_mcp_settings.json") + sandbox.stub(diskModule, "getMcpSettingsFilePath").resolves(settingsPath) + + connectArgs = [] + notifyCount = 0 + + hub = Object.create(McpHub.prototype) as McpHub + ;(hub as any).getSettingsDirectoryPath = async () => tempDir + ;(hub as any).connections = [] + ;(hub as any).isConnecting = false + // deleteConnection just drops the connection from the in-memory list. + sandbox.stub(hub as any, "deleteConnection").callsFake(async (...args: unknown[]) => { + const name = args[0] as string + ;(hub as any).connections = (hub as any).connections.filter((c: FakeConnection) => c.server.name !== name) + }) + // connectToServer records the (name, disabled) it was asked to build and + // pushes a connection matching what the real method would create. + sandbox.stub(hub as any, "connectToServer").callsFake(async (...args: unknown[]) => { + const name = args[0] as string + const config = args[1] as { disabled?: boolean } + connectArgs.push({ name, disabled: Boolean(config.disabled) }) + ;(hub as any).connections.push(makeConnection(name, Boolean(config.disabled))) + }) + sandbox.stub(hub as any, "notifyWebviewOfServerChanges").callsFake(async () => { + notifyCount++ + }) + // The error path calls HostProvider.window.showMessage; stub the static + // getter so it's harmless without a fully-initialized HostProvider. + sandbox.stub(HostProvider, "window").get(() => ({ showMessage: sinon.stub().resolves({}) })) + }) + + afterEach(async () => { + sandbox.restore() + try { + await fs.rm(tempDir, { recursive: true, force: true }) + } catch { + // Ignore cleanup errors + } + }) + + it("persists disabled=true and rebuilds the connection when disabling", async () => { + await writeSettings({ alpha: { type: "stdio", command: "a" } }) + ;(hub as any).connections = [makeConnection("alpha", false)] + + await hub.toggleServerDisabledRPC("alpha", true) + + const persisted = JSON.parse(await fs.readFile(settingsPath, "utf-8")) + persisted.mcpServers.alpha.disabled.should.equal(true) + // Rebuilt as a disabled connection. + connectArgs.should.deepEqual([{ name: "alpha", disabled: true }]) + notifyCount.should.be.greaterThan(0) + }) + + it("reconnects (not stuck connecting) when re-enabling a disabled server", async () => { + await writeSettings({ alpha: { type: "stdio", command: "a", disabled: true } }) + ;(hub as any).connections = [makeConnection("alpha", false)] + ;(hub as any).connections[0].server.disabled = true + ;(hub as any).connections[0].client = null + ;(hub as any).connections[0].transport = null + + const result = await hub.toggleServerDisabledRPC("alpha", false) + + // File reflects enabled. + const persisted = JSON.parse(await fs.readFile(settingsPath, "utf-8")) + persisted.mcpServers.alpha.disabled.should.equal(false) + // The connection was actually rebuilt as enabled — this is the fix: a + // real connect happens rather than just flipping the flag to "connecting". + connectArgs.should.deepEqual([{ name: "alpha", disabled: false }]) + // The returned (and in-memory) server is enabled and connected, not + // stuck on the "connecting" yellow state. + const alpha = result.find((s) => s.name === "alpha") + alpha!.disabled!.should.equal(false) + alpha!.status.should.equal("connected") + }) + + it("throws when the server is not found", async () => { + await writeSettings({ beta: { type: "stdio", command: "b" } }) + + let threw: Error | undefined + try { + await hub.toggleServerDisabledRPC("missing", true) + } catch (err) { + threw = err as Error + } + ;(threw === undefined).should.be.false() + threw!.message.should.match(/not found in MCP configuration/) + connectArgs.should.have.length(0) + }) +}) diff --git a/apps/vscode/src/services/mcp/__tests__/McpOAuthRedirectResolver.test.ts b/apps/vscode/src/services/mcp/__tests__/McpOAuthRedirectResolver.test.ts deleted file mode 100644 index c26a6dde07..0000000000 --- a/apps/vscode/src/services/mcp/__tests__/McpOAuthRedirectResolver.test.ts +++ /dev/null @@ -1,280 +0,0 @@ -import { describe, it } from "bun:test" -import "should" -import { type GetCallbackUrlFn, McpOAuthRedirectResolver } from "../McpOAuthRedirectResolver" - -describe("McpOAuthRedirectResolver", () => { - describe("extractLoopbackPort", () => { - it("should extract port from http://127.0.0.1:48801/path", () => { - const port = McpOAuthRedirectResolver.extractLoopbackPort("http://127.0.0.1:48801/mcp-auth/callback/abc123") - port!.should.equal(48801) - }) - - it("should extract port from http://127.0.0.1:48811 (no path)", () => { - const port = McpOAuthRedirectResolver.extractLoopbackPort("http://127.0.0.1:48811") - port!.should.equal(48811) - }) - - it("should return undefined for vscode:// URLs", () => { - const port = McpOAuthRedirectResolver.extractLoopbackPort("vscode://saoudrizwan.claude-dev/mcp-auth/callback/abc123") - should(port).be.undefined() - }) - - it("should return undefined for https:// URLs", () => { - const port = McpOAuthRedirectResolver.extractLoopbackPort("https://codespace-abc.github.dev/mcp-auth/callback/abc123") - should(port).be.undefined() - }) - - it("should return undefined for malformed URLs", () => { - const port = McpOAuthRedirectResolver.extractLoopbackPort("not-a-url") - should(port).be.undefined() - }) - - it("should return undefined for empty string", () => { - const port = McpOAuthRedirectResolver.extractLoopbackPort("") - should(port).be.undefined() - }) - - it("should return undefined for http://localhost (not 127.0.0.1)", () => { - const port = McpOAuthRedirectResolver.extractLoopbackPort("http://localhost:3000/callback") - should(port).be.undefined() - }) - - it("should return undefined for http://127.0.0.1 without a port", () => { - // http://127.0.0.1/path has no explicit port (defaults to 80) - // URL.port returns "" for default ports - const port = McpOAuthRedirectResolver.extractLoopbackPort("http://127.0.0.1/path") - should(port).be.undefined() - }) - }) - - describe("isLoopbackUrl", () => { - it("should return true for http://127.0.0.1:48801/...", () => { - McpOAuthRedirectResolver.isLoopbackUrl("http://127.0.0.1:48801/mcp-auth/callback/abc").should.be.true() - }) - - it("should return true for http://127.0.0.1 without port", () => { - McpOAuthRedirectResolver.isLoopbackUrl("http://127.0.0.1/path").should.be.true() - }) - - it("should return false for vscode:// URLs", () => { - McpOAuthRedirectResolver.isLoopbackUrl("vscode://saoudrizwan.claude-dev/path").should.be.false() - }) - - it("should return false for https:// URLs", () => { - McpOAuthRedirectResolver.isLoopbackUrl("https://example.com/path").should.be.false() - }) - - it("should return false for http://localhost (not 127.0.0.1)", () => { - McpOAuthRedirectResolver.isLoopbackUrl("http://localhost:3000/path").should.be.false() - }) - - it("should return false for malformed URLs", () => { - McpOAuthRedirectResolver.isLoopbackUrl("not-a-url").should.be.false() - }) - }) - - describe("isRedirectCompatible", () => { - it("should return true when URLs are identical", () => { - McpOAuthRedirectResolver.isRedirectCompatible( - "http://127.0.0.1:48801/mcp-auth/callback/abc123", - "http://127.0.0.1:48801/mcp-auth/callback/abc123", - ).should.be.true() - }) - - it("should return true for identical vscode:// URLs", () => { - McpOAuthRedirectResolver.isRedirectCompatible( - "vscode://saoudrizwan.claude-dev/mcp-auth/callback/abc123", - "vscode://saoudrizwan.claude-dev/mcp-auth/callback/abc123", - ).should.be.true() - }) - - it("should return true for identical https:// URLs", () => { - McpOAuthRedirectResolver.isRedirectCompatible( - "https://codespace-abc.github.dev/mcp-auth/callback/abc123", - "https://codespace-abc.github.dev/mcp-auth/callback/abc123", - ).should.be.true() - }) - - it("should return false when saved URL is undefined (legacy state)", () => { - McpOAuthRedirectResolver.isRedirectCompatible( - undefined, - "http://127.0.0.1:48801/mcp-auth/callback/abc123", - ).should.be.false() - }) - - it("should return false when ports differ", () => { - McpOAuthRedirectResolver.isRedirectCompatible( - "http://127.0.0.1:48801/mcp-auth/callback/abc123", - "http://127.0.0.1:48802/mcp-auth/callback/abc123", - ).should.be.false() - }) - - it("should return false when schemes differ (VSCode → JetBrains migration)", () => { - McpOAuthRedirectResolver.isRedirectCompatible( - "vscode://saoudrizwan.claude-dev/mcp-auth/callback/abc123", - "http://127.0.0.1:48801/mcp-auth/callback/abc123", - ).should.be.false() - }) - - it("should return false when schemes differ (JetBrains → VSCode migration)", () => { - McpOAuthRedirectResolver.isRedirectCompatible( - "http://127.0.0.1:48801/mcp-auth/callback/abc123", - "vscode://saoudrizwan.claude-dev/mcp-auth/callback/abc123", - ).should.be.false() - }) - - it("should return false when paths differ (different server hash)", () => { - McpOAuthRedirectResolver.isRedirectCompatible( - "http://127.0.0.1:48801/mcp-auth/callback/hash1", - "http://127.0.0.1:48801/mcp-auth/callback/hash2", - ).should.be.false() - }) - - it("should return false when codespace domains differ", () => { - McpOAuthRedirectResolver.isRedirectCompatible( - "https://codespace-old.github.dev/mcp-auth/callback/abc123", - "https://codespace-new.github.dev/mcp-auth/callback/abc123", - ).should.be.false() - }) - }) - - describe("resolve", () => { - it("should get fresh URL and mark registration invalid when no saved URL", async () => { - const getCallbackUrl: GetCallbackUrlFn = async (path, _preferredPort) => { - return `http://127.0.0.1:48801${path}` - } - - const result = await McpOAuthRedirectResolver.resolve(undefined, "/mcp-auth/callback/abc123", getCallbackUrl) - - result.redirectUrl.should.equal("http://127.0.0.1:48801/mcp-auth/callback/abc123") - result.isRegistrationValid.should.be.false() - }) - - it("should reuse port when saved loopback URL port is available", async () => { - const savedUrl = "http://127.0.0.1:48803/mcp-auth/callback/abc123" - - // Mock: the provider successfully binds the preferred port - const getCallbackUrl: GetCallbackUrlFn = async (path, preferredPort) => { - // Simulate: preferred port was available, so we got the same port back - const port = preferredPort || 48801 - return `http://127.0.0.1:${port}${path}` - } - - const result = await McpOAuthRedirectResolver.resolve(savedUrl, "/mcp-auth/callback/abc123", getCallbackUrl) - - result.redirectUrl.should.equal("http://127.0.0.1:48803/mcp-auth/callback/abc123") - result.isRegistrationValid.should.be.true() - }) - - it("should fall back to new port and mark registration invalid when preferred port is unavailable", async () => { - const savedUrl = "http://127.0.0.1:48803/mcp-auth/callback/abc123" - - // Mock: the provider cannot bind the preferred port, falls back to another - const getCallbackUrl: GetCallbackUrlFn = async (path, _preferredPort) => { - // Simulate: preferred port was occupied, fell back to 48805 - return `http://127.0.0.1:48805${path}` - } - - const result = await McpOAuthRedirectResolver.resolve(savedUrl, "/mcp-auth/callback/abc123", getCallbackUrl) - - result.redirectUrl.should.equal("http://127.0.0.1:48805/mcp-auth/callback/abc123") - result.isRegistrationValid.should.be.false() - }) - - it("should pass preferred port to getCallbackUrl for loopback URLs", async () => { - const savedUrl = "http://127.0.0.1:48807/mcp-auth/callback/abc123" - let receivedPreferredPort: number | undefined - - const getCallbackUrl: GetCallbackUrlFn = async (path, preferredPort) => { - receivedPreferredPort = preferredPort - const port = preferredPort || 48801 - return `http://127.0.0.1:${port}${path}` - } - - await McpOAuthRedirectResolver.resolve(savedUrl, "/mcp-auth/callback/abc123", getCallbackUrl) - - receivedPreferredPort!.should.equal(48807) - }) - - it("should NOT pass preferred port for non-loopback saved URLs (vscode://)", async () => { - const savedUrl = "vscode://saoudrizwan.claude-dev/mcp-auth/callback/abc123" - let receivedPreferredPort: number | undefined - - const getCallbackUrl: GetCallbackUrlFn = async (path, preferredPort) => { - receivedPreferredPort = preferredPort - return `vscode://saoudrizwan.claude-dev${path}` - } - - await McpOAuthRedirectResolver.resolve(savedUrl, "/mcp-auth/callback/abc123", getCallbackUrl) - - should(receivedPreferredPort).be.undefined() - }) - - it("should mark registration valid when VSCode URLs match", async () => { - const savedUrl = "vscode://saoudrizwan.claude-dev/mcp-auth/callback/abc123" - - const getCallbackUrl: GetCallbackUrlFn = async (path, _preferredPort) => { - return `vscode://saoudrizwan.claude-dev${path}` - } - - const result = await McpOAuthRedirectResolver.resolve(savedUrl, "/mcp-auth/callback/abc123", getCallbackUrl) - - result.redirectUrl.should.equal("vscode://saoudrizwan.claude-dev/mcp-auth/callback/abc123") - result.isRegistrationValid.should.be.true() - }) - - it("should mark registration invalid for VSCode → JetBrains cross-platform migration", async () => { - const savedUrl = "vscode://saoudrizwan.claude-dev/mcp-auth/callback/abc123" - - // Now running on JetBrains, which uses loopback - const getCallbackUrl: GetCallbackUrlFn = async (path, _preferredPort) => { - return `http://127.0.0.1:48801${path}` - } - - const result = await McpOAuthRedirectResolver.resolve(savedUrl, "/mcp-auth/callback/abc123", getCallbackUrl) - - result.redirectUrl.should.equal("http://127.0.0.1:48801/mcp-auth/callback/abc123") - result.isRegistrationValid.should.be.false() - }) - - it("should mark registration invalid for JetBrains → VSCode cross-platform migration", async () => { - const savedUrl = "http://127.0.0.1:48801/mcp-auth/callback/abc123" - - // Now running on VSCode - const getCallbackUrl: GetCallbackUrlFn = async (path, _preferredPort) => { - return `vscode://saoudrizwan.claude-dev${path}` - } - - const result = await McpOAuthRedirectResolver.resolve(savedUrl, "/mcp-auth/callback/abc123", getCallbackUrl) - - result.redirectUrl.should.equal("vscode://saoudrizwan.claude-dev/mcp-auth/callback/abc123") - result.isRegistrationValid.should.be.false() - }) - - it("should handle VSCode Web URLs (https://)", async () => { - const savedUrl = "https://codespace-abc.github.dev/mcp-auth/callback/abc123" - - const getCallbackUrl: GetCallbackUrlFn = async (path, _preferredPort) => { - return `https://codespace-abc.github.dev${path}` - } - - const result = await McpOAuthRedirectResolver.resolve(savedUrl, "/mcp-auth/callback/abc123", getCallbackUrl) - - result.redirectUrl.should.equal("https://codespace-abc.github.dev/mcp-auth/callback/abc123") - result.isRegistrationValid.should.be.true() - }) - - it("should detect codespace change as registration invalid", async () => { - const savedUrl = "https://codespace-old.github.dev/mcp-auth/callback/abc123" - - const getCallbackUrl: GetCallbackUrlFn = async (path, _preferredPort) => { - return `https://codespace-new.github.dev${path}` - } - - const result = await McpOAuthRedirectResolver.resolve(savedUrl, "/mcp-auth/callback/abc123", getCallbackUrl) - - result.redirectUrl.should.equal("https://codespace-new.github.dev/mcp-auth/callback/abc123") - result.isRegistrationValid.should.be.false() - }) - }) -}) diff --git a/apps/vscode/src/services/mcp/__tests__/mcpOAuthFlow.test.ts b/apps/vscode/src/services/mcp/__tests__/mcpOAuthFlow.test.ts deleted file mode 100644 index 14a175dc46..0000000000 --- a/apps/vscode/src/services/mcp/__tests__/mcpOAuthFlow.test.ts +++ /dev/null @@ -1,34 +0,0 @@ -import { describe, it } from "bun:test" -import "should" -import { shouldStartNewOAuthFlow } from "../mcpOAuthFlow" - -/** - * `shouldStartNewOAuthFlow` decides whether `redirectToAuthorization()` starts a - * new OAuth flow or keeps the one already in progress. A fresh in-progress flow - * is kept so its stored `state` stays consistent with the authorization URL the - * user is completing; a stale flow (older than the TTL, measured from when it - * started) is replaced. - */ -describe("shouldStartNewOAuthFlow", () => { - const TTL_MS = 10 * 60 * 1000 - - it("starts a flow when none is in progress", () => { - shouldStartNewOAuthFlow({ existingFlowStartedAt: undefined, now: 1000, ttlMs: TTL_MS }).should.be.true() - }) - - it("keeps a fresh in-progress flow instead of starting a new one", () => { - shouldStartNewOAuthFlow({ existingFlowStartedAt: 1000, now: 2000, ttlMs: TTL_MS }).should.be.false() - }) - - it("starts a fresh flow when the existing one is stale (older than the TTL)", () => { - shouldStartNewOAuthFlow({ existingFlowStartedAt: 1000, now: 1000 + TTL_MS + 1, ttlMs: TTL_MS }).should.be.true() - }) - - it("keeps the flow right up to the TTL boundary", () => { - shouldStartNewOAuthFlow({ existingFlowStartedAt: 1000, now: 1000 + TTL_MS, ttlMs: TTL_MS }).should.be.false() - }) - - it("treats a missing start timestamp as no flow in progress", () => { - shouldStartNewOAuthFlow({ existingFlowStartedAt: undefined, now: 5000, ttlMs: TTL_MS }).should.be.true() - }) -}) diff --git a/apps/vscode/src/services/mcp/__tests__/settingsLock.test.ts b/apps/vscode/src/services/mcp/__tests__/settingsLock.test.ts new file mode 100644 index 0000000000..1204a83652 --- /dev/null +++ b/apps/vscode/src/services/mcp/__tests__/settingsLock.test.ts @@ -0,0 +1,53 @@ +import { existsSync } from "node:fs" +import fs from "node:fs/promises" +import os from "node:os" +import path from "node:path" +import { afterEach, beforeEach, describe, expect, it } from "vitest" +import { updateMcpSettingsFile } from "../settingsLock" + +describe("updateMcpSettingsFile", () => { + let tempDir: string + let settingsPath: string + + beforeEach(async () => { + tempDir = path.join(os.tmpdir(), `mcp-settings-lock-test-${Date.now()}-${Math.random().toString(36).slice(2)}`) + await fs.mkdir(tempDir, { recursive: true }) + settingsPath = path.join(tempDir, "cline_mcp_settings.json") + await fs.writeFile(settingsPath, JSON.stringify({ mcpServers: {} }, null, 2)) + }) + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }) + }) + + it("does not yield while holding the settings lock", async () => { + let mutatorRan = false + + const update = updateMcpSettingsFile(settingsPath, (settings) => { + mutatorRan = true + settings.mcpServers = { + alpha: { + type: "stdio", + command: "node", + }, + } + return "updated" + }) + + expect(mutatorRan).toBe(true) + expect(existsSync(`${settingsPath}.lock`)).toBe(false) + await expect(update).resolves.toBe("updated") + }) + + it("creates a missing settings file inside the lock", async () => { + const missingPath = path.join(tempDir, "fresh", "cline_mcp_settings.json") + + await updateMcpSettingsFile(missingPath, (settings) => { + settings.mcpServers = { alpha: { type: "stdio", command: "node" } } + }) + + const written = JSON.parse(await fs.readFile(missingPath, "utf-8")) + expect(Object.keys(written.mcpServers)).toEqual(["alpha"]) + expect(existsSync(`${missingPath}.lock`)).toBe(false) + }) +}) diff --git a/apps/vscode/src/services/mcp/mcpOAuthFlow.ts b/apps/vscode/src/services/mcp/mcpOAuthFlow.ts deleted file mode 100644 index add2f35b3d..0000000000 --- a/apps/vscode/src/services/mcp/mcpOAuthFlow.ts +++ /dev/null @@ -1,39 +0,0 @@ -/** - * Pure decision logic for the MCP OAuth flow lifecycle. - * - * The MCP SDK calls `redirectToAuthorization()` on every connection attempt, and - * a single server can be (re)connected many times (settings watcher, reconnect - * handler, restart). Each new flow generates a `state` baked into an - * authorization URL; that URL may already be open in the user's browser. To keep - * the stored `state` consistent with the URL the user is completing, a new flow - * is only started when one isn't already in progress and fresh: - * - * - no flow in progress -> start one - * - a flow is in progress and FRESH -> keep it - * - the in-progress flow is STALE -> start a fresh one - * - * Freshness is measured from when the flow started and is never extended, so a - * flow always eventually expires and the system can make forward progress. - */ -export interface OAuthFlowDecisionInput { - /** Timestamp (ms) when the in-progress flow's state was generated, or undefined if none. */ - existingFlowStartedAt: number | undefined - /** Current time (ms). */ - now: number - /** How long a flow stays valid, measured from when it started. */ - ttlMs: number -} - -/** - * Returns true if `redirectToAuthorization()` should generate a new state/URL, - * or false if it should keep the existing in-progress flow untouched. - */ -export function shouldStartNewOAuthFlow({ existingFlowStartedAt, now, ttlMs }: OAuthFlowDecisionInput): boolean { - // No flow in progress (or partial state with no timestamp): start one. - if (existingFlowStartedAt === undefined) { - return true - } - // A flow is in progress: keep it while fresh, replace it once stale. - const isStale = now - existingFlowStartedAt > ttlMs - return isStale -} diff --git a/apps/vscode/src/services/mcp/settingsLock.ts b/apps/vscode/src/services/mcp/settingsLock.ts new file mode 100644 index 0000000000..51bab66c86 --- /dev/null +++ b/apps/vscode/src/services/mcp/settingsLock.ts @@ -0,0 +1,228 @@ +import { setTimeout as delay } from "node:timers/promises" +import { randomUUID } from "node:crypto" +import { + existsSync, + mkdirSync, + readFileSync, + renameSync, + rmdirSync, + rmSync, + statSync, + unlinkSync, + writeFileSync, +} from "node:fs" +import * as path from "node:path" +import { Logger } from "@/shared/services/Logger" + +const SETTINGS_LOCK_STALE_MS = 10_000 +const SETTINGS_LOCK_POLL_MS = 25 + +export interface McpSettingsUpdateOptions { + abortSignal?: AbortSignal +} + +export type McpSettingsMutator = (settings: Record) => T + +export class McpSettingsUpdateSkippedError extends Error { + constructor(message: string) { + super(message) + this.name = "McpSettingsUpdateSkippedError" + } +} + +export class McpSettingsLockAbortedError extends Error { + constructor(message: string) { + super(message) + this.name = "McpSettingsLockAbortedError" + } +} + +export class McpSettingsMutatorPurityError extends Error { + constructor(message: string) { + super(message) + this.name = "McpSettingsMutatorPurityError" + } +} + +function settingsLockDir(settingsPath: string): string { + return `${settingsPath}.lock` +} + +function makeLockToken(): string { + return `${process.pid}.${Date.now()}.${randomUUID()}` +} + +interface AcquiredSettingsLock { + lockDir: string + ownerFile: string +} + +function atomicWriteSettingsFile(settingsPath: string, contents: string): void { + const tempPath = `${settingsPath}.tmp.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}` + mkdirSync(path.dirname(settingsPath), { recursive: true }) + try { + writeFileSync(tempPath, contents, { encoding: "utf-8", flag: "wx" }) + renameSync(tempPath, settingsPath) + } catch (error) { + try { + unlinkSync(tempPath) + } catch { + // Best-effort cleanup of the temp file. + } + throw error + } +} + +/** + * Cross-process lock implemented as a populated directory. Acquisition creates a + * staging directory, writes a unique owner marker inside it, then renames the + * populated directory into place. That means the visible lock directory is never + * empty. Release only deletes our marker and then rmdir's the directory; if + * another owner replaced the lock, our marker is absent or the directory is + * non-empty, so we do not remove their lock. Stale takeover renames the whole + * lock directory aside before deleting it. This uses standard mkdir/rename/rmdir + * operations and avoids inode- or handle-based deletion, so it works with + * Node's portable fs APIs on Windows and POSIX. + */ +function tryAcquireSettingsLock(lockDir: string, token: string): AcquiredSettingsLock | undefined { + mkdirSync(path.dirname(lockDir), { recursive: true }) + const stagingDir = `${lockDir}.tmp.${token}` + rmSync(stagingDir, { recursive: true, force: true }) + mkdirSync(stagingDir, { recursive: true }) + const ownerFileName = `owner.${token}` + const stagingOwnerFile = path.join(stagingDir, ownerFileName) + writeFileSync(stagingOwnerFile, token, { encoding: "utf8", flag: "wx" }) + try { + renameSync(stagingDir, lockDir) + return { lockDir, ownerFile: path.join(lockDir, ownerFileName) } + } catch (error) { + rmSync(stagingDir, { recursive: true, force: true }) + if (existsSync(lockDir)) { + return undefined + } + throw error + } +} + +function reclaimStaleLock(lockDir: string): void { + let ageMs: number + try { + ageMs = Date.now() - statSync(lockDir).mtimeMs + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return + } + throw error + } + if (ageMs < SETTINGS_LOCK_STALE_MS) { + return + } + Logger.warn(`[mcp-settings] Stale lock directory at ${lockDir} (age ${ageMs}ms); reclaiming.`) + const staleDir = `${lockDir}.stale.${makeLockToken()}` + try { + renameSync(lockDir, staleDir) + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return + } + throw error + } + rmSync(staleDir, { recursive: true, force: true }) +} + +function releaseSettingsLock(lock: AcquiredSettingsLock): void { + try { + unlinkSync(lock.ownerFile) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error + } + } + try { + rmdirSync(lock.lockDir) + } catch (error) { + const code = (error as NodeJS.ErrnoException).code + if (code !== "ENOENT" && code !== "ENOTEMPTY" && code !== "EEXIST") { + throw error + } + } +} + +function checkAbort(signal: AbortSignal | undefined, lockDir: string): void { + if (signal?.aborted) { + throw new McpSettingsLockAbortedError(`Aborted waiting for MCP settings lock at ${lockDir}.`) + } +} + +function readSettingsObject(settingsPath: string): Record { + let settings: Record + try { + settings = JSON.parse(readFileSync(settingsPath, "utf-8")) as Record + } catch (error) { + // A missing file bootstraps to an empty object so the first locked write + // creates it. A present-but-malformed file still throws. + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error + } + settings = {} + } + if (!settings.mcpServers || typeof settings.mcpServers !== "object" || Array.isArray(settings.mcpServers)) { + settings.mcpServers = {} + } + return settings +} + +function runPureSettingsMutator(settings: Record, mutator: McpSettingsMutator): T { + const before = JSON.stringify(settings) + const shadow = JSON.parse(before) as Record + const shadowResult = mutator(shadow) + const shadowAfter = JSON.stringify(shadow) + const result = mutator(settings) + const after = JSON.stringify(settings) + if (after !== shadowAfter) { + throw new McpSettingsMutatorPurityError( + "MCP settings mutator must be deterministic and free of side effects; repeated calls produced different settings.", + ) + } + if (JSON.stringify(result) !== JSON.stringify(shadowResult)) { + throw new McpSettingsMutatorPurityError( + "MCP settings mutator must be deterministic and free of side effects; repeated calls produced different return values.", + ) + } + return result +} + +/** + * Locked MCP settings read-update-write. The mutator is synchronous and may be + * called more than once to validate purity/determinism. Do not perform slow work + * or side effects inside it; compute values before calling and close over them. + * + * Waiting for another process to release the lock is async, but once this + * process owns the lock, the critical section uses synchronous filesystem calls. + * @cline/core OAuth writes use a synchronous lock in the same extension host; + * yielding here while holding the lock would let that sync waiter block the + * event loop before this holder can resume and release it. + */ +export async function updateMcpSettingsFile( + settingsPath: string, + mutator: McpSettingsMutator, + options: McpSettingsUpdateOptions = {}, +): Promise { + const lockDir = settingsLockDir(settingsPath) + const token = makeLockToken() + let lock: AcquiredSettingsLock | undefined + while (!(lock = tryAcquireSettingsLock(lockDir, token))) { + checkAbort(options.abortSignal, lockDir) + reclaimStaleLock(lockDir) + await delay(SETTINGS_LOCK_POLL_MS) + } + try { + checkAbort(options.abortSignal, lockDir) + const settings = readSettingsObject(settingsPath) + const result = runPureSettingsMutator(settings, mutator) + atomicWriteSettingsFile(settingsPath, `${JSON.stringify(settings, null, 2)}\n`) + return result + } finally { + releaseSettingsLock(lock) + } +} diff --git a/apps/vscode/src/services/uri/SharedUriHandler.ts b/apps/vscode/src/services/uri/SharedUriHandler.ts index 9dcbcaf2f6..c748745602 100644 --- a/apps/vscode/src/services/uri/SharedUriHandler.ts +++ b/apps/vscode/src/services/uri/SharedUriHandler.ts @@ -120,20 +120,6 @@ export class SharedUriHandler { await visibleWebview.controller.handleTaskCreation(prompt) return true } - // Match /mcp-auth/callback/{hash} - case path.match(/^\/mcp-auth\/callback\/[^/]+$/)?.input: { - const serverHash = path.split("/").pop() - const code = query.get("code") - const state = query.get("state") - - if (!code || !serverHash) { - Logger.warn("SharedUriHandler: Missing code or hash in MCP OAuth callback") - return false - } - - await visibleWebview.controller.handleMcpOAuthCallback(serverHash, code, state) - return true - } case "/hicap": { const code = query.get("code") if (code) { diff --git a/apps/vscode/src/shared/storage/__tests__/state-keys.test.ts b/apps/vscode/src/shared/storage/__tests__/state-keys.test.ts index bfcda0d059..5f763b8877 100644 --- a/apps/vscode/src/shared/storage/__tests__/state-keys.test.ts +++ b/apps/vscode/src/shared/storage/__tests__/state-keys.test.ts @@ -31,7 +31,7 @@ * ## Running Tests * * ```bash - * npm run test:unit -- --grep "State Keys" + * bun run test:unit -- --grep "State Keys" * ``` */ diff --git a/apps/vscode/src/test/e2e/README.md b/apps/vscode/src/test/e2e/README.md index a6f03f2166..6ec29a7df8 100644 --- a/apps/vscode/src/test/e2e/README.md +++ b/apps/vscode/src/test/e2e/README.md @@ -37,13 +37,13 @@ The E2E test suite consists of several key components: To build the test environment and run all E2E tests: ```bash -npm run test:e2e +bun run test:e2e ``` To run all E2E tests without re-building the test environment (e.g. only test files were updated): ```bash -npm run e2e +bun run e2e ``` ### Debug Mode @@ -51,9 +51,9 @@ npm run e2e To run E2E tests in debug mode with Playwright's interactive debugger: ```bash -npm run test:e2e -- --debug +bun run test:e2e -- --debug # Or only run the tests without re-building -npm run e2e -- --debug +bun run e2e -- --debug ``` In debug mode, Playwright will: @@ -66,17 +66,17 @@ In debug mode, Playwright will: Run specific test files: ```bash -npm run e2e -- auth.test.ts +bun run e2e -- auth.test.ts ``` Run tests with specific tags or patterns: ```bash -npm run e2e -- --grep "Chat" +bun run e2e -- --grep "Chat" ``` Run tests in headed mode (visible browser): ```bash -npm run e2e -- --headed +bun run e2e -- --headed ``` ## Writing Tests @@ -165,7 +165,7 @@ The `--debug` flag enables Playwright's interactive debugging features: 1. **Start debugging session:** ```bash - npm run test:e2e -- --debug + bun run test:e2e -- --debug ``` 2. **Playwright will open:** diff --git a/apps/vscode/src/utils/mcpAuth.ts b/apps/vscode/src/utils/mcpAuth.ts index 842043ca3f..529764a28c 100644 --- a/apps/vscode/src/utils/mcpAuth.ts +++ b/apps/vscode/src/utils/mcpAuth.ts @@ -2,7 +2,9 @@ import crypto from "crypto" /** * Generates a unique hash for an MCP server based on its name and URL. - * Used for creating unique OAuth callback paths and storage keys. + * Used as the storage key in the legacy `mcpOAuthSecrets` blob; retained only + * for the one-time migration of legacy tokens into the shared settings file + * (see McpOAuthManager.migrateLegacySecrets). * @param serverName The name of the MCP server. * @param serverUrl The URL of the MCP server. * @returns A SHA-256 hash string. @@ -10,14 +12,3 @@ import crypto from "crypto" export const getServerAuthHash = (serverName: string, serverUrl: string): string => { return crypto.createHash("sha256").update(`${serverName}:${serverUrl}`).digest("hex") } - -/** - * Generates the unique OAuth callback path for a specific MCP server. - * @param serverName The name of the MCP server. - * @param serverUrl The URL of the MCP server. - * @returns The callback path string (e.g., /mcp-auth/callback/). - */ -export const getMcpServerCallbackPath = (serverName: string, serverUrl: string): string => { - const hash = getServerAuthHash(serverName, serverUrl) - return `/mcp-auth/callback/${hash}` -} diff --git a/apps/vscode/tsconfig.test.json b/apps/vscode/tsconfig.test.json index 13a5d726dc..18f1b98bf2 100644 --- a/apps/vscode/tsconfig.test.json +++ b/apps/vscode/tsconfig.test.json @@ -86,7 +86,7 @@ // the single source of truth for the bun-vs-integration split; this static // list only covers structural/vitest excludes. "src/test/e2e/**/*.test.ts", - // The src/sdk suites are vitest-native (run via `npm run test:vitest`), + // The src/sdk suites are vitest-native (run via `bun run test:vitest`), // not the VS Code/mocha integration runner (see .vscode-test.mjs, which // only globs core/test/utils/shared/integrations/hosts/services). Some of // them use top-level `await import(...)` after `vi.mock(...)`, which is diff --git a/apps/vscode/vitest.config.ts b/apps/vscode/vitest.config.ts index 943da2ce12..0cc0d8757e 100644 --- a/apps/vscode/vitest.config.ts +++ b/apps/vscode/vitest.config.ts @@ -9,7 +9,9 @@ export default defineConfig({ include: [ "src/sdk/**/*.test.ts", "src/shared/vsCodeSelectorUtils.test.ts", + "src/core/storage/__tests__/**/*.test.ts", "src/core/storage/remote-config/**/*.test.ts", + "src/services/mcp/__tests__/settingsLock.test.ts", "src/shared/model-catalog/provider-helpers.test.ts", "src/core/controller/models/__tests__/providerCatalogHandlers.test.ts", "src/core/controller/models/__tests__/providerSwitchNormalization.test.ts", diff --git a/sdk/packages/core/src/auth/server.test.ts b/sdk/packages/core/src/auth/server.test.ts index c63d56d494..25f588f02b 100644 --- a/sdk/packages/core/src/auth/server.test.ts +++ b/sdk/packages/core/src/auth/server.test.ts @@ -256,6 +256,82 @@ describe("auth/server startLocalOAuthServer — onClose", () => { }); }); +// --------------------------------------------------------------------------- +// Sequential flows on a fixed port (keep-alive socket teardown) +// --------------------------------------------------------------------------- + +describe("auth/server startLocalOAuthServer — sequential flows on a fixed port", () => { + // A browser / global-fetch connection pool keeps a keep-alive socket to the + // callback port alive across requests. If close() left those sockets open, a + // later request to a re-bound port could be delivered over the pooled socket + // to the first (already-settled) server, and that flow's waitForCallback() + // would never resolve. close() must therefore drop lingering connections. + socketIt( + "does not serve requests over a pooled keep-alive socket after close()", + async () => { + const port = await getFreePort(); + + // A keep-alive agent models the browser / global-fetch connection pool, + // which keeps a socket to the fixed callback port alive across requests. + const agent = new http.Agent({ keepAlive: true, maxSockets: 1 }); + const getOverAgent = (path: string) => + new Promise<{ status: number } | { error: string }>((resolve) => { + const req = http.get( + { host: "127.0.0.1", port, path, agent }, + (res) => { + res.on("data", () => {}); + res.on("end", () => resolve({ status: res.statusCode ?? 0 })); + }, + ); + req.on("error", (e) => + resolve({ error: (e as NodeJS.ErrnoException).code ?? e.message }), + ); + }); + + // First flow: deny. The 400 response leaves a pooled keep-alive socket + // attached to THIS server. + const first = await startLocalOAuthServer({ + ports: [port], + callbackPath: "/callback", + }); + const firstWait = first.waitForCallback(); + expect(await getOverAgent("/callback?error=access_denied")).toMatchObject( + { status: 400 }, + ); + expect((await firstWait)?.error).toBe("access_denied"); + first.close(); + + // A second request over the same agent must not be served by the closed + // first server. close() destroys the pooled socket, so the reused + // connection errors rather than reaching the (already-settled) server — + // which is what would otherwise swallow a subsequent flow's callback. + const reused = await getOverAgent("/callback?code=abc123&state=xyz"); + expect(reused).not.toHaveProperty("status"); + expect(reused).toHaveProperty("error"); + + // And a brand-new server can bind the same port and serve normally. + const second = await startLocalOAuthServer({ + ports: [port], + callbackPath: "/callback", + }); + const secondWait = second.waitForCallback(); + expect( + await get(`http://127.0.0.1:${port}/callback?code=abc123&state=xyz`), + ).toMatchObject({ status: 200 }); + const secondPayload = await Promise.race([ + secondWait, + new Promise((_, rej) => + setTimeout(() => rej(new Error("flow 2 callback hung")), 3000), + ), + ]); + expect(secondPayload?.code).toBe("abc123"); + + second.close(); + agent.destroy(); + }, + ); +}); + // --------------------------------------------------------------------------- // onListening + onClose ordering // --------------------------------------------------------------------------- diff --git a/sdk/packages/core/src/auth/server.ts b/sdk/packages/core/src/auth/server.ts index 4ecbb61ddf..c00a8bf5c1 100644 --- a/sdk/packages/core/src/auth/server.ts +++ b/sdk/packages/core/src/auth/server.ts @@ -99,6 +99,15 @@ export async function startLocalOAuthServer( boundPort = null; if (activeServer) { activeServer.close(); + // `Server.close()` only stops accepting new connections; existing + // keep-alive sockets keep working. A browser / global-fetch connection + // pool keeps such a socket to the (fixed) callback port alive, and a + // later request to that port can be delivered over the pooled socket to + // this server even after it has settled — so a subsequent OAuth flow + // reusing the port would have its callback delivered here and never + // resolve. Drop lingering connections so no pooled socket outlives this + // server. + activeServer.closeAllConnections?.(); activeServer = null; } if (closingPort !== null && options.onClose) { diff --git a/sdk/packages/core/src/extensions/mcp/config-loader.test.ts b/sdk/packages/core/src/extensions/mcp/config-loader.test.ts index cc7cc8878c..fa7f15825c 100644 --- a/sdk/packages/core/src/extensions/mcp/config-loader.test.ts +++ b/sdk/packages/core/src/extensions/mcp/config-loader.test.ts @@ -1,7 +1,8 @@ +import { existsSync, mkdirSync, readdirSync, renameSync, rmdirSync, statSync, unlinkSync, writeFileSync } from "node:fs"; import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { hasMcpSettingsFile, listMcpServerOAuthStatuses, @@ -10,6 +11,9 @@ import { resolveMcpServerRegistrations, setMcpServerDisabled, updateMcpServerOAuthState, + updateMcpSettingsFile, + updateMcpSettingsFileSync, + McpSettingsMutatorPurityError, } from "./config-loader"; describe("mcp config loader", () => { @@ -399,4 +403,248 @@ describe("mcp config loader", () => { } } }); + + it("serializes concurrent oauth updates so neither write is lost", async () => { + const tempRoot = await mkdtemp(join(tmpdir(), "core-mcp-config-loader-")); + tempRoots.push(tempRoot); + const filePath = join(tempRoot, "cline_mcp_settings.json"); + await writeFile( + filePath, + JSON.stringify( + { + mcpServers: { + linear: { transport: { type: "streamableHttp", url: "https://linear.example.com" } }, + github: { transport: { type: "streamableHttp", url: "https://github.example.com" } }, + }, + }, + null, + 2, + ), + "utf8", + ); + + updateMcpServerOAuthState("linear", () => ({ tokens: { access_token: "linear-token" } }), { + filePath, + }); + updateMcpServerOAuthState("github", () => ({ tokens: { access_token: "github-token" } }), { + filePath, + }); + + const written = JSON.parse(await readFile(filePath, "utf8")); + expect(written.mcpServers.linear.oauth?.tokens?.access_token).toBe("linear-token"); + expect(written.mcpServers.github.oauth?.tokens?.access_token).toBe("github-token"); + // Lockfile is released after each critical section. + expect(existsSync(`${filePath}.lock`)).toBe(false); + }); + + it("reclaims a stale lock directory older than the hang timeout", async () => { + const tempRoot = await mkdtemp(join(tmpdir(), "core-mcp-config-loader-")); + tempRoots.push(tempRoot); + const filePath = join(tempRoot, "cline_mcp_settings.json"); + await writeFile(filePath, JSON.stringify({ mcpServers: {} }, null, 2), "utf8"); + + // Simulate a crashed writer that left a lock directory behind, backdated well + // past the 10s stale threshold. + const lockPath = `${filePath}.lock`; + mkdirSync(lockPath); + writeFileSync(join(lockPath, "owner.dead"), "dead-owner"); + const stale = new Date(Date.now() - 60_000); + const { utimesSync } = await import("node:fs"); + utimesSync(lockPath, stale, stale); + + let ran = false; + updateMcpSettingsFileSync(filePath, () => { + ran = true; + }); + + expect(ran).toBe(true); + // The stale lock was reclaimed and our own lock released afterward. + expect(existsSync(lockPath)).toBe(false); + expect(statSync(filePath).isFile()).toBe(true); + }); + + it("does not delete another owner's replacement lock directory on release", async () => { + const tempRoot = await mkdtemp(join(tmpdir(), "core-mcp-config-loader-")); + tempRoots.push(tempRoot); + const filePath = join(tempRoot, "cline_mcp_settings.json"); + await writeFile(filePath, JSON.stringify({ mcpServers: {} }, null, 2), "utf8"); + + const lockPath = `${filePath}.lock`; + updateMcpSettingsFileSync(filePath, () => { + // Simulate another process reclaiming our lock directory before our + // finally release runs. Release must remove only our owner marker and then + // rmdir the directory; a populated replacement must survive. + const [owner] = readdirSync(lockPath); + unlinkSync(join(lockPath, owner)); + rmdirSync(lockPath); + const replacement = `${lockPath}.replacement`; + mkdirSync(replacement); + writeFileSync(join(replacement, "owner.replacement"), "replacement-owner", { flag: "wx" }); + renameSync(replacement, lockPath); + }); + + expect(readdirSync(lockPath)).toEqual(["owner.replacement"]); + }); + + it("rejects impure settings mutators whose output changes across calls", async () => { + const tempRoot = await mkdtemp(join(tmpdir(), "core-mcp-config-loader-")); + tempRoots.push(tempRoot); + const filePath = join(tempRoot, "cline_mcp_settings.json"); + await writeFile(filePath, JSON.stringify({ mcpServers: {} }, null, 2), "utf8"); + + let count = 0; + expect(() => + updateMcpSettingsFileSync(filePath, (settings) => { + count += 1; + settings.mcpServers = { generated: { counter: count } }; + }), + ).toThrow(McpSettingsMutatorPurityError); + }); +}); + +describe("updateMcpSettingsFile (async acquisition)", () => { + const tempRoots: string[] = []; + + afterEach(async () => { + await Promise.all( + tempRoots.map((directory) => rm(directory, { recursive: true, force: true })), + ); + tempRoots.length = 0; + }); + + async function makeSettingsFile(): Promise { + const tempRoot = await mkdtemp(join(tmpdir(), "core-mcp-config-loader-async-")); + tempRoots.push(tempRoot); + const filePath = join(tempRoot, "cline_mcp_settings.json"); + await writeFile(filePath, JSON.stringify({ mcpServers: {} }, null, 2), "utf8"); + return filePath; + } + + it("runs the mutator and releases the lock on the uncontended path", async () => { + const filePath = await makeSettingsFile(); + let mutatorRan = false; + + const result = await updateMcpSettingsFile(filePath, (settings) => { + mutatorRan = true; + settings.mcpServers = { alpha: { transport: { type: "stdio", command: "node" } } }; + return "ok"; + }); + + // The mutator (a synchronous, pure function) ran, the write landed, and the + // lock directory is gone — i.e. the lock is never left held after resolution. + // (That the lock is never held *across* an await is covered by the contended + // test below, which asserts serialization without any Atomics.wait.) + expect(mutatorRan).toBe(true); + expect(result).toBe("ok"); + expect(existsSync(`${filePath}.lock`)).toBe(false); + const written = JSON.parse(await readFile(filePath, "utf8")); + expect(written.mcpServers.alpha.transport.command).toBe("node"); + }); + + it("serializes contended async updates without losing a write and never blocks the event loop", async () => { + const filePath = await makeSettingsFile(); + const waitSpy = vi.spyOn(Atomics, "wait"); + try { + // Pre-place a held lock owned by a fictional live process so the first + // real update has to wait (and therefore exercise the await-delay path). + const lockDir = `${filePath}.lock`; + mkdirSync(lockDir); + writeFileSync(join(lockDir, "owner.holder"), "holder"); + + const linear = updateMcpSettingsFile( + filePath, + (settings) => { + const servers = settings.mcpServers as Record>; + servers.linear.oauth = { tokens: { access_token: "linear-token" } }; + }, + { timeoutMs: 5_000 }, + ).catch(() => undefined); + const github = updateMcpSettingsFile( + filePath, + (settings) => { + const servers = settings.mcpServers as Record>; + servers.github.oauth = { tokens: { access_token: "github-token" } }; + }, + { timeoutMs: 5_000 }, + ).catch(() => undefined); + + // Seed the two servers while the contender(s) are parked on the lock, + // then release the held lock so the waiters can proceed. + await writeFile( + filePath, + JSON.stringify( + { + mcpServers: { + linear: { transport: { type: "streamableHttp", url: "https://linear.example.com" } }, + github: { transport: { type: "streamableHttp", url: "https://github.example.com" } }, + }, + }, + null, + 2, + ), + "utf8", + ); + unlinkSync(join(lockDir, "owner.holder")); + rmdirSync(lockDir); + + await Promise.all([linear, github]); + + const written = JSON.parse(await readFile(filePath, "utf8")); + expect(written.mcpServers.linear.oauth?.tokens?.access_token).toBe("linear-token"); + expect(written.mcpServers.github.oauth?.tokens?.access_token).toBe("github-token"); + // Lock released after each critical section. + expect(existsSync(lockDir)).toBe(false); + // The whole point of the async path: it must never freeze the loop. + expect(waitSpy).not.toHaveBeenCalled(); + } finally { + waitSpy.mockRestore(); + } + }); + + it("creates a missing settings file inside the lock", async () => { + const tempRoot = await mkdtemp(join(tmpdir(), "core-mcp-config-loader-async-")); + tempRoots.push(tempRoot); + const filePath = join(tempRoot, "cline_mcp_settings.json"); + + await updateMcpSettingsFile(filePath, (settings) => { + const servers = settings.mcpServers as Record; + servers.docs = { transport: { type: "stdio", command: "node" } }; + }); + + const written = JSON.parse(await readFile(filePath, "utf8")); + expect(Object.keys(written.mcpServers)).toEqual(["docs"]); + }); + + it("reclaims a stale lock directory on the async path", async () => { + const filePath = await makeSettingsFile(); + const lockDir = `${filePath}.lock`; + mkdirSync(lockDir); + writeFileSync(join(lockDir, "owner.dead"), "dead-owner"); + const stale = new Date(Date.now() - 60_000); + const { utimesSync } = await import("node:fs"); + utimesSync(lockDir, stale, stale); + + let ran = false; + await updateMcpSettingsFile(filePath, () => { + ran = true; + }); + + expect(ran).toBe(true); + expect(existsSync(lockDir)).toBe(false); + }); + + it("fails fast on a reentrant settings update instead of deadlocking the loop", async () => { + const filePath = await makeSettingsFile(); + + await expect( + updateMcpSettingsFile(filePath, () => { + // A nested write on the same file would deadlock against the lock we + // already hold; the shared reentrancy guard must reject it instead. + updateMcpSettingsFileSync(filePath, () => {}); + }), + ).rejects.toThrow(/Reentrant MCP settings update/); + + // Guard ran inside the mutator, but the outer lock is still cleaned up. + expect(existsSync(`${filePath}.lock`)).toBe(false); + }); }); diff --git a/sdk/packages/core/src/extensions/mcp/config-loader.ts b/sdk/packages/core/src/extensions/mcp/config-loader.ts index a3735865f8..c62923808f 100644 --- a/sdk/packages/core/src/extensions/mcp/config-loader.ts +++ b/sdk/packages/core/src/extensions/mcp/config-loader.ts @@ -1,5 +1,18 @@ -import { existsSync, mkdirSync, readFileSync, writeFileSync } from "node:fs"; -import { dirname } from "node:path"; +import { + existsSync, + mkdirSync, + readFileSync, + renameSync, + rmdirSync, + rmSync, + statSync, + unlinkSync, + writeFileSync, +} from "node:fs"; +import { randomUUID } from "node:crypto"; +import { dirname, join } from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; +import type { BasicLogger } from "@cline/shared"; import { resolveMcpSettingsPath } from "@cline/shared/storage"; import { z } from "zod"; import type { @@ -185,10 +198,329 @@ export interface SetMcpServerDisabledOptions { disabled: boolean; } +export interface McpSettingsLockOptions { + /** Maximum time to wait for the lock before failing. Defaults to 10 seconds. */ + timeoutMs?: number; + /** Optional host logger; stale-lock takeover is logged as severity=warn. */ + logger?: BasicLogger; +} + +export type McpSettingsMutator = (settings: Record) => T; + +export class McpSettingsUpdateSkippedError extends Error { + constructor(message: string) { + super(message); + this.name = "McpSettingsUpdateSkippedError"; + } +} + +export class McpSettingsLockTimeoutError extends Error { + constructor(message: string) { + super(message); + this.name = "McpSettingsLockTimeoutError"; + } +} + +export class McpSettingsMutatorPurityError extends Error { + constructor(message: string) { + super(message); + this.name = "McpSettingsMutatorPurityError"; + } +} + export function resolveDefaultMcpSettingsPath(): string { return resolveMcpSettingsPath(); } +/** + * Atomically write the MCP settings file using a temp file + rename. + * + * Multiple processes (CLI, VSCode extension windows, JetBrains) read and write + * this file concurrently. A plain writeFileSync can be observed half-written by + * a concurrent reader, surfacing as a JSON parse error or, for a client that + * treats an unreadable file as "no servers", silently dropping MCP state. + * Rename within the same directory is atomic on POSIX and on NTFS, so a reader + * always observes either the old or the new complete file. + */ +function atomicWriteSettingsFile(filePath: string, contents: string): void { + mkdirSync(dirname(filePath), { recursive: true }); + const tempPath = `${filePath}.tmp.${process.pid}.${Date.now()}.${Math.random() + .toString(36) + .slice(2)}`; + try { + writeFileSync(tempPath, contents, { encoding: "utf8", flag: "wx" }); + renameSync(tempPath, filePath); + } catch (error) { + try { + unlinkSync(tempPath); + } catch { + // Best-effort cleanup of the temp file. + } + throw error; + } +} + +/** + * How long (ms) a lock directory may exist before it is considered stale and + * forcibly reclaimed. A real critical section here is a handful of synchronous + * file ops (sub-millisecond), so this is orders of magnitude larger than any + * legitimate hold. A lock older than this can only mean the owner crashed or + * was killed mid-update, so taking it over prevents a permanent deadlock. + */ +const SETTINGS_LOCK_STALE_MS = 10_000; + +/** Poll interval (ms) while waiting for another process to release the lock. */ +const SETTINGS_LOCK_POLL_MS = 25; + +const syncSleepBuffer = new Int32Array(new SharedArrayBuffer(4)); + +/** + * Lock directories this process currently holds. The on-disk lock is not + * reentrant, so a settings update whose file is already locked by this process + * throws rather than waiting. Both acquisition paths register here. + */ +const activeLocks = new Set(); + +function sleepSync(ms: number): void { + Atomics.wait(syncSleepBuffer, 0, 0, ms); +} + +function settingsLockDir(filePath: string): string { + return `${filePath}.lock`; +} + +function makeLockToken(): string { + return `${process.pid}.${Date.now()}.${randomUUID()}`; +} + +interface AcquiredSettingsLock { + lockDir: string; + ownerFile: string; +} + +/** + * Cross-process lock implemented as a populated directory. Acquisition creates a + * staging directory, writes a unique owner marker inside it, then renames the + * populated directory into place. That means the visible lock directory is never + * empty. Release only deletes our marker and then rmdir's the directory; if + * another owner replaced the lock, our marker is absent or the directory is + * non-empty, so we do not remove their lock. Stale takeover renames the whole + * lock directory aside before deleting it. This uses standard mkdir/rename/rmdir + * operations and avoids inode- or handle-based deletion, so it works with + * Node's portable fs APIs on Windows and POSIX. + */ +function tryAcquireSettingsLock(lockDir: string, token: string): AcquiredSettingsLock | undefined { + mkdirSync(dirname(lockDir), { recursive: true }); + const stagingDir = `${lockDir}.tmp.${token}`; + rmSync(stagingDir, { recursive: true, force: true }); + mkdirSync(stagingDir, { recursive: true }); + writeFileSync(join(stagingDir, `owner.${token}`), token, { encoding: "utf8", flag: "wx" }); + try { + renameSync(stagingDir, lockDir); + return { lockDir, ownerFile: join(lockDir, `owner.${token}`) }; + } catch (error) { + rmSync(stagingDir, { recursive: true, force: true }); + if (existsSync(lockDir)) { + return undefined; + } + throw error; + } +} + +function reclaimStaleLock(lockDir: string, options: McpSettingsLockOptions): void { + let ageMs: number; + try { + ageMs = Date.now() - statSync(lockDir).mtimeMs; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return; + } + throw error; + } + if (ageMs < SETTINGS_LOCK_STALE_MS) { + return; + } + options.logger?.log(`[mcp-settings] Stale lock directory at ${lockDir} (age ${ageMs}ms); reclaiming.`, { + severity: "warn", + }); + const staleDir = `${lockDir}.stale.${makeLockToken()}`; + try { + renameSync(lockDir, staleDir); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return; + } + throw error; + } + rmSync(staleDir, { recursive: true, force: true }); +} + +function releaseSettingsLock(lock: AcquiredSettingsLock): void { + try { + unlinkSync(lock.ownerFile); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error; + } + } + try { + rmdirSync(lock.lockDir); + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code !== "ENOENT" && code !== "ENOTEMPTY" && code !== "EEXIST") { + throw error; + } + } +} + +function beginAcquire(filePath: string): { lockDir: string; token: string } { + const lockDir = settingsLockDir(filePath); + if (activeLocks.has(lockDir)) { + throw new Error( + `Reentrant MCP settings update for ${filePath}. Keep mutators pure: compute values up front and do not call back into the settings API.`, + ); + } + return { lockDir, token: makeLockToken() }; +} + +function acquireSettingsLockSync(filePath: string, options: McpSettingsLockOptions): AcquiredSettingsLock { + const { lockDir, token } = beginAcquire(filePath); + const timeoutMs = options.timeoutMs ?? SETTINGS_LOCK_STALE_MS; + const startedAt = Date.now(); + while (true) { + const lock = tryAcquireSettingsLock(lockDir, token); + if (lock) { + activeLocks.add(lockDir); + return lock; + } + if (Date.now() - startedAt > timeoutMs) { + throw new McpSettingsLockTimeoutError( + `Timed out waiting for MCP settings lock at ${lockDir} after ${timeoutMs}ms.`, + ); + } + reclaimStaleLock(lockDir, options); + sleepSync(SETTINGS_LOCK_POLL_MS); + } +} + +/** + * Acquire the settings lock, yielding the event loop (`await delay`) between + * attempts. Reclaims a stale lock left by a crashed holder and throws + * McpSettingsLockTimeoutError once `timeoutMs` elapses. + */ +async function acquireSettingsLockAsync(filePath: string, options: McpSettingsLockOptions): Promise { + const { lockDir, token } = beginAcquire(filePath); + const timeoutMs = options.timeoutMs ?? SETTINGS_LOCK_STALE_MS; + const startedAt = Date.now(); + while (true) { + const lock = tryAcquireSettingsLock(lockDir, token); + if (lock) { + activeLocks.add(lockDir); + return lock; + } + if (Date.now() - startedAt > timeoutMs) { + throw new McpSettingsLockTimeoutError( + `Timed out waiting for MCP settings lock at ${lockDir} after ${timeoutMs}ms.`, + ); + } + reclaimStaleLock(lockDir, options); + await delay(SETTINGS_LOCK_POLL_MS); + } +} + +/** + * Run the read-modify-write while the lock is held. The body is synchronous and + * never yields, so a concurrent waiter cannot interleave between the read and + * the write, and the lock is released the moment the mutation completes. + */ +function runLockedSettingsMutation(lock: AcquiredSettingsLock, filePath: string, mutator: McpSettingsMutator): T { + try { + const settings = loadRawSettingsObject(filePath); + const result = runPureSettingsMutator(settings, mutator); + atomicWriteSettingsFile(filePath, `${JSON.stringify(settings, null, 2)}\n`); + return result; + } finally { + activeLocks.delete(lock.lockDir); + releaseSettingsLock(lock); + } +} + +/** + * Locked MCP settings read-modify-write that blocks the event loop (via + * `Atomics.wait`) while acquiring the lock. + * + * Prefer {@link updateMcpSettingsFile}: async acquisition keeps the event loop + * free and behaves identically otherwise. + * + * TODO: Delete once all callers migrate to {@link updateMcpSettingsFile}. + * + * The mutator is synchronous and may be called more than once with the same + * input to verify it is pure/deterministic. Compute any I/O, logging, network, + * timestamp, or random values before calling and close over them. Return a value + * only for a successful update; throw McpSettingsUpdateSkippedError to skip. + */ +export function updateMcpSettingsFileSync( + filePath: string, + mutator: McpSettingsMutator, + options: McpSettingsLockOptions = {}, +): T { + const lock = acquireSettingsLockSync(filePath, options); + return runLockedSettingsMutation(lock, filePath, mutator); +} + +/** + * Locked MCP settings read-modify-write that yields the event loop while + * acquiring the lock, so concurrent work on the same loop keeps running. + * + * The mutator is synchronous and may be called more than once with the same + * input to verify it is pure/deterministic. Compute any I/O, logging, network, + * timestamp, or random values before calling and close over them. The lock is + * held only across the synchronous mutation, never across an `await`. Return a + * value only for a successful update; throw McpSettingsUpdateSkippedError to skip. + */ +export async function updateMcpSettingsFile( + filePath: string, + mutator: McpSettingsMutator, + options: McpSettingsLockOptions = {}, +): Promise { + const lock = await acquireSettingsLockAsync(filePath, options); + return runLockedSettingsMutation(lock, filePath, mutator); +} + +/** + * Read the settings object for a locked read-modify-write. A missing file + * bootstraps to `{ mcpServers: {} }`, so the first write to a fresh path creates + * the file inside the lock instead of throwing. The subsequent atomic write + * persists it. + */ +function loadRawSettingsObject(filePath: string): Record { + const settings = readJsonObjectOrEmpty(filePath); + if (!settings.mcpServers || typeof settings.mcpServers !== "object" || Array.isArray(settings.mcpServers)) { + settings.mcpServers = {}; + } + return settings; +} + +function runPureSettingsMutator(settings: Record, mutator: McpSettingsMutator): T { + const before = JSON.stringify(settings); + const shadow = JSON.parse(before) as Record; + const shadowResult = mutator(shadow); + const shadowAfter = JSON.stringify(shadow); + const result = mutator(settings); + const after = JSON.stringify(settings); + if (after !== shadowAfter) { + throw new McpSettingsMutatorPurityError( + "MCP settings mutator must be deterministic and free of side effects; repeated calls produced different settings.", + ); + } + if (JSON.stringify(result) !== JSON.stringify(shadowResult)) { + throw new McpSettingsMutatorPurityError( + "MCP settings mutator must be deterministic and free of side effects; repeated calls produced different return values.", + ); + } + return result; +} + function readJsonObject(filePath: string): Record { const raw = readFileSync(filePath, "utf8"); let parsed: unknown; @@ -206,6 +538,21 @@ function readJsonObject(filePath: string): Record { return parsed as Record; } +/** + * Like {@link readJsonObject}, but treats a missing file as an empty object so a + * locked write can create it. A present-but-malformed file still throws. + */ +function readJsonObjectOrEmpty(filePath: string): Record { + try { + return readJsonObject(filePath); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return {}; + } + throw error; + } +} + function getOwnServerRecord( servers: Record, name: string, @@ -260,30 +607,6 @@ export function loadMcpSettingsFile( return result.data; } -function loadRawMcpSettingsFile(filePath: string): Record { - const raw = readFileSync(filePath, "utf8"); - let parsed: unknown; - try { - parsed = JSON.parse(raw); - } catch (error) { - const details = error instanceof Error ? error.message : String(error); - throw new Error( - `Failed to parse MCP settings JSON at "${filePath}": ${details}`, - ); - } - if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { - throw new Error(`Invalid MCP settings at "${filePath}": expected object`); - } - const settings = parsed as Record; - const servers = settings.mcpServers; - if (!servers || typeof servers !== "object" || Array.isArray(servers)) { - throw new Error( - `Invalid MCP settings at "${filePath}": mcpServers must be an object`, - ); - } - return settings; -} - export function normalizeMcpServerOAuthState( value: McpServerOAuthState | undefined, ): McpServerOAuthState | undefined { @@ -345,34 +668,31 @@ export function setMcpServerDisabled( if (!name) { throw new Error("MCP server settings toggle requires a server name."); } - const settings = readJsonObject(filePath); - const serversValue = settings.mcpServers; - if ( - !serversValue || - typeof serversValue !== "object" || - Array.isArray(serversValue) - ) { - throw new Error( - `Invalid MCP settings at "${filePath}": mcpServers must be an object.`, - ); - } - const servers = { ...(serversValue as Record) }; - const current = getOwnServerRecord(servers, name); - if (!current) { - throw new Error(`Unknown MCP server: ${name}`); - } - const next = { ...current }; - if (options.disabled) { - next.disabled = true; - } else { - delete next.disabled; - } - setOwnServerRecord(servers, name, next); - mkdirSync(dirname(filePath), { recursive: true }); - writeFileSync( - filePath, - `${JSON.stringify({ ...settings, mcpServers: servers }, null, 2)}\n`, - ); + updateMcpSettingsFileSync(filePath, (settings) => { + const serversValue = settings.mcpServers; + if ( + !serversValue || + typeof serversValue !== "object" || + Array.isArray(serversValue) + ) { + throw new Error( + `Invalid MCP settings at "${filePath}": mcpServers must be an object.`, + ); + } + const servers = { ...(serversValue as Record) }; + const current = getOwnServerRecord(servers, name); + if (!current) { + throw new Error(`Unknown MCP server: ${name}`); + } + const next = { ...current }; + if (options.disabled) { + next.disabled = true; + } else { + delete next.disabled; + } + setOwnServerRecord(servers, name, next); + settings.mcpServers = servers; + }); } export function getMcpServerOAuthState( @@ -386,30 +706,57 @@ export function getMcpServerOAuthState( return normalizeMcpServerOAuthState(config.mcpServers[serverName]?.oauth); } +function buildOAuthStateMutator( + serverName: string, + updater: (current: McpServerOAuthState) => McpServerOAuthState, +): McpSettingsMutator { + return (settings) => { + const servers = settings.mcpServers as Record; + const server = getOwnServerRecord(servers, serverName); + if (!server) { + throw new Error(`Unknown MCP server: ${serverName}`); + } + + const current = validateOauthState(server.oauth) ?? {}; + const updated = normalizeMcpServerOAuthState(updater(current)); + if (updated) { + server.oauth = updated; + } else { + delete server.oauth; + } + return updated ?? {}; + }; +} + +/** + * Scoped read-modify-write of one server's `oauth` block that blocks the event + * loop while acquiring the lock. + * + * Prefer {@link updateMcpServerOAuthStateAsync}. + * + * TODO: Delete once all callers migrate to {@link updateMcpServerOAuthStateAsync}. + */ export function updateMcpServerOAuthState( serverName: string, updater: (current: McpServerOAuthState) => McpServerOAuthState, options: LoadMcpSettingsOptions = {}, ): McpServerOAuthState { const filePath = options.filePath ?? resolveDefaultMcpSettingsPath(); - const settings = loadRawMcpSettingsFile(filePath); - const servers = settings.mcpServers as Record; - const server = getOwnServerRecord(servers, serverName); - if (!server) { - throw new Error(`Unknown MCP server: ${serverName}`); - } + return updateMcpSettingsFileSync(filePath, buildOAuthStateMutator(serverName, updater)); +} - const current = validateOauthState(server.oauth) ?? {}; - const updated = normalizeMcpServerOAuthState(updater(current)); - if (updated) { - server.oauth = updated; - } else { - delete server.oauth; - } - - mkdirSync(dirname(filePath), { recursive: true }); - writeFileSync(filePath, `${JSON.stringify(settings, null, 2)}\n`, "utf8"); - return updated ?? {}; +/** + * Scoped read-modify-write of one server's `oauth` block that yields the event + * loop while acquiring the lock. The updater is synchronous and pure; it + * receives the server's current OAuth state and returns the next state. + */ +export async function updateMcpServerOAuthStateAsync( + serverName: string, + updater: (current: McpServerOAuthState) => McpServerOAuthState, + options: LoadMcpSettingsOptions = {}, +): Promise { + const filePath = options.filePath ?? resolveDefaultMcpSettingsPath(); + return updateMcpSettingsFile(filePath, buildOAuthStateMutator(serverName, updater)); } export function listMcpServerOAuthStatuses( diff --git a/sdk/packages/core/src/extensions/mcp/index.ts b/sdk/packages/core/src/extensions/mcp/index.ts index d2a18a5d23..82c263810e 100644 --- a/sdk/packages/core/src/extensions/mcp/index.ts +++ b/sdk/packages/core/src/extensions/mcp/index.ts @@ -2,6 +2,8 @@ export type { DefaultMcpServerClientFactoryOptions } from "./client"; export { createDefaultMcpServerClientFactory } from "./client"; export type { LoadMcpSettingsOptions, + McpSettingsLockOptions, + McpSettingsMutator, McpSettingsFile, RegisterMcpServersFromSettingsOptions, SetMcpServerDisabledOptions, @@ -16,6 +18,12 @@ export { resolveMcpServerRegistrations, setMcpServerDisabled, updateMcpServerOAuthState, + updateMcpServerOAuthStateAsync, + updateMcpSettingsFile, + updateMcpSettingsFileSync, + McpSettingsLockTimeoutError, + McpSettingsMutatorPurityError, + McpSettingsUpdateSkippedError, } from "./config-loader"; export { InMemoryMcpManager } from "./manager"; export type { diff --git a/sdk/packages/core/src/extensions/mcp/oauth.ts b/sdk/packages/core/src/extensions/mcp/oauth.ts index 7502ab5085..e41adac9c3 100644 --- a/sdk/packages/core/src/extensions/mcp/oauth.ts +++ b/sdk/packages/core/src/extensions/mcp/oauth.ts @@ -144,12 +144,13 @@ export function createMcpOAuthProviderContext( }, tokens: () => state.tokens as OAuthTokens | undefined, saveTokens: async (tokens) => { + const lastAuthenticatedAt = Date.now(); await patch((current) => ({ ...current, tokens: tokens as Record, redirectUrl: options.redirectUrl, lastError: undefined, - lastAuthenticatedAt: Date.now(), + lastAuthenticatedAt, })); }, redirectToAuthorization: async (authorizationUrl) => { diff --git a/sdk/packages/core/src/index.ts b/sdk/packages/core/src/index.ts index 88962c18e4..58443cfcc3 100644 --- a/sdk/packages/core/src/index.ts +++ b/sdk/packages/core/src/index.ts @@ -290,6 +290,8 @@ export { type McpServerSnapshot, type McpServerTransportConfig, type McpSettingsFile, + type McpSettingsLockOptions, + type McpSettingsMutator, type McpSseTransportConfig, type McpStdioTransportConfig, type McpStreamableHttpTransportConfig, @@ -305,6 +307,12 @@ export { type SetMcpServerDisabledOptions, setMcpServerDisabled, updateMcpServerOAuthState, + updateMcpServerOAuthStateAsync, + updateMcpSettingsFile, + updateMcpSettingsFileSync, + McpSettingsLockTimeoutError, + McpSettingsMutatorPurityError, + McpSettingsUpdateSkippedError, } from "./extensions/mcp"; export { type AgentTask,