Repository navigation
fix: relay the whole primitive and route what we advertise - #148
Merged
Merged
Conversation
The proxy re-serialized every backend primitive through a typed struct, so title, outputSchema, icons, size and _meta never reached the client. tools/call relays structuredContent verbatim, which left a client with the payload and no schema to validate it against. resources/templates/list had no route while the resources capability was advertised unconditionally, so parameterized backend resources answered 404. Capabilities are now derived from the route table that makes them true, with a test that walks every advertised method through dispatch. A cursor the proxy never issued was answered with page one instead of -32602, which reads to a paginating client as a valid page boundary. Closes #136 Closes #137 Closes #138 Closes #140 Closes #141 Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>
Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Backend cleanup can delete templates owned by another backend with a prefix-overlapping alias, making discovery order-dependent and allowing reconnects or reloads to remove unrelated templates.
1 open issue
What changed in this PR
This PR improves protocol fidelity in the MCP CLI and proxy by preserving unmodeled fields and adding resource-template listing.
Changes:
- Preserve extra fields through serialization, proxy registration, and tool-cache snapshots.
- Discover and list resource templates with namespacing, resource ACL filtering, and cache hints.
- Reject unissued pagination cursors, tie advertised capabilities to route coverage, and update tests and documentation.
| File | Description |
|---|---|
src/transport/cli.rs |
Adapt test fixtures to extended tool structs. |
src/serve/proxy.rs |
Preserve extra fields, register templates, and define capability routes. |
src/serve/e2e_tests.rs |
Update fixtures for template-aware client 8000 installation. |
src/serve/e2e_conformance_tests.rs |
Adapt resource fixtures to the expanded API. |
src/serve/dispatch.rs |
Add template listing, ACL filtering, cursor validation, and tests. |
src/serve/discovery.rs |
Collect templates and fetch optional lists concurrently. |
src/protocol.rs |
Add extra-field preservation, template types, and round-trip tests. |
src/output.rs |
Adapt output test fixtures to extended protocol structs. |
src/client.rs |
Add paginated backend template retrieval. |
src/cli_discovery.rs |
Initialize extra fields for generated tools. |
src/classifier.rs |
Adapt classifier test fixtures. |
docs/reference/architecture.md |
Document template listing and actual pagination behavior. |
docs/guides/proxy-mode.md |
Document template ACLs, cache hints, ordering, and cursor handling. |
Review effort: Balanced
Signed-off-by: Avelino <31996+avelino@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open issues
1 resolved since last review
Review effort: Lite
Comment on lines
+460
to
+472
| let ctx = template_map_snap | ||
| .get(&t.uri_template) | ||
| .map(|(server, orig)| server_auth::ResourceContext { | ||
| server_alias: server.as_str(), | ||
| resource_uri: orig.as_str(), | ||
| }); | ||
| let decision = server_auth::is_resource_allowed( | ||
| identity, | ||
| &t.uri_template, | ||
| acl, | ||
| ctx.as_ref(), | ||
| true, | ||
| ); |
Comment on lines
+1872
to
+1882
| println!( | ||
| "after registering empty `docs`: tools={} resources={} prompts={} templates={}", | ||
| server.tools.len(), | ||
| server.resources.len(), | ||
| server.prompts.len(), | ||
| server.resource_templates.len() | ||
| ); | ||
| assert_eq!(server.tools.len(), 1, "tools"); | ||
| assert_eq!(server.resources.len(), 1, "resources"); | ||
| assert_eq!(server.prompts.len(), 1, "prompts"); | ||
| assert_eq!(server.resource_templates.len(), 1, "templates"); |
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
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
Five conformance bugs in
mcp serveand the CLI, found by reading the 2026-07-28 spec against what the code does. The proxy was dropping spec fields on every*/list, advertising a capability it did not fully route, and accepting a pagination cursor it never issued.Closes #136, #137, #138, #140, #141
Context
mcp servealready speaks the newest revision and is dual-era correct, so the gaps are not about revision support. They are places where the relay is lossy or where the proxy's answer does not match its own behavior.The one that hurts most is
outputSchema.tools/callrelays the backend result verbatim, sostructuredContentreaches the client, buttools/listre-serialized the tool through a typed struct that had no field for the schema. The client got the payload with nothing to validate it against. Same mechanism costtitle,icons,sizeand_metaon all three primitives, andstructuredContenton the CLI path.resources/templates/listhad no route whileproxy_capabilities()advertisedresourcesunconditionally, so a backend exposing parameterized resources answered 404 behind a capability that claimed to support it.On #141 I ended up disagreeing with how I had written the issue. It asked for capabilities derived from the backends. That is the wrong fix here: discovery is lazy, so
initializeknows nothing about backends yet, andserver/discovercarries a one hour TTL, so a client would cache whichever answer it happened to race. A capability is also a statement about this proxy, which answerstools/listwith or without backends. The real defect was a capability advertised without the routes behind it, so that is what this fixes and what the new test locks down.Changes
Unmodeled fields survive the hop.
#[serde(flatten)] extra: ExtraFieldsonTool,Resource,Prompt,ToolCallResultand the newResourceTemplate. The flatten alone was not enough:register_tools,register_resources,register_promptsandsnapshot_cache_entrieseach rebuild the primitive field by field, so all four had to propagateextratoo. A catch-all rather than named fields, so the next spec addition or an extension travels without touching the struct.Resource,PromptandToolCallResultgainedDefaultso test literals can spread it.ContentandPromptArgumentdid not gain a field and did not gain the derive.resources/templates/listend to end. Type,client.list_resource_templates(), a namespaced registry on the proxy, discovery collection and a handler. Templates go through the resource ACL, since a template names the resources of one backend and leaking it tells the caller what exists where they cannot read.install_clienttakes the templates slice rather than letting the caller register them separately, so registration keeps one door. That is the parameter added to six call sites.The three optional discovery lists now go out in one
tokio::join!. The copy-pastedmatch client.list_x()block that existed three times is nowlist_primitives(). Without that, adding a fourth list would have cost every backend an extra round-trip on cold start.Cursor. One guard before the dispatch match, not per arm, because the answer does not depend on the method beyond which methods paginate. A cursor present and non-null on any
*/listgets-32602.cursor: nulland params without the key still pass, since rejecting those would break peers that always send it.Capabilities.
proxy_capabilities()is now derived fromCAPABILITY_ROUTES, which maps each capability to the methods that make it true.every_advertised_capability_is_routedwalks every one of those methods through the real dispatcher.completionsandloggingstay absent because the proxy does not route them.Testing
14 new tests. Every one was watched failing first, and the ones written after their implementation were verified by mutation instead:
extrapropagation inregister_*fails the three registration testsresources/templates/listarm failsevery_advertised_capability_is_routedwithresources is advertised but resources/templates/list is not routedACL coverage for the new method follows the house rule of deny-side first: a caller with no grant on a backend does not see its templates, an identity with no role at all under
default: denygets an empty list, and both the no-ACL and legacy-ACL paths keep listing everything.Suite ran three times after the
tokio::join!change to check for flakiness in the discovery tests.Behavior change
A client that sends
cursoron a*/listused to get the full list and now gets-32602. That is what the spec asks for in every revision, anddocs/reference/architecture.mdclaimed cursor pagination that the serve side never had, which is now corrected.Out of scope
resources/readresolves by exact lookup inresource_map(src/serve/dispatch.rs:562), sosentry__issue://42derived from a template is still unknown. Moving that to a prefix match is an authorization decision, not a relay one, and deserves its own change.listChanged(serve: the catalog changes at runtime but clients are never notified #142),extensions(serve: long tool calls hold a request open for their whole duration #146) andcompletion/complete(serve: completion/complete is not routed to backends #139).*/listhandlers are now the same shape four times. Folding them means one helper taking the ACL decision as a closure, which touches the authorization path of the three that already existed. Worth doing with its own ACL test battery, not here.Verified
docs/reference/architecture.mdanddocs/guides/proxy-mode.mdSize
333 lines of production code, 448 of tests, 9 of docs. If you would rather split it, the natural cut is the field relay (#136, #137) on its own, since it is self-contained and the other three build on nothing it introduces.