From 524ee8fc03bbdb54799747af9254b85cb10252d3 Mon Sep 17 00:00:00 2001 From: "opencode-agent[bot]" <219766164+opencode-agent[bot]@users.noreply.github.com> Date: Mon, 29 Jun 2026 23:53:41 -0500 Subject: [PATCH] fix(core): gate v2 edit tools by model (#34558) Co-authored-by: Aiden Cline --- packages/core/src/session/runner/llm.ts | 4 +- packages/core/src/tool/registry.ts | 18 +++++-- packages/core/test/lib/tool.ts | 17 +++--- packages/core/test/location-layer.test.ts | 2 - .../test/session-runner-tool-registry.test.ts | 54 ++++++++++++------- packages/core/test/tool-apply-patch.test.ts | 16 +++++- packages/core/test/tool-subagent.test.ts | 6 ++- 7 files changed, 82 insertions(+), 35 deletions(-) diff --git a/packages/core/src/session/runner/llm.ts b/packages/core/src/session/runner/llm.ts index b34d8e3aa20..089991eed24 100644 --- a/packages/core/src/session/runner/llm.ts +++ b/packages/core/src/session/runner/llm.ts @@ -194,7 +194,9 @@ export const layer = Layer.effect( const entries = yield* SessionHistory.entriesForRunner(db, session.id, system.baselineSeq) const context = entries.map((entry) => entry.message) const isLastStep = agent.info?.steps !== undefined && currentStep >= agent.info.steps - const toolMaterialization = isLastStep ? undefined : yield* tools.materialize(agent.info?.permissions) + const toolMaterialization = isLastStep + ? undefined + : yield* tools.materialize({ permissions: agent.info?.permissions, model }) const promptCacheKey = /^ses_[0-9a-f]{64}$/.test(session.id) ? session.id.slice(4) : session.id const request = LLM.request({ model, diff --git a/packages/core/src/tool/registry.ts b/packages/core/src/tool/registry.ts index 41193060315..b47c1d6c6aa 100644 --- a/packages/core/src/tool/registry.ts +++ b/packages/core/src/tool/registry.ts @@ -21,11 +21,16 @@ export type ExecuteInput = { } export interface Interface { - readonly materialize: (permissions?: PermissionV2.Ruleset) => Effect.Effect + readonly materialize: (input: MaterializeInput) => Effect.Effect /** Internal registration capability exposed publicly only through Tools.Service. */ readonly register: (tools: Readonly>) => Effect.Effect } +export interface MaterializeInput { + readonly model: { readonly id: string; readonly provider: string } + readonly permissions?: PermissionV2.Ruleset +} + export interface Materialization { readonly definitions: ReadonlyArray readonly settle: (input: ExecuteInput) => Effect.Effect @@ -102,14 +107,19 @@ const registryLayer = Layer.effect( }), ) }), - materialize: Effect.fn("ToolRegistry.materialize")(function* (permissions = []) { + materialize: Effect.fn("ToolRegistry.materialize")(function* (input) { const registrations = new Map(applications.entries()) for (const [name, entries] of local) { const registration = entries.at(-1)?.registration if (registration) registrations.set(name, registration) } - for (const [name, registration] of registrations) - if (whollyDisabled(permission(registration.tool, name), permissions)) registrations.delete(name) + // OpenAI/GPT models use apply_patch; every other model uses edit and write. + const usePatch = input.model.provider.toLowerCase() === "openai" || input.model.id.toLowerCase().includes("gpt") + for (const [name, registration] of registrations) { + const wrongEditTool = name === "apply_patch" ? !usePatch : (name === "edit" || name === "write") && usePatch + if (wrongEditTool || whollyDisabled(permission(registration.tool, name), input.permissions ?? [])) + registrations.delete(name) + } return { definitions: Array.from(registrations, ([name, registration]) => definition(name, registration.tool)), settle: (input) => { diff --git a/packages/core/test/lib/tool.ts b/packages/core/test/lib/tool.ts index a711e311840..0639837163b 100644 --- a/packages/core/test/lib/tool.ts +++ b/packages/core/test/lib/tool.ts @@ -1,4 +1,5 @@ import { AgentV2 } from "@opencode-ai/core/agent" +import type { PermissionV2 } from "@opencode-ai/core/permission" import { SessionMessage } from "@opencode-ai/core/session/message" import { ToolRegistry } from "@opencode-ai/core/tool/registry" import { Effect } from "effect" @@ -8,13 +9,17 @@ export const toolIdentity = { assistantMessageID: SessionMessage.ID.make("msg_tool_test"), } +// Default fixture model: a non-OpenAI provider, so edit and write are the materialized edit tools. +export const testModel: ToolRegistry.MaterializeInput["model"] = { id: "claude-test", provider: "anthropic" } + export const toolDefinitions = ( registry: ToolRegistry.Interface, - permissions?: Parameters[0], -) => registry.materialize(permissions).pipe(Effect.map((materialized) => materialized.definitions)) + permissions?: PermissionV2.Ruleset, + model = testModel, +) => registry.materialize({ permissions, model }).pipe(Effect.map((materialized) => materialized.definitions)) -export const settleTool = (registry: ToolRegistry.Interface, input: ToolRegistry.ExecuteInput) => - registry.materialize().pipe(Effect.flatMap((materialized) => materialized.settle(input))) +export const settleTool = (registry: ToolRegistry.Interface, input: ToolRegistry.ExecuteInput, model = testModel) => + registry.materialize({ model }).pipe(Effect.flatMap((materialized) => materialized.settle(input))) -export const executeTool = (registry: ToolRegistry.Interface, input: ToolRegistry.ExecuteInput) => - settleTool(registry, input).pipe(Effect.map((settlement) => settlement.result)) +export const executeTool = (registry: ToolRegistry.Interface, input: ToolRegistry.ExecuteInput, model = testModel) => + settleTool(registry, input, model).pipe(Effect.map((settlement) => settlement.result)) diff --git a/packages/core/test/location-layer.test.ts b/packages/core/test/location-layer.test.ts index 35616256d9b..9d96add1d27 100644 --- a/packages/core/test/location-layer.test.ts +++ b/packages/core/test/location-layer.test.ts @@ -120,7 +120,6 @@ describe("LocationServiceMap", () => { expect(blockedState.providers.some((provider) => provider.id === ProviderV2.ID.make("test"))).toBe(false) expect(blockedState.tools.map((tool) => tool.name).sort()).toEqual([ "application_context", - "apply_patch", "edit", "glob", "grep", @@ -136,7 +135,6 @@ describe("LocationServiceMap", () => { expect(allowedState.providers.some((provider) => provider.id === ProviderV2.ID.make("test"))).toBe(true) expect(allowedState.tools.map((tool) => tool.name).sort()).toEqual([ "application_context", - "apply_patch", "edit", "glob", "grep", diff --git a/packages/core/test/session-runner-tool-registry.test.ts b/packages/core/test/session-runner-tool-registry.test.ts index d286c29285a..81d80e6f9f7 100644 --- a/packages/core/test/session-runner-tool-registry.test.ts +++ b/packages/core/test/session-runner-tool-registry.test.ts @@ -1,12 +1,13 @@ import { describe, expect } from "bun:test" import { Tool } from "@opencode-ai/core/tool/tool" import { AgentV2 } from "@opencode-ai/core/agent" +import type { PermissionV2 } from "@opencode-ai/core/permission" import { ApplicationTools } from "@opencode-ai/core/tool/application-tools" import { SessionV2 } from "@opencode-ai/core/session" import { SessionMessage } from "@opencode-ai/core/session/message" import { ToolOutputStore } from "@opencode-ai/core/tool-output-store" import { ToolRegistry } from "@opencode-ai/core/tool/registry" -import { executeTool, settleTool, toolDefinitions } from "./lib/tool" +import { executeTool, settleTool, testModel, toolDefinitions } from "./lib/tool" import { Cause, Deferred, Effect, Exit, Fiber, Layer, Option, Schema, SchemaGetter, SchemaIssue, Scope } from "effect" import { testEffect } from "./lib/effect" @@ -61,17 +62,11 @@ describe("ToolRegistry", () => { bash: make(), edit: make("edit"), write: make("edit"), - apply_patch: make("edit"), }) - const names = (rules: Parameters[0]) => - toolDefinitions(service, rules).pipe(Effect.map((definitions) => definitions.map((tool) => tool.name))) + const names = (permissions: PermissionV2.Ruleset) => + toolDefinitions(service, permissions).pipe(Effect.map((definitions) => definitions.map((tool) => tool.name))) - expect(yield* names([{ action: "question", resource: "*", effect: "deny" }])).toEqual([ - "bash", - "edit", - "write", - "apply_patch", - ]) + expect(yield* names([{ action: "question", resource: "*", effect: "deny" }])).toEqual(["bash", "edit", "write"]) expect( yield* names([ { action: "*", resource: "*", effect: "deny" }, @@ -88,6 +83,27 @@ describe("ToolRegistry", () => { }), ) + it.effect("selects one edit tool family for each model", () => + Effect.gen(function* () { + const service = yield* ToolRegistry.Service + yield* service.register({ + read: make(), + edit: make("edit"), + write: make("edit"), + apply_patch: make("edit"), + }) + const names = (model: ToolRegistry.MaterializeInput["model"]) => + service + .materialize({ model }) + .pipe(Effect.map((materialized) => materialized.definitions.map((tool) => tool.name))) + + expect(yield* names({ id: "gpt-5", provider: "openai" })).toEqual(["read", "apply_patch"]) + expect(yield* names({ id: "gpt-4o", provider: "opencode" })).toEqual(["read", "apply_patch"]) + expect(yield* names({ id: "computer-use-preview", provider: "openai" })).toEqual(["read", "apply_patch"]) + expect(yield* names({ id: "claude-sonnet-4", provider: "anthropic" })).toEqual(["read", "edit", "write"]) + }), + ) + it.effect("keeps permission decoration isolated between registrations", () => Effect.gen(function* () { const service = yield* ToolRegistry.Service @@ -183,7 +199,7 @@ describe("ToolRegistry", () => { }), }) expect( - yield* service.materialize().pipe( + yield* service.materialize({ model: testModel }).pipe( Effect.flatMap((materialized) => materialized.settle({ sessionID, @@ -201,7 +217,7 @@ describe("ToolRegistry", () => { Effect.gen(function* () { const service = yield* ToolRegistry.Service yield* service.register({ echo: make() }) - const materialized = yield* service.materialize() + const materialized = yield* service.materialize({ model: testModel }) const exit = yield* materialized.settle(call("echo", "call-retention-failure")).pipe(Effect.exit) expect(Exit.isFailure(exit)).toBe(true) @@ -331,7 +347,7 @@ describe("ToolRegistry", () => { Effect.gen(function* () { const service = yield* ToolRegistry.Service yield* service.register({ echo: make() }) - const materialized = yield* service.materialize() + const materialized = yield* service.materialize({ model: testModel }) expect((yield* materialized.settle(call("echo"))).result).toEqual({ type: "text", value: "echo" }) }), @@ -342,7 +358,7 @@ describe("ToolRegistry", () => { const service = yield* ToolRegistry.Service const scope = yield* Scope.make() yield* service.register({ echo: make() }).pipe(Scope.provide(scope)) - const materialized = yield* service.materialize() + const materialized = yield* service.materialize({ model: testModel }) yield* Scope.close(scope, Exit.void) expect((yield* materialized.settle(call("echo"))).result).toEqual({ @@ -356,7 +372,7 @@ describe("ToolRegistry", () => { Effect.gen(function* () { const service = yield* ToolRegistry.Service yield* service.register({ first: make(), second: make() }) - const materialized = yield* service.materialize() + const materialized = yield* service.materialize({ model: testModel }) yield* service.register({ first: make() }) expect((yield* materialized.settle(call("first"))).result).toEqual({ @@ -373,7 +389,7 @@ describe("ToolRegistry", () => { yield* service.register({ echo: make() }) const overlay = yield* Scope.make() yield* service.register({ echo: make() }).pipe(Scope.provide(overlay)) - const materialized = yield* service.materialize() + const materialized = yield* service.materialize({ model: testModel }) yield* Scope.close(overlay, Exit.void) expect((yield* materialized.settle(call("echo"))).result).toEqual({ @@ -388,7 +404,7 @@ describe("ToolRegistry", () => { const applications = yield* ApplicationTools.Service const service = yield* ToolRegistry.Service yield* applications.register({ echo: make() }) - const materialized = yield* service.materialize() + const materialized = yield* service.materialize({ model: testModel }) yield* service.register({ echo: make() }) expect((yield* materialized.settle(call("echo"))).result).toEqual({ @@ -405,7 +421,7 @@ describe("ToolRegistry", () => { yield* applications.register({ echo: make() }) const scope = yield* Scope.make() yield* service.register({ echo: make() }).pipe(Scope.provide(scope)) - const materialized = yield* service.materialize() + const materialized = yield* service.materialize({ model: testModel }) yield* Scope.close(scope, Exit.void) expect((yield* materialized.settle(call("echo"))).result).toEqual({ @@ -433,7 +449,7 @@ describe("ToolRegistry", () => { }), }) .pipe(Scope.provide(scope)) - const materialized = yield* service.materialize() + const materialized = yield* service.materialize({ model: testModel }) const settlement = yield* materialized.settle(call("echo")).pipe(Effect.forkChild) yield* Deferred.await(started) yield* Scope.close(scope, Exit.void) diff --git a/packages/core/test/tool-apply-patch.test.ts b/packages/core/test/tool-apply-patch.test.ts index 9b666f09170..304d062c7c6 100644 --- a/packages/core/test/tool-apply-patch.test.ts +++ b/packages/core/test/tool-apply-patch.test.ts @@ -109,6 +109,9 @@ const call = (patchText: string, id = "call-apply-patch") => ({ call: { type: "tool-call" as const, id, name: "apply_patch", input: { patchText } }, }) +// apply_patch is only materialized for OpenAI/GPT models. +const model = { id: "gpt-5", provider: "openai" } + const exists = (target: string) => Effect.promise(() => fs.stat(target).then( @@ -132,12 +135,15 @@ describe("ApplyPatchTool", () => { Effect.andThen( withTool(tmp.path, (registry) => Effect.gen(function* () { - expect((yield* toolDefinitions(registry)).map((tool) => tool.name)).toEqual(["apply_patch"]) + expect((yield* toolDefinitions(registry, undefined, model)).map((tool) => tool.name)).toEqual([ + "apply_patch", + ]) const settled = yield* settleTool( registry, call( "*** Begin Patch\n*** Add File: nested/new.txt\n+created\n*** Update File: update.txt\n@@\n-before\n+after\n*** Delete File: remove.txt\n*** End Patch", ), + model, ) expect(settled.result).toEqual({ type: "text", @@ -207,6 +213,7 @@ describe("ApplyPatchTool", () => { call( "*** Begin Patch\n*** Add File: created.txt\n+created\n*** Update File: old.txt\n*** Move to: moved.txt\n@@\n-before\n+after\n*** End Patch", ), + model, ), ).toEqual({ type: "error", value: "apply_patch moves are not supported yet" }) expect(yield* exists(path.join(tmp.path, "created.txt"))).toBe(false) @@ -234,6 +241,7 @@ describe("ApplyPatchTool", () => { yield* executeTool( registry, call(`*** Begin Patch\n*** Update File: ${target}\n@@\n-before\n+after\n*** End Patch`), + model, ), ).toMatchObject({ type: "text" }) expect(assertions.map((input) => input.action)).toEqual(["external_directory", "edit"]) @@ -270,6 +278,7 @@ describe("ApplyPatchTool", () => { call( `*** Begin Patch\n*** Update File: ${first}\n@@\n-before\n+after\n*** Update File: ${second}\n@@\n-before\n+after\n*** End Patch`, ), + model, ), ).toMatchObject({ type: "text" }) expect(assertions.map((input) => input.action)).toEqual(["external_directory", "edit"]) @@ -301,6 +310,7 @@ describe("ApplyPatchTool", () => { call( "*** Begin Patch\n*** Add File: created.txt\n+created\n*** Update File: missing.txt\n@@\n-before\n+after\n*** End Patch", ), + model, ), ).toEqual({ type: "error", value: "Unable to apply patch at missing.txt" }) expect(yield* exists(path.join(tmp.path, "created.txt"))).toBe(false) @@ -325,6 +335,7 @@ describe("ApplyPatchTool", () => { yield* executeTool( registry, call("*** Begin Patch\n*** Add File: existing.txt\n+replacement\n*** End Patch"), + model, ), ).toEqual({ type: "error", value: "Unable to apply patch at existing.txt" }) expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("sentinel\n") @@ -350,6 +361,7 @@ describe("ApplyPatchTool", () => { yield* executeTool( registry, call("*** Begin Patch\n*** Add File: appeared.txt\n+replacement\n*** End Patch"), + model, ), ).toEqual({ type: "error", value: "Unable to apply patch at appeared.txt" }) expect(yield* Effect.promise(() => fs.readFile(target, "utf8"))).toBe("winner\n") @@ -377,6 +389,7 @@ describe("ApplyPatchTool", () => { yield* executeTool( registry, call("*** Begin Patch\n*** Delete File: first.txt\n*** Delete File: second.txt\n*** End Patch"), + model, ).pipe(Effect.exit), ), ).toBe(true) @@ -408,6 +421,7 @@ describe("ApplyPatchTool", () => { const run = yield* executeTool( registry, call("*** Begin Patch\n*** Delete File: first.txt\n*** Delete File: second.txt\n*** End Patch"), + model, ).pipe(Effect.forkChild) yield* Deferred.await(removeStarted!) const interrupt = yield* Fiber.interrupt(run).pipe(Effect.forkChild) diff --git a/packages/core/test/tool-subagent.test.ts b/packages/core/test/tool-subagent.test.ts index 70dd693244a..1a1f2e8705e 100644 --- a/packages/core/test/tool-subagent.test.ts +++ b/packages/core/test/tool-subagent.test.ts @@ -23,7 +23,7 @@ import { ToolRegistry } from "@opencode-ai/core/tool/registry" import { ToolOutputStore } from "@opencode-ai/core/tool-output-store" import { tmpdir } from "./fixture/tmpdir" import { testEffect } from "./lib/effect" -import { executeTool, settleTool, toolIdentity } from "./lib/tool" +import { executeTool, settleTool, testModel, toolIdentity } from "./lib/tool" const childText = "child final response" const childModel = ModelV2.Ref.make({ id: ModelV2.ID.make("child"), providerID: ProviderV2.ID.make("test") }) @@ -142,7 +142,9 @@ describe("SubagentTool", () => { const locations = yield* LocationServiceMap.Service const registry = yield* ToolRegistry.Service.pipe(Effect.provide(locations.get(parent.location))) - expect((yield* registry.materialize()).definitions.map((tool) => tool.name)).toContain(SubagentTool.name) + expect((yield* registry.materialize({ model: testModel })).definitions.map((tool) => tool.name)).toContain( + SubagentTool.name, + ) expect( yield* executeTool(registry, { sessionID: parent.id,