fix(web-search): bind continuations to the serving API key - #4387
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (17)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe Responses hosted-search bridge now captures the initial API-key binding and validates it before each continuation dispatch. Binding changes fail the continuation without another upstream request. Tests cover drift, pacing, and initial-request reselection. Documentation defines the contract. ChangesHosted-search continuation binding
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant webSearchBridge
participant ProviderSelection
participant ResponsesProvider
Client->>webSearchBridge: send hosted-search request
webSearchBridge->>ProviderSelection: select API key for initial dispatch
ProviderSelection-->>webSearchBridge: return resolved binding
webSearchBridge->>ResponsesProvider: dispatch search request
ResponsesProvider-->>webSearchBridge: return search result
webSearchBridge->>ProviderSelection: validate captured binding
alt binding unchanged
webSearchBridge->>ResponsesProvider: dispatch continuation with search result
ResponsesProvider-->>Client: return completed response
else binding changed or provider unavailable
webSearchBridge-->>Client: return WEB_SEARCH_BRIDGE_ERROR_CODE
end
Merge Risk: ⚪ Minimal · up to The bridge rejects stale API-key continuations before another upstream request while preserving valid initial reselection and continuation behavior; no merge-blocking risk is evidenced. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (16 skipped: 16 unsupported.)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
리뷰 · 우선순위 71 / 80설명 이 PR은 API 키 Responses에서 hosted web-search를 켠 뒤, 첫 다리를 보낸 선택(키·인증 모드·base URL)이 이어지는 continuation에도 그대로 묶이게 합니다. 지금 패치는 첫 다리 이후 라인 - 이게 무슨 문제다
pacing 중 변경 테스트 - structure 다수 파일 한 줄 갱신 - inventory/responses/search 등에 같은 문장을 복제합니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
|
Exact-head CI for a6a3403 completed with failure: https://github.com/luvs01/opencodex/actions/runs/34681035244 The recorded assertion groups concern Cline inventories (#4386), injection/journal fixtures (#4380), all three Devin path cases (#4384), and pnpm shims (#4379). No web-search/key-binding assertion failure was identified in the inspected logs. This is evidence for separating the shared failure groups from this patch, not a proof that the entire matrix passed or that every failure cause is settled. Draft remains pending integration and exact-head validation. |
|
Correction to my previous status comment, for completeness rather than a changed conclusion. Full enumeration of all eleven failing test jobs in that run adds two clusters I did not name:
With those included, every failure in the run is accounted for by open baseline work, and nothing touches the web-search key-binding path in this PR. Please read this comment together with the previous one; the earlier list was incomplete, not inaccurate. |
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. This PR stays in draft until every box above is ticked. |
c321680 to
107aa4f
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. |
107aa4f to
3dcc43c
Compare
3dcc43c to
2a1b3c3
Compare
Lane R of the contributor carry train, the serialized responses/core lane: code-mode view_image through unified exec (#4455 by jeongjin0, also carrying the duplicate #4171 by rrmlima), routed effort ladders from models.dev with a refused-rung replay (#4409 by yxr1995-maker), and web-search continuations bound to the serving API key (#4387 by luvs01). Cross-platform CI run 34746891233 concluded success on 2c28886, the exact head merged here, and it covers every link because the lane is cumulative. #4475 and #4488 carry no ci check of their own; their head commits carry [skip ci] by design, under the owner-authorized tip-only CI economy for this batch. The fourth planned link, #4086 by Eleven-is-cool, is not here because it is already on dev as d6723f7 with its own Co-authored-by trailer. The lane attempted the carry first and found a modify/delete conflict on structure/04_transports-and-sidecars.md, which the #4276 SSOT restructure had removed; the landed version is a superset of the branch. All four source authors are credited by Co-authored-by trailers in the landed commits.
|
Closing as landed: binding web-search continuations to the serving API key is on dev via #4489 (merge commit 3f76ce4, verified as an ancestor of origin/dev). Your authorship is preserved by a Co-authored-by trailer in the landed commit itself rather than only in the pull request body, so it counts on your contributor graph. The carry folded the review findings already on this pull request. If you think something from this branch did not make it to dev, say so and I will reopen. |
A hosted-search continuation is not a new turn. It is the first leg plus the search the proxy just executed, and it has to reach the account that already served that leg. The continuation went out through the ordinary dispatch override instead, so a selection change during the search could send the search-bearing body under a different key -- or rebuild the request from the original turn and drop the search result entirely. The bridge now captures the request binding that served the first leg, after any permitted initial reselection, and rechecks it after provider pacing on every continuation dispatch. The binding must still be an API-key selection matching the configured entry, reference, revision, resolved key, auth mode and base URL; a disabled or removed provider fails the same check. Drift ends the turn with the bridge's failed terminal and issues no further provider request. Initial dispatch keeps its normal reselection policy. Carries lidge-jun#4387 by luvs01 onto current dev. The branch documents this in fifteen structure/ files. Four are kept: structure/runtime.md carries the contract itself, and transports/responses.md, data-planes/search.md and transports/streaming-health.md own the transport, the search data plane and the post-pacing check. The other eleven received the same cross-reference sentence pasted into documents that own none of the changed source -- data-planes/images.md, providers/xai-grok.md and subagents.md among them, with ops/service-and-sidecars.md a character-identical copy of the data-planes/search.md insertion. structure/AGENTS.md makes these documents a source-ownership map, so a pointer in a document that owns nothing here adds a maintenance edge without adding a fact. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
2 participants
Summary
Earlier focused verification (before this rebase)
bun test tests/web-search/web-search-passthrough-bridge.test.ts— 32 passed.bun test tests/providers/api-key-selection-capture.test.ts tests/usage/request-pacing.test.ts tests/lab/core-lab-boundary.test.ts— 38 passed.bun test tests/web-search/web-search-passthrough-bridge.test.ts -t 'key reference with the same resolved value'— 1 passed after isolating reference drift from entry-ID drift.bun run typecheck— passed.bun run privacy:scanandbun run structure:check— passed.cd docs-site && bun run build— passed, 425 pages. The generated provider configuration page contains the new binding and reselection guidance.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Documentation
Current verification (2026-09-13)
Head: 3dcc43c, rebased onto dev@17da84f89.
Focused tests, typecheck, structure check and privacy scan passed on this rebased source:
32 pass, 0 fail.
The previous green matrix tested an older PR head with the pending #4384 fix added. It is integration evidence only, not a passing full-suite result for this published head. Full CI readiness remains open. The current dev tip has advanced beyond this tested base; further refresh will be coordinated with the shared Windows fixture fix to avoid repeatedly queuing matrices that inherit the same failure.