From bf5ed80e3bd402b53851283755e20514f93f11ed Mon Sep 17 00:00:00 2001 From: "matterai-app[bot]" Date: Wed, 3 Jun 2026 15:54:28 +0530 Subject: [PATCH 1/6] feat(terminal): capture login shell environment for correct PATH When VS Code is launched from macOS Dock/Finder, the extension inherits launchd's restricted PATH (/usr/bin:/bin) instead of the user's full shell PATH configured in .zshrc/.bashrc. This causes failures for CLI tools installed via Homebrew, nvm, cargo, and other package managers. Introduce ShellEnvironment.ts which captures the user's login shell env by running 'shell -l -c env' and caching the result. This mirrors the ShellSnapshot pattern used by ClaudeCode. Changes: - New ShellEnvironment.ts: captures login-shell env at startup, caches result, exposes getShellEnvironment()/getCapturedShell()/invalidateShellEnvironment() - ExecaTerminalProcess: use captured shell path (bash/zsh) instead of generic shell:true; use captured env instead of process.env so CLI tools on user PATH are reachable - extension.ts: eagerly capture shell environment during activation before any terminal processes are spawned - Updated tests to use invalidateShellEnvironment() in beforeEach/afterEach to ensure clean state, and loosen assertions for cross-platform safety --- src/extension.ts | 7 ++ .../terminal/ExecaTerminalProcess.ts | 16 +++- src/integrations/terminal/ShellEnvironment.ts | 93 +++++++++++++++++++ .../__tests__/ExecaTerminalProcess.spec.ts | 23 ++--- 4 files changed, 126 insertions(+), 13 deletions(-) create mode 100644 src/integrations/terminal/ShellEnvironment.ts diff --git a/src/extension.ts b/src/extension.ts index baf6d9d27b..ff1af00e3e 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -26,6 +26,7 @@ import { ClineProvider } from "./core/webview/ClineProvider" import { DIFF_VIEW_URI_SCHEME } from "./integrations/editor/DiffViewProvider" import { PlanEditorProvider } from "./integrations/editor/PlanEditorProvider" import { TerminalRegistry } from "./integrations/terminal/TerminalRegistry" +import { captureShellEnvironment } from "./integrations/terminal/ShellEnvironment" import { McpServerManager } from "./services/mcp/McpServerManager" import { CodeIndexManager } from "./services/code-index/manager" import { registerCommitMessageProvider } from "./services/commit-message" @@ -155,6 +156,12 @@ export async function activate(context: vscode.ExtensionContext) { // Initialize terminal shell execution handlers. TerminalRegistry.initialize() + // Eagerly capture the user's login shell environment so that CLI tools + // installed via Homebrew, nvm, cargo, etc. are on PATH even when VS Code + // was launched from macOS Dock/Finder (which inherits launchd's restricted + // PATH instead of the user's shell rc files). + captureShellEnvironment() + // Get default commands from configuration. const defaultCommands = vscode.workspace.getConfiguration(Package.name).get("allowedCommands") || [] diff --git a/src/integrations/terminal/ExecaTerminalProcess.ts b/src/integrations/terminal/ExecaTerminalProcess.ts index 8f9651f7fb..c88dad8dbe 100644 --- a/src/integrations/terminal/ExecaTerminalProcess.ts +++ b/src/integrations/terminal/ExecaTerminalProcess.ts @@ -4,6 +4,7 @@ import process from "process" import type { RooTerminal } from "./types" import { BaseTerminalProcess } from "./BaseTerminalProcess" +import { getShellEnvironment, getCapturedShell } from "./ShellEnvironment" export class ExecaTerminalProcess extends BaseTerminalProcess { private terminalRef: WeakRef @@ -38,13 +39,24 @@ export class ExecaTerminalProcess extends BaseTerminalProcess { try { this.isHot = true + // Use the user's real login shell (bash/zsh) instead of the + // default /bin/sh so that shell syntax matches user expectations. + // On Windows, process.env already carries the full system PATH; + // using shell:true (cmd.exe) is the safest default there. + const shellPath = process.platform === "win32" ? true : getCapturedShell() + + // Use the captured login-shell environment so that CLI tools + // installed via Homebrew, nvm, cargo, etc. are on PATH even when + // VS Code was launched from the macOS Dock/Finder. + const shellEnv = getShellEnvironment() + this.subprocess = execa({ - shell: true, + shell: shellPath, cwd: this.terminal.getCurrentWorkingDirectory(), all: true, stdin: "ignore", // kilocode_change: ignore stdin to prevent blocking env: { - ...process.env, + ...shellEnv, // Ensure UTF-8 encoding for Ruby, CocoaPods, etc. LANG: "en_US.UTF-8", LC_ALL: "en_US.UTF-8", diff --git a/src/integrations/terminal/ShellEnvironment.ts b/src/integrations/terminal/ShellEnvironment.ts new file mode 100644 index 0000000000..a684431ae6 --- /dev/null +++ b/src/integrations/terminal/ShellEnvironment.ts @@ -0,0 +1,93 @@ +import { execSync } from "child_process" +import { getShell } from "../../utils/shell" + +let cachedEnv: Record | null = null +let capturedShell: string | null = null + +/** + * Captures the user's full login shell environment by running their configured + * shell with the -l (login) flag and parsing the output of `env`. + * + * This is necessary because VS Code extensions launched from the macOS + * Dock/Finder inherit launchd's restricted PATH (/usr/bin:/bin) rather than + * the user's full shell PATH (which includes Homebrew, nvm, cargo, etc.). + * + * ClaudeCode uses the same pattern via ShellSnapshot.ts — running the user's + * interactive config and capturing the resulting environment. + */ +export function captureShellEnvironment(): Record { + if (cachedEnv) { + return cachedEnv + } + + const shell = getShell() + capturedShell = shell + + // On Windows, process.env inherits system PATH correctly from the + // installer/Start Menu launch path. No login-shell roundtrip needed. + if (process.platform === "win32") { + cachedEnv = { ...process.env } as Record + return cachedEnv + } + + try { + // Run the user's shell as a login shell and capture environment. + // stderr is piped so shell startup messages (motd, etc.) don't mix + // with the env output on stdout. + const output = execSync(`${shell} -l -c 'env'`, { + encoding: "utf8", + timeout: 5000, + env: process.env as Record, + stdio: ["ignore", "pipe", "pipe"], + }) + + const env: Record = {} + for (const line of output.split("\n")) { + const eqIdx = line.indexOf("=") + if (eqIdx > 0) { + const key = line.slice(0, eqIdx) + const value = line.slice(eqIdx + 1) + // Prefer the login-shell value; only keep process.env value + // when the login shell produces an empty string for a key + // that already exists in process.env. + if (value || !(key in process.env)) { + env[key] = value + } + } + } + + cachedEnv = env + console.log( + `[ShellEnvironment] Captured ${Object.keys(env).length} env vars from ${shell} -l (PATH=${env.PATH?.slice(0, 80)}...)`, + ) + return env + } catch (error) { + console.warn( + `[ShellEnvironment] Failed to capture shell environment: ${error instanceof Error ? error.message : String(error)}`, + ) + cachedEnv = { ...process.env } as Record + return cachedEnv + } +} + +/** Returns the cached environment, capturing on first call. */ +export function getShellEnvironment(): Record { + return cachedEnv ?? captureShellEnvironment() +} + +/** + * Returns the detected user shell binary (e.g. /bin/zsh). + * Falls back to getShell() if capture hasn't run yet. + */ +export function getCapturedShell(): string { + if (!capturedShell) { + captureShellEnvironment() + } + return capturedShell ?? getShell() +} + +/** Invalidate cached environment (e.g. on PATH changes at runtime). */ +export function invalidateShellEnvironment(): void { + cachedEnv = null + capturedShell = null +} diff --git a/src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts b/src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts index 873b8f85ab..8a7423a0c0 100644 --- a/src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts +++ b/src/integrations/terminal/__tests__/ExecaTerminalProcess.spec.ts @@ -23,6 +23,7 @@ vitest.mock("ps-tree", () => ({ import { execa } from "execa" import { ExecaTerminalProcess } from "../ExecaTerminalProcess" +import { invalidateShellEnvironment } from "../ShellEnvironment" import type { RooTerminal } from "../types" describe("ExecaTerminalProcess", () => { @@ -32,6 +33,7 @@ describe("ExecaTerminalProcess", () => { beforeEach(() => { originalEnv = { ...process.env } + invalidateShellEnvironment() mockTerminal = { provider: "execa", id: 1, @@ -52,6 +54,7 @@ describe("ExecaTerminalProcess", () => { afterEach(() => { process.env = originalEnv + invalidateShellEnvironment() vitest.clearAllMocks() }) @@ -59,17 +62,15 @@ describe("ExecaTerminalProcess", () => { it("should set LANG and LC_ALL to en_US.UTF-8", async () => { await terminalProcess.run("echo test") const execaMock = vitest.mocked(execa) - expect(execaMock).toHaveBeenCalledWith( - expect.objectContaining({ - shell: true, - cwd: "/test/cwd", - all: true, - env: expect.objectContaining({ - LANG: "en_US.UTF-8", - LC_ALL: "en_US.UTF-8", - }), - }), - ) + const calledOptions = execaMock.mock.calls[0][0] as any + // shell is true on Windows, a string path on Unix (e.g. /bin/zsh) + expect(calledOptions.shell).toBeTruthy() + expect(calledOptions.cwd).toBe("/test/cwd") + expect(calledOptions.all).toBe(true) + expect(calledOptions.env).toMatchObject({ + LANG: "en_US.UTF-8", + LC_ALL: "en_US.UTF-8", + }) }) it("should preserve existing environment variables", async () => { From 792baa757ac1aed87573004e6d2b59ec73b1635d Mon Sep 17 00:00:00 2001 From: "matterai-app[bot]" Date: Wed, 3 Jun 2026 15:54:59 +0530 Subject: [PATCH 2/6] fix(kilocode): handle stale model selections after extension updates When the extension's built-in model list changes (models added/removed), a user's saved kilocodeModel config may reference a model no longer in the list. This caused either empty model selections or 404 errors from the API. Add a defense-in-depth validation at two layers: 1. ClineProvider (startup validation): On initialization, check whether the stored kilocodeModel is in the static KILO_CODE_MODELS map via new isValidKilocodeModel(). If the model was removed, reset the config to default (undefined kilocodeModel) and log the stale model name. 2. KilocodeOpenrouterHandler (runtime safety net): In getModel(), if the selected model is not found in the fetched (live) model list, fall back to the default model. This catches edge cases where initialization validation hasn't run yet or the model was removed from the remote list. New export: - isValidKilocodeModel(modelId): simple lookup in KILO_CODE_MODELS map --- src/api/providers/kilocode-models.ts | 8 ++++++++ src/api/providers/kilocode-openrouter.ts | 8 ++++++++ src/core/webview/ClineProvider.ts | 16 ++++++++++++++++ 3 files changed, 32 insertions(+) diff --git a/src/api/providers/kilocode-models.ts b/src/api/providers/kilocode-models.ts index c80823657c..ce4015de10 100644 --- a/src/api/providers/kilocode-models.ts +++ b/src/api/providers/kilocode-models.ts @@ -132,3 +132,11 @@ export const KILO_CODE_MODELS: Record = { }, }, } + +/** + * Checks whether a given model ID is present in the static KiloCode model list. + * Used to detect stale model selections after extension updates remove models. + */ +export function isValidKilocodeModel(modelId: string): boolean { + return modelId in KILO_CODE_MODELS +} diff --git a/src/api/providers/kilocode-openrouter.ts b/src/api/providers/kilocode-openrouter.ts index 368dff5fbf..401578bef6 100644 --- a/src/api/providers/kilocode-openrouter.ts +++ b/src/api/providers/kilocode-openrouter.ts @@ -85,6 +85,14 @@ export class KilocodeOpenrouterHandler extends OpenRouterHandler { override getModel() { let id = this.options.kilocodeModel ?? this.defaultModel + + // Safety net: if the selected model is not in the fetched model list, + // fall back to the default. This handles stale config values that were + // not yet caught by ClineProvider's validation during initialization. + if (id && Object.keys(this.models).length > 0 && !this.models[id]) { + id = this.defaultModel + } + let info = this.models[id] ?? openRouterDefaultModelInfo // If a specific provider is requested, use the endpoint for that provider. diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6d451b815a..6fbdfb3591 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -105,6 +105,7 @@ import { OpenRouterHandler } from "../../api/providers" import { stringifyError } from "../../shared/kilocode/errorUtils" import isWsl from "is-wsl" import { getKilocodeDefaultModel } from "../../api/providers/kilocode/getKilocodeDefaultModel" +import { isValidKilocodeModel } from "../../api/providers/kilocode-models" import { getKiloCodeWrapperProperties } from "../../core/kilocode/wrapper" import { getKiloUrlFromToken } from "@roo-code/types" // kilocode_change import { getKilocodeConfig, getWorkspaceProjectId, KilocodeConfig } from "../../utils/kilo-config-file" // kilocode_change @@ -2290,6 +2291,21 @@ ${prompt} } } + // Validate global kilocodeModel against available models. + // If the stored model ID was removed from the extension's model list + // (e.g. after an update), force-reset to the default model. + if (apiConfiguration?.apiProvider === "kilocode" && apiConfiguration?.kilocodeModel) { + if (!isValidKilocodeModel(apiConfiguration.kilocodeModel)) { + const staleModel = apiConfiguration.kilocodeModel + const updatedConfig = { ...apiConfiguration, kilocodeModel: undefined } + await this.contextProxy.setProviderSettings(updatedConfig) + mergedApiConfiguration = { ...mergedApiConfiguration, kilocodeModel: undefined } + this.log( + `[ModelValidation] Reset stale kilocodeModel "${staleModel}" to default — model no longer available`, + ) + } + } + // forked_change start wrapper information const kiloCodeWrapperProperties = getKiloCodeWrapperProperties() const taskHistory = this.getTaskHistory() From 2c3f37cdf2afdd8a4d4909a8324d6352e236c7f6 Mon Sep 17 00:00:00 2001 From: "matterai-app[bot]" Date: Wed, 3 Jun 2026 15:55:29 +0530 Subject: [PATCH 3/6] chore(release): bump version to 6.2.6 Patch version bump for the shell environment capture feature and stale model validation fix. --- src/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/package.json b/src/package.json index ff5a0dd64c..b99d865d37 100644 --- a/src/package.json +++ b/src/package.json @@ -3,7 +3,7 @@ "displayName": "%extension.displayName%", "description": "%extension.description%", "publisher": "matterai", - "version": "6.2.5", + "version": "6.2.6", "icon": "assets/icons/matterai-ic.png", "galleryBanner": { "color": "#FFFFFF", From b41a8731f0369cc2a9a3cfaebde2617f20b9bb0d Mon Sep 17 00:00:00 2001 From: "matterai-app[bot]" Date: Wed, 3 Jun 2026 17:45:52 +0530 Subject: [PATCH 4/6] fix(tools): relax native tool schema requirements for stale model compatibility Relax required fields across all native tool parameter schemas to accommodate older/stale models that may omit optional parameters: - ask_followup_question: make follow_up optional - browser_action: make url, coordinate, size, text optional (only action required) - codebase_search: make path optional - execute_command: make cwd optional, add message field for approval context - generate_image: make image optional - list_files: make recursive optional - new_task: make todos optional - run_slash_command: make args optional - switch_mode: make reason optional Also handle "null" string values in codebaseSearchTool to prevent validation failures when stale models pass null as a literal string. --- .../prompts/tools/native-tools/ask_followup_question.ts | 2 +- src/core/prompts/tools/native-tools/browser_action.ts | 2 +- src/core/prompts/tools/native-tools/codebase_search.ts | 2 +- src/core/prompts/tools/native-tools/execute_command.ts | 7 ++++++- src/core/prompts/tools/native-tools/generate_image.ts | 2 +- src/core/prompts/tools/native-tools/list_files.ts | 2 +- src/core/prompts/tools/native-tools/new_task.ts | 2 +- src/core/prompts/tools/native-tools/run_slash_command.ts | 2 +- src/core/prompts/tools/native-tools/switch_mode.ts | 2 +- src/core/tools/codebaseSearchTool.ts | 3 ++- 10 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/core/prompts/tools/native-tools/ask_followup_question.ts b/src/core/prompts/tools/native-tools/ask_followup_question.ts index d2ef7df9d6..89ce5249cd 100644 --- a/src/core/prompts/tools/native-tools/ask_followup_question.ts +++ b/src/core/prompts/tools/native-tools/ask_followup_question.ts @@ -38,7 +38,7 @@ export default { maxItems: 4, }, }, - required: ["question", "follow_up"], + required: ["question"], additionalProperties: false, }, }, diff --git a/src/core/prompts/tools/native-tools/browser_action.ts b/src/core/prompts/tools/native-tools/browser_action.ts index 6f5df50a0c..5e3378a5a9 100644 --- a/src/core/prompts/tools/native-tools/browser_action.ts +++ b/src/core/prompts/tools/native-tools/browser_action.ts @@ -57,7 +57,7 @@ export default { description: "Text to type when performing the type action", }, }, - required: ["action", "url", "coordinate", "size", "text"], + required: ["action"], additionalProperties: false, }, }, diff --git a/src/core/prompts/tools/native-tools/codebase_search.ts b/src/core/prompts/tools/native-tools/codebase_search.ts index 6ca1692c12..2764fe2d3d 100644 --- a/src/core/prompts/tools/native-tools/codebase_search.ts +++ b/src/core/prompts/tools/native-tools/codebase_search.ts @@ -19,7 +19,7 @@ export default { description: "Optional subdirectory (relative to the workspace) to limit the search scope", }, }, - required: ["query", "path"], + required: ["query"], additionalProperties: false, }, }, diff --git a/src/core/prompts/tools/native-tools/execute_command.ts b/src/core/prompts/tools/native-tools/execute_command.ts index 76f3a79f8f..cb9b286b8b 100644 --- a/src/core/prompts/tools/native-tools/execute_command.ts +++ b/src/core/prompts/tools/native-tools/execute_command.ts @@ -18,8 +18,13 @@ export default { type: ["string", "null"], description: "Optional working directory for the command, relative or absolute", }, + message: { + type: "string", + description: + "A clear, concise one-line description of what the command does, shown to the user for approval (e.g. 'Install project dependencies with npm')", + }, }, - required: ["command", "cwd"], + required: ["command"], additionalProperties: false, }, }, diff --git a/src/core/prompts/tools/native-tools/generate_image.ts b/src/core/prompts/tools/native-tools/generate_image.ts index fb5fef8075..5a437c506d 100644 --- a/src/core/prompts/tools/native-tools/generate_image.ts +++ b/src/core/prompts/tools/native-tools/generate_image.ts @@ -25,7 +25,7 @@ export default { "Optional path (relative to the workspace) to an existing image to edit; supports PNG, JPG, JPEG, GIF, and WEBP", }, }, - required: ["prompt", "path", "image"], + required: ["prompt", "path"], additionalProperties: false, }, }, diff --git a/src/core/prompts/tools/native-tools/list_files.ts b/src/core/prompts/tools/native-tools/list_files.ts index 5a2f9a83d9..163981baac 100644 --- a/src/core/prompts/tools/native-tools/list_files.ts +++ b/src/core/prompts/tools/native-tools/list_files.ts @@ -19,7 +19,7 @@ export default { description: "Set true to list contents recursively; omit or false to show only the top level", }, }, - required: ["path", "recursive"], + required: ["path"], additionalProperties: false, }, }, diff --git a/src/core/prompts/tools/native-tools/new_task.ts b/src/core/prompts/tools/native-tools/new_task.ts index 7bcb386115..3f48e51a37 100644 --- a/src/core/prompts/tools/native-tools/new_task.ts +++ b/src/core/prompts/tools/native-tools/new_task.ts @@ -24,7 +24,7 @@ export default { "Optional initial todo list written as a markdown checklist; required when the workspace mandates todos", }, }, - required: ["mode", "message", "todos"], + required: ["mode", "message"], additionalProperties: false, }, }, diff --git a/src/core/prompts/tools/native-tools/run_slash_command.ts b/src/core/prompts/tools/native-tools/run_slash_command.ts index 7a3e78e39b..68ba2d4cc5 100644 --- a/src/core/prompts/tools/native-tools/run_slash_command.ts +++ b/src/core/prompts/tools/native-tools/run_slash_command.ts @@ -19,7 +19,7 @@ export default { description: "Optional additional context or arguments for the command", }, }, - required: ["command", "args"], + required: ["command"], additionalProperties: false, }, }, diff --git a/src/core/prompts/tools/native-tools/switch_mode.ts b/src/core/prompts/tools/native-tools/switch_mode.ts index 0a9ab1815d..8c8fca0f9b 100644 --- a/src/core/prompts/tools/native-tools/switch_mode.ts +++ b/src/core/prompts/tools/native-tools/switch_mode.ts @@ -19,7 +19,7 @@ export default { description: "Optional explanation for why the mode switch is needed", }, }, - required: ["mode_slug", "reason"], + required: ["mode_slug"], additionalProperties: false, }, }, diff --git a/src/core/tools/codebaseSearchTool.ts b/src/core/tools/codebaseSearchTool.ts index 7af78cd75b..bd78a307b3 100644 --- a/src/core/tools/codebaseSearchTool.ts +++ b/src/core/tools/codebaseSearchTool.ts @@ -27,7 +27,8 @@ export async function codebaseSearchTool( // --- Parameter Extraction and Validation --- let query: string | undefined = block.params.query - let directoryPrefix: string | undefined = block.params.path + const rawPath: string | undefined = block.params.path + let directoryPrefix: string | undefined = rawPath && rawPath !== "null" ? rawPath : undefined query = removeClosingTag("query", query) From 5bfd6586bd8a83ae14576e5316ca4505f7eeedb0 Mon Sep 17 00:00:00 2001 From: "matterai-app[bot]" Date: Wed, 3 Jun 2026 17:46:24 +0530 Subject: [PATCH 5/6] feat(ui): add command approval mode with option selection and keyboard navigation Add an interactive approval mode to CommandExecution that presents users with three numbered options before running a command: - Yes: approve the command - Yes, and dont ask again: approve and remember the command pattern - No, with feedback: reject and provide guidance Support keyboard navigation (ArrowUp/Down, Enter, Escape) for quick approvals. Also add a custom message field in executeCommand so the model can describe the command purpose to the user before approval. Update parseCommandAndOutput to extract a MESSAGE: prefix from the command text, enabling the approval header display. --- src/core/tools/executeCommandTool.ts | 7 +- .../src/components/chat/CommandExecution.tsx | 239 ++++++++++++++++-- 2 files changed, 225 insertions(+), 21 deletions(-) diff --git a/src/core/tools/executeCommandTool.ts b/src/core/tools/executeCommandTool.ts index bbc67cb324..b3f54239d8 100644 --- a/src/core/tools/executeCommandTool.ts +++ b/src/core/tools/executeCommandTool.ts @@ -29,7 +29,9 @@ export async function executeCommandTool( removeClosingTag: RemoveClosingTag, ) { let command: string | undefined = block.params.command - const customCwd: string | undefined = block.params.cwd + const rawCwd: string | undefined = block.params.cwd + const customCwd: string | undefined = rawCwd && rawCwd !== "null" ? rawCwd : undefined + const customMessage: string | undefined = block.params.message try { if (block.partial) { @@ -54,7 +56,8 @@ export async function executeCommandTool( task.consecutiveMistakeCount = 0 command = unescapeHtmlEntities(command) // Unescape HTML entities. - const didApprove = await askApproval("command", command) + const askText = customMessage ? `MESSAGE:${customMessage}\n---\n${command}` : command + const didApprove = await askApproval("command", askText) if (!didApprove) { return diff --git a/webview-ui/src/components/chat/CommandExecution.tsx b/webview-ui/src/components/chat/CommandExecution.tsx index ba67e7db09..1040c9fc49 100644 --- a/webview-ui/src/components/chat/CommandExecution.tsx +++ b/webview-ui/src/components/chat/CommandExecution.tsx @@ -1,4 +1,4 @@ -import { ChevronDown, OctagonX } from "lucide-react" +import { ChevronDown, ChevronUp, CornerDownLeft, OctagonX } from "lucide-react" import { memo, useCallback, useEffect, useMemo, useRef, useState } from "react" import { useEvent } from "react-use" @@ -40,6 +40,20 @@ interface CommandExecutionProps { secondaryButtonText?: string } +type ApprovalOption = "yes" | "yes_always" | "no_feedback" + +interface ApprovalOptionDef { + key: ApprovalOption + label: string + shortLabel: string +} + +const APPROVAL_OPTIONS: ApprovalOptionDef[] = [ + { key: "yes", label: "Yes", shortLabel: "Yes" }, + { key: "yes_always", label: "Yes, and don't ask again for this command", shortLabel: "Yes, don't ask again" }, + { key: "no_feedback", label: "No, and tell Orbital the next step", shortLabel: "No, with feedback" }, +] + export const CommandExecution = memo( ({ executionId, @@ -61,10 +75,15 @@ export const CommandExecution = memo( setDeniedCommands, } = useExtensionState() - const { command, output: parsedOutput } = useMemo(() => parseCommandAndOutput(text), [text]) - - // If we aren't opening the VSCode terminal for this command then we default - // to expanding the command execution output. + const { + message: customMessage, + command, + output: parsedOutput, + } = useMemo(() => parseCommandAndOutput(text), [text]) + + // Approval mode state + const [selectedOption, setSelectedOption] = useState("yes") + const [feedbackText, setFeedbackText] = useState("") const [isExpanded, setIsExpanded] = useState(terminalShellIntegrationDisabled) const [streamingOutput, setStreamingOutput] = useState("") const [status, setStatus] = useState(null) @@ -72,6 +91,8 @@ export const CommandExecution = memo( const [completedSeconds, setCompletedSeconds] = useState(null) const startTimeRef = useRef(null) + const isApprovalMode = !!onPrimaryButtonClick && !!onSecondaryButtonClick && !!enableButtons && !status + // The command's output can either come from the text associated with the // task message (this is the case for completed commands) or from the // streaming output (this is the case for running commands). @@ -79,20 +100,15 @@ export const CommandExecution = memo( // Extract command patterns from the actual command that was executed const commandPatterns = useMemo(() => { - // First get all individual commands (including subshell commands) using parseCommand const allCommands = parseCommand(command) - - // Then extract patterns from each command using the existing pattern extraction logic const allPatterns = new Set() - // Add all individual commands first allCommands.forEach((cmd) => { if (cmd.trim()) { allPatterns.add(cmd.trim()) } }) - // Then add extracted patterns for each command allCommands.forEach((cmd) => { const patterns = extractPatternsFromCommand(cmd) patterns.forEach((pattern) => allPatterns.add(pattern)) @@ -168,13 +184,11 @@ export const CommandExecution = memo( if (status?.status === "started" && startTimeRef.current) { const startTime = startTimeRef.current - // Update elapsed time every second const interval = setInterval(() => { const elapsed = Math.floor((Date.now() - startTime) / 1000) setElapsedSeconds(elapsed) }, 1000) - // Initial calculation setElapsedSeconds(Math.floor((Date.now() - startTime) / 1000)) return () => clearInterval(interval) @@ -189,6 +203,178 @@ export const CommandExecution = memo( } }, [status]) + // Handle Submit button click based on selected option + const handleSubmit = useCallback(() => { + if (!onPrimaryButtonClick || !onSecondaryButtonClick) return + + switch (selectedOption) { + case "yes": + onPrimaryButtonClick() + break + case "yes_always": + // Add the command pattern to allowedCommands before approving + if (command && command.trim()) { + const pattern = command.trim() + if (!allowedCommands.includes(pattern)) { + const newAllowed = [...allowedCommands, pattern] + setAllowedCommands(newAllowed) + vscode.postMessage({ type: "allowedCommands", commands: newAllowed }) + } + } + onPrimaryButtonClick() + break + case "no_feedback": + onSecondaryButtonClick(feedbackText || undefined) + break + } + }, [ + selectedOption, + onPrimaryButtonClick, + onSecondaryButtonClick, + command, + allowedCommands, + setAllowedCommands, + feedbackText, + ]) + + // Handle Skip button click + const handleSkip = useCallback(() => { + if (onSecondaryButtonClick) { + onSecondaryButtonClick() + } + }, [onSecondaryButtonClick]) + + // Keyboard navigation for approval options + useEffect(() => { + if (!isApprovalMode) return + + const handleKeyDown = (e: KeyboardEvent) => { + const currentIndex = APPROVAL_OPTIONS.findIndex((o) => o.key === selectedOption) + + switch (e.key) { + case "ArrowUp": + e.preventDefault() + if (currentIndex > 0) { + setSelectedOption(APPROVAL_OPTIONS[currentIndex - 1].key) + } + break + case "ArrowDown": + e.preventDefault() + if (currentIndex < APPROVAL_OPTIONS.length - 1) { + setSelectedOption(APPROVAL_OPTIONS[currentIndex + 1].key) + } + break + case "Enter": + e.preventDefault() + handleSubmit() + break + case "Escape": + e.preventDefault() + handleSkip() + break + } + } + + window.addEventListener("keydown", handleKeyDown) + return () => window.removeEventListener("keydown", handleKeyDown) + }, [isApprovalMode, selectedOption, handleSubmit, handleSkip]) + + // Render approval mode UI + if (isApprovalMode) { + return ( +
+ {/* Header: custom message */} + {customMessage && ( +
+

{customMessage}

+
+ )} + + {/* Code block with command */} +
+
+
+
+ + {command} + +
+ {/* Expand button */} + +
+
+
+ + {/* Numbered options */} +
+
+ {APPROVAL_OPTIONS.map((option, idx) => { + const isSelected = selectedOption === option.key + return ( + + ) + })} + + {/* Feedback text area when "No with feedback" is selected */} + {selectedOption === "no_feedback" && ( +
+ setFeedbackText(e.target.value)} + onClick={(e) => e.stopPropagation()} + /> +
+ )} +
+
+ + {/* Footer: Skip + Submit */} +
+ + +
+
+ ) + } + + // Non-approval mode: existing running/completed UI return ( <>
@@ -275,7 +461,7 @@ export const CommandExecution = memo( /> )}
- {onPrimaryButtonClick && onSecondaryButtonClick && enableButtons && ( + {onPrimaryButtonClick && onSecondaryButtonClick && enableButtons && !isApprovalMode && (
{onRunEverythingClick && ( @@ -328,17 +514,32 @@ const OutputContainer = memo(OutputContainerInternal) const parseCommandAndOutput = (text: string | undefined) => { if (!text) { - return { command: "", output: "" } + return { message: "", command: "", output: "" } + } + + let message = "" + let remaining = text + + // Check for message prefix: "MESSAGE:...\n---\ncommand" + const messageSeparator = "\n---\n" + if (remaining.startsWith("MESSAGE:")) { + const separatorIdx = remaining.indexOf(messageSeparator) + if (separatorIdx !== -1) { + message = remaining.slice(8, separatorIdx) // after "MESSAGE:" + remaining = remaining.slice(separatorIdx + messageSeparator.length) + } } - const index = text.indexOf(COMMAND_OUTPUT_STRING) + // Parse command and output + const outputIdx = remaining.indexOf(COMMAND_OUTPUT_STRING) - if (index === -1) { - return { command: text, output: "" } + if (outputIdx === -1) { + return { message, command: remaining, output: "" } } return { - command: text.slice(0, index), - output: text.slice(index + COMMAND_OUTPUT_STRING.length), + message, + command: remaining.slice(0, outputIdx), + output: remaining.slice(outputIdx + COMMAND_OUTPUT_STRING.length), } } From ad8cb0acfce059e53a1b0581507d0894adccf640 Mon Sep 17 00:00:00 2001 From: code-crusher Date: Wed, 3 Jun 2026 19:21:18 +0530 Subject: [PATCH 6/6] fix: address PR #87 review comments - ShellEnvironment: use execFileSync instead of execSync for command injection prevention - ShellEnvironment: add setShellLogger() for diagnostic output via outputChannel - ShellEnvironment: fall back to process.env when login shell produces empty value - CommandExecution: add commandRef to prevent stale closure on streaming updates - CommandExecution: add mounted ref guard in keyboard useEffect cleanup - ClineProvider: await getKilocodeDefaultModel() for stale model reset - kilocode-openrouter: remove redundant Object.keys check in model lookup - executeCommandTool: guard against non-string customMessage values - extension.ts: wire ShellEnvironment logger to outputChannel - Add tests for ShellEnvironment, CommandExecution approval mode, ClineProvider model validation --- src/api/providers/kilocode-openrouter.ts | 2 +- src/core/tools/executeCommandTool.ts | 4 +- src/core/webview/ClineProvider.ts | 7 +- .../webview/__tests__/ClineProvider.spec.ts | 68 ++++++ src/extension.ts | 3 +- src/integrations/terminal/ShellEnvironment.ts | 33 ++- .../__tests__/ShellEnvironment.spec.ts | 201 +++++++++++++++++ .../src/components/chat/CommandExecution.tsx | 25 ++- .../chat/__tests__/CommandExecution.spec.tsx | 209 ++++++++++++++++++ 9 files changed, 533 insertions(+), 19 deletions(-) create mode 100644 src/integrations/terminal/__tests__/ShellEnvironment.spec.ts diff --git a/src/api/providers/kilocode-openrouter.ts b/src/api/providers/kilocode-openrouter.ts index 401578bef6..72ccc34c9e 100644 --- a/src/api/providers/kilocode-openrouter.ts +++ b/src/api/providers/kilocode-openrouter.ts @@ -89,7 +89,7 @@ export class KilocodeOpenrouterHandler extends OpenRouterHandler { // Safety net: if the selected model is not in the fetched model list, // fall back to the default. This handles stale config values that were // not yet caught by ClineProvider's validation during initialization. - if (id && Object.keys(this.models).length > 0 && !this.models[id]) { + if (id && !this.models[id]) { id = this.defaultModel } diff --git a/src/core/tools/executeCommandTool.ts b/src/core/tools/executeCommandTool.ts index b3f54239d8..05f8989037 100644 --- a/src/core/tools/executeCommandTool.ts +++ b/src/core/tools/executeCommandTool.ts @@ -31,7 +31,9 @@ export async function executeCommandTool( let command: string | undefined = block.params.command const rawCwd: string | undefined = block.params.cwd const customCwd: string | undefined = rawCwd && rawCwd !== "null" ? rawCwd : undefined - const customMessage: string | undefined = block.params.message + const rawMessage: unknown = block.params.message + const customMessage: string | undefined = + typeof rawMessage === "string" && rawMessage !== "null" ? rawMessage : undefined try { if (block.partial) { diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6fbdfb3591..6eb00324cc 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -2297,11 +2297,12 @@ ${prompt} if (apiConfiguration?.apiProvider === "kilocode" && apiConfiguration?.kilocodeModel) { if (!isValidKilocodeModel(apiConfiguration.kilocodeModel)) { const staleModel = apiConfiguration.kilocodeModel - const updatedConfig = { ...apiConfiguration, kilocodeModel: undefined } + const defaultModel = await getKilocodeDefaultModel() + const updatedConfig = { ...apiConfiguration, kilocodeModel: defaultModel } await this.contextProxy.setProviderSettings(updatedConfig) - mergedApiConfiguration = { ...mergedApiConfiguration, kilocodeModel: undefined } + mergedApiConfiguration = { ...mergedApiConfiguration, kilocodeModel: defaultModel } this.log( - `[ModelValidation] Reset stale kilocodeModel "${staleModel}" to default — model no longer available`, + `[ModelValidation] Reset stale kilocodeModel "${staleModel}" to default "${defaultModel}" — model no longer available`, ) } } diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index 656f0b5345..fb843c1971 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -2016,6 +2016,74 @@ describe("ClineProvider", () => { }) }) + describe("model validation on stale kilocodeModel", () => { + let logSpy: any + + beforeEach(async () => { + await provider.resolveWebviewView(mockWebviewView) + logSpy = vi.spyOn(provider, "log").mockImplementation(() => {}) + }) + + afterEach(() => { + logSpy.mockRestore() + }) + + test("resets stale kilocodeModel to default when model not in KILO_CODE_MODELS", async () => { + const { isValidKilocodeModel } = await import("../../../api/providers/kilocode-models") + + const setSettingsSpy = vi.spyOn(provider.contextProxy, "setProviderSettings").mockResolvedValue() + const getSettingsSpy = vi.spyOn(provider.contextProxy, "getProviderSettings").mockReturnValue({ + apiProvider: "kilocode", + kilocodeModel: "non-existent-model-12345", + kilocodeToken: "test-token", + } as any) + + expect(isValidKilocodeModel("non-existent-model-12345")).toBe(false) + + const state = await provider.getState() + + expect(state.apiConfiguration?.kilocodeModel).not.toBe("non-existent-model-12345") + expect(logSpy).toHaveBeenCalledWith(expect.stringContaining("[ModelValidation] Reset stale kilocodeModel")) + expect(setSettingsSpy).toHaveBeenCalled() + getSettingsSpy.mockRestore() + setSettingsSpy.mockRestore() + }) + + test("preserves valid kilocodeModel when it exists in KILO_CODE_MODELS", async () => { + const { isValidKilocodeModel, KILO_CODE_MODELS } = await import("../../../api/providers/kilocode-models") + + const validModelId = Object.keys(KILO_CODE_MODELS)[0] + expect(isValidKilocodeModel(validModelId)).toBe(true) + + const getSettingsSpy = vi.spyOn(provider.contextProxy, "getProviderSettings").mockReturnValue({ + apiProvider: "kilocode", + kilocodeModel: validModelId, + kilocodeToken: "test-token", + } as any) + + const state = await provider.getState() + + expect(state.apiConfiguration?.kilocodeModel).toBe(validModelId) + expect(logSpy).not.toHaveBeenCalledWith( + expect.stringContaining("[ModelValidation] Reset stale kilocodeModel"), + ) + getSettingsSpy.mockRestore() + }) + + test("skips validation when apiProvider is not kilocode", async () => { + const getSettingsSpy = vi.spyOn(provider.contextProxy, "getProviderSettings").mockReturnValue({ + apiProvider: "openrouter", + openRouterModelId: "some-openrouter-model", + } as any) + + const state = await provider.getState() + + expect(state.apiConfiguration?.apiProvider).toBe("openrouter") + expect(logSpy).not.toHaveBeenCalledWith(expect.stringContaining("[ModelValidation]")) + getSettingsSpy.mockRestore() + }) + }) + describe("updateCustomMode", () => { test("updates both file and state when updating custom mode", async () => { await provider.resolveWebviewView(mockWebviewView) diff --git a/src/extension.ts b/src/extension.ts index ff1af00e3e..9ac04e64b7 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -26,7 +26,7 @@ import { ClineProvider } from "./core/webview/ClineProvider" import { DIFF_VIEW_URI_SCHEME } from "./integrations/editor/DiffViewProvider" import { PlanEditorProvider } from "./integrations/editor/PlanEditorProvider" import { TerminalRegistry } from "./integrations/terminal/TerminalRegistry" -import { captureShellEnvironment } from "./integrations/terminal/ShellEnvironment" +import { captureShellEnvironment, setShellLogger } from "./integrations/terminal/ShellEnvironment" import { McpServerManager } from "./services/mcp/McpServerManager" import { CodeIndexManager } from "./services/code-index/manager" import { registerCommitMessageProvider } from "./services/commit-message" @@ -160,6 +160,7 @@ export async function activate(context: vscode.ExtensionContext) { // installed via Homebrew, nvm, cargo, etc. are on PATH even when VS Code // was launched from macOS Dock/Finder (which inherits launchd's restricted // PATH instead of the user's shell rc files). + setShellLogger((msg: string) => outputChannel.appendLine(msg)) captureShellEnvironment() // Get default commands from configuration. diff --git a/src/integrations/terminal/ShellEnvironment.ts b/src/integrations/terminal/ShellEnvironment.ts index a684431ae6..19e5d4d672 100644 --- a/src/integrations/terminal/ShellEnvironment.ts +++ b/src/integrations/terminal/ShellEnvironment.ts @@ -1,13 +1,25 @@ -import { execSync } from "child_process" +import { execFileSync } from "child_process" import { getShell } from "../../utils/shell" let cachedEnv: Record | null = null let capturedShell: string | null = null +export type ShellLogger = (message: string) => void + +let logger: ShellLogger = console.log + +/** Sets the logger used for ShellEnvironment diagnostic messages. */ +export function setShellLogger(newLogger: ShellLogger): void { + logger = newLogger +} + /** * Captures the user's full login shell environment by running their configured * shell with the -l (login) flag and parsing the output of `env`. * + * Uses execFileSync (not execSync) to pass the shell path and arguments as an + * array, preventing command injection via metacharacters in the shell path. + * * This is necessary because VS Code extensions launched from the macOS * Dock/Finder inherit launchd's restricted PATH (/usr/bin:/bin) rather than * the user's full shell PATH (which includes Homebrew, nvm, cargo, etc.). @@ -31,10 +43,11 @@ export function captureShellEnvironment(): Record { } try { - // Run the user's shell as a login shell and capture environment. - // stderr is piped so shell startup messages (motd, etc.) don't mix - // with the env output on stdout. - const output = execSync(`${shell} -l -c 'env'`, { + // Run the user's shell as a login shell (-l) with the env command + // using execFileSync for defense against shell metacharacters in + // the shell path. stderr is piped so shell startup messages (motd, + // etc.) don't mix with the env output on stdout. + const output = execFileSync(shell, ["-l", "-c", "env"], { encoding: "utf8", timeout: 5000, env: process.env as Record, @@ -50,19 +63,23 @@ export function captureShellEnvironment(): Record { // Prefer the login-shell value; only keep process.env value // when the login shell produces an empty string for a key // that already exists in process.env. - if (value || !(key in process.env)) { + if (value) { + env[key] = value + } else if (key in process.env) { + env[key] = process.env[key]! + } else { env[key] = value } } } cachedEnv = env - console.log( + logger( `[ShellEnvironment] Captured ${Object.keys(env).length} env vars from ${shell} -l (PATH=${env.PATH?.slice(0, 80)}...)`, ) return env } catch (error) { - console.warn( + logger( `[ShellEnvironment] Failed to capture shell environment: ${error instanceof Error ? error.message : String(error)}`, ) cachedEnv = { ...process.env } as Record diff --git a/src/integrations/terminal/__tests__/ShellEnvironment.spec.ts b/src/integrations/terminal/__tests__/ShellEnvironment.spec.ts new file mode 100644 index 0000000000..24e06e853c --- /dev/null +++ b/src/integrations/terminal/__tests__/ShellEnvironment.spec.ts @@ -0,0 +1,201 @@ +// npx vitest run integrations/terminal/__tests__/ShellEnvironment.spec.ts + +import { execFileSync } from "child_process" + +vi.mock("../../../utils/shell", () => ({ + getShell: vi.fn().mockReturnValue("/bin/zsh"), +})) + +vi.mock("child_process", () => ({ + execFileSync: vi.fn(), +})) + +import { + captureShellEnvironment, + getShellEnvironment, + getCapturedShell, + invalidateShellEnvironment, + setShellLogger, + type ShellLogger, +} from "../ShellEnvironment" +import { getShell } from "../../../utils/shell" + +describe("ShellEnvironment", () => { + let mockLogger: ReturnType + + beforeEach(() => { + vi.clearAllMocks() + invalidateShellEnvironment() + mockLogger = vi.fn() + setShellLogger(mockLogger as unknown as ShellLogger) + // Reset process.platform to darwin for consistent testing + Object.defineProperty(process, "platform", { value: "darwin", configurable: true }) + }) + + describe("captureShellEnvironment", () => { + it("should capture login shell environment on macOS", () => { + const mockEnvOutput = "PATH=/opt/homebrew/bin:/usr/bin:/bin\nHOME=/Users/test\nUSER=test\n" + vi.mocked(execFileSync).mockReturnValue(mockEnvOutput) + + const result = captureShellEnvironment() + + expect(execFileSync).toHaveBeenCalledWith("/bin/zsh", ["-l", "-c", "env"], { + encoding: "utf8", + timeout: 5000, + env: process.env as Record, + stdio: ["ignore", "pipe", "pipe"], + }) + + expect(result.PATH).toBe("/opt/homebrew/bin:/usr/bin:/bin") + expect(result.HOME).toBe("/Users/test") + expect(result.USER).toBe("test") + expect(mockLogger).toHaveBeenCalledWith(expect.stringContaining("[ShellEnvironment] Captured 3 env vars")) + }) + + it("should skip login shell capture on Windows and use process.env", () => { + Object.defineProperty(process, "platform", { value: "win32", configurable: true }) + + const result = captureShellEnvironment() + + expect(execFileSync).not.toHaveBeenCalled() + expect(result).toEqual(process.env) + }) + + it("should fall back to process.env when execFileSync fails", () => { + vi.mocked(execFileSync).mockImplementation(() => { + throw new Error("Shell not found") + }) + + const result = captureShellEnvironment() + + expect(result).toEqual(process.env) + expect(mockLogger).toHaveBeenCalledWith( + expect.stringContaining("[ShellEnvironment] Failed to capture shell environment: Shell not found"), + ) + }) + + it("should prefer login-shell values over empty process.env values for existing keys", () => { + // Simulate login shell producing empty PATH + const mockEnvOutput = "PATH=\nHOME=/Users/test\nUSER=test_user\n" + vi.mocked(execFileSync).mockReturnValue(mockEnvOutput) + + // Pre-condition: PATH exists in process.env + const originalPath = process.env.PATH + process.env.PATH = "/usr/bin:/bin" + process.env.HOME = "" + + const result = captureShellEnvironment() + + // Empty login-shell value for existing key: keep process.env value + expect(result.PATH).toBe("/usr/bin:/bin") + // Non-empty login-shell value overwrites + expect(result.HOME).toBe("/Users/test") + + // Restore + process.env.PATH = originalPath + }) + + it("should cache the result and return cached on subsequent calls", () => { + const mockEnvOutput = "PATH=/custom/bin\n" + vi.mocked(execFileSync).mockReturnValue(mockEnvOutput) + + const result1 = captureShellEnvironment() + const result2 = captureShellEnvironment() + + expect(result1).toBe(result2) + expect(execFileSync).toHaveBeenCalledTimes(1) + }) + + it("should use getShell() to determine the shell binary", () => { + ;(getShell as any).mockReturnValue("/bin/bash") + vi.mocked(execFileSync).mockReturnValue("PATH=/usr/bin\n") + + captureShellEnvironment() + + expect(execFileSync).toHaveBeenCalledWith("/bin/bash", ["-l", "-c", "env"], expect.anything()) + }) + }) + + describe("getShellEnvironment", () => { + it("should capture on first call and return cached on second", () => { + const mockEnvOutput = "HOME=/test\n" + vi.mocked(execFileSync).mockReturnValue(mockEnvOutput) + + const env1 = getShellEnvironment() + const env2 = getShellEnvironment() + + expect(env1).toBeDefined() + expect(env2).toBe(env1) + expect(execFileSync).toHaveBeenCalledTimes(1) + }) + }) + + describe("getCapturedShell", () => { + it("should capture shell on first call", () => { + vi.mocked(execFileSync).mockReturnValue("PATH=/usr/bin\n") + + const shell = getCapturedShell() + + expect(shell).toBe("/bin/zsh") + }) + + it("should return cached shell on subsequent calls", () => { + vi.mocked(execFileSync).mockReturnValue("PATH=/usr/bin\n") + + getCapturedShell() + const shell = getCapturedShell() + + expect(shell).toBe("/bin/zsh") + }) + + it("should fall back to getShell() when no capture has run", () => { + ;(getShell as any).mockReturnValue("/bin/sh") + + // Invalidate to clear cache, then call without capture + invalidateShellEnvironment() + + const shell = getCapturedShell() + + expect(shell).toBe("/bin/sh") + }) + }) + + describe("invalidateShellEnvironment", () => { + it("should clear cached environment and shell", () => { + vi.mocked(execFileSync).mockReturnValue("PATH=/usr/bin\n") + captureShellEnvironment() + + invalidateShellEnvironment() + vi.mocked(execFileSync).mockReturnValue("PATH=/new/path\n") + + const result = captureShellEnvironment() + + expect(result.PATH).toBe("/new/path") + expect(execFileSync).toHaveBeenCalledTimes(2) + }) + }) + + describe("setShellLogger", () => { + it("should use the custom logger for diagnostic messages", () => { + const customLogger = vi.fn() + setShellLogger(customLogger as unknown as ShellLogger) + vi.mocked(execFileSync).mockReturnValue("PATH=/bin\n") + + captureShellEnvironment() + + expect(customLogger).toHaveBeenCalledWith(expect.stringContaining("[ShellEnvironment]")) + }) + + it("should use the custom logger for error messages", () => { + const customLogger = vi.fn() + setShellLogger(customLogger as unknown as ShellLogger) + vi.mocked(execFileSync).mockImplementation(() => { + throw new Error("Boom") + }) + + captureShellEnvironment() + + expect(customLogger).toHaveBeenCalledWith(expect.stringContaining("[ShellEnvironment] Failed")) + }) + }) +}) diff --git a/webview-ui/src/components/chat/CommandExecution.tsx b/webview-ui/src/components/chat/CommandExecution.tsx index 1040c9fc49..99f76afc94 100644 --- a/webview-ui/src/components/chat/CommandExecution.tsx +++ b/webview-ui/src/components/chat/CommandExecution.tsx @@ -203,18 +203,25 @@ export const CommandExecution = memo( } }, [status]) + // Ref for the latest command value so handleSubmit never uses a stale + // closure-captured value when text streams in during the approval UI. + const commandRef = useRef(command) + commandRef.current = command + // Handle Submit button click based on selected option const handleSubmit = useCallback(() => { if (!onPrimaryButtonClick || !onSecondaryButtonClick) return + const currentCommand = commandRef.current + switch (selectedOption) { case "yes": onPrimaryButtonClick() break case "yes_always": // Add the command pattern to allowedCommands before approving - if (command && command.trim()) { - const pattern = command.trim() + if (currentCommand && currentCommand.trim()) { + const pattern = currentCommand.trim() if (!allowedCommands.includes(pattern)) { const newAllowed = [...allowedCommands, pattern] setAllowedCommands(newAllowed) @@ -231,7 +238,6 @@ export const CommandExecution = memo( selectedOption, onPrimaryButtonClick, onSecondaryButtonClick, - command, allowedCommands, setAllowedCommands, feedbackText, @@ -244,11 +250,17 @@ export const CommandExecution = memo( } }, [onSecondaryButtonClick]) - // Keyboard navigation for approval options + // Keyboard navigation for approval options. + // Uses a mounted ref to prevent state updates on unmounted components + // (e.g. when the message stream clears during an open approval UI). useEffect(() => { if (!isApprovalMode) return + const mounted = { current: true } + const handleKeyDown = (e: KeyboardEvent) => { + if (!mounted.current) return + const currentIndex = APPROVAL_OPTIONS.findIndex((o) => o.key === selectedOption) switch (e.key) { @@ -276,7 +288,10 @@ export const CommandExecution = memo( } window.addEventListener("keydown", handleKeyDown) - return () => window.removeEventListener("keydown", handleKeyDown) + return () => { + mounted.current = false + window.removeEventListener("keydown", handleKeyDown) + } }, [isApprovalMode, selectedOption, handleSubmit, handleSkip]) // Render approval mode UI diff --git a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx index e969c13cbe..a928814e87 100644 --- a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx @@ -566,3 +566,212 @@ Output: }) }) }) + +describe("CommandExecution - Approval Mode", () => { + const mockOnPrimary = vi.fn() + const mockOnSecondary = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + mockOnPrimary.mockClear() + mockOnSecondary.mockClear() + }) + + const renderApprovalMode = () => { + render( + + + , + ) + } + + it("renders approval mode UI when buttons and enableButtons are provided", () => { + renderApprovalMode() + + expect(screen.getByText("install dependencies")).toBeInTheDocument() + expect(screen.getByText("npm install express")).toBeInTheDocument() + expect(screen.getByText("Yes")).toBeInTheDocument() + expect(screen.getByText("Yes, and don't ask again for this command")).toBeInTheDocument() + expect(screen.getByText("No, and tell Orbital the next step")).toBeInTheDocument() + expect(screen.getByText("Submit")).toBeInTheDocument() + }) + + it('calls onPrimaryButtonClick when "Yes" is selected and submitted', () => { + renderApprovalMode() + + fireEvent.click(screen.getByText("Submit")) + + expect(mockOnPrimary).toHaveBeenCalledTimes(1) + expect(mockOnSecondary).not.toHaveBeenCalled() + }) + + it('sends allowedCommands via vscode.postMessage when "Yes, always" is selected and submitted', () => { + renderApprovalMode() + + fireEvent.click(screen.getByText("Yes, and don't ask again for this command")) + fireEvent.click(screen.getByText("Submit")) + + expect(mockOnPrimary).toHaveBeenCalledTimes(1) + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "allowedCommands", + commands: expect.arrayContaining(["npm install express"]), + }), + ) + }) + + it('shows feedback input when "No, with feedback" is selected', () => { + renderApprovalMode() + + fireEvent.click(screen.getByText("No, and tell Orbital the next step")) + + const feedbackInput = screen.getByPlaceholderText("your message...") + expect(feedbackInput).toBeInTheDocument() + }) + + it('calls onSecondaryButtonClick with feedback text when "No" is submitted', () => { + renderApprovalMode() + + fireEvent.click(screen.getByText("No, and tell Orbital the next step")) + const feedbackInput = screen.getByPlaceholderText("your message...") + fireEvent.change(feedbackInput, { target: { value: "try pip instead" } }) + fireEvent.click(screen.getByText("Submit")) + + expect(mockOnSecondary).toHaveBeenCalledWith("try pip instead") + expect(mockOnPrimary).not.toHaveBeenCalled() + }) + + it("uses commandRef to prevent stale closure when message streams", () => { + const { rerender } = render( + + + , + ) + + rerender( + + + , + ) + + fireEvent.click(screen.getByText("Yes, and don't ask again for this command")) + fireEvent.click(screen.getByText("Submit")) + + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "allowedCommands", + commands: expect.arrayContaining(["npm run test:all"]), + }), + ) + }) +}) + +describe("CommandExecution - Keyboard Navigation", () => { + const mockOnPrimary = vi.fn() + const mockOnSecondary = vi.fn() + + beforeEach(() => { + vi.clearAllMocks() + mockOnPrimary.mockClear() + mockOnSecondary.mockClear() + }) + + const renderApprovalMode = () => { + render( + + + , + ) + } + + it("navigates options with ArrowUp and ArrowDown", () => { + renderApprovalMode() + + fireEvent.keyDown(window, { key: "ArrowDown" }) + fireEvent.keyDown(window, { key: "ArrowDown" }) + fireEvent.keyDown(window, { key: "ArrowUp" }) + + fireEvent.keyDown(window, { key: "Enter" }) + + expect(mockOnPrimary).toHaveBeenCalledTimes(1) + expect(vscode.postMessage).toHaveBeenCalledWith( + expect.objectContaining({ + type: "allowedCommands", + commands: expect.arrayContaining(["npm test"]), + }), + ) + }) + + it("does not navigate past first option with ArrowUp", () => { + renderApprovalMode() + + fireEvent.keyDown(window, { key: "ArrowUp" }) + fireEvent.keyDown(window, { key: "Enter" }) + + expect(mockOnPrimary).toHaveBeenCalledTimes(1) + }) + + it("does not navigate past last option with ArrowDown", () => { + renderApprovalMode() + + fireEvent.keyDown(window, { key: "ArrowDown" }) + fireEvent.keyDown(window, { key: "ArrowDown" }) + fireEvent.keyDown(window, { key: "ArrowDown" }) + fireEvent.keyDown(window, { key: "Enter" }) + + expect(mockOnSecondary).toHaveBeenCalledWith(undefined) + }) + + it("calls handleSkip on Escape", () => { + renderApprovalMode() + + fireEvent.keyDown(window, { key: "Escape" }) + + expect(mockOnSecondary).toHaveBeenCalledTimes(1) + }) + + it("removes keyboard listener when approval mode is exited", () => { + const { unmount } = render( + + + , + ) + + unmount() + + fireEvent.keyDown(window, { key: "Enter" }) + + expect(mockOnPrimary).not.toHaveBeenCalled() + expect(mockOnSecondary).not.toHaveBeenCalled() + }) +})