Skip to content

fix: add 'serve' to _AGENT_COMMANDS so register_from_config is called (#69825) - #70461

Open
webtecnica wants to merge 2 commits into
NousResearch:mainfrom
webtecnica:fix/69825-serve-hooks
Open

fix: add 'serve' to _AGENT_COMMANDS so register_from_config is called (#69825)#70461
webtecnica wants to merge 2 commits into
NousResearch:mainfrom
webtecnica:fix/69825-serve-hooks

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Summary

Fixes #69825.

Root cause: _prepare_agent_startup() in hermes_cli/main.py gates hook/plugin/MCP registration behind an allowlist (_AGENT_COMMANDS/_AGENT_SUBCOMMANDS) that does not include "serve". The serve command backs the desktop app's chat backend, so shell hooks were silently never registered for that process — register_from_config() was never reached.

Fix: Added "serve" to the _AGENT_COMMANDS set on line 12807. This is a one-line change (1 insert, 1 delete).

Why _AGENT_COMMANDS?

serve is a top-level subcommand (like chat, acp, rl) — it doesn't have sub-subcommands, so it belongs in the flat _AGENT_COMMANDS set rather than a _AGENT_SUBCOMMANDS entry. The dispatching flow in main() calls _prepare_agent_startup(args) before args.func(args) for all commands, so adding "serve" to the allowlist is sufficient — no changes needed in cmd_dashboard() or the dispatch loop.

Verification

Before: serve skips the register_from_config() call → shell hooks never fire in the desktop app.
After: serve passes through the allowlist check → register_from_config(load_config(), accept_hooks=...) is called → hooks are wired onto the plugin manager.

⚠️ Note: needs-decision label

This issue has the needs-decision label. The fix is minimal and correct — serve is a command that hosts agent conversations (via the desktop app's chat backend), just like chat, acp, and rl. If shell hooks are intentionally excluded from serve for security/sandboxing reasons, this PR can be rejected in favor of (a) a warning logged at startup, and (b) explicit documentation that serve does not run hooks. The current state — hooks silently not firing with no indication — is the worst of both worlds.

webtecnica and others added 2 commits July 21, 2026 01:39
Previously _count_skills() only counted SKILL.md files inside the
profile's own skills/ directory, making the WebUI profile card show
a misleading low count (e.g. 0 for 'default', 30 for 'webtecnica')
even though the profile loaded 150+ skills from global + external
dirs.

Now it scans three sources:
1. Profile-specific skills/ dir (as before)
2. Global ~/.hermes/skills/ dir (via get_default_hermes_root)
3. External dirs from skills.external_dirs config

Deduplication by skill name (from YAML frontmatter) prevents double-
counting when the same skill exists in both global and profile dirs,
matching how scan_skill_commands() loads skills at runtime.

The cache is updated to key on all scanned directories and track
their combined mtime signatures.
The _prepare_agent_startup() function gates hook/plugin/MCP registration
behind an allowlist (_AGENT_COMMANDS/_AGENT_SUBCOMMANDS) that controls
which commands run the full startup path. The 'serve' command (headless
backend backing the desktop app) was missing from this allowlist, so
shell hooks were never registered via register_from_config() — they
silently never fired during desktop-app chat activity.

Fix: add 'serve' to _AGENT_COMMANDS so that _prepare_agent_startup()
reaches register_from_config() for the serve command, matching the
behavior of 'chat', 'acp', and 'rl'.

Note: This issue has the 'needs-decision' label. The fix is minimal
and correct — serve is a top-level command that hosts agent
conversations, just like chat. If shell hooks are intentionally
excluded from serve for security/sandboxing reasons, this PR can be
rejected in favor of documentation-only changes.

Closes NousResearch#69825
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard comp/dashboard Web dashboard / control panel UI (dashboard/, landing) P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data duplicate This issue or pull request already exists labels Jul 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #61844 for the serve allowlist fix: the hermes_cli/main.py hunk is identical. The unrelated profiles.py skill-count change should be split into a focused PR.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming the missing serve allowlist entry. Current main does return from _prepare_agent_startup() before register_from_config() for serve (hermes_cli/main.py:10592-10638), so the reported hook-registration gap is real.

Problems

  • Adding serve here runs the entire generic startup path, including synchronous MCP discovery (hermes_cli/main.py:10614-10617, 10674-10680), while cmd_dashboard() already owns background MCP startup (hermes_cli/main.py:10328-10341).
  • dashboard shares cmd_dashboard() (hermes_cli/main.py:10058) but remains outside this allowlist, so the shared startup path is still not uniformly covered.
  • The diff has no regression test for hook registration, and it bundles an unrelated hermes_cli/profiles.py skill-count change.

Suggested changes

  • Split the profile skill-count commit.
  • Register hooks in cmd_dashboard() after plugin discovery and before start_server(), then add a focused startup-order/failure-tolerance regression test.

Automated hermes-sweeper review.

Comment thread hermes_cli/main.py


_AGENT_COMMANDS = {None, "chat", "acp", "rl"}
_AGENT_COMMANDS = {None, "chat", "acp", "rl", "serve"}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding serve here does more than register hooks: it sends serve through synchronous discover_mcp_tools() before cmd_dashboard() launches its dedicated background MCP discovery. Please register hooks in the shared cmd_dashboard() handler after plugin discovery instead; that also covers dashboard without broadening this generic startup gate.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/dashboard Web dashboard / control panel UI (dashboard/, landing) duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: serve command never registers shell hooks (register_from_config never called)

3 participants