Reapply "Shell approval rework" (#11143)

This reverts commit bd5c158.

Author: cornmander

docs/cli/commands.md

@@ -292,8 +292,9 @@ Gemini CLI.
 
 - **`!<shell_command>`**
   - **Description:** Execute the given `<shell_command>` using `bash` on
-    Linux/macOS or `cmd.exe` on Windows. Any output or errors from the command
-    are displayed in the terminal.
+    Linux/macOS or `powershell.exe -NoProfile -Command` on Windows (unless you
+    override `ComSpec`). Any output or errors from the command are displayed in
+    the terminal.
   - **Examples:**
     - `!ls -la` (executes `ls -la` and returns to Gemini CLI)
     - `!git status` (executes `git status` and returns to Gemini CLI)

docs/tools/shell.md

@@ -10,8 +10,9 @@ command, including interactive commands that require user input (e.g., `vim`,
 `git rebase -i`) if the `tools.shell.enableInteractiveShell` setting is set to
 `true`.
 
-On Windows, commands are executed with `cmd.exe /c`. On other platforms, they
-are executed with `bash -c`.
+On Windows, commands are executed with `powershell.exe -NoProfile -Command`
+(unless you explicitly point `ComSpec` at another shell). On other platforms,
+they are executed with `bash -c`.
 
 ### Arguments
 

integration-tests/flicker.test.ts

@@ -8,7 +8,8 @@ import { describe, it, expect } from 'vitest';
 import { TestRig } from './test-helper.js';
 
 describe('Flicker Detector', () => {
-  it('should not detect a flicker under the max height budget', async () => {
+  // TODO: https://github.com/google-gemini/gemini-cli/issues/11170
+  it.skip('should not detect a flicker under the max height budget', async () => {
     const rig = new TestRig();
     await rig.setup('flicker-detector-test');
 

integration-tests/run_shell_command.test.ts

@@ -25,6 +25,46 @@ function getLineCountCommand(): { command: string; tool: string } {
   }
 }
 
+function getInvalidCommand(): string {
+  switch (shell) {
+    case 'powershell':
+      return `Get-ChildItem | | Select-Object`;
+    case 'cmd':
+      return `dir | | findstr foo`;
+    case 'bash':
+    default:
+      return `echo "hello" > > file`;
+  }
+}
+
+function getAllowedListCommand(): string {
+  switch (shell) {
+    case 'powershell':
+      return 'Get-ChildItem';
+    case 'cmd':
+      return 'dir';
+    case 'bash':
+    default:
+      return 'ls';
+  }
+}
+
+function getDisallowedFileReadCommand(testFile: string): {
+  command: string;
+  tool: string;
+} {
+  const quotedPath = `"${testFile}"`;
+  switch (shell) {
+    case 'powershell':
+      return { command: `Get-Content ${quotedPath}`, tool: 'Get-Content' };
+    case 'cmd':
+      return { command: `type ${quotedPath}`, tool: 'type' };
+    case 'bash':
+    default:
+      return { command: `cat ${quotedPath}`, tool: 'cat' };
+  }
+}
+
 describe('run_shell_command', () => {
   it('should be able to run a shell command', async () => {
     const rig = new TestRig();
@@ -106,8 +146,17 @@ describe('run_shell_command', () => {
     const foundToolCall = await rig.waitForToolCall('run_shell_command', 15000);
 
     if (!foundToolCall) {
+      const toolLogs = rig.readToolLogs().map(({ toolRequest }) => ({
+        name: toolRequest.name,
+        success: toolRequest.success,
+        args: toolRequest.args,
+      }));
       printDebugInfo(rig, result, {
         'Found tool call': foundToolCall,
+        'Allowed tools flag': `run_shell_command(${tool})`,
+        Prompt: prompt,
+        'Tool logs': toolLogs,
+        Result: result,
       });
     }
 
@@ -214,8 +263,17 @@ describe('run_shell_command', () => {
     const foundToolCall = await rig.waitForToolCall('run_shell_command', 15000);
 
     if (!foundToolCall) {
+      const toolLogs = rig.readToolLogs().map(({ toolRequest }) => ({
+        name: toolRequest.name,
+        success: toolRequest.success,
+        args: toolRequest.args,
+      }));
       printDebugInfo(rig, result, {
         'Found tool call': foundToolCall,
+        'Allowed tools flag': `ShellTool(${tool})`,
+        Prompt: prompt,
+        'Tool logs': toolLogs,
+        Result: result,
       });
     }
 
@@ -284,6 +342,73 @@ describe('run_shell_command', () => {
     }
   });
 
+  it('should reject commands not on the allowlist', async () => {
+    const rig = new TestRig();
+    await rig.setup('should reject commands not on the allowlist');
+
+    const testFile = rig.createFile('test.txt', 'Disallowed command check\n');
+    const allowedCommand = getAllowedListCommand();
+    const disallowed = getDisallowedFileReadCommand(testFile);
+    const prompt =
+      `I am testing the allowed tools configuration. ` +
+      `Attempt to run "${disallowed.command}" to read the contents of ${testFile}. ` +
+      `If the command fails because it is not permitted, respond with the single word FAIL. ` +
+      `If it succeeds, respond with SUCCESS.`;
+
+    const result = await rig.run(
+      {
+        stdin: prompt,
+        yolo: false,
+      },
+      `--allowed-tools=run_shell_command(${allowedCommand})`,
+    );
+
+    if (!result.toLowerCase().includes('fail')) {
+      printDebugInfo(rig, result, {
+        Result: result,
+        AllowedCommand: allowedCommand,
+        DisallowedCommand: disallowed.command,
+      });
+    }
+    expect(result).toContain('FAIL');
+
+    const foundToolCall = await rig.waitForToolCall(
+      'run_shell_command',
+      15000,
+      (args) => args.toLowerCase().includes(disallowed.tool.toLowerCase()),
+    );
+
+    if (!foundToolCall) {
+      printDebugInfo(rig, result, {
+        'Found tool call': foundToolCall,
+        ToolLogs: rig.readToolLogs(),
+      });
+    }
+    expect(foundToolCall).toBe(true);
+
+    const toolLogs = rig
+      .readToolLogs()
+      .filter((toolLog) => toolLog.toolRequest.name === 'run_shell_command');
+    const failureLog = toolLogs.find((toolLog) =>
+      toolLog.toolRequest.args
+        .toLowerCase()
+        .includes(disallowed.tool.toLowerCase()),
+    );
+
+    if (!failureLog || failureLog.toolRequest.success) {
+      printDebugInfo(rig, result, {
+        ToolLogs: toolLogs,
+        DisallowedTool: disallowed.tool,
+      });
+    }
+
+    expect(
+      failureLog,
+      'Expected failing run_shell_command invocation',
+    ).toBeTruthy();
+    expect(failureLog!.toolRequest.success).toBe(false);
+  });
+
   it('should allow all with "ShellTool" and other specific tools', async () => {
     const rig = new TestRig();
     await rig.setup(
@@ -390,4 +515,53 @@ describe('run_shell_command', () => {
     validateModelOutput(result, fileName, 'Platform-specific listing test');
     expect(result).toContain(fileName);
   });
+
+  it('rejects invalid shell expressions', async () => {
+    const rig = new TestRig();
+    await rig.setup('rejects invalid shell expressions');
+    const invalidCommand = getInvalidCommand();
+    const result = await rig.run(
+      `I am testing the error handling of the run_shell_command tool. Please attempt to run the following command, which I know has invalid syntax: \`${invalidCommand}\`. If the command fails as expected, please return the word FAIL, otherwise return the word SUCCESS.`,
+    );
+    expect(result).toContain('FAIL');
+
+    const escapedInvalidCommand = JSON.stringify(invalidCommand).slice(1, -1);
+    const foundToolCall = await rig.waitForToolCall(
+      'run_shell_command',
+      15000,
+      (args) =>
+        args.toLowerCase().includes(escapedInvalidCommand.toLowerCase()),
+    );
+
+    if (!foundToolCall) {
+      printDebugInfo(rig, result, {
+        'Found tool call': foundToolCall,
+        EscapedCommand: escapedInvalidCommand,
+        ToolLogs: rig.readToolLogs(),
+      });
+    }
+    expect(foundToolCall).toBe(true);
+
+    const toolLogs = rig
+      .readToolLogs()
+      .filter((toolLog) => toolLog.toolRequest.name === 'run_shell_command');
+    const failureLog = toolLogs.find((toolLog) =>
+      toolLog.toolRequest.args
+        .toLowerCase()
+        .includes(escapedInvalidCommand.toLowerCase()),
+    );
+
+    if (!failureLog || failureLog.toolRequest.success) {
+      printDebugInfo(rig, result, {
+        ToolLogs: toolLogs,
+        EscapedCommand: escapedInvalidCommand,
+      });
+    }
+
+    expect(
+      failureLog,
+      'Expected failing run_shell_command invocation for invalid syntax',
+    ).toBeTruthy();
+    expect(failureLog!.toolRequest.success).toBe(false);
+  });
 });

package-lock.json

@@ -9,27 +9,24 @@ import { ConfirmationRequiredError, ShellProcessor } from './shellProcessor.js';
 import { createMockCommandContext } from '../../test-utils/mockCommandContext.js';
 import type { CommandContext } from '../../ui/commands/types.js';
 import type { Config } from '@google/gemini-cli-core';
-import { ApprovalMode } from '@google/gemini-cli-core';
-import os from 'node:os';
+import { ApprovalMode, getShellConfiguration } from '@google/gemini-cli-core';
 import { quote } from 'shell-quote';
 import { createPartFromText } from '@google/genai';
 import type { PromptPipelineContent } from './types.js';
 
 // Helper function to determine the expected escaped string based on the current OS,
 // mirroring the logic in the actual `escapeShellArg` implementation.
 function getExpectedEscapedArgForPlatform(arg: string): string {
-  if (os.platform() === 'win32') {
-    const comSpec = (process.env['ComSpec'] || 'cmd.exe').toLowerCase();
-    const isPowerShell =
-      comSpec.endsWith('powershell.exe') || comSpec.endsWith('pwsh.exe');
+  const { shell } = getShellConfiguration();
 
-    if (isPowerShell) {
+  switch (shell) {
+    case 'powershell':
       return `'${arg.replace(/'/g, "''")}'`;
-    } else {
+    case 'cmd':
       return `"${arg.replace(/"/g, '""')}"`;
-    }
-  } else {
-    return quote([arg]);
+    case 'bash':
+    default:
+      return quote([arg]);
   }
 }
 

packages/cli/src/services/prompt-processors/shellProcessor.test.ts

@@ -20,10 +20,10 @@
     "dist"
   ],
   "dependencies": {
-    "@google/genai": "1.16.0",
+    "@google-cloud/logging": "^11.2.1",
     "@google-cloud/opentelemetry-cloud-monitoring-exporter": "^0.21.0",
     "@google-cloud/opentelemetry-cloud-trace-exporter": "^3.0.0",
-    "@google-cloud/logging": "^11.2.1",
+    "@google/genai": "1.16.0",
     "@joshua.litt/get-ripgrep": "^0.0.2",
     "@modelcontextprotocol/sdk": "^1.11.0",
     "@opentelemetry/api": "^1.9.0",
@@ -61,7 +61,9 @@
     "shell-quote": "^1.8.3",
     "simple-git": "^3.28.0",
     "strip-ansi": "^7.1.0",
+    "tree-sitter-bash": "^0.25.0",
     "undici": "^7.10.0",
+    "web-tree-sitter": "^0.25.10",
     "ws": "^8.18.0"
   },
   "optionalDependencies": {