refactor(runner): extract the sandbox and workspace lifecycle units

[mmabrouk]

Context

Part of the agent-config-editing stack. Targets agent-config-editing-s1b. Read the stack bottom up.

Expect no behavior change. This is a code move: the reconnect ladder, the fresh-create fallback, the park-versus-delete decision, the in-flight registry, and the workspace write all behave exactly as they did inline. The characterization suite from s5 and the keep-alive suites pin that across the move.

acquireEnvironment was one long function that owned every layer of an environment at once. Later lanes need to reconfigure individual layers of a running environment, and you cannot reconfigure a layer that has no boundary.

This lane is partial by design. Three units come out cleanly. The remaining three are entangled and land in the next lane.

Changes

Three units now live under src/environment/:

• sandbox-lifecycle.ts owns the provider instance: the sandbox_start stage and the sandbox half of teardown. Acquire either reconnects a parked sandbox or creates a fresh one, and both emit sandbox_start with the mode in the mode=... field, so a dashboard grouping by stage keeps one series with a mode dimension. The reconnect ladder still never fails a turn. A stored id that will not reconnect degrades to a fresh create, which is why that catch swallows.
• workspace-manager.ts owns writing the managed files and, new here, recording an inventory of what it wrote. A refresh cannot know what to delete without one, and listing the directory is not an answer because a durable cwd holds the user's own project.
• timing.ts holds the acquire-stage timing helper, which used to be a closure inside prepareEnvironmentSetup. Every 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=<name>, so ACQUIRE_STAGES lists them and a seam test asserts the whole set still fires. Adding a stage is fine. Renaming one breaks something outside this repository.

Tests / notes

• 15 new tests in environment-units.test.ts, plus the seam test over the stage list.
• Worth poking at: the inventory shape. It is what makes deletion decidable in the later refresh lane, and getting it wrong there means a removed skill stays readable.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agenta-documentation	Ready	Preview	Aug 6, 2026 5:15pm

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Improved sandbox recovery by reconnecting to existing environments when possible and creating new ones when needed.
  • Added safer sandbox shutdown, including parking eligible environments and cleaning up others.
  • Improved workspace setup and cleanup reliability.
  • Added clearer timing information for environment acquisition stages.

• Bug Fixes

  • Workspace cleanup errors no longer interrupt environment teardown.
  • Reconnect failures now fall back gracefully to creating a fresh environment.

Walkthrough

The runner extracts sandbox acquisition and teardown, workspace management, and acquire-stage timing into shared environment modules. The environment composer delegates lifecycle and workspace operations to these modules. Unit tests cover the new seams and delegation boundaries.

Changes

Sandbox environment lifecycle

Layer / File(s)	Summary
Acquire timing logging services/runner/src/environment/timing.ts, services/runner/src/engines/sandbox_agent/environment-setup.ts, services/runner/tests/unit/environment-units.test.ts	Adds shared acquire-stage timing definitions and logging. Logs resolve sandbox and session identifiers at call time.
Sandbox acquisition and teardown services/runner/src/environment/sandbox-lifecycle.ts, services/runner/src/engines/sandbox_agent/environment.ts	Centralizes sandbox reconnect, creation fallback, timing, resumability, parking, deletion, and disposal.
Workspace materialization and cleanup services/runner/src/environment/workspace-manager.ts, services/runner/src/engines/sandbox_agent/environment.ts, services/runner/tests/unit/environment-units.test.ts	Delegates workspace preparation and cleanup to shared operations. Defines the workspace manifest and an explicit not-implemented refresh entry point. Tests validate delegation and cleanup behavior.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Environment
  participant SandboxLifecycle
  participant DaytonaSandbox
  Environment->>SandboxLifecycle: Acquire stored or new sandbox
  SandboxLifecycle->>DaytonaSandbox: Reconnect by sandbox ID
  DaytonaSandbox-->>SandboxLifecycle: Handle or reconnect failure
  SandboxLifecycle-->>Environment: Handle, mode, and resumability
  Environment->>SandboxLifecycle: Teardown sandbox
  SandboxLifecycle->>DaytonaSandbox: Park, delete, and dispose
  SandboxLifecycle-->>Environment: Parked status

Loading

Possibly related PRs

• Agenta-AI/agenta#4856: Modifies shared Daytona sandbox acquisition and teardown behavior.
• Agenta-AI/agenta#5102: Modifies sandbox-agent teardown before sandbox destruction.
• Agenta-AI/agenta#5156: Refactors sandbox-agent environment acquisition, teardown, and workspace lifecycle handling.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the extraction of sandbox and workspace lifecycle units.
Description check	✅ Passed	The description directly explains the lifecycle refactor, preserved behavior, new modules, stage interface, and tests.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch agent-config-editing-s7a

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mmabrouk]

These stage names are a public interface. Dashboards and log queries match on [timing] stage=<name>.

The split must not rename, drop, or reorder a single one. That is why the names live in this array instead of at each call site, and why a seam test asserts that the whole set still fires after the move. The test is the evidence that the extraction preserved behavior at the observable boundary.

Adding a stage is safe. Renaming one breaks something outside this repository.

sandbox_start and create_session appear once each although both have two modes. The mode rides a mode=... suffix, so a dashboard grouping by stage keeps one series with a mode dimension.

[mmabrouk]

This function is a code move. The reconnect ladder, the fresh create fallback, and the extra log line for a terminal Daytona state all behave exactly as they did inline.

The swallowed reconnect failure is deliberate and predates this PR. A stored id that will not reconnect degrades to a fresh create. A dead sandbox is an ordinary outcome, not an error, and the only cost is one round trip. If that catch stopped swallowing, a stale pointer would fail a turn that used to succeed.

The helper takes an explicit input object rather than reading shared state. That is what lets the unit be tested without an environment.

[mmabrouk]

This unit records what the runner wrote into the run directory. That record is new here, and the next lane depends on it.

The runner owns AGENTS.md or CLAUDE.md, the rendered harness files, and the skill directories. It owns nothing else. An agent's own working files belong to the user, and a refresh must never touch them.

That boundary is the reason refresh takes an explicit manifest instead of reconciling the whole tree, and the reason deletions are computed from this inventory instead of from a directory listing.

refresh is deliberately unwired in this PR. It exists now, sharing its write path with materialize, so the next lane is a routing change rather than a new implementation.

[mmabrouk]

These tests assert the seam, not the behavior. That is the right division for this PR.

The behavior is already covered. The keep-alive suites and the characterization suite from the lane below exercise the same acquire and teardown paths through the engine, and they pass unedited across this move. Re-asserting that behavior here would duplicate them without adding evidence.

What these tests add is the boundary itself. Each unit gets its public surface pinned, and the stage list gets its own guard. Those are the things a later refactor could break silently, because no product test would notice a renamed stage.

[mmabrouk]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

services/runner/src/environment/timing.ts (1)

21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider typing stage as AcquireStage instead of string.

ACQUIRE_STAGES and AcquireStage declare the public set, but TimingLog accepts any string. A renamed or newly added stage therefore compiles cleanly, and the only guard is the source-regex assertion in tests/unit/environment-units.test.ts. That guard matches timingLog("<literal>" only, so a computed stage argument escapes it.

If you type the parameter, tsc --strict enforces the same contract at compile time and the regex guard becomes a backstop rather than the primary control. ACQUIRE_STAGES is declared after TimingLog today, so move the array and the AcquireStage alias above the type.

♻️ Proposed change

 export type TimingLog = (
-  stage: string,
+  stage: AcquireStage,
   startedAt: number,
   fields?: string,
 ) => void;

services/runner/src/environment/sandbox-lifecycle.ts (1)

42-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

The seam erases the sandbox handle type.

SandboxAcquireDeps.startSandboxAgent returns Promise<unknown> and SandboxAcquireResult.sandbox is unknown. The composer therefore casts on both sides: startSandboxAgent as unknown as (options: Record<string, unknown>) => Promise<unknown> at services/runner/src/engines/sandbox_agent/environment.ts lines 673-675, and it assigns the unknown result to environment.sandbox at line 681.

Before the extraction, tsc --strict checked that the composer's startSandboxAgent matched SandboxAgent.start and that the returned handle carried createSession. After the extraction it checks neither. The runtime assert at environment.ts lines 903-907 is now the only check that the handle is usable.

Make the unit generic in the handle type so the composer keeps its inference and drops both casts.

♻️ Proposed change

-export interface SandboxAcquireDeps {
-  startSandboxAgent: (options: Record<string, unknown>) => Promise<unknown>;
+export interface SandboxAcquireDeps<TSandbox> {
+  startSandboxAgent: (options: Record<string, unknown>) => Promise<TSandbox>;
   readStoredSandboxPointer?: typeof readStoredSandboxPointer;
 }
 
-export interface SandboxAcquireResult {
-  sandbox: unknown;
+export interface SandboxAcquireResult<TSandbox> {
+  sandbox: TSandbox | undefined;
   /** 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";
 }

acquire then becomes acquire<TSandbox>(input: SandboxAcquireInput, deps: SandboxAcquireDeps<TSandbox>): Promise<SandboxAcquireResult<TSandbox>>, with the local let sandbox: TSandbox | undefined.

services/runner/src/environment/workspace-manager.ts (1)

112-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

A failed workspace cleanup is now silent.

cleanup discards the rejection and takes no logger. The composer calls it at services/runner/src/engines/sandbox_agent/environment.ts line 358, inside environment.destroy. Every other teardown failure on that path is logged: the agent mountpoint failure at lines 347-349, and the skipped cleanup at lines 354-356. A failed workspace cleanup leaves a run directory on disk with no log line, so the leak is undetectable.

Swallowing is correct, because destroy must never throw. Add an optional log sink so the failure is still recorded.

♻️ Proposed change

-export async function cleanup(workspace: Workspace | undefined): Promise<void> {
-  await workspace?.cleanup().catch(() => {});
+export async function cleanup(
+  workspace: Workspace | undefined,
+  log?: Log,
+): Promise<void> {
+  await workspace?.cleanup().catch((err: unknown) => {
+    log?.(`workspace cleanup failed: ${(err as Error)?.message ?? err}`);
+  });
 }

At environment.ts line 358, pass the logger:

-      await cleanupWorkspace(environment.workspace);
+      await cleanupWorkspace(environment.workspace, logger);

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 70daaa06-555f-4ab3-b767-d2f51a7cf346

📥 Commits

Reviewing files that changed from the base of the PR and between 33255a0 and 8e17fe0.

📒 Files selected for processing (6)

• services/runner/src/engines/sandbox_agent/environment-setup.ts
• services/runner/src/engines/sandbox_agent/environment.ts
• services/runner/src/environment/sandbox-lifecycle.ts
• services/runner/src/environment/timing.ts
• services/runner/src/environment/workspace-manager.ts
• services/runner/tests/unit/environment-units.test.ts

[mmabrouk]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.