mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-18 04:41:56 +00:00
fix(kanban): correct dispatcher spawn module name + PATH-first lookup
Follow-up to the previous commit's contributor cherry-pick.
The cherry-picked change replaced the bare ``["hermes", ...]`` spawn with
``[sys.executable, "-m", "hermes", ...]``. The intent was right (avoid
PATH dependence — cron, systemd User= services, launchd jobs, and other
detached dispatcher invocations routinely run with a stripped $PATH that
doesn't include the venv's bin/, breaking the bare-shim spawn) but the
module name is wrong: there is no top-level ``hermes`` package. The
console-script entry point in pyproject.toml is
``hermes = "hermes_cli.main:main"``, and ``python -m hermes`` fails with
``No module named hermes``. The cherry-picked form would have replaced a
sometimes-broken spawn with an always-broken one.
This commit:
- Adds ``_resolve_hermes_argv()``, mirroring ``gateway.run._resolve_hermes_bin``.
Tries ``shutil.which("hermes")`` first (preferred — keeps existing ``ps``
output and log lines familiar in the common case) and falls back to
``[sys.executable, "-m", "hermes_cli.main"]`` when the shim is not on
PATH. The fallback goes through the running interpreter so it's
PATH-independent. Kept as a local helper rather than imported from
gateway because ``hermes_cli`` sits below ``gateway`` in the dependency
order.
- Switches the dispatcher's ``cmd`` list to use ``*_resolve_hermes_argv()``.
- Adds three regression tests:
* ``test_resolve_hermes_argv_prefers_path_shim`` — pins the PATH-first
branch so a future refactor doesn't silently flip the order.
* ``test_resolve_hermes_argv_falls_back_to_module_form_when_no_path_shim`` —
pins the correct module name (``hermes_cli.main``, NOT ``hermes``).
Direct regression guard for the form that shipped in the original PR.
* ``test_resolve_hermes_argv_module_actually_runs`` — runs the fallback
invocation as a real subprocess and asserts ``--version`` works, so
losing ``hermes_cli.main``'s ``__main__`` handling can't slip past the
string-match test.
Verified end-to-end: with the shim on PATH the resolver returns
``[/.../hermes]`` and ``--version`` works; with the shim removed the
resolver returns ``[python, -m, hermes_cli.main]`` and ``--version``
still works; the original PR's ``python -m hermes`` invocation fails as
expected (``No module named hermes``).
This commit is contained in:
parent
d3db6724dd
commit
62b1c74cbc
2 changed files with 95 additions and 1 deletions
|
|
@ -3749,6 +3749,35 @@ def _rotate_worker_log(log_path: Path, max_bytes: int) -> None:
|
||||||
pass
|
pass
|
||||||
|
|
||||||
|
|
||||||
|
def _resolve_hermes_argv() -> list[str]:
|
||||||
|
"""Resolve the ``hermes`` invocation as argv parts for ``Popen``.
|
||||||
|
|
||||||
|
Tries in order:
|
||||||
|
|
||||||
|
1. ``shutil.which("hermes")`` — the console-script shim, the same form
|
||||||
|
that shows up in ``ps`` output and existing logs. Preferred so live
|
||||||
|
systems' diagnostics stay familiar.
|
||||||
|
2. ``sys.executable -m hermes_cli.main`` — fallback for setups where
|
||||||
|
Hermes is launched from a venv and the ``hermes`` shim is not on
|
||||||
|
the dispatcher's ``$PATH`` (cron, systemd ``User=`` services,
|
||||||
|
launchd jobs, detached processes, etc.). Goes through the running
|
||||||
|
interpreter so the result is independent of ``$PATH``.
|
||||||
|
|
||||||
|
Mirrors ``gateway.run._resolve_hermes_bin`` for the same reason. Kept
|
||||||
|
local (not imported from gateway) because ``hermes_cli`` sits below
|
||||||
|
``gateway`` in the dependency order.
|
||||||
|
"""
|
||||||
|
import shutil
|
||||||
|
|
||||||
|
hermes_bin = shutil.which("hermes")
|
||||||
|
if hermes_bin:
|
||||||
|
return [hermes_bin]
|
||||||
|
# Fallback to the module form. ``hermes_cli.main`` is the actual
|
||||||
|
# console-script target declared in pyproject.toml, NOT a top-level
|
||||||
|
# ``hermes`` package — there is no ``hermes`` package to import.
|
||||||
|
return [sys.executable, "-m", "hermes_cli.main"]
|
||||||
|
|
||||||
|
|
||||||
def _default_spawn(
|
def _default_spawn(
|
||||||
task: Task,
|
task: Task,
|
||||||
workspace: str,
|
workspace: str,
|
||||||
|
|
@ -3805,7 +3834,7 @@ def _default_spawn(
|
||||||
env["HERMES_PROFILE"] = profile_arg
|
env["HERMES_PROFILE"] = profile_arg
|
||||||
|
|
||||||
cmd = [
|
cmd = [
|
||||||
sys.executable, "-m", "hermes",
|
*_resolve_hermes_argv(),
|
||||||
"-p", profile_arg,
|
"-p", profile_arg,
|
||||||
# Auto-load the kanban-worker skill so every dispatched worker
|
# Auto-load the kanban-worker skill so every dispatched worker
|
||||||
# has the pattern library (good summary/metadata shapes, retry
|
# has the pattern library (good summary/metadata shapes, retry
|
||||||
|
|
|
||||||
|
|
@ -1199,3 +1199,68 @@ def test_migrate_add_optional_columns_tolerates_concurrent_migration(kanban_home
|
||||||
# Running migration on an already-migrated schema must not raise.
|
# Running migration on an already-migrated schema must not raise.
|
||||||
kb._migrate_add_optional_columns(conn)
|
kb._migrate_add_optional_columns(conn)
|
||||||
conn.close()
|
conn.close()
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Dispatcher spawn invocation — _resolve_hermes_argv()
|
||||||
|
#
|
||||||
|
# Workers spawned by the dispatcher must use a `hermes` invocation that does
|
||||||
|
# not depend on PATH being set up correctly. cron jobs, systemd User= services,
|
||||||
|
# launchd jobs, and other detached processes routinely run with a stripped
|
||||||
|
# $PATH that doesn't include the venv's bin/, so a bare `["hermes", ...]`
|
||||||
|
# spawn fails with FileNotFoundError and the task gets stuck. The resolver
|
||||||
|
# prefers the PATH shim (familiar `ps` output) but falls back to the module
|
||||||
|
# form so the spawn keeps working when PATH is missing the shim.
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def test_resolve_hermes_argv_prefers_path_shim(monkeypatch):
|
||||||
|
"""When `hermes` is on PATH, use the shim — preserves familiar ps output."""
|
||||||
|
import shutil
|
||||||
|
import hermes_cli.kanban_db as kb
|
||||||
|
|
||||||
|
monkeypatch.setattr(shutil, "which", lambda name: "/usr/local/bin/hermes")
|
||||||
|
argv = kb._resolve_hermes_argv()
|
||||||
|
assert argv == ["/usr/local/bin/hermes"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_resolve_hermes_argv_falls_back_to_module_form_when_no_path_shim(monkeypatch):
|
||||||
|
"""When the shim is not on PATH, fall back to `python -m hermes_cli.main`.
|
||||||
|
|
||||||
|
Pins the correct module name (NOT `hermes` — there is no top-level
|
||||||
|
`hermes` package). Regression for #23198: the original PR shipped
|
||||||
|
`python -m hermes` which fails with `No module named hermes` on every
|
||||||
|
invocation.
|
||||||
|
"""
|
||||||
|
import shutil
|
||||||
|
import sys
|
||||||
|
import hermes_cli.kanban_db as kb
|
||||||
|
|
||||||
|
monkeypatch.setattr(shutil, "which", lambda name: None)
|
||||||
|
argv = kb._resolve_hermes_argv()
|
||||||
|
assert argv == [sys.executable, "-m", "hermes_cli.main"]
|
||||||
|
|
||||||
|
|
||||||
|
def test_resolve_hermes_argv_module_actually_runs():
|
||||||
|
"""The fallback module name must be importable + runnable.
|
||||||
|
|
||||||
|
A unit test that pins the literal string is necessary but not
|
||||||
|
sufficient — if `hermes_cli.main` ever loses `if __name__ == "__main__"`
|
||||||
|
handling or its argparse setup, `python -m hermes_cli.main --version`
|
||||||
|
would fail and so would every dispatcher spawn that hits the fallback.
|
||||||
|
Run it as a real subprocess to catch that regression.
|
||||||
|
"""
|
||||||
|
import subprocess
|
||||||
|
import sys
|
||||||
|
import hermes_cli.kanban_db as kb
|
||||||
|
import shutil
|
||||||
|
import unittest.mock as mock
|
||||||
|
|
||||||
|
with mock.patch.object(shutil, "which", return_value=None):
|
||||||
|
argv = kb._resolve_hermes_argv()
|
||||||
|
r = subprocess.run(argv + ["--version"], capture_output=True, text=True, timeout=30)
|
||||||
|
assert r.returncode == 0, (
|
||||||
|
f"`{' '.join(argv)} --version` failed (rc={r.returncode}); "
|
||||||
|
f"stderr={r.stderr[:200]!r}"
|
||||||
|
)
|
||||||
|
assert "Hermes Agent" in r.stdout, f"unexpected output: {r.stdout[:200]!r}"
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue