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..72ccc34c9e 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 && !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/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) diff --git a/src/core/tools/executeCommandTool.ts b/src/core/tools/executeCommandTool.ts index bbc67cb324..05f8989037 100644 --- a/src/core/tools/executeCommandTool.ts +++ b/src/core/tools/executeCommandTool.ts @@ -29,7 +29,11 @@ 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 rawMessage: unknown = block.params.message + const customMessage: string | undefined = + typeof rawMessage === "string" && rawMessage !== "null" ? rawMessage : undefined try { if (block.partial) { @@ -54,7 +58,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/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 6d451b815a..6eb00324cc 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,22 @@ ${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 defaultModel = await getKilocodeDefaultModel() + const updatedConfig = { ...apiConfiguration, kilocodeModel: defaultModel } + await this.contextProxy.setProviderSettings(updatedConfig) + mergedApiConfiguration = { ...mergedApiConfiguration, kilocodeModel: defaultModel } + this.log( + `[ModelValidation] Reset stale kilocodeModel "${staleModel}" to default "${defaultModel}" — model no longer available`, + ) + } + } + // forked_change start wrapper information const kiloCodeWrapperProperties = getKiloCodeWrapperProperties() const taskHistory = this.getTaskHistory() 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 baf6d9d27b..9ac04e64b7 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, setShellLogger } 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,13 @@ 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). + setShellLogger((msg: string) => outputChannel.appendLine(msg)) + 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..19e5d4d672 --- /dev/null +++ b/src/integrations/terminal/ShellEnvironment.ts @@ -0,0 +1,110 @@ +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.). + * + * 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 (-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, + 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) { + env[key] = value + } else if (key in process.env) { + env[key] = process.env[key]! + } else { + env[key] = value + } + } + } + + cachedEnv = env + logger( + `[ShellEnvironment] Captured ${Object.keys(env).length} env vars from ${shell} -l (PATH=${env.PATH?.slice(0, 80)}...)`, + ) + return env + } catch (error) { + logger( + `[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 () => { 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/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", diff --git a/webview-ui/src/components/chat/CommandExecution.tsx b/webview-ui/src/components/chat/CommandExecution.tsx index ba67e7db09..99f76afc94 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,193 @@ 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 (currentCommand && currentCommand.trim()) { + const pattern = currentCommand.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, + allowedCommands, + setAllowedCommands, + feedbackText, + ]) + + // Handle Skip button click + const handleSkip = useCallback(() => { + if (onSecondaryButtonClick) { + onSecondaryButtonClick() + } + }, [onSecondaryButtonClick]) + + // 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) { + 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 () => { + mounted.current = false + 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 +476,7 @@ export const CommandExecution = memo( /> )}
- {onPrimaryButtonClick && onSecondaryButtonClick && enableButtons && ( + {onPrimaryButtonClick && onSecondaryButtonClick && enableButtons && !isApprovalMode && (
{onRunEverythingClick && ( @@ -328,17 +529,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), } } 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() + }) +})