fix(sandbox): treat indeterminate availability probes as non-fatal - #94
Conversation
Warning Review limit reached
Next review available in:26 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughSandbox daemon queries now use 30-second timeouts. Timed-out availability checks return ChangesSandbox availability handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SandboxManager
participant SandboxCLI
participant Daemon
SandboxManager->>SandboxCLI: Query daemon status
SandboxCLI->>Daemon: Request status
Daemon-->>SandboxCLI: Respond or exceed 30-second limit
SandboxCLI-->>SandboxManager: Available, unavailable, or unknown
SandboxManager-->>SandboxManager: Continue or report definitive failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/sandbox/manager.ts`:
- Around line 230-235: Update ensureTemplate in src/sandbox/manager.ts:230-235
to return when checkAvailable reports reason === 'unknown', throwing only for
definitive unavailable results. Add the corresponding regression case in
test/sandbox/manager-caching.test.ts:100-109 where templateExists is false and
the recheck is unknown, asserting startup reaches createSandbox.
In `@src/sandbox/process.ts`:
- Around line 20-24: Extend CommandResult with an explicit timedOut flag in
src/sandbox/process.ts, setting it only when runCommand terminates the child
through either timeout path; retain exit code 124 as a normal command result
otherwise. Update checkSbxAvailability in src/sandbox/sbx.ts to classify only
result.timedOut === true as indeterminate. Apply the equivalent explicit timeout
handling in src/sandbox/template.ts instead of treating every Docker exit code
124 as a timeout, and update both sandbox test
suites—src/sandbox/process.ts-related tests and
test/sandbox/sbx-runtime.test.ts—to cover timed-out results and normal exit code
124.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3eb74284-7b41-4c2e-92bc-15d1fe5b95cc
📒 Files selected for processing (7)
docs/sandbox.mdsrc/index.tssrc/sandbox/manager.tssrc/sandbox/process.tssrc/sandbox/sbx.tstest/sandbox/manager-caching.test.tstest/sandbox/sbx-runtime.test.ts
Uh oh!
There was an error while loading. Please reload this page.
| /** | ||
| * Exit code reported when `runCommand` itself killed the child (timeout or hard deadline) rather | ||
| * than the command exiting on its own. Callers must treat it as "no answer", never as a result. | ||
| */ | ||
| export const COMMAND_TIMEOUT_EXIT_CODE = 124 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
# Map the command-result contract before updating its producers and consumers.
ast-grep outline src/sandbox --items all --type interface,function
# Find all command-result construction and timeout classification sites.
rg -n -C 3 'CommandResult|COMMAND_TIMEOUT_EXIT_CODE|exitCode:\s*' src testRepository: chriswritescode-dev/opencode-forge
Length of output: 37200
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- process.ts ---'
cat -n src/sandbox/process.ts | sed -n '1,135p'printf'%s\n''--- sbx.ts availability and runtime ---'
cat -n src/sandbox/sbx.ts | sed -n '235,315p'
cat -n src/sandbox/sbx.ts | sed -n '380,455p'printf'%s\n''--- template.ts error handling ---'
cat -n src/sandbox/template.ts | sed -n '35,75p'printf'%s\n''--- manager availability/template path ---'
rg -n -C 8 'checkAvailable|available|reason|template|unknown|missing' src/sandbox/manager.ts test/sandbox/manager-caching.test.ts
printf'%s\n''--- all explicit timeout-code references ---'
rg -n -C 4 'COMMAND_TIMEOUT_EXIT_CODE|exitCode\s*===\s*124|exitCode\s*:\s*124' src testprintf'%s\n''--- CommandResult type and construction sites ---'
rg -n -C 2 'CommandResult|Promise<CommandResult>|return \{[^}]*exitCode|exitCode:' src/sandbox test/sandbox | head -n 500Repository: chriswritescode-dev/opencode-forge
Length of output: 50390
🏁 Script executed:
#!/bin/bashset -euo pipefail
python3 - <<'PY'from pathlib import Pathimport refiles = [ Path("src/sandbox/process.ts"), Path("src/sandbox/sbx.ts"), Path("src/sandbox/template.ts"), Path("src/sandbox/manager.ts"),]for path in files: text = path.read_text() print(f"--- {path} ---") for i, line in enumerate(text.splitlines(), 1): if ("COMMAND_TIMEOUT_EXIT_CODE" in line or "timedOut" in line or "checkSbxAvailability" in line or "checkAvailable" in line or "template" in line.lower() and "Exists" in line): print(f"{i}: {line}")PYRepository: chriswritescode-dev/opencode-forge
Length of output: 1394
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- template implementation and tests ---'
cat -n src/sandbox/template.ts | sed -n '1,130p'
cat -n test/sandbox/template.test.ts | sed -n '115,150p'printf'%s\n''--- exact numeric timeout-code consumers ---'
rg -n -C 6 'COMMAND_TIMEOUT_EXIT_CODE|exitCode\s*===\s*124|exitCode\s*!==\s*124' src testprintf'%s\n''--- availability and manager tests ---'
cat -n test/sandbox/sbx-runtime.test.ts | sed -n '260,315p'
cat -n test/sandbox/manager-caching.test.ts | sed -n '75,115p'Repository: chriswritescode-dev/opencode-forge
Length of output: 17413
🏁 Script executed:
#!/bin/bashset -euo pipefail
python3 - <<'PY'from pathlib import Pathimport reprocess = Path("src/sandbox/process.ts").read_text()sbx = Path("src/sandbox/sbx.ts").read_text()template = Path("src/sandbox/template.ts").read_text()checks = { "runCommand exposes only exitCode in CommandResult": bool( re.search(r"export interface CommandResult\s*{[^}]*exitCode:\s*number[^}]*}", process, re.S) and "timedOut" not in re.search(r"export interface CommandResult\s*{[^}]*}", process, re.S).group(0) ), "normal child close maps exit code 124 without timeout state": "exitCode: timedOut ? COMMAND_TIMEOUT_EXIT_CODE : (code ?? 1)" in process, "hard deadline maps to timeout code": "exitCode: COMMAND_TIMEOUT_EXIT_CODE" in process, "availability uses exit code as timeout discriminator": "if (result.exitCode === COMMAND_TIMEOUT_EXIT_CODE)" in sbx, "template error handling uses exit code as timeout discriminator": "if (result.exitCode === 124)" in template,}for name, passed in checks.items(): print(f"{'PASS' if passed else 'FAIL'}: {name}")# Model the currently visible availability branch for the two relevant inputs.def classify(exit_code, stdout="", stderr=""): if exit_code == 0 and re.search(r"^\s*status:\s*running", stdout, re.I | re.M): return "available" combined = f"{stdout}\n{stderr}" if re.search(r"ENOENT|not found|command not found", combined, re.I): return "not-installed" if exit_code == 124: return "unknown" return "daemon-down"print(f"normal exit 124 => {classify(124)!r}")print(f"normal exit 1 => {classify(1)!r}")PYRepository: chriswritescode-dev/opencode-forge
Length of output: 524
Use an explicit timeout signal in CommandResult.
A command can exit normally with code 124. checkSbxAvailability then returns unknown, and the manager continues. Add timedOut, set it for both timeout paths, and classify only result.timedOut === true as indeterminate.
Apply the same change to src/sandbox/template.ts, which currently reports every Docker exit code 124 as a timeout. Update both test suites to cover timed-out results and normal exit code 124.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcess } from 'child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
📍 Affects 3 files
src/sandbox/process.ts#L20-L24(this comment)src/sandbox/process.ts#L99-L99src/sandbox/sbx.ts#L277-L283test/sandbox/sbx-runtime.test.ts#L296-L306
🤖 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 `@src/sandbox/process.ts` around lines 20 - 24, Extend CommandResult with an
explicit timedOut flag in src/sandbox/process.ts, setting it only when
runCommand terminates the child through either timeout path; retain exit code
124 as a normal command result otherwise. Update checkSbxAvailability in
src/sandbox/sbx.ts to classify only result.timedOut === true as indeterminate.
Apply the equivalent explicit timeout handling in src/sandbox/template.ts
instead of treating every Docker exit code 124 as a timeout, and update both
sandbox test suites—src/sandbox/process.ts-related tests and
test/sandbox/sbx-runtime.test.ts—to cover timed-out results and normal exit code
124.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Makes sandbox availability probing tolerant of a busy daemon.
sbx daemon statusandsbx ls --jsonqueue behind in-flight sandbox work, so with several loops running concurrently the old 5s probe could time out against a perfectly healthy daemon — previously that surfaced as a false "Sandbox unavailable" toast and could fail loop launches with "daemon down" remediation advice.Behavior
sbx daemon status/sbx ls --jsonquery bound raised from 5s to 30s (SBX_QUERY_TIMEOUT).unknowninstead ofdaemon-down;checkSbxAvailabilityreports the timeout detail.SandboxManager.startno longer fails a loop launch on an indeterminate probe, and does not cache the result, so the next launch re-probes immediately.sbx template lsmiss re-verifies daemon reachability before advising an image rebuild, so an unreachable-daemon error wins over a misleading "missing template" hint.COMMAND_TIMEOUT_EXIT_CODE(124) exported fromsrc/sandbox/process.tsas the single marker forrunCommand-induced termination.Docs
docs/sandbox.mddocuments the 30s bound and the indeterminate-vs-down semantics.Tests
test/sandbox/manager-caching.test.ts: indeterminate probe starts the sandbox, is not cached, and daemon-down wins over a missing-template read.test/sandbox/sbx-runtime.test.ts: timed-out probe yieldsunknownwith detail; the runner timeout assertion is updated to 30000ms.Validation
pnpm build,pnpm typecheck,pnpm lintclean; sandbox tests pass (69 tests).Summary by CodeRabbit
Bug Fixes
Improvements