feat: migrate sandbox driver from Docker to the sbx CLI - #82
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR replaces Docker-based loop sandboxing with an ChangesSBX sandbox migration
Auditor fallback recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant LoopRuntime
participant LoopService
participant LoopsRepo
participant AuditorProvider
LoopRuntime->>AuditorProvider: receive provider-limit failure
LoopRuntime->>LoopService: advance auditor fallback index
LoopService->>LoopsRepo: compare-and-swap fallback index
LoopsRepo-->>LoopService: return updated index
LoopService-->>LoopRuntime: return fallback choice
LoopRuntime->>AuditorProvider: redispatch audit prompt
sequenceDiagram
participant PluginStartup
participant SandboxManager
participant SandboxRuntime
participant SbxCli
PluginStartup->>SandboxRuntime: check availability and template
SandboxManager->>SandboxRuntime: create sandbox with host workspaces
SandboxRuntime->>SbxCli: invoke sbx create
SbxCli-->>SandboxRuntime: return sandbox state
SandboxManager->>SandboxRuntime: execute commands and apply policy
SandboxManager->>SandboxRuntime: remove sandbox
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Replace the Docker-based sandbox runtime with the sbx CLI (sbx create/exec), covering process-level execution, network env passthrough, custom and project mounts, realpath-canonicalized overlapping-workspace detection, the shell-shim and template container build, legacy-docker config warnings, and cleanup orchestration. Regenerates docs and API docs and updates tests.
c21b46b to
b0e4e07CompareThere was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/sandbox-tools.test.ts (1)
64-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake
mockRuntimeconform toSandboxRuntime.mockRuntimedefines onlyexec, butSandboxContext.runtimerequires allSandboxRuntimemethods. Add a complete test double or an explicit cast at lines 64–65 and 382–383.pnpm typecheckexcludestest/, so it will not catch these errors.🤖 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 `@test/sandbox-tools.test.ts` around lines 64 - 70, Update mockRuntime in sandbox-tools.test.ts to satisfy the complete SandboxRuntime interface before assigning it to SandboxContext.runtime. Prefer adding test-double implementations for every required SandboxRuntime method, or explicitly cast it at both the assignments around lines 64–65 and 382–383, while preserving the existing exec behavior.Source: Path instructions
🧹 Nitpick comments (10)
src/tui.tsx (2)
207-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
SANDBOX_TEMPLATE_BUILD_COMMANDfor the dialog heading.The heading duplicates the constant value
'Build sandbox template'. The constant is already imported at Line 11. Reusing it keeps the palette entry and the dialog heading in sync.♻️ Proposed change
<text fg={theme().text}> - <b>Build sandbox template</b>+ <b>{SANDBOX_TEMPLATE_BUILD_COMMAND}</b> </text>As per coding guidelines: "use
SANDBOX_TEMPLATE_BUILD_COMMANDas the shared build-template command-palette title".🤖 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/tui.tsx` around lines 207 - 208, Replace the hardcoded “Build sandbox template” dialog heading in the relevant TUI component with the imported SANDBOX_TEMPLATE_BUILD_COMMAND constant, keeping the palette entry and heading synchronized.Source: Coding guidelines
183-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord build output instead of discarding it.
The injected logger discards every message. A failed
docker buildorsbx template loadthen leaves only the thrown message in a toast, with no log trail for the user. Route the messages toconsole.error, or capture them and append the tail to the error toast.♻️ Proposed change
- const logger = { log: () => {}, error: () => {}, debug: () => {} }+ const logger = {+ log: (message: string) => console.error(`[forge:sandbox-template] ${message}`),+ error: (message: string, ...args: unknown[]) => console.error(`[forge:sandbox-template] ${message}`, ...args),+ debug: () => {},+ }🤖 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/tui.tsx` around lines 183 - 191, Update the logger injected in the buildAndLoadSandboxTemplate call to preserve build and template-load output instead of using no-op log, error, and debug handlers. Route these messages to console.error, or capture the relevant output and append its tail to the error toast while retaining the existing failure handling.src/sandbox/sbx.ts (1)
375-389: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude
stdoutin the create and remove failure messages.
loadTemplateat Line 355 usesresult.stderr || result.stdout.createSandboxandremoveSandboxuseresult.stderronly. Ifsbxreports the failure on stdout, the thrown message is empty. That message is surfaced to the user through the manager and the TUI toast.♻️ Proposed change
if (result.exitCode !== 0) { - throw new Error(`Failed to create sandbox: ${result.stderr}`)+ throw new Error(`Failed to create sandbox: ${result.stderr || result.stdout}`) } } async function removeSandbox(name: string): Promise<void> { let result: CommandResult try { result = await run(['rm', '--force', name]) } finally { invalidateSandboxList() } if (result.exitCode !== 0 && !SBX_REMOVE_MISSING_RE.test(`${result.stdout}\n${result.stderr}`)) { - throw new Error(`Failed to remove sandbox: ${result.stderr}`)+ throw new Error(`Failed to remove sandbox: ${result.stderr || result.stdout}`) }🤖 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/sbx.ts` around lines 375 - 389, Update the failure messages in createSandbox and removeSandbox to use result.stderr || result.stdout, matching loadTemplate, so errors reported only on stdout are included in the thrown Error messages.test/sandbox/template.test.ts (1)
15-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCover the
docker savefailure stage.
makeFakeRunreturns the same result for every command, so a non-zero exit always fails at the build stage. The save-stage error path inbuildAndLoadSandboxTemplatehas no coverage. Add a per-command result sodocker buildsucceeds anddocker savefails.💚 Proposed change
function makeFakeRun( record: Array<{ command: string; args: string[] }>, result?: { exitCode?: number; stdout?: string; stderr?: string }, + saveResult?: { exitCode?: number; stdout?: string; stderr?: string }, ): BuildTemplateDeps['runCommand'] { return async (command: string, args: string[]) => { record.push({ command, args }) if (command === 'docker' && args[0] === 'save') { const outIdx = args.indexOf('-o') if (outIdx !== -1) writeFileSync(args[outIdx + 1], 'fake-tar') + if (saveResult) {+ return {+ stdout: saveResult.stdout ?? '',+ stderr: saveResult.stderr ?? '',+ exitCode: saveResult.exitCode ?? 0,+ }+ } }Do you want me to generate the full save-failure test case?
🤖 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 `@test/sandbox/template.test.ts` around lines 15 - 31, Update makeFakeRun to support command-specific results, allowing docker build to return success while docker save returns a non-zero exit code. Add or adjust coverage for buildAndLoadSandboxTemplate so the save-stage failure path is exercised and asserted separately from build-stage failures.test/sandbox/manager-network-allow.test.ts (1)
18-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInject
createFakeGitServicein these tests.The first
startcall for each manager invokesrevParseGitDirandrevParseGitCommonDirthroughdefaultGitService. Pass a fake GitService with non-OK results to avoid spawning Git and depending on the host installation.🤖 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 `@test/sandbox/manager-network-allow.test.ts` around lines 18 - 22, Update the manager setup in these tests to inject createFakeGitService with non-OK revParseGitDir and revParseGitCommonDir results into createSandboxManager, ensuring the first start call uses the fake service instead of defaultGitService and does not invoke the host Git installation.src/sandbox/template.ts (1)
47-47: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMake the temp tar path unique per invocation.
process.pidis constant for the plugin process. Two overlapping build actions share one tar path, and the firstfinallydeletes the file the second still needs. Add a random or timestamp suffix.♻️ Proposed change
- const tarPath = join(deps.tmpDir, `forge-sandbox-template-${process.pid}.tar`)+ const tarPath = join(+ deps.tmpDir,+ `forge-sandbox-template-${process.pid}-${Date.now().toString(36)}.tar`,+ )🤖 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/template.ts` at line 47, Update the tarPath construction near the sandbox template generation to include a per-invocation random or timestamp-based suffix in addition to process.pid. Preserve the existing temporary-directory location and filename prefix while ensuring overlapping build actions cannot share or delete the same archive.test/sandbox/context.test.ts (1)
81-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe test title contradicts this assertion.
The test is named "returns null without calling ensureRunning when no worktreeDir", but line 81 asserts a fully populated context. Rename the test to describe the real behavior, for example "resolves the context from getActive without calling ensureRunning when no worktreeDir".
🤖 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 `@test/sandbox/context.test.ts` around lines 81 - 82, Rename the test containing the context equality assertion and ensureRunning verification to describe its actual behavior: resolving context from getActive without calling ensureRunning when no worktreeDir is provided. Keep the test implementation unchanged.container/Dockerfile (1)
64-65: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffConsider
chowntoagentinstead ofchmod 0777now that the exec user is fixed.The
0777rationale is that the container can run as an arbitrary host UID. Under sbx the manager does not pass-u, and line 106 setsUSER agent, so the exec UID is known.chown -R agent:agent /opt/forgewith0755(and0775where the store must be group-writable) grants the same access and removes world-writable caches and globally linked binaries.If some path still runs as an unpredictable UID, keep
0777and record that reason in the comment.Also applies to: 79-80, 98-98
🤖 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 `@container/Dockerfile` around lines 64 - 65, Replace the world-writable permissions in the /opt/forge setup, including the corresponding paths at the other referenced locations, with ownership assigned to agent:agent and restrictive 0755 permissions, using 0775 only for stores that require group write access. Preserve 0777 only where an unpredictable UID still requires it, and document that exception inline.src/sandbox/manager.ts (1)
254-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
isSameOrDescendantPathfor the containment checks.Lines 256, 272, 292 and 296 repeat the
x === y || x.startsWith(y + '/')rule thatisSameOrDescendantPathinsrc/sandbox/path.tsalready implements.hostPathsOverlapalso calls it. Route these three sites through the same helper so the rule has one definition.♻️ Proposed change
- if (canonicalResolved === canonicalWorkspaceDir || canonicalResolved.startsWith(canonicalWorkspaceDir + '/')) return undefined+ if (isSameOrDescendantPath(canonicalResolved, canonicalWorkspaceDir)) return undefinedAlso applies to: 270-272, 288-298
🤖 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/manager.ts` around lines 254 - 256, Replace the repeated normalized-path containment checks in the relevant sandbox manager logic with the existing isSameOrDescendantPath helper from the path utilities, including the sites around the canonicalResolved/canonicalWorkspaceDir check and the additional checks near lines 270–272 and 288–298. Preserve the current normalization and return behavior while routing all three containment cases through the shared helper.forge-config.jsonc (1)
88-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the inert
sandbox.modekey from the sample configuration, or document its current purpose.SandboxConfig.modeaccepts'sbx', so the value is type-valid.🤖 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 `@forge-config.jsonc` at line 88, Remove the inert "mode": "sbx" entry from the sample configuration, unless the configuration’s current purpose for SandboxConfig.mode is explicitly documented. Keep the remaining sandbox configuration unchanged.
🤖 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 `@docs/api/_media/configuration.md`:
- Line 228: Update the sandbox.enabled entry in the configuration table to
explicitly state that configured sandbox startup fails with remediation when the
sbx daemon is unavailable, rather than implying Forge silently runs without
sandboxing.
In `@docs/api/_media/sandbox.md`:
- Line 11: Update the platform description in sandbox.md to capitalize “Apple
Silicon” consistently with the product’s official naming, without changing the
surrounding compatibility requirements.
- Line 153: Update the sandbox configuration reference in the documentation
sentence to use the supported `sandbox.enabled: false` setting instead of the
internal `sandboxEnabled: false` alias, while preserving the existing behavior
description.
- Around line 56-68: Update the sandbox network policy documentation to
accurately describe that sbx policy allow network <host> is machine-wide across
all sandboxes, or revise the documented command and manager behavior to apply
rules per sandbox using --sandbox <name>. Also document that an empty allowlist
does not remove previously persisted global rules and that existing global rules
must be accounted for.
In `@docs/configuration.md`:
- Around line 227-228: Update the configuration table in the sandbox settings
documentation to avoid presenting sandbox.mode as an operational setting: remove
its row, or explicitly mark it as reserved and non-functional until runtime code
consumes it. Keep the sandbox.enabled documentation unchanged.
In `@docs/modules.md`:
- Around line 312-318: Update the documented SandboxManager interface in the API
section to match the current contract in src/sandbox/manager.ts: change
getActive() to accept a worktree name and return ActiveSandbox | null, add
startedAt to start() and document its { containerName: string } result, update
restore() to include project and timestamp arguments, and add ensureRunning()
with its current signature.
In `@docs/sandbox.md`:
- Around line 113-119: Update the Rules section in docs/sandbox.md to match the
current implementation: either implement and test validation for absolute paths,
invalid-entry handling, and reserved-path overlap before documenting them as
guarantees, or explicitly state that sandbox.mounts is trusted configuration
without enforced path validation.
In `@scripts/cleanup-loop.ts`:
- Line 213: Update the cleanup flow around envFilePathFor and resolveForgeDbPath
to resolve PluginConfig.dataDir and pass it through all sandbox path
calculations, including the env file, database, and worktree paths, instead of
relying on the default directory.
In `@src/sandbox/exec-fs.ts`:
- Around line 20-23: Escape the caller-supplied search path before interpolating
it into the shell command, using the same single-quote escaping as safePattern.
Apply this in both search helpers: src/sandbox/exec-fs.ts lines 20-23 for path
and lines 58-66 for searchPath, preserving the existing rg command behavior.
In `@src/sandbox/manager.ts`:
- Around line 555-561: Update the ensureRunning recreation path around
runtime.removeSandbox so removal failures are caught, logged through the
existing logger, and do not prevent start(worktreeName, projectDir, startedAt)
from recreating the sandbox. Apply the same handling to the corresponding path
near the additional referenced occurrence, matching the tolerated-failure
behavior already used by stop.
- Around line 328-332: Update the env-file creation in the surrounding method to
pass mode 0o600 in the writeFileSync options, ensuring newly created files are
private from creation; retain the existing chmodSync call because writeFileSync
ignores mode when the file already exists.
In `@src/sandbox/process.ts`:
- Around line 70-103: Update the command lifecycle around settle, onAbort, and
the SIGKILL escalation timers: declare onAbort before settle, retain references
to both timeout escalation timers, clear the relevant timers in settle, and
remove onAbort from opts.abort when settling. Preserve existing timeout and
abort termination behavior, including escalation when the process remains
unsettled.
- Around line 110-113: Update the opts.stdin handling in the child-process flow
to attach an error listener to child.stdin before calling write or end, handling
expected EPIPE and ECONNRESET stream errors without terminating the host process
while preserving existing child-process error handling.
In `@test/sandbox/resolve-custom-mounts.test.ts`:
- Around line 79-137: Update resolveCustomMounts to validate each
SandboxMountConfig.host with an absolute-path check before calling resolve,
logging the existing invalid/missing-path style message and skipping relative
values. Add a test covering a relative path that exists in the working directory
and verify it produces no mount.
In `@test/workspace/forge-adapter.test.ts`:
- Line 513: Propagate ActiveSandbox.envFile through direct MCP execution: add an
env-file placeholder to the generated opencode configuration, resolve it after
sandbox startup, and place --env-file <path> before -i when configured; remove
both the flag and placeholder when absent. Update the seven affected tests in
test/workspace/forge-adapter.test.ts:513-513, 525-525, 548-548 and
test/workspace/worktree-opencode-config.test.ts:100-100, 110-110, 121-121,
140-140 to cover configured and missing env-file paths.
---
Outside diff comments:
In `@test/sandbox-tools.test.ts`:
- Around line 64-70: Update mockRuntime in sandbox-tools.test.ts to satisfy the
complete SandboxRuntime interface before assigning it to SandboxContext.runtime.
Prefer adding test-double implementations for every required SandboxRuntime
method, or explicitly cast it at both the assignments around lines 64–65 and
382–383, while preserving the existing exec behavior.
---
Nitpick comments:
In `@container/Dockerfile`:
- Around line 64-65: Replace the world-writable permissions in the /opt/forge
setup, including the corresponding paths at the other referenced locations, with
ownership assigned to agent:agent and restrictive 0755 permissions, using 0775
only for stores that require group write access. Preserve 0777 only where an
unpredictable UID still requires it, and document that exception inline.
In `@forge-config.jsonc`:
- Line 88: Remove the inert "mode": "sbx" entry from the sample configuration,
unless the configuration’s current purpose for SandboxConfig.mode is explicitly
documented. Keep the remaining sandbox configuration unchanged.
In `@src/sandbox/manager.ts`:
- Around line 254-256: Replace the repeated normalized-path containment checks
in the relevant sandbox manager logic with the existing isSameOrDescendantPath
helper from the path utilities, including the sites around the
canonicalResolved/canonicalWorkspaceDir check and the additional checks near
lines 270–272 and 288–298. Preserve the current normalization and return
behavior while routing all three containment cases through the shared helper.
In `@src/sandbox/sbx.ts`:
- Around line 375-389: Update the failure messages in createSandbox and
removeSandbox to use result.stderr || result.stdout, matching loadTemplate, so
errors reported only on stdout are included in the thrown Error messages.
In `@src/sandbox/template.ts`:
- Line 47: Update the tarPath construction near the sandbox template generation
to include a per-invocation random or timestamp-based suffix in addition to
process.pid. Preserve the existing temporary-directory location and filename
prefix while ensuring overlapping build actions cannot share or delete the same
archive.
In `@src/tui.tsx`:
- Around line 207-208: Replace the hardcoded “Build sandbox template” dialog
heading in the relevant TUI component with the imported
SANDBOX_TEMPLATE_BUILD_COMMAND constant, keeping the palette entry and heading
synchronized.
- Around line 183-191: Update the logger injected in the
buildAndLoadSandboxTemplate call to preserve build and template-load output
instead of using no-op log, error, and debug handlers. Route these messages to
console.error, or capture the relevant output and append its tail to the error
toast while retaining the existing failure handling.
In `@test/sandbox/context.test.ts`:
- Around line 81-82: Rename the test containing the context equality assertion
and ensureRunning verification to describe its actual behavior: resolving
context from getActive without calling ensureRunning when no worktreeDir is
provided. Keep the test implementation unchanged.
In `@test/sandbox/manager-network-allow.test.ts`:
- Around line 18-22: Update the manager setup in these tests to inject
createFakeGitService with non-OK revParseGitDir and revParseGitCommonDir results
into createSandboxManager, ensuring the first start call uses the fake service
instead of defaultGitService and does not invoke the host Git installation.
In `@test/sandbox/template.test.ts`:
- Around line 15-31: Update makeFakeRun to support command-specific results,
allowing docker build to return success while docker save returns a non-zero
exit code. Add or adjust coverage for buildAndLoadSandboxTemplate so the
save-stage failure path is exercised and asserted separately from build-stage
failures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8429a029-dfa3-4ad6-95c8-bd4331803d6d
📒 Files selected for processing (85)
AGENTS.mdREADME.mdcontainer/.dockerignorecontainer/Dockerfilecontainer/dind-entrypoint.shdocs/api/README.mddocs/api/_media/architecture.mddocs/api/_media/configuration.mddocs/api/_media/loop-system.mddocs/api/_media/sandbox.mddocs/api/_media/tools.mddocs/api/functions/createForgePlugin.mddocs/api/functions/createParentSessionLookup.mddocs/api/functions/createSessionDirectoryLookup.mddocs/api/interfaces/CompactionConfig.mddocs/api/interfaces/CreateParentSessionLookupOptions.mddocs/api/interfaces/CreateSessionDirectoryLookupOptions.mddocs/api/interfaces/DashboardConfig.mddocs/api/interfaces/PluginConfig.mddocs/api/variables/VERSION.mddocs/api/variables/default.mddocs/architecture.mddocs/configuration.mddocs/loop-system.mddocs/modules.mddocs/sandbox.mddocs/tools.mdforge-config.jsoncscripts/cleanup-loop.tssrc/hooks/forge-session-attach.tssrc/hooks/sandbox-tools.tssrc/hooks/shell-env.tssrc/index.tssrc/install/paths.tssrc/prompts/commands/execute-goal.mdsrc/prompts/commands/execute-plan.mdsrc/sandbox/config-warnings.tssrc/sandbox/context.tssrc/sandbox/docker.tssrc/sandbox/exec-fs.tssrc/sandbox/manager.tssrc/sandbox/path.tssrc/sandbox/process.tssrc/sandbox/reconcile.tssrc/sandbox/sbx.tssrc/sandbox/shell-shim.tssrc/sandbox/template.tssrc/services/execution.tssrc/tui.tsxsrc/tui/execute-plan-panel.tsxsrc/types.tssrc/workspace/forge-adapter.tssrc/workspace/worktree-opencode-config.tstest/helpers/sandbox-mocks.tstest/hooks/forge-session-attach.test.tstest/hooks/shell-env.test.tstest/loop/runtime.test.tstest/plugin.test.tstest/sandbox-docker.test.tstest/sandbox-manager.test.tstest/sandbox-path.test.tstest/sandbox-tools.test.tstest/sandbox/config-warnings.test.tstest/sandbox/context.test.tstest/sandbox/detect-git-mount.test.tstest/sandbox/manager-caching.test.tstest/sandbox/manager-custom-mounts.test.tstest/sandbox/manager-env-passthrough.test.tstest/sandbox/manager-host-access.test.tstest/sandbox/manager-network-allow.test.tstest/sandbox/manager-project-mount.test.tstest/sandbox/manager-reliability.test.tstest/sandbox/manager-temp-mount.test.tstest/sandbox/manager-tool-output-mount.test.tstest/sandbox/manager.test.tstest/sandbox/process.test.tstest/sandbox/resolve-custom-mounts.test.tstest/sandbox/sbx-runtime.test.tstest/sandbox/shell-shim.test.tstest/sandbox/template.test.tstest/services/execution-restart.test.tstest/services/execution.start-loop.test.tstest/setup.test.tstest/workspace/forge-adapter.test.tstest/workspace/worktree-opencode-config.test.ts
💤 Files with no reviewable changes (6)
- container/.dockerignore
- container/dind-entrypoint.sh
- src/sandbox/docker.ts
- test/sandbox-docker.test.ts
- test/sandbox/manager-host-access.test.ts
- test/services/execution.start-loop.test.ts
| | `sandbox.mounts` | `[]` | Additional custom bind mounts. | | ||
| | `sandbox.network.hostGateway` | `true` | Enable `host.docker.internal` gateway. | | ||
| | `sandbox.network.env` | `[]` | Host environment variables to pass into the container via temp env file. | | ||
| | `sandbox.enabled` | `true` | Enable sandboxed execution when the `sbx` daemon is available. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document the configured-sandbox failure behavior.
"when the sbx daemon is available" can imply that Forge silently runs without a sandbox when the daemon is unavailable. docs/api/_media/loop-system.md states that configured sandbox startup fails with remediation instead. Update this description to state the failure behavior explicitly.
🤖 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 `@docs/api/_media/configuration.md` at line 228, Update the sandbox.enabled
entry in the configuration table to explicitly state that configured sandbox
startup fails with remediation when the sbx daemon is unavailable, rather than
implying Forge silently runs without sandboxing.
| - Sandbox image available as `oc-forge-sandbox:latest`, unless configured otherwise. | ||
| - The `sbx` CLI installed and authenticated. Run `sbx login` to authenticate. | ||
| - The `sbx` daemon (`sandboxd`) running. Start it with `sbx daemon start` if it is not already up. | ||
| - A platform the `sbx` daemon supports: macOS 14+ on Apple silicon, Windows 11 with Hypervisor Platform, or Ubuntu 24.04+ with KVM. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the product's official capitalization.
Change Apple silicon to Apple Silicon.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Did you mean the proper noun “Apple Silicon”?
Context: ...the sbx daemon supports: macOS 14+ on Apple silicon, Windows 11 with Hypervisor Platform, o...
(APPLE_PRODUCTS)
🤖 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 `@docs/api/_media/sandbox.md` at line 11, Update the platform description in
sandbox.md to capitalize “Apple Silicon” consistently with the product’s
official naming, without changing the surrounding compatibility requirements.
Source: Linters/SAST tools
Uh oh!
There was an error while loading. Please reload this page.
| - `--chromeArg=--no-sandbox` is required: the exec runs as root and the container's seccomp profile blocks Chromium's user-namespace sandbox. The per-loop container is already the isolation boundary. | ||
| - `--chromeArg=--no-sandbox` is required: the sbx sandbox's seccomp profile blocks Chromium's user-namespace sandbox. The per-loop sandbox is already the isolation boundary, so it is safe to disable. | ||
| - `--chromeArg=--disable-dev-shm-usage` avoids crashes from the small default `/dev/shm`. | ||
| - Loops without a sandbox (opt-out via `sandboxEnabled: false`, or sandbox disabled) automatically drop MCP entries referencing `{{FORGE_SANDBOX_CONTAINER}}`, so the config stays valid everywhere. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
rg -nP '\bsandboxEnabled\b|\bsandbox\.enabled\b' src test docsRepository: chriswritescode-dev/opencode-forge
Length of output: 11184
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- candidate configuration definitions and reads ---'
rg -n -C 4 'sandbox\.enabled|sandboxEnabled|isSandboxEnabled|sandbox:' src test --glob '*.{ts,tsx,js,json,yaml,yml}'| head -n 260
printf'%s\n''--- relevant source sections ---'
sed -n '230,275p' src/index.ts
sed -n '1,120p' src/sandbox/context.ts
printf'%s\n''--- documentation context ---'
sed -n '140,165p' docs/api/_media/sandbox.mdRepository: chriswritescode-dev/opencode-forge
Length of output: 23723
🏁 Script executed:
#!/bin/bashset -euo pipefail
python3 - <<'PY'from pathlib import Pathcontext = Path("src/sandbox/context.ts").read_text()adapter = Path("src/workspace/forge-adapter.ts").read_text()types = Path("src/types.ts").read_text()assert "return config?.sandbox?.enabled !== false" in contextassert "forgeLoop.sandboxEnabled" in adapterassert "sandboxEnabled?: boolean" not in typesprint("Plugin configuration gate: config.sandbox.enabled only")print("sandboxEnabled: internal forgeLoop envelope field, not a top-level PluginConfig key")PYRepository: chriswritescode-dev/opencode-forge
Length of output: 311
Use sandbox.enabled: false in the documentation.sandboxEnabled is an internal per-loop field, not a supported configuration alias.
🤖 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 `@docs/api/_media/sandbox.md` at line 153, Update the sandbox configuration
reference in the documentation sentence to use the supported `sandbox.enabled:
false` setting instead of the internal `sandboxEnabled: false` alias, while
preserving the existing behavior description.
| | `sandbox.enabled` | `true` | Enable sandboxed execution when the `sbx` daemon is available. | | ||
| | `sandbox.mode` | `"sbx"` | Sandbox mode. `sbx` is currently the only supported mode. | |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not document sandbox.mode as an operational setting while it is inert.
The PR objectives state that mode: "sbx" is currently a deferred, inert configuration key. This table says that the key selects the sandbox mode, so users may expect it to change runtime behavior.
Remove the row until the setting is consumed, or label it explicitly as reserved and non-functional.
🤖 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 `@docs/configuration.md` around lines 227 - 228, Update the configuration table
in the sandbox settings documentation to avoid presenting sandbox.mode as an
operational setting: remove its row, or explicitly mark it as reserved and
non-functional until runtime code consumes it. Keep the sandbox.enabled
documentation unchanged.
| logger.log(`Sandbox: sandbox ${active.containerName} is not running, recreating for ${worktreeName}`) | ||
| activeSandboxes.delete(worktreeName) | ||
| await docker.removeContainer(active.containerName) | ||
| await runtime.removeSandbox(active.containerName) | ||
| const result = await start(worktreeName, projectDir, startedAt) | ||
| lastLivenessCheck.set(worktreeName, Date.now()) | ||
| return result.containerName |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
A failed removeSandbox blocks recreation in ensureRunning.
runtime.removeSandbox throws for every failure that is not "sandbox missing". Here the rejection propagates out of ensureRunning, so a stale, non-running sandbox that resists removal fails the loop instead of being recreated. stop already tolerates this failure. Log the error and continue to start, or state explicitly that this path must fail closed.
🛠️ Proposed change
- await runtime.removeSandbox(active.containerName)+ try {+ await runtime.removeSandbox(active.containerName)+ } catch (err) {+ logger.log(`Sandbox ${active.containerName} removal before recreate: ${err instanceof Error ? err.message : String(err)}`)+ }- await runtime.removeSandbox(containerName)+ try {+ await runtime.removeSandbox(containerName)+ } catch (err) {+ logger.log(`Sandbox ${containerName} removal before recreate: ${err instanceof Error ? err.message : String(err)}`)+ }Also applies to: 580-584
🤖 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/manager.ts` around lines 555 - 561, Update the ensureRunning
recreation path around runtime.removeSandbox so removal failures are caught,
logged through the existing logger, and do not prevent start(worktreeName,
projectDir, startedAt) from recreating the sandbox. Apply the same handling to
the corresponding path near the additional referenced occurrence, matching the
tolerated-failure behavior already used by stop.
| function settle(result: CommandResult): void { | ||
| if (settled) return | ||
| settled = true | ||
| clearTimeout(timeoutId) | ||
| clearTimeout(hardDeadlineId) | ||
| resolve(result) | ||
| } | ||
| const timeoutId = setTimeout(() => { | ||
| timedOut = true | ||
| opts.logger.log(`[${logLabel}] timeout (${timeout}ms) for: ${cmdPreview}`) | ||
| child.kill('SIGTERM') | ||
| setTimeout(() => { | ||
| if (!settled) { | ||
| opts.logger.log(`[${logLabel}] SIGKILL after SIGTERM for: ${cmdPreview}`) | ||
| child.kill('SIGKILL') | ||
| } | ||
| }, 5000) | ||
| }, timeout) | ||
| const onAbort = () => { | ||
| opts.logger.log(`[${logLabel}] abort signal for: ${cmdPreview}`) | ||
| child.kill('SIGTERM') | ||
| setTimeout(() => { | ||
| if (!settled) child.kill('SIGKILL') | ||
| }, 5000) | ||
| } | ||
| if (opts.abort) { | ||
| if (opts.abort.aborted) { | ||
| onAbort() | ||
| } else { | ||
| opts.abort.addEventListener('abort', onAbort, { once: true }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Release the abort listener and the SIGKILL escalation timer in settle.
Two cleanup gaps exist on the normal completion path:
settledoes not callopts.abort?.removeEventListener('abort', onAbort). The listener uses{ once: true }, so it is removed only if it fires. A caller that reuses one long-livedAbortSignalfor many commands accumulates listeners; each retained closure also keeps its exitedchildreachable.- The 5000ms escalation timer created at Line 82 is never cleared. It keeps a timer referenced for up to 5s after the command settles.
🛡️ Proposed fix
let timedOut = false
let settled = false
+ let escalationId: ReturnType<typeof setTimeout> | undefined
function settle(result: CommandResult): void {
if (settled) return
settled = true
clearTimeout(timeoutId)
clearTimeout(hardDeadlineId)
+ clearTimeout(escalationId)+ opts.abort?.removeEventListener('abort', onAbort)
resolve(result)
}
const timeoutId = setTimeout(() => {
timedOut = true
opts.logger.log(`[${logLabel}] timeout (${timeout}ms) for: ${cmdPreview}`)
child.kill('SIGTERM')
- setTimeout(() => {+ escalationId = setTimeout(() => {
if (!settled) {
opts.logger.log(`[${logLabel}] SIGKILL after SIGTERM for: ${cmdPreview}`)
child.kill('SIGKILL')
}
}, 5000)
}, timeout)
const onAbort = () => {
opts.logger.log(`[${logLabel}] abort signal for: ${cmdPreview}`)
child.kill('SIGTERM')
- setTimeout(() => {+ escalationId = setTimeout(() => {
if (!settled) child.kill('SIGKILL')
}, 5000)
}Note: onAbort is declared with const after settle, so move the onAbort declaration above settle, or keep the reference through a mutable binding, to avoid a temporal dead zone if settle runs synchronously during spawn.
🧰 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)
🤖 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 70 - 103, Update the command lifecycle
around settle, onAbort, and the SIGKILL escalation timers: declare onAbort
before settle, retain references to both timeout escalation timers, clear the
relevant timers in settle, and remove onAbort from opts.abort when settling.
Preserve existing timeout and abort termination behavior, including escalation
when the process remains unsettled.
| if (opts.stdin) { | ||
| child.stdin!.write(opts.stdin) | ||
| child.stdin!.end() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Find runCommand / CommandRunner call sites that pass stdin.
rg -nP -C4 '\bstdin\s*:' --glob '*.ts' --glob '*.tsx'
rg -nP -C3 '\brunCommand\s*\(' --glob '*.ts' --glob '*.tsx'Repository: chriswritescode-dev/opencode-forge
Length of output: 172
🏁 Script executed:
#!/bin/bashset -eu
printf'%s\n''--- tracked sandbox files ---'
git ls-files 'src/sandbox/**'printf'%s\n''--- process.ts outline ---'
ast-grep outline src/sandbox/process.ts --view expanded
printf'%s\n''--- process.ts relevant section ---'
cat -n src/sandbox/process.ts | sed -n '1,180p'printf'%s\n''--- stdin and runCommand references ---'
rg -n -C3 '\bstdin\b|\brunCommand\b|\bCommandRunner\b' src --glob '*.ts' --glob '*.tsx'||trueRepository: chriswritescode-dev/opencode-forge
Length of output: 15481
🏁 Script executed:
#!/bin/bashset -eu
printf'%s\n''--- all tracked stdin/runCommand references ---'
rg -n -C3 '\bstdin\b|\brunCommand\b|\bCommandRunner\b'. --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**'||trueprintf'%s\n''--- exports and package entry points ---'
rg -n -C3 'export .*runCommand|from .*/sandbox/process|sandbox/process'. --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**'||trueprintf'%s\n''--- standalone early-exit stdin probe ---'
node - <<'JS'const { spawn } = require('child_process')const child = spawn(process.execPath, ['-e', 'process.exit(1)'], { stdio: ['pipe', 'ignore', 'ignore'],})let stdinErrorchild.stdin.on('error', (err) => { stdinError = { name: err.name, code: err.code, message: err.message }})child.on('close', (code) => { setTimeout(() => { console.log(JSON.stringify({ closeCode: code, stdinError: stdinError ?? null })) }, 50)})child.stdin.write(Buffer.alloc(1024 * 1024, 'x'))child.stdin.end()JSRepository: chriswritescode-dev/opencode-forge
Length of output: 23691
Handle child.stdin errors separately from child-process errors.
If a caller supplies stdin and the child closes the pipe early, the stream can emit EPIPE or ECONNRESET without a listener and terminate the host process. Add the listener before writing.
🧰 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)
🤖 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 110 - 113, Update the opts.stdin
handling in the child-process flow to attach an error listener to child.stdin
before calling write or end, handling expected EPIPE and ECONNRESET stream
errors without terminating the host process while preserving existing
child-process error handling.
| { host: '/definitely/not/here' }, | ||
| ] | ||
| const result = resolveCustomMounts(raw, new Set(['/workspace']), logger) | ||
| expect(result).toEqual([]) | ||
| expect(logger.log).toHaveBeenCalledTimes(1) | ||
| expect(logger.log.mock.calls[0][0]).toContain('host path does not exist') | ||
| }) | ||
| test('non-absolute container path is skipped', () => { | ||
| test('collision with reserved host path is skipped', () => { | ||
| const dir = withTempDir() | ||
| const logger = createMockLogger() | ||
| const raw: SandboxMountConfig[] = [ | ||
| { host: dir, container: 'data' }, | ||
| { host: dir }, | ||
| ] | ||
| const result = resolveCustomMounts(raw, new Set(['/workspace']), logger) | ||
| expect(result).toEqual([]) | ||
| expect(logger.log).toHaveBeenCalledTimes(1) | ||
| expect(logger.log.mock.calls[0][0]).toContain('must be absolute') | ||
| }) | ||
| test('collision with reserved container path is skipped', () => { | ||
| const dir = withTempDir() | ||
| const logger = createMockLogger() | ||
| const raw: SandboxMountConfig[] = [ | ||
| { host: dir, container: '/workspace' }, | ||
| ] | ||
| const result = resolveCustomMounts(raw, new Set(['/workspace']), logger) | ||
| const result = resolveCustomMounts(raw, new Set([resolve(dir)]), logger) | ||
| expect(result).toEqual([]) | ||
| expect(logger.log).toHaveBeenCalledTimes(1) | ||
| expect(logger.log.mock.calls[0][0]).toContain('already in use') | ||
| }) | ||
| test('nested collision with reserved container path is skipped', () => { | ||
| test('nested collision with reserved host path is skipped', () => { | ||
| const dir = withTempDir() | ||
| const nested = join(dir, 'cache') | ||
| mkdirSync(nested, { recursive: true }) | ||
| const logger = createMockLogger() | ||
| const raw: SandboxMountConfig[] = [ | ||
| { host: dir, container: '/workspace/cache', readonly: false }, | ||
| { host: nested, readonly: false }, | ||
| ] | ||
| const result = resolveCustomMounts(raw, new Set(['/workspace']), logger) | ||
| const result = resolveCustomMounts(raw, new Set([resolve(dir)]), logger) | ||
| expect(result).toEqual([]) | ||
| expect(logger.log).toHaveBeenCalledTimes(1) | ||
| expect(logger.log.mock.calls[0][0]).toContain('already in use') | ||
| }) | ||
| test('duplicate container path among entries skips the second', () => { | ||
| test('duplicate host path among entries skips the second', () => { | ||
| const dir1 = withTempDir() | ||
| const dir2 = mkdtempSync(join(tmpdir(), 'forge-mount-')) | ||
| const logger = createMockLogger() | ||
| const raw: SandboxMountConfig[] = [ | ||
| { host: dir1, container: '/shared' }, | ||
| { host: dir2, container: '/shared' }, | ||
| { host: dir1 }, | ||
| { host: dir1 }, | ||
| ] | ||
| const result = resolveCustomMounts(raw, new Set(['/workspace']), logger) | ||
| expect(result).toHaveLength(1) | ||
| expect(result[0]).toEqual({ hostDir: resolve(dir1), containerDir: '/shared', readOnly: true }) | ||
| expect(result[0]).toEqual({ hostDir: resolve(dir1), readOnly: true }) | ||
| expect(logger.log).toHaveBeenCalledTimes(1) | ||
| expect(logger.log.mock.calls[0][0]).toContain('already in use') | ||
| // Clean up the second temp dir | ||
| rmSync(dir2, { recursive: true, force: true }) | ||
| }) | ||
| test('missing host or container field is skipped', () => { | ||
| test('missing host field is skipped', () => { | ||
| const dir = withTempDir() | ||
| const logger = createMockLogger() | ||
| const raw: SandboxMountConfig[] = [ | ||
| { host: '', container: '/data' } as SandboxMountConfig, | ||
| { host: dir, container: '' } as SandboxMountConfig, | ||
| { host: '' } as SandboxMountConfig, | ||
| { host: dir }, | ||
| ] | ||
| const result = resolveCustomMounts(raw, new Set(['/workspace']), logger) | ||
| expect(result).toEqual([]) | ||
| expect(logger.log).toHaveBeenCalledTimes(2) | ||
| expect(logger.log.mock.calls[0][0]).toContain('missing host/container') | ||
| expect(logger.log.mock.calls[1][0]).toContain('missing host/container') | ||
| expect(result).toHaveLength(1) | ||
| expect(logger.log).toHaveBeenCalledTimes(1) | ||
| expect(logger.log.mock.calls[0][0]).toContain('missing host path') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject relative custom mount paths.
SandboxMountConfig.host requires an absolute host path. resolveCustomMounts accepts a relative path and resolves it against the plugin process working directory. If that directory exists, the sandbox receives an unintended mount.
Reject non-absolute host values before calling resolve. Add a test for a relative existing path.
🤖 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 `@test/sandbox/resolve-custom-mounts.test.ts` around lines 79 - 137, Update
resolveCustomMounts to validate each SandboxMountConfig.host with an
absolute-path check before calling resolve, logging the existing
invalid/missing-path style message and skipping relative values. Add a test
covering a relative path that exists in the working directory and verify it
produces no mount.
| 'chrome-devtools': { | ||
| type: 'local', | ||
| command: ['docker', 'exec', '-i', '{{FORGE_SANDBOX_CONTAINER}}', 'chrome-devtools-mcp'], | ||
| command: ['sbx', 'exec', '-i', '{{FORGE_SANDBOX_CONTAINER}}', 'chrome-devtools-mcp'], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
ast-grep outline src/workspace/forge-adapter.ts --items all
ast-grep outline src/workspace/worktree-opencode-config.ts --items all
rg -n -C 4 \
'SANDBOX_CONTAINER_PLACEHOLDER|FORGE_SANDBOX|sbx[[:space:]]+exec|env-file' \
src/workspace test/workspaceRepository: chriswritescode-dev/opencode-forge
Length of output: 15913
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- candidate files ---'
git ls-files | rg '(^|/)(forge-adapter|worktree-opencode-config|sandbox/(manager|sbx)|.*sandbox.*)\.(ts|tsx)$'printf'%s\n''--- outlines ---'forfin src/workspace/forge-adapter.ts src/workspace/worktree-opencode-config.ts;doif [ -f"$f" ];then
ast-grep outline "$f" --items all
fidoneprintf'%s\n''--- sandbox symbols and direct executions ---'
rg -n -C 5 \
'class SandboxManager|interface Sandbox|envFile|env-file|network\.env|sandboxContainerName|writeWorktreeOpencodeConfig|worktreeOpencodeConfig|sbx.*exec|exec.*sbx' \
src test/workspaceRepository: chriswritescode-dev/opencode-forge
Length of output: 50390
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- relevant source sections ---'forspecin \
'src/workspace/worktree-opencode-config.ts:1:220' \
'src/workspace/forge-adapter.ts:1:260';do
file=${spec%%:*}
rest=${spec#*:}
start=${rest%%:*}
end=${rest##*:}
[ -f"$file" ] && sed -n "${start},${end}p""$file"doneprintf'%s\n''--- sandbox manager files and outlines ---'
git ls-files | rg 'sandbox|sbx'|whileread -r f;doprintf'\n--- %s ---\n'"$f"
ast-grep outline "$f" --items all 2>/dev/null ||truedoneRepository: chriswritescode-dev/opencode-forge
Length of output: 37543
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- manager interface and env-file lifecycle ---'
sed -n '90,180p' src/sandbox/manager.ts
sed -n '295,390p' src/sandbox/manager.ts
sed -n '390,560p' src/sandbox/manager.ts
printf'%s\n''--- adapter sandbox/config lifecycle ---'
sed -n '250,375p' src/workspace/forge-adapter.ts
printf'%s\n''--- relevant tests and fixtures ---'
sed -n '450,565p' test/workspace/forge-adapter.test.ts
sed -n '1,180p' test/workspace/worktree-opencode-config.test.tsRepository: chriswritescode-dev/opencode-forge
Length of output: 31553
🏁 Script executed:
#!/bin/bashset -euo pipefail
python3 - <<'PY'from pathlib import Pathimport remanager = Path("src/sandbox/manager.ts").read_text()adapter = Path("src/workspace/forge-adapter.ts").read_text()config = Path("src/workspace/worktree-opencode-config.ts").read_text()checks = { "manager exposes envFile on ActiveSandbox": bool(re.search(r"interface ActiveSandbox[\s\S]*?envFile\?: string", manager)), "manager interface exposes getActive": bool(re.search(r"interface SandboxManager[\s\S]*?getActive\(worktreeName: string\): ActiveSandbox", manager)), "manager writes env file from network env": bool(re.search(r"config\.network\?\.env", manager) and "writeEnvPassthroughFile" in manager), "adapter dependency exposes only start/stop": "Pick<SandboxManager, 'start' | 'stop'>" in adapter, "adapter passes only container placeholder input": "sandboxContainerName: sandboxProvisioned ? sandboxContainerName(info.name) : undefined" in adapter, "config resolver substitutes every string with container name": "substituteDeep(config, sandboxContainerName)" in config,}for name, result in checks.items(): print(f"{name}: {result}")print("\nDirect MCP command shapes:")for path in ("test/workspace/forge-adapter.test.ts", "test/workspace/worktree-opencode-config.test.ts"): text = Path(path).read_text() for line_no, line in enumerate(text.splitlines(), 1): if "sbx" in line and "exec" in line: print(f"{path}:{line_no}: {line.strip()}")print("\nEnv-file references in adapter/config:")for path in ("src/workspace/forge-adapter.ts", "src/workspace/worktree-opencode-config.ts"): for line_no, line in enumerate(Path(path).read_text().splitlines(), 1): if "envFile" in line or "env-file" in line: print(f"{path}:{line_no}: {line.strip()}")PYRepository: chriswritescode-dev/opencode-forge
Length of output: 1445
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- focused manager output ---'
sed -n '100,125p' src/sandbox/manager.ts
sed -n '305,370p' src/sandbox/manager.ts
rg -n -C 8 \
'envFile|writeEnvPassthroughFile|activeSandboxes\.set|return \{ containerName|start\(' \
src/sandbox/manager.ts
printf'%s\n''--- focused adapter output ---'
rg -n -C 12 \
'sandboxManager\.start|writeWorktreeOpencodeConfig|sandboxProvisioned|sandboxContainerName' \
src/workspace/forge-adapter.ts
printf'%s\n''--- configuration definitions and construction ---'
rg -n -C 8 \
'network:|env:|SandboxManagerConfig|createSandboxManager\(' \
src/types.ts src/index.ts srcRepository: chriswritescode-dev/opencode-forge
Length of output: 23856
Propagate the sandbox env file to direct MCP executions.
SandboxManager creates ActiveSandbox.envFile, but the adapter writes opencode.jsonc before sandbox startup and passes only the container name. Add an env-file placeholder, resolve it after startup, and emit --env-file <path> before -i when the path exists. Remove the flag and placeholder when it does not. Update all seven listed tests for configured and absent env-file paths.
📍 Affects 2 files
test/workspace/forge-adapter.test.ts#L513-L513(this comment)test/workspace/forge-adapter.test.ts#L525-L525test/workspace/forge-adapter.test.ts#L548-L548test/workspace/worktree-opencode-config.test.ts#L100-L100test/workspace/worktree-opencode-config.test.ts#L110-L110test/workspace/worktree-opencode-config.test.ts#L121-L121test/workspace/worktree-opencode-config.test.ts#L140-L140
🤖 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 `@test/workspace/forge-adapter.test.ts` at line 513, Propagate
ActiveSandbox.envFile through direct MCP execution: add an env-file placeholder
to the generated opencode configuration, resolve it after sandbox startup, and
place --env-file <path> before -i when configured; remove both the flag and
placeholder when absent. Update the seven affected tests in
test/workspace/forge-adapter.test.ts:513-513, 525-525, 548-548 and
test/workspace/worktree-opencode-config.test.ts:100-100, 110-110, 121-121,
140-140 to cover configured and missing env-file paths.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
test/sandbox/manager-caching.test.ts (1)
59-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate
isRunningstub.Line 60 already sets
isRunningto always returnfalse, and Line 69 replaces it with an identical stub. The second assignment has no effect on the test outcome. Delete Line 69 so the setup states one intent.♻️ Proposed change
- mockRuntime.isRunning = vi.fn(async () => false) await manager.restore('other-wt', '/tmp/project', new Date().toISOString())🤖 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 `@test/sandbox/manager-caching.test.ts` around lines 59 - 74, Remove the second redundant mockRuntime.isRunning assignment in the test, keeping the initial stub before manager.start as the sole setup for this behavior; leave the call-count assertions and restore flow unchanged.src/sandbox/manager.ts (2)
120-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the overlap filter.
buildSandboxWorkspaces(Lines 123-131) andbuildMountPlan(Lines 224-232) implement the same overlap-drop loop with the same log text. In the production pathstartfilters withbuildMountPlanfirst, so the second pass never drops a mount. Two copies of the rule can diverge later.Extract one helper and let
buildSandboxWorkspacesmap the already-filtered mounts.♻️ Suggested shape
+function filterOverlappingMounts(mounts: SandboxMount[], logger: Logger): SandboxMount[] {+ const accepted: string[] = []+ const kept: SandboxMount[] = []+ for (const mount of mounts) {+ if (accepted.some((hostDir) => containerPathsOverlap(mount.hostDir, hostDir))) {+ logger.log(`Sandbox: dropping workspace ${mount.hostDir} because it overlaps an already-mounted host dir`)+ continue+ }+ accepted.push(mount.hostDir)+ kept.push(mount)+ }+ return kept+}+ export function buildSandboxWorkspaces(mounts: SandboxMount[], logger: Logger): SandboxWorkspace[] { - const accepted: string[] = []- const workspaces: SandboxWorkspace[] = []- for (const mount of mounts) {- const overlap = accepted.some((hostDir) => containerPathsOverlap(mount.hostDir, hostDir))- if (overlap) {- logger.log(`Sandbox: dropping workspace ${mount.hostDir} because it overlaps an already-mounted host dir`)- continue- }- accepted.push(mount.hostDir)- workspaces.push({ hostDir: mount.hostDir, readOnly: mount.readOnly })- }- return workspaces+ return filterOverlappingMounts(mounts, logger)+ .map((mount) => ({ hostDir: mount.hostDir, readOnly: mount.readOnly })) }🤖 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/manager.ts` around lines 120 - 133, Extract the shared overlap-filtering loop from buildSandboxWorkspaces and buildMountPlan into a single helper that preserves the existing accepted-order and log behavior. Update buildSandboxWorkspaces to map the mounts returned by that helper into SandboxWorkspace objects, and make buildMountPlan reuse the same helper so the overlap rule has one implementation.
245-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
isSameOrDescendantPathfor the nested-path checks.Lines 245 and 259 open-code the prefix rule with
resolved === workspaceDir || resolved.startsWith(workspaceDir + '/').detectGitMountrepeats the same rule at Lines 278 and 282.src/sandbox/path.tsalready exportsisSameOrDescendantPathwith exactly this semantic, andtest/sandbox-path.test.tscovers it. Call the helper so the sibling-prefix rule lives in one place.♻️ Proposed change
- if (resolved === workspaceDir || resolved.startsWith(workspaceDir + '/')) return undefined+ if (isSameOrDescendantPath(resolved, workspaceDir)) return undefined- if (!resolvedGitDir.startsWith(projectDir + '/')) {+ if (!isSameOrDescendantPath(resolvedGitDir, projectDir)) { paths.add(resolvedGitDir) }Note:
isSameOrDescendantPathalso returns true for an exact match, so the git-dir checks change behavior when the git dir equalsprojectDir. Confirm that case cannot occur, or keep the strict descendant form there.🤖 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/manager.ts` around lines 245 - 259, Replace the duplicated resolved-path prefix checks in resolveTempMount and detectGitMount with the exported isSameOrDescendantPath helper from src/sandbox/path.ts. Preserve strict-descendant behavior for git-directory checks if an exact projectDir match is valid, while retaining exact-match rejection for workspace and temporary mounts.test/helpers/sandbox-mocks.ts (1)
11-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
CreateSandboxOptsfor all recordedcreateSandboxoptions. Import it fromsrc/sandbox/sbxand replace the three local option types so recorded calls track future fields.🤖 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 `@test/helpers/sandbox-mocks.ts` around lines 11 - 13, Update getCreateSandboxCalls to import and use CreateSandboxOpts from src/sandbox/sbx for the recorded createSandbox options, replacing the local inline option types while preserving the existing tuple structure.test/sandbox-path.test.ts (1)
5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the path fixtures with the identical-path model.
Production code creates no divergent mounts. Add an identical-path fixture to
test/sandbox-path.test.ts. Retain divergent-path cases only ifisInsideAnyMountmust support externally supplied mounts.🤖 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 `@test/sandbox-path.test.ts` around lines 5 - 6, Update the path fixtures near WORKTREE_MOUNT and PROJECT_MOUNT to add a mount whose hostDir and containerDir are identical, matching production behavior. Remove divergent-path fixtures unless isInsideAnyMount explicitly needs to support externally supplied mounts, and adjust affected tests to use the identical-path fixture.src/sandbox/template.ts (1)
62-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the cleanup so it cannot mask the build or save error.
rmSyncruns infinally. If it throws (for example on a permission error), the thrown cleanup error replaces thedockerStageErrorand the user loses the actual failure reason.♻️ Proposed change
} finally { - rmSync(tarPath, { force: true })+ try {+ rmSync(tarPath, { force: true })+ } catch (err) {+ deps.logger.log(`Failed to remove template tar ${tarPath}: ${err instanceof Error ? err.message : String(err)}`)+ } }🤖 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/template.ts` around lines 62 - 64, Update the finally cleanup around rmSync in the template build/save flow so cleanup failures cannot replace or mask the original dockerStageError. Guard the rmSync call and preserve propagation of the build or save error while retaining the forced tarPath cleanup behavior.src/services/execution.ts (1)
739-743: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
deps.sandboxManager.runtimein the cleanup path.
SandboxManager.runtimeexposes the required methods and preserves the injected command runner. This also removes the unnecessaryLoggercast.🤖 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/services/execution.ts` around lines 739 - 743, Update the cleanup path around sandboxContainerName to use deps.sandboxManager.runtime for sandboxContainerName, isRunning, and removeSandbox instead of dynamically importing createSbxRuntime; remove the unnecessary logger cast while preserving the existing cleanup behavior.Source: Coding guidelines
test/loops-repo.test.ts (1)
482-504: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that
restartpersists a non-nullauditorModel.Both restart tests pass
auditorModel: null, so the newauditor_model = ?binding is never checked against a real value. A misordered bind parameter can survive this suite. Add one assertion that a supplied model round-trips.💚 Proposed test addition
expect(repo.get(testRow.projectId, testRow.loopName)!.auditorFallbackIndex).toBe(0) }) ++ test('restart persists the supplied auditorModel', () => {+ repo.insert(testRow, testLarge)++ repo.restart(testRow.projectId, testRow.loopName, {+ sessionId: 'restart-session',+ phase: 'coding',+ iteration: 0,+ auditCount: 0,+ sandbox: false,+ sandboxContainer: null,+ workspaceId: null,+ auditorModel: 'prov/aud',+ currentSectionIndex: 0,+ totalSections: 0,+ finalAuditDone: false,+ startedAt: Date.now(),+ executorSessionId: null,+ })++ expect(repo.get(testRow.projectId, testRow.loopName)!.auditorModel).toBe('prov/aud')+ }) })🤖 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 `@test/loops-repo.test.ts` around lines 482 - 504, Update the restart test around repo.restart to supply a non-null auditorModel value, then assert that the restarted record’s auditorModel matches it. Keep the existing auditorFallbackIndex reset assertion and restart behavior checks unchanged.src/loop/runtime.ts (1)
2436-2448: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake
clearBusyMarkerssynchronous and guardloopNameonce.
clearBusyMarkersis declaredasync, and line 2447 calls it withoutawaitorvoid. That creates a floating promise, which ano-floating-promiseslint rule can reject. The body is also fully synchronous, and both statements requireloopName, so theelsebranch performs no work at all.♻️ Proposed refactor
- const clearBusyMarkers = async (): Promise<void> => {- if (loopName) {- clearPromptInFlightBySession(loopName, sessionId)- }- if (loopName && coalescedLimitSessions.get(loopName) === sessionId) {- coalescedLimitSessions.delete(loopName)- }- } if (loopName) { - await withStateLock(loopName, clearBusyMarkers)- } else {- clearBusyMarkers()+ await withStateLock(loopName, async () => {+ clearPromptInFlightBySession(loopName, sessionId)+ if (coalescedLimitSessions.get(loopName) === sessionId) {+ coalescedLimitSessions.delete(loopName)+ }+ }) }Run the following script to confirm the repository enforces the floating-promise rule:
#!/bin/bash# Description: Check whether the lint configuration enables no-floating-promises.set -euo pipefail fd -t f -i 'eslint' -d 2 --exec cat -n {} rg -n 'no-floating-promises|strictTypeChecked|recommendedTypeChecked' --glob '*.{js,ts,mjs,cjs,json,yaml,yml}' -g '!node_modules'🤖 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/loop/runtime.ts` around lines 2436 - 2448, Make clearBusyMarkers synchronous and move the loopName guard into that function so it returns immediately when loopName is absent. Keep both marker-clearing operations inside the guarded block, then retain the withStateLock invocation only for truthy loopName and remove the no-op else call.
🤖 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 `@docs/loop-system.md`:
- Line 176: Update the loop-system documentation wording around sandbox
activation to state that sandbox provisioning is controlled by sandbox.enabled,
not sandbox.mode. Revise both referenced sentences consistently, while
preserving the existing description of automatic provisioning when the sbx
daemon is available and worktree-only fallback behavior.
In `@README.md`:
- Line 414: Correct the loop sandbox documentation to use the actual
sandbox.enabled configuration and sbx daemon availability conditions, removing
references that present sandbox.mode as the enablement gate. Apply this
correction at README.md lines 414-414 and 459-459, and docs/api/README.md lines
416-416 and 461-461, including both loop execution and termination sections.
- Line 539: Update the missing-template toast guidance in src/index.ts to use
the command name “Build sandbox template” instead of “Build sandbox image”; the
README.md and docs/api/README.md sites require no direct change because they
already document the correct command.
In `@scripts/cleanup-loop.ts`:
- Around line 199-207: Update the cleanup check around sandboxName to derive the
name with sandboxContainerName(loopName), preserving the manager’s sanitization
and truncation rules instead of concatenating the raw loop name. Also validate
the spawnSync result before parsing stdout, and report or propagate sbx CLI
failures rather than treating empty output as an absent sandbox.
In `@src/sandbox/process.ts`:
- Around line 107-115: Update the hard-deadline handling around deadlinePromise
and the spawned child so that when the deadline timer fires, it terminates the
child process before resolving the CommandResult. Reuse the existing child
reference or expose a kill hook from the inner execution flow, and preserve the
current timeout result and logging behavior.
In `@test/sandbox/manager-custom-mounts.test.ts`:
- Around line 153-175: Update the sandbox startup mount construction used by
createSandboxManager so sourceProjectDir is included only when
existsSync(resolve(sourceProjectDir)) is true, preventing stale paths from being
passed to sbx create. Preserve custom mounts and the worktree mount, and add a
regression test covering a missing sourceProjectDir.
In `@test/sandbox/manager-env-passthrough.test.ts`:
- Around line 105-127: Align the test `stop deletes the env file even when it
holds no sandbox-env dir entry` with its actual behavior: either rename it and
remove the stale-entry comment to describe the normal start/stop cleanup, or
create the stale condition by removing the sandbox-env entry before calling
`manager.stop('test')` and retain assertions that stop succeeds and cleanup
leaves the directory empty.
---
Nitpick comments:
In `@src/loop/runtime.ts`:
- Around line 2436-2448: Make clearBusyMarkers synchronous and move the loopName
guard into that function so it returns immediately when loopName is absent. Keep
both marker-clearing operations inside the guarded block, then retain the
withStateLock invocation only for truthy loopName and remove the no-op else
call.
In `@src/sandbox/manager.ts`:
- Around line 120-133: Extract the shared overlap-filtering loop from
buildSandboxWorkspaces and buildMountPlan into a single helper that preserves
the existing accepted-order and log behavior. Update buildSandboxWorkspaces to
map the mounts returned by that helper into SandboxWorkspace objects, and make
buildMountPlan reuse the same helper so the overlap rule has one implementation.
- Around line 245-259: Replace the duplicated resolved-path prefix checks in
resolveTempMount and detectGitMount with the exported isSameOrDescendantPath
helper from src/sandbox/path.ts. Preserve strict-descendant behavior for
git-directory checks if an exact projectDir match is valid, while retaining
exact-match rejection for workspace and temporary mounts.
In `@src/sandbox/template.ts`:
- Around line 62-64: Update the finally cleanup around rmSync in the template
build/save flow so cleanup failures cannot replace or mask the original
dockerStageError. Guard the rmSync call and preserve propagation of the build or
save error while retaining the forced tarPath cleanup behavior.
In `@src/services/execution.ts`:
- Around line 739-743: Update the cleanup path around sandboxContainerName to
use deps.sandboxManager.runtime for sandboxContainerName, isRunning, and
removeSandbox instead of dynamically importing createSbxRuntime; remove the
unnecessary logger cast while preserving the existing cleanup behavior.
In `@test/helpers/sandbox-mocks.ts`:
- Around line 11-13: Update getCreateSandboxCalls to import and use
CreateSandboxOpts from src/sandbox/sbx for the recorded createSandbox options,
replacing the local inline option types while preserving the existing tuple
structure.
In `@test/loops-repo.test.ts`:
- Around line 482-504: Update the restart test around repo.restart to supply a
non-null auditorModel value, then assert that the restarted record’s
auditorModel matches it. Keep the existing auditorFallbackIndex reset assertion
and restart behavior checks unchanged.
In `@test/sandbox-path.test.ts`:
- Around line 5-6: Update the path fixtures near WORKTREE_MOUNT and
PROJECT_MOUNT to add a mount whose hostDir and containerDir are identical,
matching production behavior. Remove divergent-path fixtures unless
isInsideAnyMount explicitly needs to support externally supplied mounts, and
adjust affected tests to use the identical-path fixture.
In `@test/sandbox/manager-caching.test.ts`:
- Around line 59-74: Remove the second redundant mockRuntime.isRunning
assignment in the test, keeping the initial stub before manager.start as the
sole setup for this behavior; leave the call-count assertions and restore flow
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: eed3d038-db71-4c17-9fd3-b782cb9c501f
📒 Files selected for processing (92)
AGENTS.mdREADME.mdcontainer/.dockerignorecontainer/Dockerfilecontainer/dind-entrypoint.shdocs/api/README.mddocs/api/_media/architecture.mddocs/api/_media/configuration.mddocs/api/_media/loop-system.mddocs/api/_media/sandbox.mddocs/api/_media/tools.mddocs/api/functions/createForgePlugin.mddocs/api/functions/createParentSessionLookup.mddocs/api/functions/createSessionDirectoryLookup.mddocs/api/interfaces/CompactionConfig.mddocs/api/interfaces/CreateParentSessionLookupOptions.mddocs/api/interfaces/CreateSessionDirectoryLookupOptions.mddocs/api/interfaces/DashboardConfig.mddocs/api/interfaces/PluginConfig.mddocs/api/variables/VERSION.mddocs/api/variables/default.mddocs/architecture.mddocs/configuration.mddocs/loop-system.mddocs/modules.mddocs/sandbox.mddocs/tools.mdforge-config.jsoncscripts/cleanup-loop.tssrc/hooks/sandbox-tools.tssrc/hooks/shell-env.tssrc/index.tssrc/install/paths.tssrc/loop/runtime-usage.tssrc/loop/runtime.tssrc/loop/service.tssrc/loop/state.tssrc/sandbox/config-warnings.tssrc/sandbox/context.tssrc/sandbox/docker.tssrc/sandbox/exec-fs.tssrc/sandbox/manager.tssrc/sandbox/path.tssrc/sandbox/process.tssrc/sandbox/sbx.tssrc/sandbox/shell-shim.tssrc/sandbox/template.tssrc/services/execution.tssrc/storage/migrations/index.tssrc/storage/repos/loops-repo.tssrc/tools/loop.tssrc/tui.tsxsrc/types.tssrc/utils/loop-helpers.tssrc/workspace/forge-adapter.tstest/helpers/sandbox-mocks.tstest/hooks/loop-section-advancement.test.tstest/hooks/shell-env.test.tstest/loop-helpers.test.tstest/loop/runtime.test.tstest/loop/state-mapper.test.tstest/loops-repo.test.tstest/plugin.test.tstest/sandbox-docker.test.tstest/sandbox-manager.test.tstest/sandbox-path.test.tstest/sandbox-tools.test.tstest/sandbox/config-warnings.test.tstest/sandbox/context.test.tstest/sandbox/detect-git-mount.test.tstest/sandbox/manager-caching.test.tstest/sandbox/manager-custom-mounts.test.tstest/sandbox/manager-env-passthrough.test.tstest/sandbox/manager-host-access.test.tstest/sandbox/manager-network-allow.test.tstest/sandbox/manager-project-mount.test.tstest/sandbox/manager-reliability.test.tstest/sandbox/manager-temp-mount.test.tstest/sandbox/manager-tool-output-mount.test.tstest/sandbox/manager.test.tstest/sandbox/process.test.tstest/sandbox/resolve-custom-mounts.test.tstest/sandbox/sbx-runtime.test.tstest/sandbox/shell-shim.test.tstest/sandbox/template.test.tstest/services/attach-loop.test.tstest/services/execution-restart.test.tstest/setup.test.tstest/storage-migrations.test.tstest/tools/plan-adjust.test.tstest/tools/review-section-scope.test.tstest/tools/section-read.test.ts
💤 Files with no reviewable changes (6)
- test/sandbox/manager-host-access.test.ts
- test/sandbox-docker.test.ts
- container/.dockerignore
- container/dind-entrypoint.sh
- src/sandbox/docker.ts
- src/sandbox/path.ts
🚧 Files skipped from review as they are similar to previous changes (42)
- src/workspace/forge-adapter.ts
- test/sandbox/config-warnings.test.ts
- test/setup.test.ts
- docs/api/variables/default.md
- docs/api/functions/createForgePlugin.md
- docs/api/interfaces/CreateSessionDirectoryLookupOptions.md
- docs/api/variables/VERSION.md
- docs/api/_media/tools.md
- test/hooks/shell-env.test.ts
- docs/api/interfaces/DashboardConfig.md
- forge-config.jsonc
- AGENTS.md
- docs/api/interfaces/CompactionConfig.md
- test/sandbox/manager-network-allow.test.ts
- docs/api/functions/createParentSessionLookup.md
- docs/tools.md
- docs/api/_media/architecture.md
- test/sandbox/template.test.ts
- src/install/paths.ts
- src/index.ts
- src/sandbox/context.ts
- docs/api/functions/createSessionDirectoryLookup.md
- docs/api/_media/loop-system.md
- test/sandbox/manager-temp-mount.test.ts
- test/sandbox/manager.test.ts
- docs/api/interfaces/PluginConfig.md
- src/sandbox/config-warnings.ts
- src/hooks/shell-env.ts
- test/sandbox/resolve-custom-mounts.test.ts
- test/sandbox/manager-tool-output-mount.test.ts
- test/sandbox/manager-reliability.test.ts
- docs/modules.md
- test/sandbox-tools.test.ts
- test/sandbox/manager-project-mount.test.ts
- test/sandbox/detect-git-mount.test.ts
- test/sandbox/shell-shim.test.ts
- src/tui.tsx
- src/sandbox/shell-shim.ts
- docs/architecture.md
- test/sandbox/context.test.ts
- docs/api/_media/configuration.md
- src/types.ts
| ## Worktree Isolation | ||
| Loops always run in an isolated git worktree. Sandbox is optional: when Docker is available and `sandbox.mode = 'docker'` is configured, a sandbox container is provisioned automatically; otherwise the loop runs in worktree-only mode. | ||
| Loops always run in an isolated git worktree. Sandbox is optional: when the `sbx` daemon is available and `sandbox.mode = 'sbx'` is configured, a sandbox is provisioned automatically; otherwise the loop runs in worktree-only mode. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use sandbox.enabled as the activation condition.
SandboxConfig.mode is reserved for future modes. SandboxConfig.enabled controls whether loops use sandboxed execution. The current wording can cause users to configure the inert sandbox.mode = 'sbx' key and miss the actual switch.
Update both sentences to reference sandbox.enabled:
Proposed wording
-Loops always run in an isolated git worktree. Sandbox is optional: when the `sbx` daemon is available and `sandbox.mode = 'sbx'` is configured, a sandbox is provisioned automatically; otherwise the loop runs in worktree-only mode.+Loops always run in an isolated git worktree. Sandbox is optional: when the `sbx` daemon is available and `sandbox.enabled` is true, a sandbox is provisioned automatically; otherwise the loop runs in worktree-only mode.-Sandbox is optional. When the `sbx` daemon is available and configured, a sandbox is provisioned automatically; otherwise loops run in worktree-only mode.+Sandbox is optional. When the `sbx` daemon is available and `sandbox.enabled` is true, a sandbox is provisioned automatically; otherwise loops run in worktree-only mode.The SandboxConfig contract and the PR objectives support this correction.
Also applies to: 203-203
🤖 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 `@docs/loop-system.md` at line 176, Update the loop-system documentation
wording around sandbox activation to state that sandbox provisioning is
controlled by sandbox.enabled, not sandbox.mode. Revise both referenced
sentences consistently, while preserving the existing description of automatic
provisioning when the sbx daemon is available and worktree-only fallback
behavior.
| ### Worktree Isolation | ||
| Loops always run in an isolated git worktree. Sandbox is optional: when Docker is available and `sandbox.mode = 'docker'` is configured, a sandbox container is provisioned automatically; otherwise the loop runs in worktree-only mode. Changes are auto-committed and the worktree is removed on completion (branch preserved for later merge). | ||
| Loops always run in an isolated git worktree. Sandbox is optional: when the `sbx` daemon is available and `sandbox.mode = 'sbx'` is configured, a sandbox is provisioned automatically; otherwise the loop runs in worktree-only mode. Changes are auto-committed and the worktree is removed on completion (branch preserved for later merge). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not advertise inert sandbox.mode as the sandbox gate.
The runtime does not use this key to enable sandboxing. Users can therefore believe that loop commands run in an sbx sandbox while Forge falls back to worktree-only execution.
README.md#L414-L414: document the actualsandbox.enabledand availability conditions.README.md#L459-L459: apply the same correction to the termination section.docs/api/README.md#L416-L416: document the actual sandbox enablement contract.docs/api/README.md#L461-L461: apply the same correction to the termination section.
📍 Affects 2 files
README.md#L414-L414(this comment)README.md#L459-L459docs/api/README.md#L416-L416docs/api/README.md#L461-L461
🤖 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 `@README.md` at line 414, Correct the loop sandbox documentation to use the
actual sandbox.enabled configuration and sbx daemon availability conditions,
removing references that present sandbox.mode as the enablement gate. Apply this
correction at README.md lines 414-414 and 459-459, and docs/api/README.md lines
416-416 and 461-461, including both loop execution and termination sections.
| The image includes Node.js 24, pnpm, Bun, Python 3 + uv, ripgrep, git, and jq. | ||
| The `container/Dockerfile` ships with the plugin package. If the image is missing when OpenCode starts, Forge shows a warning toast with a "Build sandbox image" command in the palette. You can also trigger the build from the command palette at any time by searching for `Build sandbox image`, which opens a confirmation dialog and runs `docker build` automatically. | ||
| The `container/Dockerfile` ships with the plugin package. If the template is missing when OpenCode starts, Forge shows a warning toast with a "Build sandbox template" command in the palette. You can also trigger the build from the command palette at any time by searching for `Build sandbox template`, which opens a confirmation dialog and runs the build/save/load sequence automatically. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
rg -n 'Build sandbox (image|template)' README.md docs/api/README.md srcRepository: chriswritescode-dev/opencode-forge
Length of output: 1380
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'%s\n''--- src/index.ts ---'
sed -n '260,325p' src/index.ts
printf'%s\n''--- src/tui.tsx (command and toast paths) ---'
sed -n '180,225p' src/tui.tsx
sed -n '325,365p' src/tui.tsx
printf'%s\n''--- command registration references ---'
rg -n -C 3 'Build sandbox|sandboxImage|buildSandbox|template load' srcRepository: chriswritescode-dev/opencode-forge
Length of output: 10187
Use Build sandbox template in the missing-template toast.
The command registration and documentation use Build sandbox template, but src/index.ts tells users to search for Build sandbox image.
📍 Affects 2 files
README.md#L539-L539(this comment)docs/api/README.md#L541-L541
🤖 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 `@README.md` at line 539, Update the missing-template toast guidance in
src/index.ts to use the command name “Build sandbox template” instead of “Build
sandbox image”; the README.md and docs/api/README.md sites require no direct
change because they already document the correct command.
| const sandboxName = `forge-${loopName}` | ||
| console.log(`\nsbx sandbox ${sandboxName}:`) | ||
| const inspect = spawnSync('sbx', ['ls', '--json'], { encoding: 'utf-8' }) | ||
| const found = parseSbxSandboxList(inspect.stdout).find((e) => e.name === sandboxName) | ||
| if (!found) { | ||
| console.log(` not present`) | ||
| return | ||
| } | ||
| logAction(dryRun, `docker rm -f ${containerName}`, () => { | ||
| const r = spawnSync('docker', ['rm', '-f', containerName], { encoding: 'utf-8' }) | ||
| console.log(` present (running=${found.running})`) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Derive the sandbox name with sandboxContainerName, not string concatenation.
The manager creates each sandbox with sandboxContainerName(worktreeName), which is forge-${sanitizeSbxName(worktreeName)}. That sanitizer lowercases the name, collapses runs of characters outside [a-z0-9.+-] into -, and truncates to 60 characters. Line 199 skips the sanitizer, so any loop name containing /, _, or uppercase letters produces a name that never matches a real sandbox. Cleanup then prints not present and leaves the sandbox running.
Line 201 also ignores spawnSync failure. If the sbx CLI is missing, stdout is empty, the parse returns [], and the script reports not present instead of reporting the failure.
🐛 Proposed fix
-import { parseSbxSandboxList } from '../src/sandbox/sbx'+import { parseSbxSandboxList, sandboxContainerName } from '../src/sandbox/sbx'- const sandboxName = `forge-${loopName}`+ const sandboxName = sandboxContainerName(loopName)
console.log(`\nsbx sandbox ${sandboxName}:`)
const inspect = spawnSync('sbx', ['ls', '--json'], { encoding: 'utf-8' })
+ if (inspect.error || inspect.status !== 0) {+ console.log(` could not list sandboxes: ${inspect.error?.message ?? inspect.stderr?.trim() ?? 'unknown error'}`)+ return+ }
const found = parseSbxSandboxList(inspect.stdout).find((e) => e.name === sandboxName)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constsandboxName=`forge-${loopName}` | |
| console.log(`\nsbx sandbox ${sandboxName}:`) | |
| constinspect=spawnSync('sbx',['ls','--json'],{encoding: 'utf-8'}) | |
| constfound=parseSbxSandboxList(inspect.stdout).find((e)=>e.name===sandboxName) | |
| if(!found){ | |
| console.log(` not present`) | |
| return | |
| } | |
| logAction(dryRun,`docker rm -f ${containerName}`,()=>{ | |
| constr=spawnSync('docker',['rm','-f',containerName],{encoding: 'utf-8'}) | |
| console.log(` present (running=${found.running})`) | |
| constsandboxName=sandboxContainerName(loopName) | |
| console.log(`\nsbx sandbox ${sandboxName}:`) | |
| constinspect=spawnSync('sbx',['ls','--json'],{encoding: 'utf-8'}) | |
| if(inspect.error||inspect.status!==0){ | |
| console.log(` could not list sandboxes: ${inspect.error?.message??inspect.stderr?.trim()??'unknown error'}`) | |
| return | |
| } | |
| constfound=parseSbxSandboxList(inspect.stdout).find((e)=>e.name===sandboxName) | |
| if(!found){ | |
| console.log(` not present`) | |
| return | |
| } | |
| console.log(` present (running=${found.running})`) |
🤖 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 `@scripts/cleanup-loop.ts` around lines 199 - 207, Update the cleanup check
around sandboxName to derive the name with sandboxContainerName(loopName),
preserving the manager’s sanitization and truncation rules instead of
concatenating the raw loop name. Also validate the spawnSync result before
parsing stdout, and report or propagate sbx CLI failures rather than treating
empty output as an absent sandbox.
| const hardDeadline = timeout + 10_000 | ||
| const deadlinePromise = new Promise<CommandResult>((resolve) => { | ||
| hardDeadlineId = setTimeout(() => { | ||
| opts.logger.log(`[${logLabel}] hard deadline (${hardDeadline}ms) hit for: ${cmdPreview}`) | ||
| resolve({ stdout: '', stderr: `Command exceeded hard deadline of ${hardDeadline}ms`, exitCode: 124 }) | ||
| }, hardDeadline) | ||
| }) | ||
| return Promise.race([inner, deadlinePromise]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Kill the child when the hard deadline wins the race.
deadlinePromise resolves the returned promise, but it does not terminate child. The inner promise stays pending and the spawned sbx process keeps running with its stdout and stderr buffers growing. For sbx exec calls that hang past timeout + 10s, this leaks a process per call.
Escalate termination from the deadline handler, or expose a kill hook from the inner promise.
🛡️ Proposed direction
+ let killChild: (() => void) | undefined
const inner = new Promise<CommandResult>((resolve) => {
...
+ killChild = () => {+ child.kill('SIGKILL')+ }
...
})
const hardDeadline = timeout + 10_000
const deadlinePromise = new Promise<CommandResult>((resolve) => {
hardDeadlineId = setTimeout(() => {
opts.logger.log(`[${logLabel}] hard deadline (${hardDeadline}ms) hit for: ${cmdPreview}`)
+ killChild?.()
resolve({ stdout: '', stderr: `Command exceeded hard deadline of ${hardDeadline}ms`, exitCode: 124 })
}, hardDeadline)
})🧰 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)
🤖 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 107 - 115, Update the hard-deadline
handling around deadlinePromise and the spawned child so that when the deadline
timer fires, it terminates the child process before resolving the CommandResult.
Reuse the existing child reference or expose a kill hook from the inner
execution flow, and preserve the current timeout result and logging behavior.
| test('custom mount coexists with the project mount', async () => { | ||
| const tmpCustom = createTempDir() | ||
| const mockDocker = createMockDockerService() | ||
| const runtime = createMockSandboxRuntime() | ||
| const logger = createMockLogger() | ||
| const config: SandboxManagerConfig = { | ||
| image: 'oc-forge-sandbox:latest', | ||
| sourceProjectDir: '/main-project', | ||
| projectMountPath: '/project', | ||
| customMounts: [ | ||
| { host: tmpCustom, container: '/tools', readonly: false }, | ||
| ], | ||
| customMounts: [{ host: tmpCustom, readonly: false }], | ||
| } | ||
| const manager = createSandboxManager(mockDocker as unknown as DockerService, config, logger) | ||
| const manager = createSandboxManager(runtime, config, logger) | ||
| await manager.start('test', '/home/user/worktrees/feature') | ||
| const calls = mockDocker.getCreateContainerCalls() | ||
| const opts = calls[0][3] as { extraMounts?: string[] } | undefined | ||
| const mounts = opts?.extraMounts ?? [] | ||
| // Project mount | ||
| expect(mounts).toContain('/main-project:/project:ro') | ||
| // Custom mount | ||
| expect(mounts).toContain(`${resolve(tmpCustom)}:/tools`) | ||
| // Active mounts should have 4 entries: workspace, worktree self-mount, project, custom | ||
| const active = manager.getActive('test') | ||
| expect(active?.mounts).toHaveLength(4) | ||
| expect(active?.mounts[2]).toEqual({ hostDir: '/main-project', containerDir: '/project', readOnly: true }) | ||
| expect(active?.mounts[3]).toEqual({ hostDir: resolve(tmpCustom), containerDir: '/tools', readOnly: false }) | ||
| expect(active?.mounts).toHaveLength(3) | ||
| expect(active?.mounts[0]).toEqual({ hostDir: '/home/user/worktrees/feature', containerDir: '/home/user/worktrees/feature' }) | ||
| expect(active?.mounts[1]).toEqual({ hostDir: '/main-project', containerDir: '/main-project', readOnly: true }) | ||
| expect(active?.mounts[2]).toEqual({ hostDir: resolve(tmpCustom), containerDir: resolve(tmpCustom), readOnly: false }) | ||
| const workspaces = runtime.getCreateSandboxCalls()[0][1] | ||
| expect(workspaces).toContainEqual({ hostDir: '/main-project', readOnly: true }) | ||
| expect(workspaces).toContainEqual({ hostDir: resolve(tmpCustom), readOnly: false }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Check whether the project mount is guarded anywhere and how sbx create handles workspace paths.
rg -n --type=ts -C6 "hasProjectMount|sourceProjectDir" src/sandbox/manager.ts
rg -n --type=ts -C8 "createSandbox" src/sandbox/sbx.tsRepository: chriswritescode-dev/opencode-forge
Length of output: 3704
🏁 Script executed:
#!/bin/bashset -eu
printf'%s\n''--- manager mount construction and create call ---'
sed -n '160,245p' src/sandbox/manager.ts
printf'%s\n''--- sbx workspace argument construction ---'
sed -n '240,360p' src/sandbox/sbx.ts
printf'%s\n''--- workspace type and tests ---'
rg -n -C5 --type=ts "interface SandboxWorkspace|type SandboxWorkspace|buildSbxCreateArgs|workspace" src/sandbox/sbx.ts test/sandbox/manager-custom-mounts.test.ts
printf'%s\n''--- project configuration and documentation ---'
rg -n -C3 "sourceProjectDir|mountProjectReadonly|sbx create|workspace" README.md docs package.json src testRepository: chriswritescode-dev/opencode-forge
Length of output: 50391
🏁 Script executed:
#!/bin/bashset -eu
log=/tmp/coderabbit-shell-logs/shell-output-1uJDAf
printf'%s\n''--- requested command output ---'
sed -n '1,220p'"$log"printf'%s\n''--- exact sbx symbols ---'
rg -n -C8 --max-count 20 "SandboxWorkspace|buildSbxCreateArgs|workspace" src/sandbox/sbx.ts
printf'%s\n''--- exact manager create path ---'
sed -n '180,235p' src/sandbox/manager.ts
rg -n -C8 "createSandbox\\(" src/sandbox/manager.ts
printf'%s\n''--- relevant custom-mount test setup ---'
sed -n '130,185p' test/sandbox/manager-custom-mounts.test.tsRepository: chriswritescode-dev/opencode-forge
Length of output: 19032
🌐 Web query:
sbx CLI "create shell" workspace host directory missing path bind mount
💡 Result:
The sbx CLI requires explicit paths for workspace bind mounts [1][2]. When you encounter a path-related error during workspace setup, it often stems from incorrect path specification, missing permissions, or invalid Git worktree configurations [1][3][4]. To resolve or investigate "missing path" or "bind mount" errors with the sbx CLI, ensure you are following these guidelines: 1. Use Absolute Paths: While the CLI may accept relative paths like. (current directory), ensure the path is valid and exists on your host [1][2]. If a path is not being correctly resolved, try providing the full absolute path to the directory [1][3]. 2. Workspace Requirements: The first positional argument provided to sbx run or sbx create is the primary workspace [1][3]. Ensure this directory exists on your host machine before running the command. If you are attempting to mount extra directories, append:ro to those paths if you intend for them to be read-only [3][5]. 3. Git Worktree Constraints: If you are using --clone, ensure you are running the command from the main repository checkout [1][3]. Clone mode can be rejected if executed from within a Git worktree that is not the main one, as the read-only bind mount cannot resolve the worktree's.git pointer file [1][5]. 4. Troubleshooting Commands: - Use sbx diagnose to check for common issues, including missing storage directories or CLI/daemon version mismatches [4]. - If the environment remains corrupted, running sbx reset will stop running sandboxes and allow you to clean up or re-initialize the state [4]. - If you suspect a deeper issue with cached state or configuration, you can remove the sbx state directory as a last resort [4]. For further debugging, verify your current directory and the specific path arguments passed to your command. If you are attempting to mount a path that the sandbox agent cannot access due to host system permissions, the bind mount operation may fail silently or report an error during the creation process [3][4].
Citations:
- 1: https://github.com/docker/docs/blob/main/content/manuals/ai/sandboxes/usage.md
- 2: https://github.com/docker/docs/blob/f63001e0/data/sbx_cli/sbx_create_shell.yaml
- 3: https://docs.docker.com/ai/sandboxes/usage/
- 4: https://docs.docker.com/ai/sandboxes/troubleshooting/
- 5: https://github.com/docker/docs/blob/f63001e0/content/manuals/ai/sandboxes/usage.md
🌐 Web query:
site:github.com/docker/sbx "workspace" "does not exist" "sbx create"
💡 Result:
The error "workspace does not exist" or similar naming conflicts when using the sbx CLI (part of Docker Sandboxes) usually indicates an issue with how the sandbox is being referenced, a leftover name claim in the metadata database, or a misconfiguration of the workspace path [1][2]. If you are encountering errors during sbx create or sbx run, consider the following troubleshooting steps: 1. Check for Existing Sandbox Names: If the error relates to a name conflict (e.g., "sandbox already exists"), you may be trying to create a sandbox with a name that is already claimed [1][2]. Use sbx ls to list active sandboxes. If you wish to use that name, use sbx run --name to start the existing one, or pass --name to create a new one [1]. 2. Resolve "Leaked" Name Claims: Occasionally, a previous failed command can leave a name "claimed" in the metadata even if the container is gone [2]. If sbx ls does not show the sandbox but you still cannot create it, this may be a "leaked" name claim [2]. While there is no dedicated reclaim command, users have historically resolved this by ensuring the daemon is cleaned up or, in extreme cases, manually removing the stale entry from the local metadata database (usually located in %LOCALAPPDATA%\DockerSandboxes\sandboxes\state\sandboxd\meta.db on Windows) [2]. 3. Verify Workspace Path: The sbx command expects a valid workspace path [3]. If you are running the command from a directory that the tool does not recognize as a valid workspace, it may report that the workspace does not exist. Ensure you are executing the command from the root of the intended project directory or providing the explicit path to the workspace [3][4]. 4. Restart the Daemon: If the system enters an inconsistent state, you can attempt to refresh the background processes by running sbx daemon restart [5]. For further debugging, check the logs located in the Docker Sandboxes state directory (e.g., sandboxd/daemon.log) to see specific API error messages from the underlying container engine [2].
Citations:
- 1: Creating a sandbox for an existing branch should not error or fallback to main workspace docker/sbx-releases#127
- 2: sbx rm <name> --force reports 'not found' but name remains claimed in meta.db, blocking subsequent sbx run --name <name> docker/sbx-releases#129
- 3: https://github.com/docker/sbx-kits-contrib
- 4: Feature request: add Pi coding agent sandbox docker/sbx-releases#34
- 5: https://github.com/docker/sbx-releases/releases
🏁 Script executed:
#!/bin/bashset -eu
forurlin \
'https://raw.githubusercontent.com/docker/docs/main/data/sbx_cli/sbx_create_shell.yaml' \
'https://raw.githubusercontent.com/docker/docs/main/content/manuals/ai/sandboxes/usage.md' \
'https://raw.githubusercontent.com/docker/docs/main/content/manuals/ai/sandboxes/troubleshooting.md'doprintf'\n--- %s ---\n'"$url"
curl -LfsS "$url"| rg -n -i -C4 "workspace|bind|mount|exist|path|directory|source"doneRepository: chriswritescode-dev/opencode-forge
Length of output: 13569
🌐 Web query:
github docker sandboxes sbx create shell validate workspace path os.Stat source directory
💡 Result:
The sbx create shell command is part of the Docker Sandboxes CLI, designed to create a sandbox environment with access to a host workspace for interactive shell use [1][2]. When executing this command, you must provide a workspace path, which is then mounted inside the sandbox at the same absolute path as on the host [1][3]. Regarding your technical inquiry about os.Stat and path validation: The sbx CLI performs validation on the provided workspace path to ensure it exists on the host system before proceeding with the sandbox creation [1]. When you run sbx create shell <PATH>, the underlying implementation typically performs checks equivalent to os.Stat on the specified directory to verify its existence and accessibility before attempting to mount it into the containerized environment [1]. Key points regarding workspace and path handling: - Required Path: Unlike sbx run, the sbx create command requires an explicit workspace path [4][5]. - Mounting: The specified path is mounted into the sandbox at its absolute host path [1][3]. You can specify multiple paths as arguments, and append :ro to any path to mount it as read-only [1][3]. - Default Behavior: For sbx run shell, the workspace path defaults to the current directory if not explicitly provided [2]. - Path Issues: If you encounter issues with commands not executing in the expected directory, note that early versions of sbx exec defaulted to /home/agent/workspace rather than the mounted host directory [6]. Updates in version 0.33.0 and later addressed this to better align the working directory with the host-mounted project path [6]. If you are developing or debugging the sbx CLI, validation logic is typically contained within the CLI's command execution flow where it parses arguments and verifies source directories using standard Go filesystem operations before invoking the Docker daemon to create the sandbox [1][7].
Citations:
- 1: https://github.com/docker/docs/blob/f63001e0/data/sbx_cli/sbx_create_shell.yaml
- 2: https://docs.docker.com/ai/sandboxes/agents/shell/
- 3: https://github.com/docker/docs/blob/f63001e0/content/manuals/ai/sandboxes/usage.md
- 4: https://docs.docker.com/ai/sandboxes/usage/
- 5: https://github.com/docker/docs/blob/main/content/manuals/ai/sandboxes/usage.md
- 6: Execute
sbx execcommands in the project directory docker/sbx-releases#215 - 7: https://github.com/docker/docs/blob/f63001e0/data/sbx_cli/sbx.yaml
Skip a missing sourceProjectDir before sbx create.sbx create shell validates workspace paths before mounting them, so a stale sourceProjectDir causes sandbox creation to fail. Guard the project mount with existsSync(resolve(sourceProjectDir)) and add a regression test.
🤖 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 `@test/sandbox/manager-custom-mounts.test.ts` around lines 153 - 175, Update
the sandbox startup mount construction used by createSandboxManager so
sourceProjectDir is included only when existsSync(resolve(sourceProjectDir)) is
true, preventing stale paths from being passed to sbx create. Preserve custom
mounts and the worktree mount, and add a regression test covering a missing
sourceProjectDir.
| test('stop deletes the env file even when it holds no sandbox-env dir entry', async () => { | ||
| setEnv('FORGE_TEST_TOKEN', 'abc123') | ||
| const dataDir = createTempDataDir() | ||
| const runtime = createMockSandboxRuntime() | ||
| const logger = createMockLogger() | ||
| const config: SandboxManagerConfig = { | ||
| image: 'oc-forge-sandbox:latest', | ||
| dataDir, | ||
| network: { env: ['FORGE_TEST_TOKEN'] }, | ||
| } | ||
| const manager = createSandboxManager(runtime, config, logger) | ||
| await manager.start('test', '/home/user/worktrees/feature') | ||
| const envFile = manager.getActive('test')?.envFile! | ||
| expect(existsSync(envFile)).toBe(true) | ||
| // Simulate a stale active entry without a sandbox-env directory listing. | ||
| await manager.stop('test') | ||
| expect(existsSync(envFile)).toBe(false) | ||
| expect(readdirSync(join(dataDir, 'sandbox-env'))).toHaveLength(0) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The test name and the comment do not match what the test does.
The name claims the sandbox-env directory holds no entry for the env file, and Line 122 states that the test simulates a stale active entry. The body performs a normal start then stop, so nothing is stale and the directory entry exists until stop removes it. The test only re-checks the deletion already covered by the first test, plus an empty-directory assertion.
Either delete the misleading name and comment, or make the test create the stale condition, for example by removing the file before stop and asserting that stop still succeeds.
♻️ Minimal rename
- test('stop deletes the env file even when it holds no sandbox-env dir entry', async () => {+ test('stop deletes the env file and leaves the sandbox-env directory empty', async () => {- // Simulate a stale active entry without a sandbox-env directory listing.
await manager.stop('test')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('stop deletes the env file even when it holds no sandbox-env dir entry',async()=>{ | |
| setEnv('FORGE_TEST_TOKEN','abc123') | |
| constdataDir=createTempDataDir() | |
| construntime=createMockSandboxRuntime() | |
| constlogger=createMockLogger() | |
| constconfig: SandboxManagerConfig={ | |
| image: 'oc-forge-sandbox:latest', | |
| dataDir, | |
| network: {env: ['FORGE_TEST_TOKEN']}, | |
| } | |
| constmanager=createSandboxManager(runtime,config,logger) | |
| awaitmanager.start('test','/home/user/worktrees/feature') | |
| constenvFile=manager.getActive('test')?.envFile! | |
| expect(existsSync(envFile)).toBe(true) | |
| // Simulate a stale active entry without a sandbox-env directory listing. | |
| awaitmanager.stop('test') | |
| expect(existsSync(envFile)).toBe(false) | |
| expect(readdirSync(join(dataDir,'sandbox-env'))).toHaveLength(0) | |
| }) | |
| test('stop deletes the env file and leaves the sandbox-env directory empty',async()=>{ | |
| setEnv('FORGE_TEST_TOKEN','abc123') | |
| constdataDir=createTempDataDir() | |
| construntime=createMockSandboxRuntime() | |
| constlogger=createMockLogger() | |
| constconfig: SandboxManagerConfig={ | |
| image: 'oc-forge-sandbox:latest', | |
| dataDir, | |
| network: {env: ['FORGE_TEST_TOKEN']}, | |
| } | |
| constmanager=createSandboxManager(runtime,config,logger) | |
| awaitmanager.start('test','/home/user/worktrees/feature') | |
| constenvFile=manager.getActive('test')?.envFile! | |
| expect(existsSync(envFile)).toBe(true) | |
| awaitmanager.stop('test') | |
| expect(existsSync(envFile)).toBe(false) | |
| expect(readdirSync(join(dataDir,'sandbox-env'))).toHaveLength(0) | |
| }) |
🤖 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 `@test/sandbox/manager-env-passthrough.test.ts` around lines 105 - 127, Align
the test `stop deletes the env file even when it holds no sandbox-env dir entry`
with its actual behavior: either rename it and remove the stale-entry comment to
describe the normal start/stop cleanup, or create the stale condition by
removing the sandbox-env entry before calling `manager.stop('test')` and retain
assertions that stop succeeds and cleanup leaves the directory empty.
Summary
Migrates the sandbox runtime from the Docker driver to the
sbxCLI, and carries the full set of changes from the source branch — including the auditor-fallback chain and the loop-phase helper consolidation produced during review.Sandbox migration (primary)
sbx.ts(SandboxRuntime facade, the single sbx entry point),process.ts(the only child-process spawner),template.ts(missing-template build/load sequence),config-warnings.ts(legacy-docker warning collection), plus reworkedmanager.ts,path.ts,context.ts,reconcile.ts.docker.tsanddind-entrypoint.shremoved.normalizeHostPathnow canonicalizes symlinks viarealpathSync(fixes the overlapping-workspace drop rule for projects under symlinked paths); env-file lifecycle forsandbox.network.env.docker.io/docker/sandbox-templates:shell-docker; noENTRYPOINT/CMD/WORKDIR.Auditor-fallback chain + loop refactors
The loop-phase helper consolidation (
isAuditorPhase/loopRoleForPhaseinsrc/loop/state.ts, used acrossruntime.ts,tools/loop.ts,loop-helpers.ts) depends on the auditor-fallback chain (buildAuditorModelChain,resolveLoopAuditorChoice), which is carried here as a required dependency. That chain's content is also tracked separately in PR #81.Validation
pnpm typecheck && pnpm lint && pnpm test && pnpm build— all green.Deferred follow-ups (pr-review ledger)
sandbox.network.envpasses real secret values into the sandbox via a plaintext env file and the process env; routing host-bound credentials throughsbx secret set-customneeds a config-surface design. User reviewed and explicitly deferred.mode: 'sbx'config key with no validation; shipping it invites stale-config drift.sanitizeSbxName,sanitizeLoopName,slugify) with different character classes/caps; changing them alters existing container names.attachLoopToSessionsandbox-not-ready cleanup branch is uncovered by tests; a single delegating call, low risk.hostdoes not enforce the documented absolute-path rule; a relative value silently resolves against cwd.Summary by CodeRabbit
New Features
sbxsandboxing with template support, identical-path mounts, network allowlists, environment passthrough, and fail-closed execution.Bug Fixes
Documentation
sbxsetup, configuration, lifecycle, and troubleshooting.