refactor(agent): fold simplify findings — DB picker parity, single scan, canonical strip delegation
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).
This commit is contained in:
kshitij
parent
4eabb595f0
commit
fecba5afcc
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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 = [
|
||||
|
|
|
|||
|
|
@ -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"]
|
||||
|
|
@ -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 ────────────────────────────────────────────────────
|
||||
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue