0.4.0 #65
0.4.0 #65
Conversation
…e for better state management
…ity and debugging
…on; refactor code structure and update configuration handling
…rotobuf compilation process and remove unused dependencies
…st request serialization and configuration settings
WalkthroughThis PR introduces a significant architectural refactor: replacing legacy configuration with type-safe, validated settings; removing OpenTelemetry for tracing; integrating Protocol Buffers for NATS event messaging; restructuring the domain layer with newtype safety; updating containerization with multi-stage Docker builds; and shifting from scattered dependency injection to unified shared state patterns across handlers, routes, and the main runtime. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as HTTP Handler
participant ProxyService as ProxyService
participant NATS as NATS
participant Backend as Backend Service
Client->>Handler: HTTP Request
Handler->>Handler: Extract client IP<br/>from headers
Handler->>Handler: Create HttpRequest<br/>proto message
Handler->>ProxyService: proxy_request(req,<br/>request_id)
ProxyService->>ProxyService: Encode to protobuf<br/>Add UUID header
ProxyService->>NATS: request("http",<br/>headers, payload)
NATS->>Backend: Route request
Backend->>NATS: Send response<br/>with status header
NATS->>ProxyService: Return message
ProxyService->>ProxyService: Extract status<br/>from headers
ProxyService->>Handler: Return (StatusCode,<br/>payload)
Handler->>Client: HTTP Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/signals.rs (1)
18-24: Consider logging before notifying waiters.The "shutdown signal received" message is logged after
notify.notify_waiters(). If waiters perform long-running or panicking cleanup, the log may be delayed or lost. Consider logging before notification for immediate visibility.🔎 Proposed change
tokio::spawn(async move { sig.recv().await; + info!("shutdown signal received"); + notify.notify_waiters(); - - info!("shutdown signal received"); });tests/integration_test.rs (1)
268-274: Consider removing unusednats_availablehelper.This function is marked
#[allow(dead_code)]and always returnsfalse. If it's not planned for future use, consider removing it to reduce code noise.src/handlers.rs (2)
49-80: Consider refactoring response building to reduce repetition.The function uses
unwrap_or_elsemultiple times (lines 64, 71, 77) as a fallback whenResponse::builder().body()fails. While acceptable for ensuring a response is always returned, consider extracting this pattern into a helper function to reduce duplication.🔎 Optional refactor to reduce duplication
+fn build_response_with_fallback( + status: StatusCode, + body: String, + content_type: &'static str, + fallback: &'static str, +) -> Response<String> { + Response::builder() + .status(status) + .header("Content-Type", content_type) + .body(body) + .unwrap_or_else(|_| Response::new(fallback.to_string())) +} + pub async fn metrics_handler(State(shared_state): State<SharedState>) -> impl IntoResponse { let metrics = &shared_state.metrics; match metrics { Some(metrics) => match metrics.render().await { - Ok(body) => match Response::builder() - .status(StatusCode::OK) - .header("Content-Type", "text/plain; version=0.0.4; charset=utf-8") - .body(body) - { - Ok(response) => response.into_response(), - Err(e) => { - error!(error = %e, "failed to build metrics response"); - Response::builder() - .status(StatusCode::INTERNAL_SERVER_ERROR) - .body("Internal server error".to_string()) - .unwrap_or_else(|_| Response::new("Fatal error".to_string())) - .into_response() - } - }, - Err(_) => Response::builder() - .status(StatusCode::INTERNAL_SERVER_ERROR) - .body("Error rendering metrics".to_string()) - .unwrap_or_else(|_| Response::new("Error rendering metrics".to_string())) - .into_response(), + Ok(body) => build_response_with_fallback( + StatusCode::OK, + body, + "text/plain; version=0.0.4; charset=utf-8", + "Fatal error" + ).into_response(), + Err(_) => build_response_with_fallback( + StatusCode::INTERNAL_SERVER_ERROR, + "Error rendering metrics".to_string(), + "text/plain", + "Error rendering metrics" + ).into_response(), }, - None => Response::builder() - .status(StatusCode::SERVICE_UNAVAILABLE) - .body("Metrics not available".to_string()) - .unwrap_or_else(|_| Response::new("Metrics not available".to_string())) - .into_response(), + None => build_response_with_fallback( + StatusCode::SERVICE_UNAVAILABLE, + "Metrics not available".to_string(), + "text/plain", + "Metrics not available" + ).into_response(), } }
134-143: Clone the NATS client to avoid holding the read lock during async operations.The read lock acquired at line 134 remains held through the entire
request_with_headers().awaitcall, blocking concurrent requests from accessing the NATS client during network I/O. Sinceasync_nats::Clientis cloneable (as a "Cloneable handle" in v0.38.0), clone it within the lock scope and release the lock immediately:- let client = shared_state.nats.read().await; + let client = { + let guard = shared_state.nats.read().await; + guard.clone() + }; // Serialize request using protobuf let buf = req.encode_to_vec();
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
.cargo/config.toml.dockerignore.github/workflows/main.yml.gitignoreAGENTS.mdCLAUDE.mdCargo.tomlDockerfileMakefileVERSIONbuild.rsconfig.jsonhttp2.imlproto/events.protosrc/client_ip.rssrc/conf.rssrc/config/mod.rssrc/config/settings.rssrc/domain/mod.rssrc/domain/types.rssrc/events.rssrc/handlers.rssrc/lib.rssrc/main.rssrc/observability.rssrc/responses/errors.rssrc/responses/statuses.rssrc/routes.rssrc/service/mod.rssrc/service/proxy.rssrc/signals.rssrc/transport/mod.rssrc/transport/nats.rssrc/transport/proto.rstests/client_ip_test.rstests/integration_test.rstests/proto_test.rstests/routes_test.rs
💤 Files with no reviewable changes (3)
- src/events.rs
- src/conf.rs
- src/observability.rs
🧰 Additional context used
📓 Path-based instructions (18)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Useanyhow::Resultfor all fallible operations in Rust code
Add context to errors with.context()method when using anyhow
Avoidunwrap()except in tests
Keep implementation details private in modules
**/*.rs: Rust 2021 edition with 4-space indentation
Use idiomatic Rust naming: modules/paths in snake_case, types in PascalCase, functions in snake_case
Files:
src/service/mod.rstests/proto_test.rssrc/responses/errors.rssrc/domain/mod.rssrc/transport/mod.rssrc/config/mod.rssrc/signals.rsbuild.rssrc/transport/nats.rssrc/main.rssrc/transport/proto.rssrc/lib.rstests/routes_test.rssrc/routes.rssrc/service/proxy.rssrc/domain/types.rssrc/responses/statuses.rssrc/client_ip.rssrc/handlers.rstests/client_ip_test.rssrc/config/settings.rstests/integration_test.rs
**/mod.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use
mod.rsfiles for public API exports
Files:
src/service/mod.rssrc/domain/mod.rssrc/transport/mod.rssrc/config/mod.rs
src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests should be in the same file with
#[cfg(test)]module attributeWrite unit tests inline within source modules (e.g., in src/conf.rs); use #[cfg(test)] modules
Files:
src/service/mod.rssrc/responses/errors.rssrc/domain/mod.rssrc/transport/mod.rssrc/config/mod.rssrc/signals.rssrc/transport/nats.rssrc/main.rssrc/transport/proto.rssrc/lib.rssrc/routes.rssrc/service/proxy.rssrc/domain/types.rssrc/responses/statuses.rssrc/client_ip.rssrc/handlers.rssrc/config/settings.rs
tests/**/*_test.rs
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*_test.rs: Integration tests should use*_test.rsnaming and be intests/directory
Tests should skip gracefully when NATS is unavailable
Files:
tests/proto_test.rstests/routes_test.rstests/client_ip_test.rstests/integration_test.rs
**/*_test.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Mock external dependencies in tests
Files:
tests/proto_test.rstests/routes_test.rstests/client_ip_test.rstests/integration_test.rs
tests/*_test.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/*_test.rs: Write integration and API tests intests/directory with*_test.rsnaming and #[tokio::test] attribute
Use tokio async tests and Axum + tower::ServiceExt for routing tests
Files:
tests/proto_test.rstests/routes_test.rstests/client_ip_test.rstests/integration_test.rs
src/responses/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
All API responses must use
content-type: application/vnd.api+jsonand follow JSON API specification
Files:
src/responses/errors.rssrc/responses/statuses.rs
src/domain/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/domain/**/*.rs: Use the newtype pattern for domain concepts (Port, NatsHost, ClientIp, AuthToken, HttpMethod)
Validate domain types at construction time
Files:
src/domain/mod.rssrc/domain/types.rs
src/config/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
src/config/**/*.rs: Configuration loading should useanyhow::Resultfor error handling with validation
Use custom deserializers for type validation in configuration
Files:
src/config/mod.rssrc/config/settings.rs
build.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Protocol Buffers types should be generated from
proto/events.protovia prost-build in build.rs
Files:
build.rs
Dockerfile
📄 CodeRabbit inference engine (AGENTS.md)
Docker image runs as non-root user (www-data); avoid committing secrets and use secure secret stores instead
Files:
Dockerfile
src/main.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce Rust 2021 edition with
#![deny(warnings)]and#![forbid(unsafe_code)]
Files:
src/main.rs
{src/main.rs,src/lib.rs}
📄 CodeRabbit inference engine (AGENTS.md)
{src/main.rs,src/lib.rs}: Forbid unsafe code with#![forbid(unsafe_code)]in crate root
Deny all compiler warnings with#![deny(warnings)]in crate root
Files:
src/main.rssrc/lib.rs
src/transport/proto.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Generated Protocol Buffers Rust code should be placed in
src/transport/proto/
Files:
src/transport/proto.rs
src/routes.rs
📄 CodeRabbit inference engine (AGENTS.md)
src/routes.rs: Keep route templates underroutes.rswith paths following/api/v1/...pattern; use constants for shared prefixes
Limit CORS origins to required hosts only
Files:
src/routes.rs
src/{routes.rs,handlers.rs}
📄 CodeRabbit inference engine (AGENTS.md)
Limit request body size to 250KB via middleware; avoid increasing without justification
Files:
src/routes.rssrc/handlers.rs
src/handlers.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Body size limit should be 250KB (enforced by middleware)
HTTP responses must set
content-type: application/vnd.api+json
Files:
src/handlers.rs
config.json
📄 CodeRabbit inference engine (AGENTS.md)
Configure application via JSON with keys like
listen_port,nats.host,allowed_origins,is_debug; use CFG_PATH environment variable to override default config.json location
Files:
config.json
🧠 Learnings (34)
📓 Common learnings
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to src/domain/**/*.rs : Use the newtype pattern for domain concepts (Port, NatsHost, ClientIp, AuthToken, HttpMethod)
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to **/mod.rs : Use `mod.rs` files for public API exports
Applied to files:
src/service/mod.rssrc/responses/errors.rssrc/domain/mod.rssrc/transport/mod.rssrc/config/mod.rs.gitignorebuild.rssrc/lib.rssrc/domain/types.rssrc/responses/statuses.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to **/*.rs : Keep implementation details private in modules
Applied to files:
src/service/mod.rs.gitignoresrc/lib.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to src/transport/proto.rs : Generated Protocol Buffers Rust code should be placed in `src/transport/proto/`
Applied to files:
tests/proto_test.rssrc/transport/mod.rs.gitignorebuild.rssrc/transport/proto.rssrc/lib.rsCargo.toml
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to build.rs : Protocol Buffers types should be generated from `proto/events.proto` via prost-build in build.rs
Applied to files:
tests/proto_test.rssrc/transport/mod.rs.gitignorebuild.rssrc/transport/proto.rsproto/events.protoCargo.toml
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to tests/*_test.rs : Write integration and API tests in `tests/` directory with `*_test.rs` naming and #[tokio::test] attribute
Applied to files:
tests/proto_test.rstests/routes_test.rssrc/domain/types.rstests/client_ip_test.rssrc/config/settings.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to **/*_test.rs : Mock external dependencies in tests
Applied to files:
tests/proto_test.rstests/routes_test.rssrc/domain/types.rstests/client_ip_test.rstests/integration_test.rsCargo.toml
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to src/**/*.rs : Write unit tests inline within source modules (e.g., in src/conf.rs); use #[cfg(test)] modules
Applied to files:
tests/proto_test.rs.gitignoretests/routes_test.rstests/client_ip_test.rssrc/config/settings.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to src/**/*.rs : Unit tests should be in the same file with `#[cfg(test)]` module attribute
Applied to files:
tests/proto_test.rstests/client_ip_test.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to tests/**/*_test.rs : Integration tests should use `*_test.rs` naming and be in `tests/` directory
Applied to files:
tests/proto_test.rstests/routes_test.rstests/client_ip_test.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to tests/*_test.rs : Use tokio async tests and Axum + tower::ServiceExt for routing tests
Applied to files:
tests/proto_test.rssrc/main.rstests/routes_test.rssrc/routes.rssrc/service/proxy.rssrc/domain/types.rstests/client_ip_test.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to **/*.rs : Use idiomatic Rust naming: modules/paths in snake_case, types in PascalCase, functions in snake_case
Applied to files:
AGENTS.mdsrc/responses/errors.rs.gitignore
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to src/config/**/*.rs : Use custom deserializers for type validation in configuration
Applied to files:
src/responses/errors.rssrc/responses/statuses.rssrc/config/settings.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to src/domain/**/*.rs : Use the newtype pattern for domain concepts (Port, NatsHost, ClientIp, AuthToken, HttpMethod)
Applied to files:
src/domain/mod.rssrc/transport/mod.rssrc/lib.rstests/routes_test.rssrc/routes.rssrc/service/proxy.rssrc/domain/types.rssrc/responses/statuses.rssrc/client_ip.rssrc/handlers.rstests/client_ip_test.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to src/domain/**/*.rs : Validate domain types at construction time
Applied to files:
src/domain/mod.rssrc/domain/types.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to tests/**/*_test.rs : Tests should skip gracefully when NATS is unavailable
Applied to files:
src/transport/mod.rssrc/transport/nats.rstests/routes_test.rssrc/service/proxy.rssrc/domain/types.rssrc/handlers.rssrc/config/settings.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to **/*.rs : Rust 2021 edition with 4-space indentation
Applied to files:
.gitignore.cargo/config.toml
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to **/*.rs : Avoid `unwrap()` except in tests
Applied to files:
.gitignore
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to Dockerfile : Docker image runs as non-root user (www-data); avoid committing secrets and use secure secret stores instead
Applied to files:
Dockerfile
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to src/main.rs : Enforce Rust 2021 edition with `#![deny(warnings)]` and `#![forbid(unsafe_code)]`
Applied to files:
src/main.rssrc/lib.rsCargo.toml
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to src/routes.rs : Keep route templates under `routes.rs` with paths following `/api/v1/...` pattern; use constants for shared prefixes
Applied to files:
src/lib.rstests/routes_test.rssrc/routes.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to {src/main.rs,src/lib.rs} : Deny all compiler warnings with `#![deny(warnings)]` in crate root
Applied to files:
src/lib.rs
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to src/routes.rs : Limit CORS origins to required hosts only
Applied to files:
tests/routes_test.rssrc/routes.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to src/{routes.rs,handlers.rs} : Limit request body size to 250KB via middleware; avoid increasing without justification
Applied to files:
tests/routes_test.rssrc/handlers.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to src/handlers.rs : Body size limit should be 250KB (enforced by middleware)
Applied to files:
tests/routes_test.rssrc/handlers.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Start application locally with `cargo run` using config.json or override config path with CFG_PATH environment variable
Applied to files:
.cargo/config.tomlsrc/config/settings.rs
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Use `make lint` to run Clippy with autofix
Applied to files:
.github/workflows/main.yml
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Use `make build` for Docker image build and push
Applied to files:
.github/workflows/main.ymlMakefile
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to src/handlers.rs : HTTP responses must set `content-type: application/vnd.api+json`
Applied to files:
src/handlers.rstests/integration_test.rs
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Applies to config.json : Configure application via JSON with keys like `listen_port`, `nats.host`, `allowed_origins`, `is_debug`; use CFG_PATH environment variable to override default config.json location
Applied to files:
config.jsonsrc/config/settings.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Run `make fmt lint test check` before committing code
Applied to files:
Makefile
📚 Learning: 2025-12-22T23:08:57.654Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T23:08:57.654Z
Learning: Use `make fmt` to format code with rustfmt
Applied to files:
Makefile
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to src/config/**/*.rs : Configuration loading should use `anyhow::Result` for error handling with validation
Applied to files:
src/config/settings.rs
📚 Learning: 2025-12-22T23:08:46.270Z
Learnt from: CR
Repo: stockwayup/http2 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-22T23:08:46.270Z
Learning: Applies to src/responses/**/*.rs : All API responses must use `content-type: application/vnd.api+json` and follow JSON API specification
Applied to files:
tests/integration_test.rs
🧬 Code graph analysis (10)
src/transport/nats.rs (2)
src/domain/types.rs (7)
as_str(56-58)as_str(97-99)as_str(204-206)new(19-25)new(46-48)new(86-88)new(193-201)src/service/proxy.rs (1)
new(20-22)
src/transport/proto.rs (1)
src/domain/types.rs (4)
new(19-25)new(46-48)new(86-88)new(193-201)
src/routes.rs (1)
src/handlers.rs (2)
metrics_handler(49-80)proxy(88-217)
src/service/proxy.rs (1)
src/domain/types.rs (4)
new(19-25)new(46-48)new(86-88)new(193-201)
src/domain/types.rs (3)
src/config/settings.rs (1)
new(59-76)src/service/proxy.rs (1)
new(20-22)src/metrics.rs (1)
new(20-36)
src/client_ip.rs (1)
src/domain/types.rs (8)
new(19-25)new(46-48)new(86-88)new(193-201)empty(51-53)empty(91-93)is_empty(62-64)is_empty(103-105)
src/handlers.rs (3)
src/client_ip.rs (1)
extract_client_ip(14-64)src/domain/types.rs (10)
as_str(56-58)as_str(97-99)as_str(204-206)from_axum_method(141-152)empty(51-53)empty(91-93)new(19-25)new(46-48)new(86-88)new(193-201)src/transport/proto.rs (1)
from_components(32-71)
tests/client_ip_test.rs (1)
src/client_ip.rs (1)
extract_client_ip(14-64)
src/config/settings.rs (1)
src/domain/types.rs (4)
new(19-25)new(46-48)new(86-88)new(193-201)
tests/integration_test.rs (4)
src/routes.rs (1)
build_routes(16-241)src/service/proxy.rs (1)
new(20-22)src/domain/types.rs (4)
new(19-25)new(46-48)new(86-88)new(193-201)tests/routes_test.rs (1)
create_test_nats_client(13-22)
🪛 checkmake (0.2.2)
Makefile
[warning] 5-5: Target body for "help" exceeds allowed length of 5 (11).
(maxbodylength)
[warning] 3-3: Missing required phony target "all"
(minphony)
[warning] 3-3: Missing required phony target "clean"
(minphony)
🪛 Checkov (3.2.334)
Dockerfile
[low] 1-48: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 Hadolint (2.14.0)
Dockerfile
[warning] 5-5: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
[warning] 41-41: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
(DL3008)
🪛 markdownlint-cli2 (0.18.1)
AGENTS.md
3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
9-9: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
17-17: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
23-23: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
29-29: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
34-34: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
CLAUDE.md
18-18: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
91-91: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
138-138: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
142-142: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
145-145: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
148-148: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
153-153: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
205-205: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
209-209: Files should end with a single newline character
(MD047, single-trailing-newline)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (62)
src/responses/errors.rs (1)
1-1: LGTM!The import path change from
serde_derive::Serializetoserde::Serializeis the modern, idiomatic approach. Theserdecrate re-exports the derive macros, making this functionally equivalent but cleaner.src/responses/statuses.rs (1)
1-1: LGTM!Consistent with the changes in
errors.rs, the import path migration toserde::{Deserialize, Serialize}is the preferred approach. The structs correctly follow JSON API specification with thedata/id/type/attributesstructure.AGENTS.md (1)
1-39: LGTM! Comprehensive repository guidelines.The documentation is well-structured and covers essential project aspects. The markdown formatting issues flagged by markdownlint (heading spacing) are minor style preferences that don't affect readability.
http2.iml (1)
8-8: LGTM! Standard IDE configuration.Properly marks the tests directory as a test source folder, aligning with the expanded test coverage in this PR.
.gitignore (2)
18-19: LGTM! Config ignore paths updated.The config file ignore patterns have been relocated to the repository root, consistent with the config.json placement.
31-32: LGTM! Protobuf artifacts properly ignored.Correctly ignores generated Protocol Buffers code, as per project learnings that generated code should be placed in
src/transport/proto/but not committed..cargo/config.toml (1)
52-59: LGTM! Environment variables configured.The environment variables are appropriate for the build process. Setting
PROTOC_PARALLELISM=1to prevent deadlocks is a reasonable workaround, though it will impact build performance on all platforms..dockerignore (1)
1-62: LGTM! Production-optimized Docker ignore patterns.The comprehensive ignore patterns are well-suited for multi-stage Docker builds, excluding development artifacts, tests, and sensitive configuration files from the build context. This aligns with the containerization improvements mentioned in the PR objectives.
Cargo.toml (3)
15-17: LGTM! Optional metrics feature properly configured.The feature flag design allows metrics to be compiled out when not needed, which is good for reducing binary size and dependencies in environments where Prometheus metrics aren't required.
19-44: LGTM! Dependencies aligned with architectural refactor.The dependency changes support the PR objectives:
- OpenTelemetry dependencies removed as intended
- Protocol Buffers support added via
prost- Modern dependency versions with appropriate feature flags
- Proper separation of logging (tracing) from OpenTelemetry
46-50: LGTM! Protobuf build configuration.The
prost-builddependency and cargo-machete ignore are correctly configured for generating Protocol Buffers code fromproto/events.proto, as per project learnings.VERSION (1)
1-1: LGTM!Simple version file following semantic versioning. This integrates well with the Makefile and GitHub Actions workflow for dynamic Docker tagging.
src/config/mod.rs (1)
1-3: LGTM!Clean module structure following the
mod.rspattern for public API exports. The re-exports centralize the configuration types at the module root.src/service/mod.rs (1)
1-3: LGTM!Follows the
mod.rspattern for public API exports. Implementation details remain private in theproxymodule while only exposingProxyServicepublicly.src/signals.rs (1)
6-6: LGTM!Clean migration from
logtotracingmacros, aligning with the project-wide observability refactor.src/domain/mod.rs (1)
1-3: LGTM!Properly exports the newtype domain concepts (
Port,NatsHost,ClientIp,AuthToken,HttpMethod) as per the coding guidelines, along with the unifiedSharedState. Clean module structure following themod.rspattern.src/transport/mod.rs (1)
1-6: LGTM!Clean module structure with appropriate separation of concerns between NATS transport and protobuf types. The re-exports provide a unified public API for the transport layer.
.github/workflows/main.yml (2)
22-25: LGTM!Installing
lld(linker) andprotobuf-compileras build dependencies is necessary for the protobuf compilation introduced inbuild.rs.
91-94: LGTM!Dynamic version tagging combined with a
http2-latesttag provides a good balance between versioned releases and cache optimization.build.rs (2)
1-2: Usingstd::io::Resultis appropriate here.While the coding guidelines prefer
anyhow::Result, build scripts are a special case where Cargo expectsfn main() -> std::io::Result<()>. This is the correct choice.
10-16: LGTM!Good defensive check ensuring the proto file exists before attempting compilation, with clear error messaging. This aligns with the coding guideline requirement that Protocol Buffers types should be generated from
proto/events.proto.tests/proto_test.rs (2)
1-53: LGTM!The protobuf serialization test is well-structured. It validates round-trip encoding/decoding of
HttpRequestwith nestedUriandArgsstructures, including proper field verification. Usingunwrap()is acceptable in tests per coding guidelines.
55-90: LGTM!Good compatibility test that validates the protobuf structure matches Go backend expectations. The
user_valuesmap insertion and retrieval test ensures the data structure works correctly for cross-language serialization.tests/client_ip_test.rs (3)
1-82: LGTM!Comprehensive integration tests covering header priority (CF-Connecting-IP > X-Forwarded-For > X-Real-IP), private IP filtering, IPv6 support, and empty header scenarios. Test naming and structure follow coding guidelines.
84-145: LGTM!Excellent real-world scenario coverage including Cloudflare + Kubernetes ingress, nginx-only setups, proxy chains with private IPs at start, and mobile carrier scenarios. The inline comments clearly explain each test case.
147-190: LGTM!Good security-focused tests validating protection against loopback IP injection via CF-Connecting-IP, invalid IP format handling, and all-private-IP attack scenarios.
src/lib.rs (1)
1-14: LGTM!The crate root correctly includes
#![deny(warnings)]and#![forbid(unsafe_code)]as per coding guidelines. The module exports reflect the new domain-driven architecture with appropriate feature-gating for the metrics module.tests/routes_test.rs (3)
11-22: Consider returning early with a skip message instead of using println!The test helper correctly skips when NATS is unavailable, which follows the coding guidelines. However, for CI environments, you might want to consider using
eprintln!or returning a specific skip status.
24-49: LGTM!The test correctly uses the new
SharedStatepattern for dependency injection and follows the coding guidelines for async tests with Axum and tower::ServiceExt. Graceful skip when NATS is unavailable is properly implemented.
51-206: LGTM!The remaining route tests consistently use
SharedStatefor dependency injection and properly skip when NATS is unavailable. Good coverage of CORS, body size limits, and route method validation.src/service/proxy.rs (1)
24-66: LGTM!The
proxy_requestmethod correctly usesanyhow::Resultwith contextual errors. The status code parsing has a sensible fallback to500 Internal Server Errorwhen parsing fails. Error handling follows coding guidelines with.with_context().src/client_ip.rs (3)
66-105: LGTM!The
validate_and_clean_ipfunction provides defense-in-depth with both string-based pre-filtering and authoritativeIpAddrvalidation. The handling of both IPv4 and IPv6 special addresses (loopback, private, link-local, unspecified) is thorough.Note: Once the
is_internal_ip172.* bug is fixed, this function will work correctly.
14-64: LGTM!The
extract_client_ipfunction correctly implements header priority (CF-Connecting-IP > X-Forwarded-For > X-Real-IP > X-Client-IP) and properly handles the comma-separated X-Forwarded-For format by selecting the first public IP. Good use of tracing for diagnostics.
144-253: LGTM!Comprehensive inline unit tests covering all major scenarios. Tests follow the
#[cfg(test)]module pattern as per coding guidelines.Note: After fixing the 172.* bug in
is_internal_ip, the testtest_is_internal_ipwill still pass correctly because 172.16.0.1 is in the private range.src/transport/nats.rs (2)
17-50: LGTM!The
connectmethod has good resilience configuration with ping intervals, request timeouts, and retry on initial connect. The event callback properly triggers application shutdown when max reconnects is reached, preventing the service from running in a degraded state. Error handling follows coding guidelines with contextual error messages.
52-74: LGTM!The
request_with_headersandclosemethods correctly useRwLocksemantics (read lock for concurrent requests, write lock for closing). Error handling with contextual messages follows coding guidelines.Dockerfile (1)
4-4: rust:1.89-bookworm is a valid Docker image. No action needed. Rust 1.89.0 was released August 7, 2025, and the official Docker image tag rust:1.89-bookworm is available on Docker Hub.Likely an incorrect or invalid review comment.
src/transport/proto.rs (3)
1-4: LGTM! Proto module structure is well-organized.The module correctly includes generated protobuf code and provides clean re-exports. Based on learnings, ensure the generated
event.rsfile is placed insrc/transport/proto/directory as indicated by the include path.
14-28: Good use of domain newtypes for type safety.The
HttpRequestInfoandRequestContextstructs correctly leverage the newtype pattern forHttpMethod,ClientIp, andAuthTokenas per coding guidelines. This provides compile-time safety for domain concepts.
30-71: Implementation is clean and correctly maps components to protobuf structure.The
from_componentsfunction properly handles optional URI components withunwrap_or_default(), which is appropriate for fields likescheme,query_string, andhostthat may not be present in all URIs.src/main.rs (2)
1-2: Crate-level safety attributes are correctly configured.The
#![deny(warnings)]and#![forbid(unsafe_code)]attributes align with coding guidelines for enforcing Rust 2021 edition safety standards.
94-106: Shutdown sequence is well-structured.The graceful shutdown correctly waits for the server to stop handling requests before closing the NATS connection. Error handling with
.context()follows the coding guidelines foranyhow::Result.proto/events.proto (2)
7-17: Well-structured HTTPRequest message with good documentation.The comments explaining Rust type compatibility (e.g., mixed binary/string types) are helpful for cross-language development. The message structure aligns with the Rust
HttpRequesttype insrc/transport/proto.rs.
84-99: Enums follow proto3 best practices.Both enums correctly use
UNSPECIFIED = 0as the default value and prefixed naming to prevent collisions. This is proper proto3 convention.src/routes.rs (2)
13-14: Body size limit and API prefix constants are correctly configured.The
BODY_SIZEof 250KB aligns with coding guidelines, andAPI_V1constant follows the/api/v1/...pattern specified in learnings.
50-161: Route definitions correctly use Axum 0.8 path parameter syntax.The double-brace syntax
{{uid}}informat!strings correctly produces{uid}in the final path string, which is the proper syntax for Axum 0.8+ path parameters. The migration from extension-based state injection towith_state(shared_state)is clean.tests/integration_test.rs (3)
8-10: Test imports correctly align with new public API surface.The imports use the new
Config,NatsConfig,Port,NatsHost, andSharedStatetypes, reflecting the architectural changes in the PR.
19-23: SharedState construction follows the new pattern correctly.The tests properly construct
SharedStatewith the NATS client andmetrics: None, then pass it tobuild_routes. This aligns with the PR's architectural shift to unified shared state. The graceful skip pattern for unavailable NATS follows the coding guidelines.Also applies to: 67-75
51-63: Test configuration correctly uses newtype constructors.The test properly validates the
Configstructure withPort::new()andNatsHost::new(), exercising the validation logic. Use ofunwrap()is appropriate in tests per coding guidelines.src/config/settings.rs (3)
1-8: Imports correctly useanyhowfor error handling.The imports align with coding guidelines specifying
anyhow::Resultfor fallible operations and.context()for error enrichment.
58-77:Config::new()implementation follows best practices.The method correctly uses
anyhow::Resultwith.with_context()for descriptive error messages at each failure point. This aligns with coding guidelines for configuration loading with validation.
79-95: Custom deserializers correctly validate at parse time.The deserializers delegate to
Port::new()andNatsHost::new()for validation, ensuring type safety at deserialization as specified in coding guidelines. Error messages from domain constructors are properly propagated.src/domain/types.rs (3)
5-10:SharedStatecorrectly encapsulates shared application state.The struct uses
Arc<RwLock<Client>>for safe concurrent NATS access and feature-gates the metrics field appropriately. This replaces the previous extension-based dependency injection pattern.
12-37:PortandNatsHostnewtypes validate at construction time.Both types correctly implement the newtype pattern with construction-time validation as specified in coding guidelines. The
#[serde(transparent)]attribute ensures clean serialization/deserialization behavior.Also applies to: 186-219
221-292: Unit tests are comprehensive and correctly structured.The tests cover validation logic,
Displayimplementations,FromStrparsing, and serde serialization. Placing tests inline with#[cfg(test)]follows the coding guidelines for source modules.src/handlers.rs (7)
23-38: LGTM!The health check endpoint correctly sets the
content-type: application/vnd.api+jsonheader as required.
101-102: LGTM!Client IP extraction using the new
extract_client_iphelper correctly follows the newtype pattern and integrates with the shared state architecture.
106-112: Authorization extraction and span recording look correct.The manual extraction of the Bearer token from the Authorization header is implemented correctly, and span attributes are properly recorded for observability.
114-127: LGTM!The structured construction of
HttpRequestInfo,RequestContext, andHttpRequestusing the new domain types (HttpMethod,AuthToken,ClientIp) aligns well with the newtype pattern mentioned in the learnings.
198-205: LGTM!Metrics recording correctly uses the optional metrics from
SharedState, and theunwrap_or(500)fallback for invalid status codes is a reasonable default.
219-259: LGTM!The error response creation includes robust fallback handling for serialization failures and correctly sets the
content-type: application/vnd.api+jsonheader as required.
261-283: LGTM!The response creation includes appropriate fallback handling and correctly sets the required content-type header.
| [build] | ||
| # Limit parallel jobs to prevent lock contention on macOS | ||
| # Using 4 jobs for better stability (adjust based on your system) | ||
| jobs = 4 # Limited parallelism for macOS |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify macOS-specific comments with Linux target.
The comments reference macOS-specific lock contention and stability (lines 4-6), but line 20 explicitly targets x86_64-unknown-linux-gnu. If this configuration is intended for cross-platform development (building Linux binaries on macOS), consider clarifying this in the comments to avoid confusion.
🤖 Prompt for AI Agents
.cargo/config.toml around lines 3 to 6: the comment currently calls out
macOS-specific lock contention but the file later targets
x86_64-unknown-linux-gnu (line ~20); update the comment to clarify intent —
either state that limiting jobs to 4 is to avoid lock contention when building
on macOS hosts (including when cross-compiling the Linux target), or make the
comment platform-neutral (e.g., "limit parallel jobs to prevent lock contention
on some hosts"); also add a short reference to the Linux target line so readers
understand this setting is for macOS host stability when producing the Linux
binaries.
| - name: Get version | ||
| id: version | ||
| run: echo "VERSION=$(cat VERSION)" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider trimming whitespace from VERSION.
If the VERSION file contains a trailing newline, it will be included in the tag. Using $(cat VERSION | tr -d '\n') or $(cat VERSION | xargs) would ensure clean version extraction.
🔎 Proposed change
- name: Get version
id: version
- run: echo "VERSION=$(cat VERSION)" >> $GITHUB_OUTPUT
+ run: echo "VERSION=$(cat VERSION | tr -d '\n\r')" >> $GITHUB_OUTPUT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Get version | |
| id: version | |
| run: echo "VERSION=$(cat VERSION)" >> $GITHUB_OUTPUT | |
| - name: Get version | |
| id: version | |
| run: echo "VERSION=$(cat VERSION | tr -d '\n\r')" >> $GITHUB_OUTPUT |
🤖 Prompt for AI Agents
.github/workflows/main.yml around lines 69 to 71: the workflow reads VERSION
with plain cat which preserves trailing newlines and can produce malformed tags;
change the command that sets VERSION to strip trailing/newline whitespace (for
example pipe through tr -d '\n' or through xargs) and then export that trimmed
value to $GITHUB_OUTPUT so the VERSION used downstream contains no extra newline
or spaces.
|
|
||
| eprintln!("Compiling protobuf with prost-build..."); | ||
|
|
||
| // Compile proto files with timeout protection |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove misleading comment about "timeout protection".
The comment mentions "timeout protection" but no timeout mechanism is implemented. This could confuse future maintainers.
🔎 Proposed fix
- // Compile proto files with timeout protection
+ // Compile proto files
match prost_build::Config::new()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Compile proto files with timeout protection | |
| // Compile proto files |
🤖 Prompt for AI Agents
In build.rs around line 29, remove or replace the misleading comment that
mentions "timeout protection" because no timeout mechanism is implemented;
update the comment to accurately describe the action (e.g., "Compile proto
files" or explain the actual behavior) so it does not imply functionality that
doesn't exist.
| println!("cargo:rerun-if-changed={}", proto_file); | ||
| eprintln!("Build script completed successfully"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding rerun-if-changed for build.rs itself.
The build script will rerun when proto/events.proto changes, but it won't rerun if build.rs itself is modified. Adding this directive ensures the script re-executes when its logic changes.
🔎 Proposed addition
println!("cargo:rerun-if-changed={}", proto_file);
+ println!("cargo:rerun-if-changed=build.rs");
eprintln!("Build script completed successfully");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| println!("cargo:rerun-if-changed={}", proto_file); | |
| eprintln!("Build script completed successfully"); | |
| println!("cargo:rerun-if-changed={}", proto_file); | |
| println!("cargo:rerun-if-changed=build.rs"); | |
| eprintln!("Build script completed successfully"); |
🤖 Prompt for AI Agents
In build.rs around lines 43 to 44, the build script tells Cargo to rerun when
proto/events.proto changes but doesn't declare that build.rs itself should
trigger a rerun; add a println!("cargo:rerun-if-changed=build.rs") directive so
Cargo will re-execute the build script when build.rs is modified, placing the
new rerun-if-changed line alongside the existing proto path directive.
| [package] | ||
| name = "http2" | ||
| version = "0.1.1" | ||
| version = "0.1.3" |
There was a problem hiding this comment.
Version mismatch with PR title.
The package version is 0.1.3, but the PR title is "0.4.0". Ensure the version in Cargo.toml matches the intended release version in the PR title, or update the PR title to reflect the actual version being released.
🤖 Prompt for AI Agents
In Cargo.toml around line 3, the version is set to "0.1.3" but the PR title
indicates "0.4.0"; update the version string to the intended release (e.g.,
change version = "0.1.3" to version = "0.4.0") so Cargo.toml matches the PR, or
alternatively change the PR title to "0.1.3" if the release should remain that
lower version—make one of these two consistent and commit the matching change.
| let nats_client = Arc::new(nats_client); | ||
|
|
||
| let routes = build_routes(conf.allowed_origins, conf.enable_cors, nats_client.clone(), metrics.clone()); | ||
| let _proxy_service = Arc::new(ProxyService::new(nats_client.clone())); |
There was a problem hiding this comment.
Unused ProxyService instance.
The _proxy_service is created but never used. If it's planned for future use, consider adding a TODO comment. Otherwise, remove this dead code.
🤖 Prompt for AI Agents
In src/main.rs around line 73, the Arc::new(ProxyService::new(...)) assignment
to _proxy_service is created but never used; either remove this dead line
entirely if the ProxyService is not needed, or add a brief TODO comment above it
(e.g. "// TODO: retain ProxyService for future use — used to keep client alive")
and keep the underscore-prefixed variable so intent is clear and the
unused-variable warning is suppressed.
| // Custom classifier that never treats HTTP status codes as failures | ||
| // Only real connection/protocol errors will be classified as failures | ||
| // Using range 600..=999 means no valid HTTP status codes will be treated as failures | ||
|
|
There was a problem hiding this comment.
Stale comment references missing classifier configuration.
The comment mentions "Using range 600..=999" for a custom classifier, but no such classifier is actually configured on the TraceLayer. The default ServerErrorsAsFailures classifier is in effect. Either add the custom classifier or update the comment.
🔎 Option 1: Remove misleading comment
- // Custom classifier that never treats HTTP status codes as failures
- // Only real connection/protocol errors will be classified as failures
- // Using range 600..=999 means no valid HTTP status codes will be treated as failures
-
router📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Custom classifier that never treats HTTP status codes as failures | |
| // Only real connection/protocol errors will be classified as failures | |
| // Using range 600..=999 means no valid HTTP status codes will be treated as failures |
🤖 Prompt for AI Agents
In src/routes.rs around lines 170-173, the comment claims a custom classifier
using range 600..=999 is applied to the TraceLayer but no such classifier is
configured; either remove or update the misleading comment to reflect that the
default ServerErrorsAsFailures classifier is in effect, or actually attach the
intended custom classifier to the TraceLayer builder (configure the TraceLayer
with the classifier that treats only 600..=999 as failures) so the code matches
the comment.
| #[cfg(test)] | ||
| mod tests { | ||
| use crate::config::{Config, NatsConfig}; | ||
| use crate::domain::types::{NatsHost, Port}; | ||
|
|
||
| #[tokio::test] | ||
| async fn test_proxy_service_creation() { | ||
| let config = Config { | ||
| listen_port: Port::new(8080).unwrap(), | ||
| enable_cors: true, | ||
| nats: NatsConfig { | ||
| host: NatsHost::new("localhost:4222".to_string()).unwrap(), | ||
| max_reconnects: Some(3), | ||
| }, | ||
| allowed_origins: vec![], | ||
| logging: crate::config::LoggingConfig::default(), | ||
| }; | ||
|
|
||
| // This test would require a real NATS connection | ||
| // For unit tests, we just verify the structure | ||
| assert_eq!(config.nats.host.as_str(), "localhost:4222"); | ||
| assert_eq!(config.nats.max_reconnects, Some(3)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test doesn't actually test ProxyService.
The test test_proxy_service_creation only validates Config construction but doesn't test ProxyService at all. Consider either renaming this test to reflect what it actually tests (e.g., test_config_for_proxy) or implementing a proper test with a mocked NATS client.
🔎 Suggested test rename
#[tokio::test]
- async fn test_proxy_service_creation() {
+ async fn test_config_validation_for_nats() {
let config = Config {
listen_port: Port::new(8080).unwrap(),
enable_cors: true,
nats: NatsConfig {
host: NatsHost::new("localhost:4222".to_string()).unwrap(),
max_reconnects: Some(3),
},
allowed_origins: vec![],
logging: crate::config::LoggingConfig::default(),
};
- // This test would require a real NATS connection
- // For unit tests, we just verify the structure
+ // Verify config structure is valid for NATS client initialization
assert_eq!(config.nats.host.as_str(), "localhost:4222");
assert_eq!(config.nats.max_reconnects, Some(3));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(test)] | |
| mod tests { | |
| use crate::config::{Config, NatsConfig}; | |
| use crate::domain::types::{NatsHost, Port}; | |
| #[tokio::test] | |
| async fn test_proxy_service_creation() { | |
| let config = Config { | |
| listen_port: Port::new(8080).unwrap(), | |
| enable_cors: true, | |
| nats: NatsConfig { | |
| host: NatsHost::new("localhost:4222".to_string()).unwrap(), | |
| max_reconnects: Some(3), | |
| }, | |
| allowed_origins: vec![], | |
| logging: crate::config::LoggingConfig::default(), | |
| }; | |
| // This test would require a real NATS connection | |
| // For unit tests, we just verify the structure | |
| assert_eq!(config.nats.host.as_str(), "localhost:4222"); | |
| assert_eq!(config.nats.max_reconnects, Some(3)); | |
| } | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use crate::config::{Config, NatsConfig}; | |
| use crate::domain::types::{NatsHost, Port}; | |
| #[tokio::test] | |
| async fn test_config_validation_for_nats() { | |
| let config = Config { | |
| listen_port: Port::new(8080).unwrap(), | |
| enable_cors: true, | |
| nats: NatsConfig { | |
| host: NatsHost::new("localhost:4222".to_string()).unwrap(), | |
| max_reconnects: Some(3), | |
| }, | |
| allowed_origins: vec![], | |
| logging: crate::config::LoggingConfig::default(), | |
| }; | |
| // Verify config structure is valid for NATS client initialization | |
| assert_eq!(config.nats.host.as_str(), "localhost:4222"); | |
| assert_eq!(config.nats.max_reconnects, Some(3)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/service/proxy.rs around lines 69 to 92, the test named
test_proxy_service_creation only checks Config values and does not exercise
ProxyService; either rename the test to something accurate like
test_config_for_proxy or replace it with a proper unit test that constructs a
ProxyService using a mocked NATS client (or a test double) to validate behavior
such as successful initialization, error paths, and handling of max_reconnects;
if choosing the mock approach, inject the mock NATS dependency into
ProxyService, assert the service calls expected methods on the mock, and avoid
requiring a real NATS connection.
| use anyhow::{Context, Result}; | ||
| use async_nats::{Client, HeaderMap}; | ||
| use bytes::Bytes; | ||
| use futures::SinkExt; |
There was a problem hiding this comment.
Unused import: futures::SinkExt.
The import futures::SinkExt on line 4 doesn't appear to be used anywhere in this file.
🔎 Proposed fix
use anyhow::{Context, Result};
use async_nats::{Client, HeaderMap};
use bytes::Bytes;
-use futures::SinkExt;
use std::sync::Arc;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use futures::SinkExt; | |
| use anyhow::{Context, Result}; | |
| use async_nats::{Client, HeaderMap}; | |
| use bytes::Bytes; | |
| use std::sync::Arc; |
🤖 Prompt for AI Agents
In src/transport/nats.rs around line 4 the import `use futures::SinkExt;` is
unused; remove that import line (or, if you intended to use SinkExt methods,
instead add the appropriate usage or import the correct trait where needed).
Simply delete the unused `use futures::SinkExt;` statement to fix the warning.
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::config::NatsConfig; | ||
| use crate::domain::types::{NatsHost, Port}; | ||
|
|
||
| #[test] | ||
| fn test_nats_client_creation() { | ||
| let config = Config { | ||
| listen_port: Port::new(8080).unwrap(), | ||
| enable_cors: true, | ||
| nats: NatsConfig { | ||
| host: NatsHost::new("localhost:4222".to_string()).unwrap(), | ||
| max_reconnects: Some(3), | ||
| }, | ||
| allowed_origins: vec![], | ||
| logging: crate::config::LoggingConfig::default(), | ||
| }; | ||
|
|
||
| // Just test that the config is valid for NatsClient | ||
| assert_eq!(config.nats.host.as_str(), "localhost:4222"); | ||
| assert_eq!(config.nats.max_reconnects, Some(3)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test doesn't actually test NatsClient.
Similar to the test in proxy.rs, this test test_nats_client_creation only validates Config construction, not NatsClient. Consider renaming to accurately reflect the test scope.
🔎 Suggested test rename
#[test]
- fn test_nats_client_creation() {
+ fn test_config_structure_for_nats() {
let config = Config {
listen_port: Port::new(8080).unwrap(),
enable_cors: true,
nats: NatsConfig {
host: NatsHost::new("localhost:4222".to_string()).unwrap(),
max_reconnects: Some(3),
},
allowed_origins: vec![],
logging: crate::config::LoggingConfig::default(),
};
- // Just test that the config is valid for NatsClient
+ // Verify config structure is valid for NatsClient initialization
assert_eq!(config.nats.host.as_str(), "localhost:4222");
assert_eq!(config.nats.max_reconnects, Some(3));
}🤖 Prompt for AI Agents
In src/transport/nats.rs around lines 76 to 99, the test named
test_nats_client_creation only validates Config construction and doesn't
exercise NatsClient; either rename the test to reflect that it verifies only
configuration (e.g., test_nats_config_validation) or extend it to instantiate
and exercise NatsClient (create a NatsClient with the config, assert successful
construction and expected behavior or errors). Update the test name or add the
client assertions accordingly and keep assertions focused and minimal.
# Conflicts: # VERSION # src/config/settings.rs # src/handlers.rs # src/main.rs # src/routes.rs # src/service/proxy.rs # src/transport/nats.rs # tests/integration_test.rs
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.