diff --git a/docs/core/policy-engine.md b/docs/core/policy-engine.md index 30b9a0ac93..ee76c73874 100644 --- a/docs/core/policy-engine.md +++ b/docs/core/policy-engine.md @@ -210,6 +210,10 @@ decision = "ask_user" # The priority of the rule, from 0 to 999. priority = 10 +# (Optional) A custom message to display when a tool call is denied by this rule. +# This message is returned to the model and user, useful for explaining *why* it was denied. +deny_message = "Deletion is permanent" + # (Optional) An array of approval modes where this rule is active. modes = ["autoEdit"] ``` @@ -282,6 +286,7 @@ only the `mcpName`. mcpName = "untrusted-server" decision = "deny" priority = 500 +deny_message = "This server is not trusted by the admin." ``` ## Default policies diff --git a/packages/core/src/policy/toml-loader.test.ts b/packages/core/src/policy/toml-loader.test.ts index 5b26c6a4bb..da851cd369 100644 --- a/packages/core/src/policy/toml-loader.test.ts +++ b/packages/core/src/policy/toml-loader.test.ts @@ -173,6 +173,22 @@ allow_redirection = true expect(result.errors).toHaveLength(0); }); + it('should parse deny_message property', async () => { + const result = await runLoadPoliciesFromToml(` +[[rule]] +toolName = "rm" +decision = "deny" +priority = 100 +deny_message = "Deletion is permanent" +`); + + expect(result.rules).toHaveLength(1); + expect(result.rules[0].toolName).toBe('rm'); + expect(result.rules[0].decision).toBe(PolicyDecision.DENY); + expect(result.rules[0].denyMessage).toBe('Deletion is permanent'); + expect(result.errors).toHaveLength(0); + }); + it('should support modes property for Tier 2 and Tier 3 policies', async () => { await fs.writeFile( path.join(tempDir, 'tier2.toml'), diff --git a/packages/core/src/policy/toml-loader.ts b/packages/core/src/policy/toml-loader.ts index a895d01572..8e3d265a9a 100644 --- a/packages/core/src/policy/toml-loader.ts +++ b/packages/core/src/policy/toml-loader.ts @@ -46,6 +46,7 @@ const PolicyRuleSchema = z.object({ }), modes: z.array(z.nativeEnum(ApprovalMode)).optional(), allow_redirection: z.boolean().optional(), + deny_message: z.string().optional(), }); /** @@ -347,6 +348,7 @@ export async function loadPoliciesFromToml( modes: rule.modes, allowRedirection: rule.allow_redirection, source: `${tierName.charAt(0).toUpperCase() + tierName.slice(1)}: ${file}`, + denyMessage: rule.deny_message, }; // Compile regex pattern diff --git a/packages/core/src/policy/types.ts b/packages/core/src/policy/types.ts index 9cec21cfcc..db487a6ab3 100644 --- a/packages/core/src/policy/types.ts +++ b/packages/core/src/policy/types.ts @@ -142,6 +142,12 @@ export interface PolicyRule { * e.g. "my-policies.toml", "Settings (MCP Trusted)", etc. */ source?: string; + + /** + * Optional message to display when this rule results in a DENY decision. + * This message will be returned to the model/user. + */ + denyMessage?: string; } export interface SafetyCheckerRule { diff --git a/packages/core/src/scheduler/policy.test.ts b/packages/core/src/scheduler/policy.test.ts index 0e347d1c62..57703abe3c 100644 --- a/packages/core/src/scheduler/policy.test.ts +++ b/packages/core/src/scheduler/policy.test.ts @@ -36,8 +36,8 @@ describe('policy.ts', () => { tool: { name: 'test-tool' }, } as ValidatingToolCall; - const decision = await checkPolicy(toolCall, mockConfig); - expect(decision).toBe(PolicyDecision.ALLOW); + const result = await checkPolicy(toolCall, mockConfig); + expect(result.decision).toBe(PolicyDecision.ALLOW); expect(mockPolicyEngine.check).toHaveBeenCalledWith( { name: 'test-tool', args: {} }, undefined, @@ -102,8 +102,8 @@ describe('policy.ts', () => { tool: { name: 'test-tool' }, } as ValidatingToolCall; - const decision = await checkPolicy(toolCall, mockConfig); - expect(decision).toBe(PolicyDecision.DENY); + const result = await checkPolicy(toolCall, mockConfig); + expect(result.decision).toBe(PolicyDecision.DENY); }); it('should return ASK_USER without throwing in interactive mode', async () => { @@ -121,8 +121,8 @@ describe('policy.ts', () => { tool: { name: 'test-tool' }, } as ValidatingToolCall; - const decision = await checkPolicy(toolCall, mockConfig); - expect(decision).toBe(PolicyDecision.ASK_USER); + const result = await checkPolicy(toolCall, mockConfig); + expect(result.decision).toBe(PolicyDecision.ASK_USER); }); }); diff --git a/packages/core/src/scheduler/policy.ts b/packages/core/src/scheduler/policy.ts index 18e7a3f852..d28ca6dad6 100644 --- a/packages/core/src/scheduler/policy.ts +++ b/packages/core/src/scheduler/policy.ts @@ -4,7 +4,11 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { ApprovalMode, PolicyDecision } from '../policy/types.js'; +import { + ApprovalMode, + PolicyDecision, + type CheckResult, +} from '../policy/types.js'; import type { Config } from '../config/config.js'; import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { @@ -28,19 +32,25 @@ import type { ValidatingToolCall } from './types.js'; export async function checkPolicy( toolCall: ValidatingToolCall, config: Config, -): Promise { +): Promise { const serverName = toolCall.tool instanceof DiscoveredMCPTool ? toolCall.tool.serverName : undefined; - const { decision } = await config + const result = await config .getPolicyEngine() .check( { name: toolCall.request.name, args: toolCall.request.args }, serverName, ); + const { decision } = result; + + /* + * Return the full check result including the rule that matched. + * This is necessary to access metadata like custom deny messages. + */ if (decision === PolicyDecision.ASK_USER) { if (!config.isInteractive()) { throw new Error( @@ -51,7 +61,7 @@ export async function checkPolicy( } } - return decision; + return { decision, rule: result.rule }; } /** diff --git a/packages/core/src/scheduler/scheduler.test.ts b/packages/core/src/scheduler/scheduler.test.ts index 3456b5ec26..4ae3e84c8c 100644 --- a/packages/core/src/scheduler/scheduler.test.ts +++ b/packages/core/src/scheduler/scheduler.test.ts @@ -194,6 +194,10 @@ describe('Scheduler (Orchestrator)', () => { vi.mocked(resolveConfirmation).mockReset(); vi.mocked(checkPolicy).mockReset(); + vi.mocked(checkPolicy).mockResolvedValue({ + decision: PolicyDecision.ALLOW, + rule: undefined, + }); vi.mocked(updatePolicy).mockReset(); mockExecutor = { @@ -663,7 +667,10 @@ describe('Scheduler (Orchestrator)', () => { }); it('should update state to error with POLICY_VIOLATION if Policy returns DENY', async () => { - vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.DENY); + vi.mocked(checkPolicy).mockResolvedValue({ + decision: PolicyDecision.DENY, + rule: undefined, + }); await scheduler.schedule(req1, signal); @@ -678,6 +685,36 @@ describe('Scheduler (Orchestrator)', () => { expect(mockExecutor.execute).not.toHaveBeenCalled(); }); + it('should include denyMessage in error response if present', async () => { + vi.mocked(checkPolicy).mockResolvedValue({ + decision: PolicyDecision.DENY, + rule: { + decision: PolicyDecision.DENY, + denyMessage: 'Custom denial reason', + }, + }); + + await scheduler.schedule(req1, signal); + + expect(mockStateManager.updateStatus).toHaveBeenCalledWith( + 'call-1', + 'error', + expect.objectContaining({ + errorType: ToolErrorType.POLICY_VIOLATION, + responseParts: expect.arrayContaining([ + expect.objectContaining({ + functionResponse: expect.objectContaining({ + response: { + error: + 'Tool execution denied by policy. Custom denial reason', + }, + }), + }), + ]), + }), + ); + }); + it('should handle errors from checkPolicy (e.g. non-interactive ASK_USER)', async () => { const error = new Error('Not interactive'); vi.mocked(checkPolicy).mockRejectedValue(error); @@ -701,7 +738,10 @@ describe('Scheduler (Orchestrator)', () => { }); it('should bypass confirmation and ProceedOnce if Policy returns ALLOW (YOLO/AllowedTools)', async () => { - vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.ALLOW); + vi.mocked(checkPolicy).mockResolvedValue({ + decision: PolicyDecision.ALLOW, + rule: undefined, + }); // Provide a mock execute to finish the loop mockExecutor.execute.mockResolvedValue({ @@ -754,8 +794,14 @@ describe('Scheduler (Orchestrator)', () => { // First call requires confirmation, second is auto-approved (simulating policy update) vi.mocked(checkPolicy) - .mockResolvedValueOnce(PolicyDecision.ASK_USER) - .mockResolvedValueOnce(PolicyDecision.ALLOW); + .mockResolvedValueOnce({ + decision: PolicyDecision.ASK_USER, + rule: undefined, + }) + .mockResolvedValueOnce({ + decision: PolicyDecision.ALLOW, + rule: undefined, + }); vi.mocked(resolveConfirmation).mockResolvedValue({ outcome: ToolConfirmationOutcome.ProceedAlways, @@ -777,7 +823,10 @@ describe('Scheduler (Orchestrator)', () => { }); it('should call resolveConfirmation and updatePolicy when ASK_USER', async () => { - vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.ASK_USER); + vi.mocked(checkPolicy).mockResolvedValue({ + decision: PolicyDecision.ASK_USER, + rule: undefined, + }); const resolution = { outcome: ToolConfirmationOutcome.ProceedAlways, @@ -820,7 +869,10 @@ describe('Scheduler (Orchestrator)', () => { }); it('should cancel and NOT execute if resolveConfirmation returns Cancel', async () => { - vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.ASK_USER); + vi.mocked(checkPolicy).mockResolvedValue({ + decision: PolicyDecision.ASK_USER, + rule: undefined, + }); const resolution = { outcome: ToolConfirmationOutcome.Cancel, @@ -842,7 +894,10 @@ describe('Scheduler (Orchestrator)', () => { }); it('should mark as cancelled (not errored) when abort happens during confirmation error', async () => { - vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.ASK_USER); + vi.mocked(checkPolicy).mockResolvedValue({ + decision: PolicyDecision.ASK_USER, + rule: undefined, + }); // Simulate shouldConfirmExecute logic throwing while aborted vi.mocked(resolveConfirmation).mockImplementation(async () => { @@ -865,7 +920,10 @@ describe('Scheduler (Orchestrator)', () => { }); it('should preserve confirmation details (e.g. diff) in cancelled state', async () => { - vi.mocked(checkPolicy).mockResolvedValue(PolicyDecision.ASK_USER); + vi.mocked(checkPolicy).mockResolvedValue({ + decision: PolicyDecision.ASK_USER, + rule: undefined, + }); const confirmDetails = { type: 'edit' as const, diff --git a/packages/core/src/scheduler/scheduler.ts b/packages/core/src/scheduler/scheduler.ts index fc159633d9..0589c50a72 100644 --- a/packages/core/src/scheduler/scheduler.ts +++ b/packages/core/src/scheduler/scheduler.ts @@ -404,15 +404,16 @@ export class Scheduler { const callId = toolCall.request.callId; // Policy & Security - const decision = await checkPolicy(toolCall, this.config); + const { decision, rule } = await checkPolicy(toolCall, this.config); if (decision === PolicyDecision.DENY) { + const denyMessage = rule?.denyMessage ? ` ${rule.denyMessage}` : ''; this.state.updateStatus( callId, 'error', createErrorResponse( toolCall.request, - new Error('Tool execution denied by policy.'), + new Error(`Tool execution denied by policy.${denyMessage}`), ToolErrorType.POLICY_VIOLATION, ), );