From 0c720f3a28276a00d41a32bc897d05bd4b04d979 Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Tue, 14 Jul 2026 12:36:02 +0100 Subject: [PATCH 1/7] fix(desktop): sandbox boot-retry, status, per-dashboard workspace, single-user UI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Auto-retry the local session-create for ~30s (12×3s) while the desktop VM boots, instead of failing the first terminal on a cold launch with "Network connection lost" (ensureDashboardSandboxCreate legacy/shared-SANDBOX_URL path). - Fix the sandbox status showing red "Sandbox asleep" while the local VM is healthy: the desktop sandbox has no Fly machine, so the status guard keyed on sandbox_machine_id short-circuited before the desktop "ready" branch. Now keys "no sandbox" on sandbox_session_id and returns ready for non-Fly deployments. - Per-dashboard workspace isolation on desktop: default each dashboard's terminals to /workspace/ (shared-sandbox deployments only; downloaded dashboards already set their own workingDir). The sandbox auto-creates the subdir on the desktop VM (ORCABOT_DEBUG_EXEC gate) so the first terminal lands there instead of falling back to the shared root; cloud is unchanged (each dashboard has its own VM). - Hide the multiplayer "N online" presence indicator on desktop (always one local user). Note: the sandbox auto-create (session.go) needs a VM-image rebuild to take effect for newly-created dashboards; downloaded dashboards isolate immediately (their subdir already exists). Control-plane + frontend changes take effect on a resources rebuild. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ --- controlplane/src/sessions/handler.ts | 56 ++++++++++++++++--- .../src/app/(app)/dashboards/[id]/page.tsx | 54 +++++++++--------- sandbox/internal/sessions/session.go | 12 ++++ 3 files changed, 88 insertions(+), 34 deletions(-) diff --git a/controlplane/src/sessions/handler.ts b/controlplane/src/sessions/handler.ts index 9f34f04d..5f8629e0 100644 --- a/controlplane/src/sessions/handler.ts +++ b/controlplane/src/sessions/handler.ts @@ -526,7 +526,28 @@ async function ensureDashbоardSandbоxCreate( } // ── Legacy path: shared SANDBOX_URL ────────────────────────────── - const sandboxSession = await sandbox.createSessiоn(dashboardId, mcpToken); + // On desktop this is the local VM, which may still be booting on a cold app launch + // (the window loads immediately and fires session-create before the VM is up). Retry + // the connection for ~30s instead of failing the first terminal. + let sandboxSession: { id: string; machineId?: string } | undefined; + let lastCreateErr: unknown; + for (let attempt = 0; attempt < 12; attempt++) { + try { + sandboxSession = await sandbox.createSessiоn(dashboardId, mcpToken); + break; + } catch (createErr) { + lastCreateErr = createErr; + if (attempt < 11) { + console.warn( + `[ensureDashboardSandboxCreate] createSession attempt ${attempt + 1}/12 failed (sandbox may still be booting): ${createErr instanceof Error ? createErr.message : createErr}. Retrying in 3s...` + ); + await new Promise((r) => setTimeout(r, 3000)); + } + } + } + if (!sandboxSession) { + throw lastCreateErr instanceof Error ? lastCreateErr : new Error('Failed to create sandbox session'); + } const insertResult = await env.DB.prepare(` INSERT OR IGNORE INTO dashboard_sandboxes (dashboard_id, sandbox_session_id, sandbox_machine_id, created_at) VALUES (?, ?, ?, ?) @@ -1361,6 +1382,15 @@ async function createSessionImpl( try { const terminalConfig = parseTerminalConfig(item.content); const { bootCommand, workingDir, modelSelection } = terminalConfig; + // Shared-sandbox deployments (desktop) put EVERY dashboard in one VM's /workspace, + // so default each dashboard's terminals into their own subdir (/workspace/) + // for isolation — unless the terminal already set a workingDir (downloaded dashboards + // set their own via cloud-sync). Fly gives each dashboard its own VM, so no default + // there. The desktop sandbox auto-creates the subdir; cloud-dev without the subdir + // just falls back to root (unchanged). + const sharedSandbox = !(env.FLY_PROVISIONING_ENABLED === 'true' + && Boolean(env.FLY_API_TOKEN) && Boolean(env.FLY_APP_NAME)); + const effectiveWorkingDir = workingDir ?? (sharedSandbox ? dashboardId : undefined); let appliedSecretNames: string[] = []; const applyDashboardSecretsBeforePtyStart = async ( @@ -1431,12 +1461,12 @@ async function createSessionImpl( pty = await sandbox.createPty(sandboxSessionId, userId, bootCommand, sandboxMachineId, { ptyId, integrationToken, - workingDir, + workingDir: effectiveWorkingDir, modelSelection, }); } catch (wdErr) { - if (workingDir && wdErr instanceof Error && wdErr.message.includes('E79708')) { - console.log(`[createSession] Working dir "${workingDir}" not found, falling back to workspace root`); + if (effectiveWorkingDir && wdErr instanceof Error && wdErr.message.includes('E79708')) { + console.log(`[createSession] Working dir "${effectiveWorkingDir}" not found, falling back to workspace root`); pty = await sandbox.createPty(sandboxSessionId, userId, bootCommand, sandboxMachineId, { ptyId, integrationToken, @@ -1860,16 +1890,26 @@ export async function getDashbоardSandbоxStatus( } const sandbox = await getDashbоardSandbоx(env, dashboardId); - if (!sandbox?.sandbox_machine_id) { - // No VM (or no machine id yet) — asleep until the first terminal/browser opens. + // "No sandbox yet" keys on the SESSION id, not the machine id: on desktop the local + // VM has no Fly machine, so sandbox_machine_id is empty even when the VM is up. Using + // machine id here made a healthy desktop sandbox show red "Sandbox asleep". + if (!sandbox?.sandbox_session_id) { + // No VM yet — asleep until the first terminal/browser opens. return Response.json({ state: 'asleep', flyState: null, machineId: null }); } const flyProvisioningEnabled = env.FLY_PROVISIONING_ENABLED === 'true' && Boolean(env.FLY_API_TOKEN) && Boolean(env.FLY_APP_NAME); if (!flyProvisioningEnabled) { - // Can't query Fly (e.g. local/desktop). A recorded session implies it's up. - return Response.json({ state: 'ready', flyState: null, machineId: sandbox.sandbox_machine_id }); + // Can't query Fly (e.g. local/desktop). A recorded session means the local VM is + // up (machine id is empty locally), so report ready. + return Response.json({ state: 'ready', flyState: null, machineId: sandbox.sandbox_machine_id || null }); + } + + // Fly path needs a machine id to query; if we somehow have a session without one, + // treat it as asleep so the code below can reprovision. + if (!sandbox.sandbox_machine_id) { + return Response.json({ state: 'asleep', flyState: null, machineId: null }); } const fly = new FlyMachinesClient(env.FLY_APP_NAME!, env.FLY_API_TOKEN!); diff --git a/frontend/src/app/(app)/dashboards/[id]/page.tsx b/frontend/src/app/(app)/dashboards/[id]/page.tsx index 936cb1ab..0b6a7a48 100644 --- a/frontend/src/app/(app)/dashboards/[id]/page.tsx +++ b/frontend/src/app/(app)/dashboards/[id]/page.tsx @@ -3626,32 +3626,34 @@ export default function DashboardPage() { - {/* Presence indicators */} -
- -
- - - {presenceUsers.length} - - {/* Connection status dot */} -
-
- - -
+ {/* Presence indicators — hidden on desktop (always a single local user) */} + {!DESKTOP_MODE && ( +
+ +
+ + + {presenceUsers.length} + + {/* Connection status dot */} +
+
+ + +
+ )} {/* Dev-only sandbox metrics (inline in title bar) */} {process.env.NODE_ENV === "development" && ( diff --git a/sandbox/internal/sessions/session.go b/sandbox/internal/sessions/session.go index 5f757871..d4016e6e 100644 --- a/sandbox/internal/sessions/session.go +++ b/sandbox/internal/sessions/session.go @@ -567,6 +567,18 @@ func (s *Session) resolveWorkingDir(workingDir string) (string, error) { // Verify directory exists info, err := os.Stat(actualWorkDir) if os.IsNotExist(err) { + // Desktop isolates each dashboard into its own /workspace/ subdir + // across the shared VM; auto-create a missing one so the first terminal lands + // there instead of the control plane falling back to the shared root. On cloud + // (each dashboard has its own VM) we keep erroring so a stale mirror dir isn't + // silently recreated. ORCABOT_DEBUG_EXEC is set only by the desktop VM init. + // REVISION: working-dir-v3-desktop-autocreate + if os.Getenv("ORCABOT_DEBUG_EXEC") == "1" { + if mkErr := os.MkdirAll(actualWorkDir, 0o755); mkErr != nil { + return "", fmt.Errorf("failed to create working directory: %w", mkErr) + } + return actualWorkDir, nil + } return "", fmt.Errorf("working directory does not exist: %s", workingDir) } if err != nil { From d8dd89bf4fd327dde0fdf7063be72e2867c84f3d Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Tue, 14 Jul 2026 13:28:30 +0100 Subject: [PATCH 2/7] fix(desktop): isolate workspace at the session root (review round 9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move per-dashboard workspace isolation from a per-PTY workingDir default to the SANDBOX session root, and add a real health check to the desktop status. - [P2] Isolate every dashboard-scoped execution path: manager.Create now roots each desktop session at /workspace/ (ORCABOT_DEBUG_EXEC gate, safe-component checked). Since the whole Session/Workspace is built from that root, UI terminals, chat run_command, schedules, and messaging all inherit it — no per-call plumbing. Reverted the control-plane per-PTY workingDir default (option a). Cloud is unchanged (one VM per dashboard; gate off). - [P2] Fixes the stale-session-retry losing isolation too — isolation no longer depends on the (retry-dropped) workingDir. - [P2] resolveWorkingDir no longer auto-creates arbitrary missing dirs — a typo errors (E79708) again; only the dashboard root is auto-created, in manager.Create. - [P2] Desktop status now probes the local sandbox /health instead of trusting the persisted dashboard_sandboxes row, so a stale row after restart/crash shows "starting" (not green) until the VM actually serves. - cloud-sync: download copies terminal content as-is (no subdir prepend) — the session root provides isolation; the download still writes files to / which equals that root. The session-root change is in the guest sandbox binary, so it needs a VM-image rebuild to take effect on desktop. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ --- controlplane/src/sessions/handler.ts | 33 ++++++++------- frontend/src/lib/cloud-sync.ts | 60 ++++++---------------------- sandbox/internal/sessions/manager.go | 25 +++++++++++- sandbox/internal/sessions/session.go | 17 ++------ 4 files changed, 59 insertions(+), 76 deletions(-) diff --git a/controlplane/src/sessions/handler.ts b/controlplane/src/sessions/handler.ts index 5f8629e0..8216cdae 100644 --- a/controlplane/src/sessions/handler.ts +++ b/controlplane/src/sessions/handler.ts @@ -1382,15 +1382,10 @@ async function createSessionImpl( try { const terminalConfig = parseTerminalConfig(item.content); const { bootCommand, workingDir, modelSelection } = terminalConfig; - // Shared-sandbox deployments (desktop) put EVERY dashboard in one VM's /workspace, - // so default each dashboard's terminals into their own subdir (/workspace/) - // for isolation — unless the terminal already set a workingDir (downloaded dashboards - // set their own via cloud-sync). Fly gives each dashboard its own VM, so no default - // there. The desktop sandbox auto-creates the subdir; cloud-dev without the subdir - // just falls back to root (unchanged). - const sharedSandbox = !(env.FLY_PROVISIONING_ENABLED === 'true' - && Boolean(env.FLY_API_TOKEN) && Boolean(env.FLY_APP_NAME)); - const effectiveWorkingDir = workingDir ?? (sharedSandbox ? dashboardId : undefined); + // NOTE: per-dashboard workspace isolation on desktop is handled in the SANDBOX + // (manager.Create roots each session at /workspace/), so it applies + // uniformly to every dashboard-scoped execution path, not just this UI-terminal + // one. `workingDir` here is only the terminal's own relative sub-path (if any). let appliedSecretNames: string[] = []; const applyDashboardSecretsBeforePtyStart = async ( @@ -1461,12 +1456,12 @@ async function createSessionImpl( pty = await sandbox.createPty(sandboxSessionId, userId, bootCommand, sandboxMachineId, { ptyId, integrationToken, - workingDir: effectiveWorkingDir, + workingDir, modelSelection, }); } catch (wdErr) { - if (effectiveWorkingDir && wdErr instanceof Error && wdErr.message.includes('E79708')) { - console.log(`[createSession] Working dir "${effectiveWorkingDir}" not found, falling back to workspace root`); + if (workingDir && wdErr instanceof Error && wdErr.message.includes('E79708')) { + console.log(`[createSession] Working dir "${workingDir}" not found, falling back to workspace root`); pty = await sandbox.createPty(sandboxSessionId, userId, bootCommand, sandboxMachineId, { ptyId, integrationToken, @@ -1901,9 +1896,17 @@ export async function getDashbоardSandbоxStatus( const flyProvisioningEnabled = env.FLY_PROVISIONING_ENABLED === 'true' && Boolean(env.FLY_API_TOKEN) && Boolean(env.FLY_APP_NAME); if (!flyProvisioningEnabled) { - // Can't query Fly (e.g. local/desktop). A recorded session means the local VM is - // up (machine id is empty locally), so report ready. - return Response.json({ state: 'ready', flyState: null, machineId: sandbox.sandbox_machine_id || null }); + // Can't query Fly (e.g. local/desktop). The dashboard_sandboxes row PERSISTS across + // app restarts / sandbox crashes, but the actual sandbox session is in-memory — so + // trusting the row alone shows green while the VM is still booting or has crashed. + // Probe the local sandbox /health instead; not-yet-serving reads as "starting". + const localSandbox = new SandboxClient(env.SANDBOX_URL, env.SANDBOX_INTERNAL_TOKEN); + const healthy = await localSandbox.health(undefined, 3000); + return Response.json({ + state: healthy ? 'ready' : 'starting', + flyState: null, + machineId: sandbox.sandbox_machine_id || null, + }); } // Fly path needs a machine id to query; if we somehow have a session without one, diff --git a/frontend/src/lib/cloud-sync.ts b/frontend/src/lib/cloud-sync.ts index 96c87652..0e775be6 100644 --- a/frontend/src/lib/cloud-sync.ts +++ b/frontend/src/lib/cloud-sync.ts @@ -1,7 +1,7 @@ // Copyright 2026 Rob Macrae. All rights reserved. // SPDX-License-Identifier: LicenseRef-Proprietary -// REVISION: cloud-sync-v3-pin-legacy-atomic +// REVISION: cloud-sync-v4-session-root-isolation "use client"; import { @@ -19,7 +19,7 @@ import type { Dashboard, DashboardItem, DashboardEdge } from "@/types"; if (typeof window !== "undefined") { console.log( - `[cloud-sync] REVISION: cloud-sync-v3-pin-legacy-atomic loaded at ${new Date().toISOString()}` + `[cloud-sync] REVISION: cloud-sync-v4-session-root-isolation loaded at ${new Date().toISOString()}` ); } @@ -38,47 +38,15 @@ export interface DownloadResult { } /** - * Sanitize a workspace-relative path to plain segments: strip leading slashes and - * `.`/empty segments, and REJECT any `..` (returns "" so the caller falls back to - * the bare subfolder). Without this, a terminal whose content has - * `workingDir: "../other"` would become `/../other` and the sandbox would - * normalize it OUT of the dashboard's isolated subfolder. - */ -function sanitizeRel(p: string): string { - const parts = p - .replace(/^\/+/, "") - .split("/") - .filter((seg) => seg !== "" && seg !== "."); - if (parts.some((seg) => seg === "..")) return ""; - return parts.join("/"); -} - -function pinTerminalToSubdir(content: string, subdir: string): string { - const trimmed = content.trim(); - // Legacy / malformed terminal content is a plain string (the parser treats it as - // the terminal's display name). Wrap it into valid JSON so it, too, is pinned to - // this dashboard's subfolder instead of starting at the shared workspace root. - if (!trimmed.startsWith("{")) { - return JSON.stringify({ name: trimmed || "Terminal", workingDir: subdir }); - } - let obj: Record; - try { - obj = JSON.parse(content) as Record; - } catch { - return JSON.stringify({ name: trimmed, workingDir: subdir }); - } - const existing = - typeof obj.workingDir === "string" ? sanitizeRel(obj.workingDir) : ""; - obj.workingDir = existing ? `${subdir}/${existing}` : subdir; - return JSON.stringify(obj); -} - -/** - * Download a cloud dashboard into the LOCAL control-plane DB and copy its - * workspace files. Fetches the cloud dashboard's structure (items + edges) via the - * native layer (PAT) and recreates it locally with `cloudId` set (so it shows as - * downloaded and Phase-2 sync can map it back), then pulls the cloud workspace - * files into a per-dashboard subfolder. The local copy runs on the local VM. + * Download a cloud dashboard into the LOCAL control-plane DB and copy its workspace + * files. Fetches the cloud dashboard's structure (items + edges) via the native + * layer (PAT) and recreates it locally with `cloudId` set (so it shows as downloaded + * and Phase-2 sync can map it back), then pulls the cloud workspace files into this + * dashboard's subfolder (`/`). The local sandbox roots + * this dashboard's session at that same subfolder, so terminals see the files with + * no per-terminal working-dir rewriting; a cloud terminal's own relative workingDir + * (e.g. "src") stays relative to the dashboard root. The local copy runs on the + * local VM. * * The workspace copy is best-effort: if it fails (cloud VM won't start, no * subscription, timeout), the canvas is still downloaded and `workspaceError` is @@ -98,14 +66,12 @@ export async function downloadCloudDashboard( // dashboard so it doesn't linger marked "downloaded" (with a cloudId) but broken. try { // Recreate items, mapping each cloud item id → the new local id (edges use it). - // Terminal items are pinned to this dashboard's workspace subfolder. + // Content is copied as-is — the sandbox session root provides the isolation. const idMap = new Map(); for (const item of data.items || []) { - const content = - item.type === "terminal" ? pinTerminalToSubdir(item.content, subdir) : item.content; const local = await createItem(dashboard.id, { type: item.type, - content, + content: item.content, position: item.position, size: item.size, metadata: item.metadata, diff --git a/sandbox/internal/sessions/manager.go b/sandbox/internal/sessions/manager.go index f10a9fa9..8e5f10c2 100644 --- a/sandbox/internal/sessions/manager.go +++ b/sandbox/internal/sessions/manager.go @@ -9,6 +9,7 @@ import ( "log" "net/http" "os" + "path/filepath" "strings" "sync" "time" @@ -127,7 +128,22 @@ func (m *Manager) Create(dashboardID string, mcpToken string) (*Session, error) return nil, err } - session := NewSessiоn(sessionID, dashboardID, mcpToken, m.workspaceBase, m.broker, m.brokerPort, m.egressProxyPort) + // Desktop shares ONE VM across ALL dashboards, so root each dashboard's session at + // /workspace/. Because the whole Session/Workspace (PTY cwd, file APIs) + // is built from this root, EVERY dashboard-scoped execution path — UI terminals, + // chat run_command, schedules, messaging — is isolated to it with no per-call + // working-dir plumbing. Cloud has one VM per dashboard (ORCABOT_DEBUG_EXEC unset), + // so it keeps the shared base and is unchanged. + // REVISION: workspace-per-dashboard-v1-session-root + workspaceRoot := m.workspaceBase + if dashboardID != "" && os.Getenv("ORCABOT_DEBUG_EXEC") == "1" && isSafeDirComponent(dashboardID) { + workspaceRoot = filepath.Join(m.workspaceBase, dashboardID) + if err := os.MkdirAll(workspaceRoot, 0755); err != nil { + return nil, err + } + } + + session := NewSessiоn(sessionID, dashboardID, mcpToken, workspaceRoot, m.broker, m.brokerPort, m.egressProxyPort) session.geminiShimPort = m.geminiShimPort m.mu.Lock() @@ -161,6 +177,13 @@ func (m *Manager) Create(dashboardID string, mcpToken string) (*Session, error) return session, nil } +// isSafeDirComponent reports whether s is a single safe path component (no +// separators or traversal) usable as a per-dashboard workspace subdir. +func isSafeDirComponent(s string) bool { + return s != "" && s != "." && s != ".." && + !strings.ContainsAny(s, "/\\") && !strings.Contains(s, "..") +} + // browserPrewarmEnabled reports whether chromium should be pre-warmed at session // creation. Defaults to ON; BROWSER_PREWARM=false/0/no/off disables it. func browserPrewarmEnabled() bool { diff --git a/sandbox/internal/sessions/session.go b/sandbox/internal/sessions/session.go index d4016e6e..3e6c0f3c 100644 --- a/sandbox/internal/sessions/session.go +++ b/sandbox/internal/sessions/session.go @@ -564,21 +564,12 @@ func (s *Session) resolveWorkingDir(workingDir string) (string, error) { return "", fmt.Errorf("invalid working directory: must be relative path within workspace") } actualWorkDir := filepath.Join(s.workspace.Root(), cleaned) - // Verify directory exists + // Verify directory exists. Per-dashboard isolation is handled by rooting the whole + // session at /workspace/ (see manager.Create), so an arbitrary missing + // sub-path here is a genuine bad path (e.g. a typo) and must still error — we do NOT + // auto-create arbitrary user-supplied dirs. info, err := os.Stat(actualWorkDir) if os.IsNotExist(err) { - // Desktop isolates each dashboard into its own /workspace/ subdir - // across the shared VM; auto-create a missing one so the first terminal lands - // there instead of the control plane falling back to the shared root. On cloud - // (each dashboard has its own VM) we keep erroring so a stale mirror dir isn't - // silently recreated. ORCABOT_DEBUG_EXEC is set only by the desktop VM init. - // REVISION: working-dir-v3-desktop-autocreate - if os.Getenv("ORCABOT_DEBUG_EXEC") == "1" { - if mkErr := os.MkdirAll(actualWorkDir, 0o755); mkErr != nil { - return "", fmt.Errorf("failed to create working directory: %w", mkErr) - } - return actualWorkDir, nil - } return "", fmt.Errorf("working directory does not exist: %s", workingDir) } if err != nil { From c5eda6d8bf0f857f2b6b7a50aa7de94c36f1855c Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Tue, 14 Jul 2026 13:46:49 +0100 Subject: [PATCH 3/7] fix(desktop): fix run_command markers + Codex trust for per-dashboard root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The session-root isolation (/workspace/) broke two hardcoded-/workspace assumptions: - [P1] Dashboard chat run_command timed out: the wrapper wrote completion markers to absolute /workspace/.orc-run-* but read them via the session-scoped file API, which now resolves under /workspace/ — marker never found. Now the shell redirect uses the SAME relative names as the read (the run PTY cwds into the session root, and the file API resolves relative to it), so it works whether the root is /workspace (cloud) or /workspace/ (desktop). - [P2] Codex folder-trust pre-trusted the fixed /workspace, so desktop terminals starting in /workspace/ would re-show the "trust this directory?" prompt and block unattended startup. generateCodexHooks now trusts the actual session workspaceRoot it's already given (= s.workspace.Root()). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ --- controlplane/src/chat/handler.ts | 15 ++++++++------- sandbox/internal/agenthooks/hooks.go | 16 ++++++++++------ 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/controlplane/src/chat/handler.ts b/controlplane/src/chat/handler.ts index 1917ecd5..56dd852e 100644 --- a/controlplane/src/chat/handler.ts +++ b/controlplane/src/chat/handler.ts @@ -910,13 +910,14 @@ async function executeTool( const sessionId = sb.sandbox_session_id; const machineId = sb.sandbox_machine_id || undefined; const runId = generateId(); - // Relative names for the file API (it resolves paths under /workspace — an - // absolute /workspace/... would double-nest to /workspace/workspace/...). - // The shell redirects still use the absolute path (correct inside the VM). + // Relative names for BOTH the shell redirect and the file API. The run PTY cwds + // into the session workspace root, and the file API resolves relative to that + // same root — so a relative name lands in one place regardless of whether the + // root is /workspace (cloud) or /workspace/ (desktop's per-dashboard + // session root). An absolute /workspace/... would miss the desktop root and the + // marker would never be found (command times out). const outRel = `.orc-run-${runId}.out`; const exitRel = `.orc-run-${runId}.exit`; - const outPath = `/workspace/${outRel}`; - const exitPath = `/workspace/${exitRel}`; // base64 the command so arbitrary quoting/metachars can't break the wrapper. const bytes = new TextEncoder().encode(command); @@ -925,7 +926,7 @@ async function executeTool( const b64 = btoa(bin); // Run it, capture stdout+stderr to .out, then write the exit code to .exit // last (so the marker never appears before the output is flushed). - const wrapped = `echo ${b64} | base64 -d | bash > ${outPath} 2>&1; echo $? > ${exitPath}`; + const wrapped = `echo ${b64} | base64 -d | bash > ${outRel} 2>&1; echo $? > ${exitRel}`; const client = new SandboxClient(env.SANDBOX_URL, env.SANDBOX_INTERNAL_TOKEN); // rel = path relative to the workspace root (/workspace). @@ -937,7 +938,7 @@ async function executeTool( let pty: { id: string } | null = null; try { pty = await client.createPty(sessionId, '', wrapped, machineId); - console.log(`[run_command] pty created id=${pty?.id} session=${sessionId} machine=${machineId ?? '(none)'} — polling ${exitPath}`); + console.log(`[run_command] pty created id=${pty?.id} session=${sessionId} machine=${machineId ?? '(none)'} — polling ${exitRel}`); // Poll the (internal-token) file API for the exit marker — no new sandbox // endpoint needed. Interval kept at 2s to bound subrequest count. diff --git a/sandbox/internal/agenthooks/hooks.go b/sandbox/internal/agenthooks/hooks.go index 7db6adcf..314c46fe 100644 --- a/sandbox/internal/agenthooks/hooks.go +++ b/sandbox/internal/agenthooks/hooks.go @@ -1125,15 +1125,19 @@ fi existingConfig += "\n# Disable update check — updates managed via Docker image rebuilds\ncheck_for_updates = false\n" } - // REVISION: codex-foldertrust-v1-pretrust - // Pre-trust /workspace so Codex doesn't show "Do you trust this directory?" on every - // reconnection. Trust is stored per-project in config.toml under [projects."/workspace"]. - // Since the sandbox is already an isolated environment, this is safe. - if !strings.Contains(existingConfig, `projects."/workspace"`) { + // REVISION: codex-foldertrust-v2-session-root + // Pre-trust the SESSION workspace root so Codex doesn't show "Do you trust this + // directory?" on every reconnection. On desktop each dashboard's session is rooted + // at /workspace/, so trusting the fixed /workspace no longer covers the + // terminal's cwd and the prompt would reappear (blocking unattended startup). Trust + // is stored per-project in config.toml under [projects.""]. Safe + // because the sandbox is already isolated. + trustKey := fmt.Sprintf(`projects."%s"`, workspaceRoot) + if !strings.Contains(existingConfig, trustKey) { if existingConfig != "" && !strings.HasSuffix(existingConfig, "\n") { existingConfig += "\n" } - existingConfig += "\n# Pre-trust /workspace — sandbox is already isolated\n[projects.\"/workspace\"]\ntrust_level = \"trusted\"\n" + existingConfig += fmt.Sprintf("\n# Pre-trust the session workspace — sandbox is already isolated\n[%s]\ntrust_level = \"trusted\"\n", trustKey) } // Check if our script is already in the config From ce84a23309d63f4e2f895808f309023dd9937b7a Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Tue, 14 Jul 2026 13:55:45 +0100 Subject: [PATCH 4/7] fixes --- desktop/app/src-tauri/Cargo.lock | 2 +- desktop/app/src-tauri/Cargo.toml | 2 +- desktop/app/src-tauri/tauri.conf.json | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/desktop/app/src-tauri/Cargo.lock b/desktop/app/src-tauri/Cargo.lock index b9d0da53..520af626 100644 --- a/desktop/app/src-tauri/Cargo.lock +++ b/desktop/app/src-tauri/Cargo.lock @@ -2305,7 +2305,7 @@ checksum = "04744f49eae99ab78e0d5c0b603ab218f515ea8cfe5a456d7629ad883a3b6e7d" [[package]] name = "orcabot-desktop" -version = "0.5.0" +version = "0.6.0" dependencies = [ "ctrlc", "filetime", diff --git a/desktop/app/src-tauri/Cargo.toml b/desktop/app/src-tauri/Cargo.toml index a5f94716..318e462b 100644 --- a/desktop/app/src-tauri/Cargo.toml +++ b/desktop/app/src-tauri/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "orcabot-desktop" -version = "0.5.0" +version = "0.6.0" edition = "2021" description = "Orcabot Desktop shell" license = "UNLICENSED" diff --git a/desktop/app/src-tauri/tauri.conf.json b/desktop/app/src-tauri/tauri.conf.json index dbc13f45..96494676 100644 --- a/desktop/app/src-tauri/tauri.conf.json +++ b/desktop/app/src-tauri/tauri.conf.json @@ -1,7 +1,7 @@ { "$schema": "https://schema.tauri.app/config/2", "productName": "Orcabot", - "version": "0.5.0", + "version": "0.6.0", "identifier": "com.orcabot.desktop", "build": { "beforeDevCommand": "sh -c \"cd ../../frontend && NEXT_PUBLIC_API_URL=http://localhost:8787 NEXT_PUBLIC_SITE_URL=http://localhost:8788 NEXT_PUBLIC_DEV_MODE_ENABLED=true NEXT_PUBLIC_DESKTOP_MODE=true npx wrangler dev -c wrangler.toml --port 8788\"", From 5f9923c940b2f7e2b085d527b2a4f51915e14169 Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Tue, 14 Jul 2026 14:06:14 +0100 Subject: [PATCH 5/7] fix(desktop): global Codex notify script + bounded boot retry (review round 11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - [P2] Codex completion events broke across dashboards: each dashboard's per-session hook path was appended to the SHARED /etc/codex/config.toml notify array as a second element, so Codex ran the first script with the second path as $1 instead of the event JSON. Now the notify script is a single GLOBAL, workspace-independent file (/etc/codex/orcabot-codex-stop.sh) — it already derives session/pty/secret from per-PTY env vars at runtime — and notify is REPLACED (not appended) with exactly that one path, idempotent across dashboards. - [P2] The "~30s" boot retry could block 3+ minutes (12 × up-to-15s client timeout + 11 × 3s) and retried deterministic 4xx failures. Now bounded by an overall 30s deadline and retries ONLY connection/transient failures ("fetch error:"), throwing immediately on HTTP status errors (e.g. auth mismatch). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ --- controlplane/src/sessions/handler.ts | 22 +++++++---- sandbox/internal/agenthooks/hooks.go | 59 +++++++++++----------------- 2 files changed, 39 insertions(+), 42 deletions(-) diff --git a/controlplane/src/sessions/handler.ts b/controlplane/src/sessions/handler.ts index 8216cdae..e354b9a1 100644 --- a/controlplane/src/sessions/handler.ts +++ b/controlplane/src/sessions/handler.ts @@ -528,21 +528,29 @@ async function ensureDashbоardSandbоxCreate( // ── Legacy path: shared SANDBOX_URL ────────────────────────────── // On desktop this is the local VM, which may still be booting on a cold app launch // (the window loads immediately and fires session-create before the VM is up). Retry - // the connection for ~30s instead of failing the first terminal. + // for up to ~30s — but ONLY connection/transient failures ("fetch error:" from the + // client, i.e. the VM isn't listening yet), never deterministic HTTP errors like a + // 4xx auth mismatch. Bounded by an overall deadline so a hung connection can't stack + // up 12×15s of client timeouts. let sandboxSession: { id: string; machineId?: string } | undefined; let lastCreateErr: unknown; - for (let attempt = 0; attempt < 12; attempt++) { + const retryDeadline = Date.now() + 30_000; + while (Date.now() < retryDeadline) { try { sandboxSession = await sandbox.createSessiоn(dashboardId, mcpToken); break; } catch (createErr) { lastCreateErr = createErr; - if (attempt < 11) { - console.warn( - `[ensureDashboardSandboxCreate] createSession attempt ${attempt + 1}/12 failed (sandbox may still be booting): ${createErr instanceof Error ? createErr.message : createErr}. Retrying in 3s...` - ); - await new Promise((r) => setTimeout(r, 3000)); + const msg = createErr instanceof Error ? createErr.message : String(createErr); + // Only "fetch error:" (connection refused/reset/timeout) is transient; an HTTP + // status error (createSession throws " ") is deterministic. + if (!msg.includes('fetch error:') || Date.now() + 3000 >= retryDeadline) { + throw createErr; } + console.warn( + `[ensureDashboardSandboxCreate] createSession failed (sandbox may still be booting), retrying in 3s: ${msg}` + ); + await new Promise((r) => setTimeout(r, 3000)); } } if (!sandboxSession) { diff --git a/sandbox/internal/agenthooks/hooks.go b/sandbox/internal/agenthooks/hooks.go index 314c46fe..80b2831d 100644 --- a/sandbox/internal/agenthooks/hooks.go +++ b/sandbox/internal/agenthooks/hooks.go @@ -1056,7 +1056,19 @@ fi // generateCodexHooks creates notify hook for Codex CLI func generateCodexHooks(workspaceRoot, hooksDir string) error { - scriptPath := filepath.Join(hooksDir, "codex-stop.sh") + _ = hooksDir // Codex uses ONE global script (below), not the per-session hooks dir. + // One GLOBAL, workspace-independent notify script shared by every dashboard's Codex + // session. Codex's notify lives in the system-wide /etc/codex/config.toml (a single + // notify for ALL projects), so a per-workspace script path gets appended as a second + // array element and corrupts the command — Codex would treat the first script as the + // executable and pass the second path as $1 instead of the event JSON. The script + // derives its session/pty/secret context from per-PTY env vars at runtime, so one + // global copy works for every dashboard. + systemCodexDir := "/etc/codex" + if err := os.MkdirAll(systemCodexDir, 0755); err != nil { + return err + } + scriptPath := filepath.Join(systemCodexDir, "orcabot-codex-stop.sh") // Codex passes JSON as first argument, not stdin // Uses ORCABOT_SESSION_ID, ORCABOT_PTY_ID, and MCP_LOCAL_PORT env vars set by the PTY process script := `#!/bin/bash @@ -1097,15 +1109,9 @@ fi return fmt.Errorf("failed to write codex hook script: %w", err) } - // Write notify to /etc/codex/config.toml (system config). - // Codex CLI overwrites ~/.codex/config.toml on startup, stripping our additions. - // System config is read but not overwritten by Codex. - systemCodexDir := "/etc/codex" - if err := os.MkdirAll(systemCodexDir, 0755); err != nil { - return err - } - fmt.Fprintf(os.Stderr, "[DEBUG] generateCodexHooks: systemCodexDir=%s\n", systemCodexDir) - + // Write notify to /etc/codex/config.toml (system config). Codex CLI overwrites + // ~/.codex/config.toml on startup, stripping our additions; the system config is + // read but not overwritten by Codex. configPath := filepath.Join(systemCodexDir, "config.toml") // Read existing system config or create new @@ -1140,42 +1146,25 @@ fi existingConfig += fmt.Sprintf("\n# Pre-trust the session workspace — sandbox is already isolated\n[%s]\ntrust_level = \"trusted\"\n", trustKey) } - // Check if our script is already in the config - if strings.Contains(existingConfig, scriptPath) { - // Already configured, write config (may have added check_for_updates above) - if err := os.WriteFile(configPath, []byte(existingConfig), 0644); err != nil { - return err - } - return nil - } - - // Check if notify is already set + // Point notify at EXACTLY our one global script — REPLACE any existing notify line + // rather than appending. Codex treats the notify array as command+args, so a second + // element would be passed as $1 instead of the event JSON and break completions. + // Because the script path is fixed and global, this is idempotent across dashboards. + notifyLine := fmt.Sprintf(`notify = ["%s"]`, scriptPath) if strings.Contains(existingConfig, "notify") { - // Parse existing notify array and append our script lines := strings.Split(existingConfig, "\n") for i, line := range lines { - trimmed := strings.TrimSpace(line) - if strings.HasPrefix(trimmed, "notify") { - start := strings.Index(line, "[") - end := strings.LastIndex(line, "]") - if start != -1 && end != -1 && end > start { - existing := strings.TrimSpace(line[start+1 : end]) - if existing == "" { - lines[i] = fmt.Sprintf(`notify = ["%s"]`, scriptPath) - } else { - lines[i] = fmt.Sprintf(`notify = [%s, "%s"]`, existing, scriptPath) - } - } + if strings.HasPrefix(strings.TrimSpace(line), "notify") { + lines[i] = notifyLine break } } existingConfig = strings.Join(lines, "\n") } else { - // Add notify line if existingConfig != "" && !strings.HasSuffix(existingConfig, "\n") { existingConfig += "\n" } - existingConfig += fmt.Sprintf("\n# OrcaBot agent stop notification\nnotify = [\"%s\"]\n", scriptPath) + existingConfig += "\n# OrcaBot agent stop notification\n" + notifyLine + "\n" } fmt.Fprintf(os.Stderr, "[DEBUG] Writing Codex system config to %s:\n%s\n", configPath, existingConfig) From f4966bc36d70e7ae064bd84500537393ec22821f Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Tue, 14 Jul 2026 15:07:04 +0100 Subject: [PATCH 6/7] fix(desktop): write Codex notify as a top-level TOML key (review round 12) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The incremental config assembly appended notify/check_for_updates AFTER the [projects.""] trust tables, so TOML scoped `notify` under a project table instead of top-level — Codex (which reads notify as a top-level array) never saw the global hook. Now rebuild the OrcaBot-managed system config deterministically: top-level keys (check_for_updates, notify) FIRST, then one [projects.""] trust table per accumulated workspace root. Extracted the pure builder (buildCodexSystemConfig) and added a regression test asserting notify precedes any [table], trust accumulates across dashboards, and there's exactly one notify line. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ --- .../internal/agenthooks/codex_config_test.go | 50 +++++++++ sandbox/internal/agenthooks/hooks.go | 101 +++++++++--------- 2 files changed, 99 insertions(+), 52 deletions(-) create mode 100644 sandbox/internal/agenthooks/codex_config_test.go diff --git a/sandbox/internal/agenthooks/codex_config_test.go b/sandbox/internal/agenthooks/codex_config_test.go new file mode 100644 index 00000000..29d5f2c9 --- /dev/null +++ b/sandbox/internal/agenthooks/codex_config_test.go @@ -0,0 +1,50 @@ +// Copyright 2026 Rob Macrae. All rights reserved. +// SPDX-License-Identifier: LicenseRef-Proprietary + +package agenthooks + +import ( + "strings" + "testing" +) + +// Regression guard: `notify` is a TOP-LEVEL Codex key and MUST precede every [table] +// header, and trust must accumulate across dashboards with exactly one notify line. +func TestBuildCodexSystemConfig(t *testing.T) { + const script = "/etc/codex/orcabot-codex-stop.sh" + + cfg := buildCodexSystemConfig("", "/workspace/aaa", script) + nIdx := strings.Index(cfg, "notify = [") + tIdx := strings.Index(cfg, "[projects.") + if nIdx < 0 { + t.Fatalf("notify missing:\n%s", cfg) + } + if tIdx < 0 || nIdx > tIdx { + t.Fatalf("notify must appear before the first [table] header:\n%s", cfg) + } + if !strings.Contains(cfg, `[projects."/workspace/aaa"]`) { + t.Fatalf("first dashboard root not trusted:\n%s", cfg) + } + + // Second dashboard: preserve the first's trust, add its own, keep ONE notify. + cfg2 := buildCodexSystemConfig(cfg, "/workspace/bbb", script) + if got := strings.Count(cfg2, "notify = ["); got != 1 { + t.Fatalf("expected exactly one notify line, got %d:\n%s", got, cfg2) + } + if strings.Count(cfg2, script) != 1 { + t.Fatalf("notify should reference the single global script once:\n%s", cfg2) + } + for _, root := range []string{`[projects."/workspace/aaa"]`, `[projects."/workspace/bbb"]`} { + if !strings.Contains(cfg2, root) { + t.Fatalf("missing trust table %s:\n%s", root, cfg2) + } + } + if strings.Index(cfg2, "notify = [") > strings.Index(cfg2, "[projects.") { + t.Fatalf("notify must stay top-level after accumulation:\n%s", cfg2) + } + // Idempotent: re-running with an already-trusted root doesn't duplicate it. + cfg3 := buildCodexSystemConfig(cfg2, "/workspace/aaa", script) + if got := strings.Count(cfg3, `[projects."/workspace/aaa"]`); got != 1 { + t.Fatalf("trust table for /workspace/aaa duplicated (%d):\n%s", got, cfg3) + } +} diff --git a/sandbox/internal/agenthooks/hooks.go b/sandbox/internal/agenthooks/hooks.go index 80b2831d..28f0c661 100644 --- a/sandbox/internal/agenthooks/hooks.go +++ b/sandbox/internal/agenthooks/hooks.go @@ -10,6 +10,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "sync" @@ -1114,64 +1115,60 @@ fi // read but not overwritten by Codex. configPath := filepath.Join(systemCodexDir, "config.toml") - // Read existing system config or create new - var existingConfig string - data, err := os.ReadFile(configPath) - if err == nil { + // Read the existing system config (owned by us — Codex only READS it, never writes + // it), then REBUILD it deterministically. Incrementally appending broke TOML scoping: + // `notify`/`check_for_updates` are TOP-LEVEL keys and MUST precede every [table] + // header, but they were being appended after the [projects.""] trust tables, so + // TOML scoped `notify` under a project and Codex never saw the global hook. + // REVISION: codex-config-v3-rebuild-toplevel + existingConfig := "" + if data, readErr := os.ReadFile(configPath); readErr == nil { existingConfig = string(data) } + finalConfig := buildCodexSystemConfig(existingConfig, workspaceRoot, scriptPath) - // REVISION: codex-autoupdate-v1-disable - // Disable Codex CLI's update check. It prompts interactively on startup - // which blocks non-interactive use in the sandbox. - if !strings.Contains(existingConfig, "check_for_updates") { - if existingConfig != "" && !strings.HasSuffix(existingConfig, "\n") { - existingConfig += "\n" - } - existingConfig += "\n# Disable update check — updates managed via Docker image rebuilds\ncheck_for_updates = false\n" - } - - // REVISION: codex-foldertrust-v2-session-root - // Pre-trust the SESSION workspace root so Codex doesn't show "Do you trust this - // directory?" on every reconnection. On desktop each dashboard's session is rooted - // at /workspace/, so trusting the fixed /workspace no longer covers the - // terminal's cwd and the prompt would reappear (blocking unattended startup). Trust - // is stored per-project in config.toml under [projects.""]. Safe - // because the sandbox is already isolated. - trustKey := fmt.Sprintf(`projects."%s"`, workspaceRoot) - if !strings.Contains(existingConfig, trustKey) { - if existingConfig != "" && !strings.HasSuffix(existingConfig, "\n") { - existingConfig += "\n" - } - existingConfig += fmt.Sprintf("\n# Pre-trust the session workspace — sandbox is already isolated\n[%s]\ntrust_level = \"trusted\"\n", trustKey) - } - - // Point notify at EXACTLY our one global script — REPLACE any existing notify line - // rather than appending. Codex treats the notify array as command+args, so a second - // element would be passed as $1 instead of the event JSON and break completions. - // Because the script path is fixed and global, this is idempotent across dashboards. - notifyLine := fmt.Sprintf(`notify = ["%s"]`, scriptPath) - if strings.Contains(existingConfig, "notify") { - lines := strings.Split(existingConfig, "\n") - for i, line := range lines { - if strings.HasPrefix(strings.TrimSpace(line), "notify") { - lines[i] = notifyLine - break - } - } - existingConfig = strings.Join(lines, "\n") - } else { - if existingConfig != "" && !strings.HasSuffix(existingConfig, "\n") { - existingConfig += "\n" - } - existingConfig += "\n# OrcaBot agent stop notification\n" + notifyLine + "\n" - } - - fmt.Fprintf(os.Stderr, "[DEBUG] Writing Codex system config to %s:\n%s\n", configPath, existingConfig) - if err := os.WriteFile(configPath, []byte(existingConfig), 0644); err != nil { + fmt.Fprintf(os.Stderr, "[DEBUG] Writing Codex system config to %s:\n%s\n", configPath, finalConfig) + if err := os.WriteFile(configPath, []byte(finalConfig), 0644); err != nil { fmt.Fprintf(os.Stderr, "[DEBUG] Failed to write Codex system config: %v\n", err) return err } fmt.Fprintf(os.Stderr, "[DEBUG] Successfully wrote Codex hooks to %s\n", configPath) return nil } + +// buildCodexSystemConfig regenerates the OrcaBot-managed Codex system config as valid +// TOML: top-level keys (check_for_updates, notify) FIRST — they must precede every +// [table] header or TOML scopes them under it — then one [projects.""] trust +// table per workspace root. Existing trusted roots are preserved and workspaceRoot is +// added, so trust accumulates across dashboards while notify stays a single top-level +// command. Pure (no filesystem) so it can be unit-tested. +func buildCodexSystemConfig(existingConfig, workspaceRoot, scriptPath string) string { + trustedRoots := map[string]bool{workspaceRoot: true} + for _, line := range strings.Split(existingConfig, "\n") { + t := strings.TrimSpace(line) + if strings.HasPrefix(t, `[projects."`) && strings.HasSuffix(t, `"]`) { + if root := t[len(`[projects."`) : len(t)-len(`"]`)]; root != "" { + trustedRoots[root] = true + } + } + } + roots := make([]string, 0, len(trustedRoots)) + for r := range trustedRoots { + roots = append(roots, r) + } + sort.Strings(roots) // stable, deterministic output + + var b strings.Builder + b.WriteString("# OrcaBot-managed Codex system config — regenerated per session.\n") + b.WriteString("# Update check disabled — updates ship via image rebuilds.\n") + b.WriteString("check_for_updates = false\n") + // Agent stop notification → one global script; it derives session/pty/secret from + // per-PTY env vars at runtime, so a single top-level notify works for all dashboards. + b.WriteString(fmt.Sprintf("notify = [\"%s\"]\n", scriptPath)) + // Per-project trust tables LAST. Pre-trust each dashboard's session workspace root + // so Codex doesn't prompt (the sandbox is already isolated). + for _, r := range roots { + b.WriteString(fmt.Sprintf("\n[projects.%q]\ntrust_level = \"trusted\"\n", r)) + } + return b.String() +} From d833f89244ba29f354dcf1c5254cf66aca242260 Mon Sep 17 00:00:00 2001 From: Rob Macrae Date: Tue, 14 Jul 2026 15:18:38 +0100 Subject: [PATCH 7/7] fix(desktop): preserve Codex MCP sections when regenerating hooks (review round 13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The config rebuild wrote only check_for_updates/notify/trust, wiping the [mcp_servers.*] sections that mcp.GenerateSettingsForAgent had just written to the same /etc/codex/config.toml — so every Codex launch lost Orcabot + user MCP tools (hook generation runs after MCP generation). buildCodexSystemConfig is now section-aware: it rewrites only the managed top-level keys (check_for_updates, notify — kept before every [table]) and the [projects.""] trust tables, and PRESERVES all other sections verbatim (notably [mcp_servers.*]), stripping only stray managed keys a prior bug nested inside tables. Regression test extended to assert an [mcp_servers.*] section survives regeneration (and stays after notify). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_014t8Ukp4NPWtqJ55X261wMZ --- .../internal/agenthooks/codex_config_test.go | 76 ++++++++++------ sandbox/internal/agenthooks/hooks.go | 87 +++++++++++++++---- 2 files changed, 117 insertions(+), 46 deletions(-) diff --git a/sandbox/internal/agenthooks/codex_config_test.go b/sandbox/internal/agenthooks/codex_config_test.go index 29d5f2c9..83d4cd0f 100644 --- a/sandbox/internal/agenthooks/codex_config_test.go +++ b/sandbox/internal/agenthooks/codex_config_test.go @@ -8,43 +8,65 @@ import ( "testing" ) -// Regression guard: `notify` is a TOP-LEVEL Codex key and MUST precede every [table] -// header, and trust must accumulate across dashboards with exactly one notify line. +// Regression guard for the OrcaBot-managed Codex system config: +// - `notify` is a TOP-LEVEL key and MUST precede every [table] header +// - existing [mcp_servers.*] (written by GenerateSettingsForAgent) is PRESERVED +// - per-dashboard trust accumulates, with exactly one notify line func TestBuildCodexSystemConfig(t *testing.T) { const script = "/etc/codex/orcabot-codex-stop.sh" - cfg := buildCodexSystemConfig("", "/workspace/aaa", script) - nIdx := strings.Index(cfg, "notify = [") - tIdx := strings.Index(cfg, "[projects.") - if nIdx < 0 { - t.Fatalf("notify missing:\n%s", cfg) - } - if tIdx < 0 || nIdx > tIdx { - t.Fatalf("notify must appear before the first [table] header:\n%s", cfg) + // A config as GenerateSettingsForAgent leaves it: an MCP server table (+ a stray + // notify that a prior bug nested inside a project table, which must be cleaned up). + existing := `check_for_updates = false +notify = ["/old/script"] + +[mcp_servers.orcabot] +command = "/opt/orcabot/mcp-bridge" +args = ["--session", "s1"] +env = { ORCABOT_MCP_SECRET = "x" } + +[projects."/workspace/aaa"] +trust_level = "trusted" +notify = ["/stray/should/be/removed"] +` + + cfg := buildCodexSystemConfig(existing, "/workspace/bbb", script) + + // notify present, single, and references only the global script. + if got := strings.Count(cfg, "notify = ["); got != 1 { + t.Fatalf("expected exactly one notify line, got %d:\n%s", got, cfg) } - if !strings.Contains(cfg, `[projects."/workspace/aaa"]`) { - t.Fatalf("first dashboard root not trusted:\n%s", cfg) + if !strings.Contains(cfg, script) || strings.Contains(cfg, "/old/script") || strings.Contains(cfg, "/stray/should/be/removed") { + t.Fatalf("notify must be the single global script, no strays:\n%s", cfg) } - - // Second dashboard: preserve the first's trust, add its own, keep ONE notify. - cfg2 := buildCodexSystemConfig(cfg, "/workspace/bbb", script) - if got := strings.Count(cfg2, "notify = ["); got != 1 { - t.Fatalf("expected exactly one notify line, got %d:\n%s", got, cfg2) + // notify is top-level: before EVERY table header (mcp_servers and projects). + nIdx := strings.Index(cfg, "notify = [") + firstTable := strings.Index(cfg, "[") + if nIdx < 0 || firstTable < 0 || nIdx > firstTable { + t.Fatalf("notify must precede the first [table]:\n%s", cfg) } - if strings.Count(cfg2, script) != 1 { - t.Fatalf("notify should reference the single global script once:\n%s", cfg2) + // MCP server section preserved verbatim. + if !strings.Contains(cfg, "[mcp_servers.orcabot]") || + !strings.Contains(cfg, `command = "/opt/orcabot/mcp-bridge"`) || + !strings.Contains(cfg, `ORCABOT_MCP_SECRET = "x"`) { + t.Fatalf("mcp_servers section must be preserved:\n%s", cfg) } + // Both dashboard roots trusted; the stray nested notify is gone. for _, root := range []string{`[projects."/workspace/aaa"]`, `[projects."/workspace/bbb"]`} { - if !strings.Contains(cfg2, root) { - t.Fatalf("missing trust table %s:\n%s", root, cfg2) + if !strings.Contains(cfg, root) { + t.Fatalf("missing trust table %s:\n%s", root, cfg) } } - if strings.Index(cfg2, "notify = [") > strings.Index(cfg2, "[projects.") { - t.Fatalf("notify must stay top-level after accumulation:\n%s", cfg2) + + // Idempotent + still preserves MCP when re-fed its own output for an existing root. + cfg2 := buildCodexSystemConfig(cfg, "/workspace/aaa", script) + if strings.Count(cfg2, `[projects."/workspace/aaa"]`) != 1 { + t.Fatalf("trust table /workspace/aaa duplicated:\n%s", cfg2) + } + if !strings.Contains(cfg2, "[mcp_servers.orcabot]") { + t.Fatalf("mcp_servers lost on regeneration:\n%s", cfg2) } - // Idempotent: re-running with an already-trusted root doesn't duplicate it. - cfg3 := buildCodexSystemConfig(cfg2, "/workspace/aaa", script) - if got := strings.Count(cfg3, `[projects."/workspace/aaa"]`); got != 1 { - t.Fatalf("trust table for /workspace/aaa duplicated (%d):\n%s", got, cfg3) + if strings.Count(cfg2, "notify = [") != 1 { + t.Fatalf("notify duplicated on regeneration:\n%s", cfg2) } } diff --git a/sandbox/internal/agenthooks/hooks.go b/sandbox/internal/agenthooks/hooks.go index 28f0c661..cc45ef23 100644 --- a/sandbox/internal/agenthooks/hooks.go +++ b/sandbox/internal/agenthooks/hooks.go @@ -10,7 +10,6 @@ import ( "fmt" "os" "path/filepath" - "sort" "strings" "sync" @@ -1143,32 +1142,82 @@ fi // added, so trust accumulates across dashboards while notify stays a single top-level // command. Pure (no filesystem) so it can be unit-tested. func buildCodexSystemConfig(existingConfig, workspaceRoot, scriptPath string) string { - trustedRoots := map[string]bool{workspaceRoot: true} + // Parse the existing config into a top-level preamble (before the first [header]) and + // its [table] sections. We MANAGE only two things: the top-level check_for_updates / + // notify keys, and the [projects.""] trust tables. EVERYTHING else — notably + // [mcp_servers.*] written by mcp.GenerateSettingsForAgent into this same file — is + // preserved verbatim, so regenerating hooks doesn't clobber MCP tools. + var preamble []string + type section struct { + header string + body []string + } + var sections []section for _, line := range strings.Split(existingConfig, "\n") { - t := strings.TrimSpace(line) - if strings.HasPrefix(t, `[projects."`) && strings.HasSuffix(t, `"]`) { - if root := t[len(`[projects."`) : len(t)-len(`"]`)]; root != "" { - trustedRoots[root] = true - } + if strings.HasPrefix(strings.TrimSpace(line), "[") { + sections = append(sections, section{header: strings.TrimSpace(line)}) + } else if len(sections) == 0 { + preamble = append(preamble, line) + } else { + sections[len(sections)-1].body = append(sections[len(sections)-1].body, line) } } - roots := make([]string, 0, len(trustedRoots)) - for r := range trustedRoots { - roots = append(roots, r) + + isManaged := func(line string) bool { + t := strings.TrimSpace(line) + return strings.HasPrefix(t, "check_for_updates") || strings.HasPrefix(t, "notify") + } + isTrustTable := func(header string) bool { + return strings.HasPrefix(header, `[projects."`) && strings.HasSuffix(header, `"]`) } - sort.Strings(roots) // stable, deterministic output var b strings.Builder - b.WriteString("# OrcaBot-managed Codex system config — regenerated per session.\n") - b.WriteString("# Update check disabled — updates ship via image rebuilds.\n") + // (1) Managed top-level keys FIRST — they MUST precede any [table] or TOML scopes + // them under it (that was the earlier notify-under-[projects] bug). + b.WriteString("# OrcaBot-managed Codex system config.\n") b.WriteString("check_for_updates = false\n") - // Agent stop notification → one global script; it derives session/pty/secret from - // per-PTY env vars at runtime, so a single top-level notify works for all dashboards. + // One global notify script; it derives session/pty/secret from per-PTY env at runtime. b.WriteString(fmt.Sprintf("notify = [\"%s\"]\n", scriptPath)) - // Per-project trust tables LAST. Pre-trust each dashboard's session workspace root - // so Codex doesn't prompt (the sandbox is already isolated). - for _, r := range roots { - b.WriteString(fmt.Sprintf("\n[projects.%q]\ntrust_level = \"trusted\"\n", r)) + // (2) Preserve any OTHER top-level keys a tool set (drop our managed ones + our own + // comment lines so they don't accumulate across regenerations). + for _, line := range preamble { + t := strings.TrimSpace(line) + if isManaged(line) || t == "" || strings.HasPrefix(t, "#") { + continue + } + b.WriteString(line + "\n") + } + + // (3) Re-emit preserved sections (e.g. [mcp_servers.*]); normalize [projects] trust + // tables and note whether this session's root is already present. + trustHeader := fmt.Sprintf(`[projects.%q]`, workspaceRoot) + haveCurrentTrust := false + for _, s := range sections { + if isTrustTable(s.header) { + if s.header == trustHeader { + haveCurrentTrust = true + } + b.WriteString("\n" + s.header + "\ntrust_level = \"trusted\"\n") + continue + } + // Non-managed section — preserve verbatim, minus any stray managed key a prior + // bug may have nested inside it, and minus trailing blank lines (idempotency). + kept := make([]string, 0, len(s.body)) + for _, line := range s.body { + if isManaged(line) { + continue + } + kept = append(kept, line) + } + body := strings.TrimRight(strings.Join(kept, "\n"), "\n") + b.WriteString("\n" + s.header + "\n") + if body != "" { + b.WriteString(body + "\n") + } + } + // (4) Ensure this dashboard's session root is trusted. + if !haveCurrentTrust { + b.WriteString("\n" + trustHeader + "\ntrust_level = \"trusted\"\n") } return b.String() }