Skip to content

Commit 78e9f7e

Browse files
committed
fix(cli): use strict boolean parsing for DEBUG env var in sandbox launcher (#28885)
The sandbox launcher (sandbox.ts) used JavaScript string truthiness to evaluate the DEBUG environment variable in four call-sites: 1. ConsolePatcher({ debugMode: !!process.env['DEBUG'] }) 2. Seatbelt NODE_OPTIONS: process.env['DEBUG'] ? ['--inspect-brk'] : [] 3. Docker port publication: if (process.env['DEBUG']) { --publish 9229 } 4. Image-pull progress logging: process.env['DEBUG'] Because any non-empty string is truthy in JavaScript, values that conventionally disable a flag — 'false', '0' — were incorrectly treated as debug-enabled. This caused: - Docker/Podman: unnecessary --publish 9229:9229 host port exposure - macOS Seatbelt: --inspect-brk injected into NODE_OPTIONS, pausing the CLI on startup even though isDebugMode() considers it disabled - ConsolePatcher: debug console output enabled spuriously - pullImage(): verbose pull-progress logging enabled spuriously Meanwhile, the container entrypoint (sandboxUtils.ts) and CLI config (config.ts:isDebugMode) already used the correct strict check: process.env['DEBUG'] === 'true' || process.env['DEBUG'] === '1' This left the launcher and entrypoint in contradictory states. Fix: - Add isDebugEnvEnabled() in sandboxUtils.ts as a shared boolean parser with strict semantics (only 'true' and '1' enable debug) - Replace all four truthy checks in sandbox.ts with isDebugEnvEnabled() - Refactor the inline check in sandboxUtils.ts entrypoint() to reuse the same function Call-chain covered: start_sandbox() → ConsolePatcher({ debugMode }) [sandbox.ts ~L55] → (seatbelt) nodeOptions --inspect-brk [sandbox.ts ~L157] → (docker) --publish debugPort:debugPort [sandbox.ts ~L516] → pullImage() → onStdoutData logging [sandbox.ts ~L1188] entrypoint() → isDebugMode → cliCmd selection [sandboxUtils.ts ~L164] Test coverage: - 18 new regression tests (sandbox.debug.test.ts): · Docker debug-port: disabled for 'false', '0', '', unset; enabled for 'true', '1'; custom DEBUG_PORT respected · Seatbelt --inspect-brk: disabled for 'false', '0', unset; enabled for 'true', '1' · Non-canonical values: 'yes', 'on', 'TRUE', 'True', '2', 'enabled' all correctly treated as disabled - 12 new unit tests for isDebugEnvEnabled() (sandboxUtils.test.ts): · Positive cases: 'true', '1' · Negative cases: 'false', '0', '', unset · Non-canonical edge cases: 'yes', 'on', 'TRUE', 'True', '2', 'enabled' - All 30 pre-existing sandbox.test.ts tests pass unchanged - All 25 pre-existing sandboxUtils.test.ts tests pass unchanged - TypeScript type-check passes (tsc --noEmit) Closes #28885
1 parent 30573d2 commit 78e9f7e

4 files changed

Lines changed: 410 additions & 8 deletions

File tree

Lines changed: 346 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,346 @@
1+
/**
2+
* @license
3+
* Copyright 2025 Google LLC
4+
* SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
/**
8+
* Regression tests for DEBUG environment variable handling in sandbox.ts.
9+
*
10+
* These tests verify that only the values 'true' and '1' enable debug
11+
* behaviour. Before this fix, sandbox.ts used JavaScript string truthiness
12+
* (!!process.env['DEBUG']) instead of strict comparison, so values like
13+
* 'false' and '0' incorrectly activated:
14+
* - Docker/Podman debug-port publication (--publish 9229:9229)
15+
* - macOS Seatbelt --inspect-brk injection
16+
* - ConsolePatcher debug mode
17+
* - Image-pull progress logging
18+
*
19+
* Call-chain covered by this test file:
20+
* start_sandbox()
21+
* → ConsolePatcher({ debugMode }) [line ~55]
22+
* → (seatbelt path) nodeOptions --inspect-brk [line ~157]
23+
* → (docker path) --publish debugPort [line ~516]
24+
* → pullImage() → onStdoutData logging [line ~1188]
25+
*
26+
* See: https://github.com/google-gemini/gemini-cli/issues/28885
27+
*/
28+
29+
import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest';
30+
import { spawn, exec, execFile, execSync } from 'node:child_process';
31+
import os from 'node:os';
32+
import fs from 'node:fs';
33+
import { start_sandbox } from './sandbox.js';
34+
import type { SandboxConfig } from '@google/gemini-cli-core';
35+
import { createMockSandboxConfig } from '@google/gemini-cli-test-utils';
36+
import { EventEmitter } from 'node:events';
37+
38+
const { mockedGetContainerPath, mockedExecCommands } = vi.hoisted(() => ({
39+
mockedGetContainerPath: vi.fn().mockImplementation((p: string) => p),
40+
mockedExecCommands: [] as string[],
41+
}));
42+
43+
vi.mock('./sandboxUtils.js', async (importOriginal) => {
44+
const actual = await importOriginal<typeof import('./sandboxUtils.js')>();
45+
return {
46+
...actual,
47+
getContainerPath: mockedGetContainerPath,
48+
};
49+
});
50+
51+
vi.mock('node:child_process');
52+
vi.mock('node:os');
53+
vi.mock('node:fs');
54+
vi.mock('node:crypto', () => ({
55+
randomBytes: vi.fn().mockReturnValue(Buffer.from('a1b2c3d4e5f6', 'hex')),
56+
}));
57+
vi.mock('node:util', async (importOriginal) => {
58+
const actual = await importOriginal<typeof import('node:util')>();
59+
return {
60+
...actual,
61+
promisify: (fn: (...args: unknown[]) => unknown) => {
62+
if (fn === exec) {
63+
return async (cmd: string) => {
64+
mockedExecCommands.push(cmd);
65+
if (cmd === 'id -u' || cmd === 'id -g') {
66+
return { stdout: '1000', stderr: '' };
67+
}
68+
if (cmd.includes('getconf DARWIN_USER_CACHE_DIR')) {
69+
return { stdout: '/tmp/cache', stderr: '' };
70+
}
71+
return { stdout: '', stderr: '' };
72+
};
73+
}
74+
if (fn === execFile) {
75+
return async () => ({ stdout: '', stderr: '' });
76+
}
77+
return actual.promisify(fn);
78+
},
79+
};
80+
});
81+
82+
/**
83+
* Helper: create a mock spawn result that mimics the Docker image-check
84+
* process (spawn 'docker images ...') and the subsequent 'docker run' process.
85+
*
86+
* The returned `capturedRunArgs` array will be populated with the `args`
87+
* parameter of the 'docker run' spawn call, allowing assertions on the
88+
* presence or absence of --publish and debug-port arguments.
89+
*/
90+
function mockDockerSpawnSequence(): { capturedRunArgs: string[][] } {
91+
const capturedRunArgs: string[][] = [];
92+
93+
interface MockProcessWithStdout extends EventEmitter {
94+
stdout: EventEmitter;
95+
}
96+
97+
// First spawn: 'docker images ...' — return image found immediately.
98+
const mockImageCheckProcess = new EventEmitter() as MockProcessWithStdout;
99+
mockImageCheckProcess.stdout = new EventEmitter();
100+
vi.mocked(spawn).mockImplementationOnce((_cmd, args) => {
101+
if (args && args[0] === 'images') {
102+
setTimeout(() => {
103+
mockImageCheckProcess.stdout.emit('data', Buffer.from('image-id'));
104+
mockImageCheckProcess.emit('close', 0);
105+
}, 1);
106+
return mockImageCheckProcess as unknown as ReturnType<typeof spawn>;
107+
}
108+
return new EventEmitter() as unknown as ReturnType<typeof spawn>;
109+
});
110+
111+
// Second spawn: 'docker run ...' — capture args and close cleanly.
112+
const mockRunProcess = new EventEmitter() as unknown as ReturnType<
113+
typeof spawn
114+
>;
115+
mockRunProcess.on = vi.fn().mockImplementation((event, cb) => {
116+
if (event === 'close') {
117+
setTimeout(() => cb(0), 10);
118+
}
119+
return mockRunProcess;
120+
});
121+
vi.mocked(spawn).mockImplementationOnce((_cmd, args) => {
122+
if (args) {
123+
capturedRunArgs.push(args as string[]);
124+
}
125+
return mockRunProcess;
126+
});
127+
128+
return { capturedRunArgs };
129+
}
130+
131+
/**
132+
* Helper: create a mock spawn result for the macOS seatbelt path
133+
* (spawn 'sandbox-exec ...'). The returned `capturedArgs` array will
134+
* be populated with the full `args` of the spawn call.
135+
*/
136+
function mockSeatbeltSpawnSequence(): { capturedArgs: string[][] } {
137+
const capturedArgs: string[][] = [];
138+
139+
interface MockProcess extends EventEmitter {
140+
stdout: EventEmitter;
141+
stderr: EventEmitter;
142+
}
143+
const mockSpawnProcess = new EventEmitter() as MockProcess;
144+
mockSpawnProcess.stdout = new EventEmitter();
145+
mockSpawnProcess.stderr = new EventEmitter();
146+
vi.mocked(spawn).mockReturnValue(
147+
mockSpawnProcess as unknown as ReturnType<typeof spawn>,
148+
);
149+
150+
// Capture args and schedule the close event.
151+
vi.mocked(spawn).mockImplementation((_cmd, args) => {
152+
if (args) {
153+
capturedArgs.push(args as string[]);
154+
}
155+
setTimeout(() => mockSpawnProcess.emit('close', 0), 10);
156+
return mockSpawnProcess as unknown as ReturnType<typeof spawn>;
157+
});
158+
159+
return { capturedArgs };
160+
}
161+
162+
describe('sandbox — DEBUG environment variable handling', () => {
163+
const originalArgv = process.argv;
164+
165+
beforeEach(() => {
166+
vi.clearAllMocks();
167+
mockedExecCommands.length = 0;
168+
process.argv = [...originalArgv];
169+
vi.stubEnv('DEBUG', '');
170+
vi.stubEnv('DEBUG_PORT', '');
171+
Object.defineProperty(process, 'stdin', {
172+
value: { pause: vi.fn(), resume: vi.fn(), isTTY: true },
173+
writable: true,
174+
});
175+
vi.mocked(os.platform).mockReturnValue('linux');
176+
vi.mocked(os.homedir).mockReturnValue('/home/user');
177+
vi.mocked(os.tmpdir).mockReturnValue('/tmp');
178+
vi.mocked(fs.existsSync).mockReturnValue(true);
179+
vi.mocked(fs.realpathSync).mockImplementation((p) => p as string);
180+
vi.mocked(execSync).mockReturnValue(Buffer.from(''));
181+
});
182+
183+
afterEach(() => {
184+
process.argv = originalArgv;
185+
vi.unstubAllEnvs();
186+
});
187+
188+
// -----------------------------------------------------------------------
189+
// Docker / Podman: --publish debug-port assertions
190+
// -----------------------------------------------------------------------
191+
describe('Docker debug-port publication', () => {
192+
const dockerConfig: SandboxConfig = createMockSandboxConfig({
193+
command: 'docker',
194+
image: 'gemini-cli-sandbox',
195+
});
196+
197+
it.each(['false', '0'])(
198+
'should NOT publish the debug port when DEBUG=%s',
199+
async (debugValue) => {
200+
vi.stubEnv('DEBUG', debugValue);
201+
const { capturedRunArgs } = mockDockerSpawnSequence();
202+
203+
await start_sandbox(dockerConfig, [], undefined, []);
204+
205+
expect(capturedRunArgs.length).toBeGreaterThan(0);
206+
const runArgs = capturedRunArgs[0];
207+
expect(runArgs).not.toContain('--publish');
208+
expect(runArgs.join(' ')).not.toContain('9229');
209+
},
210+
);
211+
212+
it('should NOT publish the debug port when DEBUG is unset', async () => {
213+
vi.stubEnv('DEBUG', '');
214+
const { capturedRunArgs } = mockDockerSpawnSequence();
215+
216+
await start_sandbox(dockerConfig, [], undefined, []);
217+
218+
expect(capturedRunArgs.length).toBeGreaterThan(0);
219+
const runArgs = capturedRunArgs[0];
220+
// The '--publish' token should only appear for user-specified SANDBOX_PORTS,
221+
// not for the debug port.
222+
const publishIndices = runArgs
223+
.map((arg, i) => (arg === '--publish' ? i : -1))
224+
.filter((i) => i >= 0);
225+
for (const idx of publishIndices) {
226+
expect(runArgs[idx + 1]).not.toContain('9229');
227+
}
228+
});
229+
230+
it('should NOT publish the debug port when DEBUG is an empty string', async () => {
231+
vi.stubEnv('DEBUG', '');
232+
const { capturedRunArgs } = mockDockerSpawnSequence();
233+
234+
await start_sandbox(dockerConfig, [], undefined, []);
235+
236+
expect(capturedRunArgs.length).toBeGreaterThan(0);
237+
const runArgs = capturedRunArgs[0];
238+
expect(runArgs.join(' ')).not.toContain('9229');
239+
});
240+
241+
it.each(['true', '1'])(
242+
'should publish the debug port when DEBUG=%s',
243+
async (debugValue) => {
244+
vi.stubEnv('DEBUG', debugValue);
245+
const { capturedRunArgs } = mockDockerSpawnSequence();
246+
247+
await start_sandbox(dockerConfig, [], undefined, []);
248+
249+
expect(capturedRunArgs.length).toBeGreaterThan(0);
250+
const runArgs = capturedRunArgs[0];
251+
expect(runArgs).toContain('--publish');
252+
expect(runArgs.join(' ')).toContain('9229:9229');
253+
},
254+
);
255+
256+
it('should respect a custom DEBUG_PORT when DEBUG=true', async () => {
257+
vi.stubEnv('DEBUG', 'true');
258+
vi.stubEnv('DEBUG_PORT', '5858');
259+
const { capturedRunArgs } = mockDockerSpawnSequence();
260+
261+
await start_sandbox(dockerConfig, [], undefined, []);
262+
263+
expect(capturedRunArgs.length).toBeGreaterThan(0);
264+
const runArgs = capturedRunArgs[0];
265+
expect(runArgs.join(' ')).toContain('5858:5858');
266+
expect(runArgs.join(' ')).not.toContain('9229');
267+
});
268+
});
269+
270+
// -----------------------------------------------------------------------
271+
// macOS Seatbelt: --inspect-brk injection
272+
// -----------------------------------------------------------------------
273+
describe('macOS Seatbelt --inspect-brk injection', () => {
274+
const seatbeltConfig: SandboxConfig = createMockSandboxConfig({
275+
command: 'sandbox-exec',
276+
image: 'some-image',
277+
});
278+
279+
beforeEach(() => {
280+
vi.mocked(os.platform).mockReturnValue('darwin');
281+
});
282+
283+
it.each(['false', '0'])(
284+
'should NOT inject --inspect-brk into NODE_OPTIONS when DEBUG=%s',
285+
async (debugValue) => {
286+
vi.stubEnv('DEBUG', debugValue);
287+
const { capturedArgs } = mockSeatbeltSpawnSequence();
288+
289+
await start_sandbox(seatbeltConfig, [], undefined, []);
290+
291+
expect(capturedArgs.length).toBeGreaterThan(0);
292+
const allArgs = capturedArgs[0].join(' ');
293+
expect(allArgs).not.toContain('--inspect-brk');
294+
},
295+
);
296+
297+
it('should NOT inject --inspect-brk when DEBUG is unset', async () => {
298+
vi.stubEnv('DEBUG', '');
299+
const { capturedArgs } = mockSeatbeltSpawnSequence();
300+
301+
await start_sandbox(seatbeltConfig, [], undefined, []);
302+
303+
expect(capturedArgs.length).toBeGreaterThan(0);
304+
const allArgs = capturedArgs[0].join(' ');
305+
expect(allArgs).not.toContain('--inspect-brk');
306+
});
307+
308+
it.each(['true', '1'])(
309+
'should inject --inspect-brk into NODE_OPTIONS when DEBUG=%s',
310+
async (debugValue) => {
311+
vi.stubEnv('DEBUG', debugValue);
312+
const { capturedArgs } = mockSeatbeltSpawnSequence();
313+
314+
await start_sandbox(seatbeltConfig, [], undefined, []);
315+
316+
expect(capturedArgs.length).toBeGreaterThan(0);
317+
const allArgs = capturedArgs[0].join(' ');
318+
expect(allArgs).toContain('--inspect-brk');
319+
},
320+
);
321+
});
322+
323+
// -----------------------------------------------------------------------
324+
// Edge cases: non-canonical truthy values that should NOT enable debug
325+
// -----------------------------------------------------------------------
326+
describe('non-canonical truthy values', () => {
327+
const dockerConfig: SandboxConfig = createMockSandboxConfig({
328+
command: 'docker',
329+
image: 'gemini-cli-sandbox',
330+
});
331+
332+
it.each(['yes', 'on', 'TRUE', 'True', '2', 'enabled'])(
333+
'should NOT publish the debug port for non-canonical value DEBUG=%s',
334+
async (debugValue) => {
335+
vi.stubEnv('DEBUG', debugValue);
336+
const { capturedRunArgs } = mockDockerSpawnSequence();
337+
338+
await start_sandbox(dockerConfig, [], undefined, []);
339+
340+
expect(capturedRunArgs.length).toBeGreaterThan(0);
341+
const runArgs = capturedRunArgs[0];
342+
expect(runArgs.join(' ')).not.toContain('9229');
343+
},
344+
);
345+
});
346+
});

packages/cli/src/utils/sandbox.ts

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import {
3434
parseImageName,
3535
ports,
3636
entrypoint,
37+
isDebugEnvEnabled,
3738
LOCAL_DEV_SANDBOX_IMAGE_NAME,
3839
SANDBOX_NETWORK_NAME,
3940
SANDBOX_PROXY_NAME,
@@ -51,7 +52,7 @@ export async function start_sandbox(
5152
cliArgs: string[] = [],
5253
): Promise<number> {
5354
const patcher = new ConsolePatcher({
54-
debugMode: cliConfig?.getDebugMode() || !!process.env['DEBUG'],
55+
debugMode: cliConfig?.getDebugMode() || isDebugEnvEnabled(),
5556
stderr: true,
5657
});
5758
patcher.patch();
@@ -151,9 +152,9 @@ export async function start_sandbox(
151152
);
152153
}
153154
debugLogger.log(`using macos seatbelt (profile: ${profile}) ...`);
154-
// if DEBUG is set, convert to --inspect-brk in NODE_OPTIONS
155+
// if DEBUG is enabled, convert to --inspect-brk in NODE_OPTIONS
155156
const nodeOptions = [
156-
...(process.env['DEBUG'] ? ['--inspect-brk'] : []),
157+
...(isDebugEnvEnabled() ? ['--inspect-brk'] : []),
157158
...nodeArgs,
158159
].join(' ');
159160

@@ -511,8 +512,8 @@ export async function start_sandbox(
511512
// expose env-specified ports on the sandbox
512513
ports().forEach((p) => args.push('--publish', `${p}:${p}`));
513514

514-
// if DEBUG is set, expose debugging port
515-
if (process.env['DEBUG']) {
515+
// if DEBUG is enabled, expose debugging port
516+
if (isDebugEnvEnabled()) {
516517
const debugPort = process.env['DEBUG_PORT'] || '9229';
517518
args.push(`--publish`, `${debugPort}:${debugPort}`);
518519
}
@@ -1184,7 +1185,7 @@ async function pullImage(
11841185
let stderrData = '';
11851186

11861187
const onStdoutData = (data: Buffer) => {
1187-
if (cliConfig?.getDebugMode() || process.env['DEBUG']) {
1188+
if (cliConfig?.getDebugMode() || isDebugEnvEnabled()) {
11881189
debugLogger.log(data.toString().trim()); // Show pull progress
11891190
}
11901191
};

0 commit comments

Comments
 (0)