Repository navigation
fix(condenser): show the summarizer whole events, not their 500-char preview - #2
Conversation
…preview
LLMSummarizingCondenser built the summarizer's input from str(event), a UI
preview capped at N_CHAR_PREVIEW (500). The summary is the agent's only
memory of every forgotten event, so that preview bounded what it could
remember:
- the previous summary was cut to its first 497 characters (in practice
USER_CONTEXT alone), so every condensation after the first dropped the
COMPLETED / CODE_STATE / VERSION_CONTROL_STATUS the last one recorded;
- an action rendered as its thought and class name ("Action:
TerminalAction") with no command, path or arguments, and most models
emit tool calls with no thought text;
- user messages and tool results lost everything past 500 characters.
Measured on an Ergatis deployment: 142 of 142 summaries exceeded 500
characters (mean 2,785); 107 condensations re-summarised a truncated
summary, in runs holding 57% of all agent actions. One run's summary
listed the backend HEAD and every model; two condensations later it said
branches and dependencies "have not been checked yet", and the agent
re-explored for 152 minutes with 0 edits.
render_event_for_summary renders what the agent itself was shown: the
previous summary whole, an action as tool + summary + thought + arguments,
results, errors and messages as their text, each capped at
max_event_length (10,000) head and tail. The prompt now says to carry the
previous summary forward and keep user answers verbatim. A summarizer
context overflow shrinks every event and retries rather than falling to a
hard context reset; other errors raise at once. Hard-reset retries scale
from the rendered length, not the preview's (~400 chars on retry one).
The summarizer now sees raw tool arguments (a token typed into a git URL
included), so the agent masks registered secrets in the emitted summary,
as it does for its own messages (OpenHands#4783).
|
Viper is reviewing this pull request — findings and a summary will be posted shortly. |
Endpoint auditContract: pinned release artifact
Actionable client-only calls (0)none Actionable server-only operations (18)
Documented non-divergences (13)Client calls intentionally absent from the filtered contract (11)
Reason: Operational Agent Server endpoints intentionally excluded from the filtered public release artifact.
Reason: Client-ahead API stacked on the pending Agent Server meta-profiles implementation.
Reason: Client-ahead API stacked on the pending Agent Server pre-flight LLM validation endpoint. Server operations covered by an exposed browser URL (2)
Reason: RemoteWorkspace.startWorkspaceSession exposes these authenticated URLs for browser iframe and file requests; they are not HttpClient method calls. |
![viper-review[bot]](https://avatars.githubusercontent.com/in/3274114?s=60&v=4){kind=small}
| other failure is raised at once. | ||
| """ | ||
| max_event_str_length: int | None = None | ||
| for _ in range(self.hard_context_reset_max_retries): |
There was a problem hiding this comment.
[Medium] This retry only lowers the per-event cap (_shrunk_event_length derives it from the longest rendered event). When the overflow comes from the summed size of many events that are already shorter than the cap, each iteration re-sends an essentially unchanged oversized prompt, burns all hard_context_reset_max_retries summarizer calls, and then still raises — so the condensation falls through to the full hard reset this retry was added to avoid. Bail out when the new cap does not reduce the rendered total (or start dropping events) instead of retrying blindly. Same defect in _asummarize at line 271.
There was a problem hiding this comment.
Fixed in 84b438a. _shrunk_event_length now picks the largest cap whose capped total is at most hard_context_reset_context_scaling of the prompt just sent. Each retry therefore cuts the whole prompt by that factor, not just the longest event. When no positive cap gets there, it returns 0 and the retry raises ("nothing left to cut"). This matters because maybe_truncate(text, 0) means no truncation and would re-send the full prompt. The hard-reset loops floor the cap at 1 for the same reason.
New tests:
test_summarizer_overflow_shrinks_the_whole_prompt_not_one_event: one 9k result plus forty 1.2k ones. It was red on a8babaa.test_summarizer_overflow_never_resends_untrimmed_events: the 0-cap guard. Mutation-checked: removing the guard turns it red.
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_async_summarizer_sees_the_whole_previous_summary( |
There was a problem hiding this comment.
[Medium] _asummarize duplicates the sync shrink-and-retry loop line for line, but the only async test covers rendering of a previous summary — its overflow branch (shrink then retry, and non-overflow then re-raise) is never exercised. A copy-paste defect there (wrong exception filter, missing retry, wrong max_event_str_length threading) passes CI. Add aget_condensation/acompletion variants of test_summarizer_overflow_shrinks_events_instead_of_failing and test_summarizer_failure_that_is_not_overflow_is_not_retried.
There was a problem hiding this comment.
Added in 84b438a:
test_async_summarizer_overflow_shrinks_events_instead_of_failingtest_async_summarizer_failure_that_is_not_overflow_is_not_retried
Both drive aget_condensation through acompletion. Mutation-checked against _asummarize:
- a copy that never retries fails the first test;
- a copy that retries every error fails the second.
SummaryReplaces the 500-char Description
Walkthrough
Findings Overview
Narrative SummaryThis PR fixes a real memory-loss defect: condensation summaries were built from a 500-character display preview, silently discarding prior summaries, tool arguments and long user turns, and the deployment data cited shows the resulting drift. The renderer, prompt changes and summary masking are well-scoped and the settings schema is untouched. The remaining risk is concentrated in the new overflow retry: shrinking only the per-event cap makes no progress when the overflow is aggregate, so the loop burns its retry budget and still lands on the hard reset it was added to prevent. That logic is duplicated verbatim in |
…est event Review on #2 (Viper): the shrink-and-retry lowered the per-event cap from the longest rendered event only. A prompt made of many mid-sized events was re-sent nearly unchanged, burned every retry, and still fell through to the hard context reset the retry exists to avoid. _shrunk_event_length now picks the largest cap whose capped total is at most hard_context_reset_context_scaling of what was just sent, so each retry cuts the prompt by that factor. When no positive cap gets there it returns 0, and the retry raises instead -- maybe_truncate treats 0 as "no truncation" and would re-send the full prompt. The hard-reset loops floor the cap at 1 for the same reason. Tests: the many-short-events shape (red before), async variants of both overflow tests (mutation-checked: never-retry and retry-everything each fail one), and the 0-cap guard (mutation-checked).
|
Viper is reviewing this pull request — findings and a summary will be posted shortly. |
| assert len(sizes) <= condenser.hard_context_reset_max_retries | ||
| assert sizes == sorted(sizes, reverse=True) | ||
| assert len(set(sizes)) == len(sizes), "a retry re-sent the same prompt" |
There was a problem hiding this comment.
[Medium] All three sizes assertions are vacuously true on an empty list (0 <= n, [] == sorted([]), len(set([])) == 0 == len([])), so the test still passes if get_condensation raises NoCondensationAvailableException before ever calling the summarizer — and the pytest.raises has no match=, so it can't distinguish that path either. The docstring claims to verify that every retry cuts the prompt, so assert len(sizes) >= 2 before the sorted/uniqueness checks (and match the exception message).
| # Since the summarization failed, cut every event to 80% of the | ||
| # longest one as rendered -- not of its 500-character preview. | ||
| max_event_str_length = max( | ||
| 1, self._shrunk_event_length(view.events, max_event_str_length) |
There was a problem hiding this comment.
[Low] max(1, ...) discards the helper's documented 0 return ('nothing left to cut'): once _shrunk_event_length returns 0, the loop retries at cap 1, and from then on every subsequent retry re-renders the same cap-1 prompt and is guaranteed to overflow again, burning the remaining retries on identical failing calls. _summarize raises NoCondensationAvailableException on this exact condition (lines 275 and 308); break/raise here instead of flooring to 1. Same pattern at line 618.
SummaryUpdate hardens overflow retry logic so retries shrink the entire prompt by a fixed factor rather than only the longest rendered event. DescriptionReplaces the previous per-event cap derivation, which only lowered the cap to the longest rendered event and therefore re-sent many-mid-sized-event prompts nearly unchanged, exhausting retries without relieving context pressure. Walkthrough
Findings Overview
Narrative SummaryThis update corrects a real defect in the overflow retry path: scaling only the longest event left many-event prompts effectively unchanged across retries, defeating the shrink-and-retry mechanism and falling through to the hard context reset it was meant to prevent. The factor-based cap selection and the 0-return semantics are the right shape, and the added async and many-short-events tests exercise previously uncovered branches. The remaining risk is concentrated in test integrity — the primary regression test can pass without ever reaching the summarizer, which weakens the mutation-checked guarantees the description relies on. Fix the vacuous assertions first, then reconcile the |
1 participant
HUMAN:
AGENT:
Why
LLMSummarizingCondenserbuilt the summarizer's input fromstr(event). That is a display preview, capped atN_CHAR_PREVIEW= 500 characters. The summary is the agent's only memory of every forgotten event, so the preview limited what the agent could remember:USER_CONTEXTsurvived, so every condensation after the first dropped the COMPLETED, CODE_STATE and VERSION_CONTROL_STATUS that the last one had recorded.Action: TerminalAction), with no command, path or arguments. 70% of actions on our deployment carry no thought text.Measured on the Ergatis dev DB:
1297b899: the summary at seq 403 listed backend HEADd006196and every model. By seq 509 the next summary said branches and dependencies "have not been checked yet". The run made 16 condensations and 0 edits in 152 minutes.Upstream
mainstill has the bug.Summary
render_event_for_summary(context/condenser/utils.py) renders:Each event is capped at
max_event_length= 10,000 characters, keeping head and tail. It is aClassVar, so the settings schema does not change. The prompt says to carry the previous summary forward and to quote short user answers verbatim.Overflow: a summarizer context overflow retries with a cap that cuts the whole prompt by
hard_context_reset_context_scaling. It raises when nothing is left to cut. Any other error is raised at once. The hard-reset retries use the same cap._mask_condensation(agent/agent.py): the emitted summary is secret-masked, because the summarizer now sees raw tool arguments (fix(sdk): mask model output in the durable MessageEvent OpenHands/software-agent-sdk#4783).Issue Number
There is no issue in this fork. Platform side: Ergartis/ergatis#197, which bumps the
runners/agent-sdkpin to this branch.How to Test
ae19dedd0, so this adds 0 new ones.task_eventrows ofkind='Condensation'for one run and compare successive summaries.Type
Notes
pre-commit --all-filesflags files from the base branch: terminal timeout files,test_acp_agent.py,test_list_directory.py:43, and pyright in older tests andagent.py_tools.cleanup-on-approvalneeds the upstream secretOPENHANDS_BOT_GITHUB_PAT_PUBLIC, which this fork does not have.ergatis-v1.54.0follows as a separate PR. That line still usesstr(forgotten_event).