mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-10 13:31:38 +00:00
fix(compressor): prevent orphan user turn after compaction via turn-pair preservation
When the last user message sits exactly at head_end (the first compressible index), _ensure_last_user_message_in_tail's final max(last_user_idx, head_end + 1) clamp returns head_end + 1, pushing the user into the compressed region without its assistant reply. The summariser then records it as a pending ask, and the next session re-executes the already-completed task (lights off twice, file deleted twice, message re-sent). Fix: apply Causal Coupling — a compaction boundary must never split a (user -> assistant [-> tool results]) turn-pair. Add _find_turn_pair_end and, when the clamp would orphan the user, push the cut forward to pair_end so the completed pair is summarised together and marked done. 8 new tests in TestTurnPairPreservation; 133 compressor tests pass.
This commit is contained in:
parent
e71f9ad0bb
commit
fc2fac73bd
2 changed files with 192 additions and 1 deletions
|
|
@ -2350,6 +2350,17 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
(``messages[cut_idx:]``), walk ``cut_idx`` back to include it. We
|
||||
then re-align backward one more time to avoid splitting any
|
||||
tool_call/result group that immediately precedes the user message.
|
||||
|
||||
Causal Coupling guard (#22523): the final ``max(last_user_idx,
|
||||
head_end + 1)`` clamp can push the cut *past* the user message when
|
||||
the user sits at ``head_end`` (the first compressible index) — the
|
||||
only case where ``head_end + 1 > last_user_idx``. That splits the
|
||||
turn-pair: the user lands in the compressed region without its
|
||||
assistant reply, so the summariser records it as a pending ask and
|
||||
the next session re-executes the already-completed task. When this
|
||||
split is unavoidable, push the cut *forward* to ``pair_end`` so the
|
||||
full pair (user + reply + tool results) is summarised together and
|
||||
correctly marked as completed.
|
||||
"""
|
||||
last_user_idx = self._find_last_user_message_idx(messages, head_end)
|
||||
if last_user_idx < 0:
|
||||
|
|
@ -2374,7 +2385,50 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
cut_idx,
|
||||
)
|
||||
# Safety: never go back into the head region.
|
||||
return max(last_user_idx, head_end + 1)
|
||||
adjusted = max(last_user_idx, head_end + 1)
|
||||
if adjusted > last_user_idx:
|
||||
# The clamp would leave the user in the compressed region without
|
||||
# its reply. Keep the pair intact by pushing the cut forward past
|
||||
# the whole (user + assistant + tool results) turn-pair so it is
|
||||
# summarised as a completed unit rather than a dangling ask.
|
||||
pair_end = self._find_turn_pair_end(messages, last_user_idx)
|
||||
if not self.quiet_mode:
|
||||
logger.debug(
|
||||
"Causal Coupling: cut would split turn-pair at user %d; "
|
||||
"pushing cut forward to pair_end %d so the completed pair "
|
||||
"is summarised together (#22523)",
|
||||
last_user_idx,
|
||||
pair_end,
|
||||
)
|
||||
return max(pair_end, head_end + 1)
|
||||
return adjusted
|
||||
|
||||
def _find_turn_pair_end(
|
||||
self,
|
||||
messages: List[Dict[str, Any]],
|
||||
user_idx: int,
|
||||
) -> int:
|
||||
"""Return the index *after* the complete turn-pair starting at *user_idx*.
|
||||
|
||||
A turn-pair is: ``user`` -> ``assistant`` [-> zero-or-more ``tool``
|
||||
results]. Returns the index of the first message that does *not*
|
||||
belong to the pair, i.e. the natural cut point that keeps the pair
|
||||
intact on one side of the boundary.
|
||||
|
||||
If *user_idx* is the last message (no assistant reply yet), returns
|
||||
``user_idx + 1`` so the user message itself is minimally covered.
|
||||
"""
|
||||
n = len(messages)
|
||||
idx = user_idx + 1
|
||||
if idx >= n:
|
||||
return idx # user is the very last message — no reply yet
|
||||
if messages[idx].get("role") != "assistant":
|
||||
return idx # no assistant reply immediately following
|
||||
idx += 1
|
||||
# Include any tool results that belong to this assistant turn.
|
||||
while idx < n and messages[idx].get("role") == "tool":
|
||||
idx += 1
|
||||
return idx
|
||||
|
||||
def _find_tail_cut_by_tokens(
|
||||
self, messages: List[Dict[str, Any]], head_end: int,
|
||||
|
|
|
|||
|
|
@ -2782,3 +2782,140 @@ class TestPreflightSentinelGuard:
|
|||
compressor.last_prompt_tokens = 50_000
|
||||
result = self._seed(compressor.last_prompt_tokens, 10_000)
|
||||
assert result == 50_000
|
||||
|
||||
|
||||
class TestTurnPairPreservation:
|
||||
"""Causal Coupling guard (#22523): compaction must never orphan a user turn.
|
||||
|
||||
``_ensure_last_user_message_in_tail`` pulls the cut back to keep the last
|
||||
user message in the tail (fixes #10896). But its final
|
||||
``max(last_user_idx, head_end + 1)`` clamp pushes the cut *past* the user
|
||||
when the user sits at ``head_end`` (the first compressible index) — the
|
||||
only case where ``head_end + 1 > last_user_idx``. The user then lands in
|
||||
the compressed region without its assistant reply; the summariser marks it
|
||||
as a pending ask and the next session re-executes the completed task.
|
||||
|
||||
The guard detects that split and pushes the cut forward to ``pair_end`` so
|
||||
the complete (user -> assistant [-> tool results]) pair is summarised as a
|
||||
finished unit.
|
||||
"""
|
||||
|
||||
@pytest.fixture
|
||||
def compressor(self):
|
||||
return ContextCompressor(
|
||||
model="test/model",
|
||||
threshold_percent=0.85,
|
||||
protect_first_n=1,
|
||||
protect_last_n=0,
|
||||
quiet_mode=True,
|
||||
)
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# _find_turn_pair_end unit tests
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
def test_pair_end_user_only(self, compressor):
|
||||
"""User at end of list — no reply yet — pair_end is user+1."""
|
||||
msgs = [{"role": "user", "content": "hello"}]
|
||||
assert compressor._find_turn_pair_end(msgs, 0) == 1
|
||||
|
||||
def test_pair_end_user_with_assistant_reply(self, compressor):
|
||||
"""User + assistant — pair_end skips both."""
|
||||
msgs = [
|
||||
{"role": "user", "content": "do x"},
|
||||
{"role": "assistant", "content": "done"},
|
||||
]
|
||||
assert compressor._find_turn_pair_end(msgs, 0) == 2
|
||||
|
||||
def test_pair_end_user_assistant_with_tools(self, compressor):
|
||||
"""User + assistant + tool results — pair_end skips the whole group."""
|
||||
msgs = [
|
||||
{"role": "user", "content": "run it"},
|
||||
{"role": "assistant", "content": None,
|
||||
"tool_calls": [{"function": {"name": "exec", "arguments": "{}"}}]},
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "ok"},
|
||||
{"role": "tool", "tool_call_id": "c2", "content": "ok"},
|
||||
]
|
||||
assert compressor._find_turn_pair_end(msgs, 0) == 4
|
||||
|
||||
def test_pair_end_stops_at_next_user(self, compressor):
|
||||
"""pair_end must not cross into the next user turn."""
|
||||
msgs = [
|
||||
{"role": "user", "content": "first"},
|
||||
{"role": "assistant", "content": "reply"},
|
||||
{"role": "user", "content": "second"},
|
||||
]
|
||||
assert compressor._find_turn_pair_end(msgs, 0) == 2
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
# _ensure_last_user_message_in_tail unit tests
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
def test_user_already_in_tail_unchanged(self, compressor):
|
||||
"""When the user message is already past cut_idx, nothing changes."""
|
||||
msgs = [
|
||||
{"role": "user", "content": "head"},
|
||||
{"role": "assistant", "content": "head reply"},
|
||||
{"role": "user", "content": "last user"},
|
||||
{"role": "assistant", "content": "last reply"},
|
||||
]
|
||||
result = compressor._ensure_last_user_message_in_tail(msgs, cut_idx=2, head_end=1)
|
||||
assert result == 2
|
||||
|
||||
def test_user_in_compressed_region_pulled_back(self, compressor):
|
||||
"""User in the middle (not at head_end) is pulled into the tail (#10896)."""
|
||||
msgs = [
|
||||
{"role": "user", "content": "head"}, # 0
|
||||
{"role": "assistant", "content": "hi"}, # 1
|
||||
{"role": "user", "content": "do thing"}, # 2 <- last user
|
||||
{"role": "assistant", "content": "done"}, # 3
|
||||
]
|
||||
# head_end=0, so head_end+1=1 <= last_user_idx=2: the #10896 pullback
|
||||
# applies and the user stays in the tail (no forward push).
|
||||
result = compressor._ensure_last_user_message_in_tail(msgs, cut_idx=3, head_end=0)
|
||||
assert result <= 2
|
||||
|
||||
def test_orphan_prevented_user_at_head_end(self, compressor):
|
||||
"""Causal Coupling: user at head_end pushes the WHOLE pair into the summary.
|
||||
|
||||
This is the #22523 case: last_user_idx == head_end, so the clamp would
|
||||
return head_end+1 and orphan the user. The guard instead pushes the
|
||||
cut forward to pair_end so user + reply + tool results are summarised
|
||||
together and the tail never starts with a dangling user ask.
|
||||
"""
|
||||
msgs = [
|
||||
{"role": "user", "content": "first exchange"}, # 0 head
|
||||
{"role": "user", "content": "THE ACTIVE ASK"}, # 1 = head_end, last user
|
||||
{"role": "assistant", "content": "done"}, # 2 reply
|
||||
{"role": "tool", "tool_call_id": "c1", "content": "toolout"}, # 3
|
||||
{"role": "assistant", "content": "final reply"}, # 4
|
||||
]
|
||||
head_end = 1
|
||||
result = compressor._ensure_last_user_message_in_tail(msgs, cut_idx=3, head_end=head_end)
|
||||
# Whole pair (indices 1..3) lands in the compressed region; tail starts at 4.
|
||||
assert result == 4
|
||||
tail = msgs[result:]
|
||||
assert tail and tail[0]["role"] == "assistant"
|
||||
|
||||
def test_no_orphan_after_full_compaction_cycle(self, compressor):
|
||||
"""End-to-end: after _find_tail_cut_by_tokens, the tail never starts
|
||||
with an unanswered user message."""
|
||||
msgs = [
|
||||
{"role": "user", "content": "initial"},
|
||||
{"role": "assistant", "content": "ok"},
|
||||
]
|
||||
for i in range(5):
|
||||
msgs.append({"role": "user", "content": f"step {i}"})
|
||||
msgs.append({"role": "assistant", "content": f"done {i}"})
|
||||
msgs.append({"role": "user", "content": "lights off please"})
|
||||
msgs.append({"role": "assistant", "content": "lights are off"})
|
||||
|
||||
head_end = compressor.protect_first_n
|
||||
cut = compressor._find_tail_cut_by_tokens(msgs, head_end)
|
||||
tail = msgs[cut:]
|
||||
|
||||
if tail and tail[0].get("role") == "user":
|
||||
assert len(tail) >= 2 and tail[1].get("role") == "assistant", (
|
||||
f"Orphan user turn at tail start: {tail[0]['content']!r} — "
|
||||
f"next role is {tail[1].get('role') if len(tail) > 1 else 'nothing'}"
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue