Skip to content

fix(billing-cost-management-mcp-server): reject cost-explorer arguments an operation does not use - #4993

Open
finklen-amazon wants to merge 1 commit into
mainfrom
fix/bcm-ce-reject-unused-args
Open

finklen-amazon wants to merge 1 commit into
mainfrom
fix/bcm-ce-reject-unused-args

Conversation

@finklen-amazon

Copy link
Copy Markdown
Contributor

Summary

Changes

cost-explorer takes one argument list for all eight operations, but each handler sends only the arguments its API takes. Before this change, any other argument was dropped with no error. The result came back as success and read as if the argument had applied:

  • search_string on getCostAndUsage, getCostAndUsageWithResources, the forecasts and getSavingsPlansUtilization
  • tag_key on getCostCategories, and cost_category_name on getTagsOrValues
  • dimension and context outside getDimensionValues
  • max_items and next_token on the forecasts and getSavingsPlansUtilization (the description already said they "take neither")
  • metric on getCostAndUsage, which then returned UnblendedCost

What changes:

  • OPERATION_ARGUMENTS lists the arguments each operation sends.
  • Any other argument the caller sets now returns a validation_error before the AWS client is created.
    • The message names each such argument, says why the operation would drop it, and says what does what the caller asked for. For example, tag_key on getCostCategories points to getTagsOrValues with tag_key, and metric on getCostAndUsage points to metrics.
    • The hint is fixed text.
  • These are not treated as set: None, '', and empty JSON for group_by or metrics ([], null).
  • granularity and prediction_interval_level are not checked. An operation that does not list them has no periods or prediction interval to misread, and callers commonly pass granularity to every operation. The description says so.
  • The table replaces OPERATIONS_WITHOUT_GROUP_BY and the getSavingsPlansUtilization billing_view_arn check. Both rejections stay, and the table makes them general.
  • The description now says that an argument outside an operation's Required and Optional lists is rejected, not ignored.

Interaction with #4990: that PR adds a dimension_key argument. The two merge without textual conflicts. Whichever merges second must add dimension_key to the getDimensionValues entry in OPERATION_ARGUMENTS, plus a reason entry. Without that, dimension_key is rejected everywhere. test_every_argument_the_operation_does_not_list_is_checked and test_documented_arguments_match_what_each_operation_accepts fail until it is added. With it added (9 lines plus one test fixture line), the merged tree passes all cost-explorer tests.

User experience

Before:

  • {"operation": "getCostCategories", "tag_key": "Environment"} returned success with the cost category names, as if they were the values of the Environment tag.
  • {"operation": "getCostAndUsage", "metric": "AMORTIZED_COST"} returned UnblendedCost.

After: both return validation_error.

  • tag_key is not supported for getCostCategories: it selects the tag key whose values getTagsOrValues lists. To list them, use operation=getTagsOrValues with tag_key; ...
  • metric is not supported for getCostAndUsage: it is the single metric of getCostForecast and getUsageForecast. getCostAndUsage and getCostAndUsageWithResources take metrics instead (e.g. ["AmortizedCost"]).

Testing

  • Unit tests: 4566 passed, with 100% patch coverage of cost_explorer_tools.py. 129 new or rewritten tests cover:
    • each rejected (operation, argument) pair
    • the pointer in each message
    • several arguments in one error
    • every parameter being checked
    • unset and unchecked values
    • the accepted arguments still reaching their handler parameter
    • the documented Required/Optional lists matching the table
    • one call through the served FastMCP tool
  • Run against the main source, 62 of the 129 fail and 67 pass as regression guards.
  • One existing test that asserted the silent drop of next_token and max_items on getSavingsPlansUtilization now asserts the rejection instead.
  • Live checks: read-only, through the served tool over stdio.
    • On this branch: 15/15 pass.
    • On main: 5/15. The 10 that fail are this change's claims: each rejected case returned success on main with the argument ignored. The 5 that pass are controls: search_string, tag_key and cost_category_name still narrow where they apply, an empty argument is not rejected, and granularity stays unchecked.
  • The API claims were checked against the public API reference, the botocore model and live calls: which operations take SearchString, TagKey, CostCategoryName, Dimension, Context, Granularity, PredictionIntervalLevel, Metric/Metrics, paging and BillingViewArn.
    • The forecasts and getSavingsPlansUtilization have no page size or page token in the public API.
    • GetUsageForecast rejects AMORTIZED_COST, so the metrics message gives a separate example for each forecast.

Checklist

  • 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) N. Calls that pass an argument the operation does not use, and that used to succeed with the argument ignored, now return a validation_error naming it, the same way group_by is already rejected on those operations.

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.

…ts an operation does not use

cost-explorer takes one argument list for all eight operations, and each
handler sends only the arguments its API takes. Any other argument was
dropped with no error, so an unnarrowed result came back as success and read
as narrowed: search_string on getCostAndUsage, tag_key on getCostCategories,
cost_category_name on getTagsOrValues, dimension and context outside
getDimensionValues, max_items and next_token on the forecasts and
getSavingsPlansUtilization, and metric on getCostAndUsage (which then used
UnblendedCost).

OPERATION_ARGUMENTS now lists the arguments each operation sends. Any other
argument the caller sets returns a validation_error before the AWS client is
created, and the message names, per argument, the operation or argument that
does what the caller asked for. None, an empty string and empty JSON are not
set. granularity and prediction_interval_level are not checked: an operation
that does not list them has no periods or interval to misread, and callers
commonly pass granularity to every operation. The table replaces
OPERATIONS_WITHOUT_GROUP_BY and the getSavingsPlansUtilization
billing_view_arn check.

Behavior change: a call that passes such an argument, which used to succeed
with it ignored, now fails with a validation_error naming it.
@codecov

codecov Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.79%. Comparing base (69e2666) to head (8fea6eb).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4993   +/-   ##
=======================================
  Coverage   93.79%   93.79%           
=======================================
  Files        1080     1080           
  Lines       94712    94734   +22     
  Branches    15442    15444    +2     
=======================================
+ Hits        88833    88857   +24     
  Misses       3505     3505           
+ Partials     2374     2372    -2     

☔ 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.

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.

2 participants