fix(cli): fall back to embedded macOS seatbelt profiles if missing - #28551
fix(cli): fall back to embedded macOS seatbelt profiles if missing#28551amelidev wants to merge 10 commits into
Conversation
|
📊 PR Size: size/L
|
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 resolves a critical startup crash in the Gemini CLI's sandbox mode on macOS, which occurred when static Seatbelt profiles were missing from the bundle. By embedding these profiles directly into the source code, the CLI can now dynamically generate the necessary sandbox configuration at runtime, ensuring consistent behavior across different deployment environments. 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
|
There was a problem hiding this comment.
Code Review
This pull request implements a fallback mechanism for the macOS seatbelt sandbox, writing embedded profile contents to a temporary file when the profile is missing on disk, and adds corresponding unit tests. Feedback on these changes highlights a potential command injection vulnerability when constructing the shell command with unescaped NODE_OPTIONS. Additionally, the feedback advises against registering custom SIGINT and SIGTERM handlers for temporary file cleanup, as they can hijack termination signals and prevent the CLI process from exiting properly, suggesting to rely solely on the exit event instead.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a fallback mechanism in the macOS sandbox to use embedded seatbelt profiles when the .sb files are not present on disk. It introduces a new file, sandboxBuiltinProfiles.ts, containing the predefined seatbelt profiles, and updates sandbox.ts to write these profiles to a temporary file and handle their cleanup. Additionally, a new unit test has been added to verify this fallback behavior. There are no review comments to address, and the changes look solid.
|
Overall LGTM, but I would like to see two minor enhancements before approval and merging. 💡 Recommendation 1: Restrict File Permissions on Temp Profile FileWhen writing temporary sandbox profiles to shared OS temp directories ( fs.writeFileSync(tempProfileFile, content, {
encoding: 'utf8',
mode: 0o600,
});💡 Recommendation 2: Handle
|
…failures under localized OS environments
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for falling back to embedded macOS seatbelt profiles when the physical .sb files are missing on disk (e.g., in bundled or bazel environments). It writes the embedded profile contents to a temporary file, registers cleanup handlers, and adds corresponding unit tests. Additionally, it configures the 'en' locale for yargs in several test files. The review feedback highlights a critical issue where registering cleanup handlers directly to SIGINT and SIGTERM signals hijacks them, preventing clean process termination. The reviewer suggests implementing dedicated signal handlers that perform the cleanup and then re-kill the process with the respective signal to allow standard termination.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces embedded macOS seatbelt profiles to allow the sandbox to function when profile files are missing from disk, writing them to temporary files and managing their cleanup. It also updates CLI tests to use the English locale for consistent yargs validation. The review feedback highlights critical improvements for sandbox execution: resolving a POSIX shell compatibility issue where the entire NODE_OPTIONS assignment is incorrectly quoted, and unifying the signal handling and cleanup logic to prevent cascading signal delivery and redundant proxy termination code.
…ompatibility for NODE_OPTIONS
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a fallback mechanism for macOS sandbox execution when seatbelt profile files (.sb) are missing from the disk (such as in bundled or Bazel environments). It adds a new file sandboxBuiltinProfiles.ts containing the embedded seatbelt profile contents and updates sandbox.ts to write these contents to a temporary file when necessary. It also implements robust cleanup logic for these temporary files and proxies using process signal handlers (SIGINT, SIGTERM, exit). Additionally, unit tests are updated to cover this fallback behavior, and several CLI command tests are updated to explicitly set the yargs locale to English. I have no feedback to provide as there are no review comments.
|
Overall, the architecture for embedded fallback profiles, file mode restriction ( 🔴 Blocking Issue1. Mismatch between
|
Summary
This PR resolves the critical startup crash when running Gemini CLI in sandbox mode (
-s) on macOS/gMac environments where static Seatbelt.sbprofiles are not found in the runfiles or bundle folder.Details
.sbseatbelt profiles) are often stripped or omitted from the deployed bundle (e.g.bundle/gemini.jsorgemini.mjsin Bazel/google3) because they are not traced by standard JS bundlers/packagers.sandboxBuiltinProfiles.ts.fs.existsSync(profileFile)returnsfalseat runtime, we fall back to writing the embedded profile string to a unique, randomized temporary file underos.tmpdir()(gemini-sandbox-macos-${profile}-${rand}.sb).'exit'event (to avoid signal hijacking / process lockup of SIGINT/SIGTERM handlers) and on spawned child process termination (close,error) to cleanly unlink the temporary file.NODE_OPTIONSQuoting): Escaped/quoted theNODE_OPTIONSargument string via thequoteutility inside thesh -cinvocation to neutralize potential shell command injection vulnerabilities..sbfile is missing.Related Issues
Resolves the issue reported in
issue.md.How to Validate
We have added robust unit test coverage to verify this fallback behavior. Run the targeted tests for the touched module:
npm test -w @google/gemini-cli -- src/utils/sandbox.test.tsAll 22 tests (including our new fallback test) compile, pass, and satisfy all linting rules cleanly!
Pre-Merge Checklist