Repository navigation
fix(ai): record mid-stream errors on streamed OpenAI, Anthropic and Gemini events - #1094
Conversation
dustinbyrne
left a comment
There was a problem hiding this comment.
The streaming error-handling fix is supported by base/head regressions and native-SDK SSE probes. I found four new errors under the locked mypy check and a stale generated API snapshot; details inline. The source-typing errors need correction before approval.
AI-assisted review.
| groups=groups, | ||
| stop_reason=stop_reason, | ||
| service_tier=service_tier, | ||
| **({"error": error} if error is not None else {}), |
There was a problem hiding this comment.
F1 — [P2] Keep the event constructors compatible with the locked type checker
MINOR · REQUIRED · Introduced static-check regression
Location: posthog/ai/openai/_streaming.py:125, RIGHT. Same issue at posthog/ai/gemini/_shared.py:226, posthog/ai/anthropic/anthropic.py:261 and posthog/ai/anthropic/anthropic_async.py:261.
The repository’s locked type check now reports four new typeddict-item errors, one for each conditional unpack added inside StreamingEventData(...):
**({"error": error} if error is not None else {}),With mypy 1.16.1, matching uv.lock:2267–2268, these conditional dictionaries are checked against the full StreamingEventData context. Each produces a diagnostic that the required event keys are missing, although those keys are present in the outer constructor.
Equivalent base/head verification produced:
| Check | Base bf873523… |
Head ec98ed67… |
|---|---|---|
| Raw mypy | Exit 0; no issues in 303 files | Exit 1; four errors in 304 files |
Actual mypy-baseline filter |
Exit 0 | Exit 4; four new errors |
The baseline is empty, so it does not absorb these errors. The configured CI step runs the same check at .github/workflows/ci.yml:61–78:
mypy --no-site-packages --config-file mypy.ini . | mypy-baseline filterThis affects the shared OpenAI builder and the Gemini/Anthropic builders used by their sync/async wrappers. It is a source-type-check regression, not evidence that runtime events lack required fields or that provider streams fail.
Consider passing error=error directly—the field already accepts None—or conditionally assigning event_data["error"] after constructing the typed event, in all four places. Then rerun the CI-aligned check rather than adding the diagnostics to the baseline.
Verification: CONTRIBUTING.md prescribes this check. Inspected all four constructions and executed the checker and actual baseline filter under equivalent sandboxed environments, with isolated caches. This deterministic static failure needs no behavioral regression sketch. No corrected implementation was applied or tested.
| groups: Optional[Dict[str, Any]] | ||
| stop_reason: Optional[str] | ||
| service_tier: NotRequired[Optional[str]] | ||
| error: NotRequired[Optional[BaseException]] # raised mid-stream, if any |
There was a problem hiding this comment.
F2 — [P2] Refresh the generated public API snapshot
MINOR · OPTIONAL acceptance feedback; outstanding mechanical merge prerequisite · Introduced reference drift
Location: posthog/ai/types.py:146, RIGHT.
Adding the optional StreamingEventData.error member changes the generated API surface, but references/public_api_snapshot.txt still describes the base type.
The snapshot checker passes on base bf873523c700d9940c8ec86b677d2bd4904dfa55 and exits 1 on head ec98ed6794f2a866b0ae40522b05dfff8b878024. Its only generated difference is:
+attribute posthog.ai.types.StreamingEventData.error: NotRequired[Optional[BaseException]]CONTRIBUTING.md:33–43 requires regenerating the reference for API-surface changes, and .github/workflows/ci.yml:80–95 runs the snapshot check.
This is a generated-reference/check failure, not a demonstrated runtime compatibility break: the member is optional and existing constructions remain valid. Consider regenerating and including the reference update, then checking it before merge:
uv run --no-sync make public_api_snapshot
uv run --no-sync make public_api_checkVerification: Executed .github/scripts/check_public_api.py without --write on both revisions under equivalent Python 3.11.13/griffe 2.0.2 environments. The optional member is the sole difference. This is deterministic artifact validation, not a behavioral test. No regeneration was performed.
|
Thanks, fixed in 1a6ef61. The four call sites now pass |
marandaneto
left a comment
There was a problem hiding this comment.
The code looks good; no qualifying findings. Verdict: correct.
sign your commits for next PRs @pcbeingused333 |
…emini events capture_streaming_event hard-coded $ai_http_status=200, and the stream generators only had a finally block. When the provider stream raised after the first chunk (overloaded, connection reset), the event was sent as a success with no $ai_is_error/$ai_error, unlike the non-streaming path. Catch the exception in each sync and async generator, pass it through StreamingEventData, and re-raise unchanged. The event now carries $ai_is_error, $ai_error and the error's status code (0 when it has none). Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
…ate the API snapshot
mypy 1.16 rejected splatting {"error": error} into StreamingEventData(...)
(typeddict-item). The field accepts None and capture_streaming_event reads it
with .get(), so pass error=error. Regenerate the public API snapshot for the new
StreamingEventData.error field.
Co-Authored-By: Claude Sonnet 5.5 <[email protected]>
1a6ef61 to
492f2ca
Compare
💡 Motivation and Context
capture_streaming_eventhard-coded$ai_http_status: 200, and every streaming generator (OpenAI chat completions and responses, Anthropic, Gemini; sync and async) only had afinallyblock. If the provider stream raised after the first chunk (overloaded, connection reset), the event was still sent as a success, with no$ai_is_error/$ai_error. Non-streaming calls already record these (call_llm_and_track_usage).Each generator now catches the exception, passes it to the event through a new optional
errorfield onStreamingEventData, and re-raises it unchanged.capture_streaming_eventthen sets$ai_is_error: true,$ai_errorand$ai_http_status(the exception'sstatus_code, or 0 when it has none, as the non-streaming path does). Streams that finish normally, or are closed early by the consumer, are captured as before.💚 How did you test it?
posthog/test/ai/test_stream_midstream_error.py: the shared capture function with and without an error, and a stream that yields one chunk then raises for OpenAI chat (sync, async), OpenAI responses (sync), Anthropic (sync, async) and Gemini (sync, async). 9 of the 10 tests fail on main (the one that passes is the no-error case); all pass with the change.posthog/test/ai: 1081 passed, 15 skipped.ruff format --checkpasses on the touched files.📝 Checklist
If releasing new changes
sampo addto generate a changeset file (added.sampo/changesets/streaming-midstream-error.mdin the same format)🤖 Agent context
Autonomy: Fully autonomous
finallyblock while the non-streaming path already records errors. Agent-authored, needs human review.🤖 Generated with Claude Code