From 2b666506da694e6918978bac53531daea74b1c66 Mon Sep 17 00:00:00 2001 From: mkorwel Date: Wed, 18 Mar 2026 20:42:57 -0700 Subject: [PATCH] feat(windows-sandbox): fix lint errors and type mismatches --- packages/cli/src/config/config.ts | 6 +- packages/core/src/config/config.ts | 59 +++++++++---------- .../core/src/services/sandboxManager.test.ts | 14 ++--- packages/core/src/services/sandboxManager.ts | 22 +------ .../src/services/sandboxManagerFactory.ts | 45 ++++++++++++++ .../sandboxedFileSystemService.test.ts | 6 +- 6 files changed, 87 insertions(+), 65 deletions(-) create mode 100644 packages/core/src/services/sandboxManagerFactory.ts diff --git a/packages/cli/src/config/config.ts b/packages/cli/src/config/config.ts index 3ab5ddb6ed..db5fd98491 100755 --- a/packages/cli/src/config/config.ts +++ b/packages/cli/src/config/config.ts @@ -703,12 +703,10 @@ export async function loadCliConfig( : specifiedModel || defaultModel; const sandboxConfig = await loadSandboxConfig(settings, argv); if (sandboxConfig) { + const existingPaths = sandboxConfig.allowedPaths || []; if (settings.tools.sandboxAllowedPaths?.length) { sandboxConfig.allowedPaths = [ - ...new Set([ - ...sandboxConfig.allowedPaths, - ...settings.tools.sandboxAllowedPaths, - ]), + ...new Set([...existingPaths, ...settings.tools.sandboxAllowedPaths]), ]; } if (settings.tools.sandboxNetworkAccess !== undefined) { diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index d6de9d41f2..b440b35d4f 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -6,7 +6,6 @@ import * as fs from 'node:fs'; import * as path from 'node:path'; -import * as os from 'node:os'; import { inspect } from 'node:util'; import process from 'node:process'; import { z } from 'zod'; @@ -43,11 +42,11 @@ import type { HookDefinition, HookEventName } from '../hooks/types.js'; import { FileDiscoveryService } from '../services/fileDiscoveryService.js'; import { GitService } from '../services/gitService.js'; import { - createSandboxManager, type SandboxManager, NoopSandboxManager, } from '../services/sandboxManager.js'; -import { WindowsSandboxManager } from '../services/windowsSandboxManager.js'; +import { createSandboxManager } from '../services/sandboxManagerFactory.js'; +import { SandboxedFileSystemService } from '../services/sandboxedFileSystemService.js'; import { initializeTelemetry, DEFAULT_TELEMETRY_TARGET, @@ -78,7 +77,6 @@ import { StandardFileSystemService, type FileSystemService, } from '../services/fileSystemService.js'; -import { SandboxedFileSystemService } from '../services/sandboxedFileSystemService.js'; import { TrackerCreateTaskTool, TrackerUpdateTaskTool, @@ -469,8 +467,8 @@ export enum AuthProviderType { export interface SandboxConfig { enabled: boolean; - allowedPaths: string[]; - networkAccess: boolean; + allowedPaths?: string[]; + networkAccess?: boolean; command?: | 'docker' | 'podman' @@ -499,6 +497,15 @@ export const ConfigSchema = z.object({ .optional(), image: z.string().optional(), }) + .superRefine((data, ctx) => { + if (data.enabled && !data.command) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: 'Sandbox command is required when sandbox is enabled', + path: ['command'], + }); + } + }) .optional(), }); @@ -882,7 +889,6 @@ export class Config implements McpContext, AgentLoopContext { this.approvedPlanPath = undefined; this.embeddingModel = params.embeddingModel ?? DEFAULT_GEMINI_EMBEDDING_MODEL; - this.fileSystemService = new StandardFileSystemService(); this.sandbox = params.sandbox ? { enabled: params.sandbox.enabled ?? false, @@ -896,6 +902,21 @@ export class Config implements McpContext, AgentLoopContext { allowedPaths: [], networkAccess: false, }; + + this._sandboxManager = createSandboxManager(this.sandbox, params.targetDir); + + if ( + !(this._sandboxManager instanceof NoopSandboxManager) && + this.sandbox.enabled + ) { + this.fileSystemService = new SandboxedFileSystemService( + this._sandboxManager, + params.targetDir, + ); + } else { + this.fileSystemService = new StandardFileSystemService(); + } + this.targetDir = path.resolve(params.targetDir); this.folderTrust = params.folderTrust ?? false; this.workspaceContext = new WorkspaceContext(this.targetDir, []); @@ -1066,25 +1087,6 @@ export class Config implements McpContext, AgentLoopContext { this.useAlternateBuffer = params.useAlternateBuffer ?? false; this.enableInteractiveShell = params.enableInteractiveShell ?? false; this.skipNextSpeakerCheck = params.skipNextSpeakerCheck ?? true; - - if ( - os.platform() === 'win32' && - (this.sandbox?.enabled || this.sandbox?.command === 'windows-native') - ) { - this._sandboxManager = new WindowsSandboxManager(); - } else { - this._sandboxManager = new NoopSandboxManager(); - } - - if (!(this._sandboxManager instanceof NoopSandboxManager)) { - this.fileSystemService = new SandboxedFileSystemService( - this._sandboxManager, - this.cwd, - ); - } else { - this.fileSystemService = new StandardFileSystemService(); - } - this.shellExecutionConfig = { terminalWidth: params.shellExecutionConfig?.terminalWidth ?? 80, terminalHeight: params.shellExecutionConfig?.terminalHeight ?? 24, @@ -1214,12 +1216,7 @@ export class Config implements McpContext, AgentLoopContext { } } this._geminiClient = new GeminiClient(this); - this._sandboxManager = createSandboxManager( - params.toolSandboxing ?? false, - this.targetDir, - ); this.a2aClientManager = new A2AClientManager(this); - this.shellExecutionConfig.sandboxManager = this._sandboxManager; this.modelRouterService = new ModelRouterService(this); } diff --git a/packages/core/src/services/sandboxManager.test.ts b/packages/core/src/services/sandboxManager.test.ts index 1c351ce483..d201314d9f 100644 --- a/packages/core/src/services/sandboxManager.test.ts +++ b/packages/core/src/services/sandboxManager.test.ts @@ -6,13 +6,11 @@ import os from 'node:os'; import { describe, expect, it, vi } from 'vitest'; -import { - NoopSandboxManager, - LocalSandboxManager, - createSandboxManager, -} from './sandboxManager.js'; +import { NoopSandboxManager } from './sandboxManager.js'; +import { createSandboxManager } from './sandboxManagerFactory.js'; import { LinuxSandboxManager } from '../sandbox/linux/LinuxSandboxManager.js'; import { MacOsSandboxManager } from '../sandbox/macos/MacOsSandboxManager.js'; +import { WindowsSandboxManager } from './windowsSandboxManager.js'; describe('NoopSandboxManager', () => { const sandboxManager = new NoopSandboxManager(); @@ -121,20 +119,20 @@ describe('NoopSandboxManager', () => { describe('createSandboxManager', () => { it('should return NoopSandboxManager if sandboxing is disabled', () => { - const manager = createSandboxManager(false, '/workspace'); + const manager = createSandboxManager({ enabled: false }, '/workspace'); expect(manager).toBeInstanceOf(NoopSandboxManager); }); it.each([ { platform: 'linux', expected: LinuxSandboxManager }, { platform: 'darwin', expected: MacOsSandboxManager }, - { platform: 'win32', expected: LocalSandboxManager }, + { platform: 'win32', expected: WindowsSandboxManager }, ] as const)( 'should return $expected.name if sandboxing is enabled and platform is $platform', ({ platform, expected }) => { const osSpy = vi.spyOn(os, 'platform').mockReturnValue(platform); try { - const manager = createSandboxManager(true, '/workspace'); + const manager = createSandboxManager({ enabled: true }, '/workspace'); expect(manager).toBeInstanceOf(expected); } finally { osSpy.mockRestore(); diff --git a/packages/core/src/services/sandboxManager.ts b/packages/core/src/services/sandboxManager.ts index 734710290b..8642edff11 100644 --- a/packages/core/src/services/sandboxManager.ts +++ b/packages/core/src/services/sandboxManager.ts @@ -4,14 +4,11 @@ * SPDX-License-Identifier: Apache-2.0 */ -import os from 'node:os'; import { sanitizeEnvironment, getSecureSanitizationConfig, type EnvironmentSanitizationConfig, } from './environmentSanitization.js'; -import { LinuxSandboxManager } from '../sandbox/linux/LinuxSandboxManager.js'; -import { MacOsSandboxManager } from '../sandbox/macos/MacOsSandboxManager.js'; /** * Request for preparing a command to run in a sandbox. @@ -90,21 +87,4 @@ export class LocalSandboxManager implements SandboxManager { } } -/** - * Creates a sandbox manager based on the provided settings. - */ -export function createSandboxManager( - sandboxingEnabled: boolean, - workspace: string, -): SandboxManager { - if (sandboxingEnabled) { - if (os.platform() === 'linux') { - return new LinuxSandboxManager({ workspace }); - } - if (os.platform() === 'darwin') { - return new MacOsSandboxManager({ workspace }); - } - return new LocalSandboxManager(); - } - return new NoopSandboxManager(); -} +export { createSandboxManager } from './sandboxManagerFactory.js'; diff --git a/packages/core/src/services/sandboxManagerFactory.ts b/packages/core/src/services/sandboxManagerFactory.ts new file mode 100644 index 0000000000..fffc366da9 --- /dev/null +++ b/packages/core/src/services/sandboxManagerFactory.ts @@ -0,0 +1,45 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import os from 'node:os'; +import { + type SandboxManager, + NoopSandboxManager, + LocalSandboxManager, +} from './sandboxManager.js'; +import { LinuxSandboxManager } from '../sandbox/linux/LinuxSandboxManager.js'; +import { MacOsSandboxManager } from '../sandbox/macos/MacOsSandboxManager.js'; +import { WindowsSandboxManager } from './windowsSandboxManager.js'; +import type { SandboxConfig } from '../config/config.js'; + +/** + * Creates a sandbox manager based on the provided settings. + */ +export function createSandboxManager( + sandbox: SandboxConfig | undefined, + workspace: string, +): SandboxManager { + const isWindows = os.platform() === 'win32'; + + if ( + isWindows && + (sandbox?.enabled || sandbox?.command === 'windows-native') + ) { + return new WindowsSandboxManager(); + } + + if (sandbox?.enabled) { + if (os.platform() === 'linux') { + return new LinuxSandboxManager({ workspace }); + } + if (os.platform() === 'darwin') { + return new MacOsSandboxManager({ workspace }); + } + return new LocalSandboxManager(); + } + + return new NoopSandboxManager(); +} diff --git a/packages/core/src/services/sandboxedFileSystemService.test.ts b/packages/core/src/services/sandboxedFileSystemService.test.ts index 8c1bf9d968..47c997f4a8 100644 --- a/packages/core/src/services/sandboxedFileSystemService.test.ts +++ b/packages/core/src/services/sandboxedFileSystemService.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { SandboxedFileSystemService } from './sandboxedFileSystemService.js'; import type { SandboxManager, @@ -40,6 +40,10 @@ describe('SandboxedFileSystemService', () => { vi.clearAllMocks(); }); + afterEach(() => { + vi.restoreAllMocks(); + }); + it('should read a file through the sandbox', async () => { const mockChild = new EventEmitter() as unknown as ChildProcess; Object.assign(mockChild, {