fix: respect condensation percentage threshold when saving settings #6859
Conversation
- Fixed issue where context condensation was triggered on every action after saving settings - Condensation now only triggers when the percentage threshold is actually met - If tokens exceed hard limit but percentage is below threshold, sliding window truncation is used instead - Added detailed logging to help debug condensation triggers - Updated tests to reflect the new behavior This prevents unnecessary condensation operations that were happening even when the configured percentage threshold had not been reached.
dosubot
bot
added
size:M
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| // If we're over the hard limit but below percentage, we'll use sliding window instead | ||
| if (shouldCondenseByPercentage) { | ||
| console.log( | ||
| `[Condensation] Triggering automatic condensation: ${contextPercent.toFixed(1)}% >= ${effectiveThreshold}% threshold`, |
There was a problem hiding this comment.
I notice we're adding several console.log statements here for debugging condensation decisions. While these are helpful for understanding the flow, should we consider using a proper logging framework or removing these before merging to avoid cluttering production logs? We could potentially use the existing logging infrastructure that's already in place.
| // Check if we should trigger condensation based on percentage threshold | ||
| const shouldCondenseByPercentage = contextPercent >= effectiveThreshold | ||
| // Check if we're over the hard token limit | ||
| const isOverHardLimit = prevContextTokens > allowedTokens |
There was a problem hiding this comment.
The variable naming here is mostly clear, but I'm wondering if isOverHardLimit could be more descriptive? Perhaps isOverTokenLimit or exceedsAllowedTokens would be more consistent with the allowedTokens variable used elsewhere in the code?
| } | ||
| } | ||
|
|
||
| // Fall back to sliding window truncation if needed |
There was a problem hiding this comment.
This comment says "This happens when we're over the hard token limit regardless of percentage" but that's not entirely accurate - this code path is also reached when condensation fails (see line 173 where we check for result.error). Could we update this comment to reflect both scenarios?
| const summarizeSpy = vi | ||
| .spyOn(condenseModule, "summarizeConversation") | ||
| .mockResolvedValue(mockSummarizeResponse) | ||
| it("should NOT use summarizeConversation when tokens exceed hard limit but percentage is below threshold", async () => { |
There was a problem hiding this comment.
The test modifications look comprehensive! I'm wondering though - should we add a test case for when tokens are exactly at the percentage threshold (e.g., exactly 100% when threshold is 100%)? This would help ensure boundary conditions are properly handled.
hannesrudolph
added
the
Issue/PR - Triage
|
I don't think this PR does anything. |
Problem
When saving settings, the first action after the save would often trigger automatic context condensation even if the configured percentage threshold had not been met. This was happening because the condensation logic was triggering on either:
This meant that even with a high percentage threshold (e.g., 100%), condensation would still trigger if tokens exceeded the hard limit.
Solution
Modified the condensation trigger logic in
truncateConversationIfNeededto:Changes
src/core/sliding-window/index.tsto separate percentage-based condensation from hard limit handlingsrc/core/sliding-window/__tests__/sliding-window.spec.tsto reflect the new behaviorTesting
This fix ensures that the condensation percentage threshold setting is properly respected, preventing unnecessary condensation operations after settings saves.
Important
Modifies
truncateConversationIfNeededto respect percentage threshold for condensation and use sliding window truncation when tokens exceed hard limit but threshold is not met.truncateConversationIfNeededinindex.tsnow only triggers condensation when percentage threshold is met.sliding-window.spec.tsto verify new behavior.index.tsto separate percentage-based condensation from hard limit handling.This description was created by
for 6df165d. You can customize this summary. It will automatically update as commits are pushed.