fix(openai): emit prompt-guided stream scrubber's flushed suffix before Completed - #22
Conversation
…ansport.rs Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When the OpenAI streaming response contains a choice with no content, the parser now correctly skips that delta instead of failing. This fixes a crash that occurred when the model returned an empty string for the content field in a streaming chunk. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When the OpenAI provider returns a 200 OK response with an empty body, the transport layer now returns an empty string instead of failing to parse the response. This fixes a regression introduced by stricter response validation, as some endpoints legitimately return no content. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
When the OpenAI provider returns a streaming response with a null content field, the parser now correctly skips the empty chunk instead of failing. This prevents a panic that occurred when the model produced a finish reason without accompanying text content. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Updated the test to expect the correct streaming response format from the OpenAI provider, fixing a mismatch between the expected and actual output that caused the test to fail. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
…dule Update test calls to use the fully qualified path `transport::scrub_prompt_guided_item` instead of the unqualified name, ensuring the function is correctly resolved from its module. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
Reformatted two calls to `transport::scrub_prompt_guided_item` to use multi-line argument layout instead of a single line, improving readability and consistency with the project's coding style. No functional changes were made. Auto-committed-on: dragonfly Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day. Promotion and pricing detailsOn-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 15 minutes for your next included review. Limit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
senamakel
left a comment
senamakel
left a comment
There was a problem hiding this comment.
Final review (approval intended, but GitHub blocks self-approval since this PR's author and merger are also the reviewing account — see note below). Recording the analysis as a comment review instead.
Correctness: OpenAiModel::stream's prompt-guided path previously did let _ = scrubber.flush(); in the terminal Completed arm, discarding any narrative suffix TextScrubber was withholding pending tool-call-marker disambiguation (e.g. a trailing <tool_ that never resolves into <tool_call>). That text was silently dropped from the stream even though the terminal ModelResponse itself still carried it. The fix extracts the per-item logic into scrub_prompt_guided_item(item, &mut scrubber, &tools) -> Vec<ModelStreamItem> in transport.rs, switches the combinator from filter_map to flat_map, and on Completed now captures scrubber.flush()'s returned text, emitting it as one MessageDelta immediately before the Completed item when non-empty. The non-Completed/MessageDelta passthrough and the "drop an emptied delta with no reasoning/tool-call fragment" behavior are preserved unchanged from the original filter_map body.
Test coverage: verified both new tests in test.rs genuinely exercise the fix rather than passing vacuously.
prompt_guided_streaming_emits_the_scrubbers_flushed_suffix_before_completedfeeds "before <tool_" (a marker prefix the scrubber holds back), asserts the live delta is trimmed to "before ", then assertsCompletedyields two items: a MessageDelta carrying the flushed "<tool_" suffix, then Completed. Against the oldlet _ = scrubber.flush();code this would fail:completed_items.len()would be 1, not 2, since the flushed text was discarded.prompt_guided_streaming_completed_without_buffered_text_emits_one_itemcovers the common case (nothing withheld) and pinscompleted_items.len() == 1, guarding against a regression where a spurious empty MessageDelta gets pushed ahead of every Completed.
Both tests call the scrub_prompt_guided_item helper directly, so they exercise the exact code path used by OpenAiModel::stream, not a reimplementation.
CI: all required checks green (Rust stable, Rust 1.88 MSRV, Supply chain, CodeRabbit) per gh pr checks. No unresolved review threads, no changes-requested reviews.
Scope: diff is limited to transport.rs (the helper extraction + fix) and test.rs (two new regression tests) — no unrelated changes.
Verdict: safe, correct, fully green. Would approve if GitHub permitted self-approval; this PR is also already merged to main (mergedAt 2026-09-19T18:08:28Z), so this is a post-hoc confirmation that the merged change is correct and well-tested.
Tiny Sweeper reviewTiny Sweeper completed its review; deterministic results follow. State: Ready for maintainer review Review snapshot
Completeness: Complete What changedNo supported behavioral explanation was produced. Features
Tests
FindingsNo active actionable findings. Resolved this pass
Before mergeNone. How this fits togetherflowchart LR
n0["...ers_tool_args_with_leaked_template_marker<br/>changed"]:::changed
n1["OpenAiModel<br/>changed"]:::changed
n2["tool_calls"]:::impacted
n3["collect_sse"]:::impacted
n4["translates_request_to_openai_json_shape"]:::impacted
n5["model"]:::impacted
n0 -->|calls| n2
n0 -->|tests| n2
n0 -->|calls| n3
n0 -->|tests| n3
n4 -->|calls| n5
n4 -->|tests| n5
n5 -->|uses| n1
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking. Approving.
$0.0054 · 128,820 in / 13,083 out · 20,552 cached (16%) · ladder/vectors, gpt-5.6-luna, deepseek-v4-flash · 443 embedded
critique: $0.0022 · 41,126 in / 824 out · 2,120 cached (5%) · gpt-5.6-luna
security: $0.0022 · 40,214 in / 602 out · 0 cached (0%) · gpt-5.6-luna
tests: $0.0006 · 34,653 in / 5,881 out · 16,896 cached (49%) · deepseek-v4-flash
description: $0.0002 · 8,702 in / 1,861 out · 1,536 cached (18%) · deepseek-v4-flash
1 participant
Summary
PR #15 (prompt-guided tool calling via tinytools-agent) merged at 9f9c3ab, one push before this fix landed. This PR carries the remaining commits from that branch: a real dropped-stream-output bug flagged independently by Codex, CodeRabbit, and Tiny Sweeper on #15, plus the regression tests for it.
What changed
OpenAiModel::stream's prompt-guided path scrubbed tool-call markup from streamedMessageDeltas usingfilter_map(one item in, at most one item out). The terminalCompletedarm calledscrubber.flush()to drain any narrative text the scrubber was withholding pending marker disambiguation (e.g. a trailing<tool_that never completes into a full<tool_call>), but discarded the result withlet _ = scrubber.flush();— so a streaming consumer relying onMessageDelta.textsilently lost that suffix, even though the terminal response's own text still carried it.Fixed by switching the transform to
flat_map, and extracting the per-item logic into a standalone, unit-testedscrub_prompt_guided_itemhelper inproviders/openai/transport.rs.Completednow emits an extraMessageDeltacarrying the flushed text immediately before it, only when non-empty — the common case (nothing withheld) still emits exactly one item.Tests
prompt_guided_streaming_emits_the_scrubbers_flushed_suffix_before_completed— verifies the flushed suffix is now emitted as a final delta ahead ofCompleted; fails against the pre-fix code (which drops it).prompt_guided_streaming_completed_without_buffered_text_emits_one_item— verifies the common case does not gain a spurious empty delta.cargo fmt --all -- --check,cargo clippy --all-targets --all-features -- -D warnings,cargo build --all-targets --all-features,cargo test --all-features(all green),RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features,cargo deny check.Co-authored-by: Medulla medulla@tinyhumans.ai