Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a comprehensive refactoring of the responses and conversations system, introducing web search capabilities via Brave API, granular response item storage, and workspace-based authentication. The changes migrate from user-based to workspace-based scoping and add support for tool calling with web search functionality.
Key changes:
- Adds Brave web search integration with comprehensive parameter support
- Implements granular response item storage with separate
response_itemstable - Migrates from user-based to workspace-based authentication for conversations and responses
- Adds infrastructure for tool execution (web search, file search, current_date)
Reviewed Changes
Copilot reviewed 41 out of 42 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| env.example | Adds configuration for model inference timeout and Brave API key |
| crates/services/src/responses/tools/* | New modules for tool abstractions (web search, file search) and Brave implementation |
| crates/services/src/responses/service.rs | Complete rewrite implementing tool-calling agent loop with web search support |
| crates/database/src/repositories/response_item.rs | New repository for granular response item storage |
| crates/database/src/migrations/sql/V0020-V0022*.sql | Database migrations for response_items table and workspace migration |
| crates/api/src/routes/responses.rs | Updated to use new service layer with workspace authentication |
| crates/inference_providers/src/vllm/mod.rs | Improved header handling to prevent panics on invalid API keys |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let api_key = std::env::var("BRAVE_SEARCH_PRO_API_KEY").unwrap_or_else(|_| { | ||
| panic!("BRAVE_SEARCH_PRO_API_KEY is not set"); | ||
| }); |
There was a problem hiding this comment.
Using panic! in initialization code can cause the entire application to crash. Consider returning a Result type from the new() method or providing a graceful fallback/error message that doesn't panic.
| let country; | ||
| if let Some(ref c) = params.country { | ||
| country = c.clone(); | ||
| query_params.push(("country", country)); | ||
| } |
There was a problem hiding this comment.
This pattern is repeated for many optional parameters (lines 74-150), creating significant code duplication. Consider refactoring to use a macro or helper function to reduce duplication and improve maintainability.
| pub name: String, | ||
| /// Function name (optional in streaming mode where it may come in a later chunk) | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub name: Option<String>, |
There was a problem hiding this comment.
The arguments field at line 77 is also optional but lacks the documentation comment explaining when it's optional (in streaming mode). For consistency, add a similar comment to this field.
| pub name: Option<String>, | |
| pub name: Option<String>, | |
| /// Function arguments (optional in streaming mode where it may come in a later chunk) |
| DELETE FROM responses; | ||
| DELETE FROM conversations; |
There was a problem hiding this comment.
These DELETE statements will remove all existing data during migration. While the safety check prevents re-deletion, consider adding a comment warning that this migration is destructive and requires data backup, or provide a data migration script instead of deletion.
No description provided.