From c8329202750662289f91fbd4fd3dd0f2fea7f935 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 15 Sep 2026 11:50:23 -0700 Subject: [PATCH] fix(tools): pre-exec guard that misses the deadline refuses the command The salvaged commit put `_pre_exec_block` behind the command's `run_bounded_sync` deadline but let a timed-out guard fall through into execution. The gateway-lifecycle, dangerous-workdir and self-repo checks apply unconditionally (`force=True` cannot bypass them), so a guard that never rendered a verdict must not let the command run unguarded: return the terminal error envelope (`status: error`, "did not finish ... Retry the call") instead, mirroring how the bounded `env.execute` path reports its own expiry as a result rather than continuing. Tests trimmed to the two invariants: a wedged guard returns a bounded error without executing; a completed guard keeps its verdict (pass -> execution, rejection -> its own blocked result). --- .../tools/test_terminal_pre_guard_deadline.py | 60 +++++++++++-------- tools/terminal_tool.py | 22 ++++--- 2 files changed, 49 insertions(+), 33 deletions(-) diff --git a/tests/tools/test_terminal_pre_guard_deadline.py b/tests/tools/test_terminal_pre_guard_deadline.py index 7dece6eb41..7d36342cc7 100644 --- a/tests/tools/test_terminal_pre_guard_deadline.py +++ b/tests/tools/test_terminal_pre_guard_deadline.py @@ -1,10 +1,18 @@ -"""Terminal pre-execution guards must not outlive the tool deadline.""" +"""Terminal pre-execution guards share the command's wall-clock deadline (#111922). + +The supervised-gateway identity probe inside ``_pre_exec_block`` ends in a kernel process +query that can wedge; before the deadline wrap, ``terminal_tool`` never returned and the +cron run held its slot forever. +""" from __future__ import annotations +import json import time from types import SimpleNamespace +import pytest + import tools.terminal_tool as terminal_module @@ -19,43 +27,47 @@ def _plan(timeout: float = 0.05) -> SimpleNamespace: ) -def test_terminal_tool_bounds_a_wedged_pre_execution_guard(monkeypatch): - """A stalled supervised-gateway identity probe cannot wedge terminal_tool.""" +@pytest.fixture +def stubbed_pipeline(monkeypatch): + """Stub planning/env/approval/execution; returns the list of executions that happened.""" + calls: list[str] = [] monkeypatch.setattr(terminal_module, "_plan_execution", lambda *_a, **_k: _plan()) monkeypatch.setattr(terminal_module, "_acquire_env", lambda *_a, **_k: object()) - monkeypatch.setattr(terminal_module, "_run_approval_guards", lambda *_a, **_k: terminal_module._ApprovalVerdict()) - monkeypatch.setattr(terminal_module, "_run_foreground", lambda *_a, **_k: "foreground-ran") + monkeypatch.setattr( + terminal_module, "_run_approval_guards", lambda *_a, **_k: terminal_module._ApprovalVerdict(), + ) + monkeypatch.setattr( + terminal_module, "_run_foreground", lambda *_a, **_k: calls.append("foreground") or "foreground-ran", + ) + return calls - def _wedged_supervised_gateway_probe(*_a, **_k): + +def test_wedged_pre_execution_guard_returns_bounded_error_without_running(monkeypatch, stubbed_pipeline): + """A stalled identity probe returns a retryable error within the deadline; the command does not run.""" + + def _wedged_probe(*_a, **_k): time.sleep(1) - monkeypatch.setattr(terminal_module, "_pre_exec_block", _wedged_supervised_gateway_probe) + monkeypatch.setattr(terminal_module, "_pre_exec_block", _wedged_probe) start = time.monotonic() - result = terminal_module.terminal_tool("echo ok") + result = json.loads(terminal_module.terminal_tool("echo ok")) elapsed = time.monotonic() - start assert elapsed < 0.5, f"pre-execution guard wedged terminal_tool for {elapsed:.2f}s" - assert result == "foreground-ran" + assert result["status"] == "error" + assert "did not finish" in result["error"] + assert stubbed_pipeline == [], "a guard with no verdict must not fail open into execution" -def test_terminal_tool_runs_normal_pre_execution_guard(monkeypatch): - """A normal guard result still reaches foreground execution unchanged.""" - monkeypatch.setattr(terminal_module, "_plan_execution", lambda *_a, **_k: _plan()) - monkeypatch.setattr(terminal_module, "_acquire_env", lambda *_a, **_k: object()) - monkeypatch.setattr(terminal_module, "_run_approval_guards", lambda *_a, **_k: terminal_module._ApprovalVerdict()) +def test_completed_pre_execution_guard_verdicts_pass_through(monkeypatch, stubbed_pipeline): + """A finished guard keeps its outcome: pass → execution, rejection → its own blocked result.""" monkeypatch.setattr(terminal_module, "_pre_exec_block", lambda *_a, **_k: None) - monkeypatch.setattr(terminal_module, "_run_foreground", lambda *_a, **_k: "foreground-ran") - assert terminal_module.terminal_tool("echo ok") == "foreground-ran" + def _rejecting_probe(*_a, **_k): + raise terminal_module._Rejected('{"status":"blocked"}') -def test_terminal_tool_preserves_pre_execution_rejection(monkeypatch): - """A completed guard rejection still returns its original tool result.""" - monkeypatch.setattr(terminal_module, "_plan_execution", lambda *_a, **_k: _plan()) - monkeypatch.setattr(terminal_module, "_acquire_env", lambda *_a, **_k: object()) - monkeypatch.setattr(terminal_module, "_pre_exec_block", lambda *_a, **_k: (_ for _ in ()).throw( - terminal_module._Rejected('{"status":"blocked"}') - )) - + monkeypatch.setattr(terminal_module, "_pre_exec_block", _rejecting_probe) assert terminal_module.terminal_tool("echo ok") == '{"status":"blocked"}' + assert stubbed_pipeline == ["foreground"] diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index be675457ec..d7a3ddabc3 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -1222,11 +1222,14 @@ def terminal_tool( session_key = get_current_session_key(default="") or (task_id or "") - # A supervised-gateway identity check can enter a kernel-level psutil - # query. Put the whole pre-execution chain behind the command's wall - # clock deadline so that a wedged probe cannot hold a cron run forever. - # A timed-out guard fails open: its worker is abandoned, while ordinary - # guard rejections and exceptions keep their original behavior. + # The supervised-gateway identity probe ends in a kernel process query + # (psutil create_time) that has wedged for the better part of an hour on + # macOS; ``env.execute`` is already behind ``run_bounded_sync`` but this + # chain ran ahead of it, so the tool call never returned and the cron + # slot stayed occupied (#111922). Share the command's own deadline. A + # guard that never rendered a verdict fails CLOSED: these checks apply + # unconditionally (``force`` cannot bypass them), so the command is + # refused with a retryable error instead of running unguarded. from agent.deadline import run_bounded_sync bounded_guard = run_bounded_sync( @@ -1237,10 +1240,11 @@ def terminal_tool( label="terminal.pre-exec-guard", ) if bounded_guard.timed_out: - logger.warning( - "Terminal pre-execution guard timed out after %ss; continuing fail-open", - plan.effective_timeout, - ) + raise _Rejected(_error_json( + f"Terminal pre-execution guard did not finish within {plan.effective_timeout}s " + "(process-identity probe wedged); the command was not run. Retry the call.", + status="error", + )) # Pre-exec security checks (tirith + dangerous command detection); # force=True means the user already confirmed. verdict = _run_approval_guards(command, env_type, plan.config, force=force)