Python: Bound tool result compaction summaries - #7396
Conversation
There was a problem hiding this comment.
🟡 Not ready to approve
The current digest-building logic can still reintroduce tool results from already-excluded messages into the new summary message, undermining exclusion semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR updates the Python ToolResultCompactionStrategy to prevent the synthesized [Tool results: ...] replacement message from embedding unbounded tool-result payload text, keeping small-result summaries unchanged while bounding large digests and adding a regression test.
Changes:
- Add a bounded summary-text helper (
_summary_text) with a truncation marker and a 4096-character cap for tool-result digests. - Update tool-result compaction to use the bounded helper when generating
[Tool results: ...]messages. - Add a regression test to ensure large tool-result payloads are not embedded verbatim and are truncated to the cap.
File summaries
| File | Description |
|---|---|
| python/packages/core/agent_framework/_compaction.py | Adds bounded tool-result digest construction and routes ToolResultCompactionStrategy summaries through it. |
| python/packages/core/tests/core/test_compaction.py | Adds a regression test asserting large tool-result summaries are truncated and capped. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Keep ToolResultCompactionStrategy from re-inserting oversized tool result payloads through the synthetic summary message by bounding the generated digest text. Add regression coverage proving a large tool result is not embedded verbatim, keeps a bounded prefix, and marks truncation.
Build ToolResultCompactionStrategy digest content from messages still included in the group so a summary cannot restore payloads that an earlier compaction already excluded. Use the strategy cap constant in the large-payload regression and add coverage for already-excluded tool results. Validation: uv run pytest packages/core/tests/core/test_compaction.py -q -k 'tool_result_compaction'; uv run ruff check packages/core/agent_framework/_compaction.py packages/core/tests/core/test_compaction.py; uv run ruff format --check packages/core/agent_framework/_compaction.py packages/core/tests/core/test_compaction.py; uv run poe test -P core; uv run poe build -P core; env HOME=/tmp/sds-home XDG_CACHE_HOME=/tmp/sds-cache uv run poe test -A.
7c85c42 to
b6881b6
Compare
There was a problem hiding this comment.
🟡 Not ready to approve
There’s a correctness risk in exclusion filtering (is not True identity check) and a brittleness issue in the new truncation test that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
python/packages/core/agent_framework/_compaction.py:903
included_group_msgschecks the exclusion flag usingis not True, which can incorrectly treat non-bool truthy values (e.g.,numpy.bool_(True)or other truthy sentinels) as included. Elsewhere in this module (included_messages,_included_group_ids,set_excluded) exclusion is treated as a boolean viaget(EXCLUDED_KEY, False). Align this filter to the same boolean semantics so excluded messages can’t leak into the digest.
group_msgs = grouped.get(group_id, [])
included_group_msgs = [msg for msg in group_msgs if msg.additional_properties.get(EXCLUDED_KEY) is not True]
# Build a call_id → function_name map from function_call contents.
python/packages/core/tests/core/test_compaction.py:778
- The "large summary payload" test hard-codes the tool-result size via
1_000lines. That makes the test brittle if_SUMMARY_MAX_CHARSis ever increased enough that this payload no longer triggers truncation, causing assertions like"[truncated]" in summary_textand"file-end" not in summary_textto fail even though the implementation is correct. Consider constructinglarge_resultbased onToolResultCompactionStrategy._SUMMARY_MAX_CHARSso the test always exercises the truncation branch.
"""Summary text should not embed an oversized tool result verbatim."""
large_result = "file-start\n" + ("line contents\n" * 1_000) + "file-end"
messages = [
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Align ToolResultCompactionStrategy's included-message filter with the module's existing EXCLUDED_KEY boolean semantics. Make the large-payload regression size scale from _SUMMARY_MAX_CHARS so it continues to exercise truncation if the digest cap changes. Validation: uv run pytest packages/core/tests/core/test_compaction.py -q -k 'tool_result_compaction'; uv run ruff check packages/core/agent_framework/_compaction.py packages/core/tests/core/test_compaction.py; uv run ruff format --check packages/core/agent_framework/_compaction.py packages/core/tests/core/test_compaction.py; uv run poe test -P core; uv run poe build -P core.
There was a problem hiding this comment.
🟢 Ready to approve
The changes directly address the reported compaction issues (unbounded summary payload and excluded-content restoration) with targeted implementation updates and clear regression tests.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Motivation & Context
ToolResultCompactionStrategyreplaces older tool-call groups with a synthetic[Tool results: ...]message, but that digest can include the full text of large tool results. In read-heavy workloads, the strategy can report that compaction ran while preserving most of the payload in the newly inserted message.The digest should also respect existing exclusion state: if an earlier compaction pass already excluded a message from context, a later tool-result summary should not restore that payload.
This change keeps the existing digest behavior for small included results while preventing one generated summary message from carrying an unbounded payload or reintroducing already-excluded content.
Description & Review Guide
Related Issue
Fixes #7393
Related: #7391/#7387 covers excluded-message restoration in the same digest path; this PR includes that guard because it affects the summary-building logic touched here.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.