-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: prevent context condensing from triggering too early in new conversations #8159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ersations - Added MIN_TOKENS_FOR_PERCENTAGE_TRIGGER constant (1000 tokens) - Modified percentage-based condensing logic to require minimum token threshold - Prevents condensing on brand new conversations with low percentage thresholds - Added comprehensive tests for the new behavior Fixes #8158
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - everything looks backward but the bugs are still mine.
| * Minimum number of tokens required before percentage-based condensing can trigger. | ||
| * This prevents condensing from happening too early in new conversations. | ||
| */ | ||
| export const MIN_TOKENS_FOR_PERCENTAGE_TRIGGER = 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we consider making this threshold configurable in the future? While 1000 tokens works well as a default, some users might benefit from being able to adjust this based on their specific use cases.
| // Only trigger percentage-based condensing if we have enough context | ||
| // This prevents condensing from happening on brand new conversations | ||
| const shouldCondenseByPercentage = | ||
| contextPercent >= effectiveThreshold && prevContextTokens >= MIN_TOKENS_FOR_PERCENTAGE_TRIGGER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be helpful to add a debug log here when condensing is skipped due to the minimum threshold? Something like:
This could help with debugging and understanding why condensing isn't triggering in certain scenarios.
| currentProfileId: "default", | ||
| }) | ||
|
|
||
| // Verify summarizeConversation was not called even though percentage (0.9%) would exceed threshold (5%) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit: For consistency with other test comments, could this be "(0.9% of context window)" instead of just "(0.9%)"?
This PR attempts to address Issue #8158. Feedback and guidance are welcome.
Problem
Intelligent context condensing was triggering too early in brand new conversations when users had low percentage thresholds configured (e.g., 5%), causing a loop of "context condensed" messages immediately after the first user message.
Solution
Added a minimum token threshold (
MIN_TOKENS_FOR_PERCENTAGE_TRIGGER = 1000) that must be met before percentage-based condensing can trigger. This prevents condensing from happening on brand new conversations while maintaining the feature's effectiveness for longer conversations.Changes
MIN_TOKENS_FOR_PERCENTAGE_TRIGGERconstant (1000 tokens) insrc/core/sliding-window/index.tsTesting
Fixes #8158
Important
Introduces a minimum token threshold to prevent premature context condensing in new conversations, addressing Issue #8158.
MIN_TOKENS_FOR_PERCENTAGE_TRIGGER(1000 tokens) inindex.tsto prevent early context condensing in new conversations.truncateConversationIfNeeded()to require both context percentage and token count thresholds.sliding-window.spec.tsto verify condensing does not trigger below the token threshold and works correctly above it.This description was created by
for 395640c. You can customize this summary. It will automatically update as commits are pushed.