Add BO Provider and improvements #66
Conversation
|
Claude finished @tysonthomas9's task —— View job PR Review: Add BO Provider and improvementsI've completed a comprehensive review of this large PR. Here are my findings: Todo List:
🔴 Critical Security Concerns1. ExecuteCodeTool - Major Security Risk
2. Visual Indicator Injection
🟡 Code Quality Issues3. BrowserOperatorProvider Hard-coded Endpoints
4. Error Handling Inconsistencies
🟠 Performance Considerations5. Large File Injection in VisualIndicatorTool
6. Missing Request Debouncing
🔵 Test Coverage Issues7. No Tests for New Critical Features
🟢 Positive Aspects8. Good Logging Infrastructure
9. Well-Structured Provider Architecture
10. Proper TypeScript Usage
📋 Specific RecommendationsImmediate Actions Required:
Code Improvements:// ExecuteCodeTool.ts - Add sandboxing
const ALLOWED_APIS = ['document.querySelector', 'document.querySelectorAll', 'Array.from'];
// Validate code against allowed APIs before execution
// BrowserOperatorProvider.ts - Make configurable
const DEFAULT_ENDPOINTS = [
'https://api.browseroperator.io/v1',
'https://backup.browseroperator.io/v1'
];
// VisualIndicatorTool.ts - Cache injection
if (!document.getElementById('devtools-agent-glow-style')) {
// Only inject if not already present
}Testing Strategy:
🎯 Overall AssessmentStrengths: Good architecture, comprehensive features, strong TypeScript usage The PR introduces valuable functionality but requires security hardening and comprehensive testing before it can be safely merged. |
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a BrowserOperator LLM provider and wires it across the AI Chat stack; registers three new tools (execute_code, update_todo, visual indicator); adds UI components for todos/files/viewing; injects file/todo context into agent runs; extends agent lifecycle hooks and emits session_completed events. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ChatView/InputBar
participant Service as AgentService
participant Runner as AgentRunner
participant LLM as BrowserOperatorProvider
participant API as BrowserOperator API
UI->>Service: request agent run / send messages (agentName)
Service->>Runner: runAgent(session, messages)
Runner->>LLM: callWithMessages(model, messages, {agentName})
LLM->>LLM: convert messages, add headers (X-Agent)
LLM->>API: POST /chat (agent header + payload)
API-->>LLM: response (choices / tool_calls)
LLM-->>Runner: parsed LLMResponse (text or tool_call)
Runner->>Runner: execute tool (if tool_call)
Runner->>Runner: emit progress events (tool_started/tool_completed)
alt completion
Runner->>Runner: emit session_completed
Runner-->>Service: final AgentSession
Service->>UI: AGENT_SESSION_COMPLETED -> update UI
end
sequenceDiagram
participant Agent as AgentRunner
participant FileMgr as FileStorageManager
participant Tool as UpdateTodoTool
Agent->>FileMgr: readFile("todos.md")
FileMgr-->>Agent: todoContent
Agent->>LLM: include CURRENT TODO LIST in system prompt
LLM-->>Agent: tool_call(update_todo)
Agent->>Tool: execute({todoList, reasoning})
Tool->>FileMgr: writeFile("todos.md", todoList)
FileMgr-->>Tool: success
Tool-->>Agent: UpdateTodoResult(success, counts)
Agent->>Agent: emit tool_completed and possibly clear todos on final/abort
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| if (!ctx?.provider || !ctx.nanoModel) { | ||
| throw new Error('Missing LLM context for extraction'); | ||
| } |
There was a problem hiding this comment.
In extractFromChunk the guard if (!ctx?.provider || !ctx.nanoModel) throws before the tool can call the LLM. However the call immediately afterwards already falls back to ctx.nanoModel || ctx.model, so the function is capable of working with just ctx.model. When the accessibility tree exceeds the token limit and the code takes the new chunking path, any configuration that doesn’t supply a nanoModel (e.g. users who only configure a main model) will now hit “Missing LLM context for extraction” and the schema extraction fails entirely. The check should allow either a nano model or a regular model instead of requiring nano specifically.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
front_end/panels/ai_chat/core/AgentNodes.ts (1)
228-242: Fix object-shorthand lint regression
agentName: agentNameviolates theobject-shorthandrule introduced in this hunk and will fail lint. Please collapse it toagentNamebefore landing.front_end/panels/ai_chat/core/LLMConfigurationManager.ts (2)
336-377: Persist browseroperator credentials like other providers
STORAGE_KEYS.BROWSEROPERATOR_API_KEYis defined andgetApiKey()reads it, butsaveProviderSpecificSettingsnever writes or clears it. Any key entered for BrowserOperator would be silently dropped, breaking persistence. Please mirror the other cases so the value is saved/removed correctly.
382-399: Redact browseroperator API key in storage-change logsWhen the BrowserOperator key changes,
handleStorageChangewill now log the first characters of the key because the new storage entry isn’t included insensitiveKeys. That’s a secret leak in logs. AddSTORAGE_KEYS.BROWSEROPERATOR_API_KEYto the redaction set so the value is treated like the other provider keys.front_end/panels/ai_chat/tools/Tools.ts (1)
1332-1402: ReturnedscrolledPagesreports0even whenpagesis omitted.When the caller scrolls by direction or absolute position, the response still includes
scrolledPages: 0, which implies a valid page count. That’s misleading for downstream reasoning. Make the property conditional.+```diff
scrolledPages: ${pages !== undefined ? pages : 0}
${pages !== undefined ? `scrolledPages: ${pages}` : ''}Or set the field only when `pages` is supplied. </blockquote></details> <details> <summary>front_end/panels/ai_chat/ui/AIChatPanel.ts (1)</summary><blockquote> `658-699`: **Add explicit handling for browseroperator model type.** The label generation logic at lines 675-678 doesn't explicitly handle the `'browseroperator'` case, causing it to fall through to the OpenAI label. Additionally, the conditional at line 692 doesn't include browseroperator when updating `MODEL_OPTIONS`. Apply this diff to fix the label generation: ```diff const newOption: ModelOption = { value: modelName, label: modelType === 'litellm' ? `LiteLLM: ${modelName}` : modelType === 'groq' ? `Groq: ${modelName}` : modelType === 'openrouter' ? `OpenRouter: ${modelName}` : + modelType === 'browseroperator' ? `BrowserOperator: ${modelName}` : `OpenAI: ${modelName}`, type: modelType };And update the conditional to include browseroperator:
// Update MODEL_OPTIONS for backwards compatibility if provider matches const currentProvider = localStorage.getItem(PROVIDER_SELECTION_KEY) || 'openai'; if ((currentProvider === 'openai' && modelType === 'openai') || (currentProvider === 'litellm' && modelType === 'litellm') || - (currentProvider === 'groq' && modelType === 'groq')) { + (currentProvider === 'groq' && modelType === 'groq') || + (currentProvider === 'browseroperator' && modelType === 'browseroperator')) { MODEL_OPTIONS = [...MODEL_OPTIONS, newOption]; localStorage.setItem('ai_chat_model_options', JSON.stringify(MODEL_OPTIONS)); }
🧹 Nitpick comments (8)
front_end/panels/ai_chat/tools/SequentialThinkingTool.ts (1)
279-290: Consider truncating the raw response in error messages.Including the full
rawResponseTextin error messages is excellent for debugging, but if the LLM returns a lengthy response (e.g., 2000+ characters), error logs could become unwieldy. Consider truncating to a reasonable preview length.Apply this diff to truncate long responses in error messages:
// Store raw response for error reporting const rawResponseText = response.text; + const truncatedResponse = rawResponseText.length > 500 + ? rawResponseText.substring(0, 500) + '... (truncated)' + : rawResponseText; // This will throw if JSON parsing fails, triggering a retry let parsedResult: SequentialThinkingResult; try { parsedResult = LLMResponseParser.parseStrictJSON(rawResponseText) as SequentialThinkingResult; } catch (parseError) { // Include full raw response in error for debugging const errorMsg = parseError instanceof Error ? parseError.message : String(parseError); - throw new Error(`Failed to parse response: ${errorMsg}. LLM returned: ${rawResponseText}`); + throw new Error(`Failed to parse response: ${errorMsg}. LLM returned: ${truncatedResponse}`); }And similarly for line 294:
// Validate result structure if (!parsedResult.currentState || !parsedResult.nextSteps || !Array.isArray(parsedResult.nextSteps)) { - throw new Error(`Invalid response structure from LLM - missing required fields. LLM returned: ${rawResponseText}`); + throw new Error(`Invalid response structure from LLM - missing required fields. LLM returned: ${truncatedResponse}`); }front_end/panels/ai_chat/core/PageInfoManager.ts (3)
8-8: Fix import ordering per ESLint rules.The import should be alphabetically ordered before
VisitHistoryManagerand needs proper grouping with an empty line.Apply this diff to fix the import order:
import * as Utils from '../common/utils.js'; // Path relative to core/ assuming utils.ts will be in common/ later, this will be common/utils.js -import { VisitHistoryManager } from '../tools/VisitHistoryManager.js'; // Path relative to core/ assuming VisitHistoryManager.ts will be in core/ -import { FileStorageManager } from '../tools/FileStorageManager.js'; + +import { FileStorageManager } from '../tools/FileStorageManager.js'; +import { VisitHistoryManager } from '../tools/VisitHistoryManager.js'; // Path relative to core/ assuming VisitHistoryManager.ts will be in core/ import { createLogger } from './Logger.js';
193-205: Consider fetching files after the pageInfo null check.Files are currently fetched before checking if
pageInfoexists (line 203). IfpageInfois null, the function returns early without using the files, making the fetch unnecessary.Apply this diff to improve efficiency:
const accessibilityTree = PageInfoManager.getInstance().getAccessibilityTree(); const iframeContent = PageInfoManager.getInstance().getIframeContent(); + // If no page info is available, return the original prompt + if (!pageInfo) { + return basePrompt; + } + // Get current session files const fileManager = FileStorageManager.getInstance(); let files: FileSummary[] = []; try { files = await fileManager.listFiles(); } catch (error) { logger.warn('Failed to fetch files for context:', error); } - - // If no page info is available, return the original prompt - if (!pageInfo) { - return basePrompt; - } // TODO: Move out of the system prompt and into a separate context prompt
236-236: Remove unnecessary parentheses around single arrow function parameter.ESLint flags the parentheses around
(file)as unnecessary for a single parameter.Apply this diff:
- ${files.map((file) => + ${files.map(file =>front_end/panels/ai_chat/LLM/LLMClient.ts (1)
165-169: Avoid casting toanyforagentName.
LLMCallRequestalready declaresagentName?: string, so we don’t need to cast the request toany. You can referencerequest.agentNamedirectly and keep the type safety.+```diff
- if ((request as any).agentName) {
options.agentName = (request as any).agentName;
- if (request.agentName) {
options.agentName = request.agentName;</blockquote></details> <details> <summary>front_end/panels/ai_chat/core/AgentService.ts (1)</summary><blockquote> `866-898`: **Wrap `case 'session_completed'` declarations in a block.** Declaring `const completedSession = …` directly inside the `case` trips `no-case-declarations` and can leak scope into other cases. Wrap this branch in braces. +```diff - case 'session_completed': - // Get the completed session from the event data or active sessions - const completedSession = progressEvent.data?.session || - this.#activeAgentSessions.get(progressEvent.sessionId); + case 'session_completed': { + // Get the completed session from the event data or active sessions + const completedSession = progressEvent.data?.session || + this.#activeAgentSessions.get(progressEvent.sessionId); … - break; + break; + }front_end/panels/ai_chat/tools/VisualIndicatorTool.ts (1)
32-38: Use specific types instead ofany.
agentServiceis accessed viagetActiveAgentSessions(), so we should type it asAgentService(or an interface exposing that method) to keep static checking intact. Same forcurrentToolInfo’s value type.+```diff
-import { createLogger } from '../core/Logger.js';
+import { createLogger } from '../core/Logger.js';
+import type { AgentService } from '../core/AgentService.js';
…
- private agentService: any | null = null; // AgentService reference for checking running state
- private agentService: AgentService | null = null; // AgentService reference for checking running state
…
- private currentToolInfo = new Map<string, { toolName: string, reasoning: string }>();
- private currentToolInfo = new Map<string, { toolName: string; reasoning: string }>();
</blockquote></details> <details> <summary>front_end/panels/ai_chat/ui/AIChatPanel.ts (1)</summary><blockquote> `21-21`: **Fix import order for consistency.** The `BrowserOperatorProvider` import should be placed before the `LLMClient` import to maintain consistent import ordering. This aligns with the ESLint rule violation flagged by static analysis. Apply this diff to fix the import order: ```diff import {AgentService, Events as AgentEvents} from '../core/AgentService.js'; -import { LLMClient } from '../LLM/LLMClient.js'; import { LLMConfigurationManager } from '../core/LLMConfigurationManager.js'; import { LLMProviderRegistry } from '../LLM/LLMProviderRegistry.js'; import { OpenAIProvider } from '../LLM/OpenAIProvider.js'; import { LiteLLMProvider } from '../LLM/LiteLLMProvider.js'; import { GroqProvider } from '../LLM/GroqProvider.js'; import { OpenRouterProvider } from '../LLM/OpenRouterProvider.js'; import { BrowserOperatorProvider } from '../LLM/BrowserOperatorProvider.js'; +import { LLMClient } from '../LLM/LLMClient.js'; import { createLogger } from '../core/Logger.js';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
config/gni/devtools_grd_files.gni(1 hunks)front_end/panels/ai_chat/BUILD.gn(4 hunks)front_end/panels/ai_chat/LLM/BrowserOperatorProvider.ts(1 hunks)front_end/panels/ai_chat/LLM/LLMClient.ts(6 hunks)front_end/panels/ai_chat/LLM/LLMTypes.ts(3 hunks)front_end/panels/ai_chat/agent_framework/AgentRunner.ts(10 hunks)front_end/panels/ai_chat/agent_framework/AgentRunnerEventBus.ts(1 hunks)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts(2 hunks)front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts(2 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/ResearchAgent.ts(2 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/SearchAgent.ts(4 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/WebTaskAgent.ts(2 hunks)front_end/panels/ai_chat/core/AgentNodes.ts(4 hunks)front_end/panels/ai_chat/core/AgentService.ts(8 hunks)front_end/panels/ai_chat/core/LLMConfigurationManager.ts(2 hunks)front_end/panels/ai_chat/core/PageInfoManager.ts(4 hunks)front_end/panels/ai_chat/tools/CombinedExtractionTool.ts(1 hunks)front_end/panels/ai_chat/tools/ExecuteCodeTool.ts(1 hunks)front_end/panels/ai_chat/tools/FullPageAccessibilityTreeToMarkdownTool.ts(1 hunks)front_end/panels/ai_chat/tools/HTMLToMarkdownTool.ts(2 hunks)front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts(7 hunks)front_end/panels/ai_chat/tools/SequentialThinkingTool.ts(1 hunks)front_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.ts(5 hunks)front_end/panels/ai_chat/tools/Tools.ts(8 hunks)front_end/panels/ai_chat/tools/UpdateTodoTool.ts(1 hunks)front_end/panels/ai_chat/tools/VisualIndicatorTool.ts(1 hunks)front_end/panels/ai_chat/ui/AIChatPanel.ts(18 hunks)front_end/panels/ai_chat/ui/ChatView.ts(5 hunks)front_end/panels/ai_chat/ui/SettingsDialog.ts(11 hunks)front_end/panels/ai_chat/ui/__tests__/ChatViewAgentSessions.test.ts(1 hunks)front_end/panels/ai_chat/ui/input/InputBar.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-24T14:13:09.842Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-09-24T14:13:09.842Z
Learning: Applies to eval-server/nodejs/src/config.js : Require OPENAI_API_KEY and PORT (default 8080 if unspecified); optionally support GROQ_API_KEY, OPENROUTER_API_KEY, LITELLM_ENDPOINT, LITELLM_API_KEY, DEFAULT_PROVIDER, DEFAULT_MAIN_MODEL, DEFAULT_MINI_MODEL, DEFAULT_NANO_MODEL
Applied to files:
front_end/panels/ai_chat/core/LLMConfigurationManager.tsfront_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.tsfront_end/panels/ai_chat/tools/FullPageAccessibilityTreeToMarkdownTool.tsfront_end/panels/ai_chat/core/AgentService.tsfront_end/panels/ai_chat/LLM/BrowserOperatorProvider.tsfront_end/panels/ai_chat/ui/AIChatPanel.tsfront_end/panels/ai_chat/ui/SettingsDialog.ts
📚 Learning: 2025-09-24T14:13:09.842Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-09-24T14:13:09.842Z
Learning: Applies to eval-server/nodejs/src/evaluator.js : LLM evaluator must integrate with OpenAI for LLM-as-a-judge functionality
Applied to files:
front_end/panels/ai_chat/tools/SequentialThinkingTool.tsfront_end/panels/ai_chat/ui/AIChatPanel.ts
📚 Learning: 2025-09-24T14:13:09.842Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-09-24T14:13:09.842Z
Learning: Applies to eval-server/nodejs/src/rpc-client.js : RPC client must support the configure_llm method for dynamic LLM provider configuration with the documented parameter structure
Applied to files:
front_end/panels/ai_chat/ui/AIChatPanel.ts
📚 Learning: 2025-09-24T14:13:09.842Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-09-24T14:13:09.842Z
Learning: Applies to eval-server/nodejs/src/server.js : src/server.js must accept WebSocket connections from LLM agents, manage agent lifecycle (connect, ready, disconnect), orchestrate evaluation sessions, and handle bidirectional RPC
Applied to files:
front_end/panels/ai_chat/ui/AIChatPanel.ts
🧬 Code graph analysis (21)
front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts (3)
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (1)
ToolRegistry(189-236)front_end/panels/ai_chat/tools/UpdateTodoTool.ts (1)
UpdateTodoTool(25-129)front_end/panels/ai_chat/tools/ExecuteCodeTool.ts (1)
ExecuteCodeTool(23-123)
front_end/panels/ai_chat/tools/ExecuteCodeTool.ts (2)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/tools/Tools.ts (2)
Tool(43-52)LLMContext(57-65)
front_end/panels/ai_chat/core/AgentNodes.ts (1)
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (1)
ConfigurableAgentTool(306-549)
front_end/panels/ai_chat/core/LLMConfigurationManager.ts (2)
front_end/panels/ai_chat/ui/AIChatPanel.ts (1)
localStorage(858-881)front_end/panels/ai_chat/core/Constants.ts (1)
STORAGE_KEYS(39-45)
front_end/panels/ai_chat/tools/SequentialThinkingTool.ts (1)
front_end/panels/ai_chat/LLM/LLMResponseParser.ts (1)
LLMResponseParser(13-306)
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (1)
front_end/panels/ai_chat/ui/AIChatPanel.ts (4)
provider(1514-1580)apiKey(1227-1258)apiKey(1300-1319)apiKey(1635-1669)
front_end/panels/ai_chat/tools/VisualIndicatorTool.ts (3)
front_end/panels/ai_chat/core/AgentService.ts (4)
logger(350-361)event(831-900)session(903-943)sessionId(955-963)front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/agent_framework/AgentRunnerEventBus.ts (2)
AgentRunnerEventBus(16-31)AgentRunnerProgressEvent(7-14)
front_end/panels/ai_chat/ui/ChatView.ts (1)
front_end/panels/ai_chat/ui/AIChatPanel.ts (2)
data(1586-1597)data(2067-2101)
front_end/panels/ai_chat/core/PageInfoManager.ts (1)
front_end/panels/ai_chat/tools/FileStorageManager.ts (1)
FileStorageManager(44-281)
front_end/panels/ai_chat/tools/CombinedExtractionTool.ts (1)
front_end/panels/ai_chat/ui/AIChatPanel.ts (4)
provider(1514-1580)apiKey(1227-1258)apiKey(1300-1319)apiKey(1635-1669)
front_end/panels/ai_chat/tools/UpdateTodoTool.ts (3)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/tools/Tools.ts (2)
Tool(43-52)LLMContext(57-65)front_end/panels/ai_chat/tools/FileStorageManager.ts (1)
FileStorageManager(44-281)
front_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.ts (2)
front_end/panels/ai_chat/tools/Tools.ts (1)
LLMContext(57-65)front_end/panels/ai_chat/core/AgentService.ts (1)
AgentService(47-964)
front_end/panels/ai_chat/tools/FullPageAccessibilityTreeToMarkdownTool.ts (1)
front_end/panels/ai_chat/ui/AIChatPanel.ts (4)
provider(1514-1580)apiKey(1227-1258)apiKey(1300-1319)apiKey(1635-1669)
front_end/panels/ai_chat/core/AgentService.ts (3)
front_end/panels/ai_chat/agent_framework/AgentSessionTypes.ts (1)
AgentSession(11-41)front_end/panels/ai_chat/agent_framework/AgentRunnerEventBus.ts (1)
AgentRunnerEventBus(16-31)front_end/panels/ai_chat/tools/VisualIndicatorTool.ts (1)
VisualIndicatorManager(29-683)
front_end/panels/ai_chat/LLM/BrowserOperatorProvider.ts (4)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/LLM/LLMTypes.ts (5)
LLMProvider(145-145)LLMMessage(179-192)LLMResponse(209-220)LLMCallOptions(197-204)ModelInfo(235-240)front_end/panels/ai_chat/LLM/LLMErrorHandler.ts (1)
LLMRetryManager(156-351)front_end/panels/ai_chat/LLM/LLMResponseParser.ts (1)
LLMResponseParser(13-306)
front_end/panels/ai_chat/agent_framework/AgentRunner.ts (2)
front_end/panels/ai_chat/tools/FileStorageManager.ts (1)
FileStorageManager(44-281)front_end/panels/ai_chat/core/PageInfoManager.ts (1)
enhancePromptWithPageContext(184-262)
front_end/panels/ai_chat/ui/AIChatPanel.ts (6)
front_end/panels/ai_chat/ui/input/InputBar.ts (1)
currentProvider(39-39)front_end/panels/ai_chat/ui/EvaluationDialog.ts (1)
currentProvider(231-286)front_end/panels/ai_chat/LLM/LLMTypes.ts (1)
ModelOption(91-95)front_end/panels/ai_chat/core/AgentService.ts (1)
selectedProvider(787-826)front_end/panels/ai_chat/LLM/LLMClient.ts (1)
LLMClient(51-428)front_end/panels/ai_chat/LLM/BrowserOperatorProvider.ts (1)
BrowserOperatorProvider(25-428)
front_end/panels/ai_chat/tools/HTMLToMarkdownTool.ts (1)
front_end/panels/ai_chat/ui/AIChatPanel.ts (4)
provider(1514-1580)apiKey(1227-1258)apiKey(1300-1319)apiKey(1635-1669)
front_end/panels/ai_chat/LLM/LLMClient.ts (2)
front_end/panels/ai_chat/LLM/BrowserOperatorProvider.ts (1)
BrowserOperatorProvider(25-428)front_end/panels/ai_chat/ui/AIChatPanel.ts (4)
data(1586-1597)data(2067-2101)error(1986-1999)provider(1514-1580)
front_end/panels/ai_chat/ui/SettingsDialog.ts (2)
front_end/panels/ai_chat/ui/AIChatPanel.ts (11)
currentProvider(919-979)currentProvider(2416-2475)localStorage(858-881)selectedProvider(886-914)selectedProvider(1487-1507)selectedProvider(1602-1612)selectedProvider(1948-1972)ModelOption(100-104)provider(1514-1580)updateModelOptions(515-650)SettingsDialog(2167-2181)front_end/panels/ai_chat/LLM/LLMTypes.ts (1)
ModelOption(91-95)
front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts (4)
front_end/panels/ai_chat/core/AgentService.ts (2)
AgentService(47-964)logger(350-361)front_end/panels/ai_chat/tools/Tools.ts (1)
LLMContext(57-65)front_end/panels/ai_chat/tools/LLMTracingWrapper.ts (1)
callLLMWithTracing(28-139)front_end/panels/ai_chat/LLM/LLMResponseParser.ts (1)
LLMResponseParser(13-306)
🪛 ast-grep (0.39.7)
front_end/panels/ai_chat/ui/SettingsDialog.ts
[warning] 2592-2595: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: browseroperatorInfoText.innerHTML = <strong>Endpoint:</strong> http://localhost:8080/v1 (hardcoded)<br> <strong>Agent Routing:</strong> Automatic based on calling agent (e.g., research_agent, action_agent)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 2592-2595: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: browseroperatorInfoText.innerHTML = <strong>Endpoint:</strong> http://localhost:8080/v1 (hardcoded)<br> <strong>Agent Routing:</strong> Automatic based on calling agent (e.g., research_agent, action_agent)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🪛 Biome (2.1.2)
front_end/panels/ai_chat/core/AgentService.ts
[error] 868-869: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 ESLint
front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts
[error] 13-13: ../../tools/ExecuteCodeTool.js import should occur before import of ../../tools/FetcherTool.js
(import/order)
front_end/panels/ai_chat/tools/ExecuteCodeTool.ts
[error] 6-6: There should be at least one empty line between import groups
(import/order)
[error] 23-23: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 70-70: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
front_end/panels/ai_chat/core/AgentNodes.ts
[error] 241-241: Expected property shorthand.
(object-shorthand)
[error] 897-897: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
front_end/panels/ai_chat/tools/VisualIndicatorTool.ts
[error] 5-5: All imports in the declaration are only used as types. Use import type.
(@typescript-eslint/consistent-type-imports)
[error] 8-8: ../agent_framework/AgentRunnerEventBus.js import should occur before import of ../core/Logger.js
(import/order)
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 54-54: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 194-194: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 199-199: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 503-503: Array type using 'Array' is forbidden for simple types. Use 'any[]' instead.
(@typescript-eslint/array-type)
[error] 503-503: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 508-508: Expected { after 'if' condition.
(curly)
[error] 567-567: Array type using 'Array' is forbidden for simple types. Use 'any[]' instead.
(@typescript-eslint/array-type)
[error] 567-567: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
front_end/panels/ai_chat/core/PageInfoManager.ts
[error] 8-8: There should be at least one empty line between import groups
(import/order)
[error] 8-8: ../tools/FileStorageManager.js import should occur before import of ../tools/VisitHistoryManager.js
(import/order)
[error] 195-195: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 236-236: Unexpected parentheses around single function argument.
(@stylistic/arrow-parens)
front_end/panels/ai_chat/tools/UpdateTodoTool.ts
[error] 5-5: There should be at least one empty line between import groups
(import/order)
[error] 7-7: ./FileStorageManager.js import should occur before type import of ./Tools.js
(import/order)
[error] 67-67: 'result' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
[error] 78-78: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 87-87: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 104-104: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 104-104: Expected a comma.
(@stylistic/member-delimiter-style)
front_end/panels/ai_chat/core/AgentService.ts
[error] 26-26: ../tools/VisualIndicatorTool.js import should occur before import of ./Graph.js
(import/order)
[error] 868-869: Unexpected lexical declaration in case block.
(no-case-declarations)
front_end/panels/ai_chat/LLM/BrowserOperatorProvider.ts
[error] 6-6: ./LLMProvider.js import should occur before type import of ./LLMTypes.js
(import/order)
[error] 7-7: ./LLMErrorHandler.js import should occur before type import of ./LLMTypes.js
(import/order)
[error] 8-8: There should be at least one empty line between import groups
(import/order)
[error] 8-8: ./LLMResponseParser.js import should occur before type import of ./LLMTypes.js
(import/order)
[error] 9-9: ../core/Logger.js import should occur before type import of ./LLMTypes.js
(import/order)
[error] 61-61: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 63-63: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 71-71: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 105-105: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 105-105: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 190-190: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 243-243: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
front_end/panels/ai_chat/ui/__tests__/ChatViewAgentSessions.test.ts
[error] 348-348: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 349-349: Avoid using methods that render DOM directly into the body. Prefer the renderElementIntoDOM helper.
(rulesdir/no-document-body-mutation)
[error] 355-355: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 356-356: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 357-357: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
front_end/panels/ai_chat/agent_framework/AgentRunner.ts
[error] 18-18: ../tools/FileStorageManager.js import should occur before type import of ../tools/Tools.js
(import/order)
[error] 704-704: Expected property shorthand.
(object-shorthand)
front_end/panels/ai_chat/ui/AIChatPanel.ts
[error] 21-21: ../LLM/BrowserOperatorProvider.js import should occur before import of ../LLM/LLMClient.js
(import/order)
front_end/panels/ai_chat/LLM/LLMClient.ts
[error] 11-11: ./BrowserOperatorProvider.js import should occur before type import of ./LLMTypes.js
(import/order)
[error] 166-166: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 167-167: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
front_end/panels/ai_chat/ui/SettingsDialog.ts
[error] 529-529: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 530-530: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts
[error] 24-24: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 25-25: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 26-26: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 30-30: 'ChunkExtractionResult' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 32-32: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 104-104: 'reasoning' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
[error] 201-201: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 216-216: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 997-997: Missing radix parameter.
(radix)
[error] 1123-1123: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 1194-1194: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 1199-1199: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 1200-1200: 'apiKey' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (2)
front_end/panels/ai_chat/ui/AIChatPanel.ts (2)
1549-1551: LGTM!The browseroperator provider case correctly instantiates with a null API key, which aligns with the provider design indicating that BrowserOperator doesn't require API key credentials.
103-103: Well-integrated browseroperator provider support.The browseroperator provider integration is comprehensive and follows consistent patterns established by other providers (groq, openrouter):
- Type definitions properly extended across all relevant signatures
- Model option handling includes browseroperator in storage, retrieval, and update flows
- Placeholder logic matches the pattern used for other providers
- Credential validation correctly includes browseroperator
- UI state properly exposes the current provider to child components
The implementation maintains good consistency with existing provider patterns throughout the codebase.
Also applies to: 139-144, 359-403, 476-476, 547-580, 611-621, 1492-1492, 2087-2087
| import { FileStorageManager } from '../tools/FileStorageManager.js'; | ||
|
|
There was a problem hiding this comment.
Restore import ordering to satisfy lint.
ESLint (import/order) now fails because the value import from ../tools/FileStorageManager.js appears after the type-only import from ../tools/Tools.js. Move the value import above the type import (and drop the late duplicate) so the build passes.
-import type { Tool } from '../tools/Tools.js';
+import { FileStorageManager } from '../tools/FileStorageManager.js';
+import type { Tool } from '../tools/Tools.js';
@@
-import { sanitizeMessagesForModel } from '../LLM/MessageSanitizer.js';
-import { FileStorageManager } from '../tools/FileStorageManager.js';
+import { sanitizeMessagesForModel } from '../LLM/MessageSanitizer.js';Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 18-18: ../tools/FileStorageManager.js import should occur before type import of ../tools/Tools.js
(import/order)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/agent_framework/AgentRunner.ts around lines 18 to
19, the value import for FileStorageManager is placed after a type-only import
from ../tools/Tools.js which violates ESLint import/order; move the value import
statement for FileStorageManager so it appears before the type-only import from
../tools/Tools.js and remove any duplicate import of FileStorageManager later in
the file, ensuring value imports come before type-only imports to satisfy the
linter.
| systemPrompt: currentSystemPrompt, | ||
| tools: toolSchemas, | ||
| temperature: temperature ?? 0, | ||
| agentName: agentName, // Pass agent identity for provider-specific routing |
There was a problem hiding this comment.
Fix object shorthand to clear ESLint.
ESLint (object-shorthand) flags agentName: agentName. Switch to property shorthand so linting succeeds.
- agentName: agentName, // Pass agent identity for provider-specific routing
+ agentName, // Pass agent identity for provider-specific routing📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| agentName: agentName, // Pass agent identity for provider-specific routing | |
| agentName, // Pass agent identity for provider-specific routing |
🧰 Tools
🪛 ESLint
[error] 704-704: Expected property shorthand.
(object-shorthand)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/agent_framework/AgentRunner.ts around line 704, the
object literal uses the verbose form "agentName: agentName" which triggers the
ESLint object-shorthand rule; change it to the property shorthand "agentName"
(i.e. remove the duplicate key:value) so the object uses concise shorthand
syntax and ESLint no longer flags the line.
| // Get current session files | ||
| const fileManager = FileStorageManager.getInstance(); | ||
| let files: any[] = []; | ||
| try { | ||
| files = await fileManager.listFiles(); | ||
| } catch (error) { | ||
| logger.warn('Failed to fetch files for context:', error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any[] with proper type FileSummary[].
The FileStorageManager.listFiles() method returns Promise<FileSummary[]>, so the explicit type should be used instead of any[] for type safety.
Apply this diff:
// Get current session files
const fileManager = FileStorageManager.getInstance();
- let files: any[] = [];
+ let files: FileSummary[] = [];
try {Additionally, you'll need to import the type:
import { FileStorageManager, type FileSummary } from '../tools/FileStorageManager.js';🧰 Tools
🪛 ESLint
[error] 195-195: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/core/PageInfoManager.ts around lines 193 to 200, the
variable files is typed as any[] but FileStorageManager.listFiles() returns
Promise<FileSummary[]>; change the declaration to use the FileSummary type
(e.g., let files: FileSummary[] = []) and add the required import at the top:
import { FileStorageManager, type FileSummary } from
'../tools/FileStorageManager.js'; this ensures correct typing while keeping the
existing try/catch and usage intact.
| const requestHeaders = { | ||
| 'Content-Type': 'application/json', | ||
| 'X-Agent': selectedAgent, | ||
| ...(this.apiKey ? { Authorization: `Bearer ${this.apiKey}` } : {}), | ||
| }; | ||
|
|
||
| logger.debug('Request headers:', requestHeaders); | ||
|
|
There was a problem hiding this comment.
Stop logging the Authorization header (API key leak).
logger.debug('Request headers:', requestHeaders); emits the raw Authorization: Bearer ${this.apiKey} value. That’s a critical security regression because it writes the BrowserOperator API key into DevTools logs. Please strip or mask the header before logging—or drop the log entirely—to avoid leaking credentials.
- logger.debug('Request headers:', requestHeaders);
+ const { Authorization, ...safeHeaders } = requestHeaders;
+ logger.debug('Request headers:', safeHeaders);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const requestHeaders = { | |
| 'Content-Type': 'application/json', | |
| 'X-Agent': selectedAgent, | |
| ...(this.apiKey ? { Authorization: `Bearer ${this.apiKey}` } : {}), | |
| }; | |
| logger.debug('Request headers:', requestHeaders); | |
| const requestHeaders = { | |
| 'Content-Type': 'application/json', | |
| 'X-Agent': selectedAgent, | |
| ...(this.apiKey ? { Authorization: `Bearer ${this.apiKey}` } : {}), | |
| }; | |
| const { Authorization, ...safeHeaders } = requestHeaders; | |
| logger.debug('Request headers:', safeHeaders); |
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/LLM/BrowserOperatorProvider.ts around lines 123 to
130, the debug log prints requestHeaders including the Authorization bearer
token which leaks the API key; remove the Authorization value from anything sent
to logs by creating a copy of requestHeaders and either deleting the
Authorization key or replacing its value with a constant mask like "REDACTED"
before passing to logger.debug, or simply stop logging headers entirely; ensure
the real requestHeaders (with Authorization) is still used for the request but
only the sanitized copy is logged.
| // Return the raw result value directly | ||
| const resultValue = result.result.value; | ||
| logger.info(`Code executed successfully, result type: ${result.result.type}`); | ||
| logger.debug(`Result preview: ${JSON.stringify(resultValue).substring(0, 200)}...`); | ||
|
|
||
| return resultValue; |
There was a problem hiding this comment.
Prevent logging from crashing on undefined or unserializable results.
When the evaluated code returns undefined (or anything that produces an unserializableValue), JSON.stringify(resultValue) yields undefined, so the subsequent .substring(...) throws. That puts us in the catch block and reports a tool failure even though the page evaluation succeeded. Please guard the preview logging (and surface unserializableValue when present) before slicing so the tool can return the actual result.
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/tools/ExecuteCodeTool.ts around lines 109 to 114,
the logging call assumes JSON.stringify(resultValue) returns a string and then
calls .substring, which will throw when resultValue is undefined or not
serializable; update the preview logging to first check for
result.result.unserializableValue and log that when present, otherwise convert
resultValue to a safe string (e.g., via JSON.stringify with try/catch or
String(resultValue)), only perform substring on a defined string, and avoid
throwing so the function can still return the actual resultValue.
| const headingMatch = line.match(/\[(\d+)\]\s+heading(?:\s+level (\d+))?:\s*(.+)/i); | ||
| if (headingMatch) { | ||
| const nodeId = headingMatch[1]; | ||
| const level = headingMatch[2] ? parseInt(headingMatch[2]) : 2; // Default to level 2 | ||
| const heading = headingMatch[3].trim(); | ||
| sectionStarts.push({ index: i, heading, nodeId, level }); | ||
| } |
There was a problem hiding this comment.
Add radix when parsing heading levels
parseInt(headingMatch[2]) is new here and ESLint flags it (radix rule). Leaving the radix implicit breaks our lint pipeline and reintroduces the old octal ambiguity on inputs like "08". Please parse with base 10 explicitly.
- const level = headingMatch[2] ? parseInt(headingMatch[2]) : 2; // Default to level 2
+ const level = headingMatch[2] ? parseInt(headingMatch[2], 10) : 2; // Default to level 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const headingMatch = line.match(/\[(\d+)\]\s+heading(?:\s+level (\d+))?:\s*(.+)/i); | |
| if (headingMatch) { | |
| const nodeId = headingMatch[1]; | |
| const level = headingMatch[2] ? parseInt(headingMatch[2]) : 2; // Default to level 2 | |
| const heading = headingMatch[3].trim(); | |
| sectionStarts.push({ index: i, heading, nodeId, level }); | |
| } | |
| const headingMatch = line.match(/\[(\d+)\]\s+heading(?:\s+level (\d+))?:\s*(.+)/i); | |
| if (headingMatch) { | |
| const nodeId = headingMatch[1]; | |
| const level = headingMatch[2] ? parseInt(headingMatch[2], 10) : 2; // Default to level 2 | |
| const heading = headingMatch[3].trim(); | |
| sectionStarts.push({ index: i, heading, nodeId, level }); | |
| } |
🧰 Tools
🪛 ESLint
[error] 997-997: Missing radix parameter.
(radix)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts around lines 994
to 1000, the code calls parseInt(headingMatch[2]) without a radix which violates
the radix ESLint rule and can misparse values like "08"; update the parsing to
use an explicit base by changing it to parseInt(headingMatch[2], 10) (or
alternatively use Number(headingMatch[2])) so the heading level is always parsed
as base-10.
| const combinedLines = [...currentChunkLines, ...sectionLines]; | ||
| const combinedText = combinedLines.join('\n'); | ||
| const combinedTokens = this.estimateTokenCount(combinedText); | ||
|
|
||
| if (combinedTokens > this.CHUNK_TOKEN_LIMIT && currentChunkLines.length > 0) { | ||
| // Create chunk from accumulated content | ||
| const chunkText = currentChunkLines.join('\n'); | ||
| chunks.push({ | ||
| id: chunkId++, | ||
| content: chunkText, | ||
| tokenCount: this.estimateTokenCount(chunkText), | ||
| sectionInfo: { | ||
| heading: sectionStarts[currentChunkStart]?.heading, | ||
| level: sectionStarts[currentChunkStart]?.level, | ||
| startNodeId: sectionStarts[currentChunkStart]?.nodeId | ||
| } | ||
| }); | ||
|
|
||
| // Start new chunk with current section | ||
| currentChunkLines = sectionLines; | ||
| currentChunkStart = i; | ||
| } else { | ||
| // Add section to current chunk | ||
| currentChunkLines.push(...sectionLines); | ||
| } | ||
| } | ||
|
|
||
| // Add final chunk if there's content | ||
| if (currentChunkLines.length > 0) { | ||
| const chunkText = currentChunkLines.join('\n'); | ||
| chunks.push({ | ||
| id: chunkId++, | ||
| content: chunkText, | ||
| tokenCount: this.estimateTokenCount(chunkText), | ||
| sectionInfo: { | ||
| heading: sectionStarts[currentChunkStart]?.heading, | ||
| level: sectionStarts[currentChunkStart]?.level, | ||
| startNodeId: sectionStarts[currentChunkStart]?.nodeId | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Chunking still emits oversized chunks
When the first section (or any section flushed into a fresh chunk) is larger than CHUNK_TOKEN_LIMIT, the current guard (combinedTokens > … && currentChunkLines.length > 0) lets it pass straight through, so we end up building chunks that are still bigger than the limit the fallback flow is supposed to avoid. That means the new “chunked” path can still hand the LLM >40k-token payloads and regress right back into context-size failures on large, single-heading DOMs.
Please split sections that individually exceed the limit before enqueueing them. One way is to flush any accumulated lines, then fall back to the token-based splitter for that section so every emitted chunk stays within CHUNK_TOKEN_LIMIT.
- if (combinedTokens > this.CHUNK_TOKEN_LIMIT && currentChunkLines.length > 0) {
- // Create chunk from accumulated content
- const chunkText = currentChunkLines.join('\n');
- chunks.push({
- id: chunkId++,
- content: chunkText,
- tokenCount: this.estimateTokenCount(chunkText),
- sectionInfo: {
- heading: sectionStarts[currentChunkStart]?.heading,
- level: sectionStarts[currentChunkStart]?.level,
- startNodeId: sectionStarts[currentChunkStart]?.nodeId
- }
- });
-
- // Start new chunk with current section
- currentChunkLines = sectionLines;
- currentChunkStart = i;
- } else {
- // Add section to current chunk
- currentChunkLines.push(...sectionLines);
- }
+ if (combinedTokens > this.CHUNK_TOKEN_LIMIT) {
+ if (currentChunkLines.length > 0) {
+ const chunkText = currentChunkLines.join('\n');
+ chunks.push({
+ id: chunkId++,
+ content: chunkText,
+ tokenCount: this.estimateTokenCount(chunkText),
+ sectionInfo: {
+ heading: sectionStarts[currentChunkStart]?.heading,
+ level: sectionStarts[currentChunkStart]?.level,
+ startNodeId: sectionStarts[currentChunkStart]?.nodeId,
+ },
+ });
+ currentChunkLines = [];
+ }
+
+ const sectionText = sectionLines.join('\n');
+ for (const tokenChunk of this.chunkByTokens(sectionText)) {
+ chunks.push({
+ id: chunkId++,
+ content: tokenChunk.content,
+ tokenCount: tokenChunk.tokenCount,
+ sectionInfo: {
+ heading: section.heading,
+ level: section.level,
+ startNodeId: section.nodeId,
+ },
+ });
+ }
+ currentChunkStart = i + 1;
+ continue;
+ }
+
+ if (currentChunkLines.length === 0) {
+ currentChunkStart = i;
+ }
+ currentChunkLines.push(...sectionLines);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts around lines 1025
to 1065, the chunking logic can emit chunks larger than CHUNK_TOKEN_LIMIT when a
single section is bigger than the limit because the guard only splits when
currentChunkLines is non-empty; fix by detecting when the sectionLines alone
exceed CHUNK_TOKEN_LIMIT and splitting that section before pushing. Concretely:
when combinedTokens > CHUNK_TOKEN_LIMIT, if currentChunkLines.length > 0 flush
the accumulated chunk as today; then if the new sectionLines themselves exceed
CHUNK_TOKEN_LIMIT, run the token-based splitter over sectionLines (using
estimateTokenCount and iterating slices of lines until each produced chunk is <=
CHUNK_TOKEN_LIMIT) and push those sub-chunks with correct sectionInfo; otherwise
start a fresh chunk with sectionLines as before. Ensure final chunk pushes
follow the same token-limit checks so no emitted chunk exceeds
CHUNK_TOKEN_LIMIT.
| let result; | ||
| if (existingFile) { | ||
| // Update existing file | ||
| result = await manager.updateFile(TODO_FILENAME, args.todoList, false); | ||
| } else { | ||
| // Create new file | ||
| result = await manager.createFile(TODO_FILENAME, args.todoList, 'text/markdown'); | ||
| } |
There was a problem hiding this comment.
Drop the unused result assignment.
result is assigned but never read, so eslint/tsc will fail this file. Please remove the temporary variable and await the file operation inline (or use the returned value).
Apply this diff:
- let result;
- if (existingFile) {
- // Update existing file
- result = await manager.updateFile(TODO_FILENAME, args.todoList, false);
- } else {
- // Create new file
- result = await manager.createFile(TODO_FILENAME, args.todoList, 'text/markdown');
- }
+ if (existingFile) {
+ await manager.updateFile(TODO_FILENAME, args.todoList, false);
+ } else {
+ await manager.createFile(TODO_FILENAME, args.todoList, 'text/markdown');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let result; | |
| if (existingFile) { | |
| // Update existing file | |
| result = await manager.updateFile(TODO_FILENAME, args.todoList, false); | |
| } else { | |
| // Create new file | |
| result = await manager.createFile(TODO_FILENAME, args.todoList, 'text/markdown'); | |
| } | |
| if (existingFile) { | |
| // Update existing file | |
| await manager.updateFile(TODO_FILENAME, args.todoList, false); | |
| } else { | |
| // Create new file | |
| await manager.createFile(TODO_FILENAME, args.todoList, 'text/markdown'); | |
| } |
🧰 Tools
🪛 ESLint
[error] 67-67: 'result' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/tools/UpdateTodoTool.ts around lines 61 to 68, the
local variable `result` is assigned but never used; remove the unused variable
and directly await the file operation instead. Replace the `result = await
manager.updateFile(...)` and `result = await manager.createFile(...)`
assignments with direct awaits (e.g., `await manager.updateFile(...)` / `await
manager.createFile(...)`) or return their promises if needed, ensuring no unused
variables remain.
| it('shows configurable agent tool error in chat when execution fails', async () => { | ||
| const view = document.createElement('devtools-chat-view') as any; | ||
| document.body.appendChild(view); | ||
| const toolCallId = 'id-err'; | ||
|
|
||
| // Simulate: agent-lane tool call (filtered from chat) + error tool result routed to chat lane | ||
| view.data = {messages: [ | ||
| makeUser('start'), | ||
| { entity: ChatMessageEntity.MODEL, action: 'tool', toolName: 'search_agent', toolCallId, isFinalAnswer: false, uiLane: 'agent' } as any, | ||
| { entity: ChatMessageEntity.TOOL_RESULT, toolName: 'search_agent', toolCallId, resultText: 'url.trim is not a function', isError: true, uiLane: 'chat' } as any, | ||
| ], state: 'idle', isTextInputEmpty: true, onSendMessage: () => {}, onPromptSelected: () => {}} as any; | ||
| await raf(); | ||
|
|
||
| const shadow = view.shadowRoot!; | ||
| const errors = shadow.querySelectorAll('.tool-result-message.error'); | ||
| assert.isAtLeast(errors.length, 1); | ||
| assert.include((errors[0].textContent || ''), 'url.trim is not a function'); | ||
| document.body.removeChild(view); | ||
| }); |
There was a problem hiding this comment.
Resolve new lint violations in added test
The freshly added test triggers rulesdir/no-document-body-mutation (direct document.body.appendChild) and multiple @typescript-eslint/no-explicit-any hits on these lines. Our test helpers already expose renderElementIntoDOM so you can create the element without mutating document.body, and you can type the element as HTMLElementTagNameMap['devtools-chat-view'] (or the exported component type) instead of casting to any. Please switch to the helper and tighten the types so the lint step passes.
🧰 Tools
🪛 ESLint
[error] 348-348: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 349-349: Avoid using methods that render DOM directly into the body. Prefer the renderElementIntoDOM helper.
(rulesdir/no-document-body-mutation)
[error] 355-355: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 356-356: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 357-357: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/__tests__/ChatViewAgentSessions.test.ts around
lines 347-365, replace the direct DOM mutation and any-casts: use the existing
test helper renderElementIntoDOM to create and attach the element (e.g. const
view = renderElementIntoDOM('devtools-chat-view') ) instead of
document.body.appendChild, type the view as
HTMLElementTagNameMap['devtools-chat-view'] (or the exported component type)
rather than using any, tighten types for the .data payload (use appropriate
message interfaces or a narrow cast only where necessary) and remove the
explicit document.body.removeChild call (call view.remove() or use the helper
cleanup if available) so the rulesdir/no-document-body-mutation and
no-explicit-any lint violations are resolved.
| static #browseroperatorMiniModelSelect: any | null = null; | ||
| static #browseroperatorNanoModelSelect: any | null = null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace the new selector fields with a concrete element type.
ESLint is now failing on Lines 529-530 (@typescript-eslint/no-explicit-any). Please type these BrowserOperator selectors the same way we do for other model selectors (e.g., HTMLElement | null or ReturnType<typeof createModelSelector> | null) so the lint step passes.
- static #browseroperatorMiniModelSelect: any | null = null;
- static #browseroperatorNanoModelSelect: any | null = null;
+ static #browseroperatorMiniModelSelect: HTMLElement | null = null;
+ static #browseroperatorNanoModelSelect: HTMLElement | null = null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static #browseroperatorMiniModelSelect: any | null = null; | |
| static #browseroperatorNanoModelSelect: any | null = null; | |
| static #browseroperatorMiniModelSelect: HTMLElement | null = null; | |
| static #browseroperatorNanoModelSelect: HTMLElement | null = null; |
🧰 Tools
🪛 ESLint
[error] 529-529: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 530-530: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/SettingsDialog.ts around lines 529-530, the two
private selector fields are typed as `any | null`, which violates
@typescript-eslint/no-explicit-any; replace `any` with the same concrete type
used by other model selector fields (for example `HTMLElement | null` or
`ReturnType<typeof createModelSelector> | null`) so the declarations match the
rest of the file and satisfy the linter; update both
`#browseroperatorMiniModelSelect` and `#browseroperatorNanoModelSelect` to that
concrete type and add any necessary imports or type references used elsewhere in
this module.
|
Claude finished @tysonthomas9's task —— View job PR Review: Add BO Provider and improvementsTodo List:
🔴 Critical Security Concerns1. ExecuteCodeTool - Major Security Risk
2. Visual Indicator Injection
🟡 Code Quality Issues3. BrowserOperatorProvider Hard-coded Endpoints
4. API Key Logging Security Issue
5. Error Handling Inconsistencies
🟠 Performance Considerations6. Large File Injection in VisualIndicatorTool
7. SchemaBasedExtractorTool Chunking Logic
8. Missing Request Debouncing
🔵 Test Coverage Issues9. No Tests for New Critical Features
🟢 Positive Aspects10. Good Logging Infrastructure
11. Well-Structured Provider Architecture
12. Proper TypeScript Usage
📋 Specific RecommendationsImmediate Actions Required:
Code Improvements:// ExecuteCodeTool.ts - Add sandboxing
const ALLOWED_APIS = ['document.querySelector', 'document.querySelectorAll', 'Array.from'];
// Validate code against allowed APIs before execution
// BrowserOperatorProvider.ts - Fix logging
const { Authorization, ...safeHeaders } = requestHeaders;
logger.debug('Request headers:', safeHeaders);
// BrowserOperatorProvider.ts - Make configurable
const DEFAULT_ENDPOINTS = [
'https://api.browseroperator.io/v1',
'https://backup.browseroperator.io/v1'
];
// VisualIndicatorTool.ts - Cache injection
if (!document.getElementById('devtools-agent-glow-style')) {
// Only inject if not already present
}Testing Strategy:
🎯 Overall AssessmentStrengths: Good architecture, comprehensive features, strong TypeScript usage The PR introduces valuable functionality but requires security hardening and comprehensive testing before it can be safely merged. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
front_end/panels/ai_chat/agent_framework/implementation/agents/ContentWriterAgent.ts (1)
28-32: Critical: Remove instructions for unavailable tools.The systemPrompt instructs the agent to "persist your outline, intermediate synthesis, and final report with 'create_file'/'update_file'", but these tools have been removed from the tools array (lines 61-64) and the IMPORTANT note (line 59) explicitly states only read_file and list_files are available. This contradiction will confuse the LLM and may cause tool invocation errors.
Apply this diff to remove references to write operations:
Use the session file workspace as your shared knowledge base: - Immediately call 'list_files' to discover research artifacts (notes, structured datasets, outstanding questions) created earlier in the session. - Read the relevant files before outlining to understand what has already been captured, current confidence levels, and any gaps that remain. - If the handoff references specific files, open them with 'read_file' and incorporate their contents, citing source filenames or URLs when appropriate. -- Persist your outline, intermediate synthesis, and final report with 'create_file'/'update_file' so future revisions or downstream agents can reuse the material.front_end/panels/ai_chat/ui/AIChatPanel.ts (1)
441-523: Fix BrowserOperator model defaults so the provider is usableRight now
updateModelOptionsleaves the BrowserOperator list empty and swaps in a “Models not loaded” placeholder. As soon as the user selects BrowserOperator,MODEL_OPTIONSonly contains that placeholder,#validateAndFixModelSelectionsfalls back to the OpenAI defaults, and we end up trying to run BrowserOperator requests with an OpenAI model id. Because the registry never registersopenaiin this flow, the next send fails withprovider is not properly initialized. Net result: the new provider can’t actually be used.Let’s populate sane defaults (
main,mini,nano) whenever we don’t have explicit BrowserOperator data so bothMODEL_OPTIONSand the storedall_model_optionsinclude valid BrowserOperator entries.- let updatedBrowserOperatorModels = existingBrowserOperatorModels; + let updatedBrowserOperatorModels = existingBrowserOperatorModels; + const DEFAULT_BROWSEROPERATOR_MODELS: ModelOption[] = [ + { value: 'main', label: 'BrowserOperator Main', type: 'browseroperator' }, + { value: 'mini', label: 'BrowserOperator Mini', type: 'browseroperator' }, + { value: 'nano', label: 'BrowserOperator Nano', type: 'browseroperator' }, + ]; … - if (providerModels.length > 0) { + if (providerModels.length > 0) { const firstModelType = providerModels[0].type; … - } else if (firstModelType === 'browseroperator') { - updatedBrowserOperatorModels = providerModels; + } else if (firstModelType === 'browseroperator') { + updatedBrowserOperatorModels = providerModels; } } + + if (updatedBrowserOperatorModels.length === 0) { + updatedBrowserOperatorModels = DEFAULT_BROWSEROPERATOR_MODELS; + } … - } else if (selectedProvider === 'browseroperator') { - MODEL_OPTIONS = updatedBrowserOperatorModels; - - // Add placeholder if no BrowserOperator models available - if (MODEL_OPTIONS.length === 0) { - MODEL_OPTIONS.push({ - value: MODEL_PLACEHOLDERS.NO_MODELS, - label: 'BrowserOperator: Models not loaded', - type: 'browseroperator' as const - }); - } + } else if (selectedProvider === 'browseroperator') { + MODEL_OPTIONS = updatedBrowserOperatorModels; }This keeps the new provider aligned with its own model ids and allows runs to proceed without tripping the provider/model mismatch.
♻️ Duplicate comments (2)
front_end/panels/ai_chat/agent_framework/AgentRunner.ts (2)
18-18: Import order violation persists.The value import of
FileStorageManagerstill appears after the type-only import fromTools.js, violating ESLint's import/order rule as previously flagged.
752-752: Object shorthand violation remains unfixed.The object literal still uses
agentName: agentNameinstead of the shorthandagentName, as previously flagged.
🧹 Nitpick comments (7)
front_end/panels/ai_chat/evaluation/test-cases/research-agent-tests.ts (1)
42-42: LGTM! Clear criterion for final output quality.The addition of this criterion requiring a comprehensive research summary with proper citations aligns well with the PR's focus on improving final result formatting. The criterion is specific and testable by the LLM judge.
Optional: Consider adding similar criteria to other test cases for consistency.
While this criterion is excellent for
basicResearchTest, other test cases use vaguer phrasing for final outputs (e.g., "Data is properly organized for content writer handoff" on line 78, "Research data is well-organized" on line 114). Consider adding an explicit "final answer with proper citations" criterion to other test cases if comprehensive summaries with citations are a standard requirement across all research tests.front_end/panels/ai_chat/tools/Tools.ts (3)
1342-1344: Improve validation error message clarity.The error message "Either position, pages, or direction must be provided" is slightly ambiguous. Consider rewording for clarity.
- return { error: 'Either position, pages, or direction must be provided' }; + return { error: 'At least one of position, pages, or direction must be provided' };
1430-1442: Schema guidance is helpful but potentially opinionated.The schema includes usage recommendations ("This is the recommended way to scroll", "Use pages parameter instead") which are helpful but might be too prescriptive for schema descriptions. Consider whether these belong in documentation instead.
This is not a blocker—the guidance does help LLMs make better choices—but consider moving detailed usage recommendations to tool documentation or examples if the schema becomes too verbose.
2146-2182: Consider refactoring repetitive verification guidance.The visual verification prompts now heavily emphasize objective tree diff metrics, which appears to address false negatives. However, the guidance is repeated verbatim across three different verification scenarios (with before/after screenshots, after-only screenshot, and DOM-based).
Recommendations:
- Extract shared prompt sections: Consider extracting the "OBJECTIVE PAGE CHANGE EVIDENCE" and "IMPORTANT VERIFICATION RULES" into a helper function to reduce duplication
- Hardcoded threshold: Line 2171 mentions "100+ modified elements" as a success indicator—this is arbitrary and might not apply to all actions (e.g., a simple text input might only modify 1-2 elements)
- Balance: While prioritizing tree diff is valuable, the prompts now strongly discourage visual analysis. Ensure legitimate visual issues aren't ignored.
Example refactoring:
private getVerificationRulesPrompt(treeDiff: TreeDiffResult | null): string { return `OBJECTIVE PAGE CHANGE EVIDENCE: ${treeDiff ? `- Tree Changes Detected: ${treeDiff.hasChanges ? 'YES' : 'NO'} - Change Summary: ${treeDiff.summary} ...` : 'Tree diff not available'} IMPORTANT VERIFICATION RULES: 1. If Tree Changes Detected = YES with significant modifications, the action likely succeeded 2. Trust objective tree change metrics as primary evidence 3. Visual analysis provides secondary context 4. For navigation: Changed root node IDs indicate successful page navigation`; }Then reuse this across all three verification branches.
Also applies to: 2227-2267, 2313-2353
front_end/panels/ai_chat/ui/ChatView.ts (3)
50-146: Example prompts configuration is well-structured.The
ExamplePromptConfiginterface andEXAMPLE_PROMPTSconstant provide a clean, centralized way to manage example prompts with per-provider model preferences. The structure is self-documenting and extensible.Optional: If you'd like to satisfy ESLint's
consistent-indexed-object-stylerule, you can replace the index signature with aRecordtype:- modelPreferences?: { - [provider: string]: { - main?: string; - mini?: string; - nano?: string; - }; - }; + modelPreferences?: Record<string, { + main?: string; + mini?: string; + nano?: string; + }>;
955-1008: Consider reducing log verbosity for production.The model-switching logic is sound and properly handles provider-specific preferences, but the extensive logging (lines 959-1007) may be too verbose for production. Consider downgrading some
logger.infocalls tologger.debugfor debugging-level detail.Example:
- logger.info('=== EXAMPLE PROMPT CLICKED ==='); - logger.info('Display text:', promptConfig.displayText); - logger.info('Prompt text:', promptText); - logger.info('Agent type:', promptConfig.agentType || 'default (none)'); + logger.debug('Example prompt clicked:', { + displayText: promptConfig.displayText, + promptText, + agentType: promptConfig.agentType || 'default' + });
1037-1087: Example suggestions logic is sound.The method correctly branches based on page context (Chrome internal pages, search sites, general pages) and uses the new
EXAMPLE_PROMPTSconfiguration.Optional style fix: Add curly braces to the one-liner at line 1085 to satisfy ESLint's
curlyrule:- [...specific, ...generic].forEach(item => { if (!map.has(item.displayText)) map.set(item.displayText, item); }); + [...specific, ...generic].forEach(item => { + if (!map.has(item.displayText)) { + map.set(item.displayText, item); + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
config/gni/devtools_grd_files.gni(3 hunks)front_end/panels/ai_chat/BUILD.gn(6 hunks)front_end/panels/ai_chat/agent_framework/AgentRunner.ts(13 hunks)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts(6 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts(2 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/ContentWriterAgent.ts(1 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/ResearchAgent.ts(3 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/SearchAgent.ts(2 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/WebTaskAgent.ts(2 hunks)front_end/panels/ai_chat/core/AgentService.ts(12 hunks)front_end/panels/ai_chat/core/Version.ts(1 hunks)front_end/panels/ai_chat/evaluation/test-cases/research-agent-tests.ts(1 hunks)front_end/panels/ai_chat/tools/Tools.ts(14 hunks)front_end/panels/ai_chat/ui/AIChatPanel.ts(28 hunks)front_end/panels/ai_chat/ui/ChatView.ts(12 hunks)front_end/panels/ai_chat/ui/FileContentViewer.ts(1 hunks)front_end/panels/ai_chat/ui/FileListDisplay.ts(1 hunks)front_end/panels/ai_chat/ui/TodoListDisplay.ts(1 hunks)front_end/panels/ai_chat/ui/chatView.css(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- front_end/panels/ai_chat/BUILD.gn
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-24T14:13:09.842Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-09-24T14:13:09.842Z
Learning: Applies to eval-server/nodejs/src/config.js : Require OPENAI_API_KEY and PORT (default 8080 if unspecified); optionally support GROQ_API_KEY, OPENROUTER_API_KEY, LITELLM_ENDPOINT, LITELLM_API_KEY, DEFAULT_PROVIDER, DEFAULT_MAIN_MODEL, DEFAULT_MINI_MODEL, DEFAULT_NANO_MODEL
Applied to files:
front_end/panels/ai_chat/core/AgentService.tsfront_end/panels/ai_chat/ui/AIChatPanel.ts
📚 Learning: 2025-09-24T14:13:09.842Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-09-24T14:13:09.842Z
Learning: Applies to eval-server/nodejs/src/evaluator.js : LLM evaluator must integrate with OpenAI for LLM-as-a-judge functionality
Applied to files:
front_end/panels/ai_chat/ui/AIChatPanel.ts
📚 Learning: 2025-09-24T14:13:09.842Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-09-24T14:13:09.842Z
Learning: Applies to eval-server/nodejs/src/rpc-client.js : RPC client must support the configure_llm method for dynamic LLM provider configuration with the documented parameter structure
Applied to files:
front_end/panels/ai_chat/ui/AIChatPanel.ts
📚 Learning: 2025-09-24T14:13:09.842Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: eval-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-09-24T14:13:09.842Z
Learning: Applies to eval-server/nodejs/src/server.js : src/server.js must accept WebSocket connections from LLM agents, manage agent lifecycle (connect, ready, disconnect), orchestrate evaluation sessions, and handle bidirectional RPC
Applied to files:
front_end/panels/ai_chat/ui/AIChatPanel.ts
🧬 Code graph analysis (10)
front_end/panels/ai_chat/ui/TodoListDisplay.ts (4)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/ui/ChatView.ts (2)
Lit(928-952)Lit(1284-1293)front_end/panels/ai_chat/ui/FileListDisplay.ts (3)
localStorage(104-108)file(110-139)file(147-164)front_end/panels/ai_chat/tools/FileStorageManager.ts (1)
FileStorageManager(44-281)
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts (3)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)
CallCtx(20-33)ToolRegistry(219-266)front_end/panels/ai_chat/ui/ChatView.ts (1)
target(1022-1034)
front_end/panels/ai_chat/ui/FileListDisplay.ts (3)
front_end/panels/ai_chat/core/AgentService.ts (3)
logger(378-389)AgentService(47-1000)event(867-936)front_end/panels/ai_chat/tools/FileStorageManager.ts (2)
FileSummary(28-34)FileStorageManager(44-281)front_end/panels/ai_chat/ui/FileContentViewer.ts (1)
FileContentViewer(21-862)
front_end/panels/ai_chat/core/AgentService.ts (4)
front_end/panels/ai_chat/agent_framework/AgentSessionTypes.ts (1)
AgentSession(11-41)front_end/panels/ai_chat/agent_framework/AgentRunnerEventBus.ts (1)
AgentRunnerEventBus(16-31)front_end/panels/ai_chat/tools/VisualIndicatorTool.ts (1)
VisualIndicatorManager(29-683)front_end/panels/ai_chat/ui/AIChatPanel.ts (5)
apiKey(1201-1232)apiKey(1274-1293)apiKey(1609-1643)provider(1488-1554)localStorage(832-855)
front_end/panels/ai_chat/ui/FileContentViewer.ts (4)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/ui/FileListDisplay.ts (2)
file(110-139)file(147-164)front_end/panels/ai_chat/tools/FileStorageManager.ts (1)
FileSummary(28-34)front_end/panels/ai_chat/tools/Tools.ts (1)
RenderWebAppTool(3568-3568)
front_end/panels/ai_chat/agent_framework/AgentRunner.ts (3)
front_end/panels/ai_chat/tools/Tools.ts (1)
Tool(43-52)front_end/panels/ai_chat/tools/FileStorageManager.ts (1)
FileStorageManager(44-281)front_end/panels/ai_chat/core/PageInfoManager.ts (1)
enhancePromptWithPageContext(184-262)
front_end/panels/ai_chat/ui/AIChatPanel.ts (6)
front_end/panels/ai_chat/ui/input/InputBar.ts (1)
currentProvider(39-39)front_end/panels/ai_chat/ui/EvaluationDialog.ts (1)
currentProvider(231-286)front_end/panels/ai_chat/LLM/LLMTypes.ts (1)
ModelOption(91-95)front_end/ui/legacy/Toolbar.ts (1)
ToolbarMenuButton(970-1096)front_end/panels/ai_chat/LLM/LLMClient.ts (1)
LLMClient(51-428)front_end/panels/ai_chat/LLM/BrowserOperatorProvider.ts (1)
BrowserOperatorProvider(25-428)
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)
front_end/panels/ai_chat/agent_framework/AgentSessionTypes.ts (1)
AgentSession(11-41)front_end/panels/ai_chat/ui/AIChatPanel.ts (9)
provider(1488-1554)apiKey(1201-1232)apiKey(1274-1293)apiKey(1609-1643)error(1956-1969)logger(967-1060)logger(1368-1438)logger(1591-1604)logger(1648-1651)
front_end/panels/ai_chat/ui/ChatView.ts (1)
front_end/panels/ai_chat/ui/AIChatPanel.ts (8)
provider(1488-1554)modelPreferences(2125-2206)data(1560-1571)data(2068-2104)logger(967-1060)logger(1368-1438)logger(1591-1604)logger(1648-1651)
front_end/panels/ai_chat/agent_framework/implementation/agents/ResearchAgent.ts (5)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)
ConfigurableAgentResult(291-331)CallCtx(20-33)front_end/panels/ai_chat/agent_framework/AgentSessionTypes.ts (1)
AgentSession(11-41)front_end/panels/ai_chat/tools/FileStorageManager.ts (1)
FileStorageManager(44-281)front_end/panels/ai_chat/tools/FetcherTool.ts (1)
FetcherToolResult(33-37)
🪛 ast-grep (0.39.7)
front_end/panels/ai_chat/ui/FileContentViewer.ts
[warning] 176-176: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(<${tag}[^>]*>.*?</${tag}>, 'gis')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 179-179: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(<${tag}[^>]*/>, 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 182-182: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(<${tag}[^>]*>, 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 203-203: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s+${handler}\\s*=\\s*["'][^"']*["'], 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 205-205: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(\\s+${handler}\\s*=\\s*[^\\s>]+, 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Biome (2.1.2)
front_end/panels/ai_chat/core/AgentService.ts
[error] 904-905: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 ESLint
front_end/panels/ai_chat/ui/TodoListDisplay.ts
[error] 7-7: ../core/Logger.js import should occur before import of ../tools/FileStorageManager.js
(import/order)
[error] 25-307: A custom element definition was found in a folder that should not contain element definitions. If you want to define a custom element, either place it in ui/components/ or in a components sub-folder of a panel. E.g. panels/elements/components/.
(rulesdir/enforce-custom-element-definitions-location)
[error] 89-89: Lit render calls should be inside of a view function
(rulesdir/no-lit-render-outside-of-view)
[error] 97-97: Lit render calls should be inside of a view function
(rulesdir/no-lit-render-outside-of-view)
[error] 101-305: Lit render calls should be inside of a view function
(rulesdir/no-lit-render-outside-of-view)
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts
[error] 1-1: Type imports must be imported in the same import statement as values, using the type keyword
(rulesdir/inline-type-imports)
[error] 3-3: ../../../models/ChatTypes.js type import should occur before type import of ../../ConfigurableAgentTool.js
(import/order)
[error] 3-3: Type imports must be imported in the same import statement as values, using the type keyword
(rulesdir/inline-type-imports)
[error] 4-4: ../../../models/ChatTypes.js import should occur before type import of ../../ConfigurableAgentTool.js
(import/order)
[error] 5-5: There should be at least one empty line between import groups
(import/order)
[error] 5-5: ../../../core/Constants.js import should occur before type import of ../../ConfigurableAgentTool.js
(import/order)
[error] 6-6: There should be at least one empty line between import groups
(import/order)
[error] 7-7: ../../../core/Logger.js import should occur before type import of ../../ConfigurableAgentTool.js
(import/order)
[error] 8-8: ../../../../../core/sdk/sdk.js import should occur before type import of ../../ConfigurableAgentTool.js
(import/order)
[error] 171-171: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
front_end/panels/ai_chat/ui/FileListDisplay.ts
[error] 8-8: ../core/Logger.js import should occur before import of ../tools/FileStorageManager.js
(import/order)
[error] 9-9: ../core/AgentService.js import should occur before import of ../tools/FileStorageManager.js
(import/order)
[error] 18-638: A custom element definition was found in a folder that should not contain element definitions. If you want to define a custom element, either place it in ui/components/ or in a components sub-folder of a panel. E.g. panels/elements/components/.
(rulesdir/enforce-custom-element-definitions-location)
[error] 156-156: Prefer template literals over imperative DOM API calls
(rulesdir/no-imperative-dom-api)
[error] 209-209: Lit render calls should be inside of a view function
(rulesdir/no-lit-render-outside-of-view)
[error] 213-629: Lit render calls should be inside of a view function
(rulesdir/no-lit-render-outside-of-view)
[error] 616-616: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
front_end/panels/ai_chat/core/AgentService.ts
[error] 26-26: ../tools/VisualIndicatorTool.js import should occur before import of ./Graph.js
(import/order)
[error] 904-905: Unexpected lexical declaration in case block.
(no-case-declarations)
front_end/panels/ai_chat/ui/FileContentViewer.ts
[error] 7-7: ../../../third_party/marked/marked.js import should occur before import of ../core/Logger.js
(import/order)
[error] 42-42: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 135-135: Expected { after 'if' condition.
(curly)
[error] 136-136: Expected { after 'if' condition.
(curly)
[error] 785-785: 'content' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
front_end/panels/ai_chat/agent_framework/AgentRunner.ts
[error] 18-18: ../tools/FileStorageManager.js import should occur before type import of ../tools/Tools.js
(import/order)
[error] 82-82: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 82-82: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 752-752: Expected property shorthand.
(object-shorthand)
front_end/panels/ai_chat/ui/AIChatPanel.ts
[error] 18-18: ../LLM/BrowserOperatorProvider.js import should occur before import of ../LLM/LLMClient.js
(import/order)
[error] 84-84: Import in body of module; reorder to top.
(import/first)
[error] 731-736: Prefer template literals over imperative DOM API calls
(rulesdir/no-imperative-dom-api)
[error] 743-748: Prefer template literals over imperative DOM API calls
(rulesdir/no-imperative-dom-api)
[error] 755-760: Prefer template literals over imperative DOM API calls
(rulesdir/no-imperative-dom-api)
[error] 769-774: Prefer template literals over imperative DOM API calls
(rulesdir/no-imperative-dom-api)
[error] 2018-2018: Unexpected parentheses around single function argument.
(@stylistic/arrow-parens)
[error] 2125-2125: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 2125-2125: Expected a comma.
(@stylistic/member-delimiter-style)
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts
[error] 198-198: There must be no hyphen before @param description.
(jsdoc/require-hyphen-before-param-description)
[error] 208-208: There must be no hyphen before @param description.
(jsdoc/require-hyphen-before-param-description)
[error] 209-209: There must be no hyphen before @param description.
(jsdoc/require-hyphen-before-param-description)
[error] 210-210: There must be no hyphen before @param description.
(jsdoc/require-hyphen-before-param-description)
[error] 569-569: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
front_end/panels/ai_chat/ui/ChatView.ts
[error] 54-60: A record is preferred over an index signature.
(@typescript-eslint/consistent-indexed-object-style)
[error] 56-56: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 57-57: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 58-58: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 59-59: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 174-174: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 174-174: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 191-191: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 191-191: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 965-965: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 966-966: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 971-971: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 972-972: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 973-973: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 1085-1085: Expected { after 'if' condition.
(curly)
front_end/panels/ai_chat/agent_framework/implementation/agents/ResearchAgent.ts
[error] 1-1: ../../ConfigurableAgentTool.js type import should occur after type import of ../../../tools/FetcherTool.js
(import/order)
[error] 2-2: ../../../models/ChatTypes.js type import should occur after import of ../../../core/Logger.js
(import/order)
[error] 2-2: Type imports must be imported in the same import statement as values, using the type keyword
(rulesdir/inline-type-imports)
[error] 3-3: ../../AgentSessionTypes.js type import should occur after type import of ../../../tools/FetcherTool.js
(import/order)
[error] 4-4: ../../../models/ChatTypes.js import should occur after import of ../../../core/Logger.js
(import/order)
[error] 5-5: There should be at least one empty line between import groups
(import/order)
[error] 6-6: There should be at least one empty line between import groups
(import/order)
[error] 6-6: ./AgentVersion.js import should occur after type import of ../../../tools/FetcherTool.js
(import/order)
[error] 8-8: ../../../tools/FileStorageManager.js import should occur after type import of ../../../tools/FetcherTool.js
(import/order)
[error] 162-162: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 162-162: Expected a comma.
(@stylistic/member-delimiter-style)
[error] 162-162: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 193-193: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 219-223: Unexpected if as the only statement in an else block.
(no-lonely-if)
[error] 225-225: Unexpected any. Specify a different type.
(@typescript-eslint/no-explicit-any)
[error] 242-242: 'domain' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 243-243: 'path' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 265-265: 'error' is defined but never used.
(@typescript-eslint/no-unused-vars)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: claude-review
🔇 Additional comments (13)
front_end/panels/ai_chat/agent_framework/implementation/agents/ContentWriterAgent.ts (2)
57-60: Good clarification, but resolve the contradiction first.The explicit IMPORTANT note clearly communicates the tool restrictions and aligns with the tools array. This will help prevent the LLM from attempting to use unavailable tools. However, this conflicts with the earlier instructions on lines 28-32 that reference create_file and update_file.
Once the contradiction flagged in the previous comment is resolved, this addition will effectively prevent tool invocation errors.
65-65: Verify maxIterations is sufficient for lengthy report generation.The agent is tasked with generating detailed 5-10 page reports (1000+ words), but maxIterations is set to 3. Depending on the complexity of the research data and the report structure, this may be insufficient for the agent to iterate on outlines, incorporate feedback from tool results, and produce a comprehensive final report.
Please confirm that 3 iterations is adequate for this agent's task, or consider increasing it to 5-7 to allow for more thorough iteration cycles (analyze → outline → draft → revise).
front_end/panels/ai_chat/core/Version.ts (1)
6-6: LGTM! Version bump is appropriate.The version bump from 0.4.0 to 0.5.0 aligns well with the scope of changes in this PR, which includes a new BrowserOperator provider, additional tools, and UI components.
front_end/panels/ai_chat/tools/Tools.ts (3)
200-202: LGTM! Useful metadata for scroll operations.The new optional fields provide valuable context about the scroll operation, enabling better scroll state tracking.
2970-2973: LGTM! BrowserOperator provider API key bypass.The logic correctly exempts the BrowserOperator provider from API key requirements, aligning with the PR's goal of adding provider-aware API key gating.
3574-3577: ****The exports are valid and properly integrated into the codebase. ExecuteCodeTool is a Tool implementation that is intentionally registered separately through ToolRegistry.registerToolFactory() in ConfiguredAgents.ts (line 60), not through getTools(). VisualIndicatorManager is a utility class (not a Tool implementation) that manages visual feedback—exporting it as a utility is appropriate and doesn't require registration in getTools().
Likely an incorrect or invalid review comment.
front_end/panels/ai_chat/agent_framework/AgentRunner.ts (4)
77-101: Well-designed cleanup helper with appropriate guards.The
clearTodoListhelper correctly limits cleanup to agents with theupdate_todotool, preventing unintended state clearing. The defensive error handling (debug logging without throwing) ensures cleanup failures don't disrupt execution flow.
635-656: Todo context injection is well-guarded and fails gracefully.The todo list is injected into the system prompt only when the agent has the
update_todotool, with appropriate fallback messaging when todos don't exist and defensive error handling when file reads fail.
57-58: Session lifecycle enhancements follow a consistent pattern.The
afterExecutehook extension and session_completed events are properly integrated across all termination paths (abort, error, final_answer, max_iterations). The error handling aroundafterExecuteinvocations correctly logs failures without blocking execution, and todo cleanup is consistently applied at each exit point.Also applies to: 439-439, 588-620, 839-869, 1310-1354, 1391-1421, 1464-1474, 1488-1519
1310-1322: Conditional summary generation is properly implemented.The summary appending logic correctly respects the
includeSummaryInAnswerconfiguration flag and uses clear formatting with a markdown separator.front_end/panels/ai_chat/ui/ChatView.ts (3)
171-174: Props and state extensions are clean.The additions of
currentProviderandonExamplePromptModelSwitchfollow existing patterns and are properly wired through the component's props and private fields.Also applies to: 191-191, 206-206, 388-388, 396-396
1121-1129: Model selector short-circuit for BrowserOperator provider is correct.The logic appropriately hides the model selector when the
browseroperatorprovider is active, aligning with the provider's managed model approach.
917-918: UI components and provider state properly integrated.The
<ai-todo-list>and<ai-file-list-display>components are correctly placed in the expanded view, andcurrentProvideris properly propagated to the input bar.Also applies to: 1162-1162
| const iterationInfo = ` | ||
| ## Current Progress | ||
| - You are currently on step ${iteration + 1} of ${maxIterations} maximum steps. | ||
| - You are currently on step ${iteration + 1} of ${maxIterations - 1} maximum steps. |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the progress denominator calculation.
The progress prompt now displays ${iteration + 1} of ${maxIterations - 1}, but this appears incorrect. If maxIterations is 10, the agent can execute 10 iterations (indexed 0-9), so the display should show "step 1 of 10" rather than "step 1 of 9".
Apply this diff if the calculation is indeed incorrect:
-- You are currently on step ${iteration + 1} of ${maxIterations - 1} maximum steps.
+- You are currently on step ${iteration + 1} of ${maxIterations} maximum steps.🏁 Script executed:
#!/bin/bash
# Examine the AgentRunner.ts file around line 632 and the surrounding context
sed -n '600,665p' front_end/panels/ai_chat/agent_framework/AgentRunner.ts | cat -n
# Find where maxIterations is defined/used in this file
echo "=== maxIterations references ==="
rg -n 'maxIterations' front_end/panels/ai_chat/agent_framework/AgentRunner.ts -B2 -A2
# Find where iteration is defined/used
echo "=== iteration references ==="
rg -n 'iteration' front_end/panels/ai_chat/agent_framework/AgentRunner.ts | head -30Length of output: 10446
🏁 Script executed:
#!/bin/bash
# Look for the loop structure to understand iteration semantics
echo "=== Looking for the main loop structure ==="
rg -n -B5 -A5 'iteration.*maxIterations|for.*iteration|while.*iteration' front_end/panels/ai_chat/agent_framework/AgentRunner.ts
# Look for any test files that may clarify expected behavior
echo "=== Looking for related test files ==="
fd -e test.ts -e spec.ts | xargs grep -l 'AgentRunner\|maxIterations' 2>/dev/null | head -5Length of output: 3627
Fix the off-by-one error in iteration progress display.
The progress message at line 632 incorrectly shows ${maxIterations - 1} as the denominator. Since the loop runs from 0 to maxIterations-1 (executing exactly maxIterations iterations), the display should show the full count. Line 627's logging correctly uses ${maxIterations}, making this inconsistent. For example, when maxIterations=10, the message should read "step 10 of 10", not "step 10 of 9".
-- You are currently on step ${iteration + 1} of ${maxIterations - 1} maximum steps.
+- You are currently on step ${iteration + 1} of ${maxIterations} maximum steps.🤖 Prompt for AI Agents
In front_end/panels/ai_chat/agent_framework/AgentRunner.ts around line 632, the
progress string uses `${maxIterations - 1}` causing an off-by-one in the
displayed denominator; change the message to use `${maxIterations}` so the
displayed progress matches the actual loop count (e.g., "You are currently on
step ${iteration + 1} of ${maxIterations} maximum steps."), keeping it
consistent with the log at line 627.
| // Create LLMContext from CallCtx for tool execution | ||
| const llmContext = { | ||
| apiKey: callCtx.apiKey, | ||
| provider: callCtx.provider!, | ||
| model: callCtx.model || callCtx.mainModel || '', | ||
| getVisionCapability: callCtx.getVisionCapability, | ||
| miniModel: callCtx.miniModel, | ||
| nanoModel: callCtx.nanoModel, | ||
| abortSignal: callCtx.abortSignal | ||
| }; | ||
| await navigateTool.execute({ | ||
| url: 'https://google.com', | ||
| reasoning: 'Auto-navigation from chrome:// URL to enable action agent functionality' | ||
| }, llmContext); |
There was a problem hiding this comment.
Guard against missing provider instead of asserting
callCtx.provider! is tripping our lint (@typescript-eslint/no-non-null-assertion) and will surface as a build blocker. More importantly, CallCtx.provider is optional—handoff contexts that haven’t resolved a provider yet will hit this path and we’ll pass undefined into the tool. Please drop the non‑null assertion and bail early (or log/skip the auto-navigation) when the provider isn’t present before calling navigate_url.execute.
- const llmContext = {
+ if (!callCtx.provider) {
+ logger.warn('No provider in CallCtx; skipping auto-navigation away from chrome://');
+ return;
+ }
+
+ const llmContext = {
apiKey: callCtx.apiKey,
- provider: callCtx.provider!,
+ provider: callCtx.provider,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Create LLMContext from CallCtx for tool execution | |
| const llmContext = { | |
| apiKey: callCtx.apiKey, | |
| provider: callCtx.provider!, | |
| model: callCtx.model || callCtx.mainModel || '', | |
| getVisionCapability: callCtx.getVisionCapability, | |
| miniModel: callCtx.miniModel, | |
| nanoModel: callCtx.nanoModel, | |
| abortSignal: callCtx.abortSignal | |
| }; | |
| await navigateTool.execute({ | |
| url: 'https://google.com', | |
| reasoning: 'Auto-navigation from chrome:// URL to enable action agent functionality' | |
| }, llmContext); | |
| // Create LLMContext from CallCtx for tool execution | |
| if (!callCtx.provider) { | |
| logger.warn('No provider in CallCtx; skipping auto-navigation away from chrome://'); | |
| return; | |
| } | |
| const llmContext = { | |
| apiKey: callCtx.apiKey, | |
| provider: callCtx.provider, | |
| model: callCtx.model || callCtx.mainModel || '', | |
| getVisionCapability: callCtx.getVisionCapability, | |
| miniModel: callCtx.miniModel, | |
| nanoModel: callCtx.nanoModel, | |
| abortSignal: callCtx.abortSignal | |
| }; | |
| await navigateTool.execute({ | |
| url: 'https://google.com', | |
| reasoning: 'Auto-navigation from chrome:// URL to enable action agent functionality' | |
| }, llmContext); |
🧰 Tools
🪛 ESLint
[error] 171-171: Forbidden non-null assertion.
(@typescript-eslint/no-non-null-assertion)
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts
around lines 168 to 181, remove the non-null assertion on callCtx.provider and
guard the code path so we don’t call navigateTool.execute when provider is
absent: check if callCtx.provider is present and if not either return/skip the
auto-navigation (or log a debug/warn) before constructing llmContext; only build
llmContext and call navigateTool.execute when provider exists, using
callCtx.provider (no !) and preserving the existing fallback for model/other
fields.
| version: '0.4.0', | ||
| buildDate: '2025-10-14', | ||
| version: '0.5.0', | ||
| buildDate: '2025-10-05', |
There was a problem hiding this comment.
Build date is moving backwards in time.
The build date changed from '2025-10-14' to '2025-10-05', which moves backwards by 9 days. Build dates should always move forward or remain the same. This likely indicates a typo or merge conflict that needs correction.
Apply this diff to set the build date to the current date:
- buildDate: '2025-10-05',
+ buildDate: '2025-11-04',Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/core/Version.ts around line 7, the buildDate was
incorrectly reverted from '2025-10-14' to '2025-10-05'; update the buildDate to
the current date '2025-11-06' to ensure build dates never move backwards and
reflect the latest build.
| viewportHeight: window.innerHeight, | ||
| scrollHeight: document.documentElement.scrollHeight, | ||
| scrolledPages: ${pages !== undefined ? pages : 0} |
There was a problem hiding this comment.
scrolledPages reports requested value, not actual scroll distance.
The scrolledPages field always returns the input pages parameter value, regardless of whether the scroll completed successfully. If the page is already at the bottom or can't scroll the full distance, this will report a misleading value.
Consider either:
- Renaming to
requestedPagesto clarify it's the input, not the outcome - Computing actual scroll distance:
Math.round((window.pageYOffset - initialY) / viewportHeight)
- scrolledPages: ${pages !== undefined ? pages : 0}
+ scrolledPages: ${pages !== undefined ? `Math.round((window.pageYOffset - (window.pageYOffset - ${pages} * window.innerHeight)) / window.innerHeight)` : 0}Or rename the field:
- scrolledPages?: number; // Number of pages scrolled (if using pages parameter)
+ requestedPages?: number; // Number of pages requested (if using pages parameter)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/tools/Tools.ts around lines 1399-1401, the
scrolledPages field is currently returning the input pages value (misleading if
the scroll didn't complete); replace it with an actual measured value by
capturing the initial vertical offset before scrolling and after the scroll
completes compute scrolledPages = Math.round((window.pageYOffset - initialY) /
window.innerHeight), clamp to >= 0 and to a sensible max (e.g., total scrollable
pages) so the returned value reflects the real scrolled distance rather than the
requested pages.
| return ` | ||
| <div class="file-viewer" data-file-type="${fileType}"> | ||
| <!-- Header --> | ||
| <header class="viewer-header"> | ||
| <div class="file-info"> | ||
| <span class="file-icon">${icon}</span> | ||
| <div class="file-details"> | ||
| <h1 class="file-name">${file.fileName}</h1> | ||
| <div class="file-meta"> | ||
| <span class="meta-item">${typeLabel}</span> | ||
| <span class="meta-separator">•</span> | ||
| <span class="meta-item">${size}</span> | ||
| <span class="meta-separator">•</span> | ||
| <span class="meta-item">Created ${created}</span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| <div class="header-actions"> | ||
| <button class="action-btn copy-btn" onclick="copyContent(event)"> | ||
| <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"> | ||
| <rect x="9" y="9" width="13" height="13" rx="2" ry="2"></rect> | ||
| <path d="M5 15H4a2 2 0 0 1-2-2V4a2 2 0 0 1 2-2h9a2 2 0 0 1 2 2v1"></path> | ||
| </svg> | ||
| <span class="btn-text">Copy</span> | ||
| </button> | ||
| <button class="action-btn download-btn" onclick="downloadFile(event)"> | ||
| <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"> | ||
| <path d="M21 15v4a2 2 0 0 1-2 2H5a2 2 0 0 1-2-2v-4"></path> | ||
| <polyline points="7 10 12 15 17 10"></polyline> | ||
| <line x1="12" y1="15" x2="12" y2="3"></line> | ||
| </svg> | ||
| <span class="btn-text">Download</span> | ||
| </button> | ||
| </div> | ||
| </header> | ||
| <!-- Content --> | ||
| <main class="content-container" id="content-main"> | ||
| ${contentHTML} | ||
| </main> | ||
| </div> | ||
| `; |
There was a problem hiding this comment.
Escape user-controlled file name before embedding in viewer HTML
We interpolate file.fileName straight into the HTML string that RenderWebAppTool renders. A file created via CreateFileTool (or any future tool) can set its name to something like <img src=x onerror=alert(1)>.md; opening the viewer would then run arbitrary script in DevTools. We already have escapeHTML, so please run the file name through it (and reuse the escaped value wherever it enters the HTML) before returning the template.
@@
- const icon = FileContentViewer.getFileIcon(fileType);
+ const icon = FileContentViewer.getFileIcon(fileType);
+ const safeFileName = FileContentViewer.escapeHTML(file.fileName);
@@
- <h1 class="file-name">${file.fileName}</h1>
+ <h1 class="file-name">${safeFileName}</h1>
@@
- <div class="modal-title">${this.#viewingFile.fileName}</div>
+ <div class="modal-title">${safeFileName}</div>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/FileContentViewer.ts around lines 268-309, the
template injects file.fileName directly into the returned HTML which allows XSS;
call the existing escapeHTML(file.fileName) once into a local const (e.g.
escapedName) and replace every occurrence of file.fileName in the template with
that escaped value before returning the string, ensuring you reuse the escaped
value wherever the filename is embedded in the HTML.
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude Code is working… I'll analyze this and get back to you. |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation