From 2033f4cc34dc861ed5144246f39b98de1589a031 Mon Sep 17 00:00:00 2001 From: fangliquanflq Date: Mon, 24 Aug 2026 02:06:59 +0800 Subject: [PATCH] fix(agent): separate cancellation diagnostics from tool output --- agent/background_review.py | 6 +- agent/interrupt_compat.py | 37 +++++++++-- agent/subagent_lifecycle.py | 4 +- run_agent.py | 32 ++++++++-- tests/agent/test_interrupt_compat.py | 32 ++++++++-- tests/agent/test_subagent_lifecycle.py | 8 ++- tests/run_agent/test_interrupt_propagation.py | 18 +++++- .../run_agent/test_sequential_tool_timeout.py | 61 ++++++++++++++----- 8 files changed, 162 insertions(+), 36 deletions(-) diff --git a/agent/background_review.py b/agent/background_review.py index f48a3d5200..f79a01959d 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -125,7 +125,11 @@ def _interrupt_background_review(review_agent: Any) -> None: try: from agent.interrupt_compat import request_hard_interrupt - request_hard_interrupt(review_agent, "superseded by a new live turn") + request_hard_interrupt( + review_agent, + "superseded by a new live turn", + tool_reason="background review superseded", + ) except Exception: logger.debug( "Failed to cancel in-flight background review for a new turn", diff --git a/agent/interrupt_compat.py b/agent/interrupt_compat.py index bf56849495..22f2784c79 100644 --- a/agent/interrupt_compat.py +++ b/agent/interrupt_compat.py @@ -6,12 +6,38 @@ import inspect from typing import Any -def request_hard_interrupt(agent: Any, message: str | None = None) -> bool: +def _accepts_keyword(callable_obj: Any, name: str) -> bool: + """Return whether a callable explicitly supports a keyword argument.""" + try: + parameters = inspect.signature(callable_obj).parameters.values() + except (TypeError, ValueError): + return False + return any( + parameter.kind is inspect.Parameter.VAR_KEYWORD + or ( + parameter.name == name + and parameter.kind is not inspect.Parameter.POSITIONAL_ONLY + ) + for parameter in parameters + ) + + +def request_hard_interrupt( + agent: Any, + message: str | None = None, + *, + tool_reason: str | None = None, +) -> bool: """Request an explicit stop, falling back to the legacy interrupt ABI. New agents expose ``hard_interrupt(message=None)``. Third-party agents and old test doubles may only expose ``interrupt(message=None)``; keep those - usable without sending the newer ``hard_cancel=`` keyword they do not know. + usable without sending newer keyword arguments they do not know. + + ``message`` is diagnostic/control-plane text. ``tool_reason`` is a trusted, + fixed category that may be exposed in model-visible tool cancellation + output. It is only forwarded when the modern callable explicitly supports + that channel. Returns ``False`` only when neither callable is available. """ # Avoid treating a dynamic ``__getattr__`` proxy (notably an unspecced @@ -28,8 +54,11 @@ def request_hard_interrupt(agent: Any, message: str | None = None) -> bool: interrupt = getattr(agent, "interrupt", None) if not callable(interrupt): return False + kwargs = {} + if tool_reason is not None and _accepts_keyword(interrupt, "tool_reason"): + kwargs["tool_reason"] = tool_reason if message is None: - interrupt() + interrupt(**kwargs) else: - interrupt(message) + interrupt(message, **kwargs) return True diff --git a/agent/subagent_lifecycle.py b/agent/subagent_lifecycle.py index 55e110aae5..319e85a784 100644 --- a/agent/subagent_lifecycle.py +++ b/agent/subagent_lifecycle.py @@ -307,7 +307,9 @@ class SubagentLifecycleService: ) try: accepted = request_hard_interrupt( - agent, f"Lifecycle cancellation requested: {reason[:500]}" + agent, + f"Lifecycle cancellation requested: {reason[:500]}", + tool_reason="subagent cancellation requested", ) except Exception: return SubagentCancelResult( diff --git a/run_agent.py b/run_agent.py index 8cc82617a5..e653b37086 100644 --- a/run_agent.py +++ b/run_agent.py @@ -3263,7 +3263,13 @@ class AIAgent: logging.warning(f"Failed to save session log: {e}") - def interrupt(self, message: Optional[str] = None, *, hard_cancel: bool = False) -> None: + def interrupt( + self, + message: Optional[str] = None, + *, + hard_cancel: bool = False, + tool_reason: Optional[str] = None, + ) -> None: """ Request the agent to interrupt its current tool-calling loop. @@ -3279,6 +3285,8 @@ class AIAgent: hard_cancel: Mark this as an explicit stop rather than a redirect or incoming-message interrupt. Compression may honor this atomic signal even while ordinary interrupts are masked. + tool_reason: Trusted fixed category safe to expose in tool output. + Arbitrary diagnostic or caller text belongs in message. Example (CLI): # In a separate input thread: @@ -3318,7 +3326,7 @@ class AIAgent: # ordinary interrupts may carry the user's full next message, which # must not be copied into tool output. tool_interrupt_reason = ( - (message or "explicit stop requested") + (tool_reason or "explicit stop requested") if hard_cancel else ("user sent a new message" if message else "user interrupt") ) @@ -3405,7 +3413,11 @@ class AIAgent: for child in children_copy: try: if hard_cancel: - request_hard_interrupt(child, message) + request_hard_interrupt( + child, + message, + tool_reason=tool_interrupt_reason, + ) else: child.interrupt(message) except Exception as e: @@ -3413,7 +3425,12 @@ class AIAgent: if not self.quiet_mode: print("\n⚡ Interrupt requested" + (f": '{message[:40]}...'" if message and len(message) > 40 else f": '{message}'" if message else "")) - def hard_interrupt(self, message: Optional[str] = None) -> None: + def hard_interrupt( + self, + message: Optional[str] = None, + *, + tool_reason: Optional[str] = None, + ) -> None: """Request an explicit stop while preserving ``interrupt()`` ABI. Frontends can feature-detect this method and fall back to the legacy @@ -3422,7 +3439,12 @@ class AIAgent: # Deliberately bypass dynamic dispatch: subclasses written against the # legacy interrupt(message=None) ABI may override interrupt without the # newer keyword-only hard_cancel argument. - AIAgent.interrupt(self, message, hard_cancel=True) + AIAgent.interrupt( + self, + message, + hard_cancel=True, + tool_reason=tool_reason, + ) def clear_interrupt(self, *, preserve_redirect: bool = False) -> bool: """Clear the interrupt request and per-thread tool signal. diff --git a/tests/agent/test_interrupt_compat.py b/tests/agent/test_interrupt_compat.py index 5b62f18e19..830c33a254 100644 --- a/tests/agent/test_interrupt_compat.py +++ b/tests/agent/test_interrupt_compat.py @@ -10,13 +10,18 @@ from agent.interrupt_compat import request_hard_interrupt class _ModernAgent: def __init__(self) -> None: - self.calls: list[tuple[str, str | None]] = [] + self.calls: list[tuple[str, str | None, str | None]] = [] - def hard_interrupt(self, message: str | None = None) -> None: - self.calls.append(("hard", message)) + def hard_interrupt( + self, + message: str | None = None, + *, + tool_reason: str | None = None, + ) -> None: + self.calls.append(("hard", message, tool_reason)) def interrupt(self, message: str | None = None) -> None: - self.calls.append(("soft", message)) + self.calls.append(("soft", message, None)) class _LegacyAgent: @@ -32,7 +37,22 @@ def test_explicit_producer_prefers_feature_detected_hard_interrupt() -> None: assert request_hard_interrupt(agent, "stop now") is True - assert agent.calls == [("hard", "stop now")] + assert agent.calls == [("hard", "stop now", None)] + + +def test_safe_tool_reason_only_reaches_supporting_modern_agent() -> None: + modern = _ModernAgent() + legacy = _LegacyAgent() + + assert request_hard_interrupt( + modern, "private diagnostic", tool_reason="fixed category" + ) + assert request_hard_interrupt( + legacy, "private diagnostic", tool_reason="fixed category" + ) + + assert modern.calls == [("hard", "private diagnostic", "fixed category")] + assert legacy.calls == [("legacy", "private diagnostic")] def test_explicit_producer_falls_back_to_old_interrupt_signature() -> None: @@ -100,4 +120,4 @@ def test_tui_subagent_interrupt_is_an_explicit_hard_stop() -> None: with delegate_tool._active_subagents_lock: delegate_tool._active_subagents.pop(subagent_id, None) - assert agent.calls == [("hard", f"Interrupted via TUI ({subagent_id})")] + assert agent.calls == [("hard", f"Interrupted via TUI ({subagent_id})", None)] diff --git a/tests/agent/test_subagent_lifecycle.py b/tests/agent/test_subagent_lifecycle.py index c32241fa8f..262e0517ba 100644 --- a/tests/agent/test_subagent_lifecycle.py +++ b/tests/agent/test_subagent_lifecycle.py @@ -25,14 +25,18 @@ class FakeChild: self.model = "test-model" self.interrupted = False self.interrupt_kind = None + self.interrupt_message = None + self.tool_reason = None def interrupt(self, _reason): self.interrupted = True self.interrupt_kind = "soft" - def hard_interrupt(self, _reason): + def hard_interrupt(self, reason, *, tool_reason=None): self.interrupted = True self.interrupt_kind = "hard" + self.interrupt_message = reason + self.tool_reason = tool_reason @pytest.fixture @@ -90,6 +94,8 @@ def test_cancel_uses_explicit_hard_interrupt(lifecycle): assert lifecycle.cancel(handle, reason="explicit user cancel").accepted assert record.agent.interrupt_kind == "hard" + assert "explicit user cancel" in record.agent.interrupt_message + assert record.agent.tool_reason == "subagent cancellation requested" lifecycle.wait(handle, timeout_seconds=1) diff --git a/tests/run_agent/test_interrupt_propagation.py b/tests/run_agent/test_interrupt_propagation.py index fccf28124d..ec5cc48615 100644 --- a/tests/run_agent/test_interrupt_propagation.py +++ b/tests/run_agent/test_interrupt_propagation.py @@ -84,13 +84,25 @@ class TestInterruptPropagationToChild(unittest.TestCase): assert get_interrupt_reason() == "user sent a new message" assert "private follow-up text" not in get_interrupt_reason() - def test_hard_interrupt_records_control_reason(self): + def test_hard_interrupt_does_not_expose_diagnostic_message(self): agent = self._make_bare_agent() agent._execution_thread_id = threading.current_thread().ident - agent.hard_interrupt("superseded by a new live turn") + agent.hard_interrupt("PRIVATE_CALLER_DETAIL") - assert get_interrupt_reason() == "superseded by a new live turn" + assert get_interrupt_reason() == "explicit stop requested" + assert "PRIVATE_CALLER_DETAIL" not in get_interrupt_reason() + + def test_hard_interrupt_records_explicit_safe_tool_reason(self): + agent = self._make_bare_agent() + agent._execution_thread_id = threading.current_thread().ident + + agent.hard_interrupt( + "PRIVATE_CALLER_DETAIL", + tool_reason="background review superseded", + ) + + assert get_interrupt_reason() == "background review superseded" def test_active_turn_redirect_does_not_set_hard_cancel(self): agent = self._make_bare_agent() diff --git a/tests/run_agent/test_sequential_tool_timeout.py b/tests/run_agent/test_sequential_tool_timeout.py index d517af8f11..df3b9fdfb3 100644 --- a/tests/run_agent/test_sequential_tool_timeout.py +++ b/tests/run_agent/test_sequential_tool_timeout.py @@ -215,8 +215,17 @@ def test_sequential_tool_timeout_suppresses_late_terminal_event(tmp_path, monkey ] -def test_sequential_tool_interrupt_reports_hard_cancel_reason(tmp_path, monkeypatch): +def test_sequential_tool_interrupt_hides_lifecycle_cancel_detail(tmp_path, monkeypatch): + from agent.subagent_lifecycle import ( + SubagentLaunchRequest, + SubagentLifecycleService, + SubagentState, + ) + agent = _make_agent(tmp_path) + agent._subagent_id = "sa-lifecycle-cancel-output" + agent._delegate_role = "leaf" + agent._delegate_depth = 1 first_started = threading.Event() release_first = threading.Event() terminal_events: list[dict] = [] @@ -226,14 +235,34 @@ def test_sequential_tool_interrupt_reports_hard_cancel_reason(tmp_path, monkeypa release_first.wait() return "late result" - def _interrupt(): - assert first_started.wait(timeout=5) - agent.hard_interrupt("superseded by a new live turn") - - interrupter = threading.Thread(target=_interrupt, daemon=True) - interrupter.start() messages: list[dict] = [] + + def _run_child(*_args, **_kwargs): + execute_tool_calls_sequential( + agent, + SimpleNamespace(tool_calls=[_tool_call("hung")]), + messages, + "task", + ) + return { + "status": "interrupted", + "summary": None, + "api_calls": 0, + "duration_seconds": 0, + } + monkeypatch.setenv("HERMES_CONCURRENT_TOOL_TIMEOUT_S", "30") + monkeypatch.setattr( + "tools.delegate_tool._build_child_preserving_parent_tools", + lambda **_kwargs: agent, + ) + monkeypatch.setattr("tools.delegate_tool._run_child_lifecycle", _run_child) + lifecycle = SubagentLifecycleService( + lambda: SimpleNamespace( + session_id="parent-lifecycle-cancel-output", + enabled_toolsets=["file"], + ) + ) try: with ( @@ -243,18 +272,20 @@ def test_sequential_tool_interrupt_reports_hard_cancel_reason(tmp_path, monkeypa side_effect=lambda *_args, **kwargs: terminal_events.append(kwargs), ), ): - execute_tool_calls_sequential( - agent, - SimpleNamespace(tool_calls=[_tool_call("hung")]), - messages, - "task", - ) + handle = lifecycle.launch(SubagentLaunchRequest(goal="cancel output test")) + assert first_started.wait(timeout=5) + assert lifecycle.cancel( + handle, + reason="PRIVATE_LIFECYCLE_REASON_DO_NOT_COPY", + ).accepted + assert lifecycle.wait(handle, timeout_seconds=5).state is SubagentState.CANCELLED finally: release_first.set() - interrupter.join(timeout=2) - assert "superseded by a new live turn" in messages[0]["content"] + assert "subagent cancellation requested" in messages[0]["content"] + assert "PRIVATE_LIFECYCLE_REASON_DO_NOT_COPY" not in messages[0]["content"] assert "user interrupt" not in messages[0]["content"] + assert "PRIVATE_LIFECYCLE_REASON_DO_NOT_COPY" not in terminal_events[0]["error_message"] assert terminal_events[0]["error_type"] == "tool_interrupted"