Skip to content

fix(billing-cost-management-mcp-server)!: request COH savings percentage, coerce boolean filters, fold summaries group_by - #4906

Open
finklen-amazon wants to merge 7 commits into
awslabs:mainfrom
finklen-amazon:fix/bcm-coh-summary-filters
Open

finklen-amazon wants to merge 7 commits into
awslabs:mainfrom
finklen-amazon:fix/bcm-coh-summary-filters

Conversation

@finklen-amazon

@finklen-amazon finklen-amazon commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Fixes

Summary

Changes

Savings percentage (list_recommendation_summaries). The tool never sent metrics, so the service always answered metrics.savingsPercentage with the placeholder "Percentage not requested.", and the tool returned that string as metrics.savings_percentage. The request now sends metrics=['SavingsPercentage']. A percentage value ('12%', or the clamped '>=80%' / '<=2%') is returned as metrics.savings_percentage. When the service does not compute it, it puts an explanation in the same field; that text is now returned as metrics.savings_percentage_unavailable_reason. The service gives an explanation in three cases: savings estimation mode BeforeDiscounts, a filter other than accountIds, regions and tags, or a failed calculation. The description says what the value is: estimated deduplicated monthly savings as a percent of the last 30 days' net amortized cost, excluding credits and refunds (the service filters out the Credit and Refund record types), for the whole filtered result, not per group.

Boolean filters. restartNeeded / rollbackPossible given as "true" / "false" strings failed botocore parameter validation. They are now coerced to booleans. Other values pass through unchanged.

Summaries group_by. Fixed dimensions now match case-insensitively (resourcetype -> ResourceType). Underscores are not ignored, so the legacy RESOURCE_TYPE spelling is still rejected. The missing-group_by error now lists the same values as the invalid-group_by error, including TagKey:<tag key>. The unreachable 'RESOURCE_TYPE' fallback is removed.

Filter docs. DatabaseSavingsPlans support (folding and valid_filter_values) now comes from main (#4948). The description now also covers three more things:

  • The resourceIds, resourceArns and recommendationIds filters are listed with the other filter parameters as list_recommendations only (summaries reject them locally, since feat(billing-cost-management-mcp-server): add COH summaries total cou… #4934).
  • There is no per-service filter, so to cover a service you list each of its types.
  • Summary fields are the same on every page, including continuation calls. The description previously said they were not repeated; a live continuation call returns them.

No tool parameter is added, renamed, retyped or given a new default.

User experience

Before:

  • "metrics": {"savings_percentage": "Percentage not requested."} on every summaries call.
  • filters='{"restartNeeded": "true"}' gave a parameter validation error.
  • group_by='resourcetype' gave a validation error.

After:

  • "metrics": {"savings_percentage": "12%"}, or {"savings_percentage_unavailable_reason": "<service text>"} when the service does not compute a percentage.
  • The string boolean is accepted.
  • group_by='resourcetype' is sent as ResourceType; RESOURCE_TYPE is still rejected locally.

Testing

Run from src/billing-cost-management-mcp-server:

  • uv run --frozen pytest: 3515 passed (after the latest rebase onto main); patch coverage 21/21 added executable lines.
  • ruff format --check, ruff check and pyright on the changed files: clean.
  • pre-commit on the changed files: clean.

The new tests failed before the fix (16 failures) and pass after.

Live check (read-only): a harness started the server over stdio and read the description as served. It then made 4 list_recommendation_summaries calls and 2 list_recommendations calls (max_items=1).

  • On this change, 21/21 passed:
    • an unfiltered summary returns a percentage string;
    • an actionTypes filter returns the service's incompatible-filter reason, not a metric;
    • group_by='resourcetype' is accepted (an earlier revision folded underscores too; it now ignores letter case only);
    • summaries reject resourceIds;
    • restartNeeded: "false" is accepted;
    • DatabaseSavingsPlans is accepted as a filter value.
  • On the base commit, 5/21 passed. Those 5 are API facts both versions share; every fixed claim fails.
  • After the rebase onto main, the full harness passed 24/24 with 5 read-only calls. Since feat(billing-cost-management-mcp-server): add COH summaries total cou… #4934 the resourceIds check expects the tool's local validation_error. On current main, the description checks pass 1/13.

Merge conflict

After #4945 and #4946 merged, this branch conflicted with main in cost_optimization_hub_helpers.py, cost_optimization_hub_tools.py and tests/tools/test_cost_optimization_hub_tools.py. I merged main in (a merge commit, no rebase) and resolved it:

After the merge: 4271 tests pass, ruff and pyright are clean, and the description checks pass 14/14.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

  • I have reviewed the contributing guidelines
  • I have performed a self-review of this change
  • Changes have been tested
  • Changes are documented

Is this a breaking change? (Y/N) Y. When no percentage is computed, metrics.savings_percentage is now absent and the service's text is in metrics.savings_percentage_unavailable_reason. Before this change the field always held the "Percentage not requested." placeholder, never a percentage. This is recorded in CHANGELOG.md.

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

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.

@codecov

codecov Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.79%. Comparing base (c6951b9) to head (032b66d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4906   +/-   ##
=======================================
  Coverage   93.79%   93.79%           
=======================================
  Files        1080     1080           
  Lines       94586    94599   +13     
  Branches    15402    15404    +2     
=======================================
+ Hits        88715    88728   +13     
  Misses       3498     3498           
  Partials     2373     2373           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@finklen-amazon
finklen-amazon force-pushed the fix/bcm-coh-summary-filters branch from ddcefdf to 45a52eb Compare October 29, 2026 16:39
yasmeenh11
yasmeenh11 previously approved these changes Oct 29, 2026
…age, coerce boolean filters, fold summaries group_by

- list_recommendation_summaries now sends metrics=['SavingsPercentage'].
  Without it the service returns the placeholder "Percentage not
  requested." in metrics.savingsPercentage, which the tool passed through
  as a metric. A percentage value ('12%', '>=80%', '<=2%') is returned as
  metrics.savings_percentage; any other text the service puts in that
  field (before-discounts mode, unsupported filter, not calculable) is
  returned as metrics.savings_percentage_unavailable_reason. A metrics
  block without a string value gives no metrics key.
- restartNeeded / rollbackPossible filters given as "true"/"false"
  strings are coerced to booleans instead of failing botocore parameter
  validation.
- Summaries group_by fixed dimensions match case- and
  underscore-insensitively (resource_type -> ResourceType), like filter
  keys. The missing-group_by error lists the same values as the
  invalid-group_by error, including TagKey:<tag key>. The unreachable
  'RESOURCE_TYPE' fallback is removed.
- The description documents the savings percentage and its limits, the
  resourceIds / resourceArns / recommendationIds filters (list only;
  summaries reject them), that there is no
  per-service resource type filter, and that summary fields are the same
  on every page, including continuation calls (it said they were not
  repeated).

No tool parameter is added, renamed, retyped or given a new default.

BREAKING CHANGE: cost-optimization list_recommendation_summaries no
longer returns the service's explanation text in
metrics.savings_percentage. When no percentage is computed,
metrics.savings_percentage is absent and the text is in
metrics.savings_percentage_unavailable_reason. Before this change the
field always held the "Percentage not requested." placeholder.
… excludes credits and refunds

The service computes metrics.savingsPercentage against the last 30 days'
net amortized cost with the Credit and Refund record types filtered out.
Say so in the cost-optimization description and the CHANGELOG entry, so a
caller comparing it to Cost Explorer's net amortized cost with no
record-type filter knows why the figures differ.

Also say that total_recommendation_count is not one of the summary fields
repeated on every page, and drop the CHANGELOG clause about documenting
the ID filters, which main already covers.
wangyuhere
wangyuhere previously approved these changes Oct 30, 2026
yasmeenh11
yasmeenh11 previously approved these changes Oct 30, 2026
@finklen-amazon
finklen-amazon added this pull request to the merge queue Oct 30, 2026
@finklen-amazon
finklen-amazon removed this pull request from the merge queue due to a manual request Oct 30, 2026
@finklen-amazon
finklen-amazon added this pull request to the merge queue Oct 30, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Oct 30, 2026
… case-insensitively only

Fold only letter case in list_recommendation_summaries group_by, not
underscores, so the legacy upper-snake spelling RESOURCE_TYPE (and
resource_type) is rejected locally as an invalid dimension instead of
being sent as ResourceType. resourcetype and ACCOUNTID still resolve.
The description, the CHANGELOG entry and the tests say the same.
@finklen-amazon
finklen-amazon added this pull request to the merge queue Oct 30, 2026
@finklen-amazon
finklen-amazon dismissed stale reviews from yasmeenh11 and wangyuhere via 594a769 October 30, 2026 20:36
@scottschreckengaust
scottschreckengaust removed this pull request from the merge queue due to a manual request Oct 30, 2026
wangyuhere
wangyuhere previously approved these changes Oct 30, 2026
# Conflicts:
#	src/billing-cost-management-mcp-server/awslabs/billing_cost_management_mcp_server/tools/cost_optimization_hub_helpers.py
#	src/billing-cost-management-mcp-server/awslabs/billing_cost_management_mcp_server/tools/cost_optimization_hub_tools.py
#	src/billing-cost-management-mcp-server/tests/tools/test_cost_optimization_hub_tools.py

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

3 participants