fix(mcp): share the tool-name rule instead of re-deriving it in extension.js

Review on #41. mcpAllowAlways hand-rolled its own regex to validate what "Always
allow" writes into levelcode.ai.mcp.toolPolicy, and that second copy of the
naming rule had drifted from the one mcpConfig owns.

The reviewer flagged three things. One is not true, one is, and checking the
first turned up a worse bug underneath it.

NOT TRUE — "tool names containing extra `__` are rejected". They are not:
`_` is inside the character class, so the leading group absorbs them and
`github__foo__bar` matches.

TRUE, and worse than reported — a name can legitimately have NO separator at all.
When a server's name alone reaches the 64-char cap, namespaceToolName truncates
INSIDE that first segment and appends a hash tag:

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

The old regex required `__`, so it rejected a name this module itself produced and
"Always allow" silently did nothing for that server.

TRUE — no length bound, so a malformed message could persist an arbitrarily long
settings key. And Object.assign over the existing setting hands a literal
`__proto__` (which survives JSON.parse as a real own key) to the prototype setter
rather than copying it.

Fixed by moving the rule to where it belongs: mcpConfig now exports
isNamespacedToolName (alphabet + MAX_TOOL_NAME bound) and its existing safeCopy,
and extension.js uses both. One definition, beside the function whose output it
describes.

One trap worth recording: 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.

47 mcpConfig cases (up from 44); 23 suites, 0 failures.

Author: ndemianc

extensions/levelcode-ai/extension.js

@@ -27,7 +27,7 @@ const { loadSkills, skillsMenu, getSkillBody } = require('./skills');
 const { openCustomize } = require('./customize');
 const { importFromVscode } = require('./importVscode');
 const { reapMcp } = require('./mcpClient');
-const { userScopedSetting } = require('./mcpConfig');
+const { userScopedSetting, isNamespacedToolName, safeCopy } = require('./mcpConfig');
 
 const SECRET_KEY = 'levelcode.ai.anthropicKey';   // legacy Anthropic key location (kept for back-compat)
 const FILE_EXCLUDES = '{**/node_modules/**,**/.git/**,**/out/**,**/dist/**,**/.vscode-test/**,**/*.map}';
@@ -776,14 +776,23 @@ let approvalSeq = 0;
  * a tool name that is not a namespaced server__tool to avoid writing junk from a malformed message.
  */
 async function mcpAllowAlways(name) {
-	if (typeof name !== 'string' || !/^[A-Za-z0-9_-]+__[A-Za-z0-9_-]+$/.test(name)) {
-		dbg('mcp.allow.reject', { name }); return;
+	// 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
+	// produces — a server whose name alone hits the 64-char cap truncates inside that first segment and
+	// comes back without one. "Always allow" then silently did nothing for that server. The shared check
+	// also bounds the length, so a malformed message cannot persist an arbitrarily long settings key.
+	if (!isNamespacedToolName(name)) {
+		dbg('mcp.allow.reject', { name: String(name).slice(0, 80) }); return;
 	}
 	try {
 		const cfg = aiConfig();
-		const cur = userScopedSetting(cfg.inspect('mcp.toolPolicy'), {}) || {};
+		// 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'), {}) || {});
 		if (cur[name] === 'allow') { return; }
-		await cfg.update('mcp.toolPolicy', Object.assign({}, cur, { [name]: 'allow' }), vscode.ConfigurationTarget.Global);
+		cur[name] = 'allow';
+		await cfg.update('mcp.toolPolicy', cur, vscode.ConfigurationTarget.Global);
 		post({ type: 'agentTool', icon: 'check', text: '🔌 mcp · always allow ' + name });
 		dbg('mcp.allow.persisted', { name });
 	} catch (e) {

extensions/levelcode-ai/mcpConfig.js

@@ -219,6 +219,36 @@ function namespaceToolName(server, tool) {
 	return full.slice(0, MAX_TOOL_NAME - tag.length - 1) + '_' + tag;
 }
 
+/**
+ * Could `name` have come out of namespaceToolName? The guard for anything that PERSISTS a tool name —
+ * today, "Always allow" writing a key into `levelcode.ai.mcp.toolPolicy`.
+ *
+ * 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('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.
+ *
+ * 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.
+ */
+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);
+}
+
 /**
  * Assign a final, unique, provider-legal name to every (server, tool) pair — the last line of defence
  * before names reach the wire. Collisions (with a built-in, or between two servers whose names
@@ -452,7 +482,8 @@ function describeMcpCall(name, args, route) {
 }
 
 module.exports = {
-	loadServerConfig, userScopedSetting, namespaceToolName, assignToolNames, buildAgentTools,
+	loadServerConfig, userScopedSetting, namespaceToolName, isNamespacedToolName, assignToolNames,
+	buildAgentTools, safeCopy,
 	toolCountsByServer, classifyMcpTool, explainMcpRefusal, describeMcpCall,
 	BUILTIN_TOOL_NAMES, MAX_TOOL_NAME, MAX_TOOL_DESC, MAX_ARG_CHARS, MAX_SERVERS, MAX_TOOLS_PER_SERVER, WORKSPACE_CONFIG_PATH
 };

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

@@ -543,4 +543,49 @@ test('CARD: arguments are shown in full but bounded, and never throw', () => {
 	assert.strictEqual(M.describeMcpCall('s__t', undefined, { server: 's', tool: 't' }).argsText, '');
 });
 
+// ---- isNamespacedToolName: the guard for anything that PERSISTS a tool name ----
+// "Always allow" writes the name into settings, so this decides what can be written.
+
+test('PERSIST: accepts every name namespaceToolName can produce', () => {
+	const produced = [
+		M.namespaceToolName('github', 'list_issues'),
+		M.namespaceToolName('github', 'foo__bar'),          // tool name already contains the separator
+		M.namespaceToolName('a', 'b'),
+		M.namespaceToolName('srv', 't'.repeat(90)),          // truncated on the TOOL side
+		M.namespaceToolName('s'.repeat(70), 'tool'),         // truncated inside the SERVER segment
+		M.namespaceToolName('has spaces', 'and.dots')        // sanitized to the legal alphabet
+	];
+	for (const name of produced) {
+		assert.ok(M.isNamespacedToolName(name), 'must accept a name it produced: ' + name);
+	}
+
+	// The regression this replaced: a server name long enough to be truncated comes back with NO `__`,
+	// so a validator that required the separator rejected it and "Always allow" silently did nothing.
+	const noSeparator = M.namespaceToolName('s'.repeat(70), 'tool');
+	assert.ok(!noSeparator.includes('__'), 'this case must actually lack the separator, or the test is vacuous');
+	assert.ok(M.isNamespacedToolName(noSeparator));
+});
+
+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: 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"}');
+	const copy = M.safeCopy(fromSettings);
+
+	assert.strictEqual(copy.gh__list, 'allow', 'legitimate entries survive');
+	assert.ok(!Object.prototype.hasOwnProperty.call(copy, '__proto__'), '__proto__ must be dropped');
+	assert.ok(!Object.prototype.hasOwnProperty.call(copy, 'constructor'), 'constructor must be dropped');
+	assert.strictEqual(Object.getPrototypeOf(copy), Object.prototype, 'the copy keeps a clean prototype');
+});
+
 console.log('\nmcpConfig.js: ' + n + ' tests passed.');