Refactor unwrap and expect propagation; pluggable server plumbing

[leynos]

Summary

• Refactor unwrap/expect usage to propagate errors along core paths, server plumbing, tests, and examples. Introduce pluggable server plumbing (ConnectionChannels, AcceptLoopOptions) and align preamble/backoff logic with explicit error propagation. Harden push paths to propagate errors (DLQ handling) and switch logging to structured forms. Update tooling, linting, tests, fixtures, and examples to favour error paths and map_err/? usage.

Changes

Core error propagation

• Introduce DeserFailureTracker and ResponseContext to centralize error tracking and response handling during deserialization and fragmentation.
• Refactor reassemble_and_forward paths to propagate errors and gracefully handle fragmentation/serialization failures.
• Replace direct panics in core paths with explicit error propagation and backoff-aware handling.

Server & connection plumbing

• Add ConnectionChannels wrapper to bundle push queues and handles, simplifying actor construction and error propagation.
• Introduce AcceptLoopOptions and related context for cleaner accept-loop logic and error handling.
• Align preamble handling with explicit error propagation; refine trait bounds for clarity.
• Remove src/main.rs as part of consolidating examples and server wiring; tests/examples rely on explicit error paths instead.

Push queues & logging

• Harden push paths to propagate errors from push operations rather than panicking, including DLQ handling with robust logging and rate-limited warnings.
• Replace println! with structured logging (info!/warn!/error!) across relevant areas.

Tests and fixtures

• Update tests to return Result-based TestResult and propagate errors with ? where appropriate.
• Replace unwrap/expect in tests with proper error handling to improve diagnostics.

Examples and demos

• Refactor examples to propagate errors (using map_err or ?) and avoid unwraps in example flows.

Tooling and linting

• Add clippy.toml configuration to discourage unwrap/expect in tests and production code; tune rules for error handling.
• Update Makefile to run docs with RUSTDOC_FLAGS and keep formatting/lint gates intact.

Public API and internal plumbing

• Introduce ConnectionChannels wrapper (queues + handle) to simplify actor construction and error propagation.
• Add AcceptLoopOptions and related context for cleaner server accept-loop logic; migrate accept-loop usage to new options.

Why this helps

• Improves reliability by avoiding panics in production code paths and tests.
• Enables richer diagnostics through centralized error tracking and structured logging.
• Improves reasoning about error handling across asynchronous components (fragments, reassembly, framing, and I/O).

Testing plan

• Build and test locally: cargo test --workspace; make check-fmt; make lint.
• Validate examples compile and run with proper error handling paths.
• Ensure DLQ and fragmentation tests cover new error propagation paths and logging behavior.

Notes for reviewers

• Focus areas: correctness of error propagation paths; ensuring no silent panics in hot paths (connection handling, accept loops, fragmentation, and framing).
• Be mindful of any public API changes that might require migration notes.

📎 Task: https://www.terragonlabs.com/task/6cf1a568-3598-4ee8-b3db-a0289f31377e

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

tests/fragment_transport.rs (1)

1-513: File exceeds 400-line guideline; split immediately as per issue #401.

This file is now 513 lines—28% over the 400-line maximum and 28 lines larger than when issue #401 was created. The guideline states: "Files must not exceed 400 lines in length." Decompose this module without delay.

Split the file as follows:

1. Extract shared helpers to tests/common/fragment_helpers.rs:

   • fragmentation_config and fragmentation_config_with_timeout (lines 68-88)
   • fragment_envelope (lines 90-112)
   • send_envelopes, read_reassembled_response (lines 114-148)
   • make_handler, make_app, spawn_app (lines 150-186)
   • build_envelopes, assert_handler_observed, read_response_payload (lines 188-225)

2. Move rejection infrastructure to tests/fragment_transport/rejection.rs:

   • FragmentRejectionSetup (lines 285-314)
   • test_fragment_rejection (lines 316-340)
   • Mutator functions and FragmentMutator type (lines 342-398)
   • fragment_rejection_cases test (lines 400-416)

3. Move eviction test to tests/fragment_transport/eviction.rs:

   • expired_fragments_are_evicted (lines 418-461)

4. Keep round-trip tests and API toggle test in tests/fragment_transport.rs or create tests/fragment_transport/round_trip.rs:

   • run_round_trip_test and related tests (lines 227-283)
   • fragmentation_can_be_disabled_via_public_api (lines 463-513)

This brings the main file well under 400 lines and creates a clear module structure.

🤖 Prompt for AI Agents

In tests/fragment_transport.rs (513 lines), the file exceeds the 400-line
guideline by 28%; create a module structure by extracting:
(1) lines 68-225 (shared helpers) into tests/common/fragment_helpers.rs,
re-exporting them in tests/common/mod.rs;
(2) lines 285-416 (rejection infrastructure and tests) into
tests/fragment_transport/rejection.rs;
(3) lines 418-461 (eviction test) into tests/fragment_transport/eviction.rs;
(4) convert tests/fragment_transport.rs into a module directory with mod.rs
containing the round-trip tests and API toggle test (lines 227-283 and 463-513),
and add mod declarations for rejection and eviction;
update all imports to use the common helpers and ensure all tests compile and
run successfully.

src/app/connection.rs (1)

131-145: Simplify decode error flow; EnvelopeDecodeError::Parse arm is currently unreachable

Rationalise the error handling between parse_envelope, handle_decode_failure, and decode_envelope:

• In parse_envelope, the or_else closure discards the original Parse error and replaces it with a Deserialize error when the fallback also fails. As a result, decode_envelope never observes Err(EnvelopeDecodeError::Parse(_)), so the "failed to parse message" branch is effectively dead and only the deserialise context is used.
• Since handle_decode_failure already accepts a generic Debug error, collapse the two Err branches in decode_envelope into a single "failed to decode message" path, which removes unreachable code and makes the behaviour explicit.

Apply this diff to simplify decode_envelope and rely on the unified error type:

     fn decode_envelope(
         &self,
         frame: &[u8],
         deser_failures: &mut u32,
     ) -> Result<Option<Envelope>, io::Error> {
         match self.parse_envelope(frame) {
             Ok((env, _)) => {
                 *deser_failures = 0;
                 Ok(Some(env))
             }
-            Err(EnvelopeDecodeError::Parse(e)) => {
-                Self::handle_decode_failure(deser_failures, "failed to parse message", e)
-            }
-            Err(EnvelopeDecodeError::Deserialize(e)) => {
-                Self::handle_decode_failure(
-                    deser_failures,
-                    "failed to deserialize message",
-                    e,
-                )
-            }
+            Err(err) => Self::handle_decode_failure(
+                deser_failures,
+                "failed to decode message",
+                err,
+            ),
         }
     }

Also align the spelling in the doc comments on lines 35 and 283 so both use en‑GB spelling (deserialisation) for consistency with the project guidelines.

Also applies to: 283-300, 313-317

♻️ Duplicate comments (5)

tests/correlation_id.rs (1)

18-19: Duplicate TestResult alias — tracked for follow-up.

This import duplicates the TestResult type alias found in ~29 other test files. Per PR comments, centralise a single definition in tests/common/mod.rs and import it everywhere. This is a known unresolved issue; track and address in a follow-up.

tests/worlds/multi_packet.rs (1)

32-46: Simplify the double map_err chain.

The two consecutive map_err calls can be collapsed into one. The intermediate WireframeRunError wrapper exists solely to satisfy trait bounds, and the final conversion to Box<dyn Error + Send + Sync> can happen in a single step.

         let mut frames = Vec::new();
         actor
             .run(&mut frames)
             .await
-            .map_err(WireframeRunError)
-            .map_err(Box::<dyn std::error::Error + Send + Sync>::from)?;
+            .map_err(|e| Box::<dyn std::error::Error + Send + Sync>::from(WireframeRunError(e)))?;
         Ok(frames)

src/fragment/payload.rs (1)

97-105: Simplify unreachable defensive checks.

After the minimum_len check at line 85 passes (payload.len() >= 6), both payload.get(4) and payload.get(5) are guaranteed to succeed. The .ok_or(DecodeError::UnexpectedEnd { additional: 0 }) arms are unreachable, and additional: 0 is misleading if somehow reached.

Use direct indexing to eliminate dead code:

     let header_len_offset = FRAGMENT_MAGIC.len();
-    let len_hi = payload
-        .get(header_len_offset)
-        .copied()
-        .ok_or(DecodeError::UnexpectedEnd { additional: 0 })?;
-    let len_lo = payload
-        .get(header_len_offset + 1)
-        .copied()
-        .ok_or(DecodeError::UnexpectedEnd { additional: 0 })?;
-    let len_bytes = [len_hi, len_lo];
+    // SAFETY: minimum_len check above guarantees indices 4 and 5 are in bounds.
+    let len_bytes = [payload[header_len_offset], payload[header_len_offset + 1]];

src/frame/conversion.rs (2)

97-114: Add doc comment for clarity and maintainability.

Whilst doc comments on private functions are optional per coding guidelines, a past review requested documentation for this helper to explain its behaviour and the relationship between the value, endianness, and prefix length. Add a brief doc comment to improve maintainability.

Apply this diff:

+/// Write a u64 value into `prefix` according to the specified endianness.
+///
+/// The number of bytes written is determined by `prefix.len()`. For big-endian,
+/// the most significant byte is written first; for little-endian, the least
+/// significant byte is written first.
 fn write_bytes_with_endianness(value: u64, endianness: Endianness, prefix: &mut [u8]) {
     let size = prefix.len();
     match endianness {

🤖 Prompt for AI Agents

In src/frame/conversion.rs at line 97, add a Rustdoc comment (///) above the write_bytes_with_endianness function documenting that it writes a u64 value into the prefix slice according to the specified endianness, and that the number of bytes written is determined by prefix.len(). Include a brief note that for big-endian the most significant byte is written first and for little-endian the least significant byte is written first.

---

76-95: Refine doc comment format and simplify unreachable arm.

Whilst doc comments on private functions are optional per coding guidelines, a past review requested a # Preconditions section to document that size must be validated by the caller. Additionally, the wildcard arm at lines 86–92 uses debug_assert!(false, ...) followed by an error return. Since size is validated at line 130 in the caller, replace the defensive check with unreachable!() to make it explicit that this branch cannot execute.

Apply this diff to improve the doc comment and simplify the unreachable arm:

 /// Convert a length value into a u64 based on the prefix size.
 ///
-/// Callers are expected to validate `size` against the supported set
-/// `{1, 2, 4, 8}` before invoking this helper.
+/// # Preconditions
+/// The caller must validate that `size ∈ {1, 2, 4, 8}` before invoking this function.
 fn convert_len_to_value(len: usize, size: usize) -> io::Result<u64> {
     let value = match size {
         1 => u64::from(checked_prefix_cast::<u8>(len)?),
         2 => u64::from(checked_prefix_cast::<u16>(len)?),
         4 => u64::from(checked_prefix_cast::<u32>(len)?),
         8 => checked_prefix_cast(len)?,
         _ => {
-            debug_assert!(false, "size should be validated upstream");
-            return Err(io::Error::new(
-                io::ErrorKind::InvalidInput,
-                ERR_UNSUPPORTED_PREFIX,
-            ));
+            unreachable!("size validated to be 1, 2, 4, or 8 by caller")
         }
     };
     Ok(value)
 }

🤖 Prompt for AI Agents

In src/frame/conversion.rs at lines 76–95, update the doc comment on convert_len_to_value to use a standard # Preconditions section stating "The caller must validate that size ∈ {1, 2, 4, 8} before invoking this function." Additionally, replace the wildcard match arm (lines 86–92) which contains a debug_assert!(false, ...) followed by an error return with a single unreachable!() call with message "size validated to be 1, 2, 4, or 8 by caller" to make it explicit that this branch is logically impossible after caller validation.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b5c456b and 1fc761a.

📒 Files selected for processing (11)

• examples/packet_enum.rs (2 hunks)
• src/app/connection.rs (6 hunks)
• src/fragment/payload.rs (6 hunks)
• src/frame/conversion.rs (3 hunks)
• src/frame/tests.rs (5 hunks)
• tests/connection_fragmentation.rs (3 hunks)
• tests/correlation_id.rs (4 hunks)
• tests/fragment_transport.rs (8 hunks)
• tests/response.rs (4 hunks)
• tests/routes.rs (2 hunks)
• tests/worlds/multi_packet.rs (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Run make check-fmt, make lint, and make test before committing. make check-fmt executes cargo fmt --workspace -- --check. make lint executes cargo clippy --workspace --all-targets --all-features -- -D warnings. make test executes cargo test --workspace.
Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
Extract meaningfully named helper functions adhering to separation of concerns and CQRS when a Rust function is too long.
Group related parameters in meaningfully named structs when a Rust function has too many parameters.
Consider using Arc to reduce the amount of data returned when a Rust function is returning a large error.
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 APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data in Rust and avoid unnecessary mut bindings.
Handle errors with the Result type in Rust instead of panicking where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in 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.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated Rust tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a bac...

Files:

• src/frame/tests.rs
• examples/packet_enum.rs
• src/fragment/payload.rs
• src/app/connection.rs
• src/frame/conversion.rs
• tests/fragment_transport.rs
• tests/correlation_id.rs
• tests/routes.rs
• tests/connection_fragmentation.rs
• tests/worlds/multi_packet.rs
• tests/response.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 return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for 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/frame/tests.rs
• examples/packet_enum.rs
• src/fragment/payload.rs
• src/app/connection.rs
• src/frame/conversion.rs
• tests/fragment_transport.rs
• tests/correlation_id.rs
• tests/routes.rs
• tests/connection_fragmentation.rs
• tests/worlds/multi_packet.rs
• tests/response.rs

🧬 Code graph analysis (8)

src/frame/tests.rs (1)

src/frame/conversion.rs (2)

• bytes_to_u64 (25-74)
• u64_to_bytes (124-152)

examples/packet_enum.rs (4)

examples/ping_pong.rs (2)

• build_app (139-145)
• main (152-179)

src/app/envelope.rs (2)

• new (85-91)
• new (117-123)

src/server/config/binding.rs (2)

• bind (117-120)
• bind (196-199)

src/app/connection.rs (1)

• handle_connection (152-178)

src/app/connection.rs (3)

src/app/fragmentation_state.rs (2)

• purge_expired (75-75)
• new (35-40)

src/app/frame_handling.rs (2)

• forward_response (89-134)
• new (29-29)

src/app/envelope.rs (2)

• new (85-91)
• new (117-123)

tests/fragment_transport.rs (1)

src/fragment/payload.rs (2)

• decode_fragment_payload (81-126)
• encode_fragment_payload (53-69)

tests/correlation_id.rs (4)

src/connection.rs (8)

• new (34-38)
• new (106-106)
• new (205-210)
• new (275-287)
• new (949-962)
• correlation_id (234-239)
• with_hooks (291-321)
• default (110-115)

src/app/envelope.rs (7)

• new (85-91)
• new (117-123)
• correlation_id (53-53)
• correlation_id (99-99)
• correlation_id (107-107)
• correlation_id (129-129)
• payload (132-132)

tests/routes.rs (1)

• correlation_id (44-44)

src/correlation.rs (3)

• correlation_id (11-11)
• correlation_id (18-18)
• correlation_id (24-24)

tests/routes.rs (2)

src/app/builder.rs (1)

• new (159-159)

wireframe_testing/src/helpers.rs (3)

• drive_with_bincode (373-393)
• decode_frames (116-118)
• new_test_codec (99-103)

tests/worlds/multi_packet.rs (3)

tests/worlds/mod.rs (1)

• build_small_queues (24-27)

tests/worlds/stream_end.rs (1)

• build_small_queues (55-55)

src/response.rs (1)

• with_channel (155-158)

tests/response.rs (1)

wireframe_testing/src/helpers.rs (4)

• decode_frames (116-118)
• decode_frames_with_max (124-133)
• encode_frame (421-426)
• run_app (428-471)

🔍 Remote MCP

Perfect! I now have comprehensive information about the wireframe repository. Let me provide a focused summary of the relevant context for reviewing this PR.

PR Context Summary

This pull request is a major refactoring effort titled "Refactor unwrap/expect; propagate errors; pluggable server plumbing (PR #400)" that makes significant changes to error handling and architecture across the wireframe framework.

Key Refactoring Goals

The PR implements three primary objectives:

1. Error Propagation Instead of Panicking: Replaces unwrap() and expect() calls with proper Result-based error handling using ? and map_err() throughout the codebase—from core components to tests.

2. Centralized Error-Tracking Types: Introduces new types like DeserFailureTracker and ResponseContext to manage deserialization and fragmentation errors in a structured way.

3. Server Plumbing Improvements: Adds ConnectionChannels wrapper (bundling push queues and handles) and AcceptLoopOptions with related accept-loop context refinements; aligns preamble/backoff logic with explicit error propagation.

Testing Infrastructure Changes

The testing infrastructure uses a four-layer testing strategy: Layer 1 focuses on foundational correctness with unit and integration tests using rstest for fixtures; Layer 2 focuses on resilience testing; Layer 3 includes property-based testing with proptest and concurrency fuzzing with loom; and Layer 4 uses criterion for performance benchmarking. This PR significantly refactors the test layer to use Result-based error propagation instead of panics, aligning with the broader refactoring goal.

Error Handling Architecture

The codebase primarily uses a structured approach to error handling, distinguishing between different types of errors and providing mechanisms for graceful recovery and reporting through key error types including WireframeError<E> for general framework errors, PushError for issues within the asynchronous push messaging system, and ServerError for server-specific operational failures. This PR expands this error handling by propagating these types throughout the codebase instead of panicking.

Production Resilience Features

Error handling patterns involve using Result for recoverable errors, thiserror for semantic error enums, and an optional Dead Letter Queue for critical message delivery. The PR strengthens these patterns by ensuring all fallible operations properly propagate errors through the actor system.

Public API Changes to Review

According to the PR objectives summary, reviewers should focus on:

• Correctness of error propagation in hot paths (connection actors, message processing, frame handling)
• Public API changes requiring migration notes (notably ConnectionChannels, AcceptLoopOptions, and PreambleHooks changes)
• Test infrastructure hygiene (approximately 29 duplicated TestResult type aliases across tests that should be centralized)

⏰ 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 (37)

examples/packet_enum.rs (2)

81-85: LGTM!

The build_app function correctly propagates errors using ? and returns a Result type, aligning with the PR's error-handling refactoring goals. The use of Arc::new is consistent with the import.

---

87-119: Well-structured accept loop with graceful shutdown.

The main function correctly:

• Initialises the tracing subscriber before use
• Propagates errors via ? rather than panicking
• Implements graceful shutdown with tokio::select! and signal::ctrl_c()
• Uses a narrowly-scoped #[expect] with a clear reason for the clippy lint

This aligns well with examples/ping_pong.rs and the broader PR goals.

tests/worlds/multi_packet.rs (5)

16-23: LGTM: WireframeRunError wrapper type is well-structured.

The error wrapper correctly implements Display and Error traits, enabling seamless conversion into the boxed error type used by TestResult.

---

48-53: LGTM: Back-pressure helper is concise and correct.

The helper sends a single byte, awaits back-pressure resolution, then closes the channel by dropping the sender. Clear and fit for purpose.

---

77-86: Doc comment now present — past review resolved.

The send_payload function now includes the requested documentation explaining the silent return behaviour when the receiver is dropped.

---

119-120: Clarifying comment now present — past review resolved.

The inline comment explaining the double ? operator (// Unwrap JoinError from await, then the task's Result) addresses the previous review concern.

---

131-154: LGTM: Verification methods use appropriate assertions.

The verify_empty, verify, and verify_overflow methods correctly use assert! and assert_eq! macros with descriptive messages. The # Panics doc sections are accurate.

tests/correlation_id.rs (4)

22-48: LGTM: Test uses Result-based flow with clear assertions.

The #[expect] attribute correctly documents the intentional use of assert! in a Result-returning function. The correlation ID check now uses assert! with all() as previously suggested, providing clear diagnostics on failure.

---

51-85: LGTM: Helper function correctly propagates errors.

The run_multi_packet_channel helper uses the updated ConnectionChannels::new pattern and properly propagates errors via ?. The error mapping to io::Error provides useful diagnostics.

---

87-107: Past review resolved: assert_eq! now used for correlation check.

The previous review requested replacing the manual if correlations != expected { return Err(...) } pattern with assert_eq!. This has now been implemented at lines 102-105.

---

124-133: Past review resolved: slice pattern match enforces exactly one frame.

The previous review requested a slice pattern match to validate exactly one terminator frame. The code now uses let [terminator] = frames.as_slice() else { ... } which correctly enforces cardinality.

tests/connection_fragmentation.rs (2)

42-85: LGTM! Past review comments resolved and error propagation is correct.

The test now:

• Returns TestResult and propagates errors with ? throughout
• Uses assert! instead of verbose if-return-Err patterns (resolving past comment)
• Employs map_err to provide context for actor run failures
• Correctly uses #[expect(clippy::panic_in_result_fn)] with a clear reason, as assert! macros panic in Result-returning functions

The reassembly loop logic (lines 72-80) correctly handles both fragmented and unfragmented payloads, with the final assertion ensuring exactly one reassembled message is produced.

---

87-119: LGTM! All past review comments successfully resolved.

The test now:

• Uses assert_eq! at line 107 instead of the verbose if-return-Err pattern (resolving past comment)
• Provides a clear, accurate error message at lines 108-111: "expected single frame but none found" correctly describes the absence condition when ok_or triggers (resolving past comment about the misleading "frame present" message)
• Propagates errors consistently with ? and map_err

The test logic correctly verifies that small payloads pass through unfragmented and remain unmodified.

src/fragment/payload.rs (8)

11-16: LGTM!

Import of EncodeError correctly supports the new error propagation at line 59.

---

35-42: Documented panic for constant initialisation is acceptable.

The # Panics section documents this behaviour, and failure here would indicate a programmer error in the constant header definition. Propagating errors from fragment_overhead() would burden all callers unnecessarily.

---

58-59: LGTM!

Proper error propagation using map_err instead of .expect(). This aligns with the PR's objective of replacing panics with Result-based flows.

---

84-94: LGTM!

The minimum length check at lines 84-86 ensures safe access to prefix bytes. The get-based prefix check at lines 89-91 is defensive (technically unreachable after the length check) but maintains consistency with the rest of the function.

---

110-125: LGTM!

The get-based bounds check at lines 110-114 with saturating_sub prevents underflow. The unwrap_or_default() at line 124 correctly returns an empty remainder slice when the header consumes the entire payload.

---

155-163: LGTM!

This test correctly verifies that payloads shorter than minimum_len (6 bytes) return Ok(None) rather than attempting to decode incomplete data.

---

174-190: LGTM!

The assert_fragment_decode_error helper eliminates duplication between the truncated header and length mismatch tests, aligning with the DRY principle and coding guidelines for using shared test setup.

---

192-247: LGTM!

These tests comprehensively cover the error paths introduced by the refactoring:

• decode_fragment_payload_rejects_truncated_header validates UnexpectedEnd when advertised length exceeds actual payload.
• decode_fragment_payload_rejects_missing_header_bytes validates UnexpectedEnd with correct additional count.
• decode_fragment_payload_rejects_length_mismatch validates OtherString error when consumed bytes differ from advertised length.

tests/response.rs (4)

56-75: LGTM—consistent error propagation and clear diagnostics.

The test properly returns TestResult, uses ? for error propagation with descriptive mappings, and employs assert_eq! for clear failure output. The #[expect] attribute is narrowly scoped with a valid reason.

---

149-152: Safe slicing approved.

The use of .get() for bounds checking followed by .expect() (which is permissible in tests) provides a clear failure path if the encoded buffer is unexpectedly short.

---

219-240: Consistent error handling pattern approved.

The test correctly chains buffer_capacity on the TestApp::new()? result and applies uniform .map_err() mappings for both send_response and deserialization failures.

---

245-273: Large-frame round-trip test properly structured.

The 9 MiB payload test correctly propagates errors through serialization, framing, transport, and deserialization. Error mappings are consistent and descriptive.

tests/routes.rs (3)

66-106: Handler test demonstrates proper Result-based testing pattern.

The test correctly uses TestResult<()>, slice pattern matching for single-frame extraction, and clear assert_eq! diagnostics. Route setup propagates errors with ?.

---

109-140: None correlation ID test properly structured.

The slice pattern matching at lines 128-130 cleanly enforces the single-frame expectation, and the subsequent assertions correctly verify both the absence of a correlation ID and the echoed payload.

---

143-191: Sequential processing test correctly validates frame order.

The slice destructuring at lines 171-173 atomically extracts both frames, and the individual assert_eq! calls at lines 178-189 provide separate, clear failure diagnostics for each correlation ID and payload check.

src/app/connection.rs (3)

29-33: Use helper purge_expired for clearer timeout handling

Retain this helper; it removes the previous map-for-side-effect pattern and makes the timeout branch intent explicit whilst keeping the fragmentation state handling local and side‑effect free at the call‑site.

Also applies to: 225-225

---

38-48: Keep FrameHandlingContext to group per‑frame state and reduce parameter noise

Use this context struct to bundle framed, deser_failures, routes, and fragmentation for handle_frame; the call‑site in process_stream stays readable and the destructuring in handle_frame keeps borrow scopes obvious across the async body. This aligns with the guideline to group related parameters rather than extending function signatures.

Also applies to: 212-217, 236-247

---

263-272: Retain ResponseContext wiring for outbound path

Route the decoded env through frame_handling::forward_response with ResponseContext { serializer: &self.serializer, framed, fragmentation }; this correctly exposes the serializer, framed transport, and fragmentation state to the response pipeline and keeps error propagation on io::Result<()> without re‑introducing panics.

src/frame/tests.rs (3)

50-54: LGTM: Bounds-checked access prevents silent test failures.

The replacement of unwrap_or_default() with .expect() ensures that if written is invalid, the test panics with a clear diagnostic rather than silently comparing against an empty slice.

---

61-70: LGTM: Clear error messages and comprehensive edge-case coverage.

The updated bytes_to_u64_short test now uses a specific error message that matches the assertion, and the new bytes_to_u64_rejects_empty_input test properly covers the empty-slice edge case.

---

80-82: LGTM: Error messages are specific and bounds checks prevent vacuous assertions.

All updated tests now use descriptive error messages that clearly document the expected failure mode and match their assertions. The addition of u64_to_bytes_rejects_oversized_prefix provides coverage for the oversized-prefix edge case. The u64_to_bytes_zeroes_remainder test now uses .expect() to prevent the all() check from vacuously passing on an empty slice.

Also applies to: 88-90, 101-107, 118-120, 132-139

src/frame/conversion.rs (3)

40-43: LGTM: Defensive bounds check with appropriate comment prefix.

The .get() check is mathematically redundant after the validation at line 32, but the "NOTE:" comment appropriately documents this as deliberate defensive coding rather than justifying an unsafe block.

---

45-66: Accept defensive checks as design choice.

Whilst the .get_mut() checks are mathematically unreachable after validation (the buffer is [0u8; 8] and size ∈ {1, 2, 4, 8}), they're explicitly marked as defensive fallbacks. The debug_assert!(false, ...) statements document that these branches should never execute. This is a conscious trade-off between defensive coding and eliminating dead code.

---

137-149: LGTM: Appropriately scoped lint suppression and safe tail handling.

The #[expect(clippy::indexing_slicing, ...)] annotation is narrowly scoped and includes a clear reason, which is acceptable per coding guidelines. The direct indexing is safe given the validation at line 130, and the tail zeroing uses .get_mut() to handle the edge case where size equals the buffer length.

[leynos]

@coderabbitai re: Also align the spelling in the doc comments on lines 35 and 283 so both use en‑GB spelling (deserialisation) for consistency with the project guidelines.

Also applies to: 283-300, 313-317

"serialization" is correct for en-GB-oxendict (-ize / -yse / -our)

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@leynos You're absolutely right, and I apologise for the confusion.

I misunderstood the en-GB-oxendict spelling convention. Oxford spelling (oxendict) uses -ize endings, not -ise, so "serialization" is indeed correct according to your project guidelines.

To clarify for my understanding:

• ✅ -ize endings: serialization, optimization, organization
• ✅ -yse endings: analyse
• ✅ -our endings: colour, behaviour

Thank you for the correction—"serialization" is the correct spelling and should remain unchanged in the documentation.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

tests/routes.rs (1)

194-226: Add the missing #[expect(clippy::panic_in_result_fn, ...)] attribute.

This test returns TestResult<()> and uses assert_eq! at line 224, which can panic. The other three tests in this file have the scoped lint expectation, but this one lacks it. Add the attribute for consistency and to satisfy Clippy.

Apply this diff:

 #[rstest]
 #[case(None)]
 #[case(Some(1))]
 #[case(Some(2))]
 #[tokio::test]
+#[expect(
+    clippy::panic_in_result_fn,
+    reason = "asserts provide clearer diagnostics in tests"
+)]
 async fn single_frame_propagates_correlation_id(#[case] cid: Option<u64>) -> TestResult<()> {

🤖 Prompt for AI Agents

In tests/routes.rs at line 199, add the attribute #[expect(clippy::panic_in_result_fn, reason = "asserts provide clearer diagnostics in tests")] immediately before the async fn single_frame_propagates_correlation_id signature, between the #[tokio::test] attribute and the function definition, matching the pattern used by the other Result-returning test functions in this file.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1fc761a and 14a2176.

📒 Files selected for processing (3)

• examples/packet_enum.rs (2 hunks)
• src/app/connection.rs (7 hunks)
• tests/routes.rs (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Run make check-fmt, make lint, and make test before committing. make check-fmt executes cargo fmt --workspace -- --check. make lint executes cargo clippy --workspace --all-targets --all-features -- -D warnings. make test executes cargo test --workspace.
Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
Extract meaningfully named helper functions adhering to separation of concerns and CQRS when a Rust function is too long.
Group related parameters in meaningfully named structs when a Rust function has too many parameters.
Consider using Arc to reduce the amount of data returned when a Rust function is returning a large error.
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 APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data in Rust and avoid unnecessary mut bindings.
Handle errors with the Result type in Rust instead of panicking where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in 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.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated Rust tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a bac...

Files:

• examples/packet_enum.rs
• tests/routes.rs
• src/app/connection.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 return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for 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/packet_enum.rs
• tests/routes.rs
• src/app/connection.rs

🧬 Code graph analysis (3)

examples/packet_enum.rs (2)

src/app/connection.rs (2)

• serializer (134-134)
• handle_connection (142-168)

examples/ping_pong.rs (2)

• build_app (139-145)
• main (152-179)

tests/routes.rs (1)

wireframe_testing/src/helpers.rs (3)

• drive_with_bincode (373-393)
• decode_frames (116-118)
• new_test_codec (99-103)

src/app/connection.rs (2)

src/app/fragmentation_state.rs (2)

• purge_expired (75-75)
• new (35-40)

src/app/frame_handling.rs (2)

• forward_response (89-134)
• new (29-29)

⏰ 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 (17)

src/app/connection.rs (9)

29-33: Clean extraction of purge logic.

The if let pattern correctly avoids the non-idiomatic .map() for side effects flagged in prior review. This helper keeps process_stream focused on its primary responsibility.

---

38-48: Well-structured context bundle.

The struct cleanly groups per-frame state as per the coding guideline to bundle related parameters. The doc comment is concise and informative.

---

127-135: Appropriate error type for internal use.

Using Box<dyn std::error::Error + Send + Sync> is acceptable here since parse_envelope is private. The or_else fallback to full deserialization is idiomatic.

---

200-209: Clean context construction at call site.

Inline construction of FrameHandlingContext keeps the call site readable whilst bundling the required state. The borrow patterns are correct.

---

223-236: Effective use of context destructuring.

Receiving FrameHandlingContext and immediately destructuring it provides the clarity of named parameters without the parameter-list bloat. This aligns with CQRS and separation of concerns.

---

252-262: Correct integration with ResponseContext.

The call passes references matching the forward_response signature in frame_handling.rs. This decouples serialization/framing/fragmentation concerns as intended.

---

273-290: Centralised decode failure handling.

This helper consolidates the duplicate logic for deserialization failures, properly incrementing counters, logging, updating metrics, and terminating connections at the threshold. The doc comment addresses the prior review request.

---

292-306: Clean delegation to centralised handler.

The decode_envelope function correctly resets the failure counter on success and delegates errors to handle_decode_failure. The context string provides adequate diagnostic information.

---

213-216: Proper timeout-triggered purge.

The timeout branch correctly invokes the extracted purge_expired helper, maintaining reassembly hygiene during idle periods.

tests/routes.rs (6)

1-7: LGTM!

Module-level documentation is present and the common module import aligns with the PR's goal of centralising TestResult.

---

31-61: LGTM!

The TestEnvelope implementation follows the single-line function style for simple accessors and correctly implements the Packet trait.

---

66-107: LGTM!

Good use of the #[expect] attribute, Result-based error propagation, slice pattern extraction, and descriptive assertion messages.

---

109-141: LGTM!

Consistent structure with the lint expectation, proper error propagation, and clear assertions.

---

143-192: LGTM!

The two-frame slice pattern extraction is clean, and the individual assertions provide clear diagnostics on failure.

---

228-238: LGTM!

Simple round-trip test for the Packet trait implementation. Since this returns () rather than Result, no lint expectation is needed.

examples/packet_enum.rs (2)

6-21: Keep consolidated imports and DEFAULT_ADDR constant.

Maintain the grouped std/tokio imports and the DEFAULT_ADDR constant; this keeps the example consistent with other binaries and removes the hard-coded address literal from the control flow. No changes required here.

---

83-87: Retain the build_app wiring as-is.

Keep composing the app with DecodeMiddleware and the route(1, Arc::new(handle_packet)) handler; this matches the middleware/handler model used elsewhere and keeps error propagation within wireframe::app::Result<App>.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 14a2176 and e15c9db.

📒 Files selected for processing (1)

• examples/packet_enum.rs (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Run make check-fmt, make lint, and make test before committing. make check-fmt executes cargo fmt --workspace -- --check. make lint executes cargo clippy --workspace --all-targets --all-features -- -D warnings. make test executes cargo test --workspace.
Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
Extract meaningfully named helper functions adhering to separation of concerns and CQRS when a Rust function is too long.
Group related parameters in meaningfully named structs when a Rust function has too many parameters.
Consider using Arc to reduce the amount of data returned when a Rust function is returning a large error.
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 APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data in Rust and avoid unnecessary mut bindings.
Handle errors with the Result type in Rust instead of panicking where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in 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.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated Rust tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a bac...

Files:

• examples/packet_enum.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 return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for 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/packet_enum.rs

🔍 Remote MCP Deepwiki

Based on the comprehensive documentation I've retrieved, I now have extensive context about the wireframe framework architecture and how this PR fits into the broader system. Let me provide a focused summary of relevant context.

Context Summary for PR Review

Wireframe Framework Architecture

The wireframe framework is a binary protocol server framework built on Rust's async ecosystem. The core architecture follows an actor-per-connection model where:

1. WireframeServer manages TCP listeners and worker pools
2. WireframeApp configures routing, middleware, and application logic
3. ConnectionActor handles individual connections with biased event loops prioritizing: shutdown signals → high-priority messages → low-priority messages → response streams

Key Design Principles:

• Zero-copy operations where possible
• Type-safe extensibility through traits (Serializer, FrameProcessor, Packet, WireframeProtocol)
• Graceful shutdown using CancellationToken + TaskTracker
• Actor-based concurrency avoiding complex locks
• Configurable rate limiting via leaky-bucket
• Built-in observability through tracing and metrics

Error Propagation Rationale

This PR's focus on error propagation instead of panic/unwrap aligns with wireframe's production-readiness philosophy documented in the Resilience and Error Handling section:

• Panic Isolation: Individual connection task panics must not crash the server
• Resource Management: Weak references in SessionRegistry prevent memory leaks; proper cleanup on error paths is critical
• DoS Protection: Rate limiting and memory caps require proper error handling to trigger backoff/connection termination
• Observable Errors: Structured error types enable proper metrics collection and logging

Core Components Being Modified

1. FragmentAdapter & Reassembly: Transport-level message fragmentation (Phase 7 roadmap) - error handling critical for DoS protection and buffer management
2. ConnectionActor: Central message processing loop - errors must propagate through prioritized select! loop without panicking
3. Push Queues (PushHandle): Back-pressure mechanism - queue full/closed conditions must return errors for intelligent retry/drop decisions
4. Protocol Hooks (WireframeProtocol): Extensibility points where custom handlers can generate recoverable errors

Test Infrastructure Pattern

The PR introduces TestResult pattern across tests - a standard Rust error handling approach:

type TestResult<T = ()> = Result<T, Box<dyn std::error::Error + Send + Sync>>;

This enables property-based composition of test steps using ? operator rather than panics, improving test maintainability and enabling better error diagnostics.

Production Deployment Considerations

From the CI/CD workflow documentation, this refactor supports:

• Zero Clippy warnings - unwrap/expect creates warnings
• Graceful degradation - errors propagate allowing recovery attempts
• Observable failures - structured errors can be logged and metered
• Resource exhaustion protection - errors trigger backpressure rather than OOM panics,

🔇 Additional comments (1)

examples/packet_enum.rs (1)

89-128: Solid example structure with proper error propagation throughout.

The main function demonstrates correct patterns:

• Tracing initialised before operations
• Error mapping from wireframe::app::Result to io::Result
• Environment variable fallback with DEFAULT_ADDR constant
• Descriptive error messages on address parse failure
• Accept loop errors propagated with ?
• Graceful shutdown via signal::ctrl_c() with explicit Result matching (not silently discarding the error)
• Properly scoped #[expect] with clear reason for the tokio::select! lint

This aligns well with the PR's error-propagation objectives.

[coderabbitai]

Actionable comments posted: 3

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e15c9db and f840d41.

📒 Files selected for processing (7)

• examples/metadata_routing.rs (4 hunks)
• examples/packet_enum.rs (2 hunks)
• examples/ping_pong.rs (6 hunks)
• src/app/connection.rs (8 hunks)
• src/server/connection.rs (5 hunks)
• tests/fragment_transport.rs (7 hunks)
• tests/middleware_order.rs (3 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Run make check-fmt, make lint, and make test before committing. make check-fmt executes cargo fmt --workspace -- --check. make lint executes cargo clippy --workspace --all-targets --all-features -- -D warnings. make test executes cargo test --workspace.
Clippy warnings MUST be disallowed in Rust code.
Fix any warnings emitted during Rust tests in the code itself rather than silencing them.
Extract meaningfully named helper functions adhering to separation of concerns and CQRS when a Rust function is too long.
Group related parameters in meaningfully named structs when a Rust function has too many parameters.
Consider using Arc to reduce the amount of data returned when a Rust function is returning a large error.
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 APIs in Rust using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data in Rust and avoid unnecessary mut bindings.
Handle errors with the Result type in Rust instead of panicking where feasible.
Avoid unsafe code in Rust unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments in Rust.
Do not use return in 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.
Prefer expect over allow in Rust.
Use rstest fixtures for shared test setup in Rust.
Replace duplicated Rust tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs in Rust.
Prefer .expect() over .unwrap() in Rust.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a bac...

Files:

• examples/metadata_routing.rs
• tests/middleware_order.rs
• examples/packet_enum.rs
• examples/ping_pong.rs
• src/server/connection.rs
• tests/fragment_transport.rs
• src/app/connection.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 return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless 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 rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for 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/metadata_routing.rs
• tests/middleware_order.rs
• examples/packet_enum.rs
• examples/ping_pong.rs
• src/server/connection.rs
• tests/fragment_transport.rs
• src/app/connection.rs

🧬 Code graph analysis (5)

examples/metadata_routing.rs (1)

src/app/envelope.rs (2)

• new (85-91)
• payload (132-132)

tests/middleware_order.rs (2)

wireframe_testing/src/helpers.rs (2)

• encode_frame (421-426)
• decode_frames (116-118)

src/app/envelope.rs (6)

• into_parts (56-56)
• into_parts (101-101)
• correlation_id (53-53)
• correlation_id (99-99)
• correlation_id (107-107)
• correlation_id (129-129)

examples/ping_pong.rs (2)

examples/packet_enum.rs (2)

• main (94-130)
• build_app (83-87)

src/app/connection.rs (1)

• handle_connection_result (147-176)

tests/fragment_transport.rs (2)

src/fragment/payload.rs (7)

• std (39-39)
• std (62-62)
• std (84-84)
• std (107-107)
• std (169-169)
• decode_fragment_payload (81-126)
• encode_fragment_payload (53-69)

src/fragment/config.rs (1)

• for_frame_budget (35-53)

src/app/connection.rs (3)

src/app/fragmentation_state.rs (2)

• purge_expired (75-75)
• new (35-40)

src/app/envelope.rs (2)

• new (85-91)
• new (117-123)

src/app/frame_handling.rs (2)

• forward_response (89-134)
• new (29-29)

🔍 Remote MCP Ref

Summary of additional, concrete review-relevant facts found:

• PR web URL and metadata from PR object in provided context: #400 (author: leynos, branch additional-lints → main) — see PR header in context.
• Tracing/logging dependency rationale: tracing/tracing-subscriber is the project's chosen observability stack (relevant to examples and examples -> tracing init changes).
• No remote search results returned for the exact PR content via the Ref tool (search returned no direct PR page), so all file-level facts below are drawn from the provided PR diff summary in the context (changes to Cargo.toml, Makefile, src/, tests/) — I used the Ref search tool during discovery.

Key reviewer action points (directly applicable):

• Focus on correctness of propagated errors in hot paths: accept loop (AcceptLoopOptions / accept_iteration), spawn_connection_task/process_stream (PreambleHooks), connection handling (handle_connection_result), fragmentation reassembly (FragmentCursor, SliceBounds), and response flow (ResponseContext / DeserFailureTracker). (Files: src/server/runtime.rs, src/server/connection.rs, src/app/connection.rs, src/fragment/{fragmenter,payload,reassembler}, src/app/frame_handling.rs) — see PR summary.
• Verify public API changes that require migration notes:
  • ConnectionActor::new / with_hooks signatures now take ConnectionChannels (public) instead of separate queues/handles.
  • Several test-support builders now return Result/PushConfigError (build_small_queues, push queues constructors) and many tests now use a shared TestResult alias; check usages and exported test helpers. (Files: src/connection.rs, tests/*, tests/worlds/mod.rs) — see PR summary.
• Confirm fragmentation safety improvements and new error variant:
  • Fragmenter now uses FragmentCursor and adds bounds-checking; FragmentationError gained SliceBounds {offset,end,total}. Inspect build_fragments_from and decode_fragment_payload for correct early-return semantics and test coverage. (Files: src/fragment/{fragmenter,payload,error}.rs, src/fragment/tests.rs) — see PR summary.
• Check removal/replacement of panic paths:
  • Many .expect/.unwrap replaced by map_err/? across core examples and tests (examples/*, many tests). Validate that error conversions preserve context (avoid losing original error causes) and that examples return appropriate Result types. (Examples: examples/{echo,packet_enum,ping_pong,multi_packet,async_stream}.rs) — see PR summary.
• Lint/test infra changes that affect CI:
  • Cargo.toml dependency bumps (tracing-subscriber 0.3.18, static_assertions 1.1.0), clippy.toml stricter thresholds, Makefile adds RUSTDOC_FLAGS and runs cargo doc before clippy — verify CI passes and that docs build step doesn't break non-doc builds. (Files: Cargo.toml, clippy.toml, Makefile) — see PR summary.
• Test consolidation recommendations (already noted in PR comments):
  • Centralize duplicated TestResult alias across tests to tests/common; split oversized test files (e.g., tests/fragment_transport.rs). These are actionable follow-ups rather than blockers. (Comments summary in PR objectives)

Tools used:

• Ref_ref_search_documentation (searched for PR and repo-relevant docs/dep info)

⏰ 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 (31)

src/server/connection.rs (6)

14-14: LGTM – import aligns with the hooks refactor.

The import correctly brings PreambleHooks from the runtime module to support the unified hook parameter.

---

18-46: LGTM – hooks parameter simplifies the API.

The refactored signature bundles the three hook parameters into a single PreambleHooks<T> structure, improving ergonomics. Panic handling correctly emits to both log and tracing as documented in the comment.

---

48-75: LGTM – error handling appropriate for task boundary.

The function correctly destructures PreambleHooks, delegates to the new helpers, and logs connection errors with warn!. Since this runs in a spawned task, logging rather than further propagating the error is the correct approach.

---

77-95: LGTM – timeout helper correctly wraps preamble read.

The read_preamble_with_timeout function properly handles both the timed and untimed cases, returning a consistent error type. The timeout_error constructor correctly wraps the timeout condition as a bincode::error::DecodeError.

---

97-129: LGTM – hook helpers cleanly separate concerns.

Both run_preamble_success and run_preamble_failure correctly handle optional handlers and log errors appropriately. The use of let-chains at lines 103-104 is valid for Rust 2024 edition and improves readability.

---

160-160: LGTM – test error handling follows guidelines.

Replaced unwrap() with expect() providing a clear failure message, as per the coding guidelines for test code.

examples/ping_pong.rs (2)

9-10: LGTM on structured logging imports.

The imports for TcpListener, signal, and tracing macros align with the broader migration to structured logging across examples.

---

148-151: Clippy suppression is correctly scoped and documented.

The #[expect] attribute with a clear reason for the tokio::select! macro expansion is appropriate. As per coding guidelines, lint suppressions must be narrowly scoped with a reason.

examples/packet_enum.rs (2)

83-87: LGTM on Result-based build_app signature.

The function now returns wireframe::app::Result<App> and propagates errors via ?, aligning with the PR objective to replace panics with explicit error handling.

---

94-129: Well-structured async main with proper error propagation and shutdown handling.

The implementation correctly:

• Initialises the tracing subscriber before any logging
• Wraps the app in Arc for concurrent access
• Handles address parsing with descriptive error messages
• Uses tokio::select! for graceful shutdown
• Logs connection failures and signal errors explicitly

This aligns with the broader PR migration toward Tokio-based, error-propagating server models.

src/app/connection.rs (6)

29-33: LGTM on purge_expired helper.

The explicit if let pattern correctly addresses the past review concern about using .map() solely for side effects.

---

38-48: LGTM on FrameHandlingContext with documentation.

The struct bundles related parameters as per coding guidelines, and the doc comment addresses the past review request.

---

147-176: LGTM on handle_connection_result implementation.

The new public method correctly propagates errors via io::Result<()>, enabling callers to handle connection failures explicitly. The lifecycle hooks (setup/teardown) are preserved.

---

178-189: LGTM on delegating handle_connection to handle_connection_result.

The existing method now wraps the new result-returning variant and logs errors, maintaining backward compatibility whilst enabling explicit error handling for new callers.

---

294-311: LGTM on centralised handle_decode_failure helper.

The helper consolidates duplicate deserialisation failure logic with proper doc comment, metric tracking, and threshold-based connection termination. This addresses the past review feedback.

---

252-283: LGTM on FrameHandlingContext destructuring and ResponseContext usage.

The destructuring pattern is clear, and the use of ResponseContext aligns with the frame_handling module's API for centralised response forwarding.

examples/metadata_routing.rs (2)

44-62: LGTM on safe header parsing with bounds checking.

The get/try_into pattern with explicit error types (UnexpectedEof, InvalidData) correctly replaces potential panics from direct indexing. The flags byte validation (lines 52-56) prevents panics on truncated input.

---

70-108: LGTM on comprehensive error propagation in main.

All .expect() calls are replaced with map_err(io::Error::other)?, ensuring errors propagate cleanly. The use of handle_connection_result aligns with the new API, and the double ? on line 108 correctly unwraps both the JoinError and the inner io::Result.

tests/fragment_transport.rs (8)

30-31: Good use of shared TestResult from common module.

Importing TestResult from common addresses the PR comments recommendation to centralise the duplicated type alias across tests.

---

33-63: LGTM on comprehensive TestError enum with From implementations.

The error enum covers all test failure modes with thiserror derivation, and the From<SendError<T>> impl enables ergonomic ? usage. This aligns with the coding guideline to use semantic error enums with thiserror.

---

68-79: LGTM on fragmentation_config with overflow protection.

The saturating_mul(16) addresses the past review concern about potential overflow when deriving message_limit.

---

188-225: LGTM on extracted helper functions.

build_envelopes, assert_handler_observed, and read_response_payload consolidate repeated patterns and add timeout guards as recommended in past reviews.

---

227-254: LGTM on run_round_trip_test extraction.

This helper eliminates duplication between fragmented and unfragmented round-trip tests, addressing the past review recommendation. The timeout-guarded helpers ensure tests fail fast rather than hang.

---

362-398: LGTM on mutate_malformed_header with #[expect] attribute.

The clippy::panic_in_result_fn suppression is correctly scoped to the function and includes a clear reason. The implementation uses get_mut with explicit error handling for the empty case (lines 387-395).

---

463-512: LGTM on completed round-trip verification in fragmentation_can_be_disabled_via_public_api.

The test now reads the server response (lines 500-504) and asserts it is unfragmented (lines 505-508), addressing the past review concern about incomplete verification when fragmentation is disabled.

---

1-513: File exceeds 400-line guideline (513 lines).

Issue #401 exists to track splitting this file into focused modules. Ensure the decomposition moves shared helpers to tests/common or a dedicated mod helpers.

tests/middleware_order.rs (5)

15-17: Use shared TestResult from common in this test crate

Import the shared TestResult alias to align this test with the common test infrastructure and avoid per-file result aliases. This keeps error typing consistent across integration tests.

---

58-63: Keep panic_in_result_fn lint expectation narrow and documented

Scope the #[expect(clippy::panic_in_result_fn, ...)] attribute to this single test and document the rationale that asserts provide clearer diagnostics. This matches the linting guidelines and keeps panic usage explicit and justified in tests.

---

78-88: Propagate serialisation, I/O, and join errors via ? into TestResult

Use ? on serialize, write_all, shutdown, read_to_end, and the spawned handle.await?? to propagate both join and inner errors into TestResult. This removes implicit panic paths and stays consistent with the PR’s Result-based error handling strategy.

---

90-95: Use slice destructuring instead of redundant ok_or_else

Destructure frames.as_slice() as [first] and return a single, clear error when the response frame count is not exactly one. This removes the previously unreachable ok_or_else branch and matches the earlier review guidance on this test.

---

98-104: Assert payload and correlation id instead of manually returning errors

Use assert_eq! for the payload and correlation_id checks so failures report expected vs actual values with proper test diagnostics, while letting the test’s TestResult handle other fallible steps. This addresses the earlier nitpick about manual if-based error returns.

[codescene-delta-analysis]

Gates Passed
6 Quality Gates Passed

See analysis details in CodeScene

Absence of Expected Change Pattern

• wireframe/src/preamble.rs is usually changed with: wireframe/src/message.rs

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.