-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: respect autoCondenseContext setting in sliding window truncation #7956
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
- Fixed bug where context condensing was triggered even when autoCondenseContext was disabled - Modified truncateConversationIfNeeded to only use sliding window truncation when auto-condense is disabled - Updated handleContextWindowExceededError to respect user preference for auto-condense - Added comprehensive test coverage for the fix Fixes #7953
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 because apparently I trust no one, not even myself.
| cost = result.cost | ||
| // If summarization failed but we still need to reduce context, | ||
| // fall back to sliding window truncation | ||
| if (prevContextTokens > allowedTokens) { |
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.
When condensing fails and we fall back to sliding window truncation, should we log this failure for debugging purposes? Currently it silently falls back which might make troubleshooting harder.
Consider adding:
| if (prevContextTokens > allowedTokens) { | |
| if (result.error) { | |
| console.warn(`[Task#${taskId}] Condensing failed: ${result.error}. Falling back to sliding window truncation.`); | |
| error = result.error | |
| cost = result.cost |
| } | ||
| } | ||
| } else { | ||
| // When auto-condense is disabled, only perform sliding window truncation |
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.
This comment could be more specific about what 'absolutely must' means. Consider:
| // When auto-condense is disabled, only perform sliding window truncation | |
| // When auto-condense is disabled, use sliding window truncation only | |
| // when tokens exceed the hard limit (context window minus buffer and reserved tokens) |
| private async handleContextWindowExceededError(): Promise<void> { | ||
| const state = await this.providerRef.deref()?.getState() | ||
| const { profileThresholds = {} } = state ?? {} | ||
| const { profileThresholds = {}, autoCondenseContext = true } = state ?? {} |
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.
For consistency with how it's done elsewhere in the codebase, consider destructuring with a default value:
| const { profileThresholds = {}, autoCondenseContext = true } = state ?? {} | |
| const { profileThresholds = {}, autoCondenseContext = true } = state ?? {} |
This matches the pattern used in line 2514 of the same file.
| expect(result.cost).toBe(mockCost) | ||
|
|
||
| summarizeSpy.mockRestore() | ||
| }) |
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.
Consider adding a test case that simulates condensing failure to ensure the fallback to sliding window truncation works correctly when autoCondenseContext is true but condensing fails. This would test the error handling path in lines 159-167 of index.ts.
This PR fixes issue #7953 where context condensing was triggered even when the
autoCondenseContextsetting was disabled.Problem
Users who have disabled auto-condense (
autoCondenseContext: false) were still experiencing "Context condensed" events during normal use. This occurred when conversations approached or exceeded the model's context limit, causing unexpected summarization and loss of conversation details despite the user's explicit preference.Solution
Modified the sliding window truncation logic to properly respect the
autoCondenseContextsetting:Fixed
truncateConversationIfNeededinsrc/core/sliding-window/index.ts:Fixed
handleContextWindowExceededErrorinsrc/core/task/Task.ts:autoCondenseContext: trueTesting
src/core/sliding-window/__tests__/auto-condense-disabled.spec.tsautoCondenseContextis falseReview Results
Code review confidence: 95% - Implementation is sound and ready for production.
Fixes #7953
Important
Fixes issue #7953 by ensuring
autoCondenseContextsetting is respected in context truncation logic.truncateConversationIfNeededinindex.tsto respectautoCondenseContextsetting, using sliding window truncation when disabled.handleContextWindowExceededErrorinTask.tsto pass user preference forautoCondenseContext.auto-condense-disabled.spec.tsto verify no condensing whenautoCondenseContextis false, sliding window truncation as fallback, and user preferences respected.This description was created by
for 64a90b9. You can customize this summary. It will automatically update as commits are pushed.