feat(platform): add durable files shared across agent sessions - #5268
Conversation
📝 WalkthroughWalkthroughAdds artifact-scoped agent mounts across the API, sandbox runner, and session inspector. The change includes deterministic mount lookup, local and remote mount setup, stronger detach safety, workflow artifact propagation, and agent-file rendering. ChangesAgent mount API
Sandbox agent mounts
Session inspector agent mounts
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SessionInspector
participant MountsAPI
participant MountsService
participant MountsDatabase
SessionInspector->>MountsAPI: POST /mounts/agents/query
MountsAPI->>MountsService: fetch agent mount
MountsService->>MountsDatabase: fetch active mount by slug
MountsDatabase-->>MountsService: mount or empty result
MountsService-->>MountsAPI: mount response
MountsAPI-->>SessionInspector: agent mount data
sequenceDiagram
participant SandboxAgent
participant MountsAPI
participant AgentMount
participant SandboxSession
SandboxAgent->>MountsAPI: sign agent mount credentials
MountsAPI-->>SandboxAgent: mount credentials
SandboxAgent->>AgentMount: mount, seed README, link agent-files
AgentMount->>SandboxSession: expose mounted path and session metadata
SandboxSession-->>SandboxAgent: session initialized
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
the
size:XXL
|
|
e2a2de7 to
e2b5d74
Compare
|
🤖 The AI agent says: @coderabbitai review |
|
✅ Action performedReview finished.
|
|
✅ Action performedReview finished.
|
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
web/oss/src/components/SessionInspector/api.ts (1)
49-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Zod boundary validation for the agent-mount response.
The coding guideline requires
safeParseWithLoggingfrom@agenta/entities/sharedat the API boundary so backend drift is caught early.fetchAgentMounttrusts the response shape without schema validation. When migrating to the Fern client, keep a local Zod schema to detect drift.As per coding guidelines: "Keep Zod validation at the API boundary even when using Fern-generated types, because local schemas still detect backend drift. Use
safeParseWithLoggingfrom@agenta/entities/sharedfor boundary validation."Source: Coding guidelines
api/oss/src/core/mounts/interfaces.py (1)
39-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the archived-exclusion contract on the interface, not just the implementation.
fetch_mount_by_slugsilently excludes archived mounts in the Postgres DAO, whilefetch_mount(by id, same interface) does not. This asymmetric contract is only documented indao.py's docstring — the abstract interface gives no hint of it, so any future DAO implementation or new caller could reasonably assume symmetric behavior withfetch_mount.📝 Proposed docstring addition
`@abstractmethod` async def fetch_mount_by_slug( self, *, project_id: UUID, # slug: str, - ) -> Optional[Mount]: ... + ) -> Optional[Mount]: + """Fetch by slug. Unlike `fetch_mount`, must exclude archived (deleted_at) rows.""" + ...api/oss/src/apis/fastapi/mounts/router.py (1)
49-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider collapsing the exception→status mapping into a table.
handle_mount_exceptionsnow has 10 near-identicalexcept ... raise HTTPException(status_code=..., detail=e.message) from eblocks, and this PR adds another one forMountArtifactIdInvalid. A{ExceptionType: status_code}dict + singleexcept MountError as elookup would remove the repetition and make future additions (e.g. this PR's) a one-line change.♻️ Proposed refactor sketch
+_MOUNT_EXCEPTION_STATUS = { + MountDataInvalid: status.HTTP_422_UNPROCESSABLE_ENTITY, + MountPathInvalid: status.HTTP_422_UNPROCESSABLE_ENTITY, + MountNameInvalid: status.HTTP_422_UNPROCESSABLE_ENTITY, + MountArtifactIdInvalid: status.HTTP_422_UNPROCESSABLE_ENTITY, + MountSlugConflict: status.HTTP_409_CONFLICT, + MountSlugReserved: status.HTTP_422_UNPROCESSABLE_ENTITY, + MountImmutableField: status.HTTP_422_UNPROCESSABLE_ENTITY, + MountFileNotFound: status.HTTP_404_NOT_FOUND, + MountNotFound: status.HTTP_404_NOT_FOUND, + MountStorageUnavailable: status.HTTP_503_SERVICE_UNAVAILABLE, +} + async def wrapper(*args, **kwargs): try: return await func(*args, **kwargs) - except MountDataInvalid as e: - raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=e.message) from e - ... (repeat for each type) ... + except MountError as e: + status_code = _MOUNT_EXCEPTION_STATUS.get(type(e), status.HTTP_400_BAD_REQUEST) + raise HTTPException(status_code=status_code, detail=e.message) from eservices/runner/src/engines/sandbox_agent.ts (2)
695-705: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSign session and agent-mount credentials in parallel.
signMount(line ~688) andsignAgentMountare independent network calls (each bounded by its own 10s timeout) but are awaited sequentially, doubling worst-case latency added toacquireEnvironmentwhen both a session mount and an agent mount apply.⚡ Proposed fix
- let mountCreds: MountCredentials | null = - presignedMount !== undefined - ? presignedMount - : sessionForMount && runCred - ? await signMount(sessionForMount, { - apiBase: apiBase(), - authorization: runCred, - log: logger, - }) - : null; - const artifactId = request.runContext?.workflow?.artifact?.id?.trim(); const signAgentMount = deps.signAgentMountCredentials ?? signAgentMountCredentials; - const agentMountCreds: MountCredentials | null = - artifactId && runCred - ? await signAgentMount(artifactId, { - apiBase: apiBase(), - authorization: runCred, - log: logger, - }) - : null; + const [mountCreds, agentMountCreds]: [ + MountCredentials | null, + MountCredentials | null, + ] = await Promise.all([ + presignedMount !== undefined + ? Promise.resolve(presignedMount) + : sessionForMount && runCred + ? signMount(sessionForMount, { + apiBase: apiBase(), + authorization: runCred, + log: logger, + }) + : Promise.resolve(null), + artifactId && runCred + ? signAgentMount(artifactId, { + apiBase: apiBase(), + authorization: runCred, + log: logger, + }) + : Promise.resolve(null), + ]);
1251-1295: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTunnel endpoint discovery can run twice per acquisition.
When both a session durable-cwd mount and an agent mount apply on Daytona,
discoverTunnelEndpointis called independently here and again at Lines 1200-1204, each an external HTTP call to the ngrok API. Since both mounts target the same object-store deployment in practice, consider discovering the tunnel endpoint once and reusing it for both mount attempts to save a round trip per run.services/runner/src/engines/sandbox_agent/session-pool.ts (1)
2-13: 🚀 Performance & Scalability | 🔵 TrivialMonitor resource impact of the new default-on keep-alive.
Flipping local keep-alive to enabled-by-default means every deployment that previously relied on the implicit off default (without explicitly setting
AGENTA_RUNNER_SESSION_KEEPALIVE) now parks warm sessions/processes up topoolMaxfor the idle TTL. Worth confirming host memory budgets andpoolMax/TTL defaults are sized for this in production before rollout, and that dashboards/alerts cover pool occupancy.Also applies to: 106-106
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df22824f-2fce-4f1f-961b-d64cc1b18ece
📒 Files selected for processing (29)
api/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/core/mounts/interfaces.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/dbs/postgres/mounts/dao.pyapi/oss/tests/pytest/acceptance/mounts/test_agent_mounts_basics.pyapi/oss/tests/pytest/unit/mounts/test_agent_mounts.pyservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/agent-mount-guidance.tsservices/runner/src/engines/sandbox_agent/agent-mount.tsservices/runner/src/engines/sandbox_agent/mount.tsservices/runner/src/engines/sandbox_agent/session-pool.tsservices/runner/src/engines/sandbox_agent/workspace.tsservices/runner/tests/unit/agent-mount.test.tsservices/runner/tests/unit/sandbox-agent-mount.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-agent-workspace.test.tsservices/runner/tests/unit/session-pool.test.tsweb/oss/src/components/AgentChatSlice/AgentChatPanel.tsxweb/oss/src/components/AgentChatSlice/components/SessionRail.tsxweb/oss/src/components/SessionInspector/PanelSessionInspectorButton.tsxweb/oss/src/components/SessionInspector/SessionInspectorButton.tsxweb/oss/src/components/SessionInspector/SessionInspectorDrawer.tsxweb/oss/src/components/SessionInspector/api.test.tsweb/oss/src/components/SessionInspector/api.tsweb/oss/src/components/SessionInspector/store.tsweb/oss/src/components/SessionInspector/tabs/MountsTab.tsxweb/packages/agenta-entities/src/workflow/state/molecule.ts
a2f87f3 to
d14e72e
Compare
dosubot
Bot
added
size:XL
ffc70be to
0bdb6ec
Compare
d14e72e to
1831f47
Compare
|
🤖 The AI agent says: Pre-merge sanity QA passed on the clean PR tip (Daytona Secret Delivery absent): Terminal write/read with no EROFS, exact agent-mount persistence, session isolation, cold cross-session recall, a real local keep-warm |
There was a problem hiding this comment.
🧹 Nitpick comments (8)
api/oss/src/core/mounts/service.py (3)
207-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant/inconsistent slugify call vs. sibling
get_or_create_session_mount.
slug_name = slugify_mount_name(name)is computed here but the rawname(notslug_name) is passed tomint_agent_slug, which slugifies again internally.get_or_create_session_mountinstead passes the already-computedslug_nameintomint_session_slug. Functionally equivalent (slugify is idempotent) but inconsistent style/duplicate work.♻️ Align with session-mount pattern
slug_name = slugify_mount_name(name) mount_create = MountCreate( - slug=mint_agent_slug(artifact_id=artifact_id, name=name), + slug=mint_agent_slug(artifact_id=artifact_id, name=slug_name), name=slug_name, )
207-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
#grouping separators between identity and payload params.Other service methods in this file (
fetch_mount,get_or_create_session_mountcall sites) group identity params (project_id/user_id) from payload params with a#separator for readability. The newget_or_create_agent_mount/fetch_agent_mountsignatures omit this.As per coding guidelines, "Prefer keyword-only parameters using
*, and use grouped sections in function signatures and calls with#separators for readability."Also applies to: 255-268
Source: Coding guidelines
207-226: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
"default"name literal duplicated across layers.The default agent-mount name
"default"is hardcoded independently inmodels.py(AgentMountQueryRequest), here (twice), and inrouter.py'sQuery(default="default"). Consider a shared constant (similar to_SESSION_CWD_NAME) to keep these in sync if the default ever changes.Also applies to: 255-268
api/oss/src/apis/fastapi/mounts/router.py (1)
313-354: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
#grouping separators in new handler calls.Sibling handlers (e.g.
create_mount) separate identity args (project_id/user_id) from payload args with a#comment for readability; the newsign_agent_mount_credentials/query_agent_mountservice calls don't follow this.As per coding guidelines, "use grouped sections in function signatures and calls with
#separators for readability."Example diff
mount = await self.mounts_service.get_or_create_agent_mount( project_id=UUID(request.state.project_id), user_id=UUID(str(request.state.user_id)), + # artifact_id=artifact_id, name=name, )Source: Coding guidelines
services/runner/src/engines/sandbox_agent.ts (2)
677-705: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSign session-mount and agent-mount credentials in parallel.
signMountandsignAgentMountare independent network calls (each with its own 10s timeout) but are awaited sequentially. On the common path where a run has both asessionIdand a workflowartifact.id, this adds unnecessary latency to every turn's environment acquisition.⚡ Proposed fix to sign both concurrently
- let mountCreds: MountCredentials | null = - presignedMount !== undefined - ? presignedMount - : sessionForMount && runCred - ? await signMount(sessionForMount, { - apiBase: apiBase(), - authorization: runCred, - log: logger, - }) - : null; - const artifactId = request.runContext?.workflow?.artifact?.id?.trim(); const signAgentMount = deps.signAgentMountCredentials ?? signAgentMountCredentials; - const agentMountCreds: MountCredentials | null = - artifactId && runCred - ? await signAgentMount(artifactId, { - apiBase: apiBase(), - authorization: runCred, - log: logger, - }) - : null; + const [mountCredsResult, agentMountCreds]: [ + MountCredentials | null, + MountCredentials | null, + ] = await Promise.all([ + presignedMount !== undefined + ? Promise.resolve(presignedMount) + : sessionForMount && runCred + ? signMount(sessionForMount, { + apiBase: apiBase(), + authorization: runCred, + log: logger, + }) + : Promise.resolve(null), + artifactId && runCred + ? signAgentMount(artifactId, { + apiBase: apiBase(), + authorization: runCred, + log: logger, + }) + : Promise.resolve(null), + ]); + let mountCreds: MountCredentials | null = mountCredsResult;
1251-1295: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant tunnel-endpoint discovery for the agent mount on Daytona.
This block re-runs
storeReachableFromSandbox/discoverTunnelEndpointindependently of the durable-cwd remote-mount block immediately above (~1194-1250), even though both mounts almost certainly point at the same underlying object-store endpoint. When a tunnel is required, this issues a second, redundant HTTP call to the tunnel-discovery API per run.Consider reusing the
endpointalready discovered for the durable cwd mount whenenvironment.mountCredsandenvironment.agentMountCredsshare the sameendpoint, falling back to a fresh discovery only otherwise.services/runner/src/engines/sandbox_agent/mount.ts (1)
394-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoot-cause mount failure is discarded if the shutdown of the failed attempt itself fails.
await attempt?.stop();is unguarded. Ifstop()throws (e.g. geesefs won't die after SIGTERM/SIGKILL — see the "refuses fallback..." test), the originalfailure(why geesefs actually failed to mount: bad creds, unreachable store, etc.) is lost; only the generic stop-failure message surfaces. That's the more actionable diagnostic for on-call debugging.♻️ Preserve original failure context when stop() also fails
// Never detach/fallback while a failed geesefs attempt may still attach later. - await attempt?.stop(); + try { + await attempt?.stop(); + } catch (stopErr) { + const origDetail = String( + failure instanceof Error ? failure.message : failure, + ).slice(0, 300); + throw new Error( + `${stopErr instanceof Error ? stopErr.message : stopErr} ` + + `(original mount failure: ${origDetail})`, + ); + } const detached = await unmountStorage(cwd, { ...deps.unmountDeps, log });services/runner/src/engines/sandbox_agent/session-pool.ts (1)
74-80: 🚀 Performance & Scalability | 🔵 TrivialDefault flip to keep-alive-on: confirm pool capacity/memory budget accounts for it.
Flipping the local-session default from off to on means idle sessions now park (holding harness process + native memory) for up to the TTL by default in every deployment that doesn't explicitly set
AGENTA_RUNNER_SESSION_KEEPALIVE=off, rather than opting in. TheboolEnv/readKeepaliveConfiglogic itself is correct and well-tested against the new default.Worth confirming
DEFAULT_POOL_MAX/host memory budgets were sized with default-on in mind across all deployment environments, not just ones that previously opted in explicitly.Also applies to: 106-106
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df22824f-2fce-4f1f-961b-d64cc1b18ece
📒 Files selected for processing (29)
api/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/core/mounts/interfaces.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/dbs/postgres/mounts/dao.pyapi/oss/tests/pytest/acceptance/mounts/test_agent_mounts_basics.pyapi/oss/tests/pytest/unit/mounts/test_agent_mounts.pyservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/agent-mount-guidance.tsservices/runner/src/engines/sandbox_agent/agent-mount.tsservices/runner/src/engines/sandbox_agent/mount.tsservices/runner/src/engines/sandbox_agent/session-pool.tsservices/runner/src/engines/sandbox_agent/workspace.tsservices/runner/tests/unit/agent-mount.test.tsservices/runner/tests/unit/sandbox-agent-mount.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-agent-workspace.test.tsservices/runner/tests/unit/session-pool.test.tsweb/oss/src/components/AgentChatSlice/AgentChatPanel.tsxweb/oss/src/components/AgentChatSlice/components/SessionRail.tsxweb/oss/src/components/SessionInspector/PanelSessionInspectorButton.tsxweb/oss/src/components/SessionInspector/SessionInspectorButton.tsxweb/oss/src/components/SessionInspector/SessionInspectorDrawer.tsxweb/oss/src/components/SessionInspector/api.test.tsweb/oss/src/components/SessionInspector/api.tsweb/oss/src/components/SessionInspector/store.tsweb/oss/src/components/SessionInspector/tabs/MountsTab.tsxweb/packages/agenta-entities/src/workflow/state/molecule.ts
0bdb6ec to
daae705
Compare
1831f47 to
18910b7
Compare
daae705 to
b6c4947
Compare
18910b7 to
51140f8
Compare
Align with big-agents: agent mounts v2 + durable files shared across sessions, mount file viewer, plus LLM-provider layout, usage/analytics and evaluator fixes. Conflict resolution: - Backend mounts (core/apis) + runner sandbox_agent + backend mounts tests: took big-agents. Our side was a cherry-pick of #5247 'for local testing' (stale preview shims, e.g. plain timing log, AGENT_MOUNT_ENV_VAR) that big-agents finalized in #5268 (agent-mounts-v2) + durable-files. session-continuity import is preserved. - FE inspector: kept our unified Inspector (AgentChatSlice/components/Inspector) and removed Mahmoud's POC mount viewer per his note — SessionInspectorDrawer/Button, tabs/MountsTab, assets/mountBrowser. Session files already surface through the Drives UI (ContextRail/FilesDrawer/DriveFileCard). Kept ours for SessionRail and PanelSessionInspectorButton (opens the unified InspectorDrawer); api/store/StatesTab/ StreamsTab stay (reused by our Inspector).
2 participants
Context
Session mounts keep files for one conversation. Agents still had nowhere to keep a note or artifact that another conversation could read.
This adds one durable S3-backed folder per agent. The folder is keyed by the workflow artifact id, shared across that agent's sessions, and visible in the Session Inspector. The PR is stacked directly on Mount File Viewer (#5204). It contains no Daytona Secret Delivery changes.
Changes
The API adds agent-mount sign and query endpoints beside the existing session-mount endpoints. Signing the same artifact id returns the same mount; different agents receive isolated mounts.
The runner mounts the durable folder separately from the session cwd and exposes it as
agent-files/. A short system-prompt segment tells the harness that the session cwd is conversation storage andagent-files/is shared agent storage. The runner keeps the author's AGENTS.md and CLAUDE.md unchanged.The Session Inspector shows one Agent files section with Session (this conversation) nested inside it. Refresh invalidates both mount metadata and cached file listings.
The QA pass also fixed two local mount failure paths. A missing FUSE device now falls back only after process termination and detach are confirmed, and util-linux exit code 32 is correctly recognized as not mounted. The runner never recursively deletes a path whose mount state is uncertain.
Tests / notes
agent-files/user-qa.txt; a different session on the same agent read the exact value. The mount API returned the same file and content.What to QA
sample.txtin the cwd. A new conversation should not see it because it belongs to the first session.agent-files/shared-notes.md. A new conversation for the same agent should read it..clauderuntime directory must not appear as agent storage.Review order
This PR should merge after #5204 because its web diff extends Mount File Viewer.