feat(hooks): add message:pre_route event + multi-role-router reference hook - #74272
feat(hooks): add message:pre_route event + multi-role-router reference hook#74272ceverson70 wants to merge 9 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused hook contract and reference implementation. The current patch needs correctness and integration work before it can provide the claimed role isolation.
Problems
optional-skills/multi-role-router/handler.py:456returnsNonefor a newly selected role with no saved session. The gateway already creates the current inbound session atgateway/run.py:15527, so that first role turn stays in the existing session rather than creating isolation.- Current main builds session context and tool session environment at
gateway/run.py:15638-15641before the proposed pre-lease insertion point. The new redirect only replacessession_entry; unlike/resume, it does not clear conversation scope or evict the cached agent (gateway/slash_commands.py:4430-4442). optional-skills/multi-role-router/has noSKILL.md, butOptionalSkillSourcediscovers optional skills only throughSKILL.md(tools/skills_hub.py:3175).
Suggested changes
- Resolve the routing-contract overlap noted for #69693 and #72942, then define the first-route session-creation behavior.
- Rework the integration around final session resolution and add an end-to-end gateway test for transcript and cached-agent isolation.
- Package the optional artifact as a valid optional skill, or relocate it to an appropriate reference-hook surface.
This is an automated hermes-sweeper review.
| target_session_id = sessions.get(target_role, "") | ||
|
|
||
| if not target_session_id: | ||
| # No existing session for this role — let the gateway create a new one |
There was a problem hiding this comment.
The gateway has already run get_or_create_session() before this hook fires, so returning None cannot create a role session; it dispatches this first target-role message in the current session. Create or otherwise prepare a distinct target session here before returning a route decision, or explicitly scope the feature to previously created sessions.
|
Mechanical fixes are in progress (first-route isolation, agent cache eviction after session swap, SKILL.md). One thing we can't resolve without a maintainer call: the routing-contract conflict with #69693 and #72942 flagged by @alt-glitch. We've documented the three-layer stack in the PR description (our hook sits between Could we get a direction on #5143 so we know which design to finalize? |
- README: remove the /role slash-command table — those commands are not implemented in this PR or on main; document the config.yaml controls (multi_role_router.auto) that actually work. Fix the troubleshooting entry that referenced /role auto off. - SKILL.md: shorten description to the <=60 char one-sentence standard. - HOOK.yaml: credit the original implementation (NousResearch#74272, Clark Everson) instead of 'community'; bump version to 1.0.1. - gateway/hooks.py: fix the message:pre_route chat_type contract to the real MessageSource values (dm|group|channel|thread|webhook), not the Telegram-specific ones. - handler.py: update the stale /role comment.
|
Update to my earlier note: rather than only offering the delta, I've rebuilt it properly as #78326 — your seven #74272 commits cherry-picked/rebased onto current The delta commit in #78326 covers what I flagged before, in case you'd rather fold it into this PR instead:
All 58 tests pass via |
On top of the NousResearch#74272 implementation (cherry-picked/rebased with original authorship preserved): - handler.py: read config via canonical load_config_readonly() instead of raw yaml.safe_load — satisfies tests/hermes_cli/test_config_read_guard.py and honors managed-scope overlay, env expansion, and profile paths. - handler.py: classifier uses async_call_llm() (triage_specifier slot) instead of the sync call_llm() — a sync call inside the async handler blocks the gateway event loop for every classified message. - handler.py: remove dead _update_meta_session() — it appends a history entry with an assistant response, but message:pre_route fires before the agent responds, so it could never be wired correctly. - gateway/hooks.py: fix the message:pre_route chat_type contract to the real MessageSource values (dm|group|channel|thread|webhook). - README: drop the /role slash-command table — those commands are not implemented in this PR or on main; document the config.yaml controls (multi_role_router.auto) that actually work. - SKILL.md: description to the <=60 char one-sentence standard. - HOOK.yaml: credit the original implementation (NousResearch#74272) explicitly. - tests: align with load_config_readonly/async_call_llm paths.
|
Thanks @raulvidis for the thorough review and for standing up #78326 with the fixes properly attributed. The delta changes (async classifier via Happy to go either way — fold those fixes into this PR, or let #78326 be the merge vehicle. Whichever is cleaner for maintainers. The blocking question remains: @alt-glitch, could we get a direction on #5143 re: the routing contract? Specifically, should |
|
Three bugs fixed in 1. 2. Synchronous 3. Removed dead code All 48 tests in |
|
Thanks for the review of the delta and for e7f96e442 — two of the three are good catches we converged on, one needs a second look: Bug 1 (history dead) — diagnosis correct, but the fix clobbers the session map. You're right that history was never populated — and to be transparent, my #78326 delta had the same gap: I removed But the wiring in e7f96e442 calls Bug 2 (event-loop block) — Bug 3 (CONTINUATION_PATTERNS) — adopted in the same commit. All 61 tests pass via |
…e hook Closes NousResearch#5143 Adds a new `message:pre_route` hook event that fires after session resolution but before the turn-lease is acquired. Hooks can return {"decision": "switch_session", "session_id": "<id>"} to transparently redirect the message to a different session (worker profile) before the agent begins processing. The user sees no friction — they just talk. Core changes (~27 lines, 2 files): - gateway/hooks.py: document message:pre_route event with full context spec and return-value contract - gateway/run.py: insert emit_collect("message:pre_route", ...) block in _handle_message_with_agent after session resolution, before turn-lease acquisition; applies switch_session on decisive results Reference hook (optional-skills/multi-role-router/): - HOOK.yaml: manifest declaring the message:pre_route subscription - handler.py (~420 lines): classifier-based router using the existing auxiliary LLM slot (triage_specifier → compression fallback); stateless from the gateway's perspective; continuation fast-path skips the LLM on short acknowledgements; role config in config.yaml with sane defaults matching the bundled worker profiles - README.md: install, config snippet, /role slash commands, troubleshooting Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- handler.py: atomic meta.yaml writes with threading.Lock + os.replace - handler.py: null-safe multi_role_router and auxiliary config reads - handler.py: user-defined roles replace defaults (not merge) - handler.py: fuzzy role match longest-first with word boundaries - run.py: wrap emit_collect in try/except, fix break placement in loop Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- Fix: wrap _call_auxiliary_llm in try/except in _classify_message so LLM exceptions don't propagate through handle() to the gateway - Fix: CONTINUATION_RE now correctly matches multi-word acks (ok thanks, got it, makes sense, sounds good) using alternation with word boundaries - tests/test_multi_role_router.py: 45 tests covering fast-path, role config, meta.yaml atomicity, LLM response parsing, handle() integration - tests/gateway/test_message_pre_route_hook.py: 10 tests covering the emit_collect block in run.py (exception handling, switch_session trigger conditions, break placement) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- handler.py: update meta.yaml on first-route-to-new-role so next turn has correct role context; log clearly instead of silent None return - optional-skills/multi-role-router/SKILL.md: add required SKILL.md so OptionalSkillSource discovers the hook reference (skills_hub.py:3175) - gateway/run.py: document agent-cache isolation limitation in pre-route block; full eviction requires earlier insertion point (out of scope) CodeRabbit major findings applied: - handler.py: restrict CONTINUATION_RE — remove what/how/why/when/where/ which as standalone openers so topic questions reach the classifier - handler.py: use META_FILE.parent in mkstemp dir (cross-filesystem safety) - handler.py: wrap _classify_message call in try/except — fail open to current_role on any classification exception - handler.py: protect meta load/mutate/save in handle() with _META_LOCK, matching _update_meta_session locking; reload inside lock for freshest state Skipped (out of scope): - async refactor of _classify_message/_call_auxiliary_llm (major restructure) - test infra improvements (HOOK_DIR monkeypatch, xfail assertion cleanup) - pending_role semantic (conflicts with PR blocker NousResearch#1 design intent) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…it_collect Mirrors the timeout guard added in the TUI path (tui_gateway/server.py). Prevents a hanging hook handler from holding the turn-lease window open indefinitely inside GatewayRunner._handle_message_with_agent.
…viction after session swap
…ging, document TOCTOU assumption
- Call _update_meta_session() from handle() after classification so history is populated for future classifier prompts (was always empty) - Wrap synchronous call_llm with asyncio.to_thread() to avoid blocking the event loop when called from async context - Remove unused CONTINUATION_PATTERNS list (CONTINUATION_RE is used) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…aulvidis review) Bug 1: _update_meta_session() was called after the decision block with session_id=current_session_id, which wrote sessions[target_role] = inbound_id and clobbered the target role's session mapping set by the decision block. Fix: inline history recording inside the decision lock atomically so the session map is never overwritten. Bug 2: _call_auxiliary_llm() used task="compression" but _get_auxiliary_config() reads the triage_specifier slot first. Users who configure auxiliary.triage_specifier would not get that model used for classification. Fix: changed to task="triage_specifier" to match the config read path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e7f96e4 to
5d57120
Compare
|
Thanks @raulvidis for the thorough review — both bugs are now fixed and the branch has been rebased onto Bug 1 (session-map clobber): The separate Bug 2 ( All 48 tests pass after the rebase. The |
Summary
Closes #5143
Implements the multi-role auto-routing feature proposed in the RFC. Users talk naturally from the home session — a lightweight classifier transparently routes each message to the appropriate worker profile (code-worker, knowledge-worker, ml-worker, ops-worker) before the agent begins processing.
gateway/hooks.py— addsmessage:pre_routeto the documented hook events with full context spec and return-value contractgateway/run.py— insertsemit_collect("message:pre_route", ...)in_handle_message_with_agentafter session resolution, before turn-lease acquisition; appliesswitch_sessionon decisive hook results (~27 lines, matches existingcommand:*pattern)optional-skills/multi-role-router/— reference hook users drop into~/.hermes/hooks/: classifier via existingauxiliary.triage_specifierslot, continuation fast-path (skips LLM on short acks), atomic meta.yaml writes,/roleslash commands, config-driven role definitions with sane defaultstests/test_multi_role_router.py— 45 unit tests covering fast-path, role config, meta.yaml atomicity, LLM response parsing, handle() integrationtests/gateway/test_message_pre_route_hook.py— 10 tests covering the emit_collect block in run.py (exception handling, switch_session trigger conditions, break placement)Two bugs found during testing and fixed:
_classify_messagewas not catching exceptions from_call_auxiliary_llm— exceptions would propagate throughhandle()to the gateway. Now caught with warning log + current_role fallback.CONTINUATION_REdidn't match multi-word acks ("ok thanks", "got it", "makes sense"). Fixed with proper alternation and word boundaries.Relationship to open routing proposals
Two open issues propose routing hooks at adjacent but distinct layers:
pre_gateway_dispatchwith{"action": "route", "profile": "..."}— this fires at gateway ingress, before session auth.pre_agent_dispatchevent — this fires at the agent dispatch layer, after the session is fully resolved and a worker is selected.message:pre_routefires between these two: after session resolution (get_or_create, delegation pinning, topic tip-walk, auto-reset) is complete, and before the turn-lease is claimed. This is a distinct layer with different semantics — session context is fully available, but the turn has not yet been committed.The three hooks are complementary, not competing. They cover different phases of the call stack:
A new event name is justified because the layer is different:
pre_gateway_dispatchhas no session context,pre_agent_dispatchhas already committed a turn, andmessage:pre_routesits between them with full session state but no turn commitment — which is exactly the right place to make a session-switch decision.Note for maintainers:
message:pre_routecould also serve as the implementation vehicle for #72942'srouteaction if that is preferred — theswitch_sessionprimitive it already uses is equivalent to what #72942 describes. The hook contract could be extended to accept a{"action": "route", "profile": "..."}return value alongside the existing{"decision": "switch_session", "session_id": "..."}form. This PR makes no claim on that design choice and defers to maintainer preference.Test plan
python -m pytest tests/test_multi_role_router.py tests/gateway/test_message_pre_route_hook.py -v— 58 tests, all passruff check gateway/hooks.py gateway/run.py optional-skills/multi-role-router/handler.py— cleanpython scripts/check-windows-footguns.py gateway/hooks.py gateway/run.py optional-skills/multi-role-router/handler.py— cleanagent:start,command:*) unaffected — no regressions in hook dispatchmulti_role_router.auto: falsein config disables routing without errorPlatform tested
macOS 15.5 (Apple Silicon), Python 3.11.15, Hermes v0.19.0 (cfa43f5)
Notes for reviewers
switch_sessionprimitive used is the same one already called for Telegram topic binding and delegation routing — no new session APIsoptional-skills/as a reference users copy to~/.hermes/hooks/— zero gateway changes required to use or remove itemit_collectis now wrapped withasyncio.wait_for(timeout=5.0)to bound hook execution and prevent a hanging handler from holding the turn-lease window open🤖 Generated with Claude Code