Repository navigation
Integrating Websearch tool - #324
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
Your plan includes 1 review of capacity. Refill in 36 minutes and 34 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR integrates agentic chat with Brave web search by extending model capabilities to flag tool-calling support, implementing a Brave Search tool, adding OpenAI streaming usage tracking, seeding and managing web search settings across backend and frontend, integrating an agentic flow with fallback to direct LLM chat, and refactoring DAO token-counting method names for consistency. ChangesAgentic Chat with Web Search Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
backend/chatservice/dao/db/postgres/scripts/seed/2_add_support_tool_calling.up.sql (1)
3-6: 💤 Low valueDifferent empty-checking logic across database backends.
The PostgreSQL migration uses
btrim(capabilities::text, '"') = ''to detect empty capabilities, which specifically catches JSONB empty strings (""), while the SQLite migration (line 4 in the SQLite file) usestrim(COALESCE(capabilities, '')) = '', which catches NULL and SQL empty strings. This inconsistency could lead to different behavior if the capabilities column contains edge-case values like""vs''vsNULLacross different database backends.Consider standardizing the empty-detection logic to ensure consistent behavior across PostgreSQL and SQLite, or document why the difference is intentional.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/chatservice/dao/db/postgres/scripts/seed/2_add_support_tool_calling.up.sql` around lines 3 - 6, The CASE expression that normalizes the capabilities column currently uses btrim(capabilities::text, '"') = '' in the Postgres migration while SQLite uses trim(COALESCE(capabilities, '')) = '', causing inconsistent empty detection; update the Postgres CASE (the block that checks capabilities) to use a consistent check such as COALESCE(NULLIF(btrim(capabilities::text, '"'), ''), '') = '' or COALESCE(trim(BOTH '"' FROM capabilities::text), '') = '' so NULL, SQL-empty string, and JSONB empty-string values are treated the same (i.e., map to '{}'::jsonb), and apply the same logic pattern used in the SQLite migration for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/chatservice/service/agentic_chat.go`:
- Line 24: The debug call currently logs the entire chat row
(slog.Debug("service:Chat", ... , "messageId", msg)), which can expose sensitive
user text; change the call to log only a stable identifier from the
dao.ChatMessageRow (for example msg.ID or msg.MessageID) and not the whole
struct or message text—update the slog.Debug invocation in the code that uses
dao.ChatMessageRow (the place that currently references msg) to include only the
identifier field and any non-sensitive metadata.
In `@backend/chatservice/service/brave_search_tool.go`:
- Around line 66-67: The log and error in BraveSearchTool's Execute currently
reference a specific environment variable; change them to be
configuration-agnostic by replacing the message string in slog.Error and the
returned fmt.Errorf to something like "Brave search API key not configured" (or
similar) so it reflects that the key may come from settings/config, not
necessarily an env var; update the messages in the BraveSearchTool.Execute
function to use the new, generic text.
- Around line 37-47: The constructors NewBraveSearchTool and
NewBraveSearchToolWithAPIKey currently assign http.DefaultClient (which has no
Timeout); change them to create and assign a dedicated http.Client with a
bounded Timeout (e.g., 5-15s or a configurable constant) so requests cannot hang
when ctx has no deadline; update both constructors to set httpClient:
&http.Client{Timeout: <duration>} instead of http.DefaultClient.
In `@backend/chatservice/service/service.go`:
- Around line 465-483: runAgenticChat currently fails and the caller immediately
falls back to a direct LLM call even if agentic streaming already emitted
tokens; change this to only allow fallback for pre-stream failures by
introducing a sentinel/typed error (e.g., ErrAgentStreamNotStarted or
ErrAgenticPreStreamFailure) returned by runAgenticChat when the failure occurred
before any response chunk was emitted, and return a different error or wrap the
real error when streaming has started; then update the caller (the block
invoking runAgenticChat in service.go) to only perform the fallback when the
returned error is the sentinel pre-stream error (otherwise propagate or log the
streaming-failure error without starting a second response stream or persisting
a duplicate assistant message).
In `@docs/design` decisions/WebSearch_tool.md:
- Around line 4-8: Edit the design doc text to fix spelling and grammar in the
listed sections: change "capabilites" to "capabilities" and rephrase "be check
the capabilities of the model" to "check the model's capabilities"; reword the
bulleted logic in service.go/Chat to a clear conditional sequence (e.g., "if the
message has no image and the model supports function/tool calling, ..."); change
"have websearch tool" to "have a web search tool" and ensure "call" is used as
"calling" or "tool-calling" consistently; apply similar copy edits to the other
flagged ranges (11-15, 23-26) so bullets read as coherent, grammatically correct
sentences and terminology like "sortedagents", "shared_models_metadata" and
"image support" are consistently cased and explained.
---
Nitpick comments:
In
`@backend/chatservice/dao/db/postgres/scripts/seed/2_add_support_tool_calling.up.sql`:
- Around line 3-6: The CASE expression that normalizes the capabilities column
currently uses btrim(capabilities::text, '"') = '' in the Postgres migration
while SQLite uses trim(COALESCE(capabilities, '')) = '', causing inconsistent
empty detection; update the Postgres CASE (the block that checks capabilities)
to use a consistent check such as COALESCE(NULLIF(btrim(capabilities::text,
'"'), ''), '') = '' or COALESCE(trim(BOTH '"' FROM capabilities::text), '') = ''
so NULL, SQL-empty string, and JSONB empty-string values are treated the same
(i.e., map to '{}'::jsonb), and apply the same logic pattern used in the SQLite
migration for consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4be4498f-659b-47c3-bc5b-24ce712f9ec0
⛔ Files ignored due to path filters (3)
backend/chatservice/proto/chatservice.pb.gois excluded by!**/*.pb.gobackend/chatservice/proto/chatservice_grpc.pb.gois excluded by!**/*.pb.gobackend/go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
backend/chatservice/dao/db/postgres/scripts/seed/2_add_support_tool_calling.up.sqlbackend/chatservice/dao/db/sqlite/scripts/seed/2_add_support_tool_calling.up.sqlbackend/chatservice/dao/models.gobackend/chatservice/service/agentic_chat.gobackend/chatservice/service/brave_search_tool.gobackend/chatservice/service/service.gobackend/chatservice/service/settings_service.gobackend/chatservice/sortedagents/llm.gobackend/chatservice/sortedagents/runner.gobackend/chatservice/sortedagents/runner_test.godocs/design decisions/WebSearch_tool.mdfrontend/proto/chatservice.tsfrontend/src/pages/setting.tsxfrontend/src/store/setting.tsproto/chatservice.proto
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces agentic chat capabilities, allowing models with tool-calling support to perform web searches using a new Brave Search tool. The implementation includes database updates for model metadata, backend logic for managing agentic sessions and token usage, and frontend components for configuring Brave Search API keys. Review feedback focuses on adhering to Go naming conventions for constants, using safer URL construction methods, simplifying redundant logic in the search tool, and correcting typos in the design documentation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/chatservice/service/brave_search_tool.go (1)
62-65:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDiscarded error from schema generation could hide configuration bugs.
GenerateSchemacan fail (e.g., invalid struct tags), but the error is silently ignored. If this fails,Parameters()returnsnil, which may cause downstream panics or unexpected behavior when the agent framework reads tool parameters.Suggested fix
func (t *BraveSearchTool) Parameters() *sortedagents.JSONSchema { - schema, _ := sortedagents.GenerateSchema[braveSearchArgs]() + schema, err := sortedagents.GenerateSchema[braveSearchArgs]() + if err != nil { + slog.Error("BraveSearchTool:Parameters", "message", "failed to generate schema", "error", err) + } return schema }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/chatservice/service/brave_search_tool.go` around lines 62 - 65, Parameters currently ignores the error returned by sortedagents.GenerateSchema which can hide schema generation failures; update the Parameters method (function Parameters on type BraveSearchTool) to capture the error from sortedagents.GenerateSchema[braveSearchArgs] and handle it instead of discarding it — e.g. if err != nil, log the error with context (using the package logger or the standard log package) and return nil (or a safe default schema) so failures are visible and won’t silently propagate; make sure to reference GenerateSchema and braveSearchArgs in the change so the error check is applied to that call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/pages/setting.tsx`:
- Around line 21-25: Validate braveSearchApiUrl before calling SetSetting: check
that webSearchApiUrl.trim() is a well-formed URL (e.g. by constructing a URL
object) and that its protocol is "https:"; if the input is empty allow falling
back to the default "https://api.search.brave.com/res/v1/web/search" but if a
non-empty value is provided reject malformed URLs (and non-https schemes), show
a user-facing validation error and do not call
SetSetting("tool.websearch.brave", ...) until the URL is valid; keep using the
braveSearchApiUrl and braveSearchApiKey variables and perform this validation
just before the SetSetting call.
---
Outside diff comments:
In `@backend/chatservice/service/brave_search_tool.go`:
- Around line 62-65: Parameters currently ignores the error returned by
sortedagents.GenerateSchema which can hide schema generation failures; update
the Parameters method (function Parameters on type BraveSearchTool) to capture
the error from sortedagents.GenerateSchema[braveSearchArgs] and handle it
instead of discarding it — e.g. if err != nil, log the error with context (using
the package logger or the standard log package) and return nil (or a safe
default schema) so failures are visible and won’t silently propagate; make sure
to reference GenerateSchema and braveSearchArgs in the change so the error check
is applied to that call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02083189-4039-4748-a175-2ff0a631cd68
📒 Files selected for processing (7)
backend/chatservice/service/agentic_chat.gobackend/chatservice/service/brave_search_tool.gobackend/chatservice/service/service.gobackend/chatservice/service/settings_service.godocs/design decisions/WebSearch_tool.mdfrontend/src/pages/setting.tsxfrontend/src/store/setting.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
docs/design decisions/WebSearch_tool.md (1)
57-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix wording and spelling in future-improvements bullets.
Line 57–58 still has readability issues (
teh, inconsistent term casing, and grammar). Please normalize these bullets to avoid ambiguity in design guidance.Suggested edit
-- we can entirely remove, direct llm call, if the model does not support function/Tool calling, simply start teh agent with no tools -- add scraping tool to get better result +- We can entirely remove the direct LLM call: if the model does not support function/tool calling, simply start the agent with no tools. +- Add a scraping tool to get better results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design` decisions/WebSearch_tool.md around lines 57 - 58, The two future-improvements bullets contain typos and inconsistent casing; update them to correct spelling, grammar, and normalize terminology so they read clearly — e.g., change "we can entirely remove, direct llm call, if the model does not support function/Tool calling, simply start teh agent with no tools" to a single clear sentence like "If the model does not support function/tool calling, start the agent without tools or fall back to a direct LLM call," and change "add scraping tool to get better result" to "Add a scraping tool to improve results" (capitalize consistently and use plural/definite articles where appropriate). Ensure both bullets use consistent casing for "tool/Tool" and "LLM" and correct "teh" to "the".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@docs/design` decisions/WebSearch_tool.md:
- Around line 57-58: The two future-improvements bullets contain typos and
inconsistent casing; update them to correct spelling, grammar, and normalize
terminology so they read clearly — e.g., change "we can entirely remove, direct
llm call, if the model does not support function/Tool calling, simply start teh
agent with no tools" to a single clear sentence like "If the model does not
support function/tool calling, start the agent without tools or fall back to a
direct LLM call," and change "add scraping tool to get better result" to "Add a
scraping tool to improve results" (capitalize consistently and use
plural/definite articles where appropriate). Ensure both bullets use consistent
casing for "tool/Tool" and "LLM" and correct "teh" to "the".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95f2e2c4-0edd-4553-a415-5cc9ae27e1e9
📒 Files selected for processing (1)
docs/design decisions/WebSearch_tool.md
xask00
approved these changes
May 25, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/chatservice/service/brave_search_tool.go (1)
68-105:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not log raw queries or raw upstream response bodies.
argscan contain user-entered search text, and the non-200 branch logs the full Brave response body. That puts arbitrary user content into server logs with no real debugging upside. Log stable metadata instead.Suggested fix
-func (t *BraveSearchTool) Execute(ctx context.Context, args map[string]any) (any, error) { - slog.Info("BraveSearchTool:Execute", "message", "executing brave search tool", "args", args) +func (t *BraveSearchTool) Execute(ctx context.Context, args map[string]any) (any, error) { if strings.TrimSpace(t.apiKey) == "" { slog.Error("BraveSearchTool:Execute", "message", "brave search api key is not configured") return nil, fmt.Errorf("brave search api key is not configured") } query, _ := args["query"].(string) query = strings.TrimSpace(query) if query == "" { slog.Error("BraveSearchTool:Execute", "message", "query parameter is required") return nil, fmt.Errorf("query parameter is required") } + slog.Info("BraveSearchTool:Execute", "message", "executing brave search tool", "queryLength", len(query)) @@ if resp.StatusCode != http.StatusOK { - slog.Error("BraveSearchTool:Execute", "message", "brave search failed", "statusCode", resp.StatusCode, "body", strings.TrimSpace(string(body))) + slog.Error("BraveSearchTool:Execute", "message", "brave search failed", "statusCode", resp.StatusCode, "bodyLength", len(body)) return nil, fmt.Errorf("brave search failed with status %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) }backend/chatservice/service/agentic_chat.go (1)
178-241:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not trigger direct-LLM fallback after agentic output has already started.
This function can stream progress/text and persist a partial assistant message before returning an error. The stack context says
backend/chatservice/service/service.gofalls back to direct LLM chat onrunAgenticChat()failure, so a mid-stream tool/runtime error here can produce a second assistant response and duplicate DB writes for the same request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/chatservice/service/agentic_chat.go` around lines 178 - 241, The handler currently returns a generic error from runAgenticChat when any mid-stream error occurs, which causes the caller (service.go) to fall back to direct LLM and emit a duplicate assistant response; change the behavior so that once any agentic output has been streamed you save the partial response (savePartialResponse) and return nil (or a non-fallback sentinel) instead of an error. Concretely, in agentic_chat.go update the error paths inside the events loop (the stream error cases and the sortedagents.ErrorEvent branch) and the final !completed && ctx.Err() block to check whether streaming started (e.g., firstToken == false or fullResponse.Len() > 0) and if so call savePartialResponse() and return nil; only return an error to trigger fallback when no agentic output was ever sent. This prevents runAgenticChat from triggering the direct-LLM fallback after partial/streamed agentic output.backend/chatservice/dao/dao_sqlite.go (1)
196-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap the insert and token bookkeeping in one transaction.
This inserts into
chat_messagesbefore loading model pricing and updating message/chat totals. If any later step fails, the method returns an error but still leaves the assistant message committed, so persisted history and aggregate token counts diverge.Suggested direction
+ tx, err := s.db.Beginx() + if err != nil { + return MessageSummary{}, fmt.Errorf("failed to begin transaction") + } + defer func() { + if err != nil { + _ = tx.Rollback() + } + }() + - result, err := s.db.Exec(` + result, err := tx.Exec(` INSERT INTO chat_messages ( chat_id, role, content, content_image, model, input_token_count, output_token_count, cached_token_count, user_id, document_references, rag_enabled ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, @@ - err = s.db.QueryRow(` + err = tx.QueryRow(` SELECT input_token_cost, output_token_cost, cached_token_cost FROM shared_models_metadata WHERE id = ?`, model).Scan(&inputCost, &outputCost, &cachedCost) @@ - _, err = s.db.Exec(` + _, err = tx.Exec(` UPDATE chat_messages SET cost = ? WHERE id = ?`, cost, messageId) @@ - _, err = s.db.Exec(` + _, err = tx.Exec(` UPDATE chat_list SET cost = COALESCE(cost, 0) + ?, input_token_count = COALESCE(input_token_count, 0) + ?, output_token_count = COALESCE(output_token_count, 0) + ?, cached_token_count = COALESCE(cached_token_count, 0) + ? WHERE chat_id = ? AND user_id = ?`, cost, inputTokens, outputTokens, cachedTokens, chatId, userID) + + if err = tx.Commit(); err != nil { + return MessageSummary{}, fmt.Errorf("failed to commit transaction") + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/chatservice/dao/dao_sqlite.go` around lines 196 - 253, The insert into chat_messages and subsequent model lookup and updates must be executed inside a single DB transaction to avoid partially-committed state; modify AddChatMessageWithTokenCount to start a transaction (tx := s.db.Begin/BeginTx), perform the INSERT into chat_messages, the SELECT from shared_models_metadata, UPDATE of chat_messages (cost) and UPDATE of chat_list using tx.Exec/tx.QueryRow (and use tx result.LastInsertId), and then commit; ensure any error along the way causes tx.Rollback and that all DB calls reference tx instead of s.db so the assistant message and token bookkeeping are atomic.backend/chatservice/dao/dao_postgres.go (1)
239-290:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid inserting the message before pricing is proven to exist.
If
shared_models_metadatahas no row for$5,calcis empty andScanreturnssql.ErrNoRows, but the independentinsCTE may already have inserted intochat_messages. That leaves a stored message without cost/chat totals even though this method reports failure.Suggested direction
- sqlQuery := ` - WITH ins AS ( - INSERT INTO chat_messages ( - chat_id, role, content, content_image, model, - input_token_count, output_token_count, cached_token_count, - user_id, document_references, rag_enabled - ) - VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11) - RETURNING id - ), - prices AS ( + sqlQuery := ` + WITH prices AS ( SELECT input_token_cost, output_token_cost, cached_token_cost FROM shared_models_metadata WHERE id = $5 + ), + ins AS ( + INSERT INTO chat_messages ( + chat_id, role, content, content_image, model, + input_token_count, output_token_count, cached_token_count, + user_id, document_references, rag_enabled + ) + SELECT $1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11 + FROM prices + RETURNING id ), calc AS ( SELECT (p.input_token_cost * $6 + p.output_token_cost * $7 + p.cached_token_cost * $8) / 1000000.0 AS cost FROM prices p ), @@ - SELECT ins.id, calc.cost - FROM ins, calc; + SELECT ins.id, calc.cost + FROM ins + JOIN calc ON true; `🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/chatservice/dao/dao_postgres.go` around lines 239 - 290, The current sqlQuery uses an ins CTE that inserts into chat_messages before verifying pricing in prices/calc, so if shared_models_metadata has no row the insert can commit while Scan returns sql.ErrNoRows; fix by ensuring pricing is validated before inserting: either (A) first query shared_models_metadata for model (the value passed as model/$5) inside AddChatMessageWithTokenCount (use p.db.QueryRow to fetch input_token_cost/output_token_cost/cached_token_cost and return an error if not found) and only then run the INSERT/updates, or (B) change the SQL so the INSERT depends on the prices CTE (e.g., perform INSERT ... SELECT ... FROM prices so no insert happens when prices is empty) and run the whole operation in a transaction; locate code around function AddChatMessageWithTokenCount, the variable sqlQuery, and the p.db.QueryRow(...).Scan(&messageId, &cost) call to apply the change.
♻️ Duplicate comments (1)
backend/chatservice/service/brave_search_tool.go (1)
37-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a bounded
http.Clientfor Brave calls.Both constructors still inject
http.DefaultClient, so a stalled Brave request can hang the chat path indefinitely when the incoming context has no deadline.Suggested fix
import ( "context" "encoding/json" "fmt" "io" "log/slog" "net/http" "net/url" "strings" + "time" settings "sortedstartup/chatservice/settings" "sortedstartup/chatservice/sortedagents" ) @@ func NewBraveSearchTool() *BraveSearchTool { return &BraveSearchTool{ - httpClient: http.DefaultClient, + httpClient: &http.Client{Timeout: 15 * time.Second}, apiURL: settings.DEFAULT_BRAVE_SEARCH_API_URL, } } @@ return &BraveSearchTool{ - httpClient: http.DefaultClient, + httpClient: &http.Client{Timeout: 15 * time.Second}, apiURL: apiURL, apiKey: apiKey, } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/chatservice/service/brave_search_tool.go` around lines 37 - 52, Both constructors (NewBraveSearchTool and NewBraveSearchToolWithConfig) currently set httpClient to http.DefaultClient which can hang; replace that with a bounded client instance (e.g., &http.Client{Timeout: <reasonableDuration>, Transport: http.DefaultTransport}) so Brave calls honor a global timeout and cannot stall forever; update both functions to initialize httpClient with this bounded client and keep apiURL/apiKey logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/chatservice/settings/settings.go`:
- Around line 315-327: extractChatDefaultPrompt currently falls back to
returning the raw stored payload even when the JSON is empty/invalid or the
"value" is missing/empty, causing literal JSON or blank prompts; update
extractChatDefaultPrompt to trim the input and, after attempting json.Unmarshal
into settingsMap, only return settingsMap["value"] if it is a non-empty,
non-whitespace string; otherwise return DEFAULT_CHAT_PROMPT. Use the existing
symbols (function extractChatDefaultPrompt, variable settingsMap, key "value",
and DEFAULT_CHAT_PROMPT) to locate the logic and ensure both the post-unmarshal
case and the plain/whitespace payload case return DEFAULT_CHAT_PROMPT instead of
the raw value.
---
Outside diff comments:
In `@backend/chatservice/dao/dao_postgres.go`:
- Around line 239-290: The current sqlQuery uses an ins CTE that inserts into
chat_messages before verifying pricing in prices/calc, so if
shared_models_metadata has no row the insert can commit while Scan returns
sql.ErrNoRows; fix by ensuring pricing is validated before inserting: either (A)
first query shared_models_metadata for model (the value passed as model/$5)
inside AddChatMessageWithTokenCount (use p.db.QueryRow to fetch
input_token_cost/output_token_cost/cached_token_cost and return an error if not
found) and only then run the INSERT/updates, or (B) change the SQL so the INSERT
depends on the prices CTE (e.g., perform INSERT ... SELECT ... FROM prices so no
insert happens when prices is empty) and run the whole operation in a
transaction; locate code around function AddChatMessageWithTokenCount, the
variable sqlQuery, and the p.db.QueryRow(...).Scan(&messageId, &cost) call to
apply the change.
In `@backend/chatservice/dao/dao_sqlite.go`:
- Around line 196-253: The insert into chat_messages and subsequent model lookup
and updates must be executed inside a single DB transaction to avoid
partially-committed state; modify AddChatMessageWithTokenCount to start a
transaction (tx := s.db.Begin/BeginTx), perform the INSERT into chat_messages,
the SELECT from shared_models_metadata, UPDATE of chat_messages (cost) and
UPDATE of chat_list using tx.Exec/tx.QueryRow (and use tx result.LastInsertId),
and then commit; ensure any error along the way causes tx.Rollback and that all
DB calls reference tx instead of s.db so the assistant message and token
bookkeeping are atomic.
In `@backend/chatservice/service/agentic_chat.go`:
- Around line 178-241: The handler currently returns a generic error from
runAgenticChat when any mid-stream error occurs, which causes the caller
(service.go) to fall back to direct LLM and emit a duplicate assistant response;
change the behavior so that once any agentic output has been streamed you save
the partial response (savePartialResponse) and return nil (or a non-fallback
sentinel) instead of an error. Concretely, in agentic_chat.go update the error
paths inside the events loop (the stream error cases and the
sortedagents.ErrorEvent branch) and the final !completed && ctx.Err() block to
check whether streaming started (e.g., firstToken == false or fullResponse.Len()
> 0) and if so call savePartialResponse() and return nil; only return an error
to trigger fallback when no agentic output was ever sent. This prevents
runAgenticChat from triggering the direct-LLM fallback after partial/streamed
agentic output.
---
Duplicate comments:
In `@backend/chatservice/service/brave_search_tool.go`:
- Around line 37-52: Both constructors (NewBraveSearchTool and
NewBraveSearchToolWithConfig) currently set httpClient to http.DefaultClient
which can hang; replace that with a bounded client instance (e.g.,
&http.Client{Timeout: <reasonableDuration>, Transport: http.DefaultTransport})
so Brave calls honor a global timeout and cannot stall forever; update both
functions to initialize httpClient with this bounded client and keep
apiURL/apiKey logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99f19f6f-d37a-41a7-a28b-753f40f49af4
📒 Files selected for processing (12)
backend/chatservice/api/api.gobackend/chatservice/dao/dao.gobackend/chatservice/dao/dao_postgres.gobackend/chatservice/dao/dao_sqlite.gobackend/chatservice/dao/dao_sqlite_test.gobackend/chatservice/service/agentic_chat.gobackend/chatservice/service/brave_search_tool.gobackend/chatservice/service/service.gobackend/chatservice/service/settings_service.gobackend/chatservice/settings/settings.gobackend/mono/main.godocs/features/agentic_chat.md
✅ Files skipped from review due to trivial changes (1)
- docs/features/agentic_chat.md
This was referenced May 25, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit