refactor(core): abstract OsSandboxManager and centralize common logic#25489
refactor(core): abstract OsSandboxManager and centralize common logic#25489
Conversation
|
Hi @ehedlund, thank you so much for your contribution to Gemini CLI! We really appreciate the time and effort you've put into this. We're making some updates to our contribution process to improve how we track and review changes. Please take a moment to review our recent discussion post: Improving Our Contribution Process & Introducing New Guidelines. Key Update: Starting January 26, 2026, the Gemini CLI project will require all pull requests to be associated with an existing issue. Any pull requests not linked to an issue by that date will be automatically closed. Thank you for your understanding and for being a part of our community! |
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 OS-specific sandbox managers by introducing a centralized 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 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 counter productive. 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
|
|
Size Change: -63 B (0%) Total Size: 33.6 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Code Review
This pull request refactors the sandbox management architecture by introducing the AbstractOsSandboxManager base class, which centralizes shared logic for environment sanitization, permission merging, and path resolution. The Linux, macOS, and Windows sandbox managers have been updated to extend this base class, significantly reducing redundancy. Additionally, the PR introduces dedicated modules for shared constants and governance utilities. Review feedback recommends refactoring module-level and static cache variables in the Linux implementation into instance-scoped properties to eliminate global state and potential concurrency issues.
Summary
Refactors the OS-specific sandbox managers to use a centralized
AbstractOsSandboxManagerbase class. This simplifies the addition of new platforms and ensures consistent permission and path resolution logic across Linux, macOS, and Windows.Details
AbstractOsSandboxManagerwhich implements the Template Method pattern for command preparation.LinuxSandboxManager,MacOsSandboxManager, andWindowsSandboxManagerto extend the new base class.sandboxPathUtils.ts.Subtle Behavior Changes
isKnownSafeCommandnow consistently checks both theapprovedToolslist and the OS-specific safe command list. TheapprovedToolscheck is case-insensitive on Windows via theisCaseInsensitive()hook.__read) are translated to their native counterparts (likecat) before the policy lookup (getCommandPermissions). This ensures that persistent permissions are tied to the actual executable being run, which is consistent with previous intent but now more explicitly enforced.isSecretFileimplementation on Windows now supports more robust regex-based matching for patterns like.env.*, improving the accuracy of secret file masking on that platform.initialize()hook allows OS-specific managers to perform asynchronous setup (likeinitializeShellParserson macOS or helper compilation on Windows) reliably at the start of command preparation.Related Issues
N/A
How to Validate
npm test -w @google/gemini-cli-core -- sandbox/npm run test:e2e(specifically those covering tool execution).Pre-Merge Checklist