Skip to content

fix(sandbox): reset SandboxManager singleton before re-initializing in session_start #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
onigunn wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(sandbox): reset SandboxManager singleton before re-initializing in session_start #3

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/sandbox/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,9 @@ export default function (pi: ExtensionAPI) {
: undefined;

try {
// Reset any stale state from a previous session (e.g. after /new)
try { await SandboxManager.reset(); } catch {}
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty catch {} blocks are likely to be flagged by the repo’s Biome linter (recommended rules) and also make it unclear that the error is intentionally ignored. Add an explanatory comment inside the catch block (or otherwise handle/log the error intentionally) to keep linting and intent consistent with the existing session_shutdown handler.

Suggested change
try { await SandboxManager.reset(); } catch {}
try {
await SandboxManager.reset();
} catch {
// Ignore reset failures: this is a best-effort cleanup before re-initializing.
}

Copilot uses AI. Check for mistakes.

await SandboxManager.initialize(
Comment on lines 398 to 402
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces new lifecycle behavior (defensive SandboxManager.reset() on session_start) that isn’t currently asserted in tests. Since this file already has Vitest coverage, add a unit test that verifies reset() is attempted before initialize(), and that a reset() rejection is swallowed and does not prevent initialize() from being called (regression coverage for the /new ordering issue).

Copilot uses AI. Check for mistakes.
{
network: activeConfig.network,
Comment on lines +399 to 404
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sandboxFailed is set to true when initialization throws, but it is never cleared on a subsequent successful initialize() call (nor reset at the start of session_start). Since the bash tool checks if (sandboxFailed) throw, a single transient init failure can permanently block bash for the lifetime of the process, even after later successful initialization. Consider resetting sandboxFailed (and related flags like sandboxEnabled/sandboxInitialized) at the beginning of this try block and explicitly setting sandboxFailed = false on the success path.

Copilot uses AI. Check for mistakes.
Expand Down
Loading