feat: refactor bash tool with shell-aware prompts for bash, pwsh+powershell, and cmd (#20039)

This commit is contained in:
Luke Parker
2026-05-03 09:18:48 +10:00
committed by GitHub
parent 1986a6e817
commit 3f459819ba
14 changed files with 506 additions and 177 deletions

View File

@@ -620,7 +620,7 @@ describe("session.message-v2.toModelMessage", () => {
status: "completed",
input: { cmd: "ls" },
output: "abcdefghij",
title: "Bash",
title: "Shell",
metadata: {},
time: { start: 0, end: 1 },
},
@@ -740,9 +740,9 @@ describe("session.message-v2.toModelMessage", () => {
"12179",
"4575",
"",
"<bash_metadata>",
"<shell_metadata>",
"User aborted the command",
"</bash_metadata>",
"</shell_metadata>",
].join("\n")
const input: MessageV2.WithParts[] = [

View File

@@ -10,7 +10,6 @@ import { toJsonSchema } from "../../src/util/effect-zod"
// byte-identical regardless of whether a tool has migrated from zod to Schema.
import { Parameters as ApplyPatch } from "../../src/tool/apply_patch"
import { Parameters as Bash } from "../../src/tool/bash"
import { Parameters as Edit } from "../../src/tool/edit"
import { Parameters as Glob } from "../../src/tool/glob"
import { Parameters as Grep } from "../../src/tool/grep"
@@ -19,6 +18,7 @@ import { Parameters as Lsp } from "../../src/tool/lsp"
import { Parameters as Plan } from "../../src/tool/plan"
import { Parameters as Question } from "../../src/tool/question"
import { Parameters as Read } from "../../src/tool/read"
import { Parameters as Shell } from "../../src/tool/shell"
import { Parameters as Skill } from "../../src/tool/skill"
import { Parameters as Task } from "../../src/tool/task"
import { Parameters as Todo } from "../../src/tool/todo"
@@ -35,7 +35,7 @@ const accepts = (schema: Schema.Decoder<unknown>, input: unknown): boolean =>
describe("tool parameters", () => {
describe("JSON Schema (wire shape)", () => {
test("apply_patch", () => expect(toJsonSchema(ApplyPatch)).toMatchSnapshot())
test("bash", () => expect(toJsonSchema(Bash)).toMatchSnapshot())
test("bash", () => expect(toJsonSchema(Shell)).toMatchSnapshot())
test("edit", () => expect(toJsonSchema(Edit)).toMatchSnapshot())
test("glob", () => expect(toJsonSchema(Glob)).toMatchSnapshot())
test("grep", () => expect(toJsonSchema(Grep)).toMatchSnapshot())
@@ -66,20 +66,20 @@ describe("tool parameters", () => {
})
})
describe("bash", () => {
describe("shell", () => {
test("accepts minimum: command + description", () => {
expect(parse(Bash, { command: "ls", description: "list" })).toEqual({ command: "ls", description: "list" })
expect(parse(Shell, { command: "ls", description: "list" })).toEqual({ command: "ls", description: "list" })
})
test("accepts optional timeout + workdir", () => {
const parsed = parse(Bash, { command: "ls", description: "list", timeout: 5000, workdir: "/tmp" })
const parsed = parse(Shell, { command: "ls", description: "list", timeout: 5000, workdir: "/tmp" })
expect(parsed.timeout).toBe(5000)
expect(parsed.workdir).toBe("/tmp")
})
test("rejects missing description (required by zod)", () => {
expect(accepts(Bash, { command: "ls" })).toBe(false)
test("rejects missing description", () => {
expect(accepts(Shell, { command: "ls" })).toBe(false)
})
test("rejects missing command", () => {
expect(accepts(Bash, { description: "list" })).toBe(false)
expect(accepts(Shell, { description: "list" })).toBe(false)
})
})

View File

@@ -4,7 +4,7 @@ import os from "os"
import path from "path"
import { Config } from "@/config/config"
import { Shell } from "../../src/shell/shell"
import { BashTool } from "../../src/tool/bash"
import { ShellTool } from "../../src/tool/shell"
import { Instance } from "../../src/project/instance"
import { Filesystem } from "@/util/filesystem"
import { tmpdir } from "../fixture/fixture"
@@ -28,9 +28,11 @@ const runtime = ManagedRuntime.make(
)
function initBash() {
return runtime.runPromise(BashTool.pipe(Effect.flatMap((info) => info.init())))
return runtime.runPromise(ShellTool.pipe(Effect.flatMap((info) => info.init())))
}
const initShell = initBash
const ctx = {
sessionID: SessionID.make("ses_test"),
messageID: MessageID.make(""),
@@ -68,6 +70,7 @@ const shells = (() => {
})()
const PS = new Set(["pwsh", "powershell"])
const ps = shells.filter((item) => PS.has(item.label))
const cmdShell = shells.find((item) => item.label === "cmd")
const sh = () => Shell.name(Shell.acceptable())
const evalarg = (text: string) => (sh() === "cmd" ? quote(text) : squote(text))
@@ -135,12 +138,12 @@ const mustTruncate = (result: {
)
}
describe("tool.bash", () => {
describe("tool.shell", () => {
each("basic", async () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const result = await Effect.runPromise(
bash.execute(
{
@@ -184,13 +187,13 @@ describe("tool.bash", () => {
})
})
describe("tool.bash permissions", () => {
describe("tool.shell permissions", () => {
each("asks for bash permission with correct pattern", async () => {
await using tmp = await tmpdir()
await Instance.provide({
directory: tmp.path,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await Effect.runPromise(
bash.execute(
@@ -213,7 +216,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: tmp.path,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await Effect.runPromise(
bash.execute(
@@ -239,7 +242,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await Effect.runPromise(
bash.execute(
@@ -261,11 +264,43 @@ describe("tool.bash permissions", () => {
)
}
for (const item of ps) {
test(
`uses PowerShell cmdlet prefixes for always-allow prompts [${item.label}]`,
withShell(item, async () => {
await using tmp = await tmpdir()
await Instance.provide({
directory: tmp.path,
fn: async () => {
const bash = await initShell()
const err = new Error("stop after permission")
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await expect(
Effect.runPromise(
bash.execute(
{
command: "Remove-Item -Recurse tmp",
description: "Remove a temp directory",
},
capture(requests, err),
),
),
).rejects.toThrow(err.message)
const bashReq = requests.find((r) => r.permission === "bash")
expect(bashReq).toBeDefined()
expect(bashReq!.always).toContain("Remove-Item *")
expect(bashReq!.always).not.toContain("Remove-Item -Recurse *")
},
})
}),
)
}
each("asks for external_directory permission for wildcard external paths", async () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const err = new Error("stop after permission")
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
const file = process.platform === "win32" ? `${process.env.WINDIR!.replaceAll("\\", "/")}/*` : "/etc/*"
@@ -301,7 +336,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const file = path.join(outerTmp.path, "outside.txt").replaceAll("\\", "/")
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await Effect.runPromise(
@@ -334,7 +369,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const err = new Error("stop after permission")
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await expect(
@@ -364,7 +399,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
const file = `${process.env.WINDIR!.replaceAll("\\", "/")}/win.ini`
await Effect.runPromise(
@@ -396,7 +431,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: tmp.path,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const err = new Error("stop after permission")
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await expect(
@@ -426,7 +461,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const err = new Error("stop after permission")
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await expect(
@@ -521,7 +556,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const err = new Error("stop after permission")
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
const root = path.parse(process.env.WINDIR!).root.replace(/[\\/]+$/, "")
@@ -680,7 +715,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await Effect.runPromise(
bash.execute(
@@ -702,6 +737,35 @@ describe("tool.bash permissions", () => {
}
}
if (process.platform === "win32" && cmdShell) {
test(
"asks for external_directory permission for cmd file commands [cmd]",
withShell(cmdShell, async () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initShell()
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await Effect.runPromise(
bash.execute(
{
command: `TYPE "${path.join(process.env.WINDIR!, "win.ini")}"`,
description: "Read Windows ini with cmd",
},
capture(requests),
),
)
const extDirReq = requests.find((r) => r.permission === "external_directory")
expect(extDirReq).toBeDefined()
expect(extDirReq!.patterns).toContain(
Filesystem.normalizePathPattern(path.join(process.env.WINDIR!, "*")),
)
},
})
}),
)
}
each("asks for external_directory permission when cd to parent", async () => {
await using tmp = await tmpdir()
await Instance.provide({
@@ -945,7 +1009,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: tmp.path,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await Effect.runPromise(
bash.execute(
@@ -967,7 +1031,7 @@ describe("tool.bash permissions", () => {
await Instance.provide({
directory: tmp.path,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const err = new Error("stop after permission")
const requests: Array<Omit<Permission.Request, "id" | "sessionID" | "tool">> = []
await expect(
@@ -1001,12 +1065,12 @@ describe("tool.bash permissions", () => {
})
})
describe("tool.bash abort", () => {
describe("tool.shell abort", () => {
test("preserves output when aborted", async () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const controller = new AbortController()
const collected: string[] = []
const res = await Effect.runPromise(
@@ -1040,7 +1104,7 @@ describe("tool.bash abort", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const result = await Effect.runPromise(
bash.execute(
{
@@ -1052,7 +1116,7 @@ describe("tool.bash abort", () => {
),
)
expect(result.output).toContain("started")
expect(result.output).toContain("bash tool terminated command after exceeding timeout")
expect(result.output).toContain("shell tool terminated command after exceeding timeout")
expect(result.output).toContain("retry with a larger timeout value in milliseconds")
},
})
@@ -1062,7 +1126,7 @@ describe("tool.bash abort", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const result = await Effect.runPromise(
bash.execute(
{
@@ -1083,7 +1147,7 @@ describe("tool.bash abort", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const result = await Effect.runPromise(
bash.execute(
{
@@ -1128,12 +1192,12 @@ describe("tool.bash abort", () => {
})
})
describe("tool.bash truncation", () => {
describe("tool.shell truncation", () => {
test("truncates output exceeding line limit", async () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const lineCount = Truncate.MAX_LINES + 500
const result = await Effect.runPromise(
bash.execute(
@@ -1155,7 +1219,7 @@ describe("tool.bash truncation", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const byteCount = Truncate.MAX_BYTES + 10000
const result = await Effect.runPromise(
bash.execute(
@@ -1177,7 +1241,7 @@ describe("tool.bash truncation", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const result = await Effect.runPromise(
bash.execute(
{
@@ -1197,7 +1261,7 @@ describe("tool.bash truncation", () => {
await Instance.provide({
directory: projectRoot,
fn: async () => {
const bash = await initBash()
const bash = await initShell()
const lineCount = Truncate.MAX_LINES + 100
const result = await Effect.runPromise(
bash.execute(