mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-05 08:10:14 +00:00
fix: prevent command output from appearing in permissions UI
- Fixed CommandExecution.tsx to only extract patterns from actual commands, not AI suggestions - Enhanced extractCommandPatterns to filter out numeric patterns and common output words - Added comprehensive test coverage for the bug scenario - Ensures 'Manage Command Permissions' only shows actual executed commands Fixes the issue where output like '0 total' from wc commands was incorrectly shown as a command pattern
This commit is contained in:
parent
ebf1b241c1
commit
f6642f6afe
4 changed files with 192 additions and 30 deletions
|
|
@ -36,11 +36,7 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec
|
|||
setDeniedCommands,
|
||||
} = useExtensionState()
|
||||
|
||||
const {
|
||||
command,
|
||||
output: parsedOutput,
|
||||
suggestions,
|
||||
} = useMemo(() => {
|
||||
const { command, output: parsedOutput } = useMemo(() => {
|
||||
// Use the enhanced parser from commandPatterns
|
||||
return parseCommandAndOutput(text || "")
|
||||
}, [text])
|
||||
|
|
@ -58,31 +54,23 @@ export const CommandExecution = ({ executionId, text, icon, title }: CommandExec
|
|||
// streaming output (this is the case for running commands).
|
||||
const output = streamingOutput || parsedOutput
|
||||
|
||||
// Extract command patterns
|
||||
// Extract command patterns from the actual command that was executed
|
||||
const commandPatterns = useMemo<CommandPattern[]>(() => {
|
||||
const patterns: CommandPattern[] = []
|
||||
|
||||
// Use AI suggestions if available
|
||||
if (suggestions.length > 0) {
|
||||
suggestions.forEach((suggestion: string) => {
|
||||
patterns.push({
|
||||
pattern: suggestion,
|
||||
description: getPatternDescription(suggestion),
|
||||
})
|
||||
// Always extract patterns from the actual command that was executed
|
||||
// We don't use AI suggestions because the patterns should reflect
|
||||
// what was actually executed, not what the AI thinks might be useful
|
||||
const extractedPatterns = extractCommandPatterns(command)
|
||||
extractedPatterns.forEach((pattern) => {
|
||||
patterns.push({
|
||||
pattern,
|
||||
description: getPatternDescription(pattern),
|
||||
})
|
||||
} else {
|
||||
// Extract patterns programmatically
|
||||
const extractedPatterns = extractCommandPatterns(command)
|
||||
extractedPatterns.forEach((pattern) => {
|
||||
patterns.push({
|
||||
pattern,
|
||||
description: getPatternDescription(pattern),
|
||||
})
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
return patterns
|
||||
}, [command, suggestions])
|
||||
}, [command])
|
||||
|
||||
// Handle pattern changes
|
||||
const handleAllowPatternChange = (pattern: string) => {
|
||||
|
|
|
|||
|
|
@ -211,10 +211,11 @@ 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 the patterns are present in the mock
|
||||
// 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)
|
||||
expect(screen.getByText("npm run")).toBeInTheDocument()
|
||||
// "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()
|
||||
})
|
||||
|
||||
it("should handle commands with pipes", () => {
|
||||
|
|
@ -417,10 +418,11 @@ Suggested patterns: npm, npm test, npm run
|
|||
|
||||
const selector = screen.getByTestId("command-pattern-selector")
|
||||
expect(selector).toBeInTheDocument()
|
||||
// Should show patterns from suggestions
|
||||
// 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()
|
||||
expect(screen.getAllByText("npm run")[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()
|
||||
})
|
||||
|
||||
it("should update both allowed and denied lists when patterns conflict", () => {
|
||||
|
|
@ -519,5 +521,71 @@ Without any command prefix`
|
|||
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
|
||||
const commandWithNumericOutput = `wc -l *.go *.java
|
||||
Output:
|
||||
10 file1.go
|
||||
20 file2.go
|
||||
15 Main.java
|
||||
45 total`
|
||||
|
||||
render(
|
||||
<ExtensionStateWrapper>
|
||||
<CommandExecution executionId="test-16" text={commandWithNumericOutput} />
|
||||
</ExtensionStateWrapper>,
|
||||
)
|
||||
|
||||
// Should render the command and output
|
||||
const codeBlocks = screen.getAllByTestId("code-block")
|
||||
expect(codeBlocks[0]).toHaveTextContent("wc -l *.go *.java")
|
||||
|
||||
// Should show pattern selector
|
||||
const selector = screen.getByTestId("command-pattern-selector")
|
||||
expect(selector).toBeInTheDocument()
|
||||
|
||||
// Should only extract "wc" from the actual command
|
||||
expect(screen.getByText("wc")).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()
|
||||
})
|
||||
|
||||
it("should handle the edge case of 0 total in output", () => {
|
||||
// This is the exact case from the bug report
|
||||
const commandWithZeroTotal = `wc -l *.go *.java
|
||||
Output:
|
||||
0 total`
|
||||
|
||||
render(
|
||||
<ExtensionStateWrapper>
|
||||
<CommandExecution executionId="test-17" text={commandWithZeroTotal} />
|
||||
</ExtensionStateWrapper>,
|
||||
)
|
||||
|
||||
// Should show pattern selector
|
||||
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")
|
||||
|
||||
// The output should still be displayed in the code block
|
||||
const codeBlocks = screen.getAllByTestId("code-block")
|
||||
expect(codeBlocks.length).toBeGreaterThan(1)
|
||||
expect(codeBlocks[1]).toHaveTextContent("0 total")
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -101,6 +101,18 @@ describe("extractCommandPatterns", () => {
|
|||
const patterns = extractCommandPatterns("npm run build && git push")
|
||||
expect(patterns).toEqual([...patterns].sort())
|
||||
})
|
||||
|
||||
it("should handle numeric input like '0 total'", () => {
|
||||
const patterns = extractCommandPatterns("0 total")
|
||||
// Should return empty array since "0" is not a valid command
|
||||
expect(patterns).toEqual([])
|
||||
})
|
||||
|
||||
it("should handle pure numeric commands", () => {
|
||||
const patterns = extractCommandPatterns("0")
|
||||
// Should return empty array since pure numbers are not valid commands
|
||||
expect(patterns).toEqual([])
|
||||
})
|
||||
})
|
||||
|
||||
describe("getPatternDescription", () => {
|
||||
|
|
@ -415,3 +427,75 @@ describe("security integration with extractCommandPatterns", () => {
|
|||
expect(patterns).not.toContain("date")
|
||||
})
|
||||
})
|
||||
|
||||
describe("integration: parseCommandAndOutput with extractCommandPatterns", () => {
|
||||
it("should not extract patterns from output text", () => {
|
||||
const text = `wc -l *.go *.java
|
||||
Output:
|
||||
wc: *.go: open: No such file or directory
|
||||
wc: *.java: open: No such file or directory
|
||||
0 total`
|
||||
const { command } = parseCommandAndOutput(text)
|
||||
const patterns = extractCommandPatterns(command)
|
||||
|
||||
// Should only extract patterns from the command, not the output
|
||||
expect(patterns).toContain("wc")
|
||||
expect(patterns).not.toContain("0")
|
||||
expect(patterns).not.toContain("total")
|
||||
expect(patterns).not.toContain("0 total")
|
||||
})
|
||||
|
||||
it("should handle the specific wc command case", () => {
|
||||
const text = `wc -l *.go *.java
|
||||
Output:
|
||||
25 hello_world.go
|
||||
316 HelloWorld.java
|
||||
341 total`
|
||||
const { command } = parseCommandAndOutput(text)
|
||||
const patterns = extractCommandPatterns(command)
|
||||
|
||||
// Should only extract "wc" from the command
|
||||
expect(patterns).toEqual(["wc"])
|
||||
expect(patterns).not.toContain("341")
|
||||
expect(patterns).not.toContain("total")
|
||||
expect(patterns).not.toContain("341 total")
|
||||
})
|
||||
|
||||
it("should handle wc command with error output", () => {
|
||||
const text = `wc -l *.go *.java
|
||||
Output:
|
||||
wc: *.go: open: No such file or directory
|
||||
wc: *.java: open: No such file or directory
|
||||
0 total`
|
||||
const { command, output } = parseCommandAndOutput(text)
|
||||
const patterns = extractCommandPatterns(command)
|
||||
|
||||
// Should only extract "wc" from the command
|
||||
expect(command).toBe("wc -l *.go *.java")
|
||||
expect(output).toContain("0 total")
|
||||
expect(patterns).toEqual(["wc"])
|
||||
expect(patterns).not.toContain("0")
|
||||
expect(patterns).not.toContain("total")
|
||||
expect(patterns).not.toContain("0 total")
|
||||
})
|
||||
|
||||
it("should handle case where only output line is provided", () => {
|
||||
// This simulates if somehow only "0 total" is passed as the text
|
||||
const text = "0 total"
|
||||
const { command } = parseCommandAndOutput(text)
|
||||
const patterns = extractCommandPatterns(command)
|
||||
|
||||
// In this case, the entire text is treated as command
|
||||
expect(command).toBe("0 total")
|
||||
// But "0 total" is not a valid command pattern (starts with number)
|
||||
expect(patterns).toEqual([])
|
||||
})
|
||||
|
||||
it("should handle commands without output separator", () => {
|
||||
const text = "npm install"
|
||||
const { command } = parseCommandAndOutput(text)
|
||||
const patterns = extractCommandPatterns(command)
|
||||
|
||||
expect(patterns).toEqual(["npm", "npm install"])
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -39,7 +39,16 @@ export function extractCommandPatterns(command: string): string[] {
|
|||
} catch (_error) {
|
||||
// If parsing fails, try to extract at least the main command
|
||||
const mainCommand = command.trim().split(/\s+/)[0]
|
||||
if (mainCommand) patterns.add(mainCommand)
|
||||
|
||||
// Apply same validation as in processCommand
|
||||
if (
|
||||
mainCommand &&
|
||||
!/^\d+$/.test(mainCommand) && // Skip pure numbers
|
||||
!["total", "error", "warning", "failed", "success", "done"].includes(mainCommand.toLowerCase()) &&
|
||||
(/[a-zA-Z]/.test(mainCommand) || mainCommand.includes("/"))
|
||||
) {
|
||||
patterns.add(mainCommand)
|
||||
}
|
||||
}
|
||||
|
||||
return Array.from(patterns).sort()
|
||||
|
|
@ -49,7 +58,20 @@ function processCommand(cmd: any[], patterns: Set<string>) {
|
|||
if (!cmd.length || typeof cmd[0] !== "string") return
|
||||
|
||||
const mainCmd = cmd[0]
|
||||
patterns.add(mainCmd)
|
||||
|
||||
// Skip if it's just a number (like "0" from "0 total")
|
||||
if (/^\d+$/.test(mainCmd)) return
|
||||
|
||||
// Skip common output patterns that aren't commands
|
||||
const skipWords = ["total", "error", "warning", "failed", "success", "done"]
|
||||
if (skipWords.includes(mainCmd.toLowerCase())) return
|
||||
|
||||
// Only add if it contains at least one letter or is a valid path
|
||||
if (/[a-zA-Z]/.test(mainCmd) || mainCmd.includes("/")) {
|
||||
patterns.add(mainCmd)
|
||||
} else {
|
||||
return // Don't process further if main command is invalid
|
||||
}
|
||||
|
||||
// Patterns that indicate we should stop looking for subcommands
|
||||
const stopPatterns = [/^-/, /[\\/.~ ]/]
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue