From d7348bf24b6157968cf069be1dd3146a655dab54 Mon Sep 17 00:00:00 2001 From: hellno Date: Fri, 19 Jun 2026 14:41:58 +0200 Subject: [PATCH] fix(interrupt): run user-approved commands from a clean interrupt slate A user-approved terminal/execute_code command could be SIGINT-killed (exit 130 + "[Command interrupted]") by a stale interrupt bit that landed on the execution thread during the blocking approval-wait, while the result still carried the "...approved by the user." note. The terminal tool runs sequentially inline on the execution thread, and nothing cleared or re-checked the bit between approval-grant and env.execute. Clear the current thread's interrupt bit once before an approved command spawns its child (terminal foreground; execute_code local + remote), and enrich the note to "...approved by the user, then interrupted." on a genuine post-start interrupt instead of implying success. A genuine interrupt arriving after execution starts (or during a retry backoff) still SIGINTs the command; non-approved commands keep current behavior. Adds regression tests covering stale-bit-clears, genuine-interrupt-still- kills, the retry-backoff window, natural-exit-130 (not mislabeled), and execute_code local + remote. --- .../test_approved_command_clean_slate.py | 259 ++++++++++++++++++ tests/tools/test_interrupt.py | 29 ++ tools/code_execution_tool.py | 10 + tools/interrupt.py | 15 + tools/terminal_tool.py | 35 ++- 5 files changed, 346 insertions(+), 2 deletions(-) create mode 100644 tests/tools/test_approved_command_clean_slate.py diff --git a/tests/tools/test_approved_command_clean_slate.py b/tests/tools/test_approved_command_clean_slate.py new file mode 100644 index 00000000000..81030771f8c --- /dev/null +++ b/tests/tools/test_approved_command_clean_slate.py @@ -0,0 +1,259 @@ +"""Regression tests: a user-approved command runs from a clean interrupt slate. + +Bug (manual approvals, the default): a user approves a scanner-flagged command, +then hits Stop / sends a message. `agent.interrupt()` sets the per-thread +interrupt bit on the execution thread *during* the blocking approval-wait; the +deny that follows is a no-op once the approval was granted, so the bit persists. +Nothing cleared it between approval-grant and `env.execute`, so +`_wait_for_process` SIGINT-killed the just-approved command on its first poll and +returned exit 130 + "[Command interrupted]" while still carrying the +"...approved by the user." note (the 3-part signature). + +Fix: clear the current thread's interrupt bit once before the approved command +spawns its child (terminal foreground; execute_code local + remote), and enrich +the note on a genuine post-start interrupt instead of implying success. + +Invariant preserved: a genuine interrupt arriving AFTER execution starts (or +during a retry backoff) must still SIGINT the command (exit 130); non-approved +commands keep current interrupt behavior. +""" +import json +import threading +import time + +import pytest + +from tools import terminal_tool as tt +from tools.interrupt import ( + set_interrupt, + is_interrupted, + clear_current_thread_interrupt, + _interrupted_threads, + _lock, +) + + +@pytest.fixture(autouse=True) +def _isolate(tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + (tmp_path / "logs").mkdir(exist_ok=True) + # Clean interrupt slate before and after every test so a stale tid left in + # the module-global set can't leak across tests in the same worker. + with _lock: + _interrupted_threads.clear() + yield + with _lock: + _interrupted_threads.clear() + + +def _wait_for_sentinel(sentinel, timeout=10.0): + """Block until the running command created its sentinel (proving the + clean-slate clear already ran and the command is in its poll loop).""" + deadline = time.monotonic() + timeout + while time.monotonic() < deadline: + if sentinel.exists(): + return True + time.sleep(0.02) + return sentinel.exists() + + +# --------------------------------------------------------------------------- +# terminal_tool +# --------------------------------------------------------------------------- + +def test_approved_command_clears_stale_interrupt_bit(): + """force=True marks the run user-approved -> the stale bit is cleared and + the command completes (exit 0), not killed with 130.""" + set_interrupt(True) # simulate a bit that landed during the approval-wait + assert is_interrupted() + + result = json.loads(tt.terminal_tool(command="sleep 0.5; echo DONE", force=True)) + + assert result["exit_code"] == 0, result + assert "DONE" in result["output"] + assert "[Command interrupted]" not in result["output"] + + +def test_non_approved_command_still_interrupts_on_stale_bit(monkeypatch): + """A command that is auto-approved but NOT user-approved keeps the current + interrupt behavior: a pre-existing bit still kills it (DO-NOT-BREAK).""" + monkeypatch.setattr(tt, "_check_all_guards", lambda *a, **k: {"approved": True}) + set_interrupt(True) + + result = json.loads(tt.terminal_tool(command="sleep 0.5; echo DONE")) + + assert result["exit_code"] == 130, result + assert "[Command interrupted]" in result["output"] + + +def test_approved_command_genuine_interrupt_after_start_still_kills(tmp_path): + """The clean-slate clear must NOT make approved commands un-interruptible: + an interrupt that arrives after execution starts still SIGINTs (130).""" + sentinel = tmp_path / "cmd_started_c" + holder = {} + + def worker(): + holder["result"] = tt.terminal_tool( + command=f"touch {sentinel}; sleep 5; echo DONE", force=True + ) + + t = threading.Thread(target=worker, daemon=True) + t.start() + # Barrier: the command is genuinely running (so the clear already ran) before + # we fire the interrupt -- no fixed-sleep timing guess. + assert _wait_for_sentinel(sentinel), "command did not start" + set_interrupt(True, thread_id=t.ident) # genuine interrupt, AFTER start + t.join(timeout=15) + assert not t.is_alive(), "worker did not exit after a genuine interrupt" + + result = json.loads(holder["result"]) + assert result["exit_code"] == 130, result + assert "[Command interrupted]" in result["output"] + set_interrupt(False, thread_id=t.ident) + + +def test_approved_note_enriched_not_misleading_on_interrupt(monkeypatch, tmp_path): + """On a genuine post-start interrupt of an approved command, the note must + read '...approved by the user, then interrupted.' — the bare + '...approved by the user.' must never co-occur with exit 130.""" + monkeypatch.setattr( + tt, + "_check_all_guards", + lambda *a, **k: {"approved": True, "user_approved": True, "description": "rm -rf x"}, + ) + sentinel = tmp_path / "cmd_started_d" + holder = {} + + def worker(): + holder["result"] = tt.terminal_tool(command=f"touch {sentinel}; sleep 5; echo DONE") + + t = threading.Thread(target=worker, daemon=True) + t.start() + assert _wait_for_sentinel(sentinel), "command did not start" + set_interrupt(True, thread_id=t.ident) + t.join(timeout=15) + assert not t.is_alive() + + result = json.loads(holder["result"]) + assert result["exit_code"] == 130, result + note = result.get("approval", "") + assert note.endswith("then interrupted."), note + assert "approved by the user, then interrupted." in note + assert "approved by the user." not in note # success-implying string is gone + set_interrupt(False, thread_id=t.ident) + + +def test_natural_exit_130_not_mislabeled_as_interrupt(monkeypatch): + """A command that legitimately exits 130 on its own (no interrupt) must NOT + get its approval note rewritten to '...then interrupted.'.""" + monkeypatch.setattr( + tt, + "_check_all_guards", + lambda *a, **k: {"approved": True, "user_approved": True, "description": "x"}, + ) + # Clean slate: no interrupt at all. + result = json.loads(tt.terminal_tool(command="bash -c 'exit 130'")) + + assert result["exit_code"] == 130, result + note = result.get("approval", "") + assert note == "Command required approval (x) and was approved by the user.", note + assert "then interrupted" not in note + assert "[Command interrupted]" not in result["output"] + + +def test_retry_backoff_does_not_clear_genuine_interrupt(monkeypatch): + """A genuine interrupt that lands during the retry backoff must survive + (the clear runs ONCE before the loop, never re-clearing on retries).""" + from tools.environments.local import LocalEnvironment + + calls = {"n": 0, "interrupted_at_retry": None} + + def fake_execute(self, command, **kw): + if "sleep 1" not in command: # ignore any incidental execute calls + return {"output": "", "returncode": 0} + calls["n"] += 1 + if calls["n"] == 1: + set_interrupt(True) # Stop lands during the first attempt / backoff + raise RuntimeError("transient backend error") + # Second attempt: the bit set during the backoff must NOT be re-cleared. + calls["interrupted_at_retry"] = is_interrupted() + return {"output": "partial\n[Command interrupted]", "returncode": 130} + + monkeypatch.setattr(LocalEnvironment, "execute", fake_execute) + monkeypatch.setattr("tools.terminal_tool.time.sleep", lambda *a, **k: None) + set_interrupt(False) + + result = json.loads(tt.terminal_tool(command="sleep 1", force=True, task_id="retry-test")) + + assert calls["n"] == 2, calls + assert calls["interrupted_at_retry"] is True, "retry must NOT re-clear a genuine interrupt" + assert result["exit_code"] == 130, result + + +# --------------------------------------------------------------------------- +# execute_code (same root cause, its own approval-wait + spawn/poll loop) +# --------------------------------------------------------------------------- + +def test_execute_code_approved_clears_stale_interrupt_bit(monkeypatch): + """An approved execute_code script (local path) runs from a clean slate.""" + from tools.code_execution_tool import execute_code + + monkeypatch.setattr( + "tools.approval.check_execute_code_guard", + lambda *a, **k: {"approved": True, "user_approved": True}, + ) + set_interrupt(True) + assert is_interrupted() + + result = json.loads(execute_code( + code='import time; time.sleep(0.5); print("CODE_DONE")', + task_id="test-clean-slate", + )) + + assert result["status"] == "success", result + assert "CODE_DONE" in result["output"] + assert "execution interrupted" not in result["output"] + + +def test_execute_code_non_approved_still_interrupts_on_stale_bit(monkeypatch): + """Non-user-approved execute_code keeps current interrupt behavior.""" + from tools.code_execution_tool import execute_code + + monkeypatch.setattr( + "tools.approval.check_execute_code_guard", + lambda *a, **k: {"approved": True}, # approved, but NOT user_approved + ) + set_interrupt(True) + + result = json.loads(execute_code( + code='import time; time.sleep(0.5); print("CODE_DONE")', + task_id="test-clean-slate-2", + )) + + # Killed on the first poll before the script can print. + assert "CODE_DONE" not in result["output"], result + + +def test_execute_code_remote_clears_stale_bit(monkeypatch): + """The clear sits above the local/remote split, so an approved remote (ssh) + script also dispatches from a clean slate.""" + from tools import code_execution_tool as cet + + monkeypatch.setattr( + "tools.approval.check_execute_code_guard", + lambda *a, **k: {"approved": True, "user_approved": True}, + ) + monkeypatch.setattr("tools.terminal_tool._get_env_config", lambda *a, **k: {"env_type": "ssh"}) + + captured = {} + + def fake_remote(code, task_id, enabled_tools): + captured["interrupted"] = is_interrupted() + return json.dumps({"status": "success", "output": ""}) + + monkeypatch.setattr(cet, "_execute_remote", fake_remote) + set_interrupt(True) # stale bit present before dispatch + + cet.execute_code(code="print(1)", task_id="remote-clean-slate") + + assert captured["interrupted"] is False, "clear must run before the remote dispatch" diff --git a/tests/tools/test_interrupt.py b/tests/tools/test_interrupt.py index 5d614f62bc5..8c71f10ffd9 100644 --- a/tests/tools/test_interrupt.py +++ b/tests/tools/test_interrupt.py @@ -55,6 +55,35 @@ class TestInterruptModule: set_interrupt(False, thread_id=t.ident) + def test_clear_current_thread_interrupt(self): + from tools.interrupt import ( + set_interrupt, is_interrupted, clear_current_thread_interrupt, + ) + set_interrupt(True) + assert is_interrupted() + clear_current_thread_interrupt() + assert not is_interrupted() + + def test_clear_current_thread_interrupt_leaves_other_threads(self): + """clear_current_thread_interrupt only touches the calling thread.""" + from tools.interrupt import ( + set_interrupt, is_interrupted, clear_current_thread_interrupt, + _interrupted_threads, _lock, + ) + with _lock: + _interrupted_threads.clear() + other_tid = threading.get_ident() + 1 # an ident that isn't us + set_interrupt(True, thread_id=other_tid) + set_interrupt(True) # current thread + assert is_interrupted() + + clear_current_thread_interrupt() + + assert not is_interrupted() # ours cleared + with _lock: + assert other_tid in _interrupted_threads # other thread untouched + _interrupted_threads.discard(other_tid) + # --------------------------------------------------------------------------- # Unit tests: pre-tool interrupt check diff --git a/tools/code_execution_tool.py b/tools/code_execution_tool.py index 707789b9f6b..54772d7d1a0 100644 --- a/tools/code_execution_tool.py +++ b/tools/code_execution_tool.py @@ -1166,6 +1166,16 @@ def execute_code( "duration_seconds": 0, }, ensure_ascii=False) + # Clean interrupt slate for a user-approved script before EITHER dispatch + # path spawns it: drop a stale bit that landed on this thread during the + # blocking approval-wait so it can't kill the just-approved run on the first + # poll (local _wait_for_process loop, or remote/ssh env.execute which routes + # through the same poll loop). A genuine post-clear interrupt re-sets the + # bit and is still caught downstream. + if _guard.get("user_approved"): + from tools.interrupt import clear_current_thread_interrupt + clear_current_thread_interrupt() + if env_type != "local": return _execute_remote(code, task_id, enabled_tools) diff --git a/tools/interrupt.py b/tools/interrupt.py index ac784332f91..da31dcfeb78 100644 --- a/tools/interrupt.py +++ b/tools/interrupt.py @@ -70,6 +70,21 @@ def is_interrupted() -> bool: return tid in _interrupted_threads +def clear_current_thread_interrupt() -> None: + """Clear any interrupt bit on the CURRENT thread. + + Gives a user-approved command a clean interrupt slate immediately before + it spawns its child process, so a stale bit that landed on this thread + during the blocking approval-wait cannot SIGINT the just-approved run + (exit 130 + "[Command interrupted]"). Single-thread ordering on this tid + keeps the DO-NOT-BREAK invariant intact: a *genuine* interrupt arriving + after this call re-sets the bit on the same thread and is still observed by + the executor's poll loop. Call this directly, never via the + _interrupt_event proxy (its .clear() binds to whatever thread runs it). + """ + set_interrupt(False) # thread_id=None -> current thread (see set_interrupt) + + # --------------------------------------------------------------------------- # Backward-compatible _interrupt_event proxy # --------------------------------------------------------------------------- diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index dc2a72f55d0..44ef03af788 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -2273,6 +2273,11 @@ def terminal_tool( # Pre-exec security checks (tirith + dangerous command detection) # Skip check if force=True (user has confirmed they want to run it) approval_note = None + # True when the user explicitly approved this run (or pre-confirmed via + # force). Drives the clean-interrupt-slate clear before env.execute so + # an approved command can't be SIGINT-killed by a bit that landed during + # the approval-wait (see clear_current_thread_interrupt). + _approved_run = bool(force) if not force: approval = _check_all_guards( command, env_type, @@ -2307,6 +2312,7 @@ def terminal_tool( if approval.get("user_approved"): desc = approval.get("description", "flagged as dangerous") approval_note = f"Command required approval ({desc}) and was approved by the user." + _approved_run = True elif approval.get("smart_approved"): desc = approval.get("description", "flagged as dangerous") approval_note = f"Command was flagged ({desc}) and auto-approved by smart approval." @@ -2388,6 +2394,9 @@ def terminal_tool( "exit_code": 0, "error": None, } + # Background spawns detached and returns exit_code 0 immediately; + # it never inline-polls is_interrupted(), so the stale-bit kill + # cannot occur here and this note never co-occurs with rc=130. if approval_note: result_data["approval"] = approval_note if pty_disabled_reason: @@ -2601,7 +2610,17 @@ def terminal_tool( retry_count = 0 result = None command_cwd = None - + + # Clean interrupt slate for an approved command, ONCE before the + # retry loop: drop a stale bit that landed on this thread during the + # approval-wait so it can't SIGINT the just-approved run. Do NOT + # re-clear inside the loop -- a genuine interrupt arriving during the + # backoff sleep between retries must survive and abort the command + # (caught by the next attempt's _wait_for_process poll loop -> 130). + if _approved_run: + from tools.interrupt import clear_current_thread_interrupt + clear_current_thread_interrupt() + while retry_count <= max_retries: try: command_cwd = _resolve_command_cwd( @@ -2743,7 +2762,19 @@ def terminal_tool( except Exception: logger.debug("verification evidence recording failed", exc_info=True) if approval_note: - result_dict["approval"] = approval_note + # Treat rc=130 as an interrupt only when the executor's marker is + # present. A command can legitimately exit 130 on its own + # (e.g. `bash -c 'exit 130'`); _wait_for_process returns the + # child's natural returncode there with no marker, and that must + # NOT be relabelled as a user interrupt in the audit note. + if returncode == 130 and "[Command interrupted]" in output: + # Approved command was interrupted mid-run by a genuine Stop. + # Keep the audit trail but never imply success: the bare + # "...approved by the user." note must not co-occur with the + # interrupt exit code (satisfies the 3-part-signature DONE). + result_dict["approval"] = approval_note.rstrip(".") + ", then interrupted." + else: + result_dict["approval"] = approval_note if exit_note: result_dict["exit_code_meaning"] = exit_note if sudo_auth_failed: