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 <noreply@anthropic.com>

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`)"

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
 

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,

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:

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()