Skip to content

fix(trimmer): keep tool parameters named like schema keywords - #4036

Merged
seratch merged 1 commit into
openai:mainfrom
Kaif10:fix/trimmer-preserve-property-names
Jul 31, 2026
Merged

seratch merged 1 commit into
openai:mainfrom
Kaif10:fix/trimmer-preserve-property-names

Conversation

@Kaif10

@Kaif10 Kaif10 commented Jul 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

ToolOutputTrimmer silently deletes tool parameters whose names collide with JSON Schema prose keywords.

_trim_json_schema strips description, title, $comment and examples to save tokens, but it recurses into every nested dict and applies that keyword filter uniformly — including to properties, whose keys are user-defined parameter names, not schema keywords. A tool declaring a parameter named description (e.g. create_ticket(title, description)) loses that parameter when its tool_search_output is replayed from an older turn.

The result is not just lossy, it is an invalid schema: the deleted names remain in required while additionalProperties: false still closes the object, so the model receives a schema whose required properties are undeclared. There is no error and no log line. Reproduced through the public filter API (CallModelData), no network or API key:

BEFORE properties: ['description', 'query', 'title']
AFTER  properties: ['query']
AFTER  required  : ['description', 'title', 'query']
required-but-absent params: ['description', 'title']

The fix handles properties before the keyword check, recursing into its values while keeping its keys verbatim. This only narrows what gets deleted — schema-level prose trimming is unchanged, and no public API, signature or field order is touched.

Haystack fixed the same bug class in its own schema walker (_remove_title_from_schema in haystack/tools/from_function.py, commented "Keys of a 'properties' mapping are property names, not schema keywords.").

Knowingly out of scope: $defs/definitions entries keyed by a lowercase keyword name are still dropped — I verified this happens ($defs keys ['Address', 'description'] → ['Address']), which would also dangle a $ref pointing at it. I left it out because that path is much harder to reach in practice — $defs keys come from Pydantic class names, which are conventionally capitalised — and folding it in would add a second permutation to the same abstraction beyond the reported defect. It is a one-line follow-up (if key in {"properties", "$defs", "definitions"}) if you'd rather have it covered here.

Test plan

Added one case to tests/extensions/test_tool_output_trimmer.py, going through the public trimmer call: parameters named description, title, $comment and examples all survive, no required name is missing from properties, and a genuine schema-level description is still stripped (so the trimming feature itself is still exercised).

  • tests/extensions/test_tool_output_trimmer.py: 39 passed with the fix; 1 failed, 38 passed without it (the new test fails on the properties assertion).
  • tests/extensions/ (whole directory): 1158 passed, 6 skipped.
  • ruff check: all checks passed. ruff format --check: 842 files already formatted (no unrelated reformatting).
  • mypy scoped to the two changed files: Success: no issues found.

Verification-script note, per the template: .agents/skills/code-change-verification/scripts/run.sh could not run here — make is not installed on this machine (both run.sh and run.ps1 abort at the make availability check), so I ran the underlying Makefile targets directly via uv run. Unrelated pre-existing failures I confirmed also fail on an unmodified main (ddc39d0): 52 tracing-test failures (KeyError: ('trace_…', 'span_…') from tests/testing_processor.py), one pre-existing pyright error in src/agents/sandbox/util/tar_utils.py:161, and mypy errors confined to sandbox/, examples/ and a Windows-only SIGQUIT reference.

Issue number

None — self-reported; found while reading the trimmer.

Checks

  • I've added new tests, if relevant
  • I've run .agents/skills/code-change-verification/scripts/run.sh — could not: make unavailable locally (see Test plan); ran the equivalent Makefile targets directly instead
  • I've confirmed all verification steps pass (format, lint, typecheck and the affected tests; remaining failures verified pre-existing on main)
  • If using Codex, I've run /review before submitting this PR — n/a

AI assistance was used to draft this change; it was reviewed, reproduced and verified by me.

_trim_json_schema recursed into "properties" and dropped any parameter
named description, title, $comment or examples, leaving those names in
"required" and hiding the parameters from the model.
@seratch seratch added this to the 0.19.x milestone Jul 31, 2026
@seratch
seratch enabled auto-merge (squash) July 31, 2026 00:05
@seratch
seratch merged commit df0b4a2 into openai:main Jul 31, 2026
9 checks passed
@LHMQ878

LHMQ878 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Picked up the $defs case you flagged as out of scope here, in #4110.

Two notes from doing it, in case they're useful: the same class also covers definitions, patternProperties, dependentSchemas and dependentRequired, and separately default/const/enum hold instance data rather than subschemas, so a title key inside a default value was being stripped out of the value itself. patternProperties is only hit when the pattern is an unanchored bare word, which makes it easy to miss.

Your read that the $defs path is hard to reach was right for hand-written schemas, but it does come through function_schema: Pydantic keys $defs by class name, so a nested model named description produces a dangling $ref and the trimmed schema fails validation with PointerToNowhere. Thanks for writing the follow-up out explicitly in the description — made it easy to pick up without duplicating your work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants