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

Author: hannesrudolph

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<void> {
+	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
 			}
 

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"
 	}
 }

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 |).