From af1174b6c30b955ace986e7056b1d055186ccfdc Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Thu, 15 Oct 2026 10:54:19 -0700 Subject: [PATCH 1/4] fix(tools): normalize optional fillers before validation Strict-schema clients send every optional property, using "", [], 0 or null for the ones they do not intend to set. These were validated as real values, so valid calls were rejected or got unsafe behavior (e.g. a zero query timeout disables the deadline). - add `fillerSafeParameters`: top-level optional `[]`, `""`, `0`, `null` are treated as omitted before validation and defaults - apply it in the shared tool wrapper, the legacy zod adapter and plugin tools - SQL tools: filler timeouts keep the configured deadline, zero row limit uses the default, oversized row limits and byte budgets clamp - `sql_check`: coverage still requires an explicit `pattern` - add regression tests for registry-wide fillers and the SQL tools Co-Authored-By: Claude Opus 5.5 --- .../opencode/src/altimate/tool-zod-compat.ts | 17 +- .../opencode/src/altimate/tools/sql-check.ts | 19 +- .../src/altimate/tools/sql-execute.ts | 8 +- .../src/altimate/tools/sql-materialize.ts | 3 +- .../opencode/src/tool/filler-parameters.ts | 54 ++++ packages/opencode/src/tool/registry.ts | 19 +- packages/opencode/src/tool/tool.ts | 16 +- .../test/altimate/sql-materialize.test.ts | 73 ++++- .../test/altimate/tools/sql-check.test.ts | 37 ++- .../test/altimate/tools/sql-execute.test.ts | 47 ++- .../test/tool/filler-parameters.test.ts | 304 ++++++++++++++++++ 11 files changed, 549 insertions(+), 48 deletions(-) create mode 100644 packages/opencode/src/tool/filler-parameters.ts create mode 100644 packages/opencode/test/tool/filler-parameters.test.ts diff --git a/packages/opencode/src/altimate/tool-zod-compat.ts b/packages/opencode/src/altimate/tool-zod-compat.ts index 6425f77c9f..2c83a2983c 100644 --- a/packages/opencode/src/altimate/tool-zod-compat.ts +++ b/packages/opencode/src/altimate/tool-zod-compat.ts @@ -22,6 +22,7 @@ import { SessionV1 } from "@opencode-ai/core/v1/session" import type { Agent } from "@/agent/agent" import type { SessionID, MessageID } from "../session/schema" import type { Context as NewContext, DefWithoutID } from "../tool/tool" +import { fillerSafeParameters } from "../tool/filler-parameters" export type Metadata = { [key: string]: any; error?: string } @@ -173,6 +174,7 @@ export function zodToJsonSchema(schema: z.ZodType): JSONSchema7 { /** Build a `DefWithoutID` from a resolved legacy tool object (no Effect wrapper). */ function legacyDefToDef(legacy: LegacyToolDef): DefWithoutID { + const validationSchema = zodToJsonSchema(legacy.parameters) return { description: legacy.description, // altimate_change start — decode THROUGH zod's `parse` so defaults, coercions and transforms @@ -180,16 +182,19 @@ function legacyDefToDef(legacy: LegacyToolDef): DefWithoutID { // Schema.declare only validates and passes the raw input through, silently dropping zod // `.default()`/`.transform()`. Validation failures still surface as Effect SchemaErrors whose // nested issue.actual carries the original input (used by formatValidationError below). - parameters: Schema.declare((u): u is unknown => legacy.parameters.safeParse(u).success).pipe( - Schema.decodeTo(Schema.Unknown, { - decode: SchemaGetter.transform((u: unknown) => legacy.parameters.parse(u)), - encode: SchemaGetter.transform((u: unknown) => u), - }), + parameters: fillerSafeParameters( + Schema.declare((u): u is unknown => legacy.parameters.safeParse(u).success).pipe( + Schema.decodeTo(Schema.Unknown, { + decode: SchemaGetter.transform((u: unknown) => legacy.parameters.parse(u)), + encode: SchemaGetter.transform((u: unknown) => u), + }), + ), + validationSchema, ), // altimate_change end // altimate_change start — upstream_fix: use an explicit JSON schema when a legacy // tool needs hidden compatibility arguments that should not be advertised. - jsonSchema: legacy.jsonSchema ?? zodToJsonSchema(legacy.parameters), + jsonSchema: legacy.jsonSchema ?? validationSchema, // altimate_change end ...(legacy.formatValidationError ? { diff --git a/packages/opencode/src/altimate/tools/sql-check.ts b/packages/opencode/src/altimate/tools/sql-check.ts index 78fb47ed7d..a47fae0705 100644 --- a/packages/opencode/src/altimate/tools/sql-check.ts +++ b/packages/opencode/src/altimate/tools/sql-check.ts @@ -1,5 +1,6 @@ import z from "zod" import { Tool } from "../../tool/tool" +import { zodToJsonSchema } from "../tool-zod-compat" import * as Precedence from "../workspace/precedence" import * as Registry from "../native/connections/registry" import { classifyAndCheck, isSingleQuery } from "./sql-classify" @@ -60,12 +61,24 @@ const publicParameters = z right_col: z.string().optional().describe("Right output column for keys"), left_warehouse: z.string().optional().describe("Optional assertion: must equal warehouse"), right_warehouse: z.string().optional().describe("Optional assertion: must equal warehouse"), - query_timeout_ms: z.number().int().min(0).max(3_600_000).optional(), + query_timeout_ms: z + .number() + .int() + .min(0) + .max(3_600_000) + .optional() + .describe("Query deadline in ms; omitted or 0 uses the common connection policy"), }) .strict() export type SqlCheckPublicInput = z.input +// Coverage requires a pattern, and an empty pattern is meaningful for that check. +const toolParameters = z.discriminatedUnion("check", [ + publicParameters.extend({ check: z.literal("coverage"), pattern: publicParameters.shape.pattern.unwrap() }), + publicParameters.extend({ check: z.enum(["fanout", "keys", "grain", "nulls"]) }), +]) + /** * Treat "", [] and null as absent so strict-schema callers can leave unused fields empty. * A coverage check's `pattern` is the one field where "" is a value (match empty strings). @@ -395,7 +408,9 @@ export const SqlCheckTool = Tool.define("sql_check", { "Use sql_materialize to put cross-warehouse or other-engine sources into scratch first. " + "Key normalization is diagnostic and may collapse unrelated values; no automatic fixes. Coverage uses regex except SQLite's explicit LIKE fallback. " + "Leave fields a check does not use empty.", - parameters: publicParameters, + parameters: toolParameters, + // Strict-schema providers require a flat object at the root of the wire schema. + jsonSchema: zodToJsonSchema(publicParameters), async execute(args, ctx) { try { // altimate_change start — validate and authorize reads before workspace redirects diff --git a/packages/opencode/src/altimate/tools/sql-execute.ts b/packages/opencode/src/altimate/tools/sql-execute.ts index e1080e2861..3f40e7af7e 100644 --- a/packages/opencode/src/altimate/tools/sql-execute.ts +++ b/packages/opencode/src/altimate/tools/sql-execute.ts @@ -41,7 +41,7 @@ import { normalizeError } from "./response-normalization" // altimate_change end const MAX_SQL_ROWS = 10_000 -const LIMIT_ERROR = `limit must be a finite positive integer between 1 and ${MAX_SQL_ROWS}` +const LIMIT_ERROR = "limit must be a finite positive integer" const parameters = z.object({ query: z.string().describe("SQL query to execute"), @@ -50,10 +50,10 @@ const parameters = z.object({ .number({ error: LIMIT_ERROR }) .int(LIMIT_ERROR) .min(1, LIMIT_ERROR) - .max(MAX_SQL_ROWS, LIMIT_ERROR) + .transform((limit) => Math.min(limit, MAX_SQL_ROWS)) .optional() .default(100) - .describe(`Max rows to return (1-${MAX_SQL_ROWS}; default 100)`), + .describe(`Max rows to return (default 100; larger values are capped at ${MAX_SQL_ROWS})`), query_timeout_ms: z .number() .int() @@ -61,7 +61,7 @@ const parameters = z.object({ .max(3_600_000) .optional() .describe( - "Optional per-query execution deadline in ms. Omit to use the configured deadline (connection setting or environment). Set only when a specific query needs a longer or shorter budget; 0 disables.", + "Optional per-query execution deadline in ms. Omit or use 0 to use the configured deadline (connection setting or environment). Set a positive value only when a specific query needs a longer or shorter budget.", ), }) diff --git a/packages/opencode/src/altimate/tools/sql-materialize.ts b/packages/opencode/src/altimate/tools/sql-materialize.ts index 0db711616f..17ada5fda0 100644 --- a/packages/opencode/src/altimate/tools/sql-materialize.ts +++ b/packages/opencode/src/altimate/tools/sql-materialize.ts @@ -545,7 +545,6 @@ export const SqlMaterializeTool = Tool.define("sql_materialize", { .number() .int() .min(0) - .max(HARD_MAX_BYTES) .optional() .describe("Serialized row byte cap enforced during transfer (default 256 MiB, hard limit 2 GiB; 0 = default)"), query_timeout_ms: z @@ -554,7 +553,7 @@ export const SqlMaterializeTool = Tool.define("sql_materialize", { .min(0) .max(3_600_000) .optional() - .describe("Source query deadline; uses the common connection policy when omitted (0 disables it)"), + .describe("Source query deadline in ms; omitted or 0 uses the common connection policy"), }), async execute(args, ctx): Promise<{ title: string; metadata: Record; output: string }> { // altimate_change start — validate and authorize source reads before workspace redirects diff --git a/packages/opencode/src/tool/filler-parameters.ts b/packages/opencode/src/tool/filler-parameters.ts new file mode 100644 index 0000000000..3926343ef2 --- /dev/null +++ b/packages/opencode/src/tool/filler-parameters.ts @@ -0,0 +1,54 @@ +import type { JSONSchema7 } from "@ai-sdk/provider" +import { Schema, SchemaGetter } from "effect" + +const normalized = new WeakSet() + +/** Normalize only optional tool arguments; required values and nested payloads retain their meaning. */ +export function fillerSafeParameters(parameters: Schema.Decoder, jsonSchema: JSONSchema7): Schema.Decoder { + // Legacy tools may publish a simpler wire schema than their validation schema. + if (normalized.has(parameters)) return parameters + const result = Schema.Unknown.pipe( + Schema.decodeTo(parameters, { + decode: SchemaGetter.transform((input) => { + if (!input || typeof input !== "object" || Array.isArray(input)) return input + const args = input as Record + const properties = new Set() + const required = new Set() + const collect = (schema: JSONSchema7 | boolean) => { + if (typeof schema === "boolean") return + for (const name of Object.keys(schema.properties ?? {})) properties.add(name) + for (const name of schema.required ?? []) required.add(name) + for (const child of schema.allOf ?? []) collect(child) + for (const child of schema.anyOf ?? schema.oneOf ?? []) { + // Literal discriminants determine which arguments are required for this call. + if ( + typeof child !== "boolean" && + Object.entries(child.properties ?? {}).some( + ([key, field]) => + typeof field !== "boolean" && + child.required?.includes(key) && + key in args && + ((field.const !== undefined && field.const !== args[key]) || + (field.enum !== undefined && !field.enum.includes(args[key] as never))), + ) + ) + continue + collect(child) + } + } + collect(jsonSchema) + return Object.fromEntries( + Object.entries(args).filter( + ([key, value]) => + !properties.has(key) || + required.has(key) || + !(value === null || value === "" || value === 0 || (Array.isArray(value) && value.length === 0)), + ), + ) + }), + encode: SchemaGetter.transform((value) => value), + }), + ) + normalized.add(result) + return result +} diff --git a/packages/opencode/src/tool/registry.ts b/packages/opencode/src/tool/registry.ts index a33b2a2e32..c4a7373f59 100644 --- a/packages/opencode/src/tool/registry.ts +++ b/packages/opencode/src/tool/registry.ts @@ -43,6 +43,11 @@ import { AppRuntime } from "@/effect/app-runtime" // altimate_change start — upstream_fix: task.background schema follows RuntimeFlags. import { RuntimeFlags } from "@/effect/runtime-flags" // altimate_change end +// altimate_change start — upstream_fix: plugin tools share the optional filler contract +import { Schema } from "effect" +import { fillerSafeParameters } from "./filler-parameters" +import { ToolJsonSchema } from "./json-schema" +// altimate_change end // altimate_change start — Effect Context.Service facade so v1.17.9 consumers that compose // ToolRegistry into the Effect runtime (yield* ToolRegistry.Service / .defaultLayer / .node) // compile. Delegates to the existing namespace functions; behavior preserved. @@ -157,6 +162,18 @@ import { TrainingImportTool } from "../altimate/tools/training-import" import { DbtPrReviewTool } from "../altimate/tools/dbt-pr-review" // altimate_change end +// altimate_change start — upstream_fix: plugin tools share the optional filler contract +function fillerSafePlugin(tool: Tool.DefWithoutID): Tool.DefWithoutID { + const parameters = fillerSafeParameters(tool.parameters, ToolJsonSchema.fromTool({ ...tool, id: "plugin" })) + const decode = Schema.decodeUnknownEffect(parameters) + return { + ...tool, + parameters, + execute: (args, ctx) => decode(args).pipe(Effect.orDie, Effect.flatMap((decoded) => tool.execute(decoded, ctx))), + } +} +// altimate_change end + export namespace ToolRegistry { const log = Log.create({ service: "tool.registry" }) @@ -275,7 +292,7 @@ export namespace ToolRegistry { ...(result.attachments ? { attachments: result.attachments as any } : {}), } }, - }), + }).pipe(Effect.map(fillerSafePlugin)), } } // altimate_change end diff --git a/packages/opencode/src/tool/tool.ts b/packages/opencode/src/tool/tool.ts index e0efaec713..78ba47288d 100644 --- a/packages/opencode/src/tool/tool.ts +++ b/packages/opencode/src/tool/tool.ts @@ -23,6 +23,10 @@ import { // altimate_change start — humanize tool-call titles at the source import { describeToolCall } from "../altimate/tool-label" // altimate_change end +// altimate_change start — upstream_fix: normalize optional strict-schema fillers before validation +import { fillerSafeParameters } from "./filler-parameters" +import { ToolJsonSchema } from "./json-schema" +// altimate_change end interface Metadata { [key: string]: any @@ -269,9 +273,15 @@ function wrap, Result extends Metadat Effect.gen(function* () { const toolInfo = typeof init === "function" ? { ...(yield* init(ctx)) } : { ...init } // altimate_change end + // altimate_change start — upstream_fix: share filler handling across all tool schemas + const jsonSchema = ToolJsonSchema.fromTool({ ...toolInfo, id }) + const parameters = fillerSafeParameters(toolInfo.parameters, jsonSchema) + // altimate_change end // Compile the parser closure once per tool init; `decodeUnknownEffect` // allocates a new closure per call, so hoisting avoids re-closing it. - const decode = Schema.decodeUnknownEffect(toolInfo.parameters) + // altimate_change start — upstream_fix: validate after optional fillers become omissions + const decode = Schema.decodeUnknownEffect(parameters) + // altimate_change end const execute = toolInfo.execute toolInfo.execute = (args, ctx) => { const attrs = { @@ -333,7 +343,9 @@ function wrap, Result extends Metadat } }).pipe(Effect.orDie, Effect.withSpan("Tool.execute", { attributes: attrs })) } - return toolInfo + // altimate_change start — upstream_fix: expose the same decoder to schema and batch callers + return { ...toolInfo, parameters, jsonSchema } + // altimate_change end }) } diff --git a/packages/opencode/test/altimate/sql-materialize.test.ts b/packages/opencode/test/altimate/sql-materialize.test.ts index 4263aa23d8..291bd2c1f3 100644 --- a/packages/opencode/test/altimate/sql-materialize.test.ts +++ b/packages/opencode/test/altimate/sql-materialize.test.ts @@ -630,26 +630,34 @@ describe("sql_materialize", () => { }) } - test("the tool rejects max_bytes above the hard 2 GiB ceiling before querying", async () => { + test("the tool clamps oversized max_bytes to the hard 2 GiB ceiling during preflight", async () => { const seen: string[] = [] const get = fakePostgres(async (sql) => { seen.push(sql) - throw new Error("No query should run") + if (/^SELECT count\(\*\)/i.test(sql)) { + return { columns: ["count"], rows: [[1_000_000]], row_count: 1, truncated: false } + } + if (sql.endsWith("LIMIT 1000")) { + return { columns: ["description"], rows: [["x".repeat(4096)]], row_count: 1, truncated: false } + } + throw new Error("Extraction must not run") }) try { const tool = await initTool(SqlMaterializeTool) - await expect( - tool.execute( - { - source_warehouse: "pg_source", - sql: "SELECT 1 AS n", - target_table: "invalid_cap", - max_bytes: 2 * 1024 ** 3 + 1, - }, - ctx as any, - ), - ).rejects.toThrow(/invalid arguments|SchemaError|Expected/i) - expect(seen).toHaveLength(0) + const result = await tool.execute( + { + source_warehouse: "pg_source", + sql: "SELECT description FROM languages", + target_table: "capped_transfer", + max_bytes: 4 * 1024 ** 3, + }, + ctx as any, + ) + expect(result.title).toContain("ERROR") + expect(result.output).toContain("2.0 GiB bound") + expect(seen).toHaveLength(2) + expect(seen[0]).toMatch(/^SELECT count\(\*\)/) + expect(seen[1]).toMatch(/LIMIT 1000$/) } finally { get.mockRestore() } @@ -719,6 +727,43 @@ describe("sql_materialize", () => { } }) + for (const [label, query_timeout_ms, expected] of [ + ["omitted", undefined, undefined], + ["zero", 0, undefined], + ["empty array", [], undefined], + ["empty string", "", undefined], + ["null", null, undefined], + ["positive", 4321, 4321], + ] as const) { + test(`the registered tool normalizes ${label} deadlines before running source queries`, async () => { + const seen: { sql: string; timeout: number | undefined }[] = [] + const get = fakePostgres(async (sql, _limit, _binds, options) => { + seen.push({ sql, timeout: options?.queryTimeoutMs }) + return { columns: ["n"], rows: [[1]], row_count: 1, truncated: false } + }) + try { + const tool = await initTool(SqlMaterializeTool) + const result = await tool.execute( + { + source_warehouse: "pg_source", + sql: "SELECT 1 AS n", + target_table: "deadline", + query_timeout_ms, + }, + ctx as any, + ) + expect(result.metadata.error).toBeUndefined() + expect(result.metadata.rowCount).toBe(1) + expect(seen).toHaveLength(3) + expect(seen[0].sql).toMatch(/^SELECT count\(\*\)/) + expect(seen[1].sql).toMatch(/LIMIT 1000$/) + expect(seen.map((query) => query.timeout)).toEqual([expected, expected, expected]) + } finally { + get.mockRestore() + } + }) + } + test("a preflight timeout stops before sampling or extraction", async () => { const seen: string[] = [] const get = fakePostgres(async (sql) => { diff --git a/packages/opencode/test/altimate/tools/sql-check.test.ts b/packages/opencode/test/altimate/tools/sql-check.test.ts index ee02e74997..dd6df349dc 100644 --- a/packages/opencode/test/altimate/tools/sql-check.test.ts +++ b/packages/opencode/test/altimate/tools/sql-check.test.ts @@ -1,6 +1,6 @@ import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, spyOn, test } from "bun:test" import { Database } from "bun:sqlite" -import { Effect } from "effect" +import { Effect, Schema } from "effect" import path from "path" import { tmpdir } from "../../fixture/fixture" import { testEffect } from "../../lib/effect" @@ -82,6 +82,15 @@ const leftSql = "SELECT * FROM orders" const rightSql = "SELECT * FROM customers" const sevenKeys = Array.from({ length: 7 }, (_, i) => `SELECT ${i + 1} AS id`).join(" UNION ALL ") +test("publishes an object schema while requiring a coverage pattern at validation", async () => { + const tool = await initTool(SqlCheckTool) + expect(tool.jsonSchema).toMatchObject({ type: "object", properties: { pattern: { type: "string" } } }) + expect(tool.jsonSchema).not.toHaveProperty("anyOf") + const decode = Schema.decodeUnknownSync(tool.parameters) + expect(() => decode({ warehouse: "sqlite_fixture", check: "coverage", sql: "SELECT '' AS note", column: "note" })).toThrow() + expect(decode({ warehouse: "sqlite_fixture", check: "grain" })).toEqual({ warehouse: "sqlite_fixture", check: "grain" }) +}) + testEffect(Agent.defaultLayer).instance( "analyst and data-explorer can invoke sql_check while SQL writes remain denied", () => @@ -355,6 +364,32 @@ for (const warehouse of ["sqlite_fixture", "scratch"] as const) { expect(result.title).not.toContain("ERROR") }) + test("registered coverage checks preserve empty patterns and default filler deadlines", async () => { + const source = warehouse === "scratch" ? await getScratch("test-session") : await Registry.get(warehouse) + const execute = spyOn(source, "execute") + const tool = await initTool(SqlCheckTool) + try { + for (const query_timeout_ms of [undefined, [], "", 0, null, 4321]) { + const result = await tool.execute( + { warehouse, check: "coverage", sql: "SELECT '' AS note", column: "note", pattern: "", query_timeout_ms }, + { + sessionID: "test-session", + messageID: "test-message", + agent: "test", + abort: new AbortController().signal, + messages: [], + }, + ) + expect(result.title).not.toContain("ERROR") + expect(result.metadata).toMatchObject({ check: "coverage", matching_rows: 1 }) + expect(execute.mock.calls.at(-1)?.[3]?.queryTimeoutMs).toBe(query_timeout_ms === 4321 ? 4321 : undefined) + } + expect(execute).toHaveBeenCalledTimes(6) + } finally { + execute.mockRestore() + } + }) + test("still reports missing required fields after empty values are dropped", async () => { await expect(checkSql({ warehouse, check: "grain", sql: leftSql, columns: [], column: "" })).rejects.toThrow( /columns/, diff --git a/packages/opencode/test/altimate/tools/sql-execute.test.ts b/packages/opencode/test/altimate/tools/sql-execute.test.ts index e3c8379683..5721e9ecaa 100644 --- a/packages/opencode/test/altimate/tools/sql-execute.test.ts +++ b/packages/opencode/test/altimate/tools/sql-execute.test.ts @@ -211,28 +211,41 @@ describe("sql_execute parser read boundary", () => { }) describe("sql_execute limit validation", () => { - for (const limit of [0, -1, 1.5, 1e9, NaN, Infinity, -Infinity]) { + for (const limit of [-1, 1.5, NaN, Infinity, -Infinity]) { test(`rejects ${limit} before querying`, async () => { const tool = await initTool(SqlExecuteTool) await expect(tool.execute({ query: "SELECT 42", limit }, ctx)).rejects.toThrow( - "limit must be a finite positive integer between 1 and 10000", + "limit must be a finite positive integer", ) expect(dispatch).not.toHaveBeenCalled() }) } - test("keeps default 100 and accepts both bounds without extra queries", async () => { + test("keeps default 100 for omitted and filler limits and accepts both bounds without extra queries", async () => { const tool = await initTool(SqlExecuteTool) - for (const limit of [undefined, 1, 10_000]) { + for (const limit of [undefined, 0, 1, 10_000]) { const output = await tool.execute({ query: "SELECT 42", limit }, ctx) expect(output.output).toBe("answer\n------\n42\n\n(1 rows)") } expect(dispatch.mock.calls).toEqual([ + ["sql.execute", { sql: "SELECT 42", warehouse: undefined, limit: 100 }], ["sql.execute", { sql: "SELECT 42", warehouse: undefined, limit: 100 }], ["sql.execute", { sql: "SELECT 42", warehouse: undefined, limit: 1 }], ["sql.execute", { sql: "SELECT 42", warehouse: undefined, limit: 10_000 }], ]) }) + + test("clamps oversized row limits before querying", async () => { + const tool = await initTool(SqlExecuteTool) + await tool.execute({ query: "SELECT 42", limit: 1e9 }, ctx) + expect(dispatch).toHaveBeenCalledTimes(1) + expect(dispatch).toHaveBeenCalledWith("sql.execute", { + sql: "SELECT 42", + warehouse: undefined, + limit: 10_000, + query_timeout_ms: undefined, + }) + }) }) describe("sql_execute query timeout", () => { @@ -244,9 +257,9 @@ describe("sql_execute query timeout", () => { }) } - test("passes explicit deadlines and disabled deadline to the native dispatcher", async () => { + test("passes positive deadlines to the native dispatcher", async () => { const tool = await initTool(SqlExecuteTool) - for (const query_timeout_ms of [0, 1, 600_000, 3_600_000]) { + for (const query_timeout_ms of [1, 600_000, 3_600_000]) { await tool.execute({ query: "SELECT 42", warehouse: "analytics", query_timeout_ms }, ctx) expect(dispatch).toHaveBeenLastCalledWith("sql.execute", { sql: "SELECT 42", @@ -255,19 +268,21 @@ describe("sql_execute query timeout", () => { query_timeout_ms, }) } - expect(dispatch).toHaveBeenCalledTimes(4) + expect(dispatch).toHaveBeenCalledTimes(3) }) - test("leaves an omitted deadline undefined so native defaults still apply", async () => { + test("leaves omitted and filler deadlines undefined so native defaults still apply", async () => { const tool = await initTool(SqlExecuteTool) - await tool.execute({ query: "SELECT 42" }, ctx) - expect(dispatch).toHaveBeenCalledTimes(1) - expect(dispatch).toHaveBeenCalledWith("sql.execute", { - sql: "SELECT 42", - warehouse: undefined, - limit: 100, - query_timeout_ms: undefined, - }) + for (const query_timeout_ms of [undefined, 0, [], "", null]) { + await tool.execute({ query: "SELECT 42", query_timeout_ms }, ctx) + expect(dispatch).toHaveBeenLastCalledWith("sql.execute", { + sql: "SELECT 42", + warehouse: undefined, + limit: 100, + query_timeout_ms: undefined, + }) + } + expect(dispatch).toHaveBeenCalledTimes(5) }) }) diff --git a/packages/opencode/test/tool/filler-parameters.test.ts b/packages/opencode/test/tool/filler-parameters.test.ts new file mode 100644 index 0000000000..62d895e1aa --- /dev/null +++ b/packages/opencode/test/tool/filler-parameters.test.ts @@ -0,0 +1,304 @@ +import { afterEach, describe, expect } from "bun:test" +import { Effect, Exit, Layer, Result, Schema } from "effect" +import z from "zod" +import type { JSONSchema7 } from "@ai-sdk/provider" +import { LayerNode } from "@opencode-ai/core/effect/layer-node" +import { Agent } from "@/agent/agent" +import { Config } from "@/config/config" +import { RuntimeFlags } from "@/effect/runtime-flags" +import { EffectBridge } from "@/effect/bridge" +import { Plugin } from "@/plugin" +import { Instance } from "@/project/instance" +import { MessageID, SessionID } from "@/session/schema" +import { ToolJsonSchema } from "@/tool/json-schema" +import { ToolRegistry } from "@/tool/registry" +import { Tool } from "@/tool/tool" +import { Truncate } from "@/tool/truncate" +import { disposeAllInstances, requireInstance } from "../fixture/fixture" +import { TestConfig } from "../fixture/config" +import { testEffect } from "../lib/effect" + +const fillers = [[], "", 0, null] +const pluginCalls: unknown[] = [] +const pluginLayer = Layer.succeed( + Plugin.Service, + Plugin.Service.of({ + init: () => Effect.void, + trigger: ((_name: unknown, _input: unknown, output: unknown) => + Effect.succeed(output)) as Plugin.Interface["trigger"], + list: () => + Effect.succeed([ + { + tool: { + optional_array_plugin: { + description: "Synthetic optional parameters", + args: { + labels: z.array(z.string()).min(1).optional(), + limit: z.number().optional().default(3), + }, + execute: async (args: unknown) => { + pluginCalls.push(args) + return "ok" + }, + }, + }, + }, + ]), + }), +) + +const it = testEffect( + LayerNode.buildLayer(LayerNode.group([ToolRegistry.node, Agent.node, Truncate.node]), { + replacements: [ + LayerNode.replace( + Config.node, + TestConfig.layer({ + get: () => Effect.succeed({ experimental: { batch_tool: true } }), + }), + ), + LayerNode.replace(RuntimeFlags.node, RuntimeFlags.layer()), + LayerNode.replace(Plugin.node, pluginLayer), + ], + }), +) + +afterEach(async () => { + pluginCalls.length = 0 + await disposeAllInstances() +}) + +function branches(schema: JSONSchema7): JSONSchema7[] { + const alternatives = schema.anyOf ?? schema.oneOf + if (!alternatives) return [schema] + const { anyOf: _, oneOf: __, ...base } = schema + return alternatives.flatMap((branch) => (typeof branch === "boolean" ? [] : branches({ ...base, ...branch }))) +} + +function fixture(definition: JSONSchema7 | boolean, root: JSONSchema7): unknown { + if (typeof definition === "boolean") return "value" + if (definition.$ref) { + const name = definition.$ref.replace(/^#\/(?:\$defs|definitions)\//, "") + const target = root.definitions?.[name] + if (!target) throw new Error(`Unresolved fixture reference: ${definition.$ref}`) + return fixture(target, root) + } + if (definition.const !== undefined) return definition.const + if (definition.enum) return definition.enum[0] + if (definition.default !== undefined) return definition.default + const alternative = definition.anyOf?.[0] ?? definition.oneOf?.[0] + if (alternative !== undefined) return fixture(alternative, root) + const type = Array.isArray(definition.type) ? definition.type[0] : definition.type + if (type === "object" || definition.properties) { + return Object.fromEntries( + (definition.required ?? []).map((name) => [name, fixture(definition.properties?.[name] ?? true, root)]), + ) + } + if (type === "array") { + const length = Math.max(definition.minItems ?? 0, Array.isArray(definition.items) ? definition.items.length : 0) + return Array.from({ length }, (_, index) => + fixture(Array.isArray(definition.items) ? definition.items[index] : (definition.items ?? true), root), + ) + } + if (type === "number" || type === "integer") { + const minimum = definition.minimum ?? 1 + const value = + typeof definition.exclusiveMinimum === "number" ? Math.max(minimum, definition.exclusiveMinimum + 1) : minimum + return definition.maximum === undefined ? value : Math.min(value, definition.maximum) + } + if (type === "boolean") return false + if (type === "null") return null + if (definition.format === "uri" || definition.format === "url") return "https://example.com" + return "value".padEnd(definition.minLength ?? 0, "x").slice(0, definition.maxLength) +} + +function context(): Tool.Context { + return { + sessionID: SessionID.make("ses_optional_parameters"), + messageID: MessageID.make("msg_optional_parameters"), + agent: "build", + abort: new AbortController().signal, + messages: [], + ask: () => Effect.void, + metadata: () => Effect.void, + } +} + +describe("optional tool parameter fillers", () => { + it.instance("every registered schema decodes optional fillers exactly like omission", () => + Effect.gen(function* () { + const registry = yield* ToolRegistry.Service + const instance = yield* requireInstance + const bridge = yield* EffectBridge.make() + const infos = yield* registry.allInfos() + expect(infos.some((info) => info.id === "optional_array_plugin")).toBe(true) + expect(infos.some((info) => info.id === "sql_execute")).toBe(true) + expect(infos.some((info) => info.id === "read")).toBe(true) + const failures: string[] = [] + let checked = 0 + for (const info of infos) { + const tool = yield* Effect.promise(() => Instance.restore(instance, () => bridge.promise(info.init()))) + const schema = ToolJsonSchema.fromTool({ ...tool, id: info.id }) + const decode = Schema.decodeUnknownResult(tool.parameters) + for (const [index, branch] of branches(schema).entries()) { + const input = fixture(branch, schema) as Record + const omitted = decode(input) + if (Result.isFailure(omitted)) { + failures.push(`${info.id}[${index}] invalid omission fixture: ${String(omitted.failure)}`) + continue + } + const optional = Object.keys(branch.properties ?? {}).filter((name) => !branch.required?.includes(name)) + for (const filler of fillers) { + for (const name of optional) { + const filled = decode({ ...input, [name]: filler }) + checked++ + if (Result.isFailure(filled)) { + failures.push(`${info.id}[${index}].${name} rejects ${JSON.stringify(filler)}`) + } else if (JSON.stringify(filled.success) !== JSON.stringify(omitted.success)) { + failures.push(`${info.id}[${index}].${name} changes omission behavior for ${JSON.stringify(filler)}`) + } + } + const filled = decode({ ...input, ...Object.fromEntries(optional.map((name) => [name, filler])) }) + if (Result.isFailure(filled) || JSON.stringify(filled.success) !== JSON.stringify(omitted.success)) { + failures.push( + `${info.id}[${index}] changes omission behavior with all optionals set to ${JSON.stringify(filler)}`, + ) + } + } + } + } + expect(checked).toBeGreaterThan(0) + expect(failures).toEqual([]) + }), + ) + + it.instance("legacy and native execution preserve defaults for fillers without relaxing required arrays", () => + Effect.gen(function* () { + const calls: unknown[] = [] + const legacyParameters = z.object({ + required: z.array(z.string()).min(1), + text: z.string(), + labels: z.array(z.string()).min(1).optional(), + limit: z.number().optional().default(3), + enabled: z.boolean().optional().default(true), + }) + const nativeParameters = Schema.Struct({ + required: Schema.NonEmptyArray(Schema.String), + text: Schema.String, + labels: Schema.optional(Schema.NonEmptyArray(Schema.String)), + limit: Schema.Number.pipe(Schema.optional, Schema.withDecodingDefaultType(Effect.succeed(3))), + enabled: Schema.Boolean.pipe(Schema.optional, Schema.withDecodingDefaultType(Effect.succeed(true))), + }) + expect(legacyParameters.safeParse({ required: ["kept"], text: "", labels: [] }).success).toBe(false) + expect( + Result.isFailure(Schema.decodeUnknownResult(nativeParameters)({ required: ["kept"], text: "", labels: [] })), + ).toBe(true) + const legacy = yield* Tool.define("optional_array_legacy", { + description: "Synthetic optional parameters", + parameters: legacyParameters, + execute: async (args) => { + calls.push(args) + return { title: "ok", output: "ok", metadata: { truncated: false } } + }, + }) + const native = yield* Tool.define( + "optional_array_native", + Effect.succeed({ + description: "Synthetic optional parameters", + parameters: nativeParameters, + execute: (args: typeof nativeParameters.Type) => + Effect.sync(() => { + calls.push(args) + return { title: "ok", output: "ok", metadata: { truncated: false } } + }), + }), + ) + for (const info of [legacy, native]) { + const tool = yield* info.init() + const execute = tool.execute as (args: unknown, ctx: Tool.Context) => ReturnType + for (const filler of fillers) { + const input = { required: ["kept"], text: "", labels: filler, limit: filler, enabled: filler } + expect(Schema.decodeUnknownSync(tool.parameters)(input)).toEqual({ + required: ["kept"], + text: "", + limit: 3, + enabled: true, + }) + yield* execute(input, context()) + expect(calls.at(-1)).toEqual({ required: ["kept"], text: "", limit: 3, enabled: true }) + expect(Result.isFailure(Schema.decodeUnknownResult(tool.parameters)({ required: filler, text: "" }))).toBe( + true, + ) + } + const supplied = { required: ["kept"], text: "", labels: ["selected"], limit: 7, enabled: false } + yield* execute(supplied, context()) + expect(calls.at(-1)).toEqual(supplied) + for (const invalid of [{}, { ...supplied, labels: [42] }, { ...supplied, limit: "invalid" }]) { + const before = calls.length + expect(Exit.isFailure(yield* execute(invalid, context()).pipe(Effect.exit))).toBe(true) + expect(calls).toHaveLength(before) + } + } + }), + ) + + it.instance("plugin execution receives the same decoded defaults as omitted arguments", () => + Effect.gen(function* () { + const registry = yield* ToolRegistry.Service + const info = (yield* registry.allInfos()).find((info) => info.id === "optional_array_plugin")! + const tool = yield* info.init() + const decode = Schema.decodeUnknownSync(tool.parameters) + for (const filler of fillers) { + const input = { labels: filler, limit: filler } + expect(decode(input)).toEqual({ limit: 3 }) + yield* tool.execute(input, context()) + expect(pluginCalls.at(-1)).toEqual({ limit: 3 }) + } + }), + ) + + it.instance("optional enum fillers do not select discriminated union branches", () => + Effect.gen(function* () { + const legacy = yield* Tool.define("optional_union_legacy", { + description: "Synthetic discriminated parameters", + parameters: z.discriminatedUnion("kind", [ + z.object({ kind: z.literal("left"), mode: z.enum(["fast", "slow"]).optional() }), + z.object({ kind: z.literal("right"), text: z.string() }), + ]), + execute: async () => ({ title: "ok", output: "ok", metadata: { truncated: false } }), + }) + const native = yield* Tool.define( + "optional_union_native", + Effect.succeed({ + description: "Synthetic discriminated parameters", + parameters: Schema.Union([ + Schema.Struct({ kind: Schema.Literal("left"), mode: Schema.optional(Schema.Literals(["fast", "slow"])) }), + Schema.Struct({ kind: Schema.Literal("right"), text: Schema.String }), + ]), + execute: () => Effect.succeed({ title: "ok", output: "ok", metadata: { truncated: false } }), + }), + ) + const failures: string[] = [] + for (const info of [legacy, native]) { + const tool = yield* info.init() + const decode = Schema.decodeUnknownResult(tool.parameters) + const omitted = decode({ kind: "left" }) + expect(Result.isSuccess(omitted)).toBe(true) + for (const mode of fillers) { + const filled = decode({ kind: "left", mode }) + if (Result.isFailure(filled) || JSON.stringify(filled.success) !== JSON.stringify({ kind: "left" })) { + failures.push(`${info.id} rejects optional mode filler ${JSON.stringify(mode)}`) + } + } + expect(Schema.decodeUnknownSync(tool.parameters)({ kind: "right", text: "" })).toEqual({ + kind: "right", + text: "", + }) + expect(Schema.decodeUnknownSync(tool.parameters)({ kind: "left", mode: "fast" })).toEqual({ + kind: "left", + mode: "fast", + }) + } + expect(failures).toEqual([]) + }), + ) +}) From aa8bc0e94fe023b4d2cefc019cc11bd1b7ae5d92 Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Thu, 15 Oct 2026 11:14:44 -0700 Subject: [PATCH 2/4] fix(tools): strip optional fillers only when validation fails Validate arguments as sent first so schema-valid intentional values (`max_entries: 0`, `description: ""`, `integration_ids: []`) are preserved. Only a call the schema rejects is retried with top-level optional fillers removed. - `fillerSafeParameters` is now a union of the original schema and the stripped-retry schema - SQL tool `query_timeout_ms` now requires a positive value, so a filler `0` falls back to the configured deadline - tests: valid empty/zero values preserved, rejected fillers still accepted Co-Authored-By: Claude Opus 5.5 --- .../opencode/src/altimate/tools/sql-check.ts | 4 +- .../src/altimate/tools/sql-execute.ts | 2 +- .../src/altimate/tools/sql-materialize.ts | 2 +- .../opencode/src/tool/filler-parameters.ts | 7 ++- .../test/tool/filler-parameters.test.ts | 61 +++++++++++++++++-- 5 files changed, 64 insertions(+), 12 deletions(-) diff --git a/packages/opencode/src/altimate/tools/sql-check.ts b/packages/opencode/src/altimate/tools/sql-check.ts index a47fae0705..3130ef05c3 100644 --- a/packages/opencode/src/altimate/tools/sql-check.ts +++ b/packages/opencode/src/altimate/tools/sql-check.ts @@ -31,7 +31,7 @@ const parameters = z right_col: column.optional().describe("Right output column for keys"), left_warehouse: z.string().optional().describe("Optional assertion: must equal warehouse"), right_warehouse: z.string().optional().describe("Optional assertion: must equal warehouse"), - query_timeout_ms: z.number().int().min(0).max(3_600_000).optional(), + query_timeout_ms: z.number().int().min(1).max(3_600_000).optional(), }) .strict() @@ -64,7 +64,7 @@ const publicParameters = z query_timeout_ms: z .number() .int() - .min(0) + .min(1) .max(3_600_000) .optional() .describe("Query deadline in ms; omitted or 0 uses the common connection policy"), diff --git a/packages/opencode/src/altimate/tools/sql-execute.ts b/packages/opencode/src/altimate/tools/sql-execute.ts index 3f40e7af7e..d74f2087a9 100644 --- a/packages/opencode/src/altimate/tools/sql-execute.ts +++ b/packages/opencode/src/altimate/tools/sql-execute.ts @@ -57,7 +57,7 @@ const parameters = z.object({ query_timeout_ms: z .number() .int() - .min(0) + .min(1) .max(3_600_000) .optional() .describe( diff --git a/packages/opencode/src/altimate/tools/sql-materialize.ts b/packages/opencode/src/altimate/tools/sql-materialize.ts index 17ada5fda0..2313f6fadb 100644 --- a/packages/opencode/src/altimate/tools/sql-materialize.ts +++ b/packages/opencode/src/altimate/tools/sql-materialize.ts @@ -550,7 +550,7 @@ export const SqlMaterializeTool = Tool.define("sql_materialize", { query_timeout_ms: z .number() .int() - .min(0) + .min(1) .max(3_600_000) .optional() .describe("Source query deadline in ms; omitted or 0 uses the common connection policy"), diff --git a/packages/opencode/src/tool/filler-parameters.ts b/packages/opencode/src/tool/filler-parameters.ts index 3926343ef2..2278e3d578 100644 --- a/packages/opencode/src/tool/filler-parameters.ts +++ b/packages/opencode/src/tool/filler-parameters.ts @@ -3,11 +3,11 @@ import { Schema, SchemaGetter } from "effect" const normalized = new WeakSet() -/** Normalize only optional tool arguments; required values and nested payloads retain their meaning. */ +/** Retry a rejected call with optional fillers removed; required values and nested payloads retain their meaning. */ export function fillerSafeParameters(parameters: Schema.Decoder, jsonSchema: JSONSchema7): Schema.Decoder { // Legacy tools may publish a simpler wire schema than their validation schema. if (normalized.has(parameters)) return parameters - const result = Schema.Unknown.pipe( + const stripped = Schema.Unknown.pipe( Schema.decodeTo(parameters, { decode: SchemaGetter.transform((input) => { if (!input || typeof input !== "object" || Array.isArray(input)) return input @@ -49,6 +49,9 @@ export function fillerSafeParameters(parameters: Schema.Decoder, jsonSchem encode: SchemaGetter.transform((value) => value), }), ) + // Validate the arguments as sent first so intentional zero/empty values survive; only a call the + // schema rejects is retried with optional fillers removed. + const result = Schema.Union([parameters, stripped]) as unknown as Schema.Decoder normalized.add(result) return result } diff --git a/packages/opencode/test/tool/filler-parameters.test.ts b/packages/opencode/test/tool/filler-parameters.test.ts index 62d895e1aa..b93e9020a0 100644 --- a/packages/opencode/test/tool/filler-parameters.test.ts +++ b/packages/opencode/test/tool/filler-parameters.test.ts @@ -124,7 +124,7 @@ function context(): Tool.Context { } describe("optional tool parameter fillers", () => { - it.instance("every registered schema decodes optional fillers exactly like omission", () => + it.instance("every registered schema accepts optional fillers, either preserved as valid values or treated as omission", () => Effect.gen(function* () { const registry = yield* ToolRegistry.Service const instance = yield* requireInstance @@ -153,15 +153,17 @@ describe("optional tool parameter fillers", () => { checked++ if (Result.isFailure(filled)) { failures.push(`${info.id}[${index}].${name} rejects ${JSON.stringify(filler)}`) - } else if (JSON.stringify(filled.success) !== JSON.stringify(omitted.success)) { + } else if ( + JSON.stringify(filled.success) !== JSON.stringify(omitted.success) && + (filled.success as Record)?.[name] !== filler && + JSON.stringify((filled.success as Record)?.[name]) !== JSON.stringify(filler) + ) { failures.push(`${info.id}[${index}].${name} changes omission behavior for ${JSON.stringify(filler)}`) } } const filled = decode({ ...input, ...Object.fromEntries(optional.map((name) => [name, filler])) }) - if (Result.isFailure(filled) || JSON.stringify(filled.success) !== JSON.stringify(omitted.success)) { - failures.push( - `${info.id}[${index}] changes omission behavior with all optionals set to ${JSON.stringify(filler)}`, - ) + if (Result.isFailure(filled)) { + failures.push(`${info.id}[${index}] rejects all optionals set to ${JSON.stringify(filler)}`) } } } @@ -301,4 +303,51 @@ describe("optional tool parameter fillers", () => { expect(failures).toEqual([]) }), ) + it.instance("schema-valid empty and zero values are preserved; fillers are stripped only when validation fails", () => + Effect.gen(function* () { + const legacy = yield* Tool.define("optional_valid_legacy", { + description: "Synthetic intentional values", + parameters: z.object({ + max_entries: z.number().min(0).optional().default(20), + description: z.string().optional(), + integration_ids: z.array(z.string()).optional(), + labels: z.array(z.string()).min(1).optional(), + limit: z.number().min(1).optional().default(3), + }), + execute: async () => ({ title: "ok", output: "ok", metadata: { truncated: false } }), + }) + const native = yield* Tool.define( + "optional_valid_native", + Effect.succeed({ + description: "Synthetic intentional values", + parameters: Schema.Struct({ + max_entries: Schema.Number.pipe(Schema.optional, Schema.withDecodingDefaultType(Effect.succeed(20))), + description: Schema.optional(Schema.String), + integration_ids: Schema.optional(Schema.Array(Schema.String)), + labels: Schema.optional(Schema.NonEmptyArray(Schema.String)), + }), + execute: () => Effect.succeed({ title: "ok", output: "ok", metadata: { truncated: false } }), + }), + ) + for (const info of [legacy, native]) { + const tool = yield* info.init() + const decode = Schema.decodeUnknownSync(tool.parameters) as (input: unknown) => Record + // Valid as sent: intentional values survive. + expect(decode({ max_entries: 0, description: "", integration_ids: [] })).toMatchObject({ + max_entries: 0, + description: "", + integration_ids: [], + }) + // Invalid as sent (labels: []): retried with fillers removed, so defaults apply. + const retried = decode({ max_entries: 0, description: "", integration_ids: [], labels: [] }) + expect(retried.max_entries).toBe(20) + expect(retried).not.toHaveProperty("description") + expect(retried).not.toHaveProperty("integration_ids") + expect(retried).not.toHaveProperty("labels") + } + // A filler the schema itself rejects (limit: 0) falls back to the default. + const legacyTool = yield* legacy.init() + expect(Schema.decodeUnknownSync(legacyTool.parameters)({ limit: 0 })).toMatchObject({ limit: 3 }) + }), + ) }) From 335b9eb88395fa971274823709e27d071275e88e Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Thu, 15 Oct 2026 11:27:21 -0700 Subject: [PATCH 3/4] fix(tools): keep valid optional siblings when one filler is rejected When validation fails, retry keeping each optional filler that is valid on its own and dropping only the rejected ones, instead of stripping all fillers. Intentional zero/empty values next to an unrelated filler survive. Co-Authored-By: Claude Opus 5.5 --- .../opencode/src/tool/filler-parameters.ts | 21 ++-- .../test/tool/filler-parameters.test.ts | 112 +++++++++--------- 2 files changed, 70 insertions(+), 63 deletions(-) diff --git a/packages/opencode/src/tool/filler-parameters.ts b/packages/opencode/src/tool/filler-parameters.ts index 2278e3d578..59c33dda79 100644 --- a/packages/opencode/src/tool/filler-parameters.ts +++ b/packages/opencode/src/tool/filler-parameters.ts @@ -1,5 +1,5 @@ import type { JSONSchema7 } from "@ai-sdk/provider" -import { Schema, SchemaGetter } from "effect" +import { Result, Schema, SchemaGetter } from "effect" const normalized = new WeakSet() @@ -7,6 +7,7 @@ const normalized = new WeakSet() export function fillerSafeParameters(parameters: Schema.Decoder, jsonSchema: JSONSchema7): Schema.Decoder { // Legacy tools may publish a simpler wire schema than their validation schema. if (normalized.has(parameters)) return parameters + const decodeResult = Schema.decodeUnknownResult(parameters) const stripped = Schema.Unknown.pipe( Schema.decodeTo(parameters, { decode: SchemaGetter.transform((input) => { @@ -37,14 +38,18 @@ export function fillerSafeParameters(parameters: Schema.Decoder, jsonSchem } } collect(jsonSchema) - return Object.fromEntries( - Object.entries(args).filter( - ([key, value]) => - !properties.has(key) || - required.has(key) || - !(value === null || value === "" || value === 0 || (Array.isArray(value) && value.length === 0)), - ), + const isFiller = (key: string, value: unknown) => + properties.has(key) && + !required.has(key) && + (value === null || value === "" || value === 0 || (Array.isArray(value) && value.length === 0)) + const base = Object.fromEntries(Object.entries(args).filter(([key, value]) => !isFiller(key, value))) + // Keep each filler that is valid on its own so intentional siblings survive an unrelated filler. + const valid = (candidate: Record) => Result.isSuccess(decodeResult(candidate)) + const kept = Object.entries(args).filter( + ([key, value]) => isFiller(key, value) && valid({ ...base, [key]: value }), ) + const merged = { ...base, ...Object.fromEntries(kept) } + return kept.length > 0 && valid(merged) ? merged : base }), encode: SchemaGetter.transform((value) => value), }), diff --git a/packages/opencode/test/tool/filler-parameters.test.ts b/packages/opencode/test/tool/filler-parameters.test.ts index b93e9020a0..61f742750b 100644 --- a/packages/opencode/test/tool/filler-parameters.test.ts +++ b/packages/opencode/test/tool/filler-parameters.test.ts @@ -124,53 +124,55 @@ function context(): Tool.Context { } describe("optional tool parameter fillers", () => { - it.instance("every registered schema accepts optional fillers, either preserved as valid values or treated as omission", () => - Effect.gen(function* () { - const registry = yield* ToolRegistry.Service - const instance = yield* requireInstance - const bridge = yield* EffectBridge.make() - const infos = yield* registry.allInfos() - expect(infos.some((info) => info.id === "optional_array_plugin")).toBe(true) - expect(infos.some((info) => info.id === "sql_execute")).toBe(true) - expect(infos.some((info) => info.id === "read")).toBe(true) - const failures: string[] = [] - let checked = 0 - for (const info of infos) { - const tool = yield* Effect.promise(() => Instance.restore(instance, () => bridge.promise(info.init()))) - const schema = ToolJsonSchema.fromTool({ ...tool, id: info.id }) - const decode = Schema.decodeUnknownResult(tool.parameters) - for (const [index, branch] of branches(schema).entries()) { - const input = fixture(branch, schema) as Record - const omitted = decode(input) - if (Result.isFailure(omitted)) { - failures.push(`${info.id}[${index}] invalid omission fixture: ${String(omitted.failure)}`) - continue - } - const optional = Object.keys(branch.properties ?? {}).filter((name) => !branch.required?.includes(name)) - for (const filler of fillers) { - for (const name of optional) { - const filled = decode({ ...input, [name]: filler }) - checked++ + it.instance( + "every registered schema accepts optional fillers, either preserved as valid values or treated as omission", + () => + Effect.gen(function* () { + const registry = yield* ToolRegistry.Service + const instance = yield* requireInstance + const bridge = yield* EffectBridge.make() + const infos = yield* registry.allInfos() + expect(infos.some((info) => info.id === "optional_array_plugin")).toBe(true) + expect(infos.some((info) => info.id === "sql_execute")).toBe(true) + expect(infos.some((info) => info.id === "read")).toBe(true) + const failures: string[] = [] + let checked = 0 + for (const info of infos) { + const tool = yield* Effect.promise(() => Instance.restore(instance, () => bridge.promise(info.init()))) + const schema = ToolJsonSchema.fromTool({ ...tool, id: info.id }) + const decode = Schema.decodeUnknownResult(tool.parameters) + for (const [index, branch] of branches(schema).entries()) { + const input = fixture(branch, schema) as Record + const omitted = decode(input) + if (Result.isFailure(omitted)) { + failures.push(`${info.id}[${index}] invalid omission fixture: ${String(omitted.failure)}`) + continue + } + const optional = Object.keys(branch.properties ?? {}).filter((name) => !branch.required?.includes(name)) + for (const filler of fillers) { + for (const name of optional) { + const filled = decode({ ...input, [name]: filler }) + checked++ + if (Result.isFailure(filled)) { + failures.push(`${info.id}[${index}].${name} rejects ${JSON.stringify(filler)}`) + } else if ( + JSON.stringify(filled.success) !== JSON.stringify(omitted.success) && + (filled.success as Record)?.[name] !== filler && + JSON.stringify((filled.success as Record)?.[name]) !== JSON.stringify(filler) + ) { + failures.push(`${info.id}[${index}].${name} changes omission behavior for ${JSON.stringify(filler)}`) + } + } + const filled = decode({ ...input, ...Object.fromEntries(optional.map((name) => [name, filler])) }) if (Result.isFailure(filled)) { - failures.push(`${info.id}[${index}].${name} rejects ${JSON.stringify(filler)}`) - } else if ( - JSON.stringify(filled.success) !== JSON.stringify(omitted.success) && - (filled.success as Record)?.[name] !== filler && - JSON.stringify((filled.success as Record)?.[name]) !== JSON.stringify(filler) - ) { - failures.push(`${info.id}[${index}].${name} changes omission behavior for ${JSON.stringify(filler)}`) + failures.push(`${info.id}[${index}] rejects all optionals set to ${JSON.stringify(filler)}`) } } - const filled = decode({ ...input, ...Object.fromEntries(optional.map((name) => [name, filler])) }) - if (Result.isFailure(filled)) { - failures.push(`${info.id}[${index}] rejects all optionals set to ${JSON.stringify(filler)}`) - } } } - } - expect(checked).toBeGreaterThan(0) - expect(failures).toEqual([]) - }), + expect(checked).toBeGreaterThan(0) + expect(failures).toEqual([]) + }), ) it.instance("legacy and native execution preserve defaults for fillers without relaxing required arrays", () => @@ -219,14 +221,11 @@ describe("optional tool parameter fillers", () => { const execute = tool.execute as (args: unknown, ctx: Tool.Context) => ReturnType for (const filler of fillers) { const input = { required: ["kept"], text: "", labels: filler, limit: filler, enabled: filler } - expect(Schema.decodeUnknownSync(tool.parameters)(input)).toEqual({ - required: ["kept"], - text: "", - limit: 3, - enabled: true, - }) + // A numeric zero is a valid `limit` on its own, so it is kept; other fillers fall back to defaults. + const expected = { required: ["kept"], text: "", limit: filler === 0 ? 0 : 3, enabled: true } + expect(Schema.decodeUnknownSync(tool.parameters)(input)).toEqual(expected) yield* execute(input, context()) - expect(calls.at(-1)).toEqual({ required: ["kept"], text: "", limit: 3, enabled: true }) + expect(calls.at(-1)).toEqual(expected) expect(Result.isFailure(Schema.decodeUnknownResult(tool.parameters)({ required: filler, text: "" }))).toBe( true, ) @@ -251,9 +250,11 @@ describe("optional tool parameter fillers", () => { const decode = Schema.decodeUnknownSync(tool.parameters) for (const filler of fillers) { const input = { labels: filler, limit: filler } - expect(decode(input)).toEqual({ limit: 3 }) + // Zero is a valid `limit` for this plugin, so it is kept; other fillers fall back to the default. + const expected = { limit: filler === 0 ? 0 : 3 } + expect(decode(input)).toEqual(expected) yield* tool.execute(input, context()) - expect(pluginCalls.at(-1)).toEqual({ limit: 3 }) + expect(pluginCalls.at(-1)).toEqual(expected) } }), ) @@ -338,11 +339,12 @@ describe("optional tool parameter fillers", () => { description: "", integration_ids: [], }) - // Invalid as sent (labels: []): retried with fillers removed, so defaults apply. + // Invalid as sent (labels: []): only the rejected filler is removed. const retried = decode({ max_entries: 0, description: "", integration_ids: [], labels: [] }) - expect(retried.max_entries).toBe(20) - expect(retried).not.toHaveProperty("description") - expect(retried).not.toHaveProperty("integration_ids") + expect(retried.max_entries).toBe(0) + // Fillers that are valid on their own are kept; only the rejected one is dropped. + expect(retried.description).toBe("") + expect(retried.integration_ids).toEqual([]) expect(retried).not.toHaveProperty("labels") } // A filler the schema itself rejects (limit: 0) falls back to the default. From 20c6997186dc41ad115ce055e8df2e599195496a Mon Sep 17 00:00:00 2001 From: anandgupta42 Date: Thu, 15 Oct 2026 12:08:09 -0700 Subject: [PATCH 4/4] fix(tools): pass plugin tool arguments through when they do not validate Plugin handlers historically received raw arguments. Use the normalized arguments only when they validate, keep arguments the schema does not list, and otherwise hand the handler the arguments as sent. Co-Authored-By: Claude Opus 5.5 --- packages/opencode/src/tool/registry.ts | 30 +++++++++++-------- .../test/tool/filler-parameters.test.ts | 6 ++++ 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/packages/opencode/src/tool/registry.ts b/packages/opencode/src/tool/registry.ts index c4a7373f59..90cb7c962a 100644 --- a/packages/opencode/src/tool/registry.ts +++ b/packages/opencode/src/tool/registry.ts @@ -44,7 +44,7 @@ import { AppRuntime } from "@/effect/app-runtime" import { RuntimeFlags } from "@/effect/runtime-flags" // altimate_change end // altimate_change start — upstream_fix: plugin tools share the optional filler contract -import { Schema } from "effect" +import { Result, Schema } from "effect" import { fillerSafeParameters } from "./filler-parameters" import { ToolJsonSchema } from "./json-schema" // altimate_change end @@ -164,12 +164,24 @@ import { DbtPrReviewTool } from "../altimate/tools/dbt-pr-review" // altimate_change start — upstream_fix: plugin tools share the optional filler contract function fillerSafePlugin(tool: Tool.DefWithoutID): Tool.DefWithoutID { - const parameters = fillerSafeParameters(tool.parameters, ToolJsonSchema.fromTool({ ...tool, id: "plugin" })) - const decode = Schema.decodeUnknownEffect(parameters) + const schema = ToolJsonSchema.fromTool({ ...tool, id: "plugin" }) + const parameters = fillerSafeParameters(tool.parameters, schema) + const decode = Schema.decodeUnknownResult(parameters) + const known = new Set(Object.keys(schema.properties ?? {})) return { ...tool, parameters, - execute: (args, ctx) => decode(args).pipe(Effect.orDie, Effect.flatMap((decoded) => tool.execute(decoded, ctx))), + // Plugin handlers historically received raw arguments: use the normalized form only when it + // validates, and keep any arguments the schema does not list. + execute: (args, ctx) => { + const decoded = decode(args) + if (Result.isFailure(decoded) || !decoded.success || typeof decoded.success !== "object") + return tool.execute(args, ctx) + const extra = Object.fromEntries( + Object.entries(args as Record).filter(([key]) => !known.has(key)), + ) + return tool.execute({ ...extra, ...(decoded.success as object) }, ctx) + }, } } // altimate_change end @@ -539,10 +551,7 @@ export namespace ToolRegistry { return all(plugins, configInput) } - export async function ids( - plugins?: Awaited>, - configInput?: RegistryConfigInput, - ) { + export async function ids(plugins?: Awaited>, configInput?: RegistryConfigInput) { return all(plugins, configInput).then((x) => x.map((t) => t.id)) } // altimate_change end @@ -662,10 +671,7 @@ export namespace ToolRegistry { }) : Effect.succeed(undefined) const bridgeWithPlugins = ( - fn: ( - plugins?: Awaited>, - configInput?: RegistryConfigInput, - ) => Promise, + fn: (plugins?: Awaited>, configInput?: RegistryConfigInput) => Promise, ): Effect.Effect => Effect.gen(function* () { const plugins = yield* resolvePlugins() diff --git a/packages/opencode/test/tool/filler-parameters.test.ts b/packages/opencode/test/tool/filler-parameters.test.ts index 61f742750b..1bf7e1ed6d 100644 --- a/packages/opencode/test/tool/filler-parameters.test.ts +++ b/packages/opencode/test/tool/filler-parameters.test.ts @@ -256,6 +256,12 @@ describe("optional tool parameter fillers", () => { yield* tool.execute(input, context()) expect(pluginCalls.at(-1)).toEqual(expected) } + // Arguments the schema does not list are kept, and a call the schema rejects still reaches the handler as sent. + yield* tool.execute({ extra: "kept", labels: [] }, context()) + expect(pluginCalls.at(-1)).toEqual({ extra: "kept", limit: 3 }) + const mismatched = { limit: "not-a-number", extra: 1 } + yield* tool.execute(mismatched, context()) + expect(pluginCalls.at(-1)).toEqual(mismatched) }), )