[NA] [SDK] fix: stop anthropic stream wrappers from swallowing exceptions in finally - #7981
Conversation
…ions in finally
The Anthropic stream patchers install class-level __iter__/__aiter__
overrides on anthropic.Stream, AsyncStream, MessageStream,
AsyncMessageStream, BetaMessageStream, and BetaAsyncMessageStream.
Because the patch is class-level, every stream instance in the process
runs through the wrapper, tracked or not.
Each wrapper used an early `return` inside a `finally` block to skip
cleanup for non-tracked streams. A `return` in `finally` silently
swallows any in-flight exception. So once any opik-tracked streaming
call installed the class patch, a non-tracked anthropic.Stream that
errored mid-iteration completed silently instead of raising — the
caller never saw the error.
Fix: invert the guard so the early return is gone and cleanup nests
under `if hasattr(...)`. Tracked streams behave exactly as before
(cleanup runs, exception propagates). Non-tracked streams now propagate
their exceptions.
Also removes the Python SyntaxWarning ("'return' in a 'finally' block")
emitted from all six wrappers on import.
Added tests/unit/integrations/anthropic/test_stream_patchers.py covering:
- non-tracked stream exception propagates, cleanup callback not called
- tracked stream exception propagates, cleanup callback runs once with error_info
github-actions
Bot
added
python
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| import httpx2 | ||
| import anthropic | ||
|
|
||
| import opik.integrations.anthropic.stream_patchers as stream_patchers |
There was a problem hiding this comment.
Third-party dependency in unit suite
These unit tests import and construct third-party anthropic and httpx2 objects, so they depend on external libraries and exercise the real stream object instead of isolated Opik logic — should we move this module to the Anthropic library-integration suite?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/integrations/anthropic/test_stream_patchers.py` around lines
4-7, move these stream patcher regression tests out of the unit-test tree into the
Anthropic library-integration test suite because they directly import and construct the
third-party `anthropic` and `httpx2` SDK objects. Update the test location and any
imports or fixtures to follow the integration suite’s dependency guarantees and
isolation conventions, while preserving coverage for tracked and untracked stream
exception behavior.
| stream_patchers.original_stream_iter_method = underlying_iter | ||
| throwaway = mock.Mock(spec=anthropic.Stream) | ||
| stream_patchers.patch_sync_stream( | ||
| throwaway, | ||
| span_to_end=None, | ||
| trace_to_end=None, | ||
| finally_callback=finally_callback, | ||
| ) |
There was a problem hiding this comment.
Five stream wrappers lack regression coverage
The regression tests install only patch_sync_stream, so they cover just anthropic.Stream.__iter__; the changed finally blocks in AsyncStream, MessageStream, AsyncMessageStream, BetaMessageStream, and BetaAsyncMessageStream remain untested, allowing a swallowed return to be reintroduced while the suite stays green. Should we add focused sync/async coverage for each wrapper family or parameterize shared cases across all patched stream classes and iteration entry points?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/integrations/anthropic/test_stream_patchers.py` around lines
31-38, extend the regression coverage beyond `_install_patched_iter` and
`anthropic.Stream.__iter__`. Add focused or parameterized synchronous and asynchronous
tests for `AsyncStream`, `MessageStream`, `AsyncMessageStream`, `BetaMessageStream`, and
`BetaAsyncMessageStream`, verifying that iteration exceptions propagate and
tracked-stream cleanup callbacks run exactly once without swallowing errors. Reuse
shared fixtures/helpers where practical while covering each patched wrapper and its
actual iteration entry point.
| def test_non_tracked_stream_exception_propagates(restore_stream_iter): | ||
| """Regression test for the `return` inside `finally` bug. | ||
|
|
||
| Once the opik class-level Stream.__iter__ patch is installed, a | ||
| non-opik-tracked Stream whose iteration raises used to have its exception | ||
| silently swallowed by the early `return` in the finally block. The | ||
| exception must propagate to the caller, and the opik cleanup callback must | ||
| not run for a stream opik never tracked. | ||
| """ | ||
| callback = mock.Mock() | ||
| _install_patched_iter(finally_callback=callback, underlying_iter=_raising_iter) | ||
| stream = _make_stream(tracked=False) |
There was a problem hiding this comment.
Five wrappers lack regression coverage
The regression suite covers only patch_sync_stream/anthropic.Stream, leaving the async, message-stream, and beta-stream wrappers in stream_patchers.py without tests that untracked exceptions propagate and tracked cleanup callbacks run, so regressions there go unnoticed — should we add equivalent tests for each wrapper family, including async iteration?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/integrations/anthropic/test_stream_patchers.py` around lines
59-76, extend the regression coverage beyond `patch_sync_stream`/`anthropic.Stream` to
the async, message-stream, and beta-stream wrapper families changed in
`stream_patchers.py`. Add equivalent tests for each wrapper asserting that exceptions
from untracked streams propagate without invoking cleanup, while tracked streams
propagate the exception and invoke the cleanup callback exactly once; include the
appropriate async-iteration tests for async wrappers. Reuse the existing fixtures and
helpers where possible, and ensure each changed `finally` path is observable by a test.`
…pers The initial test only covered patch_sync_stream (anthropic.Stream). Baz review pointed out that the other five wrappers (AsyncStream, MessageStream, AsyncMessageStream, BetaMessageStream, BetaAsyncMessageStream) had no regression coverage — a swallowed return could be reintroduced in any of them while the suite stayed green. Now parameterized across all six wrappers for both sync and async, tracked and non-tracked paths (12 test cases total). Verified: - all 12 pass with the fix - the 6 non-tracked tests fail on upstream (bug present) - the 6 tracked tests pass on both (tracked path was never broken)
|
Thanks for the review @baz-reviewer. Pushed commit 437af94 addressing both points: Third-party dependency in unit suite — the expanded tests no longer import
Five stream wrappers lack regression coverage — now parameterized across all six wrappers (Stream, AsyncStream, MessageStream, AsyncMessageStream, BetaMessageStream, BetaAsyncMessageStream) for both sync/async and tracked/non-tracked paths. 12 test cases total. Verified:
|
| @pytest.mark.parametrize("config", _sync_wrappers(), ids=lambda c: c.id) | ||
| def test_sync_non_tracked_exception_propagates(restore_stream_patches, config): |
There was a problem hiding this comment.
Reported test count omits parametrized cases
The PR reports pytest tests/unit/integrations/anthropic/test_stream_patchers.py — 2 passed, but _sync_wrappers() and _async_wrappers() expand the four tests to 12 cases under Anthropic 1.0.0, so could we update the testing evidence with the collected/pass count, including beta cases?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/integrations/anthropic/test_stream_patchers.py` around lines
166-167, verify the parametrized sync and async wrapper tests include all base and beta
variants under Anthropic 1.0.0. Rerun the test file and correct the PR testing evidence
to report 12 collected and passed cases, retaining the beta cases in that count instead
of reporting only “2 passed.”
| callback.assert_called_once() | ||
| _, kwargs = callback.call_args | ||
| assert kwargs["capture_output"] is True | ||
| assert kwargs["error_info"] is not None |
There was a problem hiding this comment.
Incorrect error diagnostics go undetected
The tracked-stream test only checks that error_info exists, so a wrapper can report incorrect exception metadata and still pass — should we assert the injected RuntimeError("stream-blew-up") type, message, and diagnostic fields here and in the sync tracked test?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/unit/integrations/anthropic/test_stream_patchers.py` around lines
240-243, update `test_async_tracked_exception_propagates_and_callback_runs` to assert
that `error_info` contains the expected `RuntimeError` type, `stream-blew-up` message,
and relevant diagnostic fields, rather than only checking it is non-null. Apply the same
concrete error-metadata assertions to the synchronous tracked-stream test around lines
200-203, using the actual `error_info` structure returned by the callback.
Baz review pointed out the tracked-stream tests only checked error_info was non-null, so a wrapper could report incorrect exception metadata and still pass. Now asserts: - exception_type == 'RuntimeError' - message == 'stream-blew-up' - traceback contains the test module name
|
Thanks @baz-reviewer — addressed both new comments in commit 4774fd9: Reported test count (line 167) — PR testing evidence updated to report Incorrect error diagnostics (line 243) — the tracked-stream tests now assert the
Applied to both sync and async tracked-stream tests via a shared |
|
Confirmed the branch is synced with main (commit 57ef06a). Happy to answer any questions on the fix — the core issue was |
| @@ -0,0 +1,255 @@ | |||
| from dataclasses import dataclass | |||
There was a problem hiding this comment.
Please move this test to tests/library_integration/anthropic
| throwaway = object.__new__(config.patch_arg_cls) | ||
| config.patch_fn( | ||
| throwaway, | ||
| span_to_end=None, | ||
| trace_to_end=None, | ||
| finally_callback=callback, | ||
| ) |
There was a problem hiding this comment.
Manager propagation remains untested
_install patches a throwaway manager while each test iterates a separate _make_stream MessageStream/beta stream directly, so __enter__/__aenter__ never transfer opik_tracked_instance, span_to_end, or trace_to_end and the real messages.create flow can skip cleanup. Could the helper return the patched manager and exercise with/async with before the raising iteration, using distinct end-candidate sentinels instead of None and asserting they reach the callback?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`sdks/python/tests/library_integration/anthropic/test_stream_patchers.py` around lines
151-157, fix `_install` and the stream exception tests so they exercise the patched
manager’s `__enter__`/`__aenter__` path instead of iterating a separately allocated
stream. Have `_install` return the patched manager, use `with` or `async with` to obtain
the inner stream before triggering the raising iteration, pass distinct non-None
span/trace sentinels, and assert those sentinels are received by the cleanup callback.
The tests import and construct real `anthropic` SDK objects, so they belong in the Anthropic library-integration suite rather than the unit tree.
7808b66 to
1c60894
Compare
|
Moved the test to CI on this branch is stuck at Also confirmed the bedrock test you couldn't run locally goes green on #8023, left the details over there. |
alexkuzmik
left a comment
alexkuzmik
left a comment
There was a problem hiding this comment.
Looks good! Thanks @trakshan-mishra
3 participants
Details
The Anthropic stream patchers install class-level
__iter__/__aiter__overrides onanthropic.Stream,AsyncStream,MessageStream,AsyncMessageStream,BetaMessageStream, andBetaAsyncMessageStream. Because the patch is class-level, every stream instance in the process runs through the wrapper — tracked or not. Each wrapper used an earlyreturninside afinallyblock to skip cleanup for non-tracked streams. Areturninfinallysilently swallows any in-flight exception, so once any opik-tracked streaming call installed the class patch, a non-tracked stream that errored mid-iteration completed silently instead of raising.Fix: invert the guard so the early return is gone and cleanup nests under
if hasattr(...). Tracked streams behave exactly as before; non-tracked streams now propagate their exceptions. Also removes the PythonSyntaxWarning: 'return' in a 'finally' blockemitted from all six wrappers on import.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
ruff format --checkandruff checkon both changed files — cleanmypyonstream_patchers.py—Success: no issues foundpytest tests/library_integration/anthropic/test_stream_patchers.py—12 passed(4 parametrized tests × 3 wrapper classes each for sync + async, including beta variants)pytest tests/unit/decorator tests/unit/llm_usage—208 passedBUG: exception swallowed; after fix →PASS (propagated)error_info(exception_type, message, traceback) matching the injected RuntimeError, exception still propagates./opik.sh --build(Docker), Python 3.14, anthropic SDK 1.0.0, editable SDK installmake precommitrequires Java/Node toolchains for non-Python hooks; the Python-relevant hooks (ruff, ruff-format, mypy) were run directly and passDocumentation