-
Notifications
You must be signed in to change notification settings - Fork 2.9k
fix: make tool invalid-arguments errors clearly actionable to the model #11961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@kilocode/cli": patch | ||
| --- | ||
|
|
||
| Tool invalid-argument errors are now actionable to the model: the raw `SchemaError(...)` fallback at the tool boundary is replaced with one readable `<json-path>: <reason>` line per failing field, and decoding enumerates every offending field instead of stopping at the first. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { Effect, Schema } from "effect" | ||
| import { Effect, Schema, SchemaIssue } from "effect" // kilocode_change | ||
| import type { JSONSchema7 } from "@ai-sdk/provider" | ||
| import type { MessageV2 } from "../session/message-v2" | ||
| import type { Permission } from "../permission" | ||
|
|
@@ -31,6 +31,41 @@ export class InvalidArgumentsError extends Schema.TaggedErrorClass<InvalidArgume | |
| } | ||
| } | ||
|
|
||
| // kilocode_change start | ||
| const formatter = SchemaIssue.makeFormatterStandardSchemaV1() | ||
|
|
||
| function format(error: unknown) { | ||
| if (!Schema.isSchemaError(error)) { | ||
| return String(error) | ||
| } | ||
| const result = formatter(error.issue) | ||
| if (result.issues.length === 0) { | ||
| return String(error) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Fallback can silently reintroduce the exact If Reply with |
||
| } | ||
| return result.issues.map((issue) => `${path(issue.path)}: ${reason(issue.message)}`).join("\n") | ||
| } | ||
|
|
||
| function path(keys: ReadonlyArray<unknown> | undefined) { | ||
| const value = (keys ?? []) | ||
| .map((key) => { | ||
| const segment = typeof key === "object" && key !== null && "key" in key ? key.key : key | ||
| return typeof segment === "number" ? `[${segment}]` : `[${JSON.stringify(String(segment))}]` | ||
| }) | ||
| .join("") | ||
| return value || "input" | ||
| } | ||
|
|
||
| function reason(message: string) { | ||
| if (message.toLowerCase().includes("required")) { | ||
| return message | ||
| } | ||
| if (message === "Missing key") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Exact string match on an internal library message is fragile
Reply with |
||
| return "is missing and is required" | ||
| } | ||
| return message | ||
| } | ||
| // kilocode_change end | ||
|
|
||
| export type Context<M extends Metadata = Metadata> = { | ||
| sessionID: SessionID | ||
| messageID: MessageID | ||
|
|
@@ -116,12 +151,12 @@ function wrap<Parameters extends Schema.Decoder<unknown>, Result extends Metadat | |
| ...(ctx.callID ? { "tool.call_id": ctx.callID } : {}), | ||
| } | ||
| return Effect.gen(function* () { | ||
| const decoded = yield* decode(args).pipe( | ||
| const decoded = yield* decode(args, { errors: "all" }).pipe( // kilocode_change | ||
| Effect.mapError( | ||
| (error) => | ||
| new InvalidArgumentsError({ | ||
| tool: id, | ||
| detail: toolInfo.formatValidationError ? toolInfo.formatValidationError(error) : String(error), | ||
| detail: toolInfo.formatValidationError ? toolInfo.formatValidationError(error) : format(error), // kilocode_change | ||
| }), | ||
| ), | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WARNING: New Kilo-only logic added directly to a shared upstream file
formatter,format(),path(), andreason()are ~35 lines of pure Kilo-specific logic added inline totool.ts, a shared opencode file. Per the Fork Isolation Rule inpackages/opencode/AGENTS.md, Kilo-specific logic touching a shared upstream file should be extracted into a mirror file undersrc/kilocode/tool/tool.tsand called from here behind a singlekilocode_changehook, rather than inlined with akilocode_change start/endblock. This keeps the upstream diff minimal for future merges — right now this whole block will conflict with any upstream change to the surrounding error-formatting code.Reply with
@kilocode-bot fix itto have Kilo Code address this issue.