Skip to content

[feat] Resume agent sessions across the sandbox lifecycle#5197

Open
jp-agenta wants to merge 18 commits into
big-agentsfrom
feat/sessions-continuity
Open

[feat] Resume agent sessions across the sandbox lifecycle#5197
jp-agenta wants to merge 18 commits into
big-agentsfrom
feat/sessions-continuity

Conversation

@jp-agenta

@jp-agenta jp-agenta commented Jul 10, 2026

Copy link
Copy Markdown
Member

Context

Until now, every agent turn started from scratch. The runner created a fresh sandbox, called session/new, and replayed the entire conversation as one flattened text prompt (the "cold replay"). Long sessions got slower and more expensive with every turn, and the harness lost its native session memory each time.

This PR gives sessions continuity. The runner now remembers which sandbox a session ran in and which native harness session it produced. On the next turn it reconnects to that sandbox and asks the harness to load its own saved session (ACP session/load). When that works, the runner sends only the new user message instead of the whole transcript. Every rung of this ladder degrades gracefully; the worst case is exactly today's behavior (fresh sandbox, cold replay).

The lifecycle

A session's sandbox moves through five states, driven by three Daytona idle timers plus an explicit "park" at the end of each clean turn:

State Meaning Entered by Next turn does
Hot Sandbox running: a turn in flight, or a live session parked in the local keep-alive pool turn running, or pool park resume the live session, send only the new message
Warm Daytona sandbox stopped, disk kept explicit pause at clean turn end, or 5 idle min (DAYTONA_AUTOSTOP) restart by stored sandbox id, session/load, send only the new message
Cold Sandbox archived to cold storage 15 idle min (DAYTONA_AUTOARCHIVE) same as warm, slower restore
Dead Sandbox deleted 30 idle min (DAYTONA_AUTODELETE), or destroy on abort fresh sandbox; the durable transcript mounts still let session/load resume; else cold replay
New First turn, nothing stored first turn of a session fresh sandbox, session/new

Before: Daytona sandboxes were created ephemeral: true with a 15-minute auto-stop that deleted them. Every turn was effectively "new".

Changes

Runner: continuity state (services/runner/src/engines/sandbox_agent/)

  • session-continuity.ts (new): in-memory store mapping (sessionId, harness) to {agentSessionId, turnIndex}, plus a per-session latestTurnIndex. The core policy is the staleness guard: a harness may session/load only if it authored the most recent completed turn. This is what makes switching between Pi and Claude inside one session safe; the harness that got switched away from replays cold once, then resumes natively.
  • session-continuity-durable.ts (new): mirrors that store into the API's session_states.data row so a runner restart does not lose continuity. It hydrates on session setup and syncs after each completed turn (fire and forget, GET then PUT).
  • sandbox-reconnect.ts (new): reads and writes session_states.sandbox_id. This field changes meaning. Before it held a display label (the provider name, e.g. "local"). Now it holds the actual Daytona instance id used to reconnect.

Runner: engine integration (sandbox_agent.ts)

  • Reconnect ladder: stored sandbox id, try to restart it, fall back to a fresh create on any failure.
  • session/load instead of session/new when an eligible native session exists; only then does the turn send just the new user text (RunTurnOptions.loaded).
  • destroy({keepWarm}): only clean successful turns park warm; failed turns and aborts destroy the sandbox. keepWarm is gated on shouldPark() which checks the turn result, client connection, and abort signal.
  • Per-harness transcript mounts: ~/.claude/projects and Pi's sessions dir get the same durable geesefs mount treatment as the session cwd, so even a dead sandbox can resume natively. The Claude mount deliberately covers only the transcript dir, never ~/.claude/.credentials.json.
  • Local multi-runner guard: a local session owned by another replica fails loudly instead of silently cold-starting on the wrong box. The guard is advisory and fail-open: claim errors and unknown owners pass.
  • Remote mount smarts: public S3 stores are mounted directly without a tunnel; in-network stores use the tunnel endpoint. Geesefs runs backgrounded on remote (no -f flag) with a liveness poll that verifies the mount serves I/O before returning.

Runner: provider and vendored patch

  • provider.ts: ephemeral: false plus the stop/archive/delete timers (defaults 5/15/30 minutes, overridable via DAYTONA_AUTOSTOP / DAYTONA_AUTOARCHIVE / DAYTONA_AUTODELETE).
  • patches/sandbox-agent@0.4.2.patch (new): teaches the vendored sandbox-agent to try ACP session/load (with Claude's resume-by-id option in _meta) before its existing text-replay + session/new path, and kills the daemon's whole process group so ACP subprocesses stop leaking to PID 1.

API (api/oss/src/)

Small and surgical: no new tables, no migrations, no new endpoints.

  • session_states.data goes from an opaque dict to a typed contract. Before: {"anything": ...}. After:
    {"latest_agent_session_id": "...", "latest_turn_index": 3,
     "harness_sessions": {"claude": {"agent_session_id": "...", "turn_index": 3}}}
    Same JSON column, no schema change; the server never queries these fields.
  • Heartbeat ownership. Before, any replica's heartbeat could steal a session (refresh-if-owner, else overwrite; last writer wins). After, one atomic Redis Lua claim that never steals from a live different owner, and the heartbeat response gains a required replica_id field reporting who actually holds the session. The runner compares it to its own id and refuses to serve if it lost.
  • GET /sessions/mounts/sign gains a name query param (default "cwd") so the runner signs the per-harness transcript mounts through the same RUN_SESSIONS permission check.

Infra, docs, CI

  • Runner Dockerfile renamed to Dockerfile.gh; both images now copy patches/ so pnpm can apply the sandbox-agent patch. CI workflow and the Railway build script point at the new path.
  • Compose files: all 7 now forward DAYTONA_AUTOSTOP / DAYTONA_AUTOARCHIVE / DAYTONA_AUTODELETE to the runner container (were documented but not wired). Ngrok tunnel moved from remote profile to with-tunnel (shared with composio); it exits 0 on missing NGROK_AUTHTOKEN and binds the agent API on all interfaces so the runner can discover the tunnel URL.
  • New env vars documented in env examples, Helm, and the self-host docs (02-configuration.mdx, guide 09 gains a "Sandbox lifecycle" section). AGENTA_SESSION_HARNESS_MOUNTS=false disables the transcript mounts.

Generated clients (Fern, Python and TS)

The sessions delta above, plus a catch-up sweep: the tools-surface changes (new list_tool_categories, the category filter, the needs_approval field removal) come from API changes that were already on big-agents with stale generated clients. They are correct against the current API but are not part of this feature.

Tests

About 1,300 lines of new runner unit tests (continuity store, durable mirror, reconnect, lifecycle policy, ownership) and new API tests (owner claim, DTO mapping, a live-stack acceptance suite). Two drive-bys ride along: a hermeticity fix so ambient AGENTA_INSECURE_EGRESS_ALLOWED / DAYTONA_* shell vars cannot flip SSRF test assertions (new conftest pins plus an allow_insecure_env marker), and fixes for sdk/api tests that were already red on big-agents.

Decisions and tradeoffs

  • Two-tier continuity store. The hot path reads process memory; the API row is only restart recovery. Every durable failure degrades to cold replay, never an error. The cost is two sources of truth, reconciled by "live record wins" and a monotonic max() on the shared turn counter.
  • Turn-index staleness guard instead of locking for multi-harness correctness. No cross-harness coordination needed; a stale harness simply cold-replays once.
  • Native Daytona timers instead of ephemeral delete. This buys the warm and cold rungs. The cost: leaked sandboxes now live 30 minutes instead of 15, and stopped/archived disks bill until deletion. Note the auto-stop default also dropped from 15 to 5 minutes.
  • Ownership is advisory and fail-open by design: it exists to fail loudly on a mis-routed multi-replica local deployment, not to be a distributed lock. "Never worse than today" wins over strictness.
  • A pnpm dist patch instead of forking sandbox-agent. Surgical and hash-pinned, but it patches minified dist/ code and embeds a Claude-specific _meta resume contract, so it is fragile across upstream bumps.
  • Transcripts are mounted, not copied in and out. Simpler and always current, at the cost of FUSE write amplification on append-heavy JSONL files. Credentials are deliberately excluded from the mounted set.

Scope / risk

  • session-continuity.ts is flagged binary by git. The file contains literal NUL bytes (used as a map-key separator), so GitHub renders no diff and accepts no inline comments on the PR's most central new file. Replace the raw bytes with the "\^@" escape (identical behavior) so the file is reviewable, here and in every future PR that touches it.
  • The durable write path has no concurrency guard. The continuity sync is GET-then-PUT of the whole data object, and writeSandboxId PUTs against the same row. Two concurrent writers can drop a harness entry. The failure mode is a silent cold replay, not corruption, but the PUT semantics (field-patch vs row-replace) need confirming on the API side.
  • Legacy rows. Old sandbox_id values hold provider names ("local"), so a migrated session attempts one doomed reconnect, logs, and self-heals by overwriting. Old opaque data blobs validate away silently; a type collision makes the row read as "no state" via a suppressed exception.
  • Kill does not clear the Redis owner key, and clear_owner now has zero callers. Kill a session on replica A and restart it on replica B within 120 s, and B refuses to serve until the TTL expires. The old steal-happy code did not have this window.
  • 5-minute auto-stop vs long approval waits. If Daytona counts a paused HITL prompt as idle, the sandbox stops underneath a live turn. The old default was 15 minutes.
  • Deploy order: API before runner. The new runner requires replica_id in the heartbeat response and name on mounts/sign; an old API silently signs the wrong mount.
  • Not touched: web UI, SDK sources (test-only changes), and transcript persistence for local (non-Daytona) sandboxes.

Tests / CI status

  • Unit coverage pins the policy layer well: the staleness guard (including the double harness switch), durable hydrate/sync semantics, reconnect helpers, park-vs-destroy, lifecycle timer parsing, mount selection, the non-stealing owner claim on both sides of the wire, and remote mount liveness polling.
  • The known gaps, in order of importance: (1) the engine's actual session/load rung (eligible id found, load attempted, fallback on failure) has no test; (2) the acceptance suite: 14 tests pass against a live stack built from this branch.
  • CI: all runner, API, SDK, and services jobs are green. run-web-unit-tests fails with 22 failures in @agenta/entities, but the identical 22 fail on the big-agents base, so they predate this branch. This PR actually fixes the API and SDK unit jobs that are red on base.

Commands:

cd services/runner && pnpm test                # runner unit suites
cd api && uv run --no-sync python -m pytest oss/tests/pytest/unit/session_states oss/tests/pytest/unit/sessions -q
# acceptance (needs a live stack built from THIS branch):
cd api && uv run --no-sync python -m pytest oss/tests/pytest/acceptance/session_states -q

How to review

Suggested reading order (inline comments mark the decision points):

  1. Check out the branch locally first: session-continuity.ts renders as binary on GitHub (see Scope / risk), and it is the file everything else orbits. The policy functions (staleness guard, ownership) are pure and short.
  2. session-continuity-durable.ts and sandbox-reconnect.ts: the persistence contract against session_states.
  3. sandbox_agent.ts: the integration. The interesting spots are the ownership guard (~line 591), the reconnect ladder (~930), the session/load bridge (~1093-1158), the post-turn record (~1687), and keepWarm gated on shouldPark() (~1884).
  4. provider.ts and mount.ts: the lifecycle timers and the transcript mounts. mount.ts includes the store reachability logic that avoids tunneling public S3 endpoints.
  5. The API side: dbs/redis/sessions/{contract,locks}.py and core/sessions/streams/service.py for the claim; core/sessions/states/dtos.py for the data contract.
  6. patches/sandbox-agent@0.4.2.patch: two hunks, session/load support and the process-group kill.
  7. Tests last; the inline comments flag what they do and do not pin. Generated clients and hosting files are mechanical.

How to QA

Prerequisites: EE dev stack with Daytona credentials (SANDBOX_AGENT_DAYTONA_*), runner image rebuilt from this branch (the pnpm patch changes the install, so a plain restart is not enough).

Steps:

  1. In the playground, run an agent turn on a Daytona sandbox session.
  2. Send a second turn right away. Expected: runner logs show a reconnect to the same sandbox id and a session/load; the agent remembers turn 1 natively and the prompt contains only the new message.
  3. Wait more than 5 idle minutes (the sandbox auto-stops), then send another turn. Expected: warm restart of the same sandbox id, memory intact.
  4. Delete the sandbox in the Daytona console, then send another turn. Expected: a fresh sandbox that still resumes from the mounted transcript, or falls back to cold replay without an error.
  5. Switch harness mid-session (Pi to Claude) and back. Expected: the switched-to harness cold-replays once, then resumes natively on later turns.

Edge cases to watch: kill a session and restart it immediately (the 120 s owner TTL window); two local runners against one session (must fail loudly, not cold-start).

@dosubot dosubot Bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Jul 10, 2026
@vercel

vercel Bot commented Jul 10, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
agenta-documentation Ready Ready Preview, Comment Jul 10, 2026 4:59pm

Request Review

@dosubot dosubot Bot added the Backend label Jul 10, 2026
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.86% which is insufficient. The required threshold is 60.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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 matches the main change: resuming agent sessions across the sandbox lifecycle.
Description check ✅ Passed The description is highly relevant and accurately summarizes the continuity, lifecycle, and runner changes in the PR.
✨ 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 feat/sessions-continuity

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

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

Brings in #5185 (Pi approval parking over the ACP permission plane).

Conflicts in services/runner/src/{engines/sandbox_agent.ts,server.ts} were
comment-only: #5185 rewords the parkable-gate doc blocks to cover the Pi ACP
plane alongside Claude ACP, and adds piToolSpecsByName / onPiGateAllowed above
one of them. Took the incoming wording; verified statically that no executable
line from either side was dropped. The S5 sandbox lifecycle (reconnect,
park-to-warm) and the Pi gate envelope coexist untouched.

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 The AI agent says:

I found three P1 and two P2 issues. The patch can corrupt active-session liveness, reconnect native state containing incomplete turns, and lose durable continuity updates under overlapping syncs. It can also park failed remote runs and collapse distinct mount names onto one durable mount.

Please address the inline findings before merge.

Comment thread api/oss/src/core/sessions/streams/service.py
Comment thread services/runner/src/engines/sandbox_agent.ts
"content-type": "application/json",
authorization: deps.authorization,
},
body: JSON.stringify({

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🤖 The AI agent says:

P2: Make durable continuity updates atomic.

With overlapping syncs for the same session, both calls can read the same old row and then replace the entire data object. Whichever PUT finishes last drops the other harness or turn and can regress the durable record. The fire-and-forget callers make this possible even for rapid sequential turns, so a restart may resume an older native session. Serialize these writes or perform a server-side atomic or CAS merge.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Confirmed and staying open. Two overlapping fire-and-forget syncs both GET the old row and PUT the whole data object; last writer wins and drops the other harness's harness_sessions entry.

The wrinkle is that the state update is a full PUT by design, not a partial patch, so a client-side merge can never be made safe no matter how it is ordered. The fix has to move server-side, which means either a CAS/if-match on the existing endpoint or a purpose-built merge endpoint. That is a design call rather than a patch, so I would rather land it deliberately than bolt it on here.

Your note on writeSandboxId checks out: the DAO's model_fields_set guard means it only overwrites explicitly-set fields, so it does not clobber data. That part is safe today.

Comment thread services/runner/src/server.ts
Comment thread api/oss/src/core/mounts/service.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 11

🧹 Nitpick comments (4)
api/oss/tests/pytest/unit/sessions/test_owner_claim.py (1)

71-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Execute the ownership script in a real Lua-capable Redis test.

This fake branches on script equality and then duplicates the intended behavior, so invalid Lua or Redis-specific regressions still pass. Add at least one integration test using Redis or a Lua-enabled fakeredis instance.

api/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.py (1)

8-11: 🎯 Functional Correctness | 🔵 Trivial

Acceptance tests shipped unrun.

The module docstring admits these tests were never executed against a live stack. Given the cross-file finding that SessionStateData sub-fields not explicitly set get persisted as null (see DAO's model_dump(mode="json") without exclude_unset/exclude_none), it's worth actually running these against a live stack before merge to confirm the round-trip/merge assertions hold.

Do you want me to help set up a quick local run against the docker-compose dev stack (per the coding guidelines' run.sh script) to validate these before merge?

services/runner/tests/unit/continuation.test.ts (1)

78-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise the production prompt-selection logic.

This local helper duplicates the implementation, so the suite passes independently of runTurn. Extract the decision into an exported helper or invoke runTurn with a minimal fake environment.

services/runner/tests/unit/session-ownership.test.ts (1)

148-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Return the advertised owner values before triggering the sentinel.

Both tests throw inside the resolver, so assertLocalRunnerOwnership never receives undefined or the same replica ID. Return those values and inject a failing startSandboxAgent seam afterward to prove the guard accepted them.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c581377-e623-4b3f-bb60-41023d0a9cef

📥 Commits

Reviewing files that changed from the base of the PR and between 41eacd0 and f19ab8e.

⛔ Files ignored due to path filters (19)
  • services/runner/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
  • web/packages/agenta-api-client/src/generated/api/resources/sessions/client/Client.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/SessionRecordIngestRequest.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/SignSessionMountCredentialsRequest.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/tools/client/Client.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/tools/client/requests/ListToolCategoriesRequest.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/tools/client/requests/ListToolIntegrationsRequest.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/tools/client/requests/index.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/BuiltinToolConfig.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/CapabilitiesResult.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/GatewayToolConfig.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/HarnessSessionRecord.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/SessionHeartbeatResponseModel.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/SessionState.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/SessionStateData.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/SessionStateUpsertRequest.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/ToolCatalogCategoriesResponse.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/ToolCatalogCategory.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/index.ts is excluded by !**/generated/**
📒 Files selected for processing (81)
  • .github/workflows/42-railway-build.yml
  • api/ee/tests/pytest/conftest.py
  • api/ee/tests/pytest/unit/services/test_organizations_oidc_ssrf.py
  • api/oss/src/apis/fastapi/sessions/models.py
  • api/oss/src/apis/fastapi/sessions/router.py
  • api/oss/src/core/mounts/service.py
  • api/oss/src/core/sessions/states/dtos.py
  • api/oss/src/core/sessions/streams/dtos.py
  • api/oss/src/core/sessions/streams/service.py
  • api/oss/src/dbs/postgres/sessions/states/dao.py
  • api/oss/src/dbs/postgres/sessions/states/mappings.py
  • api/oss/src/dbs/redis/sessions/contract.py
  • api/oss/src/dbs/redis/sessions/locks.py
  • api/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.py
  • api/oss/tests/pytest/acceptance/session_states/test_session_states_basics.py
  • api/oss/tests/pytest/conftest.py
  • api/oss/tests/pytest/unit/session_states/test_harness_sessions_mapping.py
  • api/oss/tests/pytest/unit/sessions/test_owner_claim.py
  • api/oss/tests/pytest/utils/egress.py
  • api/pytest.ini
  • clients/python/agenta_client/__init__.py
  • clients/python/agenta_client/sessions/client.py
  • clients/python/agenta_client/sessions/raw_client.py
  • clients/python/agenta_client/tools/client.py
  • clients/python/agenta_client/tools/raw_client.py
  • clients/python/agenta_client/tools/types/tool_resolve_request_tools_item.py
  • clients/python/agenta_client/types/__init__.py
  • clients/python/agenta_client/types/builtin_tool_config.py
  • clients/python/agenta_client/types/capabilities_result.py
  • clients/python/agenta_client/types/gateway_tool_config.py
  • clients/python/agenta_client/types/harness_session_record.py
  • clients/python/agenta_client/types/session_heartbeat_response_model.py
  • clients/python/agenta_client/types/session_state.py
  • clients/python/agenta_client/types/session_state_data.py
  • clients/python/agenta_client/types/session_state_upsert_request.py
  • clients/python/agenta_client/types/tool_catalog_categories_response.py
  • clients/python/agenta_client/types/tool_catalog_category.py
  • docs/docs/self-host/02-configuration.mdx
  • docs/docs/self-host/guides/08-custom-agent-runner-images.mdx
  • docs/docs/self-host/guides/09-agent-daytona-sandboxes.mdx
  • hosting/docker-compose/ee/docker-compose.gh.local.yml
  • hosting/docker-compose/ee/env.ee.dev.example
  • hosting/docker-compose/ee/env.ee.gh.example
  • hosting/docker-compose/oss/docker-compose.gh.local.yml
  • hosting/docker-compose/oss/docker-compose.gh.ssl.yml
  • hosting/docker-compose/oss/env.oss.dev.example
  • hosting/docker-compose/oss/env.oss.gh.example
  • hosting/kubernetes/helm/templates/runner-deployment.yaml
  • hosting/railway/oss/scripts/build-and-push-images.sh
  • sdks/python/oss/tests/pytest/conftest.py
  • sdks/python/oss/tests/pytest/unit/agents/connections/test_capabilities.py
  • sdks/python/oss/tests/pytest/unit/agents/test_dtos_agent_template.py
  • sdks/python/oss/tests/pytest/unit/test_secrets_manager_custom_provider_ssrf.py
  • sdks/python/oss/tests/pytest/unit/test_utils_net_ssrf.py
  • sdks/python/oss/tests/pytest/utils/test_webhook_ssrf_v0.py
  • sdks/python/pytest.ini
  • services/runner/docker/Dockerfile.dev
  • services/runner/docker/Dockerfile.gh
  • services/runner/docker/README.md
  • services/runner/package.json
  • services/runner/patches/sandbox-agent@0.4.2.patch
  • services/runner/src/engines/sandbox_agent.ts
  • services/runner/src/engines/sandbox_agent/mount.ts
  • services/runner/src/engines/sandbox_agent/provider.ts
  • services/runner/src/engines/sandbox_agent/sandbox-reconnect.ts
  • services/runner/src/engines/sandbox_agent/session-continuity-durable.ts
  • services/runner/src/engines/sandbox_agent/session-continuity.ts
  • services/runner/src/engines/sandbox_agent/session-pool.ts
  • services/runner/src/server.ts
  • services/runner/src/sessions/alive.ts
  • services/runner/tests/setup/hermetic-env.ts
  • services/runner/tests/unit/continuation.test.ts
  • services/runner/tests/unit/sandbox-agent-mount.test.ts
  • services/runner/tests/unit/sandbox-agent-provider.test.ts
  • services/runner/tests/unit/sandbox-lifecycle.test.ts
  • services/runner/tests/unit/sandbox-reconnect.test.ts
  • services/runner/tests/unit/session-continuity-durable.test.ts
  • services/runner/tests/unit/session-continuity.test.ts
  • services/runner/tests/unit/session-ownership.test.ts
  • services/runner/tests/unit/session-pool.test.ts
  • services/runner/vitest.config.ts
💤 Files with no reviewable changes (3)
  • clients/python/agenta_client/tools/types/tool_resolve_request_tools_item.py
  • clients/python/agenta_client/types/builtin_tool_config.py
  • clients/python/agenta_client/types/gateway_tool_config.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 11

🧹 Nitpick comments (4)
api/oss/tests/pytest/unit/sessions/test_owner_claim.py (1)

71-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Execute the ownership script in a real Lua-capable Redis test.

This fake branches on script equality and then duplicates the intended behavior, so invalid Lua or Redis-specific regressions still pass. Add at least one integration test using Redis or a Lua-enabled fakeredis instance.

api/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.py (1)

8-11: 🎯 Functional Correctness | 🔵 Trivial

Acceptance tests shipped unrun.

The module docstring admits these tests were never executed against a live stack. Given the cross-file finding that SessionStateData sub-fields not explicitly set get persisted as null (see DAO's model_dump(mode="json") without exclude_unset/exclude_none), it's worth actually running these against a live stack before merge to confirm the round-trip/merge assertions hold.

Do you want me to help set up a quick local run against the docker-compose dev stack (per the coding guidelines' run.sh script) to validate these before merge?

services/runner/tests/unit/continuation.test.ts (1)

78-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Exercise the production prompt-selection logic.

This local helper duplicates the implementation, so the suite passes independently of runTurn. Extract the decision into an exported helper or invoke runTurn with a minimal fake environment.

services/runner/tests/unit/session-ownership.test.ts (1)

148-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Return the advertised owner values before triggering the sentinel.

Both tests throw inside the resolver, so assertLocalRunnerOwnership never receives undefined or the same replica ID. Return those values and inject a failing startSandboxAgent seam afterward to prove the guard accepted them.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c581377-e623-4b3f-bb60-41023d0a9cef

📥 Commits

Reviewing files that changed from the base of the PR and between 41eacd0 and f19ab8e.

⛔ Files ignored due to path filters (19)
  • services/runner/pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
  • web/packages/agenta-api-client/src/generated/api/resources/sessions/client/Client.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/SessionRecordIngestRequest.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/SignSessionMountCredentialsRequest.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/tools/client/Client.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/tools/client/requests/ListToolCategoriesRequest.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/tools/client/requests/ListToolIntegrationsRequest.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/resources/tools/client/requests/index.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/BuiltinToolConfig.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/CapabilitiesResult.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/GatewayToolConfig.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/HarnessSessionRecord.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/SessionHeartbeatResponseModel.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/SessionState.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/SessionStateData.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/SessionStateUpsertRequest.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/ToolCatalogCategoriesResponse.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/ToolCatalogCategory.ts is excluded by !**/generated/**
  • web/packages/agenta-api-client/src/generated/api/types/index.ts is excluded by !**/generated/**
📒 Files selected for processing (81)
  • .github/workflows/42-railway-build.yml
  • api/ee/tests/pytest/conftest.py
  • api/ee/tests/pytest/unit/services/test_organizations_oidc_ssrf.py
  • api/oss/src/apis/fastapi/sessions/models.py
  • api/oss/src/apis/fastapi/sessions/router.py
  • api/oss/src/core/mounts/service.py
  • api/oss/src/core/sessions/states/dtos.py
  • api/oss/src/core/sessions/streams/dtos.py
  • api/oss/src/core/sessions/streams/service.py
  • api/oss/src/dbs/postgres/sessions/states/dao.py
  • api/oss/src/dbs/postgres/sessions/states/mappings.py
  • api/oss/src/dbs/redis/sessions/contract.py
  • api/oss/src/dbs/redis/sessions/locks.py
  • api/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.py
  • api/oss/tests/pytest/acceptance/session_states/test_session_states_basics.py
  • api/oss/tests/pytest/conftest.py
  • api/oss/tests/pytest/unit/session_states/test_harness_sessions_mapping.py
  • api/oss/tests/pytest/unit/sessions/test_owner_claim.py
  • api/oss/tests/pytest/utils/egress.py
  • api/pytest.ini
  • clients/python/agenta_client/__init__.py
  • clients/python/agenta_client/sessions/client.py
  • clients/python/agenta_client/sessions/raw_client.py
  • clients/python/agenta_client/tools/client.py
  • clients/python/agenta_client/tools/raw_client.py
  • clients/python/agenta_client/tools/types/tool_resolve_request_tools_item.py
  • clients/python/agenta_client/types/__init__.py
  • clients/python/agenta_client/types/builtin_tool_config.py
  • clients/python/agenta_client/types/capabilities_result.py
  • clients/python/agenta_client/types/gateway_tool_config.py
  • clients/python/agenta_client/types/harness_session_record.py
  • clients/python/agenta_client/types/session_heartbeat_response_model.py
  • clients/python/agenta_client/types/session_state.py
  • clients/python/agenta_client/types/session_state_data.py
  • clients/python/agenta_client/types/session_state_upsert_request.py
  • clients/python/agenta_client/types/tool_catalog_categories_response.py
  • clients/python/agenta_client/types/tool_catalog_category.py
  • docs/docs/self-host/02-configuration.mdx
  • docs/docs/self-host/guides/08-custom-agent-runner-images.mdx
  • docs/docs/self-host/guides/09-agent-daytona-sandboxes.mdx
  • hosting/docker-compose/ee/docker-compose.gh.local.yml
  • hosting/docker-compose/ee/env.ee.dev.example
  • hosting/docker-compose/ee/env.ee.gh.example
  • hosting/docker-compose/oss/docker-compose.gh.local.yml
  • hosting/docker-compose/oss/docker-compose.gh.ssl.yml
  • hosting/docker-compose/oss/env.oss.dev.example
  • hosting/docker-compose/oss/env.oss.gh.example
  • hosting/kubernetes/helm/templates/runner-deployment.yaml
  • hosting/railway/oss/scripts/build-and-push-images.sh
  • sdks/python/oss/tests/pytest/conftest.py
  • sdks/python/oss/tests/pytest/unit/agents/connections/test_capabilities.py
  • sdks/python/oss/tests/pytest/unit/agents/test_dtos_agent_template.py
  • sdks/python/oss/tests/pytest/unit/test_secrets_manager_custom_provider_ssrf.py
  • sdks/python/oss/tests/pytest/unit/test_utils_net_ssrf.py
  • sdks/python/oss/tests/pytest/utils/test_webhook_ssrf_v0.py
  • sdks/python/pytest.ini
  • services/runner/docker/Dockerfile.dev
  • services/runner/docker/Dockerfile.gh
  • services/runner/docker/README.md
  • services/runner/package.json
  • services/runner/patches/sandbox-agent@0.4.2.patch
  • services/runner/src/engines/sandbox_agent.ts
  • services/runner/src/engines/sandbox_agent/mount.ts
  • services/runner/src/engines/sandbox_agent/provider.ts
  • services/runner/src/engines/sandbox_agent/sandbox-reconnect.ts
  • services/runner/src/engines/sandbox_agent/session-continuity-durable.ts
  • services/runner/src/engines/sandbox_agent/session-continuity.ts
  • services/runner/src/engines/sandbox_agent/session-pool.ts
  • services/runner/src/server.ts
  • services/runner/src/sessions/alive.ts
  • services/runner/tests/setup/hermetic-env.ts
  • services/runner/tests/unit/continuation.test.ts
  • services/runner/tests/unit/sandbox-agent-mount.test.ts
  • services/runner/tests/unit/sandbox-agent-provider.test.ts
  • services/runner/tests/unit/sandbox-lifecycle.test.ts
  • services/runner/tests/unit/sandbox-reconnect.test.ts
  • services/runner/tests/unit/session-continuity-durable.test.ts
  • services/runner/tests/unit/session-continuity.test.ts
  • services/runner/tests/unit/session-ownership.test.ts
  • services/runner/tests/unit/session-pool.test.ts
  • services/runner/vitest.config.ts
💤 Files with no reviewable changes (3)
  • clients/python/agenta_client/tools/types/tool_resolve_request_tools_item.py
  • clients/python/agenta_client/types/builtin_tool_config.py
  • clients/python/agenta_client/types/gateway_tool_config.py
🛑 Comments failed to post (11)
api/oss/src/dbs/postgres/sessions/states/mappings.py (1)

9-15: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Use is not None instead of truthy check for dbe.data.

The DAO serializes with if upsert.data is not None (explicit None check), but the mapping uses if dbe.data (truthy check). An empty dict {} from legacy data or manual DB edits would be falsy, returning None instead of SessionStateData() with default fields — a semantic difference downstream code may not expect.

🔧 Consistency fix
 def dbe_to_dto(dbe: SessionStateDBE) -> SessionState:
-    data = SessionStateData.model_validate(dbe.data) if dbe.data else None
+    data = SessionStateData.model_validate(dbe.data) if dbe.data is not None else None
     return SessionState(
📝 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.

def dbe_to_dto(dbe: SessionStateDBE) -> SessionState:
    data = SessionStateData.model_validate(dbe.data) if dbe.data is not None else None
    return SessionState(
        id=dbe.id,
        project_id=dbe.project_id,
        session_id=dbe.session_id,
        data=data,
api/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.py (1)

16-19: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm how the "acceptance" marker gets applied to tests under this directory.
rg -n "acceptance" api/oss/tests/pytest/conftest.py api/pytest.ini
rg -n "pytest_collection_modifyitems|acceptance" -g '*.py' api/oss/tests

Repository: Agenta-AI/agenta

Length of output: 1421


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the test file, nearby conftest files, and pytest config that could apply markers or skips.
git ls-files api/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.py \
             api/oss/tests/pytest/acceptance/conftest.py \
             api/oss/tests/pytest/conftest.py \
             api/oss/tests/pytest/unit/conftest.py \
             api/oss/tests/legacy/conftest.py \
             api/pytest.ini

for f in \
  api/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.py \
  api/oss/tests/pytest/acceptance/conftest.py \
  api/oss/tests/pytest/conftest.py \
  api/oss/tests/pytest/unit/conftest.py \
  api/oss/tests/legacy/conftest.py \
  api/pytest.ini
do
  echo "===== $f ====="
  [ -f "$f" ] && sed -n '1,220p' "$f" || echo "(missing)"
done

Repository: Agenta-AI/agenta

Length of output: 17268


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look for any explicit acceptance markers or path-based marker application.
rg -n "`@pytest`\.mark\.acceptance|mark\.acceptance|acceptance" api/oss/tests/pytest -g '*.py'

# Inspect the shared API fixtures that the acceptance test relies on.
rg -n "def authed_api|authed_api" api/oss/tests -g '*.py'

Repository: Agenta-AI/agenta

Length of output: 50373


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the authed/unauthed fixtures and any collection hooks that mention acceptance or env gating.
rg -n "def authed_api|def unauthed_api|AGENTA_API_URL|AGENTA_AUTH_KEY|pytest_collection_modifyitems|mark\.acceptance|acceptance" api/oss/tests -g '*.py' -g '!**/__pycache__/**'

# Show the fixture definitions if they exist.
for f in \
  api/oss/tests/utils/api.py \
  api/oss/tests/pytest/utils/api.py \
  api/oss/tests/utils/env.py \
  api/oss/tests/pytest/utils/env.py
do
  [ -f "$f" ] && { echo "===== $f ====="; sed -n '1,220p' "$f"; }
done

Repository: Agenta-AI/agenta

Length of output: 4503


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find the actual fixture module(s) and inspect only the relevant sections.
fd -a 'api.py$|env.py$' api/oss/tests api/oss -t f

# Narrow down to any acceptance-specific collection or skip logic around the test tree.
rg -n "pytest_collection_modifyitems|skipif|mark\.skip|mark\.acceptance|AGENTA_API_URL|AGENTA_AUTH_KEY" api/oss/tests/pytest api/oss/tests/utils -g '*.py'

Repository: Agenta-AI/agenta

Length of output: 3587


🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path

# Print only the files likely to define fixtures or collection hooks.
candidates = [
    Path("api/oss/tests/utils/api.py"),
    Path("api/oss/tests/utils/env.py"),
    Path("api/oss/tests/pytest/conftest.py"),
    Path("api/oss/tests/pytest/acceptance/conftest.py"),
    Path("api/oss/tests/pytest/unit/conftest.py"),
    Path("api/oss/tests/legacy/conftest.py"),
]
for p in candidates:
    if p.exists():
        print(f"===== {p} =====")
        text = p.read_text()
        for i, line in enumerate(text.splitlines(), 1):
            if any(key in line for key in ["authed_api", "unauthed_api", "AGENTA_API_URL", "AGENTA_AUTH_KEY", "pytest_collection_modifyitems", "acceptance", "skipif", "mark.acceptance"]):
                print(f"{i}: {line}")
PY

Repository: Agenta-AI/agenta

Length of output: 511


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the fixture chain that provides cls_account and see whether it already gates on live env.
sed -n '1,260p' api/oss/tests/pytest/utils/accounts.py

# Also show any direct references from conftest imports to the shared fixtures module.
sed -n '1,120p' api/oss/tests/pytest/conftest.py

Repository: Agenta-AI/agenta

Length of output: 2895


Add the acceptance marker on this class. pytest.ini only declares the marker; it doesn’t apply it, and there’s no local hook doing so here. Without an explicit marker or collection filter, these tests can still be collected in runs that don’t set AGENTA_API_URL/AGENTA_AUTH_KEY, and ag_env will fail during fixture setup.

services/runner/patches/sandbox-agent@0.4.2.patch (1)

93-130: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

sed -n '1,240p' services/runner/patches/sandbox-agent@0.4.2.patch
rg -n "killGroup|detached: true|registerProcessCleanup|child.kill|process.kill\\(" services/runner/patches/sandbox-agent@0.4.2.patch

Repository: Agenta-AI/agenta

Length of output: 5595


🏁 Script executed:

sed -n '1,240p' dist/providers/local.js
rg -n "process.platform|win32|detached: true|killGroup|child.kill\\(" dist/providers/local.js services/runner/patches/sandbox-agent@0.4.2.patch

Repository: Agenta-AI/agenta

Length of output: 968


🏁 Script executed:

git ls-files | rg 'sandbox-agent|providers/local|runner|package.json|README|pnpm-workspace.yaml|workspace'

Repository: Agenta-AI/agenta

Length of output: 30600


🏁 Script executed:

rg -n "spawnSandboxAgent|killGroup|detached: true|process.kill\\(-child\\.pid|child\\.kill\\(" services/runner/src services/runner/README.md services/runner/package.json services/runner/AGENTS.md services/runner/CLAUDE.md

Repository: Agenta-AI/agenta

Length of output: 233


🏁 Script executed:

sed -n '260,340p' services/runner/src/engines/sandbox_agent/mount.ts
rg -n "win32|darwin|linux|process group|detached: true|kill\\(|SIGTERM|SIGKILL" services/runner/src/engines/sandbox_agent services/runner/README.md services/runner/package.json services/runner/AGENTS.md services/runner/CLAUDE.md

Repository: Agenta-AI/agenta

Length of output: 4481


🌐 Web query:

Node.js process.kill negative pid child.kill detached process group Windows documentation

💡 Result:

In Node.js, using a negative PID with process.kill to target a process group ID does not work on Windows [1]. The concept of process groups as implemented in POSIX systems (where a negative PID represents a process group) is not supported in the same way on Windows [1]. When you spawn a child process on Windows with the detached: true option, Node.js does not create a process group that can be signaled via a negative PID [1][2]. Consequently, Node.js cannot use process.kill or child.kill to terminate an entire tree of processes on Windows [3]. To terminate a process tree (including detached child processes and their descendants) on Windows, the recommended approach is to use the Windows-native taskkill command, often executed via child_process.exec or a similar mechanism [1][3]. Specifically, the command: taskkill /pid /T /F is commonly used, where /T terminates the process tree (the process and any child processes it started) and /F forces the termination [3]. Third-party libraries like fkill are also frequently used to handle these cross-platform differences automatically [1].

Citations:


Fall back to child.kill(signal) when group signaling fails. killGroup swallows process.kill(-pid, ...) errors, and this runner includes a win32-x64 build where negative-PID signaling is unavailable. As written, cleanup can become a no-op and leave the sandbox daemon running; calling child.kill(signal) in the catch path avoids that.

services/runner/src/engines/sandbox_agent.ts (2)

946-954: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Only mark the sandbox resumable after storing its reconnect ID.

Line 954 becomes true even without credentials or when the fire-and-forget write fails. Teardown then parks a sandbox that the next turn cannot locate, causing a cold start and leaving the warm sandbox orphaned until auto-delete. Await a success result from writeSandboxId and gate resumable on it.


1767-1772: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Destroy failed runs instead of parking their sandbox.

runTurn returns { ok: false } for errors and run-limit trips, so this finally still uses keepWarm: true. The next turn can reconnect the same wedged sandbox. Capture the result and only keep it warm when result.ok is true.

Proposed fix
+  let result: AgentRunResult | undefined;
   try {
-    return await runTurn(env, request, emit, signal, {
+    result = await runTurn(env, request, emit, signal, {
       loaded: env.loadedFromContinuity,
     });
+    return result;
   } finally {
-    await env.destroy({ keepWarm: env.resumable && !signal?.aborted });
+    await env.destroy({
+      keepWarm: Boolean(result?.ok && env.resumable && !signal?.aborted),
+    });
   }
📝 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.

  let result: AgentRunResult | undefined;
  try {
    result = await runTurn(env, request, emit, signal, {
      loaded: env.loadedFromContinuity,
    });
    return result;
  } finally {
    // Park a remote session-owned sandbox to warm on a clean turn-end; an aborted run destroys.
    await env.destroy({
      keepWarm: Boolean(result?.ok && env.resumable && !signal?.aborted),
    });
services/runner/src/engines/sandbox_agent/sandbox-reconnect.ts (1)

33-36: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n -C3 'AbortSignal\.timeout|new AbortController|fetchWith.*Timeout' services/runner/src

Repository: Agenta-AI/agenta

Length of output: 2560


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## outline sandbox-reconnect.ts\n'
ast-grep outline services/runner/src/engines/sandbox_agent/sandbox-reconnect.ts --view expanded || true

printf '\n## relevant file contents\n'
nl -ba services/runner/src/engines/sandbox_agent/sandbox-reconnect.ts | sed -n '1,220p'

printf '\n## doFetch definitions/usages\n'
rg -n -C3 '\bdoFetch\b|fetch\(' services/runner/src/engines services/runner/src/tools services/runner/src/server.ts

Repository: Agenta-AI/agenta

Length of output: 604


🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n -C4 'function doFetch|const doFetch|async function doFetch|export .*doFetch|doFetch\(' services/runner/src | sed -n '1,220p'

Repository: Agenta-AI/agenta

Length of output: 8579


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## sandbox-reconnect.ts\n'
sed -n '1,220p' services/runner/src/engines/sandbox_agent/sandbox-reconnect.ts

printf '\n## session-continuity-durable.ts relevant section\n'
sed -n '1,220p' services/runner/src/engines/sandbox_agent/session-continuity-durable.ts

printf '\n## fetchImpl usage in sandbox_agent\n'
rg -n -C2 'fetchImpl\??:|fetchImpl\s*=' services/runner/src/engines/sandbox_agent

Repository: Agenta-AI/agenta

Length of output: 11975


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n## sandbox-reconnect.ts with line numbers\n'
sed -n '1,140p' services/runner/src/engines/sandbox_agent/sandbox-reconnect.ts | cat -n

printf '\n## AbortSignal/fetch timeout usage in sandbox_agent files\n'
rg -n -C2 'AbortSignal\.timeout|AbortController|signal:' services/runner/src/engines/sandbox_agent

Repository: Agenta-AI/agenta

Length of output: 4025


Add a timeout to both reconnect requests. The GET and PUT both use bare fetch, so a stalled Daytona API can hang the reconnect path indefinitely. Reuse the runner’s existing abort/timeout pattern here.

services/runner/src/engines/sandbox_agent/session-continuity-durable.ts (2)

124-131: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Abort synchronization when the state GET fails.

A transient 401/503 currently becomes {}, followed by a whole-object PUT that can erase every previously persisted harness record. Missing rows are already represented by a successful response with session_state: null; non-2xx responses should not write.

Proposed fix
     const getRes = await doFetch(
       `${base}/sessions/states/?session_id=${encodeURIComponent(sessionId)}`,
       { method: "GET", headers: { authorization: deps.authorization } },
     );
-    const existingData: SessionStateDataWire = getRes.ok
-      ? (((await getRes.json()) as { session_state?: SessionStateWire | null })
-          .session_state?.data ?? {})
-      : {};
+    if (!getRes.ok) {
+      log(`sync GET HTTP ${getRes.status} session=${sessionId} — skipping write`);
+      return;
+    }
+    const existingData: SessionStateDataWire =
+      ((await getRes.json()) as { session_state?: SessionStateWire | null })
+        .session_state?.data ?? {};
📝 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.

    const getRes = await doFetch(
      `${base}/sessions/states/?session_id=${encodeURIComponent(sessionId)}`,
      { method: "GET", headers: { authorization: deps.authorization } },
    );
    if (!getRes.ok) {
      log(`sync GET HTTP ${getRes.status} session=${sessionId} — skipping write`);
      return;
    }
    const existingData: SessionStateDataWire =
      ((await getRes.json()) as { session_state?: SessionStateWire | null })
        .session_state?.data ?? {};

142-163: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Keep the latest session ID aligned with the latest turn.

When the durable row is already at turn 5 and a delayed turn 2 sync arrives, the code keeps latest_turn_index: 5 but replaces latest_agent_session_id with turn 2’s ID. Preserve both existing latest fields unless this write advances the counter.

Proposed fix
-    const latestTurnIndex = Math.max(
-      existingData.latest_turn_index ?? -1,
-      turnIndex,
-    );
+    const previousLatestTurnIndex = existingData.latest_turn_index ?? -1;
+    const latestTurnIndex = Math.max(previousLatestTurnIndex, turnIndex);
+    const latestAgentSessionId =
+      turnIndex >= previousLatestTurnIndex
+        ? agentSessionId
+        : existingData.latest_agent_session_id;

     const res = await doFetch(
@@
-            latest_agent_session_id: agentSessionId,
+            latest_agent_session_id: latestAgentSessionId,
📝 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.

    // Never regress the cross-harness counter: another harness may already have persisted a
    // higher latest turn. Take the max so the durable `latest_turn_index` is monotonic.
    const previousLatestTurnIndex = existingData.latest_turn_index ?? -1;
    const latestTurnIndex = Math.max(previousLatestTurnIndex, turnIndex);
    const latestAgentSessionId =
      turnIndex >= previousLatestTurnIndex
        ? agentSessionId
        : existingData.latest_agent_session_id;

    const res = await doFetch(
      `${base}/sessions/states/?session_id=${encodeURIComponent(sessionId)}`,
      {
        method: "PUT",
        headers: {
          "content-type": "application/json",
          authorization: deps.authorization,
        },
        body: JSON.stringify({
          data: {
            ...existingData,
            latest_agent_session_id: latestAgentSessionId,
            latest_turn_index: latestTurnIndex,
            harness_sessions: merged,
          },
services/runner/src/engines/sandbox_agent/session-continuity.ts (1)

177-197: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Fail closed when the ownership claim is unavailable.

Line 188 states that undefined represents a network/HTTP claim failure, but Lines 181 and 195 allow the run. During a coordination outage, multiple replicas can therefore cold-start the same local session—the exact split-brain this guard is intended to prevent. A successful first claim should return this replica’s ID; unavailable ownership should raise a distinct coordination error.

services/runner/src/sessions/alive.ts (1)

88-126: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the target file around the cited lines
sed -n '1,220p' services/runner/src/sessions/alive.ts | cat -n

printf '\n--- references ---\n'

# Find assertLocalRunnerOwnership and ownerReplicaId usages
rg -n "assertLocalRunnerOwnership|ownerReplicaId|claimSessionOwnership" services/runner/src -S

Repository: Agenta-AI/agenta

Length of output: 9842


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the ownership guard and its call site
sed -n '560,700p' services/runner/src/engines/sandbox_agent.ts | cat -n

printf '\n--- ownership helpers ---\n'
sed -n '300,370p' services/runner/src/engines/sandbox_agent.ts | cat -n

Repository: Agenta-AI/agenta

Length of output: 11154


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find and inspect assertLocalRunnerOwnership implementation
rg -n "function assertLocalRunnerOwnership|const assertLocalRunnerOwnership|assertLocalRunnerOwnership\(" services/runner/src/engines/sandbox_agent.ts -n -S

printf '\n--- definition slice ---\n'
sed -n '370,460p' services/runner/src/engines/sandbox_agent.ts | cat -n

Repository: Agenta-AI/agenta

Length of output: 4873


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the ownership assertion and related logic across the repo
rg -n "assertLocalRunnerOwnership" services/runner/src -S
printf '\n--- broader owner/claim references ---\n'
rg -n "ownerReplicaId|replicaId.*owner|unknown owner|wrong-host|cold-start" services/runner/src -S

Repository: Agenta-AI/agenta

Length of output: 2121


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Search the entire repository for the assertion definition and usage
rg -n "assertLocalRunnerOwnership" .
printf '\n--- claimSessionOwnership call sites ---\n'
rg -n "claimSessionOwnership\(" .

Repository: Agenta-AI/agenta

Length of output: 2218


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '180,240p' services/runner/tests/unit/session-continuity.test.ts | cat -n

printf '\n--- claim tests ---\n'
sed -n '1,160p' services/runner/tests/unit/session-ownership.test.ts | cat -n

printf '\n--- ownership test tail ---\n'
sed -n '160,260p' services/runner/tests/unit/session-ownership.test.ts | cat -n

Repository: Agenta-AI/agenta

Length of output: 12509


Fail closed when the owner claim is unavailable services/runner/src/sessions/alive.ts:97-124
HTTP/network errors return ownerReplicaId: undefined, and assertLocalRunnerOwnership treats that as eligible. During an API outage, multiple local replicas can cold-start the same session and mutate it concurrently. Return a claim failure or retry with a bounded timeout instead.

services/runner/tests/unit/session-ownership.test.ts (1)

215-228: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Prevent this unit test from starting a real Daytona sandbox.

With no startSandboxAgent override, the test proceeds into production sandbox startup after proving the resolver was skipped. This can hang, require credentials, or provision external infrastructure. Inject the same fail-fast startup seam used above.

@mmabrouk mmabrouk changed the title [fix] Resolve sessions continuity [feat] Resume agent sessions across the sandbox lifecycle Jul 10, 2026

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a review-readability pass on this PR: I rewrote the description (context, lifecycle, decisions, risks, reading order, QA plan) and left the inline notes below at the decision points. The notes explain what each piece does, why, and what to scrutinize. They are guidance for reviewers, not change requests.

One finding that cannot be an inline comment because GitHub will not render the file: services/runner/src/engines/sandbox_agent/session-continuity.ts contains literal NUL bytes (the map-key separator in key() and the prefix check in clear() are raw 0x00 characters), so git flags the file as binary. The PR's most central new file shows as Bin 0 -> 8125 bytes: no diff, no inline comments, and every future change to it will be equally un-reviewable. The runtime behavior is fine; please replace the raw byte with the escape sequence "\u0000" (or a plain separator like "\n") so the file becomes reviewable text. Until then, read it locally; the PR description's "How to review" section has a suggested reading order.

Comment thread services/runner/src/engines/sandbox_agent.ts
Comment thread services/runner/src/engines/sandbox_agent.ts
Comment thread services/runner/src/engines/sandbox_agent.ts
Comment thread services/runner/src/engines/sandbox_agent.ts Outdated
Comment thread services/runner/src/engines/sandbox_agent/provider.ts
Comment thread api/oss/src/apis/fastapi/sessions/router.py
Comment thread services/runner/tests/unit/continuation.test.ts Outdated
Comment thread services/runner/tests/unit/sandbox-lifecycle.test.ts
Comment thread api/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.py Outdated
Comment thread hosting/docker-compose/oss/env.oss.dev.example
- compose: forward DAYTONA_AUTOSTOP/AUTOARCHIVE/AUTODELETE to the runner
  service in all 7 compose files. They were documented in every env example
  but never wired, so setting them was a no-op (helm already wired them).
- sandbox_agent: export sendLastMessageOnly() and call it from runTurn, so
  continuation.test.ts pins the shipped decision instead of a local copy.
- session_states acceptance: drop the "NOT RUN" note; 14 pass against a live
  stack built from this branch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@mmabrouk mmabrouk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backend lgtm This PR has been approved by a maintainer size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants