fix(agent): preserve a non-empty user query after compression
This commit is contained in:
parent
021d191478
commit
3127ddcb64
|
|
@ -6264,16 +6264,18 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
None,
|
||||
)
|
||||
first_tail_role = None
|
||||
first_tail_visible_idx: Optional[int] = None
|
||||
if tail_messages:
|
||||
first_tail_role = next(
|
||||
first_tail_visible_idx, first_tail_role = next(
|
||||
(
|
||||
role
|
||||
for role in (
|
||||
_template_visible_role(m) for m in tail_messages
|
||||
(idx, role)
|
||||
for idx, role in (
|
||||
(idx, _template_visible_role(m))
|
||||
for idx, m in enumerate(tail_messages)
|
||||
)
|
||||
if role is not None
|
||||
),
|
||||
None,
|
||||
(None, None),
|
||||
)
|
||||
# When the only protected head message is the system prompt, the
|
||||
# summary becomes the first *visible* message in the API request
|
||||
|
|
@ -6300,11 +6302,27 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
# If no user-role message survives in either the protected head or the
|
||||
# preserved tail, the summary MUST carry role="user" so the request
|
||||
# always has at least one user turn.
|
||||
#
|
||||
# A bare role check is not enough: the tail's sole surviving user
|
||||
# turn can be image-only (a screenshot with no caption). The newest
|
||||
# image-bearing user message is the ``_strip_historical_media``
|
||||
# anchor and is kept byte-for-byte, so it never gains a text
|
||||
# placeholder — its role is "user" but its text content is empty,
|
||||
# which backends checking for actual query text still reject. Count
|
||||
# only user messages with non-empty text as "surviving"; when the
|
||||
# guard fires, the real (never fabricated) summary text lands in a
|
||||
# role="user" slot, which is always non-empty (falls back to
|
||||
# ``_build_static_fallback_summary`` above when generation fails).
|
||||
if not _force_user_leading:
|
||||
def _is_nonempty_user_turn(message: Dict[str, Any]) -> bool:
|
||||
return message.get("role") == "user" and bool(
|
||||
_content_text_for_contains(message.get("content")).strip()
|
||||
)
|
||||
|
||||
_user_survives = any(
|
||||
message.get("role") == "user" for message in compressed
|
||||
_is_nonempty_user_turn(message) for message in compressed
|
||||
) or any(
|
||||
message.get("role") == "user" for message in tail_messages
|
||||
_is_nonempty_user_turn(message) for message in tail_messages
|
||||
)
|
||||
if not _user_survives:
|
||||
_force_user_leading = True
|
||||
|
|
@ -6360,9 +6378,27 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
),
|
||||
})
|
||||
|
||||
# Default merge target: literal tail index 0. For an ordinary
|
||||
# alternation collision the summary only has to stay *invisible* to
|
||||
# the template, and a leading template-exempt row (bare tool-call
|
||||
# assistant message, tool result) is the ideal carrier — it absorbs
|
||||
# the summary without adding a visible turn, and it leaves the live
|
||||
# tail user message intact as the model's actual prompt. Retargeting
|
||||
# to the first template-visible row here would convert that live
|
||||
# request into the summary carrier for no benefit.
|
||||
#
|
||||
# The forced repair path is the exception. There the merge is not
|
||||
# about alternation but about guaranteeing at least one genuinely
|
||||
# non-empty role="user" message (an image-only or otherwise
|
||||
# text-empty surviving user row). An exempt carrier cannot satisfy
|
||||
# that invariant, so the summary text must land on the
|
||||
# template-visible row itself.
|
||||
_merge_target_idx = 0
|
||||
if _force_user_leading and first_tail_visible_idx is not None:
|
||||
_merge_target_idx = first_tail_visible_idx
|
||||
for tail_idx, msg in enumerate(tail_messages):
|
||||
if _merge_summary_into_tail and tail_idx == 0:
|
||||
# Merge the summary into the first (post-strip) tail message.
|
||||
if _merge_summary_into_tail and tail_idx == _merge_target_idx:
|
||||
# Merge the summary into the tail message that collided.
|
||||
old_content = msg.get("content", "")
|
||||
if _force_user_leading and summary_role == "user":
|
||||
# The summary must be part of the first user-visible
|
||||
|
|
|
|||
|
|
@ -154,3 +154,123 @@ class TestCompressAlwaysKeepsAUserTurn:
|
|||
m.get("content") for m in out if isinstance(m.get("content"), str)
|
||||
)
|
||||
assert "the latest live user question" in joined
|
||||
|
||||
|
||||
def _has_nonempty_user_text(messages: list[dict]) -> bool:
|
||||
"""True if some role="user" message carries non-empty text.
|
||||
|
||||
Deliberately narrower than "any message has text": a non-empty
|
||||
``role="assistant"`` summary does not satisfy backends that require an
|
||||
actual user query, so checking role and content together is the point.
|
||||
"""
|
||||
from agent.context_compressor import _content_text_for_contains
|
||||
|
||||
return any(
|
||||
m.get("role") == "user"
|
||||
and _content_text_for_contains(m.get("content")).strip()
|
||||
for m in messages
|
||||
)
|
||||
|
||||
|
||||
class TestCompressKeepsANonEmptyUserTurn:
|
||||
"""A bare ``role == "user"`` check is not enough: the surviving user
|
||||
message can be image-only (no caption). ``_strip_historical_media``
|
||||
anchors on the newest image-bearing user message and keeps it
|
||||
byte-for-byte — it never gains a text placeholder — so a role-only
|
||||
guard is satisfied while the request still has zero *text* user
|
||||
turns, which is what backends actually reject on.
|
||||
"""
|
||||
|
||||
def test_image_only_tail_user_turn_still_yields_non_empty_text(self, compressor):
|
||||
from agent.context_compressor import SUMMARY_PREFIX
|
||||
|
||||
c = compressor
|
||||
c.compression_count = 1
|
||||
# No system message and no protected head (compression_count=1
|
||||
# decays protect_first_n to 0), same #58753 shape — except the
|
||||
# only surviving user turn is an image with no caption text.
|
||||
messages = [{"role": "user", "content": "work kanban task 42"}]
|
||||
messages += _tool_turns(0, 12)
|
||||
messages += [{
|
||||
"role": "user",
|
||||
"content": [
|
||||
{"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}},
|
||||
],
|
||||
}]
|
||||
|
||||
mocked = f"{SUMMARY_PREFIX}\nrolled-up summary of the tool work"
|
||||
with patch.object(c, "_generate_summary", return_value=mocked):
|
||||
out = c.compress(messages, current_tokens=90_000)
|
||||
|
||||
hist = _role_hist(out)
|
||||
assert hist.get("user", 0) >= 1
|
||||
assert _has_nonempty_user_text(out), (
|
||||
"REGRESSION: an image-only user turn satisfied the role-only "
|
||||
"zero-user-turn guard, leaving a transcript with a user-role "
|
||||
"message but no actual query text — the exact class of "
|
||||
"request backends reject with 'No user query found in "
|
||||
f"messages'. Output: {out}"
|
||||
)
|
||||
|
||||
def test_image_only_protected_head_still_yields_non_empty_text(self, compressor):
|
||||
"""Isolates the ``_user_survives`` text check from the merge-target
|
||||
fix below: here ``compress_start != 0`` and there's no system
|
||||
message, so neither existing force condition
|
||||
(``compress_start == 0`` / ``last_head_role == "system"``) fires on
|
||||
its own — only the broadened (text-aware) survival check does.
|
||||
"""
|
||||
from agent.context_compressor import SUMMARY_PREFIX
|
||||
|
||||
c = compressor
|
||||
c.protect_first_n = 1
|
||||
c.protect_last_n = 2
|
||||
c.tail_token_budget = 10
|
||||
c.compression_count = 0
|
||||
|
||||
messages = [{
|
||||
"role": "user",
|
||||
"content": [
|
||||
{"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}},
|
||||
],
|
||||
}]
|
||||
messages += _tool_turns(0, 12)
|
||||
|
||||
mocked = f"{SUMMARY_PREFIX}\nrolled-up summary of the tool work"
|
||||
with patch.object(c, "_generate_summary", return_value=mocked):
|
||||
out = c.compress(messages, current_tokens=90_000)
|
||||
|
||||
assert _has_nonempty_user_text(out), (
|
||||
"REGRESSION: an image-only protected-head user turn satisfied "
|
||||
f"the role-only zero-user-turn guard. Output: {out}"
|
||||
)
|
||||
|
||||
def test_merge_targets_the_colliding_tail_message_not_index_zero(self, compressor):
|
||||
"""Template-exempt rows (bare tool-call assistant / tool messages)
|
||||
ahead of the colliding tail user message must not divert the merge:
|
||||
merging into literal tail index 0 would attach the summary to an
|
||||
exempt row and leave the real (image-only) user message untouched
|
||||
and still empty, silently defeating the forced role="user" this
|
||||
block exists to guarantee.
|
||||
"""
|
||||
from agent.context_compressor import SUMMARY_PREFIX
|
||||
|
||||
c = compressor
|
||||
c.compression_count = 1 # decays protect_first_n to 0 -> compress_start == 0
|
||||
messages = [{"role": "user", "content": "work kanban task 42"}]
|
||||
messages += _tool_turns(0, 12)
|
||||
messages += [{
|
||||
"role": "user",
|
||||
"content": [
|
||||
{"type": "image_url", "image_url": {"url": "data:image/png;base64,AAAA"}},
|
||||
],
|
||||
}]
|
||||
|
||||
mocked = f"{SUMMARY_PREFIX}\nrolled-up summary of the tool work"
|
||||
with patch.object(c, "_generate_summary", return_value=mocked):
|
||||
out = c.compress(messages, current_tokens=90_000)
|
||||
|
||||
assert _has_nonempty_user_text(out), (
|
||||
"REGRESSION: the summary merged into tail index 0 (a "
|
||||
"template-exempt row) instead of the colliding image-only "
|
||||
f"user message, leaving zero non-empty user turns. Output: {out}"
|
||||
)
|
||||
|
|
|
|||
Loading…
Reference in New Issue