From 238bae6aef50255e58b50cc8a74ea1f8dadc10d9 Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Tue, 15 Jul 2025 12:15:46 -0600 Subject: [PATCH] fix: resolve command whitelisting issues - Fixed programmatic suggestion generation when LLM suggestions are disabled - CommandExecution component now properly generates suggestions from allowed patterns - Added proper handling for when llmGeneratedSuggestions setting is false - Fixed settings persistence for command allow/deny lists - AutoApproveSettings now correctly saves patterns to globalState - Fixed state management to properly update both local and global state - Improved LLM prompt to generate complete suggestions for chained commands - Updated execute-command prompt to handle && and || operators - Ensures suggestions include full command chains, not just the first part - Added tests to verify proper handling of complex command patterns --- .../tools/__tests__/execute-command.spec.ts | 3 ++ src/core/prompts/tools/execute-command.ts | 18 ++++++- .../src/components/chat/CommandExecution.tsx | 9 ++-- .../chat/__tests__/CommandExecution.spec.tsx | 53 +++++++++++++++---- .../settings/AutoApproveSettings.tsx | 9 ++-- .../settings/__tests__/SettingsView.spec.tsx | 8 +-- 6 files changed, 75 insertions(+), 25 deletions(-) diff --git a/src/core/prompts/tools/__tests__/execute-command.spec.ts b/src/core/prompts/tools/__tests__/execute-command.spec.ts index 36ad2e8d8a..386b8c0fb2 100644 --- a/src/core/prompts/tools/__tests__/execute-command.spec.ts +++ b/src/core/prompts/tools/__tests__/execute-command.spec.ts @@ -22,6 +22,9 @@ describe("getExecuteCommandDescription", () => { expect(description).toContain("") expect(description).toContain("- suggestions: (optional) Command patterns for the user to allow/deny") expect(description).toContain("Suggestion Guidelines") + // Check for chained command guidance + expect(description).toContain("For chained commands") + expect(description).toContain("cd backend && npm install") }) it("should include suggestions section when disableLlmCommandSuggestions is not set", () => { diff --git a/src/core/prompts/tools/execute-command.ts b/src/core/prompts/tools/execute-command.ts index c34b28f7d3..d1c83ea173 100644 --- a/src/core/prompts/tools/execute-command.ts +++ b/src/core/prompts/tools/execute-command.ts @@ -37,11 +37,14 @@ Example: Requesting to execute ls in a specific directory return ( baseDescription + ` -- suggestions: (optional) Command patterns for the user to allow/deny for future auto-approval. Include 1-2 relevant patterns when executing common development commands. Use tags. +- suggestions: (optional) Command patterns for the user to allow/deny for future auto-approval. Use tags. **Suggestion Guidelines:** - Suggestions use prefix matching (case-insensitive) -- Include the base command (e.g., "npm", "git") and optionally a more specific pattern +- For simple commands: Include the base command (e.g., "npm", "git") and optionally a more specific pattern +- For chained commands (using &&, ||, ;, |): Include patterns for EACH individual command in the chain + - Example: For "cd backend && npm install", suggest: "cd backend && npm install", "cd", "npm install", "npm" +- Include 2-4 relevant patterns total - Only suggest "*" (allow all) if explicitly requested by the user Usage: @@ -63,6 +66,17 @@ Example: Requesting to execute npm run dev +Example: Requesting to execute a chained command + +cd backend && npm install + +cd backend && npm install +cd +npm install +npm + + + Example: Requesting to execute ls in a specific directory ls -la diff --git a/webview-ui/src/components/chat/CommandExecution.tsx b/webview-ui/src/components/chat/CommandExecution.tsx index 7abbe1203d..8bc5e4e001 100644 --- a/webview-ui/src/components/chat/CommandExecution.tsx +++ b/webview-ui/src/components/chat/CommandExecution.tsx @@ -45,7 +45,8 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec const [isOutputExpanded, setIsOutputExpanded] = useState(false) // Determine if we should show suggestions section - const showSuggestions = suggestions && suggestions.length > 0 + // Always show suggestions if we have a command, either from LLM or programmatic generation + const showSuggestions = (suggestions && suggestions.length > 0) || !!command?.trim() // Use suggestions if available, otherwise extract command patterns const commandPatterns = useMemo(() => { @@ -57,8 +58,8 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec })) } - // Only extract patterns if we're showing suggestions (for backward compatibility) - if (!showSuggestions || !command?.trim()) return [] + // If no LLM suggestions but we have a command, extract patterns programmatically + if (!command?.trim()) return [] // Check if this is a chained command const operators = ["&&", "||", ";", "|"] @@ -151,7 +152,7 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec ) return uniquePatterns - }, [command, suggestions, showSuggestions]) + }, [command, suggestions]) // 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 diff --git a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx index 522bebd3d6..3a1628492b 100644 --- a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx @@ -82,7 +82,7 @@ describe("CommandExecution", () => { } as any) }) - it("should render command without suggestions", () => { + it("should render command with programmatic suggestions when no LLM suggestions provided", () => { renderWithProviders( { />, ) - expect(screen.getByText("npm install")).toBeInTheDocument() - expect(screen.queryByText("Manage Command Permissions")).not.toBeInTheDocument() + // Check command is rendered in code block + const codeBlocks = screen.getAllByText("npm install") + expect(codeBlocks.length).toBeGreaterThan(0) + + // Should show manage permissions section even without LLM suggestions + expect(screen.getByText("Manage Command Permissions")).toBeInTheDocument() + + // Expand the section to verify programmatic patterns + const sectionHeader = screen.getByText("Manage Command Permissions") + fireEvent.click(sectionHeader) + + // Should show programmatically extracted pattern + const patterns = screen.getAllByText("npm install") + expect(patterns.length).toBeGreaterThan(1) // Command + pattern }) it("should render command with suggestions section collapsed by default", () => { @@ -220,7 +232,7 @@ describe("CommandExecution", () => { }) }) - it("should handle empty suggestions tag", () => { + it("should handle empty suggestions tag and show programmatic patterns", () => { const commandWithEmptySuggestions = "ls -la[]" renderWithProviders( @@ -232,8 +244,19 @@ describe("CommandExecution", () => { />, ) - expect(screen.getByText("ls -la")).toBeInTheDocument() - expect(screen.queryByText("Manage Command Permissions")).not.toBeInTheDocument() + // Check command is rendered + const codeBlocks = screen.getAllByText("ls -la") + expect(codeBlocks.length).toBeGreaterThan(0) + + // Should still show manage permissions with programmatic patterns + expect(screen.getByText("Manage Command Permissions")).toBeInTheDocument() + + // Expand the section to verify programmatic patterns + const sectionHeader = screen.getByText("Manage Command Permissions") + fireEvent.click(sectionHeader) + + // Should show the base command pattern + expect(screen.getByText("ls")).toBeInTheDocument() }) it("should handle suggestions with special characters", () => { @@ -260,7 +283,7 @@ describe("CommandExecution", () => { expect(screen.getByText("echo `date`")).toBeInTheDocument() }) - it("should handle malformed suggestions tag", () => { + it("should handle malformed suggestions tag and show programmatic patterns", () => { const commandWithMalformedSuggestions = "pwdnot-valid-json" renderWithProviders( @@ -273,9 +296,19 @@ describe("CommandExecution", () => { ) // Should still render the command - expect(screen.getByText("pwd")).toBeInTheDocument() - // Suggestions should not be shown when JSON is invalid - expect(screen.queryByText("Manage Command Permissions")).not.toBeInTheDocument() + const codeBlocks = screen.getAllByText("pwd") + expect(codeBlocks.length).toBeGreaterThan(0) + + // Should show manage permissions with programmatic patterns + expect(screen.getByText("Manage Command Permissions")).toBeInTheDocument() + + // Expand the section to verify programmatic patterns + const sectionHeader = screen.getByText("Manage Command Permissions") + fireEvent.click(sectionHeader) + + // Should show the base command pattern (there will be multiple pwd elements) + const patterns = screen.getAllByText("pwd") + expect(patterns.length).toBeGreaterThan(1) // Command + pattern }) it("should parse suggestions from JSON array and show them when expanded", () => { diff --git a/webview-ui/src/components/settings/AutoApproveSettings.tsx b/webview-ui/src/components/settings/AutoApproveSettings.tsx index 7006a59180..8c3cf20b5e 100644 --- a/webview-ui/src/components/settings/AutoApproveSettings.tsx +++ b/webview-ui/src/components/settings/AutoApproveSettings.tsx @@ -3,7 +3,6 @@ import { X } from "lucide-react" import { useAppTranslation } from "@/i18n/TranslationContext" import { VSCodeCheckbox } from "@vscode/webview-ui-toolkit/react" -import { vscode } from "@/utils/vscode" import { Button, Input, Slider } from "@/components/ui" import { SetCachedStateField } from "./types" @@ -88,7 +87,7 @@ export const AutoApproveSettings = ({ const newCommands = [...currentCommands, commandInput] setCachedStateField("allowedCommands", newCommands) setCommandInput("") - vscode.postMessage({ type: "allowedCommands", commands: newCommands }) + // Don't send message here - wait for Save button } } @@ -99,7 +98,7 @@ export const AutoApproveSettings = ({ const newCommands = [...currentCommands, deniedCommandInput] setCachedStateField("deniedCommands", newCommands) setDeniedCommandInput("") - vscode.postMessage({ type: "deniedCommands", commands: newCommands }) + // Don't send message here - wait for Save button } } @@ -318,7 +317,7 @@ export const AutoApproveSettings = ({ onClick={() => { const newCommands = (allowedCommands ?? []).filter((_, i) => i !== index) setCachedStateField("allowedCommands", newCommands) - vscode.postMessage({ type: "allowedCommands", commands: newCommands }) + // Don't send message here - wait for Save button }}>
{cmd}
@@ -369,7 +368,7 @@ export const AutoApproveSettings = ({ onClick={() => { const newCommands = (deniedCommands ?? []).filter((_, i) => i !== index) setCachedStateField("deniedCommands", newCommands) - vscode.postMessage({ type: "deniedCommands", commands: newCommands }) + // Don't send message here - wait for Save button }}>
{cmd}
diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx index 404b3a6883..f7976bdfbe 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.spec.tsx @@ -458,8 +458,8 @@ describe("SettingsView - Allowed Commands", () => { // Verify command was added expect(screen.getByText("npm test")).toBeInTheDocument() - // Verify VSCode message was sent - expect(vscode.postMessage).toHaveBeenCalledWith({ + // Verify VSCode message was NOT sent yet (only on Save) + expect(vscode.postMessage).not.toHaveBeenCalledWith({ type: "allowedCommands", commands: ["npm test"], }) @@ -489,8 +489,8 @@ describe("SettingsView - Allowed Commands", () => { // Verify command was removed expect(screen.queryByText("npm test")).not.toBeInTheDocument() - // Verify VSCode message was sent - expect(vscode.postMessage).toHaveBeenLastCalledWith({ + // Verify VSCode message was NOT sent yet (only on Save) + expect(vscode.postMessage).not.toHaveBeenCalledWith({ type: "allowedCommands", commands: [], })