-
Notifications
You must be signed in to change notification settings - Fork 2.5k
A couple more sonnet 4.5 fixes #8421
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
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.
I found some issues that need attention. See inline comments for details.
| it("should enable 1M context window when awsBedrock1MContext is true for Claude Sonnet 4", () => { | ||
| const handler = new AwsBedrockHandler({ | ||
| apiModelId: BEDROCK_CLAUDE_SONNET_4_MODEL_ID, | ||
| apiModelId: BEDROCK_1M_CONTEXT_MODEL_IDS[0], |
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.
[P1] Tests rely on BEDROCK_1M_CONTEXT_MODEL_IDS[0]. Suggest adding a test case for the 4.5 entry (index 1) or refactoring tests to avoid index assumptions so we won't regress if the array order changes.
| const apiConfiguration: ProviderSettings = { | ||
| apiProvider: "bedrock", | ||
| apiModelId: BEDROCK_CLAUDE_SONNET_4_MODEL_ID, | ||
| apiModelId: BEDROCK_1M_CONTEXT_MODEL_IDS[0], |
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.
[P1] Similar to the Bedrock tests, this relies on BEDROCK_1M_CONTEXT_MODEL_IDS[0]. Consider adding a corresponding test for the 4.5 model or removing reliance on array order in tests.
| if ( | ||
| provider === "anthropic" && | ||
| id === "claude-sonnet-4-20250514" && | ||
| (id === "claude-sonnet-4-20250514" || id === "claude-sonnet-4-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.
[P2] Robustness: selecting tiers?.[0] assumes the 1M tier is always first. Prefer selecting the highest-contextWindow tier to avoid order-dependence.
Important
Refactor Claude Sonnet 4 model handling to use
BEDROCK_1M_CONTEXT_MODEL_IDSfor 1M context window support.BEDROCK_CLAUDE_SONNET_4_MODEL_IDconstant frombedrock.ts.AwsBedrockHandlerinbedrock.tsto useBEDROCK_1M_CONTEXT_MODEL_IDS[0]for 1M context window logic.getSelectedModel()inuseSelectedModel.tsto handle 1M context for Claude Sonnet 4 usingBEDROCK_1M_CONTEXT_MODEL_IDS.bedrock.spec.tsto useBEDROCK_1M_CONTEXT_MODEL_IDS[0]instead ofBEDROCK_CLAUDE_SONNET_4_MODEL_ID.useSelectedModel.spec.tsto reflect changes in model ID handling for 1M context.This description was created by
for e056367. You can customize this summary. It will automatically update as commits are pushed.