diff options
Diffstat (limited to 'docs/refactor-groups.md')
| -rw-r--r-- | docs/refactor-groups.md | 135 |
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. |