Feature: Web Search #1276
Feature: Web Search #1276
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1276 +/- ##
==========================================
- Coverage 14.47% 14.46% -0.01%
==========================================
Files 2345 2346 +1
Lines 204298 204411 +113
Branches 184662 184775 +113
==========================================
+ Hits 29573 29574 +1
- Misses 173267 173379 +112
Partials 1458 1458 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GoodluckH
force-pushed
the
xipu/web-search
branch
3 times, most recently
from
26a3171 to
7270068
Compare
GoodluckH
force-pushed
the
xipu/web-search
branch
from
7270068 to
4901434
Compare
| @@ -175,6 +183,7 @@ impl ToolPermissions { | |||
| "execute_bash" => "trust read-only commands".dark_grey(), | |||
| "use_aws" => "trust read-only commands".dark_grey(), | |||
| "report_issue" => "trusted".dark_green().bold(), | |||
| "web_search" => "trusted".dark_green().bold(), | |||
There was a problem hiding this comment.
Don't think we should have this as trusted by default, the model could make requests to arbitrary websites that can track the user's activity in unwanted ways.
There was a problem hiding this comment.
+1. For ref, Claude Code's similar WebFetch requires permissions instead of being trusted by default: https://docs.anthropic.com/en/docs/agents-and-tools/claude-code/overview#tools-available-to-claude
| @@ -147,6 +147,33 @@ | |||
| ] | |||
| } | |||
| }, | |||
| "web_search": { | |||
| "name": "web_search", | |||
| "description": "Search/retrieving the web for the specified query. Currently only supports retrieving.", | |||
There was a problem hiding this comment.
Maybe hold off on including any search related functionality until it's supported? Model might hallucinate
| "Retrieving" | ||
| }, | ||
| if self.mode == WebSearchMode::Search { | ||
| self.query.as_ref().unwrap_or(&"".to_string()).clone().dark_green() |
There was a problem hiding this comment.
nit - unwrap_or(&"".to_string()).clone() seems strange, shouldn't need to clone here right?
| ); | ||
|
|
||
| let user_agent = "AmazonQCLI/1.0"; | ||
| let client = reqwest::Client::new(); |
There was a problem hiding this comment.
Use fig_request::client() instead, should be fine to add as a workspace dependency for this. It's used as part of checking for updates anyways. The user agent will be set appropriately
| .map_err(|e| eyre::eyre!("Failed to read response body: {}", e))?; | ||
|
|
||
| // Convert HTML to Markdown | ||
| let converter = HtmlToMarkdown::builder().skip_tags(vec!["script", "style"]).build(); |
There was a problem hiding this comment.
What's the reasoning behind this? Does the model not understand html well enough, or is this intended to primarily save on token usage?
Just asking since I would debate whether or not it's worth adding this dependency
| } | ||
|
|
||
| // Simple function to check if a path is allowed by robots.txt | ||
| fn is_allowed_by_robots_txt(robots_content: &str, user_agent: &str, path: &str) -> bool { |
There was a problem hiding this comment.
We should probably have some tests to verify robots txt parsing is correct
Currently only supports retrieving. We need to have more discussion to align on what search engine to use
websearch.mp4