diff --git a/services/runner/src/engines/sandbox_agent/environment-setup.ts b/services/runner/src/engines/sandbox_agent/environment-setup.ts index 0737492ebd..c03ce7768d 100644 --- a/services/runner/src/engines/sandbox_agent/environment-setup.ts +++ b/services/runner/src/engines/sandbox_agent/environment-setup.ts @@ -50,6 +50,7 @@ import { resolvesToLocalProvider, } from "./session-identity.ts"; import { loadRunnerConfig } from "../../config/runner-config.ts"; +import { createTimingLog } from "../../environment/timing.ts"; function defaultLog(message: string): void { process.stderr.write(`[sandbox-agent] ${message}\n`); @@ -62,14 +63,13 @@ export async function prepareEnvironmentSetup( ) { const logger = deps.log ?? defaultLog; const acquireStartedAt = Date.now(); - const timingLog = (stage: string, startedAt: number, fields = ""): void => { - const sandboxId = environment?.sandbox?.sandboxId ?? "-"; - const sessionId = - environment?.sessionId ?? request.sessionId?.trim() ?? "-"; - logger( - `[timing] stage=${stage} ms=${Math.round(Date.now() - startedAt)} sandbox=${sandboxId} session=${sessionId}${fields}`, - ); - }; + // The stage names are matched by dashboards; see `environment/timing.ts`. The accessors are + // read at call time on purpose: the sandbox does not exist yet, and the session id changes + // during acquire, so capturing either by value would log a stale `-`. + const timingLog = createTimingLog(logger, { + sandboxId: () => environment?.sandbox?.sandboxId, + sessionId: () => environment?.sessionId ?? request.sessionId?.trim(), + }); // Local multi-runner fails loudly. Session-owned + local-sandbox only (a non-session run // has no cross-replica identity to protect, and a remote sandbox has no runner-local pooled diff --git a/services/runner/src/engines/sandbox_agent/environment.ts b/services/runner/src/engines/sandbox_agent/environment.ts index ade66b9cdf..6cf072e789 100644 --- a/services/runner/src/engines/sandbox_agent/environment.ts +++ b/services/runner/src/engines/sandbox_agent/environment.ts @@ -129,6 +129,14 @@ import { } from "./session-continuity.ts"; import { mountExpiryMs, projectScopeFor } from "./session-identity.ts"; import { teardownDisposition, type TeardownReason } from "./teardown.ts"; +import { + cleanup as cleanupWorkspace, + materialize as materializeWorkspace, +} from "../../environment/workspace-manager.ts"; +import { + acquire as acquireSandbox, + teardown as teardownSandbox, +} from "../../environment/sandbox-lifecycle.ts"; import { uploadToolMcpAssets, type ToolMcpAssets } from "./tool-mcp-assets.ts"; import { prepareWorkspace } from "./workspace.ts"; import { prepareEnvironmentSetup } from "./environment-setup.ts"; @@ -307,34 +315,16 @@ export async function acquireEnvironment( await environment.sandbox ?.destroySession?.(environment.session.id) .catch(() => {}); - const disposition = teardownDisposition(opts?.reason ?? "failed-turn"); - let parked = false; - if ( - disposition === "stop" && - plan.isDaytona && - environment.sandbox?.pauseSandbox - ) { - const sandboxLogId = environment.sandbox.sandboxId ?? plan.sandboxId; - try { - await environment.sandbox.pauseSandbox(); - parked = true; - logger(`parked sandbox=${sandboxLogId}`); - } catch (err) { - logger( - `pause failed sandbox=${sandboxLogId}: ${conciseError(err, plan.harness)}`, - ); - } - } - if (!parked) { - // Record the id BEFORE the delete call, and record it even when the call throws. A delete - // that failed may still have removed the sandbox, so reconnecting to it is a wasted round - // trip either way. See `markSandboxDestroyed`. - markSandboxDestroyed( - environment.sandbox?.sandboxId ?? plan.sandboxId ?? undefined, - ); - await environment.sandbox?.destroySandbox().catch(() => {}); - } - await environment.sandbox?.dispose().catch(() => {}); + // SandboxLifecycle owns park-versus-delete. It returns `parked` because the mount teardown + // below is gated on it: a parked Daytona sandbox keeps its agent mount. + const { parked } = await teardownSandbox({ + sandbox: environment.sandbox, + plannedSandboxId: plan.sandboxId, + isDaytona: plan.isDaytona, + harness: plan.harness, + reason: opts?.reason, + log: logger, + }); // Unmount the durable cwd BEFORE removing the dir: data lives in the store, only the host // mountpoint is torn down. If unmount is not CONFIRMED gone, skip the delete: rmSync must // never run against a possibly-live FUSE mount into the durable store. @@ -365,7 +355,7 @@ export async function acquireEnvironment( `durable cwd unmount not confirmed, skipping workspace cleanup cwd=${plan.workspace.cwd}`, ); } else { - await environment.workspace?.cleanup().catch(() => {}); + await cleanupWorkspace(environment.workspace); } // The per-run Agenta agent dir (skills isolation) is throwaway; remove it too. This is only // ever a temp dir: a subscription run leaves `runAgentDir` undefined precisely so that the @@ -667,52 +657,29 @@ export async function acquireEnvironment( ? (deps.createCookieFetch ?? createCookieFetch)() : (deps.createAcpFetch ?? createAcpFetch)(), }; - // A stored sandbox id is trusted: reconnect it by id and let reconnect converge its network - // policy to this run's plan. Any reconnect failure falls through to a fresh create. Snapshot - // and image drift are accepted as per-conversation version pinning, not grounds for a rebuild. - const storedSandboxPointer = - plan.isDaytona && sessionForMount && runCred - ? await (deps.readStoredSandboxPointer ?? readStoredSandboxPointer)( - sessionForMount, - { authorization: runCred, log: logger }, - ) - : undefined; - if (storedSandboxPointer) { - const sandboxStartStartedAt = Date.now(); - try { - environment.sandbox = await startSandboxAgent({ - ...startOptions, - sandboxId: storedSandboxPointer.sandboxId, - }); - logger( - `reconnected sandbox=${storedSandboxPointer.sandboxId} session=${sessionForMount}`, - ); - } catch (err) { - logger( - `reconnect failed sandbox=${storedSandboxPointer.sandboxId}, creating fresh: ${conciseError(err, plan.harness)}`, - ); - // No explicit pointer clear needed: turns are append-only, so the fresh sandbox this - // turn creates below gets its own turn row at completion, and that row's higher - // turn_index naturally supersedes the dead one on the next `latest_turn` read — the - // staleness guard the old states model needed dissolves with the ordering. - if (err instanceof DaytonaReconnectTerminalError) { - logger( - `terminal Daytona state '${err.state}' for sandbox=${storedSandboxPointer.sandboxId}, not retrying reconnect`, - ); - } - } finally { - timingLog("sandbox_start", sandboxStartStartedAt, " mode=reconnect"); - } - } - if (!environment.sandbox) { - const sandboxStartStartedAt = Date.now(); - try { - environment.sandbox = await startSandboxAgent(startOptions); - } finally { - timingLog("sandbox_start", sandboxStartStartedAt, " mode=create"); - } - } - environment.resumable = Boolean(plan.isDaytona && sessionForMount); + // SandboxLifecycle owns the reconnect ladder, the fresh-create fallback, and both + // `sandbox_start` timing marks. See `environment/sandbox-lifecycle.ts`. + const acquiredSandbox = await acquireSandbox( + { + startOptions, + isDaytona: plan.isDaytona, + harness: plan.harness, + sessionForMount, + runCred, + log: logger, + timingLog, + }, + { + startSandboxAgent: startSandboxAgent as unknown as ( + options: Record, + ) => Promise, + ...(deps.readStoredSandboxPointer + ? { readStoredSandboxPointer: deps.readStoredSandboxPointer } + : {}), + }, + ); + environment.sandbox = acquiredSandbox.sandbox; + environment.resumable = acquiredSandbox.resumable; // Track the live handle so a shutdown signal handler can delete it if `destroy` is skipped by // a process KILL; removed in `destroy` on every normal exit so it is never double-deleted. if (environment.sandbox) inFlightSandboxes.add(environment); @@ -880,15 +847,16 @@ export async function acquireEnvironment( } const prepareWorkspaceStartedAt = Date.now(); + // WorkspaceManager owns the write; the retry stays here because it re-signs a MOUNT, which is + // the mount unit's concern, not the workspace's. + const workspaceInput = { + sandbox: environment.sandbox, + plan, + piSkillSnapshot, + log: logger, + }; try { - environment.workspace = await (deps.prepareWorkspace ?? prepareWorkspace)( - { - sandbox: environment.sandbox, - plan, - piSkillSnapshot, - log: logger, - }, - ); + environment.workspace = await materializeWorkspace(workspaceInput, deps); } catch (err) { if ( !plan.isDaytona && @@ -899,14 +867,10 @@ export async function acquireEnvironment( logger( `retrying workspace preparation after local durable cwd remount`, ); - environment.workspace = await ( - deps.prepareWorkspace ?? prepareWorkspace - )({ - sandbox: environment.sandbox, - plan, - piSkillSnapshot, - log: logger, - }); + environment.workspace = await materializeWorkspace( + workspaceInput, + deps, + ); } else { throw err; } diff --git a/services/runner/src/environment/sandbox-lifecycle.ts b/services/runner/src/environment/sandbox-lifecycle.ts new file mode 100644 index 0000000000..9aeb8feef5 --- /dev/null +++ b/services/runner/src/environment/sandbox-lifecycle.ts @@ -0,0 +1,180 @@ +/** + * `SandboxLifecycle` — the provider instance. + * + * LIFECYCLE MIGRATION, STEP 5. This unit owns the `sandbox_start` acquire stage and the sandbox + * half of teardown. It is a pure code move: the reconnect ladder, the fresh-create fallback, the + * park-versus-delete decision, and the in-flight registry all behave exactly as they did inline. + * + * TWO EVENTS, ONE STAGE NAME. `acquire` may reconnect a parked sandbox or create a fresh one. Both + * emit `sandbox_start`, and the mode rides the ` mode=...` field. A dashboard grouping by stage + * therefore sees one series with a mode dimension, which is what the existing queries expect. + * + * THE RECONNECT LADDER NEVER FAILS A TURN. A stored id that will not reconnect degrades to a fresh + * create. That is why the reconnect `catch` swallows: a dead sandbox is an ordinary outcome, not + * an error, and the only cost is the round trip. + */ +import { conciseError } from "../engines/sandbox_agent/errors.ts"; +import { DaytonaReconnectTerminalError } from "../engines/sandbox_agent/daytona-provider.ts"; +import { + markSandboxDestroyed, + readStoredSandboxPointer, +} from "../engines/sandbox_agent/sandbox-reconnect.ts"; +import { + teardownDisposition, + type TeardownReason, +} from "../engines/sandbox_agent/teardown.ts"; +import type { Log, TimingLog } from "./timing.ts"; + +/** What `acquire` needs. Deliberately narrow: this unit never sees credentials or a workspace. */ +export interface SandboxAcquireInput { + /** Provider-agnostic start options, already built by the composer. */ + startOptions: Record; + isDaytona: boolean; + harness: string; + /** The session whose stored pointer may name a parked sandbox. Undefined disables reconnect. */ + sessionForMount: string | undefined; + /** The run credential the pointer read needs. Undefined disables reconnect. */ + runCred: string | undefined; + log: Log; + timingLog: TimingLog; +} + +export interface SandboxAcquireDeps { + startSandboxAgent: (options: Record) => Promise; + readStoredSandboxPointer?: typeof readStoredSandboxPointer; +} + +export interface SandboxAcquireResult { + sandbox: unknown; + /** True when this sandbox may be parked and reconnected on a later turn. */ + resumable: boolean; + /** Which path produced the handle. Reported for the composer's logs and for tests. */ + mode: "reconnect" | "create"; +} + +/** + * Get a sandbox: reconnect a parked one when a pointer names it, otherwise create a fresh one. + * + * Byte-for-byte the inline behavior, including the swallowed reconnect failure and the extra log + * line for a confirmed terminal Daytona state. + */ +export async function acquire( + input: SandboxAcquireInput, + deps: SandboxAcquireDeps, +): Promise { + const { isDaytona, sessionForMount, runCred, log, timingLog } = input; + + // A stored sandbox id is trusted: reconnect it by id and let reconnect converge its network + // policy to this run's plan. Any reconnect failure falls through to a fresh create. Snapshot + // and image drift are accepted as per-conversation version pinning, not grounds for a rebuild. + const storedSandboxPointer = + isDaytona && sessionForMount && runCred + ? await (deps.readStoredSandboxPointer ?? readStoredSandboxPointer)( + sessionForMount, + { authorization: runCred, log }, + ) + : undefined; + + let sandbox: unknown; + let mode: "reconnect" | "create" = "create"; + + if (storedSandboxPointer) { + const sandboxStartStartedAt = Date.now(); + try { + sandbox = await deps.startSandboxAgent({ + ...input.startOptions, + sandboxId: storedSandboxPointer.sandboxId, + }); + mode = "reconnect"; + log( + `reconnected sandbox=${storedSandboxPointer.sandboxId} session=${sessionForMount}`, + ); + } catch (err) { + log( + `reconnect failed sandbox=${storedSandboxPointer.sandboxId}, creating fresh: ${conciseError(err, input.harness)}`, + ); + // No explicit pointer clear needed: turns are append-only, so the fresh sandbox this + // turn creates below gets its own turn row at completion, and that row's higher + // turn_index naturally supersedes the dead one on the next `latest_turn` read. + if (err instanceof DaytonaReconnectTerminalError) { + log( + `terminal Daytona state '${err.state}' for sandbox=${storedSandboxPointer.sandboxId}, not retrying reconnect`, + ); + } + } finally { + timingLog("sandbox_start", sandboxStartStartedAt, " mode=reconnect"); + } + } + + if (!sandbox) { + const sandboxStartStartedAt = Date.now(); + mode = "create"; + try { + sandbox = await deps.startSandboxAgent(input.startOptions); + } finally { + timingLog("sandbox_start", sandboxStartStartedAt, " mode=create"); + } + } + + return { + sandbox, + resumable: Boolean(isDaytona && sessionForMount), + mode, + }; +} + +export interface SandboxTeardownInput { + sandbox: { + sandboxId?: string; + pauseSandbox?: () => Promise; + destroySandbox?: () => Promise; + dispose?: () => Promise; + } | undefined; + /** The plan's id, used when the live handle carries none. */ + plannedSandboxId: string | undefined; + isDaytona: boolean; + harness: string; + reason: TeardownReason | undefined; + log: Log; +} + +/** + * Stop or delete the sandbox, and say which happened. + * + * `parked` is returned because the caller needs it: a parked Daytona sandbox keeps its agent + * mount, so the mount unit's teardown is gated on this answer. That coupling is why the composer + * still sequences the units rather than each unit tearing itself down independently. + * + * Never throws. Teardown must always complete. + */ +export async function teardown( + input: SandboxTeardownInput, +): Promise<{ parked: boolean }> { + const { sandbox, log } = input; + const disposition = teardownDisposition(input.reason ?? "failed-turn"); + let parked = false; + + if (disposition === "stop" && input.isDaytona && sandbox?.pauseSandbox) { + const sandboxLogId = sandbox.sandboxId ?? input.plannedSandboxId; + try { + await sandbox.pauseSandbox(); + parked = true; + log(`parked sandbox=${sandboxLogId}`); + } catch (err) { + log( + `pause failed sandbox=${sandboxLogId}: ${conciseError(err, input.harness)}`, + ); + } + } + + if (!parked) { + // Record the id BEFORE the delete call, and record it even when the call throws. A delete + // that failed may still have removed the sandbox, so reconnecting to it is a wasted round + // trip either way. See `markSandboxDestroyed`. + markSandboxDestroyed(sandbox?.sandboxId ?? input.plannedSandboxId ?? undefined); + await sandbox?.destroySandbox?.().catch(() => {}); + } + await sandbox?.dispose?.().catch(() => {}); + + return { parked }; +} diff --git a/services/runner/src/environment/timing.ts b/services/runner/src/environment/timing.ts new file mode 100644 index 0000000000..f204b0a8a1 --- /dev/null +++ b/services/runner/src/environment/timing.ts @@ -0,0 +1,68 @@ +/** + * Acquire-stage timing. + * + * LIFECYCLE MIGRATION, STEP 5. The timing helper used to be a closure inside + * `prepareEnvironmentSetup`. It moved here because every lifecycle unit emits a stage line, and a + * unit cannot reach into another module's closure. + * + * THE STAGE NAMES ARE A PUBLIC INTERFACE. Dashboards and log queries match on + * `[timing] stage=`. The split must not rename, drop, or reorder a single one, so the names + * live in `ACQUIRE_STAGES` below and a seam test asserts that the whole set still fires. Adding a + * stage is fine. Renaming one is a breaking change to something outside this repository. + */ + +/** A log sink. Matches the `deps.log` shape the engine already threads everywhere. */ +export type Log = (message: string) => void; + +/** + * Emit one stage line. `fields` is appended verbatim, which is how `sandbox_start` and + * `create_session` carry their ` mode=...` suffix. + */ +export type TimingLog = ( + stage: string, + startedAt: number, + fields?: string, +) => void; + +/** + * Every stage name the acquire path emits, in the order it emits them. + * + * `sandbox_start` and `create_session` appear once each although each has two modes; the mode + * rides the `fields` suffix rather than the stage name, so a dashboard grouping by stage sees one + * series with a mode dimension. + */ +export const ACQUIRE_STAGES = [ + "sandbox_start", + "mounts", + "agent_mount", + "prepare_workspace", + "probe_capabilities", + "create_session", + "acquire_total", +] as const; + +export type AcquireStage = (typeof ACQUIRE_STAGES)[number]; + +/** + * Build the stage logger. + * + * `sandboxId` and `sessionId` are read through accessors, not captured by value. The sandbox does + * not exist when the logger is built, and the session id changes during acquire, so a captured + * value would log `-` for every stage after the first. This is why the helper is a factory rather + * than a plain function. + */ +export function createTimingLog( + logger: Log, + read: { + sandboxId: () => string | undefined; + sessionId: () => string | undefined; + }, +): TimingLog { + return (stage, startedAt, fields = "") => { + const sandboxId = read.sandboxId() ?? "-"; + const sessionId = read.sessionId() ?? "-"; + logger( + `[timing] stage=${stage} ms=${Math.round(Date.now() - startedAt)} sandbox=${sandboxId} session=${sessionId}${fields}`, + ); + }; +} diff --git a/services/runner/src/environment/workspace-manager.ts b/services/runner/src/environment/workspace-manager.ts new file mode 100644 index 0000000000..4d12d8113d --- /dev/null +++ b/services/runner/src/environment/workspace-manager.ts @@ -0,0 +1,114 @@ +/** + * `WorkspaceManager` — the run directory's managed files. + * + * LIFECYCLE MIGRATION, STEP 5. This unit owns one acquire stage (`prepare_workspace`) and one + * teardown step (the workspace cleanup). It is the first unit to split out because it is the one + * step 6 needs: an in-place workspace refresh is the cheapest live route in the whole design. + * + * WHAT "MANAGED" MEANS. The runner owns `AGENTS.md` / `CLAUDE.md`, the rendered harness files, and + * the skill directories. It does NOT own anything else in the run directory: an agent's own + * working files are the user's, and a refresh must never touch them. That boundary is why + * `refresh` takes an explicit manifest rather than reconciling the whole tree. + * + * THE `refresh` ENTRY IS DELIBERATELY UNWIRED. Step 5 is a structural split with zero behavior + * change, so nothing calls `refresh` yet. It exists now, sharing its write path with + * `materialize`, so step 6 is a routing change rather than a new implementation. See the note on + * `refresh` for what it still owes. + */ +import { + prepareWorkspace, + type Workspace, +} from "../engines/sandbox_agent/workspace.ts"; +import type { PiSkillSnapshot } from "../engines/sandbox_agent/pi-assets.ts"; +import type { RunPlan } from "../engines/sandbox_agent/run-plan.ts"; +import type { Log } from "./timing.ts"; + +/** Everything a workspace write needs. The same shape serves both entries. */ +export interface WorkspaceInput { + sandbox: unknown; + plan: Parameters[0]["plan"]; + piSkillSnapshot?: PiSkillSnapshot; + log: Log; +} + +/** The seam tests and the composer both inject through this. */ +export interface WorkspaceDeps { + prepareWorkspace?: typeof prepareWorkspace; +} + +/** + * Write every managed file for a fresh run directory. + * + * This is byte-for-byte what the `prepare_workspace` stage did inline. The caller still owns the + * timing mark and the local-remount retry, because both are acquire-path concerns rather than + * workspace concerns: the retry re-signs a MOUNT, which belongs to the mount unit. + */ +export async function materialize( + input: WorkspaceInput, + deps: WorkspaceDeps = {}, +): Promise { + return (deps.prepareWorkspace ?? prepareWorkspace)({ + sandbox: input.sandbox, + plan: input.plan, + piSkillSnapshot: input.piSkillSnapshot, + log: input.log, + }); +} + +/** + * The set of managed files a refresh should end with. + * + * A manifest is a complete statement, not a delta. That is what lets a refresh delete a skill + * directory that disappeared from the request: the manager compares what it wrote last against + * what the manifest asks for, and removes the difference. A delta could never express a removal + * safely, because it cannot distinguish "unchanged" from "gone". + */ +export interface WorkspaceManifest { + /** Relative path to content, for every file the runner owns in this run directory. */ + readonly files: ReadonlyMap; + /** Skill directory names the runner owns. Anything else under the skills root is removed. */ + readonly skillDirs: readonly string[]; +} + +/** + * STEP 6 ENTRY. Bring an EXISTING run directory to the state a manifest describes. + * + * NOT WIRED, AND NOT YET COMPLETE. Step 5 changes no behavior, so nothing calls this. It is + * declared now so step 6 is a routing change rather than a new implementation, and so the shape + * of the manifest is settled while the split is fresh. + * + * WHAT IT STILL OWES, and none of it may be skipped when step 6 wires it: + * - DELETION. `prepareWorkspace` writes desired files and does not remove files that vanished + * from the request. A refresh that only writes leaves a removed skill readable by the model, + * which is a correctness bug and, for a removed skill, a policy one. + * - A RUNNER-OWNED INVENTORY. Deletion is only safe against a record of what the runner itself + * wrote. Never recursively clean the run directory: it holds the agent's own files. + * - ATOMIC REPLACEMENT where the platform allows it, so a half-written instructions file is + * never visible to a running harness. + * - THE OBSERVATION QUESTION. Writing a file does not prove a running harness reads it. The + * adapter matrix records instructions and skills as `not-guaranteed` for an active session, + * so step 6 must decide per harness whether a refresh alone is honest or whether it must be + * followed by a session reopen. + */ +export async function refresh( + _input: WorkspaceInput, + _manifest: WorkspaceManifest, + _deps: WorkspaceDeps = {}, +): Promise { + throw new Error( + "WorkspaceManager.refresh is not implemented: step 5 is a structural split with no " + + "behavior change. Step 6 wires it, and must first add manifest-based deletion against a " + + "runner-owned inventory. See the doc comment.", + ); +} + +/** + * Remove what this run wrote. Called from the composer's teardown. + * + * The caller decides WHETHER to call it: on a durable local run the cleanup must be skipped + * unless the unmount was confirmed, or it would delete through a live mount into the store. That + * decision needs mount state, so it stays with the mount unit and the composer. + */ +export async function cleanup(workspace: Workspace | undefined): Promise { + await workspace?.cleanup().catch(() => {}); +} diff --git a/services/runner/tests/unit/environment-units.test.ts b/services/runner/tests/unit/environment-units.test.ts new file mode 100644 index 0000000000..93e2de5008 --- /dev/null +++ b/services/runner/tests/unit/environment-units.test.ts @@ -0,0 +1,231 @@ +/** + * Seam tests for the environment lifecycle units (lifecycle migration, step 5). + * + * Same style as the S6 coordinator proof: assert on the SEAM, not on behavior the existing suites + * already cover. Each unit gets its public surface pinned, and the stage names get their own + * guard, because they are matched by dashboards outside this repository. + * + * Run: pnpm exec vitest run tests/unit/environment-units.test.ts + */ +import { describe, it } from "vitest"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +import { + ACQUIRE_STAGES, + createTimingLog, + type AcquireStage, +} from "../../src/environment/timing.ts"; +import * as workspaceManager from "../../src/environment/workspace-manager.ts"; + +const SRC = (rel: string) => + readFileSync(fileURLToPath(new URL(`../../src/${rel}`, import.meta.url)), "utf-8"); + +/** + * Every file that may emit an acquire stage. The split moves stages OUT of `environment.ts` and + * into the units, so the stage guard has to follow them. A unit added later must be listed here, + * or its stages become invisible to the guard. + */ +const STAGE_EMITTERS = [ + "engines/sandbox_agent/environment.ts", + "engines/sandbox_agent/environment-setup.ts", + "environment/sandbox-lifecycle.ts", + "environment/workspace-manager.ts", + "environment/timing.ts", +]; + +const ALL_STAGE_SOURCE = () => STAGE_EMITTERS.map(SRC).join("\n"); + +describe("timing: the stage names are a public interface", () => { + it("emits the documented line shape", () => { + const lines: string[] = []; + const timingLog = createTimingLog((m) => lines.push(m), { + sandboxId: () => "sbx-1", + sessionId: () => "sess-1", + }); + timingLog("sandbox_start", Date.now() - 25, " mode=create"); + assert.match( + lines[0], + /^\[timing\] stage=sandbox_start ms=\d+ sandbox=sbx-1 session=sess-1 mode=create$/, + ); + }); + + it("renders a missing sandbox or session as '-', never as 'undefined'", () => { + const lines: string[] = []; + const timingLog = createTimingLog((m) => lines.push(m), { + sandboxId: () => undefined, + sessionId: () => undefined, + }); + timingLog("acquire_total", Date.now()); + assert.match(lines[0], /sandbox=- session=-/); + assert.doesNotMatch(lines[0], /undefined/); + }); + + it("reads its accessors at CALL time, not at build time", () => { + // The reason this is a factory. The sandbox does not exist when the logger is built, and the + // session id changes during acquire. Capturing either by value would log a stale `-`. + let sandboxId: string | undefined; + const lines: string[] = []; + const timingLog = createTimingLog((m) => lines.push(m), { + sandboxId: () => sandboxId, + sessionId: () => "sess-1", + }); + timingLog("mounts", Date.now()); + sandboxId = "sbx-late"; + timingLog("prepare_workspace", Date.now()); + assert.match(lines[0], /sandbox=-/); + assert.match(lines[1], /sandbox=sbx-late/); + }); + + it("declares every stage the acquire path emits", () => { + // The guard that keeps dashboards working. If a stage is renamed or dropped during the split, + // the source no longer emits it and this fails. + const source = ALL_STAGE_SOURCE(); + for (const stage of ACQUIRE_STAGES) { + assert.ok( + source.includes(`"${stage}"`), + `stage '${stage}' is declared but no longer emitted; dashboards match on this name`, + ); + } + }); + + it("emits no stage that is undeclared", () => { + // The other direction: a new stage must be added to `ACQUIRE_STAGES` so the list stays the + // one place that documents the public set. + const source = ALL_STAGE_SOURCE(); + const emitted = new Set(); + for (const m of source.matchAll(/timingLog\(\s*"([a-z_]+)"/g)) emitted.add(m[1]); + for (const stage of emitted) { + assert.ok( + (ACQUIRE_STAGES as readonly string[]).includes(stage), + `'${stage}' is emitted but not declared in ACQUIRE_STAGES`, + ); + } + assert.ok(emitted.size > 0, "the source must still emit stages"); + }); + + it("keeps the two-mode stages as ONE stage name with a mode suffix", () => { + // A dashboard groups by stage and splits by mode. Turning `sandbox_start` into + // `sandbox_start_create` would silently break every existing query. + const source = ALL_STAGE_SOURCE(); + for (const [stage, modes] of [ + ["sandbox_start", ["reconnect", "create"]], + ["create_session", ["load", "create"]], + ] as Array<[AcquireStage, string[]]>) { + for (const mode of modes) { + assert.ok( + source.includes(`"${stage}", `) && source.includes(`mode=${mode}`), + `${stage} must still carry mode=${mode} as a field, not in its name`, + ); + } + } + }); +}); + +describe("workspace manager: the public surface", () => { + it("exposes materialize, refresh, and cleanup", () => { + assert.equal(typeof workspaceManager.materialize, "function"); + assert.equal(typeof workspaceManager.refresh, "function"); + assert.equal(typeof workspaceManager.cleanup, "function"); + }); + + it("materialize delegates to the injected writer with the caller's input", async () => { + const seen: unknown[] = []; + const fake = { cleanup: async () => {} }; + const result = await workspaceManager.materialize( + { + sandbox: { id: "sbx" }, + plan: { marker: "the-plan" } as never, + piSkillSnapshot: { marker: "snapshot" } as never, + log: () => {}, + }, + { + prepareWorkspace: (async (input: unknown) => { + seen.push(input); + return fake; + }) as never, + }, + ); + assert.equal(result, fake); + assert.equal(seen.length, 1); + const input = seen[0] as Record; + assert.deepEqual(input.plan, { marker: "the-plan" }); + assert.deepEqual(input.sandbox, { id: "sbx" }); + }); + + it("refresh is DECLARED but not implemented, and says so", async () => { + // Step 5 is a structural split with zero behavior change. The entry exists so step 6 is a + // routing change; throwing is what keeps it from being wired by accident. + await assert.rejects( + () => + workspaceManager.refresh( + { sandbox: {}, plan: {} as never, log: () => {} }, + { files: new Map(), skillDirs: [] }, + ), + (err: Error) => /not implemented/.test(err.message), + ); + }); + + it("refresh takes a complete manifest, because deletion needs one", () => { + // A delta cannot express a removal: it cannot tell "unchanged" from "gone". The manifest + // shape is what lets step 6 delete a skill directory that left the request. + const manifest: workspaceManager.WorkspaceManifest = { + files: new Map([["AGENTS.md", "body"]]), + skillDirs: ["pdf-tools"], + }; + assert.equal(manifest.files.get("AGENTS.md"), "body"); + assert.deepEqual([...manifest.skillDirs], ["pdf-tools"]); + }); + + it("cleanup swallows a failing cleanup and tolerates no workspace", async () => { + // Teardown must never throw. Both are exercised because a partial acquire leaves no + // workspace at all. + await workspaceManager.cleanup(undefined); + await workspaceManager.cleanup({ + cleanup: async () => { + throw new Error("boom"); + }, + }); + }); + + it("does NOT wire any in-place route yet", () => { + // The scope line for step 5. `refresh` must have no caller until step 6. + for (const rel of [ + "engines/sandbox_agent/environment.ts", + "engines/sandbox_agent/environment-setup.ts", + "lifecycle/session-coordinator.ts", + ]) { + assert.ok( + !/\brefresh(Workspace)?\s*\(/.test(SRC(rel)), + `${rel} calls the workspace refresh; step 5 must not wire an in-place route`, + ); + } + }); +}); + +describe("the composer delegates instead of inlining", () => { + it("environment.ts writes the workspace through the unit", () => { + const source = SRC("engines/sandbox_agent/environment.ts"); + assert.ok(source.includes("materializeWorkspace(")); + assert.ok(source.includes("cleanupWorkspace(")); + }); + + it("environment.ts no longer calls prepareWorkspace directly", () => { + // The seam only holds if the composer cannot reach past it. + const source = SRC("engines/sandbox_agent/environment.ts"); + assert.ok( + !/deps\.prepareWorkspace \?\? prepareWorkspace/.test(source), + "the composer still inlines the workspace write", + ); + }); + + it("environment-setup.ts builds the stage logger through the timing unit", () => { + const source = SRC("engines/sandbox_agent/environment-setup.ts"); + assert.ok(source.includes("createTimingLog(")); + assert.ok( + !source.includes("`[timing] stage="), + "the line shape must live in one place", + ); + }); +});