feat(mcp): per-call approval card — S4a

[ndemianc]

S3 refused any MCP tool the user hadn't hand-added to levelcode.ai.mcp.toolPolicy — the safe-but-unusable placeholder. S4a replaces that wall with an actual prompt.

An un-allow-listed call now shows a card — server · tool · the real arguments — with Skip / Allow once / Always allow. "Always allow" writes the tool to the allow-list, so future runs skip the prompt. The allow-list stays the one thing that grants allow (G3); Autopilot relaxes none of it.

Verified

The card, visually — I screenshotted both variants rather than shipping blind:

normal tool	destructive tool
Skip · Always allow · Allow once	Skip · Allow once (no "Always allow")
args pretty-printed & wrapping	amber "server marks this destructive" warning

(Two buttons sharing .approve was a selector bug — querySelector('.approve') grabbed the wrong one. Caught in the visual check, fixed with distinct classes.)

The flow, end-to-end against the S2 fixture server — five paths:

1. allow-listed → runs, no prompt
2. un-listed → prompts → Allow → runs (card carried the real args)
3. un-listed → prompts → Skip → declined, tool not called
4. destructive + allow-listed → still prompts, card offers no "Always allow"
5. no webview (headless/tests) → refuses with the non-interactive message

Security properties

• A destructive tool always prompts (classifyMcpTool tightens on it) and the card hides "Always allow" — a destructive tool can never be allow-listed, so offering it would be a dead button. canAllowAlways is derived from the same annotation the classifier reads, so they can't drift.
• Arguments shown in full (length-capped) — that is the decision; the user owns the credentials and needs to see the repo it touches, the row it deletes. The card is ephemeral UI, never the transcript; G4's debug-log redaction is unchanged.
• Always allow writes the USER (Global) tier, matching the setting's application scope, and rejects a non-namespaced tool name.

Also

• MCP chips used icon:'plug', which isn't a registered codicon — addAgentLine rendered the literal word "plug" in the rail. Swapped to sparkle. The identical issue on the auto-preview globe chip is feat(ai): auto-open the built-in browser when the agent starts a web server #35's; flagged, not mixed in here.
• explainMcpRefusal's wording ("this build has no prompt") corrected — it's now only the non-interactive fallback.

Deferred to S4b

The G1 trust-on-first-use launch gate for workspace-file (.levelcode/mcp.json) servers — still read-and-listed but never started. This PR is the per-call gate; the launch gate is its own reviewable slice.

Verified: 24 suites, 0 failures (mcpConfig 44 cases); describeMcpCall mutation-checked.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Implements S4a’s per-call MCP approval UX by replacing the prior “refuse unless pre-allow-listed” behavior with an interactive approval card that shows server/tool and a bounded, pretty-printed argument preview, plus “Allow once” and (when applicable) “Always allow” persistence to the user-scoped allow-list.

Changes:

• Add MCP-specific approval-card rendering in the chat webview, including destructive-tool warnings and “Always allow” gating.
• Introduce describeMcpCall() + argument preview bounding in mcpConfig.js, and wire agent MCP calls to prompt (or refuse in non-interactive contexts).
• Persist “Always allow” decisions to levelcode.ai.mcp.toolPolicy at the Global (user) tier, and update MCP activity chips to use a valid codicon.

Reviewed changes

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

Show a summary per file

File	Description
extensions/levelcode-ai/test/mcpConfig.test.js	Adds unit coverage for describeMcpCall() (server/tool derivation, destructive gating, bounded args preview).
extensions/levelcode-ai/media/chat.html	Adds MCP approval card UI (args well, button semantics, destructive warning, and selector fix via distinct classes).
extensions/levelcode-ai/mcpConfig.js	Adds MAX_ARG_CHARS, previewArgs(), and describeMcpCall() to shape approval-card data consistently with classifier annotations.
extensions/levelcode-ai/extension.js	Adds “Always allow” persistence plumbing on approval responses (writes to Global user settings).
extensions/levelcode-ai/agent.js	Changes MCP tool execution to prompt when not allow-listed (or always for destructive), with non-interactive fallback refusal and corrected codicon usage.

---

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

[ndemianc]

Fixed in fab9021 — the underlying point (a second copy of the naming rule, drifting from the one mcpConfig owns) was exactly right, and chasing it turned up a worse bug than the one reported.

(1) "extra __ is rejected" — not true. _ is inside the character class, so the leading group absorbs them:

/^[A-Za-z0-9_-]+__[A-Za-z0-9_-]+$/.test("github__foo__bar")   // true

But a name can legitimately have NO separator at all, and that the old regex did reject. When a server name alone reaches the 64-char cap, namespaceToolName truncates INSIDE the first segment and appends a hash tag:

namespaceToolName("s".repeat(70), "tool")  ->  "ssss…ssss_a1b2c3"   // 64 chars, no "__"

So the regex rejected a name this module itself produced, and "Always allow" silently did nothing for that server. Requiring the separator was the bug, not the fix.

(2) No length bound — correct. A 500-char name passed and would have been persisted as a settings key.

(3) Object.assign — correct in substance. The new key was already safe (__proto__ fails that regex), but the existing value comes from settings.json, where a literal __proto__ survives JSON.parse as a real own property and Object.assign hands it to the prototype setter. Now safeCopy, which mcpConfig already had for exactly this and which is now exported.

The fix is your suggestion, at the root: mcpConfig exports isNamespacedToolName (alphabet + MAX_TOOL_NAME) and safeCopy; extension.js uses both. One definition, beside the function whose output it describes.

One trap worth flagging. Relaxing the separator requirement silently removed an accidental protection — __proto__ failed the old regex only because it has no non-underscore character before its __. isNamespacedToolName therefore rejects UNSAFE_KEYS explicitly, so the guarantee no longer rides on an unrelated rule keeping a particular shape. My own new test caught that regression before it shipped.

47 mcpConfig cases (from 44); 23 suites, 0 failures. Also rebased onto develop — the branch was 42 commits behind.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

extensions/levelcode-ai/extension.js:777

• The doc comment says mcpAllowAlways() “refuses a tool name that is not a namespaced server__tool”, but the validator intentionally does not require the '__' separator (see isNamespacedToolName). This mismatch makes it unclear what is actually guaranteed/persisted.

 * Persist an MCP tool to the allow-list (the ONLY thing that grants 'allow' — G3). Writes to the USER
 * (Global) tier, matching the application scope the setting is declared with, so a repo can never flip
 * it. Reads the current value the same user-scoped way it is read at run start. Idempotent, and refuses
 * a tool name that is not a namespaced server__tool to avoid writing junk from a malformed message.
 */

extensions/levelcode-ai/extension.js:792

• safeCopy() will happily copy keys from non-object values (e.g. a string/array) via Object.keys(...), which could produce a surprising toolPolicy map if the user’s setting is mis-typed. It’s safer to coerce to a plain object before copying.

		const cur = safeCopy(userScopedSetting(cfg.inspect('mcp.toolPolicy'), {}) || {});

[Copilot]

Pull request overview

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

[ndemianc]

Both of these are the same finding, and you are right — fixed in #56.

isNamespacedToolName asked "could this have come out of namespaceToolName?" but only checked alphabet and length, so it accepted abc, read_file, and 'x'.repeat(64). The doc overclaimed and the caller's intent was undermined; a key like that would sit inert in the policy map looking like it had done something.

It now accepts exactly the two shapes namespaceToolName emits:

1. server__tool — whenever the joined name fits the cap
2. <57 chars>_<6-char base36 hash> — the truncated form, always exactly 64 chars

One correction to the reasoning in the second comment: truncation does not always leave a separator. When a server's name alone reaches the cap, the cut lands inside that first segment:

namespaceToolName('s'.repeat(70), 'tool')  ->  'sss…sss_a1b2c3'   // 64 chars, no '__'

So a plain "must contain __" test still rejects a legitimate name — that was the bug the previous round fixed, and shape 2 is what keeps it fixed while adding the strictness you asked for.

The 'x'.repeat(MAX_TOOL_NAME) assertion you flagged is inverted: it is now asserted as a rejection, alongside read_file, abc, an upper-case hash tail, and a hash-looking tail on a short name.

Because being too strict fails silently — "Always allow" writes nothing and the user is asked forever — the accept side is swept rather than sampled: 560 (server, tool) length pairs across the truncation boundary must all validate, and the test asserts the sweep actually reached the truncated and separator-less regions so it cannot go vacuous.

[ndemianc]

Correct — fixed in #56.

userScopedSetting returns info.globalValue whatever its type, so nothing between settings.json and safeCopy was checking the shape. A policy that is accidentally a string would have safeCopy faithfully copy its character indices and write that back:

"allow"  ->  { "0": "a", "1": "l", "2": "l", "3": "o", "4": "w", "<tool>": "allow" }

which destroys the user's policy rather than repairing it. An array would do the same with numeric indices.

mcpAllowAlways now normalizes first — a non-plain-object value is discarded, not migrated, on the grounds that we cannot know what a corrupted policy was meant to say, and silently inventing one is worse than starting clean.

[ndemianc]

Both of these are the same finding, and you are right — fixed in #56.

isNamespacedToolName asked "could this have come out of namespaceToolName?" but only checked alphabet and length, so it accepted abc, read_file, and 'x'.repeat(64). The doc overclaimed and the caller's intent was undermined; a key like that would sit inert in the policy map looking like it had done something.

It now accepts exactly the two shapes namespaceToolName emits:

1. server__tool — whenever the joined name fits the cap
2. <57 chars>_<6-char base36 hash> — the truncated form, always exactly 64 chars

One correction to the reasoning in the second comment: truncation does not always leave a separator. When a server's name alone reaches the cap, the cut lands inside that first segment:

namespaceToolName('s'.repeat(70), 'tool')  ->  'sss…sss_a1b2c3'   // 64 chars, no '__'

So a plain "must contain __" test still rejects a legitimate name — that was the bug the previous round fixed, and shape 2 is what keeps it fixed while adding the strictness you asked for.

The 'x'.repeat(MAX_TOOL_NAME) assertion you flagged is inverted: it is now asserted as a rejection, alongside read_file, abc, an upper-case hash tail, and a hash-looking tail on a short name.

Because being too strict fails silently — "Always allow" writes nothing and the user is asked forever — the accept side is swept rather than sampled: 560 (server, tool) length pairs across the truncation boundary must all validate, and the test asserts the sweep actually reached the truncated and separator-less regions so it cannot go vacuous.