feat(platform): agent mounts, one durable folder per agent#5247
feat(platform): agent mounts, one durable folder per agent#5247mmabrouk wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds artifact-scoped agent mounts across the backend, runner, and web inspector. Mounts are deterministically keyed by artifact UUID and name, mounted into sandbox environments, exposed through API endpoints, and displayed in SessionInspector. ChangesAgent mounts
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentRunner
participant MountsAPI
participant MountStorage
participant SessionInspector
AgentRunner->>MountsAPI: Sign artifact mount credentials
MountsAPI->>MountStorage: Create or fetch deterministic mount
MountStorage-->>MountsAPI: Mount credentials or mount record
MountsAPI-->>AgentRunner: Mount response
SessionInspector->>MountsAPI: Query artifact mount
MountsAPI-->>SessionInspector: Mount record or empty result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
Design PR: #5215. This implements slices 1-3 from @coderabbitai review |
|
✅ Action performedReview finished.
|
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
A few inline notes to make the review faster. The runner wiring is the substantive part; the API and web slices follow the patterns already next to them. Happy to walk through anything.
| const signAgentMount = | ||
| deps.signAgentMountCredentials ?? signAgentMountCredentials; | ||
| const agentMountCreds: MountCredentials | null = | ||
| artifactId && runCred |
There was a problem hiding this comment.
The gate is artifactId && runCred, on purpose not sessionForMount. A run with no session still gets its agent folder. A durable per-agent mount that only worked inside a session would miss the point. runCred is the invoke caller's credential, so the sign call authenticates as them and the server resolves RBAC.
| if (agentMountDir) { | ||
| // Daytona fixes daemon env at sandbox-create, before the best-effort remote mount. Exposing | ||
| // the signed mount's stable path is safe; discovery files are seeded only after mount success. | ||
| piExtEnv[AGENT_MOUNT_ENV_VAR] = agentMountDir; |
There was a problem hiding this comment.
This line is the fix for the one real bug the review caught. The Daytona daemon reads its environment from piExtEnv, frozen when the sandbox is created a few lines below, not from the local env object (only the local daemon reads that one). So the mount dir has to land in piExtEnv here, before buildSandboxProvider. The path is deterministic (<cwd>-agent), so setting it before the mount succeeds is safe: the README and symlink are still seeded only after a real mount, so a failed remote mount just leaves the agent an empty folder.
| environment.deps.unmountStorage ?? unmountStorage | ||
| )(environment.mountedCwd, { log }).catch(() => false); | ||
| } | ||
| if (!parked && !plan.isDaytona && environment.agentMountedPath) { |
There was a problem hiding this comment.
Two guards worth a look. !plan.isDaytona: on Daytona the folder is a remote path the sandbox reclaims on destroy, so a host fusermount here would be a no-op. !parked: a keep-warm pause must leave the mount alive, the same rule the durable cwd follows just above.
| const createLink = deps.symlink ?? symlink; | ||
| const removeLink = deps.unlink ?? unlink; | ||
| const linkPath = join(cwd, AGENT_FILES_LINK_NAME); | ||
| let replaceExisting = false; |
There was a problem hiding this comment.
Why this is more than create-if-absent. geesefs makes a symlink fine while the mount is live, but does not persist it across a remount: it silently comes back as a 0-byte file. So "something exists here" is not enough to trust. We keep only a correct-target symlink and rebuild anything else, which hands the agent a working link on every run.
| this same derivation byte-identically so they address the same mount. | ||
| """ | ||
| try: | ||
| canonical_artifact_id = UUID(str(artifact_id)) |
There was a problem hiding this comment.
Sign and query both derive the slug from the artifact id, so if the two derivations ever differed by a byte, a real mount would read as absent in the UI. Parsing to a UUID and lowercasing keeps them identical. It also turns a non-UUID id into a clean 422 here, instead of a 500 later when the slug validator rejects it.
dosubot
Bot
added
size:XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
web/oss/src/components/SessionInspector/api.ts (1)
49-57: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMissing Zod validation at the API boundary.
The coding guidelines require: "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 so structured errors are logged without crashing." No validation is performed on the response from/mounts/agents/query.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 so structured errors are logged without crashing."🛡️ Suggested validation addition
export async function fetchAgentMount(artifactId: string, projectId?: string | null) { const res = await axios.post<Awaited<ReturnType<typeof fetchMounts>>>( `${getAgentaApiUrl()}/mounts/agents/query`, {artifact_id: artifactId}, {params: {project_id: projectId}, _ignoreError: true} as Record<string, unknown>, ) - return res.data.mounts?.[0] ?? null + const parsed = safeParseWithLogging(agentMountQueryResponseSchema, res.data) + return parsed?.mounts?.[0] ?? null }Where
agentMountQueryResponseSchemawould be a Zod schema matching{count: number, mounts: Mount[]}.Source: Coding guidelines
services/runner/src/engines/sandbox_agent/agent-mount.ts (1)
92-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated error-formatting logic across the file.
The pattern
String(err instanceof Error ? err.message : err).slice(0, N)appears seven times with only the truncation length varying. Extracting a smallformatErr(err, max = 200)helper would remove the duplication and make truncation length consistent.♻️ Proposed refactor
+function formatErr(err: unknown, max = 200): string { + return String(err instanceof Error ? err.message : err).slice(0, max); +}Then replace each occurrence, e.g.:
- `sign failed artifact=${artifactId}: ${String(err instanceof Error ? err.message : err).slice(0, 160)}`, + `sign failed artifact=${artifactId}: ${formatErr(err, 160)}`,Also applies to: 115-121, 147-151, 183-188, 195-199, 205-209, 233-236
services/runner/tests/unit/agent-mount.test.ts (2)
128-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
seedAgentReadme's non-EEXIST failure log.Only the atomic-write and EEXIST-no-op paths are tested; the branch where
writeFilethrows something other thanEEXIST(which should calllog) has no test, unlike the symmetric coverage given tolinkAgentFiles's error branches below.
154-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
linkAgentFiles's unlink-failure log branch.Every other branch of
linkAgentFiles(missing/valid/degraded/wrong-target/lstat-failure/EEXIST/symlink-failure) is tested, but the case whereunlinkthrows a non-ENOENTerror (lines 194-199 inagent-mount.ts, the "agent-files unlink failed" log) isn't exercised.docs/design/agent-workflows/projects/agent-mounts/plan.md (1)
50-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
The code block at line 50 has no language tag, which triggers markdownlint MD040.
📝 Proposed fix
- +```text POST /mounts/agents/sign?artifact_id=<uuid>&name=default permission: RUN_SESSIONS POST /mounts/agents/query {"artifact_id": "<uuid>", "name": "default"} permission: VIEW_SESSIONSSource: Linters/SAST tools
services/runner/src/engines/sandbox_agent.ts (1)
1289-1333: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate tunnel-discovery call per Daytona turn.
discoverTunnelEndpoint()is invoked here for the agent mount, and separately for the durable cwd remote mount a few lines earlier (~1240). Both mounts are signed against the samemounts_store(seeMountsService.sign_mount_credentials), so their endpoints are effectively identical in practice — yet an unreachable store triggers two independent HTTP round-trips to the ngrok API on every single Daytona turn instead of one.♻️ Suggested approach
Hoist the reachability/tunnel-endpoint resolution once (e.g., right before the durable-cwd remote-mount block) and reuse the resolved
endpointfor the agent-mount block too, falling back to a fresh discovery only if the two stores' endpoints actually differ.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4e961ff-51d7-4fa2-b35f-5d4bd42d2c42
📒 Files selected for processing (25)
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.pydocs/design/agent-workflows/projects/agent-mounts/open-issues.mddocs/design/agent-workflows/projects/agent-mounts/plan.mddocs/design/agent-workflows/projects/agent-mounts/research.mddocs/design/agent-workflows/projects/agent-mounts/status.mdservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/agent-mount.tsservices/runner/tests/unit/agent-mount.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
| // Fern has no query_agent_mount yet; migrate after client regeneration. | ||
| export async function fetchAgentMount(artifactId: string, projectId?: string | null) { | ||
| const res = await axios.post<Awaited<ReturnType<typeof fetchMounts>>>( | ||
| `${getAgentaApiUrl()}/mounts/agents/query`, | ||
| {artifact_id: artifactId}, | ||
| {params: {project_id: projectId}, _ignoreError: true} as Record<string, unknown>, | ||
| ) | ||
| return res.data.mounts?.[0] ?? null | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Raw axios usage violates the Fern-only API guideline.
The coding guidelines explicitly require: "All new frontend API code must use the Fern-generated client (getAgentaSdkClient({host: getAgentaApiUrl()})) instead of raw axios." While the comment documents that the Fern client lacks this endpoint, this still violates the guideline. Please track the migration explicitly (e.g., a linked issue) and ensure the Fern client is regenerated promptly.
As per coding guidelines: "All new frontend API code must use the Fern-generated client (getAgentaSdkClient({host: getAgentaApiUrl()})) instead of raw axios."
📝 Suggested tracking comment
-// Fern has no query_agent_mount yet; migrate after client regeneration.
+// TODO(migration): Fern has no query_agent_mount yet; migrate to getAgentaSdkClient
+// after client regeneration. Tracked in issue #<TBD>.Source: Coding guidelines
There was a problem hiding this comment.
Deliberate for now: the Fern client has no agent-mount endpoint yet, which the code comment notes. Tracked in open-issues.md on design PR #5215 to migrate once the client is regenerated. Keeping the raw call (with _ignoreError: true so a failed fetch does not pop a global toast) until then.
There was a problem hiding this comment.
@mmabrouk, understood — thanks for the context. Deferring the migration until the Fern client is regenerated makes sense, and having it tracked in open-issues.md alongside design PR #5215 is sufficient for now. The _ignoreError: true handling is a reasonable stopgap too. No further action needed on this from my side.
mmabrouk
force-pushed
the
feat/agent-mounts
branch
from
ebb9d79 to
217d141
Compare
Live QA on Daytona: passedRan an agent workflow twice on Daytona (eu.preview key
The env-var check is the one the review flagged. It passed two ways: the var is set in the live sandbox's environment, and Daytona's own stored sandbox record echoes it, which proves it rode Unrelated infra bug found along the way (not this PR)The EE dev runner boots Daytona sandboxes from the wrong snapshot: its compose service reads plain |
Self-healing discovery symlink (geesefs drops symlinks across remount), the Daytona env-var delivery fix, live QA results (incl. Daytona verified), and a new open-issues log. Implementation is PR #5247. Claude-Session: https://claude.ai/code/session_01UogudBC9VRCRkpT5bZVxex
mmabrouk
force-pushed
the
feat/agent-mounts
branch
from
217d141 to
0255e0c
Compare
dosubot
Bot
added
size:XL
mmabrouk
force-pushed
the
feat/agent-mounts
branch
from
0255e0c to
fc0e857
Compare
…egment
On a natural "remember this for next time" prompt, an agent had no way to
know its cwd is throwaway and agent-files/ is durable, so it wrote to its
own session-scoped memory and the note was lost next session.
Append a short platform guidance segment to the harness's SYSTEM PROMPT
(never the author's CLAUDE.md/AGENTS.md) whenever an agent mount exists
for the run:
- Pi: combined into plan.appendSystemPrompt, rendered via the existing
APPEND_SYSTEM.md file channel.
- Claude: passed as the ACP session's _meta.systemPrompt.append, which
claude-agent-acp forwards into the claude-agent-sdk's
{type:"preset",preset:"claude_code",append} option - additive to the
default system prompt, so CLAUDE.md loading is unaffected.
Verified live: a fresh session writes the note into agent-files/notes.md
(confirmed durable via the mounts API after the FUSE cache clears), and a
cold new session correctly recalls it by checking agent-files/ first.
The session mount lives inside the agent's durable folder (the runner symlinks it in as agent-files/), so present it that way in the Session Inspector's Mounts tab: the agent mount is the parent Collapse panel, and the session mount is a nested section inside it, instead of two sibling sections. Falls back to the flat session-only view when there is no artifact/agent mount to nest under. Presentation only - the runtime mount layout is unchanged, and both mounts keep their own file-listing queries.
|
QA Bot seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
2 participants
Context
An agent has nowhere to keep a file between runs. Session mounts (#5197) give each
conversation a scratch folder, but that folder dies with the session. So a skill the agent
writes, a note it takes, or an artifact it builds is gone the moment the conversation ends.
This gives every agent one durable folder, backed by S3, that outlives every run and every
session. It is keyed by the agent's artifact id, mounted into the sandbox next to the working
directory, and browsable in the Session Inspector. No database migration. No
/runprotocolchange. No agent-template change.
What the agent sees
The runner mounts the folder at
<cwd>-agentand makes it findable without editing theauthor's
agent.md. An agent that lists its working directory now sees:Open the link and the folder holds a
README.mdthat states the one rule: files here persistacross every run and session; the working directory does not. The daemon also gets
AGENTA_AGENT_MOUNT_DIRpointing at the same path. Three signals, zero instructions text.How it works
feat(api)): two endpoints beside the session-mount ones.signmints a reservedslug from the artifact id, upserts the mount, and returns scoped S3 credentials.
querylooks one up and never creates it.
feat(runner)): a newagent-mount.tsmodule, plus wiring insandbox_agent.tsthat mirrors the durable session-cwd lifecycle. On every run that carries an artifact id, the
runner signs, mounts at
<cwd>-agent, seeds the README, linksagent-files, exposes the envvar, and unmounts on teardown.
feat(web)): an "Agent files" panel in the Session Inspector's Mounts tab.What to look at as a reviewer
I left inline comments on the parts that are easy to misread. The three that carry the most risk:
id, an unconfigured store, a failed sign, or a geesefs hiccup logs and continues. The durable
session cwd keeps its exact behavior; the agent mount only adds to it.
bug here: the Daytona daemon reads its environment from
piExtEnvat sandbox-create time, notfrom the local
envobject. The inline note walks through the fix.silently degrades to an empty file. So the runner rebuilds the link each run, and the env var
and README carry the durable signal.
Tests
api/:uv run --no-sync pytest oss/tests/pytest/unit/mounts oss/tests/pytest/acceptance/mounts -q.services/runner/:pnpm vitest run tests/unit/agent-mount.test.ts.Full runner suite regression-clean.
src/components/SessionInspector/api.test.ts.remount. Data persists; sign is idempotent per agent and isolated across agents.
symlink, cross-run persistence, and
AGENTA_AGENT_MOUNT_DIRreaching the Daytona daemon allpassed. See the QA comment below for the details.
What to QA
POST /mounts/agents/sign?artifact_id=<uuid>&name=defaultwith a RUN_SESSIONSkey returns credentials scoped to your project. Sign the same id again and you get the same
mount; sign a different id and you get a different one. A non-UUID id returns 422.
panel lists the folder's contents, or shows an empty state when the folder is new.
About the base branch
This is the code. The design docs and open-issues log live in the design PR #5215.
The branch depends on two branches that have not merged and that no single branch combines: the
web panel builds on #5204, and the runner wiring sits on the runner changes in #5244. To hold the
diff to the 21 files that are actually mine, the base
agent-mounts-baseis a snapshot of thecurrent integrated workspace. Review against it, but do not merge against it. Once #5204 and
#5244 land, this rebases onto the trunk.
Claude-Session: https://claude.ai/code/session_01UogudBC9VRCRkpT5bZVxex