[MI][AI][Copilot] Add agent mode to MI Copilot#1452
[MI][AI][Copilot] Add agent mode to MI Copilot#1452ChinthakaJ98 merged 109 commits intowso2:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MI Copilot Agent mode and migrates ai-panel into ai-features: new agent-mode RPC/types/handlers, full agent execution pipeline with many agents and tools (file, bash, web, connectors, data-mapper, subagents, plan-mode), session/history/undo management, unified Copilot auth and Anthropic provider utilities, and large import/entry-point reorganization. Changes
Sequence Diagram(s)sequenceDiagram
participant Webview as Client/Webview
participant RPC as MIAgentPanelRpcManager
participant History as ChatHistoryManager
participant Agent as AgentExecutor
participant Tools as Tool Executors
participant LLM as Anthropic
participant FS as File System
rect rgba(100,150,240,0.5)
Webview->>RPC: sendAgentMessage(request)
RPC->>History: saveMessage(user)
RPC->>Agent: executeAgent(request)
end
rect rgba(120,200,150,0.5)
Agent->>LLM: stream/generate(messages + system)
LLM-->>Agent: streaming assistant/tool steps
Agent->>Tools: invoke tool (e.g., file_read / bash / web_fetch)
Tools->>FS: read/write files
Tools-->>Agent: tool result
Agent->>History: saveMessage(assistant/tool_result)
Agent->>RPC: emit agentEvent
RPC->>Webview: notify agentEvent
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Adds “MI agent mode” alongside the existing AI features, addressing linked issues by expanding RPC/webview support, improving auth integration via platform extension, and enriching agent tooling (sessions, undo checkpoints, web tools, LSP validation, etc.).
Changes:
- Introduces agent-mode RPCs, event streaming, session management, undo checkpointing, and a large set of agent tools.
- Refactors “ai-panel” paths to “ai-features” and unifies auth/token/usage handling.
- Adds LSP code-action support and extends diagram “open” to support line navigation.
Reviewed changes
Copilot reviewed 87 out of 149 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/mi/mi-rpc-client/src/RpcClient.ts | Adds agent panel RPC client + agent event subscription support. |
| workspaces/mi/mi-extension/src/visualizer/activate.ts | Updates AI panel import path to ai-features. |
| workspaces/mi/mi-extension/src/uri-handler.ts | Removes legacy /signin OAuth exchange route behavior (now platform-based). |
| workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts | Refactors AI/auth usage, improves open-file navigation (line support), adjusts formatting/edit flow. |
| workspaces/mi/mi-extension/src/rpc-managers/mi-data-mapper/rpc-manager.ts | Updates mapper import path to ai-features. |
| workspaces/mi/mi-extension/src/rpc-managers/ai-features/utils.ts | Switches to unified auth storage APIs and login-method-aware token logic. |
| workspaces/mi/mi-extension/src/rpc-managers/ai-features/rpc-manager.ts | Updates imports, usage fetch parsing, and auth base URL usage. |
| workspaces/mi/mi-extension/src/rpc-managers/ai-features/event-handler.ts | Updates imports to ai-features paths. |
| workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-handler.ts | Adds RPC handler registration for agent mode requests (incl. sessions/compact/search). |
| workspaces/mi/mi-extension/src/rpc-managers/agent-mode/event-handler.ts | Adds agent event forwarding from extension to the webview. |
| workspaces/mi/mi-extension/src/lang-client/ExtendedLanguageClient.ts | Adds textDocument/codeAction request helper. |
| workspaces/mi/mi-extension/src/extension.ts | Activates AI panel via ai-features and updates state machine import path. |
| workspaces/mi/mi-extension/src/constants/index.ts | Adds OPEN_AGENT_PANEL command constant. |
| workspaces/mi/mi-extension/src/ai-panel/auth.ts | Deletes legacy AI-panel auth implementation (moved/unified). |
| workspaces/mi/mi-extension/src/ai-features/utils/file-utils.ts | Adds file tree scanning/formatting utilities for prompts. |
| workspaces/mi/mi-extension/src/ai-features/copilot/unit-tests/unit_test_generate.ts | Fixes connection import path under ai-features. |
| workspaces/mi/mi-extension/src/ai-features/copilot/unit-tests/unit_test_case_generate.ts | Fixes connection import path under ai-features. |
| workspaces/mi/mi-extension/src/ai-features/copilot/suggestions/suggestions.ts | Fixes connection import path under ai-features. |
| workspaces/mi/mi-extension/src/ai-features/copilot/message-utils.ts | Tightens image attachment validation and improves warning messaging. |
| workspaces/mi/mi-extension/src/ai-features/copilot/idp/idp.ts | Fixes connection import path under ai-features. |
| workspaces/mi/mi-extension/src/ai-features/copilot/idp/fill_schema.ts | Updates AI SDK option API (maxOutputTokens) and connection import path. |
| workspaces/mi/mi-extension/src/ai-features/copilot/generation/generations.ts | Fixes connection import path under ai-features. |
| workspaces/mi/mi-extension/src/ai-features/copilot/dmc_to_ts/dmc_to_ts.ts | Fixes connection import path under ai-features. |
| workspaces/mi/mi-extension/src/ai-features/copilot/diagnostics/diagnostics.ts | Updates AI SDK option API (maxOutputTokens) and connection import path. |
| workspaces/mi/mi-extension/src/ai-features/copilot/data-mapper/mapper.ts | Fixes connection import path under ai-features. |
| workspaces/mi/mi-extension/src/ai-features/copilot/connectors/connectors.ts | Fixes connection import path under ai-features. |
| workspaces/mi/mi-extension/src/ai-features/copilot/auto-fill/fill.ts | Updates AI SDK option API (maxOutputTokens) and connection import path. |
| workspaces/mi/mi-extension/src/ai-features/connection.ts | Updates auth flow, headers, provider accessors, and token refresh behavior. |
| workspaces/mi/mi-extension/src/ai-features/cache-utils.ts | Adds prompt-caching helper for Anthropic models. |
| workspaces/mi/mi-extension/src/ai-features/aiMachine.ts | Integrates Devant/platform extension auth and adds login listener setup. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/undo/checkpoint-manager.ts | Adds undo-checkpoint stack storage and conflict detection. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.ts | Adds web_search/web_fetch tools with approval flow. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/validation-utils.ts | Adds shared XML validation utilities + optional code actions. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/types.ts | Adds common tool types/constants/errors for agent toolset. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/tool-result-persistence.ts | Adds persistence for oversized tool outputs with cleanup policies. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/skill_tools.ts | Adds skill context loader tool for specialized guidance. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/lsp_tools.ts | Adds validate_code tool built atop shared LSP validation utils. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/index.ts | Exports tools and provides createFileTools factory. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_tools.ts | Adds connector definition tool integrating store cache + docs. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_store_cache.ts | Adds connector store caching keyed by runtime version + TTL. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tool-action-mapper.ts | Adds UI-friendly action text mapping per tool name. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/storage-paths.ts | Adds stable on-disk storage keying for projects/sessions. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/langfuse-setup.ts | Adds Langfuse OpenTelemetry initialization/shutdown helpers. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/index.ts | Exports agent mode core, prompts, and tools. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_expression_guide.ts | Adds Synapse expression guidance content for prompts. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_expression_examples.ts | Adds Synapse expression examples for prompts. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/context/connectors_guide.ts | Adds connector usage guidance + AI connector documentation content. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/attachment-utils.ts | Adds attachment validation + AI SDK message-content builder for agent mode. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/subagents/index.ts | Adds subagent exports (Explore subagent). |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/subagents/explore/system.ts | Adds Explore subagent system prompt. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/subagents/explore/agent.ts | Adds Explore subagent implementation using read-only tools. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts | Adds main agent user-prompt builder including project structure + env. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/mode.ts | Adds ask/edit/plan mode policy text and plan workflow guidance. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/system.ts | Adds data-mapper agent system prompt emphasizing dmUtils usage. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/prompt.ts | Adds data-mapper agent user prompt template. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/agent.ts | Adds data-mapper sub-agent that writes mapping code into TS file. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/compact/tools.ts | Adds compact agent tools scaffold that blocks execution. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/compact/prompt.ts | Adds compact summarization system-reminder prompt. |
| workspaces/mi/mi-extension/src/ai-features/activate.ts | Adds Langfuse setup + adjusts AI panel command registration. |
| workspaces/mi/mi-extension/src/RPCLayer.ts | Registers agent RPC handlers and exposes env config to webviews. |
| workspaces/mi/mi-extension/package.json | Upgrades AI SDK deps and adds agent-mode related libraries. |
| workspaces/mi/mi-core/src/state-machine-types.ts | Adjusts token/usage types for unified auth + usage percentage. |
| workspaces/mi/mi-core/src/rpc-types/mi-diagram/types.ts | Adds optional line to open diagram request. |
| workspaces/mi/mi-core/src/rpc-types/ai-features/rpc-type.ts | Updates fetchUsage response typing for new usage shape. |
| workspaces/mi/mi-core/src/rpc-types/ai-features/index.ts | Updates AI panel API typing for new usage shape. |
| workspaces/mi/mi-core/src/rpc-types/agent-mode/rpc-type.ts | Adds agent-mode RPC types and agent event notification type. |
| workspaces/mi/mi-core/src/rpc-types/agent-mode/index.ts | Exports agent-mode RPC types and request/response types. |
| workspaces/mi/mi-core/src/interfaces/mi-copilot.ts | Extends chat entries to optionally include model/tool messages. |
| workspaces/mi/mi-core/src/index.ts | Re-exports ai-features and agent-mode RPC types from mi-core. |
| workspaces/ballerina/overview-view/src/Overview.tsx | Reorders interfaces (non-functional change) to satisfy TS usage/order. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
workspaces/mi/mi-extension/src/ai-features/copilot/message-utils.ts
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/attachment-utils.ts
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/rpc-managers/agent-mode/event-handler.ts
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/mode.ts
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/undo/checkpoint-manager.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
workspaces/mi/mi-extension/package.json (1)
1060-1060:⚠️ Potential issue | 🟡 Minor
globis listed in bothdevDependencies(v11.0.2) anddependencies(v11.1.0) with mismatched versions.This can lead to unpredictable resolution behavior depending on the package manager. If
globis needed at runtime, keep it only independenciesand remove thedevDependenciesentry (or vice versa). Also align on a single version.Proposed fix
Remove
globfromdevDependencies(line 1060) if it's needed at runtime, or fromdependencies(line 1104) if it's only needed at build time. Pick one version.Also applies to: 1104-1104
workspaces/mi/mi-extension/src/ai-features/connection.ts (1)
180-211:⚠️ Potential issue | 🟠 MajorStale provider cache when API key changes under the same login method.
cachedAnthropicis invalidated only whenloginMethodchanges (line 184). ForANTHROPIC_KEYusers, the API key is embedded in the provider at creation time (line 199). If the user updates their API key without changing login methods, the cached provider retains the old key, causing persistent authentication failures until the extension is reloaded.Consider additionally comparing the current access token (or a hash of it) to detect key changes:
Proposed fix sketch
let cachedAnthropic: ReturnType<typeof createAnthropic> | null = null; let cachedAuthMethod: LoginMethod | null = null; +let cachedApiKey: string | null = null; export const getAnthropicProvider = async (): Promise<ReturnType<typeof createAnthropic>> => { const loginMethod = await getLoginMethod(); + const currentKey = loginMethod === LoginMethod.ANTHROPIC_KEY ? await getAccessToken() : null; // Recreate client if login method has changed or no cached instance - if (!cachedAnthropic || cachedAuthMethod !== loginMethod) { + if (!cachedAnthropic || cachedAuthMethod !== loginMethod || (loginMethod === LoginMethod.ANTHROPIC_KEY && currentKey !== cachedApiKey)) { if (loginMethod === LoginMethod.MI_INTEL) { // ... existing MI_INTEL setup ... } else if (loginMethod === LoginMethod.ANTHROPIC_KEY) { - const apiKey = await getAccessToken(); + const apiKey = currentKey; // ... existing ANTHROPIC_KEY setup ... } cachedAuthMethod = loginMethod; + cachedApiKey = currentKey; }workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts (1)
2661-2680:⚠️ Potential issue | 🟠 MajorAddress incomplete multi-file saves in ApplyEditsRequest handler.
When
params.editscontains entries with differentdocumentUrivalues (line 2650 showseditRequest.documentUri ?? params.documentUri), onlyparams.documentUriis saved (lines 2662–2665). Other edited files remain dirty. Additionally, the non-null assertion oneditRequest.documentUriat line 2671 assumes the parameter is always provided, butExtendedTextEdit.documentUriis optional.Collect all edited URIs upfront and save all modified documents. Guard against missing URIs in formatEdits:
Suggested fix (multi-file save and URI guard)
await workspace.applyEdit(edit); - const file = Uri.file(params.documentUri); - let document = workspace.textDocuments.find(doc => doc.uri.fsPath === params.documentUri) - || await workspace.openTextDocument(file); - await document.save(); + const editedUris = new Set<string>(); + if ('text' in params) { + editedUris.add(params.documentUri); + } else if ('edits' in params) { + params.edits.forEach(editRequest => { + const targetUri = editRequest.documentUri ?? params.documentUri; + if (targetUri) { + editedUris.add(targetUri); + } + }); + } + await Promise.all([...editedUris].map(async (uri) => { + const file = Uri.file(uri); + const document = workspace.textDocuments.find(doc => doc.uri.fsPath === uri) + || await workspace.openTextDocument(file); + await document.save(); + }));- return this.rangeFormat({ uri: editRequest.documentUri!, range: formatRange }); + const targetUri = editRequest.documentUri ?? params.documentUri; + if (!targetUri) { + return Promise.resolve({ status: true }); + } + return this.rangeFormat({ uri: targetUri, range: formatRange });
🤖 Fix all issues with AI agents
In `@workspaces/mi/mi-extension/src/ai-features/activate.ts`:
- Around line 44-47: The registered command sets extension.initialPrompt but
immediately calls openAIWebview() which accepts an optional initialPrompt and
reassigns extension.initialPrompt, overwriting the value; fix by passing the
command parameter through to openAIWebview so the value is preserved—update the
COMMANDS.OPEN_AI_PANEL handler to call openAIWebview(initialPrompt) (referencing
extension.initialPrompt, openAIWebview, and the PromptObject parameter) so the
initial prompt is not lost.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts`:
- Around line 54-59: The template references {{payloads}} but the template
context only sets userPreconfigured: params.payloads, so payloads is never
present and renders empty; update the context-building code (where the template
context object is constructed) to include payloads: params.payloads (or
otherwise assign payloads = params.payloads) so both userPreconfigured and
payloads are available to the template rendering; ensure the context change is
made in the same function that currently sets userPreconfigured (the template
context creation code).
- Around line 176-192: getCurrentlyOpenedFile currently returns a full sentence
which duplicates text in the template; change getCurrentlyOpenedFile to return
only the relative file path string (or null) instead of a complete sentence
(keep the existing fs.existsSync check and path.relative logic), and ensure the
template that injects {{currentlyOpenedFile}} continues to add the surrounding
sentence so consumers use the value as a bare path; update any code that
depended on the helper producing a full sentence to handle a plain path or null
accordingly.
In `@workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts`:
- Around line 4193-4195: The code assigns llmBaseUrl to both openaiUrl and
anthropicUrl causing OpenAI calls to be routed to the Claude endpoint; change
the assignments so openaiUrl is sourced from MI_COPILOT_OPENAI_PROXY_URL only
(e.g., openaiUrl = process.env.MI_COPILOT_OPENAI_PROXY_URL as string) and
anthropicUrl uses llmBaseUrl as its primary fallback (e.g., anthropicUrl =
llmBaseUrl || process.env.MI_COPILOT_ANTHROPIC_PROXY_URL as string); also remove
the unused anthropicUrl variable if it's truly never referenced (or
document/export it if intended for future use). Ensure you update usages
referenced in IdpConnectorSchemaGenerateForm.tsx to rely on the corrected
openaiUrl.
🟠 Major comments (21)
workspaces/mi/mi-extension/package.json-1072-1072 (1)
1072-1072: 🛠️ Refactor suggestion | 🟠 MajorMigrate
generateObjectcalls to v6 pattern usinggenerateTextwithoutput: Output.object(...).The core v5→v6 migration is largely complete (no v5 patterns like
CoreMessageorconvertToCoreMessagesremain), butgenerateObjectis deprecated in AI SDK v6 and still used across multiple files:
diagnostics.ts:149fill.ts:215connectors.ts:177fill_schema.ts:143All of these should be migrated to use
generateTextwithoutput: Output.object(...)to align with v6's recommended API.workspaces/mi/mi-extension/src/ai-features/activate.ts-27-27 (1)
27-27:⚠️ Potential issue | 🟠 Major
ENABLE_LANGFUSEis hardcoded totrue— this will activate Langfuse tracing for all users.This flag is described as a "dev flag" but is unconditionally enabled. If the
LANGFUSE_PUBLIC_KEY/LANGFUSE_SECRET_KEYenv vars happen to be set (e.g., in a CI or dev environment), traces will be sent tohttps://cloud.langfuse.com. Consider defaulting tofalse, or gating onprocess.env.ENABLE_LANGFUSE === 'true'so it doesn't accidentally activate in production builds.Proposed fix
-// Dev flag - set to true to enable Langfuse observability -const ENABLE_LANGFUSE = true; +// Dev flag - enable Langfuse observability via environment variable +const ENABLE_LANGFUSE = process.env.ENABLE_LANGFUSE === 'true';workspaces/mi/mi-extension/src/ai-features/agent-mode/attachment-utils.ts-19-176 (1)
19-176: 🛠️ Refactor suggestion | 🟠 MajorExtract common attachment validation and filtering logic into a shared utility module.
This file duplicates nearly all of its code from
workspaces/mi/mi-extension/src/ai-features/copilot/message-utils.ts: the constantsTEXT_MIMETYPESandIMAGE_MIMETYPES, the validation functionsisValidBase64andisValidImageDataUri, and the core exportsfilterFiles,buildMessageContent, andvalidateAttachmentsare identical between both files.The only functional difference is in text file formatting:
attachment-utils.tsuses a plain string template whilemessage-utils.tsuses a Handlebars template. This duplication should be consolidated by extracting shared validation and filtering logic into a common utility module, with only the formatting differences remaining module-specific.workspaces/mi/mi-extension/src/ai-features/agent-mode/storage-paths.ts-59-61 (1)
59-61:⚠️ Potential issue | 🟠 Major
sessionIdparameter from user input is used directly in path construction without validation — potential directory traversal.The
sessionIdparameter inSwitchSessionRequestflows directly from user RPC calls throughswitchSession()→ChatHistoryManager→getCopilotSessionDir()without sanitization. A caller could provide a value like../../etc/passwdto construct paths outside the intended storage directory. Validate thatsessionIdcontains only safe characters (alphanumeric, hyphens, underscores) or usepath.basename()to strip directory components before using it in path operations.workspaces/mi/mi-extension/src/ai-features/agent-mode/chat-history-manager.ts-231-246 (1)
231-246:⚠️ Potential issue | 🟠 Major
writeStreamis not nullified afterclose(), allowing stale writes.After
close()resolves,this.writeStreamstill holds the reference to the ended stream. A subsequent call tosaveMessage()would attemptwriteStream.write()on a closed stream (instead of hitting the "Write stream not initialized" guard at line 254), leading to an error or silent data loss.Proposed fix
async close(): Promise<void> { if (this.writeStream) { await this.writeSessionEnd(); + const stream = this.writeStream; + this.writeStream = null; return new Promise((resolve, reject) => { - this.writeStream!.end((err: Error | null | undefined) => { + stream.end((err: Error | null | undefined) => { if (err) { logError('[ChatHistory] Failed to close stream', err); reject(err); } else { logInfo('[ChatHistory] Session closed'); resolve(); } }); }); } }workspaces/mi/mi-extension/src/ai-features/utils/file-utils.ts-226-244 (1)
226-244:⚠️ Potential issue | 🟠 MajorCross-platform bug:
path.sepsplit after forward-slash normalization.
shouldIgnoreFile(Line 132) normalizes paths to/, butformatFileTreesplits onpath.sep(Line 230). On Windows,path.sepis\, so the split produces a single-element array for/-separated paths, breaking the tree structure entirely.Use a consistent separator for splitting:
Proposed fix
for (const file of filteredFiles) { - const parts = file.split(path.sep); + const parts = file.replace(/\\/g, '/').split('/'); let current = tree;workspaces/mi/mi-extension/src/ai-features/agent-mode/undo/checkpoint-manager.ts-97-120 (1)
97-120:⚠️ Potential issue | 🟠 MajorLCS diff has O(n×m) memory — risky for large files.
calculateLineChangesallocates a full(rows+1) × (cols+1)2D array. For a file with 10,000 lines, this is ~100 million entries (~800 MB for 64-bit numbers). Even moderate files (2,000 lines) would allocate ~16 million entries.Consider using a space-optimized LCS (only two rows needed) or switching to a well-tested diff library (e.g.,
diffnpm package) that handles large inputs efficiently.Space-optimized alternative (O(min(n,m)) memory)
function calculateLineChanges(beforeContent: string, afterContent: string): { addedLines: number; deletedLines: number } { const beforeLines = beforeContent.split('\n'); const afterLines = afterContent.split('\n'); const rows = beforeLines.length; const cols = afterLines.length; - const dp: number[][] = Array.from({ length: rows + 1 }, () => Array(cols + 1).fill(0)); + // Space-optimized: only keep two rows + let prev = new Array(cols + 1).fill(0); + let curr = new Array(cols + 1).fill(0); for (let i = 1; i <= rows; i++) { + curr[0] = 0; for (let j = 1; j <= cols; j++) { if (beforeLines[i - 1] === afterLines[j - 1]) { - dp[i][j] = dp[i - 1][j - 1] + 1; + curr[j] = prev[j - 1] + 1; } else { - dp[i][j] = Math.max(dp[i - 1][j], dp[i][j - 1]); + curr[j] = Math.max(prev[j], curr[j - 1]); } } + [prev, curr] = [curr, prev]; } - const lcs = dp[rows][cols]; + const lcs = prev[cols]; return { deletedLines: rows - lcs, addedLines: cols - lcs, }; }workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide.ts-444-476 (1)
444-476:⚠️ Potential issue | 🟠 MajorIncorrect path:
src/main/java/wso2mi/resourcesshould besrc/main/wso2mi/resources.Lines 445, 463, and 476 reference
src/main/java/wso2mi/resources, but the standard MI project structure used throughout the rest of the prompts (e.g., system.ts line 185, and line 395 of this file) issrc/main/wso2mi/resources. This inconsistency will cause the AI to generate incorrect resource paths.Proposed fix
-When creating supportive resources that are needed for the Integration inside src/main/java/wso2mi/resources, an entry should be added to the src/main/java/wso2mi/resources/artifact.xml. If an artifacts.xml doesn't exist, then create one and add the entry. The format should be as follows: +When creating supportive resources that are needed for the Integration inside src/main/wso2mi/resources, an entry should be added to the src/main/wso2mi/resources/artifact.xml. If an artifacts.xml doesn't exist, then create one and add the entry. The format should be as follows:-For an example if an XSLT file is added inside src/main/java/wso2mi/resources/xslt/conversion.xslt, then the artifact entry can be as follows: +For an example if an XSLT file is added inside src/main/wso2mi/resources/xslt/conversion.xslt, then the artifact entry can be as follows:-Content under api-definitions, conf, connectors and metadata are not added as registry resources and hence do not require an entry in the artifact.xml. Only supportive resources that are needed for the integration and are added inside src/main/java/wso2mi/resources need to be added as registry resources and require an entry in the artifact.xml. +Content under api-definitions, conf, connectors and metadata are not added as registry resources and hence do not require an entry in the artifact.xml. Only supportive resources that are needed for the integration and are added inside src/main/wso2mi/resources need to be added as registry resources and require an entry in the artifact.xml.workspaces/mi/mi-extension/src/rpc-managers/agent-mode/event-handler.ts-65-80 (1)
65-80: 🛠️ Refactor suggestion | 🟠 MajorUse
RPCLayer.getMessenger()instead of casting toanyto access private_messengers.
RPCLayeralready exposes a publicstatic getMessenger(projectUri: string)method (seeRPCLayer.tsline 83–85). The(RPCLayer as any)._messengers.get(...)cast breaks encapsulation unnecessarily and will silently break if the internal field is renamed or refactored.Proposed fix
private sendEventToVisualizer(event: AgentEvent): void { try { - const messenger = (RPCLayer as any)._messengers.get(this.projectUri); + const messenger = RPCLayer.getMessenger(this.projectUri); if (messenger) {workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.ts-149-152 (1)
149-152:⚠️ Potential issue | 🟠 MajorMixed injected vs module-level Anthropic provider creates inconsistency.
createWebSearchExecuteacceptsgetAnthropicClientas a parameter (line 123) and uses it for model creation (line 165), but calls the module-levelgetAnthropicProvider()(line 151) for tool factory discovery. If a caller provides a differently-configured client, the tool factory will still originate from the global cached provider, potentially using different auth or base URL.Either use the injected dependency consistently (derive the provider from it or accept the provider as a parameter too) or document why the global provider is intentionally used for tool factory access.
The same issue applies to
createWebFetchExecuteat line 251.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_store_cache.ts-145-159 (1)
145-159:⚠️ Potential issue | 🟠 MajorNo timeout on the HTTP fetch — could block the agent indefinitely.
fetch(storeUrl)at line 147 has noAbortSignalor timeout. If the connector store backend is slow or unresponsive, this call will hang for the default TCP timeout (potentially minutes), blocking the agent flow.🛡️ Proposed fix: add a timeout using AbortSignal
async function fetchStoreItems(urlTemplate: string, runtimeVersion: string | null): Promise<any[]> { const storeUrl = resolveStoreUrl(urlTemplate, runtimeVersion); - const response = await fetch(storeUrl); + const response = await fetch(storeUrl, { + signal: AbortSignal.timeout(15_000), // 15 second timeout + }); if (!response.ok) { throw new Error(`HTTP ${response.status} ${response.statusText}`); }workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts-234-242 (1)
234-242:⚠️ Potential issue | 🟠 MajorZod schema includes 'XSD' but the TypeScript type does not.
input_typeandoutput_typein the Zod schema (lines 237, 239) accept'JSON' | 'XML' | 'XSD' | 'CSV', but the correspondingCreateDataMapperExecuteFntype intypes.tsonly allows'JSON' | 'XML' | 'CSV'. Since XSD is supported elsewhere in the data mapper system (as evidenced by test suites and the component itself), add'XSD'to the type definition intypes.tsto match the Zod schema.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts-976-976 (1)
976-976:⚠️ Potential issue | 🟠 MajorDefault value mismatch between execute function and tool description.
The execute function defaults
output_modeto'content'(line 976), but the tool description at line 1315 states it "Defaults tofiles_with_matches". This inconsistency will confuse the AI model, which may omit the parameter expecting one default while the code applies a different one.Pick one default and align both locations.
Also applies to: 1315-1315
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts-980-983 (1)
980-983:⚠️ Potential issue | 🟠 MajorReDoS risk: user-supplied pattern passed directly to
RegExpconstructor.The
patternargument is provided by the AI agent and forwarded tonew RegExp(...)without any validation or timeout protection. A maliciously crafted or accidentally pathological regex (e.g.,(a+)+$) could hang the extension host.Consider wrapping the regex construction in a try/catch (to handle invalid patterns gracefully) and optionally adding a complexity check or execution timeout.
🛡️ Proposed fix
- const regex = new RegExp(pattern, caseInsensitive ? 'gi' : 'g'); + let regex: RegExp; + try { + regex = new RegExp(pattern, caseInsensitive ? 'gi' : 'g'); + } catch (regexError) { + return { + success: false, + message: `Invalid regex pattern '${pattern}': ${regexError instanceof Error ? regexError.message : String(regexError)}`, + error: 'Error: Invalid regex pattern' + }; + }workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts-984-986 (1)
984-986:⚠️ Potential issue | 🟠 Major
searchPathis not validated for path traversal.
searchPathis joined withprojectPathviapath.joinwithout any traversal check. A value like../../etcwould resolve outside the project directory. The same issue applies tocreateGlobExecuteat line 1172. Unlike write/edit/read operations, grep and glob skipvalidateFilePathSecurity.🛡️ Proposed fix
// Resolve the search path (always relative to projectPath for security) const fullSearchPath = path.join(projectPath, searchPath); + + // Security: verify resolved path is within project + if (!isPathWithin(projectPath, path.resolve(fullSearchPath))) { + return { + success: false, + message: 'Search path must be within the project directory.', + error: 'Error: Path outside project' + }; + }workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/project_tools.ts-305-311 (1)
305-311:⚠️ Potential issue | 🟠 MajorMissing validation for
item.version?.tagName— dependency could be created withundefinedversion.If a connector store entry has no
versionor theversionobject has notagName, the dependency will be constructed withversion: undefined. This could cause downstream failures when updatingpom.xmlor produce malformed XML.🛡️ Proposed fix
+ const itemVersion = item.version?.tagName; + if (!itemVersion) { + return { + name: itemName, + type: itemType, + success: false, + error: `${itemType === 'connector' ? 'Connector' : 'Inbound endpoint'} '${itemName}' has no version information in the store` + }; + } + // Prepare dependency details const dependencies: DependencyDetails[] = [{ groupId: item.mavenGroupId, artifact: item.mavenArtifactId, - version: item.version?.tagName, + version: itemVersion, type: "zip" }];workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts-604-620 (1)
604-620: 🛠️ Refactor suggestion | 🟠 Major
captureBeforeChangeis called before the file-exists-with-content guard.Line 606 captures an undo checkpoint before line 609–620 checks whether the file already has content and returns an error. This means an undo snapshot is recorded even when the tool performs no mutation. Same pattern occurs in
createEditExecuteat line 866 (before the existence check at 869).Move
captureBeforeChangeafter the existence/content guard so checkpoints are only created when a write will actually occur.♻️ Proposed fix (write)
const fullPath = resolveFullPath(projectPath, file_path); - await undoCheckpointManager?.captureBeforeChange(file_path); // Check if file exists with non-empty content const fileExists = fs.existsSync(fullPath); if (fileExists) { const existingContent = fs.readFileSync(fullPath, 'utf-8'); if (existingContent.trim().length > 0) { ... return { ... }; } } + await undoCheckpointManager?.captureBeforeChange(file_path); + // Create parent directories if they don't existworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/bash_tools.ts-149-155 (1)
149-155:⚠️ Potential issue | 🟠 MajorUnbounded output accumulation for background shells.
shell.outputis a string that grows via+=for everydatachunk from stdout and stderr. A long-running or verbose background command can accumulate tens or hundreds of megabytes, leading to memory pressure in the extension host process.Consider capping
shell.outputto a reasonable size (e.g., keep only the last N bytes), or switching to a ring-buffer / file-backed approach.Example: cap output size
+const MAX_SHELL_OUTPUT_SIZE = 1024 * 1024; // 1 MB + proc.stdout?.on('data', (data) => { - shell.output += data.toString(); + shell.output += data.toString(); + if (shell.output.length > MAX_SHELL_OUTPUT_SIZE) { + shell.output = '...[truncated]\n' + shell.output.slice(-MAX_SHELL_OUTPUT_SIZE); + } }); proc.stderr?.on('data', (data) => { - shell.output += data.toString(); + shell.output += data.toString(); + if (shell.output.length > MAX_SHELL_OUTPUT_SIZE) { + shell.output = '...[truncated]\n' + shell.output.slice(-MAX_SHELL_OUTPUT_SIZE); + } });workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts-652-653 (1)
652-653: 🛠️ Refactor suggestion | 🟠 MajorUnsafe type widening to extract
checkpointId.Line 652 uses an intersection cast
(request as UndoLastCheckpointRequest & { checkpointId?: string }).checkpointIdto access a field not on the declared type. If this field is genuinely needed, it should be added to theUndoLastCheckpointRequestinterface in@wso2/mi-core. Otherwise, this is a type-safety escape hatch that can mask breaking changes if the request shape evolves.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/plan_mode_tools.ts-251-261 (1)
251-261: 🛠️ Refactor suggestion | 🟠 MajorMultiple
as anycasts on event handler calls suggestAgentEventtype is too narrow.Event emissions on lines 251, 339, 471, 557, 649, and 741 all cast to
anyto bypass type checking. This defeats the purpose of having a typed event system. Consider extending theAgentEventunion type in@wso2/mi-coreto includeplan_approval_requested,ask_user,plan_mode_entered,plan_mode_exited, andtodo_updatedevent shapes, then remove the casts.Also applies to: 339-339, 471-471
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/runtime_tools.ts-602-605 (1)
602-605:⚠️ Potential issue | 🟠 MajorStop command may fail: environment is missing
process.env.The
envfor the stop process (line 603-604) is set to onlysetJavaHomeInEnvironmentAndPath(projectPath), unlike the run command (lines 450-453) which merges...process.env. This means the stop process lacksPATHand other environment variables, which will likely cause the stop command to fail or not find required executables.🐛 Proposed fix
- const env = setJavaHomeInEnvironmentAndPath(projectPath); - const stopProcess = childProcess.spawn(stopCommand, [], { shell: true, env }); + const env = { + ...process.env, + ...setJavaHomeInEnvironmentAndPath(projectPath), + }; + const stopProcess = childProcess.spawn(stopCommand, [], { shell: true, env });
🟡 Minor comments (28)
workspaces/mi/mi-extension/src/ai-features/copilot/connectors/inbound_db.ts-36-36 (1)
36-36:⚠️ Potential issue | 🟡 MinorCopy-paste error: every init operation says "Initialize Kafka Inbound Endpoint".
The
descriptionfield for theinitoperation is "Initialize Kafka Inbound Endpoint" across all connectors (SQS on Line 36, CDC on Line 185, ISO8583 on Line 453, Pulsar on Line 1229, SMPP on Line 1786), not just the Kafka one. Since this metadata is fed to the AI agent, incorrect descriptions may confuse the LLM when reasoning about connector types.Example fix for Amazon SQS (Line 36)
- "description": "Initialize Kafka Inbound Endpoint", + "description": "Initialize Amazon SQS Inbound Endpoint",Apply similar fixes for CDC, ISO8583, Pulsar, and SMPP entries.
Also applies to: 185-185, 453-453, 1229-1229, 1786-1786
workspaces/mi/mi-extension/src/ai-features/copilot/connectors/inbound_db.ts-670-675 (1)
670-675:⚠️ Potential issue | 🟡 MinorIncorrect description for
contentTypeparameter in Kafka connector.The
contentTypeparameter's description reads "Consumer group ID to use when consuming messages." — this is the description forgroup.id, not forcontentType.Proposed fix
{ "name": "contentType", "type": "string", "required": true, - "description": "Consumer group ID to use when consuming messages.", + "description": "The content type of the messages (e.g., application/json, text/xml).", "defaultValue": "application/json"workspaces/mi/mi-extension/src/ai-features/agent-mode/context/connectors_guide.ts-118-118 (1)
118-118:⚠️ Potential issue | 🟡 Minor
maxTokensvalue 4069 looks like a typo for 4096.This appears in both the chat example (line 118) and the RAG chat example (line 164). The common token limit for GPT-4o is typically 4096 or 16384.
workspaces/mi/mi-extension/src/ai-features/agent-mode/context/connectors_guide.ts-27-27 (1)
27-27:⚠️ Potential issue | 🟡 MinorTypo: "synaose" should be "synapse".
- - You must initialize the connection via the init operation everytime you use a connector operation in the synaose seqence itself. + - You must initialize the connection via the init operation every time you use a connector operation in the synapse sequence itself.workspaces/mi/mi-extension/src/ai-features/cache-utils.ts-73-84 (1)
73-84:⚠️ Potential issue | 🟡 MinorShallow merge may overwrite existing
anthropicprovider options on the last message.The spread
{ ...message.providerOptions, ...providerOptions }replaces top-level keys entirely. If the last message already hasproviderOptions.anthropicwith other settings, they'll be lost.Suggested deep-merge for the anthropic key
if (index === messages.length - 1) { + const existingAnthropicOpts = (message.providerOptions?.anthropic ?? {}) as Record<string, JSONValue>; + const newAnthropicOpts = (providerOptions.anthropic ?? {}) as Record<string, JSONValue>; return { ...message, providerOptions: { ...message.providerOptions, - ...providerOptions, + anthropic: { + ...existingAnthropicOpts, + ...newAnthropicOpts, + }, }, }; }workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/mode.ts-113-113 (1)
113-113:⚠️ Potential issue | 🟡 MinorTypo: "extreamly" → "extremely".
This text is part of the LLM prompt, so the typo could subtly affect model interpretation.
Fix
-3. **Then present extreamly brief summary of the plan to the user in the chat** - System will attach full plan as a collapsable markdown block in the chat window for the user to review if needed. +3. **Then present an extremely brief summary of the plan to the user in the chat** - System will attach the full plan as a collapsible markdown block in the chat window for the user to review if needed.workspaces/mi/mi-extension/src/ai-features/agent-mode/chat-history-manager.ts-1088-1191 (1)
1088-1191:⚠️ Potential issue | 🟡 MinorSwitch case declarations leak across branches (Biome:
noSwitchDeclarations).Variables declared with
let/constinsidecase 'user':(e.g.,userContent,files,images,queryMatch) are technically accessible from other case branches due to missing block scoping. While thebreakstatements prevent accidental fall-through at runtime, this is a correctness hazard if future changes remove abreak.Wrap the
case 'user':body in a block:Proposed fix (wrap in block)
case 'user': + { // Skip synthetic compact summary messages ... if (msg._compactSynthetic) { continue; } // ... existing user case body ... events.push({ ... }); break; + }Apply the same pattern for other cases flagged by Biome.
workspaces/mi/mi-extension/src/ai-features/agent-mode/chat-history-manager.ts-36-75 (1)
36-75:⚠️ Potential issue | 🟡 MinorRemove duplicate type declarations; import from
@wso2/mi-coreinstead.The types
SessionMetadata,SessionSummary, andGroupedSessionsare already exported from the public API of@wso2/mi-coreand can be imported directly. Maintaining local copies creates a maintenance risk — the types could drift from the canonical definitions in mi-core. Since the file already imports other types from@wso2/mi-core(line 26), add these three types to that import statement and remove the local declarations (lines 36–75).workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_expression_examples.ts-60-62 (1)
60-62:⚠️ Potential issue | 🟡 MinorInconsistent example:
<log>missingcategoryattribute.The
synapse_guide.tsinstructs the AI to usecategoryinstead of the deprecatedlevel, and all other log examples in this file includecategory="INFO". This example omits it, which could lead the AI to generate logs withoutcategory.Suggested fix
- <log> + <log category="INFO">workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide.ts-332-332 (1)
332-332:⚠️ Potential issue | 🟡 MinorTypo: "fault sequencs" → "fault sequences".
workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_expression_guide.ts-40-92 (1)
40-92:⚠️ Potential issue | 🟡 MinorInconsistent list formatting for the 6 global variables.
The first three variables (
payload,vars,params) use unordered bullet syntax (-), while the last three (headers,properties,configs) switch to numbered list syntax (4.,5.,6.). This may render confusingly in markdown.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts-112-112 (1)
112-112:⚠️ Potential issue | 🟡 MinorTypo: "backtickets" → "backticks".
Proposed fix
-Unless explicitly asked for by the user, DO NOT USE backtickets \` or HTML tags like code for file references - always use markdown [text](link) format. +Unless explicitly asked for by the user, DO NOT USE backticks \` or HTML tags like code for file references - always use markdown [text](link) format.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts-53-53 (1)
53-53:⚠️ Potential issue | 🟡 MinorTypo: "unncessory" → "unnecessary".
Proposed fix
-- NEVER create any file unncessory for WSO2 synapse project files unless they're absolutely necessary for achieving your goal. ALWAYS prefer editing an existing file to creating a new one. This includes markdown files. +- NEVER create any file unnecessary for WSO2 synapse project files unless they're absolutely necessary for achieving your goal. ALWAYS prefer editing an existing file to creating a new one. This includes markdown files.workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide.ts-366-388 (1)
366-388:⚠️ Potential issue | 🟡 MinorExample uses deprecated
<log level="full"/>which contradicts the guide's own rule.Line 371 uses
<log level="full"/>in the throwError example, but line 126-140 of this same guide explicitly states thatlevelis deprecated andcategoryshould be used instead. The AI may learn contradictory patterns from this.Proposed fix
<then> - <log level="full"/> + <log category="INFO"> + <message>${payload}</message> + </log> <respond/>workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/system.ts-106-106 (1)
106-106:⚠️ Potential issue | 🟡 MinorTypo: extra closing parenthesis in the prompt example.
Line 106 has a double closing paren in
input.items.find(...)). Since this is part of the system prompt sent to the AI model, it could confuse the model or be reproduced in generated code.Proposed fix
- \`input.items[0]\` (first element) or \`input.items.find(...))\` (conditional) + \`input.items[0]\` (first element) or \`input.items.find(...)\` (conditional)workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.ts-78-116 (1)
78-116:⚠️ Potential issue | 🟡 MinorApproval promise has no timeout or abort-signal integration — can hang forever.
requestWebApprovalcreates a promise (line 100) that resolves only when the user responds viapendingApprovals. If the user abandons the session or the parent agent is aborted, this promise is never settled, leaking resources.Consider wiring the
abortSignal(available from the parent agent) through to this function, or adding a configurable timeout that auto-rejects after a reasonable period.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_store_cache.ts-206-222 (1)
206-222:⚠️ Potential issue | 🟡 MinorInconsistent env var access pattern for store URLs.
Line 210 uses the centralized
APIS.MI_CONNECTOR_STORE_BACKENDconstant, but line 218 readsprocess.env.MI_CONNECTOR_STORE_BACKEND_INBOUND_ENDPOINTSdirectly. TheAPISobject (from../../../constants) doesn't includeMI_CONNECTOR_STORE_BACKEND_INBOUND_ENDPOINTS. If this env var is expected to exist, it should be added to theAPISconstant for consistency and to avoid silentundefinedwhen the env var name changes.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/subagents/explore/agent.ts-140-154 (1)
140-154:⚠️ Potential issue | 🟡 MinorRemove unnecessary type cast when accessing
step.response?.messages.The pattern
(step as any).response?.messagesbypasses type safety. Theresponseproperty withmessagesis part of the official Vercel AI SDK'sStepResulttype—not internal structure. Usestep.response?.messagesdirectly (matching the main agent's pattern) to maintain type safety without theanycast.if (result.steps && result.steps.length > 0) { for (const step of result.steps) { if (step.response?.messages) { fullMessages.push(...step.response.messages); } } logDebug(`[ExploreSubagent] Extracted messages from ${result.steps.length} steps`); }workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts-1282-1284 (1)
1282-1284:⚠️ Potential issue | 🟡 MinorTypo in comment.
"Creates the file_edit to" → "Creates the file_edit tool"
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts-87-89 (1)
87-89:⚠️ Potential issue | 🟡 MinorNaming mismatch: template uses
system_remainderwhile the section is<system_reminder>.The Handlebars variable is
system_remainder(line 88 and context line 283), but it's placed inside a<system_reminder>XML tag (line 87). This appears to be a typo — "remainder" should likely be "reminder" for consistency and clarity.Also applies to: 283-288
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts-1310-1318 (1)
1310-1318:⚠️ Potential issue | 🟡 Minor
typefield in grep input schema is unused.The
typeproperty is declared ingrepInputSchema(line 1314) butcreateGrepExecutenever destructures or uses it. This dead schema field misleads the AI model into providing a parameter that has no effect.Either implement
type-based filtering in the execute function or remove it from the schema.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/compact/agent.ts-325-328 (1)
325-328:⚠️ Potential issue | 🟡 MinorHardcoded summary prefix doesn't match user-triggered compaction.
The prefix "This session is being continued from a previous conversation that ran out of context" is inaccurate when
trigger === 'user'(user explicitly ran/compact). The context didn't "run out" — the user chose to compact.Consider varying the prefix based on
request.trigger.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/agent.ts-636-641 (1)
636-641:⚠️ Potential issue | 🟡 MinorOverly broad abort detection may cause false positives.
errorMsg.toLowerCase().includes('cancel')will match any error message that happens to contain the substring "cancel" in a non-abort context (e.g., "The operation was cancelled by the server due to rate limiting"). This could suppress genuine errors by misclassifying them as user-initiated aborts.Consider removing the
'cancel'substring check or narrowing it to match known error formats more precisely.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/agent.ts-255-264 (1)
255-264:⚠️ Potential issue | 🟡 Minor
process.chdir()is process-global and not concurrency-safe.Even though this is guarded by
ENABLE_DEVTOOLS = false, when enabled during development,process.chdir()affects the entire Node.js process. If multiple agent executions run concurrently (e.g., different projects), they would race on the working directory. Theawait import(...)at line 258 yields control, giving other async tasks a window to observe the changed cwd.Consider documenting that DevTools mode must only be used with a single project, or use an alternative that doesn't mutate global state.
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/project_tools.ts-133-142 (1)
133-142:⚠️ Potential issue | 🟡 MinorAdd
MI_CONNECTOR_STORE_BACKEND_INBOUND_ENDPOINTSto the APIS constants module for consistency.The codebase uses inconsistent approaches to access connector store URLs: connectors use
APIS.MI_CONNECTOR_STORE_BACKEND(e.g., project_tools.ts:91, connector_store_cache.ts:210), while inbound endpoints access the URL directly viaprocess.env.MI_CONNECTOR_STORE_BACKEND_INBOUND_ENDPOINTS(project_tools.ts:133, connector_store_cache.ts:218). The APIS constant defined inconstants/index.tslacks the inbound endpoints property, forcing direct environment variable access. Adding this property to the APIS constants module would ensure both connector and inbound endpoint operations follow the same pattern.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/bash_tools.ts-107-107 (1)
107-107:⚠️ Potential issue | 🟡 MinorLogging the raw command may expose secrets.
If the AI model emits a command containing tokens, passwords, or other sensitive values (e.g.,
curl -H "Authorization: ..."orexport SECRET=...), thislogInfocall will persist them in the output channel. Consider redacting or truncating the logged command.workspaces/mi/mi-extension/src/ai-features/connection.ts-76-100 (1)
76-100:⚠️ Potential issue | 🟡 MinorVariable shadowing and costly debug body parsing.
Two concerns in this debug block:
Line 79:
const headersshadows the outerheadersdeclared on line 56. Rename to e.g.requestHeadersto avoid confusion in later maintenance.Lines 84-96:
JSON.parse(options.body as string)is called on every/messagesrequest solely for debug logging. For large payloads (e.g., messages with file contents), this re-parse is expensive. Guard it behind a debug-level check or remove it before production.Proposed fix for shadowing
- const headers = options.headers as Record<string, string>; - const betaHeader = headers['anthropic-beta'] || headers['Anthropic-Beta'] || 'none'; + const reqHeaders = options.headers as Record<string, string>; + const betaHeader = reqHeaders['anthropic-beta'] || reqHeaders['Anthropic-Beta'] || 'none';workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/runtime_tools.ts-134-167 (1)
134-167:⚠️ Potential issue | 🟡 MinorBuild process has no timeout — a hanging build will block indefinitely.
The
buildProcessspawned on line 138 has no timeout. If the Maven build hangs (e.g., waiting for user input or a network resource), the tool will block forever. Consider adding a configurable timeout (e.g., similar to the server startup timeout pattern at line 518-521).
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts
Show resolved
Hide resolved
IsuruMaduranga
force-pushed
the
mi-agent-mode
branch
from
cbaa137 to
1bdb39d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
workspaces/mi/mi-extension/src/ai-features/connection.ts (1)
180-211:⚠️ Potential issue | 🟠 MajorCache not invalidated when credentials are updated within the same login method.
The Anthropic client cache is keyed solely on
loginMethod. If a user updates their credentials (e.g., corrects an API key or re-authenticates), the cache is not cleared sinceloginMethodremains the same. The stale client with the old credential continues to be used. Additionally, thelogout()function clears stored credentials but does not clearcachedAnthropicorcachedAuthMethod, risking stale client reuse after re-login.Add cache invalidation when credentials are updated (e.g., in
storeAuthCredentials) or when the user logs out, and clear bothcachedAnthropicandcachedAuthMethodin thelogout()function.
🤖 Fix all issues with AI agents
In `@workspaces/mi/mi-extension/src/ai-features/auth.ts`:
- Around line 331-367: checkToken currently calls getAccessToken() and
getLoginMethod() in Promise.all without error handling, so when getAccessToken()
throws (e.g., TOKEN_NOT_AVAILABLE_ERROR_MESSAGE) the error bubbles up and the
machine goes to Disabled; wrap the parallel fetch in a try-catch inside
checkToken (around the Promise.all) and on error treat token and/or loginMethod
as undefined (i.e., fall through to the STS bootstrap path), but rethrow only
unexpected errors if needed; reference the Promise.all call with getAccessToken
and getLoginMethod in the checkToken function to locate where to add the
try-catch.
- Around line 391-447: The rate-limit detection in validateApiKey is unreliable
because it checks error.message for "rate_limit"; update the catch to detect 429
by testing the error's statusCode property (e.g., check 'statusCode' in error
and === 429) and throw the same "Too many requests..." message when true; also
replace the hardcoded model string used in generateText (currently
"claude-3-5-haiku-20241022") with a shared model constant (e.g.,
DEFAULT_ANTHROPIC_MODEL) so the model can be changed centrally.
In `@workspaces/mi/mi-extension/src/ai-features/connection.ts`:
- Around line 142-163: Handle 401 for ANTHROPIC_KEY like MI_INTEL: when
response.status === 401 and loginMethod === LoginMethod.ANTHROPIC_KEY, call
StateMachineAI.sendEvent(AI_EVENT_TYPE.SILENT_LOGOUT) and surface an error
(throw) so the caller can prompt the user to re-enter their key (mirrors the
getRefreshedAccessToken branch for MI_INTEL); update the conditional block
around response.status === 401 to include a branch for LoginMethod.ANTHROPIC_KEY
that triggers SILENT_LOGOUT and returns/throws an appropriate authentication
error.
- Around line 38-42: Update the error message and return value in the
getCopilotLlmApiBaseUrl caller: when proxyUrl is falsy, throw an Error that
references the correct config (e.g., COPILOT_ROOT_URL or instruct to configure
via getCopilotLlmApiBaseUrl) instead of the stale
MI_COPILOT_ANTHROPIC_PROXY_URL, and change the return from the redundant
template literal `\`${proxyUrl}\`` to simply `proxyUrl`; locate the logic around
the variable proxyUrl and function getCopilotLlmApiBaseUrl to make these edits.
In `@workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts`:
- Around line 3051-3054: The POM uses (artifactID ?? name).toLowerCase() which
can diverge from user-specified artifactId and other references; change the
logic so you either stop forcing lowercase (remove .toLowerCase()) or compute a
single normalized variable (e.g., artifactIdNormalized = artifactID ?? name or
artifactIdNormalized = (artifactID ?? name).toLowerCase()) and then use that
single symbol consistently across rootPomXmlContent, folder names (tempName) and
any other places that reference the artifactId so all project files stay in
sync.
🧹 Nitpick comments (13)
workspaces/mi/mi-extension/src/uri-handler.ts (1)
29-32: Consider user-visible feedback instead ofconsole.logfor the legacy route.If a user or an external redirect still hits
/signin, the silentconsole.logprovides no indication of what happened. Awindow.showWarningMessage(...)would surface the situation to the user, helping them understand they need to use the Devant platform extension for authentication.💡 Suggested change
case '/signin': - // Legacy OAuth callback route - no longer used for MI Copilot auth. - console.log("Legacy /signin route called - MI Copilot authentication now uses Devant platform extension."); + // Legacy OAuth callback route - no longer used for MI Copilot auth. + window.showWarningMessage("MI Copilot authentication now uses the Devant platform extension.");workspaces/ballerina/overview-view/src/Overview.tsx (2)
26-47: Consider replacingany[]with concrete types for better type safety.All fields in
Moduleare typed asany[], which disables type checking on the items being filtered and rendered downstream (e.g., accessing.nameand.filePathin the filter blocks). Even a minimal shared shape would catch typos and misuse at compile time:interface ModuleItem { name: string; filePath: string; }Then each field could be
ModuleItem[](or more specific sub-types where needed).Also, the
// Create an interface for …comments are noise — the declarations are self-explanatory.
104-154: Extract the duplicated filter predicate into a helper.All ten filter blocks (services, types, functions, records, objects, classes, constants, enums, listeners, moduleVariables) are identical aside from the field name. This makes the code harder to maintain — any fix to the filtering logic must be replicated in ten places.
♻️ Proposed refactor
Define a helper above the
mapcall and apply it to each field:+ const matchesFilter = (item: { name: string; filePath: string }) => { + if ( + selectedFile === SELECT_ALL_FILES || + item.filePath === selectedFile.replace(workspaceInfo?.workspaceRoot ?? '', '').substring(1) + ) { + return item.name.toLowerCase().includes(query.toLowerCase()); + } + return false; + }; + const filteredComponents = components?.packages.map(pkg => { const modules = pkg.modules.map(module => { - const services = module.services.filter(service => { - if (selectedFile === SELECT_ALL_FILES || service.filePath === selectedFile.replace(workspaceInfo?.workspaceRoot, '').substring(1)) { - return service.name.toLowerCase().includes(query.toLowerCase()); - } - }); - // ... 9 more near-identical blocks ... + return { + ...module, + services: module.services.filter(matchesFilter), + types: module.types.filter(matchesFilter), + functions: module.functions.filter(matchesFilter), + records: module.records.filter(matchesFilter), + objects: module.objects.filter(matchesFilter), + classes: module.classes.filter(matchesFilter), + constants: module.constants.filter(matchesFilter), + enums: module.enums.filter(matchesFilter), + listeners: module.listeners.filter(matchesFilter), + moduleVariables: module.moduleVariables.filter(matchesFilter), + }; });Note the explicit
return false— the current code implicitly returnsundefinedwhen theifcondition is false, which works but is less clear.workspaces/mi/mi-core/src/state-machine-types.ts (1)
292-297: Naming convention inconsistency inAIUserTokensconfirmed.The field
remainingUsagePercentageuses camelCase whilemax_usage,remaining_tokens, andtime_to_resetuse snake_case. This pattern is intentional and repeated consistently across multiple type definitions (state-machine-types.ts,rpc-type.ts,rpc-client.ts,index.ts), indicating the snake_case fields align with an external API or RPC response shape.To resolve this inconsistency, either:
- Convert all fields to camelCase (
maxUsage,remainingTokens,timeToReset) and update all consuming code across multiple files (utils.ts, AIMapButton.tsx, rpc-manager.ts, etc.), or- Add a clarifying comment explaining that snake_case fields reflect the upstream API contract while camelCase is used for calculated/derived fields.
workspaces/mi/mi-extension/src/ai-features/connection.ts (2)
56-74: Custom headers unconditionally override caller-provided headers.The spread order
...options.headers, ...headersmeans the hardcoded headers (e.g.,Content-Type) always override whatever the@ai-sdk/anthropicSDK sets inoptions.headers. This is fragile — if the SDK ever sends a content type other thanapplication/json, or if it sets any of the same header keys, those will be silently overwritten.Consider flipping the spread order so SDK headers take precedence, or selectively merge only the headers that are truly additive (like
x-product,x-metadata, etc.).Proposed fix — let SDK headers take precedence for shared keys
options.headers = { - ...options.headers, ...headers, + ...options.headers, };
76-100: Variable shadowing:headerson line 79 shadows the outerheadersdeclared on line 56.
const headersat line 79 shadows the outerheadersrecord. It works because the outerheaderswas already merged intooptions.headers, but this makes the code confusing to read and maintain. Rename to e.g.mergedHeadersorrequestHeaders.workspaces/mi/mi-extension/src/rpc-managers/ai-features/utils.ts (3)
71-82: Redundant double fetch of credentials from secret storage.
getAccessToken()andgetLoginMethod()both callgetAuthCredentials()internally, resulting in two secret store reads. ForMI_INTEL,getAccessToken()already validates the login method and handles token refresh. You could simplify by callinggetAuthCredentials()once.♻️ Suggested simplification
-export async function getUserAccessToken(): Promise<string> { - const [token, loginMethod] = await Promise.all([ - getAccessToken(), - getLoginMethod(), - ]); - - if (!token || loginMethod !== LoginMethod.MI_INTEL) { - throw new Error('User access token not available'); - } - - return token; -} +export async function getUserAccessToken(): Promise<string> { + const credentials = await getAuthCredentials(); + if (!credentials || credentials.loginMethod !== LoginMethod.MI_INTEL) { + throw new Error('User access token not available'); + } + + const token = await getAccessToken(); + if (!token) { + throw new Error('User access token not available'); + } + + return token; +}
98-104: Unreachable null check —getRefreshedAccessTokenalways throws on failure.Looking at
auth.ts(lines 288-303),getRefreshedAccessTokeneither returns a non-emptystringor throws. Theif (!newToken)guard on line 100 is dead code.♻️ Simplified version
export async function refreshUserAccessToken(): Promise<string> { - const newToken = await getRefreshedAccessToken(); - if (!newToken) { - throw new Error('Failed to refresh access token'); - } - return newToken; + return await getRefreshedAccessToken(); }
87-93: Function namehasAnthropicApiKeyis misleading — it returns the key, not a boolean.The name suggests a boolean check, but the return type is
string | undefined. Consider renaming togetAnthropicApiKeyfor clarity, or keeping the name and returning a boolean if that's the intended contract.workspaces/mi/mi-extension/src/ai-features/auth.ts (2)
164-197:validateStatus: () => truesuppresses all HTTP errors — ensure this is intentional.With
validateStatus: () => true, axios never throws on HTTP status codes, so e.g. 500 or network timeout responses are handled solely by the manual status check on line 180. This is fine for custom error handling, but note there's no request timeout configured — a hanging token exchange endpoint would block indefinitely.⏱️ Add a timeout
{ headers: { 'Content-Type': 'application/json' }, - validateStatus: () => true + validateStatus: () => true, + timeout: 15_000 }
453-455: Unused parameter_isUserLogout— consider removing it.The
logoutfunction accepts_isUserLogoutbut ignores it, always just clearing credentials. If the distinction between user-initiated and silent logout isn't needed here (Devant session managed separately per the comment), consider dropping the parameter to avoid confusion.workspaces/mi/mi-extension/src/ai-features/aiMachine.ts (1)
406-422: Duplicated STS exchange + credential store pattern in three places.The sequence
getPlatformStsToken → exchangeStsToCopilotToken → storeAuthCredentialsis repeated in:
openLogin(here, lines 410-419)setupPlatformExtensionListenercallback (lines 504-513)auth.ts→checkToken(lines 352-361)Consider extracting a shared helper in
auth.ts(e.g.,bootstrapMiIntelFromPlatform) to consolidate this logic and reduce the maintenance burden.♻️ Example helper in auth.ts
// In auth.ts export const bootstrapMiIntelFromPlatform = async (): Promise<{ accessToken: string }> => { const stsToken = await getPlatformStsToken(); if (!stsToken) { throw new Error('Failed to get STS token from platform extension'); } const secrets = await exchangeStsToCopilotToken(stsToken); await storeAuthCredentials({ loginMethod: LoginMethod.MI_INTEL, secrets }); return secrets; };Then use it in all three call sites.
workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts (1)
5406-5410: Avoid passing an empty string as projectPathIf
projectPathis optional, passingundefinedis clearer and avoids accidental path handling downstream.♻️ Suggested adjustment
- response = await langClient.swaggerFromAPI({ apiPath: params.apiPath, port: DebuggerConfig.getServerPort(), projectPath: versionedUrl ? this.projectUri : "", ...(fs.existsSync(swaggerPath) && { swaggerPath: swaggerPath }) }); + const projectPath = versionedUrl ? this.projectUri : undefined; + response = await langClient.swaggerFromAPI({ + apiPath: params.apiPath, + port: DebuggerConfig.getServerPort(), + ...(projectPath && { projectPath }), + ...(fs.existsSync(swaggerPath) && { swaggerPath: swaggerPath }) + });
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/mode.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/mode.ts
Show resolved
Hide resolved
workspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsx
Show resolved
Hide resolved
workspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsx
Outdated
Show resolved
Hide resolved
|
Would it be possible to include a screen recording in the PR description to better illustrate the user experience of this feature? |
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/compact/prompt.ts`:
- Around line 34-54: The prompt template in prompt.ts currently forces the model
to emit an <analysis> block (lines instructing "wrap your analysis in <analysis>
tags"); remove that requirement and instead change the instruction to ask the
model to "think step-by-step internally" and only emit a safe external tag like
<summary>...</summary>; update the prompt text (the prompt template string
exported from prompt.ts) so any sentence requiring an <analysis> block is
replaced with a single sentence directing internal reasoning and explicit output
of only a <summary> tag, leaving surrounding guidance intact.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/agent.ts`:
- Around line 40-47: Request object field tsFilePath in DataMapperAgentRequest
must be validated to ensure it resides inside the provided projectPath before
any read/write; update the agent code that handles request.tsFilePath (the logic
around the file read/write operations and any helper that loads/saves mapping
files) to resolve the absolute path (use path.resolve(projectPath,
request.tsFilePath) or resolve both and normalize) and then enforce a prefix
containment check against the resolved projectPath (or use path.relative and
ensure it does not start with '..'); if the check fails, throw or return a safe
error and do not perform any file I/O. Ensure the same guard is applied wherever
tsFilePath is used in the agent flow (all code paths that read/write the file).
- Around line 106-111: The code in DataMapperAgent is using blocking
fs.existsSync and fs.readFileSync on request.tsFilePath; change these to
non-blocking async APIs (e.g., use fs/promises or vscode.workspace.fs) so the
extension host isn't blocked: replace existsSync + readFileSync with an awaited
fs.promises.access or a try/catch around an awaited fs.promises.readFile (or use
vscode.workspace.fs.readFile with URI), update the containing function to be
async and propagate/await callers, and keep the same error handling and the
logDebug call (logDebug(`[DataMapperAgent] Read file with ${content.length}
characters`)) after the awaited read; reference request.tsFilePath, logDebug,
and the DataMapperAgent function when making these changes.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/prompt.ts`:
- Around line 27-53: The prompt (DATA_MAPPER_PROMPT) currently asks to "Return
ONLY the mapFunction code" but doesn't require a fenced TypeScript block, which
breaks extractTypeScriptCode(); update the template string so the final
instruction requires the mapFunction wrapped in a fenced TypeScript code block
(e.g., start with ```typescript and end with ```), and consider allowing both
```typescript and ```ts to match the extractor; ensure the wording still
enforces "ONLY the mapFunction code" and preserves existing placeholders like
{{`#if` instructions}} and {{ts_file}}.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts`:
- Around line 65-68: The comment flags a policy conflict between the rule in the
system prompt that forbids exposing internal tool names and the injected
<system_reminder> content which embeds template variables like
${EXIT_PLAN_MODE_TOOL_NAME} and ${TODO_WRITE_TOOL_NAME}; update the code that
processes <system_reminder> (the reminder rendering path and any functions that
assemble mode reminders) to either (a) mark those reminders as internal-only and
ensure they are never rendered to end users, or (b) sanitize/replace embedded
tool-name templates with user-safe aliases before display; specifically, locate
the code that injects or renders <system_reminder> tags and the logic enforcing
"do not mention internal tool names" and implement a deterministic
replacement/suppression step so tool-name templates are not leaked to users
while keeping internal reminders intact for agent use.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide.ts`:
- Around line 23-43: The SYNAPSE_GUIDE constant contains conflicting guidance
about the call and send mediators: they are listed as supported in the mediators
list (items including "call" and "send") but later text forbids them; update
SYNAPSE_GUIDE to remove the contradiction by either deleting "call" and "send"
from the supported mediators list or, preferably, rephrasing the later absolute
ban to a conditional guidance such as "avoid using call or send mediators unless
required for legacy compatibility or specific use-cases" so the list and the
later rule are consistent.
- Around line 59-63: The XML example in synapse_guide.ts has an invalid unquoted
attribute (the snippet under the uri-template example uses
value=your_api_key_here); update that example so the API key attribute is
properly quoted (e.g., change value=your_api_key_here to
value="your_api_key_here") so the example in the documentation is valid XML and
will pass validation when copied.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/storage-paths.ts`:
- Around line 59-61: getCopilotSessionDir currently joins sessionId directly
which allows path traversal; sanitize sessionId before joining (e.g., compute a
safeSessionId via path.basename(sessionId) or validate against a UUID regex) and
use that sanitized value when calling getCopilotProjectStorageDir(projectPath)
in getCopilotSessionDir so the returned path cannot escape the project storage
directory.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tool-action-mapper.ts`:
- Around line 41-198: The switch in getToolAction declares case-local variables
that violate noSwitchDeclarations; wrap each affected case in an explicit block
({ ... }) so the const/let bindings are scoped to that case and cannot leak to
others. Specifically add braces around the bodies for FILE_WRITE_TOOL_NAME,
MANAGE_CONNECTOR_TOOL_NAME, SUBAGENT_TOOL_NAME, TODO_WRITE_TOOL_NAME,
BASH_TOOL_NAME, KILL_TASK_TOOL_NAME, and TASK_OUTPUT_TOOL_NAME, keeping the
existing logic and return statements inside the new blocks so behavior is
unchanged.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_tools.ts`:
- Around line 165-176: The code assumes def.version exists when building
messages from connectorDefinitions and inbound endpoint definitions, which can
cause a TypeError; update the loop that iterates
Object.entries(connectorDefinitions) (and the inbound endpoint section that
similarly accesses def.version) to defensively check for def.version before
reading tagName or operations (e.g., treat missing version as "unknown" and skip
operations mapping), and guard accesses to def.version.operations with optional
checks (or use short-circuiting) so you never dereference undefined while still
including a sensible fallback in the message.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts`:
- Around line 301-327: The code currently constructs fullPath by joining
projectPath and dm_config_path (used in tsFilePath resolution) without
validating that the resolved path remains inside the project directory; to fix,
resolve and normalize the candidate path (use path.resolve or fs.realpathSync)
for both absolute and relative dm_config_path values, compute the canonical
project root (e.g., const root = path.resolve(projectPath)), then ensure the
resolved full path starts with the project root (or path.relative(root,
resolvedPath) does not begin with '..'); if the check fails, return the same
error shape rejecting the request; apply this validation before any fs.statSync
or exists checks and reuse the validated resolved path for tsFilePath
construction to prevent path traversal.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts`:
- Around line 976-1054: The code constructs RegExp objects directly from
untrusted inputs (pattern and glob) in the Grep tool (see the regex creation at
"const regex = new RegExp(pattern, ...)" and the globRegex creation) which risks
ReDoS; add input validation in the grep handler and in grepInputSchema to
reject/limit overly long patterns (e.g., max length), attempt RegExp compilation
inside try/catch to return a clear error on invalid regex, and cap / sanitize
the glob before converting to a RegExp (also limit length and escape risky
constructs or reject suspicious patterns); ensure both the main regex and the
globRegex are only constructed after validation and that any RegExp construction
failures return success: false with an error message instead of blocking the
host.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/plan_mode_tools.ts`:
- Around line 288-391: The AskUser tool is logging full question/option content
and the entire event JSON (in createAskUserExecute) which can leak sensitive
user/project data, and it leaves entries in pendingQuestions indefinitely if the
session ends. Replace verbose content logs with minimal metadata (number of
questions, per-question option counts, and mask/hide option labels/paths) and
stop logging the full event JSON; then when you add the pendingQuestions entry
in createAskUserExecute include a createdAt timestamp and a per-question timeout
(e.g., 5 minutes) that automatically removes the entry from pendingQuestions and
resolves it with the USER_CANCELLED result when expired; also wire a cleanup
hook (or mention to call from session disposal code) to iterate pendingQuestions
and reject/resolve and delete entries for that session to avoid leaks.
- Around line 236-277: The call in requestPlanApproval currently casts the
payload to any which hides a type mismatch with the AgentEvent interface;
replace the cast by formalizing the payload type: add a
PlanApprovalRequestedEvent (or extend AgentEvent) in the mi-core RPC types that
includes approvalId, planFilePath?, content, approvalKind, approvalTitle,
approveLabel, rejectLabel, and allowFeedback, then import and use that new type
for the eventHandler call in requestPlanApproval (remove the "as any" cast) so
the object passed to eventHandler is statically typed and matches the mi-core
contract; keep pendingApprovals and the Promise logic unchanged.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/runtime_tools.ts`:
- Around line 41-54: The four duplicated exports (BUILD_PROJECT_TOOL_NAME,
SERVER_MANAGEMENT_TOOL_NAME, BuildProjectExecuteFn, ServerManagementExecuteFn)
should be removed and imported from the central definitions in types.ts and then
re-exported to avoid divergence; replace the local type/constant declarations
with a single import line like import { BUILD_PROJECT_TOOL_NAME,
SERVER_MANAGEMENT_TOOL_NAME, BuildProjectExecuteFn, ServerManagementExecuteFn }
from './types' and export them (or export directly from the import), and update
any local references in runtime_tools.ts to use these imported symbols so there
are no duplicate definitions.
In `@workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-handler.ts`:
- Around line 51-59: The current singleton rpcManagerInstance causes
cross-project routing; replace it with a Map keyed by projectUri (e.g.,
rpcManagerMap: Map<string, MIAgentPanelRpcManager>) and update
registerMIAgentPanelRpcHandlers to get or create the manager via
rpcManagerMap.get(projectUri) / rpcManagerMap.set(projectUri, new
MIAgentPanelRpcManager(projectUri)); ensure all handlers registered inside
registerMIAgentPanelRpcHandlers close over the local manager variable (not a
global) so each MessengerAPI instance uses its own MIAgentPanelRpcManager;
optionally add cleanup logic to remove the map entry when a project is closed.
🟡 Minor comments (21)
workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide_old.ts-60-64 (1)
60-64:⚠️ Potential issue | 🟡 MinorInvalid XML in example:
valueattribute is missing quotes.The example shows
value=your_api_key_herebut XML requires quoted attribute values. Since this guide instructs the AI agent on correct Synapse syntax, the example should be valid XML to avoid the agent reproducing the mistake.Proposed fix
- <variable name="username" value=your_api_key_here type="STRING"/> + <variable name="username" value="your_api_key_here" type="STRING"/>workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide_old.ts-120-120 (1)
120-120:⚠️ Potential issue | 🟡 MinorTypo: "sequencs" → "sequences".
Proposed fix
- - You can define fault sequencs for each API resource or each sequence. + - You can define fault sequences for each API resource or each sequence.workspaces/mi/mi-extension/src/ai-features/agent-mode/context/inbound_db.ts-36-36 (1)
36-36:⚠️ Potential issue | 🟡 MinorCopy-paste error: operation descriptions say "Initialize Kafka Inbound Endpoint" for non-Kafka connectors.
The
descriptionfield for theinitoperation is "Initialize Kafka Inbound Endpoint" across Amazon SQS (Line 36), CDC (Line 185), ISO8583 (Line 453), Pulsar (Line 1229), and SMPP (Line 1786). Since this data is used as AI agent context, incorrect descriptions will mislead the model.Example fix for Amazon SQS (Line 36)
- "description": "Initialize Kafka Inbound Endpoint", + "description": "Initialize Amazon SQS Inbound Endpoint",Apply the same pattern for CDC, ISO8583, Pulsar, and SMPP entries.
Also applies to: 185-185, 453-453, 1229-1229, 1786-1786
workspaces/mi/mi-extension/src/ai-features/agent-mode/context/inbound_db.ts-670-675 (1)
670-675:⚠️ Potential issue | 🟡 MinorIncorrect description for
contentTypeparameter in Kafka connector.The description reads "Consumer group ID to use when consuming messages." which is the description for the
group.idparameter. It should describe the content type of consumed messages.Proposed fix
{ "name": "contentType", "type": "string", "required": true, - "description": "Consumer group ID to use when consuming messages.", + "description": "The content type of the messages consumed from Kafka.", "defaultValue": "application/json"workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts-976-977 (1)
976-977:⚠️ Potential issue | 🟡 Minorgrep schema advertises options that aren’t implemented.
typeis accepted but ignored, and the schema saysoutput_modedefaults tofiles_with_matcheswhile the implementation defaults tocontent. Align schema/docs with behavior or implement the options.📌 Possible alignment (remove unsupported `type`, fix default text)
- type: z.string().optional().describe(`File type to search (rg --type). Common types: js, py, rust, go, java, etc. More efficient than include for standard file types.`), - output_mode: z.enum(['content', 'files_with_matches']).optional().describe(`Output mode: "content" shows matching lines (supports -A/-B/-C context, -n line numbers, head_limit), "files_with_matches" shows only file paths (supports head_limit). Defaults to "files_with_matches".`), + output_mode: z.enum(['content', 'files_with_matches']).optional().describe(`Output mode: "content" shows matching lines (supports -A/-B/-C context, -n line numbers, head_limit), "files_with_matches" shows only file paths (supports head_limit). Defaults to "content".`),Also applies to: 1310-1317
workspaces/mi/mi-extension/src/ai-features/auth.ts-71-82 (1)
71-82:⚠️ Potential issue | 🟡 MinorAlign LLM base‑URL JSDoc with actual fallback behavior.
The comment claims fallback to legacy env vars, but the function returnsundefinedwhenCOPILOT_ROOT_URLis missing.📝 Suggested doc fix
- * Prefers COPILOT_ROOT_URL-derived endpoint and falls back to legacy proxy env vars. + * Uses COPILOT_ROOT_URL-derived endpoint; returns undefined if not configured.workspaces/mi/mi-extension/src/ai-features/agent-mode/tool-action-mapper.ts-154-163 (1)
154-163:⚠️ Potential issue | 🟡 MinorAvoid “undefined” action text when the shell command is missing.
cmdPreviewbecomes the string"undefined"iftoolInput.commandis absent, which leaks into UI labels.🧩 Suggested fix
- const bashDesc = toolInput?.description; - const cmdPreview = toolInput?.command?.substring(0, 50) + (toolInput?.command?.length > 50 ? '...' : ''); - const displayText = bashDesc || cmdPreview || 'command'; + const bashDesc = toolInput?.description; + const cmd = toolInput?.command; + const cmdPreview = cmd ? `${cmd.substring(0, 50)}${cmd.length > 50 ? '...' : ''}` : ''; + const displayText = bashDesc || cmdPreview || 'command';workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/bash_tools.ts-107-107 (1)
107-107:⚠️ Potential issue | 🟡 MinorUser-supplied commands are logged verbatim.
logInfowill record the full shell command. If the LLM-generated command contains secrets (env vars, tokens passed inline), they end up in logs. Consider truncating or redacting sensitive patterns.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/runtime_tools.ts-462-470 (1)
462-470:⚠️ Potential issue | 🟡 MinorConsider quoting
vmArgswhen passing tospawn()withshell: true.When
shell: true, the command and args are passed to the shell as a concatenated string. If anyvmArgsentry contains spaces or special characters (e.g.,-Dsome.key="value with spaces"), they may not be properly escaped by the shell. To avoid potential parsing issues, either quote each arg in the vmArgs array before passing them, or useshell: falseif the executable can be invoked directly without a shell.This same pattern appears in
debugHelper.ts(line 340) and should be reviewed there as well.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.ts-152-152 (1)
152-152:⚠️ Potential issue | 🟡 MinorAdd fallback versions to line 152 for consistency with line 252.
Line 152 specifies only
['webSearch_20250305'], whereas line 252 already implements a fallback pattern with['webFetch_20250910', 'webFetch_20250305']. Update line 152 to include fallback versions (e.g.,['webSearch_20250305', 'webSearch_20250305']if older versions exist, or document why fallback is not needed) to match the resilience strategy already applied to webFetch.workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide.ts-45-58 (1)
45-58:⚠️ Potential issue | 🟡 MinorMinor text polish in a behavior-driving guide (typos + clarity).
A few typos reduce trust (“fault sequencs”, etc.) and some statements are absolute but context-dependent (e.g., DB credentials always inside mediators vs deployment.toml).
Also applies to: 444-477
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/compact/prompt.ts-19-28 (1)
19-28:⚠️ Potential issue | 🟡 MinorMake reminder tag spelling consistent (comment vs implementation).
Line 22 says
<system-reminder>but the actual wrappers use<system_reminder>. Aligning terminology prevents future misuse/copy-paste bugs.Also applies to: 100-112
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts-56-57 (1)
56-57:⚠️ Potential issue | 🟡 MinorTypos in “system-of-record” prompt text (small but worth fixing).
Line 56: “unncessory” → “unnecessary”
Line 115: “backtickets” → “backticks”
This text will be repeatedly seen/used by the model; small errors can propagate.Also applies to: 115-116
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_store_cache.ts-154-168 (1)
154-168:⚠️ Potential issue | 🟡 MinorNo timeout on
fetch()— a slow/unresponsive store API could block the agent indefinitely.
fetchStoreItemscallsfetch(storeUrl)without a timeout. If the connector store is down or extremely slow, this will hang until the OS-level TCP timeout (potentially minutes). Consider adding anAbortSignal.timeout():🛡️ Proposed fix
async function fetchStoreItems(urlTemplate: string, runtimeVersion: string | null): Promise<any[]> { const storeUrl = resolveStoreUrl(urlTemplate, runtimeVersion); - const response = await fetch(storeUrl); + const response = await fetch(storeUrl, { + signal: AbortSignal.timeout(15_000), // 15 second timeout + }); if (!response.ok) { throw new Error(`HTTP ${response.status} ${response.statusText}`);workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/agent.ts-639-646 (1)
639-646:⚠️ Potential issue | 🟡 MinorFragile abort detection via string matching could produce false positives.
Checking
errorMsg.toLowerCase().includes('abort')orincludes('cancel')will match any error whose message incidentally contains these words (e.g., "Failed to cancel subscription" or "Transaction aborted by database"). The first three checks (error?.name === 'AbortError',request.abortSignal?.aborted,error?.code === 'ABORT_ERR') are already robust. Consider dropping the string-matching heuristics or at least tightening them.♻️ Suggested tightening
const isAborted = error?.name === 'AbortError' || request.abortSignal?.aborted || - errorMsg.toLowerCase().includes('abort') || - errorMsg.toLowerCase().includes('cancel') || error?.code === 'ABORT_ERR';workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/compact/agent.ts-255-269 (1)
255-269:⚠️ Potential issue | 🟡 Minor
getSystemPrompt()called withoutruntimeVersion— compact agent may use wrong system prompt for older runtimes.
getSystemPrompt(frommain/system.ts, line 234) selects betweenSYSTEM_PROMPTandSYSTEM_PROMPT_OLDbased on runtime version. Here it's called without a version, so it always returns the newer prompt. If the project targets an older MI runtime (< 4.4.0), the summary will be based on the wrong system prompt, potentially producing inaccurate compaction.Consider passing the runtime version through
CompactAgentRequestand forwarding it here.🐛 Proposed fix
export interface CompactAgentRequest { messages: any[]; trigger: 'user' | 'auto'; projectPath: string; + runtimeVersion?: string | null; }- const systemPrompt = getSystemPrompt(); + const systemPrompt = getSystemPrompt(request.runtimeVersion);workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/agent.ts-282-289 (1)
282-289:⚠️ Potential issue | 🟡 MinorConsider making the thinking
budgetTokensconfigurable rather than hardcoded.The 1024 value is Anthropic's recommended starting point and minimum for extended thinking. While Anthropic suggests increasing the budget for complex reasoning tasks, the current hardcoded value allows toggling thinking on/off but not tuning the budget per request. Since
request.thinkingalready enables per-request control, extending this to include a configurablebudgetTokenswould allow better optimization based on task complexity without hardcoding a single value across all use cases.workspaces/mi/mi-extension/src/ai-features/agent-mode/chat-history-manager.ts-1063-1064 (1)
1063-1064:⚠️ Potential issue | 🟡 Minor
convertToEventFormatuses current time instead of stored timestamps.Line 1064 creates
new Date().toISOString()for each message, so every UI event gets the "replay time" rather than the original message timestamp. TheJournalEntryhas atimestampfield — if the raw entry were available (it is forcompact_summaryandundo_checkpointtypes, wheremsg.timestampis used), it should be used for all message types too.Since model messages are unwrapped from their
JournalEntrybefore being passed here, the original timestamp is lost. Consider preserving the entry timestamp on the unwrapped message (e.g.,_timestamp) ingetMessages()soconvertToEventFormat()can use it.workspaces/mi/mi-extension/src/ai-features/agent-mode/chat-history-manager.ts-341-356 (1)
341-356:⚠️ Potential issue | 🟡 Minor
loadMetadatasilently upgrades missingsessionVersionto current version.When loading metadata for a session that predates versioning (no
sessionVersionfield),loadMetadata()returns the metadata withsessionVersionset toSESSION_STORAGE_VERSION. However, this patched value is only in memory — subsequentupdateMetadata()calls will persist it. This effectively upgrades legacy sessions to the current version on first touch, which prevents future version-incompatibility detection for those sessions.If this is intentional (i.e., legacy sessions are always compatible), the behavior is fine — but it should be documented in a comment.
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts-85-91 (1)
85-91:⚠️ Potential issue | 🟡 Minor
isFilePathheuristic can produce false positives on inline content.A string like
"data.json"(meant as inline content) or any input ending with.json,.xml,.xsd, or.csvwill be misidentified as a file path. Conversely, a relative path likeresources/input.txtwon't match because.txtisn't in the extension list and it doesn't start with./,/, orsrc/.Consider tightening the heuristic — e.g., require the extension check only when the string also contains a path separator, or check for the presence of whitespace/newlines to distinguish inline content from paths.
workspaces/mi/mi-extension/src/ai-features/agent-mode/chat-history-manager.ts-1088-1191 (1)
1088-1191:⚠️ Potential issue | 🟡 MinorSwitch case declarations leak across cases (Biome lint error).
Variables
userContent,files,images,attachmentMetadata, andqueryMatchdeclared in thecase 'user'block are accessible fromcase 'assistant'andcase 'tool'cases because there are no block scopes. Biome flagged this asnoSwitchDeclarations.Wrap the
case 'user'body in braces to create a block scope.🔧 Proposed fix
switch (msg.role) { - case 'user': + case 'user': { // Skip synthetic compact summary messages ... if (msg._compactSynthetic) { continue; } // ... existing user case body ... break; + } - case 'assistant': + case 'assistant': { // ... existing assistant case body ... break; + } - case 'tool': + case 'tool': { // ... existing tool case body ... break; + } }
🧹 Nitpick comments (33)
workspaces/mi/mi-extension/src/ai-features/agent-mode/context/inbound_db.ts (1)
19-1968: Consolidate duplicateINBOUND_DBexports to a single source of truth.Both
workspaces/mi/mi-extension/src/ai-features/agent-mode/context/inbound_db.tsandworkspaces/mi/mi-extension/src/ai-features/copilot/connectors/inbound_db.tsexport the sameINBOUND_DBconstant. While the files are similar (both 1968 lines), they have different checksums, indicating content divergence. To prevent maintenance issues and drift, exportINBOUND_DBfrom a shared location and import it in both modules.workspaces/mi/mi-extension/src/ai-features/copilot/message-utils.ts (1)
83-92: Consider centralizingIMAGE_MIMETYPESto avoid drift.
The same whitelist now exists in attachment-utils; a shared constant would keep Copilot and agent-mode validators in sync.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/bash_tools.ts (2)
195-200:treeKillon timeout lacks an error callback.If
treeKillfails (e.g., the process already exited between the check and the kill), the error is silently swallowed. Consider adding an error callback to at least log the failure.Suggested fix
const timeoutHandle = setTimeout(() => { timedOut = true; if (proc.pid) { - treeKill(proc.pid, 'SIGKILL'); + treeKill(proc.pid, 'SIGKILL', (err) => { + if (err) { + logError(`[ShellTool] Failed to kill timed-out process: ${err.message}`); + } + }); } }, effectiveTimeout);
436-439: Complex ternary on line 439 is hard to follow.The unified task view construction packs a lot of conditional logic into a single expression. Consider extracting a small helper function for readability.
workspaces/mi/mi-extension/src/ai-features/connection.ts (1)
76-80: Variableheaderson line 79 shadows the outerheadersfrom line 56.Both are in scope simultaneously, which is confusing for readers. Rename the inner variable to avoid shadowing.
Suggested fix
- const headers = options.headers as Record<string, string>; - const betaHeader = headers['anthropic-beta'] || headers['Anthropic-Beta'] || 'none'; + const mergedHeaders = options.headers as Record<string, string>; + const betaHeader = mergedHeaders['anthropic-beta'] || mergedHeaders['Anthropic-Beta'] || 'none';workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/types.ts (1)
55-80: TODO comment flags incomplete file extension list.The comment on line 55 notes this list might be incomplete. Consider tracking this as a follow-up issue to audit project files against the allowlist.
Would you like me to open an issue to track auditing
VALID_FILE_EXTENSIONSagainst actual MI project file types?workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/validation-utils.ts (1)
60-64: SynchronousreadFileSyncused inside an async function.Since
validateXmlFileis alreadyasyncand the language client call on line 61 is awaited, the file read on line 63 could usefs.promises.readFilefor consistency and to avoid blocking the event loop on large XML files.Suggested fix
+ import { promises as fsPromises } from 'fs'; + // ... or at top of file const diagnosticsResponse = await langClient.getCodeDiagnostics({ fileName: absolutePath, - code: fs.readFileSync(absolutePath, 'utf8') + code: await fsPromises.readFile(absolutePath, 'utf8') });workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.ts (1)
122-127:getAnthropicClientparameter is accepted butgetAnthropicProvideris imported and used directly.
createWebSearchExecuteacceptsgetAnthropicClientas a parameter (line 123), used only for model instantiation (line 165). However, tool factory discovery on line 151 uses the directly importedgetAnthropicProvider. This creates a hidden dependency that bypasses the injected parameter, making the function harder to test and reason about.Consider either injecting the provider as a parameter too, or documenting why the split exists.
Also applies to: 149-152
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/subagent_tool.ts (2)
58-81:loadSubagentHistorysilently re-throws non-ENOENT errors.On line 79, any file-system error other than
ENOENT(e.g., permission denied, corrupt JSON) is re-thrown without additional context. Consider wrapping with a descriptive message.Suggested fix
} catch (error: any) { if (error.code === 'ENOENT') { throw new Error(`Subagent with ID ${subagentId} not found. Cannot resume.`); } - throw error; + throw new Error(`Failed to load subagent history for ${subagentId}: ${error.message}`); }
67-69: No validation of JSON parse results in history loading.Each line is parsed with
JSON.parse(line)but there's no validation that the parsed object is a valid message. Corrupt or truncated JSONL could produce unexpected values in the messages array. Given this is internal persistence, the risk is low but worth noting.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/plan_mode_tools.ts (3)
98-108: Pick a deterministic “existing plan file” when multiple.mdfiles exist.Line 101-108 picks
planFiles[0]fromreaddir()order, which is not guaranteed stable across platforms/filesystems. If multiple plan files exist (e.g., leftover files), this can select a surprising file.Example: prefer newest mtime or a fixed filename
- const planFiles = files.filter(f => f.endsWith('.md')); + const planFiles = files.filter(f => f.endsWith('.md')).sort();(Or: stat each and pick the newest mtime if that’s the intended behavior.)
623-630: mtime-based “plan updated” check is fragile on fast edits / coarse filesystem timestamps.Line 624-630 requires
planMtimeMs > baselineMtimeMs + 1. On some systems/edit flows, edits can land within the same timestamp tick, causing false negatives (“not updated” even though content changed).Suggestion: track a baseline content hash (sha256 of trimmed content) rather than (or in addition to) mtime.
730-784: Validatetodosinput (empty list, multiplein_progress) to match the tool contract.The tool description says “Only ONE task should be in_progress at a time” (Line 779-780) but the zod schema doesn’t enforce it, and execute doesn’t validate it. This is easy to harden and avoids UI inconsistencies.
workspaces/mi/mi-extension/src/ai-features/agent-mode/storage-paths.ts (1)
30-33: Compute project basename before separator normalization (portability).Line 31-33 normalizes to forward slashes, then Line 50 calls
path.basename(normalizedPath). Node usually handles/on Windows, but it’s safer/clearer to derive basename frompath.resolve(projectPath)before doing string replacements for hashing.Also applies to: 48-53
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/agent.ts (1)
63-82: Make extraction stricter: fail if response includes non-code or doesn’t definemapFunction.Right now, if the model returns prose + code without fences,
extractTypeScriptCode()returns full text andremoveMapFunctionEntry()throws. That’s okay, but error messages could be more actionable, and you can reduce failures by requiring fenced output (prompt-side) and validating the extracted content starts with a function declaration.Also applies to: 140-146
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/compact/agent.ts (2)
293-303:process.chdir()mutates global state — races possible if multiple agents run concurrently.Although guarded by
ENABLE_DEVTOOLS = false, the pattern of changingprocess.cwd()and restoring it is not safe under concurrency. If an exception were thrown between thechdircalls, the CWD would remain changed. Consider using atry/finallyat minimum:🛡️ Safer pattern
if (ENABLE_DEVTOOLS) { const originalCwd = process.cwd(); - process.chdir(request.projectPath); - const { devToolsMiddleware } = await import('@ai-sdk/devtools'); - model = wrapLanguageModel({ - model, - middleware: devToolsMiddleware() as any, - }); - process.chdir(originalCwd); + try { + process.chdir(request.projectPath); + const { devToolsMiddleware } = await import('@ai-sdk/devtools'); + model = wrapLanguageModel({ + model, + middleware: devToolsMiddleware() as any, + }); + } finally { + process.chdir(originalCwd); + } }
325-328: Minor formatting inconsistency in the summary prefix.The template string has
\n ${summary}with a trailing space after the newline. This may be intentional for readability, but consider using\n\nfor a cleaner paragraph break:- summary: `This session is being continued from a previous conversation that ran out of context. The summary below covers the earlier portion of the conversation. \n ${summary}` + summary: `This session is being continued from a previous conversation that ran out of context. The summary below covers the earlier portion of the conversation.\n\n${summary}`workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/compact/tools.ts (1)
97-122: Consider importing tool name constants from the barrel instead of re-exporting.These constants are already exported from
../../tools/types(and re-exported via../../tools/index.ts). The re-export here creates a third path for the same constants. If a new tool is added, this file must also be updated. Consumers of the compact module could import directly from../../tools/types.That said, this is a minor concern — the module boundary encapsulation may be intentional.
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/agent.ts (2)
260-270: Sameprocess.chdir()issue as in compact agent — wrap intry/finally.If the dynamic import or
wrapLanguageModelthrows, the working directory won't be restored. This is the same pattern flagged in the compact agent.🛡️ Proposed fix
if (ENABLE_DEVTOOLS) { const originalCwd = process.cwd(); - process.chdir(request.projectPath); - const { devToolsMiddleware } = await import('@ai-sdk/devtools'); - model = wrapLanguageModel({ - model, - middleware: devToolsMiddleware() as any, - }); - process.chdir(originalCwd); + try { + process.chdir(request.projectPath); + const { devToolsMiddleware } = await import('@ai-sdk/devtools'); + model = wrapLanguageModel({ + model, + middleware: devToolsMiddleware() as any, + }); + } finally { + process.chdir(originalCwd); + } }
424-516: Large if-else chain for tool display inputs could benefit from a registry pattern.This ~90-line block maps each tool name to its display-relevant input fields. While functional and readable, it will grow linearly with each new tool. A registry object or a
getToolDisplayInput(toolName, toolInput)utility would centralize this and reduce the per-tool boilerplate.Not urgent — the current structure works and each tool has unique field selections. But consider extracting this if the tool count keeps growing.
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/tool-result-persistence.ts (1)
59-61:getDisplayPathis a no-op identity function.If this is a placeholder for future path transformation (e.g., making paths relative or clickable), consider adding a brief TODO comment. Otherwise, callers could use
filePathdirectly.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts (1)
85-89: Typo in template variable:system_remaindershould likely besystem_reminder.The variable is named
system_remainder(line 88) but represents a system reminder containing mode context. This naming inconsistency could confuse future maintainers.Additionally, the value at line 285 starts with a stray period (
.) before the<mode>tag:system_remainder: `. <mode>This period will appear in the rendered prompt. Was this intentional?
♻️ Suggested cleanup
In the template (line 88):
-{{system_remainder}} +{{system_reminder}}In the context (line 285):
- system_remainder: `. - <mode> - ${mode.toUpperCase()} - </mode> - ${modeReminder}` + system_reminder: `<mode> +${mode.toUpperCase()} +</mode> +${modeReminder}`Also applies to: 285-290
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/project_tools.ts (1)
88-127: Store fetching duplicates logic fromconnector_store_cache.tswithout caching.This file fetches directly from the connector/inbound store APIs on every tool invocation, duplicating the URL resolution (
replace('${version}', ...)) and response validation logic that already exists inconnector_store_cache.ts(fetchStoreItems,resolveStoreUrl). Additionally, these fetches have no timeout protection.Consider either:
- Extracting a shared
fetchStoreItemsutility that both modules use, or- Using the cache layer from
connector_store_cache.ts(though the full store data with Maven coordinates may differ from the cached catalog).At minimum, the duplicate
process.env.MI_CONNECTOR_STORE_BACKEND_INBOUND_ENDPOINTSaccess (line 133) and theAPIS.MI_CONNECTOR_STORE_BACKEND.replace(...)pattern (line 91) should be consolidated.Also applies to: 131-177
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_store_cache.ts (1)
221-238: AddMI_CONNECTOR_STORE_BACKEND_INBOUND_ENDPOINTSto theAPISconstant for consistency.This file uses
APIS.MI_CONNECTOR_STORE_BACKENDon line 225 but accessesprocess.env.MI_CONNECTOR_STORE_BACKEND_INBOUND_ENDPOINTSdirectly on line 233. While the current code works, adding the inbound endpoints URL to theAPISconstant inconstants/index.tswould maintain consistent access patterns for API endpoint configuration.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/index.ts (1)
189-219: Remove unnecessary dynamicrequire()calls in favor of static imports already available.The static re-exports at the top of this file (lines 20–36 for
createFileToolsand related functions; line 20 for types viaexport * from './types') already make these symbols available with full type safety. The dynamicrequire()calls insidecreateFileToolsre-import the same symbols and serve no purpose—there is no circular dependency (verified: neitherfile_tools.tsnortypes.tsimport fromindex.ts). Use the statically exported symbols directly instead:♻️ Proposed refactor
export function createFileTools(projectPath: string, modifiedFiles?: string[]) { - // Import here to avoid circular dependencies - const { - createWriteExecute, - createReadExecute, - createEditExecute, - createGrepExecute, - createGlobExecute, - createWriteTool, - createReadTool, - createEditTool, - createGrepTool, - createGlobTool, - } = require('./file_tools'); - - const { - FILE_WRITE_TOOL_NAME, - FILE_READ_TOOL_NAME, - FILE_EDIT_TOOL_NAME, - FILE_GREP_TOOL_NAME, - FILE_GLOB_TOOL_NAME, - } = require('./types'); - return { [FILE_WRITE_TOOL_NAME]: createWriteTool(createWriteExecute(projectPath, modifiedFiles)), [FILE_READ_TOOL_NAME]: createReadTool(createReadExecute(projectPath), projectPath), [FILE_EDIT_TOOL_NAME]: createEditTool(createEditExecute(projectPath, modifiedFiles)), [FILE_GREP_TOOL_NAME]: createGrepTool(createGrepExecute(projectPath)), [FILE_GLOB_TOOL_NAME]: createGlobTool(createGlobExecute(projectPath)), }; }workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_tools.ts (2)
35-62: Duplicated lookup logic betweengetConnectorDefinitionsandgetInboundEndpointDefinitions.Both functions are structurally identical — they iterate a list, match by
connectorName, and build aRecord. Consider extracting a single generic helper, e.g.getDefinitionsByName(names, items), to reduce duplication.♻️ Example unified helper
-function getConnectorDefinitions(connectorNames: string[], connectors: any[]): Record<string, any> { - const definitions: Record<string, any> = {}; - for (const name of connectorNames) { - const connector = connectors.find(c => c.connectorName === name); - if (connector) { - definitions[name] = connector; - } - } - return definitions; -} - -function getInboundEndpointDefinitions(inboundNames: string[], inbounds: any[]): Record<string, any> { - const definitions: Record<string, any> = {}; - for (const name of inboundNames) { - const inbound = inbounds.find(i => i.connectorName === name); - if (inbound) { - definitions[name] = inbound; - } - } - return definitions; -} +function getDefinitionsByName(names: string[], items: any[]): Record<string, any> { + const definitions: Record<string, any> = {}; + for (const name of names) { + const item = items.find(i => i.connectorName === name); + if (item) { + definitions[name] = item; + } + } + return definitions; +}
237-249:(tool as any)bypasses type safety for the Vercel AI SDKtoolfunction.The comment mentions this is to avoid "TypeScript deep instantiation issues with Zod". This is an acceptable workaround, but worth tracking. If/when the upstream
aiSDK or Zod version fixes the deep instantiation issue, this cast should be removed to restore full type-checking.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts (1)
171-215: Duplicated input/output schema processing blocks.Lines 171–192 (input) and 194–215 (output) follow an identical pattern: check
isFilePath, read file or use inline content, callgenerateSchemaFromContent, thenupdateTsFileIoTypes. Consider extracting a helper such asprocessSchema(projectPath, schema, type, tsFilePath, dmName)to eliminate the duplication.♻️ Sketch of extracted helper
async function resolveAndApplySchema( projectPath: string, schemaInput: string, ioType: IOType, schemaType: string, dmName: string, tsFilePath: string ): Promise<ToolResult | null> { let content: string; if (isFilePath(schemaInput)) { const fullPath = path.join(projectPath, schemaInput); if (!fs.existsSync(fullPath)) { return { success: false, message: `${ioType} schema file not found: ${schemaInput}`, error: 'Error: File not found', }; } content = fs.readFileSync(fullPath, 'utf8'); } else { content = schemaInput; } const jsonSchema = await generateSchemaFromContent(projectPath, ioType, content, schemaType); await updateTsFileIoTypes(dmName, tsFilePath, jsonSchema, ioType); return null; }workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/tools.ts (2)
266-294: Shell command blocklist is inherently bypassable.The blocklist approach in
getPlanModeShellRestrictionReasoncan be circumvented via interpreters (python -c "import os; os.remove('f')",node -e "...",perl -e "..."), shell built-ins (eval,exec), encoded/obfuscated commands, or tools not in the list (e.g.,rsync --remove-source-files).This is understandable as a best-effort guard for an LLM agent running locally, but consider adding a comment acknowledging the limitation so future maintainers don't assume it's a strong security boundary.
149-149: Import statement after the re-export block.
AgentEventHandleris imported at Line 149, after the re-export block (Lines 124-148). This breaks the conventional import grouping at the top of the file.♻️ Move import to top
import * as path from 'path'; import { getCopilotSessionDir } from '../../storage-paths'; +import { AgentEventHandler } from './agent'; // Re-export tool name constants for use in agent.ts export { ... }; -import { AgentEventHandler } from './agent';workspaces/mi/mi-extension/src/ai-features/agent-mode/chat-history-manager.ts (3)
384-389: Per-message metadata I/O in batch saves is inefficient.
saveMessagescallssaveMessagein a loop, and eachsaveMessageinvocation callsincrementMessageCount(), which does aloadMetadata()→updateMetadata()→saveMetadata()round-trip (3 file I/O operations). For a batch of N messages, this results in ~3N metadata file operations.Consider batching the metadata update — increment the count once after all messages are written.
♻️ Sketch of batched metadata update
async saveMessages(messages: any[], options?: { totalInputTokens?: number; chatId?: number }): Promise<void> { if (messages.length === 0) { return; } for (let i = 0; i < messages.length; i++) { const isLast = i === messages.length - 1; - await this.saveMessage(messages[i], isLast ? options : options?.chatId !== undefined ? { chatId: options.chatId } : undefined); + // Write entry + handle title, but skip per-message metadata increment + await this.writeMessageEntry(messages[i], isLast ? options : options?.chatId !== undefined ? { chatId: options.chatId } : undefined); } + // Batch metadata update + const metadata = await this.loadMetadata(); + if (metadata) { + await this.updateMetadata({ messageCount: metadata.messageCount + messages.length }); + } }Also applies to: 499-508
654-697: Full file reads forgetLatestModeandgetLastUsagemay be slow for large sessions.Both methods read the entire JSONL file into memory and parse every line. For long-running sessions with thousands of entries, this could cause noticeable latency. Consider caching the latest mode and usage in the metadata file, which is already read for other purposes, or reading the file from the end (e.g., reading the last N bytes).
849-882: Double metadata read per session in listing flow.
listSessions()callsisSessionCompatible()for each session (reads metadata.json), thenlistSessionsWithMetadata()callsgetSessionSummary()for each session (reads metadata.json again). This means metadata.json is read twice per session during listing.Consider combining compatibility check and summary extraction into a single pass.
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/compact/prompt.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/agent.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/agent.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/prompt.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/plan_mode_tools.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/plan_mode_tools.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/runtime_tools.ts
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-handler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@workspaces/mi/mi-core/src/rpc-types/agent-mode/types.ts`:
- Around line 482-487: SearchMentionablePathsRequest currently declares query as
mandatory but the RPC manager handlers (references: rpc-manager.ts where
(request.query || '').trim() is used and the duplicate declaration with query?:
string) treat it as optional; change the interface SearchMentionablePathsRequest
to make query optional (query?: string) and adjust its JSDoc to note that an
empty or omitted query should return root items so the type matches the runtime
behavior used by the rpc-manager handlers.
In `@workspaces/mi/mi-core/src/rpc-types/ai-features/index.ts`:
- Around line 71-74: The RPC client's fetchUsage return type is using legacy
field names (max_usage, remaining_tokens, time_to_reset) and must be aligned to
the canonical core type: update the fetchUsage declaration in the RPC client
(the fetchUsage method) so its Promise resolves to { remainingUsagePercentage?:
number; resetsIn?: number } | undefined, removing the old fields and ensuring
the shape and optionality match the core definition.
In `@workspaces/mi/mi-core/src/state-machine-types.ts`:
- Around line 292-295: AIMapButton.tsx and rpc-client.ts still reference old
AIUserTokens fields; update AIMapButton.tsx usages of
machineView.userTokens.max_usage and machineView.userTokens.remaining_tokens to
machineView.userTokens.remainingUsagePercentage and
machineView.userTokens.resetsIn (adjust any percentage calculations accordingly
to use remainingUsagePercentage), and change the fetchUsage() return type in
rpc-client.ts to match the AIUserTokens shape (use remainingUsagePercentage?:
number; resetsIn?: number) so the types and runtime fields align with the
AIUserTokens interface.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tool-action-mapper.ts`:
- Around line 158-168: The current BASH_TOOL_NAME branch builds cmdPreview by
concatenating possibly undefined (toolInput?.command) which coerces to the
string "undefined" and prevents the displayText fallback; update the logic in
tool-action-mapper.ts so cmdPreview is only constructed when a real command
exists (e.g., read toolInput?.command into a local variable like command, check
if command is truthy before calling substring/length), then set displayText =
toolInput?.description || cmdPreview || 'command' so the 'command' fallback is
reachable when command is missing.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts`:
- Around line 201-239: The code currently uses path.join(projectPath,
input_schema/output_schema) to read files which allows escaping the project
root; replace those joins with resolveProjectBoundPath(projectPath,
input_schema) and resolveProjectBoundPath(projectPath, output_schema), check for
undefined and return the existing { success: false, message: ..., error: ... }
immediately if undefined, then use the returned safe path for
fs.existsSync/readFileSync before passing content into generateSchemaFromContent
and updateTsFileIoTypes (refer to isFilePath, generateSchemaFromContent,
updateTsFileIoTypes, IOType.Input and IOType.Output to locate the relevant
blocks).
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts`:
- Around line 1404-1406: The schema description for the output_mode option in
file_tools.ts is inconsistent with the runtime default used by createGrepExecute
(which defaults to "content"); update the z.enum description for output_mode to
state that the default is "content" (or alternatively change createGrepExecute
to default to "files_with_matches" if you prefer that behavior) so both the
schema and runtime agree; modify the output_mode description text around the
output_mode enum and verify createGrepExecute's default value remains correct
after the change.
- Around line 1080-1088: The search path is constructed with
path.join(projectPath, searchPath) into fullSearchPath (and similarly near the
second occurrence) without validating that the result stays inside the project
or allowed Copilot roots; update the code that computes fullSearchPath to call
the existing validateFilePathSecurity (or isPathWithin) to ensure the resolved
path is within projectPath (or allowed roots) before any fs access, and return
the existing error response if validation fails so grep/glob cannot escape via
absolute or ../ inputs.
In `@workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts`:
- Around line 172-179: The rejectPendingInteractions function currently rejects
all promises but leaves stale entries in the maps; update
rejectPendingInteractions to both reject each pending (pending.reject(reason))
and then remove those entries (e.g., call this.pendingQuestions.clear() and
this.pendingApprovals.clear() or delete inside the loops) so that subsequent
lookups like respondToQuestion won't find already-rejected entries; ensure you
operate on the same maps (pendingQuestions, pendingApprovals) and that clearing
happens after all rejects.
- Around line 490-492: The current log in sendAgentMessage logs user-provided
content (request.message.substring...) at INFO level which may expose PII;
change this to either redact the content or lower the log level to DEBUG: update
the log call in sendAgentMessage to not emit raw user text (e.g., log a
sanitized summary like "<redacted>" or only message length/metadata) or move the
existing substring output to a debug-only log (use logDebug or equivalent) while
keeping the INFO log safe and non-sensitive (include request.id or other non-PII
identifiers if needed).
- Around line 83-144: The file re-declares types that already live in
`@wso2/mi-core` (ListSessionsRequest, ListSessionsResponse, SwitchSessionRequest,
SwitchSessionResponse, CreateNewSessionRequest, CreateNewSessionResponse,
DeleteSessionRequest, DeleteSessionResponse, MentionablePathType,
MentionablePathItem, SearchMentionablePathsRequest,
SearchMentionablePathsResponse); remove the local declarations and add those
names to the existing import from '@wso2/mi-core' (the current import block at
the top), and if you for some reason must keep the empty request shapes locally
convert ListSessionsRequest and CreateNewSessionRequest to type aliases instead
of empty interfaces. Ensure all references in this file use the imported types
and then delete the entire duplicate block.
🧹 Nitpick comments (6)
workspaces/mi/mi-extension/.env.example (1)
15-15: Verify naming convention and consider placement.The new
COPILOT_ROOT_URLvariable lacks theMI_prefix that most other variables in this file use (e.g.,MI_COPILOT_OPENAI_PROXY_URL,MI_COPILOT_ANTHROPIC_PROXY_URL). If this is a shared/common Copilot URL used across multiple products (consistent with the PR's goal of migrating to a "common AI proxy"), the naming is appropriate. Otherwise, consider renaming it toMI_COPILOT_ROOT_URLfor consistency.Additionally, the variable is placed at the end of the file. For better organization, consider moving it near the other Copilot-related variables (lines 3-6) to maintain logical grouping.
♻️ Suggested placement improvement
MI_AUTH_ORG= MI_AUTH_CLIENT_ID= +COPILOT_ROOT_URL=https://7eff1239-64bb-4663-b256-30a00d187a5c-prod.e1-us-east-azure.choreoapis.dev/copilot MI_COPILOT_FEEDBACK=https://e95488c8-8511-4882-967f-ec3ae2a0f86f-prod.e1-us-east-azure.choreoapis.dev/miaideployments/copilot-analytics/v1.0 MI_COPILOT_OPENAI_PROXY_URL=https://e95488c8-8511-4882-967f-ec3ae2a0f86f-prod.e1-us-east-azure.choreoapis.dev/miaideployments/ai-proxy/v1.0 MI_COPILOT_ANTHROPIC_PROXY_URL=https://e95488c8-8511-4882-967f-ec3ae2a0f86f-prod.e1-us-east-azure.choreoapis.dev/miaideployments/ai-proxy/v1.0 MI_AUTH_REDIRECT_URL=https://mi-copilot.wso2.com/vscode-auth MI_UPDATE_VERSION_CHECK_URL=https://mi-distribution.wso2.com/versions.json MI_SAMPLE_ICONS_GITHUB_URL=https://raw.githubusercontent.com/wso2/integration-studio/main/SamplesForVSCode/icons/ MI_CONNECTOR_STORE=https://apis.wso2.com/connector-store/connector-details MI_CONNECTOR_STORE_BACKEND=https://apis.wso2.com/qgpf/connector-store-backend/endpoint-9090-803/v1.0/connectors/details?limit=100&offset=0&product=MI&type=Connector&runtimeVersion=${version} MI_CONNECTOR_STORE_BACKEND_SEARCH=https://apis.wso2.com/qgpf/connector-store-backend/endpoint-9090-803/v1.0/connectors/details?limit=10&offset=0&searchQuery=${searchValue}&type=Connector&product=MI&runtimeVersion=${version} MI_CONNECTOR_STORE_BACKEND_INBOUND_ENDPOINTS=https://apis.wso2.com/qgpf/connector-store-backend/endpoint-9090-803/v1.0/connectors/details?offset=0&product=MI&type=inbound&runtimeVersion=${version} MI_CONNECTOR_STORE_BACKEND_GETBYVERSION=https://apis.wso2.com/qgpf/connector-store-backend/endpoint-9090-803/v1.0/connectors/${repoName}/versions/${versionId}?runtimeVersion=4.3.0&product=MI ADOPTIUM_API_BASE_URL=https://api.adoptium.net/v3/assets/feature_releases -COPILOT_ROOT_URL=https://7eff1239-64bb-4663-b256-30a00d187a5c-prod.e1-us-east-azure.choreoapis.dev/copilotworkspaces/mi/mi-extension/src/ai-features/agent-mode/tool-action-mapper.ts (1)
47-54: Redundant re-assignment in the'created'branch.Lines 48–50 re-assign
completedActionandfailedActionto the same values they were initialized with on lines 45–46. You can simplify by only handling the'updated'case:♻️ Suggested simplification
if (toolResult?.message) { - if (toolResult.message.includes('created')) { - completedAction = 'created'; - failedAction = 'failed to create'; - } else if (toolResult.message.includes('updated')) { + if (toolResult.message.includes('updated')) { completedAction = 'updated'; failedAction = 'failed to update'; } }workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts (3)
660-662: Unnecessary type cast —checkpointIdalready exists onUndoLastCheckpointRequest.
UndoLastCheckpointRequestintypes.ts(line 78–81) already declarescheckpointId?: string. The intersection cast(request as UndoLastCheckpointRequest & { checkpointId?: string })is redundant.Proposed fix
-const requestedCheckpointId = (request as UndoLastCheckpointRequest & { checkpointId?: string }).checkpointId; +const requestedCheckpointId = request.checkpointId;
940-966: Duplicated session-load logic when switching to the current session.Lines 942–966 (the "already on requested session" branch) duplicate the same message-loading, event-conversion, checkpoint-normalization, and usage-fetching logic found in
loadChatHistory(lines 834–886) and the mainswitchSessionpath (lines 968–1001). Consider extracting a shared helper.
462-471: File restore viacreateFile+ full-rangereplacemay silently fail on empty files.When restoring a file that should exist but is empty,
createFilewithoverwrite: truewrites a zero-byte file, thenreplaceuses a range spanning(0,0)to(MAX_SAFE_INTEGER, MAX_SAFE_INTEGER). On an empty document this range resolves to an empty range, so the replace should insert correctly. However, the two-step approach (create then replace) means a crash between them leaves a corrupted file. Consider usingworkspaceEdit.createFilewithcontents(available viavscode.workspace.fs.writeFile) as a single atomic operation, or at minimum document this ordering dependency.workspaces/mi/mi-core/src/rpc-types/agent-mode/types.ts (1)
177-235:AgentEventis a large bag-of-optional-fields — consider a discriminated union.
AgentEventcarries ~25 optional fields for very different event kinds (content streaming, tool calls, plan approvals, shell output, usage, etc.). A discriminated union ontypewould provide compile-time exhaustiveness checks and prevent impossible field combinations (e.g.,bashCommandon ausageevent). This is a larger refactor best deferred, but worth tracking.
workspaces/mi/mi-extension/src/ai-features/agent-mode/tool-action-mapper.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts (4)
681-696: Undo checkpoint captured before validation completes.
captureBeforeChangeat Line 682 runs before the file-existence check at Line 685. If the file already has content, the write is rejected but a checkpoint was already recorded. Move the capture to after all pre-write validation passes (after Line 696).Similarly, a
readFileSyncat Line 687 can throw on permission errors with no surrounding try-catch.♻️ Proposed reorder
- const fullPath = resolveFullPath(projectPath, file_path); - await undoCheckpointManager?.captureBeforeChange(file_path); - - // Check if file exists with non-empty content - const fileExists = fs.existsSync(fullPath); - if (fileExists) { - const existingContent = fs.readFileSync(fullPath, 'utf-8'); - if (existingContent.trim().length > 0) { + const fullPath = resolveFullPath(projectPath, file_path); + + // Check if file exists with non-empty content + let fileExists = false; + try { + fileExists = fs.existsSync(fullPath); + if (fileExists) { + const existingContent = fs.readFileSync(fullPath, 'utf-8'); + if (existingContent.trim().length > 0) { + console.error(`[FileWriteTool] File already exists with content: ${file_path}`); + return { + success: false, + message: `File '${file_path}' already exists with content. Use ${FILE_EDIT_TOOL_NAME} to modify it instead.`, + error: `Error: ${ErrorMessages.FILE_ALREADY_EXISTS}` + }; + } + } + } catch (error) { + logError(`[FileWriteTool] Error checking existing file: ${file_path}`, error); + return { + success: false, + message: `Cannot access file '${file_path}': ${error instanceof Error ? error.message : String(error)}`, + error: `Error: ${ErrorMessages.FILE_WRITE_FAILED}` + }; + } + + await undoCheckpointManager?.captureBeforeChange(file_path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts` around lines 681 - 696, The undo checkpoint is captured too early—move the call to undoCheckpointManager?.captureBeforeChange(file_path) so it executes only after pre-write validation (i.e., after checking existence and non-empty content) and before the actual write; wrap the fs.readFileSync(existing) usage inside a try-catch to handle permission/IO errors and return an appropriate error message instead of throwing; reference resolveFullPath, undoCheckpointManager.captureBeforeChange, FILE_EDIT_TOOL_NAME, and ErrorMessages.FILE_ALREADY_EXISTS when making these changes so the validation happens first, IO errors are caught, and checkpoints are only recorded when the write is allowed.
941-952: Same checkpoint-before-validation issue as increateWriteExecute.
captureBeforeChangeat Line 942 fires before the existence check at Line 945. If the file doesn't exist, the edit is rejected but a checkpoint was already captured. Consider moving it after the existence check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts` around lines 941 - 952, The checkpoint is being captured before validating file existence: move the call to undoCheckpointManager.captureBeforeChange(file_path) so it occurs after the existence check (after fs.existsSync(fullPath) succeeds) to avoid creating checkpoints for rejected edits; locate the call in file_tools.ts near resolveFullPath/undoCheckpointManager and adjust so captureBeforeChange runs only when the file exists (preserve the same parameters file_path and behavior for subsequent edit logic).
1099-1159: Recursive directory walk has no depth limit.
searchInDirectoryrecurses without bounding depth. A project with deeply nested symlink loops (or just very deep trees) could overflow the call stack. Consider adding amaxDepthguard.♻️ Sketch
- const searchInDirectory = (dirPath: string) => { + const MAX_SEARCH_DEPTH = 20; + const searchInDirectory = (dirPath: string, depth: number = 0) => { + if (depth > MAX_SEARCH_DEPTH) return; if (output_mode === 'content' && results.length >= head_limit) return; ... - searchInDirectory(fullPath); + searchInDirectory(fullPath, depth + 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts` around lines 1099 - 1159, searchInDirectory currently recurses unbounded and can blow the stack; modify it to accept a currentDepth parameter and enforce a maxDepth constant (e.g., maxDepth) before recursing, incrementing currentDepth when calling searchInDirectory(fullPath); additionally, when iterating entries use fs.lstatSync (or fs.promises.lstat) to detect and skip symbolic links (entry.isSymbolicLink()) to avoid symlink loops; update any initial call site to start with currentDepth = 0 and ensure the depth-check short-circuits both directory recursion and further processing when exceeded.
269-275:~check rejects any path containing a tilde, not just home-dir shorthand.Line 270 uses
normalizedPath.includes('~'), which will reject legitimate filenames likebackup~1.xml. If this is intentional, a comment clarifying the trade-off would help; otherwise, consider narrowing to paths starting with~/or~.♻️ Narrower check
- if (normalizedPath.includes('~') || (!path.isAbsolute(normalizedPath) && normalizedPath.includes('..'))) { + if (normalizedPath.startsWith('~') || (!path.isAbsolute(normalizedPath) && normalizedPath.includes('..'))) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts` around lines 269 - 275, The current security check uses normalizedPath.includes('~') which erroneously rejects filenames containing a tilde anywhere (e.g., "backup~1.xml"); change the check to only detect home-dir shorthand by testing for a leading tilde (e.g., normalizedPath === '~' or normalizedPath.startsWith('~' + path.sep) or using a regex like /^~(?:[\/\\]|$)/) and keep the existing relative-traversal check using path.isAbsolute(normalizedPath) && normalizedPath.includes('..') logic; update the conditional around normalizedPath and the returned error/comment to reflect that only leading-tilde home shorthand is disallowed.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts (4)
115-121:isFilePathheuristic could false-positive on inline content ending with.json,.xml, etc.For example, inline XML or JSON content that happens to end with a string matching
/\.(json|xml|xsd|csv)$/iwould be misclassified as a file path. This is unlikely given LLM-generated args but worth a brief comment in the code to document the heuristic's limitations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts` around lines 115 - 121, The isFilePath function's extension-based check (function isFilePath) can false-positive when inline content ends with ".json", ".xml", ".xsd", or ".csv"; add a brief comment directly above isFilePath documenting this limitation and why the heuristic is used (simple/fast for LLM args), note the potential false-positive case (inline content that happens to end with those extensions) and that this is an accepted trade-off unless stricter validation is later required.
377-388: Appending.tsto a path that already has an extension is unlikely to match a real file.Line 383 falls back to
${fullPath}.ts(e.g.,foo.dmc.ts) when the extension-replacement attempt fails. This path pattern is improbable for data mapper files. Consider removing this fallback to reduce confusion, or adding a comment explaining when this case would apply.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts` around lines 377 - 388, The fallback that appends `.ts` to a path with an existing extension (the `${fullPath}.ts` branch using appendedTsPath) is misleading and should be removed or documented; update the block in data_mapper_tools.ts where fullPath is handled (the logic using resolveProjectBoundPath and tsFilePath) to stop trying `${fullPath}.ts` when fullPath already has an extension — instead only attempt the extension-replacement (`fullPath.replace(/\.[^/.]+$/, '.ts')`) and assign tsFilePath if that resolves and exists; if you prefer to keep the appended case, add a clear comment explaining the rare scenario where `foo.dmc.ts` would be valid and ensure you only assign tsFilePath after verifying fs.existsSync(appendedTsPath).
314-323:(tool as any)casts suppress type safety.Both
createCreateDataMapperToolandcreateGenerateDataMappingTooluse(tool as any)(...), which disables compile-time validation of the schema-to-execute type alignment. This is what allows the'XSD'mismatch above to go undetected. Consider typing these properly if theaiSDK'stoolfunction supports it in the version you're using.Also applies to: 462-471
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts` around lines 314 - 323, The code is suppressing type-safety by using (tool as any)(...), so update createCreateDataMapperTool and createGenerateDataMappingTool to call tool with the correct generic/type parameters instead of casting to any: remove "(tool as any)" and supply the tool's generic type that ties inputSchema (createDataMapperInputSchema / createGenerateDataMappingInputSchema) to the corresponding execute function type (CreateDataMapperExecuteFn / CreateGenerateDataMappingExecuteFn or equivalent), or adapt the execute types to match the tool signature; make the change in both createCreateDataMapperTool and the other function at lines ~462-471 so the schema-to-execute alignment is checked by the compiler.
174-199: Undo checkpoint only captures the main.tsfile, not all files created bycreateDMFiles.
captureBeforeChangeis called only forrelativeTsPath(Line 178), butdmRpcManager.createDMFiles(Line 194) generates additional files (e.g.,dm-utils.ts, registry resources). If the user triggers undo, only the main TS file state is restored while the other scaffolding files persist.Consider capturing before-state for all files that
createDMFileswill generate, or documenting that undo for data mapper creation is partial.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts` around lines 174 - 199, The undo checkpoint only captures the single main TS file (relativeTsPath) but createDMFiles (MiDataMapperRpcManager.createDMFiles) creates additional files (e.g., dm-utils.ts and registry resources), so update the flow to capture before-state for all files that will be generated: derive the full list of target files (the main `${name}.ts` at tsFilePath, the dm-utils file in dmFolder, and any registry/resource files that createDMFiles will create) using getDataMapperFolder/dmFolder and the name, then call undoCheckpointManager.captureBeforeChange for each file's path (use path.relative(projectPath, ...)) before invoking dmRpcManager.createDMFiles; ensure you guard for undefined undoCheckpointManager as currently done.workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts (3)
612-621: Redundant type assertion —checkpointIdis already onUndoLastCheckpointRequest.
UndoLastCheckpointRequest(types.ts, Line 80) already declarescheckpointId?: string. The intersection cast on Line 614 is unnecessary.Proposed fix
- const requestedCheckpointId = (request as UndoLastCheckpointRequest & { checkpointId?: string }).checkpointId; + const requestedCheckpointId = request.checkpointId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts` around lines 612 - 621, The code uses an unnecessary intersection cast when extracting checkpointId; remove the redundant " & { checkpointId?: string }" and simply read the optional property from the request as defined by UndoLastCheckpointRequest (i.e., set requestedCheckpointId from request.checkpointId or from (request as UndoLastCheckpointRequest).checkpointId), keeping the rest of the logic that compares requestedCheckpointId to checkpoint.summary.checkpointId unchanged.
1179-1265:buildMentionablePathCacherecursively walks with no depth or item count limit.For large Maven multi-module projects, the unbounded
walkcould traverse deeply nested trees and produce a large cache. Consider adding a max-depth parameter or a cap on the total number of items to keep the cache-build fast and memory-bounded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts` around lines 1179 - 1265, buildMentionablePathCache's inner async function walk currently recurses without limits and can exhaust memory on large repos; modify buildMentionablePathCache to enforce traversal bounds by adding parameters/guards (e.g., maxDepth and maxItems) and stop conditions: update walk to accept currentDepth and return early when currentDepth >= maxDepth or a shared counter tracking total items (incrementing when addMentionable is called) reaches maxItems, and ensure addMentionable and the final mentionablePathCache assignment respect the same counter; keep function names referenced (buildMentionablePathCache, walk, addMentionable, addMentionable calls that push to mentionables, mentionablePathCache, mentionableRootPathSet) so the traversal halts deterministically and the cache remains memory-bounded.
879-964:switchSessionduplicates the load-and-normalize-events logic in three places.The same pattern —
getMessages→convertToEventFormat→getUndoCheckpointManager→applyLatestUndoAvailabilityToEvents→getLastUsage→getLatestMode— appears inloadChatHistory(Lines 804-820),switchSessionsame-session branch (Lines 898-911), andswitchSessionnew-session branch (Lines 931-944). Extracting this into a private helper would reduce duplication and bug surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts` around lines 879 - 964, switchSession duplicates the same "load + normalize events" sequence found in loadChatHistory and two branches of switchSession; extract that sequence into a single private helper (e.g., loadAndNormalizeSessionEvents) on the same class that performs: getMessages (from either getChatHistoryManager() or this.chatHistoryManager), convert with ChatHistoryManager.convertToEventFormat, fetch undo checkpoint via getUndoCheckpointManager().getLatestCheckpoint(), call applyLatestUndoAvailabilityToEvents with the checkpoint id, fetch last usage via getLastUsage, and fetch latest mode via getLatestMode; have the helper return the normalized events, lastTotalInputTokens, and mode, then replace the duplicated blocks in loadChatHistory and both switchSession branches to call this helper and assign this.currentMode from the returned mode.workspaces/mi/mi-core/src/rpc-types/agent-mode/types.ts (1)
174-235:AgentEventis a wide union-style interface — consider documenting which fields apply to which event types.
AgentEventhas 30+ optional fields for different event types (thinking, tool, plan, usage, shell). ThePlanApprovalRequestedEventsub-interface (Lines 240-250) is a good pattern for narrowing. Consider adding similar discriminated sub-interfaces or at least a field-to-event-type mapping comment for maintainability as this surface grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-core/src/rpc-types/agent-mode/types.ts` around lines 174 - 235, AgentEvent is a large catch‑all interface; split it into discriminated sub-interfaces (e.g., ThinkingEvent, ToolCallEvent, ToolResultEvent, PlanEvent, PlanApprovalRequestedEvent, UsageEvent, ShellEvent) keyed by the existing type field (AgentEventType) and export a union alias AgentEvent = ThinkingEvent | ToolCallEvent | ... so each variant declares only the fields it needs; alternatively add a clear field-to-event mapping comment next to AgentEventType and the AgentEvent declaration if refactoring into sub-interfaces is too invasive. Use the existing PlanApprovalRequestedEvent name and AgentEventType discriminant to guide naming and ensure tsconfig strict type checking benefits from narrowing via switch/case on event.type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@workspaces/mi/mi-data-mapper/src/components/DataMapper/Header/AIMapButton.tsx`:
- Line 76: The bug: when setRemainingTokenPercentage assigns the string "Not
Available" to remainingTokenPercentage, later formatting naively appends "%"
resulting in "Not Available%". Fix: change how remainingTokenPercentage is
represented and checked—use a nullable number (e.g., number | null) or keep the
sentinel string but update the formatter to check the type/value before
appending "%" (inspect remainingTokenPercentage via typeof or Number.isFinite)
in the render/formatting code that currently does
`${remainingTokenPercentage}%`; update the setter (setRemainingTokenPercentage)
usage so it sets null or NaN-safe numbers and update the display logic in
AIMapButton (referencing remainingTokenPercentage and
setRemainingTokenPercentage) to conditionally append "%" only for valid numeric
percentages, otherwise render "Not Available" (no trailing %).
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts`:
- Around line 304-312: The zod schema createDataMapperInputSchema includes 'XSD'
for input_type and output_type but the TypeScript type CreateDataMapperExecuteFn
in types.ts restricts those fields to 'JSON' | 'XML' | 'CSV', causing a mismatch
and a cast workaround; update the type definition(s) in types.ts that define the
input_type/output_type union (the CreateDataMapperExecuteFn type and any related
Input/Output format type aliases) to include 'XSD' so the TS types match the zod
schema and existing schemaBuilder handling of XSD.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts`:
- Around line 1416-1424: The schema defines a type field (grepInputSchema) but
the grep executor (createGrepExecute) never uses it; update createGrepExecute to
destructure the type property from the input and incorporate it into the ripgrep
args (e.g., add ['-t', type] or ['--type', type] when present), ensuring you
validate/escape the value as needed or map it to allowed types, or alternatively
remove the type entry from grepInputSchema if you choose not to support
file-type filtering.
- Around line 1388-1390: Fix the typo in the JSDoc/comment that currently reads
"Creates the file_edit to" to "Creates the file_edit tool"; update the comment
text where the file_edit tool is defined (the comment immediately above the
file_edit tool creation/registration function in file_tools.ts) so it accurately
describes the purpose of the file_edit tool.
- Around line 1288-1292: The glob pattern can escape the validated searchPath
via path traversal, so after computing globPattern and calling glob.sync (the
matches array), validate and filter results to ensure each resolved match stays
inside fullSearchPath: for each match produced by glob.sync, resolve/normalize
it (path.resolve / path.normalize) and then check via
path.relative(fullSearchPath, resolved) that it does not start with '..' (or
ensure resolved.startsWith(fullSearchPath + path.sep) or equals fullSearchPath)
before accepting it; reject any matches outside the boundary and log or ignore
them. This change should be applied around the code using fullSearchPath,
pattern, globPattern and matches so the post-filter enforces the project
boundary regardless of the pattern content.
In `@workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-handler.ts`:
- Around line 51-53: disposeMIAgentPanelRpcManager currently just deletes the
rpcManagerMap entry and must first abort any in-flight operations: find the
manager by projectUri in rpcManagerMap, call its abortAgentGeneration() method
to trigger the AbortController and reject pending interaction promises, then
remove the map entry; also ensure the disposeMIAgentPanelRpcManager function is
invoked from the workspace folder removal handler in extension.ts so managers
are cleaned up when folders are closed.
In `@workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts`:
- Around line 303-379: The resolveLegacyCodeSegmentPath function extracts
artifactName from XML and then uses it in path.join without sanitization,
allowing path traversal; sanitize and validate artifactName (the variable parsed
at nameMatch[2]) before any use in path.join: reject or return null if it
contains path separators, "..", or is an absolute path, or alternatively
normalize to a safe basename/whitelisted pattern (e.g., only alphanumerics,
dots, dashes, underscores); apply this check for all branches that use
artifactName (including the apis branch that also reads versionMatch) and ensure
any invalid artifactName returns null instead of constructing a path.
- Around line 494-536: The handler runAgentOnce sets this.currentAbortController
to a single controller (via createAgentAbortController), so concurrent
sendAgentMessage calls overwrite the controller and earlier runs become
unabortable; change the logic to track controllers per run (e.g., maintain a Map
or Set of AbortControllers) or guard against concurrent runs by
rejecting/awaiting when a run is in progress. Specifically, update
sendAgentMessage/runAgentOnce to push the new controller into a controllers
collection (and remove it in the finally block), or prevent a new run while
this.currentAbortController (or an inProgress flag) exists, and modify
abortAgentGeneration to abort all controllers in the collection (or clear the
flag) so abortAgentGeneration correctly aborts the intended run(s); ensure you
still clear controllers in the finally branch of runAgentOnce.
---
Duplicate comments:
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts`:
- Around line 180-196: The current compileGrepPattern has ReDoS mitigations
(length check + try/catch) but the same protections must be applied
consistently: update compileGlobPattern to enforce the same
MAX_GREP_PATTERN_LENGTH guard, use the same try/catch error handling and
return-shape ({ regex?: RegExp; error?: string }), and make the regex flag logic
symmetric to compileGrepPattern (use 'gi' when caseInsensitive, otherwise 'g');
ensure error messages consistently include the Error.message when available.
In `@workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts`:
- Around line 440-446: The log in sendAgentMessage should avoid logging PII by
not including the actual message content; keep only metadata (chatId, mode,
messageLength) in the logInfo call and remove any references to request.message
from INFO-level logging. Update the logInfo invocation in the sendAgentMessage
function (rpc-manager.ts) to only interpolate request.chatId, request.mode (or
this.currentMode/DEFAULT_AGENT_MODE) and the computed messageLength, and ensure
no other INFO logs in this function output request.message; if deeper debugging
of message content is needed, move that to a DEBUG/TRACE-level log gated by a
config flag.
- Around line 120-129: The method rejectPendingInteractions correctly rejects
and clears both pendingQuestions and pendingApprovals, but to harden it ensure
each pending.reject call is protected so one thrown error doesn’t stop the rest:
in rejectPendingInteractions wrap pending.reject(...) calls for both
this.pendingQuestions and this.pendingApprovals in try/catch (or use
Promise.resolve().catch) and still clear the maps afterward so all entries are
removed even if individual rejects throw; reference the
rejectPendingInteractions function and the this.pendingQuestions /
this.pendingApprovals maps when making the change.
---
Nitpick comments:
In `@workspaces/mi/mi-core/src/rpc-types/agent-mode/types.ts`:
- Around line 174-235: AgentEvent is a large catch‑all interface; split it into
discriminated sub-interfaces (e.g., ThinkingEvent, ToolCallEvent,
ToolResultEvent, PlanEvent, PlanApprovalRequestedEvent, UsageEvent, ShellEvent)
keyed by the existing type field (AgentEventType) and export a union alias
AgentEvent = ThinkingEvent | ToolCallEvent | ... so each variant declares only
the fields it needs; alternatively add a clear field-to-event mapping comment
next to AgentEventType and the AgentEvent declaration if refactoring into
sub-interfaces is too invasive. Use the existing PlanApprovalRequestedEvent name
and AgentEventType discriminant to guide naming and ensure tsconfig strict type
checking benefits from narrowing via switch/case on event.type.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts`:
- Around line 115-121: The isFilePath function's extension-based check (function
isFilePath) can false-positive when inline content ends with ".json", ".xml",
".xsd", or ".csv"; add a brief comment directly above isFilePath documenting
this limitation and why the heuristic is used (simple/fast for LLM args), note
the potential false-positive case (inline content that happens to end with those
extensions) and that this is an accepted trade-off unless stricter validation is
later required.
- Around line 377-388: The fallback that appends `.ts` to a path with an
existing extension (the `${fullPath}.ts` branch using appendedTsPath) is
misleading and should be removed or documented; update the block in
data_mapper_tools.ts where fullPath is handled (the logic using
resolveProjectBoundPath and tsFilePath) to stop trying `${fullPath}.ts` when
fullPath already has an extension — instead only attempt the
extension-replacement (`fullPath.replace(/\.[^/.]+$/, '.ts')`) and assign
tsFilePath if that resolves and exists; if you prefer to keep the appended case,
add a clear comment explaining the rare scenario where `foo.dmc.ts` would be
valid and ensure you only assign tsFilePath after verifying
fs.existsSync(appendedTsPath).
- Around line 314-323: The code is suppressing type-safety by using (tool as
any)(...), so update createCreateDataMapperTool and
createGenerateDataMappingTool to call tool with the correct generic/type
parameters instead of casting to any: remove "(tool as any)" and supply the
tool's generic type that ties inputSchema (createDataMapperInputSchema /
createGenerateDataMappingInputSchema) to the corresponding execute function type
(CreateDataMapperExecuteFn / CreateGenerateDataMappingExecuteFn or equivalent),
or adapt the execute types to match the tool signature; make the change in both
createCreateDataMapperTool and the other function at lines ~462-471 so the
schema-to-execute alignment is checked by the compiler.
- Around line 174-199: The undo checkpoint only captures the single main TS file
(relativeTsPath) but createDMFiles (MiDataMapperRpcManager.createDMFiles)
creates additional files (e.g., dm-utils.ts and registry resources), so update
the flow to capture before-state for all files that will be generated: derive
the full list of target files (the main `${name}.ts` at tsFilePath, the dm-utils
file in dmFolder, and any registry/resource files that createDMFiles will
create) using getDataMapperFolder/dmFolder and the name, then call
undoCheckpointManager.captureBeforeChange for each file's path (use
path.relative(projectPath, ...)) before invoking dmRpcManager.createDMFiles;
ensure you guard for undefined undoCheckpointManager as currently done.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts`:
- Around line 681-696: The undo checkpoint is captured too early—move the call
to undoCheckpointManager?.captureBeforeChange(file_path) so it executes only
after pre-write validation (i.e., after checking existence and non-empty
content) and before the actual write; wrap the fs.readFileSync(existing) usage
inside a try-catch to handle permission/IO errors and return an appropriate
error message instead of throwing; reference resolveFullPath,
undoCheckpointManager.captureBeforeChange, FILE_EDIT_TOOL_NAME, and
ErrorMessages.FILE_ALREADY_EXISTS when making these changes so the validation
happens first, IO errors are caught, and checkpoints are only recorded when the
write is allowed.
- Around line 941-952: The checkpoint is being captured before validating file
existence: move the call to undoCheckpointManager.captureBeforeChange(file_path)
so it occurs after the existence check (after fs.existsSync(fullPath) succeeds)
to avoid creating checkpoints for rejected edits; locate the call in
file_tools.ts near resolveFullPath/undoCheckpointManager and adjust so
captureBeforeChange runs only when the file exists (preserve the same parameters
file_path and behavior for subsequent edit logic).
- Around line 1099-1159: searchInDirectory currently recurses unbounded and can
blow the stack; modify it to accept a currentDepth parameter and enforce a
maxDepth constant (e.g., maxDepth) before recursing, incrementing currentDepth
when calling searchInDirectory(fullPath); additionally, when iterating entries
use fs.lstatSync (or fs.promises.lstat) to detect and skip symbolic links
(entry.isSymbolicLink()) to avoid symlink loops; update any initial call site to
start with currentDepth = 0 and ensure the depth-check short-circuits both
directory recursion and further processing when exceeded.
- Around line 269-275: The current security check uses
normalizedPath.includes('~') which erroneously rejects filenames containing a
tilde anywhere (e.g., "backup~1.xml"); change the check to only detect home-dir
shorthand by testing for a leading tilde (e.g., normalizedPath === '~' or
normalizedPath.startsWith('~' + path.sep) or using a regex like
/^~(?:[\/\\]|$)/) and keep the existing relative-traversal check using
path.isAbsolute(normalizedPath) && normalizedPath.includes('..') logic; update
the conditional around normalizedPath and the returned error/comment to reflect
that only leading-tilde home shorthand is disallowed.
In `@workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts`:
- Around line 612-621: The code uses an unnecessary intersection cast when
extracting checkpointId; remove the redundant " & { checkpointId?: string }" and
simply read the optional property from the request as defined by
UndoLastCheckpointRequest (i.e., set requestedCheckpointId from
request.checkpointId or from (request as
UndoLastCheckpointRequest).checkpointId), keeping the rest of the logic that
compares requestedCheckpointId to checkpoint.summary.checkpointId unchanged.
- Around line 1179-1265: buildMentionablePathCache's inner async function walk
currently recurses without limits and can exhaust memory on large repos; modify
buildMentionablePathCache to enforce traversal bounds by adding
parameters/guards (e.g., maxDepth and maxItems) and stop conditions: update walk
to accept currentDepth and return early when currentDepth >= maxDepth or a
shared counter tracking total items (incrementing when addMentionable is called)
reaches maxItems, and ensure addMentionable and the final mentionablePathCache
assignment respect the same counter; keep function names referenced
(buildMentionablePathCache, walk, addMentionable, addMentionable calls that push
to mentionables, mentionablePathCache, mentionableRootPathSet) so the traversal
halts deterministically and the cache remains memory-bounded.
- Around line 879-964: switchSession duplicates the same "load + normalize
events" sequence found in loadChatHistory and two branches of switchSession;
extract that sequence into a single private helper (e.g.,
loadAndNormalizeSessionEvents) on the same class that performs: getMessages
(from either getChatHistoryManager() or this.chatHistoryManager), convert with
ChatHistoryManager.convertToEventFormat, fetch undo checkpoint via
getUndoCheckpointManager().getLatestCheckpoint(), call
applyLatestUndoAvailabilityToEvents with the checkpoint id, fetch last usage via
getLastUsage, and fetch latest mode via getLatestMode; have the helper return
the normalized events, lastTotalInputTokens, and mode, then replace the
duplicated blocks in loadChatHistory and both switchSession branches to call
this helper and assign this.currentMode from the returned mode.
workspaces/mi/mi-data-mapper/src/components/DataMapper/Header/AIMapButton.tsx
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts
Outdated
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts
Show resolved
Hide resolved
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts
Show resolved
Hide resolved
xlight05
left a comment
xlight05
left a comment
There was a problem hiding this comment.
Overall LGTM. MI/BI UI unification and deviations, we'll address separately. Lets go ahead with this for alpha.
@IsuruMaduranga shall we take a review from MI side too?
Yeah. @ChinthakaJ98? Looks good? |
|
There is a build failure. Can you please check that? |
IsuruMaduranga
force-pushed
the
mi-agent-mode
branch
from
fe5ff2e to
9af142b
Compare
IsuruMaduranga
changed the title
IsuruMaduranga
force-pushed
the
mi-agent-mode
branch
2 times, most recently
from
1a7c8df to
76d5167
Compare
IsuruMaduranga
force-pushed
the
mi-agent-mode
branch
from
b8ce720 to
09afb9f
Compare
…/anthropic', '@ai-sdk/gateway', and '@ai-sdk/provider-utils'
…and improve error messaging for token issues
IsuruMaduranga
force-pushed
the
mi-agent-mode
branch
from
09afb9f to
b95dccc
Compare
4 participants
Purpose
This PR adds Claude Code/Cursor-like autonomous agentic end-to-end integration development abilities to MI Copilot. It also migrates the extension to a common SSO login through WSO2 Devant and a unified AI proxy shared between MI and BI.
Fixes:
Key Features
Autonomous Agentic Development
Advanced Developer UX
Infrastructure & Performance
Technical Changes
stopWhen: stepCountIs()).AgentUndoCheckpointManagerhandles stack-based checkpoints (max 25) and SHA-256 hashing.ask_user_questionandexit_plan_modetools that block execution for user input.validate_codetool utilizes the LemMinx XML LSP for real-time diagnostics.Screen.Recording.2026-02-16.at.17.44.43.mov