Skip to content

fix(tui_gateway): pass live session context to plugin slash-command handlers - #44563

Open
benegessarit wants to merge 2 commits into
NousResearch:mainfrom
benegessarit:fix/tui-plugin-session-context
Open

fix(tui_gateway): pass live session context to plugin slash-command handlers#44563
benegessarit wants to merge 2 commits into
NousResearch:mainfrom
benegessarit:fix/tui-plugin-session-context

Conversation

@benegessarit

Copy link
Copy Markdown
Contributor

What does this PR do?

Plugin slash commands registered via ctx.register_command() can't tell which session invoked them when dispatched through the TUI gateway. Both dispatch paths (slash.exec's inline plugin fast path and command.dispatch's plugin fallback) call handler(arg) with only the raw argument string, and the gateway process doesn't reliably carry HERMES_SESSION_* env the way the CLI process does (where agent_init sets it in-process). I hit this building a session-scoped bookmark command: the handler either fails closed or, worse, resolves a session from profile-global state and binds the action to the wrong conversation.

This adds call_plugin_command_handler() to hermes_cli.plugins: it inspects the handler signature and passes session_id/session_key kwargs only to handlers that declare them (or **kwargs). Legacy fn(raw_args) handlers are called exactly as before. Both TUI dispatch sites now resolve the gateway session's session_key and route through it.

Explicit kwargs rather than an env-var bridge because gateway handlers can run concurrently — process-global mutation could bind one session's command to another session's context.

Scoped deliberately to the TUI gateway, the path with no working fallback. The CLI and messaging-gateway dispatch sites could adopt the same helper as a follow-up; #42416 is in flight covering those plus hook propagation, and I'm happy to converge with it whichever way you prefer (the helper here is intentionally compatible with that PR's design).

Related Issue

No existing issue found (searched for plugin session context). #42416 overlaps on the command-handler half — see note above.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/plugins.py — new call_plugin_command_handler(); register_command() docstring documents the optional context kwargs
  • tui_gateway/server.py — both plugin dispatch sites (slash.exec fast path, command.dispatch fallback) pass the live session key
  • tests/tui_gateway/test_protocol.py — regression tests: both paths deliver kwargs to declaring handlers, legacy handlers stay untouched, stale HERMES_SESSION_* env is neither consulted nor mutated
  • tests/hermes_cli/test_plugins.py — unit tests for the helper (legacy, declared-kwargs, **kwargs, empty-context skip, uninspectable-handler fallback, async)

How to Test

  1. scripts/run_tests.sh tests/tui_gateway/test_protocol.py tests/hermes_cli/test_plugins.py — 152 tests, green
  2. Manual: register a plugin command whose handler declares session_key="", run it from the TUI, and check the handler receives the live session key (before this change it gets "")
  3. The first commit adds the regression tests alone — they fail on main, pass with the fix

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate (feat(plugins): propagate session context to plugin hooks #42416 is adjacent; cross-referenced above)
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the test suite (scripts/run_tests.sh): the touched suites are fully green (152/152). A full local run has a handful of environment-dependent failures (browser/vision/provider-credential tests) that reproduce byte-identically on the unmodified base commit — verified by A/B at 880107a — so none are introduced by this change.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.4 (Apple Silicon)

Documentation & Housekeeping

  • I've updated relevant documentation (register_command() docstring) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A (no config changes)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — stdlib inspect only
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

…andlers

Both TUI plugin dispatch paths (the slash.exec inline plugin fast path
and the command.dispatch plugin fallback) called handler(arg) bare. The
gateway process carries no HERMES_SESSION_* environment, so a plugin
command that needs the invoking session (to bookmark it, annotate it,
or read its transcript) had no reliable way to resolve it: it either
failed, or worse, guessed from profile-global state and bound the
action to an unrelated session.

Add call_plugin_command_handler() to hermes_cli.plugins: it inspects
the handler signature and passes session_id/session_key kwargs only to
handlers that declare them (or **kwargs), keeping the documented
fn(raw_args) contract intact for existing plugins. Both dispatch sites
now resolve the gateway session's canonical session_key and call
handlers through it.

Explicit kwargs rather than a HERMES_SESSION_* env bridge because
gateway handlers can run concurrently; process-global mutation could
bind one session's command to another session's context.
@liuhao1024

Copy link
Copy Markdown
Contributor

Verified clean.

  • call_plugin_command_handler() uses inspect.signature to introspect the handler and only passes session_id/session_key kwargs when the handler declares them (or accepts **kwargs). Legacy fn(raw_args) handlers are called unchanged.
  • Both dispatch sites in tui_gateway/server.py (slash.exec and command.dispatch) updated to pass live session context from the RPC session store, not from env vars.
  • Tests cover: legacy handler, declared kwargs, **kwargs, empty context, uninspectable handler, async handler resolution, and env-var isolation (stale HERMES_SESSION_KEY is not consulted).
  • The concurrent-safety design is correct: session context is passed as explicit kwargs rather than mutating process-global env vars.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/tui Terminal UI (ui-tui/ + tui_gateway/) comp/plugins Plugin system and bundled plugins labels Jun 12, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Verification: session context pass-through for plugin handlers

The call_plugin_command_handler introspection approach is well-designed:

  • Uses inspect.signature to detect whether a handler accepts session_id/session_key kwargs or **kwargs, falling back gracefully for legacy fn(raw_args) handlers
  • Empty context values are correctly skipped (avoids overwriting handler defaults with empty strings)
  • Uninspectable handlers (e.g., C extensions) fall back to args-only — safe degradation
  • The two dispatch sites (slash.exec and command.dispatch) both use the new helper consistently
  • Env-var mutation is explicitly avoided (session context is passed as kwargs, not HERMES_SESSION_* globals) — correct for concurrent gateway sessions

Test coverage is thorough: 6 tests covering legacy handlers, declared kwargs, **kwargs, empty context, uninspectable handlers, and async handlers. The gateway integration tests verify that stale env vars don't leak through.

Looks good to me.

@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 isolating the two TUI dispatch paths and preserving legacy fn(raw_args) handlers.

Problems

  • The new calls pass the same session_key for both session_id and session_key. Current main explicitly keeps those identities separate during session rotation: tui_gateway/server.py:3123-3130 documents that compression can rotate agent.session_id, and _session_lookup_key() at tui_gateway/server.py:6017-6024 prefers that live agent ID. A plugin accepting session_id can therefore receive a gateway key rather than the durable session ID.
  • The added tests set agent to None and assert identical values, so they cannot detect that mismatch.

Suggested changes

  • Derive session_id from getattr(session.get("agent"), "session_id", None) or session_key, while preserving session_key from the session dict, at both TUI dispatch sites.
  • Cover a distinct agent session ID and gateway session key in both dispatch-path regressions.

Automated hermes-sweeper review.

Comment thread tui_gateway/server.py
)

handler = get_plugin_command_handler(name)
if handler:

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.

session_id and session_key are distinct TUI identities: use getattr(session.get("agent"), "session_id", None) or session_key for the former, while retaining the stored gateway key for the latter. Current main documents their divergence during compression in _sync_session_key_after_compress().

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/plugins Plugin system and bundled plugins comp/tui Terminal UI (ui-tui/ + tui_gateway/) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants