diff --git a/packages/opencode/src/shell/shell.ts b/packages/opencode/src/shell/shell.ts index e7b7cdb3e4d..b0c66e81f63 100644 --- a/packages/opencode/src/shell/shell.ts +++ b/packages/opencode/src/shell/shell.ts @@ -36,6 +36,19 @@ export namespace Shell { } const BLACKLIST = new Set(["fish", "nu"]) + function full(file: string) { + if (process.platform !== "win32") return file + if (path.win32.dirname(file) !== ".") return file + return Bun.which(file) || file + } + + function pick() { + const pwsh = Bun.which("pwsh") + if (pwsh) return pwsh + const powershell = Bun.which("powershell") + if (powershell) return powershell + } + function fallback() { if (process.platform === "win32") { if (Flag.OPENCODE_GIT_BASH_PATH) return Flag.OPENCODE_GIT_BASH_PATH @@ -56,13 +69,23 @@ export namespace Shell { export const preferred = lazy(() => { const s = process.env.SHELL - if (s) return s + if (s) return full(s) + if (process.platform === "win32") { + const shell = pick() + if (shell) return shell + } return fallback() }) export const acceptable = lazy(() => { const s = process.env.SHELL - if (s && !BLACKLIST.has(process.platform === "win32" ? path.win32.basename(s) : path.basename(s))) return s + if (s && !BLACKLIST.has(process.platform === "win32" ? path.win32.basename(s, ".exe") : path.basename(s))) { + return full(s) + } + if (process.platform === "win32") { + const shell = pick() + if (shell) return shell + } return fallback() }) } diff --git a/packages/opencode/src/tool/bash.ts b/packages/opencode/src/tool/bash.ts index 0751f789b7d..902661f8d44 100644 --- a/packages/opencode/src/tool/bash.ts +++ b/packages/opencode/src/tool/bash.ts @@ -54,10 +54,18 @@ const parser = lazy(async () => { // TODO: we may wanna rename this tool so it works better on other shells export const BashTool = Tool.define("bash", async () => { const shell = Shell.acceptable() + const name = process.platform === "win32" ? path.win32.basename(shell, ".exe") : path.basename(shell) + const chain = + name.toLowerCase() === "powershell" + ? "If the commands depend on each other and must run sequentially, avoid '&&' in this shell because Windows PowerShell 5.1 does not support it. Use PowerShell conditionals such as `cmd1; if ($?) { cmd2 }` when later commands must depend on earlier success." + : "If the commands depend on each other and must run sequentially, use a single Bash call with '&&' to chain them together (e.g., `git add . && git commit -m \"message\" && git push`). For instance, if one operation must complete before another starts (like mkdir before cp, Write before Bash for git operations, or git add before git commit), run these operations sequentially instead." log.info("bash tool using shell", { shell }) return { description: DESCRIPTION.replaceAll("${directory}", Instance.directory) + .replaceAll("${os}", process.platform) + .replaceAll("${shell}", name) + .replaceAll("${chaining}", chain) .replaceAll("${maxLines}", String(Truncate.MAX_LINES)) .replaceAll("${maxBytes}", String(Truncate.MAX_BYTES)), parameters: z.object({ @@ -169,16 +177,25 @@ export const BashTool = Tool.define("bash", async () => { { cwd, sessionID: ctx.sessionID, callID: ctx.callID }, { env: {} }, ) - const proc = spawn(params.command, { - shell, - cwd, - env: { - ...process.env, - ...shellEnv.env, - }, - stdio: ["ignore", "pipe", "pipe"], - detached: process.platform !== "win32", - }) + const env = { + ...process.env, + ...shellEnv.env, + } + const proc = + process.platform === "win32" && ["pwsh", "powershell"].includes(name.toLowerCase()) + ? spawn(shell, ["-NoLogo", "-NoProfile", "-NonInteractive", "-Command", params.command], { + cwd, + env, + stdio: ["ignore", "pipe", "pipe"], + detached: process.platform !== "win32", + }) + : spawn(params.command, { + shell, + cwd, + env, + stdio: ["ignore", "pipe", "pipe"], + detached: process.platform !== "win32", + }) let output = "" @@ -235,6 +252,10 @@ export const BashTool = Tool.define("bash", async () => { proc.once("exit", () => { exited = true + }) + + proc.once("close", () => { + exited = true cleanup() resolve() }) diff --git a/packages/opencode/src/tool/bash.txt b/packages/opencode/src/tool/bash.txt index baafb00810a..8d53c90ab4e 100644 --- a/packages/opencode/src/tool/bash.txt +++ b/packages/opencode/src/tool/bash.txt @@ -1,5 +1,7 @@ Executes a given bash command in a persistent shell session with optional timeout, ensuring proper handling and security measures. +Be aware: OS: ${os}, Shell: ${shell} + All commands run in ${directory} by default. Use the `workdir` parameter if you need to run a command in a different directory. AVOID using `cd && ` patterns - use `workdir` instead. IMPORTANT: This tool is for terminal operations like git, npm, docker, etc. DO NOT use it for file operations (reading, writing, editing, searching, finding files) - use the specialized tools for this instead. @@ -35,7 +37,7 @@ Usage notes: - Communication: Output text directly (NOT echo/printf) - When issuing multiple commands: - If the commands are independent and can run in parallel, make multiple Bash tool calls in a single message. For example, if you need to run "git status" and "git diff", send a single message with two Bash tool calls in parallel. - - If the commands depend on each other and must run sequentially, use a single Bash call with '&&' to chain them together (e.g., `git add . && git commit -m "message" && git push`). For instance, if one operation must complete before another starts (like mkdir before cp, Write before Bash for git operations, or git add before git commit), run these operations sequentially instead. + - ${chaining} - Use ';' only when you need to run commands sequentially but don't care if earlier commands fail - DO NOT use newlines to separate commands (newlines are ok in quoted strings) - AVOID using `cd && `. Use the `workdir` parameter to change directories instead. diff --git a/packages/opencode/test/tool/bash.test.ts b/packages/opencode/test/tool/bash.test.ts index ac93016927a..62e7048d128 100644 --- a/packages/opencode/test/tool/bash.test.ts +++ b/packages/opencode/test/tool/bash.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test" import os from "os" import path from "path" +import { Shell } from "../../src/shell/shell" import { BashTool } from "../../src/tool/bash" import { Instance } from "../../src/project/instance" import { Filesystem } from "../../src/util/filesystem" @@ -20,9 +21,55 @@ const ctx = { } const projectRoot = path.join(__dirname, "../..") +const bin = process.execPath.replaceAll("\\", "/") +const file = path.join(projectRoot, "test/tool/fixtures/output.ts").replaceAll("\\", "/") +const kind = () => path.win32.basename(process.env.SHELL || "", ".exe").toLowerCase() +const fill = (mode: "lines" | "bytes", n: number) => { + if (["pwsh", "powershell"].includes(kind())) { + if (mode === "lines") return `1..${n} | ForEach-Object { $_ }` + return `Write-Output ('a' * ${n})` + } + return `${bin} ${file} ${mode} ${n}` +} +const shells = (() => { + if (process.platform !== "win32") { + const shell = process.env.SHELL || Bun.which("bash") || "/bin/sh" + return [{ label: path.basename(shell), shell }] + } + + const list = [ + { label: "git bash", shell: process.env.SHELL || Bun.which("bash") }, + { label: "pwsh", shell: Bun.which("pwsh") }, + { label: "powershell", shell: Bun.which("powershell") }, + { label: "cmd", shell: process.env.COMSPEC || Bun.which("cmd.exe") }, + ].filter((item): item is { label: string; shell: string } => Boolean(item.shell)) + + return list.filter((item, i) => list.findIndex((x) => x.shell.toLowerCase() === item.shell.toLowerCase()) === i) +})() + +const withShell = (shell: string, fn: () => Promise) => async () => { + const prev = process.env.SHELL + process.env.SHELL = shell + Shell.acceptable.reset() + Shell.preferred.reset() + try { + await fn() + } finally { + if (prev === undefined) delete process.env.SHELL + else process.env.SHELL = prev + Shell.acceptable.reset() + Shell.preferred.reset() + } +} + +const each = (name: string, fn: () => Promise) => { + for (const item of shells) { + test(`${name} [${item.label}]`, withShell(item.shell, fn)) + } +} describe("tool.bash", () => { - test("basic", async () => { + each("basic", async () => { await Instance.provide({ directory: projectRoot, fn: async () => { @@ -42,7 +89,7 @@ describe("tool.bash", () => { }) describe("tool.bash permissions", () => { - test("asks for bash permission with correct pattern", async () => { + each("asks for bash permission with correct pattern", async () => { await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, @@ -69,7 +116,7 @@ describe("tool.bash permissions", () => { }) }) - test("asks for bash permission with multiple commands", async () => { + each("asks for bash permission with multiple commands", async () => { await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, @@ -97,7 +144,7 @@ describe("tool.bash permissions", () => { }) }) - test("asks for external_directory permission when cd to parent", async () => { + each("asks for external_directory permission when cd to parent", async () => { await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, @@ -123,7 +170,7 @@ describe("tool.bash permissions", () => { }) }) - test("asks for external_directory permission when workdir is outside project", async () => { + each("asks for external_directory permission when workdir is outside project", async () => { await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, @@ -151,7 +198,7 @@ describe("tool.bash permissions", () => { }) }) - test("asks for external_directory permission when file arg is outside project", async () => { + each("asks for external_directory permission when file arg is outside project", async () => { await using outerTmp = await tmpdir({ init: async (dir) => { await Bun.write(path.join(dir, "outside.txt"), "x") @@ -186,7 +233,7 @@ describe("tool.bash permissions", () => { }) }) - test("does not ask for external_directory permission when rm inside project", async () => { + each("does not ask for external_directory permission when rm inside project", async () => { await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, @@ -216,7 +263,7 @@ describe("tool.bash permissions", () => { }) }) - test("includes always patterns for auto-approval", async () => { + each("includes always patterns for auto-approval", async () => { await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, @@ -243,7 +290,7 @@ describe("tool.bash permissions", () => { }) }) - test("does not ask for bash permission when command is cd only", async () => { + each("does not ask for bash permission when command is cd only", async () => { await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, @@ -269,7 +316,7 @@ describe("tool.bash permissions", () => { }) }) - test("matches redirects in permission pattern", async () => { + each("matches redirects in permission pattern", async () => { await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, @@ -282,15 +329,18 @@ describe("tool.bash permissions", () => { requests.push(req) }, } - await bash.execute({ command: "cat > /tmp/output.txt", description: "Redirect ls output" }, testCtx) + const command = ["pwsh", "powershell"].includes(kind()) + ? "Write-Output test > output.txt" + : "cat > /tmp/output.txt" + await bash.execute({ command, description: "Redirect ls output" }, testCtx) const bashReq = requests.find((r) => r.permission === "bash") expect(bashReq).toBeDefined() - expect(bashReq!.patterns).toContain("cat > /tmp/output.txt") + expect(bashReq!.patterns).toContain(command) }, }) }) - test("always pattern has space before wildcard to not include different commands", async () => { + each("always pattern has space before wildcard to not include different commands", async () => { await using tmp = await tmpdir({ git: true }) await Instance.provide({ directory: tmp.path, @@ -314,7 +364,7 @@ describe("tool.bash permissions", () => { }) describe("tool.bash truncation", () => { - test("truncates output exceeding line limit", async () => { + each("truncates output exceeding line limit", async () => { await Instance.provide({ directory: projectRoot, fn: async () => { @@ -322,7 +372,7 @@ describe("tool.bash truncation", () => { const lineCount = Truncate.MAX_LINES + 500 const result = await bash.execute( { - command: `seq 1 ${lineCount}`, + command: fill("lines", lineCount), description: "Generate lines exceeding limit", }, ctx, @@ -334,7 +384,7 @@ describe("tool.bash truncation", () => { }) }) - test("truncates output exceeding byte limit", async () => { + each("truncates output exceeding byte limit", async () => { await Instance.provide({ directory: projectRoot, fn: async () => { @@ -342,7 +392,7 @@ describe("tool.bash truncation", () => { const byteCount = Truncate.MAX_BYTES + 10000 const result = await bash.execute( { - command: `head -c ${byteCount} /dev/zero | tr '\\0' 'a'`, + command: fill("bytes", byteCount), description: "Generate bytes exceeding limit", }, ctx, @@ -354,7 +404,7 @@ describe("tool.bash truncation", () => { }) }) - test("does not truncate small output", async () => { + each("does not truncate small output", async () => { await Instance.provide({ directory: projectRoot, fn: async () => { @@ -367,13 +417,12 @@ describe("tool.bash truncation", () => { ctx, ) expect((result.metadata as any).truncated).toBe(false) - const eol = process.platform === "win32" ? "\r\n" : "\n" - expect(result.output).toBe(`hello${eol}`) + expect(result.output).toContain("hello") }, }) }) - test("full output is saved to file when truncated", async () => { + each("full output is saved to file when truncated", async () => { await Instance.provide({ directory: projectRoot, fn: async () => { @@ -381,7 +430,7 @@ describe("tool.bash truncation", () => { const lineCount = Truncate.MAX_LINES + 100 const result = await bash.execute( { - command: `seq 1 ${lineCount}`, + command: fill("lines", lineCount), description: "Generate lines for file check", }, ctx, @@ -392,7 +441,7 @@ describe("tool.bash truncation", () => { expect(filepath).toBeTruthy() const saved = await Filesystem.readText(filepath) - const lines = saved.trim().split("\n") + const lines = saved.trim().split(/\r?\n/) expect(lines.length).toBe(lineCount) expect(lines[0]).toBe("1") expect(lines[lineCount - 1]).toBe(String(lineCount)) diff --git a/packages/opencode/test/tool/fixtures/output.ts b/packages/opencode/test/tool/fixtures/output.ts new file mode 100644 index 00000000000..5d5d0dd1aab --- /dev/null +++ b/packages/opencode/test/tool/fixtures/output.ts @@ -0,0 +1,14 @@ +const mode = Bun.argv[2] +const n = Number(Bun.argv[3]) + +if (mode === "lines") { + console.log(Array.from({ length: n }, (_, i) => i + 1).join("\n")) + process.exit(0) +} + +if (mode === "bytes") { + process.stdout.write("a".repeat(n)) + process.exit(0) +} + +throw new Error(`unknown mode: ${mode}`)