fix(tracing): mark the agent span when a non-streaming run fails - #4073
fix(tracing): mark the agent span when a non-streaming run fails#4073hsusul wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77b93795b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The streamed run loop attaches an "Error in agent run" SpanError to the active agent span, but the non-streaming failure handler only recorded run_exception, so Runner.run() and Runner.run_sync() left the agent span unmarked. Failures raised outside a generation or function span, such as a lifecycle hook error, produced a trace with no error at all. Attach the same SpanError from the non-streaming handler. The span is left alone when it already carries a more specific error, such as "Max turns exceeded", and cancellation is excluded because it is not an agent failure. Move the shared exclusion predicate into error_handlers so both paths use one source of truth.
77b9379 to
7a1dab6
Compare
seratch
left a comment
There was a problem hiding this comment.
Before merge, please move the complete generic agent-error attachment policy into a run_internal helper used by both streaming and non-streaming paths. That helper should own eligibility, preservation of an existing span error, redaction, message construction, and attachment, leaving run.py with a single orchestration-level call.
Please also ensure that redacted tracing never evaluates str(exc) and that any exception-formatting failure cannot replace the original run exception. Add a regression test using an exception whose __str__ raises and assert that the original exception still propagates. Keep the existing ModelBehaviorError and guardrail exclusion policy for this PR; broader coverage of currently unmarked ModelBehaviorError paths should be handled separately.
Move the complete generic agent-error attachment into run_internal.error_handlers.attach_generic_agent_error, which owns eligibility, preservation of an existing span error, redaction, message construction, and the attach itself. run.py and both streaming handlers now make a single call instead of repeating the policy inline, so the two paths cannot drift. Stringify the exception only when sensitive data is traced, and never let a failing __str__ escape: the formatting error is logged and recorded as a placeholder so tracing cannot replace the exception the run is propagating. Add regression tests, for the streamed and non-streamed paths, that raise an exception whose __str__ raises and assert the original exception still propagates.
|
Thanks, all four points are addressed in 43f8c95. Policy moved into a shared run_internal helper. attach_generic_agent_error(span, exc, *, trace_include_sensitive_data) in run_internal/error_handlers.py now owns the complete policy: eligibility (including the isinstance(exc, Exception) cancellation exclusion that previously lived in run.py), preservation of an existing span error, redaction, message construction, and the attach. run.py's failure handler is a single call, and both streaming handlers in run_loop.py call the same helper, so there is one implementation of the exclusion list, message, and redaction instead of parallel blocks. The previously exported predicate is now private to the helper. One behavior note: the streamed path now also preserves a more specific span error, which it did not before. Its snapshots are unchanged, because its double-attach across the nested handlers was writing the identical value twice. Redacted tracing never evaluates str(exc). The exception is stringified only inside the trace_include_sensitive_data=True branch; the redacted branch returns the constant. Formatting goes through _format_agent_error_detail, which catches Exception (not BaseException), logs a warning naming only the exception class, and returns "Error details are unavailable." — so an exception-formatting failure can never replace the exception the run is propagating. Regression tests. UnformattableError.str raises and counts its calls; RaisingHooks raises it from on_agent_start, i.e. user code inside the agent span (FakeModel stringifies its own exception output before raising, so it can't carry this case). test_run_propagates_exception_whose_str_raises and test_streamed_run_propagates_exception_whose_str_raises assert exc_info.value is error on both paths and that the span records the placeholder. test_redacted_tracing_never_stringifies_the_exception calls the helper directly with redaction on and asserts str_calls == 0 — a unit test rather than end-to-end because the pre-existing sandbox-memory finally block calls terminal_metadata_for_exception(run_exception), which stringifies independently (inside its own try/except, so propagation is unaffected). Exclusion policy unchanged. ModelBehaviorError and the guardrail tripwires stay excluded; the broader unmarked-ModelBehaviorError coverage is left for a separate PR. Re-ran on the updated branch: make format, make lint, make typecheck (mypy 833 files, pyright 0 errors), make tests (5971 passed, 3 skipped + 45 passed, 4 skipped serial), make tests-asyncio-stability 5/5, and the tracing tests on Python 3.10. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43f8c959eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| try: | ||
| return str(exc) | ||
| except Exception: |
There was a problem hiding this comment.
Catch BaseException from trace-only formatting
When a custom exception's __str__ raises KeyboardInterrupt, SystemExit, or asyncio.CancelledError, this except Exception does not catch it, so the trace-only formatter replaces the original run exception despite the helper's stated guarantee. The fresh centralized helper now guards ordinary formatter failures, but it should catch BaseException specifically around str(exc) so every secondary formatting failure is reduced to the placeholder without changing run semantics.
AGENTS.md reference: AGENTS.md:L124-L124
Useful? React with 👍 / 👎.
Summary
Runner.run()andRunner.run_sync()left the active agent span unmarked when a run failed, whileRunner.run_streamed()attached anError in agent runSpanError. Exported traces from non-streaming runs therefore under-reported failures, and failures raised outside a generation or function span produced a trace with no error marker anywhere.Affected component:
src/agents/run.py(non-streaming failure handler), with the shared exclusion predicate moved intosrc/agents/run_internal/error_handlers.pyand reused bysrc/agents/run_internal/run_loop.py.Root cause
start_streaming()inrun_loop.pywraps its turn loop and outer body inexcept Exception as e:handlers that call:The corresponding block in
run.pyisexcept BaseException as exc:, which assignsrun_exception, attachesRunErrorDetailstoAgentsExceptions, and re-raises. It never touchescurrent_span; thefinallyblock then callscurrent_span.finish(reset_current=True)with no error recorded.Parity gap between execution paths
The gap was already visible in the committed snapshots —
tests/test_tracing_errors.py::test_single_turn_model_errorhad an agent span with noerrorkey, whiletests/test_tracing_errors_streamed.py::test_single_turn_model_errorhad"error": {"message": "Error in agent run", "data": {"error": "test error"}}.I mapped the behavior of the streamed path per exception type and used it as the reference for the fix:
ValueErrorError in agent runError in agent runModelRefusalErrorError in agent runError in agent runUserErrorError in agent runError in agent runAgentsExceptionError in agent runError in agent runModelBehaviorErrorGuardrail tripwire triggeredGuardrail tripwire triggeredMaxTurnsExceededMax turns exceededMax turns exceededRunner.run_sync()delegates to the sameAgentRunner.run()code path, so it is fixed by the same change and is covered by its own test.Behavioral change
A non-streaming run that fails now marks the active agent span with the same
SpanErrormessage and data shape the streamed path uses, honouringRunConfig.trace_include_sensitive_datathrough the existing_error_tracing.get_trace_error()helper. No new error schema is introduced.The attach is guarded by three conditions:
current_span.error is None— a more specific error already on the span (Max turns exceeded,Guardrail tripwire triggered) is never overwritten, and the generic error can never be applied twice.SpanImpl.set_error()overwrites unconditionally, so this guard is what makes both properties hold.isinstance(exc, Exception)— the handler catchesBaseException, butasyncio.CancelledErroris not an agent failure and the streamed path (which catches onlyException) never marks it.should_attach_generic_agent_error(exc)— the existing exclusion list, soModelBehaviorErrorand guardrail tripwires keep their more specific reporting.Everything else is untouched: the exception still propagates unchanged,
run_exceptionassignment andRunErrorDetailsattachment are unchanged, and successful runs never enter this branch._should_attach_generic_agent_errormoved fromrun_loop.pyintoerror_handlers.pyasshould_attach_generic_agent_errorso both paths share one definition of the exclusion list rather than duplicating it.run_loop.pynow imports it; its two call sites are otherwise unchanged.Tests added
In
tests/test_tracing_errors.py, using the repository's existingFakeModelandSPAN_PROCESSOR_TESTING/fetch_span_errorsutilities:test_run_marks_agent_span_with_generic_error—Runner.run()marks the agent span, and the originalValueErrorstill propagates (pytest.raises(ValueError, match="test error")).test_run_sync_marks_agent_span_with_generic_error— same throughRunner.run_sync().test_run_agent_span_error_matches_streamed_path— runs the identical failure throughRunner.run()andRunner.run_streamed()and asserts both produce the same agent-span error.test_run_agent_span_error_redacts_sensitive_data— mirrors the streamedtest_streamed_agent_error_redacts_sensitive_data; withtrace_include_sensitive_data=Falsethe detail is"Error details are redacted.".test_run_does_not_mark_agent_span_for_model_behavior_error— the excluded case stays clean.test_run_marks_agent_span_for_other_agents_exceptions[model-refusal-error|user-error]— non-excludedAgentsExceptionsubclasses match the streamed path.test_run_keeps_specific_max_turns_agent_span_error— a pre-existing, more specificMax turns exceedederror is preserved rather than overwritten.test_run_attaches_agent_span_error_exactly_once— wrapsSpanImpl.set_errorviamonkeypatchand asserts exactly one error is recorded on the agent span.test_successful_run_leaves_agent_span_without_error— successful runs are unchanged.All tests are deterministic: no sleeps, no concurrency, no network, and no API key.
Snapshot changes. Two committed inline snapshots change, both by exactly one added line on the agent span:
in
test_single_turn_model_errorandtest_multi_turn_no_handoffs. Nothing else in those snapshots moves — the generation span error, agent data, tools and children are byte-identical, and the added line makes each snapshot match its streamed counterpart. I inspected both diffs and hand-applied the line in the repo's existing formatting rather than accepting the tool's rewrite, because--inline-snapshot=fixalso reflowed several unrelated lines to a narrower width.Pre-fix proof. With only the three
src/agents/files reverted to upstreammainand the tests in place:The three no-regression guards (
..._model_behavior_error,..._max_turns_agent_span_error,..._successful_run_...) pass both before and after. All 18 pass with the fix.Compatibility
No public API, exception type, or serialized-state change. Exported traces for failing non-streaming runs gain an agent-span error they previously lacked; consumers that assert the absence of that field would see the new value, which is the documented parity intent in
.agents/references/runner-lifecycle.md("Streaming and non-streaming paths must produce equivalent … guardrail results, session history, and interruption state for the same model behavior"). Successful runs are byte-identical.Non-goals. The streamed path's own double-attach across its nested handlers, guardrail-span error wording, and any change to
run_exceptionorRunErrorDetailssemantics.Test plan
Run from the repository root on
fix/4070-nonstreaming-agent-span-error(Python 3.12.13, macOS 15.7.3):make format842 files left unchanged;ruff check --fix→All checks passed!make lintAll checks passed!make typecheckSuccess: no issues found in 833 source files; pyright0 errors, 0 warnings, 0 informationsmake tests5968 passed, 3 skipped, 2 warnings(parallel) and45 passed, 4 skipped, 5971 deselected(serial)make tests-asyncio-stabilitygit diff --checkuv run pytest tests/test_tracing_errors.py tests/test_tracing_errors_streamed.py -q29 passeduv run pytest tests/test_tracing_errors.py tests/test_tracing_errors_streamed.py tests/test_agent_tracing.py tests/test_trace_processor.py tests/test_agent_runner.py tests/test_agent_runner_sync.py -q274 passed(run 5× consecutively, no flakes)UV_PROJECT_ENVIRONMENT=.venv_310 uv run --python 3.10 -m pytest tests/test_tracing_errors.py tests/test_tracing_errors_streamed.py tests/test_agent_tracing.py -q55 passedNo OpenAI API key, network access, or paid model call was used; the tests rely on
tests/fake_model.py::FakeModeland the in-repoSpanProcessorForTests.Not run:
make integration-tests*(requires live provider credentials and external services) andmake build-docs(no documentation files changed). No lockfile or dependency changes.Issue number
Fixes #4070
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR