✨ pluggable agent backend — Goose, OpenCode, MCP server, orchestrator + chat panel UI#1389
✨ pluggable agent backend — Goose, OpenCode, MCP server, orchestrator + chat panel UI#1389ibolton336 wants to merge 17 commits intokonveyor:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request implements a pluggable agent backend system enabling the VS Code extension to support multiple interchangeable AI backends (Goose, OpenCode, Claude, Codex). The changes introduce new type definitions for agent operations, implement two agent client variants communicating via JSON-RPC (Goose) and SSE (OpenCode), refactor state management to a centralized Zustand store replacing prior Immer-based mutations, establish a feature module system for experimental functionality, and deliver a complete chat UI with file review, settings, and permission handling workflows. Configuration management, file tracking, and batch review flows are added to support agent-produced code changes with optional review gates. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Rationale: The diff spans 9000+ lines across ~80 files with intricate, high-density logic including: (1) two distinct backend client implementations using JSON-RPC framing and SSE streaming protocols; (2) substantial state architecture refactoring (Immer→Zustand store with slice subscriptions); (3) new feature bootstrapping layer with per-module initialization; (4) complex orchestration flows for agent message handling, file change routing, and permission workflows; (5) comprehensive webview chat UI components with diff rendering and batch operations; (6) multi-pattern changes (store refactoring, message routing, client lifecycle). The heterogeneity—diverse backend protocols, structural architectural shifts, new UI system—demands reasoning for each section rather than applying a single pattern uniformly. Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
ibolton336
force-pushed
the
feature/pluggable-agent-backend
branch
from
f973bb1 to
0f5b07f
Compare
ibolton336
changed the title
ibolton336
force-pushed
the
feature/pluggable-agent-backend
branch
from
0f5b07f to
1e9b620
Compare
ibolton336
changed the title
ibolton336
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
vscode/core/src/paths.ts (1)
444-452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-add the out-of-workspace guard before ignore matching.
relative()returns../...for files outsideworkspaceRepoand can return an absolute path on Windows cross-drive paths. Without the old boundary check,isIgnoredBy(f)can treat external files as analyzable, so save events outside the workspace can leak into partial analysis.Patch sketch
-import { relative, dirname, join } from "node:path"; +import { relative, dirname, join, isAbsolute } from "node:path"; ... const f = relative(fsPaths().workspaceRepo, uri.fsPath); _logger?.debug(`isUriIgnored: ${f}`); + + if (f.startsWith("..") || isAbsolute(f)) { + return true; + } // Always ignore .konveyor directory if (f.startsWith(".konveyor/") || f === ".konveyor") { return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/paths.ts` around lines 444 - 452, Reintroduce the out-of-workspace guard before calling isIgnoredBy: after computing f = relative(fsPaths().workspaceRepo, uri.fsPath) (and before the ".konveyor" check or the isIgnoredBy(f) call), detect paths that start with ".." or are absolute (Windows cross-drive results) and return true (ignore) for those external files so that files outside the workspaceRepo cannot be treated as analyzable; update the logic around the existing variables f, uri.fsPath, and the isIgnoredBy(f) invocation to short-circuit external paths.vscode/core/src/analysis/batchedAnalysisTrigger.ts (1)
24-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not enqueue ignored paths.
With the early ignore guard gone, ignored files now stay in both
notifyFileChangesQueueandanalysisFileChangesQueueforever because the laterisUriIgnored(...)filters never remove them. In repos with lots of generated or vendor files, that turns into unbounded queue growth plus repeated re-filtering work on every schedule.Suggested fix
async notifyFileChanges(change: FileChange) { + if (isUriIgnored(change.path)) { + return; + } + if (this.enableHotRerun) { this.extensionState.mutate((draft) => { draft.isAnalysisScheduled = true; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/analysis/batchedAnalysisTrigger.ts` around lines 24 - 33, The notifyFileChanges handler currently enqueues every change into notifyFileChangesQueue and analysisFileChangesQueue which causes unbounded growth for ignored paths; fix this by checking the ignore predicate before mutating state or adding to queues: call the existing isUriIgnored (or isUriIgnoredSync) with change.path and return early if it matches so you never call this.notifyFileChangesQueue.set(...) or this.analysisFileChangesQueue.add(...). Keep the existing behavior around extensionState.mutate, this.enableHotRerun, scheduleNotifyFileChanges and schedulePartialAnalysis but only after confirming the path is not ignored.
🟡 Minor comments (11)
docs/goose-acp-messaging-guide.md-24-24 (1)
24-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language identifiers to fenced code blocks.
These fences are missing language tags (MD040). Adding explicit tags (
text,json,typescript, etc.) will satisfy lint and improve readability.Also applies to: 57-57, 457-457, 471-471, 591-591, 643-643, 672-672
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/goose-acp-messaging-guide.md` at line 24, Several fenced code blocks in the document are missing language identifiers (triple backticks with no tag); update each fenced code block to include an appropriate language tag (e.g., text, json, typescript, bash) so the Markdown linter (MD040) passes and syntax highlighting improves; search for occurrences of bare ``` fences and replace them with ```text, ```json, ```typescript, etc., matching the contained snippet content.docs/goose-acp-messaging-guide.md-13-13 (1)
13-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTOC fragment target appears invalid.
The anchor in the Table of Contents for “Webview <-> Extension Messaging” does not resolve cleanly under markdownlint (MD051). Please update the fragment to match the generated heading ID.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/goose-acp-messaging-guide.md` at line 13, The TOC anchor "[Webview <-> Extension Messaging](`#3-webview--extension-messaging`)" doesn't match the generated heading ID; update the fragment to the actual slug for the heading "Webview <-> Extension Messaging" (lowercase, spaces and special chars normalized) so the link resolves under markdownlint MD051—replace "#3-webview--extension-messaging" with the generated heading ID that your Markdown renderer produces for "Webview <-> Extension Messaging".webview-ui/src/components/ChatPage/ResourceBlock.tsx-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty text content is misclassified as binary.
Using
||converts valid empty strings to"(binary content)". Use nullish coalescing so onlynull/undefinedfalls back.Suggested fix
- const content = block.text || "(binary content)"; + const content = block.text ?? "(binary content)";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/ResourceBlock.tsx` at line 11, The assignment to content uses the || operator which treats empty string as falsy and replaces it with "(binary content)"; change the fallback to use the nullish coalescing operator so only null/undefined fall back. Update the line that sets content (using block.text) to use ?? instead of || so empty strings remain valid while null/undefined produce "(binary content)". Ensure references to block.text and the content variable are updated accordingly.webview-ui/src/components/ChatPage/ChatPage.css-710-710 (1)
710-710:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace deprecated
word-break: break-wordvalue.The
break-wordvalue forword-breakis deprecated. Useoverflow-wrap: break-wordinstead for the same behavior.🐛 Proposed fix
.agent-chat-msg--user .agent-chat-msg__content { max-width: 85%; padding: 6px 10px; border-radius: 10px 10px 2px 10px; background: var(--vscode-button-background, `#0e639c`); color: var(--vscode-button-foreground, `#fff`); font-size: 12px; line-height: 1.5; white-space: pre-wrap; - word-break: break-word; + overflow-wrap: break-word; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/ChatPage.css` at line 710, Replace the deprecated CSS declaration "word-break: break-word;" in ChatPage.css with "overflow-wrap: break-word;" (and optionally keep "word-break: normal;" for explicit behavior) so the element uses the supported overflow-wrap property instead of the deprecated value; locate the occurrence of the "word-break: break-word;" declaration and update it accordingly.scripts/test-goose-streaming.sh-27-37 (1)
27-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the session/prompt waits deterministic.
This currently mixes fixed sleeps with a non-failing timeout, so a slow
session/newcan produce a false failure and a missingstopReasoncan still exit 0 with misleading stats. Poll forsessionIdthe same way you poll for completion, and fail hard when the completion deadline is hit.Suggested fix
echo '{"jsonrpc":"2.0","id":2,"method":"session/new","params":{}}' >&3 -sleep 1 - -# Extract session ID from the response -SESSION_ID=$(grep -o '"sessionId":"[^"]*"' "$OUTFILE" | head -1 | cut -d'"' -f4) +# Extract session ID from the response +SESSION_ID="" +for _ in $(seq 1 20); do + SESSION_ID=$(grep -o '"sessionId":"[^"]*"' "$OUTFILE" | head -1 | cut -d'"' -f4) + [ -n "$SESSION_ID" ] && break + sleep 0.5 +done if [ -z "$SESSION_ID" ]; then echo "ERROR: Could not extract session ID from goose output." echo "Raw output:" cat "$OUTFILE" @@ echo "Waiting for response..." -for i in $(seq 1 60); do +GOT_STOP_REASON=0 +for _ in $(seq 1 60); do if grep -q '"stopReason"' "$OUTFILE" 2>/dev/null; then + GOT_STOP_REASON=1 break fi sleep 0.5 done +if [ "$GOT_STOP_REASON" -ne 1 ]; then + echo "ERROR: Timed out waiting for stopReason." + echo "Raw output:" + cat "$OUTFILE" + exit 1 +fiAlso applies to: 43-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-goose-streaming.sh` around lines 27 - 37, The script currently uses fixed sleeps and a non-failing timeout which can cause false negatives; replace the one-shot sleep+grep that sets SESSION_ID by implementing a poll loop that repeatedly greps "$OUTFILE" for '"sessionId":"[^"]*"' (same pattern currently used) up to the same completion deadline used elsewhere, assign SESSION_ID when matched, and exit non-zero with an error message if the deadline is hit; do the same deterministic polling and hard-fail behavior for the later block that checks for stopReason so both session/new and completion waits are polled rather than relying on fixed sleeps.webview-ui/src/components/ChatPage/AgentFileReview.tsx-338-343 (1)
338-343:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect useEffect dependency causes missing re-runs.
The dependency
[pendingFiles.length > 0]evaluates to a boolean at definition time, not a tracked dependency. This means the effect won't re-run whenpendingFileschanges from empty to non-empty after the initial render. UsependingFiles.lengthorpendingFilesas the dependency.🐛 Fix useEffect dependency
// Auto-expand the first file when the review appears React.useEffect(() => { if (pendingFiles.length > 0 && expandedTokens.size === 0) { setExpandedTokens(new Set([pendingFiles[0].messageToken])); } - }, [pendingFiles.length > 0]); // eslint-disable-line react-hooks/exhaustive-deps + }, [pendingFiles]);Alternatively, if you want to avoid re-running when files change but the list remains non-empty, use a ref to track if expansion has occurred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/AgentFileReview.tsx` around lines 338 - 343, The useEffect currently uses a boolean literal dependency `[pendingFiles.length > 0]` which doesn't track changes to the array; update the dependency to either `pendingFiles.length` (to re-run when the number of files changes) or `pendingFiles` (to re-run on any change) so the effect that auto-expands the first file (uses React.useEffect, pendingFiles, expandedTokens, setExpandedTokens, and pendingFiles[0].messageToken) will run when files are added; if you need to avoid re-running for content changes while preserving a single expansion, consider using a ref to track whether expansion has already occurred and check that inside the effect.vscode/core/src/features/agent/fileChangeRouter.ts-41-58 (1)
41-58:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistency between docstring and implementation for batch review mode.
The docstring states "When
isBatchReviewModeis enabled (orforceReviewis true)", suggesting both conditions should be checked. However, line 48 simply assignsforceReviewtoisBatchReviewMode, ignoringstate.data.isBatchReviewModeentirely.If the intent is that
forceReviewoverrides all behavior, the docstring should be updated. If both conditions should enable batch review mode, the logic needs adjustment.🔧 Suggested fix if both conditions should enable batch review
export async function routeFileChange( state: ExtensionState, filePath: string, content: string, originalContent?: string, forceReview = false, ): Promise<void> { - const isBatchReviewMode = forceReview; + const isBatchReviewMode = forceReview || state.data.isBatchReviewMode; const relativePath = normalizeFilePath(filePath, state.data.workspaceRoot);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/fileChangeRouter.ts` around lines 41 - 58, The code assigns isBatchReviewMode = forceReview but the docstring says batch review should be enabled when either forceReview is true or state.data.isBatchReviewMode is true; update the logic in routeFileChange to compute isBatchReviewMode as the OR of both conditions (e.g., use forceReview || state.data.isBatchReviewMode) so both sources enable batch review, and ensure any docstring/comment matches this behavior; touch the symbols routeFileChange, isBatchReviewMode, forceReview, and state.data.isBatchReviewMode when making the change.vscode/core/src/features/agent/batchReviewHandlers.ts-17-21 (1)
17-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential Windows path issue with
file://URL parsing.On Windows,
new URL("file:///C:/path").pathnamereturns/C:/pathwith a leading slash, which can causepath.jointo produce incorrect paths like/C:/path/relative.🛠️ Proposed fix
function resolveAbsolutePath(filePath: string, state: ExtensionState): string { if (isAbsolute(filePath)) { return filePath; } let wsRoot = state.data.workspaceRoot; if (wsRoot.startsWith("file://")) { - wsRoot = new URL(wsRoot).pathname; + const url = new URL(wsRoot); + wsRoot = process.platform === "win32" + ? url.pathname.slice(1) // Remove leading slash on Windows + : url.pathname; } return join(wsRoot, filePath); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/batchReviewHandlers.ts` around lines 17 - 21, The code naively converts a file:// URL using new URL(wsRoot).pathname which yields a leading slash on Windows (e.g. /C:/path) and breaks path.join; replace that logic to use the Node utility that correctly converts file URLs to OS paths (use fileURLToPath from 'url') so when wsRoot startsWith("file://") set wsRoot = fileURLToPath(wsRoot) before calling join(wsRoot, filePath), ensuring correct Windows drive-letter paths and preserving behavior on POSIX.vscode/core/src/client/opencodeClient.ts-430-434 (1)
430-434:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win**Missing error handling and null check in
createSession.**ThecreateSessionmethod accessessession.data.iddirectly without checking for errors or null, unlike the robust error handling instart()at lines 298-311. The SDK can throw errors that you can catch and handle.🛡️ Proposed fix to add error handling
async createSession(): Promise<string> { if (this.agentState !== "running" || !this.client) { throw new Error("OpencodeAgentClient: not running"); } const session = await this.client.session.create({ body: { title: "Konveyor Migration Assistant" }, }); + if (session.error) { + throw new Error( + `Session creation failed: ${typeof session.error === "string" ? session.error : JSON.stringify(session.error)}`, + ); + } + - this.sessionId = session.data.id; + const sessionData = session.data ?? session.response?.body; + const sessionId = sessionData?.id ?? sessionData?.ID; + if (!sessionId) { + throw new Error("Session response missing id"); + } + this.sessionId = sessionId; this.logger.info(`OpencodeAgentClient: new session created ${this.sessionId}`); return this.sessionId!; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/client/opencodeClient.ts` around lines 430 - 434, The createSession method currently uses session.data.id without any null checks or error handling; wrap the SDK call that produces session in a try/catch, validate session and session.data.id before assigning to this.sessionId, and on failure log the full error via this.logger.error (include error details and context "createSession") and either throw a descriptive error or return a rejected/undefined result consistent with start(); update references to this.sessionId only after the null check so you never set it from an invalid response.vscode/core/src/features/agent/handlers.ts-334-335 (1)
334-335:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFragile acceptance detection based on string matching.
Using
optionId.includes("allow")to determine acceptance is brittle. If option IDs change or new kinds are added, this logic could break silently. Consider using thekindproperty from thePermissionOptionif available, or document this naming convention requirement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/handlers.ts` around lines 334 - 335, The current acceptance check uses a brittle string match (const accepted = optionId.includes("allow")), so update the logic to use the explicit PermissionOption.kind (or a similarly named enum/property) instead: locate where optionId and the option object are available in the handler (symbols: optionId, accepted, PermissionOption) and replace the includes-based test with a strict check against option.kind (e.g., option.kind === "allow" or the corresponding enum value); if kind is not present, add/read a clear property on the PermissionOption shape and fall back to a documented, strict naming rule rather than a substring search.vscode/core/src/features/agent/handlers.ts-174-178 (1)
174-178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAgent restart lacks graceful error handling.
If
stop()throws,start()is never called, leaving the agent in a potentially inconsistent state. Consider handling failures in the restart cycle more gracefully.🛡️ Suggested improvement
const agentClient = getAgentClient(state); if (agentClient) { - await agentClient.stop(); - await agentClient.start(); + try { + await agentClient.stop(); + } catch (stopErr) { + logger.warn("Failed to stop agent during config update, proceeding with start:", stopErr); + } + await agentClient.start(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/handlers.ts` around lines 174 - 178, The restart sequence for the agent (getAgentClient -> agentClient.stop() -> agentClient.start()) needs robust error handling so a thrown error from stop() doesn't prevent start() from running; wrap the stop() call in a try/catch (or use Promise.catch) to log the error from agentClient.stop() and continue to call agentClient.start(), and optionally handle/rethrow start() errors separately so the agent is left in a consistent state and failures are observable via logging from the same getAgentClient/agentClient context.
🧹 Nitpick comments (22)
webview-ui/src/components/ResolutionsPage/MessageWrapper.css (1)
11-13: 💤 Low valueConsider removing the redundant last-child rule.
The
.message-wrapper:last-childrule setsmargin-bottom: 4px, but this is already defined by the default.message-wrappermargin (margin: 4px 0). Since both specify the same bottom margin value, the last-child rule has no effect and can be removed for cleaner CSS.♻️ Proposed cleanup
-.message-wrapper:last-child { - margin-bottom: 4px; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ResolutionsPage/MessageWrapper.css` around lines 11 - 13, Remove the redundant CSS rule `.message-wrapper:last-child { margin-bottom: 4px; }` because the base `.message-wrapper { margin: 4px 0; }` already applies the same bottom margin; delete the `.message-wrapper:last-child` selector from MessageWrapper.css to clean up duplicate styles and leave only the default `.message-wrapper` rule.webview-ui/src/components/ChatPage/ChatPage.css (1)
5-5: 💤 Low valueContainer name should use kebab-case per Stylelint convention.
The container name
chatPageshould bechat-pageto follow kebab-case naming conventions. This affects the definition here and all@containerqueries referencing it (lines 1185, 1200, 1264).♻️ Proposed fix
-.chat-page-container { - container-name: chatPage; +.chat-page-container { + container-name: chat-page;And update all
@container chatPageto@container chat-page.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/ChatPage.css` at line 5, Change the CSS container name from camelCase to kebab-case: replace the container-name value "chatPage" with "chat-page" in the ChatPage CSS (the declaration using container-name: chatPage;) and update every corresponding `@container` rule that references "chatPage" to use "@container chat-page" (there are multiple `@container` queries that must be updated).vscode/core/src/utilities/profiles/profileActions.ts (1)
41-55: ⚡ Quick winMake the profile update and config revalidation atomic.
These two
state.mutate(...)calls represent one logical change. Emitting them separately lets subscribers observe the new profile with staleconfigErrorsand causes an extra state-sync pass.♻️ Suggested consolidation
- state.mutate((draft) => { - const target = draft.profiles.find((p) => p.id === updated.id); - if (target) { - Object.assign(target, updated); - } - }); - - // Re-validate config errors to clear "no custom rules" error if applicable state.mutate((draft) => { + const target = draft.profiles.find((p) => p.id === updated.id); + if (target) { + Object.assign(target, updated); + } + + // Re-validate config errors to clear "no custom rules" error if applicable // Clear existing profile-related errors and re-validate using centralized logic draft.configErrors = draft.configErrors.filter( (error) => error.type !== "invalid-label-selector" && error.type !== "no-custom-rules", ); updateConfigErrors(draft, ""); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/utilities/profiles/profileActions.ts` around lines 41 - 55, The two separate state.mutate calls cause intermediate state exposure; consolidate them into one atomic mutation: inside a single state.mutate callback, find and Object.assign the matching profile in draft.profiles (same logic as current), then clear profile-related errors from draft.configErrors (filter out "invalid-label-selector" and "no-custom-rules") and call updateConfigErrors(draft, "") so both the profile update and revalidation happen in the same transaction.webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts (1)
27-41: 💤 Low valueEdge case:
file:///prefix stripping may leave leading slash on Windows.When
workspaceRootis"file:///C:/project", stripping"file://"(7 chars) leaves"/C:/project". ThenormalizedPath(e.g.,"C:/project/src/file.ts") won't start with"/C:/project", causing the fallback to extract just the filename instead of the relative path.The fallback behavior is acceptable for display purposes, but if accurate relative paths on Windows are needed, consider stripping three slashes for the
file:///case:Suggested adjustment
if (root.startsWith("file:///")) { - root = root.slice("file://".length); + root = root.slice("file:///".length); } else if (root.startsWith("file:")) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts` around lines 27 - 41, The current logic slices the wrong prefix for file URIs on Windows (it uses "file://".length), which leaves a leading slash (e.g., "/C:/project") so normalizedPath ("C:/project/...") won't match; update the handling in useModifiedFileData.ts: when detecting "file:///" slice the correct length (or use "file:///" explicitly) and then remove a single leading "/" only when it precedes a Windows drive letter (e.g., if root startsWith("/") and root[1] matches /[A-Za-z]:/), so workspaceRoot/root and normalizedPath compare correctly; keep the existing fallbacks for fileName if matching fails.vscode/core/src/features/agent/toolPermissionPolicy.ts (1)
5-14: 💤 Low valueConsider whether
undo_editshould be classified as read-only.
undo_editmodifies the file system by reverting changes, which isn't strictly "read-only" in the traditional sense. If the intent is to auto-approve safe operations that don't introduce new content, this is acceptable, but the comment/JSDoc could clarify the semantics (e.g., "safe to auto-approve" vs "read-only").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/toolPermissionPolicy.ts` around lines 5 - 14, The current code classifies "undo_edit" as read-only via READ_ONLY_COMMANDS and isReadOnlyToolCall, but undo_edit actually mutates files; update the code/comments to reflect intended semantics: either remove "undo_edit" from READ_ONLY_COMMANDS if you want a strict read-only check, or change the JSDoc and symbol names (e.g., rename READ_ONLY_COMMANDS to SAFE_AUTO_APPROVE_COMMANDS and update the comment on isReadOnlyToolCall to describe "safe to auto-approve/reversible" operations) and document why undo_edit is considered safe; adjust any callers/tests that rely on isReadOnlyToolCall accordingly.webview-ui/src/components/ChatPage/CompactModifiedFile.tsx (1)
7-17: 💤 Low valueUnused
timestampprop.The
timestampprop is declared inCompactModifiedFilePropsbut is not destructured or used in the component implementation.♻️ Remove unused prop or use it
If
timestampis intended for future use, consider removing it until needed:interface CompactModifiedFileProps { data: ModifiedFileMessageValue; - timestamp?: string; }Or if it should be displayed, add it to the component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/CompactModifiedFile.tsx` around lines 7 - 17, CompactModifiedFileProps declares a timestamp?: string that CompactModifiedFile (React.memo component) does not use; either remove timestamp from CompactModifiedFileProps if not needed, or destructure and render it inside the CompactModifiedFile component (e.g., add timestamp to the props in the CompactModifiedFile signature and display it where the filename/status are shown) and update any callers accordingly to pass a timestamp when required.webview-ui/src/components/ChatPage/AgentFileReview.tsx (1)
100-101: 💤 Low valueRedundant ternary expression.
Both branches return
line.content, making the ternary unnecessary.♻️ Simplify redundant ternary
<span className="afr-diff__content"> - {line.type === "hunk-header" ? line.content : line.content} + {line.content} </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/AgentFileReview.tsx` around lines 100 - 101, The JSX contains a redundant ternary in the span with className "afr-diff__content" where both branches return line.content; remove the ternary and render the value directly (replace the expression {line.type === "hunk-header" ? line.content : line.content} with a single {line.content}) in the AgentFileReview component to simplify the markup.mcp-server/src/index.ts (1)
38-44: 💤 Low valueConsider handling non-JSON error responses gracefully.
If the bridge returns a non-OK response, you extract the text for the error message (line 40), which is good. However, if the response is OK but not valid JSON (e.g., empty body or malformed),
response.json()will throw. This edge case may be unlikely given you control the bridge, but a defensive check would improve resilience.🛡️ Optional defensive handling
if (!response.ok) { const text = await response.text(); throw new Error(`Bridge request failed: ${response.status} ${text}`); } - return response.json(); + const text = await response.text(); + if (!text) { + return {}; + } + return JSON.parse(text); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/src/index.ts` around lines 38 - 44, The current code calls response.json() without guarding against empty or malformed JSON; wrap the final return in a defensive block: check response.headers.get('content-type') for 'application/json' and, if missing, call await response.text() and handle an empty body (return null or throw a clear Error), otherwise attempt JSON.parse on the text inside a try/catch and rethrow a descriptive error (including the raw body) if parsing fails; update the code around response.json(), response.text(), and the existing throw new Error(...) so callers get a clear, handled outcome for non-JSON or empty responses.webview-ui/src/components/ChatPage/AgentSettings.tsx (2)
266-273: 💤 Low valueOptimistic store update before server confirmation.
The experimental chat toggle updates the local store immediately before posting the message to VS Code. If the extension fails to persist this setting, the UI state will be out of sync with the actual configuration.
This is a common optimistic update pattern, but consider whether the setting should only update after confirmation from the extension (via a state change message).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/AgentSettings.tsx` around lines 266 - 273, The toggle currently flips experimentalChatEnabled optimistically by calling useExtensionStore.getState().setExperimentalChatEnabled(...) before sending window.vscode.postMessage({ type: SET_EXPERIMENTAL_CHAT, ... }), which can leave UI out-of-sync if the extension fails to persist the setting; change the flow so the click posts the SET_EXPERIMENTAL_CHAT message first (using window.vscode.postMessage) and only update the local store (via useExtensionStore.getState().setExperimentalChatEnabled) when you receive the extension's confirmation message/event (or revert the store on an error response), and ensure the component listens for that confirmation to update experimentalChatEnabled accordingly.
103-108: 💤 Low value
hasChangesmay incorrectly returnundefinedinstead ofboolean.The
.some()on the last line can returnundefinedifagentConfig?.capabilitiesis undefined (sinceundefined?.some()isundefined). This could cause the truthy check to behave correctly in practice, but the type would not be a strict boolean.💡 Suggested fix for type safety
const hasChanges = selectedProvider !== (agentConfig?.provider ?? "") || modelInput !== (agentConfig?.model ?? "") || agentModeEnabled !== (agentConfig?.agentMode ?? true) || Object.values(credentialInputs).some((v) => v.length > 0) || - agentConfig?.capabilities.some((ext) => extensionStates[ext.id] !== ext.enabled); + (agentConfig?.capabilities.some((ext) => extensionStates[ext.id] !== ext.enabled) ?? false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/AgentSettings.tsx` around lines 103 - 108, The hasChanges expression can become undefined because agentConfig?.capabilities?.some(...) may be undefined; update the last clause to ensure a boolean by using a safe default or nullish coalescing so it evaluates to false when capabilities is missing (e.g., replace agentConfig?.capabilities.some(...) with agentConfig?.capabilities?.some(...) ?? false or default agentConfig?.capabilities to [] before calling some), keeping references to hasChanges, agentConfig, extensionStates, and extension id checks unchanged.webview-ui/src/components/ChatPage/ChatPage.tsx (2)
185-194: 💤 Low valueConsider guarding against undefined
messageToken.When building the tool group key,
toolBuffer[0].messageTokenis used. WhileflushToolsonly runs whentoolBuffer.length > 0, if aChatMessagein the buffer has an undefinedmessageToken, the key would beundefined.💡 Suggested improvement
const flushTools = () => { if (toolBuffer.length > 0) { items.push({ type: "tool-group", tools: toolBuffer, - key: toolBuffer[0].messageToken, + key: toolBuffer[0].messageToken ?? `tool-group-${items.length}`, }); toolBuffer = []; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/ChatPage.tsx` around lines 185 - 194, flushTools currently uses toolBuffer[0].messageToken directly for the group key which can be undefined; change the key construction in flushTools to guard against undefined by selecting the first defined messageToken in toolBuffer (e.g., find a message with a non-undefined messageToken) or falling back to a stable alternative (such as a message id property, index-based token, or a generated unique string) so the key is never undefined; update references to toolBuffer, items, and the key generation inside the flushTools function accordingly and ensure the chosen fallback yields a unique, repeatable key for the group.
45-84: ⚖️ Poor tradeoffConsider consolidating store subscriptions.
The component has 18 individual
useExtensionStorecalls. While Zustand optimizes re-renders per selector, consolidating related state into fewer selectors or a single selector returning an object could improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/ChatPage.tsx` around lines 45 - 84, The ChatPage component makes many separate useExtensionStore calls (e.g., experimentalChatEnabled, agentState, agentError, agentConfig, agentMessages, configErrors, modelSupportsTools, chatMessages, solutionScope, isFetchingSolution, isAnalyzing, analysisProgressMessage, enhancedIncidents, ruleSets, analysisProgress, isWaitingForUserInteraction), so consolidate related subscriptions by replacing multiple calls with one or a few selectors that return grouped objects (e.g., useExtensionStore(s => ({ experimentalChatEnabled: s.experimentalChatEnabled, agentState: s.agentState, agentMessages: s.agentMessages, chatMessages: s.chatMessages, ... }))) and use shallow comparison (import shallow from 'zustand/shallow') when selecting objects to avoid unnecessary re-renders; then update references in ChatPage (isProcessing, hasWorkflowContent, hasPendingPermission, isAgentStreaming, isTriggeredByUser, incidentCount, etc.) to use the properties off the grouped selector. Ensure you keep derived values (like isProcessing = isFetchingSolution || isAnalyzing) computed from the grouped result so behavior is unchanged.vscode/core/src/client/directLLMClient.ts (2)
75-77: 💤 Low value
getState()always returns"running"regardless of actual state.This could be misleading if callers expect accurate state reporting. Consider tracking the actual state based on lifecycle events.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/client/directLLMClient.ts` around lines 75 - 77, getState() currently returns the hardcoded string "running" which misrepresents the real lifecycle; add a real state field (e.g., a private this.state or this._state typed as AgentState) in the DirectLLMClient class, set it to appropriate values during lifecycle methods (start/stop/initialize/terminate or any methods that change status), and make getState() return that field instead of the constant so callers receive the actual state.
107-131: 💤 Low value
sendMessageignores_contentand_responseMessageIdparameters.The method signature accepts
contentandresponseMessageIdbut neither is used. The workflow runs with the pre-configuredworkflowInputregardless of what message is sent. If this is intentional (the DirectLLMClient is a fire-once wrapper), consider documenting this behavior or removing the unused parameters.📝 Suggested documentation
// ─── Core: run the workflow ──────────────────────────────────────── + /** + * Runs the pre-configured workflow. The content and responseMessageId + * parameters are ignored as DirectLLMClient is designed as a single-use + * wrapper that executes the workflow with its initial workflowInput. + */ async sendMessage(_content: string, _responseMessageId: string): Promise<string> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/client/directLLMClient.ts` around lines 107 - 131, sendMessage currently ignores its parameters _content and _responseMessageId and always runs this.workflow with the preconfigured this.workflowInput; either wire those parameters into the workflow input or remove/document them: before calling this.workflow.run(this.workflowInput) assign the incoming values into the appropriate fields on this.workflowInput (e.g., set this.workflowInput.message or this.workflowInput.content = _content and this.workflowInput.responseMessageId = _responseMessageId) so the runtime message is used, or if the client is intentionally fire-once, remove the unused parameters from the sendMessage signature (or add a clear JSDoc comment) and update any call sites accordingly; keep promptActive handling and error logging unchanged.vscode/core/src/agentConfigReader.ts (2)
66-78: 💤 Low valueConsider logging config read failures for debugging.
The silent catch block swallows all errors, which could hide issues like permission errors or malformed YAML that users would want to know about.
💡 Suggested improvement
try { const { fsPaths } = require("./paths"); const content = fs.readFileSync(fsPaths().settingsYaml, "utf-8"); const doc = parse(content) as Record<string, any> | undefined; const active = doc?.active; if (active) { const langchainName = typeof active.provider === "string" ? active.provider : ""; provider = langchainProviderToUiId(langchainName, active.args) ?? langchainName; model = typeof active.args?.model === "string" ? active.args.model : ""; } - } catch { + } catch (err) { // provider-settings.yaml missing or malformed — use defaults + // Optionally log for debugging: console.debug("OpenCode config read error:", err); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/agentConfigReader.ts` around lines 66 - 78, The try/catch in agentConfigReader.ts silently swallows all errors when reading/parsing fsPaths().settingsYaml, making diagnosis impossible; update the catch to log the error (including the error object/message and context like settingsYaml path) using the project's logging mechanism or console (e.g., reference fsPaths().settingsYaml, the parse step and langchainProviderToUiId) so permission issues or YAML parse errors are visible, while still falling back to defaults for provider/model.
50-57: 💤 Low valueConsider handling undefined
workspaceRootmore defensively.When
workspaceRootis undefined,getAgentConfigPathreturns"./opencode.json"which is relative to the current working directory, potentially not the intended location.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/agentConfigReader.ts` around lines 50 - 57, getAgentConfigPath currently falls back to "./opencode.json" when workspaceRoot is undefined, which can produce an unintended relative path; update getAgentConfigPath (handling the "opencode" case) to defensively resolve a concrete absolute directory instead of relying on "./" by using path.resolve(workspaceRoot ?? process.cwd()) when building the opencode.json path (or alternatively throw/return an explicit error if workspaceRoot must be provided), so the returned path is deterministic and not relative to the current working directory.vscode/core/src/extension.ts (1)
1519-1523: ⚡ Quick winBuild the first-run globalState key from
EXTENSION_NAME.This hard-codes the branded prefix into persisted state, which makes downstream rebranding carry the old key forever. Please derive the key from
EXTENSION_NAMEinstead of embedding"konveyor"here.Based on learnings, extension names should be programmatic rather than hard-coded to support downstream rebranding efforts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/extension.ts` around lines 1519 - 1523, The persisted globalState key currently hard-codes "konveyor.hasShownChatView"; change both uses of that literal to derive the key from EXTENSION_NAME (e.g., build a key string using EXTENSION_NAME + ".hasShownChatView") so get and update call context.globalState.get<boolean>(generatedKey) / context.globalState.update(generatedKey, true) and keep the vscode.commands.executeCommand call unchanged; update references near hasShownChat, context.globalState.get, and context.globalState.update to use the computed key.vscode/core/src/features/agent/toolPermissionHandler.ts (2)
110-112: ⚡ Quick winConsider using static import for the
diffmodule.Using
require("diff")inside the function body means the module is loaded on each call. Sincediffis already a dependency (see package.json line 541), a static import at the top of the file would be cleaner and more efficient.♻️ Proposed refactor
At the top of the file:
import { v4 as uuidv4 } from "uuid"; +import { createPatch } from "diff"; import type { ExtensionData, ChatMessage, ToolMessageValue } from "@editor-extensions/shared";Then replace the dynamic requires:
if (typeof oldStr === "string" && typeof newStr === "string") { - const { createPatch } = require("diff"); const displayPath = extractDisplayPath(input, workspaceRoot) ?? "file"; const patch = createPatch(displayPath, oldStr, newStr, "", "", { context: 3 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/toolPermissionHandler.ts` around lines 110 - 112, The code dynamically requires the diff module inside the function (createPatch via require("diff")), causing repeated loads; change this to a static top-level import by adding an import { createPatch } from "diff" at the top of the file and remove the require("diff") call in the body where displayPath is computed (symbols to update: createPatch, extractDisplayPath, displayPath, workspaceRoot). Ensure the import uses ES module/TypeScript syntax consistent with the file and remove the inline require to avoid per-call module resolution overhead.
226-231: 💤 Low valueSingle
replace()only substitutes the first occurrence.If
oldStrappears multiple times inoriginalContent,String.replace(oldStr, newStr)only replaces the first match. This may not reflect the actual file state after the tool executes.💡 Consider using replaceAll or regex with global flag
if (typeof oldStr === "string" && typeof newStr === "string" && originalContent) { - rawFileContent = originalContent.replace(oldStr, newStr); + rawFileContent = originalContent.replaceAll(oldStr, newStr); }Alternatively, if the tool semantics are to replace only the first occurrence, this is correct as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/toolPermissionHandler.ts` around lines 226 - 231, The current replacement in toolPermissionHandler (when oldStr/newStr/originalContent are strings) only replaces the first occurrence; if you intend to replace all occurrences update the branch that sets rawFileContent to use a global replace (either originalContent.replaceAll(oldStr, newStr) or originalContent.replace(new RegExp(escapeRegExp(oldStr), "g"), newStr)); add a small escapeRegExp helper to safely escape special regex chars if using RegExp, and ensure rawFileContent is set from that global-replace result instead of String.replace.vscode/core/package.json (1)
381-396: 💤 Low valueDuplicate
ordervalue for configuration properties.Both
konveyor-core.genai.batchReviewMode(line 386) andkonveyor-core.genai.excludedDiagnosticSources(line 395) have"order": 22. This may cause inconsistent ordering in the VS Code settings UI.🔧 Proposed fix
"konveyor-core.genai.batchReviewMode": { "type": "boolean", "default": false, "description": "When enabled, file changes from the agent are queued for review instead of being applied immediately. Allows accepting or rejecting changes in bulk.", "scope": "window", - "order": 22 + "order": 23 }, "konveyor-core.genai.excludedDiagnosticSources": { "type": "array", "default": [ "trunk" ], "description": "List of diagnostic sources to exclude from agentic workflow.", "scope": "window", - "order": 22 + "order": 24 },Note: This would require updating subsequent
ordervalues as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/package.json` around lines 381 - 396, The two settings keys konveyor-core.genai.batchReviewMode and konveyor-core.genai.excludedDiagnosticSources both use the same "order": 22 which causes ambiguous ordering in the VS Code settings UI; update konveyor-core.genai.excludedDiagnosticSources to a unique, incremented order (e.g., 23) and then adjust any subsequent "order" values for later settings to maintain a monotonically increasing sequence so the UI displays them consistently.shared/src/types/messages.ts (1)
125-141: 💤 Low valueType guards use unsafe
as anycasts.The type guards cast
msgtoanybefore accessing.type. SinceWebviewMessageis a discriminated union, you can safely check thetypeproperty directly without casting.♻️ Safer type guard pattern
export function isStateChange(msg: WebviewMessage): msg is StateChangeMessage { - return (msg as any).type === MessageTypes.STATE_CHANGE; + return msg.type === MessageTypes.STATE_CHANGE; } export function isFocusViolation(msg: WebviewMessage): msg is FocusViolationMessage { - return (msg as any).type === MessageTypes.FOCUS_VIOLATION; + return msg.type === MessageTypes.FOCUS_VIOLATION; } // Chat export function isChatStateChange(msg: WebviewMessage): msg is ChatStateChangeMessage { - return (msg as any).type === MessageTypes.CHAT_STATE_CHANGE; + return msg.type === MessageTypes.CHAT_STATE_CHANGE; } export function isChatStreamingUpdate(msg: WebviewMessage): msg is ChatStreamingUpdateMessage { - return (msg as any).type === MessageTypes.CHAT_STREAMING_UPDATE; + return msg.type === MessageTypes.CHAT_STREAMING_UPDATE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/src/types/messages.ts` around lines 125 - 141, The type guards (isStateChange, isFocusViolation, isChatStateChange, isChatStreamingUpdate) currently use unsafe "as any" casts; remove those casts and access the discriminant directly (e.g., use msg.type === MessageTypes.STATE_CHANGE). To be defensive, first ensure msg is non-null and an object (e.g., typeof msg === "object" && msg !== null) before checking msg.type, then compare against the appropriate MessageTypes constant in each guard.vscode/core/src/features/agent/handlers.ts (1)
307-309: 💤 Low valueSilent catch swallows all errors.
The empty catch block silently ignores any failures during webview initialization. While "best-effort" is intended, consider logging at debug level to aid troubleshooting.
♻️ Add debug logging
- } catch { - // Best-effort — agent may not be configured yet - } + } catch (err) { + // Best-effort — agent may not be configured yet + logger.debug("AGENT_WEBVIEW_READY: best-effort config send failed:", err); + }Note: This requires changing
_loggerparameter tologgeron line 281.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/handlers.ts` around lines 307 - 309, The empty catch in the webview initialization swallows errors; update the function signature to rename the parameter from _logger to logger (as noted) and change the catch to capture the error (catch (err)) and call logger.debug with a concise message and the error to preserve "best-effort" behavior while enabling troubleshooting—locate the try/catch around the webview init in handlers.ts and the function that declares _logger (near line 281) and replace the silent catch with logger.debug("Failed to initialize agent webview (best-effort)", err).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d1b0702-376c-4722-89d6-61e041af03f1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (106)
.husky/pre-commitagentic/src/cache.tsagentic/src/nodes/analysisIssueFix.tsagentic/src/nodes/base.tsagentic/src/nodes/diagnosticsIssueFix.tsagentic/src/types.tschanges/unreleased/1351-pluggable-agent-backend.yamlchanges/unreleased/fix-duplicate-loading-indicators.yamldocs/goose-acp-messaging-guide.mdmcp-server/package.jsonmcp-server/src/index.tsmcp-server/tsconfig.jsonpackage.jsonscripts/test-goose-streaming.shshared/src/types/actions.tsshared/src/types/agent.tsshared/src/types/agentActions.tsshared/src/types/agentMessages.tsshared/src/types/index.tsshared/src/types/messages.tsshared/src/types/types.tsvscode/core/CONTRIBUTING-FEATURES.mdvscode/core/package.jsonvscode/core/resources/sample-provider-settings.yamlvscode/core/src/KonveyorGUIWebviewViewProvider.tsvscode/core/src/__tests__/mocks.tsvscode/core/src/agentConfigReader.tsvscode/core/src/analysis/__tests__/batchedAnalysisTrigger.test.tsvscode/core/src/analysis/batchedAnalysisTrigger.tsvscode/core/src/analysis/runAnalysis.tsvscode/core/src/api/mcpBridgeServer.tsvscode/core/src/client/agentClient.tsvscode/core/src/client/analyzerClient.tsvscode/core/src/client/directLLMClient.tsvscode/core/src/client/gooseClient.tsvscode/core/src/client/opencodeClient.tsvscode/core/src/clients/ProfileSyncClient.tsvscode/core/src/commands.tsvscode/core/src/data/loadResults.tsvscode/core/src/diff/vertical/manager.tsvscode/core/src/extension.tsvscode/core/src/extensionState.tsvscode/core/src/features/agent/__tests__/toolPermissionHandler.test.tsvscode/core/src/features/agent/agentOrchestrator.tsvscode/core/src/features/agent/batchReviewHandlers.tsvscode/core/src/features/agent/fileChangeRouter.tsvscode/core/src/features/agent/fileTracker.tsvscode/core/src/features/agent/handlers.tsvscode/core/src/features/agent/index.tsvscode/core/src/features/agent/init.tsvscode/core/src/features/agent/promptBuilder.tsvscode/core/src/features/agent/toolPermissionHandler.tsvscode/core/src/features/agent/toolPermissionPolicy.tsvscode/core/src/features/featureRegistry.tsvscode/core/src/features/index.tsvscode/core/src/gooseConfig.tsvscode/core/src/modelProvider/config.tsvscode/core/src/modelProvider/providerConfigGenerator.tsvscode/core/src/paths.tsvscode/core/src/solutionWorkflowOrchestrator.tsvscode/core/src/store/extensionStore.tsvscode/core/src/store/syncBridges.tsvscode/core/src/utilities/ModifiedFiles/handleFileResponse.tsvscode/core/src/utilities/ModifiedFiles/handleModifiedFile.tsvscode/core/src/utilities/ModifiedFiles/handleQuickResponse.tsvscode/core/src/utilities/ModifiedFiles/processMessage.tsvscode/core/src/utilities/ModifiedFiles/processModifiedFile.tsvscode/core/src/utilities/ModifiedFiles/queueManager.tsvscode/core/src/utilities/agentCredentialStorage.tsvscode/core/src/utilities/configuration.tsvscode/core/src/utilities/gooseCredentialStorage.tsvscode/core/src/utilities/profiles/profileActions.tsvscode/core/src/utilities/profiles/profileService.tsvscode/core/src/webviewMessageHandler.tswebview-ui/package.jsonwebview-ui/src/App.tsxwebview-ui/src/components/AnalysisPage/AnalysisPage.tsxwebview-ui/src/components/ChatPage/AgentFileReview.csswebview-ui/src/components/ChatPage/AgentFileReview.tsxwebview-ui/src/components/ChatPage/AgentSettings.tsxwebview-ui/src/components/ChatPage/ChatPage.csswebview-ui/src/components/ChatPage/ChatPage.tsxwebview-ui/src/components/ChatPage/CompactBatchReview.csswebview-ui/src/components/ChatPage/CompactBatchReview.tsxwebview-ui/src/components/ChatPage/CompactMigrationScope.tsxwebview-ui/src/components/ChatPage/CompactModifiedFile.csswebview-ui/src/components/ChatPage/CompactModifiedFile.tsxwebview-ui/src/components/ChatPage/PermissionReviewMessage.csswebview-ui/src/components/ChatPage/PermissionReviewMessage.tsxwebview-ui/src/components/ChatPage/ResourceBlock.tsxwebview-ui/src/components/ChatPage/ResourceLink.tsxwebview-ui/src/components/ChatPage/ThinkingIndicator.tsxwebview-ui/src/components/ChatPage/providerOptions.tswebview-ui/src/components/ProfileManager/ProfileList.tsxwebview-ui/src/components/ResolutionsPage/MessageWrapper.csswebview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.tswebview-ui/src/components/ResolutionsPage/ReceivedMessage.tsxwebview-ui/src/components/ResolutionsPage/ResolutionsPage.tsxwebview-ui/src/components/ResolutionsPage/ToolMessage.tsxwebview-ui/src/components/ResolutionsPage/toolMessage.csswebview-ui/src/components/ViolationIncidentsList.tsxwebview-ui/src/hooks/actions.tswebview-ui/src/hooks/useContainerWidth.tswebview-ui/src/hooks/useScrollManagement.tswebview-ui/src/hooks/useVSCodeMessageHandler.tswebview-ui/src/store/store.ts
💤 Files with no reviewable changes (5)
- vscode/core/src/tests/mocks.ts
- webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx
- webview-ui/src/hooks/actions.ts
- webview-ui/src/components/AnalysisPage/AnalysisPage.tsx
- vscode/core/src/solutionWorkflowOrchestrator.ts
| export interface ProviderOption { | ||
| id: string; | ||
| name: string; | ||
| envVars: ProviderEnvVar[]; | ||
| commonModels: string[]; | ||
| } |
There was a problem hiding this comment.
This schema cannot produce a valid Azure configuration.
AgentSettings is driven by envVars and commonModels, but Azure also needs non-env required args like azureOpenAIApiDeploymentName and azureOpenAIApiVersion from the shipped sample config. With the current shape, selecting Azure in the settings UI can only persist an incomplete active block, so provider initialization fails later.
Patch sketch
export interface ProviderOption {
id: string;
name: string;
envVars: ProviderEnvVar[];
commonModels: string[];
+ args?: Array<{
+ key: string;
+ label: string;
+ required?: boolean;
+ }>;
}
...
{
id: "azure",
name: "Azure OpenAI",
envVars: [
{ key: "AZURE_OPENAI_API_KEY", label: "API Key", isSecret: true },
{ key: "AZURE_OPENAI_ENDPOINT", label: "Endpoint" },
],
commonModels: [],
+ args: [
+ { key: "azureOpenAIApiDeploymentName", label: "Deployment name", required: true },
+ { key: "azureOpenAIApiVersion", label: "API version", required: true },
+ ],
},The form layer then needs to render and serialize args alongside envVars.
Also applies to: 50-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webview-ui/src/components/ChatPage/providerOptions.ts` around lines 7 - 12,
The ProviderOption schema lacks a place to persist non-environment configuration
required by some providers (e.g. Azure), causing incomplete active configs and
initialization failures; update the ProviderOption interface used by the form
(and any AgentSettings serialization) to include a new args: { [key: string]:
string } (or a typed args array) so the UI can render and serialize
provider-specific fields like azureOpenAIApiDeploymentName and
azureOpenAIApiVersion alongside envVars and commonModels, and ensure the form
layer that builds AgentSettings reads/writes this args field when
selecting/saving a provider.
Agentic package improvements and LLM reliability fixes. No runtime behavior changes to the VS Code extension UI — safe to merge independently. Removed collect-assets, provider, husky, go/java extension, and build changes that overlap with konveyor#1379. ### What's included - **Agentic package** — refactored base node with 5-min LLM timeout, improved caching, updated types (originalContent, result fields) - **LLM reliability** — warn-level logging when LLM returns no response, timeout protection against hanging requests - **Changelog** — added new fragments for pluggable agent backend and duplicate loading indicator fix ### Stack order This is **PR 1 of 3** breaking up the feature/goose branch: 1. **→ Agentic refactoring & LLM improvements** (this PR) 2. Pluggable agent backend → depends on this 3. Chat panel UI → depends on konveyor#2 Signed-off-by: ibolton336 <ibolton@redhat.com>
- husky pre-commit: respect existing NVM_DIR with parameter expansion fallback - base.ts: gate error emission on emitResponseChunks to avoid surfacing internal "thinking" operation errors to users Signed-off-by: ibolton336 <ibolton@redhat.com>
Adds the complete pluggable agent backend infrastructure: - Agent client interface with Goose and OpenCode implementations - MCP bridge server for exposing analysis tools to agents - Agent orchestrator with file tracking and batch review - State management refactoring (StateChangeData pattern, extension store, sync bridges) - Tool permission handler with policy-based access control - Agent configuration and credential storage - Shared agent types, actions, and message contracts - Updated extension wiring for agent lifecycle management - Modified file handler improvements for agent workflows - Documentation and contributing guide Signed-off-by: ibolton336 <ibolton@redhat.com>
Integrates the agent chat panel, permission review UI, streaming message handler, and updated store/hooks to match the new StateChangeData messaging pattern from shared/. Signed-off-by: Ian Bolton <ibolton@redhat.com>
The webview-ui eslint config does not include the react-hooks plugin, so the disable comment causes a 'Definition for rule not found' error. Signed-off-by: Ian Bolton <ibolton@redhat.com>
Signed-off-by: Ian Bolton <ibolton@redhat.com>
Signed-off-by: Ian Bolton <ibolton@redhat.com>
Keeps originalContent and result fields, reverts KaiWorkflow trailing comma formatting and removes verbose JSDoc comment. Signed-off-by: Ian Bolton <ibolton@redhat.com>
fix-duplicate-loading-indicators.yaml describes a separate bugfix not included in this PR. Signed-off-by: Ian Bolton <ibolton@redhat.com>
The silly→warn logging upgrade is now in its own PR (konveyor#1391). Reverting to main versions to keep this PR focused. Signed-off-by: Ian Bolton <ibolton@redhat.com>
- Remove goose-acp-messaging-guide.md (reference doc, not needed for merge) - Remove CONTRIBUTING-FEATURES.md (can land separately) - Remove test-goose-streaming.sh (test script, not needed for merge) - Revert base.ts to main (LLM timeout extracted to PR konveyor#1392) Signed-off-by: Ian Bolton <ibolton@redhat.com>
Signed-off-by: Ian Bolton <ibolton@redhat.com>
…yor#1393, konveyor#1394) These changes are now in standalone PRs that should merge first: - konveyor#1393 — scroll management refactor + useContainerWidth - konveyor#1394 — ToolMessage UI refactor Signed-off-by: Ian Bolton <ibolton@redhat.com>
MCP server package and bridge are now in their own PR. Signed-off-by: Ian Bolton <ibolton@redhat.com>
- CRITICAL: Pass bridge bearer token to MCP sidecar via KONVEYOR_BRIDGE_TOKEN env - Add missing action types to WebviewActionType union (FILE_RESPONSE, BATCH_APPLY_ALL, etc.) - Track and clean up injected env vars in OpencodeAgentClient (prevent process.env pollution) - Fix fire-and-forget race in fileTracker.cacheFileBeforeWrite (track inflight reads) - Use fileURLToPath() for all file: URI conversions (fixes Windows paths + encoded chars) - Use existing fileUriToPath utility in handleFileResponse.ts - Validate parsed YAML doc before dereferencing in config.ts - Use semantic <button> for clickable file links in ResourceLink.tsx (accessibility) - Restore rehype-sanitize in ReceivedMessage.tsx - Fix nullish coalescing in ResourceBlock.tsx (|| → ??) - Fix deprecated CSS word-break: break-word → overflow-wrap: break-word - Fix useEffect dependency in AgentFileReview.tsx - Remove redundant ternary in AgentFileReview.tsx - Remove unsafe 'as any' casts in message type guards - Add error handling for agent restart (stop() failure doesn't block start()) - Add debug logging in AGENT_WEBVIEW_READY catch block Signed-off-by: Ian Bolton <ibolton@redhat.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
ibolton336
force-pushed
the
feature/pluggable-agent-backend
branch
from
425d3f2 to
a7673cf
Compare
- CRITICAL: Pass bridge bearer token to MCP sidecar env (init.ts) - Add missing action types to WebviewActionType union (actions.ts) - Track and clean up injected env keys in opencodeClient (env pollution) - Use workflow.removeListener instead of removeAllListeners (orchestrator) - Use fileURLToPath for cross-platform URI conversion (fileTracker, batchReviewHandlers) - Track inflight reads to prevent cache race condition (fileTracker) - Validate parsed YAML document before dereferencing (config.ts) - Use existing fileUriToPath utility (handleFileResponse) - Re-add rehypeSanitize for HTML sanitization (ReceivedMessage) - Use nullish coalescing for empty string handling (ResourceBlock) - Replace deprecated word-break: break-word (ChatPage.css) - Fix useEffect boolean dependency (AgentFileReview) - Remove redundant ternary expression (AgentFileReview) - Use semantic button element for accessibility (ResourceLink) - Remove unsafe 'as any' casts in type guards (messages.ts) - Add graceful error handling for agent restart (handlers.ts) - Use deep comparison for provider disambiguation (providerConfigGenerator) Signed-off-by: Ian Bolton <ibolton@redhat.com>
ibolton336
force-pushed
the
feature/pluggable-agent-backend
branch
from
a7673cf to
ffd34be
Compare
ExtensionStore is introduced in PR konveyor#1389. Use a minimal McpBridgeStore interface so this PR builds independently. Signed-off-by: Ian Bolton <ibolton@redhat.com> Signed-off-by: ibolton336 <ibolton@redhat.com>
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
vscode/core/src/paths.ts (2)
444-452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIgnore out-of-workspace files before consulting ignore patterns.
relative()returns..-prefixed paths for files outsideworkspaceRepo. Feeding those intoisIgnoredBy()can returnfalse, so partial analysis-on-save starts treating external files as workspace files again. Restore the out-of-workspace guard before the.konveyorand ignore-file checks.Suggested fix
-import { relative, dirname, join } from "node:path"; +import { relative, dirname, join, isAbsolute } from "node:path"; @@ const f = relative(fsPaths().workspaceRepo, uri.fsPath); _logger?.debug(`isUriIgnored: ${f}`); + + if (f.startsWith("..") || isAbsolute(f)) { + return true; + } // Always ignore .konveyor directory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/paths.ts` around lines 444 - 452, The relative path f (computed via relative(fsPaths().workspaceRepo, uri.fsPath)) can be a ".."-prefixed path for files outside the workspace and must be treated as ignored before running ignore-file logic; update the isUriIgnored flow so you first check for out-of-workspace (e.g. f starts with ".." or is exactly "..") and return true for those cases, then apply the existing .konveyor guard (f.startsWith(".konveyor/") || f === ".konveyor") and finally call isIgnoredBy(f); reference variables/functions: relative, fsPaths().workspaceRepo, f, .konveyor check, and isIgnoredBy.
186-188:⚠️ Potential issue | 🟠 MajorWrap both activation-time fetch calls in AbortController timeouts.
Both
fetch()calls at lines 188 and 226 currently run indefinitely during extension startup. On blackholed networks or unresponsive proxies, this stalls activation even though a fallback mechanism exists (user-configured analyzer path viaanalyzerPathsetting). Apply the timeout + AbortController pattern already established inHubConnectionManagerto both downloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/paths.ts` around lines 186 - 188, Wrap both activation-time fetch calls (the one calling fetch(sha256sumUrl) that assigns sha256Response and the other activation download fetch) with an AbortController timeout pattern like used in HubConnectionManager: create an AbortController, pass controller.signal to fetch, start a setTimeout to call controller.abort() after the same timeout value used by HubConnectionManager, and clear the timeout when the fetch completes; ensure you handle the abort/AbortError path so the code falls back to the user-configured analyzerPath instead of stalling activation. Use the existing symbols sha256Response / fetch(sha256sumUrl) and the other activation fetch call to locate the two sites to apply this pattern.webview-ui/src/hooks/useVSCodeMessageHandler.ts (1)
59-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear pending streaming writes before applying a full chat replacement.
If a throttled
CHAT_STREAMING_UPDATEis queued and aCHAT_STATE_CHANGEarrives before the timer fires, the delayed merge will run against the new array and can overwrite finalized content or tool-status changes with stale data. Reset the timer andpendingStreamingUpdateRefbeforesetChatMessages(...)on the structural path.Also applies to: 105-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/hooks/useVSCodeMessageHandler.ts` around lines 59 - 100, The throttled CHAT_STREAMING_UPDATE merge can overwrite a subsequent CHAT_STATE_CHANGE full replacement; before applying any full chat replacement inside useVSCodeMessageHandler (the CHAT_STATE_CHANGE handling that calls setChatMessages) clear throttleTimerRef.current and pendingStreamingUpdateRef.current (and clearTimeout if needed) so any queued streaming update is discarded; conversely, inside the streaming timer callback (the setTimeout created when handling CHAT_STREAMING_UPDATE) clear the pendingStreamingUpdateRef and throttleTimerRef before calling latestStore.setChatMessages(...) so you don’t apply a stale merge after a structural replace—update both code paths (the streaming timer and the CHAT_STATE_CHANGE path) to reset these refs/timeouts prior to modifying chat state.
♻️ Duplicate comments (2)
vscode/core/src/features/agent/fileTracker.ts (1)
270-304:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
scanForMissedChanges()still races in-flight snapshots.
doScan()waits forinflightReads, but the post-run scan does not. If a pre-write snapshot is still loading when this runs, that file never gets a stable baseline and the “missed changes” pass can skip it entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/fileTracker.ts` around lines 270 - 304, scanForMissedChanges() can run while pre-write snapshots are still loading and miss files; ensure it waits for any in-flight snapshot reads before comparing cached originals to disk. Before iterating this.originalContentCache (and checking this.routedFiles), await the pending promises tracked in the inflightReads collection (e.g., Promise.all([...this.inflightReads.values()]) or equivalent) so all snapshot loads complete and produce stable baselines; then proceed to read disk, compare, log, and return missed changes.vscode/core/src/features/agent/toolPermissionHandler.ts (1)
197-217:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPermission previews can race the pre-write snapshot.
cacheFileBeforeWrite()kicks off the snapshot asynchronously, butgetOriginalContent()is called immediately afterward. When that read has not finished yet, this falls back to git/undefinedinstead of the working-tree baseline, so the user can approve against the wrong diff. This call path needs an awaitable snapshot step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/toolPermissionHandler.ts` around lines 197 - 217, The async snapshot kicked off by fileTracker.cacheFileBeforeWrite can still be in-flight when getOriginalContent is called, causing incorrect diffs; change the call site to await an ensured snapshot before reading: make cacheFileBeforeWrite return a Promise that resolves when the pre-write snapshot is complete (or add an awaitable method like fileTracker.waitForCachedSnapshot/cacheFileSnapshotComplete), then replace the fire-and-forget call with await fileTracker.cacheFileBeforeWrite(data.title, data.rawInput, workspaceRoot, data.toolCallId) (or await the new waitFor... API) so getOriginalContent(absPath, wsRoot) only runs after the snapshot finishes. Ensure the change references the existing symbols fileTracker.cacheFileBeforeWrite and fileTracker.getOriginalContent so the snapshot is completed before originalContent is fetched.
🟠 Major comments (22)
vscode/core/src/diff/vertical/manager.ts-26-45 (1)
26-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClosing a diffed document now leaves stale diff state behind.
After this refactor the manager only listens for edits. If the user closes the editor while a vertical diff is open, nothing clears
fileUriToHandler,fileUriToStreamId,activeDecorators, or the diff-visible context until some later manual cleanup path runs. Reintroduce tab/document-close cleanup here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/diff/vertical/manager.ts` around lines 26 - 45, Register a cleanup listener in the constructor (assign to userChangeListener) that listens for the editor/document/tab close event for this.fileEditor and calls a new cleanup routine which removes this file's entries from fileUriToHandler and fileUriToStreamId, clears activeDecorators for the file, and updates the diff-visible context (via mutate or the existing context updater) so no stale diff state remains; ensure the listener is disposed in any teardown/dispose path and that the cleanup routine is referenced by name from the constructor so the behavior is applied immediately when a diffed document is closed.vscode/core/src/features/agent/promptBuilder.ts-24-35 (1)
24-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe tool-mode instructions contradict each other.
Do not skip unchanged filesconflicts withOnly call write_file for files that need modifications.That ambiguity is likely to make the agent either rewrite every listed file or ignore the "complete file" requirement. Rephrase the first sentence to say not to omit unchanged portions of a file whenwrite_fileis used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/promptBuilder.ts` around lines 24 - 35, In promptBuilder.ts where the "instructions" array is built (the variable set by input.enableAgentMode), fix the contradictory sentences in the non-agent-mode branch: replace the pair "Do not skip unchanged files. Only call write_file for files that need modifications." with a single clear sentence that instructs the agent to include the complete file contents (including unchanged portions) when using write_file and to only invoke write_file for files that actually require changes; keep the rest of the non-agent-mode instructions unchanged.vscode/core/src/features/agent/promptBuilder.ts-103-110 (1)
103-110:⚠️ Potential issue | 🟠 MajorUse
fileURLToPath()instead ofnew URL(uri).pathnamefor file:// URI conversion.On Windows,
new URL(uri).pathnameproduces paths like/C:/Users/...with a leading slash, which causesfs.readFile(absPath)to fail and produces incorrect results inpath.relative(). The codebase already usesfileURLToPath()infileTracker.ts(same directory) and other utilities for this exact pattern. Import from"node:url"and apply the same approach.Suggested fix
import * as fs from "fs/promises"; import * as path from "path"; +import { fileURLToPath } from "node:url"; function uriToAbsolute(uri: string, workspaceDir: string): string { if (uri.startsWith("file://")) { - return new URL(uri).pathname; + return fileURLToPath(uri); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/promptBuilder.ts` around lines 103 - 110, The uriToAbsolute function uses new URL(uri).pathname which yields incorrect Windows paths; replace that with fileURLToPath from "node:url" to convert file:// URIs correctly. Import fileURLToPath at the top, update uriToAbsolute to call fileURLToPath(uri) when uri.startsWith("file://"), and keep the existing path.isAbsolute and path.join fallback logic unchanged; reference function name uriToAbsolute and ensure the import mirrors how fileURLToPath is used elsewhere in the codebase (e.g., fileTracker.ts).webview-ui/src/components/ChatPage/ChatPage.css-4-5 (1)
4-5:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the Stylelint errors in this file before merge.
container-name: chatPageand the matching@container chatPageusages violate the configured naming rules, and Stylelint also flags the missing blank lines before declarations in this block. Renaming the container to a kebab-case identifier and fixing the reported spacing issues will unblock lint.Also applies to: 1114-1119, 1185-1264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/ChatPage.css` around lines 4 - 5, The container declaration uses an invalid camelCase name and has spacing issues; rename the container-name value from chatPage to a kebab-case identifier (e.g., chat-page) and update every matching `@container` chatPage usage to `@container` chat-page, and fix Stylelint spacing by adding the required blank line(s) before the declarations in the container block(s) (check the container-name: and container-type: blocks and their corresponding `@container` rules between the mentioned ranges).vscode/core/src/webviewMessageHandler.ts-602-607 (1)
602-607:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis action now carries secrets into the generic message logger.
UPDATE_MODEL_PROVIDER_CONFIGaccepts rawcredentials, andmessageHandler()still debug-logs every webview message viaJSON.stringify(message). That will write API keys to the extension logs unless this payload is redacted or excluded from the generic log path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/webviewMessageHandler.ts` around lines 602 - 607, The generic webview logger in messageHandler() currently JSON.stringify(message) and will log secrets carried by the UPDATE_MODEL_PROVIDER_CONFIG action; modify messageHandler() to detect the UPDATE_MODEL_PROVIDER_CONFIG action (or presence of the payload.credentials field) and redact or omit credentials before logging (e.g., replace payload.credentials with a masked value or remove it), ensuring the destructuring of payload ({ provider, model, credentials, agentMode }) remains unchanged for processing but that credentials are excluded from any generic logging path.vscode/core/src/features/agent/index.ts-72-73 (1)
72-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
workspaceRootbefore using it asworkspaceDir.In this PR,
workspaceRootis treated as a possiblefile://URI in multiple other call sites, but here it is passed straight into Goose/OpenCode. If the store still holds the URI form, the backend gets a URI string instead of a filesystem working directory and path resolution will fail.Suggested fix
- const workspaceDir = ctx.store.getState().workspaceRoot; + const workspaceRoot = ctx.store.getState().workspaceRoot; + const workspaceDir = workspaceRoot.startsWith("file://") + ? vscode.Uri.parse(workspaceRoot).fsPath + : workspaceRoot;Also applies to: 126-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/index.ts` around lines 72 - 73, Normalize the workspaceRoot retrieved from ctx.store.getState().workspaceRoot before passing it into the agent backend: detect and convert a file:// URI to a proper filesystem path (e.g., strip the file:// scheme and decode) and then assign that normalized path to workspaceDir so getConfigAgentBackend() / Goose/OpenCode receive a valid filesystem directory; apply the same normalization wherever workspaceRoot is read (including the other usages around the block referenced at lines ~126-142) to ensure path resolution works correctly.vscode/core/src/commands.ts-861-876 (1)
861-876:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't fall back to disk when
originalContentis intentionally empty.Here
""is a valid sentinel for “new file”, but the newif (!originalContent)branch treats it as cache-miss and rereads the already-modified file from disk. That makes new files look like existing files and breaks the “skip decorator view for new file” path.Suggested fix
- const cachedFileState = state.modifiedFiles.get(uri.fsPath); - let originalContent = cachedFileState?.originalContent ?? ""; - - if (!originalContent) { + const cachedFileState = state.modifiedFiles.get(uri.fsPath); + const hasCachedOriginalContent = cachedFileState?.originalContent !== undefined; + let originalContent = cachedFileState?.originalContent ?? ""; + + if (!hasCachedOriginalContent) { try { const doc = await workspace.openTextDocument(uri); originalContent = doc.getText(); } catch { logger.debug(`File not found, treating as new file: ${filePath}`); originalContent = ""; } }Also applies to: 878-885
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/commands.ts` around lines 861 - 876, The code treats an empty string originalContent as a cache miss because it uses a falsy check; change the logic to only fall back to disk when cachedFileState?.originalContent is strictly undefined (or null) so that an intentional "" (new file sentinel) is preserved. Concretely, initialize let originalContent = cachedFileState?.originalContent; and replace the if (!originalContent) check with if (originalContent === undefined) before calling workspace.openTextDocument (same change for the repeated block around the 878-885 area), keeping the existing error handling and final assignment of "" on file-not-found.webview-ui/src/components/ChatPage/AgentFileReview.tsx-217-217 (1)
217-217:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable batch actions while an individual file response is still processing.
handleAccept/handleRejectadd the token toprocessingTokens, but the header buttons only key offisBatchOperationInProgress. That leaves “Accept All” / “Reject All” clickable while a per-file response is already in flight, so the same file can be sent through both paths.Suggested fix
- const busy = isBatchOperationInProgress; + const busy = isBatchOperationInProgress || processingTokens.size > 0;Also applies to: 244-267, 269-294, 366-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/AgentFileReview.tsx` at line 217, Header batch actions are only keyed off isBatchOperationInProgress, so per-file operations (handleAccept/handleReject) that add tokens to processingTokens don't disable the "Accept All"/"Reject All" header buttons; change the guarding condition to also check processingTokens (e.g. treat busy = isBatchOperationInProgress || processingTokens.size > 0 or processingTokens.length > 0 depending on the collection) and update every place that currently uses isBatchOperationInProgress (including the header button disable logic and the other checks referenced around the handleAccept/handleReject usages) to use this combined busy variable so batch actions are disabled while any per-file token is in processingTokens.vscode/core/src/webviewMessageHandler.ts-624-638 (1)
624-638:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersist provider/model changes even when the user leaves credentials blank.
This only writes
provider-settings.yamlwhenhasCredentialsis true. That drops provider/model updates for providers that need no credentials and for the “Stored” flow where the UI explicitly tells the user to leave secrets blank to keep the current ones.Suggested fix
- if (provider && model && hasCredentials) { + if (provider && model) { const { paths } = await import("./paths"); - const yamlContent = generateProviderSettingsYaml(provider, model, credentials); + const yamlContent = generateProviderSettingsYaml(provider, model, credentials ?? {}); const encoder = new TextEncoder(); await vscode.workspace.fs.writeFile(paths().settingsYaml, encoder.encode(yamlContent));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/webviewMessageHandler.ts` around lines 624 - 638, The current guard uses hasCredentials so provider/model changes are only persisted when credentials are non-empty; change the condition in webviewMessageHandler.ts from "if (provider && model && hasCredentials)" to simply "if (provider && model)" so generateProviderSettingsYaml(provider, model, credentials) is always written to paths().settingsYaml (via vscode.workspace.fs.writeFile) even when credentials are blank, ensuring providers that don't require secrets and the "Stored" flow aren’t dropped; keep the rest of the logic (logger.info and vscode.window.showInformationMessage) unchanged.vscode/core/src/webviewMessageHandler.ts-471-479 (1)
471-479:⚠️ Potential issue | 🟠 MajorUse
vscode.Uri.parse().fsPathfor Windows path compatibility.The
new URL(wsRoot).pathnameapproach produces/C:/repo(with leading slash) on Windows, whichpath.join()preserves, creating an invalid path. The codebase already usesvscode.Uri.parse(uri).fsPathincommands.tsfor this exact purpose—it correctly handles Windows paths by removing the leading slash.Suggested fix
- if (!isAbsolute(path)) { - let wsRoot = state.data.workspaceRoot; - if (wsRoot.startsWith("file://")) { - wsRoot = new URL(wsRoot).pathname; - } - absPath = join(wsRoot, path); - } + if (!isAbsolute(path)) { + let wsRoot = state.data.workspaceRoot; + if (wsRoot.startsWith("file://")) { + wsRoot = vscode.Uri.parse(wsRoot).fsPath; + } + absPath = join(wsRoot, path); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/webviewMessageHandler.ts` around lines 471 - 479, The code uses new URL(wsRoot).pathname which yields a leading slash on Windows (e.g., /C:/repo) and breaks path.join; update the workspaceRoot conversion to use vscode.Uri.parse(wsRoot).fsPath instead (replace new URL(wsRoot).pathname with Uri.parse(wsRoot).fsPath) so Windows paths are normalized, and ensure Uri is imported/available in webviewMessageHandler.ts (or use await import('vscode') to grab Uri) where the variables join, isAbsolute and absPath are handled.vscode/core/src/agentConfigReader.ts-35-44 (1)
35-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOpenCode provider/model updates are currently dropped.
readOpencodeConfig()reads provider/model fromprovider-settings.yaml, but the OpenCode branch ofwriteAgentConfig()is a no-op. A provider/model-only update will therefore read back the old values on the next refresh unless some other path happens to rewrite the YAML too.Also applies to: 90-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/agentConfigReader.ts` around lines 35 - 44, writeAgentConfig currently drops provider/model-only updates for the OpenCode backend because the OpenCode branch doesn't persist provider-settings.yaml; update writeOpencodeConfig (or add a helper like writeOpencodeProviderSettings) to accept and persist provider and model fields from WriteAgentConfigChanges into provider-settings.yaml (the same keys read by readOpencodeConfig), and ensure writeAgentConfig forwards the full changes object to writeOpencodeConfig; also mirror this fix for the other OpenCode handling at the other location referenced (lines ~90-96) so provider/model-only updates are saved and later read back correctly.vscode/core/src/client/opencodeClient.ts-147-154 (1)
147-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
allow_alwaysis currently mapped to one-time approval.The generic
includes("allow")branch also matches IDs likeallow_always, so a persistent approval gets downgraded to"once". Check the"always"case first, or match the option kind exactly.Suggested change
- if (optionId.includes("allow_once") || optionId.includes("allow")) { - response = "once"; - } else if (optionId.includes("always")) { + if (optionId.includes("always")) { response = "always"; + } else if (optionId.includes("allow_once") || optionId.includes("allow")) { + response = "once"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/client/opencodeClient.ts` around lines 147 - 154, The code in opencodeClient.ts misclassifies "allow_always" because the optionId.includes("allow") check runs before the "always" logic; update the logic in the block handling outcome?.outcome === "selected" (where optionId is derived and response is set) to check for the "always" case before the generic "allow" check or use exact-kind matching (e.g., optionId === "allow_always" or regex for whole-word kind) so that optionId values like "allow_always" map to response = "always" and only true one-time kinds map to "once".webview-ui/src/components/ChatPage/ChatPage.tsx-112-119 (1)
112-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear chat here doesn't clear the host-side conversation.
This only wipes the webview store. The extension store and agent session stay intact, so a later sync/reload can repopulate the old messages and the “new conversation” button does not actually start fresh. This path should round-trip to the extension host and reset the persisted chat/session there too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/ChatPage.tsx` around lines 112 - 119, handleClearChat currently only clears the webview store; after calling useExtensionStore.getState().setAgentMessages([]) and .clearChatMessages() you must also notify the extension host to reset the persisted conversation/session so it won't be repopulated on sync/reload—add a call from handleClearChat to the host RPC (e.g. use the existing postMessage/vscode API or the project's host API such as resetHostConversation / clearHostConversation) with a clearConversation/clearSession action, and ensure the extension host implements/handles that action to clear the extension-side agent session and persisted chat.webview-ui/src/components/ChatPage/ChatPage.tsx-387-401 (1)
387-401:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResource-only assistant turns can disappear.
When
msg.contentis empty,ReceivedMessageshort-circuits, so the resource blocks and cancelled label passed viaextraContentnever render. Add a fallback container for non-text content instead of routing everything throughReceivedMessage.
Based on learnings, theReceivedMessagecomponent doesn't render if it has no content.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/ChatPage.tsx` around lines 387 - 401, The resource-only assistant turns disappear because ReceivedMessage short-circuits when msg.content is empty; update the rendering in the ChatPage component (the JSX around MessageWrapper / ReceivedMessage) to detect when msg.content is falsy and render a fallback container that displays non-text children (extraContent, resource blocks, and the cancelled label) instead of passing them into ReceivedMessage; keep the existing ReceivedMessage usage for text messages (when msg.content exists) and ensure the fallback uses the same styling/structure so resource-only turns and msg.isCancelled still render.vscode/core/src/features/agent/fileTracker.ts-307-313 (1)
307-313:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
clear()can be undone by late snapshot completions.The outstanding
fs.readFile()promises keep running afterclear(). When one resolves, it can repopulateoriginalContentCachefor the next iteration, which leaks stale originals across runs. This needs an epoch/token or an await-before-clear path, not just clearing the maps.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/fileTracker.ts` around lines 307 - 313, clear() currently wipes maps but does not prevent in-flight fs.readFile() snapshot completions from re-populating originalContentCache; add a generation/token mechanism (e.g., this.generation number) on the FileTracker and increment it in clear(), capture the current generation when starting any async snapshot/read operation, and on promise resolution only write into originalContentCache/pendingToolFiles if the resolved generation matches this.generation; update places that set scanPromise or start snapshots to capture and check this token so late completions are ignored rather than repopulating state.vscode/core/src/client/directLLMClient.ts-52-77 (1)
52-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
getState()never reflectsstart()/stop().This backend always reports
"running", even beforestart()and afterstop(). Callers usegetState()for lifecycle decisions, so the UI can think the agent is active when it is not. Track the currentAgentStateon the instance, like the other backends do.Suggested change
export class DirectLLMClient extends EventEmitter implements AgentClient { + private agentState: AgentState = "stopped"; private readonly workflow: KaiInteractiveWorkflow; private readonly workflowInput: KaiInteractiveWorkflowInput; private readonly logger: winston.Logger; private promptActive = false; @@ async start(): Promise<void> { + this.agentState = "running"; this.emit("stateChange", "running"); } @@ async stop(): Promise<void> { try { this.workflow.stop(); } catch { // workflow may not be running } + this.agentState = "stopped"; this.emit("stateChange", "stopped"); } @@ getState(): AgentState { - return "running"; + return this.agentState; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/client/directLLMClient.ts` around lines 52 - 77, getState() currently always returns "running" and doesn't reflect lifecycle changes; add an instance property (e.g., this.state: AgentState initialized to "stopped" or "idle") and update it in start(), stop(), and dispose() to "running" and "stopped" respectively (ensure stop/dispose set state to "stopped" even if this.workflow.stop() throws), then have getState() return this.state; update any disposal logic around this.workflow.removeAllListeners() / this.removeAllListeners() to occur after setting the state so callers see the correct lifecycle.vscode/core/src/features/agent/handlers.ts-154-168 (1)
154-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the merged credential set when regenerating
provider-settings.yaml.This write path currently uses
payload.credentials, so editing one secret can drop previously stored keys from the YAML fallback even though they still exist in secret storage. That makes the DirectLLM fallback drift from the actual saved config.Suggested change
const yamlContent = generateProviderSettingsYaml( payload.provider, payload.model, - payload.credentials, + cleaned, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/handlers.ts` around lines 154 - 168, The write currently uses only payload.credentials which can drop keys already stored elsewhere; when regenerating provider-settings.yaml (the call to generateProviderSettingsYaml with payload.provider, payload.model, payload.credentials) you must merge the incoming payload.credentials with the existing/installed credentials before generating YAML. Fix by reading the current credential set (e.g., load and parse the existing file at paths().settingsYaml or fetch stored provider secrets for payload.provider), combine/override with payload.credentials to produce a merged credentials object, and pass that merged credentials object into generateProviderSettingsYaml so the file written to paths().settingsYaml reflects the complete credential set used by DirectLLMClient fallback.webview-ui/src/store/store.ts-417-420 (1)
417-420:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCap
agentMessagesthe same way regular chat is capped.These paths all append or replace agent messages without trimming, so long-running agent sessions can grow the Zustand state indefinitely.
Suggested fix
setAgentMessages: (messages) => set((state) => { - state.agentMessages = messages; + state.agentMessages = messages.slice(-MAX_CHAT_MESSAGES); }), @@ state.agentMessages.push(msg); + if (state.agentMessages.length > MAX_CHAT_MESSAGES) { + state.agentMessages = state.agentMessages.slice(-MAX_CHAT_MESSAGES); + } } @@ state.agentMessages.push(msg); + if (state.agentMessages.length > MAX_CHAT_MESSAGES) { + state.agentMessages = state.agentMessages.slice(-MAX_CHAT_MESSAGES); + } }Also applies to: 432-446, 519-531
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/store/store.ts` around lines 417 - 420, The agentMessages state setters (notably setAgentMessages and the other functions that append/replace agentMessages in this file) do not trim the array and can grow unbounded; update every mutator that writes to state.agentMessages to apply the same capping logic used for regular chat (either call the existing trim/cap utility or slice to keep only the last N items using the same MAX_CHAT_MESSAGES constant) so that after any assignment or append you set state.agentMessages = cappedArray (preserving newest messages).vscode/core/src/client/gooseClient.ts-247-275 (1)
247-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject overlapping prompts before reusing
currentResponseId.A second
sendMessage()while the first is still streaming overwrites the correlation id, so later chunks and tool events can land on the wrong chat message.Suggested fix
async sendMessage( content: string, responseMessageId: string, ): Promise<AcpPromptResponse["stopReason"]> { if (this.state !== "running" || !this.sessionId) { throw new Error("GooseClient: not running"); } + if (this.currentResponseId) { + throw new Error("GooseClient: prompt already in progress"); + } this.currentResponseId = responseMessageId;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/client/gooseClient.ts` around lines 247 - 275, sendMessage is overwriting this.currentResponseId when called while a previous send is still in progress, causing later chunks/events to attach to the wrong message; modify sendMessage to first check this.currentResponseId (and/or this.state === "streaming" if applicable) and reject/throw immediately if a prompt is already in flight, only set this.currentResponseId = responseMessageId after that guard, and ensure the existing reset logic (currently in the try/catch) remains in a finally-equivalent path so currentResponseId is reliably cleared when the request completes or errors; reference the sendMessage function, this.currentResponseId, and the emit("streamingComplete", ...) usage to locate and update the logic.vscode/core/src/features/agent/init.ts-363-365 (1)
363-365:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t treat every
"not found"startup error as a missing CLI.This will send users into the install flow for unrelated failures like missing models, files, or config. Restrict the fallback to known binary-discovery errors or
ENOENT.Suggested fix
- if (errorMsg.includes("binary not found") || errorMsg.includes("not found")) { + const isMissingBinary = + /binary not found/i.test(errorMsg) || /\bENOENT\b/.test(errorMsg); + + if (isMissingBinary) { await promptAgentInstall(agentClient, ctx); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/init.ts` around lines 363 - 365, The startup error handling currently treats any errorMsg containing "not found" as a missing CLI and calls promptAgentInstall(agentClient, ctx); tighten this by only triggering the install flow for true binary-discovery/ENOENT failures: check the error object for error.code === 'ENOENT' and/or match errorMsg against a strict regex like /binary.*not found/i or other known binary-discovery messages instead of a generic "not found" substring; update the conditional that references errorMsg (and the branches calling promptAgentInstall(agentClient, ctx)) so unrelated "not found" cases (missing models/files/config) fall through to the normal error handling.vscode/core/src/features/agent/init.ts-83-96 (1)
83-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to the analyzer startup poll.
If
analyzerClient.start()leavesserverStatein a transitional value, this MCPrun_analysiscall never resolves and the agent can hang on the tool forever.Suggested fix
- await new Promise<void>((resolve) => { + await new Promise<void>((resolve) => { + const startedAt = Date.now(); const checkInterval = setInterval(() => { if (analyzerClient.serverState === "running") { clearInterval(checkInterval); resolve(); } else if ( analyzerClient.serverState === "startFailed" || analyzerClient.serverState === "stopped" ) { clearInterval(checkInterval); resolve(); + } else if (Date.now() - startedAt > 30_000) { + clearInterval(checkInterval); + ctx.logger.warn("Timed out waiting for analyzer to start"); + resolve(); } }, 500); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/init.ts` around lines 83 - 96, The poll waiting for analyzerClient.serverState currently has no timeout so it can hang; modify the Promise inside init to include a startup timeout (e.g., const startupTimeoutMs = 30000) and set a setTimeout that clears the interval and rejects the Promise with a descriptive Error like "Analyzer startup timed out" if serverState doesn't reach "running" or a terminal state within that window; ensure you clear both the interval and the timeout on success/failure to avoid leaks and propagate the rejection so callers of the await (the run_analysis flow) can handle the failure.vscode/core/src/client/gooseClient.ts-11-18 (1)
11-18:⚠️ Potential issue | 🟠 MajorUse
fileURLToPath()forfile://workspace paths.
new URL(...).pathnamebreaks Windows drive letters and leaves percent-encoded characters in the path, causing Goose to start in the wrong working directory. Add the import and usefileURLToPath()instead.Suggested fix
import { ChildProcess, spawn, execFile } from "child_process"; import { EventEmitter } from "events"; import * as fs from "fs"; import * as os from "os"; import * as path from "path"; +import { fileURLToPath } from "url"; import winston from "winston"; import { AgentState } from "@editor-extensions/shared"; import type { AgentClient, McpServerConfig, PermissionRequestData } from "./agentClient"; @@ super(); // Normalize workspaceDir — it may arrive as a file:// URI from vscode if (config.workspaceDir.startsWith("file://")) { - config = { ...config, workspaceDir: new URL(config.workspaceDir).pathname }; + config = { ...config, workspaceDir: fileURLToPath(config.workspaceDir) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/client/gooseClient.ts` around lines 11 - 18, The code in gooseClient.ts currently uses new URL(...).pathname which breaks Windows drive letters and leaves percent-encoded characters; import fileURLToPath from "url" and replace any occurrences of new URL(someWorkspaceUri).pathname with fileURLToPath(new URL(someWorkspaceUri)) so the workspace path is correctly decoded and platform-correct; update the import list at the top to include fileURLToPath and adjust any variables or functions that construct the Goose working directory (search for usages of new URL(...).pathname) to use fileURLToPath instead.
🟡 Minor comments (3)
webview-ui/src/components/ChatPage/CompactMigrationScope.tsx-88-95 (1)
88-95:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
inc.uri+lineNumberis not unique enough for the list key.Multiple incidents can share the same file and line, so this can emit duplicate keys and reconcile the wrong button. Include a stable discriminator such as
violation_nameor the local index.Suggested fix
- {fileIncs.map((inc) => ( - <li key={`${inc.uri}-${inc.lineNumber}`}> + {fileIncs.map((inc, index) => ( + <li key={`${inc.uri}-${inc.lineNumber}-${inc.violation_name ?? index}`}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webview-ui/src/components/ChatPage/CompactMigrationScope.tsx` around lines 88 - 95, The list item key using `${inc.uri}-${inc.lineNumber}` in CompactMigrationScope (iterating fileIncs) can collide because multiple incidents may share file and line; update the key to include a stable discriminator such as inc.violation_name or the iteration index (e.g., `${inc.uri}-${inc.lineNumber}-${inc.violation_name}` or `${inc.uri}-${inc.lineNumber}-${idx}`) when rendering the <li> in the map that calls onIncidentSelect(inc) so each button has a truly unique key.vscode/core/src/gooseConfig.ts-63-72 (1)
63-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider distinguishing error types in write path.
The catch block at lines 70-72 silently swallows all errors, but
writeGooseConfigmay fail for reasons other than missing file (e.g., permission denied). While starting from scratch is fine for missing/malformed files, permission errors should likely propagate.🛡️ Proposed fix to handle permission errors
let raw: Record<string, any> = {}; try { const content = fs.readFileSync(configPath, "utf-8"); raw = (parse(content) as Record<string, any>) ?? {}; - } catch { - // Start from scratch if missing/malformed + } catch (err: any) { + // Start from scratch if missing/malformed, but rethrow permission errors + if (err?.code !== "ENOENT" && err?.code !== "ENOTFOUND") { + // Only swallow file-not-found errors; let permission errors propagate + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/gooseConfig.ts` around lines 63 - 72, The catch in writeGooseConfig that surrounds fs.readFileSync(getGooseConfigPath()) swallows all errors; change it to inspect the thrown error from fs.readFileSync/parse and only treat missing or parse failures as recoverable (e.g., when error.code === 'ENOENT' or a JSON/TOML parse error) while rethrowing permission-related errors (e.g., error.code === 'EACCES' or 'EPERM') so callers are notified; update the try/catch around fs.readFileSync and parse in writeGooseConfig to rethrow non-recoverable errors and keep the current behavior of initializing raw = {} for missing/malformed files.webview-ui/src/components/ChatPage/AgentFileReview.tsx-12-22 (1)
12-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat line-ending-only diffs as unchanged everywhere.
InlineDifffilters line-ending-only hunks, butcomputeDiffStats,totalStats, and thenoChanges/fileLabelchecks still use the raw diff. That produces contradictory UI for CRLF-only changes: the header shows a modified file with +/- counts and actions, while the body says “No meaningful changes”.Also applies to: 135-149, 176-179, 219-230
🧹 Nitpick comments (3)
vscode/core/src/features/agent/fileChangeRouter.ts (2)
91-93: 💤 Low valueConsider logging diff generation failures.
The catch block silently falls back to using raw content as the diff. Logging the error would aid debugging when diffs fail unexpectedly.
💡 Suggested improvement
- } catch { - diff = content; + } catch (err) { + console.warn(`Failed to generate diff for ${relativePath}:`, err); + diff = content; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/fileChangeRouter.ts` around lines 91 - 93, The catch block that falls back to diff = content in fileChangeRouter.ts should log the exception to help debugging; update the catch to capture the error (e.g., catch (err)) and call the existing logger (or processLogger) to record a warning/error with context (file path or identifier and that diff generation failed) before assigning diff = content so failures are visible; reference the diff variable and the content fallback in your change.
134-134: 💤 Low valueFire-and-forget with swallowed errors may hide issues.
While the fire-and-forget pattern is intentional here, completely swallowing errors makes debugging difficult if
changeAppliedfails silently.💡 Suggested improvement
- Promise.resolve(executeExtensionCommand("changeApplied", filePath, content)).catch(() => {}); + Promise.resolve(executeExtensionCommand("changeApplied", filePath, content)).catch((err) => { + console.warn(`changeApplied command failed for ${filePath}:`, err); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/features/agent/fileChangeRouter.ts` at line 134, The current fire-and-forget call Promise.resolve(executeExtensionCommand("changeApplied", filePath, content)).catch(() => {}) swallows all errors; update the catch to surface/log failures instead of ignoring them by capturing the error and logging contextual details (include the command name "changeApplied" and the filePath and/or content identifier) so failures in executeExtensionCommand are visible; locate this call in fileChangeRouter.ts and replace the empty catch with a call to the existing logger or console.error (or rethrow to a monitoring service) that records the error and context.vscode/core/src/utilities/ModifiedFiles/handleModifiedFile.ts (1)
21-27: Remove unused_processedTokensparameter if not required for interface compatibility.The
_processedTokensparameter is never used in the function body. If maintaining the current function signature is not necessary for API stability, consider removing it to simplify the interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vscode/core/src/utilities/ModifiedFiles/handleModifiedFile.ts` around lines 21 - 27, The function handleModifiedFileMessage currently declares an unused parameter _processedTokens; remove this parameter from the function signature to simplify the API (update the signature in ModifiedFiles/handleModifiedFile.ts to drop _processedTokens) and then update all call sites and any related type definitions or interfaces that reference handleModifiedFileMessage to match the new signature (search for handleModifiedFileMessage usages and adjust arguments accordingly), ensuring tests/consumers compile and no references to _processedTokens remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2bf970d0-2326-4060-a014-0e53284c6b32
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (85)
agentic/src/types.tschanges/unreleased/1351-pluggable-agent-backend.yamlshared/src/types/actions.tsshared/src/types/agent.tsshared/src/types/agentActions.tsshared/src/types/agentMessages.tsshared/src/types/index.tsshared/src/types/messages.tsshared/src/types/types.tsvscode/core/package.jsonvscode/core/src/KonveyorGUIWebviewViewProvider.tsvscode/core/src/__tests__/mocks.tsvscode/core/src/agentConfigReader.tsvscode/core/src/analysis/__tests__/batchedAnalysisTrigger.test.tsvscode/core/src/analysis/batchedAnalysisTrigger.tsvscode/core/src/analysis/runAnalysis.tsvscode/core/src/client/agentClient.tsvscode/core/src/client/analyzerClient.tsvscode/core/src/client/directLLMClient.tsvscode/core/src/client/gooseClient.tsvscode/core/src/client/opencodeClient.tsvscode/core/src/clients/ProfileSyncClient.tsvscode/core/src/commands.tsvscode/core/src/data/loadResults.tsvscode/core/src/diff/vertical/manager.tsvscode/core/src/extension.tsvscode/core/src/extensionState.tsvscode/core/src/features/agent/__tests__/toolPermissionHandler.test.tsvscode/core/src/features/agent/agentOrchestrator.tsvscode/core/src/features/agent/batchReviewHandlers.tsvscode/core/src/features/agent/fileChangeRouter.tsvscode/core/src/features/agent/fileTracker.tsvscode/core/src/features/agent/handlers.tsvscode/core/src/features/agent/index.tsvscode/core/src/features/agent/init.tsvscode/core/src/features/agent/promptBuilder.tsvscode/core/src/features/agent/toolPermissionHandler.tsvscode/core/src/features/agent/toolPermissionPolicy.tsvscode/core/src/features/featureRegistry.tsvscode/core/src/features/index.tsvscode/core/src/gooseConfig.tsvscode/core/src/modelProvider/config.tsvscode/core/src/modelProvider/providerConfigGenerator.tsvscode/core/src/paths.tsvscode/core/src/solutionWorkflowOrchestrator.tsvscode/core/src/store/extensionStore.tsvscode/core/src/store/syncBridges.tsvscode/core/src/utilities/ModifiedFiles/handleFileResponse.tsvscode/core/src/utilities/ModifiedFiles/handleModifiedFile.tsvscode/core/src/utilities/ModifiedFiles/handleQuickResponse.tsvscode/core/src/utilities/ModifiedFiles/processMessage.tsvscode/core/src/utilities/ModifiedFiles/processModifiedFile.tsvscode/core/src/utilities/ModifiedFiles/queueManager.tsvscode/core/src/utilities/agentCredentialStorage.tsvscode/core/src/utilities/configuration.tsvscode/core/src/utilities/gooseCredentialStorage.tsvscode/core/src/utilities/profiles/profileActions.tsvscode/core/src/utilities/profiles/profileService.tsvscode/core/src/webviewMessageHandler.tswebview-ui/package.jsonwebview-ui/src/App.tsxwebview-ui/src/components/AnalysisPage/AnalysisPage.tsxwebview-ui/src/components/ChatPage/AgentFileReview.csswebview-ui/src/components/ChatPage/AgentFileReview.tsxwebview-ui/src/components/ChatPage/AgentSettings.tsxwebview-ui/src/components/ChatPage/ChatPage.csswebview-ui/src/components/ChatPage/ChatPage.tsxwebview-ui/src/components/ChatPage/CompactBatchReview.csswebview-ui/src/components/ChatPage/CompactBatchReview.tsxwebview-ui/src/components/ChatPage/CompactMigrationScope.tsxwebview-ui/src/components/ChatPage/CompactModifiedFile.csswebview-ui/src/components/ChatPage/CompactModifiedFile.tsxwebview-ui/src/components/ChatPage/PermissionReviewMessage.csswebview-ui/src/components/ChatPage/PermissionReviewMessage.tsxwebview-ui/src/components/ChatPage/ResourceBlock.tsxwebview-ui/src/components/ChatPage/ResourceLink.tsxwebview-ui/src/components/ChatPage/ThinkingIndicator.tsxwebview-ui/src/components/ChatPage/providerOptions.tswebview-ui/src/components/ResolutionsPage/MessageWrapper.csswebview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.tswebview-ui/src/components/ResolutionsPage/ReceivedMessage.tsxwebview-ui/src/components/ResolutionsPage/ResolutionsPage.tsxwebview-ui/src/hooks/actions.tswebview-ui/src/hooks/useVSCodeMessageHandler.tswebview-ui/src/store/store.ts
💤 Files with no reviewable changes (5)
- vscode/core/src/tests/mocks.ts
- webview-ui/src/hooks/actions.ts
- webview-ui/src/components/ResolutionsPage/ResolutionsPage.tsx
- webview-ui/src/components/AnalysisPage/AnalysisPage.tsx
- vscode/core/src/solutionWorkflowOrchestrator.ts
✅ Files skipped from review due to trivial changes (17)
- changes/unreleased/1351-pluggable-agent-backend.yaml
- shared/src/types/index.ts
- agentic/src/types.ts
- webview-ui/src/components/ResolutionsPage/MessageWrapper.css
- webview-ui/src/components/ChatPage/CompactModifiedFile.css
- shared/src/types/agentActions.ts
- vscode/core/src/analysis/tests/batchedAnalysisTrigger.test.ts
- webview-ui/src/components/ChatPage/AgentFileReview.css
- webview-ui/src/components/ChatPage/providerOptions.ts
- vscode/core/src/utilities/gooseCredentialStorage.ts
- vscode/core/src/features/agent/toolPermissionPolicy.ts
- shared/src/types/agentMessages.ts
- vscode/core/src/utilities/agentCredentialStorage.ts
- webview-ui/src/components/ChatPage/PermissionReviewMessage.css
- shared/src/types/agent.ts
- webview-ui/src/components/ChatPage/CompactBatchReview.css
- vscode/core/src/utilities/profiles/profileService.ts
🚧 Files skipped from review as they are similar to previous changes (25)
- vscode/core/src/features/index.ts
- webview-ui/package.json
- vscode/core/src/modelProvider/config.ts
- vscode/core/src/utilities/ModifiedFiles/queueManager.ts
- vscode/core/src/analysis/runAnalysis.ts
- vscode/core/src/store/extensionStore.ts
- webview-ui/src/components/ChatPage/ResourceBlock.tsx
- webview-ui/src/components/ChatPage/ThinkingIndicator.tsx
- vscode/core/src/utilities/ModifiedFiles/handleQuickResponse.ts
- webview-ui/src/App.tsx
- webview-ui/src/components/ResolutionsPage/ModifiedFile/useModifiedFileData.ts
- webview-ui/src/components/ChatPage/CompactBatchReview.tsx
- webview-ui/src/components/ChatPage/CompactModifiedFile.tsx
- vscode/core/src/analysis/batchedAnalysisTrigger.ts
- webview-ui/src/components/ResolutionsPage/ReceivedMessage.tsx
- shared/src/types/types.ts
- shared/src/types/actions.ts
- vscode/core/src/extensionState.ts
- vscode/core/package.json
- vscode/core/src/features/agent/batchReviewHandlers.ts
- vscode/core/src/utilities/ModifiedFiles/handleFileResponse.ts
- vscode/core/src/features/agent/agentOrchestrator.ts
- webview-ui/src/components/ChatPage/PermissionReviewMessage.tsx
- vscode/core/src/KonveyorGUIWebviewViewProvider.ts
- webview-ui/src/components/ChatPage/ResourceLink.tsx
- Add mcpBridgeServer.ts stub (full impl in PR konveyor#1396) - Adapt checkBatchReviewComplete to use state.mutate() API (konveyor#1386 compat) - Revert removeListener back to removeAllListeners (KaiWorkflowEventEmitter lacks it) - Remove duplicate fileUriToPath import in handleFileResponse - Include useContainerWidth hook (from PR konveyor#1393) - Include updated useScrollManagement (from PR konveyor#1393) - Include ToolMessage with CollapsibleToolGroup/AgentToolGroup exports (from PR konveyor#1394) Signed-off-by: Ian Bolton <ibolton@redhat.com>
1 participant
Summary
Complete pluggable agent backend infrastructure with matching chat panel UI — enables the extension to drive external coding agents (Goose, OpenCode, Claude, Codex) for migration tasks. Replaces the monolithic
solutionWorkflowOrchestrator.tswith a modular agent architecture.No merge-order dependencies
This PR is self-contained — all required code is included. The following standalone PRs were extracted from this branch to keep individual diffs focused, but their content is already present here:
useContainerWidthhook (included in this PR)CollapsibleToolGroup/AgentToolGroup(included in this PR)When those PRs merge to
mainfirst, rebasing this PR will be a clean no-op for the overlapping files.Related standalone PRs (independent, can merge in any order):
What's included
Shared — New messaging pattern (
shared/src/types/):messages.ts— ConsolidatedStateChangeDatapattern replacing individual granular message typesagent.ts—AgentBackend,AgentState,AgentConfig,AgentCapabilitytypesagentActions.ts— Agent action contracts for extension↔webview communicationagentMessages.ts— Agent message types for state syncAgentic — Type additions (
types.ts):originalContent?: stringtoKaiModifiedFileresult?: stringtoKaiToolCallAgent clients (
vscode/core/src/client/):gooseClient.ts— Goose CLI integration with JSON-RPC/stdio streamingopencodeClient.ts— OpenCode SDK integration with SSE streamingagentClient.ts— abstract client interfacedirectLLMClient.ts— fallback LLM clientAgent orchestrator (
vscode/core/src/features/agent/):agentOrchestrator.ts— coordinates agent start/stop, message routing, statefileTracker.ts/fileChangeRouter.ts— monitor agent file modificationsbatchReviewHandlers.ts— multi-file review workflowstoolPermissionHandler.ts/toolPermissionPolicy.ts— policy-based tool access controlpromptBuilder.ts— agent interaction promptsMCP bridge server (
vscode/core/src/api/):mcpBridgeServer.ts— HTTP bridge for MCP server ↔ extension communication (stub; full impl in ✨ add MCP server package and VS Code bridge #1396)Chat panel UI (
webview-ui/src/components/ChatPage/):ChatPage.tsx— Main agent chat interface with streaming message displayAgentFileReview.tsx— File review UI for agent-modified filesAgentSettings.tsx— Agent backend configuration panelCompactBatchReview.tsx— Multi-file batch review interfacePermissionReviewMessage.tsx— Tool permission review UIThinkingIndicator.tsx— Agent thinking state indicatorWebview hooks & utilities:
useContainerWidth.ts— ResizeObserver-based container width hookuseScrollManagement.ts— Updated scroll management with agent message supportuseVSCodeMessageHandler.ts— Updated for newStateChangeDatamessaging patternToolMessage.tsx— Refactored withCollapsibleToolGroupandAgentToolGroupactions.ts— Agent mode toggle and new action typesstore.ts— Updated store shape for agent stateApp.tsx— Agent mode routingExtension wiring:
extensionStore.ts/syncBridges.ts— new store and state sync layerfeatureRegistry.ts— pluggable feature registrationextension.ts,commands.ts,webviewMessageHandler.ts,extensionState.tsChangelog:
1351-pluggable-agent-backend.yaml— Feature entryReview guide
Recommended review order:
shared/src/types/(messaging contracts)vscode/core/src/client/(Goose + OpenCode integration)vscode/core/src/features/agent/(core logic)extension.ts,webviewMessageHandler.ts,extensionStore.tswebview-ui/src/components/ChatPage/(rendering layer)Testing
toolPermissionHandler.test.ts— unit tests for tool permission handlerbatchedAnalysisTrigger.test.ts— updated for state changes