MCP OAuth Support and more updates #51
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds MCP integration (SDK v2, OAuth provider, config/registry, UI dialogs), introduces agent descriptor registry/versioning, propagates abort signals across graph, nodes, tools, and runners, updates tool names (extract_schema_data → extract_data; get_urls_from_nodeids → node_ids_to_urls), expands agents/configs, updates builds/GRD, images, tests, and evaluation YAMLs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as AIChatPanel
participant Svc as AgentService
participant Graph as StateGraph
participant Node as AgentNodes
participant Tool as Tools/AgentTool
participant Reg as AgentDescriptorRegistry
UI->>Svc: run(agentType, input)
Svc->>Reg: getDescriptor(orchestrator:<type>|default)
Reg-->>Svc: AgentDescriptor?
Svc->>Graph: invoke(state, abortSignal)
loop per node
Graph->>Node: invoke(state, abortSignal)
alt Tool execution
Node->>Tool: execute(args, { abortSignal, agentDescriptor })
Tool-->>Node: result or AbortError
end
Node-->>Graph: updated state
opt abortSignal.aborted
Graph-->>Svc: throw AbortError
end
end
Graph-->>Svc: final state
Svc-->>UI: completion
sequenceDiagram
autonumber
participant User as User
participant Panel as AIChatPanel
participant Catalog as MCPConnectorsCatalogDialog
participant Cfg as MCPConfig
participant Reg as MCPRegistry
participant OAuth as MCPOAuthProvider
User->>Panel: Click "MCP Connectors"
Panel->>Catalog: show()
Catalog->>Cfg: getMCPProviders()
User->>Catalog: Toggle connector ON
Catalog->>Cfg: saveMCPProviders(updated)
alt oauth
Catalog->>Reg: init(interactive=true)
Reg->>OAuth: redirectToAuthorization()
OAuth-->>Reg: waitForAuthorizationCode()
Reg-->>Catalog: ConnectionResult (connected/error)
else bearer
Catalog->>Reg: init()
Reg-->>Catalog: ConnectionResult
end
Catalog-->>Panel: onSave()
Panel->>Reg: refresh/init(false)
sequenceDiagram
autonumber
participant TSP as ToolSurfaceProvider
participant MCP as MCPRegistry
participant LLM as LLMClient
participant TR as ToolRegistry
TSP->>MCP: ensureToolsRegistered()
MCP-->>TSP: done
TSP->>MCP: get MCP tools
alt mode=router
TSP->>LLM: Select tools for query
LLM-->>TSP: ["toolA","toolB"]
TSP->>TR: map names to Tool objects
TR-->>TSP: tools[]
TSP-->>Caller: baseTools ∪ selectedMcpTools (dedup, capped)
else mode=all
TSP-->>Caller: baseTools ∪ allMcpTools (dedup, capped)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @tysonthomas9's task —— View job Code Review: MCP OAuth Support and Agent Framework UpdatesReview Progress
Security Analysis - OAuth Implementation✅ Strengths:
Agent Framework Architecture✅ Well-designed:
Performance Analysis
✅ Performance Positives:
Error Handling✅ Good error handling:
Test Coverage Analysis✅ Good coverage:
Code Organization✅ Strengths:
Overall AssessmentThis is a substantial and well-architected addition that properly implements OAuth support for MCP connections. The code demonstrates good security practices and comprehensive error handling. The main concerns are around performance optimization, production debugging code cleanup, and test coverage for security-critical flows. Recommendations
|
![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 suggestions.
Reply with @codex fix comments to fix any unresolved comments.
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, or 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 fix this CI failure" or "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (19)
front_end/third_party/mcp-sdk/README.md (1)
1-1498: Remove or replace Ajv README in front_end/third_party/mcp-sdk/README.mdThis file contains the upstream Ajv README (Ajv v6/v7‑beta content) and is unrelated to the MCP SDK — remove or replace it.
- Preferred: replace front_end/third_party/mcp-sdk/README.md with MCP SDK–specific documentation (usage, API, build/test notes).
- If Ajv is intentionally vendored: move the Ajv README into front_end/third_party/mcp-sdk/ajv/README.md (or keep only a short vendor notice in the top‑level README that links to the ajv/ folder and records the vendored version/license).
- Do not leave an outdated third‑party README at the package root.
front_end/panels/ai_chat/tools/SchemaBasedExtractorTool.ts (2)
250-266: URL replacement can corrupt non-URL fields (critical).
findAndReplaceNodeIdsblindly replaces any number/string that matches a nodeId, so prices/counts can become URLs. Restrict replacements to URL-typed paths derived from the schema.Apply this diff to gate replacements by URL fields:
@@ - // 5. Replace node IDs with URLs in the data - const updatedData = this.findAndReplaceNodeIds(data, nodeIdToUrlMap); + // 5. Replace node IDs with URLs only at URL fields + const urlFields = this.findUrlFields(schema); + const updatedData = this.replaceNodeIdsInUrlFields(data, nodeIdToUrlMap, urlFields); @@ } }Add these helpers (outside the shown range):
// Derive URL fields similarly to Streamlined extractor private findUrlFields(schema: SchemaDefinition): string[] { const fields: string[] = []; const walk = (prop: any, prefix = '') => { for (const [k, v] of Object.entries(prop || {})) { const full = prefix ? `${prefix}.${k}` : k; if (v && typeof v === 'object') { if (v.type === 'string' && v.format === 'url') fields.push(full); else if (v.type === 'array' && v.items?.type === 'string' && v.items?.format === 'url') fields.push(full); else if (v.type === 'object' && v.properties) walk(v.properties, full); else if (v.type === 'array' && v.items?.type === 'object' && v.items?.properties) walk(v.items.properties, `${full}[]`); } } }; if (schema.properties) walk(schema.properties); return fields; } // Replace only along URL fields private replaceNodeIdsInUrlFields(data: any, map: Record<number, string>, urlFields: string[]): any { const isTarget = (path: string) => { const np = path.replace(/\[\d+\]/g, '[]'); return urlFields.some(f => { const nf = f.replace(/\[\]/g, ''); return np.endsWith(nf) || np.includes(nf) || nf.endsWith(path.replace(/\[\d+\]/g, '')); }); }; const walk = (value: any, path: string): any => { if (isTarget(path) && (typeof value === 'number' || typeof value === 'string')) { const n = typeof value === 'number' ? value : parseInt(value, 10); if (!isNaN(n) && map[n]) return map[n]; } if (Array.isArray(value)) return value.map((it, i) => walk(it, `${path}[${i}]`)); if (value && typeof value === 'object') { const out: any = {}; for (const [k, v] of Object.entries(value)) out[k] = walk(v, `${path}.${k}`); return out; } return value; }; if (Array.isArray(data)) return data.map((it, i) => walk(it, `[${i}]`)); const out: any = {}; for (const [k, v] of Object.entries(data || {})) out[k] = walk(v, k); return out; }Also applies to: 726-761, 798-852
54-71: Interface vs runtime schema mismatch on “reasoning”.Tool schema requires
reasoning, butSchemaExtractionArgs.reasoningis optional. This will cause validation/UI inconsistencies.Prefer making it optional in the tool schema:
@@ - required: ['schema', 'instruction', 'reasoning'] + required: ['schema', 'instruction']Also applies to: 858-862
eval-server/nodejs/evals/schema-extractor/homedepot-001.yaml (1)
75-79: Standardize validation.type to "llm-judge" (hyphen).
- Replace all occurrences of type: "llm_judge" → type: "llm-judge" across eval YAMLs (several files under eval-server/nodejs/evals/** — e.g. eval-server/nodejs/evals/web-task-agent/web-task-agent-realestate-001.yaml:43, eval-server/nodejs/evals/web-task-agent/web-task-agent-jobs-001.yaml:41, eval-server/nodejs/evals/streamlined-schema-extractor/homedepot-001.yaml:76). EvalServer.js and client.schema.json expect "llm-judge"; underscore-valued types will be ignored/invalid.
- Keep the llm config key as validation.llm_judge (underscore) — EvalServer.js reads validation.llm_judge || validation.llmJudge and templates/schemas use llm_judge.
eval-server/nodejs/evals/schema-extractor/google-search-001.yaml (1)
29-31: Useformat: "uri"instead of"url"Prevents silently skipped validations.
Apply:
- url: - type: "string" - format: "url" + url: + type: "string" + format: "uri" @@ - url: - type: "string" - format: "url" + url: + type: "string" + format: "uri"Also applies to: 47-49
front_end/panels/ai_chat/core/StateGraph.ts (1)
85-104: Instrumentation must not break executionWrap tracing calls; if the provider throws, the graph shouldn’t fail.
Apply:
- await this.tracingProvider.createObservation({ + try { + await this.tracingProvider.createObservation({ id: `event-start-${currentNodeName}-${Date.now()}`, @@ - }, tracingContext.traceId); + }, tracingContext.traceId); + } catch (e) { + logger.warn('Tracing start event failed', e); + } @@ - await this.tracingProvider.createObservation({ + try { + await this.tracingProvider.createObservation({ id: `event-complete-${currentNodeName}-${Date.now()}`, @@ - }, tracingContext.traceId); + }, tracingContext.traceId); + } catch (e) { + logger.warn('Tracing complete event failed', e); + } @@ - await this.tracingProvider.createObservation({ + try { + await this.tracingProvider.createObservation({ id: `event-error-${currentNodeName}-${Date.now()}`, @@ - }, tracingContext.traceId); + }, tracingContext.traceId); + } catch (e) { + logger.warn('Tracing error event failed', e); + }Also applies to: 111-131, 139-153
eval-server/nodejs/schemas/client.schema.json (1)
133-139: Preserve backward compatibility: add temporary alias "extract_schema_data"Add "extract_schema_data" back into eval-server/nodejs/schemas/client.schema.json enum (temporary alias) so existing configs still validate, and update docs/samples that still reference the old name.
Remaining references to update: eval-server/nodejs/docs/YAML_SCHEMA.md, eval-server/nodejs/docs/PROTOCOL.md, eval-server/nodejs/docs/TRIGGERING_EVALUATIONS.md, eval-server/nodejs/docs/CLIENT_SETUP.md.
eval-server/nodejs/evals/schema-extractor/amazon-product-001.yaml (1)
41-45: Replace non‑standard JSON Schema format "url" with "uri" across YAML schemasAjv/JSON Schema expects format: "uri"; "url" is non‑standard and may disable validation.
Replace all occurrences of
format: "url"
with
format: "uri"
in these files (occurrences from your rg output):
- eval-server/nodejs/evals/streamlined-schema-extractor/wikipedia-search-001.yaml (lines 32, 54)
- eval-server/nodejs/evals/streamlined-schema-extractor/wikipedia-001.yaml (line 47)
- eval-server/nodejs/evals/streamlined-schema-extractor/macys-001.yaml (lines 52, 55)
- eval-server/nodejs/evals/streamlined-schema-extractor/homedepot-001.yaml (lines 44, 47)
- eval-server/nodejs/evals/streamlined-schema-extractor/google-search-001.yaml (lines 30, 48)
- eval-server/nodejs/evals/streamlined-schema-extractor/google-flights-001.yaml (line 62)
- eval-server/nodejs/evals/streamlined-schema-extractor/bing-search-001.yaml (line 30)
- eval-server/nodejs/evals/streamlined-schema-extractor/bbc-news-001.yaml (lines 30, 44)
- eval-server/nodejs/evals/streamlined-schema-extractor/amazon-product-001.yaml (line 44)
- eval-server/nodejs/evals/schema-extractor/wikipedia-search-001.yaml (lines 32, 54)
- eval-server/nodejs/evals/schema-extractor/macys-001.yaml (lines 52, 55)
- eval-server/nodejs/evals/schema-extractor/homedepot-001.yaml (lines 44, 47)
- eval-server/nodejs/evals/schema-extractor/google-search-001.yaml (lines 30, 48)
- eval-server/nodejs/evals/schema-extractor/google-flights-001.yaml (line 62)
- eval-server/nodejs/evals/schema-extractor/bing-search-001.yaml (line 30)
- eval-server/nodejs/evals/schema-extractor/bbc-news-001.yaml (lines 30, 44)
- eval-server/nodejs/evals/schema-extractor/amazon-product-001.yaml (line 44)
front_end/panels/ai_chat/tools/Tools.ts (4)
1129-1136: Template injection risk in page eval (unescaped selector).Embedding raw
selectorcan break evaluation or allow injection. Use JSON-stringify to safely embed.- expression: `(() => { - const element = document.querySelector("${selector}"); + expression: `(() => { + const sel = ${JSON.stringify(selector)}; + const element = document.querySelector(sel);
1204-1212: Template injection risk forquery.Same fix: JSON.stringify the user-provided text.
- const query = "${query}"; + const query = ${JSON.stringify(query)};
3181-3185: Template injection risk for XPath.Use JSON.stringify for XPath before
document.evaluate.- const element = document.evaluate("${xpath}", document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue; + const xp = ${JSON.stringify(xpath)}; + const element = document.evaluate(xp, document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue;
4204-4227: Fix getTools() types and completeness.
- The Wait tool args type in the union omits
durationandreasoning.TakeScreenshotToolis defined but not registered; agents won’t findtake_screenshot.Tool<{ domain?: string, keyword?: string, daysAgo?: number, limit?: number }, VisitHistorySearchResult | ErrorResult> | - Tool<{ seconds: number, reason?: string }, WaitResult | ErrorResult> | + Tool<{ seconds?: number, duration?: number, reason?: string, reasoning?: string }, WaitResult | ErrorResult> | Tool<SequentialThinkingArgs, SequentialThinkingResult | ErrorResult> )> { return [ @@ new SearchVisitHistoryTool(), new WaitTool(), + new TakeScreenshotTool(), new SequentialThinkingTool() ];Also applies to: 4245-4251
front_end/panels/ai_chat/ui/AIChatPanel.ts (2)
50-73: Do not log secret previews; avoid monkey‑patching storage without restore.
- Logs print first 50 chars of values (API keys). Remove previews and mask.
- Overriding
localStorage.setItem/removeItemwithout guaranteed restore can leak into other panels.- localStorage.setItem = (key: string, value: string) => { + localStorage.setItem = (key: string, value: string) => { if (key.includes('openrouter') || key.includes('ai_chat')) { logger.debug(`=== LOCALSTORAGE SET ===`); logger.debug(`Key: ${key}`); logger.debug(`Value exists: ${!!value}`); - logger.debug(`Value length: ${value?.length || 0}`); - logger.debug(`Value preview: ${value?.substring(0, 50) + (value?.length > 50 ? '...' : '') || 'null'}`); + logger.debug(`Value length: ${value?.length || 0}`); + // Never log actual values; at most log a masked hash if truly needed. } return this.originalSetItem(key, value); }; @@ - localStorage.removeItem = (key: string) => { + localStorage.removeItem = (key: string) => { if (key.includes('openrouter') || key.includes('ai_chat')) { logger.debug(`=== LOCALSTORAGE REMOVE ===`); logger.debug(`Key: ${key}`); logger.debug(`Timestamp: ${new Date().toISOString()}`); } return this.originalRemoveItem(key); };Additionally, call
StorageMonitor.getInstance().restore()inwillHide()(see suggestion below).
1962-1976: Restore storage hooks on panel hide.Avoid leaking patched
localStorageacross the DevTools lifetime.override willHide(): void { // Cancel any running agent execution when the panel is hidden/reloaded try { this.#agentService.cancelRun(); } catch {} + // Restore localStorage hooks installed by StorageMonitor + try { + StorageMonitor.getInstance().restore(); + } catch {}front_end/panels/ai_chat/core/AgentNodes.ts (2)
321-336: Fix ModelChatMessage.reasoning type (expects string[] | null).
response.reasoning?.summarymay be a string. Normalize once and reuse in all message constructions to avoid TS errors.@@ - // Directly create the ModelChatMessage object + // Directly create the ModelChatMessage object let newModelMessage: ModelChatMessage; + const reasoningSummary = + Array.isArray(response.reasoning?.summary) + ? response.reasoning.summary + : (response.reasoning?.summary ? [response.reasoning.summary] : null); @@ - newModelMessage = { + newModelMessage = { entity: ChatMessageEntity.MODEL, action: 'tool', toolName: resolvedToolName, toolArgs: parsedAction.args, toolCallId, // Add for linking with tool response isFinalAnswer: false, - reasoning: response.reasoning?.summary, + reasoning: reasoningSummary, uiLane: isAgentTool ? 'agent' : 'chat', }; @@ - newModelMessage = { + newModelMessage = { entity: ChatMessageEntity.MODEL, action: 'final', answer: parsedAction.answer, isFinalAnswer: true, - reasoning: response.reasoning?.summary, + reasoning: reasoningSummary, }; @@ - newModelMessage = { + newModelMessage = { entity: ChatMessageEntity.MODEL, action: 'final', answer: parsedAction.error || 'An error occurred', isFinalAnswer: true, - reasoning: response.reasoning?.summary, + reasoning: reasoningSummary, };Also applies to: 337-346, 347-355
431-431: Reduce PII/verbosity in logs when converting messages.Avoid logging full message contents.
- logger.info('Converting ChatMessages to LLMMessages. Messages:', messages); + logger.debug('Converting ChatMessages to LLMMessages. count:', messages.length);front_end/panels/ai_chat/core/ToolSurfaceProvider.ts (1)
255-261: Cap ALL mode by maxToolsPerTurn (currently uncapped).Returning all tools can exceed model tool limits and inflate token usage.
- resultTools = uniqByName([...resultTools, ...mcpTools]); + resultTools = uniqByName([...resultTools, ...mcpTools]).slice(0, maxToolsPerTurn); console.log('[TOOL_SELECTION_DEBUG] Final result (ALL mode):', { toolCount: resultTools.length, toolNames: resultTools.map(t => t.name) }); return { tools: resultTools, selectedNames: resultTools.map(t => t.name) };front_end/panels/ai_chat/agent_framework/AgentRunner.ts (2)
1004-1016: Thread abortSignal into tool executions.Without passing it, long-running tools can’t be cancelled mid-iteration.
- toolResultData = await toolToExecute.execute(toolArgs as any, ({ + toolResultData = await toolToExecute.execute(toolArgs as any, ({ apiKey: config.apiKey, provider: config.provider, model: modelName, miniModel: config.miniModel, nanoModel: config.nanoModel, getVisionCapability: config.getVisionCapability, overrideSessionId: preallocatedChildId, overrideParentSessionId: currentSession.sessionId, overrideTraceId: execTracingContext?.traceId, + abortSignal, } as any));
816-824: Normalize reasoning to string[] | null on ModelChatMessage.Same type issue as in AgentNodes.
- newModelMessage = { + const reasoningSummary = + Array.isArray(llmResponse.reasoning?.summary) + ? llmResponse.reasoning.summary + : (llmResponse.reasoning?.summary ? [llmResponse.reasoning.summary] : null); + newModelMessage = { entity: ChatMessageEntity.MODEL, action: 'tool', toolName, toolArgs, toolCallId, // Add for linking with tool response isFinalAnswer: false, - reasoning: llmResponse.reasoning?.summary, + reasoning: reasoningSummary, }; @@ - newModelMessage = { + newModelMessage = { entity: ChatMessageEntity.MODEL, action: 'final', answer, isFinalAnswer: true, - reasoning: llmResponse.reasoning?.summary, + reasoning: reasoningSummary, };Also applies to: 1164-1171
front_end/panels/ai_chat/agent_framework/implementation/agents/ClickActionAgent.ts
Show resolved
Hide resolved
front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts
Show resolved
Hide resolved
front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts
Show resolved
Hide resolved
front_end/panels/ai_chat/agent_framework/implementation/agents/KeyboardInputActionAgent.ts
Show resolved
Hide resolved
| const orchestratorDescriptor = await this.#orchestratorDescriptorPromise; | ||
|
|
There was a problem hiding this comment.
Guard descriptor fetch failures (don’t let trace creation fail).
await this.#orchestratorDescriptorPromise can reject and abort the whole run. Catch and fallback to null.
- const orchestratorDescriptor = await this.#orchestratorDescriptorPromise;
+ let orchestratorDescriptor: AgentDescriptor | null = null;
+ try {
+ orchestratorDescriptor = await this.#orchestratorDescriptorPromise;
+ } catch (e) {
+ logger.warn('Orchestrator descriptor unavailable; proceeding without metadata', e);
+ }📝 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 orchestratorDescriptor = await this.#orchestratorDescriptorPromise; | |
| let orchestratorDescriptor: AgentDescriptor | null = null; | |
| try { | |
| orchestratorDescriptor = await this.#orchestratorDescriptorPromise; | |
| } catch (e) { | |
| logger.warn('Orchestrator descriptor unavailable; proceeding without metadata', e); | |
| } |
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/evaluation/runner/EvaluationRunner.ts around lines
176-177, the awaited this.#orchestratorDescriptorPromise can reject and abort
the run; wrap the await in a try/catch and on rejection set
orchestratorDescriptor to null (or a safe fallback) so trace creation continues,
logging the error if appropriate; ensure downstream code handles a null
descriptor.
| const previousProviders = this.#existingProviders.map(provider => ({ ...provider })); | ||
|
|
||
| // Set global connecting state | ||
| this.setConnectingState(connector.id); | ||
|
|
||
| const progressSnackbar = Snackbars.Snackbar.Snackbar.show({ | ||
| message: i18nString(UIStrings.oauthInProgress, {PH1: connector.name}), | ||
| closable: true, | ||
| }); | ||
|
|
||
| const dismissProgressSnackbar = () => { | ||
| if (!progressSnackbar.isConnected) { | ||
| return; | ||
| } | ||
| const closeButton = progressSnackbar.shadowRoot?.querySelector('.dismiss') as HTMLElement | null; | ||
| if (closeButton) { | ||
| closeButton.click(); | ||
| } else { | ||
| progressSnackbar.remove(); | ||
| } | ||
| }; | ||
|
|
||
| // Update UI to show loading state | ||
| toggle.classList.add('connecting'); | ||
| toggle.disabled = true; | ||
| item.classList.add('connecting'); | ||
| item.setAttribute('aria-busy', 'true'); | ||
|
|
||
| // Add loading indicator to the left of toggle | ||
| const toggleContainer = toggle.parentElement as HTMLElement; | ||
| const existingStatus = toggleContainer.querySelector('.mcp-connector-status, .mcp-loading-indicator'); | ||
| if (existingStatus) { | ||
| existingStatus.remove(); | ||
| } | ||
|
|
||
| const loading = document.createElement('div'); | ||
| loading.className = 'mcp-loading-indicator'; | ||
| loading.title = i18nString(UIStrings.connecting); | ||
| toggleContainer.insertBefore(loading, toggle); | ||
|
|
||
| try { | ||
| const newProvider: MCPProviderConfig = { | ||
| id: connector.id, | ||
| name: connector.name, | ||
| endpoint: connector.endpoint, | ||
| authType: connector.authType, | ||
| enabled: true, | ||
| }; | ||
|
|
||
| // Save provider temporarily to test connection | ||
| const updatedProviders = [...previousProviders, newProvider]; | ||
| saveMCPProviders(updatedProviders); | ||
| this.#existingProviders = getMCPProviders(); | ||
|
|
||
| // Test the connection | ||
| const connectionResults = await MCPRegistry.init(true); | ||
| await MCPRegistry.refresh(); | ||
|
|
||
| // Find the result for our specific connector | ||
| const ourResult = connectionResults.find(result => | ||
| result.endpoint === connector.endpoint || result.serverId === connector.id | ||
| ); | ||
|
|
||
| // If connection failed, throw error to trigger catch block | ||
| if (!ourResult || !ourResult.connected) { | ||
| const error = ourResult?.error || new Error('Connection failed'); | ||
| throw error; | ||
| } | ||
|
|
||
| // Clear global connecting state | ||
| this.clearConnectingState(); | ||
|
|
||
| // Update UI to connected state | ||
| toggle.classList.remove('connecting'); | ||
| toggle.classList.add('enabled'); | ||
| toggle.disabled = false; | ||
| item.classList.remove('connecting'); | ||
| item.classList.add('connected'); | ||
| item.removeAttribute('aria-busy'); | ||
|
|
||
| // Replace loading indicator with status indicator | ||
| const toggleContainer = toggle.parentElement as HTMLElement; | ||
| const loadingIndicator = toggleContainer.querySelector('.mcp-loading-indicator'); | ||
| if (loadingIndicator) { | ||
| loadingIndicator.remove(); | ||
| } | ||
|
|
||
| const status = document.createElement('div'); | ||
| status.className = 'mcp-connector-status'; | ||
| const dot = document.createElement('div'); | ||
| dot.className = 'mcp-status-dot'; | ||
| dot.title = i18nString(UIStrings.connected); | ||
| status.appendChild(dot); | ||
| toggleContainer.appendChild(status); | ||
|
|
||
| logger.info(`Connected MCP connector: ${connector.name}`); | ||
|
|
||
| // Update connection status counter | ||
| this.updateConnectionStatus(); | ||
|
|
||
| dismissProgressSnackbar(); | ||
|
|
||
| const snackbar = Snackbars.Snackbar.Snackbar.show({ | ||
| message: i18nString(UIStrings.successMessage, {PH1: connector.name}), | ||
| actionProperties: { | ||
| label: i18nString(UIStrings.manageConnectionsAction), | ||
| onClick: () => this.#openManageConnections(), | ||
| }, | ||
| }); | ||
| snackbar.dismissTimeout = 4000; | ||
|
|
||
| } catch (error) { | ||
| logger.error('Failed to connect MCP connector', error); | ||
|
|
||
| // Clear global connecting state | ||
| this.clearConnectingState(); | ||
|
|
||
| try { | ||
| saveMCPProviders(previousProviders); | ||
| this.#existingProviders = getMCPProviders(); | ||
| } catch (revertError) { | ||
| logger.error('Failed to revert MCP providers after connect failure', revertError); | ||
| } | ||
|
|
||
| item.removeAttribute('aria-busy'); | ||
|
|
||
| // Remove loading indicator | ||
| const toggleContainer = toggle.parentElement as HTMLElement; | ||
| const loadingIndicator = toggleContainer.querySelector('.mcp-loading-indicator'); | ||
| if (loadingIndicator) { | ||
| loadingIndicator.remove(); | ||
| } | ||
|
|
||
| const errorMessage = error instanceof Error && error.message | ||
| ? error.message | ||
| : i18nString(UIStrings.connectionError); | ||
|
|
||
| // Show inline error instead of just snackbar | ||
| this.showConnectorError(connector, errorMessage, item, toggle); | ||
|
|
||
| // Set 90-second auto-reset timeout | ||
| const timeoutId = window.setTimeout(() => { | ||
| this.clearConnectorError(connector, item, toggle); | ||
| logger.info(`Auto-cleared error for ${connector.name} after 90 seconds`); | ||
| }, 90000); | ||
| this.#errorResetTimeouts.set(connector.id, timeoutId); | ||
|
|
||
| dismissProgressSnackbar(); | ||
|
|
||
| // Still show a snackbar for immediate feedback, but less prominent | ||
| const snackbar = Snackbars.Snackbar.Snackbar.show({ | ||
| message: i18nString(UIStrings.connectionFailed, {PH1: connector.name}), | ||
| closable: true, | ||
| }); | ||
| snackbar.dismissTimeout = 3000; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add request cancellation for in-flight OAuth requests
When a connection is in progress and the dialog is closed, the OAuth request continues in the background. Consider implementing an AbortController to cancel pending requests.
export class MCPConnectorsCatalogDialog {
// ... existing properties ...
+ #abortController: AbortController | null = null;
private async connectConnector(connector: MCPConnector, toggle: HTMLButtonElement, item: HTMLElement): Promise<void> {
+ // Cancel any existing connection attempt
+ if (this.#abortController) {
+ this.#abortController.abort();
+ }
+ this.#abortController = new AbortController();
+
// ... existing code ...
try {
// ... existing code ...
// Test the connection
- const connectionResults = await MCPRegistry.init(true);
+ const connectionResults = await MCPRegistry.init(true, { signal: this.#abortController.signal });
// ... rest of the code ...
} catch (error) {
+ if (error.name === 'AbortError') {
+ logger.info('Connection attempt cancelled');
+ return;
+ }
// ... existing error handling ...
} finally {
+ this.#abortController = null;
}
}
private close(): void {
+ // Cancel any pending connection
+ if (this.#abortController) {
+ this.#abortController.abort();
+ this.#abortController = null;
+ }
// ... existing cleanup ...
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts around lines
1252-1408, the OAuth connection flow starts network work
(MCPRegistry.init/refresh) but doesn't provide a way to cancel it if the dialog
is closed; add an AbortController to cancel in-flight requests and handle aborts
gracefully. Create an AbortController before the try block, pass
controller.signal into the MCPRegistry.init/refresh (and any underlying
fetch/HTTP call that supports a signal), and store the controller so the
dialog's close/destroy handler can call controller.abort(); in the catch block
detect aborts (error.name === 'AbortError' or similar) and treat them as
user-cancelled (skip showing error UI/snackbars or reverting state
unnecessarily), and finally ensure the controller reference is cleared after
success/failure to avoid leaks.
|
Claude finished @tysonthomas9's task —— View job Code Review: MCP OAuth Support and Agent Framework UpdatesReview Progress
Overall AssessmentThis is a substantial and well-architected addition that introduces OAuth support for MCP connections and a comprehensive agent framework. The code demonstrates good security practices, comprehensive error handling, and thoughtful architecture. However, there are several areas that need attention before production deployment. Critical Security IssuesOAuth Implementation Security✅ Strengths:
Template Injection Vulnerabilities🚨 Critical Issues:
These should use Agent Framework Architecture✅ Well-designed:
Performance Analysis
✅ Performance Positives:
Error Handling✅ Good error handling:
Test Coverage✅ Good coverage:
Code Organization Issues
Specific RecommendationsHigh Priority Fixes
Security Improvements
Performance Optimizations
Code Quality
ConclusionThis PR introduces valuable functionality with generally solid architecture and security practices. The main concerns are around production readiness (debugging code, performance optimization) and a few critical security fixes for template injection. The OAuth implementation is well-designed but needs some hardening. The agent framework is comprehensive and well-structured. Recommendation: Address the critical template injection issues and remove debugging code before merging. The other improvements can be tackled in follow-up PRs. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (18)
front_end/panels/ai_chat/tracing/LangfuseProvider.ts (6)
60-63: Approve structured logging approach with security improvement needed.The transition from console logging to structured logging enhances observability and debugging capabilities. However, exposing the public key in logs (even partially) poses a potential security risk.
Remove the public key from the logging payload:
- logger.info('Initializing Langfuse provider', { - endpoint: this.endpoint, - publicKey: this.publicKey.substring(0, 10) + '...' - }); + logger.info('Initializing Langfuse provider', { + endpoint: this.endpoint + });
562-567: Remove duplicate logging statements.There are duplicate log statements that provide the same information, which creates noise in the logs and may impact performance.
Remove the duplicate logging calls:
- logger.debug('Adding event to buffer', { - eventType: event.type, - eventId: event.id, - bufferSize: this.eventBuffer.length + 1 - }); this.eventBuffer.push(event); if (this.eventBuffer.length >= this.batchSize) { logger.debug(`Buffer full, triggering auto-flush`); - logger.debug('Buffer full, triggering auto-flush'); this.flush().catch(error => {Also applies to: 571-573
403-404: Remove duplicate success logging.There are two nearly identical log messages for successful flush operations that create redundancy in the logs.
Remove one of the duplicate log statements:
await this.sendBatch(events); - logger.debug(`Flushed ${events.length} events to Langfuse`); logger.debug(`Successfully flushed ${events.length} events`);
407-407: Consider removing console.error fallback.With the structured logger now in place, the console.error fallback may be redundant since logger.error should handle error output appropriately.
Consider removing the console.error fallback:
logger.error('Failed to flush events to Langfuse', error); - console.error(`[HIERARCHICAL_TRACING] LangfuseProvider: Failed to flush events:`, error);
522-524: Consider removing console.error fallback in serialization.Similar to the flush error handling, the console.error fallback may be redundant with the structured logger in place.
Consider removing the console.error fallback:
logger.error('Failed to serialize batch for Langfuse ingestion', error); - console.error(`[HIERARCHICAL_TRACING] LangfuseProvider: Serialization failed:`, error); throw new Error(`Failed to serialize tracing data: ${error instanceof Error ? error.message : String(error)}`);
631-633: Remove duplicate cleanup logging.There's a duplicate log statement for observation cleanup that creates redundant log entries.
Remove the duplicate logging call:
if (cleaned > 0) { logger.info(`Cleaned up ${cleaned} old observations from store`); - logger.debug(`Cleaned up ${cleaned} old observations`); }front_end/panels/ai_chat/tools/Tools.ts (1)
1432-1455: Well-implemented abortable sleep utility.The inline sleep implementation in the WaitTool properly handles abortion with cleanup logic and appropriate error handling. The implementation follows the same pattern as the utility function defined later in the file.
Consider using the
abortableSleeputility function defined at the bottom of the file instead of the inline implementation for consistency:- const sleep = (ms: number) => new Promise<void>((resolve, reject) => { - if (!ms) return resolve(); - const timer = setTimeout(() => { - cleanup(); - resolve(); - }, ms); - const onAbort = () => { - clearTimeout(timer); - cleanup(); - reject(new DOMException('The operation was aborted', 'AbortError')); - }; - const cleanup = () => { - signal?.removeEventListener('abort', onAbort); - }; - if (signal) { - if (signal.aborted) { - clearTimeout(timer); - cleanup(); - return reject(new DOMException('The operation was aborted', 'AbortError')); - } - signal.addEventListener('abort', onAbort, { once: true }); - } - }); + // Use the utility function for consistency // Wait for the specified duration (abortable) - await sleep(waitTime * 1000); + await abortableSleep(waitTime * 1000, signal);front_end/panels/ai_chat/mcp/MCPRegistry.ts (3)
11-14: Enhance interface documentationThe
RegistryServerinterface extendsMCPServerbut the relationship and purpose could be clearer. Consider adding JSDoc comments to explain whennamemight be undefined and the authentication type constraints.+/** + * Extended server configuration with registry-specific metadata + */ interface RegistryServer extends MCPServer { + /** Optional display name for the server */ name?: string; + /** Authentication mechanism for this server */ authType: 'bearer' | 'oauth'; }
168-220: Tool refresh logic needs simplificationThe
refreshToolsForServermethod has overly complex retry logic with nested setTimeout calls. Consider using the existing retry infrastructure or simplifying the approach.private async refreshToolsForServer(serverId: string, retryCount = 0): Promise<void> { - const maxRetries = 2; - const retryDelayMs = 2000; // 2 seconds + const retryConfig = this.getRetryConfig(); if (!this.client.isConnected(serverId)) { return; } - const server = this.servers.find(s => s.id === serverId); - if (!server) { - logger.warn('MCPRegistry: Server not found for tool refresh', { serverId }); - return; - } - try { - const tools = await this.client.listTools(serverId); - - // If we got tools, we're done - tools exist and will be registered by the next refresh - if (tools.length > 0) { - return; - } else if (retryCount < maxRetries) { - // No tools found, but maybe the server needs more time - - setTimeout(async () => { - try { - await this.refreshToolsForServer(serverId, retryCount + 1); - } catch (error) { - logger.warn('MCPRegistry: Delayed tool refresh failed', { serverId, error }); - } - }, retryDelayMs); - } else { - } + return; // Tools will be registered by the next refresh } catch (error) { - logger.warn('MCPRegistry: Failed to refresh tools for server', { - serverId, - attempt: retryCount + 1, - maxRetries, - error - }); - - if (retryCount < maxRetries) { - - setTimeout(async () => { - try { - await this.refreshToolsForServer(serverId, retryCount + 1); - } catch (retryError) { - logger.warn('MCPRegistry: Delayed tool refresh retry failed', { serverId, error: retryError }); - } - }, retryDelayMs); - } + logger.warn('MCPRegistry: Failed to refresh tools for server', { serverId, error }); } }
648-662: Verify tool counting performanceThe tool count calculation iterates through all registered tools and checks instance types for each server. This could become expensive with many tools/servers.
Consider caching tool counts per server or maintaining a separate counter that's updated during tool registration/deregistration:
// Add to class properties private toolCountByServer = new Map<string, number>(); // Update during tool registration this.toolCountByServer.set(serverId, (this.toolCountByServer.get(serverId) || 0) + 1); // Use in getStatus toolCount: this.toolCountByServer.get(s.id) || 0,front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts (1)
13-27: Consider making logo URLs configurable.The hardcoded URLs work for now, but consider making these configurable or loading them dynamically to support theming or different deployment environments.
config/gni/devtools_grd_files.gni (1)
915-916: Dual MCP SDKs in bundle (v1 and v2)Both mcp-sdk.js and mcp-sdk-v2.js are listed. If both are needed, OK. If v2 supersedes v1, consider removing the legacy bundle to reduce payload and avoid ambiguity at runtime.
front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts (3)
19-22: Docstring input signature is outdatedDescription lists only url and reasoning, but schema also supports product_query. Update the “Input” line to avoid confusion.
- - Input: { url (optional), reasoning (required) } + - Input: { url (optional), product_query (optional), reasoning (required) }
123-126: Stray backtick in product_query descriptionMinor typo can leak into UI/tooling.
- description: 'Optional specific product query` to refine the information extraction.' + description: 'Optional specific product query to refine the information extraction.'
134-141: Message template: newline handling and schema couplingTemplate works, but:
- It always appends two newlines before the directive.
- It directly references args.product_query; safe now that schema includes it, but keep parity if the field changes.
Optional: simplify whitespace.
- text: `${args.url ? `Product URL: ${args.url}\n` : ''}${args.product_query ? `Product Query: ${args.product_query}\n` : ''} - -Only return the product information, no other text. DO NOT HALLUCINATE`, + text: [ + args.url ? `Product URL: ${args.url}` : '', + args.product_query ? `Product Query: ${args.product_query}` : '', + 'Only return the product information, no other text. DO NOT HALLUCINATE' + ].filter(Boolean).join('\n'),front_end/panels/ai_chat/BUILD.gn (1)
148-161: mcp-sdk:bundle_v2 dependencyAdding v2 while keeping legacy bundle is okay if both are referenced. If only v2 is used by new UI, consider removing legacy dependency to reduce build size.
front_end/panels/ai_chat/core/ToolSurfaceProvider.ts (1)
248-261: ALL mode returns full union without capYou union base+MCP tools and return without applying maxToolsPerTurn. Consider capping to respect options (you do this in other branches).
- resultTools = uniqByName([...resultTools, ...mcpTools]); + resultTools = uniqByName([...resultTools, ...mcpTools]).slice(0, maxToolsPerTurn);front_end/panels/ai_chat/ui/AIChatPanel.ts (1)
300-306: Add MCP Connectors button: consider keyboard orderButton inclusion is fine. Consider placing it before Settings to group “config” actions together, and ensure tab order remains logical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
config/gni/devtools_grd_files.gni(6 hunks)front_end/panels/ai_chat/BUILD.gn(10 hunks)front_end/panels/ai_chat/agent_framework/AgentRunner.ts(9 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts(1 hunks)front_end/panels/ai_chat/core/AgentNodes.ts(26 hunks)front_end/panels/ai_chat/core/ConfigurableGraph.ts(2 hunks)front_end/panels/ai_chat/core/Logger.ts(1 hunks)front_end/panels/ai_chat/core/StateGraph.ts(3 hunks)front_end/panels/ai_chat/core/ToolNameMap.ts(3 hunks)front_end/panels/ai_chat/core/ToolSurfaceProvider.ts(5 hunks)front_end/panels/ai_chat/core/Types.ts(2 hunks)front_end/panels/ai_chat/mcp/MCPRegistry.ts(2 hunks)front_end/panels/ai_chat/mcp/__tests__/MCPIntegration.test.ts(1 hunks)front_end/panels/ai_chat/tools/Tools.ts(15 hunks)front_end/panels/ai_chat/tracing/LangfuseProvider.ts(12 hunks)front_end/panels/ai_chat/ui/AIChatPanel.ts(9 hunks)front_end/panels/ai_chat/ui/ChatView.ts(8 hunks)front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts(1 hunks)front_end/panels/ai_chat/ui/mcp/mcpConnectorsCatalogDialog.css(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- front_end/panels/ai_chat/agent_framework/AgentRunner.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- front_end/panels/ai_chat/core/ToolNameMap.ts
- front_end/panels/ai_chat/ui/ChatView.ts
- front_end/panels/ai_chat/core/StateGraph.ts
- front_end/panels/ai_chat/mcp/tests/MCPIntegration.test.ts
- front_end/panels/ai_chat/core/Types.ts
🧰 Additional context used
🧬 Code graph analysis (9)
front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts (2)
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)
AgentToolConfig(97-184)ConfigurableAgentArgs(241-256)front_end/panels/ai_chat/core/Constants.ts (1)
MODEL_SENTINELS(85-88)
front_end/panels/ai_chat/tracing/LangfuseProvider.ts (1)
front_end/panels/ai_chat/core/Logger.ts (1)
error(254-303)
front_end/panels/ai_chat/mcp/MCPRegistry.ts (2)
front_end/panels/ai_chat/LLM/LLMTypes.ts (1)
RetryConfig(26-32)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (1)
ToolRegistry(189-236)
front_end/panels/ai_chat/ui/AIChatPanel.ts (2)
front_end/panels/ai_chat/mcp/MCPRegistry.ts (1)
MCPRegistry(682-682)front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts (1)
MCPConnectorsCatalogDialog(164-1024)
front_end/panels/ai_chat/core/ToolSurfaceProvider.ts (5)
front_end/panels/ai_chat/tools/Tools.ts (1)
Tool(40-49)front_end/panels/ai_chat/ui/AIChatPanel.ts (6)
logger(964-1057)logger(1348-1418)logger(1568-1581)logger(1625-1628)error(1937-1950)AIChatPanel(326-2438)front_end/panels/ai_chat/LLM/LLMClient.ts (1)
LLMClient(49-385)front_end/panels/ai_chat/core/State.ts (1)
AgentState(29-36)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (1)
ToolRegistry(189-236)
front_end/panels/ai_chat/core/ConfigurableGraph.ts (1)
front_end/panels/ai_chat/core/State.ts (1)
AgentState(29-36)
front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts (2)
front_end/panels/ai_chat/core/Logger.ts (2)
createLogger(316-318)error(254-303)front_end/panels/ai_chat/mcp/MCPRegistry.ts (1)
MCPRegistry(682-682)
front_end/panels/ai_chat/core/AgentNodes.ts (4)
front_end/panels/ai_chat/core/State.ts (1)
AgentState(29-36)front_end/panels/ai_chat/core/ToolSurfaceProvider.ts (1)
ToolSurfaceProvider(211-348)front_end/panels/ai_chat/models/ChatTypes.ts (2)
ModelChatMessage(41-50)ToolResultMessage(53-63)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)
ToolRegistry(189-236)ConfigurableAgentTool(306-544)
front_end/panels/ai_chat/tools/Tools.ts (3)
front_end/panels/ai_chat/mcp/MCPRegistry.ts (1)
sleep(161-163)front_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.ts (1)
sleep(46-61)front_end/panels/ai_chat/LLM/LLMErrorHandler.ts (1)
sleep(337-339)
🪛 ast-grep (0.39.5)
front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts
[warning] 276-276: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: this.#connectorsContainer.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 376-376: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: errorContainer.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 483-483: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: errorContainer.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (83)
front_end/panels/ai_chat/core/Logger.ts (1)
100-100: LGTM! Visibility change enables external access.The change from
privatetostaticmakes theisDevelopmentmethod publicly accessible, which is consistent with its usage on Line 323 where it's accessed via bracket notation (Logger['isDevelopment']()) to configure the logger for development environments.front_end/panels/ai_chat/tracing/LangfuseProvider.ts (4)
114-120: Excellent structured logging implementation.The replacement of console logging with structured logger calls significantly improves debugging capabilities by providing consistent, filterable, and contextual log messages. The added context includes relevant IDs, types, and metadata that will be valuable for troubleshooting.
Also applies to: 202-208, 215-220, 233-239, 319-329
131-145: Well-implemented event type mapping.The explicit mapping from observation types to Langfuse event types with a sensible fallback ensures proper categorization of events in the tracing system.
360-370: Robust flush mechanism with improved observability.The logging additions provide excellent visibility into the flush process, including buffer states, event type summaries, and error handling. The atomic buffer operations prevent race conditions while maintaining chronological order during error recovery.
Also applies to: 384-414
547-582: Enhanced buffer management with detailed logging.The structured logging provides comprehensive visibility into buffer operations, overflow conditions, and auto-flush triggers. This will significantly aid in debugging buffer-related issues and optimizing batch sizes.
front_end/panels/ai_chat/core/ConfigurableGraph.ts (2)
71-75: AbortSignal implementation looks correct.The abort handling is properly implemented with appropriate error messages and consistent DOMException throwing. The signal check and error throwing follow the expected pattern for cancellation in web APIs.
92-96: Consistent abort handling across different code paths.The dummy error node correctly implements the same abort pattern as the tool executor node, ensuring consistent cancellation behavior throughout the graph.
front_end/panels/ai_chat/tools/Tools.ts (8)
61-61: LGTM - AbortSignal added to LLMContext.The addition of
abortSignal?: AbortSignalto the LLMContext interface enables cancellation support across all tool flows.
893-893: Parameter name change affects function signature.The NavigateBackTool execute method signature has changed from
_ctx?: LLMContexttoctx?: LLMContext, indicating the context parameter is now actively used for abort signal handling.
943-951: Proper abort signal integration in navigation polling.The abort signal is correctly integrated into the navigation polling loop, checking for cancellation before each delay and throwing the appropriate DOMException when aborted.
1978-1978: Proper abort signal propagation in DOM stability wait.The abort signal is correctly passed to the waitForDOMStability method, enabling cancellation during the post-action stabilization period.
2415-2463: Comprehensive abort handling in DOM stability methods.All the DOM stability wait methods (
waitForDOMStability,waitForDocumentReady,waitForDOMMutationStability) properly check for abort signals and throw appropriate exceptions. The implementation is consistent and thorough.
3148-3148: Tool name change reflects updated functionality.The NodeIDsToURLsTool name has been updated from
get_urls_from_nodeidstonode_ids_to_urls, which is more consistent with the naming convention used elsewhere in the codebase.
3504-3519: Well-implemented abortable sleep utility function.The
abortableSleeputility function is properly implemented with:
- Immediate resolution for zero delays
- Proper cleanup of event listeners
- Consistent error handling with DOMException
- Check for already-aborted signals
This utility can be reused across multiple tools for consistent abort handling.
2059-2060: Verify abortableSleep usage and handle the non-abortable sleep helper
- abortableSleep is defined at front_end/panels/ai_chat/tools/Tools.ts:3504 and is used at lines 950, 2060, 2162, 2433, 2456, 2543.
- A separate non-abortable sleep helper exists at front_end/panels/ai_chat/tools/Tools.ts:1433 — replace it with abortableSleep or justify/ document why a non-abortable sleep is required.
front_end/panels/ai_chat/core/AgentNodes.ts (17)
22-22: Import for AgentDescriptorRegistry correctly added.The new import for AgentDescriptorRegistry aligns with the enhanced metadata tracking capabilities mentioned in the AI summary.
38-44: Consistent AbortSignal implementation in AgentNode.The AbortSignal handling in the AgentNode.invoke method follows the same pattern as other nodes, properly checking for abortion and throwing the appropriate DOMException.
107-112: Strategic abort check placement before LLM call.The abort signal check is strategically placed before the potentially long-running LLM call, allowing for timely cancellation before expensive operations.
126-162: Enhanced tracing with agent descriptor metadata.The tracing observation creation now includes agent descriptor metadata when available, providing richer context for debugging and monitoring. The conditional metadata inclusion is well-structured.
202-208: Tool name mapping for consistent sanitization.The creation of the
originalToSanitizedmapping ensures consistent tool name resolution between the agent's tool calls and the actual tool execution, addressing potential naming conflicts with MCP tools.
210-210: Passing tool name mapping to message conversion.The
originalToSanitizedmapping is correctly passed to the message conversion method to ensure tool names are consistently resolved.
295-299: Multi-strategy tool name resolution.The tool name resolution logic implements a fallback strategy with original name, sanitized name, and resolved original name, ensuring robust tool identification across different naming schemes.
516-540: Comprehensive tool mapping with debug logging.The tool mapping logic covers multiple name variants (original, sanitized, resolved) and includes helpful debug logging for MCP tools. This should resolve tool resolution issues mentioned in the PR.
561-567: Abort handling in ToolExecutorNode.The ToolExecutorNode properly implements abort checking consistent with other nodes in the graph.
586-647: Robust tool resolution with fallback strategies.The tool resolution logic is comprehensive, trying multiple name variants and providing detailed debug logging. The fallback to direct ToolRegistry lookup is a good safety measure.
648-709: Graceful handling of missing tools.When a tool is not found, the code gracefully creates an error result and returns to the agent for retry rather than crashing the entire graph. The tracing of missing tool events will help with debugging tool resolution issues.
716-753: Enhanced tracing for configurable agents.The span creation includes additional metadata for configurable agents, incorporating descriptor information when available. This provides better observability for agent execution.
807-812: Abort check before tool execution.The abort signal is properly checked before executing the tool, allowing for timely cancellation of potentially expensive operations.
816-825: Enhanced tool execution context.The tool execution includes enhanced context with abort signal and agent descriptor information, providing tools with the necessary context for proper operation and cancellation support.
955-959: Proper abort error propagation.The code correctly propagates AbortError exceptions to allow the graph to handle cancellation gracefully, while converting other errors to tool result errors.
1037-1043: Consistent abort handling in FinalNode.The FinalNode implements the same abort checking pattern as other nodes for consistency, even though it's less likely to be long-running.
441-441: originalToSanitized usage verified — no action required.
convertChatMessagesToLLMMessages uses originalToSanitized when converting MODEL tool actions (fnName = originalToSanitized?.[msg.toolName!] || ToolNameMap.getSanitized(...)); the caller builds the originalToSanitized map from tools and passes it into the method, so behavior is correct.front_end/panels/ai_chat/mcp/MCPRegistry.ts (13)
4-5: LGTM - Clean migration to MCP SDK v2The import updates correctly migrate from the previous MCP SDK version to v2, maintaining type safety with the proper TypeScript imports.
16-24: Well-structured connection result interfaceThe
ConnectionResultinterface provides comprehensive connection status with proper error categorization and retry tracking. The optional fields are appropriately typed.
64-73: Robust retry configuration with sensible defaultsThe retry configuration implementation provides good defaults with a reasonable max delay calculation. The 10x multiplier with 10-second minimum ensures connections don't get stuck with excessively long delays.
78-116: Enhanced error categorization with SSE contextThe improved error categorization properly handles SSE-specific scenarios and OAuth context. The logic correctly maps HTTP status codes to appropriate error categories.
144-147: Conservative retry policyThe retry logic correctly avoids retrying authentication and configuration errors, focusing on transient network and server issues. This prevents infinite retry loops on non-recoverable errors.
225-241: Well-implemented event tracking with size limitsThe connection event tracking properly maintains a bounded list and persists events to localStorage. The implementation correctly handles the circular buffer behavior.
246-277: Robust persistence with error handlingThe save/load connection events methods properly handle serialization errors and data validation. The fallback to empty array on load failure is appropriate.
303-430: Comprehensive connection retry implementationThe
connectWithRetrymethod provides excellent retry logic with proper error categorization, event tracking, and exponential backoff. The integration with tool refresh and auth error clearing is well thought out.
432-487: Improved initialization with connection resultsThe enhanced
initmethod properly handles server lifecycle, loads persisted events, and returns detailed connection results. The non-interactive mode handling for OAuth is appropriate.
535-572: Validate the fixed duplicate tool naming logicThe tool naming logic addresses the previous off-by-one issue. However, verify that the
occurrenceCountstarts at 0 and increments correctly to avoid any remaining edge cases.The numeric suffix logic now correctly uses a monotonic counter starting at 0, which should resolve the previous off-by-one issue where the second occurrence became
_3.#!/bin/bash # Verify the tool naming logic handles duplicates correctly echo "Checking tool naming logic in MCPRegistry.ts" # Look for the corrected duplicate naming logic rg -nA 10 -B 5 "usedNames.*get.*baseName" front_end/panels/ai_chat/mcp/MCPRegistry.ts # Check for any remaining off-by-one patterns rg -n "currentCount.*\+.*1" front_end/panels/ai_chat/mcp/MCPRegistry.ts || echo "No off-by-one patterns found"
581-608: Clean reconnection with proper cleanupThe
reconnectmethod appropriately disconnects before reconnecting, clears errors on success, and triggers a refresh. The error handling and cleanup are well implemented.
614-623: Enhanced dispose with event trackingThe dispose method now properly tracks disconnection events while maintaining graceful error handling during cleanup.
669-669: Good integration of connection eventsThe connection events are properly exposed through the status interface, providing valuable debugging information to the UI.
front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts (9)
1-11: LGTM! Good module structure and imports.The imports are well-organized and properly typed. The logger setup follows the established pattern in the codebase.
192-200: LGTM! Proper CSS injection pattern.Using the imported CSS module follows the established pattern from past review comments and addresses the previous concern about inline styles.
276-277: innerHTML usage is safe here - clearing container.The static analysis warning about XSS is a false positive in this case. The code is clearing the container with an empty string, not inserting user content.
Using
innerHTML = ''for clearing is safe as it doesn't involve user input.
324-444: Robust error handling with detailed UI feedback.Excellent implementation of error categorization and user-friendly error display. The error type detection logic covers common failure scenarios (401/403 for auth, 404 for config, 5xx for server errors).
376-377: innerHTML usage is safe here - clearing error containers.Similar to the earlier case, these
innerHTML = ''calls are for clearing error containers and don't pose XSS risks since they're not inserting user-provided content.Also applies to: 483-484
778-845: Add request cancellation support for OAuth flows.The past review comment about adding AbortController for request cancellation is still valid and important for user experience. When users close the dialog during OAuth flows, the connection attempt should be cancelled.
847-1004: Comprehensive connection flow with proper error handling.The implementation handles the full OAuth connection lifecycle well:
- Proper state management (connecting → connected/error)
- User feedback via snackbars and loading indicators
- Rollback on failures
- Auto-clearing error states after 90 seconds
The connection testing via
MCPRegistry.init(true)and result validation ensures only successful connections are marked as connected.
988-993: Good UX: Auto-clear errors after timeout.The 90-second timeout to auto-clear errors is a nice touch that prevents the UI from staying in error states indefinitely.
1012-1024: Proper cleanup in close method.The cleanup of timeout callbacks prevents memory leaks. Good defensive programming.
front_end/panels/ai_chat/ui/mcp/mcpConnectorsCatalogDialog.css (9)
1-17: LGTM! Well-structured dialog container styles.The dialog uses CSS custom properties consistently and has appropriate responsive sizing with viewport constraints.
18-50: LGTM! Accessible header with proper interactive states.The header layout and close button styling include proper hover states and accessible dimensions (28x28px for the close button).
51-90: LGTM! Search and status sections are well-styled.The search input has proper focus states and the status section provides clear visual hierarchy.
91-129: LGTM! Collapsible categories with smooth transitions.The category toggle animation (90-degree rotation) and collapsed state handling provide good UX.
130-183: LGTM! Connector items with clear state differentiation.The different states (connected, connecting, error) are clearly differentiated with color coding and visual indicators.
184-239: LGTM! Toggle switch with smooth animations.The toggle switch implementation includes proper state styling and smooth transitions. The knob animation (
translateX(16px)) provides clear visual feedback.
257-333: LGTM! Comprehensive error UI styling.The error container styling provides clear visual hierarchy with proper color coding, collapsible details, and action buttons.
334-354: LGTM! Animated loading indicator with CSS keyframes.The dots animation using CSS keyframes is efficient and provides good visual feedback during connection attempts.
399-411: LGTM! Custom scrollbar styling maintains theme consistency.The scrollbar styling matches the overall theme and provides good visual feedback on hover.
config/gni/devtools_grd_files.gni (4)
651-657: GRD includes CSS and new MCP dialogs — ensure BOTH bundle modes coveredBundled GRD references mcpConnectorsCatalogDialog.css.js and MCP dialogs. Make sure corresponding unbundled outputs are either:
- Listed in grd_files_unbundled_sources, or
- Emitted via ai_chat_release_css_metadata/js metadata and wired in root checks.
If relying on metadata only, no action; otherwise add explicit unbundled entries to avoid release checks failing.
662-676: New core artifacts must match TS sourcesAgentDescriptorRegistry.js and BuildConfig.js entries look correct. Confirm these compile products exist in the generated output dir for both bundle modes during the GRD check.
2276-2294: ESM AJV + MCP ESM dist: both ESM wrappers are required — keep them.
BUILD.gn and README.chromium reference ajv/dist/ajv.bundle.js and ajv/dist/ajv-esm.js (ajv-esm imports ./ajv.bundle.js), and dist/zod/zod-esm.js forwards/mirrors the top-level zod wrapper to satisfy relative imports used by dist/esm modules — these duplicates are intentional; do not remove.
2280-2291: Ensure GRD ↔ BUILD alignment for MCP ESM distconfig/gni/devtools_grd_files.gni contains front_end/third_party/mcp-sdk/dist/esm/* entries (≈ lines 2279–2291); front_end/third_party/mcp-sdk/BUILD.gn lists the corresponding dist/esm paths as relative entries. Confirm the devtools_pre_built(":mcp-sdk") / devtools_entrypoint("bundle_v2") target actually exports those repo-relative paths exactly so GRD checks won't fail.
front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts (2)
107-112: Tools list now matches the system prompt—LGTMIncluding extract_data and search_content resolves earlier capability gaps.
107-112: Verified: 'extract_data' and 'search_content' are registered.
Both are registered via ToolRegistry.registerToolFactory in front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts (lines 41 and 46).front_end/panels/ai_chat/BUILD.gn (6)
14-15: CSS source added—ensure matching GRD entryYou’ve added ui/mcp/mcpConnectorsCatalogDialog.css to generate_css and mirrored its .css.js in GRD bundled list; good. Ensure unbundled flow is satisfied via ai_chat_release_css_metadata (added below).
57-58: AgentDescriptorRegistry exposure—LGTMPublic exposure aligns with new registry usage across the panel.
99-113: Agent sources included—LGTMAll new agent configs are included; matches GRD additions.
199-214: _ai_chat_sources metadata parityJS metadata list mirrors the sources—good for release checks.
343-344: CSS metadata parity—LGTMEnsures the new CSS is accounted for in release checks.
401-414: Unit tests expanded — verify presence & buildfront_end/panels/ai_chat/BUILD.gn (lines 401–414) lists new test files; confirm each listed test file exists in the repo and successfully compiles under the testonly targets.
front_end/panels/ai_chat/core/ToolSurfaceProvider.ts (3)
34-66: Async MCP tool discovery—LGTMAwaiting ensureToolsRegistered() before reading status avoids races; debug logs are useful.
340-346: Final cap applied—LGTMCapping combined tools by maxToolsPerTurn in router path is correct.
302-327: selectedToolNames is assigned elsewhere — reuse branch is reachablestate.context.selectedToolNames is written in front_end/panels/ai_chat/core/AgentNodes.ts:180 and in front_end/panels/ai_chat/core/ToolSurfaceProvider.ts (e.g. lines 127, 137, 336); tests also set it. The reuse block is not inert — either keep it or consolidate/persist the selection lifecycle so assignments are unambiguous.
Likely an incorrect or invalid review comment.
front_end/panels/ai_chat/ui/AIChatPanel.ts (5)
166-170: New i18n string for MCP Connectors—LGTMString name and usage look consistent.
224-225: Toolbar input contract updated—LGTMonMCPConnectorsClick is plumbed through properly.
811-813: Init MCP with auto-login disabled—LGTMUsing init(false) at startup aligns with OAuth UX.
1961-1964: Cancel running agent on hide—LGTMPrevents orphaned executions.
2100-2112: Catalog close refresh: also call refresh()You call MCPRegistry.init(true) on close; consider also calling refresh() (like elsewhere) to repopulate tools immediately. [suggest_minor_issue]
- await MCPRegistry.init(true); - logger.info('MCP registry refreshed after catalog closed'); + await MCPRegistry.init(true); + await MCPRegistry.refresh(); + logger.info('MCP registry refreshed after catalog closed');
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
front_end/panels/ai_chat/core/Logger.ts (1)
100-106: Harden dev detection; avoid ReferenceError outside the browser (SSR/tests) and cover IPv6 loopback.
locationcan be undefined at import time in Node/JSDOM, causing crashes. Also include::1and common dev TLDs.Apply this diff:
- static isDevelopment(): boolean { - // Check for development indicators - return location.hostname === 'localhost' || - location.hostname.includes('127.0.0.1') || - location.port === '8090' || - location.port === '8000'; - } + static isDevelopment(): boolean { + // Safe across browser/SSR/test + const hasLocation = typeof window !== 'undefined' && typeof window.location !== 'undefined'; + if (!hasLocation) { + // Prefer explicit env in non‑browser contexts + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const nodeEnv = (globalThis as any)?.process?.env?.NODE_ENV; + return Boolean(nodeEnv && nodeEnv !== 'production'); + } + const { hostname, port } = window.location; + return hostname === 'localhost' || + hostname === '127.0.0.1' || + hostname === '::1' || + hostname.endsWith('.local') || + hostname.endsWith('.test') || + port === '8090' || + port === '8000'; + }front_end/panels/ai_chat/tracing/LangfuseProvider.ts (4)
431-463: Circular-reference bug in sanitizeObservationData (new WeakSet per recursion → infinite recursion on self-cycles)
seenis re-created on each recursive call, soo.self = owill blow the stack. Thread a shared WeakSet through recursion and short‑circuit on already‑seen objects. Also reuse it for arrays.Apply:
- private sanitizeObservationData(data: any): any { - if (data === null || data === undefined) return data; - - // Handle primitive types - if (typeof data !== 'object') { - return typeof data === 'string' && data.length > 5000 ? - data.substring(0, 5000) + '... [truncated]' : data; - } - - // Handle arrays - if (Array.isArray(data)) { - return data.slice(0, 100).map(item => this.sanitizeObservationData(item)); // Limit array size - } - - // Handle objects - create a new clean object - const sanitized: any = {}; - const seen = new WeakSet(); - - for (const [key, value] of Object.entries(data)) { + private sanitizeObservationData(data: any, seen: WeakSet<object> = new WeakSet()): any { + if (data === null || data === undefined) return data; + if (typeof data !== 'object') { + return typeof data === 'string' && data.length > 5000 + ? data.substring(0, 5000) + '... [truncated]' + : data; + } + // Circulars + const obj = data as object; + if (seen.has(obj)) return '[Circular Reference]'; + seen.add(obj); + + // Arrays + if (Array.isArray(data)) { + return (data as any[]).slice(0, 100).map(item => this.sanitizeObservationData(item, seen)); + } + + // DOM/Promise/Function sentinels at object root too + if ((data as any).nodeType || (data as any).constructor?.name === 'HTMLElement') return '[DOM Element]'; + if ((data as any).constructor?.name?.includes('Promise')) return '[Promise]'; + if ((data as any).constructor?.name?.includes('Function')) return '[Function]'; + + // Plain objects + const sanitized: any = {}; + for (const [key, value] of Object.entries(data)) { // Skip problematic keys that often cause circular references if (key === 'agentSession' && value && typeof value === 'object') { // Keep only essential agentSession data sanitized[key] = { sessionId: (value as any).sessionId, agentName: (value as any).agentName, status: (value as any).status, messageCount: (value as any).messages?.length || 0, startTime: (value as any).startTime, endTime: (value as any).endTime }; continue; } - // Skip DOM elements and functions - if (value && typeof value === 'object') { - if (seen.has(value)) continue; // Skip circular refs - if ((value as any).nodeType || (value as any).constructor?.name === 'HTMLElement') continue; - if ((value as any).constructor?.name?.includes('Promise')) continue; - if ((value as any).constructor?.name?.includes('Function')) continue; - seen.add(value); - } - - // Recursively sanitize nested objects - sanitized[key] = this.sanitizeObservationData(value); + // Skip DOM elements and functions + if (value && typeof value === 'object') { + if ((value as any).nodeType || (value as any).constructor?.name === 'HTMLElement') { sanitized[key] = '[DOM Element]'; continue; } + if ((value as any).constructor?.name?.includes('Promise')) { sanitized[key] = '[Promise]'; continue; } + if ((value as any).constructor?.name?.includes('Function')) { sanitized[key] = '[Function]'; continue; } + } + // Recursively sanitize nested objects with shared WeakSet + sanitized[key] = this.sanitizeObservationData(value as any, seen); } return sanitized; }
168-176: Sanitize modelParameters and usage payloads before enqueueing
modelParameters(and nested fields) may contain large/circular values. Use the sanitizer for parity with input/output/metadata.Apply:
- if (observation.modelParameters) { - body.modelParameters = observation.modelParameters; - } + if (observation.modelParameters) { + body.modelParameters = this.sanitizeObservationData(observation.modelParameters); + } @@ - if (updatedObservation.modelParameters) { - body.modelParameters = updatedObservation.modelParameters; - } + if (updatedObservation.modelParameters) { + body.modelParameters = this.sanitizeObservationData(updatedObservation.modelParameters); + }Also applies to: 172-174, 286-299, 290-292
527-540: Add network timeout to prevent hung flushes
fetchhas no built-in timeout; a stalled request holdsflushPromiseand starves subsequent flushes. Abort after a reasonable deadline and surface a clear error.Apply:
- const response = await fetch(`${this.endpoint}/api/public/ingestion`, { + const controller = new AbortController(); + const timeoutId = window.setTimeout(() => controller.abort(), 10000); // 10s + let response: Response; + try { + response = await fetch(`${this.endpoint}/api/public/ingestion`, { method: 'POST', headers: { 'Content-Type': 'application/json', 'Authorization': 'Basic ' + btoa(`${this.publicKey}:${this.secretKey}`) - }, - body: bodyString - }); + }, + body: bodyString, + signal: controller.signal, + }); + } finally { + window.clearTimeout(timeoutId); + }
50-55: Critical: remove Langfuse secretKey from front-end — do not store or transmit secrets in browser codeConstructor stores
secretKey(front_end/panels/ai_chat/tracing/LangfuseProvider.ts: lines 50–55; also 527–533) and uses Basic Auth — this exposes the secret to end users. Use the LangfuseWeb/browser SDK with only the public key for client events, or proxy any requests requiring the secret through a trusted backend or privileged extension background service worker. Confirm this provider is only ever instantiated in a privileged, non-user-distributed environment; otherwise refactor to server-side ingestion and rotate the exposed key.front_end/panels/ai_chat/tools/Tools.ts (2)
1128-1159: Escape user-controlled strings in evaluate() to prevent code/selector injectionMultiple evaluate blocks embed raw strings (selector, query, xpath, direction). Use JSON.stringify-bound locals and then reference the local, not inline interpolation.
Apply targeted fixes:
@@ ClickElementTool.execute - const result = await target.runtimeAgent().invoke_evaluate({ - expression: `(() => { - const element = document.querySelector("${selector}"); + const result = await target.runtimeAgent().invoke_evaluate({ + expression: `(() => { + const selector = ${JSON.stringify(selector)}; + const element = document.querySelector(selector); if (!element) { return { success: false, - message: "Element not found with selector: ${selector}" + message: "Element not found with selector: " + selector }; } @@ - const result = await target.runtimeAgent().invoke_evaluate({ - expression: `(() => { - const query = "${query}"; - const limit = ${limit}; + const result = await target.runtimeAgent().invoke_evaluate({ + expression: `(() => { + const query = ${JSON.stringify(query)}; + const limit = ${limit}; @@ - const direction = "${direction}"; + const direction = ${JSON.stringify(direction)}; const amount = ${amount};@@ PerformActionTool (click pre-action details) - const escapedXPath = "${xpath.replace(/\"/g, '\\"')}"; - const element = document.evaluate(escapedXPath, document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue; + const xpath = ${JSON.stringify(xpath)}; + const element = document.evaluate(xpath, document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue;@@ PerformActionTool (fill/type verification) - const xpath = "${xpath.replace(/\"/g, '\\"')}"; + const xpath = ${JSON.stringify(xpath)}; const element = document.evaluate(xpath, document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue;@@ NodeIDsToURLsTool.execute - const element = document.evaluate("${xpath}", document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue; + const xpath = ${JSON.stringify(xpath)}; + const element = document.evaluate(xpath, document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue;Also applies to: 1205-1299, 1348-1373, 1908-1923, 2011-2027, 3182-3199
3461-3463: Fix type signature mismatches in getTools() unionThe union should reflect current tool arg types; otherwise TS invariance may fail.
- Tool<{ reasoning: string }, AccessibilityTreeResult | ErrorResult> | - Tool<{ method: string, nodeId: number, reasoning: string, args?: Record<string, unknown> | unknown[] }, PerformActionResult | ErrorResult> | + Tool<{ reasoning: string }, AccessibilityTreeResult | ErrorResult> | + Tool<{ method: string, nodeId: number | string, reasoning: string, args?: Record<string, unknown> | unknown[] }, PerformActionResult | ErrorResult> | @@ - Tool<{ seconds: number, reason?: string }, WaitResult | ErrorResult> | + Tool<{ seconds?: number, duration?: number, reason?: string, reasoning?: string }, WaitResult | ErrorResult> |Also applies to: 3472-3472
🧹 Nitpick comments (34)
front_end/panels/ai_chat/core/Logger.ts (3)
323-329: Use direct property access for the public method.Bracket access isn’t needed now that
isDevelopmentis public; dot access preserves typings and avoids stringly‑typed access.Apply this diff:
-if (Logger['isDevelopment']()) { +if (Logger.isDevelopment()) { Logger.configure({ level: LogLevel.DEBUG, includeTimestamp: true, includeSource: true, }); }
206-221: Map DEBUG/TRACE to console.debug for native filtering.This improves DevTools filtering without changing behavior (falls back to log when unavailable).
Apply this diff:
private getConsoleMethod(level: LogLevel): (...args: any[]) => void { switch (level) { case LogLevel.TRACE: - return console.log.bind(console); // Use console.log for visibility + return (console.debug ?? console.log).bind(console); case LogLevel.DEBUG: - return console.log.bind(console); // Use console.log for visibility + return (console.debug ?? console.log).bind(console); case LogLevel.INFO: return console.log.bind(console); case LogLevel.WARN: return console.warn.bind(console); case LogLevel.ERROR: return console.error.bind(console); default: return console.log.bind(console); } }
264-274: Make stack parsing more robust across browsers.Current regex misses “at fn (file:line:col)” format. Use a variant that tolerates optional function wrappers.
Apply this diff:
- const sourceMatch = stack.split('\n')[3]?.match(/at\s+(.+):(\d+):(\d+)/); + const sourceMatch = stack.split('\n')[3]?.match(/at\s+(?:.+\s+\()?(.+):(\d+):(\d+)\)?/);And similarly in
error():- const sourceMatch = stack.split('\n')[2]?.match(/at\s+(.+):(\d+):(\d+)/); + const sourceMatch = stack.split('\n')[2]?.match(/at\s+(?:.+\s+\()?(.+):(\d+):(\d+)\)?/);Also applies to: 146-153
front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts (2)
14-23: Doc mismatch: description omits product_query; align input docs with schema.The description’s “Input” bullet doesn’t list product_query, which exists in the schema. Keep docs and schema in sync.
Apply this diff:
- - Input: { url (optional), reasoning (required) } + - Input: { url (optional), product_query (optional), reasoning (required) }
116-133: Tighten schema: URL validation, string lengths, and fix stray backtick.
- Add format: 'uri' and minLength to url.
- Add minLength to product_query.
- Remove stray backtick in product_query description.
- Optional: include product_query in description (handled in a separate comment above).
Apply this diff:
properties: { url: { type: 'string', - description: 'Optional URL of the product page to navigate to before extracting information.' + description: 'Optional URL of the product page to navigate to before extracting information.', + format: 'uri', + minLength: 1 }, product_query: { type: 'string', - description: 'Optional specific product query` to refine the information extraction.' + description: 'Optional specific product query to refine the information extraction.', + minLength: 1 }, reasoning: { type: 'string', description: 'Reasoning for invoking this specialized e-commerce product information assistant.' },front_end/panels/ai_chat/tracing/LangfuseProvider.ts (5)
186-193: Avoid duplicating raw metadata at the event level; sanitize if you must include itYou already include sanitized
metadatainbody. Duplicating un-sanitized metadata atevent.metadatacan bloat payloads and leak sensitive fields. Either drop it or sanitize consistently.Apply one of:
- body, - metadata: observation.metadata + bodyor
- body, - metadata: observation.metadata + body, + metadata: observation.metadata ? this.sanitizeObservationData(observation.metadata) : undefinedAnd similarly for updates.
Also applies to: 306-313
405-408: Remove direct console.error and duplicate success logs; rely on Logger
Logger.erroralready prints to console (see Logger.ts). The extraconsole.errorlines and the duplicate successdebugadd noise.Apply:
- logger.debug(`Successfully flushed ${events.length} events`); - } catch (error) { - logger.error('Failed to flush events to Langfuse', error); - console.error(`[HIERARCHICAL_TRACING] LangfuseProvider: Failed to flush events:`, error); + } catch (error) { + logger.error('Failed to flush events to Langfuse', error); @@ - logger.error('Failed to serialize batch for Langfuse ingestion', error); - console.error(`[HIERARCHICAL_TRACING] LangfuseProvider: Serialization failed:`, error); + logger.error('Failed to serialize batch for Langfuse ingestion', error);Also applies to: 522-524
551-552: Trim redundant debug logs to reduce noiseThere are multiple duplicate or near-duplicate debug entries that make tracing harder:
- Buffer overflow: warn + debug repeating the same info
- “Adding event to buffer”: two separate debug calls
- “Buffer full, triggering auto-flush”: duplicated
- “Successfully updated observation” followed by “Updated observation”
- Cleanup: info + debug repeating same count
Apply:
- logger.debug(`Buffer overflow, discarded ${discarded} events`); @@ - logger.debug('Adding event to buffer', { - eventType: event.type, - eventId: event.id, - bufferSize: this.eventBuffer.length + 1 - }); @@ - logger.debug('Buffer full, triggering auto-flush'); @@ - logger.debug('Updated observation', { observationId, updates }); @@ - logger.debug(`Cleaned up ${cleaned} old observations`);Also applies to: 554-561, 562-566, 570-573, 578-582, 633-633, 319-329
610-612: Consider crypto-strong event IDs
Math.random()is fine for low collision risk, butcrypto.getRandomValuesyields stronger, uniform IDs.Apply:
- private generateEventId(): string { - return `${Date.now()}-${Math.random().toString(36).substring(2, 11)}`; - } + private generateEventId(): string { + const buf = new Uint8Array(8); + crypto.getRandomValues(buf); + const rand = Array.from(buf).map(b => b.toString(16).padStart(2, '0')).join(''); + return `${Date.now()}-${rand}`; + }
249-263: Action: Document Langfuse upsert semantics or switch to explicit update eventsLangfuse accepts re-sent "-create" events as idempotent upserts by observation id (updates replace the existing observation within a ~60-day window), but it also supports explicit "-update" event types and SDK .update() methods — explicit updates are the semantically correct way to express later changes.
File: front_end/panels/ai_chat/tracing/LangfuseProvider.ts — lines 249–263 and 306–313.
Either:
- Keep create-based upsert but add a comment/docs clarifying idempotent upsert behavior and the ~60-day retention caveat; OR
- Emit the corresponding "*-update" events / call the SDK update methods for updates.
front_end/panels/ai_chat/agent_framework/AgentRunner.ts (1)
624-630: Nice: descriptor metadata added to generation traces. Consider mirroring on tool spans.
For completeness, add the same descriptor fields to tool execution span metadata (success and error) so downstream analysis has uniform coverage.Apply this diff in both tool span updates:
@@ // on success (metadata block) - parentToolCallDecision: toolCallObservationId + parentToolCallDecision: toolCallObservationId, + ...(agentDescriptor ? { + agentVersion: agentDescriptor.version, + promptHash: agentDescriptor.promptHash, + toolsetHash: agentDescriptor.toolsetHash + } : {}) @@ // on error (metadata block) - parentToolCallDecision: toolCallObservationId + parentToolCallDecision: toolCallObservationId, + ...(agentDescriptor ? { + agentVersion: agentDescriptor.version, + promptHash: agentDescriptor.promptHash, + toolsetHash: agentDescriptor.toolsetHash + } : {})Also applies to: 695-701, 727-733
front_end/panels/ai_chat/tools/Tools.ts (4)
662-662: Template string bug prevents URL interpolationUses single quotes around a template literal; logs a literal "${url}".
- logger.info('Initiating navigation to: ${url}'); + logger.info(`Initiating navigation to: ${url}`);
893-893: Use appropriate log levels (avoid error for routine progress)These paths aren’t errors; they pollute error telemetry.
- logger.error('navigate_back', args); + logger.info('navigate_back', args); @@ - logger.error('Navigation completed, document ready state is complete'); + logger.info('Navigation completed, document ready state is complete'); @@ - logger.error('Still waiting for navigation to complete...'); + logger.debug('Still waiting for navigation to complete...'); @@ - logger.error('Navigation timed out after waiting for document ready state'); + logger.warn('Navigation timed out after waiting for document ready state');Also applies to: 963-964, 967-967, 972-972
1432-1455: Deduplicate sleep logic: reuse abortableSleep utilityInline abortable sleep duplicates the shared helper; reduces drift.
- const signal = ctx?.abortSignal; - const sleep = (ms: number) => new Promise<void>((resolve, reject) => { - if (!ms) return resolve(); - const timer = setTimeout(() => { - cleanup(); - resolve(); - }, ms); - const onAbort = () => { - clearTimeout(timer); - cleanup(); - reject(new DOMException('The operation was aborted', 'AbortError')); - }; - const cleanup = () => { - signal?.removeEventListener('abort', onAbort); - }; - if (signal) { - if (signal.aborted) { - clearTimeout(timer); - cleanup(); - return reject(new DOMException('The operation was aborted', 'AbortError')); - } - signal.addEventListener('abort', onAbort, { once: true }); - } - }); + const signal = ctx?.abortSignal; @@ - await sleep(waitTime * 1000); + await abortableSleep(waitTime * 1000, signal);Also applies to: 1476-1478
1689-1696: Guard optional iframes arraytreeResult.iframes may be undefined. Avoid
.mapon undefined.- const enhancedIframes = treeResult.iframes.map(iframe => ({ + const enhancedIframes = (treeResult.iframes ?? []).map(iframe => ({front_end/panels/ai_chat/mcp/MCPRegistry.ts (2)
192-200: Untracked setTimeout retries can outlive lifecycleUsing setTimeout for refresh retries creates orphan timers (possible leaks/races on dispose). Prefer awaited sleeps within the same call chain or track/clear timer IDs.
- setTimeout(async () => { - try { - await this.refreshToolsForServer(serverId, retryCount + 1); - } catch (error) { - logger.warn('MCPRegistry: Delayed tool refresh failed', { serverId, error }); - } - }, retryDelayMs); + await this.sleep(retryDelayMs); + await this.refreshToolsForServer(serverId, retryCount + 1); @@ - setTimeout(async () => { - try { - await this.refreshToolsForServer(serverId, retryCount + 1); - } catch (retryError) { - logger.warn('MCPRegistry: Delayed tool refresh retry failed', { serverId, error: retryError }); - } - }, retryDelayMs); + await this.sleep(retryDelayMs); + await this.refreshToolsForServer(serverId, retryCount + 1);Also applies to: 211-218
26-33: Avoid duplicating RetryConfig typeThis duplicates LLM/LLMTypes.ts RetryConfig with identical shape. Import the shared type to reduce drift.
-import type { MCPToolDef, MCPServer } from '../../../third_party/mcp-sdk/mcp-sdk-v2.js'; +import type { MCPToolDef, MCPServer } from '../../../third_party/mcp-sdk/mcp-sdk-v2.js'; +import type { RetryConfig } from '../LLM/LLMTypes.js'; @@ -interface RetryConfig { - maxRetries: number; - baseDelayMs: number; - maxDelayMs: number; - backoffMultiplier: number; - jitterMs: number; -} +// use shared RetryConfigfront_end/panels/ai_chat/core/ToolSurfaceProvider.ts (1)
68-75: LLM selection: resilient, but tighten JSON extractionRegex-based JSON extraction is OK; consider using a stricter parse (first/last bracket balance) to avoid partial matches in verbose models. Not blocking.
Also applies to: 115-124, 126-145
front_end/panels/ai_chat/core/StateGraph.ts (1)
55-55: Use logger instead of console.warnKeep logging consistent and test-friendly.
- console.warn(`Graph "${this.name}" started with entry point "${this.entryPoint}"`); + logger.warn(`Graph "${this.name}" started with entry point "${this.entryPoint}"`);front_end/panels/ai_chat/core/ConfigurableGraph.ts (1)
92-99: Same abort propagation for dummy unknown-type nodeMirror the abort reason handling here as well.
- if (signal?.aborted) { - logger.info('Dummy error node aborted'); - throw new DOMException('Execution was cancelled', 'AbortError'); - } + if (signal?.aborted) { + logger.info('Dummy error node aborted'); + throw (signal.reason instanceof Error + ? signal.reason + : new DOMException('Execution was cancelled', 'AbortError')); + }front_end/panels/ai_chat/ui/AIChatPanel.ts (4)
235-244: Remove commented‑out history button or track via TODOCommented UI blocks tend to linger. Prefer a TODO with an issue ID or remove until implemented.
300-305: Add aria-haspopup for dialog launcherImproves a11y semantics for a button that opens a dialog.
<devtools-button title=${i18nString(UIStrings.mcpConnectors)} aria-label=${i18nString(UIStrings.mcpConnectors)} + aria-haspopup=${'dialog'} .iconName=${'extension'} .jslogContext=${'ai-chat.mcp-connectors'} .variant=${Buttons.Button.Variant.TOOLBAR} @click=${input.onMCPConnectorsClick}></devtools-button>
1961-1964: Avoid swallowing a potentially rejected Promise from cancelRunUse a detached promise with explicit catch to prevent unhandled rejections.
- try { - this.#agentService.cancelRun(); - } catch {} + void this.#agentService.cancelRun()?.catch(() => {});
2100-2112: Refresh MCP registry after init to reflect new connectorsMirror startup flow (init + refresh) so the UI reflects newly added connectors immediately.
#onMCPConnectorsClick(): void { MCPConnectorsCatalogDialog.show({ onClose: async () => { // Refresh MCP registry when catalog is closed in case new connectors were added try { - await MCPRegistry.init(true); - logger.info('MCP registry refreshed after catalog closed'); + await MCPRegistry.init(true); + await MCPRegistry.refresh(); + logger.info('MCP registry refreshed after catalog closed'); } catch (error) { logger.error('Failed to refresh MCP registry after catalog closed', error); } } }); }front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts (4)
277-277: Avoid innerHTML for clearing nodesUse replaceChildren() to clear content; safer and faster than innerHTML = ''.
- this.#connectorsContainer.innerHTML = ''; + this.#connectorsContainer.replaceChildren(); @@ - errorContainer.innerHTML = ''; + errorContainer.replaceChildren(); @@ - errorContainer.innerHTML = ''; + errorContainer.replaceChildren();Also applies to: 378-378, 484-484
210-215: Improve accessibility for controls (labels and roles)Add an aria-label to the close button and make the toggle a proper switch with aria-checked; update it on connect/disconnect.
@@ - const closeButton = document.createElement('button'); + const closeButton = document.createElement('button'); closeButton.className = 'mcp-catalog-close'; closeButton.textContent = '×'; + closeButton.setAttribute('aria-label', i18nString(UIStrings.closeButton)); @@ - const toggle = document.createElement('button'); + const toggle = document.createElement('button'); toggle.className = 'mcp-toggle-switch'; + toggle.setAttribute('role', 'switch'); + toggle.setAttribute('aria-checked', String(isConnected)); @@ - toggle.classList.remove('connecting'); + toggle.classList.remove('connecting'); toggle.classList.add('enabled'); toggle.disabled = false; item.classList.remove('connecting'); item.classList.add('connected'); item.removeAttribute('aria-busy'); + toggle.setAttribute('aria-checked', 'true'); @@ - toggle.classList.remove('enabled'); + toggle.classList.remove('enabled'); item.classList.remove('connected'); + toggle.setAttribute('aria-checked', 'false');Also applies to: 639-649, 920-926, 809-811
324-444: De‑duplicate error UI constructionThe error block is built twice (showConnectorError and createConnectorItem). Extract a private helper (e.g., #renderErrorBlock(container, info)) to DRY and reduce divergence.
Also applies to: 696-772
318-322: Connection counter accuracy checkYou count connected providers via this.#existingProviders.length against MCP_CONNECTORS.length. If providers may include endpoints not present in the catalog (custom entries), the ratio can exceed 100%. Consider counting only providers whose endpoint matches a catalog connector.
- const connectedCount = this.#existingProviders.length; + const catalogEndpoints = new Set(MCP_CONNECTORS.map(c => c.endpoint)); + const connectedCount = this.#existingProviders.filter(p => catalogEndpoints.has(p.endpoint)).length;front_end/panels/ai_chat/BUILD.gn (1)
152-161: Confirm dual MCP SDK bundles are requiredBoth "../../third_party/mcp-sdk:bundle" and ":bundle_v2" are listed. If v2 fully supersedes v1, drop the older target to avoid duplicate code/weight.
If both are required, note why (e.g., migration overlap). If not, apply:
deps = [ ":css_files", @@ - "../../third_party/mcp-sdk:bundle", "../../third_party/mcp-sdk:bundle_v2",front_end/panels/ai_chat/ui/ChatView.ts (1)
534-536: Remove unused variable or render the iconicon is computed but unused.
- const icon = ToolDescriptionFormatter.getToolIcon(toolName); const descriptionData = ToolDescriptionFormatter.getToolDescription(toolName, toolArgs);front_end/panels/ai_chat/core/AgentNodes.ts (2)
291-343: Tool call id generation: add fallback for environments without crypto.randomUUIDDevTools likely has crypto, but adding a safe fallback is trivial.
- const toolCallId = crypto.randomUUID(); // Generate unique ID for OpenAI format + const toolCallId = (globalThis.crypto as any)?.randomUUID + ? (globalThis.crypto as any).randomUUID() + : `tc_${Date.now()}_${Math.random().toString(36).slice(2,8)}`;
441-447: Reduce log verbosity for message conversionlogger.info dumps all messages; switch to debug to avoid noisy logs/PII risk.
- logger.info('Converting ChatMessages to LLMMessages. Messages:', messages); + logger.debug('Converting ChatMessages to LLMMessages.', { count: messages.length });front_end/panels/ai_chat/ui/mcp/mcpConnectorsCatalogDialog.css (2)
32-50: Add visible focus styles for keyboard usersButtons lack explicit :focus-visible states; add to match hover for accessibility.
.mcp-catalog-close:hover { color: var(--color-text-primary); background: var(--color-background-elevation-1); } +.mcp-catalog-close:focus-visible { + outline: 2px solid var(--color-primary); + outline-offset: 2px; +} @@ .mcp-close-button:hover { background: var(--color-background-elevation-1); border-color: var(--color-primary-container-border); } +.mcp-manage-button:focus-visible, +.mcp-close-button:focus-visible, +.mcp-error-retry-button:focus-visible, +.mcp-error-clear-button:focus-visible { + outline: 2px solid var(--color-primary); + outline-offset: 2px; +}Also applies to: 384-398
345-350: Respect reduced‑motion preference for loading dotsDisable keyframe animation when prefers-reduced-motion is set.
@keyframes mcp-loading-dots { 0%, 20% { content: ''; } 40% { content: '.'; } 60% { content: '..'; } 80%, 100% { content: '...'; } } +@media (prefers-reduced-motion: reduce) { + .mcp-loading-indicator::after { + animation: none; + content: '...'; + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
config/gni/devtools_grd_files.gni(6 hunks)front_end/panels/ai_chat/BUILD.gn(10 hunks)front_end/panels/ai_chat/agent_framework/AgentRunner.ts(9 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts(1 hunks)front_end/panels/ai_chat/core/AgentNodes.ts(26 hunks)front_end/panels/ai_chat/core/ConfigurableGraph.ts(2 hunks)front_end/panels/ai_chat/core/Logger.ts(1 hunks)front_end/panels/ai_chat/core/StateGraph.ts(3 hunks)front_end/panels/ai_chat/core/ToolNameMap.ts(3 hunks)front_end/panels/ai_chat/core/ToolSurfaceProvider.ts(5 hunks)front_end/panels/ai_chat/core/Types.ts(2 hunks)front_end/panels/ai_chat/mcp/MCPRegistry.ts(2 hunks)front_end/panels/ai_chat/mcp/__tests__/MCPIntegration.test.ts(1 hunks)front_end/panels/ai_chat/tools/Tools.ts(15 hunks)front_end/panels/ai_chat/tracing/LangfuseProvider.ts(12 hunks)front_end/panels/ai_chat/ui/AIChatPanel.ts(9 hunks)front_end/panels/ai_chat/ui/ChatView.ts(8 hunks)front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts(1 hunks)front_end/panels/ai_chat/ui/mcp/mcpConnectorsCatalogDialog.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- front_end/panels/ai_chat/mcp/tests/MCPIntegration.test.ts
- front_end/panels/ai_chat/core/Types.ts
🧰 Additional context used
🧬 Code graph analysis (12)
front_end/panels/ai_chat/core/ToolNameMap.ts (1)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)
front_end/panels/ai_chat/core/ConfigurableGraph.ts (1)
front_end/panels/ai_chat/core/State.ts (1)
AgentState(29-36)
front_end/panels/ai_chat/ui/ChatView.ts (2)
front_end/panels/ai_chat/ui/AIChatPanel.ts (7)
event(1679-1686)event(1691-1695)event(1700-1704)event(1709-1713)event(1718-1722)event(1727-1731)text(1887-1894)front_end/panels/ai_chat/ui/input/ChatInput.ts (1)
Lit(55-68)
front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts (2)
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)
AgentToolConfig(97-184)ConfigurableAgentArgs(241-256)front_end/panels/ai_chat/core/Constants.ts (1)
MODEL_SENTINELS(85-88)
front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts (3)
front_end/panels/ai_chat/ui/AIChatPanel.ts (6)
logger(964-1057)logger(1348-1418)logger(1568-1581)logger(1625-1628)MCPConnectorsCatalogDialog(2100-2112)error(1937-1950)front_end/panels/ai_chat/core/Logger.ts (2)
createLogger(316-318)error(254-303)front_end/panels/ai_chat/mcp/MCPRegistry.ts (1)
MCPRegistry(682-682)
front_end/panels/ai_chat/tools/Tools.ts (1)
front_end/panels/ai_chat/tools/StreamlinedSchemaExtractorTool.ts (1)
sleep(46-61)
front_end/panels/ai_chat/core/AgentNodes.ts (5)
front_end/panels/ai_chat/core/State.ts (1)
AgentState(29-36)front_end/panels/ai_chat/core/ToolSurfaceProvider.ts (1)
ToolSurfaceProvider(211-348)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)
ToolRegistry(189-236)ConfigurableAgentTool(306-544)front_end/panels/ai_chat/agent_framework/AgentSessionTypes.ts (1)
ToolResultMessage(78-86)front_end/panels/ai_chat/tracing/TracingConfig.ts (1)
withTracingContext(313-315)
front_end/panels/ai_chat/core/ToolSurfaceProvider.ts (3)
front_end/panels/ai_chat/tools/Tools.ts (1)
Tool(40-49)front_end/panels/ai_chat/ui/AIChatPanel.ts (1)
AIChatPanel(326-2438)front_end/panels/ai_chat/LLM/LLMClient.ts (1)
LLMClient(49-385)
front_end/panels/ai_chat/tracing/LangfuseProvider.ts (1)
front_end/panels/ai_chat/core/Logger.ts (1)
error(254-303)
front_end/panels/ai_chat/mcp/MCPRegistry.ts (2)
front_end/panels/ai_chat/LLM/LLMTypes.ts (1)
RetryConfig(26-32)front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (1)
ToolRegistry(189-236)
front_end/panels/ai_chat/agent_framework/AgentRunner.ts (2)
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (2)
ConfigurableAgentResult(261-301)createErrorResult(400-419)front_end/panels/ai_chat/agent_framework/AgentSessionTypes.ts (1)
AgentSession(11-41)
front_end/panels/ai_chat/ui/AIChatPanel.ts (2)
front_end/panels/ai_chat/mcp/MCPRegistry.ts (1)
MCPRegistry(682-682)front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts (1)
MCPConnectorsCatalogDialog(164-1024)
🪛 ast-grep (0.39.5)
front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts
[warning] 276-276: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: this.#connectorsContainer.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 376-376: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: errorContainer.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 483-483: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: errorContainer.innerHTML = ''
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (36)
front_end/panels/ai_chat/core/Logger.ts (1)
128-165: Add opt‑in redaction before emitting logs (OAuth tokens, secrets).Given this PR introduces OAuth, add a minimal redaction pass to reduce accidental leakage via
message/data. Keep handlers receiving redacted entries by default; advanced sinks can opt‑out.Apply this diff to run redaction in the core path:
private log(level: LogLevel, message: string, data?: any): void { const effectiveLevel = this.getEffectiveLevel(); if (level < effectiveLevel) { return; } - const entry: LogEntry = { + const baseEntry: LogEntry = { level, module: this.module, message, timestamp: new Date(), data, }; + const entry = Logger.redactEntry(baseEntry); // Add source information if enabled if (Logger.config.includeSource) { const stack = new Error().stack; if (stack) { - const sourceMatch = stack.split('\n')[3]?.match(/at\s+(.+):(\d+):(\d+)/); + const sourceMatch = stack.split('\n')[3]?.match(/at\s+(?:.+\s+\()?(.+):(\d+):(\d+)\)?/); if (sourceMatch) { entry.source = { file: sourceMatch[1], line: parseInt(sourceMatch[2], 10), column: parseInt(sourceMatch[3], 10), }; } } }Add this helper inside the class (e.g., after
logHandlers) to support the change:// Redact common secret shapes while preserving structure private static readonly SECRET_KEYS = ['access_token','refresh_token','authorization','client_secret','password','code_verifier','code_challenge']; private static redactEntry(e: LogEntry): LogEntry { const mask = (v: any): any => { if (v == null) return v; if (typeof v === 'string') { // Hide JWTs and long bearer-like tokens const jwt = /\b[A-Za-z0-9-_]{20,}\.[A-Za-z0-9-_]{10,}\.[A-Za-z0-9-_]{10,}\b/g; return v.replace(jwt, '***'); } if (Array.isArray(v)) return v.map(mask); if (typeof v === 'object') { const out: any = {}; for (const [k, val] of Object.entries(v)) { out[k] = Logger.SECRET_KEYS.includes(k.toLowerCase()) ? '***' : mask(val); } return out; } return v; }; return { ...e, message: mask(e.message), data: mask(e.data) }; }Run this to spot any potential sensitive logging already present:
front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts (2)
107-112: Tools list now matches the system prompt. Good catch.Including extract_data and search_content resolves the earlier gap.
113-115: Resolve MODEL_SENTINELS.USE_MINI: ensure it maps to a concrete model or that miniModel is injected at runtime.Constants.ts defines USE_MINI = 'use-mini'; ConfigurableAgentTool.ts (≈456–458) and AgentRunner.ts (≈303–305) will throw if a miniModel is not provided. I did not find a repo-level mapping that translates 'use-mini' to a provider model—either ensure the orchestrator/caller injects miniModel into the agent context or add a resolver that maps MODEL_SENTINELS.USE_MINI → concrete model name.
See: front_end/panels/ai_chat/core/Constants.ts:86; front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts:456–458; front_end/panels/ai_chat/agent_framework/AgentRunner.ts:303–305; front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts:114–116.
front_end/panels/ai_chat/tracing/LangfuseProvider.ts (3)
357-381: Nice concurrency guard on flushes + atomic buffer swapThe
isFlushInProgress/flushPromisegate andsplice(0)atomic swap prevent interleaving and lost events. Good pattern.Also applies to: 383-414
585-599: Auto-flush lifecycle looks solidSingle-timer guard, error-handled periodic flush, and explicit stop on destroy are correct.
Also applies to: 601-608
114-121: Debug traces around enablement and (create|update) lifecycles are helpfulThe structured details will aid field diagnostics without leaking secrets.
Also applies to: 122-129, 202-207, 215-220, 222-228
front_end/panels/ai_chat/agent_framework/AgentRunner.ts (5)
6-6: Type-only import for AgentDescriptor looks good.
Usingimport typeavoids runtime impact. No action needed.
408-408: DestructuringagentDescriptorfrom config: OK.
No issues.
429-431: Session fieldsiterationCountanddescriptorinitialization: OK.
Good defaults for telemetry/UI.
1013-1014: Good: abort signal forwarded to tool execution.
This enables cooperative cancellation in tools.
42-44: Descriptor not propagated to handoff runs (child sessions will miss metadata).
targetRunnerConfigbuilt inexecuteHandoffdoes not setagentDescriptor, so nested sessions lose descriptor fields and traces won’t include them for child runs.Apply this diff to inject the target agent’s descriptor via the registry:
- import type { AgentDescriptor } from '../core/AgentDescriptorRegistry.js'; + import { AgentDescriptorRegistry, type AgentDescriptor } from '../core/AgentDescriptorRegistry.js'; @@ const targetRunnerConfig: AgentRunnerConfig = { apiKey, modelName: resolvedModelName, systemPrompt: enhancedSystemPrompt, tools: targetConfig.tools .map(toolName => ToolRegistry.getRegisteredTool(toolName)) .filter((tool): tool is Tool<any, any> => tool !== null), maxIterations: targetConfig.maxIterations || defaultMaxIterations, temperature: targetConfig.temperature ?? defaultTemperature, provider: defaultProvider as LLMProvider, getVisionCapability: defaultGetVisionCapability, miniModel, nanoModel, + agentDescriptor: AgentDescriptorRegistry.getDescriptor(targetAgentName), };If the registry API differs, swap in the appropriate getter and confirm
AgentDescriptorRegistryis a runtime export.Also applies to: 317-333
front_end/panels/ai_chat/core/ToolNameMap.ts (2)
5-8: LGTM: scoped logger addedModule-scoped logger initialization is clean and consistent with Logger.ts usage.
20-23: Still exceed 64 chars after suffixing; enforce limit post-suffix as wellWe truncate to 64 before collision handling, but then append "-" and "_" which can push names over 64. Providers will reject those. Trim with room for suffixes.
Apply this diff:
function sanitize(original: string): string { let name = original.replace(/[^a-zA-Z0-9_-]/g, '_'); if (!name) name = 'tool'; - if (name.length > 64) { + const MAX = 64; + if (name.length > MAX) { logger.warn(`Tool name too long (${name.length} chars), truncating to 64: ${original}`); - name = name.slice(0, 64); + name = name.slice(0, MAX); } return name; } export function addMapping(original: string): string { const existing = originalToSanitized.get(original); if (existing) return existing; let candidate = sanitize(original); if (sanitizedToOriginal.has(candidate) && sanitizedToOriginal.get(candidate) !== original) { const suffix = shortHash(original); const base = candidate.replace(/_+$/g, ''); - candidate = base + '-' + suffix; + const MAX = 64; + const suffixStr = '-' + suffix; + const baseRoom = Math.max(1, MAX - suffixStr.length); + candidate = base.slice(0, baseRoom) + suffixStr; } let unique = candidate; let counter = 1; while (sanitizedToOriginal.has(unique) && sanitizedToOriginal.get(unique) !== original) { - const add = `_${counter++}`; - unique = candidate + add; + const add = `_${counter++}`; + const MAX = 64; + const baseRoom = Math.max(1, MAX - add.length); + unique = candidate.slice(0, baseRoom) + add; } originalToSanitized.set(original, unique); sanitizedToOriginal.set(unique, original); return unique; }Also applies to: 50-51, 56-56
front_end/panels/ai_chat/tools/Tools.ts (1)
61-61: LGTM: abortSignal in LLMContextWiring AbortSignal into tool context is a good addition.
front_end/panels/ai_chat/mcp/MCPRegistry.ts (1)
335-342: LGTM: robust connectWithRetry with categorized backoffThe retry categorization and backoff/jitter look solid; returns clear ConnectionResult.
front_end/panels/ai_chat/core/ToolSurfaceProvider.ts (2)
37-38: Good: ensure MCP tools registered before selectionAwaiting ensureToolsRegistered() prevents empty lists on first run.
306-327: Verify persistence of selected MCP tool names across non-user turnsYou reuse (state.context)?.selectedToolNames but this provider doesn’t persist them. Ensure upstream stores the returned selectedNames into state.context for reuse.
Would you like me to wire that in Tool executor? I can propose a small patch in the orchestrator to stash selectedNames after each select() call.
Also applies to: 340-347
front_end/panels/ai_chat/core/StateGraph.ts (1)
53-54: AbortSignal propagation verified — no action requiredTypes.ts declares invoke(input, signal?: AbortSignal); StateGraph forwards the signal and AgentNodes (front_end/panels/ai_chat/core/AgentNodes.ts, createFinalNode) and AgentService accept/pass the optional signal — no runtime fixes needed.
front_end/panels/ai_chat/core/ConfigurableGraph.ts (1)
71-75: Propagate AbortSignal reason when availableThrow
signal.reasonwhen present so upstream sees the original abort cause.
[ suggest_optional_refactor ]- if (signal?.aborted) { - logger.info('ToolExecutorNode dummy implementation aborted'); - throw new DOMException('Tool execution was cancelled', 'AbortError'); - } + if (signal?.aborted) { + logger.info('ToolExecutorNode dummy implementation aborted'); + throw (signal.reason instanceof Error + ? signal.reason + : new DOMException('Tool execution was cancelled', 'AbortError')); + }front_end/panels/ai_chat/ui/AIChatPanel.ts (5)
93-93: Import for MCP connectors catalog looks correctImport path matches added GRD entries.
166-170: New UIStrings entry for MCP ConnectorsGood i18n coverage.
224-225: Toolbar input extended with MCP actionInterface addition is consistent with usage below.
2005-2006: Toolbar wiring passes MCP actionWiring is correct and matches the extended interface.
810-813: Init MCP without forcing OAuth on startup — verifiedMCPRegistry.init is declared as
async init(interactive: boolean = false)in front_end/panels/ai_chat/mcp/MCPRegistry.ts (line 432); callingMCPRegistry.init(false)keeps startup non-interactive and is correct.config/gni/devtools_grd_files.gni (3)
675-675: AgentDescriptorRegistry.js addedConsistent with related core changes.
2275-2294: Unbundled adds ajv bundles and MCP SDK ESM modules — verified
All listed ESM/bundle files are present (verification script returned OK). Approving changes.
915-915: Include mcp-sdk-v2 in bundled list; no legacy mcp-sdk imports found.Imports reference only mcp-sdk-v2 in front_end/panels/ai_chat/mcp/{MCPToolAdapter.ts, MCPRegistry.ts, tests/MCPClientSDK.test.ts}. Keep the mcp-sdk-v2 entry.
front_end/panels/ai_chat/BUILD.gn (1)
11-16: LGTM on wiring CSS module and metadataCSS generation and GRD metadata include the new catalog stylesheet; paths look correct.
Also applies to: 339-345
front_end/panels/ai_chat/ui/ChatView.ts (4)
751-757: Confirm ai-message-list usageYou pass .messages=[] and render items as children. Ensure ai-message-list ignores its .messages prop or expects children; otherwise, pass combinedMessages to .messages.
- <ai-message-list .messages=${[]} .state=${this.#state} .agentViewMode=${this.#agentViewMode}> + <ai-message-list .messages=${combinedMessages} .state=${this.#state} .agentViewMode=${this.#agentViewMode}>
328-376: URL listener lifecycle looks solidListener attaches only in first‑message view and is removed on disconnect/when view changes. Good cleanup.
Also applies to: 141-159
982-989: Disable send while loading is appropriateIncluding LOADING in sendDisabled prevents duplicate sends.
820-836: InputBar API present — no action required.InputBar.ts defines setInputValue(text) and clearInput() and emits 'inputchange'; ChatInput also dispatches 'inputchange'. Keep the existing guarded calls and the ai-chat-input fallback in ChatView as-is.
front_end/panels/ai_chat/core/AgentNodes.ts (2)
648-709: Graceful missing‑tool handling is well‑doneSoft‑fails with a TOOL_RESULT and tracing event instead of crashing the graph.
Also applies to: 954-1001
38-44: Confirmed: graph runner passes AbortSignal into node.invokeStateGraph calls node.invoke(currentState, signal) (StateGraph.ts:108) and AgentService passes this.#abortController.signal into graph.invoke (AgentService.ts:483); node implementations include abort checks.
front_end/panels/ai_chat/ui/mcp/MCPConnectorsCatalogDialog.ts (1)
170-180: Add cancellation (AbortController) for in‑flight connect/disconnect flowsClosing the dialog or switching connectors mid‑flow leaves MCPRegistry.init/refresh running. Add an AbortController, pass its signal into MCPRegistry calls, and treat AbortError as user‑cancelled. Clear the controller on completion/close.
@@ #errorResetTimeouts = new Map<string, number>(); + #abortController: AbortController | null = null; @@ private async connectConnector(connector: MCPConnector, toggle: HTMLButtonElement, item: HTMLElement): Promise<void> { @@ - // Set global connecting state + // Cancel any prior attempt and set global connecting state + if (this.#abortController) { + this.#abortController.abort(); + } + this.#abortController = new AbortController(); this.setConnectingState(connector.id); @@ - // Test the connection - const connectionResults = await MCPRegistry.init(true); - await MCPRegistry.refresh(); + // Test the connection + const connectionResults = await MCPRegistry.init(true, { signal: this.#abortController.signal }); + if (typeof (MCPRegistry as any).refresh === 'function') { + // Optional: refresh supports signal if implemented + await (MCPRegistry as any).refresh({ signal: this.#abortController.signal }); + } else { + await MCPRegistry.refresh(); + } @@ - } catch (error) { + } catch (error) { + if ((error as any)?.name === 'AbortError') { + logger.info('Connection attempt cancelled'); + this.clearConnectingState(); + item.removeAttribute('aria-busy'); + this.#abortController = null; + return; + } logger.error('Failed to connect MCP connector', error); @@ - } + } finally { + this.#abortController = null; + } } @@ private close(): void { + // Cancel any pending network work + if (this.#abortController) { + try { this.#abortController.abort(); } catch {} + this.#abortController = null; + } // Clean up any pending timeout callbacks this.#errorResetTimeouts.forEach((timeoutId) => { window.clearTimeout(timeoutId); });Also applies to: 847-904, 959-1003, 1012-1023
front_end/panels/ai_chat/ui/mcp/mcpConnectorsCatalogDialog.css (1)
141-152: Contrast: primary OK; confirm & fix error-state contrast
- Primary: var(--color-primary) on var(--color-background-elevation-1) ≈ 5.7:1 — acceptable.
- Error: var(--color-error) is used for small-button backgrounds/text (e.g. .mcp-error-retry-button (11px), .mcp-toggle-switch.error, .mcp-connector-item.error). I could not find a definitive definition for --color-error in tokens; if it resolves to rgb(238,68,47) or #ff0000, white text contrast ≈ 3.8:1 and fails WCAG 4.5:1 for small text. Confirm the resolved token and either use a darker error variant or darker button text (or increase text size/weight) so error-state buttons meet 4.5:1.
Location: front_end/panels/ai_chat/ui/mcp/mcpConnectorsCatalogDialog.css — affected selectors: .mcp-connector-item.error, .mcp-toggle-switch.error, .mcp-error-retry-button / .mcp-error-clear-button.
| overrides?: { sessionId?: string; parentSessionId?: string; traceId?: string }, | ||
| abortSignal?: AbortSignal | ||
| ): Promise<ConfigurableAgentResult & { agentSession: AgentSession }> { |
There was a problem hiding this comment.
🧩 Analysis chain
Abort signal isn’t propagated to handoffs or the LLM call.
Runs handed off to other agents won’t observe cancellations, and the main LLM request can’t be cancelled.
Apply this diff to thread abortSignal end‑to‑end:
@@
- private static async executeHandoff(
+ private static async executeHandoff(
currentMessages: ChatMessage[],
originalArgs: ConfigurableAgentArgs,
handoffConfig: HandoffConfig,
executingAgent: ConfigurableAgentTool,
apiKey: string,
defaultModelName: string,
defaultMaxIterations: number,
defaultTemperature: number,
defaultCreateSuccessResult: AgentRunnerHooks['createSuccessResult'],
defaultCreateErrorResult: AgentRunnerHooks['createErrorResult'],
llmToolArgs?: ConfigurableAgentArgs, // Specific args if triggered by LLM tool call
parentSession?: AgentSession, // For natural nesting
defaultProvider?: LLMProvider,
defaultGetVisionCapability?: (modelName: string) => Promise<boolean> | boolean,
miniModel?: string, // Mini model for smaller/faster operations
nanoModel?: string, // Nano model for smallest/fastest operations
- overrides?: { sessionId?: string; parentSessionId?: string; traceId?: string }
+ overrides?: { sessionId?: string; parentSessionId?: string; traceId?: string },
+ abortSignal?: AbortSignal
): Promise<ConfigurableAgentResult & { agentSession: AgentSession }> {
@@
- const handoffResult = await AgentRunner.run(
+ const handoffResult = await AgentRunner.run(
handoffMessages,
targetAgentArgs, // Use determined args
targetRunnerConfig, // Pass the constructed config
targetRunnerHooks, // Pass the constructed hooks
targetAgentTool, // Target agent is now the executing agent
parentSession, // Pass parent session for natural nesting
- overrides
+ overrides,
+ abortSignal
);
@@
- const handoffResult = await AgentRunner.executeHandoff(
+ const handoffResult = await AgentRunner.executeHandoff(
messages, // Pass current message history
toolArgs as ConfigurableAgentArgs, // <= LLM's toolArgs are the 'originalArgs' for this handoff context
handoffConfig,
executingAgent!, // executingAgent must exist if handoff config was found
apiKey, modelName, maxIterations, temperature ?? 0,
createSuccessResult, createErrorResult,
toolArgs as ConfigurableAgentArgs, // <= Pass LLM's toolArgs explicitly as llmToolArgs
currentSession, // Pass current session for natural nesting
config.provider,
config.getVisionCapability,
config.miniModel,
config.nanoModel,
- { sessionId: nestedSessionId, parentSessionId: currentSession.sessionId, traceId: getCurrentTracingContext()?.traceId }
+ { sessionId: nestedSessionId, parentSessionId: currentSession.sessionId, traceId: getCurrentTracingContext()?.traceId },
+ abortSignal
);
@@
- const handoffResult = await AgentRunner.executeHandoff(
+ const handoffResult = await AgentRunner.executeHandoff(
messages, // Pass the final message history from the loop
args, // Pass the original args of the *current* agent as 'originalArgs'
maxIterHandoffConfig,
executingAgent,
apiKey, modelName, maxIterations, temperature ?? 0,
createSuccessResult, createErrorResult,
undefined, // No llmToolArgs for max iterations handoff
currentSession, // Pass current session for natural nesting
config.provider,
config.getVisionCapability,
config.miniModel,
- config.nanoModel
+ config.nanoModel,
+ undefined,
+ abortSignal
);
@@
- llmResponse = await llm.call({
+ llmResponse = await llm.call({
provider,
model: modelName,
messages: sanitizedForMainCall,
systemPrompt: currentSystemPrompt,
tools: toolSchemas,
temperature: temperature ?? 0,
+ // If supported by LLMClient, pass through for cooperative cancellation
+ abortSignal,
});Run the checks below to confirm LLMClient.call supports an abort signal and to find any other internal call sites needing propagation:
Also applies to: 214-232, 346-354, 889-903, 1261-1274, 662-669
🏁 Script executed:
#!/bin/bash
# Inspect LLMClient.call options and abort support
fd -t f -a 'LLMClient.*\.(ts|tsx|js)' | xargs -I{} rg -n --hidden -C2 'class LLMClient|call\s*\(' {}
# Search for abortSignal or signal usage across LLM layer
rg -n --hidden -C2 -g 'front_end/**' -e '\babortSignal\b' -e '\bsignal\b'Length of output: 263412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==== locate LLMClient files ===="
fd -t f -a 'LLMClient.*\.(ts|tsx|js)' front_end || true
echo
echo "==== show LLMClient files & call method ===="
for f in $(fd -t f -a 'LLMClient.*\.(ts|tsx|js)' front_end); do
echo "----- $f -----"
rg -n -C3 'class LLMClient|async call\(' "$f" || true
sed -n '1,260p' "$f" || true
done
echo
echo "==== find LLMCallRequest/type occurrences ===="
rg -n --hidden -S --line-number 'LLMCallRequest' front_end || true
rg -n --hidden -S --line-number 'export (type|interface) .*LLM' front_end || true
echo
echo "==== show LLMTypes.ts ===="
sed -n '1,240p' front_end/panels/ai_chat/LLM/LLMTypes.ts || true
echo
echo "==== find llm.call(...) usages ===="
rg -n --hidden -S --line-number '\bllm\.call\s*\(' front_end || true
echo
echo "==== find LLMClient instantiation/getInstance usages ===="
rg -n --hidden -S --line-number 'new LLMClient|LLMClient.getInstance|LLMClient\.instance|LLMClient\.get' front_end || trueLength of output: 18619
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "==== LLMProvider interface ===="
sed -n '1,240p' front_end/panels/ai_chat/LLM/LLMProvider.ts || true
echo
echo "==== OpenAIProvider.callWithMessages signature ===="
sed -n '1,260p' front_end/panels/ai_chat/LLM/OpenAIProvider.ts || true
echo
echo "==== LiteLLMProvider ===="
sed -n '1,260p' front_end/panels/ai_chat/LLM/LiteLLMProvider.ts || true
echo
echo "==== GroqProvider ===="
sed -n '1,260p' front_end/panels/ai_chat/LLM/GroqProvider.ts || true
echo
echo "==== OpenRouterProvider ===="
sed -n '1,260p' front_end/panels/ai_chat/LLM/OpenRouterProvider.ts || true
echo
echo "==== search for callWithMessages definitions/usages ===="
rg -n --hidden --line-number 'callWithMessages' front_end || true
echo
echo "==== show AgentRunner llm.call call site context ===="
sed -n '600,740p' front_end/panels/ai_chat/agent_framework/AgentRunner.ts || true
echo
echo "==== show AgentNodes llm.call usage context ===="
sed -n '1,320p' front_end/panels/ai_chat/core/AgentNodes.ts || trueLength of output: 56648
Propagate AbortSignal end‑to‑end: handoffs → LLM call
LLM cancellations are currently ignored — AgentRunner handoffs and the main LLM request do not receive an AbortSignal and therefore cannot be cancelled. Make these concrete changes:
- Add a signal?: AbortSignal to LLMCallOptions in front_end/panels/ai_chat/LLM/LLMTypes.ts and an abortSignal?: AbortSignal (or signal) to LLMCallRequest in front_end/panels/ai_chat/LLM/LLMClient.ts.
- Update LLMClient.call to copy the request abortSignal into the call options passed to provider.callWithMessages.
- Update provider implementations to honor the signal: OpenAIProvider.ts, LiteLLMProvider.ts, GroqProvider.ts, OpenRouterProvider.ts — pass options?.signal into fetch() (or other async I/O) and ensure retry logic respects cancellation.
- Propagate abortSignal through the agent framework: add abortSignal parameter to AgentRunner.executeHandoff (and AgentRunner.run handoff call sites) and pass it to nested AgentRunner.run / executeHandoff calls so handed‑off agents observe cancellations.
- Update all direct llm.call call sites to pass the local signal/context (examples: front_end/panels/ai_chat/core/AgentNodes.ts, agent_framework/AgentRunner.ts, tools/Tools.ts, tools/SequentialThinkingTool.ts, tools/LLMTracingWrapper.ts) so calls are abortable.
Fixes must include type changes (LLMTypes), LLMClient plumbing, provider fetch calls, and all caller sites that start or forward agent work. This is a critical bug (in-flight LLM/tool work continues after cancellation).
| // Check if execution has been aborted | ||
| if (abortSignal?.aborted) { | ||
| logger.info(`${agentName} execution aborted at iteration ${iteration + 1}/${maxIterations}`); | ||
| const abortResult = createErrorResult('Execution was cancelled', messages, 'error'); | ||
| return { ...abortResult, agentSession: currentSession }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Abort returns without closing the session (status remains “running”).
This leaves sessions in an inconsistent state and can confuse UI/analytics.
Apply this diff to finalize the session before returning:
if (abortSignal?.aborted) {
logger.info(`${agentName} execution aborted at iteration ${iteration + 1}/${maxIterations}`);
+ // Finalize session
+ currentSession.status = 'error';
+ currentSession.endTime = new Date();
+ currentSession.terminationReason = 'cancelled';
const abortResult = createErrorResult('Execution was cancelled', messages, 'error');
return { ...abortResult, agentSession: currentSession };
}📝 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.
| // Check if execution has been aborted | |
| if (abortSignal?.aborted) { | |
| logger.info(`${agentName} execution aborted at iteration ${iteration + 1}/${maxIterations}`); | |
| const abortResult = createErrorResult('Execution was cancelled', messages, 'error'); | |
| return { ...abortResult, agentSession: currentSession }; | |
| } | |
| // Check if execution has been aborted | |
| if (abortSignal?.aborted) { | |
| logger.info(`${agentName} execution aborted at iteration ${iteration + 1}/${maxIterations}`); | |
| // Finalize session | |
| currentSession.status = 'error'; | |
| currentSession.endTime = new Date(); | |
| currentSession.terminationReason = 'cancelled'; | |
| const abortResult = createErrorResult('Execution was cancelled', messages, 'error'); | |
| return { ...abortResult, agentSession: currentSession }; | |
| } |
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/agent_framework/AgentRunner.ts around lines 555 to
561, the abort branch returns early without finalizing the agent session leaving
its status as "running"; before returning, update/finalize the currentSession
(e.g., set status to cancelled/finished, record end time and any final metadata)
and persist that change via the existing session update/finalize function so the
session state is consistent, then include the updated agentSession in the
returned object along with the abort result.
front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts
Show resolved
Hide resolved
front_end/panels/ai_chat/agent_framework/implementation/agents/EcommerceProductInfoAgent.ts
Show resolved
Hide resolved
|
Claude finished @tysonthomas9's task —— View job Code Review: MCP OAuth Support and Agent Framework UpdatesReview Progress
Overall AssessmentThis is a substantial and well-architected addition that introduces OAuth support for MCP connections and a comprehensive agent framework. The code demonstrates good security practices, comprehensive error handling, and thoughtful architecture. However, there are several areas that need attention before production deployment. Critical Security IssuesOAuth Implementation Security✅ Strengths:
Template Injection Vulnerabilities🚨 Critical Issues:
These should use Agent Framework Architecture✅ Well-designed:
Performance Analysis
✅ Performance Positives:
Error Handling✅ Good error handling:
Test Coverage✅ Good coverage:
Code Organization Issues
Specific RecommendationsHigh Priority Fixes
Security Improvements
Performance Optimizations
Code Quality
ConclusionThis PR introduces valuable functionality with generally solid architecture and security practices. The main concerns are around production readiness (debugging code, performance optimization) and a few critical security fixes for template injection. The OAuth implementation is well-designed but needs some hardening. The agent framework is comprehensive and well-structured. Recommendation: Address the critical template injection issues and remove debugging code before merging. The other improvements can be tackled in follow-up PRs. |
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - New Features - MCP Connectors catalog and connections dialog to browse, connect, and manage integrations (with new connector icons). - Rich example suggestions in the empty chat view, tailored to the current page. - New specialized web agents for search, research, content writing, navigation, and UI actions. - Cancel in‑flight runs without clearing conversation history. - Improvements - Smarter MCP tool selection using AI; expanded MCP connection status, retries, and event history. - Consistent “extract_data” tool naming across app and evaluations. - Better observability with agent/version metadata in traces. - Enhanced reliability with widespread abort/cancel support. - Version - App version updated to 0.3.4. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit