fix(sandbox): preserve user config and retry denied writes#801
fix(sandbox): preserve user config and retry denied writes#801anandh8x wants to merge 4 commits into
Conversation
Keep user identity and configuration paths discoverable while retaining managed cache and temporary storage. Classify native sandbox failures as typed recoverable denials so approved commands can retry outside the sandbox. Add regression coverage for global Git ignores, typed denial handling, and permission retry behavior.
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change reduces synthetic sandbox runtime paths while preserving caller configuration, and adds structured inference and retry handling for recoverable platform sandbox denials detected from command output. ChangesSandbox runtime environment
Structured sandbox denials
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Command
participant DenialInference
participant ExecutionOutcome
participant AgentRetry
Command->>DenialInference: return failed sandboxed output
DenialInference->>ExecutionOutcome: attach structured platform denial
ExecutionOutcome->>AgentRetry: provide recoverable approval request
AgentRetry->>Command: trigger approval-based retry
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/tools/sandbox_denial_test.go (1)
28-38: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd false-positive regression cases for the keyword heuristic.
The table covers obvious denial strings but not ordinary output containing
"sandbox","failed to write file", or unrelated permission/read-only errors. Add those cases and assert that both denial metadata and the returned denial remain unset.As per coding guidelines,
**/*_test.gofiles should add regression tests for behavior changes; this heuristic needs explicit false-positive coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/sandbox_denial_test.go` around lines 28 - 38, Expand the test table in the markLikelySandboxDenial test to include sandboxed commands whose output contains “sandbox”, “failed to write file”, and unrelated permission or read-only errors. Set each case to expect no denial, and assert both the SandboxLikelyDeniedMeta metadata and the returned denial remain unset.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/loop.go`:
- Around line 1622-1626: Update the denial predicate in the surrounding loop
logic to also require the typed platform denial’s Recoverable field to be true
before returning true. Preserve the existing source, capability, and next-action
checks so non-recoverable denials cannot enter the unrestricted retry or
host-escalation flow.
In `@internal/sandbox/runtime_state_test.go`:
- Line 27: Extend the runtime state filesystem assertions around the existing
Cache, Data, and Temp loop to verify the obsolete synthetic home, config, and
state paths are absent. Add regression coverage using the test’s established
assertion style, while preserving the existing existence checks for the active
directories.
- Around line 226-229: Update the environment setup in the runtime-state Git
test to remove inherited GIT_* override variables from os.Environ(), including
GIT_CONFIG_GLOBAL, GIT_DIR, and GIT_CONFIG_COUNT, and disable system Git
configuration before invoking Git. Add regression coverage verifying these
host-level settings cannot override the test’s HOME and XDG_CONFIG_HOME
configuration.
In `@internal/tools/sandbox_denial.go`:
- Around line 9-17: Narrow sandbox-denial detection to require platform-specific
evidence before treating generic keywords such as “sandbox” or “failed to write
file” as denials; update the heuristic in internal/tools/sandbox_denial.go lines
9-17 and only construct unrestricted host denial metadata in lines 35-54 after
stronger corroboration. Add regression cases in
internal/tools/sandbox_denial_test.go lines 28-38 covering generic application,
permission, and read-only errors, asserting neither denial metadata nor a
pointer is produced.
---
Nitpick comments:
In `@internal/tools/sandbox_denial_test.go`:
- Around line 28-38: Expand the test table in the markLikelySandboxDenial test
to include sandboxed commands whose output contains “sandbox”, “failed to write
file”, and unrelated permission or read-only errors. Set each case to expect no
denial, and assert both the SandboxLikelyDeniedMeta metadata and the returned
denial remain unset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 289f6db2-c754-4b9c-91f7-df23c5876269
📒 Files selected for processing (9)
internal/agent/loop.gointernal/agent/loop_test.gointernal/sandbox/runtime_state.gointernal/sandbox/runtime_state_test.gointernal/tools/bash.gointernal/tools/exec_command.gointernal/tools/exec_command_test.gointernal/tools/sandbox_denial.gointernal/tools/sandbox_denial_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tools/sandbox_denial_test.go`:
- Around line 39-42: Expand the assertions in the markLikelySandboxDenial test
to validate the complete denial structure, not just whether denial is non-nil.
Check Source, unrestricted Capability, Recoverable, NextAction, and the
reason/keyword metadata used by the retry prompt, using the expected values from
each test case and preserving the existing nil/non-nil assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b8019c76-b4ef-4dd4-84a2-3173d8acaf94
📒 Files selected for processing (5)
internal/agent/loop.gointernal/agent/loop_test.gointernal/sandbox/runtime_state_test.gointernal/tools/sandbox_denial.gointernal/tools/sandbox_denial_test.go
💤 Files with no reviewable changes (1)
- internal/tools/sandbox_denial.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/agent/loop.go
- internal/agent/loop_test.go
- internal/sandbox/runtime_state_test.go
|
Windows Smoke is currently blocked by the existing TestLoadProviderCommandTimeout startup-timing flake in internal/config/command_test.go: the test tries to read sleep.pid before the Windows helper creates it. The sandbox, agent, and tools packages pass on Windows, and this PR does not change internal/config. The same job passed once on c2429d0, then reproduced the identical missing sleep.pid failure twice after the test-only 3b9be60 commit. The scoped fix is already in #800; that PR should land independently rather than mixing its unrelated test changes into this sandbox PR. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
The fix does what it claims. A native-sandbox read-only / permission-denied shell failure now produces an ExecutionOutcome.Denial (Source=PlatformSandbox, Capability=Unrestricted, Recoverable=true, NextAction=RequestApproval) that matches the narrowed sandboxDeniedShellResult predicate in internal/agent/loop.go, so the command re-enters the unrestricted retry path. Wiring is coherent end-to-end: markLikelySandboxDenial (internal/tools/sandbox_denial.go) → execExecutionOutcome (exec_command.go:439/554) → StateDenied → loop.go retry prompt. PR-added tests pass on the PR HEAD (TestExecCommandInfersNativeSandboxDenialFromOutput, TestLikelySandboxDenialMetadataFromCommandOutput, TestExecCommandUsesStructuredAdapterDenial, TestTypedExecutionOutcomeOverridesLegacySandboxHeuristics, and the sandbox runtime/env-preservation tests). gofmt/vet/build clean.
Two minor issues, neither blocking merge.
[Minor] Non-sandbox "permission denied" errors are misclassified as recoverable sandbox denials — PR-introduced
internal/tools/sandbox_denial.go:40
The heuristic matches the bare substring "permission denied" in stderr whenever the command ran sandboxed and exited non-zero (bash.go:184-191 / exec_command.go:440-447 gate only on sandboxed && exitCode != 0). Ordinary OS permission failures therefore get classified as recoverable sandbox denials:
git push git@github.com:foo/barwith no valid key →git@github.com: Permission denied (publickey)., exit 128cat /etc/shadow→cat: /etc/shadow: Permission denied, exit 1- any
sudo ...invocation that is denied
Each yields Denial{Kind:CapabilityUnrestricted, Source:PlatformSandbox, Recoverable:true, NextAction:RequestApproval}, so maybeRetryUnsandboxedAfterSandboxRestriction (loop.go:1453) prompts the user to re-run unsandboxed with reason "sandbox blocked command execution". Because the sandbox never caused the failure, the unsandboxed retry hits the identical OS error (SSH auth / file mode / sudo) and the user is shown a factually wrong "sandbox blocked" reason. All three sample strings were confirmed to match the keyword heuristic.
Fix: require a corroborating signal before firing on "permission denied" — e.g. only when the target is a known-restricted write root or the denial monitor/adapter also flagged a block — or drop the broad "permission denied" keyword and rely on the sandbox-specific signals (EROFS "read-only file system", seccomp, landlock) plus structured adapter denials.
[Minor] bash.go sandbox-denial inference is untested on the real shell tool path — PR-introduced
internal/tools/bash.go:184
Deleting the entire if adapterReport.Denial == nil && reportErr == nil { adapterReport.Denial = markLikelySandboxDenial(...) } block (bash.go:184-192) produces zero new failures across go test ./internal/tools/... and go test ./internal/agent/... — only the pre-existing environment failures remain. So a future edit that breaks this wiring would silently disable sandbox-denied-write retry for real bash tool commands (e.g. touch "$HOME/probe" → "Read-only file system" under native enforcement no longer produces a Denial, and loop.go's retry never fires) with the suite staying green. The existing coverage doesn't reach this path: TestExecCommandInfersNativeSandboxDenialFromOutput drives exec_command.go, and the agent-level TestRunRetriesShellUnsandboxedAfterSandboxDeniedExit uses a fake tool that hand-builds the ExecutionOutcome, so neither exercises bash.go's plan.Wrapped && plan.EnforcementLevel == EnforcementNative guard or its stdout+stderr passing.
Fix: add a bash-tool-level test analogous to the exec_command one that runs the real bash tool with a native-wrapped plan and a stderr denial keyword, asserting result.ExecutionOutcome.Denial is populated and SandboxLikelyDeniedMeta is set.
Tests
All failures in the three touched packages (3 in internal/tools, 7 in internal/sandbox, 4 in internal/agent) are pre-existing environment failures — the known sandbox/path/symlink issues from the worktree living under /private/tmp. The --- FAIL name sets are byte-for-byte identical between the PR HEAD and the clean base worktree, so none is PR-attributable. -race not run (this is not a concurrency PR).
Merge is kevin's call per the program gate.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Good change in the main line: pointing HOME and the XDG vars back at the caller's real paths so sandboxed tooling sees normal user settings is the right call, and I checked the thing I was most worried about before anything else. It does not widen the read boundary. profile.go is untouched, the posture there is still the deliberate read-all/write-jail one, and credentialDenyReadPaths resolves from os.UserHomeDir() in the Zero process rather than the child's $HOME, so the deny masks still land on the same real paths after the env change. Writes are actually tighter than before, since the old synthetic HOME sat inside the runtime root and was therefore writable, while the real home is in no write root. So no credential regression, and #675/#677 are neither helped nor hurt.
Two things I would like fixed before this lands.
1. [blocker] The sandbox denial is inferred from the command's own stdout/stderr again, and it now drives an unsandboxed re-run.
markLikelySandboxDenial (internal/tools/sandbox_denial.go) scans the command output for operation not permitted, permission denied, read-only file system, seccomp, landlock and mints a real execution.Denial{Source: DenialSourcePlatformSandbox, Capability: {Kind: CapabilityUnrestricted, Scope: "host"}, Recoverable: true, NextAction: DenialNextActionRequestApproval}. It sits directly under the comment // Classification is never inferred from stdout or stderr. that #781 added when it deleted this exact heuristic.
That matters more than it did before #781, because loop.go now keys the unsandboxed-retry predicate on precisely that denial shape, and this function is its only producer. So the retry path fires on, and only on, stdout-inferred denials. The strings are generic enough to be produced by ordinary commands: a failing test, a linter, or cat on a root-owned file all print permission denied without the sandbox being involved at all. Since tool output is model- and content-controllable, a command can effectively ask Zero to offer to re-run it without the sandbox.
To be fair on severity: in ask and auto modes the retry is fail-closed and requires a fresh human Allow, so this is not a silent bypass there. But under PermissionModeUnsafe it is taken with no prompt, and the retry resolves to a nil engine, which means ModeDisabled, NetworkAllow, FileSystemUnrestricted and no DenyRead at all, so the credential masks that are the whole safety story above are gone on that path.
The fix I would want is to keep the classification typed: derive the denial from the sandbox adapter's own facts (which is what markStructuredSandboxDenial already does) and let the keyword scan at most annotate a denial that the sandbox layer already reported, never create one. If a heuristic is genuinely needed for a backend that cannot report denials structurally, it should produce a non-recoverable, non-retry-eligible marker rather than one that unlocks the unsandboxed path.
2. [minor] CARGO_HOME looks like collateral damage.
The PR removes data/cargo and the CARGO_HOME override as unused, but it is the only place in the tree that sets it, and cargo does not honour XDG_CACHE_HOME. Every sibling redirect survives (npm, yarn, corepack, pip, GOCACHE, GOMODCACHE), so cargo is the one toolchain left pointing at $HOME/.cargo, which is in no write root. That should make cargo builds fail inside the sandbox rather than land in the managed cache. Worth restoring unless you found it genuinely dead.
Verification note: Smart App Control on my machine is currently blocking the unsigned go toolchain, so this review is source-reading only; I could not run builds or tests locally. CI is green on all three OSes.
Happy to re-review quickly once the denial classification is back to typed facts.
3 participants
Summary
Behavior
Sandboxed commands now honor user configuration such as Git global ignore rules. Writes outside approved roots remain read-only by default. When a native sandbox blocks a command, Zero surfaces the denial reason and retries only after explicit user approval.
Verification
make fmt-checkgo vet ./...go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokegovulncheck ./...— no vulnerabilities foundgit diff HEAD --checkThe pinned advisory linter reports only existing repository-wide findings outside the changed files.
Summary by CodeRabbit