Repository navigation
fix(ai): capture an error event when a streaming call fails to connect - #1146
pcbeingused333 wants to merge 6 commits into
Conversation
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: Alex Castillo <alex.castillog33@gmail.com>
marandaneto
left a comment
There was a problem hiding this comment.
Automated advisory code review.
| response = super().create(**kwargs) | ||
| except Exception as exc: | ||
| accumulator = _AnthropicStreamAccumulator() | ||
| self._capture_streaming_event( |
There was a problem hiding this comment.
blocking: Preserve the provider exception when error telemetry cannot format rejected input — When Anthropic rejects malformed input such as messages=[None] with HTTP 400, the newly added capture call formats that same rejected input before reaching the bare raise. format_anthropic_input calls msg.get(...), raising AttributeError and replacing the original anthropic.BadRequestError; the async handler at posthog/ai/anthropic/anthropic_async.py:169 behaves identically. Previously callers received the actionable provider error and could handle BadRequestError normally. Ensure failures in this telemetry path cannot replace the original exception. Reproduction: reproduced — python -m pytest -p no:cacheprovider posthog/test/ai/test_review_connect.py -k invalid_anthropic --timeout=30 -vv fails both sync/async assertions on the reviewed head with AttributeError: 'NoneType' object has no attribute 'get', while both pass unchanged at the recorded merge base.
marandaneto
left a comment
There was a problem hiding this comment.
Automated advisory code review.
| kwargs, | ||
| usage_stats, | ||
| time.time() - start_time, | ||
| accumulated_content, |
There was a problem hiding this comment.
blocking: Keep the new Gemini handlers compatible with Mypy — Passing the initially empty, unannotated accumulated_content into this new exception handler makes Mypy require an explicit element type before the nested generator populates it. Both the sync and async additions introduce var-annotated errors and fail the required Mypy check; please annotate the accumulated content in both wrappers. Reproduction: reproduced — python -m mypy --no-site-packages --config-file mypy.ini posthog/ai/gemini/gemini.py posthog/ai/gemini/gemini_async.py reports exactly two annotation errors on the reviewed head and no issues on the recorded merge base.
…ror-capture # Conflicts: # posthog/ai/openai/openai.py # posthog/ai/openai/openai_async.py
|
@pcbeingused333 ping here, otherwise i can fix comments as well |
Signed-off-by: Alex Castillo <alex.castillog33@gmail.com> # Conflicts: # posthog/test/ai/test_stream_midstream_error.py
…nnect If building the telemetry event raises (for example while formatting input the provider just rejected), the capture error replaced the original exception. Log it instead and re-raise the provider error. Signed-off-by: Alex Castillo <alex.castillog33@gmail.com>
|
Thanks for the ping. Pushed two things:
The Gemini mypy annotation from the second review was already fixed in 29325a4. This comment was drafted with Claude Code. |
dustinbyrne
left a comment
There was a problem hiding this comment.
The Anthropic exception-preservation correction is verified. I still found an OpenAI Chat exception-masking case and privacy-sensitive fallback logging, plus two small repository-check failures. Details inline.
AI-assisted review.
| try: | ||
| response = self._original.create(**kwargs) | ||
| except Exception as exc: | ||
| self._capture_streaming_event( |
There was a problem hiding this comment.
[P1] Preserve OpenAI Chat’s provider exception when telemetry formatting fails
Severity: IMPORTANT
Disposition: REQUIRED
Relationship: Introduced by the original PR; newly identified in this follow-up, not introduced by the latest Anthropic correction.
Location: posthog/ai/openai/openai.py:415; matching async call at openai_async.py:419.
A malformed request rejected with HTTP 400 should still raise OpenAI’s BadRequestError. The new capture attempt instead raises TypeError, bypassing existing application error handlers.
Public trigger:
client.chat.completions.create(
model="gpt-test",
messages=123,
stream=True,
)This requires malformed dynamically supplied input; valid-message controls pass. The wrapper accepts **kwargs: Any, and the real OpenAI serializer sends numeric messages.
After the provider raises BadRequestError, _capture_streaming_event calls format_openai_input. Its loop at openai_converter.py:235 raises:
TypeError: 'int' object is not iterable
The bare raise is never reached. Base propagated the provider exception directly. OpenAI’s official error contract maps HTTP 400 to BadRequestError.
Executed regression: Actual OpenAI/AsyncOpenAI, actual serialization, Posthog(send=False), and HTTPX MockTransport returning:
{"error":{"message":"Invalid request input","type":"invalid_request_error","code":null}}The retained test_openai_rejected_input.py asserts BadRequestError, status 400, and exactly one HTTP request:
- Base: eight passed.
- Head: six controls passed; malformed Chat sync/async cases failed with
TypeError. - Malformed Responses controls passed; this finding does not establish the same defect there.
Consider making the complete new Chat telemetry attempt best-effort while preserving the provider exception, and retaining these sync/async regressions alongside capture-count tests. Avoid raw exception logging as the fallback.
| ) | ||
| except Exception: | ||
| # Telemetry must never replace the provider's own error. | ||
| log.exception("Failed to capture streaming connect error") |
There was a problem hiding this comment.
[P1] Avoid logging private provider-error text in Anthropic’s fallback
Severity: IMPORTANT
Disposition: REQUIRED
Relationship: Introduced by the latest correction.
Location: posthog/ai/anthropic/anthropic.py:188; matching async fallback at anthropic_async.py:188.
When telemetry preparation fails, log.exception(...) prints both the formatter exception and its chained provider exception. Consequently, private text in the provider’s error body reaches the SDK logger despite privacy mode.
Public trigger:
client.messages.create(
model="claude-test",
max_tokens=5,
messages=[{"role": "user", "content": "private text"}, None],
stream=True,
posthog_privacy_mode=True,
)Both conditions are required:
- The provider rejection contains private text.
- Telemetry preparation fails—for example, on the malformed
Nonemessage.
The original provider exception now correctly survives. The regression is the additional SDK diagnostic.
Existing _summarize_error_for_privacy / _format_ai_error at posthog/ai/utils.py:925–980 deliberately exclude messages and tracebacks because they can quote prompts/output. The existing privacy tests exercise provider errors containing private canaries.
Executed regression: test_logging_privacy.py uses actual Anthropic serialization, a synthetic HTTP 400 provider body containing a private canary, and formatted SDK logging—not mocked logging. It tests sync/async, per-call/client privacy, and valid/malformed histories:
- Base: eight passed.
- Head: four valid-history controls passed; all four malformed-history privacy cases failed.
- Every case first verified that the original
BadRequestErrorsurvived and exactly one HTTP request occurred.
Observed SDK output includes:
ERROR posthog:anthropic.py:188 [PostHog] Failed to capture streaming connect error
...
anthropic.BadRequestError: Error code: 400 - {
... 'message': 'Rejected history containing REVIEW_PRIVATE_PROMPT_1146'
}
...
AttributeError: 'NoneType' object has no attribute 'get'
This demonstrates disclosure through SDK logging, not transmission to PostHog or proof that every live provider rejection echoes private content. It reproduces with send=False and the caller catching the provider exception.
Consider keeping the guard but logging fixed diagnostic text without exc_info or arbitrary exception text. Changing only the log level does not redact it. Retain the privacy regressions and valid-history controls.
|
|
||
| from posthog import setup | ||
|
|
||
| log = logging.getLogger("posthog") |
There was a problem hiding this comment.
[P2] Move async logger initialization below the remaining imports
Severity: MINOR
Disposition: REQUIRED mechanical check prerequisite
Relationship: Introduced by the latest correction.
Location: posthog/ai/anthropic/anthropic_async.py:16.
The new assignment precedes six remaining imports:
log = logging.getLogger("posthog")
from ..stream import AsyncStreamWrapper as AsyncStreamWrapperThe identical focused Ruff check passes on base and fails on head with six E402 diagnostics at lines 17, 18, 23, 27, 28, and 35. CI’s Ruff job explicitly runs ruff check ..
Executed command, with SDK selecting each pinned checkout:
ruff check --no-cache --config "$SDK/pyproject.toml" \
"$SDK/posthog/ai/anthropic/anthropic.py" \
"$SDK/posthog/ai/anthropic/anthropic_async.py"Consider moving initialization below the final import, matching the sync module. This is a locally reproduced check failure, not a runtime defect or a claim that remote CI completed and failed.
| from posthog.client import Client as PostHogClient | ||
| from posthog import setup | ||
|
|
||
| log = logging.getLogger("posthog") |
There was a problem hiding this comment.
[P2] Keep implementation loggers private, or update the API snapshot
Severity: MINOR
Disposition: REQUIRED mechanical check prerequisite
Relationship: Introduced by the latest correction.
Location: posthog/ai/anthropic/anthropic.py:35; also anthropic_async.py:16.
The repository’s static scanner includes local non-underscore attributes. The new log globals therefore change its output while the checked-in snapshot remains unchanged.
The actual no-write check:
python .github/scripts/check_public_api.pypasses on base and fails on head with exactly:
+attribute posthog.ai.anthropic.anthropic.log = logging.getLogger('posthog')
+attribute posthog.ai.anthropic.anthropic_async.log = logging.getLogger('posthog')This is an artifact-consistency failure, not a demonstrated supported-API compatibility break. No user-facing need for exporting these implementation loggers was found.
Consider naming them _log and updating references. If exposure is intentional, regenerate and review the snapshot as required by CONTRIBUTING.md.
If capturing the connect-error event itself raised (e.g. unformattable input), the capture exception replaced the provider error. Wrap the capture in each OpenAI/Gemini wrapper, log a fixed message, and re-raise the original error. The Anthropic fallback log no longer includes the exception text or traceback. Signed-off-by: Alex Castillo <alex.castillog33@gmail.com>
|
Thanks, all three addressed in 8c738e1:
This change was prepared with AI assistance (Claude). |
💡 Motivation and Context
For streaming calls the OpenAI chat, OpenAI Responses, Anthropic and Gemini wrappers call the underlying
create/generate_content_streamoutside the try/finally that captures the event, and only wrap the iteration of the returned stream. A connection-time failure (401 bad key, 429 rate limit, invalid model) is raised before any generator exists, so no$ai_generationevent is sent; only errors raised mid-stream are recorded. Non-streaming calls already record these errors throughcall_llm_and_track_usage.The streaming call is now wrapped in
try/except Exception: on failure the existing_capture_streaming_eventis called with an empty stream state anderror=exc(which sets$ai_is_errorand$ai_http_status), and the exception is re-raised unchanged. Sync and async, all four providers. No issue is linked; the gap is visible from the code path and reproduced by the tests below.💚 How did you test it?
Eight tests in
posthog/test/ai/test_stream_midstream_error.py(OpenAI chat, OpenAI Responses, Anthropic and Gemini, sync and async) make the underlying call raise a 401 error and assert the captured event has$ai_is_errorand$ai_http_status == 401; all fail onmain(no event captured) and pass with the change.pytest posthog/test/ai: full suite green;ruff checkandruff format --checkclean. Not run against live provider APIs.📝 Checklist
If releasing new changes
.sampo/changesets/stream-connect-error.md(pypi/posthog: patch).🤖 Agent context
Autonomy: Human-driven (agent-assisted)
The change and the tests were written by Claude Code (Anthropic's agent) working for the PR author, who directs this contribution work and is the DRI. The agent compared the streaming and non-streaming paths of each wrapper, wrote the failing tests first, then wrapped only the initial call so the per-chunk handling is untouched. No manual testing against the providers was done.