From 82350297a4012759b0ffffc93425e17b4081777d Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Fri, 25 Jul 2025 08:23:43 -0500 Subject: [PATCH] feat: simplify command permissions UI to use full commands instead of patterns - Replace pattern extraction with single text input for full command - Allow users to edit command before approving/denying - Show active state (check/x) based on current allowed/denied lists - Remove command-parser.ts and related pattern extraction logic - Update tests to match new simplified behavior This change addresses user feedback requesting a simpler interface where users can edit the full command before setting permissions. --- .../src/components/chat/CommandExecution.tsx | 43 ++-- .../chat/CommandPatternSelector.tsx | 115 ++++----- .../chat/__tests__/CommandExecution.spec.tsx | 232 ++++++++---------- .../__tests__/CommandPatternSelector.spec.tsx | 180 ++++++++++---- .../utils/__tests__/command-parser.spec.ts | 137 ----------- webview-ui/src/utils/command-parser.ts | 68 ----- 6 files changed, 310 insertions(+), 465 deletions(-) delete mode 100644 webview-ui/src/utils/__tests__/command-parser.spec.ts delete mode 100644 webview-ui/src/utils/command-parser.ts diff --git a/webview-ui/src/components/chat/CommandExecution.tsx b/webview-ui/src/components/chat/CommandExecution.tsx index 79d2b54edb..50c52abd27 100644 --- a/webview-ui/src/components/chat/CommandExecution.tsx +++ b/webview-ui/src/components/chat/CommandExecution.tsx @@ -13,12 +13,6 @@ import { cn } from "@src/lib/utils" import { Button } from "@src/components/ui" import CodeBlock from "../common/CodeBlock" import { CommandPatternSelector } from "./CommandPatternSelector" -import { extractPatternsFromCommand } from "../../utils/command-parser" - -interface CommandPattern { - pattern: string - description?: string -} interface CommandExecutionProps { executionId: string @@ -43,7 +37,7 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec if (outputIndex !== -1) { // Text is split into command and output - const cmd = (text ?? '').slice(0, outputIndex).trim() + const cmd = (text ?? "").slice(0, outputIndex).trim() // Skip the newline and "Output:" text const afterSeparator = outputIndex + 1 + outputSeparator.length let startOfOutput = afterSeparator @@ -72,20 +66,11 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec // streaming output (this is the case for running commands). const output = streamingOutput || parsedOutput - // Extract command patterns from the actual command that was executed - const commandPatterns = useMemo(() => { - // Extract patterns from the actual command that was executed - const extractedPatterns = extractPatternsFromCommand(command) - return extractedPatterns.map((pattern) => ({ - pattern, - })) - }, [command]) - - // Handle pattern changes - const handleAllowPatternChange = (pattern: string) => { - const isAllowed = allowedCommands.includes(pattern) - const newAllowed = isAllowed ? allowedCommands.filter((p) => p !== pattern) : [...allowedCommands, pattern] - const newDenied = deniedCommands.filter((p) => p !== pattern) + // Handle command changes + const handleAllowCommandChange = (cmd: string) => { + const isAllowed = allowedCommands.includes(cmd) + const newAllowed = isAllowed ? allowedCommands.filter((c) => c !== cmd) : [...allowedCommands, cmd] + const newDenied = deniedCommands.filter((c) => c !== cmd) setAllowedCommands(newAllowed) setDeniedCommands(newDenied) @@ -93,10 +78,10 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec vscode.postMessage({ type: "deniedCommands", commands: newDenied }) } - const handleDenyPatternChange = (pattern: string) => { - const isDenied = deniedCommands.includes(pattern) - const newDenied = isDenied ? deniedCommands.filter((p) => p !== pattern) : [...deniedCommands, pattern] - const newAllowed = allowedCommands.filter((p) => p !== pattern) + const handleDenyCommandChange = (cmd: string) => { + const isDenied = deniedCommands.includes(cmd) + const newDenied = isDenied ? deniedCommands.filter((c) => c !== cmd) : [...deniedCommands, cmd] + const newAllowed = allowedCommands.filter((c) => c !== cmd) setAllowedCommands(newAllowed) setDeniedCommands(newDenied) @@ -193,13 +178,13 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec - {commandPatterns.length > 0 && ( + {command && ( )} diff --git a/webview-ui/src/components/chat/CommandPatternSelector.tsx b/webview-ui/src/components/chat/CommandPatternSelector.tsx index 8d3acf71c4..cadc90c443 100644 --- a/webview-ui/src/components/chat/CommandPatternSelector.tsx +++ b/webview-ui/src/components/chat/CommandPatternSelector.tsx @@ -5,35 +5,33 @@ import { useTranslation, Trans } from "react-i18next" import { VSCodeLink } from "@vscode/webview-ui-toolkit/react" import { StandardTooltip } from "../ui/standard-tooltip" -interface CommandPattern { - pattern: string - description?: string -} - interface CommandPatternSelectorProps { - patterns: CommandPattern[] + command: string allowedCommands: string[] deniedCommands: string[] - onAllowPatternChange: (pattern: string) => void - onDenyPatternChange: (pattern: string) => void + onAllowCommandChange: (command: string) => void + onDenyCommandChange: (command: string) => void } export const CommandPatternSelector: React.FC = ({ - patterns, + command, allowedCommands, deniedCommands, - onAllowPatternChange, - onDenyPatternChange, + onAllowCommandChange, + onDenyCommandChange, }) => { const { t } = useTranslation() const [isExpanded, setIsExpanded] = useState(false) + const [editedCommand, setEditedCommand] = useState(command) - const getPatternStatus = (pattern: string): "allowed" | "denied" | "none" => { - if (allowedCommands.includes(pattern)) return "allowed" - if (deniedCommands.includes(pattern)) return "denied" + const getCommandStatus = (cmd: string): "allowed" | "denied" | "none" => { + if (allowedCommands.includes(cmd)) return "allowed" + if (deniedCommands.includes(cmd)) return "denied" return "none" } + const currentStatus = getCommandStatus(editedCommand) + return (
{isExpanded && ( -
- {patterns.map((item) => { - const status = getPatternStatus(item.pattern) - return ( -
-
- {item.pattern} - {item.description && ( - - - {item.description} - - )} -
-
- - -
-
- ) - })} +
+
+
+ setEditedCommand(e.target.value)} + className="font-mono text-xs bg-vscode-input-background text-vscode-input-foreground border border-vscode-input-border rounded px-2 py-1 w-full focus:outline-none focus:border-vscode-focusBorder" + placeholder={command} + /> +
+
+ + +
+
)}
diff --git a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx index ed9e0034ea..23f53ccd0d 100644 --- a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx @@ -22,15 +22,11 @@ vi.mock("../../common/CodeBlock", () => ({ })) vi.mock("../CommandPatternSelector", () => ({ - CommandPatternSelector: ({ patterns, onAllowPatternChange, onDenyPatternChange }: any) => ( + CommandPatternSelector: ({ command, onAllowCommandChange, onDenyCommandChange }: any) => (
- {patterns.map((p: any, i: number) => ( -
- {p.pattern} - - -
- ))} + {command} + +
), })) @@ -88,7 +84,7 @@ describe("CommandExecution", () => { expect(screen.getByTestId("custom-title")).toBeInTheDocument() }) - it("should show command pattern selector for simple commands", () => { + it("should show command pattern selector for commands", () => { render( @@ -96,72 +92,85 @@ describe("CommandExecution", () => { ) expect(screen.getByTestId("command-pattern-selector")).toBeInTheDocument() - expect(screen.getByText("npm")).toBeInTheDocument() - expect(screen.getByText("npm install")).toBeInTheDocument() + expect(screen.getByText("npm install express")).toBeInTheDocument() }) - it("should handle allow pattern change", () => { + it("should handle allow command change", () => { render( , ) - const allowButton = screen.getByText("Allow git") + const allowButton = screen.getByText("Allow git push") fireEvent.click(allowButton) - expect(mockExtensionState.setAllowedCommands).toHaveBeenCalledWith(["npm", "git"]) + expect(mockExtensionState.setAllowedCommands).toHaveBeenCalledWith(["npm", "git push"]) expect(mockExtensionState.setDeniedCommands).toHaveBeenCalledWith(["rm"]) - expect(vscode.postMessage).toHaveBeenCalledWith({ type: "allowedCommands", commands: ["npm", "git"] }) + expect(vscode.postMessage).toHaveBeenCalledWith({ type: "allowedCommands", commands: ["npm", "git push"] }) expect(vscode.postMessage).toHaveBeenCalledWith({ type: "deniedCommands", commands: ["rm"] }) }) - it("should handle deny pattern change", () => { + it("should handle deny command change", () => { render( , ) - const denyButton = screen.getByText("Deny docker") + const denyButton = screen.getByText("Deny docker run") fireEvent.click(denyButton) expect(mockExtensionState.setAllowedCommands).toHaveBeenCalledWith(["npm"]) - expect(mockExtensionState.setDeniedCommands).toHaveBeenCalledWith(["rm", "docker"]) + expect(mockExtensionState.setDeniedCommands).toHaveBeenCalledWith(["rm", "docker run"]) expect(vscode.postMessage).toHaveBeenCalledWith({ type: "allowedCommands", commands: ["npm"] }) - expect(vscode.postMessage).toHaveBeenCalledWith({ type: "deniedCommands", commands: ["rm", "docker"] }) + expect(vscode.postMessage).toHaveBeenCalledWith({ type: "deniedCommands", commands: ["rm", "docker run"] }) }) - it("should toggle allowed pattern", () => { + it("should toggle allowed command", () => { + // Update the mock state to have "npm test" in allowedCommands + const stateWithNpmTest = { + ...mockExtensionState, + allowedCommands: ["npm test"], + deniedCommands: ["rm"], + } + render( - + - , + , ) - const allowButton = screen.getByText("Allow npm") + const allowButton = screen.getByText("Allow npm test") fireEvent.click(allowButton) - // npm is already in allowedCommands, so it should be removed - expect(mockExtensionState.setAllowedCommands).toHaveBeenCalledWith([]) - expect(mockExtensionState.setDeniedCommands).toHaveBeenCalledWith(["rm"]) + // "npm test" is already in allowedCommands, so it should be removed + expect(stateWithNpmTest.setAllowedCommands).toHaveBeenCalledWith([]) + expect(stateWithNpmTest.setDeniedCommands).toHaveBeenCalledWith(["rm"]) expect(vscode.postMessage).toHaveBeenCalledWith({ type: "allowedCommands", commands: [] }) expect(vscode.postMessage).toHaveBeenCalledWith({ type: "deniedCommands", commands: ["rm"] }) }) - it("should toggle denied pattern", () => { + it("should toggle denied command", () => { + // Update the mock state to have "rm -rf" in deniedCommands + const stateWithRmRf = { + ...mockExtensionState, + allowedCommands: ["npm"], + deniedCommands: ["rm -rf"], + } + render( - + - , + , ) - const denyButton = screen.getByText("Deny rm") + const denyButton = screen.getByText("Deny rm -rf") fireEvent.click(denyButton) - // rm is already in deniedCommands, so it should be removed - expect(mockExtensionState.setAllowedCommands).toHaveBeenCalledWith(["npm"]) - expect(mockExtensionState.setDeniedCommands).toHaveBeenCalledWith([]) + // "rm -rf" is already in deniedCommands, so it should be removed + expect(stateWithRmRf.setAllowedCommands).toHaveBeenCalledWith(["npm"]) + expect(stateWithRmRf.setDeniedCommands).toHaveBeenCalledWith([]) expect(vscode.postMessage).toHaveBeenCalledWith({ type: "allowedCommands", commands: ["npm"] }) expect(vscode.postMessage).toHaveBeenCalledWith({ type: "deniedCommands", commands: [] }) }) @@ -181,7 +190,7 @@ Installing...` expect(codeBlocks[0]).toHaveTextContent("npm install") }) - it("should parse command with AI suggestions", () => { + it("should parse command with output", () => { const commandText = `npm install Output: Suggested patterns: npm, npm install, npm run` @@ -198,11 +207,8 @@ Suggested patterns: npm, npm install, npm run` expect(codeBlocks[1]).toHaveTextContent("Suggested patterns: npm, npm install, npm run") expect(screen.getByTestId("command-pattern-selector")).toBeInTheDocument() - // Check that only patterns from the actual command are extracted, not from AI suggestions - expect(screen.getByText("npm")).toBeInTheDocument() - expect(screen.getAllByText("npm install").length).toBeGreaterThan(0) - // "npm run" should NOT be in the patterns since it's only in the AI suggestions, not the actual command - expect(screen.queryByText("npm run")).not.toBeInTheDocument() + // Should show the full command + expect(screen.getByText("npm install")).toBeInTheDocument() }) it("should handle commands with pipes", () => { @@ -213,8 +219,7 @@ Suggested patterns: npm, npm install, npm run` ) expect(screen.getByTestId("command-pattern-selector")).toBeInTheDocument() - expect(screen.getByText("ls")).toBeInTheDocument() - expect(screen.getByText("grep")).toBeInTheDocument() + expect(screen.getByText("ls -la | grep test")).toBeInTheDocument() }) it("should handle commands with && operator", () => { @@ -225,9 +230,7 @@ Suggested patterns: npm, npm install, npm run` ) expect(screen.getByTestId("command-pattern-selector")).toBeInTheDocument() - expect(screen.getByText("npm")).toBeInTheDocument() - expect(screen.getByText("npm install")).toBeInTheDocument() - expect(screen.getByText("npm test")).toBeInTheDocument() + expect(screen.getByText("npm install && npm test")).toBeInTheDocument() }) it("should not show pattern selector for empty commands", () => { @@ -279,25 +282,32 @@ Output here` expect(screen.getByTestId("command-pattern-selector")).toBeInTheDocument() }) - it("should handle pattern change when moving from denied to allowed", () => { + it("should handle command change when moving from denied to allowed", () => { + // Update the mock state to have "rm file.txt" in deniedCommands + const stateWithRmInDenied = { + ...mockExtensionState, + allowedCommands: ["npm"], + deniedCommands: ["rm file.txt"], + } + render( - + - , + , ) - const allowButton = screen.getByText("Allow rm") + const allowButton = screen.getByText("Allow rm file.txt") fireEvent.click(allowButton) - // rm should be removed from denied and added to allowed - expect(mockExtensionState.setAllowedCommands).toHaveBeenCalledWith(["npm", "rm"]) - expect(mockExtensionState.setDeniedCommands).toHaveBeenCalledWith([]) - expect(vscode.postMessage).toHaveBeenCalledWith({ type: "allowedCommands", commands: ["npm", "rm"] }) + // "rm file.txt" should be removed from denied and added to allowed + expect(stateWithRmInDenied.setAllowedCommands).toHaveBeenCalledWith(["npm", "rm file.txt"]) + expect(stateWithRmInDenied.setDeniedCommands).toHaveBeenCalledWith([]) + expect(vscode.postMessage).toHaveBeenCalledWith({ type: "allowedCommands", commands: ["npm", "rm file.txt"] }) expect(vscode.postMessage).toHaveBeenCalledWith({ type: "deniedCommands", commands: [] }) }) describe("integration with CommandPatternSelector", () => { - it("should extract patterns from complex commands with multiple operators", () => { + it("should show complex commands with multiple operators", () => { render( @@ -306,23 +316,20 @@ Output here` const selector = screen.getByTestId("command-pattern-selector") expect(selector).toBeInTheDocument() - expect(screen.getByText("npm")).toBeInTheDocument() - expect(screen.getByText("npm install")).toBeInTheDocument() - expect(screen.getByText("npm test")).toBeInTheDocument() - expect(screen.getByText("echo")).toBeInTheDocument() + expect(screen.getByText("npm install && npm test || echo 'failed'")).toBeInTheDocument() }) - it("should handle commands with malformed suggestions gracefully", () => { - const commandWithMalformedSuggestions = `npm install + it("should handle commands with output", () => { + const commandWithOutput = `npm install Output: -Suggested patterns: npm, , npm install, +Installing packages... Other output here` render( icon} title={Run Command} /> @@ -331,12 +338,11 @@ Other output here` const selector = screen.getByTestId("command-pattern-selector") expect(selector).toBeInTheDocument() - // Should still show valid patterns - expect(screen.getAllByText("npm")[0]).toBeInTheDocument() - expect(screen.getAllByText("npm install")[0]).toBeInTheDocument() + // Should show the command + expect(screen.getByText("npm install")).toBeInTheDocument() }) - it("should handle commands with subshells by not including them in patterns", () => { + it("should handle commands with subshells", () => { render( @@ -345,11 +351,7 @@ Other output here` const selector = screen.getByTestId("command-pattern-selector") expect(selector).toBeInTheDocument() - expect(screen.getByText("echo")).toBeInTheDocument() - expect(screen.getByText("git")).toBeInTheDocument() - expect(screen.getByText("git status")).toBeInTheDocument() - // Should not include subshell content - expect(screen.queryByText("whoami")).not.toBeInTheDocument() + expect(screen.getByText("echo $(whoami) && git status")).toBeInTheDocument() }) it("should handle commands with backtick subshells", () => { @@ -361,13 +363,10 @@ Other output here` const selector = screen.getByTestId("command-pattern-selector") expect(selector).toBeInTheDocument() - expect(screen.getByText("git")).toBeInTheDocument() - expect(screen.getByText("git commit")).toBeInTheDocument() - // Should not include subshell content - expect(screen.queryByText("date")).not.toBeInTheDocument() + expect(screen.getByText("git commit -m `date`")).toBeInTheDocument() }) - it("should handle pattern changes for commands with special characters", () => { + it("should handle commands with special characters", () => { render( @@ -376,22 +375,15 @@ Other output here` const selector = screen.getByTestId("command-pattern-selector") expect(selector).toBeInTheDocument() - expect(screen.getByText("cd")).toBeInTheDocument() - expect(screen.getByText("npm")).toBeInTheDocument() - expect(screen.getByText("npm start")).toBeInTheDocument() + expect(screen.getByText("cd ~/projects && npm start")).toBeInTheDocument() }) - it("should handle commands with mixed content including output and suggestions", () => { + it("should handle commands with mixed content including output", () => { const commandWithMixedContent = `npm test Output: Running tests... ✓ Test 1 passed -✓ Test 2 passed - -Suggested patterns: npm, npm test, npm run -- npm -- npm test -- npm run test` +✓ Test 2 passed` render( @@ -406,18 +398,15 @@ Suggested patterns: npm, npm test, npm run const selector = screen.getByTestId("command-pattern-selector") expect(selector).toBeInTheDocument() - // Should show patterns only from the actual command, not from AI suggestions - expect(screen.getAllByText("npm")[0]).toBeInTheDocument() - expect(screen.getAllByText("npm test")[0]).toBeInTheDocument() - // "npm run" should NOT be in the patterns since it's only in the AI suggestions - expect(screen.queryByText("npm run")).not.toBeInTheDocument() + // Should show the command + expect(screen.getByText("npm test")).toBeInTheDocument() }) - it("should update both allowed and denied lists when patterns conflict", () => { + it("should update both allowed and denied lists when commands conflict", () => { const conflictState = { ...mockExtensionState, allowedCommands: ["git"], - deniedCommands: ["git push"], + deniedCommands: ["git push origin main"], } render( @@ -426,31 +415,31 @@ Suggested patterns: npm, npm test, npm run , ) - // Click to allow "git push" - const allowButton = screen.getByText("Allow git push") + // Click to allow "git push origin main" + const allowButton = screen.getByText("Allow git push origin main") fireEvent.click(allowButton) // Should add to allowed and remove from denied - expect(conflictState.setAllowedCommands).toHaveBeenCalledWith(["git", "git push"]) + expect(conflictState.setAllowedCommands).toHaveBeenCalledWith(["git", "git push origin main"]) expect(conflictState.setDeniedCommands).toHaveBeenCalledWith([]) }) - it("should handle commands that cannot be parsed and fallback gracefully", () => { - // Test with a command that might cause parsing issues - const unparsableCommand = "echo 'test with unclosed quote" + it("should handle commands with special quotes", () => { + // Test with a command that has quotes + const commandWithQuotes = "echo 'test with unclosed quote" render( - + , ) // Should still render the command expect(screen.getByTestId("code-block")).toHaveTextContent("echo 'test with unclosed quote") - // Should show pattern selector with at least the main command + // Should show pattern selector with the full command expect(screen.getByTestId("command-pattern-selector")).toBeInTheDocument() - expect(screen.getByText("echo")).toBeInTheDocument() + expect(screen.getByText("echo 'test with unclosed quote")).toBeInTheDocument() }) it("should handle empty or whitespace-only commands", () => { @@ -486,9 +475,7 @@ Without any command prefix` expect(codeBlock.textContent).toContain("Without any command prefix") }) - it("should handle fallback case where parsed command equals original text", () => { - // This tests the case where parseCommandAndOutput returns command === text - // which happens when there's no output separator or command prefix + it("should handle simple commands", () => { const plainCommand = "docker build ." render( @@ -500,18 +487,16 @@ Without any command prefix` // Should render the command expect(screen.getByTestId("code-block")).toHaveTextContent("docker build .") - // Should show pattern selector with extracted patterns + // Should show pattern selector with the full command expect(screen.getByTestId("command-pattern-selector")).toBeInTheDocument() - expect(screen.getByText("docker")).toBeInTheDocument() - expect(screen.getByText("docker build")).toBeInTheDocument() + expect(screen.getByText("docker build .")).toBeInTheDocument() - // Verify no output is shown (since command === text means no output) + // Verify no output is shown (since there's no Output: separator) const codeBlocks = screen.getAllByTestId("code-block") expect(codeBlocks).toHaveLength(1) // Only the command block, no output block }) - it("should not extract patterns from command output numbers", () => { - // This tests the specific bug where "0 total" from wc output was being extracted as a command + it("should handle commands with numeric output", () => { const commandWithNumericOutput = `wc -l *.go *.java Output: 10 file1.go @@ -533,17 +518,15 @@ Output: const selector = screen.getByTestId("command-pattern-selector") expect(selector).toBeInTheDocument() - // Should only extract "wc" from the actual command - expect(screen.getByText("wc")).toBeInTheDocument() + // Should show the full command + expect(screen.getByText("wc -l *.go *.java")).toBeInTheDocument() - // Should NOT extract numeric patterns from output like "45 total" - expect(screen.queryByText("45")).not.toBeInTheDocument() - expect(screen.queryByText("total")).not.toBeInTheDocument() - expect(screen.queryByText("45 total")).not.toBeInTheDocument() + // The output should still be displayed in the code block + expect(codeBlocks.length).toBeGreaterThan(1) + expect(codeBlocks[1].textContent).toContain("45 total") }) - it("should handle the edge case of 0 total in output", () => { - // This is the exact case from the bug report + it("should handle commands with zero output", () => { const commandWithZeroTotal = `wc -l *.go *.java Output: 0 total` @@ -558,17 +541,8 @@ Output: const selector = screen.getByTestId("command-pattern-selector") expect(selector).toBeInTheDocument() - // Should only extract "wc" from the actual command - // Check within the pattern selector specifically - const patternTexts = Array.from(selector.querySelectorAll("span")).map((el) => el.textContent) - - // Should have "wc" as a pattern - expect(patternTexts).toContain("wc") - - // Should NOT have "0", "total", or "0 total" as patterns - expect(patternTexts).not.toContain("0") - expect(patternTexts).not.toContain("total") - expect(patternTexts).not.toContain("0 total") + // Should show the full command + expect(screen.getByText("wc -l *.go *.java")).toBeInTheDocument() // The output should still be displayed in the code block const codeBlocks = screen.getAllByTestId("code-block") diff --git a/webview-ui/src/components/chat/__tests__/CommandPatternSelector.spec.tsx b/webview-ui/src/components/chat/__tests__/CommandPatternSelector.spec.tsx index 815d4dfbfb..449fd504c6 100644 --- a/webview-ui/src/components/chat/__tests__/CommandPatternSelector.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/CommandPatternSelector.spec.tsx @@ -4,11 +4,6 @@ import { describe, it, expect, vi } from "vitest" import { CommandPatternSelector } from "../CommandPatternSelector" import { TooltipProvider } from "../../../components/ui/tooltip" -interface CommandPattern { - pattern: string - description?: string -} - // Mock react-i18next vi.mock("react-i18next", () => ({ useTranslation: () => ({ @@ -30,21 +25,15 @@ vi.mock("@vscode/webview-ui-toolkit/react", () => ({ const TestWrapper = ({ children }: { children: React.ReactNode }) => {children} describe("CommandPatternSelector", () => { - const mockPatterns: CommandPattern[] = [ - { pattern: "npm", description: "npm commands" }, - { pattern: "npm install", description: "npm install commands" }, - { pattern: "git", description: "git commands" }, - ] - const defaultProps = { - patterns: mockPatterns, - allowedCommands: ["npm"], - deniedCommands: ["git"], - onAllowPatternChange: vi.fn(), - onDenyPatternChange: vi.fn(), + command: "npm install express", + allowedCommands: ["npm install"], + deniedCommands: ["git push"], + onAllowCommandChange: vi.fn(), + onDenyCommandChange: vi.fn(), } - it("should render with unique pattern keys", () => { + it("should render with command input", () => { const { container } = render( @@ -58,39 +47,148 @@ describe("CommandPatternSelector", () => { const expandButton = screen.getByRole("button", { name: /chat:commandExecution.expandManagement/i }) fireEvent.click(expandButton) - // Check that patterns are rendered - expect(screen.getByText("npm")).toBeInTheDocument() - expect(screen.getByText("npm install")).toBeInTheDocument() - expect(screen.getByText("git")).toBeInTheDocument() + // Check that the input is rendered with the command + const input = screen.getByDisplayValue("npm install express") + expect(input).toBeInTheDocument() }) - it("should handle duplicate patterns gracefully", () => { - // Test with duplicate patterns to ensure keys are still unique - const duplicatePatterns: CommandPattern[] = [ - { pattern: "npm", description: "npm commands" }, - { pattern: "npm", description: "duplicate npm commands" }, // Duplicate pattern - { pattern: "git", description: "git commands" }, - ] - - const props = { - ...defaultProps, - patterns: duplicatePatterns, - } - - // This should not throw an error even with duplicate patterns - const { container } = render( + it("should allow editing the command", () => { + render( - + , ) - expect(container).toBeTruthy() // Click to expand the component const expandButton = screen.getByRole("button", { name: /chat:commandExecution.expandManagement/i }) fireEvent.click(expandButton) - // Both instances of "npm" should be rendered - const npmElements = screen.getAllByText("npm") - expect(npmElements).toHaveLength(2) + // Get the input and change its value + const input = screen.getByDisplayValue("npm install express") as HTMLInputElement + fireEvent.change(input, { target: { value: "npm install react" } }) + + // Check that the input value has changed + expect(input.value).toBe("npm install react") + }) + + it("should show allowed status for commands in allowed list", () => { + const props = { + ...defaultProps, + command: "npm install", + } + + render( + + + , + ) + + // Click to expand the component + const expandButton = screen.getByRole("button", { name: /chat:commandExecution.expandManagement/i }) + fireEvent.click(expandButton) + + // The allow button should have the active styling (we can check by aria-label) + const allowButton = screen.getByRole("button", { name: /chat:commandExecution.removeFromAllowed/i }) + expect(allowButton).toBeInTheDocument() + }) + + it("should show denied status for commands in denied list", () => { + const props = { + ...defaultProps, + command: "git push", + } + + render( + + + , + ) + + // Click to expand the component + const expandButton = screen.getByRole("button", { name: /chat:commandExecution.expandManagement/i }) + fireEvent.click(expandButton) + + // The deny button should have the active styling (we can check by aria-label) + const denyButton = screen.getByRole("button", { name: /chat:commandExecution.removeFromDenied/i }) + expect(denyButton).toBeInTheDocument() + }) + + it("should call onAllowCommandChange when allow button is clicked", () => { + const mockOnAllowCommandChange = vi.fn() + const props = { + ...defaultProps, + onAllowCommandChange: mockOnAllowCommandChange, + } + + render( + + + , + ) + + // Click to expand the component + const expandButton = screen.getByRole("button", { name: /chat:commandExecution.expandManagement/i }) + fireEvent.click(expandButton) + + // Click the allow button + const allowButton = screen.getByRole("button", { name: /chat:commandExecution.addToAllowed/i }) + fireEvent.click(allowButton) + + // Check that the callback was called with the command + expect(mockOnAllowCommandChange).toHaveBeenCalledWith("npm install express") + }) + + it("should call onDenyCommandChange when deny button is clicked", () => { + const mockOnDenyCommandChange = vi.fn() + const props = { + ...defaultProps, + onDenyCommandChange: mockOnDenyCommandChange, + } + + render( + + + , + ) + + // Click to expand the component + const expandButton = screen.getByRole("button", { name: /chat:commandExecution.expandManagement/i }) + fireEvent.click(expandButton) + + // Click the deny button + const denyButton = screen.getByRole("button", { name: /chat:commandExecution.addToDenied/i }) + fireEvent.click(denyButton) + + // Check that the callback was called with the command + expect(mockOnDenyCommandChange).toHaveBeenCalledWith("npm install express") + }) + + it("should use edited command value when buttons are clicked", () => { + const mockOnAllowCommandChange = vi.fn() + const props = { + ...defaultProps, + onAllowCommandChange: mockOnAllowCommandChange, + } + + render( + + + , + ) + + // Click to expand the component + const expandButton = screen.getByRole("button", { name: /chat:commandExecution.expandManagement/i }) + fireEvent.click(expandButton) + + // Edit the command + const input = screen.getByDisplayValue("npm install express") as HTMLInputElement + fireEvent.change(input, { target: { value: "npm install react" } }) + + // Click the allow button + const allowButton = screen.getByRole("button", { name: /chat:commandExecution.addToAllowed/i }) + fireEvent.click(allowButton) + + // Check that the callback was called with the edited command + expect(mockOnAllowCommandChange).toHaveBeenCalledWith("npm install react") }) }) diff --git a/webview-ui/src/utils/__tests__/command-parser.spec.ts b/webview-ui/src/utils/__tests__/command-parser.spec.ts deleted file mode 100644 index 05303f87fc..0000000000 --- a/webview-ui/src/utils/__tests__/command-parser.spec.ts +++ /dev/null @@ -1,137 +0,0 @@ -import { describe, it, expect } from "vitest" -import { extractPatternsFromCommand } from "../command-parser" - -describe("extractPatternsFromCommand", () => { - it("should extract simple command pattern", () => { - const patterns = extractPatternsFromCommand("ls") - expect(patterns).toEqual(["ls"]) - }) - - it("should extract command with subcommand", () => { - const patterns = extractPatternsFromCommand("git push origin main") - expect(patterns).toEqual(["git", "git push", "git push origin"]) - }) - - it("should stop at flags", () => { - const patterns = extractPatternsFromCommand("git commit -m 'test'") - expect(patterns).toEqual(["git", "git commit"]) - }) - - it("should stop at paths", () => { - const patterns = extractPatternsFromCommand("cd /usr/local/bin") - expect(patterns).toEqual(["cd"]) - }) - - it("should handle pipes", () => { - const patterns = extractPatternsFromCommand("ls -la | grep test") - expect(patterns).toEqual(["grep", "grep test", "ls"]) - }) - - it("should handle && operator", () => { - const patterns = extractPatternsFromCommand("npm install && git push origin main") - expect(patterns).toEqual(["git", "git push", "git push origin", "npm", "npm install"]) - }) - - it("should handle || operator", () => { - const patterns = extractPatternsFromCommand("npm test || npm run test:ci") - expect(patterns).toEqual(["npm", "npm run", "npm test"]) - }) - - it("should handle semicolon separator", () => { - const patterns = extractPatternsFromCommand("cd src; npm install") - expect(patterns).toEqual(["cd", "cd src", "npm", "npm install"]) - }) - - it("should skip numeric commands", () => { - const patterns = extractPatternsFromCommand("0 total") - expect(patterns).toEqual([]) - }) - - it("should handle empty command", () => { - const patterns = extractPatternsFromCommand("") - expect(patterns).toEqual([]) - }) - - it("should handle null/undefined", () => { - expect(extractPatternsFromCommand(null as any)).toEqual([]) - expect(extractPatternsFromCommand(undefined as any)).toEqual([]) - }) - - it("should handle scripts", () => { - const patterns = extractPatternsFromCommand("./script.sh --verbose") - expect(patterns).toEqual(["./script.sh"]) - }) - - it("should handle paths with dots", () => { - const patterns = extractPatternsFromCommand("git add .") - expect(patterns).toEqual(["git", "git add"]) - }) - - it("should handle paths with tilde", () => { - const patterns = extractPatternsFromCommand("cd ~/projects") - expect(patterns).toEqual(["cd"]) - }) - - it("should handle colons in arguments", () => { - const patterns = extractPatternsFromCommand("docker run image:tag") - expect(patterns).toEqual(["docker", "docker run"]) - }) - - it("should return sorted patterns", () => { - const patterns = extractPatternsFromCommand("npm run build && git push") - expect(patterns).toEqual(["git", "git push", "npm", "npm run", "npm run build"]) - }) - - it("should handle complex command with multiple operators", () => { - const patterns = extractPatternsFromCommand("npm install && npm test | grep success || echo 'failed'") - expect(patterns).toContain("npm") - expect(patterns).toContain("npm install") - expect(patterns).toContain("npm test") - expect(patterns).toContain("grep") - expect(patterns).toContain("echo") - }) - - it("should handle malformed commands gracefully", () => { - const patterns = extractPatternsFromCommand("echo 'unclosed quote") - expect(patterns).toContain("echo") - }) - - it("should not treat package managers specially", () => { - const patterns = extractPatternsFromCommand("npm run build") - expect(patterns).toEqual(["npm", "npm run", "npm run build"]) - // Now includes "npm run build" with 3-level extraction - }) - - it("should extract at most 3 levels", () => { - const patterns = extractPatternsFromCommand("git push origin main --force") - expect(patterns).toEqual(["git", "git push", "git push origin"]) - // Should NOT include deeper levels beyond 3 - }) - - it("should handle multi-level commands like gh pr", () => { - const patterns = extractPatternsFromCommand("gh pr checkout 123") - expect(patterns).toEqual(["gh", "gh pr", "gh pr checkout"]) - }) - - it("should extract 3 levels for git remote add", () => { - const patterns = extractPatternsFromCommand("git remote add origin https://github.com/user/repo.git") - expect(patterns).toEqual(["git", "git remote", "git remote add"]) - }) - - it("should extract 3 levels for npm run build", () => { - const patterns = extractPatternsFromCommand("npm run build --production") - expect(patterns).toEqual(["npm", "npm run", "npm run build"]) - }) - - it("should stop at file extensions even at third level", () => { - const patterns = extractPatternsFromCommand("node scripts test.js") - expect(patterns).toEqual(["node", "node scripts"]) - // Should NOT include "node scripts test.js" because of .js - }) - - it("should stop at flags at any level", () => { - const patterns = extractPatternsFromCommand("docker run -it ubuntu") - expect(patterns).toEqual(["docker", "docker run"]) - // Stops at -it flag - }) -}) diff --git a/webview-ui/src/utils/command-parser.ts b/webview-ui/src/utils/command-parser.ts deleted file mode 100644 index 14a4480ae7..0000000000 --- a/webview-ui/src/utils/command-parser.ts +++ /dev/null @@ -1,68 +0,0 @@ -import { parse } from "shell-quote" - -/** - * Extract command patterns from a command string. - * Returns at most 3 levels: base command, command + first argument, and command + first two arguments. - * Stops at flags (-), paths (/\~), file extensions (.ext), or special characters (:). - */ -export function extractPatternsFromCommand(command: string): string[] { - if (!command?.trim()) return [] - - const patterns = new Set() - - try { - const parsed = parse(command) - const commandSeparators = new Set(["|", "&&", "||", ";"]) - let currentTokens: string[] = [] - - for (const token of parsed) { - if (typeof token === "object" && "op" in token && commandSeparators.has(token.op)) { - // Process accumulated tokens as a command - if (currentTokens.length > 0) { - extractFromTokens(currentTokens, patterns) - currentTokens = [] - } - } else if (typeof token === "string") { - currentTokens.push(token) - } - } - - // Process any remaining tokens - if (currentTokens.length > 0) { - extractFromTokens(currentTokens, patterns) - } - } catch (error) { - console.warn("Failed to parse command:", error) - // Fallback: just extract the first word - const firstWord = command.trim().split(/\s+/)[0] - if (firstWord) patterns.add(firstWord) - } - - return Array.from(patterns).sort() -} - -function isValidToken(token: string): boolean { - return !!token && !token.startsWith("-") && !token.match(/[/\\~:]/) && token !== "." && !token.match(/\.\w+$/) -} - -function extractFromTokens(tokens: string[], patterns: Set): void { - if (tokens.length === 0) return - - const mainCmd = tokens[0] - - // Skip numeric commands like "0" from "0 total" - if (/^\d+$/.test(mainCmd)) return - - // Build patterns progressively up to 3 levels - let pattern = mainCmd - patterns.add(pattern) - - for (let i = 1; i < Math.min(tokens.length, 3); i++) { - if (isValidToken(tokens[i])) { - pattern += ` ${tokens[i]}` - patterns.add(pattern) - } else { - break // Stop at first invalid token - } - } -}