Merge PR #8983: fix(mcp): refresh broker-backed MCP OAuth credentials (@roboomp)

packages/coding-agent/CHANGELOG.md

@@ -15,6 +15,7 @@
 
 - Fixed the `/btw` panel re-committing its frame to native scrollback on every update while the primary turn is still streaming: a live region that pins itself (an anchored HUD/panel such as `/btw`) no longer leaks its scrolled-off rows just because an unpinned transcript seam sits above it in the same frame ([#8793](https://github.com/can1357/oh-my-pi/issues/8793)).
 - Fixed a submitted `/skill:<name>` command staying invisible in the transcript until its awaited preflight (memory recall, `before_agent_start` hooks, auto-thinking classification, pre-prompt compaction) finished, so a slow step such as a Hindsight auto-recall timeout made the command look unaccepted. Idle skill submissions now paint an optimistic row immediately — like a normal prompt — and reconcile it in place when the canonical `message_start` lands ([#8895](https://github.com/can1357/oh-my-pi/issues/8895)).
+- Fixed broker-backed MCP OAuth credentials never refreshing, so remote OAuth MCP servers dropped out of `/mcp` once their access token expired under `omp auth-broker serve`. The client threw on the broker-redacted refresh sentinel instead of asking the broker to refresh, and the broker had no `mcp_oauth:*` refresh path (`POST /v1/credential/:id/refresh` answered `Unknown OAuth provider`). The client now routes redacted MCP refreshes through the broker, and the broker refreshes MCP credentials with a generic `refresh_token` grant from the credential's embedded token endpoint and client id — so the background refresher also keeps MCP tokens live ([#8933](https://github.com/can1357/oh-my-pi/issues/8933)).
 - Fixed Claude Code marketplace plugins ignoring the `enabledPlugins` switch in `~/.claude/settings.json` and `.claude/settings(.local).json`: a plugin turned off for a project no longer loads there, and a local-scope install enabled for a project loads even when its recorded `projectPath` is a different directory
 - Fixed revived subagents (warm lifecycle reviver and cold persisted reviver) rebuilding the session without initializing the extension runtime, leaving every runtime action throwing `ExtensionRuntimeNotInitializedError`. An extension with a `tool_call` handler that touched a runtime action (e.g. `appendEntry`) then tripped the fail-closed gate in `emitToolCall` and blocked every tool — including the hidden `yield` — so the revived agent could neither finish nor exit and looped until killed. Both revivers now call the shared `initializeExtensions` helper, restoring runtime actions, `onError`, and the `session_start` event ([#8824](https://github.com/can1357/oh-my-pi/issues/8824)).
 - Fixed `omp commit` split-commit crashing with a misleading `No diff found for <path>` when a staged binary (or any payload) pushed `git diff --cached --binary` past the 8 MiB subprocess output cap. The capture is truncated silently, so files sorting after the binary vanished from the parsed diff; the split flow now requests a complete diff and fails fast naming the real cause instead ([#8897](https://github.com/can1357/oh-my-pi/issues/8897)).

packages/coding-agent/src/cli/auth-broker-cli.ts

@@ -33,10 +33,14 @@ import {
 	SqliteAuthCredentialStore,
 } from "@oh-my-pi/pi-ai";
 import { AuthBrokerClient, DEFAULT_AUTH_BROKER_BIND, startAuthBroker } from "@oh-my-pi/pi-ai/auth-broker";
+import { refreshOAuthToken } from "@oh-my-pi/pi-ai/oauth";
+import type { OAuthCredentials } from "@oh-my-pi/pi-ai/oauth/types";
 import { $which, APP_NAME, getAgentDbPath, getConfigRootDir, isEnoent, logger, VERSION } from "@oh-my-pi/pi-utils";
 import chalk from "@oh-my-pi/pi-utils/chalk";
 import { setTransports as setLoggerTransports } from "@oh-my-pi/pi-utils/logger";
 import { $ } from "bun";
+import { refreshManagedMcpOAuthCredential } from "../mcp/oauth-credentials";
+import { isManagedMCPOAuthCredentialId, mcpOAuthServerUrlFromCredentialId } from "../mcp/oauth-flow";
 import { resolveAuthBrokerConfig } from "../session/auth-broker-config";
 
 export type AuthBrokerAction = "serve" | "token" | "login" | "logout" | "status" | "import" | "migrate" | "list";
@@ -119,6 +123,34 @@ async function ensureToken(): Promise<string> {
 	return token;
 }
 
+/**
+ * OAuth refresh handler for `omp auth-broker serve`'s {@link AuthStorage}.
+ *
+ * The vault holds provider OAuth rows AND OMP-managed `mcp_oauth:*` rows.
+ * Provider rows refresh through the per-provider registry. MCP rows are
+ * self-describing — the embedded token endpoint and client credentials are the
+ * only refresh material — so they refresh with a generic `refresh_token` grant.
+ * The serve process never loads the MCP manager, so this is the only place that
+ * teaches the broker to refresh MCP tokens; without it
+ * `POST /v1/credential/:id/refresh` fails with "Unknown OAuth provider" and the
+ * background refresher lets MCP access tokens expire (issue #8933).
+ */
+export function refreshBrokerOAuthCredential(
+	provider: string,
+	credential: OAuthCredential,
+	signal?: AbortSignal,
+): Promise<OAuthCredentials> {
+	if (isManagedMCPOAuthCredentialId(provider)) {
+		return refreshManagedMcpOAuthCredential(credential, {
+			serverUrl: mcpOAuthServerUrlFromCredentialId(provider),
+			signal,
+		});
+	}
+	// Non-MCP rows: same per-provider path AuthStorage would take by default
+	// (the serve process registers no custom OAuth providers).
+	return refreshOAuthToken(provider as OAuthProvider, credential);
+}
+
 async function runServe(flags: AuthBrokerCommandArgs["flags"]): Promise<void> {
 	// The broker is a long-running headless service: route structured logs to
 	// stdout so a process supervisor (pm2, journald, k8s) captures them, and
@@ -129,7 +161,10 @@ async function runServe(flags: AuthBrokerCommandArgs["flags"]): Promise<void> {
 	const token = await ensureToken();
 	const dbPath = getAgentDbPath();
 	const store = await SqliteAuthCredentialStore.open(dbPath);
-	const storage = new AuthStorage(store);
+	const storage = new AuthStorage(store, {
+		refreshOAuthCredential: (provider, _credentialId, credential, signal) =>
+			refreshBrokerOAuthCredential(provider, credential, signal),
+	});
 	await storage.reload();
 	const handle = startAuthBroker({
 		storage,

packages/coding-agent/src/mcp/manager.ts

@@ -7,6 +7,7 @@
 import * as path from "node:path";
 import * as url from "node:url";
 import { isDefinitiveOAuthFailure, type TSchema } from "@oh-my-pi/pi-ai";
+import type { OAuthCredentials } from "@oh-my-pi/pi-ai/oauth/types";
 import { logger } from "@oh-my-pi/pi-utils";
 import type { SourceMeta } from "../capability/types";
 import { resolveConfigValue } from "../config/resolve-config-value";
@@ -30,9 +31,10 @@ import { type LoadMCPConfigsResult, loadAllMCPConfigs, validateServerConfig } fr
 import {
 	lookupMcpOAuthCredential,
 	type MCPOAuthCredentialLookup,
+	refreshManagedMcpOAuthCredential,
 	selectMcpOAuthRefreshMaterial,
 } from "./oauth-credentials";
-import { type MCPStoredOAuthCredential, refreshMCPOAuthToken } from "./oauth-flow";
+import type { MCPStoredOAuthCredential } from "./oauth-flow";
 import type { McpConnectionStatusEvent } from "./startup-events";
 import type { MCPToolDetails } from "./tool-bridge";
 import { DeferredMCPTool, MCPTool } from "./tool-bridge";
@@ -1404,6 +1406,36 @@ export class MCPManager {
 		};
 	}
 
+	/**
+	 * Refresh a broker-redacted MCP OAuth credential through the auth-broker.
+	 *
+	 * When running in broker mode the client only ever holds the redacted
+	 * refresh sentinel; the real refresh token lives on the broker. Delegating
+	 * to {@link AuthStorage.forceRefreshCredentialById} makes the broker run the
+	 * `refresh_token` grant and return a fresh access token, which the client
+	 * uses while keeping {@link REMOTE_REFRESH_SENTINEL} in the refresh slot.
+	 */
+	async #refreshBrokeredMcpCredential(credentialId: string, signal?: AbortSignal): Promise<OAuthCredentials> {
+		const storage = this.#authStorage;
+		if (!storage) throw new Error("MCP OAuth broker refresh requires an auth storage");
+		const row = storage.listStoredCredentials(credentialId).find(entry => entry.credential.type === "oauth");
+		if (!row) throw new Error(`No broker credential row for ${credentialId}`);
+		const entry = await storage.forceRefreshCredentialById(row.id, signal);
+		if (entry.credential.type !== "oauth") {
+			throw new Error(`Broker returned non-OAuth credential for ${credentialId}`);
+		}
+		const refreshed = entry.credential;
+		return {
+			access: refreshed.access,
+			refresh: REMOTE_REFRESH_SENTINEL,
+			expires: refreshed.expires,
+			accountId: refreshed.accountId,
+			email: refreshed.email,
+			projectId: refreshed.projectId,
+			enterpriseUrl: refreshed.enterpriseUrl,
+		};
+	}
+
 	/**
 	 * Resolve OAuth credentials and shell commands in config.
 	 * `oauth: false` skips credential injection (reauth's unauthenticated probe);
@@ -1435,24 +1467,15 @@ export class MCPManager {
 							return Boolean(current.refresh && material?.tokenUrl);
 						},
 						refresh: (current, signal) => {
+							// Broker-backed credentials redact the refresh token
+							// (REMOTE_REFRESH_SENTINEL); the broker holds the real one, so
+							// route the refresh through it instead of failing locally.
 							if (current.refresh === REMOTE_REFRESH_SENTINEL) {
-								throw new Error("MCP OAuth refresh token is broker-redacted; local refresh is unavailable");
+								return this.#refreshBrokeredMcpCredential(credentialId, signal);
 							}
-							const material = selectMcpOAuthRefreshMaterial(current, auth);
-							const tokenUrl = material?.tokenUrl;
-							if (!current.refresh || !tokenUrl) {
-								throw new Error("MCP OAuth credential is missing refresh material");
-							}
-							const clientId = material?.clientId;
-							const clientSecret = material?.clientSecret;
-							const authorizationUrl =
-								material && "authorizationUrl" in material ? material.authorizationUrl : undefined;
-							const resourceIsFallback =
-								!material?.resource && (config.type === "http" || config.type === "sse") && Boolean(config.url);
-							const resource = material?.resource ?? (resourceIsFallback ? config.url : undefined);
-							return refreshMCPOAuthToken(tokenUrl, current.refresh, clientId, clientSecret, resource, {
-								authorizationUrl,
-								stripSameOriginResource: resourceIsFallback,
+							return refreshManagedMcpOAuthCredential(current, {
+								serverUrl: config.type === "http" || config.type === "sse" ? config.url : undefined,
+								auth,
 								signal,
 							});
 						},
@@ -1480,10 +1503,8 @@ export class MCPManager {
 							isDefinitiveOAuthFailure(error instanceof Error ? error.message : String(error)),
 						disabledCause: error =>
 							`oauth refresh failed: ${error instanceof Error ? error.message : String(error)}`,
-						keepCredentialOnRefreshFailure: error =>
-							!(error instanceof Error && error.message.includes("broker-redacted")),
+						keepCredentialOnRefreshFailure: true,
 						onRefreshFailure: refreshError => {
-							if (refreshError instanceof Error && refreshError.message.includes("broker-redacted")) return;
 							logger.warn("MCP OAuth refresh failed, using existing token", {
 								credentialId,
 								error: refreshError,

packages/coding-agent/src/mcp/oauth-credentials.ts

@@ -1,3 +1,4 @@
+import type { OAuthCredentials } from "@oh-my-pi/pi-ai/oauth/types";
 import { getActiveProfile } from "@oh-my-pi/pi-utils/dirs";
 import { expandEnvVarsDeep } from "../discovery/helpers";
 import type { AuthStorage } from "../session/auth-storage";
@@ -6,6 +7,7 @@ import {
 	type MCPStoredOAuthCredential,
 	mcpOAuthCredentialId,
 	mcpOAuthCredentialProfile,
+	refreshMCPOAuthToken,
 } from "./oauth-flow";
 import type { MCPAuthConfig, MCPServerConfig } from "./types";
 
@@ -80,6 +82,42 @@ export function selectMcpOAuthRefreshMaterial(
 	return credential.tokenUrl ? credential : auth;
 }
 
+/**
+ * Refresh a stored MCP OAuth credential via the standard `refresh_token` grant.
+ *
+ * Refresh material is taken from the credential itself (self-contained modern
+ * credentials embed `tokenUrl`/`clientId`/`clientSecret`/`resource`) or, for
+ * legacy credentials that carry none, the server's `auth` block. Shared by the
+ * local MCP manager and the `omp auth-broker serve` refresh path so a broker
+ * with no access to the MCP config can still refresh `mcp_oauth:*` credentials
+ * from the vault.
+ *
+ * `serverUrl` supplies the RFC 8707 fallback resource indicator when neither
+ * the credential nor the auth block advertised one; the manager passes the
+ * configured server URL, the broker recovers it from the credential id via
+ * {@link mcpOAuthServerUrlFromCredentialId}.
+ *
+ * @throws when no usable refresh token or token endpoint is available.
+ */
+export function refreshManagedMcpOAuthCredential(
+	credential: MCPStoredOAuthCredential,
+	opts: { serverUrl?: string; auth?: MCPAuthConfig; signal?: AbortSignal } = {},
+): Promise<OAuthCredentials> {
+	const material = selectMcpOAuthRefreshMaterial(credential, opts.auth);
+	const tokenUrl = material?.tokenUrl;
+	if (!credential.refresh || !tokenUrl) {
+		throw new Error("MCP OAuth credential is missing refresh material");
+	}
+	const authorizationUrl = material && "authorizationUrl" in material ? material.authorizationUrl : undefined;
+	const resourceIsFallback = !material?.resource && Boolean(opts.serverUrl);
+	const resource = material?.resource ?? (resourceIsFallback ? opts.serverUrl : undefined);
+	return refreshMCPOAuthToken(tokenUrl, credential.refresh, material?.clientId, material?.clientSecret, resource, {
+		authorizationUrl,
+		stripSameOriginResource: resourceIsFallback,
+		signal: opts.signal,
+	});
+}
+
 export async function removeManagedMcpOAuthCredential(
 	authStorage: AuthStorage,
 	credentialId: string | undefined,

packages/coding-agent/src/mcp/oauth-flow.ts

@@ -53,6 +53,27 @@ export function mcpOAuthCredentialProfile(credentialId: string): string | undefi
 	return separator === -1 ? undefined : credentialId.slice(MCP_OAUTH_PROFILE_CREDENTIAL_PREFIX.length, separator);
 }
 
+/**
+ * Server URL embedded in a managed MCP OAuth credential id, or `undefined`
+ * for legacy random ids (`mcp_oauth_<rand>`) minted before URL-keyed ids.
+ *
+ * Inverse of {@link mcpOAuthCredentialId}. Mirrors {@link mcpOAuthCredentialProfile}:
+ * the URL contains `:` and `/`, so for profile-scoped ids the URL is everything
+ * after the profile segment; for legacy url-keyed ids (`mcp_oauth:<url>`) it is
+ * everything after the prefix. Lets the auth-broker — which never sees the MCP
+ * config — recover the server URL for the RFC 8707 fallback resource on refresh.
+ */
+export function mcpOAuthServerUrlFromCredentialId(credentialId: string): string | undefined {
+	if (credentialId.startsWith(MCP_OAUTH_PROFILE_CREDENTIAL_PREFIX)) {
+		const separator = credentialId.indexOf(":", MCP_OAUTH_PROFILE_CREDENTIAL_PREFIX.length);
+		return separator === -1 ? undefined : credentialId.slice(separator + 1) || undefined;
+	}
+	if (credentialId.startsWith(MCP_OAUTH_URL_CREDENTIAL_PREFIX)) {
+		return credentialId.slice(MCP_OAUTH_URL_CREDENTIAL_PREFIX.length) || undefined;
+	}
+	return undefined;
+}
+
 /**
  * Stored MCP OAuth credential. Refresh material is embedded so token refresh
  * works without any `auth` block persisted in (possibly shared) config files.

packages/coding-agent/test/mcp-broker-oauth-refresh.test.ts

@@ -0,0 +1,133 @@
+/**
+ * End-to-end regression for broker-backed MCP OAuth refresh (issue #8933).
+ *
+ * Topology mirrors `omp auth-broker serve` fronting a sandboxed client:
+ *   client (RemoteAuthCredentialStore) → broker (SqliteAuthCredentialStore
+ *   + refreshBrokerOAuthCredential override) → MCP token endpoint.
+ *
+ * Two gaps used to break this once the ~6h access token expired:
+ *   A. the client threw on the `__remote__` refresh sentinel instead of asking
+ *      the broker to refresh (mcp/manager.ts);
+ *   B. the broker had no `mcp_oauth:*` refresh path, so it answered
+ *      `Unknown OAuth provider` (auth-broker-cli.ts / auth-storage.ts).
+ *
+ * The test proves the fixed contract: a remote OAuth MCP server whose access
+ * token has expired refreshes through the broker (which holds the only real
+ * refresh token) and the client injects the freshly minted Bearer.
+ */
+import { afterEach, beforeEach, describe, expect, test } from "bun:test";
+import * as fs from "node:fs/promises";
+import * as os from "node:os";
+import * as path from "node:path";
+import { AuthStorage, type OAuthCredential, REMOTE_REFRESH_SENTINEL, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai";
+import {
+	AuthBrokerClient,
+	type AuthBrokerServerHandle,
+	RemoteAuthCredentialStore,
+	startAuthBroker,
+} from "@oh-my-pi/pi-ai/auth-broker";
+import { refreshBrokerOAuthCredential } from "@oh-my-pi/pi-coding-agent/cli/auth-broker-cli";
+import { MCPManager } from "@oh-my-pi/pi-coding-agent/mcp/manager";
+import { mcpOAuthCredentialId } from "@oh-my-pi/pi-coding-agent/mcp/oauth-flow";
+import type { MCPServerConfig } from "@oh-my-pi/pi-coding-agent/mcp/types";
+import { removeWithRetries } from "@oh-my-pi/pi-utils";
+import type { Server } from "bun";
+
+const SERVER_URL = "https://mcp.granola.ai/mcp";
+const MCP_PROVIDER = mcpOAuthCredentialId(SERVER_URL, "default");
+const BEARER = "e2e-broker-mcp-token";
+
+function getAuthorizationHeader(config: MCPServerConfig): string | undefined {
+	if (config.type !== "http" && config.type !== "sse") return undefined;
+	return config.headers?.Authorization;
+}
+
+describe("broker-backed MCP OAuth refresh", () => {
+	let tempDir = "";
+	let tokenServer: Server<undefined> | undefined;
+	let tokenRequests: URLSearchParams[] = [];
+	let serverStore: SqliteAuthCredentialStore | undefined;
+	let serverStorage: AuthStorage | undefined;
+	let handle: AuthBrokerServerHandle | undefined;
+	let remote: RemoteAuthCredentialStore | undefined;
+	let clientStorage: AuthStorage | undefined;
+	let manager: MCPManager | undefined;
+
+	beforeEach(async () => {
+		tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "omp-broker-mcp-refresh-"));
+		tokenRequests = [];
+		const server = Bun.serve({
+			port: 0,
+			async fetch(req) {
+				tokenRequests.push(new URLSearchParams(await req.text()));
+				return Response.json({ access_token: "fresh-access", refresh_token: "rotated-refresh", expires_in: 3600 });
+			},
+		});
+		tokenServer = server;
+		const tokenUrl = `http://127.0.0.1:${server.port}/token`;
+
+		serverStore = await SqliteAuthCredentialStore.open(path.join(tempDir, "broker.db"));
+		// The serve process constructs AuthStorage with this exact override.
+		serverStorage = new AuthStorage(serverStore, {
+			refreshOAuthCredential: (provider, _credentialId, credential, signal) =>
+				refreshBrokerOAuthCredential(provider, credential, signal),
+		});
+		await serverStorage.reload();
+
+		// Expired MCP OAuth credential with embedded refresh material, as the
+		// vault holds it. Spread bypasses the excess-property check for the
+		// MCP-only extension fields the base OAuthCredential type omits.
+		const credential: OAuthCredential = {
+			...{ type: "oauth", access: "stale-access", refresh: "real-refresh-token", expires: Date.now() - 60_000 },
+			...{ tokenUrl, clientId: "client-xyz" },
+		};
+		await serverStorage.set(MCP_PROVIDER, credential);
+
+		handle = startAuthBroker({
+			storage: serverStorage,
+			bind: "127.0.0.1:0",
+			bearerTokens: [BEARER],
+			disableRefresher: true,
+		});
+
+		remote = new RemoteAuthCredentialStore({
+			client: new AuthBrokerClient({ url: handle.url, token: BEARER }),
+			streamSnapshots: false,
+		});
+		clientStorage = new AuthStorage(remote);
+		await clientStorage.revalidateCredentials();
+
+		manager = new MCPManager(process.cwd());
+		manager.setAuthStorage(clientStorage);
+	});
+
+	afterEach(async () => {
+		clientStorage?.close();
+		await handle?.close();
+		serverStorage?.close();
+		serverStore?.close();
+		tokenServer?.stop(true);
+		await removeWithRetries(tempDir);
+	});
+
+	test("expired remote MCP token refreshes through the broker and injects the fresh Bearer", async () => {
+		// Sanity: the client only ever sees the redacted refresh token.
+		const stored = clientStorage!.get(MCP_PROVIDER);
+		expect(stored?.type === "oauth" ? stored.refresh : undefined).toBe(REMOTE_REFRESH_SENTINEL);
+
+		const prepared = await manager!.prepareConfig({
+			type: "http",
+			url: SERVER_URL,
+			auth: { type: "oauth", credentialId: MCP_PROVIDER },
+		});
+
+		// Gap A + B fixed: fresh access token minted and injected.
+		expect(getAuthorizationHeader(prepared)).toBe("Bearer fresh-access");
+
+		// The grant ran on the BROKER with the real refresh token — the client
+		// never held it, and the broker no longer answers "Unknown OAuth provider".
+		expect(tokenRequests).toHaveLength(1);
+		expect(tokenRequests[0].get("grant_type")).toBe("refresh_token");
+		expect(tokenRequests[0].get("refresh_token")).toBe("real-refresh-token");
+	});
+});