From e7a6d676c8c6b81ed132e0504f62f2dbf68129c9 Mon Sep 17 00:00:00 2001
From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>
Date: Tue, 7 Jul 2026 11:31:30 +0530
Subject: [PATCH] fix: redact expired confirmations in place to preserve role
 alternation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Deleting the matched user message breaks the strict role-alternation
invariant on the exact incident tail this fix targets — user(confirm) →
assistant('OK, restarting') becomes two consecutive assistant messages,
which strict providers reject and which the alternation-repair passes
upstream don't cover.  Replace the message content with an explicit
'confirmation EXPIRED, re-confirm before any destructive action'
sentinel instead: the trigger text is still neutralized, the model gets
an affirmative instruction not to act, and the message sequence stays
valid.  Adds an alternation-preservation regression test.

Follow-up to the salvage of #59640 by @knoal.
---
 agent/replay_cleanup.py                       | 37 ++++++++++++++-----
 .../gateway/test_stale_confirmation_expiry.py | 33 ++++++++++++++++-
 2 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/agent/replay_cleanup.py b/agent/replay_cleanup.py
index 767b13f6605..84815d756fa 100644
--- a/agent/replay_cleanup.py
+++ b/agent/replay_cleanup.py
@@ -168,6 +168,15 @@ _DANGEROUS_CONFIRMATION_PATTERNS: tuple = (
     "確認重啟",
 )
 
+# Replacement text for an expired confirmation. Redacting in place (rather
+# than deleting the message) preserves strict user/assistant role
+# alternation in the replayed history.
+_EXPIRED_CONFIRMATION_SENTINEL = (
+    "[A high-risk confirmation previously given here has EXPIRED and must "
+    "not be acted on. Ask the user to re-confirm explicitly before "
+    "performing any destructive action.]"
+)
+
 
 def is_dangerous_confirmation(content: Any) -> bool:
     """Return True if a user-message text matches a known dangerous confirmation.
@@ -188,7 +197,7 @@ def strip_stale_dangerous_confirmations(
     now: float,
     expiry_seconds: float = _DANGEROUS_CONFIRMATION_EXPIRY_SECONDS,
 ) -> List[Dict[str, Any]]:
-    """Strip stale dangerous-confirmation text in user messages (#59607).
+    """Expire 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
@@ -202,14 +211,19 @@ def strip_stale_dangerous_confirmations(
     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.
+    Expired confirmations are REDACTED IN PLACE, not removed: deleting a
+    user message from the incident tail (``user(confirm) →
+    assistant("OK, restarting")``) would leave two consecutive assistant
+    messages, violating the strict role-alternation invariant providers
+    enforce.  The message survives with its role intact; only the trigger
+    text is replaced by a sentinel that tells the model the confirmation
+    has expired.
+
+    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
@@ -229,12 +243,15 @@ def strip_stale_dangerous_confirmations(
             ts = msg.get("timestamp")
             if ts is not None and (now - float(ts)) > expiry_seconds:
                 logger.debug(
-                    "Stripping stale dangerous-confirmation text from user "
+                    "Redacting stale dangerous-confirmation text in user "
                     "message (age=%.1fs, expiry=%.1fs): %r",
                     now - float(ts),
                     expiry_seconds,
                     (msg.get("content") or "")[:80],
                 )
+                redacted = dict(msg)
+                redacted["content"] = _EXPIRED_CONFIRMATION_SENTINEL
+                cleaned.append(redacted)
                 continue
         cleaned.append(msg)
     return cleaned
diff --git a/tests/gateway/test_stale_confirmation_expiry.py b/tests/gateway/test_stale_confirmation_expiry.py
index 431b6ba9117..143d42d74da 100644
--- a/tests/gateway/test_stale_confirmation_expiry.py
+++ b/tests/gateway/test_stale_confirmation_expiry.py
@@ -188,4 +188,35 @@ def test_strip_stale_dangerous_confirmations_directly():
     # 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
+    assert any("OK, restarting now" in (m.get("content") or "") for m in cleaned)
+
+def test_redaction_preserves_role_alternation():
+    """Expiry must redact in place, never delete the user message.
+
+    The incident tail is ``user(confirm) → assistant("OK, restarting")``.
+    Deleting the user row would leave two consecutive assistant messages,
+    violating the strict role-alternation invariant providers enforce.
+    """
+    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)
+
+    # Same message count — nothing deleted.
+    assert len(cleaned) == len(history)
+    # The user slot survives with an expiry sentinel instead of the phrase.
+    redacted = cleaned[2]
+    assert redacted["role"] == "user"
+    assert "confirm forced restart" not in redacted["content"]
+    assert "EXPIRED" in redacted["content"]
+    # No two consecutive same-role messages anywhere.
+    roles = [m["role"] for m in cleaned]
+    assert all(a != b for a, b in zip(roles, roles[1:])), roles
+    # Original history object is not mutated.
+    assert history[2]["content"] == "confirm forced restart"