From 33a529538d1607ff1552e6eaf526cf95e3dfff49 Mon Sep 17 00:00:00 2001 From: knoal <114367649+knoal@users.noreply.github.com> Date: Mon, 6 Jul 2026 06:27:52 -0700 Subject: [PATCH] fix(gateway): strip stale dangerous-confirmation text in user messages (#59607) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Fix: - Add strip_stale_dangerous_confirmations() in agent/replay_cleanup.py that removes user messages whose content matches a known dangerous confirmation pattern AND whose timestamp is older than 60 seconds. - Add is_dangerous_confirmation() helper with the matched patterns (i18n-aware: covers 確認強制重開機 from the original incident). - Wire the stripper into _build_gateway_agent_history() right after the existing 75ed07ace strippers, so the strip chain is: strip_interrupted_tool_tails -> strip_dangling_tool_call_tail -> strip_stale_dangerous_confirmations. - Update _build_replay_entry() to preserve the timestamp on user messages (it was previously dropped), since the new stripper needs it. 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. Failing-test-first discipline: the bug-detection test test_stale_confirmation_text_is_stripped_on_resume fails on unfixed code (proves the test catches the bug) and passes after the fix. Five additional safety tests confirm no regression on: - fresh confirmations (within expiry) are preserved - non-confirmation text is preserved - non-matching histories are untouched - dangerous-pattern detection works in all cases (case, i18n, None) - direct unit test of the strip helper Refs: #59607 --- agent/replay_cleanup.py | 100 +++++++++ gateway/run.py | 40 +++- .../gateway/test_stale_confirmation_expiry.py | 191 ++++++++++++++++++ 3 files changed, 327 insertions(+), 4 deletions(-) create mode 100644 tests/gateway/test_stale_confirmation_expiry.py 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