fix(tools): classify silent wrapped Windows command failures as sandbox denials#659
Conversation
…ox denials A Windows restricted-token sandbox failure is often completely silent: when the token cannot open the target executable or its DLLs (or an MSYS runtime dies during init), the command exits nonzero having written zero bytes to stdout and stderr. The existing detection is keyword-based, so these failures were never marked as sandbox denials, the agent loop never offered the unsandboxed-retry prompt, and the model was left staring at 'Command completed with no output' until it gave up. Treat a wrapped command on the Windows restricted-token or elevated backend that exits nonzero with completely empty output as a likely sandbox denial. A command that legitimately fails silently (findstr with no match, for example) now costs one extra approval prompt; the inverse misclassification silently strands the session, so the trade goes to prompting.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe sandbox denial classifier now detects silent failures from wrapped Windows restricted or elevated backends. Tests verify denial metadata and exclude successful commands, non-Windows backends, and output containing unrelated non-empty sections. ChangesSandbox denial detection
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (1)
internal/tools/sandbox_denial_test.go (1)
45-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for
BackendWindowsElevated.
windowsWrappedBackendtreatsBackendWindowsElevatedidentically toBackendWindowsRestrictedToken, but no test exercises the elevated backend path. A quick table-driven or parameterized test covering both backends would close this gap.♻️ Suggested additional test
func TestLikelySandboxDeniedDetectsSilentWindowsWrappedFailure(t *testing.T) { + for _, backend := range []zeroSandbox.BackendName{ + zeroSandbox.BackendWindowsRestrictedToken, + zeroSandbox.BackendWindowsElevated, + } { plan := zeroSandbox.CommandPlan{ Wrapped: true, - TargetBackend: zeroSandbox.BackendWindowsRestrictedToken, + TargetBackend: backend, } if !likelySandboxDenied(plan, 1, "", " \n") { t.Fatal("wrapped Windows command failing with empty output must be classified as sandbox denied") } meta := map[string]string{} markLikelySandboxDenial(meta, plan, 1, "") if meta[SandboxLikelyDeniedMeta] != "true" || meta[SandboxDenialKindMeta] != SandboxDenialKindSandbox { t.Fatalf("silent windows denial meta = %#v", meta) } + } }🤖 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 45 - 89, Add coverage for BackendWindowsElevated in the likelySandboxDenied tests, preferably by parameterizing the Windows restricted-token and elevated backend cases. Verify the elevated backend matches the existing silent wrapped failure classification and metadata behavior while preserving the existing success and unrelated-output expectations.
🤖 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.
Nitpick comments:
In `@internal/tools/sandbox_denial_test.go`:
- Around line 45-89: Add coverage for BackendWindowsElevated in the
likelySandboxDenied tests, preferably by parameterizing the Windows
restricted-token and elevated backend cases. Verify the elevated backend matches
the existing silent wrapped failure classification and metadata behavior while
preserving the existing success and unrelated-output expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39324bf0-60c3-428d-a223-8c10747881a5
📒 Files selected for processing (2)
internal/tools/sandbox_denial.gointernal/tools/sandbox_denial_test.go
…test The heuristic treats windows-elevated identically to windows-restricted-token, but only the latter was exercised. Parameterize the silent-failure test over both backends.
|
Pushed 5d726af addressing the CodeRabbit suggestion: TestLikelySandboxDeniedDetectsSilentWindowsWrappedFailure is now parameterized over both windows-restricted-token and windows-elevated, so the elevated arm of windowsWrappedBackend has coverage too. No production code changes. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed the silent-denial classification. The heuristic is tight: it only fires for wrapped commands on the Windows restricted-token/elevated backends with a nonzero exit AND completely empty stdout+stderr, and the exitCode==0 early return (sandbox_denial.go:55) keeps a real success from ever being mislabeled as a denial. A genuine silent sandbox denial now gets the unsandboxed-retry prompt instead of stranding the session, and the only over-classification cost is one extra approval prompt for legitimately silent-failing wrapped commands like a no-match findstr — which the comment owns explicitly. Tests cover the success, empty-output, other-backend, and has-output cases; build/vet/gofmt/tools tests pass and it merges clean.
One minor note: BackendWindowsElevated is bundled into windowsWrappedBackend, but an elevated-token command isn't really "denied" by the sandbox, so a silent failure there is less clearly a denial — at worst it yields a spurious retry prompt, low harm, but worth a glance if you want to narrow it to the restricted-token backend only.
|
On the BackendWindowsElevated note: it's currently unreachable as a TargetBackend value. internal/sandbox/manager.go only constructs a Windows backend via nativeBackend(goos, BackendWindowsRestrictedToken, ...); there's no call site that produces one named BackendWindowsElevated. buildPlatformCommandPlan's switch also only has a case for BackendWindowsRestrictedToken, so anything else falls through to nativeSandboxUnavailableError before a command plan is ever built. Given that, narrowing windowsWrappedBackend() to restricted-token only wouldn't change any live behavior, and it would break the pattern used elsewhere (manager.go, adapters.go, platformForBackendName) of treating the two Windows backends as a matched pair. Leaving it bundled and covering it with the test added in 5d726af seemed like the better trade: no behavior change now, and it stays correct if the elevated path ever gets wired up to produce real command plans. |
3 participants
Problem
A Windows restricted-token sandbox failure is often completely silent. When the token cannot open the target executable or one of its DLLs (see #658), or an MSYS runtime dies during init, the wrapped command exits nonzero having written zero bytes to stdout and stderr. The denial detection in internal/tools/sandbox_denial.go is keyword-based, so these failures are never marked as sandbox denials. The agent loop then never offers the unsandboxed-retry prompt, and the model is left retrying "Command completed with no output" until it gives up. This is exactly the failure mode that stranded a real session: six consecutive gh/git calls returned exit 1 with empty output and no diagnostic.
Fix
Treat a wrapped command on the Windows restricted-token or elevated backend that exits nonzero with completely empty combined output as a likely sandbox denial (kind: sandbox). This routes the result into the existing escalation path, so the user gets the retry-unsandboxed prompt instead of a bare failure.
Trade-off: a command that legitimately fails silently (findstr with no match, for example) now costs one extra approval prompt. The inverse misclassification silently strands the whole session, so the trade goes to prompting. Other backends are unaffected: bwrap and seatbelt failures are not silent in this way, and the existing keyword and seccomp checks are unchanged.
Verification
New unit tests cover the silent-failure classification, the exit-0 and non-Windows non-cases, and a failure that produced output.
go build ./...andgo test ./internal/tools ./internal/agent ./internal/sandboxpass.Complementary to #658: that PR fixes the most common cause of these silent failures, this one makes any remaining ones (MSYS init deaths, Schannel-style limitations, DenyRead-configured profiles that keep the fully restricted token) recoverable instead of dead ends.
Summary by CodeRabbit