Skip to content

Commit fbbfca9

Browse files
fix(billing-cost-management-mcp-server): document COA account scope and criteria, fold restartNeeded criteria (#4904)
* fix(billing-cost-management-mcp-server): document COA account scope and criteria, fold restartNeeded criteria compute-optimizer-automation: - Lowercase the values of restartNeeded rule criteria in the preview operations. AWS compares restartNeeded against the strings "true" and "false", so StringEquals "True" silently matched nothing. - Describe the criteria shape, its fields and operator types, the restartNeeded case rule, and that an OrganizationRule needs organization_scope. - State that from a management account or delegated administrator the recommended-action and event list and summary operations include associated member accounts, and that summary rows carry no account ID. * docs(billing-cost-management-mcp-server): drop redundant COA criteria sentences Remove the operator-type, StringEquals case and restartNeeded lowercase sentences from the criteria paragraph, and the restartNeeded note from the criteria arg doc. The field list already pairs String* and Numeric* operators with their fields, and the tool lowercases restartNeeded values itself.
1 parent 5e26249 commit fbbfca9

3 files changed

Lines changed: 168 additions & 2 deletions

File tree

‎src/billing-cost-management-mcp-server/awslabs/billing_cost_management_mcp_server/tools/compute_optimizer_automation_operations.py‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1048,10 +1048,30 @@ def _rule_preview_request(
10481048
request_params['organizationScope'] = parsed_scope
10491049
parsed_criteria = parse_json(criteria, 'criteria')
10501050
if parsed_criteria:
1051-
request_params['criteria'] = parsed_criteria
1051+
request_params['criteria'] = _fold_restart_needed_values(parsed_criteria)
10521052
return request_params
10531053

10541054

1055+
def _fold_restart_needed_values(criteria: Any) -> Any:
1056+
"""Lowercase the string values of the restartNeeded criteria conditions.
1057+
1058+
AWS compares restartNeeded against the strings "true" and "false", so a StringEquals
1059+
"True" silently matches nothing. Any shape other than a list of condition objects
1060+
with a values list is returned unchanged for AWS to validate.
1061+
"""
1062+
if not isinstance(criteria, dict) or not isinstance(criteria.get('restartNeeded'), list):
1063+
return criteria
1064+
conditions = []
1065+
for condition in criteria['restartNeeded']:
1066+
if isinstance(condition, dict) and isinstance(condition.get('values'), list):
1067+
condition = {
1068+
**condition,
1069+
'values': [v.lower() if isinstance(v, str) else v for v in condition['values']],
1070+
}
1071+
conditions.append(condition)
1072+
return {**criteria, 'restartNeeded': conditions}
1073+
1074+
10551075
async def _collect_automation_rule_preview(
10561076
ctx: Context,
10571077
client: Any,

‎src/billing-cost-management-mcp-server/awslabs/billing_cost_management_mcp_server/tools/compute_optimizer_automation_tools.py‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -453,6 +453,13 @@ def _multi_region_guidance(
453453
list_accounts, list_automation_rules, list_tags_for_resource) accept at most one region —
454454
rules are global resources, so fanning them out would repeat identical data.
455455
456+
**Account scope:** Called from an organization's management account or a delegated
457+
administrator, list_recommended_actions, list_recommended_action_summaries,
458+
list_automation_events and list_automation_event_summaries also return data for the
459+
associated member accounts (see list_accounts), not just the calling account. Actions and
460+
events have an account_id; summary rows do not, so their counts and savings can cover several
461+
accounts. To report on one account, filter on AccountId.
462+
456463
Supported operations (pass via the `operation` parameter):
457464
458465
1. get_enrollment_configuration: Current Automation enrollment status for the account.
@@ -513,6 +520,14 @@ def _multi_region_guidance(
513520
ResourceTagsKey and ResourceTagsValue each search keys and values, so a key filter also
514521
matches tag values and the reverse. EBS volume types are lowercased for you.
515522
523+
Rule criteria (`criteria`, preview operations) is a JSON object string of the form
524+
{"<field>": [{"comparison": "<operator>", "values": [...]}]}. Text fields: `region`,
525+
`resourceArn`, `ebsVolumeType`, `restartNeeded`; they take String* operators (StringEquals,
526+
StringLike, StringEqualsIgnoreCase, ...). Number fields: `ebsVolumeSizeInGib`,
527+
`lookBackPeriodInDays`, `estimatedMonthlySavings`; they take Numeric* operators
528+
(NumericEquals, NumericGreaterThan, ...). `resourceTag` also takes a "key" and String*
529+
operators. An OrganizationRule needs organization_scope; the tool rejects one without it.
530+
516531
PAGINATION (list_* operations only; the get_* operations and list_tags_for_resource take
517532
neither max_items nor next_token):
518533
- max_items is the total number of items returned. If it is omitted, the tool fetches every page for """
@@ -576,7 +591,10 @@ async def compute_optimizer_automation(
576591
rule_type: Rule type for the preview operations ('OrganizationRule'/'AccountRule').
577592
recommended_action_types: JSON string array of action types for the preview operations.
578593
organization_scope: Optional JSON string {accountIds: [...]} for the preview operations.
579-
criteria: Optional JSON string of rule criteria conditions for the preview operations.
594+
Required when rule_type is OrganizationRule.
595+
criteria: Optional JSON object string of rule criteria for the preview operations:
596+
{"<field>": [{"comparison": "<operator>", "values": [...]}]}. See the tool
597+
description for the fields and operators.
580598
max_items: Total number of items to return across all pages (per region when
581599
regions is set). Omit it to fetch every page. List operations only.
582600
next_token: The pagination.next_token from a previous response; continue with

‎src/billing-cost-management-mcp-server/tests/tools/test_compute_optimizer_automation_tools.py‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,33 @@ async def test_tool_registered_with_name():
104104
assert tool.name == 'compute-optimizer-automation'
105105

106106

107+
async def test_description_documents_account_scope_and_value_formats():
108+
"""The description states the org scope and the criteria shape and fields."""
109+
tool = await compute_optimizer_automation_server.get_tool('compute-optimizer-automation')
110+
assert tool is not None
111+
description = ' '.join((tool.description or '').split())
112+
113+
# Management and delegated-admin callers see member accounts' data.
114+
assert '**Account scope:**' in description
115+
scope = description.split('**Account scope:**')[1].split('Supported operations')[0]
116+
assert 'member accounts' in scope
117+
assert 'AccountId' in scope
118+
# The criteria shape and its fields.
119+
assert '{"<field>": [{"comparison": "<operator>", "values": [...]}]}' in description
120+
for field in (
121+
'region',
122+
'resourceArn',
123+
'ebsVolumeType',
124+
'restartNeeded',
125+
'ebsVolumeSizeInGib',
126+
'lookBackPeriodInDays',
127+
'estimatedMonthlySavings',
128+
'resourceTag',
129+
):
130+
assert f'`{field}`' in description
131+
assert 'OrganizationRule needs organization_scope' in description
132+
133+
107134
def test_valid_operations_list():
108135
"""VALID_OPERATIONS covers exactly the 13 supported read operations."""
109136
assert len(VALID_OPERATIONS) == 13
@@ -743,6 +770,107 @@ async def test_preview_summaries_forward_scope_and_criteria(self, mock_ctx):
743770
'region': [{'comparison': 'StringEquals', 'values': ['us-east-1']}]
744771
}
745772

773+
@pytest.mark.parametrize(
774+
'operation', ['list_automation_rule_preview', 'list_automation_rule_preview_summaries']
775+
)
776+
async def test_restart_needed_values_folded_to_lowercase(self, mock_ctx, operation):
777+
"""The restartNeeded values are lowercased, since AWS matches only "true"/"false".
778+
779+
Other criteria fields keep their case.
780+
"""
781+
client = MagicMock()
782+
getattr(client, operation).return_value = {}
783+
criteria = {
784+
'restartNeeded': [
785+
{'comparison': 'StringEquals', 'values': ['False', 'TRUE']},
786+
{'comparison': 'StringNotLike', 'values': ['T*']},
787+
],
788+
'ebsVolumeType': [{'comparison': 'StringEquals', 'values': ['GP2']}],
789+
}
790+
791+
await getattr(ops, operation)(
792+
mock_ctx,
793+
client,
794+
rule_type='AccountRule',
795+
recommended_action_types='["UpgradeEbsVolumeType"]',
796+
criteria=json.dumps(criteria),
797+
)
798+
799+
_, kwargs = getattr(client, operation).call_args
800+
assert kwargs['criteria'] == {
801+
'restartNeeded': [
802+
{'comparison': 'StringEquals', 'values': ['false', 'true']},
803+
{'comparison': 'StringNotLike', 'values': ['t*']},
804+
],
805+
'ebsVolumeType': [{'comparison': 'StringEquals', 'values': ['GP2']}],
806+
}
807+
808+
async def test_restart_needed_folded_on_multi_region_tool_call(self, mock_ctx):
809+
"""Through the tool with regions, every region receives the lowercased criteria."""
810+
criteria = '{"restartNeeded": [{"comparison": "StringEquals", "values": ["True"]}]}'
811+
clients = {}
812+
813+
def make_client(region=None):
814+
client = MagicMock()
815+
client.list_automation_rule_preview.return_value = {'previewResults': []}
816+
clients[region] = client
817+
return client
818+
819+
with patch(
820+
f'{_TOOLS_MODULE}.create_compute_optimizer_automation_client', side_effect=make_client
821+
):
822+
result = await automation_fn(
823+
mock_ctx,
824+
operation='list_automation_rule_preview',
825+
regions=['us-east-1', 'eu-west-1'],
826+
rule_type='AccountRule',
827+
recommended_action_types='["UpgradeEbsVolumeType"]',
828+
criteria=criteria,
829+
)
830+
831+
assert result['status'] == STATUS_SUCCESS
832+
assert set(clients) == {'us-east-1', 'eu-west-1'}
833+
for client in clients.values():
834+
_, kwargs = client.list_automation_rule_preview.call_args
835+
assert kwargs['criteria'] == {
836+
'restartNeeded': [{'comparison': 'StringEquals', 'values': ['true']}]
837+
}
838+
839+
@pytest.mark.parametrize(
840+
'criteria, expected',
841+
[
842+
({'restartNeeded': 'True'}, {'restartNeeded': 'True'}),
843+
({'restartNeeded': ['True']}, {'restartNeeded': ['True']}),
844+
(
845+
{'restartNeeded': [{'comparison': 'StringEquals', 'values': 'True'}]},
846+
{'restartNeeded': [{'comparison': 'StringEquals', 'values': 'True'}]},
847+
),
848+
(
849+
{'restartNeeded': [{'comparison': 'StringEquals', 'values': [True, 'True']}]},
850+
{'restartNeeded': [{'comparison': 'StringEquals', 'values': [True, 'true']}]},
851+
),
852+
(['restartNeeded'], ['restartNeeded']),
853+
],
854+
)
855+
async def test_restart_needed_fold_leaves_other_shapes(self, mock_ctx, criteria, expected):
856+
"""Only string values in a well-formed condition are lowercased.
857+
858+
Any other shape reaches AWS as given, so AWS still reports its own error.
859+
"""
860+
client = MagicMock()
861+
client.list_automation_rule_preview.return_value = {}
862+
863+
await ops.list_automation_rule_preview(
864+
mock_ctx,
865+
client,
866+
rule_type='AccountRule',
867+
recommended_action_types='["UpgradeEbsVolumeType"]',
868+
criteria=json.dumps(criteria),
869+
)
870+
871+
_, kwargs = client.list_automation_rule_preview.call_args
872+
assert kwargs['criteria'] == expected
873+
746874

747875
@pytest.mark.asyncio
748876
class TestEventSummariesAndSteps:

0 commit comments

Comments
 (0)