fix(doctor): detect agent-browser in the Hermes-managed node bin (#53192) - #53205
Conversation
…etup-browser installs aren't reported as missing (NousResearch#53192) `hermes acp --setup-browser` installs agent-browser into the Hermes-managed node prefix (~/.hermes/node/bin/agent-browser), which isn't necessarily on PATH. doctor only checked PROJECT_ROOT/node_modules and PATH (shutil.which), so it false-negatived with "agent-browser not installed" even though the binary was present and runnable. Mirror dep_ensure._has_hermes_agent_browser() by also checking HERMES_HOME/node/bin and the legacy HERMES_HOME/node_modules/.bin path, each gated by agent_browser_runnable(). Tested with tests/hermes_cli/test_doctor.py (added positive + not-runnable cases) and pytest tests/hermes_cli/test_doctor.py -q (66 passed).
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real current-main diagnostic gap: hermes_cli/doctor.py:1556-1580 still checks only the project install and PATH, while ACP setup routes through ensure_dependency("browser") (acp_adapter/entry.py:189-211) and the POSIX installer places the executable at $HERMES_HOME/node/bin/agent-browser (scripts/install.sh:2479-2503).
Problems
- The new candidates omit the Windows managed layout.
scripts/install.ps1:363-371installs under$HERMES_HOME/node, andscripts/install.ps1:389-395invokes$HERMES_HOME/node/agent-browser.cmd. The proposed POSIX-only candidates would leavehermes doctorfalse-negative on that installation.hermes_cli/dep_ensure.py:60-70already distinguishes this.cmdlocation.
Suggested changes
- Add and validate the Windows
.cmdmanaged candidate, and cover it with a doctor regression test.
Automated hermes-sweeper review.
| # `hermes acp --setup-browser` installs agent-browser into the | ||
| # Hermes-managed node prefix, which isn't necessarily on PATH. Mirror | ||
| # dep_ensure._has_hermes_agent_browser() so doctor and dep_ensure agree | ||
| # on what "installed" means; otherwise doctor false-negatives (#53192). |
There was a problem hiding this comment.
This adds only the POSIX prefix layout. scripts/install.ps1:389-395 installs and invokes the managed Windows shim at HERMES_HOME/node/agent-browser.cmd; add that platform-specific candidate (also through agent_browser_runnable) or Windows hermes doctor will retain the same false negative.
Follow-up on the #53205 salvage: replace bare is_file() probes of the managed (~/.hermes/node[/bin]) and legacy (node_modules/.bin) locations with shutil.which(..., path=dir) so Windows resolves the executable .cmd shim instead of the extensionless POSIX script — the same miss class fixed for _has_agent_browser() in #73932. Also covers the Windows managed layout where the binary sits in node/ directly.
|
Landed on main via #73959 — your commit was cherry-picked with authorship preserved (f170113 → rebase-merged at c892ca2), plus a follow-up widening the managed/legacy dir probes to PATHEXT-aware |
Follow-up on the NousResearch#53205 salvage: replace bare is_file() probes of the managed (~/.hermes/node[/bin]) and legacy (node_modules/.bin) locations with shutil.which(..., path=dir) so Windows resolves the executable .cmd shim instead of the extensionless POSIX script — the same miss class fixed for _has_agent_browser() in NousResearch#73932. Also covers the Windows managed layout where the binary sits in node/ directly.
Follow-up on the NousResearch#53205 salvage: replace bare is_file() probes of the managed (~/.hermes/node[/bin]) and legacy (node_modules/.bin) locations with shutil.which(..., path=dir) so Windows resolves the executable .cmd shim instead of the extensionless POSIX script — the same miss class fixed for _has_agent_browser() in NousResearch#73932. Also covers the Windows managed layout where the binary sits in node/ directly.
Follow-up on the NousResearch#53205 salvage: replace bare is_file() probes of the managed (~/.hermes/node[/bin]) and legacy (node_modules/.bin) locations with shutil.which(..., path=dir) so Windows resolves the executable .cmd shim instead of the extensionless POSIX script — the same miss class fixed for _has_agent_browser() in NousResearch#73932. Also covers the Windows managed layout where the binary sits in node/ directly.
What does this PR do?
hermes acp --setup-browser --yesinstallsagent-browserinto the Hermes-managednode prefix at
~/.hermes/node/bin/agent-browser, which isn't necessarily onPATH.hermes doctoronly checkedPROJECT_ROOT/node_modulesandPATH(shutil.which),so it reported
agent-browser not installed (run: npm install)even though the binarywas present and runnable — a discovery false negative, not an install failure (#53192).
This mirrors the detection
hermes_cli/dep_ensure.py::_has_hermes_agent_browser()already does, so doctor and dep_ensure agree on what "installed" means.
Related Issue
Fixes #53192
Type of Change
Changes Made
hermes_cli/doctor.py: in the Node.js + agent-browser check, look for the binary inHERMES_HOME/node/bin/agent-browserand the legacyHERMES_HOME/node_modules/.bin/agent-browserbefore warning, each gated by
agent_browser_runnable()(the same dangling-symlink guardthe existing PATH branch uses). PATH,
node_modules, and Termux behavior are unchanged.tests/hermes_cli/test_doctor.py: added a positive case (managed bin present and runnable →detected, no warning) and a negative case (present but not runnable → still warns).
How to Test
agent-browseronly at~/.hermes/node/bin/agent-browser(not on PATH,no
node_modules/agent-browser), runhermes doctor— it now reports agent-browser OK instead of"not installed".
pytest tests/hermes_cli/test_doctor.py -q→ all green (66 passed).Checklist
Code
pytest tests/hermes_cli/test_doctor.py -qand all tests passDocumentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.md— N/Adep_ensure._has_hermes_agent_browser(); the existing Windows.cmdhandling there is unchanged