diff --git a/packages/core/src/tool/registry.ts b/packages/core/src/tool/registry.ts index 844e4896db39..31e4552e5f97 100644 --- a/packages/core/src/tool/registry.ts +++ b/packages/core/src/tool/registry.ts @@ -1,7 +1,7 @@ export * as ToolRegistry from "./registry" import { ToolOutput, type ToolCall, type ToolDefinition, type ToolResultValue } from "@opencode-ai/ai" -import { Context, Effect, Layer, Scope } from "effect" +import { Context, Effect, Layer, Scope, Semaphore } from "effect" import type { AgentV2 } from "../agent" import { Image } from "../image" import { PermissionV2 } from "../permission" @@ -45,6 +45,13 @@ export interface Interface { tools: Readonly>, options?: Tools.RegisterOptions, ) => Effect.Effect + /** Internal atomic registration capability used by plugin transforms. */ + readonly registerBatch: ( + registrations: ReadonlyArray<{ + readonly tools: Readonly> + readonly options?: Tools.RegisterOptions + }>, + ) => Effect.Effect } export interface Materialization { @@ -107,6 +114,7 @@ const registryLayer = Layer.effect( readonly codemode: boolean } const local = new Map>() + const registrationLock = Semaphore.makeUnsafe(1) const settleTool = Effect.fn("ToolRegistry.settleTool")(function* (input: ExecuteInput, tool: AnyTool) { // Hooks fire only for hosted/local tools; provider-executed calls never reach settleTool. @@ -192,82 +200,128 @@ const registryLayer = Layer.effect( } }) - return Service.of({ - register: Effect.fn("ToolRegistry.register")(function* (tools, options) { - if (options?.namespace !== undefined) yield* validateNamespace(options.namespace) - const entries = registrationEntries(tools, options?.namespace) - if (entries.length === 0) return - const codemode = options?.codemode ?? true - const reserved = codemode ? undefined : entries.find((entry) => entry.key === "execute") - if (reserved) - return yield* Effect.fail( - new RegistrationError({ name: reserved.key, message: 'Tool name "execute" is reserved for CodeMode' }), - ) - yield* Effect.uninterruptible( + const registerBatch: Interface["registerBatch"] = Effect.fn("ToolRegistry.registerBatch")( + function* (registrations) { + const planned = yield* Effect.forEach(registrations, ({ tools, options }) => Effect.gen(function* () { - const token = {} - for (const entry of entries) - local.set(entry.key, [ - ...(local.get(entry.key) ?? []), - { - token, - registration: { - tool: entry.tool, - name: entry.name, - namespace: entry.namespace, - codemode, - }, - }, - ]) - yield* Effect.addFinalizer(() => - Effect.sync(() => { - for (const entry of entries) { - const registrations = - local.get(entry.key)?.filter((registration) => registration.token !== token) ?? [] - if (registrations.length > 0) local.set(entry.key, registrations) - else local.delete(entry.key) - } - }), - ) + if (options?.namespace !== undefined) yield* validateNamespace(options.namespace) + const entries = registrationEntries(tools, options?.namespace) + const codemode = options?.codemode ?? true + const reserved = codemode ? undefined : entries.find((entry) => entry.key === "execute") + if (reserved) + return yield* Effect.fail( + new RegistrationError({ name: reserved.key, message: 'Tool name "execute" is reserved for CodeMode' }), + ) + return { entries, codemode } }), ) - }), - materialize: Effect.fn("ToolRegistry.materialize")(function* (permissions) { - const direct = new Map() - const codemode = new Map() - const rules = permissions ?? [] - for (const [name, entries] of local) { - const registration = entries.at(-1)?.registration - if (!registration) continue - if (whollyDisabled(permission(registration.tool, name), rules)) continue - if (registration.codemode) codemode.set(name, registration) - else direct.set(name, registration) - } - const execute = - codemode.size > 0 && !whollyDisabled("execute", rules) ? ExecuteTool.create(codemode) : undefined - return { - definitions: [ - ...Array.from(direct, ([name, registration]) => definition(name, registration.tool)), - ...(execute ? [definition("execute", execute)] : []), - ], - settle: (input) => { - if (input.call.name === "execute" && execute) return settleTool(input, execute) - const registration = direct.get(input.call.name) - if (registration) return settleTool(input, registration.tool) - return Effect.succeed({ - result: { type: "error", value: `Unknown tool: ${input.call.name}` }, - error: { type: "tool.unknown", message: `Unknown tool: ${input.call.name}` }, - }) + if (planned.every((plan) => plan.entries.length === 0)) return + yield* Effect.uninterruptible( + registrationLock.withPermit( + Effect.gen(function* () { + const token = {} + for (const { entries, codemode } of planned) + for (const entry of entries) + local.set(entry.key, [ + ...(local.get(entry.key) ?? []), + { + token, + registration: { + tool: entry.tool, + name: entry.name, + namespace: entry.namespace, + codemode, + }, + }, + ]) + yield* Effect.addFinalizer(() => + registrationLock.withPermit( + Effect.sync(() => { + for (const { entries } of planned) + for (const entry of entries) { + const registrations = + local.get(entry.key)?.filter((registration) => registration.token !== token) ?? [] + if (registrations.length > 0) local.set(entry.key, registrations) + else local.delete(entry.key) + } + }), + ), + ) + }), + ), + ) + }, + ) + + return Service.of({ + register: Effect.fn("ToolRegistry.register")((tools, options) => + registerBatch([ + { + tools, + ...(options === undefined ? {} : { options }), }, - } - }), + ]), + ), + registerBatch, + materialize: Effect.fn("ToolRegistry.materialize")((permissions) => + registrationLock.withPermit( + Effect.gen(function* () { + const direct = new Map() + const codemode = new Map() + const rules = permissions ?? [] + for (const [name, entries] of local) { + const registration = entries.at(-1)?.registration + if (!registration) continue + if (whollyDisabled(permission(registration.tool, name), rules)) continue + if (registration.codemode) codemode.set(name, registration) + else direct.set(name, registration) + } + const execute = + codemode.size > 0 && !whollyDisabled("execute", rules) ? ExecuteTool.create(codemode) : undefined + // Build each tool definition defensively. A plugin can register a structurally + // invalid tool (missing description, undecodable input schema, opaque object from + // a different module copy, etc.) that only fails when its definition is read. + // Throwing here used to break every model step in the Location because no + // definition list could be produced. Isolate the bad tool: log and omit it + // so the rest of the catalog stays usable. See + // https://github.com/anomalyco/opencode/issues/35963 + const definitions: ToolDefinition[] = [] + for (const [name, registration] of direct) { + try { + definitions.push(definition(name, registration.tool)) + } catch (error) { + yield* Effect.logWarning("failed to materialize tool definition; skipping", { + tool: name, + error: error instanceof Error ? error.message : String(error), + }) + direct.delete(name) + } + } + if (execute) definitions.push(definition("execute", execute)) + return { + definitions, + settle: (input: ExecuteInput) => { + if (input.call.name === "execute" && execute) return settleTool(input, execute) + const registration = direct.get(input.call.name) + if (registration) return settleTool(input, registration.tool) + return Effect.succeed({ + result: { type: "error", value: `Unknown tool: ${input.call.name}` }, + error: { type: "tool.unknown", message: `Unknown tool: ${input.call.name}` }, + }) + }, + } + }), + ), + ), }) }), ) const layer = Layer.effect( Tools.Service, - Service.use((registry) => Effect.succeed(Tools.Service.of({ register: registry.register }))), + Service.use((registry) => + Effect.succeed(Tools.Service.of({ register: registry.register, registerBatch: registry.registerBatch })), + ), ).pipe(Layer.provideMerge(registryLayer)) function whollyDisabled(action: string, rules: PermissionV2.Ruleset) { diff --git a/packages/core/test/session-runner-tool-registry.test.ts b/packages/core/test/session-runner-tool-registry.test.ts index 6f7b13f6e14a..1a9959882240 100644 --- a/packages/core/test/session-runner-tool-registry.test.ts +++ b/packages/core/test/session-runner-tool-registry.test.ts @@ -490,4 +490,32 @@ describe("ToolRegistry", () => { expect(executed).toEqual(["old:request"]) }), ) + + // Regression coverage for https://github.com/anomalyco/opencode/issues/35963 + // A structurally invalid tool registration used to throw inside materialize() and break + // every subsequent model step in the Location. The registry must isolate the bad tool: + // log it, drop it, and keep the rest of the catalog usable. + it.effect("isolates invalid tool definitions during materialize (#35963)", () => + Effect.gen(function* () { + const service = yield* ToolRegistry.Service + const good = make() + // Construct a tool-like object whose `definition()` reads will throw: `input` is + // missing so `inputJsonSchema(tool.input)` dereferences undefined. + const broken = { description: "broken" } as unknown as Parameters[0] + yield* service.register({ good }, { codemode: false }) + yield* service.register({ broken: Tool.make(broken) }, { codemode: false }) + + const materialized = yield* service.materialize() + const names = materialized.definitions.map((definition) => definition.name) + expect(names).toEqual(["good"]) + expect(names).not.toContain("broken") + + // Settling the dropped tool surfaces as "Unknown tool" rather than re-throwing. + const settlement = yield* settleTool(service, call("broken")) + expect(settlement.result).toEqual({ type: "error", value: "Unknown tool: broken" }) + // The good tool still executes. + const goodSettlement = yield* settleTool(service, call("good")) + expect(goodSettlement.result).toMatchObject({ type: "text", value: "good" }) + }), + ) })