From 0beb4de3e84921d64c086b31feea948d910a4176 Mon Sep 17 00:00:00 2001 From: Shoubhit Dash Date: Fri, 22 May 2026 17:46:30 +0530 Subject: [PATCH] fix(httpapi): return mcp server not found errors (#28817) --- packages/opencode/src/mcp/index.ts | 47 ++++++++------- .../server/routes/instance/httpapi/errors.ts | 9 +++ .../routes/instance/httpapi/groups/mcp.ts | 11 ++-- .../routes/instance/httpapi/handlers/mcp.ts | 59 +++++++++++++++---- packages/opencode/test/mcp/lifecycle.test.ts | 20 ++++--- .../test/server/httpapi-exercise/index.ts | 35 +++-------- .../opencode/test/server/httpapi-mcp.test.ts | 32 ++++++++++ .../server/httpapi-public-openapi.test.ts | 17 ++++++ 8 files changed, 158 insertions(+), 72 deletions(-) diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 6ebc714a27..4b831148d2 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -67,6 +67,10 @@ export const Failed = NamedError.create("MCPFailed", { name: Schema.String, }) +export class NotFoundError extends Schema.TaggedErrorClass()("MCP.NotFoundError", { + name: Schema.String, +}) {} + type MCPClient = Client const StatusConnected = Schema.Struct({ status: Schema.Literal("connected") }).annotate({ @@ -242,8 +246,8 @@ export interface Interface { readonly prompts: () => Effect.Effect> readonly resources: () => Effect.Effect> readonly add: (name: string, mcp: ConfigMCP.Info) => Effect.Effect<{ status: Record | Status }> - readonly connect: (name: string) => Effect.Effect - readonly disconnect: (name: string) => Effect.Effect + readonly connect: (name: string) => Effect.Effect + readonly disconnect: (name: string) => Effect.Effect readonly getPrompt: ( clientName: string, name: string, @@ -253,11 +257,11 @@ export interface Interface { clientName: string, resourceUri: string, ) => Effect.Effect> | undefined> - readonly startAuth: (mcpName: string) => Effect.Effect<{ authorizationUrl: string; oauthState: string }> - readonly authenticate: (mcpName: string) => Effect.Effect - readonly finishAuth: (mcpName: string, authorizationCode: string) => Effect.Effect + readonly startAuth: (mcpName: string) => Effect.Effect<{ authorizationUrl: string; oauthState: string }, NotFoundError> + readonly authenticate: (mcpName: string) => Effect.Effect + readonly finishAuth: (mcpName: string, authorizationCode: string) => Effect.Effect readonly removeAuth: (mcpName: string) => Effect.Effect - readonly supportsOAuth: (mcpName: string) => Effect.Effect + readonly supportsOAuth: (mcpName: string) => Effect.Effect readonly hasStoredTokens: (mcpName: string) => Effect.Effect readonly getAuthStatus: (mcpName: string) => Effect.Effect } @@ -642,15 +646,12 @@ export const layer = Layer.effect( }) const connect = Effect.fn("MCP.connect")(function* (name: string) { - const mcp = yield* getMcpConfig(name) - if (!mcp) { - log.error("MCP config not found or invalid", { name }) - return - } + const mcp = yield* requireMcpConfig(name) yield* createAndStore(name, { ...mcp, enabled: true }) }) const disconnect = Effect.fn("MCP.disconnect")(function* (name: string) { + yield* requireMcpConfig(name) const s = yield* InstanceState.get(state) yield* closeClient(s, name) delete s.clients[name] @@ -759,9 +760,14 @@ export const layer = Layer.effect( return mcpConfig }) - const startAuth = Effect.fn("MCP.startAuth")(function* (mcpName: string) { + const requireMcpConfig = Effect.fnUntraced(function* (mcpName: string) { const mcpConfig = yield* getMcpConfig(mcpName) - if (!mcpConfig) throw new Error(`MCP server ${mcpName} not found or disabled`) + if (!mcpConfig) return yield* new NotFoundError({ name: mcpName }) + return mcpConfig + }) + + const startAuth = Effect.fn("MCP.startAuth")(function* (mcpName: string) { + const mcpConfig = yield* requireMcpConfig(mcpName) if (mcpConfig.type !== "remote") throw new Error(`MCP server ${mcpName} is not a remote server`) if (mcpConfig.oauth === false) throw new Error(`MCP server ${mcpName} has OAuth explicitly disabled`) const url = remoteURL(mcpName, mcpConfig.url) @@ -825,11 +831,9 @@ export const layer = Layer.effect( const result = yield* startAuth(mcpName) if (!result.authorizationUrl) { const client = "client" in result ? result.client : undefined - const mcpConfig = yield* getMcpConfig(mcpName) - if (!mcpConfig) { - yield* Effect.tryPromise(() => client?.close() ?? Promise.resolve()).pipe(Effect.ignore) - return { status: "failed", error: "MCP config not found after auth" } as Status - } + const mcpConfig = yield* requireMcpConfig(mcpName).pipe( + Effect.tapError(() => Effect.tryPromise(() => client?.close() ?? Promise.resolve()).pipe(Effect.ignore)), + ) const listed = client ? yield* defs(mcpName, client, mcpConfig.timeout) : undefined if (!client || !listed) { @@ -880,6 +884,7 @@ export const layer = Layer.effect( }) const finishAuth = Effect.fn("MCP.finishAuth")(function* (mcpName: string, authorizationCode: string) { + yield* requireMcpConfig(mcpName) const transport = pendingOAuthTransports.get(mcpName) if (!transport) throw new Error(`No pending OAuth flow for MCP server: ${mcpName}`) @@ -898,8 +903,7 @@ export const layer = Layer.effect( yield* auth.clearCodeVerifier(mcpName) pendingOAuthTransports.delete(mcpName) - const mcpConfig = yield* getMcpConfig(mcpName) - if (!mcpConfig) return { status: "failed", error: "MCP config not found after auth" } as Status + const mcpConfig = yield* requireMcpConfig(mcpName) return yield* createAndStore(mcpName, mcpConfig) }) @@ -912,8 +916,7 @@ export const layer = Layer.effect( }) const supportsOAuth = Effect.fn("MCP.supportsOAuth")(function* (mcpName: string) { - const mcpConfig = yield* getMcpConfig(mcpName) - if (!mcpConfig) return false + const mcpConfig = yield* requireMcpConfig(mcpName) return mcpConfig.type === "remote" && mcpConfig.oauth !== false }) diff --git a/packages/opencode/src/server/routes/instance/httpapi/errors.ts b/packages/opencode/src/server/routes/instance/httpapi/errors.ts index f2afe7f0e3..d4fc232e3d 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/errors.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/errors.ts @@ -140,6 +140,15 @@ export class PermissionNotFoundError extends Schema.TaggedErrorClass()( + "McpServerNotFoundError", + { + name: Schema.String, + message: Schema.String, + }, + { httpApiStatus: 404 }, +) {} + export class ApiNotFoundError extends Schema.ErrorClass("NotFoundError")( { name: Schema.Literal("NotFoundError"), diff --git a/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts b/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts index c7ed4a9b95..929df4d214 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/groups/mcp.ts @@ -2,6 +2,7 @@ import { MCP } from "@/mcp" import { ConfigMCP } from "@/config/mcp" import { Schema } from "effect" import { HttpApi, HttpApiEndpoint, HttpApiError, HttpApiGroup, OpenApi } from "effect/unstable/httpapi" +import { McpServerNotFoundError } from "../errors" import { Authorization } from "../middleware/authorization" import { InstanceContextMiddleware } from "../middleware/instance-context" import { WorkspaceRoutingMiddleware, WorkspaceRoutingQuery } from "../middleware/workspace-routing" @@ -67,7 +68,7 @@ export const McpApi = HttpApi.make("mcp") params: { name: Schema.String }, query: WorkspaceRoutingQuery, success: described(AuthStartResponse, "OAuth flow started"), - error: [UnsupportedOAuthError, HttpApiError.NotFound], + error: [UnsupportedOAuthError, McpServerNotFoundError], }).annotateMerge( OpenApi.annotations({ identifier: "mcp.auth.start", @@ -80,7 +81,7 @@ export const McpApi = HttpApi.make("mcp") query: WorkspaceRoutingQuery, payload: AuthCallbackPayload, success: described(MCP.Status, "OAuth authentication completed"), - error: [HttpApiError.BadRequest, HttpApiError.NotFound], + error: [HttpApiError.BadRequest, McpServerNotFoundError], }).annotateMerge( OpenApi.annotations({ identifier: "mcp.auth.callback", @@ -93,7 +94,7 @@ export const McpApi = HttpApi.make("mcp") params: { name: Schema.String }, query: WorkspaceRoutingQuery, success: described(MCP.Status, "OAuth authentication completed"), - error: [UnsupportedOAuthError, HttpApiError.NotFound], + error: [UnsupportedOAuthError, McpServerNotFoundError], }).annotateMerge( OpenApi.annotations({ identifier: "mcp.auth.authenticate", @@ -105,7 +106,7 @@ export const McpApi = HttpApi.make("mcp") params: { name: Schema.String }, query: WorkspaceRoutingQuery, success: described(AuthRemoveResponse, "OAuth credentials removed"), - error: HttpApiError.NotFound, + error: McpServerNotFoundError, }).annotateMerge( OpenApi.annotations({ identifier: "mcp.auth.remove", @@ -117,6 +118,7 @@ export const McpApi = HttpApi.make("mcp") params: { name: Schema.String }, query: WorkspaceRoutingQuery, success: described(Schema.Boolean, "MCP server connected successfully"), + error: McpServerNotFoundError, }).annotateMerge( OpenApi.annotations({ identifier: "mcp.connect", @@ -127,6 +129,7 @@ export const McpApi = HttpApi.make("mcp") params: { name: Schema.String }, query: WorkspaceRoutingQuery, success: described(Schema.Boolean, "MCP server disconnected successfully"), + error: McpServerNotFoundError, }).annotateMerge( OpenApi.annotations({ identifier: "mcp.disconnect", diff --git a/packages/opencode/src/server/routes/instance/httpapi/handlers/mcp.ts b/packages/opencode/src/server/routes/instance/httpapi/handlers/mcp.ts index a02f2425ce..01b9e853fc 100644 --- a/packages/opencode/src/server/routes/instance/httpapi/handlers/mcp.ts +++ b/packages/opencode/src/server/routes/instance/httpapi/handlers/mcp.ts @@ -2,6 +2,7 @@ import { MCP } from "@/mcp" import { Effect, Schema } from "effect" import { HttpApiBuilder, HttpApiError } from "effect/unstable/httpapi" import { InstanceHttpApi } from "../api" +import { McpServerNotFoundError } from "../errors" import { AddPayload, AuthCallbackPayload, StatusMap, UnsupportedOAuthError } from "../groups/mcp" export const mcpHandlers = HttpApiBuilder.group(InstanceHttpApi, "mcp", (handlers) => @@ -20,38 +21,74 @@ export const mcpHandlers = HttpApiBuilder.group(InstanceHttpApi, "mcp", (handler }) const authStart = Effect.fn("McpHttpApi.authStart")(function* (ctx: { params: { name: string } }) { - if (!(yield* mcp.supportsOAuth(ctx.params.name))) { - return yield* new UnsupportedOAuthError({ error: `MCP server ${ctx.params.name} does not support OAuth` }) - } - return yield* mcp.startAuth(ctx.params.name) + return yield* Effect.gen(function* () { + if (!(yield* mcp.supportsOAuth(ctx.params.name))) { + return yield* new UnsupportedOAuthError({ error: `MCP server ${ctx.params.name} does not support OAuth` }) + } + return yield* mcp.startAuth(ctx.params.name) + }).pipe( + Effect.catchTag("MCP.NotFoundError", (error) => + Effect.fail(new McpServerNotFoundError({ name: error.name, message: `MCP server not found: ${error.name}` })), + ), + ) }) const authCallback = Effect.fn("McpHttpApi.authCallback")(function* (ctx: { params: { name: string } payload: typeof AuthCallbackPayload.Type }) { - return yield* mcp.finishAuth(ctx.params.name, ctx.payload.code) + return yield* mcp + .finishAuth(ctx.params.name, ctx.payload.code) + .pipe( + Effect.catchTag("MCP.NotFoundError", (error) => + Effect.fail(new McpServerNotFoundError({ name: error.name, message: `MCP server not found: ${error.name}` })), + ), + ) }) const authAuthenticate = Effect.fn("McpHttpApi.authAuthenticate")(function* (ctx: { params: { name: string } }) { - if (!(yield* mcp.supportsOAuth(ctx.params.name))) { - return yield* new UnsupportedOAuthError({ error: `MCP server ${ctx.params.name} does not support OAuth` }) - } - return yield* mcp.authenticate(ctx.params.name) + return yield* Effect.gen(function* () { + if (!(yield* mcp.supportsOAuth(ctx.params.name))) { + return yield* new UnsupportedOAuthError({ error: `MCP server ${ctx.params.name} does not support OAuth` }) + } + return yield* mcp.authenticate(ctx.params.name) + }).pipe( + Effect.catchTag("MCP.NotFoundError", (error) => + Effect.fail(new McpServerNotFoundError({ name: error.name, message: `MCP server not found: ${error.name}` })), + ), + ) }) const authRemove = Effect.fn("McpHttpApi.authRemove")(function* (ctx: { params: { name: string } }) { + const status = yield* mcp.status() + if (!(ctx.params.name in status)) + return yield* new McpServerNotFoundError({ + name: ctx.params.name, + message: `MCP server not found: ${ctx.params.name}`, + }) yield* mcp.removeAuth(ctx.params.name) return { success: true as const } }) const connect = Effect.fn("McpHttpApi.connect")(function* (ctx: { params: { name: string } }) { - yield* mcp.connect(ctx.params.name) + yield* mcp + .connect(ctx.params.name) + .pipe( + Effect.catchTag("MCP.NotFoundError", (error) => + Effect.fail(new McpServerNotFoundError({ name: error.name, message: `MCP server not found: ${error.name}` })), + ), + ) return true }) const disconnect = Effect.fn("McpHttpApi.disconnect")(function* (ctx: { params: { name: string } }) { - yield* mcp.disconnect(ctx.params.name) + yield* mcp + .disconnect(ctx.params.name) + .pipe( + Effect.catchTag("MCP.NotFoundError", (error) => + Effect.fail(new McpServerNotFoundError({ name: error.name, message: `MCP server not found: ${error.name}` })), + ), + ) return true }) diff --git a/packages/opencode/test/mcp/lifecycle.test.ts b/packages/opencode/test/mcp/lifecycle.test.ts index 185086fe60..dddfd9875e 100644 --- a/packages/opencode/test/mcp/lifecycle.test.ts +++ b/packages/opencode/test/mcp/lifecycle.test.ts @@ -1,5 +1,5 @@ import { expect, mock, beforeEach } from "bun:test" -import { Effect, Exit } from "effect" +import { Cause, Effect, Exit } from "effect" import type { MCP as MCPNS } from "../../src/mcp/index" import { testEffect } from "../lib/effect" @@ -635,12 +635,15 @@ it.instance( // ======================================================================== it.instance( - "connect() on nonexistent server does not throw", + "connect() on nonexistent server fails with NotFoundError", () => MCP.Service.use((mcp: MCPNS.Interface) => Effect.gen(function* () { - // Should not throw - yield* mcp.connect("nonexistent") + const exit = yield* mcp.connect("nonexistent").pipe(Effect.exit) + expect(Exit.isFailure(exit)).toBe(true) + if (Exit.isFailure(exit)) { + expect(Cause.squash(exit.cause)).toMatchObject({ _tag: "MCP.NotFoundError", name: "nonexistent" }) + } const status = yield* mcp.status() expect(status["nonexistent"]).toBeUndefined() }), @@ -653,12 +656,15 @@ it.instance( // ======================================================================== it.instance( - "disconnect() on nonexistent server does not throw", + "disconnect() on nonexistent server fails with NotFoundError", () => MCP.Service.use((mcp: MCPNS.Interface) => Effect.gen(function* () { - yield* mcp.disconnect("nonexistent") - // Should complete without error + const exit = yield* mcp.disconnect("nonexistent").pipe(Effect.exit) + expect(Exit.isFailure(exit)).toBe(true) + if (Exit.isFailure(exit)) { + expect(Cause.squash(exit.cause)).toMatchObject({ _tag: "MCP.NotFoundError", name: "nonexistent" }) + } }), ), { config: { mcp: {} } }, diff --git a/packages/opencode/test/server/httpapi-exercise/index.ts b/packages/opencode/test/server/httpapi-exercise/index.ts index 66c7bfdef2..0c834e91b7 100644 --- a/packages/opencode/test/server/httpapi-exercise/index.ts +++ b/packages/opencode/test/server/httpapi-exercise/index.ts @@ -329,58 +329,37 @@ const scenarios: Scenario[] = [ http.protected .post("/mcp/{name}/auth", "mcp.auth.start") .at((ctx) => ({ path: route("/mcp/{name}/auth", { name: "httpapi-missing" }), headers: ctx.headers() })) - .json( - 400, - (body) => { - object(body) - check(typeof body.error === "string", "unsupported MCP OAuth response should include error") - }, - "status", - ), + .json(404, object, "status"), http.protected .delete("/mcp/{name}/auth", "mcp.auth.remove") .mutating() .at((ctx) => ({ path: route("/mcp/{name}/auth", { name: "httpapi-missing" }), headers: ctx.headers() })) - .json(200, (body) => { - object(body) - check(body.success === true, "MCP auth removal should return success") - }), + .json(404, object, "status"), http.protected .post("/mcp/{name}/auth/authenticate", "mcp.auth.authenticate") .at((ctx) => ({ path: route("/mcp/{name}/auth/authenticate", { name: "httpapi-missing" }), headers: ctx.headers(), })) - .json( - 400, - (body) => { - object(body) - check(typeof body.error === "string", "unsupported MCP OAuth authenticate response should include error") - }, - "status", - ), + .json(404, object, "status"), http.protected .post("/mcp/{name}/auth/callback", "mcp.auth.callback") .at((ctx) => ({ path: route("/mcp/{name}/auth/callback", { name: "httpapi-missing" }), headers: ctx.headers(), - body: { code: 1 }, + body: { code: "code" }, })) - .status(400), + .json(404, object, "status"), http.protected .post("/mcp/{name}/connect", "mcp.connect") .mutating() .at((ctx) => ({ path: route("/mcp/{name}/connect", { name: "httpapi-missing" }), headers: ctx.headers() })) - .json(200, (body) => { - check(body === true, "missing MCP connect should remain a no-op success") - }), + .json(404, object, "status"), http.protected .post("/mcp/{name}/disconnect", "mcp.disconnect") .mutating() .at((ctx) => ({ path: route("/mcp/{name}/disconnect", { name: "httpapi-missing" }), headers: ctx.headers() })) - .json(200, (body) => { - check(body === true, "missing MCP disconnect should remain a no-op success") - }), + .json(404, object, "status"), http.protected.get("/pty/shells", "pty.shells").json(200, array), http.protected.get("/pty", "pty.list").json(200, array), http.protected diff --git a/packages/opencode/test/server/httpapi-mcp.test.ts b/packages/opencode/test/server/httpapi-mcp.test.ts index 3f7f54f294..9985ced9f6 100644 --- a/packages/opencode/test/server/httpapi-mcp.test.ts +++ b/packages/opencode/test/server/httpapi-mcp.test.ts @@ -192,4 +192,36 @@ describe("mcp HttpApi", () => { }, }, ) + + it.instance( + "returns typed not found errors for missing MCP servers", + () => + Effect.gen(function* () { + const tmp = yield* TestInstance + const handler = yield* handlerScoped + + for (const input of [ + { method: "POST", route: "/mcp/missing/auth" }, + { method: "POST", route: "/mcp/missing/auth/authenticate" }, + { method: "POST", route: "/mcp/missing/auth/callback", body: JSON.stringify({ code: "code" }) }, + { method: "DELETE", route: "/mcp/missing/auth" }, + { method: "POST", route: "/mcp/missing/connect" }, + { method: "POST", route: "/mcp/missing/disconnect" }, + ]) { + const response = yield* request(handler, input.route, tmp.directory, { + method: input.method, + headers: input.body ? { "content-type": "application/json" } : undefined, + body: input.body, + }) + + expect(response.status).toBe(404) + expect(yield* json(response)).toEqual({ + _tag: "McpServerNotFoundError", + name: "missing", + message: "MCP server not found: missing", + }) + } + }), + { config: { mcp: {} } }, + ) }) diff --git a/packages/opencode/test/server/httpapi-public-openapi.test.ts b/packages/opencode/test/server/httpapi-public-openapi.test.ts index e98e07c005..afe3dd3f07 100644 --- a/packages/opencode/test/server/httpapi-public-openapi.test.ts +++ b/packages/opencode/test/server/httpapi-public-openapi.test.ts @@ -173,4 +173,21 @@ describe("PublicApi OpenAPI v2 errors", () => { ) } }) + + test("documents MCP server not-found errors", () => { + const spec = OpenApi.fromApi(PublicApi) as OpenApiSpec + + for (const route of [ + ["post", "/mcp/{name}/auth"], + ["post", "/mcp/{name}/auth/authenticate"], + ["post", "/mcp/{name}/auth/callback"], + ["delete", "/mcp/{name}/auth"], + ["post", "/mcp/{name}/connect"], + ["post", "/mcp/{name}/disconnect"], + ] as const) { + expect(componentName(responseRef(spec.paths[route[1]]?.[route[0]]?.responses?.["404"]) ?? "")).toBe( + "McpServerNotFoundError", + ) + } + }) })