Skip to content

fix(condenser): show the summarizer whole events, not their 500-char preview (v1.54.0 port) - #4

Merged
thilinawishvekeerthi merged 2 commits into
ergatis-v1.54.0from
fix/condenser-lossless-summary-input-v1.54
Oct 17, 2026
Merged

thilinawishvekeerthi merged 2 commits into
ergatis-v1.54.0from
fix/condenser-lossless-summary-input-v1.54

Conversation

@YugmaF

@YugmaF YugmaF commented Oct 17, 2026 •

Copy link
Copy Markdown
Collaborator

HUMAN:


AGENT:

Why

This ports #2 (the v1.44.1 line) onto ergatis-v1.54.0. This line still builds the summarizer's input from str(event), a display preview capped at 500 characters:

  • The previous summary was cut to its first 497 characters. Every condensation after the first dropped what the last one had recorded.
  • An action was rendered as its class name, with no command or arguments.
  • User messages and tool results were cut at 500 characters.

Measured on the Ergatis dev DB:

  • 142 of 142 summaries were longer than 500 characters.
  • 107 condensations, in 24 runs, re-summarised a truncated summary. Those runs hold 57% of all agent actions.

The full evidence is in #2 and Ergartis/ergatis#197.

Without this port, moving the Ergatis pin to the v1.54.0 line would silently drop the fix.

Summary

  • render_event_for_summary renders:

    • the previous summary, whole;
    • an action as tool + summary + thought + arguments;
    • results, messages and errors head and tail, at 10,000 characters.

    The prompt (summarizing_system.j2 on this line) carries the previous summary forward.

  • Overflow: a summarizer overflow retries with a cap that cuts the whole prompt by 20%, and raises when nothing is left to cut. The hard-reset retries use the same cap.

  • The emitted summary is secret-masked (_mask_condensation).

  • Adapted to v1.54.0:

    • generate() / agenerate();
    • the system/user prompt split;
    • hard-reset shrinking measures the events it summarises, because the leading SystemPromptEvent is preserved on this line.

Issue Number

There is no issue in this fork. Related: #2, Ergartis/ergatis#197, and #1 / #3, now merged into the base.

How to Test

uv sync --frozen --dev
uv run pytest tests/sdk/context/condenser tests/sdk/agent/test_condensation_summary_masking.py tests/sdk/test_settings.py
uv run pytest tests/sdk tests/cross -n 6

Type

  • Bug fix

Notes

…preview

Port of a8babaa + 84b438a (fix/condenser-lossless-summary-input, the
v1.44.1 line) onto ergatis-v1.54.0. This line still built the summarizer
input from str(event), a display preview capped at N_CHAR_PREVIEW (500):

- the previous summary was cut to its first 497 characters, so every
  condensation after the first dropped what the last one recorded;
- an action rendered as its class name, with no command or arguments;
- user messages and tool results lost everything past 500 characters.

Measured on an Ergatis deployment: 142/142 summaries exceeded 500
characters; 107 condensations in 24 runs (57% of all agent actions)
re-summarised a truncated summary.

render_event_for_summary renders the previous summary whole, an action as
tool + summary + thought + arguments, and results/messages/errors head and
tail at max_event_length (10,000). The prompt (summarizing_system.j2 on
this line) carries the previous summary forward and keeps user answers
verbatim. A summarizer overflow retries with a cap that cuts the whole
prompt by hard_context_reset_context_scaling, raising when nothing is
left to cut; hard-reset retries use the same cap. The emitted summary is
secret-masked (_mask_condensation), since the summarizer now sees raw
tool arguments.

Adapted to this line: generate()/agenerate() and the system/user prompt
split; hard-reset shrinking measures the events it summarises (the
leading SystemPromptEvent is preserved here).
@viper-review

viper-review Bot commented Oct 17, 2026

Copy link
Copy Markdown

Viper is reviewing this pull request — findings and a summary will be posted shortly.

@viper-review viper-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review comments

other failure is raised at once.
"""
max_event_str_length: int | None = None
for _ in range(self.hard_context_reset_max_retries):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] _summarize drives its shrink-and-retry loop with hard_context_reset_max_retries, a field documented as the number of hard context reset attempts. A user who lowers it to bound reset cost (e.g. to 1) now also caps summarizer shrink attempts at 1: the first context-window overflow computes a smaller cap but the loop has no second iteration, so it raises and forces the hard reset this PR is trying to avoid. Use a dedicated retry field (or at least document the shared meaning).

@viper-review viper-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review comments

"Summarizer context exceeded at every event length tried."
)

async def _asummarize(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Low] _asummarize is a line-for-line copy of _summarize (283-320) differing only in await self._agenerate_condensation, and the same overflow-shrink block is repeated in hard_context_reset (514-517) and ahard_context_reset (653-655). Extracting one helper keeps the retry/abort logic (including the <= 0 bail-out) from silently drifting between the four copies.

@viper-review viper-review Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Auto decision by Viper: aggregated open high=0 (threshold 1), open medium=0 (threshold 3) => APPROVE.

@viper-review

viper-review Bot commented Oct 17, 2026

Copy link
Copy Markdown

Summary

Ports the v1.44.1 condenser fix onto the v1.54.0 line so the summarizer consumes full events instead of 500-char display previews; the change is well-scoped and carries only non-blocking duplication/config findings.
0 high · 0 medium · 2 low findings.

Description

Replaces the summarizer's input construction, which previously derived content from str(event) and truncated summaries to 497 characters, actions to their class name, and messages/tool results to 500 characters. Introduces render_event_for_summary to emit the prior summary whole and to render actions as tool + summary + thought + arguments, with head/tail truncation at 10,000 characters for results, messages, and errors. Adds overflow handling that retries with a cap cutting the whole prompt by 20% and raises when nothing remains to cut, applies the same cap to hard-reset retries, and masks secrets in the emitted summary via _mask_condensation. Adaptation to this line covers generate() / agenerate(), the system/user prompt split, and hard-reset shrinking that measures only the summarized events because the leading SystemPromptEvent is preserved.

Walkthrough

  • Condenser rendering — utils.py, llm_summarizing_condenser.py: full-event serialization, 10k head/tail budget, prompt shrink-and-retry on overflow.
  • Prompt contract — summarizing_system.j2: instructs carrying the previous summary forward.
  • Agent integration — agent.py: wiring for the reset path and summary emission.
  • Secret masking — _mask_condensation applied to the produced summary.
  • Tests — new condenser coverage plus test_condensation_summary_masking.py guards.

Findings Overview

  • Low — Configuration coupling: the shrink-and-retry loop is driven by hard_context_reset_max_retries, so lowering that field to bound reset cost also caps overflow retries and forces the hard reset this PR avoids.
  • Low — Duplication: _asummarize is a line-for-line copy of _summarize, and the overflow-shrink block is replicated in both hard_context_reset variants; four copies of retry/abort logic (including the <= 0 bail-out) risk silent drift.

Narrative Summary

This PR restores fidelity to the summarizer's input on the v1.54.0 line, where str(event) previously capped summaries at 497 characters, reduced actions to bare class names, and clipped messages and tool results at 500 characters — a defect the PR quantifies as affecting 107 of 142 condensations. The implementation adds a dedicated renderer with 10k head/tail truncation, prompt shrink-and-retry on overflow, and secret masking of the emitted summary, adapted to this line's generate()/agenerate() split and system/user prompt separation. The two findings are structural rather than behavioral: the retry counter is borrowed from hard-reset configuration, and the sync/async summarize paths plus the two hard-reset paths duplicate the same overflow logic. Address the retry-field coupling first, since it can silently disable the fix for users who tune reset retries low; consolidating the four duplicated blocks is a secondary cleanup that reduces future drift risk.

@viper-review

viper-review Bot commented Oct 17, 2026

Copy link
Copy Markdown

Viper is reviewing this pull request — findings and a summary will be posted shortly.

@viper-review

viper-review Bot commented Oct 17, 2026

Copy link
Copy Markdown

Summary

The production change is small and contained: create_mcp_tools now uses an MCPConfigTransport subclass to connect multi-server MCP configs concurrently with per-server deadlines. Remaining concerns are test robustness only.
0 high · 0 medium · 3 low findings.

Description

This update ports the parallel MCP startup behavior: multi-server MCP configs no longer connect sequentially under a single caller timeout, so one unresponsive server cannot fail the entire create_mcp_tools call. Each server is connected under its own deadline, and failures/timeouts are logged by name and skipped; single-server configs still use the direct transport path. The update also includes test-only cleanup to satisfy ruff and pyright for retained Ergatis patch tests.

Walkthrough

  • MCP transport (openhands-sdk/openhands/sdk/mcp/utils.py): Adds concurrent server startup with per-server timeout handling while preserving fastmcp mounting semantics.
  • Parallel startup tests (tests/sdk/mcp/test_mcp_parallel_startup.py): Exercises concurrent connection behavior and timeout skipping.
  • Pre-commit test fixes: Adjusts retained patch tests for formatting/type-check compliance without behavior changes.

Findings Overview

All findings are LOW severity and confined to test_mcp_parallel_startup.py:

  • Hard-coded timeout text assertion couples the test to _SERVER_CONNECT_TIMEOUT_FRACTION and timeout=5.0.
  • Concurrency timing bound can fail spuriously on loaded CI runners.
  • Fixture records the server after _wait_for_port, risking leaked uvicorn thread/port if slow boot causes an exception.

Narrative Summary

This update ports concurrent multi-server MCP startup so a silent server no longer blocks every server’s tools, while preserving fastmcp’s mounting behavior and single-server path. The production change is narrowly scoped and no functional issues were identified. The remaining findings are test-quality issues: brittle timeout/timing assertions and a fixture ordering problem that can leak a server thread/port on slow startup. Prioritize fixing the fixture ordering first, then loosen the timing assertion to a stable message check.

@thilinawishvekeerthi
thilinawishvekeerthi merged commit c7b23f3 into ergatis-v1.54.0 Oct 17, 2026
6 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants