Conversation
Document the pluggable FrameCodec decision and outline\nexecution steps for implementing protocol-specific framing.
Introduce a FrameCodec trait with a length-delimited adapter and\nthread codec selection through WireframeApp and WireframeServer.\n\nUpdate connection handling to use codec-provided decoders/encoders, add\nframed send helpers, and refresh ADR/ExecPlan/user guide documentation.
Add Hotline/MySQL codec examples showing custom framing headers and\nintroduce a TaggedFrame integration test that exercises a custom FrameCodec.\nUpdate the ExecPlan to record completion.
- Add Phase 7.6 codec hardening tasks to the roadmap. - Capture implementation findings and open questions in ADR 004. - Record tooling updates for Nixie and markdownlint.
- Document options to decouple message encoding from bincode. - Link ADR 005 in the codec hardening roadmap.
- Require the examples feature for Hotline and MySQL codecs.
- Require the examples feature for async_stream, metadata_routing, and multi_packet.
Clamp frame lengths to 64B-16MiB guardrails, warning when codecs request larger sizes, and reserve only the clamped size for reads. When decoded envelopes lack correlation IDs, merge header-derived correlation IDs so header-based codecs can round-trip IDs. Update codec tests to match the new clamp behaviour.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduce a generic FrameCodec trait with a default LengthDelimitedFrameCodec; parameterise WireframeApp, server, runtime and connection handling over codecs; thread codec behaviour through framing, encoding/decoding and response paths; add CombinedCodec, lifecycle and middleware types; add example codecs (Hotline, MySQL, RESP), RESP parser/encoder, tests, ADRs, execplan, roadmap and tooling updates. Changes
Sequence Diagram(s)%%{init: {"themeVariables":{"noteBackground":"#eef7ee","actorBorder":"#2b7a2b","sequenceNumber":"#6b6b6b"},"theme":"default"}}%%
sequenceDiagram
autonumber
participant Client as TCP Transport
participant Codec as FrameCodec (F)
participant App as WireframeApp<S,Ctx,E,F>
participant Handler as Protocol Handler
rect rgb(235,247,235)
Note over Client,Codec: Inbound — read bytes → decode → deliver frame
Client->>Codec: Read bytes
Codec->>Codec: decoder.decode()
Codec-->>App: F::Frame
App->>App: payload = F::frame_payload(frame)
App->>App: deserialize payload -> Envelope
App->>App: corr = F::correlation_id(frame)
App->>Handler: Route Envelope
Handler-->>App: Response Envelope
end
rect rgb(250,235,235)
Note over App,Codec: Outbound — serialize → wrap → encode → write
App->>App: serialize response -> payload
App->>Codec: frame = F::wrap_payload(payload)
Codec->>Codec: encoder.encode(frame)
Codec->>Client: Write bytes
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideMake framing pluggable by introducing a FrameCodec trait with a default length-delimited implementation, threading the codec type through WireframeApp and WireframeServer, aligning fragmentation and buffering with codec frame limits, improving correlation-aware deserialization logging, and adding documentation, examples, tests, and tooling updates around codecs and roadmap/ADR conventions. Sequence diagram for connection handling with pluggable FrameCodecsequenceDiagram
actor Client
participant TcpListener
participant WireframeServer
participant Connection as ConnectionTask
participant WireframeApp
participant FrameCodecImpl as FrameCodec
participant FramedStream as Framed~W,C~
participant Serializer
participant HandlerService
Client->>TcpListener: TCP connect
TcpListener->>WireframeServer: incoming connection
WireframeServer->>Connection: spawn_connection_task(stream, factory, hooks)
Connection->>WireframeApp: factory()
WireframeApp-->>Connection: app instance (with codec F)
Connection->>FrameCodecImpl: decoder() / encoder()
FrameCodecImpl-->>Connection: Decoder, Encoder
Connection->>FramedStream: Framed::new(stream, CombinedCodec)
loop per frame
FramedStream->>Connection: next() -> Frame
Connection->>WireframeApp: handle_frame(frame, FrameHandlingContext)
WireframeApp->>FrameCodecImpl: frame_payload(frame)
FrameCodecImpl-->>WireframeApp: payload bytes
WireframeApp->>Serializer: parse(payload) or deserialize(payload)
Serializer-->>WireframeApp: Envelope or error
alt deserialization ok
WireframeApp->>HandlerService: call(ServiceRequest)
HandlerService-->>WireframeApp: Response
WireframeApp->>Serializer: serialize(Response)
Serializer-->>WireframeApp: payload bytes
WireframeApp->>FrameCodecImpl: wrap_payload(payload)
FrameCodecImpl-->>WireframeApp: Frame
WireframeApp->>FramedStream: send(frame)
FramedStream->>Client: write bytes
else deserialization error
WireframeApp->>FrameCodecImpl: correlation_id(frame)
FrameCodecImpl-->>WireframeApp: correlation_id?
WireframeApp->>WireframeApp: handle_decode_failure(correlation_id)
Note over WireframeApp: increment deser_failures, maybe close connection
end
end
Class diagram for FrameCodec trait and default/ example codec implementationsclassDiagram
class FrameCodec {
<<trait>>
+type Frame
+decoder() Decoder~Frame~
+encoder() Encoder~Frame~
+frame_payload(frame: Frame) byte[]
+wrap_payload(payload: byte[]) Frame
+correlation_id(frame: Frame) u64?
+max_frame_length() usize
}
class LengthDelimitedFrameCodec {
+max_frame_length: usize
+new(max_frame_length: usize) LengthDelimitedFrameCodec
+max_frame_length() usize
}
class LengthDelimitedAdapter {
-inner: LengthDelimitedCodec
+new(max_frame_length: usize) LengthDelimitedAdapter
+decode(src: BytesMut) Vec~u8~?
+decode_eof(src: BytesMut) Vec~u8~?
+encode(item: Vec~u8~, dst: BytesMut)
}
class HotlineFrameCodec {
+max_frame_length: usize
+new(max_frame_length: usize) HotlineFrameCodec
+decoder() HotlineAdapter
+encoder() HotlineAdapter
+frame_payload(frame: HotlineFrame) byte[]
+wrap_payload(payload: byte[]) HotlineFrame
+correlation_id(frame: HotlineFrame) u64?
+max_frame_length() usize
}
class HotlineAdapter {
+max_frame_length: usize
+new(max_frame_length: usize) HotlineAdapter
+decode(src: BytesMut) HotlineFrame?
+encode(item: HotlineFrame, dst: BytesMut)
}
class HotlineFrame {
+transaction_id: u32
+payload: Vec~u8~
}
class MysqlFrameCodec {
+max_frame_length: usize
+new(max_frame_length: usize) MysqlFrameCodec
+decoder() MysqlAdapter
+encoder() MysqlAdapter
+frame_payload(frame: MysqlFrame) byte[]
+wrap_payload(payload: byte[]) MysqlFrame
+correlation_id(frame: MysqlFrame) u64?
+max_frame_length() usize
}
class MysqlAdapter {
+max_frame_length: usize
+new(max_frame_length: usize) MysqlAdapter
+decode(src: BytesMut) MysqlFrame?
+encode(item: MysqlFrame, dst: BytesMut)
}
class MysqlFrame {
+sequence_id: u8
+payload: Vec~u8~
}
class CombinedCodec {
+decoder: Decoder
+encoder: Encoder
+new(decoder: Decoder, encoder: Encoder) CombinedCodec
+decode(src: BytesMut) Item?
+decode_eof(src: BytesMut) Item?
+encode(item: Item, dst: BytesMut)
}
FrameCodec <|.. LengthDelimitedFrameCodec
FrameCodec <|.. HotlineFrameCodec
FrameCodec <|.. MysqlFrameCodec
LengthDelimitedFrameCodec ..> LengthDelimitedAdapter : uses
HotlineFrameCodec ..> HotlineAdapter : uses
MysqlFrameCodec ..> MysqlAdapter : uses
HotlineAdapter --> HotlineFrame : decodes
MysqlAdapter --> MysqlFrame : decodes
CombinedCodec ..> Decoder : composes
CombinedCodec ..> Encoder : composes
class Decoder {
<<trait>>
}
class Encoder {
<<trait>>
}
Class diagram for WireframeApp and WireframeServer with pluggable FrameCodecclassDiagram
class WireframeApp {
<<generic S, Ctx, E, F>>
+handlers: HashMap~u32, Handler~E~~
+routes: OnceCell~Arc~HashMap~u32, HandlerService~E~~~~
+middleware: Vec~Arc~dyn Middleware~E~~
+serializer: S
+app_data: HashMap~TypeId, Arc~dyn Any~~
+on_connect: Option~Arc~ConnectionSetup~Ctx~~
+on_disconnect: Option~Arc~ConnectionTeardown~Ctx~~
+protocol: Option~Arc~dyn WireframeProtocol~Frame = F::Frame~~
+push_dlq: Option~mpsc::Sender~Vec~u8~~
+codec: F
+read_timeout_ms: u64
+fragmentation: Option~FragmentationConfig~
+new() WireframeApp
+with_codec(codec: F2) WireframeApp~S, Ctx, E, F2~
+serializer(serializer: S2) WireframeApp~S2, Ctx, E, F~
+buffer_capacity(capacity: usize) WireframeApp~S, Ctx, E, LengthDelimitedFrameCodec~
+protocol() WireframeProtocol~Frame = F::Frame~?
+protocol_hooks() ProtocolHooks~F::Frame, ()~
+send_response(stream, msg)
+send_response_framed_with_codec(framed, msg)
+handle_connection(stream)
}
class WireframeServer {
<<generic FFactory, PreambleT, State, Ser, Ctx, E, Codec>>
+factory: FFactory
+workers: usize
+ready_tx: Option~oneshot::Sender~()~~
+ready_rx: ReadyState
+backoff_config: BackoffConfig
+preamble_timeout: Option~Duration~
+state: State
+_app: PhantomData~(Ser, Ctx, E, Codec)~
+_preamble: PhantomData~PreambleT~
+new(factory: FFactory) WireframeServer
+with_preamble() WireframeServer~FFactory, P, State, Ser, Ctx, E, Codec~
+bind(addr: SocketAddr) WireframeServer~FFactory, PreambleT, Bound, Ser, Ctx, E, Codec~
+run()
}
class FrameHandlingContext {
<<generic E, W, F, C>>
+framed: Framed~W, C~
+deser_failures: u32*
+routes: HashMap~u32, HandlerService~E~~
+fragmentation: Option~FragmentationState~
+codec_marker: PhantomData~F~
}
class ResponseContext {
<<generic S, W, F, C>>
+serializer: S*
+framed: Framed~W, C~
+fragmentation: Option~FragmentationState~
+codec_marker: PhantomData~F~
}
class FrameCodec_Trait {
<<alias FrameCodec>>
}
class Serializer_Trait {
<<alias Serializer>>
}
class Packet_Trait {
<<alias Packet>>
}
class Preamble_Trait {
<<alias Preamble>>
}
class FragmentationConfig {
}
class FragmentationState {
+new(cfg: FragmentationConfig) FragmentationState
}
WireframeApp --> FrameCodec_Trait : codec F FrameCodec
WireframeApp --> Serializer_Trait : S Serializer
WireframeApp --> Packet_Trait : E Packet
WireframeApp --> FragmentationConfig : uses
WireframeApp --> FrameHandlingContext : builds
WireframeApp --> ResponseContext : builds
WireframeServer --> WireframeApp : factory returns
WireframeServer --> Preamble_Trait : T Preamble
FrameHandlingContext --> FragmentationState : manages
ResponseContext --> FragmentationState : manages
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file /// Replace the serializer used for messages.
#[must_use]
pub fn serializer<Ser>(self, serializer: Ser) -> WireframeApp<Ser, C, E>
pub fn serializer<Ser>(self, serializer: Ser) -> WireframeApp<Ser, C, E, F>❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Introduce a single rebuild helper for serializer and codec transitions, using to avoid unnecessary clones. Explicitly document the clippy exception for the helper's argument list.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 16 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/app/connection.rs:76` </location>
<code_context>
+
/// Per-frame processing state bundled for `handle_frame`.
-struct FrameHandlingContext<'a, E, W>
+struct FrameHandlingContext<'a, E, W, F, C>
where
E: Packet,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the connection handling generics by fixing `FrameHandlingContext` and `handle_frame` to the concrete connection codec instead of carrying extra type parameters and `PhantomData`.
You can keep the new `FrameCodec` abstraction but significantly reduce generic / phantom complexity around `handle_frame` and the context types by tying them to the concrete connection codec you actually use.
### 1. Collapse `FrameHandlingContext` generics and drop `PhantomData`
Right now:
```rust
struct FrameHandlingContext<'a, E, W, F, C>
where
E: Packet,
W: AsyncRead + AsyncWrite + Unpin,
F: FrameCodec,
C: Encoder<F::Frame, Error = io::Error>,
{
framed: &'a mut Framed<W, C>,
deser_failures: &'a mut u32,
routes: &'a HashMap<u32, HandlerService<E>>,
fragmentation: &'a mut Option<FragmentationState>,
codec_marker: PhantomData<F>,
}
```
You only ever construct this with `CombinedCodec::new(self.codec.decoder(), self.codec.encoder())`. You can encode that directly in the type and remove `C` and the `codec_marker`:
```rust
type ConnectionCodec<F> =
CombinedCodec<<F as FrameCodec>::Decoder, <F as FrameCodec>::Encoder>;
struct FrameHandlingContext<'a, E, W, F>
where
E: Packet,
W: AsyncRead + AsyncWrite + Unpin,
F: FrameCodec,
{
framed: &'a mut Framed<W, ConnectionCodec<F>>,
deser_failures: &'a mut u32,
routes: &'a HashMap<u32, HandlerService<E>>,
fragmentation: &'a mut Option<FragmentationState>,
}
```
Use it in `process_stream`:
```rust
let codec = CombinedCodec::new(self.codec.decoder(), self.codec.encoder());
let mut framed = Framed::new(stream, codec);
// ...
self.handle_frame(
&frame,
FrameHandlingContext {
framed: &mut framed,
deser_failures: &mut deser_failures,
routes,
fragmentation: &mut fragmentation,
},
)
.await?;
```
### 2. Simplify `handle_frame` signature
You don’t need to parameterise `handle_frame` over an arbitrary encoder type `Cc` — you always use the app’s codec for this connection:
```rust
async fn handle_frame<W>(
&self,
frame: &F::Frame,
ctx: FrameHandlingContext<'_, E, W, F>,
) -> io::Result<()>
where
W: AsyncRead + AsyncWrite + Unpin,
{
let FrameHandlingContext {
framed,
deser_failures,
routes,
fragmentation,
} = ctx;
// existing logic
}
```
This keeps `send_response_framed_with_codec` generic over arbitrary `Cc` (for callers that want a custom encoder) while making the core connection handling path (`process_stream` / `handle_frame`) much easier to read: one fewer generic parameter on the context, no `PhantomData`, and one less type parameter on `handle_frame`.
</issue_to_address>
### Comment 2
<location> `src/app/builder.rs:196` </location>
<code_context>
+ clippy::too_many_arguments,
+ reason = "Rebuild helper maps fields explicitly when swapping serializer/codec types."
+ )]
+ fn rebuild_with_params<S2, F2>(
+ self,
+ serializer: S2,
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the generic `rebuild_with_params` helper and instead inline direct struct reconstruction in `with_codec` and `serializer` to simplify the type bounds and implementation surface.
You can drop `rebuild_with_params` (and the `Default` bounds it introduces) by reconstructing the struct directly in the few places where the type parameters change. That removes a generic helper with many parameters and simplifies the impl surface.
Two focused changes:
1. **Inline `with_codec` without `S: Default`**
```rust
impl<S, C, E, F> WireframeApp<S, C, E, F>
where
S: Serializer + Send + Sync,
C: Send + 'static,
E: Packet,
F: FrameCodec,
{
#[must_use]
pub fn with_codec<F2: FrameCodec>(self, codec: F2) -> WireframeApp<S, C, E, F2> {
let fragmentation = default_fragmentation(codec.max_frame_length());
let WireframeApp {
handlers,
routes: _,
middleware,
serializer,
app_data,
on_connect,
on_disconnect,
protocol: _,
push_dlq,
codec: _,
read_timeout_ms,
fragmentation: _,
} = self;
WireframeApp {
handlers,
routes: OnceCell::new(),
middleware,
serializer,
app_data,
on_connect,
on_disconnect,
protocol: None, // protocol frame type changed
push_dlq,
codec,
read_timeout_ms,
fragmentation,
}
}
}
```
This:
- Removes the `S: Default` requirement.
- Keeps the semantics: routes reset, protocol cleared, fragmentation recomputed.
2. **Inline `serializer` without `F: Default` and without `rebuild_with_params`**
```rust
impl<S, C, E, F> WireframeApp<S, C, E, F>
where
S: Serializer + Send + Sync,
C: Send + 'static,
E: Packet,
F: FrameCodec,
{
#[must_use]
pub fn serializer<Ser>(self, serializer: Ser) -> WireframeApp<Ser, C, E, F>
where
Ser: Serializer + Send + Sync,
{
let WireframeApp {
handlers,
routes: _,
middleware,
serializer: _,
app_data,
on_connect,
on_disconnect,
protocol,
push_dlq,
codec,
read_timeout_ms,
fragmentation,
} = self;
WireframeApp {
handlers,
routes: OnceCell::new(),
middleware,
serializer,
app_data,
on_connect,
on_disconnect,
protocol,
push_dlq,
codec,
read_timeout_ms,
fragmentation,
}
}
}
```
This:
- Removes the `F: Default` bound.
- Keeps all existing behaviour (routes reset; codec, protocol, fragmentation preserved).
With these two changes, `rebuild_with_params` becomes unnecessary and can be removed. That reduces the generic plumbing and avoids the `clippy::too_many_arguments` exemption, while leaving the public API and semantics intact.
</issue_to_address>
### Comment 3
<location> `src/server/config/binding.rs:20` </location>
<code_context>
/// Helper trait alias for wireframe factory functions
-trait WireframeFactory: Fn() -> WireframeApp + Send + Sync + Clone + 'static {}
-impl<F> WireframeFactory for F where F: Fn() -> WireframeApp + Send + Sync + Clone + 'static {}
+trait WireframeFactory<Ser, Ctx, E, Codec>:
+ Fn() -> WireframeApp<Ser, Ctx, E, Codec> + Send + Sync + Clone + 'static
+where
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the generic surface by removing redundant bounds from `WireframeFactory` and optionally collapsing the app generics on `WireframeServer` into a single app-type parameter with associated types.
You can trim a lot of the generic noise without changing behavior by:
1. **Removing redundant bounds from `WireframeFactory`**
2. **Hiding the app type parameters behind a single app-type parameter on `WireframeServer`** (optional but high impact).
---
### 1. Simplify `WireframeFactory` (no Ser/Ctx/E/Codec bounds here)
Right now the factory trait repeats the same bounds you already state on `WireframeServer` impls:
```rust
trait WireframeFactory<Ser, Ctx, E, Codec>:
Fn() -> WireframeApp<Ser, Ctx, E, Codec> + Send + Sync + Clone + 'static
where
Ser: Serializer + Send + Sync,
Ctx: Send + 'static,
E: Packet,
Codec: FrameCodec,
{
}
impl<F, Ser, Ctx, E, Codec> WireframeFactory<Ser, Ctx, E, Codec> for F
where
F: Fn() -> WireframeApp<Ser, Ctx, E, Codec> + Send + Sync + Clone + 'static,
Ser: Serializer + Send + Sync,
Ctx: Send + 'static,
E: Packet,
Codec: FrameCodec,
{
}
```
You can rely on the blanket `impl` for `WireframeServer` to enforce the Ser/Ctx/E/Codec constraints and keep the factory trait minimal:
```rust
trait WireframeFactory<Ser, Ctx, E, Codec>:
Fn() -> WireframeApp<Ser, Ctx, E, Codec> + Send + Sync + Clone + 'static
{
}
impl<F, Ser, Ctx, E, Codec> WireframeFactory<Ser, Ctx, E, Codec> for F
where
F: Fn() -> WireframeApp<Ser, Ctx, E, Codec> + Send + Sync + Clone + 'static,
{
}
```
Then your `WireframeServer` blanket impls keep the actual constraints in one place:
```rust
impl<F, T, S, Ser, Ctx, E, Codec> WireframeServer<F, T, S, Ser, Ctx, E, Codec>
where
F: WireframeFactory<Ser, Ctx, E, Codec>,
T: WireframePreamble,
S: ServerState,
Ser: Serializer + Send + Sync,
Ctx: Send + 'static,
E: Packet,
Codec: FrameCodec,
{
// ...
}
```
This removes duplicated where-clauses without changing the type surface.
---
### 2. Collapse app generics into a single type parameter on `WireframeServer` (bigger but cleaner)
If you want to substantially reduce the generic lists in `binding.rs`, you can wrap the app-related generics into an app-type parameter with associated types.
Example pattern (names adjusted to your domain):
```rust
pub trait WireframeAppTypes {
type Ser: Serializer + Send + Sync;
type Ctx: Send + 'static;
type Packet: Packet;
type Codec: FrameCodec;
}
impl<Ser, Ctx, E, Codec> WireframeAppTypes for WireframeApp<Ser, Ctx, E, Codec>
where
Ser: Serializer + Send + Sync,
Ctx: Send + 'static,
E: Packet,
Codec: FrameCodec,
{
type Ser = Ser;
type Ctx = Ctx;
type Packet = E;
type Codec = Codec;
}
```
Then redefine `WireframeServer` to be generic over a single app type `A` instead of the four individual parameters:
```rust
pub struct WireframeServer<F, T, S, A>
where
A: WireframeAppTypes,
{
factory: F,
// ...
state: S,
_app: std::marker::PhantomData<A>,
_preamble: std::marker::PhantomData<T>,
}
```
Your helper alias and impls in `binding.rs` then shrink to:
```rust
type BoundServer<F, T, A> = WireframeServer<F, T, Bound, A>;
trait WireframeFactory<A>: Fn() -> A + Send + Sync + Clone + 'static
where
A: WireframeAppTypes,
{
}
impl<F, A> WireframeFactory<A> for F
where
F: Fn() -> A + Send + Sync + Clone + 'static,
A: WireframeAppTypes,
{
}
impl<F, T, S, A> WireframeServer<F, T, S, A>
where
F: WireframeFactory<A>,
T: WireframePreamble,
S: ServerState,
A: WireframeAppTypes,
{
fn bind_to_listener(
self,
std_listener: StdTcpListener,
) -> Result<BoundServer<F, T, A>, ServerError> {
let WireframeServer {
factory,
workers,
on_preamble_success,
on_preamble_failure,
ready_tx,
backoff_config,
preamble_timeout,
_app: app_marker,
_preamble: preamble_marker,
..
} = self;
std_listener.set_nonblocking(true).map_err(ServerError::Bind)?;
let tokio_listener = TcpListener::from_std(std_listener).map_err(ServerError::Bind)?;
Ok(WireframeServer {
factory,
workers,
on_preamble_success,
on_preamble_failure,
ready_tx,
backoff_config,
preamble_timeout,
state: Bound {
listener: Arc::new(tokio_listener),
},
_app: app_marker,
_preamble: preamble_marker,
})
}
pub fn bind(self, addr: SocketAddr) -> Result<BoundServer<F, T, A>, ServerError> {
let std_listener = StdTcpListener::bind(addr).map_err(ServerError::Bind)?;
self.bind_existing_listener(std_listener)
}
pub fn bind_existing_listener(
self,
std_listener: StdTcpListener,
) -> Result<BoundServer<F, T, A>, ServerError> {
self.bind_to_listener(std_listener)
}
}
```
This keeps all behavior and type-level guarantees but reduces each impl header from:
```rust
impl<F, T, S, Ser, Ctx, E, Codec> WireframeServer<F, T, S, Ser, Ctx, E, Codec> where ...
```
to:
```rust
impl<F, T, S, A> WireframeServer<F, T, S, A> where ...
```
and eliminates the repeated `(Ser, Ctx, E, Codec)` quartets in this module.
</issue_to_address>
### Comment 4
<location> `src/server/runtime.rs:379` </location>
<code_context>
/// }
/// ```
-pub(super) async fn accept_loop<F, T, L>(
+pub(super) async fn accept_loop<F, T, L, Ser, Ctx, E, Codec>(
listener: Arc<L>,
factory: F,
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the repeated `Ser, Ctx, E, Codec` generics in the runtime functions with a single `App` type parameter (optionally via a small trait) to simplify the signatures and keep the same flexibility.
You can keep the new flexibility without duplicating all the generics and bounds in the accept loop by introducing a single `App` generic and tightening its bounds, instead of threading `Ser, Ctx, E, Codec` everywhere.
Right now, `accept_loop`/`accept_iteration` are carrying:
```rust
pub(super) async fn accept_loop<F, T, L, Ser, Ctx, E, Codec>(...)
where
F: Fn() -> WireframeApp<Ser, Ctx, E, Codec> + Send + Sync + Clone + 'static,
T: Preamble,
L: AcceptListener + Send + Sync + 'static,
Ser: Serializer + FrameMetadata<Frame = Envelope> + Send + Sync + 'static,
Ctx: Send + 'static,
E: Packet,
Codec: FrameCodec,
{ ... }
async fn accept_iteration<F, T, L, Ser, Ctx, E, Codec>(...)
where
F: Fn() -> WireframeApp<Ser, Ctx, E, Codec> + Send + Sync + Clone + 'static,
T: Preamble,
L: AcceptListener + Send + Sync + 'static,
Ser: Serializer + FrameMetadata<Frame = Envelope> + Send + Sync + 'static,
Ctx: Send + 'static,
E: Packet,
Codec: FrameCodec,
{ ... }
```
But inside these functions you only really use the app factory and the fact that the app’s serializer implements `FrameMetadata<Frame = Envelope>` (per the other review).
You can make the signatures much lighter by:
1. Introducing a single `App` type parameter with the minimum constraints you need.
2. Expressing `F` in terms of this `App`.
3. Letting `WireframeServer`’s own generic parameters ensure that `App = WireframeApp<Ser, Ctx, E, Codec>` where needed, without repeating them in the accept loop.
For example:
```rust
// Narrow type used by the runtime
pub trait RuntimeApp: FrameMetadata<Frame = Envelope> + Send + Sync + 'static {}
// Blanket impl for your concrete app type; keeps all functionality
impl<Ser, Ctx, E, Codec> RuntimeApp for WireframeApp<Ser, Ctx, E, Codec>
where
Ser: Serializer + FrameMetadata<Frame = Envelope> + Send + Sync + 'static,
Ctx: Send + 'static,
E: Packet,
Codec: FrameCodec,
{}
```
Then simplify the loop signatures:
```rust
pub(super) async fn accept_loop<F, T, L, App>(
listener: Arc<L>,
factory: F,
options: AcceptLoopOptions<T>,
) where
F: Fn() -> App + Send + Sync + Clone + 'static,
T: Preamble,
L: AcceptListener + Send + Sync + 'static,
App: RuntimeApp,
{
// body unchanged
}
async fn accept_iteration<F, T, L, App>(
listener: &Arc<L>,
factory: &F,
handles: &AcceptHandles<'_, T>,
delay: Duration,
) -> Option<Duration>
where
F: Fn() -> App + Send + Sync + Clone + 'static,
T: Preamble,
L: AcceptListener + Send + Sync + 'static,
App: RuntimeApp,
{
// body unchanged
}
```
On the `WireframeServer` side, you can bind `App` to the existing generics once, instead of repeating them in each function:
```rust
impl<F, T, Ser, Ctx, E, Codec> WireframeServer<F, T, Bound, Ser, Ctx, E, Codec>
where
F: Fn() -> WireframeApp<Ser, Ctx, E, Codec> + Send + Sync + Clone + 'static,
T: Preamble,
Ser: Serializer + FrameMetadata<Frame = Envelope> + Send + Sync + 'static,
Ctx: Send + 'static,
E: Packet,
Codec: FrameCodec,
{
type App = WireframeApp<Ser, Ctx, E, Codec>;
pub async fn run(self) {
// when calling into the runtime, you now rely on App: RuntimeApp
accept_loop::<_, _, _, Self::App>(/* ... */).await;
}
}
```
This keeps all the functionality (you still support arbitrary `Ser, Ctx, E, Codec` on `WireframeApp`), but it:
* Removes the need to repeat the generic bounds in `accept_loop` and `accept_iteration`.
* Simplifies tests and examples back to:
```rust
accept_loop(
listener,
|| WireframeApp::default(),
AcceptLoopOptions::<()> { /* ... */ },
).await;
```
If you don’t want to introduce a trait, you can still get a big win by going halfway and introducing a simple `App` type parameter with the minimal bound:
```rust
pub(super) async fn accept_loop<F, T, L, App>(...)
where
F: Fn() -> App + Send + Sync + Clone + 'static,
T: Preamble,
L: AcceptListener + Send + Sync + 'static,
App: FrameMetadata<Frame = Envelope> + Send + Sync + 'static,
{ ... }
```
and rely on the `WireframeServer` impl to ensure that `App = WireframeApp<Ser, Ctx, E, Codec>` satisfies the extra `Serializer + Packet + FrameCodec` constraints elsewhere, rather than restating them in the accept loop.
</issue_to_address>
### Comment 5
<location> `src/codec.rs:19` </location>
<code_context>
+use std::io;
+use tokio_util::codec::{Decoder, Encoder};
+
+/// Trait for pluggable frame codecs supporting different wire protocols.
+pub trait FrameCodec: Send + Sync + 'static {
+ /// Frame type produced by decoding.
</code_context>
<issue_to_address>
**issue (complexity):** Consider tightening the FrameCodec API (using associated types, Bytes frames, and simpler size checks) to reduce indirection and make the codec layer easier to compose and maintain.
You can simplify this substantially without losing any functionality by tightening the trait surface and reusing `LengthDelimitedCodec` directly.
### 1. Use associated types for encoder/decoder
Returning `impl Decoder/Encoder` from `FrameCodec` forces each implementation to have anonymous, non‑nameable types and makes it harder to store heterogeneous codecs or build combinators. Associated types keep the graph explicit and remove the need for ad‑hoc wrapper types elsewhere.
```rust
pub trait FrameCodec: Send + Sync + 'static {
type Frame: Send + Sync + 'static;
type Decoder: Decoder<Item = Self::Frame, Error = io::Error> + Send;
type Encoder: Encoder<Self::Frame, Error = io::Error> + Send;
fn decoder(&self) -> Self::Decoder;
fn encoder(&self) -> Self::Encoder;
fn frame_payload(frame: &Self::Frame) -> &[u8];
fn wrap_payload(payload: Vec<u8>) -> Self::Frame;
fn correlation_id(_frame: &Self::Frame) -> Option<u64> { None }
fn max_frame_length(&self) -> usize;
}
```
This preserves your current functionality but reduces the “dynamic” feel of the API and makes composition easier.
### 2. Drop `LengthDelimitedAdapter` by using `Bytes` as the frame
`LengthDelimitedAdapter` only translates `Bytes` ↔ `Vec<u8>` and adds an extra layer around `LengthDelimitedCodec`. If your frames are just payloads, you can keep everything zero‑copy by making `Frame = Bytes` and using `LengthDelimitedCodec` directly:
```rust
#[derive(Clone, Debug)]
pub struct LengthDelimitedFrameCodec {
max_frame_length: usize,
}
impl LengthDelimitedFrameCodec {
#[must_use]
pub fn new(max_frame_length: usize) -> Self {
Self {
max_frame_length: clamp_frame_length(max_frame_length),
}
}
fn new_inner_codec(&self) -> LengthDelimitedCodec {
LengthDelimitedCodec::builder()
.max_frame_length(self.max_frame_length)
.new_codec()
}
}
impl FrameCodec for LengthDelimitedFrameCodec {
type Frame = Bytes;
type Decoder = LengthDelimitedCodec;
type Encoder = LengthDelimitedCodec;
fn decoder(&self) -> Self::Decoder {
self.new_inner_codec()
}
fn encoder(&self) -> Self::Encoder {
self.new_inner_codec()
}
fn frame_payload(frame: &Self::Frame) -> &[u8] {
frame.as_ref()
}
fn wrap_payload(payload: Vec<u8>) -> Self::Frame {
Bytes::from(payload)
}
fn max_frame_length(&self) -> usize {
self.max_frame_length
}
}
```
Callers that need `Vec<u8>` can still do `frame.to_vec()`, but the core codec remains zero‑copy and doesn’t need a bespoke adapter.
### 3. Consider simplifying the “reject oversized payloads” policy
Right now the test `length_delimited_codec_rejects_oversized_payloads` forces the encoder to perform its own length check and fail with `InvalidInput` for oversized frames. That means:
- You can’t rely solely on `LengthDelimitedCodec`’s `max_frame_length`, since it only enforces this on decode.
- You’ll need to track `max_frame_length` again in your encoder logic, which re‑introduces duplication.
If you don’t need strict encode‑time validation, you can simplify by treating `max_frame_length` as a decode‑time bound only and dropping that test (and any encode‑side clamping). If you do need the check, keep the associated‑type simplification and still avoid the adapter by checking length before calling `encode`:
```rust
impl FrameCodec for LengthDelimitedFrameCodec {
// as above...
fn encoder(&self) -> impl Encoder<Self::Frame, Error = io::Error> + Send {
let max = self.max_frame_length;
// small local wrapper instead of a separate top‑level adapter type
struct CheckedEncoder {
inner: LengthDelimitedCodec,
max: usize,
}
impl Encoder<Bytes> for CheckedEncoder {
type Error = io::Error;
fn encode(&mut self, item: Bytes, dst: &mut BytesMut) -> Result<(), Self::Error> {
if item.len() > self.max {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
"frame exceeds max_frame_length",
));
}
self.inner.encode(item, dst)
}
}
CheckedEncoder { inner: self.new_inner_codec(), max }
}
}
```
This keeps your current semantics (encoded frames are validated) but avoids a standalone `LengthDelimitedAdapter` type and still reuses `LengthDelimitedCodec` as the core implementation.
</issue_to_address>
### Comment 6
<location> `src/codec.rs:23` </location>
<code_context>
+use tokio_util::codec::{Decoder, Encoder};
+
+/// Trait for pluggable frame codecs supporting different wire protocols.
+pub trait FrameCodec: Send + Sync + 'static {
+ /// Frame type produced by decoding.
+ type Frame: Send + Sync + 'static;
</code_context>
<issue_to_address>
**issue (review_instructions):** Add both unit and behavioural tests that exercise the new FrameCodec abstraction and its default LengthDelimitedFrameCodec in realistic I/O paths.
Cover the new codec layer with more than the in-module unit tests. The current tests only validate basic round-tripping and clamping on the adapter; they do not exercise:
- Using `FrameCodec` with a `Framed` stream end-to-end (decode, route, encode).
- Failure paths (e.g. malformed headers, EOF mid-frame) and how they surface.
- Integration with `WireframeApp` and `WireframeServer` when using non-default codecs.
Add behavioural tests under `tests/` (for example, in `tests/frame_codec.rs`) that stand up a minimal app/server using `LengthDelimitedFrameCodec` and assert that requests and responses flow correctly via the new trait, and complement them with focused unit tests for error cases on the adapters.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 7
<location> `src/app/connection.rs:90` </location>
<code_context>
}
-impl<S, C, E> WireframeApp<S, C, E>
+impl<S, C, E, F> WireframeApp<S, C, E, F>
where
S: Serializer + Send + Sync,
</code_context>
<issue_to_address>
**issue (review_instructions):** Cover the new codec-parameterised connection path with behavioural tests to ensure framing, timeouts, and fragmentation still work.
You introduce a codec-parameterised `WireframeApp` and change the connection handling to use `FrameCodec` (e.g. `CombinedCodec`, `F::wrap_payload`, `F::frame_payload`, and `self.codec.max_frame_length()`), but there are no new tests exercising these changes.
Add behavioural tests that:
- Start a `WireframeServer` / accept loop with the default `LengthDelimitedFrameCodec` and verify end-to-end request/response behaviour still matches the previous semantics (including read timeouts and max-frame enforcement).
- Use a custom codec implementation to confirm that frames are correctly decoded, routed, and encoded.
Also add unit-level tests around `fragmentation_config`, `decode_envelope`, and the new clamping behaviour in `handle_connection` to ensure envelope parsing and buffer sizing remain correct with arbitrary codecs.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 8
<location> `src/app/frame_handling.rs:51` </location>
<code_context>
}
-pub(crate) struct ResponseContext<'a, S, W>
+pub(crate) struct ResponseContext<'a, S, W, F, C>
where
S: Serializer + Send + Sync,
</code_context>
<issue_to_address>
**issue (review_instructions):** Test the updated response forwarding logic with codec-aware framing, including fragmented responses and error paths.
`ResponseContext` and `forward_response` now depend on a generic `FrameCodec` via `send_response_payload::<F, W, C>`, which wraps payloads into frames instead of sending raw `Vec<u8>` to a `LengthDelimitedCodec`. This is a behavioural change in how responses are framed.
Add tests that:
- Verify `forward_response` correctly serializes and sends responses using the default length-delimited codec, including fragmented `Response::Stream` cases.
- Confirm that error logging and metrics (`inc_handler_errors`) still fire as expected when `send_response_payload` fails.
- Exercise the new path with at least one non-default codec to ensure `F::wrap_payload` is applied consistently for handler responses.
These can be a mix of unit tests (mocked `HandlerService` and in-memory framed stream) and higher-level behavioural tests using a minimal app.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 9
<location> `src/app/connection.rs:376` </location>
<code_context>
/// Increment deserialization failures and close the connection if the threshold is exceeded.
fn handle_decode_failure(
deser_failures: &mut u32,
+ correlation_id: Option<u64>,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add a regression test for the updated handle_decode_failure logging and threshold behaviour.
`handle_decode_failure` is changed to accept an explicit `correlation_id` and log it, and it still enforces the `MAX_DESER_FAILURES` threshold. This is effectively a bug fix / observability fix but there is no test covering it.
Add a focused unit test that:
- Calls `decode_envelope` with frames that fail deserialization and asserts that `handle_decode_failure` increments the counter and returns the expected `io::Error` once the threshold is exceeded.
- Verifies that the correlation ID is plumbed through when available (you can assert on the log record via a test logger or, at minimum, on the value passed into `handle_decode_failure`).
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
Bug fixes *must* be demonstrated by a test.
</details>
</issue_to_address>
### Comment 10
<location> `examples/hotline_codec.rs:111` </location>
<code_context>
+ }
+}
+
+impl FrameCodec for HotlineFrameCodec {
+ type Frame = HotlineFrame;
+
</code_context>
<issue_to_address>
**issue (review_instructions):** Back the new example codecs with unit tests that validate their framing logic independently of the example binaries.
The Hotline and MySQL codecs add new framing behaviour (custom headers, length checks, correlation IDs) but are only exercised via examples. Examples compiling is not sufficient coverage for these new features.
Extract the core adapter logic into testable units (or import the example types in a `tests/` module) and add unit tests that:
- Validate correct encoding/decoding of headers and payloads, including boundary sizes.
- Assert that oversized or malformed frames produce the expected `io::Error` variants.
- Confirm that `correlation_id` is derived correctly from the protocol-specific header fields.
Complement these with at least one behavioural test that wires a `WireframeApp` with a custom codec and sends a request through a `Framed` stream using the example codec, to satisfy the behavioural-test requirement.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 11
<location> `docs/adr-004-pluggable-protocol-codecs.md:21` </location>
<code_context>
+Hotline, MySQL, and Redis RESP) from reusing Wireframe's routing, middleware,
+and serialisation infrastructure.
+
+We need a pluggable framing layer that can:
+
+- Decode frames with protocol-specific headers.
</code_context>
<issue_to_address>
**issue (review_instructions):** The sentence uses first-person plural ("We"), which the style instructions prohibit.
Consider rephrasing in an impersonal form, for example: "A pluggable framing layer is required that can:" or "The system requires a pluggable framing layer that can:"
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>
### Comment 12
<location> `docs/adr-004-pluggable-protocol-codecs.md:19` </location>
<code_context>
+`src/app/connection.rs` and `src/app/frame_handling.rs`). This makes framing
+inflexible and prevents protocols with alternative framing rules (such as
+Hotline, MySQL, and Redis RESP) from reusing Wireframe's routing, middleware,
+and serialisation infrastructure.
+
+We need a pluggable framing layer that can:
</code_context>
<issue_to_address>
**issue (review_instructions):** "serialisation" uses -isation spelling instead of the requested -ization/-izer style for en-GB-oxendic.
To match the specified en-GB-oxendic spelling convention, please change "serialisation" to "serialization" here.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 13
<location> `docs/execplans/pluggable-protocol-codecs.md:13` </location>
<code_context>
+
+## Purpose / Big Picture
+
+Wireframe must support multiple wire protocol framing schemes (Hotline, MySQL,
+and Redis RESP) without rewriting routing, middleware, or serialisation. A new
+pluggable codec layer allows protocol-specific framing while preserving default
</code_context>
<issue_to_address>
**issue (review_instructions):** "RESP" (Redis Serialization Protocol) is introduced later in this sentence without being expanded on first use.
To satisfy the acronym rule, consider expanding RESP on first use, for example: "Redis Serialization Protocol (RESP)".
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 14
<location> `docs/execplans/pluggable-protocol-codecs.md:14` </location>
<code_context>
+## Purpose / Big Picture
+
+Wireframe must support multiple wire protocol framing schemes (Hotline, MySQL,
+and Redis RESP) without rewriting routing, middleware, or serialisation. A new
+pluggable codec layer allows protocol-specific framing while preserving default
+length-delimited behaviour for existing users. Success is visible when a custom
</code_context>
<issue_to_address>
**issue (review_instructions):** "serialisation" uses -isation spelling instead of the requested -ization/-izer style for en-GB-oxendic.
Please update "serialisation" to "serialization" here to align with the specified en-GB-oxendic (-ize/-ization) convention.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 15
<location> `docs/adr-005-serializer-abstraction.md:1` </location>
<code_context>
+# Architectural decision record (ADR) 005: serialiser abstraction beyond bincode
+
+## Status
</code_context>
<issue_to_address>
**issue (review_instructions):** "serialiser" in the heading uses -iser instead of the requested -izer form for en-GB-oxendic.
To follow the en-GB-oxendic guidance, this heading should use "serializer" rather than "serialiser".
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 16
<location> `docs/adr-005-serializer-abstraction.md:23` </location>
<code_context>
+targets derivable encoders/decoders, and message versioning is expected to be
+an evolution path.[^router-design][^message-versioning]
+
+We need to decouple message encoding/decoding from bincode while preserving
+backward compatibility and performance.
+
</code_context>
<issue_to_address>
**issue (review_instructions):** This sentence uses first-person plural ("We"), which is disallowed by the style rules.
Consider rephrasing in an impersonal form, for example: "Message encoding/decoding must be decoupled from bincode while preserving…"
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/server/runtime.rs (1)
1-675: File exceeds 400-line guideline.At 675 lines, this file exceeds the 400-line limit specified in the coding guidelines. Decompose the module by extracting:
BackoffConfigand its impl into a separatebackoff.rssubmodule.- Test helpers (
setup_backoff_mock_listener,assert_backoff_intervals) and tests into a dedicatedtests.rssubmodule.As per coding guidelines: "Files must not exceed 400 lines in length... Large modules must be decomposed."
src/app/connection.rs (1)
1-419: File slightly exceeds 400-line guideline.At 419 lines, this file marginally exceeds the 400-line limit. Extract
CombinedCodecand its implementations into a separatecombined_codec.rssubmodule to improve modularity.As per coding guidelines: "Files must not exceed 400 lines in length."
src/app/builder.rs (1)
1-448: File exceeds 400-line guideline.At 448 lines, this file exceeds the 400-line limit. Extract the following into separate submodules:
- Connection setup/teardown types and impls into
lifecycle.rs.- The
HandlerandMiddlewaretypes intomiddleware_types.rs(or relocate to the middleware module).As per coding guidelines: "Files must not exceed 400 lines in length... Large modules must be decomposed."
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (26)
.markdownlint-cli2.jsoncCargo.tomlMakefiledocs/adr-001-multi-packet-streaming-response-api.mddocs/adr-002-streaming-requests-and-shared-message-assembly.mddocs/adr-003-replace-cucumber-with-rstest-bdd.mddocs/adr-004-pluggable-protocol-codecs.mddocs/adr-005-serializer-abstraction.mddocs/documentation-style-guide.mddocs/execplans/pluggable-protocol-codecs.mddocs/roadmap.mddocs/users-guide.mdexamples/hotline_codec.rsexamples/mysql_codec.rssrc/app/builder.rssrc/app/connection.rssrc/app/frame_handling.rssrc/codec.rssrc/lib.rssrc/server/config/binding.rssrc/server/config/mod.rssrc/server/config/preamble.rssrc/server/connection.rssrc/server/mod.rssrc/server/runtime.rstests/frame_codec.rs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
src/lib.rssrc/app/frame_handling.rssrc/server/mod.rsexamples/mysql_codec.rstests/frame_codec.rsexamples/hotline_codec.rssrc/server/config/binding.rssrc/server/connection.rssrc/server/config/preamble.rssrc/server/runtime.rssrc/app/connection.rssrc/codec.rssrc/app/builder.rssrc/server/config/mod.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/lib.rssrc/app/frame_handling.rssrc/server/mod.rsexamples/mysql_codec.rstests/frame_codec.rsexamples/hotline_codec.rssrc/server/config/binding.rssrc/server/connection.rssrc/server/config/preamble.rssrc/server/runtime.rssrc/app/connection.rssrc/codec.rssrc/app/builder.rssrc/server/config/mod.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/contents.md)
Follow the documentation style guide conventions when writing project documentation
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: -ize suffix in words like 'realize' and 'organization', -lyse suffix in words like 'analyse', -our suffix in words like 'colour', -re suffix in words like 'centre', double 'l' in words like 'cancelled', maintain 'e' in words like 'likeable', -ogue suffix in words like 'catalogue'
The word 'outwith' is acceptable in British English documentation
Keep US spelling when used in an API (for example, 'color')
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns: 'Lille Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations for Markdown files
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change in documentation
Prefer inline links using 'text' or angle brackets around the URL in documentation
Ensure blank lines before and after bulleted lists and fenced blocks in documentation
Ensure tables have a delimiter line below the header row in documentation
Expand any uncommon acronym on first use in documentation (for example, Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with '[^label]' in documentation
Include Mermaid diagrams where they add clarity to documentation
When embedding figures in documentation, use '' and provide brief alt text describing the content
Add a short description before each Mermaid diagram so screen readers can understand itWrite official documentation with se...
Files:
docs/roadmap.mddocs/users-guide.mddocs/adr-005-serializer-abstraction.mddocs/adr-004-pluggable-protocol-codecs.mddocs/documentation-style-guide.mddocs/execplans/pluggable-protocol-codecs.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
Every doctest should validate the public API of a crate from the perspective of an external user, treating each documentation test as a separate temporary crate that imports the library as an external dependency
Files:
docs/roadmap.mddocs/users-guide.mddocs/adr-005-serializer-abstraction.mddocs/adr-004-pluggable-protocol-codecs.mddocs/documentation-style-guide.mddocs/execplans/pluggable-protocol-codecs.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes in Markdown.
Files:
docs/roadmap.mddocs/users-guide.mddocs/adr-005-serializer-abstraction.mddocs/adr-004-pluggable-protocol-codecs.mddocs/documentation-style-guide.mddocs/execplans/pluggable-protocol-codecs.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/users-guide.mddocs/adr-005-serializer-abstraction.mddocs/adr-004-pluggable-protocol-codecs.mddocs/documentation-style-guide.mddocs/execplans/pluggable-protocol-codecs.md
docs/**
📄 CodeRabbit inference engine (docs/roadmap.md)
Provide comprehensive documentation including user guide, tutorials, and troubleshooting guidance
Files:
docs/roadmap.mddocs/users-guide.mddocs/adr-005-serializer-abstraction.mddocs/adr-004-pluggable-protocol-codecs.mddocs/documentation-style-guide.mddocs/execplans/pluggable-protocol-codecs.md
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3"). Wildcard (*) or open-ended inequality (>=) version requirements are strictly forbidden. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🧬 Code graph analysis (12)
src/app/frame_handling.rs (1)
src/codec.rs (2)
wrap_payload(37-37)wrap_payload(131-131)
src/server/mod.rs (2)
src/app/builder.rs (1)
serializer(404-413)src/app/connection.rs (1)
serializer(208-208)
examples/mysql_codec.rs (1)
examples/hotline_codec.rs (11)
new(29-29)new(38-38)max_frame_length(133-133)decode(45-79)encode(85-108)decoder(114-116)encoder(118-120)frame_payload(122-122)wrap_payload(124-129)correlation_id(131-131)main(136-143)
tests/frame_codec.rs (1)
src/codec.rs (16)
new(60-64)new(84-90)max_frame_length(45-45)max_frame_length(68-68)max_frame_length(133-133)decode(97-101)encode(113-115)decoder(28-28)decoder(121-123)encoder(31-31)encoder(125-127)frame_payload(34-34)frame_payload(129-129)wrap_payload(37-37)wrap_payload(131-131)correlation_id(42-42)
src/server/config/binding.rs (6)
src/app/builder.rs (1)
serializer(404-413)src/app/connection.rs (1)
serializer(208-208)tests/middleware_order.rs (1)
serializer(94-94)tests/fragment_transport.rs (1)
serializer(135-135)tests/preamble.rs (7)
WireframeServer(61-63)WireframeServer(247-248)WireframeServer(283-284)WireframeServer(334-335)WireframeServer(370-371)WireframeServer(431-432)WireframeServer(527-531)src/server/config/tests.rs (2)
WireframeServer(73-73)WireframeServer(204-206)
src/server/connection.rs (4)
src/preamble.rs (1)
read_preamble(112-138)src/app/builder.rs (2)
serializer(404-413)default(125-142)src/app/connection.rs (2)
serializer(208-208)process_stream(272-320)src/server/test_util.rs (1)
factory(18-20)
src/server/config/preamble.rs (3)
src/app/builder.rs (1)
serializer(404-413)src/app/connection.rs (1)
serializer(208-208)src/server/test_util.rs (1)
WireframeServer(89-89)
src/server/runtime.rs (5)
src/app/builder.rs (1)
serializer(404-413)src/app/connection.rs (1)
serializer(208-208)tests/middleware_order.rs (1)
serializer(94-94)tests/fragment_transport.rs (1)
serializer(135-135)tests/common/mod.rs (1)
factory(28-31)
src/app/connection.rs (1)
src/codec.rs (14)
decoder(28-28)decoder(121-123)encoder(31-31)encoder(125-127)decode(97-101)encode(113-115)wrap_payload(37-37)wrap_payload(131-131)max_frame_length(45-45)max_frame_length(68-68)max_frame_length(133-133)correlation_id(42-42)frame_payload(34-34)frame_payload(129-129)
src/codec.rs (4)
src/frame/format.rs (1)
bytes(54-54)src/request.rs (1)
value(192-197)tests/frame_codec.rs (10)
decoder(100-102)encoder(104-106)frame_payload(108-108)wrap_payload(110-110)correlation_id(112-112)max_frame_length(114-114)new(27-27)new(36-36)decode(43-67)encode(73-94)src/app/connection.rs (3)
new(45-45)decode(55-57)encode(70-72)
src/app/builder.rs (2)
src/codec.rs (6)
clamp_frame_length(15-17)new(60-64)new(84-90)max_frame_length(45-45)max_frame_length(68-68)max_frame_length(133-133)src/fragment/config.rs (1)
for_frame_budget(35-53)
src/server/config/mod.rs (3)
src/app/builder.rs (1)
serializer(404-413)src/app/connection.rs (1)
serializer(208-208)src/server/config/tests.rs (2)
WireframeServer(73-73)WireframeServer(204-206)
🪛 LanguageTool
docs/roadmap.md
[uncategorized] ~396-~396: Do not mix variants of the same word (‘adapter’ and ‘adaptor’) within a single text.
Context: ...ate LengthDelimitedFrameCodec and any adapters to use the new payload type. - [ ...
(EN_WORD_COHERENCY)
[uncategorized] ~398-~398: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: .... - [ ] Reuse a per-connection encoder so sequence counters can advance deter...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~406-~406: Possible missing comma found.
Context: ...ame is surfaced to handlers or protocol hooks and add tests for partial-frame clo...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~413-~413: Do not mix variants of the same word (‘adapter’ and ‘adaptor’) within a single text.
Context: ...ation. - [ ] Update the default codec adapter to avoid Bytes to Vec<u8> copying ...
(EN_WORD_COHERENCY)
[grammar] ~416-~416: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ...hmark to confirm payloads reuse the receive buffer where possible. ### 7.6.2. Frag...
(PREPOSITION_VERB)
[uncategorized] ~418-~418: Do not mix variants of the same word (‘adapter’ and ‘adaptor’) within a single text.
Context: ...er where possible. ### 7.6.2. Fragment adapter alignment - [ ] 7.6.2.1. Introduce a `...
(EN_WORD_COHERENCY)
[uncategorized] ~426-~426: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ilder. - [ ] Expose a public purge API so callers can drive timeout eviction. -...
(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~428-~428: Do not mix variants of the same word (‘serialisation’ and ‘serialization’) within a single text.
Context: ...order for codec, fragmentation, and serialisation layers. - [ ] Define and implement du...
(EN_WORD_COHERENCY)
[uncategorized] ~460-~460: Do not mix variants of the same word (‘adapter’ and ‘adaptor’) within a single text.
Context: ... a serializer-agnostic message trait or adapter layer for Message types. - [ ] ...
(EN_WORD_COHERENCY)
[style] ~463-~463: Would you like to use the Oxford spelling “deserialization”? The spelling ‘deserialisation’ is also correct.
Context: ...ne how frame metadata is exposed to the deserialisation context to enable version negotiati...
(OXFORD_SPELLING_Z_NOT_S)
docs/adr-005-serializer-abstraction.md
[style] ~17-~17: Would you like to use the Oxford spelling “serialization”? The spelling ‘serialisation’ is also correct.
Context: ...fork the library. This leaks a concrete serialisation choice into otherwise protocol-agnostic...
(OXFORD_SPELLING_Z_NOT_S)
[grammar] ~21-~21: Please add a punctuation mark at the end of paragraph.
Context: ...g is expected to be an evolution path.[^router-design][^message-versioning] We need to decou...
(PUNCTUATION_PARAGRAPH_END)
[style] ~28-~28: Would you like to use the Oxford spelling “serialization”? The spelling ‘serialisation’ is also correct.
Context: ...Decision Drivers - Support alternative serialisation formats without forking. - Preserve exi...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~30-~30: Would you like to use the Oxford spelling “deserialization”? The spelling ‘deserialisation’ is also correct.
Context: ...ed integrations where possible. - Allow deserialisation to access protocol metadata for version...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~71-~71: Would you like to use the Oxford spelling “deserializing”? The spelling ‘deserialising’ is also correct.
Context: ... | Medium | | Metadata-aware deserialising | Poor | Strong ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~75-~75: Would you like to use the Oxford spelling “serialization”? The spelling ‘serialisation’ is also correct.
Context: ... | Table 1: Trade-offs between serialisation abstraction options. ## Decision Outc...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~82-~82: Would you like to use the Oxford spelling “deserialization”? The spelling ‘deserialisation’ is also correct.
Context: ...allows frame metadata to participate in deserialisation, supports version negotiation, and avoi...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~83-~83: Possible missing article found.
Context: ...sion negotiation, and avoids committing Wireframe to a single serialiser. ## Goals and N...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~132-~132: Would you like to use the Oxford spelling “serialization”? The spelling ‘serialisation’ is also correct.
Context: ...Architectural Rationale Decoupling the serialisation boundary aligns with Wireframe’s goal o...
(OXFORD_SPELLING_Z_NOT_S)
docs/adr-004-pluggable-protocol-codecs.md
[typographical] ~5-~5: To join two clauses or introduce examples, consider using an em dash.
Context: ...ble protocol codecs ## Status Accepted - 2025-12-30: introduce a FrameCodec tra...
(DASH_RULE)
[style] ~19-~19: Would you like to use the Oxford spelling “serialization”? The spelling ‘serialisation’ is also correct.
Context: ...ng Wireframe's routing, middleware, and serialisation infrastructure. We need a pluggable fr...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~41-~41: Since ownership is already implied, this phrasing may be redundant.
Context: ...llow multiple wire protocols to plug in their own framing rules. - Preserve protocol meta...
(PRP_OWN)
[grammar] ~79-~79: The usual collocation for “Full” is “of”, not “with”. Did you mean “Full of”?
Context: ...compatibility | Full | Full with defaults | Medium (API changes)...
(FULL_WITH_OF)
[uncategorized] ~86-~86: Possible missing comma found.
Context: ...trait with a default length-delimited implementation and makeWireframeApp` generic over a ...
(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~145-~145: Would you like to use the Oxford spelling “serialization”? The spelling ‘serialisation’ is also correct.
Context: ...ction. - Replacing the existing message serialisation format. - Introducing asynchronous code...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~146-~146: Would you like to use the Oxford spelling “initialization”? The spelling ‘initialisation’ is also correct.
Context: ...ormat. - Introducing asynchronous codec initialisation. ## Migration Plan ### Phase 1: Trait...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~156-~156: Would you like to use the Oxford spelling “Parameterize”? The spelling ‘Parameterise’ is also correct.
Context: ... default codec behaviour. ### Phase 2: Parameterise WireframeApp (breaking change) - Add...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~160-~160: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...nd update the default implementation. - Add .with_codec() for builder ergonomics....
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~164-~164: Would you like to use the Oxford spelling “Parameterize”? The spelling ‘Parameterise’ is also correct.
Context: ... Phase 3: Update connection handling - Parameterise FrameHandlingContext and `ResponseCon...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~199-~199: Since ownership is already implied, this phrasing may be redundant.
Context: ... while the Connection actor retains its own framing logic. This makes protocol hook...
(PRP_OWN)
docs/documentation-style-guide.md
[style] ~232-~232: Would you like to use the Oxford spelling “summarize”? The spelling ‘summarise’ is also correct.
Context: ...tion:** State the chosen approach and summarise the rationale. For Proposed ADRs, des...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~249-~249: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sections (e.g. phases, option names). - Use tables to compare options when multiple...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~252-~252: The singular proper name ‘Rust’ must be used with a third-person or a past tense verb.
Context: ...hes. Use no_run for illustrative Rust code that should not be executed. - Add sc...
(HE_VERB_AGR)
docs/execplans/pluggable-protocol-codecs.md
[style] ~14-~14: Would you like to use the Oxford spelling “serialization”? The spelling ‘serialisation’ is also correct.
Context: ...thout rewriting routing, middleware, or serialisation. A new pluggable codec layer allows pro...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~118-~118: Would you like to use the Oxford spelling “initialize”? The spelling ‘initialise’ is also correct.
Context: .... - Add codec: F to the struct and initialise it in Default. - Add builder metho...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~125-~125: Would you like to use the Oxford spelling “Parameterize”? The spelling ‘Parameterise’ is also correct.
Context: ...with FrameCodec decoder/encoder. - Parameterise FrameHandlingContext and `ResponseCon...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~127-~127: Would you like to use the Oxford spelling “deserialization”? The spelling ‘deserialisation’ is also correct.
Context: ...c::frame_payload()to access bytes for deserialisation. - UseFrameCodec::wrap_payload()` ...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~189-~189: Consider using a different verb for a more formal wording.
Context: ...ly the changes introduced by that step, correct the issue, and reapply the edit. Use th...
(FIX_RESOLVE)
[uncategorized] ~221-~221: Possible missing comma found.
Context: ..._response_framed_with_codecfor custom codecs and keep send_response_framed` for t...
(AI_HYDRA_LEO_MISSING_COMMA)
🔍 Remote MCP
I'll search for relevant context to help review this PR effectively. Let me gather information about the FrameCodec architecture, the refactoring suggestions, and the architectural decisions.The search didn't identify the specific repository. Let me try a more targeted search based on the project structure and names visible in the PR context.Based on the PR context provided, I can see the project details but need to work with what I have. Let me examine the specific information requested about the FrameCodec trait and builder pattern more directly by analyzing the provided summaries. Since I don't have the specific GitHub repository identified, let me focus on the technical content review using the detailed context provided.
Summary of Key Findings for PR Review
Based on the comprehensive context provided, here are the critical findings relevant to reviewing this pluggable protocol codecs PR:
1. Core Architecture: FrameCodec Trait Design
The PR introduces a pluggable framing abstraction through a FrameCodec trait with the following key characteristics:
-
Associated Frame Type: Each codec implementation defines its own
Frametype (e.g.,Vec<u8>for length-delimited, custom types for Hotline/MySQL) -
Required Methods:
decoder()andencoder()for frame codec creationframe_payload()to extract payload from frameswrap_payload()to create frames from payloadsmax_frame_length()to report frame size limits- Optional
correlation_id()for extracting trace IDs from frames
-
Default Implementation:
LengthDelimitedFrameCodecprovides backward compatibility with clamped max frame length enforcement
2. Generic Propagation Through the Stack
The PR successfully parameterizes both WireframeApp and WireframeServer with a new F: FrameCodec generic parameter:
WireframeApp signature changes:
- From:
WireframeApp<S, C, E> - To:
WireframeApp<S, C, E, F>withF: FrameCodec = LengthDelimitedFrameCodecdefault
Impact on Public API:
protocolfield type updated fromArc<dyn WireframeProtocol<Frame = Vec<u8>>>toArc<dyn WireframeProtocol<Frame = F::Frame>>fragmentationconfig now derives fromcodec.max_frame_length()instead of hard-codedbuffer_capacityfield- New builder method:
with_codec<F2>()to swap codecs while preserving other config
WireframeServer signature changes:
- Expanded from
WireframeServer<F, T = (), S = Unbound>to includeSer, Ctx, E, Codecgenerics with defaults - Factory trait updated:
F: Fn() -> WireframeApp<Ser, Ctx, E, Codec> + ...
3. Code Duplication Refactoring
The PR addresses a reported code duplication issue (mentioned in comments) between serializer() and with_codec() methods in src/app/builder.rs:
- Solution implemented: A private helper method
rebuild_with_params()centralizes the reconstruction logic - Helper signature:
fn rebuild_with_params<S2, F2>( self, serializer: S2, codec: F2, protocol: Option<Arc<dyn WireframeProtocol<Frame = F2::Frame, ProtocolError = ()>>>, fragmentation: Option<FragmentationConfig> ) -> WireframeApp<S2, C, E, F2> - Benefits: Eliminates field-by-field reconstruction duplication; properly handles type parameter changes
4. Enhanced Features
- New send helper:
send_response_framed_with_codec()allows custom codecs to encode frames with proper serialization - Correlation ID propagation: Deserialization failures now include correlation IDs extracted from frame metadata when available
- Integration tests:
tests/frame_codec.rsvalidates custom codec round-tripping with aTaggedFrameexample - Example implementations: Hotline and MySQL codecs demonstrate protocol-specific framing (4-byte header for MySQL, 20-byte header for Hotline)
5. Documentation and Roadmap Updates
- ADR-004 documents the pluggable codec architectural decision
- ADR-005 covers serializer abstraction layering
- Execution Plan provides concrete implementation steps and validation commands
- Roadmap Phase 7.6 details codec hardening across fragmentation, benchmarks, and property-based testing
6. Backward Compatibility
- Length-delimited framing remains the default for all types
- Existing code using
WireframeAppcontinues to work via default generic parameters buffer_capacity()method specialized forLengthDelimitedFrameCodecpreserves the original API surface
Risk Considerations for Review
- Type Explosion: The expanded generic parameters (
Ser, Ctx, E, Codec) increase type complexity in error messages and compilation time - Trait Bounds Propagation: Five new trait bounds (
Serializer,FrameCodec,Packet) appear across multiple impl blocks—consistency should be verified - Frame Type Variance: Protocol implementations must be generic over
F::Frame; test coverage for different frame types is critical
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (63)
.markdownlint-cli2.jsonc (1)
13-21: LGTM: Improved ignore list formatting and new exclusions.The multi-line formatting enhances maintainability, and the new exclusions (Terraform state, UV cache, and
CRUSH.md) are appropriate for generated or transient content.Cargo.toml (2)
139-173: LGTM: New examples properly feature-gated.The five new example entries (including the codec examples
hotline_codecandmysql_codec) are correctly structured and feature-gated behindexamples, aligning with the pluggable codec architecture.
176-184: LGTM: Test configuration properly structured.The named
cucumbertest and newconcurrency_loomtest are correctly configured with appropriate harness and feature-gate settings.src/lib.rs (2)
15-15: LGTM: Public re-exports enhance ergonomics.The re-exports of
FrameCodecandLengthDelimitedFrameCodecprovide a clean public API surface for the pluggable codec abstraction.
9-9: Approve codec module addition.The new
codecmodule is appropriately positioned in the module hierarchy and includes proper module-level documentation explaining its purpose and utility.Makefile (2)
8-8: LGTM: Markdown linter tooling updated.The switch from
markdownlinttomarkdownlint-cli2aligns with the updated configuration file (.markdownlint-cli2.jsonc) and uses an appropriate glob pattern.Also applies to: 44-44
47-47: LGTM: Nixie sandbox flag added.The
--no-sandboxflag is appropriate for environments where sandboxing may not be available or necessary.docs/adr-005-serializer-abstraction.md (1)
1-140: LGTM: ADR structure and content are comprehensive.The ADR follows the documented style guide structure with all required sections (Status, Date, Context, Decision Drivers, Requirements, Options, Outcome, Goals, Migration Plan, Risks, Outstanding Decisions, Rationale) and includes a well-structured comparison table.
docs/adr-004-pluggable-protocol-codecs.md (1)
1-248: Excellent ADR: comprehensive documentation with implementation findings.This ADR exemplifies the style guide's ADR template with:
- All required sections present and well-structured
- A five-phase migration plan with clear goals
- Detailed implementation findings documenting codec ergonomics, error handling, and protocol handler experience
- Well-formatted comparison table and code examples
The inclusion of real-world findings (Hotline/MySQL codec experiences) adds significant value for future maintainers.
docs/documentation-style-guide.md (2)
158-197: LGTM: Comprehensive roadmap formatting guidance.The new roadmap conventions section provides clear, actionable guidance with:
- Dotted numbering notation (phases/steps/tasks)
- Checkbox tracking for completion status
- Dependency citation patterns
- A well-structured example demonstrating the hierarchy
This addition significantly improves documentation consistency.
198-309: LGTM: Thorough ADR documentation and template.The new ADR section provides:
- Clear naming conventions (
adr-NNN-short-description.md)- Required sections (Status, Date, Context) and conditional sections appropriately distinguished
- Formatting guidance including table captions and code block conventions
- A complete, reusable template
This addition enables consistent architectural documentation across the project and aligns perfectly with the new ADRs introduced in this PR.
docs/execplans/pluggable-protocol-codecs.md (1)
1-241: No action required; the identified line-length violations are not prose paragraphs. Lines 9, 65, and 193 are HTML comments with decorative formatting (═══ separators) used as section dividers, and line 178 is code within a fenced block at 83 characters (compliant with the 120-column code block limit). The prose content in the document is correctly wrapped.docs/roadmap.md (1)
381-474: Well-structured roadmap addition.Phase 7.6 follows the established roadmap format, provides clear deliverables with appropriate cross-references to design documents, and aligns with the pluggable codec architecture introduced in this PR.
examples/mysql_codec.rs (1)
1-5: Clear example demonstrating pluggable codec integration.The module documentation explains the MySQL packet format concisely, and the main function demonstrates the codec wiring pattern consistently with other examples.
Also applies to: 139-146
src/server/config/preamble.rs (2)
18-27: Generic bounds correctly propagate codec parameterisation.The expanded impl block properly constrains
Ser,Ctx,E, andCodecwith appropriate trait bounds, aligning with the pluggable codec architecture throughout the server configuration.
47-63: Type transformation preserves all fields correctly.The
with_preamblemethod properly constructs the newWireframeServervariant, resetting preamble handlers as documented whilst preserving the_appphantom marker.docs/users-guide.md (3)
104-109: Documentation clearly explains codec configuration options.The updated text accurately describes the default length-delimited codec, configuration via
buffer_capacity, codec swapping withwith_codec, and serializer customisation withwith_serializer. TheFrameCodectrait mention guides implementers appropriately.
113-118: Connection handling documentation updated with codec-aware helpers.The description of
send_response_framed_with_codecand the "length-delimited by default" framing behaviour provides clear guidance for custom codec users.
342-347: Fragmentation documentation reflects codec-derived configuration.The updated text correctly explains that
FragmentationConfignow derives from the active frame codec'smax_frame_length, maintaining consistency with the implementation.tests/frame_codec.rs (2)
117-168: Comprehensive round-trip integration test.The test exercises the full codec lifecycle: app construction, frame encoding, connection handling, response decoding, and correlation ID preservation. The assertions verify payload integrity and trailing byte absence.
1-2: Clear test module demonstrating FrameCodec implementation.The
TaggedFrameCodecimplementation covers all trait methods and serves as a reference for custom codec authors. The module documentation states the purpose succinctly.Also applies to: 97-115
src/app/frame_handling.rs (3)
51-62: ResponseContext correctly parameterised with codec marker.The
PhantomData<F>field properly carries theFrameCodectype through the context without runtime overhead, enablingsend_response_payloadto accessF::wrap_payloadfor frame construction.
184-202: Generic frame wrapping via FrameCodec trait.The renamed
send_response_payloadfunction correctly usesF::wrap_payload(payload)to construct codec-specific frames before sending, maintaining error handling and metrics instrumentation.
93-140: Response forwarding properly abstracted over codec types.The
forward_responsefunction correctly threads codec generics through the call chain, maintaining fragmentation, serialization, and error handling behaviour whilst enabling custom frame types.src/server/connection.rs (3)
21-33: Connection spawning correctly parameterised over codec and serializer.The expanded generic bounds on
spawn_connection_taskproperly constrainSer: Serializer + FrameMetadata<Frame = Envelope>, ensuring the serializer can parse frame metadata for routing whilst supporting pluggable codecs.
55-86: Stream processing correctly delegates to generic WireframeApp.The
process_streamfunction properly invokes the factory-producedWireframeApp<Ser, Ctx, E, Codec>whilst maintaining preamble handling, error logging, and stream rewinding behaviour.
183-188: Tests updated with explicit type annotations for expanded generics.The
PreambleHooks::<()>::default()syntax correctly resolves type inference where the expanded generic signature prevents automatic deduction.Also applies to: 238-243
examples/hotline_codec.rs (2)
1-6: Well-documented Hotline codec example.The module documentation clearly explains the 20-byte header format, and the
FrameCodecimplementation provides a complete reference for protocol-aware framing. The main function demonstrates the wiring pattern concisely.Also applies to: 111-134, 136-143
45-79: Decoder correctly implements Hotline framing with proper validation.The implementation validates
total_sizeconsistency, enforcesmax_frame_length, and only consumes the buffer after confirming frame completeness. Error kinds are appropriately distinguished betweenInvalidDataand insufficient data (returningOk(None)).src/server/mod.rs (2)
13-18: LGTM: Import organisation is appropriate.The imports are well-organised, grouping crate-internal types logically. The addition of
Envelope,Packet,WireframeApp,FrameCodec,LengthDelimitedFrameCodec,BincodeSerializer, andSerializercorrectly supports the expanded generic wiring.
107-156: LGTM: Generic expansion and PhantomData usage are correct.The struct signature correctly introduces
Ser,Ctx,E, andCodecgenerics with sensible defaults (BincodeSerializer,(),Envelope,LengthDelimitedFrameCodec) preserving backward compatibility. ThePhantomData<(Ser, Ctx, E, Codec)>field appropriately carries type information without runtime cost, and the trait bounds are correctly specified in the where clause.src/server/config/mod.rs (3)
17-22: LGTM: Imports correctly expanded for generic wiring.The addition of
Packet,WireframeApp,FrameCodec, andSerializerimports properly supports the genericised server configuration.
52-91: LGTM: Constructor impl correctly parameterised.The
impl<F, Ser, Ctx, E, Codec> WireframeServer<F, (), Unbound, Ser, Ctx, E, Codec>block with thenew()constructor correctly:
- Specifies trait bounds matching the struct definition.
- Returns a factory that produces
WireframeApp<Ser, Ctx, E, Codec>.- Initialises
_app: PhantomDatafor type tracking.
94-167: LGTM: Builder methods impl correctly propagates generics.The second impl block correctly extends to
WireframeServer<F, T, S, Ser, Ctx, E, Codec>with consistent trait bounds. Builder methods (workers,ready_signal,accept_backoff,worker_count) returnSelf, preserving the full generic signature through the fluent API.src/codec.rs (6)
1-5: LGTM: Module documentation is clear and appropriate.The module-level doc comment explains the purpose (pluggable framing codecs) and notes the default implementation's backward compatibility with length-prefixed format.
12-17: LGTM: Frame length constants and clamping helper are well-defined.The
MIN_FRAME_LENGTH(64 bytes) andMAX_FRAME_LENGTH(16 MiB) provide sensible guardrails. Theclamp_frame_length()function correctly uses the standard library'sclamp()method.
19-46: LGTM: FrameCodec trait design is sound.The trait correctly:
- Uses
Send + Sync + 'staticbounds for thread-safe usage.- Defines an associated
Frametype with matching bounds.- Uses RPIT (
impl Decoder/impl Encoder) for flexibility without boxing.- Provides a sensible default for
correlation_id()returningNone.- Documents methods with clear purpose descriptions.
48-77: LGTM: LengthDelimitedFrameCodec implementation is correct.The struct:
- Clamps
max_frame_lengthon construction, preventing misconfiguration.- Provides a sensible default of 1024 bytes.
- Exposes
max_frame_length()as a public accessor.
79-134: LGTM: Adapter and FrameCodec implementation are correct.The
LengthDelimitedAdaptercorrectly:
- Wraps
tokio_util::codec::LengthDelimitedCodec.- Converts
BytestoVec<u8>in the decoder.- Converts
Vec<u8>toBytesin the encoder.The
FrameCodecimplementation forLengthDelimitedFrameCodecis straightforward, withframe_payloadandwrap_payloadbeing identity operations forVec<u8>.
136-184: LGTM: Unit tests provide adequate coverage.The tests correctly:
- Verify max frame length clamping behaviour.
- Test round-trip encode/decode for payloads.
- Confirm oversized payload rejection with appropriate error kind.
- Use
.expect()as per coding guidelines for test code.src/server/runtime.rs (4)
24-30: LGTM: Imports correctly expanded for generic runtime wiring.The addition of
Envelope,Packet,WireframeApp,FrameCodec,FrameMetadata, andSerializerproperly supports the expanded generic signatures in the runtime module.
162-170: LGTM: WireframeServer Bound impl correctly parameterised.The impl block correctly:
- Introduces
Ser,Ctx,E,Codecgenerics with appropriate bounds.- Adds
FrameMetadata<Frame = Envelope>bound toSer, ensuring the serializer can parse envelope metadata.- Maintains the
Boundstate requirement forrun()andrun_with_shutdown().
379-390: LGTM: accept_loop correctly propagates generic parameters.The function signature correctly carries
Ser,Ctx,E,Codecgenerics with matching trait bounds, enabling the accept loop to work with any compliant codec and serializer combination.
423-436: LGTM: accept_iteration correctly parameterised.The internal iteration function mirrors the generic structure of
accept_loop, maintaining type safety throughout the accept cycle.src/server/config/binding.rs (5)
19-37: LGTM: WireframeFactory trait alias reduces bound repetition.The helper trait alias and its blanket impl correctly encapsulate the factory function signature with all required bounds, reducing duplication across the three impl blocks.
43-43: LGTM: BoundServer type alias improves readability.The
BoundServer<F, T, Ser, Ctx, E, Codec>alias usefully shortens the return types for binding methods whilst preserving full type information.
50-96: LGTM: bind_to_listener correctly propagates generic parameters.The internal binding method correctly:
- Destructures and carries
_appand_preamblephantom data through the binding process.- Returns
BoundServerwith all generic parameters preserved.- Handles the
set_nonblockingandfrom_stdoperations with proper error mapping.
104-178: LGTM: Unbound impl correctly parameterised.The impl block for
Unboundstate:
- Correctly returns
BoundServerfrombind()andbind_existing_listener().- Provides
local_addr()returningNonefor unbound servers.- Maintains the generic parameter chain through binding operations.
186-262: LGTM: Bound impl supports rebinding.The impl block for
Boundstate correctly:
- Returns
Selffrom rebinding methods, preserving the full type.- Provides
local_addr()that queries the listener.- Enables rebinding to new addresses whilst maintaining type safety.
src/app/connection.rs (7)
39-73: LGTM: CombinedCodec adapter is clean and correct.The
CombinedCodec<D, E>struct correctly:
- Wraps separate decoder and encoder instances.
- Implements
Decoderby delegating to the inner decoder.- Implements
Encoder<Item>by delegating to the inner encoder.- Provides
decode_eofdelegation for proper stream termination handling.
75-88: LGTM: FrameHandlingContext correctly carries codec marker.The addition of
PhantomData<F>ascodec_markercorrectly ties the context to the genericFrameCodectype without runtime overhead.
104-129: LGTM: send_response correctly uses codec for framing.The method correctly:
- Serialises the message via the serializer.
- Wraps the payload using
F::wrap_payload().- Encodes the frame using
self.codec.encoder().- Writes the encoded buffer to the stream.
131-153: LGTM: send_response_framed_with_codec provides flexible framing.The new generic method correctly:
- Accepts any encoder compatible with
F::Frame.- Wraps the serialised payload and sends via the framed stream.
- Returns appropriate
SendErroron failure.
155-190: LGTM: LengthDelimitedFrameCodec specialisation is appropriate.The specialised impl correctly:
- Provides
length_codec()for constructing aLengthDelimitedCodecwith the codec's max frame length.- Offers
send_response_framedfor the common case ofFramed<W, LengthDelimitedCodec>.- Maintains backward compatibility for existing users of length-delimited framing.
280-290: LGTM: Frame length clamping with warning is appropriate.The code correctly:
- Clamps the requested frame length using
clamp_frame_length().- Logs a warning when the requested length exceeds
MAX_FRAME_LENGTH.- Reserves buffer capacity based on the clamped value.
395-418: LGTM: decode_envelope correctly extracts correlation ID from frame.The method correctly:
- Extracts payload using
F::frame_payload(frame).- Assigns
correlation_idfromF::correlation_id(frame)when the envelope lacks one.- Passes correlation ID to
handle_decode_failurefor improved error logging.src/app/builder.rs (7)
37-45: LGTM: default_fragmentation helper correctly derives config from frame budget.The function correctly:
- Clamps the frame budget using
clamp_frame_length().- Computes max message size as a multiple of the frame budget.
- Falls back to frame_budget itself if the multiplication overflows or produces zero.
- Delegates to
FragmentationConfig::for_frame_budget()for the actual config construction.
80-98: LGTM: WireframeApp struct correctly parameterised over FrameCodec.The struct correctly:
- Introduces
F: FrameCodec = LengthDelimitedFrameCodecwith a sensible default.- Replaces
buffer_capacitywithcodec: Ffor flexible framing.- Updates
protocoltype to useF::Framefor consistency.
116-142: LGTM: Default impl correctly initialises codec and fragmentation.The implementation correctly:
- Creates the codec via
F::default().- Extracts
max_frame_lengthfrom the codec.- Derives fragmentation configuration using
default_fragmentation().
188-221: LGTM: rebuild_with_params helper eliminates duplication.The helper correctly:
- Centralises field-by-field reconstruction for type parameter changes.
- Resets
routestoOnceCell::new()since route chains may depend on serializer/codec types.- Uses
#[expect(clippy::too_many_arguments, ...)]with an appropriate reason.- Moves owned fields from
selfrather than cloning unnecessarily.This addresses the code duplication issue raised in the PR comments.
223-236: LGTM: with_codec correctly swaps codec and resets dependent state.The method correctly:
- Requires
S: Defaultto enablestd::mem::take()on the serializer.- Computes new fragmentation from the codec's max frame length.
- Resets
protocoltoNonesince the frame type may change.- Delegates to
rebuild_with_params()for reconstruction.
404-413: LGTM: serializer method correctly uses rebuild helper.The method correctly:
- Requires
F: Defaultto enablestd::mem::take()on the codec.- Preserves
protocolandfragmentationwhen only changing serializer.- Delegates to
rebuild_with_params()for reconstruction.
433-448: LGTM: buffer_capacity specialisation preserves backward compatibility.The specialised impl for
LengthDelimitedFrameCodeccorrectly:
- Clamps the capacity using
clamp_frame_length().- Creates a new codec with the clamped capacity.
- Updates fragmentation to match the new frame budget.
This preserves the original
buffer_capacity()API for users of the default codec.
Extract combined codec, lifecycle, and middleware type helpers into separate app submodules, reducing builder and connection sizes. Move BackoffConfig and runtime tests into runtime submodules to keep server runtime within the file size guideline.
Clarify that correlation currently lives in the serialised Envelope payload, and outline where to merge header correlation on inbound frames plus the codec API change needed for outbound headers.
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)
src/app/builder.rs (1)
1-402: File exceeds 400-line limit.This file contains 402 lines, exceeding the 400-line maximum specified in the coding guidelines. Decompose the module or reduce code volume.
Removing the
rebuild_with_paramshelper (as suggested in the past review) would save approximately 26 lines and bring the file comfortably under the limit.
♻️ Duplicate comments (4)
src/server/runtime.rs (1)
322-333: Generic complexity already flagged.The verbose
<F, T, L, Ser, Ctx, E, Codec>signature was addressed in a prior review suggesting aRuntimeApptrait to reduce repetition.src/app/connection.rs (2)
41-53: PhantomData complexity already flagged.A prior review suggested collapsing the generic
Cparameter and removingPhantomData<F>by fixing to aConnectionCodec<F>type alias.
341-358: Missing regression test already flagged.A prior review requested a test for
handle_decode_failurewith the newcorrelation_idparameter and threshold behaviour.src/app/builder.rs (1)
146-175: Duplicate concern: removerebuild_with_paramsto eliminate lint suppression and artificial Default bounds.The past review correctly identifies that this helper introduces unnecessary complexity:
- The
clippy::too_many_argumentssuppression can be avoided entirely by inlining the reconstruction.- The
S: Defaultbound inwith_codec(line 185) andF: Defaultbound inserializer(line 361) exist only to supportstd::mem::take, which is unnecessary with direct destructuring.- Removing the helper also reduces the file length (currently 402 lines, exceeding the 400-line limit).
Follow the detailed refactoring approach provided in the past review to inline the reconstructions in
with_codecandserializer, eliminating this helper entirely.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
src/app/builder.rssrc/app/combined_codec.rssrc/app/connection.rssrc/app/lifecycle.rssrc/app/middleware_types.rssrc/app/mod.rssrc/server/runtime.rssrc/server/runtime/backoff.rssrc/server/runtime/tests.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
src/app/lifecycle.rssrc/server/runtime/backoff.rssrc/app/combined_codec.rssrc/server/runtime/tests.rssrc/app/middleware_types.rssrc/app/mod.rssrc/app/connection.rssrc/server/runtime.rssrc/app/builder.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/app/lifecycle.rssrc/server/runtime/backoff.rssrc/app/combined_codec.rssrc/server/runtime/tests.rssrc/app/middleware_types.rssrc/app/mod.rssrc/app/connection.rssrc/server/runtime.rssrc/app/builder.rs
🧬 Code graph analysis (4)
src/server/runtime/tests.rs (3)
src/server/runtime.rs (1)
accept_loop(322-360)src/server/test_util.rs (2)
bind_server(75-82)free_listener(27-30)src/server/runtime/backoff.rs (1)
default(26-31)
src/app/connection.rs (3)
src/codec.rs (13)
clamp_frame_length(15-17)wrap_payload(37-37)wrap_payload(131-131)encoder(31-31)encoder(125-127)new(60-64)new(84-90)max_frame_length(45-45)max_frame_length(68-68)max_frame_length(133-133)correlation_id(42-42)frame_payload(34-34)frame_payload(129-129)tests/frame_codec.rs (7)
wrap_payload(110-110)encoder(104-106)new(27-27)new(36-36)max_frame_length(114-114)correlation_id(112-112)frame_payload(108-108)src/app/combined_codec.rs (1)
new(12-12)
src/server/runtime.rs (5)
src/app/builder.rs (1)
serializer(358-367)src/app/connection.rs (1)
serializer(173-173)tests/middleware_order.rs (1)
serializer(94-94)tests/fragment_transport.rs (1)
serializer(135-135)src/server/config/tests.rs (2)
WireframeServer(73-73)WireframeServer(204-206)
src/app/builder.rs (1)
src/codec.rs (7)
clamp_frame_length(15-17)new(60-64)new(84-90)default(72-76)max_frame_length(45-45)max_frame_length(68-68)max_frame_length(133-133)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (13)
src/app/combined_codec.rs (1)
1-40: LGTM!Clean delegation adapter that composes a decoder and encoder for
Framedstreams. The implementation correctly forwards all trait methods without introducing additional state or logic.src/app/middleware_types.rs (1)
1-22: LGTM!Well-structured type aliases and trait definitions for middleware composition. The blanket implementation correctly bridges
Transformto theMiddlewaretrait.src/app/mod.rs (1)
11-26: LGTM!Good modular reorganisation. The public re-exports provide a clean API surface with lifecycle hooks and middleware types now in dedicated modules.
src/app/lifecycle.rs (1)
1-33: LGTM!Clear lifecycle hook definitions with appropriate async bounds. The doc examples demonstrate correct
Arcwrapping for the trait objects.src/server/runtime/backoff.rs (1)
1-65: LGTM!Well-designed configuration type with sensible defaults and robust normalisation. The
normalised()method correctly handles misconfiguration by clamping and swapping values as needed.src/server/runtime/tests.rs (2)
174-228: Backoff test coverage is good.The test correctly validates exponential backoff behaviour with time control. The sequence
[5ms, 10ms, 20ms]demonstrates doubling with the cap atmax_delay.
34-92: Server lifecycle tests are well-structured.Good use of
rstestfixtures andoneshotchannels for shutdown signalling. The timeout guards prevent tests from hanging indefinitely.src/server/runtime.rs (2)
105-113: LGTM on the generic bounds forWireframeServer.The expanded generic surface correctly threads serializer, context, envelope, and codec types through the server runtime.
3-6: Good modular extraction.Splitting
backoffandtestsinto submodules improves maintainability and keeps file sizes manageable.src/app/connection.rs (4)
245-255: Good guardrail with informative warning.The clamping logic and warning when
max_frame_lengthexceedsMAX_FRAME_LENGTHprovides clear feedback without silently truncating.
101-117: Clean generic framed response helper.
send_response_framed_with_codeccorrectly abstracts over arbitrary encoders compatible with the codec's frame type.
120-155: Ergonomic specialisation forLengthDelimitedFrameCodec.The
length_codec()andsend_response_framedmethods provide a convenient API for the default codec without requiring users to thread generic parameters.
360-383: Frame-aware envelope decoding is correct.The use of
F::frame_payloadandF::correlation_idproperly abstracts payload extraction across different codec implementations. Backfillingcorrelation_idwhen absent is a sensible default.
Introduce a RESP framing example that parses simple strings, integers, bulk strings, and arrays, and wire it behind the feature flag.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
Cargo.tomlexamples/resp_codec.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
examples/resp_codec.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
examples/resp_codec.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3"). Wildcard (*) or open-ended inequality (>=) version requirements are strictly forbidden. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🔍 Remote MCP Deepwiki
Additional context relevant to reviewing PR #413 (pluggable protocol codecs)
- Repo docs and generated pages confirm a new pluggable framing abstraction is consistent with project design: a FrameCodec trait + default LengthDelimitedFrameCodec are now first-class and exposed in the crate API (matches PR objective) — see "Overview" / Protocol Processing and "FrameProcessor" sections.
- WireframeApp/WireframeServer are documented as builder/factory types and the project already uses generics for serializers/envelopes; the docs describe the existing builder patterns and factory usage, so making the app/server generic over a codec follows established patterns and will affect many impl signatures (expect widespread signature changes across app/server modules).
- Examples + tests: repository contains examples and tests for custom codecs (echo/ping examples, testing utilities). The test infra (wireframe_testing) supports in-memory framed tests; adding codec integration tests (TaggedFrame, Hotline, MySQL, RESP) aligns with existing testing patterns and utilities.
- Fragmentation / multi-packet streaming and correlation semantics are a cross-cutting concern in docs (roadmap, multi-packet streaming design); the PR’s alignment of fragmentation config to codec max_frame_length is consistent with documented roadmap and multi-packet design notes. Review should confirm fragmentation updates propagate correctly to existing fragmentation/reassembly subsystems.
- Public API & compatibility points to check (per documentation and builder semantics):
- WireframeApp generic parameterization (new F: FrameCodec) changes many public method signatures (with_codec, serializer, protocol(), protocol_hooks(), buffer_capacity specialization) — ensure backward-compatible defaults compile (LengthDelimitedFrameCodec default).
- WireframeServer generics threaded through factory types and binding/runtime code (accept loop, spawn_connection_task) — verify factory closures and typestate transitions still ergonomic and documented defaults preserved.
- New public exports: codec module and re-exports (FrameCodec, LengthDelimitedFrameCodec) — check crate docs and public API surface.
- Testing & CI implications:
- Repo has extensive test harnesses (rstest, cucumber, wireframe_testing). PR adds unit/integration tests for codecs and updates Cargo.toml with new test entries/features; verify CI will run these new tests and that feature flags (advanced-tests, cucumber-tests) are wired correctly.
- Suggested focused review areas (based on repo docs + PR summary):
- Consistency of trait bounds and defaults across impls (WireframeApp, WireframeServer, ConnectionActor, runtime/accept loop).
- Correct preservation of existing behavior when using default LengthDelimitedFrameCodec (buffer_capacity/clamping, fragmentation defaulting).
- Correct propagation of correlation_id and decode/error logging changes when frames are protocol-specific (docs emphasize correlation provenance).
- Integration of new CombinedCodec and CombinedEncoder/Decoder usage with tokio_util framed streams and existing push/response paths.
- Build/test changes: new examples and tests are compiled/run in CI; Cargo.toml/test entries and Makefile change (markdownlint) included — confirm CI runs them and feature gates behave as intended.
Sources:
- Project documentation and API pages retrieved from the repository wiki/docs via Deepwiki_read_wiki_contents.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test
🔇 Additional comments (8)
Cargo.toml (2)
139-177: LGTM!The new example entries follow the established pattern with correct paths and feature gating via
required-features = ["examples"]. The codec examples (hotline, mysql, resp) align with the PR's pluggable codec objectives.
181-189: LGTM!The test entries are correctly configured. The cucumber test appropriately disables the standard harness and requires both feature flags. The loom concurrency test is properly gated behind
advanced-tests.examples/resp_codec.rs (6)
1-21: LGTM!Clear module documentation explaining the RESP subset supported, and imports are appropriately minimal.
67-81: LGTM!The encoder correctly validates frame size before reserving capacity and encoding.
83-104: LGTM!The
FrameCodecimplementation correctly bridges RESP frames to the wireframe abstraction, withframe_payloadextracting bulk string contents andwrap_payloadconstructing the appropriate frame type.
322-354: LGTM!Encoding logic is correct. The
to_string()allocations mirrorencoded_len; address there if optimizing.
356-359: LGTM!Clean overflow-checked arithmetic helper.
361-368: LGTM!Minimal but effective demonstration of wiring a custom codec into
WireframeApp. The no-op handler and unused binding are appropriate for an illustrative example.
- expose codec adapter types needed by public associated types - bound RESP parsing and allocation plus zero-copy length math - align protocol tests with a Vec-backed codec and hooks - split builder defaults and connection tests into modules - refresh docs and examples for feature gating and spelling
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
examples/resp_codec_impl/encode.rsexamples/resp_codec_impl/frame.rsexamples/resp_codec_impl/parse.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
examples/resp_codec_impl/parse.rsexamples/resp_codec_impl/encode.rsexamples/resp_codec_impl/frame.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
examples/resp_codec_impl/parse.rsexamples/resp_codec_impl/encode.rsexamples/resp_codec_impl/frame.rs
🧬 Code graph analysis (1)
examples/resp_codec_impl/parse.rs (3)
src/frame/format.rs (1)
bytes(54-54)examples/resp_codec_impl/codec.rs (3)
max_frame_length(90-90)new(24-24)new(34-34)src/app/frame_handling.rs (4)
max_frame_length(308-308)new(31-31)new(228-228)new(237-237)
🔍 Remote MCP Deepwiki
Summary of additional facts relevant to reviewing PR #413 (pluggable-protocol-codecs)
-
New codec abstraction
- Adds FrameCodec trait and default LengthDelimitedFrameCodec (public, re-exported) with associated Frame, Decoder, Encoder, payload accessors, wrap_payload, correlation_id, max_frame_length.
-
WireframeApp / connection changes (public surface)
- WireframeApp is now generic over F: FrameCodec (WireframeApp<S,C,E,F>); buffer_capacity replaced by codec: F; many public method signatures updated (with_codec, serializer, with_protocol, protocol()/protocol_hooks()). Specialized impl retains buffer_capacity(capacity) for LengthDelimitedFrameCodec.
- Connection handling now uses F::Frame throughout: parsing uses F::frame_payload(frame) and F::correlation_id(frame); decode path and error logging pass correlation_id. New public methods: send_response_framed_with_codec and specialized send_response_framed for length-delimited.
-
Server / factory generics
- WireframeFactory and WireframeServer signatures expanded: factories must return WireframeApp<Ser, Ctx, E, Codec>; WireframeServer gains Ser, Ctx, E, Codec generics (defaults provided). Many bind/run APIs adjusted accordingly.
-
Framing plumbing
- CombinedCodec wrapper (ConnectionCodec alias) pairs per-connection Decoder/Encoder so Framed uses ConnectionCodec. Framing uses ConnectionCodec and reserves/clamps frame lengths; logs guardrail warnings when exceeded. Fragmentation defaults derive from codec.max_frame_length via default_fragmentation helper.
-
Tests, examples, and examples-codecs
- Added example codecs and examples: Hotline, Mysql, RESP (full example with parse/encode/frame), Tagged test codec; examples under examples/*.rs and src/codec/examples.rs. Integration/unit tests added (tests/frame_codec.rs, tests/example_codecs.rs, tests/wireframe_protocol.rs, connection & framing tests) exercising custom FrameCodec behavior and end-to-end round trips.
-
Resp encoder dependency
- Cargo.toml adds dependency itoa = "1.0" (used by RESP encoder).
-
BackoffConfig relocation
- BackoffConfig moved to src/server/runtime/backoff.rs, re-exported publicly; defaults initial_delay=10ms, max_delay=1s, with normalised() helper. Accept-loop/backoff behavior updated to use it.
-
Public/type-safety/complexity notes to verify in review
- Many public signatures changed (WireframeApp, WireframeServer, ConnectionApp trait, ResponseContext types). Confirm intended semver/API compatibility.
- Large expansion of generics across server/runtime/connection — reviewer comments in PR suggest potentially simplifying by using a single App trait/type; rebuild_with_params helper remains as an architectural choice.
Sources
- Repository documentation and code overview: Deepwiki read of leynos/wireframe contents
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test
🔇 Additional comments (15)
examples/resp_codec_impl/frame.rs (1)
1-17: LGTM!Clean, well-documented enum definition. The nullable semantics using
OptionforBulkStringandArraycorrectly model RESP null values ($-1and*-1).examples/resp_codec_impl/encode.rs (5)
15-47: LGTM!Correct length calculation with overflow protection via
checked_addand recursion depth limiting. The hardcoded length of 5 for null variants ($-1\r\n,*-1\r\n) is accurate.
49-98: LGTM!Encoding logic correctly implements the RESP wire format. Good use of
itoa::Bufferfor efficient integer formatting without heap allocation.
109-117: LGTM!The digit-counting logic correctly handles edge cases including
i64::MINand negative values. The symmetric loop conditioncursor <= -10 || cursor >= 10elegantly handles both polarities.
100-107: LGTM!Simple and correct digit-counting implementation.
119-122: LGTM!Concise overflow-safe addition helper with appropriate error handling.
examples/resp_codec_impl/parse.rs (9)
51-57: LGTM!Clean public entry point for incremental parsing.
59-72: LGTM!Correct offset handling and UTF-8 validation.
74-86: LGTM!Straightforward simple string parsing.
88-101: LGTM!Correct integer parsing with proper error handling for invalid values.
103-163: LGTM!Robust bulk string length parsing with correct null handling (
$-1), rejection of invalid negative lengths, and overflow protection.
166-212: LGTM!Payload parsing correctly validates frame length bounds before allocation and explicitly verifies CRLF termination.
214-270: LGTM!Correct array parsing with null array support (
*-1), element count limiting against allocation DoS, and proper recursion depth tracking. The final frame length check (line 263) ensures cumulative array size respects bounds.
272-297: LGTM!Correct recursion depth limiting and prefix dispatch. The missing Error (
-) prefix is flagged separately as a documentation issue.
299-325: LGTM!Correct incremental line parsing with length limiting and proper handling of incomplete CRLF sequences.
- Add Error(String) variant to RespFrame enum for `-` prefixed frames - Add parse_error function and wire `-` branch into parse_frame_at - Update encoder to handle Error variant (combined length calc with SimpleString, separate encoding for different prefix byte) - Documentation table now matches implemented parser behaviour 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: examples/resp_codec_impl/parse.rs Comment on lines +74 to +86 fn parse_simple_string(
buf: &BytesMut,
start: usize,
max_frame_length: usize,
) -> Result<Option<(RespFrame, usize)>, io::Error> {
let Some((text, next)) =
parse_text_line(buf, start, max_frame_length, "invalid simple string")?
else {
return Ok(None);
};
let frame = RespFrame::SimpleString(text.to_string());
Ok(Some((frame, next - start)))
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
examples/resp_codec_impl/encode.rsexamples/resp_codec_impl/frame.rsexamples/resp_codec_impl/parse.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
examples/resp_codec_impl/encode.rsexamples/resp_codec_impl/parse.rsexamples/resp_codec_impl/frame.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
examples/resp_codec_impl/encode.rsexamples/resp_codec_impl/parse.rsexamples/resp_codec_impl/frame.rs
🔍 Remote MCP Deepwiki
Summary of additional, review-relevant facts (from repository docs/code index) — key points to check while reviewing PR #413:
-
New public FrameCodec abstraction and default implementation
- Adds trait FrameCodec (associated Frame, Decoder, Encoder) and LengthDelimitedFrameCodec (default, constructor, max_frame_length) exported from crate root.
-
WireframeApp / connection API become codec-generic
- WireframeApp gains a new generic F: FrameCodec (WireframeApp<S,C,E,F>); buffer_capacity replaced by codec: F; many builder and accessor signatures changed (with_codec, serializer, with_protocol, protocol()/protocol_hooks()). Connection handling now uses F::Frame and F::frame_payload / F::correlation_id.
-
WireframeServer and factory wiring become codec-generic
- WireframeFactory trait and WireframeServer types expanded to include Ser, Ctx, E, Codec generics; factories now must return WireframeApp<Ser, Ctx, E, Codec>. This substantially expands public/type-level surface area.
-
Framing plumbing / per-connection codec adapter
- Added CombinedCodec / ConnectionCodec wrapper used with tokio_util::codec::Framed to pair per-connection decoder+encoder. Framed streams now use ConnectionCodec.
-
New send helpers and frame-aware paths
- New public methods: send_response_framed_with_codec (generic) and a specialized send_response_framed for LengthDelimitedFrameCodec; send path now uses F::wrap_payload(payload). decode path uses F::frame_payload(frame).
-
Examples, test coverage and example codecs added
- New example codecs and examples: Hotline, Mysql, RESP (full parse/encode), Tagged test codec; integration and unit tests added (tests/frame_codec.rs, tests/example_codecs.rs, tests/wireframe_protocol.rs, connection & framing tests) exercising custom FrameCodec round-trips, correlation-id propagation, decode-failure logging, and send_response_payload semantics.
-
Resp encoder dependency added
- Cargo.toml now adds itoa = "1.0" (used by RESP encoder).
-
BackoffConfig extracted and public
- BackoffConfig moved to src/server/runtime/backoff.rs, public with defaults (initial_delay = 10ms, max_delay = 1s) and normalised() helper; accept-loop uses it.
-
Fragmentation/defaults now derive from codec
- Default fragmentation computation uses codec.max_frame_length() (via default_fragmentation helper); specialized buffer_capacity exists only for LengthDelimitedFrameCodec.
-
Public API surface & semver risk
- Many public types/signatures changed (WireframeApp, WireframeServer, ConnectionApp trait, ResponseContext types, new public methods). These are likely breaking changes for consumers; verify intended semantic/versioning impact.
-
Notable reviewer-discussed design choices still present
- rebuild_with_params helper (used when swapping serializer/codec) remains (author added it after duplication feedback); WireframeFactory retains verbose bounds — both noted as outstanding architectural choices in PR comments.
Sources: Deepwiki repository index and documentation/code summaries extracted from leynos/wireframe.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (5)
examples/resp_codec_impl/frame.rs (1)
1-19: LGTM!Clean data model for RESP frames. The enum variants correctly represent all RESP2 types with appropriate nullable representations for bulk strings and arrays.
examples/resp_codec_impl/encode.rs (1)
1-129: LGTM!Well-structured encoder with proper recursion depth limits and overflow protection via
checked_add. The separation between public API (encoded_len,encode_frame) and internal depth-tracking implementations is clean.examples/resp_codec_impl/parse.rs (3)
1-31: LGTM!Comprehensive module documentation that clearly explains the RESP protocol, parser behaviour, supported types, and integration context. The DoS protections (
MAX_ARRAY_ELEMENTS,MAX_RECURSION_DEPTH) are well-documented with security rationale.
226-282: LGTM!Null array handling (
*-1\r\n) is correctly implemented per the RESP specification. The element count limit and cumulative byte validation provide robust DoS protection.
284-310: LGTM!All five RESP frame types are now handled, including the error prefix (
-). Recursion depth enforcement is correctly placed before any parsing work.
Introduce generic parse_text_frame helper that parameterises the frame constructor, eliminating duplicate parse-and-wrap logic between parse_simple_string and parse_error functions. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: examples/resp_codec_impl/parse.rs Comment on lines +96 to +108 fn parse_simple_string(
buf: &BytesMut,
start: usize,
max_frame_length: usize,
) -> Result<Option<(RespFrame, usize)>, io::Error> {
parse_text_frame(
buf,
start,
max_frame_length,
"invalid simple string",
RespFrame::SimpleString,
)
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: examples/resp_codec_impl/parse.rs Comment on lines +79 to +94 fn parse_text_frame<F>(
buf: &BytesMut,
start: usize,
max_frame_length: usize,
error_msg: &str,
constructor: F,
) -> Result<Option<(RespFrame, usize)>, io::Error>
where
F: FnOnce(String) -> RespFrame,
{
let Some((text, next)) = parse_text_line(buf, start, max_frame_length, error_msg)? else {
return Ok(None);
};
let frame = constructor(text.to_string());
Ok(Some((frame, next - start)))
}❌ New issue: Excess Number of Function Arguments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11523 @leynos: The duplication flagged by CodeScene is structural rather than logical and is acceptable in this case. The shared parsing logic has already been extracted into Why This Duplication Is Acceptable
RecommendationAccept this duplication. The pattern of thin wrapper functions delegating to a shared helper with different parameters is a valid design choice. Static analysis tools often flag structural similarity even when further refactoring would harm readability. If you want to silence the CodeScene warning specifically, you could:
However, I would not recommend further refactoring here — the code is already well-factored for its purpose. 🐰 |
This comment was marked as resolved.
This comment was marked as resolved.
Add Copy-able ParseContext struct that groups buf, start, and max_frame_length parameters. Update parse_text_line, parse_text_frame, and parse_integer to use the new context struct, removing the need for the clippy::too_many_arguments suppression. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Update parse_bulk_payload to calculate total frame size including
header ('$' + digits + CRLF) and trailing CRLF, rather than just
comparing payload length against max_frame_length.
Add decimal_digits helper function for computing digit count.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/resp_codec_impl/parse.rs (1)
100-139: Duplication adequately addressed byparse_text_framehelper.The CodeScene warning about duplication between
parse_simple_stringandparse_erroris a false positive in this context. Both functions are minimal wrappers (7 lines each) delegating to the sharedparse_text_framehelper, which centralises the parsing logic. This is the idiomatic pattern for type-specific constructors with shared behaviour.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
examples/resp_codec_impl/parse.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
examples/resp_codec_impl/parse.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
examples/resp_codec_impl/parse.rs
🔍 Remote MCP Deepwiki
Summary — additional review-relevant facts found
-
Repository docs and API consistently describe Wireframe as actor-per-connection, length-delimited default framing, pluggable serializer, push queues, and session registry — the PR’s FrameCodec + codec-generic app/server changes align with existing extension points (WireframeProtocol, PushHandle, SessionRegistry).
-
WireframeServer/ WireframeApp builders and factory pattern are core to runtime; server factories are expected to produce per-worker WireframeApp instances — changing the factory bound to return WireframeApp<Ser, Ctx, E, Codec> (as this PR does) materially expands the type-level surface the server APIs require callers to satisfy. This is a likely semver-breaking surface to highlight.
-
The docs emphasise fragmentation, max-message-size, and fragmentation-derived defaults; the PR’s change to derive fragmentation and buffer sizes from codec.max_frame_length follows that documented intent (so reviewers should verify clamp/guardrail behavior and default_fragmentation correctness).
-
The project has extensive tests and testing utilities (wireframe_testing) relying on in-memory duplex streams and deterministic time advancement. The PR added codec tests and integration tests (tagged examples and tests/frame_codec.rs); reviewers should ensure new codec-specific tests use the same test helpers and that CI test matrix (features like "examples", "advanced-tests") remains compatible.
-
Observability/metrics and panic-isolation are documented expectations (connection panics must be isolated); the PR threads correlation_id into decode-failure logs and decode paths — reviewers should verify log messages and panic-isolation behavior remain intact.
Citations
- Repository docs/index and API pages: Deepwiki read of leynos/wireframe wiki contents.
🔇 Additional comments (6)
examples/resp_codec_impl/parse.rs (6)
1-31: Documentation is comprehensive and well-structured.The module doc clearly explains RESP, supported types, null handling semantics, and the parser's role within the example codec. Good work addressing the previous review feedback.
38-59: DoS protections and digit counting are correct.The constants provide reasonable limits against allocation-based and stack-exhaustion attacks. The
decimal_digitshelper correctly handles zero and counts digits efficiently.
61-77: Good use of parameter grouping.
ParseContextreduces function argument counts and improves readability, aligning with the coding guidelines for grouping related parameters.
219-251: Bulk payload parsing correctly validates total frame size.The calculation at lines 224-227 properly accounts for header overhead (
$+ digits +\r\n) plus payload plus trailing\r\n. The overflow protection viachecked_addis appropriate. Previous review feedback has been addressed.
271-327: Array parsing is robust with proper safeguards.Null arrays are correctly handled (line 286-288), element count is bounded (line 300-305), recursion depth is tracked, and total consumed bytes are validated against
max_frame_length(line 320-325). Good defensive coding.
329-355: Dispatch logic is correct and complete.The recursion depth check precedes any parsing work, and all documented RESP prefixes are handled with appropriate fallback for unknown prefixes.
Change comparison from >= to > so lines with exactly max_len bytes are accepted. The error should only trigger when length strictly exceeds the limit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| parse.rs | 1 advisory rule | 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
examples/resp_codec_impl/parse.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
examples/resp_codec_impl/parse.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
examples/resp_codec_impl/parse.rs
🧬 Code graph analysis (1)
examples/resp_codec_impl/parse.rs (5)
src/frame/format.rs (1)
bytes(54-54)src/request.rs (1)
value(192-197)examples/resp_codec_impl/codec.rs (3)
new(24-24)new(34-34)max_frame_length(90-90)src/app/frame_handling.rs (4)
new(31-31)new(228-228)new(237-237)max_frame_length(308-308)src/fragment/index.rs (1)
try_from(48-48)
🔍 Remote MCP Deepwiki
Summary of additional review-relevant facts (concise)
-
Public API / semver impact
- WireframeApp and WireframeServer builder types are documented as core public APIs; changing WireframeApp/WireframeServer generics (adding codec type parameters and changing factory bounds to produce WireframeApp<Ser, Ctx, E, Codec>) affects the public factory/constructor types and is semver-sensitive.
-
Defaults and backwards-compatibility expectations
- The project documents a length-delimited framing default and BincodeSerializer default; PR preserves a length-delimited default via LengthDelimitedFrameCodec but threads codec through many builder signatures — reviewers should confirm defaults remain ergonomic and that the LengthDelimitedFrameCodec::default matches prior buffer_capacity behaviour.
-
Fragmentation / max-frame-size behavior
- Documentation and code rely on fragmentation and buffer sizing derived from codec max_frame_length; ensure clamp/guardrail constants and default_fragmentation computation preserve existing DoS protections and default fragmentation (previous buffer_capacity→now codec.max_frame_length).
-
Connection/server runtime changes
- Accept loop and spawn_connection_task are documented to use factory->WireframeApp instances per worker. PR changes make accept_loop/process_stream generic over an App/ConnectionApp; verify worker-factory expectations and that panic-isolation and BackoffConfig behaviour remain unchanged (BackoffConfig defaults: initial 10ms, max 1s; normalised() clamps to ≥1ms).
-
Testing and CI implications
- The repository uses extensive test helpers (wireframe_testing) and feature-gated advanced tests (cucumber, loom). PR adds many codec-specific tests and new example/features — reviewers should check test feature flags (examples, advanced-tests, cucumber-tests) and CI matrix to ensure new tests run under CI and harness changes (new [[test]] entries) are compatible.
-
Observability / error logging expectations
- Docs require correlation_id propagation and structured logging; PR threads correlation_id into decode-failure logs and decode paths — confirm log messages and metrics (e.g., connection panics metric) follow existing conventions.
Sources:
- Deepwiki repository docs and API pages used for above facts.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (8)
examples/resp_codec_impl/parse.rs (8)
1-31: LGTM: Documentation is comprehensive.The module-level documentation clearly explains the RESP protocol, supported types, incremental parsing behaviour, and the module's role within the example codec.
38-59: LGTM: DoS guards and helper are sound.The constants provide reasonable bounds for allocation and recursion protection. The
decimal_digitshelper is clear and avoids floating-point arithmetic.
100-139: Static analysis duplication concern resolved.The
parse_text_framehelper (lines 100-115) extracts the shared logic, andparse_simple_string/parse_errorare now thin wrappers differing only in the error message and frame constructor. This addresses the CodeScene finding whilst maintaining readable, discoverable APIs.
141-154: LGTM: Integer parsing is correct.The implementation follows the established pattern and handles parsing errors appropriately.
219-251: LGTM: Bulk payload length check now accounts for total frame size.The calculation at lines 223-227 correctly includes the header (
$+ digits +\r\n) and trailing terminator (\r\n) when validating againstmax_frame_length. The checked arithmetic guards against overflow.
271-327: LGTM: Array parsing with null support and DoS guards.The null array handling (
*-1\r\n) at lines 285-288 aligns with the RESP specification. The element count limit, recursion depth control, and cumulative byte validation provide layered protection against malicious input.
329-355: LGTM: Frame dispatch handles all documented types.The recursion depth check fails fast, and all five documented RESP prefixes (
+,-,:,$,*) are now handled—including the error prefix that was previously flagged as missing.
357-383: LGTM: Off-by-one fix applied.The comparison at line 364 now uses
>instead of>=, allowing lines with exactlymax_lenbytes of content whilst still rejecting oversized input.
1 participant
Summary by Sourcery
Introduce a pluggable
FrameCodecabstraction and makeWireframeApp/server runtime generic over protocol-specific codecs while preserving a length-delimited default.New Features:
FrameCodectrait with a defaultLengthDelimitedFrameCodecimplementation and expose it from the crate API.WireframeAppandWireframeServerto be configured with custom frame codecs and provide new helpers for sending responses over arbitrary framed streams.Enhancements:
Build:
markdownlint-cli2and simplify markdown and Mermaid validation commands.Documentation:
Tests:
FrameCodecround-trips and behaviour.