Skip to content

fix(trimmer): keep definition names and instance data in trimmed schemas - #4110

Merged
seratch merged 1 commit into
openai:mainfrom
LHMQ878:fix/trimmer-name-keyed-and-data-keywords
Aug 2, 2026
Merged

fix(trimmer): keep definition names and instance data in trimmed schemas#4110
seratch merged 1 commit into
openai:mainfrom
LHMQ878:fix/trimmer-name-keyed-and-data-keywords

Conversation

@LHMQ878

@LHMQ878 LHMQ878 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #4036, which fixed ToolOutputTrimmer deleting tool parameters whose names collide with JSON Schema prose keywords and explicitly left the $defs case out of scope. This covers that case and one more of the same kind.

_trim_json_schema drops description, title, $comment and examples at every recursion depth. #4036 recognised that properties is keyed by user-chosen parameter names, not schema keywords, and special-cased it. Two groups are still walked as if they were subschemas:

1. Other name-keyed maps$defs, definitions, patternProperties, dependentSchemas, dependentRequired. A definition named description is deleted while the $ref pointing at it survives, so the schema stops resolving. This is the case #4036 flagged as out of scope, and it is reachable through the SDK's own function_schema path, not just hand-written schemas — Pydantic keys $defs by class name, and a nested model class named description or examples produces exactly this:

$defs keys emitted by function_schema: ['description', 'examples']
after trim:                            []
dangling refs: [('note', '#/$defs/description'), ('samples', '#/$defs/examples')]

Through the public filter API (CallModelData, no network or API key), the trimmed schema is not merely lossy — it fails validation outright:

$defs in : ['Priority', 'description']
$defs out: ['Priority']
  #/$defs/description -> resolvable: False
jsonschema.Draft202012Validator(params).validate(...)
  -> PointerToNowhere: '/$defs/description' does not exist

patternProperties is affected whenever the pattern is an unanchored bare word ({"title": …}{}); an anchored ^title$ happens to survive, which is why this is easy to miss.

2. Keywords holding instance data rather than subschemasdefault, const, enum. Nothing inside these is a schema keyword, so a title key in a default value is part of the value:

default in : {'title': 'Untitled', 'description': 'auto', 'retries': 3}
default out: {'retries': 3}

The model is then shown a different default than the tool actually has. enum items and const objects lose the same keys.

The three categories are lifted into named module-level frozensets (_PROSE_SCHEMA_KEYWORDS, _NAME_KEYED_SCHEMA_MAPS, _DATA_SCHEMA_KEYWORDS) so the distinction is stated rather than implied by control flow, and adding a keyword later is a one-line change in one place. This only ever narrows what gets deleted — genuine subschema keywords such as items and propertyNames are still trimmed exactly as before, and no public API, signature or field order changes.

Test plan

Two cases added to tests/extensions/test_tool_output_trimmer.py, both driven through the public trimmer call rather than the private method:

  • test_keeps_definition_names_and_instance_data_in_schema — every $ref still resolves, all five name-keyed maps keep their keys, default/const/enum are preserved byte for byte, and schema-level and nested prose is still stripped (so the trimming feature stays exercised).
  • test_trims_prose_inside_genuine_subschema_keywords — a guard in the opposite direction: items and propertyNames must keep losing their prose, so the fix can't silently grow into a no-op.

Verified:

  • tests/extensions/test_tool_output_trimmer.py: 41 passed with the fix; 1 failed, 40 passed with the src change reverted and the tests kept (fails on the $defs assertion, ['Priority'] != ['Priority', 'description']).
  • tests/extensions/ (whole directory): 1105 passed, 40 skipped, 11 failed. I diffed the failing-test names against clean main (fc084ae2) — the two sets are identical, so there is no regression. They are pre-existing sandbox/session failures (test_bootstrap_persistent_resources_*, test_query_run_loop_*, test_get_items_*, test_runner_with_session_settings_override, test_run_loop_extension_reexports_cloud_bucket_strategy).
  • ruff check: all checks passed. ruff format --check: 2 files already formatted.
  • mypy on the two changed files: the only 2 errors are in sandbox/util/tar_utils.py:161 and extensions/sandbox/modal/sandbox.py:126; I confirmed both reproduce with my changes stashed, so they are pre-existing and untouched by this PR.

Verification-script note: .agents/skills/code-change-verification/scripts/run.sh could not run here — make is unavailable on this machine — so I ran the underlying targets directly.

Issue number

None — self-reported. This is the follow-up #4036 described as "a one-line follow-up ... if you'd rather have it covered here"; the default/const/enum half was not part of that description.

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 targets directly instead
  • I've confirmed all verification steps pass (format, lint, typecheck and the affected tests; remaining failures verified pre-existing on main by diffing failure sets)
  • 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` drops the prose keywords `description`, `title`,
`$comment` and `examples` at every recursion depth. openai#4036 established that
`properties` is keyed by user-chosen parameter names rather than by schema
keywords, and special-cased it. Two related groups were left uncovered:

- Other name-keyed maps: `$defs`, `definitions`, `patternProperties`,
  `dependentSchemas`, `dependentRequired`. A definition named `description`
  is deleted while the `$ref` pointing at it survives, so the schema no
  longer resolves.
- Keywords holding instance data rather than subschemas: `default`, `const`
  and `enum`. A `title` key inside a default value is part of the value, so
  removing it changes the tool's declared contract.

Both are grouped into named frozensets alongside the prose keywords, so the
three categories are stated once instead of implied by control flow. This
only narrows what gets deleted; genuine subschema keywords such as `items`
and `propertyNames` are still trimmed as before.
@seratch seratch added this to the 0.19.x milestone Aug 2, 2026
@seratch
seratch enabled auto-merge (squash) August 2, 2026 21:09
@seratch
seratch merged commit 718acde into openai:main Aug 2, 2026
9 checks passed
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.

2 participants