From 8dd11256816b91b0cb2e4caf1009f129e7c73a0f Mon Sep 17 00:00:00 2001 From: Brayden Moon Date: Mon, 21 Apr 2025 14:11:19 +1000 Subject: [PATCH] fix: command pipe execution by improving shell detection (#437) ## Description This PR fixes Issue #421 where commands with pipes (e.g., `grep -R ... -n | head -n 20`) were failing to execute properly after PR #391 was merged. ## Changes - Modified the `requiresShell` function to only enable shell mode when the command is a single string containing shell operators - Added logic to handle the case where shell operators are passed as separate arguments - Added comprehensive tests to verify the fix ## Root Cause The issue was that the `requiresShell` function was detecting shell operators like `|` even when they were passed as separate arguments, which caused the command to be executed with `shell: true` unnecessarily. This was causing syntax errors when running commands with pipes. ## Testing - Added unit tests to verify the fix - Manually tested with real commands using pipes - Ensured all existing tests pass Fixes #421 --- codex-cli/src/utils/agent/exec.ts | 12 +++-- codex-cli/tests/fixed-requires-shell.test.ts | 38 +++++++++++++++ codex-cli/tests/pipe-command.test.ts | 19 ++++++++ codex-cli/tests/requires-shell.test.ts | 49 ++++++++++++++++++++ 4 files changed, 115 insertions(+), 3 deletions(-) create mode 100644 codex-cli/tests/fixed-requires-shell.test.ts create mode 100644 codex-cli/tests/pipe-command.test.ts create mode 100644 codex-cli/tests/requires-shell.test.ts diff --git a/codex-cli/src/utils/agent/exec.ts b/codex-cli/src/utils/agent/exec.ts index 876e144a..25c6f86a 100644 --- a/codex-cli/src/utils/agent/exec.ts +++ b/codex-cli/src/utils/agent/exec.ts @@ -14,10 +14,16 @@ import { parse } from "shell-quote"; const DEFAULT_TIMEOUT_MS = 10_000; // 10 seconds function requiresShell(cmd: Array): boolean { - return cmd.some((arg) => { - const tokens = parse(arg) as Array; + // If the command is a single string that contains shell operators, + // it needs to be run with shell: true + if (cmd.length === 1 && cmd[0] !== undefined) { + const tokens = parse(cmd[0]) as Array; return tokens.some((token) => typeof token === "object" && "op" in token); - }); + } + + // If the command is split into multiple arguments, we don't need shell: true + // even if one of the arguments is a shell operator like '|' + return false; } /** diff --git a/codex-cli/tests/fixed-requires-shell.test.ts b/codex-cli/tests/fixed-requires-shell.test.ts new file mode 100644 index 00000000..dd1058db --- /dev/null +++ b/codex-cli/tests/fixed-requires-shell.test.ts @@ -0,0 +1,38 @@ +import { describe, it, expect } from "vitest"; +import { parse } from "shell-quote"; + +// The fixed requiresShell function +function requiresShell(cmd: Array): boolean { + // If the command is a single string that contains shell operators, + // it needs to be run with shell: true + if (cmd.length === 1 && cmd[0] !== undefined) { + const tokens = parse(cmd[0]) as Array; + return tokens.some((token) => typeof token === "object" && "op" in token); + } + + // If the command is split into multiple arguments, we don't need shell: true + // even if one of the arguments is a shell operator like '|' + return false; +} + +describe("fixed requiresShell function", () => { + it("should detect pipe in a single argument", () => { + const cmd = ['grep -n "finally:" some-file | head']; + expect(requiresShell(cmd)).toBe(true); + }); + + it("should not detect pipe in separate arguments", () => { + const cmd = ["grep", "-n", "finally:", "some-file", "|", "head"]; + expect(requiresShell(cmd)).toBe(false); + }); + + it("should handle other shell operators in a single argument", () => { + const cmd = ["echo hello && echo world"]; + expect(requiresShell(cmd)).toBe(true); + }); + + it("should not enable shell for normal commands", () => { + const cmd = ["ls", "-la"]; + expect(requiresShell(cmd)).toBe(false); + }); +}); diff --git a/codex-cli/tests/pipe-command.test.ts b/codex-cli/tests/pipe-command.test.ts new file mode 100644 index 00000000..652fee99 --- /dev/null +++ b/codex-cli/tests/pipe-command.test.ts @@ -0,0 +1,19 @@ +import { describe, it, expect } from "vitest"; +import { parse } from "shell-quote"; + +/* eslint-disable no-console */ + +describe("shell-quote parse with pipes", () => { + it("should correctly parse a command with a pipe", () => { + const cmd = 'grep -n "finally:" some-file | head'; + const tokens = parse(cmd); + console.log("Parsed tokens:", JSON.stringify(tokens, null, 2)); + + // Check if any token has an 'op' property + const hasOpToken = tokens.some( + (token) => typeof token === "object" && "op" in token, + ); + + expect(hasOpToken).toBe(true); + }); +}); diff --git a/codex-cli/tests/requires-shell.test.ts b/codex-cli/tests/requires-shell.test.ts new file mode 100644 index 00000000..5264490e --- /dev/null +++ b/codex-cli/tests/requires-shell.test.ts @@ -0,0 +1,49 @@ +import { describe, it, expect } from "vitest"; +import { parse } from "shell-quote"; + +/* eslint-disable no-console */ + +// Recreate the requiresShell function for testing +function requiresShell(cmd: Array): boolean { + // If the command is a single string that contains shell operators, + // it needs to be run with shell: true + if (cmd.length === 1 && cmd[0] !== undefined) { + const tokens = parse(cmd[0]) as Array; + console.log( + `Parsing argument: "${cmd[0]}", tokens:`, + JSON.stringify(tokens, null, 2), + ); + return tokens.some((token) => typeof token === "object" && "op" in token); + } + + // If the command is split into multiple arguments, we don't need shell: true + // even if one of the arguments is a shell operator like '|' + cmd.forEach((arg) => { + const tokens = parse(arg) as Array; + console.log( + `Parsing argument: "${arg}", tokens:`, + JSON.stringify(tokens, null, 2), + ); + }); + console.log("Result for separate arguments: false"); + return false; +} + +describe("requiresShell function", () => { + it("should detect pipe in a single argument", () => { + const cmd = ['grep -n "finally:" some-file | head']; + expect(requiresShell(cmd)).toBe(true); + }); + + it("should not detect pipe in separate arguments", () => { + const cmd = ["grep", "-n", "finally:", "some-file", "|", "head"]; + const result = requiresShell(cmd); + console.log("Result for separate arguments:", result); + expect(result).toBe(false); + }); + + it("should handle other shell operators", () => { + const cmd = ["echo hello && echo world"]; + expect(requiresShell(cmd)).toBe(true); + }); +});