[Copilot] AWS Bedrock support + web-tools restructuring#2094
Open
IsuruMaduranga wants to merge 17 commits intowso2:mainfrom
Open
[Copilot] AWS Bedrock support + web-tools restructuring#2094IsuruMaduranga wants to merge 17 commits intowso2:mainfrom
IsuruMaduranga wants to merge 17 commits intowso2:mainfrom
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughRemoves request-level web-access preapproval; adds per-turn web-tools provider selection (tavily/anthropic/none) and Tavily API-key RPC/UI; extends AWS Bedrock auth to support IAM and API-key modes; adds persisted per-session session-context block hashes for selective reinjection and drift detection. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User/UI
participant RPC as Frontend RPC Client
participant Ext as Extension Host
participant Auth as Auth/Storage
participant Agent as Agent Executor
participant Tav as Tavily API
UI->>RPC: setTavilyApiKey({ apiKey })
RPC->>Ext: sendRequest setTavilyApiKey
Ext->>Auth: persist tavilyApiKey
Ext-->>RPC: { success: true }
RPC-->>UI: update settings UI
UI->>RPC: sendAgentMessage(user input)
RPC->>Ext: sendRequest sendAgentMessage(...)
Ext->>Auth: read loginMethod + tavilyApiKey
Ext->>Agent: executeAgent(..., webToolsProvider based on loginMethod+tavilyApiKey)
Agent->>Tav: runTavilySearch/Extract (if tavily-local)
Agent-->>Ext: tool results
Ext-->>RPC: agent response stream
RPC-->>UI: render agent stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
Adds AWS Bedrock as a first-class LLM backend for MI Copilot agent mode, restructures web tool wiring to avoid extra LLM round-trips, and introduces a Tavily BYOK path for web search/fetch when running on Bedrock.
Changes:
- Switch Bedrock integration to the Anthropic InvokeModel provider (
@ai-sdk/amazon-bedrock/anthropic) and update Opus preset labeling to 4.7. - Rework web tools: use Anthropic first-party server tools inline on Proxy/BYOK; use Tavily-backed local
web_search/web_fetchon Bedrock (with settings + RPC support for storing Tavily key). - Remove the per-call web approval flow and improve provider error diagnostics surfaced in logs.
Reviewed changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/mi/mi-visualizer/src/views/LoggedOutWindow/index.tsx | Adds AWS Bedrock login CTA and updates login button copy. |
| workspaces/mi/mi-visualizer/src/views/DisabledWindow/index.tsx | Adjusts sign-out messaging to clarify MI Copilot vs platform session. |
| workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx | Implements Bedrock login form (API key vs IAM) and optional Tavily key capture. |
| workspaces/mi/mi-visualizer/src/views/AIPanel/component/SettingsPanel.tsx | Adds Bedrock-only Web Search (Tavily key) settings section + updated sign-out copy. |
| workspaces/mi/mi-visualizer/src/views/AIPanel/component/ModelSettingsMenu.tsx | Updates Opus display label to 4.7. |
| workspaces/mi/mi-visualizer/src/views/AIPanel/component/AICodeGenerator.tsx | Plumbs isAwsBedrock into SettingsPanel to gate Tavily controls. |
| workspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsx | Removes local web-approval toggle and webAccessPreapproved plumbing. |
| workspaces/mi/mi-rpc-client/src/rpc-clients/ai-features/rpc-client.ts | Adds MiAiPanel RPC methods for get/set Tavily API key. |
| workspaces/mi/mi-extension/src/rpc-managers/mi-diagram/rpc-manager.ts | Updates logout confirmation copy and button label. |
| workspaces/mi/mi-extension/src/rpc-managers/ai-features/rpc-manager.ts | Implements get/set Tavily API key RPC endpoints via auth layer. |
| workspaces/mi/mi-extension/src/rpc-managers/ai-features/rpc-handler.ts | Registers new Tavily RPC handlers. |
| workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts | Removes webAccessPreapproved from agent request forwarding. |
| workspaces/mi/mi-extension/src/ai-features/connection.ts | Switches Bedrock provider to InvokeModel Anthropic subpath; updates model IDs and cache-control providerOptions keying. |
| workspaces/mi/mi-extension/src/ai-features/auth.ts | Adds Bedrock API-key auth support, Tavily key storage helpers, and explicit-logout gating for silent bootstrap. |
| workspaces/mi/mi-extension/src/ai-features/aiMachine.ts | Threads Bedrock auth type + Tavily key through validation and token resolution; respects explicit logout state. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.ts | Replaces wrapper-based web tools with Tavily local implementations + Anthropic server-tool factory. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/types.ts | Removes web tools from deferred-tool set. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/tool_load.ts | Removes deferred-tool descriptions for web tools. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/stream_guard.ts | Enhances error diagnostics output with AI SDK APICallError fields. |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/tools.ts | Routes web tool registration by backend/provider choice (anthropic-server vs tavily-local vs none). |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts | Documents backend-aware behavior in the system prompt (“Copilot backends”). |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts | Adds backend label in <env> and a reminder when web search is unavailable on Bedrock (no Tavily key). |
| workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/agent.ts | Resolves backend per turn, wires web tools accordingly, and adds Bedrock beta header for tool-search defer-loading. |
| workspaces/mi/mi-extension/package.json | Updates AI SDK deps and adds @tavily/core. |
| workspaces/mi/mi-core/src/state-machine-types.ts | Extends Bedrock credential types with authType and optional tavilyApiKey. |
| workspaces/mi/mi-core/src/rpc-types/ai-features/rpc-type.ts | Adds RPC request types for get/set Tavily key. |
| workspaces/mi/mi-core/src/rpc-types/ai-features/index.ts | Extends MIAIPanelAPI with get/set Tavily key. |
| workspaces/mi/mi-core/src/rpc-types/agent-mode/types.ts | Removes web approval fields/kinds from agent-mode RPC types. |
| common/config/rush/pnpm-lock.yaml | Locks updated AI SDK versions and adds Tavily dependency. |
Files not reviewed (1)
- common/config/rush/pnpm-lock.yaml: Language not supported
💡 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: 7
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/agent-mode/agents/main/system.ts (1)
148-151:⚠️ Potential issue | 🟡 MinorStale "Both require user approval" text contradicts the new no-approval flow.
Line 150 still says "Both require user approval." This text is now incorrect — per the PR, the per-call web approval flow (
webAccessPreapproved,PlanApprovalKind, approval modal) was removed and web tools now run inline without per-call approval. Update or drop this sentence so it doesn't conflict with the new "Copilot backends" section below.📝 Proposed fix
## Web tools - ${WEB_SEARCH_TOOL_NAME}: external research. Prefer MI docs (allowed_domains=["mi.docs.wso2.com"]), also use GitHub issues, Stack Overflow when useful. -- ${WEB_FETCH_TOOL_NAME}: fetch URL content (not JS-rendered sites; MI docs is JS-rendered, so use ${WEB_SEARCH_TOOL_NAME} for those). Both require user approval. +- ${WEB_FETCH_TOOL_NAME}: fetch URL content (not JS-rendered sites; MI docs is JS-rendered, so use ${WEB_SEARCH_TOOL_NAME} for those).🤖 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 148 - 151, Update the documentation block that lists WEB_SEARCH_TOOL_NAME and WEB_FETCH_TOOL_NAME by removing or changing the final sentence that reads "Both require user approval." to reflect the new no-approval flow: delete that sentence (or replace it with a short note that web tools now run inline without per-call approval) and ensure consistency with the new identifiers and flow mentioned elsewhere (webAccessPreapproved, PlanApprovalKind and the "Copilot backends" section) so the description of ${WEB_SEARCH_TOOL_NAME} and ${WEB_FETCH_TOOL_NAME} no longer contradicts the updated approval behavior.
🧹 Nitpick comments (7)
workspaces/mi/mi-visualizer/src/views/AIPanel/component/AICodeGenerator.tsx (1)
56-70: Consider guarding against unmount and Promise rejection.If the component unmounts before
Promise.allresolves,setIsByok/setIsAwsBedrockwill be invoked on an unmounted component. Additionally, an unhandled rejection from either RPC will leave both flagsfalsewithout surfacing the error. A simplecancelledflag plus atry/catchwould address both:♻️ Suggested refactor
useEffect(() => { + let cancelled = false; const checkByok = async () => { if (!rpcClient) { return; } - const [hasApiKey, machineView] = await Promise.all([ - rpcClient.getMiAiPanelRpcClient().hasAnthropicApiKey(), - rpcClient.getAIVisualizerState(), - ]); - const isBedrock = machineView?.loginMethod === LoginMethod.AWS_BEDROCK; - setIsByok(!!hasApiKey || isBedrock); - setIsAwsBedrock(isBedrock); + try { + const [hasApiKey, machineView] = await Promise.all([ + rpcClient.getMiAiPanelRpcClient().hasAnthropicApiKey(), + rpcClient.getAIVisualizerState(), + ]); + if (cancelled) return; + const isBedrock = machineView?.loginMethod === LoginMethod.AWS_BEDROCK; + setIsByok(!!hasApiKey || isBedrock); + setIsAwsBedrock(isBedrock); + } catch { + // swallow — settings panel will simply hide BYOK/Tavily controls + } }; checkByok(); + return () => { cancelled = true; }; }, [rpcClient]);🤖 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/AICodeGenerator.tsx` around lines 56 - 70, The useEffect's async helper checkByok should guard against component unmount and surface RPC errors: wrap the Promise.all call to rpcClient.getMiAiPanelRpcClient().hasAnthropicApiKey() and rpcClient.getAIVisualizerState() in a try/catch and log/handle the error, and add a cancelled flag (e.g., let cancelled = false; and set cancelled = true in the effect cleanup) so that before calling setIsByok and setIsAwsBedrock you check if (!cancelled) to avoid updating state after unmount; apply this change inside the existing checkByok function referenced in the effect and ensure the cleanup returns a function that flips the cancelled flag.workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx (1)
606-616: Replacehref="#"anchor with a button-styled link.
<a href="#">with anonClickthat callspreventDefault()is an accessibility/UX anti-pattern: keyboard users navigate it as a link that goes nowhere, screen readers announce it as a link, and middle-clicking jumps to the top of the page. Since the action opens an external URL via the RPC client (not navigation), a<button>styled as a link is more correct. Alternatively, sethref="https://app.tavily.com",target="_blank",rel="noopener noreferrer"and let the click handler still capture for analytics / external-open.♻️ Suggested change
- <a - href="#" - onClick={(e) => { - e.preventDefault(); - rpcClient.getMiVisualizerRpcClient().openExternal({ uri: "https://app.tavily.com" }); - }} - style={{ color: "var(--vscode-textLink-foreground)" }} - > - Get a free Tavily API key - </a> + <a + href="https://app.tavily.com" + onClick={(e) => { + e.preventDefault(); + rpcClient.getMiVisualizerRpcClient().openExternal({ uri: "https://app.tavily.com" }); + }} + rel="noopener noreferrer" + style={{ color: "var(--vscode-textLink-foreground)" }} + > + Get a free Tavily API key + </a>🤖 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/WaitingForLoginSection.tsx` around lines 606 - 616, In WaitingForLoginSection, replace the anchor element that uses href="#" with a semantically correct control: either change it to a <button> (styled to look like a link) and keep the existing onClick that calls rpcClient.getMiVisualizerRpcClient().openExternal({ uri: "https://app.tavily.com" }), or set the anchor's href to "https://app.tavily.com" with target="_blank" and rel="noopener noreferrer" and preserve the onClick for instrumentation; remove preventDefault() when using a real href. Ensure the element still uses the same inline style color and that keyboard/screen-reader semantics reflect a button if you choose the <button> option.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.ts (1)
67-107: Optional: cache the Tavily client per-key instead of re-creating it on every call.
createTavilyClient({ apiKey })is invoked on everyweb_search/web_fetchexecution (lines 72 and 111). For long sessions with many web tool calls this is wasted allocation and (depending on the client internals) potential repeated TLS/agent setup. A simple per-processMap<apiKey, Client>cache or memoizing inside the closure ofcreateWebSearchExecute/createWebFetchExecutewould avoid it.🤖 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/web_tools.ts` around lines 67 - 107, runTavilySearch currently calls createTavilyClient({ apiKey }) on every invocation which wastes allocations; add a simple per-process cache (e.g., a Map<string, TavilyClient>) and use it to reuse clients by apiKey: check the map for an existing client before calling createTavilyClient, store newly created clients into the map, and use the cached client for subsequent calls; apply the same memoization approach to the corresponding web_fetch code paths (the other function that calls createTavilyClient) so both web_search/web_fetch reuse clients instead of recreating them each time.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/tools.ts (1)
605-610: Note: Anthropic server-side tools bypass the mode-restriction pipeline.
createAnthropicServerWebTools(anthropicProvider)returns the provider tools as-is (nogetWrappedExecutewrapping), so theevaluateModeRestriction/validateReadOnlyServerManagementArgs/persistOversizedToolResultpipeline doesn't run forweb_search/web_fetchon the Anthropic/Proxy backend. Today this is fine because both tools are inREAD_ONLY_MODE_ALLOWED_TOOLSand their results stream from Anthropic so persistence isn't applicable — but worth a comment inCreateToolsParamsor nearbuildWebToolsso a future maintainer doesn't add a server-side tool that needs Plan-mode gating and forget the asymmetry.🤖 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` around lines 605 - 610, Add a TODO comment warning that Anthropic server-side tools bypass the normal mode-restriction and persistence pipeline: note that createAnthropicServerWebTools(anthropicProvider) returns tools unwrapped (no getWrappedExecute) so evaluateModeRestriction, validateReadOnlyServerManagementArgs, and persistOversizedToolResult are not applied for web_search/web_fetch on the Anthropic/Proxy backend; suggest documenting this asymmetry near CreateToolsParams or buildWebTools and reference READ_ONLY_MODE_ALLOWED_TOOLS and anthropicProvider so future maintainers know to gate any new server-side tools that require Plan-mode checks or result persistence.workspaces/mi/mi-core/src/state-machine-types.ts (1)
155-167: SUBMIT_AWS_CREDENTIALS IAM variant letsaccessKeyId/secretAccessKeybe omitted at the type level.In the IAM branch of the event payload (lines 156–161),
accessKeyIdandsecretAccessKeyare optional, even thoughAwsBedrockIamSecrets(lines 210–212) requires them. The PR objective says the event was made "discriminated", but withauthType?: 'iam'optional and the credential fields also optional, a payload like{ region: 'us-east-1' }is type-valid for IAM. DownstreamaiMachine.tsdoes validate this at runtime (per the relevant snippet, line 640+), but it would be strictly safer to either:
- make IAM credentials required in the event type so the type system catches invalid payloads, or
- promote
authTypeto a required discriminator on both variants.Not a runtime bug given the existing validators, but tightening the type would reduce the surface for future regressions.
♻️ Tighter discriminated payload
[AI_EVENT_TYPE.SUBMIT_AWS_CREDENTIALS]: { - authType?: 'iam'; - accessKeyId?: string; - secretAccessKey?: string; + authType: 'iam'; + accessKeyId: string; + secretAccessKey: string; region: string; sessionToken?: string; tavilyApiKey?: string; } | { authType: 'api_key'; apiKey: string; region: string; tavilyApiKey?: string; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-core/src/state-machine-types.ts` around lines 155 - 167, The IAM branch of AI_EVENT_TYPE.SUBMIT_AWS_CREDENTIALS currently allows missing credentials because authType is optional and accessKeyId/secretAccessKey are optional; update the type so the discriminator and required fields are enforced — either make authType required (change authType?: 'iam' to authType: 'iam') and make accessKeyId and secretAccessKey non-optional to match AwsBedrockIamSecrets, or make authType a required discriminator on both variants (ensure authType is present for the api_key variant as well) so the compiler rejects payloads like { region: 'us-east-1' }; adjust the declaration in state-machine-types.ts and ensure aiMachine.ts validations remain consistent with AwsBedrockIamSecrets.workspaces/mi/mi-extension/src/ai-features/connection.ts (1)
76-76: Dead cache:cachedBedrockis reassigned on every call.The module-level
cachedBedrockvariable is unconditionally overwritten on each invocation (the comment "Always recreate to ensure fresh credentials" confirms intent). It contributes no caching, but does retain the last credentials object in memory after the function returns. Either remove the cache variable, or actually reuse it when credentials are unchanged (e.g., key byauthType + region + accessKeyId/apiKey-fingerprint).♻️ Proposed simplification (drop the dead cache)
let cachedAnthropic: ReturnType<typeof createAnthropic> | null = null; -let cachedBedrock: ReturnType<typeof createBedrockAnthropic> | null = null; let cachedAuthMethod: LoginMethod | null = null;const getBedrockProvider = async (): Promise<{ provider: ReturnType<typeof createBedrockAnthropic>; credentials: Awaited<ReturnType<typeof getAwsBedrockCredentials>> & {}; }> => { const credentials = await getAwsBedrockCredentials(); if (!credentials) { throw new Error("Authentication failed: Unable to get AWS Bedrock credentials"); } - // Always recreate to ensure fresh credentials - cachedBedrock = credentials.authType === 'api_key' + const provider = credentials.authType === 'api_key' ? createBedrockAnthropic({ region: credentials.region, apiKey: credentials.apiKey, }) : createBedrockAnthropic({ region: credentials.region, accessKeyId: credentials.accessKeyId, secretAccessKey: credentials.secretAccessKey, sessionToken: credentials.sessionToken, }); - return { provider: cachedBedrock, credentials }; + return { provider, credentials }; };Also applies to: 312-335
🤖 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/connection.ts` at line 76, The module-level cachedBedrock variable is useless because it gets reassigned on every call; either remove it entirely or implement real reuse keyed by credentials. Locate cachedBedrock and the factory createBedrockAnthropic (and any similar cached variables around lines 312-335) and choose one of two fixes: 1) delete cachedBedrock and any code that assumes reuse so a fresh client is always created, or 2) implement a proper cache map keyed by a stable fingerprint (e.g., authType + region + accessKeyId/apiKey-fingerprint) and return the existing client when the key matches; ensure creation only occurs when no cached entry exists and clear/replace entries when credentials change.workspaces/mi/mi-extension/src/ai-features/auth.ts (1)
596-599: Optional: extract Bedrock model-ID constants to break the circular dep.The dynamic
import('./connection')here exists only to dodge a circular dependency (connection.ts already importsgetAwsBedrockCredentialsetc. from auth.ts). PullingBEDROCK_MODEL_MAP,getBedrockRegionalPrefix, andgetBedrockValidationModelIdout ofconnection.tsinto a smallbedrock-models.tsmodule would let both files import them statically and remove the lazy-load.🤖 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/auth.ts` around lines 596 - 599, Extract the Bedrock model-ID constants and helper into a new module (e.g., bedrock-models.ts) so you can remove the dynamic import in getBedrockValidationModelId; move BEDROCK_MODEL_MAP, getBedrockRegionalPrefix, and the logic for getBedrockValidationModelId out of connection.ts into that module, export them, then update connection.ts and auth.ts to import these symbols statically and replace the dynamic import in the getBedrockValidationModelId function with a direct call to the exported getBedrockValidationModelId.
🤖 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/src/ai-features/agent-mode/agents/main/agent.ts`:
- Around line 1148-1151: The current raw JSON dump of part.error is logged at
error level and may expose sensitive request/response data; replace the
logError(...) call that stringifies part.error with logDebug(...) so the full
raw dump is only available at debug level (keep the same truncation logic), and
rely on getErrorDiagnostics(part.error) for structured, non-sensitive error
info; alternatively, if you must keep error-level raw dumps, sanitize known
sensitive fields (e.g., requestBodyValues, responseBody, responseHeaders, data)
from part.error before stringifying to avoid leaking prompts, outputs, file
contents, or auth headers.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts`:
- Around line 111-117: The {{`#if` web_search_unavailable}} system-reminder is
currently nested inside {{`#if` include_session_context}} so it only appears on
the first message/post-compaction turns; confirm whether that trade-off is
intentional, and if not, move the entire {{`#if` web_search_unavailable}} ...
{{/if}} block out of the {{`#if` include_session_context}} block so the reminder
is emitted every turn when webSearchUnavailable is true (webSearchUnavailable is
recomputed per turn in agent logic); ensure the template still references the
same message text and keep the existing fallback behavior
(WEB_SEARCH_NOT_CONFIGURED in tools.ts) unchanged.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/stream_guard.ts`:
- Around line 127-134: In extractApiCallFields: cap fields.data to the same
2000-character truncation used for responseBody (convert to string via typeof
check/JSON.stringify with try/catch, then truncate and append "…") to prevent
unbounded log growth, and do not copy responseHeaders verbatim—filter/whitelist
headers before assigning to fields.responseHeaders (e.g., allow only safe keys
like x-request-id, retry-after, content-type, x-ratelimit-*, etc., and redact or
omit others) so sensitive headers (set-cookie, x-amz-security-token, auth
echoes) are not logged.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.ts`:
- Around line 109-150: runTavilyExtract returns rawContent verbatim and can
exceed model token limits; update the runTavilyExtract function to enforce a
content cap similar to createAnthropicServerWebTools (e.g., cap at ~32k tokens ≈
128k characters), truncate first.rawContent to that character limit before
building the message, and append a clear truncation marker like "\n\n[CONTENT
TRUNCATED]" so downstream models know the page was cut; ensure the header
(taskPrompt/URL) is preserved and the success Response.message uses the
truncated content.
In `@workspaces/mi/mi-extension/src/ai-features/connection.ts`:
- Around line 51-73: The comment shows that the current assumption that the
`global.` inference profile is available from any Bedrock-enabled region is
false; update validation and regional-prefix logic so callers get clear errors
and correct profiles: tighten `validateBedrockRegion` (auth.ts) to reject
unsupported partitions (GovCloud `us-gov-*` and China `cn-*`) and return a
descriptive error listing supported source regions per model (referencing
`BEDROCK_MODEL_MAP` and `ANTHROPIC_HAIKU_4_5`), and change
`getBedrockRegionalPrefix` (currently returning the constant
`BEDROCK_INFERENCE_PROFILE_PREFIX`) to map region + model to the correct profile
prefix (e.g., `global.` only for allowed source regions of each Claude/Haiku
model, otherwise map to `us.`, `eu.`, `apac.` or force an explicit
region→profile lookup), and ensure `getBedrockValidationModelId` uses that
mapping so callers fail fast with a clear message rather than a runtime "model
not found" error.
In `@workspaces/mi/mi-visualizer/src/views/AIPanel/component/SettingsPanel.tsx`:
- Around line 326-346: The Tavily key input in SettingsPanel.tsx (inside the
SettingsPanel component) is missing an accessible label; update the <input> that
uses showTavilyKey, tavilyDraft, setTavilyDraft and tavilyStatus to include an
aria-label attribute (for example aria-label="Tavily API key") so screen readers
announce the field properly while leaving existing props and handlers unchanged.
- Around line 115-144: The handler handleBedrockWebSearchToggle currently always
opens the Tavily input when called with enabled=true, causing a re-click of the
already-selected "On" to re-open the input; add an early no-op guard at the top
of handleBedrockWebSearchToggle that returns immediately if the requested
enabled state equals the current state (e.g., compare enabled to the component
state that represents the current toggle like isBedrockWebSearchOn or
Boolean(tavilyKey)), so only real state changes trigger setTavilyInputOpen,
setTavilyDraft, and RPC calls.
---
Outside diff comments:
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts`:
- Around line 148-151: Update the documentation block that lists
WEB_SEARCH_TOOL_NAME and WEB_FETCH_TOOL_NAME by removing or changing the final
sentence that reads "Both require user approval." to reflect the new no-approval
flow: delete that sentence (or replace it with a short note that web tools now
run inline without per-call approval) and ensure consistency with the new
identifiers and flow mentioned elsewhere (webAccessPreapproved, PlanApprovalKind
and the "Copilot backends" section) so the description of
${WEB_SEARCH_TOOL_NAME} and ${WEB_FETCH_TOOL_NAME} no longer contradicts the
updated approval behavior.
---
Nitpick comments:
In `@workspaces/mi/mi-core/src/state-machine-types.ts`:
- Around line 155-167: The IAM branch of AI_EVENT_TYPE.SUBMIT_AWS_CREDENTIALS
currently allows missing credentials because authType is optional and
accessKeyId/secretAccessKey are optional; update the type so the discriminator
and required fields are enforced — either make authType required (change
authType?: 'iam' to authType: 'iam') and make accessKeyId and secretAccessKey
non-optional to match AwsBedrockIamSecrets, or make authType a required
discriminator on both variants (ensure authType is present for the api_key
variant as well) so the compiler rejects payloads like { region: 'us-east-1' };
adjust the declaration in state-machine-types.ts and ensure aiMachine.ts
validations remain consistent with AwsBedrockIamSecrets.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/tools.ts`:
- Around line 605-610: Add a TODO comment warning that Anthropic server-side
tools bypass the normal mode-restriction and persistence pipeline: note that
createAnthropicServerWebTools(anthropicProvider) returns tools unwrapped (no
getWrappedExecute) so evaluateModeRestriction,
validateReadOnlyServerManagementArgs, and persistOversizedToolResult are not
applied for web_search/web_fetch on the Anthropic/Proxy backend; suggest
documenting this asymmetry near CreateToolsParams or buildWebTools and reference
READ_ONLY_MODE_ALLOWED_TOOLS and anthropicProvider so future maintainers know to
gate any new server-side tools that require Plan-mode checks or result
persistence.
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.ts`:
- Around line 67-107: runTavilySearch currently calls createTavilyClient({
apiKey }) on every invocation which wastes allocations; add a simple per-process
cache (e.g., a Map<string, TavilyClient>) and use it to reuse clients by apiKey:
check the map for an existing client before calling createTavilyClient, store
newly created clients into the map, and use the cached client for subsequent
calls; apply the same memoization approach to the corresponding web_fetch code
paths (the other function that calls createTavilyClient) so both
web_search/web_fetch reuse clients instead of recreating them each time.
In `@workspaces/mi/mi-extension/src/ai-features/auth.ts`:
- Around line 596-599: Extract the Bedrock model-ID constants and helper into a
new module (e.g., bedrock-models.ts) so you can remove the dynamic import in
getBedrockValidationModelId; move BEDROCK_MODEL_MAP, getBedrockRegionalPrefix,
and the logic for getBedrockValidationModelId out of connection.ts into that
module, export them, then update connection.ts and auth.ts to import these
symbols statically and replace the dynamic import in the
getBedrockValidationModelId function with a direct call to the exported
getBedrockValidationModelId.
In `@workspaces/mi/mi-extension/src/ai-features/connection.ts`:
- Line 76: The module-level cachedBedrock variable is useless because it gets
reassigned on every call; either remove it entirely or implement real reuse
keyed by credentials. Locate cachedBedrock and the factory
createBedrockAnthropic (and any similar cached variables around lines 312-335)
and choose one of two fixes: 1) delete cachedBedrock and any code that assumes
reuse so a fresh client is always created, or 2) implement a proper cache map
keyed by a stable fingerprint (e.g., authType + region +
accessKeyId/apiKey-fingerprint) and return the existing client when the key
matches; ensure creation only occurs when no cached entry exists and
clear/replace entries when credentials change.
In `@workspaces/mi/mi-visualizer/src/views/AIPanel/component/AICodeGenerator.tsx`:
- Around line 56-70: The useEffect's async helper checkByok should guard against
component unmount and surface RPC errors: wrap the Promise.all call to
rpcClient.getMiAiPanelRpcClient().hasAnthropicApiKey() and
rpcClient.getAIVisualizerState() in a try/catch and log/handle the error, and
add a cancelled flag (e.g., let cancelled = false; and set cancelled = true in
the effect cleanup) so that before calling setIsByok and setIsAwsBedrock you
check if (!cancelled) to avoid updating state after unmount; apply this change
inside the existing checkByok function referenced in the effect and ensure the
cleanup returns a function that flips the cancelled flag.
In
`@workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx`:
- Around line 606-616: In WaitingForLoginSection, replace the anchor element
that uses href="#" with a semantically correct control: either change it to a
<button> (styled to look like a link) and keep the existing onClick that calls
rpcClient.getMiVisualizerRpcClient().openExternal({ uri:
"https://app.tavily.com" }), or set the anchor's href to
"https://app.tavily.com" with target="_blank" and rel="noopener noreferrer" and
preserve the onClick for instrumentation; remove preventDefault() when using a
real href. Ensure the element still uses the same inline style color and that
keyboard/screen-reader semantics reflect a button if you choose the <button>
option.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46634b51-c94b-466f-b7c2-906f462b7d43
⛔ Files ignored due to path filters (1)
common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
workspaces/mi/mi-core/src/rpc-types/agent-mode/types.tsworkspaces/mi/mi-core/src/rpc-types/ai-features/index.tsworkspaces/mi/mi-core/src/rpc-types/ai-features/rpc-type.tsworkspaces/mi/mi-core/src/state-machine-types.tsworkspaces/mi/mi-extension/package.jsonworkspaces/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/stream_guard.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/tool_load.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-handler.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-rpc-client/src/rpc-clients/ai-features/rpc-client.tsworkspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/AICodeGenerator.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/ModelSettingsMenu.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/SettingsPanel.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsxworkspaces/mi/mi-visualizer/src/views/DisabledWindow/index.tsxworkspaces/mi/mi-visualizer/src/views/LoggedOutWindow/index.tsx
💤 Files with no reviewable changes (5)
- workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/tool_load.ts
- workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/types.ts
- workspaces/mi/mi-core/src/rpc-types/agent-mode/types.ts
- workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts
- workspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
workspaces/mi/mi-visualizer/src/views/AIPanel/component/AICodeGenerator.tsx (2)
184-184: Initial-render flash for Bedrock users.
isAwsBedrockdefaults tofalseand is only flipped totrueafter the asyncPromise.allresolves. If a Bedrock user opens Settings quickly (or the RPC is slow),SettingsPanelwill render once without the Tavily / web-search controls and then re-render with them once state resolves — a brief UI flicker.If the underlying login method is already known synchronously elsewhere (e.g., from
MICopilotContextor a cached visualizer state), consider seedingisAwsBedrockfrom it to avoid the flash. Otherwise,SettingsPanelcould render a small loading state while resolution is pending. Optional polish.🤖 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/AICodeGenerator.tsx` at line 184, The SettingsPanel currently receives isAwsBedrock which defaults to false and flips after async Promise.all causing a flash; seed that prop synchronously or show a loading state to avoid flicker: in the AICodeGenerator component, read the known login/provider state synchronously (e.g., from MICopilotContext or cached visualizer state) and initialize/set isAwsBedrock before rendering, falling back to a tri-state like undefined while async RPC resolves and pass that to SettingsPanel; alternatively, modify SettingsPanel to detect an undefined isAwsBedrock and render a small loading placeholder (or disable tavily/web-search controls) until the real boolean is available so the panel does not render a full false state then re-render.
56-81: LGTM — clean async-effect hardening.Adding the
cancelledguard plus the earlyrpcClientbail-out andtry/catcharound the combinedPromise.allis a solid pattern for this hook, and derivingisAwsBedrockfrom the same resolvedmachineViewavoids an extra RPC round-trip.One small thing worth keeping in mind (not a blocker): the effect deps are still
[rpcClient], so if the active login method changes during the session withoutrpcClientre-identifying (e.g., a user switches between Anthropic key and AWS Bedrock from settings without a panel remount),isByok/isAwsBedrockwill stay stale until the component remounts. If switching auth at runtime is supported, consider re-running this on a login-method change signal as well.🤖 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/AICodeGenerator.tsx` around lines 56 - 81, The effect only depends on rpcClient, so isByok/isAwsBedrock can go stale if the login method changes at runtime; update the hook to re-run when auth/login-method changes by either including a login-method signal in the deps or subscribing to rpcClient's state change event: inside the effect (or a new effect) subscribe to rpcClient.getAIVisualizerState() change (or an auth-change event) and call checkByok on updates, and make sure to unsubscribe/cleanup on return; reference functions/vars: useEffect, checkByok, rpcClient, setIsByok, setIsAwsBedrock, and LoginMethod.AWS_BEDROCK.workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx (2)
489-578: Consider extracting the password-input + eye-toggle pattern.The same
InputContainer/StyledTextField/EyeButtonblock is repeated four times in this section (Bedrock API key, AWS access key, AWS secret, AWS session token), and a fifth time for Tavily below. A smallPasswordFieldhelper that takesvalue,onChange,placeholder,visible,onToggleVisibility, anddisabledwould shrink this branch substantially and reduce the chance of one variant drifting (e.g. aneye-closedicon getting out of sync withvisible). Non-blocking — fine to defer.🤖 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/WaitingForLoginSection.tsx` around lines 489 - 578, Extract the repeated password-input + eye-toggle block into a small reusable PasswordField component (e.g., PasswordField) and replace the repeated JSX in WaitingForLoginSection with it; PasswordField should accept props: value, onChange, placeholder, visible, onToggleVisibility, disabled and internally render the StyledTextField + EyeButton semantics (including the Codicon icon switching between "eye" and "eye-closed"), then wire existing usages (bedrockApiKey/handleBedrockApiKeyChange/showBedrockApiKey, awsCredentials.*/handleAwsCredentialChange/showAwsAccessKey/showAwsSecretKey/showAwsSessionToken, and the Tavily field below) to use PasswordField and ensure you continue to pass {...(isValidating ? { disabled: true } : {})} (or the disabled boolean) to preserve current behavior.
436-439:hasRegionis a trimmed string, not a boolean.
awsCredentials.region.trim()returns a string, soisFormValidends up as a string-or-empty-string. JSXdisabledcoerces it correctly today, but downstream consumers (e.g. ifisFormValidis ever passed somewhere typedboolean) will trip on this. Coerce explicitly to keep intent clear and types honest.🧹 Suggested coercion
- const hasRegion = awsCredentials.region.trim(); - const isFormValid = bedrockAuthType === "api_key" - ? hasRegion && bedrockApiKey.trim() - : hasRegion && awsCredentials.accessKeyId.trim() && awsCredentials.secretAccessKey.trim(); + const hasRegion = awsCredentials.region.trim().length > 0; + const isFormValid = bedrockAuthType === "api_key" + ? hasRegion && bedrockApiKey.trim().length > 0 + : hasRegion + && awsCredentials.accessKeyId.trim().length > 0 + && awsCredentials.secretAccessKey.trim().length > 0;🤖 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/WaitingForLoginSection.tsx` around lines 436 - 439, The variable hasRegion is currently assigned a trimmed string; coerce it to a boolean to reflect intent and avoid type leaks: change the initialization of hasRegion to an explicit boolean (e.g., using Boolean(...) or !!...) applied to awsCredentials.region.trim(), then use that boolean in the existing isFormValid conditional (which references bedrockAuthType, bedrockApiKey, and awsCredentials.accessKeyId/secretAccessKey) so isFormValid becomes a true boolean rather than a string.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts (1)
598-601: Duplicate session-context snapshot per turn — consider sharing it withgetUserPrompt.
computeSessionContextBlockHashes(called fromagent.tsto computeblockStatuses) andgetUserPrompteach independently callbuildSessionContextSnapshot. That doubles the per-turn cost of every input it gathers —getAvailableConnectorCatalog(projectPath),getRuntimePaths(...)(multiplefs.existsSync), the.git/HEADread, andgetRuntimeVersionFromPom(...)whenparams.runtimeVersionisn't passed in. The connector catalog read in particular can be non-trivial on larger projects.A simple fix is to expose the snapshot alongside the hashes and let
getUserPromptaccept it (falling back to building one if absent). That also guarantees the hashes thatagent.tspersists exactly match the content rendered in the prompt — no risk of a drift between the two calls (e.g., agetAvailableConnectorCatalogcache flip, or HEAD changing) producing a stored hash that doesn't correspond to what the model actually saw.♻️ Sketch — return snapshot from the hashes builder and reuse in `getUserPrompt`
-export async function computeSessionContextBlockHashes(params: SessionContextParams): Promise<SessionContextBlockHashes> { - const { snapshot } = await buildSessionContextSnapshot(params); - return deriveBlockHashes(snapshot); -} +export interface SessionContextBuildResult { + snapshot: SessionContextSnapshot; + catalogWarnings: string[]; + catalogStoreStatus: string; + runtimeVersionResolved: string | null; + hashes: SessionContextBlockHashes; +} + +export async function buildSessionContext(params: SessionContextParams): Promise<SessionContextBuildResult> { + const ctx = await buildSessionContextSnapshot(params); + return { ...ctx, hashes: deriveBlockHashes(ctx.snapshot) }; +} + +export async function computeSessionContextBlockHashes(params: SessionContextParams): Promise<SessionContextBlockHashes> { + return (await buildSessionContext(params)).hashes; +}Then accept an optional pre-built
SessionContextBuildResultonUserPromptParamssoagent.tscan pass through the same instance it already used to computeblockStatuses.🤖 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 598 - 601, computeSessionContextBlockHashes currently calls buildSessionContextSnapshot separately from getUserPrompt, doubling expensive snapshot work; change the API so the snapshot is returned and reused: have computeSessionContextBlockHashes (or a new computeSessionContextBuildResult) return the SessionContextBuildResult (containing the snapshot) alongside the derived hashes (use buildSessionContextSnapshot -> deriveBlockHashes as before), then update UserPromptParams / getUserPrompt to accept an optional SessionContextBuildResult and fall back to calling buildSessionContextSnapshot when it's not provided; finally, in agent.ts pass the same SessionContextBuildResult used to compute blockStatuses into getUserPrompt so both use the identical snapshot and avoid duplicate work.
🤖 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/src/ai-features/agent-mode/agents/main/agent.ts`:
- Around line 214-225: The decideBlockStatus logic currently treats current ===
undefined as 'omit', which hides cleared optional blocks (like payloads); change
decideBlockStatus to return a distinct status (e.g., 'cleared' or 'removed')
when current is undefined so the caller can both persist the cleared state and
emit an explicit removal/update reminder to the model, and update the
corresponding handling in buildUpdatedBlocksState (and the similar branch around
the other occurrence) to persist a clearing marker (not skip persisting) and to
produce a removal reminder message for that block instead of skipping injection.
In
`@workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx`:
- Around line 580-623: The placeholder and helper text for the Tavily key are
inconsistent and can confuse users; update the placeholder on StyledTextField
(the tavilyApiKey input) and the HelperText copy to a single consistent phrase
such as "optional at login — required later for web_search / web_fetch on AWS
Bedrock; can be added in Settings" (or similar), ensuring you change the
placeholder prop on StyledTextField and the string inside HelperText's
button-adjacent copy so both tavilyApiKey placeholder and the HelperText message
match.
- Around line 450-475: The AuthModeButton toggles are missing ARIA state so
screen readers can't tell which is active; update each AuthModeButton (the two
instances that check bedrockAuthType) to include aria-pressed={selected} (i.e.,
aria-pressed={bedrockAuthType === "api_key"} and aria-pressed={bedrockAuthType
=== "iam"}) and keep existing onClick handlers (setBedrockAuthType and
setClientError) and disabled={isValidating}; alternatively wrap AuthModeSelector
with role="radiogroup" and make each AuthModeButton role="radio" with
aria-checked matching the selected expression, but prefer the simpler
aria-pressed addition on AuthModeButton for minimal changes.
---
Nitpick comments:
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts`:
- Around line 598-601: computeSessionContextBlockHashes currently calls
buildSessionContextSnapshot separately from getUserPrompt, doubling expensive
snapshot work; change the API so the snapshot is returned and reused: have
computeSessionContextBlockHashes (or a new computeSessionContextBuildResult)
return the SessionContextBuildResult (containing the snapshot) alongside the
derived hashes (use buildSessionContextSnapshot -> deriveBlockHashes as before),
then update UserPromptParams / getUserPrompt to accept an optional
SessionContextBuildResult and fall back to calling buildSessionContextSnapshot
when it's not provided; finally, in agent.ts pass the same
SessionContextBuildResult used to compute blockStatuses into getUserPrompt so
both use the identical snapshot and avoid duplicate work.
In `@workspaces/mi/mi-visualizer/src/views/AIPanel/component/AICodeGenerator.tsx`:
- Line 184: The SettingsPanel currently receives isAwsBedrock which defaults to
false and flips after async Promise.all causing a flash; seed that prop
synchronously or show a loading state to avoid flicker: in the AICodeGenerator
component, read the known login/provider state synchronously (e.g., from
MICopilotContext or cached visualizer state) and initialize/set isAwsBedrock
before rendering, falling back to a tri-state like undefined while async RPC
resolves and pass that to SettingsPanel; alternatively, modify SettingsPanel to
detect an undefined isAwsBedrock and render a small loading placeholder (or
disable tavily/web-search controls) until the real boolean is available so the
panel does not render a full false state then re-render.
- Around line 56-81: The effect only depends on rpcClient, so
isByok/isAwsBedrock can go stale if the login method changes at runtime; update
the hook to re-run when auth/login-method changes by either including a
login-method signal in the deps or subscribing to rpcClient's state change
event: inside the effect (or a new effect) subscribe to
rpcClient.getAIVisualizerState() change (or an auth-change event) and call
checkByok on updates, and make sure to unsubscribe/cleanup on return; reference
functions/vars: useEffect, checkByok, rpcClient, setIsByok, setIsAwsBedrock, and
LoginMethod.AWS_BEDROCK.
In
`@workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx`:
- Around line 489-578: Extract the repeated password-input + eye-toggle block
into a small reusable PasswordField component (e.g., PasswordField) and replace
the repeated JSX in WaitingForLoginSection with it; PasswordField should accept
props: value, onChange, placeholder, visible, onToggleVisibility, disabled and
internally render the StyledTextField + EyeButton semantics (including the
Codicon icon switching between "eye" and "eye-closed"), then wire existing
usages (bedrockApiKey/handleBedrockApiKeyChange/showBedrockApiKey,
awsCredentials.*/handleAwsCredentialChange/showAwsAccessKey/showAwsSecretKey/showAwsSessionToken,
and the Tavily field below) to use PasswordField and ensure you continue to pass
{...(isValidating ? { disabled: true } : {})} (or the disabled boolean) to
preserve current behavior.
- Around line 436-439: The variable hasRegion is currently assigned a trimmed
string; coerce it to a boolean to reflect intent and avoid type leaks: change
the initialization of hasRegion to an explicit boolean (e.g., using Boolean(...)
or !!...) applied to awsCredentials.region.trim(), then use that boolean in the
existing isFormValid conditional (which references bedrockAuthType,
bedrockApiKey, and awsCredentials.accessKeyId/secretAccessKey) so isFormValid
becomes a true boolean rather than a string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11f64d5c-af71-4b5c-82be-54b3a3cff9bb
📒 Files selected for processing (15)
workspaces/mi/mi-core/src/rpc-types/agent-mode/index.tsworkspaces/mi/mi-core/src/rpc-types/agent-mode/types.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/agent.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/mode.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/chat-history-manager.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/stream_guard.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/web_tools.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-visualizer/src/views/AIPanel/component/AICodeGenerator.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/SettingsPanel.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx
✅ Files skipped from review due to trivial changes (1)
- workspaces/mi/mi-core/src/rpc-types/agent-mode/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts
- workspaces/mi/mi-core/src/rpc-types/agent-mode/types.ts
- workspaces/mi/mi-visualizer/src/views/AIPanel/component/SettingsPanel.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx (1)
436-439: CoerceisFormValidto a real boolean.
awsCredentials.region.trim()(and the other.trim()calls) return strings, sohasRegionandisFormValidend up beingstringvalues, not booleans. The!isFormValidcheck at line 638 happens to work because""is falsy, but a variable calledisFormValidreading as typestringis misleading and brittle for any future consumer that expects a boolean (e.g. passing it directly into anaria-disabledordisabledboolean prop without negation).♻️ Proposed tweak
- const hasRegion = awsCredentials.region.trim(); - const isFormValid = bedrockAuthType === "api_key" - ? hasRegion && bedrockApiKey.trim() - : hasRegion && awsCredentials.accessKeyId.trim() && awsCredentials.secretAccessKey.trim(); + const hasRegion = awsCredentials.region.trim().length > 0; + const isFormValid = bedrockAuthType === "api_key" + ? hasRegion && bedrockApiKey.trim().length > 0 + : hasRegion + && awsCredentials.accessKeyId.trim().length > 0 + && awsCredentials.secretAccessKey.trim().length > 0;🤖 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/WaitingForLoginSection.tsx` around lines 436 - 439, The variables hasRegion and isFormValid are currently strings (result of .trim())—coerce them to booleans: change hasRegion to an explicit boolean (e.g., Boolean(awsCredentials.region.trim()) or awsCredentials.region.trim().length > 0) and build isFormValid using boolean expressions (e.g., bedrockAuthType === "api_key" ? hasRegion && Boolean(bedrockApiKey.trim()) : hasRegion && Boolean(awsCredentials.accessKeyId.trim()) && Boolean(awsCredentials.secretAccessKey.trim())); update references to hasRegion and isFormValid accordingly so they are true booleans for consumption by props like disabled or aria-disabled.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts (2)
837-845: Optional: replace the verbose inline type with the existingSessionContextWithCataloginterface.The inline anonymous type duplicates the
SessionContextWithCatalogshape exactly;SessionContextBuildResultis structurally assignable to it (the extrahashesfield doesn't hurt). Cleaner:♻️ Proposed refactor
- const sessionContext: { snapshot: SessionContextSnapshot; catalogWarnings: string[]; catalogStoreStatus: string; runtimeVersionResolved: string | null } = - params.precomputedContext ?? await buildSessionContextSnapshot({ + const sessionContext: SessionContextWithCatalog = + params.precomputedContext ?? await buildSessionContextSnapshot({ projectPath: params.projectPath, runtimeVersion: params.runtimeVersion, webSearchUnavailable: params.webSearchUnavailable, loginMethod: params.loginMethod, mode, });🤖 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 837 - 845, The inline anonymous type used for sessionContext duplicates an existing interface; change the declaration of sessionContext to use the SessionContextWithCatalog type instead of the verbose inline type and allow assignment from buildSessionContextSnapshot (which returns SessionContextBuildResult) — update the declaration referencing sessionContext and the call to buildSessionContextSnapshot so the value is typed as SessionContextWithCatalog (preserving the existing destructuring const { snapshot } = sessionContext).
521-549: Sync.tryout/scan runs on the hot per-turn path.
scanTryoutPayloadsdoesreaddirSync+ NstatSynccalls on every user turn. For typical projects (a handful of saved payloads) this is negligible, but a user who accumulates hundreds of.tryout/*.jsonwill pay synchronous stat I/O on the event loop on each turn. Consider switching tofs.promises.readdir({ withFileTypes: true })+Promise.all(stat)if you want to keep this future-proof; otherwise a simple cap on entries (e.g. first 200, sorted) would also bound the worst case.🤖 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 521 - 549, scanTryoutPayloads is doing synchronous readdirSync + statSync on every turn which can block the event loop for projects with many .tryout files; replace the sync path by using fs.promises.readdir(tryoutDir, { withFileTypes: true }) and then build TryoutPayloadEntry objects using Promise.all to stat only files (or derive size/mtime from Dirent if possible), or if you prefer a simpler change, limit the processed entries after sorting (e.g., take first 200) before calling statSync to bound worst-case cost; update the function scanTryoutPayloads to use the async/promise APIs or add the entry cap and ensure callers handle the async change if you make it non-blocking.
🤖 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-visualizer/src/views/AIPanel/component/SettingsPanel.tsx`:
- Around line 115-149: Concurrent saves and toggle-off races between
handleTavilySave and handleBedrockWebSearchToggle cause last-wins inconsistency;
add a simple serialization/lock for Tavily mutations (e.g., a module-level
Promise or boolean lock named tavilyMutationLock or isTavilyMutating) and have
both handleTavilySave and handleBedrockWebSearchToggle await/acquire the lock
before performing RPCs (rpcClient.getMiAiPanelRpcClient().setTavilyApiKey) and
release it after finishing, updating UI state via
setTavilyStatus/setTavilyKey/setTavilyInputOpen/setTavilyDraft only after the
operation completes; alternatively disable the toggle while a save is in flight
by checking the same isTavilyMutating flag so only one mutation can proceed at a
time, ensuring the final RPC outcome deterministically wins.
---
Nitpick comments:
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/prompt.ts`:
- Around line 837-845: The inline anonymous type used for sessionContext
duplicates an existing interface; change the declaration of sessionContext to
use the SessionContextWithCatalog type instead of the verbose inline type and
allow assignment from buildSessionContextSnapshot (which returns
SessionContextBuildResult) — update the declaration referencing sessionContext
and the call to buildSessionContextSnapshot so the value is typed as
SessionContextWithCatalog (preserving the existing destructuring const {
snapshot } = sessionContext).
- Around line 521-549: scanTryoutPayloads is doing synchronous readdirSync +
statSync on every turn which can block the event loop for projects with many
.tryout files; replace the sync path by using fs.promises.readdir(tryoutDir, {
withFileTypes: true }) and then build TryoutPayloadEntry objects using
Promise.all to stat only files (or derive size/mtime from Dirent if possible),
or if you prefer a simpler change, limit the processed entries after sorting
(e.g., take first 200) before calling statSync to bound worst-case cost; update
the function scanTryoutPayloads to use the async/promise APIs or add the entry
cap and ensure callers handle the async change if you make it non-blocking.
In
`@workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx`:
- Around line 436-439: The variables hasRegion and isFormValid are currently
strings (result of .trim())—coerce them to booleans: change hasRegion to an
explicit boolean (e.g., Boolean(awsCredentials.region.trim()) or
awsCredentials.region.trim().length > 0) and build isFormValid using boolean
expressions (e.g., bedrockAuthType === "api_key" ? hasRegion &&
Boolean(bedrockApiKey.trim()) : hasRegion &&
Boolean(awsCredentials.accessKeyId.trim()) &&
Boolean(awsCredentials.secretAccessKey.trim())); update references to hasRegion
and isFormValid accordingly so they are true booleans for consumption by props
like disabled or aria-disabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ac036b9-bf00-47e4-b2a3-c0e82daa93d8
📒 Files selected for processing (12)
workspaces/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/context/synapse-core/synapse_artifact_reference.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse-core/synapse_property_reference.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse-core/synapse_registry_resource_guide.tsworkspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.tsworkspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatFooter.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/MICopilotContext.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/ModelSettingsMenu.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/SettingsPanel.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx
💤 Files with no reviewable changes (2)
- workspaces/mi/mi-extension/src/rpc-managers/agent-mode/rpc-manager.ts
- workspaces/mi/mi-visualizer/src/views/AIPanel/component/ModelSettingsMenu.tsx
✅ Files skipped from review due to trivial changes (3)
- workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse-core/synapse_property_reference.ts
- workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse-core/synapse_registry_resource_guide.ts
- workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse-core/synapse_artifact_reference.ts
Contributor
|
Have we evaluated Tavily? Bedrock users might be ok with using something from AWS instead. They seem to have similar offerings. |
Contributor
Author
Tavily Enterprise is there in AWS marketplace. Users can get an API key through AWS if want and use in our login. It works as it is. Will add a link in login page |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-visualizer/src/views/AIPanel/component/SettingsPanel.tsx`:
- Around line 142-149: The disable branch unconditionally calls
rpcClient.getMiAiPanelRpcClient().setTavilyApiKey({ apiKey: '' }) even when no
Tavily key was ever saved; add a guard so you only perform the RPC clear when
there is a saved key to remove (e.g., check the current saved key state or
tavilyDraft/savedKey variable) before invoking setTavilyApiKey. Update the code
around tavilyMutationLock.current, setTavilyInputOpen, setTavilyDraft,
setTavilyStatus and the call to
rpcClient.getMiAiPanelRpcClient().setTavilyApiKey to bail out early (skip the
RPC and set status appropriately) when there is nothing to clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2398d23-4972-4434-b8a6-cd1a349e2e4f
📒 Files selected for processing (3)
workspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatHeader.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/SettingsPanel.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx
Contributor
There was a problem hiding this comment.
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/agent-mode/tools/file_tools.ts (1)
886-894:⚠️ Potential issue | 🟡 MinorDifferentiate truly empty files from “sanitized to empty.”
At Line 886 + Line 889, a file containing only ANSI/control bytes is reported as empty. That can mislead troubleshooting and agent reasoning.
Suggested patch
- const content = stripAnsiAndControl(fs.readFileSync(fullPath, 'utf-8')); + const rawContent = fs.readFileSync(fullPath, 'utf-8'); + const content = stripAnsiAndControl(rawContent); // Handle empty file if (content.trim().length === 0) { logDebug(`[FileReadTool] File is empty: ${file_path}`); return { success: true, - message: `File '${file_path}' is empty.`, + message: rawContent.length === 0 + ? `File '${file_path}' is empty.` + : `File '${file_path}' contains only ANSI/control characters after sanitization.`, }; }🤖 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 886 - 894, The code currently treats any file whose sanitized content (via stripAnsiAndControl) is empty as "empty", which hides cases where the raw file contained only ANSI/control bytes; update the FileReadTool logic to read the raw file into a variable (e.g. rawContent = fs.readFileSync(fullPath, 'utf-8')), compute sanitizedContent = stripAnsiAndControl(rawContent), then branch: if rawContent.trim().length === 0 treat as truly empty and return/log that, else if sanitizedContent.trim().length === 0 treat as "sanitized to empty" and return/log a distinct message indicating the file contained only ANSI/control characters; reference the existing symbols stripAnsiAndControl, file_path and the FileReadTool handling to locate and change the checks and messages.
🧹 Nitpick comments (4)
workspaces/mi/mi-visualizer/src/views/AIPanel/component/SettingsPanel.tsx (1)
333-479: Minor: Unnecessary fragment wrapper.The
<>...</>fragment wrapping the content is redundant sinceSettingsSectionalready acceptschildrenasReact.ReactNode. You can remove the fragment tags for cleaner JSX.♻️ Suggested simplification
<SettingsSection title="Web Search"> - <> {/* Bedrock: enable toggle (gated on Tavily key). */} ... - </> </SettingsSection>🤖 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/SettingsPanel.tsx` around lines 333 - 479, The JSX inside the SettingsSection titled "Web Search" is wrapped in an unnecessary fragment (<>...</>); remove the fragment so the children are passed directly to SettingsSection (i.e., delete the opening <> and closing </> around the block containing the Bedrock toggle, tavily inputs, status messages, and related buttons) while keeping all inner elements, handlers (handleBedrockWebSearchToggle, handleTavilySave, handleEditTavilyKey, handleOpenTavilySignup, handleOpenTavilyMarketplace), and state references (tavilyInputOpen, tavilyKey, tavilyStatus, tavilyDraft, tavilyDirty, isBedrockWebSearchOn, showTavilyKey) intact to preserve behavior.workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide.ts (1)
400-405: De-duplicate tool and section identifiers in this prompt block.
generate_data_mappingand the section list are hardcoded here, which can drift fromtools/types.tsandcontext/data_mapper_reference.ts.♻️ Proposed refactor
-import { CREATE_DATA_MAPPER_TOOL_NAME } from "../tools/types"; +import { CREATE_DATA_MAPPER_TOOL_NAME, GENERATE_DATA_MAPPING_TOOL_NAME } from "../tools/types"; +import { DATA_MAPPER_REFERENCE_SECTIONS } from "./data_mapper_reference"; import { SYNAPSE_EXPRESSION_GUIDE } from "./synapse_expression_guide" @@ - New mapper → use \`${CREATE_DATA_MAPPER_TOOL_NAME}\` (scaffolds folder, \`.ts\` file, and \`dm-utils.ts\`). -- Generate / fill the \`mapFunction\` body → use \`generate_data_mapping\`. +- Generate / fill the \`mapFunction\` body → use \`${GENERATE_DATA_MAPPING_TOOL_NAME}\`. - Direct \`file_edit\` on the \`.ts\` file is only for targeted single-field tweaks, user-dictated formula changes, or fixing a TS2556 spread error. @@ -**Before editing an existing \`.ts\` mapping file**, load \`data-mapper-reference\` via \`load_context_reference\` for the dmUtils API, the TS2556 dynamic-array spread rule (use \`arr.reduce(...)\`, never \`dmUtils.sum(...arr)\`), and the file format. Sections: \`overview\`, \`typescript_rules\`, \`dmutils_functions\`, \`dynamic_arrays\`, \`when_to_use_dmutils\`, \`array_handling\`, \`tool_usage\`. +**Before editing an existing \`.ts\` mapping file**, load \`data-mapper-reference\` via \`load_context_reference\` for the dmUtils API, the TS2556 dynamic-array spread rule (use \`arr.reduce(...)\`, never \`dmUtils.sum(...arr)\`), and the file format. Sections: \`${Object.keys(DATA_MAPPER_REFERENCE_SECTIONS).join('`, `')}\`.🤖 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/context/synapse_guide.ts` around lines 400 - 405, The prompt block hardcodes tool and section identifiers (e.g., generate_data_mapping, CREATE_DATA_MAPPER_TOOL_NAME, the section list and data-mapper-reference), causing drift; update synapse_guide.ts to derive these identifiers from the canonical sources instead of literals by importing or loading the authoritative definitions (tools/types.ts for tool names like CREATE_DATA_MAPPER_TOOL_NAME and context/data_mapper_reference.ts via load_context_reference for the section list and dmUtils API), replace hardcoded mentions of generate_data_mapping and data-mapper-reference with the imported constants/values, and update guidance to reference dm-utils.ts, dmUtils, file_edit, TS2556, and the arr.reduce vs dmUtils.sum rule dynamically so the prompt stays synchronized with the single source of truth.workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts (1)
301-301: Use the tool-name constant here to prevent prompt drift.This line hardcodes
generate_data_mapping; interpolatingGENERATE_DATA_MAPPING_TOOL_NAMEkeeps it aligned with the canonical name intools/types.ts.♻️ Proposed fix
import { @@ CREATE_DATA_MAPPER_TOOL_NAME, + GENERATE_DATA_MAPPING_TOOL_NAME, @@ -- \`data-mapper-reference\` [overview, typescript_rules, dmutils_functions, dynamic_arrays, when_to_use_dmutils, array_handling, tool_usage] — TypeScript data mapper \`.ts\` file format, dmUtils helper signatures, **TS2556 dynamic-array spread pitfall** (\`dmUtils.sum(...arr)\` fails — use \`arr.reduce(...)\`), array handling patterns. Load BEFORE editing an existing \`.ts\` mapping file. Generation should still go through \`generate_data_mapping\`. Requires MI runtime 4.4.0+ +- \`data-mapper-reference\` [overview, typescript_rules, dmutils_functions, dynamic_arrays, when_to_use_dmutils, array_handling, tool_usage] — TypeScript data mapper \`.ts\` file format, dmUtils helper signatures, **TS2556 dynamic-array spread pitfall** (\`dmUtils.sum(...arr)\` fails — use \`arr.reduce(...)\`), array handling patterns. Load BEFORE editing an existing \`.ts\` mapping file. Generation should still go through \`${GENERATE_DATA_MAPPING_TOOL_NAME}\`. Requires MI runtime 4.4.0+🤖 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` at line 301, Replace the hardcoded tool name string "generate_data_mapping" with the canonical constant GENERATE_DATA_MAPPING_TOOL_NAME (imported from tools/types.ts) to avoid prompt drift; locate the string in the template text (the line containing "generate_data_mapping") and substitute it with the constant, ensuring the constant is imported at the top of system.ts and used where the tool name is interpolated.workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/context_tools.ts (1)
204-211: Broaden the runtime-gate note to avoid stale guidance.This adds a second runtime-gated context, but the tool description still singles out only the AI connector context. Consider making that note generic so it stays accurate as gated contexts grow.
♻️ Suggested wording update
export function createContextTool(execute: ContextExecuteFn) { return (tool as any)({ description: `Loads deep reference context on demand to avoid prompt bloat. Use context_name in the form "topic" or "topic:section". Example: "synapse-expression-spec:type_coercion". - Note: AI connector context requires MI runtime 4.4.0 or newer.`, + Note: Some contexts require a minimum MI runtime version (for example, 4.4.0+).`, inputSchema: contextInputSchema, execute, }); }🤖 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/context_tools.ts` around lines 204 - 211, The tool entry named 'data-mapper-reference' currently mentions a specific "AI connector" runtime-gated context in its description, which will become stale as more runtime-gated contexts are added; update the description string (the description property of the 'data-mapper-reference' object) to replace the AI-connector-specific note with a generic runtime-gated warning (e.g., "This content is runtime-gated and requires the MI runtime specified by minRuntimeVersion") and optionally reference minRuntimeVersion (RUNTIME_VERSION_440) so the guidance stays correct as other gated contexts are introduced; keep all other fields (content: DATA_MAPPER_REFERENCE_FULL, sections: DATA_MAPPER_REFERENCE_SECTIONS, aliases) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.ts`:
- Around line 886-894: The code currently treats any file whose sanitized
content (via stripAnsiAndControl) is empty as "empty", which hides cases where
the raw file contained only ANSI/control bytes; update the FileReadTool logic to
read the raw file into a variable (e.g. rawContent = fs.readFileSync(fullPath,
'utf-8')), compute sanitizedContent = stripAnsiAndControl(rawContent), then
branch: if rawContent.trim().length === 0 treat as truly empty and return/log
that, else if sanitizedContent.trim().length === 0 treat as "sanitized to empty"
and return/log a distinct message indicating the file contained only
ANSI/control characters; reference the existing symbols stripAnsiAndControl,
file_path and the FileReadTool handling to locate and change the checks and
messages.
---
Nitpick comments:
In `@workspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.ts`:
- Line 301: Replace the hardcoded tool name string "generate_data_mapping" with
the canonical constant GENERATE_DATA_MAPPING_TOOL_NAME (imported from
tools/types.ts) to avoid prompt drift; locate the string in the template text
(the line containing "generate_data_mapping") and substitute it with the
constant, ensuring the constant is imported at the top of system.ts and used
where the tool name is interpolated.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide.ts`:
- Around line 400-405: The prompt block hardcodes tool and section identifiers
(e.g., generate_data_mapping, CREATE_DATA_MAPPER_TOOL_NAME, the section list and
data-mapper-reference), causing drift; update synapse_guide.ts to derive these
identifiers from the canonical sources instead of literals by importing or
loading the authoritative definitions (tools/types.ts for tool names like
CREATE_DATA_MAPPER_TOOL_NAME and context/data_mapper_reference.ts via
load_context_reference for the section list and dmUtils API), replace hardcoded
mentions of generate_data_mapping and data-mapper-reference with the imported
constants/values, and update guidance to reference dm-utils.ts, dmUtils,
file_edit, TS2556, and the arr.reduce vs dmUtils.sum rule dynamically so the
prompt stays synchronized with the single source of truth.
In
`@workspaces/mi/mi-extension/src/ai-features/agent-mode/tools/context_tools.ts`:
- Around line 204-211: The tool entry named 'data-mapper-reference' currently
mentions a specific "AI connector" runtime-gated context in its description,
which will become stale as more runtime-gated contexts are added; update the
description string (the description property of the 'data-mapper-reference'
object) to replace the AI-connector-specific note with a generic runtime-gated
warning (e.g., "This content is runtime-gated and requires the MI runtime
specified by minRuntimeVersion") and optionally reference minRuntimeVersion
(RUNTIME_VERSION_440) so the guidance stays correct as other gated contexts are
introduced; keep all other fields (content: DATA_MAPPER_REFERENCE_FULL,
sections: DATA_MAPPER_REFERENCE_SECTIONS, aliases) unchanged.
In `@workspaces/mi/mi-visualizer/src/views/AIPanel/component/SettingsPanel.tsx`:
- Around line 333-479: The JSX inside the SettingsSection titled "Web Search" is
wrapped in an unnecessary fragment (<>...</>); remove the fragment so the
children are passed directly to SettingsSection (i.e., delete the opening <> and
closing </> around the block containing the Bedrock toggle, tavily inputs,
status messages, and related buttons) while keeping all inner elements, handlers
(handleBedrockWebSearchToggle, handleTavilySave, handleEditTavilyKey,
handleOpenTavilySignup, handleOpenTavilyMarketplace), and state references
(tavilyInputOpen, tavilyKey, tavilyStatus, tavilyDraft, tavilyDirty,
isBedrockWebSearchOn, showTavilyKey) intact to preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 995d9c3f-ba08-401e-98bd-73da80c4b4bc
⛔ Files ignored due to path filters (2)
workspaces/mi/mi-extension/resources/icons/dark-ai-chat.svgis excluded by!**/*.svgworkspaces/mi/mi-extension/resources/icons/light-ai-chat.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
workspaces/mi/mi-data-mapper/src/components/DataMapper/Header/AIMapButton.tsxworkspaces/mi/mi-diagram/src/components/Form/AIAutoFillBox/AIAutoFillBox.tsxworkspaces/mi/mi-diagram/src/components/Form/FormExpressionField/index.tsxworkspaces/mi/mi-extension/package.jsonworkspaces/mi/mi-extension/src/ai-features/agent-mode/agents/data-mapper/system.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/agents/main/system.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/chat-history-manager.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/data_mapper_reference.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/context/synapse_guide.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/bash_tools.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/context_tools.tsworkspaces/mi/mi-extension/src/ai-features/agent-mode/tools/file_tools.tsworkspaces/mi/mi-extension/src/ai-features/utils/sanitize-text.tsworkspaces/mi/mi-visualizer/src/views/AIPanel/component/SettingsPanel.tsxworkspaces/mi/mi-visualizer/src/views/AddArtifact/index.tsxworkspaces/mi/mi-visualizer/src/views/Forms/IDPConnectorForm/IdpHeaderSchemaGeneration.tsxworkspaces/mi/mi-visualizer/src/views/Forms/IDPConnectorForm/IdpHeaderTryout.tsxworkspaces/mi/mi-visualizer/src/views/Forms/IDPConnectorForm/InitialTryOutView.tsxworkspaces/mi/mi-visualizer/src/views/Forms/Tests/TestCaseForm.tsxworkspaces/mi/mi-visualizer/src/views/Forms/Tests/TestSuiteForm.tsx
✅ Files skipped from review due to trivial changes (10)
- workspaces/mi/mi-visualizer/src/views/Forms/Tests/TestSuiteForm.tsx
- workspaces/mi/mi-diagram/src/components/Form/AIAutoFillBox/AIAutoFillBox.tsx
- workspaces/mi/mi-extension/src/ai-features/utils/sanitize-text.ts
- workspaces/mi/mi-data-mapper/src/components/DataMapper/Header/AIMapButton.tsx
- workspaces/mi/mi-extension/src/ai-features/agent-mode/context/data_mapper_reference.ts
- workspaces/mi/mi-visualizer/src/views/AddArtifact/index.tsx
- workspaces/mi/mi-visualizer/src/views/Forms/IDPConnectorForm/IdpHeaderSchemaGeneration.tsx
- workspaces/mi/mi-visualizer/src/views/Forms/Tests/TestCaseForm.tsx
- workspaces/mi/mi-visualizer/src/views/Forms/IDPConnectorForm/IdpHeaderTryout.tsx
- workspaces/mi/mi-visualizer/src/views/Forms/IDPConnectorForm/InitialTryOutView.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- workspaces/mi/mi-extension/src/ai-features/agent-mode/chat-history-manager.ts
- Support Bedrock API-key auth alongside IAM credentials, with a mode selector in the AWS sign-in UI and discriminated AwsBedrockSecrets type. - Switch all Bedrock model IDs to the global. inference-profile prefix so the extension works in every Bedrock-enabled region (region-pinned profiles aren't published universally for Sonnet/Opus/Haiku). - Fix Sonnet 4.6, Opus 4.6, and Haiku 4.5 base IDs to match what list-inference-profiles actually publishes. - Reuse a single getBedrockValidationModelId helper so validation and runtime resolution can never drift apart. - Surface the underlying AWS error in validation failures instead of a generic "validation failed" message. - Persist an explicit-logout flag so the silent platform bootstrap and checkToken don't resurrect credentials after the user signs out. - Tighten sign-out copy in the diagram menu and settings panel to clarify that only MI Copilot credentials are cleared.
Use @ai-sdk/amazon-bedrock/anthropic (InvokeModel) instead of the default Converse-based provider. Converse silently drops Anthropic-only features the agent depends on (native compaction, deferLoading, anthropic-beta headers, adaptive thinking, ttl-based cache control); InvokeModel passes them through. Bedrock InvokeModel rejects defer_loading on type:"custom" tools unless the tool-search beta is set. The SDK auto-adds it only when a server-side tool_search tool is in the tools array, so we add it manually on Bedrock to keep the local load_tools deferred-loading optimisation working. Bump @ai-sdk/amazon-bedrock 4.0.83 → 4.0.96 and @ai-sdk/anthropic 3.0.64 → 3.0.71 to pick up the bedrock-anthropic provider fixes (tool-search beta in 4.0.84, error-shape transform in 4.0.86, reasoning-related fixes in 4.0.91/4.0.96). Surface AI SDK APICallError fields (statusCode, url, responseBody, data) in getErrorDiagnostics and dump the raw stream-error payload in agent.ts so future provider 4xx failures aren't silently swallowed.
AWS Bedrock has no first-party web tools, so web_search and web_fetch fail silently for Bedrock users today. Wire a BYOK Tavily integration as the Bedrock fallback, and lift the web-search controls out of the chat footer into the AI Panel settings. Backend - Extend AwsBedrockSecrets and SUBMIT_AWS_CREDENTIALS payloads with an optional tavilyApiKey, plumb it through validateAwsCredentials. - Add getTavilyApiKey() / setTavilyApiKey() helpers on auth.ts and expose them via new mi-ai-panel RPC methods so Settings can update the key without re-running AWS login. - web_tools.ts: createWebSearchExecute / createWebFetchExecute now branch via resolveBedrockTavilyGate(). MI_INTEL/ANTHROPIC_KEY users keep the Anthropic server-side path. Bedrock with Tavily key calls @tavily/core's search/extract directly. Bedrock without a key returns WEB_SEARCH_NOT_CONFIGURED / WEB_FETCH_NOT_CONFIGURED before any approval modal, so users aren't asked to approve calls that can't run. - agent.ts + prompt.ts: when Bedrock has no Tavily key, inject a one-shot <system-reminder> via the existing includeSessionContext gate so the model stops attempting web tools and points the user to Settings. Reuses the connector-catalog injection pattern (re-fired after compaction). UI - Move the web-search toggle out of AIChatFooter; the footer now reads webAccessPreapproved fresh from localStorage at submit time via a shared helper in utils.ts. - WaitingForLoginSection: optional Tavily API key field on the AWS form for both api_key and IAM auth, with a help link to app.tavily.com. - SettingsPanel: new "Web Search" section. For Anthropic/MI_INTEL users it's a single approval-skip toggle. For Bedrock users it's an enable toggle that exposes a Tavily key input on first activation, plus the approval-skip toggle (gated on web search being on). - AICodeGenerator: split the Bedrock-only flag into its own isAwsBedrock state (kept distinct from isByok which stays "user pays per request"), loaded in parallel with the existing hasAnthropicApiKey check. Use @tavily/core (CJS-friendly) rather than @tavily/ai-sdk (ESM-only) so the package resolves under the mi-extension webpack build.
The local web_search/web_fetch wrapper was driving Anthropic's first-party server tools through an inner generateText call, costing two LLM round-trips per web call on the Proxy/BYOK paths. Register the server tools (webSearch_20250305, webFetch_20250910 with citations + maxContentTokens) directly on the main streamText so Anthropic runs them inline, then keep a Tavily-backed local tool only for AWS Bedrock with a configured Tavily key. Bedrock without a key gets stub tools that surface WEB_SEARCH/FETCH_NOT_CONFIGURED if the model ignores the existing web_search_unavailable system reminder. Also remove the per-call web approval flow (UI toggle, RPC field, plan approval kinds, localStorage preference, Settings panel branch), make the agent backend-aware via a new "Copilot backends" section in system.ts and a "Copilot backend: ..." line in the env block, and drop the web tools from DEFERRED_TOOLS since Anthropic server tools must be registered up-front.
Updates the Anthropic constant, the Bedrock inference-profile id, and the user-facing model labels in ModelSettingsMenu and SettingsPanel.
Previously env, connectors, web availability, mode policy and payloads were injected only on the first message and after compaction. Now each block is hashed, persisted on session metadata, and re-injected with a [context updated] notice when its hash drifts (branch switch, date rollover, runtime version change, mode switch, payloads change, Tavily key add/remove). Also pre-resolves preconfigured payloads server-side in the agent RPC manager and adds an always-rendered brief Plan-mode note so the turn-ending rule stays visible even when the full policy is gated.
…harden Bedrock validation Security / log-sanitization - agent.ts: drop the raw JSON.stringify(part.error) dump; the structured getErrorDiagnostics already covers the safe fields and a raw dump risks leaking requestBodyValues / unsanitized headers at any log level. - stream_guard.ts: truncate APICallError.data to 2KB and whitelist responseHeaders to known-safe keys (rate-limit, content-type, request-id, retry-after) so set-cookie / x-amz-security-token / auth echoes never reach logs. Web tools - web_tools.ts: enforce allowed_domains / blocked_domains client-side in createWebFetchExecute (subdomain-aware) — Tavily Extract takes a single URL so the lists were previously a no-op. - web_tools.ts: cap runTavilyExtract rawContent at 128k chars (~32k tokens, matches Anthropic webFetch maxContentTokens) with a clear [CONTENT TRUNCATED] marker. - system.ts: drop the stale "Both require user approval" line for web_search / web_fetch — the per-call approval flow was removed. Bedrock - auth.ts: validateBedrockRegion now rejects GovCloud (us-gov-*) and China (cn-*) partitions with a descriptive error — the global. Bedrock inference profile we use isn't published there and would silently fail with "model not found" at runtime. - connection.ts: drop the misleading module-level cachedBedrock variable; it was reassigned on every call (not a real cache). UI / accessibility - SettingsPanel.tsx: handleBedrockWebSearchToggle no-ops when the requested toggle state already matches the current state, so a re-click of "On" no longer re-opens the Tavily input. - SettingsPanel.tsx: aria-label="Tavily API key" on the key input. - AICodeGenerator.tsx: wrap checkByok RPCs in try/catch and gate setState behind a cancelled flag in the effect cleanup. - WaitingForLoginSection.tsx: replace the href="#" + preventDefault Tavily link with a styled <button> for proper keyboard / a11y semantics.
Block injection
- Add 'cleared' to BlockInjectionStatus. decideBlockStatus now returns it
when a block was injected on a prior turn but is absent now (e.g. user
removed payloads); buildUpdatedBlocksState wipes the persisted hash so
the next non-empty injection counts as 'first-injection' rather than
're-injection'. buildPayloadsBlockText renders an explicit
"Preconfigured Values [removed]" notice so the model doesn't keep
referencing the stale prior-turn block. Other builders treat 'cleared'
defensively (their hashes are always defined, so 'cleared' is
unreachable in practice).
Snapshot dedup
- computeSessionContextBlockHashes now returns a SessionContextBuildResult
(hashes + snapshot + catalog metadata). agent.ts passes it through
UserPromptParams.precomputedContext so getUserPrompt skips the second
buildSessionContextSnapshot call — avoids the duplicate .git/HEAD read,
pom.xml read, and connector-store catalog lookup per turn.
Login form a11y / UX
- WaitingForLoginSection: aria-pressed on the Bedrock auth-mode buttons
so screen readers announce which mode is active.
- Align Tavily key placeholder + helper-text copy ("optional at login;
needed later for web_search/web_fetch on AWS Bedrock") so the two no
longer contradict each other (placeholder said "optional", helper said
"required").
…uidance
Tryout payloads
- Drop the inline JSON dump of `.tryout/*.json` from the user prompt.
prompt.ts now scans `.tryout/` directly (mtimeMs+size per file, sorted
by name) and surfaces only the file listing in a tracked
<system-reminder>. The model reads the relevant file via file_read on
demand. Saves tokens on every turn (especially with large saved
payloads) and removes the diagram-RPC pre-fetch.
- Block-level hash is over the listing — adding/removing/modifying any
file flips the hash and triggers re-injection; wiping the folder
triggers the new 'cleared' notice. Tradeoff: false-positive on
touch-without-content-change is one extra reminder; false-negative
needs a tool that preserves mtime (cp -p / git checkout of identical
content) — both fine for this use case.
- Drop the `payloads` field from AgentRequest, UserPromptParams, and
SessionContextParams; drop resolvePreconfiguredPayloads() in the
agent-mode RPC manager; drop the now-dead canonicalizePayloads helper.
- Add a brief "Tryout payloads" section to system.ts covering the file
shape (API vs other artifact), the defaultRequest pick rule, and when
to read.
Thinking behavior
- Rewrite the Thinking-behavior section to own the philosophy (when
thinking helps vs hurts) without restating the tool catalog already
covered in Tool usage policy / Validation / Build and Test / DeepWiki
/ Debugging workflow. Calls out the Synapse-specific failure mode:
trying to reason through every quirk upfront is wasted time because
the quirks aren't visible from source/docs alone. Correct loop is
rough mental model → implement → refine via available signals.
- Soften the Design Guidelines bullet ("Plan before implementing...")
to "Sketch the artifact list... refine as you implement, per the loop
in Thinking behavior" — removes the contradiction with the new
thinking guidance.
… Opus reasoning
Thinking-block UI bug
- agent.ts: hoist reasoningById + new flushOpenThinkingBlocks() above the
try block, and call it at all three terminal stream paths (finish,
unexpected end, top of catch). Previously the UI's
<thinking data-loading="true"> placeholder stayed open forever
whenever Anthropic skipped reasoning-end (Opus omitted-mode blocks,
stream cuts, mid-thought aborts).
- AIChatFooter.tsx: drop the fragile
appendThinkingPlaceholder(...).replace("</thinking>", …) fallback in
appendThinkingDelta — .replace matched the FIRST </thinking> in the
whole content, so a delta arriving without its start (panel reconnect /
event replay) injected into a prior finalized block instead of the new
one. Replaced with a direct string assembly that can't pick the wrong
block.
Adaptive thinking on by default
- Pass display: 'summarized' alongside thinking adaptive/low so Opus 4.7
actually surfaces reasoning text (its default flipped to 'omitted',
which is why the UI was showing empty thinking blocks). No-op on
Sonnet which already defaults to summarized.
- MICopilotContext.tsx: change the localStorage default from false to
true. Stored 'true'/'false' is still respected, only fresh installs
(null) flip to on — existing users keep their explicit preference.
- SettingsPanel.tsx: rename "Extended Thinking" → "Adaptive Thinking";
flip handleResetDefaults / isDefault to expect ON; replace the
warning with an info note ("Disable if the agent overthinks or feels
too slow on simple requests"). Recommendation now matches the
system-prompt guidance shipped earlier — adaptive + low effort + the
"rough mental model → implement → refine via signals" loop self-
regulates well, so default-on is appropriate.
- system.ts: rename the "Thinking behavior" section's opening line to
reflect adaptive being on by default.
Cleanup
- Delete ModelSettingsMenu.tsx — dead code, replaced by SettingsPanel
per CLAUDE.md decision wso2#87. No importers anywhere.
… AWS Marketplace link
Maven build logs read via file_read embed ANSI color codes (ESC + [...m). The raw 0x1b bytes leak through to the wire body and the Copilot proxy rejects the request with `unexpected control character in string`. Sanitize at file_read, shell stdout/stderr capture, and on history load so existing sessions recover without manual JSONL edits.
Previously the dmUtils API and TS2556 dynamic-array spread rule lived only
in the data mapper sub-agent's system prompt, so the main agent had no
detailed guidance when editing existing .ts mapping files via file_edit.
Move the shared knowledge (dmUtils signatures, TS2556 reduce-vs-spread
rule, TypeScript rules, array patterns, tool-routing) into a single
data_mapper_reference module. The sub-agent imports the same sections it
used to inline, and the main agent can load it on demand via
load_context_reference("data-mapper-reference"). Also fixes the
misleading dmUtils.average(...input.scores) example that was teaching
the model the exact TS2556-triggering pattern reported in the bug.
- file_read: distinguish truly empty files from files sanitized to empty (only ANSI/control bytes), so users see why no content is returned. - Replace hardcoded "generate_data_mapping" with GENERATE_DATA_MAPPING_TOOL_NAME in system.ts and synapse_guide.ts to prevent prompt drift. - load_context_reference description: replace AI-connector-specific note with a generic runtime-gated note referencing RUNTIME_VERSION_440, since data-mapper-reference is now also runtime-gated. - SettingsPanel: drop the unnecessary fragment wrapping the Web Search section's children inside SettingsSection.
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.
Summary
@ai-sdk/amazon-bedrock/anthropicinstead of the default Converse-based provider. Converse silently drops Anthropic-only features the agent depends on (native compaction, deferLoading, anthropic-beta headers, adaptive thinking, ttl-based cache control); InvokeModel passes them through. Manually adds the tool-search beta on Bedrock soload_toolsdeferred-loading keeps working withtype:"custom"tools.web_search/web_fetchfailed silently for Bedrock users. Wire a BYOK Tavily integration via@tavily/core(CJS-friendly;@tavily/ai-sdkis ESM-only and won't resolve in our webpack bundle). Without a Tavily key the tools returnWEB_SEARCH/FETCH_NOT_CONFIGUREDand a one-shot<system-reminder>steers the model away.web_search/web_fetchwrapper was driving Anthropic's first-party server tools through an innergenerateTextcall, costing two LLM round-trips per web call on Proxy/BYOK. RegisterwebSearch_20250305andwebFetch_20250910(with citations +maxContentTokens) directly on the mainstreamTextso Anthropic runs them inline. Tavily local wrapper is now Bedrock-only.anthropic-server/tavily-local/none), and surface the active backend to the model via a "Copilot backends" section insystem.tsand aCopilot backend: …line in the<env>block.WaitingForLoginSectionnow offers an optional Tavily key field on the AWS form for both API-key and IAM auth, with a link toapp.tavily.com. New mi-ai-panel RPC methods (getTavilyApiKey/setTavilyApiKey) let Settings update the key without re-running AWS login.webAccessPreapprovedfield,web_search/web_fetchPlanApprovalKindentries, localStorage preference, the matching Settings toggle, and the AIChatFooter approval-modal wiring all gone. Web tools now run without per-call approval (deepwiki-style), gated only by configuration.getErrorDiagnosticsand the agent stream-error path now surface AI SDKAPICallErrorfields (statusCode, url, responseBody, data) so future provider 4xx failures aren't silently swallowed.Summary by CodeRabbit
New Features
Improvements
Behavior Changes
Removals