feat(mcp): S1 — plan + the pure config/naming/policy core - #29
Merged
Conversation
Starts MCP support (docs/MCP.md). S1 is deliberately inert: a pure module plus its tests, wired to nothing, so the constraint most likely to bite is settled before any process is spawned. docs/MCP.md records the scope and five decisions, the load-bearing ones being: - HAND-ROLL the client rather than take @modelcontextprotocol/sdk. Three verified blockers: extensions are zero-dependency by policy (CLAUDE.md:161, "plain JS, no build step … Keep it that way"), `npm install` never runs for this extension (vscode/build/npm/dirs.ts is a hardcoded allow-list with zero levelcode entries), and the SDK is ESM while the extension is CJS. The needed surface is three JSON-RPC methods. This reverses my earlier SDK recommendation. - stdio first. The 2026-07-28 spec revision landing now is almost entirely a Streamable-HTTP concern (stateless core, routable headers, auth), so stdio sidesteps it. Current stable is 2025-11-25. mcpConfig.js does three things, all before anything is spawned or called: 1. Server config: merges the user setting with per-workspace .levelcode/mcp.json, KEEPING provenance — the two are not equally trusted. A workspace file is repo-authored (attacker-controlled for any repo you clone) and names a process to spawn, so the user's setting WINS on a name collision: a repo can never shadow a server the user already trusts. 2. Tool naming: `server__tool`, sanitized, capped at 64 (OpenAI's limit, stricter than Anthropic's 128), never shadowing a built-in. This is a correctness gate — nothing else validates names, and an illegal one fails the first turn with an opaque 400 and then poisons every later turn, since it is re-serialized from the transcript. 3. Policy: default ASK. Only the user's allow-list grants 'allow'; autopilot does not relax MCP. Server-supplied annotations are untrusted and may only tighten — a destructiveHint overrides an allow-list entry, a readOnlyHint grants nothing. The tests earned their keep immediately: the naming corpus caught a real collision bug — the truncation hash sliced base-36 from the LEFT, discarding exactly the low-order digits that vary, so two distinct long tool names collapsed onto one name. Fixed to take the last 6 digits; mutation-checked (restoring the bug fails the suite, exit 1). Verified: 21 tests pass; module requires only `path` (no vscode/fs/child_process); the full CI-style gate runs 16 extension suites with 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces the first (inert) slice of MCP support for extensions/levelcode-ai: a pure, dependency-free core module that handles MCP server config merging (with provenance), tool-name normalization/uniquing, and per-call approval policy—plus unit tests and a design/plan document.
Changes:
- Add
mcpConfig.jspure core for server config provenance, safe tool naming, and approval classification. - Add unit tests covering naming invariants, collision handling, config trust/merge rules, and policy defaults.
- Add
docs/MCP.mdoutlining scope, security model, and phased implementation plan.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| extensions/levelcode-ai/mcpConfig.js | Adds pure MCP config merge + tool naming + approval policy logic (no spawning/wiring yet). |
| extensions/levelcode-ai/test/mcpConfig.test.js | Adds corpus-based tests for naming correctness, trust/merge precedence, and policy defaults. |
| docs/MCP.md | Documents MCP plan, security model, and slice roadmap for incremental rollout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…the doc Addresses the PR #29 review (Copilot). All three were real. 1. namespaceToolName's hash separator was a LITERAL 0x00 byte in the source, not the two-character escape. Node parses it, but it breaks grep, patching and editors — and it had already bitten me: I saw `grep` report "Binary file mcpConfig.js matches" and wrongly blamed the em-dashes in the banner. Replaced with the escape form. The runtime value is identical, so no tool name changes. 2. loadServerConfig wrapped a missing setting as {mcpServers: undefined}, which fell through serverMapOf's bare-map branch and reported a phantom server named "mcpServers" — i.e. every user with NO MCP config got an error problem. A missing setting is now an empty map, and a settings object may itself use the {mcpServers:…} wrapper. The existing test missed this because it asserted only servers.length, never problems. Added a regression test that asserts BOTH across undefined/{}/null/ empty settings, plus one for the wrapper form. 3. docs/MCP.md contradicted the shipped policy: it said annotations "may auto-allow read-only ones" and, in the same sentence, that they can only relax toward asking. classifyMcpTool never auto-allows. Rewritten to state what the code does — only the user's allow-list grants allow; destructiveHint forces the prompt even for an allow-listed tool; readOnlyHint grants nothing. Verified: 23 tests pass (21 + 2 regressions); zero NUL bytes in the MCP files; namespacing output is byte-identical to before; the full CI gate runs 18 suites with 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
+66
to
+77
| if (raw.env != null && (typeof raw.env !== 'object' || Array.isArray(raw.env) | ||
| || Object.values(raw.env).some((v) => typeof v !== 'string'))) { | ||
| return '"env" must be an object of strings'; | ||
| } | ||
| return { | ||
| name: name, | ||
| command: raw.command, | ||
| args: raw.args ? raw.args.slice() : [], | ||
| env: raw.env ? Object.assign({}, raw.env) : {}, | ||
| source: source, // 'settings' (user-authored) | 'workspace' (repo-authored, untrusted) | ||
| origin: origin // human label for the consent card / problem messages | ||
| }; |
Contributor
Author
There was a problem hiding this comment.
Fixed in 206113d — and you are right that it should not depend on luck.
I measured it before changing anything: it is not currently exploitable. An object-valued __proto__ is rejected upstream by the existing string-value check (servers=0, "env" must be an object of strings), and a string-valued one is swallowed by the prototype setter rather than assigned. But that makes the safety incidental — it holds only for as long as nobody relaxes that value check, which is exactly the kind of coupling that breaks silently.
Now structural: safeEnvCopy() skips __proto__/constructor/prototype and copies into a fresh object. Kept a normal {} rather than Object.create(null) — later code may reasonably call hasOwnProperty on this. Side benefit: an env var legitimately named __proto__ no longer vanishes into the setter.
Mutation-checked: restoring Object.assign fails the suite (exit 1). It is caught via the constructor key — worth noting, since a test that probed only __proto__ would have passed against the unsafe version and proved nothing.
…ally collide Second round of PR #29 review (Copilot). Both were real. 1. normalizeServer copied a repo-authored env with Object.assign, which hands an own "__proto__" key (JSON.parse creates one) to the prototype SETTER instead of copying it. Measured before changing anything: this is NOT currently exploitable — the object-valued payload is rejected by the existing "env must be an object of strings" check, and a string-valued __proto__ is ignored by the setter. But that makes the safety INCIDENTAL: it holds only as long as nobody relaxes the value check. Now structural — safeEnvCopy skips __proto__/constructor/prototype and copies into a fresh object. (Kept a normal object rather than Object.create(null): later code may reasonably call hasOwnProperty on it.) It also stops an env var named __proto__ from silently vanishing into the setter. 2. The "servers that sanitize alike" test paired 'my-server' with 'my/server', but '-' is already legal so they never collided — the test passed without ever entering the dedupe path, i.e. it proved nothing. Now pairs 'my/server' with 'my:server' (both sanitize to 'my_server'), asserts the collision as an explicit PRECONDITION so it cannot rot back, and asserts the dedupe reported it. Added a prototype-pollution regression test alongside. Verified: 24 tests pass; both fixes mutation-checked — restoring Object.assign fails the suite (exit 1, caught via the `constructor` key), and restoring the non-colliding inputs also fails (exit 1, caught by the new precondition). Full CI gate: 18 suites, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
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.
Starts MCP support. S1 is deliberately inert — a pure module plus its tests, wired to nothing — so the constraint most likely to bite is settled before any process is spawned. Full scope in
docs/MCP.md.Two decisions worth reviewing
Hand-roll the client; don't take
@modelcontextprotocol/sdk. This reverses my initial recommendation, on three verified blockers:CLAUDE.md:161: "plain JS, no build step … Keep it that way." (package.jsonconfirms: no deps, no scripts.)npm installnever runs for this extension —vscode/build/npm/dirs.tsis a hardcoded allow-list with zero levelcode entries.The surface we need is three JSON-RPC methods (
initialize,tools/list,tools/call), andproviders/sse.js/skills.jsalready set the hand-rolled precedent.stdio first. The
2026-07-28spec revision landing right now is almost entirely a Streamable-HTTP concern (stateless core, routable headers, auth hardening) — stdio barely moves, so this sidesteps the churn. Current stable is2025-11-25.What
mcpConfig.jsdoes.levelcode/mcp.json. These are not equally trusted: a workspace file is repo-authored — attacker-controlled for any repo you clone — and it names a process to spawn. So the user's setting wins on a name collision; a repo can never shadow a server the user already trusts.server__tool, sanitized, capped at 64 (OpenAI's limit, stricter than Anthropic's 128), never shadowing a built-in. Nothing else in the pipeline validates names (translate.jsrenames verbatim), and an illegal one fails the first turn with an opaque 400 — then poisons every later turn, since the name is re-serialized from the transcript.allow; autopilot does not relax MCP. Server annotations are untrusted and may only tighten:destructiveHintoverrides an allow-list entry,readOnlyHintgrants nothing.The tests earned their keep immediately
The naming corpus caught a real collision bug: the truncation hash sliced base-36 from the left, discarding exactly the low-order digits that vary — so two distinct long tool names collapsed onto one name (silent tool aliasing). Fixed to take the last 6 digits, and mutation-checked: restoring the bug fails the suite (exit 1).
Verification
commandSafety.test.jsspirit (NAMING must never escape^[A-Za-z0-9_-]{1,64}$; TRUST must never let a repo shadow a user server or default toallow).path— novscode,fs, orchild_process.Next
S2 (stdio JSON-RPC client + lifecycle/reaping), then S3 (the one router line at
agent.js:442), then S4 (trust + approval UX — the slice that must not be skipped).🤖 Generated with Claude Code