ci: enforce cargo check warning gate#69
Merged
Merged
Conversation
Add a dedicated cargo check job with denied warnings so dead code and other rustc warnings fail required CI. Include the new result in required-check aggregators and fix the current clippy violations needed for the gate to pass.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds a workspace cargo-check CI job and surfaces its result in PR markdown; refactors DynamoDB key-condition parsing; computes service-scoped, overflow-safe error-rate in the studio UI with a unit test; adds S3 GET streaming buffer selection; and extends protocol, gateway, native-HTTP translators, and tests to support CloudTrail, Cognito, ECS, ElastiCache, and RDS. ChangesMaintenance and Safety Improvements
AWS Service Additions
Sequence Diagram(s)sequenceDiagram
participant NativeCLI
participant translate_command
participant translate_service
participant json_target_plan
participant query_plan
participant Gateway
NativeCLI->>translate_command: invoke translate for service:operation
translate_command->>translate_service: delegate to service-specific translator (e.g., cloudtrail, cognito-idp)
translate_service->>json_target_plan: build JSON target plan (x-amz-target)
translate_service->>query_plan: build Query plan for Query-protocol ops
query_plan->>Gateway: Gateway inspects x-amz-target/prefix to infer service
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ci.yml:
- Around line 143-145: In the check job update the three actions
(actions/checkout@v4, dtolnay/rust-toolchain@stable, Swatinem/rust-cache@v2) to
be pinned to their full commit SHAs, set the actions/checkout step to include
with: persist-credentials: false, and add a job-level permissions: override for
the check job to the least-privilege required (do not leave pull-requests:
write) so the job does not inherit write permissions from the workflow-level
setting.
In `@crates/studio-ui/src/explorer.rs`:
- Around line 365-375: The error_rate_pct is computed from the global tx_summary
but should use service-scoped counts; update the computation in
ServiceExplorerViewModel::build to call log.for_service(service).summary() (or
otherwise aggregate client_error, server_error, and total from the
service-scoped iterator) and compute errors = client_error + server_error, then
do the checked_mul/checked_div with the service total and fallback to 0/min 100
as before so division-by-zero is handled; reference tx_summary ->
service_tx_summary, log.for_service(service), service_tx_count, and
error_rate_pct when making the change.
🪄 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: 4840b441-efc3-46de-9796-d8f8fcbaab7f
📒 Files selected for processing (3)
.github/workflows/ci.ymlcrates/services/dynamodb/src/provider.rscrates/studio-ui/src/explorer.rs
Scope Studio explorer error rates to the selected service and add a regression test for mixed-service traffic. Also harden the new cargo check workflow job with pinned actions, disabled checkout credential persistence, and least-privilege permissions.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Use the same 125 MB default benchmark memory ceiling in PR CI as the benchmark workflow so the gate stays consistent across both paths.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gateway/src/server.rs (1)
2357-2361:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow hyphens in host-based service validation.
At Line 2359, the character gate rejects
cognito-idp, so host-derived Cognito requests can’t resolve via this branch even after normalization.Suggested fix
- .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit()) + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-')🤖 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/gateway/src/server.rs` around lines 2357 - 2361, The host-derived service name validation in the potential_service branch rejects hyphens (e.g., "cognito-idp"), preventing known services from matching; update the character check inside the if in server.rs so the predicate allows '-' as well (i.e., change the closure used with potential_service.chars().all(...) to accept c.is_ascii_lowercase() || c.is_ascii_digit() || c == '-'), keeping the is_known_service(&potential_service) call unchanged so normalized host names like "cognito-idp" resolve correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gateway/src/server.rs`:
- Around line 2599-2606: The X-Amz-Target parsing is too permissive: update the
CloudTrail prefix check and make ECS version stripping generic. For CloudTrail,
instead of is_some_and(|prefix|
prefix.eq_ignore_ascii_case("com.amazonaws.cloudtrail")), check that the target
starts with "com.amazonaws.cloudtrail" case-insensitively and that the next
character (if any) is a delimiter ('.', '_', or end-of-string) to avoid
over-matching. For ECS, replace the hardcoded two-version strip logic with a
generic version-trimming step that removes a trailing 'V' followed by a numeric
date/version token (e.g., regex or manual scan for 'V' + digits) before matching
"AmazonEC2ContainerService", ensuring variants like
"AmazonEC2ContainerServiceV20141113.*" match the ECS branch; apply these changes
to all occurrences that use target.split/next_back and the cloudtrail prefix
check (the branches handling ECS and CloudTrail in the X-Amz-Target parsing).
---
Outside diff comments:
In `@crates/gateway/src/server.rs`:
- Around line 2357-2361: The host-derived service name validation in the
potential_service branch rejects hyphens (e.g., "cognito-idp"), preventing known
services from matching; update the character check inside the if in server.rs so
the predicate allows '-' as well (i.e., change the closure used with
potential_service.chars().all(...) to accept c.is_ascii_lowercase() ||
c.is_ascii_digit() || c == '-'), keeping the
is_known_service(&potential_service) call unchanged so normalized host names
like "cognito-idp" resolve correctly.
🪄 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: 6ea62675-ae3f-48d4-a6b7-219d12c9b669
⛔ Files ignored due to path filters (3)
README.mdis excluded by none and included by nonetests/parity/known_differences.jsonis excluded by none and included by nonetests/parity/scenarios/all-services-smoke.jsonis excluded by none and included by none
📒 Files selected for processing (6)
.github/workflows/ci.ymlcrates/aws-protocol/src/protocol.rscrates/aws-protocol/tests/protocol_integration.rscrates/gateway/src/server.rscrates/tests/integration/src/native_http.rscrates/tests/integration/src/parity.rs
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ci.yml:
- Around line 409-418: The docker pull inside the retry loop can hang
indefinitely; update the pull command in the loop that uses "for attempt in 1 2
3; do" and the docker pull invocation of "$PARITY_LOCALSTACK_IMAGE" to run with
a per-attempt timeout (e.g., using the timeout utility) so each attempt fails
quickly on hang and the retry logic can proceed; apply the same change to the
other identical loops referenced (the ones around the other docker pull
invocations at the other locations) and ensure the loop still breaks on success
and exits with the existing error handling when the final attempt fails.
🪄 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: caa729dd-cb70-4e2d-8c47-35f01df9b731
📒 Files selected for processing (2)
.github/workflows/ci.ymlcrates/services/s3/src/provider.rs
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
|
Contributor
CI ResultsChecks
Parity
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
cargo checkCI job withRUSTFLAGS=-D warningsso dead code and other rustc warnings fail required checksstudio-uiand DynamoDB parsing so the stricter gate passes cleanlyValidation
cargo fmt --allRUSTFLAGS=\"-D warnings\" cargo check --workspace --all-targets --all-featurescargo clippy --all-targets --all-features -- -D warningsSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores