fix(core): enhance sandbox usability and fix build error (#24460)

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

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

@@ -16,6 +16,7 @@ import {
 } from 'vitest';
 
 const mockPlatform = vi.hoisted(() => vi.fn());
+const mockHomedir = vi.hoisted(() => vi.fn());
 
 const mockShellExecutionService = vi.hoisted(() => vi.fn());
 const mockShellBackground = vi.hoisted(() => vi.fn());
@@ -34,8 +35,10 @@ vi.mock('node:os', async (importOriginal) => {
     default: {
       ...actualOs,
       platform: mockPlatform,
+      homedir: mockHomedir,
     },
     platform: mockPlatform,
+    homedir: mockHomedir,
   };
 });
 vi.mock('crypto');
@@ -57,7 +60,11 @@ import { isSubpath } from '../utils/paths.js';
 import * as crypto from 'node:crypto';
 import * as summarizer from '../utils/summarizer.js';
 import { ToolErrorType } from './tool-error.js';
-import { ToolConfirmationOutcome } from './tools.js';
+import {
+  ToolConfirmationOutcome,
+  type ToolSandboxExpansionConfirmationDetails,
+  type ToolExecuteConfirmationDetails,
+} from './tools.js';
 import { SHELL_TOOL_NAME } from './tool-names.js';
 import { WorkspaceContext } from '../utils/workspaceContext.js';
 import {
@@ -69,6 +76,7 @@ import {
   type UpdatePolicy,
 } from '../confirmation-bus/types.js';
 import { type MessageBus } from '../confirmation-bus/message-bus.js';
+import { type SandboxManager } from '../services/sandboxManager.js';
 
 interface TestableMockMessageBus extends MessageBus {
   defaultToolDecision: 'allow' | 'deny' | 'ask_user';
@@ -84,6 +92,7 @@ describe('ShellTool', () => {
 
   let shellTool: ShellTool;
   let mockConfig: Config;
+  let mockSandboxManager: SandboxManager;
   let mockShellOutputCallback: (event: ShellOutputEvent) => void;
   let resolveExecutionPromise: (result: ShellExecutionResult) => void;
   let tempRootDir: string;
@@ -94,6 +103,7 @@ describe('ShellTool', () => {
     tempRootDir = fs.mkdtempSync(path.join(os.tmpdir(), 'shell-test-'));
     fs.mkdirSync(path.join(tempRootDir, 'subdir'));
 
+    mockSandboxManager = new NoopSandboxManager();
     mockConfig = {
       get config() {
         return this;
@@ -140,7 +150,15 @@ describe('ShellTool', () => {
       getEnableShellOutputEfficiency: vi.fn().mockReturnValue(true),
       getSandboxEnabled: vi.fn().mockReturnValue(false),
       sanitizationConfig: {},
-      sandboxManager: new NoopSandboxManager(),
+      get sandboxManager() {
+        return mockSandboxManager;
+      },
+      sandboxPolicyManager: {
+        getCommandPermissions: vi.fn().mockReturnValue(undefined),
+        getModeConfig: vi.fn().mockReturnValue({ readonly: false }),
+        addPersistentApproval: vi.fn(),
+        addSessionApproval: vi.fn(),
+      },
     } as unknown as Config;
 
     const bus = createMockMessageBus();
@@ -168,6 +186,7 @@ describe('ShellTool', () => {
     shellTool = new ShellTool(mockConfig, bus);
 
     mockPlatform.mockReturnValue('linux');
+    mockHomedir.mockReturnValue('/home/user');
     (vi.mocked(crypto.randomBytes) as Mock).mockReturnValue(
       Buffer.from('abcdef', 'hex'),
     );
@@ -646,7 +665,7 @@ describe('ShellTool', () => {
 
   describe('shouldConfirmExecute', () => {
     it('should request confirmation for a new command and allowlist it on "Always"', async () => {
-      const params = { command: 'npm install' };
+      const params = { command: 'ls -la' };
       const invocation = shellTool.build(params);
 
       // Accessing protected messageBus for testing purposes
@@ -920,6 +939,152 @@ describe('ShellTool', () => {
     });
   });
 
+  describe('sandbox heuristics', () => {
+    const mockAbortSignal = new AbortController().signal;
+
+    it('should suggest proactive permissions for npm commands', async () => {
+      const homeDir = path.join(tempRootDir, 'home');
+      fs.mkdirSync(homeDir);
+      fs.mkdirSync(path.join(homeDir, '.npm'));
+      fs.mkdirSync(path.join(homeDir, '.cache'));
+
+      mockHomedir.mockReturnValue(homeDir);
+
+      const sandboxManager = {
+        parseDenials: vi.fn().mockReturnValue({
+          network: true,
+          filePaths: [path.join(homeDir, '.npm/_logs/test.log')],
+        }),
+        prepareCommand: vi.fn(),
+        isKnownSafeCommand: vi.fn(),
+        isDangerousCommand: vi.fn(),
+      } as unknown as SandboxManager;
+      mockSandboxManager = sandboxManager;
+
+      const invocation = shellTool.build({ command: 'npm install' });
+      const promise = invocation.execute(mockAbortSignal);
+
+      resolveExecutionPromise({
+        exitCode: 1,
+        output: 'npm error code EPERM',
+        executionMethod: 'child_process',
+        signal: null,
+        error: null,
+        aborted: false,
+        pid: 12345,
+        rawOutput: Buffer.from('npm error code EPERM'),
+      });
+
+      const result = await promise;
+
+      expect(result.error?.type).toBe(ToolErrorType.SANDBOX_EXPANSION_REQUIRED);
+      const details = JSON.parse(result.error!.message);
+      expect(details.additionalPermissions.network).toBe(true);
+      expect(details.additionalPermissions.fileSystem.read).toContain(
+        path.join(homeDir, '.npm'),
+      );
+      expect(details.additionalPermissions.fileSystem.read).toContain(
+        path.join(homeDir, '.cache'),
+      );
+      expect(details.additionalPermissions.fileSystem.write).toContain(
+        path.join(homeDir, '.npm'),
+      );
+    });
+
+    it('should NOT consolidate paths into sensitive directories', async () => {
+      const rootDir = path.join(tempRootDir, 'fake_root');
+      const homeDir = path.join(rootDir, 'home');
+      const user1Dir = path.join(homeDir, 'user1');
+      const user2Dir = path.join(homeDir, 'user2');
+      const user3Dir = path.join(homeDir, 'user3');
+      fs.mkdirSync(homeDir, { recursive: true });
+      fs.mkdirSync(user1Dir);
+      fs.mkdirSync(user2Dir);
+      fs.mkdirSync(user3Dir);
+
+      mockHomedir.mockReturnValue(path.join(homeDir, 'user'));
+
+      vi.spyOn(mockConfig, 'isPathAllowed').mockImplementation((p) => {
+        if (p.includes('fake_root')) return false;
+        return true;
+      });
+
+      const sandboxManager = {
+        parseDenials: vi.fn().mockReturnValue({
+          network: false,
+          filePaths: [
+            path.join(user1Dir, 'file1'),
+            path.join(user2Dir, 'file2'),
+            path.join(user3Dir, 'file3'),
+          ],
+        }),
+        prepareCommand: vi.fn(),
+        isKnownSafeCommand: vi.fn(),
+        isDangerousCommand: vi.fn(),
+      } as unknown as SandboxManager;
+      mockSandboxManager = sandboxManager;
+
+      const invocation = shellTool.build({ command: `ls ${homeDir}` });
+      const promise = invocation.execute(mockAbortSignal);
+
+      resolveExecutionPromise({
+        exitCode: 1,
+        output: 'Permission denied',
+        executionMethod: 'child_process',
+        signal: null,
+        error: null,
+        aborted: false,
+        pid: 12345,
+        rawOutput: Buffer.from('Permission denied'),
+      });
+
+      const result = await promise;
+
+      expect(result.error?.type).toBe(ToolErrorType.SANDBOX_EXPANSION_REQUIRED);
+      const details = JSON.parse(result.error!.message);
+
+      // Should NOT contain homeDir as it is a parent of homedir and thus sensitive
+      expect(details.additionalPermissions.fileSystem.read).not.toContain(
+        homeDir,
+      );
+      // Should contain individual paths instead
+      expect(details.additionalPermissions.fileSystem.read).toContain(user1Dir);
+      expect(details.additionalPermissions.fileSystem.read).toContain(user2Dir);
+      expect(details.additionalPermissions.fileSystem.read).toContain(user3Dir);
+    });
+
+    it('should proactively suggest expansion for npm install in confirmation', async () => {
+      const homeDir = path.join(tempRootDir, 'home');
+      fs.mkdirSync(homeDir);
+      mockHomedir.mockReturnValue(homeDir);
+
+      const invocation = shellTool.build({ command: 'npm install' });
+      const details = (await invocation.shouldConfirmExecute(
+        new AbortController().signal,
+        'ask_user',
+      )) as ToolSandboxExpansionConfirmationDetails;
+
+      expect(details.type).toBe('sandbox_expansion');
+      expect(details.title).toContain('Recommended');
+      expect(details.additionalPermissions.network).toBe(true);
+    });
+
+    it('should NOT proactively suggest expansion for npm test', async () => {
+      const homeDir = path.join(tempRootDir, 'home');
+      fs.mkdirSync(homeDir);
+      mockHomedir.mockReturnValue(homeDir);
+
+      const invocation = shellTool.build({ command: 'npm test' });
+      const details = (await invocation.shouldConfirmExecute(
+        new AbortController().signal,
+        'ask_user',
+      )) as ToolExecuteConfirmationDetails;
+
+      // Should be regular exec confirmation, not expansion
+      expect(details.type).toBe('exec');
+    });
+  });
+
   describe('getSchema', () => {
     it('should return the base schema when no modelId is provided', () => {
       const schema = shellTool.getSchema();

packages/core/src/tools/shell.ts

@@ -25,6 +25,7 @@ import {
   type PolicyUpdateOptions,
   type ToolLiveOutput,
   type ExecuteOptions,
+  type ForcedToolDecision,
 } from './tools.js';
 
 import { getErrorMessage } from '../utils/errors.js';
@@ -48,6 +49,11 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js';
 import { getShellDefinition } from './definitions/coreTools.js';
 import { resolveToolDeclaration } from './definitions/resolver.js';
 import type { AgentLoopContext } from '../config/agent-loop-context.js';
+import { isSubpath } from '../utils/paths.js';
+import {
+  getProactiveToolSuggestions,
+  isNetworkReliantCommand,
+} from '../sandbox/utils/proactivePermissions.js';
 
 export const OUTPUT_UPDATE_INTERVAL_MS = 1000;
 
@@ -66,6 +72,8 @@ export class ShellToolInvocation extends BaseToolInvocation<
   ShellToolParams,
   ToolResult
 > {
+  private proactivePermissionsConfirmed?: SandboxPermissions;
+
   constructor(
     private readonly context: AgentLoopContext,
     params: ShellToolParams,
@@ -126,6 +134,83 @@ export class ShellToolInvocation extends BaseToolInvocation<
     return `${this.params.command} ${this.getContextualDetails()}`;
   }
 
+  private simplifyPaths(paths: Set<string>): string[] {
+    if (paths.size === 0) return [];
+    const rawPaths = Array.from(paths);
+
+    // 1. Remove redundant paths (subpaths of already included paths)
+    const sorted = rawPaths.sort((a, b) => a.length - b.length);
+    const nonRedundant: string[] = [];
+    for (const p of sorted) {
+      if (!nonRedundant.some((s) => isSubpath(s, p))) {
+        nonRedundant.push(p);
+      }
+    }
+
+    // 2. Consolidate clusters: if >= 3 paths share the same immediate parent, use the parent
+    const parentCounts = new Map<string, string[]>();
+    for (const p of nonRedundant) {
+      const parent = path.dirname(p);
+      if (!parentCounts.has(parent)) {
+        parentCounts.set(parent, []);
+      }
+      parentCounts.get(parent)!.push(p);
+    }
+
+    const finalPaths = new Set<string>();
+
+    const sensitiveDirs = new Set([
+      os.homedir(),
+      path.dirname(os.homedir()),
+      path.sep,
+      path.join(path.sep, 'etc'),
+      path.join(path.sep, 'usr'),
+      path.join(path.sep, 'var'),
+      path.join(path.sep, 'bin'),
+      path.join(path.sep, 'sbin'),
+      path.join(path.sep, 'lib'),
+      path.join(path.sep, 'root'),
+      path.join(path.sep, 'home'),
+      path.join(path.sep, 'Users'),
+    ]);
+
+    if (os.platform() === 'win32') {
+      const systemRoot = process.env['SystemRoot'];
+      if (systemRoot) {
+        sensitiveDirs.add(systemRoot);
+        sensitiveDirs.add(path.join(systemRoot, 'System32'));
+      }
+      const programFiles = process.env['ProgramFiles'];
+      if (programFiles) sensitiveDirs.add(programFiles);
+      const programFilesX86 = process.env['ProgramFiles(x86)'];
+      if (programFilesX86) sensitiveDirs.add(programFilesX86);
+    }
+
+    for (const [parent, children] of parentCounts.entries()) {
+      const isSensitive = sensitiveDirs.has(parent);
+      if (children.length >= 3 && parent.length > 1 && !isSensitive) {
+        finalPaths.add(parent);
+      } else {
+        for (const child of children) {
+          finalPaths.add(child);
+        }
+      }
+    }
+
+    // 3. Final redundancy check after consolidation
+    const finalSorted = Array.from(finalPaths).sort(
+      (a, b) => a.length - b.length,
+    );
+    const result: string[] = [];
+    for (const p of finalSorted) {
+      if (!result.some((s) => isSubpath(s, p))) {
+        result.push(p);
+      }
+    }
+
+    return result;
+  }
+
   override getDisplayTitle(): string {
     return this.params.command;
   }
@@ -155,15 +240,94 @@ export class ShellToolInvocation extends BaseToolInvocation<
 
   override async shouldConfirmExecute(
     abortSignal: AbortSignal,
+    forcedDecision?: ForcedToolDecision,
   ): Promise<ToolCallConfirmationDetails | false> {
     if (this.params[PARAM_ADDITIONAL_PERMISSIONS]) {
       return this.getConfirmationDetails(abortSignal);
     }
-    return super.shouldConfirmExecute(abortSignal);
+
+    // Proactively suggest expansion for known network-heavy Node.js ecosystem tools
+    // (npm install, etc.) to avoid hangs when network is restricted by default.
+    // We do this even if the command is "allowed" by policy because the DEFAULT
+    // permissions are usually insufficient for these commands.
+    const command = stripShellWrapper(this.params.command);
+    const rootCommands = getCommandRoots(command);
+    const rootCommand = rootCommands[0];
+
+    if (rootCommand) {
+      const proactive = await getProactiveToolSuggestions(rootCommand);
+      if (proactive) {
+        const approved =
+          this.context.config.sandboxPolicyManager.getCommandPermissions(
+            rootCommand,
+          );
+        const missingNetwork = !!proactive.network && !approved?.network;
+
+        // Detect commands or sub-commands that definitely need network
+        const parsed = parseCommandDetails(command);
+        const subCommand = parsed?.details[0]?.args?.[0];
+        const needsNetwork = isNetworkReliantCommand(rootCommand, subCommand);
+
+        if (needsNetwork) {
+          // Add write permission to the current directory if we are in readonly mode
+          const mode = this.context.config.getApprovalMode();
+          const isReadonlyMode =
+            this.context.config.sandboxPolicyManager.getModeConfig(mode)
+              ?.readonly ?? false;
+
+          if (isReadonlyMode) {
+            const cwd =
+              this.params.dir_path || this.context.config.getTargetDir();
+            proactive.fileSystem = proactive.fileSystem || {
+              read: [],
+              write: [],
+            };
+            proactive.fileSystem.write = proactive.fileSystem.write || [];
+            if (!proactive.fileSystem.write.includes(cwd)) {
+              proactive.fileSystem.write.push(cwd);
+              proactive.fileSystem.read = proactive.fileSystem.read || [];
+              if (!proactive.fileSystem.read.includes(cwd)) {
+                proactive.fileSystem.read.push(cwd);
+              }
+            }
+          }
+
+          const missingRead = (proactive.fileSystem?.read || []).filter(
+            (p) => !approved?.fileSystem?.read?.includes(p),
+          );
+          const missingWrite = (proactive.fileSystem?.write || []).filter(
+            (p) => !approved?.fileSystem?.write?.includes(p),
+          );
+
+          const needsExpansion =
+            missingRead.length > 0 || missingWrite.length > 0 || missingNetwork;
+
+          if (needsExpansion) {
+            const details = await this.getConfirmationDetails(
+              abortSignal,
+              proactive,
+            );
+            if (details && details.type === 'sandbox_expansion') {
+              const originalOnConfirm = details.onConfirm;
+              details.onConfirm = async (outcome: ToolConfirmationOutcome) => {
+                await originalOnConfirm(outcome);
+                if (outcome !== ToolConfirmationOutcome.Cancel) {
+                  this.proactivePermissionsConfirmed = proactive;
+                }
+              };
+            }
+            return details;
+          }
+        }
+      }
+    }
+
+    return super.shouldConfirmExecute(abortSignal, forcedDecision);
   }
 
   protected override async getConfirmationDetails(
     _abortSignal: AbortSignal,
+    proactivePermissions?: SandboxPermissions,
   ): Promise<ToolCallConfirmationDetails | false> {
     const command = stripShellWrapper(this.params.command);
 
@@ -184,30 +348,36 @@ export class ShellToolInvocation extends BaseToolInvocation<
     }
 
     const rootCommands = [...new Set(getCommandRoots(command))];
+    const rootCommand = rootCommands[0] || 'shell';
+
+    // Proactively suggest expansion for known network-heavy tools (npm install, etc.)
+    // to avoid hangs when network is restricted by default.
+    const effectiveAdditionalPermissions =
+      this.params[PARAM_ADDITIONAL_PERMISSIONS] || proactivePermissions;
 
     // Rely entirely on PolicyEngine for interactive confirmation.
     // If we are here, it means PolicyEngine returned ASK_USER (or no message bus),
     // so we must provide confirmation details.
     // If additional_permissions are provided, it's an expansion request
-    if (this.params[PARAM_ADDITIONAL_PERMISSIONS]) {
+    if (effectiveAdditionalPermissions) {
       return {
         type: 'sandbox_expansion',
-        title: 'Sandbox Expansion Request',
+        title: proactivePermissions
+          ? 'Sandbox Expansion Request (Recommended)'
+          : 'Sandbox Expansion Request',
         command: this.params.command,
         rootCommand: rootCommandDisplay,
-        additionalPermissions: this.params[PARAM_ADDITIONAL_PERMISSIONS],
+        additionalPermissions: effectiveAdditionalPermissions,
         onConfirm: async (outcome: ToolConfirmationOutcome) => {
           if (outcome === ToolConfirmationOutcome.ProceedAlwaysAndSave) {
-            const commandName = rootCommands[0] || 'shell';
             this.context.config.sandboxPolicyManager.addPersistentApproval(
-              commandName,
-              this.params[PARAM_ADDITIONAL_PERMISSIONS]!,
+              rootCommand,
+              effectiveAdditionalPermissions,
             );
           } else if (outcome === ToolConfirmationOutcome.ProceedAlways) {
-            const commandName = rootCommands[0] || 'shell';
             this.context.config.sandboxPolicyManager.addSessionApproval(
-              commandName,
-              this.params[PARAM_ADDITIONAL_PERMISSIONS]!,
+              rootCommand,
+              effectiveAdditionalPermissions,
             );
           }
         },
@@ -356,7 +526,25 @@ export class ShellToolInvocation extends BaseToolInvocation<
               shellExecutionConfig?.sanitizationConfig ??
               this.context.config.sanitizationConfig,
             sandboxManager: this.context.config.sandboxManager,
-            additionalPermissions: this.params[PARAM_ADDITIONAL_PERMISSIONS],
+            additionalPermissions: {
+              network:
+                this.params[PARAM_ADDITIONAL_PERMISSIONS]?.network ||
+                this.proactivePermissionsConfirmed?.network,
+              fileSystem: {
+                read: [
+                  ...(this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem
+                    ?.read || []),
+                  ...(this.proactivePermissionsConfirmed?.fileSystem?.read ||
+                    []),
+                ],
+                write: [
+                  ...(this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem
+                    ?.write || []),
+                  ...(this.proactivePermissionsConfirmed?.fileSystem?.write ||
+                    []),
+                ],
+              },
+            },
             backgroundCompletionBehavior:
               this.context.config.getShellBackgroundCompletionBehavior(),
           },
@@ -527,11 +715,33 @@ export class ShellToolInvocation extends BaseToolInvocation<
             this.params[PARAM_ADDITIONAL_PERMISSIONS]?.fileSystem?.write || [],
           );
 
+          // Proactive permission suggestions for Node ecosystem tools
+          const proactive =
+            await getProactiveToolSuggestions(rootCommandDisplay);
+          if (proactive) {
+            if (proactive.network) {
+              sandboxDenial.network = true;
+            }
+            if (proactive.fileSystem?.read) {
+              for (const p of proactive.fileSystem.read) {
+                readPaths.add(p);
+              }
+            }
+            if (proactive.fileSystem?.write) {
+              for (const p of proactive.fileSystem.write) {
+                writePaths.add(p);
+              }
+            }
+          }
+
           if (sandboxDenial.filePaths) {
             for (const p of sandboxDenial.filePaths) {
               try {
                 // Find an existing parent directory to add instead of a non-existent file
                 let currentPath = p;
+                if (currentPath.startsWith('~')) {
+                  currentPath = path.join(os.homedir(), currentPath.slice(1));
+                }
                 try {
                   if (
                     fs.existsSync(currentPath) &&
@@ -544,8 +754,18 @@ export class ShellToolInvocation extends BaseToolInvocation<
                 }
                 while (currentPath.length > 1) {
                   if (fs.existsSync(currentPath)) {
-                    writePaths.add(currentPath);
-                    readPaths.add(currentPath);
+                    const mode = this.context.config.getApprovalMode();
+                    const isReadonlyMode =
+                      this.context.config.sandboxPolicyManager.getModeConfig(
+                        mode,
+                      )?.readonly ?? false;
+                    const isAllowed =
+                      this.context.config.isPathAllowed(currentPath);
+
+                    if (!isAllowed || isReadonlyMode) {
+                      writePaths.add(currentPath);
+                      readPaths.add(currentPath);
+                    }
                     break;
                   }
                   currentPath = path.dirname(currentPath);
@@ -556,16 +776,19 @@ export class ShellToolInvocation extends BaseToolInvocation<
             }
           }
 
+          const simplifiedRead = this.simplifyPaths(readPaths);
+          const simplifiedWrite = this.simplifyPaths(writePaths);
+
           const additionalPermissions = {
             network:
               sandboxDenial.network ||
               this.params[PARAM_ADDITIONAL_PERMISSIONS]?.network ||
               undefined,
             fileSystem:
-              sandboxDenial.filePaths?.length || writePaths.size > 0
+              simplifiedRead.length > 0 || simplifiedWrite.length > 0
                 ? {
-                    read: Array.from(readPaths),
-                    write: Array.from(writePaths),
+                    read: simplifiedRead,
+                    write: simplifiedWrite,
                   }
                 : undefined,
           };
@@ -711,7 +934,7 @@ export class ShellTool extends BaseDeclarativeTool<
     _toolDisplayName?: string,
   ): ToolInvocation<ShellToolParams, ToolResult> {
     return new ShellToolInvocation(
-      this.context.config,
+      this.context,
       params,
       messageBus,
       _toolName,