From 8fd1210776c63933448cb2294c8f26c8a8906316 Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Tue, 1 Jul 2025 08:33:09 -0600 Subject: [PATCH] Address PR review comments - Use extractCommandPattern function to extract base command patterns - Add proper error handling for providerRef.deref() - Use internationalization for user-facing messages - Extract addCommandToWhitelist helper function to reduce duplication - Add comments explaining why we can't use the shared askApproval function --- src/core/tools/executeCommandTool.ts | 137 ++++++++++++++++++--- src/i18n/locales/en/tools.json | 3 + webview-ui/src/utils/command-validation.ts | 45 +++++++ 3 files changed, 165 insertions(+), 20 deletions(-) diff --git a/src/core/tools/executeCommandTool.ts b/src/core/tools/executeCommandTool.ts index e9e9a42a1f..830dbb8018 100644 --- a/src/core/tools/executeCommandTool.ts +++ b/src/core/tools/executeCommandTool.ts @@ -14,9 +14,116 @@ import { unescapeHtmlEntities } from "../../utils/text-normalization" import { ExitCodeDetails, RooTerminalCallbacks, RooTerminalProcess } from "../../integrations/terminal/types" import { TerminalRegistry } from "../../integrations/terminal/TerminalRegistry" import { Terminal } from "../../integrations/terminal/Terminal" +import { t } from "../../i18n" class ShellIntegrationError extends Error {} +/** + * Extract the base command pattern from a full command string. + * For example: "gh pr checkout 1234" -> "gh pr checkout" + * + * @param command The full command string + * @returns The base command pattern suitable for whitelisting + */ +function extractCommandPattern(command: string): string { + if (!command?.trim()) return "" + + // Split by whitespace, handling quoted strings + const parts: string[] = [] + let current = "" + let inQuotes = false + let quoteChar = "" + + for (let i = 0; i < command.length; i++) { + const char = command[i] + const prevChar = i > 0 ? command[i - 1] : "" + + if ((char === '"' || char === "'") && prevChar !== "\\") { + if (!inQuotes) { + inQuotes = true + quoteChar = char + } else if (char === quoteChar) { + inQuotes = false + quoteChar = "" + } + current += char + } else if (char === " " && !inQuotes) { + if (current) { + parts.push(current) + current = "" + } + } else { + current += char + } + } + + if (current) { + parts.push(current) + } + + // Extract pattern parts, stopping at arguments + const patternParts: string[] = [] + + for (const part of parts) { + // Remove quotes for analysis + const unquoted = part.replace(/^["']|["']$/g, "") + + // Stop at common argument patterns: + // - Pure numbers (like PR numbers, PIDs, etc.) + // - Flags starting with - or -- + // - File paths or URLs + // - Variable assignments (KEY=VALUE) + // - Operators (&&, ||, |, ;, >, <, etc.) + if ( + /^\d+$/.test(unquoted) || + unquoted.startsWith("-") || + unquoted.includes("/") || + unquoted.includes("\\") || + unquoted.includes("=") || + unquoted.startsWith("http") || + unquoted.includes(".") || + ["&&", "||", "|", ";", ">", "<", ">>", "<<", "&"].includes(unquoted) + ) { + // Stop collecting pattern parts + break + } + patternParts.push(part) + } + + // Return the base command pattern + return patternParts.join(" ") +} + +/** + * Adds a command pattern to the whitelist + */ +async function addCommandToWhitelist(cline: Task, command: string): Promise { + const clineProvider = cline.providerRef.deref() + if (!clineProvider) { + console.error("Provider reference is undefined, cannot add command to whitelist") + return + } + + const state = await clineProvider.getState() + const currentCommands = state.allowedCommands ?? [] + + // Extract the base command pattern for whitelisting + const commandPattern = extractCommandPattern(command) + + // Add command pattern to whitelist if not already present + if (commandPattern && !currentCommands.includes(commandPattern)) { + const newCommands = [...currentCommands, commandPattern] + await clineProvider.setValue("allowedCommands", newCommands) + + // Notify webview of the updated commands + await clineProvider.postMessageToWebview({ + type: "invoke", + invoke: "setChatBoxMessage", + text: t("tools:executeCommand.patternAddedToWhitelist", { pattern: commandPattern }), + }) + } +} + export async function executeCommandTool( cline: Task, block: ToolUse, @@ -53,40 +160,30 @@ export async function executeCommandTool( command = unescapeHtmlEntities(command) // Unescape HTML entities. // We need to capture the actual response to check if "Add & Run" was clicked + // Note: We cannot use the provided askApproval function here because we need to + // differentiate between "yesButtonClicked" and "addAndRunButtonClicked" responses const { response, text, images } = await cline.ask("command", command) if (response === "yesButtonClicked" || response === "addAndRunButtonClicked") { - // Handle yesButtonClicked or addAndRunButtonClicked with text. + // Handle yesButtonClicked or addAndRunButtonClicked with text (following askApproval pattern) if (text) { await cline.say("user_feedback", text, images) + pushToolResult(formatResponse.toolResult(formatResponse.toolApprovedWithFeedback(text), images)) } // Check if user selected "Add & Run" to add command to whitelist if (response === "addAndRunButtonClicked") { - const clineProvider = await cline.providerRef.deref() - if (clineProvider) { - const state = await clineProvider.getState() - const currentCommands = state.allowedCommands ?? [] - - // Add command to whitelist if not already present - if (!currentCommands.includes(command)) { - const newCommands = [...currentCommands, command] - await clineProvider.setValue("allowedCommands", newCommands) - - // Notify webview of the updated commands - await clineProvider.postMessageToWebview({ - type: "invoke", - invoke: "setChatBoxMessage", - text: `Command "${command}" added to whitelist.`, - }) - } - } + await addCommandToWhitelist(cline, command) } } else { - // Handle both messageResponse and noButtonClicked with text. + // Handle both messageResponse and noButtonClicked with text (following askApproval pattern) if (text) { await cline.say("user_feedback", text, images) + pushToolResult(formatResponse.toolResult(formatResponse.toolDeniedWithFeedback(text), images)) + } else { + pushToolResult(formatResponse.toolDenied()) } + cline.didRejectTool = true return } diff --git a/src/i18n/locales/en/tools.json b/src/i18n/locales/en/tools.json index 0265a84398..9f3992d4de 100644 --- a/src/i18n/locales/en/tools.json +++ b/src/i18n/locales/en/tools.json @@ -12,5 +12,8 @@ "errors": { "policy_restriction": "Failed to create new task due to policy restrictions." } + }, + "executeCommand": { + "patternAddedToWhitelist": "Command pattern '{{pattern}}' added to whitelist" } } diff --git a/webview-ui/src/utils/command-validation.ts b/webview-ui/src/utils/command-validation.ts index 7ad21b1675..d4943ec5af 100644 --- a/webview-ui/src/utils/command-validation.ts +++ b/webview-ui/src/utils/command-validation.ts @@ -2,6 +2,51 @@ import { parse } from "shell-quote" type ShellToken = string | { op: string } | { command: string } +/** + * Extract the base command pattern from a full command string. + * For example: "gh pr checkout 1234" -> "gh pr checkout" + * + * @param command The full command string + * @returns The base command pattern suitable for whitelisting + */ +export function extractCommandPattern(command: string): string { + if (!command?.trim()) return "" + + // Parse the command to get tokens + const tokens = parse(command.trim()) as ShellToken[] + const patternParts: string[] = [] + + for (const token of tokens) { + if (typeof token === "string") { + // Check if this token looks like an argument (number, flag, etc.) + // Common patterns to stop at: + // - Pure numbers (like PR numbers, PIDs, etc.) + // - Flags starting with - or -- + // - File paths or URLs + // - Variable assignments (KEY=VALUE) + if ( + /^\d+$/.test(token) || + token.startsWith("-") || + token.includes("/") || + token.includes("\\") || + token.includes("=") || + token.startsWith("http") || + token.includes(".") + ) { + // Stop collecting pattern parts + break + } + patternParts.push(token) + } else if (typeof token === "object" && "op" in token) { + // Stop at operators + break + } + } + + // Return the base command pattern + return patternParts.join(" ") +} + /** * Split a command string into individual sub-commands by * chaining operators (&&, ||, ;, or |).