Skip to content

feat: volume-mode workspaces#404

Merged
dpup merged 8 commits into
mainfrom
feat/volume-mode-workspaces
Jun 25, 2026
Merged

feat: volume-mode workspaces#404
dpup merged 8 commits into
mainfrom
feat/volume-mode-workspaces

Conversation

@dpup

@dpup dpup commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

What

Adds an opt-in workspace.mode: volume that copies the host working tree into an ephemeral per-run Docker named volume mounted at /workspace, instead of bind-mounting the host tree. Motivation: protect the host tree from agent writes, and get native-speed I/O on macOS (where bind mounts are slow). Changes are recovered to the host via snapshots.

# moat.yaml
workspace:
  mode: volume          # or: moat run --workspace-mode volume
  # excludes are carried on a /workspace mount entry:
  # mounts: [{ source: ".", target: /workspace, exclude: [node_modules, dist/sub] }]

Docker-only; Apple containers and git worktrees/submodules are rejected with a clear error (use workspace.mode: bind for those).

How it works

  • The run manager mounts the host tree read-only at a staging path (/mnt/host-workspace) and a fresh per-run named volume (moat-ws-<run-id>) at /workspace.
  • moat-init.sh (forced to run as root) copies staging → volume honoring exclude: patterns, then chowns /workspace to the run user before dropping privileges via gosu.
  • Extraction reuses the existing archive snapshot backend: moat snapshot <run> materializes the volume to a host temp dir via a short-lived sidecar tar container, then archives it with .git included. Recover with moat snapshot restore <run> --to <dir>.
  • moat destroy / moat clean refuse to delete a volume run that has no extraction snapshot unless --force (the volume is the only copy of the agent's work).
  • The proxy daemon's idle cleanup GCs orphaned moat-ws-* volumes whose run directory is gone.

Built on top of #402's named-volume primitive (MountConfig.Volume); this PR adds the workspace-copy layer, snapshot extraction, the data-loss guards, and the GC.

Review

Self-reviewed with /ce-code-review (multi-persona) before pushing. Findings were fixed across three follow-up commits, each verified by unit tests + the Docker e2e lifecycle:

  • Data integrity: VolumeExport now captures the source-tar exit status (a POSIX pipeline previously masked a partial-but-valid source archive → silently truncated snapshot). Orphan GC lists volumes before snapshotting live run dirs (closes a TOCTOU that could reclaim a brand-new run's volume).
  • Ownership: volume mode's 0:0 entrypoint is now treated as a root entrypoint in both the chown-env and ownership-helper paths, so combining volume mode with a config type: volume volume chowns it to the run user (not root).
  • Guards: hasExtractionSnapshot reads the snapshot store directly so the destroy/clean guard works even after the host workspace is gone; moat clean exits non-zero when volume runs are skipped (machine-readable signal). VolumeExport is time-bounded.

Testing

  • make test-unit (race) green across config/container/run/daemon/snapshot/cli, plus new companion tests (chown routing, ListSnapshots, hasExtractionSnapshot, GC live-set, IsVolumeMode).
  • internal/e2e/volume_workspace_test.go — Docker-gated full lifecycle (create → agent commits in volume → snapshot → restore → destroy guard) passes.
  • make lint 0 issues.

Docs

  • New guide: docs/content/guides/15-volume-workspaces.md
  • reference/01-cli.md (--workspace-mode), reference/02-moat-yaml.md (workspace.mode)
  • Design + plan in docs/plans/2026-06-23-volume-mode-workspaces-{design,plan}.md

dpup added 5 commits June 24, 2026 01:18
Add `workspace.mode: volume` (and `--workspace-mode volume`): copy the host
working tree into an ephemeral per-run Docker named volume (`moat-ws-<run-id>`)
instead of bind-mounting /workspace — for host protection and faster macOS I/O.
Changes are extracted via snapshots (`moat snapshot` / `moat snapshot restore
--to`), which include `.git` so commits made inside the volume are preserved.

Builds on the named-volume primitive from #402 (`MountConfig.Volume`,
`buildContainerMounts`, the `moat-init` entrypoint forcing, Apple rejection),
adding the workspace layer on top:

- copy-in from a read-only staging bind via `moat-init` (excludes honored, run
  as root, then chown + privilege drop)
- ephemeral volume lifecycle: `VolumeCreate/Remove/List/Export` on the runtime
- volume-aware snapshots (archive backend, `.git` included, 0600 archives)
  materialized via a `tar` export sidecar
- guards: Docker-only, git worktree/submodule rejected, in-place restore blocked
  (requires `--to`), destroy/clean refuse a volume run with no extraction
  snapshot unless `--force`
- daemon idle GC of orphaned `moat-ws-*` volumes
- docs, unit tests, and a Docker-gated e2e lifecycle test
- VolumeExport: capture the source tar's exit status via a temp file and
  require both pipe ends to succeed. Previously the POSIX pipeline reported
  only the destination tar's status, so a source tar that exited non-zero
  while emitting a valid subset archive produced a silently truncated
  extraction snapshot (the same trap the populate path already guards).
- Orphan-volume GC: list volumes before snapshotting live run dirs to close
  a TOCTOU where a run starting between the two reads could have its fresh
  volume seen-but-not-live and wrongly reclaimed.
- Create cleanup: remove the workspace volume with context.Background() so a
  canceled caller ctx (Ctrl-C) cannot reject the cleanup and leak the volume.
- VolumeRemove: treat "not found" as success, keeping destroy/GC idempotent
  and avoiding spurious warnings when both remove the same volume.
…; add coverage

Fixes from the second review pass, each with a companion test:

- Chown routing (#2): volume mode forces containerUser "0:0", but both
  volumeChownEnv and volumeOwnershipPlan only recognized "" as a root
  entrypoint. So a run combining workspace.mode: volume with a config
  type: volume volume skipped MOAT_VOLUME_CHOWN *and* ran the ownership
  helper, chowning the volume root to 0:0 (root) — leaving the run user
  unable to write it. Treat "0:0" as a root entrypoint in both places (only
  affects volume mode; #402's ""/UID paths are untouched). This also drops a
  redundant root->root helper round-trip the workspace volume triggered.

- Destroy/clean guard (#3): hasExtractionSnapshot built a snapshot.Engine,
  which requires the run's workspace to still exist — but in volume mode the
  host workspace is typically gone by destroy time, so the guard failed
  closed and demanded --force even when a valid extraction snapshot existed.
  Add snapshot.ListSnapshots to read the store directly (no workspace) and
  use it.

- VolumeExport timeout (#5): bound the export with a 30m context so a wedged
  container or stalled daemon can't hang `moat snapshot` forever.

- Single-source the volume-name prefix (#7): export daemon.WorkspaceVolumePrefix
  and have run.WorkspaceVolumeName build from it (run imports daemon, not the
  reverse), removing the duplicated "moat-ws-" literal.

Tests: 0:0 cases for volumeChownEnv and volumeOwnershipPlan; ListSnapshots
without a workspace; hasExtractionSnapshot (pre-run/safety -> false, manual ->
true, no store -> false) with no metadata/workspace on disk;
liveRunIDsFromRunDirs run-dir enumeration.
#6: `moat clean` exited 0 whether it cleaned everything or quietly skipped
volume runs lacking an extraction snapshot, leaving automation no signal that
work was left behind. Return a non-zero exit (with an actionable message) when
any run was skipped; SilenceUsage keeps it to a clean stderr line. --force is
unchanged (it removes them).

#8: remove duplicated literals that risked silent drift:
- config.IsVolumeMode(string) replaces the `== string(WorkspaceModeVolume)`
  cast repeated across clean/snapshot/manager/volume.
- snapshot.BackendArchive/BackendAPFS constants replace the "archive"/"apfs"
  literals in detectBackend and the three snapshot.go ForceBackend sites.

Tests: IsVolumeMode (volume true; bind/empty/mis-cased false).
@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor
📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 7m 30s —— View job


Code Review

  • Gather context and understand the PR
  • Review core volume mode implementation (internal/run/, internal/container/)
  • Review snapshot/export logic (internal/snapshot/)
  • Review daemon GC logic (internal/daemon/volume_gc.go)
  • Review init script (internal/deps/scripts/moat-init.sh)
  • Review CLI changes (cmd/moat/cli/)
  • Review config changes (internal/config/)
  • Review test coverage
  • Review documentation
  • Post findings

Summary

This is a substantial, well-engineered feature. The author has clearly done careful thinking about the hard problems: the POSIX pipeline $? masking fix, the list-before-live TOCTOU ordering in GC, the 0:0 root-entrypoint fix threading through volumeChownEnv and volumeOwnershipPlan, the fresh-context pattern for cleanup after cancel, and the data-loss guard using the storage layer instead of NewEngine (so the guard still works after the host workspace is gone). The companion test coverage is thorough.

One definite bug and one design concern worth addressing before merge:


Definite bug — internal/deps/moat_init_volume_test.go

The TestVolumeCopyInPipeline header (lines 14–18) and its inline comment (lines 68–71) both say "The script passes --no-dereference". Both are stale — the script was revised to omit the flag because GNU tar 1.34 in the container image doesn't recognise it. The truth is documented correctly by TestMoatInitNoNoDeref at line 155. This creates a direct internal contradiction in the same file. See inline comments.


Design concern — --force overloading in moat clean

cleanForce started as "skip the confirmation prompt". It now also overrides the volume data-loss guard. These are semantically very different: the confirmation prompt is a UX convenience bypass; the guard is a safety net for irreversible data loss. A user with a clean --force alias or CI script that suppresses the prompt will silently destroy un-extracted volume workspaces — the guard they probably didn't know existed is bypassed for free.

moat destroy avoids this correctly by using a separate destroyForce flag. See inline comment on clean.go:308.


Minor

  • moat clean duplicate error output: The skipped-count is printed in the summary line and then again (with cobra's Error: prefix) from the return fmt.Errorf(...). The non-zero exit is the right call for automation, but the doubled message is slightly confusing. os.Exit(1) after the fmt.Println(), or a sentinel error type that silences cobra's message, would clean this up. See clean.go inline.

  • VolumeList label filter not reflected in the interface contract: The Runtime.VolumeList signature says "prefix" but DockerRuntime also applies a moat=workspace label filter invisibly. Since only VolumeCreate creates these volumes and it always sets the label, this cannot bite today, but the mismatch between interface doc and implementation is a latent footgun. See docker.go inline.

  • GuardVolumeWorkspace allows non-git directories: A workspace with no .git at all passes the guard even though the error message references "the main checkout". The behaviour is probably correct (volume mode is useful for non-git dirs too), but the error message could be more precise. See volume.go inline.


Things that look solid

  • POSIX pipeline exit-status capture (src_rc_file pattern) applied consistently in both moat-init.sh and VolumeExport — good catch.
  • GC ordering (list volumes before snapshotting live run dirs) correctly prevents the new-run TOCTOU. Test coverage confirms the live-set semantics.
  • cleanupRunDir defer correctly uses a fresh context.Background() so a caller-cancelled context doesn't leak the volume.
  • ListSnapshots reading metadata directly (not via NewEngine) so the destroy guard works after the host workspace is gone — correct and well-tested.
  • ValidateNoGitExclude's filepath.Match(first, ".git") argument order is correct (pattern first, name second).
  • Archive permissions tightened to 0600 — right call since archives may contain .git credentials in volume mode.
  • IncludeGit Restore path correctly skips the backup/rename dance when .git comes from the archive.

@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor
📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 10m 24s —— View job


Code Review

  • Gather context and read key files
  • Review core logic (volume mode, init script, snapshot, GC)
  • Review error handling, edge cases, security
  • Review tests and documentation
  • Post findings

This is a large, well-considered feature. The TOCTOU fix in the GC (list volumes before snapshotting live run dirs), the pipeline exit-code capture pattern in moat-init.sh, the 0:0 chown-path fix in volumeOwnershipPlan, the data-loss guards in destroy/clean, and the security validation in ValidateExcludes/ValidateNoGitExclude are all correct and clearly thought through. A few issues found:

Bugs

listSnapshots missing ForceBackend: BackendArchive for volume-mode

pruneSnapshots (and runSnapshotRestore) correctly force BackendArchive for volume-mode runs, noting that auto-detect would pick APFS on macOS. listSnapshots (line 243, unchanged in this PR) uses snapshot.EngineOptions{}. The listing itself works because Engine.List() reads from the JSON metadata file rather than calling the backend — but engine.Backend().Name() at the footer line would print "apfs" instead of "archive" on macOS. Flagged inline at the prune fix site (line 319).

Codex/Gemini local MCP .mcp.json gets clobbered in volume mode

The Codex/Gemini MCP setup blocks (lines ~305–322 in moat-init.sh) write /workspace/.mcp.json to the (empty) Docker volume before populate_workspace_volume runs. When the staging tree is then tar-extracted into /workspace, any .mcp.json in the user's project overwrites the moat-injected config — which is the opposite of bind mode, where moat's cp writes last. A user who has an existing .mcp.json in their project and uses workspace.mode: volume with codex.mcp: or gemini.mcp: would silently get their own MCP config instead of moat's. Fix: move the .mcp.json copy to after populate_workspace_volume, or guard it with [ "${MOAT_WORKSPACE_VOLUME:-}" = "1" ] and re-apply after the populate call. Flagged inline at line 520.

Low

VolumeExport hardcodes debian:bookworm-slim (inline comment at docker.go:520)

Air-gapped environments, Docker Hub rate limits, or custom registries will cause moat snapshot <run> to fail with an opaque pull error. Worth making this configurable or at minimum surfacing the image name in the error message.

snapshot.NewEngine(meta.Workspace, ...) requires workspace to exist for all snapshot subcommands

list, prune, and restore all call NewEngine with meta.Workspace. If the user's original source directory has been deleted or renamed (unusual but possible, especially across machines), these commands fail with "workspace does not exist" even though the snapshot data is self-contained in snapshotDir. hasExtractionSnapshot already avoids this correctly by using snapshot.ListSnapshots(snapshotDir) directly. This is pre-existing for bind mode, but worth addressing as a follow-up for volume mode where the workspace is conceptually separated from the live data.

Trigger-based snapshots during a volume-mode run would snapshot the host staging directory

SnapEngine is created with opts.Workspace (line 2990 in manager.go) without a volume-mode guard. The pre-run snapshot is correctly skipped (line 3168), but if snapshots.triggers is configured for a volume-mode run, the trigger fires against the host directory rather than the volume content. Low priority since volume-mode snapshot triggers aren't claimed to work, but worth noting before someone combines these features.

Looks good

  • The TOCTOU ordering (list volumes → snapshot live dirs) in GCOrphanWorkspaceVolumes is correct and clearly documented.
  • The pipeline exit-code capture (echo $? > "$src_rc_file") pattern in both moat-init.sh and VolumeExport correctly handles the POSIX pipeline $? pitfall.
  • The "0:0" case in volumeOwnershipPlan correctly prevents the ownership helper from double-chowning when volume mode forces a root entrypoint.
  • Path traversal protection in RestoreTo (symlink validation, path prefix check) is solid.
  • The exclude injection goes via a temp file (not command-line expansion) — no shell injection risk.
  • The data-loss guard (CheckDestroyAllowed, moat clean exit code) is well-placed and tested.
  • The label+prefix double-check in VolumeList/GC limits false-positive orphan reclaim.
  • Test coverage for the key invariants (GC live-set, chown routing, IsVolumeMode, hasExtractionSnapshot) looks solid; companion cases are present.

Comment thread internal/deps/moat_init_volume_test.go Outdated
Comment on lines +13 to +18
// TestVolumeCopyInPipeline exercises the tar pipeline that
// populate_workspace_volume() in moat-init.sh uses. The script passes
// --no-dereference (which is the default in GNU tar; the flag makes the intent
// explicit in debian:bookworm-slim containers). The CI host's GNU tar 1.34
// does not recognize --no-dereference as a flag name, so the test pipeline
// omits it — the default behavior is identical.

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.

Stale comment — contradicts TestMoatInitNoNoDeref (lines 155-161) and the actual script. The script does not pass --no-dereference (removed because GNU tar 1.34 in the container image rejects the flag). The comment here says "The script passes --no-dereference … the test pipeline omits it" — exactly the opposite of the truth.

Suggested change
// TestVolumeCopyInPipeline exercises the tar pipeline that
// populate_workspace_volume() in moat-init.sh uses. The script passes
// --no-dereference (which is the default in GNU tar; the flag makes the intent
// explicit in debian:bookworm-slim containers). The CI host's GNU tar 1.34
// does not recognize --no-dereference as a flag name, so the test pipeline
// omits it — the default behavior is identical.
// TestVolumeCopyInPipeline exercises the tar pipeline that
// populate_workspace_volume() in moat-init.sh uses. The script does NOT pass
// --no-dereference: that long option only exists in GNU tar 1.35+, but the
// container base (debian bookworm) ships GNU tar 1.34, which rejects it. Symlink
// preservation is tar's default behavior; no flag is needed.
//
// The pipeline under test:
//
// cd "$staging" && tar --exclude-from="$excludeFile" -cf - . \
// | ( cd "$workspace" && tar -xf - )
//
// The exclude file uses the REAL emitted format: newline-delimited,
// "./"-prefixed patterns (see run.workspaceExcludes). NUL delimiting is NOT
// used — GNU tar 1.34's --null --exclude-from only applies the first record,
// silently dropping the rest.

The inline comment on line ~68–71 ("Pipeline without --no-dereference … the script keeps the flag for documentation clarity") is also stale for the same reason — the script does not keep the flag. Fix this →

Comment thread internal/deps/moat_init_volume_test.go Outdated
Comment on lines +68 to +71
// Pipeline without --no-dereference: the CI host's GNU tar 1.34 doesn't
// recognize that flag name (it is the default and has no explicit flag).
// The container image (debian:bookworm-slim, GNU tar 1.35+) does accept it;
// the script keeps the flag for documentation clarity.

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.

Second stale comment — same issue as the header. "the script keeps the flag for documentation clarity" is incorrect; the script omits the flag. TestMoatInitNoNoDeref below is the authoritative statement.

Suggested change
// Pipeline without --no-dereference: the CI host's GNU tar 1.34 doesn't
// recognize that flag name (it is the default and has no explicit flag).
// The container image (debian:bookworm-slim, GNU tar 1.35+) does accept it;
// the script keeps the flag for documentation clarity.
// Pipeline mirrors what moat-init.sh runs: tar default behavior preserves
// symlinks without --no-dereference (which GNU tar 1.34 rejects).

Comment thread cmd/moat/cli/clean.go Outdated
// named volume. Destroying without an extraction snapshot loses it, so
// `moat clean` honors the same guard as `moat destroy`: skip such runs
// unless --force was passed (mirrors destroy's --force override).
if !cleanForce && config.IsVolumeMode(r.WorkspaceMode) && !hasExtractionSnapshot(r.ID) {

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.

--force now silently serves two distinct purposes: (1) skip the "N stopped runs — remove?" confirmation prompt (original), and (2) override the volume data-loss guard (new). A user who has developed the habit of moat clean --force to skip confirmations will accidentally destroy un-extracted volume workspaces.

moat destroy correctly used a separate destroyForce flag for exactly this reason. Consider the same approach here — either a separate --force-volumes flag, or at minimum update the flag description to call out the new effect: "skip confirmation prompt and override the data-loss guard for volume-mode runs". Fix this →

Comment thread cmd/moat/cli/clean.go
Comment on lines 402 to 416
fmt.Printf(" (%d worktree dirs skipped — run from within the repo to clean)", wtSkippedCount)
}
fmt.Println()

// Exit non-zero when volume runs were skipped to protect un-extracted work.
// The summary above is human-readable, but `moat clean` (without --force)
// otherwise exits 0 whether it cleaned everything or quietly skipped runs,
// giving automation no signal that work was left behind. A non-zero exit lets
// a pipeline notice and decide to snapshot-then-clean or re-run with --force.
if skippedCount > 0 {
return fmt.Errorf("%d volume run(s) skipped to avoid data loss; snapshot them (moat snapshot <run>) or re-run with --force", skippedCount)
}
return nil
}

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.

The skipped-count info has already been printed in the summary line above ("(%d volume runs skipped — snapshot or --force to remove)"). Returning fmt.Errorf(...) causes cobra to print it a second time with an Error: prefix, producing:

Removed 0 runs... (1 volume runs skipped — snapshot or --force to remove)
Error: 1 volume run(s) skipped to avoid data loss; snapshot them (moat snapshot <run>) or re-run with --force

The human-readable summary is clear; the Error: line is redundant. If the non-zero exit is the goal, using os.Exit(1) after the fmt.Println() or a sentinel error type that cobra suppresses its message for would avoid the duplication. Low severity (it works), but slightly confusing in output.

Comment on lines +466 to +481
// VolumeList returns names of volumes whose name starts with prefix.
func (r *DockerRuntime) VolumeList(ctx context.Context, prefix string) ([]string, error) {
resp, err := r.cli.VolumeList(ctx, volume.ListOptions{
Filters: filters.NewArgs(filters.Arg("label", "moat=workspace")),
})
if err != nil {
return nil, fmt.Errorf("list volumes: %w", err)
}
var names []string
for _, v := range resp.Volumes {
if v != nil && strings.HasPrefix(v.Name, prefix) {
names = append(names, v.Name)
}
}
return names, nil
}

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.

The prefix parameter is received but the Docker API VolumeList is called with only a label filter, then the prefix is applied in Go. That's fine, but the VolumeList interface contract says "returns names of volumes whose name starts with prefix" — the label filter is an invisible extra constraint not in the signature. If someone calls this from a context where the volume was created without the moat=workspace label (e.g. from a pre-label version or a manual test), those volumes would be silently excluded despite matching the prefix.

Since all volumes are created via VolumeCreate which always adds the label, this is not a real bug today. But it's a latent footgun if the interface is reused. The doc comment on the Runtime interface should mention the label filter, or the label should be passed as a parameter. Minor.

Comment thread internal/run/volume.go
Comment on lines +33 to +40
func GuardVolumeWorkspace(hostWorkspace string, rt container.RuntimeType) error {
if rt != container.RuntimeDocker {
return fmt.Errorf("volume mode requires the Docker runtime; set workspace.mode: bind or run with --runtime docker")
}
if info, err := os.Lstat(filepath.Join(hostWorkspace, ".git")); err == nil && !info.IsDir() {
return fmt.Errorf("volume mode does not support git worktrees or submodules (.git is a file at %s/.git); use the main checkout or workspace.mode: bind", hostWorkspace)
}
return nil

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.

The guard checks os.Lstat(filepath.Join(hostWorkspace, ".git")) and rejects if .git is a file (worktree/submodule). But it silently allows the case where .git doesn't exist at all (non-git directory). The error message then reads "use the main checkout or workspace.mode: bind", implying a git repo context that may not apply.

This is probably fine since volume mode is useful for non-git workspaces too, but the error message could be clearer. Not a bug, just a minor message accuracy note.

Comment thread internal/container/docker.go Outdated

cfg := Config{
Name: id.Generate("moat-export"),
Image: "debian:bookworm-slim",

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.

debian:bookworm-slim is hard-coded. If the user's Docker daemon can't reach Docker Hub (air-gapped env, custom mirror, or rate-limiting), moat snapshot <run> fails with an opaque pull error rather than a helpful message. The image also has to match the architecture of the Docker host. Consider making this configurable (e.g., via global config or an env var with a documented override), or at least surfacing the image name in the error when pull fails so users know what to mirror.

fi

# The fresh volume mountpoint is root-owned; hand it to the agent user.
chown -R moatuser:moatuser /workspace

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.

For volume mode + Codex or Gemini with local process MCP servers (codex.mcp: / gemini.mcp: in moat.yaml), there's a sequencing issue: the Codex/Gemini MCP setup sections (around lines 305–320) write /workspace/.mcp.json to the (empty) volume before populate_workspace_volume runs. When populate_workspace_volume copies the staging tree over, it extracts the host workspace contents — and if the user's project already has a .mcp.json, that file overwrites the moat-injected one.

In bind mode, moat's cp wins because it runs on the live bind mount; in volume mode, the host copy wins. For users who have an existing .mcp.json in their project and use volume mode with Codex/Gemini MCP servers, the moat-injected MCP config is silently discarded.

Fix: move the Codex/Gemini .mcp.json copy to run after populate_workspace_volume (i.e., between populate_workspace_volume and run_pre_run_hook), or add a MOAT_WORKSPACE_VOLUME guard around the copy.

Comment thread cmd/moat/cli/snapshot.go
// the archive backend to find/delete them (auto-detect would pick APFS on macOS).
pruneOpts := snapshot.EngineOptions{}
if config.IsVolumeMode(meta.WorkspaceMode) {
pruneOpts.ForceBackend = snapshot.BackendArchive

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.

pruneSnapshots correctly forces BackendArchive here, but listSnapshots (line 243, not modified in this PR) uses snapshot.EngineOptions{} and auto-detects the backend. On macOS, auto-detect picks APFS, so engine.Backend().Name() in the listSnapshots footer prints "apfs" instead of "archive" for volume-mode runs. The listing itself works (reads from the JSON metadata file), but the footer is misleading and the inconsistency is a maintenance footgun.

Same fix as here: check config.IsVolumeMode(meta.WorkspaceMode) and set ForceBackend: snapshot.BackendArchive in listSnapshots. Fix this →

…rce split, list backend)

- B1: in volume mode the Codex/Gemini local-MCP `.mcp.json` was written to the
  fresh volume before populate_workspace_volume tar-extracted the staging tree
  over /workspace, so the user's own .mcp.json could clobber moat's injected
  config (the reverse of bind mode). Move the copy into setup_workspace_mcp_json
  and call it after populate so moat's config wins in both modes.

- D1: split `moat clean` --force. --force now only skips the confirmation prompt;
  a new --force-volumes is required to remove volume-mode runs that lack an
  extraction snapshot. Previously a `clean --force` script silently bypassed the
  data-loss guard. Adds shouldSkipVolumeRunForClean + companion test and updates
  the CLI reference and volume guide.

- P1: clean sets SilenceErrors so the skipped-volume summary line and the
  non-zero-exit error no longer double-print.

- B3: listSnapshots forces the archive backend for volume-mode runs (matching
  prune/restore) so the printed backend name isn't "apfs" on macOS.

- B2: fix stale comments in moat_init_volume_test.go that claimed the script
  passes --no-dereference; it does not (GNU tar 1.34; symlink-preserve is the
  default), as TestMoatInitNoNoDeref asserts.
@claude

claude Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor
📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 5m 58s —— View job


Code Review

  • Gather context and read key files
  • Review core volume mode implementation
  • Review snapshot/export logic
  • Review daemon GC logic
  • Review init script
  • Review CLI changes
  • Review tests and documentation
  • Post findings

Summary

This is a well-engineered, well-tested feature. The author addressed every issue flagged by previous reviews before this push:

  • ✅ Stale --no-dereference comments in moat_init_volume_test.go — test header and inline comment now correctly document what the script does.
  • --force overloading — now a separate --force-volumes flag; --force skips the prompt only, --force-volumes overrides the data-loss guard. The cleanCmd Long help text explains both clearly.
  • ✅ Duplicate cobra Error: line — SilenceErrors: true on cleanCmd suppresses it while preserving the non-zero exit.
  • listSnapshots missing ForceBackend — now guarded the same way as pruneSnapshots.
  • .mcp.json ordering race — setup_workspace_mcp_json refactored into a function called after populate_workspace_volume, with a clear comment explaining why.

The core invariants are all correct: GC list-before-live ordering prevents TOCTOU, pipeline exit-status capture (src_rc_file pattern) is consistent between moat-init.sh and VolumeExport, the "0:0" root-entrypoint case in volumeOwnershipPlan and volumeChownEnv is correctly handled, and hasExtractionSnapshot uses snapshot.ListSnapshots directly to avoid a workspace dependency in the guard path.

One definite bug worth fixing before merge:

Bug — moat snapshot {list,prune,restore} fail when original workspace is gone

All three snapshot subcommands call snapshot.NewEngine(meta.Workspace, snapshotDir, ...), and NewEngine statically validates that meta.Workspace exists on disk (engine.go:50). For --to restores, list, and prune, the workspace is never actually read — the engine operates entirely from the snapshotDir. But if the user deleted or renamed the original source directory (plausible with volume mode: host tree was the staging source, not the live data), these commands fail with "workspace does not exist: /path" even though the snapshot data is self-contained.

The hasExtractionSnapshot function already works around this correctly (uses snapshot.ListSnapshots directly), but the CLI commands haven't been updated to match.

Fix for runSnapshotRestore with --to: substitute os.TempDir() (or any existing path) for meta.Workspace when IsVolumeMode is true and we're doing an extraction-to-dir. Same fix applies to listSnapshots and pruneSnapshots. See inline comment at snapshot.go:472.

Minor

  • VolumeExport hardcodes debian:bookworm-slim — opaque error in air-gapped environments. At minimum, surface the image name in the error returned when the container fails to start. See inline at docker.go:520.
  • VolumeList interface doc says "prefix filter" but the Docker impl also applies a moat=workspace label filter. Not a bug today (all volumes have the label), but the interface contract is incomplete. See inline at docker.go:467.

@dpup

dpup commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the review-bot findings in 11af7ae:

  • .mcp.json clobbered in volume mode (real bug) — moat-init wrote Codex/Gemini .mcp.json before populate_workspace_volume extracted the staging tree, so a project's own .mcp.json could overwrite moat's. Moved the copy into setup_workspace_mcp_json and call it after populate; moat's config now wins in both modes.
  • clean --force overloading — split it: --force now only skips the confirmation prompt; a new --force-volumes is required to remove volume-mode runs lacking an extraction snapshot. Added shouldSkipVolumeRunForClean + a companion test, and updated the CLI reference + volume guide.
  • clean double error messagecleanCmd sets SilenceErrors, so the summary line and the non-zero-exit error no longer both print.
  • listSnapshots backend — forces the archive backend for volume runs (matching prune/restore) so the footer doesn't print apfs on macOS.
  • Stale --no-dereference comments in moat_init_volume_test.go — corrected; the script does not pass the flag (GNU tar 1.34; symlink-preserve is the default), as TestMoatInitNoNoDeref asserts.

Lower-priority items left as follow-ups (noted, not blocking): VolumeList interface-doc vs hidden label filter; GuardVolumeWorkspace wording for non-git dirs; VolumeExport hardcoded debian:bookworm-slim; list/prune/restore still requiring the workspace dir to exist; volume-mode snapshot triggers targeting the host staging dir.

All unit tests (race) + the Docker e2e lifecycle pass; lint clean.

Comment thread cmd/moat/cli/snapshot.go Outdated
if config.IsVolumeMode(meta.WorkspaceMode) {
restoreOpts.ForceBackend = snapshot.BackendArchive
}
engine, err := snapshot.NewEngine(meta.Workspace, snapshotDir, restoreOpts)

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.

Bug: volume-mode snapshot restore fails when original workspace is gone.

NewEngine statically validates that meta.Workspace exists on disk (engine.go:50). For a --to restore of a volume-mode run, the workspace is never read — RestoreTo only opens the archive file at meta.NativeRef and extracts it to destPath. But if the user deleted or renamed the original source directory after their volume-mode run (a natural workflow: agent ran in isolation, host tree is no longer relevant), moat snapshot restore <run> --to ~/recovered fails with "workspace does not exist: /original/path".

Same issue affects listSnapshots and pruneSnapshots (both call NewEngine(meta.Workspace, ...)).

Fix: for volume-mode runs in the --to branch (and for list/prune), substitute a path that is guaranteed to exist — os.TempDir() works since the engine only needs the workspace to pass the existence check; it never reads from it during RestoreTo, List, or Delete.

engineWorkspace := meta.Workspace
if config.IsVolumeMode(meta.WorkspaceMode) && snapshotRestoreTo != "" {
    // RestoreTo reads only the archive; workspace is never accessed.
    // Use a stable path in case the original source dir is gone.
    engineWorkspace = os.TempDir()
}
engine, err := snapshot.NewEngine(engineWorkspace, snapshotDir, restoreOpts)

Fix this →

Comment thread internal/container/docker.go Outdated

cfg := Config{
Name: id.Generate("moat-export"),
Image: "debian:bookworm-slim",

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.

Low: debian:bookworm-slim is hardcoded and will fail in air-gapped environments.

In private networks or behind corporate proxies without Docker Hub access, moat snapshot <run> fails with an opaque pull error that doesn't name the image or explain what to do. At minimum, include the image name in the error path when the container fails to start:

if startErr := r.StartContainer(ctx, ctrID); startErr != nil {
    return fmt.Errorf("start export container (image: %s): %w", cfg.Image, startErr)
}

Longer term, consider making it configurable via an env var or global config (moat.exportImage). Low priority since Docker Hub access is the common case, but worth a follow-up issue. Fix this →

}

// VolumeList returns names of volumes whose name starts with prefix.
func (r *DockerRuntime) VolumeList(ctx context.Context, prefix string) ([]string, error) {

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.

Minor: interface contract doesn't mention the label filter.

Runtime.VolumeList is documented as "returns names of volumes whose name starts with prefix", but this implementation also filters by moat=workspace label. Any caller (or future implementer) relying only on the interface doc would be surprised that volumes without this label are silently excluded even when their names match the prefix.

Since all volumes are created by VolumeCreate which sets the label, this can't bite today. But the interface doc in runtime.go should mention the label filter, or the label should be passed as a parameter. Low priority — just a documentation gap.

…ace; clearer export errors

Bug (review): moat snapshot {list,prune,restore --to} called
snapshot.NewEngine(meta.Workspace, ...), which validates the workspace exists,
even though those operations only read snapshotDir. In volume mode the host tree
was just the staging source and may be deleted/renamed, so these commands failed
with "workspace does not exist" despite the snapshot being self-contained.

Add snapshotEngineWorkspace(): use the real workspace when it exists (preserving
backend auto-detection), else a guaranteed-existing placeholder (os.TempDir).
Used for list, prune, and extract-to-dir restore; in-place restore still requires
the real path (it writes back to the workspace) and volume mode blocks in-place
restore anyway. Adds a companion test.

Minor (review):
- VolumeExport names the base image (volumeExportImage const) in the create-error
  so an unpullable image (air-gapped, rate limit, custom registry) is actionable
  instead of opaque.
- Document that Runtime.VolumeList filters by the moat=workspace label, not just
  the name prefix.
@dpup

dpup commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the latest review in 81a928f: moat snapshot {list,prune,restore --to} no longer fail when the original workspace dir is gone (they only read snapshotDir) — new snapshotEngineWorkspace falls back to a guaranteed-existing placeholder; in-place restore still requires the real path. Minors: VolumeExport names the base image in its create-error; VolumeList interface doc now notes the moat=workspace label filter. Companion test added; unit (race) + Docker e2e + lint all green.

@claude

claude Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor
📦 Previous Review (superseded by newer review)

Claude finished @dpup's task in 7m 58s —— View job


Code Review

  • Gather context and understand the PR
  • Review core volume mode implementation (internal/run/, internal/container/)
  • Review snapshot/export logic (internal/snapshot/)
  • Review daemon GC logic (internal/daemon/volume_gc.go)
  • Review init script (internal/deps/scripts/moat-init.sh)
  • Review CLI changes (cmd/moat/cli/)
  • Review config changes (internal/config/)
  • Review test coverage
  • Review documentation
  • Post findings

Summary

This is a well-engineered feature. The previous review iterations cleared the major issues: .mcp.json ordering, --force overloading, listSnapshots backend, duplicate error output, stale --no-dereference comments, and the snapshot {list,prune,restore --to} workspace-gone fix. The GC TOCTOU ordering, pipeline exit-status capture, "0:0" chown routing, and data-loss guards all look correct and are well-tested.

One definite bug before merge:


Bug — takeSnapshot (exec.go keyboard shortcut) silently snapshots the wrong directory in volume mode

takeSnapshot in cmd/moat/cli/exec.go calls r.SnapEngine.Create(snapshot.TypeManual, "") when the user presses the snapshot keyboard shortcut during an interactive moat exec run. For volume-mode runs, r.SnapEngine is non-nil (snapshots aren't disabled) but was initialised in manager.go line 2990 with opts.Workspace (the read-only host staging directory, never modified by the agent) and no IncludeGit/ForceBackend.

A keyboard snapshot in volume mode therefore:

  1. Archives the original host directory — zero agent changes included.
  2. Creates a TypeManual snapshot — so hasExtractionSnapshot returns true.
  3. That satisfies CheckDestroyAllowed, letting moat destroy proceed and delete the Docker volume, losing all the agent's work.

The pre-run guard at manager.go:3168 correctly skips the pre-run snapshot, but doesn't prevent r.SnapEngine from being used later by takeSnapshot.

Fix: guard takeSnapshot against volume mode, either in exec.go or by testing r.WorkspaceMode before delegating:

// In takeSnapshot, after the r.SnapEngine == nil check:
if config.IsVolumeMode(r.WorkspaceMode) {
    flash("Use 'moat snapshot <run>' to snapshot a volume-mode run")
    return
}

A companion test asserting this flash path would pin the guard.

Fix this →


Minor

  • snapshots.json written with 0644 while archive files use 0600 (snapshot/engine.go:296). The metadata only contains snapshot IDs, types, and paths — not sensitive data — so this is not a security issue. But it's a mild inconsistency: the same file in a restricted-permission snapshot directory will be readable by group/other even if the archive it points to is not. Low priority.

  • snapshotEngineWorkspace fallback applies to bind-mode runs too: the function is called for list/prune regardless of workspace mode. If a bind-mode workspace is gone, it falls back to os.TempDir() and backend auto-detection keys off that path. On macOS with APFS temp dir, prune would use the APFS backend for an archive-backed snapshot set. Not a real problem today (archive Delete doesn't touch the backend FS), but worth watching if APFS delete is ever wired up.


Looks solid

  • .mcp.json ordering (setup_workspace_mcp_json after populate_workspace_volume) — correct; moat's config now wins in both modes.
  • --force / --force-volumes split in moat clean — separation of concerns is right.
  • SilenceErrors: true on cleanCmd — clean suppresses the duplicate error line.
  • listSnapshots / pruneSnapshots / runSnapshotRestore all force BackendArchive for volume mode — correct.
  • snapshotEngineWorkspace workspace-gone fallback — fixes the prior "workspace must exist" bug correctly.
  • hasExtractionSnapshot reads the snapshot store directly (no workspace dependency) — correct and well-tested.
  • GC TOCTOU ordering (list volumes first, snapshot live run dirs second) — prevents the new-run race.
  • Pipeline exit-status capture (src_rc_file / moat-export-rc pattern) — consistent between moat-init.sh and VolumeExport.
  • "0:0" case in volumeOwnershipPlan and volumeChownEnv — correctly prevents double-chown.
  • VolumeRemove ignores "not found" errors — GC and destroy are idempotent.
  • VolumeExport fresh-context defer — cancellation can't leak the export container.
  • Archive permissions tightened to 0600 — right call for archives that may contain .git credentials.
  • ValidateNoGitExclude pattern matching covers glob-matches (e.g. * pattern would match .git) — correct given the restricted character set.

Comment thread internal/run/manager.go
if r.SnapEngine != nil && !r.DisablePreRunSnapshot {
// Create pre-run snapshot. Skipped in volume mode: the host staging tree is
// not the live workspace, so a pre-run snapshot of it would be meaningless.
if r.SnapEngine != nil && !r.DisablePreRunSnapshot && !config.IsVolumeMode(r.WorkspaceMode) {

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.

The pre-run guard is correct. But r.SnapEngine is still set (non-nil) for volume-mode runs, so the keyboard shortcut in cmd/moat/cli/exec.go's takeSnapshot function (line 701) can fire and call r.SnapEngine.Create(TypeManual, "") on this engine. That engine was initialised with opts.Workspace (the host staging dir, never modified) and no IncludeGit / ForceBackend, so the keyboard snapshot:

  1. Archives the original host directory — no agent changes included.
  2. Is TypeManual, so hasExtractionSnapshot returns true.
  3. That satisfies CheckDestroyAllowed, letting moat destroy delete the Docker volume and lose all the agent's work.

Fix options:

  • Guard takeSnapshot in exec.go: if config.IsVolumeMode(r.WorkspaceMode) { flash("Use 'moat snapshot <run>' instead"); return }.
  • Or nil out r.SnapEngine for volume mode here (but that would also silence the "Snapshots not configured" path in takeSnapshot for the wrong reason).

The cleanest fix is the exec.go guard. Fix this →

… loss)

Bug (review): the interactive snapshot keyboard shortcut (takeSnapshot in
exec.go) called r.SnapEngine.Create directly. In volume mode r.SnapEngine is
non-nil but was initialised with the read-only host staging dir, so a keyboard
snapshot archived the original host tree (no agent changes) yet still created a
TypeManual snapshot — which satisfies hasExtractionSnapshot and would let
`moat destroy` delete the volume and lose all the agent's work. Guard
takeSnapshot against volume mode and direct the user to `moat snapshot <run>`
(which exports the volume). Adds a companion test (volume -> no snapshot, bind ->
snapshot still taken).

Minor (review): write snapshots.json with 0600 to match the archive files it
sits beside; the metadata itself is non-sensitive but the consistency is nicer
in a restricted snapshot dir.
@dpup

dpup commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in 26f6eaf: the interactive keyboard snapshot now refuses volume-mode runs (it used the in-process SnapEngine, which points at the host staging dir — a keyboard snapshot would archive the wrong tree yet still satisfy hasExtractionSnapshot and unlock destroy). It now directs the user to moat snapshot <run>. Companion test added (volume → no snapshot; bind → still snapshots). Minor: snapshots.json now written 0600 to match the archives. Unit (race) + e2e + lint green.

@claude

claude Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @dpup's task in 6m 24s —— View job


Code Review

  • Gather context and understand the PR
  • Review latest commit (26f6eaf) — keyboard snapshot guard + snapshots.json permissions
  • Review remaining code: config, container, run, daemon, snapshot, CLI
  • Post findings

Summary

This PR is ready to merge. After four rounds of review, every flagged issue has been addressed. The implementation is correct, well-tested, and the coverage of companion cases is thorough. No new bugs found.


26f6eaf — keyboard snapshot guard

The guard and its test are correct. flashMessage handles nil statusWriter (line 668 in exec.go), so the test passing nil is safe. The flash message includes r.ID, which is helpful — the user can copy-paste it directly.

Two things verified as correct:

  • volumeChownEnv + volumeOwnershipPlan symmetry (volume_mount.go:62, docker.go:209): both check for "" || "0:0" as the root-entrypoint condition. These are now in sync, which is critical — if one had been updated without the other, named-volume chowns for combined volume-mode + config volumes would have gone wrong.

  • IncludeGit not set in runSnapshotRestore restoreOpts (snapshot.go:485): this is correct. RestoreTo extracts the archive as-is; IncludeGit only affects archive creation and the in-place Restore backup/rename dance. The archive already contains .git from when it was created with IncludeGit: true. No action needed.


Minor observations (non-blocking)

StartContainer error doesn't name the image (docker.go around StartContainer call in VolumeExport)

if startErr := r.StartContainer(ctx, ctrID); startErr != nil {
    return fmt.Errorf("start export container: %w", startErr)
}

The CreateContainer error path now names volumeExportImage (the previous review finding). The StartContainer path doesn't — but start failures are typically not pull failures, so the image name is less useful there. Genuinely minor.

liveRunIDsFromRunDirs doc comment (volume_gc.go:49)

"The GC fires from the idle timer, which only runs while the registry is EMPTY"

Slightly imprecise — the idle timer fires 5 minutes after the registry goes empty, not continuously while it's empty. The daemon.go call site is clear, so this can't mislead an operator, but it could confuse a future reader of volume_gc.go in isolation.


Everything that looks solid

  • flashMessage(nil, ...) guard prevents nil-deref in the test and in headless runs.
  • snapshotEngineWorkspace fallback: os.TempDir() is guaranteed to exist; NewEngine validates existence, so the fallback prevents "workspace not found" errors for list/prune/extract-to when the host tree is gone.
  • checkRestoreAllowed correctly blocks in-place restore for volume mode (would write agent changes into the developer's source tree, defeating the isolation purpose).
  • hasExtractionSnapshot reads ListSnapshots directly (not NewEngine) — works after the workspace is gone. Test coverage correctly excludes TypePreRun and TypeSafety, allowing only user-initiated snapshots to satisfy the guard.
  • GC TOCTOU ordering (list volumes before snapshotting live run dirs) is correct and explains itself in the comment.
  • shouldSkipVolumeRunForClean correctly separated from cleanForce — a clean --force alias in a CI script won't silently bypass the data-loss guard.
  • Pipeline exit-status capture (src_rc_file / moat-export-rc pattern) consistent between moat-init.sh and VolumeExport. The ordering is safe: echo $? in the left subshell runs before the subshell exits and closes the pipe write end; the right side is still reading, so it hasn't finished and $? is captured before the pipeline exits.
  • snapshots.json at 0600 matches the archives it sits beside.
  • VolumeRemove ignores "not found" — GC and destroy are idempotent.

@dpup dpup merged commit 9505e6d into main Jun 25, 2026
6 of 7 checks passed
@dpup dpup deleted the feat/volume-mode-workspaces branch June 25, 2026 20:44
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.

1 participant