fix(web-search): bound Anthropic sidecar SSE and error reads - #3873
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughLane C adds roadmap documents for web search, vision, health, desktop, and sub-agent fallback work. It also implements 64KiB bounds and cancellation behavior for Anthropic web-search streams and error bodies, with regression tests. ChangesLane C roadmap
Anthropic web-search bounds
Anthropic vision bounds
Startup-health snapshots
Codex Desktop authless preference
Sub-agent fallback editor
Estimated code review effort: 4 (Complex) | ~45 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 70 / 80이 PR은 #3839를 Lane C 맨 아래( 지금 테스트는 exact/oversized chunk와 never-settling cancel을 고정합니다. astra-high(Darwin) PASS. Lane C 전체 plan 문서(
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
c07456e to
47803f1
Compare
…ip ci] `parseAnthropicSidecarSSE` folded frames only when it found a `\n\n` separator, and appended every decoded chunk to `buffer` until then. An upstream that streams without ever emitting a separator therefore grew the buffer for the whole response. The non-OK branch had the same shape: it read the untrusted error body with an unbounded `res.text()` just to build one auth-failure message. Reuse the existing `MAX_SIDECAR_RESPONSE_BYTES` cap and the reader-cancel helper that `parseSidecarSSE` already applies in the same folder, so both paths stop reading at the same 64 KiB bound. Frames folded before the cap are kept; only the unterminated tail is dropped. `cancelReaderWithoutWaiting` is exported for this, unchanged. The regression drives a stream that never terminates a frame and asserts the read is cancelled well before the producer finishes. It fails without the cap and passes with it. (cherry picked from commit c25bdab) Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…ion [skip ci] Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
47803f1 to
51b4437
Compare
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. |
Summary
Carry #3839 with a 64 KiB accepted-byte bound for Anthropic web-search SSE and HTTP error bodies. Preserve complete prefix frames and discard unterminated tails at the cap. Regression fixtures cover exact/oversized chunks, HTTP 401/503 and reader cancellation that never settles.
Verification
f1b8f2d2f1edee55b68202469f86a5334710688d: Linux 4, macOS 2, gates, storage, API, all keyring/npm smokes and Docker. Overall run status iscompleted/success; this is not an assertion that every workflow job passed.macos controlto one run on the final release-train head. They are excluded only from this per-chain LANDABLE decision. Windows keyring/npm smoke jobs remain required. Lower product CI is represented by the cumulative top-head result.git diff --check: passed.Checklist
Manual dependent PR chain
Native stack:
null. Review only this layer’s diff. Main integrates bottom-up after refreshing dev ancestry, live heads/bases, review threads and required gates. This task does not merge. The CI-tested cumulative top tree is the integration target; advancing dev requires another cascade and top-head verification.51b44371860b2ec933d66de1b68ca790be3b81fb8eaa5641902b84bec6c97a94de6c2fe6d810c31d91b54b79bc780011031b00354621cc9fcaef360eabbdb227f385443228656c8440282187cb5dec75f1b8f2d2f1edee55b68202469f86a5334710688dMaintainer integration decision (MAINTAINERS.md, dev-only admin integration): @lidge-jun integrates lane C's manual chain (#3873 → #3874 → #3875 → #3876 → #3878) into
devbottom-up. Chain-top evidence at headf1b8f2d2f: Cross-platform CI run 34116228181, aggregateci= success (attempt 2: macos 1/2 timed out at 20 min in attempt 1 aftercodex-inject-write-lock.test.tswith no output; independent diagnosis found no causal link to this chain and the same head's macos control passed those tests; the failed job alone was rerun and passed — no code change). Windows 6/6 and macos control also green on this head. Prospective merge tree oforigin/dev@522ce5f8c+ top =e0b0e5886= tested tree. Independent source/security/GUI reviews PASS per PR bodies; screenshots from this run's dashboard-preview artifact. Local suites NOT RUN. Maintainer integration, not self-approval.Summary by CodeRabbit
New Features
Improvements
Bug Fixes