feat(web): Moonshot and Kimi native search (rescue of #5686, review findings fixed) - #5720
Conversation
Signed-off-by: hexin <372726039@qq.com>
Signed-off-by: hexin <372726039@qq.com>
…e search Rescue merge of abb9978 (Pinvou feat/moonshot-kimi-provider-native-search, PR #5686) onto current main (142 ahead). Contributor head preserved verbatim as the second parent; conflicts resolved following the landed native-search adapter pattern (#5682/#5683/#5690/#5691/#5693): main's Mimo/Zai/ModelStudio/ DeepSeek adapters and the post-fallback domain-constraint/budget plumbing stay, the Kimi module + Moonshot dispatch and capability wiring join them. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
…losed Moonshot/Kimi (and the other contracts without a domain filter field) report maximum_domain_count() == Some(0), so a domain-filtered query hit the shared InvalidInput branch and killed the whole backend chain instead of reaching the configured provider or DuckDuckGo. Decline with NotAvailable for that case so the chain falls back; keep InvalidInput for genuinely over-limit counts on providers that do honor domains (#5686 review). Signed-off-by: CodeWhale Bot <bot@codewhale.net>
The local case-insensitive whole-URL comparison let differently-cased paths (https://API.KIMI.COM/CODING/V1) inherit the official membership /search service. Reuse config's exact matcher (scheme/authority case-insensitive, path exact) for both the kimi.rs dispatch and the capability gate, matching the is_exact_kimi_code_route discipline everywhere else (#5686 review). Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Wikipedia-style URLs such as .../wiki/Foo_(bar) lost their balanced closing paren to the terminator scan. Track paren depth while scanning so balanced pairs stay part of the URL and an unmatched closer still ends it (#5686 review). Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @Hmbown's task in 6m 4s —— View job Claude's review of #5720Todo list
SummaryReviewed the diff between the PR's recorded pre-merge base ( One finding worth checking before merge (posted as an inline comment on
What looks solid
Environment noteThis sandboxed run's |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if tool_call.pointer("/function/name").and_then(Value::as_str) != Some("$web_search") { | ||
| bail!("Kimi native search requested an unexpected tool"); | ||
| } | ||
| let id = tool_call | ||
| .get("id") | ||
| .and_then(Value::as_str) | ||
| .context("Kimi native web-search call omitted id")?; | ||
| let arguments = tool_call | ||
| .pointer("/function/arguments") | ||
| .and_then(Value::as_str) | ||
| .context("Kimi native web-search call omitted arguments")?; | ||
| let _: Value = serde_json::from_str(arguments) | ||
| .context("Kimi native web-search arguments were not valid JSON")?; | ||
| messages.push(json!({ | ||
| "role": "tool", | ||
| "tool_call_id": id, | ||
| "name": "$web_search", | ||
| "content": arguments, | ||
| })); |
There was a problem hiding this comment.
search_builtin (used for direct kimi-k2.6) never calls out to fetch real search results for the $web_search tool call — it echoes the model's own generated arguments (the query it wants to search) back as the tool message content at line 121, then loops. Compare with search_formula just below, which POSTs to a .../fibers endpoint and injects the actual result (formula_fiber_result) before continuing.
If Moonshot's builtin_function tool type isn't fully server-executed within the same completion (i.e. the client is expected to supply real results, the way search_formula does), this path never performs a grounded search — the model just sees its own query parroted back, and the resulting "answer"/citations parsed by parse_final_message would be ungrounded rather than search-backed.
This is also the only one of the three new adapters with no mock-server round-trip test (k3_formula_executes_tool_fiber_and_returns_citations covers the Formula path, parse_kimi_code covers the Kimi Code path). Worth confirming against the actual $web_search builtin-function contract and adding an equivalent wiremock test for search_builtin.
|
Claude finished @Hmbown's task in 1m 48s —— View job Claude's review of #5720 (re-run after synchronize)Todo list
Summary
Still open — Minor/lower-confidence (also unchanged): Re-verified as correct
Environment noteNo network access was available in this run ( |
…, greening shared macOS/Windows CI Hosted `Test (macos-latest)` and `Test (windows-latest)` have been red on main since S1 landed, and every open PR riding main inherited the same failures (Hmbown#5712 Hmbown#5719 Hmbown#5720 Hmbown#5721 Hmbown#5703 Hmbown#5722 — verified from each exact head's own job logs). macOS (6 failures in sandbox::read_guard::tests): the hosted runner's $TMPDIR is /var/folders/... — a symlink into /private/var/... — so `canonicalize` and `current_dir` hand back the resolved spelling while the rule was only lexically normalized against the literal one. The canonicalized candidate could therefore never match a rule, and none of the symlink / denied-tree tests fired. Prior local verification passed only because it ran with TMPDIR on a plain volume. That is a product hole, not a test artifact: on macOS /etc, /var and /tmp are symlinks into /private, so `read_file /private/etc/sudoers` walked around the built-in /etc/sudoers rule (the Seatbelt setter already canonicalized its own copy of the list; the in-process matcher did not). A subtree rule now remembers its resolved spelling (`DenyRule::subtree`) and `check` matches a candidate against either spelling. Exemptions still compare the configured spelling only — unchanged, out of scope here. Windows (1 failure): root_parent_traversal_does_not_escape_above_root asserted a Unix `/etc` while `normalize_lexically("/../../etc")` correctly resolves a rooted-but-driveless path against the cwd's drive (`D:\etc` on the runner). The test now spells the traversal from the current drive root and keeps the Unix literal under `cfg!(unix)`. Defeat tests (fail on the unpatched matcher on every host, independent of TMPDIR): rule_spelled_through_a_symlinked_root_matches_the_resolved_spelling (unix) and macos_private_spelling_of_a_machine_wide_rule_is_refused (macOS). Local, hosted-equivalent (RUSTFLAGS=-Dwarnings, RUST_MIN_STACK=16MiB, cargo nextest run -p codewhale-tui --lib --all-features --locked --profile ci, TMPDIR = macOS default /var/folders/...): baseline at 20e9574: 22 run / 16 passed / 6 failed (the hosted six); after: 24 run / 24 passed. Defeat probe with rule resolution disabled (tests kept): plain TMPDIR 6 run / 2 failed = exactly the two new tests; symlinked TMPDIR 6 run / 4 failed. fmt clean; changelog synced. Hosted CI is the final gate. Not touched (branch-specific, not shared-main): Hmbown#5712's Windows runtime_api::tests::events_endpoint_respects_since_seq_cursor timeout (8.91 s vs the 8 s ci_scaled budget; Hmbown#5712 does not touch runtime_api) and Hmbown#5722's Ubuntu fleet::executor failure. No-Issue: shared-main CI repair; no tracking issue exists for the hosted-runner symlink shape. Signed-off-by: CodeWhale Bot <bot@codewhale.net> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @Hmbown's task in 47s —— View job Claude's review of #5720 (re-run after synchronize)Todo list
Summary
Still open — Minor/lower-confidence (also unchanged): Re-verified as correct (unchanged from prior passes)
Environment noteNo network access was available in this run, so I could not check Moonshot's actual |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| let domain_limit = client.maximum_domain_count(); | ||
| if !query.domains.is_empty() && domain_limit == Some(0) { | ||
| return Err(ToolError::not_available(format!( | ||
| "{} native web search cannot honor domain filters", | ||
| client.provider().as_str() | ||
| ))); |
There was a problem hiding this comment.
🟡 Fallback changes keep stale results
When maximum_domain_count declines a domain-filtered search, its fallback result excludes the fallback configuration from the cache key. Backend changes can return old results.
Prompt for agents
ProviderNativeSearchBackend now deliberately returns NotAvailable for domain-filtered queries on providers such as Moonshot, making the configured search backend the normal execution path. However, execute_search in crates/tui/src/tools/web_search.rs keys every provider-native-first search only by ProviderNativeSearchClient::cache_identity. It omits the configured fallback provider and search_base_url. Include the complete effective backend-chain identity in provider-native cache scope, so changing the configured fallback provider or endpoint cannot reuse a result produced by the old chain. Add a focused cache test using the same state namespace, native provider, and query with two different fallback configurations.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if provider_kind == ProviderKind::Moonshot { | ||
| let effective_base_url = req | ||
| .base_url_override | ||
| .as_deref() | ||
| .unwrap_or_else(|| descriptor.default_base_url()); | ||
| selected.capabilities.server_side_web_search = documented_moonshot_web_search_for_route( | ||
| provider_kind, | ||
| selected.wire_model_id.as_str(), | ||
| effective_base_url, | ||
| ); | ||
| } |
There was a problem hiding this comment.
📝 Info: Capability overwrite stays route-scoped
documented_moonshot_web_search_for_route runs after custom facts are cleared, but only exact endpoint/model pairs regain native search. Custom gateways remain unknown.
Was this helpful? React with 👍 or 👎 to provide feedback.
| kind == ProviderKind::Moonshot | ||
| && (is_exact_https_route(base_url, "api.moonshot.ai", "v1") | ||
| || is_exact_https_route(base_url, "api.moonshot.cn", "v1")) |
There was a problem hiding this comment.
📝 Info: China K3 keeps Formula budget
The existing Formula timeout uses the shared direct-route matcher. Adding the China host therefore grants it the same 180-second budget.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Moonshot/Kimi, Z.AI, MiMo, and the Responses-dialect routes cannot | ||
| // express domain filters in their native wire contracts. Declining | ||
| // here must not fail the whole search: report this backend unavailable | ||
| // so the chain falls back to the configured provider or DuckDuckGo, | ||
| // which honor domains natively or through post-filtering. | ||
| let domain_limit = client.maximum_domain_count(); | ||
| if !query.domains.is_empty() && domain_limit == Some(0) { | ||
| return Err(ToolError::not_available(format!( | ||
| "{} native web search cannot honor domain filters", | ||
| client.provider().as_str() | ||
| ))); | ||
| } | ||
| if let Some(maximum) = domain_limit | ||
| && query.domains.len() > maximum | ||
| { | ||
| return Err(ToolError::invalid_input(format!( |
There was a problem hiding this comment.
📝 Info: Domain refusal preserves fallback
maximum_domain_count returns zero for contracts without filtering, so NotAvailable advances the chain. Positive limits still reject oversized domain lists.
(Refers to this code)
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Balanced parentheses belong to the URL (Wikipedia titles such as | ||
| // `Foo_(bar)` keep their closing paren); an unmatched closer ends it. | ||
| let mut open_parens = 0_usize; | ||
| let end = tail | ||
| .char_indices() | ||
| .find_map(|(index, ch)| { | ||
| (index > 0 | ||
| .find_map(|(index, ch)| match ch { | ||
| '(' => { | ||
| open_parens += 1; | ||
| None | ||
| } | ||
| ')' if open_parens > 0 => { | ||
| open_parens -= 1; | ||
| None | ||
| } | ||
| _ => (index > 0 | ||
| && (ch.is_whitespace() | ||
| || matches!(ch, ')' | ']' | '}' | '>' | '"' | '\'' | '`'))) | ||
| .then_some(index) | ||
| .then_some(index), |
There was a problem hiding this comment.
📝 Info: Parenthesis scanning remains bounded
open_parens preserves balanced URL segments, while unmatched closers still terminate. Unclosed segments cannot consume text beyond the next whitespace.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Rescue of #5686 — @h3c-hexin's fork branch can't be force-pushed (403), and it conflicts with current main. This PR carries their exact commits
2258bf3cf7+abb997805(authorship preserved verbatim, second parent of the merge5f4dd7583), so #5686 auto-links as MERGED when this lands. This is the last slice of the #5681 native-search umbrella.Review findings fixed on top (the P1 was already satisfied on main by the contributor's own
d8a90177fd— KIMI_K3_FORMULA_MIN_TIMEOUT_MS = 180_000 withfirst == Some(Duration::from_millis(180_000))asserted; the PR doesn't touch web_search.rs):7e35b0447): zero-maximum_domain_count()providers (Moonshot et al.) with domain-filtered queries now decline withNotAvailableso the chain falls back to configured/DuckDuckGo, instead of erroring the whole search; over-limit on providers that honor domains staysInvalidInput. Test:zero_domain_native_providers_decline_without_failing_the_chain.3c525e636): Kimi Code dispatch now uses the exact route matcher (moonshot_base_url_is_exact_kimi_code: scheme/authority case-insensitive, path exact) —https://API.KIMI.COM/CODING/V1no longer misroutes to the official/search; the capability gate is tightened the same way.7ee65864d):citations_from_texttracks paren depth sowiki/Foo_(bar)and nestedBaz_(qux_(nested))extract intact; an unmatched closer still terminates.Conflict resolutions (additive throughout): main's adapter roster + Moonshot/Kimi modules merged; capabilities/resolver/tests kept main's modelstudio/deepseek/zai cases and added the PR's moonshot gate + tests; changelogs carry the PR's bullet.
Verification on the final tree: focused native search 24/24 (incl.
k3_formula_executes_tool_fiber_and_returns_citations); config 624 passed / 1 skipped; full TUI lib 11,530 passed / 0 failed / 13 skipped; fmt clean; CI-exact clippy exit 0.Closes #5686
Closes #5681 (with this slice the umbrella's six providers — Openai/Anthropic/Xai/Zai/ModelStudio/Deepseek arms plus Moonshot/Kimi — are all landed)