fix(condenser): split summarization prompt into system + user messages - #5143
Conversation
The LLMSummarizingCondenser previously sent its summarization request to the condenser's own LLM as a single `user` message containing both the steering instructions and the forgotten-event payload. This puts the event data in the wrong slot on the OpenAI Responses API path: a `system` message serializes to the `instructions` field with empty `input` (via Message.to_responses_value / message_to_responses_dict returning [] for system), so the payload-to-summarize never lands as a real input item. Split the single `summarizing_prompt.j2` template into two: - `summarizing_system.j2` -> sent as a `system` message (steering instructions) - `summarizing_events.j2` -> sent as a `user` message (the <EVENT> payload) This is the canonical system-preamble + user-payload shape for both the Chat Completions and Responses APIs: the steering header goes to `instructions`/ `system` and the event data goes to `input`/`user`, where it belongs. It also plays well with the subscription/Codex transport, which prepends system chunks onto the first user message. The change is isolated to the condenser's separate side-channel call (self.llm.generate(..., store=False)) and does not touch the agent's main conversation. Add a shared `_build_summary_messages` helper to dedupe the sync and async generation paths. Co-authored-by: openhands <openhands@all-hands.dev>
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review summary
The code change itself is correct and well-scoped. _build_summary_messages() produces a canonical system + user pair, both the sync and async condensation paths now share it, the removed summarizing_prompt.j2 had no other references in the tree (verified with a repo-wide grep, including pyproject.toml/MANIFEST.in which glob *.j2), and the split is content-preserving against the old combined template. I confirmed the serialization empirically on the head checkout:
- Chat Completions: roles
['system', 'user']. - Responses API:
instructions= the steering text,input= oneuseritem carrying the<EVENT>payload (this is the slot-mapping the linked issue describes). - Subscription/Codex:
transform_for_subscriptionprepends the system chunk onto the first user message as intended. - Prompt caching:
_apply_prompt_cachingnow marks the system block plus the trailing user block (two breakpoints), which is within Anthropic's limit.
pytest tests/sdk/context/condenser/test_llm_summarizing_condenser.py -> 44 passed, and the broader set (tests/sdk/context/, tests/sdk/conversation/test_condense.py, tests/sdk/agent/test_agent_context_window_condensation.py) -> 470 passed on this head. ruff check/ruff format --check are clean on the changed files.
I found no material correctness, security, or compatibility defect in the diff. Two blocking gaps remain, both process/evidence rather than code.
Blocking findings
1. Required integration evidence is missing for a prompt-template change
AGENTS.md (TESTING) states: "For changes to prompt templates, tool descriptions, or agent decision logic, add the integration-test label to trigger integration tests and verify no unexpected impact on benchmark performance." This PR rewrites the condenser's prompt templates (summarizing_system.j2, summarizing_events.j2) and changes the message roles sent on every condensation, which the review guide's "Agent behavior and evaluation" checkpoint explicitly covers (prompts, condensation).
No integration-test or condenser-test label is present, and no Run Integration Tests workflow ran for head 60ca1cc. The PR description reports only unit-test runs. The required eval evidence is therefore missing, which per the repository review guide is a COMMENT condition. Please add the integration-test (or condenser-test) label and confirm the results on this head before merge.
2. Current-head required check is failing: PR Description Check
The Validate PR description check failed on this head with six errors, including: the first visible line must be HUMAN:; a human-written note is required between HUMAN: and AGENT:; and the ## Why and ## How to Test template sections are absent. It also reports: "Linked issue(s) (#5142) carry neither ready-for-dev nor a pre-rollout creation date." Per the repository's PR_DESCRIPTION_HUMAN_CHECK policy, the HUMAN: section is reserved for a human and must not be filled in by an AI agent, so this needs the author to complete the description in their own words and the issue to be marked ready-for-dev.
Verdict
The implementation is sound and I would approve the code on its own merits. It is not mergeable at this head because the repository-required prompt-template integration evidence is absent and a required check is red. Both need to be resolved before approval.
🔄 CHANGES REQUESTED
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
juanmichelini
added
the
integration-test
|
Hi! I started running the integration tests on your PR. You will receive a comment with the results shortly. |
🧪 Integration Tests ResultsOverall Success Rate: 98.0% 📁 Detailed Logs & ArtifactsClick the links below to access detailed agent/LLM logs showing the complete reasoning process for each model. On the GitHub Actions page, scroll down to the 'Artifacts' section to download the logs.
📊 Summary
📋 Detailed Resultslitellm_proxy_openai_gpt_5.5
litellm_proxy_gemini_3.1_pro_preview
litellm_proxy_anthropic_claude_sonnet_4_6
Failed Tests:
litellm_proxy_deepseek_deepseek_v4_flash
litellm_proxy_minimax_MiniMax_M2.7
Skipped Tests:
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Review summary
I re-checked this PR fresh at head bb07a23 and found no material correctness, security, compatibility, or acceptance-criterion defect. Both concerns from my earlier reviews are now resolved: the integration-test label is applied and the linked issue #5142 now carries ready-for-dev, and the Validate PR description check is green on this head (the body now has the HUMAN:/AGENT: markers, ## Why, and ## How to Test).
What I verified on this head:
- Content preservation. Reconstructing the two new templates (
summarizing_system.j2+summarizing_events.j2) yields text identical to the removed combinedsummarizing_prompt.j2, so no instructions or payload text is lost or duplicated by the split. - No stale references. A repo-wide search finds no remaining reference to
summarizing_prompt.j2;pyproject.toml(*.j2) andMANIFEST.in(recursive-include openhands-sdk *.j2) still package both new templates, so the rename does not drop them from the distribution. - Transport shape (real code paths).
_build_summary_messages()returns['system', 'user']; on the Responses pathformat_messages_for_responsesyields the steering text ininstructionsand exactly oneuserinput item carrying the<EVENT>payload.transform_for_subscriptionprepends the system chunk onto the first user message as described. - Prompt caching side effect.
_apply_prompt_cachingnow marks the system block plus the trailing user block — two breakpoints, within provider limits, and a strict improvement over the previous single breakpoint. - Scope. The change is confined to the condenser's isolated side-channel call;
CondensationSummaryEventand the agent's main history are untouched, and no serialized event, setting, or API contract changes.
Evidence at this head: 52 check runs, 50 success / 2 skipped / 0 failure. pytest tests/sdk/context/condenser/test_llm_summarizing_condenser.py -> 45 passed; the broader set (tests/sdk/context/, tests/sdk/conversation/test_condense.py, tests/sdk/agent/test_agent_context_window_condensation.py) -> 474 passed. ruff check/ruff format --check clean on the changed packages.
Non-blocking observations
- The label-triggered Python eval run (
Run Integration Tests integration-test, run 36207121798) executed on commitd26b34b, which is an ancestor of this head, and reported 98.0% overall success with the workflow concluding success. The condenser files are byte-identical betweend26b34bandbb07a23(the only delta is unrelated grayswan security files), so the run did exercise the changed code. Flagging only for transparency, not as a blocker. - The single failed eval case (
t02_add_bash_hello, "Shell script is not executable", Claude Sonnet 4.6) is a shell-permission assertion unrelated to condenser message roles; the condenser path is not exercised in that short test, so it is not a regression from this change.
Verdict
The change is correct, minimal, and adequately evidenced. Approving.
✅ APPROVED
3 participants
HUMAN:
Splitting the condenser's summarization prompt into a canonical system + user pair so the Responses API path stops dropping the event payload into
instructionswith emptyinput. I want the steering text and the<EVENT>data to land in the correct slots on every transport.AGENT:
Why
The
LLMSummarizingCondenserpreviously sent a singleusermessage that bundled the "You are maintaining a context-aware state summary..." steering instructions with the<EVENT>list (prompts/summarizing_prompt.j2). This conflates "how to summarize" (steering) with "what to summarize" (data). On the OpenAI Responses API path, a lonesystem-style payload serialized to theinstructionsfield with emptyinput(Message.to_responses_valuereturns a string for system;message_to_responses_dictreturns[]for system), so the event data never landed as a real input item.The system + user split is the canonical shape for both Chat Completions and Responses: the steering header goes to
instructions/system, the event data goes toinput/user. It also plays well with the subscription/Codex transport (transform_for_subscription), which prepends system chunks onto the first user message.This change is safe because the condenser makes an independent, single-turn, side-channel call to its own LLM (
self.llm.generate(messages=..., store=False)) — it builds its message list from scratch (no agent system prompt, no tools, no prior turns). It only affects that isolated call and does not touch the agent's main conversation history. The summary still re-enters the agent's view as aCondensationSummaryEvent(usermessage) at the forget boundary, exactly as before. Prompt caching is also fine:_apply_prompt_cachingnow marks the system block plus the trailing user block (two breakpoints), within Anthropic's limit.Summary
prompts/summarizing_system.j2— the steering instructions, sent as asystemmessage.prompts/summarizing_events.j2— just the<EVENT>loop + "Now summarize the events using the rules above.", sent as ausermessage.prompts/summarizing_prompt.j2(the old single combined template).LLMSummarizingCondenser._build_summary_messages()helper; both the sync_generate_condensationand async_agenerate_condensationnow use it (dedupes the two paths).test_get_condensation_with_previous_summaryto assertmessages[0]issystemandmessages[1](the events payload) isuser, since the previous summary now lives in the user message.Issue Number
Closes #5142.
How to Test
Run the condenser unit and broader context tests on this head:
pytest tests/sdk/context/condenser/test_llm_summarizing_condenser.py pytest tests/sdk/context/ tests/sdk/conversation/test_condense.py tests/sdk/agent/test_agent_context_window_condensation.py ruff format --check && ruff checkTo verify the message roles end-to-end across transports, build the summary messages from a condenser instance and assert the serialization:
['system', 'user'].instructionsis the steering text andinputis oneuseritem carrying the<EVENT>payload.transform_for_subscriptionprepends the system chunk onto the first user message as intended.Verification
pytest tests/sdk/context/condenser/test_llm_summarizing_condenser.py-> 44 passedtests/sdk/context/,tests/sdk/conversation/test_condense.py,tests/sdk/agent/test_agent_context_window_condensation.py) -> 470 passedruff format+ruff checkcleanmsg[0].role == "system"(instructions) andmsg[1].role == "user"(the<EVENT>list), and the condensation produces the summary correctlyType
Notes
The
summarizing_prompt.j2removal was verified with a repo-wide grep (includingpyproject.toml/MANIFEST.inwhich glob*.j2) — no other references remain.This PR was created by an AI agent (OpenHands) on behalf of @juanmichelini.
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:bb07a23-pythonRun
All tags pushed for this build
About Multi-Architecture Support
bb07a23-python) is a multi-arch manifest supporting both amd64 and arm64bb07a23-python-amd64) are also available if needed