diff options
| -rw-r--r-- | docs/refactor-groups.md | 71 | ||||
| -rw-r--r-- | man/meshbay-node.1 | 17 | ||||
| -rw-r--r-- | packages/meshbay-node/src/meshbay_node/daemon.py | 23 | ||||
| -rw-r--r-- | packages/meshbay-node/tests/test_cli_dispatch.py | 43 | ||||
| -rw-r--r-- | packages/meshbay-node/tests/test_windows_root_shapes.py | 149 |
5 files changed, 298 insertions, 5 deletions
diff --git a/docs/refactor-groups.md b/docs/refactor-groups.md index f16f2f2..d63d858 100644 --- a/docs/refactor-groups.md +++ b/docs/refactor-groups.md @@ -1,14 +1,15 @@ # Groups Refactor — Per-Root Permissions & App Plugin Architecture -> Status: **Phases 1 and 2 complete** (2026-09-06). Phase 3 not started. +> Status: **Complete** (2026-09-07). All three phases built, reviewed and +> tested against a running node. > > 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; §7c does the same for Phase 2. Read both before starting -> Phase 3 — several entries are rules rather than one-off fixes. +> §7b, §7c and §7d record what each phase's review found and where the plan +> below was wrong. Several entries are rules rather than one-off fixes; §7d +> also lists what a person still has to test by hand. --- @@ -831,3 +832,65 @@ no syntax check at all before, which is how the file was committed. - **What Phase 3 still owes:** the HelloWorld app (§4.1) — which is the actual proof of the above, since every test here reads source rather than adding an app and watching it work — plus the CLI polish and the Windows pass. + +--- + +## 7d. Phase 3 as built (2026-09-07) + +### HelloWorld earned its place + +It was written last and immediately found two things no amount of source +reading had: `group-settings.js` fell back to the *whole* registry when a group +had no `enabled_apps` yet — which would have enabled a hidden app for everyone +— and `group-page.js` wrote out `videoDirectories` / `musicDirectories` / +`photoDirectories` by hand, so a fifth app would have needed that file edited. +Both are fixed by making the code less app-specific, and the plugin claim is +now true rather than nearly true. + +That is the argument for keeping it: every other test of the architecture reads +source for the *absence* of app names, which proves nobody wrote a special case +— not that a new app works. Deleting HelloWorld would leave the claim resting +entirely on tests that read text. + +**It is hidden behind `?dev=1`**, not excluded from the build as §4.1 imagined. +There is no build step to exclude it from, and an unregistered app proves +nothing, since registration is exactly what is claimed to be sufficient. The +flag is the same opt-in shape as `transport.js`'s `?trace=1`. + +### The CLI + +`member upload` reached the generic usage line for the other `member` verbs — +"usage: meshbay-node member upload <username>" — which advertises a removed +feature and sends the operator looking for a username it would then reject. It +names `root set --writable` now. `--upload-dir` still works, so an existing +script keeps working, but its help and the man page say it is the old spelling. + +### Windows + +`test_windows_root_shapes.py` covers what can be covered from here: drive +letters and UNC through `as_posix()` into TOML, a drive root having no basename +to derive a name from, and a case-insensitive collision — which on NTFS and +exFAT is one directory indexed as two roots. All pass. + +**What still needs a person on Windows**, and cannot be faked: + +- `ReadDirectoryChangesW` dropping events under load — the reason periodic + reconciliation is mandatory, and the reason eject exists at all +- `MAX_PATH` against a deep library, on download and on upload +- whether an eject actually lets the drive be removed, and a plug picks it back + up — the eject/plug pair is the least-exercised thing in all three phases +- the folder-tree picker against backslash paths in the UI +- the Create Group wizard with a drive-letter root + +### What the whole refactor still owes + +Nothing in the plan. Two things it did not think of: + +- **`_do_dir_delete` was never checked against RO/RW.** `_do_file_upload` and + `_do_dir_create` both gained the `writable` check; deletion is operator-only + and so is not the same hole, but the asymmetry is worth a look. +- **`index_delta` carries roots but not `dirs`.** A folder created by another + member does not reach a connected client's folder picker until a full + `index_sync`. Small, and the picker offers root names from the roots table + regardless, so nothing is unreachable — but it is the same class as the bug + §7d's roots fix closed. diff --git a/man/meshbay-node.1 b/man/meshbay-node.1 index 92067a2..890da89 100644 --- a/man/meshbay-node.1 +++ b/man/meshbay-node.1 @@ -167,6 +167,14 @@ encryption key on their next connection. Rotate the GEK afterwards with \fBmember unpin\fR \fIusername\fR Forget a member's pinned key, allowing them to pair again with a new one. . +.TP +.B member upload +Removed. Whether uploads are accepted is a property of each directory now, not +a per\-group switch \(em see +.BR "root set" . +A group whose directories are all read\-only accepts no uploads at all, which +is what turning the old switch off meant. +. .SS Group encryption key (GEK) .TP .B gek init @@ -272,6 +280,15 @@ and makes the node treat the directory suddenly disappearing as an unannounced eject rather than as a deletion. . .TP +\fB\-\-upload\-dir\fR \fIpath\fR +Deprecated. A second, read\-write root, used with +.BR "group add" , +from before roots carried their own read\-write flag. Setting it makes every +other root of that group read\-only. Use +.B "meshbay\-node root add" \fIpath\fR \fB\-\-writable\fR +instead. +. +.TP \fB\-\-name\fR \fIname\fR Explicit name for a root, used with .BR "root add" . diff --git a/packages/meshbay-node/src/meshbay_node/daemon.py b/packages/meshbay-node/src/meshbay_node/daemon.py index bcc4f6d..a9376e7 100644 --- a/packages/meshbay-node/src/meshbay_node/daemon.py +++ b/packages/meshbay-node/src/meshbay_node/daemon.py @@ -1784,7 +1784,8 @@ def main() -> None: parser.add_argument("--dir", default=None, help="shared directory, for group add") parser.add_argument("--upload-dir", default=None, - help="separate upload directory, for group add") + help="deprecated: a second read-write root, for group " + "add. Use `root add <path> --writable` instead") parser.add_argument("--yes", action="store_true", help="skip the confirmation for destructive commands") parser.add_argument("--config", type=Path, default=None, @@ -2133,6 +2134,26 @@ def main() -> None: f"expires {i['expires_at']}") return + # `member upload` is gone: whether uploads are accepted is `writable` + # on the root they would land in, not a per-group switch. Named + # explicitly rather than left to the usage line below, which offered a + # username for a verb that no longer takes one — an operator following + # it would have got "unknown subcommand" and no idea what replaced it. + if sub == "upload": + print("`member upload` is gone. Uploads are decided per directory " + "now:") + print() + print(" meshbay-node root list " + "# which are read-write") + print(" meshbay-node root set <name> --writable " + "# accept uploads there") + print(" meshbay-node root set <name> --no-writable # stop them") + print() + print("A group whose directories are all read-only accepts no " + "uploads at all,") + print("which is what turning the old switch off meant.") + sys.exit(1) + if not args.target: print(f"usage: meshbay-node member {sub} <username>") sys.exit(1) diff --git a/packages/meshbay-node/tests/test_cli_dispatch.py b/packages/meshbay-node/tests/test_cli_dispatch.py index 2ba251f..1a01eba 100644 --- a/packages/meshbay-node/tests/test_cli_dispatch.py +++ b/packages/meshbay-node/tests/test_cli_dispatch.py @@ -42,6 +42,9 @@ VERBS = [ ["member", "invite", "bob"], ["member", "revoke", "bob"], ["member", "unpin", "bob"], + # Removed, and it has to say so rather than offering a username for a verb + # that no longer takes one. + ["member", "upload"], ["operator", "pair"], ["file", "list"], ["file", "rm", "abc", "--yes"], @@ -233,3 +236,43 @@ def test_a_bare_invocation_with_no_config_yet_exits_cleanly(monkeypatch, tmp_pat assert "meshbay-node init" in capsys.readouterr().out assert not missing_config.parent.exists(), ( "a fresh, unprovisioned start must not create anything on disk") + + +def test_a_removed_verb_says_what_replaced_it(): + """ + `member upload` used to set a group-wide switch that no longer exists. It + reached the usage line for the *other* member verbs — "usage: meshbay-node + member upload <username>" — which advertises a removed feature and sends + the operator looking for a username it would then reject. + + Naming it costs three lines and is the difference between an operator + finding `root set --writable` and concluding the CLI is broken. + """ + import inspect + source = inspect.getsource(daemon_mod.main) + start = source.index('if args.command == "member":') + block = source[start:source.index('if args.command == "group":', start)] + + assert 'sub == "upload"' in block, ( + "`member upload` falls through to the generic usage line") + guidance = block[block.index('sub == "upload"'):] + guidance = guidance[:guidance.index("sys.exit")] + assert "root set" in guidance and "--writable" in guidance, ( + "the message does not name what replaced it") + + +def test_the_help_does_not_offer_the_old_upload_directory_as_current(): + """ + `--upload-dir` still works — an existing script passing it keeps working — + but the help has to say it is the old spelling, or it reads as the way to + do this. + """ + import inspect + source = inspect.getsource(daemon_mod.main) + # The whole call, not just its first string: an adjacent-literal help text + # is several strings, and matching only the first is how a test passes over + # the half that carries the meaning. + start = source.index('"--upload-dir"') + call = source[start:source.index("parser.add_argument", start + 1)] + assert "deprecated" in call.lower() + assert "root add" in call, "it does not name the replacement" diff --git a/packages/meshbay-node/tests/test_windows_root_shapes.py b/packages/meshbay-node/tests/test_windows_root_shapes.py new file mode 100644 index 0000000..5c5da25 --- /dev/null +++ b/packages/meshbay-node/tests/test_windows_root_shapes.py @@ -0,0 +1,149 @@ +""" +The root model against the shapes Windows produces. + +CLAUDE.md is explicit that exFAT/NTFS and Windows are the common case, not an +edge case: most operators are expected to share from an external drive on +Windows. The RO/RW refactor added two booleans and a config rewriter, and the +booleans are path-independent — but the rewriter, the name derivation and the +collision check all touch paths, and none of them has ever run on Windows here. + +What this can check without Windows is the *shape* work: drive letters through +`as_posix()`, a path with no basename to derive a name from, UNC, and a +case-insensitive collision. `PureWindowsPath` is used deliberately — the plain +`Path` on this machine is a `PosixPath`, where a backslash is an ordinary +filename character, which is the mistake that made +`test_a_backslash_path_written_into_node_toml_stays_parseable` fail everywhere +but the platform it was written for. + +What it cannot check is the filesystem itself: `ReadDirectoryChangesW` dropping +events under load, `MAX_PATH`, and whether an eject actually lets a drive be +removed. Those need a person with Windows, and §4.4 of the refactor plan is +where that is written down. +""" + +import os +import tempfile +import tomllib +from pathlib import Path, PureWindowsPath + +import pytest + +from meshbay_node.roots import RootError, RootSet, derive_name + +BS = chr(92) + + +# ── Paths into node.toml ───────────────────────────────────────────────────── + +@pytest.mark.parametrize("raw,expected", [ + (f"D:{BS}Movies", "D:/Movies"), + (f"E:{BS}Music{BS}Albums", "E:/Music/Albums"), + (f"C:{BS}Users{BS}alice{BS}Media", "C:/Users/alice/Media"), + (f"{BS}{BS}server{BS}share{BS}Media", "//server/share/Media"), +]) +def test_a_windows_path_survives_the_config_file(raw, expected): + """ + `ops` writes `as_posix()` into a TOML basic string, where a raw backslash + is an escape — `\\U` and `\\a` are the ones that bite — so the file would + not parse at all. pathlib reads the forward-slash form back on Windows. + """ + posix = PureWindowsPath(raw).as_posix() + assert posix == expected + parsed = tomllib.loads(f'path = "{posix}"\n') + assert parsed["path"] == expected + + +def test_a_raw_windows_path_would_not_parse(): + """The counter-property: without `as_posix()` there is no config file.""" + with pytest.raises(tomllib.TOMLDecodeError): + tomllib.loads(f'path = "C:{BS}Users{BS}alice{BS}Media"\n') + + +# ── Naming a drive ─────────────────────────────────────────────────────────── + +@pytest.mark.parametrize("raw,name", [ + (f"D:{BS}Movies", "Movies"), + (f"E:{BS}Music{BS}Albums", "Albums"), + (f"{BS}{BS}server{BS}share{BS}Media", "Media"), +]) +def test_a_name_is_derived_from_the_last_segment(raw, name): + assert PureWindowsPath(raw).name == name + + +@pytest.mark.parametrize("raw", [f"D:{BS}", f"E:{BS}", f"{BS}{BS}server{BS}share"]) +def test_a_drive_root_has_no_name_to_derive(raw): + """ + Sharing a whole drive is an ordinary thing to do on Windows and there is + nothing to call it, so the operator has to say. Refused with that as the + message rather than named "" or "D:". + """ + p = PureWindowsPath(raw) + if p.name: + pytest.skip(f"{raw!r} has a basename on this platform") + with pytest.raises(RootError, match="explicit"): + derive_name(p) + + +def test_naming_it_explicitly_works(): + with tempfile.TemporaryDirectory() as d: + roots = RootSet.build([{"path": d, "name": "Films"}]) + assert roots.names == ["Films"] + + +# ── Case, which Windows makes real ─────────────────────────────────────────── + +def test_two_roots_differing_only_in_case_are_refused(): + """ + On NTFS and exFAT `Movies` and `MOVIES` are the same directory to the + filesystem and two roots to a case-sensitive comparison — which would index + one tree twice, and make deleting a file from one copy break the other. + """ + with tempfile.TemporaryDirectory() as d: + os.makedirs(os.path.join(d, "Movies")) + os.makedirs(os.path.join(d, "other")) + with pytest.raises(RootError, match="regard to case"): + RootSet.build([ + {"path": os.path.join(d, "Movies")}, + {"path": os.path.join(d, "other"), "name": "MOVIES"}, + ]) + + +def test_a_root_is_found_by_name_without_regard_to_case(): + """ + What a client sends is what a person typed or a path it split, and on + Windows those disagree about case routinely. + """ + with tempfile.TemporaryDirectory() as d: + os.makedirs(os.path.join(d, "Movies")) + roots = RootSet.build([{"path": os.path.join(d, "Movies")}]) + for spelling in ("Movies", "movies", "MOVIES", "MoViEs"): + assert roots.by_name(spelling) is not None, spelling + + +# ── The two flags ──────────────────────────────────────────────────────────── + +def test_the_flags_do_not_touch_paths(): + """ + `writable` and `removable` are booleans and stay booleans on every + platform. Stated as a test because it is the reason the rest of the + refactor needed no Windows work: what did need it is above. + """ + with tempfile.TemporaryDirectory() as d: + os.makedirs(os.path.join(d, "USB")) + roots = RootSet.build([{"path": os.path.join(d, "USB"), + "writable": True, "removable": True}]) + described = roots.describe()[0] + assert described["writable"] is True + assert described["removable"] is True + assert "path" not in described + + +def test_an_ejected_removable_root_is_unavailable_wherever_it_runs(): + with tempfile.TemporaryDirectory() as d: + os.makedirs(os.path.join(d, "USB")) + roots = RootSet.build([{"path": os.path.join(d, "USB"), + "removable": True, "ejected": True}]) + assert roots.roots[0].available is False + assert Path(roots.roots[0].path).is_dir(), ( + "the directory is still there; `ejected` is the operator's answer, " + "not the filesystem's") |