Fix Bash output buffering/wedge, add silence timeout and exit-code reporting

- Read output incrementally with a bounded head+tail buffer: a huge
  stream can no longer exhaust memory, and delivery never waits on a
  detached child holding the stdout pipe open.
- Kill the process group by the pgid captured at spawn (no kill-time
  getpgid race); the observer thread kills directly, so Ctrl-C can
  never leave orphans.
- Add BASH_TIMEOUT_SILENCE (120s, config): a command silent that long
  is SIGTERM'd then SIGKILL'd and reported as timed out, preserving
  partial output. Builds that keep printing are never affected.
  BASH_TIMEOUT_MAX is an opt-in absolute wall-clock cap.
- Append 'Exit code: N' after truncation so it always survives.
- Share the 20 KB output cap via config.MAX_OUTPUT_CHARS (Bash
  truncation + filesystem spool threshold).

Author: beacoder

python_agent_harness/config.py

@@ -97,6 +97,23 @@
     "Question",
 ]
 
+# ---- tool output limits --------------------------------------------------------
+# Shared cap for tool results.  Bash truncates output to a head+tail at
+# this size; Read/Glob/Grep spill results larger than this to a temp
+# file (the model then Reads the file for the full content).  One
+# definition, used by all tools, so the limits never drift apart.
+MAX_OUTPUT_CHARS = 20_000
+
+# ---- Bash tool timeout --------------------------------------------------------
+# Silence-based timeout: a command that produces no output for this
+# long (seconds) is killed (SIGTERM, then SIGKILL after 2s) and
+# reported as timed out.  Builds that keep printing are never affected
+# — only genuinely stuck commands surface.  None disables the check.
+BASH_TIMEOUT_SILENCE: float | None = 120.0
+# Optional absolute wall-clock cap (seconds): no command may run longer
+# than this regardless of output.  None disables the cap (default).
+BASH_TIMEOUT_MAX: float | None = None
+
 # ---- LLM client ----------------------------------------------------------------
 DEFAULT_BASE_URL = os.environ.get("OPENAI_BASE_URL", "https://api.openai.com/v1")
 DEFAULT_MODEL = os.environ.get("OPENAI_MODEL", "gpt-5-mini")

python_agent_harness/tools/bash.py

@@ -9,44 +9,219 @@
 
 A session cancel (Ctrl-C) kills the process group and delivers a
 cancelled error.
+
+Output is read incrementally with a bounded buffer (head + tail), so a
+huge stream (e.g. ``cat`` of a multi-GB log) can never exhaust memory,
+and delivery never waits on a detached child that keeps the stdout
+pipe open after the shell has exited.
+
+Normal completion appends ``Exit code: N`` (N negative = killed by a
+signal).  A command that produces no output for ``BASH_TIMEOUT_SILENCE``
+seconds is killed (SIGTERM then SIGKILL) and reported as timed out;
+``BASH_TIMEOUT_MAX`` optionally caps the total runtime.  Ctrl-C kills
+the process group immediately.
 """
 
 from __future__ import annotations
 
+import codecs
 import contextlib
 import os
+import select
 import signal
 import subprocess
 import threading
+import time
+from collections import deque
 
+from ..config import BASH_TIMEOUT_MAX, BASH_TIMEOUT_SILENCE
+from ..config import MAX_OUTPUT_CHARS as _MAX_OUTPUT
 from .base import PendingToolResult, Tool, ToolContext
 
-# Maximum output size (chars) before truncation.  Matches the 20 KB
-# spool threshold used by Glob/Grep/Read so that a model-issued
-# command cannot exhaust memory.
-_MAX_OUTPUT = 20_000
+# Tail lines kept after truncation (the head budget is derived from
+# MAX_OUTPUT_CHARS, shared with the filesystem spool threshold).
 _TAIL_LINES = 50  # lines kept from the tail after truncation
+_READ_CHUNK = 64 * 1024  # bytes read from the pipe per iteration
+_POLL_INTERVAL = 0.02  # seconds between cancel/exit checks
+_DRAIN_GRACE = 0.25  # seconds to keep reading after the process exits
+
+
+def _kill_pgid(pgid: int) -> None:
+    """Kill the process group PGID, ignoring "already gone" errors.
+
+    The group id is captured at spawn (with ``start_new_session=True``
+    the child is the group leader, so its pid IS the pgid): it must
+    never be resolved at kill time via ``os.getpgid`` — by then the
+    shell may already be dead while a detached child that keeps the
+    stdout pipe open is still running in the group.
+    """
+    with contextlib.suppress(ProcessLookupError, PermissionError, OSError):
+        os.killpg(pgid, signal.SIGKILL)
 
 
-def _kill_process(proc: subprocess.Popen) -> None:
-    """Kill PROC and its whole process group.
+def _kill_graceful(pgid: int, proc: subprocess.Popen) -> None:
+    """SIGTERM the process group; SIGKILL if it is still alive 2s later.
 
-    The process is started in its own session so a shell's children
-    (e.g. `sleep 30` spawned by the shell) die too; otherwise they keep
-    the stdout pipe open and ``communicate()`` blocks until they exit.
+    Used for timeouts so shells/compilers get a chance to clean up
+    children before the hard kill.
     """
+    with contextlib.suppress(ProcessLookupError, PermissionError, OSError):
+        os.killpg(pgid, signal.SIGTERM)
     try:
-        os.killpg(os.getpgid(proc.pid), signal.SIGKILL)
-    except (ProcessLookupError, PermissionError, OSError):
-        with contextlib.suppress(ProcessLookupError):
-            proc.kill()
+        proc.wait(timeout=2)
+    except subprocess.TimeoutExpired:
+        _kill_pgid(pgid)
+
+
+def _timeout_message(out: str, silence: bool) -> str:
+    """Report a timed-out command, preserving any output so far."""
+    if silence:
+        timeout = BASH_TIMEOUT_SILENCE or 0.0
+        reason = f"no output for {timeout:.0f}s"
+    else:
+        timeout = BASH_TIMEOUT_MAX or 0.0
+        reason = f"exceeded the {timeout:.0f}s maximum"
+    out = out.rstrip("\n")
+    suffix = f"Error: Bash command timed out ({reason})."
+    return f"{out}\n\n{suffix}" if out else suffix
+
+
+def _append_exit_code(out: str, proc: subprocess.Popen) -> str:
+    """Append ``Exit code: N`` (N negative = killed by a signal).
+
+    The exit-code line is added AFTER truncation, so it is always the
+    last line and survives the head+tail retention.
+    """
+    try:
+        rc = proc.wait(timeout=2)
+    except subprocess.TimeoutExpired:
+        return out  # still alive; nothing useful to report
+    if out and not out.endswith("\n"):
+        out += "\n"
+    return f"{out}Exit code: {rc}"
+
+
+def _assemble_truncated(head: str, tail: deque[str]) -> str:
+    """Assemble a truncated output within the cap.
+
+    ``head`` holds the first ``_MAX_OUTPUT`` chars, ``tail`` the last
+    ``_TAIL_LINES`` lines (each already line-capped).  The tail is
+    preferred: as many trailing lines as fit are kept and the head gets
+    the remaining budget, so the delivered string never exceeds
+    ``_MAX_OUTPUT`` (plus the truncation notice).
+    """
+    notice = f"... [truncated: output exceeded {_MAX_OUTPUT} chars] ..."
+    budget = _MAX_OUTPUT - len(notice) - 4  # room for the "\n\n" separators
+    tail_parts: list[str] = []
+    used = 0
+    for line in reversed(tail):
+        cost = len(line) + (1 if tail_parts else 0)
+        if used + cost > budget:
+            break
+        tail_parts.append(line)
+        used += cost
+    head = head[: max(0, budget - used)]
+    out = f"{head}\n\n{notice}"
+    if tail_parts:
+        out += "\n\n" + "\n".join(reversed(tail_parts))
+    return out
+
+
+def _collect_output(proc: subprocess.Popen, cancel: threading.Event | None) -> tuple[str, str]:
+    """Read PROC's merged output incrementally; return (text, status).
+
+    Status is one of ``"ok"``, ``"cancelled"``, ``"timeout_silence"``,
+    ``"timeout_max"``.  Keeps the head (first ``_MAX_OUTPUT`` chars) and
+    the tail (last ``_TAIL_LINES`` lines) and discards the middle, so
+    memory stays bounded no matter how much the process writes.  The
+    read loop is poll-based: a cancel is noticed promptly, a process
+    silent for ``BASH_TIMEOUT_SILENCE`` seconds (or running past
+    ``BASH_TIMEOUT_MAX``) is reported as timed out, and a process that
+    has exited is only drained for ``_DRAIN_GRACE`` seconds — a
+    detached child holding the pipe open can never wedge delivery.
+    """
+    stdout = proc.stdout
+    if stdout is None:  # unreachable (stdout=PIPE), kept for the type checker
+        return "", "ok"
+    fd = stdout.fileno()
+    os.set_blocking(fd, False)
+    decoder = codecs.getincrementaldecoder("utf-8")(errors="replace")
+    head: list[str] = []
+    head_len = 0
+    tail: deque[str] = deque(maxlen=_TAIL_LINES)
+    pending_line = ""
+    total = 0
+    exited = False
+    drain_until: float | None = None
+    start = time.monotonic()
+    last_output = start
+
+    def finish() -> str:
+        nonlocal pending_line
+        if pending_line:
+            if len(pending_line) > _MAX_OUTPUT:
+                pending_line = pending_line[:_MAX_OUTPUT]
+            tail.append(pending_line)
+        head_text = "".join(head)
+        if total > _MAX_OUTPUT:
+            return _assemble_truncated(head_text, tail)
+        return head_text
+
+    while True:
+        if cancel is not None and cancel.is_set():
+            return "", "cancelled"
+        now = time.monotonic()
+        if not exited:
+            if BASH_TIMEOUT_SILENCE is not None and now - last_output >= BASH_TIMEOUT_SILENCE:
+                return finish(), "timeout_silence"
+            if BASH_TIMEOUT_MAX is not None and now - start >= BASH_TIMEOUT_MAX:
+                return finish(), "timeout_max"
+        if exited and drain_until is not None and now >= drain_until:
+            break
+        readable, _, _ = select.select([fd], [], [], _POLL_INTERVAL)
+        if not readable:
+            if not exited and proc.poll() is not None:
+                exited = True
+                drain_until = time.monotonic() + _DRAIN_GRACE
+            continue
+        try:
+            raw = os.read(fd, _READ_CHUNK)
+        except BlockingIOError:
+            continue
+        except OSError:
+            break
+        if not raw:
+            break  # EOF: every writer closed the pipe
+        chunk = decoder.decode(raw)
+        total += len(chunk)
+        last_output = time.monotonic()
+        if head_len < _MAX_OUTPUT:
+            take = chunk[: _MAX_OUTPUT - head_len]
+            head.append(take)
+            head_len += len(take)
+        parts = chunk.split("\n")
+        parts[0] = pending_line + parts[0]
+        pending_line = parts.pop()
+        for line in parts:
+            if len(line) > _MAX_OUTPUT:
+                line = line[:_MAX_OUTPUT]
+            tail.append(line)
+    return finish(), "ok"
 
 
 class Bash(Tool):
     name = "Bash"
+    _timeout_silence = BASH_TIMEOUT_SILENCE
+    _timeout_note = (
+        f"A command silent for {_timeout_silence:.0f}s is killed and reported as timed out. "
+        if _timeout_silence is not None
+        else ""
+    )
     description = (
-        "Execute a shell command. Returns stdout, or an error string. "
-        "A session cancel (Ctrl-C) kills the process."
+        "Execute a shell command. Returns stdout followed by 'Exit code: N' "
+        "(N is the command's exit status; negative means killed by a signal). "
+        + _timeout_note
+        + "A session cancel (Ctrl-C) kills the process."
     )
     parameters = {
         "type": "object",
@@ -61,60 +236,63 @@ def run(self, args: dict, ctx: ToolContext) -> str | PendingToolResult:
         return self._execute(command, ctx)
 
     def _execute(self, command: str, ctx: ToolContext) -> str | PendingToolResult:
+        cancel = ctx.cancel_event
+        if cancel is not None and cancel.is_set():
+            # Ctrl-C already pending: do not spawn a process that would
+            # be killed moments later.
+            return "Error: Bash command cancelled."
         try:
             proc = subprocess.Popen(
                 command,
                 shell=True,
                 stdin=subprocess.DEVNULL,
                 stdout=subprocess.PIPE,
                 stderr=subprocess.STDOUT,
-                text=True,
-                encoding="utf-8",
-                errors="replace",
+                bufsize=0,
                 cwd=ctx.cwd,
                 start_new_session=True,
             )
         except OSError as e:
             return f"Error: {e}"
 
+        # start_new_session=True makes the child the session/group
+        # leader, so its pid IS the group id — captured once here,
+        # never resolved again at kill time.
+        pgid = proc.pid
         pending = PendingToolResult()
-        cancel = ctx.cancel_event
-        done = threading.Event()
-        killed = threading.Event()
-
-        def watcher() -> None:
-            """Kill the process group when the session is cancelled."""
-            while True:
-                if done.wait(0.05):
-                    return
-                if cancel is not None and cancel.is_set():
-                    killed.set()
-                    _kill_process(proc)
-                    return
 
         def deliverer() -> None:
-            """Collect output; deliver it once the process exits."""
+            """Collect output; deliver it once the process exits.
+
+            The kill happens HERE (not in a separate watcher thread):
+            the collector is the thread that observed the condition, so
+            there is no race window in which a watcher exits without
+            killing and the process group survives.  Cancel is an
+            immediate SIGKILL; a timeout kills gracefully (SIGTERM,
+            then SIGKILL after 2s) so children get a chance to clean up.
+            """
             try:
-                out, _ = proc.communicate()
+                out, status = _collect_output(proc, cancel)
             except Exception as e:  # noqa: BLE001 - delivered as an error string
                 out = f"Error: Bash failed — {e}"
-            finally:
-                done.set()
-            if killed.is_set():
-                pending.deliver("Error: Bash command cancelled.")
             else:
-                out = out or ""
-                if len(out) > _MAX_OUTPUT:
-                    # Keep head + tail so the model sees the start and end
-                    tail = "\n".join(out.splitlines()[-_TAIL_LINES:])
-                    head_budget = _MAX_OUTPUT - len(tail) - 200  # room for notice
-                    head = out[: max(head_budget, 1000)]
-                    out = (
-                        f"{head}\n\n... [truncated: output exceeded "
-                        f"{_MAX_OUTPUT} chars] ...\n\n{tail}"
-                    )
-                pending.deliver(out)
-
-        threading.Thread(target=watcher, daemon=True).start()
+                if status == "cancelled":
+                    _kill_pgid(pgid)
+                    out = "Error: Bash command cancelled."
+                elif status == "timeout_silence":
+                    _kill_graceful(pgid, proc)
+                    out = _timeout_message(out, silence=True)
+                elif status == "timeout_max":
+                    _kill_graceful(pgid, proc)
+                    out = _timeout_message(out, silence=False)
+                elif status == "ok":
+                    out = _append_exit_code(out, proc)
+            pending.deliver(out)
+            with contextlib.suppress(Exception):
+                if proc.stdout is not None:
+                    proc.stdout.close()
+            with contextlib.suppress(Exception):
+                proc.wait(timeout=2)  # reap (bounded; never wedges)
+
         threading.Thread(target=deliverer, daemon=True).start()
         return pending

python_agent_harness/tools/filesystem.py

@@ -46,10 +46,10 @@
 from pathlib import Path
 from typing import TypeGuard
 
+from ..config import MAX_OUTPUT_CHARS as MAX_OUTPUT
 from ..diffrender import unified_diff
 from .base import Tool, ToolContext
 
-MAX_OUTPUT = 20_000  # spill threshold (chars), matching gptel-agent--truncate-buffer
 SPOOL_LINES = 50  # preview lines kept when results are spilled
 READ_SIZE_LIMIT = 400 * 1024  # whole-file reads above this are refused
 # (mirrors gptel-agent-read-file-size-threshold)