Parcourir la source

fix(httpapi): return mcp server not found errors (#28817)

Shoubhit Dash il y a 3 mois
Parent
commit
0beb4de3e8

+ 25 - 22
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<NotFoundError>()("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<Record<string, PromptInfo & { client: string }>>
   readonly resources: () => Effect.Effect<Record<string, ResourceInfo & { client: string }>>
   readonly add: (name: string, mcp: ConfigMCP.Info) => Effect.Effect<{ status: Record<string, Status> | Status }>
-  readonly connect: (name: string) => Effect.Effect<void>
-  readonly disconnect: (name: string) => Effect.Effect<void>
+  readonly connect: (name: string) => Effect.Effect<void, NotFoundError>
+  readonly disconnect: (name: string) => Effect.Effect<void, NotFoundError>
   readonly getPrompt: (
     clientName: string,
     name: string,
@@ -253,11 +257,11 @@ export interface Interface {
     clientName: string,
     resourceUri: string,
   ) => Effect.Effect<Awaited<ReturnType<MCPClient["readResource"]>> | undefined>
-  readonly startAuth: (mcpName: string) => Effect.Effect<{ authorizationUrl: string; oauthState: string }>
-  readonly authenticate: (mcpName: string) => Effect.Effect<Status>
-  readonly finishAuth: (mcpName: string, authorizationCode: string) => Effect.Effect<Status>
+  readonly startAuth: (mcpName: string) => Effect.Effect<{ authorizationUrl: string; oauthState: string }, NotFoundError>
+  readonly authenticate: (mcpName: string) => Effect.Effect<Status, NotFoundError>
+  readonly finishAuth: (mcpName: string, authorizationCode: string) => Effect.Effect<Status, NotFoundError>
   readonly removeAuth: (mcpName: string) => Effect.Effect<void>
-  readonly supportsOAuth: (mcpName: string) => Effect.Effect<boolean>
+  readonly supportsOAuth: (mcpName: string) => Effect.Effect<boolean, NotFoundError>
   readonly hasStoredTokens: (mcpName: string) => Effect.Effect<boolean>
   readonly getAuthStatus: (mcpName: string) => Effect.Effect<AuthStatus>
 }
@@ -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
     })
 

+ 9 - 0
packages/opencode/src/server/routes/instance/httpapi/errors.ts

@@ -140,6 +140,15 @@ export class PermissionNotFoundError extends Schema.TaggedErrorClass<PermissionN
   { httpApiStatus: 404 },
 ) {}
 
+export class McpServerNotFoundError extends Schema.TaggedErrorClass<McpServerNotFoundError>()(
+  "McpServerNotFoundError",
+  {
+    name: Schema.String,
+    message: Schema.String,
+  },
+  { httpApiStatus: 404 },
+) {}
+
 export class ApiNotFoundError extends Schema.ErrorClass<ApiNotFoundError>("NotFoundError")(
   {
     name: Schema.Literal("NotFoundError"),

+ 7 - 4
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",

+ 48 - 11
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
     })
 

+ 13 - 7
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: {} } },

+ 7 - 28
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

+ 32 - 0
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: {} } },
+  )
 })

+ 17 - 0
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",
+      )
+    }
+  })
 })