From 9cf20460815b5b6d24e5a2eb6f4cbd77a26c693c Mon Sep 17 00:00:00 2001 From: joaomarcos Date: Tue, 21 Jul 2026 01:49:59 -0300 Subject: [PATCH] fix(photon): unify sidecar-deps check, harden log unlink and truncation Addresses teknium1 review on #50983: - cli.py's `hermes photon status` and adapter.py's _start_sidecar() still used the old node_modules/-existence check while check_requirements() had moved to a spectrum-ts content check. Extracted the check into a shared sidecar_deps_installed(), used by all three, so an empty/partial node_modules/ (aborted npm install) is rejected consistently instead of only in check_requirements(). - _install_sidecar()'s success-path _NPM_ERROR_LOG.unlink() only caught FileNotFoundError, so a PermissionError/OSError on a locked file would propagate. Broadened to OSError. - npm stderr was truncated only when read back in check_requirements(); an unbounded stderr was written to disk on every failed install. Now truncated to _NPM_ERROR_LOG_MAX_CHARS before write_text(). Co-Authored-By: Claude Sonnet 5 --- plugins/platforms/photon/adapter.py | 20 +++++++-- plugins/platforms/photon/cli.py | 9 ++-- .../photon/test_check_requirements_risks.py | 28 ++++++++++++ .../photon/test_npm_error_log_regression.py | 45 +++++++++++++++++++ .../photon/test_sidecar_lifecycle.py | 20 ++++++++- 5 files changed, 115 insertions(+), 7 deletions(-) diff --git a/plugins/platforms/photon/adapter.py b/plugins/platforms/photon/adapter.py index 1ec24e1d3743..535eb9898a1f 100644 --- a/plugins/platforms/photon/adapter.py +++ b/plugins/platforms/photon/adapter.py @@ -87,6 +87,7 @@ _FFFC_WAIT_SECONDS = 15.0 # Timeout for waiting on an attachment after a U+FFFC _SIDECAR_DIR = Path(__file__).parent / "sidecar" _NPM_ERROR_LOG = _SIDECAR_DIR / ".photon-npm-error.log" +_NPM_ERROR_LOG_MAX_CHARS = 300 # Cap on a self-heal `npm ci`/`npm install` of the sidecar deps. A cold # install of the pinned spectrum-ts tree normally takes well under a minute; @@ -132,6 +133,19 @@ def _coerce_port(value: Any, default: int) -> int: return default +def sidecar_deps_installed() -> bool: + """True when spectrum-ts is present under node_modules/. + + Checks the dependency's own directory, not just node_modules/'s + existence: npm creates node_modules/ before aborting on ENOSPC, a + network timeout, or EACCES, so an empty/partial node_modules/ would + otherwise read as "installed". Shared by check_requirements(), + _start_sidecar(), and `hermes photon status` so all three agree on + what "installed" means. + """ + return (_SIDECAR_DIR / "node_modules" / "spectrum-ts").exists() + + def check_requirements() -> bool: """Return True when both Python deps and the Node sidecar are available.""" if not HTTPX_AVAILABLE: @@ -143,7 +157,7 @@ def check_requirements() -> bool: os.getenv("PHOTON_NODE_BIN") or "node", ) return False - if not (_SIDECAR_DIR / "node_modules" / "spectrum-ts").exists(): + if not sidecar_deps_installed(): # spectrum-ts not installed yet, or node_modules/ was partially created # by an aborted npm install (ENOSPC, network timeout, EACCES). # Checking spectrum-ts presence — not just node_modules/ existence — @@ -157,7 +171,7 @@ def check_requirements() -> bool: npm_error = "" try: if _NPM_ERROR_LOG.exists(): - npm_error = _NPM_ERROR_LOG.read_text(encoding="utf-8").strip()[:300] + npm_error = _NPM_ERROR_LOG.read_text(encoding="utf-8").strip()[:_NPM_ERROR_LOG_MAX_CHARS] except OSError: pass if npm_error: @@ -1011,7 +1025,7 @@ class PhotonAdapter(BasePlatformAdapter): ) async def _start_sidecar(self) -> None: - if not (_SIDECAR_DIR / "node_modules").exists(): + if not sidecar_deps_installed(): raise RuntimeError( f"Photon sidecar deps not installed. Run: " f"cd {_SIDECAR_DIR} && npm install (or `hermes photon setup`)" diff --git a/plugins/platforms/photon/cli.py b/plugins/platforms/photon/cli.py index 9a2095f494ff..871e43a595ad 100644 --- a/plugins/platforms/photon/cli.py +++ b/plugins/platforms/photon/cli.py @@ -29,6 +29,7 @@ from pathlib import Path from hermes_cli.colors import Colors, color from . import auth as photon_auth +from .adapter import _NPM_ERROR_LOG_MAX_CHARS, sidecar_deps_installed _SIDECAR_DIR = Path(__file__).parent / "sidecar" # Written on npm failure so check_requirements() can surface the root cause @@ -362,7 +363,7 @@ def _cmd_status(_args: argparse.Namespace) -> int: # cli.py keeps zero taint flow according to CodeQL. photon_auth.print_credential_summary(print) node_bin = os.getenv("PHOTON_NODE_BIN") or shutil.which("node") - sidecar_installed = (_SIDECAR_DIR / "node_modules").exists() + sidecar_installed = sidecar_deps_installed() print(f" node binary : {node_bin or '✗ missing (install Node 18+)'}") print(f" sidecar deps : {'✓ installed' if sidecar_installed else '✗ run `hermes photon install-sidecar`'}") print(f" telemetry : {'on' if _telemetry_enabled() else 'off'} (`hermes photon telemetry on|off`)") @@ -460,7 +461,9 @@ def _install_sidecar() -> int: print(proc.stderr, end="", file=sys.stderr) if proc.returncode != 0: print("npm install failed", file=sys.stderr) - error = (proc.stderr or "").strip() + # Bound to the same length check_requirements() truncates to on + # read, so the log file never holds more than what's ever surfaced. + error = (proc.stderr or "").strip()[:_NPM_ERROR_LOG_MAX_CHARS] if error: try: _NPM_ERROR_LOG.write_text(error, encoding="utf-8") @@ -469,7 +472,7 @@ def _install_sidecar() -> int: else: try: _NPM_ERROR_LOG.unlink() - except FileNotFoundError: + except OSError: pass return proc.returncode diff --git a/tests/plugins/platforms/photon/test_check_requirements_risks.py b/tests/plugins/platforms/photon/test_check_requirements_risks.py index a387da9f7444..5bcd38921881 100644 --- a/tests/plugins/platforms/photon/test_check_requirements_risks.py +++ b/tests/plugins/platforms/photon/test_check_requirements_risks.py @@ -221,6 +221,34 @@ def test_fix_risk3_error_log_cleared_on_success( ) +# --------------------------------------------------------------------------- +# Shared predicate — status / _start_sidecar / check_requirements must agree +# --------------------------------------------------------------------------- + + +def test_cli_status_shares_adapter_sidecar_deps_check(tmp_path: Path) -> None: + """`hermes photon status` must use the exact same spectrum-ts check as + check_requirements() / _start_sidecar() — not a separate node_modules-only + existence check that would disagree on a partial/empty install.""" + assert cli_mod.sidecar_deps_installed is adapter_mod.sidecar_deps_installed + + +def test_sidecar_deps_installed_false_on_empty_node_modules( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.setattr(adapter_mod, "_SIDECAR_DIR", tmp_path) + (tmp_path / "node_modules").mkdir() # empty — spectrum-ts absent + assert adapter_mod.sidecar_deps_installed() is False + + +def test_sidecar_deps_installed_true_with_spectrum_ts( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.setattr(adapter_mod, "_SIDECAR_DIR", tmp_path) + (tmp_path / "node_modules" / "spectrum-ts").mkdir(parents=True) + assert adapter_mod.sidecar_deps_installed() is True + + @_requires_node def test_fix_risk3_check_requirements_surfaces_npm_error_in_debug_log( monkeypatch: pytest.MonkeyPatch, diff --git a/tests/plugins/platforms/photon/test_npm_error_log_regression.py b/tests/plugins/platforms/photon/test_npm_error_log_regression.py index f4c308073c7d..6a280c6226b8 100644 --- a/tests/plugins/platforms/photon/test_npm_error_log_regression.py +++ b/tests/plugins/platforms/photon/test_npm_error_log_regression.py @@ -152,6 +152,51 @@ def test_regression_empty_stderr_does_not_write_log( # 5. proc.stderr is None — no AttributeError # --------------------------------------------------------------------------- +def test_regression_permissionerror_on_success_unlink_does_not_propagate( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A successful npm install must still return 0 even if deleting the + stale _NPM_ERROR_LOG raises something other than FileNotFoundError + (e.g. PermissionError on a locked file) — the unlink is best-effort.""" + error_log = tmp_path / ".photon-npm-error.log" + + class _UnremovablePath(type(error_log)): + def unlink(self, *a, **kw): + raise PermissionError("access denied") + + monkeypatch.setattr(cli_mod.shutil, "which", lambda _: "/usr/bin/npm") + monkeypatch.setattr( + cli_mod.subprocess, "run", + lambda cmd, **kw: types.SimpleNamespace(returncode=0, stderr=""), + ) + monkeypatch.setattr(cli_mod, "_NPM_ERROR_LOG", _UnremovablePath(error_log)) + + rc = cli_mod._install_sidecar() + assert rc == 0 # PermissionError on cleanup must not fail the install + + +def test_regression_long_stderr_truncated_before_write( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A huge npm stderr must be bounded before it hits disk, not just when + read back later — otherwise a verbose npm failure writes an unbounded + file to the sidecar directory on every retry.""" + error_log = tmp_path / ".photon-npm-error.log" + huge_stderr = "npm ERR! " + ("x" * 10_000) + + monkeypatch.setattr(cli_mod.shutil, "which", lambda _: "/usr/bin/npm") + monkeypatch.setattr( + cli_mod.subprocess, "run", + lambda cmd, **kw: types.SimpleNamespace(returncode=1, stderr=huge_stderr), + ) + monkeypatch.setattr(cli_mod, "_NPM_ERROR_LOG", error_log) + + cli_mod._install_sidecar() + + written = error_log.read_text(encoding="utf-8") + assert len(written) <= cli_mod._NPM_ERROR_LOG_MAX_CHARS + + def test_regression_none_stderr_does_not_crash( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: diff --git a/tests/plugins/platforms/photon/test_sidecar_lifecycle.py b/tests/plugins/platforms/photon/test_sidecar_lifecycle.py index cfa3e52dff86..3838cc2bdb0d 100644 --- a/tests/plugins/platforms/photon/test_sidecar_lifecycle.py +++ b/tests/plugins/platforms/photon/test_sidecar_lifecycle.py @@ -134,7 +134,7 @@ async def test_start_sidecar_spawns_with_stdin_pipe( pass monkeypatch.setattr(adapter, "_reap_stale_sidecar", _no_reap) - (tmp_path / "node_modules").mkdir() + (tmp_path / "node_modules" / "spectrum-ts").mkdir(parents=True) monkeypatch.setattr(photon_adapter, "_SIDECAR_DIR", tmp_path) spawned: Dict[str, Any] = {} @@ -188,3 +188,21 @@ async def test_start_sidecar_spawns_with_stdin_pipe( assert kwargs["env"]["PHOTON_SIDECAR_WATCH_STDIN"] == "1" assert spawned["patch_kwargs"]["creationflags"] == hidden_flags assert kwargs["creationflags"] == hidden_flags + + +@pytest.mark.asyncio +async def test_start_sidecar_rejects_empty_node_modules( + monkeypatch: pytest.MonkeyPatch, tmp_path +) -> None: + """A partial/aborted npm install leaves an empty node_modules/ behind. + + _start_sidecar() must reject it the same way check_requirements() does + (both go through sidecar_deps_installed()) instead of spawning a sidecar + that immediately crashes on a missing spectrum-ts module. + """ + adapter = _make_adapter(monkeypatch) + (tmp_path / "node_modules").mkdir() # empty — spectrum-ts absent + monkeypatch.setattr(photon_adapter, "_SIDECAR_DIR", tmp_path) + + with pytest.raises(RuntimeError, match="not installed"): + await adapter._start_sidecar()