From b47ae25868dc33c5c38e04c288e6eae1dfab94b4 Mon Sep 17 00:00:00 2001 From: LHMQ878 <72402929@cityu-dg.edu.cn> Date: Fri, 31 Jul 2026 08:59:58 +0800 Subject: [PATCH] fix: run before/after_llm_call hooks on the LiteLLM fallback's acall() crewai/llm.py is the fallback used whenever no native provider matches the model, and via is_litellm=True. Its call() dispatches both hook families (:1876 before, :1904 after); its acall() at :1959 dispatched neither. before_llm_call is the only interception point that can abort a call, so a hook returning False on the async path did not merely fail to observe the request -- the request was issued and billed. after_llm_call silently stopped rewriting, so a redaction hook returned the raw response. Probe on ebe0082, litellm.completion/acompletion stubbed so the observable is whether a request left the process: blocking before hook + rewriting after hook call before:1 after:0 issued:no -> ValueError: blocked by hook acall before:0 after:0 issued:async -> 'the model said something sensitive' observing before hook + rewriting after hook call before:1 after:1 issued:sync -> 'REDACTED BY HOOK' acall before:0 after:0 issued:async -> 'the model said something sensitive' acall() now mirrors its sync twin exactly: the before guard sits after the o1 role rewrite and before _prepare_completion_params, and the two handler calls bind to a result so the after dispatch can run on a str before returning. That restructuring is why the source diff is larger than two inserted lines -- the async body returned directly from each branch, with no join point to dispatch from. This is a different seam from the five provider acall() bodies in #6740 and the eight handlers in #6737: same defect class, different file, and not covered by either. #6740's body claimed this path already had the guard, which was wrong; that claim is corrected there. Adds TestLLMHooksOnTheLiteLLMFallbackAsyncPath. The stub records which litellm entry point was reached, so the blocking test asserts issued == [] -- a test that only asserted on the raised error would also pass on an implementation that ran the hook after the provider had been paid. Reverting only llm.py fails exactly the three async tests (3 failed / 2 passed); the sync test and the no-hook negative control pass either way by design. tests/hooks: 137 passed, up from 132. The 5 teardown errors are Windows tempdir cleanup in test_crew_scoped_hooks.py and test_interception_conformance.py, identical on a clean tree. --- lib/crewai/src/crewai/llm.py | 28 ++++-- lib/crewai/tests/hooks/test_llm_hooks.py | 119 +++++++++++++++++++++++ 2 files changed, 138 insertions(+), 9 deletions(-) diff --git a/lib/crewai/src/crewai/llm.py b/lib/crewai/src/crewai/llm.py index b0b5cd3a19..f9f06ca2e0 100644 --- a/lib/crewai/src/crewai/llm.py +++ b/lib/crewai/src/crewai/llm.py @@ -2015,6 +2015,9 @@ async def acall( msg_role: Literal["assistant"] = "assistant" message["role"] = msg_role + if not self._invoke_before_llm_call_hooks(messages, from_agent): + raise ValueError("LLM call blocked by before_llm_call hook") + with suppress_warnings(): if callbacks and len(callbacks) > 0: self.set_callbacks(callbacks) @@ -2024,7 +2027,16 @@ async def acall( ) if self._effective_stream(): - return await self._ahandle_streaming_response( + result = await self._ahandle_streaming_response( + params=params, + callbacks=callbacks, + available_functions=available_functions, + from_task=from_task, + from_agent=from_agent, + response_model=response_model, + ) + else: + result = await self._ahandle_non_streaming_response( params=params, callbacks=callbacks, available_functions=available_functions, @@ -2033,14 +2045,12 @@ async def acall( response_model=response_model, ) - return await self._ahandle_non_streaming_response( - params=params, - callbacks=callbacks, - available_functions=available_functions, - from_task=from_task, - from_agent=from_agent, - response_model=response_model, - ) + if isinstance(result, str): + result = self._invoke_after_llm_call_hooks( + messages, result, from_agent + ) + + return result except LLMContextLengthExceededError: raise except Exception as e: diff --git a/lib/crewai/tests/hooks/test_llm_hooks.py b/lib/crewai/tests/hooks/test_llm_hooks.py index 0c917712d4..459b574a83 100644 --- a/lib/crewai/tests/hooks/test_llm_hooks.py +++ b/lib/crewai/tests/hooks/test_llm_hooks.py @@ -2,6 +2,7 @@ from __future__ import annotations +from typing import Any from unittest.mock import Mock from crewai.hooks import ( @@ -670,3 +671,121 @@ def redact(ctx: LLMCallHookContext) -> str: ) assert result == "contains [REDACTED]" + + +def _litellm_response(text: str = "the model answered") -> Any: + """A litellm ModelResponse, which uses attribute access rather than a dict.""" + from litellm.types.utils import ModelResponse + + return ModelResponse( + **{ + "choices": [ + { + "message": {"content": text, "role": "assistant"}, + "finish_reason": "stop", + } + ], + "usage": {"prompt_tokens": 1, "completion_tokens": 1, "total_tokens": 2}, + } + ) + + +class TestLLMHooksOnTheLiteLLMFallbackAsyncPath: + """``crewai.llm.LLM`` -- the fallback -- must gate ``acall()`` as it gates ``call()``. + + The fallback is reached whenever no native provider matches the model, and by + ``is_litellm=True``. Its ``call()`` ran both hook families; ``acall()`` ran + neither, so a policy hook that blocked synchronously let the request out once + the caller switched to async, and a redaction hook stopped rewriting. + + ``base_llm.BaseLLM`` only defines the two ``_invoke_*`` helpers -- its own + ``acall`` raises ``NotImplementedError`` -- so this class is the only place + the fallback's async seam can be covered. + """ + + @pytest.fixture + def stub_litellm(self, monkeypatch: pytest.MonkeyPatch) -> list[str]: + """Replace both litellm entry points, recording which one was reached.""" + import litellm + + issued: list[str] = [] + + def completion(*_args: Any, **_kwargs: Any) -> Any: + issued.append("sync") + return _litellm_response() + + async def acompletion(*_args: Any, **_kwargs: Any) -> Any: + issued.append("async") + return _litellm_response() + + monkeypatch.setattr(litellm, "completion", completion) + monkeypatch.setattr(litellm, "acompletion", acompletion) + return issued + + @staticmethod + def _fallback_llm() -> Any: + """``is_litellm=True`` is required: without it the constructor returns a + native provider, and the fallback's own seam is never exercised.""" + from crewai.llm import LLM + + llm = LLM(model="gpt-4o-mini", is_litellm=True, stream=False) + assert type(llm) is LLM, f"expected the fallback, got {type(llm).__name__}" + return llm + + def test_sync_call_is_blocked_by_before_hook(self, stub_litellm: list[str]) -> None: + register_before_llm_call_hook(lambda ctx: False) + + with pytest.raises(ValueError, match="blocked by before_llm_call hook"): + self._fallback_llm().call("hi") + + assert stub_litellm == [] + + @pytest.mark.asyncio + async def test_async_call_is_blocked_by_before_hook( + self, stub_litellm: list[str] + ) -> None: + """The regression, and the reason it matters: the request went out. + + ``issued == []`` is the load-bearing assertion. Asserting only that the + call raised would also pass on an implementation that ran the hook after + the provider had already been paid. + """ + register_before_llm_call_hook(lambda ctx: False) + + with pytest.raises(ValueError, match="blocked by before_llm_call hook"): + await self._fallback_llm().acall("hi") + + assert stub_litellm == [] + + @pytest.mark.asyncio + async def test_async_call_runs_after_hook(self, stub_litellm: list[str]) -> None: + """A response-rewriting hook applies on the async path too.""" + register_after_llm_call_hook(lambda ctx: "REDACTED BY HOOK") + + result = await self._fallback_llm().acall("hi") + + assert result == "REDACTED BY HOOK" + assert stub_litellm == ["async"] + + @pytest.mark.asyncio + async def test_async_call_before_hook_sees_the_messages( + self, stub_litellm: list[str] + ) -> None: + seen: list[list[dict[str, Any]]] = [] + register_before_llm_call_hook(lambda ctx: seen.append(list(ctx.messages))) + + result = await self._fallback_llm().acall("hi") + + assert result == "the model answered" + assert stub_litellm == ["async"] + assert seen and seen[0][-1]["content"] == "hi" + + @pytest.mark.asyncio + async def test_async_call_without_hooks_is_unchanged( + self, stub_litellm: list[str] + ) -> None: + """Negative control: the seam is inert when nothing is registered.""" + result = await self._fallback_llm().acall("hi") + + assert result == "the model answered" + assert stub_litellm == ["async"]