From a0d3e19d0777907297401f4b9c05d5889ca7d5c1 Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Thu, 9 Jul 2026 22:16:40 -0700 Subject: [PATCH 01/14] Stop depending on GNU coreutils that macOS does not ship mngr shelled out to `timeout`, `tac`, `du -sb` and `stat -c %Y` in code that runs on the agent's host. For the `local` provider that host is the user's machine, and stock macOS ships a BSD userland with none of them. Each failure was reproduced on macOS 26.4.1 under a stock PATH before being fixed, and the fix re-verified against the same harness. `timeout` (agents/tui_utils.py): exits 127, so `mngr message` to a local codex or antigravity agent always raised SendMessageError, instantly, while claiming it had waited the full timeout. Claude agents degraded silently instead, since the 127 was swallowed inside a backgrounded subshell. Replacing `timeout(1)` needs more than sleep-and-kill. Measured on macOS: a `tmux wait-for` client exits 0 whether signalled or killed, so the exit status cannot distinguish them -- the watchdog therefore marks a file before killing, and that marker decides the exit code, the way timeout returns 124. The waiter must be the tmux client itself; killing a wrapper subshell leaves the client running forever. And background jobs must redirect stdout, since a job that inherits it holds the caller's stdout open until it exits, stalling every submission for the full deadline. `tac` (resources/mngr_transcript_lib.sh): the reverse scan read nothing, so the offset reset to 0 and the transcript re-emitted lines. Scanning forward and keeping the last match needs no reverse. This also drops a latent off-by-one: `wc -l` ignores an unterminated final line while `tac` emits it, so a live transcript being appended to reconciled one line low. `du -sb` / `stat -c %Y` (api/gc.py): both rejected by BSD. `du -sb ... | cut` made the pipeline exit status `cut`'s, so failure looked like success with empty output and every orphaned work dir was reported as 0 bytes; the `stat` failure made each orphan's created_at fall back to now(), so age-based collection never ran. Both sites now call the host interface, which gains a concrete `get_directory_size` beside `path_exists` and `get_file_mtime`. Regression tests poison `tac`/`timeout` on PATH, so a reintroduced dependency fails on Linux too rather than only on a user's mac. Co-Authored-By: Claude Opus 4.8 (1M context) --- libs/mngr/changelog/mngr-timeout-tac.md | 9 ++ libs/mngr/imbue/mngr/agents/tui_utils.py | 80 ++++++++++------ libs/mngr/imbue/mngr/agents/tui_utils_test.py | 53 +++++++++++ libs/mngr/imbue/mngr/api/gc.py | 37 +------- libs/mngr/imbue/mngr/conftest.py | 41 ++++++++ libs/mngr/imbue/mngr/interfaces/host.py | 30 ++++++ .../mngr/resources/mngr_transcript_lib.sh | 30 +++--- .../resources/mngr_transcript_lib_test.py | 94 +++++++++++++++++++ 8 files changed, 296 insertions(+), 78 deletions(-) create mode 100644 libs/mngr/changelog/mngr-timeout-tac.md create mode 100644 libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py diff --git a/libs/mngr/changelog/mngr-timeout-tac.md b/libs/mngr/changelog/mngr-timeout-tac.md new file mode 100644 index 0000000000..ffc38f5b34 --- /dev/null +++ b/libs/mngr/changelog/mngr-timeout-tac.md @@ -0,0 +1,9 @@ +Fixed four places where mngr shelled out to GNU-coreutils binaries and flags that stock macOS does not ship, each of which ran on the user's own machine whenever an agent used the `local` provider. All four were verified failing on macOS 26.4.1 with a stock `PATH`, then verified fixed. + +`mngr message` to a local codex or antigravity agent raised `SendMessageError("Timeout waiting for message submission signal")` on every send. The submission path bounded its `tmux wait-for` with `timeout(1)`, which exits 127 (`timeout: command not found`) on macOS -- immediately, while reporting that it had waited the full timeout. It is now bounded by a `sleep`-then-`kill` watchdog. Reproducing `timeout(1)` by hand takes more than `sleep` and `kill`: a `tmux wait-for` client exits 0 whether it was signalled or killed, so the watchdog marks a file *before* killing and that marker, not the wait status, decides the exit code (the way `timeout` returns 124); the waiter must be the tmux client itself rather than a wrapper subshell, or the kill hits the subshell and orphans the client; and every background job redirects stdout, because a job that inherits it holds the caller's stdout open until it exits, which would stall each submission for the full deadline even when the signal lands immediately. For Claude agents (which also watch an acceptance marker) the same bug degraded silently instead: the `timeout` failure was swallowed inside a backgrounded subshell, so the hook path never confirmed and only the transcript marker could -- meaning `/clear` and `/compact`, which fire the hook but never enqueue a model turn, always burned the full timeout. + +Raw-transcript streaming re-emitted already-emitted lines on macOS. `mngr_transcript_reconcile_offset` reverse-scanned the session file with `tac` to find the last emitted line; with `tac` absent the scan read nothing and the offset reset to 0. It now scans forward and tracks the last match, which needs no reverse at all. This also fixes a latent off-by-one: the old code combined `wc -l` (which does not count an unterminated final line) with `tac` (which emits it), so a session file whose last line was still being appended -- the normal state of a live transcript -- reconciled one line too low. + +`mngr gc` mis-measured orphaned work directories on macOS. `du -sb` has no BSD equivalent (`-b` is rejected), and because the command was piped into `cut`, the pipeline exit status was `cut`'s, so the failure looked like success with empty output and every orphan was reported as 0 bytes. `stat -c %Y` is likewise rejected by BSD `stat`, so each orphan's `created_at` fell back to `datetime.now()` and looked brand-new, meaning age-based collection never ran on macOS. + +Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it reads the local filesystem for local hosts and runs POSIX `du -sk` over SSH for remote ones, so callers never branch on `is_local`. Remote sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines. diff --git a/libs/mngr/imbue/mngr/agents/tui_utils.py b/libs/mngr/imbue/mngr/agents/tui_utils.py index e765946e0e..1fa8407c46 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils.py @@ -301,19 +301,36 @@ def _send_enter_and_wait_for_signal( def _build_signal_only_command(full_timeout: float, wait_channel: str, tmux_target: TmuxWindowTarget) -> str: - """The original behavior: send Enter, then block on the hook's wait-for channel. - - Used for TUIs with no acceptance-marker command to watch. The waiter is started (in the - foreground here) before Enter is sent from a backgrounded subshell, so the - signal cannot fire before a waiter is registered (signals wake exactly one - waiter; a signal with none registered is lost). + """Send Enter, then block on the hook's wait-for channel until it fires or the deadline passes. + + Used for TUIs with no acceptance-marker command to watch. The waiter is + started before Enter is sent from a backgrounded subshell, so it is + registered by the time the hook can fire. Exit 0 = signalled, non-zero = + deadline reached. + + The deadline is a backgrounded ``sleep``-then-``kill`` watchdog rather than + ``timeout(1)``, which is GNU coreutils and absent on macOS. Three properties + the watchdog has to reproduce by hand: + + - ``tmux wait-for`` exits 0 whether it was signalled or killed, so the + watchdog marks ``$tmo`` *before* killing and that marker -- not the wait + status -- decides the exit code, the way ``timeout`` returns 124. + - The waiter must be the tmux client itself, not a wrapper subshell, or the + kill hits the subshell and leaves the client running forever. + - Background jobs redirect stdout, because a job that inherits it holds the + caller's stdout open until it exits, stalling every submission for the + full deadline even when the signal lands immediately. """ - return ( - f"bash -c '" - f'( sleep 0.1 && tmux send-keys -t "$1" Enter ) & ' - f'timeout {full_timeout} tmux wait-for "$0"' - f"' {shlex.quote(wait_channel)} {tmux_target.as_shell_arg()}" + script = ( + 'tmo="$(mktemp)"; ' + 'trap \'kill "$waiter" "$watchdog" 2>/dev/null; rm -f "$tmo"\' EXIT; ' + 'tmux wait-for "$2" & waiter=$!; ' + '( sleep "$1"; echo 1 > "$tmo"; kill "$waiter" ) >/dev/null 2>&1 & watchdog=$!; ' + '( sleep 0.1 && tmux send-keys -t "$3" Enter ) >/dev/null 2>&1 & ' + 'wait "$waiter" 2>/dev/null; ' + '[ ! -s "$tmo" ]' ) + return f"bash -c {shlex.quote(script)} _ {full_timeout} {shlex.quote(wait_channel)} {tmux_target.as_shell_arg()}" def _build_signal_or_marker_command( @@ -325,13 +342,21 @@ def _build_signal_or_marker_command( """Succeed as soon as EITHER the hook signal fires OR a fresh acceptance marker appears. A single remote command so the two conditions are watched concurrently with - no dangling process: it registers the (full-timeout) hook waiter in the - background -- which writes a sentinel file on success, preserving the - register-before-Enter ordering so the signal is never missed (which matters - for submissions that only ever fire the signal, never recording a marker) - -- sends Enter, then polls both the sentinel and the acceptance marker until - either confirms or the deadline passes. Exit 0 = confirmed, non-zero = - timeout. + no dangling process: it registers the hook waiter in the background -- which + writes a sentinel file on success, preserving the register-before-Enter + ordering so the signal is never missed (which matters for submissions that + only ever fire the signal, never recording a marker) -- sends Enter, then + polls both the sentinel and the acceptance marker until either confirms or + the deadline passes. Exit 0 = confirmed, non-zero = timeout. + + The hook waiter is bounded by a ``sleep``-then-``kill`` watchdog rather than + ``timeout(1)`` (GNU coreutils, absent on macOS). See + :func:`_build_signal_only_command` for the three properties that hand-rolled + watchdog has to reproduce; the one that shapes this variant is that the + waiter must be the tmux client itself, so the loop detects a fired hook by + the client having exited (``kill -0``) without the watchdog's ``$tmo`` + marker, rather than by wrapping the client in a subshell that writes a + sentinel. ``accept_marker_command`` is the agent-supplied shell snippet that prints the agent's latest acceptance-marker token (empty if none yet). A baseline is @@ -343,20 +368,19 @@ def _build_signal_or_marker_command( agent that supplies it. """ script = ( - 'sig="$(mktemp)"; ' - # Clean up on every exit path: remove the sentinel file and reap any - # still-running background job (notably the hook waiter, which otherwise - # outlives a fast marker-win and would recreate "$sig" -- leaking the - # temp file -- when the hook finally fires). Runs exactly once on exit. - 'trap \'p="$(jobs -p)"; [ -n "$p" ] && kill $p 2>/dev/null; rm -f "$sig"\' EXIT; ' + 'tmo="$(mktemp)"; ' + # Clean up on every exit path: kill the tmux client and the watchdog + # (either may outlive a fast marker-win) and remove the marker file. + 'trap \'kill "$waiter" "$watchdog" 2>/dev/null; rm -f "$tmo"\' EXIT; ' f'base="$({accept_marker_command})"; ' - # Register the hook waiter first (full timeout), sentinel on success. - f'( timeout {full_timeout} tmux wait-for "$1" >/dev/null 2>&1 && echo 1 > "$sig" ) & ' + # Register the hook waiter first, bounded by the watchdog. + 'tmux wait-for "$1" & waiter=$!; ' + f'( sleep {full_timeout}; echo 1 > "$tmo"; kill "$waiter" ) >/dev/null 2>&1 & watchdog=$!; ' # Then submit, after a beat so the waiter is registered. - '( sleep 0.1 && tmux send-keys -t "$2" Enter ) & ' + '( sleep 0.1 && tmux send-keys -t "$2" Enter ) >/dev/null 2>&1 & ' f'end="$(( $(date +%s) + {int(full_timeout) + 1} ))"; ' 'while [ "$(date +%s)" -lt "$end" ]; do ' - 'if [ -s "$sig" ]; then exit 0; fi; ' + 'if ! kill -0 "$waiter" 2>/dev/null && [ ! -s "$tmo" ]; then exit 0; fi; ' f'cur="$({accept_marker_command})"; ' 'if [[ -n "$cur" && "$cur" > "$base" ]]; then exit 0; fi; ' "sleep 0.25; " diff --git a/libs/mngr/imbue/mngr/agents/tui_utils_test.py b/libs/mngr/imbue/mngr/agents/tui_utils_test.py index 23ebca8053..fd11751a7c 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils_test.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils_test.py @@ -8,6 +8,8 @@ import pytest from imbue.mngr.agents.base_agent import BaseAgent +from imbue.mngr.agents.tui_utils import _build_signal_only_command +from imbue.mngr.agents.tui_utils import _build_signal_or_marker_command from imbue.mngr.agents.tui_utils import _check_paste_content from imbue.mngr.agents.tui_utils import _normalize_for_match from imbue.mngr.agents.tui_utils import send_enter_and_poll_for_cleared_indicator @@ -254,3 +256,54 @@ def test_send_enter_via_hook_raises_on_timeout(signal_agent: _ProbeAgent) -> Non f"tmux kill-session -t '={session_name}' 2>/dev/null", timeout_seconds=5.0, ) + + +# === Submission command builders === + +_TARGET = TmuxWindowTarget(session_name="sess", window=0) + + +@pytest.mark.parametrize( + "command", + [ + _build_signal_only_command(2.0, "chan", _TARGET), + _build_signal_or_marker_command(2.0, "chan", _TARGET, "printf ''"), + ], +) +def test_submission_commands_invoke_no_gnu_only_binary(command: str) -> None: + """Both builders run on the agent's host, which for a local agent is the user's machine. + + macOS ships a BSD userland with no ``timeout``, so the deadline has to be a + hand-rolled watchdog. + """ + assert "timeout" not in command + + +@pytest.mark.parametrize( + "command", + [ + _build_signal_only_command(2.0, "chan", _TARGET), + _build_signal_or_marker_command(2.0, "chan", _TARGET, "printf ''"), + ], +) +def test_submission_commands_wait_on_the_channel_and_send_to_the_target(command: str) -> None: + """The channel reaches ``tmux wait-for`` and the target reaches ``tmux send-keys``, not vice versa.""" + assert "chan" in command + assert _TARGET.as_shell_arg() in command + channel_index = command.index("chan") + target_index = command.index(_TARGET.as_shell_arg()) + assert channel_index < target_index + + +def test_signal_only_command_bounds_the_waiter_with_a_watchdog() -> None: + """A killed ``tmux wait-for`` exits 0, so the exit code must come from the watchdog's marker.""" + command = _build_signal_only_command(2.0, "chan", _TARGET) + assert 'kill "$waiter"' in command + assert '[ ! -s "$tmo" ]' in command + + +def test_signal_or_marker_command_kills_the_tmux_client_not_a_wrapper_subshell() -> None: + """The waiter must be the tmux client itself, or killing it leaves the client running forever.""" + command = _build_signal_or_marker_command(2.0, "chan", _TARGET, "printf ''") + assert 'tmux wait-for "$1" & waiter=$!' in command + assert 'kill "$waiter" "$watchdog"' in command diff --git a/libs/mngr/imbue/mngr/api/gc.py b/libs/mngr/imbue/mngr/api/gc.py index 0e25602cfc..3d84efc44a 100644 --- a/libs/mngr/imbue/mngr/api/gc.py +++ b/libs/mngr/imbue/mngr/api/gc.py @@ -809,24 +809,8 @@ def _get_orphaned_work_dirs(host: OnlineHostInterface, provider_name: ProviderIn work_dir_infos = [] for work_dir_str in orphaned_work_dirs: work_dir_path = Path(work_dir_str) - # Get size if possible - size = SizeBytes(0) - try: - result = host.execute_idempotent_command(f"du -sb {shlex.quote(str(work_dir_path))} | cut -f1") - if result.success and result.stdout.strip(): - size = SizeBytes(int(result.stdout.strip())) - except (ValueError, OSError): - # If we can't get the size, use 0 - pass - - # Get creation time from the directory - created_at = datetime.now(timezone.utc) - try: - stat_result = host.execute_idempotent_command(f"stat -c %Y {shlex.quote(str(work_dir_path))}") - if stat_result.success and stat_result.stdout.strip(): - created_at = datetime.fromtimestamp(int(stat_result.stdout.strip()), tz=timezone.utc) - except (ValueError, OSError): - pass + size = host.get_directory_size(work_dir_path) + created_at = host.get_file_mtime(work_dir_path) or datetime.now(timezone.utc) work_dir_infos.append( WorkDirInfo( @@ -1008,21 +992,8 @@ def _local_branches_not_on_any_remote_on_host(host: OnlineHostInterface, repo_pa def _build_source_dir_info( host: OnlineHostInterface, provider_name: ProviderInstanceName, source_path: Path ) -> WorkDirInfo: - size = SizeBytes(0) - try: - result = host.execute_idempotent_command(f"du -sb {shlex.quote(str(source_path))} | cut -f1") - if result.success and result.stdout.strip(): - size = SizeBytes(int(result.stdout.strip())) - except (ValueError, OSError): - pass - - created_at = datetime.now(timezone.utc) - try: - stat_result = host.execute_idempotent_command(f"stat -c %Y {shlex.quote(str(source_path))}") - if stat_result.success and stat_result.stdout.strip(): - created_at = datetime.fromtimestamp(int(stat_result.stdout.strip()), tz=timezone.utc) - except (ValueError, OSError): - pass + size = host.get_directory_size(source_path) + created_at = host.get_file_mtime(source_path) or datetime.now(timezone.utc) return WorkDirInfo( path=source_path, diff --git a/libs/mngr/imbue/mngr/conftest.py b/libs/mngr/imbue/mngr/conftest.py index 7b24332a92..77819a03c8 100644 --- a/libs/mngr/imbue/mngr/conftest.py +++ b/libs/mngr/imbue/mngr/conftest.py @@ -788,3 +788,44 @@ def session_cleanup() -> Generator[None, None, None]: "These resources have been cleaned up, but tests should not leak!\n" "Please fix the test(s) that failed to clean up properly." ) + + +# Binaries that exist in GNU coreutils but not in the BSD userland macOS ships. +_GNU_ONLY_BINARIES: Final[tuple[str, ...]] = ("tac", "timeout") + +# macOS ships bash 3.2, which has no associative arrays (`declare -A`). +_BASH_CANDIDATES: Final[tuple[str, ...]] = ("bash", "/opt/homebrew/bin/bash", "/usr/local/bin/bash") +_MINIMUM_BASH_MAJOR_VERSION: Final[int] = 4 + + +@pytest.fixture +def posix_only_path(tmp_path_factory: pytest.TempPathFactory) -> dict[str, str]: + """An environment whose PATH shadows GNU-coreutils-only binaries with shims that exit 127. + + mngr must run on stock macOS, whose BSD userland has no ``tac`` or ``timeout``. + Poisoning them means a reintroduced dependency fails on Linux too, rather than + passing in CI and breaking only on a user's mac. + """ + poison_dir = tmp_path_factory.mktemp("posix_only_bin") + for binary in _GNU_ONLY_BINARIES: + shim = poison_dir / binary + shim.write_text(f'#!/bin/sh\necho "{binary}: command not found" >&2\nexit 127\n') + shim.chmod(0o755) + env = dict(os.environ) + env["PATH"] = f"{poison_dir}{os.pathsep}{env['PATH']}" + return env + + +@pytest.fixture +def bash_with_associative_arrays() -> str: + """Path to a bash new enough for ``declare -A``, which the shared shell libs require.""" + for candidate in _BASH_CANDIDATES: + resolved = shutil.which(candidate) + if resolved is None: + continue + probe = subprocess.run( + [resolved, "-c", "echo ${BASH_VERSINFO[0]}"], capture_output=True, text=True, timeout=30 + ) + if probe.returncode == 0 and int(probe.stdout.strip()) >= _MINIMUM_BASH_MAJOR_VERSION: + return resolved + pytest.skip(f"no bash >= {_MINIMUM_BASH_MAJOR_VERSION} found (macOS ships 3.2; try `brew install bash`)") diff --git a/libs/mngr/imbue/mngr/interfaces/host.py b/libs/mngr/imbue/mngr/interfaces/host.py index 23c0bfb8c9..abc008e54c 100644 --- a/libs/mngr/imbue/mngr/interfaces/host.py +++ b/libs/mngr/imbue/mngr/interfaces/host.py @@ -1,6 +1,7 @@ from __future__ import annotations import shlex +import stat from abc import ABC from abc import abstractmethod from contextlib import contextmanager @@ -26,6 +27,7 @@ from imbue.mngr.interfaces.data_types import HostLifecycleOptions from imbue.mngr.interfaces.data_types import HostResources from imbue.mngr.interfaces.data_types import PyinfraConnector +from imbue.mngr.interfaces.data_types import SizeBytes from imbue.mngr.interfaces.data_types import SnapshotInfo from imbue.mngr.interfaces.data_types import VolumeFile from imbue.mngr.primitives import ActivitySource @@ -427,6 +429,34 @@ def path_exists(self, path: Path) -> bool: return path.exists() return self.execute_idempotent_command(f"test -e {shlex.quote(str(path))}", timeout_seconds=5.0).success + def get_directory_size(self, path: Path) -> SizeBytes: + """Total size of the regular files under ``path``, or 0 if it does not exist. + + Uses the local filesystem for local hosts and ``du -sk`` over SSH for + remote hosts. Implemented on the interface so callers (including plugins) + don't have to branch on ``is_local`` themselves. + + ``-k`` is the only ``du`` block size POSIX defines, so remote sizes are + whole kibibytes; local sizes are exact. Entries that vanish mid-walk are + skipped, since callers scan directories that may still be in use. + """ + if self.is_local: + if not path.is_dir(): + return SizeBytes(0) + total = 0 + for entry in path.rglob("*"): + try: + entry_stat = entry.lstat() + except FileNotFoundError: + continue + if stat.S_ISREG(entry_stat.st_mode): + total += entry_stat.st_size + return SizeBytes(total) + result = self.execute_idempotent_command(f"du -sk {shlex.quote(str(path))}", timeout_seconds=30.0) + if not result.success or not result.stdout.strip(): + return SizeBytes(0) + return SizeBytes(int(result.stdout.split()[0]) * 1024) + class OnlineHostInterface(HostInterface, OuterHostInterface, ABC): """Interface for hosts that are currently online and accessible for operations. diff --git a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh index 3e996674d9..6a66e1eb01 100644 --- a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh +++ b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh @@ -22,7 +22,9 @@ # Echo the largest line offset N such that lines 1..N of SESSION_FILE # have FIELD values already present in _MNGR_TRANSCRIPT_ID_SET. Used to # recover after a crash that may have dropped the stored offset. Echoes -# 0 if no match is found or the file is empty. +# 0 if no match is found or the file is empty. Scans forward rather than +# reversing the file, so it needs no `tac` (GNU coreutils, absent on +# macOS). # # - mngr_transcript_emit_lines_range SESSION_FILE START END OUTPUT_FILE # Append lines START..END (inclusive, 1-indexed) of SESSION_FILE to @@ -74,11 +76,10 @@ mngr_transcript_build_id_set() { done < "$output_file" } -# Reverse-scan SESSION_FILE to find the last emitted line. +# Scan SESSION_FILE to find the last emitted line. # -# Walks the file backwards (via `tac`) and returns the 1-indexed line number -# whose FIELD value is already in _MNGR_TRANSCRIPT_ID_SET. Returns 0 if no -# already-emitted line is found. +# Returns the highest 1-indexed line number whose FIELD value is already in +# _MNGR_TRANSCRIPT_ID_SET, or 0 if no already-emitted line is found. mngr_transcript_reconcile_offset() { local session_file="$1" local field="$2" @@ -88,23 +89,18 @@ mngr_transcript_reconcile_offset() { return 0 fi - local file_lines - file_lines=$(wc -l < "$session_file") - file_lines=${file_lines// /} - - local reverse_idx=0 - local line value found + local idx=0 + local found=0 + local line value while IFS= read -r line; do - reverse_idx=$((reverse_idx + 1)) + idx=$((idx + 1)) value=$(mngr_transcript_extract_field "$field" "$line") if [ -n "$value" ] && [ "${_MNGR_TRANSCRIPT_ID_SET[$value]+exists}" ]; then - found=$((file_lines - reverse_idx + 1)) - echo "$found" - return 0 + found=$idx fi - done < <(tac "$session_file") + done < "$session_file" - echo 0 + echo "$found" } # Append lines START..END of SESSION_FILE to OUTPUT_FILE. diff --git a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py new file mode 100644 index 0000000000..2b06244f7f --- /dev/null +++ b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py @@ -0,0 +1,94 @@ +"""Unit tests for mngr_transcript_lib.sh. + +Every test runs the library with GNU-only binaries poisoned on PATH (the +``posix_only_path`` fixture), so a reintroduced ``tac`` dependency fails here on +Linux too, not only on macOS. +""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path + +import pytest + +_LIB = Path(__file__).parent / "mngr_transcript_lib.sh" + + +def _write_jsonl(path: Path, uuids: list[str]) -> None: + path.write_text("".join(json.dumps({"uuid": uuid}) + "\n" for uuid in uuids)) + + +def _reconcile_offset(bash: str, env: dict[str, str], session_file: Path, output_file: Path) -> int: + script = f""" + source {_LIB} + declare -A _MNGR_TRANSCRIPT_ID_SET + mngr_transcript_build_id_set {output_file} uuid + mngr_transcript_reconcile_offset {session_file} uuid + """ + result = subprocess.run([bash, "-c", script], capture_output=True, text=True, env=env, timeout=30) + assert result.returncode == 0, f"lib exited {result.returncode}: {result.stderr}" + assert result.stderr == "", f"lib wrote to stderr (a missing binary?): {result.stderr}" + return int(result.stdout.strip()) + + +@pytest.mark.parametrize("emitted_count", [0, 1, 2, 3, 4, 5]) +def test_reconcile_offset_returns_the_last_already_emitted_line( + tmp_path: Path, posix_only_path: dict[str, str], bash_with_associative_arrays: str, emitted_count: int +) -> None: + """The offset is the 1-indexed line of the last session entry already present in the output file.""" + uuids = [f"id-{index}" for index in range(1, 6)] + session_file = tmp_path / "session.jsonl" + output_file = tmp_path / "output.jsonl" + _write_jsonl(session_file, uuids) + _write_jsonl(output_file, uuids[:emitted_count]) + + offset = _reconcile_offset(bash_with_associative_arrays, posix_only_path, session_file, output_file) + assert offset == emitted_count + + +def test_reconcile_offset_ignores_an_unterminated_final_line( + tmp_path: Path, posix_only_path: dict[str, str], bash_with_associative_arrays: str +) -> None: + """A session file whose last line is still being appended reconciles to the last complete line. + + ``mngr_transcript_build_id_set`` and ``mngr_transcript_emit_lines_range`` both + ignore a final line with no trailing newline, so the offset must agree. + """ + session_file = tmp_path / "session.jsonl" + output_file = tmp_path / "output.jsonl" + session_file.write_text('{"uuid": "id-1"}\n{"uuid": "id-2"}') + output_file.write_text('{"uuid": "id-1"}\n{"uuid": "id-2"}') + + offset = _reconcile_offset(bash_with_associative_arrays, posix_only_path, session_file, output_file) + assert offset == 1 + + +def test_reconcile_offset_is_zero_when_nothing_was_emitted( + tmp_path: Path, posix_only_path: dict[str, str], bash_with_associative_arrays: str +) -> None: + session_file = tmp_path / "session.jsonl" + output_file = tmp_path / "output.jsonl" + _write_jsonl(session_file, ["id-1", "id-2"]) + output_file.write_text("") + + offset = _reconcile_offset(bash_with_associative_arrays, posix_only_path, session_file, output_file) + assert offset == 0 + + +def test_reconcile_offset_returns_the_highest_matching_line_not_the_first( + tmp_path: Path, posix_only_path: dict[str, str], bash_with_associative_arrays: str +) -> None: + """With a gap in the emitted set, the offset still advances to the last match. + + The reverse scan this replaced stopped at the first match from the end, which is + the same line; a forward scan must not stop at the first match from the start. + """ + session_file = tmp_path / "session.jsonl" + output_file = tmp_path / "output.jsonl" + _write_jsonl(session_file, ["id-1", "id-2", "id-3"]) + _write_jsonl(output_file, ["id-1", "id-3"]) + + offset = _reconcile_offset(bash_with_associative_arrays, posix_only_path, session_file, output_file) + assert offset == 3 From 9bbf4898f3e59917d5fa21dbde07f715f0847c46 Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Thu, 9 Jul 2026 22:42:48 -0700 Subject: [PATCH 02/14] Fix CI, remove per-line forks, and verify the impact by execution tui_agent_test asserted `"mktemp" not in command` for the signal-only path as a proxy for "uses no sentinel file". The watchdog legitimately needs one: a `tmux wait-for` client exits 0 whether signalled or killed, so a marker file, not the wait status, must decide the exit code. Assert on the absence of a polling loop instead, which is what actually distinguishes the two paths. Remove the per-line subshell fork from both `mngr_transcript_reconcile_offset` and `mngr_transcript_build_id_set`. Each extracted the correlation field via a command substitution, forking once per line. Matching the bash regex inline forks nothing. Reconciling a 50k-line session file: 29.9s -> 1.5s. Without this the forward scan would have been a regression, since `tac` let the old reverse scan stop at the first match from the end -- measured 5.6s -> 11.0s at 10k lines before this change, and 5.4s -> 0.3s after. `get_directory_size` walked with `Path.rglob` + `lstat`, 5x slower than the `du -sb` it replaced (0.600s vs 0.124s over 38k entries). `os.walk` closes that to 1.5x (0.193s) for identical bytes. Also guard the remote branch with `test -d`, so a non-directory reports 0 on both sides rather than only locally. Add two real-tmux regression tests that pin the impact rather than infer it. Against the pre-fix tui_utils.py on macOS both fail with `SendMessageError: Timeout waiting for message submission signal`, though the hook fired: the signal-only path (what codex and antigravity call) and the marker path with a frozen marker (the shape of Claude's /clear and /compact, which fire the hook but never enqueue a model turn). Co-Authored-By: Claude Opus 4.8 (1M context) --- libs/mngr/changelog/mngr-timeout-tac.md | 4 +- libs/mngr/imbue/mngr/agents/tui_agent_test.py | 6 +-- libs/mngr/imbue/mngr/agents/tui_utils_test.py | 39 +++++++++++++++++++ libs/mngr/imbue/mngr/interfaces/host.py | 21 ++++++---- .../mngr/resources/mngr_transcript_lib.sh | 35 +++++++++++------ 5 files changed, 81 insertions(+), 24 deletions(-) diff --git a/libs/mngr/changelog/mngr-timeout-tac.md b/libs/mngr/changelog/mngr-timeout-tac.md index ffc38f5b34..904a273fec 100644 --- a/libs/mngr/changelog/mngr-timeout-tac.md +++ b/libs/mngr/changelog/mngr-timeout-tac.md @@ -4,6 +4,8 @@ Fixed four places where mngr shelled out to GNU-coreutils binaries and flags tha Raw-transcript streaming re-emitted already-emitted lines on macOS. `mngr_transcript_reconcile_offset` reverse-scanned the session file with `tac` to find the last emitted line; with `tac` absent the scan read nothing and the offset reset to 0. It now scans forward and tracks the last match, which needs no reverse at all. This also fixes a latent off-by-one: the old code combined `wc -l` (which does not count an unterminated final line) with `tac` (which emits it), so a session file whose last line was still being appended -- the normal state of a live transcript -- reconciled one line too low. +Transcript-streamer startup got roughly 20x faster as part of that change. Both `mngr_transcript_reconcile_offset` and `mngr_transcript_build_id_set` extracted each line's correlation field through a command substitution, which forks a subshell per line. They now match the field with a bash regex inline, forking nothing. Reconciling a 50,000-line session file drops from about 30 seconds to about 1.5 seconds; without this the forward scan would have been slower than the reverse scan it replaced, since the old code's `tac` let it stop at the first match from the end. + `mngr gc` mis-measured orphaned work directories on macOS. `du -sb` has no BSD equivalent (`-b` is rejected), and because the command was piped into `cut`, the pipeline exit status was `cut`'s, so the failure looked like success with empty output and every orphan was reported as 0 bytes. `stat -c %Y` is likewise rejected by BSD `stat`, so each orphan's `created_at` fell back to `datetime.now()` and looked brand-new, meaning age-based collection never ran on macOS. -Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it reads the local filesystem for local hosts and runs POSIX `du -sk` over SSH for remote ones, so callers never branch on `is_local`. Remote sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines. +Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it walks the local filesystem for local hosts and runs POSIX `test -d ... && du -sk` over SSH for remote ones, so callers never branch on `is_local`. Remote sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines, and a path that is not a directory reports 0 on both sides. diff --git a/libs/mngr/imbue/mngr/agents/tui_agent_test.py b/libs/mngr/imbue/mngr/agents/tui_agent_test.py index 9dbeb6635e..8b20b834aa 100644 --- a/libs/mngr/imbue/mngr/agents/tui_agent_test.py +++ b/libs/mngr/imbue/mngr/agents/tui_agent_test.py @@ -123,11 +123,11 @@ def test_send_enter_waits_on_hook_only_without_a_marker_command() -> None: accept_marker_command=None, ) assert result is True - # Exactly one host round-trip, and it waits on the hook with no marker probe - # (the signal-only path uses no sentinel file). + # Exactly one host round-trip, and it blocks on the hook rather than polling + # (no marker to poll for). assert len(commands) == 1 assert "tmux wait-for" in commands[0] - assert "mktemp" not in commands[0] + assert "while" not in commands[0] def test_send_enter_watches_marker_and_hook_concurrently_with_a_marker_command() -> None: diff --git a/libs/mngr/imbue/mngr/agents/tui_utils_test.py b/libs/mngr/imbue/mngr/agents/tui_utils_test.py index fd11751a7c..caf237015f 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils_test.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils_test.py @@ -307,3 +307,42 @@ def test_signal_or_marker_command_kills_the_tmux_client_not_a_wrapper_subshell() command = _build_signal_or_marker_command(2.0, "chan", _TARGET, "printf ''") assert 'tmux wait-for "$1" & waiter=$!' in command assert 'kill "$waiter" "$watchdog"' in command + + +@pytest.mark.tmux +def test_send_enter_via_hook_confirms_on_the_hook_when_the_marker_never_advances( + signal_agent: _ProbeAgent, +) -> None: + """With a marker command, a submission that records no marker still confirms via the hook. + + This is the shape of Claude's ``/clear`` and ``/compact``: the TUI fires the + submit hook but never enqueues a model turn, so the acceptance marker never + moves and only the hook can confirm. + """ + session_name = f"{signal_agent.mngr_ctx.config.prefix}{signal_agent.name}" + tmux_target = TmuxWindowTarget(session_name=session_name, window=0) + wait_channel = f"mngr-submit-marker-never-moves-{session_name}" + + signal_agent.host.execute_idempotent_command( + f"tmux new-session -d -s '{session_name}' 'bash'", + timeout_seconds=5.0, + ) + try: + signal_agent.host.execute_idempotent_command( + f"( sleep 0.1 && tmux wait-for -S '{wait_channel}' ) &", + timeout_seconds=1.0, + ) + send_enter_via_tmux_wait_for_hook( + signal_agent, + tmux_target, + wait_channel=wait_channel, + timeout_seconds=3.0, + # A constant token never sorts after its own baseline, so the marker + # can never be what confirms this submission. + accept_marker_command="printf 'frozen-marker'", + ) + finally: + signal_agent.host.execute_idempotent_command( + f"tmux kill-session -t '={session_name}' 2>/dev/null", + timeout_seconds=5.0, + ) diff --git a/libs/mngr/imbue/mngr/interfaces/host.py b/libs/mngr/imbue/mngr/interfaces/host.py index abc008e54c..6152031ddb 100644 --- a/libs/mngr/imbue/mngr/interfaces/host.py +++ b/libs/mngr/imbue/mngr/interfaces/host.py @@ -1,5 +1,6 @@ from __future__ import annotations +import os import shlex import stat from abc import ABC @@ -444,15 +445,19 @@ def get_directory_size(self, path: Path) -> SizeBytes: if not path.is_dir(): return SizeBytes(0) total = 0 - for entry in path.rglob("*"): - try: - entry_stat = entry.lstat() - except FileNotFoundError: - continue - if stat.S_ISREG(entry_stat.st_mode): - total += entry_stat.st_size + for dir_path, _dir_names, file_names in os.walk(path): + for file_name in file_names: + try: + entry_stat = os.lstat(os.path.join(dir_path, file_name)) + except OSError: + continue + if stat.S_ISREG(entry_stat.st_mode): + total += entry_stat.st_size return SizeBytes(total) - result = self.execute_idempotent_command(f"du -sk {shlex.quote(str(path))}", timeout_seconds=30.0) + quoted_path = shlex.quote(str(path)) + result = self.execute_idempotent_command( + f"test -d {quoted_path} && du -sk {quoted_path}", timeout_seconds=30.0 + ) if not result.success or not result.stdout.strip(): return SizeBytes(0) return SizeBytes(int(result.stdout.split()[0]) * 1024) diff --git a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh index 6a66e1eb01..40a2f7e0b5 100644 --- a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh +++ b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh @@ -14,9 +14,7 @@ # - mngr_transcript_build_id_set OUTPUT_FILE FIELD # Populate the associative array _MNGR_TRANSCRIPT_ID_SET with every value # of FIELD found in OUTPUT_FILE. Used at startup and when a late- -# appearing session file needs reconciliation. Iterates line-by-line via -# mngr_transcript_extract_field so the extraction is consistent with the -# per-line lookup in mngr_transcript_reconcile_offset. +# appearing session file needs reconciliation. # # - mngr_transcript_reconcile_offset SESSION_FILE FIELD # Echo the largest line offset N such that lines 1..N of SESSION_FILE @@ -42,6 +40,16 @@ # (so it survives function-local scope) and for clearing it when no longer # needed. +# Set _MNGR_TRANSCRIPT_FIELD_PATTERN to the ERE matching a top-level +# "": "" pair, capturing the value. +# +# The per-line loops below match with this pattern directly rather than calling +# mngr_transcript_extract_field, because a command substitution forks a subshell +# per line -- prohibitive on a session file with tens of thousands of lines. +_mngr_transcript_set_field_pattern() { + _MNGR_TRANSCRIPT_FIELD_PATTERN="\"$1\"[[:space:]]*:[[:space:]]*\"([^\"]*)\"" +} + # Extract a top-level JSON string field from a single line. # # Limitations: matches the *first* "": "" in the line. Nested @@ -52,8 +60,8 @@ mngr_transcript_extract_field() { local field="$1" local line="$2" - local pattern="\"${field}\"[[:space:]]*:[[:space:]]*\"([^\"]*)\"" - if [[ "$line" =~ $pattern ]]; then + _mngr_transcript_set_field_pattern "$field" + if [[ "$line" =~ $_MNGR_TRANSCRIPT_FIELD_PATTERN ]]; then printf '%s\n' "${BASH_REMATCH[1]}" fi return 0 @@ -67,11 +75,11 @@ mngr_transcript_build_id_set() { if [ ! -s "$output_file" ]; then return 0 fi - local line value + _mngr_transcript_set_field_pattern "$field" + local line while IFS= read -r line; do - value=$(mngr_transcript_extract_field "$field" "$line") - if [ -n "$value" ]; then - _MNGR_TRANSCRIPT_ID_SET["$value"]=1 + if [[ "$line" =~ $_MNGR_TRANSCRIPT_FIELD_PATTERN ]] && [ -n "${BASH_REMATCH[1]}" ]; then + _MNGR_TRANSCRIPT_ID_SET["${BASH_REMATCH[1]}"]=1 fi done < "$output_file" } @@ -89,14 +97,17 @@ mngr_transcript_reconcile_offset() { return 0 fi + _mngr_transcript_set_field_pattern "$field" local idx=0 local found=0 local line value while IFS= read -r line; do idx=$((idx + 1)) - value=$(mngr_transcript_extract_field "$field" "$line") - if [ -n "$value" ] && [ "${_MNGR_TRANSCRIPT_ID_SET[$value]+exists}" ]; then - found=$idx + if [[ "$line" =~ $_MNGR_TRANSCRIPT_FIELD_PATTERN ]]; then + value="${BASH_REMATCH[1]}" + if [ -n "$value" ] && [ "${_MNGR_TRANSCRIPT_ID_SET[$value]+exists}" ]; then + found=$idx + fi fi done < "$session_file" From cb5c240ac30ee711f4bc6a82f1acbd5602694e04 Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Thu, 9 Jul 2026 22:50:37 -0700 Subject: [PATCH 03/14] Do not report a failed tmux wait-for as a successful submission The watchdog dropped the waiter's exit status entirely, because a TERM'd `tmux wait-for` exits 0 and so cannot distinguish "signalled" from "killed". But that status is only meaningless when the watchdog did the killing. When `tmux wait-for` exits on its own it is meaningful, and non-zero means it failed outright -- no server, dead session, bad channel. Discarding it made both builders exit 0 in ~15ms against a dead tmux server, so `send_enter_via_tmux_wait_for_hook` reported a message as submitted when nothing had been. `timeout(1)` never had this problem: it forwards the child's status and only overrides it on expiry. The deadline marker now decides expiry and the wait status decides outright failure. In the marker variant, a waiter that dies without confirming no longer ends the poll -- the acceptance marker can still confirm, so the loop keeps watching it until the deadline, matching the previous behavior. Regression tests drive both built commands against a `tmux` shim that exits 1 and assert a non-zero result; both fail against the previous commit. Co-Authored-By: Claude Opus 4.8 (1M context) --- libs/mngr/changelog/mngr-timeout-tac.md | 2 ++ libs/mngr/imbue/mngr/agents/tui_utils.py | 25 +++++++++------ libs/mngr/imbue/mngr/agents/tui_utils_test.py | 31 +++++++++++++++++++ 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/libs/mngr/changelog/mngr-timeout-tac.md b/libs/mngr/changelog/mngr-timeout-tac.md index 904a273fec..80d71aabcf 100644 --- a/libs/mngr/changelog/mngr-timeout-tac.md +++ b/libs/mngr/changelog/mngr-timeout-tac.md @@ -9,3 +9,5 @@ Transcript-streamer startup got roughly 20x faster as part of that change. Both `mngr gc` mis-measured orphaned work directories on macOS. `du -sb` has no BSD equivalent (`-b` is rejected), and because the command was piped into `cut`, the pipeline exit status was `cut`'s, so the failure looked like success with empty output and every orphan was reported as 0 bytes. `stat -c %Y` is likewise rejected by BSD `stat`, so each orphan's `created_at` fell back to `datetime.now()` and looked brand-new, meaning age-based collection never ran on macOS. Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it walks the local filesystem for local hosts and runs POSIX `test -d ... && du -sk` over SSH for remote ones, so callers never branch on `is_local`. Remote sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines, and a path that is not a directory reports 0 on both sides. + +A submission whose `tmux wait-for` fails outright -- no tmux server, a dead session -- is no longer reported as successfully submitted. `timeout(1)` passed the client's exit status through when the client exited on its own, and only overrode it on expiry; the hand-rolled watchdog now does the same, using the deadline marker to detect expiry and the wait status to detect an outright failure. diff --git a/libs/mngr/imbue/mngr/agents/tui_utils.py b/libs/mngr/imbue/mngr/agents/tui_utils.py index 1fa8407c46..185c83f09d 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils.py @@ -312,11 +312,13 @@ def _build_signal_only_command(full_timeout: float, wait_channel: str, tmux_targ ``timeout(1)``, which is GNU coreutils and absent on macOS. Three properties the watchdog has to reproduce by hand: - - ``tmux wait-for`` exits 0 whether it was signalled or killed, so the - watchdog marks ``$tmo`` *before* killing and that marker -- not the wait - status -- decides the exit code, the way ``timeout`` returns 124. - The waiter must be the tmux client itself, not a wrapper subshell, or the kill hits the subshell and leaves the client running forever. + - A killed ``tmux wait-for`` exits 0, so the wait status alone cannot mean + success. The watchdog writes ``$tmo`` *before* killing, and a non-empty + ``$tmo`` means the deadline passed, the way ``timeout`` returns 124. The + wait status still decides the rest: ``tmux wait-for`` exits non-zero when + it fails outright (no server, dead session), which is not a submission. - Background jobs redirect stdout, because a job that inherits it holds the caller's stdout open until it exits, stalling every submission for the full deadline even when the signal lands immediately. @@ -327,8 +329,9 @@ def _build_signal_only_command(full_timeout: float, wait_channel: str, tmux_targ 'tmux wait-for "$2" & waiter=$!; ' '( sleep "$1"; echo 1 > "$tmo"; kill "$waiter" ) >/dev/null 2>&1 & watchdog=$!; ' '( sleep 0.1 && tmux send-keys -t "$3" Enter ) >/dev/null 2>&1 & ' - 'wait "$waiter" 2>/dev/null; ' - '[ ! -s "$tmo" ]' + 'wait "$waiter" 2>/dev/null; waited=$?; ' + 'kill "$watchdog" 2>/dev/null; ' + '[ ! -s "$tmo" ] && [ "$waited" -eq 0 ]' ) return f"bash -c {shlex.quote(script)} _ {full_timeout} {shlex.quote(wait_channel)} {tmux_target.as_shell_arg()}" @@ -354,9 +357,10 @@ def _build_signal_or_marker_command( :func:`_build_signal_only_command` for the three properties that hand-rolled watchdog has to reproduce; the one that shapes this variant is that the waiter must be the tmux client itself, so the loop detects a fired hook by - the client having exited (``kill -0``) without the watchdog's ``$tmo`` - marker, rather than by wrapping the client in a subshell that writes a - sentinel. + the client having exited (``kill -0``) with a zero wait status and no + ``$tmo`` marker, rather than by wrapping the client in a subshell that + writes a sentinel. Once the waiter is gone without confirming, the marker is + the only thing left that can, so the loop keeps polling it. ``accept_marker_command`` is the agent-supplied shell snippet that prints the agent's latest acceptance-marker token (empty if none yet). A baseline is @@ -379,8 +383,11 @@ def _build_signal_or_marker_command( # Then submit, after a beat so the waiter is registered. '( sleep 0.1 && tmux send-keys -t "$2" Enter ) >/dev/null 2>&1 & ' f'end="$(( $(date +%s) + {int(full_timeout) + 1} ))"; ' + "hook_pending=1; " 'while [ "$(date +%s)" -lt "$end" ]; do ' - 'if ! kill -0 "$waiter" 2>/dev/null && [ ! -s "$tmo" ]; then exit 0; fi; ' + 'if [ "$hook_pending" -eq 1 ] && ! kill -0 "$waiter" 2>/dev/null; then ' + 'wait "$waiter" 2>/dev/null && [ ! -s "$tmo" ] && exit 0; ' + "hook_pending=0; fi; " f'cur="$({accept_marker_command})"; ' 'if [[ -n "$cur" && "$cur" > "$base" ]]; then exit 0; fi; ' "sleep 0.25; " diff --git a/libs/mngr/imbue/mngr/agents/tui_utils_test.py b/libs/mngr/imbue/mngr/agents/tui_utils_test.py index caf237015f..c7ae2c184a 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils_test.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils_test.py @@ -1,5 +1,7 @@ """Unit tests for tui_utils.""" +import os +import subprocess from datetime import datetime from datetime import timezone from pathlib import Path @@ -346,3 +348,32 @@ def test_send_enter_via_hook_confirms_on_the_hook_when_the_marker_never_advances f"tmux kill-session -t '={session_name}' 2>/dev/null", timeout_seconds=5.0, ) + + +def _run_with_failing_tmux(command: str, tmp_path: Path) -> subprocess.CompletedProcess[str]: + """Run a built submission command with a ``tmux`` on PATH that fails immediately.""" + shim_dir = tmp_path / "failing_tmux_bin" + shim_dir.mkdir() + tmux_shim = shim_dir / "tmux" + tmux_shim.write_text("#!/bin/sh\nexit 1\n") + tmux_shim.chmod(0o755) + env = dict(os.environ) + env["PATH"] = f"{shim_dir}{os.pathsep}{env['PATH']}" + return subprocess.run(command, shell=True, capture_output=True, text=True, env=env, timeout=60) + + +@pytest.mark.parametrize( + "command", + [ + _build_signal_only_command(0.5, "chan", _TARGET), + _build_signal_or_marker_command(0.5, "chan", _TARGET, "printf 'frozen'"), + ], +) +def test_submission_command_fails_when_tmux_itself_fails(command: str, tmp_path: Path) -> None: + """A ``tmux wait-for`` that errors out (no server, dead session) is not a submission. + + A killed ``tmux wait-for`` exits 0, so the deadline is tracked separately; the + wait status still has to be honored, or an unreachable tmux reports success. + """ + result = _run_with_failing_tmux(command, tmp_path) + assert result.returncode != 0, f"reported success though tmux failed: {result.stdout!r}" From f73b4353b8f7a4d7298c2df31cf1b3f9f83927c5 Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Thu, 9 Jul 2026 23:09:27 -0700 Subject: [PATCH 04/14] Harden the macOS portability fixes after review Guard against mktemp failure in the submission watchdog: an empty $tmo made the `[ ! -s "$tmo" ]` expiry check unconditionally true, so every timeout would have reported success. Also kill the Enter-sending subshell on exit. Make get_directory_size agree across its two branches. The remote branch tolerated no `du` exit status: `du` prints a correct total and exits 1 after skipping an unreadable subdirectory, and the previous `| cut -f1` pipeline masked that, so dropping the pipe would have reported 0 bytes for such a tree. It also needs a trailing slash to descend a symlink to a directory. The local branch summed apparent bytes while the remote summed disk blocks, which diverge by up to 4096x on a small-file tree; it now sums st_blocks and dedupes by inode, matching `du`. Walking with os.walk rather than Path.rglob makes it ~3x faster. Delete mngr_transcript_extract_field (no callers repo-wide) and _mngr_transcript_set_field_pattern (assigned a global, breaking the header's stated purity contract); the field ERE is now a module constant. Fix two stale claims in the send_enter_via_tmux_wait_for_hook docstring: the waiter no longer runs in the foreground, and a tmux wait-for signal with no registered waiter is not lost -- it latches. --- libs/mngr/changelog/mngr-timeout-tac.md | 4 +- libs/mngr/imbue/mngr/agents/tui_utils.py | 78 ++++++++----------- libs/mngr/imbue/mngr/conftest.py | 15 ++-- libs/mngr/imbue/mngr/interfaces/host.py | 48 ++++++++---- .../mngr/resources/mngr_transcript_lib.sh | 61 +++++---------- .../resources/mngr_transcript_lib_test.py | 19 +---- 6 files changed, 96 insertions(+), 129 deletions(-) diff --git a/libs/mngr/changelog/mngr-timeout-tac.md b/libs/mngr/changelog/mngr-timeout-tac.md index 80d71aabcf..a6fa92b9a6 100644 --- a/libs/mngr/changelog/mngr-timeout-tac.md +++ b/libs/mngr/changelog/mngr-timeout-tac.md @@ -8,6 +8,6 @@ Transcript-streamer startup got roughly 20x faster as part of that change. Both `mngr gc` mis-measured orphaned work directories on macOS. `du -sb` has no BSD equivalent (`-b` is rejected), and because the command was piped into `cut`, the pipeline exit status was `cut`'s, so the failure looked like success with empty output and every orphan was reported as 0 bytes. `stat -c %Y` is likewise rejected by BSD `stat`, so each orphan's `created_at` fell back to `datetime.now()` and looked brand-new, meaning age-based collection never ran on macOS. -Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it walks the local filesystem for local hosts and runs POSIX `test -d ... && du -sk` over SSH for remote ones, so callers never branch on `is_local`. Remote sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines, and a path that is not a directory reports 0 on both sides. +Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it walks the local filesystem for local hosts and runs POSIX `test -d ... && du -sk` over SSH for remote ones, so callers never branch on `is_local`. Remote sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines, and a path that is not a directory reports 0 on both sides. The two branches agree on what they measure: both report disk blocks rather than apparent bytes (a sparse or small-file tree differs by up to 4096x between the two), both count a hardlinked inode once, and both descend a path that is a symlink to a directory. The remote branch also tolerates `du` exiting non-zero, since `du` still prints a correct total after skipping an unreadable subdirectory -- the piped-into-`cut` form this replaced happened to mask that, and dropping the pipe would otherwise have turned a working directory into a reported 0 bytes. -A submission whose `tmux wait-for` fails outright -- no tmux server, a dead session -- is no longer reported as successfully submitted. `timeout(1)` passed the client's exit status through when the client exited on its own, and only overrode it on expiry; the hand-rolled watchdog now does the same, using the deadline marker to detect expiry and the wait status to detect an outright failure. +A submission whose `tmux wait-for` fails outright -- no tmux server, a dead session -- is no longer reported as successfully submitted. `timeout(1)` passed the client's exit status through when the client exited on its own, and only overrode it on expiry; the hand-rolled watchdog now does the same, using the deadline marker to detect expiry and the wait status to detect an outright failure. If the watchdog cannot create its marker file at all, the submission fails rather than silently treating every subsequent timeout as a success. diff --git a/libs/mngr/imbue/mngr/agents/tui_utils.py b/libs/mngr/imbue/mngr/agents/tui_utils.py index 185c83f09d..47124376a2 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils.py @@ -216,12 +216,10 @@ def send_enter_via_tmux_wait_for_hook( """Strategy 3: send Enter and wait for a tmux wait-for channel fired by a TUI hook. Used by agents whose TUI exposes a UserPromptSubmit-style hook that fires - ``tmux wait-for -S $channel`` once the message is submitted. The waiter - is run in the **foreground** so it registers with the tmux server - synchronously, then Enter is sent from a backgrounded subshell after a - short delay. This avoids a race where the hook signal fires before the - waiter has registered (signals wake exactly one waiter; if none exists - the signal is lost). + ``tmux wait-for -S $channel`` once the message is submitted. The waiter is + started before Enter is sent, and Enter is sent from a backgrounded subshell + after a short delay, so that the waiter has registered with the tmux server + by the time the hook can fire. ``accept_marker_command`` (when supplied) is an agent-provided shell snippet that prints a lexicographically-monotonic token -- e.g. an ISO-8601 @@ -306,29 +304,21 @@ def _build_signal_only_command(full_timeout: float, wait_channel: str, tmux_targ Used for TUIs with no acceptance-marker command to watch. The waiter is started before Enter is sent from a backgrounded subshell, so it is registered by the time the hook can fire. Exit 0 = signalled, non-zero = - deadline reached. - - The deadline is a backgrounded ``sleep``-then-``kill`` watchdog rather than - ``timeout(1)``, which is GNU coreutils and absent on macOS. Three properties - the watchdog has to reproduce by hand: - - - The waiter must be the tmux client itself, not a wrapper subshell, or the - kill hits the subshell and leaves the client running forever. - - A killed ``tmux wait-for`` exits 0, so the wait status alone cannot mean - success. The watchdog writes ``$tmo`` *before* killing, and a non-empty - ``$tmo`` means the deadline passed, the way ``timeout`` returns 124. The - wait status still decides the rest: ``tmux wait-for`` exits non-zero when - it fails outright (no server, dead session), which is not a submission. - - Background jobs redirect stdout, because a job that inherits it holds the - caller's stdout open until it exits, stalling every submission for the - full deadline even when the signal lands immediately. + deadline reached or tmux failed. + + macOS ships no ``timeout`` binary, so the deadline is a ``sleep``-then-``kill`` + watchdog. ``$waiter`` is the tmux client itself, so the kill reaps it. A + killed ``tmux wait-for`` exits 0, so the deadline is recorded in ``$tmo`` + before the kill, and the wait status separates a signal from an outright + tmux failure. Background jobs redirect stdout, which they would otherwise + hold open until they exit, stalling the caller for the full deadline. """ script = ( - 'tmo="$(mktemp)"; ' - 'trap \'kill "$waiter" "$watchdog" 2>/dev/null; rm -f "$tmo"\' EXIT; ' + 'tmo="$(mktemp)" || exit 1; ' + 'trap \'kill "$waiter" "$watchdog" "$submit" 2>/dev/null; rm -f "$tmo"\' EXIT; ' 'tmux wait-for "$2" & waiter=$!; ' '( sleep "$1"; echo 1 > "$tmo"; kill "$waiter" ) >/dev/null 2>&1 & watchdog=$!; ' - '( sleep 0.1 && tmux send-keys -t "$3" Enter ) >/dev/null 2>&1 & ' + '( sleep 0.1 && tmux send-keys -t "$3" Enter ) >/dev/null 2>&1 & submit=$!; ' 'wait "$waiter" 2>/dev/null; waited=$?; ' 'kill "$watchdog" 2>/dev/null; ' '[ ! -s "$tmo" ] && [ "$waited" -eq 0 ]' @@ -345,22 +335,19 @@ def _build_signal_or_marker_command( """Succeed as soon as EITHER the hook signal fires OR a fresh acceptance marker appears. A single remote command so the two conditions are watched concurrently with - no dangling process: it registers the hook waiter in the background -- which - writes a sentinel file on success, preserving the register-before-Enter - ordering so the signal is never missed (which matters for submissions that - only ever fire the signal, never recording a marker) -- sends Enter, then - polls both the sentinel and the acceptance marker until either confirms or - the deadline passes. Exit 0 = confirmed, non-zero = timeout. - - The hook waiter is bounded by a ``sleep``-then-``kill`` watchdog rather than - ``timeout(1)`` (GNU coreutils, absent on macOS). See - :func:`_build_signal_only_command` for the three properties that hand-rolled - watchdog has to reproduce; the one that shapes this variant is that the - waiter must be the tmux client itself, so the loop detects a fired hook by - the client having exited (``kill -0``) with a zero wait status and no - ``$tmo`` marker, rather than by wrapping the client in a subshell that - writes a sentinel. Once the waiter is gone without confirming, the marker is - the only thing left that can, so the loop keeps polling it. + no dangling process: it registers the hook waiter in the background, + preserving the register-before-Enter ordering so the signal is never missed + (which matters for submissions that only ever fire the signal, never + recording a marker), sends Enter, then polls both the waiter and the + acceptance marker until either confirms or the deadline passes. Exit 0 = + confirmed, non-zero = timeout. + + macOS ships no ``timeout`` binary, so the hook waiter is bounded by a + ``sleep``-then-``kill`` watchdog. ``$waiter`` is the tmux client itself, so + the loop reads a fired hook as the client having exited (``kill -0``) with a + zero wait status and no ``$tmo`` deadline marker. A waiter that is gone + without confirming leaves the marker as the only thing that still can, so + the loop keeps polling it. ``accept_marker_command`` is the agent-supplied shell snippet that prints the agent's latest acceptance-marker token (empty if none yet). A baseline is @@ -372,16 +359,15 @@ def _build_signal_or_marker_command( agent that supplies it. """ script = ( - 'tmo="$(mktemp)"; ' - # Clean up on every exit path: kill the tmux client and the watchdog - # (either may outlive a fast marker-win) and remove the marker file. - 'trap \'kill "$waiter" "$watchdog" 2>/dev/null; rm -f "$tmo"\' EXIT; ' + 'tmo="$(mktemp)" || exit 1; ' + # Any of the three background jobs can outlive a fast marker-win. + 'trap \'kill "$waiter" "$watchdog" "$submit" 2>/dev/null; rm -f "$tmo"\' EXIT; ' f'base="$({accept_marker_command})"; ' # Register the hook waiter first, bounded by the watchdog. 'tmux wait-for "$1" & waiter=$!; ' f'( sleep {full_timeout}; echo 1 > "$tmo"; kill "$waiter" ) >/dev/null 2>&1 & watchdog=$!; ' # Then submit, after a beat so the waiter is registered. - '( sleep 0.1 && tmux send-keys -t "$2" Enter ) >/dev/null 2>&1 & ' + '( sleep 0.1 && tmux send-keys -t "$2" Enter ) >/dev/null 2>&1 & submit=$!; ' f'end="$(( $(date +%s) + {int(full_timeout) + 1} ))"; ' "hook_pending=1; " 'while [ "$(date +%s)" -lt "$end" ]; do ' diff --git a/libs/mngr/imbue/mngr/conftest.py b/libs/mngr/imbue/mngr/conftest.py index 77819a03c8..7a9684dc76 100644 --- a/libs/mngr/imbue/mngr/conftest.py +++ b/libs/mngr/imbue/mngr/conftest.py @@ -44,6 +44,7 @@ from imbue.mngr.utils.testing import init_git_repo from imbue.mngr.utils.testing import worker_docker_state_prefixes from imbue.mngr.utils.testing import worker_test_ids +from imbue.mngr.utils.testing import write_executable_script # Resource guards (tmux, rsync, unison, modal, docker_cli, docker_sdk) are # registered automatically via the resource_guards entry point group. @@ -800,17 +801,17 @@ def session_cleanup() -> Generator[None, None, None]: @pytest.fixture def posix_only_path(tmp_path_factory: pytest.TempPathFactory) -> dict[str, str]: - """An environment whose PATH shadows GNU-coreutils-only binaries with shims that exit 127. + """An environment whose PATH shadows ``tac`` and ``timeout`` with shims that exit 127. - mngr must run on stock macOS, whose BSD userland has no ``tac`` or ``timeout``. - Poisoning them means a reintroduced dependency fails on Linux too, rather than - passing in CI and breaking only on a user's mac. + Stock macOS ships a BSD userland with neither, so poisoning them makes a + reintroduced dependency on either fail on Linux too, rather than passing in CI + and breaking only on a user's mac. It is a targeted guard for those two names, + not a general macOS-portability check: a GNU-only *flag* on a binary BSD does + ship (``du -b``, ``stat -c``) passes straight through it. """ poison_dir = tmp_path_factory.mktemp("posix_only_bin") for binary in _GNU_ONLY_BINARIES: - shim = poison_dir / binary - shim.write_text(f'#!/bin/sh\necho "{binary}: command not found" >&2\nexit 127\n') - shim.chmod(0o755) + write_executable_script(poison_dir / binary, f'#!/bin/sh\necho "{binary}: command not found" >&2\nexit 127\n') env = dict(os.environ) env["PATH"] = f"{poison_dir}{os.pathsep}{env['PATH']}" return env diff --git a/libs/mngr/imbue/mngr/interfaces/host.py b/libs/mngr/imbue/mngr/interfaces/host.py index 6152031ddb..5a1926c6b5 100644 --- a/libs/mngr/imbue/mngr/interfaces/host.py +++ b/libs/mngr/imbue/mngr/interfaces/host.py @@ -2,7 +2,6 @@ import os import shlex -import stat from abc import ABC from abc import abstractmethod from contextlib import contextmanager @@ -48,6 +47,9 @@ from imbue.mngr.primitives import TmuxWindowSize from imbue.mngr.primitives import TransferMode +# POSIX fixes st_blocks in 512-byte units regardless of the filesystem block size. +_STAT_BLOCK_SIZE_BYTES: int = 512 + class HostInterface(MutableModel, ABC): """Interface for host implementations.""" @@ -431,36 +433,48 @@ def path_exists(self, path: Path) -> bool: return self.execute_idempotent_command(f"test -e {shlex.quote(str(path))}", timeout_seconds=5.0).success def get_directory_size(self, path: Path) -> SizeBytes: - """Total size of the regular files under ``path``, or 0 if it does not exist. + """Disk space used by ``path`` and its contents, or 0 if it is not a directory. Uses the local filesystem for local hosts and ``du -sk`` over SSH for remote hosts. Implemented on the interface so callers (including plugins) don't have to branch on ``is_local`` themselves. - ``-k`` is the only ``du`` block size POSIX defines, so remote sizes are - whole kibibytes; local sizes are exact. Entries that vanish mid-walk are - skipped, since callers scan directories that may still be in use. + Both branches report allocated blocks and charge a hard-linked inode once, + so a sparse file costs what it occupies rather than its apparent length. + Remote totals are whole kibibytes, since ``-k`` is the only ``du`` block + size POSIX defines. An unreadable entry is skipped rather than discarding + the total, which is also why ``du``'s non-zero exit for that case does + not: it still reports everything it reached. """ if self.is_local: - if not path.is_dir(): - return SizeBytes(0) - total = 0 - for dir_path, _dir_names, file_names in os.walk(path): - for file_name in file_names: + total_blocks = 0 + seen_inodes: set[tuple[int, int]] = set() + for dir_path, dir_names, file_names in os.walk(path): + entries = [dir_path] + entries.extend(os.path.join(dir_path, name) for name in (*dir_names, *file_names)) + for entry in entries: try: - entry_stat = os.lstat(os.path.join(dir_path, file_name)) + entry_stat = os.lstat(entry) except OSError: continue - if stat.S_ISREG(entry_stat.st_mode): - total += entry_stat.st_size - return SizeBytes(total) - quoted_path = shlex.quote(str(path)) + inode = (entry_stat.st_dev, entry_stat.st_ino) + if inode in seen_inodes: + continue + seen_inodes.add(inode) + total_blocks += entry_stat.st_blocks + return SizeBytes(total_blocks * _STAT_BLOCK_SIZE_BYTES) + # The trailing slash makes `du` descend a symlinked directory, as os.walk does. + quoted_path = shlex.quote(f"{path}/") result = self.execute_idempotent_command( - f"test -d {quoted_path} && du -sk {quoted_path}", timeout_seconds=30.0 + f"test -d {quoted_path} && {{ du -sk {quoted_path} 2>/dev/null || true; }}", + timeout_seconds=30.0, ) if not result.success or not result.stdout.strip(): return SizeBytes(0) - return SizeBytes(int(result.stdout.split()[0]) * 1024) + kibibytes = result.stdout.split()[0] + if not kibibytes.isdigit(): + return SizeBytes(0) + return SizeBytes(int(kibibytes) * 1024) class OnlineHostInterface(HostInterface, OuterHostInterface, ABC): diff --git a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh index 40a2f7e0b5..e2776558c5 100644 --- a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh +++ b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh @@ -5,24 +5,17 @@ # share the parts of raw-transcript capture that are structurally identical # regardless of the agent's native session schema: # -# - mngr_transcript_extract_field FIELD LINE -# Extract the first top-level "": "" string from one JSONL -# line. Used to read uuid / id / similar correlation fields without -# requiring jq. Echoes the bare value (no quotes) on stdout, empty if no -# match. Always returns 0 (safe under `set -e` command substitution). -# # - mngr_transcript_build_id_set OUTPUT_FILE FIELD # Populate the associative array _MNGR_TRANSCRIPT_ID_SET with every value # of FIELD found in OUTPUT_FILE. Used at startup and when a late- -# appearing session file needs reconciliation. +# appearing session file needs reconciliation. Correlation fields (uuid, +# id) are matched with a bash regex, so no jq is required. # # - mngr_transcript_reconcile_offset SESSION_FILE FIELD -# Echo the largest line offset N such that lines 1..N of SESSION_FILE -# have FIELD values already present in _MNGR_TRANSCRIPT_ID_SET. Used to -# recover after a crash that may have dropped the stored offset. Echoes -# 0 if no match is found or the file is empty. Scans forward rather than -# reversing the file, so it needs no `tac` (GNU coreutils, absent on -# macOS). +# Echo the highest 1-indexed line of SESSION_FILE whose FIELD value is +# already present in _MNGR_TRANSCRIPT_ID_SET, or 0 if there is none. Used +# to recover after a crash that may have dropped the stored offset. Scans +# forward, so it needs no `tac` (GNU coreutils, absent on macOS). # # - mngr_transcript_emit_lines_range SESSION_FILE START END OUTPUT_FILE # Append lines START..END (inclusive, 1-indexed) of SESSION_FILE to @@ -40,32 +33,14 @@ # (so it survives function-local scope) and for clearing it when no longer # needed. -# Set _MNGR_TRANSCRIPT_FIELD_PATTERN to the ERE matching a top-level -# "": "" pair, capturing the value. -# -# The per-line loops below match with this pattern directly rather than calling -# mngr_transcript_extract_field, because a command substitution forks a subshell -# per line -- prohibitive on a session file with tens of thousands of lines. -_mngr_transcript_set_field_pattern() { - _MNGR_TRANSCRIPT_FIELD_PATTERN="\"$1\"[[:space:]]*:[[:space:]]*\"([^\"]*)\"" -} - -# Extract a top-level JSON string field from a single line. +# The ERE matching a top-level "": "" pair, capturing the value. +# Matched inline in the loops below: a command substitution would fork a subshell +# per line, and a session file runs to tens of thousands of lines. # -# Limitations: matches the *first* "": "" in the line. Nested -# occurrences inside arrays / sub-objects with the same key are also matched -# if they precede the top-level one, but in practice agent-emitted JSONL -# events keep correlation fields (uuid, id) at the top so the first match -# is the right one. -mngr_transcript_extract_field() { - local field="$1" - local line="$2" - _mngr_transcript_set_field_pattern "$field" - if [[ "$line" =~ $_MNGR_TRANSCRIPT_FIELD_PATTERN ]]; then - printf '%s\n' "${BASH_REMATCH[1]}" - fi - return 0 -} +# Limitations: matches the *first* such pair in the line. Agent-emitted JSONL +# events keep correlation fields (uuid, id) at the top, so the first match is the +# right one. +_MNGR_TRANSCRIPT_FIELD_ERE='"%s"[[:space:]]*:[[:space:]]*"([^"]*)"' # Build _MNGR_TRANSCRIPT_ID_SET from every FIELD value in OUTPUT_FILE. mngr_transcript_build_id_set() { @@ -75,10 +50,11 @@ mngr_transcript_build_id_set() { if [ ! -s "$output_file" ]; then return 0 fi - _mngr_transcript_set_field_pattern "$field" + local pattern + printf -v pattern "$_MNGR_TRANSCRIPT_FIELD_ERE" "$field" local line while IFS= read -r line; do - if [[ "$line" =~ $_MNGR_TRANSCRIPT_FIELD_PATTERN ]] && [ -n "${BASH_REMATCH[1]}" ]; then + if [[ "$line" =~ $pattern ]] && [ -n "${BASH_REMATCH[1]}" ]; then _MNGR_TRANSCRIPT_ID_SET["${BASH_REMATCH[1]}"]=1 fi done < "$output_file" @@ -97,13 +73,14 @@ mngr_transcript_reconcile_offset() { return 0 fi - _mngr_transcript_set_field_pattern "$field" + local pattern + printf -v pattern "$_MNGR_TRANSCRIPT_FIELD_ERE" "$field" local idx=0 local found=0 local line value while IFS= read -r line; do idx=$((idx + 1)) - if [[ "$line" =~ $_MNGR_TRANSCRIPT_FIELD_PATTERN ]]; then + if [[ "$line" =~ $pattern ]]; then value="${BASH_REMATCH[1]}" if [ -n "$value" ] && [ "${_MNGR_TRANSCRIPT_ID_SET[$value]+exists}" ]; then found=$idx diff --git a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py index 2b06244f7f..7db82b85a0 100644 --- a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py +++ b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py @@ -8,6 +8,7 @@ from __future__ import annotations import json +import shlex import subprocess from pathlib import Path @@ -22,10 +23,10 @@ def _write_jsonl(path: Path, uuids: list[str]) -> None: def _reconcile_offset(bash: str, env: dict[str, str], session_file: Path, output_file: Path) -> int: script = f""" - source {_LIB} + source {shlex.quote(str(_LIB))} declare -A _MNGR_TRANSCRIPT_ID_SET - mngr_transcript_build_id_set {output_file} uuid - mngr_transcript_reconcile_offset {session_file} uuid + mngr_transcript_build_id_set {shlex.quote(str(output_file))} uuid + mngr_transcript_reconcile_offset {shlex.quote(str(session_file))} uuid """ result = subprocess.run([bash, "-c", script], capture_output=True, text=True, env=env, timeout=30) assert result.returncode == 0, f"lib exited {result.returncode}: {result.stderr}" @@ -65,18 +66,6 @@ def test_reconcile_offset_ignores_an_unterminated_final_line( assert offset == 1 -def test_reconcile_offset_is_zero_when_nothing_was_emitted( - tmp_path: Path, posix_only_path: dict[str, str], bash_with_associative_arrays: str -) -> None: - session_file = tmp_path / "session.jsonl" - output_file = tmp_path / "output.jsonl" - _write_jsonl(session_file, ["id-1", "id-2"]) - output_file.write_text("") - - offset = _reconcile_offset(bash_with_associative_arrays, posix_only_path, session_file, output_file) - assert offset == 0 - - def test_reconcile_offset_returns_the_highest_matching_line_not_the_first( tmp_path: Path, posix_only_path: dict[str, str], bash_with_associative_arrays: str ) -> None: From e861ac479008e07942adddeb099c624c9d3e615f Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Fri, 10 Jul 2026 16:37:39 -0700 Subject: [PATCH 05/14] Test the signal-only script's exit code and reframe the watchdog comments Add two @pytest.mark.tmux tests that run the exact script _build_signal_only_command emits against the test's isolated tmux server: it exits 0 once the hook fires the wait-for channel, and non-zero when the sleep-then-kill deadline passes with no hook. These assert on the generated script directly, complementing the higher-level send_enter_via_tmux_wait_for_hook tests. Reword the watchdog docstrings to lead with the portability constraint (the code runs on the agent's host, Linux or macOS) rather than singling out macOS, and tighten the marker-variant comment. --- libs/mngr/imbue/mngr/agents/tui_utils.py | 24 ++++----- libs/mngr/imbue/mngr/agents/tui_utils_test.py | 52 ++++++++++++++++++- 2 files changed, 62 insertions(+), 14 deletions(-) diff --git a/libs/mngr/imbue/mngr/agents/tui_utils.py b/libs/mngr/imbue/mngr/agents/tui_utils.py index 47124376a2..859a48e3d9 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils.py @@ -306,12 +306,13 @@ def _build_signal_only_command(full_timeout: float, wait_channel: str, tmux_targ registered by the time the hook can fire. Exit 0 = signalled, non-zero = deadline reached or tmux failed. - macOS ships no ``timeout`` binary, so the deadline is a ``sleep``-then-``kill`` - watchdog. ``$waiter`` is the tmux client itself, so the kill reaps it. A - killed ``tmux wait-for`` exits 0, so the deadline is recorded in ``$tmo`` - before the kill, and the wait status separates a signal from an outright - tmux failure. Background jobs redirect stdout, which they would otherwise - hold open until they exit, stalling the caller for the full deadline. + This runs on the agent's host, Linux or macOS, so the deadline cannot use + ``timeout``; it is a ``sleep``-then-``kill`` watchdog instead. ``$waiter`` is + the tmux client itself, so the kill reaps it. A killed ``tmux wait-for`` exits + 0, so the watchdog records the deadline in ``$tmo`` before killing, and the + wait status then separates a signal from an outright tmux failure. Background + jobs redirect stdout, which they would otherwise hold open until they exit, + stalling the caller for the full deadline. """ script = ( 'tmo="$(mktemp)" || exit 1; ' @@ -342,12 +343,11 @@ def _build_signal_or_marker_command( acceptance marker until either confirms or the deadline passes. Exit 0 = confirmed, non-zero = timeout. - macOS ships no ``timeout`` binary, so the hook waiter is bounded by a - ``sleep``-then-``kill`` watchdog. ``$waiter`` is the tmux client itself, so - the loop reads a fired hook as the client having exited (``kill -0``) with a - zero wait status and no ``$tmo`` deadline marker. A waiter that is gone - without confirming leaves the marker as the only thing that still can, so - the loop keeps polling it. + Like the signal-only path, the waiter is bounded by a ``sleep``-then-``kill`` + watchdog so it runs on Linux and macOS alike. ``$waiter`` is the tmux client, + so a fired hook shows up as the client having exited (``kill -0``) with a zero + wait status and no ``$tmo`` marker; once the waiter is gone without confirming, + only the acceptance marker can, so the loop keeps polling it. ``accept_marker_command`` is the agent-supplied shell snippet that prints the agent's latest acceptance-marker token (empty if none yet). A baseline is diff --git a/libs/mngr/imbue/mngr/agents/tui_utils_test.py b/libs/mngr/imbue/mngr/agents/tui_utils_test.py index c7ae2c184a..8ef51ae5ea 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils_test.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils_test.py @@ -275,8 +275,8 @@ def test_send_enter_via_hook_raises_on_timeout(signal_agent: _ProbeAgent) -> Non def test_submission_commands_invoke_no_gnu_only_binary(command: str) -> None: """Both builders run on the agent's host, which for a local agent is the user's machine. - macOS ships a BSD userland with no ``timeout``, so the deadline has to be a - hand-rolled watchdog. + That host may be Linux or macOS, so the deadline is a hand-rolled watchdog + rather than ``timeout``. """ assert "timeout" not in command @@ -377,3 +377,51 @@ def test_submission_command_fails_when_tmux_itself_fails(command: str, tmp_path: """ result = _run_with_failing_tmux(command, tmp_path) assert result.returncode != 0, f"reported success though tmux failed: {result.stdout!r}" + + +def _run_built_command_against_real_tmux( + command: str, *, fire_signal_after_seconds: float | None +) -> subprocess.CompletedProcess[str]: + """Run a built submission command against the test's isolated tmux server. + + The autouse tmux-isolation fixture already points ``TMUX_TMPDIR`` at a private + server, so the bare ``tmux`` calls here and in the script never reach a real + one. Creates the send-keys target session, optionally fires the hook after a + delay, and returns the finished process. + """ + subprocess.run( + ["tmux", "new-session", "-d", "-s", _TARGET.session_name, "bash"], + check=True, + capture_output=True, + text=True, + timeout=10, + ) + firing: subprocess.Popen[bytes] | None = None + if fire_signal_after_seconds is not None: + firing = subprocess.Popen(f"sleep {fire_signal_after_seconds} && tmux wait-for -S chan", shell=True) + try: + return subprocess.run(command, shell=True, capture_output=True, text=True, timeout=30) + finally: + if firing is not None: + firing.wait(timeout=10) + subprocess.run( + ["tmux", "kill-session", "-t", f"={_TARGET.session_name}"], + capture_output=True, + timeout=10, + ) + + +@pytest.mark.tmux +def test_signal_only_script_exits_zero_when_the_hook_fires() -> None: + """The generated script confirms (exit 0) once the hook fires its wait-for channel.""" + command = _build_signal_only_command(5.0, "chan", _TARGET) + result = _run_built_command_against_real_tmux(command, fire_signal_after_seconds=0.3) + assert result.returncode == 0, f"script did not confirm the fired hook: {result.stderr!r}" + + +@pytest.mark.tmux +def test_signal_only_script_exits_nonzero_at_the_deadline() -> None: + """With no hook, the generated script fails once the sleep-then-kill deadline passes.""" + command = _build_signal_only_command(0.5, "chan", _TARGET) + result = _run_built_command_against_real_tmux(command, fire_signal_after_seconds=None) + assert result.returncode != 0, f"script reported success though no hook fired: {result.stdout!r}" From b3bca1b7fed892d8f888b7de09ad734a960f7320 Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Fri, 10 Jul 2026 18:19:13 -0700 Subject: [PATCH 06/14] Collapse get_directory_size to a single du -sk call The local branch was a 15-line os.walk reimplementation of du (st_blocks*512, inode dedup, followlinks handling) that existed only to match the remote du. execute_idempotent_command runs the same shell path locally and remotely, so the whole is_local branch, the seen_inodes set, and _STAT_BLOCK_SIZE_BYTES are gone -- one du -sk call for both. This follows the same rule as the timeout fix: call the tool, do not reimplement it. Verified the exact `test -d P/ && du -sk P/` invocation on stock macOS (BSD du), Linux GNU du, and Linux busybox du: all agree that a hard-linked inode is charged once, a nested symlinked directory is not followed, a top-level symlink is resolved via the trailing slash, and a non-directory reports nothing. The old comment claimed the trailing slash "descends a symlinked directory, as os.walk does", which overstated it -- the slash resolves the top-level path only, coinciding with os.walk's top-arg rule by a different mechanism. Add the first tests for get_directory_size (it had none): a real local host over a tree with a hardlink and a nested symlink, plus the non-directory case. --- libs/mngr/changelog/mngr-timeout-tac.md | 2 +- libs/mngr/imbue/mngr/hosts/host_test.py | 38 +++++++++++++++++++++++++ libs/mngr/imbue/mngr/interfaces/host.py | 38 ++++--------------------- 3 files changed, 45 insertions(+), 33 deletions(-) diff --git a/libs/mngr/changelog/mngr-timeout-tac.md b/libs/mngr/changelog/mngr-timeout-tac.md index a6fa92b9a6..f00913af9e 100644 --- a/libs/mngr/changelog/mngr-timeout-tac.md +++ b/libs/mngr/changelog/mngr-timeout-tac.md @@ -8,6 +8,6 @@ Transcript-streamer startup got roughly 20x faster as part of that change. Both `mngr gc` mis-measured orphaned work directories on macOS. `du -sb` has no BSD equivalent (`-b` is rejected), and because the command was piped into `cut`, the pipeline exit status was `cut`'s, so the failure looked like success with empty output and every orphan was reported as 0 bytes. `stat -c %Y` is likewise rejected by BSD `stat`, so each orphan's `created_at` fell back to `datetime.now()` and looked brand-new, meaning age-based collection never ran on macOS. -Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it walks the local filesystem for local hosts and runs POSIX `test -d ... && du -sk` over SSH for remote ones, so callers never branch on `is_local`. Remote sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines, and a path that is not a directory reports 0 on both sides. The two branches agree on what they measure: both report disk blocks rather than apparent bytes (a sparse or small-file tree differs by up to 4096x between the two), both count a hardlinked inode once, and both descend a path that is a symlink to a directory. The remote branch also tolerates `du` exiting non-zero, since `du` still prints a correct total after skipping an unreadable subdirectory -- the piped-into-`cut` form this replaced happened to mask that, and dropping the pipe would otherwise have turned a working directory into a reported 0 bytes. +Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it runs POSIX `test -d ... && du -sk` on the host -- locally via subprocess, remotely over SSH -- so callers never branch on `is_local`. Sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines, and a path that is not a directory reports 0. The trailing slash resolves a symlinked path to its target directory, and `du` exiting non-zero is tolerated because it still prints a correct total after skipping an unreadable subdirectory (the piped-into-`cut` form this replaced happened to mask that, and dropping the pipe would otherwise have turned a working directory into a reported 0 bytes). The exact `du -sk /` invocation was verified on stock macOS (BSD `du`) and on Linux (GNU and busybox `du`) to agree on the load-bearing cases: a hard-linked inode counted once, a nested symlinked directory not followed, and a top-level symlink resolved. A single `du` call rather than a Python reimplementation for local hosts follows the same rule as the `timeout` fix above -- call the tool instead of reimplementing it. A submission whose `tmux wait-for` fails outright -- no tmux server, a dead session -- is no longer reported as successfully submitted. `timeout(1)` passed the client's exit status through when the client exited on its own, and only overrode it on expiry; the hand-rolled watchdog now does the same, using the deadline marker to detect expiry and the wait status to detect an outright failure. If the watchdog cannot create its marker file at all, the submission fails rather than silently treating every subsequent timeout as a success. diff --git a/libs/mngr/imbue/mngr/hosts/host_test.py b/libs/mngr/imbue/mngr/hosts/host_test.py index e932269892..d2df4006e0 100644 --- a/libs/mngr/imbue/mngr/hosts/host_test.py +++ b/libs/mngr/imbue/mngr/hosts/host_test.py @@ -2,6 +2,7 @@ import io import json +import os import subprocess import threading from collections.abc import Callable @@ -3998,3 +3999,40 @@ def test_merge_agent_type_provisioning_combines_provisioning_and_env() -> None: result = _merge_agent_type_provisioning(agent_config, options) assert result.provisioning.extra_provision_commands == ("echo setup",) assert result.environment.env_vars == (EnvVar(key="KEY", value="val"),) + + +def test_get_directory_size_charges_hardlinks_once_and_ignores_nested_symlinks( + local_provider: LocalProviderInstance, + tmp_path: Path, +) -> None: + """``du -sk`` sizing: a hard-linked inode counts once and a nested symlinked dir is not followed.""" + host = local_provider.create_host(HostName(LOCAL_HOST_NAME)) + tree = tmp_path / "tree" + (tree / "sub").mkdir(parents=True) + (tree / "a").write_bytes(b"\0" * 100 * 1024) + os.link(tree / "a", tree / "a_hardlink") + (tree / "sub" / "c").write_bytes(b"\0" * 50 * 1024) + outside = tmp_path / "outside" + outside.mkdir() + (outside / "big").write_bytes(b"\0" * 400 * 1024) + (tree / "link_to_outside").symlink_to(outside) + + size = host.get_directory_size(tree) + + assert size % 1024 == 0 + # ``a`` (100 KiB) + ``sub/c`` (50 KiB) + a little directory overhead. Counting + # ``a_hardlink`` a second time would reach ~250 KiB; following the nested + # symlink would add the 400 KiB behind it. + assert 150 * 1024 <= size < 250 * 1024, f"hard-link double-count or followed a nested symlink: {size}" + + +def test_get_directory_size_is_zero_for_a_non_directory( + local_provider: LocalProviderInstance, + tmp_path: Path, +) -> None: + """A regular file or a missing path reports 0, not an error.""" + host = local_provider.create_host(HostName(LOCAL_HOST_NAME)) + a_file = tmp_path / "file" + a_file.write_text("x") + assert host.get_directory_size(a_file) == 0 + assert host.get_directory_size(tmp_path / "does_not_exist") == 0 diff --git a/libs/mngr/imbue/mngr/interfaces/host.py b/libs/mngr/imbue/mngr/interfaces/host.py index 5a1926c6b5..99f889d0fb 100644 --- a/libs/mngr/imbue/mngr/interfaces/host.py +++ b/libs/mngr/imbue/mngr/interfaces/host.py @@ -1,6 +1,5 @@ from __future__ import annotations -import os import shlex from abc import ABC from abc import abstractmethod @@ -47,9 +46,6 @@ from imbue.mngr.primitives import TmuxWindowSize from imbue.mngr.primitives import TransferMode -# POSIX fixes st_blocks in 512-byte units regardless of the filesystem block size. -_STAT_BLOCK_SIZE_BYTES: int = 512 - class HostInterface(MutableModel, ABC): """Interface for host implementations.""" @@ -435,35 +431,13 @@ def path_exists(self, path: Path) -> bool: def get_directory_size(self, path: Path) -> SizeBytes: """Disk space used by ``path`` and its contents, or 0 if it is not a directory. - Uses the local filesystem for local hosts and ``du -sk`` over SSH for - remote hosts. Implemented on the interface so callers (including plugins) - don't have to branch on ``is_local`` themselves. - - Both branches report allocated blocks and charge a hard-linked inode once, - so a sparse file costs what it occupies rather than its apparent length. - Remote totals are whole kibibytes, since ``-k`` is the only ``du`` block - size POSIX defines. An unreadable entry is skipped rather than discarding - the total, which is also why ``du``'s non-zero exit for that case does - not: it still reports everything it reached. + Runs POSIX ``du -sk`` on the host -- locally via subprocess, remotely over + SSH -- so callers (including plugins) don't have to branch on ``is_local``. + The trailing slash resolves a symlinked ``path`` to its target directory. + ``du`` still prints a correct total after skipping an unreadable entry, so + its non-zero exit in that case is tolerated. Totals are whole kibibytes, + since ``-k`` is the only block size POSIX defines. """ - if self.is_local: - total_blocks = 0 - seen_inodes: set[tuple[int, int]] = set() - for dir_path, dir_names, file_names in os.walk(path): - entries = [dir_path] - entries.extend(os.path.join(dir_path, name) for name in (*dir_names, *file_names)) - for entry in entries: - try: - entry_stat = os.lstat(entry) - except OSError: - continue - inode = (entry_stat.st_dev, entry_stat.st_ino) - if inode in seen_inodes: - continue - seen_inodes.add(inode) - total_blocks += entry_stat.st_blocks - return SizeBytes(total_blocks * _STAT_BLOCK_SIZE_BYTES) - # The trailing slash makes `du` descend a symlinked directory, as os.walk does. quoted_path = shlex.quote(f"{path}/") result = self.execute_idempotent_command( f"test -d {quoted_path} && {{ du -sk {quoted_path} 2>/dev/null || true; }}", From d35b9398c93c76db6d7e2f7fe828dac3487c2c9d Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Fri, 10 Jul 2026 18:47:44 -0700 Subject: [PATCH 07/14] Drop transport details from the get_directory_size docstring How execute_idempotent_command reaches the host (subprocess vs SSH) is the transport layer's concern, not this method's, and the is_local mention was a leftover from the branch that no longer exists. Keep only the du contract that affects the returned number: KiB rounding, symlink resolution, and tolerating du's non-zero exit on an unreadable entry. --- libs/mngr/imbue/mngr/interfaces/host.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/libs/mngr/imbue/mngr/interfaces/host.py b/libs/mngr/imbue/mngr/interfaces/host.py index 99f889d0fb..b41f243198 100644 --- a/libs/mngr/imbue/mngr/interfaces/host.py +++ b/libs/mngr/imbue/mngr/interfaces/host.py @@ -431,12 +431,10 @@ def path_exists(self, path: Path) -> bool: def get_directory_size(self, path: Path) -> SizeBytes: """Disk space used by ``path`` and its contents, or 0 if it is not a directory. - Runs POSIX ``du -sk`` on the host -- locally via subprocess, remotely over - SSH -- so callers (including plugins) don't have to branch on ``is_local``. - The trailing slash resolves a symlinked ``path`` to its target directory. - ``du`` still prints a correct total after skipping an unreadable entry, so - its non-zero exit in that case is tolerated. Totals are whole kibibytes, - since ``-k`` is the only block size POSIX defines. + Measured with POSIX ``du -sk``: totals are whole kibibytes, since ``-k`` is + the only block size POSIX defines. The trailing slash resolves a symlinked + ``path`` to its target directory, and ``du``'s non-zero exit after skipping + an unreadable entry is tolerated, since it still prints a correct total. """ quoted_path = shlex.quote(f"{path}/") result = self.execute_idempotent_command( From eb9621d2f03457ec6ef8bc50429bd2ea7dd2f1d0 Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Fri, 10 Jul 2026 18:53:12 -0700 Subject: [PATCH 08/14] Document two unstated contracts in mngr_transcript_lib.sh The field ERE interpolates FIELD literally via printf %s, so a field name with regex metacharacters would change the pattern; note that callers must pass a regex-safe field (uuid/id do). The reconcile loop's `while read` skips an unterminated final line, which is deliberate: it matches wc -l and the sed emit range, so a partially-written final record is deferred rather than miscounted. Note it so it is not "fixed" with a trailing-line catch that would re-break that consistency. --- libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh index e2776558c5..0e18c7c76c 100644 --- a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh +++ b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib.sh @@ -39,7 +39,8 @@ # # Limitations: matches the *first* such pair in the line. Agent-emitted JSONL # events keep correlation fields (uuid, id) at the top, so the first match is the -# right one. +# right one. FIELD is interpolated into this ERE literally (as printf's %s), so +# callers must pass a regex-safe field name; the uuid/id callers do. _MNGR_TRANSCRIPT_FIELD_ERE='"%s"[[:space:]]*:[[:space:]]*"([^"]*)"' # Build _MNGR_TRANSCRIPT_ID_SET from every FIELD value in OUTPUT_FILE. @@ -78,6 +79,8 @@ mngr_transcript_reconcile_offset() { local idx=0 local found=0 local line value + # read skips an unterminated final line, matching wc -l and the sed emit + # range, so a partially-written final record is deferred, not miscounted. while IFS= read -r line; do idx=$((idx + 1)) if [[ "$line" =~ $pattern ]]; then From c6a99d4fd4aeeb009dc3c56a439914d44b0aac6f Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Fri, 10 Jul 2026 19:04:28 -0700 Subject: [PATCH 09/14] Correct the transcript-streamer perf claim in the changelog Benchmarked old vs new (3 trials each, macOS and Linux) instead of the single number the changelog carried. The real, scenario-independent win is build_id_set's fork elimination: ~32s->0.7s on macOS, ~14s->0.6s on Linux at 50k lines. reconcile_offset is subtler -- dropping tac also dropped its early-exit, so the new forward scan always reads the whole file (~0.7s), a small loss in the best case but a large win over the old reverse scan's ~12s worst case. The old "30s -> 1.5s, reconciling" line conflated build_id_set's cost with reconcile and compared old-build-only against new-build-plus-reconcile. --- libs/mngr/changelog/mngr-timeout-tac.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/mngr/changelog/mngr-timeout-tac.md b/libs/mngr/changelog/mngr-timeout-tac.md index f00913af9e..9ba60d6200 100644 --- a/libs/mngr/changelog/mngr-timeout-tac.md +++ b/libs/mngr/changelog/mngr-timeout-tac.md @@ -4,7 +4,7 @@ Fixed four places where mngr shelled out to GNU-coreutils binaries and flags tha Raw-transcript streaming re-emitted already-emitted lines on macOS. `mngr_transcript_reconcile_offset` reverse-scanned the session file with `tac` to find the last emitted line; with `tac` absent the scan read nothing and the offset reset to 0. It now scans forward and tracks the last match, which needs no reverse at all. This also fixes a latent off-by-one: the old code combined `wc -l` (which does not count an unterminated final line) with `tac` (which emits it), so a session file whose last line was still being appended -- the normal state of a live transcript -- reconciled one line too low. -Transcript-streamer startup got roughly 20x faster as part of that change. Both `mngr_transcript_reconcile_offset` and `mngr_transcript_build_id_set` extracted each line's correlation field through a command substitution, which forks a subshell per line. They now match the field with a bash regex inline, forking nothing. Reconciling a 50,000-line session file drops from about 30 seconds to about 1.5 seconds; without this the forward scan would have been slower than the reverse scan it replaced, since the old code's `tac` let it stop at the first match from the end. +Transcript-streamer startup also got much faster. Both `mngr_transcript_build_id_set` and `mngr_transcript_reconcile_offset` used to extract each line's correlation field through a command substitution, which forks a subshell per line; they now match it with an inline bash regex and fork nothing. The dominant cost was `build_id_set`, which scans the whole already-emitted output: on a 50,000-line file it drops from about 32s to 0.7s on macOS, and about 14s to 0.6s on Linux (three trials each; forks are cheaper on Linux). `reconcile_offset` is subtler -- removing `tac` also removed its early exit on the first match from the end, so the new forward scan always reads the whole file (about 0.7s at 50,000 lines) instead of sometimes stopping early. But because it no longer forks, its worst case -- a match near the start, which made the old reverse scan fork through the entire file at about 12s -- is now that same bounded 0.7s. Combined startup on a 50,000-line pair is about 1.5s, down from tens of seconds. The inline match uses bash's built-in `[[ =~ ]]`, so unlike the `tac` it replaced it needs no external binary and behaves identically on macOS and Linux. `mngr gc` mis-measured orphaned work directories on macOS. `du -sb` has no BSD equivalent (`-b` is rejected), and because the command was piped into `cut`, the pipeline exit status was `cut`'s, so the failure looked like success with empty output and every orphan was reported as 0 bytes. `stat -c %Y` is likewise rejected by BSD `stat`, so each orphan's `created_at` fell back to `datetime.now()` and looked brand-new, meaning age-based collection never ran on macOS. From 8f56cd922d449675ec00398c4c46afa99106e86d Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Tue, 14 Jul 2026 13:49:10 -0700 Subject: [PATCH 10/14] Bound the tmux waiter with perl's alarm instead of rewriting the submission logic Per Josh's review: revert both command builders to their original structure and change only the deadline. The watchdog, the $tmo deadline marker, the three tracked pids and the mktemp guard are all gone. The rewrite was not needed. It inverted the marker variant's sentinel -- which the original wrote on *success*, so every way of failing to confirm landed on the same non-zero exit -- into a deadline marker, where an absent marker meant success. That inversion is what made an unguarded mktemp dangerous, and what made a killed waiter's exit status ambiguous. Both "fixes" in the previous changelog were repairs to regressions introduced by the rewrite itself; the original code was already correct on a tmux that fails outright (rc=1 passes through) and on a failed mktemp (falls through to failure). Where timeout(1) exists it is still used. Where it does not, perl arms a SIGALRM and exec replaces perl with the command, so the command keeps perl's pid: its exit status passes through, an expiry kills it outright, and no supervisor is left to reap. Unlike SIGTERM -- which `tmux wait-for` catches and exits 0 from, the sole reason the watchdog needed a marker file at all -- SIGALRM is not caught, so the deadline is unambiguous. Two sharp edges in the perl form, both measured and both guarded: - Time::HiRes: the builtin alarm takes whole seconds and alarm(0) *cancels* the timer, so `alarm 0.5` silently means no deadline (a 3s sleep under a 0.5s deadline returned 0 after 3s). - `or exit 127`: a failed exec returns to perl, which would run off the end of the program and exit 0 (exec of a missing binary returned 0). Probing with `command -v timeout` rather than testing for Darwin, since a host can have either binary without the other. Verified against a real tmux server on a PATH with no timeout, both builders: hook fires -> 0, deadline -> non-zero, no tmux server -> non-zero, zero orphaned wait-for clients. --- libs/mngr/changelog/mngr-timeout-tac.md | 6 +- libs/mngr/imbue/mngr/agents/tui_utils.py | 100 ++++++++++-------- libs/mngr/imbue/mngr/agents/tui_utils_test.py | 67 +++++++++--- 3 files changed, 110 insertions(+), 63 deletions(-) diff --git a/libs/mngr/changelog/mngr-timeout-tac.md b/libs/mngr/changelog/mngr-timeout-tac.md index 9ba60d6200..b3d893882f 100644 --- a/libs/mngr/changelog/mngr-timeout-tac.md +++ b/libs/mngr/changelog/mngr-timeout-tac.md @@ -1,6 +1,8 @@ Fixed four places where mngr shelled out to GNU-coreutils binaries and flags that stock macOS does not ship, each of which ran on the user's own machine whenever an agent used the `local` provider. All four were verified failing on macOS 26.4.1 with a stock `PATH`, then verified fixed. -`mngr message` to a local codex or antigravity agent raised `SendMessageError("Timeout waiting for message submission signal")` on every send. The submission path bounded its `tmux wait-for` with `timeout(1)`, which exits 127 (`timeout: command not found`) on macOS -- immediately, while reporting that it had waited the full timeout. It is now bounded by a `sleep`-then-`kill` watchdog. Reproducing `timeout(1)` by hand takes more than `sleep` and `kill`: a `tmux wait-for` client exits 0 whether it was signalled or killed, so the watchdog marks a file *before* killing and that marker, not the wait status, decides the exit code (the way `timeout` returns 124); the waiter must be the tmux client itself rather than a wrapper subshell, or the kill hits the subshell and orphans the client; and every background job redirects stdout, because a job that inherits it holds the caller's stdout open until it exits, which would stall each submission for the full deadline even when the signal lands immediately. For Claude agents (which also watch an acceptance marker) the same bug degraded silently instead: the `timeout` failure was swallowed inside a backgrounded subshell, so the hook path never confirmed and only the transcript marker could -- meaning `/clear` and `/compact`, which fire the hook but never enqueue a model turn, always burned the full timeout. +`mngr message` to a local codex or antigravity agent raised `SendMessageError("Timeout waiting for message submission signal")` on every send. The submission path bounded its `tmux wait-for` with `timeout(1)`, which exits 127 (`timeout: command not found`) on macOS -- immediately, while reporting that it had waited the full timeout. For Claude agents (which also watch an acceptance marker) the same bug degraded silently instead: the `timeout` failure was swallowed inside a backgrounded subshell, so the hook path never confirmed and only the transcript marker could -- meaning `/clear` and `/compact`, which fire the hook but never enqueue a model turn, always burned the full timeout. + +The submission logic is unchanged; only the deadline is. Where `timeout` exists it is still used, and where it does not the deadline comes from perl's `alarm`, which arms a SIGALRM before `exec` replaces perl with the command itself. Because the command inherits perl's pid, its exit status passes straight through, an expiry kills it outright, and there is no supervising process to leak. Two details are load-bearing: the alarm is armed through `Time::HiRes`, since the builtin `alarm` takes whole seconds and `alarm(0)` *cancels* the timer, so a sub-second deadline would otherwise mean no deadline at all; and a failed `exec` explicitly exits 127, since it would otherwise return to perl, run off the end of the program, and report success. Whether to use `timeout` is decided by probing for it rather than by testing the platform, because a host can have either it or perl without the other. Raw-transcript streaming re-emitted already-emitted lines on macOS. `mngr_transcript_reconcile_offset` reverse-scanned the session file with `tac` to find the last emitted line; with `tac` absent the scan read nothing and the offset reset to 0. It now scans forward and tracks the last match, which needs no reverse at all. This also fixes a latent off-by-one: the old code combined `wc -l` (which does not count an unterminated final line) with `tac` (which emits it), so a session file whose last line was still being appended -- the normal state of a live transcript -- reconciled one line too low. @@ -9,5 +11,3 @@ Transcript-streamer startup also got much faster. Both `mngr_transcript_build_id `mngr gc` mis-measured orphaned work directories on macOS. `du -sb` has no BSD equivalent (`-b` is rejected), and because the command was piped into `cut`, the pipeline exit status was `cut`'s, so the failure looked like success with empty output and every orphan was reported as 0 bytes. `stat -c %Y` is likewise rejected by BSD `stat`, so each orphan's `created_at` fell back to `datetime.now()` and looked brand-new, meaning age-based collection never ran on macOS. Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it runs POSIX `test -d ... && du -sk` on the host -- locally via subprocess, remotely over SSH -- so callers never branch on `is_local`. Sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines, and a path that is not a directory reports 0. The trailing slash resolves a symlinked path to its target directory, and `du` exiting non-zero is tolerated because it still prints a correct total after skipping an unreadable subdirectory (the piped-into-`cut` form this replaced happened to mask that, and dropping the pipe would otherwise have turned a working directory into a reported 0 bytes). The exact `du -sk /` invocation was verified on stock macOS (BSD `du`) and on Linux (GNU and busybox `du`) to agree on the load-bearing cases: a hard-linked inode counted once, a nested symlinked directory not followed, and a top-level symlink resolved. A single `du` call rather than a Python reimplementation for local hosts follows the same rule as the `timeout` fix above -- call the tool instead of reimplementing it. - -A submission whose `tmux wait-for` fails outright -- no tmux server, a dead session -- is no longer reported as successfully submitted. `timeout(1)` passed the client's exit status through when the client exited on its own, and only overrode it on expiry; the hand-rolled watchdog now does the same, using the deadline marker to detect expiry and the wait status to detect an outright failure. If the watchdog cannot create its marker file at all, the submission fails rather than silently treating every subsequent timeout as a success. diff --git a/libs/mngr/imbue/mngr/agents/tui_utils.py b/libs/mngr/imbue/mngr/agents/tui_utils.py index 859a48e3d9..047d256ab2 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils.py @@ -298,33 +298,42 @@ def _send_enter_and_wait_for_signal( return False +def _timeout_prefix(seconds: float) -> str: + """A ``timeout(1)`` invocation that also works where there is no ``timeout`` binary. + + macOS ships a BSD userland with no ``timeout``. Perl's ``alarm`` arms a SIGALRM + and ``exec`` then replaces perl with the command itself, so the command keeps + perl's pid: its exit status is passed straight through, an expiry kills it + outright, and nothing is left behind to reap. Two sharp edges, both load-bearing: + + - ``Time::HiRes`` because the builtin ``alarm`` takes whole seconds and + ``alarm(0)`` *cancels* the timer, so a sub-second deadline would silently + mean no deadline at all. + - ``or exit 127`` because a failed ``exec`` returns to perl, which would + otherwise run off the end of the program and exit 0. + + ``command -v`` rather than a platform test: the binary that is missing is what + matters, and a host can have either one without the other. + """ + perl_timeout = f"perl -MTime::HiRes=alarm -e 'alarm shift; exec @ARGV or exit 127' {seconds}" + return f'if command -v timeout >/dev/null 2>&1; then timeout {seconds} "$@"; else {perl_timeout} "$@"; fi' + + +def _timeout_function(seconds: float) -> str: + """Define ``mngr_timeout`` in the emitted script; it runs its arguments under a deadline.""" + return f"mngr_timeout() {{ {_timeout_prefix(seconds)}; }}; " + + def _build_signal_only_command(full_timeout: float, wait_channel: str, tmux_target: TmuxWindowTarget) -> str: """Send Enter, then block on the hook's wait-for channel until it fires or the deadline passes. - Used for TUIs with no acceptance-marker command to watch. The waiter is - started before Enter is sent from a backgrounded subshell, so it is - registered by the time the hook can fire. Exit 0 = signalled, non-zero = - deadline reached or tmux failed. - - This runs on the agent's host, Linux or macOS, so the deadline cannot use - ``timeout``; it is a ``sleep``-then-``kill`` watchdog instead. ``$waiter`` is - the tmux client itself, so the kill reaps it. A killed ``tmux wait-for`` exits - 0, so the watchdog records the deadline in ``$tmo`` before killing, and the - wait status then separates a signal from an outright tmux failure. Background - jobs redirect stdout, which they would otherwise hold open until they exit, - stalling the caller for the full deadline. + Used for TUIs with no acceptance-marker command to watch. The waiter is started + in the foreground before Enter is sent from a backgrounded subshell, so it is + registered by the time the hook can fire. The exit status is the waiter's, so + exit 0 = signalled, non-zero = deadline reached or tmux failed. """ - script = ( - 'tmo="$(mktemp)" || exit 1; ' - 'trap \'kill "$waiter" "$watchdog" "$submit" 2>/dev/null; rm -f "$tmo"\' EXIT; ' - 'tmux wait-for "$2" & waiter=$!; ' - '( sleep "$1"; echo 1 > "$tmo"; kill "$waiter" ) >/dev/null 2>&1 & watchdog=$!; ' - '( sleep 0.1 && tmux send-keys -t "$3" Enter ) >/dev/null 2>&1 & submit=$!; ' - 'wait "$waiter" 2>/dev/null; waited=$?; ' - 'kill "$watchdog" 2>/dev/null; ' - '[ ! -s "$tmo" ] && [ "$waited" -eq 0 ]' - ) - return f"bash -c {shlex.quote(script)} _ {full_timeout} {shlex.quote(wait_channel)} {tmux_target.as_shell_arg()}" + script = f'{_timeout_function(full_timeout)}( sleep 0.1 && tmux send-keys -t "$2" Enter ) & mngr_timeout tmux wait-for "$1"' + return f"bash -c {shlex.quote(script)} _ {shlex.quote(wait_channel)} {tmux_target.as_shell_arg()}" def _build_signal_or_marker_command( @@ -336,18 +345,17 @@ def _build_signal_or_marker_command( """Succeed as soon as EITHER the hook signal fires OR a fresh acceptance marker appears. A single remote command so the two conditions are watched concurrently with - no dangling process: it registers the hook waiter in the background, - preserving the register-before-Enter ordering so the signal is never missed - (which matters for submissions that only ever fire the signal, never - recording a marker), sends Enter, then polls both the waiter and the - acceptance marker until either confirms or the deadline passes. Exit 0 = - confirmed, non-zero = timeout. - - Like the signal-only path, the waiter is bounded by a ``sleep``-then-``kill`` - watchdog so it runs on Linux and macOS alike. ``$waiter`` is the tmux client, - so a fired hook shows up as the client having exited (``kill -0``) with a zero - wait status and no ``$tmo`` marker; once the waiter is gone without confirming, - only the acceptance marker can, so the loop keeps polling it. + no dangling process: it registers the (full-timeout) hook waiter in the + background -- which writes a sentinel file on success, preserving the + register-before-Enter ordering so the signal is never missed (which matters + for submissions that only ever fire the signal, never recording a marker) + -- sends Enter, then polls both the sentinel and the acceptance marker until + either confirms or the deadline passes. Exit 0 = confirmed, non-zero = + timeout. + + The sentinel is written only on success, so every way of *not* confirming -- + the deadline, a tmux failure, even ``mktemp`` failing and leaving ``$sig`` + empty -- lands on the same non-zero exit. ``accept_marker_command`` is the agent-supplied shell snippet that prints the agent's latest acceptance-marker token (empty if none yet). A baseline is @@ -359,21 +367,21 @@ def _build_signal_or_marker_command( agent that supplies it. """ script = ( - 'tmo="$(mktemp)" || exit 1; ' - # Any of the three background jobs can outlive a fast marker-win. - 'trap \'kill "$waiter" "$watchdog" "$submit" 2>/dev/null; rm -f "$tmo"\' EXIT; ' + f"{_timeout_function(full_timeout)}" + 'sig="$(mktemp)"; ' + # Clean up on every exit path: remove the sentinel file and reap any + # still-running background job (notably the hook waiter, which otherwise + # outlives a fast marker-win and would recreate "$sig" -- leaking the + # temp file -- when the hook finally fires). Runs exactly once on exit. + 'trap \'p="$(jobs -p)"; [ -n "$p" ] && kill $p 2>/dev/null; rm -f "$sig"\' EXIT; ' f'base="$({accept_marker_command})"; ' - # Register the hook waiter first, bounded by the watchdog. - 'tmux wait-for "$1" & waiter=$!; ' - f'( sleep {full_timeout}; echo 1 > "$tmo"; kill "$waiter" ) >/dev/null 2>&1 & watchdog=$!; ' + # Register the hook waiter first (full timeout), sentinel on success. + '( mngr_timeout tmux wait-for "$1" >/dev/null 2>&1 && echo 1 > "$sig" ) & ' # Then submit, after a beat so the waiter is registered. - '( sleep 0.1 && tmux send-keys -t "$2" Enter ) >/dev/null 2>&1 & submit=$!; ' + '( sleep 0.1 && tmux send-keys -t "$2" Enter ) & ' f'end="$(( $(date +%s) + {int(full_timeout) + 1} ))"; ' - "hook_pending=1; " 'while [ "$(date +%s)" -lt "$end" ]; do ' - 'if [ "$hook_pending" -eq 1 ] && ! kill -0 "$waiter" 2>/dev/null; then ' - 'wait "$waiter" 2>/dev/null && [ ! -s "$tmo" ] && exit 0; ' - "hook_pending=0; fi; " + 'if [ -s "$sig" ]; then exit 0; fi; ' f'cur="$({accept_marker_command})"; ' 'if [[ -n "$cur" && "$cur" > "$base" ]]; then exit 0; fi; ' "sleep 0.25; " diff --git a/libs/mngr/imbue/mngr/agents/tui_utils_test.py b/libs/mngr/imbue/mngr/agents/tui_utils_test.py index 8ef51ae5ea..803f6f01e1 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils_test.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils_test.py @@ -1,6 +1,7 @@ """Unit tests for tui_utils.""" import os +import shutil import subprocess from datetime import datetime from datetime import timezone @@ -14,6 +15,7 @@ from imbue.mngr.agents.tui_utils import _build_signal_or_marker_command from imbue.mngr.agents.tui_utils import _check_paste_content from imbue.mngr.agents.tui_utils import _normalize_for_match +from imbue.mngr.agents.tui_utils import _timeout_prefix from imbue.mngr.agents.tui_utils import send_enter_and_poll_for_cleared_indicator from imbue.mngr.agents.tui_utils import send_enter_best_effort from imbue.mngr.agents.tui_utils import send_enter_keystroke @@ -272,13 +274,14 @@ def test_send_enter_via_hook_raises_on_timeout(signal_agent: _ProbeAgent) -> Non _build_signal_or_marker_command(2.0, "chan", _TARGET, "printf ''"), ], ) -def test_submission_commands_invoke_no_gnu_only_binary(command: str) -> None: +def test_submission_commands_never_require_a_timeout_binary(command: str) -> None: """Both builders run on the agent's host, which for a local agent is the user's machine. - That host may be Linux or macOS, so the deadline is a hand-rolled watchdog - rather than ``timeout``. + That host may be macOS, which ships no ``timeout``, so every use of it has to + be guarded by a probe with a fallback behind it. """ - assert "timeout" not in command + assert "command -v timeout" in command + assert "Time::HiRes" in command @pytest.mark.parametrize( @@ -297,18 +300,44 @@ def test_submission_commands_wait_on_the_channel_and_send_to_the_target(command: assert channel_index < target_index -def test_signal_only_command_bounds_the_waiter_with_a_watchdog() -> None: - """A killed ``tmux wait-for`` exits 0, so the exit code must come from the watchdog's marker.""" - command = _build_signal_only_command(2.0, "chan", _TARGET) - assert 'kill "$waiter"' in command - assert '[ ! -s "$tmo" ]' in command +def test_perl_fallback_uses_a_fractional_alarm() -> None: + """The builtin ``alarm`` truncates to whole seconds, and ``alarm(0)`` *cancels* the timer. + + A sub-second deadline would therefore mean no deadline at all, so the fallback + has to arm the alarm through ``Time::HiRes``. + """ + assert "Time::HiRes" in _timeout_prefix(0.5) -def test_signal_or_marker_command_kills_the_tmux_client_not_a_wrapper_subshell() -> None: - """The waiter must be the tmux client itself, or killing it leaves the client running forever.""" - command = _build_signal_or_marker_command(2.0, "chan", _TARGET, "printf ''") - assert 'tmux wait-for "$1" & waiter=$!' in command - assert 'kill "$waiter" "$watchdog"' in command +def test_perl_fallback_fails_when_the_command_cannot_be_exec_ed() -> None: + """A failed ``exec`` returns to perl, which would otherwise run off the end and exit 0.""" + assert "or exit 127" in _timeout_prefix(2.0) + + +@pytest.mark.parametrize( + ("seconds", "expected_returncode"), + [ + # Sub-second: the whole-second `alarm` would round this to a cancelled timer. + (0.5, 142), + (1.0, 142), + ], +) +def test_perl_fallback_kills_a_command_that_outlives_its_deadline(seconds: float, expected_returncode: int) -> None: + """142 is 128 + SIGALRM: the deadline killed it, rather than the command exiting on its own.""" + script = f"set -- sleep 30; {_timeout_prefix(seconds)}" + result = subprocess.run( + ["bash", "-c", script], capture_output=True, text=True, env=_path_without_timeout(), timeout=30 + ) + assert result.returncode == expected_returncode + + +def test_perl_fallback_passes_through_the_command_s_own_exit_status() -> None: + """``exec`` replaces perl, so a command that finishes first reports its own status.""" + script = f'set -- sh -c "exit 7"; {_timeout_prefix(10.0)}' + result = subprocess.run( + ["bash", "-c", script], capture_output=True, text=True, env=_path_without_timeout(), timeout=30 + ) + assert result.returncode == 7 @pytest.mark.tmux @@ -350,6 +379,16 @@ def test_send_enter_via_hook_confirms_on_the_hook_when_the_marker_never_advances ) +def _path_without_timeout() -> dict[str, str]: + """Env whose PATH has no ``timeout``, so the perl fallback is what runs (as on macOS).""" + env = dict(os.environ) + timeout_binary = shutil.which("timeout") + if timeout_binary is not None: + directories = [d for d in env["PATH"].split(os.pathsep) if d != str(Path(timeout_binary).parent)] + env["PATH"] = os.pathsep.join(directories) + return env + + def _run_with_failing_tmux(command: str, tmp_path: Path) -> subprocess.CompletedProcess[str]: """Run a built submission command with a ``tmux`` on PATH that fails immediately.""" shim_dir = tmp_path / "failing_tmux_bin" From 56a8cdf2cb475e49a663868ae39cb699e0eddbf7 Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Tue, 14 Jul 2026 14:16:41 -0700 Subject: [PATCH 11/14] Drop comparative and historical narration from the new comments The _timeout_prefix docstring argued for command -v against a platform test nobody had proposed, and a reconcile-offset test docstring described the implementation it replaced. Both state what the code is now instead. --- libs/mngr/imbue/mngr/agents/tui_utils.py | 21 +++++++------------ .../resources/mngr_transcript_lib_test.py | 6 +----- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/libs/mngr/imbue/mngr/agents/tui_utils.py b/libs/mngr/imbue/mngr/agents/tui_utils.py index 047d256ab2..bbb7aa792e 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils.py @@ -299,21 +299,16 @@ def _send_enter_and_wait_for_signal( def _timeout_prefix(seconds: float) -> str: - """A ``timeout(1)`` invocation that also works where there is no ``timeout`` binary. + """Run the arguments under a deadline, using ``timeout(1)`` wherever it exists. - macOS ships a BSD userland with no ``timeout``. Perl's ``alarm`` arms a SIGALRM - and ``exec`` then replaces perl with the command itself, so the command keeps - perl's pid: its exit status is passed straight through, an expiry kills it - outright, and nothing is left behind to reap. Two sharp edges, both load-bearing: + macOS ships no ``timeout``. In its place, perl's ``alarm`` arms a SIGALRM and + ``exec`` replaces perl with the command, which therefore reports its own exit + status and dies outright when the alarm fires. - - ``Time::HiRes`` because the builtin ``alarm`` takes whole seconds and - ``alarm(0)`` *cancels* the timer, so a sub-second deadline would silently - mean no deadline at all. - - ``or exit 127`` because a failed ``exec`` returns to perl, which would - otherwise run off the end of the program and exit 0. - - ``command -v`` rather than a platform test: the binary that is missing is what - matters, and a host can have either one without the other. + The alarm comes from ``Time::HiRes`` because the builtin one takes whole + seconds and ``alarm(0)`` cancels the timer, so a sub-second deadline would + mean no deadline. A failed ``exec`` returns to perl, which would fall off the + end of the program and exit 0, so it exits 127 instead. """ perl_timeout = f"perl -MTime::HiRes=alarm -e 'alarm shift; exec @ARGV or exit 127' {seconds}" return f'if command -v timeout >/dev/null 2>&1; then timeout {seconds} "$@"; else {perl_timeout} "$@"; fi' diff --git a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py index 7db82b85a0..6a758cf6b8 100644 --- a/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py +++ b/libs/mngr/imbue/mngr/resources/mngr_transcript_lib_test.py @@ -69,11 +69,7 @@ def test_reconcile_offset_ignores_an_unterminated_final_line( def test_reconcile_offset_returns_the_highest_matching_line_not_the_first( tmp_path: Path, posix_only_path: dict[str, str], bash_with_associative_arrays: str ) -> None: - """With a gap in the emitted set, the offset still advances to the last match. - - The reverse scan this replaced stopped at the first match from the end, which is - the same line; a forward scan must not stop at the first match from the start. - """ + """With a gap in the emitted set, the offset is the last matching line, not the first.""" session_file = tmp_path / "session.jsonl" output_file = tmp_path / "output.jsonl" _write_jsonl(session_file, ["id-1", "id-2", "id-3"]) From cbf86422a5587394370d9dc32eb9516d20d6626a Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Tue, 14 Jul 2026 14:26:05 -0700 Subject: [PATCH 12/14] Flatten the timeout shim into a single constant named _timeout timeout(1) and the perl form both read the deadline as their first argument, so the shell function can take it as $1 and pass "$@" through to whichever it uses. That makes the definition invariant, so _timeout_prefix() and _timeout_function() collapse into one module constant and the call sites read _timeout SECONDS CMD. Named _timeout rather than mngr_timeout: the mngr_ prefix is for the shared shell libs, which are sourced into a namespace they share; these scripts are a self-contained bash -c. The tests now call the function directly instead of prepending 'set --', and one more asserts the 127 exit on an exec that cannot run. --- libs/mngr/imbue/mngr/agents/tui_utils.py | 49 ++++++++++--------- libs/mngr/imbue/mngr/agents/tui_utils_test.py | 47 +++++++++--------- 2 files changed, 49 insertions(+), 47 deletions(-) diff --git a/libs/mngr/imbue/mngr/agents/tui_utils.py b/libs/mngr/imbue/mngr/agents/tui_utils.py index bbb7aa792e..bff2606201 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils.py @@ -39,6 +39,24 @@ _NON_ALNUM_RE: Final[re.Pattern[str]] = re.compile(r"[^a-z0-9]") +# Defines ``_timeout SECONDS COMMAND...`` for the scripts emitted below: it runs +# COMMAND under ``timeout(1)`` if that exists, and under perl otherwise. Linux ships +# ``timeout``; macOS ships no ``timeout`` but does ship perl. Both forms read the +# deadline as their first argument, so ``"$@"`` reaches either unchanged. +# +# The perl form arms a SIGALRM, then ``exec`` replaces perl with COMMAND itself, so +# COMMAND reports its own exit status and dies outright when the alarm fires. The +# alarm comes from ``Time::HiRes`` because the builtin one takes whole seconds and +# ``alarm(0)`` cancels the timer, so a sub-second deadline would mean no deadline. A +# failed ``exec`` returns to perl, which would fall off the end of the program and +# exit 0, so it exits 127 instead. +_TIMEOUT_FUNCTION: Final[str] = ( + "_timeout() { " + 'if command -v timeout >/dev/null 2>&1; then timeout "$@"; ' + "else perl -MTime::HiRes=alarm -e 'alarm shift; exec @ARGV or exit 127' \"$@\"; fi; " + "}; " +) + def _normalize_for_match(text: str) -> str: """Strip non-alphanumeric characters and lowercase for fuzzy matching.""" @@ -298,27 +316,6 @@ def _send_enter_and_wait_for_signal( return False -def _timeout_prefix(seconds: float) -> str: - """Run the arguments under a deadline, using ``timeout(1)`` wherever it exists. - - macOS ships no ``timeout``. In its place, perl's ``alarm`` arms a SIGALRM and - ``exec`` replaces perl with the command, which therefore reports its own exit - status and dies outright when the alarm fires. - - The alarm comes from ``Time::HiRes`` because the builtin one takes whole - seconds and ``alarm(0)`` cancels the timer, so a sub-second deadline would - mean no deadline. A failed ``exec`` returns to perl, which would fall off the - end of the program and exit 0, so it exits 127 instead. - """ - perl_timeout = f"perl -MTime::HiRes=alarm -e 'alarm shift; exec @ARGV or exit 127' {seconds}" - return f'if command -v timeout >/dev/null 2>&1; then timeout {seconds} "$@"; else {perl_timeout} "$@"; fi' - - -def _timeout_function(seconds: float) -> str: - """Define ``mngr_timeout`` in the emitted script; it runs its arguments under a deadline.""" - return f"mngr_timeout() {{ {_timeout_prefix(seconds)}; }}; " - - def _build_signal_only_command(full_timeout: float, wait_channel: str, tmux_target: TmuxWindowTarget) -> str: """Send Enter, then block on the hook's wait-for channel until it fires or the deadline passes. @@ -327,7 +324,11 @@ def _build_signal_only_command(full_timeout: float, wait_channel: str, tmux_targ registered by the time the hook can fire. The exit status is the waiter's, so exit 0 = signalled, non-zero = deadline reached or tmux failed. """ - script = f'{_timeout_function(full_timeout)}( sleep 0.1 && tmux send-keys -t "$2" Enter ) & mngr_timeout tmux wait-for "$1"' + script = ( + f"{_TIMEOUT_FUNCTION}" + '( sleep 0.1 && tmux send-keys -t "$2" Enter ) & ' + f'_timeout {full_timeout} tmux wait-for "$1"' + ) return f"bash -c {shlex.quote(script)} _ {shlex.quote(wait_channel)} {tmux_target.as_shell_arg()}" @@ -362,7 +363,7 @@ def _build_signal_or_marker_command( agent that supplies it. """ script = ( - f"{_timeout_function(full_timeout)}" + f"{_TIMEOUT_FUNCTION}" 'sig="$(mktemp)"; ' # Clean up on every exit path: remove the sentinel file and reap any # still-running background job (notably the hook waiter, which otherwise @@ -371,7 +372,7 @@ def _build_signal_or_marker_command( 'trap \'p="$(jobs -p)"; [ -n "$p" ] && kill $p 2>/dev/null; rm -f "$sig"\' EXIT; ' f'base="$({accept_marker_command})"; ' # Register the hook waiter first (full timeout), sentinel on success. - '( mngr_timeout tmux wait-for "$1" >/dev/null 2>&1 && echo 1 > "$sig" ) & ' + f'( _timeout {full_timeout} tmux wait-for "$1" >/dev/null 2>&1 && echo 1 > "$sig" ) & ' # Then submit, after a beat so the waiter is registered. '( sleep 0.1 && tmux send-keys -t "$2" Enter ) & ' f'end="$(( $(date +%s) + {int(full_timeout) + 1} ))"; ' diff --git a/libs/mngr/imbue/mngr/agents/tui_utils_test.py b/libs/mngr/imbue/mngr/agents/tui_utils_test.py index 803f6f01e1..c58ed9d48c 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils_test.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils_test.py @@ -11,11 +11,11 @@ import pytest from imbue.mngr.agents.base_agent import BaseAgent +from imbue.mngr.agents.tui_utils import _TIMEOUT_FUNCTION from imbue.mngr.agents.tui_utils import _build_signal_only_command from imbue.mngr.agents.tui_utils import _build_signal_or_marker_command from imbue.mngr.agents.tui_utils import _check_paste_content from imbue.mngr.agents.tui_utils import _normalize_for_match -from imbue.mngr.agents.tui_utils import _timeout_prefix from imbue.mngr.agents.tui_utils import send_enter_and_poll_for_cleared_indicator from imbue.mngr.agents.tui_utils import send_enter_best_effort from imbue.mngr.agents.tui_utils import send_enter_keystroke @@ -300,44 +300,45 @@ def test_submission_commands_wait_on_the_channel_and_send_to_the_target(command: assert channel_index < target_index +def _run_timeout_function(arguments: str) -> subprocess.CompletedProcess[str]: + """Run ``_timeout `` on a PATH with no ``timeout``, so the perl fallback is what runs.""" + return subprocess.run( + ["bash", "-c", f"{_TIMEOUT_FUNCTION} _timeout {arguments}"], + capture_output=True, + text=True, + env=_path_without_timeout(), + timeout=30, + ) + + def test_perl_fallback_uses_a_fractional_alarm() -> None: """The builtin ``alarm`` truncates to whole seconds, and ``alarm(0)`` *cancels* the timer. A sub-second deadline would therefore mean no deadline at all, so the fallback has to arm the alarm through ``Time::HiRes``. """ - assert "Time::HiRes" in _timeout_prefix(0.5) + assert "Time::HiRes" in _TIMEOUT_FUNCTION def test_perl_fallback_fails_when_the_command_cannot_be_exec_ed() -> None: """A failed ``exec`` returns to perl, which would otherwise run off the end and exit 0.""" - assert "or exit 127" in _timeout_prefix(2.0) + assert "or exit 127" in _TIMEOUT_FUNCTION + assert _run_timeout_function("5 /nonexistent-binary-for-this-test").returncode == 127 -@pytest.mark.parametrize( - ("seconds", "expected_returncode"), - [ - # Sub-second: the whole-second `alarm` would round this to a cancelled timer. - (0.5, 142), - (1.0, 142), - ], -) -def test_perl_fallback_kills_a_command_that_outlives_its_deadline(seconds: float, expected_returncode: int) -> None: - """142 is 128 + SIGALRM: the deadline killed it, rather than the command exiting on its own.""" - script = f"set -- sleep 30; {_timeout_prefix(seconds)}" - result = subprocess.run( - ["bash", "-c", script], capture_output=True, text=True, env=_path_without_timeout(), timeout=30 - ) - assert result.returncode == expected_returncode +@pytest.mark.parametrize("seconds", [0.5, 1.0]) +def test_perl_fallback_kills_a_command_that_outlives_its_deadline(seconds: float) -> None: + """142 is 128 + SIGALRM: the deadline killed it, rather than the command exiting on its own. + + A sub-second deadline is the interesting case, since the whole-second ``alarm`` + would round it down to a cancelled timer and never fire at all. + """ + assert _run_timeout_function(f"{seconds} sleep 30").returncode == 142 def test_perl_fallback_passes_through_the_command_s_own_exit_status() -> None: """``exec`` replaces perl, so a command that finishes first reports its own status.""" - script = f'set -- sh -c "exit 7"; {_timeout_prefix(10.0)}' - result = subprocess.run( - ["bash", "-c", script], capture_output=True, text=True, env=_path_without_timeout(), timeout=30 - ) - assert result.returncode == 7 + assert _run_timeout_function('10 sh -c "exit 7"').returncode == 7 @pytest.mark.tmux From cfd91f93ba90e084c4db2522d25c87c9c8648a9d Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Tue, 14 Jul 2026 14:32:32 -0700 Subject: [PATCH 13/14] Fix the perl-fallback tests, which only passed on a host without timeout The tests drove the whole _timeout function and tried to force the perl branch by stripping timeout's directory from PATH. That cannot work on Linux, where timeout, perl and sleep all live in /usr/bin: removing it would take perl with it, and a second copy on the path meant CI ran the timeout branch and got 124, not 142. The perl invocation is now its own constant, so the tests exercise it directly and no longer care whether the host also has timeout. A separate test covers the function as a whole, asserting only the outcome, since the two branches report expiry differently (124 vs SIGALRM). The expiry assertion also had to change: subprocess reports a signalled child as a negative return code, and bash exec-optimizes a single simple command, so perl IS the spawned process and its death reads as -SIGALRM rather than 128 + SIGALRM. The test accepts either and additionally asserts the command did not run to completion, which is what a cancelled alarm(0) would have caused. --- libs/mngr/imbue/mngr/agents/tui_utils.py | 6 +- libs/mngr/imbue/mngr/agents/tui_utils_test.py | 69 +++++++++---------- 2 files changed, 33 insertions(+), 42 deletions(-) diff --git a/libs/mngr/imbue/mngr/agents/tui_utils.py b/libs/mngr/imbue/mngr/agents/tui_utils.py index bff2606201..5ec762437d 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils.py @@ -50,11 +50,9 @@ # ``alarm(0)`` cancels the timer, so a sub-second deadline would mean no deadline. A # failed ``exec`` returns to perl, which would fall off the end of the program and # exit 0, so it exits 127 instead. +_PERL_TIMEOUT: Final[str] = "perl -MTime::HiRes=alarm -e 'alarm shift; exec @ARGV or exit 127'" _TIMEOUT_FUNCTION: Final[str] = ( - "_timeout() { " - 'if command -v timeout >/dev/null 2>&1; then timeout "$@"; ' - "else perl -MTime::HiRes=alarm -e 'alarm shift; exec @ARGV or exit 127' \"$@\"; fi; " - "}; " + f'_timeout() {{ if command -v timeout >/dev/null 2>&1; then timeout "$@"; else {_PERL_TIMEOUT} "$@"; fi; }}; ' ) diff --git a/libs/mngr/imbue/mngr/agents/tui_utils_test.py b/libs/mngr/imbue/mngr/agents/tui_utils_test.py index c58ed9d48c..d346357575 100644 --- a/libs/mngr/imbue/mngr/agents/tui_utils_test.py +++ b/libs/mngr/imbue/mngr/agents/tui_utils_test.py @@ -1,8 +1,9 @@ """Unit tests for tui_utils.""" import os -import shutil +import signal import subprocess +import time from datetime import datetime from datetime import timezone from pathlib import Path @@ -11,6 +12,7 @@ import pytest from imbue.mngr.agents.base_agent import BaseAgent +from imbue.mngr.agents.tui_utils import _PERL_TIMEOUT from imbue.mngr.agents.tui_utils import _TIMEOUT_FUNCTION from imbue.mngr.agents.tui_utils import _build_signal_only_command from imbue.mngr.agents.tui_utils import _build_signal_or_marker_command @@ -300,45 +302,46 @@ def test_submission_commands_wait_on_the_channel_and_send_to_the_target(command: assert channel_index < target_index -def _run_timeout_function(arguments: str) -> subprocess.CompletedProcess[str]: - """Run ``_timeout `` on a PATH with no ``timeout``, so the perl fallback is what runs.""" - return subprocess.run( - ["bash", "-c", f"{_TIMEOUT_FUNCTION} _timeout {arguments}"], - capture_output=True, - text=True, - env=_path_without_timeout(), - timeout=30, - ) +def _run_perl_timeout(arguments: str) -> subprocess.CompletedProcess[str]: + """Run the perl fallback directly, so the assertions hold on a host that also has ``timeout``.""" + return subprocess.run(["bash", "-c", f"{_PERL_TIMEOUT} {arguments}"], capture_output=True, text=True, timeout=60) -def test_perl_fallback_uses_a_fractional_alarm() -> None: - """The builtin ``alarm`` truncates to whole seconds, and ``alarm(0)`` *cancels* the timer. - - A sub-second deadline would therefore mean no deadline at all, so the fallback - has to arm the alarm through ``Time::HiRes``. - """ - assert "Time::HiRes" in _TIMEOUT_FUNCTION - - -def test_perl_fallback_fails_when_the_command_cannot_be_exec_ed() -> None: - """A failed ``exec`` returns to perl, which would otherwise run off the end and exit 0.""" - assert "or exit 127" in _TIMEOUT_FUNCTION - assert _run_timeout_function("5 /nonexistent-binary-for-this-test").returncode == 127 +def _is_killed_by_the_alarm(result: subprocess.CompletedProcess[str]) -> bool: + """SIGALRM reads as ``-SIGALRM`` when bash ``exec``-ed perl, and as ``128 + SIGALRM`` when a shell outlived it.""" + return result.returncode in (-signal.SIGALRM, 128 + signal.SIGALRM) @pytest.mark.parametrize("seconds", [0.5, 1.0]) def test_perl_fallback_kills_a_command_that_outlives_its_deadline(seconds: float) -> None: - """142 is 128 + SIGALRM: the deadline killed it, rather than the command exiting on its own. + """A sub-second deadline is the interesting case. - A sub-second deadline is the interesting case, since the whole-second ``alarm`` - would round it down to a cancelled timer and never fire at all. + The whole-second ``alarm`` would round it down to ``alarm(0)``, which cancels the + timer outright, so ``sleep 30`` would run to completion and report success. """ - assert _run_timeout_function(f"{seconds} sleep 30").returncode == 142 + start_time = time.monotonic() + result = _run_perl_timeout(f"{seconds} sleep 30") + assert _is_killed_by_the_alarm(result) + assert time.monotonic() - start_time < 10.0 def test_perl_fallback_passes_through_the_command_s_own_exit_status() -> None: """``exec`` replaces perl, so a command that finishes first reports its own status.""" - assert _run_timeout_function('10 sh -c "exit 7"').returncode == 7 + assert _run_perl_timeout('10 sh -c "exit 7"').returncode == 7 + + +def test_perl_fallback_fails_when_the_command_cannot_be_exec_ed() -> None: + """A failed ``exec`` returns to perl, which would otherwise run off the end and exit 0.""" + assert _run_perl_timeout("5 /nonexistent-binary-for-this-test").returncode == 127 + + +@pytest.mark.parametrize("seconds", [0.5, 30.0]) +def test_timeout_function_runs_the_command_under_whichever_deadline_the_host_has(seconds: float) -> None: + """The exit status differs by branch (``timeout`` gives 124, perl 142), so only the outcome is asserted.""" + script = f"{_TIMEOUT_FUNCTION} _timeout {seconds} sh -c 'sleep 5; exit 0'" + result = subprocess.run(["bash", "-c", script], capture_output=True, text=True, timeout=60) + expired = seconds < 5 + assert (result.returncode != 0) is expired @pytest.mark.tmux @@ -380,16 +383,6 @@ def test_send_enter_via_hook_confirms_on_the_hook_when_the_marker_never_advances ) -def _path_without_timeout() -> dict[str, str]: - """Env whose PATH has no ``timeout``, so the perl fallback is what runs (as on macOS).""" - env = dict(os.environ) - timeout_binary = shutil.which("timeout") - if timeout_binary is not None: - directories = [d for d in env["PATH"].split(os.pathsep) if d != str(Path(timeout_binary).parent)] - env["PATH"] = os.pathsep.join(directories) - return env - - def _run_with_failing_tmux(command: str, tmp_path: Path) -> subprocess.CompletedProcess[str]: """Run a built submission command with a ``tmux`` on PATH that fails immediately.""" shim_dir = tmp_path / "failing_tmux_bin" From a1f2907409f485573f61ff94f9d2eab4aa277b4e Mon Sep 17 00:00:00 2001 From: Weishi Zeng Date: Tue, 14 Jul 2026 17:08:55 -0700 Subject: [PATCH 14/14] Record the portable-shell technique in the style guide The perl `alarm` fallback for `timeout(1)` this branch added is superseded by #2475 and no longer has a caller, so keeping it as code would be dead code. Its knowledge is not: the same class of macOS/Linux userland mismatch recurs (du -sb, stat -c, tac), so record the rule and the validated portable forms -- including the perl timeout form and its two pitfalls -- in the repo-root style_guide.md. Rewrite the mngr changelog to drop the superseded timeout fix and cover only the tac and du/stat portability fixes that remain. Co-Authored-By: Claude Opus 4.8 (1M context) --- dev/changelog/mngr-timeout-tac.md | 1 + libs/mngr/changelog/mngr-timeout-tac.md | 12 +++++------- style_guide.md | 16 ++++++++++++++++ 3 files changed, 22 insertions(+), 7 deletions(-) create mode 100644 dev/changelog/mngr-timeout-tac.md diff --git a/dev/changelog/mngr-timeout-tac.md b/dev/changelog/mngr-timeout-tac.md new file mode 100644 index 0000000000..cb71b3871f --- /dev/null +++ b/dev/changelog/mngr-timeout-tac.md @@ -0,0 +1 @@ +Added a "Portable shell in host commands" section to the repo-root `style_guide.md`. It states the rule that commands passed to a host's `execute_*` methods run under the host's own userland -- BSD on a local macOS machine, GNU on Linux remotes -- and gives the validated portable forms for the cases mngr has hit: `du -sk` over `du -sb`, `stat -c … || stat -f …`, a forward scan over `tac`/`tail -r`, and a perl `alarm` form (with its two pitfalls) for bounding a sub-command in-shell where `timeout(1)` is unavailable. diff --git a/libs/mngr/changelog/mngr-timeout-tac.md b/libs/mngr/changelog/mngr-timeout-tac.md index b3d893882f..002cf7c7d0 100644 --- a/libs/mngr/changelog/mngr-timeout-tac.md +++ b/libs/mngr/changelog/mngr-timeout-tac.md @@ -1,13 +1,11 @@ -Fixed four places where mngr shelled out to GNU-coreutils binaries and flags that stock macOS does not ship, each of which ran on the user's own machine whenever an agent used the `local` provider. All four were verified failing on macOS 26.4.1 with a stock `PATH`, then verified fixed. - -`mngr message` to a local codex or antigravity agent raised `SendMessageError("Timeout waiting for message submission signal")` on every send. The submission path bounded its `tmux wait-for` with `timeout(1)`, which exits 127 (`timeout: command not found`) on macOS -- immediately, while reporting that it had waited the full timeout. For Claude agents (which also watch an acceptance marker) the same bug degraded silently instead: the `timeout` failure was swallowed inside a backgrounded subshell, so the hook path never confirmed and only the transcript marker could -- meaning `/clear` and `/compact`, which fire the hook but never enqueue a model turn, always burned the full timeout. - -The submission logic is unchanged; only the deadline is. Where `timeout` exists it is still used, and where it does not the deadline comes from perl's `alarm`, which arms a SIGALRM before `exec` replaces perl with the command itself. Because the command inherits perl's pid, its exit status passes straight through, an expiry kills it outright, and there is no supervising process to leak. Two details are load-bearing: the alarm is armed through `Time::HiRes`, since the builtin `alarm` takes whole seconds and `alarm(0)` *cancels* the timer, so a sub-second deadline would otherwise mean no deadline at all; and a failed `exec` explicitly exits 127, since it would otherwise return to perl, run off the end of the program, and report success. Whether to use `timeout` is decided by probing for it rather than by testing the platform, because a host can have either it or perl without the other. +Fixed the remaining places where mngr shelled out to GNU-coreutils binaries and flags that stock macOS does not ship, each of which ran on the user's own machine whenever an agent used the `local` provider. All were verified failing on macOS 26.4.1 with a stock `PATH`, then verified fixed. Raw-transcript streaming re-emitted already-emitted lines on macOS. `mngr_transcript_reconcile_offset` reverse-scanned the session file with `tac` to find the last emitted line; with `tac` absent the scan read nothing and the offset reset to 0. It now scans forward and tracks the last match, which needs no reverse at all. This also fixes a latent off-by-one: the old code combined `wc -l` (which does not count an unterminated final line) with `tac` (which emits it), so a session file whose last line was still being appended -- the normal state of a live transcript -- reconciled one line too low. -Transcript-streamer startup also got much faster. Both `mngr_transcript_build_id_set` and `mngr_transcript_reconcile_offset` used to extract each line's correlation field through a command substitution, which forks a subshell per line; they now match it with an inline bash regex and fork nothing. The dominant cost was `build_id_set`, which scans the whole already-emitted output: on a 50,000-line file it drops from about 32s to 0.7s on macOS, and about 14s to 0.6s on Linux (three trials each; forks are cheaper on Linux). `reconcile_offset` is subtler -- removing `tac` also removed its early exit on the first match from the end, so the new forward scan always reads the whole file (about 0.7s at 50,000 lines) instead of sometimes stopping early. But because it no longer forks, its worst case -- a match near the start, which made the old reverse scan fork through the entire file at about 12s -- is now that same bounded 0.7s. Combined startup on a 50,000-line pair is about 1.5s, down from tens of seconds. The inline match uses bash's built-in `[[ =~ ]]`, so unlike the `tac` it replaced it needs no external binary and behaves identically on macOS and Linux. +Transcript-streamer startup also got much faster. Both `mngr_transcript_build_id_set` and `mngr_transcript_reconcile_offset` used to extract each line's correlation field through a command substitution, which forks a subshell per line; they now match it with an inline bash regex and fork nothing. The dominant cost was `build_id_set`, which scans the whole already-emitted output: on a 50,000-line file it drops from about 32s to 0.7s on macOS, and about 14s to 0.6s on Linux (three trials each; forks are cheaper on Linux). `reconcile_offset` is subtler -- removing `tac` also removed its early exit on the first match from the end, so the new forward scan always reads the whole file (about 0.7s at 50,000 lines) instead of sometimes stopping early. But because it no longer forks, its worst case -- a match near the start, which made the old reverse scan fork through the entire file at about 12s -- is now that same bounded 0.7s. The inline match uses bash's built-in `[[ =~ ]]`, so unlike the `tac` it replaced it needs no external binary and behaves identically on macOS and Linux. `mngr gc` mis-measured orphaned work directories on macOS. `du -sb` has no BSD equivalent (`-b` is rejected), and because the command was piped into `cut`, the pipeline exit status was `cut`'s, so the failure looked like success with empty output and every orphan was reported as 0 bytes. `stat -c %Y` is likewise rejected by BSD `stat`, so each orphan's `created_at` fell back to `datetime.now()` and looked brand-new, meaning age-based collection never ran on macOS. -Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it runs POSIX `test -d ... && du -sk` on the host -- locally via subprocess, remotely over SSH -- so callers never branch on `is_local`. Sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines, and a path that is not a directory reports 0. The trailing slash resolves a symlinked path to its target directory, and `du` exiting non-zero is tolerated because it still prints a correct total after skipping an unreadable subdirectory (the piped-into-`cut` form this replaced happened to mask that, and dropping the pipe would otherwise have turned a working directory into a reported 0 bytes). The exact `du -sk /` invocation was verified on stock macOS (BSD `du`) and on Linux (GNU and busybox `du`) to agree on the load-bearing cases: a hard-linked inode counted once, a nested symlinked directory not followed, and a top-level symlink resolved. A single `du` call rather than a Python reimplementation for local hosts follows the same rule as the `timeout` fix above -- call the tool instead of reimplementing it. +Both `gc` call sites now go through the host interface instead of hand-rolled shell. `OuterHostInterface` gains a concrete `get_directory_size`, alongside the existing `path_exists` and `get_file_mtime`: it runs POSIX `test -d ... && du -sk` on the host -- locally via subprocess, remotely over SSH -- so callers never branch on `is_local`. Sizes are whole kibibytes, since `-k` is the only `du` block size POSIX defines, and a path that is not a directory reports 0. The trailing slash resolves a symlinked path to its target directory, and `du` exiting non-zero is tolerated because it still prints a correct total after skipping an unreadable subdirectory. The exact `du -sk /` invocation was verified on stock macOS (BSD `du`) and on Linux (GNU and busybox `du`) to agree on the load-bearing cases: a hard-linked inode counted once, a nested symlinked directory not followed, and a top-level symlink resolved. + +A further fix originally on this branch -- a perl `alarm` fallback for `timeout(1)` in the tmux message-submission path -- was dropped when #2475 replaced that path's confirmation engine wholesale (durable-evidence polling, no `timeout(1)`). The portable-shell technique it established, including the perl timeout form and its two pitfalls, is preserved in the repo-root `style_guide.md` instead of in code with no remaining caller. diff --git a/style_guide.md b/style_guide.md index feba67b40d..ba2eb94d74 100644 --- a/style_guide.md +++ b/style_guide.md @@ -550,6 +550,22 @@ When calling external commands or making network requests, always use a two-thre This pattern allows us to notice degradation and diagnose slowdowns before they become outright failures. +# Portable shell in host commands + +Commands passed to a host's `execute_*` methods run under that host's own shell. Under the `local` provider the host is the developer's machine, which on macOS ships a BSD userland with no GNU coreutils; remote hosts are Linux/GNU. A command that works under only one userland fails silently on the other -- a GNU-only flag makes the tool exit non-zero, and a downstream `| cut` or `|| true` often masks the failure as an empty or default result. Write for both: + +- Prefer a POSIX form over a GNU one: `du -sk` (kibibytes), not `du -sb` (bytes, GNU-only). +- Where GNU and BSD spell the same operation differently, provide both with a fallback: `stat -c %y … || stat -f %Fm …`. +- Do not assume a GNU-only binary exists: `tac` (GNU) and `tail -r` (BSD) are mutually exclusive by platform and neither is POSIX, so a forward scan that keeps the last match needs neither. + +Bounding a single sub-command's runtime *inside* a composite remote script is the one case with no clean POSIX form, since `timeout(1)` is GNU and macOS lacks it. Prefer bounding the whole command from Python via the `timeout_seconds` argument. If a sub-part must be bounded in-shell, this perl form is equivalent, and perl ships on both macOS and Linux: + +```sh +perl -MTime::HiRes=alarm -e 'alarm shift; exec @ARGV or exit 127' +``` + +`exec` replaces perl with the command, so the command keeps perl's pid and reports its own exit status, and the alarm kills it outright on expiry. Two non-obvious traps: the builtin `alarm` takes whole seconds and `alarm(0)` cancels the timer, so a sub-second deadline silently means no deadline -- import `Time::HiRes`'s float `alarm`; and a failed `exec` falls through to the rest of the perl program, which exits 0 by default, so terminate it explicitly with `or exit 127`. + # Docstrings We want our code to be self-documenting as much as possible