Skip to content

fix(acp): parse shell command for terminal/create to fix acpx compatibility #1688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zhzy0077 wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −1
Open

fix(acp): parse shell command for terminal/create to fix acpx compatibility #1688

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/kimi_cli/acp/tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import asyncio
from contextlib import suppress
import shlex

import acp
from kaos import get_current_kaos
Expand Down Expand Up @@ -88,8 +89,13 @@ async def __call__(self, params: ShellParams) -> ToolReturnValue:
timed_out = False

try:
# Parse command string into command and args for ACP
parts = shlex.split(params.command)
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shlex.split() can raise ValueError (e.g., unmatched quotes). Right now that exception will bubble up and likely surface as a generic ACP "Internal error". Consider catching ValueError here and returning a user-facing tool error like "Invalid command" (including the parse error in debug logs).

Suggested change
parts = shlex.split(params.command)
try:
parts = shlex.split(params.command)
except ValueError as e:
# Handle malformed command strings (e.g., unmatched quotes) gracefully.
return builder.error(
f"Invalid command: {e}",
brief="Invalid command syntax",
)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 shlex.split() raises unhandled ValueError on malformed quotes in command

shlex.split() raises ValueError ("No closing quotation") when the command string contains unmatched quotes, e.g. echo "hello. This exception is not caught in the try block at line 91, which only handles TimeoutError in its inner block. The ValueError will propagate up as an unhandled exception instead of returning a clean tool error response via builder.error(). While there may be a generic exception handler further up the call stack, this produces an unhelpful error message rather than the user-friendly format the tool is designed to return.

Suggested change
parts = shlex.split(params.command)
try:
parts = shlex.split(params.command)
except ValueError as e:
return builder.error(f"Invalid command syntax: {e}", brief="Invalid command")
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve Windows paths when parsing terminal command

Using shlex.split(params.command) with default POSIX behavior removes backslashes, so Windows-style commands are mangled before terminal/create is called (for example, C:\Windows\System32\whoami.exe becomes C:WindowsSystem32whoami.exe). This breaks ACP terminal execution on Windows for absolute executable paths and arguments containing \, even when the original command string is valid.

Useful? React with 👍 / 👎.

cmd = parts[0] if parts else ""
args = parts[1:] if len(parts) > 1 else []
Comment on lines +93 to +95
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration bot Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 shlex.split() breaks shell compound commands (pipes, redirects, &&, ||, etc.)

The LLM generates commands intended for shell interpretation (the Shell tool description tells it to write bash commands, and the original Shell tool wraps them in bash -c <command> at src/kimi_cli/tools/shell/__init__.py:235). Using shlex.split() to tokenize the command and pass the first token as command and the rest as args will break any command that uses shell features. For example, cat file.txt | grep pattern gets split into cmd='cat', args=['file.txt', '|', 'grep', 'pattern'], which would execute cat with those literal arguments rather than piping through grep. Similarly, echo hello && echo world, ls > output.txt, for i in 1 2 3; do echo $i; done, variable expansion (echo $HOME), and glob patterns (ls *.py) will all silently produce wrong results or fail. Since the Shell tool's contract with the LLM is to accept full shell command strings, splitting them into exec-style argv is a semantic mismatch.

Prompt for agents 
In src/kimi_cli/acp/tools.py lines 92-98, instead of using shlex.split() to parse the command string, the command should be wrapped in a shell invocation to preserve shell semantics (pipes, redirects, &&, ||, variable expansion, globs, etc.). The ACP create_terminal should receive the shell executable (e.g. 'bash' or the shell from the environment) as the command and ['-c', params.command] as the args. This mirrors what the original Shell tool does in src/kimi_cli/tools/shell/__init__.py:232-235 where it uses _shell_args to produce ('bash', '-c', command). You may need to plumb the shell path from the Environment/Shell tool configuration into the Terminal tool, or hard-code a reasonable default like '/bin/sh'.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

resp = await self._acp_conn.create_terminal(
Comment on lines +92 to 96
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new command/args parsing is core to ACP terminal compatibility but currently isn’t covered by tests. Consider adding a focused unit test (mocking _acp_conn.create_terminal) that asserts "ls -la" becomes command="ls" and args=["-la"], plus a quoted-arg case (e.g. echo "a b").

Copilot uses AI. Check for mistakes.
command=params.command,
command=cmd,
Comment on lines +93 to +97
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the earlier guard is if not params.command, a whitespace-only command (e.g. " ") will pass, but shlex.split() will return [], resulting in command="" being sent to create_terminal. Consider stripping before splitting and erroring if no parts are produced.

Copilot uses AI. Check for mistakes.
args=args,
session_id=self._acp_session_id,
output_byte_limit=builder.max_chars,
)
Expand Down
Loading