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).
This commit is contained in:
@@ -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"]
|
||||
|
||||
+13
-9
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user