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. |
📝 WalkthroughWalkthroughAdds five new service crates, canonicalizes service names, wires optional provider registration, extends many providers and their stores, introduces numerous integration and performance tests, and updates CI/benchmark workflow guards. ChangesAWS services expansion and wiring
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
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
+288
to
+578
| // ---------------------------------------------------------------- | ||
| // GetSendQuota | ||
| // ---------------------------------------------------------------- | ||
| "GetSendQuota" => { | ||
| let sent_count = self | ||
| .store | ||
| .get(account_id, region) | ||
| .map(|s| s.emails.len() as f64) | ||
| .unwrap_or(0.0); | ||
| let inner = format!( | ||
| "<Max24HourSend>50000.0</Max24HourSend>\ | ||
| <MaxSendRate>14.0</MaxSendRate>\ | ||
| <SentLast24Hours>{sent_count}</SentLast24Hours>" | ||
| ); | ||
| Ok(xml_resp("GetSendQuota", &rid, &inner)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // GetSendStatistics | ||
| // ---------------------------------------------------------------- | ||
| "GetSendStatistics" => { | ||
| let data_points = self | ||
| .store | ||
| .get(account_id, region) | ||
| .map(|store| { | ||
| // Group emails by hour bucket for realistic stats | ||
| let total = store.emails.len() as u64; | ||
| if total == 0 { | ||
| return String::new(); | ||
| } | ||
| format!( | ||
| "<member>\ | ||
| <Timestamp>{}</Timestamp>\ | ||
| <DeliveryAttempts>{total}</DeliveryAttempts>\ | ||
| <Bounces>0</Bounces>\ | ||
| <Complaints>0</Complaints>\ | ||
| <Rejects>0</Rejects>\ | ||
| </member>", | ||
| Utc::now().format("%Y-%m-%dT%H:00:00Z") | ||
| ) | ||
| }) | ||
| .unwrap_or_default(); | ||
| let inner = format!("<SendDataPoints>{data_points}</SendDataPoints>"); | ||
| Ok(xml_resp("GetSendStatistics", &rid, &inner)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // CreateTemplate | ||
| // ---------------------------------------------------------------- | ||
| "CreateTemplate" => { | ||
| let template_name = match str_param(ctx, "Template.TemplateName") { | ||
| Some(n) => n.to_string(), | ||
| None => { | ||
| return Ok(xml_error( | ||
| "MissingParameter", | ||
| "Template.TemplateName required", | ||
| 400, | ||
| )); | ||
| } | ||
| }; | ||
| let subject_part = str_param(ctx, "Template.SubjectPart") | ||
| .unwrap_or("") | ||
| .to_string(); | ||
| let html_part = str_param(ctx, "Template.HtmlPart") | ||
| .unwrap_or("") | ||
| .to_string(); | ||
| let text_part = str_param(ctx, "Template.TextPart") | ||
| .unwrap_or("") | ||
| .to_string(); | ||
|
|
||
| let mut store = self.store.get_or_create(account_id, region); | ||
| if store.templates.contains_key(&template_name) { | ||
| return Ok(xml_error( | ||
| "AlreadyExists", | ||
| &format!("Template {template_name} already exists"), | ||
| 400, | ||
| )); | ||
| } | ||
| store.templates.insert( | ||
| template_name.clone(), | ||
| EmailTemplate { | ||
| template_name, | ||
| subject_part, | ||
| html_part, | ||
| text_part, | ||
| created_at: Utc::now(), | ||
| }, | ||
| ); | ||
| Ok(xml_no_result("CreateTemplate", &rid)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // DeleteTemplate | ||
| // ---------------------------------------------------------------- | ||
| "DeleteTemplate" => { | ||
| let template_name = match str_param(ctx, "TemplateName") { | ||
| Some(n) => n.to_string(), | ||
| None => { | ||
| return Ok(xml_error("MissingParameter", "TemplateName required", 400)); | ||
| } | ||
| }; | ||
| let mut store = self.store.get_or_create(account_id, region); | ||
| store.templates.remove(&template_name); | ||
| Ok(xml_no_result("DeleteTemplate", &rid)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // GetTemplate | ||
| // ---------------------------------------------------------------- | ||
| "GetTemplate" => { | ||
| let template_name = match str_param(ctx, "TemplateName") { | ||
| Some(n) => n.to_string(), | ||
| None => { | ||
| return Ok(xml_error("MissingParameter", "TemplateName required", 400)); | ||
| } | ||
| }; | ||
| let Some(store) = self.store.get(account_id, region) else { | ||
| return Ok(xml_error( | ||
| "TemplateDoesNotExist", | ||
| &format!("Template {template_name} does not exist"), | ||
| 400, | ||
| )); | ||
| }; | ||
| match store.templates.get(&template_name) { | ||
| Some(t) => { | ||
| let inner = format!( | ||
| "<Template>\ | ||
| <TemplateName>{}</TemplateName>\ | ||
| <SubjectPart>{}</SubjectPart>\ | ||
| <HtmlPart>{}</HtmlPart>\ | ||
| <TextPart>{}</TextPart>\ | ||
| </Template>", | ||
| t.template_name, t.subject_part, t.html_part, t.text_part | ||
| ); | ||
| Ok(xml_resp("GetTemplate", &rid, &inner)) | ||
| } | ||
| None => Ok(xml_error( | ||
| "TemplateDoesNotExist", | ||
| &format!("Template {template_name} does not exist"), | ||
| 400, | ||
| )), | ||
| } | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // ListTemplates | ||
| // ---------------------------------------------------------------- | ||
| "ListTemplates" => { | ||
| let Some(store) = self.store.get(account_id, region) else { | ||
| return Ok(xml_resp("ListTemplates", &rid, "<TemplatesMetadata />")); | ||
| }; | ||
| let members: String = store | ||
| .templates | ||
| .values() | ||
| .map(|t| { | ||
| format!( | ||
| "<member><Name>{}</Name><CreatedTimestamp>{}</CreatedTimestamp></member>", | ||
| t.template_name, | ||
| t.created_at.format("%Y-%m-%dT%H:%M:%SZ") | ||
| ) | ||
| }) | ||
| .collect(); | ||
| let inner = if members.is_empty() { | ||
| "<TemplatesMetadata />".to_string() | ||
| } else { | ||
| format!("<TemplatesMetadata>{members}</TemplatesMetadata>") | ||
| }; | ||
| Ok(xml_resp("ListTemplates", &rid, &inner)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // SendTemplatedEmail | ||
| // ---------------------------------------------------------------- | ||
| "SendTemplatedEmail" => { | ||
| let source = match str_param(ctx, "Source") { | ||
| Some(s) => s.to_string(), | ||
| None => return Ok(xml_error("MissingParameter", "Source required", 400)), | ||
| }; | ||
| let template_name = match str_param(ctx, "Template") { | ||
| Some(t) => t.to_string(), | ||
| None => return Ok(xml_error("MissingParameter", "Template required", 400)), | ||
| }; | ||
| let to = addresses_from_params(ctx, "Destination.ToAddresses.member"); | ||
| let cc = addresses_from_params(ctx, "Destination.CcAddresses.member"); | ||
| let bcc = addresses_from_params(ctx, "Destination.BccAddresses.member"); | ||
|
|
||
| let (subject, html_part, text_part) = { | ||
| let Some(store) = self.store.get(account_id, region) else { | ||
| return Ok(xml_error( | ||
| "TemplateDoesNotExist", | ||
| &format!("Template {template_name} does not exist"), | ||
| 400, | ||
| )); | ||
| }; | ||
| match store.templates.get(&template_name) { | ||
| Some(t) => ( | ||
| t.subject_part.clone(), | ||
| t.html_part.clone(), | ||
| t.text_part.clone(), | ||
| ), | ||
| None => { | ||
| return Ok(xml_error( | ||
| "TemplateDoesNotExist", | ||
| &format!("Template {template_name} does not exist"), | ||
| 400, | ||
| )); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| let message_id = Uuid::new_v4().to_string(); | ||
| let email = StoredEmail { | ||
| message_id: message_id.clone(), | ||
| source, | ||
| destination_to: to, | ||
| destination_cc: cc, | ||
| destination_bcc: bcc, | ||
| subject, | ||
| body_text: text_part, | ||
| body_html: html_part, | ||
| sent_at: Utc::now(), | ||
| }; | ||
| let mut store = self.store.get_or_create(account_id, region); | ||
| store.emails.insert(message_id.clone(), email); | ||
| let inner = format!("<MessageId>{message_id}</MessageId>"); | ||
| Ok(xml_resp("SendTemplatedEmail", &rid, &inner)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // SetIdentityFeedbackForwardingEnabled | ||
| // ---------------------------------------------------------------- | ||
| "SetIdentityFeedbackForwardingEnabled" => { | ||
| let identity = match str_param(ctx, "Identity") { | ||
| Some(i) => i.to_string(), | ||
| None => return Ok(xml_error("MissingParameter", "Identity required", 400)), | ||
| }; | ||
| let enabled = str_param(ctx, "ForwardingEnabled") | ||
| .map(|v| v.eq_ignore_ascii_case("true")) | ||
| .unwrap_or(true); | ||
| let mut store = self.store.get_or_create(account_id, region); | ||
| let attrs = store.notification_attrs.entry(identity).or_default(); | ||
| attrs.forwarding_enabled = enabled; | ||
| Ok(xml_no_result("SetIdentityFeedbackForwardingEnabled", &rid)) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // GetIdentityNotificationAttributes | ||
| // ---------------------------------------------------------------- | ||
| "GetIdentityNotificationAttributes" => { | ||
| let identities = addresses_from_params(ctx, "Identities.member"); | ||
| let entries = self | ||
| .store | ||
| .get(account_id, region) | ||
| .map(|store| { | ||
| identities | ||
| .iter() | ||
| .map(|id| { | ||
| let attrs = store | ||
| .notification_attrs | ||
| .get(id) | ||
| .cloned() | ||
| .unwrap_or_default(); | ||
| let bounce = attrs | ||
| .bounce_topic | ||
| .as_deref() | ||
| .map(|t| format!("<BounceTopic>{t}</BounceTopic>")) | ||
| .unwrap_or_default(); | ||
| let complaint = attrs | ||
| .complaint_topic | ||
| .as_deref() | ||
| .map(|t| format!("<ComplaintTopic>{t}</ComplaintTopic>")) | ||
| .unwrap_or_default(); | ||
| let delivery = attrs | ||
| .delivery_topic | ||
| .as_deref() | ||
| .map(|t| format!("<DeliveryTopic>{t}</DeliveryTopic>")) | ||
| .unwrap_or_default(); | ||
| format!( | ||
| "<entry><key>{id}</key><value>\ | ||
| {bounce}{complaint}{delivery}\ | ||
| <ForwardingEnabled>{}</ForwardingEnabled>\ | ||
| </value></entry>", | ||
| attrs.forwarding_enabled | ||
| ) | ||
| }) | ||
| .collect::<String>() | ||
| }) | ||
| .unwrap_or_default(); | ||
| let inner = format!("<NotificationAttributes>{entries}</NotificationAttributes>"); | ||
| Ok(xml_resp("GetIdentityNotificationAttributes", &rid, &inner)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify SES test coverage for new operations.
echo "=== Checking SES unit tests ==="
if [[ -f crates/services/ses/tests/ses_tests.rs ]]; then
rg -n 'test_.*template|test_.*quota|test_.*notification' crates/services/ses/tests/ses_tests.rs | head -15
else
echo "SES unit test file may be incomplete"
fi
echo "=== Checking SES parity scenarios ==="
fd -e json . tests/parity/scenarios/ -x cat {} \; | grep -i 'ses.*template\|GetSendQuota\|GetSendStatistics' || echo "No parity scenarios found for new operations"Repository: JesseKoldewijn/openstack
Length of output: 657
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate SES perf tests file ==="
fd -g'*ses*_perf_tests.rs' crates/services/ses/tests -a 2>/dev/null || true
fd -e rs ses_perf_tests.rs crates/services/ses/tests -a 2>/dev/null || true
fd -e rs ses_perf_tests crates/services/ses/tests -a 2>/dev/null || true
# If file exists, search for operation identifiers
if ls crates/services/ses/tests/*perf*tests*.rs >/dev/null 2>&1; then
echo "=== Searching perf tests for operation names ==="
rg -n --no-heading "GetSendQuota|GetSendStatistics|CreateTemplate|DeleteTemplate|GetTemplate|ListTemplates|SendTemplatedEmail|SetIdentityFeedbackForwardingEnabled|GetIdentityNotificationAttributes" crates/services/ses/tests/*perf*tests*.rs || true
fi
echo "=== Search SES unit tests for error-path coverage per operation ==="
if [[ -f crates/services/ses/tests/ses_tests.rs ]]; then
for op in "GetSendQuota" "GetSendStatistics" "CreateTemplate" "DeleteTemplate" "GetTemplate" "ListTemplates" "SendTemplatedEmail" "SetIdentityFeedbackForwardingEnabled" "GetIdentityNotificationAttributes"; do
echo "--- unit tests: $op ---"
rg -n --no-heading "$op|test_.*$(echo "$op" | tr 'A-Z' 'a-z' | sed 's/[^a-z0-9]//g')" crates/services/ses/tests/ses_tests.rs || true
done
fi
echo "=== Search parity scenarios for SES operation names ==="
if [[ -d tests/parity/scenarios ]]; then
rg -n --no-heading "GetSendQuota|GetSendStatistics|CreateTemplate|DeleteTemplate|GetTemplate|ListTemplates|SendTemplatedEmail|SetIdentityFeedbackForwardingEnabled|GetIdentityNotificationAttributes" tests/parity/scenarios || true
fi
echo "=== Enumerate any ses-related parity scenario files ==="
if [[ -d tests/parity/scenarios ]]; then
fd -e json tests/parity/scenarios -t f | rg -i "ses|amazon.*ses" || true
# show filenames for SES
fd -e json tests/parity/scenarios -t f -i | rg -i "ses" | head -50 || true
fiRepository: JesseKoldewijn/openstack
Length of output: 3381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate SES perf tests file ==="
fd -e rs ses_perf_tests.rs crates/services/ses/tests -a 2>/dev/null || true
if ls crates/services/ses/tests/*perf*tests*.rs >/dev/null 2>&1; then
echo "Perf test files:"
ls -1 crates/services/ses/tests/*perf*tests*.rs
fi
echo "=== Searching perf tests for operation names ==="
if ls crates/services/ses/tests/*perf*tests*.rs >/dev/null 2>&1; then
rg -n --no-heading "GetSendQuota|GetSendStatistics|CreateTemplate|DeleteTemplate|GetTemplate|ListTemplates|SendTemplatedEmail|SetIdentityFeedbackForwardingEnabled|GetIdentityNotificationAttributes" crates/services/ses/tests/*perf*tests*.rs || true
fi
echo "=== Search SES unit tests for coverage per operation ==="
if [[ -f crates/services/ses/tests/ses_tests.rs ]]; then
for op in "GetSendQuota" "GetSendStatistics" "CreateTemplate" "DeleteTemplate" "GetTemplate" "ListTemplates" "SendTemplatedEmail" "SetIdentityFeedbackForwardingEnabled" "GetIdentityNotificationAttributes"; do
echo "--- unit tests: $op ---"
rg -n --no-heading "$op" crates/services/ses/tests/ses_tests.rs || true
done
echo "=== unit tests: function name patterns (best-effort) ==="
rg -n --no-heading "test_.*quota|test_.*statistics|test_.*template|test_.*send_templated|test_.*identity_notification" crates/services/ses/tests/ses_tests.rs || true
fi
echo "=== Search parity scenarios for SES operation names ==="
if [[ -d tests/parity/scenarios ]]; then
rg -n --no-heading "GetSendQuota|GetSendStatistics|CreateTemplate|DeleteTemplate|GetTemplate|ListTemplates|SendTemplatedEmail|SetIdentityFeedbackForwardingEnabled|GetIdentityNotificationAttributes" tests/parity/scenarios || true
fi
echo "=== List SES-related parity scenario files (filenames only) ==="
if [[ -d tests/parity/scenarios ]]; then
fd -e json tests/parity/scenarios -t f | rg -i "ses" || true
fiRepository: JesseKoldewijn/openstack
Length of output: 3615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect ses_perf_tests.rs for Instant + exercised operations ==="
if [[ -f crates/services/ses/tests/ses_perf_tests.rs ]]; then
wc -l crates/services/ses/tests/ses_perf_tests.rs
rg -n "Instant::now|std::time::Instant" crates/services/ses/tests/ses_perf_tests.rs || true
rg -n "make_ctx\\(\"|dispatch\\(&make_ctx\\(|GetSendQuota|GetSendStatistics|CreateTemplate|DeleteTemplate|GetTemplate|ListTemplates|SendTemplatedEmail|SetIdentityFeedbackForwardingEnabled|GetIdentityNotificationAttributes" crates/services/ses/tests/ses_perf_tests.rs || true
echo "--- Context around any dispatch/make_ctx calls (first 120 matches) ---"
rg -n "make_ctx\\(\"" crates/services/ses/tests/ses_perf_tests.rs | head -120 || true
else
echo "Missing crates/services/ses/tests/ses_perf_tests.rs"
fi
echo
echo "=== Inspect ses_tests.rs test bodies for happy vs error-path per operation ==="
if [[ -f crates/services/ses/tests/ses_tests.rs ]]; then
wc -l crates/services/ses/tests/ses_tests.rs
# Print the full test functions for each relevant test name (best effort by range around signatures).
# We'll extract by line numbers from rg and then show ~40 lines around each.
for sig in \
"test_get_send_quota_empty" "test_get_send_quota_after_sends" \
"test_get_send_statistics_empty" "test_get_send_statistics_after_sends" \
"test_create_and_get_template" "test_create_template_duplicate_fails" \
"test_get_template_not_found" "test_list_templates" "test_delete_template" \
"test_send_templated_email" "test_send_templated_email_missing_template" \
"test_set_identity_notification_attributes" \
"test_get_identity_notification_attributes"; do
line=$(rg -n "async fn ${sig}" crates/services/ses/tests/ses_tests.rs | head -1 | cut -d: -f1 || true)
if [[ -n "${line}" ]]; then
start=$(( line>10 ? line-10 : 1 ))
end=$(( line+70 ))
echo "---- ${sig} (lines ${start}-${end}) ----"
sed -n "${start},${end}p" crates/services/ses/tests/ses_tests.rs
fi
done
echo
echo "=== Quick scan: any explicit error assertion helpers in ses_tests.rs ==="
rg -n "assert_.*error|error|Fault|MissingParameter|TemplateDoesNotExist|AlreadyExists|does not exist|unwrap_err|is_err" crates/services/ses/tests/ses_tests.rs | head -200 || true
else
echo "Missing crates/services/ses/tests/ses_tests.rs"
fi
echo
echo "=== Search parity scenarios (properly) for SES operations ==="
if [[ -d tests/parity/scenarios ]]; then
echo "Parity scenario files (json):"
fd -e json --full-path . tests/parity/scenarios | head -200 || true
# Search for operation names in JSON scenario files
rg -n --no-heading "GetSendQuota|GetSendStatistics|CreateTemplate|DeleteTemplate|GetTemplate|ListTemplates|SendTemplatedEmail|SetIdentityFeedbackForwardingEnabled|GetIdentityNotificationAttributes" tests/parity/scenarios -S --glob '*.json' || true
else
echo "Missing tests/parity/scenarios directory"
fiRepository: JesseKoldewijn/openstack
Length of output: 34926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Parity scenarios: search operation names in core.json/extended.json ==="
for f in tests/parity/scenarios/core.json tests/parity/scenarios/extended.json tests/parity/scenarios/all-services-smoke.json; do
[[ -f "$f" ]] || continue
echo "--- $f ---"
rg -n --no-heading \
"GetSendQuota|GetSendStatistics|CreateTemplate|DeleteTemplate|GetTemplate|ListTemplates|SendTemplatedEmail|SetIdentityFeedbackForwardingEnabled|GetIdentityNotificationAttributes" \
"$f" -S || echo "No matches"
done
echo
echo "=== List ses_tests.rs functions per operation (quota/statistics/template/etc) ==="
ops_re='GetSendQuota|GetSendStatistics|CreateTemplate|DeleteTemplate|GetTemplate|ListTemplates|SendTemplatedEmail|SetIdentityFeedbackForwardingEnabled|GetIdentityNotificationAttributes'
# Find test fn names around dispatch contexts
rg -n --no-heading "async fn test_.*" crates/services/ses/tests/ses_tests.rs | head -50
for needle in \
"test_.*quota" "test_.*statistics" \
"test_create_template" "test_.*template" \
"test_delete_template" "test_list_templates" \
"test_send_templated_email" \
"test_set_identity_feedback_forwarding_enabled" \
"test_get_identity_notification_attributes"; do
echo "--- matching: $needle ---"
rg -n --no-heading "async fn ${needle}" crates/services/ses/tests/ses_tests.rs || true
done
echo
echo "=== Quick check: any error-path assertions tied to specific ops ==="
for op in \
"GetSendQuota" "GetSendStatistics" "DeleteTemplate" "ListTemplates" \
"SetIdentityFeedbackForwardingEnabled" "GetIdentityNotificationAttributes" ; do
echo "--- assertions near $op ---"
# show nearby lines where status_code/contains MissingParameter/TemplateDoesNotExist/AlreadyExists appear
# (best-effort: exact matching of error-path per op may require manual inspection)
rg -n --no-heading "$op|MissingParameter|TemplateDoesNotExist|AlreadyExists" crates/services/ses/tests/ses_tests.rs \
| head -80 || true
doneRepository: JesseKoldewijn/openstack
Length of output: 6370
SES provider operations are missing required parity/perf coverage and most lack error-path unit tests
- Parity scenarios missing:
tests/parity/scenarios/{core.json,extended.json,all-services-smoke.json}contain no references to:GetSendQuota,GetSendStatistics,CreateTemplate,DeleteTemplate,GetTemplate,ListTemplates,SendTemplatedEmail,SetIdentityFeedbackForwardingEnabled,GetIdentityNotificationAttributes. - Performance regression coverage missing:
crates/services/ses/tests/ses_perf_tests.rsusesstd::time::Instantbut does not dispatch/cover any of the above SES operations. - Unit tests missing required error paths (happy-only):
crates/services/ses/tests/ses_tests.rsincludes happy-path tests forGetSendQuota,GetSendStatistics,DeleteTemplate,ListTemplates,SetIdentityFeedbackForwardingEnabled, andGetIdentityNotificationAttributes, but no corresponding error-path tests for those operations. - Only some error paths exist:
CreateTemplate(duplicate) +GetTemplate(not found) +SendTemplatedEmail(missing template) have error-path tests; the rest do not.
🤖 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/src/provider.rs` around lines 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.).
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
+151
to
+214
| "AssumeRoleWithWebIdentity" => { | ||
| let role_arn = param(ctx, "RoleArn") | ||
| .unwrap_or_else(|| format!("arn:aws:iam::{account_id}:role/default")); | ||
| let role_name = role_arn | ||
| .rsplit('/') | ||
| .next() | ||
| .filter(|s| !s.is_empty()) | ||
| .unwrap_or("default") | ||
| .to_string(); | ||
| let session_name = | ||
| param(ctx, "RoleSessionName").unwrap_or_else(|| "web-session".to_string()); | ||
| let provider_id = | ||
| param(ctx, "ProviderId").unwrap_or_else(|| "www.amazon.com".to_string()); | ||
| let subject = param(ctx, "WebIdentityToken") | ||
| .map(|t| t[..t.len().min(12)].to_string()) | ||
| .unwrap_or_else(|| "sub-fake-0000".to_string()); | ||
| let creds = temp_credentials_xml("wi//"); | ||
| let role_id_suffix = | ||
| &Uuid::new_v4().to_string().replace('-', "")[..16].to_uppercase(); | ||
| 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) | ||
| ); | ||
| Ok(xml_resp("AssumeRoleWithWebIdentity", &rid, &inner)) | ||
| } | ||
|
|
||
| "AssumeRoleWithSAML" => { | ||
| let role_arn = param(ctx, "RoleArn") | ||
| .unwrap_or_else(|| format!("arn:aws:iam::{account_id}:role/default")); | ||
| let role_name = role_arn | ||
| .rsplit('/') | ||
| .next() | ||
| .filter(|s| !s.is_empty()) | ||
| .unwrap_or("default") | ||
| .to_string(); | ||
| 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(); | ||
| let creds = temp_credentials_xml("saml//"); | ||
| let role_id_suffix = | ||
| &Uuid::new_v4().to_string().replace('-', "")[..16].to_uppercase(); | ||
| 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>\ | ||
| </AssumedRoleUser>\ | ||
| <NameQualifier>fake-nq</NameQualifier>\ | ||
| <Subject>fake-subject</Subject>\ | ||
| <SubjectType>persistent</SubjectType>\ | ||
| <Audience>{}</Audience>", | ||
| xml_escape(&principal_arn), | ||
| xml_escape(&role_arn) | ||
| ); | ||
| Ok(xml_resp("AssumeRoleWithSAML", &rid, &inner)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify STS test coverage for new operations.
echo "=== Checking STS unit tests ==="
if [[ -f crates/services/sts/tests/sts_tests.rs ]]; then
rg -n 'AssumeRoleWithWebIdentity|AssumeRoleWithSAML' crates/services/sts/tests/sts_tests.rs | head -10
else
echo "STS unit test file may be incomplete"
fi
echo "=== Checking STS parity scenarios ==="
fd -e json . tests/parity/scenarios/ -x cat {} \; | grep -i 'sts.*web.*identity\|sts.*saml' || echo "No parity scenarios found for new operations"Repository: JesseKoldewijn/openstack
Length of output: 592
Fix missing required parity coverage for AssumeRoleWithWebIdentity and AssumeRoleWithSAML (plus ensure error/perf tests exist)
crates/services/sts/tests/sts_tests.rsalready contains unit tests for both operations, covering the success path (assertsstatus_code == 200and response XML contains the expected*Response).- No parity scenarios matching these operations were found under
tests/parity/scenarios/; addcore.jsonorextended.jsonscenarios that exercise them against both OpenStack and LocalStack. - Also ensure each operation has: (1) at least one error-path unit test (using the service’s existing error helper/formatting), and (2) an
Instant-timed performance regression test incrates/services/sts/tests/sts_perf_tests.rs.
🤖 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 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.
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
+325
to
+769
| "GetParameterHistory" => { | ||
| let name = match str_param(ctx, "Name") { | ||
| Some(n) => n, | ||
| None => return Ok(json_error("ValidationException", "Name is required", 400)), | ||
| }; | ||
| let Some(store) = self.store.get(account_id, region) else { | ||
| return Ok(json_error( | ||
| "ParameterNotFound", | ||
| &format!("Parameter {name} not found"), | ||
| 400, | ||
| )); | ||
| }; | ||
| if !store.parameters.contains_key(&name) { | ||
| return Ok(json_error( | ||
| "ParameterNotFound", | ||
| &format!("Parameter {name} not found"), | ||
| 400, | ||
| )); | ||
| } | ||
| let history: Vec<Value> = store | ||
| .parameter_history | ||
| .get(&name) | ||
| .map(|versions| versions.iter().map(param_to_json).collect()) | ||
| .unwrap_or_default(); | ||
| Ok(json_ok(json!({ "Parameters": history }))) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // CreateDocument | ||
| // ---------------------------------------------------------------- | ||
| "CreateDocument" => { | ||
| let name = match str_param(ctx, "Name") { | ||
| Some(n) => n, | ||
| None => return Ok(json_error("ValidationException", "Name is required", 400)), | ||
| }; | ||
| let content = match str_param(ctx, "Content") { | ||
| Some(c) => c, | ||
| None => { | ||
| return Ok(json_error( | ||
| "ValidationException", | ||
| "Content is required", | ||
| 400, | ||
| )); | ||
| } | ||
| }; | ||
| let document_type = | ||
| str_param(ctx, "DocumentType").unwrap_or_else(|| "Command".to_string()); | ||
| let document_format = | ||
| str_param(ctx, "DocumentFormat").unwrap_or_else(|| "JSON".to_string()); | ||
|
|
||
| let mut store = self.store.get_or_create(account_id, region); | ||
| if store.documents.contains_key(&name) { | ||
| return Ok(json_error( | ||
| "DocumentAlreadyExists", | ||
| &format!("Document {name} already exists"), | ||
| 400, | ||
| )); | ||
| } | ||
| let doc = Document { | ||
| name: name.clone(), | ||
| document_type: document_type.clone(), | ||
| document_format: document_format.clone(), | ||
| schema_version: "2.2".to_string(), | ||
| status: "Active".to_string(), | ||
| content, | ||
| owner: account_id.clone(), | ||
| created: Utc::now(), | ||
| tags: Default::default(), | ||
| }; | ||
| store.documents.insert(name.clone(), doc); | ||
| Ok(json_ok(json!({ | ||
| "DocumentDescription": { | ||
| "Name": name, | ||
| "DocumentType": document_type, | ||
| "DocumentFormat": document_format, | ||
| "Status": "Active", | ||
| "SchemaVersion": "2.2", | ||
| "Owner": account_id, | ||
| } | ||
| }))) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // DeleteDocument | ||
| // ---------------------------------------------------------------- | ||
| "DeleteDocument" => { | ||
| let name = match str_param(ctx, "Name") { | ||
| Some(n) => n, | ||
| None => return Ok(json_error("ValidationException", "Name is required", 400)), | ||
| }; | ||
| let mut store = self.store.get_or_create(account_id, region); | ||
| if store.documents.remove(&name).is_none() { | ||
| return Ok(json_error( | ||
| "InvalidDocument", | ||
| &format!("Document {name} does not exist"), | ||
| 400, | ||
| )); | ||
| } | ||
| Ok(json_ok(json!({}))) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // DescribeDocument | ||
| // ---------------------------------------------------------------- | ||
| "DescribeDocument" => { | ||
| let name = match str_param(ctx, "Name") { | ||
| Some(n) => n, | ||
| None => return Ok(json_error("ValidationException", "Name is required", 400)), | ||
| }; | ||
| let Some(store) = self.store.get(account_id, region) else { | ||
| return Ok(json_error( | ||
| "InvalidDocument", | ||
| &format!("Document {name} does not exist"), | ||
| 400, | ||
| )); | ||
| }; | ||
| match store.documents.get(&name) { | ||
| Some(doc) => Ok(json_ok(json!({ | ||
| "Document": { | ||
| "Name": doc.name, | ||
| "DocumentType": doc.document_type, | ||
| "DocumentFormat": doc.document_format, | ||
| "SchemaVersion": doc.schema_version, | ||
| "Status": doc.status, | ||
| "Owner": doc.owner, | ||
| } | ||
| }))), | ||
| None => Ok(json_error( | ||
| "InvalidDocument", | ||
| &format!("Document {name} does not exist"), | ||
| 400, | ||
| )), | ||
| } | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // GetDocument | ||
| // ---------------------------------------------------------------- | ||
| "GetDocument" => { | ||
| let name = match str_param(ctx, "Name") { | ||
| Some(n) => n, | ||
| None => return Ok(json_error("ValidationException", "Name is required", 400)), | ||
| }; | ||
| let Some(store) = self.store.get(account_id, region) else { | ||
| return Ok(json_error( | ||
| "InvalidDocument", | ||
| &format!("Document {name} does not exist"), | ||
| 400, | ||
| )); | ||
| }; | ||
| match store.documents.get(&name) { | ||
| Some(doc) => Ok(json_ok(json!({ | ||
| "Name": doc.name, | ||
| "DocumentType": doc.document_type, | ||
| "DocumentFormat": doc.document_format, | ||
| "Content": doc.content, | ||
| "Status": doc.status, | ||
| }))), | ||
| None => Ok(json_error( | ||
| "InvalidDocument", | ||
| &format!("Document {name} does not exist"), | ||
| 400, | ||
| )), | ||
| } | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // ListDocuments | ||
| // ---------------------------------------------------------------- | ||
| "ListDocuments" => { | ||
| let Some(store) = self.store.get(account_id, region) else { | ||
| return Ok(json_ok(json!({ "DocumentIdentifiers": [] }))); | ||
| }; | ||
| let docs: Vec<Value> = store | ||
| .documents | ||
| .values() | ||
| .map(|d| { | ||
| json!({ | ||
| "Name": d.name, | ||
| "DocumentType": d.document_type, | ||
| "DocumentFormat": d.document_format, | ||
| "SchemaVersion": d.schema_version, | ||
| "Owner": d.owner, | ||
| }) | ||
| }) | ||
| .collect(); | ||
| Ok(json_ok(json!({ "DocumentIdentifiers": docs }))) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // SendCommand | ||
| // ---------------------------------------------------------------- | ||
| "SendCommand" => { | ||
| let document_name = match str_param(ctx, "DocumentName") { | ||
| Some(n) => n, | ||
| None => { | ||
| return Ok(json_error( | ||
| "ValidationException", | ||
| "DocumentName is required", | ||
| 400, | ||
| )); | ||
| } | ||
| }; | ||
| let instance_ids: Vec<String> = ctx | ||
| .request_body | ||
| .get("InstanceIds") | ||
| .and_then(|v| v.as_array()) | ||
| .map(|arr| { | ||
| arr.iter() | ||
| .filter_map(|v| v.as_str().map(String::from)) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| let command_id = req_id(); | ||
| let now = Utc::now(); | ||
|
|
||
| let invocations: Vec<CommandInvocation> = instance_ids | ||
| .iter() | ||
| .map(|iid| CommandInvocation { | ||
| command_id: command_id.clone(), | ||
| instance_id: iid.clone(), | ||
| document_name: document_name.clone(), | ||
| status: "Success".to_string(), | ||
| status_details: "Success".to_string(), | ||
| output: String::new(), | ||
| response_code: 0, | ||
| }) | ||
| .collect(); | ||
|
|
||
| let command = Command { | ||
| command_id: command_id.clone(), | ||
| document_name: document_name.clone(), | ||
| status: "Success".to_string(), | ||
| requested_date: now, | ||
| instance_ids: instance_ids.clone(), | ||
| invocations, | ||
| }; | ||
|
|
||
| let mut store = self.store.get_or_create(account_id, region); | ||
| store.commands.insert(command_id.clone(), command); | ||
|
|
||
| Ok(json_ok(json!({ | ||
| "Command": { | ||
| "CommandId": command_id, | ||
| "DocumentName": document_name, | ||
| "Status": "Success", | ||
| "RequestedDateTime": now.to_rfc3339(), | ||
| "InstanceIds": instance_ids, | ||
| } | ||
| }))) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // ListCommands | ||
| // ---------------------------------------------------------------- | ||
| "ListCommands" => { | ||
| let command_id_filter = str_param(ctx, "CommandId"); | ||
| let instance_id_filter = str_param(ctx, "InstanceId"); | ||
|
|
||
| let Some(store) = self.store.get(account_id, region) else { | ||
| return Ok(json_ok(json!({ "Commands": [] }))); | ||
| }; | ||
|
|
||
| let commands: Vec<Value> = store | ||
| .commands | ||
| .values() | ||
| .filter(|c| { | ||
| command_id_filter | ||
| .as_deref() | ||
| .map(|id| c.command_id == id) | ||
| .unwrap_or(true) | ||
| }) | ||
| .filter(|c| { | ||
| instance_id_filter | ||
| .as_deref() | ||
| .map(|id| c.instance_ids.contains(&id.to_string())) | ||
| .unwrap_or(true) | ||
| }) | ||
| .map(|c| { | ||
| json!({ | ||
| "CommandId": c.command_id, | ||
| "DocumentName": c.document_name, | ||
| "Status": c.status, | ||
| "RequestedDateTime": c.requested_date.to_rfc3339(), | ||
| "InstanceIds": c.instance_ids, | ||
| }) | ||
| }) | ||
| .collect(); | ||
|
|
||
| Ok(json_ok(json!({ "Commands": commands }))) | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // GetCommandInvocation | ||
| // ---------------------------------------------------------------- | ||
| "GetCommandInvocation" => { | ||
| let command_id = match str_param(ctx, "CommandId") { | ||
| Some(id) => id, | ||
| None => { | ||
| return Ok(json_error( | ||
| "ValidationException", | ||
| "CommandId is required", | ||
| 400, | ||
| )); | ||
| } | ||
| }; | ||
| let instance_id = match str_param(ctx, "InstanceId") { | ||
| Some(id) => id, | ||
| None => { | ||
| return Ok(json_error( | ||
| "ValidationException", | ||
| "InstanceId is required", | ||
| 400, | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| let Some(store) = self.store.get(account_id, region) else { | ||
| return Ok(json_error( | ||
| "InvocationDoesNotExist", | ||
| "Command invocation not found", | ||
| 400, | ||
| )); | ||
| }; | ||
|
|
||
| let invocation = store.commands.get(&command_id).and_then(|cmd| { | ||
| cmd.invocations | ||
| .iter() | ||
| .find(|inv| inv.instance_id == instance_id) | ||
| }); | ||
|
|
||
| match invocation { | ||
| Some(inv) => Ok(json_ok(json!({ | ||
| "CommandId": inv.command_id, | ||
| "InstanceId": inv.instance_id, | ||
| "DocumentName": inv.document_name, | ||
| "Status": inv.status, | ||
| "StatusDetails": inv.status_details, | ||
| "StandardOutputContent": inv.output, | ||
| "ResponseCode": inv.response_code, | ||
| }))), | ||
| None => Ok(json_error( | ||
| "InvocationDoesNotExist", | ||
| "Command invocation not found", | ||
| 400, | ||
| )), | ||
| } | ||
| } | ||
|
|
||
| // ---------------------------------------------------------------- | ||
| // AddTagsToResource / ListTagsForResource / RemoveTagsFromResource | ||
| // ---------------------------------------------------------------- | ||
| "AddTagsToResource" => { | ||
| let resource_type = match str_param(ctx, "ResourceType") { | ||
| Some(t) => t, | ||
| None => { | ||
| return Ok(json_error( | ||
| "ValidationException", | ||
| "ResourceType is required", | ||
| 400, | ||
| )); | ||
| } | ||
| }; | ||
| let resource_id = match str_param(ctx, "ResourceId") { | ||
| Some(id) => id, | ||
| None => { | ||
| return Ok(json_error( | ||
| "ValidationException", | ||
| "ResourceId is required", | ||
| 400, | ||
| )); | ||
| } | ||
| }; | ||
| let tag_list = ctx | ||
| .request_body | ||
| .get("Tags") | ||
| .and_then(|v| v.as_array()) | ||
| .cloned() | ||
| .unwrap_or_default(); | ||
|
|
||
| let mut store = self.store.get_or_create(account_id, region); | ||
| if resource_type == "Document" | ||
| && let Some(doc) = store.documents.get_mut(&resource_id) | ||
| { | ||
| 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()), | ||
| ) { | ||
| doc.tags.insert(k.to_string(), v.to_string()); | ||
| } | ||
| } | ||
| } | ||
| Ok(json_ok(json!({}))) | ||
| } | ||
|
|
||
| "ListTagsForResource" => { | ||
| let resource_type = str_param(ctx, "ResourceType").unwrap_or_default(); | ||
| let resource_id = str_param(ctx, "ResourceId").unwrap_or_default(); | ||
|
|
||
| let tags: Vec<Value> = self | ||
| .store | ||
| .get(account_id, region) | ||
| .and_then(|store| { | ||
| if resource_type == "Document" { | ||
| store.documents.get(&resource_id).map(|doc| { | ||
| doc.tags | ||
| .iter() | ||
| .map(|(k, v)| json!({"Key": k, "Value": v})) | ||
| .collect() | ||
| }) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| Ok(json_ok(json!({ "TagList": tags }))) | ||
| } | ||
|
|
||
| "RemoveTagsFromResource" => { | ||
| let resource_type = str_param(ctx, "ResourceType").unwrap_or_default(); | ||
| let resource_id = str_param(ctx, "ResourceId").unwrap_or_default(); | ||
| let tag_keys: Vec<String> = ctx | ||
| .request_body | ||
| .get("TagKeys") | ||
| .and_then(|v| v.as_array()) | ||
| .map(|arr| { | ||
| arr.iter() | ||
| .filter_map(|v| v.as_str().map(String::from)) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default(); | ||
|
|
||
| let mut store = self.store.get_or_create(account_id, region); | ||
| if resource_type == "Document" | ||
| && let Some(doc) = store.documents.get_mut(&resource_id) | ||
| { | ||
| for k in &tag_keys { | ||
| doc.tags.remove(k); | ||
| } | ||
| } | ||
| Ok(json_ok(json!({}))) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== New operations (should be 12) ==="
echo "GetParameterHistory CreateDocument DeleteDocument DescribeDocument GetDocument"
echo "ListDocuments SendCommand ListCommands GetCommandInvocation"
echo "AddTagsToResource ListTagsForResource RemoveTagsFromResource"
echo ""
echo "=== Unit tests in ssm_tests.rs ==="
rg -n 'async fn test_' crates/services/ssm/tests/ssm_tests.rs | rg -i 'document|command|invocation|history|tags'
echo ""
echo "=== Perf tests in ssm_perf_tests.rs ==="
rg -n 'async fn perf_' crates/services/ssm/tests/ssm_perf_tests.rs
echo ""
echo "=== Parity scenarios for SSM ==="
fd -e json . tests/parity/scenarios/ --exec rg -l 'ssm|SSM' {}Repository: JesseKoldewijn/openstack
Length of output: 1197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== List SSM test files ==="
ls -la crates/services/ssm/tests || true
echo
echo "=== Inspect ssm_tests.rs for coverage of required operations ==="
sed -n '1,220p' crates/services/ssm/tests/ssm_tests.rs | cat
echo
rg -n "async fn test_" crates/services/ssm/tests/ssm_tests.rs | cat
echo
rg -n "test_(add_tags|list_tags|remove_tags|.*document.*|.*command.*|.*invocation.*|.*history.*)" crates/services/ssm/tests/ssm_tests.rs || true
echo
echo "=== Inspect ssm_perf_tests.rs for coverage ==="
ls -la crates/services/ssm/tests/ssm_perf_tests.rs || true
rg -n "async fn perf_" crates/services/ssm/tests/ssm_perf_tests.rs | cat
echo
sed -n '1,240p' crates/services/ssm/tests/ssm_perf_tests.rs | cat
echo
echo "=== Search parity scenarios for SSM operations ==="
# Show any parity scenario JSON mentioning SSM operations / action names / service keyword
fd -e json . tests/parity/scenarios/ --exec rg -n -i "(ssm|GetParameterHistory|CreateDocument|DeleteDocument|DescribeDocument|GetDocument|ListDocuments|SendCommand|ListCommands|GetCommandInvocation|AddTagsToResource|ListTagsForResource|RemoveTagsFromResource)" {} \; || true
echo
echo "=== Count/locate parity scenarios that mention SSM ==="
fd -e json . tests/parity/scenarios/ --exec sh -c 'echo "$0"; rg -i -n "ssm" "$0" >/dev/null && echo " matches ssm"' {} || trueRepository: JesseKoldewijn/openstack
Length of output: 13419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check for tag-related unit tests ==="
rg -n -i "add_tags_to_resource|list_tags_for_resource|remove_tags_from_resource|tag(s)?_for_resource|tags" crates/services/ssm/tests/ssm_tests.rs || true
echo
echo "=== Check ssm_perf_tests.rs for doc/command/history/tag perf tests ==="
rg -n -i "document|command|invocation|history|tag(s)?" crates/services/ssm/tests/ssm_perf_tests.rs || true
echo
echo "=== Check parity core/extended for SSM operations ==="
for f in tests/parity/scenarios/core.json tests/parity/scenarios/extended.json; do
echo "--- $f ---"
rg -n -i '"ssm"|"get-parameter-history"|"create-document"|"delete-document"|"describe-document"|"get-document"|"list-documents"|"send-command"|"list-commands"|"get-command-invocation"|"add-tags-to-resource"|"list-tags-for-resource"|"remove-tags-from-resource"|' "$f" || true
done
echo
echo "=== Show any parity scenario lines for SSM in all-services-smoke.json ==="
rg -n -i "ssm|get-parameter" tests/parity/scenarios/all-services-smoke.json || trueRepository: JesseKoldewijn/openstack
Length of output: 24987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/services/ssm/tests/ssm_tests.rs"
echo "=== Extract relevant test bodies (line ranges around each test) ==="
# Helper: print from start line to just before next test_async
print_test() {
local name="$1"
local start
start=$(rg -n "async fn ${name}\b" "$file" | head -n1 | cut -d: -f1)
if [[ -z "${start:-}" ]]; then
echo "MISSING: ${name}"
return 0
fi
# Grab next ~120 lines; should include body for these tests
echo
echo "----- ${name} (around line ${start}) -----"
sed -n "$start,$((start+140))p" "$file" | cat
}
print_test "test_get_parameter_history"
print_test "test_get_parameter_history_not_found"
print_test "test_create_and_describe_document"
print_test "test_create_document_duplicate_fails"
print_test "test_get_document"
print_test "test_list_documents"
print_test "test_delete_document"
print_test "test_send_command"
print_test "test_list_commands"
print_test "test_list_commands_filter_by_instance"
print_test "test_get_command_invocation"
print_test "test_get_command_invocation_not_found"
# Tags tests (if they exist)
rg -n "test_(add_tags|list_tags|remove_tags)" "$file" || trueRepository: JesseKoldewijn/openstack
Length of output: 38683
SSM provider: missing required parity/unit/perf coverage for new operations
crates/services/ssm/tests/ssm_tests.rshas unit tests for GetParameterHistory and most document/command operations (including an error case for CreateDocument duplicate and GetCommandInvocation not-found), but there’s no explicit error-path test forDeleteDocument, and there are no unit tests forAddTagsToResource/ListTagsForResource/RemoveTagsFromResource.crates/services/ssm/tests/ssm_perf_tests.rsonly benchmarks parameter-store operations; there are no perf tests (withstd::time::Instant) for GetParameterHistory, any document/command operations, or any tag operations.tests/parity/scenarios/only contains an SSM probe forget-parameterinall-services-smoke.json;core.jsonandextended.jsoncontain no parity scenarios for the required new SSM operations.
🤖 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 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.
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>
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
New Features
Tests