fix(patch-tool): advertise per-mode required params in schema descriptions#15673
Conversation
There was a problem hiding this comment.
Pull request overview
Improves the patch tool’s schema guidance for strict tool-calling LLM backends by explicitly documenting which parameters are required for each mode, without using conditional JSON Schema constructs that are rejected by some providers.
Changes:
- Restructures
PATCH_SCHEMA["description"]to clearly list required parameters forreplacevspatchmode. - Prefixes conditionally-required parameter descriptions with
REQUIRED when mode='…'. - Adds regression tests to lock the schema shape and ensure provider-compatibility constraints (e.g., no
anyOf/oneOf,required == ["mode"]).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tools/file_tools.py | Updates PATCH_SCHEMA descriptions to advertise per-mode required parameters without changing validation structure. |
| tests/tools/test_file_tools.py | Adds TestPatchSchemaShape to prevent regressions in schema wording/shape and to enforce provider-compat constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tions Models that enforce required-only constraints (e.g. kimi-k2.x) were omitting old_string/new_string for replace mode and patch for patch mode because the schema only declared required: ["mode"]. Add explicit "REQUIRED when mode='X'" markers to each conditionally-required property description and a top-level "REQUIRED PARAMETERS: ..." summary for each mode. Avoids anyOf/oneOf which break Anthropic, Fireworks, and Kimi/Moonshot providers. Add TestPatchSchemaShape to lock the shape. Fixes NousResearch#15524 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
47c581b to
0826b4d
Compare
|
Rebased onto current Re-ran focused tests on the rebased head (
The fix is unchanged from review: PATCH_SCHEMA still uses |
|
Salvaged onto current main in #22143 — merged as commit 8e4f3ba with your authorship preserved via rebase-merge (branch was 1198 commits behind). Trimmed the schema-shape test block from 9 assertions to 2 invariants per Teknium's direction; the actual schema change is exactly as you wrote it. Thanks @briandevans! |
|
|
Problem
Strict LLM backends (e.g. kimi-k2.x, reported in #15524) omit conditionally-required parameters like
old_string/new_stringforreplacemode because thePATCH_SCHEMAonly declaresrequired: ["mode"]. The model sees every other parameter as optional and skips them, resulting in runtime errors.Root cause
JSON Schema has no standard way to express "required if another field equals X" that all providers accept. The obvious fix—
anyOfat the parameters level—breaks at least three providers:anyOf/oneOf/allOfon tool parameter schemas (documented intests/honcho_plugin/test_session.py:376)agent/moonshot_schema.pyhas a sanitizer bug whereanyOfcauses it to striptypethen re-add it, resulting in a schema Kimi rejectsFix
Add explicit guidance in the descriptions — the only universally-accepted signalling mechanism:
descriptionrestructured withREPLACE MODEandPATCH MODEsections, each listingREQUIRED PARAMETERS: ...descriptionprefixed withREQUIRED when mode='replace'/REQUIRED when mode='patch'for the conditionally-required fieldsrequiredarray stays["mode"]— noanyOf/oneOfTests
Added
TestPatchSchemaShape(9 assertions) totests/tools/test_file_tools.pythat lock the schema shape and guard against regressions:REQUIRED when mode=in its descriptionanyOf/oneOfat parameters levelrequired == ["mode"]All 32 tests pass.
Closes #15524