Structural review: mount plan fragmented across four shapes, and 15 smaller findings

[sshine]

Structural review of the whole tree at 97b387d, looking for accidental coupling rather than
bugs. Findings are ordered by leverage, not by size. Every one has a concrete alternative; the
prioritized checklist is at the bottom.

The first finding is the only one where the code makes a promise it does not keep.

1. One mount plan, four representations — and the macOS backend drops three of them

Four things in main() all mean "bind a host path into the sandbox at some mode", and all four
have a different shape:

Field	Shape	Mode	Missing path
homeBindMounts (src/bubblebox.js:560)	{src, dst}	always rw	created on the host
parentMount (:604)	bare string	always ro	n/a
profileMounts (:621)	{path, mode}	either	skipped (--*-try)
extraMounts (:628)	{path, mode}	either	hard exit

Unenforced invariants holding them together:

• every backend must consume all four fields
• emission order matters (:293-300, later bind wins), so the four must be rendered in sequence
• homeBindMounts.src === .dst always — :562-563 computes them identically, so the two-field
  shape is misleading

The consumption site is where this shows. SeatbeltSandbox.wrap destructures exactly three keys
(:314):

const { repoRoot, profileMounts, extraMounts } = this.config;

It never reads sandboxHome, homeBindMounts, parentMount, or any of the four allow* flags.
Reading the resulting policy, on darwin:

• $HOME is not isolated. The dynamic policy emits a blanket (allow file-read*) (:346) and
  nothing redirects $HOME to sandboxHome — sandboxHome is created in main() and then unused.
  The isolated-$HOME claim in README.md:16 and the HOME_LEAKED assertion in
  tests/mounts.test.js:154 are Linux-only facts.
• ~/.claude is not writable. Only PROJECT_DIR, TMPDIR and rw mounts get file-write*, so
  homeBindings has no effect at all.
• parentMounts is inert. The ancestry is readable regardless, so "none" and "tree" describe
  the same policy.
• The four allow* flags are accepted and ignored. The sockets they gate on Linux appear to be
  reachable anyway under the blanket (allow file-read*) plus (allow network-outbound) (:353).

Nothing catches it: nix/tests.nix:14 gates the whole suite on isLinux. A backend could omit three
of four mount sources precisely because there is no single field it would have been forced to handle.

Fix. Collapse to one ordered list, built once:

// resolvePlan() returns, among other things:
mounts: [
  { path: sandboxHome, at: home,     mode: "rw", required: true },
  ...homeBindings.map(p => ({ path: p, at: p, mode: "rw", required: true })),
  ...(parent ? [{ path: parent, at: parent, mode: "ro", required: true }] : []),
  { path: repoRoot,    at: repoRoot, mode: "rw", required: true },
  ...profileMounts.map(m => ({ ...m, at: m.path, required: false })),
  ...extraMounts.map(m   => ({ ...m, at: m.path, required: true  })),
]

Each backend then renders one list. Seatbelt's inability to express at !== path, or a read
denial, becomes an explicit and testable capability gap — a throw or a one-time warning — instead
of three omitted destructuring keys. Either way README.md needs to stop claiming darwin gets the
same isolation.

2. Four allow* booleans encoding one state machine, spread over nine sites

allowSshAgent, allowGpgAgent, allowXdgRuntime, allowDbus are peers in the type but not in
the domain. :259-291 reads:

if (allowXdgRuntime) { /* bind the whole runtime dir */ }
else                 { /* ssh, gpg, dbus individually */ }

allowXdgRuntime subsumes the other three, so {allowXdgRuntime: true, allowSshAgent: true} is a
representable state in which the second setting silently does nothing. Four independent booleans,
five reachable states.

Each flag also has to be threaded through nine places. Not an estimate — 56a9313, which added the
single boolean --allow-dbus, touched exactly nine hunks of src/bubblebox.js: CONFIG_DEFAULTS
(:120), the wrap destructure (:187), the wrap body (:280), mergeOptions (:393),
cliOverrides init (:408), the parseArgs case (:429), the help options list (:496), the help
config example (:519), and the Sandbox.create argument (:646) — plus the README.

Fix. One enum plus a table:

const HOLES = {
  ssh:  { flag: "--allow-ssh-agent", key: "allowSshAgent", help: "…", bind: (xdg) => … },
  gpg:  { flag: "--allow-gpg-agent", key: "allowGpgAgent", help: "…", bind: (xdg) => … },
  dbus: { flag: "--allow-dbus",      key: "allowDbus",     help: "…", bind: (xdg) => … },
};
// plus a single `runtimeAccess: "selective" | "all"` replacing allowXdgRuntime as a peer boolean

CONFIG_DEFAULTS, the parseArgs cases, the help text and the bwrap args all derive from HOLES.
Adding a hole becomes one table entry, and the whole-runtime-dir case stops pretending to be a
sibling of the three narrow ones.

3. main() braids plan resolution with host mutation, so the dry-run seam leaks

:527-674 does eight things: arg parsing, repo-root discovery, session id, sandboxHome mkdir,
three signal handlers, mutation of the real $HOME (:568-571, mkdirSync/writeFileSync),
profile resolution with exit, parentMounts computation, tilde expansion, mount validation with
exit, backend construction, script assembly, the dry-run branch, and spawn.

The seam sits downstream of the effects. By the time BUBBLEBOX_DRYRUN is checked at :666, the run
has already created sandboxHome on disk and possibly created ~/.claude on the host.
tests/mounts.test.js:33 sets a throwaway HOME specifically to contain that — the test suite works
around the braid rather than the code separating it.

Fix. Four stages instead of a knot:

const plan = resolvePlan(argv, env);   // pure: no fs writes, no process.exit; returns errors
if (dryRun) return print(plan);        // seam moves above every effect
materialize(plan);                     // mkdtemp, create homeBindings, register cleanup
run(render(plan));

resolvePlan then unit-tests without bwrap, without a scratch $HOME, and without a Nix-built
testbox — which also unblocks finding 6.

4. mergeOptions is a hand-unrolled spread that silently eats typos

:379-400 is 22 lines computing {...fileConfig, ...definedKeys(cliOverrides)}. Two consequences of
unrolling it:

• Unknown keys in the user's config.json are dropped without a word. loadUserConfig (:137)
  preserves them via spread, then mergeOptions projects exactly six. allowSSHAgent — wrong case —
  is a silent no-op.
• profile and extraMounts come from the CLI only (:397-398), so neither can be set in the config
  file. That asymmetry has no stated reason and is visible only by reading the unrolled body.

Meanwhile the Nix side validates rigorously through evalModules (nix/bubblebox.nix:138, whose
header comment advertises exactly that). Two config surfaces for one box, two validation regimes,
opposite strictness.

Fix. Derive the merge from one schema — the CONFIG_DEFAULTS keys, or the HOLES table from
finding 2 — and reject unknown keys the way the Nix module system would. The help text's config
example (:514-520) becomes JSON.stringify(CONFIG_DEFAULTS, null, 2) instead of a fourth
hand-maintained copy of the same key list.

5. The Sandbox hierarchy has no shared code

:152-179 contributes a constructor that stores config, a wrap() that throws, a three-line
spawn(), and a static factory switching on process.platform. The two subclasses share zero
code — no common helper, no template-method step. Both wrap() bodies are pure
config -> {cmd, args, env}, and both return the identical env: process.env (:304, :371), so
that third field is noise.

This is also why finding 1 went unnoticed: a base class with no shared code gave the appearance of
a contract that nothing actually enforced.

Fix.

const BACKENDS = { linux: bwrapInvocation, darwin: seatbeltInvocation };
const backend = BACKENDS[process.platform] ?? die(`Unsupported platform: ${process.platform}`);
spawn(...backend(plan), { stdio: "inherit", cwd: plan.projectDir });

The dry-run branch becomes print(plan) — and per finding 1 the plan is what the tests want to
assert on, not a rendered argv.

6. The mount-test fixture is split across a .nix file and a .js file

nix/tests.nix:24-43 bakes seven profiles (default, ro, rw, tilde, parent, tree, e2e)
whose names and paths mean nothing except to tests/mounts.test.js, and scratch()
(tests/mounts.test.js:28-32) must create exactly d/default, d/roA, d/rwA, ~/tthome. String
literals matched by hand across two languages; adding a case means editing both files and rebuilding
to change a fixture path. BUBBLEBOX_TESTBOX (nix/tests.nix:52 → test :18) is the hole punched to
rejoin the halves at runtime.

Fix. The launcher already reads its config from a path in BUBBLEBOX_CONFIG (:37), so the test
can write its own config JSON per case and invoke node src/bubblebox.js directly. nix/tests.nix
shrinks to "run node --test with nodejs and bubblewrap on PATH", and the profile matrix becomes data
in the file that asserts on it.

7. homeBindings is a second mount language, with the missing half guessed

nix/bubblebox.nix:25-44 establishes the pattern for "a path with a mode": coercedTo str into a
submodule, so both "~/.config/gh" and {path; mode;} are accepted. homeBindings (:83) is
listOf str, and the JS then infers file-vs-directory from the filename (:565-572):

if (path.extname(rel)) { /* create empty file */ } else { /* mkdir */ }

Same kind of declaration, second representation, missing information recovered by heuristic. It
happens to hold for .claude / .claude.json / .local/share/opencode; a binding like
~/.config/foo.d would be created as an empty file.

Fix. Give homeBindings the same coercedTo treatment mountType already models, carrying
kind = "dir" | "file" with "dir" as the default, and delete the heuristic. Feeds straight into
finding 1's unified list.

8. A dead env field, and tool meaning two things one hop apart

Nix toolEnv becomes JSON env (nix/bubblebox.nix:190) becomes BOX.env (:55) — which is never
read. The env is actually applied by the second makeWrapper (nix/bubblebox.nix:232-240), so one
concern is expressed in two places and one copy is vestigial.

The same boundary renames inconsistently: Nix tool is a package, JSON tool is a binary name
(nix/bubblebox.nix:189 assigns toolBinary to it). One word, two meanings.

Fix. Drop env from the JSON and from loadBoxConfig; rename the JSON key to toolBinary to
match its source.

9. nix/packages.nix mixes the mechanism with three of the instances

The file declares the boxes option (:6-11), populates three specific boxes (:31-65), and maps
boxes onto packages/apps (:28, :67-68). Meanwhile nix/agents/pi.nix and
nix/agents/hermes.nix contribute to boxes from their own files.

So "where does a new box go?" has two answers, and the rule lives in prose — README.md:162 tells
contributors to edit packages.nix. In a tree that otherwise follows one-file-one-aspect, that is a
convention maintained by documentation.

Fix. Keep only the option and the packages/apps mapping in one file; move claudebox, codexbox
and opencodebox to nix/agents/*.nix, each matching hermes.nix's shape. Adding a CLI becomes
"add a file", uniformly, and the README section collapses to a pointer at nix/agents/hermes.nix as
the template.

10. shellQuote plus bash -c for what cwd already does

The concern is "run this argv with cwd = projectDir". The implementation serializes argv into a shell
string (:656-662) and hands it to bash -c, so both backends terminate in "bash", "-c", script
(:302, :369) and wrap()'s parameter is an opaque string the dry-run seam cannot inspect
structurally.

Fix. spawn(cmd, args, { cwd: projectDir }) gives the child its directory directly; bwrap and
sandbox-exec both preserve cwd. shellQuote (:112-114), script, the extra bash process and
the quoting surface all delete.

11. Two config loaders, three failure policies

• loadBoxConfig (:36-59): missing env var gives a clean exit(2); malformed JSON gives an
  uncaught throw and a stack trace; a missing required field gives throw new Error and a stack
  trace. Inconsistent with itself.
• loadUserConfig (:133-146): ENOENT gives silent defaults; anything else, malformed JSON
  included, warns and continues.

A typo in the baked config prints a Node stack trace; a typo in the user's config prints a friendly
warning.

Fix. One readJsonConfig(path, { required, defaults }) with one error path, and a die(msg)
helper instead of bare throws. Related: parseArgs calls loadUserConfig() at :403, so argument
parsing cannot run without touching the filesystem — which is why there is no unit test for it. Pass
fileConfig in as a parameter.

12. parentMounts computed imperatively, then corrected

:604-617 opens let parentMount = null, branches three ways, walks up in a while, then applies a
corrective if that nulls out results the branches should not have produced. Correctness depends on
that trailing if running: the tree branch at ~/foo genuinely does produce realHome and relies
on being retracted afterwards.

Fix.

const PARENT_MOUNT = {
  none:   () => null,
  parent: (root) => path.dirname(root),
  tree:   (root, home) => topChildOf(root.startsWith(home + "/") ? home : "/", root),
};
const NEVER_MOUNT = (p, home, root) => p === home || p === "/" || p === root;

One lookup, one predicate, no reassignment.

13. randomHex and sessionId

:78-85 is eight lines of Math.random() used only to build sandboxHome (:536) before
mkdirSync (:555). fs.mkdtempSync(path.join(tmp, BOX.name + "-")) does both atomically and
collision-free; randomHex and the sessionId concept both disappear.

14. claudebox-latest reimplements the name option

nix/devshell.nix:15-23 builds claudebox-dev via .override, then adds a second derivation
(writeShellScriptBin) whose only job is to exec it under a different name. mkBubblebox already
takes name, and toolBinary is set explicitly in nix/packages.nix:34 so it survives an override:

config.packages.claudebox.override {
  name = "claudebox-latest";
  profiles.gh.mounts = [ "~/.config/gh" "~/.ssh" ];
}

Two derivations and a shell hop become one .override.

Separately: which of a developer's dotfiles get mounted (~/.config/gh, ~/.ssh) is a per-person
choice currently living in the shared devshell.

15. Minor

• nix/tests.nix:2 and :12 state the same fact ("Linux-only: the tests drive bwrap directly") ten
  lines apart. Keep one.
• realpath (:87-89) is a pure alias for fs.realpathSync, and the guard-then-realpath pairs at
  :625 and :634 stat twice. try { return fs.realpathSync(p) } catch { return null } states the
  one concern once.

16. Optional: -- strictness

parseArgs errors on any unrecognized token (:467-470), so the box's flags and the tool's flags
share one argv namespace disambiguated solely by a sentinel. claudebox --continue fails, and
README.md:151-158 documents needing -- -- under nix run. The passthrough sentinel is defensible,
but the strictness is a choice: git's convention — the first unrecognized token ends the box's own
options — would accept claudebox --continue while keeping -- for genuinely ambiguous cases.

Already tracked

nix/agents/pi.nix puts an upstream buildNpmPackage (pinned npmDepsHash, a sed rewriting
tsgo -p, a dist-save dance around npmInstallHook) in the same file as the box declaration — two
unrelated axes of change, and the package list tui ai agent coding-agent mom web-ui pods written out
three times at :45, :55 and :61. hermes.nix is 19 lines of pure declaration by comparison.
The fix/pibox-npm-lock branch already reduces pi.nix to that same shape via llm-agents.nix, so
this needs no separate entry:

• Improve pibox's npm lock #2

Checklist

Roughly in priority order. 1–3 are where the structure pays.

• 1. Unify the four mount sources into one ordered list; make each backend render it and fail
  loudly on what it cannot express. Correct the README's darwin claims either way.
• 2. Replace the four allow* booleans with one enum plus a HOLES table driving defaults,
  parsing, help and binds.
• 3. Split main() into resolvePlan / materialize / render / run; move the dry-run seam
  above all effects.
• 4. Derive mergeOptions from one schema; reject unknown config keys; generate the help
  example from CONFIG_DEFAULTS.
• 5. Collapse the Sandbox hierarchy to two functions plus a BACKENDS lookup; drop the
  always-identical env from the return.
• 6. Let the mount tests write their own BUBBLEBOX_CONFIG; reduce nix/tests.nix to a runner.
• 7. Give homeBindings a coercedTo type carrying kind; delete the extension heuristic.
• 8. Drop the dead env field; rename the JSON tool key to toolBinary.
• 9. Split the boxes mechanism from the box instances; one file per box under nix/agents/.
• 10. Pass cwd to spawn and argv directly; delete shellQuote and the bash -c hop.
• 11. One readJsonConfig with one error path; pass fileConfig into parseArgs.
• 12. Replace the parentMounts branches with a lookup table and a NEVER_MOUNT predicate.
• 13. Use fs.mkdtempSync; delete randomHex and sessionId.
• 14. Replace the claudebox-latest shim with .override { name = …; }; move the personal
  dotfile mounts out of the shared devshell.
• 15. Delete the duplicated nix/tests.nix comment; collapse the realpath guard pairs.
• 16. Optional: end box options at the first unrecognized token, git-style.