-
Notifications
You must be signed in to change notification settings - Fork 137
fix(tui): rename internal bash tool to terminal for cross-platform accuracy #1077
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,8 +81,8 @@ describe("ConfigAgentPlugin.Plugin", () => { | |
| { action: "read", resource: "*", effect: "allow" }, | ||
| { action: "bash", resource: "git *", effect: "allow" }, | ||
| ]) | ||
| expect(PermissionV2.evaluate("bash", "git status", buildAgent.permissions).effect).toBe("allow") | ||
| expect(PermissionV2.evaluate("bash", "bun test", buildAgent.permissions).effect).toBe("ask") | ||
| expect(PermissionV2.evaluate("terminal", "git status", buildAgent.permissions).effect).toBe("allow") | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: The renamed assertion calls evaluate("terminal", ...) but the permission rules under test still carry action "bash", which PermissionV2.evaluate matches by exact wildcard with no bash→terminal remap. "git status" therefore never matches the "bash" allow rule and the first assertion expecting "allow" will fail with "ask". Update the permission rules (and the expected buildAgent.permissions/toMatchObject lists) to use action "terminal", or keep evaluating with "bash" — the test is currently internally inconsistent. Prompt for AI agents |
||
| expect(PermissionV2.evaluate("terminal", "bun test", buildAgent.permissions).effect).toBe("ask") | ||
|
|
||
| const reviewer = yield* agents.get(AgentV2.ID.make("reviewer")) | ||
| if (!reviewer) throw new Error("expected configured reviewer agent") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,8 +66,7 @@ describe("ToolRegistry", () => { | |
| const names = (rules: Parameters<ToolRegistry.Interface["materialize"]>[0]) => | ||
| toolDefinitions(service, rules).pipe(Effect.map((definitions) => definitions.map((tool) => tool.name))) | ||
|
|
||
| expect(yield* names([{ action: "question", resource: "*", effect: "deny" }])).toEqual([ | ||
| "bash", | ||
| expect(yield* names([{ action: "question", resource: "*", effect: "deny" }])).toEqual(["terminal", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: The test registers tools via Prompt for AI agents |
||
| "edit", | ||
| "write", | ||
| "apply_patch", | ||
|
|
@@ -84,7 +83,7 @@ describe("ToolRegistry", () => { | |
| { action: "*", resource: "*", effect: "deny" }, | ||
| ]), | ||
| ).toEqual([]) | ||
| expect(yield* names([{ action: "edit", resource: "*", effect: "deny" }])).toEqual(["question", "bash"]) | ||
| expect(yield* names([{ action: "edit", resource: "*", effect: "deny" }])).toEqual(["question", "terminal"]) | ||
| }), | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ import { PermissionV2 } from "@opencode-ai/core/permission" | |
| import { AppProcess } from "@opencode-ai/core/process" | ||
| import { AbsolutePath } from "@opencode-ai/core/schema" | ||
| import { SessionV2 } from "@opencode-ai/core/session" | ||
| import { BashTool } from "@opencode-ai/core/tool/bash" | ||
| import { TerminalTool } from "@opencode-ai/core/tool/terminal" | ||
| import { ToolRegistry } from "@opencode-ai/core/tool/registry" | ||
| import { location } from "./fixture/location" | ||
| import { tmpdir } from "./fixture/tmpdir" | ||
|
|
@@ -102,7 +102,7 @@ const withTool = <A, E, R>( | |
| ) | ||
| const mutation = LocationMutation.layer.pipe(Layer.provide(filesystem), Layer.provide(activeLocation)) | ||
| const registry = ToolRegistry.defaultLayer.pipe(Layer.provide(permission)) | ||
| const bash = BashTool.layer.pipe( | ||
| const bash = TerminalTool.layer.pipe( | ||
| Layer.provide(registry), | ||
| Layer.provide(permission), | ||
| Layer.provide(mutation), | ||
|
|
@@ -115,15 +115,15 @@ const withTool = <A, E, R>( | |
| }).pipe(Effect.provide(Layer.mergeAll(registry, bash))) | ||
| } | ||
|
|
||
| const call = (input: typeof BashTool.Input.Type, id = "call-bash") => ({ | ||
| const call = (input: typeof TerminalTool.Input.Type, id = "call-bash") => ({ | ||
| sessionID, | ||
| ...toolIdentity, | ||
| call: { type: "tool-call" as const, id, name: "bash", input }, | ||
| call: { type: "tool-call" as const, id, name: "terminal", input }, | ||
| }) | ||
|
|
||
| const it = testEffect(Layer.empty) | ||
|
|
||
| describe("BashTool", () => { | ||
| describe("TerminalTool", () => { | ||
| it.live("registers and returns structured successful output from the active Location", () => | ||
| Effect.acquireUseRelease( | ||
| Effect.promise(() => tmpdir()), | ||
|
|
@@ -132,9 +132,9 @@ describe("BashTool", () => { | |
| return withTool(tmp.path, (registry) => | ||
| Effect.gen(function* () { | ||
| const definitions = yield* toolDefinitions(registry) | ||
| expect(definitions.map((tool) => tool.name)).toEqual(["bash"]) | ||
| expect(definitions.map((tool) => tool.name)).toEqual(["terminal"]) | ||
| expect(definitions[0]?.inputSchema).not.toHaveProperty("properties.background") | ||
| expect(yield* toolDefinitions(registry, [{ action: "bash", resource: "*", effect: "deny" }])).toEqual([]) | ||
| expect(yield* toolDefinitions(registry, [{ action: "terminal", resource: "*", effect: "deny" }])).toEqual([]) | ||
| expect( | ||
| yield* settleTool(registry, call({ command: "pwd", description: "Print working directory" })), | ||
| ).toEqual({ | ||
|
|
@@ -152,10 +152,10 @@ describe("BashTool", () => { | |
| }) | ||
| expect(runs).toMatchObject([{ command: "pwd", cwd: realpathSync(tmp.path) }]) | ||
| expect(runs[0]?.options).toMatchObject({ | ||
| maxOutputBytes: BashTool.MAX_CAPTURE_BYTES, | ||
| maxErrorBytes: BashTool.MAX_CAPTURE_BYTES, | ||
| maxOutputBytes: TerminalTool.MAX_CAPTURE_BYTES, | ||
| maxErrorBytes: TerminalTool.MAX_CAPTURE_BYTES, | ||
| }) | ||
| expect(assertions).toMatchObject([{ sessionID, action: "bash", resources: ["pwd"], save: ["pwd"] }]) | ||
| expect(assertions).toMatchObject([{ sessionID, action: "terminal", resources: ["pwd"], save: ["pwd"] }]) | ||
| }), | ||
| ) | ||
| }, | ||
|
|
@@ -188,7 +188,7 @@ describe("BashTool", () => { | |
| reset() | ||
| const workdir = path.join(tmp.path, "src") | ||
| afterPermission = (input) => | ||
| input.action === "bash" | ||
| input.action === "terminal" | ||
| ? Effect.promise(async () => { | ||
| await fs.rm(workdir, { recursive: true }) | ||
| await fs.writeFile(workdir, "not a directory") | ||
|
|
@@ -201,7 +201,7 @@ describe("BashTool", () => { | |
| Effect.andThen( | ||
| Effect.sync(() => { | ||
| expect(runs).toEqual([]) | ||
| expect(assertions.map((input) => input.action)).toEqual(["bash"]) | ||
| expect(assertions.map((input) => input.action)).toEqual(["terminal"]) | ||
| }), | ||
| ), | ||
| ) | ||
|
|
@@ -249,7 +249,7 @@ describe("BashTool", () => { | |
| ).pipe( | ||
| Effect.andThen( | ||
| Effect.sync(() => { | ||
| expect(assertions.map((item) => item.action)).toEqual(["external_directory", "bash"]) | ||
| expect(assertions.map((item) => item.action)).toEqual(["external_directory", "terminal"]) | ||
| expect(assertions[0]).toMatchObject({ | ||
| resources: [path.join(realpathSync(outside.path), "*").replaceAll("\\", "/")], | ||
| }) | ||
|
|
@@ -281,7 +281,7 @@ describe("BashTool", () => { | |
| reset() | ||
| denyAction = "bash" | ||
| yield* withTool(active.path, (registry) => executeTool(registry, call({ command: "pwd" }))) | ||
| expect(assertions.map((item) => item.action)).toEqual(["bash"]) | ||
| expect(assertions.map((item) => item.action)).toEqual(["terminal"]) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The rename left Prompt for AI agents |
||
| expect(runs).toEqual([]) | ||
| }), | ||
| ([active, outside]) => | ||
|
|
@@ -301,7 +301,7 @@ describe("BashTool", () => { | |
| return withTool(active.path, (registry) => settleTool(registry, call({ command: `cat ${target}` }))).pipe( | ||
| Effect.andThen((settled) => | ||
| Effect.sync(() => { | ||
| expect(assertions.map((item) => item.action)).toEqual(["bash"]) | ||
| expect(assertions.map((item) => item.action)).toEqual(["terminal"]) | ||
| expect(runs).toHaveLength(1) | ||
| expect(settled.output?.structured).toMatchObject({ | ||
| warnings: [ | ||
|
|
@@ -399,7 +399,7 @@ describe("BashTool", () => { | |
| }) | ||
|
|
||
| test("keeps locked deferred parity TODOs visible", async () => { | ||
| const source = await fs.readFile(new URL("../src/tool/bash.ts", import.meta.url), "utf8") | ||
| const source = await fs.readFile(new URL("../src/tool/terminal.ts", import.meta.url), "utf8") | ||
| for (const todo of [ | ||
| "Port tree-sitter bash / PowerShell parser-based approval reduction.", | ||
| "Port BashArity reusable command-prefix approvals.", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -296,7 +296,7 @@ function shellCommand(input: ToolInput) { | |
|
|
||
| function isShell(toolName: string) { | ||
| const tool = toolName.toLocaleLowerCase() | ||
| return tool === "bash" || tool === "shell" | ||
| return tool === "terminal" || tool === "shell" | ||
|
Comment on lines
297
to
+299
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Add
Add Proposed fix switch (tool) {
+ case "terminal":
case "bash":
case "shell": {🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: This rename only updated isShell to detect "terminal", but the sibling functions in the same file — toToolKind and toLocations — still switch on "bash"/"shell". Since the tool ID is now "terminal" (Tool.define("terminal")), ACP tool calls for the terminal tool will be classified as kind "other" instead of "execute" and will lose their shell working-directory location. Add case "terminal" to both switches for consistency with the rename. Prompt for AI agents |
||
| } | ||
|
Comment on lines
297
to
300
|
||
|
|
||
| export const mapToolKind = toToolKind | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -595,7 +595,7 @@ function showDetail(span) { | |
| if (fp) changedFiles[fp] = lname.indexOf('write') >= 0 ? 'write' : 'edit'; | ||
| } else if (lname.indexOf('read') >= 0 || lname === 'glob' || lname === 'grep') { | ||
| if (fp && !changedFiles[fp]) readFiles[fp] = 1; | ||
| } else if (lname === 'bash' || lname.indexOf('shell') >= 0) { | ||
| } else if (lname === 'terminal' || lname.indexOf('shell') >= 0) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The viewer now only recognizes shell tools whose span name is Prompt for AI agents |
||
| var cmd = inpObj ? (inpObj.command || '') : (typeof inp === 'string' ? inp : ''); | ||
| if (cmd) { | ||
| // Extract the meaningful command — strip cd prefixes, take last command in chain | ||
|
|
@@ -652,7 +652,7 @@ function showDetail(span) { | |
| } | ||
|
|
||
| // For bash/shell commands — extract meaningful command and its outcome | ||
| if ((lname === 'bash' || lname.indexOf('shell') >= 0) && outStr) { | ||
| if ((lname === 'terminal' || lname.indexOf('shell') >= 0) && outStr) { | ||
| var rawCmd = String(inp.command || ''); | ||
| var cmdParts = rawCmd.split(/\\s*&&\\s*/); | ||
| var displayCmd = cmdParts[cmdParts.length - 1].trim(); | ||
|
|
@@ -1475,7 +1475,7 @@ function showDetail(span) { | |
| var fp = inp.file_path || inp.filePath || inp.path || null; | ||
| if (nm.indexOf('write') >= 0 || nm.indexOf('edit') >= 0) { if (fp) mdChanged[fp] = nm.indexOf('write') >= 0 ? 'new' : 'edited'; } | ||
| else if (nm.indexOf('read') >= 0) { mdReadCount++; } | ||
| else if (nm === 'bash') { | ||
| else if (nm === 'terminal' || nm.indexOf('shell') >= 0) { | ||
| var cmd = inp.command || ''; | ||
| var parts = cmd.split(/\\s*&&\\s*/); | ||
| var last = parts[parts.length - 1].trim().toLowerCase(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: When a user's config defines only the legacy
bashpermission key (the common backward-compat case, with noterminalkey), the decodedconfig.permissionobject is returned unchanged with thebashkey still present. The two updated config tests (''config parser preserves permission order...'' and ''agent markdown permission config preserves user key order'') both use onlybashand assert the parsed keys are["terminal", "*", "edit"], but sincenormalizeInputonly deletesbashwhenterminalis also set, the object keepsbashand the tests fail. The runtime ruleset path inPermission.fromConfigdoes remapbash→terminal, so enforcement is correct, but the exposedconfig.permissionstays inconsistent with the rename. Rename a lonebashkey toterminal(and drop it only whenterminalis also defined).Prompt for AI agents