fix(groups): finish Phase 1 — MNP root management, upload targets, eject state

Review of the Phase 1 commit found the RO/RW model sound but three paths
unfinished, each of which broke the flow the phase exists to deliver. Plus
29 test failures it introduced and no coverage for anything it added.

Uploads went to the wrong directory. The node read a `root` field on
file_upload that no client ever sent, so every upload landed in the first
writable root while the Files toolbar offered its button based on the root
being browsed — with two writable roots, uploading from one wrote into the
other. Files now names the root it is showing; Chat names one chosen in the
shell (an operator-configured directory arrives in Phase 2); the node refuses
an unknown name rather than falling back, and refuses read-only and ejected
roots by code.

Shared directories were unreachable on the web. The table read its roots
only from the loopback API, which resolves to "not available" in a browser,
so the section rendered for nobody there — while the Uploads controls it
replaced had worked — and the transport.updateRoot/ejectRoot/plugRoot methods
beside it were dead. MNP is now the path, loopback the fallback for a local
node with no live connection, and adding a root over MNP takes a typed path
since no web page can browse a remote disk.

Ejecting updated nobody's screen. transport.js resolves an admin ack against
the pending request and returns, which is right for every op whose caller
knows the value it chose; the root acks carry state only the node can compute,
so the operator who clicked Eject was the one client that never saw it happen.
And the ejected flag reached roster.db but was never read back, so a restart
undid it and the next scan read an empty mount point as an erased library.

Also: the member-upload endpoint answered 200 and did nothing (removed); the
wizard ignored the first root's RW switch; reload compared roots on name and
path, so editing writable in node.toml did nothing; the table had no path
column, which is the only thing separating two libraries sharing a basename;
apps_enabled normalisation differed between the two sides of a signed subject.

Tests: eject/plug, per-root upload refusal and the node.toml rewrite had no
coverage at all. test_member_upload_policy.py is replaced by
test_root_writable_policy.py — it tested a removed feature — and every
property worth keeping from it moved rather than being dropped.

Docs: draft-v6 structural decision 9 is annotated as superseded (the operator
can no longer have a directory only they may write to — a real capability
removed, flagged rather than hidden), the man page documents the root verb and
the RO/RW fields, and refactor-groups.md §7b records what the plan got wrong.

Suite: 41 failures before, 13 after — all 13 pre-existing on main.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us

3 files changed, 177 insertions, 39 deletions

diff --git a/docs/apps.md b/docs/apps.md
index ef8cc1a..7ce1e73 100644
--- a/docs/apps.md
+++ b/docs/apps.md
@@ -130,9 +130,10 @@ any app with similar per-group local state.
 
 ## 3. Enable/disable: the mechanism
 
-Same shape as `member_upload` (`meshbay-draft-v6.md` §2.1b) — an
+Same shape as a root's `writable` flag (`refactor-groups.md` §1.1) — an
 operator-signed setting, stored on the node, enforced by absence rather than
-by the client's honesty.
+by the client's honesty. It used to be described against `member_upload`,
+which was the group-wide upload switch; that was removed in the same refactor.
 
 **Node side** (`meshbay_node/roster.py`):
 ```python
@@ -148,8 +149,13 @@ from exactly one place: `webrtc_server.py`'s `_admin_exec_apps_enabled`, after
 `_do_apps_enabled` in `webrtc_server.py` validates before it ever issues a
 challenge:
 - `apps` non-empty — the operator can never lock a group down to nothing.
-- every entry in `WebRTCPeerSession.ALLOWED_APPS` (`{"chat", "files"}` today)
-  — **this is the line a new app's node-side registration touches.**
+- every entry in `WebRTCPeerSession.ALLOWED_APPS`
+  (`{"chat", "files", "video", "music", "photo"}` today) — **this is the line
+  a new app's node-side registration touches.**
+- `files` is added to the list if it is absent, at both writers
+  (`_do_apps_enabled` and `ops.set_enabled_apps`, both at the front so the two
+  agree). It is not a toggle: MNP permits root exploration regardless of what
+  this list says, so hiding the tab only ever misled.
 
 The whole set is signed in one message (`apps_enabled`, `OP_APPS_ENABLED` in
 `meshbay_common.adminop`) rather than one op per app — ticking several boxes
@@ -158,11 +164,11 @@ sorted, comma-joined app list (`"chat,files"`), built the same way on both
 sides so the operator's browser and the node arrive at identical bytes to
 sign/verify.
 
-`enabled_apps` rides in `handshake_ack` and `node_status`, next to
-`member_upload`. Changing it broadcasts `apps_enabled_ack` to everyone already
-connected — `transport.js`'s `onAppsEnabled` — so a disabled tab disappears
-without waiting for a reconnection, the same as `member_upload`'s live
-broadcast.
+`enabled_apps` rides in `handshake_ack` and `node_status`, next to the roots
+table. Changing it broadcasts `apps_enabled_ack` to everyone already connected
+— `transport.js`'s `onAppsEnabled` — so a disabled tab disappears without
+waiting for a reconnection. The root ops (`root_update_ack`, `root_eject_ack`,
+`root_plug_ack`) broadcast the same way, through `onRootsChanged`.
 
 **Client side:** `apps.js`'s `visibleApps(enabledKeys)` filters the registry;
 `group-page.js` calls it with `enabledApps` state (from the ack, `null` until
@@ -234,9 +240,17 @@ the only node-side touches, and both are allow-lists, not new wire messages.
   machinery again; unlike Videos/Music it needs several root folders per
   group rather than one, has a single album-grid view with no third-party
   matching step, and reads EXIF locally on the node instead.
-- **The offline/loopback settings path.** `member_upload` can be toggled two
-  ways: over a live MNP connection, or (Electron only) via the node's local
-  HTTP API when MNP isn't connected (`platform.node.call('PUT', .../member-
-  upload')`, `group-settings.js`). `apps_enabled` only has the MNP path today.
-  Adding the loopback twin is a `meshbay_node.ui` endpoint plus a
-  `group-settings.js` branch, mirroring the existing `member_upload` one.
+- **The offline/loopback settings path.** A root's flags can be changed two
+  ways: over a live MNP connection (any browser, anywhere), or — Electron
+  only, and only when MNP is not connected — via the node's local HTTP API
+  (`platform.node.call('PATCH', '/api/groups/<id>/roots/<name>')`,
+  `SharedDirectoriesTable` in `group-settings.js`). `apps_enabled` only has
+  the MNP path today. Adding the loopback twin is a `meshbay_node.ui` endpoint
+  plus a branch in the table's `run()` helper, mirroring the root ops.
+
+  **MNP is the path that must exist, not the fallback.** The operator of a
+  node is not necessarily sitting at it. The first version of the shared
+  directories table read its roots exclusively from the loopback API, which
+  resolves to "not available" in a browser — so the whole section rendered for
+  nobody on the web, while the controls it replaced had worked there. Any
+  operator-facing setting added here needs the MNP route first.
diff --git a/docs/meshbay-draft-v6.md b/docs/meshbay-draft-v6.md
index 180050c..4439cd1 100644
--- a/docs/meshbay-draft-v6.md
+++ b/docs/meshbay-draft-v6.md
@@ -57,7 +57,7 @@
 | 6 | Portability | exFAT/NTFS and Windows are the **common** case. Case folding and Unicode normalization become correctness requirements, not compatibility notes | E8 / decision 12 |
 | 7 | Accounts | Native registration is **hybrid**: passphrase-derived `auth_key` (the recovery path) plus a device Ed25519 key for day-to-day authentication | E3 / decision 4 |
 | 8 | Authorship | Chat senders are **cryptographically authenticated to each other**; an upload has a **provable owner** who may delete it, as the operator may. v5's node-asserted attribution is replaced | operator decision, §2.4b |
-| 9 | Node authority | The operator may **close uploading to everyone but themselves**, per group. Signed MNP op, stored on the node, enforced by the node — the hidden button is a courtesy, the refusal is the control | §2.1b |
+| 9 | Node authority | The operator decides **which directories accept uploads**, per root. Signed MNP op, stored on the node, enforced by the node — the hidden button is a courtesy, the refusal is the control. **Superseded 2026-09-06** by `docs/refactor-groups.md` §1.1: the group-wide `member_upload` switch this section described is replaced by RO/RW per root, and the "everyone but the operator" carve-out is gone | §2.1b |
 | 10 | Client | A group's UI is a **set of pluggable applications** (Chat, Files today), not one monolithic page. Which are shown is a per-group, operator-signed setting on the same pattern as change 9 | §2.7 |
 | 11 | Hub role | The hub gains a **runtime instance-policy store** (`hub_settings`). First policy: an admin switches **public groups off** hub-wide, enforced server-side on every hub-mediated path. `suspend` vs `revoke` on a group are now written down as the distinct things they are | §2.8 |
 | 12 | Group registry | A group name is **unique per owner account**, not globally; the group's identity is still its UUID. Listed everywhere as `name@owner` | §2.9 |
@@ -74,10 +74,12 @@
 v5 confines uploads to `shared_root/uploads/` with a filename allowlist, no overwrite,
 chunk ordering and a size cap. All four protections stand. Two amendments:
 
-- There is no single `shared_root`. **The operator designates one root as the upload
-  destination**; the quarantine lives inside it. If that root is unavailable the upload
-  fails with a stated reason and never falls back to another; if none is designated,
-  uploads are refused rather than guessed.
+- There is no single `shared_root`. **Each root is read-only or read-write**, and the
+  quarantine lives inside whichever writable root the upload is addressed to. If that
+  root is unavailable the upload fails with a stated reason and never falls back to
+  another; if the group has no writable root, uploads are refused rather than guessed.
+  (Amended 2026-09-06 — the original text designated *one* root as the upload
+  destination, and the client named none. See `docs/refactor-groups.md` §1.1.)
 - **The no-overwrite rule is unchanged and still holds on exFAT/NTFS.** An earlier
   draft claimed a string comparison let `README.TXT` land on `readme.txt` there. It does
   not: the check is `Path.exists()`, and `stat()` is itself case-insensitive on those
@@ -91,6 +93,31 @@ device that asked, so the node keeps no thumbnail store.
 
 ### 2.1b §5.2 Uploads — the operator may close them
 
+> **Superseded 2026-09-06.** `member_upload` is gone; the mechanism is `writable` on
+> each root. What the three load-bearing properties below say is *unchanged* — read
+> "the root's `writable` flag" for "`member_upload`" and every word of them still
+> holds, which is why they are kept rather than deleted. What did change:
+>
+> - **It is per root, not per group.** A group can publish one library read-only and
+>   accept uploads into another, which the single switch could not express.
+> - **There is no carve-out for the operator.** Read-only means read-only for
+>   everyone, because a published library that quietly accepts writes from whoever
+>   holds admin authority is not one. The paragraph below justifying the setting by
+>   "the only way to get a curated library was to designate no upload root at all,
+>   which refuses the operator too" is therefore the reasoning that was reversed: that
+>   *is* the model now, and refusing the operator is the point rather than the defect.
+> - **The client names the destination root.** With several writable roots the node
+>   cannot choose without guessing, and a guess sends a member's file to a disk the
+>   operator did not intend. It names a root, never a path; everything below the root
+>   is still decided by the node.
+> - The signed op is `OP_ROOT_UPDATE` (plus `OP_ROOT_EJECT` / `OP_ROOT_PLUG`) rather
+>   than `OP_MEMBER_UPLOAD`, and the flags live in `node.toml` — they are
+>   configuration — while the *ejected* runtime state lives in `roster.db`.
+>   `member_upload` survives on the handshake ack alone, computed as "any root is
+>   writable", for MNP 1.0 clients that read no other field.
+>
+> See `docs/refactor-groups.md` §1.1 and §1.5b.
+
 New. A group where every member may add files is the default and stays the default;
 some groups want a library the operator curates, and until now the only way to get one
 was to designate no upload root at all, which refuses the operator too.
diff --git a/docs/refactor-groups.md b/docs/refactor-groups.md
index 11956ce..3d7412b 100644
--- a/docs/refactor-groups.md
+++ b/docs/refactor-groups.md
@@ -1,10 +1,14 @@
 # Groups Refactor — Per-Root Permissions & App Plugin Architecture
 
-> Status: **Phase 1 implemented.** Phase 2 and 3 not started.
+> Status: **Phase 1 complete and reviewed** (2026-09-06). Phase 2 and 3 not started.
 >
 > This is the most significant refactoring of the project. It changes how roots
 > are permissioned, how group applications are configured, and how the Settings
 > and Create Group pages are structured.
+>
+> §7b records what the review of Phase 1 found and how the plan below was wrong
+> where it was wrong. Read it before starting Phase 2 — two of its entries are
+> rules the later phases have to follow, not one-off fixes.
 
 ---
 
@@ -175,17 +179,31 @@ flip it back to available.
    refuses with "Directory not found. Is the device connected?"
 4. On success: sets `ejected = false`, marks root `available = true`, restarts
    the watchdog observer
-5. The indexer runs a **reconciliation** (not a full rescan) — compares frozen
-   entries against current filesystem state. New/changed/deleted files are
-   handled normally
+5. The indexer **rescans the root** — its frozen entries are dropped and the
+   directory is read again. (The plan said "a reconciliation, not a full
+   rescan"; it is a rescan, deliberately. It is the same path a root coming
+   back from `refresh_availability` already took, and a device people carry
+   around can come back arbitrarily different — the hash cache means unchanged
+   files are not re-read, which is where the cost would have been.)
 6. `index_sync` update propagates — entries reappear in all apps
 
+**The flag is persisted, and restored at startup.** `ejected` lives in
+`roster.db` (`root_ejected:<folded name>`), not in `node.toml`: it is runtime
+state, and an operator's hand-written config must not be rewritten because a USB
+drive was unplugged. It has to survive a restart — a restart is exactly what an
+operator does after noticing a drive fell off, and a flag that only lived in
+memory would let the scan that follows read the empty mount point as an erased
+library. `daemon._build_roots()` merges the two sources; it is the only place
+that builds a `RootSet` for a group.
+
 **Auto-detection safety net.** If a `removable` root's path suddenly disappears
 (operator unplugged without clicking eject):
 
 - `refresh_availability()` detects `is_live() = false`
 - Because `removable = true`, it sets `ejected = true` automatically (as if the
-  operator had clicked eject)
+  operator had clicked eject), and reports it through the indexer's
+  `on_root_ejected` callback so the daemon writes it to `roster.db` — an
+  auto-eject that only existed in memory would be undone by the next restart
 - Entries freeze, no deletions propagate
 - The root stays in "ejected" state until the operator explicitly plugs it back
 
@@ -281,25 +299,27 @@ For backward compatibility with MNP 1.0 peers:
 
 ### 1.10 CLI changes
 
+As built. The group is a `--group` option rather than a positional, matching
+every other verb in this CLI, and the negative flags are spelled `--no-writable`
+/ `--no-removable` rather than `--read-only`, so each pair reads as one setting.
+
 ```
 # Group creation (first root defaults to RW)
-meshbay-node group add <name> --dir <path>                # first root, RW
-meshbay-node group add <name> --dir <path> --read-only    # first root, RO
+meshbay-node group add <name> --dir <path>                    # first root, RW
+meshbay-node group add <name> --dir <path> --no-writable      # first root, RO
 
-# Root management
-meshbay-node root add <group> <path> [--name <name>] [--writable] [--removable]
-meshbay-node root remove <group> <name>
-meshbay-node root set <group> <name> --writable           # toggle to RW
-meshbay-node root set <group> <name> --read-only          # toggle to RO
-meshbay-node root set <group> <name> --removable          # mark as removable
-meshbay-node root set <group> <name> --no-removable       # unmark
-meshbay-node root eject <group> <name>                    # safe eject
-meshbay-node root plug <group> <name>                     # re-plug
-meshbay-node root list <group>
+# Root management (--group is optional with one group configured)
+meshbay-node root list [--group <name>]
+meshbay-node root add <path> [--name <name>] [--writable] [--removable]
+meshbay-node root remove <name> [--yes]
+meshbay-node root set <name> --writable | --no-writable
+meshbay-node root set <name> --removable | --no-removable
+meshbay-node root eject <name>                                # safe eject
+meshbay-node root plug <name>                                 # re-plug
 
-# Deprecated (removed with warning)
+# Deprecated (accepted with a warning)
 --upload-dir   → "use --writable on the target root instead"
-member upload  → "use 'root set --read-only' / 'root set --writable' instead"
+member upload  → removed; use 'root set --no-writable' / '--writable'
 ```
 
 ### 1.11 HelloWorld proof-of-concept
@@ -655,3 +675,80 @@ QE/migration/migrate_groups_v2.py                                      (new, not
 | Phase 3 | HelloWorld + CLI polish + migration script | ~500 lines |
 
 Each phase is one focused Claude session. Test between phases.
+
+---
+
+## 7b. Phase 1 review (2026-09-06)
+
+What the plan above got wrong, and what was actually built. The first three
+entries are **rules for phases 2 and 3**, not one-off fixes: each describes a
+shape the same code can take again.
+
+### The rules
+
+**An operator is not sitting at their node.** The shared directories table read
+its roots exclusively from the loopback API (`platform.node.available`), which
+resolves to "not available" in a browser. So the section rendered for nobody on
+the web — while the Uploads controls it replaced *had* worked there — and the
+`transport.updateRoot` / `ejectRoot` / `plugRoot` methods written next to it
+were unreachable. §2.2 of the plan said "calls loopback API", and that was the
+mistake: MNP is the path that must exist, and loopback is the fallback for a
+local node with no live connection. Every operator-facing control phase 2 adds
+(the folder tree, four app settings panes, the link-preview toggle) needs the
+MNP route first. Pinned by `test_upload_controls_hidden.py`.
+
+**A reply that carries state nobody could have predicted has to be handed on.**
+`transport.js` resolves an admin `*_ack` against the pending request and
+returns, deliberately: every caller already updates local state from the value
+it chose. The root acks are not like that — they carry the node's whole roots
+table, including things only it knows (availability, the name it settled on,
+the eject a failed plug left in place). Returning left the operator who clicked
+Eject as the single client that never saw it happen, while every *other* peer
+got the broadcast. Any phase-2 op returning computed state has the same shape.
+
+**A control that writes needs to name where.** The node was given a `root`
+field on `file_upload` and no client ever sent it, so every upload went to
+`writable_roots[0]` while the Files toolbar offered the button based on the root
+being browsed. With two writable roots, uploading from one wrote into the other.
+This is the failure `_settle_upload_root`'s deleted docstring existed to
+prevent, reintroduced by removing it. Chat's attachments have the same problem
+one level up and get an explicit `attachRoot` until §1.7 gives them a
+configured directory.
+
+### The rest
+
+- **`ejected` was written to `roster.db` and never read back**, and the
+  auto-eject path did not persist at all. Both fixed; see §1.5b.
+- **`PUT /api/groups/{gid}/member-upload` became a stub returning `200
+  {"deprecated": true}`.** A route that answers OK and changes nothing is
+  indistinguishable from a working one to whoever calls it. Removed.
+- **The wizard ignored the first root's RW switch** — `ops.attach_group` always
+  wrote `writable = true`. It takes the flag now.
+- **`refresh_availability` was the only reader of a root's config.** The reload
+  path compared roots on `(name, path)`, so an operator editing `writable` in
+  `node.toml` and reloading saw nothing happen. The comparison includes the
+  flags.
+- **The table had no Path column** (§1.5 asked for one). Two libraries whose
+  folders share a basename are indistinguishable without it, and the basename is
+  the identity — so it is the one thing that has to be visible.
+- **Phase 1 shipped no tests.** 29 of the suite's failures were its own. The
+  gap that mattered was not the broken helpers but that eject, plug, per-root
+  upload refusal and the `node.toml` rewrite had no coverage at all:
+  `test_root_eject.py`, `test_root_writable_policy.py` and the new cases in
+  `test_ops.py` / `test_node_status.py` / `test_security_regressions.py` are
+  that. `test_member_upload_policy.py` is gone — it tested a removed feature.
+- **`chat-app.js` was in §2.2's file list and was never touched.**
+- **Eight of the ten locales were missing the new keys.** `test_locales.py`
+  holds them to `en.js`, so this was a failing test rather than a silent gap —
+  but it is worth noting that adding a key means adding it ten times.
+
+### Still open, deliberately
+
+- **The operator can no longer have a directory only they may write to.** RW is
+  open to every member; RO refuses everyone including the operator. This
+  reverses draft-v6's structural decision 9, which is annotated there. It is a
+  real capability removed, and if it turns out to be wanted the answer is a
+  third state on the root, not the old group-wide switch.
+- `test_ops.py::test_a_backslash_path_written_into_node_toml_stays_parseable`
+  fails on any non-Windows machine and always has — it builds a
+  `PurePosixPath` from a Windows path. Unrelated to this refactor, left alone.