feat(mcp): S6a — Manage MCP servers, and take back a G1 approval - #58
Open
ndemianc wants to merge 1 commit into
Open
feat(mcp): S6a — Manage MCP servers, and take back a G1 approval#58ndemianc wants to merge 1 commit into
ndemianc wants to merge 1 commit into
Conversation
Adds `levelcode.ai.manageMcp` on the pickModel QuickPick pattern, linked from the foot of `/mcp`. Rows come from the same `mcpOverview()` S5 already uses, so the two views cannot disagree. Three things it adds beyond looking: **Add / remove a server** without hand-writing JSON — the last place MCP forced people into a settings file. Writes the Global tier only, never Workspace: `mcp.servers` is `application`-scoped precisely so a repo cannot introduce a server that starts without consent (G1), and offering the workspace tier would quietly undo that. **Revoke G1 trust**, per server or workspace-wide. This is the missing half of trust-on-first-use — approving was write-once with no way back short of editing `workspaceState` by hand, which makes the consent prompt harder to say yes to than it should be. Revoking does not kill a running server, and the wording says so. **Names a stale approval.** `summarizeMcp` reports "never approved" and "approved, then the repo changed the command" both as `trusted:false`. That is right for starting the server and wrong for explaining it: the second case is exactly the G1 attack — get something benign approved, then swap the command. The row now reads `command changed — needs approval` and offers to forget the dead entry. `parseArgv()` in mcpConfig.js turns the typed argument line into argv. Not a whitespace split: `-y @modelcontextprotocol/server-filesystem "/Users/me/My Documents"` is close to the most common MCP server there is, and splitting it turns one argument into three. It is a splitter, not a shell — no expansion, no globbing — matching `shell:false` at the spawn site. The Add wizard has no `env` step on purpose. The servers that need one need an API token, and prompting for it would put a live credential in plaintext settings.json while looking like the recommended way to do it. `connect()` inherits `process.env`, so exporting the variable keeps the secret out of any file; the success notification offers "Open settings JSON" for anyone who wants to do it differently. docs/MCP.md §9 documents the GitHub MCP recipe, verified end-to-end through mcpClient today: 41 tools, all names legal under D4, a real PR read back, write access confirmed. Also records that no GitHub tool sets `destructiveHint`, so `"*": "allow"` would let autopilot merge a pull request. Tests: 9 new in test/mcpManage.test.js (extracted from extension.js the way ctxSegments does with chat.html) + 4 in mcpConfig.test.js. Each verified non-vacuous by bypassing the fix and confirming failure.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an in-editor “AI: Manage MCP Servers…” QuickPick to manage MCP server configuration and G1 launch-trust, plus supporting parsing, UI wiring, docs, and tests. This fits the extensions/levelcode-ai MCP feature set by making MCP setup/trust state actionable (not just observable via /mcp).
Changes:
- Add
levelcode.ai.manageMcpcommand + QuickPick UI for listing servers, adding/removing settings servers, and revoking workspace trust. - Introduce
parseArgv()for quote-aware argument splitting, and extend unit tests for both the new manage UI helpers and argv parsing. - Update
/mcpwebview output and docs to link/discover the new management UI and document GitHub MCP usage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| extensions/levelcode-ai/test/mcpManage.test.js | Adds tests for the new manage UI pure helpers extracted from extension.js. |
| extensions/levelcode-ai/test/mcpConfig.test.js | Adds tests validating parseArgv() behavior for quoted/escaped input. |
| extensions/levelcode-ai/package.json | Contributes the new levelcode.ai.manageMcp command to the Command Palette. |
| extensions/levelcode-ai/media/chat.html | Adds a “Manage servers…” link at the bottom of /mcp output that posts manageMcp to the extension. |
| extensions/levelcode-ai/mcpConfig.js | Adds parseArgv() and exports it for use by the add-server wizard. |
| extensions/levelcode-ai/extension.js | Implements the Manage MCP servers QuickPick flow + add/remove server + revoke trust actions, and wires the webview message + command registration. |
| docs/MCP.md | Documents S6a as done and adds a verified GitHub MCP server recipe section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+617
to
+621
| if (ch === '\\' && i + 1 < s.length && quote !== "'") { | ||
| // A backslash escapes the next char, except inside single quotes where it is literal — | ||
| // matching the shell convention users will expect from muscle memory. | ||
| cur += s[++i]; started = true; continue; | ||
| } |
Comment on lines
+1478
to
+1486
| validateInput: (v) => { | ||
| const t = String(v || '').trim(); | ||
| if (!t) { return 'A name is required.'; } | ||
| if (Object.prototype.hasOwnProperty.call(existing, t)) { return 'A server called "' + t + '" already exists.'; } | ||
| // Not a hard rule — namespaceToolName sanitizes anyway — but a name that survives verbatim | ||
| // makes the tool names in the transcript readable. | ||
| if (!/^[A-Za-z0-9_-]+$/.test(t)) { return 'Use letters, digits, dashes or underscores so the tool names stay readable.'; } | ||
| return null; | ||
| } |
Comment on lines
+28
to
+34
| function extract(name) { | ||
| const start = src.indexOf('function ' + name + '('); | ||
| assert.ok(start >= 0, 'extension.js no longer defines ' + name + '()'); | ||
| const end = src.indexOf('\n}', start); | ||
| assert.ok(end >= 0, 'no closing brace found for ' + name + '()'); | ||
| return src.slice(start, end + 2); | ||
| } |
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.
Adds
levelcode.ai.manageMcp(AI: Manage MCP Servers…) on thepickModelQuickPick pattern, linked from the foot of/mcp. Rows come from the samemcpOverview()S5 already uses, so the two views cannot disagree.What it adds beyond looking
Add / remove a server without hand-writing JSON — the last place MCP forced people into a settings file. It writes the Global tier only, never Workspace:
mcp.serversisapplication-scoped precisely so a repo cannot introduce a server that starts without consent (G1), and a UI that offered the workspace tier would quietly undo that.Revoke G1 trust, per server or workspace-wide. The missing half of trust-on-first-use: approving was write-once with no way back short of editing
workspaceStateby hand, which makes the consent prompt harder to say yes to than it should be. Revoking does not kill a running server, and the wording says "will ask before starting again" rather than implying otherwise.Names a stale approval.
summarizeMcpreports never approved and approved, then the repo changed the command both astrusted:false. Right for starting the server, wrong for explaining it — the second case is exactly the G1 attack: get something benign approved, then swap the command. The row now readscommand changed — needs approval, and the detail view offers to forget the dead entry.parseArgv()The arguments box takes a command line, so something has to turn it into argv. Not a whitespace split —
-y @modelcontextprotocol/server-filesystem "/Users/me/My Documents"is close to the most common MCP server there is, and splitting on whitespace turns one argument into three. It handles single and double quotes and backslash escapes, and it is a splitter, not a shell: no variable expansion, no globbing, operators are plain text. That matchesshell:falseat the spawn site — anything cleverer would be a lie about what runs.Why the Add wizard has no
envstepThe servers that need one need an API token. Prompting for it would put a live credential in plaintext
settings.jsonand make that look like the recommended way.connect()spawns withObject.assign({}, process.env, server.env)(mcpClient.js:52), so exporting the variable and launching from that shell keeps the secret out of any file. The success notification offers Open settings JSON for anyone who wants to do it differently.GitHub MCP, verified end-to-end
docs/MCP.md§9 documents the recipe, run today throughmcpClient.connectrather than described from memory: handshake 122 ms, 41 tools, every name legal under D4, PR #93 read back live, and write access confirmed by probe (no mutation). Also recorded: no GitHub tool setsdestructiveHint, so nothing hits G2's force-ask rule —"*": "allow"would let autopilot merge a pull request.Tests
9 new in
test/mcpManage.test.js, extracted from the shippedextension.jsthe wayctxSegments.test.jsdoes withchat.html(extension.js requiresvscode, and a copied function would be a test of the copy). 4 new inmcpConfig.test.jsforparseArgv.Each was verified non-vacuous by bypassing the fix and confirming failure:
hasOwnProperty→intoStringwould fake an approval)1 toolsingularnode test/*.test.js— 25 files, all green.