-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: Handle AWS Bedrock 20-image conversation limit (#6348) #6349
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
- Add image limiting functionality to prevent "too many images" errors - Automatically remove oldest images when conversation exceeds 20 images - Preserve most recent 20 images for Browser tool continuity - Add comprehensive error handling for image limit errors - Include extensive test coverage for image limiting logic Fixes #6348
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.
Code Review Summary
This PR effectively addresses the AWS Bedrock 20-image conversation limit issue with a well-structured solution. The implementation is solid with excellent test coverage and proper integration.
Important Suggestions (Should Consider)
1. Deep copy implementation could be more robust
Location: https://github.com/RooCodeInc/Roo-Code/blob/fix/aws-bedrock-image-limit-6348/src/api/transform/image-limiting.ts#L53-L56
The current shallow copy approach may not handle nested objects properly if the Anthropic SDK types evolve to include more complex nested structures. Consider using a deep cloning utility or JSON.parse(JSON.stringify()) for more robust copying.
2. Missing JSDoc parameter documentation
Location: https://github.com/RooCodeInc/Roo-Code/blob/fix/aws-bedrock-image-limit-6348/src/api/transform/image-limiting.ts#L28-L36
While the function has good documentation, adding @param tags would improve IDE support and developer experience:
3. Error handling enhancement opportunity
Location: https://github.com/RooCodeInc/Roo-Code/blob/fix/aws-bedrock-image-limit-6348/src/api/providers/bedrock.ts#L710-L720
Could we add more specific logging when image limiting is applied, such as the original vs final image count? This would help with debugging and monitoring.
Minor Improvements (Nice to Have)
4. Test file path comment correction
Location: https://github.com/RooCodeInc/Roo-Code/blob/fix/aws-bedrock-image-limit-6348/src/api/providers/__tests__/bedrock-image-limiting.spec.ts#L1
The vitest run command path doesn't match the actual file location. Should be:
5. Placeholder text specificity
Location: https://github.com/RooCodeInc/Roo-Code/blob/fix/aws-bedrock-image-limit-6348/src/api/transform/image-limiting.ts#L70
The placeholder text assumes 'Browser tool screenshot' but images could come from other sources. Consider a more generic message like '[Image removed due to conversation limit]'.
Positive Observations
- Excellent test coverage with 23 comprehensive test cases
- Clean separation of concerns with dedicated image-limiting module
- Proper integration into existing Bedrock handler
- Maintains backward compatibility
- Handles edge cases well (exactly 20 images, mixed content types, etc.)
- Good error handling with specific 'TOO_MANY_IMAGES' error type
- Addresses the reported issue effectively
Overall, this is a high-quality implementation that solves the problem elegantly. The suggestions above are minor improvements that would enhance robustness and maintainability.
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.
Code Review Summary
This PR effectively addresses the AWS Bedrock 20-image conversation limit issue with a well-structured solution. The implementation is solid with excellent test coverage and proper integration.
Important Suggestions (Should Consider)
1. Deep copy implementation could be more robust
Location: https://github.com/RooCodeInc/Roo-Code/blob/fix/aws-bedrock-image-limit-6348/src/api/transform/image-limiting.ts#L53-L56
The current shallow copy approach may not handle nested objects properly if the Anthropic SDK types evolve to include more complex nested structures. Consider using a deep cloning utility or JSON.parse(JSON.stringify()) for more robust copying.
2. Missing JSDoc parameter documentation
Location: https://github.com/RooCodeInc/Roo-Code/blob/fix/aws-bedrock-image-limit-6348/src/api/transform/image-limiting.ts#L28-L36
While the function has good documentation, adding @param tags would improve IDE support and developer experience.
3. Error handling enhancement opportunity
Location: https://github.com/RooCodeInc/Roo-Code/blob/fix/aws-bedrock-image-limit-6348/src/api/providers/bedrock.ts#L710-L720
Could we add more specific logging when image limiting is applied, such as the original vs final image count? This would help with debugging and monitoring.
Minor Improvements (Nice to Have)
4. Test file path comment correction
Location: https://github.com/RooCodeInc/Roo-Code/blob/fix/aws-bedrock-image-limit-6348/src/api/providers/__tests__/bedrock-image-limiting.spec.ts#L1
The vitest run command path does not match the actual file location.
5. Placeholder text specificity
Location: https://github.com/RooCodeInc/Roo-Code/blob/fix/aws-bedrock-image-limit-6348/src/api/transform/image-limiting.ts#L70
The placeholder text assumes Browser tool screenshot but images could come from other sources. Consider a more generic message.
Positive Observations
- Excellent test coverage with 23 comprehensive test cases
- Clean separation of concerns with dedicated image-limiting module
- Proper integration into existing Bedrock handler
- Maintains backward compatibility
- Handles edge cases well (exactly 20 images, mixed content types, etc.)
- Good error handling with specific TOO_MANY_IMAGES error type
- Addresses the reported issue effectively
Overall, this is a high-quality implementation that solves the problem elegantly. The suggestions above are minor improvements that would enhance robustness and maintainability.
|
I feel like this should be handled in a simpler way, I don't see why we should send all the previous images when using the browser tool, maybe we should limit it to 10 images |
Fixes the AWS Bedrock "too many images" error when using Browser tool continuously.
Problem
When using the Browser tool continuously in AWS Bedrock, conversations accumulate more than 20 images, causing the API to return: "too many images and documents: 21 + 0 > 20"
Solution
Impact
Closes #6348
Important
This PR adds automatic image limiting to AWS Bedrock conversations, ensuring no more than 20 images are retained, with comprehensive test coverage.
AwsBedrockHandler.bedrock.ts.countImagesInConversation(),limitImagesInConversation(), andhasExceededImageLimit()inimage-limiting.ts.bedrock-image-limiting.spec.tsandimage-limiting.spec.tswith 23 test cases to cover image limiting functionality.AWS_BEDROCK_MAX_IMAGES_PER_CONVERSATIONas 20 inimage-limiting.ts.This description was created by
for 9eb8562. You can customize this summary. It will automatically update as commits are pushed.