fix: run after_llm_call hooks on async provider paths - #6737
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAsync OpenAI, Anthropic, and Bedrock handlers now invoke after-call hooks. Anthropic async handlers also retain thinking blocks, with regression tests covering hook behavior, structured-output fallbacks, and multi-turn thinking replay. ChangesAsync LLM provider paths
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 |
The native providers invoked `_invoke_after_llm_call_hooks` only from their sync handlers, so a hook registered to redact, rewrite or audit a response was silently skipped for every async direct call -- the unmodified response reached the caller with nothing logged. Eight handlers were missing the call their twin makes: openai's four async handlers, anthropic's two, bedrock's `_ahandle_converse`, and -- with the polarity inverted -- bedrock's sync `_handle_streaming_converse`. That last one sits in the same file as `_ahandle_streaming_converse`, which does call the hook, which is what shows this is drift rather than a decision about async. Each call is placed where its twin already places it, and the async streaming handler for openai keeps the same `isinstance(result, str)` guard so structured tool-call payloads are not stringified. Also fixes a second defect in the same code: anthropic's two async handlers never called `_extract_thinking_block`, leaving `_previous_thinking_blocks` empty. `_format_messages_for_anthropic` replays that list into the assistant message on the next turn when thinking is enabled, so the async path dropped the signed thinking block that the sync path preserves. `_invoke_after_llm_call_hooks` returns the response unchanged when `from_agent` is not None, so the agent-driven path -- which dispatches POST_MODEL_CALL separately via `agent_utils._setup_after_llm_call_hooks` -- is unaffected. Fixes crewAIInc#6736
f065483 to
119706a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/crewai/src/crewai/llms/providers/openai/completion.py (2)
2636-2651: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not apply LLM hooks to string tool results.
_finalize_streaming_responsecan return a string fromavailable_functions; thisisinstanceguard then sends that tool output throughafter_llm_callhooks. That differs from the async non-streaming and sync paths and can corrupt tool results through redaction or rewriting. Hook only the finalized model-text path, or return an explicit result type indicating its origin.🤖 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/openai/completion.py` around lines 2636 - 2651, The streaming completion flow after _finalize_streaming_response must not apply _invoke_after_llm_call_hooks to strings originating from available_functions. Distinguish finalized tool outputs from finalized model text, and invoke hooks only for the model-text result while returning tool results unchanged; keep non-string finalized responses unchanged.
2636-2651: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHook the structured-output streaming fallback.
When
response_modelvalidation fails, the earlier branch returnsaccumulated_contentdirectly, so this new tail is never reached. A direct async call can therefore return an unhooked string; route that fallback through_invoke_after_llm_call_hooks(...)before returning.🤖 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/openai/completion.py` around lines 2636 - 2651, Update the response-model validation fallback in the streaming completion flow so its accumulated string output is passed through _invoke_after_llm_call_hooks(...) before returning. Ensure the direct async path receives the same hook treatment while preserving the existing structured-result return behavior, and use the nearby finalization logic in the completion method as the implementation reference.
🧹 Nitpick comments (1)
lib/crewai/tests/hooks/test_llm_hooks.py (1)
869-886: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the follow-up request instead of the private formatter.
Calling
_format_messages_for_anthropictests implementation details and can pass even ifacall()later stops sending those blocks. Capture the second stubbedmessages.create()arguments, make a secondacall(), and assert its assistant content contains the signed thinking block.🤖 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 869 - 886, Update the test to verify the follow-up request through the public acall() flow instead of directly calling the private _format_messages_for_anthropic method. Capture the second messages.create() invocation from the stubbed client, perform a second acall(), and assert that its assistant content includes the signed thinking block with the expected type, thinking text, and signature.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.
Inline comments:
In `@lib/crewai/src/crewai/llms/providers/anthropic/completion.py`:
- Around line 1621-1633: Update the Anthropic response handling around the
tool-use early returns so content-block extraction and
`_previous_thinking_blocks` persistence occur before any return for tool calls.
Ensure async non-streaming responses containing signed thinking blocks plus tool
calls retain those blocks for the next turn, and add a regression test covering
this combination.
In `@lib/crewai/src/crewai/llms/providers/bedrock/completion.py`:
- Around line 1429-1433: Update the empty Bedrock response handling in the
completion flow so it assigns the fallback string to the normal response
variable instead of returning early. Ensure async direct calls continue through
the shared completion/event path and reach _invoke_after_llm_call_hooks for
auditing or rewriting.
In `@lib/crewai/tests/hooks/test_llm_hooks.py`:
- Around line 717-764: Expand TestAfterLLMCallHooksOnAsyncPaths.llm and its
supporting stubs to cover Bedrock and OpenAI Responses async handlers, plus
streaming cases. Include a streaming structured tool-call result and assert
after_llm_call hooks still run while preserving the streaming type guard; retain
the existing non-streaming OpenAI Chat Completions and Anthropic coverage.
---
Outside diff comments:
In `@lib/crewai/src/crewai/llms/providers/openai/completion.py`:
- Around line 2636-2651: The streaming completion flow after
_finalize_streaming_response must not apply _invoke_after_llm_call_hooks to
strings originating from available_functions. Distinguish finalized tool outputs
from finalized model text, and invoke hooks only for the model-text result while
returning tool results unchanged; keep non-string finalized responses unchanged.
- Around line 2636-2651: Update the response-model validation fallback in the
streaming completion flow so its accumulated string output is passed through
_invoke_after_llm_call_hooks(...) before returning. Ensure the direct async path
receives the same hook treatment while preserving the existing structured-result
return behavior, and use the nearby finalization logic in the completion method
as the implementation reference.
---
Nitpick comments:
In `@lib/crewai/tests/hooks/test_llm_hooks.py`:
- Around line 869-886: Update the test to verify the follow-up request through
the public acall() flow instead of directly calling the private
_format_messages_for_anthropic method. Capture the second messages.create()
invocation from the stubbed client, perform a second acall(), and assert that
its assistant content includes the signed thinking block with the expected type,
thinking text, and signature.
🪄 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: 1d701871-86c8-4f35-82bc-e71cd5dc14a5
📒 Files selected for processing (4)
lib/crewai/src/crewai/llms/providers/anthropic/completion.pylib/crewai/src/crewai/llms/providers/bedrock/completion.pylib/crewai/src/crewai/llms/providers/openai/completion.pylib/crewai/tests/hooks/test_llm_hooks.py
| llm = OpenAICompletion(model="gpt-4o", api_key="stub", stream=False) | ||
| llm._client = SimpleNamespace(chat=SimpleNamespace(completions=_Completions())) | ||
| llm._async_client = SimpleNamespace( | ||
| chat=SimpleNamespace(completions=_AsyncCompletions()) | ||
| ) | ||
| return llm | ||
|
|
||
|
|
||
| def _stub_anthropic_llm(): | ||
| from types import SimpleNamespace | ||
|
|
||
| from crewai.llms.providers.anthropic.completion import AnthropicCompletion | ||
|
|
||
| class _Messages: | ||
| def create(self, **kwargs: object) -> object: | ||
| return _anthropic_response("the model said SECRET") | ||
|
|
||
| class _AsyncMessages: | ||
| async def create(self, **kwargs: object) -> object: | ||
| return _anthropic_response("the model said SECRET") | ||
|
|
||
| llm = AnthropicCompletion( | ||
| model="claude-sonnet-4-5", | ||
| api_key="stub", | ||
| thinking={"type": "enabled", "budget_tokens": 1024}, | ||
| max_tokens=2048, | ||
| stream=False, | ||
| ) | ||
| llm._client = SimpleNamespace(messages=_Messages()) | ||
| llm._async_client = SimpleNamespace(messages=_AsyncMessages()) | ||
| return llm | ||
|
|
||
|
|
||
| class TestAfterLLMCallHooksOnAsyncPaths: | ||
| """``acall()`` must run after_llm_call hooks exactly as ``call()`` does. | ||
|
|
||
| Regression: the native providers invoked ``_invoke_after_llm_call_hooks`` | ||
| only from their sync handlers, so a hook registered to redact or rewrite a | ||
| response was silently skipped for every async direct call — the response | ||
| reached the caller unmodified with nothing logged. | ||
| """ | ||
|
|
||
| @pytest.fixture(params=["openai", "anthropic"]) | ||
| def llm(self, request): | ||
| return { | ||
| "openai": _stub_openai_llm, | ||
| "anthropic": _stub_anthropic_llm, | ||
| }[request.param]() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Cover the remaining changed async handler paths.
This fixture matrix only exercises non-streaming OpenAI Chat Completions and Anthropic Messages. Add cases for Bedrock, OpenAI Responses, and streaming—including a structured tool-call result—to validate the PR’s affected-handler and streaming-type-guard contracts.
🤖 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 717 - 764, Expand
TestAfterLLMCallHooksOnAsyncPaths.llm and its supporting stubs to cover Bedrock
and OpenAI Responses async handlers, plus streaming cases. Include a streaming
structured tool-call result and assert after_llm_call hooks still run while
preserving the streaming type guard; retain the existing non-streaming OpenAI
Chat Completions and Anthropic coverage.
Source: Coding guidelines
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/src/crewai/llms/providers/openai/completion.py (1)
2636-2651: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winHook the structured-streaming text fallback.
When structured parsing fails, Line 2560 returns
accumulated_contentbefore this string guard. That raw fallback can bypass redaction/audit hooks; preserve the BaseModel guard but route the fallback string through the hook.Proposed fix
except Exception as e: logging.error(f"Failed to parse structured output from stream: {e}") self._emit_call_completed_event( response=accumulated_content, ... ) - return accumulated_content + return self._invoke_after_llm_call_hooks( + params["messages"], accumulated_content, from_agent + )🤖 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/openai/completion.py` around lines 2636 - 2651, Update the structured-streaming fallback in the surrounding response-finalization flow so the accumulated text returned when parsing fails is passed through _invoke_after_llm_call_hooks before returning. Preserve the existing BaseModel result guard and the current hook arguments used by the isinstance(result, str) path, ensuring both string outcomes are audited and redacted consistently.
🤖 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/src/crewai/llms/providers/openai/completion.py`:
- Around line 2636-2651: Update the structured-streaming fallback in the
surrounding response-finalization flow so the accumulated text returned when
parsing fails is passed through _invoke_after_llm_call_hooks before returning.
Preserve the existing BaseModel result guard and the current hook arguments used
by the isinstance(result, str) path, ensuring both string outcomes are audited
and redacted consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f9cbcfbf-54a1-4c2a-b778-740ac40f30d5
📒 Files selected for processing (4)
lib/crewai/src/crewai/llms/providers/anthropic/completion.pylib/crewai/src/crewai/llms/providers/bedrock/completion.pylib/crewai/src/crewai/llms/providers/openai/completion.pylib/crewai/tests/hooks/test_llm_hooks.py
When `response_model.model_validate_json` raises, `_ahandle_streaming_completion` returns the accumulated text from inside the `response_model` branch -- upstream of the hook call this PR added at the end of the method. So a direct async call whose structured output failed to parse returned raw model text with `after_llm_call` never running: nothing redacted, nothing audited. That text is the same kind of value every other path here hands to the hooks, and the method already reports it as an LLM_CALL immediately before returning it, so the hook has to see it. Only the async path is affected; the sync handler's `response_model` branch has no equivalent raw-text fallback. Two tests: the fallback runs the hook, and without a hook registered the text passes through untouched. Reverting only the source change fails exactly the first.
|
Went through all five findings against the code. One was a real bug and is fixed in Fixed: the structured-output streaming fallback (
|
2 participants
Summary
Fixes #6736.
after_llm_callhooks never ran onacall(). The native providers invoked_invoke_after_llm_call_hooksonly from their sync handlers, so a hook registered to redact,rewrite or audit a response was silently skipped for every async direct call — the unmodified
response reached the caller and nothing was logged.
Eight handlers were missing the call their twin makes:
_ahandle_responses:1068)_ahandle_streaming_responses:1376)_ahandle_completion:2008)_ahandle_streaming_completion:2298)_ahandle_completion:1104)_ahandle_streaming_completion:1307)_ahandle_converse:823)_handle_streaming_converse:1772)That last row is what shows this is drift rather than a decision about async: within one file
_ahandle_streaming_conversecalls the hook and its sync twin does not, with the polarityinverted relative to every other row.
Each call is placed where its twin already places it — after
_emit_call_completed_event— andopenai's async streaming handler keeps the same
isinstance(result, str)guard the sync one uses,so structured tool-call payloads are not stringified (#6529).
_invoke_after_llm_call_hooksreturns the response unchanged whenfrom_agentis notNone, sothe agent-driven path — which dispatches
POST_MODEL_CALLseparately viaagent_utils._setup_after_llm_call_hooks— is unaffected. No double dispatch.Second defect in the same code
anthropic's
_ahandle_completionand_ahandle_streaming_completionnever called_extract_thinking_block, so_previous_thinking_blocksstayed empty on the async path._format_messages_for_anthropic:851replays that list into the assistant message on the nextturn when
thinkingis enabled, so the async path dropped the signed thinking block that thesync path preserves — losing the reasoning context across turns.
test_anthropic_thinking_blocks_preserved_across_turnspins this forcall()only.Why this is worth fixing
after_llm_callis the documented interception point for the things that must not be skippable:redacting PII before a response is stored or displayed, enforcing an output policy, and audit
logging. A guardrail that works under
call()and silently stops working underacall()isworse than one that never worked, because the failure is invisible — no exception, no warning,
and the response looks plausible.
acall()is whatkickoff_async, async flows and any concurrent usage reach, so moving aworking crew from sync to async silently disabled every registered
after_llm_callhook.Type of change
Evidence
No network — the SDK client is stubbed and the same payload is served to both paths.
Before, on
main(3266932):After:
The hook was not merely failing to modify the response on the async path — it was never invoked,
so an observe-only hook (logging, auditing, metrics) recorded nothing either.
Tests
11 tests appended to
lib/crewai/tests/hooks/test_llm_hooks.py. No network; the existingclear_hooksautouse fixture handles isolation.TestAfterLLMCallHooksOnAsyncPaths::test_sync_call_runs_after_hook::test_async_call_runs_after_hook::test_async_call_without_hooks_is_unchanged::test_async_call_hook_returning_none_keeps_responseNoneobserves without replacingTestAnthropicThinkingBlocksOnAsyncPath::test_sync_call_stores_thinking_blocks::test_async_call_stores_thinking_blocks::test_async_thinking_blocks_are_replayed_on_the_next_turnThe negative controls matter: without them, a "fix" that unconditionally rewrote the response, or
that stuffed something into
_previous_thinking_blocksregardless of content, would pass.Control experiment
Reverting the three source files and keeping the tests:
Exactly the six tests that depend on the fix fail; the sync baselines, the negative control and
all 26 pre-existing tests in the file pass unchanged.
Suite runs
lib/crewai/tests/hooks/test_llm_hooks.py— 37 passed (26 pre-existing + 11 new)lib/crewai/tests/llms/anthropic/— 66 passed, 0 failedlib/crewai/tests/llms/openai/— 150 passed, 1 skipped, 0 failedlib/crewai/tests/llms/bedrock/— 35 passed, 8 skipped, 0 failedruff format --check—3 files already formattedruff check—All checks passed!The provider runs report teardown errors on my machine (
PermissionError: [WinError 32]whileshutil.rmtreeremovescrewai_test_storage/latest_kickoff_task_outputs.db) — a Windows sqlitefile-handle artifact in
conftest.py, present on unmodifiedmainand unrelated to this change.No test failed.
Additional notes
Scope: azure and gemini call the hook on neither path. That is a symmetric gap — those
providers do not support direct-call hooks at all — rather than sync/async drift, so I left it
out; it needs a separate decision. Happy to follow up if wanted.
Searched open PRs and issues for
after_llm_call,acall hooks,_invoke_after_llm_call_hooksand
thinking blocks async— no existing report or PR. The scan that found this and the testswere developed with AI assistance; every claim above was verified by running it.