refactor(runtime): migrate web_search.rs to ToolError (#3576 slice)#6107
Conversation
One slice of the structured error-contracts migration. web_search.rs's 9 Result<String,String> tool fns now return Result<String, ToolError>, reusing existing variants: Unavailable for absent API keys / unconfigured SearXNG URL, InvalidParameter for bad pageno/category, upstream() for reqwest failures (preserving the reqwest::Error source chain per #3745), upstream_msg() for no-results / all-providers-failed. Per docs/architecture/error-contracts.md the LLM-visible strings change to the variant Display form; affected tests re-pinned to exact new strings. Brave key-rotation preserved by matching e.to_string(). Dispatch boundary still narrows ToolError->String. No cross-crate changes, no clippy gate added.
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
librefang-deploy | 472cd96 | Jun 14 2026, 10:43 PM |
github-actions
Bot
added
size/M
…HANGELOG CHANGELOG entry for the web_search ToolError migration had multiple sentences collapsed onto one line; split to one sentence per line per the prose-wrapping rule. Removed #3576 task-reference comments from dispatch.rs and three two-line comment blocks from the web_search tests — those notes belong in the PR description, not in code or test comments.
houko
left a comment
houko
left a comment
There was a problem hiding this comment.
Pushed one style commit (e20745c) with three mechanical fixes:
- CHANGELOG.md: the new entry had four sentences on a single line; split to one sentence per line per the prose-wrapping rule (new prose is in scope).
- dispatch.rs: removed the
// #3576: search() returns Result<String, ToolError>; narrow here.comment added by this PR — task-reference comments belong in the PR body, not in code. - web_search.rs tests: removed three
// #3576:comment blocks (two were multi-line) above the re-pinned assertions — same reason.
One ambiguous finding left as an inline comment: the search_auto DuckDuckGo fallback arm re-wraps e: ToolError in upstream_msg(format!("… {e}")), which may lose the reqwest::Error source chain the PR says is preserved.
Generated by Claude Code
| Ok(_) => Err("All search providers failed; DuckDuckGo returned empty results (likely captcha-blocked). Configure a Brave, Tavily, Jina, Perplexity, or SearXNG provider for reliable search.".to_string()), | ||
| Err(e) => Err(format!("All search providers exhausted. Last error (DuckDuckGo): {e}")), | ||
| Ok(_) => Err(ToolError::upstream_msg("All search providers failed; DuckDuckGo returned empty results (likely captcha-blocked). Configure a Brave, Tavily, Jina, Perplexity, or SearXNG provider for reliable search.")), | ||
| Err(e) => Err(ToolError::upstream_msg(format!("All search providers exhausted. Last error (DuckDuckGo): {e}"))), |
There was a problem hiding this comment.
The Err(e) => arm wraps an existing ToolError in a new upstream_msg(format!("… {e}")), converting e to its Display string and losing whatever source chain e carries (e.g. the reqwest::Error that search_duckduckgo may have returned).
The PR description says reqwest failures are routed via ToolError::upstream to preserve the source chain per #3745, but this specific arm re-wraps rather than propagates.
If the "all providers exhausted" context string matters, one option is to keep upstream_msg here since the message is the deliverable and the chain is already summarised in {e} — but that should be a conscious choice, not a side-effect. If the source chain matters at the dispatch boundary (e.g. for structured logging of the root reqwest::Error), the arm could propagate e directly and let search_auto's callers see the DuckDuckGo error as-is.
Generated by Claude Code
2 participants
Summary
One slice of the ongoing structured error-contracts migration (#3576). Migrates
crates/librefang-runtime/src/web_search.rsfrom stringly-typedResult<String, String>toResult<String, ToolError>, reusing existingToolErrorvariants.Per
docs/architecture/error-contracts.md, the migration intentionally changes the LLM-visible error strings to the variantDisplayform — affected tests are re-pinned to the exact new strings (strengthened from.contains()to exact.to_string()asserts).Variant mapping
Unavailablepageno == 0, invalid categoryInvalidParameter { name, reason }.send()/.json()/.text()failuresupstream(e)— preserves thereqwest::Errorsource chain (#3745)upstream_msg(...)Brave key-rotation logic preserved (matches on
e.to_string()). Dispatch boundary still narrowsToolError -> StringviaDisplay. Stays in-crate (librefang-runtime); no cross-crate error-shape changes; does not add theclippy::disallowed_typesgate.Test plan
cargo clippy -p librefang-runtime --all-targets -- -D warnings(verified: clean)cargo test -p librefang-runtime --lib web_search::(verified: 24 passed)cargo test -p librefang-runtime --lib tool_runner::error::(verified: 14 passed)Refs #3576 (one slice; does not close the issue).