fix(cli): register config shell hooks on serve and dashboard startup - #102521
ericmaddox wants to merge 1 commit into
Conversation
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 106dba07008794c516b0a251ff42005625c2ad79 against base 63279301bcbdc185c1b07b98a9312eb0c862f26d, including the changed startup gates, fast-serve path, new tests, current cmd_dashboard lifecycle/profile behavior, shell-hook registration semantics, exact-head Actions, and the existing implementation history around #102504/#69825/#61806.
The underlying bug is real, and exact-head CI is green: the single commit has the CI, Nix flake, and Docker workflow suites completed successfully, with the required-check gate passing. I do see merge blockers in this particular wiring, though.
1. _AGENT_COMMANDS is the wrong ownership boundary for dashboard/serve lifecycle commands
Current main() deliberately calls _prepare_agent_startup(args) before command dispatch, with an explicit contract that management/introspection commands should not pay discovery cost or trigger hook consent. cmd_dashboard() then handles --status and --stop as early, dependency-free exits.
By adding both "serve" and "dashboard" to _AGENT_COMMANDS, this PR makes those commands run plugin discovery + config hook registration before cmd_dashboard() gets a chance to take its --status / --stop early exits. That changes a management operation into an agent-startup operation; it can load user code and can trigger shell-hook consent on a command whose current contract is specifically side-effect-light.
That exact concern already exists in the prior work on this defect class: #69832 intentionally registered at the shared dashboard/serve runtime boundary instead of broadening _AGENT_COMMANDS, and #81409 explicitly pins serve --status / --stop as hook-free.
Required fix: keep registration behind the runtime-owning boundary after lifecycle-only exits, or add an equally strong gate that proves --status/--stop never enter agent startup. Please add regression coverage for both management paths.
2. This can invert the documented plugin-before-shell-hook policy precedence
agent/shell_hooks.py documents a load-bearing ordering contract: Python plugins are registered first so their block decisions win ties over config shell hooks. _prepare_agent_startup() does not synchronously establish that order: it starts plugin discovery in a background thread and then continues to register_from_config(...). On the normal dashboard path, cmd_dashboard() later calls synchronous discover_plugins(), but with this patch the shell-hook callbacks may already have been appended to the process-global hook list before that join happens.
So the new route can make callback precedence timing-dependent on startup scheduling. That is a security-policy semantic change, not just startup performance.
Required fix: register config hooks only after plugin discovery is synchronously complete at this boundary (or explicitly join discovery before registration), and pin the invariant in a test where a plugin directive and shell-hook directive conflict and plugin policy wins deterministically.
3. The new tests prove invocation, not the security behavior being repaired
All three new tests replace config/plugin/hook modules or the startup functions with mocks and assert that registration was called. For a config-propagation/security-boundary fix, that misses the failure modes above: callback ordering, real allowlist/config loading, process-global registration, and whether an actual hook blocks an actual pre_tool_call lookup.
The repository guidance specifically calls for real-path validation for config propagation and security boundaries because mocks hide integration bugs. #81409 has a useful shape here: temp HERMES_HOME, real config loader/plugin manager, a real allowlisted hook subprocess, and an assertion on the resulting block decision.
Required fix: add at least one real-import regression that demonstrates the repaired serve/Desktop path reaches a real configured hook and preserves plugin precedence, plus the management-command no-side-effect tests above.
Interlock / attribution / merge order
This is a heavily duplicated defect family, so I would not merge this independently without consolidation:
- #102504 is already triaged as a duplicate of #69825; #69825 was subsequently linked back to #61806.
- #61844 is still open with the same
serve -> _AGENT_COMMANDSapproach. - #70461 is another open implementation of that same allowlist approach.
- #69832 is the earlier focused implementation that moves registration to the dashboard/serve-owned boundary after plugin discovery.
- #102513 is a fresh competing implementation on the same current base that extracts a shared config-hook helper and registers it inside
cmd_dashboard()after synchronous plugin discovery. - #81409 is broader, complementary class coverage: it treats profile-scoped Desktop/TUI/compute-host/slash-worker processes as separate registration boundaries, preserves plugin-first precedence, avoids leaking launch-profile hooks into the shared browser Dashboard, and keeps lifecycle-only commands hook-free.
Those are not interchangeable credits: #61844/#70461 are duplicate allowlist implementations; #69832 and #102513 are targeted competing repairs at the dashboard/serve owner; #81409 is the broader profile/process-boundary treatment. Please preserve that lineage when consolidating rather than re-solving the same bug as a new island.
One additional “other side of the shape” to keep explicit: fixing the Desktop serve process does not automatically fix isolated child processes, because hook registries are process-local. If this PR remains intentionally scoped to #102504, that broader coverage should stay credited/tracked with #81409 rather than being implied closed here.
The diagnosis and fast-path attention are useful work, and the exact-head CI is clean. The remaining work is to put the fix on the runtime boundary that actually owns the policy, preserve deterministic plugin precedence, and prove it with the real path. 🚀
Related: this is one of several open PRs fixing the same |
|
Thanks for the detailed review and guidance, @andrexibiza! I've updated the PR to address all three points: Ownership Boundary: Reverted _AGENT_COMMANDS and moved config shell hook + outbound webhook registration inside cmd_dashboard() directly after synchronous discover_plugins(), keeping --status and --stop completely hook- and discovery-free. Plugin Precedence: Ensured synchronous plugin discovery finishes before config shell hook registration runs, maintaining the deterministic plugin-before-shell-hook ordering contract. Real-Path Tests: Updated tests/hermes_cli/test_serve_shell_hooks.py with real-path tests verifying side-effect-free management exits, real $HERMES_HOME config loading, and callback ordering in the hook registry. |
andrexibiza
left a comment
There was a problem hiding this comment.
Follow-up on exact head 71e9a24c5b3d59643569155dd2bf4c97d1b5b6da after the review-fix commit.
The three behavioral blockers from my prior review are now substantively addressed:
serve/dashboardare no longer broadened into_AGENT_COMMANDS; registration moved insidecmd_dashboard()after the lifecycle-only--status/--stopexits, and both exits are pinned hook/plugin-free by regressions.- Config shell-hook registration now occurs after synchronous
discover_plugins(), so callback order is deterministic instead of startup-thread timing-dependent. The new registry-order regression drivescmd_dashboard()through real config loading and realshell_hooks.register_from_config()and confirms plugin callback first, config shell hook second. - The fast-serve path now dispatches directly to the runtime owner instead of invoking
_prepare_agent_startup()and reintroducing the earlier ownership problem.
I also rechecked the surviving history rather than transferring the old head receipt. Both commits are hosted-green: 106dba07008794c516b0a251ff42005625c2ad79 has CI 33814046995, Docker 33814046219, Nix 33814046124; 71e9a24c5b3d59643569155dd2bf4c97d1b5b6da has CI 33825794578, Docker 33825791122, Nix 33825791127.
The remaining landing gate is structural/topological, not the original security behavior. This branch still adds runtime ownership inside current-base hermes_cli/main.py, a godfile whose changed hunk is already past line 12,500. #102117 is concurrently decomposing that same owner (and reports main.py at 15,210 lines on this base, 3,534 after its current split), while #102513 is still an open same-base competing carrier for the same #102504 registration boundary. Please compose one implementation into the bounded dashboard/serve owner rather than landing another responsibility into main.py or allowing duplicate carriers to merge by order. The behavior on this head is now the version I would preserve during that composition; fresh exact-object proof is required after rematerialization.
71e9a24 to
092e0ad
Compare
|
@andrexibiza Thanks for the follow-up review! I have rematerialized the fix on current
|
…ousResearch#102504) Consolidate dashboard/serve runtime hook registration into topical `hermes_cli/config_shell_hooks.py` following the main.py decomposition. Ensures deterministic plugin-first ordering after synchronous plugin discovery, preserves hook-free contracts for early lifecycle exits (--status / --stop), and includes real-path precedence test coverage. Co-authored-by: NousResearch#61844 <contributor@nousresearch.com> Co-authored-by: NousResearch#70461 <contributor@nousresearch.com> Co-authored-by: NousResearch#69832 <contributor@nousresearch.com> Co-authored-by: NousResearch#102513 <contributor@nousresearch.com> Co-authored-by: NousResearch#81409 <contributor@nousresearch.com>
092e0ad to
62af8c7
Compare
What does this PR do?
Fixes a security and lifecycle bug where
hermes serve(the Hermes Desktop app's local agent backend) andhermes dashboardsilently skipped registering shell hooks defined inconfig.yaml.In
hermes_cli/main.py:_AGENT_COMMANDSon line 12785 was defined as{None, "chat", "acp", "rl"}, omitting"serve"and"dashboard"._prepare_agent_startup()early-returned on line 12844 becauseargs.commandwas not in_AGENT_COMMANDSor_AGENT_SUBCOMMANDS. This bypassedregister_from_config(_hooks_cfg)entirely.hooks:inconfig.yaml—pre_tool_calldestructive command guards, tenant guards, and outbound-send blockers — was silently disarmed during Desktop sessions, even though the exact same configuration armed and executed underhermes chatand messaging gateway sessions._try_fast_serve_launch()(the fast cold-start path taken on every start of the Desktop app) dispatched straight tocmd_dashboard(args)without calling_prepare_agent_startup(args)(unlike_try_fast_chat_launchand_try_termux_fast_cli_launchwhich properly invoke it).This PR:
"serve"and"dashboard"to_AGENT_COMMANDSso_prepare_agent_startup()executes hook registration."serve"and"dashboard"to_command_has_dedicated_mcp_startup()to prevent redundant synchronous inline MCP discovery, preservingcmd_dashboard's existing backgrounded / post-socket-bind MCP startup._prepare_agent_startup(args)inside_try_fast_serve_launch()prior to callingcmd_dashboard(args).Related Issue
Fixes #102504
Type of Change
Changes Made
hermes_cli/main.py:"serve"and"dashboard"to_AGENT_COMMANDS.args.command in {"acp", "serve", "dashboard"}to_command_has_dedicated_mcp_startup()._prepare_agent_startup(args)to_try_fast_serve_launch().tests/hermes_cli/test_serve_shell_hooks.py:test_prepare_agent_startup_registers_shell_hooks_for_serve: Assertsagent.shell_hooks.register_from_configis called with loaded config andaccept_hooks=Trueonserve.test_prepare_agent_startup_registers_shell_hooks_for_dashboard: Asserts shell hooks register ondashboard.test_try_fast_serve_launch_invokes_prepare_agent_startup: Asserts_prepare_agent_startupruns beforecmd_dashboard.How to Test
Run the new serve shell hook regression test suite and sibling CLI tests:
Checklist
Code
fix(cli): register config shell hooks on serve and dashboard startup (#102504))pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs