Reject incomplete upstream streams before Anthropic success - #104
sanchitmonga22 wants to merge 2 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Final independent review: PR #104 at Independent checks: full CTest 13/13 passed, including the terminal fixture's 21 scenarios; targeted terminal/Anthropic suites passed 2/2. Invalid EOF, terminal framing and tool arguments cannot emit successful message_stop or buffered executable tools. Normal text/tool streams retain final usage. Windows, packaged Swift/MLX and live provider fault injection were not verified. The author's broader modality E2E reports 4 pass, 2 unrelated local-model failures, 4 skips; this review does not claim that suite is green. CodeRabbit skipped the non-default base. @Siddhesh2377 merge order: #82 → #103 → #104. After #103 merges, rebase only #104's issue-84 commits onto updated |
Related to #84.
An HTTP 200 stream ending after partial text or tool arguments previously emitted a successful Anthropic message_stop. Malformed frames were silently skipped. This issue is reproducible on the existing PR82 transport base.
The shim now requires complete SSE framing, a recognized finish reason and the final
[DONE]marker before successful closure. Invalid/unfinished streams emit one typed SSE error and never flush buffered tools. Every tool must have a name and complete JSON object arguments before any tool block is exposed. Valid text/tool streams preserve final usage and exactly one message_stop; LF, CRLF, comments, multi-line data and split transport chunks are supported.Validation
Merge sequence and ownership
main.main.main, retarget this PR tomain, rerun CI, then merge this PR.Current review base:
codex/issue83-stream-overloadataf0a482. Ultimate integration base:main(Wally has nodevordevelopmentbranch). Do not merge this PR into the dependency branch. The stack avoids competing edits to the same stream handler. Release and live end-to-end verification follow the merged stack. Keep issue #84 open for acceptance.