fix(sandbox): create sandbox in user workspace instead of temp directory#35
Merged
louisdevzz merged 3 commits intomainfrom Mar 5, 2026
Merged
fix(sandbox): create sandbox in user workspace instead of temp directory#35louisdevzz merged 3 commits intomainfrom
louisdevzz merged 3 commits intomainfrom
Conversation
Fixes #24 - Sandbox was being created in system temp directory (/var/folders/.../T/ on macOS, /tmp/ on Linux) making it difficult to monitor, debug, and clean up. Changes: - Sandbox now created in ~/.zerobuild/workspace/sandbox/ by default - Support ZEROBUILD_SANDBOX_PATH environment variable for custom location - Updated module documentation to reflect new behavior - Improved error messages with full path information Benefits: - Easy to find: Consistent location under user's home directory - Simple monitoring: Users can check ~/.zerobuild/workspace/sandbox/ - Controlled cleanup: Users decide when to clean up, not the OS - Better debugging: Easy to inspect sandbox contents
PR Intake Checks - Warnings (non-blocking)The following are recommendations:
|
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Sandbox implementation src/sandbox/local.rs |
Add ZEROBUILD_SANDBOX_PATH support with validation (reject empty, relative, or containing ..); default to ~/.zerobuild/workspace/sandbox; create sandbox at {base}/zerobuild-sandbox-{uuid}; pre-create npm-related subdirs; enforce safe_join confinement and improve error messages. |
Tests & test scaffolding src/sandbox/...tests*, src/sandbox/local.rs (test additions) |
Add tests covering custom absolute env path, default home fallback, and rejection cases (empty, relative, parent-traversal); serialize env-var–mutating tests and restore env state between tests. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | The PR description is largely incomplete against the required template. It lacks most critical sections: Label Snapshot, Change Metadata, Linked Issue details, Validation Evidence, Security Impact, Privacy/Data Hygiene, Compatibility, i18n, Human Verification, Side Effects, Rollback Plan, and Risks. | Fill out the complete template including Label Snapshot (risk/size/scope labels), Change Metadata (change type, primary scope), all Linked Issue fields, Validation Evidence with command outputs, and required security/compatibility/rollback sections. | |
| Docstring Coverage | Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the primary change: moving sandbox creation from temp directory to user workspace, which is the main objective of the PR. |
| Linked Issues check | ✅ Passed | The PR addresses all core requirements from issue #24: sandbox now created in ~/.zerobuild/workspace/sandbox/ by default, ZEROBUILD_SANDBOX_PATH env var added for customization, paths are consistent and easy to locate, and users have control over cleanup. |
| Out of Scope Changes check | ✅ Passed | All changes are directly scoped to issue #24 objectives: sandbox path configuration, environment variable support, error message enhancements, and comprehensive test coverage for the new behavior. No unrelated changes detected. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
- 📝 Generate docstrings (stacked PR)
- 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
fix/sandbox-workspace-path
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sandbox/local.rs (1)
128-151: Add coverage for new path-selection edge cases.Please add tests for: env var precedence, empty
ZEROBUILD_SANDBOX_PATH, and relative-path rejection. This change is core behavior and currently high-impact if misconfigured.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandbox/local.rs` around lines 128 - 151, Add unit tests around the sandbox path selection logic to cover env var precedence, empty ZEROBUILD_SANDBOX_PATH, and relative-path rejection: write tests that (1) set ZEROBUILD_SANDBOX_PATH to an absolute custom path and assert the code uses that path as sandbox_base and successfully creates sandbox_dir (verify sandbox_dir contains "zerobuild-sandbox-" and is under the provided path), (2) set ZEROBUILD_SANDBOX_PATH to an empty string or remove it and assert the fallback uses HOME/USERPROFILE (i.e., sandbox_base resolves to ~/.zerobuild/workspace/sandbox), and (3) set ZEROBUILD_SANDBOX_PATH to a relative path and assert the creation fails (the code should return an error when attempting to create_dir_all for a non-absolute path); use std::env::set_var/remove_var in tests and check errors from the function that computes sandbox_base/sandbox_dir and calls std::fs::create_dir_all to validate behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sandbox/local.rs`:
- Around line 130-141: The code reads ZEROBUILD_SANDBOX_PATH raw and constructs
sandbox_base directly which allows empty or relative paths; validate and
normalize the env value before using it: in the block that builds sandbox_base
(the ZEROBUILD_SANDBOX_PATH branch) check that the variable is non-empty, reject
absolute paths that are outside the intended root or resolve to parent traversal
(e.g., deny paths with ..), canonicalize or join the value against a known root
and convert to an absolute PathBuf (using std::fs::canonicalize or by rejecting
relative paths and requiring a whitelisted prefix), and return an error if
validation fails; update any code that uses sandbox_base (same scope where
sandbox_base is declared) to rely on the validated/normalized PathBuf.
---
Nitpick comments:
In `@src/sandbox/local.rs`:
- Around line 128-151: Add unit tests around the sandbox path selection logic to
cover env var precedence, empty ZEROBUILD_SANDBOX_PATH, and relative-path
rejection: write tests that (1) set ZEROBUILD_SANDBOX_PATH to an absolute custom
path and assert the code uses that path as sandbox_base and successfully creates
sandbox_dir (verify sandbox_dir contains "zerobuild-sandbox-" and is under the
provided path), (2) set ZEROBUILD_SANDBOX_PATH to an empty string or remove it
and assert the fallback uses HOME/USERPROFILE (i.e., sandbox_base resolves to
~/.zerobuild/workspace/sandbox), and (3) set ZEROBUILD_SANDBOX_PATH to a
relative path and assert the creation fails (the code should return an error
when attempting to create_dir_all for a non-absolute path); use
std::env::set_var/remove_var in tests and check errors from the function that
computes sandbox_base/sandbox_dir and calls std::fs::create_dir_all to validate
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 550b77a2-fe0a-4955-874f-b412ce7c4c05
📒 Files selected for processing (1)
src/sandbox/local.rs
Address code review comments on PR #35: Validation improvements: - Check that ZEROBUILD_SANDBOX_PATH is non-empty - Reject relative paths (require absolute paths) - Reject paths containing parent directory traversal (..) - Return descriptive errors when validation fails Unit tests added: - sandbox_path_uses_custom_absolute_path_from_env - sandbox_path_falls_back_to_default_when_env_not_set - sandbox_path_rejects_empty_env_var - sandbox_path_rejects_relative_path - sandbox_path_rejects_parent_traversal All 21 sandbox tests pass, plus 3139 total tests pass.
PR Intake Checks - Warnings (non-blocking)The following are recommendations:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/sandbox/local.rs (1)
152-160: Consider using the same home/workspace resolver as the config system.Line [153]-Line [160] manually resolves home via
HOME/USERPROFILE. The config layer already centralizes this logic (src/config/schema.rs:3565-3571), so reusing that resolver (orworkspace_dir) would avoid path-behavior drift across modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandbox/local.rs` around lines 152 - 160, The code in src/sandbox/local.rs manually resolves the home directory via env vars; replace that manual logic with the centralized resolver used by the config layer (the workspace_dir function in src/config/schema.rs) to compute the default "~/.zerobuild/workspace/sandbox" path so behavior stays consistent; import or call workspace_dir (or the exported resolver) where the PathBuf is built, join "sandbox" onto the returned workspace path, and propagate or map any errors from workspace_dir instead of re-implementing HOME/USERPROFILE resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/sandbox/local.rs`:
- Around line 662-800: Tests mutate the process-global ZEROBUILD_SANDBOX_PATH
concurrently causing flakiness; serialize them by adding a shared test mutex
(e.g., a static Lazy<Mutex<()>>) and lock it at the start of each test
(sandbox_path_uses_custom_absolute_path_from_env,
sandbox_path_falls_back_to_default_when_env_not_set,
sandbox_path_rejects_empty_env_var, sandbox_path_rejects_relative_path,
sandbox_path_rejects_parent_traversal). For each test, capture the prior env
state with std::env::var_os("ZEROBUILD_SANDBOX_PATH") before changing it, then
set/remove the variable for the test, and ensure you restore the original value
in a finally/cleanup block (or drop guard) after using LocalProcessSandboxClient
and its create_sandbox/kill_sandbox calls so the env is always restored even on
failure.
---
Nitpick comments:
In `@src/sandbox/local.rs`:
- Around line 152-160: The code in src/sandbox/local.rs manually resolves the
home directory via env vars; replace that manual logic with the centralized
resolver used by the config layer (the workspace_dir function in
src/config/schema.rs) to compute the default "~/.zerobuild/workspace/sandbox"
path so behavior stays consistent; import or call workspace_dir (or the exported
resolver) where the PathBuf is built, join "sandbox" onto the returned workspace
path, and propagate or map any errors from workspace_dir instead of
re-implementing HOME/USERPROFILE resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dac9078a-dca2-4ebf-9045-a70566340b0c
📒 Files selected for processing (1)
src/sandbox/local.rs
Address code review feedback on PR #35: Test improvements: - Add shared test mutex (ENV_MUTEX) to serialize tests that mutate ZEROBUILD_SANDBOX_PATH environment variable - Add EnvGuard struct with Drop impl to ensure env var is always restored to original value, even on test failure or panic - All 5 env var tests now use consistent pattern: 1. Lock mutex 2. Save original env state 3. Create EnvGuard for cleanup 4. Set test env var 5. Run test 6. Guard restores env on drop This prevents test flakiness from concurrent test execution and ensures env var state is always cleaned up properly. Also added OsString import for env var handling.
PR Intake Checks - Warnings (non-blocking)The following are recommendations:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/sandbox/local.rs (1)
681-899: Consolidate duplicatedEnvGuardtest helper into one shared utilityThe same
EnvGuardstruct/Drop logic is repeated in each env-var test block. Extracting it once in the test module will reduce duplication and future drift risk.♻️ Suggested refactor
mod tests { use super::*; use std::sync::Mutex; + use std::ffi::OsString; static ENV_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + struct EnvGuard(Option<OsString>); + impl Drop for EnvGuard { + fn drop(&mut self) { + match &self.0 { + Some(val) => std::env::set_var("ZEROBUILD_SANDBOX_PATH", val), + None => std::env::remove_var("ZEROBUILD_SANDBOX_PATH"), + } + } + } + + fn guard_sandbox_env() -> EnvGuard { + EnvGuard(std::env::var_os("ZEROBUILD_SANDBOX_PATH")) + }Then in each env-mutating test:
- let original_env = std::env::var_os("ZEROBUILD_SANDBOX_PATH"); - struct EnvGuard(Option<OsString>); - impl Drop for EnvGuard { ... } - let _env_guard = EnvGuard(original_env); + let _env_guard = guard_sandbox_env();As per coding guidelines: "Extract shared utilities only after repeated, stable patterns appear at least three times (Rule of Three)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sandbox/local.rs` around lines 681 - 899, The tests duplicate the EnvGuard struct/drop logic across multiple sandbox-path tests; extract that into a single shared test helper to reduce duplication. Create a module-scoped or test-module helper named EnvGuard (or rename to TestEnvGuard) and move the Drop impl and constructor usage there, then update each test (those using ENV_MUTEX and calling EnvGuard) to instantiate the shared helper instead of redefining the struct; ensure visibility so LocalProcessSandboxClient tests can use it and keep the same behavior of restoring ZEROBUILD_SANDBOX_PATH on Drop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/sandbox/local.rs`:
- Around line 681-899: The tests duplicate the EnvGuard struct/drop logic across
multiple sandbox-path tests; extract that into a single shared test helper to
reduce duplication. Create a module-scoped or test-module helper named EnvGuard
(or rename to TestEnvGuard) and move the Drop impl and constructor usage there,
then update each test (those using ENV_MUTEX and calling EnvGuard) to
instantiate the shared helper instead of redefining the struct; ensure
visibility so LocalProcessSandboxClient tests can use it and keep the same
behavior of restoring ZEROBUILD_SANDBOX_PATH on Drop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 45d4affa-6c10-4e56-b8dc-781a6c384ab4
📒 Files selected for processing (1)
src/sandbox/local.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #24 - Sandbox was being created in system temp directory, making it difficult to monitor, debug, and clean up.
Changes
Before
After
Benefits
Testing
Checklist
Fixes #24
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests