Skip to content

fix(mcp): preserve 'definitions' as a property name in tool schemas - #30491

Closed
MattKotsenas wants to merge 1 commit into
NousResearch:mainfrom
MattKotsenas:fix/mcp-definitions-property-name
Closed

fix(mcp): preserve 'definitions' as a property name in tool schemas#30491
MattKotsenas wants to merge 1 commit into
NousResearch:mainfrom
MattKotsenas:fix/mcp-definitions-property-name

Conversation

@MattKotsenas

Copy link
Copy Markdown
Contributor

What does this PR do?

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: a CI MCP server exposes a pipelines_build tool whose definitions parameter is an array of pipeline-definition IDs. Because the full tools array is sent on every request, the broken schema poisons every conversation, not just calls to that one tool.

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 regression case)
  • test_definitions_property_and_meta_keyword_coexist (both forms in one schema; the property stays, the meta-keyword promotes)

Related Issue

None filed. Happy to open one if you'd prefer the issue-first flow; the PR title and body are otherwise the canonical description.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tools/mcp_tool.py: in _normalize_mcp_input_schema._rewrite_local_refs, special-case descent through properties / patternProperties so keys at that level are treated as property names rather than schema keywords. The definitions$defs rename and the #/definitions/...#/$defs/... $ref rewrite continue to apply everywhere else.
  • tests/tools/test_mcp_tool.py: two new regression tests covering (1) definitions as a sole property name, and (2) the both-forms-in-one-schema case where the meta-keyword promotion must still happen.

How to Test

  1. Verify the regression tests cover the bug:
    pytest tests/tools/test_mcp_tool.py::TestMcpTool::test_definitions_as_property_name_is_preserved \
           tests/tools/test_mcp_tool.py::TestMcpTool::test_definitions_property_and_meta_keyword_coexist -v
    
  2. Run the surrounding schema-handling suites to confirm no regressions in the legitimate meta-keyword rewrite path:
    pytest tests/tools/test_mcp_tool.py tests/agent/test_moonshot_schema.py tests/tools/test_schema_sanitizer.py -q
    
    (279 tests pass locally on this branch.)
  3. Manual repro / proof, requires an MCP server whose tool input schema declares a property literally named definitions. Before the fix, every conversation 400s with tools.N.custom.input_schema.properties: Property keys should match pattern '^[a-zA-Z0-9_.-]{1,64}$'. After the fix, the same conversation succeeds; the affected tool's input schema arrives at the provider with "definitions" preserved as a property key.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(mcp): ...)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run pytest tests/tools/test_mcp_tool.py tests/agent/test_moonshot_schema.py tests/tools/test_schema_sanitizer.py -q and all 279 tests pass (did not run the full tests/ suite; happy to if a maintainer wants it)
  • I've added tests for my changes
  • I've tested on my platform: Ubuntu 24.04 on WSL2

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) - docstring on _rewrite_local_refs rewritten to spell out the contextual-rename rule and the failure mode it prevents
  • I've updated cli-config.yaml.example if I added/changed config keys - N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows - N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide - N/A; pure-Python schema transformation, no OS-specific paths
  • I've updated tool descriptions/schemas if I changed tool behavior - N/A; this corrects unintended schema corruption, no tool behavior change

Screenshots / Logs

Provider 400 before the fix (Anthropic-via-Copilot proxy; same wording on Anthropic direct and OpenAI):

HTTP 400: tools.19.custom.input_schema.properties: Property keys should match pattern '^[a-zA-Z0-9_.-]{1,64}$'

Inspecting the offending tool definition in the request body shows properties: {..., "$defs": {...}, "top": {...}} where the MCP server actually emits properties: {..., "definitions": {...}, "top": {...}}. After this change, the wire format matches the server's input.

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 alt-glitch added type/bug Something isn't working tool/mcp MCP client and OAuth P1 High — major feature broken, no workaround labels May 22, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to #14977 (schema-aware MCP normalization preserves property names) — same normalizer code path, different bug. #14977 fixes stray type:object injection; this PR fixes definitions property name being renamed to $defs.

@teknium1

teknium1 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Merged via #56150 — your commit was cherry-picked onto current main with your authorship preserved (dd22c2f53, Author: Matt Kotsenas).

Confirmed the bug on current main and E2E-verified the fix across three schema shapes: definitions as a property name is now preserved, while the definitions meta-keyword (including nested inside a property's schema) is still promoted to $defs and #/definitions/... refs are still rewritten. 202 MCP tests pass plus your two regression tests.

Clean, well-scoped fix with exactly the right tests. Thanks for the contribution!

@teknium1 teknium1 closed this Jul 1, 2026
waefrebeorn pushed a commit to waefrebeorn/slermes that referenced this pull request Jul 2, 2026
Jasper6439 pushed a commit to Jasper6439/hermes-agent that referenced this pull request Jul 5, 2026
habarmc1223-sudo pushed a commit to habarmc1223-sudo/hermes-agent-fluxmem that referenced this pull request Jul 8, 2026
santhreal pushed a commit to santhreal/hermes-agent that referenced this pull request Jul 13, 2026
Gravezzz pushed a commit to Gravezzz/hermes-agent that referenced this pull request Jul 21, 2026
leewenjie pushed a commit to leewenjie/hermes-agent that referenced this pull request Aug 7, 2026
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 High — major feature broken, no workaround sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants