Skip to content

fix(photon): resolve the Hermes-managed Node for the sidecar - #76804

Open
Drexuxux wants to merge 1 commit into
NousResearch:mainfrom
Drexuxux:drex/photon-managed-node
Open

fix(photon): resolve the Hermes-managed Node for the sidecar#76804
Drexuxux wants to merge 1 commit into
NousResearch:mainfrom
Drexuxux:drex/photon-managed-node

Conversation

@Drexuxux

@Drexuxux Drexuxux commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

What

On an install whose Node is the Hermes-managed one, the Photon platform never works — and stays broken on every run.

$HERMES_HOME/node is not on an arbitrary process's PATH; only generated service units get it prepended. Photon resolved node and npm with bare shutil.which() at five sites, so all five resolved nothing:

site effect
adapter.py check_requirements() photon reports unavailable — the gateway never creates the adapter
adapter.py npm gate + _reinstall_sidecar_deps() gives up before running npm; deps never installed
adapter.py _node_bin spawn falls back to the literal "node"FileNotFoundError
cli.py install-sidecar refuses to run: "npm is not on PATH"
cli.py status prints ✗ missing (install Node 18+)

That is exactly the population the installer provisions a managed Node for: system Node/npm missing, or below the engines floor.

Reproduced with a managed tree present and no system Node on PATH:

managed node dirs       : ['<home>/node', '<home>/node/bin']
managed node present    : True

adapter.py -> _node_bin : 'node'
  spawn                 -> FileNotFoundError
cli.py install-sidecar  : npm is not on PATH -> exit 1
cli.py status           : 'missing (install Node 18+)'

25d0bcd42 fixed this class across 11 files and tests/test_managed_runtime_resolution.py guards it, but that guard's _EXEMPT_DIRS skips plugins/ — so the platform adapters were never swept.

Fix

Route the five sites through hermes_constants.find_node_executable(): managed tree first, PATH kept as the fallback rung, None when a managed tree exists but cannot be healed (never silently a system copy).

npm also needs the managed dirs on PATH, not just an absolute path — npm's launcher runs under env node, and the sidecar shells out to node too. The npm runs and the sidecar spawn now use with_hermes_node_path().

An explicit PHOTON_NODE_BIN stays authoritative and is still validated.

Tests

tests/plugins/platforms/photon/test_managed_node_resolution.py — 11 tests. PATH is pinned empty in each one, so the managed rung decides the outcome rather than whatever the host has installed:

  • check_requirements() accepts a managed Node; still False with no Node anywhere
  • the sidecar spawns the managed interpreter
  • reinstall and install-sidecar run the managed npm, with the managed dir on the child's PATH
  • PHOTON_NODE_BIN still wins; a broken override is still a hard failure
  • the env overlay keeps the caller's existing variables
scripts/run_tests.sh tests/plugins/platforms/photon/test_managed_node_resolution.py
=== Summary: 1 files, 11 tests passed, 0 failed ===

# without the fix
=== Summary: 1 files, 6 tests passed, 5 failed ===

Two existing tests were updated rather than worked around:

  • test_inbound.py::test_check_requirements_without_node encoded the old "not on PATH → refuse" contract; it now stubs the managed resolver too, preserving its intent — no Node anywhere → refuse.
  • test_npm_error_log_regression.py patched cli_mod.shutil.which, which cli.py no longer uses; the 7 patch lines now target the resolver. Assertions unchanged.

Broader run — tests/plugins/: 1077 passed, 9 failed, the same 9 that fail on a clean checkout. Baseline verified by stashing the change: 1066 passed, 9 failed — same set, zero new failures.

$HERMES_HOME/node is never on an arbitrary process's PATH — only generated
service units get it prepended — so on an install whose Node is the managed
one, every bare shutil.which() in the photon platform resolved nothing:

  * check_requirements() reported photon unavailable, so the gateway never
    created the adapter at all
  * _reinstall_sidecar_deps() gave up before running npm
  * `hermes photon install-sidecar` refused to run
  * the spawn fell back to the literal "node" and died with FileNotFoundError
    on every connect

That is precisely the population the installer provisions a managed Node
for: system Node/npm missing, or below the engines floor.

Route the five call sites through hermes_constants.find_node_executable(),
which checks the managed tree first and keeps PATH as the fallback rung, and
run npm (plus the sidecar itself) with the managed directories ahead on PATH
so npm's own `env node` launcher resolves too. An explicit PHOTON_NODE_BIN
stays authoritative.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins area/install-update Installer, updater, packaging, wheels, doctor sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 2, 2026
@teknium1

teknium1 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for tracing the managed-runtime gap across the Photon adapter and CLI. The premise is confirmed on current main: plugins/platforms/photon/adapter.py:385, :406, :461, and :715 still rely on PATH-only resolution, while plugins/platforms/photon/cli.py:388 and :445 do the same. This conflicts with the established resolver contract in hermes_constants.py:653-679, which prefers the managed runtime and supplies its PATH to child processes.

The PR applies that existing mechanism at the affected sites, preserves the explicit PHOTON_NODE_BIN branch, and adds managed-path coverage for dependency installation and sidecar spawning. Remote main is only two unrelated commits beyond the PR base, so this should be mechanically salvageable.

Automated hermes-sweeper review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants