fix(core): prioritize conditional policy rules and harden Plan Mode (#18882)

packages/core/src/policy/policy-engine.test.ts

@@ -2046,33 +2046,91 @@ describe('PolicyEngine', () => {
         rules: [],
         expected: [],
       },
+      {
+        name: 'should apply rules without explicit modes to all modes',
+        rules: [{ toolName: 'tool1', decision: PolicyDecision.DENY }],
+        expected: ['tool1'],
+      },
+      {
+        name: 'should NOT exclude tool if higher priority argsPattern rule exists',
+        rules: [
+          {
+            toolName: 'tool1',
+            decision: PolicyDecision.ALLOW,
+            argsPattern: /safe/,
+            priority: 100,
+            modes: [ApprovalMode.DEFAULT],
+          },
+          {
+            toolName: 'tool1',
+            decision: PolicyDecision.DENY,
+            priority: 10,
+            modes: [ApprovalMode.DEFAULT],
+          },
+        ],
+        expected: [],
+      },
       {
         name: 'should include tools with DENY decision',
         rules: [
-          { toolName: 'tool1', decision: PolicyDecision.DENY },
+          {
-          { toolName: 'tool2', decision: PolicyDecision.ALLOW },
+            toolName: 'tool1',
+            decision: PolicyDecision.DENY,
+            modes: [ApprovalMode.DEFAULT],
+          },
+          {
+            toolName: 'tool2',
+            decision: PolicyDecision.ALLOW,
+            modes: [ApprovalMode.DEFAULT],
+          },
         ],
         expected: ['tool1'],
       },
       {
         name: 'should respect priority and ignore lower priority rules (DENY wins)',
         rules: [
-          { toolName: 'tool1', decision: PolicyDecision.DENY, priority: 100 },
+          {
-          { toolName: 'tool1', decision: PolicyDecision.ALLOW, priority: 10 },
+            toolName: 'tool1',
+            decision: PolicyDecision.DENY,
+            priority: 100,
+            modes: [ApprovalMode.DEFAULT],
+          },
+          {
+            toolName: 'tool1',
+            decision: PolicyDecision.ALLOW,
+            priority: 10,
+            modes: [ApprovalMode.DEFAULT],
+          },
         ],
         expected: ['tool1'],
       },
       {
         name: 'should respect priority and ignore lower priority rules (ALLOW wins)',
         rules: [
-          { toolName: 'tool1', decision: PolicyDecision.ALLOW, priority: 100 },
+          {
-          { toolName: 'tool1', decision: PolicyDecision.DENY, priority: 10 },
+            toolName: 'tool1',
+            decision: PolicyDecision.ALLOW,
+            priority: 100,
+            modes: [ApprovalMode.DEFAULT],
+          },
+          {
+            toolName: 'tool1',
+            decision: PolicyDecision.DENY,
+            priority: 10,
+            modes: [ApprovalMode.DEFAULT],
+          },
         ],
         expected: [],
       },
       {
         name: 'should NOT include ASK_USER tools even in non-interactive mode',
-        rules: [{ toolName: 'tool1', decision: PolicyDecision.ASK_USER }],
+        rules: [
+          {
+            toolName: 'tool1',
+            decision: PolicyDecision.ASK_USER,
+            modes: [ApprovalMode.DEFAULT],
+          },
+        ],
         nonInteractive: true,
         expected: [],
       },
@@ -2083,6 +2141,7 @@ describe('PolicyEngine', () => {
             toolName: 'tool1',
             decision: PolicyDecision.DENY,
             argsPattern: /something/,
+            modes: [ApprovalMode.DEFAULT],
           },
         ],
         expected: [],
@@ -2123,6 +2182,7 @@ describe('PolicyEngine', () => {
             toolName: 'dangerous-tool',
             decision: PolicyDecision.DENY,
             priority: 10,
+            modes: [ApprovalMode.YOLO],
           },
         ],
         approvalMode: ApprovalMode.YOLO,
@@ -2130,7 +2190,13 @@ describe('PolicyEngine', () => {
       },
       {
         name: 'should respect server wildcard DENY',
-        rules: [{ toolName: 'server__*', decision: PolicyDecision.DENY }],
+        rules: [
+          {
+            toolName: 'server__*',
+            decision: PolicyDecision.DENY,
+            modes: [ApprovalMode.DEFAULT],
+          },
+        ],
         expected: ['server__*'],
       },
       {
@@ -2140,15 +2206,44 @@ describe('PolicyEngine', () => {
             toolName: 'server__*',
             decision: PolicyDecision.DENY,
             priority: 100,
+            modes: [ApprovalMode.DEFAULT],
           },
           {
             toolName: 'server__tool1',
             decision: PolicyDecision.DENY,
             priority: 10,
+            modes: [ApprovalMode.DEFAULT],
           },
         ],
         expected: ['server__*', 'server__tool1'],
       },
+      {
+        name: 'should exclude run_shell_command but NOT write_file in simulated Plan Mode',
+        approvalMode: ApprovalMode.PLAN,
+        rules: [
+          {
+            // Simulates the high-priority allow for plans directory
+            toolName: 'write_file',
+            decision: PolicyDecision.ALLOW,
+            priority: 70,
+            argsPattern: /plans/,
+            modes: [ApprovalMode.PLAN],
+          },
+          {
+            // Simulates the global deny in Plan Mode
+            decision: PolicyDecision.DENY,
+            priority: 60,
+            modes: [ApprovalMode.PLAN],
+          },
+          {
+            // Simulates a tool from another policy (e.g. write.toml)
+            toolName: 'run_shell_command',
+            decision: PolicyDecision.ASK_USER,
+            priority: 10,
+          },
+        ],
+        expected: ['run_shell_command'],
+      },
       {
         name: 'should NOT exclude tool if covered by a higher priority wildcard ALLOW',
         rules: [
@@ -2156,11 +2251,13 @@ describe('PolicyEngine', () => {
             toolName: 'server__*',
             decision: PolicyDecision.ALLOW,
             priority: 100,
+            modes: [ApprovalMode.DEFAULT],
           },
           {
             toolName: 'server__tool1',
             decision: PolicyDecision.DENY,
             priority: 10,
+            modes: [ApprovalMode.DEFAULT],
           },
         ],
         expected: [],

packages/core/src/policy/policy-engine.ts

@@ -538,8 +538,10 @@ export class PolicyEngine {
     let globalVerdict: PolicyDecision | undefined;
 
     for (const rule of this.rules) {
-      // We only care about rules without args pattern for exclusion from the model
       if (rule.argsPattern) {
+        if (rule.toolName && rule.decision !== PolicyDecision.DENY) {
+          processedTools.add(rule.toolName);
+        }
         continue;
       }