feat: add generic interception-hook dispatcher#6516
Conversation
📝 WalkthroughWalkthroughThe PR adds a centralized interception-hook dispatcher with global and scoped registration, reducers, filtering, abort handling, telemetry, legacy LLM/tool adapters, executor integration, and dispatcher tests. ChangesHook dispatch integration
Sequence Diagram(s)sequenceDiagram
participant Executor
participant HookDispatcher
participant Hooks
participant HookDispatchedEvent
Executor->>HookDispatcher: dispatch interception point and context
HookDispatcher->>Hooks: resolve and run registered hooks
Hooks-->>HookDispatcher: proceed, modified context, or abort
HookDispatcher->>HookDispatchedEvent: emit dispatch metrics
HookDispatcher-->>Executor: return updated context or blocked result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
lib/crewai/tests/hooks/test_dispatch.py (1)
187-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing telemetry test for the
HookAbortedpath.
run_hooksexplicitly documents that telemetry is emitted in thefinallyblock beforeHookAbortedpropagates (withoutcome="aborted",abort_reason, andabort_sourcepopulated). No test inTestTelemetrycovers this path. Add a test that registers an aborting hook, captures events, and asserts the event reflectsoutcome == "aborted"with the correct reason and source.♻️ Suggested test for abort telemetry
assert events[0].hook_count == 1 + def test_event_reports_abort_outcome(self): + events: list[HookDispatchedEvent] = [] + + def blocker(ctx): + raise HookAborted(reason="blocked", source="policy") + + register(InterceptionPoint.INPUT, blocker) + + with crewai_event_bus.scoped_handlers(): + + `@crewai_event_bus.on`(HookDispatchedEvent) + def _capture(_source, event): + events.append(event) + + with pytest.raises(HookAborted): + dispatch(InterceptionPoint.INPUT, _Ctx()) + + assert len(events) == 1 + assert events[0].outcome == "aborted" + assert events[0].abort_reason == "blocked" + assert events[0].abort_source == "policy"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/tests/hooks/test_dispatch.py` around lines 187 - 215, Add a test in TestTelemetry alongside test_event_reports_outcome that registers a hook raising HookAborted with a known reason and source, captures HookDispatchedEvent through scoped_handlers, and asserts dispatch propagates the abort while emitting one event with interception_point "input", outcome "aborted", and matching abort_reason and abort_source.lib/crewai/src/crewai/hooks/dispatch.py (2)
237-243: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
_default_reducerreports "modified" even when payload wasn't applied.If
result is not Nonebutctxhas nopayloadattribute, nothing is mutated yet the function still returnsTrue, causing dispatch to record an inaccurate"modified"telemetry outcome. ReturnTrueonly when the payload is actually set.Proposed tweak
def _default_reducer(ctx: Any, result: Any) -> bool: """Default payload semantics: a non-None return replaces ``ctx.payload``.""" - if result is not None: - if hasattr(ctx, "payload"): - ctx.payload = result - return True + if result is not None and hasattr(ctx, "payload"): + ctx.payload = result + return True return False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/hooks/dispatch.py` around lines 237 - 243, Update _default_reducer so it returns True only when result is non-None and ctx has a payload attribute that is successfully assigned; return False when no payload attribute exists or result is None, preserving the existing payload replacement behavior.
398-416: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
on()registers the wrapped hook but returns the originalfunc, breakingunregister.When
agents/toolsfilters are present,register(point, hook)stores thefilteredwrapper, yet the decorator returnsfunc. A laterunregister(point, func)(orunregister_hook) will fail to remove the registered wrapper since it never matches the stored callable. Consider stashing the registered wrapper onfunc(e.g.func._registered_hook = hook) so unregistration can resolve it, or haveunregisteraccount for the wrapper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/hooks/dispatch.py` around lines 398 - 416, The decorator’s filtered path registers a wrapped hook but returns the original callable, so unregister cannot match the stored registration. Update decorator to retain the exact hook passed to register in a discoverable attribute on func, and update the corresponding unregister logic to resolve and remove that registered hook while preserving unfiltered and method behavior.lib/crewai/src/crewai/hooks/contexts.py (1)
21-31: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueFilter wrapper ignores
agent_role. The base context provides bothagentandagent_role, but_wrap_with_filtersindispatch.pyonly inspectsagent.role. For contexts whereagentisNonebutagent_roleis set (e.g. flow seams), theagents=filter won't apply. Consider having the filter fall back toagent_rolefor consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/src/crewai/hooks/contexts.py` around lines 21 - 31, The agent filter in _wrap_with_filters should use InterceptionContext.agent_role when context.agent is absent, while preserving the existing agent.role behavior when an agent exists. Update the agent-matching logic in dispatch.py so agents= filters apply consistently to contexts that provide only agent_role.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/src/crewai/utilities/tool_utils.py`:
- Around line 103-107: Update both blocked-call branches in
aexecute_tool_and_check_finality and execute_tool_and_check_finality to invoke
run_after_tool_call_hooks before returning the blocked ToolResult. Preserve the
existing blocked message and return behavior while matching the native tool-call
paths’ POST_TOOL_CALL handling at both referenced locations in
lib/crewai/src/crewai/utilities/tool_utils.py (103-107 and 206-210).
In `@lib/crewai/tests/hooks/test_dispatch.py`:
- Around line 221-229: Make test_noop_dispatch_overhead_budget non-gating by
marking it with the repository’s established slow or opt-in pytest marker, or
replace its absolute 5µs assertion with a relative baseline comparison against a
bare function call. Preserve validation that no-op dispatch overhead remains
bounded while avoiding dependence on absolute CI timing.
---
Nitpick comments:
In `@lib/crewai/src/crewai/hooks/contexts.py`:
- Around line 21-31: The agent filter in _wrap_with_filters should use
InterceptionContext.agent_role when context.agent is absent, while preserving
the existing agent.role behavior when an agent exists. Update the agent-matching
logic in dispatch.py so agents= filters apply consistently to contexts that
provide only agent_role.
In `@lib/crewai/src/crewai/hooks/dispatch.py`:
- Around line 237-243: Update _default_reducer so it returns True only when
result is non-None and ctx has a payload attribute that is successfully
assigned; return False when no payload attribute exists or result is None,
preserving the existing payload replacement behavior.
- Around line 398-416: The decorator’s filtered path registers a wrapped hook
but returns the original callable, so unregister cannot match the stored
registration. Update decorator to retain the exact hook passed to register in a
discoverable attribute on func, and update the corresponding unregister logic to
resolve and remove that registered hook while preserving unfiltered and method
behavior.
In `@lib/crewai/tests/hooks/test_dispatch.py`:
- Around line 187-215: Add a test in TestTelemetry alongside
test_event_reports_outcome that registers a hook raising HookAborted with a
known reason and source, captures HookDispatchedEvent through scoped_handlers,
and asserts dispatch propagates the abort while emitting one event with
interception_point "input", outcome "aborted", and matching abort_reason and
abort_source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aaeefb79-630e-47e9-83ba-8fe10cf99f7b
📒 Files selected for processing (12)
lib/crewai/src/crewai/agents/crew_agent_executor.pylib/crewai/src/crewai/events/types/hook_events.pylib/crewai/src/crewai/experimental/agent_executor.pylib/crewai/src/crewai/hooks/__init__.pylib/crewai/src/crewai/hooks/contexts.pylib/crewai/src/crewai/hooks/dispatch.pylib/crewai/src/crewai/hooks/llm_hooks.pylib/crewai/src/crewai/hooks/tool_hooks.pylib/crewai/src/crewai/llms/base_llm.pylib/crewai/src/crewai/utilities/agent_utils.pylib/crewai/src/crewai/utilities/tool_utils.pylib/crewai/tests/hooks/test_dispatch.py
Introduces `crewai/hooks/dispatch.py` as a single engine behind every interception point: a hook receives a typed context, may mutate or replace its `payload`, or raise `HookAborted(reason, source)` to stop the operation. The full `InterceptionPoint` catalog is frozen from day zero, with global and contextvar-scoped registries, an `@on` decorator, a no-op fast path, and a `HookDispatchedEvent` for telemetry. The four existing `before/after_llm_call` and `before/after_tool_call` hooks become adapters over the dispatcher, so the legacy dialect and `return False` semantics keep working unchanged while gaining the new contract.
lucasgomide
force-pushed
the
luzk/hooks-dispatcher
branch
from
8bc4fa2 to
a85e100
Compare
Corrects several dispatcher edge cases surfaced in review. `_default_reducer` now reports a modification only when a `payload` is actually applied, the `agents=` filter falls back to `agent_role` for contexts without an `agent` object, and `unregister` resolves the filter wrapper stashed by `on` so a filtered hook can be removed. The tool-hook runners honor the executing agent's `verbose` flag instead of silently swallowing hook errors, and the ReAct tool path now runs `POST_TOOL_CALL` on blocked calls to match the native paths. Also adds abort-telemetry coverage and replaces the flaky absolute no-op timing budget with a relative one.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f7bd240. Configure here.
|
|
||
| before_hooks = get_before_llm_call_hooks() | ||
| if not before_hooks: | ||
| if not get_before_llm_call_hooks(): |
There was a problem hiding this comment.
Scoped hooks skipped on direct LLM
Medium Severity
Direct LLM hook invocation still short-circuits when the global pre_model_call / post_model_call lists are empty, so execution-scoped hooks registered via scoped_hooks never run on that path even though dispatch would resolve them.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f7bd240. Configure here.
| # can resolve the filter wrapper. | ||
| func._registered_hook = hook # type: ignore[attr-defined] | ||
|
|
||
| return func |
There was a problem hiding this comment.
CrewBase @on methods never register
Medium Severity
The @on decorator skips global registration for methods with a self parameter and only sets _interception_point, but CrewBase still registers crew hooks by legacy is_*_hook markers, so those methods never run.
Reviewed by Cursor Bugbot for commit f7bd240. Configure here.
1 participant
CrewAI only exposed two interception points (LLM and tool calls), each a bespoke module, with no way to abort with a reason or intercept the rest of the run. This introduces a single dispatcher where a hook receives a typed context and may mutate or replace its
payloador raiseHookAborted(reason, source), freezing the fullInterceptionPointcatalog from day zero. The four existing hooks become adapters over it, so their decorators andreturn Falsesemantics keep working unchanged. First of three stacked PRs for the OSS-86 interception-hook catalog.Note
Medium Risk
Centralizes all LLM/tool interception on one dispatcher; behavior should stay compatible via reducers, but hook ordering, fail-open errors, and the new post-hook-on-block path in tool_utils affect every tool/LLM execution path.
Overview
Introduces
crewai.hooks.dispatchas the shared engine for interception: a frozenInterceptionPointcatalog,HookAborted, global +contextvars-scoped registration,dispatch/run_hooks, and@on(...)with optional agent/tool filters.Legacy LLM and tool hook lists are aliased to the dispatcher’s global queues so existing
register_*/ decorators and@on(PRE_MODEL_CALL)hooks run in one order. Point-specific reducers preservereturn Falseblocking and in-place message / result mutation.Call sites (
CrewAgentExecutor, experimental executor,agent_utils,tool_utils,base_llm) replace inline hook loops withrun_before_tool_call_hooks/run_after_tool_call_hooksand dispatcher-backed LLM paths.HookDispatchedEventtelemetry fires only when hooks actually run.tool_utilsnow runs post-tool hooks even when a before-hook blocks, aligning with native tool paths. Unit tests cover contract, shared queue, scoping, telemetry, and no-op overhead.Reviewed by Cursor Bugbot for commit f7bd240. Bugbot is set up for automated code reviews on this repo. Configure here.