fix(compressor): pin summary role to user when only system prompt is protected (#52160)
After the first compaction protect_first_n decays, so on a later compaction the only protected head message can be the system prompt. Adapters like Anthropic and Bedrock send the system prompt as a separate parameter, so the summary becomes the first message in messages[] — and Anthropic rejects any request whose first message is not role=user (HTTP 400). Pin the summary to role=user when the head is system-only, and stop the collision-flip logic from reverting it back to assistant. Salvaged from #52167. Co-authored-by: liuhao1024 <sunsky.lau@gmail.com>
This commit is contained in:
parent
82ac7e16b8
commit
8f4d195d5f
2 changed files with 110 additions and 2 deletions
|
|
@ -2818,9 +2818,17 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
_merge_summary_into_tail = False
|
||||
last_head_role = messages[compress_start - 1].get("role", "user") if compress_start > 0 else "user"
|
||||
first_tail_role = messages[compress_end].get("role", "user") if compress_end < n_messages else "user"
|
||||
# When the only protected head message is the system prompt, the
|
||||
# summary becomes the first *visible* message in the API request
|
||||
# (most adapters — Anthropic, Bedrock — send the system prompt as
|
||||
# a separate ``system`` parameter, not inside ``messages[]``).
|
||||
# Anthropic unconditionally rejects requests whose first message
|
||||
# is not role=user, so we must pin the summary to "user" and
|
||||
# prevent the flip logic below from reverting it (#52160).
|
||||
_force_user_leading = last_head_role == "system"
|
||||
# Pick a role that avoids consecutive same-role with both neighbors.
|
||||
# Priority: avoid colliding with head (already committed), then tail.
|
||||
if last_head_role in {"assistant", "tool"}:
|
||||
if last_head_role in {"assistant", "tool"} or _force_user_leading:
|
||||
summary_role = "user"
|
||||
else:
|
||||
summary_role = "assistant"
|
||||
|
|
@ -2828,7 +2836,7 @@ This compaction should PRIORITISE preserving all information related to the focu
|
|||
# collide with the head, flip it.
|
||||
if summary_role == first_tail_role:
|
||||
flipped = "assistant" if summary_role == "user" else "user"
|
||||
if flipped != last_head_role:
|
||||
if flipped != last_head_role and not _force_user_leading:
|
||||
summary_role = flipped
|
||||
else:
|
||||
# Both roles would create consecutive same-role messages
|
||||
|
|
|
|||
|
|
@ -3104,3 +3104,103 @@ class TestCooldownReentryAbort:
|
|||
)
|
||||
assert c._last_compress_aborted is True
|
||||
assert c._last_summary_fallback_used is False
|
||||
|
||||
|
||||
|
||||
|
||||
class TestDoubleCompactionSummaryRole:
|
||||
"""PR #52160 (salvaged from #52167): when only the system prompt is
|
||||
protected, the summary must lead with role=user (Anthropic/Bedrock send
|
||||
system as a separate param, so the summary is the first visible message)."""
|
||||
|
||||
def test_double_compaction_summary_must_be_user_when_only_system_protected(self):
|
||||
"""After the first compression, protect_first_n decays to 0.
|
||||
|
||||
On the second compression the only protected head message is the
|
||||
system prompt (role=system). The summary becomes the first
|
||||
*visible* message in the API request because adapters like
|
||||
Anthropic and Bedrock send the system prompt as a separate
|
||||
``system`` parameter. The summary MUST be role=user or the
|
||||
provider rejects with HTTP 400 (#52160).
|
||||
"""
|
||||
mock_response = MagicMock()
|
||||
mock_response.choices = [MagicMock()]
|
||||
mock_response.choices[0].message.content = "summary of earlier turns"
|
||||
|
||||
with patch("agent.context_compressor.get_model_context_length", return_value=100000):
|
||||
c = ContextCompressor(
|
||||
model="test", quiet_mode=True, protect_first_n=2, protect_last_n=2,
|
||||
)
|
||||
# Simulate second compression: protect_first_n decays to 0.
|
||||
c.compression_count = 1
|
||||
|
||||
# compress_start will be 1 (system only), last_head_role = "system".
|
||||
# Without the fix, summary_role would be "assistant".
|
||||
msgs = [
|
||||
{"role": "system", "content": "You are a helpful assistant."},
|
||||
{"role": "user", "content": "msg 1"},
|
||||
{"role": "assistant", "content": "msg 2"},
|
||||
{"role": "user", "content": "msg 3"},
|
||||
{"role": "assistant", "content": "msg 4"},
|
||||
{"role": "user", "content": "msg 5"},
|
||||
{"role": "assistant", "content": "msg 6"},
|
||||
]
|
||||
with patch("agent.context_compressor.call_llm", return_value=mock_response):
|
||||
result = c.compress(msgs)
|
||||
|
||||
# The system message must still be at index 0.
|
||||
assert result[0]["role"] == "system"
|
||||
# The summary (first non-system message) must be role=user.
|
||||
non_system = [m for m in result if m.get("role") != "system"]
|
||||
assert non_system, "expected at least one non-system message"
|
||||
assert non_system[0]["role"] == "user", (
|
||||
f"first non-system message must be role=user for Anthropic "
|
||||
f"compatibility, got role={non_system[0]['role']!r}"
|
||||
)
|
||||
|
||||
def test_double_compaction_user_tail_merges_into_tail(self):
|
||||
"""When the summary is forced to role=user (system-only head) and
|
||||
the first tail message is also user, the summary must merge into
|
||||
the tail rather than flipping back to assistant (#52160).
|
||||
"""
|
||||
mock_response = MagicMock()
|
||||
mock_response.choices = [MagicMock()]
|
||||
mock_response.choices[0].message.content = "summary of earlier turns"
|
||||
|
||||
with patch("agent.context_compressor.get_model_context_length", return_value=100000):
|
||||
c = ContextCompressor(
|
||||
model="test", quiet_mode=True, protect_first_n=2, protect_last_n=2,
|
||||
)
|
||||
c.compression_count = 1 # decay protect_first_n
|
||||
|
||||
# tail starts with user → would collide with forced summary_role=user.
|
||||
# The fix should merge into tail instead of flipping to assistant.
|
||||
msgs = [
|
||||
{"role": "system", "content": "You are a helpful assistant."},
|
||||
{"role": "user", "content": "msg 1"},
|
||||
{"role": "assistant", "content": "msg 2"},
|
||||
{"role": "user", "content": "msg 3"},
|
||||
{"role": "assistant", "content": "msg 4"},
|
||||
{"role": "user", "content": "msg 5"}, # tail start (user)
|
||||
{"role": "assistant", "content": "msg 6"},
|
||||
{"role": "user", "content": "msg 7"},
|
||||
]
|
||||
with patch("agent.context_compressor.call_llm", return_value=mock_response):
|
||||
result = c.compress(msgs)
|
||||
|
||||
# No standalone summary message should exist (merged into tail).
|
||||
summary_msgs = [
|
||||
m for m in result
|
||||
if m.get("_compressed_summary") and "msg 5" not in (m.get("content") or "")
|
||||
]
|
||||
assert len(summary_msgs) == 0, (
|
||||
"summary should be merged into tail, not standalone"
|
||||
)
|
||||
# The first non-system message must be role=user.
|
||||
non_system = [m for m in result if m.get("role") != "system"]
|
||||
assert non_system[0]["role"] == "user"
|
||||
# The merged tail should contain the summary text.
|
||||
assert any(
|
||||
"summary of earlier turns" in (m.get("content") or "")
|
||||
for m in result
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue