Repository navigation
Conversation
|
📊 PR Size: size/M
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the sandbox utility logic to fix an issue where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](/web-api/iframe-proxy?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function isDebugModeEnabled to consolidate debug mode checks across the CLI sandbox utility, checking both cliConfig and the DEBUG / DEBUG_MODE environment variables. It also adds corresponding unit tests. The feedback suggests refactoring the newly added tests in sandbox.test.ts to reduce duplication and improve maintainability by combining test cases using parameterized it.each blocks.
| it.each(['false', '0'])( | ||
| 'should not publish the debug port when DEBUG=%s', | ||
| async (debugValue) => { | ||
| process.env['DEBUG'] = debugValue; | ||
| const config: SandboxConfig = createMockSandboxConfig({ | ||
| command: 'docker', | ||
| image: 'gemini-cli-sandbox', | ||
| }); | ||
|
|
||
| interface MockProcessWithStdout extends EventEmitter { | ||
| stdout: EventEmitter; | ||
| } | ||
| const mockImageCheckProcess = | ||
| new EventEmitter() as MockProcessWithStdout; | ||
| mockImageCheckProcess.stdout = new EventEmitter(); | ||
| vi.mocked(spawn).mockImplementationOnce(() => { | ||
| setTimeout(() => { | ||
| mockImageCheckProcess.stdout.emit('data', Buffer.from('image-id')); | ||
| mockImageCheckProcess.emit('close', 0); | ||
| }, 1); | ||
| return mockImageCheckProcess as unknown as ReturnType<typeof spawn>; | ||
| }); | ||
|
|
||
| const mockSpawnProcess = new EventEmitter() as unknown as ReturnType< | ||
| typeof spawn | ||
| >; | ||
| mockSpawnProcess.on = vi.fn().mockImplementation((event, cb) => { | ||
| if (event === 'close') { | ||
| setTimeout(() => cb(0), 10); | ||
| } | ||
| return mockSpawnProcess; | ||
| }); | ||
| vi.mocked(spawn).mockImplementationOnce(() => mockSpawnProcess); | ||
|
|
||
| await start_sandbox(config, [], undefined, ['arg1']); | ||
|
|
||
| const dockerArgs = vi.mocked(spawn).mock.calls[1]?.[1]; | ||
| expect(dockerArgs).not.toContain('--publish'); | ||
| expect(dockerArgs).not.toContain('9229:9229'); | ||
| }, | ||
| ); | ||
|
|
||
| it.each(['true', '1'])( | ||
| 'should publish the debug port when DEBUG=%s', | ||
| async (debugValue) => { | ||
| process.env['DEBUG'] = debugValue; | ||
| const config: SandboxConfig = createMockSandboxConfig({ | ||
| command: 'docker', | ||
| image: 'gemini-cli-sandbox', | ||
| }); | ||
|
|
||
| interface MockProcessWithStdout extends EventEmitter { | ||
| stdout: EventEmitter; | ||
| } | ||
| const mockImageCheckProcess = | ||
| new EventEmitter() as MockProcessWithStdout; | ||
| mockImageCheckProcess.stdout = new EventEmitter(); | ||
| vi.mocked(spawn).mockImplementationOnce(() => { | ||
| setTimeout(() => { | ||
| mockImageCheckProcess.stdout.emit('data', Buffer.from('image-id')); | ||
| mockImageCheckProcess.emit('close', 0); | ||
| }, 1); | ||
| return mockImageCheckProcess as unknown as ReturnType<typeof spawn>; | ||
| }); | ||
|
|
||
| const mockSpawnProcess = new EventEmitter() as unknown as ReturnType< | ||
| typeof spawn | ||
| >; | ||
| mockSpawnProcess.on = vi.fn().mockImplementation((event, cb) => { | ||
| if (event === 'close') { | ||
| setTimeout(() => cb(0), 10); | ||
| } | ||
| return mockSpawnProcess; | ||
| }); | ||
| vi.mocked(spawn).mockImplementationOnce(() => mockSpawnProcess); | ||
|
|
||
| await start_sandbox(config, [], undefined, ['arg1']); | ||
|
|
||
| const dockerArgs = vi.mocked(spawn).mock.calls[1]?.[1]; | ||
| expect(dockerArgs).toContain('--publish'); | ||
| expect(dockerArgs).toContain('9229:9229'); | ||
| }, | ||
| ); | ||
|
|
||
| it.each(['false', '0'])( | ||
| 'should not add --inspect-brk to seatbelt NODE_OPTIONS when DEBUG=%s', | ||
| async (debugValue) => { | ||
| vi.mocked(os.platform).mockReturnValue('darwin'); | ||
| vi.mocked(fs.existsSync).mockReturnValue(true); | ||
| process.env['DEBUG'] = debugValue; | ||
|
|
||
| const config: SandboxConfig = createMockSandboxConfig({ | ||
| command: 'sandbox-exec', | ||
| image: 'some-image', | ||
| }); | ||
|
|
||
| interface MockProcess extends EventEmitter { | ||
| stdout: EventEmitter; | ||
| stderr: EventEmitter; | ||
| } | ||
| const mockSpawnProcess = new EventEmitter() as MockProcess; | ||
| mockSpawnProcess.stdout = new EventEmitter(); | ||
| mockSpawnProcess.stderr = new EventEmitter(); | ||
| vi.mocked(spawn).mockReturnValue( | ||
| mockSpawnProcess as unknown as ReturnType<typeof spawn>, | ||
| ); | ||
|
|
||
| const promise = start_sandbox(config, [], undefined, ['arg1']); | ||
| setTimeout(() => { | ||
| mockSpawnProcess.emit('close', 0); | ||
| }, 10); | ||
|
|
||
| await expect(promise).resolves.toBe(0); | ||
|
|
||
| const spawnArgs = vi.mocked(spawn).mock.calls[0]?.[1]; | ||
| const shellCmd = spawnArgs?.[spawnArgs.length - 1]; | ||
| expect(shellCmd).not.toContain('--inspect-brk'); | ||
| }, | ||
| ); | ||
|
|
||
| it.each(['true', '1'])( | ||
| 'should add --inspect-brk to seatbelt NODE_OPTIONS when DEBUG=%s', | ||
| async (debugValue) => { | ||
| vi.mocked(os.platform).mockReturnValue('darwin'); | ||
| vi.mocked(fs.existsSync).mockReturnValue(true); | ||
| process.env['DEBUG'] = debugValue; | ||
|
|
||
| const config: SandboxConfig = createMockSandboxConfig({ | ||
| command: 'sandbox-exec', | ||
| image: 'some-image', | ||
| }); | ||
|
|
||
| interface MockProcess extends EventEmitter { | ||
| stdout: EventEmitter; | ||
| stderr: EventEmitter; | ||
| } | ||
| const mockSpawnProcess = new EventEmitter() as MockProcess; | ||
| mockSpawnProcess.stdout = new EventEmitter(); | ||
| mockSpawnProcess.stderr = new EventEmitter(); | ||
| vi.mocked(spawn).mockReturnValue( | ||
| mockSpawnProcess as unknown as ReturnType<typeof spawn>, | ||
| ); | ||
|
|
||
| const promise = start_sandbox(config, [], undefined, ['arg1']); | ||
| setTimeout(() => { | ||
| mockSpawnProcess.emit('close', 0); | ||
| }, 10); | ||
|
|
||
| await expect(promise).resolves.toBe(0); | ||
|
|
||
| const spawnArgs = vi.mocked(spawn).mock.calls[0]?.[1]; | ||
| const shellCmd = spawnArgs?.[spawnArgs.length - 1]; | ||
| expect(shellCmd).toContain('--inspect-brk'); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
While these tests are thorough and correctly validate the new DEBUG environment variable handling, there's a significant amount of duplicated setup code across the four new test cases. The test blocks for DEBUG='false'/'0' and DEBUG='true'/'1' are nearly identical for both the Docker and Seatbelt scenarios.
To improve maintainability and reduce redundancy, consider refactoring these tests to be more DRY (Don't Repeat Yourself). You could either:
- Use a single
it.eachfor each scenario (Docker/Seatbelt): Combine the positive and negative cases into one parameterized test. - Create a setup helper function: Extract the common mocking and setup logic into a reusable function that can be called by each test.
Here's an example of how you could combine the Docker tests:
it.each([
{ debugValue: 'false', shouldPublish: false },
{ debugValue: '0', shouldPublish: false },
{ debugValue: 'true', shouldPublish: true },
{ debugValue: '1', shouldPublish: true },
])('should publish debug port: $shouldPublish when DEBUG=$debugValue', async ({ debugValue, shouldPublish }) => {
// ... common setup logic ...
await start_sandbox(config, [], undefined, ['arg1']);
const dockerArgs = vi.mocked(spawn).mock.calls[1]?.[1];
if (shouldPublish) {
expect(dockerArgs).toContain('--publish');
expect(dockerArgs).toContain('9229:9229');
} else {
expect(dockerArgs).not.toContain('--publish');
expect(dockerArgs).not.toContain('9229:9229');
}
});Applying a similar pattern to the Seatbelt tests would make the test suite more concise and easier to maintain in the future.
References
- When adding new functionality, consolidate it with existing similar logic to avoid duplication and improve maintainability.
There was a problem hiding this comment.
Great catch! Refactored the tests to use a single parameterized it.each block for both Docker and Seatbelt scenarios to eliminate duplication and follow DRY principles. Thanks for the suggestion!
gemini-cli
Bot
added
the
area/platform
f93398b to
f3b3e1d
Compare
|
Hi there! Thank you for your interest in contributing to Gemini CLI. To ensure we maintain high code quality and focus on our prioritized roadmap, we only guarantee review and consideration of pull requests for issues that are explicitly labeled as 'help wanted'. This PR will be closed in 7 days if it remains without that designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
|
This pull request is being closed as it has been open for 14 days without a 'help wanted' designation. We encourage you to find and contribute to existing 'help wanted' issues in our backlog! Thank you for your understanding. |
1 participant
Summary
Normalizes the interpretation of the
DEBUGenvironment variable across sandbox utilities. This prevents string values like"false"and"0"from inadvertently enabling debug features such as port publishing, pull logging, and--inspect-brkexecution pauses in Docker, Podman, and macOS Seatbelt sandboxes.Problem
Previously,
packages/cli/src/utils/sandbox.tsevaluatedprocess.env.DEBUGusing standard JavaScript string truthiness. Because non-empty strings like"false"and"0"evaluate totruein JavaScript, this caused several unintended behaviors:--publish 9229:9229).--inspect-brkintoNODE_OPTIONS, unexpectedly pausing CLI execution.Fix
Introduced a centralized
isDebugModeEnabled()helper insandboxUtils.ts.Instead of relying on string truthiness, this helper strictly checks for explicitly enabled values (
"true","1", or an active CLI debug mode flag). This aligns the sandbox logic with existing behaviors in the entrypoint andconfig.ts.Related Issues
Fixes #28885
Validation
Run the unit tests for the sandbox utilities:
npm test -w @google/gemini-cli -- src/utils/sandboxUtils.test.ts src/utils/sandbox.test.tsVerify that all parameterized test cases pass, specifically covering:
DEBUG=falseDEBUG=0DEBUG=trueDEBUG=1Pre-Merge Checklist
npm run ...)