fix(mcp): keep the MCP chip and refusal message honest about destructive tools

PR #31 review (Copilot). Both findings had one root: a destructiveHint tool can
never be enabled by the allow-list (classifyMcpTool forces 'ask' regardless), yet
two callers spoke as if it could.

1. The startup chip counted allow-listed tools by calling classifyMcpTool WITHOUT
   annotations, while runTool calls it WITH them. So a destructive-but-allow-listed
   tool was counted as "allow-listed" in the chip and then refused at call time —
   the chip lied. Now threads the same route annotations into the count.

2. The refusal message unconditionally told the model to add the tool to
   levelcode.ai.mcp.toolPolicy as "allow". For a destructive tool that is
   impossible, so the model would allow-list it, retry, and be refused again.

Root-cause fix rather than two patches: classifyMcpTool now also returns
policyCanAllow (false only for the destructive tighten-path), and the refusal
message moves OUT of agent.js into explainMcpRefusal() beside the classifier —
the message that drifted from the policy now branches solely on the verdict, in
the same module, and is unit-testable off the editor (agent.js requires vscode).
agent.js just calls it.

Verified:
- 37 tests (2 new + policyCanAllow assertions on the existing POLICY cases).
- Both fixes mutation-checked: a destructive verdict claiming policyCanAllow:true
  fails; a message that ignores the discriminator (the original bug) fails.
- End-to-end against the S2 fixture with 'boom' marked destructive: the exact
  chip-count logic returns 0/3 for an allow-listed destructive tool, the message
  omits any toolPolicy instruction, and a plain allow-listed tool still counts 1
  and would run. The chip-count line itself lives in agent.js and so is covered by
  that integration run + reading, not CI — same as the router glue.
- Full gate: 18 suites, 0 failures.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: ndemianc

extensions/levelcode-ai/agent.js

@@ -17,7 +17,7 @@ const providers = require('./providers/index');
 const { formatVerifyFeedback, verifyOutcome, looksUnrunnable, sniffPort, looksReady } = require('./verify');
 const { classifyCommand, dangerLabel } = require('./commandSafety');
 const { loadProjectRules } = require('./projectRules');
-const { loadServerConfig, buildAgentTools, classifyMcpTool } = require('./mcpConfig');
+const { loadServerConfig, buildAgentTools, classifyMcpTool, explainMcpRefusal } = require('./mcpConfig');
 const { connectAll, getServer } = require('./mcpClient');
 
 const SYSTEM_BASE = [
@@ -457,13 +457,12 @@ async function runTool(tu, ctx) {
 			if (verdict.approve !== 'allow') {
 				// S3 deliberately ships no approval CARD (S4 owns it), so anything the user has not
 				// explicitly allow-listed is REFUSED rather than run — the alternative would be silently
-				// executing third-party code on the user's behalf with no way to say no. The message is
-				// addressed to the model but written for the user's benefit: it names the exact setting.
+				// executing third-party code on the user's behalf with no way to say no. The explanation
+				// lives in mcpConfig beside the classifier so it can't drift from it (PR #31 review): a
+				// destructive tool is refused for a reason the allow-list cannot fix, and must not be
+				// described as allow-listable.
 				ctx.post({ type: 'agentTool', icon: 'shield', text: '🔌 mcp · refused ' + tu.name + ' — ' + verdict.reason });
-				return 'ERROR: the MCP tool "' + tu.name + '" is not approved to run (' + verdict.reason + '), and '
-					+ 'approval prompts are not available in this build. To allow it, the USER must add '
-					+ '"' + tu.name + '": "allow" to the "levelcode.ai.mcp.toolPolicy" setting. Do NOT retry it in '
-					+ 'this run — continue without it, or tell the user what you needed it for.';
+				return explainMcpRefusal(tu.name, verdict);
 			}
 			const server = getServer(route.server);
 			if (!server || !server.alive) { return 'ERROR: the MCP server "' + route.server + '" is not running.'; }
@@ -543,8 +542,13 @@ async function setupMcp(ctx, wsFolders, dbg) {
 		if (!built.tools.length) { return empty; }
 
 		// Show the allow-listed count up front: with no policy set it reads "0/12 allow-listed", which is
-		// what makes a later refusal legible instead of looking broken.
-		const allowed = built.tools.filter((t) => classifyMcpTool(t.name, cfg.toolPolicy).approve === 'allow').length;
+		// what makes a later refusal legible instead of looking broken. Pass the SAME annotations runTool
+		// will (PR #31 review) — otherwise a destructive-but-allow-listed tool is counted here yet refused
+		// there, and the chip lies. The route always exists for a built tool; guard defensively anyway.
+		const allowed = built.tools.filter((t) => {
+			const route = built.routes.get(t.name);
+			return classifyMcpTool(t.name, cfg.toolPolicy, route && route.annotations).approve === 'allow';
+		}).length;
 		const summary = handles.map((h) => h.name + ' (' + h.tools.length + ')').join(', ');
 		dbg('mcp.ready', { servers: handles.map((h) => h.name), tools: built.tools.length, allowed });
 		ctx.post({ type: 'agentTool', icon: 'plug', text: '🔌 mcp · ' + summary + ' · ' + allowed + '/' + built.tools.length + ' allow-listed' });

extensions/levelcode-ai/mcpConfig.js

@@ -309,25 +309,52 @@ function buildAgentTools(servers, opts) {
  * only push toward asking, never toward allowing: a `destructiveHint` overrides an allow-list entry
  * (worst case, one extra prompt), while a `readOnlyHint` grants nothing on its own.
  *
+ * `policyCanAllow` answers a question the callers kept getting wrong (PR #31 review): would adding
+ * this tool to the allow-list actually grant it? For every ordinary refusal, yes. For a `destructiveHint`
+ * refusal, NO — a server hint may only tighten, so the allow-list cannot override it. Callers use this to
+ * avoid (a) counting a destructive-but-allow-listed tool as "allow-listed" in the startup chip, and
+ * (b) telling the model to allow-list a tool that allow-listing can never enable.
+ *
  * @param {string} name       the namespaced tool name (server__tool)
  * @param {object} [policy]   user map, e.g. { 'github__list_issues': 'allow', '*': 'ask' }
  * @param {object} [annotations]  the server's own hints for this tool (untrusted)
- * @returns {{ approve: 'ask'|'allow', reason: string }}
+ * @returns {{ approve: 'ask'|'allow', reason: string, policyCanAllow: boolean }}
  */
 function classifyMcpTool(name, policy, annotations) {
 	if (annotations && annotations.destructiveHint === true) {
-		return { approve: 'ask', reason: 'the server marks this tool destructive' };
+		return { approve: 'ask', reason: 'the server marks this tool destructive', policyCanAllow: false };
 	}
 	const p = policy || {};
 	const exact = p[name];
-	if (exact === 'allow') { return { approve: 'allow', reason: 'allow-listed by you' }; }
-	if (exact === 'ask') { return { approve: 'ask', reason: 'set to ask by you' }; }
+	if (exact === 'allow') { return { approve: 'allow', reason: 'allow-listed by you', policyCanAllow: true }; }
+	if (exact === 'ask') { return { approve: 'ask', reason: 'set to ask by you', policyCanAllow: true }; }
 	const star = p['*'];
-	if (star === 'allow') { return { approve: 'allow', reason: 'allow-listed by you (*)' }; }
-	return { approve: 'ask', reason: 'third-party tool (default)' };
+	if (star === 'allow') { return { approve: 'allow', reason: 'allow-listed by you (*)', policyCanAllow: true }; }
+	return { approve: 'ask', reason: 'third-party tool (default)', policyCanAllow: true };
+}
+
+/**
+ * The agent-facing explanation for a refused MCP call in a build with no approval card (S3). It lives
+ * HERE, beside classifyMcpTool, on purpose: the PR #31 review caught this message telling the model to
+ * allow-list a destructive tool that allow-listing can never enable — the message had drifted from the
+ * policy. Keeping both in one module (and unit-testing this off the editor) is what stops the drift
+ * recurring. Branches solely on the verdict, so it cannot disagree with the classifier.
+ *
+ * @param {string} name  the namespaced tool name
+ * @param {{reason:string, policyCanAllow:boolean}} verdict  from classifyMcpTool (a non-'allow' one)
+ * @returns {string}
+ */
+function explainMcpRefusal(name, verdict) {
+	const head = 'ERROR: the MCP tool "' + name + '" is not approved to run (' + verdict.reason + '). ';
+	const fix = verdict.policyCanAllow
+		? 'This build has no per-call approval prompt, so the only way to permit it is for the USER to add '
+			+ '"' + name + '": "allow" to the "levelcode.ai.mcp.toolPolicy" setting. '
+		: 'Such tools always require per-call approval — which this build does not yet provide — so it '
+			+ 'CANNOT be enabled through the allow-list. ';
+	return head + fix + 'Do NOT retry it in this run — continue without it, or tell the user what you needed it for.';
 }
 
 module.exports = {
-	loadServerConfig, namespaceToolName, assignToolNames, buildAgentTools, classifyMcpTool,
+	loadServerConfig, namespaceToolName, assignToolNames, buildAgentTools, classifyMcpTool, explainMcpRefusal,
 	BUILTIN_TOOL_NAMES, MAX_TOOL_NAME, MAX_TOOL_DESC, MAX_SERVERS, MAX_TOOLS_PER_SERVER, WORKSPACE_CONFIG_PATH
 };

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

@@ -250,12 +250,43 @@ test('POLICY: a server destructiveHint OVERRIDES an allow-list entry (tighten-on
 	const r = M.classifyMcpTool('gh__nuke', { 'gh__nuke': 'allow' }, { destructiveHint: true });
 	assert.strictEqual(r.approve, 'ask');
 	assert.ok(/destructive/.test(r.reason));
+	// PR #31 review: the allow-list did not override it, so it must ALSO report that the allow-list
+	// cannot help — otherwise the chip counts it and the refusal message misdirects the model.
+	assert.strictEqual(r.policyCanAllow, false);
+});
+
+test('POLICY: policyCanAllow is true for every refusal the allow-list CAN fix', () => {
+	// Everything except a destructive hint is unblockable by editing the policy — the callers rely on
+	// this to decide whether to say "add it to the allow-list".
+	assert.strictEqual(M.classifyMcpTool('gh__x').policyCanAllow, true);                       // default ask
+	assert.strictEqual(M.classifyMcpTool('gh__x', { 'gh__x': 'ask' }).policyCanAllow, true);    // explicit ask
+	assert.strictEqual(M.classifyMcpTool('gh__x', { 'gh__x': 'allow' }).policyCanAllow, true);  // already allowed
+	assert.strictEqual(M.classifyMcpTool('gh__x', {}, { readOnlyHint: true }).policyCanAllow, true);
 });
 
 test('POLICY: a readOnlyHint grants nothing on its own (annotations are untrusted)', () => {
 	assert.strictEqual(M.classifyMcpTool('gh__read', {}, { readOnlyHint: true }).approve, 'ask');
 });
 
+test('REFUSAL: the message tells the model to allow-list ONLY when that would work', () => {
+	// The exact bug from the PR #31 review: a destructive tool was told to allow-list itself, which
+	// classifyMcpTool can never honour. Drive explainMcpRefusal off the real verdicts so the message
+	// and the policy cannot disagree.
+	const destructive = M.classifyMcpTool('gh__nuke', { 'gh__nuke': 'allow' }, { destructiveHint: true });
+	const mDestructive = M.explainMcpRefusal('gh__nuke', destructive);
+	assert.ok(!/toolPolicy/.test(mDestructive), 'must NOT point a destructive tool at the allow-list');
+	assert.ok(/destructive/.test(mDestructive) && /CANNOT/.test(mDestructive), 'must say why it is unfixable');
+
+	const plain = M.classifyMcpTool('gh__read');   // default ask, fixable
+	const mPlain = M.explainMcpRefusal('gh__read', plain);
+	assert.ok(/"gh__read": "allow"/.test(mPlain) && /toolPolicy/.test(mPlain), 'must name the exact setting to add');
+
+	for (const m of [mDestructive, mPlain]) {
+		assert.ok(/^ERROR:/.test(m), 'stays an ERROR string so the loop treats it as a tool failure');
+		assert.ok(/Do NOT retry/.test(m), 'must tell the model not to retry, or it loops');
+	}
+});
+
 // ---- 4. buildAgentTools: MCP tool specs → agent descriptors + routing table (S3) --------------
 
 const SRV = (name, tools) => ({ name, tools });