fix(sdk): stop the overflow clip from dropping media and over-clipping - #5638
Open
Rajarshi Datta (rajarshidattapy) wants to merge 3 commits into
Conversation
Rajarshi Datta (rajarshidattapy)
force-pushed
the
fix/4954-overflow-clip-media-v2
branch
from
August 19, 2026 04:05
5667a95 to
76ca9f0
Compare
Author
|
Mason Daugherty (@mdrxy) done, can you check this once? |
Mason Daugherty (mdrxy)
force-pushed
the
mdrxy/sdk/preview-note-conditional
branch
from
August 19, 2026 22:32
29d5f63 to
44d11a3
Compare
…low-clip-media-v2 Keep the overflow-clip module docstring that explains why results already smaller than the slice size are left untouched. The incoming branch never had that paragraph; it documents the _is_worth_clipping behavior this PR adds. Co-authored-by: Cursor <cursoragent@cursor.com>
Author
|
Mason Daugherty (@mdrxy) can you review this? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4954. Reopens #5060, rewritten against #5563 as requested — this targets
mdrxy/sdk/preview-note-conditional, so the diff here is only the overflow-clip fix on top of that branch.When summarization's overflow fallback clips the trailing tool-result batch, a
read_fileresult that returned an image became a text-only "Output was truncated" notice — the image was dropped with nothing pointing at where it went. Clipped media now leaves an explicit pointer to the originalfile_path, and small results that happen to sit in an over-budget batch are left alone instead of being rewritten and labelled truncated._slice_read_file_tmrebuilt the message from_extract_text_from_message, which returns text blocks only, so a media-only result collapsed to""plus a notice. It also appended that notice unconditionally, including to results that were never truncated.Two changes:
_is_worth_clippingskips results already smaller than the slice size. Rewriting those cannot shrink the batch: on the generic path the replacement is a large-tool-result stub with a head+tail preview, which is frequently longer than the result it replaces. Everything big enough to matter is still clipped, so the aggregate reduction the post-clip retry depends on is unchanged.On the aggregate guarantee: the narrower reading of "leave results untouched when possible" — clip largest-first and stop once the batch is back under the threshold — breaks
test_summarization_clips_read_file_batch_on_overflowand its siblings, because thekeep-derived threshold is not the model's real input limit, so stopping early leaves the retry still overflowing. Skipping only what is too small to shrink gets the "don't rewrite what wasn't the problem" behavior without weakening the reduction.New unit tests in
tests/unit_tests/middleware/test_overflow_clip.pycover the media pointer, small siblings surviving intact, no false truncation notice, and async parity. The existing end-to-end clip tests still pass unchanged.Not addressed here: on the generic (non-
read_file) path,_build_evicted_contentstill retains media blocks inline, so an image in some other tool's result won't shrink either. That helper is shared with the archive/eviction path where retaining media is deliberate (#3990), so changing it is a separate decision rather than something to fold into this fix.