fix(gateway): correct macOS gateway-pid detection (#15225) - #15318
fix(gateway): correct macOS gateway-pid detection (#15225)#15318briandevans wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes macOS gateway PID detection so hermes cron list (and other callers of find_gateway_pids()) no longer incorrectly warns that the Gateway isn’t running when it is managed by launchd.
Changes:
- Add a launchd output parser that correctly handles both
launchctl listoutput shapes (plist-dict vs tabular). - Replace the non-portable
ps ... eww ...invocation with a portableps -A -ww ...form. - Add targeted regression tests for the macOS launchctl parsing and the portable
psinvocation (and update an existing test expectation).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
hermes_cli/gateway.py |
Fixes launchctl parsing via a new helper and updates the ps invocation for portability/security. |
tests/hermes_cli/test_gateway_pid_detection_macos.py |
Adds regression tests covering both launchctl output formats and the corrected ps argv. |
tests/hermes_cli/test_gateway.py |
Updates an existing test to expect the new portable ps invocation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pids: set = set() | ||
| if not stdout: | ||
| return pids |
There was a problem hiding this comment.
The local variable annotation uses an unparameterized set. Since this set is intended to contain integer PIDs, consider annotating it as set[int] for consistency with other PID collections in this module.
| from types import SimpleNamespace | ||
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
|
|
||
| from hermes_cli import gateway |
There was a problem hiding this comment.
patch is imported but not used in this test module. Removing the unused import will keep the file clean and avoid unused-import warnings in environments that run linting or stricter test settings.
| def _parse_launchd_list_output(stdout: str, label: str) -> set: | ||
| """Extract PIDs for ``label`` from ``launchctl list`` output. |
There was a problem hiding this comment.
Type annotations here are less specific than the rest of this module: the helper returns a set of PIDs, so the return type should be set[int] (consistent with e.g. seen: set[int] = set() in this file) to improve readability and static analysis.
|
Thanks @copilot — all three nits addressed in
39/39 tests still pass locally — no behaviour change. |
``hermes cron list`` falsely reports "Gateway is not running" on macOS even when launchd has the service loaded and cron jobs fire correctly. Two independent bugs in ``find_gateway_pids()`` each drop macOS into an empty-result path; fixing either alone would clear the warning, but both are real and both are worth fixing. ### Bug 1 — ``_get_service_pids()`` mis-parses ``launchctl list <label>`` ``launchctl list`` returns two different formats depending on whether a label is passed: * No label → tab-separated table ``PID\\tStatus\\tLabel`` * With label → plist-dict dump, e.g. ``"PID" = 855;`` The old code always called ``launchctl list <label>`` (the plist-dict path) but parsed with ``string.split()`` expecting the tab-separated format. ``parts[2]`` on ``"Label" = "ai.hermes.gateway";`` is ``'"ai.hermes.gateway";'`` — quoted, semicolon'd — so the ``== label`` comparison never matched and no PID was ever extracted. Fix: extracted a ``_parse_launchd_list_output(stdout, label)`` helper that tries the plist-dict ``"PID" = N;`` regex first and falls back to the tab-separated path when no plist matches were found. Handles both formats so a future change to the caller (passing or not passing the label) can't re-break detection. Regex anchored to the ``"PID"`` key so sibling fields like ``LastExitStatus`` can't match; PID 0 rejected so downstream ``os.kill(0, ...)`` never sees it. ### Bug 2 — ``_scan_gateway_pids()`` passes ``eww`` to ``ps`` The old invocation ``ps -A eww -o pid=,command=`` has two problems: * **Darwin** rejects ``eww`` as "illegal argument" and exits 1 — ``stdout`` is empty, the parse loop iterates nothing, and no PID is extracted (NousResearch#15225). * **FreeBSD** accepts ``eww`` but the ``e`` attaches environment variables to the command column, so ``split(None, 1)`` picks up the first env var as the command (NousResearch#9069). The env vars can include API keys and tokens, which also leaked them into any log line that echoed the command. Fix: replaced with ``ps -A -ww -o pid=,command=`` — portable across Linux (procps), Darwin, FreeBSD, and busybox. Drops env-var leakage as a side benefit. ### Tests (19 new + 1 updated, all passing) ``tests/hermes_cli/test_gateway_pid_detection_macos.py``: * **``TestParseLaunchdListOutput``** (8 cases) — exercises the new helper directly with the exact plist-dict output from the NousResearch#15225 repro, the tab-separated format, the dash-PID unloaded case, mixed-shape defensive input, PID=0 rejection, and 4 whitespace-variant tolerance cases for the PID regex (``"PID" =``, ``"PID"=``, ``"PID" = ``, leading tabs). * **``TestGetServicePidsMacOS``** (3 cases) — end-to-end macOS branch with ``subprocess.run`` patched to return the plist-dict payload; asserts the single-PID set is returned, a non-zero launchctl exit returns empty, and a missing ``launchctl`` binary (FileNotFoundError) falls through cleanly. * **``TestPsInvocationPortability``** (4 cases) — captures the exact argv the production code passes to ``ps``, asserts ``"eww"`` is never on the command line, pins the ``["ps", "-A", "-ww", "-o", "pid=,command="]`` shape, parses a realistic Darwin-style ps sample end-to-end, and exercises the nonzero-returncode fallback. Also updated ``test_find_gateway_pids_falls_back_to_pid_file_when_process_scan_fails`` in ``tests/hermes_cli/test_gateway.py`` to match the new portable ``-ww`` invocation; added an inline comment pointing at NousResearch#15225 history. **Verified tests are real regression guards**: temporarily reverted Bug 1 and Bug 2 independently; the relevant test classes correctly failed with clear messages pointing at the regressed invariant. Closes NousResearch#15225 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three minor cleanups from Copilot's review on NousResearch#15318: 1. ``_parse_launchd_list_output`` return type: ``set`` → ``set[int]`` (matches the ``seen: set[int] = set()`` style elsewhere in the module). 2. ``_get_service_pids`` return type + local: same parameterization; was bare ``set`` before this branch existed, but Copilot flagged the pre-existing annotation while reviewing the diff so worth tightening as a drive-by. 3. ``unittest.mock.patch`` was imported in ``test_gateway_pid_detection_macos.py`` but never called — the tests use ``monkeypatch`` exclusively. Removed the unused import. No behaviour change. 39/39 tests still pass locally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
0344959 to
7555352
Compare
|
Closing to keep the queue clean — 17 days idle and now conflicting on hermes_cli/gateway.py. Happy to reopen if this is still useful. |
What does this PR do?
Fixes `#15225`. `hermes cron list` falsely reports "Gateway is not running" on macOS even when launchd has the service loaded and cron jobs fire correctly.
Two independent bugs in `hermes_cli/gateway.py` each drop the macOS PID-detection path into an empty-result state. Fixing either alone clears the warning; both are real and both are worth fixing — the detector is the source for several other views (`cron list` warning, secondary `gateway status` check, `hermes update`'s broad sweep) and each should be resilient.
Bug 1 — `_get_service_pids()` mis-parses `launchctl list `
macOS `launchctl list` has two output formats:
The old code always called `launchctl list ` but parsed with `string.split()` expecting the tab-separated format. On a real macOS install that means `parts[2]` on the label line is `'"ai.hermes.gateway";'` (quoted, semicolon'd) — so `parts[2] == label` never matched and no PID was ever extracted, even though the service was actively running.
Fix: extracted a `_parse_launchd_list_output(stdout, label)` helper that tries the plist-dict `"PID" = N;` regex first and falls back to the tab-separated path when no plist matches are found. Handles both formats so a future change to the caller can't silently re-break detection.
Bug 2 — `_scan_gateway_pids()` passes `eww` to `ps`
Old: `["ps", "-A", "eww", "-o", "pid=,command="]`
hermes cron listfalsely reports "Gateway is not running" on macOS (two-stage detection failure in find_gateway_pids) #15225).ps ewwoutput format #9069). Env vars can include API keys — leaking them into any log line that echoes the command.Fix: replaced with `["ps", "-A", "-ww", "-o", "pid=,command="]` — portable across Linux (procps), Darwin, FreeBSD, busybox. Drops env-var leakage as a side benefit.
Related Issue
Fixes #15225
Related: #9069 (same function, different FreeBSD failure mode), #9723 (same function, Docker, fixed via `c483b4c` for that narrower case).
Type of Change
Test plan
hermes cron listfalsely reports "Gateway is not running" on macOS (two-stage detection failure in find_gateway_pids) #15225` comment for future readers)Test coverage detail
`TestParseLaunchdListOutput` (8 cases) — exercises the new helper directly:
hermes cron listfalsely reports "Gateway is not running" on macOS (two-stage detection failure in find_gateway_pids) #15225 repro → extracts PID 855`TestGetServicePidsMacOS` (3 cases) — end-to-end macOS branch with mocked `subprocess.run`:
hermes cron listfalsely reports "Gateway is not running" on macOS (two-stage detection failure in find_gateway_pids) #15225 repro state)`TestPsInvocationPortability` (4 cases) — captures exact argv:
Not in scope