Two per-call WARNING surfaces were still outside the warn-once reporter:
- agent/plugin_stream_hooks.py::_worker — the on_stream_start/on_stream_delta/
on_stream_end consumer, which fires once per streaming delta (far more often than
per tool call). A mis-declared callback (signature naming tool_data) logged
"Hook ... raised" at WARNING on every delta: 20 deltas -> 20 WARNING lines.
- hermes_cli/plugins_dispatch.py::_deliver_event — plugin event subscribers that
raise identically were warned on every emit.
Both now go through PluginManager._report_hook_failure (keyed by module/qualname,
cleared on unload): the first failure warns and names the fields the hook/event
provides, identical repeats are DEBUG. Skip-and-continue semantics are unchanged.
Part of #111922
Review follow-up on the warn-once hook reporter. Middleware (agent_tool_execution
etc.) runs once per tool call exactly like a hook, so a mis-declared middleware
callback flooded WARNING identically; route its except through the same
_report_hook_failure helper with a "Middleware" surface label.
The reported-failure set was keyed on (hook, id(cb), repr(exc)) and never
cleared: a hook whose message embeds tool args grew it by one entry per call,
and id() recycling across plugin reloads could swallow a reloaded callback's
first failure. Key on (hook, module, qualname, exc type, str(exc)[:200]) and
clear the set in _reset_after_unload_all next to _hook_timeout_suppressed_until.
Part of #111922
A plugin callback whose signature names a parameter the hook never sends
(on_pre_tool(tool_data) where core provides tool_name/args) raises the same
TypeError on every tool call; core logged a WARNING each time — ~1700 lines an
hour in the report, burying the freeze signature it was trying to find
(finding (c) of #111922). The mis-declared signature is the plugin's bug; the
per-call repeat is ours.
invoke_hook now reports one WARNING per distinct (hook, callback, error) and
names the fields the hook actually provides so the author can fix the
signature; identical repeats are logged at DEBUG. A callback failing in a new
way still warns.
Part of #111922
The timeout branch of _run_hook_callback_bounded unconditionally added
gate_key to _hook_abandoned. A worker that finishes between done.wait()
returning False and the caller taking the lock has already popped its
token via _release_token, so nothing would ever clear that entry: the
callback stayed blocked for every later call id until reload with no
thread behind it. Guard the insert on the worker still being registered.
The new test makes the race deterministic by swapping the module's
threading.Event for one whose wait() lets the worker finish and then
reports a timeout, and asserts a fresh call id still runs.
Also pass tool_call_id inline from terminal_tool_result instead of the
conditional dict plumbing: an empty id is already treated as "no
identity" by _hook_call_identity and unknown fields are withheld from
narrow-signature callbacks (same shape as _fire_approval_hook). Update
the stale "(hook_name, id(cb))" comment above _hook_running_callbacks.
Gating hook callbacks by call identity lets two concurrent calls of the
same tool both run their hooks, but it also let a fresh tool_call_id pass
the gate once the 60 s suppression window lapsed even though the previous
worker for that callback never returned. A hung plugin then leaked one
daemon thread per minute for the life of the process; on the old
coarse-keyed gate it leaked exactly one.
Track abandoned-but-running workers per callback: the timeout branch
records the gate key, the worker's own release discards it, and the gate
treats any non-empty abandoned set as "still running" for that callback.
Healthy callbacks keep distinct-id concurrency; hung ones are back to
at most one outstanding worker.
Concurrent invocations of the same tool in one session collapsed into a single
busy key (hook_name, id(cb)): the second invocation was reported as 'still
running' and dropped. For pre_tool_call a drop is a fail-closed block, so the
gate silenced itself on an ordinary, healthy callback.
Measured on a busy profile: 3574 skip lines and 0 timeout lines in one hour —
every skip was the 'while still running' branch, i.e. pure key collision, not
slowness.
The gate now keys on the call identity that is already in the payload
(tool_call_id, else turn_id, else none — the last case behaves exactly as
before). Suppression stays keyed coarsely on (hook_name, id(cb)): a hung
callback is a fact about the callback, so its back-off must not be diluted
per call.
Refs #98382. Independent of #107894 (that one releases the slot on timeout;
this one stops healthy concurrency from colliding).
(cherry picked from commit 53b3dacd008418fcdf5fa6dfcadde575a35a776e)
Slash-command handlers gained loop-safe awaiting in ca9a61ae38, but
`PluginManager.invoke_hook` still called `async def` hook callbacks directly:
the coroutine object was appended to the results (so `pre_llm_call` context
injection silently did nothing) and Python warned "coroutine was never
awaited". `_invoke_hook_callback` now routes every return through
`resolve_plugin_command_result`, which covers both the direct and the
timeout-bounded paths and is safe under the gateway's running loop.
Fixes#12449 (remaining hook half). Salvage of #63240 by @Bartok9, applied
one layer down so the bounded-worker path is covered too.
Co-authored-by: Bartok9 <Bartok9@users.noreply.github.com>
The identity-guarded token pop appeared twice (the runner's finally and the
new worker-start except); a future edit to one copy would silently reintroduce
the sticky-token class. Hoist into a local closure called from both sites, and
extend the _run_hook_callback_bounded docstring with the new skip reason.
Behavior-preserving follow-up on #104651.
For each issue anchor present in BASE 63279301bc non-test .py and absent on HEAD, the BASE comment/docstring block was re-attached at the HEAD location of the code it explained (matched by the distinctive code line / enclosing def). Sentences already covered by an existing HEAD comment were deduped; the issue number always survives. Insert-only: no code lines changed.
