Uh oh!
There was an error while loading. Please reload this page.
feat(mcp): S6a — Manage MCP servers, and take back a G1 approval - #58
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.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
Fixed in a6374d3. parseArgv now treats a backslash as an escape only when the next character is a delimiter — whitespace, a quote, or another backslash — and keeps it literal otherwise:
if(ch==='\\'&"e!=="'"){constnxt=s[i+1];if(nxt!==undefined&&(/\s/.test(nxt)||nxt==='"'||nxt==="'"||nxt==='\\')){cur+=nxt;i++;started=true;continue;}// not escaping a delimiter → fall through, backslash is an ordinary char}So \bword\b, C:\Users\me, and a JSON blob survive verbatim; \ / \" / \\ still escape a space/quote/backslash, and a trailing \ is kept literally. Added a regression test (mcpConfig.test.js) covering each of those cases.
| 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; | ||
| } |
There was a problem hiding this comment.
Fixed in a6374d3. The add-server name validator now rejects the reserved keys explicitly, reusing the UNSAFE_KEYS list that safeCopy() and normalizeServer() already enforce (now exported from mcpConfig.js):
if(UNSAFE_KEYS.indexOf(t)!==-1){return'That name is reserved — choose another.';}You're right that they slipped through: __proto__ / constructor / prototype all pass the [A-Za-z0-9_-]+ charset rule, so existing['__proto__'] = entry would have hit the prototype setter and corrupted the map. safeCopy() already guards the read path (it drops those keys); this closes the write path. The QuickPick can't accept a name while validateInput returns a message, so a reserved name never reaches the assignment — and normalizeServer() would reject it at config-load as a final backstop.
| 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); | ||
| } |
There was a problem hiding this comment.
Fixed in a6374d3. Replaced the first-\n} scan with a brace-depth match: it scans from the function's opening brace, skips braces that live inside strings or comments, and stops at the brace that returns depth to zero.
if(ch==='{'){depth++;}elseif(ch==='}'&&--depth===0){returnsrc.slice(start,i+1);}Now a closing brace reformatted onto its own line (or a {}/object literal inside the body) no longer truncates the extracted function. All 9 mcpManage tests still pass against the real mcpTrustIsStale / mcpServerItem.
…obust test extract - parseArgv: a backslash now escapes ONLY a following space/quote/backslash and is otherwise literal, so a regex arg (\bword\b), a Windows path (C:\Users\me) or a JSON string survives instead of silently losing its backslashes — it is a splitter, not a shell. - Add-server name validator: reject the reserved object keys (__proto__/constructor/prototype), reusing mcpConfig's UNSAFE_KEYS. They pass the charset rule but existing[name] = entry would hit the prototype setter and corrupt the settings map (safeCopy already guards the read path). - mcpManage test extract(): replace the fragile first-\n} scan with a string/comment-aware brace-depth match, so reformatting a closing brace onto its own line no longer truncates. New parseArgv backslash tests. Full extension suite: 25 files, 0 failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
extensions/levelcode-ai/extension.js:1447
- The QuickPick placeholder for server actions uses
s.origindirectly for workspace-defined servers. Whenoriginis missing/empty (whichmcpServerItem()already treats asworkspace), this placeholder can show as"<name> · "or"<name> · undefined", which is confusing/inconsistent with the main list row.
const pick = await vscode.window.showQuickPick(items, { placeHolder: s.name + ' · ' + (s.source === 'settings' ? 'your settings' : s.origin) });
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.