Skip to content

Stage Managed Agents scratch files inside the workspace - #2221

Closed
nkurraDO wants to merge 3 commits into
yc-software:mainfrom
nkurraDO:managed-agents-workspace-scratch
Closed

nkurraDO wants to merge 3 commits into
yc-software:mainfrom
nkurraDO:managed-agents-workspace-scratch

Conversation

@nkurraDO

@nkurraDO nkurraDO commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Fixes the /tmp vs /workspace failures reported against the do-managed-agents backend added in #2128.

The problem

Managed Agents draws two different boundaries, and they don't agree:

  • Exec is unconfined. A command can write anywhere in the guest, /tmp included.
  • File transfer is confined to /workspace. Anything else is refused with 403 path outside workspace.

Shared backend code assumes /tmp is a safe place for a scratch file. That assumption holds for every other backend and breaks here the moment the file has to be transferred rather than merely written. Because the scratch file is created by exec, it appears to work; the failure surfaces later, at the upload or download.

Two paths were broken:

  • prepareExecutionFiles stages per-command credentials in a /tmp directory and uploads each file into it, so any command needing a credential failed.
  • createExecExport tars the computer into /tmp and reads the archive back, so export failed.

Verified against a live sandbox: exec writes and reads /tmp/probe.txt fine, a transfer of the same path returns 403, a ../tmp/... traversal is also refused, and a /workspace path succeeds.

The fix

Backends now declare where their scratch files belong. scratchDir defaults to /tmp, so every other backend is byte-identical, and Managed Agents declares /workspace/.qm-scratch — a path exec and transfer both see. It sits outside /workspace/home, so credentials still stay out of the home tar and the export.

Transfer paths are validated at the backend's own chokepoint too, so a caller that reaches outside /workspace gets a local error naming the constraint and the offending path rather than a bare remote 403:

do-managed-agents transfers files only under /workspace, so it cannot reach
/tmp/qm-credentials.LUfCNUKhhM/home/.aws/credentials. Temporary files this
backend has to read back or write belong in /workspace/.qm-scratch, which
exec and transfer both see.

This also drops a hardcoded slice(5) in the credential sweep that assumed the scratch prefix was exactly /tmp/.

Why the tests didn't catch it

The fake answered a transfer for any path by falling through to the host filesystem, so writing /tmp in a test succeeded against the machine running the suite while the same call 403s in production. The double was more permissive than the platform it stands in for, which is exactly how this shipped.

The fake now fails outside-/workspace transfers the way the platform fails them, maps guest paths onto a per-session root instead of the host, and maps command output back, so a path printed by mktemp is a guest path the caller can actually transfer.

New tests cover both broken paths, the local guard, the guest refusing outside-/workspace transfers, and the stale-credential sweep actually clearing an old credential directory from the scratch directory. Reverting execute-files.ts to main fails the staging and sweep tests.

Verification

  • managed-agents-sandbox 38/38, managed-agents-client 26/26, execute-credential-preparation 11/11
  • execute-file-credentials 15/16 — the one failure needs GNU touch -d and fails identically on main from macOS
  • Lint clean, no new typecheck errors

Follow-up cleanup

  • The export's scratch directory now comes from the backend profile (scratchDirFor(profile)). Before, the profile and createExecExport each declared it separately.
  • ManagedAgentsOutsideWorkspaceError is folded into transferPath, which throws a plain error that still names the path and the constraint.
  • Dropped tests that only re-asserted a constant or string-matched the generated shell script. The sweep test now plants a three-hour-old credential directory and checks that it is removed.

Live QA

Ran on 2026-10-15 against real DigitalOcean Managed Agents sessions (template qm-sandbox), using this PR's backend through createSdkManagedAgentsClient. Each session was destroyed afterwards.

main (before): credential staging fails with 7 PERMISSION_DENIED: path outside workspace: "/tmp/qm-credentials.…/home/.aws/credentials". Export fails the same way on /tmp/agent-computer-export.….

This PR at 98c103d4:

  • scratchDir resolves to /workspace/.qm-scratch.

  • A three-hour-old qm-credentials.* directory planted in the scratch dir is gone after prepareExecutionFiles (the stale sweep works on the real guest).

  • A file credential stages at /workspace/.qm-scratch/qm-credentials.<id>/home/.aws/credentials. A command run with the prepared env, the way execute merges it, sees HOME and AWS_SHARED_CREDENTIALS_FILE pointing there and reads the file back (exit 0).

  • finish() removes the credential directory.

  • exportFiles({ include: ["workspace"] }) returns the workspace file with the right contents.

  • End-to-end publish on a dev instance (in-process core from scripts/api-livetest.ts, SANDBOX_BACKEND=do-managed-agents, DEPLOY_PROVIDER=docker, real model turn): the agent wrote a two-file Node app on the Managed Agents computer and called apps publish. At this PR the publish returned v1, the container came up, and it served the app's page. On main the same turn fails with [publish failed] do-managed-agents download /tmp/agent-computer-export.… failed: 7 PERMISSION_DENIED: path outside workspace, and no deployment is created.

Config matrix and schema

No settings, env vars, or schema changes. scratchDir is an optional, code-level backend profile property that defaults to /tmp.

Wall of shame review report

Reviewed commit: 98c103d428cab0a0d10a17ad06a57c7e0ca2b263

Lenses were triaged by claude-haiku-5-5. Each relevant lens was then reviewed by a separate fresh-context claude-opus-5-5 reviewer that read the diff, the full changed files, and this description.

Lens Reviewer Verdict Score Assessment
Overengineering claude-opus-5-5 (independent reviewer) Accept 76/100 The core fix is proportionate: one optional scratchDir profile field, defaulting to /tmp, threaded into the two broken call sites, with no settings or schema growth. It also removes the brittle slice(5). There is minor flab worth trimming but not blocking: the transferPath local guard duplicates a 403 the platform already returns, scratchDirFor(profile) in managed-agents just re-reads a constant set three lines above, and createExecExport takes a separate defaulted scratchDir param alongside the profile field.
Band-aid fixes claude-opus-5-5 (independent reviewer) Accept 84/100 This fixes the cause, not the symptom. The shared code's hidden assumption that /tmp is transferable is replaced by a backend-declared scratchDir, which both prepareExecutionFiles and createExecExport consume via scratchDirFor(profile), and the hardcoded slice(5) is removed rather than worked around. The transferPath guard rejects bad paths at the backend's single transfer chokepoint instead of adding path rewrites or special-case branches, and the over-permissive fake that hid the bug is corrected rather than patched around.
Duplication claude-opus-5-5 (independent reviewer) Accept 84/100 The scratch location has one owner: DEFAULT_SCRATCH_DIR and scratchDirFor(profile) in sandbox.ts. Both the credential staging and the managed-agents export read it from the profile instead of declaring /tmp twice, and the hardcoded slice(5) is gone. Two small leftovers: createExecExport keeps its own scratchDir default rather than taking the profile, and the home-snapshot io still calls session.read/writeFileBytes directly instead of going through the transferPath chokepoint. Neither causes drift today.
Hand-rolling claude-opus-5-5 (independent reviewer) Accept 88/100 The PR adds an optional scratchDir profile property and a local transferPath guard, and it keeps using the existing Managed Agents session file APIs (writeFileBytes/readFileBytes) and the shared exec helpers. It does not build any new protocol, queue, or transport, so nothing here duplicates an SDK or library capability. The only custom code is a small path check that mirrors the platform's /workspace confinement, plus fixes to the test double, and neither reimplements functionality a maintained integration already provides.
Non-durability claude-opus-5-5 (independent reviewer) Accept 88/100 Through the non-durability lens, the PR adds no process-local state: scratchDir is a static profile constant, and the existing Maps in managed-agents-sandbox are unchanged. Credential and export scratch files now live in /workspace/.qm-scratch, which survives provider pause at least as well as /tmp did, and the stale sweep relies on on-disk mtimes rather than in-memory tracking, so there is no new deploy- or crash-fragile state.
Config-matrix expansion claude-opus-5-5 (independent reviewer) Accept 78/100 The PR adds no env vars, settings, or schema. scratchDir is a code-level backend capability that replaces an implicit hardcoded /tmp assumption and drops the slice(5) prefix hack, so the implicit-mode footprint shrinks. One small redundancy remains: ExecExportDeps.scratchDir is optional and has its own DEFAULT_SCRATCH_DIR fallback alongside scratchDirFor. Making it required and always passing scratchDirFor(profile) would leave a single defaulting point.
Provider drift claude-opus-5-5 (independent reviewer) Accept 80/100 The PR makes Managed Agents' transfer confinement an explicit scratchDir profile property. This follows the same pattern as parksOnTeardown, replacing hidden /tmp assumptions in prepareExecutionFiles and createExecExport. Every other backend keeps the byte-identical /tmp default, and the fake now refuses outside-/workspace transfers the way the platform does. Two residual risks remain: a live QA run is still pending, and other writeAbsBytes callers such as installLayerTools.writeAbs were not audited for non-/workspace targets, though these would now fail locally with a clear message rather than drift silently.
God files claude-haiku-5-5 (triage) Not relevant — The changes are small and localized to their existing responsibilities, with no unrelated concerns absorbed into a large module.
Mismatched UI claude-haiku-5-5 (triage) Not relevant — The diff contains no user-interface changes.
Over-indexing on YC claude-haiku-5-5 (triage) Not relevant — The diff contains no YC-specific architecture or customization.
Regex claude-opus-5-5 (independent reviewer) Accept 80/100 No new production regex: transferPath checks paths with startsWith and hasParentPathSegment, and the PR replaces the brittle prefix.slice(5) with structured scratch/basename values. The only regex growth is in the test fake. GUEST_ROOT_PREFIX uses a lookbehind/lookahead rewrite of /workspace inside raw shell scripts, extending an existing textual remap. That is fragile but test-only and not a reason to block.
YC info leaking into public qm claude-haiku-5-5 (triage) Not relevant — No organization-specific names, identities, or internal references appear in the diff or description.
Discarded error evidence claude-haiku-5-5 (triage) Not relevant — The new transferPath error names the path and constraint rather than discarding diagnostic evidence, and no failure is swallowed.
Legacy preservation claude-haiku-5-5 (triage) Not relevant — Defaulting scratchDir to /tmp is a current design choice for other backends, not preservation of a retired path or data shape.
Over-testing claude-opus-5-5 (independent reviewer) Accept 82/100 The five new tests are proportionate: each production fix (credential staging, stale sweep, export, local transferPath guard) gets one behavioral test, and the PR already dropped tests that re-asserted constants or string-matched the shell script. The one marginal case is "the guest refuses a transfer outside the workspace", which exercises the fake rather than production code. It is cheap, though, and pins the fidelity property whose absence let this bug ship, so it does not warrant a change.
Prod-like QA claude-opus-5-5 (independent reviewer) Accept 84/100 The live run on a real DigitalOcean Managed Agents session at the PR head exercised every changed Managed Agents path. It staged a credential under /workspace/.qm-scratch, read it back with the merged env (exit 0), swept a planted 3h-old directory, confirmed finish() removed the credential directory, and exported through the scratch tar. Two minor caveats: the claimed main 403 baseline appears in prose, not in the raw output, and the default-backend scripts are not byte-identical. They now add mkdir -p '/tmp' and quote the find root, but that is semantically inert, so it does not block merge.
Performance claude-haiku-5-5 (triage) Not relevant — The changes add no polling, repeated expensive work, or lock contention.
Token performance claude-haiku-5-5 (triage) Not relevant — The diff does not change anything sent to the model.
Honorable mentions claude-haiku-5-5 (triage) Not relevant — The diff adds no screenshots or images and no code comments.

Prod-like QA was reassessed after the live run (see Live QA above). The other lenses carry forward unchanged; the code at this commit has not changed since they were reviewed.

nkurraDO and others added 3 commits October 12, 2026 23:04
Managed Agents draws two different boundaries. Exec is unconfined, so a
command can write anywhere in the guest, but file transfer resolves only
under /workspace and answers 403 "path outside workspace" for anything
else. Shared backend code assumes /tmp is a safe place for a scratch
file, which holds for every other backend and breaks here the moment the
file has to be transferred rather than merely written.

Two paths were broken. prepareExecutionFiles stages per-command
credentials in a /tmp directory and uploads each file into it, so any
command needing a credential failed. createExecExport tars the computer
into /tmp and reads the archive back, so export failed.

Backends now declare where their scratch files belong. scratchDir
defaults to /tmp, leaving the other backends byte-identical, and
Managed Agents declares /workspace/.qm-scratch, which exec and transfer
both see. It sits outside /workspace/home, so credentials still stay out
of the home tar and the export.

Transfer paths are checked at the backend's own chokepoint as well, so a
caller that reaches outside /workspace gets a local error naming the
constraint and the offending path instead of a bare remote 403.
The fake answered a transfer for any path by falling through to the host
filesystem, so writing /tmp in a test "succeeded" against the machine
running the suite while the same call 403s in production. That inversion
is why the scratch-directory bug shipped: the double was more permissive
than the platform it stands in for.

Transfers outside /workspace now fail the way the platform fails them,
guest paths map onto a per-session root rather than the host, and
command output maps back, so a path printed by mktemp is a guest path
the caller can actually transfer.

Covers both paths that were broken, that the sweep still finds stale
credential directories in the backend's scratch directory, and that a
backend declaring no scratch directory still gets /tmp.
@16francej

Copy link
Copy Markdown
Contributor

Thanks @nkurraDO! Continuing in #2335. It has the same commits on a branch in this repo, so the tested-tree certification can run. Credit for the fix stays with you.

@16francej 16francej closed this Oct 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants