feat(mcp): wire MCP tools into the agent loop (S3) by ndemianc · Pull Request #31 · levelcodeai/levelcode

ndemianc · 2026-07-23T04:25:00Z

Turns S1 (#29) and S2 (#30) from inert modules into tools the model can actually call. Two commits: the orphan fix lands first and separately because it is independently verifiable.

What changed

mcpConfig.buildAgentTools() — the whole MCP→agent translation. MCP's {name, description, inputSchema} becomes our {name, description, input_schema}, plus a routes map (namespaced name → server / tool / annotations). Pure — takes plain data rather than live handles, so it is testable without spawning a process.
agent.js — TOOLS / TOOLS_TOKENS_EST become per-run, built right beside system / systemTokensEst, which already had exactly this shape. The doc called these "module constants" as though this were invasive; there are 5 reference sites in total. The router sits immediately before the unknown tool return — an MCP name matches none of the built-in branches, so every MCP call necessarily arrives at that one line.
extension.js — reads the two new settings and hands them in, like verify / commandTimeout, so agent.js keeps its distance from the editor API.
G4 hardening — tool descriptions are capped (they ride every turn and are the injection surface), and MCP arguments are redacted in the debug log: that line is posted into the chat when levelcode.ai.debug is on, and MCP args routinely carry tokens.
Reaping — S2 shipped reapMcp() but nothing called it; extension.js never required mcpClient. Latent until S3 connects something, then every New Chat and reload orphans a subprocess.

Trust — why this slice is deliberately small

Only source:'settings' servers are started. A .levelcode/mcp.json is repo-authored, i.e. attacker-controlled for any repo you clone, and a server entry names a process to spawn — starting those here would make S3 itself the RCE-on-clone hole that S4's launch gate exists to close. They are reported rather than silently skipped, so the gap reads as a missing feature, not a bug.

And because S3 ships no approval card (S4 owns it), a tool the user has not explicitly allow-listed is refused, not run — including under Autopilot. The refusal names the exact setting to change and tells the model not to retry, so it degrades into an explanation instead of a loop. The run-start chip shows 0/12 allow-listed up front, so a later refusal is legible rather than mysterious.

Verification

35 unit tests (11 new); full gate 18 suites, 0 failures.

Four mutations, each caught: unsafe Object.assign in schemaOf, dropping the object-type normalization, removing the description cap, and correlating specs by index rather than by (server, tool) key.

That last mutation initially did not fail. With a single server, all cap-drops land at the tail, so indices coincidentally line up — the test passed against the broken code while proving nothing. That is the same defect flagged in the #29 review, reproduced in my own test. Rewritten with an over-cap server followed by a second server, which is what makes a mis-correlation observable; it now fails as it should.

End-to-end against a real MCP server (the S2 fixture): connect → tools/list → buildAgentTools → legal names + object schemas → refused with no policy → allow-listed → real call returns echo: hello from S3 → a failing tool returns ERROR: it broke rather than throwing → reaped, process confirmed dead past the SIGKILL grace.

Not covered by CI: the ~20 lines of router glue inside runTool. agent.js requires vscode, so it cannot be required from a plain node test; that glue is covered by reading plus the integration run of every function it calls.

One more thing

I reintroduced a raw NUL byte into mcpConfig.js while writing this. The separator is deliberately NUL (a space would let server a b + tool c collide with server a + tool b c), but the byte was written raw instead of escaped — which makes file report "data" and makes grep and diff skip the file silently. Same defect the #29 review caught. No test, type-check, or lint notices it, so there is now a byte-level guard over all four MCP sources. It fired immediately on the test file, which had one too.

Try it

Set levelcode.ai.mcp.servers to {"filesystem": {"command": "npx", "args": ["-y", "@modelcontextprotocol/server-filesystem", "/tmp"]}}, start a run, and the chip reads 🔌 mcp · filesystem (11) · 0/11 allow-listed. Add {"filesystem__read_text_file": "allow"} to levelcode.ai.mcp.toolPolicy to let that one through.

Next: S4 — the trust-on-first-use launch gate and the kind:'mcp' approval card, which is what makes workspace servers usable and removes the allow-list-only restriction.

🤖 Generated with Claude Code

S2 shipped reapMcp() but nothing ever called it — extension.js did not require mcpClient at all, and both newChat() and deactivate() reaped only commands. An MCP server is a detached child holding ports and handles exactly like a bgRun, so the moment S3 starts connecting them, every New Chat and every window reload would orphan one. Latent rather than broken today (nothing connects yet), which is precisely why it is worth landing before S3 rather than after: it is three lines and it is independently verifiable. Verified against the S2 fixture server, not just by inspection: connect → pid alive → reapMcp() → registry empty → process confirmed dead after the 1500ms SIGTERM→SIGKILL grace (checking before the grace elapses shows it mid- termination and reads as a false orphan). Also checked deactivate()'s path never throws with zero servers connected — two consecutive reaps on an empty registry are a clean no-op. Full gate: 18 suites, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Turns S1+S2 from inert modules into tools the model can actually call. WHAT CHANGED - mcpConfig.buildAgentTools() — the whole MCP→agent translation: MCP's {name, description, inputSchema} becomes our {name, description, input_schema} plus a routes map (namespaced name → server/tool/annotations). Pure, takes plain data rather than live handles, so it is testable without spawning. - agent.js — TOOLS/TOOLS_TOKENS_EST become PER-RUN, built beside system/ systemTokensEst which already had exactly this shape. The router sits immediately before the `unknown tool` return: an MCP name matches none of the built-in branches, so every MCP call necessarily arrives at that one line. - extension.js — reads the two new settings and hands them in, like verify/ commandTimeout, so agent.js keeps its distance from the editor API. - G4 hardening: tool descriptions capped (they ride every turn and are the injection surface), and MCP arguments are REDACTED in the debug log — that line is posted into the chat, and MCP args routinely carry tokens. TRUST (the reason this slice is deliberately small) Only source:'settings' servers are started. A .levelcode/mcp.json is repo- authored — attacker-controlled for any repo you clone — and a server entry names a process to spawn, so starting those here would make S3 itself the RCE-on-clone hole that S4's launch gate exists to close. They are reported, not silently skipped, so the gap reads as a missing feature rather than a bug. And because S3 ships no approval CARD (S4 owns it), a tool the user has not explicitly allow-listed is REFUSED, not run — including under Autopilot. The refusal names the exact setting to change, and tells the model not to retry it, so it degrades into an explanation instead of a loop. The run-start chip shows "0/12 allow-listed" up front so a later refusal is legible. VERIFIED - 35 unit tests (11 new). Four mutations, each caught: unsafe Object.assign in schemaOf, dropping the object-type normalization, removing the description cap, and correlating specs by index instead of by (server, tool) key. - That last one initially did NOT fail against the broken code: with a single server all drops land at the tail so indices coincidentally line up, and the test passed while proving nothing — the same defect flagged in the PR #29 review, in my own test. Rewritten with an over-cap server FOLLOWED by a second server, which is what makes a mis-correlation observable; it now fails. - End-to-end against a real MCP server (the S2 fixture): connect → tools/list → buildAgentTools → legal names + object schemas → refused with no policy → allow-listed → real call returns "echo: hello from S3" → a failing tool comes back as `ERROR: it broke` rather than throwing → reaped, process confirmed dead. The ~20 lines of router glue inside runTool are NOT unit-tested (agent.js requires `vscode`); they are covered by reading plus this integration run of every function they call. - Full gate: 18 suites, 0 failures. Also: I reintroduced a raw NUL byte into mcpConfig.js while writing this (the separator is deliberately NUL, but the byte was written raw instead of escaped), which makes `file` report "data" and makes grep and diff skip the file in silence. Same defect the PR #29 review caught. Nothing in the toolchain notices, so there is now a byte-level guard test covering all four mcp sources — it fired on this very test file, which had one too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

This PR wires the MCP (Model Context Protocol) implementation (from prior S1/S2 slices) into the agent run loop so that MCP tool specs can be surfaced as callable agent tools and routed back to the correct MCP server, with additional security hardening around prompt-injection surface area and logging.

Changes:

Adds mcpConfig.buildAgentTools() to translate MCP {name, description, inputSchema} tool specs into agent tool descriptors plus a routing table.
Integrates MCP setup + routing into agent.js (per-run tool list, router inside runTool, and redacted debug logging for MCP args).
Introduces new user settings (levelcode.ai.mcp.servers, levelcode.ai.mcp.toolPolicy) and ensures MCP subprocesses are reaped on New Chat / deactivate.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
extensions/levelcode-ai/agent.js	Builds per-run tool list including MCP tools; routes MCP tool calls; redacts MCP args in debug logs; connects and reports MCP server/tool availability.
extensions/levelcode-ai/extension.js	Passes MCP settings into the agent context and reaps MCP subprocesses on session resets/unload.
extensions/levelcode-ai/mcpConfig.js	Implements MCP→agent tool translation (`buildAgentTools`), schema normalization, safe copying, and description capping.
extensions/levelcode-ai/package.json	Adds settings schema/docs for MCP servers and MCP tool allow/ask policy.
extensions/levelcode-ai/test/mcpConfig.test.js	Adds unit tests for MCP→agent translation, schema/description hardening, and source hygiene (raw control-byte guard).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ive tools PR #31 review (Copilot). Both findings had one root: a destructiveHint tool can never be enabled by the allow-list (classifyMcpTool forces 'ask' regardless), yet two callers spoke as if it could. 1. The startup chip counted allow-listed tools by calling classifyMcpTool WITHOUT annotations, while runTool calls it WITH them. So a destructive-but-allow-listed tool was counted as "allow-listed" in the chip and then refused at call time — the chip lied. Now threads the same route annotations into the count. 2. The refusal message unconditionally told the model to add the tool to levelcode.ai.mcp.toolPolicy as "allow". For a destructive tool that is impossible, so the model would allow-list it, retry, and be refused again. Root-cause fix rather than two patches: classifyMcpTool now also returns policyCanAllow (false only for the destructive tighten-path), and the refusal message moves OUT of agent.js into explainMcpRefusal() beside the classifier — the message that drifted from the policy now branches solely on the verdict, in the same module, and is unit-testable off the editor (agent.js requires vscode). agent.js just calls it. Verified: - 37 tests (2 new + policyCanAllow assertions on the existing POLICY cases). - Both fixes mutation-checked: a destructive verdict claiming policyCanAllow:true fails; a message that ignores the discriminator (the original bug) fails. - End-to-end against the S2 fixture with 'boom' marked destructive: the exact chip-count logic returns 0/3 for an allow-listed destructive tool, the message omits any toolPolicy instruction, and a plain allow-listed tool still counts 1 and would run. The chip-count line itself lives in agent.js and so is covered by that integration run + reading, not CI — same as the router glue. - Full gate: 18 suites, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

extensions/levelcode-ai/package.json:443

Same as the servers setting: the MCP tool allow-list should not be configurable by workspace/folder settings, otherwise a repo can commit a permissive policy (e.g. "*": "allow") and silently weaken execution gating. Mark this setting as application-scoped.

        "levelcode.ai.mcp.toolPolicy": {
          "type": "object",
          "default": {},
          "markdownDescription": "Which MCP tools may run, as `{ \"server__tool\": \"allow\" | \"ask\" }` — use `\"*\": \"allow\"` for all of them. Tool names are namespaced `server__tool` exactly as they appear in the chat.\n\nAnything not allow-listed is **refused**, including under Autopilot: an MCP tool is third-party code, so Autopilot deliberately does not relax this. A server's own `destructiveHint` overrides an `allow` here — server hints may only ever tighten, never loosen."
        },

…ettings RCE PR #31 review (Copilot), and it is the serious one: RCE on clone-and-open. The trust model is "user-authored = trusted, repo-authored = untrusted." I had it HALF right — .levelcode/mcp.json is gated (source:'workspace', never started) — but I missed that VS Code SETTINGS themselves have a repo-authored tier. A committed .vscode/settings.json (or a folder entry in a .code-workspace) can set levelcode.ai.mcp.servers, and cfg.get() returns that merged effective value. S3 treats everything from the settings read as source:'settings' and auto-starts it. So a hostile repo could ship .vscode/settings.json naming `sh -c "curl … | sh"` as an MCP server and have it spawn the moment the folder is opened — exactly the RCE the whole model exists to prevent. Fixed two ways, deliberately redundant for a spawn-on-open surface: 1. package.json — both settings are now "scope": "application", so VS Code drops any workspace/folder value and greys them out in the workspace settings UI. This is the idiomatic mechanism and covers the reviewer's package.json comment (both lines). 2. extension.js — reads them via cfg.inspect() and takes ONLY the global (user) tier, never workspaceValue/workspaceFolderValue. The spawn decision is too dangerous to rest on a declarative manifest guard alone; this enforces the same boundary in code, at the point of use, so it survives a future scope regression. The trust logic is a pure helper (userScopedSetting) in mcpConfig so it is unit-testable off the editor — the manifest scope is not. Verified: - 38 tests (1 new). It simulates a repo injecting a server via the workspaceValue tier and asserts it is NOT honoured, that a real user globalValue IS, and that we read one tier rather than merging. - Mutation-checked: making the helper fall back to workspaceValue (the bug) fails the suite. - Confirmed the ONLY VS Code read of these keys is the user-scoped inspect() call; agent.js reads the already-scoped value handed to it. The extension.js glue (inspect() → userScopedSetting) requires vscode, so it is covered by that unit test of the helper plus reading, not CI. - Full gate: 18 suites, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

describeTool capped the server's own description but returned the no-description fallback uncapped. That fallback embeds `tool` — the server-chosen, untrusted tool name — so a server could ship a giant name with a blank description and sail past MAX_TOOL_DESC, re-bloating every turn's prompt and widening the G4 injection surface the cap exists to bound. Fix applies the cap to the final string, so the server's text and the fallback obey the same bound through one exit — which also drops the duplicated slice logic. (The namespaced name is separately truncated to 64; untouched.) Verified: 39 tests (1 new — a 6000-char tool name with no description must still yield a description within MAX_TOOL_DESC). Mutation-checked: restoring the uncapped-fallback branch fails the new test. Full gate: 18 suites, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

…t rescued PR #31 review (Copilot), documentation accuracy. The safeCopy JSDoc said it "stops a key legitimately named __proto__ from silently vanishing into a setter," which reads as if such a key is preserved. It is not: the loop `continue`s on __proto__/constructor/prototype, dropping them outright. The behavior is correct (dropping is the safe choice, and such a name is meaningless as an env var and unused as a top-level JSON-Schema keyword); only the comment mis-described it. Reworded to say plainly that those keys are dropped, never copied, and that this is a drop rather than a rescue — with the one-line rationale for why losing them costs nothing. Also strengthened the existing TRUST test to match the now-accurate contract: it asserts up front that the input genuinely carries own __proto__/constructor keys (so the drop path is provably exercised, not vacuous) and that they do not appear in the output. Mutation-checked: removing the `continue` (i.e. copying the keys through) fails the test. Full gate: 18 suites, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

…ools PR #31 review (Copilot). The chip built its per-server counts from handles[i].tools.length — the raw tools/list payload — while the run actually exposes built.tools, which is smaller after the per-server cap (MAX_TOOLS_PER_SERVER) and after junk specs are skipped. So a server listing 100 tools rendered "server (100)" even though only 64 were callable, and worse, that per-server number sat right next to an "allowed/total" denominator computed from built.tools.length — the chip contradicted itself. Per-server counts now derive from built.routes (the tools actually exposed) via a new pure helper, toolCountsByServer, so they reflect what the agent can call and SUM to the denominator. A server that exposed nothing shows "(0)" — honest, and a useful signal that it connected but contributed no usable tools. Verified: 41 tests (2 new). The key test reproduces the reviewer's scenario — a server 5 over the cap plus a server listing junk — and asserts the counts are the capped/filtered numbers AND that they sum to built.tools.length. Mutation-checked: a wrong per-tool increment fails, and dropping the malformed-entry guard fails. The agent.js consumption is one line reading the tested helper, covered by that test + reading (agent.js requires vscode). Full gate: 18 suites, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

ndemianc and others added 2 commits July 23, 2026 00:11

Copilot AI review requested due to automatic review settings July 23, 2026 04:25

Copilot started reviewing on behalf of ndemianc July 23, 2026 04:25 View session