fix(mcp): tighten the allow-list name check; never copy a non-object policy

Follow-up review on #41, after merge. Three findings, all valid.

1. isNamespacedToolName was too permissive (and its doc overclaimed).

   It asked "could this have come out of namespaceToolName?" but only checked the
   alphabet and the length, so it accepted `abc`, `read_file`, and
   `'x'.repeat(64)` — none of which namespacing can emit. Such a key would sit
   inert in mcp.toolPolicy looking like it did 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

   Shape 2 is why "must contain __" is still wrong: when a server's name ALONE
   reaches the cap, the cut lands inside that first segment and no separator
   survives. That case must stay legal or "Always allow" silently does nothing for
   that server — the bug the previous round fixed.

   Because being too strict fails SILENTLY, the guard is now swept rather than
   sampled: 560 (server, tool) length pairs across the truncation boundary must all
   validate, and the test asserts it actually reached the truncated and
   separator-less regions so it cannot go vacuous.

2. A non-object mcp.toolPolicy would be copied key-by-key.

   userScopedSetting returns whatever is in settings.json, of whatever type. A
   policy that is accidentally a STRING would have safeCopy faithfully copy its
   character indices — {"0":"a","1":"l",…} — and write that back as the policy,
   destroying it. A malformed value is now discarded, not migrated.

3. The blocking await on mcpAllowAlways was already fixed in f247f1a. Verified it
   is correct; restored the security rationale that commit dropped (the button only
   ever adds an `allow`, is offered only for non-destructive tools, and is re-checked
   regardless) and recorded why it is deliberately not awaited.

49 mcpConfig cases; 23 suites, 0 failures.

Author: ndemianc

extensions/levelcode-ai/extension.js

@@ -775,6 +775,11 @@ let approvalSeq = 0;
  * 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.
  */
+/** A real map — not an array, not null, not a boxed primitive. Guards what we copy out of settings. */
+function isPlainObject(v) {
+	return !!v && typeof v === 'object' && !Array.isArray(v);
+}
+
 async function mcpAllowAlways(name) {
 	// isNamespacedToolName owns the rule (mcpConfig.js), rather than a second regex here: this used to
 	// hand-roll one that required a `__` separator, which REJECTED names namespaceToolName legitimately
@@ -789,7 +794,13 @@ async function mcpAllowAlways(name) {
 		// safeCopy, not Object.assign: the existing value comes from the user's settings.json, where a
 		// literal "__proto__" key survives JSON.parse as a real own property. Object.assign would hand it
 		// to the prototype setter instead of copying it; safeCopy drops the unsafe keys outright.
-		const cur = safeCopy(userScopedSetting(cfg.inspect('mcp.toolPolicy'), {}) || {});
+		//
+		// The plain-object check in front of it matters just as much. userScopedSetting hands back
+		// whatever is in settings.json, of whatever type — and a policy that is accidentally a STRING
+		// would have safeCopy faithfully copy its character indices ({"0":"a","1":"l",…}) and then write
+		// that back as the user's policy, destroying it. A malformed value is discarded, not migrated.
+		const stored = userScopedSetting(cfg.inspect('mcp.toolPolicy'), {});
+		const cur = safeCopy(isPlainObject(stored) ? stored : {});
 		if (cur[name] === 'allow') { return; }
 		cur[name] = 'allow';
 		await cfg.update('mcp.toolPolicy', cur, vscode.ConfigurationTarget.Global);
@@ -1390,7 +1401,14 @@ class ChatViewProvider {
 				case 'stop': dbg('stop.clicked', { running: commandStops.size }); for (const [, stop] of commandStops) { try { stop(); } catch (e) { /* gone */ } } if (abort) { abort.abort(); } clearApprovals(); clearQuestions(); break;
 				case 'stopCommand': { dbg('stopCommand', { id: msg.id }); const s = commandStops.get(msg.id); if (s) { try { s(); } catch (e) { /* gone */ } } break; }
 				case 'approvalResponse': {
-					// Persisting "Always allow" should not block the approved tool call.
+					// "Always allow" persists the tool to the allow-list so a FUTURE run skips the prompt. It
+					// only ever adds an `allow` (never a broadening default), and the webview offers the button
+					// only for non-destructive tools — mcpAllowAlways re-checks regardless.
+					//
+					// Deliberately not awaited: this call is already approved by the click, and the write only
+					// affects later runs, so blocking the tool on a settings round-trip buys nothing. Failures
+					// are swallowed inside mcpAllowAlways and logged; the .catch here is belt-and-braces so a
+					// rejection can never surface as an unhandled one.
 					if (msg.approved && msg.remember && msg.mcpName) {
 						Promise.resolve(mcpAllowAlways(msg.mcpName)).catch(() => { /* best-effort */ });
 					}

extensions/levelcode-ai/mcpConfig.js

@@ -226,27 +226,33 @@ function namespaceToolName(server, tool) {
  * It lives here, beside the function whose output it describes, because the caller was hand-rolling its
  * own regex, and two copies of one naming rule is how they drift apart.
  *
- * Deliberately does NOT require the `__` separator, however much the `server__tool` shape invites it.
- * When a server's name alone reaches the cap, truncation cuts INSIDE that first segment and the
- * hash-tagged result carries no separator at all:
+ * namespaceToolName emits exactly two SHAPES, and this accepts those two and nothing else:
+ *
+ *   1. `server__tool` — the separator survives whenever the joined name fits the cap.
+ *   2. `<57 chars>_<6-char hash>` — the truncated form, always exactly MAX_TOOL_NAME long.
+ *
+ * Shape 2 is why a plain "must contain `__`" test is wrong: when a server's name ALONE reaches the cap,
+ * the cut lands inside that first segment and the result carries no separator at all —
  *
  *   namespaceToolName('s'.repeat(70), 'tool')  ->  'sss…sss_a1b2c3'   // 64 chars, no '__'
  *
- * Requiring it would reject a name this module itself produced, and "Always allow" would then silently
- * do nothing for that server. The alphabet and the length are the real guarantee — they are what makes a
- * name safe to write into settings — so those are what this checks.
+ * — so requiring one rejects a name this module itself produced, and "Always allow" then silently does
+ * nothing for that server. Checking the two shapes keeps that case legal while still refusing a bare
+ * `read_file` or `x`.repeat(64), which namespacing can never emit and which would only sit inert in the
+ * policy map.
  *
- * UNSAFE_KEYS is then rejected EXPLICITLY. The separator requirement used to exclude `__proto__` by
- * accident (it has no non-underscore character before its `__`); dropping that requirement takes the
- * accident with it, since underscores are otherwise legal. Stating it outright means the protection no
- * longer depends on an unrelated rule staying a certain shape.
+ * UNSAFE_KEYS is rejected EXPLICITLY rather than left to fall out of the shape rules, so the protection
+ * does not depend on an unrelated rule keeping a particular form.
  */
+const HASH_TAGGED = /_[0-9a-z]{6}$/;   // shortHash is base36, lower-case, padded to 6
+
 function isNamespacedToolName(name) {
-	return typeof name === 'string'
-		&& name.length > 0
-		&& name.length <= MAX_TOOL_NAME
-		&& UNSAFE_KEYS.indexOf(name) === -1
-		&& /^[A-Za-z0-9_-]+$/.test(name);
+	if (typeof name !== 'string') { return false; }
+	if (name.length === 0 || name.length > MAX_TOOL_NAME) { return false; }
+	if (UNSAFE_KEYS.indexOf(name) !== -1) { return false; }
+	if (!/^[A-Za-z0-9_-]+$/.test(name)) { return false; }
+	return name.indexOf(NAME_SEPARATOR) !== -1                       // shape 1
+		|| (name.length === MAX_TOOL_NAME && HASH_TAGGED.test(name));  // shape 2
 }
 
 /**

extensions/levelcode-ai/test/mcpConfig.test.js

@@ -566,17 +566,56 @@ test('PERSIST: accepts every name namespaceToolName can produce', () => {
 	assert.ok(M.isNamespacedToolName(noSeparator));
 });
 
+test('PERSIST: every name namespaceToolName can emit validates — swept, not sampled', () => {
+	// isNamespacedToolName is deliberately strict, and the failure mode of being too strict is SILENT:
+	// "Always allow" writes nothing and the user is simply asked again forever. A handful of examples
+	// cannot cover the boundary where truncation starts, so sweep both segment lengths across it.
+	let checked = 0;
+	let sawTruncated = 0;
+	let sawNoSeparator = 0;
+
+	for (let s = 1; s <= 80; s++) {
+		for (const t of [1, 2, 7, 30, 63, 64, 90]) {
+			const name = M.namespaceToolName('s'.repeat(s), 't'.repeat(t));
+			assert.ok(LEGAL.test(name), 'precondition: emitted name must be provider-legal: ' + name);
+			assert.ok(M.isNamespacedToolName(name),
+				'rejected a name namespaceToolName produced (server=' + s + ', tool=' + t + '): ' + name);
+			checked++;
+			if (name.length === M.MAX_TOOL_NAME) { sawTruncated++; }
+			if (!name.includes('__')) { sawNoSeparator++; }
+		}
+	}
+
+	// Assert the sweep actually reached the interesting regions, so it cannot quietly become vacuous.
+	assert.ok(checked > 500, 'swept a meaningful space');
+	assert.ok(sawTruncated > 0, 'the sweep must include truncated names');
+	assert.ok(sawNoSeparator > 0, 'the sweep must include the separator-less truncation case');
+});
+
 test('PERSIST: rejects prototype-pollution keys, junk, and unbounded names', () => {
 	for (const bad of ['__proto__', 'constructor', 'prototype']) {
 		assert.ok(!M.isNamespacedToolName(bad), bad + ' must never become a settings key');
 	}
 	assert.ok(!M.isNamespacedToolName('x'.repeat(M.MAX_TOOL_NAME + 1)), 'must be bounded by MAX_TOOL_NAME');
-	assert.ok(M.isNamespacedToolName('x'.repeat(M.MAX_TOOL_NAME)), 'the cap itself is legal');
 	for (const bad of ['', 'has space', 'semi;colon', 'quote"', 'slash/es', null, undefined, 42, {}, []]) {
 		assert.ok(!M.isNamespacedToolName(bad), 'must reject ' + JSON.stringify(bad));
 	}
 });
 
+test('PERSIST: rejects safe-looking names that namespacing can never emit', () => {
+	// Length and alphabet alone are not the contract. These are all "safe" strings, but none can come
+	// out of namespaceToolName, so none belongs in the tool-policy map — an entry like `read_file` would
+	// just sit there inert, looking like it did something.
+	assert.ok(!M.isNamespacedToolName('read_file'), 'a built-in name is not an MCP tool name');
+	assert.ok(!M.isNamespacedToolName('abc'), 'no separator, not the truncated shape');
+	assert.ok(!M.isNamespacedToolName('x'.repeat(M.MAX_TOOL_NAME)),
+		'exactly at the cap but with no hash tag — truncation always appends one');
+	assert.ok(!M.isNamespacedToolName('x'.repeat(57) + '_ABCDEF'),
+		'the hash tag is lower-case base36; upper-case is not a shape this module emits');
+	assert.ok(!M.isNamespacedToolName('short_a1b2c3'),
+		'a hash-looking tail only counts at exactly MAX_TOOL_NAME, which is the only way truncation ends');
+});
+
 test('PERSIST: safeCopy drops the keys that reach the prototype setter', () => {
 	// JSON.parse creates a REAL own __proto__ key, which is how one arrives from settings.json.
 	const fromSettings = JSON.parse('{"gh__list":"allow","__proto__":"allow","constructor":"allow"}');