Repository navigation
fix(billing-cost-management-mcp-server): flag COH summaries that reach the 1,000-group limit - #4973
Open
mattkelsey wants to merge 3 commits into
Open
mattkelsey wants to merge 3 commits into
mattkelsey wants to merge 3 commits into
Conversation
…h the 1,000-group limit ListRecommendationSummaries returns at most 1,000 groups, the ones with the highest estimated savings, and no nextToken for the rest. Grouped by AccountId in an organization with more than 1,000 accounts that have recommendations, the tool fetched every page and reported complete_dataset: true, so callers presented the top 1,000 accounts as every account. - A result that holds 1,000 groups is now marked complete_dataset: false, with has_more: false, no next_token, and a warning: groups may be missing, estimated_total_deduped_savings may cover only the returned groups, and the caller should group by a dimension with fewer values for totals, or narrow filters (or, for rows and counts, use list_recommendations) to cover every group. total_recommendation_count is left out, as on any partial result. - With TagKey grouping the warning adds that NoTagKey may be missing, or may also count recommendations whose tag value was left out. - The tool description states the limit, that a larger max_items or a continuation cannot fetch past it, and that a continuation's last page is not marked, since it cannot count earlier pages. Results under 1,000 groups are unchanged.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4973 +/- ##
==========================================
- Coverage 93.79% 93.79% -0.01%
==========================================
Files 1080 1080
Lines 94586 94595 +9
Branches 15402 15404 +2
==========================================
+ Hits 88715 88723 +8
Misses 3498 3498
- Partials 2373 2374 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mattkelsey
commented
Oct 30, 2026
| ) | ||
| # ListRecommendationSummaries returns at most this many groups, the ones with the highest | ||
| # estimated savings, and no nextToken for the rest. | ||
| COH_SUMMARIES_MAX_GROUPS = 1000 |
Contributor
Author
There was a problem hiding this comment.
I don't love hard-coding this, but there is not a source we can consume it from.
In botocore model there is max page size, but that is not the same as max groups. No model exists for this
mattkelsey
marked this pull request as ready for review
October 30, 2026 21:19
mattkelsey
requested review from
Rahul-1404,
chittev,
johnwangwyx,
shsrams and
somsubhro
as code owners
October 30, 2026 21:19
mattkelsey
enabled auto-merge
October 30, 2026 21:19
mattkelsey
requested review from
a team,
Comusus,
Dinkie,
anniewn,
bhatia-di,
callnmm,
finklen-amazon,
junaidgirkar,
wangyuhere and
yasmeenh11
as code owners
October 30, 2026 21:19
pputra
reviewed
Oct 30, 2026
| - If the result has complete_dataset: false, it is partial: totals and counts undercount. Call again with a larger 'max_items', continue with 'next_token', or state that the result is partial. Never report the number of returned items as a total count. | ||
| - Fields from the first page only (summary totals such as estimated_total_deduped_savings, currency_code, metrics) are not repeated on continuation calls. | ||
| - list_recommendation_summaries returns total_recommendation_count (the sum of the groups' recommendation_count) only when every group was fetched; use it as the overall count instead of adding the groups. It is absent on a partial result, a continuation call, or when a group has no recommendation_count. | ||
| - list_recommendation_summaries returns at most 1,000 groups, those with the highest estimated savings, and no next_token for the rest, so a larger 'max_items' or a continuation cannot fetch them. A result of 1,000 groups is marked complete_dataset: false: groups (e.g. accounts) may be missing and estimated_total_deduped_savings may cover only the returned groups. For totals, group by a dimension with fewer values (such as Region). To cover every account, narrow 'filters' until fewer than 1,000 groups come back, or, for rows and counts, use list_recommendations with the same filters (its savings are not de-duplicated). A continuation call cannot see earlier pages, so its last page is not marked: when paging with 'max_items', 1,000 groups across all pages means the same. |
Contributor
There was a problem hiding this comment.
remove "for rows and counts, use list_recommendations with the same filters (its savings are not de-duplicated)"
Contributor
Author
There was a problem hiding this comment.
is there a different alternative we can give it?
added 2 commits
October 30, 2026 21:43
… COH group-limit guidance Review feedback: the tool description and the 1,000-group warning no longer suggest a way to fetch every group (narrowing filters or calling list_recommendations); no API returns every account well today. They state the limit, that the result may be missing groups and its total may cover only the returned groups, and that totals come from a grouping with fewer values.
…up-limit # Conflicts: # src/billing-cost-management-mcp-server/tests/tools/test_cost_optimization_hub_tools.py
wangyuhere
previously approved these changes
Oct 30, 2026
Contributor
Author
|
Had to rebase sorry @wangyuhere |
Contributor
|
no permission to approve, but LGTM |
wangyuhere
approved these changes
Oct 30, 2026
finklen-amazon
approved these changes
Nov 1, 2026
This branch has not been deployed
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
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.
Fixes
Summary
Changes
ListRecommendationSummariesreturns at most 1,000 groups, the ones with the highest estimated savings, and nonextTokenfor the rest. Withgroup_by=AccountIdin an organization where more than 1,000 accounts have recommendations,list_recommendation_summariesfetched every page and reportedcomplete_dataset: true. Callers then presented the top 1,000 accounts as every account, leaving out accounts that had matching recommendations.complete_dataset: false, withhas_more: false, nonext_token, and awarning. The warning says groups may be missing andestimated_total_deduped_savingsmay cover only the returned groups. It also says to group by a dimension with fewer values for totals. It does not suggest a way to fetch the missing groups.total_recommendation_countis left out, as on any partial result.TagKey:grouping, the warning adds thatNoTagKeymay be missing, or may also count recommendations whose tag value was left out.max_itemsnor a continuation can fetch past it. A continuation call can't count earlier pages, so its last page isn't marked; the description tells callers who page withmax_itemsthat 1,000 groups across all pages means the same.Results under 1,000 groups are unchanged. A result of exactly 1,000 groups can still be complete, so the warning says groups may be missing.
User experience
Before: a 1,000-group summaries result looked complete, so the agent answered "which accounts have X?" with exactly 1,000 accounts and no caveat.
After: the same result leads with
complete_dataset: falseand a warning. The agent says the list covers the 1,000 groups with the most savings, and takes totals from a coarser grouping such asRegion.Testing
max_items=1000, theTagKey:warning, and the served tool description. 4 are regression guards that pass on both: 999 groups stay complete withtotal_recommendation_count, 999TagKey:groups stay complete, a continuation is not flagged, and amax_itemspartial with a token keeps its own warning.ruff check,ruff format --checkandpyrightare clean.list_tools: the 5 new statements are present with the change and absent on the base, and the description suggests no way to fetch the missing groups.Checklist
Is this a breaking change? (Y/N) N
RFC issue number:
Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.