Skip to content

fix(plugins): propagate session_id to pre_tool_call hook from all 3 callsites - #34622

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/plugin-session-id-propagation
Closed

fix(plugins): propagate session_id to pre_tool_call hook from all 3 callsites#34622
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/plugin-session-id-propagation

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Propagates session_id to the pre_tool_call plugin hook from all three callsites in tool_executor.py and agent_runtime_helpers.py. Previously, session_id was accepted by get_pre_tool_call_block_message() but never passed by callers, causing plugins that scope by session to silently no-op.

Related Issue

Fixes #34618

Type of Change

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

Changes Made

  • agent/tool_executor.py: Added session_id=agent.session_id or "" to both callsites (concurrent path line ~215, sequential path line ~593) of get_pre_tool_call_block_message()
  • agent/agent_runtime_helpers.py: Added session_id=agent.session_id or "" to the invoke_tool() callsite (line ~1622)
  • tests/hermes_cli/test_plugins.py: Added 2 regression tests: test_session_id_forwarded_to_hook (verifies kwarg reaches hook) and test_session_id_defaults_to_empty_string (verifies default behavior preserved)

How to Test

  1. Run pytest tests/hermes_cli/test_plugins.py -k TestPreToolCallBlock -v — all 6 tests should pass (4 existing + 2 new)
  2. Verify the fix: write a plugin with a pre_tool_call hook that checks session_id, confirm it receives a non-empty value during agent execution
  3. Verify backward compatibility: plugins that don't use session_id continue to work unchanged

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
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/hermes_cli/test_plugins.py -v and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

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

Code Intelligence

  • Analyzed: get_pre_tool_call_block_message (3 callsites in agent/tool_executor.py and agent/agent_runtime_helpers.py)
  • Blast radius: LOW — adds one keyword argument to existing function calls; no behavioral change for plugins that don't use session_id
  • Related patterns: task_id is already propagated at all callsites; session_id follows the same pattern

…allsites

All three callsites of get_pre_tool_call_block_message() passed
task_id but omitted session_id. Plugin hooks that scope their
check on session_id (e.g. per-session rate limiting or security
policies) silently received the empty-string default and became
no-ops. Now each call forwards agent.session_id so hooks that
need session context actually receive it.

Regression tests confirm:
- session_id kwarg reaches the hook when provided
- Empty string default is preserved when omitted

Fixes NousResearch#34618
@Bartok9

Bartok9 commented May 29, 2026

Copy link
Copy Markdown
Contributor

Verified this fix against current origin/main. The approach is correct:

  • agent.session_id is the right in-scope attribute at all three callsites — it's already the canonical handle used elsewhere in tool_executor.py (e.g. current_session_id=agent.session_id at the session-tool path, and session_id=agent.session_id or "" at the post-tool hook callsites ~838/860). Propagating it to get_pre_tool_call_block_message mirrors the existing task_id=effective_task_id or "" pattern exactly.
  • The or "" guard is the right call: it keeps the "" default contract the hook signature already documents, so session-agnostic plugins are unaffected (blast radius LOW, as you note).
  • The two regression tests pin both directions (forwarded value + empty-string default), which is what was missing — the silent no-op had no coverage.

LGTM. Confirms the issue's root cause: all three callers passed task_id but dropped session_id, so any pre_tool_call plugin scoping on session context was a registered-but-inert no-op with no error/warning.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins labels May 29, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused regression fix. The reported behavior is already implemented on current main through the newer pre-tool dispatch architecture.

  • agent/tool_executor.py:453 and agent/tool_executor.py:1111 pass agent.session_id to resolve_pre_tool_block() for concurrent and sequential execution.
  • agent/agent_runtime_helpers.py:2197 does the same for agent-owned tools.
  • hermes_cli/plugins.py:2226 centralizes the hook resolution and forwards session_id into the directive lookup.
  • These paths were introduced by the post-PR refactor rooted in d682f320b and later centralized by 36308f0667.

Automated hermes-sweeper review.

@teknium1 teknium1 closed this Jul 13, 2026
@teknium1 teknium1 added sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

session_id not propagated to get_pre_tool_call_block_message from its 3 callsites — pre_tool_call plugin hooks that scope by session silently no-op

4 participants