feat(web-search): ✨ add web search tool with multi-engine support#197
feat(web-search): ✨ add web search tool with multi-engine support#197jorben wants to merge 7 commits into
Conversation
…n filtering Migrate API key and base URL storage from single fields to per-engine maps, enabling independent configuration for each search engine. Remove legacy `clearApiKey` action in favor of saving an empty string. - Store `apiKeys` and `baseUrls` as engine-keyed maps in settings - Automatically migrate existing single `apiKey`/`baseUrl` on read - Update agent tool to remove `maxResults`/`includeRawContent` parameters (now exclusively controlled by app settings) - Improve agent tool descriptions for query and timeRange parameters - Add Brave domain filtering via `site:`/`-site:` query modifiers - Add Exa published date range support for timeRange parameter - Change default `includeRawContent` to `true` - Remove unused translation keys `alwaysOn` and `off` in profile agent access - UI: replace lock icon with read-only checkbox for built-in agents, clamp descriptions, clear API key input on engine change
… search Introduce ToolContext to bundle workspace path, writable roots, thread id, terminal manager, and database pool, simplifying function signatures across executors and tool gateway. Web search improvements: - Use a static OnceLock HTTP client to avoid recreating per request. - Remove providerResponse from tool output to reduce payload size. - Adjust topic_from_time_range to only return "news" for day and week. Subagent tool selection: - Conditionally add web_search tool based on user settings. - Pass a flag to helper_tools to control inclusion. UI: refine profile library card styling (font sizes, spacing, layout) and add debounced web search base URL update.
Add logic to transfer subagent access IDs from the source profile to the new duplicate when cloning a profile. On failure, the created profile is preserved and a warning is logged, ensuring robustness. Also replace inline error message construction in agents settings panel with a shared utility function for consistent invoke error handling.
AI Code Review SummaryPR: #197 (feat(web-search): ✨ add web search tool with multi-engine support) Overall AssessmentDetected 10 actionable findings, prioritize CRITICAL/HIGH before merge. Major Findings by Severity
Actionable Suggestions
Potential Risks
Test Suggestions
File-Level Coverage Notes
Inline Downgraded Items (processed but not inline)
Coverage Status
Uncovered list:
No-patch covered list:
Runtime/Budget
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| insert_string(&mut body, "country", country); | ||
| } | ||
|
|
||
| let value = post_json(client.post(endpoint).bearer_auth(api_key).json(&body)).await?; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| response_json(builder.send().await).await | ||
| } | ||
|
|
||
| async fn response_json( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) | ||
| })?; | ||
| let status = response.status(); | ||
| let body = response.text().await.map_err(|error| { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
| } | ||
|
|
||
| fn map_tavily_result(value: &Value) -> StandardSearchResult { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| fn parse_input(input: &Value, settings: &WebSearchSettings) -> Result<WebSearchInput, AppError> { | ||
| let query = input |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| const HTTP_TIMEOUT_SECS: u64 = 30; | ||
|
|
||
| static HTTP_CLIENT: OnceLock<reqwest::Client> = OnceLock::new(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
| } | ||
|
|
||
| pub async fn load_web_search_settings(pool: &SqlitePool) -> Result<WebSearchSettings, AppError> { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| return optimistic; | ||
| } | ||
|
|
||
| try { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| })); | ||
| } | ||
|
|
||
| export async function updateWebSearchSettings(patch: WebSearchSettingsPatch) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| </span> | ||
|
|
||
| <div | ||
| className={cn( |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Add MAX_QUERY_CHARS (500) and MAX_RESPONSE_BYTES (5MB) constants - Validate web search query length before making request - Check HTTP response size to prevent large payloads - Move Tavily API key from bearer auth to request body - Fix http_error function to accept &str instead of String - Update settings overlay to debounce max results value with onBlur - Add local state for max results to prevent premature saves
| profileList(), | ||
| promptCommandList(), | ||
| customSubagentList(), | ||
| settingsGet(WEB_SEARCH_SETTINGS_KEY), |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) | ||
| })?; | ||
| let status = response.status(); | ||
| let raw_body = response.bytes().await.map_err(|error| { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) | ||
| })?; | ||
| let status = response.status(); | ||
| let raw_body = response.bytes().await.map_err(|error| { |
There was a problem hiding this comment.
[MEDIUM] Unbounded HTTP Response Buffering
The response bytes are fully downloaded and buffered into memory via response.bytes().await prior to checking if the size exceeds MAX_RESPONSE_BYTES (5 MB). This exposes the app to memory exhaustion (OOM) if a search engine endpoint returns a massive payload.
Suggestion: Read the response as a chunked stream or check the Content-Length header first (as a quick pre-check, though not entirely sufficient on its own). For robust streaming, use a bounded reader or read from the byte stream while tracking the count, breaking early if it crosses the limit.
Risk: High memory allocation spikes or process crashes (OOM) if querying a compromised or malfunctioning search API that streams infinite or extremely large responses.
Confidence: 0.95
| ) | ||
| })?; | ||
| let status = response.status(); | ||
| let raw_body = response.bytes().await.map_err(|error| { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| [], | ||
| ); | ||
|
|
||
| const handleSaveWebSearchApiKey = async () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| ) | ||
| .then(async (profile) => { | ||
| const mapped = mapProfileDto(profile); | ||
| try { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| })); | ||
| } | ||
|
|
||
| export async function updateWebSearchSettings(patch: WebSearchSettingsPatch) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| const optimistic: WebSearchSettings = { | ||
| ...current, | ||
| ...patch, | ||
| hasApiKey: Object.prototype.hasOwnProperty.call(patch, "apiKey") |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
| }; | ||
|
|
||
| useEffect(() => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| className="w-full md:w-[360px]" | ||
| onChange={(event) => setWebSearchBaseUrl(event.target.value)} | ||
| onBlur={() => { | ||
| const trimmed = webSearchBaseUrl.trim(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Add localized error handling for slug conflicts when creating or saving custom agents. The application now detects the `custom_subagent.slug_conflict` error code and displays a specific translation with the conflicting slug, improving user feedback. Split error state into page-level (`pageErrorMessage`) and editor-level (`editorErrorMessage`) to show errors in their correct context. The editor now displays inline error messages near the header, with a dismiss button, while global errors like create/delete failures remain at the bottom. This provides clearer error attribution.
| const mapped = mapPersistedWebSearchSettings(persisted); | ||
| settingsStore.setState({ webSearch: mapped }); | ||
| return mapped; | ||
| } catch (error) { |
There was a problem hiding this comment.
[HIGH] Silent Failure of Web Search API Key Save on Backend Errors
Buddy, the updateWebSearchSettings function catches backend storage write rejections internally, prints a console warning, and returns the previous state (current). Since it does not propagate/rethrow the error, calling UI handlers like handleSaveWebSearchApiKey in GeneralSettingsPanel assume success and immediately clear the input text (setWebSearchApiKey("")). This leads to a silent failure where the user's input is cleared and lost without any UI-level error notification.
Suggestion: Modify updateWebSearchSettings to either rethrow errors, return a success boolean/status, or dispatch a distinct UI error notification. Then, update handleSaveWebSearchApiKey to only clear the text input on successful persistence, and keep the user's typed key intact if saving fails so they can retry or see the error message.
Risk: Users can lose their typed API keys or custom base URLs during background save failures with no visual indication of failure, causing hard-to-debug tool authentication errors.
Confidence: 0.90
| ) | ||
| })?; | ||
| let status = response.status(); | ||
| let raw_body = response.bytes().await.map_err(|error| { |
There was a problem hiding this comment.
[MEDIUM] Memory Exhaustion Risk due to Unrestricted Response Body Buffering
In web_search.rs, response.bytes().await is invoked to load the entire HTTP response body into memory before verifying its size against MAX_RESPONSE_BYTES. If a configured search engine endpoint returns an extremely large payload or an infinite stream, this can result in excessive memory consumption or an Out-Of-Memory (OOM) crash in the Tauri backend.
Suggestion: Buddy, consider checking the Content-Length header first (and rejecting the response if it exceeds MAX_RESPONSE_BYTES), or read the response body in chunks up to MAX_RESPONSE_BYTES using a limited stream reader to prevent downloading too much data into memory.
Risk: A malicious, compromised, or misconfigured search engine endpoint could return an extremely large payload (e.g. decompression bomb or endless stream), causing the Tauri application backend to crash due to memory exhaustion.
Confidence: 0.95
| ) | ||
| })?; | ||
| let status = response.status(); | ||
| let raw_body = response.bytes().await.map_err(|error| { |
There was a problem hiding this comment.
[MEDIUM] Potential Memory Exhaustion via Unbounded HTTP Response Download
The response.bytes().await call downloads the entire HTTP response body into memory before checking its length against MAX_RESPONSE_BYTES. If a custom or compromised endpoint returns an extremely large payload, this can lead to memory exhaustion.
Suggestion: Limit the body download size upfront by using a chunked reader or checking the Content-Length header, and short-circuit the request if the size exceeds MAX_RESPONSE_BYTES.
Risk: A massive payload from a customized base URL or faulty server could cause memory bloat or crash the Tauri app process.
Confidence: 0.90
| }); | ||
| }); | ||
|
|
||
| describe("updateWebSearchSettings", () => { |
There was a problem hiding this comment.
[MEDIUM] Missing Test Coverage for Web Search Settings Rejection & Rollback
Buddy, although there are several integration tests verifying happy-path persistence and key/URL switching for Web Search, there are zero tests verifying the behavior of updateWebSearchSettings when the backend storage settingsGet or settingsSet calls reject.
Suggestion: Add a test case in settings-ipc-actions.test.ts where mockSettingsSet or mockSettingsGet is mocked to reject with an Error, and assert that the store correctly rolls back to the pre-optimistic state.
Risk: Without test cases for rejections, regressions in state recovery/rollback logic could go undetected, causing the UI and store state to permanently drift from the backend state.
Confidence: 0.95
| @@ -0,0 +1,165 @@ | |||
| import type { WebSearchEngine, WebSearchSettings } from "@/modules/settings-center/model/types"; | |||
There was a problem hiding this comment.
[MEDIUM] Missing Unit Tests for Web Search Serialization & Parsing Utilities
Buddy, the pure parsing, mapping, and patch-building utility functions in web-search-settings.ts contain extensive normalization logic (e.g., negative or floating-point maxResults, nulls, undefineds, custom engines) and migration code (moving a legacy flat apiKey/baseUrl into multi-engine map structures). However, these are only tested indirectly via the store, rather than using highly focused unit tests.
Suggestion: Create a web-search-settings.test.ts unit test file in the same directory. Write tests verifying correct output when passing malformed values, edge cases like NaN or invalid engines, and legacy migration configurations.
Risk: A subtle regression in parsing or migration utilities could corrupt the persistent settings file or cause settings to silently revert to default states upon hydration.
Confidence: 0.95
| placeholder={webSearch.hasApiKey ? "••••••••••••" : t("settings.general.webSearchApiKeyPlaceholder")} | ||
| onChange={(event) => setWebSearchApiKey(event.target.value)} | ||
| /> | ||
| <Button |
There was a problem hiding this comment.
[MEDIUM] Ambiguous "Save" button behavior for empty Web Search API keys
When the API key input is cleared, the button remains enabled to allow the user to delete/clear the existing saved key. However, the button's label remains 'Save'. Clicking it deletes the key, which might surprise users expecting it to do nothing or save a blank key.
Suggestion: Buddy, let's dynamically set the button text to 'Clear' when the input is empty and a key exists, using the translation key t("settings.general.webSearchApiKeyClear").
Risk: Users could accidentally clear their API keys, or become confused on how to remove an API key because no explicit delete/clear button is shown.
Confidence: 0.95
| ), | ||
| )); | ||
| } | ||
| let body = String::from_utf8_lossy(&raw_body); |
There was a problem hiding this comment.
[LOW] Unnecessary UTF-8 conversion and string allocation before JSON parsing
On successful HTTP requests, the entire raw body (up to 5MB) is converted into a UTF-8 string before being parsed. This leads to redundant memory allocation and CPU cycles traversing the bytes.
Suggestion: Use serde_json::from_slice::(&raw_body) to parse the JSON directly from the raw byte buffer. Only construct the string lossy copy if the HTTP status code indicates an error and you need to construct an error message.
Risk: Increased memory churn and CPU overhead for larger search engine responses, especially when processing up to 5MB of search results.
Confidence: 0.90
| } | ||
| } | ||
|
|
||
| fn brave_query(query: &str, include_domains: &[String], exclude_domains: &[String]) -> String { |
There was a problem hiding this comment.
[LOW] Lack of Domain Validation in Query Construction
Domain inputs for include_domains and exclude_domains are incorporated into the query string without validation. If they contain spaces or search operator syntax, they could mutate the query's behavior.
Suggestion: Sanitize domain strings to guarantee they only contain alphanumeric, dots, and hyphens, and strip out any spaces.
Risk: Unexpected search behavior or search engine filtering mutations if malformed or query-injected domains are passed.
Confidence: 0.85
| const optimistic: WebSearchSettings = { | ||
| ...current, | ||
| ...patch, | ||
| hasApiKey: Object.prototype.hasOwnProperty.call(patch, "apiKey") |
There was a problem hiding this comment.
[LOW] Optimistic hasApiKey state inconsistency on engine switch in non-Tauri environments
In non-Tauri web development environments, changing engines keeps the old engine's hasApiKey status because the optimistic state falls back to current.hasApiKey when apiKey is not in the patch.
Suggestion: Buddy, we could reset or compute hasApiKey on engine changes if we are in non-Tauri mode, or we can just accept this minor web dev inconsistency.
Risk: Minor UI discrepancy during web-only mock testing/debugging where switching engines incorrectly shows a key as configured.
Confidence: 0.85
1 participant
Summary
Add a built-in web search tool with multi-engine support (Tavily, Brave, Exa, Firecrawl), configurable per-engine API keys and base URLs, domain filtering, and a full settings UI in the General settings panel.
Changes
Backend (Rust)
web_search.rs): implements tool execution with standardized result format across all four engines, including time-range filtering, domain include/exclude, and country hintsweb_search_settings.rs): persists and loads web search settings from SQLite, with per-engine API key and base URL maps, legacy migration, and sanitizationexecutors/mod.rs): introducesToolContextstruct to bundle workspace path, writable roots, thread ID, terminal manager, and DB pool — simplifies theexecute_toolsignature and passes the pool needed by web searchagent_session_tools.rs):web_searchtool is added to default and plan-read-only profiles only when the feature is enabled and an API key is configured; also wired into subagent helper profilestool_gateway.rs): updated to passToolContextinstead of individual parametersorchestrator.rs,runtime_orchestration.rs): propagates web search enabled state to helper agentsFrontend (TypeScript/React)
types.ts,settings-store.ts,defaults.ts): addsWebSearchSettingstype with engine, API key status, base URL, max results, and raw content toggleweb-search-settings.ts): handles mapping between persisted and in-memory formats, per-engine API key/base URL normalization, and legacy field migrationsettings-ipc-actions.ts,settings-ipc-actions.test.ts): addsupdateWebSearchSettingswith optimistic UI updates and backend sync; includes comprehensive unit tests for per-engine key/URL persistence and migrationsettings-center-overlay.tsx): adds Web Search section with enable toggle, engine picker, API key input (masked), base URL, max results slider, and raw content switchsettings-center-overlay.tsx): redesigns cards to show model brand icon and compact layout with hover-reveal actionsprofile-agent-access.tsx): replaces lock icon with checkbox for always-on agents, improves truncation and layoutagents-settings-panel.tsx): adds web_search to tool categories, uses shared error message helper, shows invocation descriptionen.ts,zh-CN.ts): adds translation keys for all web search settings labels and descriptionstool-names.ts): addsweb_searchto default-collapsed tools listTest Plan
npm run typecheckandnpm run test:unitcargo test --locked --manifest-path src-tauri/Cargo.toml🤖 Generated with TiyCode