Skip to content

fix(hermes): skip Omnigent relay tools in the pre_tool_call hook - #2220

Merged
TomeHirata merged 1 commit into
omnigent-ai:mainfrom
dosenr:contrib/hermes-hook-skip-relay
Jul 9, 2026
Merged

fix(hermes): skip Omnigent relay tools in the pre_tool_call hook#2220
TomeHirata merged 1 commit into
omnigent-ai:mainfrom
dosenr:contrib/hermes-hook-skip-relay

Conversation

@dosenr

@dosenr dosenr commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Related issue

Closes #2219

Summary

hermes_policy_hook.py evaluated PHASE_TOOL_CALL policy for every tool, including the
Omnigent relay tools (mcp_omnigent_* / mcp__omnigent__*). Those tools are already gated
when the relay dispatches them back through the server's tool path (fail-closed, re-checked on
retry), so the hook gated them a second time — parking a duplicate approval card whose hook
long-poll never returns after the first card is resolved, wedging the turn.

Skip those prefixes in the hook, matching the guard the native claude/codex hooks already apply
(native_policy_hook.hook_payload_to_evaluation_request skips mcp__omnigent__*; Hermes names
the same relay tools mcp_omnigent_*, so both forms are matched). Hermes' own tools (shell,
file) and non-Omnigent MCP servers lack the prefix and stay gated by the hook.

Test plan

tests/inner/test_hermes_policy_hook.py asserts the relay tools are allowed without a call to
/policies/evaluate (both underscore forms) and that native tools and non-Omnigent MCP servers
still round-trip the hook's policy gate. The relay-skip cases fail before the change (the hook
evaluates them) and pass after. uv run pytest tests/inner/test_hermes_policy_hook.py green.

Demo

Live web UI run (headless hermes with the Omnigent MCP server from #2216
registered, demo ASK policy on sys_session_get_info): the gated relay call
parks exactly ONE approval card - the dispatch-side policy evaluation. Without
this fix the pre_tool_call hook would evaluate the same call a second time and
park a second card whose long-poll outlives the human's single approval.

one approval card, not two

approved, tool runs

Original terminal transcript

This is a hook-logic change with no UI surface, so the demonstration is the test: the relay
tools short-circuit to allow without a /policies/evaluate round-trip, while native tools still
round-trip. The test_relay_tools_are_skipped cases fail before the change and pass after:

$ uv run pytest tests/inner/test_hermes_policy_hook.py -q
......                                                                     [100%]
6 passed

End-to-end (a single approval card per gated relay call, no post-resolution wedge) was verified
against a running deployment.

Type of change

  • Bug fix

Test coverage

  • Unit test added

Coverage notes

The end-to-end behavior (a single approval card per gated relay call, no post-resolution wedge)
was verified against a running deployment; the added unit tests cover the hook-level skip that
was the defect.

Changelog

Fixed the Hermes pre_tool_call hook double-gating Omnigent relay tools, which parked a
duplicate approval card and could wedge the turn.

Omnigent relay tools surfaced into Hermes (mcp_omnigent_* / mcp__omnigent__*)
are already policy-gated when the relay dispatches them back through the
server's tool path. The pre_tool_call hook evaluated them a second time, parking
a duplicate approval card per call; a human resolves one and the other's
long-poll never returns, wedging the turn after the approved tool runs. Skip
those prefixes in the hook, matching the guard the native claude/codex hooks
already apply. Hermes' own tools (shell, file) and non-Omnigent MCP servers lack
the prefix and stay gated.

Signed-off-by: rdosen <robert.dosen@gmail.com>
@github-actions github-actions Bot added the size/M Pull request size: M label Jul 8, 2026
@github-actions
github-actions Bot requested a review from SabhyaC26 July 8, 2026 18:39
@dosenr dosenr closed this Jul 8, 2026
@dosenr dosenr reopened this Jul 8, 2026
@github-actions
github-actions Bot requested review from TomeHirata and removed request for SabhyaC26 July 8, 2026 18:58
@github-actions github-actions Bot assigned TomeHirata and unassigned SabhyaC26 Jul 8, 2026
@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

@dosenr This PR is a Bug fix, Feature, or UI / frontend change but the Demo section is missing or only contains a placeholder.

These change types require a screenshot or screen recording so reviewers can see the new behaviour without checking out the branch. Please update the Demo section with:

  • A screenshot or screen recording of the change, or
  • A link to a hosted video or GIF showing the new behaviour.

Use N/A only when the change has no user-visible effect whatsoever (e.g. a pure refactor or test-only change). If that's the case, uncheck the relevant type box and check Refactor / chore or Test / CI instead.

@github-actions github-actions Bot added the needs-demo PR needs a demo screenshot or recording label Jul 8, 2026
@dosenr

dosenr commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Headless/server-side change with no UI surface; the terminal transcript in the Demo section is the demonstration.

@TomeHirata TomeHirata 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.

LGTM!

@TomeHirata
TomeHirata enabled auto-merge (squash) July 9, 2026 10:29
@TomeHirata
TomeHirata merged commit 255a5f8 into omnigent-ai:main Jul 9, 2026
100 of 104 checks passed
@github-actions github-actions Bot added the no-doc-update Merged PR does not need a docs update label Jul 9, 2026
@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

🏷️ Doc impact: no-doc-update

Internal bugfix to the Hermes policy hook skipping duplicate gating of Omnigent relay tools; no user-facing surface, integration, or documented behavior changed.

Auto-classified on merge. Set the label manually before merging to override. · run

@omnigent-ci

omnigent-ci Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Polly AI Review

Blocking issues

None.

Security vulnerabilities

None. The change narrows what the hook evaluates but does not weaken the security boundary: the skipped mcp_omnigent_* / mcp__omnigent__* tools remain gated at the relay dispatch path (ProxyMcpManager → Omnigent /mcp_evaluate_tool_call_policy), which is fail-closed and re-checked on retry. This exactly mirrors the guard already in native_policy_hook.hook_payload_to_evaluation_request (which skips mcp__omnigent__* for the same double-count reason), so it's an established, deliberate pattern rather than a new hole. Hermes' own tools (terminal, editor) and non-Omnigent MCP servers (mcp_github_*) lack the prefix and continue to round-trip the hook — confirmed by the second test.

Non-blocking notes

  • Spy return shape doesn't match the real function. In the test helper _spy returns a bare _R(), but the hook unpacks resp, api_error = post_evaluate_with_retry(...) (a 2-tuple). For the "still gated" cases the unpack therefore raises and the hook's broad except Exception fails open. The test still passes because it only asserts called["hit"] is True (set before the return), so the assertion it does make is valid — but the helper doesn't faithfully model a successful evaluation. Consider returning (_R(), None) so the gated-path test also exercises the ALLOW branch end-to-end.
  • Single-underscore prefix breadth. mcp_omnigent_ (single underscore) is broader than the canonical mcp__omnigent__ and could in principle match a differently-named tool from another server that happens to start mcp_omnigent_.... In practice Hermes derives this name from the registered omnigent MCP server, so collision is not realistic; worth a mental note only if tool-name derivation ever changes.
  • The mcp_omnigent_* (Hermes single-underscore) naming is asserted by the PR rather than visible in the checked-out source (the server is registered as omnigent, and the transform lives in the external Hermes CLI). Matching both forms is a reasonable defensive choice; just flagging that the single-underscore form couldn't be independently confirmed from this repo.

Summary

A tight, well-scoped bug fix. It resolves the double-gating that parked a duplicate approval card and wedged the turn by short-circuiting Omnigent relay tools in the Hermes pre_tool_call hook — consistent with the existing native-hook guard and with the relay's authoritative dispatch-side gate. Tests cover both the skip (no /policies/evaluate round-trip, both underscore forms) and the still-gated path. The PR includes before/after approval-card screenshots, satisfying the visual-demonstration expectation for a stuck-then-fixed behavior. No blocking or security concerns; only minor test-fidelity polish suggested.


Automated review by Polly · workflow run

yours-aditya pushed a commit to yours-aditya/omnigent that referenced this pull request Jul 16, 2026
…igent-ai#2220)

Omnigent relay tools surfaced into Hermes (mcp_omnigent_* / mcp__omnigent__*)
are already policy-gated when the relay dispatches them back through the
server's tool path. The pre_tool_call hook evaluated them a second time, parking
a duplicate approval card per call; a human resolves one and the other's
long-poll never returns, wedging the turn after the approved tool runs. Skip
those prefixes in the hook, matching the guard the native claude/codex hooks
already apply. Hermes' own tools (shell, file) and non-Omnigent MCP servers lack
the prefix and stay gated.

Signed-off-by: rdosen <robert.dosen@gmail.com>
Signed-off-by: Aditya Devarapalli <adityareddyd2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-demo PR needs a demo screenshot or recording no-doc-update Merged PR does not need a docs update size/M Pull request size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hermes pre_tool_call hook double-gates Omnigent relay tools (duplicate approval card wedges the turn)

3 participants