From 51279924955e46d072190c61679bcde3e565882d Mon Sep 17 00:00:00 2001 From: Amanbig Date: Sat, 31 Oct 2026 15:44:28 +0530 Subject: [PATCH 1/4] test: wait for the virtual terminal to settle instead of a fixed sleep The screen-test harness snapshotted after a fixed 0.15s, which is racy on a slow CI runner: one job snapshotted before the first frame was drawn and indexed into an empty screen. Wait at least that long, then until the output has stopped changing, and fail with a clear message if nothing renders. --- tests/_screen.py | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/tests/_screen.py b/tests/_screen.py index c8fc260..1df62c9 100644 --- a/tests/_screen.py +++ b/tests/_screen.py @@ -15,7 +15,16 @@ from prompt_toolkit.output.vt100 import Vt100_Output -def render_screen(keys: str, cols=60, rows=14, wait=0.15, **kwargs) -> pyte.Screen: +def render_screen( + keys: str, + cols=60, + rows=14, + wait=0.15, + limit=8.0, + poll=0.02, + quiet_polls=8, + **kwargs, +) -> pyte.Screen: """Type ``keys`` (without Enter) and snapshot the screen while the prompt is still active.""" class Buf(io.StringIO): @@ -31,15 +40,30 @@ class Buf(io.StringIO): def pre_run(): app = session.app - loop = asyncio.get_event_loop() - def finish(): + async def settle(): + # A fixed sleep is racy on slow CI runners (the snapshot can land before the + # first frame is drawn). Wait at least `wait`, then until the output has stopped + # changing for a while, and give up (clearly) after `limit` seconds. + loop = asyncio.get_running_loop() + deadline = loop.time() + limit + await asyncio.sleep(wait) + last, quiet = -1, 0 + while loop.time() < deadline: + size = len(buf.getvalue()) + quiet = quiet + 1 if size and size == last else 0 + last = size + if quiet >= quiet_polls: + break + await asyncio.sleep(poll) snap.append(buf.getvalue()) app.exit(result="") - loop.call_later(wait, finish) + asyncio.ensure_future(settle()) session.prompt(kwargs.get("message", "> "), pre_run=pre_run) + if not snap or not snap[0]: + raise AssertionError(f"nothing was rendered within {limit}s (keys={keys!r})") screen = pyte.Screen(cols, rows) pyte.Stream(screen).feed(snap[0]) return screen From e46f71425d4bc18ce90b06aa4a446c115ba4e8f9 Mon Sep 17 00:00:00 2001 From: Amanbig Date: Sat, 31 Oct 2026 15:49:01 +0530 Subject: [PATCH 2/4] test: make the hooks tests portable to Windows The hooks tests used POSIX shell syntax (echo x >&2; exit 2, sleep 5). In cmd.exe there is no ; separator or sleep, so the hook never exited with 2 and two tests failed on every Windows job. Run Python for the exit code, stderr output and sleep instead, and assert the timeout test really stops the hook. --- tests/test_hooks.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/tests/test_hooks.py b/tests/test_hooks.py index e252380..9c2b677 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -18,6 +18,13 @@ def run(self, arguments): return "ran" +def py(code: str) -> str: + """A hook command that runs Python code. Works in sh and in Windows cmd.exe, where `echo x >&2; + exit 2` is not valid (cmd has no `;` separator and no `sleep`). Use single quotes inside `code`.""" + exe = f'"{sys.executable}"' if " " in sys.executable else sys.executable + return f'{exe} -c "{code}"' + + def _executor(hooks): return ToolExecutor([EchoTool()], require_confirmation=False, hooks=hooks) @@ -29,25 +36,27 @@ def test_matcher(): def test_pre_hook_blocks_with_exit_2(): - h = HookManager({PRE_TOOL_USE: [Hook("echo nope >&2; exit 2")]}) + h = HookManager({PRE_TOOL_USE: [Hook(py("import sys; sys.stderr.write('nope'); sys.exit(2)"))]}) out = _executor(h).execute("echo", {}) assert "Blocked by PreToolUse hook" in out and "nope" in out def test_pre_hook_nonblocking_error_allows(): - h = HookManager({PRE_TOOL_USE: [Hook("exit 1")]}) + h = HookManager({PRE_TOOL_USE: [Hook(py("import sys; sys.exit(1)"))]}) assert _executor(h).execute("echo", {}) == "ran" def test_post_hook_feedback_appended(): - h = HookManager({POST_TOOL_USE: [Hook("echo lint failed >&2; exit 2")]}) + h = HookManager( + {POST_TOOL_USE: [Hook(py("import sys; sys.stderr.write('lint failed'); sys.exit(2)"))]} + ) out = _executor(h).execute("echo", {}) assert out.startswith("ran") and "lint failed" in out def test_hook_receives_json_payload(tmp_path): dump = tmp_path / "payload.json" - cmd = f"{sys.executable} -c \"import sys; open(r'{dump}','w').write(sys.stdin.read())\"" + cmd = py(f"import sys; open(r'{dump}','w').write(sys.stdin.read())") h = HookManager({POST_TOOL_USE: [Hook(cmd)]}, cwd=tmp_path) _executor(h).execute("echo", {"a": 1}) data = json.loads(dump.read_text()) @@ -58,8 +67,12 @@ def test_hook_receives_json_payload(tmp_path): def test_timeout_does_not_block(): - h = HookManager({PRE_TOOL_USE: [Hook("sleep 5", timeout=1)]}) + import time + + h = HookManager({PRE_TOOL_USE: [Hook(py("import time; time.sleep(30)"), timeout=1)]}) + started = time.monotonic() assert _executor(h).execute("echo", {}) == "ran" + assert time.monotonic() - started < 15, "the hook was not stopped by its timeout" def test_load_file(tmp_path: Path): From 64eeda6819471a3c6aacaf644df3d87a50d25b60 Mon Sep 17 00:00:00 2001 From: Amanbig Date: Sat, 31 Oct 2026 15:54:34 +0530 Subject: [PATCH 3/4] fix: stop hook and shell commands for real when they time out subprocess.run(shell=True, timeout=...) only kills the shell. On Windows the command's own process kept running and held the output pipes open, so run() blocked until it finished anyway: a hook with a 1 second timeout took 30 seconds, and the shell tool's 120 second limit had the same flaw. Add utils.proc.run_shell, which starts the command in its own process group or session and kills the whole tree on timeout or Ctrl+C, and use it for hooks and the shell tool. --- cli/main.py | 5 +-- core/hooks.py | 11 +---- tests/test_proc.py | 103 +++++++++++++++++++++++++++++++++++++++++++++ tools/shell.py | 12 +----- utils/proc.py | 77 +++++++++++++++++++++++++++++++++ 5 files changed, 186 insertions(+), 22 deletions(-) create mode 100644 tests/test_proc.py create mode 100644 utils/proc.py diff --git a/cli/main.py b/cli/main.py index 64b61e3..cad3b0f 100644 --- a/cli/main.py +++ b/cli/main.py @@ -9,15 +9,14 @@ from rich.panel import Panel from rich.table import Table +# Import commands dynamically using Typer's app.add_command from cli.commands._shared import ( # noqa: E402 SYSTEM_PROMPT, # noqa: E402, F401 (used by start_repl via SimplePlanner) SimplePlanner, console, create_provider, ) -from cli.commands.ask import ask # noqa: E402 -from cli.commands.edit import edit # noqa: E402 -from cli.commands.run import run # noqa: E402 +from cli.commands import ask, edit, run # Custom style for questionary prompts from cli.constants import ( # noqa: E402 diff --git a/core/hooks.py b/core/hooks.py index 90834ab..e7d1c39 100644 --- a/core/hooks.py +++ b/core/hooks.py @@ -34,6 +34,7 @@ from typing import Any from config.project_settings import HOOKS_REL, find_hooks_root, is_trusted +from utils.proc import run_shell try: import yaml @@ -161,15 +162,7 @@ def run( if not hook.matches(tool_name): continue try: - proc = subprocess.run( - hook.command, - shell=True, - input=stdin, - capture_output=True, - text=True, - timeout=hook.timeout, - cwd=self.cwd, - ) + proc = run_shell(hook.command, input=stdin, timeout=hook.timeout, cwd=self.cwd) except subprocess.TimeoutExpired: feedback.append(f"hook '{hook.command}' timed out after {hook.timeout}s") continue diff --git a/tests/test_proc.py b/tests/test_proc.py new file mode 100644 index 0000000..5842faa --- /dev/null +++ b/tests/test_proc.py @@ -0,0 +1,103 @@ +"""Tests for run_shell: timeouts must stop the whole process tree (the Windows hang).""" + +import subprocess +import sys +import time + +import pytest + +from tools.shell import ShellTool +from utils import proc as proc_mod +from utils.proc import run_shell + + +def py(code: str) -> str: + exe = f'"{sys.executable}"' if " " in sys.executable else sys.executable + return f'{exe} -c "{code}"' + + +def test_captures_output_and_exit_code(): + r = run_shell(py("import sys; print('out'); sys.stderr.write('err'); sys.exit(3)"), timeout=30) + assert r.returncode == 3 and "out" in r.stdout and "err" in r.stderr + + +def test_input_is_passed_on_stdin(): + r = run_shell(py("import sys; print(sys.stdin.read().upper())"), input="hello", timeout=30) + assert "HELLO" in r.stdout + + +def test_runs_in_the_given_directory(tmp_path): + r = run_shell(py("import os; print(os.getcwd())"), cwd=tmp_path, timeout=30) + assert tmp_path.name in r.stdout + + +def test_utf8_output_decodes(tmp_path): + r = run_shell( + py("import sys; sys.stdout.buffer.write('caf\\u00e9'.encode('utf-8'))"), + timeout=30, + encoding="utf-8", + errors="replace", + ) + assert "café" in r.stdout + + +def test_timeout_returns_promptly_instead_of_waiting_for_the_command(): + started = time.monotonic() + with pytest.raises(subprocess.TimeoutExpired): + run_shell(py("import time; time.sleep(30)"), timeout=1) + assert time.monotonic() - started < 15, "the command was not stopped by its timeout" + + +def test_timeout_kills_grandchildren_too(tmp_path): + # parent.py starts child.py (which would write a marker after 2s) and then sleeps; killing only the + # shell/parent would let the child finish and write the marker. + marker = tmp_path / "marker.txt" + (tmp_path / "child.py").write_text( + f"import time\ntime.sleep(2)\nopen(r'{marker}', 'w').write('alive')\n" + ) + (tmp_path / "parent.py").write_text( + "import subprocess, sys, time\n" + "subprocess.Popen([sys.executable, 'child.py'])\n" + "time.sleep(30)\n" + ) + with pytest.raises(subprocess.TimeoutExpired): + run_shell(py("exec(open('parent.py').read())"), timeout=1, cwd=tmp_path) + time.sleep(3.5) # longer than the child's 2s sleep + assert not marker.exists(), "a grandchild survived the timeout" + + +def test_keyboard_interrupt_kills_the_child_and_propagates(monkeypatch): + spawned = [] + + class InterruptedPopen(subprocess.Popen): + def __init__(self, *a, **k): + super().__init__(*a, **k) + spawned.append(self) + self._first = True + + def communicate(self, *a, **k): + if self._first: + self._first = False + raise KeyboardInterrupt + return super().communicate(*a, **k) + + monkeypatch.setattr(proc_mod.subprocess, "Popen", InterruptedPopen) + with pytest.raises(KeyboardInterrupt): + run_shell(py("import time; time.sleep(30)"), timeout=60) + assert spawned[0].poll() is not None, "the child was left running after Ctrl+C" + + +def test_shell_tool_reports_a_timeout_and_returns(monkeypatch): + import tools.shell as shell_mod + + real = shell_mod.run_shell + monkeypatch.setattr(shell_mod, "run_shell", lambda cmd, **kw: real(cmd, **{**kw, "timeout": 1})) + started = time.monotonic() + out = ShellTool().run({"command": py("import time; time.sleep(30)")}) + assert "timed out" in out.lower() + assert time.monotonic() - started < 15 + + +def test_shell_tool_still_formats_output(): + out = ShellTool().run({"command": py("print('hi')")}) + assert out.startswith("STDOUT:") and "hi" in out diff --git a/tools/shell.py b/tools/shell.py index 09f9ac1..7cde74e 100644 --- a/tools/shell.py +++ b/tools/shell.py @@ -6,6 +6,7 @@ from pydantic import BaseModel, Field from tools.base import Tool +from utils.proc import run_shell def normalize_windows_command(command: str) -> str: @@ -78,16 +79,7 @@ def run(self, arguments: dict[str, Any]) -> Any: if is_win: exec_cmd = normalize_windows_command(command) - result = subprocess.run( - exec_cmd, - shell=True, - capture_output=True, - text=True, - check=False, - timeout=120, - encoding="utf-8", - errors="replace", - ) + result = run_shell(exec_cmd, timeout=120, encoding="utf-8", errors="replace") output = "" if result.stdout: diff --git a/utils/proc.py b/utils/proc.py new file mode 100644 index 0000000..bbae6c3 --- /dev/null +++ b/utils/proc.py @@ -0,0 +1,77 @@ +"""Run shell commands with a timeout that really stops them. + +``subprocess.run(cmd, shell=True, timeout=...)`` only kills the *shell*. On Windows the command's own +process keeps running and keeps the output pipes open, so ``run`` blocks until it exits anyway (a +30-second sleep with a 1-second timeout took 30 seconds). On POSIX the same happens whenever the shell +does not exec its last command. This helper starts the command in its own process group / session and +kills the whole tree on timeout or on Ctrl+C. +""" + +import os +import signal +import subprocess +from typing import Any + +REAP_TIMEOUT = 5 # seconds to wait for pipes to close after the tree has been killed + + +def kill_process_tree(proc: subprocess.Popen) -> None: + """Kill ``proc`` and everything it started.""" + try: + if os.name == "nt": + # /T = the whole tree, /F = force. taskkill ships with every Windows install. + subprocess.run( + ["taskkill", "/F", "/T", "/PID", str(proc.pid)], + capture_output=True, + check=False, + timeout=REAP_TIMEOUT, + ) + else: + os.killpg(proc.pid, signal.SIGKILL) # the group id equals the pid (new session) + except (ProcessLookupError, PermissionError, OSError, subprocess.SubprocessError): + pass + try: + proc.kill() # belt and braces, e.g. if taskkill was unavailable + except OSError: + pass + + +def run_shell( + command: str, + *, + input: str | None = None, + timeout: float | None = None, + cwd: Any = None, + encoding: str | None = None, + errors: str | None = None, +) -> subprocess.CompletedProcess: + """``subprocess.run(command, shell=True, capture_output=True, text=True)`` that kills the whole + process tree on timeout or interrupt. Raises ``subprocess.TimeoutExpired`` like ``run`` does. + + stdin is inherited unless ``input`` is given, matching ``subprocess.run``. + """ + extra: dict[str, Any] = {} + if os.name != "nt": + extra["start_new_session"] = True + proc = subprocess.Popen( + command, + shell=True, + stdin=subprocess.PIPE if input is not None else None, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + encoding=encoding, + errors=errors, + cwd=cwd, + **extra, + ) + try: + stdout, stderr = proc.communicate(input=input, timeout=timeout) + except BaseException: # TimeoutExpired, KeyboardInterrupt, ... + kill_process_tree(proc) + try: + proc.communicate(timeout=REAP_TIMEOUT) + except Exception: + pass + raise + return subprocess.CompletedProcess(command, proc.returncode, stdout, stderr) From 658cb0f58904fc950befee769166f1cc1b7b26a6 Mon Sep 17 00:00:00 2001 From: Amanbig Date: Sat, 31 Oct 2026 15:57:54 +0530 Subject: [PATCH 4/4] revert: undo an accidental edit to cli/main.py An unrelated DevOrch session run in this directory replaced the command imports with 'from cli.commands import ask, edit, run', which imports modules instead of the command functions, and that edit was swept into the previous commit. Restore the original imports. --- cli/main.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cli/main.py b/cli/main.py index cad3b0f..64b61e3 100644 --- a/cli/main.py +++ b/cli/main.py @@ -9,14 +9,15 @@ from rich.panel import Panel from rich.table import Table -# Import commands dynamically using Typer's app.add_command from cli.commands._shared import ( # noqa: E402 SYSTEM_PROMPT, # noqa: E402, F401 (used by start_repl via SimplePlanner) SimplePlanner, console, create_provider, ) -from cli.commands import ask, edit, run +from cli.commands.ask import ask # noqa: E402 +from cli.commands.edit import edit # noqa: E402 +from cli.commands.run import run # noqa: E402 # Custom style for questionary prompts from cli.constants import ( # noqa: E402