fix(think): bash tool deletes workspace directories under sandbox roots - #2239
mattzcarey wants to merge 5 commits into
Conversation
…andbox roots The sync pass recorded every pre-existing directory, filtered anything under /tmp, /bin, /usr, /dev, /proc and /sys out of the final tree, and then treated those directories as deleted by the script and rm -rf'd them after writing their files back. Workspace-owned directories below a sandbox root now sync like any other directory; only the roots themselves are skipped.
🦋 Changeset detectedLatest commit: dc3cd93 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
⚪ agents import sizesMeasured 343 runtime imports as minified bundles. The primary size is gzip; raw minified size is included for diagnosis. An existing import growing by more than 10% is marked red. This report is informational.
Compared No import sizes changed. All 343 current runtime imports
Reported by agent-think[bot]. |
agents
@cloudflare/ai-chat
@cloudflare/codemode
hono-agents
@cloudflare/shell
@cloudflare/think
@cloudflare/voice
@cloudflare/worker-bundler
commit: |
Workspace ownership in the bash sync was exact-path: only the files and directories the snapshot recorded synced back. A file created, or a file renamed, inside a workspace-owned directory below a sandbox root was dropped by shouldSyncBashPath while the deletion pass still removed the rename source, so `mv /tmp/cache/data.txt /tmp/cache/renamed.txt` lost the content outright. Decide ownership by ancestry instead, matching the doc comment: a path syncs if it is, or descends from, an initial workspace directory, with the sandbox roots themselves excluded as ancestors so scratch written directly under /tmp or /usr still never persists. Collapse the duplicated /tmp special case into a single BASH_SANDBOX_ROOTS constant.
…dbox-root renames Derive the shell's own infrastructure subtrees by booting a throwaway just-bash and reading its filesystem, then exclude them from ancestry-based ownership: a workspace holding /usr/bin no longer adopts the file-per-builtin the shell writes there, while exact workspace files under those paths keep syncing. Renames that land directly on a sandbox root (mv /tmp/cache /tmp/archive, or a file moved out of an owned subdirectory) had no initial ancestor, so the destination was dropped while the deletion pass removed the source. just-bash exposes no rename events or fs journal, so provenance is inferred from the before/after trees: a new entry directly under a sandbox root is owned when an initial workspace path under the same root vanished and the new entry's contents match it exactly.
…tent matching Inferring moves from content equality was wrong in three ways: a file edited after its move no longer matched, an empty directory had nothing to match on, and scratch that happened to equal a deleted workspace file was adopted as its destination. just-bash accepts a caller-supplied IFileSystem, so the sandbox is now built on an InMemoryFs whose mv/cp/rm are wrapped to record a journal. Replaying it gives real provenance: a move from a workspace-owned source makes its destination workspace-owned, transitively and regardless of what happens to the content afterwards, while a later rm of the destination revokes it. Copy events are recorded but grant nothing, because a directory mv is implemented internally as cp plus rm. Nothing else created directly under a sandbox root is adopted, so identical scratch stays scratch.
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| fs.mv = (source, destination) => { | ||
| journal.push({ | ||
| type: "move", | ||
| source: normalizeWorkspacePath(source), | ||
| destination: normalizeWorkspacePath(destination) | ||
| }); | ||
| return mv(source, destination); |
There was a problem hiding this comment.
🟡 Failed moves persist sandbox scratch
When mv fails for an owned source, journal still marks its destination as owned. Existing scratch there then persists into the workspace.
Learn more
Move ownership is based on journal entries, not the final filesystem alone. The wrapper appends a move before the underlying operation settles, so rejected moves remain indistinguishable from successful moves during resolveMovedWorkspaceEntries. Recording only after success would lose the required ordering relative to the nested cp and rm events, so the journal needs an explicit operation result or transaction boundary.
Example: Start with workspace file /tmp/cache/data.txt. Run echo scratch > /tmp/archive; mv /tmp/cache /tmp/archive. InMemoryFs.mv rejects because the destination is a file, but /tmp/archive remains in the final sandbox and the journal grants it ownership. The sync writes scratch into the workspace even though direct scratch under /tmp is excluded.
Recommended fix: Record move lifecycle events with an operation identifier. Replay a move only when its matching completion event confirms success, while preserving the outer move's position before nested cp and rm events. Apply the same success semantics to removal events that can reject.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Think's
bashtool runs scripts against an in-memory snapshot of the Workspace and syncs changes back afterwards. The final sync pass deletes any directory that existed before the run and is missing from the shell's final tree. Three pieces interacted badly:/tmp,/bin,/usr,/dev,/proc,/sys;shouldSyncBashPathfiltered every path under those roots out of the final tree;rm(path, { recursive: true, force: true }).So a Workspace holding
/tmp/cache/data.txtlost/tmp/cacheon every bash call, even one that never touched it. Files under those roots were synced back in an earlier pass and wiped by the directory pass moments later.Fix: workspace-owned directories below a sandbox root are treated like any other directory (they were mounted from the snapshot, so their absence from the final tree does mean the script removed them), and only the roots themselves are skipped since the shell always materializes those.
Found while porting this sync engine into the pi harness in #2229, where Devin flagged the same bug in the copy.
Test plan
assistant-tools.test.ts: seeded/tmp/cache/data.txtand/usr/notes.txtsurvive an unrelated script, andrm -rf /tmp/cacheis still honoured. Verified red without the fix.pnpm vitest --run -c src/tests/vitest.config.tsinpackages/think)Review fixes
Round 1
shouldSyncBashPath). Devin's inline comment was right: with exact-path ownership, a file created or renamed inside a workspace-owned directory below a sandbox root was filtered out of the final tree while the deletion pass still removed the rename source, somv /tmp/cache/data.txt /tmp/cache/renamed.txtdestroyed the content andecho x > /tmp/cache/new.txtwas silently dropped. A path now syncs if it is, or descends from, an initial workspace directory; the sandbox roots themselves are excluded as ancestors, so scratch written directly under/tmpor/usrstill never persists./tmpspecial case into oneBASH_SANDBOX_ROOTS = ["/tmp", ...BASH_EXCLUDED_SYNC_ROOTS]used by bothisBashSandboxRootand the sync predicate.mkdir, and rename beneath/tmp/cacheand beneath a workspace-owned/usr/project, asserting the renamed file keeps its content (Devin's repro) and that/tmp/loose.txtwritten directly under the root is still discarded.patch).Round 2
/binand/usr/binwhile constructing the shell (confirmed: a bareBashboots with ~190 such paths). A workspace that happens to hold/usr/bintherefore claimed all of them under the new ancestry rule.shellInfrastructurePaths()derives the excluded subtrees by booting one throwawayBash({ files: {}, cwd: "/" })and readingfs.getAllPaths()— no hand-copied list, so the set tracks whatever the installed just-bash actually materializes — and memoizes it. Those paths are skipped as ancestry owners; an exact initial file such as/usr/bin/custom-toolstill syncs, becauseshouldSyncBashPathmatches it by path first.mv /tmp/cache /tmp/archiveleft the destination with no initial ancestor, so it was dropped while the deletion pass removed the source. just-bash exposes no rename/mutation events and no journal on its in-memory fs, so provenance is inferred from the before/after trees bydetectMovedWorkspaceEntries(): a new entry directly under a sandbox root is workspace content when an initial workspace path under that same root has vanished from the final tree and the new entry's contents match it exactly — same set of relative file paths, same bytes. Empty vanished subtrees are not matched (nothing to lose, and emptiness alone would adopt unrelated scratch). The rule is documented in the function's doc comment./usr/binnever appear inchangedFilesor the workspace while a seeded/usr/bin/custom-toolsurvives;mv /tmp/cache /tmp/archiveandmv /tmp/notes/note.txt /tmp/note.txtboth keep their content, with the existing assertion thatecho scratch > /tmp/loose.txtis still discarded.Round 3
Devin's three follow-ups were all consequences of inferring moves from content equality, so the inference is gone:
detectMovedWorkspaceEntriesand its content-matching helpers are deleted and replaced with a real filesystem journal.IFileSystem(BashOptions.fs) and exportsInMemoryFs, socreateJournalingBashFs()builds the snapshot'sInMemoryFsand wrapsmv,cpandrmto record every mutation. Verified against the installed just-bash: these methods always receive absolute paths already resolved against the script's cwd, including undercd /tmp && mv archive arch2.resolveMovedWorkspaceEntries()replays the journal: a move whose source is workspace-owned at that point in the script makes its destination workspace-owned, and with it every path below. That applies transitively, so it survives edits made after the move, covers empty directories, chains through repeated moves and crosses sandbox roots. A move from unowned scratch, and a laterrmof a destination, revoke it again.mvis implemented inside just-bash as acpof each entry plus anrmof the source (confirmed by journalling a real run), so a copy event cannot be distinguished from a move's own machinery. Events are recorded before the operation runs for the same reason — the outermvmust be replayed ahead of the calls it decomposes into.rmstays scratch.mvthen append;mvof an empty directory;rmfollowed by identical scratch that must not be adopted; a cross-root move (/tmp/cache/data.txt->/usr/project/data.txt) together with a chained move (/tmp/chain->/tmp/first->/usr/second). The round-2 move tests and the loose-/tmp-scratch assertion are unchanged and still pass.