fix: address PR review feedback

- Fix import error in CommandExecution.tsx (removed undefined parseCommandAndOutput)
- Add security features with detectSecurityIssues function for subshell detection
- Remove hardcoded command descriptions, use dynamic pattern instead
- Add comprehensive security tests for subshell detection
- Add integration tests for CommandExecution + CommandPatternSelector
- Fix Polish translation typo: 'z list' -> 'z listy'
- Simplify commandPatterns.ts by removing unnecessary complexity

webview-ui/src/components/chat/CommandExecution.tsx

@@ -6,7 +6,6 @@ import { CommandExecutionStatus, commandExecutionStatusSchema } from "@roo-code/
 
 import { ExtensionMessage } from "@roo/ExtensionMessage"
 import { safeJsonParse } from "@roo/safeJsonParse"
-import { COMMAND_OUTPUT_STRING } from "@roo/combineCommandSequences"
 
 import { vscode } from "@src/utils/vscode"
 import { useExtensionState } from "@src/context/ExtensionStateContext"
@@ -42,15 +41,8 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec
 		output: parsedOutput,
 		suggestions,
 	} = useMemo(() => {
-		// First try our enhanced parser
-		const enhanced = parseCommandAndOutputUtil(text || "")
-		// If it found a command, use it, otherwise fall back to the original parser
-		if (enhanced.command && enhanced.command !== text) {
-			return enhanced
-		}
-		// Fall back to original parser
-		const original = parseCommandAndOutput(text)
-		return { ...original, suggestions: [] }
+		// Use the enhanced parser from commandPatterns
+		return parseCommandAndOutputUtil(text || "")
 	}, [text])
 
 	// If we aren't opening the VSCode terminal for this command then we default
@@ -230,20 +222,3 @@ const OutputContainerInternal = ({ isExpanded, output }: { isExpanded: boolean;
 )
 
 const OutputContainer = memo(OutputContainerInternal)
-
-const parseCommandAndOutput = (text: string | undefined) => {
-	if (!text) {
-		return { command: "", output: "" }
-	}
-
-	const index = text.indexOf(COMMAND_OUTPUT_STRING)
-
-	if (index === -1) {
-		return { command: text, output: "" }
-	}
-
-	return {
-		command: text.slice(0, index),
-		output: text.slice(index + COMMAND_OUTPUT_STRING.length),
-	}
-}

webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx

@@ -274,4 +274,143 @@ describe("CommandExecution", () => {
 		expect(vscode.postMessage).toHaveBeenCalledWith({ type: "allowedCommands", commands: ["npm", "rm"] })
 		expect(vscode.postMessage).toHaveBeenCalledWith({ type: "deniedCommands", commands: [] })
 	})
+
+	describe("integration with CommandPatternSelector", () => {
+		it("should extract patterns from complex commands with multiple operators", () => {
+			render(
+				<ExtensionStateWrapper>
+					<CommandExecution executionId="test-6" text="npm install && npm test || echo 'failed'" />
+				</ExtensionStateWrapper>,
+			)
+
+			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()
+		})
+
+		it("should handle commands with malformed suggestions gracefully", () => {
+			const commandWithMalformedSuggestions = `npm install
+Output:
+Suggested patterns: npm, , npm install,
+Other output here`
+
+			render(
+				<ExtensionStateWrapper>
+					<CommandExecution
+						executionId="test-6"
+						text={commandWithMalformedSuggestions}
+						icon={<span>icon</span>}
+						title={<span>Run Command</span>}
+					/>
+				</ExtensionStateWrapper>,
+			)
+
+			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()
+		})
+
+		it("should handle commands with subshells by not including them in patterns", () => {
+			render(
+				<ExtensionStateWrapper>
+					<CommandExecution executionId="test-7" text="echo $(whoami) && git status" />
+				</ExtensionStateWrapper>,
+			)
+
+			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()
+		})
+
+		it("should handle commands with backtick subshells", () => {
+			render(
+				<ExtensionStateWrapper>
+					<CommandExecution executionId="test-8" text="git commit -m `date`" />
+				</ExtensionStateWrapper>,
+			)
+
+			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()
+		})
+
+		it("should handle pattern changes for commands with special characters", () => {
+			render(
+				<ExtensionStateWrapper>
+					<CommandExecution executionId="test-9" text="cd ~/projects && npm start" />
+				</ExtensionStateWrapper>,
+			)
+
+			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()
+		})
+
+		it("should handle commands with mixed content including output and suggestions", () => {
+			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`
+
+			render(
+				<ExtensionStateWrapper>
+					<CommandExecution
+						executionId="test-10"
+						text={commandWithMixedContent}
+						icon={<span>icon</span>}
+						title={<span>Run Command</span>}
+					/>
+				</ExtensionStateWrapper>,
+			)
+
+			const selector = screen.getByTestId("command-pattern-selector")
+			expect(selector).toBeInTheDocument()
+			// Should show patterns from suggestions
+			expect(screen.getAllByText("npm")[0]).toBeInTheDocument()
+			expect(screen.getAllByText("npm test")[0]).toBeInTheDocument()
+			expect(screen.getAllByText("npm run")[0]).toBeInTheDocument()
+		})
+
+		it("should update both allowed and denied lists when patterns conflict", () => {
+			const conflictState = {
+				...mockExtensionState,
+				allowedCommands: ["git"],
+				deniedCommands: ["git push"],
+			}
+
+			render(
+				<ExtensionStateContext.Provider value={conflictState as any}>
+					<CommandExecution executionId="test-11" text="git push origin main" />
+				</ExtensionStateContext.Provider>,
+			)
+
+			// Click to allow "git push"
+			const allowButton = screen.getByText("Allow git push")
+			fireEvent.click(allowButton)
+
+			// Should add to allowed and remove from denied
+			expect(conflictState.setAllowedCommands).toHaveBeenCalledWith(["git", "git push"])
+			expect(conflictState.setDeniedCommands).toHaveBeenCalledWith([])
+		})
+	})
 })

webview-ui/src/i18n/locales/pl/chat.json

@@ -197,7 +197,7 @@
 		"pid": "PID: {{pid}}",
 		"exited": "Zakończono ({{exitCode}})",
 		"manageCommands": "Zarządzaj uprawnieniami poleceń",
-		"commandManagementDescription": "Zarządzaj uprawnieniami poleceń: Kliknij ✓, aby zezwolić na automatyczne wykonanie, ✗, aby odmówić wykonania. Wzorce można włączać/wyłączać lub usuwać z list. <settingsLink>Zobacz wszystkie ustawienia</settingsLink>",
+		"commandManagementDescription": "Zarządzaj uprawnieniami poleceń: Kliknij ✓, aby zezwolić na automatyczne wykonanie, ✗, aby odmówić wykonania. Wzorce można włączać/wyłączać lub usuwać z listy. <settingsLink>Zobacz wszystkie ustawienia</settingsLink>",
 		"addToAllowed": "Dodaj do listy dozwolonych",
 		"removeFromAllowed": "Usuń z listy dozwolonych",
 		"addToDenied": "Dodaj do listy odrzuconych",

webview-ui/src/utils/__tests__/commandPatterns.spec.ts

@@ -1,5 +1,10 @@
 import { describe, it, expect } from "vitest"
-import { extractCommandPatterns, getPatternDescription, parseCommandAndOutput } from "../commandPatterns"
+import {
+	extractCommandPatterns,
+	getPatternDescription,
+	parseCommandAndOutput,
+	detectSecurityIssues,
+} from "../commandPatterns"
 
 describe("extractCommandPatterns", () => {
 	it("should extract simple command", () => {
@@ -99,16 +104,16 @@ describe("extractCommandPatterns", () => {
 })
 
 describe("getPatternDescription", () => {
-	it("should return descriptions for common commands", () => {
-		expect(getPatternDescription("cd")).toBe("directory navigation")
+	it("should return pattern followed by commands", () => {
+		expect(getPatternDescription("cd")).toBe("cd commands")
 		expect(getPatternDescription("npm")).toBe("npm commands")
 		expect(getPatternDescription("npm install")).toBe("npm install commands")
 		expect(getPatternDescription("git")).toBe("git commands")
 		expect(getPatternDescription("git push")).toBe("git push commands")
-		expect(getPatternDescription("python")).toBe("python scripts")
+		expect(getPatternDescription("python")).toBe("python commands")
 	})
 
-	it("should return default description for unknown commands", () => {
+	it("should handle any command pattern", () => {
 		expect(getPatternDescription("unknowncommand")).toBe("unknowncommand commands")
 		expect(getPatternDescription("custom-tool")).toBe("custom-tool commands")
 	})
@@ -116,13 +121,13 @@ describe("getPatternDescription", () => {
 	it("should handle package managers", () => {
 		expect(getPatternDescription("yarn")).toBe("yarn commands")
 		expect(getPatternDescription("pnpm")).toBe("pnpm commands")
-		expect(getPatternDescription("bun")).toBe("bun scripts")
+		expect(getPatternDescription("bun")).toBe("bun commands")
 	})
 
 	it("should handle build tools", () => {
-		expect(getPatternDescription("make")).toBe("build automation")
-		expect(getPatternDescription("cmake")).toBe("CMake build system")
-		expect(getPatternDescription("cargo")).toBe("Rust cargo commands")
+		expect(getPatternDescription("make")).toBe("make commands")
+		expect(getPatternDescription("cmake")).toBe("cmake commands")
+		expect(getPatternDescription("cargo")).toBe("cargo commands")
 		expect(getPatternDescription("go build")).toBe("go build commands")
 	})
 })
@@ -273,3 +278,77 @@ Installing...`
 		expect(result.output).toBe("First output\nOutput: Second output")
 	})
 })
+
+describe("detectSecurityIssues", () => {
+	it("should detect subshell execution with $()", () => {
+		const warnings = detectSecurityIssues("echo $(malicious)")
+		expect(warnings).toHaveLength(1)
+		expect(warnings[0].type).toBe("subshell")
+		expect(warnings[0].message).toContain("subshell execution")
+	})
+
+	it("should detect subshell execution with backticks", () => {
+		const warnings = detectSecurityIssues("echo `malicious`")
+		expect(warnings).toHaveLength(1)
+		expect(warnings[0].type).toBe("subshell")
+		expect(warnings[0].message).toContain("subshell execution")
+	})
+
+	it("should detect nested subshells", () => {
+		const warnings = detectSecurityIssues("echo $(echo $(date))")
+		expect(warnings).toHaveLength(1)
+		expect(warnings[0].type).toBe("subshell")
+	})
+
+	it("should detect subshells in complex commands", () => {
+		const warnings = detectSecurityIssues("npm install && echo $(whoami) || git push")
+		expect(warnings).toHaveLength(1)
+		expect(warnings[0].type).toBe("subshell")
+	})
+
+	it("should not detect issues in safe commands", () => {
+		const warnings = detectSecurityIssues("npm install express")
+		expect(warnings).toHaveLength(0)
+	})
+
+	it("should handle empty commands", () => {
+		const warnings = detectSecurityIssues("")
+		expect(warnings).toHaveLength(0)
+	})
+
+	it("should detect multiple subshell patterns", () => {
+		const warnings = detectSecurityIssues("echo $(date) && echo `whoami`")
+		expect(warnings).toHaveLength(1) // Should still be 1 warning for subshell presence
+		expect(warnings[0].type).toBe("subshell")
+	})
+
+	it("should detect subshells in quoted strings", () => {
+		const warnings = detectSecurityIssues('echo "Current user: $(whoami)"')
+		expect(warnings).toHaveLength(1)
+		expect(warnings[0].type).toBe("subshell")
+	})
+})
+
+describe("security integration with extractCommandPatterns", () => {
+	it("should not include subshell content in patterns", () => {
+		const patterns = extractCommandPatterns("echo $(malicious)")
+		expect(patterns).toContain("echo")
+		expect(patterns).not.toContain("$(malicious)")
+		expect(patterns).not.toContain("malicious")
+	})
+
+	it("should handle commands with subshells properly", () => {
+		const patterns = extractCommandPatterns("npm install && echo $(whoami)")
+		expect(patterns).toContain("npm")
+		expect(patterns).toContain("npm install")
+		expect(patterns).toContain("echo")
+		expect(patterns).not.toContain("whoami")
+	})
+
+	it("should extract patterns from commands with backtick subshells", () => {
+		const patterns = extractCommandPatterns("git commit -m `date`")
+		expect(patterns).toContain("git")
+		expect(patterns).toContain("git commit")
+		expect(patterns).not.toContain("date")
+	})
+})

webview-ui/src/utils/commandPatterns.ts

@@ -5,13 +5,23 @@ export interface CommandPattern {
 	description?: string
 }
 
+export interface SecurityWarning {
+	type: "subshell" | "injection"
+	message: string
+}
+
 export function extractCommandPatterns(command: string): string[] {
 	if (!command?.trim()) return []
 
 	const patterns = new Set<string>()
 
 	try {
-		const parsed = parse(command)
+		// First, remove subshell expressions to avoid extracting their contents
+		const cleanedCommand = command
+			.replace(/\$\([^)]*\)/g, "") // Remove $() subshells
+			.replace(/`[^`]*`/g, "") // Remove backtick subshells
+
+		const parsed = parse(cleanedCommand)
 
 		const commandSeparators = new Set(["|", "&&", "||", ";"])
 		let current: any[] = []
@@ -54,73 +64,26 @@ function processCommand(cmd: any[], patterns: Set<string>) {
 	}
 }
 
-export function getPatternDescription(pattern: string): string {
-	// Generate human-readable descriptions for common patterns
-	const descriptions: Record<string, string> = {
-		cd: "directory navigation",
-		ls: "list directory contents",
-		pwd: "print working directory",
-		mkdir: "create directories",
-		rm: "remove files/directories",
-		cp: "copy files/directories",
-		mv: "move/rename files",
-		cat: "display file contents",
-		echo: "display text",
-		npm: "npm commands",
-		"npm install": "npm install commands",
-		"npm run": "all npm run scripts",
-		"npm test": "npm test commands",
-		"npm start": "npm start commands",
-		"npm build": "npm build commands",
-		yarn: "yarn commands",
-		"yarn install": "yarn install commands",
-		"yarn run": "all yarn run scripts",
-		pnpm: "pnpm commands",
-		"pnpm install": "pnpm install commands",
-		"pnpm run": "all pnpm run scripts",
-		git: "git commands",
-		"git add": "git add commands",
-		"git commit": "git commit commands",
-		"git push": "git push commands",
-		"git pull": "git pull commands",
-		"git clone": "git clone commands",
-		"git checkout": "git checkout commands",
-		"git branch": "git branch commands",
-		"git merge": "git merge commands",
-		"git status": "git status commands",
-		"git log": "git log commands",
-		python: "python scripts",
-		python3: "python3 scripts",
-		node: "node.js scripts",
-		deno: "deno scripts",
-		bun: "bun scripts",
-		docker: "docker commands",
-		"docker run": "docker run commands",
-		"docker build": "docker build commands",
-		"docker compose": "docker compose commands",
-		curl: "HTTP requests",
-		wget: "download files",
-		grep: "search text patterns",
-		find: "find files/directories",
-		sed: "stream editor",
-		awk: "text processing",
-		make: "build automation",
-		cmake: "CMake build system",
-		go: "go commands",
-		"go run": "go run commands",
-		"go build": "go build commands",
-		"go test": "go test commands",
-		cargo: "Rust cargo commands",
-		"cargo run": "cargo run commands",
-		"cargo build": "cargo build commands",
-		"cargo test": "cargo test commands",
-		dotnet: ".NET commands",
-		"dotnet run": "dotnet run commands",
-		"dotnet build": "dotnet build commands",
-		"dotnet test": "dotnet test commands",
+export function detectSecurityIssues(command: string): SecurityWarning[] {
+	const warnings: SecurityWarning[] = []
+
+	// Check for subshell execution attempts
+	if (command.includes("$(") || command.includes("`")) {
+		warnings.push({
+			type: "subshell",
+			message: "Command contains subshell execution which could bypass restrictions",
+		})
 	}
 
-	return descriptions[pattern] || `${pattern} commands`
+	return warnings
+}
+
+/**
+ * Get a human-readable description for a command pattern.
+ * Simply returns the pattern followed by "commands".
+ */
+export function getPatternDescription(pattern: string): string {
+	return `${pattern} commands`
 }
 
 export function parseCommandAndOutput(text: string): {