fix(search): keep native routing aligned with model wire - #2179
Conversation
|
Thanks for the fix — the core-side gap is real and still present on main: Conclusion: FAIL — the runtime half is obsolete on main; rebase and drop it (keep only the core half). P1 — the other half of this PR ( P2 — the PR body's verification claim ("rebuilt from latest main") is not credible as-is: the PR is P3 (optional): AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on a kimi-backed review model). The subagent verified the core-side logic against current main and confirmed the runtime-side files are gone via main's commit history (#2249/#2518); the P1 is a fact of the current tree, not a prediction. Please weigh these findings with your own judgment. 中文摘要(AI 辅助审查)结论:FAIL——runtime 半已过时,rebase 后整体丢弃,只保留 core 半。core 侧缺口真实且仍在 main 上:resolveHostedWebSearchCapability 在用户元数据声明 capabilities.webSearch:true 且未声明 apiProtocol 时无条件返回 Responses 适配器,而 model factory 按模型族规则把该模型放 Chat wire → Responses-only 的内置 web_search 工具被编译到 Chat 请求上(被 provider 静默忽略或报错)。修复方向正确(元数据不能选 wire,回落模型族规则对 gpt-4.1/deepseek-chat 返回 null、dual-wire 四方保留 native search,provider 白名单收窄到四方是有意的)。P1:本 PR 的另一半(usesOpenAiResponses + openAiResponsesThinking)针对的代码已被 main 删除/改名——usesOpenAiResponses 被 #2249(8/5)整体删除,openAiResponsesThinking 被 #2518(8/14)重命名收紧为 openAiResponsesEncryptedThinking;main 现在从 resolveModelRuntime().reasoningReplay.kind 单一来源推导 wire(precedence 由 reasoningReplayContract 与 resolveModelRuntimeWire 共享),正是本 PR 想要的"与 factory wire precedence 对齐"且无手工同步表。rebase 必冲突;强行保留会回退 #2518 的加密门控并重引入并行 precedence 表。处理:rebase 后整体丢弃 runtime 半,只留 core 半 + 测试。P2:PR 正文"rebuilt from latest main"不成立(CONFLICTING + 触碰的代码在 main 上已不存在),rebase 后重写 Verification。P3(可选):providerSelectsOpenAiWireByModel 漏了 alibaba(同样是 openai-responses 适配器 + qwen3.5 族规则,当前因 implemented:false 不可达,是潜在地雷);测试只测负例(返回 null),建议补正例(gpt-5.5 + webSearch:true + 无 apiProtocol → 仍返回 capability)锁死对齐方向。 |
Astro-Han
left a comment
There was a problem hiding this comment.
The core diagnosis is sound: web-search metadata must not silently change the model's selected wire protocol. The negative coverage for Chat models closes the original regression.
The current head is not mergeable with current main, however, and the Runtime half now conflicts with the newer single source of truth. From first principles, wire selection should have one authority. The smallest solution is to rebase, keep the Core resolver change and its tests, and remove the obsolete Runtime source/test changes instead of recreating a second precedence table. Then rerun the affected Core/Runtime checks on the rebased head.
Reviewed with Codex using two independent reviewer agents; I verified the latest head, current-main merge state, changed hunk, and CI state.
中文
核心问题判断是正确的:Web Search 元数据不应暗中改变模型选定的 wire protocol,新增的 Chat 模型负向用例也覆盖了原始回归。
但当前 head 无法与最新 main 合并,而且 Runtime 部分已经与新的单一权威来源冲突。按第一性原理,wire 选择应只有一个权威。最小方案是 rebase 后保留 Core resolver 的修改与测试,删除过时的 Runtime 源码和测试,而不是重新建立第二套优先级判断;之后基于新 head 重跑受影响的 Core/Runtime 检查。
本次由 Codex 配合两个独立 reviewer agent 审查;我核验了最新 head、与当前 main 的合并状态、变更 hunk 和 CI 状态。
| @@ -444,12 +444,26 @@ function usesKimiOpenAiChat(connection: RuntimeExecutionConnection, modelId: str | |||
|
|
|||
| function usesOpenAiResponses(connection: RuntimeExecutionConnection, modelId: string): boolean { | |||
There was a problem hiding this comment.
P1 — Remove the stale parallel wire resolver after rebasing. Current main caches ResolvedModelRuntime and derives replay/encrypted-thinking behavior from that resolved contract; this PR's Runtime half conflicts with it and refers to the superseded thinking shape. Retaining this helper would recreate a second wire-precedence authority and can diverge from the model factory. Rebase and drop the Runtime source/test half; the Core resolver change is the minimal fix for this PR's problem.
|
/agentic_review |
Code Review by Qodo
1. GPT-5 wire rules diverge
|
| providerSelectsOpenAiWireByModel(providerType) | ||
| ) { | ||
| return providerDefaultHostedWebSearchCapability(providerType, id, adapter); |
There was a problem hiding this comment.
1. Gpt-5 wire rules diverge 🐞 Bug ≡ Correctness
For an OpenAI model such as gpt-5foo with webSearch: true and no explicit protocol, this branch returns null even though the model factory selects OpenAI Responses. Native web search is therefore removed from a request whose effective wire supports it, violating the PR's wire-alignment invariant.
Agent Prompt
## Issue description
The new hosted-search metadata guard delegates to provider-specific search patterns that do not exactly match the model factory's protocol authority. In particular, `openAiAdapterApiProtocol` classifies every ID beginning with `gpt-5` as Responses, while the search fallback requires a delimiter after `gpt-5`, causing explicit `webSearch: true` metadata to be ignored for IDs such as `gpt-5foo`.
## Issue Context
Consolidate rather than add another model-family rule: import and reuse `openAiAdapterApiProtocol` when this branch needs to determine the effective wire. Keep the separate provider-default search rules for models without affirmative capability metadata, since those encode provider search support rather than wire selection.
## Fix Focus Areas
- packages/core/src/model-web-search.ts[50-75]
- packages/core/src/model-metadata.ts[91-109]
- packages/core/src/__tests__/model-web-search.test.ts[77-97]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Heads up — this has drifted into conflict with Worth knowing before you rebase: #3397 landed on 2026-08-22 and added ASF license headers across ~2685 files, so a rebase will touch more than you'd expect, and any file you add now needs a header ( I'd like to review this — just ping me once it's rebased and CI is green. |
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at exact head 864ea3a00bdd50c34ff19d076fa8d4041218c9fa. The branch's hosted checks are terminal green, but GitHub currently reports CONFLICTING / DIRTY; those pre-rebase checks do not validate the current runtime authority. The Core diagnosis remains sound, but one blocking inline finding must be resolved before approval.
| runtime.apiProtocol === 'openai-responses' || | ||
| openAiAdapterApiProtocol(modelId, connection.providerType) === 'openai-responses' | ||
| ); | ||
| switch (runtime.adapter.kind) { |
There was a problem hiding this comment.
[P1] Drop this Runtime-side wire precedence table when rebasing. This one-commit branch is 903 commits behind current main; a real rebase conflicts only in this model-adapter.ts file, and GitHub reports the current head as conflicting. Current main already resolves the effective wire once in resolveModelRuntime(...).wire and derives replay support from that runtime's reasoningReplay contract. Carrying this switch through the conflict would create a second authority that can drift from the model factory/wire resolver—the exact class of mismatch this PR is meant to fix. Keep the Core hosted-search correction and negative tests, remove this obsolete Runtime source/test half, then rerun affected Core/Runtime checks on the rebased exact head.
Follow-up to #2152.
What changed
openai-chatoverridesVerification
npm cinpm run build:testnpm run typecheckmainand reran the changed suites (33 tests)npm run lintnpm run format:checkgit diff --checkThe full parallel workspace run reached 6,400+ passing tests but exposed existing package-import warnings from
node:sqlitein unrelated workspaces; the changed and adjacent suites pass.