fix(claude): accept data-only Responses SSE (#700) - #711
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Claude outbound SSE converter now accepts frames without ChangesResponses SSE compatibility
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
CI note: the Windows job reached the workflow's 12-minute cap after the root test suite, GUI tests, privacy scan, release-helper syntax check, and GUI lint had all passed. It was cancelled during GUI build; Ubuntu, macOS, all npm-global jobs, React Doctor, target enforcement, and CodeRabbit passed. The contributor account cannot rerun an upstream Actions job, so a maintainer rerun should be sufficient. I have not changed the reviewed commit just to retrigger CI. |
|
Heads-up on a policy change that affects the port note in this PR's description: the
Nothing is required from you — this PR is unaffected other than that the port line in the description no longer applies. Sorry for the mixed signals while the policy was in flux. |
The `test` job ran with `timeout-minutes: 12` while a green Windows run takes 11.8 minutes, leaving about 12 seconds of margin. Runner variance, not the code under review, decided the outcome: PR #711's rerun finished at 11.8min and passed, PR #653's was killed at 12.0min with `Test` cancelled and every later step skipped. That flakiness is expensive to review around, because `gh pr checks` renders a cancelled job as `fail`. A red Windows check could mean a real failure, a `cancel-in-progress` concurrency cancel, or this timeout, and telling them apart requires reading `conclusion` from the check-runs API plus per-job timings. Measured on run 30459554635 (same suite, same commit): ubuntu-latest 4.6 min macos-latest 5.6 min windows-latest 11.8 min 20 minutes keeps a green Windows run green with margin. It does not address why Windows needs ~2.5x Linux on an identical suite; the comment says so explicitly, because the ceiling has already been raised once (8 -> 12) for this same reason and chasing it again is not a fix. The `npm-global-smoke` job is untouched at 8 minutes: it completes in 1-2 minutes and was never near its limit. Refs #717
The windows leg dominates this matrix at 11.8min against ubuntu's 4.6min, and #711/#653 were decided by runner variance rather than by the code under review. A maintainer's own Windows box removes that, but it is a personal machine on a home network: whatever runs on it runs as a local user with that user's files and LAN in reach. So the routing decision is made from the event, not from the workflow body. `push` on dev/main/preview requires the push permission, which only the MAINTAINERS.md set holds, and `workflow_dispatch` requires write access. Those two go to the home box. `pull_request` never does. An author check inside this file would not be equivalent. `ci.yml` runs from the PR head on `pull_request`, so any `if: author_association == ...` guard is deletable by the same patch it is meant to stop, as are the tsconfig, lockfile and postinstall hooks the run would otherwise honour. Only a decision made before a runner is chosen survives a hostile head. Two supporting details. `matrix.include` carries a fixed `name` because a label-array runner renders into the check name, so deriving it would rename the required check depending on which machine served it. And the self-hosted leg wipes its workspace first: a persistent working directory keeps files that the commit under test deleted, which passes a suite against a tree that no longer exists in git. Routing is gated on the `OCX_SELF_HOSTED_WINDOWS` repository variable, which is unset today, so every leg still resolves to windows-latest until the runner is registered and the variable is flipped.
Closes #700.
Summary
to the parsed JSON payload's
typewhen the optional SSEevent:field is absent;regressions.
Problem
The Claude Messages bridge currently requires every upstream Responses SSE record to carry both an
event:line and adata:line before it parses the JSON:The provider in #700 emits valid data-only records whose Responses event type is in the JSON
payload:
The HTML SSE standard permits data-only messages;
event:is not required and the default eventtype is
message. Because the bridge drops these records before parsing them, it never observesresponse.completed. Both Claude streaming and non-streaming requests consequently end as:The two Claude response modes share this converter, so one parser gate causes both failures.
Fix
Parse any record with a non-empty
data:payload, then resolve its Responses event name as:event:value, when present; otherwisetype.The scope remains narrow:
event:values still take precedence over a conflicting payloadtype;[DONE]remain ignored;response.completed,response.incomplete, orresponse.failedis still required before EOF;This accepts the gateway's framing without weakening the existing fail-closed truncation contract.
Tests
New regressions cover:
[DONE]without a Responses terminal frame still producing a truncation error rather thanmessage_stop.Activation was verified before the source change: the streaming and non-streaming data-only tests
failed against the old gate, while the explicit-precedence control passed.
Final verification on commit
fcd3298f:bun test tests/claude-outbound.test.ts— 30 passed, 0 failedbun test tests/claude-messages-endpoint.test.ts— 26 passed, 0 failedbun run typecheck— passedbun run privacy:scan— passedgit diff --check— passedbun scripts/test.ts --max-concurrency 4 ./tests/—5,977 passed, 1 Windows-only skip, 0 failed across 429 files
The bounded concurrency is a local WSL scheduling accommodation, not a test exclusion. At Bun's
default concurrency, two pre-existing deadline-sensitive tests intermittently starved under load;
the same tests passed 10/10 and 5/5 in isolated repetitions, and the complete lower-concurrency run
was clean.
Notes
devcontribution. Anydev2-goport remains the maintainer's transitionresponsibility under the repository policy.
Checklist
documentation changes are required.)
Summary by CodeRabbit