fix(mcp): preserve 'definitions' as a property name in tool schemas - #56150
Merged
Conversation
The MCP input-schema normalizer in _normalize_mcp_input_schema promotes the
legacy JSON Schema 'definitions' meta-keyword to '$defs' (draft 2019-09+)
so local '$ref' resolution works downstream. The previous walk renamed
*any* key named 'definitions' anywhere in the tree, including inside
'properties' dicts. That turned user-facing parameter names into '$defs',
producing property keys that contain '$', which Anthropic and OpenAI
both reject with HTTP 400 (pattern '^[a-zA-Z0-9_.-]{1,64}$').
Real-world repro: an MCP server that exposes a CI/pipelines tool whose
'definitions' parameter is an array of pipeline-definition IDs. Such a tool
is enough on its own to break every conversation, because the full tools
array is sent on every request.
Fix: when descending into a 'properties' or 'patternProperties' mapping,
iterate property-name -> schema pairs directly, leaving the property names
verbatim. Ordinary JSON Schema semantics resume inside each property's
schema, so a legitimately nested 'definitions' meta-keyword inside a
property's schema is still promoted.
Adds two regression tests:
- test_definitions_as_property_name_is_preserved (the property-name case)
- test_definitions_property_and_meta_keyword_coexist (both forms in one
schema; the property name stays, the meta-keyword promotes)
alt-glitch
added
type/bug
Collaborator
Salvage of #30491 (@mattko…), the earliest-open canonical fix — related, not a duplicate (this is the authoritative salvaged version). Also related to issue #55081 and the competing/dup PRs #55082 and #36955 (the latter a superset with an extra defensive property-key sanitize). A reviewer should close the community PRs in favor of this salvage. |
13 tasks
MaxFreedomPollard
added a commit
to MaxFreedomPollard/hermes-agent
that referenced
this pull request
Sep 4, 2026
…tions to $defs NousResearch#56150 gated properties/patternProperties, but a definitions MAP's keys are member names too. A member literally named "definitions" was renamed to $defs while the reference to it was rewritten to #/$defs/definitions, leaving a dangling $ref that 400s the whole tool array. Also fixes silent member loss when both spellings coexist on one node: normalized["$defs" if key == "definitions" else key] let whichever key came last overwrite the other's members outright. They now merge, with $defs winning a name collision regardless of key order. Rewritten against tools/mcp_tool_schema.py, where the helper now lives after the Sep 2026 decomposition; the original branch targeted tools/mcp_tool.py.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MCP tools that expose a parameter literally named
definitionsno longer get that parameter name corrupted to$defs, which providers reject.Root cause:
_rewrite_local_refsin_normalize_mcp_input_schemapromoted the legacydefinitionsmeta-keyword to$defsby renaming any key nameddefinitionsanywhere in the tree — including keys inside apropertiesdict, which are user-facing parameter names, not schema keywords. A parameter renamed to$defsviolates the provider property-name pattern^[a-zA-Z0-9_.-]{1,64}$($is illegal). Because the full tools array ships on every request, one such tool 400s every conversation, not just calls to that tool.Changes
tools/mcp_tool.py:_rewrite_local_refsnow descends intoproperties/patternPropertiesby iterating name→schema pairs directly, leaving property names verbatim. Thedefinitions→$defsrename and the#/definitions/...→#/$defs/...$refrewrite still apply everywhere else, so a legitimately-nesteddefinitionsmeta-keyword inside a property's schema is still promoted.tests/tools/test_mcp_tool.py: two regression tests —definitionsas a sole property name (preserved), and both forms coexisting in one schema (property preserved, meta-keyword promoted,$refrewritten).Validation
definitionsproperty name$defs→ provider 400definitionsmeta-keyword$defs$defs(unchanged)#/definitions/...$refscripts/run_tests.sh tests/tools/test_mcp_tool.py— 202 passed (incl. 2 new regression tests)._normalize_mcp_input_schemaagainst the three schema shapes above with real imports; all assertions pass.Salvage of #30491 by @MattKotsenas — cherry-picked onto current
mainwith authorship preserved.Infographic