feat: Aggregate upstream MCP prompts through prompts/list - #973
feat: Aggregate upstream MCP prompts through prompts/list#973nlaurance wants to merge 24 commits into
Conversation
3055af7 to
a4b01b2
Compare
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Dumbris
left a comment
There was a problem hiding this comment.
Review: aggregate upstream MCP prompts through prompts/list
Thanks @nlaurance — this is a well-structured contribution. We checked it out locally (head 821428d), built both editions, ran the new tests under -race, ran the strict CI lint config, and QA'd the running binary against a live prompts-serving upstream (@modelcontextprotocol/server-everything) over both transports. The aggregation core is genuinely solid: correct server__prompt naming with no colon leak, argument passthrough works, quarantined/disabled servers are excluded with no leak on either list or get, graceful all-upstreams-down behavior, clean enable_prompts:false gating, and notifications/prompts/list_changed fires on refresh. Test discipline is unusually good for an external PR.
That said, live QA found one blocking issue the unit tests can't see, plus a few smaller ones.
Blocking
-
P1 — Prompts are unreachable over the Streamable HTTP
/mcpendpoint in every routing mode.RefreshPrompts/AddPromptregister only onp.server(internal/server/mcp_routing.go:720,mcp.go:1080), but/mcpservesGetMCPServerForMode(cfg.RoutingMode)(internal/server/server.go:774), which returnscallToolServer/directServer/codeExecServer— none created withWithPromptCapabilities(mcp_routing.go:563-602). Sinceconfig.Validate()normalizesrouting_modetoretrieve_tools(internal/config/config.go:2430), thep.serverfallback is never hit over HTTP. Verified live:initializeon/mcpadvertises nopromptscapability andprompts/listreturns-32601 "prompts not supported", while the same binary over stdio serves all aggregated prompts correctly. As it stands the headline feature only works for stdio deployments. Fix: register prompts (and the prompts capability) on the routing-mode server instances actually served at/mcp. -
P2 —
expose_promptscannot be toggled at runtime by any path.- REST:
PATCH /api/v1/servers/{id}with{"expose_prompts": false}is rejected with{"success":false,"error":"No fields to update"}—MergeServerConfig(internal/config/merge.go:159+) has noExposePromptsbranch (onlyCopyServerConfigwas updated). Please add the non-nil-pointer merge branch likeInitTimeout/MaxConcurrentRequests, plus amerge_test.gocase. - File hot-reload: flipping
expose_promptsalone bumps the config version but emits noservers.changed,RefreshPromptsnever runs, and the core client's cached ServerConfig stays stale — the toggle only takes effect when coupled with a reconnect-forcing change (enabled/quarantine flip) or restart. It needs to be wired into config change detection too.
- REST:
-
P2 — Missing SSE request serialization in
core.Client.ListPrompts/GetPrompt.ListTools(internal/upstream/core/client.go:300-305) andCallTool(:400-401) takec.sseRequestMufor SSE transports because concurrent requests cause response-delivery failures; the newcore/prompts.gomethods skip this, so a prompts refresh racing a tool call against an SSE upstream reintroduces exactly that failure mode. Please mirror theListToolspattern. -
P3 — gofmt:
gofmt -lflags all four new files (internal/upstream/core/prompts.go,core/prompts_test.go,managed/prompts.go,managed/prompts_test.go— missing trailing newline). Mechanical.
Recommended (non-blocking)
Manager.GetPrompt(internal/upstream/manager_prompts.go) lacks theEnabled/QuarantinedguardsManager.CallToolenforces (manager.go:1220). QA showed deregistration currently masks this (prompts/geton a quarantined server returns "prompt not found"), but there's a race window between quarantining and theservers.changed-driven refresh during which the proxy would still forward. Defense-in-depth: mirror CallTool's checks.- Pagination:
core/prompts.gofetches only the first page (NextCursorignored). Consistent withListToolstoday, so fine as a follow-up — a// TODOwould help. - Upstream
notifications/prompts/list_changedis not consumed (re-aggregation only onservers.changed) — worth a line in the design doc's non-goals. - Docs:
docs/configuration.mdhas noexpose_promptsentry. Test nit:disableOAuthForTestincore/prompts_test.gohand-rolls env save/restore;t.Setenvdoes it in one line.
Verified locally
go build ./... + -tags server both pass · prompt unit tests green under -race · golangci-lint v2 CI config: 0 issues on touched packages · no merge conflicts with main · live QA evidence: stdio prompts/list returns everything__args-prompt, everything__completable-prompt, everything__resource-prompt, everything__simple-prompt + 2 built-ins; prompts/get everything__args-prompt {city, state} echoes arguments; after quarantine: list drops to built-ins and get returns -32602 (no leak).
Happy to merge once items 1-4 are addressed — the core design won't need to change.
…mcp-proxy#972) MCP-770 replaced managed.Client's public Config field with an atomic-pointer-backed GetConfig() accessor while this branch was rebasing onto upstream/main. Update the two prompt-aggregation call sites (added by this PR) to use the accessor instead of the removed field.
…-proxy#972) make swagger picks up the new per-server expose_prompts override on ServerConfig.
…-mcp-proxy#972) Codecov flagged low patch coverage across the prompts-aggregation change. Add tests for the previously-uncovered paths: - managed.Client.ListPrompts/GetPrompt (0% -> 100%): not-connected, success, and upstream-error branches via a real in-process MCP server. - core.Client.ListPrompts/GetPrompt (66% -> 100%): not-connected, nil server info, and upstream-error branches. - Manager.ListPrompts (manager_prompts.go): quarantined-server skip, disabled-server skip, and per-client error skip-and-continue. - MCPProxyServer.RefreshPrompts: disabled no-op, aggregation of built-in + upstream prompts, and the no-upstream-clients case. - CopyServerConfig: ExposePrompts is copied by value, not aliased. Remaining uncovered lines are defensive nil-checks and an error branch unreachable through the public API (Manager.ListPrompts never returns a non-nil error today).
/mcp is served via GetMCPServerForMode(cfg.RoutingMode), which after config.Validate() normalizes routing_mode to retrieve_tools almost never resolves back to p.server. RefreshPrompts and the built-in prompt registration only ever touched p.server, so the aggregated prompts feature was unreachable over Streamable HTTP in every non-default routing mode (live QA: initialize on /mcp advertised no prompts capability, prompts/list returned -32601). Advertise WithPromptCapabilities on the direct/code_execution/call_tool servers too, and set the aggregated prompt set on all of them in RefreshPrompts. Addresses PR smart-mcp-proxy#973 review comment (P1, blocking).
ListPrompts already excludes disabled/quarantined servers from aggregation, but GetPrompt itself had no such check, unlike Manager.CallTool (manager.go:1220). A client that already knows a qualified "server:prompt" name from before a quarantine flip could still forward prompts/get during the race window before the next servers.changed-driven refresh. Addresses PR smart-mcp-proxy#973 review comment (recommended, item 5).
MergeServerConfig had no branch for ExposePrompts (only CopyServerConfig was updated for smart-mcp-proxy#972), so PATCH /api/v1/servers/{id} with {"expose_prompts": false} was rejected with "No fields to update". Add the tri-state pointer-merge branch, mirroring InitTimeout/ MaxConcurrentRequests. Separately, LoadConfiguredServers's hasChanged comparison (the file hot-reload path) didn't count ExposePrompts, so flipping it alone in the config file bumped the config version but never emitted servers.changed — meaning RefreshPrompts never re-ran. Add the missing comparison. Addresses PR smart-mcp-proxy#973 review comment (P2, blocking, part 1).
core.Client.config is set once in NewClient and never reassigned, so even after MergeServerConfig/hot-reload correctly refreshed the ServerConfig, ListPrompts kept enforcing whatever ExposePrompts value was in effect when the connection was created — the toggle only took effect after a reconnect-forcing change or a restart. Add an atomic override on core.Client (SetExposePrompts) and wire managed.Client.SetConfig to push the new value down whenever the upstream Manager refreshes a client's config, without touching the rest of core.Client's config which stays connection-scoped. Also mirror the SSE request-serialization pattern ListTools/CallTool use: ListPrompts/GetPrompt now take c.sseRequestMu for SSE transports, closing the same response-delivery race a concurrent prompts refresh against an SSE upstream would otherwise reintroduce. Addresses PR smart-mcp-proxy#973 review comments (P2, blocking, part 2; P2, blocking, item 3).
…gination TODO) - gofmt managed/prompts.go (missing trailing newline, P3). - Document expose_prompts in the Server Fields table. - Update the design doc's non-goals: the routing-mode-server limitation is fixed (see the mcp_routing.go commit); add the previously-implicit non-goal that notifications/prompts/list_changed isn't consumed. - Note the first-page-only pagination limitation in core/prompts.go (matches ListTools today). Addresses PR smart-mcp-proxy#973 review comments (P3; recommended items 6-8).
821428d to
58c3f66
Compare
Resolve oas/docs.go conflict by regenerating OpenAPI artifacts with 'make swagger' (keeps expose_prompts field alongside main's new endpoints).
The test asserted the FIRST event after the flip is servers.changed, but async supervisor work from the initial load (e.g. activity.quarantine_change) can land after subscribing. Drain events until servers.changed or timeout.
Summary
Capabilities.Promptsinto mcpproxy's ownprompts/list/prompts/get, alongside the existing built-in prompts.expose_promptsoverride so a server can be excluded from aggregation even if it advertises the capability.retrieve_tools-mode) server only, per the design spec's non-goals.ExposePromptswasn't persisted through the BBolt storage layer (would have been lost across a restart), andRefreshPrompts's upstream fan-out had no timeout (could block the shared routing-refresh goroutine on a hung upstream).Closes #972
Test plan
go build ./... && go vet ./... && go test ./internal/... -race(4541 passed; one pre-existing, unrelated failure inlauncher.TestWaitForURL_InfersDefaultPort, confirmed pre-existing on the base branch)prompts/list/prompts/getsurface it correctly