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

1 files changed, 116 insertions, 19 deletions

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.