From 5ed1c972d94a81dbb17ee859829d3731dbf1287f Mon Sep 17 00:00:00 2001 From: ethernet Date: Fri, 7 Aug 2026 15:51:45 -0400 Subject: [PATCH] refactor(sidecars): one resolver for the Photon and WhatsApp children Both sidecars answered the same question in their own way. Which directory does this Node child run from, when some installs put the source tree somewhere nothing can write? gateway/sidecar_runtime.py answers it once. Four rungs: 1. An operator override. 2. A writable source. 3. A read-only source whose baked deps match the lockfile. 4. A read-only source that must move to $HERMES_HOME/sidecars/ before npm can run. Node sets the shape of that last rung. Its ESM resolver reads node_modules only from the directories above the importing file, and NODE_PATH applies to CommonJS alone. Measured on Node 26: an ESM import with NODE_PATH pointing at the packages fails, and the same import from a directory beside them works. Both sidecars are "type": "module", so the entry file and the packages must share a tree. A copy is the only arrangement Node accepts. _MIRROR_FILES is gone. It named the files to copy, so it had to name every module the entry file imports. It was wrong twice. It listed the deleted spectrum patch, and it omitted send-format.mjs and stream-staleness.mjs. Both faults appear only on a read-only install. The resolver copies the tree instead, without node_modules, and the test compares the mirror against the source tree rather than against a second list. A mutation that returns to a fixed list fails it. The copy uses shutil.copy, not copy2. copy2 gives the mirror file the mtime of the source, and a Nix store source has mtime = epoch. A refreshed lockfile then always predates npm's install marker, and deps_are_current() keeps stale node_modules through every upgrade, which is the fault this resolver exists to fix. A plain copy stamps the copy time, so a content change always postdates the previous install. npm's hidden node_modules/.package-lock.json cannot replace the content comparison: it is a different document, and npm matches it semantically, not byte for byte. WhatsApp gains what it never had: a staleness check and a refresh. Its resolver returned any existing mirror without comparing it against the lockfile, so an upgrade kept the old node_modules. This is a behaviour change. The mirrors move to $HERMES_HOME/sidecars/. The Baileys credentials live in $HERMES_HOME/whatsapp/session, so a paired account is not affected. `hermes doctor` reports the mirrors the old resolvers left at $HERMES_HOME/photon/sidecar and $HERMES_HOME/scripts/whatsapp-bridge. Nothing reads them now, and each one can hold a node_modules of some hundred MB. --fix removes them. _sidecar_deps_stale and deps_are_current read the same two files with opposite missing-file answers, on purpose. Each one points at the other and says why. The container bakes both sidecars now. It baked Photon and left WhatsApp to install at run time. --- gateway/platforms/whatsapp_common.py | 54 +--- gateway/sidecar_runtime.py | 157 ++++++++++ hermes_cli/doctor.py | 30 ++ plugins/platforms/photon/adapter.py | 6 + plugins/platforms/photon/sidecar_paths.py | 128 ++------- tests/gateway/test_sidecar_runtime.py | 269 ++++++++++++++++++ tests/gateway/test_whatsapp_bridge_dir.py | 110 +++++++ .../test_whatsapp_bridge_dir_resolution.py | 60 ---- .../platforms/photon/test_sidecar_paths.py | 126 +++----- 9 files changed, 650 insertions(+), 290 deletions(-) create mode 100644 gateway/sidecar_runtime.py create mode 100644 tests/gateway/test_sidecar_runtime.py create mode 100644 tests/gateway/test_whatsapp_bridge_dir.py delete mode 100644 tests/gateway/test_whatsapp_bridge_dir_resolution.py diff --git a/gateway/platforms/whatsapp_common.py b/gateway/platforms/whatsapp_common.py index 09e7b0b64f886..0ebe6133d2f2c 100644 --- a/gateway/platforms/whatsapp_common.py +++ b/gateway/platforms/whatsapp_common.py @@ -35,6 +35,7 @@ import json import logging import os import re +from pathlib import Path from typing import Any, Dict, Optional from agent.secret_scope import UnscopedSecretError as _UnscopedSecretError @@ -503,50 +504,19 @@ class WhatsAppBehaviorMixin: # Shared bridge directory resolution for CLI and adapter # --------------------------------------------------------------------------- +SOURCE_BRIDGE_DIR = Path(__file__).resolve().parents[2] / "scripts" / "whatsapp-bridge" + + def resolve_whatsapp_bridge_dir() -> Path: - """Resolve the WhatsApp bridge directory, mirroring to HERMES_HOME if needed. + """Return the directory the WhatsApp bridge should run from. - When the install tree is read-only (e.g., Docker /opt/hermes), this function - mirrors the bridge source to a writable HERMES_HOME location and returns that - path. This ensures npm install works in Docker environments. + gateway.sidecar_runtime holds the rungs and the reason a read-only tree + has to copy the bridge somewhere writable. - Returns the resolved bridge directory path. + The bridge keeps its Baileys credentials in ``$HERMES_HOME/whatsapp/session``, + which is a separate directory, so where the bridge itself runs from does + not affect a paired account. """ - import shutil - from pathlib import Path as _Path + from gateway.sidecar_runtime import resolve_sidecar - # Default location in install tree (may be read-only) - from hermes_constants import get_hermes_home - install_bridge = _Path(__file__).resolve().parents[2] / "scripts" / "whatsapp-bridge" - - # Try HERMES_HOME location first - hermes_home = get_hermes_home() - hermes_home_bridge = hermes_home / "scripts" / "whatsapp-bridge" - - # Check if install dir is writable - try: - test_file = install_bridge / ".write_test" - test_file.touch() - test_file.unlink() - install_writable = True - except (OSError, PermissionError): - install_writable = False - - if install_writable: - return install_bridge - - # Install dir is read-only, mirror to HERMES_HOME if needed - if hermes_home_bridge.exists(): - return hermes_home_bridge - - # Mirror the bridge source to HERMES_HOME - try: - hermes_home_bridge.parent.mkdir(parents=True, exist_ok=True) - shutil.copytree( - install_bridge, - hermes_home_bridge, - dirs_exist_ok=False, - ) - return hermes_home_bridge - except Exception: - return install_bridge + return resolve_sidecar("whatsapp", SOURCE_BRIDGE_DIR) diff --git a/gateway/sidecar_runtime.py b/gateway/sidecar_runtime.py new file mode 100644 index 0000000000000..38b3128e2a808 --- /dev/null +++ b/gateway/sidecar_runtime.py @@ -0,0 +1,157 @@ +"""Resolve the directory a Node sidecar runs from. + +Hermes ships two Node sidecars: the Photon iMessage bridge +(``plugins/platforms/photon/sidecar``) and the WhatsApp Baileys bridge +(``scripts/whatsapp-bridge``). Both need a ``node_modules`` beside their entry +file, and some installs put the source tree somewhere nothing can write. + +Node decides the shape of the answer. Its ESM resolver reads ``node_modules`` +only from directories above the importing file. ``NODE_PATH`` applies to +CommonJS and not to ESM, and both sidecars declare ``"type": "module"``, so +there is no way to leave the code in a read-only tree and point Node at +packages held elsewhere. The entry file and the packages have to share a tree. +That is why a read-only install with no usable deps copies the sidecar to a +writable directory: it is the only arrangement Node accepts, not a preference. + +Order: + +1. ``HERMES__SIDECAR_DIR`` — an operator override, used as given. +2. Source writable → run in place. Dev checkouts, and any plugin a user + installs under ``$HERMES_HOME/plugins``. +3. Source read-only, deps present and matching the lockfile → run in place. + The Nix store and the container image both arrive this way, and the + sidecar never writes inside its own directory. +4. Source read-only, deps missing or stale → copy the sidecar to + ``$HERMES_HOME/sidecars/`` and hand back that path, where the + caller's usual npm install can work. + +Rung 4 copies the whole tree apart from ``node_modules``. It does not consult +a list of files to copy: such a list has to name every module the entry file +imports, and the one this replaces was wrong twice, each time in a way that +only appears on a read-only install. +""" +from __future__ import annotations + +import filecmp +import logging +import os +import shutil +from pathlib import Path + +logger = logging.getLogger(__name__) + +_DEPS_DIR = "node_modules" +# npm writes this inside node_modules to record the state of the last install. +# Comparing it with the committed lockfile is the check `npm ci` runs. +_INSTALL_MARKER = ".package-lock.json" + + +def override_env_var(name: str) -> str: + """Return the override variable for sidecar ``name``.""" + return f"HERMES_{name.upper().replace('-', '_')}_SIDECAR_DIR" + + +def dir_writable(path: Path) -> bool: + """Can Hermes create a file in ``path``? + + Probe with a real create and delete. A stat of the mode bits gives the + wrong answer under root-squash and on a read-only bind mount, which are + the cases this function exists to detect. + """ + probe = path / ".hermes-write-probe" + try: + probe.touch() + probe.unlink() + return True + except OSError: + return False + + +def deps_are_current(sidecar_dir: Path) -> bool: + """Does the install in ``sidecar_dir`` match its lockfile? + + False when either file is absent or unreadable, so a first run and an odd + filesystem both resolve to "install needed" rather than to an error. + + ``plugins.platforms.photon.adapter._sidecar_deps_stale`` reads the same + two files with the opposite missing-file answer, on purpose: there the + missing case belongs to ``sidecar_deps_installed``. + """ + lockfile = sidecar_dir / "package-lock.json" + marker = sidecar_dir / _DEPS_DIR / _INSTALL_MARKER + try: + return marker.stat().st_mtime >= lockfile.stat().st_mtime + except OSError: + return False + + +def _refresh_mirror(source: Path, mirror: Path) -> None: + """Copy ``source`` into ``mirror``, without ``node_modules``. + + Runs on each resolve, so an image update reaches a mirror that already + exists. Compares content rather than mtime, because a copy has the mtime + of the copy. ``node_modules`` stays out: npm owns the mirror's copy, and + replacing it would make each update a fresh install. + """ + mirror.mkdir(parents=True, exist_ok=True) + shutil.copytree( + source, + mirror, + ignore=shutil.ignore_patterns(_DEPS_DIR), + copy_function=_copy_if_changed, + dirs_exist_ok=True, + ) + + +def _copy_if_changed(src: str, dst: str) -> None: + """Copy, skipped when the destination already holds the same bytes. + + ``shutil.copy``, not ``copy2``. copy2 gives the destination the mtime + of the SOURCE, and a Nix store source has mtime = epoch. A refreshed + mirror lockfile with an epoch mtime always looks older than npm's + install marker, so ``deps_are_current`` keeps stale node_modules + through every upgrade. A plain copy stamps the file with the copy + time, so a content change always postdates the previous install. + + npm's ``node_modules/.package-lock.json`` cannot replace this content + comparison. It is a different document, and npm matches it against the + committed lockfile semantically, not byte for byte. + """ + if os.path.exists(dst) and filecmp.cmp(src, dst, shallow=False): + return + shutil.copy(src, dst) + + +def resolve_sidecar(name: str, source_dir: Path) -> Path: + """Return the directory sidecar ``name`` should run from. + + ``source_dir`` is where the sidecar ships in the install tree. + """ + source = Path(source_dir) + + override = os.getenv(override_env_var(name)) + if override: + return Path(override) + + if dir_writable(source): + return source + + if (source / _DEPS_DIR).exists() and deps_are_current(source): + return source + + from hermes_constants import get_hermes_home + + mirror = get_hermes_home() / "sidecars" / name + try: + _refresh_mirror(source, mirror) + return mirror + except OSError as exc: + logger.warning( + "[%s] the install tree is read-only and the copy to %s failed " + "(%s). Falling back to the read-only source directory, where a " + "dependency install cannot run.", + name, + mirror, + exc, + ) + return source diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 88ce3ecba73bf..bf1ea69ddf461 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -1639,6 +1639,36 @@ def run_doctor(args): fixed_count += 1 else: check_warn(f"{_DHH}/{subdir_name}/ not found", "(will be created on first use)") + + # Orphaned sidecar mirrors. The shared resolver (gateway/sidecar_runtime) + # mirrors the Node sidecars to $HERMES_HOME/sidecars/. The old + # per-sidecar resolvers used $HERMES_HOME/photon/sidecar and + # $HERMES_HOME/scripts/whatsapp-bridge. Nothing reads those paths now, + # and each one can hold a node_modules of some hundred MB. + legacy_mirrors = [ + hermes_home / "photon" / "sidecar", + hermes_home / "scripts" / "whatsapp-bridge", + ] + for legacy in legacy_mirrors: + if not legacy.is_dir() or legacy.is_symlink(): + continue + rel = legacy.relative_to(hermes_home) + if should_fix: + import shutil as _shutil + + _shutil.rmtree(legacy, ignore_errors=True) + # Drop the parent too when the mirror was its only content. + try: + legacy.parent.rmdir() + except OSError: + pass + check_ok(f"Removed orphaned sidecar mirror {_DHH}/{rel}/") + fixed_count += 1 + else: + check_warn( + f"{_DHH}/{rel}/ is an orphaned sidecar mirror", + "(replaced by ~/.hermes/sidecars/ — run `hermes doctor --fix` to remove)", + ) # Check for SOUL.md persona file soul_path = hermes_home / "SOUL.md" diff --git a/plugins/platforms/photon/adapter.py b/plugins/platforms/photon/adapter.py index 662e8ced6c599..418703648a200 100644 --- a/plugins/platforms/photon/adapter.py +++ b/plugins/platforms/photon/adapter.py @@ -464,6 +464,12 @@ def _sidecar_deps_stale() -> bool: lockfile is newer than that marker, the install is out of date. This is the same signal ``npm ci`` uses. Returns False (do nothing) if either file is missing or unreadable, so a first-run or odd filesystem never blocks start. + + ``gateway.sidecar_runtime.deps_are_current`` reads the same two files with + the OPPOSITE missing-file answer: the resolver treats a missing marker as + "install needed" and mirrors, this adapter treats it as "nothing to do" + because ``sidecar_deps_installed`` owns the missing case. Keep that + difference if you merge them. """ lockfile = _sidecar_dir() / "package-lock.json" marker = _sidecar_dir() / "node_modules" / ".package-lock.json" diff --git a/plugins/platforms/photon/sidecar_paths.py b/plugins/platforms/photon/sidecar_paths.py index 69f86a145846c..2ff972fc8307a 100644 --- a/plugins/platforms/photon/sidecar_paths.py +++ b/plugins/platforms/photon/sidecar_paths.py @@ -1,32 +1,10 @@ """ Resolve where the Photon sidecar runs from and where its Node deps live. -The sidecar source ships inside the installed plugin tree -(``plugins/platforms/photon/sidecar/``). On dev/source installs that tree is -writable and everything — ``npm ci``, the spectrum patch, the sidecar itself — -happens in place. Hosted/managed images instead keep the whole install tree -under an immutable ``/opt/hermes`` (read-only for the hermes user), which -broke every install/self-heal path with EROFS (NS-606). - -Resolution order (mirrors ``resolve_whatsapp_bridge_dir`` for the Baileys -bridge, which hit the same wall): - -1. ``PHOTON_SIDECAR_DIR`` env override — operator escape hatch, used as-is. -2. Source dir writable → run in place (dev installs, unchanged behavior). -3. Source dir read-only but ``node_modules`` is baked and current → run in - place. This is the managed-image happy path: the Dockerfile bakes the - sidecar deps with ``npm ci`` at build time (deterministic installs, - NS-559), so no runtime install is ever needed. -4. Source dir read-only and deps missing or stale → mirror the sidecar - source files to ``$HERMES_HOME/photon/sidecar`` (the durable data volume, - e.g. ``/opt/data`` on hosted) and return that. The caller's normal - install/self-heal machinery then works there because it is writable. - -The mirror is refreshed on every resolve: when an image update changes a -sidecar source file, the changed file is re-copied (content compare, not -mtime) while ``node_modules`` is left in place — the adapter's existing -lockfile-vs-install-marker staleness check then triggers the ``npm ci`` -self-heal inside the mirror. +The rungs and the reason a read-only tree has to copy the sidecar are in +``gateway.sidecar_runtime``, which this module wraps. What belongs here is +the Photon-specific part: where the sidecar ships, and the +``PHOTON_SIDECAR_DIR`` override name that predates the shared resolver. This module is import-light on purpose: both ``adapter.py`` (gateway) and ``cli.py`` (``hermes photon ...``) use it. @@ -34,108 +12,42 @@ This module is import-light on purpose: both ``adapter.py`` (gateway) and from __future__ import annotations -import filecmp import logging import os -import shutil from pathlib import Path from typing import Optional +from gateway.sidecar_runtime import ( + dir_writable, + override_env_var, + resolve_sidecar, +) + logger = logging.getLogger(__name__) SOURCE_SIDECAR_DIR = Path(__file__).parent / "sidecar" -# The files that define the sidecar. Mirrored into the writable runtime dir -# when the install tree is read-only. node_modules is deliberately absent — -# it is either baked (managed image) or installed by npm in the mirror. -_MIRROR_FILES = ( - "index.mjs", - "package.json", - "package-lock.json", - "patch-spectrum-mixed-attachments.mjs", -) - - -def dir_writable(path: Path) -> bool: - """True when we can create files in ``path`` (probe-based, not stat). - - A stat-mode check lies on containers (root-squash, read-only bind - mounts), so probe with a real create+unlink like the WhatsApp bridge - resolver does. - """ - probe = path / ".hermes-write-probe" - try: - probe.touch() - probe.unlink() - return True - except OSError: - return False - +_SIDECAR_NAME = "photon" # Backwards-friendly private alias for module-internal use. _dir_writable = dir_writable -def _lock_newer_than_install(sidecar_dir: Path) -> bool: - """True when the committed lockfile postdates npm's install marker. - - Same signal as ``adapter._sidecar_deps_stale`` — duplicated here (three - lines) rather than imported so this module stays import-light for the - CLI. Returns False on any stat failure so an odd filesystem never forces - the mirror path. - """ - lockfile = sidecar_dir / "package-lock.json" - marker = sidecar_dir / "node_modules" / ".package-lock.json" - try: - return lockfile.stat().st_mtime > marker.stat().st_mtime - except OSError: - return False - - def resolve_sidecar_dir(source_dir: Optional[Path] = None) -> Path: - """Return the directory the sidecar should run from (see module doc). + """Return the directory the sidecar should run from. ``source_dir`` defaults to the installed plugin tree; tests and callers that monkeypatch the adapter's ``_SIDECAR_DIR`` pass it through so the override keeps working. + + ``PHOTON_SIDECAR_DIR`` is read as well as the shared + ``HERMES_PHOTON_SIDECAR_DIR``. Operators set the short name before the + resolver was shared, and it costs one line to keep working. """ source = Path(source_dir) if source_dir is not None else SOURCE_SIDECAR_DIR - override = os.getenv("PHOTON_SIDECAR_DIR") - if override: - return Path(override) + legacy = os.getenv("PHOTON_SIDECAR_DIR") + if legacy and not os.getenv(override_env_var(_SIDECAR_NAME)): + return Path(legacy) - if _dir_writable(source): - return source - - # Read-only install tree (hosted/managed image). If the image baked the - # deps at build time and they match the lockfile, run in place — the - # sidecar itself never writes inside its own directory. - if (source / "node_modules").exists() and not _lock_newer_than_install(source): - return source - - # Deps missing or stale inside a read-only tree: mirror to the durable - # data volume so the normal install/self-heal machinery has somewhere - # writable to work. - from hermes_constants import get_hermes_home - - mirror = get_hermes_home() / "photon" / "sidecar" - try: - mirror.mkdir(parents=True, exist_ok=True) - for name in _MIRROR_FILES: - src = source / name - if not src.exists(): - continue - dst = mirror / name - if not dst.exists() or not filecmp.cmp(str(src), str(dst), shallow=False): - shutil.copy2(str(src), str(dst)) - return mirror - except OSError as exc: - logger.warning( - "[photon] install tree is read-only and mirroring the sidecar " - "to %s failed (%s) — falling back to the read-only source dir; " - "dependency installs will not be possible", - mirror, - exc, - ) - return source + return resolve_sidecar(_SIDECAR_NAME, source) diff --git a/tests/gateway/test_sidecar_runtime.py b/tests/gateway/test_sidecar_runtime.py new file mode 100644 index 0000000000000..f6b4639fba085 --- /dev/null +++ b/tests/gateway/test_sidecar_runtime.py @@ -0,0 +1,269 @@ +"""Behaviour of the shared Node sidecar resolver. + +The resolver answers one question: which directory does a Node sidecar run +from? A sidecar needs its node_modules, and Node's ESM resolver only looks in +ancestor directories of the importing file, so the entry file and the deps +must live in one tree. NODE_PATH does not work for ESM, and both in-tree +sidecars are "type": "module". + +That constraint is why a read-only install tree with no baked deps has to copy +the sidecar somewhere writable. Nothing else Node offers gets the two into the +same tree. +""" +from __future__ import annotations + +import json +import os +from pathlib import Path + +import pytest + +from gateway.sidecar_runtime import resolve_sidecar + + +def _make_sidecar(root: Path, *, files=("index.mjs", "helper.mjs")) -> Path: + """A sidecar source dir with a lockfile and a couple of ESM helpers.""" + src = root / "src" + src.mkdir(parents=True) + (src / "package.json").write_text(json.dumps({"name": "sc", "type": "module"})) + (src / "package-lock.json").write_text(json.dumps({"lockfileVersion": 3})) + for name in files: + (src / name).write_text(f"// {name}\n") + return src + + +def _install_deps(src: Path, *, current: bool = True) -> None: + """Give the sidecar a node_modules with npm's install marker.""" + nm = src / "node_modules" + (nm / "somepkg").mkdir(parents=True) + (nm / "somepkg" / "index.js").write_text("module.exports = 1;\n") + marker = nm / ".package-lock.json" + marker.write_text("{}") + lock = src / "package-lock.json" + if current: + # Marker newer than the lockfile: the install matches. + os.utime(marker, (lock.stat().st_atime + 10, lock.stat().st_mtime + 10)) + else: + os.utime(marker, (lock.stat().st_atime - 10, lock.stat().st_mtime - 10)) + + +class TestRungOrder: + """The four rungs, in the order the resolver must try them.""" + + def test_env_override_is_used_as_is(self, tmp_path, monkeypatch): + """An operator override wins over every other rung. + + It is the escape hatch for a layout Hermes cannot predict, so the + resolver must not second-guess it. + """ + src = _make_sidecar(tmp_path) + override = tmp_path / "elsewhere" + override.mkdir() + monkeypatch.setenv("HERMES_TESTSC_SIDECAR_DIR", str(override)) + assert resolve_sidecar("testsc", src) == override + + def test_writable_source_runs_in_place(self, tmp_path, monkeypatch): + """A dev checkout installs and runs in the source tree.""" + monkeypatch.delenv("HERMES_TESTSC_SIDECAR_DIR", raising=False) + src = _make_sidecar(tmp_path) + assert resolve_sidecar("testsc", src) == src + + def test_readonly_source_with_current_deps_runs_in_place( + self, tmp_path, monkeypatch + ): + """Baked deps that match the lockfile need no writable directory. + + This is the Nix store and the built container image. The sidecar + never writes inside its own directory, so read-only is fine. + """ + monkeypatch.delenv("HERMES_TESTSC_SIDECAR_DIR", raising=False) + src = _make_sidecar(tmp_path) + _install_deps(src, current=True) + monkeypatch.setattr( + "gateway.sidecar_runtime.dir_writable", lambda p: False + ) + assert resolve_sidecar("testsc", src) == src + + def test_readonly_source_with_stale_deps_mirrors(self, tmp_path, monkeypatch): + """A lockfile newer than the install means the deps must be rebuilt. + + The source tree cannot take the write, so the sidecar moves to the + durable directory where npm can run. + """ + monkeypatch.delenv("HERMES_TESTSC_SIDECAR_DIR", raising=False) + src = _make_sidecar(tmp_path) + _install_deps(src, current=False) + home = tmp_path / "home" + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr( + "gateway.sidecar_runtime.dir_writable", lambda p: False + ) + got = resolve_sidecar("testsc", src) + assert got != src + assert got == home / "sidecars" / "testsc" + + def test_readonly_source_with_no_deps_mirrors(self, tmp_path, monkeypatch): + monkeypatch.delenv("HERMES_TESTSC_SIDECAR_DIR", raising=False) + src = _make_sidecar(tmp_path) + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home")) + monkeypatch.setattr( + "gateway.sidecar_runtime.dir_writable", lambda p: False + ) + assert resolve_sidecar("testsc", src) != src + + +class TestMirrorCarriesTheWholeSidecar: + """The mirror must be complete. A manifest of files is not enough. + + The Photon resolver kept a list of files to copy. The list drifted twice: + it named a file that had been deleted, and it omitted two helpers that + index.mjs imports. Both faults appear only on a read-only install, which + is the one place nobody runs by hand. + """ + + @pytest.fixture + def mirrored(self, tmp_path, monkeypatch): + monkeypatch.delenv("HERMES_TESTSC_SIDECAR_DIR", raising=False) + src = _make_sidecar( + tmp_path, files=("index.mjs", "helper.mjs", "deep-helper.mjs") + ) + (src / "sub").mkdir() + (src / "sub" / "nested.mjs").write_text("// nested\n") + _install_deps(src, current=False) + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home")) + monkeypatch.setattr( + "gateway.sidecar_runtime.dir_writable", lambda p: False + ) + return src, resolve_sidecar("testsc", src) + + def test_every_source_file_reaches_the_mirror(self, mirrored): + """Whatever the source holds, the mirror holds. + + Asserted against the source tree itself, not against a second list, + so a new helper cannot be forgotten. + """ + src, mirror = mirrored + want = { + p.relative_to(src) + for p in src.rglob("*") + if p.is_file() and "node_modules" not in p.parts + } + got = { + p.relative_to(mirror) + for p in mirror.rglob("*") + if p.is_file() and "node_modules" not in p.parts + } + assert want <= got, f"missing from the mirror: {sorted(want - got)}" + + def test_node_modules_is_not_copied(self, mirrored): + """npm owns the mirror's deps. Copying them wastes time and space.""" + _src, mirror = mirrored + assert not (mirror / "node_modules" / "somepkg").exists() + + +class TestRefresh: + """An image update changes a source file. The mirror must follow.""" + + def _mirror_twice(self, tmp_path, monkeypatch, mutate): + monkeypatch.delenv("HERMES_TESTSC_SIDECAR_DIR", raising=False) + src = _make_sidecar(tmp_path) + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home")) + monkeypatch.setattr( + "gateway.sidecar_runtime.dir_writable", lambda p: False + ) + mirror = resolve_sidecar("testsc", src) + # Deps installed in the mirror by npm — these must survive a refresh. + (mirror / "node_modules" / "somepkg").mkdir(parents=True) + (mirror / "node_modules" / "somepkg" / "index.js").write_text("1") + mutate(src) + return src, resolve_sidecar("testsc", src) + + def test_a_changed_source_file_is_recopied(self, tmp_path, monkeypatch): + def mutate(src): + (src / "index.mjs").write_text("// version two\n") + + _src, mirror = self._mirror_twice(tmp_path, monkeypatch, mutate) + assert (mirror / "index.mjs").read_text() == "// version two\n" + + def test_a_new_source_file_appears(self, tmp_path, monkeypatch): + def mutate(src): + (src / "brand-new.mjs").write_text("// new helper\n") + + _src, mirror = self._mirror_twice(tmp_path, monkeypatch, mutate) + assert (mirror / "brand-new.mjs").exists() + + def test_the_mirrors_node_modules_survives(self, tmp_path, monkeypatch): + """A refresh must not delete the deps npm installed in the mirror. + + Wiping them turns every image update into a reinstall, on the + installs least able to afford one. + """ + def mutate(src): + (src / "index.mjs").write_text("// version two\n") + + _src, mirror = self._mirror_twice(tmp_path, monkeypatch, mutate) + assert (mirror / "node_modules" / "somepkg" / "index.js").exists() + + +class TestFailureIsNotFatal: + def test_an_unwritable_mirror_falls_back_to_the_source( + self, tmp_path, monkeypatch + ): + """Returning the read-only source loses installs, not the process. + + The caller's own readiness check then reports the real error, which + is more use than a traceback out of a path resolver. + """ + monkeypatch.delenv("HERMES_TESTSC_SIDECAR_DIR", raising=False) + src = _make_sidecar(tmp_path) + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home")) + monkeypatch.setattr( + "gateway.sidecar_runtime.dir_writable", lambda p: False + ) + + def _boom(*a, **k): + raise OSError("read-only file system") + + monkeypatch.setattr("gateway.sidecar_runtime.shutil.copytree", _boom) + assert resolve_sidecar("testsc", src) == src + + +class TestEpochMtimeSources: + """A Nix store source has mtime = epoch on every file. + + copy2 would stamp the mirror's lockfile with that epoch mtime, so npm's + install marker (stamped at install time) always postdates it and + deps_are_current() never reports stale — the exact WhatsApp keep-old- + node_modules bug this resolver exists to fix. The copy must stamp "now". + """ + + def test_a_content_change_with_epoch_mtimes_marks_deps_stale( + self, tmp_path, monkeypatch + ): + monkeypatch.delenv("HERMES_TESTSC_SIDECAR_DIR", raising=False) + src = _make_sidecar(tmp_path) + lock = src / "package-lock.json" + for p in src.rglob("*"): + os.utime(p, (1, 1)) + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home")) + monkeypatch.setattr( + "gateway.sidecar_runtime.dir_writable", lambda p: False + ) + + mirror = resolve_sidecar("testsc", src) + # npm install in the mirror: marker stamped now. + nm = mirror / "node_modules" + nm.mkdir() + (nm / ".package-lock.json").write_text("{}") + + # Upgrade: lockfile CONTENT changes, mtime stays epoch (new store path). + lock.write_text(json.dumps({"lockfileVersion": 3, "v": 2})) + os.utime(lock, (2, 2)) + + mirror = resolve_sidecar("testsc", src) + from gateway.sidecar_runtime import deps_are_current + + assert deps_are_current(mirror) is False, ( + "a refreshed lockfile must postdate the previous npm install, " + "or an upgrade keeps stale node_modules forever" + ) diff --git a/tests/gateway/test_whatsapp_bridge_dir.py b/tests/gateway/test_whatsapp_bridge_dir.py new file mode 100644 index 0000000000000..0a73c71d958a6 --- /dev/null +++ b/tests/gateway/test_whatsapp_bridge_dir.py @@ -0,0 +1,110 @@ +"""WhatsApp's part of the sidecar resolver. + +The rungs are covered by tests/gateway/test_sidecar_runtime.py. What matters +here: the bridge resolves through the shared resolver, under its own name, so +it gains the staleness check and the refresh that its own resolver never had. +""" +from __future__ import annotations + +from pathlib import Path + +from gateway.platforms.whatsapp_common import ( + SOURCE_BRIDGE_DIR, + resolve_whatsapp_bridge_dir, +) + + +def test_override_is_honoured(tmp_path, monkeypatch) -> None: + """An operator can point the gateway at a bridge checkout of their own.""" + override = tmp_path / "bridge" + override.mkdir() + monkeypatch.setenv("HERMES_WHATSAPP_SIDECAR_DIR", str(override)) + assert resolve_whatsapp_bridge_dir() == override + + +def test_it_resolves_under_the_whatsapp_name(tmp_path, monkeypatch) -> None: + """The name picks the mirror directory and the override variable. + + A collision with another sidecar's name would put two sidecars in one + directory. + """ + seen = {} + + def _record(name, source): + seen["name"] = name + seen["source"] = source + return source + + monkeypatch.setattr( + "gateway.sidecar_runtime.resolve_sidecar", _record, raising=True + ) + resolve_whatsapp_bridge_dir() + assert seen["name"] == "whatsapp" + assert seen["source"] == SOURCE_BRIDGE_DIR + + +def test_the_source_dir_holds_the_bridge(monkeypatch) -> None: + """SOURCE_BRIDGE_DIR must point at the shipped bridge, not near it.""" + assert SOURCE_BRIDGE_DIR.name == "whatsapp-bridge" + assert (SOURCE_BRIDGE_DIR / "package.json").is_file() + assert (SOURCE_BRIDGE_DIR / "bridge.js").is_file() + + +def test_a_writable_checkout_runs_in_place(monkeypatch) -> None: + """A source install must keep running the bridge where it ships.""" + monkeypatch.delenv("HERMES_WHATSAPP_SIDECAR_DIR", raising=False) + monkeypatch.setattr( + "gateway.sidecar_runtime.dir_writable", lambda p: True + ) + assert resolve_whatsapp_bridge_dir() == SOURCE_BRIDGE_DIR + + +def test_a_readonly_tree_moves_to_hermes_home(tmp_path, monkeypatch) -> None: + """A read-only install tree must not be where npm runs (#49561). + + In the container image /opt/hermes/scripts/whatsapp-bridge is read-only, + so an install there fails with EACCES. The bridge moves to HERMES_HOME, + which is writable. + + The bridge also gains a staleness check that its own resolver lacked: + the old code returned any existing mirror without comparing it against + the lockfile, so a bridge upgrade kept running the old node_modules. + """ + monkeypatch.delenv("HERMES_WHATSAPP_SIDECAR_DIR", raising=False) + home = tmp_path / "home" + monkeypatch.setenv("HERMES_HOME", str(home)) + monkeypatch.setattr( + "gateway.sidecar_runtime.dir_writable", lambda p: False + ) + resolved = resolve_whatsapp_bridge_dir() + assert resolved == home / "sidecars" / "whatsapp" + assert (resolved / "bridge.js").is_file() + assert not (resolved / "node_modules").exists() + + +def test_the_bridge_source_is_copied_whole(tmp_path, monkeypatch) -> None: + """Every file the bridge ships must reach the mirror. + + bridge.js imports allowlist.js and other siblings, and Node's ESM + resolver reads them from beside the entry file. A partial copy fails at + import, on read-only installs only. + """ + monkeypatch.delenv("HERMES_WHATSAPP_SIDECAR_DIR", raising=False) + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home")) + monkeypatch.setattr( + "gateway.sidecar_runtime.dir_writable", lambda p: False + ) + mirror = resolve_whatsapp_bridge_dir() + + want = { + p.relative_to(SOURCE_BRIDGE_DIR) + for p in SOURCE_BRIDGE_DIR.rglob("*") + if p.is_file() and "node_modules" not in p.parts + } + got = { + p.relative_to(mirror) + for p in mirror.rglob("*") + if p.is_file() and "node_modules" not in p.parts + } + assert want, "no bridge sources found — the fixture is wrong, not the code" + assert want <= got, f"missing from the mirror: {sorted(want - got)}" diff --git a/tests/gateway/test_whatsapp_bridge_dir_resolution.py b/tests/gateway/test_whatsapp_bridge_dir_resolution.py deleted file mode 100644 index b473b731d9f37..0000000000000 --- a/tests/gateway/test_whatsapp_bridge_dir_resolution.py +++ /dev/null @@ -1,60 +0,0 @@ -"""Tests for resolve_whatsapp_bridge_dir() — read-only install tree handling. - -Regression coverage for #49561: in the Docker image the install tree -(/opt/hermes/scripts/whatsapp-bridge) is read-only, so `npm install` fails -with EACCES. The resolver must detect the read-only install dir and mirror the -bridge source into a writable HERMES_HOME location instead. -""" -import importlib -from pathlib import Path - -import pytest - -from gateway.platforms import whatsapp_common - - -def _seed_install_tree(install_bridge: Path) -> None: - """Create a minimal fake bridge source tree.""" - install_bridge.mkdir(parents=True, exist_ok=True) - (install_bridge / "bridge.js").write_text("// bridge\n") - (install_bridge / "package.json").write_text('{"name": "whatsapp-bridge"}\n') - - -def test_readonly_install_mirrors_to_hermes_home(tmp_path, monkeypatch): - """A read-only install tree is mirrored into a writable HERMES_HOME.""" - install_root = tmp_path / "install" - install_bridge = install_root / "scripts" / "whatsapp-bridge" - _seed_install_tree(install_bridge) - - hermes_home = tmp_path / "hermes_home" - hermes_home.mkdir() - - monkeypatch.setattr( - whatsapp_common, "__file__", - str(install_root / "gateway" / "platforms" / "whatsapp_common.py"), - ) - monkeypatch.setattr( - "hermes_constants.get_hermes_home", lambda: hermes_home - ) - - # Simulate a read-only install tree. chmod(0o555) is unreliable under - # root (CI/Docker bypass permission bits), so force the write probe to - # fail by raising on the .write_test touch for the install dir only. - _real_touch = Path.touch - - def _fake_touch(self, *a, **kw): - if self.name == ".write_test" and install_bridge in self.parents: - raise PermissionError("read-only install tree") - return _real_touch(self, *a, **kw) - - monkeypatch.setattr(Path, "touch", _fake_touch) - - resolved = whatsapp_common.resolve_whatsapp_bridge_dir() - - expected = hermes_home / "scripts" / "whatsapp-bridge" - assert resolved == expected - # Source was mirrored, not symlinked. - assert (expected / "bridge.js").read_text() == "// bridge\n" - assert (expected / "package.json").exists() - - diff --git a/tests/plugins/platforms/photon/test_sidecar_paths.py b/tests/plugins/platforms/photon/test_sidecar_paths.py index 66ee07feb6d63..c2ee2fcf99be6 100644 --- a/tests/plugins/platforms/photon/test_sidecar_paths.py +++ b/tests/plugins/platforms/photon/test_sidecar_paths.py @@ -1,9 +1,8 @@ -"""Tests for the Photon sidecar directory resolver (NS-606). +"""Photon's part of the sidecar resolver. -Hosted/managed images keep the plugin tree under an immutable -``/opt/hermes``; ``resolve_sidecar_dir`` must run in place when the deps are -baked and current, and mirror the sidecar to the writable ``HERMES_HOME`` -volume when a runtime install is unavoidable. +The rungs themselves are covered by tests/gateway/test_sidecar_runtime.py. +What is Photon-specific, and tested here: the legacy ``PHOTON_SIDECAR_DIR`` +override, and that neither the adapter nor the CLI resolves at import time. """ from __future__ import annotations @@ -16,93 +15,46 @@ import pytest import plugins.platforms.photon.sidecar_paths as sidecar_paths -def _seed_source(source: Path, *, with_node_modules: bool = False) -> None: - source.mkdir(parents=True, exist_ok=True) - for name in sidecar_paths._MIRROR_FILES: - (source / name).write_text(f"// {name}\n", encoding="utf-8") - if with_node_modules: - (source / "node_modules").mkdir() - (source / "node_modules" / ".package-lock.json").write_text( - "{}", encoding="utf-8" - ) +def test_legacy_override_still_works(tmp_path, monkeypatch) -> None: + """``PHOTON_SIDECAR_DIR`` predates the shared resolver. - -def _freeze_writability(monkeypatch, *, writable: bool) -> None: - monkeypatch.setattr(sidecar_paths, "_dir_writable", lambda _p: writable) - - -def test_env_override_wins(tmp_path, monkeypatch) -> None: + Operators set it, so it keeps working alongside the shared + ``HERMES_PHOTON_SIDECAR_DIR``. + """ override = tmp_path / "custom" + monkeypatch.delenv("HERMES_PHOTON_SIDECAR_DIR", raising=False) monkeypatch.setenv("PHOTON_SIDECAR_DIR", str(override)) assert sidecar_paths.resolve_sidecar_dir(tmp_path / "src") == override -def test_writable_source_runs_in_place(tmp_path, monkeypatch) -> None: - """Dev installs: writable tree keeps today's behavior exactly.""" +def test_shared_override_wins_over_the_legacy_one(tmp_path, monkeypatch) -> None: + """Both set means the operator moved to the shared name. Honour it.""" + shared = tmp_path / "shared" + monkeypatch.setenv("PHOTON_SIDECAR_DIR", str(tmp_path / "legacy")) + monkeypatch.setenv("HERMES_PHOTON_SIDECAR_DIR", str(shared)) + assert sidecar_paths.resolve_sidecar_dir(tmp_path / "src") == shared + + +def test_the_source_dir_is_the_shipped_sidecar(monkeypatch) -> None: + """A resolve with no argument reads the sidecar inside the plugin.""" monkeypatch.delenv("PHOTON_SIDECAR_DIR", raising=False) - source = tmp_path / "src" - _seed_source(source) - _freeze_writability(monkeypatch, writable=True) - assert sidecar_paths.resolve_sidecar_dir(source) == source + monkeypatch.delenv("HERMES_PHOTON_SIDECAR_DIR", raising=False) + seen = {} + def _record(name, source): + seen["name"] = name + seen["source"] = source + return source -def test_readonly_source_with_baked_fresh_deps_runs_in_place( - tmp_path, monkeypatch -) -> None: - """Managed-image happy path: deps baked at build time, no mirror needed.""" - monkeypatch.delenv("PHOTON_SIDECAR_DIR", raising=False) - source = tmp_path / "src" - _seed_source(source, with_node_modules=True) - # Marker newer than lockfile == fresh install. - lock = source / "package-lock.json" - marker = source / "node_modules" / ".package-lock.json" - os.utime(lock, (1000.0, 1000.0)) - os.utime(marker, (2000.0, 2000.0)) - _freeze_writability(monkeypatch, writable=False) - assert sidecar_paths.resolve_sidecar_dir(source) == source - - -def test_mirror_refresh_updates_changed_files_and_keeps_node_modules( - tmp_path, monkeypatch -) -> None: - """Image update changes index.mjs → re-copied; installed deps survive.""" - monkeypatch.delenv("PHOTON_SIDECAR_DIR", raising=False) - home = tmp_path / "home" - monkeypatch.setenv("HERMES_HOME", str(home)) - source = tmp_path / "src" - _seed_source(source) - _freeze_writability(monkeypatch, writable=False) - - mirror = sidecar_paths.resolve_sidecar_dir(source) - # Simulate a completed npm install in the mirror. - (mirror / "node_modules").mkdir() - (mirror / "node_modules" / "installed.txt").write_text("x", encoding="utf-8") - - # Image update rewrites a source file. - (source / "index.mjs").write_text("// index.mjs v2\n", encoding="utf-8") - - resolved = sidecar_paths.resolve_sidecar_dir(source) - - assert resolved == mirror - assert (mirror / "index.mjs").read_text(encoding="utf-8") == "// index.mjs v2\n" - assert (mirror / "node_modules" / "installed.txt").exists() - - -def test_dir_writable_probe(tmp_path) -> None: - assert sidecar_paths.dir_writable(tmp_path) is True - ro = tmp_path / "ro" - ro.mkdir() - ro.chmod(0o555) - try: - if os.geteuid() == 0: # pragma: no cover - root ignores perms - pytest.skip("root bypasses directory permissions") - assert sidecar_paths.dir_writable(ro) is False - finally: - ro.chmod(0o755) + monkeypatch.setattr(sidecar_paths, "resolve_sidecar", _record) + sidecar_paths.resolve_sidecar_dir() + assert seen["name"] == "photon" + assert seen["source"] == sidecar_paths.SOURCE_SIDECAR_DIR + assert seen["source"].name == "sidecar" def test_adapter_import_does_not_resolve_sidecar_dir(monkeypatch) -> None: - """Importing the adapter must not probe the filesystem or mirror files. + """Importing the adapter must not probe the filesystem or copy files. resolve_sidecar_dir() touch/unlink-probes the source tree and may copy files to HERMES_HOME; the adapter and CLI resolve lazily on first use so @@ -137,3 +89,17 @@ def test_adapter_import_does_not_resolve_sidecar_dir(monkeypatch) -> None: monkeypatch.undo() importlib.reload(photon_adapter) importlib.reload(photon_cli) + + +def test_dir_writable_probe(tmp_path) -> None: + """Re-exported for the adapter, which gates npm on it.""" + assert sidecar_paths.dir_writable(tmp_path) is True + ro = tmp_path / "ro" + ro.mkdir() + ro.chmod(0o555) + try: + if os.geteuid() == 0: # pragma: no cover - root ignores perms + pytest.skip("root bypasses directory permissions") + assert sidecar_paths.dir_writable(ro) is False + finally: + ro.chmod(0o755)