From fecba5afcc58e0bc10c623e2982bdfba63afc25b Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Fri, 7 Aug 2026 19:21:14 +0530 Subject: [PATCH] =?UTF-8?q?refactor(agent):=20fold=20simplify=20findings?= =?UTF-8?q?=20=E2=80=94=20DB=20picker=20parity,=20single=20scan,=20canonic?= =?UTF-8?q?al=20strip=20delegation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-pass follow-ups (three parallel reviewers, findings verified): - hermes_state_search.py list_recent_user_messages now drops legacy standalone compaction handoffs in the decode loop (SQL can't see them: durable role=user, no display_kind). Closes the /undo N pairing skew where the in-memory count (new predicate) and the DB soft-delete pick (old predicate) targeted different turns on legacy sessions. Fetches with headroom so the requested limit is still honored. 3 new tests, mutation-checked (no-op'ing the skip fails 2/3). - _should_skip_model_call_for_reference_handoff: single drive-check scan (was two — once inside the restore helper, once after); the restore helper no longer re-scans and its return value now decides the verdict. - _final_response_from_messages replaced by the _HANDOFF_SKIP_FINAL_RESPONSE constant it always returned (parameter was unused). - _handoff_carries_live_user_content delegates to the canonical _strip_context_summary_handoff_message — also fixes the edge where a merged-shaped row with an EMPTY preserved prior tail was wrongly treated as carrying live content. - Site-level guard test for rollback.restore with a legacy handoff row (predicate-in-context, complements the unit tests). --- agent/context_compressor.py | 22 +++--- agent/conversation_loop.py | 50 ++++++------ hermes_state_search.py | 16 +++- ...test_list_recent_user_messages_handoffs.py | 79 +++++++++++++++++++ tests/test_tui_gateway_server.py | 60 ++++++++++++++ 5 files changed, 190 insertions(+), 37 deletions(-) create mode 100644 tests/test_list_recent_user_messages_handoffs.py diff --git a/agent/context_compressor.py b/agent/context_compressor.py index 80b3a424f2dab..e9471aaa43355 100644 --- a/agent/context_compressor.py +++ b/agent/context_compressor.py @@ -7025,18 +7025,22 @@ def _handoff_carries_live_user_content(message: Any) -> bool: Force-user-leading merges prepend the handoff + end marker to the real ask, leaving a non-empty remainder after ``_SUMMARY_END_MARKER``. Either shape must remain actionable (#80622 must not treat them as sole-handoff). + + Delegates to ``_strip_context_summary_handoff_message`` — the canonical + "does anything survive once the handoff is removed" logic (it also + handles multimodal list content and returns ``None`` for a merged-shaped + row whose preserved prior tail is EMPTY, which a bare + ``classify_summary_content(...) == "merged"`` check would wrongly treat + as live). Callers must pre-filter with ``is_compaction_summary_message``: + for non-summary rows the strip helper returns the message unchanged, + which would read as "carries live content" here. """ if not isinstance(message, dict): return False - content = message.get("content") - kind = ContextCompressor.classify_summary_content(content) - if kind == "merged": - return True - text = _content_text_for_contains(content) - marker_idx = text.find(_SUMMARY_END_MARKER) - if marker_idx < 0: - return False - return bool(text[marker_idx + len(_SUMMARY_END_MARKER) :].strip()) + return ( + ContextCompressor._strip_context_summary_handoff_message(message) + is not None + ) def reference_handoff_would_drive_next_model_call( diff --git a/agent/conversation_loop.py b/agent/conversation_loop.py index 3a448ed3a2b54..0437d3b149fc2 100644 --- a/agent/conversation_loop.py +++ b/agent/conversation_loop.py @@ -100,14 +100,10 @@ def _restore_user_after_reference_handoff( ) -> bool: """Re-append this turn's real user ask when compaction left only a handoff. - Returns True when a restore append happened. Used before deciding whether - a post-compaction ``continue`` would let the reference-only summary drive - the next model call (#80622). + Returns True when a restore append happened. The caller has already + established that a reference-only handoff would drive the next model + call (#80622); this helper only decides whether a restorable ask exists. """ - from agent.context_compressor import reference_handoff_would_drive_next_model_call - - if not reference_handoff_would_drive_next_model_call(messages): - return False if user_message is None: return False if isinstance(user_message, str): @@ -137,23 +133,25 @@ def _should_skip_model_call_for_reference_handoff( """Guard post-compaction continues against sole-handoff active turns (#80622).""" from agent.context_compressor import reference_handoff_would_drive_next_model_call - _restore_user_after_reference_handoff(messages, user_message) - return reference_handoff_would_drive_next_model_call(messages) + if not reference_handoff_would_drive_next_model_call(messages): + return False + if _restore_user_after_reference_handoff(messages, user_message): + # The restored ask is an actionable non-synthetic user row appended + # after the handoff — by construction the handoff no longer drives. + return False + return True -def _final_response_from_messages(messages: List[Dict[str, Any]]) -> str: - """Fallback text for a turn ended by the sole-handoff skip (#80622). - - Deliberately NOT a replay of the last assistant text: finalize_turn's - non-assistant-tail chokepoint (#43849) appends ``final_response`` as a - fresh assistant row, so recovering the previous turn's prose here would - duplicate it in the durable transcript AND re-deliver it to the user as - if it were this turn's answer. A short status is honest and idempotent. - """ - return ( - "Context was compacted. The previous response is complete — " - "awaiting your next message." - ) +# Fallback final_response for a turn ended by the sole-handoff skip (#80622). +# Deliberately NOT a replay of the last assistant text: finalize_turn's +# non-assistant-tail chokepoint (#43849) appends final_response as a fresh +# assistant row, so recovering the previous turn's prose here would duplicate +# it in the durable transcript AND re-deliver it to the user as if it were +# this turn's answer. A short status is honest and idempotent. +_HANDOFF_SKIP_FINAL_RESPONSE = ( + "Context was compacted. The previous response is complete — " + "awaiting your next message." +) # Stable prefix of the local interrupt status string emitted when a turn is @@ -2234,7 +2232,7 @@ def run_conversation( "handoff would be the sole active user turn (#80622)" ) if not final_response: - final_response = _final_response_from_messages(messages) + final_response = _HANDOFF_SKIP_FINAL_RESPONSE _turn_exit_reason = "compaction_handoff_not_actionable" break continue @@ -5815,7 +5813,7 @@ def run_conversation( "handoff would be the sole active user turn (#80622)" ) if not final_response: - final_response = _final_response_from_messages(messages) + final_response = _HANDOFF_SKIP_FINAL_RESPONSE _turn_exit_reason = "compaction_handoff_not_actionable" break # In-loop compression rebuilt `messages` with fresh compaction @@ -6681,9 +6679,7 @@ def run_conversation( "active user turn (#80622)" ) if not final_response: - final_response = _final_response_from_messages( - messages - ) + final_response = _HANDOFF_SKIP_FINAL_RESPONSE _turn_exit_reason = "compaction_handoff_not_actionable" break elif agent.compression_enabled: diff --git a/hermes_state_search.py b/hermes_state_search.py index 32eae0ae77ae9..e8f32aa5f41c1 100644 --- a/hermes_state_search.py +++ b/hermes_state_search.py @@ -1103,19 +1103,33 @@ class SessionSearchMixin: active_clause = "" if include_inactive else " AND active = 1" # Match CLI/desktop: only real user turns, not timeline bookkeeping. display_clause = " AND (display_kind IS NULL OR display_kind = '')" + # Legacy standalone compaction handoffs (persisted pre-#80622) are + # durable role='user' rows with NO display_kind — SQL can't see them, + # so fetch with headroom and drop them in the decode loop below. + # Without this, /undo N and rewind pair an in-memory count that + # excludes handoffs with a DB pick that includes them, soft-deleting + # the wrong turn. + fetch_limit = int(limit) * 2 + 5 with self._lock: cursor = self._conn.execute( "SELECT id, timestamp, content FROM messages " "WHERE session_id = ? AND role = 'user'" f"{active_clause}{display_clause} " "ORDER BY id DESC LIMIT ?", - (session_id, int(limit)), + (session_id, fetch_limit), ) rows = cursor.fetchall() + from agent.context_compressor import ContextCompressor + result: List[Dict[str, Any]] = [] for row in rows: + if len(result) >= int(limit): + break decoded = self._decode_content(row["content"]) + if ContextCompressor._is_context_summary_content(decoded): + # Compaction handoff — never a user-originated turn (#80622). + continue if isinstance(decoded, list): # Multimodal — flatten text parts. text_parts = [ diff --git a/tests/test_list_recent_user_messages_handoffs.py b/tests/test_list_recent_user_messages_handoffs.py new file mode 100644 index 0000000000000..6a6c9563bc781 --- /dev/null +++ b/tests/test_list_recent_user_messages_handoffs.py @@ -0,0 +1,79 @@ +"""list_recent_user_messages must skip legacy compaction handoffs (#80622). + +Legacy standalone ``[CONTEXT COMPACTION — REFERENCE ONLY]`` handoffs persisted +pre-#80622 are durable ``role='user'`` rows with NO ``display_kind``, so the +SQL-side display filter cannot exclude them. Every /undo-class command pairs an +in-memory count that (post-#80622) excludes handoffs via +``is_user_originated_turn`` with this DB picker — if the picker still counted +handoffs, the on-disk soft-delete would target a different turn than the +in-memory cut (memory/disk transcript divergence). + +Drives the real SQL + decode path through SessionDB. +""" + +import pytest + +from agent.context_compressor import ( + HISTORICAL_TASK_HEADING, + SUMMARY_PREFIX, + _SUMMARY_END_MARKER, +) +from hermes_state import SessionDB + +HANDOFF_CONTENT = ( + f"{SUMMARY_PREFIX}\n{HISTORICAL_TASK_HEADING}\n" + f"User asked: 'old task'\n\n{_SUMMARY_END_MARKER}" +) + + +@pytest.fixture() +def db(tmp_path): + session_db = SessionDB(db_path=tmp_path / "state.db") + yield session_db + session_db.close() + + +def test_legacy_handoff_rows_are_not_recent_user_messages(db): + db.create_session(session_id="s1", source="cli", model="m") + db.append_message("s1", role="user", content="first question") + db.append_message("s1", role="assistant", content="first answer") + # Legacy shape: durable role=user handoff with NO display_kind. + db.append_message("s1", role="user", content=HANDOFF_CONTENT) + db.append_message("s1", role="user", content="second question") + db.append_message("s1", role="assistant", content="second answer") + + recents = db.list_recent_user_messages("s1", limit=10) + previews = [r["preview"] for r in recents] + + assert len(recents) == 2 + assert previews[0].startswith("second question") + assert previews[1].startswith("first question") + assert not any("[CONTEXT COMPACTION" in p for p in previews) + + +def test_handoff_skip_respects_limit_with_headroom(db): + """The requested limit is still honored when handoff rows are dropped.""" + db.create_session(session_id="s2", source="cli", model="m") + for i in range(3): + db.append_message("s2", role="user", content=HANDOFF_CONTENT) + db.append_message("s2", role="user", content=f"question {i}") + + recents = db.list_recent_user_messages("s2", limit=2) + + assert [r["preview"] for r in recents] == ["question 2", "question 1"] + + +def test_display_kind_rows_still_excluded(db): + """The pre-existing SQL-side display_kind filter is unchanged.""" + db.create_session(session_id="s3", source="cli", model="m") + db.append_message("s3", role="user", content="real question") + db.append_message( + "s3", + role="user", + content="background agent finished", + display_kind="async_delegation_complete", + ) + + recents = db.list_recent_user_messages("s3", limit=10) + + assert [r["preview"] for r in recents] == ["real question"] diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 1b75d65f2fd14..c8e6679aa509d 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -8653,6 +8653,66 @@ def test_rollback_restore_truncates_from_real_user_turn_not_marker(monkeypatch): server._sessions.pop("sid", None) +def test_rollback_restore_skips_legacy_compaction_handoff(monkeypatch): + """rollback.restore must not truncate from a legacy standalone compaction + handoff — a durable role=user row persisted pre-#80622 with NO + display_kind. Same bug class as the display_kind marker above, caught + only by the is_user_originated_turn predicate. + """ + from agent.context_compressor import ( + COMPRESSED_SUMMARY_METADATA_KEY, + HISTORICAL_TASK_HEADING, + SUMMARY_PREFIX, + _SUMMARY_END_MARKER, + ) + + class _Mgr: + enabled = True + + def list_checkpoints(self, cwd): + return [{"hash": "abc123"}] + + def restore(self, cwd, target, file_path=None): + return {"success": True, "message": "restored"} + + handoff = { + "role": "user", + "content": ( + f"{SUMMARY_PREFIX}\n{HISTORICAL_TASK_HEADING}\n" + f"User asked: 'old task'\n\n{_SUMMARY_END_MARKER}" + ), + COMPRESSED_SUMMARY_METADATA_KEY: True, + # NOTE: no display_kind — the legacy-persistence shape (#80622). + } + history = [ + {"role": "user", "content": "first question"}, + {"role": "assistant", "content": "first answer"}, + {"role": "user", "content": "second question"}, + {"role": "assistant", "content": "second answer"}, + handoff, + ] + server._sessions["sid"] = _session( + agent=types.SimpleNamespace(_checkpoint_mgr=_Mgr()), + history=list(history), + ) + try: + resp = server.handle_request( + { + "id": "1", + "method": "rollback.restore", + "params": {"session_id": "sid", "hash": "abc123"}, + } + ) + + assert resp["result"]["success"] is True + # Truncation lands on "second question", not the handoff row. + assert resp["result"]["history_removed"] == 3 # q2 + a2 + handoff + remaining = server._sessions["sid"]["history"] + assert [m["content"] for m in remaining] == ["first question", "first answer"] + finally: + server._sessions.pop("sid", None) + + # ── session.steer ────────────────────────────────────────────────────