feat(mounts): host, volume and NFS mounts for workspaces (closes #85) #88
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/mounts"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #85. Lets an operator mount a host path, a named Docker volume, or a remote NFS export into a workspace — declared per repo, editable per workspace at creation, with a Test mount button that probes a real mount before you spend a spawn on it.
The concrete driver: myuzik is developed in a workspace on this host while its media library lives on the
ds9.lanNAS. Verified end-to-end against that export.Design
Mounts happen at the Docker daemon, not inside the container, so the worker stays unprivileged — no
SYS_ADMIN, no/dev/fuse, no mount tooling in the image. Three kinds, differing only in what survives removal:bindvolumenfslocaldriver +driver_optsPer-repo defaults live in
repo_mounts(keyed onrepo_full_namelikerepo_env_vars, so they surviveremoveWorker). The set actually applied is frozen ontoworkers.mounts_json— mounts can't change without re-creating the container, and both UIs say so rather than implying a live apply.Design spec and plan are committed under
docs/superpowers/specs|plans/2026-07-22-worker-mounts*. Durable facts are recorded as AGENTS.md #22.What the probing changed
The issue proposed
o: "addr=…,ro,nfsvers=4". That would never have mounted. Docker'slocaldriver callsmount(2)directly and never execsmount.nfs, so nothing negotiates a version or translates option names —nfsversis amount.nfsname; the kernel's isvers. Measured against the real NAS:type=nfs4/vers=4/nfsvers=4protocol not supportedaddr=…,ro(bare)permission denied/connection refusedaddr=…,ro,vers=3,proto=tcp,nolockTwo more findings: the NFS client is the Docker host (the source address NATs), so export rules must allow the host, not the worker subnet. And a refused mount fails fast at
container.start(), while an unreachable one retries indefinitely — which is why the probe arms its timeout beforestart().Six attempts to find that string is the entire justification for the Test button.
Review
Ran
/simplify(4 agents) and a 5-agent PR review. Notable fixes:/root/.claudedestroyed host data./rootwas exact-match reserved but not prefix-reserved, and the entrypoint runsrm -rf /root/.claude—rmempties the bind's host contents before failing on the mountpoint. Reproduced on the daemon;/rootis now prefix-reserved.//workspacebypassed every reserved-path check. Dockerpath.Cleans the destination, so it mounted at/workspacewhile matching neither the exact set nor the prefix guard. Targets are canonicalised now (..preserved, not resolved).[]is a deliberate instruction the server honours; a failed load, a mid-load Create, or a repo switch could all produce it — the last applying repo A's mounts to repo B. NowMountSpec[] | null, and the spawn body omits the field when null so the server falls back to defaults.buildMountSpecsandassertValidMountwereif-chains ending in the NFS case, so CIFS would have been emitted as an NFS volume with an empty device. Both are nowswitch+never.capabilities.test.tsasserted from an empty HostConfig, which passes equally for the clobbering form its own comment warned against — a dind workspace would have lost every operator mount while the card still showed them.myuzik-cahcemounted fine and listed nothing. It also created the typo'd volume, so a retry would find it "existing" and report clean. Now warns without probing.Testing
tscclean,svelte-check0 errors, lint cleanmounts_jsonclobber, the CIFS exhaustiveness trap, the capabilities clobber, thedemuxLogsremoval, client/server type driftds9docker rm -v, re-create, read it backNot done
Spawning a real workspace end-to-end with a mount needs the live Nexus rather than a throwaway instance. The mount path itself is proven —
testMountuses the samebuildMountSpecsoutput provisioning does.Deferred with reasons in-thread: a
MountTestResultunion (ergonomics only), anEnvVarsEditorextraction (pre-existing code, real regression risk), and a per-kind descriptor table (CIFS's real cost is a sealed credential column, which no such table would save).Three mount kinds in one HostConfig.Mounts array — bind (host path), named volume (persists across workspace re-create), and NFS (anonymous volume with local-driver driver_opts, mounted by the host kernel so the worker stays unprivileged). Per-repo defaults in repo_mounts, per-workspace snapshot in workers.mounts_json, edited inline from the creation modal and the Environment settings tab. Records the probe results that shaped it: vers=3,proto=tcp is mandatory (the local driver calls mount(2) directly, so the issue's nfsvers=4 would never have mounted), the NFS client is the Docker host after NAT, and remove({v:true}) reaps the anonymous volume while leaving a named one alone.From the type-design review. Three real gaps, all on the path a future CIFS kind would take: - buildMountSpecs and assertValidMount were if-chains ending in the NFS case, so a 4th kind fell through and was emitted as an NFS volume with an empty device (and rejected with 'needs a server address'). Adding 'cifs' to MOUNT_KINDS produced ZERO type errors. Both are now switches with a never default — verified: the same edit now errors in exactly those two files. - resolveSpawnMounts trusted saved rows because they were validated on write. That stops being true the moment a rule tightens, so it re-validates. - rowToDTO coerced an unrecognized kind to 'bind', which reaches Docker as {Type:'bind', Source:''} — a silently wrong mount. It now drops the row with a warning; a missing mount is debuggable, a bind of nothing is not. Also corrects MountSpec's doc comment: the flat shape is justified by the DB row and the editor draft, not by JSON — zod's discriminatedUnion would be better at that boundary, and the old wording implied otherwise.From the silent-failure review. The client half of this feature had the real defects; the server plumbing was found sound. Critical — NewWorkerModal treated an unreadable repo config as an empty one. `[]` is an explicit instruction (resolveSpawnMounts honours it and does NOT fall back to defaults), and `mounts` reached `[]` three ways that had nothing to do with the operator emptying it: a failed load, Create clicked mid-load, and a repo switch — the last applying repo A's mounts to repo B's workspace, since the old code never cleared them before fetching. `mounts` is now `MountSpec[] | null`, the spawn body OMITS the field when null (so the server applies saved defaults — the safe direction), Create is gated on the load completing, and a sequence guard stops a stale response winning. Important: - A failed mount delete set rowError, which only rendered inside the editor snippet — and Del is only clickable when no editor is open. The operator confirmed a delete, it failed, and nothing appeared. - testMount reported a green tick for a named volume that did not exist: docker auto-creates it, so a typo'd name mounted fine and listed nothing — the exact mistake the button exists to catch. It also CREATED the typo'd volume, which survives remove({v:true}). Now pre-checks and returns a warning. - Test is no longer offered for an image that isn't local yet; 'No such image' was rendering as though the operator's NFS options were wrong. Also: parseMountsJson now shape-checks elements and logs what it discarded, and EnvironmentPanel's cosmetic count-refresh can no longer report a persisted save as 'save failed'.Browser pass caught the residual: warning about a non-existent named volume was not enough, because the probe still MOUNTED it — which auto-creates it, and a named volume survives remove({v:true}). Confirmed after the run: a stray 'myuzik-cahce' was left on the host. A second Test would then find it existing and report a clean tick, so the button lied on retry. Now returns the warning without probing. There is nothing to learn from mounting an empty volume Docker would have created for us anyway, and the test asserts createContainer is never called.