fix: make tool invalid-arguments errors clearly actionable to the model - #11961
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| // kilocode_change start |
There was a problem hiding this comment.
WARNING: New Kilo-only logic added directly to a shared upstream file
formatter, format(), path(), and reason() are ~35 lines of pure Kilo-specific logic added inline to tool.ts, a shared opencode file. Per the Fork Isolation Rule in packages/opencode/AGENTS.md, Kilo-specific logic touching a shared upstream file should be extracted into a mirror file under src/kilocode/tool/tool.ts and called from here behind a single kilocode_change hook, rather than inlined with a kilocode_change start/end block. 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 it to have Kilo Code address this issue.
| } | ||
| const result = formatter(error.issue) | ||
| if (result.issues.length === 0) { | ||
| return String(error) |
There was a problem hiding this comment.
WARNING: Fallback can silently reintroduce the exact SchemaError(...) jargon this PR fixes
If Schema.isSchemaError(error) is true but the standard-schema formatter yields zero issues (e.g. a compound/union failure the formatter doesn't flatten to leaf issues), this falls back to String(error), which is the raw, model-unreadable text the PR is trying to eliminate. Since this is a real schema error (not just an unrecognized error shape), it may be worth a more generic-but-still-readable fallback (e.g. error.message) instead of the raw String(error) jargon.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (message.toLowerCase().includes("required")) { | ||
| return message | ||
| } | ||
| if (message === "Missing key") { |
There was a problem hiding this comment.
SUGGESTION: Exact string match on an internal library message is fragile
message === "Missing key" depends on an exact, undocumented string from Effect's SchemaIssue formatter. If a future Effect upgrade changes this wording even slightly (casing, punctuation, phrasing), this branch silently stops matching and the reason falls through to the raw internal message again — with no test failure unless the exact .toLowerCase().includes("missing")/"required" assertions catch it. Consider matching more defensively (e.g. message.toLowerCase().includes("missing")) so minor wording changes upstream don't regress the readability fix.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Suggestions only — safe to merge at the author's discretion Overview
Issue Details (click to expand)SUGGESTION
Resolved Since Last Review
Files Reviewed (4 files)
Notes and assumptions
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 5ed6d8a)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5ed6d8a)Status: 2 Issues Found | Recommendation: Suggestions only — safe to merge at the author's discretion Overview
Issue Details (click to expand)SUGGESTION
Resolved Since Last Review
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 450e54f)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Resolved Since Last Review
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit b834cc9)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (3 files)
Reviewed by claude-opus-5 · Input: 34 · Output: 8.1K · Cached: 762K Review guidance: REVIEW.md from base branch |
|
could you fix the merge conflicts and resolve the review comments if this PR is still relevant? |
Return a jargon-free actionable message when the schema formatter yields no issues (instead of the raw SchemaError), and match Effect's missing-key message case-insensitively so it survives library wording changes. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
b4ac9b7 to
450e54f
Compare
|
Rebased onto main (resolved the import conflict in tool.ts). On the kilo-code-bot review: fixed the two substantive ones - the empty-issues path now returns an actionable message instead of the raw SchemaError, and the missing-key check is case-insensitive so it survives Effect wording changes. I kept the third as inline kilocode_change markers rather than a src/kilocode mirror, since that's the pattern the rest of this file already uses for small changes. The tool-define tests (7) and the workspace typecheck both pass locally. Still relevant on my end - happy to keep it moving. |
| if (result.issues.length === 0) { | ||
| return "The input did not match the expected schema. Please rewrite the arguments so they satisfy it." | ||
| } | ||
| return result.issues.map((issue) => `${path(issue.path)}: ${reason(issue.message)}`).join("\n") |
There was a problem hiding this comment.
This renders every schema violation without a cap. Because validation fails before the normal 50 KiB output truncation in this wrap, the complete error is stored and replayed to the model without application-level truncation. In my test, 1,000 malformed entries expanded 3,011 bytes of arguments into a 52,001-byte, 1,001-line error.
Could you adjust this so the model-facing error remains bounded, for example, render a representative subset of up to 20 issues and 4 KiB, then append …and N more validation errors? Please add a regression test confirming that large malformed arrays stay within the bound while distinct validation failures remain actionable.
| "@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. |
There was a problem hiding this comment.
The current changeset is implementation-focused, but it will appear in user-facing release notes. Could you rewrite it around the user-visible outcome? For example: Help models recover from invalid tool calls with clear, field-specific validation errors.
|
approved if johnny's comments are addressed |
…allbacks - Move formatter/format/path/reason out of the shared upstream tool.ts into packages/opencode/src/kilocode/tool/tool.ts. - Cap output at 20 issues / 4 KiB with an '...and N more' suffix, since validation fails before the normal output truncation. - Return a readable message when the formatter yields zero issues instead of falling back to SchemaError jargon. - Match missing-key messages with a regex instead of an exact string compare. - Rewrite the changeset around the user-visible outcome.
|
Thanks @johnnyeric, both addressed, and I took the bot's three too.
Heads up on verification: my local bun is 1.3.11 against the repo's ^1.3.14, so the pre-push hook wouldn't run and I pushed past it. |
The multi-line reformat left lines 122-125 unannotated in a shared upstream file. Wrap the whole statement in a start/end block instead of per-line inline markers, and drop the two now-redundant inline ones. Comments only.
|
Thanks for addressing the comments! Just merged. |
|
Appreciate the merge and the review, @johnnyeric. Glad the invalid-arguments errors read actionable now. |
…el (Kilo-Org#11961) * fix: make tool invalid-arguments errors clearly actionable to the model * fix: address kilo-code-bot review on schema error formatting Return a jargon-free actionable message when the schema formatter yields no issues (instead of the raw SchemaError), and match Effect's missing-key message case-insensitively so it survives library wording changes. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> * Address review: cap rendered issues, isolate Kilo logic, harden the fallbacks - Move formatter/format/path/reason out of the shared upstream tool.ts into packages/opencode/src/kilocode/tool/tool.ts. - Cap output at 20 issues / 4 KiB with an '...and N more' suffix, since validation fails before the normal output truncation. - Return a readable message when the formatter yields zero issues instead of falling back to SchemaError jargon. - Match missing-key messages with a regex instead of an exact string compare. - Rewrite the changeset around the user-visible outcome. * chore: annotate the reformatted decode call with kilocode_change markers The multi-line reformat left lines 122-125 unannotated in a shared upstream file. Wrap the whole statement in a start/end block instead of per-line inline markers, and drop the two now-redundant inline ones. Comments only. --------- Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Co-authored-by: Christiaan Arnoldus <christiaan.arnoldus@outlook.com>
3 participants
Issue
Fixes #11391
Context
When a tool is called with arguments that fail its parameter schema, the model-facing error
detailwas the raw Effect schema failure string, e.g.SchemaError(Missing key at ["pattern"]). The reported case is a model callinggrepwithout the requiredpatternargument and getting backThe grep tool was called with invalid arguments: SchemaError(Missing key at ["pattern"]). Please rewrite the input so it satisfies the expected schema.TheSchemaError(...)fragment is internal jargon the model cannot act on, so it loops without self-correcting. This makes that detail readable and actionable.Implementation
The root cause is the
detailconstruction in thewrap()closure inpackages/opencode/src/tool/tool.ts:toolInfo.formatValidationError ? toolInfo.formatValidationError(error) : String(error). Built-in tools do not setformatValidationError, sodetailfell through toString(error)— the opaque parse-error text.The fallback now runs a shared formatter that renders one line per failing field as
<json-path>: <reason>(for example["pattern"]: is missing and is required). Because the fix lives at the tool boundary, every built-in tool without its ownformatValidationErrorhook gets the clearer message with no per-tool change. Decoding also runs witherrors: "all", so an input missing several required keys enumerates every offending field instead of stopping at the first. TheInvalidArgumentsErrorenvelope and its typed matchability are unchanged; only thedetailbody becomes readable.Note on API: the plan referenced Effect 3's
ParseResult.ArrayFormatter, but this repo ships Effect4.0.0-beta.66where that API is gone; the implementation uses the v4SchemaIssue.makeFormatterStandardSchemaV1()+Schema.isSchemaError.Screenshots
N/A — model-facing error text, no visual surface.
How to Test
Manual/local verification
bun test ./test/tool/tool-define.test.tsinpackages/opencode/: 7 pass, 0 fail (executed by the agent).bun run typecheckinpackages/opencode/: clean (executed by the agent).Reviewer test steps
packages/opencode/, runbun test ./test/tool/tool-define.test.ts.{}against apatternschema) surfaces["pattern"]with a human-readable missing/required reason and noSchemaError(substring; multiple missing fields enumerate every path; the existing nested-path regression (["questions"][0]["question"]) still holds.Blocked checks and substitute verification
Checklist
Get in Touch
Reachable via GitHub @mvanhorn.