[WIP] feat(mcp): serve multiple MCP servers from one router on separate paths - #3151
Draft
asoorm wants to merge 18 commits into
Draft
[WIP] feat(mcp): serve multiple MCP servers from one router on separate paths#3151asoorm wants to merge 18 commits into
asoorm wants to merge 18 commits into
Conversation
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Contributor
WalkthroughMCP configuration now supports multiple named servers mounted on distinct paths. A shared host manages routing, CORS, reloads, and shutdown. Deprecated top-level settings remain supported through a synthetic single-server configuration. ChangesMCP multi-server support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Comment |
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3151 +/- ##
==========================================
+ Coverage 62.37% 62.61% +0.24%
==========================================
Files 262 266 +4
Lines 31003 31217 +214
==========================================
+ Hits 19337 19546 +209
+ Misses 10158 10143 -15
- Partials 1508 1528 +20
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
router/pkg/config/config.schema.json (1)
2741-2772: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRequire non-empty per-entry
oauth.jwkswhen OAuth is enabled.
base_urlcorrectly falls back tomcp.server.base_url, so do not require it per entry. The Go server rejects an entry with emptyoauth.jwks, but schema validation should report this configuration error earlier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/pkg/config/config.schema.json` around lines 2741 - 2772, Update the per-entry schema under the servers additionalProperties object so that enabling oauth requires a non-empty oauth.jwks value, matching the Go server validation. Preserve base_url as optional because it inherits from mcp.server.base_url, and use the existing mcp_oauth definition or conditional schema mechanisms rather than requiring base_url per entry.router/pkg/mcpserver/host.go (1)
102-108: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDisable the fixed write deadline for stateful MCP streams
GraphQLSchemaServer.Serveused the same timeout values, butv1.7.0stateful mode keeps standalone SSEGETresponses open. A 30-secondWriteTimeoutcan reject later events on these streams. SetWriteTimeout: 0or clear the deadline for streaming requests. Use explicit request or session contexts for stream lifetime;IdleTimeoutdoes not limit an active response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@router/pkg/mcpserver/host.go` around lines 102 - 108, Update the http.Server initialization in the host server setup to disable the fixed write deadline by setting WriteTimeout to zero, preserving the existing read and idle timeouts. Ensure stateful MCP standalone SSE streams use their request or session context for lifetime management rather than relying on the HTTP server write timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-website/router/configuration.mdx`:
- Around line 332-337: Update the deprecation notice in the Info block before
the single-server options table to explicitly exclude mcp.enabled,
mcp.server.listen_addr, and mcp.server.base_url from the deprecated options.
Keep the migration guidance for the remaining single-server options and align
the wording with the MCP Configuration page.
In `@docs-website/router/mcp/oauth/configuration.mdx`:
- Around line 127-131: The OAuth configuration examples must use the full
resource identifier including the default /mcp path. Update the resource
response and oauth.jwks[].audiences example values from the host-only URL to
https://mcp.example.com/mcp, while preserving the surrounding configuration and
explanatory text.
In `@router/core/router_test.go`:
- Around line 576-599: Update
TestWarnIgnoredDeprecatedMCPOptionsSkipsUntouchedGraphName so cfg includes
another deprecated MCP option that causes warnIgnoredDeprecatedMCPOptions to
emit a warning. Assert that a warning entry is present, then verify its
ignored_options field contains the other option but excludes mcp.graph_name,
ensuring the graph-name assertion always executes.
In `@router/pkg/config/config.go`:
- Line 1382: Update the MCP server conversion involving MCPServerEntry before
constructing authentication.JWKSConfig: apply the default 1m RefreshInterval and
RefreshUnknownKID values, then copy the configured AllowedUse into the resulting
OAuth configuration. Ensure these resolved values are used for mcp.servers
entries despite MCPServerEntry lacking env tags.
In `@router/pkg/mcpserver/paths_test.go`:
- Line 21: Update the “interior double slash” case in ValidateMountPath tests to
expect validation failure for /a//b, ensuring the test asserts an error rather
than accepting the path.
---
Nitpick comments:
In `@router/pkg/config/config.schema.json`:
- Around line 2741-2772: Update the per-entry schema under the servers
additionalProperties object so that enabling oauth requires a non-empty
oauth.jwks value, matching the Go server validation. Preserve base_url as
optional because it inherits from mcp.server.base_url, and use the existing
mcp_oauth definition or conditional schema mechanisms rather than requiring
base_url per entry.
In `@router/pkg/mcpserver/host.go`:
- Around line 102-108: Update the http.Server initialization in the host server
setup to disable the fixed write deadline by setting WriteTimeout to zero,
preserving the existing read and idle timeouts. Ensure stateful MCP standalone
SSE streams use their request or session context for lifetime management rather
than relying on the HTTP server write timeout.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47a8d347-4ad6-4304-9b9d-ea1c05cb32bc
📒 Files selected for processing (20)
docs-website/router/configuration.mdxdocs-website/router/mcp/configuration.mdxdocs-website/router/mcp/oauth/configuration.mdxrouter/core/graph_server.gorouter/core/router.gorouter/core/router_config.gorouter/core/router_test.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/config_test.gorouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/mcpserver/host.gorouter/pkg/mcpserver/host_test.gorouter/pkg/mcpserver/paths.gorouter/pkg/mcpserver/paths_test.gorouter/pkg/mcpserver/server.gorouter/pkg/mcpserver/server_test.gorouter/pkg/mcpserver/validation.gorouter/pkg/mcpserver/validation_test.go
Comment on lines
+332
to
+337
| <Info> | ||
| This table documents the deprecated, single-server options. Use `mcp.servers` to run one or more MCP servers from | ||
| one router. See [MCP Configuration](/router/mcp/configuration#running-multiple-mcp-servers) for the full | ||
| reference, including the migration path from these options. | ||
| </Info> | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the scope of the deprecation notice.
The block marks the whole table as deprecated. The table includes mcp.enabled (Line 340) and mcp.server.listen_addr (Line 341). docs-website/router/mcp/configuration.mdx Lines 249-250 state that mcp.server.listen_addr and mcp.server.base_url are not deprecated, and Lines 121-122 state that mcp.enabled still controls all servers. Name the exceptions here so the two pages agree.
📝 Proposed wording fix
<Info>
- This table documents the deprecated, single-server options. Use `mcp.servers` to run one or more MCP servers from
- one router. See [MCP Configuration](/router/mcp/configuration#running-multiple-mcp-servers) for the full
- reference, including the migration path from these options.
+ Most options in this table are deprecated single-server options. `mcp.enabled` and `mcp.server.listen_addr`
+ remain current. They control all MCP servers and the shared listener. Use `mcp.servers` to run one or more MCP
+ servers from one router. See [MCP Configuration](/router/mcp/configuration#running-multiple-mcp-servers) for the
+ full reference, including the migration path from these options.
</Info>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Info> | |
| This table documents the deprecated, single-server options. Use `mcp.servers` to run one or more MCP servers from | |
| one router. See [MCP Configuration](/router/mcp/configuration#running-multiple-mcp-servers) for the full | |
| reference, including the migration path from these options. | |
| </Info> | |
| <Info> | |
| Most options in this table are deprecated single-server options. `mcp.enabled` and `mcp.server.listen_addr` | |
| remain current. They control all MCP servers and the shared listener. Use `mcp.servers` to run one or more MCP | |
| servers from one router. See [MCP Configuration](/router/mcp/configuration#running-multiple-mcp-servers) for the | |
| full reference, including the migration path from these options. | |
| </Info> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-website/router/configuration.mdx` around lines 332 - 337, Update the
deprecation notice in the Info block before the single-server options table to
explicitly exclude mcp.enabled, mcp.server.listen_addr, and mcp.server.base_url
from the deprecated options. Keep the migration guidance for the remaining
single-server options and align the wording with the MCP Configuration page.
Comment on lines
+127
to
+131
| <Info> | ||
| This example uses `mcp.server.base_url` and the default `/mcp` path from the deprecated single-server form. With | ||
| `mcp.servers`, each server publishes its own metadata at its own `base_url` and `path`. See | ||
| [Multiple Servers with OAuth Behind a Load Balancer](/router/mcp/configuration#multiple-servers-with-oauth-behind-a-load-balancer). | ||
| </Info> |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect how the resource identifier and metadata path are derived from base_url and mount path.
set -uo pipefail
fd -t f 'paths.go|paths_test.go|server.go' router/pkg/mcpserver --exec rg -n -C 6 'oauth-protected-resource|Resource|base_url|BaseURL|MountPath'Repository: wundergraph/cosmo
Length of output: 13681
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- OAuth configuration example ---'
sed -n '110,155p' docs-website/router/mcp/oauth/configuration.mdx
printf '%s\n' '--- Multiple-server configuration reference ---'
sed -n '220,245p' docs-website/router/mcp/configuration.mdx
printf '%s\n' '--- base_url and path references in the OAuth documentation ---'
rg -n -C 3 'base_url|oauth\.jwks|audiences|resource' docs-website/router/mcp/oauth/configuration.mdx docs-website/router/mcp/configuration.mdxRepository: wundergraph/cosmo
Length of output: 29131
Include /mcp in the resource identifier examples. The server emits https://mcp.example.com/mcp for the default /mcp mount path. Update the resource response and oauth.jwks[].audiences values to match.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-website/router/mcp/oauth/configuration.mdx` around lines 127 - 131, The
OAuth configuration examples must use the full resource identifier including the
default /mcp path. Update the resource response and oauth.jwks[].audiences
example values from the host-only URL to https://mcp.example.com/mcp, while
preserving the surrounding configuration and explanatory text.
Comment on lines
+576
to
+599
| func TestWarnIgnoredDeprecatedMCPOptionsSkipsUntouchedGraphName(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cfg := config.MCPConfiguration{ | ||
| Enabled: true, | ||
| GraphName: defaultMCPGraphName, | ||
| Servers: map[string]config.MCPServerEntry{ | ||
| "support": {Enabled: true, Path: "/mcp/support"}, | ||
| }, | ||
| } | ||
|
|
||
| obsCore, logs := observer.New(zapcore.WarnLevel) | ||
| logger := zap.New(obsCore) | ||
|
|
||
| warnIgnoredDeprecatedMCPOptions(cfg, logger) | ||
|
|
||
| for _, entry := range logs.All() { | ||
| for _, field := range entry.Context { | ||
| if field.Key == "ignored_options" { | ||
| require.NotContains(t, field.Interface, "mcp.graph_name") | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The graph_name assertion never executes.
The config sets only GraphName: defaultMCPGraphName and Servers. warnIgnoredDeprecatedMCPOptions therefore builds an empty ignored slice and returns before it logs. logs.All() is empty, both loops iterate zero times, and require.NotContains never runs. The test passes even if the defaultMCPGraphName comparison is removed.
Set one other deprecated option so a warning is emitted, then assert the warning exists and excludes mcp.graph_name.
💚 Proposed fix to make the assertion effective
cfg := config.MCPConfiguration{
Enabled: true,
GraphName: defaultMCPGraphName,
+ // Force a warning so the graph_name assertion below is reachable.
+ ExcludeMutations: true,
Servers: map[string]config.MCPServerEntry{
"support": {Enabled: true, Path: "/mcp/support"},
},
}
obsCore, logs := observer.New(zapcore.WarnLevel)
logger := zap.New(obsCore)
warnIgnoredDeprecatedMCPOptions(cfg, logger)
- for _, entry := range logs.All() {
- for _, field := range entry.Context {
- if field.Key == "ignored_options" {
- require.NotContains(t, field.Interface, "mcp.graph_name")
- }
- }
- }
+ entries := logs.All()
+ require.Len(t, entries, 1)
+
+ var ignored []string
+ for _, field := range entries[0].Context {
+ if field.Key == "ignored_options" {
+ ignored = field.Interface.([]string)
+ }
+ }
+ require.Contains(t, ignored, "mcp.exclude_mutations")
+ require.NotContains(t, ignored, "mcp.graph_name")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestWarnIgnoredDeprecatedMCPOptionsSkipsUntouchedGraphName(t *testing.T) { | |
| t.Parallel() | |
| cfg := config.MCPConfiguration{ | |
| Enabled: true, | |
| GraphName: defaultMCPGraphName, | |
| Servers: map[string]config.MCPServerEntry{ | |
| "support": {Enabled: true, Path: "/mcp/support"}, | |
| }, | |
| } | |
| obsCore, logs := observer.New(zapcore.WarnLevel) | |
| logger := zap.New(obsCore) | |
| warnIgnoredDeprecatedMCPOptions(cfg, logger) | |
| for _, entry := range logs.All() { | |
| for _, field := range entry.Context { | |
| if field.Key == "ignored_options" { | |
| require.NotContains(t, field.Interface, "mcp.graph_name") | |
| } | |
| } | |
| } | |
| } | |
| func TestWarnIgnoredDeprecatedMCPOptionsSkipsUntouchedGraphName(t *testing.T) { | |
| t.Parallel() | |
| cfg := config.MCPConfiguration{ | |
| Enabled: true, | |
| GraphName: defaultMCPGraphName, | |
| // Force a warning so the graph_name assertion below is reachable. | |
| ExcludeMutations: true, | |
| Servers: map[string]config.MCPServerEntry{ | |
| "support": {Enabled: true, Path: "/mcp/support"}, | |
| }, | |
| } | |
| obsCore, logs := observer.New(zapcore.WarnLevel) | |
| logger := zap.New(obsCore) | |
| warnIgnoredDeprecatedMCPOptions(cfg, logger) | |
| entries := logs.All() | |
| require.Len(t, entries, 1) | |
| var ignored []string | |
| for _, field := range entries[0].Context { | |
| if field.Key == "ignored_options" { | |
| ignored = field.Interface.([]string) | |
| } | |
| } | |
| require.Contains(t, ignored, "mcp.exclude_mutations") | |
| require.NotContains(t, ignored, "mcp.graph_name") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/core/router_test.go` around lines 576 - 599, Update
TestWarnIgnoredDeprecatedMCPOptionsSkipsUntouchedGraphName so cfg includes
another deprecated MCP option that causes warnIgnoredDeprecatedMCPOptions to
emit a warning. Assert that a warning entry is present, then verify its
ignored_options field contains the other option but excludes mcp.graph_name,
ensuring the graph-name assertion always executes.
| ExposeSchema bool `yaml:"expose_schema"` | ||
| OmitToolNamePrefix bool `yaml:"omit_tool_name_prefix"` | ||
| Session MCPServerSessionConfig `yaml:"session,omitempty"` | ||
| OAuth MCPOAuthConfiguration `yaml:"oauth,omitempty"` |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: List envDefault-bearing fields in the MCP OAuth config subtree and the router-side resolvers for mcp.servers entries.
set -euo pipefail
fd -t f 'config.go' router/pkg/config --exec rg -n 'MCPOAuthConfiguration|MCPJWKSConfiguration|JWKS(Configuration)? struct|envDefault' {} \; | rg -n 'MCP|JWKS' || true
echo '--- router-side resolvers ---'
rg -n 'resolveMCPServer|deprecatedServerEntry|MCPServerEntry' router/core --type=go -C3Repository: wundergraph/cosmo
Length of output: 19257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- config declarations ---'
sed -n '650,730p;1335,1450p' router/pkg/config/config.go
echo '--- all OAuth/JWKS references and default-related code ---'
rg -n 'MCPOAuthConfiguration|JWKSConfiguration|JWKS|Refresh|Allowed.*Use|MaxScopeCombinations|envDefault' --glob '*.go' router mcp 2>/dev/null | head -n 300
echo '--- OAuth consumer implementation ---'
rg -l 'WithOAuth|MaxScopeCombinations|RefreshInterval|AllowedKeyUse|JWKS' --glob '*.go' . | head -n 100Repository: wundergraph/cosmo
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- MCP OAuth and JWKS tags only ---'
rg -n -A25 -B3 '^type (JWKSConfiguration|RefreshUnknownKID|MCPOAuthConfiguration|MCPOAuthScopesConfiguration) struct' router/pkg/config/config.go
echo '--- MCP server OAuth conversion ---'
sed -n '225,275p;435,465p;525,555p' router/pkg/mcpserver/server.go
echo '--- authentication JWKS defaults and use handling ---'
sed -n '40,155p' router/pkg/authentication/jwks_token_decoder.go
echo '--- config loading and env.Parse/YAML merge ---'
rg -n -A8 -B8 'env\.Parse|yaml|LoadConfig' router/pkg/config --glob '*.go' | head -n 240Repository: wundergraph/cosmo
Length of output: 28892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- complete MCP OAuth JWKS mapping ---'
sed -n '245,275p' router/pkg/mcpserver/server.go
sed -n '300,330p' router/core/supervisor_instance.go
echo '--- allowed-use conversion and enforcement ---'
rg -n -A35 -B8 'func toJwksetUseType|allowedUse|AllowedUse' router/pkg/authentication --glob '*.go'
echo '--- jwkset dependency version and available source ---'
rg -n 'github.com/.*/jwkset|jwkset' go.mod go.sum router/go.mod router/go.sum 2>/dev/null || true
go env GOPATH GOMODCACHE 2>/dev/null || true
find "$(go env GOMODCACHE 2>/dev/null || echo /nonexistent)" -path '*jwkset*' -type f 2>/dev/null | head -n 30Repository: wundergraph/cosmo
Length of output: 24686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
mod=/home/jailuser/go/pkg/mod/github.com/!micah!parks/jwkset@v0.11.0
echo '--- jwkset refresh option handling ---'
rg -n -A12 -B12 'RefreshInterval|RefreshUnknownKID|RateLimitWaitMax|NewStorageFromHTTP|NewHTTPClient' "$mod" --glob '*.go' | head -n 260
echo '--- repository allowed_use examples and documentation ---'
rg -n -A8 -B8 'allowed_use|refresh_unknown_kid|refresh_interval' --glob '*.yaml' --glob '*.yml' --glob '*.md' --glob '*.go' . | head -n 260Repository: wundergraph/cosmo
Length of output: 41798
Resolve JWKS defaults for mcp.servers entries.
Because MCPServerEntry has no env tags, resolve RefreshInterval to 1m and the RefreshUnknownKID defaults before constructing authentication.JWKSConfig. Also copy AllowedUse; the MCP conversion currently ignores configured allowed_use.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/pkg/config/config.go` at line 1382, Update the MCP server conversion
involving MCPServerEntry before constructing authentication.JWKSConfig: apply
the default 1m RefreshInterval and RefreshUnknownKID values, then copy the
configured AllowedUse into the resulting OAuth configuration. Ensure these
resolved values are used for mcp.servers entries despite MCPServerEntry lacking
env tags.
| {name: "nested", path: "/billing/mcp"}, | ||
| {name: "root", path: "/", wantErr: "shared listener"}, | ||
| {name: "single character", path: "/a"}, | ||
| {name: "interior double slash", path: "/a//b"}, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the multiplexer type and registration used for MCP mount paths.
set -euo pipefail
fd -t f 'host.go' router/pkg/mcpserver --exec cat -n {} \;
echo '--- route registration ---'
rg -n --type=go -C4 'Handle\(|HandleFunc\(|ServeMux' router/pkg/mcpserverRepository: wundergraph/cosmo
Length of output: 12334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- mount-path validation and tests ---'
cat -n router/pkg/mcpserver/paths.go
cat -n router/pkg/mcpserver/paths_test.go
echo '--- declared Go version ---'
rg -n '^(go|toolchain) ' go.mod router/go.mod 2>/dev/null || true
echo '--- standalone ServeMux probe ---'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/go.mod" <<'EOF'
module muxprobe
go 1.22
EOF
cat >"$tmpdir/main.go" <<'EOF'
package main
import (
"fmt"
"net/http"
"net/http/httptest"
)
func main() {
mux := http.NewServeMux()
mux.Handle("/a//b", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, "handler")
}))
for _, path := range []string{"/a//b", "/a/b", "/a//b/" } {
req := httptest.NewRequest(http.MethodGet, path, nil)
handler, pattern := mux.Handler(req)
rec := httptest.NewRecorder()
handler.ServeHTTP(rec, req)
fmt.Printf("path=%q pattern=%q status=%d location=%q body=%q request_path=%q\n",
path, pattern, rec.Code, rec.Header().Get("Location"), rec.Body.String(), req.URL.Path)
}
}
EOF
(cd "$tmpdir" && go run .)Repository: wundergraph/cosmo
Length of output: 6047
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Go runtime ---'
go version
echo '--- standalone ServeMux probe with cgo disabled ---'
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/go.mod" <<'EOF'
module muxprobe
go 1.25
EOF
cat >"$tmpdir/main.go" <<'EOF'
package main
import (
"fmt"
"net/http"
"net/http/httptest"
)
func main() {
mux := http.NewServeMux()
mux.Handle("/a//b", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, "handler")
}))
for _, path := range []string{"/a//b", "/a/b", "/a//b/"} {
req := httptest.NewRequest(http.MethodGet, path, nil)
handler, pattern := mux.Handler(req)
rec := httptest.NewRecorder()
handler.ServeHTTP(rec, req)
fmt.Printf("path=%q pattern=%q status=%d location=%q body=%q request_path=%q\n",
path, pattern, rec.Code, rec.Header().Get("Location"), rec.Body.String(), req.URL.Path)
}
}
EOF
(cd "$tmpdir" && CGO_ENABLED=0 go run .)Repository: wundergraph/cosmo
Length of output: 604
Reject interior // in ValidateMountPath.
http.ServeMux redirects /a//b to /a/b before route matching, so the /a//b handler is unreachable. Update the test case to expect an error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@router/pkg/mcpserver/paths_test.go` at line 21, Update the “interior double
slash” case in ValidateMountPath tests to expect validation failure for /a//b,
ensuring the test asserts an error rather than accepting the path.
asoorm
changed the title
This was referenced Aug 12, 2026
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
Documentation
mcp.serversconfiguration and migration guidance.Closes ROUTER-624.
Motivation
As a platform team, I want to give different agents different tool sets without running a
router per tool set. A support agent needs curated read-only operations, an internal agent
needs write access, a partner agent needs a small public set. Today each of those needs its
own router deployment, which costs money and drifts in configuration.
Changes
The router now mounts several MCP servers on one listener, each on its own path, each with
its own operation collection and its own OAuth policy.
mcp.serversmap, keyed by server name. Each entry setspath,storage.provider_id,an optional
base_url, and the existing per-server MCP options.Hosttype owns the MCP listener and its mux.GraphQLSchemaServerno longer owns anhttp.Server; it registers its own routes at its own mount path.resourceidentifier both derive from the mount path, so a server on
/billing/mcppublisheshttps://billing.example.com/billing/mcprather than a shared/mcp.any server is mounted, because
http.ServeMuxpanics on a duplicate pattern.instead of failing the whole router.
Breaking behaviour, please read
MCP reload can no longer fail the router. Previously an unreadable operations directory
failed router startup and every config reload. Now the affected server serves built-in tools
only and logs an error. Alert on the error log rather than on startup failure. This applies to
existing single-server deployments too.
The top-level
mcpoptions are deprecated. Whenmcp.servershas entries the routerignores all of them and logs a warning naming each one you set. It never merges the two forms.
With no entries the deprecated options build one server on
/mcp, so existing configs keepworking unchanged.
Migrating changes the advertised server identity.
graph_namefeeds theNamefield inMCP
serverInfoaswundergraph-cosmo-<kebab-case graph_name>, and in the map formgraph_namedefaults to the map key. Setgraph_nameexplicitly to keep the old name. SomeMCP clients store trust against it.
Limitations
work, which is why a mount path of
/is rejected: on a shared mux Go treats it as asubtree pattern that swallows every other server's requests.
mcp.serversmap is YAML only. Environment variables cannot address map entries.header allowlists, and token exchange are out of scope.
Config example
Set
oauth.jwks[].audiencesto each server's resource identifier. Without it the routeraccepts a token minted for one server on every other server.
Test plan
Manual check with two servers configured as above:
Reviewer notes
MCPServerEntry.Statelessis a*boolon purpose. It cuts against the repo convention thatbooleans default to false via the zero value, because the top-level default is
trueand aplain bool cannot tell "unset" from "explicitly false". Without the pointer the two config
forms silently disagree on session behaviour. There is a comment saying so.
mcp.server.base_urlstays a global default that per-server entries override. It is notdeprecated. Audience isolation still holds, because the mount path is part of every resource
identifier and validation guarantees distinct mount paths.
Follow-ups, deliberately not in this PR
WWW-Authenticatemetadata URL for a non-default mount path.Consistency currently rests on both call sites using the same helper.
Host.Starthas no double-call guard and returns nil on a bind failure. Pre-existing shape,single caller today.
envDefaulttags onJWKSConfigurationare unreachable for every jwks entry repo-wide,because
env.Parseruns beforeyaml.Unmarshaland the list is empty at parse time. Thedocs claim defaults that never apply, in three places. Separate issue to follow.
Checklist