mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
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.
This commit is contained in:
hellno
Teknium
parent
8235f484c9
commit
d7348bf24b
5 changed files with 346 additions and 2 deletions
259
tests/tools/test_approved_command_clean_slate.py
Normal file
259
tests/tools/test_approved_command_clean_slate.py
Normal file
|
|
@ -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"
|
||||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue