fix(node): the MNP root path never reloaded, and my first repair made it worse

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pvMdvLBG92jyhvD5pD6us

1 files changed, 232 insertions, 63 deletions

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.
+Adding or removing a root has to reach the running node, not only node.toml.
 
-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.
+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.
 
-`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.
+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 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.
+**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
+        assert result["roots"][-1]["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):
+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()