feat(mcp): MCP gateway — aggregate MCP servers behind /mcp#502
Open
SantiagoDePolonia wants to merge 9 commits into
Open
feat(mcp): MCP gateway — aggregate MCP servers behind /mcp#502SantiagoDePolonia wants to merge 9 commits into
SantiagoDePolonia wants to merge 9 commits into
Conversation
GoModel now terminates the Model Context Protocol on both legs: it is an
MCP server to agent clients (streamable HTTP at /mcp and /mcp/{server})
and an MCP client to configured upstream servers (streamable HTTP, legacy
SSE, and declarative-only stdio). Tools, prompts, and resources aggregate
with {server}_{name} namespacing, verbatim schemas, deterministic ordering,
and merged upstream instructions.
The gateway is a credential boundary: client bearer keys authenticate at
the gateway and are never forwarded; upstream headers come from config
with ${ENV} support. tools/list is filtered per caller (server user_paths
subtree scoping, operator tool allow/deny lists, X-MCP-Servers header),
sessions are bound to the initializing user path, and every tools/call is
recorded as a usage entry with labels. MCP POSTs are gated by user-path
rate limits and budgets, and MCP paths are audited model interactions.
Servers come from mcp.servers in config.yaml, the MCP_SERVERS env var
(JSON object merged per name), or the mcp_servers admin store (dashboard
MCP Servers page + /admin/mcp-servers CRUD with reconnect); declarative
entries shadow store rows and are read-only in the dashboard. A failed
listing marks a server degraded while keeping its last catalog, with 60s
re-probe and list_changed-triggered resync. stdio servers are rejected via
the admin API by design (runtime-registered subprocesses are an RCE class).
Built on the official modelcontextprotocol/go-sdk v1.6.1. Spec and
competitor research: docs/dev/2026-07-07_mcp-gateway-spec.md.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an authenticated MCP gateway with configurable upstream servers, namespaced aggregation, per-server access, session and visibility controls, persistence, admin APIs, dashboard management, lifecycle wiring, usage gating, generated API documentation, and end-to-end coverage. ChangesMCP configuration and runtime
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPServer
participant MCPService
participant GatewayService
participant UpstreamManager
participant MCPUpstream
Client->>HTTPServer: POST /mcp
HTTPServer->>MCPService: authenticate and classify request
MCPService->>MCPService: apply rate and budget gates
MCPService->>GatewayService: serve MCP request
GatewayService->>UpstreamManager: resolve visible server/tool
UpstreamManager->>MCPUpstream: forward MCP operation
MCPUpstream-->>UpstreamManager: return result
UpstreamManager-->>GatewayService: return relayed result
GatewayService-->>Client: MCP response and usage record
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 9
🤖 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 `@config/mcp.go`:
- Around line 89-112: `applyMCPEnv` currently overwrites entries when two
`MCP_SERVERS` keys normalize to the same canonical name, which can silently drop
a server. Update `applyMCPEnv` to detect collisions among the env-provided keys
before merging into `cfg.MCP.Servers`, using `canonicalTextKey` consistently and
returning an error when two names resolve to the same canonical key. Keep the
existing merge behavior for distinct names, but fail fast on duplicate canonical
names so the startup behavior matches `normalizeMCPConfig` and does not depend
on map iteration order.
In `@docs/dev/2026-07-07_mcp-gateway-spec.md`:
- Around line 238-247: The Markdown table row in the MCP gateway spec is being
split into extra columns because the protocol name contains unescaped pipe
characters. Update the entry in the handling table so the notifications symbol
for the list_changed protocols is escaped or otherwise formatted as a single
cell, keeping the row under the same table structure. Use the table row
containing notifications/tools|prompts|resources/list_changed and keep the rest
of the matrix unchanged.
In `@internal/mcpgateway/manager.go`:
- Around line 79-93: The background refresh launched in manager.go for each
upstream in the toRefresh loop can still reconnect after that upstream has been
removed or replaced, because upstream.close() does not permanently mark the
instance as disposed. Update the upstream lifecycle in manager.go so there is a
clear split between temporary reset-for-reconnect and permanent disposal, and
make upstream.refresh refuse to dial when the upstream has been disposed. Use
the existing upstream, close, and refresh methods to add a closed/disposed state
check before any reconnect attempt so stale goroutines cannot create new MCP
sessions after reconciliation.
In `@internal/mcpgateway/service_test.go`:
- Around line 380-418: The current test only covers manual Reconnect() after an
upstream dies, but it misses the automatic degraded re-probe recovery path. Add
a regression test alongside TestDegradedUpstreamKeepsCatalogAndReportsError that
simulates a catalog-list failure on the shared session, restarts the upstream,
and then verifies the manager redials and returns to a healthy state without
calling service.Reconnect(). Use the existing helpers and symbols such as
newTestService, service.Views(), StatusDegraded, and connectClient to assert the
catalog is restored and LastError clears after recovery.
In `@internal/mcpgateway/store_sqlite_test.go`:
- Around line 66-78: The update test in store.Upsert is missing the CreatedAt
preservation check that the comment describes. After the initial store.Get call
in store_sqlite_test.go, keep the original CreatedAt value from got and, after
the second Get into updated, assert that updated.CreatedAt matches the original
value along with the existing Description and Enabled checks. Use the existing
test flow and symbols Upsert, Get, got, and updated to place the assertion in
the update-preserve case.
In `@internal/mcpgateway/store.go`:
- Around line 49-74: Validate all fields on ManagedServer in Validate before
mutating the receiver: the current m.Transport and m.URL assignments happen
before the ToolTimeoutSeconds negative check, which can leave partial normalized
state on error. Move the tool_timeout_seconds < 0 guard earlier, or otherwise
defer the m.Transport/m.URL updates until after every validation branch in
Validate and config.ValidateMCPServerConfig has succeeded.
- Around line 32-45: The ManagedServer.Headers field is being persisted in
plaintext, so update the storage flow around ManagedServer in store.go to
protect sensitive header values before they are written. Add encryption or
secure secret handling in the create/update paths for ManagedServer (and any
serialization helpers that write to SQLite/PostgreSQL/MongoDB), and only decrypt
when necessary for authorized use. Make sure the fix is applied where
ManagedServer is stored, not just in the admin read/redaction layer.
In `@internal/mcpgateway/upstream.go`:
- Around line 95-98: The refresh path in upstream.go is keeping a dead MCP
session alive after a failed catalog listing. In `(*upstream).refresh`, when
`u.list` returns an error, clear `u.session` before returning so later probes do
not reuse a closed session; keep the degraded marking, and ensure
`ensureSessionLocked` will redial a new session on the next attempt.
- Around line 244-274: The catalog aggregation in the upstream refresh path
still preserves upstream iteration order for prompts, resources, and resource
templates, which makes published list output nondeterministic. After collecting
items in the refresh logic that appends to fresh.prompts, fresh.resources, and
fresh.templates, sort each slice by a stable key before returning the catalog.
Use the existing deterministic ordering approach already used for tools, and
keep the sorting inside the same refresh flow that consumes session.Prompts,
session.Resources, and session.ResourceTemplates.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 22686570-1a0f-42b0-abc4-558edca4c333
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (48)
.env.templateCLAUDE.mdcmd/gomodel/docs/docs.goconfig/config.example.yamlconfig/config.goconfig/mcp.goconfig/mcp_test.godocs/advanced/api-endpoints.mdxdocs/advanced/configuration.mdxdocs/dev/2026-07-07_mcp-gateway-spec.mddocs/docs.jsondocs/features/mcp-gateway.mdxdocs/openapi.jsongo.modinternal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/mcp-servers.jsinternal/admin/dashboard/static/js/modules/mcp-servers.test.cjsinternal/admin/dashboard/templates/index.htmlinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/page-mcp-servers.htmlinternal/admin/dashboard/templates/sidebar.htmlinternal/admin/handler.gointernal/admin/handler_mcpservers.gointernal/admin/handler_mcpservers_test.gointernal/admin/routes.gointernal/admin/routes_test.gointernal/app/app.gointernal/core/endpoints.gointernal/mcpgateway/catalog.gointernal/mcpgateway/catalog_test.gointernal/mcpgateway/factory.gointernal/mcpgateway/manager.gointernal/mcpgateway/service.gointernal/mcpgateway/service_test.gointernal/mcpgateway/store.gointernal/mcpgateway/store_mongodb.gointernal/mcpgateway/store_postgresql.gointernal/mcpgateway/store_sqlite.gointernal/mcpgateway/store_sqlite_test.gointernal/mcpgateway/types.gointernal/mcpgateway/upstream.gointernal/mcpgateway/usage.gointernal/server/handlers.gointernal/server/http.gointernal/server/mcp_service.gotests/e2e/mcp_test.gotests/e2e/setup_test.go
…sm per review Address PR #502 review findings: - stdio subprocesses no longer inherit the gateway environment (provider API keys, master key); they get PATH/HOME/TMPDIR/USER/LANG plus the explicitly configured env map only. - Split upstream reset (force reconnect) from permanent close; removed upstreams refuse redial so a stale background refresh cannot leak a session after reconciliation. - A failed catalog listing drops the shared session so re-probes always start from a fresh dial; added a regression test that a degraded server recovers via the background refresh path once the upstream returns. - Sort prompts, resources, and templates (not just tools) for deterministic catalogs. - Reject MCP_SERVERS entries whose names collide after canonicalization instead of silently keeping a map-order survivor. - ManagedServer.Validate no longer mutates the receiver before all checks pass; removed an unreachable stdio check; upsert/delete report when a row was persisted but applying it to the running set failed. - Document the stdio minimal environment and the dashboard-managed-credentials-in-store tradeoff (config/env declarations keep secrets out of the database); store CreatedAt-preservation test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/mcpgateway/upstream.go (1)
122-156: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClosed upstream can still leak a session dialed in-flight.
u.closedis only checked beforeclient.Connect, not after. Ifclose()(called frommanager.Apply'stoCloseloop, outsideconnectMu) runs while a dial is in flight, the completed dial still gets stored intou.sessionat Line 151-153 despite the upstream being permanently disposed — leaking a session that will never be closed again.🐛 Proposed fix
session, err = client.Connect(dialCtx, transport, nil) if err != nil { return nil, fmt.Errorf("connect to mcp server %q: %w", u.spec.Name, err) } u.stateMu.Lock() + if u.closed { + u.stateMu.Unlock() + _ = session.Close() + return nil, fmt.Errorf("mcp server %q was removed", u.spec.Name) + } u.session = session u.connectedAt = time.Now().UTC() u.stateMu.Unlock() return session, nil🤖 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 `@internal/mcpgateway/upstream.go` around lines 122 - 156, In ensureSessionLocked, the in-flight connect can finish after the upstream has been closed, which lets a disposed session get stored in u.session. Add a second u.closed check immediately after client.Connect succeeds and before assigning u.session/connectedAt, and if it is closed, close the newly created session and return the removed/closed error instead of keeping it.
🤖 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 `@internal/mcpgateway/service_test.go`:
- Around line 485-512: The current test only covers the sequential
removed-upstream case and misses the concurrent dial-vs-close race in
upstream.go’s ensureSessionLocked/close path. Extend
TestRemovedUpstreamRefusesRedial, or add a new test рядом with it, to start
u.refresh in a goroutine, block it while dialing, then call
service.manager.Apply(nil) concurrently and verify the in-flight dial cannot be
stored into u.session. Keep the assertions that the removed upstream stays
closed and callTool fails, but make the setup explicitly exercise the race
window around ensureSessionLocked and refresh.
In `@internal/mcpgateway/upstream.go`:
- Around line 190-204: The stdio launch path in `upstream.go` leaves `cmd.Env`
nil when both the allowlisted gateway variables and `u.spec.Env` are empty,
causing `exec.Command` to inherit the parent environment. Update the `stdio`
branch so `cmd.Env` is always initialized to an explicit environment slice (even
if empty) before appending allowlisted values and `spec.Env`, ensuring `cmd`
never falls back to `os.Environ` and the subprocess stays isolated.
---
Outside diff comments:
In `@internal/mcpgateway/upstream.go`:
- Around line 122-156: In ensureSessionLocked, the in-flight connect can finish
after the upstream has been closed, which lets a disposed session get stored in
u.session. Add a second u.closed check immediately after client.Connect succeeds
and before assigning u.session/connectedAt, and if it is closed, close the newly
created session and return the removed/closed error instead of keeping it.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: cbf58331-ac94-4f3c-a7e3-b5d0ca624692
📒 Files selected for processing (11)
config/config.example.yamlconfig/mcp.goconfig/mcp_test.godocs/dev/2026-07-07_mcp-gateway-spec.mddocs/features/mcp-gateway.mdxinternal/mcpgateway/manager.gointernal/mcpgateway/service.gointernal/mcpgateway/service_test.gointernal/mcpgateway/store.gointernal/mcpgateway/store_sqlite_test.gointernal/mcpgateway/upstream.go
Two follow-up review findings: - A dial in flight when close() disposed the upstream could complete and store its session on the removed upstream, leaking it until process exit. ensureSessionLocked now re-checks disposal under the state lock after connecting and discards the fresh session. Regression test blocks the upstream mid-dial and races close() against it. - cmd.Env is initialized non-nil for stdio servers: if every allowlisted variable were unset and no env map configured, a nil cmd.Env would fall back to inheriting the full parent environment — exactly the leak the allowlist prevents. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…equest logs
Make MCP traffic and upstream inventories first-class in the UI:
- Catalog inspector: GET /admin/mcp-servers/{name}/catalog returns the
tools, prompts, resources, and templates one server currently exposes
(post-filter, original names); the MCP Servers page gains a read-only
inspector modal per row that also shows the aggregated {server}_{name}
form clients see on /mcp.
- Overview card: a compact MCP Servers connected/total card appears on
the Overview page whenever servers are configured, with a degraded
accent and a link to the MCP Servers page; hidden when the feature is
absent. Loads in parallel with the other overview fetches.
- Request-log labels: MCP POSTs enrich their audit entry with the
JSON-RPC method — the tool or prompt name for calls — and provider
"mcp", so request-log and live-log rows read "github_create_issue"
instead of a bare path. The body peek restores the stream for the MCP
handler untouched.
Verified against the real binary: audit rows show initialize /
notifications/initialized / smoke_ping with provider mcp; the catalog
endpoint lists the upstream tools; headless-Chrome screenshots confirm
the overview card and server page render. Dashboard node tests 446 -> 451.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/admin/dashboard/static/js/modules/mcp-servers.js (1)
246-278: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSurface a user-facing error message on generic fetch failures.
Unlike
submitMcpServerForm/deleteMcpServer/reconnectMcpServer, which all callmcpServerResponseMessage(...)to populatemcpServerErrorwhen a request fails,fetchMcpServersPage's!handledbranch (non-401/503/404 failures, e.g. 500) silently clears the list without settingmcpServerError. The user sees an empty table with no indication the fetch actually failed vs. there simply being no servers configured.🐛 Proposed fix
this.mcpServersAvailable = true; if (!handled) { this.mcpServers = []; + this.mcpServerError = await this.mcpServerResponseMessage(res, 'Failed to load MCP servers.'); return; }🤖 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 `@internal/admin/dashboard/static/js/modules/mcp-servers.js` around lines 246 - 278, The generic failure path in fetchMcpServersPage clears the MCP server list without surfacing any user-facing error, unlike submitMcpServerForm, deleteMcpServer, and reconnectMcpServer. Update the !handled branch after handleFetchResponse(...) so it also sets mcpServerError with mcpServerResponseMessage(...) (or equivalent) for non-401/503/404 failures, while keeping the existing stale-auth and availability handling intact. Use the fetchMcpServersPage method and mcpServerError state as the main touchpoints.
🤖 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.
Outside diff comments:
In `@internal/admin/dashboard/static/js/modules/mcp-servers.js`:
- Around line 246-278: The generic failure path in fetchMcpServersPage clears
the MCP server list without surfacing any user-facing error, unlike
submitMcpServerForm, deleteMcpServer, and reconnectMcpServer. Update the
!handled branch after handleFetchResponse(...) so it also sets mcpServerError
with mcpServerResponseMessage(...) (or equivalent) for non-401/503/404 failures,
while keeping the existing stale-auth and availability handling intact. Use the
fetchMcpServersPage method and mcpServerError state as the main touchpoints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 24b7e443-179a-4723-ac14-ba6dd3153207
📒 Files selected for processing (19)
CLAUDE.mdcmd/gomodel/docs/docs.godocs/features/mcp-gateway.mdxdocs/openapi.jsoninternal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/static/js/modules/mcp-servers.jsinternal/admin/dashboard/static/js/modules/mcp-servers.test.cjsinternal/admin/dashboard/templates/page-mcp-servers.htmlinternal/admin/dashboard/templates/page-overview.htmlinternal/admin/handler_mcpservers.gointernal/admin/handler_mcpservers_test.gointernal/admin/routes.gointernal/admin/routes_test.gointernal/mcpgateway/catalog_view.gointernal/mcpgateway/service_test.gointernal/mcpgateway/upstream.gointernal/server/mcp_service.gointernal/server/mcp_service_test.go
Table-driven tests for mcpService.handle per review: disabled/nil gateway 501, rate-limit breach 429 with Retry-After and no gateway delegation, budget rejection blocking after admission, happy-path delegation (Mcp-Session-Id proves the SDK handler ran), GET bypassing the admission gates, and unknown pinned server 404. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Conflicts were confined to additive blocks landing in the same locations: app.go (MCP gateway init vs main's shared usage reader — kept both), handlers.go imports and Handler fields (mcpgateway vs realtime WebRTC — kept both), and the handler methods block (MCP endpoints vs realtime calls/client_secrets — kept both). Swagger docs regenerated on the merged tree instead of trusting the auto-merge of generated files, and go fix applied to clear pre-existing drift on main (keyring_test.go) that the new fix-check pre-commit hook flags. Verified the mcp: config block passes main's new strict unknown-key validation (#511). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.env.template (1)
70-75: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winExpand
MCP_SERVERSheader placeholders before parsing
.env.template:70-75uses${GITHUB_PAT}, butMCP_SERVERSis decoded withjson.Unmarshaldirectly, so that placeholder will stay literal unless something expands it first. Either apply the same${VAR}expansion used by YAML config or change the example to a form that the loader actually supports.🤖 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 @.env.template around lines 70 - 75, The MCP_SERVERS example currently uses a ${GITHUB_PAT} placeholder that will remain literal if the value is parsed directly by json.Unmarshal, so update the loading path to expand environment placeholders before JSON parsing or change the template to match the supported input format. Focus on the MCP_SERVERS handling and any related config-loading code so the behavior matches the YAML config’s ${VAR} expansion and the example works as documented.internal/core/endpoints.go (1)
32-32: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd MCP cases to
internal/core/endpoints_test.goAdd table-driven coverage for
/mcp,/mcp/{server}, and the POST/GET/DELETE body-mode combinations forOperationMCP.🤖 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 `@internal/core/endpoints.go` at line 32, Add missing table-driven coverage in endpoints_test.go for the new OperationMCP path handling, including /mcp, /mcp/{server}, and the GET/POST/DELETE body-mode combinations. Update the existing endpoint test table and assertions to include the OperationMCP cases alongside the other operation mappings so the new core/endpoints.go constant is exercised.Source: Path instructions
🤖 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.
Outside diff comments:
In @.env.template:
- Around line 70-75: The MCP_SERVERS example currently uses a ${GITHUB_PAT}
placeholder that will remain literal if the value is parsed directly by
json.Unmarshal, so update the loading path to expand environment placeholders
before JSON parsing or change the template to match the supported input format.
Focus on the MCP_SERVERS handling and any related config-loading code so the
behavior matches the YAML config’s ${VAR} expansion and the example works as
documented.
In `@internal/core/endpoints.go`:
- Line 32: Add missing table-driven coverage in endpoints_test.go for the new
OperationMCP path handling, including /mcp, /mcp/{server}, and the
GET/POST/DELETE body-mode combinations. Update the existing endpoint test table
and assertions to include the OperationMCP cases alongside the other operation
mappings so the new core/endpoints.go constant is exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fa9110c0-05ae-4865-a323-5c58fbd3ec18
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.env.templateCLAUDE.mdcmd/gomodel/docs/docs.goconfig/config.example.yamlconfig/config.godocs/advanced/api-endpoints.mdxdocs/docs.jsondocs/openapi.jsongo.modinternal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/dashboard.jsinternal/admin/dashboard/templates/layout.htmlinternal/admin/dashboard/templates/page-overview.htmlinternal/admin/handler.gointernal/admin/routes.gointernal/admin/routes_test.gointernal/app/app.gointernal/core/endpoints.gointernal/providers/keyring_test.gointernal/server/handlers.gointernal/server/http.gointernal/server/mcp_service_test.go
# Conflicts: # internal/app/app.go # internal/server/handlers.go # internal/server/http.go # tests/e2e/setup_test.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/app/app.go (1)
1123-1127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent potential panic from
niloption function.If
mcpResultisnilormcpResult.Serviceisnil,mcpOptionwill be a zero-value (anilfunction). When passed intoadmin.NewHandler's variadic...Optionslice, ranging over the options and invoking them (e.g.,opt(h)) will panic with a nil pointer dereference if the constructor doesn't explicitly checkif opt != nil.Initialize
mcpOptionwith a no-op function to ensure it is always safe to call.🛠 Proposed fix
- var mcpOption admin.Option + mcpOption := func(*admin.Handler) {} if mcpResult != nil && mcpResult.Service != nil { mcpOption = admin.WithMCPServers(mcpResult.Service) }🤖 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 `@internal/app/app.go` around lines 1123 - 1127, Initialize mcpOption in the setup around mcpResult with a no-op admin.Option before the conditional assignment, so it is always safe when passed to admin.NewHandler. Preserve the existing admin.WithMCPServers assignment when mcpResult and its Service are non-nil.
🤖 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.
Outside diff comments:
In `@internal/app/app.go`:
- Around line 1123-1127: Initialize mcpOption in the setup around mcpResult with
a no-op admin.Option before the conditional assignment, so it is always safe
when passed to admin.NewHandler. Preserve the existing admin.WithMCPServers
assignment when mcpResult and its Service are non-nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69f551aa-2dea-404e-9bbc-4cae6253664a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (31)
.env.templateCLAUDE.mdcmd/gomodel/docs/docs.goconfig/config.example.yamlconfig/config.goconfig/mcp.goconfig/mcp_test.godocs/advanced/configuration.mdxdocs/docs.jsondocs/features/mcp-gateway.mdxdocs/openapi.jsongo.modinternal/admin/dashboard/static/css/dashboard.cssinternal/admin/dashboard/static/js/modules/mcp-servers.jsinternal/admin/dashboard/static/js/modules/mcp-servers.test.cjsinternal/admin/dashboard/templates/page-mcp-servers.htmlinternal/admin/dashboard/templates/page-overview.htmlinternal/admin/handler.gointernal/admin/handler_mcpservers.gointernal/admin/handler_mcpservers_test.gointernal/app/app.gointernal/mcpgateway/catalog.gointernal/mcpgateway/catalog_test.gointernal/mcpgateway/factory.gointernal/mcpgateway/service.gointernal/mcpgateway/service_test.gointernal/mcpgateway/store.gointernal/mcpgateway/store_sqlite_test.gointernal/mcpgateway/types.gointernal/mcpgateway/upstream.gointernal/mcpgateway/usage.go
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
2 participants
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.
What
GoModel becomes an MCP (Model Context Protocol) gateway: agent clients (Claude Code, Claude Desktop, Cursor, VS Code, …) connect to one authenticated endpoint with a GoModel API key, and the gateway aggregates every configured upstream MCP server behind it — the same role GoModel already plays between apps and model providers.
POST/GET/DELETE /mcp— aggregated streamable-HTTP MCP endpoint; tools and prompts namespaced{server}_{name}, deterministic order, raw schemas relayed verbatim, upstreaminstructionsmerged intoinitialize.POST/GET/DELETE /mcp/{server}— one upstream with original tool names.modelcontextprotocol/go-sdkv1.6.1 (same choice as Docker MCP Gateway and Envoy AI Gateway).Why
Every comparable gateway (LiteLLM, Bifrost, Kong, Portkey) ships an MCP gateway. Competitor research (docs, source, issue trackers — full write-up in
docs/dev/2026-07-07_mcp-gateway-spec.md) shaped the design around their loudest failures: context bloat from unfiltered tool lists, session-brokering bugs, empty-but-connected upstreams, DB-drift registries, tool-name collisions, and a stdio-via-admin-API RCE (CVE-2026-42271 class).User-visible behavior
${ENV}refs, redacted as***in admin reads (a***on PUT preserves the stored secret).user_pathssubtree scoping andallowed_tools/disallowed_toolsfiltertools/listitself — clients never see tools they cannot call.X-MCP-Servers: a,bnarrows a session further.mcp.servers:YAML map →MCP_SERVERSenv (JSON object, wins per name) →mcp_serversadmin store (new dashboard MCP Servers page +/admin/mcp-serversCRUD andPOST …/{name}/reconnect); declarative entries are read-only in the dashboard. Invalid declarations abort startup loudly.degradedand keeps the last catalog (stale carry-forward, like provider inventories), 60s re-probe, 5m re-list,list_changedresync. One shared upstream session per server with redial-once on death (avoids LiteLLM's session-per-operation breakage).tools/callwrites a usage entry (provider=mcp,provider_name=server,model=namespaced tool, duration/sizes/error, labels + user_path); MCP paths are audited model interactions; MCP POSTs are gated by user-path rate limits and budgets.MCP_ENABLED(defaulttrue, no-op with zero servers).Protocol coverage (v1)
initialize/ping (local), tools/list+call, prompts/list+get, resources/list+read+templates — aggregated/routed. Sampling, elicitation, roots, subscriptions, completion, logging are legally not negotiated (most are deprecated in the upcoming 2026-07-28 spec revision); future work is listed in the spec doc, including serving
sampling/createMessagefrom GoModel's own model routing.Testing
internal/mcpgateway: 11 integration tests driving a real SDK client through the gateway against real SDK upstreams (namespacing, filtering, scoping, session binding, degraded carry-forward, prompts/resources relay, usage attribution, verbatim isError relay) + table-driven naming/filtering/store/config tests; race-clean.tests/e2e/mcp_test.go: fullserver.Newwiring — bearer auth (401s), aggregated + per-server endpoints, unknown-server 404, disabled-route 404.gomodelwithMCP_SERVERSenv against a live upstream — initialize/tools-list/tools-call verified with curl, usage row persisted to SQLite, admin view reportedconnected.go test ./...,-raceon touched packages, full e2e suite,golangci-lint(0 issues), swagger/openapi regenerated.Docs
docs/features/mcp-gateway.mdx(user guide),docs/dev/2026-07-07_mcp-gateway-spec.md(requirements/design + competitor research),.env.template,config/config.example.yaml, configuration/api-endpoints references, CLAUDE.md config reference.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
/mcpand per-server/mcp/{server}endpoints with namespaced tools.Documentation