fix(ci): align parity service aliases - #68
Draft
JesseKoldewijn wants to merge 21 commits into
Draft
Conversation
…gap-fills across 8 existing services New services: ECS, RDS, Cognito, ElastiCache, CloudTrail Deepened services: SES (+9 ops), Redshift (+8 ops), Route53 (+7 ops), OpenSearch (+8 ops), EC2 (+21 ops), SSM (+12 ops), STS (+2 ops) Adds ~120 operations total, bringing the supported service count from 24 to 29 (ECS, RDS, Cognito, ElastiCache, CloudTrail added to README). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Contributor
CI ResultsChecks
Parity
|
Points AI agents to AGENTS.md as the canonical context file. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
- cargo fmt --all: fix line-length formatting across cloudtrail, cognito, ecs, elasticache, and multiple test files - Add cloudtrail, cognito, ecs, elasticache, rds to service-matrix.json - Add probe entries for all 5 new services in all-services-smoke.json - Add guided manifest JSON for cloudtrail, cognito, ecs, elasticache, rds Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…py lint - parity.rs: add ecs, rds, cognito, elasticache, cloudtrail to all_service_names() and the README→service-id match block so the readme_supported_services_match_all_services_smoke_inventory test passes - studio-ui/explorer.rs: replace manual integer division with checked_div to fix the pre-existing `manual checked division` clippy lint Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Add the remaining API Gateway, CloudFormation, and Step Functions lifecycle perf benchmarks from the coverage plan. Sync the Dockerfile manifest cache layer with the expanded workspace and fix a DynamoDB clippy lint so CI image builds and clippy pass again.
Contributor
Benchmark Gate: ✅ PASSProfile: Memory Footprint
ACM
APIGATEWAY
CLOUDFORMATION
CLOUDWATCH
DYNAMODB
EC2
ECR
EVENTBRIDGE
FIREHOSE
IAM
KINESIS
KMS
LAMBDA
OPENSEARCH
REDSHIFT
ROUTE53
S3S3 — 1MB
S3 — 10MB
S3 — 50MB
S3 — 100MB
SECRETSMANAGER
SES
SNS
SQS
SSM
STEPFUNCTIONS
STS
Overall Performance
|
Normalize service alias handling across config, gateway, protocol selection, and the integration harness so parity probes route consistently. Record LocalStack free-tier gaps for cloudtrail, cognito, ecs, elasticache, and rds to unblock all-services parity.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (23)
📒 Files selected for processing (73)
📝 WalkthroughWalkthroughAdds new service crates, canonicalizes service names, wires optional provider registration, expands multiple service providers and stores, adds integration/performance coverage, and updates workflow guards and S3 write behavior. ChangesAWS services expansion and wiring
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Align protocol selection with gateway service alias handling and recognize CloudTrail target variants used by guided flows. Add regression tests for Cognito alias routing and CloudTrail target parsing.
JesseKoldewijn
changed the title
Apply rustfmt's import ordering in gateway tests and add a per-operation benchmark threshold for opensearch/list_domain_names to match observed CI variance. The PR title was updated separately to satisfy the conventional title gate.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 57
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/services/s3/src/object_store.rs (1)
213-218: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUpdate the doc comment to reflect the Linux 50 MiB pre-allocation threshold.
The doc comment states that pre-allocation happens "when size is known", but after the change at line 610, Linux only pre-allocates for objects >= 50 MiB. The current documentation is misleading for callers trying to understand the performance characteristics of this method.
📝 Proposed documentation update
/// Write object data from a reader (async), streaming to disk. /// /// All objects use [`write_via_copy`]: an async read loop with a 512 KiB /// `BufReader` feeding an adaptive `BufWriter` (2–8 MiB depending on - /// `content_length`). Pre-allocates the file with `set_len` when size - /// is known to avoid block-level fragmentation. + /// `content_length`). On Linux, pre-allocates the file with `fallocate` + /// for known sizes >= 50 MiB to avoid block-level fragmentation. On other + /// platforms, pre-allocates with `set_len` when size is known and > 0. /// /// Returns `(final_path, bytes_written)`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/s3/src/object_store.rs` around lines 213 - 218, Update the doc comment for the async writer (the block describing write_via_copy) to note that pre-allocation via set_len is conditional on platform and size: on Linux the code only pre-allocates for objects >= 50 MiB; otherwise pre-allocation is skipped. Edit the comment that currently says "when size is known" to explicitly mention the 50 MiB Linux threshold and that behavior may differ on other platforms.crates/services/opensearch/src/store.rs (1)
19-51:⚠️ Potential issue | 🟠 MajorAdd parity assertion coverage for
ServiceSoftwareOptions
ServiceSoftwareOptionsis already exercised incrates/services/opensearch/src/provider.rsand covered by unit tests (UpdateStatus/Cancellableassertions incrates/services/opensearch/tests/opensearch_tests.rs).- The parity scenario currently hit (
tests/parity/scenarios/all-services-smoke.json) only runsopensearch list-domain-namesand has no assertions/captured JSON validatingServiceSoftwareOptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/opensearch/src/store.rs` around lines 19 - 51, Add parity assertion coverage for ServiceSoftwareOptions by ensuring the parity test captures and asserts its fields: update the parity scenario or test harness that produces the JSON for opensearch domains so that service_software_options (fields current_version, new_version, update_available, cancellable, update_status, description, automated_update_date) are emitted and validated; modify the parity scenario (tests/parity/scenarios/all-services-smoke.json) or the parity capture logic in the opensearch provider code paths (see ServiceSoftwareOptions struct and Default impl in store.rs and usages in provider.rs) to include these fields in the recorded output, and add assertions in the parity test suite (similar to the unit assertions in tests/opensearch_tests.rs) that verify expected values/types for those ServiceSoftwareOptions properties.Source: Coding guidelines
crates/services/rds/tests/rds_perf_tests.rs (1)
1-131:⚠️ Potential issue | 🟠 MajorAdd missing perf coverage for RDS provider operations
crates/services/rds/src/provider.rsdispatches 15 operations, butcrates/services/rds/tests/rds_perf_tests.rsonly has perf tests forCreateDBInstance,DescribeDBInstances, andCreateDBSnapshot. Add perf test entries (with timing assertions) for the remaining operations:CreateDBParameterGroup,CreateDBSubnetGroup,DeleteDBInstance,DeleteDBParameterGroup,DeleteDBSnapshot,DeleteDBSubnetGroup,DescribeDBParameterGroups,DescribeDBSnapshots,DescribeDBSubnetGroups,ModifyDBInstance,RebootDBInstance,RestoreDBInstanceFromDBSnapshot.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/rds/tests/rds_perf_tests.rs` around lines 1 - 131, Tests are missing perf coverage for the other RDS operations; add new tokio::test functions that mirror the existing patterns (use RdsProvider::new(), make_ctx, body_str) to exercise the remaining operations and assert status_code == 200 and timing bounds. Specifically add tests named perf_create_db_parameter_group, perf_create_db_subnet_group, perf_delete_db_instance, perf_delete_db_parameter_group, perf_delete_db_snapshot, perf_delete_db_subnet_group, perf_describe_db_parameter_groups, perf_describe_db_snapshots, perf_describe_db_subnet_groups, perf_modify_db_instance, perf_reboot_db_instance, and perf_restore_db_instance_from_snapshot; for mutating ops use n=100 loop with unique identifiers and assert elapsed.as_millis() < 2000, for describe ops perform the query after creating sample resources and assert elapsed.as_millis() < 500, reusing make_ctx and body_str helpers and creating any prerequisite resources (e.g., DB instances/snapshots/subnet/parameter groups) before the measured loop.Source: Coding guidelines
crates/services/ec2/src/store.rs (1)
73-159:⚠️ Potential issue | 🟠 MajorEC2 store: add parity coverage for new KeyPair/Address/InternetGateway/Volume resources
- Unit tests in
crates/services/ec2/tests/ec2_tests.rscover KeyPair, Elastic IP (Allocate/Associate/Release/Describe), InternetGateway (create/attach/detach/delete/describe), and Volume (create/attach/delete/describe).- No parity scenarios under
tests/parity/scenarios/matchCreateKeyPair,AllocateAddress,CreateInternetGateway, orCreateVolume, so the “at least one parity scenario” requirement for these store changes is not met.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/ec2/src/store.rs` around lines 73 - 159, The new store types KeyPair, Address, InternetGateway, and Volume were added (in structs KeyPair, Address, InternetGateway, Volume and aggregated in Ec2Store) but there are no parity scenarios exercising CreateKeyPair, AllocateAddress, CreateInternetGateway, or CreateVolume under tests/parity/scenarios; add at least one parity scenario file for each resource mapping to the corresponding API flows (CreateKeyPair, AllocateAddress, CreateInternetGateway, CreateVolume) so the parity test suite covers those new store changes and references the same resource IDs/fields used by the unit tests in crates/services/ec2/tests/ec2_tests.rs.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/benchmark.yml:
- Line 211: The workflow uses mutable action tags like actions/checkout@v4 (and
other occurrences flagged) which should be pinned to specific commit SHAs;
update each unpinned uses: replace the tag (e.g., actions/checkout@v4) with the
corresponding commit SHA for that action, keeping an inline comment with the
human-friendly tag (e.g., "# v4") for clarity, and ensure you pin every other
unpinned action occurrence flagged in the file so all actions are referenced by
exact commit SHAs.
- Around line 262-276: The checkout step using actions/checkout@v4 currently
lacks persist-credentials: false; update the checkout step (the job step that
uses actions/checkout@v4 and sets ref: ${{ needs.check-trigger.outputs.pr_sha ||
github.sha }}) to include persist-credentials: false so credentials are not
persisted to the workspace, matching the change applied at lines 210-213.
- Around line 210-213: Update the checkout step that uses actions/checkout@v4 to
explicitly set persist-credentials: false so Git credentials are not left in the
workspace; locate the checkout block (the step using actions/checkout@v4 and the
ref: ${{ needs.check-trigger.outputs.pr_sha || github.sha }}) and add the
persist-credentials: false input to that step.
In `@crates/aws-protocol/src/protocol.rs`:
- Line 21: The canonical_service_name function is case-sensitive and only
special-cases "es" and "cognito*" so mixed-case inputs like "SQS" fall through
and get misclassified as RestJson; fix it by normalizing the incoming service
identifier to lowercase at the start of canonical_service_name (e.g., let s =
service.to_ascii_lowercase()) and then perform the existing matches against that
normalized string (handle starts_with checks for "cognito" and exact matches for
services such as "es", "sqs", "s3", etc.), ensuring the match branches that
currently return RestJson only apply to true unknowns after the lowercase
comparison.
In `@crates/config/src/services.rs`:
- Around line 53-55: The provider override suffix is being canonicalized to use
underscores (via canonical_service_name) but lookups expect hyphenated service
names (e.g., "cognito-idp"), so change the mapping to normalize to the
hyphenated form: after calling canonical_service_name(service) convert any
underscores to hyphens (e.g., .map(|service|
(canonical_service_name(service).replace('_', "-"), value))). This ensures keys
produced by key.strip_prefix("PROVIDER_OVERRIDE_") match lookup names used
elsewhere.
In `@crates/services/cloudtrail/src/provider.rs`:
- Around line 38-60: The json_error function in provider.rs bypasses the service
error module; replace the inline JSON error construction by using a
CloudTrail-specific error type in the error module and its helper constructor
(mirror the CognitoError/ElastiCacheError pattern). Add CloudTrail error
variants and a helper (e.g., CloudTrailError::new or
CloudTrailError::to_dispatch_response) to
crates/services/cloudtrail/src/error.rs, then update provider.rs to call that
helper from places that currently call json_error (and keep json_ok for success
responses). Ensure the resulting return type is DispatchResponse and that
content_type/headers are produced by the new error helper.
- Around line 187-188: Extract the repeated ARN-to-key parsing into a small
helper, e.g., fn extract_key_from_name(name: &str) -> String, placed near the
top of the module (next to other helper functions). Replace each occurrence of
name.rsplit('/').next().unwrap_or(&name).to_string() in provider.rs (the places
where a local variable key is being computed) with a call to
extract_key_from_name(name). Ensure the helper handles both ARN and non-ARN
inputs and returns the same String result so callers like the key assignments in
the provider logic remain unchanged.
- Line 12: Remove the unused uuid import and the suppression helper: delete the
`use uuid::Uuid;` import and the `_use_uuid()` function (the dummy function that
references Uuid at the bottom of the file) so there is no unused-import
workaround; ensure no other references to `Uuid` remain in provider.rs before
committing.
- Around line 101-584: Add explicit parity scenarios for CloudTrail covering
each operation handled in provider.rs—CreateTrail, DeleteTrail, DescribeTrails,
GetTrail, GetTrailStatus, StartLogging, StopLogging, UpdateTrail, LookupEvents,
AddTags, ListTags, and RemoveTags—into the parity scenario sets (core.json and
extended.json). For each scenario, invoke the corresponding CLI/API action that
maps to the operation string (e.g., cloudtrail create-trail → "CreateTrail",
cloudtrail delete-trail → "DeleteTrail", cloudtrail describe-trails →
"DescribeTrails", etc.), include any necessary setup/teardown steps (create a
trail before testing GetTrail/StartLogging/UpdateTrail/DeleteTrail and add tags
before ListTags/RemoveTags), assert the expected responses (presence/absence of
trail, logging_enabled toggles, tags present), and ensure the scenarios exercise
the code paths used by trail_to_json, trail_arn creation,
StartLogging/StopLogging mutating logging_enabled, LookupEvents returning
Events, and the AddTags/ListTags/RemoveTags tag map manipulation so parity tests
cover all twelve operations.
In `@crates/services/cloudtrail/src/store.rs`:
- Around line 55-56: The events Vec on the store (pub events:
Vec<CloudTrailEvent>) is unbounded despite the comment "last N events"; update
the provider code paths that append to store.events (the method(s) that
create/persist events used by LookupEvents) to enforce a fixed capacity eviction
policy: add a MAX_EVENTS constant and after pushing a new CloudTrailEvent, trim
the oldest entries so store.events.len() <= MAX_EVENTS (e.g., drain the front
excess). Ensure the constant is documented and used consistently where events
are mutated so LookupEvents reads a bounded collection.
In `@crates/services/cloudtrail/tests/cloudtrail_perf_tests.rs`:
- Around line 1-91: Add performance tests for the missing CloudTrail operations
by modeling new async #[tokio::test] functions after the existing
perf_create_trail_throughput and perf_describe_trails_many tests: create tests
for DeleteTrail, GetTrail, GetTrailStatus, StartLogging, StopLogging,
UpdateTrail, LookupEvents, AddTags, ListTags, and RemoveTags that instantiate
CloudTrailProvider::new(), use make_ctx(...) to build appropriate RequestContext
bodies, call p.dispatch(...).await.unwrap(), assert resp.status_code == 200 (or
expected code) and check relevant response bodies with body_json(&resp) where
applicable, and include throughput assertions similar to the existing
elapsed.as_millis() checks to satisfy perf thresholds.
In `@crates/services/cloudtrail/tests/cloudtrail_tests.rs`:
- Around line 1-363: Add tests that cover the missing error paths by using
CloudTrailProvider::new() and dispatching the relevant operations via make_ctx
with non-existent or invalid resource identifiers and asserting the proper error
status and __type: specifically add a GetTrailStatus not-found test (e.g.,
test_get_trail_status_not_found) that dispatches "GetTrailStatus" for a missing
trail and expects 404 + TrailNotFoundException, StartLogging and StopLogging
not-found tests (test_start_logging_not_found, test_stop_logging_not_found) that
dispatch "StartLogging"/"StopLogging" for a missing trail and expect error
status + TrailNotFoundException, an UpdateTrail not-found test
(test_update_trail_not_found) that dispatches "UpdateTrail" for a missing trail
and expects error + TrailNotFoundException, an AddTags invalid-resource test
(test_add_tags_invalid_resource) that calls "AddTags" with a bogus ResourceId
and asserts an error type for invalid resource, and tests for RemoveTags (both
happy and error path, e.g., test_remove_tags and test_remove_tags_not_found)
that dispatch "RemoveTags" for an existing trail and for a non-existent resource
respectively, asserting correct success and error responses; use make_ctx to
build request bodies and body_json to validate __type and status_code.
In `@crates/services/cognito/src/provider.rs`:
- Around line 133-893: The parity and perf coverage for Cognito is incomplete:
add parity scenarios for all missing operations (CreateUserPool, DeleteUserPool,
DescribeUserPool, ListUserPools, UpdateUserPool, CreateUserPoolClient,
DeleteUserPoolClient, DescribeUserPoolClient, ListUserPoolClients,
AdminCreateUser, AdminDeleteUser, AdminGetUser, ListUsers, AdminSetUserPassword,
AdminEnableUser, AdminDisableUser, AdminUpdateUserAttributes, AdminInitiateAuth)
into tests/parity/scenarios/core.json and tests/parity/scenarios/extended.json
so the test runner exercises the corresponding operation strings handled in
provider.rs (e.g., "CreateUserPool", "AdminCreateUser", "AdminInitiateAuth",
etc.), and extend crates/services/cognito/tests/cognito_perf_tests.rs to add
timing/throughput assertions for each of those operations (mirroring existing
tests for CreateUserPool, AdminCreateUser, ListUsers) by issuing the same client
calls the provider implements and asserting expected latency/throughput
thresholds. Ensure scenario entries use the correct request shape/parameters to
hit the branches in provider.rs and that perf tests cover success and relevant
error states where applicable.
- Around line 133-893: Add the missing error-path unit tests, parity scenarios,
and perf coverage for the Cognito operations: create unit tests in
crates/services/cognito/tests/cognito_tests.rs that exercise the failure
branches for the match arms "CreateUserPool", "DeleteUserPool", "ListUserPools",
"UpdateUserPool", "CreateUserPoolClient", "DeleteUserPoolClient",
"DescribeUserPoolClient", "ListUserPoolClients", "AdminDeleteUser",
"AdminSetUserPassword", "AdminEnableUser", "AdminDisableUser", and
"AdminUpdateUserAttributes" (e.g., missing params -> InvalidParameterException,
non-existent pool/client/user ->
ResourceNotFoundException/UsernameExistsException as appropriate) by calling the
provider dispatch logic in provider.rs; add parity scenarios for these
operations into tests/parity/scenarios/core.json and
tests/parity/scenarios/extended.json describing request input and expected error
responses so parity runner covers them; and extend
crates/services/cognito/tests/cognito_perf_tests.rs to add perf measurements
(using std::time::Instant like existing tests) for the omitted operations
(include CreateUserPoolClient, DeleteUserPoolClient, AdminDeleteUser,
AdminSetUserPassword, AdminEnableUser/AdminDisableUser,
AdminUpdateUserAttributes, ListUserPoolClients) to ensure regression coverage —
keep tests deterministic and reuse existing helpers to create
pools/clients/users where needed.
In `@crates/services/cognito/src/store.rs`:
- Around line 38-39: Rename the misspelled struct fields callback_ur_ls and
logout_ur_ls to callback_urls and logout_urls respectively; update all
references (constructors, pattern matches, field access, tests, Serde
attributes) to use the new names, and if needed add #[serde(rename =
"callback_ur_ls")] / #[serde(rename = "logout_ur_ls")] temporarily to preserve
backward compatibility during de-serialization until callers are updated.
In `@crates/services/ec2/src/provider.rs`:
- Around line 270-287: Add the missing unit and perf tests and expand parity
scenarios: add unit tests in crates/services/ec2/tests/ec2_tests.rs that
exercise AuthorizeSecurityGroupEgress and DeleteTags covering both happy paths
and error paths (e.g., validate parameter errors and expected xml_error
responses), add perf benchmarks in crates/services/ec2/tests/ec2_perf_tests.rs
using std::time::Instant for DeleteSubnet, DeleteSecurityGroup, StopInstances,
StartInstances, etc., measuring and asserting elapsed time thresholds, and
update tests/parity/scenarios/ to include dedicated scenario files that exercise
each modified operation against both OpenStack and LocalStack (ensuring handlers
like the DeleteSubnet branch that uses store.subnets.remove and returns
xml_ok/xml_error are actually hit).
In `@crates/services/ec2/tests/ec2_tests.rs`:
- Around line 550-569: Add a failing-path test that attempts to ReleaseAddress
with a non-existent AllocationId to exercise the error path: extend or add a
test near test_release_address that calls p.dispatch(&make_ctx("ReleaseAddress",
rel)) where rel contains a made-up AllocationId (not returned by
AllocateAddress), assert the response is an error (non-200) and/or contains the
expected error message/code, and verify DescribeAddresses still does not contain
that fake allocation; reference test_release_address, Ec2Provider::dispatch,
make_ctx, "ReleaseAddress", and "DescribeAddresses" to locate and implement the
new test.
- Around line 397-424: Extend the existing test_stop_and_start_instances test
suite by adding separate test cases (or additional assertions) that call
p.dispatch(&make_ctx("StopInstances", params)) and
p.dispatch(&make_ctx("StartInstances", params)) with a non-existent InstanceId
(e.g., "i-doesnotexist") to exercise the error paths; verify the provider
returns the appropriate error HTTP status and error body (assert status_code !=
200 and that body_str contains the expected error code/message for
InstanceNotFound or similar). Use the same Ec2Provider::new, dispatch, make_ctx
helpers and mirror how run/start/stop responses are inspected (body_str,
status_code) so the tests fail if the provider doesn't return the expected error
behavior.
- Around line 366-391: Add a new test that exercises the error path for
DeleteSecurityGroup by attempting to delete a non-existent GroupId: create a new
async test (e.g., test_delete_security_group_not_found) that instantiates
Ec2Provider::new(), calls p.dispatch(&make_ctx("DeleteSecurityGroup", params))
with a bogus GroupId (not created earlier) and then asserts the provider returns
the expected error outcome (e.g., response status_code != 200 and body_str
contains the AWS-style error like "InvalidGroup.NotFound" or that dispatch
returned an Err). Reuse make_ctx, body_str, and Ec2Provider::dispatch as in
test_delete_security_group and keep assertions specific to DeleteSecurityGroup's
error signal.
- Around line 718-737: Add a new test function (e.g.,
test_delete_volume_not_found) alongside test_delete_volume that constructs an
Ec2Provider via Ec2Provider::new(), calls p.dispatch(&make_ctx("DeleteVolume",
del)) with a HashMap where "VolumeId" is a non-existent id (e.g.,
"vol-00000000"), awaits the result, and asserts the error path: check
resp.status_code is not 200 and that body_str(&resp) contains the expected error
identifier (e.g., "InvalidVolume.NotFound" or other service-specific not-found
string); also keep the existing DescribeVolumes check separate — reference
functions/methods: test_delete_volume, Ec2Provider::new, p.dispatch, make_ctx,
"DeleteVolume", and body_str.
- Around line 807-813: The extract_tag function currently uses unwrap_or(0)
which hides missing tags and produces incorrect slice bounds; update extract_tag
to explicitly handle absent tags by checking xml.find(&open) and
xml.find(&close) for None and returning a clear failure (preferably
Option<String> returning None for missing tags or a Result with an error)
instead of computing invalid start/end; compute start = pos_open + open.len()
and end = pos_close, ensure end >= start, then return
Some(xml[start..end].to_string()) (or Err) so callers can fail fast; reference
the function name extract_tag and the variables open, close, start, end when
making the change.
In `@crates/services/ecs/src/provider.rs`:
- Around line 165-885: The test suite is missing coverage for many ECS
operations (notably DescribeServices), lacks error-path assertions per
operation, misses perf timing checks for several operations, and lacks
per-operation parity scenarios; update tests to add unit/integration tests in
crates/services/ecs/tests/ecs_tests.rs to include a happy-path and at least one
error-path assertion (expecting 400) for each operation string (e.g.,
"DescribeServices", "CreateService", "UpdateService", "DeleteService",
"RunTask", "StopTask", "DescribeTasks", "ListTasks", etc.), add
timing/regression assertions in crates/services/ecs/tests/ecs_perf_tests.rs for
the operations currently missing perf checks (add Instant-based assertion blocks
similar to existing CreateCluster/ListClusters/RegisterTaskDefinition/RunTask
checks), and add parity scenario JSONs under tests/parity/scenarios/* (core.json
or extended.json) to exercise each operation against both OpenStack and
LocalStack so parity runs cover per-operation flows; ensure tests exercise the
provider code paths that handle operation strings in provider.rs (match arm
names like "DescribeServices", "ListServices", "CreateService", "DeleteService",
"UpdateService", "RunTask", "StopTask", "DescribeTasks", "ListTasks",
"RegisterTaskDefinition", "DeregisterTaskDefinition", "DescribeTaskDefinition",
"ListTaskDefinitions") and include assertions for expected responses and status
codes.
- Around line 1-13: The file declares a local json_error helper and doesn't
import the crate's centralized error helpers from the service error module;
replace usage of the inline json_error with the project's standard error
constructors by adding a use crate::error (or the appropriate error module name
exported in lib.rs) and update all places that call json_error to return the
service's error wrapper functions (e.g., the error module's functions/structs
for formatting DispatchError/ResponseBody) so error responses follow the
established pattern; locate references to json_error and any direct
DispatchError/ResponseBody constructions in provider.rs and swap them to the
error module helpers.
In `@crates/services/ecs/src/store.rs`:
- Around line 88-100: EcsStore's five HashMaps (clusters, task_definitions,
task_def_revisions, services, tasks) are unbounded and need eviction; add a
bounded/TTL cleanup strategy by introducing constants (e.g. MAX_CLUSTERS,
MAX_SERVICES, MAX_TASKS or TTL_MS) and implement a method on EcsStore like
prune_evicted(&mut self) or ensure_capacity_on_insert(&mut self, key: &str) that
is called from all insertion/update paths (where
Cluster/Service/Task/TaskDefinition are created) to evict oldest/stale entries
(LRU, FIFO, or age-based) or trim to the max size; also consider replacing
HashMap with an LRU cache type for specific maps (or maintain an auxiliary
VecDeque/timestamps map to track insertion times) and update
serialize/deserialize logic to preserve behavior.
In `@crates/services/elasticache/src/provider.rs`:
- Line 304: The Delete/Modify/Reboot handlers (DeleteCacheCluster,
ModifyCacheCluster, RebootCacheCluster, DeleteReplicationGroup,
ModifyReplicationGroup, DeleteCacheSubnetGroup) currently call
self.store.get_or_create(account_id, region) which creates and leaks empty
ElastiCacheStore entries when the targeted resource is missing; change those
calls to self.store.get(account_id, region) and explicitly handle the None case
by returning NotFound without creating or persisting a store entry. Update the
code paths inside the referenced functions (e.g., the blocks around the
get_or_create call in DeleteCacheCluster, ModifyCacheCluster,
RebootCacheCluster, DeleteReplicationGroup, ModifyReplicationGroup,
DeleteCacheSubnetGroup) to check for None, perform no store mutation or
creation, and ensure any cleanup logic is applied so empty account/region stores
are not left behind.
In `@crates/services/elasticache/tests/elasticache_tests.rs`:
- Around line 185-208: Add a negative test for ModifyCacheCluster: create a new
async test (e.g., test_modify_cache_cluster_not_found) that constructs an
ElastiCacheProvider, dispatches a ModifyCacheCluster request with CacheClusterId
set to a non-existent id (e.g., "nonexistent") and a CacheNodeType, then assert
the response status_code is 400 and that body_str(&resp) contains
"CacheClusterNotFound"; this complements the existing happy-path
test_modify_cache_cluster and verifies the provider correctly returns the
not-found error.
- Around line 210-229: Add a negative test for the RebootCacheCluster operation:
create a new async test (e.g., test_reboot_cache_cluster_not_found) that
instantiates ElastiCacheProvider, calls svc.dispatch with action
"RebootCacheCluster" and CacheClusterId set to a nonexistent id, awaits the
response, and asserts that resp.status_code == 400 and body_str(&resp) contains
"CacheClusterNotFound"; place this alongside the existing
test_reboot_cache_cluster to ensure the error path for RebootCacheCluster is
covered.
- Around line 402-426: Add a test that exercises the error path for
DeleteCacheSubnetGroup by dispatching a DeleteCacheSubnetGroup request for a
non-existent group name (e.g., "nonexistent-sg") using the same
ElastiCacheProvider::dispatch and make_ctx helpers; assert the response
indicates failure (error status code and/or expected error code/message in
body_str) rather than 200, and keep it alongside test_delete_cache_subnet_group
to ensure the negative-case behavior of DeleteCacheSubnetGroup is covered.
- Around line 335-365: Add a negative test that covers the primary error path
for ModifyReplicationGroup by dispatching a ModifyReplicationGroup request
against a non-existent ReplicationGroupId and asserting the service returns the
expected error response (e.g., non-200 status, proper error code/message like
"ReplicationGroupNotFound" or similar). In the test file add a tokio::test
(similar to test_modify_replication_group) that creates
ElastiCacheProvider::new(), calls
svc.dispatch(&make_ctx("ModifyReplicationGroup",
[p("ReplicationGroupId","no-such"),
p("ReplicationGroupDescription","x")].into())).await.unwrap() (or handle the
Result) and then assert on resp.status_code and the body_str(resp) contains the
expected error element/string; use the same helpers (make_ctx, p, body_str) so
the test mirrors the existing pattern.
- Around line 235-265: The test test_create_and_describe_replication_group only
covers the success path; add an error-path test that attempts to create a
duplicate replication group (call ElastiCacheProvider::new() and dispatch
CreateReplicationGroup twice using make_ctx with the same "ReplicationGroupId"
like "my-rg"), then assert the second response is an error (non-200 status_code)
and its body contains the expected duplicate error text (mirror the duplicate
cache cluster test pattern: check error code/message for an already-existing
replication group). Ensure you use the same helper functions (make_ctx, p,
body_str, and ElastiCacheProvider::dispatch) so the new assertions align with
existing tests.
- Around line 371-400: Add an error-path test that attempts to create the same
CacheSubnetGroup twice using ElastiCacheProvider::dispatch with the
CreateCacheSubnetGroup parameters (reuse test_create_and_describe_subnet_group
setup), then assert the second response is an error: verify non-200 status or
that the body contains the ElastiCache error identifier (e.g.,
"CacheSubnetGroupAlreadyExistsFault" or an appropriate error message) and a
descriptive message; ensure the test name reflects duplicate creation (e.g.,
test_create_subnet_group_duplicate_error) and uses the same request shape
(CacheSubnetGroupName, SubnetIds.*) so it exercises the duplicate-path in
CreateCacheSubnetGroup handling.
In `@crates/services/opensearch/src/provider.rs`:
- Around line 270-293: Add missing perf, parity, and error-path tests for the
new OpenSearch operations: add perf_* benchmarks in
crates/services/opensearch/tests/opensearch_perf_tests.rs for DescribeDomains,
AddTags, RemoveTags, ListTags, GetCompatibleVersions, ListVersions,
StartServiceSoftwareUpdate, and CancelServiceSoftwareUpdate; add corresponding
service:"opensearch" entries in tests/parity/scenarios/core.json and the
extended.json parity scenarios to cover DescribeDomains, GetCompatibleVersions,
ListVersions, StartServiceSoftwareUpdate, CancelServiceSoftwareUpdate and the
tag operations (AddTagsToStream, ListTagsForStream, RemoveTagsFromStream) using
the non-stream API names; and add unit tests in
crates/services/opensearch/tests/opensearch_tests.rs that exercise error paths
(e.g., DescribeDomains when store missing or domain not found, returning
appropriate empty list or 404/ResourceNotFoundException depending on API
contract; AddTags/RemoveTags/ListTags when domain missing -> 404
ResourceNotFoundException; GetCompatibleVersions/ListVersions when target
domain/state invalid -> test expected error responses; and a
CancelServiceSoftwareUpdate error test similar to
test_start_service_software_update_domain_not_found) — reference existing test
helper patterns and functions (store.get, domain_status_json, json_ok, and
test_start_service_software_update_domain_not_found) to implement these tests
consistently.
In `@crates/services/opensearch/src/store.rs`:
- Around line 61-62: DeleteDomain currently removes the domain from
store.domains but leaves stale entries in OpenSearchStore.tags; update the
DeleteDomain handler (the function/method that removes domains) to also remove
the tags entry for that domain (e.g., call store.tags.remove(&d.arn) or
equivalent using the domain ARN) so tags are cleaned up on deletion, and add a
unit test that creates a domain, calls AddTags, deletes the domain via
DeleteDomain, then asserts ListTags returns an empty TagList for that domain to
prevent regressions.
In `@crates/services/opensearch/tests/opensearch_tests.rs`:
- Around line 490-632: Add tests that exercise the error paths for tag ops:
create new async tests (e.g., test_add_tags_error_invalid_arn and
test_remove_tags_error_nonexistent_domain) that use OpenSearchProvider::new(),
call p.dispatch(&make_ctx(...)) with the "AddTags" and "RemoveTags" actions and
invalid inputs (bad ARN format for AddTags and an ARN that doesn't correspond to
any created domain for RemoveTags), then assert non-200 status_code and that
body_json(&resp) contains the expected error structure/code/message; reuse
make_ctx, dispatch, and body_json helpers and mirror the existing happy-path
tests (test_add_and_list_tags, test_remove_tags) for locating where to add them.
In `@crates/services/rds/src/provider.rs`:
- Around line 428-446: The code currently uses unwrap_or_else on
store.instances.get(&db_id) to supply hardcoded defaults for (engine,
engine_version, allocated_storage, master_username) which masks a missing DB;
change this so snapshot creation fails when the instance is not found: replace
the unwrap_or_else branch with an explicit match on store.instances.get(&db_id)
and return an Err(...) (or propagate a suitable error) if None, including the
db_id in the message, otherwise extract the tuple from the found db; update the
function's return path to propagate that error instead of proceeding with
defaults.
- Around line 208-766: Add comprehensive parity, unit, and perf coverage for all
RDS operations: extend the parity scenarios to include rds actions (at minimum
CreateDBInstance, DeleteDBInstance, DescribeDBInstances, ModifyDBInstance,
RebootDBInstance, CreateDBSnapshot, DeleteDBSnapshot, DescribeDBSnapshots,
RestoreDBInstanceFromDBSnapshot, CreateDBSubnetGroup, DeleteDBSubnetGroup,
DescribeDBSubnetGroups, CreateDBParameterGroup, DeleteDBParameterGroup,
DescribeDBParameterGroups); update the unit test module rds_tests.rs to add
happy-path tests and at least one error-path test per operation (use the
existing patterns for DBInstanceAlreadyExists/DBInstanceNotFound to assert
xml_error responses and instance_xml/snapshot_xml outputs for successful flows,
exercising functions that handle these ops in provider.rs); and expand the perf
test module rds_perf_tests.rs to add Instant perf/regression tests for the
missing operations (CreateDBInstance, DescribeDBInstances, CreateDBSnapshot
already present—add tests for Delete*, ModifyDBInstance, RebootDBInstance,
RestoreDBInstanceFromDBSnapshot, subnet/parameter group create/delete/describe)
using the same test harness and request sequences as existing perf tests.
In `@crates/services/rds/tests/rds_tests.rs`:
- Around line 323-356: Add a negative test case to cover the error path for
CreateDBSubnetGroup by duplicating the call inside the existing
test_create_and_describe_subnet_group flow (or as a new #[tokio::test]) that
calls RdsProvider::dispatch with the same "CreateDBSubnetGroup" parameters a
second time and asserts a non-200 response or that the error body contains the
expected duplicate-name error; locate the existing
test_create_and_describe_subnet_group, reuse the same params map used for the
first CreateDBSubnetGroup call, invoke
p.dispatch(&make_ctx("CreateDBSubnetGroup", params)) again, await unwrap_err or
check resp.status_code != 200 and assert the returned error message matches the
duplicate-subnet-group condition.
- Around line 411-435: Add a new test case that exercises the error path for
DeleteDBParameterGroup by calling RdsProvider::dispatch via make_ctx with a
DBParameterGroupName that was never created (e.g., "pg-missing") and assert the
provider returns the expected error response (non-200 status_code and/or error
body indicating the parameter group does not exist). Implement this alongside
the existing test_delete_parameter_group (or as a separate async #[tokio::test])
and use DescribeDBParameterGroups or the response body to verify the correct
error handling for DeleteDBParameterGroup.
- Around line 388-409: Add an error-path test to
test_create_and_describe_parameter_group that attempts to create the same
DBParameterGroup twice: use RdsProvider::new(), call dispatch with
make_ctx("CreateDBParameterGroup", params) to create "my-pg" as before, then
call dispatch again with the same params and assert the response indicates
failure (non-200 status_code) and the body contains a duplicate-group error
message (e.g., mentions "DBParameterGroupAlreadyExists" or similar); ensure you
reference the same parameter map and use the existing helper functions
(dispatch, make_ctx, body_str) to validate the error path.
- Around line 182-202: Add a negative-path test for RebootDBInstance: create a
new async test (e.g., test_reboot_db_instance_not_found) that calls
RdsProvider::dispatch with make_ctx("RebootDBInstance", params) where
"DBInstanceIdentifier" is a non-existent id (do not create it first), await the
response and assert the status_code indicates an error (e.g., 404) and the
response body contains the expected error structure/message (e.g.,
"DBInstanceNotFound" or "ErrorResponse"). Use the same helpers (make_ctx,
body_str) and ensure you cover the primary error path for RebootDBInstance.
- Around line 221-248: Extend test_create_and_describe_snapshot to cover
CreateDBSnapshot error paths: add a test case that calls
p.dispatch(make_ctx("CreateDBSnapshot", params)) twice with the same
DBSnapshotIdentifier ("snap-001") and assert the second response returns an
error status (non-200) and contains an appropriate duplicate-snapshot error
message; also add a test where you call CreateDBSnapshot with a
DBSnapshotIdentifier but with DBInstanceIdentifier set to a non-existent
instance (e.g., "no-such-db") and assert the response is an error with the
expected not-found message; use the same RdsProvider instance, make_ctx helper
and body_str() to inspect responses and assert status_code and body contents.
- Around line 277-317: Add a new async test (e.g.,
test_restore_db_instance_from_nonexistent_snapshot) alongside
test_restore_db_instance_from_snapshot that calls RdsProvider::dispatch with
action "RestoreDBInstanceFromDBSnapshot" using a valid DBInstanceIdentifier but
a non-existent DBSnapshotIdentifier (e.g., "no-such-snap"); assert the provider
returns the error path (non-200 status or the specific error XML/message your
provider uses) and that the body contains the expected error code/text about the
missing snapshot. Use the same make_ctx helper and HashMap param construction
pattern as in test_restore_db_instance_from_snapshot to locate where to add the
test and to build the request for RestoreDBInstanceFromDBSnapshot.
- Around line 250-275: Add a test covering the error path for DeleteDBSnapshot
by creating a new async test (similar to test_delete_snapshot) that uses
RdsProvider::new() and calls dispatch with a DeleteDBSnapshot request for a
non-existent DBSnapshotIdentifier (e.g., "no-such-snap"); assert that the
response indicates failure (non-200 or contains the expected error/exception
information) and verify via DescribeDBSnapshots that the snapshot is not
present; place the test alongside test_delete_snapshot and reference the same
request name strings "DeleteDBSnapshot" and "DescribeDBSnapshots" when
constructing the contexts.
- Around line 156-180: Add a test that covers the primary error path for
ModifyDBInstance by asserting the provider returns the expected error when the
target DB instance does not exist: create a new async test (similar to
test_modify_db_instance) that calls RdsProvider::dispatch with
make_ctx("ModifyDBInstance", params) where "DBInstanceIdentifier" is a
non-existent name, then assert the response status_code and body contain the
not-found error (e.g., ResourceNotFound or equivalent message); reference
test_modify_db_instance, RdsProvider::dispatch, make_ctx, and the
"ModifyDBInstance" operation when implementing.
- Around line 358-382: Add a negative test covering the error path for
DeleteDBSubnetGroup by creating a new async test (e.g.,
test_delete_subnet_group_not_found) that uses RdsProvider::new() and calls
p.dispatch(&make_ctx("DeleteDBSubnetGroup", params)) where params contains a
non-existent DBSubnetGroupName; assert the response represents the expected
error (non-200 status and/or error body indicating not found) instead of a
successful 200, similar to how test_delete_subnet_group asserts success, so the
DeleteDBSubnetGroup error path is covered.
In `@crates/services/redshift/src/provider.rs`:
- Around line 364-424: The repo is missing performance regression tests for the
11 new Redshift operations; add timing-based tests to
crates/services/redshift/tests/redshift_perf_tests.rs that exercise each API
operation (CreateClusterSnapshot, DeleteClusterSnapshot,
DescribeClusterSnapshots, CreateClusterSubnetGroup, DeleteClusterSubnetGroup,
DescribeClusterSubnetGroups, CreateClusterParameterGroup,
DeleteClusterParameterGroup, DescribeClusterParameterGroups, EnableLogging,
DisableLogging) by invoking the provider endpoints (the handler that routes on
the operation name in provider.rs) and asserting latency using
std::time::Instant (measure start/elapsed and fail if above an acceptable
threshold), create one test per operation, reuse existing test helpers for
request construction and account/region setup, and name tests clearly (e.g.,
perf_create_cluster_snapshot) so they run with cargo test.
- Around line 364-719: The parity test suite lacks scenarios exercising the
newly implemented Redshift actions (CreateClusterSnapshot,
DeleteClusterSnapshot, DescribeClusterSnapshots, CreateClusterSubnetGroup,
DeleteClusterSubnetGroup, DescribeClusterSubnetGroups,
CreateClusterParameterGroup, DeleteClusterParameterGroup,
DescribeClusterParameterGroups, EnableLogging, DisableLogging) in provider.rs;
add entries to tests/parity/scenarios/core.json (and
extended.json/all-services-smoke.json as appropriate) that call these API
actions with representative parameters (e.g.,
SnapshotIdentifier/ClusterIdentifier, ClusterSubnetGroupName/SubnetIds,
ParameterGroupName/ParameterGroupFamily, ClusterIdentifier for logging) so
parity tests exercise the match arms handling those operations (match arms
labeled by the action strings in provider.rs).
In `@crates/services/redshift/tests/redshift_tests.rs`:
- Around line 577-605: Add two unit tests that cover the ClusterNotFound error
path for EnableLogging and DisableLogging: create tests named like
test_enable_logging_not_found and test_disable_logging_not_found that
instantiate RedshiftProvider, call dispatch with make_ctx("EnableLogging",
params) and make_ctx("DisableLogging", params) where params contains a
non-existent ClusterIdentifier (e.g., "no-cluster"), await the response, and
assert the response is an error with the ClusterNotFound payload similar to
test_reboot_cluster_not_found (use body_str(&resp) and assert it contains
"ClusterNotFound" and the appropriate XML error structure); mirror the structure
and assertions used in test_reboot_cluster_not_found so the provider.rs error
branches for EnableLogging and DisableLogging are exercised.
- Around line 508-534: Add a new test that asserts the duplicate-create error
path for CreateClusterParameterGroup: call RedshiftProvider::dispatch with
make_ctx("CreateClusterParameterGroup", params) twice (same params as in
test_create_and_describe_parameter_group) and verify the second response returns
the ClusterParameterGroupAlreadyExists error (similar to
test_create_snapshot_duplicate_fails). Ensure the test name indicates duplicate
failure (e.g., test_create_parameter_group_duplicate_fails), awaits the dispatch
calls, and asserts the error response status/body matches the provider's
ClusterParameterGroupAlreadyExists output.
- Around line 432-465: Add a new async test (modeled after
test_create_snapshot_duplicate_fails and located near
test_create_and_describe_subnet_group) that uses RedshiftProvider::new(), calls
dispatch with make_ctx("CreateClusterSubnetGroup", params) to create "my-sg"
once, then calls dispatch a second time with the same params and asserts the
response is an error for the duplicate path: check resp.status_code is the
expected error code (e.g., 400) and that body_str(&resp) contains
"ClusterSubnetGroupAlreadyExists" (or the exact error string returned by the
provider implementation); reuse the same param keys ("ClusterSubnetGroupName",
"Description", "VpcId", "SubnetIds.SubnetIdentifier.1",
"SubnetIds.SubnetIdentifier.2") as in test_create_and_describe_subnet_group to
reproduce the duplicate creation scenario.
In `@crates/services/route53/src/provider.rs`:
- Around line 469-774: The PR is missing parity scenario entries, performance
assertions, and error-path unit tests for the eight Route53 operations; add
parity scenarios for "ListHostedZonesByName", "GetHostedZoneCount",
"CreateHealthCheck", "GetHealthCheck", "DeleteHealthCheck", "ListHealthChecks",
"ChangeTagsForResource", and "ListTagsForResource" to
tests/parity/scenarios/core.json and extended.json; add Instant-based perf
assertions in crates/services/route53/tests/route53_perf_tests.rs exercising
those handlers (same pattern used for hosted-zone/rrset control-plane ops); and
add unit tests in crates/services/route53/tests/route53_tests.rs that exercise
the error paths for each handler (e.g., missing store or missing resource)
verifying the XML error responses produced by the handler branches (look at
handlers matching "ListHostedZonesByName", "GetHostedZoneCount",
"CreateHealthCheck", "DeleteHealthCheck", "ListHealthChecks",
"ChangeTagsForResource", "ListTagsForResource" and reuse the existing test
pattern from test_get_health_check_not_found to assert xml_error outputs and
status codes).
In `@crates/services/route53/tests/route53_tests.rs`:
- Around line 522-568: Add error-path tests alongside
test_create_and_get_health_check: (1) Add a test (e.g.,
test_create_health_check_missing_fields) that uses Route53Provider::dispatch
with make_ctx("CreateHealthCheck", <invalid XML missing
CallerReference/Port/Type>, "/2013-04-01/healthcheck", "POST") and assert
resp.status_code == 400 and body contains an error element/validation message;
(2) Add a test (e.g., test_delete_nonexistent_health_check) that calls dispatch
with make_ctx("DeleteHealthCheck", "",
&format!("/2013-04-01/healthcheck/nonexistent-id"), "DELETE") and assert
resp.status_code == 404 and body contains "NoSuchHealthCheck"; follow the same
pattern as test_create_and_get_health_check for using body_str/xml_text to
inspect response contents.
In `@crates/services/ses/src/provider.rs`:
- Around line 288-578: Add missing parity, perf, and error-path tests for the
SES operations: implement scenario entries referencing GetSendQuota,
GetSendStatistics, CreateTemplate, DeleteTemplate, GetTemplate, ListTemplates,
SendTemplatedEmail, SetIdentityFeedbackForwardingEnabled, and
GetIdentityNotificationAttributes in the parity JSONs (core.json, extended.json,
all-services-smoke.json); update crates/services/ses/tests/ses_perf_tests.rs to
dispatch/measure these operations (use the existing dispatch helper used by
other perf tests rather than adding new timing primitives) so perf coverage
includes them; and add unit tests in crates/services/ses/tests/ses_tests.rs
covering the missing error paths (e.g., GetSendQuota/GetSendStatistics when no
store, DeleteTemplate/GetTemplate/ListTemplates when template missing,
SendTemplatedEmail when template absent, and GetIdentityNotificationAttributes
for unknown identities) by asserting xml_error responses from the provider
handlers (targeting the same operation names used in provider.rs like
"GetTemplate", "CreateTemplate", "SendTemplatedEmail", etc.).
In `@crates/services/ses/tests/ses_tests.rs`:
- Around line 532-576: Add negative-path tests in the same test module using
SesProvider, dispatch and make_ctx: create one test (e.g.,
test_set_identity_feedback_forwarding_enabled_unverified) that calls
SetIdentityFeedbackForwardingEnabled with an identity that wasn't
VerifyEmailIdentity-verified and assert the response is an error (non-200) and
contains the expected error element/message; create another test (e.g.,
test_get_identity_notification_attributes_nonexistent) that calls
GetIdentityNotificationAttributes with an identity that doesn't exist and assert
it returns an error response and contains the appropriate error element/message;
reuse the existing test patterns (SesProvider::new(), make_ctx, body_str) and
mirror assertions style from test_set_identity_feedback_forwarding_enabled and
test_get_identity_notification_attributes to keep consistency.
In `@crates/services/ssm/src/provider.rs`:
- Around line 325-769: The PR is missing unit, perf, and parity coverage for
several new SSM operations; add unit tests in
crates/services/ssm/tests/ssm_tests.rs that exercise the DeleteDocument error
path (DeleteDocument when document missing),
AddTagsToResource/ListTagsForResource/RemoveTagsFromResource (verify tags
added/listed/removed on Document), and ensure CreateDocument duplicate error and
GetCommandInvocation not-found remain covered; add performance benchmarks in
crates/services/ssm/tests/ssm_perf_tests.rs using std::time::Instant for
GetParameterHistory, a representative document flow (CreateDocument,
SendCommand, ListCommands, GetCommandInvocation), and tag operations
(Add/List/RemoveTagsFromResource); finally, add parity scenario entries under
tests/parity/scenarios (core/extended/all-services) to include probes for
get-parameter-history, create/delete/describe/get/list documents, send/list/get
commands, and tag operations so CI parity tests cover them. Ensure tests
reference the handler operation names exactly (GetParameterHistory,
CreateDocument, DeleteDocument, DescribeDocument, GetDocument, ListDocuments,
SendCommand, ListCommands, GetCommandInvocation, AddTagsToResource,
ListTagsForResource, RemoveTagsFromResource) so they exercise the code paths
shown.
In `@crates/services/sts/src/provider.rs`:
- Around line 151-214: The PR is missing parity scenarios and negative/perf
tests for the AssumeRoleWithWebIdentity and AssumeRoleWithSAML handlers: add
parity scenario JSON entries (core.json or extended.json) under
tests/parity/scenarios that invoke "AssumeRoleWithWebIdentity" and
"AssumeRoleWithSAML" so they run against OpenStack and LocalStack; add
error-path unit tests in crates/services/sts/tests/sts_tests.rs that exercise
failure branches for those handlers (use the repo's error helper/formatting) and
assert error XML/status; and add Instant-timed performance tests in
crates/services/sts/tests/sts_perf_tests.rs that call the handlers (e.g., using
the same request construction as in existing success tests) and assert they
complete within expected durations. Ensure test names reference
AssumeRoleWithWebIdentity and AssumeRoleWithSAML so they’re discoverable.
In `@crates/services/sts/tests/sts_tests.rs`:
- Around line 174-243: The tests add happy-path coverage for
AssumeRoleWithWebIdentity and AssumeRoleWithSAML but lack error-path tests; add
new tokio::test functions (e.g., test_assume_role_with_web_identity_error,
test_assume_role_with_web_identity_missing_token,
test_assume_role_with_saml_error) that use StsProvider::new() and
p.dispatch(&make_ctx(...)) with invalid/missing parameters (missing/invalid
RoleArn, missing/invalid WebIdentityToken or SAMLAssertion, and malformed
params) and assert the response.status_code is an error (400-range) and the body
contains the appropriate STS error code/message; reuse existing helpers body_str
and make_ctx to construct requests and assert specific error codes/messages
returned by the provider for the given failure scenarios.
---
Outside diff comments:
In `@crates/services/ec2/src/store.rs`:
- Around line 73-159: The new store types KeyPair, Address, InternetGateway, and
Volume were added (in structs KeyPair, Address, InternetGateway, Volume and
aggregated in Ec2Store) but there are no parity scenarios exercising
CreateKeyPair, AllocateAddress, CreateInternetGateway, or CreateVolume under
tests/parity/scenarios; add at least one parity scenario file for each resource
mapping to the corresponding API flows (CreateKeyPair, AllocateAddress,
CreateInternetGateway, CreateVolume) so the parity test suite covers those new
store changes and references the same resource IDs/fields used by the unit tests
in crates/services/ec2/tests/ec2_tests.rs.
In `@crates/services/opensearch/src/store.rs`:
- Around line 19-51: Add parity assertion coverage for ServiceSoftwareOptions by
ensuring the parity test captures and asserts its fields: update the parity
scenario or test harness that produces the JSON for opensearch domains so that
service_software_options (fields current_version, new_version, update_available,
cancellable, update_status, description, automated_update_date) are emitted and
validated; modify the parity scenario
(tests/parity/scenarios/all-services-smoke.json) or the parity capture logic in
the opensearch provider code paths (see ServiceSoftwareOptions struct and
Default impl in store.rs and usages in provider.rs) to include these fields in
the recorded output, and add assertions in the parity test suite (similar to the
unit assertions in tests/opensearch_tests.rs) that verify expected values/types
for those ServiceSoftwareOptions properties.
In `@crates/services/rds/tests/rds_perf_tests.rs`:
- Around line 1-131: Tests are missing perf coverage for the other RDS
operations; add new tokio::test functions that mirror the existing patterns (use
RdsProvider::new(), make_ctx, body_str) to exercise the remaining operations and
assert status_code == 200 and timing bounds. Specifically add tests named
perf_create_db_parameter_group, perf_create_db_subnet_group,
perf_delete_db_instance, perf_delete_db_parameter_group,
perf_delete_db_snapshot, perf_delete_db_subnet_group,
perf_describe_db_parameter_groups, perf_describe_db_snapshots,
perf_describe_db_subnet_groups, perf_modify_db_instance,
perf_reboot_db_instance, and perf_restore_db_instance_from_snapshot; for
mutating ops use n=100 loop with unique identifiers and assert
elapsed.as_millis() < 2000, for describe ops perform the query after creating
sample resources and assert elapsed.as_millis() < 500, reusing make_ctx and
body_str helpers and creating any prerequisite resources (e.g., DB
instances/snapshots/subnet/parameter groups) before the measured loop.
In `@crates/services/s3/src/object_store.rs`:
- Around line 213-218: Update the doc comment for the async writer (the block
describing write_via_copy) to note that pre-allocation via set_len is
conditional on platform and size: on Linux the code only pre-allocates for
objects >= 50 MiB; otherwise pre-allocation is skipped. Edit the comment that
currently says "when size is known" to explicitly mention the 50 MiB Linux
threshold and that behavior may differ on other platforms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcab6e1b-996c-4d40-852c-921db2a8827f
⛔ Files ignored due to path filters (23)
.opencode/package-lock.jsonis excluded by!**/package-lock.jsonand included by noneCLAUDE.mdis excluded by none and included by noneCargo.lockis excluded by!**/*.lockand included byCargo.lockDockerfileis excluded by none and included by nonecrates/openstack/Cargo.tomlis excluded by none and included by nonecrates/services/cloudtrail/CHANGELOG.mdis excluded by none and included by nonecrates/services/cloudtrail/Cargo.tomlis excluded by none and included by nonecrates/services/cognito/CHANGELOG.mdis excluded by none and included by nonecrates/services/cognito/Cargo.tomlis excluded by none and included by nonecrates/services/ecs/CHANGELOG.mdis excluded by none and included by nonecrates/services/ecs/Cargo.tomlis excluded by none and included by nonecrates/services/elasticache/CHANGELOG.mdis excluded by none and included by nonecrates/services/elasticache/Cargo.tomlis excluded by none and included by nonecrates/services/rds/CHANGELOG.mdis excluded by none and included by nonecrates/services/rds/Cargo.tomlis excluded by none and included by nonecrates/tests/integration/Cargo.tomlis excluded by none and included by nonemanifests/guided/cloudtrail.guided.jsonis excluded by none and included by nonemanifests/guided/cognito.guided.jsonis excluded by none and included by nonemanifests/guided/ecs.guided.jsonis excluded by none and included by nonemanifests/guided/elasticache.guided.jsonis excluded by none and included by nonemanifests/guided/rds.guided.jsonis excluded by none and included by nonetests/bench/bench_services.shis excluded by none and included by nonetests/harness/service-matrix.jsonis excluded by none and included by none
📒 Files selected for processing (72)
.github/workflows/benchmark.yml.github/workflows/ci.ymlCargo.tomlcrates/aws-protocol/src/protocol.rscrates/aws-protocol/tests/protocol_integration.rscrates/config/src/services.rscrates/gateway/src/server.rscrates/openstack/src/main.rscrates/services/apigateway/tests/apigateway_perf_tests.rscrates/services/cloudformation/tests/cloudformation_perf_tests.rscrates/services/cloudtrail/src/error.rscrates/services/cloudtrail/src/lib.rscrates/services/cloudtrail/src/provider.rscrates/services/cloudtrail/src/store.rscrates/services/cloudtrail/tests/cloudtrail_perf_tests.rscrates/services/cloudtrail/tests/cloudtrail_tests.rscrates/services/cognito/src/error.rscrates/services/cognito/src/lib.rscrates/services/cognito/src/provider.rscrates/services/cognito/src/store.rscrates/services/cognito/tests/cognito_perf_tests.rscrates/services/cognito/tests/cognito_tests.rscrates/services/ec2/src/provider.rscrates/services/ec2/src/store.rscrates/services/ec2/tests/ec2_perf_tests.rscrates/services/ec2/tests/ec2_tests.rscrates/services/ecs/src/error.rscrates/services/ecs/src/lib.rscrates/services/ecs/src/provider.rscrates/services/ecs/src/store.rscrates/services/ecs/tests/ecs_perf_tests.rscrates/services/ecs/tests/ecs_tests.rscrates/services/elasticache/src/error.rscrates/services/elasticache/src/lib.rscrates/services/elasticache/src/provider.rscrates/services/elasticache/src/store.rscrates/services/elasticache/tests/elasticache_perf_tests.rscrates/services/elasticache/tests/elasticache_tests.rscrates/services/firehose/tests/firehose_perf_tests.rscrates/services/iam/tests/iam_perf_tests.rscrates/services/kinesis/tests/kinesis_perf_tests.rscrates/services/opensearch/src/provider.rscrates/services/opensearch/src/store.rscrates/services/opensearch/tests/opensearch_perf_tests.rscrates/services/opensearch/tests/opensearch_tests.rscrates/services/rds/src/error.rscrates/services/rds/src/lib.rscrates/services/rds/src/provider.rscrates/services/rds/src/store.rscrates/services/rds/tests/rds_perf_tests.rscrates/services/rds/tests/rds_tests.rscrates/services/redshift/src/provider.rscrates/services/redshift/src/store.rscrates/services/redshift/tests/redshift_perf_tests.rscrates/services/redshift/tests/redshift_tests.rscrates/services/route53/src/provider.rscrates/services/route53/src/store.rscrates/services/route53/tests/route53_tests.rscrates/services/s3/src/object_store.rscrates/services/ses/src/provider.rscrates/services/ses/src/store.rscrates/services/ses/tests/ses_tests.rscrates/services/sns/tests/sns_perf_tests.rscrates/services/ssm/src/provider.rscrates/services/ssm/src/store.rscrates/services/ssm/tests/ssm_perf_tests.rscrates/services/ssm/tests/ssm_tests.rscrates/services/stepfunctions/tests/stepfunctions_perf_tests.rscrates/services/sts/src/provider.rscrates/services/sts/tests/sts_tests.rscrates/tests/integration/src/harness.rscrates/tests/integration/src/parity.rs
Comment on lines
+469
to
+774
| // ---------------------------------------------------------------- | ||
| // ListHostedZonesByName GET /2013-04-01/hostedzonesbyname | ||
| // ---------------------------------------------------------------- | ||
| "ListHostedZonesByName" => { | ||
| let Some(store) = self.store.get(account_id, ROUTE53_REGION) else { | ||
| let body = format!( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ | ||
| <ListHostedZonesByNameResponse xmlns=\"{ROUTE53_NS}\">\ | ||
| <HostedZones></HostedZones>\ | ||
| <IsTruncated>false</IsTruncated>\ | ||
| <MaxItems>100</MaxItems>\ | ||
| </ListHostedZonesByNameResponse>" | ||
| ); | ||
| return Ok(xml_ok(body)); | ||
| }; | ||
| let dns_name_filter = ctx | ||
| .query_params | ||
| .get("dnsname") | ||
| .map(|s| s.as_str()) | ||
| .unwrap_or(""); | ||
| let mut zones: Vec<&HostedZone> = store | ||
| .zones | ||
| .values() | ||
| .filter(|z| { | ||
| dns_name_filter.is_empty() | ||
| || z.name | ||
| .trim_end_matches('.') | ||
| .ends_with(dns_name_filter.trim_end_matches('.')) | ||
| }) | ||
| .collect(); | ||
| zones.sort_by(|a, b| a.name.cmp(&b.name)); | ||
| let zones_xml: String = zones.iter().map(|z| zone_xml(z)).collect(); | ||
| let body = format!( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ | ||
| <ListHostedZonesByNameResponse xmlns=\"{ROUTE53_NS}\">\ | ||
| <HostedZones>{zones_xml}</HostedZones>\ | ||
| <IsTruncated>false</IsTruncated>\ | ||
| <MaxItems>100</MaxItems>\ | ||
| </ListHostedZonesByNameResponse>" | ||
| ); | ||
| Ok(xml_ok(body)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // GetHostedZoneCount GET /2013-04-01/hostedzonecount | ||
| // ---------------------------------------------------------------- | ||
| "GetHostedZoneCount" => { | ||
| let count = self | ||
| .store | ||
| .get(account_id, ROUTE53_REGION) | ||
| .map(|s| s.zones.len()) | ||
| .unwrap_or(0); | ||
| let body = format!( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ | ||
| <GetHostedZoneCountResponse xmlns=\"{ROUTE53_NS}\">\ | ||
| <HostedZoneCount>{count}</HostedZoneCount>\ | ||
| </GetHostedZoneCountResponse>" | ||
| ); | ||
| Ok(xml_ok(body)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // CreateHealthCheck POST /2013-04-01/healthcheck | ||
| // ---------------------------------------------------------------- | ||
| "CreateHealthCheck" => { | ||
| let raw = String::from_utf8_lossy(ctx.raw_body_bytes()); | ||
| let caller_reference = xml_text(&raw, "CallerReference").unwrap_or_else(req_id); | ||
| let health_check_type = | ||
| xml_text(&raw, "Type").unwrap_or_else(|| "HTTP".to_string()); | ||
| let ip_address = xml_text(&raw, "IPAddress"); | ||
| let port: u16 = xml_text(&raw, "Port") | ||
| .and_then(|s| s.parse().ok()) | ||
| .unwrap_or(80); | ||
| let resource_path = xml_text(&raw, "ResourcePath"); | ||
| let fqdn = xml_text(&raw, "FullyQualifiedDomainName"); | ||
| let request_interval: u32 = xml_text(&raw, "RequestInterval") | ||
| .and_then(|s| s.parse().ok()) | ||
| .unwrap_or(30); | ||
| let failure_threshold: u32 = xml_text(&raw, "FailureThreshold") | ||
| .and_then(|s| s.parse().ok()) | ||
| .unwrap_or(3); | ||
|
|
||
| let hc_id = short_id(); | ||
| let hc = HealthCheck { | ||
| id: hc_id.clone(), | ||
| caller_reference, | ||
| config: HealthCheckConfig { | ||
| ip_address, | ||
| port, | ||
| health_check_type, | ||
| resource_path, | ||
| fully_qualified_domain_name: fqdn, | ||
| request_interval, | ||
| failure_threshold, | ||
| }, | ||
| health_check_version: 1, | ||
| }; | ||
|
|
||
| let mut store = self.store.get_or_create(account_id, ROUTE53_REGION); | ||
| store.health_checks.insert(hc_id.clone(), hc.clone()); | ||
|
|
||
| let body = format!( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ | ||
| <CreateHealthCheckResponse xmlns=\"{ROUTE53_NS}\">\ | ||
| <HealthCheck>{}</HealthCheck>\ | ||
| </CreateHealthCheckResponse>", | ||
| health_check_xml(&hc) | ||
| ); | ||
| Ok(xml_created( | ||
| body, | ||
| &format!("/2013-04-01/healthcheck/{hc_id}"), | ||
| )) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // GetHealthCheck GET /2013-04-01/healthcheck/{Id} | ||
| // ---------------------------------------------------------------- | ||
| "GetHealthCheck" => { | ||
| let hc_id = ctx.path.split('/').next_back().unwrap_or("").to_string(); | ||
| let Some(store) = self.store.get(account_id, ROUTE53_REGION) else { | ||
| return Ok(xml_error( | ||
| "NoSuchHealthCheck", | ||
| &format!("No health check with ID: {hc_id}"), | ||
| 404, | ||
| )); | ||
| }; | ||
| match store.health_checks.get(&hc_id) { | ||
| Some(hc) => { | ||
| let body = format!( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ | ||
| <GetHealthCheckResponse xmlns=\"{ROUTE53_NS}\">\ | ||
| <HealthCheck>{}</HealthCheck>\ | ||
| </GetHealthCheckResponse>", | ||
| health_check_xml(hc) | ||
| ); | ||
| Ok(xml_ok(body)) | ||
| } | ||
| None => Ok(xml_error( | ||
| "NoSuchHealthCheck", | ||
| &format!("No health check with ID: {hc_id}"), | ||
| 404, | ||
| )), | ||
| } | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // DeleteHealthCheck DELETE /2013-04-01/healthcheck/{Id} | ||
| // ---------------------------------------------------------------- | ||
| "DeleteHealthCheck" => { | ||
| let hc_id = ctx.path.split('/').next_back().unwrap_or("").to_string(); | ||
| let mut store = self.store.get_or_create(account_id, ROUTE53_REGION); | ||
| if store.health_checks.remove(&hc_id).is_none() { | ||
| return Ok(xml_error( | ||
| "NoSuchHealthCheck", | ||
| &format!("No health check with ID: {hc_id}"), | ||
| 404, | ||
| )); | ||
| } | ||
| let body = format!( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ | ||
| <DeleteHealthCheckResponse xmlns=\"{ROUTE53_NS}\"></DeleteHealthCheckResponse>" | ||
| ); | ||
| Ok(xml_ok(body)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // ListHealthChecks GET /2013-04-01/healthcheck | ||
| // ---------------------------------------------------------------- | ||
| "ListHealthChecks" => { | ||
| let Some(store) = self.store.get(account_id, ROUTE53_REGION) else { | ||
| let body = format!( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ | ||
| <ListHealthChecksResponse xmlns=\"{ROUTE53_NS}\">\ | ||
| <HealthChecks></HealthChecks>\ | ||
| <IsTruncated>false</IsTruncated>\ | ||
| <MaxItems>100</MaxItems>\ | ||
| </ListHealthChecksResponse>" | ||
| ); | ||
| return Ok(xml_ok(body)); | ||
| }; | ||
| let hcs_xml: String = store | ||
| .health_checks | ||
| .values() | ||
| .map(|hc| format!("<HealthCheck>{}</HealthCheck>", health_check_xml(hc))) | ||
| .collect(); | ||
| let body = format!( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ | ||
| <ListHealthChecksResponse xmlns=\"{ROUTE53_NS}\">\ | ||
| <HealthChecks>{hcs_xml}</HealthChecks>\ | ||
| <IsTruncated>false</IsTruncated>\ | ||
| <MaxItems>100</MaxItems>\ | ||
| </ListHealthChecksResponse>" | ||
| ); | ||
| Ok(xml_ok(body)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // ChangeTagsForResource POST /2013-04-01/tags/{ResourceType}/{ResourceId} | ||
| // ---------------------------------------------------------------- | ||
| "ChangeTagsForResource" => { | ||
| // Path: /2013-04-01/tags/{resourcetype}/{resourceid} | ||
| let parts: Vec<&str> = ctx.path.split('/').collect(); | ||
| let (resource_type, resource_id) = parts | ||
| .iter() | ||
| .enumerate() | ||
| .find(|(_, p)| *p == &"tags") | ||
| .map(|(i, _)| { | ||
| let rtype = parts.get(i + 1).copied().unwrap_or(""); | ||
| let rid = parts.get(i + 2).copied().unwrap_or(""); | ||
| (rtype.to_string(), rid.to_string()) | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| let raw = String::from_utf8_lossy(ctx.raw_body_bytes()); | ||
| let mut store = self.store.get_or_create(account_id, ROUTE53_REGION); | ||
| let tag_map = store.tags.entry((resource_type, resource_id)).or_default(); | ||
|
|
||
| // Parse <AddTags><Tag><Key>...</Key><Value>...</Value></Tag></AddTags> | ||
| let mut rest = raw.as_ref(); | ||
| while let Some(start) = rest.find("<Tag>") { | ||
| let chunk = &rest[start..]; | ||
| let end = chunk.find("</Tag>").unwrap_or(chunk.len()); | ||
| let tag_block = &chunk[..end]; | ||
| if let (Some(key), Some(value)) = | ||
| (xml_text(tag_block, "Key"), xml_text(tag_block, "Value")) | ||
| { | ||
| tag_map.insert(key, value); | ||
| } | ||
| rest = &rest[start + end..]; | ||
| } | ||
|
|
||
| // Parse <RemoveTagKeys><Key>...</Key></RemoveTagKeys> | ||
| let mut remove_rest = raw.as_ref(); | ||
| if let Some(remove_start) = remove_rest.find("<RemoveTagKeys>") { | ||
| let remove_chunk = &remove_rest[remove_start..]; | ||
| let remove_end = remove_chunk | ||
| .find("</RemoveTagKeys>") | ||
| .unwrap_or(remove_chunk.len()); | ||
| let remove_block = &remove_chunk[..remove_end]; | ||
| remove_rest = remove_block; | ||
| while let Some(ks) = remove_rest.find("<Key>") { | ||
| let after = &remove_rest[ks + 5..]; | ||
| if let Some(ke) = after.find("</Key>") { | ||
| let k = &after[..ke]; | ||
| tag_map.remove(k); | ||
| } | ||
| remove_rest = &remove_rest[ks + 5..]; | ||
| } | ||
| } | ||
|
|
||
| let body = format!( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ | ||
| <ChangeTagsForResourceResponse xmlns=\"{ROUTE53_NS}\"></ChangeTagsForResourceResponse>" | ||
| ); | ||
| Ok(xml_ok(body)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // ListTagsForResource GET /2013-04-01/tags/{ResourceType}/{ResourceId} | ||
| // ---------------------------------------------------------------- | ||
| "ListTagsForResource" => { | ||
| let parts: Vec<&str> = ctx.path.split('/').collect(); | ||
| let (resource_type, resource_id) = parts | ||
| .iter() | ||
| .enumerate() | ||
| .find(|(_, p)| *p == &"tags") | ||
| .map(|(i, _)| { | ||
| let rtype = parts.get(i + 1).copied().unwrap_or(""); | ||
| let rid = parts.get(i + 2).copied().unwrap_or(""); | ||
| (rtype.to_string(), rid.to_string()) | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| let tags_xml = self | ||
| .store | ||
| .get(account_id, ROUTE53_REGION) | ||
| .and_then(|store| { | ||
| store | ||
| .tags | ||
| .get(&(resource_type.clone(), resource_id.clone())) | ||
| .map(|tags| { | ||
| tags.iter() | ||
| .map(|(k, v)| { | ||
| format!( | ||
| "<Tag><Key>{}</Key><Value>{}</Value></Tag>", | ||
| xml_escape(k), | ||
| xml_escape(v) | ||
| ) | ||
| }) | ||
| .collect::<String>() | ||
| }) | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| let body = format!( | ||
| "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ | ||
| <ListTagsForResourceResponse xmlns=\"{ROUTE53_NS}\">\ | ||
| <ResourceTagSet>\ | ||
| <ResourceType>{resource_type}</ResourceType>\ | ||
| <ResourceId>{resource_id}</ResourceId>\ | ||
| <Tags>{tags_xml}</Tags>\ | ||
| </ResourceTagSet>\ | ||
| </ListTagsForResourceResponse>" | ||
| ); | ||
| Ok(xml_ok(body)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify Route53 test coverage for new operations.
echo "=== Checking Route53 unit tests ==="
if [[ -f crates/services/route53/tests/route53_tests.rs ]]; then
rg -n 'health_check|ListHostedZonesByName|GetHostedZoneCount|ChangeTagsForResource|ListTagsForResource' crates/services/route53/tests/route53_tests.rs | head -10
else
echo "Route53 unit test file not found or incomplete"
fi
echo "=== Checking Route53 parity scenarios ==="
fd -e json . tests/parity/scenarios/ -x cat {} \; | grep -i 'route53.*health\|ChangeTagsForResource' || echo "No parity scenarios found for new operations"Repository: JesseKoldewijn/openstack
Length of output: 626
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Route53 tests directory ==="
ls -la crates/services/route53/tests || true
echo
echo "=== Search unit tests for 8 operations ==="
for f in crates/services/route53/tests/*.rs; do
[ -f "$f" ] || continue
echo "--- $f ---"
rg -n "ListHostedZonesByName|GetHostedZoneCount|CreateHealthCheck|GetHealthCheck|DeleteHealthCheck|ListHealthChecks|ChangeTagsForResource|ListTagsForResource" "$f" || true
done
echo
echo "=== Search perf tests for std::time::Instant + operations ==="
for f in crates/services/route53/tests/*_perf_tests.rs; do
[ -f "$f" ] || continue
echo "--- $f ---"
rg -n "Instant|ListHostedZonesByName|GetHostedZoneCount|CreateHealthCheck|GetHealthCheck|DeleteHealthCheck|ListHealthChecks|ChangeTagsForResource|ListTagsForResource" "$f" || true
done
echo
echo "=== Search parity scenarios for Route53 and operations ==="
# list a few scenario files
fd -e json . tests/parity/scenarios/ | head -n 50
echo
echo "Route53-related parity files (by keyword 'route53'):"
rg -n -i "route53" tests/parity/scenarios/ --glob "*.json" || true
echo
echo "Parity scenarios mentioning each operation: "
for op in ListHostedZonesByName GetHostedZoneCount CreateHealthCheck GetHealthCheck DeleteHealthCheck ListHealthChecks ChangeTagsForResource ListTagsForResource; do
echo "--- $op ---"
rg -n "$op" tests/parity/scenarios/ --glob "*.json" || true
doneRepository: JesseKoldewijn/openstack
Length of output: 5741
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect route53_perf_tests.rs ==="
ls -la crates/services/route53/tests/route53_perf_tests.rs
wc -l crates/services/route53/tests/route53_perf_tests.rs
rg -n "health[-_ ]?check|healthcheck|health[-_ ]?checks|HostedZonesByName|hosted[-_ ]?zones[-_ ]?by[-_ ]?name|hosted[-_ ]?zone[-_ ]?count|tags[-_ ]?for[-_ ]?resource|list[-_ ]?tags[-_ ]?for[-_ ]?resource|ChangeTagsForResource|ListTagsForResource" crates/services/route53/tests/route53_perf_tests.rs || true
echo
echo "=== Show perf test sections around Instant::now() ==="
# print small windows around each occurrence
perl -ne 'if(/Instant::now\(\)/){ $line=$.; print "---- around $line ----\n"; }' crates/services/route53/tests/route53_perf_tests.rs >/tmp/lines.txt
cat /tmp/lines.txt || true
while read -r L; do
# L looks like "---- around 123 ----"
n=$(echo "$L" | rg -o '[0-9]+' | tail -1)
if [[ -n "${n:-}" ]]; then
echo "---- $n ----"
sed -n "$((n-20)),$((n+20))p" crates/services/route53/tests/route53_perf_tests.rs
fi
done < <(rg -n "Instant::now\(\)" crates/services/route53/tests/route53_perf_tests.rs | head -n 20 | awk -F: '{print "---- around "$1" ----"}')
echo
echo "=== Search parity scenarios for CLI-like names ==="
for f in tests/parity/scenarios/core.json tests/parity/scenarios/extended.json; do
echo "--- $f ---"
rg -n -i "route53.*(list-hosted-zones-by-name|get-hosted-zone-count|create-health-check|get-health-check|delete-health-check|list-health-checks|change-tags-for-resource|list-tags-for-resource|listhealthchecks|change.*tags.*resource|list.*tags.*resource|healthcheck)" "$f" || true
done
echo
echo "=== Search parity scenarios for API-like names ==="
for f in tests/parity/scenarios/core.json tests/parity/scenarios/extended.json; do
echo "--- $f ---"
rg -n "ListHostedZonesByName|GetHostedZoneCount|CreateHealthCheck|GetHealthCheck|DeleteHealthCheck|ListHealthChecks|ChangeTagsForResource|ListTagsForResource" "$f" || true
doneRepository: JesseKoldewijn/openstack
Length of output: 498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
PERF="crates/services/route53/tests/route53_perf_tests.rs"
UNIT="crates/services/route53/tests/route53_tests.rs"
CORE="tests/parity/scenarios/core.json"
EXT="tests/parity/scenarios/extended.json"
echo "=== route53_perf_tests.rs (first 220 lines) ==="
sed -n '1,220p' "$PERF"
echo
echo "=== route53_perf_tests.rs (last 80 lines) ==="
sed -n '181,261p' "$PERF"
echo
echo "=== route53_perf_tests.rs: lines containing key identifiers ==="
rg -n "dispatch\(&make_ctx|make_ctx\(|Instant::now\(\)|ListHostedZonesByName|GetHostedZoneCount|CreateHealthCheck|GetHealthCheck|DeleteHealthCheck|ListHealthChecks|ChangeTagsForResource|ListTagsForResource" "$PERF" || true
echo
echo "=== route53_tests.rs: locate operation-specific unit tests ==="
rg -n "ListHostedZonesByName|GetHostedZoneCount|CreateHealthCheck|GetHealthCheck|DeleteHealthCheck|ListHealthChecks|ChangeTagsForResource|ListTagsForResource" "$UNIT"
echo
echo "=== route53_tests.rs: show surrounding blocks for each operation ==="
for pat in "ListHostedZonesByName" "GetHostedZoneCount" "CreateHealthCheck" "GetHealthCheck" "DeleteHealthCheck" "ListHealthChecks" "ChangeTagsForResource" "ListTagsForResource"; do
echo
echo "--- block around $pat ---"
# show ~35 lines around first match for each op
line=$(rg -n "$pat" "$UNIT" | head -n1 | awk -F: '{print $1}')
if [[ -n "$line" ]]; then
start=$((line-25)); end=$((line+80));
if (( start < 1 )); then start=1; fi
sed -n "${start},${end}p" "$UNIT"
fi
done
echo
echo "=== parity core/extended: search for CLI-like route53 subcommands ==="
for f in "$CORE" "$EXT"; do
echo "--- $f ---"
rg -n -i "route53.*(list-hosted-zones-by-name|get-hosted-zone-count|create-health-check|get-health-check|delete-health-check|list-health-checks|change-tags-for-resource|list-tags-for-resource)" "$f" || true
done
echo
echo "=== parity core/extended: search for API operation-like strings ==="
for f in "$CORE" "$EXT"; do
echo "--- $f ---"
rg -n "(ListHostedZonesByName|GetHostedZoneCount|CreateHealthCheck|GetHealthCheck|DeleteHealthCheck|ListHealthChecks|ChangeTagsForResource|ListTagsForResource)" "$f" || true
doneRepository: JesseKoldewijn/openstack
Length of output: 34079
🏁 Script executed:
#!/bin/bash
set -euo pipefail
UNIT="crates/services/route53/tests/route53_tests.rs"
PERF="crates/services/route53/tests/route53_perf_tests.rs"
echo "=== Confirm unit tests cover each operation name ==="
for op in ListHostedZonesByName GetHostedZoneCount CreateHealthCheck GetHealthCheck DeleteHealthCheck ListHealthChecks ChangeTagsForResource ListTagsForResource; do
echo "--- $op ---"
rg -n "\"$op\"|fn test_.*$op|$op" "$UNIT" | head -n 50 || true
done
echo
echo "=== Look for error-path tests (4xx/Code/NoSuch/Error) near each operation ==="
for op in ListHostedZonesByName GetHostedZoneCount CreateHealthCheck GetHealthCheck DeleteHealthCheck ListHealthChecks ChangeTagsForResource ListTagsForResource; do
echo
echo "--- $op (near keywords: NoSuch|not found|404|error) ---"
# Show up to 120 lines around the first occurrence for each op
line=$(rg -n "\"$op\"" "$UNIT" | head -n1 | awk -F: '{print $1}') || true
if [[ -n "${line:-}" ]]; then
start=$((line-60)); end=$((line+180))
if (( start < 1 )); then start=1; fi
sed -n "${start},${end}p" "$UNIT" | rg -n "NoSuch|not found|404|error|Code" || true
fi
done
echo
echo "=== Confirm perf tests do NOT mention the 8 operations ==="
for op in ListHostedZonesByName GetHostedZoneCount CreateHealthCheck GetHealthCheck DeleteHealthCheck ListHealthChecks ChangeTagsForResource ListTagsForResource; do
echo "--- $op ---"
rg -n "$op" "$PERF" || true
done
echo
echo "=== Quick perf file scan for key structs/dispatch ops ==="
rg -n "dispatch\\(&make_ctx\\(|Instant::now\\(" "$PERF" | head -n 50Repository: JesseKoldewijn/openstack
Length of output: 4140
Route53: missing required parity/perf coverage and incomplete error-path unit tests for 7/8 listed operations
tests/parity/scenarios/core.jsonandtests/parity/scenarios/extended.jsoncontain no parity scenarios for:ListHostedZonesByName,GetHostedZoneCount,CreateHealthCheck,GetHealthCheck,DeleteHealthCheck,ListHealthChecks,ChangeTagsForResource,ListTagsForResource.crates/services/route53/tests/route53_perf_tests.rshasInstant-based performance assertions, but none for these 8 operations (only hosted-zone/rrset/control-plane ops).crates/services/route53/tests/route53_tests.rsincludes happy-path tests for all 8 operations, but onlyGetHealthCheckhas an explicit error-path test (test_get_health_check_not_found); the other 7 operations lack an error-path unit test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/route53/src/provider.rs` around lines 469 - 774, The PR is
missing parity scenario entries, performance assertions, and error-path unit
tests for the eight Route53 operations; add parity scenarios for
"ListHostedZonesByName", "GetHostedZoneCount", "CreateHealthCheck",
"GetHealthCheck", "DeleteHealthCheck", "ListHealthChecks",
"ChangeTagsForResource", and "ListTagsForResource" to
tests/parity/scenarios/core.json and extended.json; add Instant-based perf
assertions in crates/services/route53/tests/route53_perf_tests.rs exercising
those handlers (same pattern used for hosted-zone/rrset control-plane ops); and
add unit tests in crates/services/route53/tests/route53_tests.rs that exercise
the error paths for each handler (e.g., missing store or missing resource)
verifying the XML error responses produced by the handler branches (look at
handlers matching "ListHostedZonesByName", "GetHostedZoneCount",
"CreateHealthCheck", "DeleteHealthCheck", "ListHealthChecks",
"ChangeTagsForResource", "ListTagsForResource" and reuse the existing test
pattern from test_get_health_check_not_found to assert xml_error outputs and
status codes).
Source: Coding guidelines
Comment on lines
+522
to
+568
| #[tokio::test] | ||
| async fn test_create_and_get_health_check() { | ||
| let p = Route53Provider::new(); | ||
| let hc_xml = r#"<CreateHealthCheckRequest> | ||
| <CallerReference>hc-ref-1</CallerReference> | ||
| <HealthCheckConfig> | ||
| <IPAddress>1.2.3.4</IPAddress> | ||
| <Port>80</Port> | ||
| <Type>HTTP</Type> | ||
| <ResourcePath>/health</ResourcePath> | ||
| <RequestInterval>30</RequestInterval> | ||
| <FailureThreshold>3</FailureThreshold> | ||
| </HealthCheckConfig> | ||
| </CreateHealthCheckRequest>"#; | ||
|
|
||
| let resp = p | ||
| .dispatch(&make_ctx( | ||
| "CreateHealthCheck", | ||
| hc_xml, | ||
| "/2013-04-01/healthcheck", | ||
| "POST", | ||
| )) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 201); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("CreateHealthCheckResponse")); | ||
| assert!(body.contains("<Type>HTTP</Type>")); | ||
| assert!(body.contains("1.2.3.4")); | ||
|
|
||
| let hc_id = xml_text(&body, "Id").unwrap(); | ||
|
|
||
| let get_resp = p | ||
| .dispatch(&make_ctx( | ||
| "GetHealthCheck", | ||
| "", | ||
| &format!("/2013-04-01/healthcheck/{hc_id}"), | ||
| "GET", | ||
| )) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(get_resp.status_code, 200); | ||
| let get_body = body_str(&get_resp); | ||
| assert!(get_body.contains("GetHealthCheckResponse")); | ||
| assert!(get_body.contains("1.2.3.4")); | ||
| assert!(get_body.contains("<Port>80</Port>")); | ||
| } |
There was a problem hiding this comment.
Missing error-path coverage for CreateHealthCheck and DeleteHealthCheck.
The test suite covers happy-path flows for both operations but lacks error-path tests:
- CreateHealthCheck (lines 522–568): No test for missing required fields (e.g.,
CallerReference,Port,Type). The provider should validate these and return a 400 error; add a test to verify that behavior. - DeleteHealthCheck (lines 623–662): No test for attempting to delete a nonexistent health check. Add a test that calls
DeleteHealthCheckon a health check ID that was never created and assert the expected error response (likely 404 withNoSuchHealthCheck).
As per coding guidelines, every operation must have at least one test covering the primary error path.
Also applies to: 623-662
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/route53/tests/route53_tests.rs` around lines 522 - 568, Add
error-path tests alongside test_create_and_get_health_check: (1) Add a test
(e.g., test_create_health_check_missing_fields) that uses
Route53Provider::dispatch with make_ctx("CreateHealthCheck", <invalid XML
missing CallerReference/Port/Type>, "/2013-04-01/healthcheck", "POST") and
assert resp.status_code == 400 and body contains an error element/validation
message; (2) Add a test (e.g., test_delete_nonexistent_health_check) that calls
dispatch with make_ctx("DeleteHealthCheck", "",
&format!("/2013-04-01/healthcheck/nonexistent-id"), "DELETE") and assert
resp.status_code == 404 and body contains "NoSuchHealthCheck"; follow the same
pattern as test_create_and_get_health_check for using body_str/xml_text to
inspect response contents.
Source: Coding guidelines
Comment on lines
+532
to
+576
| #[tokio::test] | ||
| async fn test_set_identity_feedback_forwarding_enabled() { | ||
| let p = SesProvider::new(); | ||
| let mut verify = HashMap::new(); | ||
| verify.insert("EmailAddress".to_string(), "notif@example.com".to_string()); | ||
| p.dispatch(&make_ctx("VerifyEmailIdentity", verify)) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let mut params = HashMap::new(); | ||
| params.insert("Identity".to_string(), "notif@example.com".to_string()); | ||
| params.insert("ForwardingEnabled".to_string(), "false".to_string()); | ||
| let resp = p | ||
| .dispatch(&make_ctx("SetIdentityFeedbackForwardingEnabled", params)) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("SetIdentityFeedbackForwardingEnabledResponse")); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_get_identity_notification_attributes() { | ||
| let p = SesProvider::new(); | ||
| let mut verify = HashMap::new(); | ||
| verify.insert("EmailAddress".to_string(), "notif2@example.com".to_string()); | ||
| p.dispatch(&make_ctx("VerifyEmailIdentity", verify)) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let mut params = HashMap::new(); | ||
| params.insert( | ||
| "Identities.member.1".to_string(), | ||
| "notif2@example.com".to_string(), | ||
| ); | ||
| let resp = p | ||
| .dispatch(&make_ctx("GetIdentityNotificationAttributes", params)) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("GetIdentityNotificationAttributesResponse")); | ||
| assert!(body.contains("notif2@example.com")); | ||
| assert!(body.contains("<ForwardingEnabled>true</ForwardingEnabled>")); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add error path tests for notification attribute operations.
Per coding guidelines, every operation must have at least one test covering the primary error path. The notification attribute operations (SetIdentityFeedbackForwardingEnabled, GetIdentityNotificationAttributes) only have happy-path tests. Consider adding error tests for scenarios like:
- Setting feedback forwarding for a non-existent or unverified identity
- Getting notification attributes for a non-existent identity
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/ses/tests/ses_tests.rs` around lines 532 - 576, Add
negative-path tests in the same test module using SesProvider, dispatch and
make_ctx: create one test (e.g.,
test_set_identity_feedback_forwarding_enabled_unverified) that calls
SetIdentityFeedbackForwardingEnabled with an identity that wasn't
VerifyEmailIdentity-verified and assert the response is an error (non-200) and
contains the expected error element/message; create another test (e.g.,
test_get_identity_notification_attributes_nonexistent) that calls
GetIdentityNotificationAttributes with an identity that doesn't exist and assert
it returns an error response and contains the appropriate error element/message;
reuse the existing test patterns (SesProvider::new(), make_ctx, body_str) and
mirror assertions style from test_set_identity_feedback_forwarding_enabled and
test_get_identity_notification_attributes to keep consistency.
Source: Coding guidelines
Comment on lines
+366
to
+391
| #[tokio::test] | ||
| async fn test_delete_security_group() { | ||
| let p = Ec2Provider::new(); | ||
| let mut sg_p = HashMap::new(); | ||
| sg_p.insert("GroupName".to_string(), "del-sg".to_string()); | ||
| sg_p.insert("Description".to_string(), "to delete".to_string()); | ||
| let sg_resp = p | ||
| .dispatch(&make_ctx("CreateSecurityGroup", sg_p)) | ||
| .await | ||
| .unwrap(); | ||
| let group_id = extract_tag(&body_str(&sg_resp), "groupId"); | ||
|
|
||
| let mut del = HashMap::new(); | ||
| del.insert("GroupId".to_string(), group_id.clone()); | ||
| let resp = p | ||
| .dispatch(&make_ctx("DeleteSecurityGroup", del)) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
|
|
||
| let list = p | ||
| .dispatch(&make_ctx("DescribeSecurityGroups", HashMap::new())) | ||
| .await | ||
| .unwrap(); | ||
| assert!(!body_str(&list).contains(&group_id)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Missing error path test for DeleteSecurityGroup.
Per coding guidelines, every operation must cover the primary error path. Add a test for deleting a non-existent security group.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/ec2/tests/ec2_tests.rs` around lines 366 - 391, Add a new
test that exercises the error path for DeleteSecurityGroup by attempting to
delete a non-existent GroupId: create a new async test (e.g.,
test_delete_security_group_not_found) that instantiates Ec2Provider::new(),
calls p.dispatch(&make_ctx("DeleteSecurityGroup", params)) with a bogus GroupId
(not created earlier) and then asserts the provider returns the expected error
outcome (e.g., response status_code != 200 and body_str contains the AWS-style
error like "InvalidGroup.NotFound" or that dispatch returned an Err). Reuse
make_ctx, body_str, and Ec2Provider::dispatch as in test_delete_security_group
and keep assertions specific to DeleteSecurityGroup's error signal.
Source: Coding guidelines
Comment on lines
+718
to
+737
| #[tokio::test] | ||
| async fn test_delete_volume() { | ||
| let p = Ec2Provider::new(); | ||
| let vol_resp = p | ||
| .dispatch(&make_ctx("CreateVolume", HashMap::new())) | ||
| .await | ||
| .unwrap(); | ||
| let volume_id = extract_tag(&body_str(&vol_resp), "volumeId"); | ||
|
|
||
| let mut del = HashMap::new(); | ||
| del.insert("VolumeId".to_string(), volume_id.clone()); | ||
| let resp = p.dispatch(&make_ctx("DeleteVolume", del)).await.unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
|
|
||
| let desc = p | ||
| .dispatch(&make_ctx("DescribeVolumes", HashMap::new())) | ||
| .await | ||
| .unwrap(); | ||
| assert!(!body_str(&desc).contains(&volume_id)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Missing error path test for DeleteVolume.
Per coding guidelines, every operation must cover the primary error path. Add a test for deleting a non-existent volume.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/ec2/tests/ec2_tests.rs` around lines 718 - 737, Add a new
test function (e.g., test_delete_volume_not_found) alongside test_delete_volume
that constructs an Ec2Provider via Ec2Provider::new(), calls
p.dispatch(&make_ctx("DeleteVolume", del)) with a HashMap where "VolumeId" is a
non-existent id (e.g., "vol-00000000"), awaits the result, and asserts the error
path: check resp.status_code is not 200 and that body_str(&resp) contains the
expected error identifier (e.g., "InvalidVolume.NotFound" or other
service-specific not-found string); also keep the existing DescribeVolumes check
separate — reference functions/methods: test_delete_volume, Ec2Provider::new,
p.dispatch, make_ctx, "DeleteVolume", and body_str.
Source: Coding guidelines
Comment on lines
+235
to
+265
| #[tokio::test] | ||
| async fn test_create_and_describe_replication_group() { | ||
| let svc = ElastiCacheProvider::new(); | ||
| let resp = svc | ||
| .dispatch(&make_ctx( | ||
| "CreateReplicationGroup", | ||
| [ | ||
| p("ReplicationGroupId", "my-rg"), | ||
| p("ReplicationGroupDescription", "Test RG"), | ||
| p("NumCacheClusters", "2"), | ||
| p("AutomaticFailoverEnabled", "true"), | ||
| p("CacheNodeType", "cache.t3.micro"), | ||
| ] | ||
| .into(), | ||
| )) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("CreateReplicationGroupResponse")); | ||
| assert!(body.contains("<ReplicationGroupId>my-rg</ReplicationGroupId>")); | ||
| assert!(body.contains("<AutomaticFailover>enabled</AutomaticFailover>")); | ||
| assert!(body.contains("<NumCacheClusters>2</NumCacheClusters>")); | ||
|
|
||
| let desc = svc | ||
| .dispatch(&make_ctx("DescribeReplicationGroups", HashMap::new())) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(desc.status_code, 200); | ||
| assert!(body_str(&desc).contains("my-rg")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Missing error path test for CreateReplicationGroup.
Per coding guidelines, every operation must cover at least one error path. Consider adding a test for duplicate replication group creation (similar to the cache cluster duplicate test pattern).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/elasticache/tests/elasticache_tests.rs` around lines 235 -
265, The test test_create_and_describe_replication_group only covers the success
path; add an error-path test that attempts to create a duplicate replication
group (call ElastiCacheProvider::new() and dispatch CreateReplicationGroup twice
using make_ctx with the same "ReplicationGroupId" like "my-rg"), then assert the
second response is an error (non-200 status_code) and its body contains the
expected duplicate error text (mirror the duplicate cache cluster test pattern:
check error code/message for an already-existing replication group). Ensure you
use the same helper functions (make_ctx, p, body_str, and
ElastiCacheProvider::dispatch) so the new assertions align with existing tests.
Source: Coding guidelines
Comment on lines
+371
to
+400
| #[tokio::test] | ||
| async fn test_create_and_describe_subnet_group() { | ||
| let svc = ElastiCacheProvider::new(); | ||
| let resp = svc | ||
| .dispatch(&make_ctx( | ||
| "CreateCacheSubnetGroup", | ||
| [ | ||
| p("CacheSubnetGroupName", "my-sg"), | ||
| p("CacheSubnetGroupDescription", "test"), | ||
| p("VpcId", "vpc-12345"), | ||
| p("SubnetIds.SubnetIdentifier.1", "subnet-aaa"), | ||
| p("SubnetIds.SubnetIdentifier.2", "subnet-bbb"), | ||
| ] | ||
| .into(), | ||
| )) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("<CacheSubnetGroupName>my-sg</CacheSubnetGroupName>")); | ||
|
|
||
| let desc = svc | ||
| .dispatch(&make_ctx("DescribeCacheSubnetGroups", HashMap::new())) | ||
| .await | ||
| .unwrap(); | ||
| let desc_body = body_str(&desc); | ||
| assert!(desc_body.contains("my-sg")); | ||
| assert!(desc_body.contains("subnet-aaa")); | ||
| assert!(desc_body.contains("subnet-bbb")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Missing error path test for CreateCacheSubnetGroup.
Per coding guidelines, every operation must cover at least one error path. Consider adding a test for duplicate subnet group creation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/elasticache/tests/elasticache_tests.rs` around lines 371 -
400, Add an error-path test that attempts to create the same CacheSubnetGroup
twice using ElastiCacheProvider::dispatch with the CreateCacheSubnetGroup
parameters (reuse test_create_and_describe_subnet_group setup), then assert the
second response is an error: verify non-200 status or that the body contains the
ElastiCache error identifier (e.g., "CacheSubnetGroupAlreadyExistsFault" or an
appropriate error message) and a descriptive message; ensure the test name
reflects duplicate creation (e.g., test_create_subnet_group_duplicate_error) and
uses the same request shape (CacheSubnetGroupName, SubnetIds.*) so it exercises
the duplicate-path in CreateCacheSubnetGroup handling.
Source: Coding guidelines
Comment on lines
+432
to
+465
| #[tokio::test] | ||
| async fn test_create_and_describe_subnet_group() { | ||
| let p = RedshiftProvider::new(); | ||
| let mut params = HashMap::new(); | ||
| params.insert("ClusterSubnetGroupName".to_string(), "my-sg".to_string()); | ||
| params.insert("Description".to_string(), "test subnet group".to_string()); | ||
| params.insert("VpcId".to_string(), "vpc-12345".to_string()); | ||
| params.insert( | ||
| "SubnetIds.SubnetIdentifier.1".to_string(), | ||
| "subnet-aaa".to_string(), | ||
| ); | ||
| params.insert( | ||
| "SubnetIds.SubnetIdentifier.2".to_string(), | ||
| "subnet-bbb".to_string(), | ||
| ); | ||
| let resp = p | ||
| .dispatch(&make_ctx("CreateClusterSubnetGroup", params)) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("CreateClusterSubnetGroupResponse")); | ||
| assert!(body.contains("<ClusterSubnetGroupName>my-sg</ClusterSubnetGroupName>")); | ||
|
|
||
| let resp = p | ||
| .dispatch(&make_ctx("DescribeClusterSubnetGroups", HashMap::new())) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("my-sg")); | ||
| assert!(body.contains("subnet-aaa")); | ||
| assert!(body.contains("subnet-bbb")); | ||
| } |
There was a problem hiding this comment.
Missing error path test for CreateClusterSubnetGroup duplicate.
The provider implementation at provider.rs:511 returns a ClusterSubnetGroupAlreadyExists error when attempting to create a duplicate subnet group, but no unit test covers this error path. The coding guidelines require at least one test covering the primary error path for each operation.
Add a test similar to test_create_snapshot_duplicate_fails that attempts to create the same subnet group twice and asserts the duplicate error response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/redshift/tests/redshift_tests.rs` around lines 432 - 465, Add
a new async test (modeled after test_create_snapshot_duplicate_fails and located
near test_create_and_describe_subnet_group) that uses RedshiftProvider::new(),
calls dispatch with make_ctx("CreateClusterSubnetGroup", params) to create
"my-sg" once, then calls dispatch a second time with the same params and asserts
the response is an error for the duplicate path: check resp.status_code is the
expected error code (e.g., 400) and that body_str(&resp) contains
"ClusterSubnetGroupAlreadyExists" (or the exact error string returned by the
provider implementation); reuse the same param keys ("ClusterSubnetGroupName",
"Description", "VpcId", "SubnetIds.SubnetIdentifier.1",
"SubnetIds.SubnetIdentifier.2") as in test_create_and_describe_subnet_group to
reproduce the duplicate creation scenario.
Source: Coding guidelines
Comment on lines
+508
to
+534
| #[tokio::test] | ||
| async fn test_create_and_describe_parameter_group() { | ||
| let p = RedshiftProvider::new(); | ||
| let mut params = HashMap::new(); | ||
| params.insert("ParameterGroupName".to_string(), "my-pg".to_string()); | ||
| params.insert( | ||
| "ParameterGroupFamily".to_string(), | ||
| "redshift-1.0".to_string(), | ||
| ); | ||
| params.insert("Description".to_string(), "test pg".to_string()); | ||
| let resp = p | ||
| .dispatch(&make_ctx("CreateClusterParameterGroup", params)) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("CreateClusterParameterGroupResponse")); | ||
| assert!(body.contains("<ParameterGroupName>my-pg</ParameterGroupName>")); | ||
|
|
||
| let resp = p | ||
| .dispatch(&make_ctx("DescribeClusterParameterGroups", HashMap::new())) | ||
| .await | ||
| .unwrap(); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("my-pg")); | ||
| assert!(body.contains("redshift-1.0")); | ||
| } |
There was a problem hiding this comment.
Missing error path test for CreateClusterParameterGroup duplicate.
The provider implementation at provider.rs:598 returns a ClusterParameterGroupAlreadyExists error when attempting to create a duplicate parameter group, but no unit test covers this error path. The coding guidelines require at least one test covering the primary error path for each operation.
Add a test similar to test_create_snapshot_duplicate_fails that attempts to create the same parameter group twice and asserts the duplicate error response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/redshift/tests/redshift_tests.rs` around lines 508 - 534, Add
a new test that asserts the duplicate-create error path for
CreateClusterParameterGroup: call RedshiftProvider::dispatch with
make_ctx("CreateClusterParameterGroup", params) twice (same params as in
test_create_and_describe_parameter_group) and verify the second response returns
the ClusterParameterGroupAlreadyExists error (similar to
test_create_snapshot_duplicate_fails). Ensure the test name indicates duplicate
failure (e.g., test_create_parameter_group_duplicate_fails), awaits the dispatch
calls, and asserts the error response status/body matches the provider's
ClusterParameterGroupAlreadyExists output.
Source: Coding guidelines
Comment on lines
+577
to
+605
| #[tokio::test] | ||
| async fn test_enable_and_disable_logging() { | ||
| let p = RedshiftProvider::new(); | ||
| let mut params = HashMap::new(); | ||
| params.insert("ClusterIdentifier".to_string(), "log-cluster".to_string()); | ||
| p.dispatch(&make_ctx("CreateCluster", params)) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let mut enable = HashMap::new(); | ||
| enable.insert("ClusterIdentifier".to_string(), "log-cluster".to_string()); | ||
| let resp = p | ||
| .dispatch(&make_ctx("EnableLogging", enable.clone())) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("EnableLoggingResponse")); | ||
| assert!(body.contains("<LoggingEnabled>true</LoggingEnabled>")); | ||
|
|
||
| let resp = p | ||
| .dispatch(&make_ctx("DisableLogging", enable)) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("DisableLoggingResponse")); | ||
| assert!(body.contains("<LoggingEnabled>false</LoggingEnabled>")); | ||
| } |
There was a problem hiding this comment.
Missing error path tests for EnableLogging and DisableLogging cluster not found.
Both operations return a ClusterNotFound error when the cluster does not exist (provider.rs:684 and provider.rs:713), but no unit tests cover these error paths. The coding guidelines require at least one test covering the primary error path for each operation.
Add two tests similar to test_reboot_cluster_not_found that attempt to enable/disable logging on a non-existent cluster and assert the ClusterNotFound error response.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/redshift/tests/redshift_tests.rs` around lines 577 - 605, Add
two unit tests that cover the ClusterNotFound error path for EnableLogging and
DisableLogging: create tests named like test_enable_logging_not_found and
test_disable_logging_not_found that instantiate RedshiftProvider, call dispatch
with make_ctx("EnableLogging", params) and make_ctx("DisableLogging", params)
where params contains a non-existent ClusterIdentifier (e.g., "no-cluster"),
await the response, and assert the response is an error with the ClusterNotFound
payload similar to test_reboot_cluster_not_found (use body_str(&resp) and assert
it contains "ClusterNotFound" and the appropriate XML error structure); mirror
the structure and assertions used in test_reboot_cluster_not_found so the
provider.rs error branches for EnableLogging and DisableLogging are exercised.
Source: Coding guidelines
Comment on lines
+174
to
+243
| #[tokio::test] | ||
| async fn test_assume_role_with_web_identity() { | ||
| let p = StsProvider::new(); | ||
| let resp = p | ||
| .dispatch(&make_ctx( | ||
| "AssumeRoleWithWebIdentity", | ||
| &[ | ||
| ("RoleArn", "arn:aws:iam::000000000000:role/WebRole"), | ||
| ("RoleSessionName", "web-session"), | ||
| ("WebIdentityToken", "fake-web-token-12345"), | ||
| ("ProviderId", "cognito-identity.amazonaws.com"), | ||
| ], | ||
| )) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("AssumeRoleWithWebIdentityResponse")); | ||
| assert!(body.contains("<AccessKeyId>")); | ||
| assert!(body.contains("<SessionToken>")); | ||
| assert!(body.contains("<Expiration>")); | ||
| assert!(body.contains("<SubjectFromWebIdentityToken>")); | ||
| assert!(body.contains("<Provider>cognito-identity.amazonaws.com</Provider>")); | ||
| assert!(body.contains("arn:aws:sts::000000000000:assumed-role/WebRole/web-session")); | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_assume_role_with_web_identity_defaults() { | ||
| let p = StsProvider::new(); | ||
| let resp = p | ||
| .dispatch(&make_ctx("AssumeRoleWithWebIdentity", &[])) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("AssumeRoleWithWebIdentityResponse")); | ||
| assert!(body.contains("<AccessKeyId>")); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // AssumeRoleWithSAML | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| #[tokio::test] | ||
| async fn test_assume_role_with_saml() { | ||
| let p = StsProvider::new(); | ||
| let resp = p | ||
| .dispatch(&make_ctx( | ||
| "AssumeRoleWithSAML", | ||
| &[ | ||
| ("RoleArn", "arn:aws:iam::000000000000:role/SamlRole"), | ||
| ( | ||
| "PrincipalArn", | ||
| "arn:aws:iam::000000000000:saml-provider/MySAML", | ||
| ), | ||
| ("SAMLAssertion", "PHNhbWw+ZmFrZTwvc2FtbD4="), | ||
| ], | ||
| )) | ||
| .await | ||
| .unwrap(); | ||
| assert_eq!(resp.status_code, 200); | ||
| let body = body_str(&resp); | ||
| assert!(body.contains("AssumeRoleWithSAMLResponse")); | ||
| assert!(body.contains("<AccessKeyId>")); | ||
| assert!(body.contains("<SessionToken>")); | ||
| assert!(body.contains("<Expiration>")); | ||
| assert!(body.contains("<Issuer>")); | ||
| assert!(body.contains("arn:aws:sts::000000000000:assumed-role/SamlRole/saml-session")); | ||
| assert!(body.contains("<SubjectType>persistent</SubjectType>")); | ||
| } |
There was a problem hiding this comment.
Add error path tests for AssumeRoleWithWebIdentity and AssumeRoleWithSAML.
The coding guidelines mandate that every operation must have at least one test covering the happy path and one covering the primary error path. The three new tests added here (test_assume_role_with_web_identity, test_assume_role_with_web_identity_defaults, test_assume_role_with_saml) only cover happy paths. Please add error path tests for common failure scenarios such as:
- Invalid or missing
RoleArn - Invalid or missing
WebIdentityToken/SAMLAssertion - Malformed parameters
These error tests should assert that the provider returns appropriate error responses (status 400 or similar) with meaningful error codes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/sts/tests/sts_tests.rs` around lines 174 - 243, The tests add
happy-path coverage for AssumeRoleWithWebIdentity and AssumeRoleWithSAML but
lack error-path tests; add new tokio::test functions (e.g.,
test_assume_role_with_web_identity_error,
test_assume_role_with_web_identity_missing_token,
test_assume_role_with_saml_error) that use StsProvider::new() and
p.dispatch(&make_ctx(...)) with invalid/missing parameters (missing/invalid
RoleArn, missing/invalid WebIdentityToken or SAMLAssertion, and malformed
params) and assert the response.status_code is an error (400-range) and the body
contains the appropriate STS error code/message; reuse existing helpers body_str
and make_ctx to construct requests and assert specific error codes/messages
returned by the provider for the given failure scenarios.
Source: Coding guidelines
Owner
Author
CodeRabbit blocker remediationAddressed all blocker-scope inline findings in this push. Summary: Fixed in this PR
Deferred (guideline debt — not CI blockers)Tracked for follow-up PR
Already addressed / false positives
Required CI was already green before these fixes; re-run expected to stay green. |
Normalize service name aliases for protocol routing and provider overrides, correct RDS snapshot and ElastiCache store semantics, clean up OpenSearch tags on domain delete, fix Cognito callback URL fields, harden EC2 test XML parsing, and align benchmark workflow checkout hardening with CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
crates/services/cognito/src/provider.rs (3)
399-449:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce
UserPoolIdon client-scoped lookups and deletes.
DeleteUserPoolClientandDescribeUserPoolClientonly key byClientId, so a client created in pool A can still be deleted or described while the caller passes pool B. ParseUserPoolIdin both branches and reject when it does not matchclient.user_pool_id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/cognito/src/provider.rs` around lines 399 - 449, DeleteUserPoolClient and DescribeUserPoolClient currently identify clients only by ClientId, allowing operations across user pools; parse the UserPoolId from ctx in both handlers (same way str_param(ctx, "ClientId") is used), look up the client via self.store.get/_or_create and then verify that the parsed UserPoolId equals the client's user_pool_id before proceeding; if missing UserPoolId return InvalidParameterException and if it doesn't match return ResourceNotFoundException (or appropriate error) — update the DeleteUserPoolClient and DescribeUserPoolClient branches to perform this UserPoolId check against store.clients.get(&client_id) / removed client entry and reject when client.user_pool_id != provided UserPoolId.
849-889:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject missing or empty passwords in
AdminInitiateAuth.Lines 855-863 coerce a missing
PASSWORDto"", and Line 883 only rejects mismatches when the caller supplied a non-empty password. That means an enabled user can get tokens by omittingPASSWORDentirely. Fail the request before token issuance whenPASSWORDis absent or empty.Suggested fix
- let password = auth_params - .get("PASSWORD") - .and_then(|v| v.as_str()) - .unwrap_or("") - .to_string(); + let Some(password) = auth_params + .get("PASSWORD") + .and_then(|v| v.as_str()) + .filter(|p| !p.is_empty()) + else { + return Ok(json_error( + "NotAuthorizedException", + "Incorrect username or password", + 400, + )); + }; @@ - let stored_password = user.password.as_deref().unwrap_or(""); - if !password.is_empty() && stored_password != password { + let stored_password = user.password.as_deref(); + if stored_password != Some(password) { return Ok(json_error( "NotAuthorizedException", "Incorrect username or password", 400, ));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/cognito/src/provider.rs` around lines 849 - 889, The AdminInitiateAuth flow currently treats a missing or empty PASSWORD as "" and only rejects when a non-empty password mismatches, allowing token issuance with no password; update the auth logic in the AdminInitiateAuth handling (where auth_params, username, password are extracted and where store.users.get(...) and user.password are checked) to explicitly reject when password.is_empty() by returning the same error used for bad credentials (e.g., NotAuthorizedException/400) before any stored_password comparison, while keeping the existing enabled check and stored_password comparison for non-empty inputs.
207-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid
get_or_createon not-found error paths.These branches create an account/region store before checking whether the pool, client, or user exists. A failing request against a fresh account/region therefore leaves behind an empty
CognitoStoreentry and can grow bundle state without bound. Use a read path first and only allocate store state in the actual create operations. This mirrors the empty-store leak pattern already called out elsewhere in this PR.Also applies to: 278-292, 410-418, 591-600, 713-729, 757-770, 811-832
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/cognito/src/provider.rs` around lines 207 - 218, The code currently calls self.store.get_or_create(account_id, region) in delete/read paths (see usage around get_or_create, store.user_pools.remove, store.clients.retain, store.users.retain), which allocates an empty CognitoStore for missing account/region and leaks state; change these branches to first perform a non-allocating lookup (e.g. use a read/get or get_mut variant that does not create) to see if a store exists and only proceed to check/remove the pool, clients, and users if the store is present; if the store or pool is missing return the same ResourceNotFound json error, and only use get_or_create in actual create operations.crates/services/opensearch/src/provider.rs (2)
607-614:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the full
ServiceSoftwareOptionsshape on cancel as well.
StartServiceSoftwareUpdateincludesNewVersion, butCancelServiceSoftwareUpdatedrops it even though both responses are serializing the same persistedServiceSoftwareOptionsobject. That inconsistent payload shape is easy to miss in tests and can break clients that deserialize one shared response model.🛠️ Minimal fix
Ok(json_ok(json!({ "ServiceSoftwareOptions": { "CurrentVersion": options.current_version, + "NewVersion": options.new_version, "UpdateAvailable": options.update_available, "Cancellable": options.cancellable, "UpdateStatus": options.update_status, "Description": options.description, } })))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/opensearch/src/provider.rs` around lines 607 - 614, The cancel path returns a truncated ServiceSoftwareOptions JSON; update the CancelServiceSoftwareUpdate response to return the full ServiceSoftwareOptions shape (matching StartServiceSoftwareUpdate) by including the NewVersion field from the persisted options object (use options.new_version or the existing field name) alongside CurrentVersion, UpdateAvailable, Cancellable, UpdateStatus, and Description so both handlers serialize the same shape.
444-515:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the ARN against an existing domain before serving tag ops.
AddTagscurrently doesstore.tags.entry(arn).or_default()without proving that ARN belongs to a stored domain, andRemoveTags/ListTagslikewise succeed against unknown ARNs. That creates orphan tag state thatDeleteDomaincan never clean up and masks the primaryResourceNotFoundExceptionpath these operations are expected to expose.🛠️ Minimal fix sketch
+fn domain_exists_by_arn(store: &OpenSearchStore, arn: &str) -> bool { + store.domains.values().any(|domain| domain.arn == arn) +} + "AddTags" => { let arn = match ctx.request_body.get("ARN").and_then(|v| v.as_str()) { Some(a) => a.to_string(), None => { return Ok(json_error("ValidationException", "ARN required", 400)); } }; @@ let mut store = self.store.get_or_create(account_id, region); + if !domain_exists_by_arn(&store, &arn) { + return Ok(json_error( + "ResourceNotFoundException", + &format!("Domain not found for ARN: {arn}"), + 404, + )); + } let tag_map = store.tags.entry(arn).or_default(); @@ "RemoveTags" => { @@ let mut store = self.store.get_or_create(account_id, region); - if let Some(tag_map) = store.tags.get_mut(&arn) { - for key in &tag_keys { - tag_map.remove(key); - } - } + if !domain_exists_by_arn(&store, &arn) { + return Ok(json_error( + "ResourceNotFoundException", + &format!("Domain not found for ARN: {arn}"), + 404, + )); + } + if let Some(tag_map) = store.tags.get_mut(&arn) { + for key in &tag_keys { + tag_map.remove(key); + } + } Ok(json_ok(json!({}))) } @@ "ListTags" => { @@ - let tags = self - .store - .get(account_id, region) - .and_then(|store| store.tags.get(&arn).cloned()) - .unwrap_or_default(); + let Some(store) = self.store.get(account_id, region) else { + return Ok(json_error( + "ResourceNotFoundException", + &format!("Domain not found for ARN: {arn}"), + 404, + )); + }; + if !domain_exists_by_arn(&store, &arn) { + return Ok(json_error( + "ResourceNotFoundException", + &format!("Domain not found for ARN: {arn}"), + 404, + )); + } + let tags = store.tags.get(&arn).cloned().unwrap_or_default();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/opensearch/src/provider.rs` around lines 444 - 515, Add validation to ensure the ARN belongs to an existing domain before mutating or returning tags: for AddTags, RemoveTags, and ListTags stop using get_or_create for the store or unconditional entry creation and instead call self.store.get(account_id, region) (or check the store.domains map) to verify the domain exists; if the domain/ARN is not found return Ok(json_error("ResourceNotFoundException", "Domain not found", 404)) rather than creating or returning empty tag state. Specifically update the AddTags handler (currently using store.tags.entry(arn).or_default()), the RemoveTags handler (which checks store.tags.get_mut(&arn)), and the ListTags handler (which reads store.tags.get(&arn)) to first lookup the domain existence (e.g., store.domains.contains_key(&arn) or store.get(...).and_then(|s| s.domains.get(&arn))) and only proceed to mutate/read tags if present.Source: Coding guidelines
crates/services/opensearch/tests/opensearch_tests.rs (1)
724-739:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing error-path test for
CancelServiceSoftwareUpdate.
provider.rshas an explicit 404ResourceNotFoundExceptionbranch for cancel, but this file only exercises the happy path for cancel. Please mirror the existing start/not-found case forCancelServiceSoftwareUpdate, and assert the error message in the existing start test as well so the body-content requirement is covered.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/opensearch/tests/opensearch_tests.rs` around lines 724 - 739, Add a failing-path test for CancelServiceSoftwareUpdate and strengthen the StartServiceSoftwareUpdate not-found test: create a new async test similar to test_start_service_software_update_domain_not_found that calls OpenSearchProvider::dispatch with make_ctx("CancelServiceSoftwareUpdate", json!({ "DomainName": "nonexistent" }), "/2021-01-01/opensearch/serviceSoftwareUpdate/cancel", "POST"), assert resp.status_code == 404 and that body_json(&resp)["code"] == "ResourceNotFoundException" (and optionally matching ["message"]), and update the existing test_start_service_software_update_domain_not_found to also assert the error message in body_json(&resp)["message"] so both body code and message are validated; use the same helpers OpenSearchProvider, make_ctx, and body_json to locate code.Source: Coding guidelines
crates/services/rds/src/provider.rs (1)
420-434:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid creating an empty store on the missing-instance error path.
Line 420 still calls
get_or_createbefore validatingDBInstanceIdentifier, so a failedCreateDBSnapshotagainst a brand-new account/region leaves behind an emptyRdsStore. Repeating that with arbitrary scopes can growAccountRegionBundleindefinitely even though every request fails.🔧 Minimal fix
- let mut store = self.store.get_or_create(account_id, region); + if self.store.get(account_id, region).is_none() { + return Ok(xml_error( + "DBInstanceNotFound", + &format!("DB instance {db_id} not found"), + 404, + )); + } + let mut store = self.store.get_or_create(account_id, region); if store.snapshots.contains_key(&snapshot_id) { return Ok(xml_error( "DBSnapshotAlreadyExists", &format!("Snapshot {snapshot_id} already exists"), 400,Based on learnings from the PR summary, this same
get_or_create-on-error-path leak pattern was already fixed in the ElastiCache handlers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/rds/src/provider.rs` around lines 420 - 434, The handler currently calls self.store.get_or_create(account_id, region) before validating the DBInstanceIdentifier, which leaks empty RdsStore entries on failed CreateDBSnapshot requests; change the flow to check for the instance existence first (e.g., query the existing store with a non-creating lookup or inspect self.store.get/account-region accessor) and only call get_or_create(account_id, region) when you are about to actually mutate the store (e.g., when adding to store.snapshots). Update the logic around get_or_create, store.snapshots, and store.instances in the CreateDBSnapshot handling code so that get_or_create is invoked after confirming the DB instance exists.crates/services/elasticache/src/provider.rs (4)
726-733:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject subnet-group deletes while clusters still reference the group.
This branch drops the subnet group without checking
store.clusters, so any cluster created withCacheSubnetGroupNameis left pointing at a deleted resource. That breaks the store’s internal contract and makes later describes return dangling references.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/elasticache/src/provider.rs` around lines 726 - 733, The DeleteCacheSubnetGroup branch currently removes store.subnet_groups without validating references; update the handler (the code around store.subnet_groups.remove and the DeleteCacheSubnetGroup response) to first scan store.clusters for any cluster whose CacheSubnetGroupName (or equivalent field) equals the subnet group name and, if any are found, return an xml_error indicating the group is in use (do not remove it); only call store.subnet_groups.remove(&name) and return Ok(xml_resp(...)) when no clusters reference that subnet group.
59-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape XML error fields before writing them into the response.
messageis built from request parameters in the not-found/error branches, so IDs containing<or&will produce malformed XML and reflected markup in the fault payload.🐛 Proposed fix
fn xml_error(code: &str, message: &str, status: u16) -> DispatchResponse { let xml = format!( "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\ <ErrorResponse xmlns=\"{EC_NS}\">\ -<Error><Code>{code}</Code><Message>{message}</Message></Error>\ -</ErrorResponse>" +<Error><Code>{}</Code><Message>{}</Message></Error>\ +</ErrorResponse>", + xml_escape(code), + xml_escape(message), ); DispatchResponse { status_code: status, body: ResponseBody::Buffered(Bytes::from(xml.into_bytes())), content_type: Cow::Borrowed("text/xml"),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/elasticache/src/provider.rs` around lines 59 - 63, The xml_error function writes unescaped code and message into the XML payload; escape XML entities for the code and message before inserting them into the format string to prevent malformed XML or injected markup. Update xml_error to run code and message through an XML-escaping helper (e.g., an existing escape utility or a small helper that replaces &, <, >, ", ') and then use those escaped values in the format! call so the generated ErrorResponse is always well-formed.
311-315:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the replication group in sync when deleting a member cluster.
If this cluster belongs to a replication group, the delete path removes the cluster but leaves
member_clustersandnum_cache_clustersstale on the owningReplicationGroup. After that,DescribeReplicationGroupscan advertise members that no longer exist.🛠️ Suggested direction
match store.clusters.remove(&cluster_id) { Some(c) => { + if let Some(rg_id) = &c.replication_group_id { + if let Some(rg) = store.replication_groups.get_mut(rg_id) { + rg.member_clusters.retain(|id| id != &cluster_id); + rg.num_cache_clusters = rg.member_clusters.len() as u32; + } + } let inner = format!("<CacheCluster>{}</CacheCluster>", cluster_xml(&c, region)); Ok(xml_resp("DeleteCacheCluster", &rid, &inner)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/elasticache/src/provider.rs` around lines 311 - 315, When removing a cluster via store.clusters.remove(&cluster_id) also locate its owning replication group (use the cluster's replication_group_id / ReplicationGroup id), remove the cluster_id from ReplicationGroup.member_clusters and adjust ReplicationGroup.num_cache_clusters accordingly, updating the store.replication_groups entry in place so subsequent DescribeReplicationGroups reflects the change; ensure you handle the case where the replication group becomes empty and persist the modified ReplicationGroup back into the store.
293-323:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd the required parity, unit, and perf coverage for these modified arms.
These operations were modified, and the supplied PR notes still call out remaining ElastiCache error-path test debt. Per the provider guideline, each modified arm needs a parity scenario, a happy-path + primary-error-path unit test, and a perf test entry with an
Instanttiming assertion before this is merge-ready.As per coding guidelines,
crates/services/*/src/provider.rs: “Every new or modified operation match arm in a service provider MUST have” parity coverage, a happy-path and error-path unit test, and a performance regression test; the PR summary says there are still “Missing error-path tests … across … ElastiCache … subsets.”Also applies to: 350-388, 393-426, 541-577, 608-650, 708-734
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/elasticache/src/provider.rs` around lines 293 - 323, The DeleteCacheCluster match arm in provider.rs (symbols: DeleteCacheCluster, store.clusters.remove, cluster_xml, xml_resp, xml_error) was modified and needs full test coverage: add a parity integration scenario, a unit test for the happy-path (ensure removal returns the expected xml_resp with cluster_xml content), a unit test for the primary error-path (missing CacheClusterId and cluster-not-found cases asserting xml_error is returned), and a perf regression test that invokes the DeleteCacheCluster code path and asserts an Instant timing bound; apply the same pattern for the other modified ElastiCache arms in this file (ensure each new/modified operation match arm has parity, happy-path + error-path unit tests, and a perf test with an Instant assertion).Source: Coding guidelines
♻️ Duplicate comments (1)
.github/workflows/benchmark.yml (1)
244-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin remaining GitHub Actions to commit SHAs.
actions/upload-artifact@v4andactions/download-artifact@v4are still tag-pinned instead of SHA-pinned, which leaves mutable supply-chain exposure in CI.#!/bin/bash # Read-only verification: list all non-SHA-pinned uses entries in this workflow rg -n '^\s*uses:\s*[^#\s]+@v[0-9]+' .github/workflows/benchmark.ymlAlso applies to: 270-270, 373-373
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/benchmark.yml at line 244, The workflow uses mutable tags for GitHub Actions (e.g., actions/upload-artifact@v4 and actions/download-artifact@v4); replace those tag-pinned references with the exact commit SHAs for the corresponding action releases (e.g., actions/upload-artifact@<commit-sha>, actions/download-artifact@<commit-sha>) so the workflow is SHA-pinned; update every occurrence of these symbols in the file (all uses of actions/upload-artifact@v4 and actions/download-artifact@v4) and verify the SHAs by fetching them from the official action repositories' tags/releases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/services/cognito/src/provider.rs`:
- Around line 399-449: DeleteUserPoolClient and DescribeUserPoolClient currently
identify clients only by ClientId, allowing operations across user pools; parse
the UserPoolId from ctx in both handlers (same way str_param(ctx, "ClientId") is
used), look up the client via self.store.get/_or_create and then verify that the
parsed UserPoolId equals the client's user_pool_id before proceeding; if missing
UserPoolId return InvalidParameterException and if it doesn't match return
ResourceNotFoundException (or appropriate error) — update the
DeleteUserPoolClient and DescribeUserPoolClient branches to perform this
UserPoolId check against store.clients.get(&client_id) / removed client entry
and reject when client.user_pool_id != provided UserPoolId.
- Around line 849-889: The AdminInitiateAuth flow currently treats a missing or
empty PASSWORD as "" and only rejects when a non-empty password mismatches,
allowing token issuance with no password; update the auth logic in the
AdminInitiateAuth handling (where auth_params, username, password are extracted
and where store.users.get(...) and user.password are checked) to explicitly
reject when password.is_empty() by returning the same error used for bad
credentials (e.g., NotAuthorizedException/400) before any stored_password
comparison, while keeping the existing enabled check and stored_password
comparison for non-empty inputs.
- Around line 207-218: The code currently calls
self.store.get_or_create(account_id, region) in delete/read paths (see usage
around get_or_create, store.user_pools.remove, store.clients.retain,
store.users.retain), which allocates an empty CognitoStore for missing
account/region and leaks state; change these branches to first perform a
non-allocating lookup (e.g. use a read/get or get_mut variant that does not
create) to see if a store exists and only proceed to check/remove the pool,
clients, and users if the store is present; if the store or pool is missing
return the same ResourceNotFound json error, and only use get_or_create in
actual create operations.
In `@crates/services/elasticache/src/provider.rs`:
- Around line 726-733: The DeleteCacheSubnetGroup branch currently removes
store.subnet_groups without validating references; update the handler (the code
around store.subnet_groups.remove and the DeleteCacheSubnetGroup response) to
first scan store.clusters for any cluster whose CacheSubnetGroupName (or
equivalent field) equals the subnet group name and, if any are found, return an
xml_error indicating the group is in use (do not remove it); only call
store.subnet_groups.remove(&name) and return Ok(xml_resp(...)) when no clusters
reference that subnet group.
- Around line 59-63: The xml_error function writes unescaped code and message
into the XML payload; escape XML entities for the code and message before
inserting them into the format string to prevent malformed XML or injected
markup. Update xml_error to run code and message through an XML-escaping helper
(e.g., an existing escape utility or a small helper that replaces &, <, >, ", ')
and then use those escaped values in the format! call so the generated
ErrorResponse is always well-formed.
- Around line 311-315: When removing a cluster via
store.clusters.remove(&cluster_id) also locate its owning replication group (use
the cluster's replication_group_id / ReplicationGroup id), remove the cluster_id
from ReplicationGroup.member_clusters and adjust
ReplicationGroup.num_cache_clusters accordingly, updating the
store.replication_groups entry in place so subsequent DescribeReplicationGroups
reflects the change; ensure you handle the case where the replication group
becomes empty and persist the modified ReplicationGroup back into the store.
- Around line 293-323: The DeleteCacheCluster match arm in provider.rs (symbols:
DeleteCacheCluster, store.clusters.remove, cluster_xml, xml_resp, xml_error) was
modified and needs full test coverage: add a parity integration scenario, a unit
test for the happy-path (ensure removal returns the expected xml_resp with
cluster_xml content), a unit test for the primary error-path (missing
CacheClusterId and cluster-not-found cases asserting xml_error is returned), and
a perf regression test that invokes the DeleteCacheCluster code path and asserts
an Instant timing bound; apply the same pattern for the other modified
ElastiCache arms in this file (ensure each new/modified operation match arm has
parity, happy-path + error-path unit tests, and a perf test with an Instant
assertion).
In `@crates/services/opensearch/src/provider.rs`:
- Around line 607-614: The cancel path returns a truncated
ServiceSoftwareOptions JSON; update the CancelServiceSoftwareUpdate response to
return the full ServiceSoftwareOptions shape (matching
StartServiceSoftwareUpdate) by including the NewVersion field from the persisted
options object (use options.new_version or the existing field name) alongside
CurrentVersion, UpdateAvailable, Cancellable, UpdateStatus, and Description so
both handlers serialize the same shape.
- Around line 444-515: Add validation to ensure the ARN belongs to an existing
domain before mutating or returning tags: for AddTags, RemoveTags, and ListTags
stop using get_or_create for the store or unconditional entry creation and
instead call self.store.get(account_id, region) (or check the store.domains map)
to verify the domain exists; if the domain/ARN is not found return
Ok(json_error("ResourceNotFoundException", "Domain not found", 404)) rather than
creating or returning empty tag state. Specifically update the AddTags handler
(currently using store.tags.entry(arn).or_default()), the RemoveTags handler
(which checks store.tags.get_mut(&arn)), and the ListTags handler (which reads
store.tags.get(&arn)) to first lookup the domain existence (e.g.,
store.domains.contains_key(&arn) or store.get(...).and_then(|s|
s.domains.get(&arn))) and only proceed to mutate/read tags if present.
In `@crates/services/opensearch/tests/opensearch_tests.rs`:
- Around line 724-739: Add a failing-path test for CancelServiceSoftwareUpdate
and strengthen the StartServiceSoftwareUpdate not-found test: create a new async
test similar to test_start_service_software_update_domain_not_found that calls
OpenSearchProvider::dispatch with make_ctx("CancelServiceSoftwareUpdate",
json!({ "DomainName": "nonexistent" }),
"/2021-01-01/opensearch/serviceSoftwareUpdate/cancel", "POST"), assert
resp.status_code == 404 and that body_json(&resp)["code"] ==
"ResourceNotFoundException" (and optionally matching ["message"]), and update
the existing test_start_service_software_update_domain_not_found to also assert
the error message in body_json(&resp)["message"] so both body code and message
are validated; use the same helpers OpenSearchProvider, make_ctx, and body_json
to locate code.
In `@crates/services/rds/src/provider.rs`:
- Around line 420-434: The handler currently calls
self.store.get_or_create(account_id, region) before validating the
DBInstanceIdentifier, which leaks empty RdsStore entries on failed
CreateDBSnapshot requests; change the flow to check for the instance existence
first (e.g., query the existing store with a non-creating lookup or inspect
self.store.get/account-region accessor) and only call get_or_create(account_id,
region) when you are about to actually mutate the store (e.g., when adding to
store.snapshots). Update the logic around get_or_create, store.snapshots, and
store.instances in the CreateDBSnapshot handling code so that get_or_create is
invoked after confirming the DB instance exists.
---
Duplicate comments:
In @.github/workflows/benchmark.yml:
- Line 244: The workflow uses mutable tags for GitHub Actions (e.g.,
actions/upload-artifact@v4 and actions/download-artifact@v4); replace those
tag-pinned references with the exact commit SHAs for the corresponding action
releases (e.g., actions/upload-artifact@<commit-sha>,
actions/download-artifact@<commit-sha>) so the workflow is SHA-pinned; update
every occurrence of these symbols in the file (all uses of
actions/upload-artifact@v4 and actions/download-artifact@v4) and verify the SHAs
by fetching them from the official action repositories' tags/releases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 689a6177-cbe0-4876-bb88-33396063f085
📒 Files selected for processing (13)
.github/workflows/benchmark.ymlcrates/aws-protocol/src/protocol.rscrates/aws-protocol/tests/protocol_integration.rscrates/config/src/services.rscrates/services/cognito/src/provider.rscrates/services/cognito/src/store.rscrates/services/ec2/tests/ec2_perf_tests.rscrates/services/ec2/tests/ec2_tests.rscrates/services/elasticache/src/provider.rscrates/services/opensearch/src/provider.rscrates/services/opensearch/tests/opensearch_tests.rscrates/services/rds/src/provider.rscrates/services/rds/tests/rds_tests.rs
Harden Cognito auth and delete paths, validate OpenSearch tag ARNs, fix ElastiCache delete side effects, and pin benchmark artifact actions. Co-authored-by: Cursor <cursoragent@cursor.com>
Collapse nested if in DeleteCacheCluster and move s3 test module to end of provider.rs to satisfy items-after-test-module lint. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
@coderabbitai full review |
|
✅ Action performedFull review finished. |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 59 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 36
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/services/route53/src/provider.rs (1)
273-277: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTag entries leak when hosted zones or health checks are deleted.
DeleteHostedZone(line 276) removes the zone and its record sets but does not remove associated tags fromstore.tagskeyed by("hostedzone", zone_id). Similarly,DeleteHealthCheck(line 620) removes the health check but leaves any tags keyed by("healthcheck", hc_id). This causes stale tag state to persist across deletions. As per path instructions, new state fields must have corresponding cleanup logic so state does not leak.🔧 Proposed fix
"DeleteHostedZone" => { let zone_id = ctx.path.split('/').next_back().unwrap_or("").to_string(); let mut store = self.store.get_or_create(account_id, ROUTE53_REGION); store.zones.remove(&zone_id); store.records.retain(|(zid, _, _), _| zid != &zone_id); + store.tags.remove(&("hostedzone".to_string(), zone_id));"DeleteHealthCheck" => { let hc_id = ctx.path.split('/').next_back().unwrap_or("").to_string(); let mut store = self.store.get_or_create(account_id, ROUTE53_REGION); if store.health_checks.remove(&hc_id).is_none() { return Ok(xml_error( "NoSuchHealthCheck", &format!("No health check with ID: {hc_id}"), 404, )); } + store.tags.remove(&("healthcheck".to_string(), hc_id));Also applies to: 617-632
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/route53/src/provider.rs` around lines 273 - 277, The deletion handlers are leaving stale tag state behind in the shared Route53 store. Update the `DeleteHostedZone` branch and the `DeleteHealthCheck` branch in `provider.rs` to also remove the corresponding entries from `store.tags` using the same keys used when tags are created, alongside the existing zone/record and health check cleanup. Use the existing `zone_id` and health check ID handling in those match arms so the tag cleanup stays consistent with `store.zones`, `store.records`, and the health check state.Source: Path instructions
crates/services/redshift/src/provider.rs (1)
725-741: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
storage_snapshotomits all new resources and thelogging_enabledfield.The method only serializes
clusterswith three attributes (status, node_type, db_name). After this PR's store changes,snapshots,subnet_groups,parameter_groups, and the newlogging_enabledcluster field are invisible to any consumer ofstorage_snapshot, making state inspection or persistence incomplete.🛡️ Proposed fix to include new resources
async fn storage_snapshot(&self) -> Option<serde_json::Value> { use serde_json::json; let mut clusters = Vec::new(); + let mut snapshots = Vec::new(); + let mut subnet_groups = Vec::new(); + let mut parameter_groups = Vec::new(); for entry in self.store.iter() { for cluster in entry.value().clusters.values() { clusters.push(json!({ "id": cluster.cluster_identifier, "kind": "cluster", "attributes": [ {"key": "status", "value": cluster.cluster_status.clone()}, {"key": "node_type", "value": cluster.node_type.clone()}, {"key": "db_name", "value": cluster.db_name.clone()}, + {"key": "logging_enabled", "value": cluster.logging_enabled.to_string()}, ] })); } + for snapshot in entry.value().snapshots.values() { + snapshots.push(json!({ + "id": snapshot.snapshot_identifier, "kind": "snapshot", + "attributes": [ + {"key": "cluster_identifier", "value": snapshot.cluster_identifier.clone()}, + {"key": "status", "value": snapshot.status.clone()}, + ] + })); + } + for sg in entry.value().subnet_groups.values() { + subnet_groups.push(json!({ + "id": sg.cluster_subnet_group_name, "kind": "subnet_group", + "attributes": [ + {"key": "status", "value": sg.status.clone()}, + {"key": "vpc_id", "value": sg.vpc_id.clone()}, + ] + })); + } + for pg in entry.value().parameter_groups.values() { + parameter_groups.push(json!({ + "id": pg.parameter_group_name, "kind": "parameter_group", + "attributes": [ + {"key": "family", "value": pg.parameter_group_family.clone()}, + ] + })); + } } - Some(json!({ "kind": "redshift", "clusters": clusters })) + Some(json!({ + "kind": "redshift", + "clusters": clusters, + "snapshots": snapshots, + "subnet_groups": subnet_groups, + "parameter_groups": parameter_groups, + })) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/redshift/src/provider.rs` around lines 725 - 741, The `storage_snapshot` implementation in `provider.rs` is only serializing `clusters` with a subset of fields, so update it to include the newly added store resources (`snapshots`, `subnet_groups`, `parameter_groups`) and add the `logging_enabled` field to each cluster’s serialized attributes. Use the existing `storage_snapshot` method and the related store entry/cluster access patterns as the place to extend the JSON payload so consumers see the full Redshift state.crates/services/ec2/src/provider.rs (1)
674-685: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
TerminateInstanceshardcodespreviousStatetorunninginstead of capturing the actual previous state.Unlike
StopInstancesandStartInstanceswhich correctly capturelet prev = inst.state.clone()before mutating and useinstance_state_code(&prev), this arm hardcodes<code>16</code><name>running</name>. If a stopped instance is terminated, the response incorrectly reports its previous state asrunning.🐛 Proposed fix: capture previous state before mutation
for id in &ids { if let Some(inst) = store.instances.get_mut(id) { + let prev = inst.state.clone(); inst.state = "terminated".to_string(); items_xml.push_str(&format!( "<item><instanceId>{}</instanceId>\ <currentState><code>48</code><name>terminated</name></currentState>\ -<previousState><code>16</code><name>running</name></previousState>\ +<previousState><code>{}</code><name>{prev}</name></previousState>\ </item>", - inst.instance_id + inst.instance_id, + instance_state_code(&prev) )); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/ec2/src/provider.rs` around lines 674 - 685, In `TerminateInstances`, the response currently hardcodes `previousState` to running instead of using the instance’s real prior state. Update the loop that mutates `store.instances` to capture `inst.state.clone()` before setting it to terminated, then use that saved value when building the XML, matching the pattern already used in `StopInstances` and `StartInstances` with `instance_state_code`.crates/services/ecs/tests/ecs_tests.rs (1)
1-550: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift14 of 16 operations have happy-path coverage only — error-path tests required.
Per path instructions, every operation must have at least one happy-path test and one error-path test. Only
CreateClusterandDeleteClusterhave both. The following 14 operations lack error-path tests:
- Resource-specific: DescribeClusters, DescribeTaskDefinition, DeregisterTaskDefinition, CreateService, UpdateService, DeleteService, RunTask, StopTask, DescribeTasks
- List operations: ListClusters, ListTaskDefinitions, ListServices, ListTasks
The resource-specific operations clearly need error-path tests (e.g., cluster not found, service not found, task not found). List operations may have less obvious error paths but should still cover invalid input scenarios.
Tests correctly use
make_ctx(), assert on bothstatus_codeand response body content, and use descriptive snake_case names.As per path instructions for
crates/services/*/tests/*_tests.rs: "Flag any operation that has happy-path coverage only."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/ecs/tests/ecs_tests.rs` around lines 1 - 550, Add error-path coverage for the ECS operations that currently only have happy-path tests in ecs_tests.rs. Keep using make_ctx() and EcsProvider::dispatch(), and add focused negative cases for DescribeClusters, DescribeTaskDefinition, DeregisterTaskDefinition, CreateService, UpdateService, DeleteService, RunTask, StopTask, DescribeTasks, ListClusters, ListTaskDefinitions, ListServices, and ListTasks, asserting the expected non-200 status and error type/body. Prefer obvious invalid inputs or missing resources (for example unknown cluster, service, or task identifiers) so each operation has at least one failure test alongside the existing happy-path tests.Source: Path instructions
♻️ Duplicate comments (5)
crates/services/ses/tests/ses_tests.rs (1)
532-551: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing error-path test for
SetIdentityFeedbackForwardingEnabled.The provider has a
MissingParametererror branch whenIdentityis absent (provider line 522), but no test covers it. As per path instructions, every operation with an error branch must have at least one error-path test. This was previously flagged and appears unaddressed.💚 Proposed error-path test
+#[tokio::test] +async fn test_set_identity_feedback_forwarding_enabled_missing_identity() { + let p = SesProvider::new(); + let resp = p + .dispatch(&make_ctx( + "SetIdentityFeedbackForwardingEnabled", + HashMap::new(), + )) + .await + .unwrap(); + assert_eq!(resp.status_code, 400); + let body = body_str(&resp); + assert!(body.contains("MissingParameter")); + assert!(body.contains("Identity required")); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/ses/tests/ses_tests.rs` around lines 532 - 551, Add an error-path test for SesProvider::dispatch covering the MissingParameter branch in SetIdentityFeedbackForwardingEnabled when the Identity parameter is omitted. Extend the existing ses_tests.rs coverage рядом with test_set_identity_feedback_forwarding_enabled by adding a new async tokio test that calls make_ctx("SetIdentityFeedbackForwardingEnabled", ...) without "Identity", then assert the dispatch returns the expected error instead of success. This should directly exercise the SetIdentityFeedbackForwardingEnabled handling in SesProvider and satisfy the required error-branch coverage.Source: Path instructions
crates/services/ssm/tests/ssm_perf_tests.rs (1)
157-180:⚠️ Potential issue | 🟠 MajorMissing perf tests for all new SSM operations.
Per the
crates/services/*/tests/*_perf_tests.rspath instruction, every operation added to the provider must have a corresponding perf test entry. Onlyperf_put_and_delete_parameter_round_tripexists — there are no perf entries forGetParameterHistory,CreateDocument,DeleteDocument,DescribeDocument,GetDocument,ListDocuments,SendCommand,ListCommands,GetCommandInvocation,AddTagsToResource,ListTagsForResource, orRemoveTagsFromResource.As per path instructions, every operation added to the provider must have a corresponding perf test entry in this file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/ssm/tests/ssm_perf_tests.rs` around lines 157 - 180, Add perf coverage for each newly added SSM operation in the perf test suite: `perf_put_and_delete_parameter_round_trip` is present, but `GetParameterHistory`, `CreateDocument`, `DeleteDocument`, `DescribeDocument`, `GetDocument`, `ListDocuments`, `SendCommand`, `ListCommands`, `GetCommandInvocation`, `AddTagsToResource`, `ListTagsForResource`, and `RemoveTagsFromResource` still need matching `#[tokio::test]` perf entries. Extend `crates/services/ssm/tests/ssm_perf_tests.rs` with one round-trip perf test per operation, following the existing `SsmProvider::new()`, `dispatch(...)`, and `assert_eq!(resp.status_code, 200)` pattern so every provider operation is represented.Source: Path instructions
crates/services/ssm/tests/ssm_tests.rs (1)
278-599:⚠️ Potential issue | 🟠 MajorMissing unit tests for tag operations and several error paths.
Per the
crates/services/*/tests/*_tests.rspath instruction, every operation must have at least one happy-path and one error-path test. The following gaps remain:
- No tests at all for
AddTagsToResource,ListTagsForResource,RemoveTagsFromResource.- No error-path test for
DeleteDocument(deleting a non-existent document should returnInvalidDocument).- No error-path test for
GetDocument(requesting a non-existent document).- No error-path test for
SendCommand(missingDocumentNamevalidation).As per path instructions, every operation dispatched in the service's provider.rs must have at least one test covering the happy path and one covering the primary error path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/ssm/tests/ssm_tests.rs` around lines 278 - 599, Add the missing tag-operation coverage in SsmProvider tests by creating happy-path and error-path cases for AddTagsToResource, ListTagsForResource, and RemoveTagsFromResource, using the existing dispatch/make_ctx patterns in ssm_tests.rs. Also add negative tests for DeleteDocument, GetDocument, and SendCommand in the relevant test groups so each operation has a primary failure case: non-existent document deletion, non-existent document lookup, and missing DocumentName validation. Keep the new tests alongside the existing document and command tests so they’re easy to locate by the operation names.Source: Path instructions
crates/services/ssm/src/provider.rs (1)
322-770:⚠️ Potential issue | 🟠 MajorMissing required parity, unit, and perf coverage for new SSM operations.
Per the
crates/services/*/src/provider.rspath instruction, every new match arm must have (1) a parity scenario, (2) a unit test covering happy and error paths, and (3) a perf test withstd::time::Instant. The following gaps remain:
- Unit tests: No tests for
AddTagsToResource,ListTagsForResource,RemoveTagsFromResource; no error-path test forDeleteDocument(deleting non-existent doc); no error-path test forGetDocument(non-existent doc); no error-path test forSendCommand(missingDocumentName).- Perf tests: Only
perf_put_and_delete_parameter_round_tripexists — no perf entries forGetParameterHistory, any document operation, any command operation, or any tag operation.- Parity scenarios: No entries in
tests/parity/scenarios/for any of the 12 new operations.As per coding guidelines and path instructions, each new match arm in a service provider must have a parity test scenario, a unit test covering happy and error paths, and a performance regression test with
std::time::Instant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/ssm/src/provider.rs` around lines 322 - 770, The new SSM match arms in provider.rs are missing required parity, unit, and perf coverage. Add parity scenarios under tests/parity/scenarios for each new operation handled in the match block, and add unit tests covering both success and failure paths for AddTagsToResource, ListTagsForResource, RemoveTagsFromResource, DeleteDocument, GetDocument, and SendCommand in the provider methods. Also add perf tests using std::time::Instant for GetParameterHistory plus the document, command, and tag flows so each new arm has a performance regression check.Sources: Coding guidelines, Path instructions
crates/services/rds/tests/rds_tests.rs (1)
156-180: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMissing error path tests for 8 operations (previously flagged).
The following operations still have only happy-path coverage, as flagged in prior review comments:
ModifyDBInstance(156-180),RebootDBInstance(182-202),DeleteDBSnapshot(269-298),RestoreDBInstanceFromDBSnapshot(300-340),CreateDBSubnetGroup(346-379),DeleteDBSubnetGroup(381-405),CreateDBParameterGroup(411-432),DeleteDBParameterGroup(434-458).Per path instructions, every operation must have at least one error path test (e.g., resource not found, duplicate creation). The PR summary notes these are deferred to a later PR.
Also applies to: 182-202, 269-298, 300-340, 346-379, 381-405, 411-432, 434-458
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/services/rds/tests/rds_tests.rs` around lines 156 - 180, Add at least one negative-path test for each of the still-happy-path-only RDS operations: ModifyDBInstance, RebootDBInstance, DeleteDBSnapshot, RestoreDBInstanceFromDBSnapshot, CreateDBSubnetGroup, DeleteDBSubnetGroup, CreateDBParameterGroup, and DeleteDBParameterGroup. Use the existing test module in rds_tests.rs and the relevant dispatch helpers like RdsProvider::new, make_ctx, and body_str to assert error responses for cases such as missing resources or duplicate creation, so each operation has explicit failure coverage.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aws-protocol/src/protocol.rs`:
- Around line 44-51: The `canonical_service_name` logic is duplicated between
`aws-protocol` and `config`, so update the implementation in `protocol.rs` to
use a shared source of truth instead of keeping a separate copy. Extract the
normalization/mapping behavior from `canonical_service_name` into a shared
utility module or crate, then have both `canonical_service_name` and the
corresponding code in `services.rs` call that shared helper so service detection
and config enablement stay aligned.
- Around line 44-51: The catch-all arm in canonical_service_name is doing an
unnecessary allocation by cloning the already-owned normalized String; update
the match to return the owned value directly in the fallback path. Keep the
special-case mappings for es and cognito/cognito-idp intact, and ensure
AwsProtocol::from_service still receives the same canonicalized output without
extra copying.
In `@crates/config/src/services.rs`:
- Around line 3-10: The catch-all arm in canonical_service_name is doing an
unnecessary allocation by cloning normalized into a new String. Update the match
in canonical_service_name so the default branch moves the existing normalized
value directly instead of calling to_string, mirroring the fix used in
crates/aws-protocol/src/protocol.rs and avoiding the extra copy.
In `@crates/services/cloudtrail/src/provider.rs`:
- Around line 497-508: Add explicit not-found handling in the CloudTrail tag
handlers so missing trails no longer return success: in `AddTags`, `RemoveTags`,
and `ListTags` within `provider.rs`, detect when `store.trails` has no entry for
the requested key(s) and return the appropriate not-found error instead of
falling through to `json_ok`. Reuse the existing lookup patterns in `AddTags`,
`RemoveTags`, and `ListTags`, and make sure `ListTags` either errors when none
of the `ResourceIdList` items resolve or includes an error/not-found result for
missing resource IDs.
- Around line 193-198: The CloudTrail error mapping is inconsistent for
TrailNotFoundException across operations, so align the handlers in the provider
logic to return HTTP 400 everywhere. Update the relevant branches in the
CloudTrail request handlers for GetTrail, GetTrailStatus, StartLogging,
StopLogging, and UpdateTrail to match the existing DeleteTrail behavior by using
json_error with status 400 when a trail is missing.
In `@crates/services/cognito/src/provider.rs`:
- Around line 386-406: `CreateUserPoolClient` has a TOCTOU race because the pool
existence check is done with a separate `self.store.get(...)` guard before the
later `self.store.get_or_create(...)` insert. Move the existence check and the
client insert into the same write-lock scope so the pool cannot be deleted
between validation and mutation; use the same pattern as the other
state-mutating arms in `provider.rs` and keep the logic centered around
`CreateUserPoolClient`/`AdminCreateUser`.
In `@crates/services/cognito/tests/cognito_perf_tests.rs`:
- Around line 32-140: The perf test file only covers a small subset of Cognito
operations; add missing `perf_`-prefixed timing tests in `cognito_perf_tests.rs`
for each provider operation listed in the review, using the existing
`perf_create_user_pool_throughput`, `perf_admin_create_user_throughput`, and
`perf_list_users_many` patterns as a template. For each operation, invoke the
matching `CognitoProvider::dispatch` path with representative input, measure
with `Instant::now()`, and assert a reasonable threshold so every added provider
operation has a corresponding perf test entry.
In `@crates/services/cognito/tests/cognito_tests.rs`:
- Around line 34-57: Add the missing error-path coverage in the Cognito test
suite so each operation has at least one failure case alongside the existing
happy-path tests. Extend the existing test functions in cognito_tests.rs for
CreateUserPool, DeleteUserPool, UpdateUserPool, CreateUserPoolClient,
DescribeUserPoolClient, ListUserPoolClients, DeleteUserPoolClient, ListUsers,
AdminDeleteUser, AdminSetUserPassword, AdminDisableUser, AdminEnableUser, and
AdminUpdateUserAttributes to use CognitoProvider::dispatch with invalid or
missing resources and assert the expected error response. Keep using make_ctx
and body_json, and make the new assertions target the relevant operation names
so the error-path behavior is clearly covered.
In `@crates/services/ec2/src/provider.rs`:
- Around line 87-96: The XML-building paths in ec2 provider responses are
interpolating user-provided strings without escaping, so fix `tags_xml` by
escaping tag keys and values before formatting them into XML. Add a reusable XML
escape helper and apply it anywhere `provider.rs` inserts external input into
response XML, including `tags_xml` and other fields such as `group_name`,
`description`, `key_name`, and similar inline interpolations, so the generated
XML remains well-formed.
- Around line 330-365: DescribeSecurityGroups in provider.rs only serializes
SecurityGroup.ingress_rules into ipPermissions, so add serialization for
sg.egress_rules as ipPermissionsEgress in the same response-building path. Use
the existing mapping pattern around the DescribeSecurityGroups XML assembly to
emit each egress rule’s protocol, from/to ports, and CIDR ranges, so clients can
see the persisted egress_rules alongside ingress_rules.
- Around line 1316-1340: DescribeTags is missing tagged resources for key pairs,
addresses, and internet gateways, so those tags never appear in the response.
Update the DescribeTags assembly logic in provider.rs to iterate over
store.key_pairs, store.addresses, and store.internet_gateways the same way it
already does for store.vpcs, store.subnets, store.security_groups,
store.instances, and store.volumes, and emit matching item entries using the
corresponding IDs and resourceType values.
In `@crates/services/ec2/tests/ec2_perf_tests.rs`:
- Around line 33-57: The helper functions used in ec2 tests are duplicated
across test files and should be shared to prevent drift. Move the repeated
utilities like extract_tag, make_ctx, and body_str into a common test helper
module such as test_helpers or common.rs, then update run_instance and the
existing ec2 test code to call those shared helpers instead of maintaining
separate copies.
In `@crates/services/ecs/src/provider.rs`:
- Line 499: Normalize cluster_ref to a cluster ARN before calling service_arn in
CreateService, DeleteService, UpdateService, and DescribeServices so the
generated ECS service ARN never nests a full cluster ARN inside the service
path. Reuse the already computed c_arn in CreateService for the cluster lookup
instead of recomputing cluster_arn(account_id, region, &cluster_ref), so
active_services_count is updated correctly when cluster_ref is already an ARN.
Focus on the service_arn and cluster_arn usages in provider.rs and keep
name-based and ARN-based cluster references working consistently.
- Around line 717-728: The fallback in the RunTask task-definition resolution
path is returning the raw task_def_ref as if it were a valid ARN when no match
is found, which should be treated as an error instead. Update the lookup logic
in provider.rs around the task definition selection to stop using
unwrap_or_else(|| task_def_ref.clone()) and return a not-found error when the
family or family:revision match fails. Keep the existing ARN passthrough
behavior, but ensure the non-ARN branch in the RunTask flow surfaces an error
from the task definition resolution code path rather than fabricating an invalid
ARN.
In `@crates/services/ecs/tests/ecs_perf_tests.rs`:
- Around line 1-162: Add the missing perf test coverage in ecs_perf_tests by
creating one perf_ test entry for each uncovered ECS operation: DeleteCluster,
DescribeClusters, DescribeTaskDefinition, DeregisterTaskDefinition,
ListTaskDefinitions, CreateService, UpdateService, DeleteService, ListServices,
ListTasks, StopTask, and DescribeTasks. Follow the existing pattern in
perf_create_cluster_throughput, perf_list_clusters_many,
perf_register_task_definition_throughput, and perf_run_task_throughput using
EcsProvider::new, make_ctx, dispatch, Instant timing, and loose duration
assertions. Ensure each test exercises its corresponding operation with valid
request payloads and checks a 200 response where applicable so the file covers
every provider operation.
In `@crates/services/elasticache/src/error.rs`:
- Around line 1-17: The ElastiCache error helper is unused because `provider.rs`
builds `xml_error` responses inline and never references `ElastiCacheError`;
either remove the dead `ElastiCacheError` enum and its `From<ElastiCacheError>
for DispatchError` impl from `error.rs`, or refactor the provider flow to
construct and convert errors through `ElastiCacheError` so it becomes the shared
error layer.
In `@crates/services/elasticache/src/provider.rs`:
- Around line 87-95: The cluster endpoint XML builder is interpolating
CacheClusterId raw into the Address element, so update cluster_endpoint_xml to
XML-escape the id before formatting the endpoint. Reuse the same escaping
approach already used by cluster_xml (xml_escape) and ensure the formatted
Address in cluster_endpoint_xml cannot emit malformed XML from user-provided
cluster IDs.
In `@crates/services/elasticache/tests/elasticache_perf_tests.rs`:
- Around line 1-78: Add perf test coverage for the ElastiCache provider
operations that are currently missing from this test module:
`DeleteCacheCluster`, `ModifyCacheCluster`, `RebootCacheCluster`,
`CreateReplicationGroup`, `DeleteReplicationGroup`, `DescribeReplicationGroups`,
`ModifyReplicationGroup`, `CreateCacheSubnetGroup`, `DeleteCacheSubnetGroup`,
and `DescribeCacheSubnetGroups`. Follow the existing
`perf_create_cache_cluster_throughput` and `perf_describe_cache_clusters_many`
patterns in this file by adding `#[tokio::test]` functions with `perf_`
prefixes, using `Instant` timing, `make_ctx`, and `body_str` assertions as
appropriate. Use `ElastiCacheProvider::new()` and `dispatch(...)` for each
operation so every provider operation from `provider.rs` has a corresponding
perf test entry.
In `@crates/services/opensearch/src/provider.rs`:
- Around line 90-112: The `ServiceSoftwareOptions` JSON mapping is duplicated in
`domain_status_json`, `StartServiceSoftwareUpdate`, and
`CancelServiceSoftwareUpdate`, and the latter two are cloning the whole struct
unnecessarily. Extract a shared helper for the `ServiceSoftwareOptions` → JSON
conversion in `provider.rs` and have `domain_status_json` and both service
update handlers call it directly from the existing `domain` borrow instead of
cloning. Ensure the helper is reused consistently so any future field changes
only need one update.
In `@crates/services/rds/src/provider.rs`:
- Around line 56-69: The xml_error helper is interpolating the message parameter
directly into the XML response without escaping, unlike instance_xml and
snapshot_xml which already use xml_escape. Update xml_error to XML-escape
message before formatting the ErrorResponse, and keep the fix localized to
xml_error so any caller passing user-derived values like db_id, snapshot_id, or
name is rendered safely.
- Around line 420-458: CreateDBSnapshot has a TOCTOU race because it reads from
self.store multiple times before inserting the snapshot. Refactor the
CreateDBSnapshot flow to use a single self.store.get_or_create(account_id,
region) acquisition, then perform the snapshot-exists check, DB instance lookup,
and insert against that same store reference. Keep the logic aligned with the
atomic patterns already used in CreateDBInstance,
RestoreDBInstanceFromDBSnapshot, and CreateDBSubnetGroup.
- Around line 294-298: Update all RDS NotFound XML responses in provider.rs to
return 404 instead of 400, matching the existing behavior in CreateDBSnapshot.
Review the handlers that emit DBInstanceNotFound, DBSnapshotNotFound,
DBSubnetGroupNotFoundFault, and DBParameterGroupNotFound, including the logic in
DeleteDBInstance, ModifyDBInstance, RebootDBInstance, DeleteDBSnapshot,
RestoreDBInstanceFromDBSnapshot, DeleteDBSubnetGroup, and
DeleteDBParameterGroup, and change each xml_error call to use the correct 404
status.
In `@crates/services/rds/tests/rds_perf_tests.rs`:
- Around line 1-130: Add perf test coverage for the missing RDS operations by
extending rds_perf_tests.rs with new perf_ tests that follow the existing
Instant/timing/assertion pattern used in perf_create_db_instance_throughput,
perf_describe_db_instances_many, and perf_create_snapshot_throughput. Use
RdsProvider::new(), make_ctx(), and dispatch() to cover DeleteDBInstance,
ModifyDBInstance, RebootDBInstance, DescribeDBSnapshots, DeleteDBSnapshot,
RestoreDBInstanceFromDBSnapshot, CreateDBSubnetGroup, DescribeDBSubnetGroups,
DeleteDBSubnetGroup, CreateDBParameterGroup, DescribeDBParameterGroups, and
DeleteDBParameterGroup so every provider operation has a corresponding perf
entry.
In `@crates/services/redshift/src/provider.rs`:
- Around line 393-409: CreateClusterSnapshot currently falls back to default
cluster values when the requested ClusterIdentifier is missing, which makes
non-existent clusters look valid. Update the snapshot creation path in
provider.rs to return ClusterNotFound from the CreateClusterSnapshot logic
instead of using the dc2.large/dev/admin defaults, and keep the behavior aligned
with EnableLogging/DisableLogging. Adjust any tests that depend on the fallback,
including test_create_snapshot_duplicate_fails and
test_describe_snapshots_filtered_by_cluster, so they create a real cluster
before exercising snapshot behavior.
In `@crates/services/route53/src/provider.rs`:
- Around line 492-497: The DNS suffix check in the ListHostedZonesByName filter
is too loose because the current ends_with comparison can match partial labels
like notexample.com for example.com. Update the filtering logic in the hosted
zone listing path to perform label-boundary-aware matching against
dns_name_filter, allowing only exact matches or true subdomains, and keep the
behavior localized around the z.name trimming/filtering in provider.rs.
In `@crates/services/route53/src/store.rs`:
- Around line 64-67: The new state in the Route53 store grows without cleanup,
so add eviction in the delete flow to keep `health_checks` and `tags` from
leaking across runs. Update the provider delete handlers that process resource
removals, especially `DeleteHostedZone` and `DeleteHealthCheck`, and use the
store’s existing keys to remove the matching entries from `tags` (and any
associated `health_checks` state if applicable). Keep the cleanup close to the
deletion logic so the `Store` struct and its resource lifecycle stay in sync.
In `@crates/services/route53/tests/route53_tests.rs`:
- Around line 668-713: Add coverage for the untested RemoveTagKeys branch in
Route53Provider’s tag flow by extending the existing tag test or adding a new
one around ChangeTagsForResource and ListTagsForResource. Create a hosted zone,
add multiple tags, then send a ChangeTagsForResource request using RemoveTagKeys
with one of the keys, and verify via ListTagsForResource that the removed key no
longer appears while the remaining tag still does. Use the existing helpers like
make_ctx, body_str, and xml_text to keep the test aligned with the current
Route53 test patterns.
- Around line 652-661: The `test_delete_health_check` check for the
post-deletion `GetHealthCheck` response only verifies `status_code` and misses
the required body assertion. Update the assertion around the
`p.dispatch(...GetHealthCheck...)` response to also validate the response body
contains `NoSuchHealthCheck`, keeping the existing 404 status check so the test
confirms both the error code and the expected error payload.
In `@crates/services/ses/src/provider.rs`:
- Around line 382-392: DeleteTemplate currently removes from store.templates and
always returns success even when the template is missing. Update the
DeleteTemplate branch in provider.rs to check whether the named template exists
before removal, and when it does not, return the SES-specific error response for
TemplateDoesNotExist instead of xml_no_result. Use the existing helpers around
xml_error/str_param and the DeleteTemplate match arm to add the missing error
branch while preserving the successful path for existing templates.
- Around line 308-332: `GetSendStatistics` is generating the `<Timestamp>` from
query time instead of the emails’ actual send time, so the stats are unstable
and inaccurate. Update the `GetSendStatistics` branch in `provider.rs` to derive
the timestamp from the stored emails’ `sent_at` values, ideally using the most
recent email or grouping by the hour bucket from `sent_at`. Keep the existing
`store.get(account_id, region)` flow, but replace the `Utc::now()`-based
timestamp inside the `format!` with logic based on the email data so repeated
calls return a stable, realistic timestamp.
In `@crates/services/ssm/src/provider.rs`:
- Around line 706-769: The tag handlers are silently returning success for
unsupported resource types or missing Documents, so update AddTagsToResource,
RemoveTagsFromResource, and ListTagsForResource to validate the target before
touching store.documents. Reuse the same InvalidDocument error pattern already
used by DeleteDocument, DescribeDocument, and GetDocument: if ResourceType is
not "Document" or the document is absent, return that error instead of json_ok.
Keep the existing tag mutation/query logic in the AddTagsToResource,
RemoveTagsFromResource, and ListTagsForResource match arms, but add the
existence guard before the store access.
In `@crates/services/ssm/src/store.rs`:
- Around line 96-105: SsmStore’s in-memory state has no cleanup for deleted or
long-lived entries, so update the store management logic to remove stale
`parameter_history` when `DeleteParameter` and `DeleteParameters` run, using the
existing `SsmStore` fields and the delete handlers in `provider.rs`. Also add
bounded retention or eviction for the version lists in `parameter_history`, and
introduce a cleanup/expiry path for `commands` so both collections cannot grow
indefinitely in long-running instances.
In `@crates/services/sts/src/provider.rs`:
- Around line 170-178: The XML built in AssumeRoleWithWebIdentity and
AssumeRoleWithSAML is still inserting user-controlled values without escaping,
which can break the response. Update the response construction in provider.rs so
every interpolated field inside the XML uses xml_escape(), including subject,
session_name, and role_name in the web identity handler, and role_name in the
SAML handler; keep provider_id and role_arn escaped as well. Use the existing
helper patterns in the AssumeRole response assembly to locate and apply the fix
consistently.
- Around line 164-166: The WebIdentityToken truncation in the subject
construction can panic because it slices by byte index inside the provider
logic. Update the `subject` derivation in `provider.rs` to truncate
`WebIdentityToken` using a char-based approach instead of
`t[..t.len().min(12)]`, and keep the existing fallback behavior in the
`param(ctx, "WebIdentityToken")` chain unchanged. Use the `subject` local in
this STS provider flow as the place to fix the UTF-8-safe truncation.
- Line 195: The SAML flow in provider.rs hardcodes the session name in the
AssumeRoleWithSAML path instead of honoring the request parameter. Update the
logic around the session_name assignment in the SAML handler to read
RoleSessionName from the incoming request, following the same pattern used by
the AssumeRole and AssumeRoleWithWebIdentity handlers, and preserve the existing
fallback behavior only if the parameter is absent.
In `@crates/tests/integration/src/parity.rs`:
- Around line 139-143: The `all_service_names()` list in
`crates/tests/integration/src/parity.rs` contains duplicate service entries,
specifically the later `ecs`, `rds`, `cognito`, `elasticache`, and `cloudtrail`
entries that already appear earlier in the same `Vec<String>`. Remove the
repeated items so `all_service_names()` returns a unique service list, since
this vector is used directly by the `all-services-smoke` profile and duplicate
entries cause redundant service startup work. Keep the earlier canonical entries
and verify the parity inventory still matches the list expected by
`readme_supported_services_match_all_services_smoke_inventory`.
---
Outside diff comments:
In `@crates/services/ec2/src/provider.rs`:
- Around line 674-685: In `TerminateInstances`, the response currently hardcodes
`previousState` to running instead of using the instance’s real prior state.
Update the loop that mutates `store.instances` to capture `inst.state.clone()`
before setting it to terminated, then use that saved value when building the
XML, matching the pattern already used in `StopInstances` and `StartInstances`
with `instance_state_code`.
In `@crates/services/ecs/tests/ecs_tests.rs`:
- Around line 1-550: Add error-path coverage for the ECS operations that
currently only have happy-path tests in ecs_tests.rs. Keep using make_ctx() and
EcsProvider::dispatch(), and add focused negative cases for DescribeClusters,
DescribeTaskDefinition, DeregisterTaskDefinition, CreateService, UpdateService,
DeleteService, RunTask, StopTask, DescribeTasks, ListClusters,
ListTaskDefinitions, ListServices, and ListTasks, asserting the expected non-200
status and error type/body. Prefer obvious invalid inputs or missing resources
(for example unknown cluster, service, or task identifiers) so each operation
has at least one failure test alongside the existing happy-path tests.
In `@crates/services/redshift/src/provider.rs`:
- Around line 725-741: The `storage_snapshot` implementation in `provider.rs` is
only serializing `clusters` with a subset of fields, so update it to include the
newly added store resources (`snapshots`, `subnet_groups`, `parameter_groups`)
and add the `logging_enabled` field to each cluster’s serialized attributes. Use
the existing `storage_snapshot` method and the related store entry/cluster
access patterns as the place to extend the JSON payload so consumers see the
full Redshift state.
In `@crates/services/route53/src/provider.rs`:
- Around line 273-277: The deletion handlers are leaving stale tag state behind
in the shared Route53 store. Update the `DeleteHostedZone` branch and the
`DeleteHealthCheck` branch in `provider.rs` to also remove the corresponding
entries from `store.tags` using the same keys used when tags are created,
alongside the existing zone/record and health check cleanup. Use the existing
`zone_id` and health check ID handling in those match arms so the tag cleanup
stays consistent with `store.zones`, `store.records`, and the health check
state.
---
Duplicate comments:
In `@crates/services/rds/tests/rds_tests.rs`:
- Around line 156-180: Add at least one negative-path test for each of the
still-happy-path-only RDS operations: ModifyDBInstance, RebootDBInstance,
DeleteDBSnapshot, RestoreDBInstanceFromDBSnapshot, CreateDBSubnetGroup,
DeleteDBSubnetGroup, CreateDBParameterGroup, and DeleteDBParameterGroup. Use the
existing test module in rds_tests.rs and the relevant dispatch helpers like
RdsProvider::new, make_ctx, and body_str to assert error responses for cases
such as missing resources or duplicate creation, so each operation has explicit
failure coverage.
In `@crates/services/ses/tests/ses_tests.rs`:
- Around line 532-551: Add an error-path test for SesProvider::dispatch covering
the MissingParameter branch in SetIdentityFeedbackForwardingEnabled when the
Identity parameter is omitted. Extend the existing ses_tests.rs coverage рядом
with test_set_identity_feedback_forwarding_enabled by adding a new async tokio
test that calls make_ctx("SetIdentityFeedbackForwardingEnabled", ...) without
"Identity", then assert the dispatch returns the expected error instead of
success. This should directly exercise the SetIdentityFeedbackForwardingEnabled
handling in SesProvider and satisfy the required error-branch coverage.
In `@crates/services/ssm/src/provider.rs`:
- Around line 322-770: The new SSM match arms in provider.rs are missing
required parity, unit, and perf coverage. Add parity scenarios under
tests/parity/scenarios for each new operation handled in the match block, and
add unit tests covering both success and failure paths for AddTagsToResource,
ListTagsForResource, RemoveTagsFromResource, DeleteDocument, GetDocument, and
SendCommand in the provider methods. Also add perf tests using
std::time::Instant for GetParameterHistory plus the document, command, and tag
flows so each new arm has a performance regression check.
In `@crates/services/ssm/tests/ssm_perf_tests.rs`:
- Around line 157-180: Add perf coverage for each newly added SSM operation in
the perf test suite: `perf_put_and_delete_parameter_round_trip` is present, but
`GetParameterHistory`, `CreateDocument`, `DeleteDocument`, `DescribeDocument`,
`GetDocument`, `ListDocuments`, `SendCommand`, `ListCommands`,
`GetCommandInvocation`, `AddTagsToResource`, `ListTagsForResource`, and
`RemoveTagsFromResource` still need matching `#[tokio::test]` perf entries.
Extend `crates/services/ssm/tests/ssm_perf_tests.rs` with one round-trip perf
test per operation, following the existing `SsmProvider::new()`,
`dispatch(...)`, and `assert_eq!(resp.status_code, 200)` pattern so every
provider operation is represented.
In `@crates/services/ssm/tests/ssm_tests.rs`:
- Around line 278-599: Add the missing tag-operation coverage in SsmProvider
tests by creating happy-path and error-path cases for AddTagsToResource,
ListTagsForResource, and RemoveTagsFromResource, using the existing
dispatch/make_ctx patterns in ssm_tests.rs. Also add negative tests for
DeleteDocument, GetDocument, and SendCommand in the relevant test groups so each
operation has a primary failure case: non-existent document deletion,
non-existent document lookup, and missing DocumentName validation. Keep the new
tests alongside the existing document and command tests so they’re easy to
locate by the operation names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f52f429e-a969-406a-8687-0560a4d08b0e
⛔ Files ignored due to path filters (23)
.opencode/package-lock.jsonis excluded by!**/package-lock.jsonand included by noneCLAUDE.mdis excluded by none and included by noneCargo.lockis excluded by!**/*.lockand included byCargo.lockDockerfileis excluded by none and included by nonecrates/openstack/Cargo.tomlis excluded by none and included by nonecrates/services/cloudtrail/CHANGELOG.mdis excluded by none and included by nonecrates/services/cloudtrail/Cargo.tomlis excluded by none and included by nonecrates/services/cognito/CHANGELOG.mdis excluded by none and included by nonecrates/services/cognito/Cargo.tomlis excluded by none and included by nonecrates/services/ecs/CHANGELOG.mdis excluded by none and included by nonecrates/services/ecs/Cargo.tomlis excluded by none and included by nonecrates/services/elasticache/CHANGELOG.mdis excluded by none and included by nonecrates/services/elasticache/Cargo.tomlis excluded by none and included by nonecrates/services/rds/CHANGELOG.mdis excluded by none and included by nonecrates/services/rds/Cargo.tomlis excluded by none and included by nonecrates/tests/integration/Cargo.tomlis excluded by none and included by nonemanifests/guided/cloudtrail.guided.jsonis excluded by none and included by nonemanifests/guided/cognito.guided.jsonis excluded by none and included by nonemanifests/guided/ecs.guided.jsonis excluded by none and included by nonemanifests/guided/elasticache.guided.jsonis excluded by none and included by nonemanifests/guided/rds.guided.jsonis excluded by none and included by nonetests/bench/bench_services.shis excluded by none and included by nonetests/harness/service-matrix.jsonis excluded by none and included by none
📒 Files selected for processing (73)
.github/workflows/benchmark.yml.github/workflows/ci.ymlCargo.tomlcrates/aws-protocol/src/protocol.rscrates/aws-protocol/tests/protocol_integration.rscrates/config/src/services.rscrates/gateway/src/server.rscrates/openstack/src/main.rscrates/services/apigateway/tests/apigateway_perf_tests.rscrates/services/cloudformation/tests/cloudformation_perf_tests.rscrates/services/cloudtrail/src/error.rscrates/services/cloudtrail/src/lib.rscrates/services/cloudtrail/src/provider.rscrates/services/cloudtrail/src/store.rscrates/services/cloudtrail/tests/cloudtrail_perf_tests.rscrates/services/cloudtrail/tests/cloudtrail_tests.rscrates/services/cognito/src/error.rscrates/services/cognito/src/lib.rscrates/services/cognito/src/provider.rscrates/services/cognito/src/store.rscrates/services/cognito/tests/cognito_perf_tests.rscrates/services/cognito/tests/cognito_tests.rscrates/services/ec2/src/provider.rscrates/services/ec2/src/store.rscrates/services/ec2/tests/ec2_perf_tests.rscrates/services/ec2/tests/ec2_tests.rscrates/services/ecs/src/error.rscrates/services/ecs/src/lib.rscrates/services/ecs/src/provider.rscrates/services/ecs/src/store.rscrates/services/ecs/tests/ecs_perf_tests.rscrates/services/ecs/tests/ecs_tests.rscrates/services/elasticache/src/error.rscrates/services/elasticache/src/lib.rscrates/services/elasticache/src/provider.rscrates/services/elasticache/src/store.rscrates/services/elasticache/tests/elasticache_perf_tests.rscrates/services/elasticache/tests/elasticache_tests.rscrates/services/firehose/tests/firehose_perf_tests.rscrates/services/iam/tests/iam_perf_tests.rscrates/services/kinesis/tests/kinesis_perf_tests.rscrates/services/opensearch/src/provider.rscrates/services/opensearch/src/store.rscrates/services/opensearch/tests/opensearch_perf_tests.rscrates/services/opensearch/tests/opensearch_tests.rscrates/services/rds/src/error.rscrates/services/rds/src/lib.rscrates/services/rds/src/provider.rscrates/services/rds/src/store.rscrates/services/rds/tests/rds_perf_tests.rscrates/services/rds/tests/rds_tests.rscrates/services/redshift/src/provider.rscrates/services/redshift/src/store.rscrates/services/redshift/tests/redshift_perf_tests.rscrates/services/redshift/tests/redshift_tests.rscrates/services/route53/src/provider.rscrates/services/route53/src/store.rscrates/services/route53/tests/route53_tests.rscrates/services/s3/src/object_store.rscrates/services/s3/src/provider.rscrates/services/ses/src/provider.rscrates/services/ses/src/store.rscrates/services/ses/tests/ses_tests.rscrates/services/sns/tests/sns_perf_tests.rscrates/services/ssm/src/provider.rscrates/services/ssm/src/store.rscrates/services/ssm/tests/ssm_perf_tests.rscrates/services/ssm/tests/ssm_tests.rscrates/services/stepfunctions/tests/stepfunctions_perf_tests.rscrates/services/sts/src/provider.rscrates/services/sts/tests/sts_tests.rscrates/tests/integration/src/harness.rscrates/tests/integration/src/parity.rs
Comment on lines
+44
to
+51
| fn canonical_service_name(service: &str) -> String { | ||
| let normalized = service.trim().to_ascii_lowercase().replace('_', "-"); | ||
| match normalized.as_str() { | ||
| "es" => "opensearch".to_string(), | ||
| "cognito" | "cognito-idp" => "cognito-idp".to_string(), | ||
| other => other.to_string(), | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
canonical_service_name is duplicated across aws-protocol and config crates.
The identical function exists in both crates/aws-protocol/src/protocol.rs and crates/config/src/services.rs. Divergent edits to one copy could cause inconsistent service detection vs. config enablement behavior. Consider extracting to a shared utility module or crate that both can depend on.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/aws-protocol/src/protocol.rs` around lines 44 - 51, The
`canonical_service_name` logic is duplicated between `aws-protocol` and
`config`, so update the implementation in `protocol.rs` to use a shared source
of truth instead of keeping a separate copy. Extract the normalization/mapping
behavior from `canonical_service_name` into a shared utility module or crate,
then have both `canonical_service_name` and the corresponding code in
`services.rs` call that shared helper so service detection and config enablement
stay aligned.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Avoid unnecessary allocation in canonical_service_name catch-all arm.
other => other.to_string() clones the string data when normalized is already an owned String that can be moved directly. This function is called on every request via AwsProtocol::from_service, so the extra allocation is on a hot path.
As per coding guidelines for **/*.rs: flag unnecessary cloning or allocations.
⚡ Proposed fix
fn canonical_service_name(service: &str) -> String {
let normalized = service.trim().to_ascii_lowercase().replace('_', "-");
match normalized.as_str() {
"es" => "opensearch".to_string(),
"cognito" | "cognito-idp" => "cognito-idp".to_string(),
- other => other.to_string(),
+ _ => normalized,
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/aws-protocol/src/protocol.rs` around lines 44 - 51, The catch-all arm
in canonical_service_name is doing an unnecessary allocation by cloning the
already-owned normalized String; update the match to return the owned value
directly in the fallback path. Keep the special-case mappings for es and
cognito/cognito-idp intact, and ensure AwsProtocol::from_service still receives
the same canonicalized output without extra copying.
Source: Coding guidelines
Comment on lines
+3
to
+10
| fn canonical_service_name(service: &str) -> String { | ||
| let normalized = service.trim().to_ascii_lowercase().replace('_', "-"); | ||
| match normalized.as_str() { | ||
| "es" => "opensearch".to_string(), | ||
| "cognito" | "cognito-idp" => "cognito-idp".to_string(), | ||
| other => other.to_string(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Avoid unnecessary allocation in canonical_service_name catch-all arm.
Same issue as crates/aws-protocol/src/protocol.rs: other => other.to_string() clones when normalized can be moved directly.
As per coding guidelines for **/*.rs: flag unnecessary cloning or allocations.
⚡ Proposed fix
fn canonical_service_name(service: &str) -> String {
let normalized = service.trim().to_ascii_lowercase().replace('_', "-");
match normalized.as_str() {
"es" => "opensearch".to_string(),
"cognito" | "cognito-idp" => "cognito-idp".to_string(),
- other => other.to_string(),
+ _ => normalized,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn canonical_service_name(service: &str) -> String { | |
| let normalized = service.trim().to_ascii_lowercase().replace('_', "-"); | |
| match normalized.as_str() { | |
| "es" => "opensearch".to_string(), | |
| "cognito" | "cognito-idp" => "cognito-idp".to_string(), | |
| other => other.to_string(), | |
| } | |
| } | |
| fn canonical_service_name(service: &str) -> String { | |
| let normalized = service.trim().to_ascii_lowercase().replace('_', "-"); | |
| match normalized.as_str() { | |
| "es" => "opensearch".to_string(), | |
| "cognito" | "cognito-idp" => "cognito-idp".to_string(), | |
| _ => normalized, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/config/src/services.rs` around lines 3 - 10, The catch-all arm in
canonical_service_name is doing an unnecessary allocation by cloning normalized
into a new String. Update the match in canonical_service_name so the default
branch moves the existing normalized value directly instead of calling
to_string, mirroring the fix used in crates/aws-protocol/src/protocol.rs and
avoiding the extra copy.
Source: Coding guidelines
Comment on lines
+193
to
+198
| if store.trails.remove(&key).is_none() { | ||
| return Ok(json_error( | ||
| "TrailNotFoundException", | ||
| &format!("Trail {name} not found"), | ||
| 400, | ||
| )); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
AWS CloudTrail API TrailNotFoundException HTTP status code DeleteTrail GetTrail
💡 Result:
The AWS CloudTrail TrailNotFoundException typically corresponds to an HTTP 400 status code [1][2][3][4]. This exception is thrown by various CloudTrail API operations, including DeleteTrail and GetTrail, when the specified trail name or ARN cannot be found [1][5][6][3][4]. While some third-party API specifications or general REST mapping patterns may sometimes associate "Not Found" errors with HTTP 404 [7], official AWS CloudTrail API documentation consistently identifies TrailNotFoundException as having an HTTP status code of 400 [1][2][3][4].
Citations:
- 1: https://docs.aws.amazon.com/awscloudtrail/latest/APIReference/API_GetTrailStatus.html
- 2: https://docs.aws.amazon.com/awscloudtrail/latest/APIReference/API_UpdateTrail.html
- 3: https://docs.aws.amazon.com/awscloudtrail/latest/APIReference/API_DeleteTrail.html
- 4: https://docs.aws.amazon.com/awscloudtrail/latest/APIReference/API_GetTrail.html
- 5: https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/services/cloudtrail/model/TrailNotFoundException.html
- 6: https://docs.aws.amazon.com/botocore/latest/reference/services/cloudtrail/client/exceptions/TrailNotFoundException.html
- 7: https://github.com/api-evangelist/amazon-cloudtrail/blob/main/openapi/amazon-cloudtrail-openapi.yml
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant CloudTrail provider section with line numbers.
sed -n '180,440p' crates/services/cloudtrail/src/provider.rs | cat -n
# Find all TrailNotFoundException occurrences in the CloudTrail provider.
rg -n 'TrailNotFoundException|json_error\(' crates/services/cloudtrail/src/provider.rsRepository: JesseKoldewijn/openstack
Length of output: 14137
Use HTTP 400 for TrailNotFoundException across CloudTrail operations
DeleteTrail already returns 400; bring GetTrail, GetTrailStatus, StartLogging, StopLogging, and UpdateTrail in line with the CloudTrail API mapping so the same error doesn’t alternate between 400 and 404.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/cloudtrail/src/provider.rs` around lines 193 - 198, The
CloudTrail error mapping is inconsistent for TrailNotFoundException across
operations, so align the handlers in the provider logic to return HTTP 400
everywhere. Update the relevant branches in the CloudTrail request handlers for
GetTrail, GetTrailStatus, StartLogging, StopLogging, and UpdateTrail to match
the existing DeleteTrail behavior by using json_error with status 400 when a
trail is missing.
Comment on lines
+497
to
+508
| let mut store = self.store.get_or_create(account_id, region); | ||
| if let Some(trail) = store.trails.get_mut(&key) { | ||
| for tag in &tag_list { | ||
| if let (Some(k), Some(v)) = ( | ||
| tag.get("Key").and_then(|v| v.as_str()), | ||
| tag.get("Value").and_then(|v| v.as_str()), | ||
| ) { | ||
| trail.tags.insert(k.to_string(), v.to_string()); | ||
| } | ||
| } | ||
| } | ||
| Ok(json_ok(json!({}))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tag operations silently succeed on non-existent trails.
AddTags, RemoveTags, and ListTags all silently return a 200 response when the referenced trail doesn't exist. AddTags and RemoveTags use if let Some(trail) = store.trails.get_mut(&key) and fall through to Ok(json_ok(json!({}))) on miss. ListTags uses filter_map which silently skips non-existent resource IDs. AWS CloudTrail returns TrailNotFoundException (or ResourceNotFoundException) for these cases.
🐛 Proposed fix for AddTags and RemoveTags
"AddTags" => {
// ... existing ResourceId / TagsList extraction ...
let mut store = self.store.get_or_create(account_id, region);
- if let Some(trail) = store.trails.get_mut(&key) {
- for tag in &tag_list {
- if let (Some(k), Some(v)) = (
- tag.get("Key").and_then(|v| v.as_str()),
- tag.get("Value").and_then(|v| v.as_str()),
- ) {
- trail.tags.insert(k.to_string(), v.to_string());
+ match store.trails.get_mut(&key) {
+ Some(trail) => {
+ for tag in &tag_list {
+ if let (Some(k), Some(v)) = (
+ tag.get("Key").and_then(|v| v.as_str()),
+ tag.get("Value").and_then(|v| v.as_str()),
+ ) {
+ trail.tags.insert(k.to_string(), v.to_string());
+ }
}
+ Ok(json_ok(json!({})))
}
+ None => Ok(json_error(
+ "TrailNotFoundException",
+ &format!("Trail {resource_id} not found"),
+ 400,
+ )),
}
- Ok(json_ok(json!({})))
}Apply the same pattern to RemoveTags (lines 574-580). For ListTags, return an error when none of the provided ResourceIdList entries match an existing trail, or at minimum include a ResourceId that was not found in the response.
Also applies to: 527-542, 574-580
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/cloudtrail/src/provider.rs` around lines 497 - 508, Add
explicit not-found handling in the CloudTrail tag handlers so missing trails no
longer return success: in `AddTags`, `RemoveTags`, and `ListTags` within
`provider.rs`, detect when `store.trails` has no entry for the requested key(s)
and return the appropriate not-found error instead of falling through to
`json_ok`. Reuse the existing lookup patterns in `AddTags`, `RemoveTags`, and
`ListTags`, and make sure `ListTags` either errors when none of the
`ResourceIdList` items resolve or includes an error/not-found result for missing
resource IDs.
Comment on lines
+386
to
+406
| // Verify pool exists | ||
| { | ||
| let Some(store) = self.store.get(account_id, region) else { | ||
| return Ok(json_error( | ||
| "ResourceNotFoundException", | ||
| &format!("User pool {pool_id} not found"), | ||
| 400, | ||
| )); | ||
| }; | ||
| if !store.user_pools.contains_key(&pool_id) { | ||
| return Ok(json_error( | ||
| "ResourceNotFoundException", | ||
| &format!("User pool {pool_id} not found"), | ||
| 400, | ||
| )); | ||
| } | ||
| } | ||
| let mut store = self.store.get_or_create(account_id, region); | ||
| store.clients.insert(client_id, client.clone()); | ||
| Ok(json_ok(json!({ "UserPoolClient": client_json(&client) }))) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
TOCTOU race between existence checks and inserts in CreateUserPoolClient/AdminCreateUser.
Both arms check preconditions (pool exists / username not already present) under a self.store.get(...) read guard that is scoped and dropped, then acquire a separate self.store.get_or_create(...) write guard to perform the insert. Between the two acquisitions, a concurrent DeleteUserPool can remove the pool (leaving an orphaned client/user), or a concurrent duplicate AdminCreateUser for the same username can pass the contains_key check before either insert lands, silently overwriting instead of raising UsernameExistsException. Other arms in this file (e.g. CreateUserPool, DeleteUserPool) correctly perform check-then-mutate under a single lock acquisition — these two arms should follow the same pattern.
🔒 Proposed fix: perform check-and-insert under a single write lock
- // Verify pool exists
- {
- let Some(store) = self.store.get(account_id, region) else {
- return Ok(json_error(
- "ResourceNotFoundException",
- &format!("User pool {pool_id} not found"),
- 400,
- ));
- };
- if !store.user_pools.contains_key(&pool_id) {
- return Ok(json_error(
- "ResourceNotFoundException",
- &format!("User pool {pool_id} not found"),
- 400,
- ));
- }
- }
- let mut store = self.store.get_or_create(account_id, region);
+ let mut store = self.store.get_or_create(account_id, region);
+ if !store.user_pools.contains_key(&pool_id) {
+ return Ok(json_error(
+ "ResourceNotFoundException",
+ &format!("User pool {pool_id} not found"),
+ 400,
+ ));
+ }
store.clients.insert(client_id, client.clone());Apply the analogous change to the AdminCreateUser arm (fold the pool-existence and store.users.contains_key(&key) checks into the same get_or_create write-lock scope used for the insert).
Based on path instructions for crates/services/*/src/store.rs: "Review for thread-safety of state mutations... flag any mutation that bypasses the existing locking or interior-mutability pattern," and for **/*.rs: "Flag error handling that drops context, unnecessary cloning or allocations, blocking operations in async paths."
Also applies to: 571-613
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/cognito/src/provider.rs` around lines 386 - 406,
`CreateUserPoolClient` has a TOCTOU race because the pool existence check is
done with a separate `self.store.get(...)` guard before the later
`self.store.get_or_create(...)` insert. Move the existence check and the client
insert into the same write-lock scope so the pool cannot be deleted between
validation and mutation; use the same pattern as the other state-mutating arms
in `provider.rs` and keep the logic centered around
`CreateUserPoolClient`/`AdminCreateUser`.
Source: Path instructions
Comment on lines
96
to
+105
| #[derive(Debug, Default, Clone, Serialize, Deserialize)] | ||
| pub struct SsmStore { | ||
| /// parameter name → Parameter | ||
| pub parameters: HashMap<String, Parameter>, | ||
| /// parameter name → version history (sorted by version) | ||
| pub parameter_history: HashMap<String, Vec<Parameter>>, | ||
| /// document name → Document | ||
| pub documents: HashMap<String, Document>, | ||
| /// command_id → Command | ||
| pub commands: HashMap<String, Command>, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
parameter_history and commands grow without bound and lack cleanup logic.
Two concerns per the crates/services/*/src/store.rs path instruction:
-
Orphaned
parameter_history:DeleteParameterandDeleteParametersinprovider.rs(lines 254–296) remove entries fromparametersbut never clean upparameter_history. The orphaned history is inaccessible (GetParameterHistoryreturnsParameterNotFoundfor deleted parameters) yet persists in memory. -
Unbounded collections:
parameter_historyaccumulates versions per parameter without a version cap, andcommandshas no eviction or deletion path. Both grow without limit in long-running instances.
As per path instructions, ensure new state fields have corresponding cleanup logic so state does not leak, and flag unbounded collections without eviction or limit enforcement.
🔧 Proposed fix: clean up parameter_history on deletion
In provider.rs, DeleteParameter (around line 260):
if store.parameters.remove(&name).is_none() {
return Ok(json_error("ParameterNotFound", &format!("Parameter {name} not found"), 400));
}
+ store.parameter_history.remove(&name);
Ok(json_ok(json!({})))And in DeleteParameters (around line 285):
if store.parameters.remove(name).is_some() {
deleted.push(json!(name));
+ store.parameter_history.remove(name);
} else {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/ssm/src/store.rs` around lines 96 - 105, SsmStore’s in-memory
state has no cleanup for deleted or long-lived entries, so update the store
management logic to remove stale `parameter_history` when `DeleteParameter` and
`DeleteParameters` run, using the existing `SsmStore` fields and the delete
handlers in `provider.rs`. Also add bounded retention or eviction for the
version lists in `parameter_history`, and introduce a cleanup/expiry path for
`commands` so both collections cannot grow indefinitely in long-running
instances.
Source: Path instructions
Comment on lines
+164
to
+166
| let subject = param(ctx, "WebIdentityToken") | ||
| .map(|t| t[..t.len().min(12)].to_string()) | ||
| .unwrap_or_else(|| "sub-fake-0000".to_string()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Potential panic: byte-index slicing of WebIdentityToken can crash on multi-byte UTF-8.
t[..t.len().min(12)] slices by byte position. If the token contains multi-byte characters and the 12th byte falls mid-codepoint, this panics at runtime. Use char-based truncation instead.
🔧 Proposed fix
let subject = param(ctx, "WebIdentityToken")
- .map(|t| t[..t.len().min(12)].to_string())
+ .map(|t| t.chars().take(12).collect::<String>())
.unwrap_or_else(|| "sub-fake-0000".to_string());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let subject = param(ctx, "WebIdentityToken") | |
| .map(|t| t[..t.len().min(12)].to_string()) | |
| .unwrap_or_else(|| "sub-fake-0000".to_string()); | |
| let subject = param(ctx, "WebIdentityToken") | |
| .map(|t| t.chars().take(12).collect::<String>()) | |
| .unwrap_or_else(|| "sub-fake-0000".to_string()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/sts/src/provider.rs` around lines 164 - 166, The
WebIdentityToken truncation in the subject construction can panic because it
slices by byte index inside the provider logic. Update the `subject` derivation
in `provider.rs` to truncate `WebIdentityToken` using a char-based approach
instead of `t[..t.len().min(12)]`, and keep the existing fallback behavior in
the `param(ctx, "WebIdentityToken")` chain unchanged. Use the `subject` local in
this STS provider flow as the place to fix the UTF-8-safe truncation.
Comment on lines
+170
to
+178
| let inner = format!( | ||
| "{creds}\ | ||
| <SubjectFromWebIdentityToken>{subject}</SubjectFromWebIdentityToken>\ | ||
| <Provider>{}</Provider>\ | ||
| <AssumedRoleUser>\ | ||
| <AssumedRoleId>AROA{role_id_suffix}:{session_name}</AssumedRoleId>\ | ||
| <Arn>arn:aws:sts::{account_id}:assumed-role/{role_name}/{session_name}</Arn>\ | ||
| </AssumedRoleUser>", | ||
| xml_escape(&provider_id) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing XML escaping for user-controlled values in both new handlers.
In AssumeRoleWithWebIdentity, subject (line 172), session_name (line 175), and role_name (line 176) are embedded directly in XML without xml_escape(), while only provider_id is escaped. In AssumeRoleWithSAML, role_name (line 204) is unescaped even though role_arn is escaped in <Audience>. The IAM AssumeRole reference escapes all user-controlled values. Any <, &, or " in these fields produces malformed XML.
🔧 Proposed fix for AssumeRoleWithWebIdentity
let inner = format!(
"{creds}\
-<SubjectFromWebIdentityToken>{subject}</SubjectFromWebIdentityToken>\
+<SubjectFromWebIdentityToken>{}</SubjectFromWebIdentityToken>\
<Provider>{}</Provider>\
<AssumedRoleUser>\
-<AssumedRoleId>AROA{role_id_suffix}:{session_name}</AssumedRoleId>\
-<Arn>arn:aws:sts::{account_id}:assumed-role/{role_name}/{session_name}</Arn>\
+<AssumedRoleId>AROA{role_id_suffix}:{}</AssumedRoleId>\
+<Arn>arn:aws:sts::{account_id}:assumed-role/{}/{}</Arn>\
</AssumedRoleUser>",
- xml_escape(&provider_id)
+ xml_escape(&subject),
+ xml_escape(&provider_id),
+ xml_escape(&session_name),
+ xml_escape(&role_name),
+ xml_escape(&session_name)
);🔧 Proposed fix for AssumeRoleWithSAML
let inner = format!(
"{creds}\
<Issuer>{}</Issuer>\
<AssumedRoleUser>\
-<AssumedRoleId>AROA{role_id_suffix}:{session_name}</AssumedRoleId>\
-<Arn>arn:aws:sts::{account_id}:assumed-role/{role_name}/{session_name}</Arn>\
+<AssumedRoleId>AROA{role_id_suffix}:{}</AssumedRoleId>\
+<Arn>arn:aws:sts::{account_id}:assumed-role/{}/{}</Arn>\
</AssumedRoleUser>\
<NameQualifier>fake-nq</NameQualifier>\
<Subject>fake-subject</Subject>\
<SubjectType>persistent</SubjectType>\
<Audience>{}</Audience>",
xml_escape(&principal_arn),
- xml_escape(&role_arn)
+ xml_escape(&role_arn),
+ xml_escape(&role_name),
+ xml_escape(&session_name)
);Also applies to: 199-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/sts/src/provider.rs` around lines 170 - 178, The XML built in
AssumeRoleWithWebIdentity and AssumeRoleWithSAML is still inserting
user-controlled values without escaping, which can break the response. Update
the response construction in provider.rs so every interpolated field inside the
XML uses xml_escape(), including subject, session_name, and role_name in the web
identity handler, and role_name in the SAML handler; keep provider_id and
role_arn escaped as well. Use the existing helper patterns in the AssumeRole
response assembly to locate and apply the fix consistently.
| let principal_arn = param(ctx, "PrincipalArn").unwrap_or_else(|| { | ||
| format!("arn:aws:iam::{account_id}:saml-provider/MyProvider") | ||
| }); | ||
| let session_name = "saml-session".to_string(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
SAML handler ignores RoleSessionName parameter.
session_name is hardcoded to "saml-session" instead of reading from the request. AWS AssumeRoleWithSAML accepts RoleSessionName just like AssumeRole and AssumeRoleWithWebIdentity do. Clients passing a custom session name will see it silently ignored.
🔧 Proposed fix
-let session_name = "saml-session".to_string();
+let session_name =
+ param(ctx, "RoleSessionName").unwrap_or_else(|| "saml-session".to_string());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let session_name = "saml-session".to_string(); | |
| let session_name = | |
| param(ctx, "RoleSessionName").unwrap_or_else(|| "saml-session".to_string()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/services/sts/src/provider.rs` at line 195, The SAML flow in
provider.rs hardcodes the session name in the AssumeRoleWithSAML path instead of
honoring the request parameter. Update the logic around the session_name
assignment in the SAML handler to read RoleSessionName from the incoming
request, following the same pattern used by the AssumeRole and
AssumeRoleWithWebIdentity handlers, and preserve the existing fallback behavior
only if the parameter is absent.
Comment on lines
+139
to
+143
| "ecs", | ||
| "rds", | ||
| "cognito", | ||
| "elasticache", | ||
| "cloudtrail", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove duplicate service entries in all_service_names().
The five services added at lines 139–143 (ecs, rds, cognito, elasticache, cloudtrail) already exist earlier in the list at lines 122–135. This creates duplicate entries in the Vec<String> returned by all_service_names(), which is used directly as the service list for the all-services-smoke profile (line 81).
The readme_supported_services_match_all_services_smoke_inventory test (line 1638) masks this because it collects into a BTreeSet, which silently deduplicates. However, the raw Vec with duplicates is used to configure and start parity test services, causing redundant work.
🐛 Proposed fix: remove duplicate entries
"redshift",
"cloudformation",
"lambda",
- "ecs",
- "rds",
- "cognito",
- "elasticache",
- "cloudtrail",
]
.into_iter()
.map(|s| s.to_string())
.collect()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "ecs", | |
| "rds", | |
| "cognito", | |
| "elasticache", | |
| "cloudtrail", | |
| "redshift", | |
| "cloudformation", | |
| "lambda", | |
| ] | |
| .into_iter() | |
| .map(|s| s.to_string()) | |
| .collect() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/tests/integration/src/parity.rs` around lines 139 - 143, The
`all_service_names()` list in `crates/tests/integration/src/parity.rs` contains
duplicate service entries, specifically the later `ecs`, `rds`, `cognito`,
`elasticache`, and `cloudtrail` entries that already appear earlier in the same
`Vec<String>`. Remove the repeated items so `all_service_names()` returns a
unique service list, since this vector is used directly by the
`all-services-smoke` profile and duplicate entries cause redundant service
startup work. Keep the earlier canonical entries and verify the parity inventory
still matches the list expected by
`readme_supported_services_match_all_services_smoke_inventory`.
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 29 minutes. |
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
1 participant
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.
Summary
Continues the service coverage expansion started in #62. This batch adds 5 new services and significantly deepens 8 existing ones, adding ~120 operations total and raising the supported service count from 24 → 29.
New services
AdminInitiateAuth(17 ops)StartLogging/StopLogging,LookupEvents, tags (11 ops)Deepened existing services
DeleteSubnet/DeleteSecurityGroup,StopInstances/StartInstances,CreateTags/DescribeTags,DescribeAvailabilityZones,DescribeRegionsSendCommand,ListCommands,GetCommandInvocation,GetParameterHistory, tag managementGetSendQuota/GetSendStatistics,SendTemplatedEmail, notification attributesEnableLogging/DisableLoggingListHostedZonesByName,GetHostedZoneCount,ChangeTagsForResource/ListTagsForResourceDescribeDomains(batch),AddTags/RemoveTags/ListTags,GetCompatibleVersions,ListVersions, software update opsAssumeRoleWithWebIdentity,AssumeRoleWithSAMLBug fix
TerminateInstances/StopInstances/StartInstances:collect_indexed_paramsnow checks bothquery_paramsandrequest_body, fixing a regression in thesmoke_ec2_instance_lifecycle_with_latency_guardrailintegration test where form-body parameters weren't found.Test plan
cargo test --workspace— all tests pass (zero failures)cargo clippy --all-targets --all-features -- -D warnings— zero warnings in changed cratescargo build --workspace— clean buildsmoke_ec2_instance_lifecycle_with_latency_guardrailpassesSummary by CodeRabbit