Ver Fonte

fix(core): gate v2 edit tools by model (#34558)

Co-authored-by: Aiden Cline <aidenpcline@gmail.com>
opencode-agent[bot] há 1 mês atrás
pai
commit
524ee8fc03

+ 3 - 1
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,

+ 14 - 4
packages/core/src/tool/registry.ts

@@ -21,11 +21,16 @@ export type ExecuteInput = {
 }
 
 export interface Interface {
-  readonly materialize: (permissions?: PermissionV2.Ruleset) => Effect.Effect<Materialization>
+  readonly materialize: (input: MaterializeInput) => Effect.Effect<Materialization>
   /** Internal registration capability exposed publicly only through Tools.Service. */
   readonly register: (tools: Readonly<Record<string, AnyTool>>) => Effect.Effect<void, RegistrationError, Scope.Scope>
 }
 
+export interface MaterializeInput {
+  readonly model: { readonly id: string; readonly provider: string }
+  readonly permissions?: PermissionV2.Ruleset
+}
+
 export interface Materialization {
   readonly definitions: ReadonlyArray<ToolDefinition>
   readonly settle: (input: ExecuteInput) => Effect.Effect<Settlement, ToolOutputStore.Error>
@@ -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) => {

+ 11 - 6
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<typeof registry.materialize>[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))

+ 0 - 2
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",

+ 36 - 20
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<ToolRegistry.Interface["materialize"]>[0]) =>
-        toolDefinitions(service, rules).pipe(Effect.map((definitions) => definitions.map((tool) => tool.name)))
-
-      expect(yield* names([{ action: "question", resource: "*", effect: "deny" }])).toEqual([
-        "bash",
-        "edit",
-        "write",
-        "apply_patch",
-      ])
+      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"])
       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)

+ 15 - 1
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)

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