Skip to content

Reject incomplete upstream streams before Anthropic success - #104

Open
sanchitmonga22 wants to merge 2 commits into
codex/issue83-stream-overloadfrom
codex/issue84-stream-terminal
Open

sanchitmonga22 wants to merge 2 commits into
codex/issue83-stream-overloadfrom
codex/issue84-stream-terminal

Conversation

@sanchitmonga22

Copy link
Copy Markdown
Collaborator

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

  • Red: initial 12-case real loopback fixture failed 9 invalid cases on PR82 ded8a74 (partial tool arguments incorrectly became executable tool_use).
  • Green: 21 terminal scenarios using seven-byte upstream chunks, including EOF, missing/unknown/malformed termination, post-terminal data, upstream error, malformed/missing tool args, and valid text/tools plus usage.
  • Verified pinned SDK 0.20.37; full macOS arm64 C++ build; ctest 13/13 pass; command smoke 5/5 pass; agent-sync and diff checks pass. Independent review completed; numeric tool argument edge found and covered.
  • Additional repository e2e.sh modelless checks pass; local modality auto-discovery gives 4 pass, 2 fail, 4 skip. Failures are unavailable Kokoro ANE backend and a diarization model selected for VLM. They are unrelated to the HTTP shim. No Windows, packaged Swift/MLX or live provider fault-injection claim; telemetry/MLX ctest entries use default non-live behavior.

Merge sequence and ownership

  1. Siddhesh merges existing connection-pool PR Reuse upstream connections in the Anthropic shim and the JetBrains proxy #82 to main.
  2. Rebase/retarget and merge overload PR Preserve overload status and Retry-After before Anthropic streaming #103 (issue Preserve HTTP overload status and Retry-After through streaming shim #83) to main.
  3. Rebase only this PR's issue Reject incomplete upstream SSE instead of emitting message_stop #84 commits onto updated main, retarget this PR to main, rerun CI, then merge this PR.

Current review base: codex/issue83-stream-overload at af0a482. Ultimate integration base: main (Wally has no dev or development branch). 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.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 8348e5c7-25bb-4d59-b404-36734d077f8a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sanchitmonga22

Copy link
Copy Markdown
Collaborator Author

Final independent review: PR #104 at ea4eb4ac2bec2f8eb596e760fd1df9a300c73417, based on PR #103 at af0a482, has no unresolved blocking findings in its scoped diff. Reviewed the published GitHub diff, body and comments. The malformed numeric-tool-argument edge found during review is fixed, and missing arguments also fail rather than becoming an invented {} tool call.

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 main, retarget to main and rerun validation before merging. Issue #84 stays open for final packaged CLI/live acceptance; nothing is claimed deployed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant