Repository navigation
Conversation
chrikrah
left a comment
There was a problem hiding this comment.
@ch-z-hc refusal text now lands on the generation in all four sync/async and stream/no-stream cases, and 6579cf1 is ready to go in as it stands. Your four new cases fail against main's openai.py.
$ pytest -q tests/unit/test_openai.py tests/unit/test_openai_prompt_extraction.py # uv sync --locked, Python 3.12, openai 2.29.0
64 passed # head 6579cf1
4 failed, 60 passed # langfuse/openai.py from bf11ec1, your tests kept
FAILED tests/unit/test_openai.py::test_chat_completion_captures_refusal[False-False]
FAILED tests/unit/test_openai.py::test_chat_completion_captures_refusal[True-True]
$ pytest -q tests/unit/test_openai.py # prepare-v5-release edf44eb + this diff, Python 3.14.7, openai 2.29.0
41 passed # with the change
4 failed (-k refusal) # v5's own openai.py
# not run: the e2e suites against a Langfuse server, a live OpenAI call
non-blocking: a streamed refusal now logs a {role, refusal, content} dict. Other text streams log a bare string. I'd keep the dict. The non-streamed path records the same dict, so a filter on output.refusal finds both.
@hassiebp the diff also applies onto prepare-v5-release with no conflict in openai.py, and the refusal cases pass there too. Should it land on main now, or ride along with v5?
chrikrah
left a comment
There was a problem hiding this comment.
@ch-z-hc c3585de takes the merge of main without disturbing the refusal capture, and I approve this head. My earlier "main or v5" question is moot: main itself carries v5.0.1 now.
$ git merge-tree --write-tree c3585de refs/remotes/origin/main; echo rc=$?
6dad633f...
rc=0
$ uv sync --locked && uv run pytest -q tests/unit/test_openai.py tests/unit/test_openai_prompt_extraction.py
64 passed
# same tree, langfuse/openai.py replaced by main's version
FAILED tests/unit/test_openai.py::test_chat_completion_captures_refusal[...]
4 failed, 60 passed
# Python 3.14.7, pytest 8.4.2 as pinned by uv.lock
The delta against my reviewed head is 236 files of v5 releases. In openai.py it carries only main's cleanup, and all four refusal sites survive at openai.py:460, :461, :813, :890, which are the capture paths that stop working. With main's openai.py in the tree the four failures report at test_openai.py:1452 and none inside openai.py: the non-stream pair misses refusal in OBSERVATION_OUTPUT, the stream pair misses langfuse.observation.output outright.
@hassiebp, does anything in v5's openai.py rework need coverage before this merges?
|
Merged the latest main ( On the updated head, the two OpenAI unit suites pass (64 tests), along with Ruff, full-source mypy and changed-file formatting. Verified on Windows / Python 3.12.14 with the locked dependency versions. The PR description now records these results and distinguishes the previous full-suite run. Could a maintainer authorize the updated CI run? It currently reports Updated and validated with Codex assistance. |
What does this PR do?
Preserves OpenAI Chat Completions refusal text in generation output, including streamed responses.
When OpenAI returns
content=Noneand arefusal, the SDK consumer receives the refusal but the Langfuse trace loses it. Non-streaming output only containsroleandcontent; refusal-only streams have no recorded output. This makes refused requests indistinguishable from empty responses when inspecting traces or evaluating model behavior.The patch copies non-null
refusalinto chat output and accumulates streamed refusal deltas. Existing output shapes for responses without a refusal are unchanged. The field is part of the OpenAI Chat Completions streaming contract.The regression test exercises the actual OpenAI SDK with HTTPX MockTransport and the in-memory exporter, covering sync/async clients and streaming/non-streaming responses. It verifies both the application-visible response and recorded output/usage. Before the fix, all four new cases fail; the other 37 tests in
test_openai.pypass.Type of change
Verification
Current head
9b967330b2969c78d3f62386eed8af28cd1b96c0merges main35f8e7ee0c9c450cecf059563edbab14656a9fd1. The PR diff remains limited tolangfuse/openai.pyandtests/unit/test_openai.py; the main sync introduced no additional changes to the refusal fix.Current-head verification on Windows, Python 3.12.14, with the dependencies exported from
uv.lockand installed with hash verification (including OpenAI 2.29.0):c3585de: 921 passed, 2 existing skips in the complete unit suite on native Linux, Python 3.12.3. A fresh control using that main's source and the same refusal tests failed all four refusal cases. Those are previous-head results, not a full-suite claim for the current head.action_requiredand needs maintainer authorization. Local checks do not replace hosted CI.Checklist
code_review.md..env.templateif needed. (Not needed: no examples or configuration become stale.)AI assistance: Codex assisted with investigation, implementation, tests and diff self-review. This is not a claim of independent human review.
The refusal fix looks sound, but the new test must satisfy the repository’s import rule before merging.
Summary
Preserves OpenAI refusal text in completed and streamed chat traces.
httpximport to the module’s import block to satisfy the repository rule.Reviews (1) · Last reviewed commit: "fix(openai): preserve refusal text in tr..." · Reviewed by Greptile