fix: run before_llm_call hooks on async provider paths - #6740
Conversation
before_llm_call is the only interception point that can abort an LLM call, but none of the five native providers invoked it from acall(). A hook returning False therefore did not block anything on the async path -- the request was still issued and billed. Coverage on main: provider sync call() async acall() openai :465 missing anthropic :377 missing bedrock :380 missing gemini :316 missing azure :524 missing base_llm.py (the LiteLLM fallback) had the guard on both paths, so only users on the native providers were affected. Each acall() now runs its own sync twin's guard verbatim, right after the provider-specific message formatting. Gemini additionally needs _convert_contents_to_dict, as its sync path does, because its payload is Content objects rather than dicts. No double dispatch on the agent path: _invoke_before_llm_call_hooks returns True immediately when from_agent is not None, so the agent-executor dispatch stays the single source. This is the before counterpart to the after_llm_call fix in crewAIInc#6737, and the more severe half: a skipped after hook can only fail to rewrite a response, while a skipped before hook means a call the user explicitly blocked went out anyway. Adds TestBeforeLLMCallHooksOnAsyncPaths, parametrised over openai and anthropic. The stubbed clients record whether a request was issued, so the assertions cover the actual leak rather than the returned value. Includes a negative control proving no-hook behaviour is unchanged. Reverting only the source files fails exactly the three async-blocking tests on both providers (6 failed / 30 passed).
📝 WalkthroughWalkthroughNative Anthropic, Azure, Bedrock, Gemini, and OpenAI async completion paths now run ChangesAsync LLM hook gating
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProviderCompletion
participant BeforeLLMCallHook
participant ProviderClient
Caller->>ProviderCompletion: acall(messages)
ProviderCompletion->>BeforeLLMCallHook: invoke with formatted messages
BeforeLLMCallHook-->>ProviderCompletion: allow or block
ProviderCompletion->>ProviderClient: issue async completion request
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![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 (1)
lib/crewai/tests/hooks/test_llm_hooks.py (1)
718-751: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMutation test doesn't verify the mutated message reaches the provider request.
create/acreatestubs discard**_kwargs, sotest_async_call_before_hook_can_mutate_messagesonly provesctx.messageswas appended to, not that the appended message was included in the payload sent tocreate(). Capturing_kwargsin the stub and asserting on the capturedmessages/inputwould make this a stronger regression guard against the actual behavior it aims to protect.♻️ Proposed strengthening of the stub and assertion
def _stub_openai_llm(issued: list[str]) -> Any: """A real OpenAICompletion whose SDK client records every request.""" from crewai.llms.providers.openai.completion import OpenAICompletion llm = OpenAICompletion(model="gpt-4o", api_key="stub", stream=False) - def create(**_kwargs: Any) -> Any: + def create(**kwargs: Any) -> Any: issued.append("sync") + issued_kwargs.append(kwargs) return _openai_response() - async def acreate(**_kwargs: Any) -> Any: + async def acreate(**kwargs: Any) -> Any: issued.append("async") + issued_kwargs.append(kwargs) return _openai_response()Also applies to: 829-845
🤖 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_llm_hooks.py` around lines 718 - 751, Strengthen the hook mutation tests by having the create/acreate stubs in _stub_anthropic_llm and _stub_openai_llm capture their **_kwargs payloads instead of discarding them. Update test_async_call_before_hook_can_mutate_messages and the corresponding test near the second stub to assert that the mutated message appears in the provider request’s messages or input field, while preserving the existing sync/async tracking.
🤖 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/llms/providers/bedrock/completion.py`:
- Around line 513-516: Ensure recursive Bedrock follow-up requests are also
checked by _invoke_before_llm_call_hooks, preferably at the shared async request
boundary or immediately before _ahandle_converse() issues its recursive converse
call. Preserve blocking via the existing ValueError behavior, and add a
regression test covering a hook that blocks based on follow-up messages or tool
results.
In `@lib/crewai/tests/hooks/test_llm_hooks.py`:
- Around line 754-845: Extend TestBeforeLLMCallHooksOnAsyncPaths.provider and
its supporting stubs to include Bedrock, Gemini, and Azure alongside OpenAI and
Anthropic, ensuring each provider exercises the existing async blocking,
allowing, mutation, and no-hook tests while verifying blocked calls issue no
request.
---
Nitpick comments:
In `@lib/crewai/tests/hooks/test_llm_hooks.py`:
- Around line 718-751: Strengthen the hook mutation tests by having the
create/acreate stubs in _stub_anthropic_llm and _stub_openai_llm capture their
**_kwargs payloads instead of discarding them. Update
test_async_call_before_hook_can_mutate_messages and the corresponding test near
the second stub to assert that the mutated message appears in the provider
request’s messages or input field, while preserving the existing sync/async
tracking.
🪄 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: dc34ec28-c7ff-4ec8-a68c-a620fcc320c1
📒 Files selected for processing (6)
lib/crewai/src/crewai/llms/providers/anthropic/completion.pylib/crewai/src/crewai/llms/providers/azure/completion.pylib/crewai/src/crewai/llms/providers/bedrock/completion.pylib/crewai/src/crewai/llms/providers/gemini/completion.pylib/crewai/src/crewai/llms/providers/openai/completion.pylib/crewai/tests/hooks/test_llm_hooks.py
| if not self._invoke_before_llm_call_hooks( | ||
| formatted_messages, from_agent | ||
| ): | ||
| raise ValueError("LLM call blocked by before_llm_call hook") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Guard recursive Bedrock follow-up requests too.
acall() gates only the initial request. After tool execution, _ahandle_converse() recursively issues another async converse request without invoking _invoke_before_llm_call_hooks; a hook that blocks based on tool results or follow-up messages can therefore be bypassed. Apply the guard at the shared async request boundary or immediately before the recursive call, and add a regression test.
🤖 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/llms/providers/bedrock/completion.py` around lines 513
- 516, Ensure recursive Bedrock follow-up requests are also checked by
_invoke_before_llm_call_hooks, preferably at the shared async request boundary
or immediately before _ahandle_converse() issues its recursive converse call.
Preserve blocking via the existing ValueError behavior, and add a regression
test covering a hook that blocks based on follow-up messages or tool results.
The regression class this PR closes is per-provider -- each acall() had its own missing guard -- so testing two providers left the other three asserting nothing. TestBeforeLLMCallHooksOnAsyncPaths is now parametrised over openai, anthropic, bedrock, gemini and azure, all built the same way: a real completion object whose SDK client is a stub appending to an `issued` list, so each assertion is about whether the request went out. Two provider-specific details: - Bedrock's acall() raises NotImplementedError before reaching the hook when aiobotocore is absent, and aiobotocore is an optional extra. The gate under test is upstream of any AWS I/O and the client is a stub, so the stub builder patches AIOBOTOCORE_AVAILABLE rather than depending on the extra being installed. _async_client_initialized is preset so _ensure_async_client returns the stub instead of entering the exit stack. - Gemini serves both paths from one client with the async surface under .aio, matching _get_async_client delegating to _get_sync_client. test_async_call_runs_before_hook_that_allows now locates the prompt inside the last message instead of comparing to one provider's content layout, since the formatted shape differs (plain string, Converse content blocks, Gemini part dicts). Control experiment: reverting only the five source files fails exactly the three async-blocking tests on all five providers -- 15 failed / 36 passed, with the sync baseline, the negative control and all 26 pre-existing tests in the file passing unchanged.
|
Went through both findings against the code. The test-coverage one was right and is fixed in Fixed: coverage for all five providers
Correct, and it matters more here than it usually would: the regression class is per-provider -- each Two provider-specific details worth noting for review:
Control experiment, rerun at five providers: reverting only the five source files fails exactly the three async-blocking tests on every provider -- 15 failed / 36 passed -- with the sync baseline, the negative control and all 26 pre-existing tests in the file passing unchanged. Not changed: the recursive Bedrock follow-up
The behaviour is real. Two reasons it does not belong in this PR:
That second question is a real one and I don't think it should be answered silently inside a drift fix. Happy to file it separately with the line references above if maintainers agree it's worth closing; my read is that gating the follow-up turn is probably right, since a |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/crewai/tests/hooks/test_llm_hooks.py (1)
718-724: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAssert that mutated messages reach the provider payload.
All five async stubs discard their request kwargs, while the mutation test only checks the same in-memory
ctx.messageslist after the hook runs. It would pass even ifacall()ignored the mutation before invoking the provider. Capture each stub’s async request arguments and assert that"and be brief"is present in the actual provider payload.As per coding guidelines, unit tests for new functionality should focus on observable behavior rather than implementation details.
Also applies to: 737-743, 785-791, 831-837, 873-879, 973-989
🤖 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_llm_hooks.py` around lines 718 - 724, Update the async provider stubs at create/acreate sites, including the additional listed cases, to capture their request kwargs instead of discarding them. Extend the mutation test to inspect the captured provider payload after the hook runs and assert that the mutated message contains “and be brief,” verifying observable data sent to the provider rather than only ctx.messages.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@lib/crewai/tests/hooks/test_llm_hooks.py`:
- Around line 718-724: Update the async provider stubs at create/acreate sites,
including the additional listed cases, to capture their request kwargs instead
of discarding them. Extend the mutation test to inspect the captured provider
payload after the hook runs and assert that the mutated message contains “and be
brief,” verifying observable data sent to the provider rather than only
ctx.messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5735e79b-29d6-437e-8afc-55c24ef503d7
📒 Files selected for processing (1)
lib/crewai/tests/hooks/test_llm_hooks.py
The async tests only inspected ctx.messages after the hook ran, which is
the same list object the hook appended to. That assertion holds whatever
acall() does with it afterwards, so it would have passed even if the
guard were handed a copy and the mutation dropped -- exactly the failure
mode the tests exist to catch.
The provider stubs now record their request kwargs instead of discarding
them, and the mutation tests assert against what the stubbed client was
actually sent:
test_async_before_hook_mutation_reaches_the_provider_as_on_sync
pins async propagation to whatever the same provider's sync path
does. Whether a mutation reaches the payload at all is a
pre-existing per-provider property: four providers hand the guard
the payload list itself, while Gemini converts its Content objects
to dicts first and so hands over a copy on *both* paths. Asserting
async == sync states the invariant this PR owns -- acall() gates
and forwards exactly as call() does -- without claiming Gemini's
copy is right.
test_async_before_hook_mutation_reaches_the_provider
the symmetry check cannot distinguish "both propagate" from
"neither does", so this pins the four propagating providers by
name. Gemini is skipped, with its sync-path asymmetry named in the
skip reason rather than papered over.
test_async_call_runs_before_hook_that_allows additionally asserts the
prompt is in the recorded payload, so the hook is known to observe the
request that is really about to go out.
Reverting only the five provider files fails 18 tests -- both mutation
tests on the four propagating providers, plus the blocking and
allowing tests on all five -- and no sync test. 55 passed / 1 skipped
with the fix in place.
Signed-off-by: LHMQ878 <LHMQ878@users.noreply.github.com>
|
The payload-assertion finding is correct, and thanks for pushing on it — the test was weaker than I thought. Fixed in 9a9a9f9. The gap was real. What I did. The provider stubs now record their request kwargs via a small Whether a
Gemini differs because its guard is fed So the tests are split to say exactly that, and no more:
Discrimination check. Reverting only the five provider source files (tests untouched) fails 18 tests and no sync test: both mutation tests on the four propagating providers (8), plus the blocking and the allowing test on all five (10). With the fix in place: On the Bedrock recursion finding, my position from the earlier reply stands: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/crewai/tests/hooks/test_llm_hooks.py (1)
968-991: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a distinctive prompt for payload propagation checks.
_Recorder.payload_textisrepr(self.payloads), so a field name containing"hi"would satisfyassert "hi" in rec.payload_textwithout the prompt being present. The Gemini fixtures use thethinking_configfield, and the existing async mutation checks use"and be brief"for this exact reason. Use the same distinctive string here instead of"hi".🤖 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_llm_hooks.py` around lines 968 - 991, Update test_async_call_runs_before_hook_that_allows to use the distinctive prompt string “and be brief” instead of “hi” in the llm.acall invocation and corresponding assertions, including the seen message and rec.payload_text checks, so payload propagation cannot pass due to a field name containing “hi”.
🤖 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.
Nitpick comments:
In `@lib/crewai/tests/hooks/test_llm_hooks.py`:
- Around line 968-991: Update test_async_call_runs_before_hook_that_allows to
use the distinctive prompt string “and be brief” instead of “hi” in the
llm.acall invocation and corresponding assertions, including the seen message
and rec.payload_text checks, so payload propagation cannot pass due to a field
name containing “hi”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dd10941d-64b2-4e34-8057-50f219d7b8a1
📒 Files selected for processing (1)
lib/crewai/tests/hooks/test_llm_hooks.py
1 participant
Summary
before_llm_callhooks never ran on the asyncacall()path of any native provider. Becausebefore_llm_callis the interception point that can abort a call, a hook returningFalsedid not merely fail to observe the request — the request was actually issued and billed.This is the
beforecounterpart to #6736 / #6737 (after_llm_callon async paths), and the more severe half: anafterhook that is skipped can only fail to rewrite a response, whereas a skippedbeforehook means a call the user explicitly blocked still reached the provider.Fixes #6739
Root cause
Coverage of
_invoke_before_llm_call_hooksonmain(ebe0082):call()acall()(before)acall()(after this PR)openai/completion.py:465:603anthropic/completion.py:377:452bedrock/completion.py:380:513gemini/completion.py:316:402azure/completion.py:524:606llms/base_llm.py(the LiteLLM fallback) already had the guard on both paths, so users on the fallback path were unaffected -- which is likely why this went unnoticed.Correction (2026-07-31): that sentence was wrong, in a way that understates the bug.
llms/base_llm.pyonly defines_invoke_before_llm_call_hooks; it has no callers, and itsacall()israise NotImplementedError. The LiteLLM fallback iscrewai/llm.py, and therecall()guards both hook families (:1876,:1904) whileacall()at:1959guards neither. So the fallback has the same hole as the native providers rather than being the safe path, and nothing in this PR fixes it.Probe on unmodified
origin/main(ebe0082), withlitellm.completion/acompletionstubbed so the observable is whether a request left the process:LLM(model=..., is_litellm=True)is required to reach it -- without the flag the constructor returns a native provider, which is part of why I read the fallback as covered.I am leaving that fix out of this PR rather than adding it: it is a different file and a different seam from the five provider
acall()bodies under review here, and this PR already has review history against its current shape. It will follow separately. Flagging it here because "users on the fallback path were unaffected" is a claim a reviewer could reasonably act on, and it is not true.Proof, before any fix was written
A no-network probe that swaps the provider client for a stub recording whether a request was issued, so the observable is the HTTP call itself rather than the returned string:
After this PR, all four rows are identical:
The change
Each
acall()now runs its own sync twin's guard, verbatim, at the equivalent point in the body — right after the provider-specific message formatting and before the request parameters are assembled:Gemini needs the extra line its sync path uses at
:310, because its formatted payload isContentobjects rather than dicts:Deliberately not changed:
_invoke_before_llm_call_hooks(llms/base_llm.py:981) returnsTrueimmediately whenfrom_agent is not None, so the agent-executor dispatch remains the single source and adding the call toacall()cannot fire hooks twice.responsesdelegate returns before this block whenself.api == "responses", delegating to anOpenAICompletion— which now carries its own guard, so that path is covered too.Tests
lib/crewai/tests/hooks/test_llm_hooks.py, new classTestBeforeLLMCallHooksOnAsyncPaths, parametrised overopenaiandanthropic(10 tests total). The stubbed clients append to anissuedlist, so every assertion is about whether the request went out, not just what was returned.test_sync_call_is_blocked_by_before_hookissued == []test_async_call_is_blocked_by_before_hookacall()raises andissued == []test_async_call_runs_before_hook_that_allowsissued == ["async"])test_async_call_without_hooks_is_unchangedacall()behaves exactly as beforetest_async_call_before_hook_can_mutate_messagesctx.messagesis the one handed to the providerControl experiment
Reverting only the five source files and keeping the tests fails exactly the async-blocking tests and nothing else:
The 6 are the three async-blocking assertions × 2 providers. The sync baseline, the negative control and all 26 pre-existing tests in the file pass unchanged — the tests are pinned to this bug, not to incidental behaviour.
Verification
tests/hooks/test_llm_hooks.pytests/llms/anthropic+tests/llms/openaitests/llms/bedrock+google+azure+llms/hooksruff format --check/ruff checkon all 6 changed filesThe provider suites additionally report Windows-only teardown errors (not failures) —
PermissionError: [WinError 32]fromshutil.rmtreeoncrewai_test_storage/latest_kickoff_task_outputs.dbinconftest.py:223. These reproduce on cleanmainand are unrelated to this change; there are zero test failures.