Shell approval rework (#11073)

packages/core/src/tools/__snapshots__/shell.test.ts.snap

@@ -17,7 +17,7 @@ exports[`ShellTool > getDescription > should return the non-windows description
 `;
 
 exports[`ShellTool > getDescription > should return the windows description when on windows 1`] = `
-"This tool executes a given shell command as \`cmd.exe /c <command>\`. Command can start background processes using \`start /b\`.
+"This tool executes a given shell command as \`powershell.exe -NoProfile -Command <command>\`. Command can start background processes using PowerShell constructs such as \`Start-Process -NoNewWindow\` or \`Start-Job\`.
 
       The following information is returned:
 

packages/core/src/tools/shell.test.ts

@@ -9,6 +9,7 @@ import {
   describe,
   it,
   expect,
+  beforeAll,
   beforeEach,
   afterEach,
   type Mock,
@@ -23,7 +24,10 @@ vi.mock('os');
 vi.mock('crypto');
 vi.mock('../utils/summarizer.js');
 
-import { isCommandAllowed } from '../utils/shell-utils.js';
+import {
+  initializeShellParsers,
+  isCommandAllowed,
+} from '../utils/shell-utils.js';
 import { ShellTool } from './shell.js';
 import { type Config } from '../config/config.js';
 import {
@@ -41,6 +45,9 @@ import { ToolConfirmationOutcome } from './tools.js';
 import { OUTPUT_UPDATE_INTERVAL_MS } from './shell.js';
 import { createMockWorkspaceContext } from '../test-utils/mockWorkspaceContext.js';
 
+const originalComSpec = process.env['ComSpec'];
+const itWindowsOnly = process.platform === 'win32' ? it : it.skip;
+
 describe('ShellTool', () => {
   let shellTool: ShellTool;
   let mockConfig: Config;
@@ -71,6 +78,8 @@ describe('ShellTool', () => {
     (vi.mocked(crypto.randomBytes) as Mock).mockReturnValue(
       Buffer.from('abcdef', 'hex'),
     );
+    process.env['ComSpec'] =
+      'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe';
 
     // Capture the output callback to simulate streaming events from the service
     mockShellExecutionService.mockImplementation((_cmd, _cwd, callback) => {
@@ -84,23 +93,36 @@ describe('ShellTool', () => {
     });
   });
 
+  afterEach(() => {
+    if (originalComSpec === undefined) {
+      delete process.env['ComSpec'];
+    } else {
+      process.env['ComSpec'] = originalComSpec;
+    }
+  });
+
   describe('isCommandAllowed', () => {
     it('should allow a command if no restrictions are provided', () => {
       (mockConfig.getCoreTools as Mock).mockReturnValue(undefined);
       (mockConfig.getExcludeTools as Mock).mockReturnValue(undefined);
-      expect(isCommandAllowed('ls -l', mockConfig).allowed).toBe(true);
+      expect(isCommandAllowed('goodCommand --safe', mockConfig).allowed).toBe(
+        true,
+      );
     });
 
-    it('should block a command with command substitution using $()', () => {
-      expect(isCommandAllowed('echo $(rm -rf /)', mockConfig).allowed).toBe(
-        false,
+    it('should allow a command with command substitution using $()', () => {
+      const evaluation = isCommandAllowed(
+        'echo $(goodCommand --safe)',
+        mockConfig,
       );
+      expect(evaluation.allowed).toBe(true);
+      expect(evaluation.reason).toBeUndefined();
     });
   });
 
   describe('build', () => {
     it('should return an invocation for a valid command', () => {
-      const invocation = shellTool.build({ command: 'ls -l' });
+      const invocation = shellTool.build({ command: 'goodCommand --safe' });
       expect(invocation).toBeDefined();
     });
 
@@ -207,7 +229,7 @@ describe('ShellTool', () => {
       );
     });
 
-    it('should not wrap command on windows', async () => {
+    itWindowsOnly('should not wrap command on windows', async () => {
       vi.mocked(os.platform).mockReturnValue('win32');
       const invocation = shellTool.build({ command: 'dir' });
       const promise = invocation.execute(mockAbortSignal);
@@ -426,3 +448,6 @@ describe('ShellTool', () => {
     });
   });
 });
+beforeAll(async () => {
+  await initializeShellParsers();
+});

packages/core/src/tools/shell.ts

@@ -34,6 +34,7 @@ import { formatMemoryUsage } from '../utils/formatters.js';
 import type { AnsiOutput } from '../utils/terminalSerializer.js';
 import {
   getCommandRoots,
+  initializeShellParsers,
   isCommandAllowed,
   SHELL_TOOL_NAMES,
   stripShellWrapper,
@@ -388,25 +389,17 @@ function getShellToolDescription(): string {
       Process Group PGID: Process group started or \`(none)\``;
 
   if (os.platform() === 'win32') {
-    return `This tool executes a given shell command as \`cmd.exe /c <command>\`. Command can start background processes using \`start /b\`.${returnedInfo}`;
+    return `This tool executes a given shell command as \`powershell.exe -NoProfile -Command <command>\`. Command can start background processes using PowerShell constructs such as \`Start-Process -NoNewWindow\` or \`Start-Job\`.${returnedInfo}`;
   } else {
     return `This tool executes a given shell command as \`bash -c <command>\`. Command can start background processes using \`&\`. Command is executed as a subprocess that leads its own process group. Command process group can be terminated as \`kill -- -PGID\` or signaled as \`kill -s SIGNAL -- -PGID\`.${returnedInfo}`;
   }
 }
 
 function getCommandDescription(): string {
-  const cmd_substitution_warning =
-    '\n*** WARNING: Command substitution using $(), `` ` ``, <(), or >() is not allowed for security reasons.';
   if (os.platform() === 'win32') {
-    return (
-      'Exact command to execute as `cmd.exe /c <command>`' +
-      cmd_substitution_warning
-    );
+    return 'Exact command to execute as `powershell.exe -NoProfile -Command <command>`';
   } else {
-    return (
-      'Exact bash command to execute as `bash -c <command>`' +
-      cmd_substitution_warning
-    );
+    return 'Exact bash command to execute as `bash -c <command>`';
   }
 }
 
@@ -418,6 +411,7 @@ export class ShellTool extends BaseDeclarativeTool<
   private allowlist: Set<string> = new Set();
 
   constructor(private readonly config: Config) {
+    void initializeShellParsers();
     super(
       ShellTool.Name,
       'Shell',
@@ -451,6 +445,10 @@ export class ShellTool extends BaseDeclarativeTool<
   protected override validateToolParamValues(
     params: ShellToolParams,
   ): string | null {
+    if (!params.command.trim()) {
+      return 'Command cannot be empty.';
+    }
+
     const commandCheck = isCommandAllowed(params.command, this.config);
     if (!commandCheck.allowed) {
       if (!commandCheck.reason) {
@@ -461,9 +459,6 @@ export class ShellTool extends BaseDeclarativeTool<
       }
       return commandCheck.reason;
     }
-    if (!params.command.trim()) {
-      return 'Command cannot be empty.';
-    }
     if (getCommandRoots(params.command).length === 0) {
       return 'Could not identify command root to obtain permission from user.';
     }