diff --git a/agent/replay_cleanup.py b/agent/replay_cleanup.py index 12de7a5c7e9..767b13f6605 100644 --- a/agent/replay_cleanup.py +++ b/agent/replay_cleanup.py @@ -138,3 +138,103 @@ def sanitize_replay_history( if not agent_history: return agent_history return strip_dangling_tool_call_tail(strip_interrupted_tool_tails(agent_history)) + + +# ────────────────────────────────────────────────────────────────────── +# Stale dangerous-confirmation text expiry (#59607) +# ────────────────────────────────────────────────────────────────────── + +# How long a high-risk confirmation phrase remains valid. +# Short on purpose: dangerous side effects should not survive any restart +# or session resumption gap. The user can always re-confirm if needed. +_DANGEROUS_CONFIRMATION_EXPIRY_SECONDS = 60.0 + +# Confirmation phrases that unlock destructive host actions. +# Substring match (case-insensitive) so that user variants (e.g. trailing +# punctuation, additional context) still match. Add new patterns here when +# new high-risk actions are introduced. +_DANGEROUS_CONFIRMATION_PATTERNS: tuple = ( + "confirm forced restart", + "confirm forced reboot", + "confirm shutdown", + "confirm reboot", + "confirm power off", + "yes, delete everything", + "confirm wipe", + "confirm factory reset", + # i18n variants observed in the original incident + "確認強制重開機", + "確認強制重開", + "確認重啟", +) + + +def is_dangerous_confirmation(content: Any) -> bool: + """Return True if a user-message text matches a known dangerous confirmation. + + Used by ``strip_stale_dangerous_confirmations`` to decide which + transcript rows to expire. Substring + case-insensitive so that + ``"Please confirm forced restart, the host is critical"`` still matches. + """ + if not isinstance(content, str): + return False + text = content.strip().lower() + return any(pattern in text for pattern in _DANGEROUS_CONFIRMATION_PATTERNS) + + +def strip_stale_dangerous_confirmations( + agent_history: List[Dict[str, Any]], + *, + now: float, + expiry_seconds: float = _DANGEROUS_CONFIRMATION_EXPIRY_SECONDS, +) -> List[Dict[str, Any]]: + """Strip stale dangerous-confirmation text in user messages (#59607). + + When a high-risk side effect (e.g. host restart via ``shutdown.exe``) + runs, the user's plain-text confirmation phrase is persisted in the + conversation transcript. If the host restart killed the gateway + process before the assistant's tool result was written, the + transcript tail ends on the assistant's text response — and the + dangerous confirmation text remains in the user role. + + On the next inbound message — possibly a casual "are you there?" from + the user minutes later — the LLM sees the stale confirmation and may + interpret the new turn as a fresh re-confirmation, re-executing the + destructive action. This is the failure mode reported in #59607. + + This function strips user messages whose content matches a known + dangerous confirmation pattern *and* whose timestamp is older than + ``expiry_seconds`` (default: 60s). Messages without a timestamp are + left untouched (backward compatibility: legacy transcripts and + in-memory test scaffolding have no timestamps). User messages that + contain dangerous confirmation text but are within the expiry window + are also left untouched — they represent a fresh confirmation that + has not yet been acted on. + + Complements 75ed07ace (which strips the *assistant* side of the + broken tail) by handling the *user* side: a stale plain-text + confirmation that the assistant has not yet responded to in a way + the resume logic recognises. + """ + if not agent_history: + return agent_history + + cleaned: List[Dict[str, Any]] = [] + for msg in agent_history: + if ( + isinstance(msg, dict) + and msg.get("role") == "user" + and is_dangerous_confirmation(msg.get("content", "")) + ): + ts = msg.get("timestamp") + if ts is not None and (now - float(ts)) > expiry_seconds: + logger.debug( + "Stripping stale dangerous-confirmation text from user " + "message (age=%.1fs, expiry=%.1fs): %r", + now - float(ts), + expiry_seconds, + (msg.get("content") or "")[:80], + ) + continue + cleaned.append(msg) + return cleaned diff --git a/gateway/run.py b/gateway/run.py index 0754a994c14..d47b11ebb77 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -722,7 +722,12 @@ _ASSISTANT_REPLAY_FIELDS: tuple[str, ...] = ( ) -def _build_replay_entry(role: str, content: Any, msg: Dict[str, Any]) -> Dict[str, Any]: +def _build_replay_entry( + role: str, + content: Any, + msg: Dict[str, Any], + preserve_timestamp: bool = False, +) -> Dict[str, Any]: """Build a replay entry for a non-tool-calling message, preserving the assistant fields the agent's API builders rely on for multi-turn fidelity. @@ -730,13 +735,20 @@ def _build_replay_entry(role: str, content: Any, msg: Dict[str, Any]) -> Dict[st be unit-tested in isolation. Mirrors the ``_ASSISTANT_REPLAY_FIELDS`` contract above. + ``preserve_timestamp``: when True, copy the source row's ``timestamp`` + onto the replay entry. Currently only user messages need this — the + stale-dangerous-confirmation stripper in ``agent/replay_cleanup.py`` + reads the timestamp to decide whether a confirmation is too old to + replay safely. Assistant/tool messages are not timestamp-stripped in + the same way, so we keep the existing default of dropping it. + Empty values: most fields are dropped when falsy (matching the original PR #2974 behaviour) since an empty list/string for those carries no information. The exception is ``reasoning_content``: DeepSeek/Kimi thinking-mode replay treats an empty string as a meaningful sentinel that ``_copy_reasoning_content_for_api`` upgrades to a single space. - Dropping it here would make the gateway send no ``reasoning_content`` at - all on the next turn, which can cause HTTP 400 from strict thinking + Dropping it here would make the gateway send no ``reasoning_content`` + at all on the next turn, which can cause HTTP 400 from strict thinking providers. """ entry: Dict[str, Any] = {"role": role, "content": content} @@ -752,6 +764,10 @@ def _build_replay_entry(role: str, content: Any, msg: Dict[str, Any]) -> Dict[st elif not _rval: continue entry[_rkey] = _rval + if preserve_timestamp: + ts = msg.get("timestamp") + if ts: + entry["timestamp"] = ts return entry @@ -867,7 +883,12 @@ def _build_gateway_agent_history( if msg.get("mirror"): mirror_src = msg.get("mirror_source", "another session") content = f"[Delivered from {mirror_src}] {content}" - entry = _build_replay_entry(role, content, msg) + # Preserve the timestamp on user messages so the + # stale-dangerous-confirmation stripper in agent/replay_cleanup.py + # can read it. The timestamp is dropped from assistant messages + # because they don't need it; the replay-tail strippers look at + # assistant(tool_calls), not timestamps. + entry = _build_replay_entry(role, content, msg, preserve_timestamp=(role == "user")) agent_history.append(entry) # Strip interrupted tool-call tails so the LLM doesn't re-execute @@ -881,6 +902,15 @@ def _build_gateway_agent_history( # on resume and loops the restart forever (#49201). agent_history = _strip_dangling_tool_call_tail(agent_history) + # Strip stale dangerous-confirmation text in user messages (#59607). + # A high-risk confirmation phrase (e.g. "confirm forced restart") that + # is older than the expiry window must not be replayed to the model, + # otherwise an unrelated follow-up message can be interpreted as a + # fresh confirmation and trigger the destructive action a second time. + agent_history = _strip_stale_dangerous_confirmations( + agent_history, now=time.time() + ) + observed_context = "\n".join(observed_group_context).strip() or None return agent_history, observed_context @@ -978,6 +1008,8 @@ from agent.replay_cleanup import ( # noqa: E402 is_interrupted_tool_result as _is_interrupted_tool_result, strip_interrupted_tool_tails as _strip_interrupted_tool_tails, strip_dangling_tool_call_tail as _strip_dangling_tool_call_tail, + strip_stale_dangerous_confirmations as _strip_stale_dangerous_confirmations, + is_dangerous_confirmation as _is_dangerous_confirmation, ) diff --git a/tests/gateway/test_stale_confirmation_expiry.py b/tests/gateway/test_stale_confirmation_expiry.py new file mode 100644 index 00000000000..431b6ba9117 --- /dev/null +++ b/tests/gateway/test_stale_confirmation_expiry.py @@ -0,0 +1,191 @@ +"""Tests for stale confirmation text expiry in gateway history. + +Reproduces the failure mode in #59607: when a host-restart-causing tool +runs, the user confirmation text remains in conversation history. On +resume, the LLM sees the confirmation at the tail and may re-execute the +destructive action if the user sends any follow-up. + +The fix: high-risk confirmation messages (user messages containing a +known dangerous confirmation pattern) are tagged with a creation +timestamp when they enter conversation history. On history replay, if +the confirmation is older than EXPIRY (60s by default), the confirmation +text is stripped from agent history before the model sees it. + +This complements 75ed07ace (which handles dangling assistant tool_calls +tail) by handling the user-side confirmation tail. +""" + +import time +import pytest +from typing import Dict, List + +from gateway.run import ( + _build_gateway_agent_history, + _is_dangerous_confirmation, + _strip_stale_dangerous_confirmations, +) + + +# High-risk confirmation patterns. A user message matching one of these +# (case-insensitive) is considered a "confirmation text" and is subject +# to the expiry rule. Add new patterns here as new high-risk side effects +# are introduced. +def test_dangerous_confirmation_helper(): + """The pattern matcher is case-insensitive and substring-based.""" + assert _is_dangerous_confirmation("confirm forced restart") + assert _is_dangerous_confirmation("CONFIRM FORCED RESTART") + assert _is_dangerous_confirmation(" confirm forced restart please ") + assert _is_dangerous_confirmation("I want to confirm forced restart the server") + + # i18n + assert _is_dangerous_confirmation("確認強制重開機") + + # Not a confirmation + assert not _is_dangerous_confirmation("can you restart the docker container?") + assert not _is_dangerous_confirmation("hello world") + assert not _is_dangerous_confirmation("") + assert not _is_dangerous_confirmation(None) + assert not _is_dangerous_confirmation(123) + + +def _make_history_with_confirmation( + *, + user_message_at: float, + assistant_warning_at: float, + confirmation_message: str, + confirmation_at: float, + assistant_action_at: float, +) -> List[Dict]: + """Build a synthetic conversation history with a confirmation text. + + Uses the real gateway's "timestamp" field (epoch seconds, as set in + gateway/run.py:11616, 11650, 11692, etc). + """ + return [ + {"role": "user", "content": "can you force a restart?", "timestamp": user_message_at}, + {"role": "assistant", "content": "Rebooting the host is dangerous. To confirm, please type: 'confirm forced restart'", "timestamp": assistant_warning_at}, + {"role": "user", "content": confirmation_message, "timestamp": confirmation_at}, + {"role": "assistant", "content": "OK, restarting now.", "timestamp": assistant_action_at}, + ] + + +def test_stale_confirmation_text_is_stripped_on_resume(): + """A confirmation text older than EXPIRY (60s by default) is stripped. + + This is the core fix for #59607. + """ + current_time = time.time() + # User confirmation was 5 minutes ago — well past the 60s expiry + user_message_at = current_time - 1000 + assistant_warning_at = current_time - 999 + confirmation_at = current_time - 300 # 5 minutes ago + assistant_action_at = current_time - 299 + + history = _make_history_with_confirmation( + user_message_at=user_message_at, + assistant_warning_at=assistant_warning_at, + confirmation_message="confirm forced restart", + confirmation_at=confirmation_at, + assistant_action_at=assistant_action_at, + ) + + agent_history, _ = _build_gateway_agent_history(history) + + # The stale confirmation text should be gone + confirmation_present = any( + m.get("role") == "user" and "confirm forced restart" in (m.get("content") or "") + for m in agent_history + ) + assert not confirmation_present, ( + f"Stale confirmation text should be stripped on resume. " + f"Got agent_history: {agent_history}" + ) + + +def test_fresh_confirmation_text_is_preserved(): + """A confirmation text within EXPIRY is kept (not yet expired).""" + current_time = time.time() + user_message_at = current_time - 30 + assistant_warning_at = current_time - 29 + confirmation_at = current_time - 5 # 5 seconds ago — fresh + assistant_action_at = current_time - 4 + + history = _make_history_with_confirmation( + user_message_at=user_message_at, + assistant_warning_at=assistant_warning_at, + confirmation_message="confirm forced restart", + confirmation_at=confirmation_at, + assistant_action_at=assistant_action_at, + ) + + agent_history, _ = _build_gateway_agent_history(history) + + # Fresh confirmation should still be there + confirmation_present = any( + m.get("role") == "user" and "confirm forced restart" in (m.get("content") or "") + for m in agent_history + ) + assert confirmation_present, ( + f"Fresh confirmation (5s old) should NOT be stripped. " + f"Got agent_history: {agent_history}" + ) + + +def test_non_confirmation_text_is_preserved(): + """A regular user message is never treated as a confirmation.""" + current_time = time.time() + user_message_at = current_time - 1000 # 17 min ago + confirmation_at = current_time - 300 # 5 min ago + + history = [ + {"role": "user", "content": "can you help me with the docs?", "timestamp": user_message_at}, + {"role": "assistant", "content": "Sure, what do you need?", "timestamp": confirmation_at}, + ] + + agent_history, _ = _build_gateway_agent_history(history) + + # Both messages should still be there + user_msgs = [m for m in agent_history if m.get("role") == "user"] + assert len(user_msgs) == 1 + assert "help me with the docs" in user_msgs[0].get("content", "") + + +def test_no_dangerous_pattern_at_all_preserves_everything(): + """If the conversation has no dangerous confirmation, nothing is stripped.""" + current_time = time.time() + user_message_at = current_time - 1000 + + history = [ + {"role": "user", "content": "tell me a joke", "timestamp": user_message_at}, + {"role": "assistant", "content": "Why did the chicken cross the road?", "timestamp": user_message_at + 1}, + {"role": "user", "content": "haha", "timestamp": user_message_at + 2}, + ] + + agent_history, _ = _build_gateway_agent_history(history) + + assert len(agent_history) == 3 + + +def test_strip_stale_dangerous_confirmations_directly(): + """Unit test the strip helper in isolation.""" + current_time = time.time() + history = _make_history_with_confirmation( + user_message_at=current_time - 1000, + assistant_warning_at=current_time - 999, + confirmation_message="confirm forced restart", + confirmation_at=current_time - 300, + assistant_action_at=current_time - 299, + ) + + cleaned = _strip_stale_dangerous_confirmations(history, now=current_time) + + # The dangerous confirmation should be gone + assert not any( + "confirm forced restart" in (m.get("content") or "") + for m in cleaned + if m.get("role") == "user" + ) + # The original user question and the assistant responses stay + assert any("can you force a restart" in (m.get("content") or "") for m in cleaned) + assert any("Rebooting the host is dangerous" in (m.get("content") or "") for m in cleaned) + assert any("OK, restarting now" in (m.get("content") or "") for m in cleaned) \ No newline at end of file