Repository navigation
fix(mcp): accept boolean JSON Schema property schemas on tools/list - #101810
littlestar030 wants to merge 1 commit into
Conversation
alt-glitch
added
type/bug
Thanks for the note — agreed these are competing mechanisms, not duplicates. This PR matches the upstream mcp fix: accept boolean property sub-schemas as-is on the pinned 2.0.0 models (no-op once bumped). #101804 rewrites them to dicts and adds a stream guard for per-tool isolation. Happy to defer to maintainers on which fits better. |
… server The mcp 2.x SDK validates each tools/list page as a whole against the negotiated protocol version's wire schema before the client sees it (send_request runs mcp_types.methods.validate_server_result first), so one tool the schema rejects fails the entire response. Hermes then parked the server: in NousResearch#101669 one tool out of 312 took every other tool with it, and the log identified the offender only by index (tools.291). Accepting the shape that tripped the reporter — a boolean property subschema, legal JSON Schema — is the SDK model's job: upstream widened it (modelcontextprotocol/python-sdk#3354) and NousResearch#101810 carries that widening for the pinned mcp==2.0.0. This change is the complementary layer the reporter asked for independently: whatever the SDK still rejects should cost that one tool, not the server. Add tools/mcp_listing_guard.py, a read-stream proxy placed between the transport and ClientSession. For each tools/list page it validates every tool on its own against the SDK's versioned Tool model (derived from the same SERVER_RESULTS entry the SDK validates with, so it tracks any widening applied to that model), drops only the tools that fail, and logs each by name with the failing path. A broken page-level field neither hides an invalid tool nor condemns a valid one. Verdicts are latched per connection so a keepalive that falls back to list_tools does not repeat the same WARNING every tick, and anything unexpected passes through untouched, so behaviour is never worse than the SDK's own. MCPServerTask._guard_read_stream wraps the stream at all four ClientSession sites (stdio, SSE, streamable HTTP on both SDK generations), which covers every list_tools caller at once: discovery, tools/list_changed refreshes, the keepalive fallback and `hermes mcp test`. The guard reads the negotiated protocol_version off the live session and, before that is available, falls back to the protocolVersion the server announced in the handshake response that passed through the same stream. On mcp 1.x, which has no per-version surface, it is inert. Dropped tools are recorded on the task (reset on every discovery or refresh) and surfaced as dropped_tools in get_mcp_status(), so a connected server no longer looks perfectly healthy while a tool went missing. Tests drive a real ClientSession over in-memory streams against a fake server: the premise test pins that the SDK rejects the whole page without the guard, and the guarded run keeps the valid tools while dropping the invalid one by name. The boolean-schema verdict test tracks whether the installed model accepts that shape, so it holds with or without NousResearch#101810. The streamable-HTTP arity test now asserts the read stream is handed over wrapped rather than by identity. Fixes NousResearch#101669
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Exact-head review on 865d94711b94a26a3f1551afa7d319fe0e3d1764.
The compat mechanism is directionally correct: it widens the same 2025-11-25 InputSchema.properties and OutputSchema.properties types that upstream python-sdk#3354 widened, rebuilds the dependent models, and clears the cached methods adapter before tools/list validation. The regression also reaches the real versioned mcp_types.methods.validate_server_result("tools/list", "2025-11-25", ...) path rather than only testing Hermes normalization.
Two gates remain before this is landing-ready:
-
Prove the complete compatibility surface this patch changes. The implementation deliberately mutates both
InputSchema.propertiesandOutputSchema.properties, and upstream #3354 carries a regression for both. This PR's wire-level fixture exercises boolean sub-schemas only underinputSchema;outputSchemais never present. Add anoutputSchemacontaining bothtrueandfalseproperty schemas to the versionedtools/listfixture and assert that it survives validation/round-trip. That closes the second half of the actual patch instead of leaving it dependent on inference from the input path. -
Recompose onto the live MCP owner graph instead of extending the old godfile. This head adds 90 lines to
tools/mcp_tool.py, which is still >9,400 lines on this object (line 9,450 exists). Current-main-based #102117 (aec50eb5979afb08af9c1c0d5e6a869412d2af47, base exactlymain@63279301bcbdc185c1b07b98a9312eb0c862f26d) is an active structural carrier that rewritestools/mcp_tool.pyto a 629-line facade and moves MCP responsibilities into bounded sibling modules. This PR and #102117 therefore have a direct owner collision, and old-monolith proof cannot transfer across that rewrite. The landing-safe order is #102117 first, then rebase/recompose this compatibility shim into a bounded MCP compat owner with only the minimal SDK-import hook left in the facade, followed by fresh exact-head proof.
Hosted proof is also not complete for this sole surviving commit: CI 33707140527 is failure with zero dispatched jobs; Docker 33707140402 and Nix 33707140551 are action_required; all label-rerun objects are also action_required. A direct rerun of 33707140527 from the available integration returns 403 Resource not accessible by integration, so there is no green hosted receipt to transfer.
I would re-review the recomposed exact head after the output-schema regression and fresh hosted matrix exist.
|
Thanks for the exact-head review.
|
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact head a84f9766469dcde7c06325a053c0b4012a101bf3 after the requested test follow-up.
The missing compatibility proof is now closed: the real versioned tools/list fixture carries true and false boolean sub-schemas under outputSchema.properties, asserts them after ListToolsResult validation, round-trips them through the wire aliases, and revalidates the dumped result. The idempotency test now covers both input and output boolean schemas as well. I found no new defect in the two-file compatibility slice.
Two landing gates remain, both source-of-truth rather than inferred from the old head:
- #102117 is still open and is now at
e293872ac5cc79a3ce74f5e01a6f28b9d946946e; it rewritestools/mcp_tool.pyfrom the old ~9.4K owner into the 629-line facade. This PR still targets the pre-extraction monolith, so it must recompose after that owner lands (or otherwise carry the compat mechanism into the bounded owner) and rerun the wire regression on the composed object. - Hosted exact-object proof is still absent for the new tip: CI
33789811558, Docker33789810264, and Nix33789810319are allaction_required. The surviving first commit also remains the previously unproved object, so the two-commit train is not every-commit green.
The implementation/test blocker from my prior review is therefore resolved; the requested next object is the bounded-owner recompose with green hosted proof for every surviving commit.
…102117 owners Move the mcp 2.0.0 InputSchema/OutputSchema widening into mcp_tool_schema and keep only the SDK-import hook on the facade. Retargets NousResearch#101669 proof for the post-extraction MCP layout. Co-authored-by: Cursor <[email protected]>
a84f976 to
7bdbdac
Compare
|
Recomposed onto the live #102117 MCP owner graph (exact head based on
PR base retargeted to Hosted CI/Docker/Nix are still behind the fork-PR approval gate on my side — please Approve and run when convenient so we can collect exact-head hosted proof. |
mcp 2.0.0's 2025-11-25 InputSchema/OutputSchema reject properties.x: true/false, parking the whole server. Widen the wire models in mcp_tool_schema and hook from _ensure_mcp_sdk (no-op on mcp>=2.1.0). Follow-up to NousResearch#101810 after NousResearch#102117 landed. Related to NousResearch#101669 (close with complementary NousResearch#101804 isolation).
What does this PR do?
One MCP tool whose
inputSchema.propertiesuses a JSON Schema boolean sub-schema ("refresh": true) makes Hermes failListToolsResultvalidation on negotiated pre-2026 sessions and park the entire server. Boolean schemas are legal in JSON Schema 2020-12; mcp 2.0.0's 2025-11-25 wire models typed property values as objects only.This applies the same widening upstream landed in mcp 2.1.0 (modelcontextprotocol/python-sdk#3354) at Hermes SDK import time, without bumping the pinned
mcp==2.0.0.Related Issue
Fixes #101669
Type of Change
Changes Made
tools/mcp_tool.py:_patch_mcp_boolean_property_schemas()called from_ensure_mcp_sdk(); widens 2025-11-25InputSchema/OutputSchema.properties, rebuilds nested models, clearsmethods._adaptercache; no-op on mcp>=2.1.0tests/tools/test_mcp_boolean_property_schema.py: regression coveringtrue/falseproperty schemas viavalidate_server_result("tools/list", "2025-11-25", ...)for bothinputSchemaandoutputSchema, including dump/validate round-tripHow to Test
uv sync --extra mcp --extra devpython -m pytest tests/tools/test_mcp_boolean_property_schema.py -q"properties": {"refresh": true}and confirm the server loads instead of parking withdict_typeChecklist
Code
Documentation & Housekeeping
Review follow-ups
outputSchemabooleantrue/falsecoverage on the versionedtools/listfixture (plus dump/validate round-trip), matching the InputSchema+OutputSchema surface the patch widens.