fix: address SandboxManager PR feedback

- docs: Update tools.sandbox configuration type to boolean | string |
  object
- core: Add validation to ConfigSchema requiring a command when sandbox
  is enabled
- core: Remove redundant sandbox defaulting logic from Config
  constructor
- cli: Update LXC removeDevices exit listener to use spawnSync with
  SIGKILL to prevent hanging processes
- core: Integrate NoopSandboxManager into ShellExecutionService to
  correctly utilize sanitized environment
This commit is contained in:
galz10
2026-03-11 13:11:53 -07:00
parent 2ea7a67106
commit 450a331e36
4 changed files with 28 additions and 25 deletions
+1 -1
View File
@@ -756,7 +756,7 @@ their corresponding top-level category object in your `settings.json` file.
#### `tools` #### `tools`
- **`tools.sandbox`** (string): - **`tools.sandbox`** (boolean | string | object):
- **Description:** Sandbox execution environment. Set to a boolean to enable - **Description:** Sandbox execution environment. Set to a boolean to enable
or disable the sandbox, provide a string path to a sandbox profile, or or disable the sandbox, provide a string path to a sandbox profile, or
specify an explicit sandbox command (e.g., "docker", "podman", "lxc"). specify an explicit sandbox command (e.g., "docker", "podman", "lxc").
+3 -3
View File
@@ -7,9 +7,9 @@
import { import {
exec, exec,
execFile, execFile,
execFileSync,
execSync, execSync,
spawn, spawn,
spawnSync,
type ChildProcess, type ChildProcess,
} from 'node:child_process'; } from 'node:child_process';
import path from 'node:path'; import path from 'node:path';
@@ -876,10 +876,10 @@ async function start_lxc_sandbox(
const removeDevices = () => { const removeDevices = () => {
for (const deviceName of devicesToRemove) { for (const deviceName of devicesToRemove) {
try { try {
execFileSync( spawnSync(
'lxc', 'lxc',
['config', 'device', 'remove', containerName, deviceName], ['config', 'device', 'remove', containerName, deviceName],
{ timeout: 2000 }, { timeout: 1000, killSignal: 'SIGKILL', stdio: 'ignore' },
); );
} catch { } catch {
// Best-effort cleanup; ignore errors on exit. // Best-effort cleanup; ignore errors on exit.
+10 -13
View File
@@ -469,6 +469,15 @@ export const ConfigSchema = z.object({
.optional(), .optional(),
image: z.string().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(), .optional(),
}); });
@@ -839,19 +848,7 @@ export class Config implements McpContext, AgentLoopContext {
this.embeddingModel = this.embeddingModel =
params.embeddingModel ?? DEFAULT_GEMINI_EMBEDDING_MODEL; params.embeddingModel ?? DEFAULT_GEMINI_EMBEDDING_MODEL;
this.fileSystemService = new StandardFileSystemService(); this.fileSystemService = new StandardFileSystemService();
this.sandbox = params.sandbox this.sandbox = params.sandbox;
? {
enabled: params.sandbox.enabled ?? false,
allowedPaths: params.sandbox.allowedPaths ?? [],
networkAccess: params.sandbox.networkAccess ?? false,
command: params.sandbox.command,
image: params.sandbox.image,
}
: {
enabled: false,
allowedPaths: [],
networkAccess: false,
};
this.targetDir = path.resolve(params.targetDir); this.targetDir = path.resolve(params.targetDir);
this.folderTrust = params.folderTrust ?? false; this.folderTrust = params.folderTrust ?? false;
this.workspaceContext = new WorkspaceContext(this.targetDir, []); this.workspaceContext = new WorkspaceContext(this.targetDir, []);
@@ -26,10 +26,8 @@ import {
serializeTerminalToObject, serializeTerminalToObject,
type AnsiOutput, type AnsiOutput,
} from '../utils/terminalSerializer.js'; } from '../utils/terminalSerializer.js';
import { import { sanitizeEnvironment, type EnvironmentSanitizationConfig } from './environmentSanitization.js';
sanitizeEnvironment, import { NoopSandboxManager } from './sandboxManager.js';
type EnvironmentSanitizationConfig,
} from './environmentSanitization.js';
import { killProcessGroup } from '../utils/process-utils.js'; import { killProcessGroup } from '../utils/process-utils.js';
const { Terminal } = pkg; const { Terminal } = pkg;
@@ -326,6 +324,15 @@ export class ShellExecutionService {
shouldUseNodePty: boolean, shouldUseNodePty: boolean,
shellExecutionConfig: ShellExecutionConfig, shellExecutionConfig: ShellExecutionConfig,
): Promise<ShellExecutionHandle> { ): Promise<ShellExecutionHandle> {
const sandboxManager = new NoopSandboxManager();
const { env: sanitizedEnv } = await sandboxManager.prepareCommand({
command: commandToExecute,
args: [],
env: process.env,
cwd,
config: shellExecutionConfig,
});
if (shouldUseNodePty) { if (shouldUseNodePty) {
const ptyInfo = await getPty(); const ptyInfo = await getPty();
if (ptyInfo) { if (ptyInfo) {
@@ -337,6 +344,7 @@ export class ShellExecutionService {
abortSignal, abortSignal,
shellExecutionConfig, shellExecutionConfig,
ptyInfo, ptyInfo,
sanitizedEnv,
); );
} catch (_e) { } catch (_e) {
// Fallback to child_process // Fallback to child_process
@@ -695,6 +703,7 @@ export class ShellExecutionService {
abortSignal: AbortSignal, abortSignal: AbortSignal,
shellExecutionConfig: ShellExecutionConfig, shellExecutionConfig: ShellExecutionConfig,
ptyInfo: PtyImplementation, ptyInfo: PtyImplementation,
sanitizedEnv: Record<string, string | undefined>,
): Promise<ShellExecutionHandle> { ): Promise<ShellExecutionHandle> {
if (!ptyInfo) { if (!ptyInfo) {
// This should not happen, but as a safeguard... // This should not happen, but as a safeguard...
@@ -724,10 +733,7 @@ export class ShellExecutionService {
cols, cols,
rows, rows,
env: { env: {
...sanitizeEnvironment( ...sanitizedEnv,
process.env,
shellExecutionConfig.sanitizationConfig,
),
GEMINI_CLI: '1', GEMINI_CLI: '1',
TERM: 'xterm-256color', TERM: 'xterm-256color',
PAGER: shellExecutionConfig.pager ?? 'cat', PAGER: shellExecutionConfig.pager ?? 'cat',