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,