mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-09-20 01:55:38 +00:00
* fix(openai): surface gateway errors reported inside an HTTP 200 OpenAI-compatible gateways commit `200 OK` as soon as the upstream provider accepts a request, so a later provider failure arrives in the body as an `error` object with no `choices`. That reached the SDK's parse helper and surfaced as `TypeError: 'NoneType' object is not iterable`, naming neither the provider, the status, nor the fact that a timeout happened. The four non-streaming paths now inspect the raw body before parsing and raise the exception the upstream code maps to, so a masked 504 is catchable exactly like an honest one. Streaming already had this guard inside the SDK. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(openai): teach the tool-cache fake about with_raw_response The provider now reads the raw body before parsing, so a client double that only implements `create` no longer satisfies it. Same shape as the fixes to the reasoning-effort retry and Snowflake doubles. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(tracing): reset the TraceCollectionListener singleton between tests TraceCollectionListener caches a TraceBatchManager on the class and `_initialized` short-circuits `__init__`, so batch state survives for the whole xdist worker. `test_nested_agent_executor_flow_does_not_finalize_parent_batch` left `trace_batch_id="debug-trace-batch"` behind, which moved every later trace POST from /tracing/ephemeral/batches to /tracing/batches/<id>/events. The recorded cassette then stopped matching, the agent retried, and the second call found the cassette consumed -- surfacing as ConnectionError in an unrelated test hundreds of tests later. Reproduced deterministically by running the leaking test followed by tests/tracing/test_trace_enable_disable.py::test_trace_calls_when_enabled_via_env; fails ona68b5e903too, so this predates the gateway fix it was blocking. An autouse fixture now clears the cached instance after each test. Two canaries pin the invariant and fail without the fixture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(tracing): drop the unwritable _listeners_setup canary Both review bots flagged that the canary read `_listeners_setup` off the class, where it is always False, so it could never fail. Correct, and the suggested fix does not work either: `BaseEventListener.__init__` calls `setup_listeners` (base_event_listener.py:16), which sets the flag on the instance (trace_listener.py:229), so reading it back through `TraceCollectionListener()` is always True. Neither read observes a leak, so the canary is deleted rather than replaced, with the reasoning recorded so it is not re-added. The same finding showed the fixture was resetting two class attributes that are never assigned at class level. Only dropping `_instance` is load-bearing, so the fixture is now one line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(tracing): correct why the _listeners_setup canary is unwritable setup_listeners returns early when tracing is off and no override applies (trace_listener.py:213-220), assigning the flag at :229 only when it actually registers. Construction therefore does not always set it, as the previous note claimed: with tracing disabled the flag never even reaches the instance dict. The instance read reports ambient tracing state rather than isolation, which is a better reason not to assert on it than the one recorded before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tests): clear trace batch state in place instead of dropping the singleton Dropping `TraceCollectionListener._instance` made the next construction re-run `setup_listeners`, re-registering its handlers on the event bus. That broke tests/telemetry/test_task_failure_instrumentation.py, which requires exactly one handler per event: the re-registered `on_task_failed` made two. Verified againsta53ecc17f, where the same sequence passes -- the regression was mine. The leak that needed fixing was batch state, not registration, so the fixture now clears the manager's batch fields in place. Handler cleanup already belongs to `cleanup_event_handlers`, and `first_time_handler` keeps its reference to the same manager object. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(tests): clear tracing context vars so the listener can re-register Bugbot flagged that keeping the singleton leaves `_listeners_setup` set, so after `cleanup_event_handlers` wipes the bus `setup_listeners` returns early (trace_listener.py:208) and tracing silently registers nothing for the rest of the worker. Confirmed: after a tracing-enabled run, re-running setup restores 0 of 119 handler entries. Dropping the singleton fixes that but previously broke test_task_failure_instrumentation. The real cause was a third leak: the `_tracing_enabled` context var stayed set, so the replacement listener still believed tracing was on and re-registered `on_task_failed` next to telemetry's. Clearing the context vars is what makes replacing the listener safe, so the fixture now does both, and a canary pins it (fails with `assert True is False` without the drop). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
282 lines
9.9 KiB
Python
282 lines
9.9 KiB
Python
# mypy: ignore-errors
|
|
"""Regression tests for EPD-180: tool-result caching used to be ON by default,
|
|
so an LLM calling the same tool with identical arguments twice in one run got
|
|
the first (possibly stale) result back without the tool executing — silently
|
|
wrong for live-data tools, and silently dropped actions for stateful tools.
|
|
|
|
Caching is now opt-in: ``Crew(cache=True)`` for crews, ``Agent(cache=True)``
|
|
(or an explicit ``cache_handler``) for standalone agents. The machinery —
|
|
including per-tool ``cache_function`` write gating — is unchanged once opted
|
|
in.
|
|
|
|
The end-to-end tests run fully offline: a fake OpenAI client scripts two
|
|
identical tool calls followed by a final answer, mirroring the EPD-180
|
|
clean-room repro.
|
|
"""
|
|
|
|
import json
|
|
from types import SimpleNamespace
|
|
|
|
import httpx
|
|
from openai.types.chat import ChatCompletion
|
|
from pydantic import BaseModel, Field
|
|
|
|
from crewai import LLM, Agent, Crew, Task
|
|
from crewai.agents.cache.cache_handler import CacheHandler
|
|
from crewai.tools import BaseTool
|
|
|
|
|
|
class LookupArgs(BaseModel):
|
|
city: str = Field(description="City to look up.")
|
|
|
|
|
|
def make_live_tool():
|
|
"""A tool returning a different value on every real execution."""
|
|
executions = []
|
|
|
|
class LiveLookupTool(BaseTool):
|
|
name: str = "live_lookup"
|
|
description: str = "Returns a live (time-varying) reading for a city."
|
|
args_schema: type[BaseModel] = LookupArgs
|
|
# cache_function deliberately NOT set — exercising the default.
|
|
|
|
def _run(self, city: str) -> str:
|
|
executions.append(city)
|
|
return f"reading #{len(executions)} for {city}"
|
|
|
|
return LiveLookupTool(), executions
|
|
|
|
|
|
def make_scripted_llm():
|
|
"""An offline LLM whose client scripts two identical tool calls."""
|
|
|
|
def tool_call_response(call_id: str):
|
|
return {
|
|
"index": 0,
|
|
"finish_reason": "tool_calls",
|
|
"message": {
|
|
"role": "assistant",
|
|
"content": None,
|
|
"tool_calls": [
|
|
{
|
|
"id": call_id,
|
|
"type": "function",
|
|
"function": {
|
|
"name": "live_lookup",
|
|
"arguments": '{"city": "paris"}',
|
|
},
|
|
}
|
|
],
|
|
},
|
|
}
|
|
|
|
scripted = [
|
|
tool_call_response("call_1"),
|
|
tool_call_response("call_2"), # identical name+args, new id
|
|
{
|
|
"index": 0,
|
|
"finish_reason": "stop",
|
|
"message": {"role": "assistant", "content": "Final answer: done."},
|
|
},
|
|
]
|
|
|
|
class FakeCompletions:
|
|
def __init__(self):
|
|
self.n = 0
|
|
|
|
@property
|
|
def with_raw_response(self):
|
|
"""The provider reads the raw body to spot upstream errors that a
|
|
gateway reported inside an HTTP 200."""
|
|
return self
|
|
|
|
def create(self, **params):
|
|
choice = scripted[min(self.n, len(scripted) - 1)]
|
|
self.n += 1
|
|
payload = {
|
|
"id": f"chatcmpl-fake-{self.n}",
|
|
"object": "chat.completion",
|
|
"created": 1,
|
|
"model": params.get("model", "gpt-4o"),
|
|
"choices": [choice],
|
|
"usage": {
|
|
"prompt_tokens": 10,
|
|
"completion_tokens": 5,
|
|
"total_tokens": 15,
|
|
},
|
|
}
|
|
return SimpleNamespace(
|
|
text=json.dumps(payload),
|
|
parse=lambda: ChatCompletion.model_validate(payload),
|
|
http_response=httpx.Response(
|
|
200,
|
|
request=httpx.Request(
|
|
"POST", "https://api.openai.com/v1/chat/completions"
|
|
),
|
|
),
|
|
)
|
|
|
|
class FakeClient:
|
|
def __init__(self):
|
|
self.chat = type("Chat", (), {"completions": FakeCompletions()})()
|
|
|
|
llm = LLM(model="openai/gpt-4o")
|
|
llm._client = FakeClient()
|
|
return llm
|
|
|
|
|
|
def run_crew(**crew_kwargs):
|
|
tool, executions = make_live_tool()
|
|
agent = Agent(
|
|
role="reader",
|
|
goal="Look things up.",
|
|
backstory="Test agent.",
|
|
llm=make_scripted_llm(),
|
|
tools=[tool],
|
|
verbose=False,
|
|
)
|
|
task = Task(
|
|
description="Look up paris twice and report.",
|
|
expected_output="A report.",
|
|
agent=agent,
|
|
)
|
|
crew = Crew(agents=[agent], tasks=[task], verbose=False, **crew_kwargs)
|
|
crew.kickoff()
|
|
return executions
|
|
|
|
|
|
class TestToolCachingIsOptIn:
|
|
def test_default_reexecutes_identical_tool_calls(self):
|
|
"""EPD-180: with no opt-in, both identical calls must really execute."""
|
|
executions = run_crew()
|
|
assert len(executions) == 2
|
|
|
|
def test_crew_cache_true_dedupes_identical_tool_calls(self):
|
|
"""Opting in via Crew(cache=True) restores the dedup behavior."""
|
|
executions = run_crew(cache=True)
|
|
assert len(executions) == 1
|
|
|
|
|
|
class TestAgentCacheWiring:
|
|
def _agent(self, **kwargs) -> Agent:
|
|
return Agent(
|
|
role="reader",
|
|
goal="Look things up.",
|
|
backstory="Test agent.",
|
|
**kwargs,
|
|
)
|
|
|
|
def test_standalone_agent_has_no_cache_by_default(self):
|
|
agent = self._agent()
|
|
assert agent.tools_handler.cache is None
|
|
assert agent.cache_handler is None
|
|
|
|
def test_standalone_agent_explicit_cache_true_opts_in(self):
|
|
agent = self._agent(cache=True)
|
|
assert agent.tools_handler.cache is not None
|
|
assert agent.cache_handler is not None
|
|
|
|
def test_standalone_agent_explicit_cache_handler_opts_in(self):
|
|
handler = CacheHandler()
|
|
agent = self._agent(cache_handler=handler)
|
|
assert agent.tools_handler.cache is handler
|
|
|
|
def test_explicit_cache_false_stays_off_even_with_handler(self):
|
|
agent = self._agent(cache=False, cache_handler=CacheHandler())
|
|
assert agent.tools_handler.cache is None
|
|
|
|
def test_agents_accept_a_crew_offered_handler_by_default(self):
|
|
"""``Crew(cache=True)`` offers its handler via set_cache_handler at
|
|
kickoff; agents that didn't explicitly opt out must accept it."""
|
|
agent = self._agent()
|
|
assert agent.tools_handler.cache is None
|
|
|
|
handler = CacheHandler()
|
|
agent.set_cache_handler(handler)
|
|
assert agent.tools_handler.cache is handler
|
|
|
|
def test_agents_that_opted_out_refuse_a_crew_offered_handler(self):
|
|
agent = self._agent(cache=False)
|
|
agent.set_cache_handler(CacheHandler())
|
|
assert agent.tools_handler.cache is None
|
|
|
|
def test_copy_of_default_agent_does_not_opt_in(self):
|
|
"""copy() rebuilds from model_dump(), which includes the field
|
|
default cache=True — that must not read as an explicit opt-in on
|
|
the copy (Bugbot review finding on the original PR)."""
|
|
copied = self._agent().copy()
|
|
assert copied.tools_handler.cache is None
|
|
assert copied.cache_handler is None
|
|
|
|
def test_copy_of_opted_in_agent_stays_opted_in(self):
|
|
copied = self._agent(cache=True).copy()
|
|
assert copied.tools_handler.cache is not None
|
|
|
|
def test_copy_of_handler_opted_in_agent_stays_opted_in(self):
|
|
"""An explicit cache_handler is an opt-in too; copy() excludes the
|
|
handler itself, but the consent must survive — the copy wires its
|
|
own fresh handler (Bugbot review finding on the original PR)."""
|
|
source = self._agent(cache_handler=CacheHandler())
|
|
copied = source.copy()
|
|
assert copied.tools_handler.cache is not None
|
|
assert copied.tools_handler.cache is not source.tools_handler.cache
|
|
|
|
def test_copy_of_explicit_cache_false_with_handler_stays_off(self):
|
|
copied = self._agent(cache=False, cache_handler=CacheHandler()).copy()
|
|
assert copied.tools_handler.cache is None
|
|
|
|
def test_copy_of_crew_wired_agent_does_not_opt_in(self):
|
|
"""A handler offered by a crew at kickoff (set_cache_handler) is
|
|
runtime wiring, not construction-time consent — copies of such
|
|
agents must not become standalone cachers (Bugbot review finding
|
|
on the original PR)."""
|
|
agent = self._agent()
|
|
agent.set_cache_handler(CacheHandler()) # what Crew(cache=True) does
|
|
assert agent.tools_handler.cache is not None
|
|
|
|
copied = agent.copy()
|
|
assert copied.tools_handler.cache is None
|
|
assert copied.cache_handler is None
|
|
|
|
|
|
class TestHierarchicalManagerCacheWiring:
|
|
"""The auto-created hierarchical manager is built outside the agents
|
|
loop that offers the crew's cache handler; an opted-in crew must wire
|
|
the manager too (Bugbot review finding on the original PR)."""
|
|
|
|
def _crew(self, **crew_kwargs) -> Crew:
|
|
from crewai.process import Process
|
|
|
|
agent = Agent(role="worker", goal="Do work.", backstory="Test agent.")
|
|
task = Task(description="Do the work.", expected_output="Done.")
|
|
return Crew(
|
|
agents=[agent],
|
|
tasks=[task],
|
|
process=Process.hierarchical,
|
|
manager_llm="gpt-4o",
|
|
**crew_kwargs,
|
|
)
|
|
|
|
def test_manager_gets_crew_handler_when_cache_enabled(self):
|
|
crew = self._crew(cache=True)
|
|
crew._create_manager_agent()
|
|
assert crew.manager_agent.tools_handler.cache is crew._cache_handler
|
|
|
|
def test_manager_has_no_cache_when_crew_did_not_opt_in(self):
|
|
crew = self._crew()
|
|
crew._create_manager_agent()
|
|
assert crew.manager_agent.tools_handler.cache is None
|
|
|
|
def test_user_provided_manager_with_cache_false_stays_excluded(self):
|
|
manager = Agent(
|
|
role="manager",
|
|
goal="Manage.",
|
|
backstory="Test manager.",
|
|
cache=False,
|
|
allow_delegation=True,
|
|
)
|
|
crew = self._crew(cache=True)
|
|
crew.manager_agent = manager
|
|
crew._create_manager_agent()
|
|
assert manager.tools_handler.cache is None
|