Skip to content

feat: Aggregate upstream MCP prompts through prompts/list - #973

Open
nlaurance wants to merge 24 commits into
smart-mcp-proxy:mainfrom
nlaurance:feat/aggregate-upstream-prompts
Open

feat: Aggregate upstream MCP prompts through prompts/list#973
nlaurance wants to merge 24 commits into
smart-mcp-proxy:mainfrom
nlaurance:feat/aggregate-upstream-prompts

Conversation

@nlaurance

Copy link
Copy Markdown
Contributor

Summary

  • Aggregates prompts from connected upstream servers that advertise Capabilities.Prompts into mcpproxy's own prompts/list/prompts/get, alongside the existing built-in prompts.
  • Adds a per-server expose_prompts override so a server can be excluded from aggregation even if it advertises the capability.
  • Scoped to the default (retrieve_tools-mode) server only, per the design spec's non-goals.
  • Also fixes two gaps caught during verification: ExposePrompts wasn't persisted through the BBolt storage layer (would have been lost across a restart), and RefreshPrompts'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 in launcher.TestWaitForURL_InfersDefaultPort, confirmed pre-existing on the base branch)
  • Manual: connect a real upstream advertising prompts and confirm prompts/list/prompts/get surface it correctly

@nlaurance
nlaurance force-pushed the feat/aggregate-upstream-prompts branch from 3055af7 to a4b01b2 Compare August 12, 2026 06:19
@nlaurance nlaurance changed the title Aggregate upstream MCP prompts through prompts/list feat: Aggregate upstream MCP prompts through prompts/list Aug 12, 2026
@codecov-commenter

codecov-commenter commented Aug 12, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 91.50943% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/upstream/core/prompts.go 74.41% 9 Missing and 2 partials ⚠️
internal/upstream/manager_prompts.go 94.20% 2 Missing and 2 partials ⚠️
internal/server/mcp_routing.go 92.30% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@Dumbris Dumbris left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. P1 — Prompts are unreachable over the Streamable HTTP /mcp endpoint in every routing mode. RefreshPrompts/AddPrompt register only on p.server (internal/server/mcp_routing.go:720, mcp.go:1080), but /mcp serves GetMCPServerForMode(cfg.RoutingMode) (internal/server/server.go:774), which returns callToolServer/directServer/codeExecServer — none created with WithPromptCapabilities (mcp_routing.go:563-602). Since config.Validate() normalizes routing_mode to retrieve_tools (internal/config/config.go:2430), the p.server fallback is never hit over HTTP. Verified live: initialize on /mcp advertises no prompts capability and prompts/list returns -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.

  2. P2 — expose_prompts cannot 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 no ExposePrompts branch (only CopyServerConfig was updated). Please add the non-nil-pointer merge branch like InitTimeout/MaxConcurrentRequests, plus a merge_test.go case.
    • File hot-reload: flipping expose_prompts alone bumps the config version but emits no servers.changed, RefreshPrompts never 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.
  3. P2 — Missing SSE request serialization in core.Client.ListPrompts/GetPrompt. ListTools (internal/upstream/core/client.go:300-305) and CallTool (:400-401) take c.sseRequestMu for SSE transports because concurrent requests cause response-delivery failures; the new core/prompts.go methods skip this, so a prompts refresh racing a tool call against an SSE upstream reintroduces exactly that failure mode. Please mirror the ListTools pattern.

  4. P3 — gofmt: gofmt -l flags 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)

  1. Manager.GetPrompt (internal/upstream/manager_prompts.go) lacks the Enabled/Quarantined guards Manager.CallTool enforces (manager.go:1220). QA showed deregistration currently masks this (prompts/get on a quarantined server returns "prompt not found"), but there's a race window between quarantining and the servers.changed-driven refresh during which the proxy would still forward. Defense-in-depth: mirror CallTool's checks.
  2. Pagination: core/prompts.go fetches only the first page (NextCursor ignored). Consistent with ListTools today, so fine as a follow-up — a // TODO would help.
  3. Upstream notifications/prompts/list_changed is not consumed (re-aggregation only on servers.changed) — worth a line in the design doc's non-goals.
  4. Docs: docs/configuration.md has no expose_prompts entry. Test nit: disableOAuthForTest in core/prompts_test.go hand-rolls env save/restore; t.Setenv does 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).
@nlaurance
nlaurance force-pushed the feat/aggregate-upstream-prompts branch from 821428d to 58c3f66 Compare August 17, 2026 12:40
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Aggregate/proxy upstream servers' MCP prompts through prompts/list

3 participants