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_ops.py | 100 ++++++++++++++++++++++++++++---- 1 file changed, 90 insertions(+), 10 deletions(-) (limited to 'packages/meshbay-node/tests/test_ops.py') diff --git a/packages/meshbay-node/tests/test_ops.py b/packages/meshbay-node/tests/test_ops.py index 92e32bf..b3f0378 100644 --- a/packages/meshbay-node/tests/test_ops.py +++ b/packages/meshbay-node/tests/test_ops.py @@ -12,11 +12,13 @@ call them. import asyncio import inspect from pathlib import Path +from types import SimpleNamespace import pytest from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey from meshbay_node import ops from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.roots import RootSet from meshbay_node.transport.quic_server import Denylist from conftest import one_root @@ -64,8 +66,8 @@ def test_the_http_adapter_adds_no_logic(): # Every endpoint that performs an operation routes through _op(...). for endpoint in ("operator_pair", "create_invite", "revoke_member", "unpin_member", "init_gek", "attach_group", "delete_file", - "add_root", "remove_root", "set_member_upload", - "reload_config"): + "add_root", "remove_root", "update_root", + "eject_root", "plug_root", "reload_config"): start = source.index(f"async def {endpoint}(") body = source[start:start + 700] assert "_op(" in body.split("\n\n")[0] + body, ( @@ -181,25 +183,103 @@ async def test_an_unhosted_group_offers_what_it_does_host(tmp_path): assert exc.value.extra.get("available") -# ── Upload policy (set_member_upload) ─────────────────────────────────────── +# ── Upload policy (per-root writable) ─────────────────────────────────────── -async def test_set_member_upload_toggles_and_persists(tmp_path): +async def test_the_group_wide_upload_switch_is_gone(tmp_path): + """ + `set_member_upload` was the whole of the old policy, and it is deliberately + not here any more — RO/RW on the root replaced it. A wrapper kept "for + compatibility" would be a second way to decide who writes to the operator's + disk, and two answers to that question is how C1 and C6 both happened. + """ + assert not hasattr(ops, "set_member_upload") + from meshbay_node.roster import Roster + assert not hasattr(Roster, "set_member_upload") + assert not hasattr(Roster, "member_upload_allowed") + + +async def test_eject_and_plug_persist_through_the_roster(tmp_path): + """ + The state has to outlive the process: an operator ejects a drive, unplugs + it, and restarts the node — and the rescan that follows must not read the + empty mount point as an erased library. + """ from meshbay_node.roster import Roster state = _state(tmp_path) + usb = tmp_path / "USB" + usb.mkdir() + state["groups_ctx"]["g" * 32]["roots"] = RootSet.build( + [{"path": str(usb), "removable": True, "writable": True}]) + state["config"] = SimpleNamespace( + groups=[SimpleNamespace(id="g" * 32, roots=[])]) roster = Roster(db_path=tmp_path / "roster.db") await roster.open() state["roster"] = roster state["node_user_id"] = "operator" + try: + out = await ops.eject_root(state, "g" * 32, "USB") + assert out["status"] == "ejected" + assert await roster.ejected_roots("g" * 32) == {"usb"} + assert out["roots"][0]["ejected"] is True + assert out["roots"][0]["available"] is False + + out = await ops.plug_root(state, "g" * 32, "USB") + assert out["status"] == "plugged" + assert await roster.ejected_roots("g" * 32) == set() + finally: + await roster.close() - out = await ops.set_member_upload(state, "g" * 32, True) - assert out["allowed"] is True - assert state["groups_ctx"]["g" * 32]["member_upload"] is True +async def test_a_root_that_is_not_removable_cannot_be_ejected(tmp_path): + """ + Eject means "I am about to unplug this". On a directory that is not on a + removable device it would hide a library with no way for the safety net to + notice anything happened, and nothing to plug back in. + """ + from meshbay_node.roster import Roster + state = _state(tmp_path) + fixed = tmp_path / "Fixed" + fixed.mkdir() + state["groups_ctx"]["g" * 32]["roots"] = RootSet.build( + [{"path": str(fixed), "writable": True}]) + state["config"] = SimpleNamespace( + groups=[SimpleNamespace(id="g" * 32, roots=[])]) + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + state["roster"] = roster + try: + with pytest.raises(ops.OpError, match="removable"): + await ops.eject_root(state, "g" * 32, "Fixed") + finally: + await roster.close() - out2 = await ops.set_member_upload(state, "g" * 32, False) - assert out2["allowed"] is False - assert state["groups_ctx"]["g" * 32]["member_upload"] is False +async def test_plugging_a_drive_that_is_not_there_is_refused(tmp_path): + """ + Clearing the flag while the device is still absent would restart the + watchdog on a missing path and hand the next reconcile an empty directory — + the deletion storm the eject was there to prevent, produced by the recovery. + """ + from meshbay_node.roster import Roster + state = _state(tmp_path) + usb = tmp_path / "USB" + usb.mkdir() + roots = RootSet.build([{"path": str(usb), "removable": True}]) + roots.roots[0].ejected = True + roots.roots[0].available = False + state["groups_ctx"]["g" * 32]["roots"] = roots + state["config"] = SimpleNamespace( + groups=[SimpleNamespace(id="g" * 32, roots=[])]) + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + state["roster"] = roster + usb.rmdir() + try: + with pytest.raises(ops.OpError, match="device connected"): + await ops.plug_root(state, "g" * 32, "USB") + assert roots.roots[0].ejected is True + finally: + await roster.close() # ── Reload ────────────────────────────────────────────────────────────────── -- cgit v1.2.3 From 4e6d6573003b06dd58268602d50a98988d4ce3d0 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 17:58:27 +0200 Subject: test(node): the backslash-path test modelled the wrong platform MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It asserted `Path(win_dir).as_posix() == "C:/Users/alice/Media"`, which is only true where `Path` is a `WindowsPath`. On every other machine a backslash is an ordinary filename character, `as_posix()` converts nothing, and the test failed against correct code — so it has never passed on this suite's usual host, and never guarded anything there. `PureWindowsPath` names the flavour and makes it the same assertion on all three platforms. The round trip also only ever proved that `as_posix()` produces a parseable string, never that the config writer calls it — which is the defect, and one no Linux machine can reproduce: the file is written, parsed and served correctly here and fails on the operator's Windows box. A second test reads ops.py for every f-string landing on the right of a TOML `path =` and requires `as_posix()` in it. Weak evidence, and the only kind available for a platform the suite does not run on; checked to fail with the call removed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- packages/meshbay-node/tests/test_ops.py | 46 ++++++++++++++++++++++++++++----- 1 file changed, 40 insertions(+), 6 deletions(-) (limited to 'packages/meshbay-node/tests/test_ops.py') diff --git a/packages/meshbay-node/tests/test_ops.py b/packages/meshbay-node/tests/test_ops.py index b3f0378..c118b5a 100644 --- a/packages/meshbay-node/tests/test_ops.py +++ b/packages/meshbay-node/tests/test_ops.py @@ -11,7 +11,7 @@ call them. import asyncio import inspect -from pathlib import Path +from pathlib import Path, PureWindowsPath from types import SimpleNamespace import pytest @@ -359,13 +359,47 @@ def test_update_node_toml_forces_lf_and_keeps_standalone_comments(tmp_path): def test_a_backslash_path_written_into_node_toml_stays_parseable(): - # attach_group / add_root / init embed a directory into a TOML basic string. - # A raw Windows path there (drive + backslash + "Users" + ...) is a parse - # error since backslash sequences are escapes; the code writes as_posix(). + """ + attach_group and add_root embed a directory into a TOML basic string. A raw + Windows path there is a parse error, because backslash sequences are escapes + (`\\U`, `\\a`, ...); the code writes `as_posix()` and pathlib reads `/` back + on Windows. + + `PureWindowsPath`, not `Path`: on this suite's usual machine `Path` is a + `PosixPath`, where a backslash is an ordinary filename character and + `as_posix()` converts nothing — so the test modelled the wrong platform and + failed everywhere except the one it was written for. Naming the flavour + explicitly is what makes it the same assertion on all three. + """ import tomllib bs = chr(92) win_dir = f"C:{bs}Users{bs}alice{bs}Media" - assert tomllib.loads(f'path = "{Path(win_dir).as_posix()}"\n')["path"] == \ - "C:/Users/alice/Media" + + assert tomllib.loads( + f'path = "{PureWindowsPath(win_dir).as_posix()}"\n' + )["path"] == "C:/Users/alice/Media" + with pytest.raises(tomllib.TOMLDecodeError): tomllib.loads(f'path = "{win_dir}"\n') # the bug this guards against + + +def test_every_path_written_into_node_toml_goes_through_as_posix(): + """ + The half the round trip above cannot see. + + Proving `as_posix()` produces a parseable string says nothing about whether + the code calls it, and this is a defect no Linux machine can reproduce: the + config is written, parsed and served correctly here, and fails on the + operator's Windows box. So the source is read for the shape instead — + weak evidence, and the only kind available for a platform the suite does + not run on. + """ + import re + source = inspect.getsource(ops) + # Every f-string interpolation that lands on the right of a TOML `path =`. + writes = re.findall(r'path\s*=\s*\\?"\{([^}]+)\}', source) + assert writes, "no TOML path writer found — did the config writer move?" + for expr in writes: + assert "as_posix()" in expr, ( + f'node.toml path written as `{expr}` — a Windows path needs ' + f'as_posix(), or the file it lands in will not parse') -- cgit v1.2.3