Repository navigation
fix(agent): stub oversized history only on an empty summary - #133
Conversation
Compaction treated a truncated stub as the whole message. When earlier lines had already filled the 64 KiB summary source, a later message that cannot fit was cut to the leftover sliver and then dropped. Stub that line only on an empty source so this pass summarizes the prefix and the next pass keeps about 64 KiB of the oversized line. Consolidated from cursor/critical-bug-identification-d77c onto the post-promote development base: the only-empty rule replaces the half-source stub threshold from 0d4e380 (same if/branch), and the later-line regression now drives the oversized turn through several turns with a 50 KiB predecessor to assert the defer-then-full-stub sequence instead of a one-shot sliver stub. Supersedes #132.
… line The only-empty stub rule lets a pass defer a never-fit line to the next one, which works for a user line: the pass folds its summary into the kept line, the line lands at the head of the session and the next pass stubs it against a full 64 KiB source. An assistant line cannot take that path. The kept tail starts on it, a user summary gets prepended in front of it, and the next pass sees the same pair again - the wrapper always counts toward used, so no pass ever reaches the only-empty stub. The count then only grows until SESSION_JSON_MAX stops the saves and the stored session stays above max_context_messages forever. The new regression test reproduces the stall on the exact post-partial-pass state: without the fix the count stayed at 6..22 across eight turns. Fix shape: a partial pass that consumed exactly one line and breaks on a never-fit non-user line now folds its summary into that line instead of prepending a wrapper. The line keeps its content and role, the count shrinks, and the next pass starts on the line with an empty source and stubs it whole. Chosen over forcing a truncated stub at the stuck pass: that restores progress but re-creates the sliver this branch exists to prevent whenever the one consumed line was large; the fold always reaches the full stub. The wrapper variant (not counting the summary toward used) was rejected: the wrapper cannot be told from real content structurally and dropping it from the source would lose the accumulated summary. The full-buffer next-pass test now expects the count to shrink on the first partial pass (5 + 2 instead of 6 + 2): the fold consumes the head line together with the oversized line, where the old half-source threshold left the count unchanged for one pass. The full-buffer stub assertion itself is unchanged and still holds.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR changes session compaction in src/core/agent.c so a never-fit line is stubbed only against an empty summary source, and so a partial pass that stops on an oversized non-user line at index 1 folds its summary into that line instead of prepending a user wrapper. The logic is mostly sound and make test_agent passes here (built with gcc and the multiarch curl header path). But I reproduced a no-progress state that the PR description says cannot happen, in a narrow size band just below the never-fit threshold.
Must Fix
None strictly blocking. See the first Should Fix: the PR states "every compaction pass strictly decreases the message count", and that is false for one reachable input.
Should Fix
src/core/agent.c:284The fold only fires whensummary_line_never_fits()is true. An assistant line that fits a fresh source but not behind the previous head (the head is typically the summary, soused > 0) takes the old path:next_unsummarized = 1,merge == 0, summary prepended, count unchanged. The next pass sees[summary, bigA]again and repeats: one provider call per turn, count growing by 2 per turn. Repro: I changedOVERSIZEintest_compaction_progresses_when_assistant_message_is_oversized(tests/test_agent.c:1136) from65 * 1024to65500and gotFAIL: oversized assistant line never left the session or count stayed at 22, the same symptom the PR describes for the 65 KiB case. The band isLfrom about65536 - 12 - len("user: " + summary + " ")up to 65523, so it is a few hundred bytes wide for a typical summary and up to about 4 KiB for a max-size one. Narrow, but it ends in the same frozen session atSESSION_JSON_MAX. Fix: dropsummary_line_never_fits()from the fold condition, so any non-user line that stopped the pass at index 1 folds. The folded line is `summary + "
" + line`, so it is larger than the line and will take the never-fit stub next pass. Add a test at this size.
-
src/core/agent.c:284The fold is keyed oni == 1, which means "index 1", not "exactly one line consumed". If index 0 is not an object it is skipped,used == 0at index 1, and the stub path runs, so behavior is fine. But the comment says "directly behind the only line this pass consumed". State the real condition or track a consumed-line counter. -
src/core/agent.c:340After the fold, whenmsg_count - 1 == max_ctx, the first history message sent to the provider is anassistantturn (the folded line). Before this PR the tail always began with ausersummary.append_anthropic_messages(src/providers/anthropic.c:370) does not guard a leading assistant, and the repo already treats role alternation on Anthropic as a hard requirement (comment atagent.c:320). Please confirm against the Anthropic Messages API, or add a test that asserts the first history role sent to the provider. The summary is also now attributed to the assistant role; worth one line in the comment. -
The new comment blocks are long (14 lines at
agent.c:262-283) and narrate history ("would leave the count unchanged...", "the wrapper always counts toward used...") rather than the invariant. Keep the invariant ("a partial pass must shrink the count or defer to an empty-source stub") and leave the walk-through to the PR or CHANGELOG.compact_session_via_llmis also about 130 lines now, against the repo's 5 to 40 line target. Extracting "decide how to stop the pass" into a helper would make the two special cases (stub-on-empty, fold-on-index-1) testable on their own. -
tests/test_agent.cThe three oversized-line tests share about 80 lines of near-identical setup each (seed JSON, write config, loopagent_run, load session). A small helper would cut about 150 lines and make the band case above cheap to add.
Nice to Have
tests/test_agent.c:1207Thefor (i = 0; i < 8; i++)loop with earlybreakaccepts convergence on any turn up to 8. Asserting the expected turn (for example, shrunk by turn 3) would catch a regression that only slows convergence.- No test covers an oversized non-user line at
i >= 2(count shrinks via the normal prepend path) or atoolrole line. Both go through the same branch and are cheap to pin. - CHANGELOG: the earlier Unreleased bullet ("summarizes a message larger than the summary source as a truncated stub...") is now partly superseded by the two new bullets. Consider merging the three into one so the release note describes the final behavior.
Positive Highlights
- The empty-source-only stub is the right call. The previous half-buffer threshold could still delete an unread tail after a sliver stub, and
used == 0is simple to reason about.test_compaction_progresses_when_later_message_is_oversizednow checks the three things that matter on pass 1 (oversized line kept, predecessor summarized, summary present). - The PR description documents the failure it reproduced (count 6 to 22) and why the alternatives were rejected, and the regression test seeds the exact post-partial-pass state.
- Generalizing
fold_summary_into_usertofold_summary_into_messagereuses the existing fold code instead of adding a second merge path, and thefoldedguard still fails closed with-1when nothing was folded.
Sent by Cursor Automation: Code review
| * pass's summary into the line instead: the count shrinks | ||
| * and the next pass starts on the line with an empty source, | ||
| * so it gets the full 64 KiB stub (see the tail build). */ | ||
| if (i == 1 && strcmp(role, "user") != 0 && |
There was a problem hiding this comment.
Repro of a no-progress case: with OVERSIZE = 65500 in the assistant test (a line that fits a fresh source but not behind the summary line), summary_line_never_fits() is false, so this fold is skipped, the summary is prepended, and the count stays put every turn (22 after 8 turns). Dropping summary_line_never_fits(role, content) from this condition makes any non-user line that stopped the pass at index 1 fold, which is what the PR description promises ("every pass strictly decreases the count").
| if (merge && i == start) { | ||
| if (fold_summary_into_user(dup, summary_buf) != 0) { | ||
| if ((merge || fold_into_oversized) && i == start) { | ||
| if (fold_summary_into_message(dup, summary_buf) != 0) { |
There was a problem hiding this comment.
When the fold triggers and the window reaches the folded line, the first message sent to the provider is an assistant turn. Before this PR the tail always began with a user summary. append_anthropic_messages does not guard a leading assistant. Please confirm the API accepts it, or assert the first history role in a test.
A line that fits a fresh source but not the wrapped remainder behind the previous head used to miss the fold (it is not "never-fit"), take the old prepend path and freeze the session at the same count: every pass rebuilt [summary, bigA] and the count only grew until SESSION_JSON_MAX. The fold now fires for any non-user line that stops the pass at index 1, never-fit or not; regression pins the 65500-byte band (stalled red at count 14 after 4 turns, converges by turn 3 green).
The assistant regression now requires convergence within three turns instead of accepting any of eight, and a tool-role oversized line behind two consumed lines pins the normal prepend path: the first pass must shrink the count and the session must still converge like the assistant cases.
…avior The stub-only-empty, defer-whole and fold behaviors are one mechanism; the release note now reads as the resulting guarantee instead of three incremental fixes.
|
Review response — all items closed:
|
Summary
Consolidates Bugbot PR #132 as operator-authored commits, plus two follow-ups from this PR's own review round:
The compaction guarantee is the fold -> empty-source-stub chain: every pass either shrinks the stored count or advances an oversized line toward its full-buffer stub, and the band freeze at a constant count is closed by the fold (regression-pinned from both directions).
Testing
make test_agent— 26 tests, 0 failed (run twice)CI=true make test— all 46 test binaries passedmake static— passedSupersedes #132.