From ea56b8c79538323875c00db2e7006b255f7cd494 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 17:48:36 +0200 Subject: fix(groups): finish Phase 1 — MNP root management, upload targets, eject state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- packages/meshbay-node/tests/test_cli_dispatch.py | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'packages/meshbay-node/tests/test_cli_dispatch.py') diff --git a/packages/meshbay-node/tests/test_cli_dispatch.py b/packages/meshbay-node/tests/test_cli_dispatch.py index f58c020..2ba251f 100644 --- a/packages/meshbay-node/tests/test_cli_dispatch.py +++ b/packages/meshbay-node/tests/test_cli_dispatch.py @@ -26,6 +26,15 @@ VERBS = [ ["status"], ["group", "list"], ["group", "add"], # missing --dir: usage, then exit + ["group", "add", "g", "--dir", "/tmp/media", "--no-writable"], + ["root", "list"], + ["root", "add"], # missing path: usage, then exit + ["root", "add", "/tmp/media", "--writable", "--removable"], + ["root", "remove", "media", "--yes"], + ["root", "set", "media", "--no-writable"], + ["root", "set", "media"], # nothing to change: usage, then exit + ["root", "eject", "media"], + ["root", "plug", "media"], ["gek", "init"], ["gek", "rotate", "--yes"], ["gek-init"], -- cgit v1.2.3 From 2d3cbdec301c592daa2faff8e1ca1cab155ebb58 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Mon, 7 Sep 2026 00:58:37 +0200 Subject: chore(node): finish Phase 3 — CLI deprecations, Windows shapes, docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `member upload` reached the generic usage line for the other `member` verbs — "usage: meshbay-node member upload " — which advertises a removed feature and sends the operator looking for a username it would then reject. It names `root set --writable` now, and the man page carries the same. Three lines between an operator finding the replacement and concluding the CLI is broken. `--upload-dir` still works, so an existing script keeps working, but its help and the man page say it is the old spelling and name what replaced it. The Windows pass (§4.4) is what can be checked from here, made checkable: drive letters and UNC through `as_posix()` into TOML, a drive root having no basename to derive a name from — sharing a whole drive is ordinary there — and a case-insensitive collision, which on NTFS and exFAT is one directory indexed as two roots. `PureWindowsPath` throughout, for the reason the backslash test earlier this branch got wrong. What it cannot check is written down rather than glossed: ReadDirectoryChangesW dropping events, MAX_PATH, and whether an eject actually lets a drive be removed. §7d says so, along with two things the plan never considered — the RO/RW asymmetry in `_do_dir_delete`, and `index_delta` carrying roots but not `dirs`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- docs/refactor-groups.md | 71 +++++++++- man/meshbay-node.1 | 17 +++ packages/meshbay-node/src/meshbay_node/daemon.py | 23 +++- packages/meshbay-node/tests/test_cli_dispatch.py | 43 ++++++ .../meshbay-node/tests/test_windows_root_shapes.py | 149 +++++++++++++++++++++ 5 files changed, 298 insertions(+), 5 deletions(-) create mode 100644 packages/meshbay-node/tests/test_windows_root_shapes.py (limited to 'packages/meshbay-node/tests/test_cli_dispatch.py') 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 " — 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 --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 --writable " + "# accept uploads there") + print(" meshbay-node root set --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} ") 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 " — 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") -- cgit v1.2.3 From 435f54b382004de28196aa44c9b1d2c7368ae212 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Mon, 7 Sep 2026 01:17:29 +0200 Subject: fix(node): remove --upload-dir rather than document it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught in review, and the review was right. The previous commit documented the flag as deprecated so that `--help` and the man page would agree. That solved the wrong problem: the flag contradicts the model this whole refactor exists to establish, and the coherent answer was to delete it. It wrote `upload_dir` into a *brand-new* `[[groups]]` block, and `GroupConfig.__post_init__` reads that key by forcing every other root read-only and appending that path as the one writable one. So `group add --dir X --writable --upload-dir Y` silently made X read-only — two mechanisms deciding which directories accept uploads, one of them invisible, in a group created after the model that replaced it. Gone from the CLI, from `ops.attach_group`, from the loopback API and from the MNP `group_attach` payload, which now carries `writable` instead. The *read* path in `config.py` is deliberately untouched: an existing node.toml using `upload_dir` must keep working, and that is the only legitimate use left. The man page says so under the config key, and no longer lists an option. The test that guarded the deprecation wording now guards its absence — and earned itself immediately by finding a `group add` usage string still offering the flag. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- docs/refactor-groups.md | 10 ++++++ man/meshbay-node.1 | 24 ++++++--------- packages/meshbay-node/src/meshbay_node/daemon.py | 18 +++-------- packages/meshbay-node/src/meshbay_node/ops.py | 18 +++++------ .../src/meshbay_node/transport/webrtc_server.py | 10 ++++-- packages/meshbay-node/src/meshbay_node/ui/app.py | 1 - packages/meshbay-node/tests/test_cli_dispatch.py | 36 +++++++++++++++------- 7 files changed, 64 insertions(+), 53 deletions(-) (limited to 'packages/meshbay-node/tests/test_cli_dispatch.py') diff --git a/docs/refactor-groups.md b/docs/refactor-groups.md index d63d858..147afda 100644 --- a/docs/refactor-groups.md +++ b/docs/refactor-groups.md @@ -859,6 +859,16 @@ flag is the same opt-in shape as `transport.js`'s `?trace=1`. ### The CLI +**`--upload-dir` is gone, not deprecated.** It was documented as the old +spelling first, which was wrong and was caught in review. It wrote `upload_dir` +into a *brand-new* `[[groups]]` block, and `GroupConfig.__post_init__` reads +that key by forcing every other root read-only and appending that path as the +one writable one — so `group add --dir X --writable --upload-dir Y` silently +made X read-only. Two mechanisms deciding which directories accept uploads, one +of them invisible, in a group created after the model that replaced it. The +*read* path stays, because an existing node.toml must keep working; that is the +only legitimate use, and nothing writes the key any more. + `member upload` reached the generic usage line for the other `member` verbs — "usage: meshbay-node member upload " — which advertises a removed feature and sends the operator looking for a username it would then reject. It diff --git a/man/meshbay-node.1 b/man/meshbay-node.1 index 890da89..7c7cf96 100644 --- a/man/meshbay-node.1 +++ b/man/meshbay-node.1 @@ -280,15 +280,6 @@ 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" . @@ -442,12 +433,15 @@ named after the directory's basename. Cannot be combined with . .TP .B upload_dir -Deprecated. A separate filesystem path for uploads, from before roots -carried their own read\-write flag. A configuration still using it is -read as a second, writable root and every other root is forced -read\-only. Use -.B writable -on the intended root instead. +Deprecated, and read only. A separate filesystem path for uploads, from before +roots carried their own read\-write flag. An existing configuration using it +still works: it is read as a second, writable root, and every other root of +that group is forced read\-only. Nothing writes it any more \(em there is no +\fB\-\-upload\-dir\fR option \(em because that would be a second mechanism +deciding which directories accept uploads, silently overriding +.BR writable . +To have two writable directories, mark them both +.BR "writable = true" . . .TP .B visibility diff --git a/packages/meshbay-node/src/meshbay_node/daemon.py b/packages/meshbay-node/src/meshbay_node/daemon.py index a9376e7..74fff4c 100644 --- a/packages/meshbay-node/src/meshbay_node/daemon.py +++ b/packages/meshbay-node/src/meshbay_node/daemon.py @@ -1783,9 +1783,6 @@ def main() -> None: help="hub username, for init") parser.add_argument("--dir", default=None, help="shared directory, for group add") - parser.add_argument("--upload-dir", default=None, - help="deprecated: a second read-write root, for group " - "add. Use `root add --writable` instead") parser.add_argument("--yes", action="store_true", help="skip the confirmation for destructive commands") parser.add_argument("--config", type=Path, default=None, @@ -2571,11 +2568,13 @@ def main() -> None: print("usage: meshbay-node group list|add|remove ") sys.exit(1) if not args.target or not args.dir: - print("usage: meshbay-node group add --dir [--upload-dir ]") + print("usage: meshbay-node group add --dir " + "[--no-writable]") print() print("The group must already exist on the hub and be yours. This") - print("only tells the node to host it, and picks the directory.") - print("--upload-dir sets a separate directory for uploaded files.") + print("only tells the node to host it, and picks its first") + print("directory, which accepts uploads unless --no-writable.") + print("Add more with: meshbay-node root add [--writable]") sys.exit(1) cfg = load_config(args.config or DEFAULT_CONFIG_PATH) @@ -2586,17 +2585,10 @@ def main() -> None: writable = args.writable is not False body = {"name": args.target, "shared_dir": args.dir, "writable": writable} - if args.upload_dir: - print("WARNING: --upload-dir is deprecated. The shared directory is " - "read-write by default; use 'meshbay-node root add " - " --writable' for a second one.") - body["upload_dir"] = args.upload_dir out = _daemon_api(cfg, "/api/groups/attach", method="POST", body=body) print(f"{out['name']} ({out['group_id'][:8]}) added to {out['config']}") print(f" shared_dir {out['shared_dir']}" f" ({'read-write' if writable else 'read-only'})") - if out.get("upload_dir"): - print(f" upload_dir {out['upload_dir']}") print() print("Tell the daemon to re-read its config, then give the group a key:") print(" meshbay-node reload") diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index 4c759c8..a10504e 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -394,7 +394,7 @@ async def list_groups(state: dict) -> dict: async def attach_group(state: dict, name: str, shared_dir: str, - upload_dir: str = "", writable: bool = True) -> dict: + writable: bool = True) -> dict: """ Write a new [[groups]] block into node.toml. @@ -439,15 +439,13 @@ async def attach_group(state: dict, name: str, shared_dir: str, f'name = "{group["name"]}"\n' f'visibility = "{group.get("visibility", "private")}"\n' f'join_policy = "{join_policy}"\n') - # Legacy: upload_dir becomes a second writable root - if upload_dir: - upload_path = Path(upload_dir).expanduser().resolve() - if upload_path != path.resolve(): - try: - upload_path.mkdir(parents=True, exist_ok=True) - except OSError as e: - raise OpError(f"Cannot create {upload_path}: {e}") from e - block += f'upload_dir = "{upload_path.as_posix()}"\n' + # No `upload_dir` here. `GroupConfig.__post_init__` still *reads* it, so an + # existing node.toml keeps working — but what it does on read is force every + # other root read-only and append that path as the one writable one, which + # is the model this refactor replaced. Writing it into a group created + # today would mean two mechanisms deciding the same thing, one of them + # invisible: `group add --dir X --writable --upload-dir Y` silently made X + # read-only. A second writable directory is `root add --writable`. block += (f'\n [[groups.roots]]\n' # Forward slashes: a Windows path in a TOML basic string is a # parse error (`\U`, `\a`, ... are escapes). pathlib reads `/`. diff --git a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py index acdbe29..8e357c9 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -2483,11 +2483,14 @@ class WebRTCPeerSession: if not self._has_admin_authority(): self._send({"type": "error", "detail": "No authorized key for this"}) return - upload_dir = str(msg.get("upload_dir", "")).strip() + # `upload_dir` is not read here any more, and a client still sending it + # is ignored rather than obeyed: on load it forces every other root + # read-only, which is the model the RO/RW one replaced. A second + # writable directory is `root_add` with `writable`. self._issue_admin_challenge( OP_GROUP_ATTACH, name, payload={"name": name, "shared_dir": shared_dir, - "upload_dir": upload_dir}, + "writable": bool(msg.get("writable", True))}, group_id="") async def _admin_exec_group_attach( @@ -2501,7 +2504,8 @@ class WebRTCPeerSession: p = pending.get("payload") or {} try: result = await self._run_op( - ops.attach_group, p["name"], p["shared_dir"], p.get("upload_dir", "")) + ops.attach_group, p["name"], p["shared_dir"], + writable=bool(p.get("writable", True))) except ops.OpError as e: self._send({"type": "error", "detail": e.message}) return diff --git a/packages/meshbay-node/src/meshbay_node/ui/app.py b/packages/meshbay-node/src/meshbay_node/ui/app.py index 130c59e..fc6c04a 100644 --- a/packages/meshbay-node/src/meshbay_node/ui/app.py +++ b/packages/meshbay-node/src/meshbay_node/ui/app.py @@ -175,7 +175,6 @@ def create_ui_app(state: dict) -> FastAPI: state, (payload.get("name") or "").strip(), (payload.get("shared_dir") or "").strip(), - upload_dir=(payload.get("upload_dir") or "").strip(), writable=bool(payload.get("writable", True)), )) reload_fn = state.get("reload_fn") diff --git a/packages/meshbay-node/tests/test_cli_dispatch.py b/packages/meshbay-node/tests/test_cli_dispatch.py index 1a01eba..cf91564 100644 --- a/packages/meshbay-node/tests/test_cli_dispatch.py +++ b/packages/meshbay-node/tests/test_cli_dispatch.py @@ -261,18 +261,32 @@ def test_a_removed_verb_says_what_replaced_it(): "the message does not name what replaced it") -def test_the_help_does_not_offer_the_old_upload_directory_as_current(): +def test_there_is_no_way_to_create_a_group_in_the_old_shape(): """ - `--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. + `--upload-dir` is gone, and documenting it as deprecated was the wrong + answer — which is what it got at first. + + It wrote `upload_dir` into a brand-new `[[groups]]` block, and + `GroupConfig.__post_init__` reads that by forcing *every other root + read-only* and appending that path as the one writable one. So + `group add --dir X --writable --upload-dir Y` silently made X read-only: + two mechanisms deciding which directories accept uploads, one of them + invisible, in a group created after the model that replaced it. + + Reading it stays — an existing node.toml must keep working, and that is the + only legitimate use. Writing it does not. """ 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" + assert "--upload-dir" not in source, ( + "the CLI can still create a group in the pre-RO/RW shape") + + from meshbay_node import ops + params = inspect.signature(ops.attach_group).parameters + assert "upload_dir" not in params, ( + "attach_group still writes the legacy key") + + # The read path is deliberately untouched. + from meshbay_node.config import GroupConfig + assert "upload_dir" in inspect.getsource(GroupConfig), ( + "an existing node.toml using upload_dir would stop working") -- cgit v1.2.3