Repository navigation
feat(mcp): share compatibility-aware result conversion across tool surfaces - #43089
Conversation
…rfaces Adds one converter that turns text, JSON, SDK results, interim InputRequiredResult values and exceptions into a CallToolResult shaped for the negotiated MCP revision. Legacy revisions keep object-only structuredContent with a lossless text fallback for other JSON values, and reject interim results through failure accounting. Modern revisions pass arbitrary structuredContent and InputRequiredResult through without completed-success accounting or post-call hooks. OpenAPI tools keep the upstream body verbatim and gain structuredContent Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…erter Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…eout Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@greptileai @veria-ai Please review 87f9a28 for compatibility, accounting, and lossless conversion. Added boundary regressions and clarified the exhaustive return |
|
bugbot run Please review commit 87f9a28, including legacy conversion, interim result handling, error accounting, and the added regression tests |
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
|
@greptileai @veria-ai Please review 72331cb: SSE now uses the SDK legacy-only loop. Live tests confirm modern rejection and unchanged legacy calls, accounting, authentication, cancellation |
|
bugbot run Please review 72331cb, especially the SSE legacy-only SDK loop, preserved lifespan handling, modern-envelope regression, and existing initialization behavior |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 72331cb. Configure here.
Resolve the conflicts with the MCP result conversion change (BerriAI#43089). Its allow_input_required and wire_compat parameters now sit next to on_dispatch in the MCP client, the OBO retry helper, the regular tool path and call_tool, and the test fakes accept both keywords Signed-off-by: Atharva-Kanherkar <142440039+Atharva-Kanherkar@users.noreply.github.com>
TLDR
Problem this solves:
How it solves it:
User Flow
Before: an OpenAPI tool returns JSON only as text
reports-list_reportsthroughPOST http://localhost:4000/mcp/structuredContentAfter: the same tool also returns the parsed object
reports-list_reportsthroughPOST http://localhost:4000/mcp/structuredContentLinear ticket
Resolves LIT-7742
Pre-Submission checklist
Validation
Review found a real exception to the version gate: headerless SSE can select modern mode through its opening message. Commit 72331cb uses the SDK legacy-only loop while preserving its lifespan. New regressions fail before the fix and pass afterward; live testing confirms modern rejection and unchanged legacy listing, calls, authentication, accounting, and cancellation. Greptile rates this commit 5/5, Veria confirms both prior concerns resolved with zero open security findings, and Cursor Bugbot reports no issues. All review threads are resolved
Commit
72331cb7bb7836109e66dfc27caca1c6b13db831: 2,045 affected tests pass with line and branch coverage. Local changed executable-line coverage is 122/123, 99.19%, up from the prior completed Codecov patch report's 97.52%. The completed hosted report covers 124/125 changed executable lines, 99.20%. All 16 uploaded reports are merged, including the MCP rerun; 15 unchanged reports are carried forward. Repository-wide coverage is 77.36%, a separate measureThe sole uncovered changed line is
assert_neverafter every declared input arm returns. No coverage exclusions, thresholds, or budgets changed. All reachable branches in the new conversion modules are covered. Across entire touched functions, including existing authentication and request handling, local branch coverage is 181/216, 83.80%; this is separate from changed-line coverageFifteen added cases cover unsupported methods without HTTP traffic, request-context restoration, exact HTTP body classification, falsy structured values, preservation of image blocks and metadata, input immutability, and headerless modern SSE envelopes on both existing mount variants. The terminal exhaustiveness check now sits after the match to make the explicit return visible to CodeQL
Full
make checkpassed in an independent managed checkout using the frozen canonical environment, with the changed files matching this commit byte-for-byte. This includes full-tree source/test Ruff, formatting, strict-rule, type-discipline, test-quality and basedpyright budget gates, operation-boundary and import checks, E2E type checking, and dashboard API-type synchronization. Independent Devin verification of the exact pushed tree also passes: live legacy MCP, the REST endpoint, SSE admission/authentication/cancellation, and the separately labeled private modern lanes below. CI is green: 89 checks pass and 13 are skipped. The MCP shard passed unchanged on its targeted rerun (380 passed, 5 skipped) after a transient timeout in an existing 200 ms connection-setup deadline; no assertion or timeout was weakenedScreenshots / Proof of Fix
Independent verification used Python 3.12.13 and MCP 2.2.0, real local HTTP upstreams, and source-built proxies at the hashes below. No LLM calls were needed. The normal gateway uses legacy upstream negotiation; modern results below use a separate, explicitly modified test process.
Shared setup: reference MCP upstream on
8765, OpenAPI service on8766, proxy on4000.LITELLM_KEYis a local test master key. Save the fixtures below in a separate directory, setTREEto the selected checkout, and use the frozen proxy development environment.For the private modern lane only, copy
config_base.yamltoconfig.yaml, addcallbacks: harness_callbacks.trackerunderlitellm_settings, and add$FIXTUREStoPYTHONPATH. This harness changes both the downstream version gate and upstream negotiation. It is not shipped or evidence that production enables modern MCP.Reproduction fixtures, including the explicit private modern harness
upstream.py
legacy_proof.sh
modern_harness.py
modern_proof.py
harness_callbacks.py
SSE probe, against the normal gateway without the modern harness: open an authenticated SSE connection without a protocol header, then post to its returned session URL. Use a fresh connection for each opening request. The independent run additionally used a logging-only callback and a raw slow upstream to check unchanged cancellation and hook behavior.
REST probe: look up
reportswithGET /v1/mcp/server, then use its ID with the actual mounted REST route:Before (27d1974)
Legacy tool calls
bash legacy_proof.shagainst the normal gateway. Each requested revision (2024-11-05,2025-03-26,2025-06-18,2025-11-25) is echoed by initializationreports-list_reportsreturns only text:{"content":[{"text":"{\"total\":1.1,\"items\":[]}","type":"text"}],"isError":false}reports-list_tagsreturns text["a","b"]withoutstructuredContent. Nativeref-object_resultretains{"answer":42};ref-array_resultandref-needs_inputboth returnisError:truewithMCPError: Handler returned an invalid resultbecause the upstream SDK rejects them under legacy negotiationREST result shape
list_tags. Both return HTTP 200list_reportscontains the exact JSON text and"structuredContent":null;list_tagsalso has null structure. Both already contain"resultType":"complete"and null metadata fields through FastAPI serializationPrivate modern propagation
python modern_proof.py needs_input ref-array_result reports-list_tags reports-list_reports ref-object_result. Both connections negotiate2026-07-28needs_inputreturns an errorCallToolResult:RuntimeError: Server returned InputRequiredResult; pass allow_input_required=True to receive it and retry call_tool(..., input_responses=..., request_state=result.request_state).The failure handler and post-call hook runstructuredContent:[1,2]Private modern upstream with legacy downstream
bash legacy_proof.shagain. The downstream uses each legacy revision while the upstream negotiates2026-07-28ref-array_resultfails on the gateway wire:{"code":-32603,"message":"Handler returned an invalid result"}SSE protocol admission
server/discoverprobe above. The merge base accepts the headerless modern envelope and advertisessupportedVersions:["2026-07-28"]tools/listinstead also succeeds and emitsresultType:"complete". This admission gap already exists at the merge baseGET /mcp/ssereturns HTTP 401After (72331cb)
Legacy tool calls
bash legacy_proof.shagainst the normal gateway. All four initialization revisions match the baselinereports-list_reportsretains identical text and adds the parsed object:{"content":[{"text":"{\"total\":1.1,\"items\":[]}","type":"text"}],"isError":false,"structuredContent":{"total":1.1,"items":[]}}resultTypeis added to these legacy MCP framesREST result shape
list_tags. Both still return HTTP 200list_reportsnow has"structuredContent":{"total":1.1,"items":[]}. Original text,resultType, null metadata, and the array response shape remain unchanged. Independent logging captures confirm the same success/error accounting and post-call hooksPrivate modern propagation
modern_proof.pycommand. Both connections negotiate2026-07-28needs_inputnow returns the original interim result:{"_meta":{"io.modelcontextprotocol/serverInfo":{"name":"ref-upstream","version":""}},"resultType":"input_required","inputRequests":{"req-1":{"method":"elicitation/create","params":{"mode":"form","message":"Pick one","requestedSchema":{"properties":{},"type":"object"}}}},"requestState":"state-1"}structuredContent:["a","b"]; objects gainstructuredContent:{"total":1.1,"items":[]}. Both retain original text. Native array structure remains unchangedPrivate modern upstream with legacy downstream
bash legacy_proof.shagainref-array_resultnow retains its existing block and appends the serialized array, without legacy-incompatible structure:{"_meta":{"io.modelcontextprotocol/serverInfo":{"name":"ref-upstream","version":""}},"content":[{"text":"Done","type":"text"},{"text":"[1, 2]","type":"text"}],"isError":false}SSE protocol admission
server/discoverprobe. The response is now:{"jsonrpc":"2.0","id":0,"error":{"code":-32601,"message":"Method not found","data":"server/discover"}}tools/listinstead returns-32602,Invalid request parameters. No tool calls reach the upstream and no tool hooks runType
New feature, refactoring, tests
Caveats
The live hybrid-upstream probe did not reproduce interim accounting or guardrail bypass: the SDK strips later-revision fields on legacy upstream connections and normal hooks run. The confirmed defect was SSE modern admission, now closed. Interim charging and output inspection remain requirements for any future modern activation
Medium
Low
Final Attestation