From 95f6d08317c54b98598960d3d1fafcba7127e72b Mon Sep 17 00:00:00 2001 From: EricSanchez Date: Mon, 31 Aug 2026 18:16:04 +0800 Subject: [PATCH 1/3] fix(mcp): isolate OAuth ownership from background connects and harden observability Background supervisor auto-connects shared the PendingOAuth registry with interactive `mcp auth` flows: a queued 401 connect for the same server disposed the interactive pending entry, rejecting the callback wait with "Authorization cancelled" and failing authentication with no log trail. - McpAuth: drop the never-invalidated in-process cache so CLI-written tokens are visible to a running server without restart. - McpOAuthProvider: add background mode; probe connects no longer write PKCE state or tokens into the shared auth store. - supervisor: background 401s never register PendingOAuth entries; servers enter NeedsAuth and a 30s local check reconnects once credentials exist, with zero network probes while unauthenticated. - observability: cancelPending logs WARN with reason, callback port conflicts name SYNERGY_OAUTH_CALLBACK_PORT, CLI auth failures write to the file log. - tests: race regression (background 401 during interactive wait), live token visibility, NeedsAuth auto-recovery, port-conflict error. Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com> --- ...026-08-31-mcp-oauth-ownership-isolation.md | 39 +++++ docs/reference/configuration-layout.md | 25 +++ packages/synergy/src/cli/cmd/mcp.ts | 7 + packages/synergy/src/mcp/auth.ts | 11 +- packages/synergy/src/mcp/oauth-callback.ts | 13 +- packages/synergy/src/mcp/oauth-provider.ts | 26 ++++ packages/synergy/src/mcp/supervisor.ts | 67 +++++--- packages/synergy/test/mcp/oauth.test.ts | 147 ++++++++++++++++++ 8 files changed, 299 insertions(+), 36 deletions(-) create mode 100644 docs/decisions/implemented/bug-fix/2026-08-31-mcp-oauth-ownership-isolation.md diff --git a/docs/decisions/implemented/bug-fix/2026-08-31-mcp-oauth-ownership-isolation.md b/docs/decisions/implemented/bug-fix/2026-08-31-mcp-oauth-ownership-isolation.md new file mode 100644 index 000000000..326105654 --- /dev/null +++ b/docs/decisions/implemented/bug-fix/2026-08-31-mcp-oauth-ownership-isolation.md @@ -0,0 +1,39 @@ +# Decision Record: Isolate MCP OAuth ownership and harden observability + +Status: implemented + +## Problem + +`synergy mcp auth ` failed with `Authentication failed` / `Authorization cancelled` immediately after opening the browser for any OAuth remote server (reproduced with Notion, 2026-08-31). The CLI process log ended right after `opening browser for oauth` with no callback received and no error line, making the failure unobservable. + +Root cause: the MCP supervisor's background auto-connect (default `startup: "eager"`, `MAX_CONCURRENT_STARTS = 3`) shares the process-level `PendingOAuth` registry with the interactive OAuth flow. When a queued background connect for the same server reached 401, `connectPipeline` ran `PendingOAuth.disposeIfIdentity(handle.name, handle.identity, "connection restarted")` or `PendingOAuth.register` (which replaces the existing entry and fires its `onDispose`), and the interactive flow's `onDispose` — `clearPendingOAuthState` — called `McpOAuthCallback.cancelPending`, rejecting the callback wait with `Authorization cancelled`. The CLI then exited via `process.exit()` before the supervisor could write its "requires authentication" log, producing the silent failure. + +Additional gaps found along the same ownership line: + +1. `McpAuth` kept an in-process cache (`auth.ts`) that was never invalidated in production, so a CLI-authenticated token was invisible to a long-running server process until restart. +2. Background connects used the interactive `McpOAuthProvider`, whose `saveState`/`saveCodeVerifier`/`saveTokens` wrote into the shared `authMcp` file, overwriting interactive-flow state mid-flight (state-mismatch risk). +3. `PendingOAuth` entries were created by background 401 probes on every connect attempt, repeatedly re-registering clients and widening the race window. +4. `cancelPending` rejected silently; CLI auth failures were only printed to the terminal, never logged to file; the fixed callback port 19876 failed with a bare message when already in use. + +## Decision + +Keep the CLI direct-connect architecture and eliminate the race by separating background probing from interactive ownership: + +- **R1 — `McpAuth` reads go to disk every time.** Removed the module cache; `all()` reads `Global.Path.authMcp` directly. `invalidateCache()` remains as a no-op for test compatibility. CLI-written tokens are visible to the server process on its next read. +- **R2 — `McpOAuthProvider` gained a `mode: "interactive" | "background"` (default `interactive`).** In background mode, `saveCodeVerifier`, `saveState`, `saveClientInformation`, and `saveTokens` are no-ops, and `codeVerifier()`/`state()` return provider-local memory values, so a background probe never writes the shared `authMcp` file. `tokens()`/`clientInformation()` still read live from disk so an already-authenticated server connects directly. +- **R3 — supervisor background connects never touch `PendingOAuth`.** Removed the `disposeIfIdentity` call at the top of `connectPipeline`; the 401 branch now closes the failed client and transitions the handle to `NeedsAuth` (with an actionable `lastError`) instead of registering a pending entry. A single `setInterval` (30 s, `unref()`-ed, lazily started when a handle enters `NeedsAuth`, cleared when none remain) checks each `NeedsAuth` handle: reads `McpAuth.getForUrl` locally, reconnects only when valid tokens exist, skips the handle while an interactive `PendingOAuth` entry exists, and never issues network calls without local credentials. `reset()` clears the timer. `checkNeedsAuthNow()` exposes one pass for tests. +- **R4 — observability.** `cancelPending` logs a WARN with `{ mcpName, reason }` before rejecting; the callback-port-in-use error names `SYNERGY_OAUTH_CALLBACK_PORT` as the escape hatch; the CLI `auth` handler writes `mcp auth failed` to the file log in every failure branch. +- **R5 — behavioral regression tests** in `test/mcp/oauth.test.ts`: a background 401 during an interactive wait no longer cancels the pending callback and the interactive flow completes end-to-end; background provider never persists state and sees externally written tokens immediately; `NeedsAuth` recovers automatically once credentials appear; callback-port conflict produces an actionable error. + +Behavioral coverage: `test/mcp/oauth.test.ts` (new "MCP OAuth race and recovery" describe) plus the existing `pending-oauth.test.ts`/`supervisor.test.ts` suites staying green unchanged. + +## Alternatives considered + +- **Server-hosted authentication (CLI delegates OAuth to a running server via `--attach`)** — rejected: changes the CLI behavior contract, depends on a running server, adds a dual-process test matrix, and the fallback path would still need this fix. +- **Minimal patch: pause background connects for a server while an interactive flow is pending** — rejected: closes one race exit but leaves the stale-cache gap, the cross-process state-file overwrite, the repeated registration probes, and the observability gaps. +- **Add conditional-replacement/locking semantics to `PendingOAuth`** — rejected: background connections would still own registry entries and write state files; cross-process interference remains. +- **Database or shared-state service for `authMcp`** — rejected: over-engineering for a small single-file JSON store with no concurrency-volume justification. + +## Consequences + +`synergy mcp auth ` is stable under concurrent background connects; the interactive flow owns `PendingOAuth` exclusively. A server process recovers within ≤30 s after CLI authentication without restart, because reads are live and the NeedsAuth timer reconnects when tokens appear. Unauthenticated servers stop re-registering clients in the background (zero network traffic while `NeedsAuth`). Failures are now visible in both terminal and file logs. The trade-offs: every `McpAuth` read is a disk read (small file, non-hot path — connections/status only); background probes that hit 401 no longer keep the transport alive for a later finishAuth (the interactive flow always creates its own transport, so nothing is lost); the 30 s recovery interval is the worst-case reconnect latency; the callback port remains fixed with a clear conflict error rather than dynamic port selection, keeping `redirect_uris` consistent with server-registered clients. diff --git a/docs/reference/configuration-layout.md b/docs/reference/configuration-layout.md index d0aeea1e4..73fdc5a1c 100644 --- a/docs/reference/configuration-layout.md +++ b/docs/reference/configuration-layout.md @@ -281,6 +281,31 @@ Feishu/Lark account configuration may pair an explicit `model` with `variant`. T A Feishu/Lark account may also set `projectDir` to bind the account's sessions to a project Scope. Resolution rules and error behavior are documented in the [Channels reference](../product/connections.md). +## MCP Server Authentication + +Remote MCP servers (`type: "remote"` in `40-mcp.jsonc`) use OAuth 2.0 authorization-code flows with PKCE and dynamic client registration by default. Credentials (tokens, registered client info, and in-flight PKCE state) live in the auth store, never in `40-mcp.jsonc`; the `oauth` object in the server config only carries a pre-registered `clientId`/`clientSecret`/`scope` when a server does not support dynamic registration. + +```jsonc +{ + "mcp": { + "notion": { + "type": "remote", + "url": "https://mcp.notion.com/mcp", + "oauth": {}, + "startup": "eager", + }, + }, +} +``` + +Authenticate a server with `synergy mcp auth ` (or `synergy mcp auth` to pick from a list). The command opens the provider's authorization page in a browser, waits for the local callback, exchanges the code for tokens, and saves them to the auth store. A long-running server process picks up CLI-authenticated credentials automatically within ~30 seconds (the supervisor re-checks servers in the `needs_auth` state on an interval and reconnects when valid tokens appear), so no restart is required after authenticating from the CLI. + +Background supervision never writes PKCE state: an unauthenticated server is marked `needs_auth` and left idle (no network probes) until credentials exist. This keeps background auto-connect from interfering with an interactive `mcp auth` flow for the same server. + +`startup` controls when a server connects: `"eager"` (default) connects at runtime start, `"lazy"` connects on first tool use, and `"manual"` never auto-connects (use `synergy mcp connect `). Set `"manual"` for a server you only authenticate on demand. + +The OAuth callback listener runs on `127.0.0.1:19876` by default. If another Synergy process is already running an OAuth flow on that port, `mcp auth` fails with an actionable error; set `SYNERGY_OAUTH_CALLBACK_PORT` to a free port to run a flow in the second process. + ## Feishu/Lark Channel Settings `90-channels.jsonc` owns the built-in Feishu/Lark Channel provider under `channel.feishu`. A minimal configuration is: diff --git a/packages/synergy/src/cli/cmd/mcp.ts b/packages/synergy/src/cli/cmd/mcp.ts index 3f927abbe..cc105c196 100644 --- a/packages/synergy/src/cli/cmd/mcp.ts +++ b/packages/synergy/src/cli/cmd/mcp.ts @@ -1,4 +1,5 @@ import { formatLocalDateTime } from "../../util/time-format" +import { Log } from "../../util/log" import { cmd } from "./cmd" import { Client } from "@modelcontextprotocol/sdk/client/index.js" import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js" @@ -229,6 +230,7 @@ export const McpAuthCommand = cmd({ spinner.stop("Authentication successful!") } else if (status.status === "needs_client_registration") { spinner.stop("Authentication failed", 1) + Log.Default.error("mcp auth failed: needs client registration", { serverName, error: status.error }) prompts.log.error(status.error) prompts.log.info("Add clientId to your MCP server config:") prompts.log.info(` @@ -244,12 +246,17 @@ export const McpAuthCommand = cmd({ }`) } else if (status.status === "failed") { spinner.stop("Authentication failed", 1) + Log.Default.error("mcp auth failed", { serverName, error: status.error }) prompts.log.error(status.error) } else { spinner.stop("Unexpected status: " + status.status, 1) } } catch (error) { spinner.stop("Authentication failed", 1) + Log.Default.error("mcp auth failed", { + serverName, + error: error instanceof Error ? error.message : String(error), + }) prompts.log.error(error instanceof Error ? error.message : String(error)) } diff --git a/packages/synergy/src/mcp/auth.ts b/packages/synergy/src/mcp/auth.ts index fd34966eb..e7cbce7b1 100644 --- a/packages/synergy/src/mcp/auth.ts +++ b/packages/synergy/src/mcp/auth.ts @@ -32,7 +32,6 @@ export namespace McpAuth { isCurrent?: () => boolean } - let cache: { filepath: string; data: Record } | undefined let mutation: Promise = Promise.resolve() function serialize(fn: () => Promise): Promise { @@ -61,16 +60,12 @@ export namespace McpAuth { }) } - export function invalidateCache() { - cache = undefined - } + /** Kept as a no-op for compatibility; reads always go to disk. */ + export function invalidateCache() {} export async function all(): Promise> { - const filepath = Global.Path.authMcp - if (cache?.filepath === filepath) return cache.data - const file = Bun.file(filepath) + const file = Bun.file(Global.Path.authMcp) const data = (await file.json().catch(() => ({}))) as Record - cache = { filepath, data } return data } diff --git a/packages/synergy/src/mcp/oauth-callback.ts b/packages/synergy/src/mcp/oauth-callback.ts index 673ac3b76..cae93fe2d 100644 --- a/packages/synergy/src/mcp/oauth-callback.ts +++ b/packages/synergy/src/mcp/oauth-callback.ts @@ -159,7 +159,9 @@ export namespace McpOAuthCallback { const port = getOAuthCallbackPort() const portFreed = await waitForPortInUse(false) if (!portFreed) { - throw new Error(`OAuth callback port ${port} is already in use`) + throw new Error( + `OAuth callback port ${port} is already in use — another Synergy process is running an OAuth flow. Complete it there or set SYNERGY_OAUTH_CALLBACK_PORT to a free port.`, + ) } server = Bun.serve({ @@ -171,7 +173,7 @@ export namespace McpOAuthCallback { } export function waitForCallback(oauthState: string, mcpName = oauthState): Promise { - cancelPending(mcpName) + cancelPending(mcpName, undefined, "superseded by a new OAuth flow") return new Promise((resolve, reject) => { const timeout = setTimeout(() => { const pending = removePending(oauthState) @@ -184,11 +186,14 @@ export namespace McpOAuthCallback { }) } - export function cancelPending(mcpName: string, expectedState?: string): void { + export function cancelPending(mcpName: string, expectedState?: string, reason = "cancelled"): void { const oauthState = stateByMcpName.get(mcpName) if (!oauthState || (expectedState !== undefined && oauthState !== expectedState)) return const pending = removePending(oauthState) - pending?.reject(new Error("Authorization cancelled")) + if (pending) { + log.warn("oauth callback cancelled", { mcpName, reason }) + pending.reject(new Error("Authorization cancelled")) + } } export async function isPortInUse(): Promise { diff --git a/packages/synergy/src/mcp/oauth-provider.ts b/packages/synergy/src/mcp/oauth-provider.ts index 4b18c7d2c..e946eccad 100644 --- a/packages/synergy/src/mcp/oauth-provider.ts +++ b/packages/synergy/src/mcp/oauth-provider.ts @@ -25,6 +25,8 @@ function getOAuthCallbackPort(): number { return port } +export type McpOAuthMode = "interactive" | "background" + export interface McpOAuthConfig { clientId?: string clientSecret?: string @@ -37,6 +39,9 @@ export interface McpOAuthCallbacks { } export class McpOAuthProvider implements OAuthClientProvider { + private memoryCodeVerifier: string | undefined + private memoryState: string | undefined + private get mutationOptions(): McpAuth.MutationOptions { return { isCurrent: this.callbacks.isCurrent } } @@ -45,6 +50,7 @@ export class McpOAuthProvider implements OAuthClientProvider { private serverUrl: string, private config: McpOAuthConfig, private callbacks: McpOAuthCallbacks, + private mode: McpOAuthMode = "interactive", ) {} get redirectUrl(): string { @@ -92,6 +98,7 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveClientInformation(info: OAuthClientInformationFull): Promise { + if (this.mode === "background") return await McpAuth.updateClientInfo( this.mcpName, { @@ -126,6 +133,7 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveTokens(tokens: OAuthTokens): Promise { + if (this.mode === "background") return await McpAuth.updateTokens( this.mcpName, { @@ -150,10 +158,20 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveCodeVerifier(codeVerifier: string): Promise { + if (this.mode === "background") { + this.memoryCodeVerifier = codeVerifier + return + } await McpAuth.updateCodeVerifier(this.mcpName, codeVerifier, this.mutationOptions) } async codeVerifier(): Promise { + if (this.mode === "background") { + if (!this.memoryCodeVerifier) { + throw new Error(`No code verifier saved for MCP server: ${this.mcpName}`) + } + return this.memoryCodeVerifier + } const entry = await McpAuth.get(this.mcpName) if (!entry?.codeVerifier) { throw new Error(`No code verifier saved for MCP server: ${this.mcpName}`) @@ -162,10 +180,18 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveState(state: string): Promise { + if (this.mode === "background") { + this.memoryState = state + return + } await McpAuth.updateOAuthState(this.mcpName, state, this.mutationOptions) } async state(): Promise { + if (this.mode === "background") { + if (!this.memoryState) this.memoryState = crypto.randomUUID() + return this.memoryState + } const entry = await McpAuth.get(this.mcpName) if (entry?.oauthState) return entry.oauthState const state = crypto.randomUUID() diff --git a/packages/synergy/src/mcp/supervisor.ts b/packages/synergy/src/mcp/supervisor.ts index 63e37c1fd..b71b0051e 100644 --- a/packages/synergy/src/mcp/supervisor.ts +++ b/packages/synergy/src/mcp/supervisor.ts @@ -141,6 +141,7 @@ export type ResourceCache = Record const log = Log.create({ service: "mcp.supervisor" }) const DEFAULT_TIMEOUT = 30_000 const MAX_CONCURRENT_STARTS = 3 +const NEEDS_AUTH_CHECK_INTERVAL_MS = 30_000 const SAFE_BASE_ENV_KEYS = new Set(["PATH", "HOME", "USER", "TMPDIR", "SHELL", "LANG", "XDG_CACHE_HOME"]) @@ -388,6 +389,7 @@ class McpSupervisorImpl { private initPromise?: Promise private mutation = Promise.resolve() private identityGeneration = 0 + private needsAuthTimer: ReturnType | undefined // ── Public ────────────────────────────────────────────────────────── @@ -502,6 +504,7 @@ class McpSupervisorImpl { this.activeStarts = 0 this.pendingStarts = [] this.initPromise = undefined + this.clearNeedsAuthCheck() await PendingOAuth.disposeAll("supervisor reset") await Promise.all(handles.map((handle) => this.disposeHandle(handle, "supervisor reset"))) @@ -810,13 +813,47 @@ class McpSupervisorImpl { } } + private scheduleNeedsAuthCheck(): void { + if (this.needsAuthTimer) return + this.needsAuthTimer = setInterval(() => { + void this.checkNeedsAuthHandles() + }, NEEDS_AUTH_CHECK_INTERVAL_MS) + if (typeof this.needsAuthTimer === "object" && "unref" in this.needsAuthTimer) this.needsAuthTimer.unref() + } + + /** Run one NeedsAuth recovery pass immediately. Exposed for tests and manual triggers. */ + async checkNeedsAuthNow(): Promise { + await this.checkNeedsAuthHandles() + } + + private clearNeedsAuthCheck(): void { + if (this.needsAuthTimer) { + clearInterval(this.needsAuthTimer) + this.needsAuthTimer = undefined + } + } + + private async checkNeedsAuthHandles(): Promise { + for (const handle of this.handles.values()) { + if (handle.state !== HS.NeedsAuth || !this.isCurrent(handle) || handle.config.type !== "remote") continue + // A running interactive OAuth flow owns this server; do not probe or reconnect. + if (PendingOAuth.get(handle.name)) continue + const entry = await McpAuth.getForUrl(handle.name, handle.config.url) + const valid = entry?.tokens && (!entry.tokens.expiresAt || entry.tokens.expiresAt > Date.now() / 1000) + if (!valid) continue + log.info("oauth credentials found, reconnecting", { key: handle.name }) + this.scheduleStart(handle) + } + const anyNeedsAuth = [...this.handles.values()].some((h) => h.state === HS.NeedsAuth) + if (!anyNeedsAuth) this.clearNeedsAuthCheck() + } + private async connectPipeline(handle: McpHandle): Promise { if (!this.isCurrent(handle)) return const gen = ++handle.generation handle.state = HS.Connecting const config = handle.config let client: Client | undefined - await PendingOAuth.disposeIfIdentity(handle.name, handle.identity, "connection restarted") if (!this.isCurrent(handle, gen)) return if (config.type === "remote") { @@ -840,6 +877,7 @@ class McpSupervisorImpl { }, isCurrent: () => this.isCurrent(handle, gen), }, + "background", ) } @@ -896,35 +934,16 @@ class McpSupervisorImpl { "Server does not support dynamic client registration. Please provide clientId in config." log.warn("mcp server requires pre-registered client", { key: handle.name, transport: transportName }) } else { - const authEntry = await McpAuth.get(handle.name) - const codeVerifier = authEntry?.codeVerifier - const oauthState = authEntry?.oauthState - const registered = await PendingOAuth.register( - handle.name, - { - client: candidateClient, - transport, - identity: handle.identity, - onDispose: async () => { - await Promise.all([ - codeVerifier === undefined - ? undefined - : McpAuth.clearCodeVerifier(handle.name, codeVerifier).catch(() => undefined), - oauthState === undefined - ? undefined - : McpAuth.clearOAuthState(handle.name, oauthState).catch(() => undefined), - ]) - }, - }, - { isCurrent: () => this.isCurrent(handle, gen) }, - ) - if (!registered) return + await closeFailedClient(candidateClient, handle.name, `connect:${transportName}:auth`) + if (!this.isCurrent(handle, gen)) return handle.state = HS.NeedsAuth + handle.lastError = "Server requires OAuth authentication. Run: synergy mcp auth " + handle.name log.warn("mcp server requires authentication", { key: handle.name, transport: transportName, command: `synergy mcp auth ${handle.name}`, }) + this.scheduleNeedsAuthCheck() } return } diff --git a/packages/synergy/test/mcp/oauth.test.ts b/packages/synergy/test/mcp/oauth.test.ts index 2f45e9f7f..1234318fd 100644 --- a/packages/synergy/test/mcp/oauth.test.ts +++ b/packages/synergy/test/mcp/oauth.test.ts @@ -11,6 +11,7 @@ import { startForPlugin } from "../../src/plugin/mcp" import { ScopeContext } from "../../src/scope/context" import { tmpdir } from "../fixture/fixture" import { Log } from "../../src/util/log" +import { createOAuthMcpServerFixture } from "../fixture/oauth-mcp-server" Log.init({ print: false }) @@ -689,3 +690,149 @@ describe.serial("McpOAuthCallback", () => { await expect(promise2).rejects.toThrow("OAuth callback server stopped") }) }) + +describe.serial("MCP OAuth race and recovery", () => { + let backup: string | undefined + + beforeEach(async () => { + McpAuth.invalidateCache() + const file = Bun.file(Global.Path.authMcp) + const exists = await file.exists() + backup = exists ? await file.text() : undefined + await Bun.write(Global.Path.authMcp, "{}") + }) + + afterEach(async () => { + if (backup !== undefined) { + await Bun.write(Global.Path.authMcp, backup) + } else { + await Bun.write(Global.Path.authMcp, "{}") + } + McpAuth.invalidateCache() + await MCP.stop() + }) + + test("background 401 during an interactive OAuth flow does not cancel the pending callback", async () => { + await using tmp = await tmpdir({ config: {} }) + await using fixture = createOAuthMcpServerFixture() + await ScopeContext.provide({ + scope: await tmp.scope(), + fn: async () => { + const name = "race-server" + const handle = McpSupervisor.add(name, { + type: "remote", + url: fixture.url, + oauth: { scope: "mcp:connect" }, + startup: "manual", + }) + + const { authorizationUrl } = await MCP.startAuth(name) + expect(authorizationUrl).not.toBe("") + const pendingBefore = PendingOAuth.get(name) + expect(pendingBefore).toBeDefined() + + // The background supervisor reconnects while the interactive flow waits for the callback. + await McpSupervisor.connect(name, handle.identity) + expect(PendingOAuth.get(name)).toBe(pendingBefore) + expect((await MCP.status())[name]).toEqual({ status: "needs_auth" }) + + // The interactive flow still completes end-to-end. + const oauthState = await McpAuth.getOAuthState(name) + expect(oauthState).toBeDefined() + const callbackPromise = McpOAuthCallback.waitForCallback(oauthState!, name) + const { redirectUrl } = await fixture.followAuthorization(authorizationUrl) + const redirect = new URL(redirectUrl) + const code = redirect.searchParams.get("code") + expect(code).not.toBeNull() + const response = McpOAuthCallback.handleRequest( + new Request( + `http://127.0.0.1:${getOAuthCallbackPort()}${OAUTH_CALLBACK_PATH}?code=${encodeURIComponent(code!)}&state=${encodeURIComponent(redirect.searchParams.get("state")!)}`, + ), + ) + expect(response.status).toBe(200) + await expect(callbackPromise).resolves.toBe(code!) + + const status = await MCP.finishAuth(name, code!) + expect(status.status).toBe("connected") + }, + }) + }) + + test("background provider never persists OAuth state and reads tokens live", async () => { + const provider = new McpOAuthProvider( + "bg-server", + "https://mcp.example.com", + {}, + { onRedirect: async () => {} }, + "background", + ) + + await provider.saveState("bg-state") + await provider.saveCodeVerifier("bg-verifier") + await provider.saveTokens({ access_token: "bg-token", token_type: "Bearer" }) + await provider.saveClientInformation({ + client_id: "bg-client", + client_secret: undefined, + client_id_issued_at: 1, + client_secret_expires_at: undefined, + redirect_uris: [], + grant_types: [], + response_types: [], + token_endpoint_auth_method: "none", + }) + + expect(await McpAuth.get("bg-server")).toBeUndefined() + expect(await provider.state()).toBe("bg-state") + expect(await provider.codeVerifier()).toBe("bg-verifier") + + // A token written by another process (e.g. the CLI) is visible immediately. + await McpAuth.set("bg-server", { tokens: { accessToken: "ext-token" } }, "https://mcp.example.com") + const tokens = await provider.tokens() + expect(tokens?.access_token).toBe("ext-token") + }) + + test("NeedsAuth recovers automatically once credentials appear", async () => { + await using tmp = await tmpdir({ config: {} }) + await using fixture = createOAuthMcpServerFixture() + await ScopeContext.provide({ + scope: await tmp.scope(), + fn: async () => { + const name = "recover-server" + const handle = McpSupervisor.add(name, { + type: "remote", + url: fixture.url, + oauth: { scope: "mcp:connect" }, + startup: "manual", + }) + + await McpSupervisor.connect(name, handle.identity) + expect((await MCP.status())[name]).toEqual({ status: "needs_auth" }) + + // No credentials yet: a check pass does not reconnect. + await McpSupervisor.checkNeedsAuthNow() + expect((await MCP.status())[name]).toEqual({ status: "needs_auth" }) + + // Another process writes credentials; the next check reconnects automatically. + await McpAuth.set( + name, + { tokens: { accessToken: "fixture-access-token", expiresAt: Date.now() / 1000 + 3600 } }, + fixture.url, + ) + await McpSupervisor.checkNeedsAuthNow() + await handle.startPromise + expect((await MCP.status())[name]).toEqual({ status: "connected" }) + }, + }) + }) + + test("callback port conflict produces an actionable error", async () => { + await MCP.stop() + const port = getOAuthCallbackPort() + const blocker = Bun.serve({ port, fetch: () => new Response("occupied") }) + try { + await expect(McpOAuthCallback.ensureRunning()).rejects.toThrow("SYNERGY_OAUTH_CALLBACK_PORT") + } finally { + blocker.stop(true) + } + }) +}) From b370b7ff6b517f6f85db22dd38cbcc702314fc83 Mon Sep 17 00:00:00 2001 From: EricSanchez Date: Mon, 31 Aug 2026 18:37:48 +0800 Subject: [PATCH 2/3] test(plugin): background 401 no longer registers a pending OAuth owner The supervisor's background connect path now transitions a server to needs_auth without creating a PendingOAuth entry; only the interactive startAuth flow registers one. Update the declarative plugin OAuth integration assertion to match. Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com> --- packages/synergy/test/plugin/mcp-declarative-oauth.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/synergy/test/plugin/mcp-declarative-oauth.test.ts b/packages/synergy/test/plugin/mcp-declarative-oauth.test.ts index 4e1c30c98..2fa747838 100644 --- a/packages/synergy/test/plugin/mcp-declarative-oauth.test.ts +++ b/packages/synergy/test/plugin/mcp-declarative-oauth.test.ts @@ -149,7 +149,9 @@ export default definePlugin({ await handle!.startPromise expect((await MCP.status())[SERVER_NAME]).toEqual({ status: "needs_auth" }) - expect(PendingOAuth.get(SERVER_NAME)?.identity).toBe(handle!.identity) + // Background connects never register a PendingOAuth owner; only the + // interactive startAuth flow does, so nothing is pending yet. + expect(PendingOAuth.get(SERVER_NAME)).toBeUndefined() const registration = fixture.snapshot().registrations.at(-1) expect(registration).toEqual( From 39d5c9096397c4c29ebfcff103249481610c9eb4 Mon Sep 17 00:00:00 2001 From: yzxoi Date: Mon, 31 Aug 2026 19:23:34 +0800 Subject: [PATCH 3/3] fix(mcp): restore background refresh-token renewal and surface needs_auth error Address review findings on PR #1292: - Background saveTokens now persists when a stored entry exists, so SDK refresh-token renewal survives instead of being discarded (probe-only connects still never write shared state). - NeedsAuth recovery treats expired-but-refreshable entries as reconnectable, restoring self-healing for long-running daemons. - needs_auth status now carries the actionable error (mirrors needs_client_registration); SDK/OpenAPI regenerated to match. - Remove the dead invalidateCache no-op shim and its test call sites. - Fixture supports refresh_token grant; new regression tests cover token persistence and expired-refreshable recovery. - Declarative plugin test now asserts background 401 no longer creates a PendingOAuth entry (was stale pre-PR behavior). Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com> --- ...026-08-31-mcp-oauth-ownership-isolation.md | 7 +- docs/reference/configuration-layout.md | 2 +- .../session/session-connection-stats.test.ts | 2 + packages/sdk/js/src/gen/types.gen.ts | 5 ++ packages/sdk/openapi.json | 9 ++- packages/synergy/src/cli/cmd/mcp.ts | 1 + packages/synergy/src/mcp/auth.ts | 3 - packages/synergy/src/mcp/oauth-provider.ts | 7 +- packages/synergy/src/mcp/supervisor.ts | 7 +- .../synergy/test/fixture/oauth-mcp-server.ts | 23 +++++- packages/synergy/test/mcp/auth.test.ts | 1 - packages/synergy/test/mcp/oauth.test.ts | 75 +++++++++++++++++-- .../test/plugin/mcp-declarative-oauth.test.ts | 7 +- 13 files changed, 122 insertions(+), 27 deletions(-) diff --git a/docs/decisions/implemented/bug-fix/2026-08-31-mcp-oauth-ownership-isolation.md b/docs/decisions/implemented/bug-fix/2026-08-31-mcp-oauth-ownership-isolation.md index 326105654..f5e7762f8 100644 --- a/docs/decisions/implemented/bug-fix/2026-08-31-mcp-oauth-ownership-isolation.md +++ b/docs/decisions/implemented/bug-fix/2026-08-31-mcp-oauth-ownership-isolation.md @@ -19,11 +19,12 @@ Additional gaps found along the same ownership line: Keep the CLI direct-connect architecture and eliminate the race by separating background probing from interactive ownership: -- **R1 — `McpAuth` reads go to disk every time.** Removed the module cache; `all()` reads `Global.Path.authMcp` directly. `invalidateCache()` remains as a no-op for test compatibility. CLI-written tokens are visible to the server process on its next read. -- **R2 — `McpOAuthProvider` gained a `mode: "interactive" | "background"` (default `interactive`).** In background mode, `saveCodeVerifier`, `saveState`, `saveClientInformation`, and `saveTokens` are no-ops, and `codeVerifier()`/`state()` return provider-local memory values, so a background probe never writes the shared `authMcp` file. `tokens()`/`clientInformation()` still read live from disk so an already-authenticated server connects directly. -- **R3 — supervisor background connects never touch `PendingOAuth`.** Removed the `disposeIfIdentity` call at the top of `connectPipeline`; the 401 branch now closes the failed client and transitions the handle to `NeedsAuth` (with an actionable `lastError`) instead of registering a pending entry. A single `setInterval` (30 s, `unref()`-ed, lazily started when a handle enters `NeedsAuth`, cleared when none remain) checks each `NeedsAuth` handle: reads `McpAuth.getForUrl` locally, reconnects only when valid tokens exist, skips the handle while an interactive `PendingOAuth` entry exists, and never issues network calls without local credentials. `reset()` clears the timer. `checkNeedsAuthNow()` exposes one pass for tests. +- **R1 — `McpAuth` reads go to disk every time.** Removed the module cache (and the now-dead `invalidateCache()` shim); `all()` reads `Global.Path.authMcp` directly. CLI-written tokens are visible to the server process on its next read. +- **R2 — `McpOAuthProvider` gained a `mode: "interactive" | "background"` (default `interactive`).** In background mode, `saveCodeVerifier`, `saveState`, and `saveClientInformation` are no-ops and `codeVerifier()`/`state()` return provider-local memory values, so a background probe never writes PKCE state into the shared `authMcp` file. `saveTokens` is also a no-op while no stored entry exists (probe-only), but persists when the store already has a token entry so SDK refresh-token renewal survives; `tokens()`/`clientInformation()` always read live from disk so an already-authenticated server connects directly. +- **R3 — supervisor background connects never touch `PendingOAuth`.** Removed the `disposeIfIdentity` call at the top of `connectPipeline`; the 401 branch now closes the failed client and transitions the handle to `NeedsAuth` (with an actionable `lastError`) instead of registering a pending entry. A single `setInterval` (30 s, `unref()`-ed, lazily started when a handle enters `NeedsAuth`, cleared when none remain) checks each `NeedsAuth` handle: reads `McpAuth.getForUrl` locally, reconnects when the stored tokens are valid or expired-but-refreshable (has a refresh token), skips the handle while an interactive `PendingOAuth` entry exists, and never issues network calls without local credentials. `reset()` clears the timer. `checkNeedsAuthNow()` exposes one pass for tests. - **R4 — observability.** `cancelPending` logs a WARN with `{ mcpName, reason }` before rejecting; the callback-port-in-use error names `SYNERGY_OAUTH_CALLBACK_PORT` as the escape hatch; the CLI `auth` handler writes `mcp auth failed` to the file log in every failure branch. - **R5 — behavioral regression tests** in `test/mcp/oauth.test.ts`: a background 401 during an interactive wait no longer cancels the pending callback and the interactive flow completes end-to-end; background provider never persists state and sees externally written tokens immediately; `NeedsAuth` recovers automatically once credentials appear; callback-port conflict produces an actionable error. +- **R6 — follow-up fixes from review.** `needs_auth` status now carries an `error` field (mirroring `needs_client_registration`) so the actionable `synergy mcp auth` hint reaches the CLI status line and any status consumer, not just the file log; the SDK/OpenAPI contracts were regenerated to match; the background-provider token-persistence and expired-but-refreshable recovery behaviors above are covered by new tests; the `invalidateCache` no-op shim was removed along with its test call sites. Behavioral coverage: `test/mcp/oauth.test.ts` (new "MCP OAuth race and recovery" describe) plus the existing `pending-oauth.test.ts`/`supervisor.test.ts` suites staying green unchanged. diff --git a/docs/reference/configuration-layout.md b/docs/reference/configuration-layout.md index 73fdc5a1c..75f3aa33a 100644 --- a/docs/reference/configuration-layout.md +++ b/docs/reference/configuration-layout.md @@ -300,7 +300,7 @@ Remote MCP servers (`type: "remote"` in `40-mcp.jsonc`) use OAuth 2.0 authorizat Authenticate a server with `synergy mcp auth ` (or `synergy mcp auth` to pick from a list). The command opens the provider's authorization page in a browser, waits for the local callback, exchanges the code for tokens, and saves them to the auth store. A long-running server process picks up CLI-authenticated credentials automatically within ~30 seconds (the supervisor re-checks servers in the `needs_auth` state on an interval and reconnects when valid tokens appear), so no restart is required after authenticating from the CLI. -Background supervision never writes PKCE state: an unauthenticated server is marked `needs_auth` and left idle (no network probes) until credentials exist. This keeps background auto-connect from interfering with an interactive `mcp auth` flow for the same server. +Background supervision never writes PKCE state: an unauthenticated server is marked `needs_auth` (with an actionable `synergy mcp auth ` error on the status) and left idle (no network probes) until credentials exist. Once credentials exist, the supervisor reconnects within ~30 seconds even if the stored access token already expired, as long as a refresh token is present — refreshed tokens are persisted back to the auth store. This keeps background auto-connect from interfering with an interactive `mcp auth` flow for the same server. `startup` controls when a server connects: `"eager"` (default) connects at runtime start, `"lazy"` connects on first tool use, and `"manual"` never auto-connects (use `synergy mcp connect `). Set `"manual"` for a server you only authenticate on demand. diff --git a/packages/app/test/components/session/session-connection-stats.test.ts b/packages/app/test/components/session/session-connection-stats.test.ts index ced099d51..af51e293b 100644 --- a/packages/app/test/components/session/session-connection-stats.test.ts +++ b/packages/app/test/components/session/session-connection-stats.test.ts @@ -59,6 +59,8 @@ function mcp(status: McpStatus["status"]): McpStatus { return { status, attempt: 1, maxAttempts: 3 } case "needs_client_registration": return { status, error: "register" } + case "needs_auth": + return { status, error: "auth" } default: return { status } } diff --git a/packages/sdk/js/src/gen/types.gen.ts b/packages/sdk/js/src/gen/types.gen.ts index f97a03ac8..24a8fb9ec 100644 --- a/packages/sdk/js/src/gen/types.gen.ts +++ b/packages/sdk/js/src/gen/types.gen.ts @@ -2602,6 +2602,10 @@ export type ProviderConfig = { * Enable promptCacheKey for this provider (default false) */ setCacheKey?: boolean + /** + * Merge leading system messages into a single system message for strict OpenAI-compatible endpoints that reject multiple or non-leading system messages (e.g. vLLM Qwen chat templates). Default false. + */ + mergeSystemMessages?: boolean /** * Idle timeout in milliseconds for requests to this provider. Set to false to disable timeout. */ @@ -4474,6 +4478,7 @@ export type McpStatusDisabled = { export type McpStatusNeedsAuth = { status: "needs_auth" + error: string } export type McpStatusNeedsClientRegistration = { diff --git a/packages/sdk/openapi.json b/packages/sdk/openapi.json index 7b7263df6..573092689 100644 --- a/packages/sdk/openapi.json +++ b/packages/sdk/openapi.json @@ -31758,6 +31758,10 @@ "description": "Enable promptCacheKey for this provider (default false)", "type": "boolean" }, + "mergeSystemMessages": { + "description": "Merge leading system messages into a single system message for strict OpenAI-compatible endpoints that reject multiple or non-leading system messages (e.g. vLLM Qwen chat templates). Default false.", + "type": "boolean" + }, "timeout": { "description": "Idle timeout in milliseconds for requests to this provider. Set to false to disable timeout.", "anyOf": [ @@ -35291,9 +35295,12 @@ "status": { "type": "string", "const": "needs_auth" + }, + "error": { + "type": "string" } }, - "required": ["status"] + "required": ["status", "error"] }, "MCPStatusNeedsClientRegistration": { "type": "object", diff --git a/packages/synergy/src/cli/cmd/mcp.ts b/packages/synergy/src/cli/cmd/mcp.ts index cc105c196..583d938eb 100644 --- a/packages/synergy/src/cli/cmd/mcp.ts +++ b/packages/synergy/src/cli/cmd/mcp.ts @@ -98,6 +98,7 @@ export const McpListCommand = cmd({ } else if (status.status === "needs_auth") { statusIcon = "⚠" statusText = "needs authentication" + hint = "\n " + status.error } else if (status.status === "needs_client_registration") { statusIcon = "✗" statusText = "needs client registration" diff --git a/packages/synergy/src/mcp/auth.ts b/packages/synergy/src/mcp/auth.ts index e7cbce7b1..ae03cc66e 100644 --- a/packages/synergy/src/mcp/auth.ts +++ b/packages/synergy/src/mcp/auth.ts @@ -60,9 +60,6 @@ export namespace McpAuth { }) } - /** Kept as a no-op for compatibility; reads always go to disk. */ - export function invalidateCache() {} - export async function all(): Promise> { const file = Bun.file(Global.Path.authMcp) const data = (await file.json().catch(() => ({}))) as Record diff --git a/packages/synergy/src/mcp/oauth-provider.ts b/packages/synergy/src/mcp/oauth-provider.ts index e946eccad..8d8c5f7a0 100644 --- a/packages/synergy/src/mcp/oauth-provider.ts +++ b/packages/synergy/src/mcp/oauth-provider.ts @@ -133,7 +133,12 @@ export class McpOAuthProvider implements OAuthClientProvider { } async saveTokens(tokens: OAuthTokens): Promise { - if (this.mode === "background") return + if (this.mode === "background") { + // Persist refreshed tokens so later requests send the new access token. + // Probe-only connects (no stored entry) still never write shared state. + const existing = await McpAuth.getForUrl(this.mcpName, this.serverUrl) + if (!existing) return + } await McpAuth.updateTokens( this.mcpName, { diff --git a/packages/synergy/src/mcp/supervisor.ts b/packages/synergy/src/mcp/supervisor.ts index b71b0051e..d5692a1c4 100644 --- a/packages/synergy/src/mcp/supervisor.ts +++ b/packages/synergy/src/mcp/supervisor.ts @@ -116,7 +116,7 @@ export const Status = z .meta({ ref: "MCPStatusReconnecting" }), z.object({ status: z.literal("failed"), error: z.string() }).meta({ ref: "MCPStatusFailed" }), z.object({ status: z.literal("disabled") }).meta({ ref: "MCPStatusDisabled" }), - z.object({ status: z.literal("needs_auth") }).meta({ ref: "MCPStatusNeedsAuth" }), + z.object({ status: z.literal("needs_auth"), error: z.string() }).meta({ ref: "MCPStatusNeedsAuth" }), z .object({ status: z.literal("needs_client_registration"), error: z.string() }) .meta({ ref: "MCPStatusNeedsClientRegistration" }), @@ -358,7 +358,7 @@ export function mapStatus(handle: McpHandle): Status { case HS.Disabled: return { status: "disabled" } case HS.NeedsAuth: - return { status: "needs_auth" } + return { status: "needs_auth", error: handle.lastError ?? "" } case HS.NeedsClientRegistration: return { status: "needs_client_registration", error: handle.lastError ?? "" } case HS.Stopping: @@ -839,7 +839,8 @@ class McpSupervisorImpl { // A running interactive OAuth flow owns this server; do not probe or reconnect. if (PendingOAuth.get(handle.name)) continue const entry = await McpAuth.getForUrl(handle.name, handle.config.url) - const valid = entry?.tokens && (!entry.tokens.expiresAt || entry.tokens.expiresAt > Date.now() / 1000) + const tokens = entry?.tokens + const valid = !!tokens && (!tokens.expiresAt || tokens.expiresAt > Date.now() / 1000 || !!tokens.refreshToken) if (!valid) continue log.info("oauth credentials found, reconnecting", { key: handle.name }) this.scheduleStart(handle) diff --git a/packages/synergy/test/fixture/oauth-mcp-server.ts b/packages/synergy/test/fixture/oauth-mcp-server.ts index a95838731..1192a81ea 100644 --- a/packages/synergy/test/fixture/oauth-mcp-server.ts +++ b/packages/synergy/test/fixture/oauth-mcp-server.ts @@ -8,6 +8,8 @@ const RESOURCE_URI = "fixture://figma/design" as const const CLIENT_ID = "fixture-client" const AUTHORIZATION_CODE = "fixture-authorization-code" const ACCESS_TOKEN = "fixture-access-token" +export const REFRESH_TOKEN = "fixture-refresh-token" +export const REFRESHED_ACCESS_TOKEN = "fixture-access-token-refreshed" export interface OAuthRegistrationObservation { readonly clientId: string @@ -123,7 +125,7 @@ export function createOAuthMcpServerFixture(): OAuthMcpServerFixture { token_endpoint: `${origin}/token`, registration_endpoint: `${origin}/register`, response_types_supported: ["code"], - grant_types_supported: ["authorization_code"], + grant_types_supported: ["authorization_code", "refresh_token"], token_endpoint_auth_methods_supported: ["none"], code_challenge_methods_supported: ["S256"], }) @@ -183,6 +185,19 @@ export function createOAuthMcpServerFixture(): OAuthMcpServerFixture { if (request.method === "POST" && url.pathname === "/token") { const form = await request.formData() const clientId = required(String(form.get("client_id") ?? ""), "client_id") + const grantType = String(form.get("grant_type") ?? "") + if (grantType === "refresh_token") { + const refreshToken = required(String(form.get("refresh_token") ?? ""), "refresh_token") + if (clientId !== CLIENT_ID || refreshToken !== REFRESH_TOKEN) { + return oauthError("invalid_grant", "Refresh token request failed exact validation") + } + return json({ + access_token: REFRESHED_ACCESS_TOKEN, + token_type: "Bearer", + expires_in: 3600, + scope: SCOPE, + }) + } const code = required(String(form.get("code") ?? ""), "code") const redirectUri = required(String(form.get("redirect_uri") ?? ""), "redirect_uri") const resource = required(String(form.get("resource") ?? ""), "resource") @@ -190,7 +205,7 @@ export function createOAuthMcpServerFixture(): OAuthMcpServerFixture { tokenExchanges.push({ clientId, code, redirectUri, resource, codeVerifier }) const issued = codes.get(code) if ( - form.get("grant_type") !== "authorization_code" || + grantType !== "authorization_code" || clientId !== CLIENT_ID || !issued || issued.redirectUri !== redirectUri || @@ -204,7 +219,9 @@ export function createOAuthMcpServerFixture(): OAuthMcpServerFixture { } if (url.pathname === "/mcp") { - const authorized = request.headers.get("authorization") === `Bearer ${ACCESS_TOKEN}` + const authorized = + request.headers.get("authorization") === `Bearer ${ACCESS_TOKEN}` || + request.headers.get("authorization") === `Bearer ${REFRESHED_ACCESS_TOKEN}` mcpRequests.push({ method: request.method, authorized }) if (!authorized) { return json({ error: "unauthorized" }, 401, { diff --git a/packages/synergy/test/mcp/auth.test.ts b/packages/synergy/test/mcp/auth.test.ts index 4f76f0253..f9f159200 100644 --- a/packages/synergy/test/mcp/auth.test.ts +++ b/packages/synergy/test/mcp/auth.test.ts @@ -10,7 +10,6 @@ describe.serial("McpAuth", () => { let backup: string | undefined beforeEach(async () => { - McpAuth.invalidateCache() const file = Bun.file(Global.Path.authMcp) const exists = await file.exists() backup = exists ? await file.text() : undefined diff --git a/packages/synergy/test/mcp/oauth.test.ts b/packages/synergy/test/mcp/oauth.test.ts index 1234318fd..24006ae82 100644 --- a/packages/synergy/test/mcp/oauth.test.ts +++ b/packages/synergy/test/mcp/oauth.test.ts @@ -11,7 +11,7 @@ import { startForPlugin } from "../../src/plugin/mcp" import { ScopeContext } from "../../src/scope/context" import { tmpdir } from "../fixture/fixture" import { Log } from "../../src/util/log" -import { createOAuthMcpServerFixture } from "../fixture/oauth-mcp-server" +import { REFRESH_TOKEN, REFRESHED_ACCESS_TOKEN, createOAuthMcpServerFixture } from "../fixture/oauth-mcp-server" Log.init({ print: false }) @@ -54,7 +54,6 @@ describe.serial("McpOAuthProvider", () => { let backup: string | undefined beforeEach(async () => { - McpAuth.invalidateCache() const file = Bun.file(Global.Path.authMcp) const exists = await file.exists() backup = exists ? await file.text() : undefined @@ -68,7 +67,6 @@ describe.serial("McpOAuthProvider", () => { } else { await Bun.write(Global.Path.authMcp, "{}") } - McpAuth.invalidateCache() }) function createProvider( @@ -695,7 +693,6 @@ describe.serial("MCP OAuth race and recovery", () => { let backup: string | undefined beforeEach(async () => { - McpAuth.invalidateCache() const file = Bun.file(Global.Path.authMcp) const exists = await file.exists() backup = exists ? await file.text() : undefined @@ -708,7 +705,6 @@ describe.serial("MCP OAuth race and recovery", () => { } else { await Bun.write(Global.Path.authMcp, "{}") } - McpAuth.invalidateCache() await MCP.stop() }) @@ -734,7 +730,10 @@ describe.serial("MCP OAuth race and recovery", () => { // The background supervisor reconnects while the interactive flow waits for the callback. await McpSupervisor.connect(name, handle.identity) expect(PendingOAuth.get(name)).toBe(pendingBefore) - expect((await MCP.status())[name]).toEqual({ status: "needs_auth" }) + expect((await MCP.status())[name]).toMatchObject({ + status: "needs_auth", + error: expect.stringContaining("synergy mcp auth"), + }) // The interactive flow still completes end-to-end. const oauthState = await McpAuth.getOAuthState(name) @@ -791,6 +790,60 @@ describe.serial("MCP OAuth race and recovery", () => { expect(tokens?.access_token).toBe("ext-token") }) + test("background provider persists tokens when a stored entry exists", async () => { + const provider = new McpOAuthProvider( + "bg-persist", + "https://mcp.example.com", + {}, + { onRedirect: async () => {} }, + "background", + ) + await McpAuth.set("bg-persist", { tokens: { accessToken: "old" } }, "https://mcp.example.com") + await provider.saveTokens({ access_token: "new", token_type: "Bearer", refresh_token: "rt" }) + expect((await McpAuth.get("bg-persist"))?.tokens?.accessToken).toBe("new") + }) + + test("NeedsAuth recovers with expired-but-refreshable credentials", async () => { + await using tmp = await tmpdir({ config: {} }) + await using fixture = createOAuthMcpServerFixture() + await ScopeContext.provide({ + scope: await tmp.scope(), + fn: async () => { + const name = "refresh-server" + const handle = McpSupervisor.add(name, { + type: "remote", + url: fixture.url, + oauth: { scope: "mcp:connect" }, + startup: "manual", + }) + + await McpSupervisor.connect(name, handle.identity) + expect((await MCP.status())[name]).toMatchObject({ + status: "needs_auth", + error: expect.stringContaining("synergy mcp auth"), + }) + + // Expired access token, but a refresh token exists: recovery must reconnect. + await McpAuth.set( + name, + { + tokens: { + accessToken: "stale-access-token", + refreshToken: REFRESH_TOKEN, + expiresAt: Date.now() / 1000 - 60, + }, + }, + fixture.url, + ) + await McpSupervisor.checkNeedsAuthNow() + await handle.startPromise + expect((await MCP.status())[name]).toEqual({ status: "connected" }) + // The background provider persisted the refreshed token. + expect((await McpAuth.get(name))?.tokens?.accessToken).toBe(REFRESHED_ACCESS_TOKEN) + }, + }) + }) + test("NeedsAuth recovers automatically once credentials appear", async () => { await using tmp = await tmpdir({ config: {} }) await using fixture = createOAuthMcpServerFixture() @@ -806,11 +859,17 @@ describe.serial("MCP OAuth race and recovery", () => { }) await McpSupervisor.connect(name, handle.identity) - expect((await MCP.status())[name]).toEqual({ status: "needs_auth" }) + expect((await MCP.status())[name]).toMatchObject({ + status: "needs_auth", + error: expect.stringContaining("synergy mcp auth"), + }) // No credentials yet: a check pass does not reconnect. await McpSupervisor.checkNeedsAuthNow() - expect((await MCP.status())[name]).toEqual({ status: "needs_auth" }) + expect((await MCP.status())[name]).toMatchObject({ + status: "needs_auth", + error: expect.stringContaining("synergy mcp auth"), + }) // Another process writes credentials; the next check reconnects automatically. await McpAuth.set( diff --git a/packages/synergy/test/plugin/mcp-declarative-oauth.test.ts b/packages/synergy/test/plugin/mcp-declarative-oauth.test.ts index 2fa747838..8a7c01be7 100644 --- a/packages/synergy/test/plugin/mcp-declarative-oauth.test.ts +++ b/packages/synergy/test/plugin/mcp-declarative-oauth.test.ts @@ -116,7 +116,6 @@ export default definePlugin({ const authBackup = (await authFile.exists()) ? await authFile.text() : undefined await fs.mkdir(path.dirname(Global.Path.authMcp), { recursive: true }) await Bun.write(Global.Path.authMcp, "{}") - McpAuth.invalidateCache() let installed = false let uninstalled = false @@ -148,7 +147,10 @@ export default definePlugin({ expect((await MCP.status())[SERVER_NAME]).toBeDefined() await handle!.startPromise - expect((await MCP.status())[SERVER_NAME]).toEqual({ status: "needs_auth" }) + expect((await MCP.status())[SERVER_NAME]).toMatchObject({ + status: "needs_auth", + error: expect.stringContaining("synergy mcp auth"), + }) // Background connects never register a PendingOAuth owner; only the // interactive startAuth flow does, so nothing is pending yet. expect(PendingOAuth.get(SERVER_NAME)).toBeUndefined() @@ -288,7 +290,6 @@ export default definePlugin({ await PendingOAuth.disposeAll("declarative plugin OAuth test cleanup") if (authBackup === undefined) await Bun.write(Global.Path.authMcp, "{}") else await Bun.write(Global.Path.authMcp, authBackup) - McpAuth.invalidateCache() } }) })