feat(backend): inject MCP servers in CLI passthrough mode per agent#1158
feat(backend): inject MCP servers in CLI passthrough mode per agent#1158carlosflorencio wants to merge 8 commits into
Conversation
|
Claude finished @carlosflorencio's task in 5m 27s —— View job FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready with suggestions — the design is sound, the strategy abstraction is clean, cleanup is properly wired (verified in |
📝 WalkthroughWalkthroughThis PR refactors how MCP servers are injected into passthrough-mode agent invocations by introducing a strategy pattern that lets each CLI (Claude, Codex, Cursor, OpenCode) emit its own config format. The runtime resolves servers once and delegates materialization to agent-specific strategies, replacing the prior monolithic config-flag approach. ChangesPassthrough MCP Strategy Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go | Core passthrough lifecycle logic with MCP injection, symlink-escape detection, O_EXCL writes, and cleanup tracking; writePassthroughMCPFiles has a latent tracking gap on error that would orphan already-written files in any future multi-file strategy. |
| apps/backend/internal/agent/mcpconfig/passthrough.go | New file implementing four PassthroughMCPStrategy implementations (Claude, Codex, Cursor, OpenCode); serialization, filtering, and TOML key quoting are correct; empty-entries guard present after previous fix. |
| apps/backend/internal/agent/agents/passthrough.go | BuildPassthroughCommand correctly appends MCPArgs last so Claude's variadic --mcp-config cannot swallow a positional prompt; logic is clean and well-tested. |
| apps/backend/internal/agent/mcpconfig/passthrough_test.go | Thorough unit tests covering all four strategies, empty-entries guard, TOML quoting, transport mapping, and the interface-implementation check. |
| apps/backend/internal/agent/runtime/lifecycle/manager_passthrough_test.go | Good integration-level coverage for all four agents, union-tracking across relaunches, symlink escape, redaction, cleanup on RemoveExecution, and MCP port absence errors. |
Reviews (7): Last reviewed commit: "fix(backend): redact MCP secrets at inte..." | Re-trigger Greptile
![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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/backend/internal/agent/mcpconfig/passthrough.go (1)
153-205: 💤 Low valueCodex server names are interpolated raw into the dotted
-ckey path.
mcp_servers."+srv.Name+"."+p.keyinlinessrv.Nameinto Codex's dotted config-key path. A profile server whose name contains a.(or other char Codex treats specially in a key path) would silently target the wrong nested key. kandev/github names are safe, but profile-supplied names may not be — worth confirming Codex's key-path grammar and/or sanitizing names.Codex CLI -c override config key path syntax for nested keys with dots, e.g. mcp_servers.<name>.command🤖 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 `@apps/backend/internal/agent/mcpconfig/passthrough.go` around lines 153 - 205, The code currently in codexServerArgs inlines srv.Name into the dotted override key ("mcp_servers."+srv.Name+"."+p.key) which can silently mis-target nested keys if the name contains '.' or other special characters; fix this by validating/sanitizing server names early: in codexServerArgs (or called from BuildPassthroughMCP) ensure srv.Name matches a safe pattern (e.g. regex ^[A-Za-z0-9_-]+$) and return a clear error if it contains disallowed characters (like '.'), or alternatively normalize/escape disallowed characters before composing the "-c" token so the produced key path cannot be interpreted as nested (update the error text and validation in codexServerArgs and callers accordingly).
🤖 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/decisions/0011-passthrough-mcp-injection-strategies.md`:
- Line 40: The table's OpenCode cell contains unescaped pipe characters (the
JSON snippet referencing OPENCODE_CONFIG and the
{"mcp":{name:{type:"local"|"remote",…}}} payload) which breaks the 4-column
table; fix by escaping the inner pipe characters or wrapping the entire JSON
snippet in inline code/backticks so the pipes are treated as literal characters
(update the cell that includes OpenCode and OPENCODE_CONFIG to use escaped pipes
or backticks to restore proper Markdown table rendering).
---
Nitpick comments:
In `@apps/backend/internal/agent/mcpconfig/passthrough.go`:
- Around line 153-205: The code currently in codexServerArgs inlines srv.Name
into the dotted override key ("mcp_servers."+srv.Name+"."+p.key) which can
silently mis-target nested keys if the name contains '.' or other special
characters; fix this by validating/sanitizing server names early: in
codexServerArgs (or called from BuildPassthroughMCP) ensure srv.Name matches a
safe pattern (e.g. regex ^[A-Za-z0-9_-]+$) and return a clear error if it
contains disallowed characters (like '.'), or alternatively normalize/escape
disallowed characters before composing the "-c" token so the produced key path
cannot be interpreted as nested (update the error text and validation in
codexServerArgs and callers accordingly).
🪄 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: CHILL
Plan: Pro
Run ID: 2e8dedcb-bdda-4d8e-808f-b22a6b7e057f
📒 Files selected for processing (13)
apps/backend/internal/agent/agents/agent.goapps/backend/internal/agent/agents/claude_acp.goapps/backend/internal/agent/agents/codex_acp.goapps/backend/internal/agent/agents/cursor_acp.goapps/backend/internal/agent/agents/helpers.goapps/backend/internal/agent/agents/opencode_acp.goapps/backend/internal/agent/agents/passthrough.goapps/backend/internal/agent/mcpconfig/passthrough.goapps/backend/internal/agent/mcpconfig/passthrough_test.goapps/backend/internal/agent/runtime/lifecycle/manager_passthrough.goapps/backend/internal/agent/runtime/lifecycle/manager_passthrough_test.godocs/decisions/0011-passthrough-mcp-injection-strategies.mddocs/decisions/INDEX.md
💤 Files with no reviewable changes (1)
- apps/backend/internal/agent/agents/helpers.go
|
Claude finished @carlosflorencio's task in 3m 5s —— View job FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready with suggestions — the strategy abstraction is clean, the four CLI-specific implementations are well-tested, cleanup is correctly wired, the metadata dual-shape tolerance (in-memory vs JSON-rehydrated) is thorough, and the previous reviewer's four suggestions (Codex dot-in-name, ps-visible tokens, redundant config fetch, missing port-absent tests) were all addressed in the follow-up commit |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 13 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go">
<violation number="1" location="apps/backend/internal/agent/runtime/lifecycle/manager_passthrough.go:374">
P1: Validate parent directories against symlinks before `MkdirAll`/`WriteFile`; otherwise a symlinked `.cursor` path can redirect writes outside the intended workspace.
(Based on your team's feedback about validating parent chain before MkdirAll.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/backend/internal/agent/mcpconfig/passthrough.go">
<violation number="1" location="apps/backend/internal/agent/mcpconfig/passthrough.go:109">
P3: Add a guard for `len(entries) == 0` before writing the Claude config; otherwise an empty `mcpServers` file is still emitted and `--mcp-config` is injected with no effective servers.</violation>
<violation number="2" location="apps/backend/internal/agent/mcpconfig/passthrough.go:184">
P1: Avoid passing `srv.Env` and `srv.Headers` through `-c` argv overrides because this exposes secret values in the process command line to other local users.</violation>
<violation number="3" location="apps/backend/internal/agent/mcpconfig/passthrough.go:202">
P2: Codex passthrough builds `-c` keys from unvalidated server names, so names with `.` (or other path-significant chars) can produce incorrect `mcp_servers` paths and fail to load the intended server.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready with suggestions — the strategy abstraction is clean, the four CLI-specific implementations are well-tested, cleanup is correctly wired, the metadata dual-shape tolerance (in-memory vs JSON-rehydrated) is thorough, and the previous reviewer's four suggestions were all addressed in the follow-up commit (84bb172). The two remaining items are both minor. |
|
Claude finished @carlosflorencio's task in 3m 24s —— View job FindingsSuggestion (recommended, doesn't block)
Summary
Verdict: Ready with suggestions — all four previous review rounds' findings (Codex dot-in-name, ps-visible tokens, redundant config fetch, missing port-absent tests, ClaudeStrategy empty-entries guard, ADR pipe escaping, symlink containment) were fully addressed by f098366. The three remaining items are minor consistency/cleanup issues; none block merge. |
|
Claude finished @carlosflorencio's task in 3m 27s —— View job FindingsAll previous-round findings (Codex TOML dot-quoting, ps-visibility comment, redundant No new issues found in the current tree. Summary
Verdict: Ready to merge — the strategy abstraction is clean, all four CLI-specific implementations are well-tested (including the edge-case |
FindingsAll previous-round findings (Codex TOML dot-quoting, ps-visibility comment, redundant No new issues found in the current tree. Summary
Verdict: Ready to merge — the strategy abstraction is clean, all four CLI-specific implementations are well-tested (including the edge-case |
|
Claude finished @carlosflorencio's task in 2m 12s —— View job FindingsAll previous-round findings have been resolved across commits
The new commit ( Summary
Verdict: Ready to merge — the strategy abstraction is clean, all four CLI-specific implementations are well-tested (including |
|
Claude finished @carlosflorencio's task in 5m 7s —— View job Findings
FindingsBlocker (must fix before merge)
Summary
Verdict: Blocked — the security fix introduced in |
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Passthrough mode previously wired only kandev's own HTTP server (Claude only) into MCP config; codex/cursor/opencode got nothing and profile-configured servers were never applied. Add a per-agent PassthroughMCPStrategy that materializes the resolved server list (kandev + profile servers, via the same resolver ACP uses) into each CLI's native shape without touching the user's global config: - Claude: temp file + --mcp-config (additive) - Codex: repeated -c mcp_servers.<name>.<key>=<json> overrides - Cursor: project-local .cursor/mcp.json, created only if absent - OpenCode: temp file + OPENCODE_CONFIG env var Replaces the old MCPConfigFlag/MCPConfigPath mechanism. See ADR 0011.
- Quote Codex server names containing dots so the -c key path isn't misread as TOML nesting - Thread agentConfig through applyPassthroughMCP (drop redundant registry lookup) - Document that Codex env/headers land in process args (ps-visible), unavoidable with -c - Add port-absent tests for freshPassthroughCommand and resumePassthroughCommand
- Guard the Cursor workspace write against a symlinked parent escaping the worktree (mirrors the copyfiles containment check) - codexKeyName now JSON-encodes non-bare names, escaping control chars as well as dots/quotes for valid TOML keys - ClaudeStrategy emits nothing when all servers are filtered out (no empty --mcp-config), matching CodexStrategy - Escape the pipe in the ADR 0011 table cell
…egies - Cursor and OpenCode strategies now also return no artifacts when every server is filtered out (matching Claude), so no empty mcp.json / OPENCODE_CONFIG is emitted - Drop the now-unreachable len(servers)==0 guard in applyPassthroughMCP (the kandev server is always present); strategies guard their own empty case - Add TestStrategies_NoArtifactsWhenAllServersFiltered
…ink write Two security issues introduced by passthrough MCP injection: - Codex serializes MCP env/http_headers into -c argv, which the passthrough start/resume paths logged via cmd.Args(), writing tokens into backend logs. Redact mcp_servers.*.(env|http_headers) values at every log site (the executed command is unchanged). - The Cursor workspace write guarded symlinked parents but probed existence with os.Stat (follows symlinks), so a committed dangling .cursor/mcp.json symlink would be written through to an outside path. Probe with os.Lstat and write with O_CREATE|O_EXCL so a leaf symlink (even racing in) is never followed. Regression tests for both.
main landed ADR 0011 (transient provider-error retry); the passthrough MCP ADR is renumbered to 0012. File rename and INDEX were resolved during the rebase; this fixes the lingering reference in add-agent-cli.md.
…n orphan config - The interactive runner logged req.Command verbatim on every passthrough start/restart/resume, re-leaking the Codex MCP -c env/header tokens that the manager log sites redacted. Add a LogCommand field (redacted) and log it. - writeFileNoFollow now removes the empty/partial file on write or close failure so a later SkipIfExists probe doesn't skip writing the real config.
carlosflorencio
force-pushed
the
feature/mcp-servers-cli-agen-s1q
branch
from
3317519 to
1537528
Compare
|
Claude finished @carlosflorencio's task in 2m 15s —— View job FindingsAll previous-round findings have been fully resolved across commits
No new issues found. Summary
Verdict: Ready to merge — the strategy abstraction is clean, all four CLI-specific implementations are well-tested (including edge cases: all-servers-filtered, TOML-quoted names, dangling-symlink leaf, workspace-path-escaping via symlink, port-absent error propagation across all three command builders), the metadata dual-shape tolerance for post-restart rehydration is thorough, and all blocker and suggestion findings from five prior review passes have been addressed. |
1 participant
CLI passthrough mode only ever wired kandev's own HTTP tools server into MCP config (and only for Claude) — codex, cursor, and opencode passthrough sessions got no MCP servers, and the servers configured on an agent profile's settings page were never applied. This adds a per-agent strategy that injects the full resolved server list into each CLI's native config shape so passthrough reaches parity with ACP mode, without ever writing to the user's global CLI config.
Important Changes
PassthroughMCPStrategy(internal/agent/mcpconfig/passthrough.go): each passthrough agent declares how its CLI loads MCP servers. Claude → temp file +--mcp-config(additive); Codex → repeated-c mcp_servers.<name>.<key>=<json>overrides (noCODEX_HOME, so auth is untouched); Cursor → project-local.cursor/mcp.json, created only if absent; OpenCode → temp file +OPENCODE_CONFIGenv var.resolveMcpServersWithParams), prepends kandev's HTTP server, and drops a profile server namedkandevso it can't shadow the real one. Written files are tracked (unioned across relaunch/resume/restart) and cleaned up; a user's pre-existing Cursor file is never overwritten or deleted.--mcp-configcan't swallow a positional prompt. Removes the oldMCPConfigFlag/MCPConfigPathmechanism in favor ofMCPStrategy/MCPArgs.Validation
go build ./...,go vet ./internal/agent/...— cleango test ./internal/agent/...— 1171 passed (sole failure is the pre-existing, unrelatedTestConnectWorkspaceStream_BackoffDrainsOnStop, confirmed failing on the clean tree)golangci-lint runon mcpconfig/agents/lifecycle — 0 issuesgofmtcleanPossible Improvements
Codex currently loads
url-based MCP servers natively; if a pinned Codex build ever regresses to requiringexperimental_use_rmcp_client, the kandev tools server would silently not load (documented at the call site). Cursor's skip-if-exists means kandev tools are absent for Cursor when a user.cursor/mcp.jsonalready exists.Checklist
Preview Environment
1537528