feat(mcp-client): scaffold built-in MCP client contribution#25
Conversation
This PR reserves the workbench slot at 'src/vs/workbench/contrib/mcpClient/' and lays out the design in 'docs/zeus-mcp-client.md'. When the implementation lands it will: - read '.zeus/mcp.json' from the workspace root - spawn stdio servers and open SSE connections - aggregate tool definitions for the agent runtime - reload on config change without restarting unaffected servers Depends on 'feat/zeus-conventions' (#23) for the '.zeus/mcp.json' schema. Implementation depends on '@modelcontextprotocol/sdk', which lands with the agent SDK PR.
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 59 minutes and 59 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the design documentation and initial scaffolding for a built-in Model Context Protocol (MCP) client. The changes include a comprehensive design document outlining the architecture—comprising a configuration loader, client registry, and tool aggregator—and a placeholder README in the source directory. A review comment correctly identified an incorrect relative path in the README's link to the design document and provided a specific code suggestion to fix it.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the design documentation and directory scaffold for a built-in Model Context Protocol (MCP) client. The documentation outlines the architecture for managing MCP servers via a workspace configuration file. Feedback on the design highlights several critical areas for the implementation phase: addressing Remote Code Execution (RCE) risks through a Workspace Trust mechanism, ensuring secure storage for authentication credentials, and implementing a namespacing strategy to prevent tool name collisions. Additionally, the reviewer pointed out that the proposed directory structure needs adjustment to comply with VS Code's architecture, as subprocess management for stdio servers cannot be performed directly from the browser layer.
…, layering
Address Gemini review:
- Trust model section: .zeus/mcp.json lists commands to execute, which
is an RCE vector if someone commits a malicious entry and a teammate
pulls + opens the workspace. Spell out: first-encounter trust prompt
per server, inherit Workspace Trust, refuse shell-form commands.
- Secret storage: ${env:NAME} and ${secret:keychain:NAME} references
only; warn on plain values for *_TOKEN/*_KEY/*_SECRET/PASSWORD field
names; loader is read-only against mcp.json.
- Tool name collision across servers: namespace as
'<server-name>__<tool-name>' in the aggregated registry; UI shows
the short name with server as secondary text.
- Directory layout: split common/ (platform-agnostic), browser/ (UI,
workspace registration), node/ (stdio child processes — cannot live
in browser/ per vscode's sandbox).
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the design and scaffolding for a built-in Model Context Protocol (MCP) client in Zeus. It includes a comprehensive design document detailing the architecture, security mitigations for remote code execution (RCE), and secret storage, alongside a placeholder directory for the upcoming implementation. Feedback focuses on enhancing the security model by extending trust prompts to modified configurations and SSE connections, strictly enforcing executable-only commands, and namespacing internal tools to prevent naming collisions with third-party servers.
…e shell wrappers, zeus_ prefix
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the design documentation and directory scaffolding for a built-in Model Context Protocol (MCP) client. The documentation outlines the architecture across VS Code's layers, security mitigations for remote code execution, and secret storage strategies. Review feedback focused on strengthening the security model, such as refining the trust fingerprinting process, expanding restricted shell command lists, and enforcing stricter secret handling. Additionally, improvements were suggested for tool namespacing to ensure AI agent stability and prevent naming collisions, along with a minor branding correction in the README.
…fuse plain secrets, zeus__ prefix, always-namespace, VS Code spelling
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the design documentation and directory scaffolding for a built-in Model Context Protocol (MCP) client. The documentation outlines the architecture, a trust model to mitigate RCE risks, and a strategy for secure secret storage. Feedback identifies a contradiction in the cross-platform hashing logic for the trust prompt, suggests expanding the heuristic list for detecting sensitive environment variables, and highlights a potential naming collision in the tool namespacing logic if a third-party server is named 'zeus'.
|
|
||
| Mitigations: | ||
|
|
||
| - **Trust prompt** — the first time a workspace is opened with a non-empty `mcp.json`, *or* whenever a server entry is added **or modified** (any change to `command`, `args`, `env`, `url`, or `transport`), Zeus blocks startup of those servers and shows a per-server confirmation pane (similar to VS Code's "Restricted Mode" workspace trust). The fingerprint stored in per-user (not in-git) state is `sha256(canonical_json(serverConfig))`, where `canonical_json` (a) sorts object keys lexicographically at every depth, (b) resolves relative `command:` paths against the workspace root to absolute, (c) drops trailing whitespace in env values, and (d) uses LF line endings. Equal-meaning configurations therefore hash identically across platforms and JSON serializers, and any tweak invalidates the prior consent. |
There was a problem hiding this comment.
There is a contradiction in the trust model design. The document states that relative command: paths are resolved to absolute paths before hashing, but also claims that configurations will hash identically across platforms. Absolute paths are typically machine-specific (e.g., containing the user's home directory), which would cause the hash to differ between teammates even for the same .zeus/mcp.json file. To achieve cross-platform consistency, consider hashing the relative path (with normalized separators) or using a placeholder like ${workspaceFolder} during the canonicalization process.
There was a problem hiding this comment.
Genuine contradiction, fixed in b4b6414. The fingerprint is intentionally per-user / per-machine, not cross-platform. If command: 'node' resolves to /usr/local/bin/node on one box and /opt/homebrew/bin/node on another, those are different binaries and re-consent is the correct outcome. Trust travels with the user, never with the repo. Within a single machine, equal-meaning configs (key order, whitespace, line endings) still hash identically.
|
|
||
| - `${env:NAME}` resolves at spawn time from the user's environment | ||
| - `${secret:keychain:NAME}` reads from `vscode.SecretStorage` (per-user, OS-keychain backed) | ||
| - Plain string values are accepted only for non-secret config. The loader **refuses to start** a server (rather than just warning) when a field name matches the heuristic list (`*_TOKEN`, `*_KEY`, `*_SECRET`, `PASSWORD`) and the value is not a `${env:...}` / `${secret:...}` reference. The user sees a per-server error in the status bar with a "Move to keychain" quick-fix that creates the secret and rewrites the reference for them. |
There was a problem hiding this comment.
The heuristic list for secret detection (*_TOKEN, *_KEY, *_SECRET, PASSWORD) is somewhat narrow. Many services use variations like API_KEY, ACCESS_TOKEN, PRIVATE_KEY, or AUTH_TOKEN. Consider expanding this list or using a more comprehensive set of common secret naming patterns to improve the effectiveness of the security check and prevent accidental commits of sensitive data.
There was a problem hiding this comment.
The name-based heuristic (*_TOKEN / *_KEY / *_SECRET / PASSWORD) is intentionally narrow because it must be high-precision to avoid false positives. Variants like GITHUB_PAT / PRIVATE_PEM / GH_TOKEN are caught by the high-entropy fallback on every plain env value (≥32 chars, base64/hex-ish) — see the second bullet of 'Secret storage'. That's the layered approach: precise name match catches obvious cases with a clear error message, entropy heuristic catches the rest.
| - [ ] Zeus's own MCP **server** half publishes its tools under the `zeus__` prefix (double underscore, matching the third-party namespacing rule below — e.g. `zeus__buffer_read`, `zeus__editor_open`). The double-underscore separator means a third-party server *literally* named `zeus` does not collide with our internal surface | ||
| - [ ] **All** third-party tools are always exposed as `<server-name>__<tool-name>` in the aggregated registry, regardless of whether another server has registered the same short name. Always-namespacing (rather than namespacing-on-collision) means the tool name the agent sees is stable: it does not change when a new server is added later. UI surfaces still show the short tool name with the server name as secondary text. The underlying call dispatches to the originally-named tool on the right server. |
There was a problem hiding this comment.
The namespacing logic described here does not actually prevent collisions for a third-party server named zeus. If internal tools use the zeus__ prefix and third-party tools are mapped to <server-name>__<tool-name>, then a third-party server named zeus will produce identical identifiers (e.g., zeus__my_tool). To ensure stability and avoid collisions, the system should either reserve the zeus server name or use a unique, non-replicable prefix for internal tools.
There was a problem hiding this comment.
Correct, this was a real hole — the double-underscore separator alone doesn't prevent it. Closed in b4b6414: the loader now reserves zeus as a server name and refuses any third-party entry whose lowercase name is zeus, with a clear error pointing at the offending entry. That makes the zeus__ namespace genuinely safe from third-party collision.
…per-machine Two issues raised by reviewer: 1. Reserve 'zeus' as a server name in .zeus/mcp.json. The original acceptance criterion claimed the zeus__ prefix on internal tools prevented collision with a third-party server literally named zeus, but the always-namespace rule (<server>__<tool>) would still map such a server's tools into the same zeus__<tool> namespace. Refuse the configuration outright instead. 2. The trust-prompt fingerprint says it resolves relative command: paths to absolute, which is intentional (we want re-consent if a different binary would be launched), but the doc also claimed configs would hash identically across platforms. Those two are inconsistent — the fingerprint is per-user / per-machine on purpose; cross-machine identity was never the goal. Clarified.
1 participant
Goal
Reserve the workbench contribution slot for the built-in MCP client and document the design. The client reads
.zeus/mcp.jsonand exposes the aggregated tool list to Zeus's agent runtime.Design doc:
docs/zeus-mcp-client.mdWhat this PR ships
src/vs/workbench/contrib/mcpClient/README.md(slot marker)docs/zeus-mcp-client.md(design doc)Why built-in, not a VS Code extension
We want the MCP-first stance to be honest. Going through
@modelcontextprotocol/sdkdirectly means the same.zeus/mcp.jsonworks in Claude Code CLI, ChatGPT desktop / Codex, and any future MCP client. Tying MCP server lifecycles to a user-disable-able extension would weaken that.Dependencies
feat/zeus-conventions—.zeus/mcp.jsonschema@modelcontextprotocol/sdkPart of the MCP-first pivot. See
zeus-internal/ARCHITECTURE.md.