From 69a3a42f4044a101b8525ede870dc7ad23950c0b Mon Sep 17 00:00:00 2001 From: "jinye.djy" Date: Wed, 29 Apr 2026 13:57:56 +0800 Subject: [PATCH] feat(core): add Monitor(...) permission namespace for monitor tool Introduce a dedicated Monitor(...) permission namespace so monitor and shell tools have independent permission boundaries. Previously monitor emitted Bash(...) rules, causing "Always Allow" to fail for future monitor invocations while unintentionally granting run_shell_command. Changes: - rule-parser.ts: add Monitor alias, SHELL_TOOL_NAMES entry, CANONICAL_TO_RULE_DISPLAY, DISPLAY_NAME_TO_VERB - permission-manager.ts: extract SHELL_LIKE_TOOLS set so evaluate(), evaluateSingle(), hasRelevantRules(), hasMatchingAskRule() handle both run_shell_command and monitor - monitor.ts: emit Monitor(...) instead of Bash(...) in permissionRules - Tests: parseRule, matchesRule, cross-tool isolation regression, buildPermissionRules, buildHumanReadableRuleLabel for Monitor Co-authored-by: Qwen-Coder --- .../permissions/permission-manager.test.ts | 131 ++++++++++++++++++ .../src/permissions/permission-manager.ts | 19 ++- packages/core/src/permissions/rule-parser.ts | 18 ++- packages/core/src/tools/monitor.test.ts | 10 +- packages/core/src/tools/monitor.ts | 6 +- 5 files changed, 169 insertions(+), 15 deletions(-) diff --git a/packages/core/src/permissions/permission-manager.test.ts b/packages/core/src/permissions/permission-manager.test.ts index 7196615ba2d..1a61fd93fdf 100644 --- a/packages/core/src/permissions/permission-manager.test.ts +++ b/packages/core/src/permissions/permission-manager.test.ts @@ -137,6 +137,11 @@ describe('parseRule', () => { expect(r.toolName).toBe('run_shell_command'); }); + it('parses Monitor alias', async () => { + const r = parseRule('Monitor'); + expect(r.toolName).toBe('monitor'); + }); + it('parses a shell tool with a specifier', async () => { const r = parseRule('Bash(git *)'); expect(r.toolName).toBe('run_shell_command'); @@ -144,6 +149,13 @@ describe('parseRule', () => { expect(r.specifierKind).toBe('command'); }); + it('parses Monitor with command specifier', async () => { + const r = parseRule('Monitor(tail -f *)'); + expect(r.toolName).toBe('monitor'); + expect(r.specifier).toBe('tail -f *'); + expect(r.specifierKind).toBe('command'); + }); + it('parses Read with path specifier', async () => { const r = parseRule('Read(./secrets/**)'); expect(r.toolName).toBe('read_file'); @@ -645,6 +657,28 @@ describe('matchesRule', () => { expect(matchesRule(rule, 'run_shell_command', 'echo hello')).toBe(false); }); + // Monitor command specifier + it('Monitor rule matches monitor invocations with command specifier', async () => { + const rule = parseRule('Monitor(tail -f *)'); + expect(matchesRule(rule, 'monitor')).toBe(false); // no command + expect(matchesRule(rule, 'monitor', 'tail -f /var/log/app.log')).toBe(true); + expect(matchesRule(rule, 'monitor', 'echo hello')).toBe(false); + }); + + it('Monitor rule does not match run_shell_command', async () => { + const rule = parseRule('Monitor(tail -f *)'); + expect( + matchesRule(rule, 'run_shell_command', 'tail -f /var/log/app.log'), + ).toBe(false); + }); + + it('Bash rule does not match monitor', async () => { + const rule = parseRule('Bash(tail -f *)'); + expect(matchesRule(rule, 'monitor', 'tail -f /var/log/app.log')).toBe( + false, + ); + }); + it('matchesRule checks individual simple commands (compound splitting is at PM level)', async () => { const rule = parseRule('Bash(git *)'); // matchesRule receives a simple command (already split by PM) @@ -933,6 +967,81 @@ describe('PermissionManager', () => { }); }); + describe('monitor command-level evaluation', () => { + it('Monitor(...) allow rule matches monitor invocations', async () => { + const pm2 = new PermissionManager( + makeConfig({ + permissionsAllow: ['Monitor(tail -f *)'], + }), + ); + pm2.initialize(); + expect( + await pm2.evaluate({ + toolName: 'monitor', + command: 'tail -f /var/log/app.log', + }), + ).toBe('allow'); + }); + + it('Monitor(...) deny rule blocks monitor invocations', async () => { + const pm2 = new PermissionManager( + makeConfig({ + permissionsDeny: ['Monitor(rm *)'], + }), + ); + pm2.initialize(); + expect( + await pm2.evaluate({ + toolName: 'monitor', + command: 'rm -rf /', + }), + ).toBe('deny'); + }); + + it('Monitor approval does NOT allow run_shell_command', async () => { + const pm2 = new PermissionManager( + makeConfig({ + permissionsAllow: ['Monitor(npm *)'], + }), + ); + pm2.initialize(); + // Same command via shell tool should NOT be allowed by Monitor rule + expect( + await pm2.evaluate({ + toolName: 'run_shell_command', + command: 'npm install', + }), + ).not.toBe('allow'); + }); + + it('Bash approval does NOT allow monitor', async () => { + const pm2 = new PermissionManager( + makeConfig({ + permissionsAllow: ['Bash(npm *)'], + }), + ); + pm2.initialize(); + // Same command via monitor tool should NOT be allowed by Bash rule + expect( + await pm2.evaluate({ + toolName: 'monitor', + command: 'npm install', + }), + ).not.toBe('allow'); + }); + + it('resolves default to allow for readonly monitor commands', async () => { + const pm2 = new PermissionManager(makeConfig({})); + pm2.initialize(); + expect( + await pm2.evaluate({ + toolName: 'monitor', + command: 'echo hello', + }), + ).toBe('allow'); + }); + }); + describe('compound command evaluation', () => { it('all sub-commands allowed → allow', async () => { pm = new PermissionManager( @@ -1691,6 +1800,19 @@ describe('buildPermissionRules', () => { const rules = buildPermissionRules({ toolName: 'run_shell_command' }); expect(rules).toEqual(['Bash']); }); + + it('generates Monitor rule with command specifier', async () => { + const rules = buildPermissionRules({ + toolName: 'monitor', + command: 'tail -f /var/log/app.log', + }); + expect(rules).toEqual(['Monitor(tail -f /var/log/app.log)']); + }); + + it('falls back to bare Monitor display name when no command', async () => { + const rules = buildPermissionRules({ toolName: 'monitor' }); + expect(rules).toEqual(['Monitor']); + }); }); describe('literal-specifier tools', () => { @@ -1741,6 +1863,10 @@ describe('buildHumanReadableRuleLabel', () => { expect(buildHumanReadableRuleLabel(['Bash'])).toBe('run commands'); }); + it('converts bare Monitor rule to "monitor commands"', () => { + expect(buildHumanReadableRuleLabel(['Monitor'])).toBe('monitor commands'); + }); + it('converts Read with absolute path specifier', () => { const label = buildHumanReadableRuleLabel(['Read(//Users/mochi/.qwen/**)']); expect(label).toBe('read files in /Users/mochi/.qwen/'); @@ -1761,6 +1887,11 @@ describe('buildHumanReadableRuleLabel', () => { expect(label).toBe("run 'git *' commands"); }); + it('converts Monitor with command specifier', () => { + const label = buildHumanReadableRuleLabel(['Monitor(tail -f *)']); + expect(label).toBe("monitor 'tail -f *' commands"); + }); + it('converts WebFetch with domain specifier', () => { const label = buildHumanReadableRuleLabel(['WebFetch(github.com)']); expect(label).toBe('fetch from github.com'); diff --git a/packages/core/src/permissions/permission-manager.ts b/packages/core/src/permissions/permission-manager.ts index f6e5c11ae62..1d6fdf2bf98 100644 --- a/packages/core/src/permissions/permission-manager.ts +++ b/packages/core/src/permissions/permission-manager.ts @@ -29,6 +29,13 @@ import type { const debugLogger = createDebugLogger('PERMISSIONS'); +/** + * Tools that spawn shell commands and share the same permission evaluation + * semantics: compound-command splitting, AST read-only analysis, and + * virtual file/network operation matching. + */ +const SHELL_LIKE_TOOLS = new Set(['run_shell_command', 'monitor']); + /** * Numeric priority for each PermissionDecision. * Higher number = more restrictive. Used to combine decisions by taking @@ -178,7 +185,7 @@ export class PermissionManager { // a concrete permission (deny/ask/allow) based on the command's readonly status. if ( decision === 'default' && - toolName === 'run_shell_command' && + SHELL_LIKE_TOOLS.has(toolName) && command !== undefined ) { return this.resolveDefaultPermission(command); @@ -256,7 +263,7 @@ export class PermissionManager { // must never downgrade an explicit 'allow' decision from a Bash rule. // Example: `git status` has no file ops; an allow rule for `Bash(git *)` // should return 'allow', not be downgraded to 'default'. - if (toolName === 'run_shell_command' && command !== undefined) { + if (SHELL_LIKE_TOOLS.has(toolName) && command !== undefined) { const cwd = pathCtx?.cwd ?? process.cwd(); const virtualDecision = this.evaluateShellVirtualOps( extractShellOperations(command, cwd), @@ -546,7 +553,7 @@ export class PermissionManager { hasRelevantRules(ctx: PermissionCheckContext): boolean { const { toolName, command, filePath, domain, specifier } = ctx; - if (ctx.toolName === 'run_shell_command' && command !== undefined) { + if (SHELL_LIKE_TOOLS.has(ctx.toolName) && command !== undefined) { const subCommands = splitCompoundCommand(command); if (subCommands.length > 1) { return subCommands.some((subCmd) => @@ -587,7 +594,7 @@ export class PermissionManager { // extracted from the command has a relevant rule. This ensures the PM is // consulted (and the confirmation dialog shown) when Read/Edit/etc. rules // would match equivalent shell commands. - if (ctx.toolName === 'run_shell_command' && ctx.command !== undefined) { + if (SHELL_LIKE_TOOLS.has(ctx.toolName) && ctx.command !== undefined) { const cwd = pathCtx?.cwd ?? process.cwd(); const ops = extractShellOperations(ctx.command, cwd); if ( @@ -622,7 +629,7 @@ export class PermissionManager { hasMatchingAskRule(ctx: PermissionCheckContext): boolean { const { toolName, command, filePath, domain, specifier } = ctx; - if (ctx.toolName === 'run_shell_command' && command !== undefined) { + if (SHELL_LIKE_TOOLS.has(ctx.toolName) && command !== undefined) { const subCommands = splitCompoundCommand(command); if (subCommands.length > 1) { return subCommands.some((subCmd) => @@ -654,7 +661,7 @@ export class PermissionManager { return true; } - if (ctx.toolName === 'run_shell_command' && ctx.command !== undefined) { + if (SHELL_LIKE_TOOLS.has(ctx.toolName) && ctx.command !== undefined) { const cwd = pathCtx?.cwd ?? process.cwd(); const ops = extractShellOperations(ctx.command, cwd); return ops.some((op) => { diff --git a/packages/core/src/permissions/rule-parser.ts b/packages/core/src/permissions/rule-parser.ts index b034069c57c..12951e6f477 100644 --- a/packages/core/src/permissions/rule-parser.ts +++ b/packages/core/src/permissions/rule-parser.ts @@ -118,6 +118,11 @@ export const TOOL_NAME_ALIASES: Readonly> = { Lsp: 'lsp', LspTool: 'lsp', + // Monitor tool + monitor: 'monitor', + Monitor: 'monitor', + MonitorTool: 'monitor', + // Legacy edit tool name replace: 'edit', }; @@ -125,7 +130,7 @@ export const TOOL_NAME_ALIASES: Readonly> = { /** * Shell tool canonical names. */ -const SHELL_TOOL_NAMES = new Set(['run_shell_command']); +const SHELL_TOOL_NAMES = new Set(['run_shell_command', 'monitor']); /** * File-reading tools — "Read" rules apply to all of these (best-effort). @@ -305,6 +310,8 @@ const CANONICAL_TO_RULE_DISPLAY: Readonly> = { write_file: 'Edit', // Shell run_shell_command: 'Bash', + // Monitor + monitor: 'Monitor', // Web web_fetch: 'WebFetch', // Agent / Skill @@ -419,6 +426,7 @@ const DISPLAY_NAME_TO_VERB: Readonly> = { Read: 'read files', Edit: 'edit files', Bash: 'run commands', + Monitor: 'monitor commands', WebFetch: 'fetch from', Agent: 'use agent', Skill: 'use skill', @@ -494,9 +502,13 @@ export function buildHumanReadableRuleLabel(rules: string[]): string { parts.push(`${verb} in ${cleanPath}`); break; } - case 'command': - parts.push(`run '${specifier}' commands`); + case 'command': { + const cmdVerb = DISPLAY_NAME_TO_VERB[displayName] ?? 'run'; + // Extract just the verb word (e.g. "run commands" → "run", "monitor commands" → "monitor") + const verbWord = cmdVerb.split(' ')[0]!; + parts.push(`${verbWord} '${specifier}' commands`); break; + } case 'domain': parts.push(`${verb} ${specifier}`); break; diff --git a/packages/core/src/tools/monitor.test.ts b/packages/core/src/tools/monitor.test.ts index 901e0d814e4..c6d1ffc1563 100644 --- a/packages/core/src/tools/monitor.test.ts +++ b/packages/core/src/tools/monitor.test.ts @@ -151,7 +151,7 @@ describe('MonitorTool', () => { }; expect(details.type).toBe('exec'); - expect(details.permissionRules).toEqual(['Bash(tail -f *)']); + expect(details.permissionRules).toEqual(['Monitor(tail -f *)']); }); it('does not consult Bash permission rules for monitor commands', async () => { @@ -183,8 +183,8 @@ describe('MonitorTool', () => { expect(pm.isCommandAllowed).not.toHaveBeenCalled(); // Both subcommands remain in confirmation scope expect(details.permissionRules).toEqual([ - 'Bash(git add *)', - 'Bash(git commit *)', + 'Monitor(git add *)', + 'Monitor(git commit *)', ]); }); @@ -201,7 +201,9 @@ describe('MonitorTool', () => { permissionRules?: string[]; }; - expect(details.permissionRules).toEqual(['Bash(python -c "print(1)")']); + expect(details.permissionRules).toEqual([ + 'Monitor(python -c "print(1)")', + ]); }); }); diff --git a/packages/core/src/tools/monitor.ts b/packages/core/src/tools/monitor.ts index 87a80d02130..1b4265dfab1 100644 --- a/packages/core/src/tools/monitor.ts +++ b/packages/core/src/tools/monitor.ts @@ -139,10 +139,12 @@ class MonitorToolInvocation extends BaseToolInvocation< const rules = await extractCommandRules(sub); allRules.push(...rules); } - permissionRules = [...new Set(allRules)].map((rule) => `Bash(${rule})`); + permissionRules = [...new Set(allRules)].map( + (rule) => `Monitor(${rule})`, + ); } catch (e) { debugLogger.warn('Failed to extract monitor command rules:', e); - permissionRules = [`Bash(${command})`]; + permissionRules = [`Monitor(${command})`]; } return {