From ebf1b241c17818dc18fc8075ed989e0ac31bf6da Mon Sep 17 00:00:00 2001 From: hannesrudolph Date: Tue, 22 Jul 2025 14:50:52 -0600 Subject: [PATCH] fix: improve command parsing to handle Output: separator correctly - Fixed parseCommandAndOutput to properly handle the newline + 'Output:' separator - Added test cases for commands with numbers at the start of output lines - Updated existing tests to use template literals for proper newline handling - Fixed test assertions to handle multiple code blocks when output is present This resolves the issue where output lines starting with numbers (like 'wc -l' output) were being incorrectly parsed as the command instead of the actual command text. --- .../chat/__tests__/CommandExecution.spec.tsx | 52 ++++++-- .../utils/__tests__/commandPatterns.spec.ts | 111 ++++++++++++++---- webview-ui/src/utils/commandPatterns.ts | 28 +++-- 3 files changed, 148 insertions(+), 43 deletions(-) diff --git a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx index 3312434ab2..9809726267 100644 --- a/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/CommandExecution.spec.tsx @@ -21,6 +21,19 @@ vi.mock("../../common/CodeBlock", () => ({ default: ({ source }: { source: string }) =>
{source}
, })) +// Mock the commandPatterns module but use the actual implementation +vi.mock("../../../utils/commandPatterns", async () => { + const actual = await vi.importActual( + "../../../utils/commandPatterns", + ) + return { + ...actual, + parseCommandAndOutput: actual.parseCommandAndOutput, + extractCommandPatterns: actual.extractCommandPatterns, + getPatternDescription: actual.getPatternDescription, + } +}) + vi.mock("../CommandPatternSelector", () => ({ CommandPatternSelector: ({ patterns, onAllowPatternChange, onDenyPatternChange }: any) => (
@@ -66,7 +79,7 @@ describe("CommandExecution", () => { it("should render command with output", () => { render( - + , ) @@ -166,29 +179,42 @@ describe("CommandExecution", () => { expect(vscode.postMessage).toHaveBeenCalledWith({ type: "deniedCommands", commands: [] }) }) - it("should parse command with $ prefix", () => { + it("should parse command with Output: separator", () => { + const commandText = `npm install +Output: +Installing...` + render( - + , ) - expect(screen.getByTestId("code-block")).toHaveTextContent("npm install") + const codeBlocks = screen.getAllByTestId("code-block") + expect(codeBlocks[0]).toHaveTextContent("npm install") }) it("should parse command with AI suggestions", () => { + const commandText = `npm install +Output: +Suggested patterns: npm, npm install, npm run` + render( - + , ) + // First check that the command was parsed correctly + const codeBlocks = screen.getAllByTestId("code-block") + expect(codeBlocks[0]).toHaveTextContent("npm install") + 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 expect(screen.getByText("npm")).toBeInTheDocument() + expect(screen.getAllByText("npm install").length).toBeGreaterThan(0) + expect(screen.getByText("npm run")).toBeInTheDocument() }) it("should handle commands with pipes", () => { @@ -232,14 +258,20 @@ describe("CommandExecution", () => { terminalShellIntegrationDisabled: true, } + const commandText = `npm install +Output: +Output here` + render( - + , ) // Output should be visible when shell integration is disabled - expect(screen.getByText(/Output here/)).toBeInTheDocument() + const codeBlocks = screen.getAllByTestId("code-block") + expect(codeBlocks).toHaveLength(2) // Command and output blocks + expect(codeBlocks[1]).toHaveTextContent("Output here") }) it("should handle undefined allowedCommands and deniedCommands", () => { diff --git a/webview-ui/src/utils/__tests__/commandPatterns.spec.ts b/webview-ui/src/utils/__tests__/commandPatterns.spec.ts index b3d5ffacdd..9d26531a4e 100644 --- a/webview-ui/src/utils/__tests__/commandPatterns.spec.ts +++ b/webview-ui/src/utils/__tests__/commandPatterns.spec.ts @@ -133,25 +133,28 @@ describe("getPatternDescription", () => { }) describe("parseCommandAndOutput", () => { - it("should parse command with $ prefix", () => { + it("should handle command with $ prefix without Output: separator", () => { const text = "$ npm install\nInstalling packages..." const result = parseCommandAndOutput(text) - expect(result.command).toBe("npm install") - expect(result.output).toBe("Installing packages...") + // Without Output: separator, the entire text is treated as command + expect(result.command).toBe("$ npm install\nInstalling packages...") + expect(result.output).toBe("") }) - it("should parse command with ❯ prefix", () => { + it("should handle command with ❯ prefix without Output: separator", () => { const text = "❯ git status\nOn branch main" const result = parseCommandAndOutput(text) - expect(result.command).toBe("git status") - expect(result.output).toBe("On branch main") + // Without Output: separator, the entire text is treated as command + expect(result.command).toBe("❯ git status\nOn branch main") + expect(result.output).toBe("") }) - it("should parse command with > prefix", () => { + it("should handle command with > prefix without Output: separator", () => { const text = "> echo hello\nhello" const result = parseCommandAndOutput(text) - expect(result.command).toBe("echo hello") - expect(result.output).toBe("hello") + // Without Output: separator, the entire text is treated as command + expect(result.command).toBe("> echo hello\nhello") + expect(result.output).toBe("") }) it("should return original text if no command prefix found", () => { @@ -161,43 +164,50 @@ describe("parseCommandAndOutput", () => { expect(result.output).toBe("") }) - it("should extract AI suggestions from output", () => { - const text = "$ npm install\nSuggested patterns: npm, npm install, npm run" + it("should extract AI suggestions from output with Output: separator", () => { + const text = "npm install\nOutput:\nSuggested patterns: npm, npm install, npm run" const result = parseCommandAndOutput(text) + expect(result.command).toBe("npm install") expect(result.suggestions).toEqual(["npm", "npm install", "npm run"]) }) it("should extract suggestions with different formats", () => { - const text = "$ git push\nCommand patterns: git, git push" + const text = "git push\nOutput:\nCommand patterns: git, git push" const result = parseCommandAndOutput(text) + expect(result.command).toBe("git push") expect(result.suggestions).toEqual(["git", "git push"]) }) it('should extract suggestions from "you can allow" format', () => { - const text = "$ docker run\nYou can allow: docker, docker run" + const text = "docker run\nOutput:\nYou can allow: docker, docker run" const result = parseCommandAndOutput(text) + expect(result.command).toBe("docker run") expect(result.suggestions).toEqual(["docker", "docker run"]) }) it("should extract suggestions from bullet points", () => { - const text = `$ npm test + const text = `npm test +Output: Output here... - npm - npm test - npm run` const result = parseCommandAndOutput(text) + expect(result.command).toBe("npm test") expect(result.suggestions).toContain("npm") expect(result.suggestions).toContain("npm test") expect(result.suggestions).toContain("npm run") }) it("should extract suggestions from various bullet formats", () => { - const text = `$ command + const text = `command +Output: • npm * git - docker ▪ python` const result = parseCommandAndOutput(text) + expect(result.command).toBe("command") expect(result.suggestions).toContain("npm") expect(result.suggestions).toContain("git") expect(result.suggestions).toContain("docker") @@ -205,8 +215,9 @@ Output here... }) it("should extract suggestions with backticks", () => { - const text = "$ npm install\n- `npm`\n- `npm install`" + const text = "npm install\nOutput:\n- `npm`\n- `npm install`" const result = parseCommandAndOutput(text) + expect(result.command).toBe("npm install") expect(result.suggestions).toContain("npm") expect(result.suggestions).toContain("npm install") }) @@ -218,25 +229,28 @@ Output here... expect(result.suggestions).toEqual([]) }) - it("should handle multiline commands", () => { + it("should handle multiline commands without Output: separator", () => { const text = `$ npm install \\ - express \\ - mongoose + express \\ + mongoose Installing...` const result = parseCommandAndOutput(text) - expect(result.command).toBe("npm install \\") - expect(result.output).toContain("express") + // Without Output: separator, entire text is treated as command + expect(result.command).toBe(text) + expect(result.output).toBe("") }) - it("should include all suggestions from comma-separated list", () => { - const text = "$ test\nSuggested patterns: npm, npm install, npm run" + it("should include all suggestions from comma-separated list with Output: separator", () => { + const text = "test\nOutput:\nSuggested patterns: npm, npm install, npm run" const result = parseCommandAndOutput(text) + expect(result.command).toBe("test") expect(result.suggestions).toEqual(["npm", "npm install", "npm run"]) }) it("should handle case variations in suggestion patterns", () => { - const text = "$ test\nSuggested Patterns: npm, git\nCommand Patterns: docker" + const text = "test\nOutput:\nSuggested Patterns: npm, git\nCommand Patterns: docker" const result = parseCommandAndOutput(text) + expect(result.command).toBe("test") // Now it should accumulate all suggestions expect(result.suggestions).toContain("npm") expect(result.suggestions).toContain("git") @@ -277,6 +291,55 @@ Installing...` expect(result.command).toBe('echo "test"') expect(result.output).toBe("First output\nOutput: Second output") }) + + it("should handle output with numbers at the start of lines", () => { + const text = `wc -l *.go *.java +Output: +25 hello_world.go +316 HelloWorld.java +341 total` + const result = parseCommandAndOutput(text) + expect(result.command).toBe("wc -l *.go *.java") + expect(result.output).toBe("25 hello_world.go\n316 HelloWorld.java\n341 total") + expect(result.suggestions).toEqual([]) + }) + + it("should handle edge case where text starts with Output:", () => { + const text = "Output:\nSome output without a command" + const result = parseCommandAndOutput(text) + expect(result.command).toBe("") + expect(result.output).toBe("Some output without a command") + }) + + it("should not be confused by Output: appearing in the middle of output", () => { + const text = `echo "Output: test" +Output: +Output: test` + const result = parseCommandAndOutput(text) + expect(result.command).toBe('echo "Output: test"') + expect(result.output).toBe("Output: test") + }) + + it("should handle commands without shell prompt when Output: separator is present", () => { + const text = `npm install +Output: +Installing packages...` + const result = parseCommandAndOutput(text) + expect(result.command).toBe("npm install") + expect(result.output).toBe("Installing packages...") + }) + + it("should not parse shell prompts from output when Output: separator exists", () => { + const text = `ls -la +Output: +$ total 341 +drwxr-xr-x 10 user staff 320 Jan 22 12:00 . +drwxr-xr-x 20 user staff 640 Jan 22 11:00 ..` + const result = parseCommandAndOutput(text) + expect(result.command).toBe("ls -la") + expect(result.output).toContain("$ total 341") + expect(result.output).toContain("drwxr-xr-x") + }) }) describe("detectSecurityIssues", () => { diff --git a/webview-ui/src/utils/commandPatterns.ts b/webview-ui/src/utils/commandPatterns.ts index 23421c1bf0..7fe97f074f 100644 --- a/webview-ui/src/utils/commandPatterns.ts +++ b/webview-ui/src/utils/commandPatterns.ts @@ -101,20 +101,30 @@ export function parseCommandAndOutput(text: string): { // First check if the text already has been split by COMMAND_OUTPUT_STRING // This happens when the command has already been executed and we have the output const outputSeparator = "Output:" - const outputIndex = text.indexOf(outputSeparator) + const outputIndex = text.indexOf(`\n${outputSeparator}`) if (outputIndex !== -1) { // Text is already split into command and output + // The command is everything before the output separator result.command = text.slice(0, outputIndex).trim() - result.output = text.slice(outputIndex + outputSeparator.length).trim() - } else { - // Try to extract command from the text - // Look for patterns like "$ command" or "❯ command" at the start - const commandMatch = text.match(/^[$❯>]\s*(.+?)(?:\n|$)/m) - if (commandMatch) { - result.command = commandMatch[1].trim() - result.output = text.substring(commandMatch.index! + commandMatch[0].length).trim() + // The output is everything after the output separator + // We need to skip the newline and "Output:" text + const afterNewline = outputIndex + 1 // Skip the newline + const afterSeparator = afterNewline + outputSeparator.length // Skip "Output:" + // Check if there's a colon and potential space after it + let startOfOutput = afterSeparator + if (text[afterSeparator] === "\n") { + startOfOutput = afterSeparator + 1 // Skip additional newline after "Output:" } + result.output = text.slice(startOfOutput).trim() + } else if (text.indexOf(outputSeparator) === 0) { + // Edge case: text starts with "Output:" (no command) + result.command = "" + result.output = text.slice(outputSeparator.length).trim() + } else { + // No output separator found, the entire text is the command + result.command = text.trim() + result.output = "" } // Look for AI suggestions in the output