From 32a86417d0edc3bf5c4cf859243c9e34b29bf1ef Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Thu, 10 Sep 2026 19:26:10 +0200 Subject: fix(node): the handshake ack dropped one app's directories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ack was assembled from its own tuple of application names, a copy of the daemon's `APP_DIR_KEYS`, and the two had drifted: the copy was missing `helloworld`. So the reference application — the one that exists to prove a new application needs no special-casing — was the single application whose configured folders never reached a client, which made the plugin claim false exactly where it is demonstrated. Fixed by removing the copy rather than syncing it. The ack now emits whatever `_directories` the group context carries, and `_app_directories_ctx` is the only thing that puts one there, so the two cannot disagree again. The transport names an application in one place, `ALLOWED_APPS`, which is enforcement rather than a directory list. The client had the same fault one layer up: `group-page.js` read three names by hand from the ack while the live-update path beside it was already generic. It derives the map from the ack's own keys now, so the fix reaches the settings pane instead of stopping at the wire. A first attempt moved the list to `roster.py`, where directory *storage* lives, and `test_helloworld_proves_the_plugin_claim.py` refused it: the roster, the ops, the config and the root set must name no application at all. That test is the architecture's own guard and it was right — the list belongs on the daemon, which is what wires a group's context, and everything downstream is derived from it. Two new tests, both verified to fail against the previous shape: the ack carries an application the node names nowhere else, and the ack keeps no list of its own. `test_the_lists_are_read_under_one_name_each` now asserts the shell names no application rather than that it names exactly three. Two stale comments went with it — the ack's, which described scalars removed in 07ff8b4, and the client's, which said those scalars still rode the wire for MNP 1.0 peers that can no longer connect. Full suite: 2258 passed, 4 skipped. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YVoHVCcfBqud6ZjG4db3y7 --- docs/MESHBAY_DESIGN.md | 24 +++++++- .../src/meshbay_hub/static/group-page.js | 29 ++++++---- .../meshbay-hub/tests/test_app_settings_plugin.py | 25 ++++++-- packages/meshbay-node/src/meshbay_node/daemon.py | 18 ++++-- .../src/meshbay_node/transport/webrtc_server.py | 24 +++++++- .../meshbay-node/tests/test_app_directories.py | 66 +++++++++++++++++++++- 6 files changed, 158 insertions(+), 28 deletions(-) diff --git a/docs/MESHBAY_DESIGN.md b/docs/MESHBAY_DESIGN.md index 73f6b42..67f0bff 100644 --- a/docs/MESHBAY_DESIGN.md +++ b/docs/MESHBAY_DESIGN.md @@ -1952,7 +1952,9 @@ There is no folder-browsing protocol and this does not add one. under. **One identifier per application, everywhere.** 3. **`-app-settings.js`** if it has anything to configure. Do not import the settings page — that is the cycle in §9.1. -4. **Add the key to the node's allow-list.** +4. **Add the key to the node's allow-list**, and — if the application keeps + directories — to `NodeDaemon.APP_DIR_KEYS`, the one list a group's context is + built from. Nothing else on the node may name an application. 5. **i18n:** at minimum a tab label key **in all ten catalogues**. A settings key added to the client must be added ten times; write the table and generate the insert. @@ -1971,6 +1973,26 @@ There is no folder-browsing protocol and this does not add one. No protocol change, no hub change, no daemon change. Steps 4 and 7 are the only node-side and test-side touches, and both are allow-lists. +> **A list of application names is only ever kept in one place, and everything +> downstream is derived from it.** There were three. The daemon built a group's +> context from one; the handshake ack was assembled from a copy that had already +> lost an entry — the reference application's, so the one application that exists +> to prove a new one needs no special-casing was the single one whose directories +> never reached a client; and the client shell named three applications by hand +> while the live-update path beside it was already generic. +> +> The fix that holds is **removing the copies, not syncing them**: the ack emits +> whatever `_directories` the context carries, and the shell reads the ack's +> own keys. Neither can drift, because neither has anything of its own to drift +> from. +> +> **Where the one list lives matters too.** It is on the daemon, which is what +> wires a group's context; the roster, the operator ops, the config and the root +> set must name no application at all, and a test holds them to it. That is the +> property the reference application exists to demonstrate, and it is the reason +> a first attempt at this fix — moving the list to the roster, where the +> directory *storage* lives — was wrong and was caught. + **A reference application exists in the tree behind a development flag.** Every other test of this architecture reads source for the *absence* of application names, which proves nobody wrote a special case — not that a new application works. 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 fec685b..c59d097 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/group-page.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/group-page.js @@ -350,15 +350,18 @@ function GroupPage({ groupId, group, token, username, userId, userPrefs, tokenCustomized: !!ack.tmdb_token_customized, language: ack.tmdb_language || '', }); - // The plural form when the node speaks it, the old scalars when it - // does not — an MNP 1.0 node sends only the latter, and reading its - // missing `video_directories` as "nothing configured" would empty a - // working Videos tab. - setAppDirectories({ - video: ack.video_directories || [], - music: ack.music_directories || [], - photo: ack.photo_directories || [], - }); + // Every `_directories` the ack carries, keyed by the app's own + // name — read off the ack rather than from a list of app names held + // here, so an application the node knows about is one this page already + // handles. Three names were hardcoded until 2026-09-10 and `helloworld` + // was not among them, so the app that exists to prove a new one needs + // no special-casing had its directories dropped on arrival. The live + // path below (`onAppDirectories`) was always generic; this was the half + // that was not. + setAppDirectories(Object.fromEntries( + Object.keys(ack) + .filter((k) => k.endsWith('_directories')) + .map((k) => [k.slice(0, -'_directories'.length), ack[k] || []]))); setChatDirectory(ack.chat_directory || ''); setChatLinkPreview(ack.chat_link_preview !== false); setMusicbrainzConfig({ @@ -677,9 +680,11 @@ function GroupPage({ groupId, group, token, username, userId, userPrefs, setEntries, setNodeDirs, setNodeRoots, applyIndex, 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, - // for a node that speaks MNP 1.0 — nothing in the client carries them. + // Plural everywhere, and built from the registry rather than a list of app + // names kept here: Videos and Music read a list, Photos always did, and an + // application added to `APPS` gets its own entry without this file + // changing. The scalar `videoRoot`/`audioRoot` shapes are gone from the + // wire too, so nothing anywhere carries them. ...perAppDirectories, tmdbConfig, musicbrainzConfig, onPlayQueue, diff --git a/packages/meshbay-hub/tests/test_app_settings_plugin.py b/packages/meshbay-hub/tests/test_app_settings_plugin.py index af86f12..940d975 100644 --- a/packages/meshbay-hub/tests/test_app_settings_plugin.py +++ b/packages/meshbay-hub/tests/test_app_settings_plugin.py @@ -256,15 +256,28 @@ def test_each_app_takes_a_list_of_directories(app, prop): def test_the_lists_are_read_under_one_name_each(): """ - One name per app on the ack — `_directories`, always a list. A second - name for the same answer means a page that reads whichever it thinks of - first, and a node that fills only the other one empties a working tab. + One name per app on the ack — `_directories`, always a list — and the + shell names none of them. + + It read three by hand until 2026-09-10, and `helloworld` was not among them: + the app that exists to prove a new one needs no special-casing had its + directories dropped on arrival, which made the plugin claim false exactly + where it is demonstrated. An app the node knows about must be one this page + already handles, and the only way to guarantee that is to read the ack's own + keys instead of a list kept here. + + A second *name* for the same answer would be the older fault: a page reading + whichever it thinks of first, and a node filling only the other one emptying + a working tab. """ page = GROUP_PAGE.read_text(encoding="utf-8") - block = page[page.index("setAppDirectories({"):] + block = page[page.index("setAppDirectories("):] block = block[:block.index("setChatDirectory")] - assert "ack.video_directories" in block and "ack.music_directories" in block - assert "ack.photo_directories" in block + assert "endsWith('_directories')" in block, ( + "the shell must derive the map from the ack's keys, not name apps") + for named in ("ack.video_directories", "ack.music_directories", + "ack.photo_directories"): + assert named not in block, f"{named} is an application named by the shell" for gone in ("ack.video_root", "ack.audio_root", "ack.photo_roots"): assert gone not in block, f"{gone} is a second name for the same answer" diff --git a/packages/meshbay-node/src/meshbay_node/daemon.py b/packages/meshbay-node/src/meshbay_node/daemon.py index 966dc69..dcfa8f8 100644 --- a/packages/meshbay-node/src/meshbay_node/daemon.py +++ b/packages/meshbay-node/src/meshbay_node/daemon.py @@ -1190,10 +1190,20 @@ 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. + # Every application that keeps directories. This is the one list, and it + # lives here because the daemon is what wires a group's context: `roster.py`, + # `ops.py` and the rest must name no application at all — that is the + # property the reference app exists to demonstrate + # (`test_helloworld_proves_the_plugin_claim.py`). + # + # Not derived from `enabled_apps`: the context is read once at load, and an + # application enabled later must not find its own setting missing. + # + # The handshake ack does **not** get a copy of this. It emits whatever + # `_directories` the context holds, so the two cannot drift — a copy + # lived in `webrtc_server.py` until 2026-09-10 and had already lost + # `helloworld`, which made the app that proves a new one needs no + # special-casing the single app whose directories never reached a client. APP_DIR_KEYS = ("video", "music", "photo", "chat", "helloworld") async def _app_directories_ctx(self, group_id: str) -> dict: 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 54f1541..a0776d2 100644 --- a/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py +++ b/packages/meshbay-node/src/meshbay_node/transport/webrtc_server.py @@ -897,9 +897,7 @@ class WebRTCPeerSession: # safe for the reason those were not: it is *derived* from this list # on every build rather than stored beside it, so the two cannot # drift apart. - **{f"{app}_directories": - list(self._group_ctx().get(f"{app}_directories") or []) - for app in ("video", "music", "photo", "chat")}, + **self._app_directories_ack(), # 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. @@ -3182,6 +3180,26 @@ class WebRTCPeerSession: task.add_done_callback(_on_done) return task + def _app_directories_ack(self) -> dict: + """ + Every application's configured folders, for the handshake ack. + + Read off the group context rather than from a list of applications kept + here, so this cannot name an application the node knows nothing else + about — and cannot fail to name one the daemon does. A copy of the + daemon's `APP_DIR_KEYS` lived here until 2026-09-10 and had already lost + an entry, which made the app that entry belonged to the single one whose + directories never reached a client. This module names an application in + exactly one place, and it is `ALLOWED_APPS`. + + `_app_directories_ctx` is the only thing that puts a `*_directories` key + in that context, and an absent one reads as none configured — never as + "the whole group index". + """ + return {key: list(value or []) + for key, value in self._group_ctx().items() + if key.endswith("_directories")} + def _group_ctx(self) -> dict: if "groups" in self._ctx and self._group_id: # `.get`, not a bare subscript. A config reload removes a group diff --git a/packages/meshbay-node/tests/test_app_directories.py b/packages/meshbay-node/tests/test_app_directories.py index 69b3be5..726f5bd 100644 --- a/packages/meshbay-node/tests/test_app_directories.py +++ b/packages/meshbay-node/tests/test_app_directories.py @@ -14,8 +14,9 @@ Two properties are new rather than moved, and both matter more than the tidying: 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 +* **a second name is derived, never stored.** `video_root` and friends are gone + from the wire entirely; `chat_directory` is the one that survives, and it is + computed from the list on every build. 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". """ @@ -370,3 +371,64 @@ async def test_an_empty_set_is_signable(tmp_path): session = _handler_session(tmp_path, authorized=True) session._do_app_directories({"app": "video", "directories": []}) assert session.issued == [(OP_APP_DIRECTORIES, "video:")] + + +# ── One list of applications, not two ──────────────────────────────────────── + +async def test_the_ack_carries_every_app_the_context_knows_about(): + """ + The handshake ack emits whatever `_directories` the group context + holds, so an application cannot be configurable on the node and invisible on + the wire. + + There were two lists until 2026-09-10 — the daemon built the context from + one and the ack was assembled from a copy — and they had already drifted by + one entry. The application that entry belonged to is the reference app, + which exists precisely to prove that a new application needs no + special-casing; it was the single application whose directories never + reached a client, so the claim was false exactly where it is demonstrated. + + Deriving the ack from the context removes the second list rather than + syncing it, which is the only version of this that cannot drift again. + """ + session = WebRTCPeerSession.__new__(WebRTCPeerSession) + session._ctx = { + "video_directories": ["Media/Films"], + "photo_directories": [], + # An application nothing on the node names. It reaches the ack because + # the context carries it, which is the whole property. + "helloworld_directories": ["Media"], + # Not directories, and must not be swept in. + "chat_directory": "Media", + "chat_epoch": 3, + } + session._group_id = "" + + ack = session._app_directories_ack() + + assert ack == { + "video_directories": ["Media/Films"], + "photo_directories": [], + "helloworld_directories": ["Media"], + } + + +async def test_the_ack_has_no_list_of_applications_of_its_own(): + """ + `NodeDaemon.APP_DIR_KEYS` is the list, and the transport must not keep a + copy: a second list is a second thing to remember when an application is + added, and the one that is forgotten disagrees silently. + + The transport names an application in exactly one place — `ALLOWED_APPS`, + which is server-side enforcement rather than a directory list. + """ + from meshbay_node.daemon import NodeDaemon + + source = Path(WebRTCPeerSession.__module__.replace(".", "/")) + text = (Path(__file__).resolve().parents[2] / "meshbay-node" / "src" + / f"{source}.py").read_text(encoding="utf-8") + body = text[text.index("def _app_directories_ack"):] + body = body[:body.index("\n def ", 1)] + for app in NodeDaemon.APP_DIR_KEYS: + assert app not in body, ( + f"the ack names {app!r}; it must read the context's own keys") -- cgit v1.2.3