Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
19 changes: 19 additions & 0 deletions config/gni/devtools_grd_files.gni
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,8 @@ grd_files_bundled_sources = [
"front_end/panels/ai_chat/core/PageInfoManager.js",
"front_end/panels/ai_chat/core/AgentNodes.js",
"front_end/panels/ai_chat/core/GraphHelpers.js",
"front_end/panels/ai_chat/core/ToolSurfaceProvider.js",
"front_end/panels/ai_chat/core/ToolNameMap.js",
"front_end/panels/ai_chat/core/StateGraph.js",
"front_end/panels/ai_chat/core/Logger.js",
"front_end/panels/ai_chat/core/AgentErrorHandler.js",
Expand Down Expand Up @@ -723,6 +725,11 @@ grd_files_bundled_sources = [
"front_end/panels/ai_chat/evaluation/utils/PromptTemplates.js",
"front_end/panels/ai_chat/evaluation/utils/ResponseParsingUtils.js",
"front_end/panels/ai_chat/evaluation/utils/SanitizationUtils.js",
"front_end/panels/ai_chat/mcp/MCPConfig.js",
"front_end/panels/ai_chat/mcp/MCPRegistry.js",
"front_end/panels/ai_chat/mcp/MCPToolAdapter.js",
"front_end/panels/ai_chat/mcp/MCPMetaTools.js",
"front_end/panels/ai_chat/tools/LLMTracingWrapper.js",
"front_end/panels/animation/animation-meta.js",
"front_end/panels/animation/animation.js",
"front_end/panels/application/application-meta.js",
Expand Down Expand Up @@ -860,6 +867,7 @@ grd_files_bundled_sources = [
"front_end/third_party/lighthouse/lighthouse-dt-bundle.js",
"front_end/third_party/lighthouse/report/report.js",
"front_end/third_party/lit/lit.js",
"front_end/third_party/mcp-sdk/mcp-sdk.js",
"front_end/third_party/marked/marked.js",
"front_end/third_party/puppeteer-replay/puppeteer-replay.js",
"front_end/third_party/puppeteer/puppeteer.js",
Expand Down Expand Up @@ -1372,6 +1380,7 @@ grd_files_unbundled_sources = [
"front_end/panels/ai_assistance/components/ChatView.js",
"front_end/panels/ai_assistance/components/ExploreWidget.js",
"front_end/panels/ai_assistance/components/MarkdownRendererWithCodeBlock.js",
"front_end/panels/ai_assistance/components/ScrollPinHelper.js",
"front_end/panels/ai_assistance/components/UserActionRow.js",
"front_end/panels/ai_assistance/components/chatView.css.js",
"front_end/panels/ai_assistance/components/exploreWidget.css.js",
Expand Down Expand Up @@ -2201,6 +2210,16 @@ grd_files_unbundled_sources = [
"front_end/third_party/lit/lib/lit.js",
"front_end/third_party/lit/lib/static-html.js",
"front_end/third_party/marked/package/lib/marked.esm.js",
"front_end/third_party/mcp-sdk/ajv/dist/ajv.js",
"front_end/third_party/mcp-sdk/eventsource-parser/package/dist/index.js",
"front_end/third_party/mcp-sdk/eventsource-parser/package/dist/stream.js",
"front_end/third_party/mcp-sdk/package/dist/client/index.js",
"front_end/third_party/mcp-sdk/package/dist/client/sse.js",
"front_end/third_party/mcp-sdk/package/dist/shared/protocol.js",
"front_end/third_party/mcp-sdk/package/dist/shared/transport.js",
"front_end/third_party/mcp-sdk/package/dist/types.js",
"front_end/third_party/mcp-sdk/zod/lib/index.js",
"front_end/third_party/mcp-sdk/zod/lib/index.mjs",
"front_end/third_party/puppeteer-replay/package/lib/main.js",
"front_end/third_party/puppeteer/package/lib/esm/puppeteer/api/Browser.js",
"front_end/third_party/puppeteer/package/lib/esm/puppeteer/api/BrowserContext.js",
Expand Down
16 changes: 16 additions & 0 deletions front_end/panels/ai_chat/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ devtools_module("ai_chat") {
"core/PageInfoManager.ts",
"core/AgentNodes.ts",
"core/GraphHelpers.ts",
"core/ToolNameMap.ts",
"core/ToolSurfaceProvider.ts",
"core/StateGraph.ts",
"core/Logger.ts",
"core/AgentErrorHandler.ts",
Expand Down Expand Up @@ -122,6 +124,10 @@ devtools_module("ai_chat") {
"tracing/TracingConfig.ts",
"auth/PKCEUtils.ts",
"auth/OpenRouterOAuth.ts",
"mcp/MCPConfig.ts",
"mcp/MCPToolAdapter.ts",
"mcp/MCPRegistry.ts",
"mcp/MCPMetaTools.ts",
]

deps = [
Expand All @@ -131,6 +137,7 @@ devtools_module("ai_chat") {
"../../core/sdk:bundle",
"../../generated:protocol",
"../../models/logs:bundle",
"../../third_party/mcp-sdk:bundle",
"../../ui/components/helpers:bundle",
"../../ui/components/markdown_view:bundle",
"../../ui/components/snackbars:bundle",
Expand Down Expand Up @@ -183,6 +190,8 @@ _ai_chat_sources = [
"core/PageInfoManager.ts",
"core/AgentNodes.ts",
"core/GraphHelpers.ts",
"core/ToolNameMap.ts",
"core/ToolSurfaceProvider.ts",
"core/StateGraph.ts",
"core/Logger.ts",
"core/AgentErrorHandler.ts",
Expand All @@ -200,6 +209,7 @@ _ai_chat_sources = [
"LLM/MessageSanitizer.ts",
"LLM/LLMClient.ts",
"tools/Tools.ts",
"tools/LLMTracingWrapper.ts",
"tools/CritiqueTool.ts",
"tools/FetcherTool.ts",
"tools/FinalizeWithCritiqueTool.ts",
Expand Down Expand Up @@ -249,6 +259,10 @@ _ai_chat_sources = [
"tracing/TracingConfig.ts",
"auth/PKCEUtils.ts",
"auth/OpenRouterOAuth.ts",
"mcp/MCPConfig.ts",
"mcp/MCPToolAdapter.ts",
"mcp/MCPRegistry.ts",
"mcp/MCPMetaTools.ts",
]

# Construct the expected JS output paths for the metadata
Expand Down Expand Up @@ -345,6 +359,8 @@ ts_library("unittests") {
"agent_framework/__tests__/AgentRunner.sanitizeToolResult.test.ts",
"agent_framework/__tests__/AgentRunner.computeToolResultText.test.ts",
"agent_framework/__tests__/AgentRunner.run.flows.test.ts",
"mcp/MCPClientSDK.test.ts",
"core/ToolSurfaceProvider.test.ts",
]

deps = [
Expand Down
1 change: 1 addition & 0 deletions front_end/panels/ai_chat/agent_framework/AgentRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,7 @@ export class AgentRunner {
logger.info(`${agentName} Executing tool: ${toolToExecute.name}`);
const execTracingContext = getCurrentTracingContext();
toolResultData = await toolToExecute.execute(toolArgs as any, ({
apiKey: config.apiKey,
provider: config.provider,
model: modelName,
miniModel: config.miniModel,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import { AgentService } from '../core/AgentService.js';
import type { Tool } from '../tools/Tools.js';
import { ChatMessageEntity, type ChatMessage } from '../models/ChatTypes.js';
import { createLogger } from '../core/Logger.js';
Expand All @@ -17,6 +16,7 @@ import { AgentRunner, type AgentRunnerConfig, type AgentRunnerHooks } from './Ag

// Context passed along with agent/tool calls
export interface CallCtx {
apiKey?: string,
provider?: LLMProvider,
model?: string,
miniModel?: string,
Expand Down Expand Up @@ -196,7 +196,7 @@ export class ToolRegistry {
try {
const instance = factory();
this.registeredTools.set(name, instance);
logger.info('Registered and instantiated tool: ${name}');
logger.info(`Registered and instantiated tool: ${name}`);
} catch (error) {
logger.error(`Failed to instantiate tool '${name}' during registration:`, error);
// Remove the factory entry if instantiation fails
Expand Down Expand Up @@ -402,8 +402,8 @@ export class ConfigurableAgentTool implements Tool<ConfigurableAgentArgs, Config

// Get current tracing context for debugging
const tracingContext = getCurrentTracingContext();
const agentService = AgentService.getInstance();
const apiKey = agentService.getApiKey();
const callCtx = (_ctx || {}) as CallCtx;
const apiKey = callCtx.apiKey;

if (!apiKey) {
const errorResult = this.createErrorResult(`API key not configured for ${this.name}`, [], 'error');
Expand All @@ -427,9 +427,6 @@ export class ConfigurableAgentTool implements Tool<ConfigurableAgentArgs, Config
// Initialize
const maxIterations = this.config.maxIterations || 10;

// Parse execution context first
const callCtx = (_ctx || {}) as CallCtx;

// Resolve model name from context or configuration
let modelName: string;
if (this.config.modelName === MODEL_SENTINELS.USE_MINI) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { WaitTool } from '../../tools/Tools.js';
import { MODEL_SENTINELS } from '../../core/Constants.js';
import { ThinkingTool } from '../../tools/ThinkingTool.js';
import type { Tool } from '../../tools/Tools.js';
import { registerMCPMetaTools } from '../../mcp/MCPMetaTools.js';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix ESLint import/order violation for MCP import (must come before tools imports).

ESLint flags this new import placement. Move it above ../../tools/FetcherTool.js (i.e., to the top of the internal import block). Also avoid duplicate imports after reordering.

Apply within this hunk (remove misplaced import):

- import { registerMCPMetaTools } from '../../mcp/MCPMetaTools.js';

Place this at the top of the import section (before any ../../tools/* imports):

import { registerMCPMetaTools } from '../../mcp/MCPMetaTools.js';
import { FetcherTool } from '../../tools/FetcherTool.js';
import { FinalizeWithCritiqueTool } from '../../tools/FinalizeWithCritiqueTool.js';
...

Optional: If bundle size is a concern, consider lazy-loading MCP meta-tools (dynamic import) guarded by settings/allowlist in a follow-up, but that would require adjusting call sites to handle async.

🧰 Tools
🪛 ESLint

[error] 22-22: ../../mcp/MCPMetaTools.js import should occur before import of ../../tools/FetcherTool.js

(import/order)

🤖 Prompt for AI Agents
In front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts
around line 22, the import for registerMCPMetaTools is placed after tools
imports causing an ESLint import/order violation; move the registerMCPMetaTools
import to the top of the internal import block so it comes before any
../../tools/* imports, remove any duplicate imports that result from the
reorder, and ensure the tools (e.g., FetcherTool, FinalizeWithCritiqueTool,
etc.) remain imported after this MCP import; optionally consider converting the
MCP import to a dynamic import behind a feature flag in a follow-up if bundle
size is a concern.


/**
* Configuration for the Direct URL Navigator Agent
Expand Down Expand Up @@ -93,6 +94,8 @@ Remember: Always use navigate_url to actually go to the constructed URLs. Return
* Initialize all configured agents
*/
export function initializeConfiguredAgents(): void {
// Ensure MCP meta-tools are available regardless of mode; selection logic decides if they are surfaced
registerMCPMetaTools();
// Register core tools
ToolRegistry.registerToolFactory('navigate_url', () => new NavigateURLTool());
ToolRegistry.registerToolFactory('navigate_back', () => new NavigateBackTool());
Expand Down
204 changes: 204 additions & 0 deletions front_end/panels/ai_chat/core/AgentNodes.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
// Copyright 2025 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

import { createToolExecutorNode } from './AgentNodes.js';
import { ConfigurableAgentTool } from '../agent_framework/ConfigurableAgentTool.js';
import { ChatMessageEntity } from '../ui/ChatView.js';
import type { AgentState } from './State.js';
import type { ConfigurableAgentResult } from '../agent_framework/ConfigurableAgentTool.js';

declare global {
function describe(name: string, fn: () => void): void;
function it(name: string, fn: () => void): void;
function beforeEach(fn: () => void): void;
function afterEach(fn: () => void): void;
namespace assert {
function strictEqual(actual: unknown, expected: unknown): void;
function deepEqual(actual: unknown, expected: unknown): void;
function isTrue(value: unknown): void;
function isFalse(value: unknown): void;
function doesNotMatch(actual: string, regexp: RegExp): void;
}
}

describe('AgentNodes ToolExecutorNode', () => {
describe('ConfigurableAgentTool result filtering', () => {
it('should filter out agentSession data from error results', async () => {
// Create a mock ConfigurableAgentTool that returns error with intermediateSteps
const mockAgentSession = {
sessionId: 'test-session-123',
agentName: 'test-agent',
status: 'error',
messages: [
{ id: '1', type: 'tool_call', content: { toolName: 'test_tool' } },
{ id: '2', type: 'tool_result', content: { result: 'test result' } }
],
// This contains sensitive data that should not leak to LLM
nestedSessions: [],
tools: [],
startTime: new Date(),
endTime: new Date(),
terminationReason: 'max_iterations'
};

const errorResultWithSession: ConfigurableAgentResult & { agentSession: any } = {
success: false,
error: 'Agent reached maximum iterations',
terminationReason: 'max_iterations',
// This is the problematic field that contains session data
intermediateSteps: [
{ entity: ChatMessageEntity.USER, text: 'test query' },
{ entity: ChatMessageEntity.AGENT_SESSION, agentSession: mockAgentSession, summary: 'test' }
],
agentSession: mockAgentSession
};

Comment on lines +45 to +56
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Replace any with a typed extension; avoid leaking agentSession types into LLM.

-      const errorResultWithSession: ConfigurableAgentResult & { agentSession: any } = {
+      type AgentResultWithSession = ConfigurableAgentResult & { agentSession: unknown };
+      const errorResultWithSession: AgentResultWithSession = {
@@
-      class MockConfigurableAgentTool extends ConfigurableAgentTool {
+      class MockConfigurableAgentTool extends ConfigurableAgentTool {
@@
-        async execute(): Promise<ConfigurableAgentResult & { agentSession: any }> {
+        async execute(): Promise<AgentResultWithSession> {
           return errorResultWithSession;
         }
       }
📝 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.

Suggested change
const errorResultWithSession: ConfigurableAgentResult & { agentSession: any } = {
success: false,
error: 'Agent reached maximum iterations',
terminationReason: 'max_iterations',
// This is the problematic field that contains session data
intermediateSteps: [
{ entity: ChatMessageEntity.USER, text: 'test query' },
{ entity: ChatMessageEntity.AGENT_SESSION, agentSession: mockAgentSession, summary: 'test' }
],
agentSession: mockAgentSession
};
// Define a reusable result type that carries an opaque session
type AgentResultWithSession = ConfigurableAgentResult & { agentSession: unknown };
const errorResultWithSession: AgentResultWithSession = {
success: false,
error: 'Agent reached maximum iterations',
terminationReason: 'max_iterations',
// This is the problematic field that contains session data
intermediateSteps: [
{ entity: ChatMessageEntity.USER, text: 'test query' },
{ entity: ChatMessageEntity.AGENT_SESSION, agentSession: mockAgentSession, summary: 'test' }
],
agentSession: mockAgentSession
};
class MockConfigurableAgentTool extends ConfigurableAgentTool {
// Use the alias here instead of repeating an `any`-based type
async execute(): Promise<AgentResultWithSession> {
return errorResultWithSession;
}
}
🧰 Tools
🪛 ESLint

[error] 45-45: Unexpected any. Specify a different type.

(@typescript-eslint/no-explicit-any)

🤖 Prompt for AI Agents
In front_end/panels/ai_chat/core/AgentNodes.test.ts around lines 45-56, the test
uses "any" for agentSession and embeds full session objects in
intermediateSteps, leaking agentSession internals into LLM-facing step data; fix
by replacing "any" with a concrete typed extension: import/define an
AgentSession (or AgentSessionSummary) type and type the test value as
ConfigurableAgentResult & { agentSession: AgentSession | null }; change
intermediateSteps so AGENT_SESSION steps carry a minimal typed summary (e.g., {
entity: ChatMessageEntity.AGENT_SESSION; agentSession: AgentSessionSummary;
summary: string }) or remove the full session from intermediateSteps and only
store it on agentSession; update the mock to match the new types and ensure no
raw session internals are passed into LLM-facing fields.

// Create mock ConfigurableAgentTool
class MockConfigurableAgentTool extends ConfigurableAgentTool {
constructor() {
super({
name: 'mock_agent',
description: 'Mock agent for testing',
systemPrompt: 'Test prompt',
tools: [],
schema: { type: 'object', properties: {}, required: [] }
});
}

async execute(): Promise<ConfigurableAgentResult & { agentSession: any }> {
return errorResultWithSession;
}
}

const mockTool = new MockConfigurableAgentTool();

// Create initial state with a tool call message
const initialState: AgentState = {
messages: [
{
entity: ChatMessageEntity.MODEL,
action: 'tool',
toolName: 'mock_agent',
toolArgs: { query: 'test' },
toolCallId: 'test-call-id',
isFinalAnswer: false
}
],
agentType: 'web_task',
context: {}
};

// Create modified state with mock tool in registry
const stateWithMockTool = {
...initialState,
// We need to mock the tool registry or provide tools directly
tools: [mockTool]
};

// Create ToolExecutorNode
const toolExecutorNode = createToolExecutorNode(stateWithMockTool);

// Execute the node
const result = await toolExecutorNode.invoke(stateWithMockTool);

Comment on lines +99 to +104
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

createToolExecutorNode signature updated — pass provider and model.

Current test calls will fail to compile.

-      const toolExecutorNode = createToolExecutorNode(stateWithMockTool);
+      const toolExecutorNode = createToolExecutorNode(
+        stateWithMockTool,
+        'test' as unknown as LLMProvider,
+        'test-model'
+      );
@@
-      const toolExecutorNode = createToolExecutorNode(stateWithMockTool);
+      const toolExecutorNode = createToolExecutorNode(
+        stateWithMockTool,
+        'test' as unknown as LLMProvider,
+        'test-model'
+      );

Also applies to: 189-191

🤖 Prompt for AI Agents
In front_end/panels/ai_chat/core/AgentNodes.test.ts around lines 99-104 (and
similarly at 189-191), the test calls createToolExecutorNode with a single
argument but the function signature now requires provider and model parameters;
update the test to pass the provider and model when creating the node (e.g.,
extract or mock provider and model from stateWithMockTool or create small mock
values) so the call matches the new signature and the test compiles.

// Verify the result
assert.isTrue(result.messages.length > initialState.messages.length, 'Should add tool result message');

const toolResultMessage = result.messages[result.messages.length - 1];
assert.strictEqual(toolResultMessage.entity, ChatMessageEntity.TOOL_RESULT);

// The critical test: resultText should NOT contain agentSession data
const resultText = (toolResultMessage as any).resultText;
assert.strictEqual(resultText, 'Error: Agent reached maximum iterations');

// Verify that the resultText does not contain session data
assert.doesNotMatch(resultText, /sessionId/);
assert.doesNotMatch(resultText, /test-session-123/);
assert.doesNotMatch(resultText, /intermediateSteps/);
assert.doesNotMatch(resultText, /agentSession/);
assert.doesNotMatch(resultText, /nestedSessions/);

// The resultText should be clean and only contain the error message
assert.strictEqual(resultText.includes('test-session-123'), false);
});

it('should filter out agentSession data from success results', async () => {
// Create a success result with intermediateSteps containing session data
const mockAgentSession = {
sessionId: 'success-session-456',
agentName: 'success-agent',
status: 'completed',
messages: [],
nestedSessions: [],
tools: [],
startTime: new Date(),
endTime: new Date(),
terminationReason: 'final_answer'
};

const successResultWithSession: ConfigurableAgentResult & { agentSession: any } = {
success: true,
output: 'Task completed successfully',
terminationReason: 'final_answer',
// This should not leak to LLM
intermediateSteps: [
{ entity: ChatMessageEntity.AGENT_SESSION, agentSession: mockAgentSession, summary: 'test' }
],
agentSession: mockAgentSession
};

class MockSuccessAgentTool extends ConfigurableAgentTool {
constructor() {
super({
name: 'mock_success_agent',
description: 'Mock success agent for testing',
systemPrompt: 'Test prompt',
tools: [],
schema: { type: 'object', properties: {}, required: [] }
});
}

async execute(): Promise<ConfigurableAgentResult & { agentSession: any }> {
return successResultWithSession;
}
}

Comment on lines +140 to +166
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Mirror type cleanup for success-path tool result.

-      const successResultWithSession: ConfigurableAgentResult & { agentSession: any } = {
+      type AgentResultWithSession = ConfigurableAgentResult & { agentSession: unknown };
+      const successResultWithSession: AgentResultWithSession = {
@@
-        async execute(): Promise<ConfigurableAgentResult & { agentSession: any }> {
+        async execute(): Promise<AgentResultWithSession> {
           return successResultWithSession;
         }
🧰 Tools
🪛 ESLint

[error] 140-140: Unexpected any. Specify a different type.

(@typescript-eslint/no-explicit-any)


[error] 162-162: Unexpected any. Specify a different type.

(@typescript-eslint/no-explicit-any)

const mockTool = new MockSuccessAgentTool();

const initialState: AgentState = {
messages: [
{
entity: ChatMessageEntity.MODEL,
action: 'tool',
toolName: 'mock_success_agent',
toolArgs: { query: 'test' },
toolCallId: 'test-call-id-2',
isFinalAnswer: false
}
],
agentType: 'web_task',
context: {}
};

const stateWithMockTool = {
...initialState,
tools: [mockTool]
};

const toolExecutorNode = createToolExecutorNode(stateWithMockTool);
const result = await toolExecutorNode.invoke(stateWithMockTool);

const toolResultMessage = result.messages[result.messages.length - 1];
const resultText = (toolResultMessage as any).resultText;

// Should contain only the clean output
assert.strictEqual(resultText, 'Task completed successfully');

// Should NOT contain session data
assert.doesNotMatch(resultText, /success-session-456/);
assert.doesNotMatch(resultText, /intermediateSteps/);
assert.doesNotMatch(resultText, /agentSession/);
});
});
});
Loading