fix(gateway): register declarative shell hooks in the Desktop/TUI backend - #63781
gcunharodrigues wants to merge 1 commit into
Conversation
…kend Shell hooks declared in config (`hooks:`) are registered onto the plugin manager by `agent.shell_hooks.register_from_config()`, and fired from the shared AIAgent turn path (`agent/turn_context.py` for `pre_llm_call`, `cli.py` for the session-finalize events). Registration was wired into three frontends — `cli.py`, `hermes_cli/main.py`, `gateway/run.py` — but NOT into `tui_gateway`, which is the only backend behind Hermes Desktop and the TUI (both the stdio `tui_gateway.entry` host and the `hermes serve` WS host build every agent through `_make_agent`). Consequence: in a Desktop/TUI session the entire `hooks:` block silently never runs. The firing sites are present in the process (turn_context, session lifecycle), so nothing errors — the callbacks were simply never attached. This is the same retrofit-one-frontend-at-a-time gap that NousResearch#38945 fixed for MCP discovery on the WS path; `acp_adapter` has the same omission for the same reason. It is an oversight, not a security boundary: the desktop dashboard itself ships a shell-hook admin surface (`GET/POST /api/ops/hooks`), and the consent model (`_prompt_and_record` fails closed when stdin is not a TTY) was built precisely so a headless frontend can register. Registration goes in `_make_agent` — the single chokepoint both the stdio and WS hosts route through — rather than `tui_gateway/entry.py`, which is only the stdio transport (the Electron/dashboard path reaches the agent via `tui_gateway/ws.py`, never `entry.main()`; see NousResearch#38945). It mirrors `gateway/run.py`: `accept_hooks=False` lets `register_from_config` resolve consent from `HERMES_ACCEPT_HOOKS` / `hooks_auto_accept` (no TTY here either), and a try/except logs but never blocks an agent build. The registration is gated on `get_hermes_home_override() is None` so only the launch profile binds the process's hooks. The plugin manager is a process-global singleton and its shell-hook callbacks carry no profile guard; in app-global remote mode one backend serves every local profile, and a non-launch profile builds here under `set_hermes_home_override(profile)`. Registering its config would append that profile's commands onto the shared manager and fire them on every other profile's turns — cross-profile command execution plus double context injection. The process's hooks belong to the launch profile alone. Tests: `test_make_agent_registers_configured_shell_hooks` asserts the Desktop backend's agent-build path registers a configured `pre_llm_call` hook onto a real plugin manager (RED before the fix: zero callbacks registered); `test_make_agent_skips_shell_hooks_for_non_launch_profile` asserts a build under an active HERMES_HOME override registers nothing, pinning the cross-profile guard.
Duplicate of #57020 (earliest open) — both register declarative shell hooks at the same |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Reviewed by Hermes Agent
|
Thanks for tracing the missing Desktop/TUI registration path. The current-main premise is valid: Problems
Suggested changes
The maintainer comment also identifies #57020 as the earlier implementation at this same chokepoint; please preserve the useful diagnosis while maintainers select the canonical salvage path. This is an automated hermes-sweeper review. |
|
Thanks @gcunharodrigues for tracing the process-local hook gap. This The merged implementation reuses the already profile-keyed registrar at the actual agent factory, with real launch/A/B/A subprocess checks for file and terminal hooks, exactly-once dispatch, and unapproved controls. It does not need another registration helper or a launch-profile-only omission. Your diagnosis and regression direction are acknowledged alongside the earlier #57020 by @grimmjoww. Closing as superseded on this shared factory path. The separate startup-before-MCP/refusal contract remains open in #69825; this is not a blanket closure of ACP or unrelated lifecycle work. |
The bug
Declarative shell hooks (
hooks:in config) are registered onto the plugin manager byagent.shell_hooks.register_from_config()and fired from the sharedAIAgentturn path —pre_llm_callfromagent/turn_context.py, the session-lifecycle events fromcli.py. Registration is wired into three frontends:cli.py(interactive CLI/TUI)hermes_cli/main.py(CLI startup)gateway/run.py(messaging gateway)It is not wired into
tui_gateway, which is the only backend behind Hermes Desktop and the TUI — both the stdiotui_gateway.entryhost and thehermes serveWS host build every agent through_make_agent.Consequence: in a Desktop/TUI session the entire
hooks:block silently never runs. The firing sites are present in the process, so nothing errors — the callbacks were simply never attached. Every shell-hook event is inert there:pre_tool_callguardrails,post_tool_call, thetransform_*filters, session lifecycle, subagent hooks,pre_approval_request, andpre_llm_callcontext injection.The mute is invisible:
hermes hooks listand the dashboard hook panel (GET /api/ops/hooks) both report the hooks as configured, approved, and executable, because they read config + allowlist, never the live plugin manager.Why it's an oversight, not a boundary
grep -rn "shell_hooks\|register_from_config" tui_gateway/is empty; no comment, guard, or config key names the GUI/TUI as hook-free.agent/shell_hooks.py's own docstring names only "the CLI entry point and the gateway entry point" — it predates the newer frontends.acp_adapter/has the identical omission, i.e. the two newest frontends both missed it: a retrofit gap._prompt_and_recordreturnsFalsewhenstdinis not a TTY (fail-closed, never blocks), andhooks_auto_accept/HERMES_ACCEPT_HOOKSexist for exactly this.GET/POST /api/ops/hooks, which writes a hook into config and records its approval) — a project deliberately keeping shell hooks out of the desktop backend would not ship an "approve this shell hook" button in the desktop dashboard.tui_gateway/ws.py).The fix
Register in
_make_agent— the single chokepoint both the stdio and WS hosts route through — rather thantui_gateway/entry.py, which is only the stdio transport (the Electron/dashboard path reaches the agent viatui_gateway/ws.py, neverentry.main()). It mirrorsgateway/run.py:accept_hooks=Falseletsregister_from_configresolve consent fromHERMES_ACCEPT_HOOKS/hooks_auto_accept(no TTY here either), wrapped in a try/except that logs but never blocks an agent build.Gated on
get_hermes_home_override() is Noneso only the launch profile binds the process's hooks. The plugin manager is a process-global singleton and its shell-hook callbacks carry no profile guard. In app-global remote mode one backend serves every local profile, and a non-launch profile builds here underset_hermes_home_override(profile); registering its config would append that profile's commands onto the shared manager and fire them on every other profile's turns — cross-profile command execution plus double context injection. The process's hooks belong to the launch profile alone.Not a double-fire with the slash worker
Slash commands run in a
_SlashWorkersubprocess that boots HermesCLI and registers hooks there (cli.py). Verified this does not double-fire:slash.execroutes only command text toworker.run(cmd)in that separate process (its own plugin-manager singleton); normal chat never crosses that boundary, so no hook fires twice for one user action.Tests
test_make_agent_registers_configured_shell_hooks— asserts the Desktop backend's agent-build path registers a configuredpre_llm_callhook onto a real plugin manager. RED before the fix: zero callbacks registered (not a consent/allowlist failure).test_make_agent_skips_shell_hooks_for_non_launch_profile— builds under an activeHERMES_HOMEoverride and asserts nothing registers, pinning the cross-profile guard.Both use the real
register_from_configand a realPluginManager, and restore the process-global registration state on teardown.No new dependency, no new config key, no refactor.