From b8ee2396d572c9e34490fd6999a5c3f825819966 Mon Sep 17 00:00:00 2001 From: Dax Raad Date: Mon, 27 Jul 2026 15:58:46 -0400 Subject: [PATCH] fix(core): drop invalid optional tool inputs --- packages/core/src/tool/runtime.ts | 40 ++++++++++++++++++++++++-- packages/core/test/tool-schema.test.ts | 39 +++++++++++++++++++++---- packages/core/test/tool-search.test.ts | 25 +++++----------- 3 files changed, 77 insertions(+), 27 deletions(-) diff --git a/packages/core/src/tool/runtime.ts b/packages/core/src/tool/runtime.ts index 5c5b767c8c..dc28a04526 100644 --- a/packages/core/src/tool/runtime.ts +++ b/packages/core/src/tool/runtime.ts @@ -1,7 +1,7 @@ import type { ToolDefinition } from "@opencode-ai/ai" import { Tool } from "@opencode-ai/schema/tool" import type { StandardJSONSchemaV1, StandardSchemaV1 } from "@standard-schema/spec" -import { Effect, JsonSchema, Schema } from "effect" +import { Effect, JsonSchema, Schema, SchemaAST, SchemaIssue } from "effect" export const definition = (tool: Tool.Info): ToolDefinition => ({ name: effectiveName(tool), @@ -33,18 +33,52 @@ export const execute = (tool: Tool.Info, input: unknown, context: Tool const decodeInput = (schema: Tool.ValueSchema, value: unknown) => { if (Schema.isSchema(schema)) - return Schema.decodeUnknownEffect(schema)(value).pipe( + return Schema.decodeUnknownEffect(schema)(value, { errors: "all" }).pipe( + Effect.catch((error) => { + const input = dropInvalidOptionalFields(schema, value, error) + return input === value ? Effect.fail(error) : Schema.decodeUnknownEffect(schema)(input) + }), Effect.mapError((error) => new Tool.Error({ message: `Invalid tool input: ${error.message}` })), ) if (isStandardSchema(schema)) return validateStandard(schema, value, "Invalid tool input") return Effect.succeed(value) } +const dropInvalidOptionalFields = (schema: Schema.Codec, value: unknown, error: unknown) => { + if ( + schema.ast._tag !== "Objects" || + typeof value !== "object" || + value === null || + Array.isArray(value) || + !Schema.isSchemaError(error) + ) + return value + const optional = new Set( + schema.ast.propertySignatures + .filter((field) => SchemaAST.isOptional(field.type)) + .map((field) => field.name) + .filter((key): key is string => typeof key === "string"), + ) + const invalid = new Set( + issueKeys(error.issue).filter((key): key is string => typeof key === "string" && optional.has(key)), + ) + if (invalid.size === 0) return value + return Object.fromEntries(Object.entries(value).filter(([key]) => !invalid.has(key))) +} + +const issueKeys = (issue: SchemaIssue.Issue): ReadonlyArray => { + if (issue._tag === "Pointer") return issue.path.slice(0, 1) + if (issue._tag === "Composite" || issue._tag === "AnyOf") return issue.issues.flatMap(issueKeys) + if (issue._tag === "Encoding" || issue._tag === "Filter") return issueKeys(issue.issue) + return [] +} + const encodeOutput = (schema: Tool.ValueSchema, value: unknown) => { if (Schema.isSchema(schema)) return Schema.encodeEffect(schema)(value).pipe( Effect.mapError( - (error) => new Tool.Error({ message: `Tool returned an invalid value for its output schema: ${error.message}` }), + (error) => + new Tool.Error({ message: `Tool returned an invalid value for its output schema: ${error.message}` }), ), ) if (isStandardSchema(schema)) diff --git a/packages/core/test/tool-schema.test.ts b/packages/core/test/tool-schema.test.ts index 4008e0a8c4..6b77725825 100644 --- a/packages/core/test/tool-schema.test.ts +++ b/packages/core/test/tool-schema.test.ts @@ -60,13 +60,13 @@ test("portable schemas validate and describe typed tools", async () => { }, }, } - const tool: Info = ({ + const tool: Info = { name: "portable", description: "Portable tool", input, output, execute: ({ count }) => Effect.succeed({ output: count + 1 }), - }) + } expect(definition(tool)).toEqual({ name: "portable", @@ -106,16 +106,43 @@ test("portable schema failures become tool failures", async () => { expect(error.toString()).toContain("Invalid tool input: expected a string") }) +test("invalid optional Effect schema fields are dropped", async () => { + const tool: Info = { + name: "optional", + description: "Optional", + input: Schema.Struct({ value: Schema.String, limit: Schema.optional(Schema.Number) }), + execute: (input) => Effect.succeed({ content: JSON.stringify(input) }), + } + + expect(await Effect.runPromise(execute(tool, { value: "ok", limit: "many" }, {} as Tool.Context))).toEqual({ + output: undefined, + content: [{ type: "text", text: '{"value":"ok"}' }], + }) +}) + +test("invalid required Effect schema fields still fail", async () => { + const tool: Info = { + name: "required", + description: "Required", + input: Schema.Struct({ value: Schema.String, limit: Schema.optional(Schema.Number) }), + execute: () => Effect.succeed({ content: "unused" }), + } + + const error = await Effect.runPromiseExit(execute(tool, { value: 1, limit: "many" }, {} as Tool.Context)) + expect(error.toString()).toContain("Invalid tool input") + expect(error.toString()).toContain('["value"]') +}) + test("canonical results carry metadata with typed output", async () => { const input = Schema.Struct({ value: Schema.String }) const output = Schema.Struct({ value: Schema.String, internal: Schema.Boolean }) - const tool: Info = ({ + const tool: Info = { name: "annotated", description: "Annotated tool", input, output, execute: ({ value }) => Effect.succeed({ output: { value, internal: true }, metadata: { value }, content: value }), - }) + } expect(await Effect.runPromise(tool.execute({ value: "out" }, {} as Tool.Context))).toEqual({ output: { value: "out", internal: true }, @@ -126,12 +153,12 @@ test("canonical results carry metadata with typed output", async () => { test("raw JSON schemas are render-only and omitted output means model-only", async () => { const input = { type: "object", properties: { value: { type: "string" } } } - const tool: Info = ({ + const tool: Info = { name: "raw", description: "Raw tool", input, execute: (input) => Effect.succeed({ content: JSON.stringify(input) }), - }) + } expect(definition(tool)).toEqual({ name: "raw", diff --git a/packages/core/test/tool-search.test.ts b/packages/core/test/tool-search.test.ts index b88f4e7e88..0f61992e82 100644 --- a/packages/core/test/tool-search.test.ts +++ b/packages/core/test/tool-search.test.ts @@ -24,14 +24,7 @@ import { executeTool, registerToolPlugin, toolIdentity } from "./lib/tool" const globToolNode = makeLocationNode({ name: "test/glob-tool-plugin", layer: Layer.effectDiscard(registerToolPlugin(GlobTool.Plugin)), - deps: [ - Tool.node, - FSUtil.node, - Ripgrep.node, - Location.node, - LocationMutation.node, - Permission.node, - ], + deps: [Tool.node, FSUtil.node, Ripgrep.node, Location.node, LocationMutation.node, Permission.node], }) const grepToolNode = makeLocationNode({ name: "test/grep-tool-plugin", @@ -84,7 +77,7 @@ const call = (name: "glob" | "grep", input: unknown) => ({ const it = testEffect(Layer.empty) describe("search tools", () => { - it.live("bounds omitted glob and grep limits", () => + it.live("bounds omitted and invalid optional search limits", () => Effect.acquireUseRelease( Effect.promise(() => tmpdir()), (tmp) => @@ -99,9 +92,11 @@ describe("search tools", () => { yield* withTools(tmp.path, (registry) => Effect.gen(function* () { const glob = yield* executeTool(registry, call("glob", { pattern: "*" })) + const invalidGlob = yield* executeTool(registry, call("glob", { pattern: "*", limit: "many" })) const grep = yield* executeTool(registry, call("grep", { pattern: "needle" })) expect(glob.metadata).toEqual({ count: FileSystem.DEFAULT_SEARCH_LIMIT, truncated: true }) + expect(invalidGlob.metadata).toEqual({ count: FileSystem.DEFAULT_SEARCH_LIMIT, truncated: true }) expect(grep.metadata).toEqual({ matches: FileSystem.DEFAULT_SEARCH_LIMIT, truncated: true }) expect(glob.content).toHaveLength(1) expect(grep.content).toHaveLength(1) @@ -186,9 +181,7 @@ describe("search tools", () => { Effect.promise(() => tmpdir()), (tmp) => Effect.promise(() => fs.writeFile(path.join(tmp.path, "file.txt"), "haystack\n")).pipe( - Effect.andThen( - withTools(tmp.path, (registry) => executeTool(registry, call("grep", { pattern: "needle" }))), - ), + Effect.andThen(withTools(tmp.path, (registry) => executeTool(registry, call("grep", { pattern: "needle" })))), Effect.tap((result) => Effect.sync(() => { expect(result).toMatchObject({ @@ -297,9 +290,7 @@ describe("search tools", () => { (tmp) => Effect.promise(() => fs.writeFile(path.join(tmp.path, "file.txt"), "content\n")).pipe( Effect.andThen( - withTools(tmp.path, (registry) => - executeTool(registry, call("glob", { path: "file.txt", pattern: "*" })), - ), + withTools(tmp.path, (registry) => executeTool(registry, call("glob", { path: "file.txt", pattern: "*" }))), ), Effect.tap((result) => Effect.sync(() => { @@ -331,9 +322,7 @@ describe("search tools", () => { Effect.sync(() => { expect(result.status).toBe("completed") expect(assertions.map((input) => input.action)).toEqual(["external_directory", "glob"]) - expect(assertions[0]?.resources).toEqual([ - path.join(outside.path, "*").replaceAll("\\", "/"), - ]) + expect(assertions[0]?.resources).toEqual([path.join(outside.path, "*").replaceAll("\\", "/")]) }), ), )