From f6b2e2dbc4f3f363f0839865d1036a2893f10086 Mon Sep 17 00:00:00 2001 From: suryaiyer95 Date: Tue, 5 May 2026 11:28:05 -0700 Subject: [PATCH 1/3] fix: [#791][#792] support bearer-auth remote MCP servers (Microsoft Fabric, etc.) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three coupled fixes that together let altimate-code connect to MCP servers gated by short-lived bearer tokens — most prominently Microsoft Fabric Core MCP — without a local proxy or per-hour config edits. Issues addressed: - #791 (dynamic auth for remote MCP servers): static `headers` only. - #792 (external MCP servers not auto-activated at session start): mostly caused by OAuth dynamic-client-registration probes pre-empting valid bearer headers and ending the connect in a non-`connected` status. - A third defect surfaced during this work: the strict `ListToolsResultSchema` from `@modelcontextprotocol/sdk` rejects the `null` annotation hints that Fabric returns, causing `listTools()` to throw and the connection to be marked failed even after a successful initialize. None of these are altimate-specific — they all reproduce on upstream `opencode`. Changes: 1. `McpRemote.headersCommand: Record` — header values produced by running an argv command (executed via `execFile`, not a shell, so values aren't subject to shell injection). Resolved on every connect so expiring tokens refresh automatically. Example: "headersCommand": { "Authorization": ["sh", "-c", "printf 'Bearer %s' \"$(az account get-access-token ... -o tsv)\""] } 2. OAuth provider is no longer attached when the user supplies an explicit `Authorization` header (statically or via `headersCommand`) and `oauth` is not specified. Prevents Entra ID's DCR rejection from short-circuiting a bearer-authenticated connection. Behavior is unchanged when `oauth` is explicitly configured. 3. `listTools()` is wrapped to retry with a permissive Zod schema on schema-validation errors. The fast path is unchanged for compliant servers; non-compliant servers (Fabric returns `null` for `annotations.{readOnlyHint,destructiveHint,idempotentHint,openWorldHint}`) no longer fail the connection. 4. `normalizeMcpConfig` was silently dropping `oauth` and (would have dropped) `headersCommand` when reconstructing remote entries. Both are now passed through. This was a pre-existing latent bug that made `oauth: false` configurations no-op at runtime. Tests: - 22 new tests across `test/mcp/mcp-bearer-auth.test.ts` (lenient schema, `headersCommand` resolution + execFile-not-shell semantics, Authorization detection, schema round-trip) and `test/mcp/headers.test.ts` (OAuth auto-disable on static bearer, on `headersCommand`, and explicit-OAuth override). All pass. - Zero regressions vs `origin/main` baseline on existing 185 MCP tests. - Live-verified end-to-end against `https://api.fabric.microsoft.com/v1/mcp/core` using `headersCommand` + `az account get-access-token`: server connected, 29 tools registered, no proxy required. Co-Authored-By: Claude Opus 4.7 --- packages/opencode/src/config/config.ts | 26 +- packages/opencode/src/mcp/index.ts | 172 +++++++++++- packages/opencode/test/mcp/headers.test.ts | 92 +++++- .../opencode/test/mcp/mcp-bearer-auth.test.ts | 263 ++++++++++++++++++ 4 files changed, 537 insertions(+), 16 deletions(-) create mode 100644 packages/opencode/test/mcp/mcp-bearer-auth.test.ts diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index 736f9215f6..8e31b120ae 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -649,11 +649,26 @@ export namespace Config { url: z.string().describe("URL of the remote MCP server"), enabled: z.boolean().optional().describe("Enable or disable the MCP server on startup"), headers: z.record(z.string(), z.string()).optional().describe("Headers to send with the request"), + // altimate_change start — dynamic header values produced by a shell command, + // resolved on each (re)connect so callers can refresh expiring bearer tokens + // without restarting the session (e.g. `az account get-access-token`). + headersCommand: z + .record(z.string(), z.array(z.string()).nonempty()) + .optional() + .describe( + "Headers whose values are produced by running a command (argv form: [cmd, ...args]). " + + "stdout is trimmed and used as the header value. Resolved on every connect so tokens " + + "with short TTLs (e.g. Microsoft Entra ID bearer tokens for Fabric) refresh automatically. " + + "Values from headersCommand override matching keys in `headers`.", + ), + // altimate_change end oauth: z .union([McpOAuth, z.literal(false)]) .optional() .describe( - "OAuth authentication configuration for the MCP server. Set to false to disable OAuth auto-detection.", + "OAuth authentication configuration for the MCP server. Set to false to disable OAuth auto-detection. " + + "When `headers.Authorization` or `headersCommand.Authorization` is set and `oauth` is not specified, " + + "OAuth is disabled automatically so a static bearer token isn't overridden by a competing OAuth flow.", ), timeout: z .number() @@ -1440,6 +1455,15 @@ export namespace Config { } else if (entry.url && typeof entry.url === "string") { const transformed: Record = { type: "remote", url: entry.url } if (entry.headers && typeof entry.headers === "object") transformed.headers = entry.headers + // altimate_change start — preserve fields that the original normalizer dropped + // silently. Without these passes, a user-supplied `oauth: false` or + // `headersCommand` would be reconstructed-away, leaving the runtime + // believing the config was bare. See #791 / #792. + if (entry.headersCommand && typeof entry.headersCommand === "object") { + transformed.headersCommand = entry.headersCommand + } + if (entry.oauth !== undefined) transformed.oauth = entry.oauth + // altimate_change end if (typeof entry.timeout === "number") transformed.timeout = entry.timeout if (typeof entry.enabled === "boolean") transformed.enabled = entry.enabled servers[name] = transformed diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index b110467cec..668c07d430 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -13,6 +13,12 @@ import { Config } from "../config/config" import { Log } from "../util/log" import { NamedError } from "@opencode-ai/util/error" import z from "zod/v4" +// altimate_change start — needed to resolve `headersCommand` for remote MCP +// servers that require bearer tokens with short TTLs (Microsoft Fabric, etc.) +import { execFile } from "node:child_process" +import { promisify } from "node:util" +const execFileAsync = promisify(execFile) +// altimate_change end import { Instance } from "../project/instance" import { Installation } from "../installation" import { withTimeout } from "@/util/timeout" @@ -121,6 +127,118 @@ export namespace MCP { const toolListCache = new Map() // altimate_change end + // altimate_change start — Microsoft Fabric Core MCP returns `null` for + // `tool.annotations.{readOnlyHint,destructiveHint,idempotentHint,openWorldHint}`, + // which the SDK's `ListToolsResultSchema` (z.boolean().optional()) rejects via + // Zod, blocking listTools() entirely. We accept `null` as "hint absent" by + // calling `client.request()` with a permissive schema in place of the SDK's + // strict one. See https://github.com/AltimateAI/altimate-code/issues/792. + const LenientToolAnnotationsSchema = z + .object({ + title: z.string().optional(), + readOnlyHint: z.boolean().nullable().optional(), + destructiveHint: z.boolean().nullable().optional(), + idempotentHint: z.boolean().nullable().optional(), + openWorldHint: z.boolean().nullable().optional(), + }) + .loose() + + const LenientToolSchema = z + .object({ + name: z.string(), + title: z.string().optional(), + description: z.string().optional(), + inputSchema: z.any(), + outputSchema: z.any().optional(), + annotations: LenientToolAnnotationsSchema.optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) + .loose() + + const LenientListToolsResultSchema = z + .object({ + tools: z.array(LenientToolSchema), + nextCursor: z.string().optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) + .loose() + + function isSchemaError(err: unknown): boolean { + if (!err) return false + if (err instanceof Error && (err.name === "ZodError" || err.constructor?.name === "$ZodError")) return true + if (typeof err === "object" && err !== null && "issues" in err) return true + return false + } + + /** + * Calls the SDK's strict `listTools()` first; on a Zod schema-validation + * failure (e.g. server emits non-spec values like `null` annotation hints), + * retries via `client.request()` with a permissive schema. This keeps the + * fast path unchanged for compliant servers while letting non-compliant + * ones (Microsoft Fabric, etc.) still register their tools. + */ + async function listToolsLenient(client: MCPClient): Promise<{ tools: MCPToolDef[] }> { + try { + return await client.listTools() + } catch (err) { + if (!isSchemaError(err)) throw err + log.info("listTools strict schema rejected response, retrying with lenient schema", { + error: err instanceof Error ? err.message.slice(0, 200) : String(err).slice(0, 200), + }) + const result = await client.request( + { method: "tools/list", params: {} }, + LenientListToolsResultSchema as any, + ) + return result as { tools: MCPToolDef[] } + } + } + + /** @internal — exported only for unit tests. Prefer using `tools()` in production code. */ + export const _testing = { + LenientListToolsResultSchema, + isSchemaError, + resolveHeadersCommand: (spec: Record | undefined, key = "test") => + resolveHeadersCommand(spec, key), + hasAuthorizationHeader, + } + // altimate_change end + + // altimate_change start — resolve dynamic header values produced by shell + // commands (e.g. `az account get-access-token`). Runs via execFile (not a + // shell) so values aren't subject to shell injection. Re-runs on every + // connect so expiring bearer tokens refresh without manual config edits. + // See https://github.com/AltimateAI/altimate-code/issues/791. + async function resolveHeadersCommand( + spec: Record | undefined, + serverKey: string, + ): Promise> { + if (!spec) return {} + const out: Record = {} + for (const [name, argv] of Object.entries(spec)) { + if (!Array.isArray(argv) || argv.length === 0) { + throw new Error(`headersCommand[${name}] must be a non-empty argv array`) + } + const [cmd, ...args] = argv + const { stdout } = await execFileAsync(cmd, args, { + encoding: "utf-8", + maxBuffer: 1024 * 1024, + timeout: 30_000, + }) + const value = stdout.trim() + if (!value) { + throw new Error(`headersCommand[${name}] produced empty output`) + } + log.info("resolved dynamic header", { server: serverKey, header: name }) + out[name] = value + } + return out + } + + function hasAuthorizationHeader(headers: Record): boolean { + return Object.keys(headers).some((k) => k.toLowerCase() === "authorization") + } + // altimate_change end + // Register notification handlers for MCP client function registerNotificationHandlers(client: MCPClient, serverName: string) { client.setNotificationHandler(ToolListChangedNotificationSchema, async () => { @@ -364,8 +482,36 @@ export namespace MCP { let connectedTransport: "stdio" | "sse" | "streamable-http" | undefined = undefined if (mcp.type === "remote") { - // OAuth is enabled by default for remote servers unless explicitly disabled with oauth: false - const oauthDisabled = mcp.oauth === false + // altimate_change start — resolve dynamic headers (e.g. bearer tokens + // produced by `az account get-access-token`) before constructing + // transports. Failure to resolve aborts the connect attempt with a + // clear error so the user sees `failed: headersCommand[...] failed` + // in `mcp list` rather than a generic transport error. + let dynamicHeaders: Record = {} + try { + dynamicHeaders = await resolveHeadersCommand(mcp.headersCommand, key) + } catch (err) { + const message = err instanceof Error ? err.message : String(err) + log.error("headersCommand resolution failed", { key, error: message }) + return { + mcpClient: undefined, + status: { status: "failed" as const, error: `headersCommand failed: ${message}` }, + } + } + const mergedHeaders: Record = { ...(mcp.headers ?? {}), ...dynamicHeaders } + // altimate_change end + + // altimate_change start — OAuth is enabled by default for remote servers, + // BUT if the user provided an explicit Authorization header (statically or + // via headersCommand) and didn't ask for OAuth, skip OAuth so the bearer + // header isn't pre-empted by an OAuth flow that fails (e.g. Microsoft + // Entra ID rejects RFC 7591 dynamic client registration). See #792. + const oauthExplicitlyDisabled = mcp.oauth === false + const oauthExplicitlyConfigured = typeof mcp.oauth === "object" + const oauthDisabled = + oauthExplicitlyDisabled || + (!oauthExplicitlyConfigured && hasAuthorizationHeader(mergedHeaders)) + // altimate_change end const oauthConfig = typeof mcp.oauth === "object" ? mcp.oauth : undefined let authProvider: McpOAuthProvider | undefined @@ -387,22 +533,25 @@ export namespace MCP { ) } + // altimate_change start — pass merged (static + dynamic) headers to transports + const requestInit = Object.keys(mergedHeaders).length > 0 ? { headers: mergedHeaders } : undefined const transports: Array<{ name: string; transport: TransportWithAuth }> = [ { name: "StreamableHTTP", transport: new StreamableHTTPClientTransport(new URL(mcp.url), { authProvider, - requestInit: mcp.headers ? { headers: mcp.headers } : undefined, + requestInit, }), }, { name: "SSE", transport: new SSEClientTransport(new URL(mcp.url), { authProvider, - requestInit: mcp.headers ? { headers: mcp.headers } : undefined, + requestInit, }), }, ] + // altimate_change end let lastError: Error | undefined const connectTimeout = mcp.timeout ?? DEFAULT_TIMEOUT @@ -429,8 +578,10 @@ export namespace MCP { duration_ms: Date.now() - connectStart, }) // altimate_change start — bridge merge: prefetch tool list synchronously - // for cache so MCP.tools() doesn't re-call listTools. - const toolsList = await client.listTools().catch(() => undefined) + // for cache so MCP.tools() doesn't re-call listTools. Use lenient + // schema so servers that emit `null` annotation hints (e.g. Fabric) + // don't trip Zod validation. See #792. + const toolsList = await listToolsLenient(client).catch(() => undefined) if (toolsList) toolListCache.set(key, toolsList.tools) // altimate_change end // Census: collect resource counts (fire-and-forget, never block connect) @@ -567,8 +718,9 @@ export namespace MCP { duration_ms: Date.now() - localConnectStart, }) // altimate_change start — bridge merge: prefetch tool list synchronously - // for cache so MCP.tools() doesn't re-call listTools. - const toolsListSync = await client.listTools().catch(() => undefined) + // for cache so MCP.tools() doesn't re-call listTools. Use lenient schema + // so non-compliant annotation hints (`null`) don't fail validation. + const toolsListSync = await listToolsLenient(client).catch(() => undefined) if (toolsListSync) toolListCache.set(key, toolsListSync.tools) // altimate_change end // Census: collect resource counts (fire-and-forget, never block connect) @@ -635,7 +787,7 @@ export namespace MCP { const cachedTools = toolListCache.get(key) const result = cachedTools ? { tools: cachedTools } - : await withTimeout(mcpClient.listTools(), mcp.timeout ?? DEFAULT_TIMEOUT).catch((err) => { + : await withTimeout(listToolsLenient(mcpClient), mcp.timeout ?? DEFAULT_TIMEOUT).catch((err) => { log.error("failed to get tools from client", { key, error: err }) return undefined }) @@ -784,7 +936,7 @@ export namespace MCP { if (cached) { return { clientName, client, toolsResult: { tools: cached } } } - const toolsResult = await client.listTools().catch((e) => { + const toolsResult = await listToolsLenient(client).catch((e) => { log.error("failed to get tools", { clientName, error: e.message }) const failedStatus = { status: "failed" as const, diff --git a/packages/opencode/test/mcp/headers.test.ts b/packages/opencode/test/mcp/headers.test.ts index 8c488d4c4f..d4fb3f55b2 100644 --- a/packages/opencode/test/mcp/headers.test.ts +++ b/packages/opencode/test/mcp/headers.test.ts @@ -47,7 +47,7 @@ const { MCP } = await import("../../src/mcp/index") const { Instance } = await import("../../src/project/instance") const { tmpdir } = await import("../fixture/fixture") -test("headers are passed to transports when oauth is enabled (default)", async () => { +test("headers are passed to transports when oauth is enabled (default, no Authorization header)", async () => { await using tmp = await tmpdir({ init: async (dir) => { await Bun.write( @@ -59,8 +59,8 @@ test("headers are passed to transports when oauth is enabled (default)", async ( type: "remote", url: "https://example.com/mcp", headers: { - Authorization: "Bearer test-token", "X-Custom-Header": "custom-value", + "X-Trace-Id": "trace-1", }, }, }, @@ -77,8 +77,8 @@ test("headers are passed to transports when oauth is enabled (default)", async ( type: "remote", url: "https://example.com/mcp", headers: { - Authorization: "Bearer test-token", "X-Custom-Header": "custom-value", + "X-Trace-Id": "trace-1", }, }).catch(() => {}) @@ -88,15 +88,97 @@ test("headers are passed to transports when oauth is enabled (default)", async ( for (const call of transportCalls) { expect(call.options.requestInit).toBeDefined() expect(call.options.requestInit?.headers).toEqual({ - Authorization: "Bearer test-token", "X-Custom-Header": "custom-value", + "X-Trace-Id": "trace-1", }) - // OAuth should be enabled by default, so authProvider should exist + // OAuth should be enabled by default when no Authorization header is provided. + expect(call.options.authProvider).toBeDefined() + } + }, + }) +}) + +// altimate_change start — covers the OAuth auto-disable behavior added for +// https://github.com/AltimateAI/altimate-code/issues/792. When the user +// supplies an explicit Authorization header (statically or via headersCommand), +// the OAuth provider is not attached, so a failing OAuth flow (e.g. Microsoft +// Entra ID rejecting RFC 7591 dynamic client registration) cannot pre-empt the +// bearer token. +test("OAuth is auto-disabled when an explicit Authorization header is present", async () => { + await using tmp = await tmpdir() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + transportCalls.length = 0 + await MCP.add("auto-disable-server", { + type: "remote", + url: "https://example.com/mcp", + headers: { + Authorization: "Bearer static-token", + "X-Custom-Header": "x", + }, + }).catch(() => {}) + + expect(transportCalls.length).toBeGreaterThanOrEqual(1) + for (const call of transportCalls) { + expect(call.options.requestInit?.headers).toMatchObject({ + Authorization: "Bearer static-token", + }) + // No authProvider — OAuth was auto-disabled because user provided bearer. + expect(call.options.authProvider).toBeUndefined() + } + }, + }) +}) + +test("OAuth is auto-disabled when Authorization is supplied via headersCommand", async () => { + await using tmp = await tmpdir() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + transportCalls.length = 0 + await MCP.add("auto-disable-cmd-server", { + type: "remote", + url: "https://example.com/mcp", + headersCommand: { + Authorization: ["printf", "Bearer dynamic-token"], + }, + } as any).catch(() => {}) + + expect(transportCalls.length).toBeGreaterThanOrEqual(1) + for (const call of transportCalls) { + expect(call.options.requestInit?.headers).toMatchObject({ + Authorization: "Bearer dynamic-token", + }) + expect(call.options.authProvider).toBeUndefined() + } + }, + }) +}) + +test("OAuth still attaches when Authorization header is present but oauth is explicitly configured", async () => { + await using tmp = await tmpdir() + await Instance.provide({ + directory: tmp.path, + fn: async () => { + transportCalls.length = 0 + await MCP.add("explicit-oauth-server", { + type: "remote", + url: "https://example.com/mcp", + headers: { Authorization: "Bearer fallback" }, + oauth: { clientId: "client-xyz" }, + }).catch(() => {}) + + expect(transportCalls.length).toBeGreaterThanOrEqual(1) + for (const call of transportCalls) { + // User explicitly opted in to OAuth, so provider is attached even + // though a static Authorization header is also present. expect(call.options.authProvider).toBeDefined() } }, }) }) +// altimate_change end test("headers are passed to transports when oauth is explicitly disabled", async () => { await using tmp = await tmpdir() diff --git a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts new file mode 100644 index 0000000000..7da2d70196 --- /dev/null +++ b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts @@ -0,0 +1,263 @@ +import { describe, test, expect } from "bun:test" +import z from "zod/v4" +import { Config } from "../../src/config/config" + +// We replicate the lenient ListTools schema used in mcp/index.ts here so the +// test does not depend on the MCP module's heavy import surface (Instance, +// Telemetry, Bus, Plugin, etc.). The schema is byte-for-byte identical to +// the one in mcp/index.ts; if it drifts, the integration test in mcp.test.ts +// will catch it. +const LenientToolAnnotationsSchema = z + .object({ + title: z.string().optional(), + readOnlyHint: z.boolean().nullable().optional(), + destructiveHint: z.boolean().nullable().optional(), + idempotentHint: z.boolean().nullable().optional(), + openWorldHint: z.boolean().nullable().optional(), + }) + .loose() + +const LenientToolSchema = z + .object({ + name: z.string(), + title: z.string().optional(), + description: z.string().optional(), + inputSchema: z.any(), + outputSchema: z.any().optional(), + annotations: LenientToolAnnotationsSchema.optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) + .loose() + +const LenientListToolsResultSchema = z + .object({ + tools: z.array(LenientToolSchema), + nextCursor: z.string().optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) + .loose() + +// --------------------------------------------------------------------------- +// 1. Lenient tools/list schema accepts what real-world servers emit. +// --------------------------------------------------------------------------- +describe("lenient tools/list schema", () => { + test("accepts null annotation hints (Microsoft Fabric Core MCP behavior)", () => { + // Real payload shape we observed from https://api.fabric.microsoft.com/v1/mcp/core + const fabricStyleResponse = { + tools: [ + { + name: "list_workspaces", + description: "Lists all Microsoft fabric workspaces user has access to.", + inputSchema: { type: "object", properties: {} }, + annotations: { + title: "List Workspaces", + readOnlyHint: true, + destructiveHint: null, + idempotentHint: null, + openWorldHint: null, + }, + }, + ], + } + const result = LenientListToolsResultSchema.safeParse(fabricStyleResponse) + expect(result.success).toBe(true) + if (result.success) { + expect(result.data.tools).toHaveLength(1) + expect(result.data.tools[0].name).toBe("list_workspaces") + } + }) + + test("accepts proper boolean annotation hints (compliant servers)", () => { + const compliantResponse = { + tools: [ + { + name: "delete_workspace", + inputSchema: { type: "object" }, + annotations: { + readOnlyHint: false, + destructiveHint: true, + idempotentHint: true, + openWorldHint: false, + }, + }, + ], + } + const result = LenientListToolsResultSchema.safeParse(compliantResponse) + expect(result.success).toBe(true) + }) + + test("accepts tools without annotations at all", () => { + const result = LenientListToolsResultSchema.safeParse({ + tools: [{ name: "minimal", inputSchema: {} }], + }) + expect(result.success).toBe(true) + }) + + test("rejects malformed top-level (missing tools array)", () => { + expect(LenientListToolsResultSchema.safeParse({ tools: "not-an-array" }).success).toBe(false) + expect(LenientListToolsResultSchema.safeParse({}).success).toBe(false) + }) + + test("preserves unknown fields via .loose() (forward compatibility)", () => { + const future = { + tools: [{ name: "x", inputSchema: {}, futureField: { nested: 1 } }], + futureTopLevel: "ok", + } + const result = LenientListToolsResultSchema.safeParse(future) + expect(result.success).toBe(true) + }) +}) + +// --------------------------------------------------------------------------- +// 2. McpRemote schema accepts new headersCommand field (issue #791). +// --------------------------------------------------------------------------- +describe("McpRemote.headersCommand schema (#791)", () => { + test("accepts headersCommand as record of header → argv", () => { + const config = { + type: "remote" as const, + url: "https://example.com/mcp", + headersCommand: { + Authorization: ["az", "account", "get-access-token", "--query", "accessToken", "-o", "tsv"], + }, + } + const result = Config.McpRemote.safeParse(config) + expect(result.success).toBe(true) + }) + + test("rejects headersCommand with empty argv (would silently no-op at runtime)", () => { + const result = Config.McpRemote.safeParse({ + type: "remote", + url: "https://example.com/mcp", + headersCommand: { Authorization: [] }, + }) + expect(result.success).toBe(false) + }) + + test("allows static headers and headersCommand to coexist", () => { + const config = { + type: "remote" as const, + url: "https://example.com/mcp", + headers: { "X-Trace-Id": "abc" }, + headersCommand: { Authorization: ["echo", "Bearer xyz"] }, + } + const result = Config.McpRemote.safeParse(config) + expect(result.success).toBe(true) + }) + + test("headersCommand is optional (existing configs still validate)", () => { + const result = Config.McpRemote.safeParse({ + type: "remote", + url: "https://example.com/mcp", + headers: { Authorization: "Bearer static" }, + }) + expect(result.success).toBe(true) + }) +}) + +// --------------------------------------------------------------------------- +// 3. headersCommand resolution behavior (#791). +// Tests the actual helper from the MCP module. +// --------------------------------------------------------------------------- +describe("resolveHeadersCommand helper", () => { + test("returns empty object when spec is undefined", async () => { + const { MCP } = await import("../../src/mcp") + const result = await MCP._testing.resolveHeadersCommand(undefined) + expect(result).toEqual({}) + }) + + test("runs argv via execFile and uses trimmed stdout as header value", async () => { + const { MCP } = await import("../../src/mcp") + const result = await MCP._testing.resolveHeadersCommand({ + Authorization: ["printf", "Bearer hello-world"], + "X-Trace": ["printf", "trace-123\n"], + }) + expect(result.Authorization).toBe("Bearer hello-world") + expect(result["X-Trace"]).toBe("trace-123") + }) + + test("throws when command emits empty output", async () => { + const { MCP } = await import("../../src/mcp") + await expect(MCP._testing.resolveHeadersCommand({ Authorization: ["true"] })).rejects.toThrow( + /produced empty output/, + ) + }) + + test("throws when command does not exist", async () => { + const { MCP } = await import("../../src/mcp") + await expect( + MCP._testing.resolveHeadersCommand({ Authorization: ["this-binary-does-not-exist-xyz"] }), + ).rejects.toThrow() + }) + + test("does not invoke a shell (argv is passed directly to execFile)", async () => { + // If a shell were used, the metacharacters below would be interpreted. + // execFile passes argv directly, so the literal string is echoed back. + const { MCP } = await import("../../src/mcp") + const result = await MCP._testing.resolveHeadersCommand({ + X: ["printf", "%s", "$(whoami); rm -rf /"], + }) + expect(result.X).toBe("$(whoami); rm -rf /") + }) +}) + +// --------------------------------------------------------------------------- +// 4. Authorization-header detection used to auto-disable OAuth (#792). +// --------------------------------------------------------------------------- +describe("hasAuthorizationHeader helper (#792)", () => { + test("matches case-insensitively", async () => { + const { MCP } = await import("../../src/mcp") + expect(MCP._testing.hasAuthorizationHeader({ Authorization: "Bearer x" })).toBe(true) + expect(MCP._testing.hasAuthorizationHeader({ authorization: "Bearer x" })).toBe(true) + expect(MCP._testing.hasAuthorizationHeader({ AUTHORIZATION: "Bearer x" })).toBe(true) + }) + + test("returns false when no auth header is present", async () => { + const { MCP } = await import("../../src/mcp") + expect(MCP._testing.hasAuthorizationHeader({})).toBe(false) + expect(MCP._testing.hasAuthorizationHeader({ "X-Trace": "abc" })).toBe(false) + }) + + test("does not match prefixes that merely contain 'authorization'", async () => { + const { MCP } = await import("../../src/mcp") + expect(MCP._testing.hasAuthorizationHeader({ "X-Authorization-Type": "Bearer" })).toBe(false) + expect(MCP._testing.hasAuthorizationHeader({ "Pre-Authorization": "x" })).toBe(false) + }) +}) + +// --------------------------------------------------------------------------- +// 5. normalizeMcpConfig preserves headersCommand and oauth (round-trip). +// +// Without this, the field-stripping normalizer drops user-supplied values +// silently, leaving the runtime to behave as if the user hadn't configured +// them. See #791 / #792. +// --------------------------------------------------------------------------- +describe("config normalize round-trip", () => { + test("McpRemote with headersCommand survives Mcp parse", () => { + // Simulates the post-normalize entry: with our fix, the load path + // forwards `headersCommand` through into the typed shape. + const entry = { + type: "remote", + url: "https://example.com/mcp", + headersCommand: { Authorization: ["echo", "Bearer x"] }, + } + const result = Config.Mcp.safeParse(entry) + expect(result.success).toBe(true) + if (result.success && result.data.type === "remote") { + expect(result.data.headersCommand).toEqual({ Authorization: ["echo", "Bearer x"] }) + } + }) + + test("McpRemote with oauth=false survives Mcp parse", () => { + const entry = { + type: "remote", + url: "https://example.com/mcp", + headers: { Authorization: "Bearer x" }, + oauth: false, + } + const result = Config.Mcp.safeParse(entry) + expect(result.success).toBe(true) + if (result.success && result.data.type === "remote") { + expect(result.data.oauth).toBe(false) + } + }) +}) From 477baab48b4faba56ba133f0b0654328db5fd827 Mon Sep 17 00:00:00 2001 From: suryaiyer95 Date: Tue, 23 Jun 2026 10:30:10 -0700 Subject: [PATCH 2/3] fix: [#791][#792] address review feedback on bearer-auth remote MCP Follow-up to f6b2e2dbc resolving all bot review comments: - `isSchemaError`: narrow to `name === ZodError|$ZodError` AND an `issues` array, so unrelated errors carrying an `issues` property no longer trigger the lenient retry (Copilot). - `resolveHeadersCommand`: wrap `execFile` failures with the header key (`headersCommand[] failed: ...`) and append trimmed `stderr` so `mcp list` points to the exact failing command (Copilot). - Drop the now-redundant `headersCommand failed:` prefix at the call site since the thrown message already names the header. - Use Zod v4 `z.looseObject(...)` instead of deprecated `.loose()` for the lenient `tools/list` schemas (CodeRabbit). - `config.ts`: reword `headersCommand` comment to clarify it is argv via `execFile` (no shell interpolation); document why `normalizeMcpConfig` passes malformed array shapes through (schema rejects them with an actionable `invalid_type` rather than silently dropping) (Copilot). - Tests: remove unnecessary `as any` cast; assert against the production `LenientListToolsResultSchema` via `MCP._testing` instead of a duplicated copy; add an end-to-end `listToolsLenient` retry test using a real SDK `$ZodError`, and a test that array-shaped headers are rejected loudly. Co-Authored-By: Claude Opus 4.8 --- packages/opencode/src/config/config.ts | 14 ++- packages/opencode/src/mcp/index.ts | 112 ++++++++++------- packages/opencode/test/mcp/headers.test.ts | 2 +- .../opencode/test/mcp/mcp-bearer-auth.test.ts | 118 ++++++++++++------ 4 files changed, 159 insertions(+), 87 deletions(-) diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index 8e31b120ae..e6939cfdbe 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -649,9 +649,11 @@ export namespace Config { url: z.string().describe("URL of the remote MCP server"), enabled: z.boolean().optional().describe("Enable or disable the MCP server on startup"), headers: z.record(z.string(), z.string()).optional().describe("Headers to send with the request"), - // altimate_change start — dynamic header values produced by a shell command, - // resolved on each (re)connect so callers can refresh expiring bearer tokens - // without restarting the session (e.g. `az account get-access-token`). + // altimate_change start — dynamic header values produced by an argv command + // (run via execFile, not a shell — no shell interpolation unless the user + // explicitly invokes one like `sh -c`), resolved on each (re)connect so + // callers can refresh expiring bearer tokens without restarting the session + // (e.g. `az account get-access-token`). headersCommand: z .record(z.string(), z.array(z.string()).nonempty()) .optional() @@ -1454,6 +1456,12 @@ export namespace Config { servers[name] = transformed } else if (entry.url && typeof entry.url === "string") { const transformed: Record = { type: "remote", url: entry.url } + // Copy `headers` / `headersCommand` through as-is — including malformed + // array shapes. The downstream `Info.safeParse` validates `mcp` against + // the McpRemote schema, and `z.record(...)` rejects an array with an + // actionable `invalid_type` error. Stripping arrays here would instead + // drop the field silently and connect a header-less server with no + // feedback to the user. See #791 / #792. if (entry.headers && typeof entry.headers === "object") transformed.headers = entry.headers // altimate_change start — preserve fields that the original normalizer dropped // silently. Without these passes, a user-supplied `oauth: false` or diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 668c07d430..f0d44de18a 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -133,41 +133,42 @@ export namespace MCP { // Zod, blocking listTools() entirely. We accept `null` as "hint absent" by // calling `client.request()` with a permissive schema in place of the SDK's // strict one. See https://github.com/AltimateAI/altimate-code/issues/792. - const LenientToolAnnotationsSchema = z - .object({ - title: z.string().optional(), - readOnlyHint: z.boolean().nullable().optional(), - destructiveHint: z.boolean().nullable().optional(), - idempotentHint: z.boolean().nullable().optional(), - openWorldHint: z.boolean().nullable().optional(), - }) - .loose() - - const LenientToolSchema = z - .object({ - name: z.string(), - title: z.string().optional(), - description: z.string().optional(), - inputSchema: z.any(), - outputSchema: z.any().optional(), - annotations: LenientToolAnnotationsSchema.optional(), - _meta: z.record(z.string(), z.unknown()).optional(), - }) - .loose() - - const LenientListToolsResultSchema = z - .object({ - tools: z.array(LenientToolSchema), - nextCursor: z.string().optional(), - _meta: z.record(z.string(), z.unknown()).optional(), - }) - .loose() + const LenientToolAnnotationsSchema = z.looseObject({ + title: z.string().optional(), + readOnlyHint: z.boolean().nullable().optional(), + destructiveHint: z.boolean().nullable().optional(), + idempotentHint: z.boolean().nullable().optional(), + openWorldHint: z.boolean().nullable().optional(), + }) + + const LenientToolSchema = z.looseObject({ + name: z.string(), + title: z.string().optional(), + description: z.string().optional(), + inputSchema: z.any(), + outputSchema: z.any().optional(), + annotations: LenientToolAnnotationsSchema.optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) + + const LenientListToolsResultSchema = z.looseObject({ + tools: z.array(LenientToolSchema), + nextCursor: z.string().optional(), + _meta: z.record(z.string(), z.unknown()).optional(), + }) function isSchemaError(err: unknown): boolean { - if (!err) return false - if (err instanceof Error && (err.name === "ZodError" || err.constructor?.name === "$ZodError")) return true - if (typeof err === "object" && err !== null && "issues" in err) return true - return false + // Narrowly match Zod validation errors only. We key off the error *name* + // (not `instanceof z.ZodError`) because the MCP SDK may throw a ZodError + // from a different zod copy (it depends on `zod@^3.25 || ^4.0`), which + // would fail a cross-realm `instanceof`. In practice the SDK's z4-mini path + // names the error `$ZodError` (the primary case); the public ZodError + // subclass names it `ZodError`. We require an `issues` *array* (rather than + // `"issues" in err`) to avoid a throwing getter and to reject unrelated + // errors that merely carry an `issues` property of some other type. + if (typeof err !== "object" || err === null) return false + const name = (err as { name?: string }).name ?? (err as { constructor?: { name?: string } }).constructor?.name + return (name === "ZodError" || name === "$ZodError") && Array.isArray((err as { issues?: unknown }).issues) } /** @@ -197,16 +198,20 @@ export namespace MCP { export const _testing = { LenientListToolsResultSchema, isSchemaError, + listToolsLenient: (client: { + listTools: () => Promise<{ tools: MCPToolDef[] }> + request: (...args: any[]) => Promise + }) => listToolsLenient(client as unknown as MCPClient), resolveHeadersCommand: (spec: Record | undefined, key = "test") => resolveHeadersCommand(spec, key), hasAuthorizationHeader, } // altimate_change end - // altimate_change start — resolve dynamic header values produced by shell - // commands (e.g. `az account get-access-token`). Runs via execFile (not a - // shell) so values aren't subject to shell injection. Re-runs on every - // connect so expiring bearer tokens refresh without manual config edits. + // altimate_change start — resolve dynamic header values from argv commands + // (e.g. `az account get-access-token`). Each value is an argv array run via + // execFile (no shell) so values aren't subject to shell injection. Re-runs on + // every connect so expiring bearer tokens refresh without manual config edits. // See https://github.com/AltimateAI/altimate-code/issues/791. async function resolveHeadersCommand( spec: Record | undefined, @@ -219,11 +224,26 @@ export namespace MCP { throw new Error(`headersCommand[${name}] must be a non-empty argv array`) } const [cmd, ...args] = argv - const { stdout } = await execFileAsync(cmd, args, { - encoding: "utf-8", - maxBuffer: 1024 * 1024, - timeout: 30_000, - }) + let stdout = "" + try { + stdout = ( + await execFileAsync(cmd, args, { + encoding: "utf-8", + maxBuffer: 1024 * 1024, + timeout: 30_000, + }) + ).stdout + } catch (err) { + // Wrap with the header key so `mcp list` points to the exact failing + // command (ENOENT, timeout, non-zero exit) rather than a bare error. + // On a non-zero exit the actionable reason lives on `err.stderr` (e.g. + // `az`'s "run 'az login'"), which `err.message` omits — append it. + const e = err as { message?: string; stderr?: string } + const stderr = typeof e.stderr === "string" ? e.stderr.trim().slice(0, 500) : "" + const base = err instanceof Error ? err.message : String(err) + const message = stderr ? `${base}: ${stderr}` : base + throw new Error(`headersCommand[${name}] failed: ${message}`) + } const value = stdout.trim() if (!value) { throw new Error(`headersCommand[${name}] produced empty output`) @@ -484,9 +504,9 @@ export namespace MCP { if (mcp.type === "remote") { // altimate_change start — resolve dynamic headers (e.g. bearer tokens // produced by `az account get-access-token`) before constructing - // transports. Failure to resolve aborts the connect attempt with a - // clear error so the user sees `failed: headersCommand[...] failed` - // in `mcp list` rather than a generic transport error. + // transports. Failure to resolve aborts the connect attempt. The thrown + // message already names the failing header (`headersCommand[] failed: + // ...`), so the user sees exactly which command broke in `mcp list`. let dynamicHeaders: Record = {} try { dynamicHeaders = await resolveHeadersCommand(mcp.headersCommand, key) @@ -495,7 +515,7 @@ export namespace MCP { log.error("headersCommand resolution failed", { key, error: message }) return { mcpClient: undefined, - status: { status: "failed" as const, error: `headersCommand failed: ${message}` }, + status: { status: "failed" as const, error: message }, } } const mergedHeaders: Record = { ...(mcp.headers ?? {}), ...dynamicHeaders } diff --git a/packages/opencode/test/mcp/headers.test.ts b/packages/opencode/test/mcp/headers.test.ts index d4fb3f55b2..8ce78dd2bc 100644 --- a/packages/opencode/test/mcp/headers.test.ts +++ b/packages/opencode/test/mcp/headers.test.ts @@ -143,7 +143,7 @@ test("OAuth is auto-disabled when Authorization is supplied via headersCommand", headersCommand: { Authorization: ["printf", "Bearer dynamic-token"], }, - } as any).catch(() => {}) + }).catch(() => {}) expect(transportCalls.length).toBeGreaterThanOrEqual(1) for (const call of transportCalls) { diff --git a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts index 7da2d70196..71779cd243 100644 --- a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts +++ b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts @@ -1,41 +1,12 @@ import { describe, test, expect } from "bun:test" -import z from "zod/v4" import { Config } from "../../src/config/config" -// We replicate the lenient ListTools schema used in mcp/index.ts here so the -// test does not depend on the MCP module's heavy import surface (Instance, -// Telemetry, Bus, Plugin, etc.). The schema is byte-for-byte identical to -// the one in mcp/index.ts; if it drifts, the integration test in mcp.test.ts -// will catch it. -const LenientToolAnnotationsSchema = z - .object({ - title: z.string().optional(), - readOnlyHint: z.boolean().nullable().optional(), - destructiveHint: z.boolean().nullable().optional(), - idempotentHint: z.boolean().nullable().optional(), - openWorldHint: z.boolean().nullable().optional(), - }) - .loose() - -const LenientToolSchema = z - .object({ - name: z.string(), - title: z.string().optional(), - description: z.string().optional(), - inputSchema: z.any(), - outputSchema: z.any().optional(), - annotations: LenientToolAnnotationsSchema.optional(), - _meta: z.record(z.string(), z.unknown()).optional(), - }) - .loose() - -const LenientListToolsResultSchema = z - .object({ - tools: z.array(LenientToolSchema), - nextCursor: z.string().optional(), - _meta: z.record(z.string(), z.unknown()).optional(), - }) - .loose() +// Assert against the *production* lenient schema directly (exported via +// MCP._testing) so this test can never pass against a stale duplicate. The MCP +// module is already imported dynamically by the resolveHeadersCommand tests +// below, so pulling it in here adds no new import surface. +const { MCP } = await import("../../src/mcp") +const { LenientListToolsResultSchema } = MCP._testing // --------------------------------------------------------------------------- // 1. Lenient tools/list schema accepts what real-world servers emit. @@ -108,6 +79,67 @@ describe("lenient tools/list schema", () => { }) }) +// --------------------------------------------------------------------------- +// 1b. End-to-end listToolsLenient retry (#792). Locks in the load-bearing +// contract: when the SDK's strict listTools() rejects a Fabric-style payload, +// the rejected error is named `$ZodError` (zod v4-mini), isSchemaError catches +// it, and the lenient client.request() retry returns the tools. A future +// re-narrowing of isSchemaError would break this without tripping the +// schema-only tests above. +// --------------------------------------------------------------------------- +describe("listToolsLenient retry against SDK $ZodError (#792)", () => { + test("retries with lenient schema when strict listTools() rejects Fabric-style nulls", async () => { + const { ListToolsResultSchema } = await import("@modelcontextprotocol/sdk/types.js") + const { safeParse } = await import("@modelcontextprotocol/sdk/server/zod-compat.js") + // Real payload shape from Microsoft Fabric Core MCP: null annotation hints. + const fabricPayload = { + tools: [ + { + name: "list_workspaces", + inputSchema: { type: "object", properties: {} }, + annotations: { readOnlyHint: true, destructiveHint: null, idempotentHint: null, openWorldHint: null }, + }, + ], + } + // Produce the exact error the SDK would reject with (a `$ZodError`). + const strict: any = safeParse(ListToolsResultSchema as any, fabricPayload) + expect(strict.success).toBe(false) + expect(strict.error?.name).toBe("$ZodError") + + let requestCalled = false + const fakeClient = { + listTools: async () => { + throw strict.error + }, + request: async () => { + requestCalled = true + return fabricPayload + }, + } + + const result = await MCP._testing.listToolsLenient(fakeClient) + expect(requestCalled).toBe(true) + expect(result.tools).toHaveLength(1) + expect(result.tools[0].name).toBe("list_workspaces") + }) + + test("does NOT retry (rethrows) when the error is not a schema error", async () => { + const transportError = new Error("ECONNREFUSED") + let requestCalled = false + const fakeClient = { + listTools: async () => { + throw transportError + }, + request: async () => { + requestCalled = true + return { tools: [] } + }, + } + await expect(MCP._testing.listToolsLenient(fakeClient)).rejects.toThrow(/ECONNREFUSED/) + expect(requestCalled).toBe(false) + }) +}) + // --------------------------------------------------------------------------- // 2. McpRemote schema accepts new headersCommand field (issue #791). // --------------------------------------------------------------------------- @@ -133,6 +165,16 @@ describe("McpRemote.headersCommand schema (#791)", () => { expect(result.success).toBe(false) }) + test("rejects array-shaped headers/headersCommand with an actionable invalid_type error", () => { + // normalizeMcpConfig passes these malformed shapes through unchanged so the + // schema rejects them loudly instead of the normalizer silently dropping + // them (which would connect a header-less server with no feedback). + const headersArr = Config.McpRemote.safeParse({ type: "remote", url: "https://x/mcp", headers: ["a", "b"] }) + expect(headersArr.success).toBe(false) + const cmdArr = Config.McpRemote.safeParse({ type: "remote", url: "https://x/mcp", headersCommand: [["x"]] }) + expect(cmdArr.success).toBe(false) + }) + test("allows static headers and headersCommand to coexist", () => { const config = { type: "remote" as const, @@ -182,11 +224,13 @@ describe("resolveHeadersCommand helper", () => { ) }) - test("throws when command does not exist", async () => { + test("throws when command does not exist, naming the failing header", async () => { const { MCP } = await import("../../src/mcp") + // The error must name the specific header so `mcp list` points to the + // exact failing command rather than a bare ENOENT. await expect( MCP._testing.resolveHeadersCommand({ Authorization: ["this-binary-does-not-exist-xyz"] }), - ).rejects.toThrow() + ).rejects.toThrow(/headersCommand\[Authorization\] failed:/) }) test("does not invoke a shell (argv is passed directly to execFile)", async () => { From 5b75ca40c3b5e55bf503a662660afd65c9f39f5b Mon Sep 17 00:00:00 2001 From: Haider Date: Wed, 8 Jul 2026 00:16:00 +0530 Subject: [PATCH 3/3] fix: [#986] follow tools/list pagination so no tools are dropped `listToolsLenient` fetched only the first `tools/list` page and ignored `nextCursor`, so servers that paginate their tool catalog had every tool past the first page silently dropped. Both the strict `client.listTools()` path and the permissive-schema fallback are affected. - Loop on `nextCursor`, concatenating `tools`, across pages. - Pass the cursor through on both the strict and lenient request paths; once a schema error switches to the lenient path, stay lenient for the remaining pages. - Cap at `MAX_TOOL_LIST_PAGES` (100) to guard against a repeating cursor. - Add tests covering strict multi-page pagination and pagination that continues through the lenient fallback. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/opencode/src/mcp/index.ts | 63 ++++++++++++----- .../opencode/test/mcp/mcp-bearer-auth.test.ts | 70 +++++++++++++++++++ 2 files changed, 115 insertions(+), 18 deletions(-) diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 27babe2628..ef6e7770aa 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -175,27 +175,54 @@ export namespace MCP { return (name === "ZodError" || name === "$ZodError") && Array.isArray((err as { issues?: unknown }).issues) } + // Upper bound on tools/list pages we will follow, guarding against a server + // that returns a repeating or never-terminating `nextCursor`. + const MAX_TOOL_LIST_PAGES = 100 + /** - * Calls the SDK's strict `listTools()` first; on a Zod schema-validation - * failure (e.g. server emits non-spec values like `null` annotation hints), - * retries via `client.request()` with a permissive schema. This keeps the - * fast path unchanged for compliant servers while letting non-compliant - * ones (Microsoft Fabric, etc.) still register their tools. + * Fetches a server's complete tool list, following `nextCursor` pagination so + * tools beyond the first page are not dropped. Calls the SDK's strict + * `listTools()` first; on a Zod schema-validation failure (e.g. server emits + * non-spec values like `null` annotation hints), switches to `client.request()` + * with a permissive schema for the remaining pages. This keeps the fast path + * unchanged for compliant servers while letting non-compliant ones (Microsoft + * Fabric, etc.) still register all of their tools. See #986. */ async function listToolsLenient(client: MCPClient): Promise<{ tools: MCPToolDef[] }> { - try { - return await client.listTools() - } catch (err) { - if (!isSchemaError(err)) throw err - log.info("listTools strict schema rejected response, retrying with lenient schema", { - error: err instanceof Error ? err.message.slice(0, 200) : String(err).slice(0, 200), - }) - const result = await client.request( - { method: "tools/list", params: {} }, - LenientListToolsResultSchema as any, - ) - return result as { tools: MCPToolDef[] } + const tools: MCPToolDef[] = [] + let cursor: string | undefined = undefined + let lenient = false + for (let page = 0; page < MAX_TOOL_LIST_PAGES; page++) { + let result: { tools: MCPToolDef[]; nextCursor?: string } + if (!lenient) { + try { + result = await client.listTools(cursor ? { cursor } : {}) + } catch (err) { + if (!isSchemaError(err)) throw err + log.info("listTools strict schema rejected response, retrying with lenient schema", { + error: err instanceof Error ? err.message.slice(0, 200) : String(err).slice(0, 200), + }) + // Re-fetch the same cursor via the permissive schema, and stay lenient + // for any remaining pages (a non-compliant server rarely complies later). + lenient = true + page-- + continue + } + } else { + result = (await client.request( + { method: "tools/list", params: cursor ? { cursor } : {} }, + LenientListToolsResultSchema as any, + )) as { tools: MCPToolDef[]; nextCursor?: string } + } + tools.push(...result.tools) + if (!result.nextCursor) return { tools } + cursor = result.nextCursor } + log.warn("listTools pagination exceeded page cap, returning partial tool list", { + pages: MAX_TOOL_LIST_PAGES, + tools: tools.length, + }) + return { tools } } /** @internal — exported only for unit tests. Prefer using `tools()` in production code. */ @@ -203,7 +230,7 @@ export namespace MCP { LenientListToolsResultSchema, isSchemaError, listToolsLenient: (client: { - listTools: () => Promise<{ tools: MCPToolDef[] }> + listTools: (params?: any) => Promise<{ tools: any[]; nextCursor?: string }> request: (...args: any[]) => Promise }) => listToolsLenient(client as unknown as MCPClient), resolveHeadersCommand: (spec: Record | undefined, key = "test") => diff --git a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts index 71779cd243..0f7e3a4a3f 100644 --- a/packages/opencode/test/mcp/mcp-bearer-auth.test.ts +++ b/packages/opencode/test/mcp/mcp-bearer-auth.test.ts @@ -140,6 +140,76 @@ describe("listToolsLenient retry against SDK $ZodError (#792)", () => { }) }) +// --------------------------------------------------------------------------- +// 1c. listToolsLenient follows `nextCursor` pagination (#986) so tools beyond +// the first page are not silently dropped. Covers both the strict path and the +// case where the schema-error fallback engages mid-pagination. +// --------------------------------------------------------------------------- +describe("listToolsLenient pagination (#986)", () => { + test("follows nextCursor and concatenates tools across strict pages", async () => { + const cursors: (string | undefined)[] = [] + const pages: Record = { + "": { tools: [{ name: "a" }, { name: "b" }], nextCursor: "p2" }, + p2: { tools: [{ name: "c" }], nextCursor: "p3" }, + p3: { tools: [{ name: "d" }] }, + } + const fakeClient = { + listTools: async (params?: { cursor?: string }) => { + cursors.push(params?.cursor) + return pages[params?.cursor ?? ""] + }, + request: async () => { + throw new Error("strict path should not fall back here") + }, + } + const result = await MCP._testing.listToolsLenient(fakeClient) + expect(result.tools.map((t) => t.name)).toEqual(["a", "b", "c", "d"]) + expect(cursors).toEqual([undefined, "p2", "p3"]) + }) + + test("keeps paginating via the lenient path after a schema error, passing the cursor", async () => { + const { ListToolsResultSchema } = await import("@modelcontextprotocol/sdk/types.js") + const { safeParse } = await import("@modelcontextprotocol/sdk/server/zod-compat.js") + // Page 1 is Fabric-style (null hints) and trips strict validation; the retry + // and every later page must go through the lenient request path. + const page1 = { + tools: [ + { + name: "list_workspaces", + inputSchema: { type: "object" }, + annotations: { readOnlyHint: true, destructiveHint: null, idempotentHint: null, openWorldHint: null }, + }, + ], + nextCursor: "p2", + } + const strict: any = safeParse(ListToolsResultSchema as any, page1) + expect(strict.success).toBe(false) + + const requestCursors: (string | undefined)[] = [] + const lenientPages: Record = { + "": page1, + p2: { tools: [{ name: "second_page_tool" }] }, + } + const fakeClient = { + listTools: async (params?: { cursor?: string }) => { + // Strict path only rejects the first (cursorless) page; it is never + // retried on the strict path once we go lenient. + if (!params?.cursor) throw strict.error + throw new Error("strict path should not be reused after fallback") + }, + request: async (req: { params?: { cursor?: string } }) => { + const cursor = req.params?.cursor + requestCursors.push(cursor) + return lenientPages[cursor ?? ""] + }, + } + const result = await MCP._testing.listToolsLenient(fakeClient) + expect(result.tools.map((t) => t.name)).toEqual(["list_workspaces", "second_page_tool"]) + // Lenient path re-fetches page 1 (cursorless) then follows nextCursor to p2. + expect(requestCursors).toEqual([undefined, "p2"]) + }) +}) + // --------------------------------------------------------------------------- // 2. McpRemote schema accepts new headersCommand field (issue #791). // ---------------------------------------------------------------------------