fix: ensure 1M context window is respected in auto-condensing for Bedrock #9868
Conversation
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
…rock - Update Task.ts to use current API handler model info instead of cached values - This ensures the 1M context window setting is properly applied when calculating auto-condensing thresholds - Add tests to verify the fix works correctly Fixes #9831
Contributor
Author
Review completed. The PR correctly addresses the 1M context window issue by ensuring Issues Found
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
Comment on lines
+363
to
436
| it("should respect 1M context window in auto-condensing threshold calculations", () => { | ||
| // Test case 1: Without 1M context enabled | ||
| const handlerWithout1M = new AwsBedrockHandler({ | ||
| apiModelId: BEDROCK_1M_CONTEXT_MODEL_IDS[0], | ||
| awsAccessKey: "test", | ||
| awsSecretKey: "test", | ||
| awsRegion: "us-east-1", | ||
| awsBedrock1MContext: false, | ||
| }) | ||
|
|
||
| const modelWithout1M = handlerWithout1M.getModel() | ||
|
|
||
| // Should use default 200K context window | ||
| expect(modelWithout1M.info.contextWindow).toBe(200_000) | ||
|
|
||
| // With 100K tokens and 100% threshold, should trigger at 200K (100% of 200K) | ||
| const contextPercentWithout1M = (100 * 100_000) / modelWithout1M.info.contextWindow | ||
| expect(contextPercentWithout1M).toBe(50) // 100K is 50% of 200K | ||
|
|
||
| // Test case 2: With 1M context enabled | ||
| const handlerWith1M = new AwsBedrockHandler({ | ||
| apiModelId: BEDROCK_1M_CONTEXT_MODEL_IDS[0], | ||
| awsAccessKey: "test", | ||
| awsSecretKey: "test", | ||
| awsRegion: "us-east-1", | ||
| awsBedrock1MContext: true, | ||
| }) | ||
|
|
||
| const modelWith1M = handlerWith1M.getModel() | ||
|
|
||
| // Should use 1M context window | ||
| expect(modelWith1M.info.contextWindow).toBe(1_000_000) | ||
|
|
||
| // With 100K tokens and 100% threshold, should trigger at 1M (100% of 1M) | ||
| const contextPercentWith1M = (100 * 100_000) / modelWith1M.info.contextWindow | ||
| expect(contextPercentWith1M).toBe(10) // 100K is 10% of 1M | ||
|
|
||
| // Verify that auto-condensing would NOT trigger at 100K tokens with 100% threshold | ||
| // when 1M context is enabled (since 100K is only 10% of 1M) | ||
| const autoCondenseThreshold = 100 // 100% threshold | ||
| expect(contextPercentWith1M < autoCondenseThreshold).toBe(true) | ||
|
|
||
| // But it WOULD trigger without 1M context at 50% threshold | ||
| const lowerThreshold = 50 | ||
| expect(contextPercentWithout1M >= lowerThreshold).toBe(true) | ||
| }) | ||
|
|
||
| it("should maintain 1M context window across API handler rebuilds", () => { | ||
| // This test ensures that when the API handler is rebuilt or accessed multiple times, | ||
| // the 1M context window setting is consistently applied | ||
|
|
||
| const handler = new AwsBedrockHandler({ | ||
| apiModelId: BEDROCK_1M_CONTEXT_MODEL_IDS[0], | ||
| awsAccessKey: "test", | ||
| awsSecretKey: "test", | ||
| awsRegion: "us-east-1", | ||
| awsBedrock1MContext: true, | ||
| }) | ||
|
|
||
| // Get model info multiple times to simulate what happens in Task.ts | ||
| const model1 = handler.getModel() | ||
| const model2 = handler.getModel() | ||
| const model3 = handler.getModel() | ||
|
|
||
| // All should consistently return 1M context window | ||
| expect(model1.info.contextWindow).toBe(1_000_000) | ||
| expect(model2.info.contextWindow).toBe(1_000_000) | ||
| expect(model3.info.contextWindow).toBe(1_000_000) | ||
|
|
||
| // Verify the model ID remains consistent | ||
| expect(model1.id).toBe(model2.id) | ||
| expect(model2.id).toBe(model3.id) | ||
| }) | ||
| }) |
Contributor
Author
There was a problem hiding this comment.
These tests about 1M context window calculations are placed in the "ARN parsing with inference profiles" describe block, but they should be moved to the existing "1M context beta feature" describe block (line 649) since they test the 1M context feature rather than ARN parsing. This would improve test organization and make these tests easier to find and maintain alongside the other 1M context tests.
Fix it with Roo Code or mention @roomote and request a fix.
Contributor
There was a problem hiding this comment.
@mrubens @hannesrudolph @ronyblum this implementation looks correct to me
hannesrudolph
added
the
Issue/PR - Triage
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.
This PR attempts to address Issue #9831.
Problem
When auto context condensing is enabled with the 1M token option for Claude Sonnet on Bedrock, the calculation logic was not respecting the extended 1M context window. Instead, context would get automatically condensed when crossing the base model's max token value (100K) rather than the 1M limit.
Root Cause
The issue was in
Task.tswhere themanageContextfunction was being called with a cached/localmodelInfovariable instead of getting the current model info from the API handler. The API handler correctly updates the context window to 1M when the option is enabled, but this wasn't being used in the condensing calculations.Solution
Updated two locations in
Task.ts:handleContextWindowExceededError()- Now usesthis.api.getModel()to get current model infoattemptApiRequest()- Now usesthis.api.getModel()to get current model infoThis ensures that when the 1M context option is enabled for Bedrock models, the auto-condensing logic correctly uses the 1M context window for its threshold calculations.
Testing
Feedback and guidance are welcome!