Plugin hook policy: hung or raising guards fail closed without wedging, block outranks approve, shell approve reaches the gate, async pre_gateway_dispatch awaited (#105223 #109624 #87420 #92553 #105827 #110241) - #118844
Merged
Conversation
Contributor
૮ >ﻌ< ა ci reviewran on 6505462 — fix(plugins): ainvoke_hook shares the sync path's failure co debug infoCI timingsCI timings · View report · View jobWall time 5m47s vs 6m24s (-9.6%). 6 job(s) slower, 4 faster, 3 unchanged.
|
This was referenced Sep 22, 2026
) `_run_execution_chain` read `get_plugin_manager()._middleware` directly, bypassing the lazy discovery every other delivery entry point gained in the #64178 parity work, so `tool_execution`/`llm_execution` middleware registered by user plugins silently failed to fire (fail-open) on surfaces that never run discovery at startup: query mode `chat -q`/`-z`, cron delivery, dashboard, TUI slash workers. Route it through `_delivery_manager()` like invoke_hook/invoke_middleware/has_middleware. (cherry picked from commit 06b9d44) Fixes #105827 Salvages #105832
A `pre_tool_call` guard that raised failed OPEN (only `_report_hook_failure`, results stayed empty, the tool ran) while one that timed out failed CLOSED with a block directive. For a veto hook a crashing guard is the control quietly disappearing; make both failure modes of `_HOOK_TIMEOUT_FAIL_CLOSED_HOOKS` consistent: an exception appends a block directive in addition to the warning. Fixes #109624 Salvages #109632 (cherry picked from commit 6ccf5d8)
Follow-up trim of the #109632 salvage: the block directive a raising `pre_tool_call` guard produces reused the timeout wording, so an operator could not tell a crashing guard from a slow one from the tool result. Build it from the callback name and `TypeError: ...` (error text truncated like `_report_hook_failure`). The two salvaged tests collapse into one parametrized invariant (caller-thread and bounded-worker path) that also pins the message shape and that a sibling callback's result still flows. Part of #109624 Salvages #109632
…call until restart `_run_hook_callback_bounded` treated any live abandoned worker for a callback (`bool(self._hook_abandoned.get(suppression_key))`) as "still running", so one never-returning `pre_tool_call` callback made every later tool call fail closed with the timeout message until the process restarted. The timeout path self-heals through the 60s suppression window; the abandoned path never did. Policy now: while the suppression window is open the callback is skipped as before. After it expires a fresh call id may start a new worker even though the abandoned one is still alive — capped at `_HOOK_MAX_ABANDONED_WORKERS` (3) live abandoned workers per callback so a hung plugin cannot leak a thread per call (the #98382 constraint). At the cap the callback keeps being skipped (fail-closed for pre_tool_call) with a WARNING naming the callback and its module, until one of its workers finishes and frees a slot. Test changes: `test_hung_worker_blocks_new_call_identity_after_suppression` encoded the removed behaviour (exactly one worker, forever); it becomes `test_hung_worker_caps_new_call_identities_after_suppression`, which pins the same invariant it was protecting — bounded leak, never one per call — at the new bound and checks the warning. `test_hung_worker_does_not_fail_closed_forever` is the #105223 regression (red on base: call-c returned the block directive). Redone slim against the per-call-id gate that landed in #111177; #105241 targeted the pre-#111177 shape and needed plugins_ledger/__init__ changes for a one-retry-then-quarantine policy. Its analysis and shape informed this fix. Fixes #105223 Supersedes #105241 Co-authored-by: fangliquanflq <fangliquan@qq.com>
`_get_pre_tool_call_directive_details` returned the first valid block-or-approve in registration order, so a plugin registered earlier that returned `approve` hid a later security plugin's `block`; under `approvals.mode: off` an approve means no prompt at all, so the veto was dropped silently. Precedence is now `block` > `approve` > none: a valid block still returns immediately (modify directives seen before it stay attached, as before), a valid approve is held back until the whole result list has been scanned for a veto, and among approves the first valid one (with its rule_key) still wins. Modify accumulation is unchanged and now also keeps modify directives that follow the winning approve, since the scan no longer stops there. Docstring and hooks.md no longer describe "first valid directive wins". Slim redo of #68644 (earliest) and #87449 against the modify-aware shape of the function on main; both PRs predate it and could not be cherry-picked. Fixes #87420 Supersedes #68644 Supersedes #87449 Co-authored-by: synscott <1563043+synscott@users.noreply.github.com> Co-authored-by: Jack Lau <72348727+jackulau@users.noreply.github.com>
…te with "approve"
`agent/shell_hooks.py::_parse_pre_tool_call` translated only the block and
modify dialects, so a shell hook printing the documented
`{"action": "approve", ...}` parsed to None and the tool ran with no approval
prompt — silently, with exit 0, valid JSON and `hermes hooks doctor` green.
The Python-plugin side already accepts approve and routes it through
`_resolve_block_from_details` → `request_tool_approval`; the shell parser now
yields the same `{"action": "approve", "message"?, "rule_key"?}` shape (optional
fields kept only as non-empty stripped strings), so `hermes hooks test` prints
it under `parsed:` and the dispatcher escalates it. the `decision` dialect's
`{"decision": "approve"}` means auto-ALLOW, not "ask a human", so it is
deliberately not mapped; that dialect has no top-level ask dialect to mirror.
Slim redo with credit: #92562 (earliest) bundled a larger policy-authority
rework; #110325 carried the same parser change plus an unrelated rule_key
default change and 10+ tests.
Fixes #92553
Supersedes #92562
Supersedes #110325
Co-authored-by: fangliquanflq <fangliquan@qq.com>
… loop `GatewayInboundMixin._hm_pre_gateway_dispatch_hook` was a plain `def` calling the sync `hermes_cli.lifecycle.invoke_hook` from the async `_hm_admit_event`, so an `async def pre_gateway_dispatch` callback was resolved through `resolve_plugin_command_result` on a helper thread with its own loop: the gateway loop blocked for the callback's whole duration and any loop-bound await (an `asyncio.Event` set by a loop task, a loop-bound aiohttp session, `asyncio.to_thread`) could never complete, failing at 30s. Add `PluginManager.ainvoke_hook` (+ `hermes_cli.plugins.ainvoke_hook` / `hermes_cli.lifecycle.ainvoke_hook`): same payload narrowing (shared `_hook_callback_kwargs`), observer + isolation semantics and result contract as `invoke_hook`, but awaitable results are awaited on the caller's loop. `pre_gateway_dispatch` stays intentionally unbounded. The inbound hook becomes `async def` and `_hm_admit_event` awaits it; the sync `invoke_hook` is untouched for every other caller. Existing tests that stubbed the hook synchronously are adapted to the async seam. Fixes #110241 Salvages #110265 (cherry picked from commit 22bb10d)
Follow-up trim of the #110265 salvage. `ainvoke_hook` logged raising callbacks with a bare warning; route them through `_report_hook_failure` (warn-once per distinct failure, #111922) and, for `_HOOK_TIMEOUT_FAIL_CLOSED_HOOKS`, append the same named block directive the sync path emits (#109624), so the async twin cannot drift into a fail-open policy path. Tests trimmed to the salvage bar: the in-loop await is proven once through the real `_handle_message` path (`test_async_hook_callback_is_awaited_on_the_gateway_loop`); the manager-level duplicate is dropped and the narrowing test also pins failure isolation. Docs: `pre_gateway_dispatch` callbacks may be `async def` and stay unbounded. Credit order for the three PRs fixing this gap: #102485 (dmspark, earliest, pre-decomposition `gateway/run.py`), #110253 (KoNit-K, bounded the hook — rejected by design: neither fail mode is acceptable for a policy gate), #110265 (twidtwid, reporter; cherry-picked because it matches the ainvoke_hook shape, keeps the hook unbounded, and adapts the existing sync test seams honestly). Part of #110241 Supersedes #102485 Supersedes #110253 Co-authored-by: David Marcus <dmspark@users.noreply.github.com> Co-authored-by: KoNit-K <konit.block@protonmail.com>
teknium1
force-pushed
the
fix/hooks-policy
branch
from
September 22, 2026 06:41
2479005 to
6505462
Compare
This was referenced Sep 22, 2026
This was referenced Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Guard/policy plugins can no longer be silently defeated on the hook path: a raising or hung
pre_tool_callguard fails closed without wedging the agent until restart, a plugin's veto outranks another plugin's approve, shell hooks can escalate to the approval gate, execution middleware fires on cold surfaces, and asyncpre_gateway_dispatchcallbacks run on the gateway loop.plugins_dispatch.py::_run_hook_callback_bounded: after the 60s suppression window a fresh call id may respawn a worker for a hung callback, capped at_HOOK_MAX_ABANDONED_WORKERS = 3live abandoned workers per callback; at the cap it keeps failing closed with a WARNING naming callback + module (the Concurrent observer-hook invocations are dropped as if a callback had timed out #98382 "no unbounded workers" constraint holds). Supersedes fix(plugins): recover tool calls after a hung policy guard #105241 (@fangliquanflq, co-authored), fix(plugins): per-session hook single-flight + matcher gate before the flight window #109441, fix(plugins): serialize overlapping hook callback fires instead of dropping #104763.pre_tool_callhook callbacks fail OPEN on exception but fail CLOSED on timeout — inconsistent policy-hook contract #109624 apre_tool_callcallback that raises now appends a block directive naming the callback and error — the same veto a timeout already produced, so exception and timeout no longer disagree. Salvages fix(plugins): fail closed on policy hook exceptions #109632 (@KoNit-K).plugins.py::_get_pre_tool_call_directive_details:block>approve> none regardless of registration order; first valid approve wins among approves;modifyaccumulation unchanged. Supersedes fix(plugins): give block directives precedence #68644 (@synscott, earliest, co-authored), fix(plugins): rank pre_tool_call block above approve #87449 (@jackulau, co-authored).shell_hooks.py::_parse_pre_tool_calltranslates{"action":"approve","message"?,"rule_key"?}to the plugin-side directive; thedecision: approvedialect (auto-allow) deliberately unmapped. Supersedes fix(hooks): stop shell hook approval requests from bypassing confirmation #92562 (@fangliquanflq, co-authored), feat(hooks): support approval escalation from shell hooks #110325 (@Mauridios)._run_execution_chainmissing lazy discovery #105827middleware.py::_run_execution_chainuses_delivery_manager()so tool/llm execution middleware fires in query/cron processes where discovery has not run. Salvages fix(plugins): route execution middleware through lazy discovery (#105827) #105832 (@liuhao1024).ainvoke_hook(plugins_dispatch→plugins→lifecycle);_hm_pre_gateway_dispatch_hookis async and awaited on the gateway loop;pre_gateway_dispatchstays unbounded; the syncinvoke_hookis untouched for every other caller. Salvages fix(gateway): await async pre_gateway_dispatch callbacks on the event loop #110265 (@twidtwid); supersedes feat(plugins): await async pre-gateway dispatch hooks #102485 (@dmspark, earliest, co-authored), fix(gateway): await async pre-dispatch hooks #110253 (@KoNit-K, co-authored).hooks.md): precedence wording, timeout/raise/recovery paragraph, shellapprovestdout shape, asyncpre_gateway_dispatchnote.Root cause in one sentence: the dispatcher treated an abandoned worker as "running forever", treated a raising guard as "no opinion", picked the first approve/block in registration order, and only resolved async callbacks through a helper thread.
Validation
test_hung_worker_does_not_fail_closed_forever,test_hung_worker_caps_new_call_identities_after_suppressionresults=[]— tool ran (fail-open)[{'action':'block','message':'pre_tool_call plugin callback boom raised RuntimeError: …'}]test_pre_tool_call_callback_exception_fails_closed[2]('approve', 'earlier plugin approves')('block', 'later security plugin blocks')test_later_block_outranks_earlier_approve,test_first_approve_wins_among_approves_and_keeps_later_modifyNone, tool ran unprompted{'action':'approve','message':…}→ human gatetest_approve_is_parsed_like_the_plugin_directive[4],test_approve_reaches_the_human_gate_through_plugin_managertest_execution_chain_lazily_discoversskiplostskiphonouredtest_async_hook_callback_is_awaited_on_the_gateway_loop,test_narrow_legacy_signature_still_gets_only_its_fieldsExisting-test changes (justified in commit bodies):
test_hung_worker_blocks_new_call_identity_after_suppressionre-pinned at the new bound (#105223); #109632's two tests merged into one parametrized test; three sync hook stubs intests/gateway/test_bot_loop_guard.pymade async (#110241). No other existing assertion changed.Lane run
scripts/run_tests.sh tests/hermes_cli tests/agent/test_shell_hooks.py tests/gateway: 22775 passed, 13 failed — all 13 in untouched files, 9 pass in isolation (40-worker load), 4test_dashboard_auth_gateSystemExit: 75fail identically on a pristine base worktree (port held on the host). Post-rebase onee8a919fd27: touched test files 163 passed, 0 failed.Prompt-cache invariant: nothing here mutates past context or the system prompt mid-conversation.
Fixes #105223. Fixes #109624. Fixes #87420. Fixes #92553. Fixes #105827. Fixes #110241.
Salvages #105832, #109632, #110265. Supersedes #105241, #109441, #104763, #68644, #87449, #92562, #110325, #102485, #110253.
Infographic