diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 0285152dc..cc2a044b0 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/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:` 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 ` 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)). diff --git a/packages/coding-agent/src/cli/auth-broker-cli.ts b/packages/coding-agent/src/cli/auth-broker-cli.ts index f7b80602f..78b8b06f9 100644 --- a/packages/coding-agent/src/cli/auth-broker-cli.ts +++ b/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 { 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 { + 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 { // 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 { 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, diff --git a/packages/coding-agent/src/mcp/manager.ts b/packages/coding-agent/src/mcp/manager.ts index 8775464cb..7ccfed5f0 100644 --- a/packages/coding-agent/src/mcp/manager.ts +++ b/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 { + 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, diff --git a/packages/coding-agent/src/mcp/oauth-credentials.ts b/packages/coding-agent/src/mcp/oauth-credentials.ts index 2c78ca276..bcce60b1f 100644 --- a/packages/coding-agent/src/mcp/oauth-credentials.ts +++ b/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 { + 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, diff --git a/packages/coding-agent/src/mcp/oauth-flow.ts b/packages/coding-agent/src/mcp/oauth-flow.ts index e312a395b..838361720 100644 --- a/packages/coding-agent/src/mcp/oauth-flow.ts +++ b/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_`) 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:`) 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. diff --git a/packages/coding-agent/test/mcp-broker-oauth-refresh.test.ts b/packages/coding-agent/test/mcp-broker-oauth-refresh.test.ts new file mode 100644 index 000000000..b7c3a76b8 --- /dev/null +++ b/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; + 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"); + }); +});