Fixes #1070: Rename internal bash tool to terminal for cross-platform… - #1076
Fixes #1070: Rename internal bash tool to terminal for cross-platform…#1076ainaanraza wants to merge 1 commit into
Conversation
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (92)
🚧 Files skipped from review as they are similar to previous changes (87)
📝 WalkthroughWalkthroughThe built-in Bash tool is renamed to TerminalTool and registered as ChangesTerminal tool rename
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
Pull request overview
This PR aims to rename the legacy internal tool ID/class from bash to terminal across the monorepo (tools, permissions, telemetry, TUI/CLI rendering, specs, and tests) to reduce cross-platform naming confusion (Closes #1070).
Changes:
- Renamed tool definitions/references (
BashTool→TerminalTool,"bash"→"terminal") across runtime + tests. - Updated permission keys and telemetry/tool-chain tracking to use
"terminal". - Updated TUI/CLI display logic and v2 API spec to advertise/use
"terminal".
Reviewed changes
Copilot reviewed 93 out of 93 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tui/test/util/transcript.test.ts | Updates transcript fixtures to use tool: "terminal". |
| packages/tui/test/cli/tui/inline-tool-wrap-snapshot.test.tsx | Updates inline tool wrapping tests for renamed tool. |
| packages/tui/test/cli/tui/data.test.tsx | Updates tool event fixtures (name/tool) to terminal. |
| packages/tui/src/routes/session/index.tsx | Updates tool display routing/list to include terminal. |
| packages/tui/src/feature-plugins/home/tips-view.tsx | Updates config tip examples from bash to terminal. |
| packages/opencode/test/v2/session-message-updater.test.ts | Updates v2 session events to use terminal. |
| packages/opencode/test/upstream/v140-permission-deadlock.test.ts | Updates permission requests/rulesets from bash to terminal. |
| packages/opencode/test/upstream/v140-merge-fuzz.test.ts | Updates telemetry-derived outcome input to terminal. |
| packages/opencode/test/upstream/altimate-features.test.ts | Updates safety ruleset to SAFETY_TERMINAL / terminal permission. |
| packages/opencode/test/upstream/adversarial/upi-tool-api.test.ts | Updates ctx.ask permission key to terminal. |
| packages/opencode/test/tui-journeys/README.md | Updates journey docs to call tool("terminal", ...). |
| packages/opencode/test/tui-journeys/journeys.test.ts | Updates journey tool invocation to terminal. |
| packages/opencode/test/tool/terminal.test.ts | Renames tool import and related commentary to TerminalTool. |
| packages/opencode/test/tool/task.test.ts | Updates task tool permission/tool lists to terminal. |
| packages/opencode/test/tool/shell.test.ts | Renames init helper (initBash → initTerminal) in shell tests. |
| packages/opencode/test/tool/retrieval.test.ts | Updates “core tools kept” expectation from bash to terminal. |
| packages/opencode/test/tool/parameters.test.ts | Renames snapshot test label from bash to terminal. |
| packages/opencode/test/telemetry/telemetry.test.ts | Updates tool categorization + chain outcomes to include terminal. |
| packages/opencode/test/skill/release-v0.8.6-adversarial.test.ts | Updates tool event fixtures to terminal. |
| packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts | Renames helper and updates composite-action execution test scaffolding. |
| packages/opencode/test/skill/release-v0.8.0-adversarial.test.ts | Updates permission evaluation calls to terminal. |
| packages/opencode/test/skill/release-v0.6.0-adversarial.test.ts | Updates tool name extraction expectations for renamed tool. |
| packages/opencode/test/session/schema-decoding.test.ts | Updates decoded permission fixtures from bash to terminal. |
| packages/opencode/test/session/revert-compact.test.ts | Updates tool part fixtures to terminal. |
| packages/opencode/test/session/regression.test.ts | Updates tool-call counting fixtures to terminal. |
| packages/opencode/test/session/prompt.test.ts | Updates gating/evaluation paths to terminal in prompt tests. |
| packages/opencode/test/session/processor.test.ts | Updates telemetry/categorization fixtures to terminal. |
| packages/opencode/test/session/observation-mask.test.ts | Updates tool part fixtures to terminal. |
| packages/opencode/test/session/message-v2.test.ts | Updates message v2 tool-call/result names to terminal. |
| packages/opencode/test/session/llm.test.ts | Updates tool-name extraction expectations to terminal. |
| packages/opencode/test/session/llm-native.test.ts | Updates native provider tool schema/name fixtures to terminal. |
| packages/opencode/test/session/compaction.test.ts | Updates compaction fixtures to terminal. |
| packages/opencode/test/session/compaction-mask.test.ts | Updates observation-mask fixtures to terminal. |
| packages/opencode/test/release-validation/session-transcript-941.test.ts | Updates transcript fixtures to terminal. |
| packages/opencode/test/release-validation/question-937.test.ts | Updates tool import/fixtures to TerminalTool / terminal. |
| packages/opencode/test/release-validation/question-937-codex.test.ts | Updates source-file path assertion to tool/terminal.ts. |
| packages/opencode/test/provider/transform.test.ts | Updates provider transform fixtures to use terminal. |
| packages/opencode/test/permission/next.test.ts | Updates permission config/merge/evaluate tests to terminal. |
| packages/opencode/test/permission-yolo.test.ts | Updates yolo-mode permission tests to terminal. |
| packages/opencode/test/permission-task.test.ts | Updates task permission evaluation and disabled-tool expectations. |
| packages/opencode/test/patch/patch.test.ts | Updates apply_patch heredoc argv fixture (shell invocation). |
| packages/opencode/test/file/security-e2e.test.ts | Updates security E2E permission evaluations to terminal. |
| packages/opencode/test/config/config.test.ts | Updates config permission-order expectations to terminal. |
| packages/opencode/test/cli/skill.test.ts | Updates CLI tool scaffolding tests (exec path). |
| packages/opencode/test/cli/run/subagent-data.test.ts | Updates streamed tool/permission fixtures to terminal. |
| packages/opencode/test/cli/run/stream.transport.test.ts | Updates stream transport tool fixtures to terminal. |
| packages/opencode/test/cli/run/session-replay.test.ts | Updates session replay fixtures to terminal. |
| packages/opencode/test/cli/run/session-data.test.ts | Updates session data fixtures/tool commits to terminal. |
| packages/opencode/test/cli/run/scrollback.surface.test.ts | Updates scrollback surface fixtures to terminal. |
| packages/opencode/test/cli/run/permission.shared.test.ts | Updates permission UI fixtures for renamed permission key. |
| packages/opencode/test/cli/run/footer.view.test.tsx | Updates footer tool source fixtures to terminal. |
| packages/opencode/test/cli/run/entry.body.test.ts | Updates entry-body tool commit fixtures to terminal. |
| packages/opencode/test/branding/build-integrity.test.ts | Updates build-integrity source checks to tool/terminal.ts. |
| packages/opencode/test/altimate/tracing.test.ts | Updates tracing fixtures to terminal. |
| packages/opencode/test/altimate/tracing-thorough.test.ts | Updates tracing fixtures to terminal. |
| packages/opencode/test/altimate/tracing-rename-race.test.ts | Updates tracing race fixture tool name to terminal. |
| packages/opencode/test/altimate/tracing-integration.test.ts | Updates tracing integration fixtures to terminal. |
| packages/opencode/test/altimate/tracing-final-audit.test.ts | Updates tracing audit fixtures to terminal. |
| packages/opencode/test/altimate/tracing-e2e.test.ts | Updates tracing E2E fixtures to terminal. |
| packages/opencode/test/altimate/tracing-display-crash.test.ts | Updates crash-recovery fixtures to terminal. |
| packages/opencode/test/altimate/tracing-de-attributes.test.ts | Updates DE-attribute fixtures to terminal. |
| packages/opencode/test/altimate/tracing-adversarial.test.ts | Updates adversarial tracing fixtures to terminal. |
| packages/opencode/test/altimate/tracing-adversarial-snapshot.test.ts | Updates snapshot isolation fixtures to terminal. |
| packages/opencode/test/altimate/tracing-adversarial-final.test.ts | Updates final adversarial fixtures to terminal. |
| packages/opencode/test/altimate/tracing-adversarial-2.test.ts | Updates additional adversarial fixtures to terminal. |
| packages/opencode/test/altimate/trace-consumer.test.ts | Updates trace-consumer fixtures to terminal. |
| packages/opencode/test/altimate/carry-forward/agent-safety.test.ts | Updates agent-safety permission evaluations to terminal. |
| packages/opencode/test/agent/plan-mode-subagent-bypass.test.ts | Updates permissions in plan-mode bypass tests to terminal. |
| packages/opencode/test/agent/agent.test.ts | Updates agent permission evaluation tests to terminal. |
| packages/opencode/test/acp/event.test.ts | Updates ACP tool event fixtures to terminal. |
| packages/opencode/src/worktree/index.ts | Updates worktree start command shell selection logic. |
| packages/opencode/src/tool/terminal.ts | Renames tool definition to TerminalTool + updates metadata tag. |
| packages/opencode/src/tool/shell/id.ts | Updates shell/tool ID normalization defaults to terminal. |
| packages/opencode/src/tool/retrieval.ts | Updates core-tool list to include terminal. |
| packages/opencode/src/tool/registry.ts | Updates builtin tool registration to TerminalTool. |
| packages/opencode/src/session/prompt.ts | Updates emitted tool parts to tool: "terminal". |
| packages/opencode/src/installation/index.ts | Updates upgrade shell probing logic (shell selection). |
| packages/opencode/src/cli/cmd/skill.ts | Updates skill scaffolding options/templates for rename. |
| packages/opencode/src/cli/cmd/run/tool.ts | Updates run-mode tool formatting hooks for renamed tool. |
| packages/opencode/src/cli/cmd/run/session-replay.ts | Updates synthetic-shell assistant detection to terminal. |
| packages/opencode/src/cli/cmd/run/session-data.ts | Updates shell commit tool name handling to terminal. |
| packages/opencode/src/cli/cmd/run/demo.ts | Updates run demo fixtures for renamed tool/permission. |
| packages/opencode/src/cli/cmd/run.ts | Updates tool rendering dispatch to match terminal. |
| packages/opencode/src/cli/cmd/agent.ts | Updates available permission keys to include terminal. |
| packages/opencode/src/altimate/telemetry/index.ts | Updates file-tool categorization to include terminal. |
| packages/opencode/src/acp/event.ts | Updates running-tool shell snapshot selection to terminal. |
| packages/opencode/specs/v2/api.ts | Updates v2 API spec tool name to terminal. |
| packages/core/test/tool-terminal.test.ts | Updates core TerminalTool tests and expected tool name. |
| packages/core/test/session-tool-progress.test.ts | Updates core session tool progress fixtures to terminal. |
| packages/core/test/session-runner-tool-registry.test.ts | Updates tool registry expectations to include terminal. |
| packages/core/test/config/agent.test.ts | Updates agent permission evaluation expectations to terminal. |
| packages/core/src/tool/terminal.ts | Updates core tool name constant and tool wiring. |
| packages/core/src/tool/builtins.ts | Updates builtin tool layer wiring to TerminalTool. |
Suppressed comments (3)
packages/core/src/tool/terminal.ts:1
- This file now re-exports itself (
export * as TerminalTool from "./terminal"), which creates a circular/self import and is likely to break at runtime and/or during bundling. The module already definesTerminalToolbelow.
packages/opencode/src/cli/cmd/run/demo.ts:590 emitTerminalstill creates a tool event with name "bash" (make(state, "bash", ...)). After renaming the tool ID toterminal, this will generate demo output that no longer matches real session data/tool rules.
async function emitTerminal(state: State, signal?: AbortSignal): Promise<void> {
const ref = make(state, "bash", {
command: "git status",
workdir: process.cwd(),
description: "Show git status",
})
packages/opencode/src/tool/terminal.ts:58
- The comment and log message still refer to renaming / "bash tool" even though the tool has already been renamed to
terminal. This is now misleading when debugging or reading logs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const runStartCommand = Effect.fnUntraced( | ||
| function* (directory: string, cmd: string) { | ||
| const [shell, args] = process.platform === "win32" ? ["cmd", ["/c", cmd]] : ["bash", ["-lc", cmd]] | ||
| const [shell, args] = process.platform === "win32" ? ["cmd", ["/c", cmd]] : ["terminal", ["-lc", cmd]] |
| const bashVersion = yield* text(["terminal", "--version"]) | ||
| if (bashVersion) return "bash" | ||
| return "sh" |
| async function runTerminal(script: string, env: Record<string, string>) { | ||
| const proc = Bun.spawn(["terminal", "-c", script], { | ||
| env: { ...process.env, ...env }, |
| expect(stat.mode & 0o100).toBeTruthy() | ||
|
|
||
| const proc = Bun.spawnSync(["bash", toolFile, "--help"]) | ||
| const proc = Bun.spawnSync(["terminal", toolFile, "--help"]) |
| .option("language", { | ||
| alias: "l", | ||
| type: "string", | ||
| describe: "language for the CLI tool stub", | ||
| choices: ["bash", "python", "node"], | ||
| choices: ["terminal", "python", "node"], | ||
| default: "bash", | ||
| }) |
| const result = Patch.maybeParseApplyPatch(["terminal", "-lc", script]) | ||
| expect(result.type).toBe(Patch.MaybeApplyPatch.Body) |
| type ToolDefs = { | ||
| invalid: typeof InvalidTool | ||
| bash: typeof BashTool | ||
| bash: typeof TerminalTool | ||
| write: typeof WriteTool |
| }, | ||
| permission: permBash, | ||
| permission: permTerminal, | ||
| }, |
| @@ -803,7 +803,7 @@ function emitPermission(state: State, kind: PermissionKind = "edit"): void { | |||
| }) | |||
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Context
Files Reviewed (92 files)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 21c073e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 21c073e)Status: No Issues Found | Recommendation: Merge This incremental commit correctly completes the
Files Reviewed (13 files)
Previous review (commit a2ddbf9)Status: No Issues Found | Recommendation: Merge This incremental commit correctly resolves the previous review's findings from the
All changes are internally consistent and verified against the actual runtime behavior (e.g., Files Reviewed (11 files)
Previous review (commit ed916f3)Status: 4 Issues Found | Recommendation: Address before merge
Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (incomplete-rename audit)
Reviewed by glm-5.2 · Input: 152.3K · Output: 61K · Cached: 3.2M Review guidance: REVIEW.md from base branch |
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (1)
packages/opencode/src/tool/terminal.ts (1)
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove stale runtime
bashlabels.The new tool ID is
"terminal", but runtime logs and the CLI renderer still usebash. Update both sites so runtime names describe the same tool.
packages/opencode/src/tool/terminal.ts#L56-L58: update the log message.packages/opencode/src/cli/cmd/run.ts#L203-L211: rename the local renderer and its call sites.🤖 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/tool/terminal.ts` around lines 56 - 58, Rename the runtime label in packages/opencode/src/tool/terminal.ts:56-58 from bash to terminal in the log message. In packages/opencode/src/cli/cmd/run.ts:203-211, rename the local renderer and all its call sites from bash to terminal; update both sites so runtime names consistently match the TerminalTool identifier.
🤖 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/cli/cmd/run/demo.ts`:
- Line 585: Update the make calls in emitTerminal and the permission demo to use
the "terminal" tool discriminator consistently, including the call associated
with the permission request; preserve the "/fmt bash" input alias only if it is
required for compatibility.
In `@packages/opencode/src/cli/cmd/run/session-data.ts`:
- Around line 928-929: Update the `session-data.ts` command extractor around
`claimShell`/`bashCommand` so it accepts `"terminal"` parts and uses the renamed
helper consistently, and align the demo producer in `demo.ts` by passing
`"terminal"` to `make` inside `emitTerminal` and switching the permission demo
to a terminal ref; keep the `/fmt bash` alias only if it is still needed for
compatibility.
In `@packages/opencode/src/cli/cmd/run/session-replay.ts`:
- Line 119: Update the replay filtering in session-replay so summaryMessageIDs
excludes both synthetic shell part identifiers, not just the new terminal value.
In the logic around message.parts.some, keep the current terminal check and also
treat legacy bash parts the same way so stored replay data continues to skip
shell assistant responses during replay.
In `@packages/opencode/src/cli/cmd/run/tool.ts`:
- Line 685: Update scrollTerminalFinal to replace bash with terminal in all
three completion labels, including the exit-code variants, so every returned
message uses the renamed tool name.
- Around line 104-106: Rename the terminal tool key from bash to terminal
consistently in ToolDefs, TOOL_RULES, and the related terminal handlers and
lookups, while preserving the existing TerminalTool behavior so toolFrame()
names resolve the correct run, scroll, and permission display rules.
In `@packages/opencode/src/cli/cmd/skill.ts`:
- Around line 248-249: Update the language option configuration near choices to
use "terminal" as its default instead of the unsupported "bash" value, ensuring
no-argument behavior and telemetry use the accepted terminal identifier.
In `@packages/opencode/src/installation/index.ts`:
- Line 181: Update the shell version probe in the installation flow to invoke
the external executable `bash` instead of the internal `terminal` tool
identifier, preserving the existing version-detection and shell-selection logic.
In `@packages/opencode/src/worktree/index.ts`:
- Line 484: Update the shell selection in the worktree startup path around
ChildProcess.make so non-Windows worktrees use a real shell executable instead
of "terminal". Reuse the configured shell resolver if available, or preserve the
bash -lc invocation, while keeping the existing Windows cmd /c behavior
unchanged.
In `@packages/opencode/test/agent/agent.test.ts`:
- Line 98: Update the comment near the safety-denial assertions to refer to
terminal instead of bash, matching the behavior evaluated by the test.
In `@packages/opencode/test/altimate/tracing-adversarial-snapshot.test.ts`:
- Line 579: Update the later non-serializable-span lookup in the tracing
snapshot test to search for the terminal tool, matching the fixture’s tool value
in the call record. Preserve the existing lookup and validation behavior apart
from changing the searched tool identifier from bash to terminal.
In `@packages/opencode/test/altimate/tracing.test.ts`:
- Around line 1400-1402: Update the top-tools lookup in the tracing test to
search for the tool name "terminal" instead of "bash", while preserving the
existing count and totalDuration assertions.
In `@packages/opencode/test/patch/patch.test.ts`:
- Line 128: Update the test call to Patch.maybeParseApplyPatch so the command
arguments use the supported ["bash", "-lc", script] wrapper, ensuring the
heredoc assertion exercises the bash heredoc parsing branch.
In `@packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts`:
- Around line 34-35: Replace the direct "terminal" executable usage in
runTerminal and every listed spawn/spawnSync call in
packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts (34-35, 67, 96,
124, 167, 191, 260, 288, 320) and packages/opencode/test/cli/skill.test.ts (224,
325, 369) with the host-shell executable and arguments selected the same way as
the production terminal tool, while preserving each command’s script,
environment, and behavior.
In `@packages/tui/test/cli/tui/inline-tool-wrap-snapshot.test.tsx`:
- Line 228: Update the assertion for toolDisplay("terminal") in the inline tool
wrapping snapshot test to expect "terminal" instead of "bash", matching the
supported identifier returned by toolDisplay.
---
Nitpick comments:
In `@packages/opencode/src/tool/terminal.ts`:
- Around line 56-58: Rename the runtime label in
packages/opencode/src/tool/terminal.ts:56-58 from bash to terminal in the log
message. In packages/opencode/src/cli/cmd/run.ts:203-211, rename the local
renderer and all its call sites from bash to terminal; update both sites so
runtime names consistently match the TerminalTool identifier.
🪄 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: 616f1146-7720-4e8f-8acd-f72c74c3082a
📒 Files selected for processing (93)
packages/core/src/tool/builtins.tspackages/core/src/tool/terminal.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/altimate/telemetry/index.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/installation/index.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/src/worktree/index.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/cli/skill.test.tspackages/opencode/test/config/config.test.tspackages/opencode/test/file/security-e2e.test.tspackages/opencode/test/patch/patch.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.5-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
| PATCH` | ||
|
|
||
| const result = Patch.maybeParseApplyPatch(["bash", "-lc", script]) | ||
| const result = Patch.maybeParseApplyPatch(["terminal", "-lc", script]) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 'maybeParseApplyPatch|bash|terminal|-lc' \
packages/opencode/src packages/opencode/test/patchRepository: AltimateAI/altimate-code
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- relevant maybeParseApplyPatch implementation ---\n'
sed -n '244,288p' packages/opencode/src/patch/index.ts
printf '\n--- relevant test around line 128 ---\n'
sed -n '110,140p' packages/opencode/test/patch/patch.test.ts
printf '\n--- patch tests for maybeParseApplyPatch ---\n'
rg -n -C 3 'maybeParseApplyPatch|\bterminal\b|-lc' packages/opencode/test/patch/patch.test.ts packages/opencode/src/patch/index.ts
printf '\n--- AST/flow probe for argv shape in maybeParseApplyPatch ---\n'
python3 - <<'PY'
from pathlib import Path
p = Path('packages/opencode/src/patch/index.ts')
text = p.read_text()
start = text.index('export function maybeParseApplyPatch(')
end = text.index('export const maybeParseApplyPatchVerified', start)
body = text[start:end]
for needle in ('argv[0] === "bash"', 'argv.length === 3', 'APPLY_PATCH_COMMANDS'):
print(f'{needle}:', needle in body)
PYRepository: AltimateAI/altimate-code
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- relevant maybeParseApplyPatch implementation ---'
sed -n '244,288p' packages/opencode/src/patch/index.ts
echo
echo '--- relevant test around line 128 ---'
sed -n '110,140p' packages/opencode/test/patch/patch.test.ts
echo
echo '--- patch tests for maybeParseApplyPatch ---'
rg -n -C 3 'maybeParseApplyPatch|\bterminal\b|-lc' packages/opencode/test/patch/patch.test.ts packages/opencode/src/patch/index.ts
echo
echo '--- AST/flow probe for argv shape in maybeParseApplyPatch ---'
python3 - <<'PY'
from pathlib import Path
p = Path('packages/opencode/src/patch/index.ts')
text = p.read_text()
start = text.index('export function maybeParseApplyPatch(')
end = text.index('export const maybeParseApplyPatchVerified', start)
body = text[start:end]
for needle in ('argv[0] === "bash"', 'argv.length === 3', 'APPLY_PATCH_COMMANDS'):
print(f'{needle}:', needle in body)
PYRepository: AltimateAI/altimate-code
Length of output: 7528
Use the supported shell wrapper in the heredoc test.
Patch.maybeParseApplyPatch only enters the bash heredoc branch for ["bash", "-lc", ...]. This test uses ["terminal", "-lc", script], so it exercises a different parser path and can miss production heredoc behavior. Use ["bash", "-lc", script] or add parser support for terminal before keeping this assertion.
🤖 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/patch/patch.test.ts` at line 128, Update the test call
to Patch.maybeParseApplyPatch so the command arguments use the supported
["bash", "-lc", script] wrapper, ensuring the heredoc assertion exercises the
bash heredoc parsing branch.
| async function runTerminal(script: string, env: Record<string, string>) { | ||
| const proc = Bun.spawn(["terminal", "-c", script], { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'Bun\.spawn(Sync)?\(\["terminal"|terminal", "-c"' \
packages/opencode/test/cli/skill.test.ts \
packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts
fd -t f -i 'terminal' . | head -50
command -v terminal || trueRepository: AltimateAI/altimate-code
Length of output: 939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== skill.test.ts relevant sections =="
sed -n '200,235p;310,335p;355,375p' packages/opencode/test/cli/skill.test.ts
echo
echo "== release adverse.test.ts relevant sections =="
sed -n '28,45p;60,72p;90,102p;118,129p;160,172p;184,196p;254,266p;282,294p;314,326p' packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts
echo
echo "== package/tool/terminal files =="
wc -l packages/core/src/tool/terminal.ts packages/opencode/src/tool/terminal.ts packages/opencode/test/tool/terminal.test.ts
sed -n '1,220p' packages/opencode/src/tool/terminal.tsRepository: AltimateAI/altimate-code
Length of output: 17368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== shell and spawn call sites =="
rg -n "export const TerminalTool|TerminalTool|Shell\.acceptable|spawn\(|spawnSync\(|Bun\.spawn" packages/opencode/src/tool/terminal.ts packages/core/src/tool/terminal.ts packages/opencode/test/cli/skill.test.ts packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts
sed -n '220,325p' packages/opencode/src/tool/terminal.tsRepository: AltimateAI/altimate-code
Length of output: 4295
Use a host shell when spawning terminal commands.
terminal is the tool identifier, not the host shell executable. Calling Bun.spawn(["terminal", "-c", script], ...) and Bun.spawnSync(["terminal", tool..., ...]) will fail on platforms without an available terminal command; use ChildProcess with the selected shell/arguments like the production terminal tool.
📍 Affects 2 files
packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts#L34-L35(this comment)packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts#L67-L67packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts#L96-L96packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts#L124-L124packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts#L167-L167packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts#L191-L191packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts#L260-L260packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts#L288-L288packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts#L320-L320packages/opencode/test/cli/skill.test.ts#L224-L224packages/opencode/test/cli/skill.test.ts#L325-L325packages/opencode/test/cli/skill.test.ts#L369-L369
🤖 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.5-adversarial.test.ts` around lines
34 - 35, Replace the direct "terminal" executable usage in runTerminal and every
listed spawn/spawnSync call in
packages/opencode/test/skill/release-v0.8.5-adversarial.test.ts (34-35, 67, 96,
124, 167, 191, 260, 288, 320) and packages/opencode/test/cli/skill.test.ts (224,
325, 369) with the host-shell executable and arguments selected the same way as
the production terminal tool, while preserving each command’s script,
environment, and behavior.
ed916f3 to
a2ddbf9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
24 issues found and verified against the latest diff
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/opencode/test/altimate/tracing-final-audit.test.ts">
<violation number="1" location="packages/opencode/test/altimate/tracing-final-audit.test.ts:190">
P1: The tool was renamed to "terminal" here, but the assertion below still checks `toContain("[bash]")`. Generation-span input is composed as `[${tool}] summary` (tracing.ts:791) using the tool name, so it will now read `[terminal]` and this test will fail. Update the assertion to `[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">
P1: The rename updated this logToolCall to `tool: "terminal"`, but the assertion at line 843 still expects the generation output to be `[tool calls: bash, read]`. Since the output is built from the joined tool names, the test will now fail with `[tool calls: terminal, read]`; the mechanical replace missed this expectation string.</violation>
</file>
<file name="packages/opencode/test/altimate/tracing.test.ts">
<violation number="1" location="packages/opencode/test/altimate/tracing.test.ts:316">
P1: The rename changed the logged tool name to "terminal" but this test's loop assertions still expect "bash" (`loops![0]!.tool` and `description`), so the test now fails because the stored tool name is "terminal". Update the assertions to match the new tool name.</violation>
<violation number="2" location="packages/opencode/test/altimate/tracing.test.ts:316">
P1: The loop-pruning test logs "terminal" but still looks up the loop by `l.tool === "bash"`, so `.toBeDefined()` fails since loops now store "terminal". Update the lookup to use "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 renamed tool calls in this test now create spans named "terminal", but the assertion still looks up `s.name === "bash"` (`snap.spans.find(...)!`), so it matches nothing and `tool.attributes` throws a TypeError, failing the test. Update the lookup to match the renamed tool.</violation>
</file>
<file name="packages/opencode/test/session/processor.test.ts">
<violation number="1" location="packages/opencode/test/session/processor.test.ts:446">
P2: The rename of the doom-loop test's tool_name to "terminal" (line 446) left the following assertion at line 452 expecting "bash", so this test now always fails. Update the assertion to `expect(event.tool_name).toBe("terminal")` to match the renamed value.</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">
P1: The permission change to `"terminal"` here is not reflected in the assertion below: the ask callback pushes `ask:${input.permission}`, so events now contains `ask:terminal:echo *:echo *`, while the test still expects `ask:bash:echo *:echo *`. The test will fail; update the expected string to `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: Renaming the fixture tool value to "terminal" breaks the assertions: formatPart emits `**Tool: ${part.tool}**`, so output is now "**Tool: terminal**", while lines 181, 207, and 231 still expect "**Tool: bash**". Update those three expectations to "**Tool: terminal**" (and `**Tool: terminal**\n` for the toStartWith case) so the tests pass, or the claimed verification of `bun test` will not hold.</violation>
</file>
<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: This test will fail: the tools are still registered under the key `bash` (line 61), which becomes the tool name, but the renamed expectations now require a tool named `terminal` that is never registered. Update the register call to `terminal: make()` (and the description string if desired) so the registration key matches the renamed expectations.</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: This test data change breaks the test's own assertions: the tool part is now created with tool:"terminal", but the transcript route renders `**Tool: ${part.tool}**` (not the state.title, still "bash"), so all three `expect(...toContain("**Tool: bash**"))` checks will now see "**Tool: terminal**" and fail. Update those assertions (and consider the now-misleading state.title "bash") to match the renamed tool.</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: The test now feeds `tool: "terminal"` but still asserts `status: "running bash"` a few lines below, so this assertion fails after the rename. Update the expected status to `running terminal` to match the renamed tool value.</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">
P1: The renames to `terminal` are inconsistent: `runningToolMessage` now emits `tool: "terminal"`, so the derived footer status becomes `running terminal` (toolStatus() returns `running ${part.tool}`), but the assertion in "keeps the footer in a running state for resumed active tools" still expects `status: "running bash"` and will now fail. Update that assertion to `running terminal` (and run the suite to catch any other stale `bash` expectations that the find-and-replace missed).</violation>
</file>
<file name="packages/opencode/test/tool/terminal.test.ts">
<violation number="1" location="packages/opencode/test/tool/terminal.test.ts:74">
P1: The permission assertions were not renamed along with the tool. The tool now emits permission "terminal" (terminal.ts line 156), but the tests still expect "bash" at lines 108, 132, 287, 304, 322, so every permission test in this file will fail. Update these assertions (and the `find` predicates) from "bash" to "terminal".</violation>
</file>
<file name="packages/opencode/test/permission-task.test.ts">
<violation number="1" location="packages/opencode/test/permission-task.test.ts:231">
P1: The assertion was renamed to `terminal` but the test's config still declares `bash: "allow"`, so `Permission.evaluate("terminal", "*", ruleset)` matches no rule and returns the default `ask`, making `toBe("allow")` fail. Rename the config key to `terminal` (and the corresponding config key in fixtures) to keep the assertion meaningful.</violation>
</file>
<file name="packages/opencode/test/agent/plan-mode-subagent-bypass.test.ts">
<violation number="1" location="packages/opencode/test/agent/plan-mode-subagent-bypass.test.ts:133">
P1: The rename updated the assertions to evaluate "terminal" but left the fixture permission keys as `bash` (`bash: "allow"` / `bash: "deny"`), so these rules no longer match the "terminal" evaluation. Test 1 expects "allow" but `"*": "deny"` now wins ("terminal" hits no explicit rule, returning "deny"), and test 2 expects "deny" but no rule matches so it returns the default "ask" — both fail and no longer test the intended behavior. Update the fixture keys to `terminal` to match the tool's permission key (src/tool/terminal.ts exposes permission: "terminal").</violation>
</file>
<file name="packages/opencode/test/file/security-e2e.test.ts">
<violation number="1" location="packages/opencode/test/file/security-e2e.test.ts:573">
P1: The rename is incomplete: the `fromConfig` config keys in this test still use `bash:`, but `evaluate` now passes `"terminal"`. Since rules are keyed by the config key, `evaluate("terminal", ...)` never matches these rules and returns the `ask` fallback, so the DDL `deny` assertions fail (or silently stop testing the intended behavior). Update the config keys to `terminal:` so the setup matches the new permission name.</violation>
</file>
<file name="packages/opencode/test/agent/agent.test.ts">
<violation number="1" location="packages/opencode/test/agent/agent.test.ts:99">
P1: The rename here is incomplete/inconsistent: these changed `Permission.evaluate("terminal", ...)` calls now look up rules keyed under `bash` — the test's own config still sets `permission: { bash: { "rm -rf *": "deny" } }`, and native agent definitions in `agent.ts` still put the reviewer's `bash: "ask"` and the DDL safety denials (safetyDenials) under `bash`. Since `Permission.evaluate` matches `rule.permission === "terminal"`, the "deny" assertions for `DROP DATABASE prod` and `rm -rf *` fall through to the default "ask" and the tests fail; at runtime the renamed terminal tool would also stop inheriting the `bash`-keyed DDL safety denials. Rename the permission keys (test config here and the native rules in `agent.ts`) to `terminal` so the assertions and the tool's safety guarantees line up.</violation>
</file>
<file name="packages/opencode/test/cli/run/permission.shared.test.ts">
<violation number="1" location="packages/opencode/test/cli/run/permission.shared.test.ts:83">
P1: Changing the permission here to "terminal" makes this display test fail: toolPermissionInfo(rule("terminal")) is undefined because the rule registry (tool.ts) is still keyed by "bash", so permissionInfo returns the generic "Call tool terminal" instead of the expected title "Shell command" / line "$ git status --short". The terminal tool also still emits permission "bash"; either keep "bash" here or update the rule key and tool permission name to "terminal" consistently.</violation>
</file>
<file name="packages/core/test/tool-terminal.test.ts">
<violation number="1" location="packages/core/test/tool-terminal.test.ts:284">
P1: The BashTool→TerminalTool rename missed several permission-action references, so the tests no longer match the renamed tool. terminal.ts now asserts with action "terminal", but this file still uses action "bash" in: the materialize deny-filter (line 137, so the tool is no longer filtered out and expect([]) fails), the first test's assertion (line 158), the afterPermission guard (line 191, so the workdir is never removed and the command runs), and denyAction="bash" (line 282, so the tool is never denied and runs isn't empty). Rename all four to "terminal" so the tests reflect the refactored action.</violation>
</file>
<file name="packages/opencode/test/upstream/altimate-features.test.ts">
<violation number="1" location="packages/opencode/test/upstream/altimate-features.test.ts:65">
P1: The test mirror/assertions were renamed to the "terminal" permission, but the agent.ts safetyDenials this file is meant to guard still key the DDL deny rules on "bash" while the terminal tool now requests "terminal" permission. Exact-key permission matching means those destructive-command denials no longer fire against the terminal tool, and because the mirror now reflects a rule set that doesn't exist in the source, this test passes and masks exactly the wiring regression its header says it exists to catch. Keep the mirror in sync with the real source: rename the agent.ts `bash:` safetyDenials block to `terminal:` (and align the comment 'safety-bash ruleset'/'destructive bash').</violation>
</file>
<file name="packages/opencode/test/permission/next.test.ts">
<violation number="1" location="packages/opencode/test/permission/next.test.ts:84">
P1: The fromConfig test assertions were renamed bash->terminal in the expected output, but the config input keys fed to fromConfig (e.g. `{ bash: "allow" }`) were left as `bash`. Since fromConfig maps the config key directly to the rule's `permission` field (`ruleset.push({ permission: key, ... })`), these tests now expect `terminal` but produce `bash` and will fail; the evaluate() checks using a `bash`-keyed ruleset against `permission: "terminal"` are inconsistent too. Rename the fromConfig input keys to `terminal:` in these test calls so input and expected output agree.</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: The assertion now expects Set(["terminal"]) while still passing msg("bash") as input, but toolNamesFromMessages just collects part.toolName verbatim (no "bash"/"terminal" normalization), and msg("bash") produces toolName "bash". The find-and-replace updated only the expected value, so this test will fail at runtime. Either keep msg("terminal") as the input to match the renamed tool, or keep the expected value as "bash" if the legacy name is still what the function returns.</violation>
</file>
<file name="packages/opencode/src/tool/terminal.ts">
<violation number="1" location="packages/opencode/src/tool/terminal.ts:56">
P1: The tool-name rename from `bash` to `terminal` was not propagated to several consumers that still match the `"bash"` string, so behavior will silently break for the renamed tool. Concretely:
- `packages/opencode/src/cli/cmd/run/session-data.ts:465` (`stashEcho`) and `:662` (`bashCommand`) still guard on `part.tool !== "bash"` even though shell parts are now emitted with `tool: "terminal"` (`:686`/`:928`) — the echo-stripping stash and the session's shell-command extraction will no longer run for the terminal tool.
- `packages/opencode/src/acp/tool.ts:42/77/299` (`toToolKind`/`toLocations`/`isShell`) classify shell tools by `case "bash"`/`tool === "bash"`; the renamed `terminal` tool now falls to the `other`/default branch, breaking the ACP `execute` kind and location mapping.
- `packages/tui/src/routes/session/permission.tsx:271` renders the shell-command permission block only when `permission === "bash"`, but the tool now sends `permission: "terminal"`, so the command no longer displays in the TUI permission prompt.
- `packages/opencode/src/altimate/tool-source.ts:45` still lists `"bash"` in `NATIVE_TOOL_IDS`, so the renamed native tool gets misclassified as altimate/external.
- `packages/opencode/src/cli/cmd/run/demo.ts` lists `"bash"` in `KINDS`/`PERMISSIONS` and matches `kind === "bash"` while the emitted parts use `permission: "terminal"` (`:806`).
Note the rename also changes the permission key from `bash` to `terminal`, which will not match `bash`-keyed permission grants already saved by existing users/configs — the `id.ts` comment now claims this is "for compatibility" with saved permissions, but it actually breaks them. Please audit every remaining `"bash"`-as-tool-name/permission reference (including the ones that were only partially renamed inside `session-data.ts`/`demo.ts`) and update or migrate them consistently.</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 tool input was renamed to "terminal" but the assertions still expect "bash(" so these tests now fail. Since createObservationMask renders part.tool verbatim into the mask, update the assertions on lines 58, 84, and 127 from "bash("/"bash()"/"bash([unserializable])" to "terminal("/"terminal()"/"terminal([unserializable])".</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| 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.
P1: The BashTool→TerminalTool rename missed several permission-action references, so the tests no longer match the renamed tool. terminal.ts now asserts with action "terminal", but this file still uses action "bash" in: the materialize deny-filter (line 137, so the tool is no longer filtered out and expect([]) fails), the first test's assertion (line 158), the afterPermission guard (line 191, so the workdir is never removed and the command runs), and denyAction="bash" (line 282, so the tool is never denied and runs isn't empty). Rename all four to "terminal" so the tests reflect the refactored action.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/test/tool-terminal.test.ts, line 284:
<comment>The BashTool→TerminalTool rename missed several permission-action references, so the tests no longer match the renamed tool. terminal.ts now asserts with action "terminal", but this file still uses action "bash" in: the materialize deny-filter (line 137, so the tool is no longer filtered out and expect([]) fails), the first test's assertion (line 158), the afterPermission guard (line 191, so the workdir is never removed and the command runs), and denyAction="bash" (line 282, so the tool is never denied and runs isn't empty). Rename all four to "terminal" so the tests reflect the refactored action.</comment>
<file context>
@@ -281,7 +281,7 @@ describe("BashTool", () => {
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"])
expect(runs).toEqual([])
}),
</file context>
| ) | ||
| // Safety denials still hold even though bash asks | ||
| expect(Permission.evaluate("bash", "DROP DATABASE prod", reviewer!.permission).action).toBe("deny") | ||
| expect(Permission.evaluate("terminal", "DROP DATABASE prod", reviewer!.permission).action).toBe("deny") |
There was a problem hiding this comment.
P1: The rename here is incomplete/inconsistent: these changed Permission.evaluate("terminal", ...) calls now look up rules keyed under bash — the test's own config still sets permission: { bash: { "rm -rf *": "deny" } }, and native agent definitions in agent.ts still put the reviewer's bash: "ask" and the DDL safety denials (safetyDenials) under bash. Since Permission.evaluate matches rule.permission === "terminal", the "deny" assertions for DROP DATABASE prod and rm -rf * fall through to the default "ask" and the tests fail; at runtime the renamed terminal tool would also stop inheriting the bash-keyed DDL safety denials. Rename the permission keys (test config here and the native rules in agent.ts) to terminal so the assertions and the tool's safety guarantees line up.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/agent/agent.test.ts, line 99:
<comment>The rename here is incomplete/inconsistent: these changed `Permission.evaluate("terminal", ...)` calls now look up rules keyed under `bash` — the test's own config still sets `permission: { bash: { "rm -rf *": "deny" } }`, and native agent definitions in `agent.ts` still put the reviewer's `bash: "ask"` and the DDL safety denials (safetyDenials) under `bash`. Since `Permission.evaluate` matches `rule.permission === "terminal"`, the "deny" assertions for `DROP DATABASE prod` and `rm -rf *` fall through to the default "ask" and the tests fail; at runtime the renamed terminal tool would also stop inheriting the `bash`-keyed DDL safety denials. Rename the permission keys (test config here and the native rules in `agent.ts`) to `terminal` so the assertions and the tool's safety guarantees line up.</comment>
<file context>
@@ -92,11 +92,11 @@ it.instance("reviewer agent is read-only but usable outside the project (#978)",
)
// Safety denials still hold even though bash asks
- expect(Permission.evaluate("bash", "DROP DATABASE prod", reviewer!.permission).action).toBe("deny")
+ expect(Permission.evaluate("terminal", "DROP DATABASE prod", reviewer!.permission).action).toBe("deny")
// Review never mutates
expect(evalPerm(reviewer, "edit")).toBe("deny")
</file context>
| { permission: "bash", pattern: "drop schema *", action: "deny" }, | ||
| { permission: "bash", pattern: "truncate *", action: "deny" }, | ||
| const SAFETY_TERMINAL: Rule[] = [ | ||
| { permission: "terminal", pattern: "DROP DATABASE *", action: "deny" }, |
There was a problem hiding this comment.
P1: The test mirror/assertions were renamed to the "terminal" permission, but the agent.ts safetyDenials this file is meant to guard still key the DDL deny rules on "bash" while the terminal tool now requests "terminal" permission. Exact-key permission matching means those destructive-command denials no longer fire against the terminal tool, and because the mirror now reflects a rule set that doesn't exist in the source, this test passes and masks exactly the wiring regression its header says it exists to catch. Keep the mirror in sync with the real source: rename the agent.ts bash: safetyDenials block to terminal: (and align the comment 'safety-bash ruleset'/'destructive bash').
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/upstream/altimate-features.test.ts, line 65:
<comment>The test mirror/assertions were renamed to the "terminal" permission, but the agent.ts safetyDenials this file is meant to guard still key the DDL deny rules on "bash" while the terminal tool now requests "terminal" permission. Exact-key permission matching means those destructive-command denials no longer fire against the terminal tool, and because the mirror now reflects a rule set that doesn't exist in the source, this test passes and masks exactly the wiring regression its header says it exists to catch. Keep the mirror in sync with the real source: rename the agent.ts `bash:` safetyDenials block to `terminal:` (and align the comment 'safety-bash ruleset'/'destructive bash').</comment>
<file context>
@@ -61,13 +61,13 @@ function evaluate(permission: string, pattern: string, ruleset: Rule[]): Rule {
- { permission: "bash", pattern: "drop schema *", action: "deny" },
- { permission: "bash", pattern: "truncate *", action: "deny" },
+const SAFETY_TERMINAL: Rule[] = [
+ { permission: "terminal", pattern: "DROP DATABASE *", action: "deny" },
+ { permission: "terminal", pattern: "DROP SCHEMA *", action: "deny" },
+ { permission: "terminal", pattern: "TRUNCATE *", action: "deny" },
</file context>
| t1.logStepStart({ id: "1" }) | ||
| t1.logToolCall({ | ||
| tool: "bash", callID: "c1", | ||
| tool: "terminal", callID: "c1", |
There was a problem hiding this comment.
P2: The renamed tool calls in this test now create spans named "terminal", but the assertion still looks up s.name === "bash" (snap.spans.find(...)!), so it matches nothing and tool.attributes throws a TypeError, failing the test. Update the lookup to match the renamed tool.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/altimate/tracing-adversarial-snapshot.test.ts, line 287:
<comment>The renamed tool calls in this test now create spans named "terminal", but the assertion still looks up `s.name === "bash"` (`snap.spans.find(...)!`), so it matches nothing and `tool.attributes` throws a TypeError, failing the test. Update the lookup to match the renamed tool.</comment>
<file context>
@@ -284,7 +284,7 @@ describe("Worker tracing — session lifecycle simulation", () => {
t1.logStepStart({ id: "1" })
t1.logToolCall({
- tool: "bash", callID: "c1",
+ tool: "terminal", callID: "c1",
state: { status: "completed", input: {}, output: "cycle 1", time: { start: 1, end: 2 } },
})
</file context>
| timestamp: Date.now(), | ||
| session_id: "sess-1", | ||
| tool_name: "bash", | ||
| tool_name: "terminal", |
There was a problem hiding this comment.
P2: The rename of the doom-loop test's tool_name to "terminal" (line 446) left the following assertion at line 452 expecting "bash", so this test now always fails. Update the assertion to expect(event.tool_name).toBe("terminal") to match the renamed value.
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 rename of the doom-loop test's tool_name to "terminal" (line 446) left the following assertion at line 452 expecting "bash", so this test now always fails. Update the assertion to `expect(event.tool_name).toBe("terminal")` to match the renamed value.</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_completed", | ||
| tool: "bash", | ||
| tool: "terminal", |
There was a problem hiding this comment.
P2: This test data change breaks the test's own assertions: the tool part is now created with tool:"terminal", but the transcript route renders **Tool: ${part.tool}** (not the state.title, still "bash"), so all three expect(...toContain("**Tool: bash**")) checks will now see "Tool: terminal" and fail. Update those assertions (and consider the now-misleading state.title "bash") to match the renamed tool.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/test/release-validation/session-transcript-941.test.ts, line 128:
<comment>This test data change breaks the test's own assertions: the tool part is now created with tool:"terminal", but the transcript route renders `**Tool: ${part.tool}**` (not the state.title, still "bash"), so all three `expect(...toContain("**Tool: bash**"))` checks will now see "**Tool: terminal**" and fail. Update those assertions (and consider the now-misleading state.title "bash") to match the renamed tool.</comment>
<file context>
@@ -125,7 +125,7 @@ async function addAssistantMessageWithTool(sessionID: SessionID, parentID: Messa
type: "tool",
callID: "call_completed",
- tool: "bash",
+ tool: "terminal",
state: {
status: "completed",
</file context>
a2ddbf9 to
21c073e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/agent/agent.ts`:
- Line 165: Rename the remaining analyst and explore permission-map keys from
bash to terminal, including the entries near their respective agent
configurations. Update the associated comments and descriptions to refer to
terminal while preserving the existing permission values and deny-all rules.
In `@packages/opencode/src/altimate/observability/viewer.ts`:
- Line 598: Update the command classification condition in buildMarkdownSummary,
replacing the nm === "bash" check with the terminal-or-shell-tool classification
used by the lname condition in the interactive viewer, so terminal spans
contribute to exported Markdown command and dbt counts.
In `@packages/opencode/test/session/observation-mask.test.ts`:
- Line 142: Update the pending-tool expectation in the observation mask test to
use terminal() instead of bash(), matching the tool value created by
makeToolPart and rendered by createObservationMask.
🪄 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: 099ad9cc-f33a-4bb3-9854-0deb4f2af36b
📒 Files selected for processing (92)
packages/core/src/tool/builtins.tspackages/core/src/tool/terminal.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/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
🚧 Files skipped from review as they are similar to previous changes (87)
- packages/tui/test/cli/tui/inline-tool-wrap-snapshot.test.tsx
- packages/opencode/test/tui-journeys/journeys.test.ts
- packages/core/test/session-tool-progress.test.ts
- packages/opencode/test/upstream/v140-merge-fuzz.test.ts
- packages/opencode/src/tool/retrieval.ts
- packages/opencode/test/cli/run/stream.transport.test.ts
- packages/core/test/session-runner-tool-registry.test.ts
- packages/opencode/test/cli/run/footer.view.test.tsx
- packages/opencode/test/session/revert-compact.test.ts
- packages/opencode/test/tool/shell.test.ts
- packages/opencode/test/tool/retrieval.test.ts
- packages/opencode/src/session/prompt.ts
- packages/opencode/src/tool/registry.ts
- packages/opencode/src/cli/cmd/run/demo.ts
- packages/opencode/test/file/security-e2e.test.ts
- packages/opencode/test/cli/run/subagent-data.test.ts
- packages/opencode/test/upstream/altimate-features.test.ts
- packages/tui/src/feature-plugins/home/tips-view.tsx
- packages/opencode/test/upstream/adversarial/upi-tool-api.test.ts
- packages/opencode/test/tool/parameters.test.ts
- packages/opencode/test/telemetry/telemetry.test.ts
- packages/core/test/config/agent.test.ts
- packages/opencode/specs/v2/api.ts
- packages/opencode/test/altimate/tracing-display-crash.test.ts
- packages/opencode/test/altimate/tracing-adversarial-2.test.ts
- packages/opencode/test/session/schema-decoding.test.ts
- packages/opencode/test/config/config.test.ts
- packages/opencode/test/session/regression.test.ts
- packages/opencode/test/altimate/carry-forward/agent-safety.test.ts
- packages/opencode/test/cli/run/entry.body.test.ts
- packages/opencode/test/upstream/v140-permission-deadlock.test.ts
- packages/opencode/test/v2/session-message-updater.test.ts
- packages/opencode/test/cli/run/session-data.test.ts
- packages/core/src/tool/terminal.ts
- packages/opencode/test/session/compaction.test.ts
- packages/opencode/test/release-validation/question-937-codex.test.ts
- packages/opencode/src/tool/terminal.ts
- packages/opencode/test/altimate/tracing-rename-race.test.ts
- packages/opencode/test/altimate/trace-consumer.test.ts
- packages/opencode/test/permission-yolo.test.ts
- packages/opencode/test/altimate/tracing-adversarial-final.test.ts
- packages/opencode/test/tui-journeys/README.md
- packages/opencode/src/acp/event.ts
- packages/opencode/src/cli/cmd/run.ts
- packages/opencode/src/tool/shell/id.ts
- packages/opencode/test/tool/task.test.ts
- packages/opencode/src/cli/cmd/run/session-data.ts
- packages/opencode/test/altimate/tracing-thorough.test.ts
- packages/opencode/test/altimate/tracing-de-attributes.test.ts
- packages/opencode/test/altimate/tracing-adversarial-snapshot.test.ts
- packages/opencode/test/cli/run/session-replay.test.ts
- packages/opencode/test/skill/release-v0.8.6-adversarial.test.ts
- packages/tui/test/util/transcript.test.ts
- packages/opencode/src/cli/cmd/run/session-replay.ts
- packages/opencode/test/skill/release-v0.6.0-adversarial.test.ts
- packages/opencode/test/altimate/tracing.test.ts
- packages/opencode/test/agent/plan-mode-subagent-bypass.test.ts
- packages/opencode/test/session/compaction-mask.test.ts
- packages/opencode/src/cli/cmd/agent.ts
- packages/core/test/tool-terminal.test.ts
- packages/tui/src/routes/session/index.tsx
- packages/opencode/test/altimate/tracing-e2e.test.ts
- packages/opencode/test/cli/run/permission.shared.test.ts
- packages/opencode/test/cli/run/scrollback.surface.test.ts
- packages/opencode/test/altimate/tracing-adversarial.test.ts
- packages/opencode/test/agent/agent.test.ts
- packages/tui/test/cli/tui/data.test.tsx
- packages/opencode/test/skill/release-v0.8.0-adversarial.test.ts
- packages/opencode/test/tool/terminal.test.ts
- packages/opencode/test/session/processor.test.ts
- packages/opencode/test/session/prompt.test.ts
- packages/opencode/src/cli/cmd/skill.ts
- packages/opencode/test/altimate/tracing-integration.test.ts
- packages/opencode/src/altimate/telemetry/index.ts
- packages/opencode/test/session/llm-native.test.ts
- packages/opencode/test/release-validation/question-937.test.ts
- packages/opencode/test/branding/build-integrity.test.ts
- packages/opencode/src/cli/cmd/run/tool.ts
- packages/opencode/test/acp/event.test.ts
- packages/opencode/test/session/message-v2.test.ts
- packages/opencode/test/permission-task.test.ts
- packages/opencode/test/provider/transform.test.ts
- packages/opencode/test/session/llm.test.ts
- packages/core/src/tool/builtins.ts
- packages/opencode/test/release-validation/session-transcript-941.test.ts
- packages/opencode/test/altimate/tracing-final-audit.test.ts
- packages/opencode/test/permission/next.test.ts
…s-platform accuracy
21c073e to
b585e9c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
| export const ToolID = "bash" | ||
| // Keep the exposed tool ID and permission key as "terminal" for compatibility with | ||
| // existing plugins, users, and saved permissions. | ||
| export const ToolID = "terminal" |
There was a problem hiding this comment.
WARNING: Renaming the tool ID / permission key to terminal silently orphans existing user permission configs still keyed on bash.
Permission.fromConfig (src/permission/index.ts:207) and PermissionNext.fromConfig (src/permission/next.ts:49) emit the config key verbatim (permission: key) with no bash→terminal alias, and ConfigPermissionV1 (core/src/v1/config/permission.ts:24) still declares bash. After this rename the tool emits permission: "terminal", which does not match any rule a user wrote under "bash" (matching is literal: Wildcard.match("terminal","bash") is false), so customized shell permission rules are silently ignored.
The updated next.test.ts expectations already assume this normalization — fromConfig({ bash: "allow" }) is asserted to yield permission: "terminal" — so those assertions currently fail; a string-only replace also left unquoted object keys like { bash: "allow" } untouched. Either add a bash→terminal alias in fromConfig (preserves existing user configs and fixes the tests) or update the test inputs and document the breaking change.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Issue for this PR
Closes #1070
Type of change
What does this PR do?
This PR renames the legacy internal
bashtool toterminalto resolve cross-platform naming confusion for Windows users.(Note: The original issue proposed renaming this to
shell, but since a newshell.tstool was recently merged as part of a v2 refactor,terminalwas chosen instead to avoid any name collisions).How did you verify your code works?
I wrote a strict AST/regex script to safely replace the tool name across the monorepo while explicitly avoiding touching parsers like
tree-sitter-bash. Afterward, I ranbun testacross the workspace and verified the strict TypeScript compiler (bun run typecheck) passes with no broken imports or type mismatches.Screenshots / recordings
N/A
Checklist
Summary by CodeRabbit
terminal.