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/conftest.py | 10 +- .../meshbay-node/tests/test_apps_enabled_policy.py | 4 +- packages/meshbay-node/tests/test_cli_dispatch.py | 9 + .../tests/test_member_upload_policy.py | 176 -------------- packages/meshbay-node/tests/test_node_status.py | 117 ++++++++- packages/meshbay-node/tests/test_ops.py | 100 +++++++- .../meshbay-node/tests/test_root_availability.py | 11 +- packages/meshbay-node/tests/test_root_eject.py | 268 +++++++++++++++++++++ .../tests/test_root_writable_policy.py | 203 ++++++++++++++++ packages/meshbay-node/tests/test_roots.py | 81 +++++-- .../tests/test_scan_settings_policy.py | 2 +- .../tests/test_security_regressions.py | 145 ++++++++++- 12 files changed, 895 insertions(+), 231 deletions(-) delete mode 100644 packages/meshbay-node/tests/test_member_upload_policy.py create mode 100644 packages/meshbay-node/tests/test_root_eject.py create mode 100644 packages/meshbay-node/tests/test_root_writable_policy.py (limited to 'packages/meshbay-node/tests') diff --git a/packages/meshbay-node/tests/conftest.py b/packages/meshbay-node/tests/conftest.py index 20724aa..3dc9cd9 100644 --- a/packages/meshbay-node/tests/conftest.py +++ b/packages/meshbay-node/tests/conftest.py @@ -24,14 +24,18 @@ win32_todo = pytest.mark.skipif( ) -def one_root(path: Path, *, name: str = "", kind: str = "generic") -> RootSet: +def one_root(path: Path, *, name: str = "", kind: str = "generic", + writable: bool = True) -> RootSet: """ - A RootSet with a single root over `path`, receiving uploads. + A RootSet with a single writable root over `path`. The equivalent of the old `shared_dir`. Note what it implies for assertions: a file directly in `path` now has `entry.path == `, not `""` — every index path carries its root name, in a group with one root as much as in a group with five. + + Writable by default because most callers are testing something else and + want a root an upload can reach. `writable=False` is the read-only group. """ return RootSet.build([{"path": str(path), "name": name, "kind": kind, - "upload": True}]) + "writable": writable}]) diff --git a/packages/meshbay-node/tests/test_apps_enabled_policy.py b/packages/meshbay-node/tests/test_apps_enabled_policy.py index 671005a..ac44ab3 100644 --- a/packages/meshbay-node/tests/test_apps_enabled_policy.py +++ b/packages/meshbay-node/tests/test_apps_enabled_policy.py @@ -1,7 +1,7 @@ """ The operator decides which group "applications" (Chat, Files, ...) are shown. -Same shape as `test_member_upload_policy.py`, because it is the same kind of +Same shape as `test_root_writable_policy.py`, because it is the same kind of setting: changed by a signed operator instruction, stored on the node rather than the hub, and safe for an existing group to have never heard of. The two things specific to this one: the whole set is signed in one message rather @@ -88,7 +88,7 @@ async def test_a_request_with_nobody_to_authorize_it_is_refused(tmp_path): async def test_changing_it_needs_a_signature(tmp_path): """The request only ever produces a challenge. Nothing is applied until a - signature over the transcript verifies — the same path as member_upload.""" + signature over the transcript verifies — the same path as the root ops.""" session = _session(tmp_path, "op", operator="op") session._has_admin_authority = lambda: True issued = [] 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"], diff --git a/packages/meshbay-node/tests/test_member_upload_policy.py b/packages/meshbay-node/tests/test_member_upload_policy.py deleted file mode 100644 index b1dc0cb..0000000 --- a/packages/meshbay-node/tests/test_member_upload_policy.py +++ /dev/null @@ -1,176 +0,0 @@ -""" -The operator can close uploading to everyone but themselves. - -The point of these tests is the difference between a hidden button and a closed -door. The interface stops offering the control, which is a courtesy to the -people who are not trying; **the node refuses the upload**, which is the part -that holds against someone who is. A member who kept an old tab open, or who -speaks MNP directly, gets the same answer as everyone else. - -Two further things are worth holding: - -* the setting is changed by a **signed** operator instruction. A node that took - it from an unsigned message would let any member turn it back on, and the - control would be a suggestion; -* it is stored on the **node**, not the hub. A hub that could decide who may - write to the operator's disk is a hub with authority over the node, which is - the thing this whole design is arranged to avoid. -""" - -import base64 -from pathlib import Path - -import pytest -from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey - -from meshbay_common.adminop import OP_MEMBER_UPLOAD -from meshbay_node.indexer.group_index import GroupIndex -from meshbay_node.roster import Roster -from meshbay_node.transport.webrtc_server import WebRTCPeerSession - -from conftest import one_root - -pytestmark = pytest.mark.asyncio - - -def _session(tmp_path: Path, user_id: str, *, member_upload: bool, - operator: str | None = None) -> WebRTCPeerSession: - shared_root = tmp_path / "shared" - shared_root.mkdir(exist_ok=True) - index = GroupIndex(group_id="g" * 32, sk_node=Ed25519PrivateKey.generate()) - ctx = { - "roots": one_root(shared_root), - "index": index, - "sk_node": index.sk_node, - "member_upload": member_upload, - "node_user_id": operator, - } - session = WebRTCPeerSession.__new__(WebRTCPeerSession) - session._ctx = ctx - session._group_id = None - session._user_id = user_id - session._pk_user = "" - session._uploads = {} - session.sent = [] - session._send = session.sent.append - session._audit = lambda *a, **k: None - return session - - -def _upload(session, filename="clip.mp4", body=b"bytes"): - session._do_file_upload({ - "filename": filename, "chunk_index": 0, "total_chunks": 1, - "data": base64.b64encode(body).decode(), - }) - - -def _uploads_dir(session) -> Path: - return session._ctx["roots"].upload_root.path / "uploads" - - -# ── The door, not the button ──────────────────────────────────────────────── - -async def test_a_member_cannot_upload_when_it_is_turned_off(tmp_path): - session = _session(tmp_path, "member-1", member_upload=False, - operator="the-operator") - _upload(session) - - assert not (_uploads_dir(session) / "clip.mp4").exists(), ( - "the file was written even though uploading is off — the setting is " - "decorative and the hidden button was the whole control") - refusal = [m for m in session.sent if m.get("type") == "error"] - assert refusal and refusal[0].get("code") == "member_upload_off" - - -async def test_the_operator_can_still_upload(tmp_path): - """Otherwise turning it off locks the operator out of their own node, and - the only way back is a config file and a restart.""" - session = _session(tmp_path, "the-operator", member_upload=False, - operator="the-operator") - _upload(session) - - assert (_uploads_dir(session) / "clip.mp4").read_bytes() == b"bytes" - - -async def test_members_upload_normally_when_it_is_on(tmp_path): - session = _session(tmp_path, "member-1", member_upload=True, - operator="the-operator") - _upload(session) - - assert (_uploads_dir(session) / "clip.mp4").read_bytes() == b"bytes" - - -async def test_a_node_that_never_heard_of_the_setting_still_accepts_uploads(tmp_path): - """An existing node's context has no such key. The absence must read as - "allowed", or upgrading the node silently closes every group.""" - session = _session(tmp_path, "member-1", member_upload=True, - operator="the-operator") - del session._ctx["member_upload"] - _upload(session) - - assert (_uploads_dir(session) / "clip.mp4").read_bytes() == b"bytes" - - -# ── Who may change it ─────────────────────────────────────────────────────── - -async def test_changing_it_needs_a_signature(tmp_path): - """ - The request only ever produces a challenge. Nothing is applied until a - signature over the transcript verifies — the same path as removing a member. - """ - session = _session(tmp_path, "member-1", member_upload=True, - operator="the-operator") - session._has_admin_authority = lambda: True - issued = [] - session._issue_admin_challenge = lambda op, subject: issued.append((op, subject)) - - session._do_member_upload({"allowed": False}) - - assert issued == [(OP_MEMBER_UPLOAD, "off")] - assert session._ctx["member_upload"] is True, "applied before it was signed" - - -async def test_the_subject_names_the_outcome_not_the_operation(tmp_path): - """The operator is shown the subject before signing. "member_upload" tells - them nothing; "off" tells them what they are about to do.""" - session = _session(tmp_path, "op", member_upload=False, operator="op") - session._has_admin_authority = lambda: True - issued = [] - session._issue_admin_challenge = lambda op, subject: issued.append((op, subject)) - - session._do_member_upload({"allowed": True}) - - assert issued == [(OP_MEMBER_UPLOAD, "on")] - - -async def test_a_request_with_nobody_to_authorize_it_is_refused(tmp_path): - session = _session(tmp_path, "member-1", member_upload=True, - operator="the-operator") - session._has_admin_authority = lambda: False - - session._do_member_upload({"allowed": False}) - - assert [m for m in session.sent if m.get("type") == "error"] - - -# ── Where it is stored ────────────────────────────────────────────────────── - -async def test_the_setting_lives_on_the_node_and_survives_a_restart(tmp_path): - roster = Roster(db_path=tmp_path / "roster.db") - await roster.open() - try: - assert await roster.member_upload_allowed("g1") is True, ( - "absent must mean allowed, or an upgrade closes every group") - await roster.set_member_upload("g1", False, set_by="op") - assert await roster.member_upload_allowed("g1") is False - finally: - await roster.close() - - reopened = Roster(db_path=tmp_path / "roster.db") - await reopened.open() - try: - assert await reopened.member_upload_allowed("g1") is False - assert await reopened.member_upload_allowed("g2") is True, ( - "one group's setting must not answer for another") - finally: - await reopened.close() diff --git a/packages/meshbay-node/tests/test_node_status.py b/packages/meshbay-node/tests/test_node_status.py index b56eb6e..091b1db 100644 --- a/packages/meshbay-node/tests/test_node_status.py +++ b/packages/meshbay-node/tests/test_node_status.py @@ -255,7 +255,7 @@ async def test_add_root_creates_directory_and_returns_info(tmp_path): from meshbay_node.config import NodeConfig, GroupConfig, RootSpec cfg = GroupConfig(id=GROUP, name="test", roots=[ - RootSpec(path=str(shared), name="shared", kind="generic", upload=True), + RootSpec(path=str(shared), name="shared", kind="generic", writable=True), ]) conf = tmp_path / "node.toml" @@ -295,7 +295,7 @@ async def test_remove_root_requires_at_least_one_remaining(tmp_path): from meshbay_node.config import GroupConfig, RootSpec, NodeConfig cfg = GroupConfig(id=GROUP, name="test", roots=[ - RootSpec(path=str(shared), name="shared", kind="generic", upload=True), + RootSpec(path=str(shared), name="shared", kind="generic", writable=True), ]) node_cfg = NodeConfig.__new__(NodeConfig) node_cfg.groups = [cfg] @@ -314,16 +314,22 @@ async def test_remove_root_requires_at_least_one_remaining(tmp_path): await ops.remove_root(state, GROUP, "shared") -async def test_remove_root_refuses_upload_root(tmp_path): - d1 = tmp_path / "uploads" +async def test_removing_a_writable_root_is_allowed(tmp_path): + """ + It used to be refused: with one designated upload root, removing it left + the group with nowhere to put an upload and no way to say so. Several roots + can be writable now, and a group with none is a valid read-only group — so + the refusal would be protecting a state that is no longer special. + """ + d1 = tmp_path / "incoming" d2 = tmp_path / "shared" d1.mkdir() d2.mkdir() from meshbay_node.config import GroupConfig, RootSpec, NodeConfig cfg = GroupConfig(id=GROUP, name="test", roots=[ - RootSpec(path=str(d1), name="uploads", kind="generic", upload=True), - RootSpec(path=str(d2), name="shared", kind="generic", upload=False), + RootSpec(path=str(d1), name="incoming", kind="generic", writable=True), + RootSpec(path=str(d2), name="shared", kind="generic", writable=False), ]) node_cfg = NodeConfig.__new__(NodeConfig) node_cfg.groups = [cfg] @@ -331,7 +337,7 @@ async def test_remove_root_refuses_upload_root(tmp_path): conf = tmp_path / "node.toml" conf.write_text( f'[[groups]]\nid = "{GROUP}"\nname = "test"\n\n' - f' [[groups.roots]]\n path = "{d1}"\n name = "uploads"\n upload = true\n\n' + f' [[groups.roots]]\n path = "{d1}"\n name = "incoming"\n writable = true\n\n' f' [[groups.roots]]\n path = "{d2}"\n name = "shared"\n') roots = RootSet.build([asdict(r) for r in cfg.roots]) index = GroupIndex(group_id=GROUP, sk_node=Ed25519PrivateKey.generate()) @@ -340,8 +346,97 @@ async def test_remove_root_refuses_upload_root(tmp_path): "config_path": str(conf), "groups_ctx": {GROUP: {"index": index, "roots": roots, "gek": b"\x01" * 32}}, } - with pytest.raises(ops.OpError, match="upload root"): - await ops.remove_root(state, GROUP, "uploads") + result = await ops.remove_root(state, GROUP, "incoming") + assert result["status"] == "removed" + assert [r["name"] for r in result["roots"]] == ["shared"] + assert conf.read_text().count("[[groups.roots]]") == 1 + + +async def test_update_root_rewrites_the_flags_in_node_toml(tmp_path): + """ + The flags live in the operator's config file, so they survive a restart — + and the file is hand-written and full of comments, so the change is a line + edit rather than a round trip through a TOML writer that would discard + every one of them. + """ + d1 = tmp_path / "media" + d1.mkdir() + + from meshbay_node.config import GroupConfig, RootSpec, NodeConfig + cfg = GroupConfig(id=GROUP, name="test", roots=[ + RootSpec(path=str(d1), name="media", kind="generic", writable=False), + ]) + node_cfg = NodeConfig.__new__(NodeConfig) + node_cfg.groups = [cfg] + + conf = tmp_path / "node.toml" + conf.write_text( + f'[[groups]]\nid = "{GROUP}"\nname = "test"\n\n' + f' [[groups.roots]]\n' + f' # the operator explained this one to themselves\n' + f' path = "{d1}"\n name = "media"\n') + roots = RootSet.build([asdict(r) for r in cfg.roots]) + index = GroupIndex(group_id=GROUP, sk_node=Ed25519PrivateKey.generate()) + state = { + "config": node_cfg, + "config_path": str(conf), + "groups_ctx": {GROUP: {"index": index, "roots": roots, "gek": b"\x01" * 32}}, + } + + result = await ops.update_root(state, GROUP, "media", + writable=True, removable=True) + assert result["status"] == "updated" + text = conf.read_text() + assert "writable = true" in text + assert "removable = true" in text + assert "the operator explained this one to themselves" in text, ( + "the config file was rewritten instead of edited") + + # And the live root set agrees immediately, without waiting for a reload: + # the loopback API reads it, and an operator who toggles a switch and sees + # it snap back assumes the change did not take. + assert roots.roots[0].writable is True + assert roots.roots[0].removable is True + + # A second call that changes nothing must not append a duplicate line. + await ops.update_root(state, GROUP, "media", writable=True, removable=True) + assert conf.read_text().count("writable =") == 1 + + +async def test_update_root_replaces_a_legacy_upload_line(tmp_path): + """ + A config written before the refactor says `upload = true`. Leaving it in + place next to a new `writable` line would give the file two answers, and + `RootSet.build` prefers `writable` — so the stale one would sit there + contradicting the running node for as long as anyone read it. + """ + d1 = tmp_path / "media" + d1.mkdir() + + from meshbay_node.config import GroupConfig, RootSpec, NodeConfig + cfg = GroupConfig(id=GROUP, name="test", roots=[ + RootSpec(path=str(d1), name="media", kind="generic", writable=True), + ]) + node_cfg = NodeConfig.__new__(NodeConfig) + node_cfg.groups = [cfg] + + conf = tmp_path / "node.toml" + conf.write_text( + f'[[groups]]\nid = "{GROUP}"\nname = "test"\n\n' + f' [[groups.roots]]\n path = "{d1}"\n name = "media"\n' + f' upload = true\n') + roots = RootSet.build([asdict(r) for r in cfg.roots]) + index = GroupIndex(group_id=GROUP, sk_node=Ed25519PrivateKey.generate()) + state = { + "config": node_cfg, + "config_path": str(conf), + "groups_ctx": {GROUP: {"index": index, "roots": roots, "gek": b"\x01" * 32}}, + } + + await ops.update_root(state, GROUP, "media", writable=False) + text = conf.read_text() + assert "upload = true" not in text + assert "writable = false" in text async def test_remove_root_succeeds_with_two_roots(tmp_path): @@ -352,8 +447,8 @@ async def test_remove_root_succeeds_with_two_roots(tmp_path): from meshbay_node.config import GroupConfig, RootSpec, NodeConfig cfg = GroupConfig(id=GROUP, name="test", roots=[ - RootSpec(path=str(d1), name="dir1", kind="generic", upload=True), - RootSpec(path=str(d2), name="dir2", kind="generic", upload=False), + RootSpec(path=str(d1), name="dir1", kind="generic", writable=True), + RootSpec(path=str(d2), name="dir2", kind="generic", writable=False), ]) node_cfg = NodeConfig.__new__(NodeConfig) node_cfg.groups = [cfg] 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 ────────────────────────────────────────────────────────────────── diff --git a/packages/meshbay-node/tests/test_root_availability.py b/packages/meshbay-node/tests/test_root_availability.py index 0201c1f..d514dee 100644 --- a/packages/meshbay-node/tests/test_root_availability.py +++ b/packages/meshbay-node/tests/test_root_availability.py @@ -26,9 +26,12 @@ from meshbay_node.roots import RootSet pytestmark = pytest.mark.asyncio -def _roots(*paths: Path) -> RootSet: +def _roots(*paths: Path, removable: bool = False) -> RootSet: specs = [{"path": str(p)} for p in paths] - specs[0]["upload"] = True + specs[0]["writable"] = True + if removable: + for spec in specs: + spec["removable"] = True return RootSet.build(specs) @@ -117,7 +120,9 @@ async def test_members_are_told_which_roots_are_unavailable(tmp_path): idx = await _indexer(_roots(films)) assert idx.index.roots == [ - {"name": "Films", "kind": "generic", "available": True, "upload": True}] + {"name": "Films", "kind": "generic", "available": True, + "writable": True, "removable": False, "ejected": False, + "upload": True}] (films / "a.mkv").unlink() films.rmdir() diff --git a/packages/meshbay-node/tests/test_root_eject.py b/packages/meshbay-node/tests/test_root_eject.py new file mode 100644 index 0000000..0ec36a4 --- /dev/null +++ b/packages/meshbay-node/tests/test_root_eject.py @@ -0,0 +1,268 @@ +""" +Safe eject, and the surprise unplug it exists to survive. + +`test_root_availability.py` pins the freeze: a root that goes away keeps its +entries. This pins the half the operator drives — telling the node the drive is +about to leave, and telling it the drive is back. + +The distinction that makes any of this work is that `ejected` and `is_live()` +are separate answers. Between clicking Eject and physically unplugging, the +directory is still readable; a design that recomputed availability from the +filesystem alone would flip the root straight back to available and start +serving files from a disk somebody has their hand on. + +The other property here is that the flag is *persisted*. It reached the roster +in the first implementation and was never read back, so a restart — which is +exactly what an operator does after noticing a drive fell off — silently undid +the eject, and the next scan read an empty mount point as an erased library. +""" + +import asyncio +from pathlib import Path + +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey + +from meshbay_node.indexer.indexer import DirectoryIndexer +from meshbay_node.roots import RootSet +from meshbay_node.roster import Roster + +pytestmark = pytest.mark.asyncio + + +def _roots(*paths: Path, removable: bool = True) -> RootSet: + return RootSet.build([ + {"path": str(p), "removable": removable} for p in paths]) + + +async def _indexer(roots: RootSet, **kw) -> DirectoryIndexer: + idx = DirectoryIndexer(roots=roots, group_id="g" * 32, + sk_node=Ed25519PrivateKey.generate(), gek=None, **kw) + await idx.initial_scan() + return idx + + +def _names(idx: DirectoryIndexer) -> set[str]: + return {e.name for e in idx.index.entries} + + +# ── The two states are not the same question ───────────────────────────────── + +async def test_ejecting_hides_a_root_that_is_still_readable(tmp_path): + """ + The whole point of an eject button: the operator says the drive is leaving + *before* it leaves. The directory is still there and still readable at this + moment, so anything deriving availability from the filesystem would refuse + to believe it. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films) + idx = await _indexer(roots) + idx.eject_root("Films") + + assert films.is_dir(), "the drive has not been unplugged yet" + assert roots.roots[0].is_live() is True + assert roots.roots[0].available is False + assert idx.index.roots[0]["ejected"] is True + assert idx.index.roots[0]["available"] is False + + +async def test_an_eject_freezes_entries_rather_than_dropping_them(tmp_path): + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + (films / "b.mkv").write_bytes(b"b") + + idx = await _indexer(_roots(films)) + idx.eject_root("Films") + + assert _names(idx) == {"a.mkv", "b.mkv"}, "eject deleted entries" + + +async def test_reconciling_does_not_un_eject_a_root(tmp_path): + """ + The backstop runs every minute regardless. An ejected root whose directory + is still readable must stay ejected, or the operator's eject lasts until + the next tick. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films) + idx = await _indexer(roots) + idx.eject_root("Films") + await idx.reconcile() + + assert roots.roots[0].ejected is True + assert roots.roots[0].available is False + + +async def test_plugging_back_relists_the_files(tmp_path): + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films) + idx = await _indexer(roots) + idx.eject_root("Films") + await idx.plug_root("Films") + + assert roots.roots[0].ejected is False + assert roots.roots[0].available is True + assert _names(idx) == {"a.mkv"} + + +async def test_what_changed_while_unplugged_is_picked_up_on_plug(tmp_path): + """ + A drive people take away comes back different. The plug pass has to see + that, or the index describes a library that no longer exists on the disk + the node is about to serve from. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films) + idx = await _indexer(roots) + idx.eject_root("Films") + + (films / "a.mkv").unlink() + (films / "c.mkv").write_bytes(b"c") + + await idx.plug_root("Films") + assert _names(idx) == {"c.mkv"} + + +# ── The surprise unplug ────────────────────────────────────────────────────── + +async def test_a_removable_root_that_vanishes_is_auto_ejected(tmp_path): + """ + Nobody clicks Eject when they are in a hurry. A removable root whose path + disappears is treated as ejected rather than merely unavailable, so it does + not silently come back the moment the same mount point is readable again — + which on a machine with automount is any other drive, or an empty stub. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films) + idx = await _indexer(roots) + + (films / "a.mkv").unlink() + films.rmdir() + await idx.reconcile() + + assert roots.roots[0].ejected is True + assert _names(idx) == {"a.mkv"}, "the library was treated as erased" + + +async def test_a_non_removable_root_is_not_auto_ejected(tmp_path): + """ + The counter-property. Auto-eject requires the operator to have said the + device is removable; an ordinary directory that briefly fails to stat must + keep the old behaviour and come back on its own. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + roots = _roots(films, removable=False) + idx = await _indexer(roots) + + (films / "a.mkv").unlink() + films.rmdir() + await idx.reconcile() + assert roots.roots[0].ejected is False + assert roots.roots[0].available is False + + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + await idx.reconcile() + assert roots.roots[0].available is True + + +async def test_an_auto_eject_is_reported_so_it_can_be_persisted(tmp_path): + """ + The flag has to outlive the process. The first version of this set it in + memory only, so restarting the node — which is what an operator does after + noticing a drive fell off — cleared it, and the scan that followed read the + empty mount point as a deletion of the whole library. + """ + films = tmp_path / "Films" + films.mkdir() + (films / "a.mkv").write_bytes(b"a") + + seen: list[tuple[str, bool]] = [] + + async def record(name: str, ejected: bool) -> None: + seen.append((name, ejected)) + + roots = _roots(films) + idx = await _indexer(roots, on_root_ejected=record) + + (films / "a.mkv").unlink() + films.rmdir() + await idx.reconcile() + + assert seen == [("Films", True)] + + # And only once, however many times the backstop runs afterwards. + await idx.reconcile() + await idx.reconcile() + assert seen == [("Films", True)] + + +# ── Restoring the flag ─────────────────────────────────────────────────────── + +async def test_a_root_built_as_ejected_starts_unavailable(tmp_path): + """ + What the daemon does with what the roster remembers. `available` must not + be left at its default `True` here, or the group serves a drive that is not + there for as long as it takes the first reconcile to run. + """ + films = tmp_path / "Films" + films.mkdir() + roots = RootSet.build([{"path": str(films), "removable": True, + "ejected": True}]) + assert roots.roots[0].ejected is True + assert roots.roots[0].available is False + + +async def test_the_roster_round_trips_the_ejected_set(tmp_path): + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + try: + assert await roster.ejected_roots("g1") == set() + + await roster.set_root_ejected("g1", "Films", True, set_by="op") + await roster.set_root_ejected("g1", "Music", False, set_by="op") + assert await roster.ejected_roots("g1") == {"films"} + + # Another group's drives are its own. + assert await roster.ejected_roots("g2") == set() + + await roster.set_root_ejected("g1", "Films", False, set_by="op") + assert await roster.ejected_roots("g1") == set() + finally: + await roster.close() + + +async def test_the_ejected_key_is_case_folded(tmp_path): + """ + Root names are compared without regard to case everywhere else, and a key + that did not fold would let `Films` and `films` disagree about the same + drive — on Windows and macOS, the same directory. + """ + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + try: + await roster.set_root_ejected("g1", "FILMS", True, set_by="op") + assert await roster.ejected_roots("g1") == {"films"} + assert Roster.root_ejected_key("Films") == Roster.root_ejected_key("FILMS") + finally: + await roster.close() diff --git a/packages/meshbay-node/tests/test_root_writable_policy.py b/packages/meshbay-node/tests/test_root_writable_policy.py new file mode 100644 index 0000000..da95032 --- /dev/null +++ b/packages/meshbay-node/tests/test_root_writable_policy.py @@ -0,0 +1,203 @@ +""" +Who may write to the operator's disk, now that RO/RW on the root decides it. + +This replaces `test_member_upload_policy.py`. The old model had two orthogonal +controls — one root designated as the upload target, and a group-wide +`member_upload` switch — and collapsed into one property per root: `writable`. +The properties worth keeping from the old file survive the change unaltered: + +* the interface hiding a control is a courtesy to the people who are not + trying; **the node refusing is the part that holds** against someone who is. + A member with an old tab open, or one speaking MNP directly, gets the same + answer. That half is pinned in `test_security_regressions.py`, next to the + overwrite properties it belongs with; +* the setting is changed by a **signed** operator instruction, or it is a + suggestion any member can undo; +* it is stored on the **node**, never the hub. A hub that could decide who + writes to the operator's disk would have authority over the node. + +And one that is new: the *old* message must no longer be able to change +anything. A deprecated instruction that still works is not deprecated, and this +one would reopen uploads group-wide. +""" + +import base64 +from pathlib import Path + +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey + +from meshbay_common.adminop import OP_ROOT_UPDATE, OP_ROOT_EJECT, OP_ROOT_PLUG +from meshbay_common.protocol import MNP +from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.roots import RootSet +from meshbay_node.transport.webrtc_server import WebRTCPeerSession + +pytestmark = pytest.mark.asyncio + + +def _session(tmp_path: Path, user_id: str, *, + writable: bool = True, + operator: str | None = None) -> WebRTCPeerSession: + shared_root = tmp_path / "shared" + shared_root.mkdir(exist_ok=True) + index = GroupIndex(group_id="g" * 32, sk_node=Ed25519PrivateKey.generate()) + ctx = { + "roots": RootSet.build([{"path": str(shared_root), "writable": writable}]), + "index": index, + "sk_node": index.sk_node, + "node_user_id": operator, + } + session = WebRTCPeerSession.__new__(WebRTCPeerSession) + session._ctx = ctx + session._group_id = "g" * 32 + session._user_id = user_id + session._pk_user = "" + session._uploads = {} + session.sent = [] + session._send = session.sent.append + session._audit = lambda *a, **k: None + return session + + +def _upload(session, filename="clip.mp4", body=b"bytes"): + session._do_file_upload({ + "filename": filename, "root": "shared", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(body).decode(), + }) + + +def _uploads_dir(session) -> Path: + return session._ctx["roots"].roots[0].path / "uploads" + + +# ── The door, not the button ───────────────────────────────────────────────── + +async def test_a_member_cannot_upload_to_a_read_only_root(tmp_path): + session = _session(tmp_path, "member-1", writable=False) + _upload(session) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + assert not _uploads_dir(session).exists() + + +async def test_members_upload_normally_to_a_writable_root(tmp_path): + session = _session(tmp_path, "member-1", writable=True) + _upload(session) + + assert not [m for m in session.sent if m.get("type") == "error"] + assert (_uploads_dir(session) / "clip.mp4").read_bytes() == b"bytes" + + +async def test_read_only_binds_the_operator_too(tmp_path): + """ + The old model exempted the operator, because the switch was about *members*. + RO is about the directory: a published library is read-only for everyone, and + an exception for admin authority is how a rule turns into a default. + """ + session = _session(tmp_path, "the-operator", writable=False, + operator="the-operator") + session._is_node_admin = lambda: True + _upload(session) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + + +# ── Signed, or it is a suggestion ──────────────────────────────────────────── + +def _capture_challenges(session) -> list[tuple[str, str]]: + issued: list[tuple[str, str]] = [] + + def issue(op, subject, **kw): + issued.append((op, subject)) + + session._issue_admin_challenge = issue + session._has_admin_authority = lambda: True + return issued + + +async def test_changing_a_roots_flags_needs_a_signature(tmp_path): + """The flags are not applied by the request — only by the signed response.""" + session = _session(tmp_path, "the-operator", operator="the-operator") + issued = _capture_challenges(session) + + session._do_root_update({"group_id": "g" * 32, "root_name": "shared", + "writable": False}) + + assert [op for op, _ in issued] == [OP_ROOT_UPDATE] + assert session._ctx["roots"].roots[0].writable is True, ( + "applied before it was signed") + + +async def test_the_subject_names_the_outcome_not_the_operation(tmp_path): + """ + The operator is shown the subject before signing, so it has to say what will + be true afterwards. "shared" alone would have them authorize a change they + cannot see the direction of. + """ + session = _session(tmp_path, "op", operator="op") + issued = _capture_challenges(session) + + session._do_root_update({"group_id": "g" * 32, "root_name": "shared", + "writable": True, "removable": True}) + + assert issued == [(OP_ROOT_UPDATE, "shared:rw=on,rem=on")] + + +async def test_eject_and_plug_are_signed_too(tmp_path): + """ + Hiding a group's whole library from every member is not a lesser act than + changing a flag. An unsigned one would let any member black out a group. + """ + session = _session(tmp_path, "op", operator="op") + issued = _capture_challenges(session) + + session._do_root_eject({"group_id": "g" * 32, "root_name": "shared"}) + session._do_root_plug({"group_id": "g" * 32, "root_name": "shared"}) + + assert issued == [(OP_ROOT_EJECT, "shared"), (OP_ROOT_PLUG, "shared")] + + +async def test_a_request_with_nobody_to_authorize_it_is_refused(tmp_path): + """ + An unpaired node has no key to check a signature against, so the challenge + is never issued rather than issued and then unverifiable. + """ + session = _session(tmp_path, "member-1") + issued = _capture_challenges(session) + session._has_admin_authority = lambda: False + + session._do_root_update({"group_id": "g" * 32, "root_name": "shared", + "writable": True}) + + assert issued == [] + assert [m for m in session.sent if m.get("type") == "error"] + + +# ── The deprecated message must not still work ─────────────────────────────── + +async def test_the_old_member_upload_message_changes_nothing(tmp_path): + """ + MNP still parses `member_upload` so an old client gets an answer instead of + a dropped request. What it must not do is act: this instruction could + reopen uploads for a whole group, and a client old enough to send it is + exactly one that knows nothing about read-only roots. + """ + session = _session(tmp_path, "member-1", writable=False) + session._has_admin_authority = lambda: True + issued = _capture_challenges(session) + + session._do_member_upload({"allowed": True}) + + assert issued == [], "a deprecated instruction asked to be signed" + assert session._ctx["roots"].roots[0].writable is False + acks = [m for m in session.sent if m.get("type") == MNP.MEMBER_UPLOAD_ACK] + assert acks and acks[0].get("deprecated") is True + + # And the door is still shut. + _upload(session) + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" diff --git a/packages/meshbay-node/tests/test_roots.py b/packages/meshbay-node/tests/test_roots.py index 1beb220..505091b 100644 --- a/packages/meshbay-node/tests/test_roots.py +++ b/packages/meshbay-node/tests/test_roots.py @@ -108,31 +108,61 @@ def test_a_sibling_with_a_shared_prefix_is_fine(tmp_path): assert roots.names == ["Media", "Media2"] -# ── Uploads ────────────────────────────────────────────────────────────────── +# ── Writable roots ─────────────────────────────────────────────────────────── -def test_a_single_root_receives_uploads_without_being_asked(tmp_path): +def test_a_root_is_read_only_unless_it_says_otherwise(tmp_path): + """ + The default is the safe one. An operator who shares a directory has not + thereby agreed to let anyone write into it, and the version of this that + guessed — one root, so it must be the upload target — meant adding a + second directory silently changed what the first one was. + """ (tmp_path / "Media").mkdir() roots = RootSet.build([_spec(tmp_path / "Media")]) - assert roots.upload_root is roots.roots[0] + assert roots.roots[0].writable is False + assert roots.writable_roots == [] + + +def test_several_roots_can_be_writable_at_once(tmp_path): + (tmp_path / "A").mkdir() + (tmp_path / "B").mkdir() + (tmp_path / "C").mkdir() + roots = RootSet.build([_spec(tmp_path / "A", writable=True), + _spec(tmp_path / "B"), + _spec(tmp_path / "C", writable=True)]) + assert [r.name for r in roots.writable_roots] == ["A", "C"] -def test_several_roots_and_no_designation_means_no_uploads(tmp_path): +def test_a_fully_read_only_group_is_valid(tmp_path): """ - Refused, never guessed: picking one would send a member's file to a disk the - operator did not intend, and that is discovered weeks later. + A group that only publishes is the point of the read-only model, not a + misconfiguration — build must not refuse it, and nothing downstream may + promote a root to writable to have somewhere to put an upload. """ (tmp_path / "A").mkdir() (tmp_path / "B").mkdir() roots = RootSet.build([_spec(tmp_path / "A"), _spec(tmp_path / "B")]) - assert roots.upload_root is None + assert roots.writable_roots == [] + assert len(roots) == 2 -def test_two_upload_roots_are_refused(tmp_path): - (tmp_path / "A").mkdir() - (tmp_path / "B").mkdir() - with pytest.raises(RootError, match="exactly one"): - RootSet.build([_spec(tmp_path / "A", upload=True), - _spec(tmp_path / "B", upload=True)]) +def test_the_old_upload_flag_still_reads_as_writable(tmp_path): + """A node.toml written before this refactor must not change meaning.""" + (tmp_path / "Media").mkdir() + roots = RootSet.build([_spec(tmp_path / "Media", upload=True)]) + assert roots.roots[0].writable is True + assert roots.describe()[0]["writable"] is True + + +def test_writable_wins_over_a_leftover_upload_flag(tmp_path): + """ + A config carrying both is one a migration touched. `writable` is the field + the operator's tooling writes now, so it is the one that decides — reading + the legacy field there would undo the migration on the next load. + """ + (tmp_path / "Media").mkdir() + roots = RootSet.build([_spec(tmp_path / "Media", upload=True, writable=False)]) + assert roots.roots[0].writable is False # ── Resolution ─────────────────────────────────────────────────────────────── @@ -236,18 +266,35 @@ def test_availability_follows_the_directory(tmp_path): def test_describe_reports_what_a_member_needs(tmp_path): (tmp_path / "Media").mkdir() (tmp_path / "Music").mkdir() - roots = RootSet.build([_spec(tmp_path / "Media", upload=True), - _spec(tmp_path / "Music", kind="audio")]) + roots = RootSet.build([_spec(tmp_path / "Media", writable=True), + _spec(tmp_path / "Music", kind="audio", + removable=True)]) described = roots.describe() assert described == [ - {"name": "Media", "kind": "generic", "available": True, "upload": True}, - {"name": "Music", "kind": "audio", "available": True, "upload": False}, + {"name": "Media", "kind": "generic", "available": True, + "writable": True, "removable": False, "ejected": False, + "upload": True}, + {"name": "Music", "kind": "audio", "available": True, + "writable": False, "removable": True, "ejected": False, + "upload": False}, ] # Deliberately no paths: a member is told what exists and whether it is # readable, not where on the operator's disk it lives. assert not any("path" in d for d in described) +def test_describe_still_carries_upload_for_mnp_1_0_clients(tmp_path): + """ + `upload` is `writable` under its old name, kept because an MNP 1.0 client + reads no other field and would otherwise decide the group takes no uploads + at all. It is derived, never stored — the two can never disagree. + """ + (tmp_path / "Media").mkdir() + roots = RootSet.build([_spec(tmp_path / "Media", writable=True)]) + described = roots.describe()[0] + assert described["upload"] == described["writable"] is True + + # ── SAFE_UPLOAD_NAME ──────────────────────────────────────────────────────── def test_safe_name_accepts_unicode_letters(): diff --git a/packages/meshbay-node/tests/test_scan_settings_policy.py b/packages/meshbay-node/tests/test_scan_settings_policy.py index 719b988..94f4421 100644 --- a/packages/meshbay-node/tests/test_scan_settings_policy.py +++ b/packages/meshbay-node/tests/test_scan_settings_policy.py @@ -2,7 +2,7 @@ The operator can tune how often the indexer's reconciliation backstop runs, and how long it waits after a file's last write before hashing it. -Same shape as test_apps_enabled_policy.py / test_member_upload_policy.py: +Same shape as test_apps_enabled_policy.py / test_root_writable_policy.py: changed by a signed operator instruction, stored on the node rather than the hub. Unlike those two, there is also a *live* DirectoryIndexer object to update — see test_set_scan_settings_updates_the_live_indexer below. diff --git a/packages/meshbay-node/tests/test_security_regressions.py b/packages/meshbay-node/tests/test_security_regressions.py index 7f71da5..9db8ac1 100644 --- a/packages/meshbay-node/tests/test_security_regressions.py +++ b/packages/meshbay-node/tests/test_security_regressions.py @@ -18,6 +18,7 @@ from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey from meshbay_common.protocol import IndexEntry from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.roots import RootSet from conftest import one_root from meshbay_node.transport.webrtc_server import WebRTCPeerSession @@ -132,14 +133,14 @@ def test_the_node_never_generates_a_name_it_would_refuse(tmp_path): def _uploads_dir(session) -> Path: """ - Where this session's uploads land: uploads/ inside the group's upload root. + Where this session's uploads land: uploads/ inside its first writable root. Asked of the root set rather than assembled by hand, so a test cannot pass while agreeing with a wrong answer the code also produced. """ - root = session._ctx["roots"].upload_root - assert root is not None, "the fixture must designate an upload root" - return root.path / "uploads" + writable = session._ctx["roots"].writable_roots + assert writable, "the fixture must give the group a writable root" + return writable[0].path / "uploads" def _session(tmp_path: Path, user_id: str) -> WebRTCPeerSession: @@ -227,14 +228,17 @@ def test_dir_create_cannot_escape_the_shared_root(tmp_path, bad): def test_upload_ignores_any_directory_the_client_asks_for(tmp_path): """ - Uploads land in uploads/, chosen by the node. A client that names somewhere - else — or nowhere at all — changes nothing, so the traversal surface that a - client-chosen destination would open does not exist on this path. + The destination inside a root is the node's decision, and stays so. + + A client now names the *root* it is uploading into — it has to, once a group + can have several writable ones — but that is a name looked up in the root + table, never a path. Everything below the root is still chosen here, so the + traversal surface a client-chosen destination would open does not exist. """ session = _session(tmp_path, "user-1") session._do_file_upload({ - "filename": "note.txt", "dir": "../../etc", + "filename": "note.txt", "dir": "../../etc", "path": "/etc", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) @@ -243,6 +247,131 @@ def test_upload_ignores_any_directory_the_client_asks_for(tmp_path): assert not (tmp_path / "etc").exists() +@pytest.mark.parametrize("named_root", [ + "../../etc", "/etc", "shared/../..", "Shared/uploads", "nope", +]) +def test_a_root_name_is_looked_up_never_joined(tmp_path, named_root): + """ + The name the client sends is matched against the group's root table and + refused when it matches nothing. A version that joined it to a path — or + that quietly fell back to the first writable root — would turn "which + directory" into either a traversal or a file on a disk the operator did + not intend, and the second is discovered weeks later. + """ + session = _session(tmp_path, "user-1") + before = set(tmp_path.rglob("*")) + + session._do_file_upload({ + "filename": "note.txt", "root": named_root, + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "no_such_root", named_root + assert set(tmp_path.rglob("*")) == before, f"wrote something via {named_root!r}" + + +def test_an_upload_goes_to_the_root_it_names(tmp_path): + """ + With two writable roots there is no defensible default, and the client is + the only party that knows which directory the person is looking at. The + node picking one meant a file uploaded from a folder on screen landed in a + different one — the same "uploads went somewhere else" the single upload + root was never allowed to guess about. + """ + media = tmp_path / "Media" + incoming = tmp_path / "Incoming" + media.mkdir() + incoming.mkdir() + session = _session(tmp_path, "user-1") + session._ctx["roots"] = RootSet.build([ + {"path": str(media), "writable": True}, + {"path": str(incoming), "writable": True}, + ]) + + session._do_file_upload({ + "filename": "note.txt", "root": "Incoming", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + + assert (incoming / "uploads" / "note.txt").read_bytes() == b"x" + assert not (media / "uploads").exists(), "it went to the first root instead" + + +def test_a_read_only_root_refuses_an_upload(tmp_path): + """ + RO is the mechanism now, not a hidden button. It binds the operator too: + "read-only for everyone" is what makes a published library one, and an + exception for whoever happens to hold admin authority is the sort of + carve-out that later reads as the rule. + """ + published = tmp_path / "Published" + published.mkdir() + session = _session(tmp_path, "user-1") + session._ctx["roots"] = RootSet.build([{"path": str(published)}]) + session._is_node_admin = lambda: True + + session._do_file_upload({ + "filename": "note.txt", "root": "Published", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + assert not (published / "uploads").exists() + + +def test_a_fully_read_only_group_refuses_an_unaddressed_upload(tmp_path): + """ + An MNP 1.0 client names no root, so the node falls back to the first + writable one. There isn't one here, and the fallback must refuse rather + than write into whatever root happens to come first. + """ + published = tmp_path / "Published" + published.mkdir() + session = _session(tmp_path, "user-1") + session._ctx["roots"] = RootSet.build([{"path": str(published)}]) + + session._do_file_upload({ + "filename": "note.txt", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "no_writable_root" + assert not (published / "uploads").exists() + + +def test_an_ejected_root_refuses_an_upload(tmp_path): + """ + Writing to a drive somebody has their hand on is the thing eject exists to + stop. `writable` is still true — that is configuration — so availability + has to be checked separately, which is what an earlier version conflated. + """ + usb = tmp_path / "USB" + usb.mkdir() + session = _session(tmp_path, "user-1") + roots = RootSet.build([{"path": str(usb), "writable": True, + "removable": True}]) + roots.roots[0].ejected = True + roots.roots[0].available = False + session._ctx["roots"] = roots + + session._do_file_upload({ + "filename": "note.txt", "root": "USB", + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_unavailable" + assert not (usb / "uploads").exists() + + def test_two_members_can_send_the_same_filename(tmp_path): """ One shared uploads/ means collisions are ordinary — every camera produces -- 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') 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 From 85a2ec47b7ad334208a3dbb091fadccc7631785c Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 18:16:57 +0200 Subject: feat(node): Phase 2 server side — one directory setting for every app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `video_root` (a string), `audio_root` (a string) and `photo_roots` (a list) said the same thing three ways: three roster accessors, three ops, three MNP messages, three admin-op subjects. They become `set_app_directories(app_key, paths)` and its single-directory wrapper, stored under `_directories` and keyed by the app's registry name — so an application can be added without touching this layer, which is the whole claim of the plugin architecture. The three old names still work. Their MNP messages are handled, and the roster falls back to the old key when the new one is unset, so a node upgraded into this keeps working with no migration step — the plan called for a script, and a script nobody runs on the machine where it matters is worse than a fallback. Two things are new rather than moved: The paths are validated. The setters this replaces accepted anything, so a typo — or a path left behind when a root was removed — was stored happily and then matched no entry, leaving an app showing an empty tab with nothing to distinguish "misconfigured" from "no files yet". Deliberately not `RootSet.resolve()`: that also refuses a currently-unavailable root, and an operator must be able to point an app at a library on a drive they ejected. The legacy scalar is derived, never stored. `video_root` still rides on the handshake ack for MNP 1.0 clients; kept as a second stored value it would drift from the list within one run, which reads as "it works after a restart". Also here: chat's own two settings (a directory, which must be on a read-write root because it is a destination rather than a view, and a link-preview switch gating the unfurl path — checked before the cache, or turning it off would still serve every preview already fetched), the `app_directories`, `chat_directory` and `chat_link_preview` MNP messages, the plural `_directories` on the handshake ack, and `music` as the app's one identifier where storage said `audio` and the registry said `music`. The Music enricher now resolves a boundary per configured directory rather than one for the group: with several, a single boundary is wrong for all but one of them, and for Music that is the difference between reading a folder as an artist and reading it as a release. Suite: 11 failures, all pre-existing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../meshbay-common/src/meshbay_common/adminop.py | 6 + .../meshbay-common/src/meshbay_common/protocol.py | 12 + packages/meshbay-node/src/meshbay_node/daemon.py | 151 +++++++---- packages/meshbay-node/src/meshbay_node/ops.py | 205 +++++++++++---- packages/meshbay-node/src/meshbay_node/roster.py | 146 +++++++---- .../src/meshbay_node/transport/webrtc_server.py | 190 ++++++++++++++ .../meshbay-node/tests/test_app_directories.py | 292 +++++++++++++++++++++ .../tests/test_audio_root_gates_enrichment.py | 14 +- .../meshbay-node/tests/test_audio_root_policy.py | 17 +- .../meshbay-node/tests/test_rename_reenrichment.py | 4 +- packages/meshbay-node/tests/test_root_eject.py | 1 - .../tests/test_startup_scan_enrichment.py | 5 +- .../tests/test_video_root_gates_enrichment.py | 8 +- .../meshbay-node/tests/test_video_root_policy.py | 13 +- 14 files changed, 883 insertions(+), 181 deletions(-) create mode 100644 packages/meshbay-node/tests/test_app_directories.py (limited to 'packages/meshbay-node/tests') diff --git a/packages/meshbay-common/src/meshbay_common/adminop.py b/packages/meshbay-common/src/meshbay_common/adminop.py index 1d2e2b2..9a56934 100644 --- a/packages/meshbay-common/src/meshbay_common/adminop.py +++ b/packages/meshbay-common/src/meshbay_common/adminop.py @@ -106,6 +106,12 @@ OP_AUDIO_ROOT = "audio_root" OP_PHOTO_ROOTS = "photo_roots" OP_ROOT_ADD = "root_add" OP_ROOT_REMOVE = "root_remove" +# One op for every application's directories. The subject is +# ":" so what the operator is shown before +# signing names both the app and the outcome — "video_root" alone said neither. +OP_APP_DIRECTORIES = "app_directories" +OP_CHAT_DIRECTORY = "chat_directory" +OP_CHAT_LINK_PREVIEW = "chat_link_preview" OP_ROOT_UPDATE = "root_update" OP_ROOT_EJECT = "root_eject" OP_ROOT_PLUG = "root_plug" diff --git a/packages/meshbay-common/src/meshbay_common/protocol.py b/packages/meshbay-common/src/meshbay_common/protocol.py index 7ee5df1..689adbf 100644 --- a/packages/meshbay-common/src/meshbay_common/protocol.py +++ b/packages/meshbay-common/src/meshbay_common/protocol.py @@ -164,6 +164,18 @@ class MNP: ROOT_ADD_ACK = "root_add_ack" # node → operator: confirmed ROOT_REMOVE = "root_remove" # operator → node: remove a root from a group ROOT_REMOVE_ACK = "root_remove_ack" # node → operator: confirmed + # One message for every application's directories, keyed by the app's own + # name — adding an app adds no message type. VIDEO_ROOT / AUDIO_ROOT / + # PHOTO_ROOTS above are the same instruction under three earlier names and + # are still handled, for clients that predate this. + APP_DIRECTORIES = "app_directories" # operator → node: an app's folder(s) + APP_DIRECTORIES_ACK = "app_directories_ack" + # Chat's own two: where attachments are written (a destination, so it must + # be a read-write root), and whether the node unfurls links members post. + CHAT_DIRECTORY = "chat_directory" + CHAT_DIRECTORY_ACK = "chat_directory_ack" + CHAT_LINK_PREVIEW = "chat_link_preview" + CHAT_LINK_PREVIEW_ACK = "chat_link_preview_ack" ROOT_UPDATE = "root_update" # operator → node: change writable/removable on a root ROOT_UPDATE_ACK = "root_update_ack" ROOT_EJECT = "root_eject" # operator → node: mark removable root as ejected diff --git a/packages/meshbay-node/src/meshbay_node/daemon.py b/packages/meshbay-node/src/meshbay_node/daemon.py index 4fc07ad..7637b41 100644 --- a/packages/meshbay-node/src/meshbay_node/daemon.py +++ b/packages/meshbay-node/src/meshbay_node/daemon.py @@ -70,26 +70,36 @@ if WEBRTC_AVAILABLE: log = logging.getLogger(__name__) -def _under_video_root(path: str, video_root: str) -> bool: - """Mirrors video-app.js's underVideoRoot: same folder, or a descendant.""" - path = path or "" - return path == video_root or path.startswith(video_root + "/") - +def _under_any_directory(path: str, directories: list[str]) -> bool: + """ + Whether an entry's folder is one of an app's directories, or inside one. -def _under_audio_root(path: str, audio_root: str) -> bool: - """Mirrors music-app.js's underAudioRoot — same shape as _under_video_root.""" + Mirrors `underAnyDirectory` in the SPA's app modules. One helper for every + app since they all take a list: Videos and Music used to take a single + folder and had a function each saying the same thing, which is how the two + came to differ in what they did with a trailing slash. + """ path = path or "" - return path == audio_root or path.startswith(audio_root + "/") + return any(path == d or path.startswith(d + "/") for d in directories) -def _under_any_photo_root(path: str, photo_roots: list[str]) -> bool: +def _owning_directory(path: str, directories: list[str]) -> str | None: """ - Mirrors photos-app.js's underAnyPhotoRoot. Unlike video/audio's single - root, photo_roots is a list (docs/photos.md §2.1) — a match against any - one of them is enough. + Which of an app's directories an entry belongs to — the deepest match. + + Deepest, because directories may nest: with both `Media` and + `Media/Albums` configured, a file under the second belongs to the second. + Taking the first match instead would measure it against a boundary one + level too shallow, which for Music is the difference between reading a + folder as an artist and reading it as a release. """ path = path or "" - return any(path == r or path.startswith(r + "/") for r in photo_roots) + best: str | None = None + for d in directories: + if path == d or path.startswith(d + "/"): + if best is None or len(d) > len(best): + best = d + return best # ── Argon2id calibration ────────────────────────────────────────────────────── @@ -396,19 +406,15 @@ class NodeDaemon: # which the RootSet above already carries.) "enabled_apps": await self._roster.enabled_apps( group_cfg.id) if self._roster else list(Roster.DEFAULT_APPS), - # Which folder is the Videos app's entry point for this - # group — "" means the whole group index. - "video_root": await self._roster.video_root( - group_cfg.id) if self._roster else "", - # Same shape, Music app's own entry point. - "audio_root": await self._roster.audio_root( - group_cfg.id) if self._roster else "", - # Photos app's entry points — a *list*, unlike video_root/ - # audio_root above (docs/photos.md §2.1: a photo library - # is routinely scattered across several folders). Empty - # list means nothing configured yet. - "photo_roots": await self._roster.photo_roots( - group_cfg.id) if self._roster else [], + # Which folder(s) inside the shared roots each app works + # over. One shape for every app (roster.py's + # app_directories) — an empty list means nothing has been + # chosen, which every app reads as "show nothing yet", + # never "the whole group index". + **(await self._app_directories_ctx(group_cfg.id)), + # Whether the node unfurls links members post here. + "chat_link_preview": await self._roster.chat_link_preview( + group_cfg.id) if self._roster else True, # Whether TMDB lookups run for this group at all — # per-group (2026-08-24, used to be node-wide), same # "read once, kept current in place by the signed op" @@ -631,9 +637,14 @@ class NodeDaemon: self._state["quic_server"] = self._quic_server self._state["hub"] = hub self._state["reload_fn"] = self._reload_config - self._state["enrich_video_root_fn"] = self._enrich_video_root_now - self._state["enrich_audio_root_fn"] = self._enrich_audio_root_now - self._state["enrich_photo_roots_fn"] = self._enrich_photo_roots_now + # Keyed by app, so `ops.set_app_directories` finds the right + # sweep without knowing which apps exist — an app with nothing to + # enrich simply has no entry. + self._state["enrich_app_dirs_fns"] = { + "video": self._enrich_video_root_now, + "music": self._enrich_audio_root_now, + "photo": self._enrich_photo_roots_now, + } # Rotating a key has to reach every transport holding a copy of it, # and clearing the denylist has to reach the one the handshake # consults — so both are published rather than reachable only @@ -854,15 +865,10 @@ class NodeDaemon: "enabled_apps": ( await self._roster.enabled_apps(group_cfg.id) if self._roster else list(Roster.DEFAULT_APPS)), - "video_root": ( - await self._roster.video_root(group_cfg.id) - if self._roster else ""), - "audio_root": ( - await self._roster.audio_root(group_cfg.id) - if self._roster else ""), - "photo_roots": ( - await self._roster.photo_roots(group_cfg.id) - if self._roster else []), + **(await self._app_directories_ctx(group_cfg.id)), + "chat_link_preview": ( + await self._roster.chat_link_preview(group_cfg.id) + if self._roster else True), "tmdb_enabled": ( await self._roster.tmdb_enabled(group_cfg.id) if self._roster else True), @@ -1076,6 +1082,36 @@ class NodeDaemon: spec["ejected"] = True return RootSet.build(specs) + # Every app that keeps directories. Not derived from `enabled_apps`: the + # context is read once at load and an app enabled later must not find its + # own setting missing. Adding an app adds a name here and nowhere else on + # this side. + APP_DIR_KEYS = ("video", "music", "photo", "chat") + + async def _app_directories_ctx(self, group_id: str) -> dict: + """ + Each app's configured directories, plus the legacy scalar names the + rest of the tree still reads. + + The scalars are derived here rather than stored, so the two can never + disagree: `video_root` is the first of `video_directories` and exists + for MNP 1.0 clients and for the handful of call sites that predate the + list. A group with several video directories reports the first as its + `video_root` — which is what an old client can represent, and all it + could ever have shown. + """ + dirs = {} + for app in self.APP_DIR_KEYS: + dirs[f"{app}_directories"] = ( + await self._roster.app_directories(group_id, app) + if self._roster else []) + aliases = {} + for app in self.APP_DIR_KEYS: + alias = Roster.ctx_alias(app, dirs[f"{app}_directories"]) + if alias: + aliases[alias[0]] = alias[1] + return {**dirs, **aliases} + def _eject_persister(self, group_id: str): """`on_root_ejected` bound to one group, for that group's indexer.""" async def persist(root_name: str, ejected: bool) -> None: @@ -1249,13 +1285,13 @@ class NodeDaemon: """ if not self._enricher or not self._roster: return - video_root = await self._roster.video_root(indexer.group_id) - if not video_root: + video_dirs = await self._roster.app_directories(indexer.group_id, "video") + if not video_dirs: return for entry in entries: if entry.type != "video" or (indexer.group_id, entry.id) in self._enriched_attempted: continue - if not _under_video_root(entry.path, video_root): + if not _under_any_directory(entry.path, video_dirs): continue file_path = entry_abs_path(indexer.roots, entry) if not file_path or not file_path.exists(): @@ -1330,14 +1366,18 @@ class NodeDaemon: """ if not self._audio_enricher or not self._roster: return - audio_root = await self._roster.audio_root(indexer.group_id) - if not audio_root: + audio_dirs = await self._roster.app_directories(indexer.group_id, "music") + if not audio_dirs: return - root_boundary = indexer.roots.resolve(audio_root, require_available=False) + # Resolved once per directory, not once per file: a library is + # thousands of entries and this is a filesystem call each time. + boundaries = {d: indexer.roots.resolve(d, require_available=False) + for d in audio_dirs} for entry in entries: if entry.type != "audio" or (indexer.group_id, entry.id) in self._enriched_attempted: continue - if not _under_audio_root(entry.path, audio_root): + owner = _owning_directory(entry.path, audio_dirs) + if owner is None: continue file_path = entry_abs_path(indexer.roots, entry) if not file_path or not file_path.exists(): @@ -1347,13 +1387,16 @@ class NodeDaemon: async def on_done(file_id: str, fields: dict, _indexer=indexer) -> None: await self._on_enriched(_indexer, file_id, fields) - # `root_boundary` — audio_root itself, not the shared root it - # lives under — so the ancestor walk + # The boundary is *the configured directory this file is under*, + # not the shared root it lives in — so the ancestor walk # (enrich_audio._artist_album_from_ancestors) treats a flat - # top-level folder right under the *configured* Music root as - # ambiguous (artist-or-release, §2.1), not one level too shallow - # if audio_root is itself a subfolder of a larger shared root. - self._audio_enricher.spawn(entry, file_path, on_done, root_boundary) + # top-level folder right under the configured Music directory as + # ambiguous (artist-or-release, musicbay.md §2.1), rather than one + # level too shallow when that directory is itself a subfolder. + # With several configured, each file is measured against its own: + # a single shared boundary would be wrong for all but one of them. + self._audio_enricher.spawn(entry, file_path, on_done, + boundaries.get(owner)) async def _enrich_audio_root_now(self, group_id: str) -> None: """ @@ -1403,13 +1446,13 @@ class NodeDaemon: """ if not self._photo_enricher or not self._roster: return - photo_roots = await self._roster.photo_roots(indexer.group_id) - if not photo_roots: + photo_dirs = await self._roster.app_directories(indexer.group_id, "photo") + if not photo_dirs: return for entry in entries: if entry.type != "image" or (indexer.group_id, entry.id) in self._enriched_attempted: continue - if not _under_any_photo_root(entry.path, photo_roots): + if not _under_any_directory(entry.path, photo_dirs): continue file_path = entry_abs_path(indexer.roots, entry) if not file_path or not file_path.exists(): diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index 13a7250..f7f6190 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -1205,76 +1205,171 @@ async def set_musicbrainz_enabled(state: dict, group_id: str, enabled: bool) -> return {"enabled": enabled, "group_id": group_id} -async def set_video_root(state: dict, group_id: str, path: str) -> dict: +# ── App directories ────────────────────────────────────────────────────────── + +def _validate_app_dirs(state: dict, group_id: str, paths: list[str], *, + require_writable: bool) -> list[str]: """ - Which folder (possibly a subfolder of a shared root) is the Videos app's - entry point for this group. Same shape as set_enabled_apps: lives on the - node (roster.db), takes effect without a restart, signed by the operator. - `path=""` clears it — the Videos tab then asks for one to be chosen - before anything (including TMDB enrichment, docs/mediacenter.md §5.2) - runs, rather than defaulting to the whole shared index. - - A non-empty path fires (never awaits) a sweep of whatever that folder - already contains: the ordinary per-change enrichment path only ever - looks at files new since the last broadcast, so anything already sitting - in a folder before it became the video_root would otherwise never be - picked up. + Every path an app is pointed at must live inside one of the group's roots. + + The per-app setters this replaces validated nothing: a typo, or a path left + behind by a root that was removed, was stored and then quietly matched no + entry — an app showing an empty tab with no way to tell "misconfigured" + from "no files yet". Refusing at the point of setting is the only moment + the operator is present to be told. + + Not `RootSet.resolve()`, deliberately: that also refuses a directory whose + root is currently *unavailable*, and an operator must be able to configure + a library on a drive they have unplugged. What is checked here is the + shape — inside a named root, no traversal — which does not change with + what happens to be mounted. """ - roster = _roster(state) - ctx = _group_ctx(state, group_id) - await roster.set_video_root(group_id, path, set_by=state.get("node_user_id", "")) - ctx["video_root"] = path - log.info("Videos root for group %s: %r", group_id[:8], path) - if path: - enrich_fn = state.get("enrich_video_root_fn") - if enrich_fn: - asyncio.ensure_future(enrich_fn(group_id)) - return {"path": path, "group_id": group_id} + roots: RootSet | None = _group_ctx(state, group_id).get("roots") + if roots is None: + raise OpError("Group has no roots", status=503) + clean: list[str] = [] + for raw in paths: + path = str(raw or "").strip().strip("/") + if not path: + continue + if ".." in path.split("/"): + raise OpError(f"{path!r} is not a directory inside this group", + status=400) + found = roots.split(path) + if found is None: + raise OpError( + f"{path!r} is not inside any of this group's shared " + f"directories", status=400, + extra={"available": roots.names}) + root, _tail = found + if require_writable and not root.writable: + raise OpError( + f"{root.name!r} is read-only, and this setting needs a " + f"directory that accepts uploads", status=400) + clean.append(path) + return sorted(set(clean)) -async def set_audio_root(state: dict, group_id: str, path: str) -> dict: + +async def set_app_directories(state: dict, group_id: str, app_key: str, + paths: list[str], *, + require_writable: bool = False) -> dict: """ - Same shape as set_video_root above — the Music app's own entry point, - added later (docs/musicbay.md's original "no root, works over the - whole shared tree" simplification didn't hold up against a real messy - library). `path=""` clears it — the Music tab then asks for one to be - chosen before anything (including tag/cover enrichment) runs, rather - than defaulting to the whole shared index. + Which folder(s) inside the group's shared roots an application works over. + + One function for every app, keyed by the app's own name: adding an + application is a registry entry and a settings component, not another + near-identical op here. It replaces `set_video_root`, `set_audio_root` and + `set_photo_roots`, which differed only in the key they wrote and whether + they took a string or a list. + + Empty means nothing configured, which every app reads as "show nothing + until an operator has chosen" — never "the whole group index". Pointing an + app at the whole library is a decision, not a default nobody made. + + A change always fires (never awaits) a sweep of what the new directories + already contain: the ordinary per-change enrichment path only looks at + entries new since the last broadcast, so files already sitting in a folder + when it was chosen would otherwise never be picked up. """ roster = _roster(state) ctx = _group_ctx(state, group_id) - await roster.set_audio_root(group_id, path, set_by=state.get("node_user_id", "")) - ctx["audio_root"] = path - log.info("Music root for group %s: %r", group_id[:8], path) - if path: - enrich_fn = state.get("enrich_audio_root_fn") - if enrich_fn: - asyncio.ensure_future(enrich_fn(group_id)) - return {"path": path, "group_id": group_id} + clean = _validate_app_dirs(state, group_id, paths, + require_writable=require_writable) + await roster.set_app_directories(group_id, app_key, clean, + set_by=state.get("node_user_id", "")) + ctx[f"{app_key}_directories"] = clean + # The scalar the handshake ack still publishes for MNP 1.0 clients is + # derived, and has to be re-derived here: leaving it behind would make the + # ack disagree with the list within a single run, and only until a restart + # — the shape of bug that reads as "it works after a restart". + from meshbay_node.roster import Roster + alias = Roster.ctx_alias(app_key, clean) + if alias: + ctx[alias[0]] = alias[1] + log.info("%s directories for group %s: %s", app_key, group_id[:8], + ", ".join(clean) or "(none)") + + enrich = (state.get("enrich_app_dirs_fns") or {}).get(app_key) + if enrich: + asyncio.ensure_future(enrich(group_id)) + return {"app": app_key, "directories": clean, "group_id": group_id} + + +async def set_app_directory(state: dict, group_id: str, app_key: str, + path: str, *, + require_writable: bool = False) -> dict: + """ + The single-directory form, for an app that only ever wants one. + + Stored as a one-element list like every other app, because two storage + shapes for one idea is what made `video_root` (scalar) and `photo_roots` + (list) need separate ops, separate MNP messages and separate widgets to + say the same thing. `path=""` clears it. + """ + result = await set_app_directories( + state, group_id, app_key, [path] if path else [], + require_writable=require_writable) + dirs = result["directories"] + return {**result, "path": dirs[0] if dirs else ""} + + +# The per-app wrappers MNP still names. They exist so an MNP 1.0 client's +# `video_root` / `audio_root` / `photo_roots` messages keep working; nothing +# new should be added here — a new app calls the generic pair above. + +async def set_video_root(state: dict, group_id: str, path: str) -> dict: + result = await set_app_directory(state, group_id, "video", path) + return {"path": result["path"], "group_id": group_id} + + +async def set_audio_root(state: dict, group_id: str, path: str) -> dict: + # "music", not "audio": the app's registry key is what identifies it + # everywhere, and `audio_root` is only the name the setting used to have. + result = await set_app_directory(state, group_id, "music", path) + return {"path": result["path"], "group_id": group_id} async def set_photo_roots(state: dict, group_id: str, roots: list[str]) -> dict: + result = await set_app_directories(state, group_id, "photo", roots) + return {"roots": result["directories"], "group_id": group_id} + + +# ── Chat ───────────────────────────────────────────────────────────────────── + +async def set_chat_directory(state: dict, group_id: str, path: str) -> dict: """ - Which folder(s) are the Photos app's entry points for this group. Unlike - `set_video_root`/`set_audio_root`, the whole *set* is replaced in one - call (docs/photos.md §2.1) — signed once, same shape as - `set_enabled_apps`, rather than one op per root added/removed. - - Always fires a sweep, even to an empty list: a root just added needs its - existing contents enriched (nothing else re-visits already-indexed - entries), and a root just removed leaves its cache entries harmlessly - unused rather than needing any cleanup — re-sweeping the new set costs - nothing when it's empty. + Where chat attachments are written. + + `require_writable`, unlike every other app directory: this one is a + *destination*, not a view. Pointing it at a read-only root would produce an + attachment button that fails at the moment somebody uses it, which is the + failure mode the RO/RW model exists to move earlier. + """ + return await set_app_directory(state, group_id, "chat", path, + require_writable=True) + + +async def set_chat_link_preview(state: dict, group_id: str, + enabled: bool) -> dict: + """ + Whether the node fetches a page's title and image when a member posts a + link. + + Outbound third-party traffic on the operator's connection, caused by a + message they did not write and pointing at a URL they did not choose — so + it is theirs to switch off, on the same reasoning as the per-group TMDB + switch. Absent means on, because that is what the node did before this + existed. """ roster = _roster(state) ctx = _group_ctx(state, group_id) - await roster.set_photo_roots(group_id, roots, set_by=state.get("node_user_id", "")) - ctx["photo_roots"] = roots - log.info("Photo roots for group %s: %s", group_id[:8], ", ".join(sorted(roots)) or "(none)") - enrich_fn = state.get("enrich_photo_roots_fn") - if enrich_fn: - asyncio.ensure_future(enrich_fn(group_id)) - return {"roots": roots, "group_id": group_id} + await roster.set_chat_link_preview(group_id, enabled, + set_by=state.get("node_user_id", "")) + ctx["chat_link_preview"] = enabled + log.info("Chat link previews for group %s: %s", group_id[:8], + "on" if enabled else "off") + return {"enabled": enabled, "group_id": group_id} # ── Scan settings ──────────────────────────────────────────────────────────── diff --git a/packages/meshbay-node/src/meshbay_node/roster.py b/packages/meshbay-node/src/meshbay_node/roster.py index 5f38acd..af87f92 100644 --- a/packages/meshbay-node/src/meshbay_node/roster.py +++ b/packages/meshbay-node/src/meshbay_node/roster.py @@ -661,58 +661,110 @@ class Roster: await self.set_setting(self.NODE_WIDE_GROUP_ID, self.SETTING_TMDB_LANGUAGE, language, set_by) - # Which folder is the Videos app's entry point for this group — per-group - # (unlike the token/language above), since different groups share - # different trees. Empty/unset means the whole group index, exactly as - # today. - SETTING_VIDEO_ROOT = "video_root" - - async def video_root(self, group_id: str) -> str: - return await self.get_setting(group_id, self.SETTING_VIDEO_ROOT, "") or "" - - async def set_video_root(self, group_id: str, path: str, set_by: str = "") -> str: - await self.set_setting(group_id, self.SETTING_VIDEO_ROOT, path or "", set_by) - return path or "" - - # Same shape as SETTING_VIDEO_ROOT — the Music app's own entry point, - # added later (docs/musicbay.md's original "no root, works over the - # whole shared tree" simplification turned out not to hold up against a - # real messy library: the operator asked for the same scoping Videos - # already had). Empty/unset means Music shows nothing yet, exactly like - # an unset video_root — see daemon.py's enrichment gate. - SETTING_AUDIO_ROOT = "audio_root" - - async def audio_root(self, group_id: str) -> str: - return await self.get_setting(group_id, self.SETTING_AUDIO_ROOT, "") or "" - - async def set_audio_root(self, group_id: str, path: str, set_by: str = "") -> str: - await self.set_setting(group_id, self.SETTING_AUDIO_ROOT, path or "", set_by) - return path or "" - - # Which folder(s) are the Photos app's entry points for this group — - # a *set*, unlike video_root/audio_root above: a photo library is - # routinely scattered across several unrelated folders (docs/photos.md - # §2.1), so there is no single natural root to pick. Stored the same way - # `enabled_apps` already is (json.dumps(sorted(...))). Empty/unset means - # nothing configured yet — same "show nothing until an operator has - # chosen" discipline video_root/audio_root already established, not - # "the whole group index". - SETTING_PHOTO_ROOTS = "photo_roots" - - async def photo_roots(self, group_id: str) -> list[str]: - value = await self.get_setting(group_id, self.SETTING_PHOTO_ROOTS) - if value is None: + # ── App directories ───────────────────────────────────────────────────── + # + # Which folder(s) inside the group's shared roots each application uses as + # its entry point. One storage shape for every app, keyed by the app's own + # name, so adding an application needs no change here at all — that is the + # whole point of the plugin architecture (docs/refactor-groups.md §1.6). + # + # Always a JSON list, even for an app that only ever wants one directory. + # Two shapes for one idea is how `video_root` (scalar) and `photo_roots` + # (list) ended up needing separate ops, separate MNP messages and separate + # settings widgets to say the same thing. + # + # Empty/unset means nothing configured yet, and every app reads that as + # "show nothing until an operator has chosen" rather than "the whole group + # index" — the discipline video_root established, kept. + SETTING_APP_DIRS_SUFFIX = "_directories" + + # What each app's directories used to be stored under, before they were + # one shape. Read as a fallback so an existing node keeps working with no + # migration step: the legacy key is never written again, and the first + # save through the new path leaves it behind. + # Keyed by the *registry* name the app is known by everywhere else + # (apps.js, ALLOWED_APPS, enabled_apps) — which for Music is "music", while + # its old setting was called `audio_root`. One identifier per app, and the + # place the two names meet is this table and nowhere else. + LEGACY_DIR_KEYS = { + "video": ("video_root", "scalar"), + "music": ("audio_root", "scalar"), + "photo": ("photo_roots", "list"), + } + + # The name each app's directories are *also* published under, for readers + # that predate the list — the handshake ack's `video_root`, and the group + # context the ack builds from. Derived from the list, never stored beside + # it, so the two cannot disagree; the shape says how to derive it. + CTX_ALIASES = { + "video": ("video_root", "scalar"), + "music": ("audio_root", "scalar"), + "photo": ("photo_roots", "list"), + "chat": ("chat_directory", "scalar"), + } + + @classmethod + def app_dirs_key(cls, app_key: str) -> str: + return f"{app_key}{cls.SETTING_APP_DIRS_SUFFIX}" + + @classmethod + def ctx_alias(cls, app_key: str, directories: list[str]) -> tuple[str, object] | None: + """The (name, value) an app's directories are also published under.""" + alias = cls.CTX_ALIASES.get(app_key) + if not alias: + return None + name, shape = alias + if shape == "list": + return name, list(directories) + return name, (directories[0] if directories else "") + + async def app_directories(self, group_id: str, app_key: str) -> list[str]: + value = await self.get_setting(group_id, self.app_dirs_key(app_key)) + if value is not None: + try: + return [str(p) for p in json.loads(value)] + except (ValueError, TypeError): + return [] + + legacy = self.LEGACY_DIR_KEYS.get(app_key) + if not legacy: + return [] + key, shape = legacy + raw = await self.get_setting(group_id, key) + if raw is None: return [] + if shape == "scalar": + return [raw] if raw else [] try: - return list(json.loads(value)) + return [str(p) for p in json.loads(raw)] except (ValueError, TypeError): return [] - async def set_photo_roots(self, group_id: str, roots: list[str], - set_by: str = "") -> list[str]: - await self.set_setting(group_id, self.SETTING_PHOTO_ROOTS, - json.dumps(sorted(roots)), set_by) - return roots + async def set_app_directories(self, group_id: str, app_key: str, + paths: list[str], set_by: str = "") -> list[str]: + clean = sorted({str(p).strip("/") for p in paths if str(p).strip("/")}) + await self.set_setting(group_id, self.app_dirs_key(app_key), + json.dumps(clean), set_by) + return clean + + # ── Chat ──────────────────────────────────────────────────────────────── + + # Whether the node fetches a page's title/preview when a member posts a + # link. Outbound third-party traffic on the operator's connection, from a + # message they did not write, so it is theirs to switch off — the same + # reasoning as the per-group TMDB switch. Unset means on, because that is + # what the node did before this existed. + SETTING_CHAT_LINK_PREVIEW = "chat_link_preview" + + async def chat_link_preview(self, group_id: str) -> bool: + value = await self.get_setting(group_id, self.SETTING_CHAT_LINK_PREVIEW, "1") + return value != "0" + + async def set_chat_link_preview(self, group_id: str, enabled: bool, + set_by: str = "") -> bool: + await self.set_setting(group_id, self.SETTING_CHAT_LINK_PREVIEW, + "1" if enabled else "0", set_by) + return enabled # Whether TMDB lookups run for this group at all — per-group, unlike the # token/language above: one node process can share a real media library 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 b99affc..c4d053e 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -76,6 +76,9 @@ from meshbay_common.adminop import ( OP_MUSICBRAINZ_ENABLED, OP_AUDIO_ROOT, OP_PHOTO_ROOTS, + OP_APP_DIRECTORIES, + OP_CHAT_DIRECTORY, + OP_CHAT_LINK_PREVIEW, OP_ROOT_ADD, OP_ROOT_REMOVE, OP_ROOT_UPDATE, @@ -484,6 +487,12 @@ class WebRTCPeerSession: self._do_audio_root(msg) elif mtype == MNP.PHOTO_ROOTS: self._do_photo_roots(msg) + elif mtype == MNP.APP_DIRECTORIES: + self._do_app_directories(msg) + elif mtype == MNP.CHAT_DIRECTORY: + self._do_chat_directory(msg) + elif mtype == MNP.CHAT_LINK_PREVIEW: + self._do_chat_link_preview(msg) elif mtype == MNP.MEDIA_META_REQ: self._spawn(self._do_media_meta_request(msg)) elif mtype == MNP.SEASON_META_REQ: @@ -802,6 +811,23 @@ class WebRTCPeerSession: # (docs/photos.md §2.1). Empty means unset (the Photos tab shows # nothing yet). "photo_roots": list(self._group_ctx().get("photo_roots") or []), + # The same three answers in one shape, plus every other app's — + # `_directories`, keyed by the app's registry name, always a + # list. The scalars above are derived from these (daemon.py's + # `_app_directories_ctx`) and kept for MNP 1.0 clients, which can + # represent one folder and never more. A client reading the plural + # form gets all of them. + **{f"{app}_directories": + list(self._group_ctx().get(f"{app}_directories") or []) + for app in ("video", "music", "photo", "chat")}, + # Where chat attachments are written — the singular form, because + # Chat genuinely has one destination. "" means the operator has not + # chosen, and the paperclip says so. + "chat_directory": self._group_ctx().get("chat_directory") or "", + # Whether the node unfurls links members post here. Absent means + # on, which is what it did before this existed. + "chat_link_preview": bool( + self._group_ctx().get("chat_link_preview", True)), # So a client that connects mid-scan shows the indexing state # immediately, instead of waiting for the next periodic # INDEX_PROGRESS push. Never a path or filename — see @@ -2096,6 +2122,150 @@ class WebRTCPeerSession: except Exception: pass + # ── App directories (generic) ──────────────────────────────────────── + + def _do_app_directories(self, msg: dict) -> None: + """ + Which folder(s) an application works over, for any application. + + One handler where there were three near-identical ones (`video_root`, + `audio_root`, `photo_roots`) differing only in the key they wrote and + whether they carried a string or a list. Those three still exist for + clients that speak them; nothing new is added beside them. + + `app` must be one this node knows (`ALLOWED_APPS`) — a client-supplied + key is otherwise a way to write arbitrary rows into `group_settings`. + The paths are checked by `ops._validate_app_dirs`, which runs after the + signature: this is a settings change, not a capability, so refusing + early here would be a courtesy rather than the control. + """ + app = str(msg.get("app", "")).strip() + dirs = msg.get("directories") + if app not in self.ALLOWED_APPS: + self._send({"type": "error", "detail": f"Unknown app {app!r}"}) + return + if not isinstance(dirs, list) or not all(isinstance(d, str) for d in dirs): + self._send({"type": "error", + "detail": "Missing or invalid 'directories'"}) + return + clean = sorted({d.strip("/") for d in dirs if d.strip("/")}) + if not self._has_admin_authority(): + self._send({"type": "error", "detail": "No authorized key for this"}) + return + # The app is in the subject, not only the paths: an operator shown + # "Media/Films" alone cannot tell which application is about to be + # pointed at it, and two apps' challenges would be indistinguishable. + self._issue_admin_challenge( + OP_APP_DIRECTORIES, f"{app}:{','.join(clean)}") + + async def _admin_exec_app_directories( + self, pending: dict, transcript: bytes, sig: bytes, + ) -> None: + app, _, joined = pending["subject"].partition(":") + dirs = joined.split(",") if joined else [] + if not await self._verify_admin_sig(transcript, sig): + self._send({"type": "error", "detail": "Signature verification failed"}) + self._audit("admin_auth_failed", f"app_directories:{pending['subject']}") + return + try: + result = await self._run_op( + ops.set_app_directories, self._group_id or "", app, dirs) + except ops.OpError as e: + self._send({"type": "error", "detail": e.message}) + return + self._audit("app_directories", pending["subject"]) + self._broadcast_to_group({"type": MNP.APP_DIRECTORIES_ACK, + "v": MNP_VERSION, "app": app, + "directories": result["directories"]}) + + # ── Chat ───────────────────────────────────────────────────────────── + + def _do_chat_directory(self, msg: dict) -> None: + """ + Where chat attachments are written. + + Unlike every other app directory this one is a destination, so it has + to be on a read-write root — checked by `ops.set_chat_directory` after + the signature, which is where the refusal actually lives. + """ + path = msg.get("path") + if not isinstance(path, str): + self._send({"type": "error", "detail": "Missing or invalid 'path'"}) + return + path = path.strip("/") + if not self._has_admin_authority(): + self._send({"type": "error", "detail": "No authorized key for this"}) + return + self._issue_admin_challenge(OP_CHAT_DIRECTORY, path) + + async def _admin_exec_chat_directory( + self, pending: dict, transcript: bytes, sig: bytes, + ) -> None: + path = pending["subject"] + if not await self._verify_admin_sig(transcript, sig): + self._send({"type": "error", "detail": "Signature verification failed"}) + self._audit("admin_auth_failed", f"chat_directory:{path}") + return + try: + await self._run_op( + ops.set_chat_directory, self._group_id or "", path) + except ops.OpError as e: + self._send({"type": "error", "detail": e.message}) + return + self._audit("chat_directory", path) + self._broadcast_to_group( + {"type": MNP.CHAT_DIRECTORY_ACK, "v": MNP_VERSION, "path": path}) + + def _do_chat_link_preview(self, msg: dict) -> None: + """ + Whether the node fetches a page's title and image when a member posts + a link — outbound traffic on the operator's connection, from a message + they did not write, so it is signed like everything else that decides + what leaves this machine. + """ + enabled = msg.get("enabled") + if not isinstance(enabled, bool): + self._send({"type": "error", "detail": "Missing or invalid 'enabled'"}) + return + if not self._has_admin_authority(): + self._send({"type": "error", "detail": "No authorized key for this"}) + return + self._issue_admin_challenge( + OP_CHAT_LINK_PREVIEW, "on" if enabled else "off") + + async def _admin_exec_chat_link_preview( + self, pending: dict, transcript: bytes, sig: bytes, + ) -> None: + enabled = pending["subject"] == "on" + if not await self._verify_admin_sig(transcript, sig): + self._send({"type": "error", "detail": "Signature verification failed"}) + self._audit("admin_auth_failed", + f"chat_link_preview:{pending['subject']}") + return + try: + await self._run_op( + ops.set_chat_link_preview, self._group_id or "", enabled) + except ops.OpError as e: + self._send({"type": "error", "detail": e.message}) + return + self._audit("chat_link_preview", pending["subject"]) + self._broadcast_to_group({"type": MNP.CHAT_LINK_PREVIEW_ACK, + "v": MNP_VERSION, "enabled": enabled}) + + def _broadcast_to_group(self, notice: dict) -> None: + """ + Tell everyone connected to this group about a setting that changed. + + Enforcement never depends on this reaching them — the node is what + refuses — but a control that stays on screen until the next + reconnection is a control people use. + """ + for _uid, session in list(self._peer_registry().items()): + try: + session._send(notice) + except Exception: + pass + def _do_musicbrainz_enabled(self, msg: dict) -> None: """ Whether MusicBrainz lookups run for this group at all. Per-group @@ -3734,6 +3904,17 @@ class WebRTCPeerSession: """ url = msg.get("url") key = url if isinstance(url, str) else "" + + # Checked before the cache, not after: the operator turning previews + # off has to stop serving the ones already fetched too, or the setting + # takes effect only for links nobody has posted yet. Refused as an + # ordinary miss — the client shows the bare link, which is exactly what + # "no preview" looks like for a page that has none. + if not self._group_ctx().get("chat_link_preview", True): + self._send({"type": MNP.LINK_PREVIEW_RESP, "v": MNP_VERSION, + "url": key, "ok": False}) + return + cached = _link_preview_cache_get(key) if cached is not None: self._send({**cached, "type": MNP.LINK_PREVIEW_RESP, "v": MNP_VERSION}) @@ -4155,6 +4336,15 @@ class WebRTCPeerSession: elif pending["op"] == OP_ROOT_REMOVE: self._spawn( self._admin_exec_root_remove(pending, transcript, sig_bytes)) + elif pending["op"] == OP_APP_DIRECTORIES: + self._spawn( + self._admin_exec_app_directories(pending, transcript, sig_bytes)) + elif pending["op"] == OP_CHAT_DIRECTORY: + self._spawn( + self._admin_exec_chat_directory(pending, transcript, sig_bytes)) + elif pending["op"] == OP_CHAT_LINK_PREVIEW: + self._spawn( + self._admin_exec_chat_link_preview(pending, transcript, sig_bytes)) elif pending["op"] == OP_ROOT_UPDATE: self._spawn( self._admin_exec_root_update(pending, transcript, sig_bytes)) diff --git a/packages/meshbay-node/tests/test_app_directories.py b/packages/meshbay-node/tests/test_app_directories.py new file mode 100644 index 0000000..3ede1b6 --- /dev/null +++ b/packages/meshbay-node/tests/test_app_directories.py @@ -0,0 +1,292 @@ +""" +One shape for every application's directories. + +`video_root` (a string), `audio_root` (a string) and `photo_roots` (a list) +said the same thing three ways, and each needed its own op, its own MNP message +and its own settings widget. They are one function keyed by the app's own name +now, which is what lets an application be added without touching this layer at +all — the whole claim of the plugin architecture. + +Two properties are new rather than moved, and both matter more than the tidying: + +* **the paths are validated.** The setters this replaces accepted anything. A + typo, or a path left behind when a root was removed, was stored happily and + then matched no entry — an app showing an empty tab, with nothing to + distinguish "misconfigured" from "no files yet". The moment of setting is the + only one where the operator is present to be told; +* **the legacy scalar is derived, never stored.** `video_root` still rides on + the handshake ack for MNP 1.0 clients. Kept as a second stored value it would + drift from the list within one run — the shape of bug that reads as "it works + after a restart". +""" + +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.roster import Roster + +pytestmark = pytest.mark.asyncio + +GROUP = "g" * 32 + + +async def _state(tmp_path: Path, *, writable: bool = True) -> tuple[dict, Roster]: + media = tmp_path / "Media" + (media / "Films").mkdir(parents=True) + (media / "Albums").mkdir() + published = tmp_path / "Published" + published.mkdir() + + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + index = GroupIndex(group_id=GROUP, sk_node=Ed25519PrivateKey.generate()) + roots = RootSet.build([ + {"path": str(media), "writable": writable}, + {"path": str(published)}, + ]) + state = { + "roster": roster, + "node_user_id": "operator", + "groups_ctx": {GROUP: {"index": index, "roots": roots}}, + "config": SimpleNamespace(groups=[SimpleNamespace(id=GROUP, roots=[])]), + } + return state, roster + + +# ── One function, any app ──────────────────────────────────────────────────── + +async def test_an_app_nobody_wrote_code_for_stores_its_directories(tmp_path): + """ + The point of the generic pair. Nothing in ops.py, roster.py or the daemon + names this app, and it round-trips anyway — which is the difference between + a plugin architecture and a list of special cases. + """ + state, roster = await _state(tmp_path) + try: + await ops.set_app_directories(state, GROUP, "helloworld", ["Media/Films"]) + assert await roster.app_directories(GROUP, "helloworld") == ["Media/Films"] + finally: + await roster.close() + + +async def test_directories_are_deduplicated_and_ordered(tmp_path): + """ + The stored form is what the operator signs a subject built from, on both + sides. Two clients sending the same set in different orders must produce + the same bytes, or one of them refuses to sign its own request. + """ + state, roster = await _state(tmp_path) + try: + out = await ops.set_app_directories( + state, GROUP, "video", ["Media/Films", "Media", "Media/Films"]) + assert out["directories"] == ["Media", "Media/Films"] + finally: + await roster.close() + + +async def test_a_single_directory_app_stores_a_one_element_list(tmp_path): + state, roster = await _state(tmp_path) + try: + out = await ops.set_app_directory(state, GROUP, "chat", "Media/Films", + require_writable=True) + assert out["path"] == "Media/Films" + assert await roster.app_directories(GROUP, "chat") == ["Media/Films"] + + cleared = await ops.set_app_directory(state, GROUP, "chat", "") + assert cleared["path"] == "" + assert await roster.app_directories(GROUP, "chat") == [] + finally: + await roster.close() + + +# ── Validation ─────────────────────────────────────────────────────────────── + +@pytest.mark.parametrize("bad", [ + "Nowhere", "Nowhere/Deeper", "/etc", "Media/../../etc", "..", +]) +async def test_a_directory_outside_every_root_is_refused(tmp_path, bad): + state, roster = await _state(tmp_path) + try: + with pytest.raises(ops.OpError): + await ops.set_app_directories(state, GROUP, "video", [bad]) + assert await roster.app_directories(GROUP, "video") == [] + finally: + await roster.close() + + +async def test_a_read_only_root_is_refused_where_writability_is_required(tmp_path): + """ + Chat's directory is a destination, not a view. Storing one on a read-only + root would produce a paperclip that fails at the moment somebody uses it, + which is the failure the RO/RW model exists to move earlier. + """ + state, roster = await _state(tmp_path) + try: + with pytest.raises(ops.OpError, match="read-only"): + await ops.set_app_directory(state, GROUP, "chat", "Published", + require_writable=True) + # The same path is fine for an app that only reads it. + await ops.set_app_directories(state, GROUP, "video", ["Published"]) + assert await roster.app_directories(GROUP, "video") == ["Published"] + finally: + await roster.close() + + +async def test_a_directory_on_an_unplugged_drive_can_still_be_configured(tmp_path): + """ + Deliberately *not* `RootSet.resolve()`, which also refuses a root that is + currently unavailable. An operator must be able to point an app at a + library on a drive they have ejected — what is checked is the shape, which + does not change with what happens to be mounted. + """ + state, roster = await _state(tmp_path) + roots = state["groups_ctx"][GROUP]["roots"] + roots.roots[0].available = False + try: + out = await ops.set_app_directories(state, GROUP, "video", ["Media/Films"]) + assert out["directories"] == ["Media/Films"] + finally: + await roster.close() + + +# ── The derived scalar ─────────────────────────────────────────────────────── + +async def test_the_legacy_scalar_follows_the_list_in_the_live_context(tmp_path): + """ + `video_root` rides on the handshake ack for MNP 1.0 clients and is read + from the group context. Left behind by a save, it would disagree with the + list until the next restart. + """ + state, roster = await _state(tmp_path) + ctx = state["groups_ctx"][GROUP] + try: + await ops.set_app_directories(state, GROUP, "video", + ["Media/Films", "Media/Albums"]) + assert ctx["video_directories"] == ["Media/Albums", "Media/Films"] + assert ctx["video_root"] == "Media/Albums", ( + "the scalar must be the first of the list, not a stale value") + + await ops.set_app_directories(state, GROUP, "video", []) + assert ctx["video_root"] == "" + finally: + await roster.close() + + +async def test_the_photo_alias_stays_a_list_and_chat_stays_a_string(tmp_path): + """The alias table has to carry the shape, not just the name.""" + state, roster = await _state(tmp_path) + ctx = state["groups_ctx"][GROUP] + try: + await ops.set_app_directories(state, GROUP, "photo", + ["Media/Films", "Media/Albums"]) + assert ctx["photo_roots"] == ["Media/Albums", "Media/Films"] + await ops.set_app_directory(state, GROUP, "chat", "Media", + require_writable=True) + assert ctx["chat_directory"] == "Media" + finally: + await roster.close() + + +# ── Reading what an older node stored ──────────────────────────────────────── + +async def test_an_existing_video_root_is_read_without_a_migration(tmp_path): + """ + A node upgraded into this reads its old key until the first save through + the new path. Requiring a migration script to run before the Videos tab + works again would be a step nobody performs on the machine where it + matters. + """ + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + try: + await roster.set_setting(GROUP, "video_root", "Media/Films") + assert await roster.app_directories(GROUP, "video") == ["Media/Films"] + + await roster.set_setting(GROUP, "audio_root", "Media/Albums") + assert await roster.app_directories(GROUP, "music") == ["Media/Albums"] + + await roster.set_setting(GROUP, "photo_roots", '["A", "B"]') + assert await roster.app_directories(GROUP, "photo") == ["A", "B"] + finally: + await roster.close() + + +async def test_an_empty_legacy_value_means_nothing_configured(tmp_path): + """`video_root = ""` was how "unset" was spelled; it must not become `[""]`.""" + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + try: + await roster.set_setting(GROUP, "video_root", "") + assert await roster.app_directories(GROUP, "video") == [] + finally: + await roster.close() + + +async def test_the_new_key_wins_over_the_legacy_one(tmp_path): + """ + Both present is a group saved once through the new path. Reading the old + key there would undo that save on every load. + """ + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + try: + await roster.set_setting(GROUP, "video_root", "Old/Place") + await roster.set_app_directories(GROUP, "video", ["New/Place"]) + assert await roster.app_directories(GROUP, "video") == ["New/Place"] + finally: + await roster.close() + + +async def test_an_app_with_no_legacy_name_simply_has_none(tmp_path): + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + try: + assert await roster.app_directories(GROUP, "helloworld") == [] + finally: + await roster.close() + + +# ── Chat's own settings ────────────────────────────────────────────────────── + +async def test_link_previews_default_on_and_survive_a_restart(tmp_path): + """ + Absent means on, because that is what the node did before the switch + existed — an upgrade must not silently change what a group's chat does. + """ + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + try: + assert await roster.chat_link_preview(GROUP) is True + await roster.set_chat_link_preview(GROUP, False, set_by="op") + assert await roster.chat_link_preview(GROUP) is False + finally: + await roster.close() + + reopened = Roster(db_path=tmp_path / "roster.db") + await reopened.open() + try: + assert await reopened.chat_link_preview(GROUP) is False + assert await reopened.chat_link_preview("other") is True, ( + "one group's setting must not answer for another") + finally: + await reopened.close() + + +async def test_setting_link_previews_updates_the_live_context(tmp_path): + """ + The unfurl handler reads the context, not the database — it runs per + message and a round trip there would be absurd. So the two are kept in + step by the op that changes it. + """ + state, roster = await _state(tmp_path) + try: + await ops.set_chat_link_preview(state, GROUP, False) + assert state["groups_ctx"][GROUP]["chat_link_preview"] is False + finally: + await roster.close() diff --git a/packages/meshbay-node/tests/test_audio_root_gates_enrichment.py b/packages/meshbay-node/tests/test_audio_root_gates_enrichment.py index deab1bd..5a8f9b1 100644 --- a/packages/meshbay-node/tests/test_audio_root_gates_enrichment.py +++ b/packages/meshbay-node/tests/test_audio_root_gates_enrichment.py @@ -102,7 +102,7 @@ async def test_only_entries_under_the_configured_root_are_enriched(tmp_path): daemon = await _make_daemon(tmp_path, shared, group_id) try: - await daemon._roster.set_audio_root(group_id, "shared/Music", set_by="op") + await daemon._roster.set_app_directories(group_id, "music", ["shared/Music"], set_by="op") indexer = DirectoryIndexer( roots=one_root(shared), group_id=group_id, sk_node=Ed25519PrivateKey.generate(), gek=generate_gek()) @@ -142,8 +142,10 @@ async def test_setting_the_audio_root_sweeps_what_it_already_contains(tmp_path): # this file all along. state = { "roster": daemon._roster, - "groups_ctx": {group_id: {}}, - "enrich_audio_root_fn": daemon._enrich_audio_root_now, + # Real roots, because ops now refuses a directory that is not + # inside one — the per-app setters this replaced validated nothing. + "groups_ctx": {group_id: {"roots": one_root(shared)}}, + "enrich_app_dirs_fns": {"music": daemon._enrich_audio_root_now}, } await ops.set_audio_root(state, group_id, "shared/Music") await asyncio.sleep(0.05) # let the fire-and-forget sweep actually run @@ -197,8 +199,10 @@ async def test_the_same_file_shared_into_two_groups_enriches_in_both(tmp_path): "the fixture itself must produce identical content hashes — " "otherwise this test isn't exercising the collision at all") - await daemon._roster.set_audio_root(group_a, "shared_a/Music", set_by="op") - await daemon._roster.set_audio_root(group_b, "shared_b/Music", set_by="op") + await daemon._roster.set_app_directories( + group_a, "music", ["shared_a/Music"], set_by="op") + await daemon._roster.set_app_directories( + group_b, "music", ["shared_b/Music"], set_by="op") await daemon._enrich_new_audio_entries(indexer_a, list(indexer_a.index.entries)) await daemon._enrich_new_audio_entries(indexer_b, list(indexer_b.index.entries)) diff --git a/packages/meshbay-node/tests/test_audio_root_policy.py b/packages/meshbay-node/tests/test_audio_root_policy.py index 576c08a..e2e9254 100644 --- a/packages/meshbay-node/tests/test_audio_root_policy.py +++ b/packages/meshbay-node/tests/test_audio_root_policy.py @@ -125,17 +125,20 @@ async def test_the_setting_lives_on_the_node_and_survives_a_restart(tmp_path): roster = Roster(db_path=tmp_path / "roster.db") await roster.open() try: - assert await roster.audio_root("g1") == "", "absent must mean unset" - await roster.set_audio_root("g1", "shared/Music", set_by="op") - assert await roster.audio_root("g1") == "shared/Music" + assert await roster.app_directories("g1", "music") == [], ( + "absent must mean unset") + await roster.set_app_directories("g1", "music", ["shared/Music"], + set_by="op") + assert await roster.app_directories("g1", "music") == ["shared/Music"] finally: await roster.close() reopened = Roster(db_path=tmp_path / "roster.db") await reopened.open() try: - assert await reopened.audio_root("g1") == "shared/Music" - assert await reopened.audio_root("g2") == "", "one group's setting must not answer for another" + assert await reopened.app_directories("g1", "music") == ["shared/Music"] + assert await reopened.app_directories("g2", "music") == [], ( + "one group's setting must not answer for another") finally: await reopened.close() @@ -239,7 +242,7 @@ async def test_a_real_signed_save_persists_and_survives_a_fresh_roster_read(tmp_ assert session._ctx["groups"][GROUP]["audio_root"] == "shared/Music", ( "the live in-memory context must reflect the new root immediately") - assert await roster.audio_root(GROUP) == "shared/Music", ( + assert await roster.app_directories(GROUP, "music") == ["shared/Music"], ( "the same Roster instance must read back what it just wrote") finally: await roster.close() @@ -249,7 +252,7 @@ async def test_a_real_signed_save_persists_and_survives_a_fresh_roster_read(tmp_ # actually answers "does it survive a reload". reopened = await open_roster(tmp_path) try: - assert await reopened.audio_root(GROUP) == "shared/Music", ( + assert await reopened.app_directories(GROUP, "music") == ["shared/Music"], ( "a freshly-opened Roster against the same db file must see the " "committed value — anything else means the write was never " "durable in the first place") diff --git a/packages/meshbay-node/tests/test_rename_reenrichment.py b/packages/meshbay-node/tests/test_rename_reenrichment.py index 7a77368..f6761d9 100644 --- a/packages/meshbay-node/tests/test_rename_reenrichment.py +++ b/packages/meshbay-node/tests/test_rename_reenrichment.py @@ -37,8 +37,8 @@ def _free_port() -> int: class _StubRoster: - async def video_root(self, group_id): - return "shared" + async def app_directories(self, group_id, app_key): + return ["shared"] if app_key == "video" else [] class _SpyEnricher: diff --git a/packages/meshbay-node/tests/test_root_eject.py b/packages/meshbay-node/tests/test_root_eject.py index 0ec36a4..f57fb92 100644 --- a/packages/meshbay-node/tests/test_root_eject.py +++ b/packages/meshbay-node/tests/test_root_eject.py @@ -17,7 +17,6 @@ exactly what an operator does after noticing a drive fell off — silently undid the eject, and the next scan read an empty mount point as an erased library. """ -import asyncio from pathlib import Path import pytest diff --git a/packages/meshbay-node/tests/test_startup_scan_enrichment.py b/packages/meshbay-node/tests/test_startup_scan_enrichment.py index 65b9728..d2cbc3e 100644 --- a/packages/meshbay-node/tests/test_startup_scan_enrichment.py +++ b/packages/meshbay-node/tests/test_startup_scan_enrichment.py @@ -60,8 +60,9 @@ async def test_a_file_already_on_disk_at_startup_gets_enrichment_scheduled(tmp_p data_dir=tmp_path / "data", ) class _StubRoster: - async def video_root(self, group_id): - return "shared" # the root itself, i.e. "enrich the whole thing" + async def app_directories(self, group_id, app_key): + # The root itself, i.e. "enrich the whole thing". + return ["shared"] if app_key == "video" else [] daemon = NodeDaemon(config) daemon._broadcast_coalesce_secs = 0.01 # real value would make this test wait 0.5s diff --git a/packages/meshbay-node/tests/test_video_root_gates_enrichment.py b/packages/meshbay-node/tests/test_video_root_gates_enrichment.py index 9cfb819..b88ecaf 100644 --- a/packages/meshbay-node/tests/test_video_root_gates_enrichment.py +++ b/packages/meshbay-node/tests/test_video_root_gates_enrichment.py @@ -99,7 +99,7 @@ async def test_only_entries_under_the_configured_root_are_enriched(tmp_path): daemon = await _make_daemon(tmp_path, shared, group_id) try: - await daemon._roster.set_video_root(group_id, "shared/Movies", set_by="op") + await daemon._roster.set_app_directories(group_id, "video", ["shared/Movies"], set_by="op") indexer = DirectoryIndexer( roots=one_root(shared), group_id=group_id, sk_node=Ed25519PrivateKey.generate(), gek=generate_gek()) @@ -139,8 +139,10 @@ async def test_setting_the_video_root_sweeps_what_it_already_contains(tmp_path): # this file all along. state = { "roster": daemon._roster, - "groups_ctx": {group_id: {}}, - "enrich_video_root_fn": daemon._enrich_video_root_now, + # Real roots, because ops now refuses a directory that is not + # inside one — the per-app setters this replaced validated nothing. + "groups_ctx": {group_id: {"roots": one_root(shared)}}, + "enrich_app_dirs_fns": {"video": daemon._enrich_video_root_now}, } await ops.set_video_root(state, group_id, "shared/Movies") await asyncio.sleep(0.05) # let the fire-and-forget sweep actually run diff --git a/packages/meshbay-node/tests/test_video_root_policy.py b/packages/meshbay-node/tests/test_video_root_policy.py index 8cc1540..8d8c45a 100644 --- a/packages/meshbay-node/tests/test_video_root_policy.py +++ b/packages/meshbay-node/tests/test_video_root_policy.py @@ -126,16 +126,19 @@ async def test_the_setting_lives_on_the_node_and_survives_a_restart(tmp_path): roster = Roster(db_path=tmp_path / "roster.db") await roster.open() try: - assert await roster.video_root("g1") == "", "absent must mean the whole group index" - await roster.set_video_root("g1", "shared/Movies", set_by="op") - assert await roster.video_root("g1") == "shared/Movies" + assert await roster.app_directories("g1", "video") == [], ( + "absent must mean nothing configured") + await roster.set_app_directories("g1", "video", ["shared/Movies"], + set_by="op") + assert await roster.app_directories("g1", "video") == ["shared/Movies"] finally: await roster.close() reopened = Roster(db_path=tmp_path / "roster.db") await reopened.open() try: - assert await reopened.video_root("g1") == "shared/Movies" - assert await reopened.video_root("g2") == "", "one group's setting must not answer for another" + assert await reopened.app_directories("g1", "video") == ["shared/Movies"] + assert await reopened.app_directories("g2", "video") == [], ( + "one group's setting must not answer for another") finally: await reopened.close() -- cgit v1.2.3 From 4c4e9ba7a17e058dc12cb10171743329201dd7e6 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 21:25:23 +0200 Subject: fix(node): adding a root reached node.toml but not the running node MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `add_root` appended to the config and to node.toml and stopped there. `_retarget_indexer` — the MNP path — then re-points the indexer at `groups_ctx[gid]["roots"]`, an object nobody had touched, so it was retargeted at exactly what it already had. The directory was in the config file and invisible everywhere else until a restart. Worse than invisible: the ack does carry the new set, so the client showed the directory for one paint and the next index_sync took it away again — which reads as a UI bug and is not one. Adding it a second time was then refused as colliding with itself, which is the only reason anyone found out. `remove_root` and `update_root` already updated the live set; this one was missed. The loopback API hid it, because `ui/app.py` fires `reload_fn()` after the op and that re-reads node.toml from disk. The MNP path does not, and the shared-directories table only started offering Add over MNP in this refactor — a latent bug made reachable. The ack now describes the set the node will actually serve rather than one built on the side, so the two cannot disagree. test_root_ops_reach_the_live_set.py holds all three ops to it, including the counter-property that the same directory is still refused twice and that node.toml and the live set stay in step — the two halves drifting is how an operator's next restart silently undoes their last change. Four of its seven fail against the code above. Recovery on a node already in this state is `meshbay-node reload`: node.toml has everything, nothing was lost. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- packages/meshbay-node/src/meshbay_node/ops.py | 23 ++- .../tests/test_root_ops_reach_the_live_set.py | 192 +++++++++++++++++++++ 2 files changed, 214 insertions(+), 1 deletion(-) create mode 100644 packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py (limited to 'packages/meshbay-node/tests') diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index 11694c3..10f6a8f 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -682,9 +682,30 @@ async def add_root(state: dict, group_id: str, path: str, *, writable=added.writable, removable=added.removable, direct=added.direct)) + # The *live* set, not only the config — the same thing `remove_root` and + # `update_root` do, and the one this was missing. + # + # `_retarget_indexer` (the MNP path) re-points the indexer at + # `groups_ctx[gid]["roots"]`, so leaving that object untouched retargeted + # it at exactly what it already had: node.toml gained the directory, the + # index went on reporting the old set, and the next `index_sync` overwrote + # whatever the ack had just told the client. The root was there, invisible, + # until a restart — and adding it again was refused as a duplicate of + # itself, which is the only reason anyone found out. + # + # Safe to append rather than rebuild: `RootSet.build(specs)` above already + # validated the whole set, this root included, for name collisions and + # nesting. + live_roots: RootSet | None = state.get("groups_ctx", {}).get( + group_id, {}).get("roots") + if live_roots is not None and not any( + r.folded == added.folded for r in live_roots.roots): + live_roots.roots.append(added) + log.info("Root added: %s → group %s", added.name, group_id[:8]) return {"status": "added", "name": added.name, "path": str(added.path), - "group_id": group_id, "roots": built.describe()} + "group_id": group_id, + "roots": (live_roots or built).describe()} async def remove_root(state: dict, group_id: str, root_name: str) -> dict: diff --git a/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py b/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py new file mode 100644 index 0000000..d45a5b2 --- /dev/null +++ b/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py @@ -0,0 +1,192 @@ +""" +A root operation changes what the node is *serving*, not only what it will +serve after a restart. + +Every root op writes two places: `node.toml`, which survives a restart, and the +live `RootSet` in `groups_ctx[gid]["roots"]`, which is what the running node +answers from. The index payload is built from the second, so an op that updates +only the first is invisible until the daemon is restarted — and worse than +invisible, because the ack it sends *does* carry the change, so the client shows +it for a moment and the next `index_sync` takes it away again. + +`add_root` was like that. It appended to the config and to node.toml, and +`_retarget_indexer` then re-pointed the indexer at a `RootSet` object nobody had +touched — retargeting it at exactly what it already had. Found by an operator +adding a directory, seeing nothing, and being told on the second attempt that +its name collided with itself. + +The loopback API hid it: `ui/app.py` fires `reload_fn()` after the op, which +re-reads node.toml from disk. The MNP path does not, and the shared-directories +table started offering Add over MNP in this refactor — a latent bug made +reachable. +""" + +from dataclasses import asdict +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.config import GroupConfig, NodeConfig, RootSpec +from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.roots import RootSet +from meshbay_node.roster import Roster + +pytestmark = pytest.mark.asyncio + +GROUP = "g" * 32 + + +async def _state(tmp_path: Path) -> tuple[dict, Roster]: + """A node hosting one group with two roots, as node.toml and as live state.""" + for name in ("one", "two"): + (tmp_path / name).mkdir() + + cfg = GroupConfig(id=GROUP, name="plop", roots=[ + RootSpec(path=str(tmp_path / "one"), name="one"), + RootSpec(path=str(tmp_path / "two"), name="two"), + ]) + node_cfg = NodeConfig.__new__(NodeConfig) + node_cfg.groups = [cfg] + + conf = tmp_path / "node.toml" + conf.write_text( + f'[[groups]]\nid = "{GROUP}"\nname = "plop"\n\n' + f' [[groups.roots]]\n path = "{(tmp_path / "one").as_posix()}"\n' + f' name = "one"\n\n' + f' [[groups.roots]]\n path = "{(tmp_path / "two").as_posix()}"\n' + f' name = "two"\n') + + live = RootSet.build([asdict(r) for r in cfg.roots]) + index = GroupIndex(group_id=GROUP, sk_node=Ed25519PrivateKey.generate()) + roster = Roster(db_path=tmp_path / "roster.db") + await roster.open() + state = { + "config": node_cfg, + "config_path": str(conf), + "groups_ctx": {GROUP: {"index": index, "roots": live}}, + "roster": roster, + "node_user_id": "operator", + } + return state, roster + + +def _live(state) -> RootSet: + return state["groups_ctx"][GROUP]["roots"] + + +# ── Adding ─────────────────────────────────────────────────────────────────── + +async def test_adding_a_root_reaches_the_running_node(tmp_path): + state, roster = await _state(tmp_path) + (tmp_path / "uploads").mkdir() + try: + result = await ops.add_root(state, GROUP, str(tmp_path / "uploads")) + + assert [r.name for r in _live(state)] == ["one", "two", "uploads"], ( + "the live root set did not learn about the new directory, so the " + "index will keep reporting the old one until a restart") + assert [r["name"] for r in result["roots"]] == ["one", "two", "uploads"] + finally: + await roster.close() + + +async def test_the_ack_describes_the_set_the_node_will_serve(tmp_path): + """ + Not a set built on the side. Describing something the node is not actually + using is how the client shows a directory for one paint and loses it on the + next index push — which reads as a UI bug and is not one. + """ + state, roster = await _state(tmp_path) + (tmp_path / "uploads").mkdir() + try: + result = await ops.add_root(state, GROUP, str(tmp_path / "uploads")) + assert result["roots"] == _live(state).describe() + finally: + await roster.close() + + +async def test_adding_the_same_directory_twice_is_still_refused(tmp_path): + """ + The counter-property. The live set gaining the root must not make the + collision check pass the second time — a group with the same path under two + names indexes every file in it twice. + """ + state, roster = await _state(tmp_path) + (tmp_path / "uploads").mkdir() + try: + await ops.add_root(state, GROUP, str(tmp_path / "uploads")) + with pytest.raises(ops.OpError): + await ops.add_root(state, GROUP, str(tmp_path / "uploads")) + assert len(_live(state)) == 3, "the refused add left something behind" + finally: + await roster.close() + + +async def test_a_second_different_root_still_lands(tmp_path): + state, roster = await _state(tmp_path) + (tmp_path / "uploads").mkdir() + (tmp_path / "incoming").mkdir() + try: + await ops.add_root(state, GROUP, str(tmp_path / "uploads")) + await ops.add_root(state, GROUP, str(tmp_path / "incoming"), + writable=True) + assert [r.name for r in _live(state)] == [ + "one", "two", "uploads", "incoming"] + assert _live(state).by_name("incoming").writable is True + finally: + await roster.close() + + +# ── The other two, which already did this ──────────────────────────────────── + +async def test_removing_a_root_reaches_the_running_node(tmp_path): + state, roster = await _state(tmp_path) + try: + result = await ops.remove_root(state, GROUP, "two") + assert [r.name for r in _live(state)] == ["one"] + assert result["roots"] == _live(state).describe() + finally: + await roster.close() + + +async def test_updating_a_root_reaches_the_running_node(tmp_path): + state, roster = await _state(tmp_path) + state["config"] = SimpleNamespace(groups=state["config"].groups) + try: + result = await ops.update_root(state, GROUP, "two", + writable=True, removable=True) + live = _live(state).by_name("two") + assert live.writable is True and live.removable is True + assert result["roots"] == _live(state).describe() + finally: + await roster.close() + + +# ── And node.toml, so a restart agrees with the running node ───────────────── + +async def test_the_config_file_and_the_live_set_say_the_same_thing(tmp_path): + """ + The two halves must not drift: what the node serves now and what it will + serve after a restart are the same answer, or the operator's next restart + silently undoes their last change. + """ + state, roster = await _state(tmp_path) + (tmp_path / "uploads").mkdir() + try: + await ops.add_root(state, GROUP, str(tmp_path / "uploads"), + writable=True) + await ops.remove_root(state, GROUP, "one") + + from_disk = RootSet.build([ + asdict(r) for r in state["config"].groups[0].roots]) + assert ([r.name for r in from_disk] + == [r.name for r in _live(state)]) + + text = Path(state["config_path"]).read_text() + assert text.count("[[groups.roots]]") == 2 + assert "uploads" in text + finally: + await roster.close() -- cgit v1.2.3 From 232fd2a8d15f63b6bf6ad26286dd9836d6819668 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 21:43:00 +0200 Subject: fix(node): the MNP root path never reloaded, and my first repair made it worse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit was the wrong fix. `add_root` did leave the running node unchanged, but editing the live `RootSet` in place — which is what I did — is wrong in the other direction. `DirectoryIndexer.retarget` decides what to scan by diffing the names it already holds against the ones it is handed, and `_retarget_indexer` hands it `groups_ctx[gid]["roots"]`: the very object the op had edited. So the new root sat on both sides of the comparison, nothing was scanned, and the directory would have appeared in the table permanently empty. `_reload_config_inner` diffs the same way and would have concluded nothing changed. `remove_root` had the same shape and would have kept serving a removed directory's files. The real defect is that two front doors did different things. `ui/app.py` has always fired the daemon's `reload_fn` after these ops, which re-reads node.toml and builds a *fresh* set; the MNP path retargeted a stale object instead. That asymmetry is exactly what `ops.py` exists to prevent, and it is why the bug survived until an operator added a directory from a browser — the loopback path worked all along. So: the ops leave the live set alone, `_retarget_indexer` asks the daemon to reload, and `update_root` keeps editing in place because flags change no files and the synchronous upload handler reads that object on the next request. The tests now check the files rather than `describe()`, which proves nothing about whether anything was scanned. One of them demonstrates the failure mode instead of describing it, so the rule is checkable and will say so if `retarget` ever changes. Two more cover the seam itself — that the MNP path reloads, and that a context with no daemon still retargets. Diagnosed by reading the running node's journal rather than the source: the first add logged "Reloading config" and a rescan, the two later ones logged neither. I should have looked there before the first attempt. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- packages/meshbay-node/src/meshbay_node/ops.py | 50 ++-- .../src/meshbay_node/transport/webrtc_server.py | 28 +- .../tests/test_root_ops_reach_the_live_set.py | 301 ++++++++++++++++----- 3 files changed, 280 insertions(+), 99 deletions(-) (limited to 'packages/meshbay-node/tests') diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index 10f6a8f..5b10452 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -682,30 +682,21 @@ async def add_root(state: dict, group_id: str, path: str, *, writable=added.writable, removable=added.removable, direct=added.direct)) - # The *live* set, not only the config — the same thing `remove_root` and - # `update_root` do, and the one this was missing. + # Deliberately *not* mutating the live RootSet in place. # - # `_retarget_indexer` (the MNP path) re-points the indexer at - # `groups_ctx[gid]["roots"]`, so leaving that object untouched retargeted - # it at exactly what it already had: node.toml gained the directory, the - # index went on reporting the old set, and the next `index_sync` overwrote - # whatever the ack had just told the client. The root was there, invisible, - # until a restart — and adding it again was refused as a duplicate of - # itself, which is the only reason anyone found out. + # `DirectoryIndexer.retarget` decides what to scan by diffing the names it + # already has against the ones it is given — so handing it the same object, + # edited, means the new root is in both sides of the comparison and is + # never scanned. It would appear in the table and stay permanently empty. + # `_reload_config_inner` diffs the same way and would likewise conclude + # nothing changed. The caller reloads instead, which builds a fresh set + # from the file this just wrote. # - # Safe to append rather than rebuild: `RootSet.build(specs)` above already - # validated the whole set, this root included, for name collisions and - # nesting. - live_roots: RootSet | None = state.get("groups_ctx", {}).get( - group_id, {}).get("roots") - if live_roots is not None and not any( - r.folded == added.folded for r in live_roots.roots): - live_roots.roots.append(added) - + # `built` is that set, computed here only to validate and to answer with; + # what the node serves comes from the reload. log.info("Root added: %s → group %s", added.name, group_id[:8]) return {"status": "added", "name": added.name, "path": str(added.path), - "group_id": group_id, - "roots": (live_roots or built).describe()} + "group_id": group_id, "roots": built.describe()} async def remove_root(state: dict, group_id: str, root_name: str) -> dict: @@ -741,20 +732,15 @@ async def remove_root(state: dict, group_id: str, root_name: str) -> dict: cfg.roots.pop(match_idx) - # Update the live RootSet so GET /api/groups returns correct data - # immediately, without waiting for the async reload. - live_roots = state.get("groups_ctx", {}).get( - group_id, {}).get("roots") - if live_roots: - live_roots.roots = [ - r for r in live_roots.roots if fold(r.name) != target] - - # Built from config when there is no live set, never returned empty: an + # Not mutating the live set here either — see `add_root`. Dropping the + # root from it would leave `retarget` unable to tell that its entries + # should go, so the removed directory's files would stay in the index. + # + # Built from the config this just edited, and never returned empty: an # empty list is a *valid answer* meaning "this group has no directories", - # and the client cannot tell it from "the node could not say". It would + # which the client cannot tell from "the node could not say" — it would # blank the operator's table on an op that succeeded. - result_roots = (live_roots.describe() if live_roots - else RootSet.build([asdict(r) for r in cfg.roots]).describe()) + result_roots = RootSet.build([asdict(r) for r in cfg.roots]).describe() log.info("Root removed: %s from group %s", root_name, group_id[:8]) return {"status": "removed", "name": root_name, "group_id": group_id, 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 c4d053e..d341d8c 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -2773,10 +2773,36 @@ class WebRTCPeerSession: return await fn(state, *args, **kwargs) async def _retarget_indexer(self, group_id: str) -> None: - """Tell the indexer to rescan after roots changed.""" + """ + Pick up a root that was just added to or removed from node.toml. + + Through the daemon's own reload, which is what the loopback API has + always done after the same operations (`ui/app.py`). This used to + re-point the indexer at `groups_ctx[gid]["roots"]` instead — the very + object the op had just edited — so `retarget` diffed a set against + itself, found no new names, scanned nothing, and dropped nothing. A + directory added over MNP reached node.toml and was invisible until a + restart; one removed kept serving its files. + + Two front doors doing different things is the shape `ops.py` exists to + prevent, and this was it: the loopback path worked and the MNP path did + not, which is why it survived until the operator added a directory from + a browser. + + Not awaited: a reload rescans, and a new library is minutes. The ack + the caller sends carries the set the node is moving to, and the + `index_sync` that follows the scan carries what it found. + """ state = self._ctx.get("daemon_state") if not state: return + reload_fn = state.get("reload_fn") + if reload_fn: + self._spawn(reload_fn()) + return + # No daemon to ask — a test harness, or a context assembled by hand. + # Retarget directly, which is correct as long as the caller did not + # edit the live set in place. indexer = state.get("indexers", {}).get(group_id) roots = state.get("groups_ctx", {}).get(group_id, {}).get("roots") if indexer and roots: diff --git a/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py b/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py index d45a5b2..976af82 100644 --- a/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py +++ b/packages/meshbay-node/tests/test_root_ops_reach_the_live_set.py @@ -1,24 +1,25 @@ """ -A root operation changes what the node is *serving*, not only what it will -serve after a restart. - -Every root op writes two places: `node.toml`, which survives a restart, and the -live `RootSet` in `groups_ctx[gid]["roots"]`, which is what the running node -answers from. The index payload is built from the second, so an op that updates -only the first is invisible until the daemon is restarted — and worse than -invisible, because the ack it sends *does* carry the change, so the client shows -it for a moment and the next `index_sync` takes it away again. - -`add_root` was like that. It appended to the config and to node.toml, and -`_retarget_indexer` then re-pointed the indexer at a `RootSet` object nobody had -touched — retargeting it at exactly what it already had. Found by an operator -adding a directory, seeing nothing, and being told on the second attempt that -its name collided with itself. - -The loopback API hid it: `ui/app.py` fires `reload_fn()` after the op, which -re-reads node.toml from disk. The MNP path does not, and the shared-directories -table started offering Add over MNP in this refactor — a latent bug made -reachable. +Adding or removing a root has to reach the running node, not only node.toml. + +Two front doors do this — the loopback API and a signed MNP op — and `ops.py` +exists so they behave identically. They did not. The loopback path fired the +daemon's `reload_fn`, which re-reads node.toml and builds a fresh `RootSet`; +the MNP path instead re-pointed the indexer at `groups_ctx[gid]["roots"]`, the +very object the op had just been asked about. `DirectoryIndexer.retarget` +decides what to scan by diffing the names it holds against the ones it is +given, so a set compared against itself scans nothing and drops nothing. + +A directory added from a browser therefore reached node.toml and was invisible +everywhere else until a restart — and adding it again was refused as colliding +with itself, which is the only reason anyone found out. One removed would have +kept serving its files. + +**The obvious repair is wrong in the other direction**, and was committed once +before this file said so: making the op edit the live set in place puts the new +root on *both* sides of retarget's comparison. The table would show it and it +would stay permanently empty. So the ops leave that object alone, the MNP path +reloads like the loopback one always did, and the tests below check the files — +`describe()` agreeing proves nothing about whether anything was scanned. """ from dataclasses import asdict @@ -31,6 +32,7 @@ from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey from meshbay_node import ops from meshbay_node.config import GroupConfig, NodeConfig, RootSpec from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.indexer.indexer import DirectoryIndexer from meshbay_node.roots import RootSet from meshbay_node.roster import Roster @@ -77,50 +79,159 @@ def _live(state) -> RootSet: return state["groups_ctx"][GROUP]["roots"] -# ── Adding ─────────────────────────────────────────────────────────────────── +async def _indexer(state) -> DirectoryIndexer: + idx = DirectoryIndexer(roots=_live(state), group_id=GROUP, + sk_node=Ed25519PrivateKey.generate(), gek=None) + await idx.initial_scan() + return idx -async def test_adding_a_root_reaches_the_running_node(tmp_path): + +def _rebuilt(state) -> RootSet: + """What a reload produces: a fresh set from the config the op just wrote.""" + return RootSet.build([asdict(r) for r in state["config"].groups[0].roots]) + + +# ── What the op writes ─────────────────────────────────────────────────────── + +async def test_adding_a_root_reaches_node_toml_and_the_ack(tmp_path): state, roster = await _state(tmp_path) (tmp_path / "uploads").mkdir() try: result = await ops.add_root(state, GROUP, str(tmp_path / "uploads")) - assert [r.name for r in _live(state)] == ["one", "two", "uploads"], ( - "the live root set did not learn about the new directory, so the " - "index will keep reporting the old one until a restart") assert [r["name"] for r in result["roots"]] == ["one", "two", "uploads"] + assert [r.name for r in state["config"].groups[0].roots] == [ + "one", "two", "uploads"] + assert "uploads" in Path(state["config_path"]).read_text() + finally: + await roster.close() + + +async def test_removing_a_root_reaches_node_toml_and_the_ack(tmp_path): + state, roster = await _state(tmp_path) + try: + result = await ops.remove_root(state, GROUP, "two") + assert [r["name"] for r in result["roots"]] == ["one"] + assert Path(state["config_path"]).read_text().count( + "[[groups.roots]]") == 1 finally: await roster.close() -async def test_the_ack_describes_the_set_the_node_will_serve(tmp_path): +async def test_the_op_does_not_edit_the_live_set_in_place(tmp_path): """ - Not a set built on the side. Describing something the node is not actually - using is how the client shows a directory for one paint and loses it on the - next index push — which reads as a UI bug and is not one. + The property that made the original bug, and then made the first repair for + it wrong in the other direction. + + `retarget` diffs the names it holds against the ones it is handed. Editing + that same object and passing it back puts a new root on both sides of the + comparison: nothing is scanned, and the directory shows in the table + permanently empty. `_reload_config_inner` diffs the same way and would + likewise conclude nothing had changed. """ state, roster = await _state(tmp_path) + before = [r.name for r in _live(state)] (tmp_path / "uploads").mkdir() try: - result = await ops.add_root(state, GROUP, str(tmp_path / "uploads")) - assert result["roots"] == _live(state).describe() + await ops.add_root(state, GROUP, str(tmp_path / "uploads")) + assert [r.name for r in _live(state)] == before, ( + "add_root edited the live RootSet, which is the object retarget " + "diffs against — the new root would never be scanned") + + await ops.remove_root(state, GROUP, "two") + assert [r.name for r in _live(state)] == before, ( + "remove_root edited the live RootSet, so retarget cannot tell the " + "removed root's entries should go") finally: await roster.close() -async def test_adding_the_same_directory_twice_is_still_refused(tmp_path): +# ── What the node then serves ──────────────────────────────────────────────── + +async def test_a_retarget_from_the_config_scans_the_new_root(tmp_path): + """ + The half no assertion about `describe()` can reach: the files. + + A root that appears in the table and holds nothing is the same bug one step + later, and it is what editing the live set in place would produce. + """ + state, roster = await _state(tmp_path) + (tmp_path / "one" / "kept.txt").write_bytes(b"kept") + fresh = tmp_path / "uploads" + fresh.mkdir() + (fresh / "new.txt").write_bytes(b"new") + + idx = await _indexer(state) + assert {e.name for e in idx.index.entries} == {"kept.txt"} + try: + await ops.add_root(state, GROUP, str(fresh)) + await idx.retarget(_rebuilt(state)) + + assert {e.name for e in idx.index.entries} == {"kept.txt", "new.txt"}, ( + "the added directory was not scanned — it would show in the table " + "and stay empty") + assert [r["name"] for r in idx.index.roots] == ["one", "two", "uploads"] + finally: + await roster.close() + + +async def test_handing_retarget_the_edited_set_scans_nothing(tmp_path): """ - The counter-property. The live set gaining the root must not make the - collision check pass the second time — a group with the same path under two - names indexes every file in it twice. + The failure mode above, demonstrated rather than described — so the reason + the ops leave the live set alone is checkable instead of asserted in a + comment. If this ever starts failing, `retarget` has changed and the rule + in `add_root` can be revisited. """ state, roster = await _state(tmp_path) + fresh = tmp_path / "uploads" + fresh.mkdir() + (fresh / "new.txt").write_bytes(b"new") + + idx = await _indexer(state) + try: + await ops.add_root(state, GROUP, str(fresh)) + # What editing in place would have left behind. + _live(state).roots.append(_rebuilt(state).roots[-1]) + await idx.retarget(_live(state)) + + assert {e.name for e in idx.index.entries} == set(), ( + "retarget now scans a root it was handed on both sides of its own " + "diff — the constraint this file is built on has changed") + finally: + await roster.close() + + +async def test_a_retarget_from_the_config_drops_a_removed_root(tmp_path): + """The mirror: a removed directory's files must stop being served.""" + state, roster = await _state(tmp_path) + (tmp_path / "one" / "kept.txt").write_bytes(b"kept") + (tmp_path / "two" / "going.txt").write_bytes(b"going") + + idx = await _indexer(state) + assert {e.name for e in idx.index.entries} == {"kept.txt", "going.txt"} + try: + await ops.remove_root(state, GROUP, "two") + await idx.retarget(_rebuilt(state)) + assert {e.name for e in idx.index.entries} == {"kept.txt"}, ( + "the removed directory's files are still being served") + finally: + await roster.close() + + +# ── The invariants around them ─────────────────────────────────────────────── + +async def test_adding_the_same_directory_twice_is_still_refused(tmp_path): + """A group with one path under two names indexes every file in it twice.""" + state, roster = await _state(tmp_path) (tmp_path / "uploads").mkdir() try: await ops.add_root(state, GROUP, str(tmp_path / "uploads")) with pytest.raises(ops.OpError): await ops.add_root(state, GROUP, str(tmp_path / "uploads")) - assert len(_live(state)) == 3, "the refused add left something behind" + assert len(state["config"].groups[0].roots) == 3, ( + "the refused add left something behind") + assert Path(state["config_path"]).read_text().count( + "[[groups.roots]]") == 3 finally: await roster.close() @@ -131,28 +242,22 @@ async def test_a_second_different_root_still_lands(tmp_path): (tmp_path / "incoming").mkdir() try: await ops.add_root(state, GROUP, str(tmp_path / "uploads")) - await ops.add_root(state, GROUP, str(tmp_path / "incoming"), - writable=True) - assert [r.name for r in _live(state)] == [ + result = await ops.add_root(state, GROUP, str(tmp_path / "incoming"), + writable=True) + assert [r["name"] for r in result["roots"]] == [ "one", "two", "uploads", "incoming"] - assert _live(state).by_name("incoming").writable is True - finally: - await roster.close() - - -# ── The other two, which already did this ──────────────────────────────────── - -async def test_removing_a_root_reaches_the_running_node(tmp_path): - state, roster = await _state(tmp_path) - try: - result = await ops.remove_root(state, GROUP, "two") - assert [r.name for r in _live(state)] == ["one"] - assert result["roots"] == _live(state).describe() + assert result["roots"][-1]["writable"] is True finally: await roster.close() -async def test_updating_a_root_reaches_the_running_node(tmp_path): +async def test_updating_flags_may_edit_the_live_set(tmp_path): + """ + The exception, and why it is one: `writable` and `removable` change nothing + about which files exist, so there is nothing for retarget to scan or drop. + Editing in place is what makes the flag true for the upload handler on the + very next request, which is synchronous and reads the live set. + """ state, roster = await _state(tmp_path) state["config"] = SimpleNamespace(groups=state["config"].groups) try: @@ -165,13 +270,11 @@ async def test_updating_a_root_reaches_the_running_node(tmp_path): await roster.close() -# ── And node.toml, so a restart agrees with the running node ───────────────── - -async def test_the_config_file_and_the_live_set_say_the_same_thing(tmp_path): +async def test_the_file_on_disk_and_the_config_in_memory_agree(tmp_path): """ - The two halves must not drift: what the node serves now and what it will - serve after a restart are the same answer, or the operator's next restart - silently undoes their last change. + A reload re-reads the file, so a config edited in memory but not on disk is + undone by the next restart — and one written to disk but not in memory + makes the *next* op validate against a stale picture. """ state, roster = await _state(tmp_path) (tmp_path / "uploads").mkdir() @@ -180,13 +283,79 @@ async def test_the_config_file_and_the_live_set_say_the_same_thing(tmp_path): writable=True) await ops.remove_root(state, GROUP, "one") - from_disk = RootSet.build([ - asdict(r) for r in state["config"].groups[0].roots]) - assert ([r.name for r in from_disk] - == [r.name for r in _live(state)]) + import tomllib + on_disk = tomllib.loads(Path(state["config_path"]).read_text()) + disk_paths = [str(r["path"]) for r in on_disk["groups"][0]["roots"]] + memory_paths = [Path(r.path).as_posix() + for r in state["config"].groups[0].roots] + assert disk_paths == memory_paths + assert Path(state["config_path"]).read_text().count( + "[[groups.roots]]") == 2 + finally: + await roster.close() + + +# ── The seam that was actually broken ──────────────────────────────────────── - text = Path(state["config_path"]).read_text() - assert text.count("[[groups.roots]]") == 2 - assert "uploads" in text +async def test_the_mnp_path_reloads_like_the_loopback_one(tmp_path): + """ + The two front doors, doing the same thing. + + `ui/app.py` has always fired the daemon's `reload_fn` after a root op. + `_retarget_indexer` did not — it re-pointed the indexer at the live set + instead, which is the object the ops leave alone, so nothing happened at + all. That divergence *is* the bug: the loopback path worked, the MNP path + did not, and it survived until an operator added a directory from a + browser. + + Not awaited: a reload rescans, and a new library is minutes. The ack + already carries the set the node is moving to. + """ + from meshbay_node.transport.webrtc_server import WebRTCPeerSession + + state, roster = await _state(tmp_path) + reloaded: list[bool] = [] + + async def fake_reload(): + reloaded.append(True) + + state["reload_fn"] = fake_reload + spawned = [] + + session = WebRTCPeerSession.__new__(WebRTCPeerSession) + session._ctx = {"daemon_state": state} + session._spawn = lambda coro: spawned.append(coro) + try: + await session._retarget_indexer(GROUP) + assert spawned, "the MNP path did not ask the daemon to reload" + await spawned[0] + assert reloaded == [True] + finally: + await roster.close() + + +async def test_without_a_daemon_it_still_retargets(tmp_path): + """ + A context assembled by hand — a harness, or a test — has no `reload_fn`. + Falling through to a direct retarget keeps those working, and is correct + precisely because the ops no longer edit the set being passed. + """ + from meshbay_node.transport.webrtc_server import WebRTCPeerSession + + state, roster = await _state(tmp_path) + fresh = tmp_path / "uploads" + fresh.mkdir() + (fresh / "new.txt").write_bytes(b"new") + idx = await _indexer(state) + state["indexers"] = {GROUP: idx} + + session = WebRTCPeerSession.__new__(WebRTCPeerSession) + session._ctx = {"daemon_state": state} + try: + await ops.add_root(state, GROUP, str(fresh)) + # What a reload would have installed, done by hand here. + state["groups_ctx"][GROUP]["roots"] = _rebuilt(state) + await session._retarget_indexer(GROUP) + assert {e.name for e in idx.index.entries} == {"new.txt"} finally: await roster.close() -- cgit v1.2.3 From f3fb449f3a943096a2569dc383f2819a612bccd5 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 22:06:45 +0200 Subject: feat(node): an upload lands in the folder it was sent to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There is no `uploads/` subdirectory any more, and the client names the folder rather than the root. It was the last of v5's quarantine — the per-user layer went on 2026-08-14 for the same reason — and it goes on the same grounds: a folder appearing beside the operator's library because somebody sent a file is the node deciding how their disk is arranged. Somebody dropping a file into the folder they are looking at expects it to be in that folder. **What made the quarantine worth having was never the subdirectory.** It is the filename allowlist, the size cap, the chunk ordering and the no-overwrite rule, and all four are untouched: an existing file is never replaced, the second sender of IMG_1234.jpg gets a free name, and the check still sits at the write. Letting the client choose the destination is safe for one reason and only one: it is resolved through `RootSet.resolve()`, which refuses `..`, absolute segments and anything whose resolved form escapes its root, symlinks included. A member answers "which of this group's folders", never "which path on the operator's disk" — and the test that used to assert the node chose now asserts that, with six shapes of escape. `direct` goes with it. Its only job was to say "no subdirectory for this root", which is now every root, and a config flag that does nothing is worse than none. Chat's attachment folder finally does something: the directory the operator picks in the Chat settings pane is where attachments are written, falling back to the first writable root while they have not chosen one, or if the one they chose has since been made read-only or ejected — a stale choice should not become a refusal at send time. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- docs/meshbay-draft-v6.md | 14 +++- .../meshbay-hub/src/meshbay_hub/static/chat-app.js | 10 ++- .../src/meshbay_hub/static/files-app.js | 13 ++-- .../src/meshbay_hub/static/group-page.js | 13 +++- .../src/meshbay_hub/static/transport.js | 18 +++-- .../tests/test_upload_controls_hidden.py | 7 +- packages/meshbay-node/src/meshbay_node/config.py | 3 +- packages/meshbay-node/src/meshbay_node/ops.py | 3 +- packages/meshbay-node/src/meshbay_node/roots.py | 6 +- .../src/meshbay_node/transport/webrtc_server.py | 55 +++++++++---- .../tests/test_root_writable_policy.py | 8 +- .../tests/test_security_regressions.py | 90 +++++++++++++--------- 12 files changed, 156 insertions(+), 84 deletions(-) (limited to 'packages/meshbay-node/tests') diff --git a/docs/meshbay-draft-v6.md b/docs/meshbay-draft-v6.md index 4439cd1..28aea0c 100644 --- a/docs/meshbay-draft-v6.md +++ b/docs/meshbay-draft-v6.md @@ -74,12 +74,22 @@ v5 confines uploads to `shared_root/uploads/` with a filename allowlist, no overwrite, chunk ordering and a size cap. All four protections stand. Two amendments: -- There is no single `shared_root`. **Each root is read-only or read-write**, and the - quarantine lives inside whichever writable root the upload is addressed to. If that +- There is no single `shared_root`. **Each root is read-only or read-write**, and an + upload goes to the folder the sender is looking at, inside a writable root. If that root is unavailable the upload fails with a stated reason and never falls back to another; if the group has no writable root, uploads are refused rather than guessed. (Amended 2026-09-06 — the original text designated *one* root as the upload destination, and the client named none. See `docs/refactor-groups.md` §1.1.) +- **There is no `uploads/` quarantine directory any more** (2026-09-06). It was the + last of v5's, the per-user layer having gone on 2026-08-14, and it went for the same + reason: a folder appearing beside the operator's library because somebody sent a + file is the node deciding how their disk is arranged. **What made the quarantine + worth having was never the subdirectory** — it is the filename allowlist, the size + cap, the chunk ordering and the no-overwrite rule, and all four are unchanged. + The client now names the destination folder, which is safe for one reason and only + one: it is resolved through `RootSet.resolve()`, which refuses `..`, absolute + segments and anything escaping its root, symlinks included. A member answers "which + of this group's folders", never "which path on the operator's disk". - **The no-overwrite rule is unchanged and still holds on exFAT/NTFS.** An earlier draft claimed a string comparison let `README.TXT` land on `readme.txt` there. It does not: the check is `Path.exists()`, and `stat()` is itself case-insensitive on those diff --git a/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js b/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js index 4714674..0a7ef26 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/chat-app.js @@ -204,7 +204,8 @@ function ChatImage({ filename, entries, transportRef, gekRef }) { // root is read-only, or the one drive that was writable is unplugged — and the // paperclip says so rather than producing a refusal from the node. function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, - onPreview, attachRoot = '', onActivity, status }) { + onPreview, attachRoot = '', attachDir = '', + onActivity, status }) { const [messages, setMessages] = useState([]); const [hasMore, setHasMore] = useState(false); const [loadingOlder, setLoadingOlder] = useState(false); @@ -484,7 +485,10 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, try { // Two people sending IMG_1234.jpg both succeed; the node picks a free name // and the message has to point at the one it chose. - const ack = await transport.uploadFile(file, { root: attachRoot }); + // `attachDir` is the folder the operator chose in Settings; `attachRoot` + // is the fallback for a group where they have not chosen one yet. + const ack = await transport.uploadFile( + file, { root: attachRoot, dir: attachDir || undefined }); const storedAs = (ack && ack.stored_as) || file.name; await new Promise(r => setTimeout(r, 2500)); if (onRefreshIndex) await onRefreshIndex(); @@ -506,7 +510,7 @@ function ChatPanel({ transportRef, username, entries, gekRef, onRefreshIndex, } finally { setAttaching(false); } - }, [username, onRefreshIndex, jumpToBottom, attachRoot]); + }, [username, onRefreshIndex, jumpToBottom, attachRoot, attachDir]); const onKeyDown = useCallback((e) => { if (e.key === 'Enter' && !e.shiftKey) { diff --git a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js index a6a3ca4..9ca3aae 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js @@ -75,12 +75,12 @@ function FilesPanel({ e.target.value = ''; const transport = transportRef.current; if (!files.length || !transport || !transport.connected) return; - // The root being browsed is the destination. A group can have several - // writable roots, so leaving the node to pick one means a file uploaded - // from a folder the operator is looking at lands in a different one — - // which is only noticed much later, if at all. - const uploadRoot = currentPath ? currentPath.split('/')[0] : ''; - if (!uploadRoot) return; + // The folder on screen is the destination — not its root, and not a + // subdirectory of the node's invention. Somebody dropping a file into the + // folder they are looking at expects it to be in that folder. + const uploadDir = currentPath; + if (!uploadDir) return; + const uploadRoot = uploadDir.split('/')[0]; setError(''); for (const file of files) { @@ -92,6 +92,7 @@ function FilesPanel({ onProgress: (sent) => onProgress(sent, file.size), signal, root: uploadRoot, + dir: uploadDir, }); // The node re-indexes on a filesystem event, so there is nothing to // wait on but the clock. Refreshing here means the file appears in diff --git a/packages/meshbay-hub/src/meshbay_hub/static/group-page.js b/packages/meshbay-hub/src/meshbay_hub/static/group-page.js index 513792b..a1e6411 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/group-page.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/group-page.js @@ -590,7 +590,16 @@ function GroupPage({ groupId, group, token, username, userId, userPrefs, [nodeRoots]); const legacyNode = nodeRoots.length > 0 && nodeRoots.every((r) => r.writable === undefined); - const attachRoot = writableRoots.length ? writableRoots[0].name + // The operator's chosen attachment folder wins where there is one — that is + // what the Chat settings pane is for. Its root has to be writable and + // present, or the choice is stale (they made it read-only, or ejected the + // drive) and the fallback is better than a refusal at send time. + const chatDirRoot = chatDirectory ? chatDirectory.split('/')[0] : ''; + const chatDirUsable = Boolean( + chatDirRoot && writableRoots.some((r) => r.name === chatDirRoot)); + const attachDir = chatDirUsable ? chatDirectory : ''; + const attachRoot = chatDirUsable ? chatDirRoot + : writableRoots.length ? writableRoots[0].name : (legacyNode && memberUpload ? (nodeRoots.find((r) => r.upload) || nodeRoots[0]).name : ''); @@ -655,7 +664,7 @@ function GroupPage({ groupId, group, token, username, userId, userPrefs, groupId, transportRef, gekRef, status, username, entries, availableEntries, nodeDirs, nodeRoots, setEntries, setNodeDirs, setNodeRoots, applyIndex, - isNodeAdmin, operatorPaired, attachRoot, userId, setError, onPreview, + isNodeAdmin, operatorPaired, attachRoot, attachDir, userId, setError, onPreview, onRefreshIndex: refreshIndex, onActivity: touchActivity, // Plural everywhere: Videos and Music read a list now, and Photos always // did. The scalar `videoRoot`/`audioRoot` shapes survive only on the wire, diff --git a/packages/meshbay-hub/src/meshbay_hub/static/transport.js b/packages/meshbay-hub/src/meshbay_hub/static/transport.js index 32f8539..3acfd20 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/transport.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/transport.js @@ -1778,13 +1778,18 @@ class MeshBayTransport { * free one rather than replacing anything. The ack says which, and that is what * this returns. * - * `root` names which shared directory to upload into — a name, never a path; - * the node picks the destination inside it. Since a group can have several - * writable roots, leaving it out is a guess, and the node's fallback ("the - * first writable one") exists only for MNP 1.0 clients, which had exactly one - * destination. Every caller here browses a root and knows which one it is. + * `dir` names the folder to upload into, as a virtual path + * (`Media/Films/1999`) — where the sender is actually looking. The node + * resolves it against the group's own roots, which refuses `..`, absolute + * segments and anything escaping its root; it is a place among the group's + * folders, never a path on the operator's filesystem. + * + * `root` is the older, coarser form: the root's name and nothing below it. + * Kept because a node that predates `dir` reads it, and because Chat has no + * folder on screen to name. Omitting both leaves the node to pick, which it + * only does for a client old enough to have had one destination. */ - async uploadFile(file, { chunkSize, onProgress, signal, root } = {}) { + async uploadFile(file, { chunkSize, onProgress, signal, root, dir } = {}) { // The same file twice at once would confuse the node, which keys its own // upload state by name — and would race for the same destination. if (this._uploaders.has(file.name)) { @@ -1836,6 +1841,7 @@ class MeshBayTransport { total_chunks: total, data: buf, ...(root ? { root } : {}), + ...(dir ? { dir } : {}), }); } while (acked < total) { diff --git a/packages/meshbay-hub/tests/test_upload_controls_hidden.py b/packages/meshbay-hub/tests/test_upload_controls_hidden.py index ae1f444..7b8b0e3 100644 --- a/packages/meshbay-hub/tests/test_upload_controls_hidden.py +++ b/packages/meshbay-hub/tests/test_upload_controls_hidden.py @@ -125,8 +125,11 @@ def test_files_uploads_into_the_root_it_is_showing(): page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") upload = page[page.index("const uploadFile"):] upload = upload[:upload.index("const makeDirectory")] - assert "root: uploadRoot" in upload, "the node is left to choose" - assert "currentPath.split('/')[0]" in upload + assert "dir: uploadDir" in upload, "the node is left to choose the folder" + assert "const uploadDir = currentPath" in upload, ( + "the destination is not the folder on screen") + assert "root: uploadRoot" in upload, ( + "a node too old for `dir` reads `root`, and gets nothing without it") # ── Learning the answer ───────────────────────────────────────────────────── diff --git a/packages/meshbay-node/src/meshbay_node/config.py b/packages/meshbay-node/src/meshbay_node/config.py index 4712312..7673a51 100644 --- a/packages/meshbay-node/src/meshbay_node/config.py +++ b/packages/meshbay-node/src/meshbay_node/config.py @@ -184,7 +184,6 @@ class RootSpec: kind: str = "generic" # generic|video|audio|photo — a view hint, unused for now writable: bool = False # RW roots accept uploads from group members removable: bool = False # operator can eject this root before unplugging the device - direct: bool = False # uploads land at root path, not in a subdirectory @dataclass @@ -225,7 +224,7 @@ class GroupConfig: for r in self.roots: r.writable = False self.roots.append(RootSpec( - path=self.upload_dir.strip(), writable=True, direct=True)) + path=self.upload_dir.strip(), writable=True)) @dataclass diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index 5b10452..3dfdc23 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -679,8 +679,7 @@ async def add_root(state: dict, group_id: str, path: str, *, from meshbay_node.config import RootSpec cfg.roots.append(RootSpec( path=str(added.path), name=added.name, kind=added.kind, - writable=added.writable, removable=added.removable, - direct=added.direct)) + writable=added.writable, removable=added.removable)) # Deliberately *not* mutating the live RootSet in place. # diff --git a/packages/meshbay-node/src/meshbay_node/roots.py b/packages/meshbay-node/src/meshbay_node/roots.py index 9d3f7cb..ffe801c 100644 --- a/packages/meshbay-node/src/meshbay_node/roots.py +++ b/packages/meshbay-node/src/meshbay_node/roots.py @@ -122,7 +122,6 @@ class Root: kind: str = "generic" writable: bool = False removable: bool = False - direct: bool = False ejected: bool = False available: bool = True @@ -226,8 +225,7 @@ class RootSet: writable=writable, removable=bool(spec.get("removable", False)), ejected=bool(spec.get("ejected", False)), - available=not bool(spec.get("ejected", False)), - direct=bool(spec.get("direct", False))) + available=not bool(spec.get("ejected", False))) _refuse_nesting(root, roots) roots.append(root) by_folded[root.folded] = root @@ -370,8 +368,6 @@ class RootSet: "ejected": r.ejected, # Backward compat for MNP 1.0 clients "upload": r.writable} - if r.direct: - d["direct"] = True out.append(d) return out 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 d341d8c..61458f2 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -214,7 +214,6 @@ JOIN_FAILURE_WINDOW = 600 # seconds # attachments from the chat alike. One visible directory the operator can look # into, back up or empty — rather than a hidden tree of per-user uuids that # nobody could read, or files scattered wherever someone happened to be looking. -UPLOAD_DIR_NAME = "uploads" def _extract_dtls_fingerprint(sdp: str) -> bytes: @@ -4016,7 +4015,12 @@ class WebRTCPeerSession: # later — the same reason the old single upload root was never guessed. # A client that names nothing is an MNP 1.0 one, and there was exactly # one destination in its world: the first writable root. - target_root_name = str(msg.get("root") or "").strip() + # `dir` is the folder being browsed, as a virtual path + # (`Media/Films/1999`); `root` is the older, coarser form and is what + # its first segment means on its own. + target_rel = str(msg.get("dir") or "").strip().strip("/") + target_root_name = (target_rel.split("/")[0] if target_rel + else str(msg.get("root") or "").strip()) upload_root = None if target_root_name: upload_root = roots.by_name(target_root_name) @@ -4052,18 +4056,43 @@ class WebRTCPeerSession: "filename": filename}) return - if upload_root.direct: - rel_dir = upload_root.name - target_dir = upload_root.path + # The folder the sender is looking at, and no subdirectory of the node's + # invention. + # + # Uploads used to be confined to `/uploads/`, created on demand. + # That was the last of v5's quarantine (the per-user layer went on + # 2026-08-14, for the same reason): a shared directory nobody can + # organise is not a shared directory, and a folder appearing beside the + # operator's library because somebody sent a file is the node deciding + # how their disk is arranged. + # + # What made the quarantine worth having is not the subdirectory — it is + # the filename allowlist, the size cap, the chunk ordering, and the + # no-overwrite rule below. All four are unchanged. + # + # `resolve()` and not a join: it refuses `..`, absolute segments and + # anything whose resolved form escapes its root, symlinks included. The + # client names *where among the group's own folders*, never a path on + # the operator's filesystem. + if target_rel: + target_dir = roots.resolve(target_rel) + if target_dir is None or not target_dir.is_dir(): + self._send({"type": "error", + "detail": "Not a directory in this group", + "code": "no_such_directory", + "filename": filename}) + return + rel_dir = target_rel else: - rel_dir = f"{upload_root.name}/{UPLOAD_DIR_NAME}" - target_dir = upload_root.path / UPLOAD_DIR_NAME - try: - target_dir.mkdir(parents=True, exist_ok=True) - except OSError as e: - log.warning("Cannot create upload folder in root %r: %s", - upload_root.name, e) - self._send({"type": "error", "detail": "Upload folder unavailable", + # An MNP 1.0 client names nothing; the root itself is where its one + # destination now is. + target_dir = upload_root.path + rel_dir = upload_root.name + if not target_dir.is_dir(): + self._send({"type": "error", + "detail": f"Directory '{upload_root.name}' is " + f"currently unavailable", + "code": "root_unavailable", "filename": filename}) return diff --git a/packages/meshbay-node/tests/test_root_writable_policy.py b/packages/meshbay-node/tests/test_root_writable_policy.py index da95032..d7f2666 100644 --- a/packages/meshbay-node/tests/test_root_writable_policy.py +++ b/packages/meshbay-node/tests/test_root_writable_policy.py @@ -62,14 +62,16 @@ def _session(tmp_path: Path, user_id: str, *, def _upload(session, filename="clip.mp4", body=b"bytes"): session._do_file_upload({ - "filename": filename, "root": "shared", + "filename": filename, "dir": "shared", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(body).decode(), }) def _uploads_dir(session) -> Path: - return session._ctx["roots"].roots[0].path / "uploads" + # The root itself: the `uploads/` subdirectory the node used to create is + # gone (see test_security_regressions._uploads_dir for why). + return session._ctx["roots"].roots[0].path # ── The door, not the button ───────────────────────────────────────────────── @@ -80,7 +82,7 @@ async def test_a_member_cannot_upload_to_a_read_only_root(tmp_path): refusal = [m for m in session.sent if m.get("type") == "error"] assert refusal and refusal[0].get("code") == "root_read_only" - assert not _uploads_dir(session).exists() + assert not (_uploads_dir(session) / "clip.mp4").exists() async def test_members_upload_normally_to_a_writable_root(tmp_path): diff --git a/packages/meshbay-node/tests/test_security_regressions.py b/packages/meshbay-node/tests/test_security_regressions.py index 9db8ac1..1a318f7 100644 --- a/packages/meshbay-node/tests/test_security_regressions.py +++ b/packages/meshbay-node/tests/test_security_regressions.py @@ -133,14 +133,22 @@ def test_the_node_never_generates_a_name_it_would_refuse(tmp_path): def _uploads_dir(session) -> Path: """ - Where this session's uploads land: uploads/ inside its first writable root. + Where an unaddressed upload lands: the first writable root itself. + + There is no `uploads/` subdirectory any more. It was the last of v5's + quarantine — the per-user layer went on 2026-08-14 — and it went for the + same reason: a folder appearing beside the operator's library because + somebody sent a file is the node deciding how their disk is arranged. The + protections that made the quarantine worth having are the allowlist, the + size cap, the chunk ordering and the no-overwrite rule, and every one of + them is asserted below, unchanged. Asked of the root set rather than assembled by hand, so a test cannot pass while agreeing with a wrong answer the code also produced. """ writable = session._ctx["roots"].writable_roots assert writable, "the fixture must give the group a writable root" - return writable[0].path / "uploads" + return writable[0].path def _session(tmp_path: Path, user_id: str) -> WebRTCPeerSession: @@ -176,7 +184,6 @@ def test_upload_cannot_overwrite_another_members_file(tmp_path): """ victim = _session(tmp_path, "victim-user") uploads = _uploads_dir(victim) - uploads.mkdir() original = uploads / "important.mp4" original.write_bytes(b"operator's original content") @@ -226,50 +233,57 @@ def test_dir_create_cannot_escape_the_shared_root(tmp_path, bad): assert set(tmp_path.rglob("*")) == before, f"created something via {bad!r}" -def test_upload_ignores_any_directory_the_client_asks_for(tmp_path): +def test_the_client_names_a_folder_and_never_a_filesystem_path(tmp_path): """ - The destination inside a root is the node's decision, and stays so. + The destination is now the folder the sender is looking at, which means the + client does choose it — and the whole of what keeps that safe is that the + choice is *resolved against the group's own roots* rather than joined to + one. - A client now names the *root* it is uploading into — it has to, once a group - can have several writable ones — but that is a name looked up in the root - table, never a path. Everything below the root is still chosen here, so the - traversal surface a client-chosen destination would open does not exist. + `RootSet.resolve()` refuses `..`, absolute segments and anything whose + resolved form escapes its root, symlinks included. So "which of this + group's folders" is answerable by a member and "which path on the + operator's disk" is not. """ session = _session(tmp_path, "user-1") + (session._ctx["roots"].roots[0].path / "sub").mkdir() + before = set(tmp_path.rglob("*")) - session._do_file_upload({ - "filename": "note.txt", "dir": "../../etc", "path": "/etc", - "chunk_index": 0, "total_chunks": 1, - "data": base64.b64encode(b"x").decode(), - }) + for bad in ("../../etc", "/etc", "shared/../..", "shared/../../etc", + "nope", "shared/missing"): + session.sent.clear() + session._do_file_upload({ + "filename": "note.txt", "dir": bad, + "chunk_index": 0, "total_chunks": 1, + "data": base64.b64encode(b"x").decode(), + }) + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal, f"{bad!r} was accepted" + assert refusal[0].get("code") in ("no_such_root", "no_such_directory"), bad - assert (_uploads_dir(session) / "note.txt").read_bytes() == b"x" - assert not (tmp_path / "etc").exists() + assert set(tmp_path.rglob("*")) == before, "a refused upload still wrote" -@pytest.mark.parametrize("named_root", [ - "../../etc", "/etc", "shared/../..", "Shared/uploads", "nope", -]) -def test_a_root_name_is_looked_up_never_joined(tmp_path, named_root): +def test_an_upload_lands_in_the_folder_it_names(tmp_path): """ - The name the client sends is matched against the group's root table and - refused when it matches nothing. A version that joined it to a path — or - that quietly fell back to the first writable root — would turn "which - directory" into either a traversal or a file on a disk the operator did - not intend, and the second is discovered weeks later. + And in that folder itself — the `uploads/` subdirectory the node used to + create is gone. Somebody dropping a file into the folder they are looking + at expects it to be in that folder. """ session = _session(tmp_path, "user-1") - before = set(tmp_path.rglob("*")) + root = session._ctx["roots"].roots[0] + (root.path / "Albums").mkdir() session._do_file_upload({ - "filename": "note.txt", "root": named_root, + "filename": "note.txt", "dir": f"{root.name}/Albums", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) - refusal = [m for m in session.sent if m.get("type") == "error"] - assert refusal and refusal[0].get("code") == "no_such_root", named_root - assert set(tmp_path.rglob("*")) == before, f"wrote something via {named_root!r}" + assert (root.path / "Albums" / "note.txt").read_bytes() == b"x" + assert not (root.path / "Albums" / "uploads").exists(), ( + "the node invented a subdirectory in the operator's library") + assert not (root.path / "uploads").exists() def test_an_upload_goes_to_the_root_it_names(tmp_path): @@ -291,13 +305,13 @@ def test_an_upload_goes_to_the_root_it_names(tmp_path): ]) session._do_file_upload({ - "filename": "note.txt", "root": "Incoming", + "filename": "note.txt", "dir": "Incoming", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) - assert (incoming / "uploads" / "note.txt").read_bytes() == b"x" - assert not (media / "uploads").exists(), "it went to the first root instead" + assert (incoming / "note.txt").read_bytes() == b"x" + assert not (media / "note.txt").exists(), "it went to the first root instead" def test_a_read_only_root_refuses_an_upload(tmp_path): @@ -314,14 +328,14 @@ def test_a_read_only_root_refuses_an_upload(tmp_path): session._is_node_admin = lambda: True session._do_file_upload({ - "filename": "note.txt", "root": "Published", + "filename": "note.txt", "dir": "Published", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) refusal = [m for m in session.sent if m.get("type") == "error"] assert refusal and refusal[0].get("code") == "root_read_only" - assert not (published / "uploads").exists() + assert not (published / "note.txt").exists() def test_a_fully_read_only_group_refuses_an_unaddressed_upload(tmp_path): @@ -343,7 +357,7 @@ def test_a_fully_read_only_group_refuses_an_unaddressed_upload(tmp_path): refusal = [m for m in session.sent if m.get("type") == "error"] assert refusal and refusal[0].get("code") == "no_writable_root" - assert not (published / "uploads").exists() + assert not (published / "note.txt").exists() def test_an_ejected_root_refuses_an_upload(tmp_path): @@ -362,14 +376,14 @@ def test_an_ejected_root_refuses_an_upload(tmp_path): session._ctx["roots"] = roots session._do_file_upload({ - "filename": "note.txt", "root": "USB", + "filename": "note.txt", "dir": "USB", "chunk_index": 0, "total_chunks": 1, "data": base64.b64encode(b"x").decode(), }) refusal = [m for m in session.sent if m.get("type") == "error"] assert refusal and refusal[0].get("code") == "root_unavailable" - assert not (usb / "uploads").exists() + assert not (usb / "note.txt").exists() def test_two_members_can_send_the_same_filename(tmp_path): -- cgit v1.2.3 From d6e1cc19a09df35988f6a90c226c94f2fdf6b209 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 22:20:46 +0200 Subject: fix(node): `root list` printed "?" for every path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `RootSet.describe()` feeds two audiences that want opposite things. The index payload goes to every member and has always deliberately carried no paths — a member is told what exists and whether it is readable, not that the library sits under someone's home directory. The loopback API answers the operator themselves, over a channel that already requires their machine and the run token, and the path is exactly what they asked for. The `root list` CLI I added in Phase 1 read `path` from the member form, so it printed a placeholder for every directory. Nothing caught it: the CLI reads a dict, the API returns a dict, and neither end states which keys it owes. `describe(with_paths=True)` is the operator's view and `list_groups` is the only caller. The shared-directories table has the same hole over MNP — the roots there come from the index payload — so its Path column now appears only when a path is actually present, rather than rendering a column of blanks. The test asserts both halves, because they pull opposite ways: one that only checked the operator sees paths would be satisfied by leaking them to every member. It reads the indexer's source for the member side, and compares the CLI's key reads against what the payload offers for the other — checked to fail in each direction independently. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../src/meshbay_hub/static/group-settings.js | 16 ++- packages/meshbay-node/src/meshbay_node/ops.py | 6 +- packages/meshbay-node/src/meshbay_node/roots.py | 18 +++- .../tests/test_root_paths_are_operator_only.py | 115 +++++++++++++++++++++ 4 files changed, 147 insertions(+), 8 deletions(-) create mode 100644 packages/meshbay-node/tests/test_root_paths_are_operator_only.py (limited to 'packages/meshbay-node/tests') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js b/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js index f8b4aa5..ce36a40 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js @@ -88,6 +88,14 @@ function SharedDirectoriesTable({ roots, groupId, transport, signFn, const displayRoots = serverRoots.map(r => optimistic[r.name] ? { ...r, ...optimistic[r.name] } : r); + // Paths are the operator's own view and do not cross MNP: the roots table + // rides in the index payload, which every member receives, and it tells them + // what exists and whether it is readable — never where on the operator's + // disk it lives. So the column appears when the answer is actually + // available (the loopback API, which is already this machine only) and is + // left out otherwise, rather than printing a row of blanks. + const hasPaths = displayRoots.some((r) => r.path); + // Which door a change goes through. MNP first: it is the only one that // exists for an operator on the web, and it is signed, which the loopback // API is not (it is authorized by being on localhost with the run token). @@ -261,7 +269,7 @@ function SharedDirectoriesTable({ roots, groupId, transport, signFn, ${t('node.directory')} - ${t('node.root_path')} + ${hasPaths && html`${t('node.root_path')}`} ${canEdit && html`${t('node.root_rw')}`} ${canEdit && !isLocal && html`${t('node.removable')}`} @@ -283,10 +291,8 @@ function SharedDirectoriesTable({ roots, groupId, transport, signFn, ${!isLocal && r.available === false && !r.ejected && html` ${t('node.unavailable')}`} - ${/* Two roots can never share a name, so the name is the identity — - but it is the *basename*, and two libraries under different - parents look identical without this. */''} - ${r.path || ''} + ${hasPaths && html` + ${r.path || ''}`} ${canEdit && html` <${ToggleSwitch} checked=${!!r.writable} disabled=${busy || !!r.ejected} diff --git a/packages/meshbay-node/src/meshbay_node/ops.py b/packages/meshbay-node/src/meshbay_node/ops.py index 3dfdc23..4c759c8 100644 --- a/packages/meshbay-node/src/meshbay_node/ops.py +++ b/packages/meshbay-node/src/meshbay_node/ops.py @@ -362,7 +362,11 @@ async def list_groups(state: dict) -> dict: "has_gek": bool(ctx.get("gek")), "file_count": idx.count if idx else 0, "index_version": idx.version if idx else 0, - "roots": roots.describe() if roots else [], + # With paths: this answers the loopback API, which is the + # operator's own channel. `meshbay-node root list` printed "?" for + # every directory without it — it was reading a field the member + # form of this deliberately omits. + "roots": roots.describe(with_paths=True) if roots else [], "peers": sum(1 for p in peers.values() if p.get("group_id") == gid), }) roster = state.get("roster") diff --git a/packages/meshbay-node/src/meshbay_node/roots.py b/packages/meshbay-node/src/meshbay_node/roots.py index ffe801c..d288231 100644 --- a/packages/meshbay-node/src/meshbay_node/roots.py +++ b/packages/meshbay-node/src/meshbay_node/roots.py @@ -357,8 +357,14 @@ class RootSet: "available" if live else "UNAVAILABLE", root.path) return changed - def describe(self) -> list[dict]: - """Per-root state for the index payload and the admin UI.""" + def describe(self, *, with_paths: bool = False) -> list[dict]: + """ + Per-root state for the index payload and the admin UI. + + Deliberately no paths by default: this is what every member receives. + `with_paths=True` is the operator's own view, over a channel that is + already theirs alone (loopback + run token). + """ out = [] for r in self.roots: d: dict = {"name": r.name, "kind": r.kind, @@ -368,6 +374,14 @@ class RootSet: "ejected": r.ejected, # Backward compat for MNP 1.0 clients "upload": r.writable} + # `with_paths` is for the operator's *own* channels only — the + # loopback API and the CLI reading it, both of which already + # require being on this machine with the run token. A member is + # told what exists and whether it is readable, never where on the + # operator's disk it lives, and the index payload every member + # receives must keep calling this without the flag. + if with_paths: + d["path"] = str(r.path) out.append(d) return out diff --git a/packages/meshbay-node/tests/test_root_paths_are_operator_only.py b/packages/meshbay-node/tests/test_root_paths_are_operator_only.py new file mode 100644 index 0000000..6ba2e08 --- /dev/null +++ b/packages/meshbay-node/tests/test_root_paths_are_operator_only.py @@ -0,0 +1,115 @@ +""" +Where a directory lives on the operator's disk is theirs, not the group's. + +`RootSet.describe()` feeds two very different audiences. The index payload goes +to every member, and has always deliberately carried no paths — a member is +told what exists and whether it is readable, not that the library sits in +`/media//BACKUP2`. The loopback API answers the operator +themselves, over a channel that already requires being on their machine with +the run token, where the path is exactly what they are asking for. + +`meshbay-node root list` printed `?` for every directory because it read a +field the member form omits. Nothing caught it: the CLI reads a dict, the +payload is a dict, and neither end says what keys it owes the other. + +Both halves matter and they pull opposite ways, so both are asserted here — a +test that only checked the operator gets paths would be satisfied by putting +them in the member payload too. +""" + +import inspect +import re +from pathlib import Path + +import pytest + +from meshbay_node import daemon as daemon_mod +from meshbay_node import ops +from meshbay_node.roots import RootSet + + +def _roots(tmp_path: Path) -> RootSet: + for name in ("Films", "Albums"): + (tmp_path / name).mkdir() + return RootSet.build([ + {"path": str(tmp_path / "Films"), "writable": True}, + {"path": str(tmp_path / "Albums"), "removable": True}, + ]) + + +# ── The member's half ──────────────────────────────────────────────────────── + +def test_the_default_form_carries_no_path(tmp_path): + described = _roots(tmp_path).describe() + assert described, "no roots described" + assert not any("path" in d for d in described), ( + "the index payload every member receives would carry the operator's " + "filesystem layout") + + +def test_the_default_form_still_says_what_a_member_needs(tmp_path): + """The counter-property: dropping the path must not drop the rest.""" + described = _roots(tmp_path).describe() + for d in described: + assert set(d) >= {"name", "kind", "available", "writable", + "removable", "ejected"} + + +def test_the_index_payload_is_built_without_paths(): + """ + Read from the source, because the alternative is asserting it about a + payload built by a test rather than by the node. + """ + from meshbay_node.indexer import indexer as indexer_mod + source = inspect.getsource(indexer_mod) + for call in re.findall(r"roots\.describe\([^)]*\)", source): + assert "with_paths" not in call, ( + f"the indexer builds the member-facing roots table as {call} — " + f"that payload goes to everyone in the group") + + +# ── The operator's half ────────────────────────────────────────────────────── + +def test_the_operator_form_carries_the_path(tmp_path): + described = _roots(tmp_path).describe(with_paths=True) + assert all(d.get("path") for d in described) + assert described[0]["path"] == str(tmp_path / "Films") + + +def test_the_loopback_api_asks_for_paths(): + """ + `list_groups` answers the operator's own channel, and the CLI's `root list` + prints what it returns. Asking for the member form there is what printed a + column of question marks. + """ + source = inspect.getsource(ops.list_groups) + assert "describe(with_paths=True)" in source, ( + "list_groups uses the member form, so every path it reports is missing") + + +def test_the_cli_only_reads_fields_the_payload_carries(): + """ + The gap this whole file exists for. The CLI reads a dict and the API + returns a dict; nothing between them says which keys are owed, so a name + that is simply absent prints as a placeholder and looks like a node + problem. + """ + source = inspect.getsource(daemon_mod.main) + start = source.index('if args.command == "root":') + block = source[start:source.index('if args.command == "operator":', start)] + + read = set(re.findall(r"r\.get\(['\"](\w+)['\"]", block)) + read |= set(re.findall(r"r\[['\"](\w+)['\"]\]", block)) + assert read, "the root CLI no longer reads the payload this way" + + class _Any: + path = Path("/tmp/x") + name = "x" + kind = "generic" + writable = removable = ejected = False + available = True + + offered = set(RootSet(roots=[_Any()]).describe(with_paths=True)[0]) + assert read <= offered, ( + f"the `root` CLI reads keys the loopback payload does not carry: " + f"{sorted(read - offered)}") -- cgit v1.2.3 From 13ccc4a16c604f46ed10b6c5dbfbc8fdd7d008c0 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 23:01:36 +0200 Subject: fix: a root change reaches every client without a page reload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The directory table travelled on `index_sync` alone — a *full* index, which the node only ever sends on request. Every ongoing change went out as an `index_delta`, which carried files and nothing else. So the message that says "something changed" was the one message that could not say a root had. The acks hid it: `root_add_ack` and friends broadcast the new table to whoever is connected, so the common cases looked right. What that could not cover was the operator's own client, where the ack landed and was then overwritten — the table calls `onRefreshIndex` after an add, that fetch returns the set from *before* the node's reload (fire-and-forget, because a rescan is minutes on a real library), and `applyIndex` writes it over what the ack had just delivered. The new directory appeared for one paint and vanished. Two halves. `index_delta` now carries the roots table, sealed with the rest and identical to `index_sync`'s — additive, so a 1.0 client sees a field it does not read. And the table no longer refreshes the index after a root change: the ack gives it the new set immediately, and the delta the node pushes when the scan finishes gives it again, along with the files. The test that pins it uses an *eject* as its case, because an eject changes no file at all — the entries freeze — so its delta is empty of additions, deletions and updates. Without the table it says literally nothing, which is how a library disappearing from under a group went unannounced to everyone but whoever pressed the button. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../src/meshbay_hub/static/group-page.js | 7 ++ .../src/meshbay_hub/static/group-settings.js | 24 +++- packages/meshbay-node/src/meshbay_node/daemon.py | 3 +- .../src/meshbay_node/transport/wire.py | 12 +- .../tests/test_index_delta_carries_roots.py | 131 +++++++++++++++++++++ .../tests/test_root_paths_are_operator_only.py | 1 - 6 files changed, 169 insertions(+), 9 deletions(-) create mode 100644 packages/meshbay-node/tests/test_index_delta_carries_roots.py (limited to 'packages/meshbay-node/tests') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/group-page.js b/packages/meshbay-hub/src/meshbay_hub/static/group-page.js index a1e6411..0a43724 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/group-page.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/group-page.js @@ -244,6 +244,13 @@ function GroupPage({ groupId, group, token, username, userId, userPrefs, // enrichment (duration/thumb_hash/display_title/...) arriving for a file // already in the table — same id, new fields (see group_index.py diff()). const applyIndexDelta = useCallback((deltaMsg) => { + // The roots table rides on the delta as of MNP 1.1. Before that it + // travelled only on a full index_sync, which is sent on request — so a + // root added, removed, ejected or plugged by anyone left every other + // client's directory table stale until they reloaded the page. + if (Array.isArray(deltaMsg.roots) && deltaMsg.roots.length) { + setNodeRoots(deltaMsg.roots); + } setEntries((prev) => { const deletions = new Set(deltaMsg.deletions || []); const kept = prev.filter((e) => !deletions.has(e.id)); diff --git a/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js b/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js index ce36a40..9646405 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/group-settings.js @@ -42,7 +42,8 @@ import * as platform from './platform.js'; * nodeDetected — whether the loopback node API answers * readOnly — suppress every edit control * onRootsChange — called after a change, to re-read the loopback list - * onRefreshIndex — full index refresh, needed after an add or a remove + * onRefreshIndex — full index refresh. Not called after a root change: see + * `run()` for why the node's own push is what settles it * mode — "live" (default) or "local" * localRoots / onLocalRootsChange — the array, in "local" mode */ @@ -106,18 +107,29 @@ function SharedDirectoriesTable({ roots, groupId, transport, signFn, const rootUrl = (name, suffix = '') => '/api/groups/' + groupId + '/roots/' + encodeURIComponent(name) + suffix; - const run = useCallback(async (work, { refreshIndex = false } = {}) => { + // Deliberately no index refresh after a root change. + // + // Adding a root makes the node reload, which rescans — minutes on a real + // library — and the reload is fire-and-forget for that reason. Fetching the + // index in the moment after therefore returns the set from *before* it, and + // `applyIndex` writes that over the roots the ack had just delivered: the + // new directory appeared for one paint and vanished, which is what "it only + // shows up after a refresh" was. + // + // Nothing is lost by waiting. The ack carries the new table immediately, and + // the delta the node pushes when the scan finishes carries it again along + // with the files. + const run = useCallback(async (work) => { setBusy(true); setMsg(''); try { await work(); if (onRootsChange) await onRootsChange(); - if (refreshIndex && onRefreshIndex) await onRefreshIndex(); return true; } catch (err) { setMsg(platform.bridgeMessage(err)); return false; } finally { setBusy(false); } - }, [onRootsChange, onRefreshIndex]); + }, [onRootsChange]); const doUpdateRoot = useCallback(async (rootName, updates) => { if (isLocal) { @@ -171,7 +183,7 @@ function SharedDirectoriesTable({ roots, groupId, transport, signFn, await platform.node.call('DELETE', rootUrl(rootName)); await platform.node.call('POST', '/api/reload'); } else throw new Error(t('node.root_no_route')); - }, { refreshIndex: true }); + }); if (ok) setMsg(t('node.root_removed')); }, [isLocal, localRoots, onLocalRootsChange, overMnp, overLoopback, transport, groupId, signFn, run]); @@ -203,7 +215,7 @@ function SharedDirectoriesTable({ roots, groupId, transport, signFn, await platform.node.call('POST', '/api/reload'); await platform.watchIndexProgress(groupId, setIndexProgress); } else throw new Error(t('node.root_no_route')); - }, { refreshIndex: true }); + }); }, [isLocal, localRoots, onLocalRootsChange, overMnp, overLoopback, transport, groupId, signFn, run]); diff --git a/packages/meshbay-node/src/meshbay_node/daemon.py b/packages/meshbay-node/src/meshbay_node/daemon.py index 7637b41..028918d 100644 --- a/packages/meshbay-node/src/meshbay_node/daemon.py +++ b/packages/meshbay-node/src/meshbay_node/daemon.py @@ -1240,7 +1240,8 @@ class NodeDaemon: # node refuses every handshake while the GEK is None (NS8) — so this is # "nobody is listening", not a case to send in clear for. if peers and idx.gek: - msg = (index_delta_message(idx, delta) if delta is not None + msg = (index_delta_message(idx, delta, indexer.roots) + if delta is not None else index_sync_message(idx, indexer.roots)) pushed = 0 for session in peers: diff --git a/packages/meshbay-node/src/meshbay_node/transport/wire.py b/packages/meshbay-node/src/meshbay_node/transport/wire.py index c683204..6986b01 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/wire.py +++ b/packages/meshbay-node/src/meshbay_node/transport/wire.py @@ -89,13 +89,21 @@ def index_sync_message(index, roots: RootSet | None) -> dict: } -def index_delta_message(index, delta) -> dict: +def index_delta_message(index, delta, roots=None) -> dict: """ One `index_delta` — what changed since the last thing this node broadcast. Built here rather than inline in the daemon, which is where it lived and which made it the third place an index message was constructed: precisely the drift that produced two `index_sync` encodings and two `file_chunk` encodings before it. + + `roots` rides along (MNP 1.1, additive — a 1.0 client ignores it). It used + to travel on `index_sync` alone, which is a *full* index and therefore only + ever sent on request. So a root added, removed, ejected or plugged left + every connected client's directory table stale until somebody reloaded the + page: the delta that told them something had changed was the one message + that could not say what. It is a handful of dicts, bounded by the number of + directories a group has, and it is sealed with the rest. """ payload = { "base_version": delta.base_version, @@ -104,6 +112,8 @@ def index_delta_message(index, delta) -> dict: "deletions": list(delta.deletions), "updates": [index_entry_wire(e) for e in delta.updates], } + if roots is not None: + payload["roots"] = roots.describe() return { "type": MNP.INDEX_DELTA, "v": MNP_VERSION, diff --git a/packages/meshbay-node/tests/test_index_delta_carries_roots.py b/packages/meshbay-node/tests/test_index_delta_carries_roots.py new file mode 100644 index 0000000..227f2d0 --- /dev/null +++ b/packages/meshbay-node/tests/test_index_delta_carries_roots.py @@ -0,0 +1,131 @@ +""" +The message that says something changed has to be able to say what. + +A group's directory table travelled on `index_sync` alone — a *full* index, +which the node only ever sends on request. Every ongoing change went out as an +`index_delta`, which carried files and nothing else. So a root added, removed, +ejected or plugged by the operator reached every other client's screen only +when somebody happened to reload the page. + +It was hidden by the acks: `root_add_ack` and friends broadcast the new table +to whoever was connected, so the common cases looked fine. What that could not +cover is a client connecting mid-change, one whose ack was lost, or — the one +that surfaced it — the operator's own client, where the ack landed and was then +overwritten by an index fetched before the node had rebuilt anything. + +Additive on the wire (MNP 1.1): a 1.0 client sees a field it does not read. +""" + +from pathlib import Path + +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey + +from meshbay_common.crypto import generate_gek +from meshbay_common.groupbox import PURPOSE_INDEX, unseal +from meshbay_node.indexer.group_index import GroupIndex +from meshbay_node.roots import RootSet +from meshbay_node.transport.wire import index_delta_message, index_sync_message + + +GROUP = "g" * 32 + + +def _roots(tmp_path: Path) -> RootSet: + for name in ("Films", "Albums"): + (tmp_path / name).mkdir() + return RootSet.build([ + {"path": str(tmp_path / "Films"), "writable": True}, + {"path": str(tmp_path / "Albums"), "removable": True}, + ]) + + +def _index() -> GroupIndex: + return GroupIndex(group_id=GROUP, sk_node=Ed25519PrivateKey.generate(), + gek=generate_gek()) + + +def _payload(msg: dict, index: GroupIndex) -> dict: + """What a member actually reads, through the seal rather than around it.""" + return unseal(index.gek, PURPOSE_INDEX, msg["type"], GROUP, msg) + + +class _Delta: + base_version = 1 + version = 2 + additions: list = [] + deletions: list = [] + updates: list = [] + + +def test_a_delta_carries_the_directory_table(tmp_path): + index = _index() + msg = index_delta_message(index, _Delta(), _roots(tmp_path)) + payload = _payload(msg, index) + + assert [r["name"] for r in payload["roots"]] == ["Films", "Albums"] + assert payload["roots"][0]["writable"] is True + assert payload["roots"][1]["removable"] is True + + +def test_the_table_is_sealed_with_the_rest(tmp_path): + """ + It is group content, not routing. Only `type`, `v` and `group_id` stay in + clear, because a receiver has to route and authenticate before it would + trust a decryption. + """ + index = _index() + msg = index_delta_message(index, _Delta(), _roots(tmp_path)) + assert set(msg) - {"type", "v", "group_id"}, "nothing was sealed" + assert "roots" not in msg, "the directory table is outside the envelope" + + +def test_a_delta_still_works_without_a_table(tmp_path): + """ + The argument is optional, so an older caller — or a path that has no root + set to hand — produces a message a client reads exactly as before. + """ + index = _index() + payload = _payload(index_delta_message(index, _Delta()), index) + assert "roots" not in payload + assert payload["version"] == 2 + + +def test_the_table_says_the_same_thing_on_both_messages(tmp_path): + """ + Two encodings of one idea is the drift `wire.py` exists to prevent — it + already happened twice, for `index_sync` and for `file_chunk`. + """ + index = _index() + roots = _roots(tmp_path) + delta = _payload(index_delta_message(index, _Delta(), roots), index) + sync = _payload(index_sync_message(index, roots), index) + assert delta["roots"] == sync["roots"] + + +def test_the_table_never_carries_a_path(tmp_path): + """ + This message goes to every member. Where a directory lives on the + operator's disk is theirs — see test_root_paths_are_operator_only.py. + """ + index = _index() + payload = _payload(index_delta_message(index, _Delta(), _roots(tmp_path)), + index) + assert not any("path" in r for r in payload["roots"]) + + +def test_an_ejected_root_is_visible_in_the_delta(tmp_path): + """ + The case this was written for. An eject changes no file — the entries + freeze — so the delta it produces is empty of additions, deletions and + updates. Without the table it says literally nothing, which is how a + library disappearing from under the group's feet went unannounced. + """ + index = _index() + roots = _roots(tmp_path) + roots.roots[1].ejected = True + roots.roots[1].available = False + + payload = _payload(index_delta_message(index, _Delta(), roots), index) + assert payload["additions"] == [] and payload["deletions"] == [] + assert payload["roots"][1]["ejected"] is True + assert payload["roots"][1]["available"] is False diff --git a/packages/meshbay-node/tests/test_root_paths_are_operator_only.py b/packages/meshbay-node/tests/test_root_paths_are_operator_only.py index 6ba2e08..080d4be 100644 --- a/packages/meshbay-node/tests/test_root_paths_are_operator_only.py +++ b/packages/meshbay-node/tests/test_root_paths_are_operator_only.py @@ -21,7 +21,6 @@ import inspect import re from pathlib import Path -import pytest from meshbay_node import daemon as daemon_mod from meshbay_node import ops -- cgit v1.2.3 From 920284009d634cb568f95b3e93b93012c4b803bb Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Sun, 6 Sep 2026 23:42:59 +0200 Subject: feat(client): bring back New folder, icon-only — and close the hole it opened MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The control was hidden and its `canCreateDir` left computed and unused. It is back in the Files toolbar as an icon: the toolbar already carries one labelled primary action, and a second beside it competes for the width the breadcrumb trail needs. The name is in `title` *and* `aria-label` — a title is invisible to a screen reader on a button with no text, so an icon-only control without both is simply unnamed for anyone not reading with their eyes. Its gate changes. It required `isNodeAdmin`, which contradicted the node's own rule — "making a directory is not a privileged act; a member who can add a file can organise where it goes" — and hid the control from everyone who could have used it. It now follows the Upload button: a writable root, and not at the top of a group, where the level is the set of roots rather than a directory on anyone's disk. Restoring it surfaced a real gap. `_do_dir_create` never learned about RO/RW: `_do_file_upload` gained the `writable` check with the model and this one did not, so a member refused a file in a published library could still leave empty directories all through it, and could write to a drive mid-eject. Read-only has to mean read-only for every way of writing, not just for files. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../src/meshbay_hub/static/files-app.js | 19 +++++++++- .../meshbay-hub/src/meshbay_hub/static/style.css | 4 +++ .../tests/test_upload_controls_hidden.py | 30 ++++++++++++++++ .../src/meshbay_node/transport/webrtc_server.py | 22 ++++++++++++ .../tests/test_root_writable_policy.py | 41 ++++++++++++++++++++++ 5 files changed, 115 insertions(+), 1 deletion(-) (limited to 'packages/meshbay-node/tests') diff --git a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js index 9ca3aae..fb57a0f 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/files-app.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/files-app.js @@ -234,7 +234,13 @@ function FilesPanel({ // set of roots, which is the operator's configuration and not a directory on // anyone's disk. The node refuses it, so offering it would only produce an // error nobody can act on. - const canCreateDir = Boolean(currentPath) && isNodeAdmin; + // + // Otherwise the rule is the same as the Upload button's, and for the same + // reason the node gives: "making a directory is not a privileged act — a + // member who can add a file can organise where it goes". It used to require + // `isNodeAdmin`, which contradicted the node and hid the control from + // everyone who could actually use it. + const canCreateDir = Boolean(currentPath) && currentRootWritable && !readOnly; const breadcrumbs = currentPath ? currentPath.split('/') : []; @@ -363,6 +369,17 @@ function FilesPanel({ onChange=${uploadFile} /> `} + ${/* Icon only: the toolbar already carries a labelled primary + action, and a second one beside it competes with it for the + width a breadcrumb trail needs. The name lives in the tooltip + and in aria-label, so it is not lost to anyone reading with + something other than their eyes. */''} + ${canCreateDir && html` + + `}
diff --git a/packages/meshbay-hub/src/meshbay_hub/static/style.css b/packages/meshbay-hub/src/meshbay_hub/static/style.css index 0441e14..4647209 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/style.css +++ b/packages/meshbay-hub/src/meshbay_hub/static/style.css @@ -833,6 +833,10 @@ button:disabled { opacity: 0.5; cursor: not-allowed; } .tb-btn:disabled { opacity: 0.45; cursor: not-allowed; } .tb-btn:disabled:hover { border-color: var(--border); color: var(--text); } .tb-btn .icon { width: 15px; height: 15px; } +/* Square, for a toolbar button whose name is in its tooltip rather than + beside it. The horizontal padding goes; the height does not, so it lines up + with the labelled buttons next to it. */ +.tb-btn-icon { padding: 0; width: 32px; justify-content: center; gap: 0; } .tb-btn.primary { background: var(--accent); border-color: var(--accent); diff --git a/packages/meshbay-hub/tests/test_upload_controls_hidden.py b/packages/meshbay-hub/tests/test_upload_controls_hidden.py index 7b8b0e3..a61de1f 100644 --- a/packages/meshbay-hub/tests/test_upload_controls_hidden.py +++ b/packages/meshbay-hub/tests/test_upload_controls_hidden.py @@ -73,6 +73,36 @@ def test_the_files_upload_button_is_not_offered_at_the_top_of_a_group(): assert "currentPath &&" in toolbar +def test_the_new_folder_button_follows_the_same_rule_as_upload(): + """ + Both write to the operator's disk, so both need a writable root — the node + refuses either otherwise. It used to require `isNodeAdmin`, which + contradicted the node ("a member who can add a file can organise where it + goes") and hid the control from everyone who could have used it. + """ + page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") + decl = page[page.index("const canCreateDir"):] + decl = decl[:decl.index(";") + 1] + assert "currentRootWritable" in decl + assert "currentPath" in decl, ( + "the top of a group is the set of roots, not a directory to create in") + assert "isNodeAdmin" not in decl + + +def test_an_icon_only_button_still_says_what_it_is(): + """ + The name moved into a tooltip to save toolbar width. A `title` is invisible + to a screen reader on a button with no text, so the label has to be there + as well — otherwise the control is simply unnamed for anyone not reading + with their eyes. + """ + page = _component(FILES_APP.read_text(encoding="utf-8"), "FilesPanel") + block = page[page.index("canCreateDir && html`"):] + block = block[:block.index("")] + assert "title=" in block and "aria-label=" in block + assert "group.mkdir" in block + + def test_the_chat_composer_hides_its_paperclip(): chat = _component(CHAT_APP.read_text(encoding="utf-8"), "ChatPanel") composer = chat[chat.index("chat-input-row"):] 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 61458f2..0e2d418 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -1595,6 +1595,28 @@ class WebRTCPeerSession: "detail": "Choose a folder to create this in"}) return + # Read-only means read-only, and creating a folder writes to the + # operator's disk. `_do_file_upload` gained this check with the RO/RW + # model and this one did not — so a member could not add a file to a + # published library but could still leave empty directories in it. + owner = roots.split(parent_rel) + if owner is None: + self._send({"type": "error", "detail": "Invalid directory"}) + return + parent_root, _tail = owner + if not parent_root.writable: + self._send({"type": "error", + "detail": f"Directory '{parent_root.name}' is read-only", + "code": "root_read_only"}) + self._audit("dir_create_refused", parent_rel[:64]) + return + if not parent_root.available: + self._send({"type": "error", + "detail": f"Directory '{parent_root.name}' is " + f"currently unavailable", + "code": "root_unavailable"}) + return + parent = safe_subdir(roots, parent_rel) if parent is None or not parent.is_dir(): self._send({"type": "error", "detail": "Invalid directory"}) diff --git a/packages/meshbay-node/tests/test_root_writable_policy.py b/packages/meshbay-node/tests/test_root_writable_policy.py index d7f2666..7eb75fd 100644 --- a/packages/meshbay-node/tests/test_root_writable_policy.py +++ b/packages/meshbay-node/tests/test_root_writable_policy.py @@ -108,6 +108,47 @@ async def test_read_only_binds_the_operator_too(tmp_path): assert refusal and refusal[0].get("code") == "root_read_only" +async def test_a_member_cannot_create_a_folder_in_a_read_only_root(tmp_path): + """ + Read-only has to mean read-only for every way of writing, not just for + files. `_do_file_upload` gained this check with the RO/RW model and + `_do_dir_create` did not, so a member refused a file in a published library + could still leave empty directories all through it. + + Creating a folder stays unprivileged — the node's own words: "a member who + can add a file can organise where it goes". What changed is that it now + requires the same root to be writable that adding the file would have. + """ + session = _session(tmp_path, "member-1", writable=False) + session._do_dir_create({"dir": "shared", "name": "New folder"}) + + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_read_only" + assert not (tmp_path / "shared" / "New folder").exists() + + +async def test_a_member_can_create_a_folder_in_a_writable_root(tmp_path): + """The counter-property: it must stay unprivileged where it is allowed.""" + session = _session(tmp_path, "member-1", writable=True) + session._do_dir_create({"dir": "shared", "name": "New folder"}) + + assert not [m for m in session.sent if m.get("type") == "error"] + assert (tmp_path / "shared" / "New folder").is_dir() + + +async def test_an_ejected_root_refuses_a_new_folder(tmp_path): + """Writing to a drive somebody has their hand on, one level up from a file.""" + session = _session(tmp_path, "member-1", writable=True) + roots = session._ctx["roots"] + roots.roots[0].ejected = True + roots.roots[0].available = False + + session._do_dir_create({"dir": "shared", "name": "New folder"}) + refusal = [m for m in session.sent if m.get("type") == "error"] + assert refusal and refusal[0].get("code") == "root_unavailable" + assert not (tmp_path / "shared" / "New folder").exists() + + # ── Signed, or it is a suggestion ──────────────────────────────────────────── def _capture_challenges(session) -> list[tuple[str, str]]: -- 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') 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') 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 From e1dbdf0b7bebc27c2da7ae0c7095c5f88d4e1967 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Mon, 7 Sep 2026 02:26:17 +0200 Subject: fix(node): a replugged root came back without its metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported live: a removable root ejected from Files and plugged back in returned with its files and without its albums. Music showed "no music found" and stayed there through a force reload — the loss was on the node, not in the client. `plug_root` drops the root's entries and rescans, which is right; the drive may have changed while it was away. What comes back is a bare IndexEntry: `_hash_or_cached` fills id/name/path/size/type and nothing else. Every enrichment field goes with the old object, and the Music tag fields are cached nowhere by design (enrich_audio.py re-reads them so a rename can re-derive the filename fallback), so re-enrichment is the only way back. Two gates then made sure it never ran: * enrichment is scheduled for `delta.additions`, and ejecting broadcasts nothing, so `_last_broadcast_snapshot` still held those ids — the rebuilt entries diffed as updates, not additions; * `_enrich_new_*_entries` skips anything in `_enriched_attempted`, which is only discarded for `delta.deletions` — and dropping and rescanning inside one call broadcasts no deletion either. A restart cleared both, since an empty snapshot makes every entry an addition. Nothing short of one did. The indexer now records the ids it rebuilt and the daemon drains them at broadcast time: their "already attempted" mark is discarded and they rejoin the entries offered to the three enrichment passes. Not Music-specific — Videos lost durations and titles and Photos lost thumbnails the same way; Music is just where an untagged file has no album to file itself under, so the app goes empty rather than plain. `reconcile()` does the same drop-and-rescan when a root reappears on its own, so a USB drive that fell off and re-mounted hit this with nobody touching the UI. Covered too. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- packages/meshbay-node/src/meshbay_node/daemon.py | 18 ++ .../src/meshbay_node/indexer/indexer.py | 25 +++ .../tests/test_replug_restores_enrichment.py | 218 +++++++++++++++++++++ 3 files changed, 261 insertions(+) create mode 100644 packages/meshbay-node/tests/test_replug_restores_enrichment.py (limited to 'packages/meshbay-node/tests') diff --git a/packages/meshbay-node/src/meshbay_node/daemon.py b/packages/meshbay-node/src/meshbay_node/daemon.py index 74fff4c..f9e992c 100644 --- a/packages/meshbay-node/src/meshbay_node/daemon.py +++ b/packages/meshbay-node/src/meshbay_node/daemon.py @@ -1172,6 +1172,24 @@ class NodeDaemon: # this broadcast — enrichment fields arrive later as their own # INDEX_DELTA update (_on_enriched below). new_entries = delta.additions if delta is not None else list(idx.entries) + + # A root that was ejected and plugged back in, or that fell off and + # re-mounted, has had its entries thrown away and rebuilt from disk + # (`indexer._drop_root_entries`). The rebuilt entry has the same + # content-hash id and none of the enrichment fields, so the diff above + # reports neither an addition nor a deletion — and `_enriched_attempted` + # still says "done" for a file whose album and cover no longer exist. + # Found live: a Music library came back with its files and without its + # albums, and stayed that way, because only a restart (which starts + # with no snapshot, making every entry an addition) could clear either + # gate. Treated here as what it is — those entries are new again. + rebuilt_ids = indexer.drain_rescanned_ids() + if rebuilt_ids: + rebuilt = [e for e in idx.entries if e.id in rebuilt_ids] + for entry in rebuilt: + self._enriched_attempted.discard((group_id, entry.id)) + seen = {e.id for e in new_entries} + new_entries = new_entries + [e for e in rebuilt if e.id not in seen] asyncio.ensure_future(self._enrich_new_video_entries(indexer, new_entries)) # Music app (docs/musicbay.md §6): same shape, gated on audio_root # exactly like video_root above (added later — musicbay.md's diff --git a/packages/meshbay-node/src/meshbay_node/indexer/indexer.py b/packages/meshbay-node/src/meshbay_node/indexer/indexer.py index f7ffdca..eea5b4f 100644 --- a/packages/meshbay-node/src/meshbay_node/indexer/indexer.py +++ b/packages/meshbay-node/src/meshbay_node/indexer/indexer.py @@ -314,6 +314,14 @@ class DirectoryIndexer: # stay up for exactly as long as the slow part (hashing) is running. self._burst_inflight = 0 self._burst_sizes: dict[str, int] = {} + # Ids whose entry this indexer threw away and rebuilt from disk, since + # the last time a consumer drained this. A rebuilt entry carries only + # what `_hash_or_cached` fills in — every enrichment field the Videos, + # Music and Photos apps put there is gone — but its id is the file's + # content hash, so a diff against the last broadcast sees no addition + # and no deletion and nothing downstream can tell the fields were + # wiped. See `_drop_root_entries`. + self.rescanned_ids: set[str] = set() @property def index(self) -> GroupIndex: @@ -704,9 +712,26 @@ class DirectoryIndexer: if fold(e.path).split("/", 1)[0] == prefix] def _drop_root_entries(self, root: Root) -> None: + """ + Throw away a root's entries, always in order to rescan it. + + Both callers — `reconcile` when a root reappears, `plug_root` when the + operator plugs one back in — rebuild immediately, so nothing outside + ever observes the gap: no deletion is broadcast, and the entries that + come back have the same content-hash ids they had before. What they do + not have is anything enrichment put on them, which is why the ids are + recorded for `daemon._broadcast_index_change` to re-enrich rather than + simply forgotten. + """ for entry in self._entries_under(root): + self.rescanned_ids.add(entry.id) self._index.remove_entry(entry.id) + def drain_rescanned_ids(self) -> set[str]: + """Take the ids rebuilt since the last call; leave the set empty.""" + drained, self.rescanned_ids = self.rescanned_ids, set() + return drained + @staticmethod def _entry_path(root: Root, entry: IndexEntry) -> Path | None: _, _, tail = entry.path.partition("/") diff --git a/packages/meshbay-node/tests/test_replug_restores_enrichment.py b/packages/meshbay-node/tests/test_replug_restores_enrichment.py new file mode 100644 index 0000000..23fc97e --- /dev/null +++ b/packages/meshbay-node/tests/test_replug_restores_enrichment.py @@ -0,0 +1,218 @@ +""" +A root that comes back keeps its Videos/Music/Photos metadata. + +Reported live against a real library: a removable root ejected from the +Files app and plugged back in came back with its files but *without* its +albums — Music showed "no music found" and stayed that way through a force +reload, because the loss was on the node, not in the client. + +`DirectoryIndexer.plug_root` drops the root's entries and rescans, which is +right: the drive may have changed while it was away. What it produces is +bare `IndexEntry` objects — `_hash_or_cached` fills id/name/path/size/type +and nothing else. Every enrichment field (`artist`, `album`, `track_no`, +`duration`, `thumb_hash`, `display_title`, `taken_at`, ...) is gone, and +unlike the video/photo probe results those tag fields are cached nowhere: +`enrich_audio.py` deliberately re-reads them each time so a rename can +re-derive the filename fallback. Re-enrichment is the only way back. + +Two independent gates then made sure it never ran: + +* `_broadcast_index_change` schedules enrichment for `delta.additions`. + Ejecting never broadcasts, so `_last_broadcast_snapshot` still held those + ids — the re-added entries diffed as *updates*, not additions. +* `_enrich_new_*_entries` skips anything already in `_enriched_attempted`, + which is only discarded for `delta.deletions`. Dropping and rescanning + inside one call means no deletion is ever broadcast, so the mark survived + a wipe of the very fields it was standing for. + +A restart cleared both (an empty snapshot makes every entry an addition), +which is why this looked like it might fix itself and never did. + +The same drop-and-rescan runs in `reconcile()` — "Root %r is back" — so a +USB drive that falls off and returns on its own hits this without anybody +touching the UI. +""" + +import asyncio +import os + +import pytest +from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey + +from meshbay_common.crypto import generate_gek +from meshbay_node.config import (Config, GroupConfig, HubConfig, KeystoreConfig, + NodeConfig) +from meshbay_node.daemon import NodeDaemon +from meshbay_node.indexer import DirectoryIndexer +from meshbay_node.media_cache import MediaCache +from meshbay_node.roots import RootSet +from meshbay_node.roster import Roster + +pytestmark = pytest.mark.asyncio + +# Above indexer.py's MIN_AUDIO_SIZE_BYTES, or nothing would be indexed. +_AUDIO_BYTES = os.urandom(60 * 1024) + + +def _free_port() -> int: + import socket + with socket.socket() as s: + s.bind(("127.0.0.1", 0)) + return s.getsockname()[1] + + +class _CountingEnricher: + """Stands in for AudioEnricher: records who it was asked to enrich.""" + + def __init__(self): + self.spawned: list[str] = [] + + def spawn(self, entry, file_path, on_done, boundary=None): + self.spawned.append(entry.name) + + +async def _daemon(tmp_path, shared, group_id): + config = Config( + hub=HubConfig(url="http://localhost:9999", username="testuser"), + node=NodeConfig(quic_port=_free_port(), ui_port=_free_port()), + groups=[GroupConfig( + id=group_id, name="test-group", shared_dir=str(shared), + visibility="private", quic_port=_free_port(), + )], + keystore=KeystoreConfig(path=tmp_path / "keystore.enc"), + data_dir=tmp_path / "data", + ) + daemon = NodeDaemon(config) + daemon._broadcast_coalesce_secs = 0.01 + daemon._media_cache = MediaCache(db_path=tmp_path / "media_cache.db") + await daemon._media_cache.open() + daemon._roster = Roster(db_path=tmp_path / "roster.db") + await daemon._roster.open() + return daemon + + +async def _settled(daemon, indexer): + await daemon._on_index_change(indexer) + await asyncio.sleep(0.05) + + +async def test_a_replugged_root_gets_its_music_metadata_back(tmp_path): + group_id = "a" * 32 + library = tmp_path / "music" + (library / "an album").mkdir(parents=True) + (library / "an album" / "track.mp3").write_bytes(_AUDIO_BYTES) + + daemon = await _daemon(tmp_path, library, group_id) + enricher = _CountingEnricher() + daemon._audio_enricher = enricher + try: + await daemon._roster.set_app_directories( + group_id, "music", ["music/an album"], set_by="op") + + roots = RootSet.build([{"path": str(library), "removable": True}]) + indexer = DirectoryIndexer( + roots=roots, group_id=group_id, + sk_node=Ed25519PrivateKey.generate(), gek=generate_gek()) + await indexer.initial_scan() + await _settled(daemon, indexer) + assert enricher.spawned == ["track.mp3"], "the first pass never ran" + + # The operator ejects the drive from Files, then plugs it back in. + indexer.eject_root("music") + await indexer.plug_root("music") + await _settled(daemon, indexer) + + entry = next(iter(indexer.index.entries)) + assert entry.artist is None and entry.album is None, ( + "the rescan is supposed to produce a bare entry — if this ever " + "stops being true the rest of this test is measuring nothing") + assert enricher.spawned == ["track.mp3", "track.mp3"], ( + "a replugged root came back with its files and without its " + "albums, and nothing was ever going to fill them in again") + finally: + await daemon._media_cache.close() + await daemon._roster.close() + + +async def test_a_root_that_returns_on_its_own_is_treated_the_same(tmp_path): + """ + `reconcile()` rescans a root that reappears without anyone asking — a USB + drive re-mounting. It goes through the same drop-and-rescan, so it loses + the same fields, with no click anywhere to blame it on. + """ + group_id = "b" * 32 + library = tmp_path / "music" + (library / "an album").mkdir(parents=True) + (library / "an album" / "track.mp3").write_bytes(_AUDIO_BYTES) + + daemon = await _daemon(tmp_path, library, group_id) + enricher = _CountingEnricher() + daemon._audio_enricher = enricher + try: + await daemon._roster.set_app_directories( + group_id, "music", ["music/an album"], set_by="op") + + roots = RootSet.build([{"path": str(library), "removable": True}]) + indexer = DirectoryIndexer( + roots=roots, group_id=group_id, + sk_node=Ed25519PrivateKey.generate(), gek=generate_gek()) + await indexer.initial_scan() + await _settled(daemon, indexer) + assert enricher.spawned == ["track.mp3"] + + # Gone, then back — availability is what reconcile() watches. + roots.roots[0].available = False + await indexer.reconcile() + await _settled(daemon, indexer) + await indexer.reconcile() + await _settled(daemon, indexer) + + assert enricher.spawned.count("track.mp3") >= 2, ( + "a drive that fell off and came back left the library with no " + "metadata until the next daemon restart") + finally: + await daemon._media_cache.close() + await daemon._roster.close() + + +async def test_videos_and_photos_lost_the_same_fields(tmp_path): + """ + Nothing here is specific to Music — Videos and Photos hang off the same + `new_entries` list in `_broadcast_index_change`, so a replug took their + durations, titles and thumbnails with it too. Music is simply where it + shows up loudest: a track with no tags has no album to file it under, so + the app goes empty rather than merely plain. + """ + group_id = "c" * 32 + library = tmp_path / "media" + (library / "films").mkdir(parents=True) + (library / "films" / "clip.mkv").write_bytes(os.urandom(60 * 1024)) + (library / "album").mkdir(parents=True) + (library / "album" / "shot.jpg").write_bytes(os.urandom(60 * 1024)) + + daemon = await _daemon(tmp_path, library, group_id) + video, photo = _CountingEnricher(), _CountingEnricher() + daemon._enricher, daemon._photo_enricher = video, photo + try: + await daemon._roster.set_app_directories( + group_id, "video", ["media/films"], set_by="op") + await daemon._roster.set_app_directories( + group_id, "photo", ["media/album"], set_by="op") + + roots = RootSet.build([{"path": str(library), "removable": True}]) + indexer = DirectoryIndexer( + roots=roots, group_id=group_id, + sk_node=Ed25519PrivateKey.generate(), gek=generate_gek()) + await indexer.initial_scan() + await _settled(daemon, indexer) + assert video.spawned == ["clip.mkv"] and photo.spawned == ["shot.jpg"] + + indexer.eject_root("media") + await indexer.plug_root("media") + await _settled(daemon, indexer) + + assert video.spawned == ["clip.mkv"] * 2, "the film lost its probe" + assert photo.spawned == ["shot.jpg"] * 2, "the photo lost its thumbnail" + finally: + await daemon._media_cache.close() + await daemon._roster.close() -- cgit v1.2.3 From eeda274d751c537f4ecef3087994a16a9517478f Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Mon, 7 Sep 2026 03:13:14 +0200 Subject: fix(node): carry enrichment across a rescan instead of re-deriving it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit e1dbdf0 made a replugged root re-enrich, which was correct and not enough: the operator still watched their albums vanish. Measured on the reported library with a cold metadata cache, the node broadcast twice — the first delta stripped every album, the second put them back 14 seconds later. Fourteen seconds of "no music found" is the bug, whatever happens after. An entry's id is its content hash, so an entry that comes back under the same id, name and path is the same bytes in the same place and everything enrichment derived from it still holds. `_rescan_root` now carries those fields across the drop-and-rescan that `reconcile` and `plug_root` share. Re-enrichment stays as the fallback for what genuinely changed: a different id is different content, and a different name or path can change the folder and filename fallbacks that artist, album, display_title and track_no rest on, so those entries are still handed to the daemon through `rescanned_ids`. `uploader_id`/`uploader_pk` ride along. They are the same shape of field — set once on an entry, readable from nowhere on disk — and they decide who may delete the file, so losing them to a replug quietly took a right away. Verified on the running node: one broadcast 550ms after the plug, carrying the albums, and no metadata lookups at all. The tests now assert the field on the entry rather than a call to an enricher. Counting calls is what let the previous version of this file pass while the operator still saw an empty tab. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us --- .../src/meshbay_node/indexer/indexer.py | 68 +++++- .../tests/test_replug_restores_enrichment.py | 264 ++++++++++++++------- 2 files changed, 237 insertions(+), 95 deletions(-) (limited to 'packages/meshbay-node/tests') diff --git a/packages/meshbay-node/src/meshbay_node/indexer/indexer.py b/packages/meshbay-node/src/meshbay_node/indexer/indexer.py index eea5b4f..33e7210 100644 --- a/packages/meshbay-node/src/meshbay_node/indexer/indexer.py +++ b/packages/meshbay-node/src/meshbay_node/indexer/indexer.py @@ -609,8 +609,7 @@ class DirectoryIndexer: for root, available in changed: if available: log.info("Root %r is back — rescanning", root.name) - self._drop_root_entries(root) - await self._scan_root(root) + await self._rescan_root(root) touched = True else: # Frozen: entries stay, marked unavailable to members through @@ -711,17 +710,61 @@ class DirectoryIndexer: return [e for e in self._index.entries if fold(e.path).split("/", 1)[0] == prefix] + # Everything on an IndexEntry that a scan does not produce. `_scan_root` + # fills id/name/path/size/type/added_at/hash_version from the file itself; + # every field below was derived by one of the enrichment passes and is + # nowhere on disk to be read back. + _ENRICHED_FIELDS = ( + "duration", "thumb_hash", "width", "height", + "display_title", "season", "episode", + "artist", "album", "track_no", "taken_at", "camera", + "uploader_id", "uploader_pk", + ) + + async def _rescan_root(self, root: Root) -> int: + """ + Rebuild one root's entries from disk, keeping what the files still say. + + The two callers — `reconcile` when a root reappears, `plug_root` when + the operator plugs one back in — have to re-walk: the drive may have + changed while it was away. What they must not do is throw away the + enrichment. An entry's id is its content hash, so an entry that comes + back under the same id, name and path is the same bytes in the same + place, and every field the Videos, Music and Photos passes derived from + it still holds. Re-deriving them means minutes of tag reads, ffprobe + runs and rate-limited metadata lookups during which the operator's + library sits empty — which is exactly what a replug looked like. + + Anything that does *not* match is left bare on purpose: a different id + is different content, and a different name or path can change the + filename and folder fallbacks that `display_title`, `track_no`, + `artist` and `album` fall back to. Those are the entries + `daemon._broadcast_index_change` re-enriches, off `rescanned_ids`. + """ + carried = {(e.id, e.name, e.path): e for e in self._entries_under(root)} + self._drop_root_entries(root) + count = await self._scan_root(root) + for entry in self._entries_under(root): + old = carried.get((entry.id, entry.name, entry.path)) + if old is None: + continue + for field in self._ENRICHED_FIELDS: + setattr(entry, field, getattr(old, field)) + # It came back intact, so it is not one of the entries the daemon + # needs to enrich again. + self.rescanned_ids.discard(entry.id) + return count + def _drop_root_entries(self, root: Root) -> None: """ - Throw away a root's entries, always in order to rescan it. - - Both callers — `reconcile` when a root reappears, `plug_root` when the - operator plugs one back in — rebuild immediately, so nothing outside - ever observes the gap: no deletion is broadcast, and the entries that - come back have the same content-hash ids they had before. What they do - not have is anything enrichment put on them, which is why the ids are - recorded for `daemon._broadcast_index_change` to re-enrich rather than - simply forgotten. + Throw away a root's entries. Only ever called to rebuild them. + + The ids are recorded because nothing outside can otherwise tell they + were rebuilt: no deletion is broadcast (the rescan is immediate) and + the entries come back under the same content-hash ids, so a diff + against the last broadcast reports neither an addition nor a deletion. + `_rescan_root` clears the ones it managed to carry over intact; what is + left is genuinely new to the apps and is re-enriched by the daemon. """ for entry in self._entries_under(root): self.rescanned_ids.add(entry.id) @@ -794,8 +837,7 @@ class DirectoryIndexer: root.available = root.is_live() if root.available: log.info("Root %r plugged — rescanning", root.name) - self._drop_root_entries(root) - await self._scan_root(root) + await self._rescan_root(root) self._restart_observer() self._index.roots = self.roots.describe() self._index.version = int(time.time()) diff --git a/packages/meshbay-node/tests/test_replug_restores_enrichment.py b/packages/meshbay-node/tests/test_replug_restores_enrichment.py index 23fc97e..04a06ae 100644 --- a/packages/meshbay-node/tests/test_replug_restores_enrichment.py +++ b/packages/meshbay-node/tests/test_replug_restores_enrichment.py @@ -1,36 +1,44 @@ """ A root that comes back keeps its Videos/Music/Photos metadata. -Reported live against a real library: a removable root ejected from the -Files app and plugged back in came back with its files but *without* its -albums — Music showed "no music found" and stayed that way through a force -reload, because the loss was on the node, not in the client. - -`DirectoryIndexer.plug_root` drops the root's entries and rescans, which is -right: the drive may have changed while it was away. What it produces is -bare `IndexEntry` objects — `_hash_or_cached` fills id/name/path/size/type -and nothing else. Every enrichment field (`artist`, `album`, `track_no`, -`duration`, `thumb_hash`, `display_title`, `taken_at`, ...) is gone, and -unlike the video/photo probe results those tag fields are cached nowhere: -`enrich_audio.py` deliberately re-reads them each time so a rename can -re-derive the filename fallback. Re-enrichment is the only way back. - -Two independent gates then made sure it never ran: - -* `_broadcast_index_change` schedules enrichment for `delta.additions`. - Ejecting never broadcasts, so `_last_broadcast_snapshot` still held those - ids — the re-added entries diffed as *updates*, not additions. -* `_enrich_new_*_entries` skips anything already in `_enriched_attempted`, - which is only discarded for `delta.deletions`. Dropping and rescanning - inside one call means no deletion is ever broadcast, so the mark survived - a wipe of the very fields it was standing for. - -A restart cleared both (an empty snapshot makes every entry an addition), -which is why this looked like it might fix itself and never did. +Reported live: a removable root ejected from the Files app and plugged back +in returned with its files and without its albums. Music showed "no music +found", and it did not come back. + +`plug_root` has to re-walk the root — the drive may have changed while it +was away — and `_scan_root` produces bare entries: `_hash_or_cached` fills +id/name/path/size/type and nothing else. Every enrichment field went with +the old object, and the Music tag fields are cached nowhere by design +(`enrich_audio.py` re-reads them so a rename can re-derive the filename +fallback). + +Two gates then stopped anything from filling them in again: + +* enrichment is scheduled for `delta.additions`, and ejecting broadcasts + nothing — the last snapshot still held those ids, so the rebuilt entries + diffed as *updates*; +* `_enrich_new_*_entries` skips anything in `_enriched_attempted`, which is + only discarded for `delta.deletions` — and dropping and rescanning inside + one call broadcasts no deletion either. + +Only a restart cleared both, an empty snapshot making every entry an +addition. That is why it looked like it might fix itself and never did. + +Re-enriching is now the *fallback*, not the fix. An entry's id is its +content hash, so one that comes back under the same id, name and path is +the same bytes in the same place and its enrichment still holds: +`_rescan_root` carries those fields across. Re-deriving them instead meant +tag reads, ffprobe runs and rate-limited lookups — measured at 14 seconds +of empty Music tab on a real library with a cold cache, which to the +operator is indistinguishable from the original bug. + +What these assert is therefore the field on the entry, not a call to an +enricher. Counting calls is what made an earlier version of this file pass +while the operator still watched their albums vanish. The same drop-and-rescan runs in `reconcile()` — "Root %r is back" — so a -USB drive that falls off and returns on its own hits this without anybody -touching the UI. +USB drive that falls off and returns on its own hits all of this without +anybody touching the UI. """ import asyncio @@ -44,6 +52,7 @@ from meshbay_node.config import (Config, GroupConfig, HubConfig, KeystoreConfig, NodeConfig) from meshbay_node.daemon import NodeDaemon from meshbay_node.indexer import DirectoryIndexer +from meshbay_node.indexer.enrich_audio import AudioEnricher from meshbay_node.media_cache import MediaCache from meshbay_node.roots import RootSet from meshbay_node.roster import Roster @@ -62,7 +71,7 @@ def _free_port() -> int: class _CountingEnricher: - """Stands in for AudioEnricher: records who it was asked to enrich.""" + """Stands in for an enricher: records who it was asked to enrich.""" def __init__(self): self.spawned: list[str] = [] @@ -96,94 +105,177 @@ async def _settled(daemon, indexer): await asyncio.sleep(0.05) -async def test_a_replugged_root_gets_its_music_metadata_back(tmp_path): - group_id = "a" * 32 +async def _library(tmp_path, group_id, *, enricher=None): + """A one-track library under //, enriched once.""" library = tmp_path / "music" - (library / "an album").mkdir(parents=True) - (library / "an album" / "track.mp3").write_bytes(_AUDIO_BYTES) + (library / "an artist" / "a record").mkdir(parents=True) + (library / "an artist" / "a record" / "01 first track.mp3").write_bytes(_AUDIO_BYTES) daemon = await _daemon(tmp_path, library, group_id) + daemon._audio_enricher = enricher or AudioEnricher(daemon._media_cache) + await daemon._roster.set_app_directories( + group_id, "music", ["music"], set_by="op") + + roots = RootSet.build([{"path": str(library), "removable": True}]) + indexer = DirectoryIndexer( + roots=roots, group_id=group_id, + sk_node=Ed25519PrivateKey.generate(), gek=generate_gek()) + await indexer.initial_scan() + await _settled(daemon, indexer) + await asyncio.sleep(0.4) # the enricher runs off the broadcast + return daemon, indexer, roots, library + + +def _only(indexer): + return next(iter(indexer.index.entries)) + + +# ── The operator's own eject and plug ──────────────────────────────────────── + +async def test_the_albums_are_still_there_after_a_replug(tmp_path): + """ + No tags are written: `enrich_audio._artist_album_from_ancestors` derives + artist and album from the folder names when a file has none, which is the + /// layout this was reported against. + """ + group_id = "a" * 32 + daemon, indexer, _, _ = await _library(tmp_path, group_id) + try: + before = _only(indexer) + assert before.album == "a record" and before.artist == "an artist", ( + f"the first pass never filled the fields: {before}") + + indexer.eject_root("music") + await indexer.plug_root("music") + await _settled(daemon, indexer) + + after = _only(indexer) + assert after.album == "a record" and after.artist == "an artist", ( + "the entry came back from the rescan with no album — this is what " + "an empty Music tab after a replug looks like on the node") + finally: + await daemon._media_cache.close() + await daemon._roster.close() + + +async def test_the_fields_survive_without_re_deriving_them(tmp_path): + """ + Carried across, not recomputed. Re-deriving is correct and far too slow: + on a real library with a cold metadata cache it left Music empty for 14 + seconds, and an operator who looks in that window sees the bug. + """ + group_id = "b" * 32 enricher = _CountingEnricher() - daemon._audio_enricher = enricher + daemon, indexer, _, _ = await _library(tmp_path, group_id, enricher=enricher) try: - await daemon._roster.set_app_directories( - group_id, "music", ["music/an album"], set_by="op") + assert enricher.spawned == ["01 first track.mp3"] - roots = RootSet.build([{"path": str(library), "removable": True}]) - indexer = DirectoryIndexer( - roots=roots, group_id=group_id, - sk_node=Ed25519PrivateKey.generate(), gek=generate_gek()) - await indexer.initial_scan() + indexer.eject_root("music") + await indexer.plug_root("music") await _settled(daemon, indexer) - assert enricher.spawned == ["track.mp3"], "the first pass never ran" - # The operator ejects the drive from Files, then plugs it back in. + assert enricher.spawned == ["01 first track.mp3"], ( + "an unchanged file was enriched a second time — the whole point " + "of the content hash is that it did not need to be") + finally: + await daemon._media_cache.close() + await daemon._roster.close() + + +async def test_who_uploaded_a_file_survives_it_too(tmp_path): + """ + `uploader_id`/`uploader_pk` are the same shape of field — set once, on an + entry, readable from nowhere on disk — and they decide who may delete the + file. Losing them to a replug quietly takes a right away. + """ + group_id = "c" * 32 + daemon, indexer, _, _ = await _library(tmp_path, group_id) + try: + entry = _only(indexer) + entry.uploader_id = "alice" + entry.uploader_pk = "a-pinned-key" + indexer.eject_root("music") await indexer.plug_root("music") await _settled(daemon, indexer) - entry = next(iter(indexer.index.entries)) - assert entry.artist is None and entry.album is None, ( - "the rescan is supposed to produce a bare entry — if this ever " - "stops being true the rest of this test is measuring nothing") - assert enricher.spawned == ["track.mp3", "track.mp3"], ( - "a replugged root came back with its files and without its " - "albums, and nothing was ever going to fill them in again") + after = _only(indexer) + assert after.uploader_id == "alice" and after.uploader_pk == "a-pinned-key" finally: await daemon._media_cache.close() await daemon._roster.close() +# ── A drive that leaves and returns on its own ────────────────────────────── + async def test_a_root_that_returns_on_its_own_is_treated_the_same(tmp_path): """ `reconcile()` rescans a root that reappears without anyone asking — a USB - drive re-mounting. It goes through the same drop-and-rescan, so it loses - the same fields, with no click anywhere to blame it on. + drive re-mounting. Same drop-and-rescan, so it lost the same fields, with + no click anywhere to blame it on. """ - group_id = "b" * 32 - library = tmp_path / "music" - (library / "an album").mkdir(parents=True) - (library / "an album" / "track.mp3").write_bytes(_AUDIO_BYTES) - - daemon = await _daemon(tmp_path, library, group_id) - enricher = _CountingEnricher() - daemon._audio_enricher = enricher + group_id = "d" * 32 + daemon, indexer, roots, _ = await _library(tmp_path, group_id) try: - await daemon._roster.set_app_directories( - group_id, "music", ["music/an album"], set_by="op") - - roots = RootSet.build([{"path": str(library), "removable": True}]) - indexer = DirectoryIndexer( - roots=roots, group_id=group_id, - sk_node=Ed25519PrivateKey.generate(), gek=generate_gek()) - await indexer.initial_scan() - await _settled(daemon, indexer) - assert enricher.spawned == ["track.mp3"] + assert _only(indexer).album == "a record" - # Gone, then back — availability is what reconcile() watches. roots.roots[0].available = False await indexer.reconcile() await _settled(daemon, indexer) await indexer.reconcile() await _settled(daemon, indexer) + await asyncio.sleep(0.4) - assert enricher.spawned.count("track.mp3") >= 2, ( + assert _only(indexer).album == "a record", ( "a drive that fell off and came back left the library with no " - "metadata until the next daemon restart") + "metadata") finally: await daemon._media_cache.close() await daemon._roster.close() -async def test_videos_and_photos_lost_the_same_fields(tmp_path): +# ── What genuinely does have to be re-derived ─────────────────────────────── + +async def test_a_track_moved_while_the_drive_was_away_is_enriched_again(tmp_path): """ - Nothing here is specific to Music — Videos and Photos hang off the same - `new_entries` list in `_broadcast_index_change`, so a replug took their - durations, titles and thumbnails with it too. Music is simply where it - shows up loudest: a track with no tags has no album to file it under, so - the app goes empty rather than merely plain. + The counter-case, and the reason the carry-over is keyed on name and path + as well as id. `artist`, `album`, `display_title` and `track_no` all fall + back to the folder and filename when a file carries no tags, so the same + bytes under a new name are not the same metadata. Those are the entries + the daemon still re-enriches, off `rescanned_ids`. """ - group_id = "c" * 32 + group_id = "e" * 32 + enricher = _CountingEnricher() + daemon, indexer, _, library = await _library( + tmp_path, group_id, enricher=enricher) + try: + assert enricher.spawned == ["01 first track.mp3"] + + moved = library / "another artist" / "another record" + moved.mkdir(parents=True) + (library / "an artist" / "a record" / "01 first track.mp3").rename( + moved / "01 first track.mp3") + + indexer.eject_root("music") + await indexer.plug_root("music") + await _settled(daemon, indexer) + + assert enricher.spawned == ["01 first track.mp3"] * 2, ( + "the file is under a different artist and album now; carrying the " + "old ones across would file it under a folder it left") + finally: + await daemon._media_cache.close() + await daemon._roster.close() + + +async def test_videos_and_photos_are_covered_by_the_same_path(tmp_path): + """ + Nothing here is specific to Music — Videos and Photos lost their durations, + titles and thumbnails the same way. Music is simply where it shows up + loudest: a track with no tags has no album to file it under, so the app + goes empty rather than merely plain. + """ + group_id = "f" * 32 library = tmp_path / "media" (library / "films").mkdir(parents=True) (library / "films" / "clip.mkv").write_bytes(os.urandom(60 * 1024)) @@ -207,12 +299,20 @@ async def test_videos_and_photos_lost_the_same_fields(tmp_path): await _settled(daemon, indexer) assert video.spawned == ["clip.mkv"] and photo.spawned == ["shot.jpg"] + by_name = {e.name: e for e in indexer.index.entries} + by_name["clip.mkv"].duration = 1234 + by_name["shot.jpg"].thumb_hash = "a-thumbnail" + indexer.eject_root("media") await indexer.plug_root("media") await _settled(daemon, indexer) - assert video.spawned == ["clip.mkv"] * 2, "the film lost its probe" - assert photo.spawned == ["shot.jpg"] * 2, "the photo lost its thumbnail" + back = {e.name: e for e in indexer.index.entries} + assert back["clip.mkv"].duration == 1234, "the film lost its probe" + assert back["shot.jpg"].thumb_hash == "a-thumbnail", ( + "the photo lost its thumbnail") + assert video.spawned == ["clip.mkv"] and photo.spawned == ["shot.jpg"], ( + "unchanged files were probed and thumbnailed all over again") finally: await daemon._media_cache.close() await daemon._roster.close() -- cgit v1.2.3