Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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 @@ -140,7 +140,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 @@ -226,7 +227,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 @@ -143,7 +156,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 @@ -182,7 +195,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 @@ -213,7 +226,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 2 and Tier 3 policies', async () => {
Expand Down Expand Up @@ -497,8 +510,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 @@ -526,8 +539,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 @@ -542,7 +555,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 @@ -562,6 +575,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 override default subagent rules when in Plan Mode', async () => {
const planTomlPath = path.resolve(__dirname, 'policies', 'plan.toml');
Expand Down Expand Up @@ -617,4 +801,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