feat(sandbox): add per-session host sandbox toggle - #85
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
… cannot stall the plugin A persisted desired ON names a session from the previous run, so after a restart the ownership lookup targets a session the freshly-booting server may not answer for. The startup reconcile is awaited before the plugin returns its hooks, so an unbounded lookup blocked initialization forever and the TUI never rendered. Bound a single lookup at 5s; on timeout ownership resolves to 'uncertain', which already fails closed (nothing starts and the session is blocked rather than run host-side). The TUI sandbox indicator also drops the equals sign and reads SBX enabled/disabled, using the secondary colour while an acknowledged sandbox is active.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (14)
test/tui/session-sandbox-store.test.ts (2)
180-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the worker-based concurrency test fail loudly instead of hanging.
Two reliability gaps:
doneresolves only on amessageevent. If the worker throws — for example when the nativebetter-sqlite3binding fails to load — no message arrives andawait doneat line 203 hangs until the Vitest timeout. The real cause is then lost.- The test has no explicit timeout. It spends 500 ms in the worker write window and 500 ms in the main read loop, plus native module load time in a fresh worker. On a slow CI machine the total can approach Vitest's 5000 ms default.
Reject on worker errors and raise the timeout.
💚 Proposed fix
- const done = new Promise<{ writes: number }>((resolve) => worker.once('message', resolve))+ const done = new Promise<{ writes: number }>((resolve, reject) => {+ worker.once('message', resolve)+ worker.once('error', reject)+ worker.once('exit', (code) => {+ if (code !== 0) reject(new Error(`sandbox preference writer worker exited with code ${code}`))+ })+ })Set an explicit timeout on the test declaration at line 147:
- test('assembles desired and applied from one snapshot despite an intervening desired write', async () => {+ test('assembles desired and applied from one snapshot despite an intervening desired write', async () => {+ // ...+ }, 20_000)Apply the timeout as the third argument to
test, replacing the existing closing})at line 209.🤖 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/tui/session-sandbox-store.test.ts` around lines 180 - 209, Update the worker-based concurrency test around the worker creation and done promise to reject when the Worker emits an error, so failures such as native binding load errors surface immediately instead of hanging. Add an explicit timeout as the third argument to the test declaration, replacing its current closing call, and choose a value that accommodates the worker and read-loop durations on slow CI.
283-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a case for
applied.enabled === falsealone.
deriveSessionSandboxAcknowledgedrequiresapplied.enabledto be true. No test exercises that clause on its own. The current cases pair a falseapplied.enabledwith either anerroror a disableddesired, so removing theapplied.enabledcheck from the predicate would not fail this suite.+ test('derives OFF when a matching, error-free applied row reports disabled', () => {+ const appliedOff = writeApplied({ revision: 'r1', enabled: false, error: null })+ expect(deriveSessionSandboxAcknowledged({ desired: desired(), applied: appliedOff })).toBeNull()+ })🤖 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/tui/session-sandbox-store.test.ts` around lines 283 - 288, The test for deriveSessionSandboxAcknowledged should independently cover an applied record with enabled set to false and no error, while keeping desired enabled. Add a matching-revision case using writeApplied and assert the result is null, ensuring the applied.enabled requirement is tested without other disabling conditions.src/utils/tui-client.ts (1)
417-438: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecord the discovery failure cause with
tuiDebug.Both
catchblocks discard the error. The sandbox init loop insrc/tui.tsxretriesresolveTuiProjectIdevery 1.5 seconds and only observesnull, so a persistent failure gives no cause. This file already usestuiDebuginconnectForgeProject.try { const current = await client.project.current(directory ? { directory } : undefined) projectId = current?.id ?? null - } catch {+ } catch (err) {+ tuiDebug(`project.current failed: ${err instanceof Error ? err.message : String(err)}`) projectId = null }Apply the same change to the
project.listfallback.🤖 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/utils/tui-client.ts` around lines 417 - 438, Update both catch blocks in resolveTuiProjectId to capture the caught error and record its details with the existing tuiDebug utility, matching the usage in connectForgeProject. Apply the same diagnostic logging to failures from both client.project.current and the project.list fallback while preserving the existing null-return behavior.src/tui.tsx (1)
362-385: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider the cost of the perpetual 5s poll.
stepreschedules forever once started. Each tick callsreadSessionSandboxPreference, which opens a newDatabasehandle, runs a transaction, and closes the handle. The loop never stops while the plugin lives, so every TUI process pays this cost for the whole session, including when no desired state was ever written.Two options reduce the cost without changing the contract:
- Back off further when the pair is settled and
desiredis null (for example 15-30s), because nothing is pending.- Keep one long-lived read handle for the poll loop instead of reopening per tick.
Both are optional. The current behavior is correct.
🤖 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 362 - 385, Reduce the overhead of the perpetual polling started by ensureSandboxPolling without changing its acknowledgement behavior: when the preference pair is settled and desired is null, use a substantially longer reschedule interval (such as 15–30 seconds) while retaining the faster interval for unsettled or unavailable states.src/storage/repos/session-sandbox-preferences-repo.ts (2)
125-135: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winValidate state before persisting it.
setDesiredandsetAppliedwritestatewithout validation, butreadDesired/readAppliedvalidate on the way out viaparseDesired/parseApplied. If a caller ever writes an invalid state (for example, an emptyrevision), the write succeeds, but the next read silently returnsnulland discards it. Validate the state before the upsert to fail fast instead of silently losing data on read, since this data drives fail-closed sandbox reconciliation.As per path instructions, "Storage migrations and repositories must preserve explicit migration ordering and database integrity."
🛡️ Proposed fix to validate before write
setDesired(projectId: string, state: SessionSandboxDesiredState): void { + if (!parseDesired(state)) {+ throw new Error('invalid session sandbox desired state')+ } const ts = now() upsertStmt.run(projectId, SESSION_SANDBOX_DESIRED_KEY, JSON.stringify(state), ts) },🤖 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/storage/repos/session-sandbox-preferences-repo.ts` around lines 125 - 135, Validate the state in setDesired and setApplied with the same parseDesired and parseApplied validators used by readDesired and readApplied before calling upsertStmt.run. Reject invalid states, including empty revisions, so no invalid data is persisted; preserve the existing upsert behavior for valid states.Source: Path instructions
36-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the desired/applied validators.
parseDesiredandparseAppliedrepeat the sameversion,revision,enabled, andsessionIdchecks. Extract the shared checks into one helper. This reduces the risk that a future invariant change is applied to one function but not the other.♻️ Proposed refactor to share validation logic
+function parseCommon(o: Record<string, unknown>): { revision: string; enabled: boolean; sessionId: string | null } | null {+ if (o.version !== 1) return null+ if (typeof o.revision !== 'string' || o.revision.trim() === '') return null+ if (typeof o.enabled !== 'boolean') return null+ if (o.sessionId !== null && (typeof o.sessionId !== 'string' || o.sessionId.trim() === '')) return null+ return { revision: o.revision, enabled: o.enabled, sessionId: o.sessionId as string | null }+}+ function parseDesired(data: unknown): SessionSandboxDesiredState | null { if (typeof data !== 'object' || data === null) return null const o = data as Record<string, unknown> - if (o.version !== 1) return null- if (typeof o.revision !== 'string' || o.revision.trim() === '') return null- if (typeof o.enabled !== 'boolean') return null- if (o.sessionId !== null && (typeof o.sessionId !== 'string' || o.sessionId.trim() === '')) return null+ const common = parseCommon(o)+ if (!common) return null if (typeof o.requestedAt !== 'number' || !Number.isFinite(o.requestedAt)) return null - return {- version: 1,- revision: o.revision,- enabled: o.enabled,- sessionId: o.sessionId as string | null,- requestedAt: o.requestedAt,- }+ return { version: 1, ...common, requestedAt: o.requestedAt } }🤖 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/storage/repos/session-sandbox-preferences-repo.ts` around lines 36 - 70, Extract the duplicated version, revision, enabled, and sessionId validation from parseDesired and parseApplied into a shared helper, then have both parsers reuse its validated result while retaining their distinct error, timestamp, and return-shape checks.test/session-sandbox-preferences-repo.test.ts (1)
24-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct test coverage for
getPair.No test calls
repo.getPair(...)directly.getPairis the method that downstream consumers (session-sandbox-store.ts and session-controller.ts) rely on for an atomic desired/applied snapshot. Add a test that seeds both desired and applied state and assertsgetPairreturns them together, plus a case where one row is malformed and the other is valid.test('getPair returns both states from one call',()=>{constdesired=makeDesired({revision: 'd1'})constapplied=makeApplied({revision: 'a1'})repo.setDesired(PROJECT_A,desired)repo.setApplied(PROJECT_A,applied)expect(repo.getPair(PROJECT_A)).toEqual({ desired, applied })})🤖 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/session-sandbox-preferences-repo.test.ts` around lines 24 - 45, Add direct tests for the repository’s getPair method: seed desired and applied records for a project and assert one call returns both states together, then add a case with one malformed row and the other valid to verify the expected handling. Place the coverage alongside the existing SessionSandboxPreferencesRepo setup and use the existing makeDesired, makeApplied, PROJECT_A, setDesired, and setApplied helpers.test/unified-sandbox-resolver.test.ts (1)
143-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a loop restore rejection without
throwOnRestoreError.The suite covers a
nullloop sandbox under both flag states at Lines 150-151. It does not coverresolveLoopSandboxrejecting while the same loop stays active andthrowOnRestoreErroris unset.That path matters because the resolver treats the two failure modes differently. In
src/services/unified-sandbox-resolver.tsthe checkif (error !== undefined) throw errorruns before thethrowOnRestoreErrorcheck, so a rejection always propagates while anullresult only throws when the flag is set. A test would pin that asymmetry.💚 Proposed test
+ test('a loop restore rejection propagates even without throwOnRestoreError', async () => {+ const resolver = createUnifiedSandboxResolver(+ makeDeps({+ resolveActiveLoopForSession: async () => ({ loopName: 'loop-x', active: true, sandbox: true }),+ resolveLoopSandbox: async () => {+ throw new Error('loop restore failed')+ },+ }),+ )+ await expect(resolver('ses-1')).rejects.toThrow(/loop restore failed/)+ })+🤖 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/unified-sandbox-resolver.test.ts` around lines 143 - 152, Add a test alongside the existing unavailable-loop test that keeps the loop active, makes resolveLoopSandbox reject with an error, and calls the resolver without throwOnRestoreError; assert that the rejection propagates. Reuse the existing makeDeps and resolver setup, while preserving the current null-result assertions.src/sandbox/session-controller.ts (3)
258-271: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider canonicalizing both paths before the ownership comparison.
resolveOwnershipcomparesresolve(dir)withresolve(directory).resolvedoes not follow symlinks. If the OpenCode server reports a session directory through a symlinked path (for example/tmpvs/private/tmpon macOS) and the plugin receives the real path, ownership resolves toforeign. The toggle then silently never applies for that project.src/sandbox/manager.tsalready uses acanonicalizePathhelper for a comparable path comparison indetectGitMount.Reusing that helper here would make the comparison consistent with the rest of the sandbox module.
🤖 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/session-controller.ts` around lines 258 - 271, Update resolveOwnership to canonicalize both dir and directory with the existing canonicalizePath helper before comparing them, matching the path comparison approach used by detectGitMount in manager.ts. Preserve the existing local, foreign, and uncertain outcomes and lookup error handling.
784-793: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog when the supersede loop exhausts its iterations.
reconcilereturns silently afterMAX_SUPERSEDE_ITERATIONS. The next tick retries, so this is not a correctness problem. But a project whose desired revision churns faster than reconciliation can settle produces no signal at all. A single log line at the exit of the loop would make that state diagnosable.🤖 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/session-controller.ts` around lines 784 - 793, Add a single diagnostic log at the end of reconcile, after the MAX_SUPERSEDE_ITERATIONS loop completes without returning, indicating that reconciliation exhausted its supersede iterations for the project. Keep the existing early-return behavior unchanged and use the surrounding controller’s established logging mechanism.
424-432: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated
trustedOncomputation.
trustedOnis computed at Lines 425-432 and then shadowed by an identical expression at Lines 532-538. The inner copy omitsappliedAtDesiredRevision, which the enclosingif (applied && applied.revision === desired.revision)at Line 531 already guarantees. The two values are therefore always equal inside that block.Reuse the outer constant and delete the inner declaration. That removes the shadowing and keeps a single definition of "trusted persisted ON".
🤖 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/session-controller.ts` around lines 424 - 432, Remove the inner trustedOn declaration within the enclosing applied/revision check and reuse the outer trustedOn constant. Preserve the existing trust conditions and references so there is a single definition of “trusted persisted ON.”test/sandbox/session-controller.test.ts (1)
1095-1105: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign
ResolveActiveLoopForSessionwithResolvedLoop. The production resolver returnsResolvedLoop, which includesworktree?: boolean. TypeScript accepts the test callback through structural typing, so no compile error occurs. ReuseResolvedLoopor addworktree?: booleanto keep the dependency contract aligned with its producer.🤖 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/session-controller.test.ts` around lines 1095 - 1105, Update the resolveActiveLoopForSession callback contract used by createController and its test dependency to align with the producer’s ResolvedLoop type, preserving the optional worktree boolean field. Reuse ResolvedLoop where available, or add worktree?: boolean to the local type definition.test/parent-session-lookup.test.ts (1)
98-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
not-foundbranch.These tests cover the propagate path. The changed code also keeps one branch that swallows the error:
kind === 'not-found'records a failure, continues to the next attempt, and negative-caches the session. No test in this range asserts that branch, so a future change to thekindcheck would pass silently.Add a test that throws a
not-foundForgeClientError, expectsnull, and expects the second call to returnnullwithout a secondsession.getinvocation.🤖 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/parent-session-lookup.test.ts` around lines 98 - 147, Add a test near the existing transient failure cases for createParentSessionLookup that makes session.get throw a ForgeClientError with kind "not-found", verifies the first lookup returns null, and verifies a second lookup also returns null without invoking session.get again, confirming negative caching of definitive absence.src/hooks/plan-approval.ts (1)
140-146: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer
Object.hasOwnover theinoperator for tool-name lookups.
input.toolcomes from the model. Theinoperator also matches inherited keys, so a tool namedconstructorortoStringpasses the guard. The before hook then throwsnew Error(LOOP_BLOCKED_TOOLS[input.tool]!)with a prototype value instead of a block message.Object.hasOwnrestricts the check to real entries.♻️ Proposed refactor
- if (!(input.tool in LOOP_BLOCKED_TOOLS)) return+ if (!Object.hasOwn(LOOP_BLOCKED_TOOLS, input.tool)) return- if (input.tool in LOOP_BLOCKED_TOOLS) {+ if (Object.hasOwn(LOOP_BLOCKED_TOOLS, input.tool)) { blockedState = await resolveBlockedLoopToolState(loop, input.sessionID, deps) }Also applies to: 162-167
🤖 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/hooks/plan-approval.ts` around lines 140 - 146, Replace the LOOP_BLOCKED_TOOLS membership checks in the before-hook guards around resolveBlockedLoopToolState and the later block-message lookup with Object.hasOwn, so model-provided names such as constructor or toString are not treated as configured tools; preserve the existing behavior for actual own entries.
🤖 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/configuration.md`:
- Line 155: Update the toggleHostSandbox documentation entry to describe
enabling or disabling the host sandbox for the current session, matching the
behavior and wording of the corresponding description in tui.tsx while
preserving the existing keybind and prerequisite details.
In `@src/index.ts`:
- Around line 209-267: Update the SessionSandboxController acquisition and usage
flow around acquireSessionSandboxController so worktree instances do not
register or reuse the project-shared controller. Skip acquisition and
reconciliation for worktrees, or ensure any shared controller’s directory and
preference lookups are resolved from the project root so root sessions remain
owned and reconciled correctly.
In `@src/sandbox/session-controller.ts`:
- Around line 449-483: Preserve the fail-closed block when ownership is
uncertain, the desired state is enabled, and a session is selected, even if
sandboxManager.stop fails. Update the ownership-transfer failure path in the
surrounding reconciliation method to assign the same failedSelection state
before returning, or centralize and reuse that assignment so it cannot be
cleared by the early return.
In `@src/services/unified-sandbox-resolver.ts`:
- Around line 20-26: Update the doc comment for MAX_REVALIDATION_RETRIES to
state that exceeding the retry cap fails closed by throwing rather than falling
back to the most recently resolved loop context. Keep the existing description
of bounded revalidation and stale loop protection.
In `@src/tui.tsx`:
- Around line 455-457: Update the toggle direction logic near turningOff and
nextEnabled to derive the current state from deriveSessionSandboxAcknowledged
rather than pref.desired alone, treating a failed or unacknowledged apply as OFF
so the first press retries enabling the sandbox. Preserve the existing
session-specific toggle behavior for acknowledged states.
- Around line 387-414: Update the sandbox initialization polling in the
createEffect IIFE to make its retry delay disposal-aware: store the pending
sleep resolver alongside sandboxInitTimer, invoke it from the dispose handler
before clearing the timer, and register it when starting the timeout so the
promise always settles and the loop reaches its disposed check. Also enforce the
comment’s bounded-polling contract by adding an attempt or time limit while
preserving retries for transient project discovery failures.
---
Nitpick comments:
In `@src/hooks/plan-approval.ts`:
- Around line 140-146: Replace the LOOP_BLOCKED_TOOLS membership checks in the
before-hook guards around resolveBlockedLoopToolState and the later
block-message lookup with Object.hasOwn, so model-provided names such as
constructor or toString are not treated as configured tools; preserve the
existing behavior for actual own entries.
In `@src/sandbox/session-controller.ts`:
- Around line 258-271: Update resolveOwnership to canonicalize both dir and
directory with the existing canonicalizePath helper before comparing them,
matching the path comparison approach used by detectGitMount in manager.ts.
Preserve the existing local, foreign, and uncertain outcomes and lookup error
handling.
- Around line 784-793: Add a single diagnostic log at the end of reconcile,
after the MAX_SUPERSEDE_ITERATIONS loop completes without returning, indicating
that reconciliation exhausted its supersede iterations for the project. Keep the
existing early-return behavior unchanged and use the surrounding controller’s
established logging mechanism.
- Around line 424-432: Remove the inner trustedOn declaration within the
enclosing applied/revision check and reuse the outer trustedOn constant.
Preserve the existing trust conditions and references so there is a single
definition of “trusted persisted ON.”
In `@src/storage/repos/session-sandbox-preferences-repo.ts`:
- Around line 125-135: Validate the state in setDesired and setApplied with the
same parseDesired and parseApplied validators used by readDesired and
readApplied before calling upsertStmt.run. Reject invalid states, including
empty revisions, so no invalid data is persisted; preserve the existing upsert
behavior for valid states.
- Around line 36-70: Extract the duplicated version, revision, enabled, and
sessionId validation from parseDesired and parseApplied into a shared helper,
then have both parsers reuse its validated result while retaining their distinct
error, timestamp, and return-shape checks.
In `@src/tui.tsx`:
- Around line 362-385: Reduce the overhead of the perpetual polling started by
ensureSandboxPolling without changing its acknowledgement behavior: when the
preference pair is settled and desired is null, use a substantially longer
reschedule interval (such as 15–30 seconds) while retaining the faster interval
for unsettled or unavailable states.
In `@src/utils/tui-client.ts`:
- Around line 417-438: Update both catch blocks in resolveTuiProjectId to
capture the caught error and record its details with the existing tuiDebug
utility, matching the usage in connectForgeProject. Apply the same diagnostic
logging to failures from both client.project.current and the project.list
fallback while preserving the existing null-return behavior.
In `@test/parent-session-lookup.test.ts`:
- Around line 98-147: Add a test near the existing transient failure cases for
createParentSessionLookup that makes session.get throw a ForgeClientError with
kind "not-found", verifies the first lookup returns null, and verifies a second
lookup also returns null without invoking session.get again, confirming negative
caching of definitive absence.
In `@test/sandbox/session-controller.test.ts`:
- Around line 1095-1105: Update the resolveActiveLoopForSession callback
contract used by createController and its test dependency to align with the
producer’s ResolvedLoop type, preserving the optional worktree boolean field.
Reuse ResolvedLoop where available, or add worktree?: boolean to the local type
definition.
In `@test/session-sandbox-preferences-repo.test.ts`:
- Around line 24-45: Add direct tests for the repository’s getPair method: seed
desired and applied records for a project and assert one call returns both
states together, then add a case with one malformed row and the other valid to
verify the expected handling. Place the coverage alongside the existing
SessionSandboxPreferencesRepo setup and use the existing makeDesired,
makeApplied, PROJECT_A, setDesired, and setApplied helpers.
In `@test/tui/session-sandbox-store.test.ts`:
- Around line 180-209: Update the worker-based concurrency test around the
worker creation and done promise to reject when the Worker emits an error, so
failures such as native binding load errors surface immediately instead of
hanging. Add an explicit timeout as the third argument to the test declaration,
replacing its current closing call, and choose a value that accommodates the
worker and read-loop durations on slow CI.
- Around line 283-288: The test for deriveSessionSandboxAcknowledged should
independently cover an applied record with enabled set to false and no error,
while keeping desired enabled. Add a matching-revision case using writeApplied
and assert the result is null, ensuring the applied.enabled requirement is
tested without other disabling conditions.
In `@test/unified-sandbox-resolver.test.ts`:
- Around line 143-152: Add a test alongside the existing unavailable-loop test
that keeps the loop active, makes resolveLoopSandbox reject with an error, and
calls the resolver without throwOnRestoreError; assert that the rejection
propagates. Reuse the existing makeDeps and resolver setup, while preserving the
current null-result assertions.
🪄 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: 8ab7528a-39c4-4c41-9ef4-fd1d9a37c3de
📒 Files selected for processing (27)
docs/configuration.mdsrc/hooks/plan-approval.tssrc/hooks/sandbox-tools.tssrc/hooks/shell-env.tssrc/index.tssrc/sandbox/manager.tssrc/sandbox/session-controller.tssrc/services/unified-sandbox-resolver.tssrc/storage/index.tssrc/storage/repos/session-sandbox-preferences-repo.tssrc/tui.tsxsrc/tui/session-sandbox-store.tssrc/types-bun.d.tssrc/utils/logger.tssrc/utils/tui-client.tstest/__shims__/bun-sqlite.mjstest/hooks/shell-env.test.tstest/parent-session-lookup.test.tstest/plugin.test.tstest/sandbox-manager.test.tstest/sandbox-tools.test.tstest/sandbox/manager-env-passthrough.test.tstest/sandbox/manager-reliability.test.tstest/sandbox/session-controller.test.tstest/session-sandbox-preferences-repo.test.tstest/tui/session-sandbox-store.test.tstest/unified-sandbox-resolver.test.ts
| | `tui.showVersion` | `true` | Show the Forge version in the sidebar title. | | ||
| | `tui.keybinds.executePlan` | `"<leader>f"` | Open the execution dialog. Avoid `<leader>e`, which conflicts with opencode's built-in `editor_open`. | | ||
| | `tui.keybinds.dashboard` | `""` | Optional keybind for opening the dashboard. Empty registers the command without a default binding. | | ||
| | `tui.keybinds.toggleHostSandbox` | `""` | Optional keybind for `Toggle host sandbox`, which runs the current session inside a sandbox container. Empty registers the command without a default binding. Requires `sandbox.enabled`. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe both toggle directions.
The text says the command "runs the current session inside a sandbox container". The command enables and disables the host sandbox. The in-code description at src/tui.tsx reads "Enable or disable the host sandbox for the current session". Align the documentation with that behavior.
-| `tui.keybinds.toggleHostSandbox` | `""` | Optional keybind for `Toggle host sandbox`, which runs the current session inside a sandbox container. Empty registers the command without a default binding. Requires `sandbox.enabled`. |+| `tui.keybinds.toggleHostSandbox` | `""` | Optional keybind for `Toggle host sandbox`, which enables or disables the sandbox container for the current session. Empty registers the command without a default binding. Requires `sandbox.enabled`. |📝 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.
| |`tui.keybinds.toggleHostSandbox`|`""`| Optional keybind for `Toggle host sandbox`, which runs the current session inside a sandbox container. Empty registers the command without a default binding. Requires `sandbox.enabled`. | | |
| |`tui.keybinds.toggleHostSandbox`|`""`| Optional keybind for `Toggle host sandbox`, which enables or disables the sandbox container for the current session. Empty registers the command without a default binding. Requires `sandbox.enabled`. | |
🤖 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` at line 155, Update the toggleHostSandbox
documentation entry to describe enabling or disabling the host sandbox for the
current session, matching the behavior and wording of the corresponding
description in tui.tsx while preserving the existing keybind and prerequisite
details.
| /** | ||
| * Process-wide registry of host-session sandbox controllers, keyed by project id. | ||
| * | ||
| * OpenCode can instantiate this plugin more than once for the same directory in a single process, | ||
| * and every instance builds its own database handle, sandbox manager and controller. Two | ||
| * controllers reconciling the same per-project preference row race on one container: one creates | ||
| * while the other force-deletes underneath it, which surfaces as `operation in progress`, | ||
| * `already exists`, `failed to run sandbox container`, or an acknowledgement timeout. The | ||
| * container and the preference row are both per project, so exactly one reconciler may exist per | ||
| * project per process; additional instances share it and release it by reference count. | ||
| */ | ||
| type SharedSessionSandboxController = { | ||
| controller: SessionSandboxController | ||
| started: Promise<void> | ||
| refs: number | ||
| close: () => void | ||
| } | ||
| const sharedSessionSandboxControllers = new Map<string, SharedSessionSandboxController>() | ||
| /** | ||
| * Returns the process-wide controller for `projectId`, creating and starting it on first use. | ||
| * The returned `started` promise is shared, so every caller awaits the same initial reconcile | ||
| * rather than triggering a second one. | ||
| */ | ||
| function acquireSessionSandboxController( | ||
| projectId: string, | ||
| create: () => { controller: SessionSandboxController; close: () => void }, | ||
| ): SharedSessionSandboxController { | ||
| const existing = sharedSessionSandboxControllers.get(projectId) | ||
| if (existing) { | ||
| existing.refs += 1 | ||
| return existing | ||
| } | ||
| const { controller, close } = create() | ||
| const entry: SharedSessionSandboxController = { controller, started: controller.start(), refs: 1, close } | ||
| sharedSessionSandboxControllers.set(projectId, entry) | ||
| return entry | ||
| } | ||
| /** | ||
| * Drops one reference and disposes the controller once the last instance releases it. The | ||
| * controller owns a dedicated database handle, closed here after disposal, so it can outlive the | ||
| * instance that happened to create it: instances release before closing their own handles, and a | ||
| * borrowed handle would otherwise be closed while other instances still hold a reference. | ||
| */ | ||
| async function releaseSessionSandboxController(projectId: string): Promise<void> { | ||
| const entry = sharedSessionSandboxControllers.get(projectId) | ||
| if (!entry) return | ||
| entry.refs -= 1 | ||
| if (entry.refs > 0) return | ||
| sharedSessionSandboxControllers.delete(projectId) | ||
| try { | ||
| await entry.controller.dispose() | ||
| } finally { | ||
| entry.close() | ||
| } | ||
| } | ||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Inspect ownership resolution and worktree-instance guards.
rg -n -C5 'isForgeWorktreeDir' --type=ts
rg -n -C10 'resolveOwnership|resolve\(dir\) === resolve\(directory\)' src/sandbox/session-controller.tsRepository: chriswritescode-dev/opencode-forge
Length of output: 172
🏁 Script executed:
#!/bin/bashset -e
printf'%s\n''--- candidate files ---'
git ls-files | rg '(^|/)(index\.ts|session-controller\.ts)$'printf'%s\n''--- worktree and acquisition references ---'
rg -n -C8 'ForgeWorktree|forge worktree|acquireSessionSandboxController|releaseSessionSandboxController|isForge' src --glob '*.ts'||trueprintf'%s\n''--- session controller outline ---'if [ -f src/sandbox/session-controller.ts ];then
ast-grep outline src/sandbox/session-controller.ts
fiprintf'%s\n''--- index outline ---'if [ -f src/index.ts ];then
ast-grep outline src/index.ts
fiRepository: chriswritescode-dev/opencode-forge
Length of output: 18629
🏁 Script executed:
#!/bin/bashset -e
printf'%s\n''--- plugin setup and directory identity ---'
sed -n '274,330p;390,455p;580,635p' src/index.ts
printf'%s\n''--- session controller ownership implementation ---'
sed -n '60,190p' src/sandbox/session-controller.ts
printf'%s\n''--- remaining ownership references ---'
rg -n -C12 'resolveOwnership|foreign|directory|projectId' src/sandbox/session-controller.ts
printf'%s\n''--- project-id creation and worktree helper tests/usages ---'
rg -n -C8 'projectId|isForgeWorktreeDir|forgeWorktreeDir' src/index.ts src/utils src/workspace --glob '*.ts'Repository: chriswritescode-dev/opencode-forge
Length of output: 50391
🏁 Script executed:
#!/bin/bashset -e
printf'%s\n''--- session directory lookup ---'
sed -n '130,220p' src/index.ts
printf'%s\n''--- controller startup and reconcile entry points ---'
sed -n '190,290p;400,490p;780,825p' src/sandbox/session-controller.ts
printf'%s\n''--- tests covering worktree acquisition or ownership ---'
git ls-files | rg '(^|/)(test|tests|__tests__|.*\.test\.|.*\.spec\.)'| head -200
rg -n -C6 'getSessionDirectory|resolveOwnership|acquireSessionSandboxController|isForgeWorktreeDir' --glob '*.{test,spec}.{ts,tsx,js,jsx}' --glob '*.test.*' --glob '*.spec.*'.||trueRepository: chriswritescode-dev/opencode-forge
Length of output: 50390
🏁 Script executed:
#!/bin/bashset -e
python3 - <<'PY'from pathlib import Pathimport reindex = Path("src/index.ts").read_text()controller = Path("src/sandbox/session-controller.ts").read_text()acquire = re.search( r"const sharedSessionSandbox = acquireSessionSandboxController\(projectId, \(\) => \{(.*?)\n\s*\}\)", index, re.S,)assert acquire, "shared controller acquisition block not found"block = acquire.group(1)assert "if (!isForgeWorktreeDir(dataDir, directory))" not in index[index.index("const sharedSessionSandbox"):index.index("const sharedSessionSandbox") + 4000]assert "directory," in blockassert "getSessionDirectory: sessionDirectoryLookup" in blockassert "resolve(dir) === resolve(directory)" in controller# Model the exact first-wins behavior and ownership comparison.project_id = "same-project"root = "/repo"worktree = "/data/worktrees/loop-a"registry = {}def acquire(directory): if project_id not in registry: registry[project_id] = {"directory": directory} return registry[project_id]entry = acquire(worktree) # worktree child initializes firstsame_entry = acquire(root) # root instance reuses the entryassert entry is same_entryassert same_entry["directory"] == worktreesession_directory = rootownership = "local" if session_directory == same_entry["directory"] else "foreign"assert ownership == "foreign"print("acquisition_guarded_for_worktrees:", "no")print("controller_directory_after_child_then_root:", same_entry["directory"])print("root_session_ownership:", ownership)print("root_preference_reconciliation:", "skipped")PYRepository: chriswritescode-dev/opencode-forge
Length of output: 349
Prevent Forge worktree instances from acquiring the shared controller. Acquisition is unconditional, so a worktree instance can bind the project-shared controller to its directory. A later root instance reuses that controller, and resolveOwnership treats root sessions as foreign, so the root preference row is not reconciled. Skip controller acquisition and use for worktree instances, or resolve the controller directory and lookups from the project root.
🤖 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/index.ts` around lines 209 - 267, Update the SessionSandboxController
acquisition and usage flow around acquireSessionSandboxController so worktree
instances do not register or reuse the project-shared controller. Skip
acquisition and reconciliation for worktrees, or ensure any shared controller’s
directory and preference lookups are resolved from the project root so root
sessions remain owned and reconciled correctly.
| if (hostActive) { | ||
| try { | ||
| await sandboxManager.stop(managerKey) | ||
| } catch (err) { | ||
| logger.log( | ||
| `[session-sandbox] stop failed during ownership transfer: ${err instanceof Error ? err.message : String(err)}`, | ||
| ) | ||
| // A failed stop means the container may still be live. Retain retryable ownership and | ||
| // record pending cleanup so no superseding start can adopt or acknowledge it until | ||
| // removal succeeds. | ||
| hostActive = true | ||
| pendingCleanup = true | ||
| lastValidatedRevision = null | ||
| acknowledgedSessionId = null | ||
| acknowledgedRevision = null | ||
| failedSelection = null | ||
| return desired.revision | ||
| } | ||
| hostActive = false | ||
| lastValidatedRevision = null | ||
| } | ||
| bind(null) | ||
| // Uncertain ownership with an ON request must fail closed: this instance could not confirm it | ||
| // owns the selected session (e.g. a transient directory-lookup failure), so blocking host | ||
| // fallback is safer than running tools on the host while the shared ON row is left untouched. | ||
| // Re-evaluated on the next reconcile tick once ownership can be confirmed. | ||
| if (ownership === 'uncertain' && desired.enabled && desired.sessionId) { | ||
| failedSelection = { | ||
| sessionId: desired.sessionId, | ||
| error: 'Host sandbox ownership could not be confirmed for the selected session', | ||
| } | ||
| } else { | ||
| failedSelection = null | ||
| } | ||
| return desired.revision |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Uncertain ownership with a failed stop clears the fail-closed block.
The ownership-transfer failure path at Lines 456-465 sets failedSelection = null and returns. It runs before the uncertain-ownership block at Lines 475-479. So when ownership is uncertain, the desired state is ON, and sandboxManager.stop fails, the selected session is left unblocked. resolveSandboxForSession then returns null for that session and the hooks treat null as permission to run on the host. This contradicts the stated invariant in the comment at Lines 471-474.
The window lasts until a later tick performs a successful stop and reaches Line 475.
Move the uncertain-ON fail-closed assignment before the early return, or reuse it on the failure path.
🛡️ Proposed fix
if (ownership !== 'local') {
+ // An ON request whose ownership could not be confirmed must fail closed regardless of+ // whether the transfer stop succeeds.+ const uncertainOnBlock =+ ownership === 'uncertain' && desired.enabled && desired.sessionId+ ? {+ sessionId: desired.sessionId,+ error: 'Host sandbox ownership could not be confirmed for the selected session',+ }+ : null
if (hostActive) {
try {
await sandboxManager.stop(managerKey)
} catch (err) {
logger.log(
`[session-sandbox] stop failed during ownership transfer: ${err instanceof Error ? err.message : String(err)}`,
)
hostActive = true
pendingCleanup = true
lastValidatedRevision = null
acknowledgedSessionId = null
acknowledgedRevision = null
- failedSelection = null+ failedSelection = uncertainOnBlock
return desired.revision
}
hostActive = false
lastValidatedRevision = null
}
bind(null)
- if (ownership === 'uncertain' && desired.enabled && desired.sessionId) {- failedSelection = {- sessionId: desired.sessionId,- error: 'Host sandbox ownership could not be confirmed for the selected session',- }- } else {- failedSelection = null- }+ failedSelection = uncertainOnBlock
return desired.revision
}📝 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.
| if(hostActive){ | |
| try{ | |
| awaitsandboxManager.stop(managerKey) | |
| }catch(err){ | |
| logger.log( | |
| `[session-sandbox] stop failed during ownership transfer: ${errinstanceofError ? err.message : String(err)}`, | |
| ) | |
| // A failed stop means the container may still be live. Retain retryable ownership and | |
| // record pending cleanup so no superseding start can adopt or acknowledge it until | |
| // removal succeeds. | |
| hostActive=true | |
| pendingCleanup=true | |
| lastValidatedRevision=null | |
| acknowledgedSessionId=null | |
| acknowledgedRevision=null | |
| failedSelection=null | |
| returndesired.revision | |
| } | |
| hostActive=false | |
| lastValidatedRevision=null | |
| } | |
| bind(null) | |
| // Uncertain ownership with an ON request must fail closed: this instance could not confirm it | |
| // owns the selected session (e.g. a transient directory-lookup failure), so blocking host | |
| // fallback is safer than running tools on the host while the shared ON row is left untouched. | |
| // Re-evaluated on the next reconcile tick once ownership can be confirmed. | |
| if(ownership==='uncertain'&&desired.enabled&&desired.sessionId){ | |
| failedSelection={ | |
| sessionId: desired.sessionId, | |
| error: 'Host sandbox ownership could not be confirmed for the selected session', | |
| } | |
| }else{ | |
| failedSelection=null | |
| } | |
| returndesired.revision | |
| constuncertainOnBlock= | |
| ownership==='uncertain'&&desired.enabled&&desired.sessionId | |
| ? { | |
| sessionId: desired.sessionId, | |
| error: 'Host sandbox ownership could not be confirmed for the selected session', | |
| } | |
| : null | |
| if(hostActive){ | |
| try{ | |
| awaitsandboxManager.stop(managerKey) | |
| }catch(err){ | |
| logger.log( | |
| `[session-sandbox] stop failed during ownership transfer: ${errinstanceofError ? err.message : String(err)}`, | |
| ) | |
| // A failed stop means the container may still be live. Retain retryable ownership and | |
| // record pending cleanup so no superseding start can adopt or acknowledge it until | |
| // removal succeeds. | |
| hostActive=true | |
| pendingCleanup=true | |
| lastValidatedRevision=null | |
| acknowledgedSessionId=null | |
| acknowledgedRevision=null | |
| failedSelection=uncertainOnBlock | |
| returndesired.revision | |
| } | |
| hostActive=false | |
| lastValidatedRevision=null | |
| } | |
| bind(null) | |
| // Uncertain ownership with an ON request must fail closed: this instance could not confirm it | |
| // owns the selected session (e.g. a transient directory-lookup failure), so blocking host | |
| // fallback is safer than running tools on the host while the shared ON row is left untouched. | |
| // Re-evaluated on the next reconcile tick once ownership can be confirmed. | |
| failedSelection=uncertainOnBlock | |
| returndesired.revision |
🤖 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/session-controller.ts` around lines 449 - 483, Preserve the
fail-closed block when ownership is uncertain, the desired state is enabled, and
a session is selected, even if sandboxManager.stop fails. Update the
ownership-transfer failure path in the surrounding reconciliation method to
assign the same failedSelection state before returning, or centralize and reuse
that assignment so it cannot be cleared by the early return.
| /** | ||
| * Bounded revalidation retries. After an asynchronous loop sandbox restoration a loop may have | ||
| * terminated, changed mode, or been replaced; loop membership is re-checked and re-routed up to | ||
| * this many times so a stale loop context is never returned. A loop that keeps changing identity | ||
| * past this cap falls back to the most recently resolved loop context rather than looping forever. | ||
| */ | ||
| const MAX_REVALIDATION_RETRIES = 4 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the doc comment: exhaustion fails closed, it does not fall back.
The comment states that a loop which keeps changing identity past the cap "falls back to the most recently resolved loop context". Lines 75-79 throw in every branch instead. The comment misdescribes the fail-closed contract that callers such as createShellEnvHook depend on.
📝 Proposed doc fix
- * this many times so a stale loop context is never returned. A loop that keeps changing identity- * past this cap falls back to the most recently resolved loop context rather than looping forever.+ * this many times so a stale loop context is never returned. A loop that keeps changing identity+ * past this cap fails closed: resolution throws instead of returning a possibly stale context.📝 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.
| /** | |
| *Boundedrevalidationretries.Afteranasynchronousloopsandboxrestorationaloopmayhave | |
| *terminated,changedmode,orbeenreplaced;loopmembership is re-checkedandre-routedupto | |
| *thismanytimessoastaleloopcontextis never returned.Aloopthatkeepschangingidentity | |
| *pastthiscapfallsbacktothemostrecentlyresolvedloopcontextratherthanloopingforever. | |
| */ | |
| constMAX_REVALIDATION_RETRIES=4 | |
| /** | |
| *Boundedrevalidationretries.Afteranasynchronousloopsandboxrestorationaloopmayhave | |
| *terminated,changedmode,orbeenreplaced;loopmembership is re-checkedandre-routedupto | |
| *thismanytimessoastaleloopcontextis never returned.Aloopthatkeepschangingidentity | |
| *pastthiscapfailsclosed: resolutionthrowsinsteadofreturningapossiblystalecontext. | |
| */ | |
| constMAX_REVALIDATION_RETRIES=4 |
🤖 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/unified-sandbox-resolver.ts` around lines 20 - 26, Update the
doc comment for MAX_REVALIDATION_RETRIES to state that exceeding the retry cap
fails closed by throwing rather than falling back to the most recently resolved
loop context. Keep the existing description of bounded revalidation and stale
loop protection.
| createEffect(() => { | ||
| if (!api.state.ready || sandboxInitStarted) return | ||
| sandboxInitStarted = true | ||
| void (async () => { | ||
| // Retry transient project discovery with bounded, disposal-aware polling so a temporarily | ||
| // failing lookup (or a forge.db that is not yet available) does not permanently leave the | ||
| // acknowledged state OFF for this process. Polling below also retries unavailable DB reads. | ||
| let projectId: string | null = null | ||
| while (!disposed && !projectId) { | ||
| projectId = await resolveTuiProjectId(api, directory) | ||
| if (disposed || projectId) break | ||
| await new Promise<void>((resolve) => { | ||
| sandboxInitTimer = setTimeout(() => { | ||
| sandboxInitTimer = null | ||
| resolve() | ||
| }, 1500) | ||
| }) | ||
| } | ||
| if (disposed) return | ||
| setSandboxProjectId(projectId) | ||
| if (!projectId) return | ||
| // Poll until the preference pair settles. On a clean restart the applied | ||
| // row can lag the persisted desired state while the server reconciles, so | ||
| // a single read would leave ON invisible forever. ensureSandboxPolling | ||
| // reuses the store reader and stops as soon as the pair is settled. | ||
| ensureSandboxPolling(projectId) | ||
| })() | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
The retry sleep never settles when disposal happens during the wait.
The dispose handler at line 335 clears sandboxInitTimer but does not call the pending resolve. The promise created at line 398 then never settles. The async IIFE stays suspended and keeps api, pluginConfig, and the loop closure alive. The if (disposed) return check at line 405 is never reached.
Also, the comment says "bounded" polling. The loop has no attempt or time limit. It only exits on success or disposal.
Store the resolver so the dispose handler can settle it, or drive the wait from api.lifecycle.signal.
🛠️ Proposed fix to settle the pending sleep on disposal
Add a resolver alongside the timer near line 324:
let sandboxInitTimer: ReturnType<typeof setTimeout> | null = null
+let sandboxInitWake: (() => void) | null = nullSettle it in the dispose handler:
if (sandboxInitTimer) {
clearTimeout(sandboxInitTimer)
sandboxInitTimer = null
}
+ if (sandboxInitWake) {+ sandboxInitWake()+ sandboxInitWake = null+ }Register it when the sleep starts:
await new Promise<void>((resolve) => {
+ sandboxInitWake = resolve
sandboxInitTimer = setTimeout(() => {
sandboxInitTimer = null
+ sandboxInitWake = null
resolve()
}, 1500)
})🤖 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 387 - 414, Update the sandbox initialization
polling in the createEffect IIFE to make its retry delay disposal-aware: store
the pending sleep resolver alongside sandboxInitTimer, invoke it from the
dispose handler before clearing the timer, and register it when starting the
timeout so the promise always settles and the loop reaches its disposed check.
Also enforce the comment’s bounded-polling contract by adding an attempt or time
limit while preserving retries for transient project discovery failures.
| const { desired } = pref | ||
| const turningOff = desired?.enabled === true && desired.sessionId === sessionId | ||
| const nextEnabled = !turningOff |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The toggle direction ignores a failed apply, so the first press repeats the failure state.
turningOff uses only desired. Consider this persisted pair:
desired = { revision: 'r1', enabled: true, sessionId: 'sess-1' }applied = { revision: 'r1', enabled: false, error: 'sbx failed to start' }
deriveSessionSandboxAcknowledged returns null for that pair, so the sidebar shows SBX disabled. The user then presses the toggle to enable the sandbox. turningOff evaluates to true, so nextEnabled becomes false and the TUI sends a disable request. The user must press the toggle twice to retry the enable.
Derive the direction from the acknowledged state instead, so a failed apply is treated as OFF.
🐛 Proposed fix to derive the toggle direction from the acknowledged state
- const { desired } = pref- const turningOff = desired?.enabled === true && desired.sessionId === sessionId+ // Derive from the acknowledged pair, not from `desired` alone: a desired ON whose+ // apply failed renders as OFF, so the next press must retry ON rather than send OFF.+ const acknowledged = deriveSessionSandboxAcknowledged(pref)+ const turningOff = acknowledged !== null && acknowledged.sessionId === sessionId
const nextEnabled = !turningOff📝 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.
| const{ desired }=pref | |
| constturningOff=desired?.enabled===true&&desired.sessionId===sessionId | |
| constnextEnabled=!turningOff | |
| // Derive from the acknowledged pair, not from `desired` alone: a desired ON whose | |
| // apply failed renders as OFF, so the next press must retry ON rather than send OFF. | |
| constacknowledged=deriveSessionSandboxAcknowledged(pref) | |
| constturningOff=acknowledged!==null&&acknowledged.sessionId===sessionId | |
| constnextEnabled=!turningOff |
🤖 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 455 - 457, Update the toggle direction logic near
turningOff and nextEnabled to derive the current state from
deriveSessionSandboxAcknowledged rather than pref.desired alone, treating a
failed or unacknowledged apply as OFF so the first press retries enabling the
sandbox. Preserve the existing session-specific toggle behavior for acknowledged
states.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Adds a per-session host sandbox toggle: a session outside any loop can be run inside an
sbxcontainer from the TUI, reconciled server-side through a persisted desired/applied preference pair.Behavior
forge.sandbox.toggleHost("Toggle host sandbox"), with an optionaltui.keybinds.toggleHostSandboxbinding (empty by default, so the command registers without a default key).desiredrow (revision, session, enabled); the server-sideSessionSandboxControllerreconciles it and writes anappliedrow at the same revision. ON is trusted only when the revisions match, both target the same session, andappliedcarries no error.Fail-closed routing
createUnifiedSandboxResolveris the single loop-first resolver feedingbash,glob, andgrep. Loop resolution always wins: an active sandbox loop owns its sessions, an active non-sandbox loop forces host, and only sessions with no active loop consult the host-session sandbox.One controller per project, per process
OpenCode can instantiate this plugin more than once for the same directory in a single process, and each instance builds its own database handle, sandbox manager, and controller. Two controllers reconciling the same per-project preference row race on one container — one creates while the other force-deletes underneath it, surfacing as
409 operation in progress,already exists,500 failed to run sandbox container, or an acknowledgement timeout. Because the container and the preference row are both per project,src/index.tsnow keeps a process-wide registry keyed by project id: the first instance constructs and starts the controller, later instances share it and await the same initial reconcile, and disposal is reference-counted. The shared controller owns a dedicated database handle so it never depends on the lifetime of whichever instance happened to create it.The container key is derived from the project id rather than the instance directory, matching the granularity of the preference row. Consequently a non-owning instance may no longer stop the shared container: it removes only a container it started itself, and
restoringPersistedOnis set only once ownership is confirmed local, since that flag makes disposal stop the key.Fixes found while validating
bun:sqliteopen flags.openForgeDbusednew Database(path, { create: false }), which sets neitherSQLITE_OPEN_READONLYnorSQLITE_OPEN_READWRITEand therefore throws unconditionally at runtime. Every TUI read and write of the preference row failed, so the toggle could never persist anything. Now{ readwrite: true, create: false }.test/__shims__/bun-sqlite.mjsforwarded Bun options straight intobetter-sqlite3, which has unrelated option semantics and never validates open flags, so the whole suite passed against a code path that cannot execute under Bun. The shim now reproduces Bun's flag validation and mapscreate: falsetofileMustExist.bun:sqlitetype declaration insrc/types-bun.d.tsomittedreadwrite, so the correct option was not expressible while the broken one type-checked.readSessionSandboxPreferencereturnsunavailableReasoninstead of swallowing the error, so a misresolved database path reportsno such table: tui_preferences.forge.log, so concurrent work by two instances was indistinguishable from one instance repeating itself. Lines now carrypid:instanceId.Notes
logger.tsstill clears the log file on init by default. With a shared log file this erases a second instance's startup lines; left as-is here since it is pre-existing behaviour with a documented rationale.sbx create. The duplicate-controller race made that stall for 30s+ and block plugin init; with the race fixed this is no longer reachable in practice, but the startup path is still unbounded and would benefit from a timeout that degrades to "sandbox unavailable".Validation
pnpm typecheck && pnpm lint && pnpm test && pnpm build— all green (3116 tests).Two behaviours were verified by deliberately reverting the fix and confirming the new tests fail: removing the process-wide controller registry fails
two plugin instances for one project share a single refcounted sandbox controller, and restoring{ create: false }fails 14 tests insession-sandbox-store.Summary by CodeRabbit
New Features
Bug Fixes