2 Commits

Author	SHA1	Message	Date
João Moura	d729cade6b	fix(openai): surface gateway errors reported inside an HTTP 200 (#7342) ... * 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 on a68b5e903 too, 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 against a53ecc17f, 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>	2026-09-09 15:12:57 -07:00
João Moura	4fdb7f2bfb	fix(tools)!: make tool-result caching opt-in instead of on by default (#6509) ... * fix(tools)!: make tool-result caching opt-in instead of on by default Tool-result caching defaulted to on (Crew.cache=True, and standalone agents self-wired a CacheHandler at construction), so an LLM calling the same tool with identical arguments twice in one run silently got the first result back without the tool executing. For live-data tools that is a confidently stale answer; for state-mutating tools the second action is silently dropped. Caching is now opt-in with the machinery unchanged: - Crew.cache defaults to False; Crew(cache=True) restores today's behavior exactly (agents still default to participating when a crew offers its handler, and Agent(cache=False) still opts an agent out). - Standalone agents no longer self-wire a cache; Agent(cache=True) or an explicit cache_handler opts in. Previously even Crew(cache=False) agents cached via this self-wired handler. - Per-tool cache_function write gating is unchanged once opted in. Existing tests that exercised the caching machinery now opt in explicitly; new regression tests cover the default (both identical calls execute), crew-level opt-in dedup, and agent-level wiring. Fixes EPD-180. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(agent): don't let copy() turn the cache default into an explicit opt-in Agent.copy() rebuilds from model_dump(), which includes the field default cache=True, so the copy's model_fields_set contained "cache" and _setup_agent_executor wired a CacheHandler the source agent never opted into (Bugbot review finding). Drop "cache" from the dump when it was not explicitly set on the source; explicit opt-ins still survive copying. Also sync the Crew and BaseAgent class docstrings with the new opt-in cache semantics (CodeRabbit review findings). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(agent): preserve cache_handler-only opt-in across Agent.copy() copy() excludes cache_handler from the rebuilt agent, so an agent that opted into tool-result caching solely via an explicit cache_handler lost caching after copy() (Bugbot review finding). Carry the consent as cache=True on the copy when the source has a handler wired and hasn't explicitly disabled caching — the copy wires its own fresh handler, matching pre-change copy semantics (copies never shared the source's handler instance). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(crew): offer the crew cache handler to the hierarchical manager The hierarchical manager agent is created in _create_manager_agent, outside the validation-time agents loop that offers the crew's cache handler — and managers no longer self-wire a handler — so Crew(cache=True) hierarchical runs never cached the manager's delegation tool calls (Bugbot review finding). Offer the shared crew handler when the crew opted in; a user-provided manager with cache=False stays excluded via the existing set_cache_handler gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(agent): only construction-time cache opt-ins survive Agent.copy() The previous copy() fix treated any wired cache_handler as consent, but agents that merely received the crew's shared handler at kickoff (set_cache_handler from Crew(cache=True)) never opted in themselves — their copies must not become standalone cachers (Bugbot review finding). Record the opt-in signal in _setup_agent_executor, which runs at construction before any crew wiring can happen, and have copy() consult that flag instead of inspecting cache_handler after the fact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-07-10 17:37:36 -07:00