fix(web-search): preserve buffered upstream policy - #1155
Conversation
📝 WalkthroughWalkthroughRouted web-search iterations now follow the effective upstream streaming policy. Streaming routes use ChangesRouted web-search transport policy
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/sidecars.md`:
- Around line 32-37: Correct the routed-loop buffering description so it says
events are buffered only as needed to decide whether to search or finalize,
while explicitly phased commentary may be forwarded live. Apply this behavior
correction in docs-site/src/content/docs/guides/sidecars.md lines 32-37 and the
corresponding translated sections in
docs-site/src/content/docs/ja/guides/sidecars.md lines 33-37,
docs-site/src/content/docs/ko/guides/sidecars.md lines 33-37,
docs-site/src/content/docs/ru/guides/sidecars.md lines 37-43, and
docs-site/src/content/docs/zh-cn/guides/sidecars.md lines 30-34, preserving
equivalent meaning in each locale.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6b900e54-0714-4ae8-aca2-d56983dc63ca
📒 Files selected for processing (11)
docs-site/src/content/docs/guides/sidecars.mddocs-site/src/content/docs/ja/guides/sidecars.mddocs-site/src/content/docs/ko/guides/sidecars.mddocs-site/src/content/docs/ru/guides/sidecars.mddocs-site/src/content/docs/zh-cn/guides/sidecars.mdsrc/server/responses/core.tssrc/web-search/loop.tssrc/web-search/progress-stream.tsstructure/04_transports-and-sidecars.mdtests/web-search-progress-stream.test.tstests/web-search.test.ts
| Every routed-model iteration follows the route's effective upstream streaming policy. Streaming | ||
| routes use the streaming parser; compatibility routes use the bounded non-streaming parser while | ||
| the client-facing response stays Responses SSE. opencodex fully buffers semantic events internally | ||
| before deciding whether to search or return the final answer. Only the first iteration's final | ||
| headers/status and 429 key rotations are acquired eagerly. Thus synthetic search calls and | ||
| preliminary output are never exposed as client-visible model output. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the absolute buffering claim.
structure/04_transports-and-sidecars.md states that explicitly phased commentary text can forward live. The sidecar pages state that all semantic events are buffered and that preliminary output is never exposed. This contradicts the documented routed-loop behavior.
docs-site/src/content/docs/guides/sidecars.md#L32-L37: State that the loop buffers events needed to decide whether to search or finalize. State that explicitly phasedcommentarycan forward live.docs-site/src/content/docs/ja/guides/sidecars.md#L33-L37: Apply the same behavior correction in Japanese.docs-site/src/content/docs/ko/guides/sidecars.md#L33-L37: Apply the same behavior correction in Korean.docs-site/src/content/docs/ru/guides/sidecars.md#L37-L43: Apply the same behavior correction in Russian.docs-site/src/content/docs/zh-cn/guides/sidecars.md#L30-L34: Apply the same behavior correction in Simplified Chinese.
As per path instructions, user-facing docs must stay in sync with actual CLI/API behavior and translated locale pages must not contradict the English source.
📍 Affects 5 files
docs-site/src/content/docs/guides/sidecars.md#L32-L37(this comment)docs-site/src/content/docs/ja/guides/sidecars.md#L33-L37docs-site/src/content/docs/ko/guides/sidecars.md#L33-L37docs-site/src/content/docs/ru/guides/sidecars.md#L37-L43docs-site/src/content/docs/zh-cn/guides/sidecars.md#L30-L34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/guides/sidecars.md` around lines 32 - 37, Correct
the routed-loop buffering description so it says events are buffered only as
needed to decide whether to search or finalize, while explicitly phased
commentary may be forwarded live. Apply this behavior correction in
docs-site/src/content/docs/guides/sidecars.md lines 32-37 and the corresponding
translated sections in docs-site/src/content/docs/ja/guides/sidecars.md lines
33-37, docs-site/src/content/docs/ko/guides/sidecars.md lines 33-37,
docs-site/src/content/docs/ru/guides/sidecars.md lines 37-43, and
docs-site/src/content/docs/zh-cn/guides/sidecars.md lines 30-34, preserving
equivalent meaning in each locale.
Source: Path instructions
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 307045c555
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| throw new LoopError(502, `Provider adapter ${prepared.responseAdapter.name} does not support buffered responses`); | ||
| } | ||
| parse = async function* (response, budget) { | ||
| for (const event of await parseResponse(response, budget)) yield event; |
There was a problem hiding this comment.
When upstreamStreaming is false for an openai-responses route—the adapter family that core.ts can force into buffered mode—this invokes openai-responses.parseResponse, but that parser is compaction-specific: it extracts only summary text and returns upstream compaction returned no summary text for a function-call-only response. As a result, a buffered model request for the synthetic web_search tool never reaches scanEventsForWebSearch, so the hosted sidecar is not dispatched. The added integration test uses openai-chat, masking this incompatibility; use a buffered Responses parser that emits function/tool-call events before routing its output into the loop.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| throw new LoopError(502, `Provider adapter ${prepared.responseAdapter.name} does not support buffered responses`); | ||
| } | ||
| parse = async function* (response, budget) { | ||
| for (const event of await parseResponse(response, budget)) yield event; |
There was a problem hiding this comment.
When a buffered adapter such as openai-chat, Anthropic, or Google returns a web_search call, its parseResponse charges the resulting batch through retainTranslatedEventBatch. The loop then discards that iteration's original events after copying the query and thinking, while only events eventually consumed by bridgeToResponsesSSE have their leases released. Consequently every intercepted buffered iteration remains charged for the rest of the turn, and a later response can spuriously hit translation_buffer_limit when their combined size exceeds 32 MiB (or a smaller configured test budget), even though the earlier batch is no longer retained. Release each discarded source event after the loop has copied the data it needs.
Useful? React with 👍 / 👎.
|
Correcting an assessment I made and got wrong. I had this queued to close as unreachable: no production registry entry opts into The hook that matters is Withdrawing the unreachable rationale. The path is reachable and worth keeping. It is not merge-ready as it stands, though. The buffered |
|
Correcting my own correction — one detail above is wrong. I wrote that a request with The substance of the withdrawal stands: the branch is reachable from a public request, so closing this as an unreachable path would have been wrong. It is narrower than I stated — explicit Sorry for the noise. The merge-readiness point is unchanged: the buffered |
Records a campaign that gave a terminal disposition to every open bug issue and bug PR at the 2026-08-08 cutoff, and what it got wrong along the way. The unit carries the inventory and disposition matrix with file:line evidence per verdict, the rebase-and-co-author republish protocol, and per-work-phase implementation docs. Execution records cover the CI approval unblock, the first CI results, and the merges and closes. What the audits overturned is the more useful half. Two issues were queued for closure as resolved and were not - lidge-jun#1176 carried a maintainer comment from the same morning asking for a v2.11.0 retest, and lidge-jun#1024 rested on an upstream attribution the plan itself proposed to test and had not tested. lidge-jun#1155 was queued as an unreachable path and is reachable. lidge-jun#1263 was diagnosed as having no defect and has a real TOCTOU race, shown by contrast experiment. lidge-jun#1119 was described as fully absorbed and was not, so its coverage was recreated on dev before it was closed. Two execution faults are recorded as faults: lidge-jun#1202 was merged without checking its exact-head CI had concluded success, violating a reading rule written earlier in this same unit; and a public comment to a contributor made a false claim about stream defaulting, corrected on the PR. A transient six-failure test run is recorded as unexplained rather than dismissed as flaky, since its log was overwritten before the names could be preserved. Nothing in the build, typecheck, or test path reads from devlog/. privacy:scan passes; repo-hygiene 11 pass / 0 fail.
|
Status update, since this one has been sitting without any signal at all. No CI has ever run on this PR. Not failed, not cancelled — zero workflow runs against The likely cause is that this comes from a fork ( Two things that would move it:
Separately, my two comments above stand as the substantive review: the first retracted a wrong close-as-unreachable assessment, and the second corrected an error in that retraction about which branch a No action needed from you if you would rather leave it; say so and I will close it as stale with the record intact. But I would rather see it rebased and actually run. |
028 WP6 — the two bug issues that had no fix PR. #1297 shipped; #1296 diagnosed and deliberately left open. The record worth keeping is the pattern review kept catching: four rounds, and each time I had read a name or a comment as evidence of behaviour. I used promptCacheKey as a conversation identity because the name sounded right, when it is unvalidated client input the repo already warns against. I then found vertexReplaySessionId, whose comment claims a stable thread key while the code reads promptCacheKey, and mistook a second instance of the reported bug for the fixed version. I confirmed #1296 by reading three lines that pass err.message into a 401 without checking whether anything can reach them carrying an ACL error — nothing can. And I called codex-thread: prefixing a namespace that cannot collide, then reproduced the collision myself. 029 WP17 — the goalplan read complete while nine pre-cutoff PRs had never had CI executed. Approved 37 pending runs SHA-matched and drove all nine to a recorded outcome: eight green, and #1155 documented as having no run object to approve. Two corrections folded there too. An approval starts CI and is not a disposition, so the completion gate became an outcome per item rather than an unblock per item. And my claim that a cutoff campaign cannot close against its starting snapshot contradicts 000_plan.md, which freezes the inventory at the cutoff — complete means every item open then has a final disposition, not that the repository has no open bug work now.
Wibias
added
the
stale
3 participants
Summary
parseResponsepath while keeping the client response as Responses SSE.Refs #1143
Scope note
Public
devcurrently has no OpenCode Go / GPT-5.6 Luna entry inmodelResponsesUpstreamStreaming. Its onlyfalseentry is DeepSeek Flash on the nativeopenai-responsespassthrough path, which does not enter the web-search loop. This PR fixes and tests the loop-level transport contract, but intentionally does not add unverified Luna provider metadata or claim an end-to-end reproduction of that provider route.Verification
bun test tests/web-search.test.ts tests/web-search-progress-stream.test.ts— 72 pass, 0 fail.bun run typecheck— passed.bun run test— passed on commit307045c5.bun run privacy:scan— passed.cd docs-site && bun run build— 221 pages built.git diff --check— passed.Checklist
Summary by CodeRabbit
Bug Fixes
Documentation