mirror of
https://github.com/HKUDS/nanobot.git
synced 2026-08-06 17:38:35 +00:00
@
fix(exec): default Windows commands to PowerShell and allow shell parameter Single-line commands on Windows were routed through cmd.exe (asyncio.create_subprocess_shell) while multi-line commands used PowerShell. This caused cross-drive cd failures, missing $VAR expansion, and inconsistent behavior depending on whether a command contained a newline. The shell parameter was also rejected on Windows. - Route all Windows commands through PowerShell by default so single-line and multi-line commands share the same shell semantics. - Allow the shell parameter on Windows: accepts powershell, pwsh, or cmd. - cmd.exe remains reachable via shell="cmd" as an explicit escape hatch. - Update tests to cover the new default and shell-parameter paths. Fixes #4544 @
This commit is contained in:
parent
10b52cfb3a
commit
33b1c6f601
@ -89,7 +89,7 @@ class _PreparedCommand:
|
|||||||
maximum=600,
|
maximum=600,
|
||||||
),
|
),
|
||||||
shell=StringSchema(
|
shell=StringSchema(
|
||||||
"Optional shell binary to launch. On Unix, supports sh, bash, or zsh.",
|
"Optional shell binary to launch. Unix: sh, bash, zsh. Windows: powershell, pwsh, cmd.",
|
||||||
nullable=True,
|
nullable=True,
|
||||||
),
|
),
|
||||||
login=BooleanSchema(
|
login=BooleanSchema(
|
||||||
@ -468,17 +468,23 @@ class ExecTool(Tool):
|
|||||||
) -> asyncio.subprocess.Process:
|
) -> asyncio.subprocess.Process:
|
||||||
"""Launch *command* in a platform-appropriate shell."""
|
"""Launch *command* in a platform-appropriate shell."""
|
||||||
if _IS_WINDOWS:
|
if _IS_WINDOWS:
|
||||||
if "\n" in command:
|
# Default to PowerShell so single-line and multi-line commands
|
||||||
return await asyncio.create_subprocess_exec(
|
# share the same shell semantics. cmd.exe is reachable via the
|
||||||
"powershell", "-NoProfile", "-Command", command,
|
# explicit shell="cmd" parameter (see _resolve_shell).
|
||||||
|
default_program = shutil.which("powershell") or "powershell"
|
||||||
|
program = shell_program or default_program
|
||||||
|
program_name = Path(program).name.lower()
|
||||||
|
if program_name in ("cmd", "cmd.exe"):
|
||||||
|
return await asyncio.create_subprocess_shell(
|
||||||
|
command,
|
||||||
stdin=stdin,
|
stdin=stdin,
|
||||||
stdout=asyncio.subprocess.PIPE,
|
stdout=asyncio.subprocess.PIPE,
|
||||||
stderr=asyncio.subprocess.PIPE,
|
stderr=asyncio.subprocess.PIPE,
|
||||||
cwd=cwd,
|
cwd=cwd,
|
||||||
env=env,
|
env=env,
|
||||||
)
|
)
|
||||||
return await asyncio.create_subprocess_shell(
|
return await asyncio.create_subprocess_exec(
|
||||||
command,
|
program, "-NoProfile", "-Command", command,
|
||||||
stdin=stdin,
|
stdin=stdin,
|
||||||
stdout=asyncio.subprocess.PIPE,
|
stdout=asyncio.subprocess.PIPE,
|
||||||
stderr=asyncio.subprocess.PIPE,
|
stderr=asyncio.subprocess.PIPE,
|
||||||
@ -504,10 +510,33 @@ class ExecTool(Tool):
|
|||||||
def _resolve_shell(shell: str | None) -> tuple[str | None, str | None]:
|
def _resolve_shell(shell: str | None) -> tuple[str | None, str | None]:
|
||||||
if not shell:
|
if not shell:
|
||||||
return None, None
|
return None, None
|
||||||
if _IS_WINDOWS:
|
|
||||||
return None, ToolResult.error("Error: shell parameter is not supported on Windows")
|
|
||||||
if "\0" in shell or "\n" in shell or "\r" in shell:
|
if "\0" in shell or "\n" in shell or "\r" in shell:
|
||||||
return None, ToolResult.error("Error: shell contains invalid characters")
|
return None, ToolResult.error("Error: shell contains invalid characters")
|
||||||
|
if _IS_WINDOWS:
|
||||||
|
win_allowed = {"powershell", "powershell.exe", "pwsh", "pwsh.exe", "cmd", "cmd.exe"}
|
||||||
|
path = Path(shell).expanduser()
|
||||||
|
if path.is_absolute():
|
||||||
|
name = path.name.lower()
|
||||||
|
if name not in win_allowed:
|
||||||
|
return None, ToolResult.error(
|
||||||
|
f"Error: unsupported shell {shell!r}. "
|
||||||
|
"Allowed: powershell, pwsh, cmd"
|
||||||
|
)
|
||||||
|
if not path.is_file():
|
||||||
|
return None, ToolResult.error(f"Error: shell is not found: {shell}")
|
||||||
|
return str(path), None
|
||||||
|
if "/" in shell or "\\" in shell:
|
||||||
|
return None, ToolResult.error("Error: shell must be a shell name or absolute path")
|
||||||
|
if shell.lower() not in win_allowed:
|
||||||
|
return None, ToolResult.error(
|
||||||
|
f"Error: unsupported shell {shell!r}. "
|
||||||
|
"Allowed: powershell, pwsh, cmd"
|
||||||
|
)
|
||||||
|
if shell.lower() in ("cmd", "cmd.exe"):
|
||||||
|
resolved = os.environ.get("COMSPEC") or shutil.which("cmd") or "cmd"
|
||||||
|
return resolved, None
|
||||||
|
resolved = shutil.which(shell) or shell
|
||||||
|
return resolved, None
|
||||||
allowed = {"sh", "bash", "zsh"}
|
allowed = {"sh", "bash", "zsh"}
|
||||||
path = Path(shell).expanduser()
|
path = Path(shell).expanduser()
|
||||||
if path.is_absolute():
|
if path.is_absolute():
|
||||||
|
|||||||
@ -116,32 +116,37 @@ class TestSpawnUnix:
|
|||||||
class TestSpawnWindows:
|
class TestSpawnWindows:
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_single_line_uses_shell(self):
|
async def test_single_line_uses_powershell(self):
|
||||||
|
"""Single-line commands on Windows now route through PowerShell."""
|
||||||
env = {"COMSPEC": r"C:\Windows\system32\cmd.exe", "PATH": ""}
|
env = {"COMSPEC": r"C:\Windows\system32\cmd.exe", "PATH": ""}
|
||||||
with (
|
with (
|
||||||
patch("nanobot.agent.tools.shell._IS_WINDOWS", True),
|
patch("nanobot.agent.tools.shell._IS_WINDOWS", True),
|
||||||
patch("asyncio.create_subprocess_shell", new_callable=AsyncMock) as mock_shell,
|
patch("asyncio.create_subprocess_exec", new_callable=AsyncMock) as mock_exec,
|
||||||
):
|
):
|
||||||
mock_shell.return_value = AsyncMock()
|
mock_exec.return_value = AsyncMock()
|
||||||
await ExecTool._spawn("dir", r"C:\work", env)
|
await ExecTool._spawn("dir", r"C:\work", env)
|
||||||
|
|
||||||
args = mock_shell.call_args[0]
|
args = mock_exec.call_args[0]
|
||||||
assert "dir" in args
|
assert "powershell" in args[0].lower()
|
||||||
|
assert "-NoProfile" in args
|
||||||
|
assert "-Command" in args
|
||||||
|
assert "dir" in args[-1]
|
||||||
|
|
||||||
kwargs = mock_shell.call_args[1]
|
kwargs = mock_exec.call_args[1]
|
||||||
assert kwargs["stdin"] == asyncio.subprocess.DEVNULL
|
assert kwargs["stdin"] == asyncio.subprocess.DEVNULL
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_single_line_passes_cwd_and_env(self):
|
async def test_single_line_passes_cwd_and_env(self):
|
||||||
|
"""PowerShell should receive cwd and env from the caller."""
|
||||||
env = {"PATH": "/usr/bin"}
|
env = {"PATH": "/usr/bin"}
|
||||||
with (
|
with (
|
||||||
patch("nanobot.agent.tools.shell._IS_WINDOWS", True),
|
patch("nanobot.agent.tools.shell._IS_WINDOWS", True),
|
||||||
patch("asyncio.create_subprocess_shell", new_callable=AsyncMock) as mock_shell,
|
patch("asyncio.create_subprocess_exec", new_callable=AsyncMock) as mock_exec,
|
||||||
):
|
):
|
||||||
mock_shell.return_value = AsyncMock()
|
mock_exec.return_value = AsyncMock()
|
||||||
await ExecTool._spawn("echo hi", r"C:\work", env)
|
await ExecTool._spawn("echo hi", r"C:\work", env)
|
||||||
|
|
||||||
kwargs = mock_shell.call_args[1]
|
kwargs = mock_exec.call_args[1]
|
||||||
assert kwargs["cwd"] == r"C:\work"
|
assert kwargs["cwd"] == r"C:\work"
|
||||||
assert kwargs["env"] == env
|
assert kwargs["env"] == env
|
||||||
|
|
||||||
@ -156,7 +161,7 @@ class TestSpawnWindows:
|
|||||||
await ExecTool._spawn('python -c "print(1)\nprint(2)"', r"C:\work", env)
|
await ExecTool._spawn('python -c "print(1)\nprint(2)"', r"C:\work", env)
|
||||||
|
|
||||||
args = mock_exec.call_args[0]
|
args = mock_exec.call_args[0]
|
||||||
assert args[0] == "powershell"
|
assert "powershell" in args[0].lower()
|
||||||
assert "-NoProfile" in args
|
assert "-NoProfile" in args
|
||||||
assert "-Command" in args
|
assert "-Command" in args
|
||||||
assert "print(1)" in args[-1]
|
assert "print(1)" in args[-1]
|
||||||
@ -166,6 +171,24 @@ class TestSpawnWindows:
|
|||||||
assert kwargs["cwd"] == r"C:\work"
|
assert kwargs["cwd"] == r"C:\work"
|
||||||
assert kwargs["env"] == env
|
assert kwargs["env"] == env
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_explicit_cmd_shell_uses_create_subprocess_shell(self):
|
||||||
|
"""Explicit shell='cmd' should use create_subprocess_shell."""
|
||||||
|
env = {"COMSPEC": r"C:\Windows\system32\cmd.exe", "PATH": ""}
|
||||||
|
with (
|
||||||
|
patch("nanobot.agent.tools.shell._IS_WINDOWS", True),
|
||||||
|
patch("asyncio.create_subprocess_shell", new_callable=AsyncMock) as mock_shell,
|
||||||
|
):
|
||||||
|
mock_shell.return_value = AsyncMock()
|
||||||
|
await ExecTool._spawn(
|
||||||
|
"dir", r"C:\work", env,
|
||||||
|
shell_program=r"C:\Windows\system32\cmd.exe",
|
||||||
|
)
|
||||||
|
|
||||||
|
mock_shell.assert_called_once()
|
||||||
|
kwargs = mock_shell.call_args[1]
|
||||||
|
assert kwargs["cwd"] == r"C:\work"
|
||||||
|
|
||||||
|
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
# path_append
|
# path_append
|
||||||
@ -488,7 +511,7 @@ class TestExtractAbsolutePaths:
|
|||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
class TestWindowsMultilineExec:
|
class TestWindowsMultilineExec:
|
||||||
"""Verify multi-line commands on Windows route through PowerShell."""
|
"""Verify commands on Windows route through PowerShell (now the default)."""
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_multiline_python_uses_powershell(self):
|
async def test_multiline_python_uses_powershell(self):
|
||||||
@ -509,7 +532,7 @@ class TestWindowsMultilineExec:
|
|||||||
assert "2" in result
|
assert "2" in result
|
||||||
assert "Exit code: 0" in result
|
assert "Exit code: 0" in result
|
||||||
args = mock_exec.call_args[0]
|
args = mock_exec.call_args[0]
|
||||||
assert args[0] == "powershell"
|
assert "powershell" in args[0].lower()
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_multiline_node_uses_powershell(self):
|
async def test_multiline_node_uses_powershell(self):
|
||||||
@ -528,10 +551,11 @@ class TestWindowsMultilineExec:
|
|||||||
|
|
||||||
assert "1" in result
|
assert "1" in result
|
||||||
args = mock_exec.call_args[0]
|
args = mock_exec.call_args[0]
|
||||||
assert args[0] == "powershell"
|
assert "powershell" in args[0].lower()
|
||||||
|
|
||||||
@pytest.mark.asyncio
|
@pytest.mark.asyncio
|
||||||
async def test_single_line_uses_shell(self):
|
async def test_single_line_uses_powershell(self):
|
||||||
|
"""Single-line commands also route through PowerShell now."""
|
||||||
mock_proc = AsyncMock()
|
mock_proc = AsyncMock()
|
||||||
mock_proc.communicate.return_value = (b"1\n", b"")
|
mock_proc.communicate.return_value = (b"1\n", b"")
|
||||||
mock_proc.returncode = 0
|
mock_proc.returncode = 0
|
||||||
@ -563,3 +587,60 @@ class TestWindowsMultilineExec:
|
|||||||
|
|
||||||
assert "1" in result
|
assert "1" in result
|
||||||
mock_spawn.assert_called_once()
|
mock_spawn.assert_called_once()
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# _resolve_shell — Windows support
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
class TestResolveShellWindows:
|
||||||
|
"""shell parameter is now accepted on Windows."""
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_shell_powershell_accepted(self):
|
||||||
|
"""shell='powershell' should resolve and route through PowerShell."""
|
||||||
|
mock_proc = AsyncMock()
|
||||||
|
mock_proc.communicate.return_value = (b"hello\n", b"")
|
||||||
|
mock_proc.returncode = 0
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch("nanobot.agent.tools.shell._IS_WINDOWS", True),
|
||||||
|
patch("asyncio.create_subprocess_exec", new_callable=AsyncMock) as mock_exec,
|
||||||
|
patch.object(ExecTool, "_guard_command", return_value=None),
|
||||||
|
):
|
||||||
|
mock_exec.return_value = mock_proc
|
||||||
|
tool = ExecTool()
|
||||||
|
result = await tool.execute(command="echo hello", shell="powershell")
|
||||||
|
|
||||||
|
assert "hello" in result
|
||||||
|
args = mock_exec.call_args[0]
|
||||||
|
assert "powershell" in args[0].lower()
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_shell_cmd_accepted(self):
|
||||||
|
"""shell='cmd' should use create_subprocess_shell."""
|
||||||
|
mock_proc = AsyncMock()
|
||||||
|
mock_proc.communicate.return_value = (b"hello\n", b"")
|
||||||
|
mock_proc.returncode = 0
|
||||||
|
|
||||||
|
with (
|
||||||
|
patch("nanobot.agent.tools.shell._IS_WINDOWS", True),
|
||||||
|
patch("asyncio.create_subprocess_shell", new_callable=AsyncMock) as mock_shell,
|
||||||
|
patch.object(ExecTool, "_guard_command", return_value=None),
|
||||||
|
):
|
||||||
|
mock_shell.return_value = mock_proc
|
||||||
|
tool = ExecTool()
|
||||||
|
result = await tool.execute(command="echo hello", shell="cmd")
|
||||||
|
|
||||||
|
assert "hello" in result
|
||||||
|
mock_shell.assert_called_once()
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_shell_bash_rejected_on_windows(self):
|
||||||
|
"""shell='bash' should still be rejected on Windows."""
|
||||||
|
with patch("nanobot.agent.tools.shell._IS_WINDOWS", True):
|
||||||
|
tool = ExecTool()
|
||||||
|
result = await tool.execute(command="echo hello", shell="bash")
|
||||||
|
|
||||||
|
assert "Error: unsupported shell" in result
|
||||||
|
assert "Allowed: powershell, pwsh, cmd" in result
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user