Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/core/src/policy/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ export function getPolicyTier(
*/
export function formatPolicyError(error: PolicyFileError): string {
const tierLabel = error.tier.toUpperCase();
let message = `[${tierLabel}] Policy file error in ${error.fileName}:\n`;
const severityLabel = error.severity === 'warning' ? 'warning' : 'error';
let message = `[${tierLabel}] Policy file ${severityLabel} in ${error.fileName}:\n`;
message += ` ${error.message}`;
if (error.details) {
message += `\n${error.details}`;
Expand Down Expand Up @@ -293,7 +294,10 @@ export async function createPolicyEngineConfig(
// coreEvents has a buffer that will display these once the UI is ready
if (errors.length > 0) {
for (const error of errors) {
coreEvents.emitFeedback('error', formatPolicyError(error));
coreEvents.emitFeedback(
error.severity ?? 'error',
formatPolicyError(error),
);
}
}

Expand Down
286 changes: 277 additions & 9 deletions packages/core/src/policy/toml-loader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,26 @@ import * as fs from 'node:fs/promises';
import * as path from 'node:path';
import * as os from 'node:os';
import { fileURLToPath } from 'node:url';
import { loadPoliciesFromToml } from './toml-loader.js';
import {
loadPoliciesFromToml,
validateMcpPolicyToolNames,
} from './toml-loader.js';
import type { PolicyLoadResult } from './toml-loader.js';
import { PolicyEngine } from './policy-engine.js';

const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);

/** Returns only errors (severity !== 'warning') from a PolicyLoadResult. */
function getErrors(result: PolicyLoadResult): PolicyLoadResult['errors'] {
return result.errors.filter((e) => e.severity !== 'warning');
}

/** Returns only warnings (severity === 'warning') from a PolicyLoadResult. */
function getWarnings(result: PolicyLoadResult): PolicyLoadResult['errors'] {
return result.errors.filter((e) => e.severity === 'warning');
}

describe('policy-toml-loader', () => {
let tempDir: string;

Expand Down Expand Up @@ -189,7 +202,7 @@ priority = 100
'grep',
'read',
]);
expect(result.errors).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
});

it('should transform mcpName to composite toolName', async () => {
Expand Down Expand Up @@ -228,7 +241,7 @@ modes = ["yolo"]
expect(result.rules[0].modes).toEqual(['default', 'yolo']);
expect(result.rules[1].toolName).toBe('grep');
expect(result.rules[1].modes).toEqual(['yolo']);
expect(result.errors).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
});

it('should parse and transform allow_redirection property', async () => {
Expand Down Expand Up @@ -259,7 +272,7 @@ deny_message = "Deletion is permanent"
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);
expect(getErrors(result)).toHaveLength(0);
});

it('should support modes property for Tier 4 and Tier 5 policies', async () => {
Expand Down Expand Up @@ -547,8 +560,8 @@ commandRegex = ".*"
decision = "allow"
priority = 100
`);
expect(result.errors).toHaveLength(1);
const error = result.errors[0];
expect(getErrors(result)).toHaveLength(1);
const error = getErrors(result)[0];
expect(error.errorType).toBe('rule_validation');
expect(error.details).toContain('run_shell_command');
});
Expand Down Expand Up @@ -576,8 +589,8 @@ argsPattern = "([a-z)"
decision = "allow"
priority = 100
`);
expect(result.errors).toHaveLength(1);
const error = result.errors[0];
expect(getErrors(result)).toHaveLength(1);
const error = getErrors(result)[0];
expect(error.errorType).toBe('regex_compilation');
expect(error.message).toBe('Invalid regex pattern');
});
Expand All @@ -592,7 +605,7 @@ priority = 100
const getPolicyTier = (_dir: string) => 1;
const result = await loadPoliciesFromToml([filePath], getPolicyTier);

expect(result.errors).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
expect(result.rules).toHaveLength(1);
expect(result.rules[0].toolName).toBe('test-tool');
expect(result.rules[0].decision).toBe(PolicyDecision.ALLOW);
Expand All @@ -612,6 +625,177 @@ priority = 100
});
});

describe('Tool name validation', () => {
it('should warn for unrecognized tool names with suggestions', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
toolName = "grob"
decision = "allow"
priority = 100
`);

const warnings = getWarnings(result);
expect(warnings).toHaveLength(1);
expect(warnings[0].errorType).toBe('tool_name_warning');
expect(warnings[0].severity).toBe('warning');
expect(warnings[0].details).toContain('Unrecognized tool name "grob"');
expect(warnings[0].details).toContain('glob');
// Rules should still load despite warnings
expect(result.rules).toHaveLength(1);
expect(result.rules[0].toolName).toBe('grob');
});

it('should not warn for valid built-in tool names', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
toolName = "glob"
decision = "allow"
priority = 100

[[rule]]
toolName = "read_file"
decision = "allow"
priority = 100
`);

expect(getWarnings(result)).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
expect(result.rules).toHaveLength(2);
});

it('should not warn for wildcard "*"', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
toolName = "*"
decision = "allow"
priority = 100
`);

expect(getWarnings(result)).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
});

it('should not warn for MCP format tool names', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
toolName = "my-server__my-tool"
decision = "allow"
priority = 100

[[rule]]
toolName = "my-server__*"
decision = "allow"
priority = 100
`);

expect(getWarnings(result)).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
});

it('should not warn when mcpName is present (skips validation)', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
mcpName = "my-server"
toolName = "nonexistent"
decision = "allow"
priority = 100
`);

expect(getWarnings(result)).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
});

it('should not warn for legacy aliases', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
toolName = "search_file_content"
decision = "allow"
priority = 100
`);

expect(getWarnings(result)).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
});

it('should not warn for discovered tool prefix', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
toolName = "discovered_tool_my_custom_tool"
decision = "allow"
priority = 100
`);

expect(getWarnings(result)).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
});

it('should warn for each invalid name in a toolName array', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
toolName = ["grob", "glob", "replce"]
decision = "allow"
priority = 100
`);

const warnings = getWarnings(result);
expect(warnings).toHaveLength(2);
expect(warnings[0].details).toContain('"grob"');
expect(warnings[1].details).toContain('"replce"');
// All rules still load
expect(result.rules).toHaveLength(3);
});

it('should not warn for names far from any built-in (dynamic/agent tools)', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
toolName = "delegate_to_agent"
decision = "allow"
priority = 100

[[rule]]
toolName = "my_custom_tool"
decision = "allow"
priority = 100
`);

expect(getWarnings(result)).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
expect(result.rules).toHaveLength(2);
});

it('should not warn for catch-all rules (no toolName)', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
decision = "deny"
priority = 100
`);

expect(getWarnings(result)).toHaveLength(0);
expect(getErrors(result)).toHaveLength(0);
expect(result.rules).toHaveLength(1);
});

it('should still load rules even with warnings', async () => {
const result = await runLoadPoliciesFromToml(`
[[rule]]
toolName = "wrte_file"
decision = "deny"
priority = 50

[[rule]]
toolName = "glob"
decision = "allow"
priority = 100
`);

expect(getWarnings(result)).toHaveLength(1);
expect(getErrors(result)).toHaveLength(0);
expect(result.rules).toHaveLength(2);
expect(result.rules[0].toolName).toBe('wrte_file');
expect(result.rules[1].toolName).toBe('glob');
});
});

describe('Built-in Plan Mode Policy', () => {
it('should allow MCP tools with readOnlyHint annotation in Plan Mode (ASK_USER, not DENY)', async () => {
const planTomlPath = path.resolve(__dirname, 'policies', 'plan.toml');
Expand Down Expand Up @@ -779,4 +963,88 @@ priority = 100
}
});
});

describe('validateMcpPolicyToolNames', () => {
it('should warn for MCP tool names that are likely typos', () => {
const warnings = validateMcpPolicyToolNames(
'google-workspace',
['people.getMe', 'calendar.list', 'calendar.get'],
[
{
toolName: 'google-workspace__people.getxMe',
source: 'User: workspace.toml',
},
],
);

expect(warnings).toHaveLength(1);
expect(warnings[0]).toContain('people.getxMe');
expect(warnings[0]).toContain('google-workspace');
expect(warnings[0]).toContain('people.getMe');
});

it('should not warn for matching MCP tool names', () => {
const warnings = validateMcpPolicyToolNames(
'google-workspace',
['people.getMe', 'calendar.list'],
[
{ toolName: 'google-workspace__people.getMe' },
{ toolName: 'google-workspace__calendar.list' },
],
);

expect(warnings).toHaveLength(0);
});

it('should not warn for wildcard MCP rules', () => {
const warnings = validateMcpPolicyToolNames(
'my-server',
['tool1', 'tool2'],
[{ toolName: 'my-server__*' }],
);

expect(warnings).toHaveLength(0);
});

it('should not warn for rules targeting other servers', () => {
const warnings = validateMcpPolicyToolNames(
'server-a',
['tool1'],
[{ toolName: 'server-b__toolx' }],
);

expect(warnings).toHaveLength(0);
});

it('should not warn for tool names far from any discovered tool', () => {
const warnings = validateMcpPolicyToolNames(
'my-server',
['tool1', 'tool2'],
[{ toolName: 'my-server__completely_different_name' }],
);

expect(warnings).toHaveLength(0);
});

it('should skip rules without toolName', () => {
const warnings = validateMcpPolicyToolNames(
'my-server',
['tool1'],
[{ toolName: undefined }],
);

expect(warnings).toHaveLength(0);
});

it('should include source in warning when available', () => {
const warnings = validateMcpPolicyToolNames(
'my-server',
['tool1'],
[{ toolName: 'my-server__tol1', source: 'User: custom.toml' }],
);

expect(warnings).toHaveLength(1);
expect(warnings[0]).toContain('User: custom.toml');
});
});
});
Loading
Loading