Skip to content

feat(mcp): MCP gateway — aggregate MCP servers behind /mcp#502

Open
SantiagoDePolonia wants to merge 5 commits into
mainfrom
feat/mcp-server
Open

feat(mcp): MCP gateway — aggregate MCP servers behind /mcp#502
SantiagoDePolonia wants to merge 5 commits into
mainfrom
feat/mcp-server

Conversation

@SantiagoDePolonia

@SantiagoDePolonia SantiagoDePolonia commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

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, upstream instructions merged into initialize.
  • POST/GET/DELETE /mcp/{server} — one upstream with original tool names.
  • Upstream transports: streamable HTTP (default), legacy SSE, and stdio (declarative config only — see security).
  • Built on the official modelcontextprotocol/go-sdk v1.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

  • Credential boundary: client bearer keys terminate at the gateway (MCP forbids token passthrough); upstream headers come from config with ${ENV} refs, redacted as *** in admin reads (a *** on PUT preserves the stored secret).
  • Least-privilege discovery: per-server user_paths subtree scoping and allowed_tools/disallowed_tools filter tools/list itself — clients never see tools they cannot call. X-MCP-Servers: a,b narrows a session further.
  • Three config sources with the existing precedence idiom: mcp.servers: YAML map → MCP_SERVERS env (JSON object, wins per name) → mcp_servers admin store (new dashboard MCP Servers page + /admin/mcp-servers CRUD and POST …/{name}/reconnect); declarative entries are read-only in the dashboard. Invalid declarations abort startup loudly.
  • Honest health: a failed listing marks the server degraded and keeps the last catalog (stale carry-forward, like provider inventories), 60s re-probe, 5m re-list, list_changed resync. One shared upstream session per server with redial-once on death (avoids LiteLLM's session-per-operation breakage).
  • Observability: every tools/call writes 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.
  • Session hygiene: bearer auth on every request; sessions additionally bound to the initializing user path (foreign principal presenting a session ID → 404); 30m idle timeout.
  • Feature gate MCP_ENABLED (default true, 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/createMessage from 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: full server.New wiring — bearer auth (401s), aggregated + per-server endpoints, unknown-server 404, disabled-route 404.
  • Real-binary smoke test performed: booted gomodel with MCP_SERVERS env against a live upstream — initialize/tools-list/tools-call verified with curl, usage row persisted to SQLite, admin view reported connected.
  • Dashboard: 15 new node tests (446 total pass). go test ./..., -race on 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
    • Added an authenticated MCP Gateway with /mcp (aggregated, namespaced tools) and /mcp/{server} (single server view).
    • Added admin APIs and an MCP Servers dashboard page to create/edit servers, reconnect, delete, and inspect server catalogs.
    • Enabled MCP gateway by default, with configuration via mcp YAML plus MCP_ENABLED / MCP_SERVERS.
  • Bug Fixes
    • Improved resilience and recovery of upstream catalogs/sessions, including stricter session-to-user-path enforcement and degraded behavior.
  • Documentation
    • Updated feature, configuration, and API endpoint docs for MCP gateway and admin server management.

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>
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@SantiagoDePolonia, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 17 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 26f16df1-b87f-45b8-b849-851687233248

📥 Commits

Reviewing files that changed from the base of the PR and between 12312fd and 393face.

📒 Files selected for processing (1)
  • internal/server/mcp_service_test.go
📝 Walkthrough

Walkthrough

This PR adds an MCP gateway feature: new config and validation, gateway runtime and storage, admin CRUD/reconnect APIs, dashboard UI, /mcp routing, app wiring, end-to-end tests, and docs.

Changes

MCP Gateway Feature

Layer / File(s) Summary
MCP configuration and docs
config/*, .env.template, CLAUDE.md, docs/*, cmd/gomodel/docs/docs.go, go.mod
Adds MCP config types, env merging, validation, default enablement, feature docs, Swagger updates, and SDK dependencies.
Gateway runtime and storage
internal/mcpgateway/*
Defines the MCP gateway model, upstream manager/service, catalog and persistence layers, usage logging, and package tests.
Admin MCP server API
internal/admin/*, cmd/gomodel/docs/docs.go
Adds admin MCP server list/upsert/delete/reconnect/catalog endpoints, request redaction, route registration, and handler tests.
Dashboard MCP Servers UI
internal/admin/dashboard/*
Adds the MCP Servers dashboard page, routing, client module, styles, overview card, and module tests.
HTTP routing and app wiring
internal/core/endpoints.go, internal/server/*, internal/app/app.go, tests/e2e/*
Wires /mcp routing, request gating, app startup/shutdown integration, and end-to-end gateway coverage.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant HTTP as internal/server/mcpService
  participant Gateway as mcpgateway.Service
  participant Manager as mcpgateway.Manager
  participant Upstream

  Client->>HTTP: POST /mcp
  HTTP->>Gateway: ServeHTTP
  Gateway->>Manager: visibleServers / CallTool
  Manager->>Upstream: forward request
  Upstream-->>Manager: result
  Manager-->>Gateway: result
  Gateway-->>Client: relayed response
Loading
sequenceDiagram
  participant Dashboard
  participant AdminHandler
  participant Service as mcpgateway.Service
  participant Store

  Dashboard->>AdminHandler: PUT /admin/mcp-servers
  AdminHandler->>AdminHandler: merge redacted headers
  AdminHandler->>Service: Upsert(ManagedServer)
  Service->>Store: persist row
  Service->>Service: Reload
  AdminHandler-->>Dashboard: redacted view
Loading

Possibly related PRs

Suggested labels: release:feature

Poem

A rabbit hopped where gateways gleam,
And spun a namespaced tool-catalog dream.
With /mcp lit up in bright new hue,
The dashboard tab now peeks right through.
Thump—thump—each server found its seat,
And every hop felt neatly complete.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding an MCP gateway that aggregates MCP servers behind /mcp.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/mcp-server

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@mintlify

mintlify Bot commented Jul 6, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
gomodel 🟢 Ready View Preview Jul 6, 2026, 11:39 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@codecov-commenter

codecov-commenter commented Jul 6, 2026

Copy link
Copy Markdown

@greptile-apps

greptile-apps Bot commented Jul 6, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

This PR appears safe to merge from the reviewed changes.

The MCP gateway core, upstream session handling, server routing, admin handler, configuration, and dashboard paths were reviewed. The earlier reported issues appear addressed. No new runtime, security, or correctness issues were accepted for inline comments.

No files require special attention.

T-Rex T-Rex Logs

What T-Rex did

  • Ran the end-to-end MCP tests with the e2e tag in the repository to validate the MCP gateway workflow.
  • The test run finished with exit status 0 and showed passes for TestMCPGatewayEndToEnd, TestMCPGatewayRequiresAuth, TestMCPGatewayPerServerEndpoint, and TestMCPGatewayDisabled.
  • The full test output was captured to a log artifact for review, including the PASS lines and the overall success.

View all artifacts

T-Rex Ran code and verified through T-Rex

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client as MCP Client
participant HTTP as GoModel HTTP/Auth
participant Gateway as mcpgateway.Service
participant Manager as MCP Manager
participant Upstream as Upstream MCP Server
participant Usage as Usage Logger

Client->>HTTP: "POST/GET/DELETE /mcp or /mcp/{server}\nBearer API key"
HTTP->>HTTP: Authenticate, capture user_path/labels\nPOST: rate limit + budget
HTTP->>Gateway: ServeHTTP(pinnedServer)
Gateway->>Gateway: Validate visible server and session binding
Gateway->>Gateway: Build per-session MCP view\nfilter user_paths + X-MCP-Servers
Gateway->>Manager: Snapshot catalogs
Manager-->>Gateway: Tools/prompts/resources
Gateway-->>Client: initialize/list responses
Client->>Gateway: tools/call
Gateway->>Manager: CallTool(server, original_tool, args)
Manager->>Upstream: Forward tools/call over shared upstream session
Upstream-->>Manager: CallToolResult / error
Manager-->>Gateway: Result / error
Gateway->>Usage: "Record provider=mcp usage entry"
Gateway-->>Client: Relay MCP result
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client as MCP Client
participant HTTP as GoModel HTTP/Auth
participant Gateway as mcpgateway.Service
participant Manager as MCP Manager
participant Upstream as Upstream MCP Server
participant Usage as Usage Logger

Client->>HTTP: "POST/GET/DELETE /mcp or /mcp/{server}\nBearer API key"
HTTP->>HTTP: Authenticate, capture user_path/labels\nPOST: rate limit + budget
HTTP->>Gateway: ServeHTTP(pinnedServer)
Gateway->>Gateway: Validate visible server and session binding
Gateway->>Gateway: Build per-session MCP view\nfilter user_paths + X-MCP-Servers
Gateway->>Manager: Snapshot catalogs
Manager-->>Gateway: Tools/prompts/resources
Gateway-->>Client: initialize/list responses
Client->>Gateway: tools/call
Gateway->>Manager: CallTool(server, original_tool, args)
Manager->>Upstream: Forward tools/call over shared upstream session
Upstream-->>Manager: CallToolResult / error
Manager-->>Gateway: Result / error
Gateway->>Usage: "Record provider=mcp usage entry"
Gateway-->>Client: Relay MCP result
Loading

Reviews (2): Last reviewed commit: "fix(mcp): close dial-vs-close race and g..." | Re-trigger Greptile

Comment thread internal/mcpgateway/upstream.go
Comment thread internal/mcpgateway/store.go Outdated
Comment thread internal/mcpgateway/service.go Outdated
Comment thread internal/mcpgateway/upstream.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

📥 Commits

Reviewing files that changed from the base of the PR and between 14d4bde and deb731b.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (48)
  • .env.template
  • CLAUDE.md
  • cmd/gomodel/docs/docs.go
  • config/config.example.yaml
  • config/config.go
  • config/mcp.go
  • config/mcp_test.go
  • docs/advanced/api-endpoints.mdx
  • docs/advanced/configuration.mdx
  • docs/dev/2026-07-07_mcp-gateway-spec.md
  • docs/docs.json
  • docs/features/mcp-gateway.mdx
  • docs/openapi.json
  • go.mod
  • internal/admin/dashboard/static/css/dashboard.css
  • internal/admin/dashboard/static/js/dashboard.js
  • internal/admin/dashboard/static/js/modules/mcp-servers.js
  • internal/admin/dashboard/static/js/modules/mcp-servers.test.cjs
  • internal/admin/dashboard/templates/index.html
  • internal/admin/dashboard/templates/layout.html
  • internal/admin/dashboard/templates/page-mcp-servers.html
  • internal/admin/dashboard/templates/sidebar.html
  • internal/admin/handler.go
  • internal/admin/handler_mcpservers.go
  • internal/admin/handler_mcpservers_test.go
  • internal/admin/routes.go
  • internal/admin/routes_test.go
  • internal/app/app.go
  • internal/core/endpoints.go
  • internal/mcpgateway/catalog.go
  • internal/mcpgateway/catalog_test.go
  • internal/mcpgateway/factory.go
  • internal/mcpgateway/manager.go
  • internal/mcpgateway/service.go
  • internal/mcpgateway/service_test.go
  • internal/mcpgateway/store.go
  • internal/mcpgateway/store_mongodb.go
  • internal/mcpgateway/store_postgresql.go
  • internal/mcpgateway/store_sqlite.go
  • internal/mcpgateway/store_sqlite_test.go
  • internal/mcpgateway/types.go
  • internal/mcpgateway/upstream.go
  • internal/mcpgateway/usage.go
  • internal/server/handlers.go
  • internal/server/http.go
  • internal/server/mcp_service.go
  • tests/e2e/mcp_test.go
  • tests/e2e/setup_test.go

Comment thread config/mcp.go
Comment thread docs/dev/2026-07-07_mcp-gateway-spec.md
Comment thread internal/mcpgateway/manager.go
Comment thread internal/mcpgateway/service_test.go
Comment thread internal/mcpgateway/store_sqlite_test.go
Comment thread internal/mcpgateway/store.go
Comment thread internal/mcpgateway/store.go
Comment thread internal/mcpgateway/upstream.go
Comment thread internal/mcpgateway/upstream.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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 win

Closed upstream can still leak a session dialed in-flight.

u.closed is only checked before client.Connect, not after. If close() (called from manager.Apply's toClose loop, outside connectMu) runs while a dial is in flight, the completed dial still gets stored into u.session at 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

📥 Commits

Reviewing files that changed from the base of the PR and between deb731b and cf34017.

📒 Files selected for processing (11)
  • config/config.example.yaml
  • config/mcp.go
  • config/mcp_test.go
  • docs/dev/2026-07-07_mcp-gateway-spec.md
  • docs/features/mcp-gateway.mdx
  • internal/mcpgateway/manager.go
  • internal/mcpgateway/service.go
  • internal/mcpgateway/service_test.go
  • internal/mcpgateway/store.go
  • internal/mcpgateway/store_sqlite_test.go
  • internal/mcpgateway/upstream.go

Comment thread internal/mcpgateway/service_test.go
Comment thread internal/mcpgateway/upstream.go
SantiagoDePolonia and others added 2 commits July 7, 2026 12:00
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>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 win

Surface a user-facing error message on generic fetch failures.

Unlike submitMcpServerForm/deleteMcpServer/reconnectMcpServer, which all call mcpServerResponseMessage(...) to populate mcpServerError when a request fails, fetchMcpServersPage's !handled branch (non-401/503/404 failures, e.g. 500) silently clears the list without setting mcpServerError. 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

📥 Commits

Reviewing files that changed from the base of the PR and between cf34017 and 12312fd.

📒 Files selected for processing (19)
  • CLAUDE.md
  • cmd/gomodel/docs/docs.go
  • docs/features/mcp-gateway.mdx
  • docs/openapi.json
  • internal/admin/dashboard/static/css/dashboard.css
  • internal/admin/dashboard/static/js/dashboard.js
  • internal/admin/dashboard/static/js/modules/mcp-servers.js
  • internal/admin/dashboard/static/js/modules/mcp-servers.test.cjs
  • internal/admin/dashboard/templates/page-mcp-servers.html
  • internal/admin/dashboard/templates/page-overview.html
  • internal/admin/handler_mcpservers.go
  • internal/admin/handler_mcpservers_test.go
  • internal/admin/routes.go
  • internal/admin/routes_test.go
  • internal/mcpgateway/catalog_view.go
  • internal/mcpgateway/service_test.go
  • internal/mcpgateway/upstream.go
  • internal/server/mcp_service.go
  • internal/server/mcp_service_test.go

Comment thread internal/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>
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.

2 participants