fix(gateway): emit session:end from idle-expiry watcher + auto-reset paths - #28750
fix(gateway): emit session:end from idle-expiry watcher + auto-reset paths#28750mbs-vhs wants to merge 3 commits into
Conversation
…paths The gateway ``session:end`` hook only fired from the explicit ``/new``/``/reset`` command handler. Two other paths that terminate a session — the background idle-expiry watcher and the auto-reset branch in ``SessionStore.get_or_create_session`` — fired only the plugin-level ``on_session_finalize`` hook and closed the session locally without notifying gateway hook subscribers. External hooks under ``~/.hermes/hooks/`` that subscribe to ``session:end`` (to close mirror rows in an external DB, finalize logging, etc.) silently missed every idle-expiry- and auto-reset-driven close — leaving stale state forever. This change makes ``session:end`` symmetric with ``session:start``: - ``gateway/run.py:_session_expiry_watcher`` now emits ``session:end`` with ``reason="idle_expiry"`` right after marking ``entry.expiry_finalized = True``. Wrapped in ``try/except`` so a misbehaving subscriber can't break the watcher loop. - ``gateway/run.py:_handle_message_with_agent`` now emits ``session:end`` for the OLD session_id (with ``reason="auto_reset"``) in the ``_is_new_session`` branch — BEFORE emitting ``session:start`` for the new one — when ``SessionEntry.auto_reset_prior_session_id`` is set. - ``gateway/session.py`` adds a transient ``auto_reset_prior_session_id`` field on ``SessionEntry`` (default ``None``, not persisted to ``sessions.json``) so ``get_or_create_session`` can carry the prior id forward to the emit pipeline. - ``gateway/hooks.py`` docstring updated to document the new firing contract (``reason`` keys for the new emit paths). Existing behavior is preserved: the explicit ``/new``/``/reset`` emit shape at the original call site is unchanged, and existing subscribers keep receiving the same events for the same paths. The new emits include ``session_id`` (the explicit ``/new`` emit currently doesn't — that asymmetry is left for a separate change because it's load-bearing for the new emit paths only). Closes NousResearch#28746 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
teknium1
left a comment
There was a problem hiding this comment.
Thanks for chasing this. The premise checks out on current main: gateway session:end is only emitted from the explicit reset/new path at gateway/slash_commands.py:144, while _session_expiry_watcher finalizes expired sessions without emitting session:end at gateway/run.py:5624-5672, and the auto-reset start path emits only session:start at gateway/run.py:8239-8255.
Problems
- The branch includes large unrelated changes: gh pr view #28750 reports 29 files and +4433/-18, including a new tufte-data-viz skill at skills/devops/tufte-data-viz/SKILL.md:2 plus xurl/docs/sidebar moves. That should be split from this gateway bugfix.
- The auto-reset regression test copies the production emit fragment instead of calling the production path; see tests/gateway/test_session_boundary_hooks.py:371 and :377. That can pass while _handle_message_with_agent is still wrong.
- The user-facing hooks docs still list session:end as platform/user_id/session_key only on current main at website/docs/user-guide/features/hooks.md:79; the PR only updates gateway/hooks.py.
Suggested changes
- Keep the gateway/run.py + gateway/session.py fix, but drop/split the unrelated skill/docs commits.
- Exercise the real auto-reset emit path in tests, or extract a helper and test that helper through production code.
- Update the public hooks docs for the new session_id/reason fields.
This is an automated hermes-sweeper review.
| runner._maybe_load_user_skill = MagicMock(return_value=None) | ||
| runner.session_store.get_or_create_session.return_value = fresh_entry | ||
|
|
||
| # Drive only the relevant section by calling the runner method we |
There was a problem hiding this comment.
This test stops short of calling the production handler and then copies the emit fragment below, so it will not catch drift or reachability bugs in _handle_message_with_agent. Please exercise the real path or extract the emit logic into a helper used by production and tested here.
| @@ -0,0 +1,242 @@ | |||
| --- | |||
| name: tufte-data-viz | |||
There was a problem hiding this comment.
This new skill is unrelated to the gateway session:end bugfix. Please split it into a separate PR or drop it from this branch so the lifecycle fix can be reviewed and salvaged cleanly.
|
Thanks for the original fix. Current Problems
Suggested changes
This is an automated hermes-sweeper review. |
What does this PR do?
Fix the gateway
session:endhook so it fires from every session-close path, not just/new//reset. Without this, external hook subscribers (under~/.hermes/hooks/) silently miss every idle-expiry- and auto-reset-driven close — leaving stale mirror state forever.The gateway emits the lifecycle event
session:endfrom exactly one call site, the/new//resethandler. Two other paths close sessions but don't emit:Idle-expiry watcher (
gateway/run.py:_session_expiry_watcher) — fires the plugin-levelon_session_finalize(per [Bug]: The on_session_finalize hook is not being fired when gateway sessions expire due to configured idle time. #14981), evicts the cached agent, marksentry.expiry_finalized = True. But neverself.hooks.emit("session:end", ...).Auto-reset branch (
gateway/session.py:get_or_create_session) — when a stale entry is rolled over for a newsession_id, the local SQLite session ends but no gateway event fires for the oldsession_id.This PR makes
session:endsymmetric withsession:start— every close path now emits, every subscriber sees the close.Related Issue
Fixes #28746
Type of Change
Changes Made
gateway/run.py:_session_expiry_watcher— emitsession:endwithreason="idle_expiry"right afterentry.expiry_finalized = True. Wrapped intry/except logger.debug(..., exc_info=True)so a misbehaving subscriber can't break the watcher loop. (platformis pulled from the session_key just like the existingon_session_finalizeinvocation;user_idfromentry.originwith the sameor ""guard pattern used elsewhere in this file.)gateway/run.py:_handle_message_with_agent— in the_is_new_sessionbranch, before emittingsession:start, checkgetattr(session_entry, "auto_reset_prior_session_id", None). If set, emitsession:endwithreason="auto_reset", then clear the field so a follow-up turn cannot re-emit for the same close.gateway/session.py— add a transient fieldauto_reset_prior_session_id: Optional[str] = NonetoSessionEntry.get_or_create_sessionpopulates it (with the priorsession_id) whenever the auto-reset branch fires. Not added toto_dict()— this is a transient signal consumed by the next emit pass, never persisted tosessions.json.gateway/hooks.py— docstring update to reflect the new firing contract:session:endnow lists the three close paths and notes thereasonkey on the new emit paths.tests/gateway/test_session_boundary_hooks.py— three new tests:test_idle_expiry_emits_session_end— runs_session_expiry_watcheragainst a mocked-out store with one expired entry; asserts the emit fires with the expired session_id,session_key, andreason="idle_expiry". Regression guard for the watcher path.test_auto_reset_emits_session_end_for_prior_session— exercises the emit fragment in_handle_message_with_agentagainst aSessionEntrythat was just produced by auto-reset (withauto_reset_prior_session_id="sess-old-prior"); assertssession:endfires beforesession:start, with the right session_id +reason="auto_reset", and that the transient field is cleared after consume.test_session_entry_has_auto_reset_prior_session_id_field— dataclass-level sanity check (defaultNone, writable, not into_dict()).The existing 6 tests in this file (
test_reset_*,test_finalize_before_reset,test_shutdown_*,test_hook_error_*,test_idle_expiry_fires_finalize_hook) all still pass — no existing call site changes behavior.How to Test
Automated
pytest tests/gateway/test_session_boundary_hooks.py -v --override-ini="addopts="All 9 tests pass (6 pre-existing + 3 new).
Manual reproduction
Install a gateway hook in
~/.hermes/hooks/test-session-end/:Start the gateway. Open a Telegram DM session. Send one message. Confirm
session:startlands in~/session_end_test.log.Stop messaging. Wait for the configured idle-reset window to pass plus one 5-min watcher tick.
Before this fix: the log shows only
session:start(nosession:end).~/.hermes/sessions/sessions.jsonshowsexpiry_finalized: truefor the session.After this fix: the log shows
session:endwithreason: "idle_expiry",session_id,session_key,platform,user_id.sessions.jsonstill showsexpiry_finalized: true.Repeat for the auto-reset path: start a fresh session, idle past the reset window, then send a NEW message (not
/new). The handler logssession:endfor the OLDsession_id(withreason: "auto_reset"), thensession:startfor the NEW one.Note on Hermes version
Tested against the fork's
mainbranch (currently aligned withhermes-agent 0.14.0perpyproject.toml). The patched code paths exist identically in0.13.0and earlier — the line numbers in the original issue reference the v0.13.0 layout. The semantic anchors (function names, code shapes) are unchanged.Checklist
Code
fix(gateway): ...)chat_idto the existing emit sites — different bug, cleanly compatible with this change)pytest tests/gateway/test_session_boundary_hooks.py -vand all tests pass (9/9)test_session_boundary_hooks.py)Documentation & Housekeeping
gateway/hooks.pyEvents docstring reflects the new firing contract forsession:endcli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A