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 (
|
📝 WalkthroughWalkthroughAdds a cross-environment CDP adapter layer, frame-aware hybrid accessibility snapshots with EncodedId addressing, shadow-DOM/composed-tree resolution and tooling, runtime/agent runtime changes (runtime IDs/timestamps, pre-tool hooks), many new tools/caches, extensive evaluation test suites, and a full eval-runner CLI + browser executor. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Tool/Agent
participant GetAdapter as getAdapter()
participant Adapter as CDPSessionAdapter
participant SDKTarget as SDKTargetAdapter / DirectCDPAdapter
participant Browser as Browser Target
Client->>GetAdapter: getAdapter(ctx?)
alt ctx.cdpAdapter provided
GetAdapter->>Client: return ctx.cdpAdapter
else browser deps available
GetAdapter->>SDKTarget: create SDKTargetAdapter(primaryPageTarget)
SDKTarget->>Adapter: return SDKTargetAdapter
GetAdapter->>Client: return Adapter
else node/no-browser
GetAdapter->>Client: return null or DirectCDPAdapter (if provided)
end
Client->>Adapter: adapter.runtimeAgent().invoke(...) / adapter.domAgent().invoke(...)
sequenceDiagram
participant Caller as captureHybridSnapshot()
participant FrameReg as FrameRegistryUniversal / FrameRegistry
participant DOM as DOM Traversal (utils-universal)
participant A11y as Accessibility fetcher (AX)
participant Maps as XPath/URL maps
Caller->>FrameReg: collectFrames()
FrameReg->>FrameReg: DFS frame tree, assign ordinals
FrameReg->>Caller: frames list
Caller->>DOM: build DOM maps per frame (pierce shadow optional)
DOM->>Maps: populate xpathMap, tagNameMap, scrollableMap
Caller->>A11y: getAccessibilityTree per frame
A11y->>Maps: associate encodedIds -> URLs
Caller->>Caller: merge per-frame maps -> HybridSnapshot
Caller->>Caller: expose resolveEncodedIdToXPath/URL
sequenceDiagram
participant User as Caller
participant Resolver as ElementResolver / EnhancedElementResolver
participant Piercer as ShadowPiercer
participant Composed as ComposedTreeResolver
participant Adapter as CDPSessionAdapter / Target
User->>Resolver: resolveElement(target, selector, options)
alt ensure piercer
Resolver->>Piercer: ensurePiercerInjected(target)
Piercer->>Adapter: inject SHADOW_PIERCER_RUNTIME
Piercer->>Resolver: piercer installed
end
Resolver->>Composed: resolveSelector (hop notation / xpath / css)
Composed->>Adapter: runtime evaluate / DOM traversal
Adapter->>Resolver: return objectId + backendNodeId
Resolver->>User: ElementResolutionResult (objectId/backendNodeId/usedPiercer)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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. 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".
| 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 👍 / 👎.
| 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.
| /** | ||
| * Test filling a form - simplified to use standard login form. | ||
| * Uses the-internet.herokuapp.com which provides stable test pages. | ||
| */ | ||
| export const shadowFormFillTest: TestCase<ActionAgentArgs> = { | ||
| id: 'action-agent-shadow-004', | ||
| name: 'Fill login form', | ||
| description: 'Fill out a login form with username and password', | ||
| url: 'https://the-internet.herokuapp.com/login', | ||
| tool: 'action_agent', | ||
| input: { | ||
| objective: 'Fill the username field with "tomsmith" and the password field with "SuperSecretPassword!" then click the Login button', | ||
| reasoning: 'Testing form fill and submit capability', | ||
| hint: 'The form has username and password input fields. These are the valid credentials for this test page.', | ||
| }, | ||
| validation: { | ||
| type: 'llm-judge', | ||
| llmJudge: { | ||
| criteria: [ | ||
| 'Located the username input field', | ||
| 'Username field was filled with "tomsmith"', | ||
| 'Password field was filled with "SuperSecretPassword!"', | ||
| 'Login button was clicked and page changed', | ||
| ], | ||
| visualVerification: { | ||
| enabled: true, | ||
| captureBeforeAction: true, | ||
| captureAfterAction: true, | ||
| verificationPrompts: [ | ||
| 'Verify the form fields were filled correctly', | ||
| 'Confirm the login was successful', | ||
| 'Check for success message or secure area page', | ||
| ], | ||
| }, | ||
| }, | ||
| }, | ||
| metadata: { | ||
| tags: ['action', 'form', 'fill', 'input', 'login', 'the-internet'], | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Test misplaced in shadow DOM suite.
This test (shadowFormFillTest) doesn't involve shadow DOM interaction. The URL points to a standard login form on the-internet.herokuapp.com, and the metadata tags don't include 'shadow-dom'. This appears to be a standard form-filling test that should be in a general action agent test suite, not in shadow DOM tests.
Consider moving this test to a more appropriate suite or updating it to actually test shadow DOM form interaction.
| /** | ||
| * 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.
| async function ensureBrowserDeps(): Promise<boolean> { | ||
| if (isNodeEnvironment) return false; | ||
| if (!browserDepsLoaded) { | ||
| browserDepsLoaded = true; | ||
| try { | ||
| const [sdkModule, agentServiceModule] = await Promise.all([ | ||
| import('../../../core/sdk/sdk.js'), | ||
| import('../core/AgentService.js'), | ||
| ]); | ||
| SDK = sdkModule; | ||
| AgentService = agentServiceModule.AgentService; | ||
| } catch { return false; } | ||
| } | ||
| return SDK !== null; | ||
| } |
There was a problem hiding this comment.
Race condition: browserDepsLoaded set before imports complete.
Setting browserDepsLoaded = true on line 18 before the imports succeed means that if Promise.all fails, subsequent calls to ensureBrowserDeps() will return SDK !== null (which is false) without retrying the imports.
🔎 Proposed fix
async function ensureBrowserDeps(): Promise<boolean> {
if (isNodeEnvironment) return false;
- if (!browserDepsLoaded) {
- browserDepsLoaded = true;
- try {
- const [sdkModule, agentServiceModule] = await Promise.all([
- import('../../../core/sdk/sdk.js'),
- import('../core/AgentService.js'),
- ]);
- SDK = sdkModule;
- AgentService = agentServiceModule.AgentService;
- } catch { 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;
+ } catch {
+ return false;
+ }
return SDK !== null;
}🤖 Prompt for AI Agents
In front_end/panels/ai_chat/tools/CombinedExtractionTool.ts around lines 15 to
29, the function sets browserDepsLoaded before the dynamic imports complete
which introduces a race where failed imports prevent retries; change the logic
so browserDepsLoaded is only set true after the imports and assignments succeed
(or ensure it is reset to false inside the catch), await Promise.all and then
assign SDK and AgentService, and in the catch return false (or rethrow) so
subsequent calls will retry the imports.
| content.push({ | ||
| type: 'image_url', | ||
| image_url: { url: `data:image/png;base64,${imageData}` }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Anthropic API uses different image format than OpenAI.
The image_url format with { url: "data:..." } is OpenAI-specific. Anthropic's Messages API expects { type: "image", source: { type: "base64", media_type: "image/png", data: "..." } }. This will fail when using Anthropic with screenshots.
🔎 Proposed fix for Anthropic image format
private async buildMessages(
prompt: string,
screenshots: { beforeScreenshot?: string; afterScreenshot?: string }
): Promise<any[]> {
const content: any[] = [{ type: 'text', text: prompt }];
+ const isAnthropic = this.config.provider === 'anthropic';
// Add screenshots if visual verification is enabled
if (screenshots.beforeScreenshot && fs.existsSync(screenshots.beforeScreenshot)) {
const imageData = fs.readFileSync(screenshots.beforeScreenshot).toString('base64');
content.push({
type: 'text',
text: '\n\n## Before Screenshot (state before action):',
});
- content.push({
- type: 'image_url',
- image_url: { url: `data:image/png;base64,${imageData}` },
- });
+ content.push(isAnthropic ? {
+ type: 'image',
+ source: { type: 'base64', media_type: 'image/png', data: imageData },
+ } : {
+ type: 'image_url',
+ image_url: { url: `data:image/png;base64,${imageData}` },
+ });
}Also applies to: 174-178
🤖 Prompt for AI Agents
In scripts/eval-runner/LLMJudge.ts around lines 162-166 (and similarly 174-178),
the code builds an OpenAI-style image payload ({ type: 'image_url', image_url: {
url: 'data:...' } }) which will fail with Anthropic; update the logic to branch
on the target model/provider and emit Anthropic's expected structure ({ type:
'image', source: { type: 'base64', media_type: 'image/png', data: '...' } })
when using Anthropic, while keeping the existing OpenAI format for OpenAI
models; factor this into a small helper (e.g., formatImageForProvider(provider,
imageData)) and replace the current image object creation at both locations with
a call to that helper.
|
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.
| /** | ||
| * Join two XPath segments. | ||
| * Handles shadow root hops ('//' markers) correctly. | ||
| */ | ||
| function joinXPath(base: string, step: string): string { | ||
| // Shadow root hop marker | ||
| if (step === '//') { | ||
| if (!base || base === '/') { | ||
| return '//'; | ||
| } | ||
| return base.endsWith('/') ? `${base}/` : `${base}//`; | ||
| } | ||
|
|
||
| if (!base || base === '/') { | ||
| return step ? `/${step}` : '/'; | ||
| } | ||
|
|
||
| if (base.endsWith('//')) { | ||
| return `${base}${step}`; | ||
| } | ||
|
|
||
| if (!step) { | ||
| return base; | ||
| } | ||
|
|
||
| return `${base}/${step}`; | ||
| } |
There was a problem hiding this comment.
'//' as “shadow hop marker” is not valid/portable XPath and can be misinterpreted downstream.
Standard XPath uses // as “descendant-or-self” axis, not “enter shadow root”. If any consumer treats these as real XPath (e.g., document.evaluate), results will be surprising/wrong. Consider a clearly non-XPath token (e.g., ::shadow::) or keep two representations: displayXPath vs runtimeXPath.
Also applies to: 256-265
| } else if (method === 'drag') { | ||
| // Get element coordinates | ||
| const nodeResponse = await domAgent.invoke_describeNode({ objectId }); | ||
| if (!nodeResponse.node.backendNodeId) { | ||
| throw new Error('Could not get backend node ID for element'); | ||
| } | ||
|
|
||
| const boxModel = await domAgent.invoke_getBoxModel({ | ||
| backendNodeId: nodeResponse.node.backendNodeId as Protocol.DOM.BackendNodeId | ||
| }); | ||
|
|
||
| if (!boxModel.model) { | ||
| throw new Error('Could not get box model for element'); | ||
| } | ||
|
|
||
| // Calculate center point of element (start position) | ||
| const contentQuad = boxModel.model.content; | ||
| const startX = (contentQuad[0] + contentQuad[2] + contentQuad[4] + contentQuad[6]) / 4; | ||
| const startY = (contentQuad[1] + contentQuad[3] + contentQuad[5] + contentQuad[7]) / 4; | ||
|
|
||
| // Get drag offset from args | ||
| const dragArgs = args[0] as { offsetX?: number; offsetY?: number; toX?: number; toY?: number }; | ||
| let endX: number; | ||
| let endY: number; | ||
|
|
||
| if (dragArgs.toX !== undefined && dragArgs.toY !== undefined) { | ||
| // Absolute position | ||
| endX = dragArgs.toX; | ||
| endY = dragArgs.toY; | ||
| } else { | ||
| // Relative offset (default) | ||
| endX = startX + (dragArgs.offsetX || 0); | ||
| endY = startY + (dragArgs.offsetY || 0); | ||
| } | ||
|
|
||
| // Mouse down at start position | ||
| await inputAgent.invoke_dispatchMouseEvent({ | ||
| type: Protocol.Input.DispatchMouseEventRequestType.MousePressed, | ||
| x: startX, | ||
| y: startY, | ||
| button: Protocol.Input.MouseButton.Left, | ||
| clickCount: 1 | ||
| }); | ||
|
|
||
| // Move in steps for smoother dragging | ||
| const steps = 10; | ||
| for (let i = 1; i <= steps; i++) { | ||
| const progress = i / steps; | ||
| const currentX = startX + (endX - startX) * progress; | ||
| const currentY = startY + (endY - startY) * progress; | ||
|
|
||
| await inputAgent.invoke_dispatchMouseEvent({ | ||
| type: Protocol.Input.DispatchMouseEventRequestType.MouseMoved, | ||
| x: currentX, | ||
| y: currentY, | ||
| button: Protocol.Input.MouseButton.Left | ||
| }); | ||
|
|
||
| // Small delay between moves | ||
| await new Promise(resolve => setTimeout(resolve, 10)); | ||
| } | ||
|
|
||
| // Mouse up at end position | ||
| await inputAgent.invoke_dispatchMouseEvent({ | ||
| type: Protocol.Input.DispatchMouseEventRequestType.MouseReleased, | ||
| x: endX, | ||
| y: endY, | ||
| button: Protocol.Input.MouseButton.Left, | ||
| clickCount: 1 | ||
| }); |
There was a problem hiding this comment.
Make drag robust: validate args and guarantee MouseReleased via try/finally.
If any step throws after MousePressed, the pointer can remain “down”. Also args[0] may be missing/non-object.
Proposed fix
- const dragArgs = args[0] as { offsetX?: number; offsetY?: number; toX?: number; toY?: number };
+ const dragArgs = (args?.[0] && typeof args[0] === 'object' ? args[0] : {}) as {
+ offsetX?: number; offsetY?: number; toX?: number; toY?: number;
+ };
- // Mouse down at start position
- await inputAgent.invoke_dispatchMouseEvent({
- type: Protocol.Input.DispatchMouseEventRequestType.MousePressed,
- x: startX,
- y: startY,
- button: Protocol.Input.MouseButton.Left,
- clickCount: 1
- });
-
- // Move in steps for smoother dragging
- const steps = 10;
- for (let i = 1; i <= steps; i++) {
- const progress = i / steps;
- const currentX = startX + (endX - startX) * progress;
- const currentY = startY + (endY - startY) * progress;
-
- await inputAgent.invoke_dispatchMouseEvent({
- type: Protocol.Input.DispatchMouseEventRequestType.MouseMoved,
- x: currentX,
- y: currentY,
- button: Protocol.Input.MouseButton.Left
- });
-
- // Small delay between moves
- await new Promise(resolve => setTimeout(resolve, 10));
- }
-
- // Mouse up at end position
- await inputAgent.invoke_dispatchMouseEvent({
- type: Protocol.Input.DispatchMouseEventRequestType.MouseReleased,
- x: endX,
- y: endY,
- button: Protocol.Input.MouseButton.Left,
- clickCount: 1
- });
+ await inputAgent.invoke_dispatchMouseEvent({
+ type: Protocol.Input.DispatchMouseEventRequestType.MousePressed,
+ x: startX,
+ y: startY,
+ button: Protocol.Input.MouseButton.Left,
+ clickCount: 1,
+ });
+
+ try {
+ const steps = 10;
+ for (let i = 1; i <= steps; i++) {
+ const progress = i / steps;
+ const currentX = startX + (endX - startX) * progress;
+ const currentY = startY + (endY - startY) * progress;
+
+ await inputAgent.invoke_dispatchMouseEvent({
+ type: Protocol.Input.DispatchMouseEventRequestType.MouseMoved,
+ x: currentX,
+ y: currentY,
+ });
+
+ await new Promise(resolve => setTimeout(resolve, 10));
+ }
+ } finally {
+ await inputAgent.invoke_dispatchMouseEvent({
+ type: Protocol.Input.DispatchMouseEventRequestType.MouseReleased,
+ x: endX,
+ y: endY,
+ button: Protocol.Input.MouseButton.Left,
+ clickCount: 1,
+ });
+ }🤖 Prompt for AI Agents
In @front_end/panels/ai_chat/common/utils.ts around lines 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.
| 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
| "front_end/panels/ai_chat/tools/DOMToolsRegistration.js", | ||
| "front_end/panels/ai_chat/tools/HybridAccessibilityTreeTool.js", | ||
| "front_end/panels/ai_chat/tools/CachedSchemaExtractorTool.js", | ||
| "front_end/panels/ai_chat/tools/GetAccessibilityTreeToolV0.js", | ||
| "front_end/panels/ai_chat/tools/SearchTool.js", | ||
| "front_end/panels/ai_chat/tools/TryCachedActionTool.js", | ||
| "front_end/panels/ai_chat/tools/action_cache/ActionPatternCache.js", | ||
| "front_end/panels/ai_chat/tools/action_cache/ActionPatternCapture.js", | ||
| "front_end/panels/ai_chat/tools/action_cache/types.js", | ||
| "front_end/panels/ai_chat/tools/search/SearchPatternCache.js", | ||
| "front_end/panels/ai_chat/tools/search/SearchStrategy.js", | ||
| "front_end/panels/ai_chat/tools/search/types.js", | ||
| "front_end/panels/ai_chat/tools/selector_cache/SelectorCache.js", | ||
| "front_end/panels/ai_chat/tools/selector_cache/types.js", | ||
| "front_end/panels/ai_chat/a11y/FrameRegistry.js", | ||
| "front_end/panels/ai_chat/a11y/HybridSnapshot.js", | ||
| "front_end/panels/ai_chat/a11y/HybridSnapshotUniversal.js", | ||
| "front_end/panels/ai_chat/dom/ComposedTreeResolver.js", | ||
| "front_end/panels/ai_chat/dom/ElementResolver.js", | ||
| "front_end/panels/ai_chat/dom/EnhancedElementResolver.js", | ||
| "front_end/panels/ai_chat/dom/ShadowPiercer.js", | ||
| "front_end/panels/ai_chat/dom/shadow-piercer-runtime.js", | ||
| "front_end/panels/ai_chat/dom/index.js", | ||
| "front_end/panels/ai_chat/cdp/CDPSessionAdapter.js", | ||
| "front_end/panels/ai_chat/cdp/DirectCDPAdapter.js", | ||
| "front_end/panels/ai_chat/cdp/FrameRegistryUniversal.js", | ||
| "front_end/panels/ai_chat/cdp/SDKTargetAdapter.js", | ||
| "front_end/panels/ai_chat/cdp/getAdapter.js", | ||
| "front_end/panels/ai_chat/cdp/index.js", | ||
| "front_end/panels/ai_chat/common/utils.js", | ||
| "front_end/panels/ai_chat/common/utils-universal.js", | ||
| "front_end/panels/ai_chat/common/xpath-builder.js", | ||
| "front_end/panels/ai_chat/common/geometry-helpers.js", | ||
| "front_end/panels/ai_chat/common/mouse-helpers.js", | ||
| "front_end/panels/ai_chat/common/log.js", | ||
| "front_end/panels/ai_chat/common/context.js", | ||
| "front_end/panels/ai_chat/common/page.js", | ||
| "front_end/panels/ai_chat/common/accessibility-tree-search.js", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the file exists and get its size
if [[ -f "config/gni/devtools_grd_files.gni" ]]; then
echo "File found. Line count:"
wc -l "config/gni/devtools_grd_files.gni"
else
echo "File not found"
exit 1
fiRepository: BrowserOperator/browser-operator-core
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Verify a sample of newly added files mentioned in the review
files=(
"front_end/panels/ai_chat/tools/DOMToolsRegistration.js"
"front_end/panels/ai_chat/tools/HybridAccessibilityTreeTool.js"
"front_end/panels/ai_chat/a11y/FrameRegistry.js"
"front_end/panels/ai_chat/dom/ComposedTreeResolver.js"
"front_end/panels/ai_chat/cdp/CDPSessionAdapter.js"
"front_end/panels/ai_chat/common/utils.js"
"front_end/panels/ai_chat/agent_framework/RuntimeContext.js"
"front_end/panels/ai_chat/evaluation/test-cases/action-agent-shadow-dom-tests.js"
)
missing=0
for file in "${files[@]}"; do
if [[ ! -f "$file" ]]; then
echo "❌ Missing: $file"
((missing++))
else
echo "✅ Found: $file"
fi
done
echo ""
echo "Summary: $missing missing files"
if [[ $missing -gt 0 ]]; then
exit 1
fiRepository: BrowserOperator/browser-operator-core
Length of output: 641
🏁 Script executed:
#!/bin/bash
# Extract lines 780-817 from the gni file to verify content
echo "=== Lines 780-817 ==="
sed -n '780,817p' config/gni/devtools_grd_files.gni
echo ""
echo "=== Line 854 ==="
sed -n '854p' config/gni/devtools_grd_files.gni
echo ""
echo "=== Lines 870-871 ==="
sed -n '870,871p' config/gni/devtools_grd_files.gni
echo ""
echo "=== Lines 880-890 ==="
sed -n '880,890p' config/gni/devtools_grd_files.gniRepository: BrowserOperator/browser-operator-core
Length of output: 3417
🏁 Script executed:
find front_end/panels/ai_chat -type f -name "*.js" 2>/dev/null | head -20Repository: BrowserOperator/browser-operator-core
Length of output: 186
🏁 Script executed:
# Check if the entire ai_chat directory exists
ls -la front_end/panels/ 2>&1 | head -20Repository: BrowserOperator/browser-operator-core
Length of output: 1244
🏁 Script executed:
# List all files in ai_chat directory recursively
find front_end/panels/ai_chat -type f -name "*.js" | sortRepository: BrowserOperator/browser-operator-core
Length of output: 186
🏁 Script executed:
# Check subdirectories in ai_chat
ls -la front_end/panels/ai_chat/Repository: BrowserOperator/browser-operator-core
Length of output: 1849
🏁 Script executed:
# Check what files exist in the tools subdirectory
find front_end/panels/ai_chat/tools -type f | sortRepository: BrowserOperator/browser-operator-core
Length of output: 3397
🏁 Script executed:
# Check what files exist in the cdp subdirectory
find front_end/panels/ai_chat/cdp -type f | sortRepository: BrowserOperator/browser-operator-core
Length of output: 364
🏁 Script executed:
# Check what files exist in common subdirectory
find front_end/panels/ai_chat/common -type f | sortRepository: BrowserOperator/browser-operator-core
Length of output: 992
🏁 Script executed:
# Check evaluation/test-cases subdirectory
find front_end/panels/ai_chat/evaluation/test-cases -type f 2>/dev/null | sortRepository: BrowserOperator/browser-operator-core
Length of output: 1106
🏁 Script executed:
# Check a11y subdirectory
find front_end/panels/ai_chat/a11y -type f | sortRepository: BrowserOperator/browser-operator-core
Length of output: 333
🏁 Script executed:
# Check dom subdirectory
find front_end/panels/ai_chat/dom -type f | sortRepository: BrowserOperator/browser-operator-core
Length of output: 444
🏁 Script executed:
# Check agent_framework subdirectory
find front_end/panels/ai_chat/agent_framework -type f | sortRepository: BrowserOperator/browser-operator-core
Length of output: 2739
CRITICAL: All file paths use incorrect .js extension instead of .ts
The gni file lists files with .js extensions (e.g., DOMToolsRegistration.js), but all referenced files in the repository use .ts extensions (e.g., DOMToolsRegistration.ts). This affects approximately 40+ entries across:
front_end/panels/ai_chat/tools/*(lines 780-793)front_end/panels/ai_chat/a11y/*(lines 794-796)front_end/panels/ai_chat/dom/*(lines 797-802)front_end/panels/ai_chat/cdp/*(lines 803-808)front_end/panels/ai_chat/common/*(lines 810-817)front_end/panels/ai_chat/agent_framework/*(line 854, 870-871)front_end/panels/ai_chat/evaluation/test-cases/*(lines 880-890)
The build will fail because the build system cannot locate these files with the .js extension. Update all file paths to use .ts extension to match the actual TypeScript source files.
🤖 Prompt for AI Agents
In @config/gni/devtools_grd_files.gni around lines 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.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.