fix(web-search): bind continuations to the serving API key - #4489
Conversation
…ip ci] A routed provider that echoes the nested helper name emitted `view_image` instead of the declared code-mode `exec`, and the undeclared-tool guard failed the turn. `view_image` now joins the helper names the guard admits behind a bare `exec` declaration, and the compiler turns the call into `await tools.view_image(...)`, surfacing the returned `image_url` through `image()` and falling back to `text()` when the host returns no image. The `default.view_image` spelling some providers invent is stripped to the bare helper first. Explicit `path` wins over the `file_path`, `file` and `image_path` aliases, in that order. Carries #4455 by jeongjin0 and the overlapping four-file subset in #4171 by rrmlima, both answering #4412. Folds in the review findings recorded on #4171: the compat path executes the helper rather than emitting a text-only stub, it asserts nothing about `view_image` being unavailable in code mode, and the composition cases the review named are covered end to end — a namespaced `view_image` keeps its full wire name, a flat-bridge catalog that declares `exec` beside a bare `exec_command` is never rewritten, and malformed arguments still reach nested validation as data. Co-authored-by: Jeongjin Shin <80797980+jeongjin0@users.noreply.github.com> Co-authored-by: rrmlima <137737127+rrmlima@users.noreply.github.com>
…lay a refused rung [skip ci] A routed gateway publishes model ids but not the reasoning ladder behind them, so the catalog advertised rungs the upstream refuses. A models.dev snapshot now supplies the ladder when nothing is configured for that model, and a rung the upstream actually refuses is learned, dropped from every later ladder, and replayed once at the next lower published rung instead of failing the turn. `requestedEffort` and `effectiveEffort` keep both values in usage under the `reasoning-effort-downgrade` recovery kind. Carries #4409 by yxr1995-maker onto current dev. Conflict resolved in src/server/responses/core.ts: dev grew a `consoleGoUploadRetryGuard` replay block at both of the insertion points this branch targets. The two recoveries are independent and share only their trailing `continue` tail, so both blocks are kept, console-go first, each closing its own `if`. Review findings folded in: - The generic `recovery:` loop declared its downgrade guard inside the loop, so every `continue recovery` handed the turn a fresh downgrade budget. The guard now sits outside, beside the opaque-blob and console-go guards, and a regression test pins one downgrade for a replay that is refused again. - `planReasoningEffortDowngrade` read the models.dev ladder before the configured one, so a replay could land on a rung a pinned registry ladder deliberately excludes. Precedence now matches `configuredReasoningEfforts()`: model ladder, then provider ladder, then metadata, with the same family and case-folded id lookup. - `isReasoningEffortRejection` treated the bare parameter name as evidence, so a 400 refusing another field while echoing the request back spent the turn's one replay and persisted a false refusal for thirty days. It now needs the upstream to name the parameter, or rejection language beside the effort term. - `loadSupport()` applied the 30-day TTL only on the first disk read, so a long-running proxy kept clamping on month-old refusals through the memo. - `configuredReasoningEfforts()` asked for a metadata refresh only after a successful lookup, which is the one path a missing or corrupt snapshot never reaches. The refresh is now requested before the lookup. - The streamed test used an `openai-chat` fixture and so exercised the generic recovery loop while claiming to cover the passthrough one. It keeps that coverage and gains an `openai-responses` case for `passthroughRecovery:`. - The decision record claimed models.dev outranks a hand-written ladder, which contradicted both the code and its own layer list. `responses-reasoning-effort-downgrade.test.ts` is registered in both `scripts/test-layout/layout.json` and `tests/fixtures/test-layout-expected.json`; the branch had registered only `reasoning-metadata.test.ts`. Not folded in: the refusal cache still keys on destination, model and effort, so two configured entries pointing at the same gateway share learned refusals. Widening the key needs a provider identity threaded through every read path in the catalog, and a credential-derived key would put this link inside the security-review boundary. Recorded as open rather than half-applied. Co-authored-by: yxr1995-maker <257504378+yxr1995-maker@users.noreply.github.com>
…tting OPENCODEX_HOME [skip ci] Two defects found by running the carried change against tests/web-search, which the reasoning-focused test selection did not reach. configuredReasoningEfforts() asked for a models.dev snapshot refresh before the lookup so a missing or corrupt snapshot could recover, but it asked for every provider. A destination the snapshot does not cover gained a background fetch on its request path that could never help it, and in tests it consumed the mocked fetch that the web-search bridge was counting. The refresh now sits behind providerUsesReasoningMetadata(), which is true only for the gated destinations ladders are stored for. tests/codex-integration/reasoning-metadata.test.ts deleted OPENCODEX_HOME in afterEach instead of restoring it, so every later file in the same bun process read the real ~/.opencodex. That failed unrelated suites depending on file order, and made the run depend on the machine's actual configuration. Co-authored-by: yxr1995-maker <257504378+yxr1995-maker@users.noreply.github.com>
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 #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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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. |
리뷰 · 우선순위 72 / 80설명 고치는 문제는 한 문장으로 말할 수 있다. hosted web search 연속 요청(continuation)은 “새 턴”이 아니다. 1번 다리(first leg)에서 이미 쓴 API 키로, 방금 프록시가 실행한 검색 결과를 붙여 다시 보내야 한다. 그런데 예전에는 continuation이 일반 이번 수정은 우선순위 72는 자격 증명 바인딩(보안·정확성)이고 테스트가 계약을 실제로 증명하기 때문이다. 다만 스택 tip이라 아래 PR이 안 오면 머지가 막힌다. types/config 분할과 무관하다. 원본 #4387은 이 캐리가 랜딩되면 leftover로 닫으면 된다.
심볼
경로 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
Reverts the review finding that asked configuredReasoningEfforts() to request a models.dev refresh before the metadata lookup rather than after it. The reasoning was that a missing or corrupt snapshot is the case the lookup cannot serve, so asking only on success never refreshes it. That is true, and it is still the wrong place. A missing snapshot is the default state of a fresh install and of every test process. Asking there put a models.dev fetch on the request path of the first routed turn to a gated destination, which is observable: the lane tip run failed tests/responses/responses-console-go-upload-retry.test.ts and tests/providers/opencode-go-session-header.test.ts, where the extra bodyless request landed in the middle of a recovery replay the test was counting, and tests/web-search saw it consume the mocked destination's next leg. Refreshing a snapshot that does not exist yet is catalog-sync work. The refresh stays where the branch put it, so it only ever refreshes a stale snapshot that has already answered a lookup. Co-authored-by: yxr1995-maker <257504378+yxr1995-maker@users.noreply.github.com>
1 participant
Summary
dev, with aCo-authored-bytrailer on the branch commit.This is the lane R tip. Stacked on #4488 → #4475; retarget down the chain as each lands.
Structure documentation scope
The branch documents this across fifteen
structure/files. Four are carried:structure/runtime.mdsrc/web-search/.structure/transports/responses.md/v1/responsestransport this send belongs to.structure/data-planes/search.mdstructure/transports/streaming-health.mdThe 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,clients/claude-desktop.mdandsubagents.mdamong them, withops/service-and-sidecars.mda character-identical copy of thedata-planes/search.mdinsertion.structure/AGENTS.mdmakes these documents a source-ownership map, so a pointer in a document that owns nothing here adds a maintenance edge without adding a fact. Dropping them keepsbun run structure:checkgreen and leaves the contract discoverable from every document that does own a piece of this path.Carrying the
responses.mdandgui-and-management-api.mdfiles wholesale would also have reverted laterdevedits, since both drifted after the branch's merge base while staying at the 600-line budget. Theresponses.mdsentence is carried as the branch wrote it, appended to an existing line so the document stays at exactly 600.Verification
bun test tests/web-search/ tests/codex-integration/reasoning-metadata.test.ts tests/codex-integration/reasoning-effort.test.ts tests/responses/responses-reasoning-effort-downgrade.test.ts tests/responses/responses-undeclared-tool-guard.test.ts tests/responses/legacy-shell-compat.test.ts tests/adapters/bridge-legacy-shell-normalization.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts— 438 pass, 0 fail on this exact head.bun run typecheck— clean.bun run structure:check— passed. Required for this link because of thestructure/edits.bun run privacy:scan— passed.origin/dev(f7d9dbad0) is merged up through the stack, so this tip carries the current integration state.[skip ci], so it runs the full matrix. That tip run is the lane's suite proof.The carried tests are what make the contract real rather than asserted: eight drift cases mutated during the search (revision, same-value key reference, pool entry id, env-resolved key behind an unchanged reference, auth mode, base URL, provider disabled, provider removed) each assert one search, one outbound request, a
response.failedcarrying the bridge error code, and no answer text. Separate cases cover drift during the continuation's pacing wait and drift after the first-leg send but before its headers return. One case pins the other direction: reselection before the first physical send is still allowed, and both legs then use the new key with the search result preserved.Checklist
This is credential-selection handling, so it is the security-relevant one in lane R. The change is fail-closed in both directions: a binding that is not an API-key selection, or no binding at all, refuses the continuation rather than falling back. No key material is logged or serialized — the check compares selection identity and the resolved value in memory, and the tests assert only the
Authorizationheader the mock destination observed.