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 a405972f51..a044102130 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" @@ -31,7 +32,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() @@ -61,12 +62,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(1) + .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). @@ -440,7 +453,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 3823a7da9d..14059614e9 100644 --- a/packages/opencode/src/altimate/tools/sql-execute.ts +++ b/packages/opencode/src/altimate/tools/sql-execute.ts @@ -42,7 +42,7 @@ import { normalizeError } from "./response-normalization" import { readSql } from "./sql-check" 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 MAX_GENERATED_SQL_BYTES = 1024 * 1024 // Below the default tool-output cap, so a redirect never spills the SQL into a truncated file. const MAX_REDIRECT_SQL_BYTES = 40 * 1024 @@ -71,18 +71,18 @@ 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() - .min(0) + .min(1) .max(3_600_000) .optional() .describe( - "Optional per-query execution deadline in ms. Omitted or 0 uses the configured default. Bounded by the remaining run budget.", + "Optional per-query execution deadline in ms. Omit or use 0 to use the configured deadline (connection setting or environment), bounded by the remaining run budget. 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 8d52897daf..1362f83a47 100644 --- a/packages/opencode/src/altimate/tools/sql-materialize.ts +++ b/packages/opencode/src/altimate/tools/sql-materialize.ts @@ -465,16 +465,15 @@ 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 .number() .int() - .min(0) + .min(1) .max(3_600_000) .optional() - .describe("Query deadline in ms; omitted or 0 uses the configured default, bounded by the remaining run budget"), + .describe("Source query deadline in ms; omitted or 0 uses the configured default, bounded by the remaining run budget"), }), 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..59c33dda79 --- /dev/null +++ b/packages/opencode/src/tool/filler-parameters.ts @@ -0,0 +1,62 @@ +import type { JSONSchema7 } from "@ai-sdk/provider" +import { Result, Schema, SchemaGetter } from "effect" + +const normalized = new WeakSet() + +/** 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 decodeResult = Schema.decodeUnknownResult(parameters) + const stripped = 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) + 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), + }), + ) + // 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/src/tool/registry.ts b/packages/opencode/src/tool/registry.ts index a33b2a2e32..90cb7c962a 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 { Result, 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,30 @@ 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 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, + // 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 + export namespace ToolRegistry { const log = Log.create({ service: "tool.registry" }) @@ -275,7 +304,7 @@ export namespace ToolRegistry { ...(result.attachments ? { attachments: result.attachments as any } : {}), } }, - }), + }).pipe(Effect.map(fillerSafePlugin)), } } // altimate_change end @@ -522,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 @@ -645,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/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 5bde89d7e4..05b9e0a5eb 100644 --- a/packages/opencode/test/altimate/sql-materialize.test.ts +++ b/packages/opencode/test/altimate/sql-materialize.test.ts @@ -532,26 +532,25 @@ describe("sql_materialize", () => { }) } - test("the tool rejects max_bytes above the hard 2 GiB ceiling before querying", async () => { - const seen: string[] = [] - const get = fakePostgres(async (sql) => { - seen.push(sql) - throw new Error("No query should run") + test("the tool clamps oversized max_bytes to the hard 2 GiB ceiling for the source read", async () => { + const limits: (number | undefined)[] = [] + const get = fakePostgres(async (_sql, _limit, _binds, options) => { + limits.push(options?.maxResultBytes) + return { columns: ["n"], rows: [[1]], row_count: 1, truncated: false } }) 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 1 AS n", + target_table: "capped_transfer", + max_bytes: 4 * 1024 ** 3, + }, + ctx as any, + ) + expect(result.metadata.error).toBeUndefined() + expect(limits).toEqual([2 * 1024 ** 3]) } finally { get.mockRestore() } @@ -605,6 +604,42 @@ 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(1) + expect(seen[0].sql).toMatch(/LIMIT 2000001$/) + expect(seen[0].timeout).toBe(expected ?? 120_000) + } finally { + get.mockRestore() + } + }) + } + test("a remote source cannot outlast its deadline and later write to scratch", async () => { await getScratch() const finished = Promise.withResolvers() diff --git a/packages/opencode/test/altimate/tools/sql-check.test.ts b/packages/opencode/test/altimate/tools/sql-check.test.ts index ebb91f93d3..d1e3bf092e 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", () => @@ -454,6 +463,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 : 120_000) + } + 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-query-from.test.ts b/packages/opencode/test/altimate/tools/sql-execute-query-from.test.ts index fb913e33dc..3adc47d5c8 100644 --- a/packages/opencode/test/altimate/tools/sql-execute-query-from.test.ts +++ b/packages/opencode/test/altimate/tools/sql-execute-query-from.test.ts @@ -168,7 +168,7 @@ describe("sql_execute generated reads", () => { await tool.execute({ query_from: source, warehouse: "analytics", query_timeout_ms: 0 }, ctx) expect(dispatch).toHaveBeenCalledTimes(2) for (const [, input] of dispatch.mock.calls) { - expect(input).toMatchObject({ query_timeout_ms: 0 }) + expect(input).toMatchObject({ query_timeout_ms: undefined }) expect((input as { read_only?: boolean }).read_only).toBe(type === "sqlite" ? undefined : true) } }) diff --git a/packages/opencode/test/altimate/tools/sql-execute.test.ts b/packages/opencode/test/altimate/tools/sql-execute.test.ts index 29e9ffc362..12e09877ef 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 }, { signal: ctx.abort, sessionID: ctx.sessionID }], ["sql.execute", { sql: "SELECT 42", warehouse: undefined, limit: 100 }, { signal: ctx.abort, sessionID: ctx.sessionID }], ["sql.execute", { sql: "SELECT 42", warehouse: undefined, limit: 1 }, { signal: ctx.abort, sessionID: ctx.sessionID }], ["sql.execute", { sql: "SELECT 42", warehouse: undefined, limit: 10_000 }, { signal: ctx.abort, sessionID: ctx.sessionID }], ]) }) + + 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, + }, { signal: ctx.abort, sessionID: ctx.sessionID }) + }) }) describe("sql_execute query timeout", () => { @@ -244,9 +257,9 @@ describe("sql_execute query timeout", () => { }) } - test("passes explicit deadlines and the default sentinel 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, }, { signal: ctx.abort, sessionID: ctx.sessionID }) } - 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, - }, { signal: ctx.abort, sessionID: ctx.sessionID }) + 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, + }, { signal: ctx.abort, sessionID: ctx.sessionID }) + } + 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..1bf7e1ed6d --- /dev/null +++ b/packages/opencode/test/tool/filler-parameters.test.ts @@ -0,0 +1,361 @@ +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 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}] rejects 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 } + // 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(expected) + 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 } + // 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(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) + }), + ) + + 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([]) + }), + ) + 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: []): only the rejected filler is removed. + const retried = decode({ max_entries: 0, description: "", integration_ids: [], labels: [] }) + 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. + const legacyTool = yield* legacy.init() + expect(Schema.decodeUnknownSync(legacyTool.parameters)({ limit: 0 })).toMatchObject({ limit: 3 }) + }), + ) +})