Merge pull request #2843 from QwenLM/fix/coretools-mcp-bypass

fix(permissions): allow non-core tools to bypass coreTools allowlist

packages/core/src/permissions/permission-manager.test.ts

@@ -1254,6 +1254,57 @@ describe('PermissionManager', () => {
       expect(await pm.isToolEnabled('read_file')).toBe(true);
       expect(await pm.isToolEnabled('run_shell_command')).toBe(true); // not denied, just unreviewed
     });
+
+    // Non-core tools bypass coreTools allowlist
+    it('MCP tools bypass coreTools allowlist check', async () => {
+      pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] }));
+      pm.initialize();
+      // MCP tools should be enabled even if not in coreTools
+      expect(
+        await pm.isToolEnabled('mcp__markitdown__convert_to_markdown'),
+      ).toBe(true);
+      expect(await pm.isToolEnabled('mcp__puppeteer__navigate')).toBe(true);
+    });
+
+    it('Skill tool bypasses coreTools allowlist check', async () => {
+      pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] }));
+      pm.initialize();
+      expect(await pm.isToolEnabled('skill')).toBe(true);
+    });
+
+    it('Agent tool bypasses coreTools allowlist check', async () => {
+      pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] }));
+      pm.initialize();
+      expect(await pm.isToolEnabled('agent')).toBe(true);
+    });
+
+    it('exit_plan_mode tool bypasses coreTools allowlist check', async () => {
+      pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] }));
+      pm.initialize();
+      expect(await pm.isToolEnabled('exit_plan_mode')).toBe(true);
+    });
+
+    it('ask_user_question tool bypasses coreTools allowlist check', async () => {
+      pm = new PermissionManager(makeConfig({ coreTools: ['read_file'] }));
+      pm.initialize();
+      expect(await pm.isToolEnabled('ask_user_question')).toBe(true);
+    });
+
+    it('Non-core tools still respect deny rules', async () => {
+      pm = new PermissionManager(
+        makeConfig({
+          coreTools: ['read_file'],
+          permissionsDeny: ['mcp__markitdown'],
+        }),
+      );
+      pm.initialize();
+      // MCP tool should be disabled due to deny rule, even though it bypasses coreTools
+      expect(
+        await pm.isToolEnabled('mcp__markitdown__convert_to_markdown'),
+      ).toBe(false);
+      // Other MCP tools without deny rule should still be enabled
+      expect(await pm.isToolEnabled('mcp__puppeteer__navigate')).toBe(true);
+    });
   });
 
   describe('session rules', () => {

packages/core/src/permissions/permission-manager.ts

@@ -396,6 +396,35 @@ export class PermissionManager {
   // Registry-level helper
   // ---------------------------------------------------------------------------
 
+  /**
+   * Core tools that are subject to the coreTools allowlist check.
+   * Tools not in this set (MCP, Skill, Agent, etc.) bypass the check.
+   */
+  private static readonly CORE_TOOLS = new Set([
+    'read_file',
+    'write_file',
+    'edit',
+    'glob',
+    'grep_search',
+    'run_shell_command',
+    'list_directory',
+    'web_fetch',
+    'web_search',
+    'todo_write',
+    'save_memory',
+    'lsp',
+    'cron_create',
+    'cron_list',
+    'cron_delete',
+  ]);
+
+  /**
+   * Check if a tool is a core tool subject to the coreTools allowlist check.
+   */
+  private isCoreTool(toolName: string): boolean {
+    return PermissionManager.CORE_TOOLS.has(toolName);
+  }
+
   /**
    * Determine whether a tool should be present in the tool registry.
    *
@@ -403,13 +432,23 @@ export class PermissionManager {
    * (i.e. a whole-tool deny) matches.  Specifier-based deny rules such as
    * `"Bash(rm -rf *)"` do NOT remove the tool from the registry – they only
    * deny specific invocations at runtime.
+   *
+   * Non-core tools (MCP, Skill, Agent, etc.) skip the coreTools allowlist
+   * check because they are dynamically discovered or essential for system
+   * operation.
    */
   async isToolEnabled(toolName: string): Promise<boolean> {
     const canonicalName = resolveToolName(toolName);
 
-    // If a coreTools allowlist is active, only explicitly listed tools are
-    // registered. This mirrors the legacy `tools.core` whitelist semantic:
-    // any tool NOT in the allowlist is excluded from the registry entirely.
+    // Non-core tools bypass coreTools allowlist check
+    if (!this.isCoreTool(canonicalName)) {
+      const decision = await this.evaluate({ toolName: canonicalName });
+      return decision !== 'deny';
+    }
+
+    // Core tools: if a coreTools allowlist is active, only explicitly listed
+    // tools are registered. This mirrors the legacy `tools.core` whitelist
+    // semantic: any tool NOT in the allowlist is excluded from the registry.
     if (this.coreToolsAllowList !== null && this.coreToolsAllowList.size > 0) {
       if (!this.coreToolsAllowList.has(canonicalName)) {
         return false;