refactor(runner): decompose the sandbox_agent engine into small modules#5369
Merged
mmabrouk merged 11 commits intoJul 18, 2026
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe sandbox-agent runner is decomposed into focused modules for contracts, policy, session identity, environment setup and lifecycle, turn execution, and engine composition. The original entrypoint remains a public re-export facade, while provider, harness, and tool-delivery ports are documented for future work. ChangesSandbox-agent decomposition
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Runner
participant acquireEnvironment
participant SandboxAgent
participant runTurn
participant SessionContinuity
Runner->>acquireEnvironment: acquire or reconnect environment
acquireEnvironment->>SandboxAgent: create session and probe capabilities
SandboxAgent-->>acquireEnvironment: session environment
acquireEnvironment-->>Runner: SessionEnvironment
Runner->>runTurn: execute one turn
runTurn->>SandboxAgent: prompt and handle events
SandboxAgent-->>runTurn: output, tool calls, and permissions
runTurn->>SessionContinuity: record or invalidate continuity
runTurn-->>Runner: AgentRunResult
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
mmabrouk
force-pushed
the
refactor/runner-sandbox-agent-decomp
branch
from
July 17, 2026 21:38
d980ded to
b019a74
Compare
Move the pure identity, fingerprint, credential-epoch, and pool-key helpers out of session-pool.ts into a new session-identity.ts, leaving session-pool.ts as the mutable pool (map, timers, capacity, state, teardown). Pure code move: importers repointed, no behavior change. Typecheck clean; 1158 runner unit tests pass. Claude-Session: https://claude.ai/code/session_01RZnMTx5et6hRo5EHN187tT
Move the small pure policy functions (runCredential, serverPermissionsFromRequest, shouldSuppressPausedToolCallUpdate, applyClaudeConnectionEnv, modelResolutionStrict, defaultResolveLocalRunnerOwner, and the two transport-endpoint-disconnected checks) out of the monolith into runtime-policy.ts. Pure code move; monolith imports them back. Typecheck clean; 1158 runner unit tests pass. Claude-Session: https://claude.ai/code/session_01RZnMTx5et6hRo5EHN187tT
Move the engine's shared interfaces and types (SandboxAgentDeps, CurrentTurn, ParkedApproval, ResumeApprovalInput, RunTurnOptions, sendLastMessageOnly, SessionEnvironment, AcquireEnvironmentResult, RUN_LIMIT_TRIPPED) into runtime-contracts.ts. Monolith re-exports the public ones so external importers are unchanged. Pure code move; typecheck clean; 1158 runner unit tests pass. Claude-Session: https://claude.ai/code/session_01RZnMTx5et6hRo5EHN187tT
Move the two session-listener routers (routeSessionEventToActiveTurn, routePermissionRequestToActiveTurn) into session-events.ts. Pure code move (today's versions verbatim, including the any-typed event/req params); monolith imports them. Typecheck clean; 1158 runner unit tests pass. Claude-Session: https://claude.ai/code/session_01RZnMTx5et6hRo5EHN187tT
Move the environment lifecycle whole and unchanged (acquireEnvironment, destroyInFlightSandboxes, destroyInFlightSandboxesForSession, resolveKeepaliveMount, invalidateContinuity, plus the inFlightSandboxes registry and remount-limit const) into environment.ts. acquireEnvironment stays a single function; the setup carve into environment-setup.ts is a later step. Monolith re-exports the public functions and imports acquireEnvironment/invalidateContinuity back. Pure code move; typecheck clean; 1158 runner unit tests pass. Claude-Session: https://claude.ai/code/session_01RZnMTx5et6hRo5EHN187tT
Move runTurn whole and unchanged into run-turn.ts. Monolith re-exports it and imports it back for runSandboxAgent. Pure code move; typecheck clean; 1158 runner unit tests pass. Claude-Session: https://claude.ai/code/session_01RZnMTx5et6hRo5EHN187tT
…facade Move shouldPark and runSandboxAgent into engine.ts. sandbox_agent.ts is now only re-exports of the public surface, unchanged from before the split. Pure code move; typecheck clean; 1158 runner unit tests pass. Claude-Session: https://claude.ai/code/session_01RZnMTx5et6hRo5EHN187tT
Lift the setup prefix of acquireEnvironment (logger/timing, ownership check, mount signing, durable cwd, buildRunPlan, daemon env, pi extension env, binary resolve, initial SessionEnvironment construction) into prepareEnvironmentSetup, which returns a bundle that acquireEnvironment destructures. Statement order and side effects unchanged; typecheck enforces the bundle is complete. 1158 runner unit tests pass. Claude-Session: https://claude.ai/code/session_01RZnMTx5et6hRo5EHN187tT
…osition handoff Add a placement table to services/runner/AGENTS.md so future agents can self-route to the right sandbox_agent module, including the daemon.ts location-dependency warning. Add the project handoff and the Phase 3 port design under docs/design/agent-workflows/projects/runner-desloppify-redo/. Claude-Session: https://claude.ai/code/session_01RZnMTx5et6hRo5EHN187tT
…position v0.105.4 (#5345) added the OpenAI-compatible custom-connection model-config plan inside acquireEnvironment in the old monolith. This decomposition moved acquireEnvironment into environment.ts + environment-setup.ts, so on rebase that release logic is threaded through the new split: the plan is built and localModelConfigUnwritable computed in prepareEnvironmentSetup and returned in its bundle; the fail-loud/fail-closed throws, the Daytona asset piModelConfig arg, and the fully-qualified wantedModel selection live in acquireEnvironment. Behavior is identical to #5345.
mmabrouk
force-pushed
the
refactor/runner-sandbox-agent-decomp
branch
from
July 18, 2026 11:38
b019a74 to
db13a17
Compare
mmabrouk
commented
Jul 18, 2026
mmabrouk
left a comment
mmabrouk
left a comment
Member
Author
There was a problem hiding this comment.
Reviewed commit by commit after the rebase onto released main (80daf23). Verdict: the decomposition is faithful and the rebase dropped nothing from v0.105.4.
What I verified:
- Move fidelity. The full runner diff vs main is exactly the decomposition: the facade, the new
sandbox_agent/modules, thesession-identitysplit out ofsession-pool, and a 4-line import change inserver.ts. No stray edits. - Release code preserved. Every unmoved release file is byte-identical to main:
transcript.ts(#5364),run-plan.ts(#5366),pi-model-config.ts/pi-assets.ts(#5345),daytona.ts,tracing/otel.ts(#5362),package.json,pnpm-lock.yaml. - The one behavioral commit, db13a17 (the #5345 re-home). I diffed all four re-homed hunks against main's monolith line by line: the plan build + log, the
localModelConfigUnwritablecomputation, the three gate throws inside thetry(same order: config error, permission extension, unwritable config), thepiModelConfigarg toprepareDaytonaPiAssets, and thewantedModelselection. All verbatim identical to main, comments included. - Older fixes intact. #5318 fail-closed gating and the #5343-era "produced no output" Pi error surfacing both live in the moved modules (
environment-setup.ts,pi-error.ts,run-turn.ts). - Unit suite green at 76 files / 1192 tests, typecheck clean.
Remaining gate before merge: the full agent release gate on a freshly built stack (in progress). Not blocking review sign-off; blocking merge.
| // Fail closed (Decision 6): a local managed custom run whose models.json could not be written | ||
| // must stop rather than run on a default provider. Recorded here (the write ran above) and | ||
| // thrown inside the try below, like the permission-extension gate. | ||
| const localModelConfigUnwritable = |
Member
Author
There was a problem hiding this comment.
Verified verbatim against main's monolith (lines 896-900): same predicate, same plan.isDaytona exclusion, same comment. One Phase-2 cleanup idea, not a change request for this PR: piModelConfig, piModelConfigError, and localModelConfigUnwritable now travel as three loose fields in the setup bundle. When we do the folder grouping, folding them into a single modelConfigPlan object with a status would shrink the bundle's surface and make the fail-closed contract self-describing.
| // openai provider live) would be selected ahead of the custom `<slug>/<model>` when both share | ||
| // the model id. That would silently route to api.openai.com instead of the user's endpoint. | ||
| // The qualified id is an EXACT match, so it always wins over any bare-suffix collision. | ||
| const wantedModel = |
Member
Author
There was a problem hiding this comment.
Verified verbatim against main (#5345, monolith lines 1758-1764), including the suffix-collision rationale in the comment above: the fully qualified <connection-slug>/<model-id> must win over a bare-suffix match on Pi's still-live built-in openai provider. This was the subtlest piece of the re-home and it survived intact.
|
|
||
| export { |
Member
Author
There was a problem hiding this comment.
The facade's export surface matches every external consumer I could find (server.ts, the CLI, and the three keepalive/pool test files import through here or through session-pool directly). Typecheck plus the 1192-test suite pin this at compile and runtime level. The doc comment's module map matches the actual layout.
mmabrouk
marked this pull request as ready for review
July 18, 2026 12:10
dosubot
Bot
added
size:XXL
Contributor
Railway Preview Environment
Updated at 2026-07-18T12:26:18.224Z |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3ff1c8e7-69d6-4385-b922-b60e742b7e53
📒 Files selected for processing (17)
docs/design/agent-workflows/projects/runner-desloppify-redo/HANDOFF.mddocs/design/agent-workflows/projects/runner-desloppify-redo/port-design.mdservices/runner/AGENTS.mdservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/engine.tsservices/runner/src/engines/sandbox_agent/environment-setup.tsservices/runner/src/engines/sandbox_agent/environment.tsservices/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/engines/sandbox_agent/runtime-contracts.tsservices/runner/src/engines/sandbox_agent/runtime-policy.tsservices/runner/src/engines/sandbox_agent/session-events.tsservices/runner/src/engines/sandbox_agent/session-identity.tsservices/runner/src/engines/sandbox_agent/session-pool.tsservices/runner/src/server.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-pool.test.ts
Comment on lines
+191
to
+199
| 1. A real, hard-to-verify-tonight behavior risk in grouping. `daemon.ts` computes its package root | ||
| as `dirname(dirname(dirname(fileURLToPath(import.meta.url))))` — three directory levels up from | ||
| the file's own location. That package root drives where the Pi and adapter binaries are found at | ||
| runtime. Moving `daemon.ts` one level deeper into a subfolder silently changes that root and | ||
| breaks binary resolution in a real deployment, and the unit tests do not exercise real binary | ||
| resolution, so typecheck and the unit suite would BOTH stay green while the runtime broke. It is | ||
| the only file with self-location path logic (verified by grep for import.meta.url / __dirname / | ||
| fileURLToPath across the whole folder), but it means grouping is not the pure, test-gated | ||
| mechanical change it appears to be. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the documented daemon.ts package-root depth.
The implementation uses four nested dirname calls, while both documents say three.
docs/design/agent-workflows/projects/runner-desloppify-redo/HANDOFF.md#L191-L199: change “three directory levels up” to “four directory levels up.”services/runner/AGENTS.md#L54-L56: make the same correction to the runtime warning.
📍 Affects 2 files
docs/design/agent-workflows/projects/runner-desloppify-redo/HANDOFF.md#L191-L199(this comment)services/runner/AGENTS.md#L54-L56
Comment on lines
+88
to
+90
| Asset preparation is owned by the harness and dispatched in exactly one place, keyed on the harness | ||
| id, and only when the provider is remote. An earlier attempt grew three competing places for this; | ||
| the port collapses them to one. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve local Pi asset preparation.
Asset preparation cannot be globally gated on a remote provider: the current local path uses prepareLocalPiAssets. Keep one harness dispatch, but gate only remote-specific uploads rather than the entire preparation path.
Proposed clarification
-Asset preparation is owned by the harness and dispatched in exactly one place, keyed on the harness
-id, and only when the provider is remote.
+Asset preparation is owned by the harness and dispatched in exactly one place, keyed on the harness
+id. Preserve both local preparation (`prepareLocalPiAssets`) and remote upload/preparation, gating
+only operations that are inherently remote.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Asset preparation is owned by the harness and dispatched in exactly one place, keyed on the harness | |
| id, and only when the provider is remote. An earlier attempt grew three competing places for this; | |
| the port collapses them to one. | |
| Asset preparation is owned by the harness and dispatched in exactly one place, keyed on the harness | |
| id. Preserve both local preparation (`prepareLocalPiAssets`) and remote upload/preparation, gating | |
| only operations that are inherently remote. |
Comment on lines
+186
to
+191
| const otlpAuthFilePath = | ||
| plan.isPi && !plan.isDaytona ? `${plan.relayDir}.otlp-auth` : undefined; | ||
| const otlpAuthorization = | ||
| request.telemetry?.exporters?.otlp?.headers?.authorization; | ||
| if (otlpAuthFilePath && otlpAuthorization) { | ||
| writeOtlpAuthFile(otlpAuthFilePath, otlpAuthorization, logger); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle both authorization header casings.
HTTP header names are case-insensitive, but this object lookup is not. A request using Authorization skips the auth-file write and leaves local Pi telemetry unauthenticated.
Proposed fix
- const otlpAuthorization =
- request.telemetry?.exporters?.otlp?.headers?.authorization;
+ const otlpHeaders = request.telemetry?.exporters?.otlp?.headers;
+ const otlpAuthorization =
+ otlpHeaders?.authorization ?? otlpHeaders?.Authorization;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const otlpAuthFilePath = | |
| plan.isPi && !plan.isDaytona ? `${plan.relayDir}.otlp-auth` : undefined; | |
| const otlpAuthorization = | |
| request.telemetry?.exporters?.otlp?.headers?.authorization; | |
| if (otlpAuthFilePath && otlpAuthorization) { | |
| writeOtlpAuthFile(otlpAuthFilePath, otlpAuthorization, logger); | |
| const otlpAuthFilePath = | |
| plan.isPi && !plan.isDaytona ? `${plan.relayDir}.otlp-auth` : undefined; | |
| const otlpHeaders = request.telemetry?.exporters?.otlp?.headers; | |
| const otlpAuthorization = | |
| otlpHeaders?.authorization ?? otlpHeaders?.Authorization; | |
| if (otlpAuthFilePath && otlpAuthorization) { | |
| writeOtlpAuthFile(otlpAuthFilePath, otlpAuthorization, logger); |
Comment on lines
+259
to
+260
| const setup = await prepareEnvironmentSetup(request, deps, presignedMount); | ||
| if (!setup.ok) return setup; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep setup inside the acquisition error boundary.
prepareEnvironmentSetup() performs awaited calls and filesystem writes before the try at Line 568. Any thrown error rejects acquireEnvironment instead of returning AcquireEnvironmentResult, and partially created setup artifacts bypass teardown. Add rollback-aware handling around setup.
Comment on lines
+291
to
+294
| environment.destroy = async (opts?: { reason?: TeardownReason }) => { | ||
| if (environment.destroyed) return; | ||
| environment.destroyed = true; | ||
| await environment.runtimeRemount?.catch(() => {}); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Return the same teardown promise to concurrent callers.
destroyed is set before the first await, so a concurrent call returns while teardown is still running. Shutdown draining can consequently finish before the sandbox pause/delete completes. Memoize and return one shared destruction promise.
Comment on lines
+738
to
+773
| const canMount = storeReachableFromSandbox(storeEndpoint) || !!endpoint; | ||
| if ( | ||
| canMount && | ||
| (await (deps.mountStorageRemote ?? mountStorageRemote)( | ||
| environment.sandbox, | ||
| plan.cwd, | ||
| environment.mountCreds, | ||
| { | ||
| endpoint, | ||
| log: logger, | ||
| }, | ||
| )) | ||
| ) { | ||
| logger(`remote durable cwd active for session=${sessionForMount}`); | ||
| } | ||
| // Per-harness session/transcript-dir mounts, remote-only by construction (this whole | ||
| // branch is `plan.isDaytona`) — local runs never reach here, so they stay mount-free/ | ||
| // byte-identical. Transcript mounts derive from the session contract (a durable cwd mount | ||
| // is active), with no separate public switch or credential/session-id path. | ||
| if (canMount && sessionForMount && runCred) { | ||
| const dirs = harnessSessionMounts( | ||
| plan.acpAgent, | ||
| "/home/sandbox", | ||
| DAYTONA_PI_DIR, | ||
| ); | ||
| await (deps.mountHarnessSessionDirs ?? mountHarnessSessionDirs)( | ||
| environment.sandbox, | ||
| sessionForMount, | ||
| dirs, | ||
| endpoint, | ||
| { | ||
| apiBase: apiBase(), | ||
| authorization: runCred, | ||
| log: logger, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mount harness session directories only after the durable cwd mount succeeds.
canMount proves connectivity, not mount success. When mountStorageRemote() returns false, the code still mounts harness transcript directories, producing a partially durable environment contrary to the stated invariant.
Proposed fix
- if (
- canMount &&
- (await (deps.mountStorageRemote ?? mountStorageRemote)(
+ const durableCwdMounted =
+ canMount &&
+ (await (deps.mountStorageRemote ?? mountStorageRemote)(
environment.sandbox,
plan.cwd,
environment.mountCreds,
{
endpoint,
log: logger,
},
- ))
- ) {
+ ));
+ if (durableCwdMounted) {
logger(`remote durable cwd active for session=${sessionForMount}`);
}
- if (canMount && sessionForMount && runCred) {
+ if (durableCwdMounted && sessionForMount && runCred) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const canMount = storeReachableFromSandbox(storeEndpoint) || !!endpoint; | |
| if ( | |
| canMount && | |
| (await (deps.mountStorageRemote ?? mountStorageRemote)( | |
| environment.sandbox, | |
| plan.cwd, | |
| environment.mountCreds, | |
| { | |
| endpoint, | |
| log: logger, | |
| }, | |
| )) | |
| ) { | |
| logger(`remote durable cwd active for session=${sessionForMount}`); | |
| } | |
| // Per-harness session/transcript-dir mounts, remote-only by construction (this whole | |
| // branch is `plan.isDaytona`) — local runs never reach here, so they stay mount-free/ | |
| // byte-identical. Transcript mounts derive from the session contract (a durable cwd mount | |
| // is active), with no separate public switch or credential/session-id path. | |
| if (canMount && sessionForMount && runCred) { | |
| const dirs = harnessSessionMounts( | |
| plan.acpAgent, | |
| "/home/sandbox", | |
| DAYTONA_PI_DIR, | |
| ); | |
| await (deps.mountHarnessSessionDirs ?? mountHarnessSessionDirs)( | |
| environment.sandbox, | |
| sessionForMount, | |
| dirs, | |
| endpoint, | |
| { | |
| apiBase: apiBase(), | |
| authorization: runCred, | |
| log: logger, | |
| }, | |
| ); | |
| const durableCwdMounted = | |
| canMount && | |
| (await (deps.mountStorageRemote ?? mountStorageRemote)( | |
| environment.sandbox, | |
| plan.cwd, | |
| environment.mountCreds, | |
| { | |
| endpoint, | |
| log: logger, | |
| }, | |
| )); | |
| if (durableCwdMounted) { | |
| logger(`remote durable cwd active for session=${sessionForMount}`); | |
| } | |
| // Per-harness session/transcript-dir mounts, remote-only by construction (this whole | |
| // branch is `plan.isDaytona`) — local runs never reach here, so they stay mount-free/ | |
| // byte-identical. Transcript mounts derive from the session contract (a durable cwd mount | |
| // is active), with no separate public switch or credential/session-id path. | |
| if (durableCwdMounted && sessionForMount && runCred) { | |
| const dirs = harnessSessionMounts( | |
| plan.acpAgent, | |
| "/home/sandbox", | |
| DAYTONA_PI_DIR, | |
| ); | |
| await (deps.mountHarnessSessionDirs ?? mountHarnessSessionDirs)( | |
| environment.sandbox, | |
| sessionForMount, | |
| dirs, | |
| endpoint, | |
| { | |
| apiBase: apiBase(), | |
| authorization: runCred, | |
| log: logger, | |
| }, | |
| ); |
Comment on lines
+145
to
+149
| // Every emitted event is a progress signal for the idle/TTFB deadlines (message/thought | ||
| // deltas, tool calls and results, usage, ...) — the one seam every harness's output flows | ||
| // through. Per-tool-call timers are driven separately from `handleUpdate` below. | ||
| emit: emit && runLimits.wrapEmit(emit), | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep run-limit progress tracking active for non-streaming runs.
Line 148 wraps events only when emit exists. Callers collecting run.events() therefore provide no message/thought progress to the TTFB and idle timers, allowing active non-streaming turns to time out. Always route internal events through the limits tracker, forwarding externally only when emit is present.
Comment on lines
+413
to
+418
| // Ordering invariant: the relay's stale-file sweep must complete before the | ||
| // resume's respondPermission or the fresh prompt below can cause a legitimate | ||
| // request, so nothing legitimate can predate the sweep and be swallowed as | ||
| // stale. Optional-chained so a fake relay without `ready` is tolerated, and a | ||
| // sweep failure never kills the turn. | ||
| await turn.toolRelay?.ready?.catch?.(() => {}); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Race startup and approval-response waits against the run limit.
The relay readiness and respondPermission awaits occur before the Promise.race on Line 467. If either stalls, runLimitTripped cannot unblock the turn, defeating the total deadline and retaining the sandbox indefinitely.
Also applies to: 449-470
Comment on lines
+27
to
+35
| // Record live ACP tool_call ids so a paused client_tool can correlate to Claude's bubble | ||
| // (session-scoped; a lookup CONSUMES its matched id). | ||
| environment.toolCallIndex.record(update); | ||
| const turn = environment.currentTurn; | ||
| if (turn) { | ||
| turn.handleUpdate(update); | ||
| } else { | ||
| // Between turns (parked/idle): no turn owns this event. Log and drop by decision. | ||
| logger(`[keepalive] between-turns event dropped`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not record events that are dropped between turns.
toolCallIndex.record(update) mutates session-scoped correlation state before ownership is checked. A late tool_call can therefore be consumed by the next turn despite being logged as dropped.
Proposed fix
- environment.toolCallIndex.record(update);
const turn = environment.currentTurn;
if (turn) {
+ environment.toolCallIndex.record(update);
turn.handleUpdate(update);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Record live ACP tool_call ids so a paused client_tool can correlate to Claude's bubble | |
| // (session-scoped; a lookup CONSUMES its matched id). | |
| environment.toolCallIndex.record(update); | |
| const turn = environment.currentTurn; | |
| if (turn) { | |
| turn.handleUpdate(update); | |
| } else { | |
| // Between turns (parked/idle): no turn owns this event. Log and drop by decision. | |
| logger(`[keepalive] between-turns event dropped`); | |
| const turn = environment.currentTurn; | |
| if (turn) { | |
| environment.toolCallIndex.record(update); | |
| turn.handleUpdate(update); | |
| } else { | |
| // Between turns (parked/idle): no turn owns this event. Log and drop by decision. | |
| logger(`[keepalive] between-turns event dropped`); |
Comment on lines
+142
to
+171
| export function configFingerprint(request: AgentRunRequest): string { | ||
| const workflow = request.runContext?.workflow; | ||
| const shape = { | ||
| harness: request.harness ?? null, | ||
| sandbox: request.sandbox ?? null, | ||
| model: request.model ?? null, | ||
| provider: request.provider ?? null, | ||
| connection: request.connection ?? null, | ||
| deployment: request.deployment ?? null, | ||
| endpoint: request.endpoint ?? null, | ||
| credentialMode: request.credentialMode ?? null, | ||
| agentsMd: request.agentsMd ?? null, | ||
| systemPrompt: request.systemPrompt ?? null, | ||
| appendSystemPrompt: request.appendSystemPrompt ?? null, | ||
| tools: request.tools ?? null, | ||
| skills: request.skills ?? null, | ||
| customTools: request.customTools ?? null, | ||
| mcpServers: request.mcpServers ?? null, | ||
| toolCallbackEndpoint: request.toolCallback?.endpoint ?? null, | ||
| permissions: request.permissions ?? null, | ||
| sandboxPermission: request.sandboxPermission ?? null, | ||
| harnessFiles: request.harnessFiles ?? null, | ||
| workflowRevision: workflow?.revision | ||
| ? { | ||
| id: workflow.revision.id ?? null, | ||
| version: workflow.revision.version ?? null, | ||
| } | ||
| : null, | ||
| isDraft: workflow?.is_draft ?? null, | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include the workflow artifact ID in the configuration fingerprint.
prepareEnvironmentSetup mounts request.runContext.workflow.artifact.id, but this fingerprint omits it. Changing artifacts can therefore resume an environment still mounted to the previous artifact, risking reads or writes against the wrong workspace.
Proposed fix
harnessFiles: request.harnessFiles ?? null,
+ workflowArtifactId: workflow?.artifact?.id ?? null,
workflowRevision: workflow?.revision📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function configFingerprint(request: AgentRunRequest): string { | |
| const workflow = request.runContext?.workflow; | |
| const shape = { | |
| harness: request.harness ?? null, | |
| sandbox: request.sandbox ?? null, | |
| model: request.model ?? null, | |
| provider: request.provider ?? null, | |
| connection: request.connection ?? null, | |
| deployment: request.deployment ?? null, | |
| endpoint: request.endpoint ?? null, | |
| credentialMode: request.credentialMode ?? null, | |
| agentsMd: request.agentsMd ?? null, | |
| systemPrompt: request.systemPrompt ?? null, | |
| appendSystemPrompt: request.appendSystemPrompt ?? null, | |
| tools: request.tools ?? null, | |
| skills: request.skills ?? null, | |
| customTools: request.customTools ?? null, | |
| mcpServers: request.mcpServers ?? null, | |
| toolCallbackEndpoint: request.toolCallback?.endpoint ?? null, | |
| permissions: request.permissions ?? null, | |
| sandboxPermission: request.sandboxPermission ?? null, | |
| harnessFiles: request.harnessFiles ?? null, | |
| workflowRevision: workflow?.revision | |
| ? { | |
| id: workflow.revision.id ?? null, | |
| version: workflow.revision.version ?? null, | |
| } | |
| : null, | |
| isDraft: workflow?.is_draft ?? null, | |
| }; | |
| export function configFingerprint(request: AgentRunRequest): string { | |
| const workflow = request.runContext?.workflow; | |
| const shape = { | |
| harness: request.harness ?? null, | |
| sandbox: request.sandbox ?? null, | |
| model: request.model ?? null, | |
| provider: request.provider ?? null, | |
| connection: request.connection ?? null, | |
| deployment: request.deployment ?? null, | |
| endpoint: request.endpoint ?? null, | |
| credentialMode: request.credentialMode ?? null, | |
| agentsMd: request.agentsMd ?? null, | |
| systemPrompt: request.systemPrompt ?? null, | |
| appendSystemPrompt: request.appendSystemPrompt ?? null, | |
| tools: request.tools ?? null, | |
| skills: request.skills ?? null, | |
| customTools: request.customTools ?? null, | |
| mcpServers: request.mcpServers ?? null, | |
| toolCallbackEndpoint: request.toolCallback?.endpoint ?? null, | |
| permissions: request.permissions ?? null, | |
| sandboxPermission: request.sandboxPermission ?? null, | |
| harnessFiles: request.harnessFiles ?? null, | |
| workflowArtifactId: workflow?.artifact?.id ?? null, | |
| workflowRevision: workflow?.revision | |
| ? { | |
| id: workflow.revision.id ?? null, | |
| version: workflow.revision.version ?? null, | |
| } | |
| : null, | |
| isDraft: workflow?.is_draft ?? null, | |
| }; |
mmabrouk
added a commit
that referenced
this pull request
Jul 18, 2026
Rebases the runner third of JP's feat/sessions-extensions (PR #5363) onto current main, ported into the post-decomposition module layout (PR #5369). JP wrote these against the old monolithic sandbox_agent.ts; they are re-homed by region into the decomposed modules: - The SandboxAgentDeps interface change (syncHarnessSessionDurable renamed to appendSessionTurn; the write/clear sandbox-pointer deps dropped) goes into runtime-contracts.ts. - The two acquireEnvironment branches (the reconnect-failure path and the post-hydrate pointer write, both now append-only with no pre-turn pointer PUT) go into environment.ts. - The turn-completion sync (syncHarnessSessionDurable replaced by appendSessionTurn with the streamId gate, workflow references, sandbox id, and trace id) goes into run-turn.ts. The sandbox-reconnect.ts and session-continuity-durable.ts rewrites apply as-is. protocol.ts, sessions/alive.ts, sessions/persist.ts, version.ts, and the tests are JP's versions verbatim. Ported from feat/sessions-extensions@1532ec7fe5 (JP). Reconciliations with the release main: - server.ts: kept main's session-identity / session-pool import split from the decomposition and applied JP's watchdog changes (startAliveWatchdog is now awaited and takes an onInterrupted callback that aborts the controller, request.streamId is threaded from the heartbeat, and buildPersistingEmitter gets turnId and span_id). - tracing/otel.ts, package.json, pnpm-lock.yaml: kept main's. The OTel 2.x bump JP carried is already in main, and main additionally has the #5364 trace-id-on-done feature that JP's branch predates. Verified: pnpm run typecheck clean, pnpm test 1202/1202 pass, build:extension ok. Claude-Session: https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
mmabrouk
added a commit
that referenced
this pull request
Jul 18, 2026
Rebases the runner third of JP's feat/sessions-extensions (PR #5363) onto current main, ported into the post-decomposition module layout (PR #5369). JP wrote these against the old monolithic sandbox_agent.ts; they are re-homed by region into the decomposed modules: - The SandboxAgentDeps interface change (syncHarnessSessionDurable renamed to appendSessionTurn; the write/clear sandbox-pointer deps dropped) goes into runtime-contracts.ts. - The two acquireEnvironment branches (the reconnect-failure path and the post-hydrate pointer write, both now append-only with no pre-turn pointer PUT) go into environment.ts. - The turn-completion sync (syncHarnessSessionDurable replaced by appendSessionTurn with the streamId gate, workflow references, sandbox id, and trace id) goes into run-turn.ts. The sandbox-reconnect.ts and session-continuity-durable.ts rewrites apply as-is. protocol.ts, sessions/alive.ts, sessions/persist.ts, version.ts, and the tests are JP's versions verbatim. Ported from feat/sessions-extensions@1532ec7fe5 (JP). Reconciliations with the release main: - server.ts: kept main's session-identity / session-pool import split from the decomposition and applied JP's watchdog changes (startAliveWatchdog is now awaited and takes an onInterrupted callback that aborts the controller, request.streamId is threaded from the heartbeat, and buildPersistingEmitter gets turnId and span_id). - tracing/otel.ts, package.json, pnpm-lock.yaml: kept main's. The OTel 2.x bump JP carried is already in main, and main additionally has the #5364 trace-id-on-done feature that JP's branch predates. Verified: pnpm run typecheck clean, pnpm test 1202/1202 pass, build:extension ok. Claude-Session: https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
mmabrouk
added a commit
that referenced
this pull request
Jul 18, 2026
Rebases the runner third of JP's feat/sessions-extensions (PR #5363) onto current main, ported into the post-decomposition module layout (PR #5369). JP wrote these against the old monolithic sandbox_agent.ts; they are re-homed by region into the decomposed modules: - The SandboxAgentDeps interface change (syncHarnessSessionDurable renamed to appendSessionTurn; the write/clear sandbox-pointer deps dropped) goes into runtime-contracts.ts. - The two acquireEnvironment branches (the reconnect-failure path and the post-hydrate pointer write, both now append-only with no pre-turn pointer PUT) go into environment.ts. - The turn-completion sync (syncHarnessSessionDurable replaced by appendSessionTurn with the streamId gate, workflow references, sandbox id, and trace id) goes into run-turn.ts. The sandbox-reconnect.ts and session-continuity-durable.ts rewrites apply as-is. protocol.ts, sessions/alive.ts, sessions/persist.ts, version.ts, and the tests are JP's versions verbatim. Ported from feat/sessions-extensions@1532ec7fe5 (JP). Reconciliations with the release main: - server.ts: kept main's session-identity / session-pool import split from the decomposition and applied JP's watchdog changes (startAliveWatchdog is now awaited and takes an onInterrupted callback that aborts the controller, request.streamId is threaded from the heartbeat, and buildPersistingEmitter gets turnId and span_id). - tracing/otel.ts, package.json, pnpm-lock.yaml: kept main's. The OTel 2.x bump JP carried is already in main, and main additionally has the #5364 trace-id-on-done feature that JP's branch predates. Verified: pnpm run typecheck clean, pnpm test 1202/1202 pass, build:extension ok. Claude-Session: https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
mmabrouk
added a commit
that referenced
this pull request
Jul 19, 2026
Rebases the runner third of JP's feat/sessions-extensions (PR #5363) onto current main, ported into the post-decomposition module layout (PR #5369). JP wrote these against the old monolithic sandbox_agent.ts; they are re-homed by region into the decomposed modules: - The SandboxAgentDeps interface change (syncHarnessSessionDurable renamed to appendSessionTurn; the write/clear sandbox-pointer deps dropped) goes into runtime-contracts.ts. - The two acquireEnvironment branches (the reconnect-failure path and the post-hydrate pointer write, both now append-only with no pre-turn pointer PUT) go into environment.ts. - The turn-completion sync (syncHarnessSessionDurable replaced by appendSessionTurn with the streamId gate, workflow references, sandbox id, and trace id) goes into run-turn.ts. The sandbox-reconnect.ts and session-continuity-durable.ts rewrites apply as-is. protocol.ts, sessions/alive.ts, sessions/persist.ts, version.ts, and the tests are JP's versions verbatim. Ported from feat/sessions-extensions@1532ec7fe5 (JP). Reconciliations with the release main: - server.ts: kept main's session-identity / session-pool import split from the decomposition and applied JP's watchdog changes (startAliveWatchdog is now awaited and takes an onInterrupted callback that aborts the controller, request.streamId is threaded from the heartbeat, and buildPersistingEmitter gets turnId and span_id). - tracing/otel.ts, package.json, pnpm-lock.yaml: kept main's. The OTel 2.x bump JP carried is already in main, and main additionally has the #5364 trace-id-on-done feature that JP's branch predates. Verified: pnpm run typecheck clean, pnpm test 1202/1202 pass, build:extension ok. Claude-Session: https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
mmabrouk
added a commit
that referenced
this pull request
Jul 19, 2026
Rebases the runner third of JP's feat/sessions-extensions (PR #5363) onto current main, ported into the post-decomposition module layout (PR #5369). JP wrote these against the old monolithic sandbox_agent.ts; they are re-homed by region into the decomposed modules: - The SandboxAgentDeps interface change (syncHarnessSessionDurable renamed to appendSessionTurn; the write/clear sandbox-pointer deps dropped) goes into runtime-contracts.ts. - The two acquireEnvironment branches (the reconnect-failure path and the post-hydrate pointer write, both now append-only with no pre-turn pointer PUT) go into environment.ts. - The turn-completion sync (syncHarnessSessionDurable replaced by appendSessionTurn with the streamId gate, workflow references, sandbox id, and trace id) goes into run-turn.ts. The sandbox-reconnect.ts and session-continuity-durable.ts rewrites apply as-is. protocol.ts, sessions/alive.ts, sessions/persist.ts, version.ts, and the tests are JP's versions verbatim. Ported from feat/sessions-extensions@1532ec7fe5 (JP). Reconciliations with the release main: - server.ts: kept main's session-identity / session-pool import split from the decomposition and applied JP's watchdog changes (startAliveWatchdog is now awaited and takes an onInterrupted callback that aborts the controller, request.streamId is threaded from the heartbeat, and buildPersistingEmitter gets turnId and span_id). - tracing/otel.ts, package.json, pnpm-lock.yaml: kept main's. The OTel 2.x bump JP carried is already in main, and main additionally has the #5364 trace-id-on-done feature that JP's branch predates. Verified: pnpm run typecheck clean, pnpm test 1202/1202 pass, build:extension ok. Claude-Session: https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
The runner's main engine file,
services/runner/src/engines/sandbox_agent.ts, had grown to2,477 lines. This splits it into small, well-named files and turns the original file into a thin
44-line facade that only re-exports the public surface. Nothing about what the code does changes.
This is the same decomposition that stale PR #5264 proposed, redone from scratch on current
mainso it carries every behavior the runner has gained since (the v7 security fixes, Pi transcript
persistence, fail-closed permissions, and provider-key scoping).
How to review
Read the eight commits in order, bottom to top. Each one moves one module out and is a pure
relocation: the code is identical, only its file and its import lines change. Each commit was
verified on its own before the next, so the history bisects cleanly. The tenth commit is docs only.
session-identity.tsout ofsession-pool.ts(fingerprints, credential epochs, pool keys).runtime-policy.ts(small pure policy helpers).runtime-contracts.ts(the shared interfaces and types).session-events.ts(the two session-listener routers).environment.ts(the wholeacquireEnvironmentand its lifecycle helpers).run-turn.ts(runTurn).engine.ts(runSandboxAgent+shouldPark), andsandbox_agent.tsbecomes the facade.environment-setup.tscarved out ofacquireEnvironment(the setup prefix it runs first).The public exports of
sandbox_agent.tsare unchanged, soserver.ts,cli.ts, and every testimport the engine exactly as before.
Before and after
Before: one file.
After: a facade plus focused modules.
Behavior preservation
/runcontract, the public export surface, and theruntime paths are identical.
pnpm run typecheckclean, all 1,158 runner unit tests pass, andpnpm run build:extensionsucceeds.by a separate change on its own branch (the client-tools silent-drop in
run-plan.ts).What is deferred, and why
The task also asked for two improvements on top of the decomposition: grouping the modules into
subfolders, and drawing three explicit port interfaces (sandbox provider, harness, tool delivery).
Both are deferred to a focused follow-up, for reasons written up in the handoff:
daemon.tsderives its packageroot from its own file location (three directory levels up), which controls where the Pi and
adapter binaries are found. Moving it into a subfolder silently breaks that. Grouping also can't
move
run-plan.tsright now (another change is actively editing it) and would force a renameconflict on the in-flight pi-openai PRs (feat(agents): OpenAI-compatible endpoints for Pi via vault custom connections #5345/feat(frontend): OpenAI-compatible endpoint label and harness-gated picker #5346) that edit
daytona.tsandpi-assets.ts.is exactly where a subtle behavior difference can hide, and it touches the same conflicting files.
The full port design is written up and ready to implement with a real end-to-end verification.
See
docs/design/agent-workflows/projects/runner-desloppify-redo/HANDOFF.md(the full record,including the rebase map for whoever merges this next to the pi-openai and run-plan changes) and
port-design.md(the port interface design).Do not merge yet
This is a draft for review. It is behavior-preserving but large, and the morning reviewer should
sanity-check the module boundaries and confirm the deferral decisions before merge. When it does
merge, it will need a small rebase over whichever of the pi-openai / run-plan changes land first;
the rebase map in the handoff says exactly where each moved region now lives.
https://claude.ai/code/session_01RZnMTx5et6hRo5EHN187tT