From dc647f4d6331549859c79141862cb6e59f736d2e Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Fri, 10 Jul 2026 15:25:21 -0700 Subject: [PATCH] fix(bash): report exit code for killed shells; single import style in registry tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot: bash_output's schema promises the exit code once the shell has exited, but the formatting attached it only to 'completed' — a killed shell has one too (the negated signal number). Attach it to any exited state. Code-quality: the registry test file mixed a top-level from-import with function-local 'import ... as bg_mod' for monkeypatching module attributes; one from-style module alias at the top now serves all of them. --- tests/test_background_shells.py | 15 +++++---------- tests/test_bash_background_tool.py | 4 ++++ turnstone/core/session.py | 7 +++++-- 3 files changed, 14 insertions(+), 12 deletions(-) diff --git a/tests/test_background_shells.py b/tests/test_background_shells.py index 334cc45d..a748b66a 100644 --- a/tests/test_background_shells.py +++ b/tests/test_background_shells.py @@ -20,8 +20,13 @@ import pytest from tests._proc_helpers import kill_pid as _kill_pid from tests._proc_helpers import pid_alive as _pid_alive from tests._proc_helpers import poll_until as _wait_until + +# Module alias (from-style, matching the symbol imports below) for tests +# that monkeypatch module attributes (os.killpg, subprocess.Popen, ...). +from turnstone.core import background_shells as bg_mod from turnstone.core.background_shells import ( BackgroundShellRegistry, + FilterExecError, FilterTimeoutError, TooManyShellsError, UnknownShellError, @@ -441,8 +446,6 @@ def test_kill_on_completed_shell_does_not_signal_group(registry, monkeypatch): """A completed shell's pgid is a stale snapshot the OS may have recycled to an unrelated process group — kill() must not signal it (the waiter's own group kill already ran at exit, when the pgid was fresh).""" - import turnstone.core.background_shells as bg_mod - shell = registry.spawn("true") assert _wait_status(shell, "completed") calls = [] @@ -562,8 +565,6 @@ def test_thread_start_failure_leaves_no_orphan_record(registry, monkeypatch, tmp unregistered and the fresh group reaped — an orphan with never-started Thread objects would make every later close()/reap() join raise and abort session teardown.""" - import turnstone.core.background_shells as bg_mod - pidfile = tmp_path / "leader.pid" real_thread = bg_mod.threading.Thread @@ -587,9 +588,6 @@ def test_thread_start_failure_leaves_no_orphan_record(registry, monkeypatch, tmp def test_filter_helper_failure_reports_exec_error_not_timeout(registry, monkeypatch): """A crashed helper must not tell the model its (fine) pattern was too slow — and must not consume the delta.""" - import turnstone.core.background_shells as bg_mod - from turnstone.core.background_shells import FilterExecError - shell = registry.spawn("echo hello") assert _wait_status(shell, "completed") monkeypatch.setattr(bg_mod.sys, "executable", "/bin/false") @@ -643,9 +641,6 @@ def test_filter_helper_spawn_failure_is_exec_error(registry, monkeypatch): """A helper that fails to LAUNCH (fork pressure) must land in the same honest FilterExecError as a crashed helper — not escape as a raw OSError blaming nothing — and must not consume the delta.""" - import turnstone.core.background_shells as bg_mod - from turnstone.core.background_shells import FilterExecError - shell = registry.spawn("echo hello") assert _wait_status(shell, "completed") diff --git a/tests/test_bash_background_tool.py b/tests/test_bash_background_tool.py index 2172e033..8e1bcae3 100644 --- a/tests/test_bash_background_tool.py +++ b/tests/test_bash_background_tool.py @@ -219,6 +219,10 @@ def test_kill_shell_kills_and_reports(session): _cid, output = prepared["execute"](prepared) assert "killed" in output.lower() assert _wait_until(lambda: not _pid_alive(shell.pid)) + # The schema promises the exit code for ANY exited state, killed included. + read_prepared = session._prepare_bash_output("r", {"id": shell.shell_id}) + _cid, read_output = read_prepared["execute"](read_prepared) + assert "exit code" in read_output def test_kill_shell_unknown_id_reports_error(session): diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 9725d068..7df217d9 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -14574,8 +14574,11 @@ class ChatSession: self._report_tool_result(call_id, "bash_output", msg, is_error=True) return call_id, msg - if read.status == "completed": - state = f"completed, exit code {read.exit_code}" + # Exit code rides ANY exited state — the schema promises it "once + # the shell has exited", and a killed shell has one too (the signal + # number, negated). + if read.exit_code is not None: + state = f"{read.status}, exit code {read.exit_code}" else: state = read.status parts = [f"{shell_id} ({state})"]