diff --git a/packages/core/src/plugin/host.ts b/packages/core/src/plugin/host.ts index c36ceef093bb..ae6ce7727f14 100644 --- a/packages/core/src/plugin/host.ts +++ b/packages/core/src/plugin/host.ts @@ -384,7 +384,7 @@ export const make = Effect.fn("PluginHost.make")(function* (plugin: Interface, p add: (tool) => draft.add(tool), }), ) - .pipe(Effect.orDie, Effect.as({ dispose: Effect.void })), + .pipe(Effect.as({ dispose: Effect.void })), hook: (name, callback) => hooks.register("tool", name, callback), }, vcs: { diff --git a/packages/core/src/tool.ts b/packages/core/src/tool.ts index a31352d4e516..01aa17d2c4da 100644 --- a/packages/core/src/tool.ts +++ b/packages/core/src/tool.ts @@ -25,7 +25,7 @@ export class RegistrationError extends Schema.TaggedError()(" export interface Interface { readonly transform: ( callback: (draft: { readonly add: (tool: Tool.Info) => void }) => void, - ) => Effect.Effect + ) => Effect.Effect readonly snapshot: (permissions?: Permission.Ruleset) => Effect.Effect } @@ -140,45 +140,35 @@ const layer = Layer.effect( const transform: Interface["transform"] = Effect.fn("Tool.transform")(function* (callback) { const tools: Array = [] yield* Effect.sync(() => callback({ add: (tool) => tools.push(tool) })) - yield* Effect.forEach( - tools.flatMap((tool) => (tool.options?.namespace === undefined ? [] : [tool.options.namespace])), - validateNamespace, - { discard: true }, - ) - const entries = normalizedEntries(tools) - yield* Effect.forEach(entries, (entry) => validateName(normalizedName(entry.tool)), { discard: true }) - const collision = entries.find( - (entry, index) => entries.findIndex((candidate) => candidate.key === entry.key) !== index, - ) - if (collision) - return yield* Effect.fail( - new RegistrationError({ - name: collision.key, - message: `Duplicate normalized tool name: ${collision.key}`, - }), - ) - const reserved = entries.find((entry) => entry.tool.options?.codemode === false && entry.key === "execute") - if (reserved) - return yield* Effect.fail( - new RegistrationError({ - name: reserved.key, - message: 'Tool name "execute" is reserved for CodeMode', - }), - ) - if (entries.length === 0) return - yield* Effect.forEach( - entries, - (entry) => - Effect.try({ + const valid = yield* Effect.filter(normalizedEntries(tools), (entry) => + Effect.gen(function* () { + if (entry.tool.options?.namespace !== undefined) yield* validateNamespace(entry.tool.options.namespace) + yield* validateName(normalizedName(entry.tool)) + if (entry.tool.options?.codemode === false && entry.key === "execute") + return yield* new RegistrationError({ + name: entry.key, + message: 'Tool name "execute" is reserved for CodeMode', + }) + yield* Effect.try({ try: () => ToolDefinition.make(definition(entry.tool)), catch: (error) => new RegistrationError({ name: entry.key, message: `Invalid tool definition ${entry.key}: ${schemaMakeError(error)}`, }), - }), - { discard: true }, + }) + return true + }).pipe(Effect.catchTag("Tool.RegistrationError", (error) => skipRegistration(entry.tool, error))), ) + // Reject every ambiguous entry rather than choosing a winner. + const entries = yield* Effect.filter(valid, (entry) => { + if (!valid.some((candidate) => candidate !== entry && candidate.key === entry.key)) return Effect.succeed(true) + return skipRegistration( + entry.tool, + new RegistrationError({ name: entry.key, message: `Duplicate normalized tool name: ${entry.key}` }), + ) + }) + if (entries.length === 0) return yield* Effect.uninterruptible( lock.withPermit( Effect.gen(function* () { @@ -270,6 +260,13 @@ function schemaMakeError(error: unknown) { return error instanceof Error ? error.message : String(error) } +const skipRegistration = (tool: Tool.Info, error: RegistrationError) => + Effect.logError("Skipping invalid tool registration", { + name: tool.name, + namespace: tool.options?.namespace, + error: error.message, + }).pipe(Effect.as(false)) + const validateName = (name: string) => /^[A-Za-z0-9_-]{1,64}$/.test(name) ? Effect.void diff --git a/packages/core/src/tool/mcp.ts b/packages/core/src/tool/mcp.ts index 26e4c18f7eb9..fed8bff9ac21 100644 --- a/packages/core/src/tool/mcp.ts +++ b/packages/core/src/tool/mcp.ts @@ -115,7 +115,7 @@ export const layer = Layer.effect( }) } }) - .pipe(Scope.provide(next), Effect.orDie) + .pipe(Scope.provide(next)) if (current) yield* Scope.close(current, Exit.void) current = next }), diff --git a/packages/core/test/mcp.test.ts b/packages/core/test/mcp.test.ts index 48cbdbf5f908..89ae414ed5a1 100644 --- a/packages/core/test/mcp.test.ts +++ b/packages/core/test/mcp.test.ts @@ -35,7 +35,7 @@ import { AbsolutePath } from "@opencode-ai/core/schema" import { Session } from "@opencode-ai/core/session" import { McpTool } from "@opencode-ai/core/tool/mcp" import { Tool } from "@opencode-ai/core/tool" -import { DateTime, Deferred, Effect, Exit, Fiber, Layer, PubSub, Schedule, Schema, Sink, Stream } from "effect" +import { DateTime, Deferred, Effect, Exit, Fiber, Layer, PubSub, Ref, Schedule, Schema, Sink, Stream } from "effect" import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process" import { ExitCode, makeHandle, ProcessId } from "effect/unstable/process/ChildProcessSpawner" import { Image } from "@opencode-ai/core/image" @@ -1193,6 +1193,86 @@ test("serializes concurrent MCP lifecycle operations", async () => { ) }) +testEffect(Layer.empty).live("isolates invalid MCP tools and keeps catalog updates alive", () => + Effect.gen(function* () { + const tool = (server: string, name: string) => + new MCP.Tool({ + server: MCP.ServerName.make(server), + name, + codemode: false, + inputSchema: { type: "object", properties: {} }, + }) + const healthy = [tool("demo", "search"), tool("other", "lookup")] + const namespace = tool("x".repeat(65), "lookup") + const catalog = yield* Ref.make([tool("demo", "x".repeat(65)), ...healthy, namespace]) + + yield* Effect.gen(function* () { + const registry = yield* Tool.Service + const registration = yield* McpTool.Service + const bus = yield* Bus.Service + yield* registration.flush + expect((yield* toolDefinitions(registry)).map((tool) => tool.name)).toEqual([ + "demo_search", + "other_lookup", + "execute", + ]) + + yield* Ref.set(catalog, [tool("demo", "y".repeat(65)), ...healthy, tool("demo", "added"), namespace]) + yield* bus.publish(McpEvent.ToolsChanged, { server: "demo" }) + yield* waitForTool(registry, "demo_added") + expect((yield* toolDefinitions(registry)).map((tool) => tool.name)).toEqual([ + "demo_added", + "demo_search", + "other_lookup", + "execute", + ]) + yield* Effect.forEach(["demo_search", "other_lookup"], (name) => + executeTool(registry, { + sessionID: Session.ID.make("ses_mcp_invalid_catalog"), + ...toolIdentity, + call: { type: "tool-call", id: `call_${name}`, name, input: {} }, + }).pipe(Effect.tap((result) => Effect.sync(() => expect(result).toMatchObject({ status: "completed" })))), + ) + + yield* Ref.set(catalog, [tool("demo", "status"), ...healthy, tool("demo", "added"), tool("repaired", "lookup")]) + yield* bus.publish(McpEvent.ToolsChanged, { server: "demo" }) + yield* waitForTool(registry, "demo_status") + expect((yield* toolDefinitions(registry)).map((tool) => tool.name)).toEqual([ + "demo_added", + "demo_search", + "demo_status", + "other_lookup", + "repaired_lookup", + "execute", + ]) + }).pipe( + Effect.provide( + Layer.fresh( + AppNodeBuilder.build(LayerNode.group([Tool.node, McpTool.node, Bus.node]), [ + [ + MCP.node, + Layer.mock(MCP.Service, { + tools: () => Ref.get(catalog), + callTool: (input) => + Effect.succeed( + new MCP.ToolResult({ + server: MCP.ServerName.make(input.server), + tool: input.name, + isError: false, + content: [{ type: "text", text: "healthy" }], + }), + ), + }), + ], + [Permission.node, Layer.mock(Permission.Service, { assert: () => Effect.void })], + [Image.node, imagePassthrough], + ]), + ), + ), + ) + }), +) + it.effect("advertises MCP output schemas to Code Mode", () => Effect.gen(function* () { const registry = yield* Tool.Service diff --git a/packages/core/test/plugin.test.ts b/packages/core/test/plugin.test.ts index 53c2323215bc..eec029208204 100644 --- a/packages/core/test/plugin.test.ts +++ b/packages/core/test/plugin.test.ts @@ -282,6 +282,47 @@ describe("Plugin", () => { }), ) + it.effect("keeps plugins active when a tool registration is invalid", () => + Effect.gen(function* () { + const plugins = yield* Plugin.Service + const tools = yield* Tool.Service + const agents = yield* Agent.Service + yield* plugins.activate([ + { + id: "partial-tools", + version: "1", + effect: (ctx) => + Effect.gen(function* () { + yield* ctx.tool.transform((draft) => { + const tool = { + name: "healthy", + description: "Healthy tool", + input: Schema.Struct({}), + execute: () => Effect.succeed({ content: "ok" }), + options: { codemode: false }, + } + draft.add({ ...tool, name: "invalid", options: { namespace: "invalid..namespace" } }) + draft.add(tool) + }) + yield* ctx.agent.transform((draft) => + draft.update("configured", (agent) => { + agent.description = "setup continued" + }), + ) + }), + }, + ]) + + expect(yield* plugins.list()).toEqual([ + { id: Plugin.ID.make("partial-tools"), source: { type: "builtin" }, status: "active", tui: false }, + ]) + expect((yield* agents.get(Agent.ID.make("configured")))?.description).toBe("setup continued") + expect((yield* tools.snapshot()).definitions.map((tool) => tool.name)).toEqual(["healthy", "execute"]) + yield* plugins.activate([]) + expect((yield* tools.snapshot()).definitions.map((tool) => tool.name)).toEqual(["execute"]) + }), + ) + it.effect("restores the previous plugin when its replacement fails", () => Effect.gen(function* () { const plugins = yield* Plugin.Service diff --git a/packages/core/test/session-runner-tool-registry.test.ts b/packages/core/test/session-runner-tool-registry.test.ts index 69b8d936006b..f8fb24fa46db 100644 --- a/packages/core/test/session-runner-tool-registry.test.ts +++ b/packages/core/test/session-runner-tool-registry.test.ts @@ -10,7 +10,7 @@ import { Tool } from "@opencode-ai/core/tool" import type { Info } from "@opencode-ai/schema/tool" import { LayerNode } from "@opencode-ai/util/effect/layer-node" import { executeTool, toolDefinitions } from "./lib/tool" -import { Cause, Deferred, Effect, Exit, Fiber, Layer, Schema, SchemaGetter, SchemaIssue, Scope } from "effect" +import { Cause, Deferred, Effect, Exit, Fiber, Layer, Logger, Schema, SchemaGetter, SchemaIssue, Scope } from "effect" import { z } from "zod" import { testEffect } from "./lib/effect" @@ -71,30 +71,49 @@ const transform = (service: Tool.Interface, tools: Readonly ) describe("Tool", () => { - it.effect("rejects invalid dotted namespaces", () => - Effect.gen(function* () { + it.effect("logs and skips invalid dotted namespaces", () => { + const output: unknown[] = [] + const logger = Logger.map(Logger.formatStructured, (entry) => { + output.push(entry.message) + }) + return Effect.gen(function* () { const service = yield* Tool.Service - const error = yield* transform(service, { echo: make() }, { namespace: "slack..admin" }).pipe(Effect.flip) + yield* transform(service, { echo: make() }, { namespace: "slack..admin" }) - expect(error).toBeInstanceOf(Tool.RegistrationError) - expect(error.message).toBe('Invalid tool namespace: "slack..admin"') - expect((yield* service.snapshot()).definitions.map((tool) => tool.name)).toEqual(["execute"]) - }), - ) + expect(output).toEqual([ + [ + "Skipping invalid tool registration", + { name: "echo", namespace: "slack..admin", error: 'Invalid tool namespace: "slack..admin"' }, + ], + ]) + const snapshot = yield* service.snapshot() + expect(snapshot.definitions.map((tool) => tool.name)).toEqual(["execute"]) + expect(snapshot.codeModeCatalog).toEqual([]) + }).pipe(Effect.provide(Logger.layer([logger]))) + }) - it.effect("rejects invalid and colliding normalized names", () => + it.effect("skips invalid, reserved, and colliding names without dropping healthy tools", () => Effect.gen(function* () { const service = yield* Tool.Service - for (const name of ["", "x".repeat(65)]) { - const invalid = yield* transform(service, { [name]: make() }, { codemode: false }).pipe(Effect.flip) - expect(invalid.message).toBe(`Invalid tool name: ${name}`) - } - - const collision = yield* transform(service, { "echo.tool": make(), echo_tool: make() }, { codemode: false }).pipe( - Effect.flip, + yield* transform( + service, + { + before: make(), + "": make(), + ["x".repeat(65)]: make(), + "echo.tool": make(), + echo_tool: make(), + execute: make(), + after: make(), + }, + { codemode: false }, ) - expect(collision.message).toBe("Duplicate normalized tool name: echo_tool") - expect((yield* service.snapshot()).definitions.map((tool) => tool.name)).toEqual(["execute"]) + const snapshot = yield* service.snapshot() + expect(snapshot.definitions.map((tool) => tool.name)).toEqual(["after", "before", "execute"]) + expect((yield* snapshot.execute(call("before"))).output).toEqual({ text: "before" }) + expect((yield* snapshot.execute(call("after"))).output).toEqual({ text: "after" }) + expect((yield* snapshot.execute(call("echo_tool")).pipe(Effect.flip)).message).toBe("Unknown tool: echo_tool") + expect(snapshot.codeModeCatalog).toEqual([]) }), ) @@ -159,40 +178,72 @@ describe("Tool", () => { }), ) - it.effect("validates a registration batch before installing any tools", () => + it.effect("keeps healthy tools when another namespace is invalid", () => Effect.gen(function* () { const service = yield* Tool.Service - const error = yield* service - .transform((draft) => { - draft.add({ ...make(), name: "first", options: { codemode: false } }) - draft.add({ ...make(), name: "second", options: { namespace: "invalid..namespace", codemode: false } }) - }) - .pipe(Effect.flip) - - expect(error).toBeInstanceOf(Tool.RegistrationError) - expect((yield* service.snapshot()).definitions.map((tool) => tool.name)).toEqual(["execute"]) + yield* service.transform((draft) => { + draft.add({ ...make(), name: "first", options: { codemode: false } }) + draft.add({ ...make(), name: "second", options: { namespace: "invalid..namespace", codemode: false } }) + draft.add({ ...make(), name: "second", options: { namespace: "invalid__namespace" } }) + }) + + const snapshot = yield* service.snapshot() + expect(snapshot.definitions.map((tool) => tool.name)).toEqual(["first", "execute"]) + expect(snapshot.codeModeCatalog?.map((tool) => tool.path)).toEqual(["invalid__namespace.second"]) }), ) - it.effect("rejects invalid tool definitions before installing any tools", () => - Effect.gen(function* () { + it.effect("logs invalid tool definitions without dropping healthy tools", () => { + const output: unknown[] = [] + const logger = Logger.map(Logger.formatStructured, (entry) => { + output.push(entry.message) + }) + return Effect.gen(function* () { const service = yield* Tool.Service - const error = yield* service - .transform((draft) => { - draft.add({ ...make(), name: "healthy", options: { codemode: false } }) - draft.add({ + yield* service.transform((draft) => { + draft.add({ ...make(), name: "healthy", options: { codemode: false } }) + draft.add({ + name: "phone_type", + input: Schema.Struct({}), + execute: () => Effect.succeed({ content: "ok" }), + options: { codemode: false }, + } as unknown as Info) + draft.add({ ...make(), name: "codemode" }) + }) + + expect(output).toEqual([ + [ + "Skipping invalid tool registration", + { name: "phone_type", - input: Schema.Struct({}), - execute: () => Effect.succeed({ content: "ok" }), - options: { codemode: false }, - } as unknown as Info) - }) - .pipe(Effect.flip) - - expect(error).toBeInstanceOf(Tool.RegistrationError) - expect(error.name).toBe("phone_type") - expect(error.message).toContain('Expected string\n at ["description"]') - expect((yield* service.snapshot()).definitions.map((tool) => tool.name)).toEqual(["execute"]) + namespace: undefined, + error: expect.stringContaining('Expected string\n at ["description"]'), + }, + ], + ]) + const snapshot = yield* service.snapshot() + expect(snapshot.definitions.map((tool) => tool.name)).toEqual(["healthy", "execute"]) + expect(snapshot.codeModeCatalog?.map((tool) => tool.path)).toEqual(["codemode"]) + expect((yield* snapshot.execute(call("phone_type")).pipe(Effect.flip)).message).toBe("Unknown tool: phone_type") + }).pipe(Effect.provide(Logger.layer([logger]))) + }) + + it.effect("skipped registrations leave existing tools and scoped cleanup intact", () => + Effect.gen(function* () { + const service = yield* Tool.Service + yield* transform(service, { echo: constant("original") }, { codemode: false }) + yield* Effect.scoped( + Effect.gen(function* () { + yield* service.transform((draft) => { + draft.add({ ...constant("invalid"), name: "echo", description: undefined } as unknown as Info) + draft.add({ ...make(), name: "temporary", options: { codemode: false } }) + }) + const snapshot = yield* service.snapshot() + expect(snapshot.definitions.map((tool) => tool.name)).toEqual(["echo", "temporary", "execute"]) + expect((yield* snapshot.execute(call("echo"))).output).toEqual({ text: "original" }) + }), + ) + expect((yield* service.snapshot()).definitions.map((tool) => tool.name)).toEqual(["echo", "execute"]) }), )