fix(tui): rename internal bash tool to terminal for cross-platform accuracy - #1077
Open
ainaanraza wants to merge 1 commit into
Open
fix(tui): rename internal bash tool to terminal for cross-platform accuracy#1077ainaanraza wants to merge 1 commit into
ainaanraza wants to merge 1 commit into
Conversation
…s-platform accuracy
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe built-in ChangesTerminal tool rename
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR performs a broad internal rename of the shell-execution tool from bash to terminal to better reflect cross-platform behavior (e.g., PowerShell/CMD on Windows) across the CLI/TUI runtime, permission wiring, telemetry, and tests.
Changes:
- Renames tool IDs/usages from
bash→terminalacross TUI/CLI/session/telemetry codepaths and associated test fixtures/snapshots. - Updates permission parsing and evaluation call sites to use
terminalas the permission key (with some compatibility handling). - Updates documentation strings/readmes/specs to reflect the new tool name.
Reviewed changes
Copilot reviewed 95 out of 95 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tui/test/util/transcript.test.ts | Update transcript fixtures to terminal tool name |
| packages/tui/test/cli/tui/inline-tool-wrap-snapshot.test.tsx | Update tool display test inputs for terminal |
| packages/tui/test/cli/tui/data.test.tsx | Update tool event fixtures from bash → terminal |
| packages/tui/src/routes/session/index.tsx | Render shell tool under terminal and update known tool set |
| packages/tui/src/feature-plugins/home/tips-view.tsx | Update config tip example to terminal |
| packages/opencode/test/v2/session-message-updater.test.ts | Update v2 session updater fixtures to terminal |
| packages/opencode/test/upstream/v140-permission-deadlock.test.ts | Update permission deadlock regression to terminal |
| packages/opencode/test/upstream/v140-merge-fuzz.test.ts | Update telemetry outcome fixture tool name |
| packages/opencode/test/upstream/altimate-features.test.ts | Rename safety rule fixtures from bash → terminal |
| packages/opencode/test/upstream/adversarial/upi-tool-api.test.ts | Update UPI adversarial test to request terminal permission |
| packages/opencode/test/tui-journeys/README.md | Update journey scripting docs to terminal tool |
| packages/opencode/test/tui-journeys/journeys.test.ts | Update journey tool invocation to terminal |
| packages/opencode/test/tool/terminal.test.ts | Rename tool import/usage to TerminalTool |
| packages/opencode/test/tool/task.test.ts | Update task tool permission fixtures to terminal |
| packages/opencode/test/tool/shell.test.ts | Update test helper naming for terminal init |
| packages/opencode/test/tool/retrieval.test.ts | Update core-tool retention assertion to terminal |
| packages/opencode/test/tool/parameters.test.ts | Update schema snapshot test label to terminal |
| packages/opencode/test/telemetry/telemetry.test.ts | Update telemetry categorization fixtures |
| packages/opencode/test/skill/release-v0.8.6-adversarial.test.ts | Update skill adversarial fixtures to terminal |
| packages/opencode/test/skill/release-v0.8.0-adversarial.test.ts | Update release adversarial permission eval to terminal |
| packages/opencode/test/skill/release-v0.6.0-adversarial.test.ts | Update tool name extraction expectations |
| packages/opencode/test/session/schema-decoding.test.ts | Update session schema decode fixture permissions |
| packages/opencode/test/session/revert-compact.test.ts | Update compact/revert tool fixture to terminal |
| packages/opencode/test/session/regression.test.ts | Update regression fixture tool name |
| packages/opencode/test/session/prompt.test.ts | Update prompt tests to use terminal tool key |
| packages/opencode/test/session/processor.test.ts | Update processor telemetry fixtures to terminal |
| packages/opencode/test/session/observation-mask.test.ts | Update observation mask formatting expectations |
| packages/opencode/test/session/message-v2.test.ts | Update message-v2 tool call/result fixtures to terminal |
| packages/opencode/test/session/llm.test.ts | Update toolNamesFromMessages expectations |
| packages/opencode/test/session/llm-native.test.ts | Update native request/response fixtures to terminal |
| packages/opencode/test/session/compaction.test.ts | Update compaction tool fixture to terminal |
| packages/opencode/test/session/compaction-mask.test.ts | Update compaction mask fixtures/expectations |
| packages/opencode/test/release-validation/session-transcript-941.test.ts | Update release validation transcript tool fixture |
| packages/opencode/test/release-validation/question-937.test.ts | Update validation test tool import and references |
| packages/opencode/test/release-validation/question-937-codex.test.ts | Update source-path read to terminal.ts |
| packages/opencode/test/provider/transform.test.ts | Update provider transform fixtures to terminal |
| packages/opencode/test/permission/next.test.ts | Update PermissionNext config parsing/eval fixtures |
| packages/opencode/test/permission-yolo.test.ts | Update yolo-mode permission fixtures to terminal |
| packages/opencode/test/permission-task.test.ts | Update permission-task assertions to terminal |
| packages/opencode/test/file/security-e2e.test.ts | Update security E2E permission eval fixtures |
| packages/opencode/test/config/config.test.ts | Update config-order preservation assertions |
| packages/opencode/test/cli/run/subagent-data.test.ts | Update run subagent tool/permission fixtures |
| packages/opencode/test/cli/run/stream.transport.test.ts | Update stream transport tool fixture to terminal |
| packages/opencode/test/cli/run/session-replay.test.ts | Update replay fixtures to terminal |
| packages/opencode/test/cli/run/session-data.test.ts | Update session-data fixtures to terminal |
| packages/opencode/test/cli/run/scrollback.surface.test.ts | Update scrollback tool fixtures to terminal |
| packages/opencode/test/cli/run/permission.shared.test.ts | Update permission UI copy fixtures to terminal |
| packages/opencode/test/cli/run/footer.view.test.tsx | Update footer view fixtures to terminal |
| packages/opencode/test/cli/run/entry.body.test.ts | Update entry body fixtures to terminal |
| packages/opencode/test/branding/build-integrity.test.ts | Update source path checked for PATH injection |
| packages/opencode/test/altimate/tracing.test.ts | Update tracing fixtures/top-tools assertions |
| packages/opencode/test/altimate/tracing-thorough.test.ts | Update tracing fixtures to terminal |
| packages/opencode/test/altimate/tracing-rename-race.test.ts | Update trace race fixture tool name |
| packages/opencode/test/altimate/tracing-integration.test.ts | Update integration fixtures to terminal |
| packages/opencode/test/altimate/tracing-final-audit.test.ts | Update audit fixtures to terminal |
| packages/opencode/test/altimate/tracing-e2e.test.ts | Update E2E tracing fixtures to terminal |
| packages/opencode/test/altimate/tracing-display-crash.test.ts | Update crash recovery fixture tool name |
| packages/opencode/test/altimate/tracing-de-attributes.test.ts | Update DE attribute fixture tool name |
| packages/opencode/test/altimate/tracing-adversarial.test.ts | Update adversarial tracing fixtures to terminal |
| packages/opencode/test/altimate/tracing-adversarial-snapshot.test.ts | Update snapshot isolation fixtures to terminal |
| packages/opencode/test/altimate/tracing-adversarial-final.test.ts | Update final adversarial fixtures to terminal |
| packages/opencode/test/altimate/tracing-adversarial-2.test.ts | Update additional adversarial fixtures to terminal |
| packages/opencode/test/altimate/trace-consumer.test.ts | Update trace consumer fixture tool name |
| packages/opencode/test/altimate/carry-forward/agent-safety.test.ts | Update agent safety assertions to terminal |
| packages/opencode/test/agent/plan-mode-subagent-bypass.test.ts | Update permission assertions to terminal |
| packages/opencode/test/agent/agent.test.ts | Update agent default permission assertions and copy |
| packages/opencode/test/acp/event.test.ts | Update ACP event fixtures to terminal |
| packages/opencode/src/tool/terminal.ts | Rename tool definition/id and metadata tag naming |
| packages/opencode/src/tool/shell/id.ts | Update shell tool kind/id defaults to terminal |
| packages/opencode/src/tool/retrieval.ts | Update core tool list to include terminal |
| packages/opencode/src/tool/registry.ts | Register TerminalTool instead of BashTool |
| packages/opencode/src/session/prompt.ts | Emit tool parts with tool: "terminal" |
| packages/opencode/src/permission/next.ts | Remap legacy bash config key to terminal |
| packages/opencode/src/permission/index.ts | Remap legacy bash config key to terminal |
| packages/opencode/src/cli/cmd/skill.ts | Rename default tool template helper (bash → terminal) |
| packages/opencode/src/cli/cmd/run/tool.ts | Rename tool rule wiring and scroll copy to terminal |
| packages/opencode/src/cli/cmd/run/session-replay.ts | Add bash/terminal compatibility for replay detection |
| packages/opencode/src/cli/cmd/run/session-data.ts | Update session reduction logic to terminal tool name |
| packages/opencode/src/cli/cmd/run/demo.ts | Update demo emitter/tool kind to terminal |
| packages/opencode/src/cli/cmd/run.ts | Render tool parts under terminal handler |
| packages/opencode/src/cli/cmd/agent.ts | Update available permissions list (terminal) |
| packages/opencode/src/altimate/tool-source.ts | Update native tool id set (terminal) |
| packages/opencode/src/altimate/telemetry/index.ts | Update file-tool categorization list to terminal |
| packages/opencode/src/altimate/observability/viewer.ts | Update viewer shell-tool detection to terminal |
| packages/opencode/src/agent/agent.ts | Update agent default permission config from bash → terminal |
| packages/opencode/src/acp/tool.ts | Update ACP shell-tool detection logic to terminal |
| packages/opencode/src/acp/event.ts | Update running-tool snapshot gating for terminal |
| packages/opencode/specs/v2/api.ts | Update API spec tool name to terminal |
| packages/core/test/tool-terminal.test.ts | Update core tool tests and permission assertions to terminal |
| packages/core/test/session-tool-progress.test.ts | Update core session tool progress fixtures |
| packages/core/test/session-runner-tool-registry.test.ts | Update registry materialization expectations |
| packages/core/test/config/agent.test.ts | Update agent permission evaluation to terminal |
| packages/core/src/v1/config/permission.ts | Add terminal config key and normalization logic |
| packages/core/src/tool/terminal.ts | Rename core tool name constant to terminal |
| packages/core/src/tool/builtins.ts | Wire built-in location layer to TerminalTool |
Suppressed comments (4)
packages/opencode/src/cli/cmd/run/session-data.ts:930
- Shell-claiming is now gated on
part.tool === "terminal"only. That means older sessions containingtool: "bash"won’t be treated as shell calls (affecting replay formatting and any synthetic shell commits). Consider accepting both names here, consistent with session-replay’s compatibility check.
if (part.type === "tool") {
const view = syncPermission(data, part) ?? syncQuestion(data, part)
if (part.tool === "terminal" && part.callID) {
if (claimShell(data, part.callID, "tool", terminalCommand(part)).source === "shell") {
return out(data, commits, view)
packages/opencode/src/cli/cmd/run/session-data.ts:664
terminalCommand()ignores legacytool: "bash"parts, which prevents shell command extraction when replaying older sessions that were saved before the rename.
function terminalCommand(part: ToolPart): string | undefined {
if (part.tool !== "terminal") {
return undefined
}
packages/opencode/src/tool/terminal.ts:56
- The TODO comment says this tool should be renamed for other shells, but the rename to
terminalhas already happened in this PR. Leaving it in place is misleading.
packages/tui/src/routes/session/index.tsx:2652 - To keep historical sessions readable, consider treating the legacy tool name
bashas a known display tool. Without this,toolDisplay("bash")will returngeneric, and older transcripts won’t get the shell renderer.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+41
to
+48
| const normalizeInput = (input: Schema.Schema.Type<typeof InputSchema>): Schema.Schema.Type<typeof InputObject> => { | ||
| if (typeof input === "string") return { "*": input } | ||
| const result = { ...input } | ||
| if (result.terminal !== undefined && result.bash !== undefined) { | ||
| delete (result as any).bash | ||
| } | ||
| return result | ||
| } |
Comment on lines
297
to
300
| function isShell(toolName: string) { | ||
| const tool = toolName.toLocaleLowerCase() | ||
| return tool === "bash" || tool === "shell" | ||
| return tool === "terminal" || tool === "shell" | ||
| } |
| <Show when={!shouldHide()}> | ||
| <Switch> | ||
| <Match when={display() === "bash"}> | ||
| <Match when={display() === "terminal"}> |
Comment on lines
+465
to
467
| if (part.tool !== "terminal") { | ||
| return | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tui/test/util/transcript.test.ts (1)
170-181: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUpdate all expected values that still use
bash.The fixtures now use
terminal, but several expected values retain the old identifier.
packages/tui/test/util/transcript.test.ts#L170-L181: expect**Tool: terminal**.packages/tui/test/util/transcript.test.ts#L195-L207: expect**Tool: terminal**.packages/tui/test/util/transcript.test.ts#L220-L231: expect**Tool: terminal**.packages/opencode/test/session/processor.test.ts#L446-L446: expectterminalin the doom-loop assertion.packages/opencode/test/cli/run/stream.transport.test.ts#L713-L713: expectrunning terminal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tui/test/util/transcript.test.ts` around lines 170 - 181, Update all stale expected tool identifiers from bash to terminal: adjust the three formatPart expectations in packages/tui/test/util/transcript.test.ts at lines 170-181, 195-207, and 220-231; update the doom-loop assertion in packages/opencode/test/session/processor.test.ts at line 446; and update the running-tool expectation in packages/opencode/test/cli/run/stream.transport.test.ts at line 713.
🧹 Nitpick comments (3)
packages/opencode/test/skill/release-v0.8.0-adversarial.test.ts (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
bashActionto match the permission category.The helper now evaluates
"terminal"but remains namedbashAction. Rename it toterminalActionand update its call sites so the test does not retain a stale tool identifier.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/skill/release-v0.8.0-adversarial.test.ts` at line 19, Rename the helper bashAction to terminalAction to match the "terminal" permission category, and update every call site in the test to use the new name.packages/opencode/test/session/prompt.test.ts (1)
342-342: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign
hasTerminalwith the shell under test.
hasTerminalstill checksBun.which("bash"). If the configured shell is PowerShell or CMD, these guards can skip tests even when the terminal is available. Keep the namehasBashfor Bash-only cases, or check the configured shell for cross-platform coverage.Also applies to: 1485-1485, 1684-1684
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/session/prompt.test.ts` at line 342, Update the terminal availability guards around hasTerminal to match the shell being tested: retain a Bash-specific name and Bun.which("bash") check for Bash-only cases, while cross-platform cases must check the configured shell rather than assuming Bash. Apply the same correction to all corresponding occurrences.packages/core/test/config/agent.test.ts (1)
84-85: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression coverage for legacy permission keys.
These assertions cover only direct
terminalrules. Add cases for a legacybashrule and for both keys, with the explicitterminalrule taking precedence. If equivalent coverage exists elsewhere, reference that test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/test/config/agent.test.ts` around lines 84 - 85, Extend the permission evaluation tests around buildAgent.permissions with regression cases for a legacy bash rule and for simultaneous bash and terminal rules. Verify bash-only behavior is covered, and verify an explicit terminal rule takes precedence when both keys are present, reusing existing permission fixtures or assertions where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/acp/tool.ts`:
- Around line 297-299: Update the toLocations() switch to handle the "terminal"
tool name and return the same working-directory ToolCallLocation as the existing
shell/bash handling. Preserve the "bash" case for legacy ACP messages.
In `@packages/opencode/test/altimate/tracing-adversarial-2.test.ts`:
- Line 830: Update the expected tool-name assertion in the tracing adversarial
test to use "terminal, read", matching the fixture’s tool value of "terminal"
and the existing "read" entry.
In `@packages/opencode/test/release-validation/session-transcript-941.test.ts`:
- Line 128: Update the session transcript fixture and related assertions to
consistently use "terminal" instead of "bash", including the values near the
referenced lines and the expected transcript checks. Preserve state.title as
"bash" only if it intentionally represents a separate legacy label; otherwise
update it too, and add a dedicated compatibility test if that legacy behavior
must remain.
---
Outside diff comments:
In `@packages/tui/test/util/transcript.test.ts`:
- Around line 170-181: Update all stale expected tool identifiers from bash to
terminal: adjust the three formatPart expectations in
packages/tui/test/util/transcript.test.ts at lines 170-181, 195-207, and
220-231; update the doom-loop assertion in
packages/opencode/test/session/processor.test.ts at line 446; and update the
running-tool expectation in
packages/opencode/test/cli/run/stream.transport.test.ts at line 713.
---
Nitpick comments:
In `@packages/core/test/config/agent.test.ts`:
- Around line 84-85: Extend the permission evaluation tests around
buildAgent.permissions with regression cases for a legacy bash rule and for
simultaneous bash and terminal rules. Verify bash-only behavior is covered, and
verify an explicit terminal rule takes precedence when both keys are present,
reusing existing permission fixtures or assertions where applicable.
In `@packages/opencode/test/session/prompt.test.ts`:
- Line 342: Update the terminal availability guards around hasTerminal to match
the shell being tested: retain a Bash-specific name and Bun.which("bash") check
for Bash-only cases, while cross-platform cases must check the configured shell
rather than assuming Bash. Apply the same correction to all corresponding
occurrences.
In `@packages/opencode/test/skill/release-v0.8.0-adversarial.test.ts`:
- Line 19: Rename the helper bashAction to terminalAction to match the
"terminal" permission category, and update every call site in the test to use
the new name.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e493ffa3-5d08-4851-a668-22b7198973ea
📒 Files selected for processing (95)
packages/core/src/tool/builtins.tspackages/core/src/tool/terminal.tspackages/core/src/v1/config/permission.tspackages/core/test/config/agent.test.tspackages/core/test/session-runner-tool-registry.test.tspackages/core/test/session-tool-progress.test.tspackages/core/test/tool-terminal.test.tspackages/opencode/specs/v2/api.tspackages/opencode/src/acp/event.tspackages/opencode/src/acp/tool.tspackages/opencode/src/agent/agent.tspackages/opencode/src/altimate/observability/viewer.tspackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/altimate/tool-source.tspackages/opencode/src/cli/cmd/agent.tspackages/opencode/src/cli/cmd/run.tspackages/opencode/src/cli/cmd/run/demo.tspackages/opencode/src/cli/cmd/run/session-data.tspackages/opencode/src/cli/cmd/run/session-replay.tspackages/opencode/src/cli/cmd/run/tool.tspackages/opencode/src/cli/cmd/skill.tspackages/opencode/src/permission/index.tspackages/opencode/src/permission/next.tspackages/opencode/src/session/prompt.tspackages/opencode/src/tool/registry.tspackages/opencode/src/tool/retrieval.tspackages/opencode/src/tool/shell/id.tspackages/opencode/src/tool/terminal.tspackages/opencode/test/acp/event.test.tspackages/opencode/test/agent/agent.test.tspackages/opencode/test/agent/plan-mode-subagent-bypass.test.tspackages/opencode/test/altimate/carry-forward/agent-safety.test.tspackages/opencode/test/altimate/trace-consumer.test.tspackages/opencode/test/altimate/tracing-adversarial-2.test.tspackages/opencode/test/altimate/tracing-adversarial-final.test.tspackages/opencode/test/altimate/tracing-adversarial-snapshot.test.tspackages/opencode/test/altimate/tracing-adversarial.test.tspackages/opencode/test/altimate/tracing-de-attributes.test.tspackages/opencode/test/altimate/tracing-display-crash.test.tspackages/opencode/test/altimate/tracing-e2e.test.tspackages/opencode/test/altimate/tracing-final-audit.test.tspackages/opencode/test/altimate/tracing-integration.test.tspackages/opencode/test/altimate/tracing-rename-race.test.tspackages/opencode/test/altimate/tracing-thorough.test.tspackages/opencode/test/altimate/tracing.test.tspackages/opencode/test/branding/build-integrity.test.tspackages/opencode/test/cli/run/entry.body.test.tspackages/opencode/test/cli/run/footer.view.test.tsxpackages/opencode/test/cli/run/permission.shared.test.tspackages/opencode/test/cli/run/scrollback.surface.test.tspackages/opencode/test/cli/run/session-data.test.tspackages/opencode/test/cli/run/session-replay.test.tspackages/opencode/test/cli/run/stream.transport.test.tspackages/opencode/test/cli/run/subagent-data.test.tspackages/opencode/test/config/config.test.tspackages/opencode/test/file/security-e2e.test.tspackages/opencode/test/permission-task.test.tspackages/opencode/test/permission-yolo.test.tspackages/opencode/test/permission/next.test.tspackages/opencode/test/provider/transform.test.tspackages/opencode/test/release-validation/question-937-codex.test.tspackages/opencode/test/release-validation/question-937.test.tspackages/opencode/test/release-validation/session-transcript-941.test.tspackages/opencode/test/session/compaction-mask.test.tspackages/opencode/test/session/compaction.test.tspackages/opencode/test/session/llm-native.test.tspackages/opencode/test/session/llm.test.tspackages/opencode/test/session/message-v2.test.tspackages/opencode/test/session/observation-mask.test.tspackages/opencode/test/session/processor.test.tspackages/opencode/test/session/prompt.test.tspackages/opencode/test/session/regression.test.tspackages/opencode/test/session/revert-compact.test.tspackages/opencode/test/session/schema-decoding.test.tspackages/opencode/test/skill/release-v0.6.0-adversarial.test.tspackages/opencode/test/skill/release-v0.8.0-adversarial.test.tspackages/opencode/test/skill/release-v0.8.6-adversarial.test.tspackages/opencode/test/telemetry/telemetry.test.tspackages/opencode/test/tool/parameters.test.tspackages/opencode/test/tool/retrieval.test.tspackages/opencode/test/tool/shell.test.tspackages/opencode/test/tool/task.test.tspackages/opencode/test/tool/terminal.test.tspackages/opencode/test/tui-journeys/README.mdpackages/opencode/test/tui-journeys/journeys.test.tspackages/opencode/test/upstream/adversarial/upi-tool-api.test.tspackages/opencode/test/upstream/altimate-features.test.tspackages/opencode/test/upstream/v140-merge-fuzz.test.tspackages/opencode/test/upstream/v140-permission-deadlock.test.tspackages/opencode/test/v2/session-message-updater.test.tspackages/tui/src/feature-plugins/home/tips-view.tsxpackages/tui/src/routes/session/index.tsxpackages/tui/test/cli/tui/data.test.tsxpackages/tui/test/cli/tui/inline-tool-wrap-snapshot.test.tsxpackages/tui/test/util/transcript.test.ts
Comment on lines
297
to
+299
| function isShell(toolName: string) { | ||
| const tool = toolName.toLocaleLowerCase() | ||
| return tool === "bash" || tool === "shell" | ||
| return tool === "terminal" || tool === "shell" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add terminal to toLocations().
isShell() now recognizes "terminal", but toLocations() still handles "bash" and "shell". A terminal call therefore reaches the default branch and returns no ToolCallLocation. ACP clients lose the terminal working-directory location.
Add case "terminal": to the toLocations() switch. Keep case "bash": if legacy ACP messages must remain supported.
Proposed fix
switch (tool) {
+ case "terminal":
case "bash":
case "shell": {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/acp/tool.ts` around lines 297 - 299, Update the
toLocations() switch to handle the "terminal" tool name and return the same
working-directory ToolCallLocation as the existing shell/bash handling. Preserve
the "bash" case for legacy ACP messages.
| tracer.logStepStart({ id: "1" }) | ||
| tracer.logToolCall({ | ||
| tool: "bash", | ||
| tool: "terminal", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the stale tool-name expectation.
Line 830 now records tool: "terminal", but the test still expects "bash, read". The fixture and assertion no longer describe the same tool list. Change the expected value to "terminal, read".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/altimate/tracing-adversarial-2.test.ts` at line 830,
Update the expected tool-name assertion in the tracing adversarial test to use
"terminal, read", matching the fixture’s tool value of "terminal" and the
existing "read" entry.
| type: "tool", | ||
| callID: "call_completed", | ||
| tool: "bash", | ||
| tool: "terminal", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Update the transcript fixture and assertions to terminal.
Line 128 now uses tool: "terminal", but Line 133 and Lines 175 and 190 still use "bash". This leaves the fixture and expected transcript inconsistent with the rename. Update those values to terminal, unless state.title is intentionally a separate legacy label. In that case, add a separate compatibility test instead of keeping the main transcript test on bash.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/test/release-validation/session-transcript-941.test.ts` at
line 128, Update the session transcript fixture and related assertions to
consistently use "terminal" instead of "bash", including the values near the
referenced lines and the expected transcript checks. Preserve state.title as
"bash" only if it intentionally represents a separate legacy label; otherwise
update it too, and add a dedicated compatibility test if that legacy behavior
must remain.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge This PR is a large (~95-file), mostly mechanical Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (95 files — rename refactor)Substantive source files reviewed:
Notes:
Fix these issues in Kilo Cloud Reviewed by glm-5.2 · Input: 149.4K · Output: 44.4K · Cached: 2.6M Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
40 issues found across 95 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/test/session-runner-tool-registry.test.ts">
<violation number="1" location="packages/core/test/session-runner-tool-registry.test.ts:69">
P1: The test registers tools via `service.register({ question: make(), bash: make(), edit: ... })`, and registry names are the registration keys (materialize calls `definition(name, tool)` with `name` from the key), so the fake shell tool materializes as "bash", not "terminal". The renamed expectation `["terminal", "edit", "write", "apply_patch"]` (and the later `["question", "terminal"]`) will now fail because the registration key `bash:` was never renamed. Rename the register key to `terminal:` to match, or the test breaks.</violation>
</file>
<file name="packages/opencode/test/cli/run/stream.transport.test.ts">
<violation number="1" location="packages/opencode/test/cli/run/stream.transport.test.ts:713">
P1: This test will fail: the runningTool input was renamed to "terminal", but the assertion still expects status "running bash". Since toolStatus() builds the label as `running ${part.tool}`, the actual status is now "running terminal" and the expect.objectContaining won't match. Update the expected status to "running terminal".</violation>
</file>
<file name="packages/opencode/test/session/compaction-mask.test.ts">
<violation number="1" location="packages/opencode/test/session/compaction-mask.test.ts:51">
P1: The test inputs were renamed to "terminal" but the assertions still check for "bash(" / "bash()" / "bash([unserializable])", and createObservationMask echoes part.tool verbatim. Update those assertions to "terminal(" etc. so the tests pass and mirror the rename.</violation>
</file>
<file name="packages/opencode/test/session/processor.test.ts">
<violation number="1" location="packages/opencode/test/session/processor.test.ts:253">
P2: The 'shell tools' categorization test still asserts categorizeToolName("bash","standard") === "file", but bash was removed from FILE_TOOLS in this rename, so it now returns "standard" and the test fails. Update it to use "terminal" (or drop the stale case).</violation>
<violation number="2" location="packages/opencode/test/session/processor.test.ts:446">
P1: The doom_loop_detected test sets tool_name to "terminal" but still asserts it equals "bash", so this test now always fails. Update line 452 to expect "terminal".</violation>
</file>
<file name="packages/tui/test/util/transcript.test.ts">
<violation number="1" location="packages/tui/test/util/transcript.test.ts:170">
P1: The tool part inputs in these four transcript tests were renamed from 'bash' to 'terminal', but the assertions in the same describe block were not updated and still expect `**Tool: bash**` (lines 181, 207, 231). Since `formatPart` in `src/util/transcript.ts` builds the header directly from `part.tool` with no display-name mapping, the output is now `**Tool: terminal**`, so these assertions will fail. Update the expected labels to `terminal` so the tests pass and continue to verify the renamed tool.</violation>
</file>
<file name="packages/opencode/test/skill/release-v0.6.0-adversarial.test.ts">
<violation number="1" location="packages/opencode/test/skill/release-v0.6.0-adversarial.test.ts:196">
P1: This test now fails: the input is still `msg("bash")`, which produces a tool-call whose `toolName` is `"bash"`, and `LLM.toolNamesFromMessages` returns that name verbatim (`new Set(["bash"])`), not `"terminal"`. The bash->terminal rename updated the expected value but missed the argument passed to `msg(...)`, so the assertion compares `["bash"]` against `["terminal"]`. Change the input to `msg("terminal")` so the tool name and the expected set match.</violation>
<violation number="2" location="packages/opencode/test/skill/release-v0.6.0-adversarial.test.ts:270">
P1: This test also fails after the rename: the messages still come from `msg("bash")`, so `toolNamesFromMessages` returns `new Set(["bash", "read"])`, while the assertion now expects `new Set(["terminal", "read"])`. Update the `msg("bash")` inputs to `msg("terminal")` (in the `messages` array above this line) so the input tool name matches the expected set.</violation>
</file>
<file name="packages/opencode/src/tool/terminal.ts">
<violation number="1" location="packages/opencode/src/tool/terminal.ts:56">
P2: On Windows, the renamed terminal still presents the model with Bash-specific instructions, which can lead it to generate Bash syntax for the PowerShell/CMD shell selected by `Shell.acceptable()`. Use a shell-neutral description (or generate the instructions from the selected shell) instead of continuing to load `bash.txt`.</violation>
<violation number="2" location="packages/opencode/src/tool/terminal.ts:311">
P3: Timed-out terminal calls still report `bash tool terminated...` inside the new `<terminal_metadata>` wrapper, leaving the renamed tool visibly inconsistent. Rename that timeout message to `terminal tool ...` as part of the metadata rename.</violation>
</file>
<file name="packages/opencode/test/upstream/adversarial/upi-tool-api.test.ts">
<violation number="1" location="packages/opencode/test/upstream/adversarial/upi-tool-api.test.ts:153">
P2: The renamed ctx.ask permission ("terminal") no longer matches the test's assertion, which still expects an event containing "ask:bash:echo *:echo *". Since the ask callback serializes input.permission into the event string, this test will now fail; update the expected event to "ask:terminal:echo *:echo *".</violation>
</file>
<file name="packages/core/test/config/agent.test.ts">
<violation number="1" location="packages/core/test/config/agent.test.ts:84">
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.</violation>
</file>
<file name="packages/opencode/src/tool/shell/id.ts">
<violation number="1" location="packages/opencode/src/tool/shell/id.ts:16">
P1: Existing integrations that invoke the legacy `bash` tool ID will no longer resolve because this changes the registered ID to `terminal` without an alias; the compatibility code only covers config permission keys. Preserve a `bash` alias/normalization for tool calls, or remove the compatibility claim and explicitly accept this breaking change.</violation>
<violation number="2" location="packages/opencode/src/tool/shell/id.ts:16">
P2: Persisted permission rules keyed by `bash` stop applying to terminal requests after this ID change, so users can unexpectedly receive prompts or lose stored denials. Normalize stored rules on load (with `terminal` taking precedence) or migrate the persisted permission rows alongside the config mapping.</violation>
</file>
<file name="packages/core/test/tool-terminal.test.ts">
<violation number="1" location="packages/core/test/tool-terminal.test.ts:284">
P2: The rename left `denyAction = "bash"` stale, so the deny branch never matches the tool's new `"terminal"` action and the tool executes, failing `expect(runs).toEqual([])` in the 'does not execute after external-directory or bash denial' test. Change it to `denyAction = "terminal"` so the renamed test still verifies the denial path.</violation>
</file>
<file name="packages/opencode/test/altimate/tracing-final-audit.test.ts">
<violation number="1" location="packages/opencode/test/altimate/tracing-final-audit.test.ts:190">
P2: The tool name was renamed to "terminal" here but the assertion 15 lines below still expects the generation input to contain "[bash]". Since tracing.ts (line 791) builds the input as `[${tool}] ...`, the input will be "[terminal] ..." and this `toContain("[bash]")` assertion will fail. Update it to "[terminal]".</violation>
</file>
<file name="packages/opencode/test/altimate/tracing-adversarial-snapshot.test.ts">
<violation number="1" location="packages/opencode/test/altimate/tracing-adversarial-snapshot.test.ts:287">
P2: The first tool call in this test was renamed to "terminal", but the span lookup still searches for `s.name === "bash"` (line 600). Since `logToolCall` sets the span name to the tool value, the find returns undefined and `tool.attributes!.normal` throws, so this test now fails. Update the lookup to `s.name === "terminal"`.</violation>
</file>
<file name="packages/opencode/test/altimate/tracing-adversarial-2.test.ts">
<violation number="1" location="packages/opencode/test/altimate/tracing-adversarial-2.test.ts:54">
P2: The renamed `tool: "terminal"` now makes the generation output `"[tool calls: terminal, read]"`, but the assertion at line 843 still expects `"[tool calls: bash, read]"`, so this test fails. Update the expected string to match the renamed tool.</violation>
</file>
<file name="packages/opencode/test/tool/parameters.test.ts">
<violation number="1" location="packages/opencode/test/tool/parameters.test.ts:77">
P2: Renaming the test from `bash` to `terminal` changes the snapshot key, but the snapshot file still only records `... wire shape) bash 1` and has no `terminal` entry. `bun test` in CI will fail: the new `terminal` snapshot is unmatched and the old `bash` snapshot is obsolete. Regenerate the snapshot (or rename the key in parameters.test.ts.snap) to `tool parameters JSON Schema (wire shape) terminal 1`.</violation>
</file>
<file name="packages/opencode/test/altimate/tracing-integration.test.ts">
<violation number="1" location="packages/opencode/test/altimate/tracing-integration.test.ts:110">
P2: The tool-call rename to "terminal" wasn't propagated to the span lookup: the roundtrip test still queries traceFile.spans.find((s) => s.name === "bash"), which now returns undefined and makes the subsequent dbtTool.attributes! throw, failing the test. Update the assertion to s.name === "terminal".</violation>
</file>
<file name="packages/opencode/src/cli/cmd/agent.ts">
<violation number="1" location="packages/opencode/src/cli/cmd/agent.ts:23">
P2: Existing `agent create --tools bash` invocations now generate agents with terminal access denied. Mapping the legacy `bash` value to `terminal` in `LEGACY_TOOL_PERMISSION` would preserve the CLI contract just as the existing `write` and `apply_patch` mappings do.</violation>
</file>
<file name="packages/opencode/test/altimate/tracing.test.ts">
<violation number="1" location="packages/opencode/test/altimate/tracing.test.ts:316">
P2: This test logs tool "terminal" but still asserts the loop records tool "bash" (lines 1009-1011). Loop detection stores the exact tool name passed in, so this now fails in CI. Update those assertions to "terminal".</violation>
</file>
<file name="packages/opencode/test/cli/run/session-replay.test.ts">
<violation number="1" location="packages/opencode/test/cli/run/session-replay.test.ts:172">
P2: The test "keeps the footer in a running state for resumed active tools" now builds a tool part with tool "terminal", but the assertion at line 368 still expects status "running bash". Since toolStatus() derives the status from part.tool (session-data.ts:401), replaying this message produces "running terminal", so the assertion is stale and the test will fail. Update the expectation to "running terminal".</violation>
</file>
<file name="packages/opencode/src/acp/tool.ts">
<violation number="1" location="packages/opencode/src/acp/tool.ts:299">
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.</violation>
</file>
<file name="packages/opencode/test/tool/terminal.test.ts">
<violation number="1" location="packages/opencode/test/tool/terminal.test.ts:74">
P2: The permission-based tests in this file will fail after the rename. The tool now requests permission `"terminal"` (changed in `packages/opencode/src/tool/terminal.ts`), but the assertions in the `tool.bash permissions` block were not updated and still look for `"bash"` — e.g. `expect(requests[0].permission).toBe("bash")` (lines 108 and 132) and `requests.find((r) => r.permission === "bash")` followed by `expect(bashReq).toBeDefined()` (lines 304 and 322). Since no request with permission `"bash"` is emitted anymore, these expectations are now wrong and the tests will throw. The rename updated the tool import/usage but missed these permission assertions, which contradicts the PR's claim that the full suite passes. Please update the assertions to use `"terminal"` alongside the tool rename.</violation>
</file>
<file name="packages/opencode/test/release-validation/session-transcript-941.test.ts">
<violation number="1" location="packages/opencode/test/release-validation/session-transcript-941.test.ts:128">
P2: The tool part was renamed to "terminal" but the assertions still expect `**Tool: bash**`. `formatTranscript` renders the header from `part.tool`, so now it emits `**Tool: terminal**` and both `expect(...).toContain("**Tool: bash**")` assertions will fail. Update those assertions to `"**Tool: terminal**"`.</violation>
</file>
<file name="packages/opencode/src/cli/cmd/run/tool.ts">
<violation number="1" location="packages/opencode/src/cli/cmd/run/tool.ts:1046">
P2: Replaying sessions created before this rename now loses the bash tool rule: historical `bash` parts fall back to the generic formatter and emit a final completion that the previous rule intentionally suppressed. Keeping a legacy `bash` alias or normalizing `bash` to `terminal` in `rule()` would preserve existing session output.</violation>
</file>
<file name="packages/core/src/v1/config/permission.ts">
<violation number="1" location="packages/core/src/v1/config/permission.ts:44">
P2: When a user's config defines only the legacy `bash` permission key (the common backward-compat case, with no `terminal` key), the decoded `config.permission` object is returned unchanged with the `bash` key still present. The two updated config tests (''config parser preserves permission order...'' and ''agent markdown permission config preserves user key order'') both use only `bash` and assert the parsed keys are `["terminal", "*", "edit"]`, but since `normalizeInput` only deletes `bash` when `terminal` is *also* set, the object keeps `bash` and the tests fail. The runtime ruleset path in `Permission.fromConfig` does remap `bash`→`terminal`, so enforcement is correct, but the exposed `config.permission` stays inconsistent with the rename. Rename a lone `bash` key to `terminal` (and drop it only when `terminal` is also defined).</violation>
</file>
<file name="packages/opencode/src/cli/cmd/run/demo.ts">
<violation number="1" location="packages/opencode/src/cli/cmd/run/demo.ts:38">
P2: The built-in `/permission bash` example is now rejected because `permissionKind` accepts only the renamed `terminal` entry, so users following the displayed example get the picker error instead of a permission request; update the example to `/permission terminal`.</violation>
<violation number="2" location="packages/opencode/src/cli/cmd/run/demo.ts:797">
P3: The demo mode's bash→terminal rename is incomplete: the `/fmt` dispatcher was updated to match `kind === "terminal"`, but the `KINDS` array (and the `/permission bash` footer example) still list `bash`. As a result the documented `/fmt bash` slash command no longer produces any output (it silently returns false) while `/fmt terminal` works, and `/permission bash` points at a kind that `permissionKind`/`PERMISSIONS` no longer accept. Please also update the `KINDS` array entry and the footer examples to `terminal` so the demo help and the dispatcher stay in sync.</violation>
</file>
<file name="packages/opencode/test/session/llm-native.test.ts">
<violation number="1" location="packages/opencode/test/session/llm-native.test.ts:246">
P2: The renamed expectations (name: "terminal") don't match the still-`bash`-keyed tools map: native-request.ts derives each tool's name from the map key, so this tools block produces `name: "bash"` both in request.tools and as the definition for the tool-call, failing the assertion and breaking the message/definition name alignment. Rename the `tools` record key to `terminal` so the definition name matches the renamed tool-call and assertions.</violation>
</file>
<file name="packages/opencode/test/altimate/carry-forward/agent-safety.test.ts">
<violation number="1" location="packages/opencode/test/altimate/carry-forward/agent-safety.test.ts:89">
P2: The rename updated the bare Permission.evaluate(...,"terminal") calls but left the evalPerm(...,"bash") assertions in the same blocks stale. Since defaults/safetyDenials now register rules only under "terminal" and there is no "bash" rule anymore, evalPerm(builder,"bash") falls through to the default `<code>*: "allow"` catch-all and returns "allow" instead of the asserted "ask", so this guard test no longer asserts the intended behavior (and the first-block assertion fails). Update those helper calls to "terminal" (expectations stay "ask"/"allow") so the renamed safety rules are what actually get checked.</violation>
</file>
<file name="packages/opencode/src/cli/cmd/run.ts">
<violation number="1" location="packages/opencode/src/cli/cmd/run.ts:569">
P2: When `run --attach` talks to a pre-rename server, completed shell calls now fall through to the generic fallback, so users see the command input but not its output. Keeping `bash` as a legacy alias here (or normalizing incoming parts) would preserve cross-version rendering while using `terminal` for new events.</violation>
</file>
<file name="packages/opencode/test/acp/event.test.ts">
<violation number="1" location="packages/opencode/test/acp/event.test.ts:238">
P2: The rename was applied inconsistently within this test file: runningTool/errorTool now emit `tool: "terminal"`, while completedTool still defaults to `tool: "bash"` and all fixtures still set `title: "bash"`. Align completedTool's default and the title labels so the fixtures match production's `terminal` naming and stay consistent with the other helpers.</violation>
</file>
<file name="packages/tui/src/routes/session/index.tsx">
<violation number="1" location="packages/tui/src/routes/session/index.tsx:2652">
P2: Existing/peristed tool parts in session history still carry tool id "bash"; after this rename toolDisplay() returns "generic" for them, so resuming an old session renders those bash calls as GenericTool instead of the Shell block. The PR only added a bash→terminal compatibility layer for config permission keys, not for stored messages. Recommend keeping "bash" in toolDisplays (or mapping it to Shell) so legacy history still renders correctly.</violation>
</file>
<file name="packages/opencode/test/permission-yolo.test.ts">
<violation number="1" location="packages/opencode/test/permission-yolo.test.ts:250">
P3: Missed rename: the ruleset in the "DeniedError contains relevant ruleset info" test now uses permission "terminal", but this filter still matches "bash", so the DeniedError is constructed with an empty ruleset and carries no relevant rule info. The test passes only because it asserts the message contains "deny", silently defeating its intent. Update the filter to "terminal".</violation>
</file>
<file name="packages/opencode/src/altimate/observability/viewer.ts">
<violation number="1" location="packages/opencode/src/altimate/observability/viewer.ts:598">
P2: The viewer now only recognizes shell tools whose span name is `terminal` (or contains `shell`), which drops the legacy `bash` name it previously matched. Existing saved trace files recorded before this rename store shell-tool spans as `bash`, so after this change shell commands in those historical traces will silently stop being extracted: they will no longer appear in the shell-command list, dbt detection, command-outcome summaries, or the 'Ran N shell command(s)' line in the markdown summary. This is the same backward-compat concern the PR addressed for config permissions with a `bash`->`terminal` remap, so consider also accepting the legacy `bash` name when classifying spans here (e.g. `lname === 'terminal' || lname === 'bash' || lname.indexOf('shell') >= 0` in all three branches), so historical traces keep rendering correctly.</violation>
</file>
<file name="packages/opencode/test/permission/next.test.ts">
<violation number="1" location="packages/opencode/test/permission/next.test.ts:142">
P3: The backward-compat precedence behavior added in this PR (when both `bash` and `terminal` are configured, `terminal` wins) is not covered by any test. The renames here keep the bash-input cases but never assert the documented both-keys-defined case or a plain `terminal` input key, so a regression in that new precedence logic would pass CI. Consider adding a fromConfig test that supplies both keys and expects only the terminal rule (plus a `terminal`-only input test).</violation>
</file>
<file name="packages/opencode/src/cli/cmd/run/session-data.ts">
<violation number="1" location="packages/opencode/src/cli/cmd/run/session-data.ts:928">
P2: When a session saved before this rename (whose tool parts still carry `tool: "bash"`) is replayed, `reduceSessionData` no longer treats those parts as shell: echo-stripping and the direct-shell source claim are skipped. This conflicts with `isShellSyntheticAssistant` in session-replay.ts, which — in the same batch — explicitly kept matching `"bash"` for those very transcripts, so the same old session is classified as shell in one place and not shell in another. Consider keeping the legacy `"bash"` name in these runtime matches (as the config backend-compat layer and session-replay already do) so replayed legacy sessions behave consistently.</violation>
</file>
<file name="packages/opencode/src/session/prompt.ts">
<violation number="1" location="packages/opencode/src/session/prompt.ts:2664">
P3: This fixture now records `tool: "terminal"`, so any expectation that still checks for `bash` in the tool list is stale. Updating the expected string to `terminal, read` keeps the test aligned with the renamed tool IDs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| 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.
P1: The test registers tools via service.register({ question: make(), bash: make(), edit: ... }), and registry names are the registration keys (materialize calls definition(name, tool) with name from the key), so the fake shell tool materializes as "bash", not "terminal". The renamed expectation ["terminal", "edit", "write", "apply_patch"] (and the later ["question", "terminal"]) will now fail because the registration key bash: was never renamed. Rename the register key to terminal: to match, or the test breaks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/test/session-runner-tool-registry.test.ts, line 69:
<comment>The test registers tools via `service.register({ question: make(), bash: make(), edit: ... })`, and registry names are the registration keys (materialize calls `definition(name, tool)` with `name` from the key), so the fake shell tool materializes as "bash", not "terminal". The renamed expectation `["terminal", "edit", "write", "apply_patch"]` (and the later `["question", "terminal"]`) will now fail because the registration key `bash:` was never renamed. Rename the register key to `terminal:` to match, or the test breaks.</comment>
<file context>
@@ -66,8 +66,7 @@ describe("ToolRegistry", () => {
- expect(yield* names([{ action: "question", resource: "*", effect: "deny" }])).toEqual([
- "bash",
+ expect(yield* names([{ action: "question", resource: "*", effect: "deny" }])).toEqual(["terminal",
"edit",
"write",
</file context>
| id: "bash-1", | ||
| callID: "call-1", | ||
| tool: "bash", | ||
| tool: "terminal", |
There was a problem hiding this comment.
P1: This test will fail: the runningTool input was renamed to "terminal", but the assertion still expects status "running bash". Since toolStatus() builds the label as running ${part.tool}, the actual status is now "running terminal" and the expect.objectContaining won't match. Update the expected status to "running terminal".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/cli/run/stream.transport.test.ts, line 713:
<comment>This test will fail: the runningTool input was renamed to "terminal", but the assertion still expects status "running bash". Since toolStatus() builds the label as `running ${part.tool}`, the actual status is now "running terminal" and the expect.objectContaining won't match. Update the expected status to "running terminal".</comment>
<file context>
@@ -710,7 +710,7 @@ describe("run stream transport", () => {
id: "bash-1",
callID: "call-1",
- tool: "bash",
+ tool: "terminal",
body: {
command: "pwd",
</file context>
| test("includes tool name, args, line count, byte size, and fingerprint for completed part", () => { | ||
| const part = makeCompletedPart({ | ||
| tool: "bash", | ||
| tool: "terminal", |
There was a problem hiding this comment.
P1: The test inputs were renamed to "terminal" but the assertions still check for "bash(" / "bash()" / "bash([unserializable])", and createObservationMask echoes part.tool verbatim. Update those assertions to "terminal(" etc. so the tests pass and mirror the rename.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/session/compaction-mask.test.ts, line 51:
<comment>The test inputs were renamed to "terminal" but the assertions still check for "bash(" / "bash()" / "bash([unserializable])", and createObservationMask echoes part.tool verbatim. Update those assertions to "terminal(" etc. so the tests pass and mirror the rename.</comment>
<file context>
@@ -48,7 +48,7 @@ function makePendingPart(overrides?: { tool?: string }): MessageV2.ToolPart {
test("includes tool name, args, line count, byte size, and fingerprint for completed part", () => {
const part = makeCompletedPart({
- tool: "bash",
+ tool: "terminal",
input: { command: "git status" },
output: "On branch main\nnothing to commit, working tree clean\n",
</file context>
| timestamp: Date.now(), | ||
| session_id: "sess-1", | ||
| tool_name: "bash", | ||
| tool_name: "terminal", |
There was a problem hiding this comment.
P1: The doom_loop_detected test sets tool_name to "terminal" but still asserts it equals "bash", so this test now always fails. Update line 452 to expect "terminal".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/session/processor.test.ts, line 446:
<comment>The doom_loop_detected test sets tool_name to "terminal" but still asserts it equals "bash", so this test now always fails. Update line 452 to expect "terminal".</comment>
<file context>
@@ -443,7 +443,7 @@ describe("doom loop detection telemetry", () => {
timestamp: Date.now(),
session_id: "sess-1",
- tool_name: "bash",
+ tool_name: "terminal",
repeat_count: DOOM_LOOP_THRESHOLD,
}
</file context>
| type: "tool", | ||
| callID: "call_1", | ||
| tool: "bash", | ||
| tool: "terminal", |
There was a problem hiding this comment.
P1: The tool part inputs in these four transcript tests were renamed from 'bash' to 'terminal', but the assertions in the same describe block were not updated and still expect **Tool: bash** (lines 181, 207, 231). Since formatPart in src/util/transcript.ts builds the header directly from part.tool with no display-name mapping, the output is now **Tool: terminal**, so these assertions will fail. Update the expected labels to terminal so the tests pass and continue to verify the renamed tool.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/test/util/transcript.test.ts, line 170:
<comment>The tool part inputs in these four transcript tests were renamed from 'bash' to 'terminal', but the assertions in the same describe block were not updated and still expect `**Tool: bash**` (lines 181, 207, 231). Since `formatPart` in `src/util/transcript.ts` builds the header directly from `part.tool` with no display-name mapping, the output is now `**Tool: terminal**`, so these assertions will fail. Update the expected labels to `terminal` so the tests pass and continue to verify the renamed tool.</comment>
<file context>
@@ -167,7 +167,7 @@ describe("transcript", () => {
type: "tool",
callID: "call_1",
- tool: "bash",
+ tool: "terminal",
state: {
status: "completed",
</file context>
|
|
||
| if (resultMetadata.length > 0) { | ||
| output += "\n\n<bash_metadata>\n" + resultMetadata.join("\n") + "\n</bash_metadata>" | ||
| output += "\n\n<terminal_metadata>\n" + resultMetadata.join("\n") + "\n</terminal_metadata>" |
There was a problem hiding this comment.
P3: Timed-out terminal calls still report bash tool terminated... inside the new <terminal_metadata> wrapper, leaving the renamed tool visibly inconsistent. Rename that timeout message to terminal tool ... as part of the metadata rename.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/tool/terminal.ts, line 311:
<comment>Timed-out terminal calls still report `bash tool terminated...` inside the new `<terminal_metadata>` wrapper, leaving the renamed tool visibly inconsistent. Rename that timeout message to `terminal tool ...` as part of the metadata rename.</comment>
<file context>
@@ -308,7 +308,7 @@ export const BashTool = Tool.define("bash", async () => {
if (resultMetadata.length > 0) {
- output += "\n\n<bash_metadata>\n" + resultMetadata.join("\n") + "\n</bash_metadata>"
+ output += "\n\n<terminal_metadata>\n" + resultMetadata.join("\n") + "\n</terminal_metadata>"
}
</file context>
| try { | ||
| // Simulate what ask() does when it encounters deny | ||
| const result = PermissionNext.evaluate("bash", "DROP TABLE users", rules) | ||
| const result = PermissionNext.evaluate("terminal", "DROP TABLE users", rules) |
There was a problem hiding this comment.
P3: Missed rename: the ruleset in the "DeniedError contains relevant ruleset info" test now uses permission "terminal", but this filter still matches "bash", so the DeniedError is constructed with an empty ruleset and carries no relevant rule info. The test passes only because it asserts the message contains "deny", silently defeating its intent. Update the filter to "terminal".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/permission-yolo.test.ts, line 250:
<comment>Missed rename: the ruleset in the "DeniedError contains relevant ruleset info" test now uses permission "terminal", but this filter still matches "bash", so the DeniedError is constructed with an empty ruleset and carries no relevant rule info. The test passes only because it asserts the message contains "deny", silently defeating its intent. Update the filter to "terminal".</comment>
<file context>
@@ -206,48 +206,48 @@ describe("yolo mode: permission evaluation is unchanged", () => {
try {
// Simulate what ask() does when it encounters deny
- const result = PermissionNext.evaluate("bash", "DROP TABLE users", rules)
+ const result = PermissionNext.evaluate("terminal", "DROP TABLE users", rules)
if (result.action === "deny") {
throw new PermissionNext.DeniedError(rules.filter((r) => r.permission === "bash"))
</file context>
|
|
||
| expect(wildcardFirst.map((r) => r.permission)).toEqual(["*", "bash"]) | ||
| expect(specificFirst.map((r) => r.permission)).toEqual(["bash", "*"]) | ||
| expect(wildcardFirst.map((r) => r.permission)).toEqual(["*", "terminal"]) |
There was a problem hiding this comment.
P3: The backward-compat precedence behavior added in this PR (when both bash and terminal are configured, terminal wins) is not covered by any test. The renames here keep the bash-input cases but never assert the documented both-keys-defined case or a plain terminal input key, so a regression in that new precedence logic would pass CI. Consider adding a fromConfig test that supplies both keys and expects only the terminal rule (plus a terminal-only input test).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/permission/next.test.ts, line 142:
<comment>The backward-compat precedence behavior added in this PR (when both `bash` and `terminal` are configured, `terminal` wins) is not covered by any test. The renames here keep the bash-input cases but never assert the documented both-keys-defined case or a plain `terminal` input key, so a regression in that new precedence logic would pass CI. Consider adding a fromConfig test that supplies both keys and expects only the terminal rule (plus a `terminal`-only input test).</comment>
<file context>
@@ -139,17 +139,17 @@ test("fromConfig - preserves top-level config key order", () => {
- expect(wildcardFirst.map((r) => r.permission)).toEqual(["*", "bash"])
- expect(specificFirst.map((r) => r.permission)).toEqual(["bash", "*"])
+ expect(wildcardFirst.map((r) => r.permission)).toEqual(["*", "terminal"])
+ expect(specificFirst.map((r) => r.permission)).toEqual(["terminal", "*"])
</file context>
| messageID: msg.id, | ||
| sessionID: input.sessionID, | ||
| tool: "bash", | ||
| tool: "terminal", |
There was a problem hiding this comment.
P3: This fixture now records tool: "terminal", so any expectation that still checks for bash in the tool list is stale. Updating the expected string to terminal, read keeps the test aligned with the renamed tool IDs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/session/prompt.ts, line 2664:
<comment>This fixture now records `tool: "terminal"`, so any expectation that still checks for `bash` in the tool list is stale. Updating the expected string to `terminal, read` keeps the test aligned with the renamed tool IDs.</comment>
<file context>
@@ -2661,7 +2661,7 @@ NOTE: At any point in time through this workflow you should feel free to ask the
messageID: msg.id,
sessionID: input.sessionID,
- tool: "bash",
+ tool: "terminal",
callID: ulid(),
state: {
</file context>
| const file = path.join(root, "src", "demo-format.ts") | ||
|
|
||
| if (kind === "bash") { | ||
| if (kind === "terminal") { |
There was a problem hiding this comment.
P3: The demo mode's bash→terminal rename is incomplete: the /fmt dispatcher was updated to match kind === "terminal", but the KINDS array (and the /permission bash footer example) still list bash. As a result the documented /fmt bash slash command no longer produces any output (it silently returns false) while /fmt terminal works, and /permission bash points at a kind that permissionKind/PERMISSIONS no longer accept. Please also update the KINDS array entry and the footer examples to terminal so the demo help and the dispatcher stay in sync.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/cli/cmd/run/demo.ts, line 797:
<comment>The demo mode's bash→terminal rename is incomplete: the `/fmt` dispatcher was updated to match `kind === "terminal"`, but the `KINDS` array (and the `/permission bash` footer example) still list `bash`. As a result the documented `/fmt bash` slash command no longer produces any output (it silently returns false) while `/fmt terminal` works, and `/permission bash` points at a kind that `permissionKind`/`PERMISSIONS` no longer accept. Please also update the `KINDS` array entry and the footer examples to `terminal` so the demo help and the dispatcher stay in sync.</comment>
<file context>
@@ -794,16 +794,16 @@ function emitPermission(state: State, kind: PermissionKind = "edit"): void {
const file = path.join(root, "src", "demo-format.ts")
- if (kind === "bash") {
+ if (kind === "terminal") {
const command = "git status --short"
- const ref = make(state, "bash", {
</file context>
Author
|
Thanks for the thorough bot reviews! We've completed the core rename across 90+ files and handled backward compatibility, but there are still several edge-case tests and legacy identifiers left to fix. Because this refactor touches so many foundational files, it’s a bit too large for me to tackle alone. I’d love for other contributors to jump in and collaborate to help get this over the finish line. Please feel free to push directly to this branch! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue for this PR
Closes #1070
Type of change
What does this PR do?
This renames the internal
bashtool toterminalacross the codebase.Calling the shell tool "bash" was misleading on platforms like Windows where the shell is usually PowerShell or CMD. This updates the tool IDs, exported classes (
BashTool->TerminalTool), and UI labels to be shell-agnostic.(Note: Because this is a core tool rename, this PR unavoidably touches a large number of files—over 90—but the vast majority of these changes are simple 1-to-1 string and import renames).
I also added a backward compatibility layer in
packages/core/src/v1/config/permission.tsandnext.ts. This remaps the legacybashkey toterminalduring config parsing, so existing users'altimate-code.jsonpermission rules won't break. If a user happens to define bothbashandterminalin their config,terminaltakes precedence.How did you verify your code works?
I ran
bun run typecheckandbun testacross the monorepo to ensure everything still builds and all snapshot/permission tests pass. I also verified that the module exports (likeTerminalTool.layer) resolve correctly without circular dependencies.Screenshots / recordings
N/A
Checklist
Summary by cubic
Renamed the internal
bashtool toterminalfor cross‑platform accuracy and clearer UX. Adds a compatibility layer that mapsbashpermissions toterminalso existing configs keep working (closes #1070).Refactors
bash->terminal(BashTool->TerminalTool), registry defaults and UI labels updated.bashtoterminal; if both exist,terminalwins. Tests, telemetry, retrieval lists, and viewers updated.terminaland accept legacybashwhere needed.Migration
altimate-code.json;bashcontinues to work.TerminalTooland use theterminalpermission key.Written for commit 5861452. Summary will update on new commits.
Summary by CodeRabbit