Introduce Shell tool sandboxing and connector caching#1570
Open
IsuruMaduranga wants to merge 20 commits intowso2:mainfrom
Open
Introduce Shell tool sandboxing and connector caching#1570IsuruMaduranga wants to merge 20 commits intowso2:mainfrom
IsuruMaduranga wants to merge 20 commits intowso2:mainfrom
Conversation
…r names and improve operation name management
…n creation for projects, and enhance AIChatFooter tooltip alignment
…cessing logic for connector summaries and details
…ing and error diagnostics
Contributor
📝 WalkthroughWalkthroughIntroduces shell command approval flow with session-based rule persistence, stream watchdog timeout management, structured file patching, comprehensive shell sandbox analysis with PowerShell support, connector store caching layer, and integration throughout agent execution, authentication, and frontend components. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent
participant BashTool as Bash Tool
participant Sandbox as Shell Sandbox<br/>(Analysis)
participant Store as Approval Rule<br/>Store
participant Handler as Event Handler<br/>(Approval)
participant User as User
Agent->>BashTool: Execute shell command
BashTool->>Sandbox: analyzeShellCommand()
Sandbox->>Sandbox: Parse & classify<br/>(safe/destructive/sensitive)
Sandbox-->>BashTool: ShellCommandAnalysis<br/>(requiresApproval,<br/>blocked, reasons)
alt Command Blocked
BashTool-->>Agent: buildShellSandboxBlockedResult()
else Approval Needed
BashTool->>Store: Check isAnalysisCoveredByRules()
Store-->>BashTool: Coverage result
alt Rules Cover Command
BashTool->>BashTool: Bypass approval
BashTool->>Agent: Execute command
else Rules Don't Cover
BashTool->>Handler: requestShellApproval()
Handler->>User: Emit approval request<br/>event with reasons
User-->>Handler: Approve/Deny +<br/>rememberForSession?
Handler-->>BashTool: Approval response
alt User Approved & Remember
BashTool->>Store: addRule()<br/>(suggested prefix)
Store-->>BashTool: Rule persisted
end
alt User Approved
BashTool->>Agent: Execute command
else User Denied
BashTool-->>Agent: Approval denied
end
end
else Safe Command
BashTool->>Agent: Execute command
end
sequenceDiagram
participant Agent as Agent Runtime
participant Watchdog as Stream Watchdog
participant LLM as LLM Stream
participant Tools as Tool Execution
participant Handler as Error/Timeout<br/>Handler
Agent->>Watchdog: createStreamWatchdog()<br/>(idleTimeout,<br/>totalTimeout)
loop Stream Processing
Tools->>Watchdog: markActivity()
Watchdog->>Watchdog: Reset idle timer
Tools->>LLM: Stream tokens
LLM-->>Tools: Content/Delta
Tools-->>Agent: Process token
end
alt Idle Timeout Exceeded
Watchdog->>Watchdog: Trigger idle<br/>timeout
Watchdog->>Watchdog: createStreamTimeoutError()
Watchdog->>Handler: onTimeout(error)
Handler->>Agent: Emit error event
Handler->>Watchdog: abort()
else Total Timeout Exceeded
Watchdog->>Watchdog: Trigger total<br/>timeout
Watchdog->>Handler: onTimeout(error)
Handler->>Agent: Emit error event
Handler->>Watchdog: abort()
else Stream Completes
LLM-->>Tools: Stream end
Tools->>Watchdog: cleanup()
Watchdog->>Agent: Return result with<br/>continuationSuggested?
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces two major features and several stability improvements to the MI VS Code extension's AI agent capabilities:
Purpose: Add shell command sandboxing with user approval workflows and implement granular connector store caching to improve resilience and performance.
Changes:
- Shell tool sandboxing with policy-based approval system for potentially destructive commands
- Connector store caching infrastructure with per-definition granular caching and fallback mechanisms
- Agent execution improvements including stream watchdog, timeout handling, and continuation after limits
- Authentication improvements for STS token handling
- UI improvements for time formatting (seconds instead of days) and shell approval dialogs
Reviewed changes
Copilot reviewed 42 out of 44 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shell_sandbox*.ts | New shell command analysis and sandboxing implementation (3 files) |
| file_edit_patch.ts | New structured hunk-based file editing with context/line hints |
| connector_store_cache.ts | Rewritten caching with per-definition granular cache files |
| bash_tools.ts | Integrated shell sandbox approval flow |
| agent.ts | Added stream watchdog, timeout handling, and continuation logic |
| auth.ts | Improved STS token error handling |
| utils.ts | Changed time display from days to seconds |
| AIChatFooter.tsx | Added shell approval UI with "remember for session" checkbox |
| AIChatHeader.tsx | Added formatResetTime helper for seconds-based display |
| stream_guard.ts | New stream lifecycle management with idle/total timeouts |
| connector_tools.ts | Changed to single-item lookup API pattern |
| project_tools.ts | Updated to use new connector lookup APIs |
| system.ts | Updated system prompt with new tool usage guidelines |
| synapse_guide*.ts | Documentation improvements and clarifications |
| Test files (3) | Comprehensive tests for shell sandbox and file patch |
| update-connector-context-db.js | New script for updating connector fallback data |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts (1)
428-435:⚠️ Potential issue | 🟡 MinorCheckpoint captured before
mapFunctionvalidity check.
captureBeforeChange(line 429) runs unconditionally before themapFunctionguard on line 430. If the file fails the check, the undo history contains a spurious entry for a file that was never modified.♻️ Proposed fix — move checkpoint after validation
- const content = fs.readFileSync(tsFilePath, 'utf8'); - await undoCheckpointManager?.captureBeforeChange(path.relative(projectPath, tsFilePath)); - if (!content.includes('mapFunction')) { + const content = fs.readFileSync(tsFilePath, 'utf8'); + if (!content.includes('mapFunction')) { return { success: false, message: `File at ${tsFilePath} does not appear to be a valid data mapper file (missing mapFunction).`, error: 'Error: Invalid data mapper file', }; } + await undoCheckpointManager?.captureBeforeChange(path.relative(projectPath, tsFilePath));🤖 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 428 - 435, The code currently calls undoCheckpointManager?.captureBeforeChange(path.relative(projectPath, tsFilePath)) before validating the file contains mapFunction, which creates unnecessary undo entries for invalid files; move the captureBeforeChange call so it runs only after you verify content.includes('mapFunction') (i.e., after the guard that returns the invalid-file error) to ensure checkpoints are only created for files that will be modified; update references to undoCheckpointManager.captureBeforeChange, mapFunction check, tsFilePath and projectPath accordingly.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/project_tools.ts (1)
312-318:⚠️ Potential issue | 🟡 Minor
versionmay beundefined, potentially producing an invalid pom.xml entry.
item.version?.tagNamecan beundefinedif the resolved item lacks version info (e.g., from a fallback definition). This would passundefinedas the version in the dependency, which could result in a malformed pom.xml update or a silent failure downstream.Consider validating version alongside the Maven coordinates check at Line 283, or defaulting to a known version.
Proposed validation addition
- if (typeof item?.mavenGroupId !== 'string' || typeof item?.mavenArtifactId !== 'string') { + if (typeof item?.mavenGroupId !== 'string' || typeof item?.mavenArtifactId !== 'string' || !item?.version?.tagName) { return { name: itemName, type: itemType, success: false, - error: `${itemType === 'connector' ? 'Connector' : 'Inbound endpoint'} definition is missing Maven coordinates` + error: `${itemType === 'connector' ? 'Connector' : 'Inbound endpoint'} definition is missing Maven coordinates or version` }; }🤖 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/project_tools.ts` around lines 312 - 318, The dependency object is using item.version?.tagName which can be undefined and yield an invalid pom entry; update the code before constructing dependencies (the block creating the dependencies: DependencyDetails[] and the use of item.mavenGroupId/item.mavenArtifactId) to validate that item.version?.tagName exists (or supply a safe default) and treat missing version as an error or skip the dependency: e.g., check the same place where Maven coordinates are validated (the earlier Maven check) and either throw/log and return/continue when version is missing, or set version to a known default string; ensure the dependency entry uses the validated non-undefined version value instead of item.version?.tagName.
🧹 Nitpick comments (13)
workspaces/mi/mi-extension/scripts/update-connector-context-db.js (1)
10-11: Minor: using an escaped${...}inside a template literal as a manual replacement token is confusing.
`${API_BASE}/connectors/summaries?type=\${type}&...`produces a string containing the literal text${type}, which is then replaced via.replace('${type}', ...). A plain function or a regular string with a placeholder likeTYPE_PLACEHOLDERis clearer.♻️ Cleaner alternative
-const SUMMARY_URL_TEMPLATE = `${API_BASE}/connectors/summaries?type=\${type}&limit=100&offset=0&product=MI`; +function buildSummaryUrl(type) { + return `${API_BASE}/connectors/summaries?type=${encodeURIComponent(type)}&limit=100&offset=0&product=MI`; +}Then in
fetchSummaries:- const summaryUrl = SUMMARY_URL_TEMPLATE.replace('${type}', encodeURIComponent(type)); + const summaryUrl = buildSummaryUrl(type);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/scripts/update-connector-context-db.js` around lines 10 - 11, The SUMMARY_URL_TEMPLATE currently embeds an escaped `${type}` token which yields a literal `${type}` and is later replaced, which is confusing; change SUMMARY_URL_TEMPLATE to use a clear placeholder (e.g. 'TYPE_PLACEHOLDER') or make it a function like getSummaryUrl(type) that returns `${API_BASE}/connectors/summaries?type=${type}&limit=100&offset=0&product=MI`; then update fetchSummaries to call the new function or replace the new placeholder instead of relying on the escaped `${type}` token (references: SUMMARY_URL_TEMPLATE and fetchSummaries).workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/tools.ts (1)
118-118: Consolidate the duplicate import from../../tools/types.Line 118 opens a second
importblock from the same module path already imported in Lines 94–117. MergeBashExecuteFn,ToolResult, andShellApprovalRuleStoreinto the existing import.♻️ Proposed consolidation
import { FILE_WRITE_TOOL_NAME, ... WEB_SEARCH_TOOL_NAME, WEB_FETCH_TOOL_NAME, + BashExecuteFn, + ToolResult, + ShellApprovalRuleStore, } from '../../tools/types'; -import { BashExecuteFn, ToolResult, ShellApprovalRuleStore } from '../../tools/types';🤖 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/agents/main/tools.ts` at line 118, Remove the duplicate import statement that brings in BashExecuteFn, ToolResult, and ShellApprovalRuleStore and instead add these three symbols to the existing import from '../../tools/types' (the earlier import block that already imports other types from that module); ensure there are no duplicate specifiers, keep the import list sorted/consistent with the project's style, and run the linter/TS compile to confirm the redundant import is gone.workspaces/mi/mi-extension/.env.example (1)
14-15: Optional: sort new keys to match existing alphabetical ordering.
dotenv-linterflags both new keys as out of order.MI_CONNECTOR_STORE_BACKEND_SUMMARIESshould precedeMI_COPILOT_ANTHROPIC_PROXY_URL, andMI_CONNECTOR_STORE_BACKEND_DETAILS_FILTERshould precedeMI_CONNECTOR_STORE_BACKEND_GETBYVERSION.♻️ Proposed ordering
MI_CONNECTOR_STORE_BACKEND=https://... +MI_CONNECTOR_STORE_BACKEND_DETAILS_FILTER=https://apis.wso2.com/qgpf/connector-store-backend/endpoint-9090-803/v1.0/connectors/details/filter MI_CONNECTOR_STORE_BACKEND_GETBYVERSION=https://... -MI_CONNECTOR_STORE_BACKEND_SUMMARIES=https://... -MI_CONNECTOR_STORE_BACKEND_DETAILS_FILTER=https://... +MI_CONNECTOR_STORE_BACKEND_SUMMARIES=https://apis.wso2.com/qgpf/connector-store-backend/endpoint-9090-803/v1.0/connectors/summaries?type=${type}&limit=100&offset=0&product=MI🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/.env.example` around lines 14 - 15, Reorder the two new environment keys so they follow the file's alphabetical ordering: move MI_CONNECTOR_STORE_BACKEND_SUMMARIES to appear before MI_COPILOT_ANTHROPIC_PROXY_URL, and move MI_CONNECTOR_STORE_BACKEND_DETAILS_FILTER to appear before MI_CONNECTOR_STORE_BACKEND_GETBYVERSION; update .env.example accordingly and re-run dotenv-linter to confirm no ordering warnings remain.workspaces/mi/mi-extension/src/test/suite/file-edit.patch.test.ts (1)
22-163: Solid test coverage for the new patching module.Good mix of happy-path and error-path tests. Two additional edge cases worth covering:
- Deletion hunk:
{ old_text: 'line_to_delete', new_text: '' }— verifiessplitReplacementLinesreturns[]and lines are removed.- Equidistant
line_hint: Two matches equally close to the hint — should this resolve or returnHUNK_AMBIGUOUS?These can be added as follow-ups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/test/suite/file-edit.patch.test.ts` around lines 22 - 163, Add two new tests to the File Edit Patch Tests suite to cover (1) a deletion hunk and (2) the equidistant line_hint edge case: create a test calling applyStructuredFilePatch with a hunk where old_text is 'line_to_delete' and new_text is '' and assert success=true, newContent no longer contains that line and splitReplacementLines behavior (empty array) is respected; and create a test where the content has two identical matches equidistant from a provided line_hint, call applyStructuredFilePatch with that line_hint and assert the expected behavior (either success with deterministic selection or success=false with code 'HUNK_AMBIGUOUS' per product decision). Reference applyStructuredFilePatch and splitReplacementLines when adding assertions so reviewers can validate the intended behavior and expected result code.workspaces/mi/mi-extension/src/ai-features/aiMachine.ts (1)
196-207: BothCOMPLETE_AUTHandSIGN_IN_SUCCESSinUnauthenticatedare identical — consider whether one event suffices.Both transitions do the same thing: move to
Authenticatedand clearerrorMessage. The semantic distinction (explicit auth vs. silent bootstrap) is the only difference. This is fine for traceability, but if maintaining two identical transitions becomes a burden, they could be consolidated.🤖 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/aiMachine.ts` around lines 196 - 207, In the Unauthenticated state both AI_EVENT_TYPE.COMPLETE_AUTH and AI_EVENT_TYPE.SIGN_IN_SUCCESS transition to 'Authenticated' and clear errorMessage; consolidate them by handling a single event (e.g., map SIGN_IN_SUCCESS to COMPLETE_AUTH or collapse both into one shared transition handler) or extract the shared transition into a named transition/action used by both to avoid duplication; update the transition definitions around the Unauthenticated state's handlers (AI_EVENT_TYPE.COMPLETE_AUTH, AI_EVENT_TYPE.SIGN_IN_SUCCESS) and ensure traceability by optionally adding a short comment indicating why events were consolidated or kept distinct.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_edit_patch.ts (1)
145-163: Validation:old_text === new_textcheck uses raw string comparison, but matching uses normalized lines.If
old_textandnew_textdiffer only in trailing whitespace (e.g.,"foo "vs"foo"), this validation passes (they're not string-equal), but after normalization the old_text match will succeed and the replacement will effectively only change trailing whitespace. This is an acceptable edge case — it's a valid edit — but worth noting that the "identical" check is less strict than the matching.🤖 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_edit_patch.ts` around lines 145 - 163, The validateHunk function currently compares FileEditHunk.old_text and .new_text with a raw string equality which can miss cases where only normalization (trimming/trailing whitespace normalization or line-ending normalization) makes them identical; update validateHunk to normalize both old_text and new_text using the same normalization logic used when matching/applying patches (the same routine that splits/normalizes lines for matching) before performing the equality check so that identical content after normalization is treated as identical; keep the existing error shape (PatchApplyFailure) and the hunkIndex reference but use normalizedOld/new variables when comparing in validateHunk.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/plan_mode_tools.ts (1)
263-276: No timeout on pending plan approvals — unlikePendingQuestionwhich hasASK_USER_TIMEOUT_MS.
requestPlanApprovalcreates a promise that stays pending indefinitely if the user never responds (e.g., closes the panel).PendingQuestionhas a 5-minute timeout + cleanup viacleanupPendingQuestionsForSession, butPendingPlanApprovalhas neither. If the session is abandoned, the promise leaks along with the entry inpendingApprovals.Consider adding a timeout or session-cleanup hook for pending approvals, similar to
cleanupPendingQuestionsForSession.🤖 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/plan_mode_tools.ts` around lines 263 - 276, The pending plan-approval promise created in requestPlanApproval (and stored in pendingApprovals / PendingPlanApproval) has no timeout or session cleanup and can leak; add a timeout similar to PendingQuestion using ASK_USER_TIMEOUT_MS: when creating the pending approval, start a timer that rejects the promise with a timeout Error and deletes the entry from pendingApprovals, and store/clear that timer so resolve/reject callbacks clear it; also ensure cleanupPendingQuestionsForSession (or an analogous cleanup function) removes any pendingApprovals for an abandoned session to avoid leaks.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts (1)
409-417: RedundantresolveProjectBoundPathcall.By the time line 409 is reached,
tsFilePathalready holds the result ofresolveProjectBoundPathfrom one of the earlier branches (lines 392–406). The second call is a no-op in the success path and only adds overhead. Consider removing it and keeping only theundefinedguard from lines 393–398.🤖 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 409 - 417, The second call to resolveProjectBoundPath is redundant because tsFilePath already holds its result in the earlier branch; remove the duplicate call and simply keep the undefined guard/early-return that checks tsFilePath (and returns the error object referencing dm_config_path), i.e. delete the later resolveProjectBoundPath invocation and ensure the code uses the existing tsFilePath variable and its null/undefined check to preserve the same early-return behavior; references: resolveProjectBoundPath, tsFilePath, dm_config_path.workspaces/mi/mi-visualizer/src/views/AIPanel/component/MICopilotContext.tsx (1)
40-45: Import and usePlanApprovalKindfrom@wso2/mi-coreinstead of duplicating the union locally.The
approvalKindfield redefines a union type that is already available asPlanApprovalKindin@wso2/mi-core. Maintaining a parallel inline union means any future changes to the core type must be manually mirrored here, risking silent mismatches between backend and UI.♻️ Proposed refactor
-import { FileObject, ImageObject, TodoItem, Question } from "@wso2/mi-core"; +import { FileObject, ImageObject, TodoItem, Question, PlanApprovalKind } from "@wso2/mi-core";export interface PendingPlanApproval { approvalId: string; - approvalKind?: 'enter_plan_mode' | 'exit_plan_mode' | 'exit_plan_mode_without_plan' | 'web_search' | 'web_fetch' | 'shell_command' | 'continue_after_limit'; + approvalKind?: PlanApprovalKind; approvalTitle?: string; approveLabel?: string; rejectLabel?: string; allowFeedback?: boolean; suggestedPrefixRule?: string[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-visualizer/src/views/AIPanel/component/MICopilotContext.tsx` around lines 40 - 45, Replace the inline union type used for the approvalKind prop in MICopilotContext with the canonical PlanApprovalKind from `@wso2/mi-core`: import PlanApprovalKind and change the approvalKind?: 'enter_plan_mode' | ... declaration to approvalKind?: PlanApprovalKind, updating any references/usages in MICopilotContext to the imported type so the UI stays in sync with the core type and avoids duplication.workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts (1)
4926-4928: Return a consistentDevantMetadatashape in the API-unavailable branch.This branch omits
hasLocalChangeswhile other branches include it. Returning an explicitfalsekeeps downstream handling predictable.Suggested patch
- if (!platformExtAPI) { - return { hasComponent: hasContextYaml, isLoggedIn: false }; - } + if (!platformExtAPI) { + return { hasComponent: hasContextYaml, isLoggedIn: false, hasLocalChanges: false }; + }🤖 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/mi-diagram/rpc-manager.ts` around lines 4926 - 4928, The API-unavailable branch in rpc-manager.ts returns { hasComponent: hasContextYaml, isLoggedIn: false } but omits hasLocalChanges; update that return to include hasLocalChanges: false so the DevantMetadata shape is consistent (modify the branch that checks platformExtAPI and returns when falsy to return { hasComponent: hasContextYaml, isLoggedIn: false, hasLocalChanges: false }).workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/shell_sandbox_powershell_ast.ts (1)
71-73: Cold-start latency consideration for PowerShell parser.
powershell.execold start on Windows can take 500ms–2s. On the first shell command of a session, this could make the 1500ms timeout tight. If timeouts are frequent in practice, consider bumpingPARSER_TIMEOUT_MSslightly or caching the "known working binary" after first successful parse to skip thepowershell.exe→pwshfallback chain on subsequent calls.🤖 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/shell_sandbox_powershell_ast.ts` around lines 71 - 73, The current PARSER_TIMEOUT_MS (1500) is likely too tight given powershell.exe cold-starts; increase PARSER_TIMEOUT_MS to a safer value (e.g., 3000) or implement caching of the confirmed working binary to avoid repeatedly trying the fallback chain in PARSER_BINARIES ('powershell.exe', 'pwsh') — update the parsing logic that iterates PARSER_BINARIES to store the first successful binary and reuse it for subsequent parses to reduce cold-start latency.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/project_tools.ts (1)
257-268: Consider using a proper type instead ofanyforresolvedItem.
resolvedItem: any | nullloses type safety. IfConnectorDefinitionLookupResult.definitionsByNamehas a typed value, propagating that type here would catch shape mismatches at compile time.🤖 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/project_tools.ts` around lines 257 - 268, The parameter resolvedItem in processItem is declared as any|null; replace it with the concrete type from ConnectorDefinitionLookupResult.definitionsByName (or a small union/alias that matches that lookup's value, e.g. ConnectorDefinition or ConnectorDefinitionLookupResult['definitionsByName'][keyof ConnectorDefinitionLookupResult['definitionsByName']] | null) so callers get compile-time shape checks; update the processItem signature and any call sites/imports, and adjust any internal handling/optional checks in processItem to match the new typed shape rather than using any.workspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsx (1)
1003-1008: Only send session-rule fields on approved shell-command responses.Line 1003 and Line 1006 currently populate
rememberForSession/suggestedPrefixRuleeven whenapprovedis false. Guarding these fields withapprovedavoids ambiguous backend semantics.🔧 Proposed fix
- rememberForSession: pendingPlanApproval.approvalKind === 'shell_command' + rememberForSession: pendingPlanApproval.approvalKind === 'shell_command' && approved ? rememberShellApprovalForSession : undefined, - suggestedPrefixRule: pendingPlanApproval.approvalKind === 'shell_command' && rememberShellApprovalForSession + suggestedPrefixRule: pendingPlanApproval.approvalKind === 'shell_command' && approved && rememberShellApprovalForSession ? pendingPlanApproval.suggestedPrefixRule : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsx` around lines 1003 - 1008, The code currently sets pendingPlanApproval session fields regardless of approval state; update the logic in AIChatFooter (the block that assigns rememberForSession and suggestedPrefixRule from pendingPlanApproval) so both fields are only populated when pendingPlanApproval.approved is true and pendingPlanApproval.approvalKind === 'shell_command'; in particular, set rememberForSession to rememberShellApprovalForSession only when approved && approvalKind === 'shell_command', and set suggestedPrefixRule only when approved && approvalKind === 'shell_command' && rememberShellApprovalForSession, using the same property names (pendingPlanApproval.suggestedPrefixRule) to locate the change.
🤖 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-extension/scripts/update-connector-context-db.js`:
- Around line 274-301: The current readExistingRecordsByName function is brittle
because it slices a TypeScript array literal and passes it to JSON.parse (the
JSON.parse call on the substring), which will throw on trailing commas, single
quotes or comments; replace that JSON.parse usage with a safer parser/evaluator
(e.g., JSON5.parse or evaluating the array literal in a sandboxed VM) so
TypeScript/JS syntax is accepted, and wrap the parse/eval in a try/catch to fall
back to returning an empty Map instead of crashing; update references in
readExistingRecordsByName and preserve the existing mapping logic that uses
getConnectorName to populate recordsByName.
- Around line 303-314: The writeTsArrayFile function currently overwrites
everything after the export declaration because exportRegex only matches up to
"export const <exportName> =" and the code writes match[0] + JSON without
preserving the trailing semicolon or any following content; update
writeTsArrayFile to locate the end of the array literal in the existing file
(find the closing ] for the exported array, include any trailing
whitespace/comments and the existing semicolon if present), then write back the
prefix up through "export const <exportName> =" + the new
JSON.stringify(records, null, 4) + a semicolon and the original suffix
(everything after the closing ]), ensuring the regex/exportRegex and match usage
in writeTsArrayFile are adjusted to capture or compute the suffix to preserve
subsequent declarations.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/agent.ts`:
- Around line 769-776: The code tries to read .messages from a possibly
undefined result of awaitWithTimeout, causing a null dereference; update each
occurrence (the block using awaitWithTimeout, finalResponseWaitTimeoutMs and
assigning finalModelMessages) to first await into a local const (e.g.,
finalResponse) and then safely access messages using optional chaining or a
fallback (e.g., finalResponse?.messages ?? []), so finalModelMessages is always
an array; apply the same safe-access pattern to the other two identical blocks
to avoid NPEs and keep the existing logError catch behavior.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts`:
- Around line 88-89: Remove or guard the hardcoded development notice string
("YOU ARE IN DEVELOPMENT PHASE...") in the prompt assembly so it is never sent
to production users; instead gate this message behind a debug/dev flag (e.g., a
config like isDevMode or process.env.DEBUG) or annotate it with a clear TODO to
delete before release. Specifically, update prompt.ts to stop injecting the
development string into the assembled prompt and ensure the variable
system_remainder is never exposed to end users (align with the non-disclosure
rule implemented in system.ts and its policy), and add a short unit/integration
check to confirm dev-only text is omitted unless debug mode is explicitly
enabled.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts`:
- Around line 238-246: SYSTEM_PROMPT_OLD currently substitutes SYNAPSE_GUIDE
with SYNAPSE_GUIDE_OLD but reuses CONNECTOR_DOCUMENTATION which documents
"Revamped response handling" (responseVariable, overwriteBody) not available for
older MI runtimes; fix by adding a version-appropriate connector doc (e.g.,
CONNECTOR_DOCUMENTATION_OLD) that omits the revamped response handling and use
it when building SYSTEM_PROMPT_OLD, or update the SYSTEM_PROMPT generation to
conditionally include the revamped section only when the runtime version >=
4.4.0 (check runtime version before choosing CONNECTOR_DOCUMENTATION vs
CONNECTOR_DOCUMENTATION_OLD); update references to SYSTEM_PROMPT_OLD,
SYNAPSE_GUIDE, SYNAPSE_GUIDE_OLD, and CONNECTOR_DOCUMENTATION to reflect the
chosen approach.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_expression_examples.ts`:
- Around line 92-97: In the FreeMarker JSON example inside
synapse_expression_examples.ts locate the JSON payload string that starts with
"name": "${payload.customer_name}" and add a comma after that field so the
object becomes valid JSON; ensure the resulting snippet reads "name":
"${payload.customer_name}", followed by the existing "customer_id" and other
properties (update only the punctuation in the example string).
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide.ts`:
- Around line 445-446: Update the guide text to use "artifact.xml" consistently
instead of the mixed "artifacts.xml"/"artifact.xml" wording: replace occurrences
of the string "artifacts.xml" with "artifact.xml" (e.g., change "If an
artifacts.xml doesn't exist" to "If an artifact.xml doesn't exist") and ensure
the sentence referencing the data mapper tool (${CREATE_DATA_MAPPER_TOOL_NAME})
still reads correctly; keep the registry descriptor format example unchanged but
confirm it uses "artifact.xml" throughout so the agent won't generate the wrong
file name.
- Around line 216-225: The guidance in synapse_guide.ts currently contradicts
the expression guide by unconditionally stating to "ALWAYS" use JSON paths on
${payload} and to "NEVER" use XML/xpath(), which conflicts with examples using
$body/xpath; update this block to: state that ${payload} is JSON after SOAP call
and prefer JSON paths (e.g., payload.NumberToWordsResponse.NumberToWordsResult)
when using ${payload}, but allow xpath() or $body access only when the raw XML
body is explicitly materialized and documented by the expression engine; remove
absolute terms ("ALWAYS"/"NEVER"/"AVOID") and add a short note referencing the
expression guide's $body/xpath usage so the two sources are aligned and
unambiguous.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_store_cache.ts`:
- Around line 122-127: The code is using runtimeVersion directly in path
segments which allows path-segment injection; update buildItemDirectory and
buildCatalogFilePath to sanitize/validate runtimeVersion before using it (e.g.,
reject or canonicalize values containing path separators or traversal patterns,
or restrict to a safe regex like /^[A-Za-z0-9._-]+$/), use the sanitized value
when joining with CACHE_ROOT_DIR and CATALOG_FILE_NAME, and ensure any callers
(places referenced around pom.xml usage) pass or check the sanitized value as
well; reference functions: buildItemDirectory, buildCatalogFilePath, and
constants: CACHE_ROOT_DIR, CATALOG_FILE_NAME.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/shell_sandbox_powershell.ts`:
- Around line 707-710: The code computes networkDetected using
POWERSHELL_NETWORK_COMMANDS and detectNetworkFromDotNet (based on commandNames
and trimmedCommand) but only bypasses the read-only allowlist check without
marking the action as requiring approval; update the approval logic so that when
networkDetected is true you set requiresApproval = true and append a clear
approvalReason (e.g., "network access detected via command/dotnet call") so
commands like curl/Invoke-WebRequest cannot slip through, and ensure this change
is applied wherever requiresApproval and approvalReason are set in the approval
flow handling (reference variables networkDetected, POWERSHELL_NETWORK_COMMANDS,
detectNetworkFromDotNet, requiresApproval, approvalReason).
- Around line 60-79: POWERSHELL_SAFE_READ_COMMANDS currently includes 'git',
which incorrectly treats all git invocations as read-only; remove 'git' from
POWERSHELL_SAFE_READ_COMMANDS and instead add explicit handling where commands
are analyzed: when the parsed command is 'git' inspect the first
subcommand/argument and only allow a curated whitelist of safe git subcommands
(e.g., status, rev-parse, log, show, diff, ls-files, branch -r, remote -v, tag
-l) to be treated as read-only; any other git subcommand (commit, reset, clean,
checkout, merge, rebase, etc.) must require approval or be routed to the
mutating branch. Update the logic that checks commands (the code that currently
consults POWERSHELL_SAFE_READ_COMMANDS) to call this git-specific validator so
git is not universally trusted.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/shell_sandbox.ts`:
- Around line 97-127: The SAFE_READ_COMMANDS entry for "find" is unsafe because
it can perform mutations via flags like -exec, -execdir, or -delete; remove
"find" from the SAFE_READ_COMMANDS Set so any use of find requires approval, or
alternatively implement a helper (e.g., isFindMutation(tokens)) that inspects
the tokenized arguments for dangerous flags and include that check in
analyzeSegment's isMutation computation (refer to analyzeSegment, tokens, and
isMutation) so find remains allowed only when its arguments are confirmed
read-only.
In `@workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts`:
- Around line 230-249: The addShellApprovalRule method currently allows errors
from persistShellApprovalRulesForSession to bubble up; wrap the await
this.persistShellApprovalRulesForSession(this.currentSessionId) call in a
try/catch, catch any error, log it via logInfo or a dedicated logger with
context (including this.currentSessionId and normalizedRule), and do not rethrow
so the in-memory this.shellApprovalRules remains and the agent flow/shell tool
callback is not disrupted; keep the existing push to this.shellApprovalRules
before persistence so the method still succeeds even if persistence fails.
In `@workspaces/mi/mi-extension/src/rpc-managers/ai-features/rpc-manager.ts`:
- Around line 666-691: The code reads currentState once via
StateMachineAI.state() and then may call
StateMachineAI.sendEvent(AI_EVENT_TYPE.*) later, which can race with other
callers; to fix, avoid using the stale local currentState: instead re-check
StateMachineAI.state() immediately before each send (for the UPDATE_USAGE,
USAGE_EXCEEDED and USAGE_RESET branches) and only call StateMachineAI.sendEvent
when the freshly read state still matches the expected state, or alternatively
send the event unconditionally but rely on the StateMachineAI guards/guards in
the state machine to ignore invalid transitions; update the logic around
StateMachineAI.state(), StateMachineAI.sendEvent, and the AI_EVENT_TYPE.* usages
accordingly so state decisions are based on a fresh read or delegated to the
state machine itself.
---
Outside diff comments:
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts`:
- Around line 428-435: The code currently calls
undoCheckpointManager?.captureBeforeChange(path.relative(projectPath,
tsFilePath)) before validating the file contains mapFunction, which creates
unnecessary undo entries for invalid files; move the captureBeforeChange call so
it runs only after you verify content.includes('mapFunction') (i.e., after the
guard that returns the invalid-file error) to ensure checkpoints are only
created for files that will be modified; update references to
undoCheckpointManager.captureBeforeChange, mapFunction check, tsFilePath and
projectPath accordingly.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/project_tools.ts`:
- Around line 312-318: The dependency object is using item.version?.tagName
which can be undefined and yield an invalid pom entry; update the code before
constructing dependencies (the block creating the dependencies:
DependencyDetails[] and the use of item.mavenGroupId/item.mavenArtifactId) to
validate that item.version?.tagName exists (or supply a safe default) and treat
missing version as an error or skip the dependency: e.g., check the same place
where Maven coordinates are validated (the earlier Maven check) and either
throw/log and return/continue when version is missing, or set version to a known
default string; ensure the dependency entry uses the validated non-undefined
version value instead of item.version?.tagName.
---
Nitpick comments:
In `@workspaces/mi/mi-extension/.env.example`:
- Around line 14-15: Reorder the two new environment keys so they follow the
file's alphabetical ordering: move MI_CONNECTOR_STORE_BACKEND_SUMMARIES to
appear before MI_COPILOT_ANTHROPIC_PROXY_URL, and move
MI_CONNECTOR_STORE_BACKEND_DETAILS_FILTER to appear before
MI_CONNECTOR_STORE_BACKEND_GETBYVERSION; update .env.example accordingly and
re-run dotenv-linter to confirm no ordering warnings remain.
In `@workspaces/mi/mi-extension/scripts/update-connector-context-db.js`:
- Around line 10-11: The SUMMARY_URL_TEMPLATE currently embeds an escaped
`${type}` token which yields a literal `${type}` and is later replaced, which is
confusing; change SUMMARY_URL_TEMPLATE to use a clear placeholder (e.g.
'TYPE_PLACEHOLDER') or make it a function like getSummaryUrl(type) that returns
`${API_BASE}/connectors/summaries?type=${type}&limit=100&offset=0&product=MI`;
then update fetchSummaries to call the new function or replace the new
placeholder instead of relying on the escaped `${type}` token (references:
SUMMARY_URL_TEMPLATE and fetchSummaries).
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/tools.ts`:
- Line 118: Remove the duplicate import statement that brings in BashExecuteFn,
ToolResult, and ShellApprovalRuleStore and instead add these three symbols to
the existing import from '../../tools/types' (the earlier import block that
already imports other types from that module); ensure there are no duplicate
specifiers, keep the import list sorted/consistent with the project's style, and
run the linter/TS compile to confirm the redundant import is gone.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.ts`:
- Around line 409-417: The second call to resolveProjectBoundPath is redundant
because tsFilePath already holds its result in the earlier branch; remove the
duplicate call and simply keep the undefined guard/early-return that checks
tsFilePath (and returns the error object referencing dm_config_path), i.e.
delete the later resolveProjectBoundPath invocation and ensure the code uses the
existing tsFilePath variable and its null/undefined check to preserve the same
early-return behavior; references: resolveProjectBoundPath, tsFilePath,
dm_config_path.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_edit_patch.ts`:
- Around line 145-163: The validateHunk function currently compares
FileEditHunk.old_text and .new_text with a raw string equality which can miss
cases where only normalization (trimming/trailing whitespace normalization or
line-ending normalization) makes them identical; update validateHunk to
normalize both old_text and new_text using the same normalization logic used
when matching/applying patches (the same routine that splits/normalizes lines
for matching) before performing the equality check so that identical content
after normalization is treated as identical; keep the existing error shape
(PatchApplyFailure) and the hunkIndex reference but use normalizedOld/new
variables when comparing in validateHunk.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/plan_mode_tools.ts`:
- Around line 263-276: The pending plan-approval promise created in
requestPlanApproval (and stored in pendingApprovals / PendingPlanApproval) has
no timeout or session cleanup and can leak; add a timeout similar to
PendingQuestion using ASK_USER_TIMEOUT_MS: when creating the pending approval,
start a timer that rejects the promise with a timeout Error and deletes the
entry from pendingApprovals, and store/clear that timer so resolve/reject
callbacks clear it; also ensure cleanupPendingQuestionsForSession (or an
analogous cleanup function) removes any pendingApprovals for an abandoned
session to avoid leaks.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/project_tools.ts`:
- Around line 257-268: The parameter resolvedItem in processItem is declared as
any|null; replace it with the concrete type from
ConnectorDefinitionLookupResult.definitionsByName (or a small union/alias that
matches that lookup's value, e.g. ConnectorDefinition or
ConnectorDefinitionLookupResult['definitionsByName'][keyof
ConnectorDefinitionLookupResult['definitionsByName']] | null) so callers get
compile-time shape checks; update the processItem signature and any call
sites/imports, and adjust any internal handling/optional checks in processItem
to match the new typed shape rather than using any.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/shell_sandbox_powershell_ast.ts`:
- Around line 71-73: The current PARSER_TIMEOUT_MS (1500) is likely too tight
given powershell.exe cold-starts; increase PARSER_TIMEOUT_MS to a safer value
(e.g., 3000) or implement caching of the confirmed working binary to avoid
repeatedly trying the fallback chain in PARSER_BINARIES ('powershell.exe',
'pwsh') — update the parsing logic that iterates PARSER_BINARIES to store the
first successful binary and reuse it for subsequent parses to reduce cold-start
latency.
In `@workspaces/mi/mi-extension/src/ai-features/aiMachine.ts`:
- Around line 196-207: In the Unauthenticated state both
AI_EVENT_TYPE.COMPLETE_AUTH and AI_EVENT_TYPE.SIGN_IN_SUCCESS transition to
'Authenticated' and clear errorMessage; consolidate them by handling a single
event (e.g., map SIGN_IN_SUCCESS to COMPLETE_AUTH or collapse both into one
shared transition handler) or extract the shared transition into a named
transition/action used by both to avoid duplication; update the transition
definitions around the Unauthenticated state's handlers
(AI_EVENT_TYPE.COMPLETE_AUTH, AI_EVENT_TYPE.SIGN_IN_SUCCESS) and ensure
traceability by optionally adding a short comment indicating why events were
consolidated or kept distinct.
In `@workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts`:
- Around line 4926-4928: The API-unavailable branch in rpc-manager.ts returns {
hasComponent: hasContextYaml, isLoggedIn: false } but omits hasLocalChanges;
update that return to include hasLocalChanges: false so the DevantMetadata shape
is consistent (modify the branch that checks platformExtAPI and returns when
falsy to return { hasComponent: hasContextYaml, isLoggedIn: false,
hasLocalChanges: false }).
In `@workspaces/mi/mi-extension/src/test/suite/file-edit.patch.test.ts`:
- Around line 22-163: Add two new tests to the File Edit Patch Tests suite to
cover (1) a deletion hunk and (2) the equidistant line_hint edge case: create a
test calling applyStructuredFilePatch with a hunk where old_text is
'line_to_delete' and new_text is '' and assert success=true, newContent no
longer contains that line and splitReplacementLines behavior (empty array) is
respected; and create a test where the content has two identical matches
equidistant from a provided line_hint, call applyStructuredFilePatch with that
line_hint and assert the expected behavior (either success with deterministic
selection or success=false with code 'HUNK_AMBIGUOUS' per product decision).
Reference applyStructuredFilePatch and splitReplacementLines when adding
assertions so reviewers can validate the intended behavior and expected result
code.
In `@workspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsx`:
- Around line 1003-1008: The code currently sets pendingPlanApproval session
fields regardless of approval state; update the logic in AIChatFooter (the block
that assigns rememberForSession and suggestedPrefixRule from
pendingPlanApproval) so both fields are only populated when
pendingPlanApproval.approved is true and pendingPlanApproval.approvalKind ===
'shell_command'; in particular, set rememberForSession to
rememberShellApprovalForSession only when approved && approvalKind ===
'shell_command', and set suggestedPrefixRule only when approved && approvalKind
=== 'shell_command' && rememberShellApprovalForSession, using the same property
names (pendingPlanApproval.suggestedPrefixRule) to locate the change.
In
`@workspaces/mi/mi-visualizer/src/views/AIPanel/component/MICopilotContext.tsx`:
- Around line 40-45: Replace the inline union type used for the approvalKind
prop in MICopilotContext with the canonical PlanApprovalKind from `@wso2/mi-core`:
import PlanApprovalKind and change the approvalKind?: 'enter_plan_mode' | ...
declaration to approvalKind?: PlanApprovalKind, updating any references/usages
in MICopilotContext to the imported type so the UI stays in sync with the core
type and avoids duplication.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (44)
workspaces/mi/mi-core/src/rpc-types/agent-mode/types.tsworkspaces/mi/mi-extension/.env.exampleworkspaces/mi/mi-extension/scripts/update-connector-context-db.jsworkspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/agent.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/tools.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/chat-history-manager.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/connector_db.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/connectors_guide.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/inbound_db.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_expression_examples.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_expression_guide.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide_old.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/stream_guard.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tool-action-mapper.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/bash_tools.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_store_cache.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/connector_tools.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/data_mapper_tools.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_edit_patch.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/plan_mode_tools.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/project_tools.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/shell_sandbox.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/shell_sandbox_powershell.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/shell_sandbox_powershell_ast.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/subagent_tool.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/types.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.tsworkspaces/mi/mi-extension/src/ai-features/aiMachine.tsworkspaces/mi/mi-extension/src/ai-features/auth.tsworkspaces/mi/mi-extension/src/ai-features/connection.tsworkspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.tsworkspaces/mi/mi-extension/src/rpc-managers/ai-features/rpc-manager.tsworkspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.tsworkspaces/mi/mi-extension/src/test/suite/file-edit.patch.test.tsworkspaces/mi/mi-extension/src/test/suite/shell-sandbox.powershell.test.tsworkspaces/mi/mi-extension/src/test/suite/shell-sandbox.test.tsworkspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatHeader.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/MICopilotContext.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/utils.ts
Comment on lines
+274
to
+301
| async function readExistingRecordsByName(filePath, exportName) { | ||
| const existing = await fs.readFile(filePath, 'utf8'); | ||
| const exportIndex = existing.indexOf(`export const ${exportName} =`); | ||
| if (exportIndex < 0) { | ||
| throw new Error(`Could not find export declaration for ${exportName} in ${filePath}.`); | ||
| } | ||
|
|
||
| const arrayStart = existing.indexOf('[', exportIndex); | ||
| const arrayEnd = existing.lastIndexOf(']'); | ||
| if (arrayStart < 0 || arrayEnd < 0 || arrayEnd < arrayStart) { | ||
| throw new Error(`Could not parse array contents from ${filePath}.`); | ||
| } | ||
|
|
||
| const parsed = JSON.parse(existing.slice(arrayStart, arrayEnd + 1)); | ||
| if (!Array.isArray(parsed)) { | ||
| throw new Error(`Parsed existing data from ${filePath} is not an array.`); | ||
| } | ||
|
|
||
| const recordsByName = new Map(); | ||
| for (const record of parsed) { | ||
| const name = getConnectorName(record); | ||
| if (name) { | ||
| recordsByName.set(name, record); | ||
| } | ||
| } | ||
|
|
||
| return recordsByName; | ||
| } |
Contributor
There was a problem hiding this comment.
Fragile fallback: JSON.parse on TypeScript source will fail for common TS array syntax.
readExistingRecordsByName extracts a substring from a .ts file and passes it to JSON.parse (Line 287). TypeScript/JavaScript array literals routinely contain trailing commas, single-quoted strings, or comments — none of which are valid JSON. Any such pattern in connector_db.ts or inbound_db.ts will throw, crashing the entire fallback path and leaving the file unupdated instead of keeping the existing records.
Consider using JSON5, vm.runInNewContext, or a targeted regex/AST parse rather than raw JSON.parse on TS source.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/mi/mi-extension/scripts/update-connector-context-db.js` around
lines 274 - 301, The current readExistingRecordsByName function is brittle
because it slices a TypeScript array literal and passes it to JSON.parse (the
JSON.parse call on the substring), which will throw on trailing commas, single
quotes or comments; replace that JSON.parse usage with a safer parser/evaluator
(e.g., JSON5.parse or evaluating the array literal in a sandboxed VM) so
TypeScript/JS syntax is accepted, and wrap the parse/eval in a try/catch to fall
back to returning an empty Map instead of crashing; update references in
readExistingRecordsByName and preserve the existing mapping logic that uses
getConnectorName to populate recordsByName.
Comment on lines
+303
to
+314
| async function writeTsArrayFile(filePath, exportName, records) { | ||
| const existing = await fs.readFile(filePath, 'utf8'); | ||
| const exportRegex = new RegExp(`^[\\s\\S]*?export const\\s+${exportName}\\s*=\\s*`); | ||
| const match = existing.match(exportRegex); | ||
|
|
||
| if (!match) { | ||
| throw new Error(`Could not find export declaration for ${exportName} in ${filePath}.`); | ||
| } | ||
|
|
||
| const content = `${match[0]}${JSON.stringify(records, null, 4)}\n`; | ||
| await fs.writeFile(filePath, content, 'utf8'); | ||
| } |
Contributor
There was a problem hiding this comment.
writeTsArrayFile silently truncates everything after the array (including the trailing semicolon and any subsequent declarations).
The regex on Line 305 matches from the start of file up to and including export const ${exportName} =, and the replacement is written as ${match[0]}${JSON.stringify(records)}\n with no trailing ;. This means:
- The written file will always lack a trailing semicolon, which will trigger lint/compiler warnings on every refresh.
- Any declarations that follow the array in the file (e.g., re-exports, helper constants) are permanently deleted on each run.
At minimum, append ; to the written content. If additional declarations may follow, capture and re-append the portion of the file after the closing ].
🐛 Minimal fix — preserve semicolon
- const content = `${match[0]}${JSON.stringify(records, null, 4)}\n`;
+ const arrayEnd = existing.lastIndexOf(']');
+ const suffix = existing.slice(arrayEnd + 1); // preserves ';' and anything that follows
+ const content = `${match[0]}${JSON.stringify(records, null, 4)}${suffix.startsWith(';') ? suffix : ';\n'}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function writeTsArrayFile(filePath, exportName, records) { | |
| const existing = await fs.readFile(filePath, 'utf8'); | |
| const exportRegex = new RegExp(`^[\\s\\S]*?export const\\s+${exportName}\\s*=\\s*`); | |
| const match = existing.match(exportRegex); | |
| if (!match) { | |
| throw new Error(`Could not find export declaration for ${exportName} in ${filePath}.`); | |
| } | |
| const content = `${match[0]}${JSON.stringify(records, null, 4)}\n`; | |
| await fs.writeFile(filePath, content, 'utf8'); | |
| } | |
| async function writeTsArrayFile(filePath, exportName, records) { | |
| const existing = await fs.readFile(filePath, 'utf8'); | |
| const exportRegex = new RegExp(`^[\\s\\S]*?export const\\s+${exportName}\\s*=\\s*`); | |
| const match = existing.match(exportRegex); | |
| if (!match) { | |
| throw new Error(`Could not find export declaration for ${exportName} in ${filePath}.`); | |
| } | |
| const arrayEnd = existing.lastIndexOf(']'); | |
| const suffix = existing.slice(arrayEnd + 1); // preserves ';' and anything that follows | |
| const content = `${match[0]}${JSON.stringify(records, null, 4)}${suffix.startsWith(';') ? suffix : ';\n'}`; | |
| await fs.writeFile(filePath, content, 'utf8'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/mi/mi-extension/scripts/update-connector-context-db.js` around
lines 303 - 314, The writeTsArrayFile function currently overwrites everything
after the export declaration because exportRegex only matches up to "export
const <exportName> =" and the code writes match[0] + JSON without preserving the
trailing semicolon or any following content; update writeTsArrayFile to locate
the end of the array literal in the existing file (find the closing ] for the
exported array, include any trailing whitespace/comments and the existing
semicolon if present), then write back the prefix up through "export const
<exportName> =" + the new JSON.stringify(records, null, 4) + a semicolon and the
original suffix (everything after the closing ]), ensuring the regex/exportRegex
and match usage in writeTsArrayFile are adjusted to capture or compute the
suffix to preserve subsequent declarations.
Comment on lines
769
to
776
| try { | ||
| const finalResponse = await response; | ||
| const finalResponse: any = response | ||
| ? await awaitWithTimeout<any>(response, finalResponseWaitTimeoutMs) | ||
| : undefined; | ||
| finalModelMessages = finalResponse.messages || []; | ||
| } catch (error) { | ||
| logError('[Agent] Failed to capture model messages', error); | ||
| logError('[Agent] Failed to capture model messages on finish', error); | ||
| } |
Contributor
There was a problem hiding this comment.
Null dereference when response is falsy or awaitWithTimeout times out.
When response is undefined (or null), the ternary on Line 770-772 evaluates to undefined, and Line 773 immediately accesses .messages on it — throwing a TypeError. The same pattern repeats at Lines 798-801 and 824-827.
The catch on Line 774 will swallow it, but in the finish case the intent is clearly to capture messages for a successful return, and silently losing them due to a preventable NPE is undesirable.
Proposed fix (apply to all three occurrences)
- const finalResponse: any = response
- ? await awaitWithTimeout<any>(response, finalResponseWaitTimeoutMs)
- : undefined;
- finalModelMessages = finalResponse.messages || [];
+ const finalResponse: any = response
+ ? await awaitWithTimeout<any>(response, finalResponseWaitTimeoutMs)
+ : undefined;
+ finalModelMessages = finalResponse?.messages || [];🤖 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/agents/main/agent.ts`
around lines 769 - 776, The code tries to read .messages from a possibly
undefined result of awaitWithTimeout, causing a null dereference; update each
occurrence (the block using awaitWithTimeout, finalResponseWaitTimeoutMs and
assigning finalModelMessages) to first await into a local const (e.g.,
finalResponse) and then safely access messages using optional chaining or a
fallback (e.g., finalResponse?.messages ?? []), so finalModelMessages is always
an array; apply the same safe-access pattern to the other two identical blocks
to avoid NPEs and keep the existing logError catch behavior.
Comment on lines
+88
to
89
| YOU ARE IN DEVELOPMENT PHASE. NOT IN PRODUCTION YET. HELP THE DEVELOPER IF DEVELOPER ASKS ABOUT YOUR INTERNALS/TOOL CALLS etc | ||
| {{system_remainder}} |
Contributor
There was a problem hiding this comment.
Development-phase notice should not ship to production.
Line 88 instructs the model to reveal internals and tool calls. This contradicts the system prompt's own "Do not mention internal tool names to users" policy (system.ts Line 235) and the retrieved learning that system_reminder content must not be shown to end users. If this is intentionally temporary, consider gating it behind a debug/dev flag or adding a TODO to remove it before release.
🤖 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/agents/main/prompt.ts`
around lines 88 - 89, Remove or guard the hardcoded development notice string
("YOU ARE IN DEVELOPMENT PHASE...") in the prompt assembly so it is never sent
to production users; instead gate this message behind a debug/dev flag (e.g., a
config like isDevMode or process.env.DEBUG) or annotate it with a clear TODO to
delete before release. Specifically, update prompt.ts to stop injecting the
development string into the assembled prompt and ensure the variable
system_remainder is never exposed to end users (align with the non-disclosure
rule implemented in system.ts and its policy), and add a short unit/integration
check to confirm dev-only text is omitted unless debug mode is explicitly
enabled.
Comment on lines
238
to
246
| <SYNAPSE_DEVELOPMENT_GUIDELINES> | ||
| ${SYNAPSE_GUIDE} | ||
| </SYNAPSE_DEVELOPMENT_GUIDELINES> | ||
|
|
||
| <CONNECTOR_DEVELOPMENT_GUIDELINES> | ||
| ${CONNECTOR_DOCUMENTATION} | ||
| </CONNECTOR_DEVELOPMENT_GUIDELINES> | ||
| `; | ||
| const SYSTEM_PROMPT_OLD = SYSTEM_PROMPT.replace(SYNAPSE_GUIDE, SYNAPSE_GUIDE_OLD); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the target file
cat -n workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts | sed -n '230,255p'Repository: wso2/vscode-extensions
Length of output: 1404
🏁 Script executed:
# Search for CONNECTOR_DOCUMENTATION definition
rg -n 'CONNECTOR_DOCUMENTATION\s*=' --type=ts -A 5Repository: wso2/vscode-extensions
Length of output: 1814
🏁 Script executed:
# Search for the version-gated features mentioned
rg -n 'responseVariable|overwriteBody|connectionLocalEntryNeeded|noInitializationNeeded' --type=ts | head -30Repository: wso2/vscode-extensions
Length of output: 4288
🏁 Script executed:
# Find where SYNAPSE_GUIDE and CONNECTOR_DOCUMENTATION are defined
rg -n 'SYNAPSE_GUIDE|CONNECTOR_DOCUMENTATION' --type=ts -B 2 -A 2 | head -50Repository: wso2/vscode-extensions
Length of output: 5296
🏁 Script executed:
# Check the full CONNECTOR_DOCUMENTATION content
cat -n workspaces/mi/mi-extension/src/ai-features/agent-mode/context/connectors_guide.ts | sed -n '21,97p'Repository: wso2/vscode-extensions
Length of output: 3838
🏁 Script executed:
# Search for version-specific information in connectors_guide.ts
rg -n 'version|4\.4|runtime|responseVariable|overwriteBody|connectionLocalEntryNeeded|noInitializationNeeded' workspaces/mi/mi-extension/src/ai-features/agent-mode/context/connectors_guide.tsRepository: wso2/vscode-extensions
Length of output: 911
🏁 Script executed:
# Check if there's an old version of CONNECTOR_DOCUMENTATION
fd 'connectors_guide' --type=fRepository: wso2/vscode-extensions
Length of output: 147
🏁 Script executed:
# Look at the agent-mode system.ts file more thoroughly to understand the version handling
cat -n workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts | head -50Repository: wso2/vscode-extensions
Length of output: 2215
🏁 Script executed:
# Check what happens with CONNECTOR_DOCUMENTATION in SYSTEM_PROMPT_OLD
cat -n workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts | sed -n '1,100p'Repository: wso2/vscode-extensions
Length of output: 7259
🏁 Script executed:
# Look for any OLD version of connector documentation
fd 'connectors' --type=f -e ts | xargs rg -l 'old|OLD|v1|version'Repository: wso2/vscode-extensions
Length of output: 233
🏁 Script executed:
# Check the complete getSystemPrompt function to understand the version logic
cat -n workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts | sed -n '248,260p'Repository: wso2/vscode-extensions
Length of output: 420
🏁 Script executed:
# Search for RUNTIME_VERSION_440 to understand what it means
rg -n 'RUNTIME_VERSION_440' --type=ts -B 2 -A 2Repository: wso2/vscode-extensions
Length of output: 44633
Create or use a version-appropriate connector documentation for older runtimes.
SYSTEM_PROMPT_OLD swaps SYNAPSE_GUIDE → SYNAPSE_GUIDE_OLD but retains the same CONNECTOR_DOCUMENTATION. The current documentation includes "Revamped response handling" features (responseVariable, overwriteBody) that are explicitly documented as supported only by certain connectors and newer runtimes. For MI runtime < 4.4.0, older code paths lack these features, making the guidance inapplicable. Either create CONNECTOR_DOCUMENTATION_OLD without the revamped response handling section, or conditionally render that section based on runtime version.
🤖 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/agents/main/system.ts`
around lines 238 - 246, SYSTEM_PROMPT_OLD currently substitutes SYNAPSE_GUIDE
with SYNAPSE_GUIDE_OLD but reuses CONNECTOR_DOCUMENTATION which documents
"Revamped response handling" (responseVariable, overwriteBody) not available for
older MI runtimes; fix by adding a version-appropriate connector doc (e.g.,
CONNECTOR_DOCUMENTATION_OLD) that omits the revamped response handling and use
it when building SYSTEM_PROMPT_OLD, or update the SYSTEM_PROMPT generation to
conditionally include the revamped section only when the runtime version >=
4.4.0 (check runtime version before choosing CONNECTOR_DOCUMENTATION vs
CONNECTOR_DOCUMENTATION_OLD); update references to SYSTEM_PROMPT_OLD,
SYNAPSE_GUIDE, SYNAPSE_GUIDE_OLD, and CONNECTOR_DOCUMENTATION to reflect the
chosen approach.
Comment on lines
+60
to
+79
| const POWERSHELL_SAFE_READ_COMMANDS = new Set([ | ||
| 'cat', | ||
| 'dir', | ||
| 'get-childitem', | ||
| 'get-content', | ||
| 'get-item', | ||
| 'get-location', | ||
| 'get-process', | ||
| 'git', | ||
| 'ls', | ||
| 'measure-object', | ||
| 'pwd', | ||
| 'resolve-path', | ||
| 'select-string', | ||
| 'sort-object', | ||
| 'split-path', | ||
| 'test-path', | ||
| 'type', | ||
| 'whoami', | ||
| ]); |
Contributor
There was a problem hiding this comment.
Do not classify all git invocations as safe read-only.
Line 68 places git in the unconditional safe-read set. That lets mutating/destructive subcommands (for example git commit, git reset --hard, git clean) bypass approval in the read-only branch.
🔧 Minimal safe fix
const POWERSHELL_SAFE_READ_COMMANDS = new Set([
'cat',
'dir',
@@
- 'git',
'ls',Also applies to: 804-813
🤖 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/shell_sandbox_powershell.ts`
around lines 60 - 79, POWERSHELL_SAFE_READ_COMMANDS currently includes 'git',
which incorrectly treats all git invocations as read-only; remove 'git' from
POWERSHELL_SAFE_READ_COMMANDS and instead add explicit handling where commands
are analyzed: when the parsed command is 'git' inspect the first
subcommand/argument and only allow a curated whitelist of safe git subcommands
(e.g., status, rev-parse, log, show, diff, ls-files, branch -r, remote -v, tag
-l) to be treated as read-only; any other git subcommand (commit, reset, clean,
checkout, merge, rebase, etc.) must require approval or be routed to the
mutating branch. Update the logic that checks commands (the code that currently
consults POWERSHELL_SAFE_READ_COMMANDS) to call this git-specific validator so
git is not universally trusted.
Comment on lines
+707
to
+710
| const networkFromCommands = commandNames.some((commandName) => POWERSHELL_NETWORK_COMMANDS.has(commandName)); | ||
| const networkFromDotNet = detectNetworkFromDotNet(trimmedCommand); | ||
| const networkDetected = networkFromCommands || networkFromDotNet; | ||
|
|
Contributor
There was a problem hiding this comment.
Require approval when network access is detected.
On Line 804, networkDetected only bypasses the read-only allowlist check; it does not add any approval reason. As a result, commands like curl/Invoke-WebRequest can end up with requiresApproval=false (Line 824), which is a sandbox bypass.
🔧 Proposed fix
const networkFromCommands = commandNames.some((commandName) => POWERSHELL_NETWORK_COMMANDS.has(commandName));
const networkFromDotNet = detectNetworkFromDotNet(trimmedCommand);
const networkDetected = networkFromCommands || networkFromDotNet;
+ if (networkDetected) {
+ reasons.push('Commands that access the network require approval.');
+ }Also applies to: 804-815, 823-826
🤖 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/shell_sandbox_powershell.ts`
around lines 707 - 710, The code computes networkDetected using
POWERSHELL_NETWORK_COMMANDS and detectNetworkFromDotNet (based on commandNames
and trimmedCommand) but only bypasses the read-only allowlist check without
marking the action as requiring approval; update the approval logic so that when
networkDetected is true you set requiresApproval = true and append a clear
approvalReason (e.g., "network access detected via command/dotnet call") so
commands like curl/Invoke-WebRequest cannot slip through, and ensure this change
is applied wherever requiresApproval and approvalReason are set in the approval
flow handling (reference variables networkDetected, POWERSHELL_NETWORK_COMMANDS,
detectNetworkFromDotNet, requiresApproval, approvalReason).
Comment on lines
+97
to
+127
| const SAFE_READ_COMMANDS = new Set([ | ||
| 'cat', | ||
| 'cd', | ||
| 'cut', | ||
| 'dir', | ||
| 'dirname', | ||
| 'du', | ||
| 'echo', | ||
| 'env', | ||
| 'find', | ||
| 'git', | ||
| 'grep', | ||
| 'head', | ||
| 'id', | ||
| 'ls', | ||
| 'pwd', | ||
| 'readlink', | ||
| 'realpath', | ||
| 'rg', | ||
| 'select-string', | ||
| 'sort', | ||
| 'stat', | ||
| 'tail', | ||
| 'tree', | ||
| 'type', | ||
| 'uniq', | ||
| 'wc', | ||
| 'where', | ||
| 'which', | ||
| 'whoami', | ||
| ]); |
Contributor
There was a problem hiding this comment.
find in SAFE_READ_COMMANDS can execute arbitrary mutations via -exec, -execdir, or -delete.
find is classified as a safe read-only command, but find /path -delete or find /path -exec rm {} \; performs destructive mutations. Since the per-segment analyzer doesn't inspect find's arguments for -exec/-delete/-execdir, these commands would pass through without approval.
Consider either removing find from SAFE_READ_COMMANDS (so it always requires approval) or adding argument-level detection for its mutation flags in analyzeSegment.
Minimal detection approach
Add a check in analyzeSegment (or a dedicated helper) for find's dangerous flags:
+function isFindMutation(tokens: string[]): boolean {
+ if (normalizeToken(tokens[0] || '') !== 'find') {
+ return false;
+ }
+ const dangerousFlags = ['-exec', '-execdir', '-delete', '-ok', '-okdir'];
+ return tokens.some((token) => dangerousFlags.includes(normalizeToken(token)));
+}Then include isFindMutation(tokens) in the isMutation computation within analyzeSegment.
🤖 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/shell_sandbox.ts`
around lines 97 - 127, The SAFE_READ_COMMANDS entry for "find" is unsafe because
it can perform mutations via flags like -exec, -execdir, or -delete; remove
"find" from the SAFE_READ_COMMANDS Set so any use of find requires approval, or
alternatively implement a helper (e.g., isFindMutation(tokens)) that inspects
the tokenized arguments for dangerous flags and include that check in
analyzeSegment's isMutation computation (refer to analyzeSegment, tokens, and
isMutation) so find remains allowed only when its arguments are confirmed
read-only.
Comment on lines
+230
to
+249
| private async addShellApprovalRule(rule: string[]): Promise<void> { | ||
| if (!this.currentSessionId) { | ||
| return; | ||
| } | ||
|
|
||
| const normalizedRule = this.normalizeShellApprovalRule(rule); | ||
| if (normalizedRule.length === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const existingKeys = new Set(this.shellApprovalRules.map((currentRule) => currentRule.join('\u0000'))); | ||
| const ruleKey = normalizedRule.join('\u0000'); | ||
| if (existingKeys.has(ruleKey)) { | ||
| return; | ||
| } | ||
|
|
||
| this.shellApprovalRules.push(normalizedRule); | ||
| await this.persistShellApprovalRulesForSession(this.currentSessionId); | ||
| logInfo(`[AgentPanel] Added shell approval rule for session ${this.currentSessionId}: ${normalizedRule.join(' ')}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Persistence error in addShellApprovalRule propagates to the agent tool, potentially disrupting execution.
If persistShellApprovalRulesForSession throws (e.g., disk full, permission error), the error bubbles up through the shellApprovalRuleStore.addRule() callback into the shell tool handler. The in-memory rule at Line 246 is already added, so only the disk persistence fails — consider catching and logging the error to keep the agent flow uninterrupted.
Proposed fix
this.shellApprovalRules.push(normalizedRule);
- await this.persistShellApprovalRulesForSession(this.currentSessionId);
+ try {
+ await this.persistShellApprovalRulesForSession(this.currentSessionId);
+ } catch (error) {
+ logError(`[AgentPanel] Failed to persist shell approval rule for session ${this.currentSessionId}`, error);
+ }
logInfo(`[AgentPanel] Added shell approval rule for session ${this.currentSessionId}: ${normalizedRule.join(' ')}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private async addShellApprovalRule(rule: string[]): Promise<void> { | |
| if (!this.currentSessionId) { | |
| return; | |
| } | |
| const normalizedRule = this.normalizeShellApprovalRule(rule); | |
| if (normalizedRule.length === 0) { | |
| return; | |
| } | |
| const existingKeys = new Set(this.shellApprovalRules.map((currentRule) => currentRule.join('\u0000'))); | |
| const ruleKey = normalizedRule.join('\u0000'); | |
| if (existingKeys.has(ruleKey)) { | |
| return; | |
| } | |
| this.shellApprovalRules.push(normalizedRule); | |
| await this.persistShellApprovalRulesForSession(this.currentSessionId); | |
| logInfo(`[AgentPanel] Added shell approval rule for session ${this.currentSessionId}: ${normalizedRule.join(' ')}`); | |
| } | |
| private async addShellApprovalRule(rule: string[]): Promise<void> { | |
| if (!this.currentSessionId) { | |
| return; | |
| } | |
| const normalizedRule = this.normalizeShellApprovalRule(rule); | |
| if (normalizedRule.length === 0) { | |
| return; | |
| } | |
| const existingKeys = new Set(this.shellApprovalRules.map((currentRule) => currentRule.join('\u0000'))); | |
| const ruleKey = normalizedRule.join('\u0000'); | |
| if (existingKeys.has(ruleKey)) { | |
| return; | |
| } | |
| this.shellApprovalRules.push(normalizedRule); | |
| try { | |
| await this.persistShellApprovalRulesForSession(this.currentSessionId); | |
| } catch (error) { | |
| logError(`[AgentPanel] Failed to persist shell approval rule for session ${this.currentSessionId}`, error); | |
| } | |
| logInfo(`[AgentPanel] Added shell approval rule for session ${this.currentSessionId}: ${normalizedRule.join(' ')}`); | |
| } |
🤖 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 230 - 249, The addShellApprovalRule method currently allows errors from
persistShellApprovalRulesForSession to bubble up; wrap the await
this.persistShellApprovalRulesForSession(this.currentSessionId) call in a
try/catch, catch any error, log it via logInfo or a dedicated logger with
context (including this.currentSessionId and normalizedRule), and do not rethrow
so the in-memory this.shellApprovalRules remains and the agent flow/shell tool
callback is not disrupted; keep the existing push to this.shellApprovalRules
before persistence so the method still succeeds even if persistence fails.
Comment on lines
+666
to
+691
| // Get current state before updating | ||
| const currentState = StateMachineAI.state(); | ||
|
|
||
| if (isUsageExceeded && currentState === 'Authenticated') { | ||
| logInfo('Quota exceeded. Transitioning to UsageExceeded state.'); | ||
| StateMachineAI.sendEvent(AI_EVENT_TYPE.USAGE_EXCEEDED); | ||
| } | ||
| // Update usage via state machine event (proper XState pattern) | ||
| if (currentState === 'Authenticated' || currentState === 'UsageExceeded') { | ||
| StateMachineAI.sendEvent({ type: AI_EVENT_TYPE.UPDATE_USAGE, payload: { usage } }); | ||
| } | ||
|
|
||
| // Check if we're in UsageExceeded state and if usage has reset | ||
| const isUsageReset = usage.remainingUsagePercentage > 0; | ||
| // Check if quota is exceeded and transition to UsageExceeded state | ||
| const isUnlimitedUsage = usage.remainingUsagePercentage === -1; | ||
| const isUsageExceeded = !isUnlimitedUsage && usage.remainingUsagePercentage <= 0; | ||
|
|
||
| if (currentState === 'UsageExceeded' && isUsageReset) { | ||
| logInfo('Usage has reset. Transitioning back to Authenticated state.'); | ||
| StateMachineAI.sendEvent(AI_EVENT_TYPE.USAGE_RESET); | ||
| } | ||
| if (isUsageExceeded && currentState === 'Authenticated') { | ||
| logInfo('Quota exceeded. Transitioning to UsageExceeded state.'); | ||
| StateMachineAI.sendEvent(AI_EVENT_TYPE.USAGE_EXCEEDED); | ||
| } | ||
|
|
||
| return usage; | ||
| // Check if we're in UsageExceeded state and if usage has reset | ||
| const isUsageReset = isUnlimitedUsage || usage.remainingUsagePercentage > 0; | ||
|
|
||
| if (currentState === 'UsageExceeded' && isUsageReset) { | ||
| logInfo('Usage has reset. Transitioning back to Authenticated state.'); | ||
| StateMachineAI.sendEvent(AI_EVENT_TYPE.USAGE_RESET); | ||
| } | ||
|
|
||
| return usage; |
Contributor
There was a problem hiding this comment.
Race condition: state may change between StateMachineAI.state() read and sendEvent calls.
currentState is read at Line 667, but by the time sendEvent fires at Line 672/680/688, the state may have already changed (e.g., another concurrent fetchUsage call or a 429 in fetchWithAuth triggering USAGE_EXCEEDED). The window is small and the worst outcome is a redundant/ignored event, so this is low-risk, but worth noting.
🤖 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/ai-features/rpc-manager.ts`
around lines 666 - 691, The code reads currentState once via
StateMachineAI.state() and then may call
StateMachineAI.sendEvent(AI_EVENT_TYPE.*) later, which can race with other
callers; to fix, avoid using the stale local currentState: instead re-check
StateMachineAI.state() immediately before each send (for the UPDATE_USAGE,
USAGE_EXCEEDED and USAGE_RESET branches) and only call StateMachineAI.sendEvent
when the freshly read state still matches the expected state, or alternatively
send the event unconditionally but rely on the StateMachineAI guards/guards in
the state machine to ignore invalid transitions; update the logic around
StateMachineAI.state(), StateMachineAI.sendEvent, and the AI_EVENT_TYPE.* usages
accordingly so state decisions are based on a fresh read or delegated to the
state machine itself.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes:
Add a few authentication and stability improvements
Summary by CodeRabbit
Release Notes
New Features
Improvements