More tool improvements and addition of eval CLI #90
Conversation
|
Claude finished @tysonthomas9's task —— View job PR Review: Tool improvements and eval CLI ✅Reviewing this large PR with 96 changed files and significant architectural improvements. Todo List:
📋 Overall Assessment: EXCELLENT ⭐This is a substantial and well-engineered improvement to the Browser Operator core. The architectural changes are thoughtful, the implementation is clean, and the testing is comprehensive. ✅ Major Strengths1. CDP Adapter Architecture (
|
📝 WalkthroughWalkthroughThis PR introduces a comprehensive multi-frame accessibility framework with CDP abstraction, shadow DOM piercing, EncodedId-based element identification, and a complete evaluation infrastructure for browser automation agents. Key additions include frame-aware accessibility snapshots, cross-browser adapter abstraction, DOM traversal with shadow DOM support, action/search caching, and an eval runner with agent orchestration and LLM-based evaluation. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Action Agent
participant FrameReg as FrameRegistry
participant HybridSnap as HybridSnapshot
participant DOM as DOM Resolver
participant Piercer as ShadowPiercer
participant Cache as ActionCache
participant Tool as Perform Action
Agent->>FrameReg: collectFrames()
FrameReg->>FrameReg: DFS traverse, assign ordinals
FrameReg-->>Agent: FrameInfo[]
Agent->>HybridSnap: captureHybridSnapshot(target)
HybridSnap->>Piercer: ensurePiercerInjected()
Piercer-->>HybridSnap: piercer ready
HybridSnap->>DOM: buildDomMaps(frame)
DOM->>Piercer: traverse shadow roots
HybridSnap->>HybridSnap: buildA11yOutline(frame)
HybridSnap-->>Agent: HybridSnapshot {xpathMap, urlMap}
Agent->>Cache: get(semantic_intent)
Cache-->>Agent: cached pattern or null
alt Cache Hit
Agent->>Tool: performAction(cachedEncodedId)
Tool-->>Agent: success
else Cache Miss
Agent->>DOM: resolveElement(selector)
DOM-->>Agent: EncodedId
Agent->>Tool: performAction(encodedId)
Tool-->>Agent: success
Agent->>Cache: save(encodedId, pattern)
end
sequenceDiagram
participant CLI as Eval CLI
participant Runner as TestRunner
participant Browser as BrowserExecutor
participant Agent as AgentBridge
participant Judge as LLMJudge
participant Braintrust as BraintrustTracker
CLI->>Runner: init()
Runner->>Browser: launch()
Runner->>Agent: init()
Runner->>Judge: init()
Runner->>Braintrust: init(config)
CLI->>Runner: runTests(testCases)
loop For each test
Runner->>Browser: createContext()
Runner->>Browser: navigateTo(testUrl)
Runner->>Agent: execute(testCase, context)
Agent->>Agent: setupCDPAdapter
Agent-->>Runner: AgentResult
Runner->>Judge: evaluate(testCase, result, screenshots)
Judge->>Judge: buildPrompt with criteria
Judge->>Judge: callLLM
Judge-->>Runner: EvaluationResult
Runner->>Braintrust: logResult(testCase, result)
Runner->>Browser: closeContext(context)
end
Runner->>Braintrust: finalize(summary)
Runner-->>CLI: RunSummary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Rationale: This PR introduces substantial architectural changes with 40+ new files and significant modifications to existing modules. The complexity stems from: (1) interdependent components (CDP adapters used by DOM resolvers, used by tools), (2) new design patterns (EncodedId system, frame ordinals, adapter abstraction), (3) cross-environment support requiring conditional logic, (4) multiple public API surfaces with careful type definitions, and (5) diverse concerns spanning accessibility, DOM manipulation, caching, and evaluation infrastructure. However, individual modules follow consistent patterns, tests are comprehensive, and the codebase is well-organized by concern. Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Reviewed commit: 3ece01dd66
ℹ️ 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".
scripts/eval-runner/cli.ts
Outdated
| apiKey: getProviderConfig(opts.provider as LLMProvider, opts.apiKey).apiKey, | ||
| judgeApiKey: getProviderConfig(opts.judgeProvider as LLMProvider, opts.apiKey).apiKey, |
There was a problem hiding this comment.
The judge API key is derived with getProviderConfig(opts.judgeProvider, opts.apiKey), which reuses the agent --api-key even when the judge provider differs. If a user runs --provider cerebras --api-key <cerebras> with the default OpenAI judge, this overwrites OPENAI_API_KEY and the judge will fail to authenticate, causing all LLM-judge evaluations to error out. Consider adding a separate --judge-api-key or passing undefined so the judge uses its provider-specific env var.
Useful? React with 👍 / 👎.
scripts/eval-runner/LLMJudge.ts
Outdated
| content.push({ | ||
| type: 'image_url', | ||
| image_url: { url: `data:image/png;base64,${imageData}` }, | ||
| }); |
There was a problem hiding this comment.
The judge message builder always adds screenshots using the OpenAI image_url schema. When judgeProvider is anthropic and visual verification is enabled, this payload format is invalid for the Anthropic SDK and will cause the evaluation request to fail or silently drop images. Gate image formatting by provider (Anthropic expects {type: "image", source: {type: "base64", media_type: "image/png", data: ...}}) to keep anthropic-based judging working with screenshots.
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: 5
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
front_end/panels/ai_chat/tools/HTMLToMarkdownTool.ts (1)
95-123: Bug: API key retrieval from AgentService will always fail.Line 97 checks
AgentServicebeforeensureBrowserDeps()is called (line 117). SinceAgentServiceis initialized insideensureBrowserDeps(), it will always benullat line 97, making the fallback toAgentService.getInstance().getApiKey()unreachable.🔎 Proposed fix - move ensureBrowserDeps before API key retrieval
async execute(args: HTMLToMarkdownArgs, ctx?: LLMContext): Promise<HTMLToMarkdownResult> { logger.info('Executing with args', { args }); const { instruction } = args; const READINESS_TIMEOUT_MS = 15000; // 15 seconds timeout for page readiness + // Ensure browser dependencies are loaded first + if (!(await ensureBrowserDeps()) || !SDK) { + return { + success: false, + markdownContent: null, + error: 'SDK not available (Node.js environment)' + }; + } + // Get API key from context first, fallback to AgentService in browser let apiKey = ctx?.apiKey; - if (!apiKey && !isNodeEnvironment && AgentService) { + if (!apiKey && AgentService) { apiKey = AgentService.getInstance().getApiKey() ?? undefined; } // ... rest of validation ... try { // *** Add wait for page load *** - if (!(await ensureBrowserDeps()) || !SDK) { - return { - success: false, - markdownContent: null, - error: 'SDK not available (Node.js environment)' - }; - } const target = SDK.TargetManager.TargetManager.instance().primaryPageTarget();front_end/panels/ai_chat/evaluation/test-cases/action-agent-tests.ts (1)
1277-1279: Inconsistency between objective and criteria insearchFilterTest.The objective says to type "Anna" but the criteria on line 1278 says "Typed 'a' in the filter field". These should match.
🔎 Proposed fix
llmJudge: { criteria: [ 'Located the search filter input', - 'Typed "a" in the filter field', + 'Typed "Anna" in the filter field', 'List items filtered to show only matching results',front_end/panels/ai_chat/agent_framework/AgentRunner.ts (1)
285-305: PropagatecdpAdapterinto handoff target agents so nested runs work in eval-runner
AgentRunnerConfignow supports acdpAdapter, and regular tool execution threads it into the tool context:toolResultData = await toolToExecute.execute(toolArgs as any, ({ // ... cdpAdapter: config.cdpAdapter, } as any));However, the handoff path does not currently propagate this adapter:
executeHandoffaccepts many parameters but nocdpAdapter.targetRunnerConfigomitscdpAdapter, so the target agent run seesconfig.cdpAdapter === undefined.- Both the LLM-triggered handoff (
handoff_to_*tools) and themax_iterationshandoff callexecuteHandoffwithout passing an adapter.In environments where browser access is only available via the CDP adapter (e.g., the eval-runner / Node path), any CDP-dependent tools used by the handoff target agent will lose their adapter and may fail or silently fall back.
Consider threading
cdpAdapterthroughexecuteHandoffand intotargetRunnerConfig. For example:Suggested change
- private static async executeHandoff( + private static async executeHandoff( currentMessages: ChatMessage[], originalArgs: ConfigurableAgentArgs, handoffConfig: HandoffConfig, executingAgent: ConfigurableAgentTool, @@ miniModel?: string, // Mini model for smaller/faster operations nanoModel?: string, // Nano model for smallest/fastest operations - overrides?: { sessionId?: string; parentSessionId?: string; traceId?: string }, - onBeforeToolExecution?: (toolName: string, toolArgs: unknown) => Promise<void> + cdpAdapter?: AgentRunnerConfig['cdpAdapter'], + overrides?: { sessionId?: string; parentSessionId?: string; traceId?: string }, + onBeforeToolExecution?: (toolName: string, toolArgs: unknown) => Promise<void> ): Promise<ConfigurableAgentResult & { agentSession: AgentSession }> { @@ - const targetRunnerConfig: AgentRunnerConfig = { + const targetRunnerConfig: AgentRunnerConfig = { apiKey, modelName: resolvedModelName, systemPrompt: enhancedSystemPrompt, @@ getVisionCapability: defaultGetVisionCapability, miniModel, nanoModel, + cdpAdapter, onBeforeToolExecution, };Then update both call sites:
// LLM 'handoff_to_*' tool: - const handoffResult = await AgentRunner.executeHandoff( + const handoffResult = await AgentRunner.executeHandoff( messages, toolArgs as ConfigurableAgentArgs, handoffConfig, executingAgent!, apiKey, modelName, maxIterations, temperature ?? 0, createSuccessResult, createErrorResult, toolArgs as ConfigurableAgentArgs, currentSession, config.provider, config.getVisionCapability, config.miniModel, config.nanoModel, - { sessionId: nestedSessionId, parentSessionId: currentSession.sessionId, traceId: getCurrentTracingContext()?.traceId }, - config.onBeforeToolExecution + config.cdpAdapter, + { sessionId: nestedSessionId, parentSessionId: currentSession.sessionId, traceId: getCurrentTracingContext()?.traceId }, + config.onBeforeToolExecution ); // max_iterations handoff: - const handoffResult = await AgentRunner.executeHandoff( + const handoffResult = await AgentRunner.executeHandoff( messages, args, maxIterHandoffConfig, executingAgent, apiKey, modelName, maxIterations, temperature ?? 0, createSuccessResult, createErrorResult, undefined, currentSession, config.provider, config.getVisionCapability, config.miniModel, - config.nanoModel, - undefined, - config.onBeforeToolExecution + config.nanoModel, + config.cdpAdapter, + undefined, + config.onBeforeToolExecution );This keeps CDP access available for nested agents in the eval-runner path while preserving existing DevTools behavior.
Also applies to: 389-405, 1047-1062, 1490-1505
front_end/panels/ai_chat/tools/Tools.ts (1)
583-715:waitForPageLoadis actively used by multiple tools and doesn't need removal.The function is called by
HTMLToMarkdownToolandReadabilityExtractorTool. However, there is an inconsistency in the codebase: some tools (e.g.,NavigateURLTool) use the adapter pattern viawaitForPageLoadViaAdapter, while others (e.g.,HTMLToMarkdownTool,ReadabilityExtractorTool) use the direct SDK approach. Consider updatingHTMLToMarkdownToolandReadabilityExtractorToolto use the adapter pattern for consistency across the codebase.
🟡 Minor comments (16)
front_end/panels/ai_chat/core/PageInfoManager.ts-82-101 (1)
82-101: Potential race condition in initialization.Setting
this.initialized = truesynchronously at line 84 before the async initialization completes could cause issues. IfensureInitialized()is called concurrently from multiple places, subsequent calls will return immediately while the first call's async setup is still in progress, potentially leading to operations attempting to use SDK before it's ready.Also, the SDK null check at line 104 is redundant since it's already checked at line 98.
🔎 Proposed fix using a Promise-based initialization guard
export class PageInfoManager { private static instance: PageInfoManager; private currentInfo: { url: string, title: string } | null = null; private accessibilityTree: string | null = null; private iframeContent: Array<{ role: string, name?: string, contentSimplified?: string }> | null = null; private listeners = new Set<(info: { url: string, title: string } | null) => void>(); - private initialized = false; + private initializationPromise: Promise<void> | null = null; // ... private async ensureInitialized(): Promise<void> { - if (this.initialized) return; - this.initialized = true; + if (this.initializationPromise) { + return this.initializationPromise; + } + this.initializationPromise = this.doInitialize(); + return this.initializationPromise; + } + + private async doInitialize(): Promise<void> { // Skip browser initialization in Node environment if (isNodeEnvironment) { logger.debug('PageInfoManager running in Node environment - SDK features disabled'); return; } // ... rest of initialization logic - if (!browserDepsAvailable || !SDK) { + if (!browserDepsAvailable || !SDK) { logger.debug('Browser dependencies not available'); return; } // Set up navigation event listeners - if (!SDK) { - logger.warn('SDK not loaded, skipping target observation'); - return; - } SDK.TargetManager.TargetManager.instance().observeTargets({Committable suggestion skipped: line range outside the PR's diff.
front_end/panels/ai_chat/common/__tests__/utils-universal.test.ts-204-216 (1)
204-216: Test comment may not match actual behavior.The comment on line 213 says "Empty name should still show colon with empty value", but the implementation in
formatSimplifiedTreeusesnode.name ?: ${node.name}: '', which treats empty strings as falsy (no colon). The test assertions only verify'img'is present, not whether the colon appears or not.Consider either updating the comment to reflect actual behavior, or adding an explicit assertion:
// If empty string should NOT show colon (current behavior): assert.notInclude(result, ':'); // Or if this is a bug and empty string should show colon, update the implementationfront_end/panels/ai_chat/tools/CritiqueTool.ts-9-26 (1)
9-26: Lazy-loaded AgentService is defined but never used.The
ensureAgentService()function andAgentServicevariable are defined but never called or used in this file. Theexecute()method doesn't referenceAgentServiceat all.If this was added for future use, consider removing it until needed to avoid dead code. If it was intended to be used (e.g., to get API key or state), the implementation appears incomplete.
#!/bin/bash # Verify that AgentService and ensureAgentService are not used in this file rg -n "AgentService|ensureAgentService" front_end/panels/ai_chat/tools/CritiqueTool.tsfront_end/panels/ai_chat/dom/__tests__/ComposedTreeResolver.test.ts-215-218 (1)
215-218: Potential test bug:namefield includes predicate.This test case sets
name: 'iframe[123]', but based onparseXPathToStepsimplementation, thenamefield should have predicates stripped (e.g.,'iframe'). Withname: 'iframe[123]', theisIframeStepcheck against/^iframe$/ishould returnfalse, nottrue.If this test is intended to verify that
isIframeStephandles malformed names, the assertion should beassert.isFalse. Otherwise, thenameshould be'iframe'.🔎 Suggested fix
it('should return true for iframe[123]', () => { - const step = {axis: 'child' as const, raw: 'iframe[123]', name: 'iframe[123]'}; + const step = {axis: 'child' as const, raw: 'iframe[123]', name: 'iframe'}; assert.isTrue(isIframeStep(step)); });front_end/panels/ai_chat/testing/fixtures/shadow-dom-test.html-263-269 (1)
263-269: Potential XSS via innerHTML in test fixture.Line 268 uses
innerHTMLwith unsanitized user input from the email field:result.innerHTML = `<span class="success">Form submitted with email: ${email}</span>`;While this is a test fixture, it sets a bad example. If the email field contains
<script>or event handlers, they could execute.🔎 Suggested fix using textContent
- result.innerHTML = `<span class="success">Form submitted with email: ${email}</span>`; + result.textContent = `Form submitted with email: ${email}`;Or if styling is needed:
result.innerHTML = '<span class="success"></span>'; result.querySelector('.success').textContent = `Form submitted with email: ${email}`;scripts/dom-cdp-tests.ts-1185-1195 (1)
1185-1195: Hardcodedheadless: falsewill cause CI failures.The script is intended for both local and CI use (per comment), but
headless: falseis hardcoded. Consider using an environment variable.🔎 Suggested fix
const browser: Browser = await puppeteer.launch({ - headless: false, // Set to true for CI + headless: process.env.CI === 'true' || process.env.HEADLESS === 'true', executablePath: chromePath,front_end/panels/ai_chat/cdp/FrameRegistryUniversal.ts-121-126 (1)
121-126:getOrdinalreturns 0 for non-existent frames, same as main frame.Returning
0as the default (line 125) when a frame doesn't exist is ambiguous since0is the valid ordinal for the main frame. Consider returning-1or usinghasFrame()check before calling.🔎 Suggested alternatives
Option 1: Return -1 for not found:
getOrdinal(frameId: string): number { - return this.frames.get(frameId)?.ordinal ?? 0; + return this.frames.get(frameId)?.ordinal ?? -1; }Option 2: Document the behavior:
/** * Get the ordinal for a frame (0 for main frame). * Returns 0 if frame is not found - use hasFrame() to check existence first. */front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts-196-206 (1)
196-206: Non-null assertion onprovidermay cause runtime issues.Line 199 uses
callCtx.provider!with a non-null assertion, butprovidercould potentially be undefined based on theCallCtxtype. Ifprovideris undefined, tool execution may fail unexpectedly.🔎 Suggested defensive handling
const llmContext = { apiKey: callCtx.apiKey, - provider: callCtx.provider!, + provider: callCtx.provider ?? 'anthropic', // or appropriate default model: callCtx.model || callCtx.mainModel || '',Alternatively, add a guard before creating the context:
if (!callCtx.provider) { logger.warn('No provider available for auto-navigation'); return; }scripts/eval-runner/LLMJudge.ts-186-203 (1)
186-203: Missing error handling for Anthropic response structure.Line 193 assumes
response.content[0].textexists, but Anthropic responses can have different content block types. Add defensive checks.🔎 Proposed fix
if (this.config.provider === 'anthropic') { const response = await this.client.messages.create({ model: this.config.model, max_tokens: 2000, messages, }); - return response.content[0].text; + const textBlock = response.content.find((block: { type: string }) => block.type === 'text'); + if (!textBlock || !('text' in textBlock)) { + throw new Error('Anthropic response did not contain text content'); + } + return textBlock.text; }front_end/panels/ai_chat/a11y/HybridSnapshotUniversal.ts-597-604 (1)
597-604: Per-frameoutlineandurlMapare not fully populated.Non-main frames get empty
outlinestrings (line 601), and all frames get emptyurlMapobjects (line 603). This differs fromHybridSnapshot.tswhich populates these per-frame. This could cause issues for consumers expecting per-frame data.🔎 Analysis
The combined data (
combinedTree,combinedUrlMap) appears correct, but theperFramearray has incomplete data:
outline: Only main frame has contenturlMap: Always empty for all framesIf
perFrameis only for debugging (per the interface comment), this may be acceptable, but it's inconsistent withHybridSnapshot.ts.front_end/panels/ai_chat/a11y/HybridSnapshot.ts-369-372 (1)
369-372: Potential issue: no guard for empty frames array.If
frameRegistry.listAllFrameIds()returns an empty array,frames[0]will beundefined, andabsPrefix.set(mainFrameId, '')will set an entry with anundefinedkey, which could cause subtle bugs.🔎 Suggested fix
const frames = frameRegistry.listAllFrameIds(); - const parentByFrame = frameRegistry.getParentMap(); - - // Output maps - const combinedXpathMap: Record<EncodedId, string> = {}; - const combinedUrlMap: Record<EncodedId, string> = {}; - const perFrame: FrameSnapshot[] = []; - - // Absolute XPath prefix for each frame (empty for main frame) - const absPrefix = new Map<string, string>(); const mainFrameId = frames[0]; - if (mainFrameId) { - absPrefix.set(mainFrameId, ''); + if (!mainFrameId) { + return {combinedTree: '', combinedXpathMap: {}, combinedUrlMap: {}, perFrame: []}; } + + const parentByFrame = frameRegistry.getParentMap(); + // ... rest of initialization + const absPrefix = new Map<string, string>(); + absPrefix.set(mainFrameId, '');Committable suggestion skipped: line range outside the PR's diff.
scripts/eval-runner/AgentBridge.ts-260-262 (1)
260-262:toolCall.resulttruthiness check may incorrectly mark valid results as failed.If
toolCall.resultis0,'',false, ornull(valid return values), the checktoolCall.result ? 'success' : 'failed'will incorrectly mark them as failed.🔎 Suggested fix
actions.push({ action: toolCall.toolName || 'unknown', target: toolCall.toolArgs?.nodeId ? `nodeId: ${toolCall.toolArgs.nodeId}` : toolCall.toolArgs?.xpath ? `xpath: ${toolCall.toolArgs.xpath}` : undefined, - result: toolCall.result ? 'success' : 'failed', + result: toolCall.error ? 'failed' : 'success', timestamp: Date.now(), });front_end/panels/ai_chat/dom/EnhancedElementResolver.ts-213-218 (1)
213-218: InconsistentusedShadowPiercersemantics.Here
usedShadowPierceris set to theensurePiercerparameter value, meaning it reflects whether piercer injection was requested, not whether the piercer was actually installed or used. This is inconsistent withresolveWithPiercerEnhancedwhich checksstats?.installed.Consider using consistent logic across all resolution functions.
front_end/panels/ai_chat/common/utils-universal.ts-1010-1058 (1)
1010-1058: Multiple functions have unusedexecutionContextIdparameters.The functions
selectOption,checkElement,uncheckElement, andsetCheckedStateall declare anexecutionContextIdparameter that is never used. Either use these parameters for proper iframe context handling or remove them to avoid confusion.Also applies to: 1079-1101, 1103-1125, 1127-1153
front_end/panels/ai_chat/dom/EnhancedElementResolver.ts-239-242 (1)
239-242:optionsnot passed toresolveByEncodedId.The
optionsparameter (containingensurePiercer) is not passed toresolveByEncodedId, unlike the other resolution paths. This means piercer injection behavior is inconsistent for EncodedId selectors.🔎 Proposed fix
// Check for EncodedId format (e.g., "0-123") if (/^\d+-\d+$/.test(trimmed)) { - return resolveByEncodedId(target, trimmed as EncodedId); + return resolveByEncodedId(target, trimmed as EncodedId, options); }This also requires updating
resolveByEncodedIdsignature to accept options.Committable suggestion skipped: line range outside the PR's diff.
front_end/panels/ai_chat/common/utils-universal.ts-924-973 (1)
924-973: UnusedexecutionContextIdparameter.The
executionContextIdparameter is declared but never used in the function body. This could cause issues when trying to fill elements in iframe contexts where the execution context matters.🔎 Proposed fix - either use it or remove it
If the parameter should be used:
await runtimeAgent.invoke('callFunctionOn', { objectId, + executionContextId, functionDeclaration: ` function() { this.focus();If it's not needed, remove the parameter:
async function fillElement( runtimeAgent: ReturnType<CDPSessionAdapter['runtimeAgent']>, inputAgent: ReturnType<CDPSessionAdapter['inputAgent']>, objectId: string, args: unknown[], - executionContextId?: number, ): Promise<void> {
| const frameRegistry = new FrameRegistry(target); | ||
| await frameRegistry.collectFrames(); | ||
|
|
||
| const frameInfo = frameRegistry.getFrameByOrdinal(frameOrdinal); | ||
| if (!frameInfo) { | ||
| return { | ||
| found: false, | ||
| usedShadowPiercer: false, | ||
| error: `Frame with ordinal ${frameOrdinal} not found`, | ||
| }; | ||
| } | ||
|
|
||
| // Resolve the node by backend ID | ||
| const domAgent = target.domAgent(); | ||
| const resolveResponse = await domAgent.invoke_resolveNode({ | ||
| backendNodeId: backendNodeId as Protocol.DOM.BackendNodeId, | ||
| }); | ||
|
|
||
| if (!resolveResponse.object?.objectId) { | ||
| return { | ||
| found: false, | ||
| usedShadowPiercer: false, | ||
| error: `Could not resolve backend node ${backendNodeId} in frame ${frameInfo.frameId}`, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Frame context not used during node resolution.
The function retrieves frameInfo to validate the frame exists, but the subsequent domAgent.invoke_resolveNode call doesn't use the frame context. For nodes in iframes, this may fail to resolve correctly since resolveNode without an executionContextId resolves in the main frame context.
Consider passing the frame's execution context:
🔎 Proposed fix
// Resolve the node by backend ID
const domAgent = target.domAgent();
+ // For iframe nodes, we may need to resolve with frame context
const resolveResponse = await domAgent.invoke_resolveNode({
backendNodeId: backendNodeId as Protocol.DOM.BackendNodeId,
+ // Note: executionContextId may be needed for cross-frame resolution
});Additionally, creating a new FrameRegistry and calling collectFrames() on every resolution is potentially expensive. Consider caching or accepting a pre-built registry as a parameter.
📝 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 frameRegistry = new FrameRegistry(target); | |
| await frameRegistry.collectFrames(); | |
| const frameInfo = frameRegistry.getFrameByOrdinal(frameOrdinal); | |
| if (!frameInfo) { | |
| return { | |
| found: false, | |
| usedShadowPiercer: false, | |
| error: `Frame with ordinal ${frameOrdinal} not found`, | |
| }; | |
| } | |
| // Resolve the node by backend ID | |
| const domAgent = target.domAgent(); | |
| const resolveResponse = await domAgent.invoke_resolveNode({ | |
| backendNodeId: backendNodeId as Protocol.DOM.BackendNodeId, | |
| }); | |
| if (!resolveResponse.object?.objectId) { | |
| return { | |
| found: false, | |
| usedShadowPiercer: false, | |
| error: `Could not resolve backend node ${backendNodeId} in frame ${frameInfo.frameId}`, | |
| }; | |
| } | |
| const frameRegistry = new FrameRegistry(target); | |
| await frameRegistry.collectFrames(); | |
| const frameInfo = frameRegistry.getFrameByOrdinal(frameOrdinal); | |
| if (!frameInfo) { | |
| return { | |
| found: false, | |
| usedShadowPiercer: false, | |
| error: `Frame with ordinal ${frameOrdinal} not found`, | |
| }; | |
| } | |
| // Resolve the node by backend ID | |
| const domAgent = target.domAgent(); | |
| // For iframe nodes, we need to resolve with frame context | |
| const resolveResponse = await domAgent.invoke_resolveNode({ | |
| backendNodeId: backendNodeId as Protocol.DOM.BackendNodeId, | |
| executionContextId: frameInfo.executionContextId, | |
| }); | |
| if (!resolveResponse.object?.objectId) { | |
| return { | |
| found: false, | |
| usedShadowPiercer: false, | |
| error: `Could not resolve backend node ${backendNodeId} in frame ${frameInfo.frameId}`, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/dom/EnhancedElementResolver.ts around lines 117-141,
the code builds a new FrameRegistry and resolves a backend node without using
the frame's execution context, which can fail for iframe nodes; change the
resolve call to include the frame's executionContextId from frameInfo (pass
executionContextId to domAgent.invoke_resolveNode) so resolution happens in the
correct frame context, and refactor the function signature or implementation to
accept a pre-built FrameRegistry or reuse a cached registry instead of creating
and collecting frames on every call to avoid the expensive per-call
FrameRegistry creation.
front_end/panels/ai_chat/evaluation/test-cases/action-agent-shadow-dom-tests.ts
Outdated
Show resolved
Hide resolved
| /** | ||
| * Test interacting with checkboxes on the-internet.herokuapp.com. | ||
| */ | ||
| export const shadowToggleTest: TestCase<ActionAgentArgs> = { | ||
| id: 'action-agent-shadow-007', | ||
| name: 'Toggle checkbox', | ||
| description: 'Toggle a checkbox to change its state', | ||
| url: 'https://the-internet.herokuapp.com/checkboxes', | ||
| tool: 'action_agent', | ||
| input: { | ||
| objective: 'Click checkbox 1 to toggle its checked state', | ||
| reasoning: 'Testing interactive checkbox toggle capability', | ||
| hint: 'There are two checkboxes on the page. Click the first one to toggle it.', | ||
| }, | ||
| validation: { | ||
| type: 'llm-judge', | ||
| llmJudge: { | ||
| criteria: [ | ||
| 'Located the first checkbox element', | ||
| 'Successfully clicked the checkbox', | ||
| 'Checkbox state changed (checked->unchecked or unchecked->checked)', | ||
| 'Visual feedback reflects the new state', | ||
| ], | ||
| visualVerification: { | ||
| enabled: true, | ||
| captureBeforeAction: true, | ||
| captureAfterAction: true, | ||
| verificationPrompts: [ | ||
| 'Compare before/after to confirm checkbox state changed', | ||
| 'Verify the checkbox visual state is different', | ||
| ], | ||
| }, | ||
| }, | ||
| }, | ||
| metadata: { | ||
| tags: ['action', 'checkbox', 'toggle', 'interactive', 'the-internet'], | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Test misplaced in shadow DOM suite.
Similar to shadowFormFillTest, this test (shadowToggleTest) doesn't involve shadow DOM. It tests basic checkbox interaction on the-internet.herokuapp.com and lacks the 'shadow-dom' tag in metadata. This should be moved to a general action test suite.
🤖 Prompt for AI Agents
In
front_end/panels/ai_chat/evaluation/test-cases/action-agent-shadow-dom-tests.ts
around lines 263 to 300, the shadowToggleTest is incorrectly placed in the
shadow DOM suite even though it does not interact with shadow DOM; move this
test to the general action test suite file (e.g., action-agent-tests.ts or
equivalent), remove it from this shadow-dom file, and update its metadata tags
if necessary (remove any 'shadow-dom' tag or ensure it has tags like 'action',
'checkbox', 'toggle'); also update any exports/imports to reflect the new file
location and run tests to ensure references are updated.
|
Claude finished @tysonthomas9's task —— View job PR Review: Tool improvements and eval CLI ✅Reviewing this large PR with 96 changed files and significant architectural improvements. Todo List:
📋 Overall Assessment: EXCELLENT ⭐This is a substantial and well-engineered improvement to the Browser Operator core. The architectural changes are thoughtful, the implementation is clean, and the testing is comprehensive. ✅ Major Strengths1. CDP Adapter Architecture (
|
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
front_end/panels/ai_chat/common/utils.ts (2)
934-946: Fix XPath injection/quoting bug by usingJSON.stringify(xpath)in Runtime.evaluate.Line 937 interpolates
xpathinto a JS string literal; any"/\breaks evaluation and (depending on source) enables injection into the evaluated snippet. UseJSON.stringify(xpath)like you already do elsewhere (Line 832).Proposed fix
- let evaluateResult = await runtimeAgent.invoke_evaluate({ - expression: ` - (function() { - const result = document.evaluate("${xpath}", document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null); - return result.singleNodeValue; - })() - `, + let evaluateResult = await runtimeAgent.invoke_evaluate({ + expression: ` + (function() { + const result = document.evaluate( + ${JSON.stringify(xpath)}, + document, + null, + XPathResult.FIRST_ORDERED_NODE_TYPE, + null + ); + return result.singleNodeValue; + })() + `, returnByValue: false });Also applies to: 828-839
1392-1393: Fix logger interpolation: these should be template literals.Lines 1392 and 1431 use single quotes, so
${...}won’t interpolate.Proposed fix
- logger.info('Found ${visibleBackendIds.size} visible backendNodeIds:', Array.from(visibleBackendIds)); + logger.info(`Found ${visibleBackendIds.size} visible backendNodeIds:`, Array.from(visibleBackendIds)); - logger.info('Found ${relevantNodeIds.size} relevant nodeIds (visible + ancestors):', Array.from(relevantNodeIds)); + logger.info(`Found ${relevantNodeIds.size} relevant nodeIds (visible + ancestors):`, Array.from(relevantNodeIds));Also applies to: 1431-1432
front_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.ts (1)
171-199: Avoid URL-mapping ID format mismatches: preferUtilsUniversal.getAccessibilityTree(adapter).idToUrl.Right now
treeTextcomes fromUtilsUniversal.getAccessibilityTree(adapter)buturlMappingsare built from rawAccessibility.getFullAXTreenodeIds. If those nodeId formats differ (e.g., frame-aware EncodedIds vs raw AX ids), URL resolution will silently fail and the LLM retry loop won’t help.Proposed fix (single source of truth)
- private async getAccessibilityData(adapter: CDPSessionAdapter): Promise<{urlMappings: Record<string, string>, treeText: string}> { - // Get raw accessibility tree nodes to build URL mapping - const accessibilityAgent = adapter.accessibilityAgent(); - const rawAxTree = await accessibilityAgent.invoke<{nodes: Protocol.Accessibility.AXNode[]}>('getFullAXTree', {}); - - // Build URL mapping from raw accessibility nodes - const urlMappings = this.buildUrlMapping(rawAxTree?.nodes || []); - logger.debug(`Built URL mapping with ${Object.keys(urlMappings).length} entries`); - - // Get the processed accessibility tree text - const processedTreeResult = await UtilsUniversal.getAccessibilityTree(adapter); - - return { - treeText: processedTreeResult.simplified, - urlMappings - }; - } + private async getAccessibilityData(adapter: CDPSessionAdapter): Promise<{urlMappings: Record<string, string>, treeText: string}> { + const processedTreeResult = await UtilsUniversal.getAccessibilityTree(adapter); + const urlMappings = processedTreeResult.idToUrl ?? {}; + logger.debug(`Using UtilsUniversal idToUrl mapping with ${Object.keys(urlMappings).length} entries`); + return { + treeText: processedTreeResult.simplified, + urlMappings, + }; + }Also applies to: 183-228
🤖 Fix all issues with AI agents
In @front_end/panels/ai_chat/common/utils.ts:
- Around line 1083-1152: The drag branch should validate args[0] is an object
with numeric offsetX/offsetY or toX/toY and normalize values before using them,
and must wrap the mouse sequence in a try/finally so that after calling
inputAgent.invoke_dispatchMouseEvent with type MousePressed you always call a
MouseReleased regardless of errors; update the code in the method === 'drag'
block to (1) check typeof args[0] === 'object' and coerce/validate
dragArgs.offsetX/offsetY/toX/toY to numbers (throw a clear error if invalid),
(2) perform the MousePressed call, then run the move loop and any awaits inside
a try block, and (3) put the final inputAgent.invoke_dispatchMouseEvent with
type MouseReleased in the finally block using the computed endX/endY so the
pointer is never left down.
In @front_end/panels/ai_chat/evaluation/test-cases/cdp-tool-tests.ts:
- Around line 752-755: The NodeIDsToURLsArgs interface is typed incorrectly:
change NodeIDsToURLsArgs.nodeIds from number[] to string[] to match the
EncodedId format expected by NodeIDsToURLsTool, and update any test fixtures in
this file that pass numeric IDs to instead pass EncodedId strings (e.g.,
"0-123", "0-456"); ensure the tool's error/message text remains consistent with
the string type.
In @front_end/panels/ai_chat/evaluation/test-cases/search-tool-tests.ts:
- Around line 7-14: SearchTool's schema enum is missing two valid
SearchStrategyType values ('xpath-llm' and 'css-llm'), and the test's
SearchToolArgs interface should mirror that set; update the enum used in the
SearchTool schema (referencing SearchTool and SearchStrategyType) to include
['xpath-schema','semantic-xpath','encoded-id','text-pattern','xpath-llm','css-llm','cdp','js-eval']
and modify the SearchToolArgs interface in the test (SearchToolArgs) so its
strategy union includes 'xpath-llm' and 'css-llm' as valid string literals.
In @front_end/panels/ai_chat/tools/action_cache/ActionPatternCapture.ts:
- Around line 432-449: The global singleton captureInstance leaks across
adapters and relies on brittle (as any).adapter access; replace it with a
WeakMap<CDPSessionAdapter, ActionPatternCapture> that stores a single
ActionPatternCapture per adapter, remove captureInstance, and update
getActionPatternCapture to lookup/create the instance in the WeakMap using the
CDPSessionAdapter key so comparisons are safe and instances are GC-able;
reference ActionPatternCapture, getActionPatternCapture, and CDPSessionAdapter
when making the change.
In @front_end/panels/ai_chat/tools/GetAccessibilityTreeToolV0.ts:
- Around line 32-52: The tool's execute implementation returns only simplified
and idToUrl but ActionAgentV0 promises xpathMap/tagNameMap/iframes; update the
execute method to include xpathMap, tagNameMap, and iframes (if present) from
UtilsUniversal.getAccessibilityTree() in the returned AccessibilityTreeResult,
or if those keys may be absent, conditionally include them (e.g.,
treeResult.xpathMap, treeResult.tagNameMap, treeResult.iframes) and ensure the
AccessibilityTreeResult type/signature is updated to accept these fields so the
agent prompt is accurate; if you prefer the other route, remove the xpath/tag
claims from ActionAgentV0 instead so the tool and prompt remain consistent.
In @front_end/panels/ai_chat/tools/SearchTool.ts:
- Around line 148-195: When a cached pattern exists but the resolved
strategyType differs (e.g., args.strategy overrides) we must avoid executing the
wrong strategy with a mismatched pattern; check pattern.strategy (or
pattern.metadata) against strategyType after computing strategyType and before
calling strategy.executePattern, and if they differ regenerate a new pattern via
strategy.generatePattern (then this.cache.savePattern) or explicitly invalidate
the cache entry; ensure executePattern always receives a pattern produced by the
same strategy instance (use strategy.generatePattern, generationResult.pattern,
and this.cache.savePattern as in the existing generation path).
In @front_end/panels/ai_chat/tools/TryCachedActionTool.ts:
- Around line 92-96: The code in TryCachedActionTool.ts incorrectly treats a
cache hit as invalid when lookup.xpathSuccess is false even though
lookup.encodedId can be valid via CSS fallback; change the conditional in the
lookup handling (the block using lookup.encodedId and lookup.xpathSuccess) to
accept any truthy lookup.encodedId as a valid cache hit regardless of
xpathSuccess, adjust the logger message to reflect "XPath failed but CSS
fallback succeeded" when xpathSuccess is false, and return { cached: true,
message: ... } (or the existing cached-hit payload) when encodedId exists
instead of forcing the full get_page_content flow.
🟡 Minor comments (12)
CLAUDE.md-61-66 (1)
61-66: Add language identifier to code block.This code block lacks a language specification. Should be a plain text block describing usage or a bash/text block if meant to show example output.
📝 Proposed fix
-``` +```text # In Claude Code, use the Task tool with eval-runner-analyst agent:CLAUDE.md-99-106 (1)
99-106: Add language identifier to code block.This directory tree structure block needs a language identifier for consistency.
📝 Proposed fix
-``` +```text front_end/CLAUDE.md-136-145 (1)
136-145: Convert bold emphasis to proper markdown heading."Agent Framework" should be a level-3 heading (
### Agent Framework) for proper markdown structure and semantic clarity.📝 Proposed fix
-**Agent Framework** +### Agent FrameworkCLAUDE.md-179-187 (1)
179-187: Add language identifier to code block.This directory structure block needs a language identifier.
📝 Proposed fix
-``` +```text scripts/eval-runner/CLAUDE.md-112-132 (1)
112-132: Add language identifier to code block and convert section header to proper heading.The AI Chat Panel structure block needs a language identifier. Additionally, the "Agent Framework" section on line 136 should use markdown heading syntax instead of bold emphasis.
📝 Proposed fixes
-``` +```text ai_chat/And on line 136:
-**Agent Framework** +### Agent Frameworkfront_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.ts-40-41 (1)
40-41: Potential memory leak: module-level Map without cleanup.The
lastActionMapstores session data but is never cleared. Over time with many sessions, this could accumulate memory. Consider adding cleanup logic when sessions complete, or using a bounded cache (e.g., LRU with size limit).💡 Suggested approach
// Option 1: Clear on session completion (add to afterExecute) lastActionMap.delete(sessionId); // Option 2: Use a bounded Map with size limit const MAX_CACHED_SESSIONS = 100; if (lastActionMap.size > MAX_CACHED_SESSIONS) { const oldestKey = lastActionMap.keys().next().value; lastActionMap.delete(oldestKey); }front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.ts-276-291 (1)
276-291: Non-null assertion on provider may cause runtime error.Line 279 uses
callCtx.provider!but the provider could be undefined. The earlier ActionAgent.ts implementation (lines 215-218) properly guards against this case.🐛 Suggested fix
const navigateTool = ToolRegistry.getRegisteredTool('navigate_url'); if (navigateTool) { + if (!callCtx.provider) { + logger.warn('Provider not available for auto-navigation, skipping'); + return; + } const llmContext = { apiKey: callCtx.apiKey, - provider: callCtx.provider!, + provider: callCtx.provider, model: callCtx.model || callCtx.mainModel || '', // ... rest of context };front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV0.ts-142-151 (1)
142-151: Use type casting to ensure type safety when accessingConfigurableAgentArgsproperties.
ConfigurableAgentArgsextendsRecord<string, unknown>, making direct interpolation ofargs.objective,args.reasoning, etc., unsafe under strict TypeScript settings. Follow the pattern inActionAgentV2.tsby casting to a specific typed interface:const v0Args = args as ActionAgentV0Args;before interpolation, or use fallback defaults likeString(args.objective ?? '').front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts-32-33 (1)
32-33: Overrideaction_agent_v1config name to avoid collision with mainaction_agent.
action_agent_v1(line 142–144) wrapscreateActionAgentConfig(), which returnsname: 'action_agent'. This creates a name collision with the mainaction_agentregistered on lines 132–137. While the registry keys differ ('action_agent_v1'vs'action_agent'), the internal config name must be distinct to prevent UI/tool identity conflicts. Override the name to'action_agent_v1'similar to how the mainaction_agentis handled on line 135.
action_agent_v0andaction_agent_v2use properly distinct names and do not require changes.front_end/panels/ai_chat/evaluation/test-cases/action-agent-tests.ts-577-644 (1)
577-644: Date picker “fill” test may be blocked by readonly/input masking—consider clarifying expected interaction path.On
https://jqueryui.com/datepicker/the input is inside an iframe; depending on implementation, direct typing may be overridden/readonly. If the intention is “open picker and click the date”, consider stating that explicitly inobjective(and/or add a hint).front_end/panels/ai_chat/a11y/HybridSnapshotUniversal.ts-690-706 (1)
690-706:combinedTreeis derived fromperFrame.outline, but only main frame outline is populated.If the intent is “single combined outline”, this is fine but
perFramecurrently looks misleading (non-main frames have empty outlines). Consider either populating per-frame outlines or documenting thatcombinedTreealready includes iframes via DOM traversal.front_end/panels/ai_chat/tools/CachedSchemaExtractorTool.ts-460-501 (1)
460-501: IIFE normalization should handle trailing;to avoid double-wrapping.A common LLM output is
(function(){ ... })();(note semicolon). Current checks can fail and wrap it again.Small robustness tweak
let code = codeBlockMatch ? codeBlockMatch[1].trim() : response.trim(); + code = code.replace(/;\s*$/, '').trim();
🧹 Nitpick comments (26)
CLAUDE.md (1)
200-205: Enhance key patterns section with frame-registry guidance.The "Key Patterns" section could benefit from explicit mention of frame-tracking using
FrameRegistry, which is a key addition in this PR for handling multi-frame scenarios and iframe resolution (as referenced in the summary's "frame-aware hybrid accessibility snapshots" feature).Consider adding a bullet point:
- Frame Registry:
FrameRegistrytracks frame context and element relationships across iframes/shadow DOM; useFrameRegistry.getFrameContext()andEncodedIdaddressing for cross-frame element resolutionThis would help developers working with the new frame-aware capabilities understand how to leverage them.
front_end/panels/ai_chat/tools/HTMLToMarkdownTool.ts (2)
22-32: Consider logging failed AgentService imports.The empty catch block silently swallows import errors, which could make debugging difficult if the module path changes or there are bundling issues.
♻️ Suggested improvement
async function ensureAgentService(): Promise<boolean> { if (isNodeEnvironment) return false; if (!agentServiceLoaded) { agentServiceLoaded = true; try { const agentServiceModule = await import('../core/AgentService.js'); AgentService = agentServiceModule.AgentService; - } catch { return false; } + } catch (e) { + logger.debug('AgentService not available (expected in Node):', e); + return false; + } } return AgentService !== null; }
87-91: Remove unused variableREADINESS_TIMEOUT_MS.The constant
READINESS_TIMEOUT_MSis declared but never used in the method, appearing to be leftover from removed page-readiness logic.♻️ Suggested fix
async execute(args: HTMLToMarkdownArgs, ctx?: LLMContext): Promise<HTMLToMarkdownResult> { logger.info('Executing with args', { args }); const { instruction } = args; - const READINESS_TIMEOUT_MS = 15000; // 15 seconds timeout for page readinessfront_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.ts (1)
233-253: Type assertions (as any) indicate missing type definitions.Multiple
as anycasts (lines 234, 249, 313, 326, 328, 453) suggest thatCallCtxand related types don't include the new fields (args,cacheHit). Consider extending the types properly.This would improve type safety and IDE support. The
CallCtxinterface should be extended to include optionalargsandcacheHitfields if they're expected to be used.front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts (1)
21-31: Same empty catch block pattern as HTMLToMarkdownTool.Consider logging failed imports for debugging purposes, similar to the suggestion for HTMLToMarkdownTool.
front_end/panels/ai_chat/tools/SearchTool.ts (1)
132-135: Consider validatingmaxResultsand using??instead of||.Today
maxResults = args.maxResults || DEFAULT_MAX_RESULTStreats0as “unset” and allows negatives/NaN through.Also applies to: 286-297
front_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.ts (1)
65-80: Consider a non-DOMException fallback for aborts (Node portability).If this runs under a Node runtime lacking
DOMException,new DOMException(...)will throw aReferenceErrorwhile handling an abort.front_end/panels/ai_chat/tools/action_cache/types.ts (1)
16-24: Constants look fine; consider making schema version a monotonic integer for simpler invalidation.Semver string works, but cache invalidation checks are usually simpler with an integer
SCHEMA_VERSION. Not required, but will likely reduce edge cases in comparison logic.front_end/panels/ai_chat/tools/action_cache/ActionPatternCapture.ts (1)
381-427: Visibility check should treat opacity numerically (not string equality).
style.opacity !== '0'(Line 412) misses'0.0', and can mis-handle inherited opacity. ConsiderparseFloat(style.opacity) > 0.front_end/panels/ai_chat/evaluation/test-cases/search-tool-tests.ts (1)
171-209: Amazon/Google tests: flakiness flags make sense; consider addingwaitForSelectorwhere available.If your harness supports
metadata.waitForSelector, it can reduce “loaded but not ready” failures on dynamic SERPs.front_end/panels/ai_chat/a11y/HybridSnapshotUniversal.ts (1)
169-172:DOM.getDocument({ depth: -1 })can be a perf footgun on large pages; consider guardrails.If this runs in an eval loop, you may want a max node limit, time budget, or a fallback to smaller depth.
front_end/panels/ai_chat/tools/CachedSchemaExtractorTool.ts (1)
225-233: ConsiderawaitPromise: trueto support selectors that return Promises (even accidentally).If you keep “no async” as a rule, this is optional—but
awaitPromise: truetends to make the system more resilient to minor LLM output drift.front_end/panels/ai_chat/cdp/FrameRegistryUniversal.ts (2)
79-119: Sequential CDP calls in visitFrame may impact performance for pages with many iframes.The
getFrameOwnercall is made sequentially for each non-main frame. For pages with many iframes, this could add latency.Consider batching these calls or collecting ownership info in parallel where CDP supports it. However, since iframe counts are typically low, this is acceptable for now.
190-197: O(n) lookup for getFrameByOwnerBackendNodeId could benefit from indexing.Unlike other lookups that have O(1) secondary indexes, this method iterates through all frames. If this is called frequently, consider adding an
ownerBackendNodeIdToFrameIdindex.♻️ Optional index addition
// Secondary indexes for O(1) lookups private ordinalToFrameId = new Map<number, string>(); private childrenByParent = new Map<string, string[]>(); + private ownerBackendNodeIdToFrame = new Map<number, FrameInfo>(); // In visitFrame, after setting ownerBackendNodeId: + if (info.ownerBackendNodeId !== undefined) { + this.ownerBackendNodeIdToFrame.set(info.ownerBackendNodeId, info); + } // Replace getFrameByOwnerBackendNodeId: getFrameByOwnerBackendNodeId(ownerBackendNodeId: number): FrameInfo | undefined { - for (const frame of this.frames.values()) { - if (frame.ownerBackendNodeId === ownerBackendNodeId) { - return frame; - } - } - return undefined; + return this.ownerBackendNodeIdToFrame.get(ownerBackendNodeId); }front_end/panels/ai_chat/common/utils-universal.ts (5)
33-38: Unused constants detected.
SCROLL_STABILIZATION_TIMEOUT_MSandPOSITION_CHECK_INTERVAL_MSare defined but not referenced elsewhere in the file. The scroll stabilization logic inline uses hardcoded values (1000ms and 50ms respectively).♻️ Use the defined constants or remove them
Either use the constants in the scroll stabilization code:
- const maxWaitTime = 1000; + const maxWaitTime = ${SCROLL_STABILIZATION_TIMEOUT_MS}; ... - setTimeout(checkPosition, 50); + setTimeout(checkPosition, ${POSITION_CHECK_INTERVAL_MS});Or remove the unused constants if they're not needed.
272-299: Sequential XPath resolution could be slow for pages with many scrollable elements.Each scrollable element's XPath is resolved individually with separate
evaluateanddescribeNodeCDP calls. For pages with many scrollable containers, this could add latency.Consider using
Promise.allfor parallel resolution if order doesn't matter.♻️ Parallelize XPath resolution
- for (const xpath of xpaths) { - if (!xpath) { - continue; - } - - const evaluateResponse = await runtimeAgent.invoke<{result?: {objectId?: string}}>('evaluate', { - // ... evaluate code - }); - // ... rest of sequential processing - } + await Promise.all(xpaths.filter(Boolean).map(async (xpath) => { + const evaluateResponse = await runtimeAgent.invoke<{result?: {objectId?: string}}>('evaluate', { + expression: ` + (function() { + const res = document.evaluate(${JSON.stringify(xpath)}, document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null); + return res.singleNodeValue; + })(); + `, + returnByValue: false, + }); + + const result = evaluateResponse.result; + if (result?.objectId) { + const nodeResponse = await domAgent.invoke<{node?: {backendNodeId?: number}}>('describeNode', { + objectId: result.objectId, + }); + const node = nodeResponse.node; + if (node?.backendNodeId) { + scrollableBackendIds.add(node.backendNodeId); + } + } + }));
857-859: Redundant re-throw provides no additional context.The catch block simply re-throws the same error without adding context or logging.
♻️ Either remove the try-catch or add context
- } catch (error) { - throw error; - } + // Remove the try-catch entirely since it doesn't add value
1207-1256: Consider validating input text before processing.The
fillElementfunction processes each character individually. For very long text inputs, this could be slow. Consider adding a length check or warning for excessively long inputs.Additionally, the function clears the value then types character-by-character, then sets the value again. This triple-action approach ensures compatibility but may trigger unnecessary events.
1362-1384: Unused executionContextId parameter in checkElement and uncheckElement.Both functions accept
executionContextIdas a parameter but never use it in thecallFunctionOninvocation.♻️ Either use the parameter or remove it
async function checkElement( runtimeAgent: ReturnType<CDPSessionAdapter['runtimeAgent']>, objectId: string, - executionContextId?: number, ): Promise<void> { await runtimeAgent.invoke('callFunctionOn', { objectId, + // executionContextId is not needed when objectId is provided functionDeclaration: `...`,Also applies to: 1386-1408
front_end/panels/ai_chat/tools/search/SearchPatternCache.ts (2)
199-218: Upsert behavior is clear but could be more atomic.The
deletePatternBySitefollowed bystore.addimplements upsert semantics, but this is not atomic—if the add fails after delete, data is lost. Consider usingstore.putdirectly for true upsert.♻️ Use put instead of delete+add
- // Delete existing pattern for this site (upsert) - await this.deletePatternBySite(normalizedSite); - return new Promise((resolve, reject) => { const transaction = db.transaction(STORE_NAME, 'readwrite'); const store = transaction.objectStore(STORE_NAME); - const request = store.add(fullPattern); + // First, delete any existing pattern with this site via the index + const index = store.index('site'); + const cursorRequest = index.openCursor(IDBKeyRange.only(normalizedSite)); + + cursorRequest.onsuccess = () => { + const cursor = cursorRequest.result; + if (cursor) { + store.delete(cursor.primaryKey); + cursor.continue(); + } else { + // All old patterns deleted, now add new one + const addRequest = store.add(fullPattern); + addRequest.onsuccess = () => { + this.memoryCache.set(normalizedSite, fullPattern); + resolve(fullPattern); + }; + addRequest.onerror = () => reject(addRequest.error); + } + };Or simpler, if the index is truly unique, just use
put:- await this.deletePatternBySite(normalizedSite); return new Promise((resolve, reject) => { const transaction = db.transaction(STORE_NAME, 'readwrite'); const store = transaction.objectStore(STORE_NAME); - const request = store.add(fullPattern); + const request = store.put(fullPattern);
505-506: Type coercion could be cleaner.The expression
request.result as SearchPattern | undefined || nullhas unusual precedence. Consider explicit handling.♻️ Clearer null handling
request.onsuccess = () => { - resolve(request.result as SearchPattern | undefined || null); + resolve((request.result as SearchPattern | undefined) ?? null); };front_end/panels/ai_chat/tools/action_cache/ActionPatternCache.ts (2)
160-174: File writes are not debounced—could cause excessive I/O.Every call to
save,update, orrecordSuccess/recordFailuretriggers a file write. For high-frequency updates, this could cause performance issues.Consider debouncing file writes or batching them.
♻️ Add debounced file saving
private saveTimeout: ReturnType<typeof setTimeout> | null = null; private readonly SAVE_DEBOUNCE_MS = 1000; private scheduleSaveToFile(): void { if (!isNodeEnvironment) return; if (this.saveTimeout) { clearTimeout(this.saveTimeout); } this.saveTimeout = setTimeout(() => { this.saveToFile(); this.saveTimeout = null; }, this.SAVE_DEBOUNCE_MS); } // Replace direct saveToFile() calls with scheduleSaveToFile()
376-383: Consider adding error handling for file operations in Node.js save path.If
loadFromFileorsaveToFilefails, the error propagates but the pattern is returned successfully. This could lead to inconsistent state.♻️ Add try-catch around file operations
if (isNodeEnvironment) { - await this.loadFromFile(); // Ensure existing cache is loaded - this.memoryCache.set(cacheKey, pattern); - await this.saveToFile(); - logger.info(`Saved pattern to file cache for ${cacheKey}`); - return pattern; + try { + await this.loadFromFile(); + this.memoryCache.set(cacheKey, pattern); + await this.saveToFile(); + logger.info(`Saved pattern to file cache for ${cacheKey}`); + } catch (err) { + logger.warn(`File cache operation failed, using memory only: ${err}`); + this.memoryCache.set(cacheKey, pattern); + } + return pattern; }front_end/panels/ai_chat/common/accessibility-tree-search.ts (1)
277-284: Double application of startsWithBoost for role matches.When
roleMatchis true, the boost is applied (line 281). But role matching already requires a prefix match (line 254), so this may be double-counting the "starts-with" semantic.This appears intentional to further prioritize role matches, but consider documenting this behavior.
front_end/panels/ai_chat/evaluation/test-cases/cdp-tool-tests.ts (2)
99-132: Test URL using twitter.com may cause issues.Twitter (now X) has aggressive anti-automation measures and frequently changes its login flow. Consider using a more stable test target for the history boundary test.
♻️ Use a more stable test URL
export const navigateBackHistoryBoundaryTest: TestCase<NavigateBackArgs> = { id: 'tool-navigate-back-003', name: 'History Boundary Error Handling', description: 'Attempt to go back 5 steps with insufficient history - should handle gracefully', - url: 'https://twitter.com', + url: 'https://the-internet.herokuapp.com/', tool: 'navigate_back',
1211-1220: Combined cdpToolTests array lacks explicit type annotation.The array spreads multiple typed arrays, but the combined array loses the specific generic types. Consider adding explicit typing for better tooling support.
♻️ Add explicit type
-export const cdpToolTests = [ +export const cdpToolTests: TestCase<unknown>[] = [ ...navigateBackToolTests, ...executeCodeToolTests, ...hybridA11yToolTests, ...waitToolTests, ...nodeIdsToUrlsToolTests, ...networkAnalysisToolTests, ...objectiveActionToolTests, ...getPageContentSearchTests, ];
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
CLAUDE.mdfront_end/panels/ai_chat/BUILD.gnfront_end/panels/ai_chat/a11y/HybridSnapshotUniversal.tsfront_end/panels/ai_chat/agent_framework/AgentRunner.tsfront_end/panels/ai_chat/agent_framework/AgentSessionTypes.tsfront_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.tsfront_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.tsfront_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV0.tsfront_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.tsfront_end/panels/ai_chat/cdp/FrameRegistryUniversal.tsfront_end/panels/ai_chat/common/accessibility-tree-search.tsfront_end/panels/ai_chat/common/utils-universal.tsfront_end/panels/ai_chat/common/utils.tsfront_end/panels/ai_chat/evaluation/framework/types.tsfront_end/panels/ai_chat/evaluation/test-cases/action-agent-tests.tsfront_end/panels/ai_chat/evaluation/test-cases/cdp-tool-tests.tsfront_end/panels/ai_chat/evaluation/test-cases/search-tool-tests.tsfront_end/panels/ai_chat/tools/BookmarkStoreTool.tsfront_end/panels/ai_chat/tools/CachedSchemaExtractorTool.tsfront_end/panels/ai_chat/tools/GetAccessibilityTreeToolV0.tsfront_end/panels/ai_chat/tools/HTMLToMarkdownTool.tsfront_end/panels/ai_chat/tools/SchemaBasedExtractorTool.tsfront_end/panels/ai_chat/tools/SearchTool.tsfront_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.tsfront_end/panels/ai_chat/tools/Tools.tsfront_end/panels/ai_chat/tools/TryCachedActionTool.tsfront_end/panels/ai_chat/tools/action_cache/ActionPatternCache.tsfront_end/panels/ai_chat/tools/action_cache/ActionPatternCapture.tsfront_end/panels/ai_chat/tools/action_cache/types.tsfront_end/panels/ai_chat/tools/search/SearchPatternCache.tsfront_end/panels/ai_chat/tools/search/SearchStrategy.tsfront_end/panels/ai_chat/tools/search/types.tsfront_end/panels/ai_chat/tools/selector_cache/SelectorCache.tsfront_end/panels/ai_chat/tools/selector_cache/types.tsfront_end/panels/ai_chat/utils/ContentChunker.tsscripts/eval-runner/AgentBridge.tsscripts/eval-runner/BrowserExecutor.tsscripts/eval-runner/TestRunner.tsscripts/eval-runner/cli.tsscripts/eval-runner/lib/ToolSetup.tsscripts/eval-runner/reporters/ComparisonReporter.tsscripts/eval-runner/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- front_end/panels/ai_chat/tools/BookmarkStoreTool.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/lib/EvalServer.js : Use Chrome DevTools Protocol (CDP) for direct browser communication including screenshot capture via Page.captureScreenshot, page content via Runtime.evaluate, and tab management via Target.createTarget/closeTarget
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Browser must be started with remote debugging enabled via --remote-debugging-port=9223 with CDP endpoint at http://localhost:9223/json/version
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/lib/EvalServer.js : Use Chrome DevTools Protocol (CDP) for direct browser communication including screenshot capture via Page.captureScreenshot, page content via Runtime.evaluate, and tab management via Target.createTarget/closeTarget
Applied to files:
front_end/panels/ai_chat/tools/HTMLToMarkdownTool.tsCLAUDE.mdfront_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.tsfront_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.tsfront_end/panels/ai_chat/tools/SchemaBasedExtractorTool.tsfront_end/panels/ai_chat/agent_framework/AgentRunner.ts
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/api-server.js : POST /page/execute endpoint must accept clientId, tabId, expression, returnByValue, and awaitPromise; use CDP Runtime.evaluate and return result with type and value
Applied to files:
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts
🧬 Code graph analysis (11)
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV0.ts (2)
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)
AgentToolConfig(112-229)ConfigurableAgentArgs(293-308)front_end/panels/ai_chat/core/Constants.ts (1)
MODEL_SENTINELS(81-84)
front_end/panels/ai_chat/common/utils.ts (4)
front_end/panels/ai_chat/common/xpath-builder.ts (1)
XPATH_BUILDER_FUNCTION_STRING(19-63)front_end/panels/ai_chat/cdp/DirectCDPAdapter.ts (2)
domAgent(88-90)inputAgent(104-106)front_end/panels/ai_chat/cdp/SDKTargetAdapter.ts (2)
domAgent(79-81)inputAgent(95-97)front_end/generated/protocol.ts (1)
BackendNodeId(4383-4383)
front_end/panels/ai_chat/tools/CachedSchemaExtractorTool.ts (6)
front_end/panels/ai_chat/tools/Tools.ts (2)
Tool(147-156)LLMContext(162-172)front_end/panels/ai_chat/tools/selector_cache/types.ts (2)
CachedSchemaExtractionArgs(57-70)CachedSchemaExtractionResult(75-88)front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts (2)
SchemaBasedExtractorTool(73-1246)SchemaDefinition(1260-1266)front_end/panels/ai_chat/tools/selector_cache/SelectorCache.ts (1)
SelectorCache(29-544)front_end/panels/ai_chat/a11y/HybridSnapshotUniversal.ts (1)
captureHybridSnapshotUniversal(624-707)front_end/panels/ai_chat/tools/LLMTracingWrapper.ts (1)
callLLMWithTracing(28-139)
front_end/panels/ai_chat/tools/search/SearchPatternCache.ts (1)
front_end/panels/ai_chat/tools/search/types.ts (6)
SiteIdentifier(10-10)SearchPattern(136-165)PATTERN_SCHEMA_VERSION(306-306)PatternExport(273-280)PATTERN_EXPIRY_MS(312-312)FAILURE_RATE_THRESHOLD(315-315)
front_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.ts (6)
front_end/panels/ai_chat/cdp/index.ts (2)
getAdapter(12-12)CDPSessionAdapter(8-8)front_end/panels/ai_chat/cdp/getAdapter.ts (1)
getAdapter(84-103)front_end/panels/ai_chat/cdp/CDPSessionAdapter.ts (1)
CDPSessionAdapter(34-66)front_end/panels/ai_chat/cdp/DirectCDPAdapter.ts (1)
accessibilityAgent(100-102)front_end/panels/ai_chat/cdp/SDKTargetAdapter.ts (1)
accessibilityAgent(91-93)front_end/panels/ai_chat/common/context.ts (1)
AXNode(10-19)
front_end/panels/ai_chat/tools/GetAccessibilityTreeToolV0.ts (2)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(322-324)front_end/panels/ai_chat/tools/Tools.ts (4)
Tool(147-156)AccessibilityTreeResult(333-363)ErrorResult(220-222)LLMContext(162-172)
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.ts (7)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(322-324)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (3)
ConfigurableAgentArgs(293-308)AgentToolConfig(112-229)CallCtx(22-48)front_end/panels/ai_chat/core/Constants.ts (1)
MODEL_SENTINELS(81-84)front_end/panels/ai_chat/tools/TryCachedActionTool.ts (1)
getCurrentUrl(146-156)front_end/panels/ai_chat/tools/action_cache/ActionPatternCapture.ts (1)
getActionPatternCapture(444-449)front_end/panels/ai_chat/tools/action_cache/types.ts (1)
PerformActionWithIntent(160-175)front_end/panels/ai_chat/tools/action_cache/ActionPatternCache.ts (1)
ActionPatternCache(37-628)
front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts (3)
front_end/panels/ai_chat/cdp/getAdapter.ts (1)
getAdapter(84-103)front_end/panels/ai_chat/cdp/DirectCDPAdapter.ts (1)
accessibilityAgent(100-102)front_end/panels/ai_chat/cdp/SDKTargetAdapter.ts (1)
accessibilityAgent(91-93)
front_end/panels/ai_chat/evaluation/test-cases/cdp-tool-tests.ts (1)
scripts/eval-runner/types.ts (1)
TestCase(56-74)
front_end/panels/ai_chat/agent_framework/AgentRunner.ts (7)
front_end/panels/ai_chat/core/AgentService.ts (1)
logger(498-509)front_end/panels/ai_chat/tools/FileStorageManager.ts (1)
FileStorageManager(44-298)front_end/panels/ai_chat/models/ChatTypes.ts (2)
ChatMessage(74-75)ToolResultMessage(53-63)front_end/panels/ai_chat/agent_framework/AgentSessionTypes.ts (3)
ToolResultMessage(94-102)AgentMessage(62-69)AgentSession(11-44)front_end/panels/ai_chat/agent_framework/RuntimeContext.ts (1)
getRuntime(27-29)front_end/panels/ai_chat/core/AgentErrorHandler.ts (1)
addSessionMessage(213-228)front_end/panels/ai_chat/tracing/TracingConfig.ts (1)
getCurrentTracingContext(299-301)
front_end/panels/ai_chat/evaluation/test-cases/action-agent-tests.ts (4)
front_end/panels/ai_chat/evaluation/test-cases/index.ts (1)
datePickerFillTest(30-30)front_end/panels/ai_chat/evaluation/framework/types.ts (1)
TestCase(12-30)front_end/panels/ai_chat/evaluation/test-cases/action-agent-shadow-dom-tests.ts (1)
ActionAgentArgs(7-12)front_end/panels/ai_chat/evaluation/test-cases/action-agent-iframe-tests.ts (1)
ActionAgentArgs(7-12)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
99-99: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
112-112: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
136-136: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
179-179: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (41)
front_end/panels/ai_chat/agent_framework/AgentSessionTypes.ts (1)
41-57: Well-structured metrics interface.The
AgentSessionMetricsinterface provides comprehensive telemetry tracking for agent sessions. The separation of prompt/completion tokens and the tool call breakdown by name will be valuable for debugging and optimization.front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts (2)
193-242: Solid CDP adapter integration with proper error handling.The refactoring to use
cdpAdapterinstead of SDK TargetManager is well-structured. The guard for missing adapter (lines 196-199) and the try-catch wrapper (lines 201-242) ensure graceful degradation. The auto-navigation to Google when onchrome://URLs is a practical workaround.One minor observation: the provider check at line 215-218 is good, but consider logging more context about why auto-navigation was skipped.
90-97: Helpful EncodedId documentation and examples.The updated method examples with EncodedId format (
"0-123") and frame ordinal prefixes provide clear guidance for the LLM on proper nodeId usage.front_end/panels/ai_chat/tools/HTMLToMarkdownTool.ts (1)
116-128: Clean cross-environment adapter integration.The adapter retrieval with structured error response when unavailable is a clean pattern that enables the tool to work in both DevTools and eval runner contexts.
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.ts (1)
50-169: Well-documented agent configuration with clear cache workflow.The system prompt clearly explains the cache-first workflow with the decision tree and semantic_intent requirements. The method examples with EncodedId format are helpful.
front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts (2)
498-525: Efficient pre-built URL mapping approach.Building the URL mapping upfront from AX nodes eliminates the need for separate NodeIDsToURLsTool calls during resolution. The logging of sample mappings (lines 520-521) will be helpful for debugging.
872-903: Robust node ID handling for both numeric and string types.The
findAndReplaceNodeIdsmethod correctly handles the type mismatch between LLM-returned numeric IDs and accessibility tree string IDs by normalizing to string for lookup. This is a practical solution.front_end/panels/ai_chat/agent_framework/AgentRunner.ts (5)
97-141: Well-implemented idempotent cleanup with race condition handling.The cleanup lock pattern using
cleanupInProgressSet prevents concurrent cleanup operations. The try-finally ensures the lock is always released, and the specific handling of "file not found" errors (lines 128-129) gracefully handles the race condition where another agent already deleted the file.
265-305: Smart context management with accessibility tree pruning.Keeping only the last 2
get_page_contentresults and redacting older ones is a practical solution to prevent context overflow. The redaction message clearly indicates how to fetch fresh data if needed.
549-558: Comprehensive metrics tracking initialization.The metrics object is properly initialized with all counters at zero. This will enable accurate tracking of LLM calls and tool usage throughout the session.
851-862: Correct token usage accumulation from raw LLM response.The code properly handles both OpenAI-style (
prompt_tokens/completion_tokens) and alternative naming conventions (input_tokens/output_tokens) for token counts.
1242-1267: Pre-execution hook properly wrapped with error handling.The
onBeforeToolExecutionhook is correctly wrapped in try-catch (lines 1244-1251) to ensure tool execution continues even if the hook fails. This is important for observability hooks that shouldn't block the main execution flow.front_end/panels/ai_chat/evaluation/framework/types.ts (1)
25-28: Useful metadata fields for dynamic content handling.The
waitForSelectorandwaitAfterNavigationoptions provide flexibility for test cases that need to handle dynamically loaded content or animations before validation.front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts (1)
54-59: Tool/DOM registration sequencing looks reasonable.Registering
registerDOMTools()immediately afterinitializeMiniApps()keeps tool wiring centralized and predictable.Also applies to: 67-77
front_end/panels/ai_chat/common/utils.ts (1)
33-34: Centralizing the XPath builder constant is a solid cleanup.Using
XPATH_BUILDER_FUNCTION_STRINGreduces duplication and keeps behavior consistent across call sites.Also applies to: 659-663
front_end/panels/ai_chat/tools/SearchTool.ts (1)
95-242: Overall structure (adapter retrieval, caching, and metadata) is clean.The control flow is easy to follow and “cache errors don’t block results” is a good reliability choice.
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV0.ts (1)
30-160: V0 baseline config is nicely self-contained for A/B evaluation.Clear tool surface, explicit maxIterations, and a single handoff point make it straightforward to compare against v1/v2.
front_end/panels/ai_chat/tools/GetAccessibilityTreeToolV0.ts (1)
30-65: Good: adapter-based implementation keeps this usable in eval runner.Nice minimal surface while still going through
getAdapter(ctx)for cross-environment support.front_end/panels/ai_chat/BUILD.gn (1)
126-260: BUILD wiring for new tools/agents/eval tests looks consistent.New modules and tests are added to both the main
sourceslist and_ai_chat_sources, which should keep the release metadata generation aligned.Also applies to: 417-550, 689-692
front_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.ts (1)
127-148: Adapter-based setup + lazy browser-only import is a good direction.The
getAdapter(ctx)+ lazyAgentServicefallback keeps the tool usable in both DevTools and eval runner contexts.front_end/panels/ai_chat/evaluation/test-cases/action-agent-tests.ts (1)
132-172: E-commerce test stability: good that it’s marked flaky + long timeout.Given how often HomeDepot changes UI/cookie gates, the
flaky: true+ retries/timeout looks appropriate.front_end/panels/ai_chat/tools/action_cache/ActionPatternCapture.ts (1)
169-176: Clarify OOPIF handling: code usesPage.getFrameTree, notDOM.getDocument, which has the same cross-origin iframe limitation.The code relies on
FrameRegistryUniversalcallingPage.getFrameTreeandDOM.getFrameOwner. LikeDOM.getDocumentwithpierce:true,Page.getFrameTreedoes not include cross-origin OOPIF (out-of-process iframe) DOM trees in a single target—these require separateTarget.attachToTargetcalls. The current implementation handles same-origin iframes through frame ordinals and per-frame execution contexts, but won't traverse into cross-origin frames without additional target attachment logic.front_end/panels/ai_chat/cdp/FrameRegistryUniversal.ts (2)
17-28: Well-structured interface definition.The
FrameInfointerface cleanly captures all necessary frame metadata including ordinal for EncodedId generation and optionalownerBackendNodeIdfor iframe element tracking.
49-74: Ensure error from getFrameTree is propagated appropriately.The catch block returns an empty array on failure, which is fine for graceful degradation. However, consider whether calling code needs to distinguish between "no frames exist" and "failed to collect frames" scenarios.
front_end/panels/ai_chat/common/utils-universal.ts (3)
111-150: Good shadow DOM handling in walkNode.The recursive
walkNodecorrectly handles shadow roots first before regular children, ensuring shadow DOM content is properly captured with XPath context inherited from the host element.
920-959: Good cross-frame element resolution with isolated world.The logic correctly handles iframe elements by creating an isolated world in the target frame and resolving nodes within that execution context. This is essential for proper cross-frame action execution.
1640-1643: Good note about CDP mutual exclusivity.The comment correctly explains that
objectIdandexecutionContextIdare mutually exclusive in CDP'scallFunctionOn. This prevents a subtle bug.front_end/panels/ai_chat/tools/search/SearchPatternCache.ts (3)
29-47: Singleton pattern correctly implemented.The private constructor and static
getInstance()method properly implement the singleton pattern, ensuring consistent cache state across the application.
130-165: getPattern correctly checks expiry and degradation before returning.The pattern retrieval logic properly validates the pattern state (expired/degraded) before returning, ensuring callers don't receive stale or unreliable patterns.
543-551: Good degradation detection with minimum sample size.Requiring at least 5 uses before calculating failure rate prevents premature pattern invalidation from initial failures during learning phase.
front_end/panels/ai_chat/tools/action_cache/ActionPatternCache.ts (3)
127-155: File loading uses IIFE pattern for async initialization.The pattern of assigning
fileLoadPromiseto an immediately-invoked async function ensures the load happens once and can be awaited multiple times. Good pattern for lazy initialization.
229-237: Cache key generation is well-designed for intent-based lookup.The combination of normalized domain + path pattern + semantic intent creates a meaningful cache key that allows pattern reuse across similar pages on the same site.
371-372: Initial successCount of 1 is intentional.Starting with
successCount: 1since the pattern is saved after a successful action is a good design choice that accurately reflects the pattern's initial validation.front_end/panels/ai_chat/common/accessibility-tree-search.ts (5)
120-123: Pre-compiled regex patterns are a good performance optimization.Compiling regex patterns once at module load avoids repeated compilation during search operations, which is important for the stated <100ms latency goal.
130-140: Robust parseFrameOrdinal implementation.The function correctly handles edge cases: empty string, missing dash, and non-numeric ordinal values all return 0 (main frame) as a safe default.
252-263: Role matching uses prefix match while name uses substring.The asymmetric matching strategy (prefix for role, substring for name) is well-considered—it prevents false positives like "box" matching "checkbox" while still allowing flexible text search in element names.
329-354: Good defensive programming in search method.The method validates inputs, merges options with defaults, caps
maxResultsto prevent abuse, and preprocesses the query once for efficiency before iterating.
419-426: Convenience function creates new instances per call.For repeated searches, this creates new
AccessibilityTreeSearcherinstances each time. If search is called frequently in a tight loop, consider allowing reuse.For typical usage patterns, this is fine and provides a simple API.
front_end/panels/ai_chat/evaluation/test-cases/cdp-tool-tests.ts (3)
143-187: Good use of flaky flag for unstable tests.The
flaky: trueflag on Amazon extraction tests acknowledges the inherent instability of testing against live e-commerce sites with dynamic content.
1131-1136: Test collection correctly groups related tests.The exported arrays like
getPageContentSearchTestsprovide logical groupings that enable selective test execution.
19-27: No type inconsistency exists.The
TestCase<TInput = any>generic inframework/types.tsproperly supports the local argument interfaces likeNavigateBackArgs. TheTestCaseinterface inscripts/eval-runner/types.tsis a separate, unrelated type used by the CLI runner and does not affect test case definitions incdp-tool-tests.ts. The test cases are correctly typed withTestCase<NavigateBackArgs>,TestCase<ExecuteCodeArgs>, etc.Likely an incorrect or invalid review comment.
| export interface NodeIDsToURLsArgs { | ||
| nodeIds: number[]; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for NodeIDsToURLsTool and related implementations
rg -n "NodeIDsToURLs" --type ts -A 5 -B 2 front_end/panels/ai_chat/Repository: BrowserOperator/browser-operator-core
Length of output: 11671
🏁 Script executed:
#!/bin/bash
# Search for EncodedIds definition and usage patterns
rg -n "type EncodedIds|interface EncodedIds" --type ts -A 2Repository: BrowserOperator/browser-operator-core
Length of output: 63
🏁 Script executed:
#!/bin/bash
# Look at the specific file context around the interface
sed -n '745,765p' front_end/panels/ai_chat/evaluation/test-cases/cdp-tool-tests.tsRepository: BrowserOperator/browser-operator-core
Length of output: 724
Fix type mismatch: NodeIDsToURLsArgs.nodeIds should use string[] for EncodedIds, not number[].
The NodeIDsToURLsTool implementation expects nodeIds: string[] in the format of EncodedIds (e.g., ["0-123", "0-456"]), but the test interface defines it as number[]. The tool's error message explicitly states: "nodeIds must be an array of EncodedId strings". Update the interface and test data to use strings instead of numbers.
🤖 Prompt for AI Agents
In @front_end/panels/ai_chat/evaluation/test-cases/cdp-tool-tests.ts around
lines 752 - 755, The NodeIDsToURLsArgs interface is typed incorrectly: change
NodeIDsToURLsArgs.nodeIds from number[] to string[] to match the EncodedId
format expected by NodeIDsToURLsTool, and update any test fixtures in this file
that pass numeric IDs to instead pass EncodedId strings (e.g., "0-123",
"0-456"); ensure the tool's error/message text remains consistent with the
string type.
| export interface SearchToolArgs { | ||
| query: string; | ||
| site: string; | ||
| reasoning: string; | ||
| maxResults?: number; | ||
| forceRefresh?: boolean; | ||
| strategy?: 'xpath-schema' | 'semantic-xpath' | 'encoded-id' | 'text-pattern' | 'cdp' | 'js-eval'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for search tool implementation and strategy definitions
rg -n --type=ts "strategy|name\s*[=:]\s*['\"]search['\"]" front_end/panels/ai_chat -S -A 3 -B 1 | head -200Repository: BrowserOperator/browser-operator-core
Length of output: 17006
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the actual types used in SearchTool.ts for SearchStrategyType
rg -n "type SearchStrategyType|enum SearchStrategyType" front_end/panels/ai_chat -A 2Repository: BrowserOperator/browser-operator-core
Length of output: 412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the complete SearchStrategyType definition
cat -n front_end/panels/ai_chat/tools/search/types.ts | sed -n '170,180p'Repository: BrowserOperator/browser-operator-core
Length of output: 699
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check what SearchTool.ts declares in its schema and compare with the type definition
echo "=== SearchTool.ts schema enum (lines 82-86) ==="
sed -n '82,86p' front_end/panels/ai_chat/tools/SearchTool.ts
echo ""
echo "=== SearchStrategyType in types.ts (complete definition) ==="
sed -n '170,178p' front_end/panels/ai_chat/tools/search/types.tsRepository: BrowserOperator/browser-operator-core
Length of output: 1050
Update SearchTool schema enum to include all valid SearchStrategyType values.
The tool schema in SearchTool.ts line 84 is missing two strategy types from the SearchStrategyType definition: 'xpath-llm' and 'css-llm'. These are valid strategies but not exposed in the tool's interface, making them inaccessible to callers.
Update the enum to:
enum: ['xpath-schema', 'semantic-xpath', 'encoded-id', 'text-pattern', 'xpath-llm', 'css-llm', 'cdp', 'js-eval']
Update SearchToolArgs in the test file to match.
🤖 Prompt for AI Agents
In @front_end/panels/ai_chat/evaluation/test-cases/search-tool-tests.ts around
lines 7 - 14, SearchTool's schema enum is missing two valid SearchStrategyType
values ('xpath-llm' and 'css-llm'), and the test's SearchToolArgs interface
should mirror that set; update the enum used in the SearchTool schema
(referencing SearchTool and SearchStrategyType) to include
['xpath-schema','semantic-xpath','encoded-id','text-pattern','xpath-llm','css-llm','cdp','js-eval']
and modify the SearchToolArgs interface in the test (SearchToolArgs) so its
strategy union includes 'xpath-llm' and 'css-llm' as valid string literals.
| /** | ||
| * Extract XPath, CSS selector, and attributes from an element | ||
| */ | ||
| private async extractElementInfo(encodedId: string): Promise<{ | ||
| xpath: string | null; | ||
| cssSelector: string | null; | ||
| attributes: ElementAttributes; | ||
| }> { | ||
| const parsed = parseEncodedId(encodedId); | ||
| if (!parsed) { | ||
| return { xpath: null, cssSelector: null, attributes: {} }; | ||
| } | ||
|
|
||
| const { frameOrdinal, backendNodeId } = parsed; | ||
|
|
||
| try { | ||
| const domAgent = this.adapter.domAgent(); | ||
| const runtimeAgent = this.adapter.runtimeAgent(); | ||
|
|
||
| // Get execution context for the frame | ||
| let executionContextId: number | undefined; | ||
| if (frameOrdinal > 0) { | ||
| const frameRegistry = new FrameRegistryUniversal(this.adapter); | ||
| await frameRegistry.collectFrames(); | ||
| const frameInfo = frameRegistry.getFrameByOrdinal(frameOrdinal); | ||
| if (frameInfo) { | ||
| executionContextId = await this.getFrameExecutionContextId(frameInfo.frameId); | ||
| } | ||
| } | ||
|
|
||
| // Resolve the node to get objectId | ||
| const resolveResponse = await domAgent.invoke<{ | ||
| object?: { objectId?: string }; | ||
| }>('resolveNode', { | ||
| backendNodeId, | ||
| executionContextId, | ||
| }); | ||
|
|
||
| if (!resolveResponse.object?.objectId) { | ||
| return { xpath: null, cssSelector: null, attributes: {} }; | ||
| } | ||
|
|
||
| const objectId = resolveResponse.object.objectId; | ||
|
|
||
| // Extract all info in one call | ||
| const result = await runtimeAgent.invoke<{ | ||
| result?: { value?: { | ||
| xpath: string; | ||
| cssSelector: string; | ||
| idAttr: string | null; | ||
| nameAttr: string | null; | ||
| ariaLabel: string | null; | ||
| placeholder: string | null; | ||
| inputType: string | null; | ||
| tagName: string | null; | ||
| role: string | null; | ||
| textContent: string | null; | ||
| } }; | ||
| }>('callFunctionOn', { | ||
| objectId, | ||
| functionDeclaration: ` | ||
| function() { | ||
| const el = this; | ||
| // Generate XPath | ||
| function getXPath(element) { | ||
| if (!element) return ''; | ||
| // Prefer ID-based XPath (most stable) | ||
| if (element.id) { | ||
| return '//*[@id="' + element.id + '"]'; | ||
| } | ||
| // Try name attribute for form elements | ||
| if (element.name && ['INPUT', 'SELECT', 'TEXTAREA', 'BUTTON'].includes(element.tagName)) { | ||
| const tag = element.tagName.toLowerCase(); | ||
| return '//' + tag + '[@name="' + element.name + '"]'; | ||
| } | ||
| // Try aria-label | ||
| const ariaLabel = element.getAttribute('aria-label'); | ||
| if (ariaLabel) { | ||
| return '//*[@aria-label="' + ariaLabel + '"]'; | ||
| } | ||
| // Fall back to positional XPath | ||
| if (element === document.body) return '/html/body'; | ||
| let ix = 0; | ||
| const siblings = element.parentNode?.children || []; | ||
| for (let i = 0; i < siblings.length; i++) { | ||
| const sibling = siblings[i]; | ||
| if (sibling === element) { | ||
| const tag = element.tagName.toLowerCase(); | ||
| const parentPath = getXPath(element.parentNode); | ||
| return parentPath + '/' + tag + '[' + (ix + 1) + ']'; | ||
| } | ||
| if (sibling.nodeType === 1 && sibling.tagName === element.tagName) { | ||
| ix++; | ||
| } | ||
| } | ||
| return ''; | ||
| } | ||
| // Generate CSS selector | ||
| function getCssSelector(element) { | ||
| if (!element) return ''; | ||
| const tag = element.tagName.toLowerCase(); | ||
| if (element.id) { | ||
| return tag + '#' + element.id; | ||
| } | ||
| if (element.name) { | ||
| return tag + '[name="' + element.name + '"]'; | ||
| } | ||
| if (element.className && typeof element.className === 'string') { | ||
| const classes = element.className.trim().split(/\\s+/).slice(0, 2).join('.'); | ||
| if (classes) return tag + '.' + classes; | ||
| } | ||
| return tag; | ||
| } | ||
| // Get text content (trimmed, first 50 chars) | ||
| let textContent = (el.textContent || '').trim().substring(0, 50); | ||
| if (textContent.length === 50) textContent += '...'; | ||
| return { | ||
| xpath: getXPath(el), | ||
| cssSelector: getCssSelector(el), | ||
| idAttr: el.id || null, | ||
| nameAttr: el.name || null, | ||
| ariaLabel: el.getAttribute('aria-label') || null, | ||
| placeholder: el.placeholder || null, | ||
| inputType: el.type || null, | ||
| tagName: el.tagName?.toLowerCase() || null, | ||
| role: el.getAttribute('role') || null, | ||
| textContent: textContent || null, | ||
| }; | ||
| } | ||
| `, | ||
| returnByValue: true, | ||
| executionContextId, | ||
| }); | ||
|
|
||
| if (!result.result?.value) { | ||
| return { xpath: null, cssSelector: null, attributes: {} }; | ||
| } | ||
|
|
||
| const info = result.result.value; | ||
|
|
||
| const attributes: ElementAttributes = {}; | ||
| if (info.idAttr) attributes.idAttr = info.idAttr; | ||
| if (info.nameAttr) attributes.nameAttr = info.nameAttr; | ||
| if (info.ariaLabel) attributes.ariaLabel = info.ariaLabel; | ||
| if (info.placeholder) attributes.placeholder = info.placeholder; | ||
| if (info.inputType) attributes.inputType = info.inputType; | ||
| if (info.tagName) attributes.tagName = info.tagName; | ||
| if (info.role) attributes.role = info.role; | ||
| if (info.textContent) attributes.textContent = info.textContent; | ||
|
|
||
| return { | ||
| xpath: info.xpath || null, | ||
| cssSelector: info.cssSelector || null, | ||
| attributes, | ||
| }; | ||
| } catch (error) { | ||
| logger.error('Error extracting element info:', error); | ||
| return { xpath: null, cssSelector: null, attributes: {} }; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Find element by XPath and return its EncodedId | ||
| */ | ||
| private async findElementByXPath(xpath: string): Promise<string | null> { | ||
| try { | ||
| const runtimeAgent = this.adapter.runtimeAgent(); | ||
| const domAgent = this.adapter.domAgent(); | ||
|
|
||
| // Evaluate XPath to find element | ||
| const evalResult = await runtimeAgent.invoke<{ | ||
| result?: { objectId?: string }; | ||
| exceptionDetails?: unknown; | ||
| }>('evaluate', { | ||
| expression: ` | ||
| (function() { | ||
| const result = document.evaluate( | ||
| ${JSON.stringify(xpath)}, | ||
| document, | ||
| null, | ||
| XPathResult.FIRST_ORDERED_NODE_TYPE, | ||
| null | ||
| ); | ||
| return result.singleNodeValue; | ||
| })() | ||
| `, | ||
| returnByValue: false, | ||
| }); | ||
|
|
||
| if (!evalResult.result?.objectId) { | ||
| return null; | ||
| } | ||
|
|
||
| // Get backendNodeId from objectId | ||
| const nodeResult = await domAgent.invoke<{ | ||
| nodeId?: number; | ||
| node?: { backendNodeId?: number }; | ||
| }>('describeNode', { | ||
| objectId: evalResult.result.objectId, | ||
| }); | ||
|
|
||
| const backendNodeId = nodeResult.node?.backendNodeId; | ||
| if (!backendNodeId) { | ||
| return null; | ||
| } | ||
|
|
||
| // Return EncodedId (frame 0 for main frame) | ||
| return `0-${backendNodeId}`; | ||
| } catch (error) { | ||
| logger.debug('XPath lookup failed:', error); | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Find element by CSS selector and return its EncodedId | ||
| */ | ||
| private async findElementByCssSelector(selector: string): Promise<string | null> { | ||
| try { | ||
| const runtimeAgent = this.adapter.runtimeAgent(); | ||
| const domAgent = this.adapter.domAgent(); | ||
|
|
||
| const evalResult = await runtimeAgent.invoke<{ | ||
| result?: { objectId?: string }; | ||
| }>('evaluate', { | ||
| expression: `document.querySelector(${JSON.stringify(selector)})`, | ||
| returnByValue: false, | ||
| }); | ||
|
|
||
| if (!evalResult.result?.objectId) { | ||
| return null; | ||
| } | ||
|
|
||
| const nodeResult = await domAgent.invoke<{ | ||
| node?: { backendNodeId?: number }; | ||
| }>('describeNode', { | ||
| objectId: evalResult.result.objectId, | ||
| }); | ||
|
|
||
| const backendNodeId = nodeResult.node?.backendNodeId; | ||
| if (!backendNodeId) { | ||
| return null; | ||
| } | ||
|
|
||
| return `0-${backendNodeId}`; | ||
| } catch (error) { | ||
| logger.debug('CSS selector lookup failed:', error); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: cache lookup hardcodes frameOrdinal=0, breaking iframe correctness.
findElementByXPath() and findElementByCssSelector() return `0-${backendNodeId}` (Line 334, Line 371). If the cached pattern belongs to an iframe, this will generate the wrong EncodedId and can cascade into incorrect actions. Also, both lookups run document.evaluate/querySelector without selecting the correct frame execution context.
Proposed direction (requires schema change): persist frame identity and use it during lookup
- private async findElementByXPath(xpath: string): Promise<string | null> {
+ private async findElementByXPath(xpath: string, frameOrdinal: number): Promise<string | null> {
@@
- // Return EncodedId (frame 0 for main frame)
- return `0-${backendNodeId}`;
+ return `${frameOrdinal}-${backendNodeId}`;
}You’ll likely also need to persist frameOrdinal (or frameId) on the cached pattern at capture-time and pass it into lookup-time evaluation (so document is the iframe’s document, not the main frame’s).
📝 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.
| /** | |
| * Extract XPath, CSS selector, and attributes from an element | |
| */ | |
| private async extractElementInfo(encodedId: string): Promise<{ | |
| xpath: string | null; | |
| cssSelector: string | null; | |
| attributes: ElementAttributes; | |
| }> { | |
| const parsed = parseEncodedId(encodedId); | |
| if (!parsed) { | |
| return { xpath: null, cssSelector: null, attributes: {} }; | |
| } | |
| const { frameOrdinal, backendNodeId } = parsed; | |
| try { | |
| const domAgent = this.adapter.domAgent(); | |
| const runtimeAgent = this.adapter.runtimeAgent(); | |
| // Get execution context for the frame | |
| let executionContextId: number | undefined; | |
| if (frameOrdinal > 0) { | |
| const frameRegistry = new FrameRegistryUniversal(this.adapter); | |
| await frameRegistry.collectFrames(); | |
| const frameInfo = frameRegistry.getFrameByOrdinal(frameOrdinal); | |
| if (frameInfo) { | |
| executionContextId = await this.getFrameExecutionContextId(frameInfo.frameId); | |
| } | |
| } | |
| // Resolve the node to get objectId | |
| const resolveResponse = await domAgent.invoke<{ | |
| object?: { objectId?: string }; | |
| }>('resolveNode', { | |
| backendNodeId, | |
| executionContextId, | |
| }); | |
| if (!resolveResponse.object?.objectId) { | |
| return { xpath: null, cssSelector: null, attributes: {} }; | |
| } | |
| const objectId = resolveResponse.object.objectId; | |
| // Extract all info in one call | |
| const result = await runtimeAgent.invoke<{ | |
| result?: { value?: { | |
| xpath: string; | |
| cssSelector: string; | |
| idAttr: string | null; | |
| nameAttr: string | null; | |
| ariaLabel: string | null; | |
| placeholder: string | null; | |
| inputType: string | null; | |
| tagName: string | null; | |
| role: string | null; | |
| textContent: string | null; | |
| } }; | |
| }>('callFunctionOn', { | |
| objectId, | |
| functionDeclaration: ` | |
| function() { | |
| const el = this; | |
| // Generate XPath | |
| function getXPath(element) { | |
| if (!element) return ''; | |
| // Prefer ID-based XPath (most stable) | |
| if (element.id) { | |
| return '//*[@id="' + element.id + '"]'; | |
| } | |
| // Try name attribute for form elements | |
| if (element.name && ['INPUT', 'SELECT', 'TEXTAREA', 'BUTTON'].includes(element.tagName)) { | |
| const tag = element.tagName.toLowerCase(); | |
| return '//' + tag + '[@name="' + element.name + '"]'; | |
| } | |
| // Try aria-label | |
| const ariaLabel = element.getAttribute('aria-label'); | |
| if (ariaLabel) { | |
| return '//*[@aria-label="' + ariaLabel + '"]'; | |
| } | |
| // Fall back to positional XPath | |
| if (element === document.body) return '/html/body'; | |
| let ix = 0; | |
| const siblings = element.parentNode?.children || []; | |
| for (let i = 0; i < siblings.length; i++) { | |
| const sibling = siblings[i]; | |
| if (sibling === element) { | |
| const tag = element.tagName.toLowerCase(); | |
| const parentPath = getXPath(element.parentNode); | |
| return parentPath + '/' + tag + '[' + (ix + 1) + ']'; | |
| } | |
| if (sibling.nodeType === 1 && sibling.tagName === element.tagName) { | |
| ix++; | |
| } | |
| } | |
| return ''; | |
| } | |
| // Generate CSS selector | |
| function getCssSelector(element) { | |
| if (!element) return ''; | |
| const tag = element.tagName.toLowerCase(); | |
| if (element.id) { | |
| return tag + '#' + element.id; | |
| } | |
| if (element.name) { | |
| return tag + '[name="' + element.name + '"]'; | |
| } | |
| if (element.className && typeof element.className === 'string') { | |
| const classes = element.className.trim().split(/\\s+/).slice(0, 2).join('.'); | |
| if (classes) return tag + '.' + classes; | |
| } | |
| return tag; | |
| } | |
| // Get text content (trimmed, first 50 chars) | |
| let textContent = (el.textContent || '').trim().substring(0, 50); | |
| if (textContent.length === 50) textContent += '...'; | |
| return { | |
| xpath: getXPath(el), | |
| cssSelector: getCssSelector(el), | |
| idAttr: el.id || null, | |
| nameAttr: el.name || null, | |
| ariaLabel: el.getAttribute('aria-label') || null, | |
| placeholder: el.placeholder || null, | |
| inputType: el.type || null, | |
| tagName: el.tagName?.toLowerCase() || null, | |
| role: el.getAttribute('role') || null, | |
| textContent: textContent || null, | |
| }; | |
| } | |
| `, | |
| returnByValue: true, | |
| executionContextId, | |
| }); | |
| if (!result.result?.value) { | |
| return { xpath: null, cssSelector: null, attributes: {} }; | |
| } | |
| const info = result.result.value; | |
| const attributes: ElementAttributes = {}; | |
| if (info.idAttr) attributes.idAttr = info.idAttr; | |
| if (info.nameAttr) attributes.nameAttr = info.nameAttr; | |
| if (info.ariaLabel) attributes.ariaLabel = info.ariaLabel; | |
| if (info.placeholder) attributes.placeholder = info.placeholder; | |
| if (info.inputType) attributes.inputType = info.inputType; | |
| if (info.tagName) attributes.tagName = info.tagName; | |
| if (info.role) attributes.role = info.role; | |
| if (info.textContent) attributes.textContent = info.textContent; | |
| return { | |
| xpath: info.xpath || null, | |
| cssSelector: info.cssSelector || null, | |
| attributes, | |
| }; | |
| } catch (error) { | |
| logger.error('Error extracting element info:', error); | |
| return { xpath: null, cssSelector: null, attributes: {} }; | |
| } | |
| } | |
| /** | |
| * Find element by XPath and return its EncodedId | |
| */ | |
| private async findElementByXPath(xpath: string): Promise<string | null> { | |
| try { | |
| const runtimeAgent = this.adapter.runtimeAgent(); | |
| const domAgent = this.adapter.domAgent(); | |
| // Evaluate XPath to find element | |
| const evalResult = await runtimeAgent.invoke<{ | |
| result?: { objectId?: string }; | |
| exceptionDetails?: unknown; | |
| }>('evaluate', { | |
| expression: ` | |
| (function() { | |
| const result = document.evaluate( | |
| ${JSON.stringify(xpath)}, | |
| document, | |
| null, | |
| XPathResult.FIRST_ORDERED_NODE_TYPE, | |
| null | |
| ); | |
| return result.singleNodeValue; | |
| })() | |
| `, | |
| returnByValue: false, | |
| }); | |
| if (!evalResult.result?.objectId) { | |
| return null; | |
| } | |
| // Get backendNodeId from objectId | |
| const nodeResult = await domAgent.invoke<{ | |
| nodeId?: number; | |
| node?: { backendNodeId?: number }; | |
| }>('describeNode', { | |
| objectId: evalResult.result.objectId, | |
| }); | |
| const backendNodeId = nodeResult.node?.backendNodeId; | |
| if (!backendNodeId) { | |
| return null; | |
| } | |
| // Return EncodedId (frame 0 for main frame) | |
| return `0-${backendNodeId}`; | |
| } catch (error) { | |
| logger.debug('XPath lookup failed:', error); | |
| return null; | |
| } | |
| } | |
| /** | |
| * Find element by CSS selector and return its EncodedId | |
| */ | |
| private async findElementByCssSelector(selector: string): Promise<string | null> { | |
| try { | |
| const runtimeAgent = this.adapter.runtimeAgent(); | |
| const domAgent = this.adapter.domAgent(); | |
| const evalResult = await runtimeAgent.invoke<{ | |
| result?: { objectId?: string }; | |
| }>('evaluate', { | |
| expression: `document.querySelector(${JSON.stringify(selector)})`, | |
| returnByValue: false, | |
| }); | |
| if (!evalResult.result?.objectId) { | |
| return null; | |
| } | |
| const nodeResult = await domAgent.invoke<{ | |
| node?: { backendNodeId?: number }; | |
| }>('describeNode', { | |
| objectId: evalResult.result.objectId, | |
| }); | |
| const backendNodeId = nodeResult.node?.backendNodeId; | |
| if (!backendNodeId) { | |
| return null; | |
| } | |
| return `0-${backendNodeId}`; | |
| } catch (error) { | |
| logger.debug('CSS selector lookup failed:', error); | |
| return null; | |
| } | |
| } | |
| /** | |
| * Find element by XPath and return its EncodedId | |
| */ | |
| private async findElementByXPath(xpath: string, frameOrdinal: number): Promise<string | null> { | |
| try { | |
| const runtimeAgent = this.adapter.runtimeAgent(); | |
| const domAgent = this.adapter.domAgent(); | |
| // Evaluate XPath to find element | |
| const evalResult = await runtimeAgent.invoke<{ | |
| result?: { objectId?: string }; | |
| exceptionDetails?: unknown; | |
| }>('evaluate', { | |
| expression: ` | |
| (function() { | |
| const result = document.evaluate( | |
| ${JSON.stringify(xpath)}, | |
| document, | |
| null, | |
| XPathResult.FIRST_ORDERED_NODE_TYPE, | |
| null | |
| ); | |
| return result.singleNodeValue; | |
| })() | |
| `, | |
| returnByValue: false, | |
| }); | |
| if (!evalResult.result?.objectId) { | |
| return null; | |
| } | |
| // Get backendNodeId from objectId | |
| const nodeResult = await domAgent.invoke<{ | |
| nodeId?: number; | |
| node?: { backendNodeId?: number }; | |
| }>('describeNode', { | |
| objectId: evalResult.result.objectId, | |
| }); | |
| const backendNodeId = nodeResult.node?.backendNodeId; | |
| if (!backendNodeId) { | |
| return null; | |
| } | |
| // Return EncodedId (frame 0 for main frame) | |
| return `${frameOrdinal}-${backendNodeId}`; | |
| } catch (error) { | |
| logger.debug('XPath lookup failed:', error); | |
| return null; | |
| } | |
| } |
| private async executeCachedSelector( | ||
| selectorScript: string, | ||
| adapter: CDPSessionAdapter | ||
| ): Promise<unknown> { | ||
| const runtimeAgent = adapter.runtimeAgent(); | ||
|
|
||
| const result = await runtimeAgent.invoke<{ | ||
| result?: { value?: unknown; type?: string }; | ||
| exceptionDetails?: { text?: string; exception?: { description?: string } }; | ||
| }>('evaluate', { | ||
| expression: selectorScript, | ||
| returnByValue: true, | ||
| awaitPromise: false, | ||
| }); | ||
|
|
||
| if (result.exceptionDetails) { | ||
| const errorMsg = | ||
| result.exceptionDetails.exception?.description || | ||
| result.exceptionDetails.text || | ||
| 'Unknown error'; | ||
| throw new Error(`Selector execution failed: ${errorMsg}`); | ||
| } | ||
|
|
||
| const data = result.result?.value; | ||
| if (data === undefined || data === null) { | ||
| throw new Error('Selector returned no data'); | ||
| } | ||
|
|
||
| return data; | ||
| } |
There was a problem hiding this comment.
Security: executing LLM-generated JS needs a denylist/allowlist (exfiltration + mutation risk).
Right now, a “selector” can still call fetch, open sockets, mutate DOM, etc. Even if prompts say “don’t”, you need a hard gate before Runtime.evaluate.
Minimal mitigation: reject obviously-dangerous APIs before evaluation
private async executeCachedSelector(
selectorScript: string,
adapter: CDPSessionAdapter
): Promise<unknown> {
+ // Minimal safety gate: block common exfiltration / mutation primitives.
+ // (Consider moving this to a shared validator and expanding over time.)
+ const forbidden = /\b(fetch|XMLHttpRequest|WebSocket|EventSource|sendBeacon|navigator\.sendBeacon|postMessage|eval|Function|import\s*\(|document\.cookie|localStorage|sessionStorage)\b/;
+ if (forbidden.test(selectorScript)) {
+ throw new Error('Selector script contains forbidden APIs');
+ }
+
const runtimeAgent = adapter.runtimeAgent();Also applies to: 361-502
| description = 'V0 BASELINE: Gets the accessibility tree of the current page, providing a hierarchical structure of all accessible elements. Simple interface without search, focus, or chunking features.'; | ||
|
|
||
| async execute(args: { reasoning: string }, ctx?: LLMContext): Promise<AccessibilityTreeResult | ErrorResult> { | ||
| try { | ||
| logger.warn(`[V0] Getting accessibility tree: ${args.reasoning}`); | ||
|
|
||
| const adapter = await getAdapter(ctx); | ||
| if (!adapter) { | ||
| return { error: 'No browser connection available' }; | ||
| } | ||
|
|
||
| // Original v0 implementation: just get the full tree, no chunking or search | ||
| const treeResult = await UtilsUniversal.getAccessibilityTree(adapter); | ||
|
|
||
| return { | ||
| simplified: treeResult.simplified, | ||
| idToUrl: treeResult.idToUrl, | ||
| }; | ||
| } catch (error) { | ||
| return { error: `Failed to get accessibility tree: ${String(error)}` }; | ||
| } |
There was a problem hiding this comment.
Align tool output with the V0 agent prompt (xpathMap/tagNameMap).
ActionAgentV0’s system prompt explicitly claims XPath mappings and HTML tag names are available, but this tool currently drops everything except simplified and idToUrl. Either return xpathMap/tagNameMap/iframes if UtilsUniversal.getAccessibilityTree() provides them, or remove those claims from the prompt.
Proposed fix (return available fields)
const treeResult = await UtilsUniversal.getAccessibilityTree(adapter);
return {
simplified: treeResult.simplified,
idToUrl: treeResult.idToUrl,
+ xpathMap: treeResult.xpathMap,
+ tagNameMap: treeResult.tagNameMap,
+ iframes: treeResult.iframes,
};🤖 Prompt for AI Agents
In @front_end/panels/ai_chat/tools/GetAccessibilityTreeToolV0.ts around lines 32
- 52, The tool's execute implementation returns only simplified and idToUrl but
ActionAgentV0 promises xpathMap/tagNameMap/iframes; update the execute method to
include xpathMap, tagNameMap, and iframes (if present) from
UtilsUniversal.getAccessibilityTree() in the returned AccessibilityTreeResult,
or if those keys may be absent, conditionally include them (e.g.,
treeResult.xpathMap, treeResult.tagNameMap, treeResult.iframes) and ensure the
AccessibilityTreeResult type/signature is updated to accept these fields so the
agent prompt is accurate; if you prefer the other route, remove the xpath/tag
claims from ActionAgentV0 instead so the tool and prompt remain consistent.
| // Get strategy | ||
| const strategyType: SearchStrategyType = args.strategy || | ||
| (pattern?.strategy) || | ||
| (getSiteConfig(normalizedSite)?.preferredStrategy) || | ||
| 'xpath-schema'; | ||
|
|
||
| const strategy = getStrategy(strategyType) || getPreferredStrategy(normalizedSite); | ||
| logger.debug(`Using strategy: ${strategy.name}`); | ||
|
|
||
| // Generate pattern if not cached | ||
| if (!pattern) { | ||
| logger.info(`No cached pattern, generating new pattern for ${normalizedSite}`); | ||
|
|
||
| const generationResult = await strategy.generatePattern( | ||
| { | ||
| site: normalizedSite, | ||
| sampleQuery: args.query, | ||
| strategy: strategyType, | ||
| }, | ||
| adapter, | ||
| ctx | ||
| ); | ||
|
|
||
| if (!generationResult.success || !generationResult.pattern) { | ||
| return { | ||
| success: false, | ||
| results: [], | ||
| cached: false, | ||
| error: generationResult.error || 'Failed to generate search pattern', | ||
| }; | ||
| } | ||
|
|
||
| // Save pattern to cache | ||
| pattern = await this.cache.savePattern(generationResult.pattern); | ||
| logger.info(`Saved new pattern for ${normalizedSite}`); | ||
| } | ||
|
|
||
| // Execute pattern to extract results | ||
| const executionResult = await strategy.executePattern( | ||
| { | ||
| pattern, | ||
| query: args.query, | ||
| maxResults, | ||
| }, | ||
| adapter, | ||
| ctx | ||
| ); | ||
|
|
There was a problem hiding this comment.
Prevent cached-pattern / overridden-strategy mismatch.
If pattern is loaded from cache and the caller passes args.strategy, strategyType will switch but pattern remains from the prior strategy, and you still call strategy.executePattern({ pattern, ... }). This can fail subtly depending on how patterns are typed per strategy.
Proposed fix
- // Get strategy
- const strategyType: SearchStrategyType = args.strategy ||
- (pattern?.strategy) ||
- (getSiteConfig(normalizedSite)?.preferredStrategy) ||
- 'xpath-schema';
+ // If caller overrides strategy, don't reuse an incompatible cached pattern
+ if (pattern && args.strategy && pattern.strategy !== args.strategy) {
+ logger.info(`Strategy override (${args.strategy}) differs from cached (${pattern.strategy}); regenerating pattern`);
+ pattern = null;
+ cached = false;
+ }
+
+ // Get strategy
+ const strategyType: SearchStrategyType =
+ args.strategy ||
+ pattern?.strategy ||
+ getSiteConfig(normalizedSite)?.preferredStrategy ||
+ 'xpath-schema';📝 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.
| // Get strategy | |
| const strategyType: SearchStrategyType = args.strategy || | |
| (pattern?.strategy) || | |
| (getSiteConfig(normalizedSite)?.preferredStrategy) || | |
| 'xpath-schema'; | |
| const strategy = getStrategy(strategyType) || getPreferredStrategy(normalizedSite); | |
| logger.debug(`Using strategy: ${strategy.name}`); | |
| // Generate pattern if not cached | |
| if (!pattern) { | |
| logger.info(`No cached pattern, generating new pattern for ${normalizedSite}`); | |
| const generationResult = await strategy.generatePattern( | |
| { | |
| site: normalizedSite, | |
| sampleQuery: args.query, | |
| strategy: strategyType, | |
| }, | |
| adapter, | |
| ctx | |
| ); | |
| if (!generationResult.success || !generationResult.pattern) { | |
| return { | |
| success: false, | |
| results: [], | |
| cached: false, | |
| error: generationResult.error || 'Failed to generate search pattern', | |
| }; | |
| } | |
| // Save pattern to cache | |
| pattern = await this.cache.savePattern(generationResult.pattern); | |
| logger.info(`Saved new pattern for ${normalizedSite}`); | |
| } | |
| // Execute pattern to extract results | |
| const executionResult = await strategy.executePattern( | |
| { | |
| pattern, | |
| query: args.query, | |
| maxResults, | |
| }, | |
| adapter, | |
| ctx | |
| ); | |
| // If caller overrides strategy, don't reuse an incompatible cached pattern | |
| if (pattern && args.strategy && pattern.strategy !== args.strategy) { | |
| logger.info(`Strategy override (${args.strategy}) differs from cached (${pattern.strategy}); regenerating pattern`); | |
| pattern = null; | |
| cached = false; | |
| } | |
| // Get strategy | |
| const strategyType: SearchStrategyType = | |
| args.strategy || | |
| pattern?.strategy || | |
| getSiteConfig(normalizedSite)?.preferredStrategy || | |
| 'xpath-schema'; | |
| const strategy = getStrategy(strategyType) || getPreferredStrategy(normalizedSite); | |
| logger.debug(`Using strategy: ${strategy.name}`); | |
| // Generate pattern if not cached | |
| if (!pattern) { | |
| logger.info(`No cached pattern, generating new pattern for ${normalizedSite}`); | |
| const generationResult = await strategy.generatePattern( | |
| { | |
| site: normalizedSite, | |
| sampleQuery: args.query, | |
| strategy: strategyType, | |
| }, | |
| adapter, | |
| ctx | |
| ); | |
| if (!generationResult.success || !generationResult.pattern) { | |
| return { | |
| success: false, | |
| results: [], | |
| cached: false, | |
| error: generationResult.error || 'Failed to generate search pattern', | |
| }; | |
| } | |
| // Save pattern to cache | |
| pattern = await this.cache.savePattern(generationResult.pattern); | |
| logger.info(`Saved new pattern for ${normalizedSite}`); | |
| } | |
| // Execute pattern to extract results | |
| const executionResult = await strategy.executePattern( | |
| { | |
| pattern, | |
| query: args.query, | |
| maxResults, | |
| }, | |
| adapter, | |
| ctx | |
| ); |
🤖 Prompt for AI Agents
In @front_end/panels/ai_chat/tools/SearchTool.ts around lines 148 - 195, When a
cached pattern exists but the resolved strategyType differs (e.g., args.strategy
overrides) we must avoid executing the wrong strategy with a mismatched pattern;
check pattern.strategy (or pattern.metadata) against strategyType after
computing strategyType and before calling strategy.executePattern, and if they
differ regenerate a new pattern via strategy.generatePattern (then
this.cache.savePattern) or explicitly invalidate the cache entry; ensure
executePattern always receives a pattern produced by the same strategy instance
(use strategy.generatePattern, generationResult.pattern, and
this.cache.savePattern as in the existing generation path).
| async execute(input: TryCachedActionInput, ctx: unknown): Promise<TryCachedActionResult> { | ||
| const context = ctx as { cdpAdapter?: any }; | ||
| const adapter = context.cdpAdapter; | ||
|
|
||
| if (!adapter) { | ||
| logger.warn('No CDP adapter available for cache lookup'); | ||
| return { cached: false, message: 'No CDP adapter, proceed with get_page_content' }; | ||
| } | ||
|
|
||
| try { | ||
| // Get current URL | ||
| const url = await this.getCurrentUrl(adapter); | ||
| if (!url) { | ||
| return { cached: false, message: 'Could not get current URL, proceed with get_page_content' }; | ||
| } | ||
|
|
||
| logger.info(`Checking cache for ${input.semantic_intent} at ${url}`); | ||
|
|
||
| // Look up cached pattern | ||
| const capture = getActionPatternCapture(adapter); | ||
| const lookup = await capture.lookupFromCache(url, input.semantic_intent); | ||
|
|
||
| if (!lookup.found) { | ||
| logger.debug(`Cache MISS for ${input.semantic_intent}`); | ||
| return { cached: false, message: `No cached pattern for "${input.semantic_intent}", proceed with get_page_content` }; | ||
| } | ||
|
|
||
| if (!lookup.encodedId || !lookup.xpathSuccess) { | ||
| logger.debug(`Cache found but XPath failed: ${lookup.error}`); | ||
| return { cached: false, message: `Cached pattern invalid: ${lookup.error}, proceed with get_page_content` }; | ||
| } | ||
|
|
||
| logger.info(`Cache HIT for ${input.semantic_intent}, executing with nodeId ${lookup.encodedId}`); | ||
|
|
||
| // Execute action using cached EncodedId | ||
| const performAction = ToolRegistry.getRegisteredTool('perform_action'); | ||
| if (!performAction) { | ||
| return { cached: true, success: false, message: 'perform_action tool not found', error: 'Tool not found' }; | ||
| } | ||
|
|
||
| const result = await performAction.execute({ | ||
| method: input.method, | ||
| nodeId: lookup.encodedId, | ||
| args: input.args, | ||
| reasoning: input.reasoning || `Using cached pattern for ${input.semantic_intent}`, | ||
| semantic_intent: input.semantic_intent, | ||
| }, ctx as any) as { error?: string; pageChange?: { hasChanges: boolean } }; | ||
|
|
||
| // Update cache stats | ||
| const cache = ActionPatternCache.getInstance(); | ||
| const cacheKey = cache.generateCacheKey(url, input.semantic_intent); | ||
|
|
||
| if (result.error) { | ||
| await cache.recordFailure(cacheKey); | ||
| logger.warn(`Cached action failed: ${result.error}`); | ||
| return { | ||
| cached: true, | ||
| success: false, | ||
| message: `Cached action failed: ${result.error}`, | ||
| error: result.error, | ||
| }; | ||
| } | ||
|
|
||
| await cache.recordSuccess(cacheKey); | ||
| logger.info(`Cached action succeeded for ${input.semantic_intent}`); | ||
|
|
||
| return { | ||
| cached: true, | ||
| success: true, | ||
| message: `Action executed via cache: ${input.semantic_intent}`, | ||
| nodeId: lookup.encodedId, | ||
| }; | ||
| } catch (error) { | ||
| logger.error('Cache lookup/execution error:', error); | ||
| return { | ||
| cached: false, | ||
| message: `Cache error: ${error}, proceed with get_page_content`, | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| private async getCurrentUrl(adapter: any): Promise<string | null> { | ||
| try { | ||
| const result = await adapter.runtimeAgent().invoke('evaluate', { | ||
| expression: 'window.location.href', | ||
| returnByValue: true, | ||
| }) as { result?: { value?: string } }; | ||
| return result?.result?.value || null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# 1) Find the perform_action tool implementation + its execute() return shape
rg -n --type=ts "name\s*=\s*'perform_action'|class\s+.*PerformAction|perform_action" front_end/panels/ai_chat -S
# 2) Find how CDP adapter is modeled on tool contexts
rg -n --type=ts "interface\s+.*Context|cdpAdapter\??:" front_end/panels/ai_chat -SRepository: BrowserOperator/browser-operator-core
Length of output: 11233
🏁 Script executed:
# Get PerformActionTool class definition + execute signature
sed -n '2361,2450p' front_end/panels/ai_chat/tools/Tools.ts
# Get AdapterContext and LLMContext definitions
sed -n '61,70p' front_end/panels/ai_chat/cdp/getAdapter.ts
sed -n '162,175p' front_end/panels/ai_chat/tools/Tools.tsRepository: BrowserOperator/browser-operator-core
Length of output: 3759
🏁 Script executed:
# Find PerformActionResult definition
rg -n "type\s+PerformActionResult|interface\s+PerformActionResult" front_end/panels/ai_chat --type=ts -A 8Repository: BrowserOperator/browser-operator-core
Length of output: 701
🏁 Script executed:
# Get ErrorResult definition
rg -n "interface\s+ErrorResult|type\s+ErrorResult" front_end/panels/ai_chat --type=ts -A 3
# Get full PerformActionResult definition
sed -n '396,415p' front_end/panels/ai_chat/tools/Tools.tsRepository: BrowserOperator/browser-operator-core
Length of output: 1752
🏁 Script executed:
# Check TryCachedActionTool imports
head -30 front_end/panels/ai_chat/tools/TryCachedActionTool.ts
# Check if these types are exported from Tools.ts
grep -n "export.*PerformActionResult\|export.*ErrorResult\|export.*LLMContext" front_end/panels/ai_chat/tools/Tools.tsRepository: BrowserOperator/browser-operator-core
Length of output: 1277
Import and use proper types for ctx parameter and perform_action result to prevent silent type mismatches.
TryCachedActionTool receives ctx: unknown and manually casts it to { cdpAdapter?: any }, and casts the perform_action result to an incomplete type. Import LLMContext, PerformActionResult, and ErrorResult from Tools.js instead:
ctxshould be typed asLLMContext(which extendsAdapterContext)- Result cast should be
PerformActionResult | ErrorResultinstead of the incomplete{ error?: string; pageChange?: { hasChanges: boolean } }
This avoids silently missing fields like summary, added, removed, modified, hasMore, visualCheck, and stateVerification from the actual PerformActionResult structure.
| if (!lookup.encodedId || !lookup.xpathSuccess) { | ||
| logger.debug(`Cache found but XPath failed: ${lookup.error}`); | ||
| return { cached: false, message: `Cached pattern invalid: ${lookup.error}, proceed with get_page_content` }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: rejects valid CSS-fallback cache hits (xpathSuccess === false).
ActionPatternCapture.lookupFromCache() can return { encodedId, xpathSuccess: false } when CSS selector works. This tool treats that as invalid and forces a full flow.
Minimal fix: accept any validated `encodedId`
- if (!lookup.encodedId || !lookup.xpathSuccess) {
+ if (!lookup.encodedId) {
logger.debug(`Cache found but XPath failed: ${lookup.error}`);
return { cached: false, message: `Cached pattern invalid: ${lookup.error}, proceed with get_page_content` };
}🤖 Prompt for AI Agents
In @front_end/panels/ai_chat/tools/TryCachedActionTool.ts around lines 92 - 96,
The code in TryCachedActionTool.ts incorrectly treats a cache hit as invalid
when lookup.xpathSuccess is false even though lookup.encodedId can be valid via
CSS fallback; change the conditional in the lookup handling (the block using
lookup.encodedId and lookup.xpathSuccess) to accept any truthy lookup.encodedId
as a valid cache hit regardless of xpathSuccess, adjust the logger message to
reflect "XPath failed but CSS fallback succeeded" when xpathSuccess is false,
and return { cached: true, message: ... } (or the existing cached-hit payload)
when encodedId exists instead of forcing the full get_page_content flow.
|
Claude finished @tysonthomas9's task —— View job PR Review: Tool improvements and eval CLI ✅Reviewing this large PR with 96 changed files and significant architectural improvements. Todo List:
📋 Overall Assessment: EXCELLENT ⭐This is a substantial and well-engineered improvement to the Browser Operator core. The architectural changes are thoughtful, the implementation is clean, and the testing is comprehensive. ✅ Major Strengths1. CDP Adapter Architecture (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @config/gni/devtools_grd_files.gni:
- Around line 780-817: The gni list incorrectly references many source files
with a .js extension (e.g., DOMToolsRegistration.js,
HybridAccessibilityTreeTool.js, ComposedTreeResolver.js, CDPSessionAdapter.js,
context.js, plus entries under agent_framework and evaluation/test-cases),
causing the build to miss the actual .ts sources; update every listed file name
in this block to use the .ts extension (replace each *.js → *.ts) so entries
like DOMToolsRegistration.ts, HybridAccessibilityTreeTool.ts,
ComposedTreeResolver.ts, CDPSessionAdapter.ts, context.ts and the
agent_framework and test-cases entries match the repository’s TypeScript files.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/gni/devtools_grd_files.gni
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/lib/EvalServer.js : Use Chrome DevTools Protocol (CDP) for direct browser communication including screenshot capture via Page.captureScreenshot, page content via Runtime.evaluate, and tab management via Target.createTarget/closeTarget
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Browser must be started with remote debugging enabled via --remote-debugging-port=9223 with CDP endpoint at http://localhost:9223/json/version
|
Claude finished @tysonthomas9's task —— View job 🔍 PR Review: More Tool Improvements and Eval CLI (#90)Overall Assessment: Well-architected PR adding comprehensive eval CLI system and tool improvements. Strong separation of concerns and robust error handling throughout. ✅ STRENGTHS🏗️ Excellent Architecture
🔧 Robust Implementation
📋 Excellent Test Coverage
|
There was a problem hiding this comment.
Actionable comments posted: 4
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/evaluation/test-cases/action-agent-tests.ts (1)
1279-1281: Criteria text does not match the objective.The objective specifies typing
"Anna", but the criteria at line 1280 says'Typed "a" in the filter field'. This inconsistency could cause confusion during evaluation.Suggested fix
criteria: [ 'Located the search filter input', - 'Typed "a" in the filter field', + 'Typed "Anna" in the filter field', 'List items filtered to show only matching results',front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-tests.ts (1)
7-12: Interface inconsistency with other test files.This
WebTaskAgentArgsdiffers from the iframe and shadow-dom test files:
- Here:
reasoning: string(required),context?: string- Others:
reasoning?: string(optional),context?: Record<string, unknown>This creates a type mismatch. Consider unifying the interface definition.
🤖 Fix all issues with AI agents
In @front_end/panels/ai_chat/dom/EnhancedElementResolver.ts:
- Around line 224-229: The returned field usedShadowPiercer is currently set to
the requested flag ensurePiercer which is misleading because
domAgent.invoke_resolveNode does not actually use the piercer; change the return
to reflect actual usage by either setting usedShadowPiercer: false (since the
piercer isn't used for backend node resolution) or, if you want parity with
resolveWithPiercerEnhanced, call getPiercerStats and set usedShadowPiercer:
stats?.installed === true so the flag reflects whether a piercer was actually
installed; update the return object containing found, objectId
(resolveResponse.object.objectId), backendNodeId and usedShadowPiercer
accordingly.
In @front_end/panels/ai_chat/evaluation/framework/types.ts:
- Around line 20-29: Add a top-level optional skip?: boolean field to the
TestCase interface in scripts/eval-runner/types.ts to match the framework type;
locate the TestCase declaration and insert skip?: boolean alongside existing
properties (e.g., metadata, scripts, etc.) so both interfaces are consistent.
In
@front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-iframe-tests.ts:
- Around line 158-173: The validation criteria in validation.llmJudge.criteria
mismatches the input.task: update the criterion that currently reads "Set
departure city to New York (JFK)" to match the task's departure "Seattle (SEA)"
(e.g., "Set departure city to Seattle (SEA)"); ensure the criteria in
validation.llmJudge.criteria now align with input.task (departure, destination,
and dates) so the llm-judge validation reflects the specified Seattle→Tokyo
search.
🧹 Nitpick comments (5)
front_end/panels/ai_chat/tools/CombinedExtractionTool.ts (2)
15-32: Potential race condition and silent error suppression.Two observations:
Race condition: If
ensureBrowserDeps()is called concurrently before the first call completes, multiple redundant dynamic imports will be triggered sincebrowserDepsLoadedis only set after the await resolves. Consider using a promise-based singleton pattern:Silent catch: The empty catch block (line 28) silently swallows import errors. While expected to fail in Node.js, unexpected failures in the browser environment would be difficult to debug.
♻️ Suggested improvement with deduplication and error logging
let SDK: typeof import('../../../core/sdk/sdk.js') | null = null; let AgentService: typeof import('../core/AgentService.js').AgentService | null = null; -let browserDepsLoaded = false; +let browserDepsPromise: Promise<boolean> | null = null; async function ensureBrowserDeps(): Promise<boolean> { if (isNodeEnvironment) return false; - if (browserDepsLoaded) { - return SDK !== null; - } - try { - const [sdkModule, agentServiceModule] = await Promise.all([ - import('../../../core/sdk/sdk.js'), - import('../core/AgentService.js'), - ]); - SDK = sdkModule; - AgentService = agentServiceModule.AgentService; - browserDepsLoaded = true; // Only set after successful import - } catch { - return false; + if (!browserDepsPromise) { + browserDepsPromise = (async () => { + try { + const [sdkModule, agentServiceModule] = await Promise.all([ + import('../../../core/sdk/sdk.js'), + import('../core/AgentService.js'), + ]); + SDK = sdkModule; + AgentService = agentServiceModule.AgentService; + return true; + } catch (e) { + logger.warn('Failed to load browser dependencies', { error: e }); + return false; + } + })(); } - return SDK !== null; + return browserDepsPromise; }
175-175: Minor: Redundant optional chaining.After the
schema &&guard, the optional chaining inschema?.typeis unnecessary.♻️ Suggested fix
- if (schema && schema?.type === 'object') { + if (schema && schema.type === 'object') {front_end/panels/ai_chat/evaluation/test-cases/action-agent-shadow-dom-tests.ts (2)
7-12: Consider sharingActionAgentArgsinterface.This interface is also defined in other action-agent test files. To maintain consistency and reduce duplication, consider:
- Exporting from a shared types file, or
- Re-exporting from one canonical location
This is a minor maintainability suggestion.
143-144: Test ID gap:action-agent-shadow-004is missing.The test IDs skip from 003 to 005. If this is intentional (e.g., a removed test), consider adding a comment explaining the gap, or renumber to maintain sequence.
front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-shadow-dom-tests.ts (1)
7-11: DuplicateWebTaskAgentArgsinterface.Same interface definition exists in
web-task-agent-iframe-tests.ts. Consider importing from a shared location instead of duplicating.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
front_end/panels/ai_chat/dom/EnhancedElementResolver.tsfront_end/panels/ai_chat/evaluation/framework/types.tsfront_end/panels/ai_chat/evaluation/test-cases/action-agent-shadow-dom-tests.tsfront_end/panels/ai_chat/evaluation/test-cases/action-agent-tests.tsfront_end/panels/ai_chat/evaluation/test-cases/index.tsfront_end/panels/ai_chat/evaluation/test-cases/web-task-agent-iframe-tests.tsfront_end/panels/ai_chat/evaluation/test-cases/web-task-agent-shadow-dom-tests.tsfront_end/panels/ai_chat/evaluation/test-cases/web-task-agent-tests.tsfront_end/panels/ai_chat/tools/CombinedExtractionTool.tsscripts/eval-runner/BrowserExecutor.tsscripts/eval-runner/LLMJudge.tsscripts/eval-runner/cli.tsscripts/eval-runner/lib/ToolSetup.ts
✅ Files skipped from review due to trivial changes (1)
- front_end/panels/ai_chat/evaluation/test-cases/index.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/lib/EvalServer.js : Use Chrome DevTools Protocol (CDP) for direct browser communication including screenshot capture via Page.captureScreenshot, page content via Runtime.evaluate, and tab management via Target.createTarget/closeTarget
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Browser must be started with remote debugging enabled via --remote-debugging-port=9223 with CDP endpoint at http://localhost:9223/json/version
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/lib/EvalServer.js : Use Chrome DevTools Protocol (CDP) for direct browser communication including screenshot capture via Page.captureScreenshot, page content via Runtime.evaluate, and tab management via Target.createTarget/closeTarget
Applied to files:
front_end/panels/ai_chat/tools/CombinedExtractionTool.ts
🧬 Code graph analysis (5)
front_end/panels/ai_chat/tools/CombinedExtractionTool.ts (1)
front_end/panels/ai_chat/core/AgentService.ts (1)
AgentService(143-1451)
front_end/panels/ai_chat/evaluation/test-cases/action-agent-shadow-dom-tests.ts (2)
front_end/panels/ai_chat/evaluation/framework/types.ts (1)
TestCase(12-31)scripts/eval-runner/types.ts (1)
TestCase(56-74)
front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-shadow-dom-tests.ts (5)
front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-iframe-tests.ts (1)
WebTaskAgentArgs(7-11)front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-tests.ts (1)
WebTaskAgentArgs(7-12)front_end/panels/ai_chat/evaluation/test-cases/index.ts (6)
shadowDomFormWorkflowTest(77-77)githubIssueCreationTest(78-78)notionBlockEditingTest(79-79)shadowDomShoppingTest(80-80)shadowDomVideoPlayerTest(81-81)webTaskAgentShadowDOMTests(76-76)front_end/panels/ai_chat/evaluation/framework/types.ts (1)
TestCase(12-31)scripts/eval-runner/types.ts (1)
TestCase(56-74)
front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-iframe-tests.ts (3)
front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-shadow-dom-tests.ts (1)
WebTaskAgentArgs(7-11)front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-tests.ts (1)
WebTaskAgentArgs(7-12)front_end/panels/ai_chat/evaluation/framework/types.ts (1)
TestCase(12-31)
front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-tests.ts (3)
front_end/panels/ai_chat/evaluation/framework/types.ts (1)
TestCase(12-31)front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-iframe-tests.ts (1)
WebTaskAgentArgs(7-11)front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-shadow-dom-tests.ts (1)
WebTaskAgentArgs(7-11)
⏰ 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 (28)
front_end/panels/ai_chat/tools/CombinedExtractionTool.ts (3)
7-8: Environment detection looks correct.Standard pattern for distinguishing browser from Node.js environments.
109-114: API key fallback logic is well-structured.Correctly prioritizes context-provided API key and safely falls back to AgentService only when in a browser environment with the dependency loaded.
157-163: SDK check is appropriate for TypeScript narrowing.While
ensureBrowserDeps()returningtrueimpliesSDK !== null, the explicit|| !SDKcheck is necessary for TypeScript to narrow the type and allow safe access on line 164. Acceptable defensive pattern.front_end/panels/ai_chat/evaluation/test-cases/action-agent-tests.ts (10)
137-141: LGTM - Product URL with appropriate flaky handling.The specific product URL could become unavailable over time, but the test is already marked with
flaky: trueand generous timeout/retries, which is appropriate for e-commerce tests.
578-610: LGTM - Clear test focus on calendar UI navigation.Good update to clarify the test's purpose. The objective now explicitly tests month navigation and day selection, which better validates calendar widget interaction capabilities.
612-643: LGTM - Good complementary test.This test provides good coverage for direct date input as opposed to calendar UI navigation. Using a past date is fine for testing form fill capabilities.
679-681: LGTM - Reasonable timeout increase.Two minutes is appropriate for complex date range picker interactions that require multiple navigation steps.
762-802: LGTM - Improved modal test reliability.Using
entry_adpage provides a more predictable modal scenario. ThewaitAfterNavigation: 2000is a thoughtful addition to ensure the modal appears before the test executes.
843-879: LGTM - Smart workaround for JS alert blocking.Switching to a DOM-change-based right-click test avoids the main thread blocking issue with JS alerts while still validating context menu functionality.
1303-1339: LGTM - Useful complementary test.This test extends the existing
loginFormTestby adding the submit action. Testing both form fill and form fill + submit provides good coverage of different interaction patterns.
1341-1376: LGTM - Simple checkbox test addition.This provides a straightforward checkbox test without iframe complexity, complementing the existing
checkboxActionTest(w3schools iframe) andtoggleCheckboxTest(httpbin form).
1378-1436: LGTM - Test array properly updated.All new tests are correctly added to the
actionAgentTestsarray with clear organization comments.
1050-1062: LGTM - Simplified validation criteria.The relaxed criteria focusing on action completion and page change detection (rather than specific visual outcomes) makes these tests more resilient to UI changes on third-party test sites.
front_end/panels/ai_chat/dom/EnhancedElementResolver.ts (3)
54-94: LGTM!Clean implementation with proper error handling and consistent result structure. The piercer usage tracking based on
stats?.installedis correct.
104-168: LGTM!Good implementation with proper frame registry caching support and clear documentation of the OOPIF limitation. The error messages provide useful context for debugging.
243-267: LGTM!Smart dispatcher with correct detection order—EncodedId pattern is checked before bare numeric to avoid false matches. The omission of
optionsin theresolveByEncodedIdcall is appropriate since that function doesn't use piercer injection.front_end/panels/ai_chat/evaluation/test-cases/action-agent-shadow-dom-tests.ts (3)
23-58: LGTM!Well-structured test case with clear objectives, appropriate validation criteria, and visual verification prompts. The MDN web components example is a stable test target.
222-229: LGTM!The exported array properly aggregates all shadow DOM action tests.
65-69: No issues found. Thefixture://protocol handler is properly implemented inscripts/eval-runner/TestRunner.ts(lines 177-196), and the fixture fileshadow-dom-closed.htmlexists atfront_end/panels/ai_chat/testing/fixtures/. The test configuration is valid.front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-iframe-tests.ts (2)
65-103: LGTM - Properly skipped placeholder test.The test uses a fake URL and is correctly marked with
skip: truewith a clear comment explaining the reason.
413-427: LGTM!Export arrays are well-organized, separating iframe tests from hybrid snapshot tests for flexible test selection.
front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-shadow-dom-tests.ts (3)
21-56: LGTM!Well-designed test case using Shoelace's official documentation as a stable shadow DOM test target. Validation criteria appropriately cover the shadow DOM interaction workflow.
62-99: LGTM - Properly skipped auth-required test.Test correctly marked with
skip: trueand documented reason (requires GitHub authentication).
227-234: LGTM!Export array properly aggregates all shadow DOM web task tests.
front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-tests.ts (4)
330-386: LGTM - Well-updated job search test.Task and criteria properly aligned for Indeed.com. The description and validation criteria accurately reflect the updated target site.
749-757: LGTM - Appropriate skip for auth-required test.Twitter/X now requires authentication, and the test is correctly marked with
skip: truewith a clear comment.
867-921: LGTM - Good site-specific constraint.The "CRITICAL" note in criteria (line 898) appropriately emphasizes staying on Medium.com to prevent the agent from substituting other news sites.
1114-1143: LGTM!Export array properly aggregates all web task agent tests with clear categorization comments.
| export interface WebTaskAgentArgs { | ||
| task: string; | ||
| reasoning?: string; | ||
| context?: Record<string, unknown>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate WebTaskAgentArgs interface with inconsistent signatures.
This interface is defined in three files with differences:
- This file & shadow-dom-tests:
reasoning?: string,context?: Record<string, unknown> - web-task-agent-tests.ts:
reasoning: string(required),context?: string,extraction_schema?: object
This inconsistency could cause confusion and type errors. Consider consolidating into a single shared interface.
#!/bin/bash
# Find all WebTaskAgentArgs definitions
ast-grep --pattern 'export interface WebTaskAgentArgs {
$$$
}'| input: { | ||
| task: 'Search for a round-trip flight from Seattle (SEA) to Tokyo (NRT), departing March 20, 2026 and returning March 30, 2026, for 1 adult passenger', | ||
| reasoning: 'Testing real-world airline booking with complex iframe and widget structure', | ||
| }, | ||
| validation: { | ||
| type: 'llm-judge', | ||
| llmJudge: { | ||
| criteria: [ | ||
| 'Located the flight search widget', | ||
| 'Selected round-trip option', | ||
| 'Set departure city to New York (JFK)', | ||
| 'Set destination city to Tokyo (NRT)', | ||
| 'Set travel dates for next month', | ||
| 'Initiated the flight search', | ||
| 'Search results were displayed', | ||
| ], |
There was a problem hiding this comment.
Task/criteria mismatch: Seattle vs New York.
The task specifies Seattle (SEA) to Tokyo (NRT), but the validation criteria reference New York (JFK):
- Line 159:
"from Seattle (SEA) to Tokyo (NRT)" - Line 168:
"Set departure city to New York (JFK)"
This appears to be a copy-paste error that will cause validation failures.
🐛 Proposed fix
criteria: [
'Located the flight search widget',
'Selected round-trip option',
- 'Set departure city to New York (JFK)',
+ 'Set departure city to Seattle (SEA)',
'Set destination city to Tokyo (NRT)',
'Set travel dates for next month',🤖 Prompt for AI Agents
In
@front_end/panels/ai_chat/evaluation/test-cases/web-task-agent-iframe-tests.ts
around lines 158 - 173, The validation criteria in validation.llmJudge.criteria
mismatches the input.task: update the criterion that currently reads "Set
departure city to New York (JFK)" to match the task's departure "Seattle (SEA)"
(e.g., "Set departure city to Seattle (SEA)"); ensure the criteria in
validation.llmJudge.criteria now align with input.task (departure, destination,
and dates) so the llm-judge validation reflects the specified Seattle→Tokyo
search.
|
Claude finished @tysonthomas9's task —— View job PR #90 Code Review - Final SummaryReview Checklist
🎯 OVERALL ASSESSMENT: EXCELLENTThis PR represents a major architectural advancement for Browser Operator with exceptional engineering quality. The changes are well-designed, thoroughly tested, and production-ready. ⭐ MAJOR STRENGTHS1. Architectural Excellence
2. Innovation in Browser Automation
3. Production-Grade Quality
4. Developer Experience
🔍 DETAILED ANALYSIS
💡 MINOR RECOMMENDATIONS
🏆 CONCLUSIONThis PR sets a new standard for browser automation frameworks. The combination of:
Makes this a standout contribution that significantly advances the state of the art. Recommendation: ✅ APPROVE with enthusiasm The code quality is exceptional, the architecture is sound, and the testing is comprehensive. This PR is ready for production deployment. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.ts:
- Around line 40-41: The module-level Map lastActionMap storing LastActionInfo
per session has no cleanup and can leak memory; add eviction/cleanup by (a)
exposing a clearSessionActionInfo(sessionId: string) to delete entries when
sessions end (reference lastActionMap and LastActionInfo), (b) implementing a
capped-size policy (e.g., MAX_SESSION_ENTRIES) with FIFO/LRU eviction inside any
setter like setLastAction(sessionId, info), or (c) adding TTL-based eviction
where entries store timestamp and a periodic sweeper removes stale session keys;
implement one or more of these strategies and ensure all code paths that update
lastActionMap use the chosen setter/cleanup API.
- Around line 276-291: The code uses a non-null assertion on callCtx.provider
when building llmContext for navigateTool.execute, which can throw if provider
is undefined; update ActionAgentV2 to first check for callCtx.provider (e.g., if
(!callCtx.provider) { /* handle: return, throw, or log and skip navigation */ })
before constructing llmContext and calling navigateTool.execute, or else supply
a safe fallback value for the provider field; modify the block around
navigateTool and llmContext so navigateTool.execute is only called when provider
is present.
In @front_end/panels/ai_chat/BUILD.gn:
- Around line 187-189: Add the missing barrel file "a11y/index.ts" to the
ai_chat build sources: insert "a11y/index.ts" into the
devtools_module("ai_chat") sources list alongside "a11y/FrameRegistry.ts",
"a11y/HybridSnapshot.ts", and "a11y/HybridSnapshotUniversal.ts", and also add
"a11y/index.ts" into the _ai_chat_sources list so it matches how "dom/index.ts"
and "cdp/index.ts" are included; update both lists accordingly to ensure the
barrel is compiled with the module.
🧹 Nitpick comments (5)
front_end/panels/ai_chat/BUILD.gn (1)
689-691: Test file path inconsistency.
common/EncodedId.test.tsdoesn't follow the__tests__subdirectory convention used by all other tests in this file (e.g.,dom/__tests__/ComposedTreeResolver.test.ts,a11y/__tests__/FrameRegistry.test.ts). Thecommon/__tests__/directory already exists in the project structure.Consider moving the test file to
common/__tests__/EncodedId.test.tsand updating the BUILD.gn entry accordingly for consistency with the project structure.Suggested change
"dom/__tests__/ComposedTreeResolver.test.ts", - "common/EncodedId.test.ts", + "common/__tests__/EncodedId.test.ts", "a11y/__tests__/FrameRegistry.test.ts",front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts (1)
143-193: Consider extracting shared auto-navigation logic.The
beforeExecutehook for chrome:// URL detection and auto-navigation is nearly identical across ActionAgent, ActionAgentV1, and ActionAgentV2. This duplication increases maintenance burden.Consider extracting a shared utility function:
♻️ Suggested refactor
Create a shared helper in a common location:
// e.g., in agents/utils/chromeUrlNavigation.ts export async function autoNavigateFromChromeUrl(callCtx: CallCtx): Promise<void> { const adapter = callCtx.cdpAdapter; if (!adapter) { return; } try { const urlResult = await adapter.runtimeAgent().invoke<{result?: {value?: string}}>('evaluate', { expression: 'window.location.href', returnByValue: true, }); const currentUrl = urlResult?.result?.value as string; if (currentUrl && currentUrl.startsWith('chrome://')) { const navigateTool = ToolRegistry.getRegisteredTool('navigate_url'); if (navigateTool && callCtx.provider) { const llmContext = buildLLMContextFromCallCtx(callCtx); await navigateTool.execute({ url: 'https://google.com', reasoning: 'Auto-navigation from chrome:// URL' }, llmContext); } } } catch (error) { // Log and continue } }front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV1.ts (1)
195-245: Duplicate code: beforeExecute is nearly identical to ActionAgent.ts.This hook duplicates the chrome:// auto-navigation logic from ActionAgent.ts. As noted in the ActionAgent.ts review, consider extracting this into a shared utility to reduce maintenance burden and ensure consistent behavior across agent variants.
front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts (1)
70-70: Clarify tool version naming convention.The mapping appears intentional but could be confusing:
get_page_content→GetAccessibilityTreeToolV0(baseline)get_page_content_v1→GetAccessibilityTreeTool(enhanced, despite class not having V1 suffix)Consider adding a brief comment explaining the versioning scheme, or renaming the enhanced class to
GetAccessibilityTreeToolV1for consistency.Also applies to: 130-130
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.ts (1)
375-385: Consider usinggetCurrentUrlhelper inbeforeExecute.The
beforeExecutehook duplicates the URL retrieval logic that's already extracted intogetCurrentUrl. Consider reusing this helper to reduce code duplication within the same file.♻️ Example usage in beforeExecute
// Auto-navigate away from chrome:// URLs - try { - const urlResult = await adapter.runtimeAgent().invoke<{result?: {value?: string}}>('evaluate', { - expression: 'window.location.href', - returnByValue: true, - }); - - const currentUrl = urlResult?.result?.value as string; + try { + const currentUrl = await getCurrentUrl(adapter); if (currentUrl && currentUrl.startsWith('chrome://')) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
front_end/panels/ai_chat/BUILD.gnfront_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.tsfront_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.tsfront_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV1.tsfront_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.tsfront_end/panels/ai_chat/tools/GetAccessibilityTreeToolV0.tsfront_end/panels/ai_chat/tools/Tools.tsscripts/eval-runner/AgentBridge.tsscripts/eval-runner/lib/ToolSetup.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- front_end/panels/ai_chat/tools/GetAccessibilityTreeToolV0.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/lib/EvalServer.js : Use Chrome DevTools Protocol (CDP) for direct browser communication including screenshot capture via Page.captureScreenshot, page content via Runtime.evaluate, and tab management via Target.createTarget/closeTarget
Applied to files:
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts
📚 Learning: 2025-12-07T00:27:56.465Z
Learnt from: CR
Repo: BrowserOperator/browser-operator-core PR: 0
File: agent-server/nodejs/CLAUDE.md:0-0
Timestamp: 2025-12-07T00:27:56.465Z
Learning: Applies to agent-server/nodejs/src/api-server.js : POST /page/execute endpoint must accept clientId, tabId, expression, returnByValue, and awaitPromise; use CDP Runtime.evaluate and return result with type and value
Applied to files:
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts
🧬 Code graph analysis (2)
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV1.ts (3)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(322-324)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (4)
AgentToolConfig(112-229)ConfigurableAgentArgs(293-308)CallCtx(22-48)ToolRegistry(234-288)front_end/panels/ai_chat/core/Constants.ts (1)
MODEL_SENTINELS(81-84)
front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts (3)
front_end/panels/ai_chat/tools/DOMToolsRegistration.ts (1)
registerDOMTools(24-48)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)
ToolRegistry(234-288)ConfigurableAgentTool(358-633)front_end/panels/ai_chat/tools/GetAccessibilityTreeToolV0.ts (1)
GetAccessibilityTreeToolV0(30-65)
⏰ 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 (14)
front_end/panels/ai_chat/BUILD.gn (3)
168-189: LGTM! New module additions are properly synchronized.The new
tools/,dom/,cdp/, anda11y/source files are correctly added to both thedevtools_module("ai_chat")sources list and the_ai_chat_sourcesmetadata list, ensuring the build and release GRD check will include all files.Also applies to: 459-480
238-245: LGTM! Comprehensive test case coverage for new features.The new evaluation test cases for shadow DOM (
action-agent-shadow-dom-tests.ts,web-task-agent-shadow-dom-tests.ts), iframes (action-agent-iframe-tests.ts,web-task-agent-iframe-tests.ts), and CDP tools (cdp-tool-tests.ts) properly cover the multi-frame accessibility and DOM traversal capabilities introduced in this PR.
250-259: LGTM! Well-organized utility additions.The new common utilities (
utils-universal.ts,xpath-builder.ts,geometry-helpers.ts,mouse-helpers.ts,accessibility-tree-search.ts) provide foundational support for the frame-aware accessibility and DOM traversal features.front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts (1)
41-44: LGTM! Documentation updates for nodeId format and setValue.The updates correctly reflect the EncodedId format (string-based like "0-123") and add comprehensive examples for slider/range inputs using
setValue.Also applies to: 89-92
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV1.ts (3)
1-10: LGTM! Standard imports and logger setup.The imports are correctly organized and the logger is properly named for this V1 variant.
91-99: Good addition of frame ordinal documentation.The explanation of EncodedId format with frame prefixes (e.g., "1-456" for frame 1) is valuable for cross-frame element targeting. This helps the LLM understand the ID structure when working with iframes.
144-151: LGTM! Consistent tool configuration.The tools array correctly uses
get_page_content_v1and the handoffs configuration properly includes it inincludeToolResults, maintaining consistency.Also applies to: 188-194
front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts (3)
9-9: LGTM! New imports are correctly organized.The new tool and agent imports are properly grouped with their related imports and follow the existing file structure.
Also applies to: 14-14, 19-21, 32-33, 45-45
56-58: LGTM! DOM tools registration order is appropriate.The
registerDOMTools()call is correctly placed afterinitializeMiniApps()with a clear comment explaining its purpose.
132-142: LGTM! Agent registrations maintain backward compatibility.All three action agent variants (base, V1, V2) are registered, allowing existing consumers to continue using
action_agentwhile new features can leverage the V1/V2 variants.front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.ts (4)
1-16: LGTM! Imports and setup are correct.The imports properly include the new action cache utilities and follow the established patterns.
297-367: LGTM! afterExecute logic for pattern capture is well-structured.The hook correctly:
- Builds a map of tool results by ID
- Finds the last successful
perform_actionwithsemantic_intent- Captures the XPath pattern for caching
- Handles errors gracefully
391-405: LGTM! recordPerformAction correctly captures action metadata.The function properly stores the action info only when
semantic_intentis present. Note that this function populates thelastActionMapwhich was flagged earlier for lacking cleanup.
448-453: No issue found - minimal context is sufficient for perform_action.The
perform_actiontool's context requirement is satisfied by providing onlycdpAdapter. ThegetAdapter()function that processes the context parameter only checks forcdpAdapterand does not useprovider,apiKey, or other fields fromLLMContext. The implementation correctly passes the minimal required context.
| // Store last action info per session for caching | ||
| const lastActionMap = new Map<string, LastActionInfo>(); |
There was a problem hiding this comment.
Potential memory leak: lastActionMap has no cleanup mechanism.
The module-level Map stores per-session action info but has no eviction or cleanup logic. If sessions accumulate without being cleaned, memory usage will grow unbounded.
Consider:
- Adding a TTL-based eviction
- Limiting Map size with LRU semantics
- Clearing entries when sessions complete
🔧 Suggested approach
// Option 1: Clear on session end (if you have session lifecycle hooks)
export function clearSessionActionInfo(sessionId: string): void {
lastActionMap.delete(sessionId);
}
// Option 2: Simple size limit with FIFO eviction
const MAX_SESSION_ENTRIES = 100;
function setLastAction(sessionId: string, info: LastActionInfo): void {
if (lastActionMap.size >= MAX_SESSION_ENTRIES) {
const firstKey = lastActionMap.keys().next().value;
lastActionMap.delete(firstKey);
}
lastActionMap.set(sessionId, info);
}🤖 Prompt for AI Agents
In
@front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.ts
around lines 40 - 41, The module-level Map lastActionMap storing LastActionInfo
per session has no cleanup and can leak memory; add eviction/cleanup by (a)
exposing a clearSessionActionInfo(sessionId: string) to delete entries when
sessions end (reference lastActionMap and LastActionInfo), (b) implementing a
capped-size policy (e.g., MAX_SESSION_ENTRIES) with FIFO/LRU eviction inside any
setter like setLastAction(sessionId, info), or (c) adding TTL-based eviction
where entries store timestamp and a periodic sweeper removes stale session keys;
implement one or more of these strategies and ensure all code paths that update
lastActionMap use the chosen setter/cleanup API.
| if (navigateTool) { | ||
| 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, | ||
| cdpAdapter: callCtx.cdpAdapter | ||
| }; | ||
| await navigateTool.execute({ | ||
| url: 'https://google.com', | ||
| reasoning: 'Auto-navigation from chrome:// URL' | ||
| }, llmContext); | ||
| } |
There was a problem hiding this comment.
Missing provider null check before chrome:// navigation.
Unlike ActionAgent and ActionAgentV1, this implementation uses a non-null assertion (callCtx.provider!) at line 279 without first checking if provider is available. This could cause a runtime error if provider is undefined.
🐛 Proposed fix
const navigateTool = ToolRegistry.getRegisteredTool('navigate_url');
- if (navigateTool) {
+ if (navigateTool && callCtx.provider) {
const llmContext = {
apiKey: callCtx.apiKey,
- provider: callCtx.provider!,
+ provider: callCtx.provider,
model: callCtx.model || callCtx.mainModel || '',
getVisionCapability: callCtx.getVisionCapability,
miniModel: callCtx.miniModel,
nanoModel: callCtx.nanoModel,
abortSignal: callCtx.abortSignal,
cdpAdapter: callCtx.cdpAdapter
};
await navigateTool.execute({
url: 'https://google.com',
reasoning: 'Auto-navigation from chrome:// URL'
}, llmContext);
+ } else if (!callCtx.provider) {
+ logger.warn('Provider not available for auto-navigation, skipping');
}📝 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.
| if (navigateTool) { | |
| 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, | |
| cdpAdapter: callCtx.cdpAdapter | |
| }; | |
| await navigateTool.execute({ | |
| url: 'https://google.com', | |
| reasoning: 'Auto-navigation from chrome:// URL' | |
| }, llmContext); | |
| } | |
| if (navigateTool && callCtx.provider) { | |
| 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, | |
| cdpAdapter: callCtx.cdpAdapter | |
| }; | |
| await navigateTool.execute({ | |
| url: 'https://google.com', | |
| reasoning: 'Auto-navigation from chrome:// URL' | |
| }, llmContext); | |
| } else if (!callCtx.provider) { | |
| logger.warn('Provider not available for auto-navigation, skipping'); | |
| } |
🤖 Prompt for AI Agents
In
@front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgentV2.ts
around lines 276 - 291, The code uses a non-null assertion on callCtx.provider
when building llmContext for navigateTool.execute, which can throw if provider
is undefined; update ActionAgentV2 to first check for callCtx.provider (e.g., if
(!callCtx.provider) { /* handle: return, throw, or log and skip navigation */ })
before constructing llmContext and calling navigateTool.execute, or else supply
a safe fallback value for the provider field; modify the block around
navigateTool and llmContext so navigateTool.execute is only called when provider
is present.
| "a11y/FrameRegistry.ts", | ||
| "a11y/HybridSnapshot.ts", | ||
| "a11y/HybridSnapshotUniversal.ts", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if a11y/index.ts exists in the repository
echo "=== Searching for a11y/index.ts file ==="
find . -name "index.ts" -path "*/a11y/*" 2>/dev/null
echo ""
echo "=== Searching more broadly for a11y directory with index.ts ==="
fd "index\.ts" | grep a11y
echo ""
echo "=== Checking if a11y directory exists ==="
find . -type d -name "a11y" 2>/dev/null | head -20
echo ""
echo "=== Checking all TypeScript files in a11y directories ==="
find . -type d -name "a11y" -exec find {} -name "*.ts" \; 2>/dev/null | head -30Repository: BrowserOperator/browser-operator-core
Length of output: 663
🏁 Script executed:
#!/bin/bash
# Check for imports from a11y barrel/index
echo "=== Checking for imports from a11y barrel ==="
rg "from ['\"].*a11y['\"]" --type ts -A 1 | head -30
echo ""
echo "=== Checking for imports from a11y/index ==="
rg "from ['\"].*a11y/index['\"]" --type ts -A 1
echo ""
echo "=== Viewing a11y/index.ts content ==="
cat "front_end/panels/ai_chat/a11y/index.ts"
echo ""
echo "=== Checking dom/index.ts for comparison ==="
cat "front_end/panels/ai_chat/dom/index.ts" | head -30
echo ""
echo "=== Checking cdp/index.ts for comparison ==="
cat "front_end/panels/ai_chat/cdp/index.ts" | head -30Repository: BrowserOperator/browser-operator-core
Length of output: 2382
🏁 Script executed:
#!/bin/bash
# Check the BUILD.gn structure around the sources sections
echo "=== Viewing BUILD.gn devtools_module sources around lines 175-195 ==="
sed -n '175,195p' "front_end/panels/ai_chat/BUILD.gn"
echo ""
echo "=== Checking if _ai_chat_sources list exists and needs sync ==="
sed -n '300,330p' "front_end/panels/ai_chat/BUILD.gn" | head -40
echo ""
echo "=== Checking for any duplicates of a11y files in _ai_chat_sources ==="
rg -n "a11y/" "front_end/panels/ai_chat/BUILD.gn"Repository: BrowserOperator/browser-operator-core
Length of output: 2280
Add missing a11y/index.ts to BUILD.gn sources.
The a11y/index.ts barrel file exists and should be included in both the devtools_module("ai_chat") sources (after line 189) and the _ai_chat_sources list (after line 480), consistent with how dom/index.ts (line 180) and cdp/index.ts (line 185) are included.
🤖 Prompt for AI Agents
In @front_end/panels/ai_chat/BUILD.gn around lines 187 - 189, Add the missing
barrel file "a11y/index.ts" to the ai_chat build sources: insert "a11y/index.ts"
into the devtools_module("ai_chat") sources list alongside
"a11y/FrameRegistry.ts", "a11y/HybridSnapshot.ts", and
"a11y/HybridSnapshotUniversal.ts", and also add "a11y/index.ts" into the
_ai_chat_sources list so it matches how "dom/index.ts" and "cdp/index.ts" are
included; update both lists accordingly to ensure the barrel is compiled with
the module.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.