fix(web-search): honor provider HTTP version pins - #2908
Conversation
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. |
|
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 (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughWeb-search execution now receives the current routed provider fetch implementation, applies the provider’s upstream HTTP version, and uses the same transport for adapter and retry requests. Tests verify provider routing and ChangesWeb-search transport routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR routes web-search requests through the configured provider transport, including HTTP-version pins and credential rotation paths, while keeping routed and hosted-search providers separate. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Responses as handleResponsesInner
participant SearchLoop as runWithWebSearch
participant ProviderFetch as routed provider fetch
participant Executor as runWebSearch
Responses->>SearchLoop: pass incomingMeta.providerFetch
SearchLoop->>ProviderFetch: fetch adapter or retry request
ProviderFetch->>Executor: apply provider transport configuration
Executor->>ProviderFetch: send request with upstream HTTP version
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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. |
![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: a3e36ced99
ℹ️ 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".
| timeoutMs: connectTimeoutMs, | ||
| returnRawErrors: true, | ||
| stream: true, | ||
| executor: routedProviderFetch, |
There was a problem hiding this comment.
When web search is routed through command-code, mimo-free, or Kiro, this supplied executor is never called: command-code uses its captured executor in src/adapters/command-code.ts:536-537, MiMo calls global fetch in src/adapters/mimo-free.ts:249-274, and Kiro's retry helper invokes fetchWithAttemptDeadline without ctx.executor. Because the fetchResponse branch prevents the fallback send below from running, these supported adapters still discard upstreamHttpVersion and the provider fetch seam on every model iteration. Update each adapter-owned physical send and retry to use ctx.executor, and cover at least one real adapter rather than only an executor-aware test double.
Useful? React with 👍 / 👎.
리뷰 · 우선순위 66 / 80이 PR은 지금 지금 고치는 방법은 세 갈래다. 호스티드 검색 사이드카는 라우트 fetch 를 쓰지 않는다. 이 PR은 #2885 를 닫지 않는다. 그 이슈는 Windows 에서 Bun 1.4.0 이면 Zhipu GLM-5.3 스트림 소켓이 갑자기 닫히고, 같은 설정으로 1.3.14 이면 200 이 나는 단일 변수 A/B 다. 웹서치가 없어도 재현된다. 여기서 고친 것은 테스트 세 개는 각자 자기 배선만 빼면 깨지도록 만들어져 있다. 글로벌 fetch 를 던지게 해 두고 제공자 fetch 로 핀이 가는지, 어댑터 라인 5005 - 보낼 때 route.provider 를 다시 읽는 래퍼는 맞다. rotateSidecarProviderOn429 가 새 객체를 끼운다. 그런데 회전 다음에 새 키와 새 핀이 나가는지 보는 테스트는 없다. 한 번만 providerFetch(route.provider) 를 잡아 두어도 지금 테스트는 통과한다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
1 participant
Summary
providerFetch, including adapters that perform their own physical fetch throughctx.executor.IncomingMetaand resolve the mutable routed provider at send time so 429 credential rotation cannot retain a stale provider object.upstreamHttpVersionpin to its/responsesrequest without crossing routed and sidecar credential boundaries.fetchResponse, and the hosted-search sidecar.This resolves the configuration-ignored gap for web-search turns in #2885: an operator's
upstreamHttpVersion: "http1.1"workaround now reaches both routed model iterations and a separately pinned hosted-search sidecar. It does not identify or fix the underlying Windows Bun 1.4.0 versus 1.3.14 HTTP/2/socket behavior reported by the issue, so this PR intentionally does not close it.Verification
bun test tests/web-search.test.ts— 59 pass, 0 failbun test tests/upstream-http-version.test.ts— 15 pass, 0 failbun x tsc --noEmit— passbun run privacy:scan— passcore.tsroutedproviderFetchwiring — issue [Bug][Windows][Zhipu] Bun 1.4.0 causes GLM-5.3 streams to fail with 502; the same OpenCodex 2.33 setup works on Bun 1.3.14 #2885 test failed 502; restored — pass.fetch— issue [Bug][Windows][Zhipu] Bun 1.4.0 causes GLM-5.3 streams to fail with 502; the same OpenCodex 2.33 setup works on Bun 1.3.14 #2885 test failed 502; restored — pass.ctx.executorfrom adapterfetchResponse— executor-aware test failed 502; restored — pass.IncomingMetapreservation frombuildRequest— executor-aware test failed 502; restored — pass.withUpstreamHttpVersionwrapper — pinned sidecar assertion receivedundefined; restored — pass.Checklist
upstreamHttpVersionbehavior now reaches the omitted paths.)Summary by CodeRabbit