Refactor message assembly test API; multiplexing tests#418
Conversation
|
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. WalkthroughAdd keyed, multiplexed message assembly and continuity validation: new error/status enums, FrameSequence checked increment, MessageSeries for per-message sequencing, MessageAssemblyState for keyed multi-message assembly, types for first-frame/assembled messages, public re‑exports, documentation, unit tests and BDD feature/steps. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MessageAssemblyState
participant MessageSeries
participant ErrorHandler
Client->>MessageAssemblyState: accept_first_frame(FirstFrameInput)
activate MessageAssemblyState
MessageAssemblyState->>MessageAssemblyState: validate header, metadata, body sizes
alt Single-frame final
MessageAssemblyState-->>Client: return AssembledMessage
else Multi-frame start
MessageAssemblyState->>MessageSeries: MessageSeries::from_first_frame(header)
MessageAssemblyState->>MessageAssemblyState: store partial keyed by MessageKey
MessageAssemblyState-->>Client: return None
end
deactivate MessageAssemblyState
Client->>MessageAssemblyState: accept_continuation_frame(ContinuationFrameHeader, body)
activate MessageAssemblyState
MessageAssemblyState->>MessageSeries: lookup by MessageKey -> accept_continuation(header)
activate MessageSeries
alt Key mismatch
MessageSeries-->>ErrorHandler: KeyMismatch
MessageSeries-->>MessageAssemblyState: Err(MessageSeriesError)
else Sequence issue (duplicate/out-of-order/overflow)
MessageSeries-->>ErrorHandler: SequenceMismatch/DuplicateFrame/SequenceOverflow
MessageSeries-->>MessageAssemblyState: Err(MessageSeriesError)
else Valid continuation
MessageSeries->>MessageSeries: validate_and_advance_sequence()
MessageSeries-->>MessageAssemblyState: Ok(MessageSeriesStatus)
end
deactivate MessageSeries
alt Continuation completed series
MessageAssemblyState->>MessageAssemblyState: assemble final body
MessageAssemblyState-->>Client: return AssembledMessage
else Still incomplete
MessageAssemblyState->>MessageAssemblyState: accumulate body
MessageAssemblyState-->>Client: return None
end
deactivate MessageAssemblyState
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (3)📚 Learning: 2026-01-03T17:44:28.792ZApplied to files:
📚 Learning: 2026-01-08T08:16:31.989ZApplied to files:
📚 Learning: 2026-01-08T20:33:24.570ZApplied to files:
🔍 Remote MCP DeepwikiSummary of additional repository context relevant to this PR review
Sources
⏰ 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)
🔇 Additional comments (5)
✏️ Tip: You can disable this entire section by setting Comment |
Reviewer's GuideRefactors the message assembly test and public API to support protocol-level multiplexing with new MessageSeries and MessageAssemblyState types, associated error handling, and comprehensive unit/Cucumber tests and docs for interleaved, validated message frame assembly. Sequence diagram for multiplexed message assembly with MessageAssemblyStatesequenceDiagram
actor Protocol
participant MessageAssemblyState
participant MessageSeries_key1 as MessageSeries_key1
participant MessageSeries_key2 as MessageSeries_key2
participant AssembledMessage
Note over Protocol,MessageAssemblyState: Start first frame for message key 1
Protocol->>MessageAssemblyState: accept_first_frame(FirstFrameInput{key=1, is_last=false})
activate MessageAssemblyState
MessageAssemblyState->>MessageSeries_key1: from_first_frame(FirstFrameHeader{key=1})
MessageAssemblyState-->>Protocol: Ok(None)
deactivate MessageAssemblyState
Note over Protocol,MessageAssemblyState: Interleaved first frame for message key 2
Protocol->>MessageAssemblyState: accept_first_frame(FirstFrameInput{key=2, is_last=false})
activate MessageAssemblyState
MessageAssemblyState->>MessageSeries_key2: from_first_frame(FirstFrameHeader{key=2})
MessageAssemblyState-->>Protocol: Ok(None)
deactivate MessageAssemblyState
Note over Protocol,MessageAssemblyState: Continuation for key 1 completes message 1
Protocol->>MessageAssemblyState: accept_continuation_frame(ContinuationFrameHeader{key=1, seq=1, is_last=true}, body)
activate MessageAssemblyState
MessageAssemblyState->>MessageSeries_key1: accept_continuation(header)
MessageSeries_key1-->>MessageAssemblyState: Ok(Complete)
MessageAssemblyState->>AssembledMessage: new(key=1, metadata, body)
MessageAssemblyState-->>Protocol: Ok(Some(AssembledMessage))
deactivate MessageAssemblyState
Note over Protocol,MessageAssemblyState: Continuation for key 2 keeps assembly in progress
Protocol->>MessageAssemblyState: accept_continuation_frame(ContinuationFrameHeader{key=2, seq=1, is_last=false}, body)
activate MessageAssemblyState
MessageAssemblyState->>MessageSeries_key2: accept_continuation(header)
MessageSeries_key2-->>MessageAssemblyState: Ok(Incomplete)
MessageAssemblyState-->>Protocol: Ok(None)
deactivate MessageAssemblyState
Note over Protocol,MessageAssemblyState: Optional cleanup of expired assemblies
Protocol->>MessageAssemblyState: purge_expired()
activate MessageAssemblyState
MessageAssemblyState-->>Protocol: Vec<MessageKey> (evicted keys)
deactivate MessageAssemblyState
Class diagram for MessageSeries and MessageAssemblyState multiplexing APIclassDiagram
class MessageKey {
+u64 0
}
class FrameSequence {
+u32 0
+checked_increment() Option~FrameSequence~
}
class FirstFrameHeader {
+MessageKey message_key
+usize metadata_len
+usize body_len
+Option~usize~ total_body_len
+bool is_last
}
class ContinuationFrameHeader {
+MessageKey message_key
+Option~FrameSequence~ sequence
+usize body_len
+bool is_last
}
class MessageSeries {
-MessageKey message_key
-Option~FrameSequence~ next_sequence
-SequenceTracking sequence_tracking
-bool complete
-Option~usize~ expected_total
+from_first_frame(header FirstFrameHeader) MessageSeries
+message_key() MessageKey
+is_complete() bool
+expected_total() Option~usize~
+accept_continuation(header ContinuationFrameHeader) Result~MessageSeriesStatus, MessageSeriesError~
+force_next_sequence_for_tests(next FrameSequence) void
}
class SequenceTracking {
<<enumeration>>
Untracked
Tracked
}
class MessageSeriesStatus {
<<enumeration>>
Incomplete
Complete
}
class MessageSeriesError {
<<enumeration>>
KeyMismatch
SequenceMismatch
SeriesComplete
SequenceOverflow
MissingFirstFrame
DuplicateFrame
}
class MessageAssemblyError {
<<enumeration>>
Series
DuplicateFirstFrame
MessageTooLarge
}
class FirstFrameInput~'a~ {
+&'a FirstFrameHeader header
+Vec~u8~ metadata
+&'a [u8] body
+new(header &'a FirstFrameHeader, metadata Vec~u8~, body &'a [u8]) FirstFrameInput~'a~
}
class AssembledMessage {
-MessageKey message_key
-Vec~u8~ metadata
-Vec~u8~ body
+new(message_key MessageKey, metadata Vec~u8~, body Vec~u8~) AssembledMessage
+message_key() MessageKey
+metadata() &[u8]
+body() &[u8]
+into_body() Vec~u8~
}
class PartialAssembly {
<<internal>>
-MessageSeries series
-Vec~u8~ metadata
-Vec~u8~ body_buffer
-Instant started_at
+new(series MessageSeries, started_at Instant) PartialAssembly
+push_body(data &[u8]) void
+set_metadata(data Vec~u8~) void
+accumulated_len() usize
}
class MessageAssemblyState {
-NonZeroUsize max_message_size
-Duration timeout
-HashMap~MessageKey, PartialAssembly~ assemblies
+new(max_message_size NonZeroUsize, timeout Duration) MessageAssemblyState
+accept_first_frame(input FirstFrameInput) Result~Option~AssembledMessage~, MessageAssemblyError~
+accept_first_frame_at(input FirstFrameInput, now Instant) Result~Option~AssembledMessage~, MessageAssemblyError~
+accept_continuation_frame(header ContinuationFrameHeader, body &[u8]) Result~Option~AssembledMessage~, MessageAssemblyError~
+accept_continuation_frame_at(header ContinuationFrameHeader, body &[u8], now Instant) Result~Option~AssembledMessage~, MessageAssemblyError~
+purge_expired() Vec~MessageKey~
+purge_expired_at(now Instant) Vec~MessageKey~
+buffered_count() usize
}
MessageSeries --> MessageKey
MessageSeries --> FrameSequence
MessageSeries --> SequenceTracking
MessageSeries ..> MessageSeriesStatus
MessageSeries ..> MessageSeriesError
MessageSeriesStatus <.. MessageAssemblyState
MessageSeriesError <.. MessageAssemblyError
MessageAssemblyError --> MessageKey
MessageAssemblyError --> NonZeroUsize
FirstFrameInput --> FirstFrameHeader
FirstFrameInput --> MessageKey
AssembledMessage --> MessageKey
PartialAssembly --> MessageSeries
PartialAssembly --> AssembledMessage
MessageAssemblyState --> PartialAssembly
MessageAssemblyState --> MessageKey
MessageAssemblyState --> MessageAssemblyError
MessageAssemblyState --> MessageSeries
MessageAssemblyState --> AssembledMessage
FirstFrameHeader --> MessageKey
ContinuationFrameHeader --> MessageKey
ContinuationFrameHeader --> FrameSequence
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: Code DuplicationWhat lead to degradation?src/message_assembler/tests.rs: tests/steps/message_assembly_steps.rs: Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
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: tests/steps/message_assembly_steps.rs Comment on file //! Step definitions for message assembly multiplexing and continuity validation.
❌ New issue: Primitive Obsession |
This comment was marked as resolved.
This comment was marked as resolved.
…or message assembly - Introduced MessageSeries type to track sequence ordering per message key - Created MessageAssemblyState to manage concurrent assemblies keyed by MessageKey - Added error types for key mismatch, sequence mismatch, duplicates, and overflow - Supported optional sequence validation when protocol sequence numbers are present - Implemented unit and cucumber BDD tests covering multiplexing, ordering, duplicates, size limits, and timeouts - Updated message assembler module exports and lib.rs for new public API - Documented new functionality in users-guide.md and marked roadmap items 8.2.3 and 8.2.4 as complete Enables protocol-level multiplexed message assembly with strict continuity checks on interleaved frame streams, providing robust frame ordering, duplicate detection, and expiry handling for streamed requests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…rtions Refactor tests in message_assembler and message_assembly_steps to reduce duplication by introducing helper functions for frame headers and assertion checks. Simplify continuation frame creation in message_assembler tests by using helper functions. Improve step assertion readability in message_assembly_steps by introducing a common assert_condition helper function, reducing boilerplate error handling. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
force-pushed
the
terragon/feature/8-2-3-message-key-support-7qvxt8
branch
from
278c62f to
6b49f17
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Excess Number of Function ArgumentsWhat lead to degradation?tests/worlds/message_assembly.rs: MessageAssemblyWorld.add_first_frame tests/worlds/message_assembly.rs: MessageAssemblyWorld.accept_continuation Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Primitive ObsessionWhat lead to degradation?tests/steps/message_assembly_steps.rs: tests/worlds/message_assembly.rs: Why does this problem occur?Code that uses a high degree of built-in, primitives such as integers, strings, floats, lacks a domain language that encapsulates the validation and semantics of function arguments. Primitive Obsession has several consequences: 1) In a statically typed language, the compiler will detect less erroneous assignments. 2) Security impact since the possible value range of a variable/argument isn't retricted. How to fix it?Primitive Obsession indicates a missing domain language. Introduce data types that encapsulate the details and constraints of your domain. For example, instead of int userId, consider User clicked. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# primitive-obsession.ts
-// Problem: It's hard to know what this function does, and what values are valid as parameters.
-function getPopularRepositories(String baseURL, String query, Integer pages, Integer pageSize, String sortorder): Json {
- let pages == null ? 10 : pages
- let pageSize == null ? 10 : pageSize
- return httpClient.get(`${baseURL}?q=${query}&pages=${pages}&pageSize=${pageSize}&sortorder=${sortorder}`)
+// Refactoring: extract the pagination & API logic into a class, and it will
+// attract validation and other logic related to the specific query. It's now
+// easier to use and to maintain the getPopularRepositories function!
+function getPopularRepositories(query: PaginatedRepoQuery): Json {
+ return httpClient.get(query.getURL())
.map(json => json.repositories)
.filter(repository => repositry.stargazersCount > 1000)
} |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Excess Number of Function Argumentssrc/message_assembler/state.rs: MessageAssemblyState.accept_first_frame_at What lead to degradation?MessageAssemblyState.accept_first_frame_at has 5 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Add FirstFrameInput parameter object to production code and FirstFrameParams/ContinuationFrameParams to test code, reducing function arguments and eliminating primitive obsession. - Add FirstFrameInput<'a> struct that borrows header/body, owns metadata - Refactor accept_first_frame and accept_first_frame_at to use it - Remove #[expect(clippy::too_many_arguments)] from state.rs - Add FirstFrameParams with builder methods (.with_metadata(), .final_frame()) - Add ContinuationFrameParams with builder methods (.with_sequence(), .final_frame()) - Push primitive-to-domain-type conversion to Cucumber step boundary - Update all unit tests and step definitions to use new APIs 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
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: tests/steps/message_assembly_steps.rs Comment on file //! Step definitions for message assembly multiplexing and continuity validation.
❌ New issue: Primitive Obsession |
This comment was marked as resolved.
This comment was marked as resolved.
![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 8 issues, and left some high level feedback:
- The exec plan in
docs/execplans/8-2-3-message-key-support.mdstill hasStatus: IN PROGRESSand many unchecked items inProgresseven though this PR appears to complete most of them; consider updating the status and checklist so the plan accurately reflects the implemented work. - In
MessageAssemblyWorld, pending first frames are stored in aVecand consumed withpop(), which makes acceptance LIFO; if future scenarios depend on FIFO semantics, it may be clearer and less error-prone to switch to aVecDequeand pop from the front instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The exec plan in `docs/execplans/8-2-3-message-key-support.md` still has `Status: IN PROGRESS` and many unchecked items in `Progress` even though this PR appears to complete most of them; consider updating the status and checklist so the plan accurately reflects the implemented work.
- In `MessageAssemblyWorld`, pending first frames are stored in a `Vec` and consumed with `pop()`, which makes acceptance LIFO; if future scenarios depend on FIFO semantics, it may be clearer and less error-prone to switch to a `VecDeque` and pop from the front instead.
## Individual Comments
### Comment 1
<location> `src/message_assembler/series.rs:195-204` </location>
<code_context>
+ Ok(MessageSeriesStatus::Incomplete)
+ }
+
+ fn validate_and_advance_sequence(
+ &mut self,
+ incoming: FrameSequence,
+ ) -> Result<(), MessageSeriesError> {
+ match self.sequence_tracking {
+ SequenceTracking::Untracked => {
+ // First continuation with a sequence number; start tracking
+ self.sequence_tracking = SequenceTracking::Tracked;
+ self.next_sequence = incoming.checked_increment();
+ if self.next_sequence.is_none() && !self.complete {
+ return Err(MessageSeriesError::SequenceOverflow { last: incoming });
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Sequence overflow handling incorrectly fails even for the final frame.
In `validate_and_advance_sequence`, `incoming.checked_increment()` overflow causes `SequenceOverflow` whenever `self.next_sequence` is `None` and `!self.complete`. But `self.complete` is only set after `accept_continuation` calls this helper, so a final frame with `sequence == FrameSequence(u32::MAX)` is incorrectly rejected as overflow.
To fix this, either:
- pass `header.is_last` into `validate_and_advance_sequence` and only treat overflow as an error when another frame is expected, or
- move the overflow check to `accept_continuation`, after deciding completion, and skip the error when the current frame is final.
This preserves `u32::MAX` as a valid final sequence while still detecting real overflow when more frames should follow.
</issue_to_address>
### Comment 2
<location> `src/message_assembler/state.rs:249` </location>
<code_context>
+ }
+
+ // Validate body size
+ if input.body.len() > self.max_message_size.get() {
+ return Err(MessageAssemblyError::MessageTooLarge {
+ key,
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Size checks on first frames ignore declared total length and metadata size.
`accept_first_frame_at` only checks `input.body.len()`. If `header.total_body_len` is present and already exceeds `max_message_size`, we should reject immediately instead of buffering any data. The guard also ignores `metadata.len()`, even though it contributes to the per-message memory footprint. Tightening the check to use `total_body_len` when available and to optionally include metadata in the accounting would make the limit more accurate and avoid starting assemblies that can never succeed.
Suggested implementation:
```rust
// Validate message size (prefer declared total body length and include metadata)
let declared_body_len = input
.header
.total_body_len
.unwrap_or_else(|| input.body.len());
let total_message_size = declared_body_len
.saturating_add(input.metadata.len());
if total_message_size > self.max_message_size.get() {
return Err(MessageAssemblyError::MessageTooLarge {
key,
attempted: total_message_size,
limit: self.max_message_size,
});
}
let series = MessageSeries::from_first_frame(input.header);
```
Depending on the exact types in your codebase, you may need to adjust:
1. `header.total_body_len`:
- If this is not `Option<usize>`, adapt the unwrap to match the type, e.g.:
- `total_body_len.map(|n| n as usize).unwrap_or_else(...)` or
- `total_body_len.get()` if it’s a `NonZeroUsize`.
2. `input.metadata.len()`:
- If metadata is stored differently (e.g. `Option<Bytes>` or length in the header), replace `input.metadata.len()` with the appropriate expression (e.g. `input.metadata.as_ref().map_or(0, |m| m.len())` or `input.header.metadata_len`).
3. If `MessageTooLarge::attempted` is not `usize`, cast `total_message_size` to the expected type (e.g. `total_message_size as u64`).
</issue_to_address>
### Comment 3
<location> `tests/steps/message_assembly_steps.rs:103-108` </location>
<code_context>
+ world.accept_continuation(ContinuationFrameParams::new(to_key(key), body.into_bytes()))
+}
+
+#[when(expr = "another first frame for key {int} with body {string} arrives")]
+fn when_another_first_frame(world: &mut MessageAssemblyWorld, key: u64, body: String) {
+ world.add_first_frame(FirstFrameParams::new(to_key(key), body.into_bytes()));
+ // Ignore result, we check error in Then
+ let _ = world.accept_first_frame();
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid discarding the result in the "another first frame" step to keep assertions precise
This step currently calls `accept_first_frame()` and discards the `TestResult` with `let _ = ...`. Because `accept_first_frame` mutates `last_result`, Then steps that assert on errors still work, but any failure in `accept_first_frame` itself would not fail the step, and this step’s signature is inconsistent with the others that return `TestResult`. Consider returning and propagating the `TestResult` here (e.g. `fn when_another_first_frame(...) -> TestResult { world.add_first_frame(...); world.accept_first_frame() }`) so infrastructure errors surface and the cucumber steps stay consistent.
```suggestion
+#[when(expr = "another first frame for key {int} with body {string} arrives")]
+fn when_another_first_frame(
+ world: &mut MessageAssemblyWorld,
+ key: u64,
+ body: String,
+) -> TestResult {
+ world.add_first_frame(FirstFrameParams::new(to_key(key), body.into_bytes()));
+ world.accept_first_frame()
+}
```
</issue_to_address>
### Comment 4
<location> `src/message_assembler/state.rs:40` </location>
<code_context>
+/// let input = FirstFrameInput::new(&header, vec![0x01, 0x02], b"hello");
+/// assert_eq!(input.header.message_key, MessageKey(1));
+/// ```
+#[derive(Debug)]
+pub struct FirstFrameInput<'a> {
+ /// The frame header.
</code_context>
<issue_to_address>
**issue (review_instructions):** Add both unit tests and behavioural (Cucumber) tests that exercise MessageAssemblyState, MessageSeries, and FrameSequence::checked_increment; the new functionality is currently untested.
Add unit tests under `src/message_assembler/tests.rs` that cover the new message-key multiplexing and continuity behaviour: e.g. single-message assembly, interleaved assemblies for multiple `MessageKey`s, continuation without a first frame, duplicate first frames, size-limit enforcement, timeout purging, and sequence-order validation (including duplicates, gaps, and overflow via `force_next_sequence_for_tests`).
In addition, add the required behavioural coverage: implement scenarios in `tests/features/message_assembly.feature` along with corresponding worlds and step definitions in `tests/worlds/message_assembly.rs` and `tests/steps/message_assembly_steps.rs`. Ensure these are wired into the Cucumber runner so that the new protocol-level assembly behaviour is exercised end-to-end through the existing harness.
<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.
- Bug fixes *must* be demonstrated by a test.
</details>
</issue_to_address>
### Comment 5
<location> `src/message_assembler/state.rs:157` </location>
<code_context>
+/// };
+///
+/// let mut state =
+/// MessageAssemblyState::new(NonZeroUsize::new(1024).unwrap(), Duration::from_secs(30));
+///
+/// // Start assembly for key 1
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The example uses `unwrap()` instead of the preferred `.expect()` for `NonZeroUsize::new(1024)`.
To follow the "Prefer `.expect()` over `.unwrap()`" guideline, please change this example to use `.expect()` with a brief message, e.g.:
```rust
let mut state = MessageAssemblyState::new(
NonZeroUsize::new(1024).expect("non-zero message size"),
Duration::from_secs(30),
);
```
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Prefer `.expect()` over `.unwrap()`
</details>
</issue_to_address>
### Comment 6
<location> `src/message_assembler/state.rs:169` </location>
<code_context>
+/// };
+/// let result = state.accept_first_frame(FirstFrameInput::new(&first, vec![0x01, 0x02], b"hello"));
+/// assert!(result.is_ok());
+/// assert!(result.unwrap().is_none()); // Not yet complete
+///
+/// // Complete with continuation
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The example asserts on `result.unwrap()` instead of using `.expect()` as required by the guidelines.
To comply with the "Prefer `.expect()` over `.unwrap()`" rule, consider rewriting this assertion to avoid a bare `unwrap`, for example:
```rust
let msg = result.expect("first frame processing should succeed");
assert!(msg.is_none()); // Not yet complete
```
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Prefer `.expect()` over `.unwrap()`
</details>
</issue_to_address>
### Comment 7
<location> `docs/users-guide.md:458` </location>
<code_context>
+};
+
+let mut state = MessageAssemblyState::new(
+ NonZeroUsize::new(1_048_576).unwrap(), // 1 MiB max message
+ Duration::from_secs(30), // 30s timeout for partial assemblies
+);
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym “MiB” is used without being defined on first use.
To follow the documentation guideline about defining uncommon acronyms, consider expanding this comment to something like `// 1 mebibyte (MiB) max message` so the meaning of MiB is clear at first use.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 8
<location> `docs/execplans/8-2-3-message-key-support.md:139` </location>
<code_context>
+
+- Unit tests use rstest for parameterised cases
+ (`src/message_assembler/tests.rs`).
+- Cucumber BDD tests in `tests/features/message_assembler.feature`.
+- Test helper `TestAssembler` in `src/test_helpers.rs`.
+- World/steps in `tests/worlds/message_assembler.rs` and
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym “BDD” is introduced without being defined on first use.
To comply with the acronym rule, please expand this to something like “Cucumber Behaviour-Driven Development (BDD) tests” the first time BDD appears, and then use BDD afterwards.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9763e4d45e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/message_assembler/tests.rs (1)
1-677: Decompose this file to comply with the 400-line limit.This file contains 677 lines, exceeding the coding guideline maximum of 400 lines. Split the test modules into separate files:
- Keep header parsing tests in this file
- Move
series_teststotests/series_tests.rs- Move
state_teststotests/state_tests.rsUpdate the module structure with:
#[cfg(test)] mod header_tests; #[cfg(test)] mod series_tests; #[cfg(test)] mod state_tests;Or use a subdirectory structure (
tests/message_assembler/*.rs) if further organisation is needed.As per coding guidelines, files exceeding 400 lines must be decomposed.
🤖 Fix all issues with AI agents
In @docs/users-guide.md:
- Around line 494-548: Add two brief examples to the Continuity validation
section showing how MessageSeries handles duplicate sequences and frames after
completion: create a MessageSeries from a FirstFrameHeader, then call
MessageSeries::accept_continuation with a ContinuationFrameHeader having a
previously-seen FrameSequence to demonstrate
Err(MessageSeriesError::DuplicateFrame { .. }), and call accept_continuation
after a continuation with is_last=true to demonstrate
Err(MessageSeriesError::SeriesComplete { .. }); reference the types
ContinuationFrameHeader, FrameSequence, MessageSeries, accept_continuation, and
the error variants MessageSeriesError::DuplicateFrame and
MessageSeriesError::SeriesComplete so readers can locate the examples.
In @src/message_assembler/error.rs:
- Around line 12-90: The MessageTooLarge error string is grammatically awkward
for limits not equal to one; update the #[error(...)] on
MessageAssemblyError::MessageTooLarge to use consistent pluralization (e.g.,
render the limit as "{limit} bytes" or rephrase to "exceeds size limit:
attempted {attempted} bytes > limit {limit} bytes") so the message reads
correctly for any NonZeroUsize limit; change the annotation for MessageTooLarge
(and keep the field names key, attempted, limit) accordingly.
In @src/message_assembler/series.rs:
- Around line 164-239: The overflow check currently uses self.complete which is
set only after handling the header, causing false SequenceOverflow on a final
continuation whose sequence is max; change validate_and_advance_sequence to take
a boolean is_last (or the header.is_last) and drive the "no further increment
required" check from that flag instead of self.complete: update the call in
accept_continuation to pass header.is_last, and inside
validate_and_advance_sequence replace checks of "!self.complete" with "!is_last"
in both the Untracked and Tracked branches so that an incoming sequence equal to
the overflow sentinel is allowed when is_last is true.
In @src/message_assembler/state.rs:
- Around line 21-60: Change FirstFrameInput::new to validate header.metadata_len
and header.body_len against the provided metadata.len() and body.len() and
return Result<Self, MessageAssemblyError> instead of unconditionally
constructing; if they differ return
MessageAssemblyError::FirstFrameLengthMismatch { expected_metadata:
header.metadata_len, actual_metadata: metadata.len(), expected_body:
header.body_len, actual_body: body.len() } (add that variant in
src/message_assembler/error.rs). Keep the function signature as pub fn
new(header: &'a FirstFrameHeader, metadata: Vec<u8>, body: &'a [u8]) ->
Result<Self, MessageAssemblyError> and only build and return Ok(Self { header,
metadata, body }) when lengths match. Apply the same validation pattern to the
analogous constructor/validator handling first-frame inputs in the 218-275
region.
- Around line 286-361: Ensure continuation frames are rejected when the header's
declared body_len doesn't match the actual payload: in
accept_continuation_frame_at, after obtaining the occupied Entry and before
accumulated size checks (and before push_body), compare header.body_len to
body.len(); if they differ, remove the entry and return a new
MessageAssemblyError::ContinuationBodyLenMismatch containing the key, expected
(header.body_len) and actual (body.len()). Add the
MessageAssemblyError::ContinuationBodyLenMismatch variant in
src/message_assembler/error.rs to support this return path.
- Around line 234-275: In accept_first_frame_at, check
input.header.total_body_len (if Some) and reject immediately when it exceeds
self.max_message_size by returning MessageAssemblyError::MessageTooLarge with
attempted set to that total, instead of only validating input.body.len();
implement this before creating MessageSeries or buffering (i.e., right after
computing key or before starting PartialAssembly) and keep the existing
body-length check as a fallback when total_body_len is None.
- Around line 234-275: The continuation-frame handler currently removes the
PartialAssembly unconditionally on any continuity error; update
accept_continuation_frame_at so that on Err(e) you do NOT call entry.remove()
for DuplicateFrame, SequenceMismatch, or SeriesComplete; specifically: if the
series error is DuplicateFrame, treat it as a harmless retransmit (do not remove
state and return Ok(None) or otherwise ignore the frame), and for
SequenceMismatch and SeriesComplete return Err(MessageAssemblyError::Series(e))
but keep the PartialAssembly in self.assemblies so future retransmits or
timeout-based cleanup can recover; retain the existing removal behavior only
when the continuation successfully completes and you assemble the final
AssembledMessage (or for truly fatal/unrelated errors if any).
In @tests/worlds/message_assembly.rs:
- Around line 1-387: The file exceeds the 400-line guideline because helper
structs FirstFrameParams and ContinuationFrameParams are defined inline; extract
these into a new helper module (e.g., message_assembly_helpers.rs or mod
helpers) and re-export or import them into the test module so
MessageAssemblyWorld stays focused on orchestration; move the impls and their
derives (Debug, methods new/with_metadata/final_frame and
new/with_sequence/final_frame) into the new module, update use/imports so the
test still references FirstFrameParams and ContinuationFrameParams, and ensure
no public API names (FirstFrameParams, ContinuationFrameParams) change to avoid
breaking references in MessageAssemblyWorld.
- Around line 220-239: accept_all_first_frames currently drains
pending_first_frames using pop(), which processes frames LIFO; change it to
drain in FIFO order so queued frames are handled in insertion order. Update
accept_all_first_frames to pull items from the front (e.g., if
pending_first_frames remains a Vec use while
!self.pending_first_frames.is_empty() { let pending =
self.pending_first_frames.remove(0); ... } or better change pending_first_frames
to a VecDeque and use pop_front()) while keeping the existing calls to
state.accept_first_frame_at, pushing to completed_messages, and setting
last_result.
- Around line 155-173: Change create_state to return a cucumber TestResult
instead of panicking: update the signature of create_state to ->
cucumber::TestResult, replace the current panic path (the
NonZeroUsize::new(None) branch) with an early return of a failing TestResult
containing "max_size must be non-zero", and keep the successful path to
initialize MessageAssemblyState, current_time, pending_first_frames,
last_result, completed_messages, and evicted_keys and then return a successful
TestResult; reference the create_state function, NonZeroUsize::new,
MessageAssemblyState::new, and cucumber::TestResult to implement the change.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
docs/execplans/8-2-3-message-key-support.mddocs/roadmap.mddocs/users-guide.mdsrc/lib.rssrc/message_assembler/error.rssrc/message_assembler/header.rssrc/message_assembler/mod.rssrc/message_assembler/series.rssrc/message_assembler/state.rssrc/message_assembler/tests.rstests/cucumber.rstests/features/message_assembly.featuretests/steps/message_assembly_steps.rstests/steps/mod.rstests/world.rstests/worlds/message_assembly.rstests/worlds/mod.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.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:
tests/steps/mod.rstests/worlds/mod.rssrc/message_assembler/header.rstests/world.rstests/steps/message_assembly_steps.rstests/cucumber.rssrc/message_assembler/error.rssrc/message_assembler/mod.rssrc/message_assembler/tests.rssrc/message_assembler/series.rssrc/lib.rssrc/message_assembler/state.rstests/worlds/message_assembly.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:
tests/steps/mod.rstests/worlds/mod.rssrc/message_assembler/header.rstests/world.rstests/steps/message_assembly_steps.rstests/cucumber.rssrc/message_assembler/error.rssrc/message_assembler/mod.rssrc/message_assembler/tests.rssrc/message_assembler/series.rssrc/lib.rssrc/message_assembler/state.rstests/worlds/message_assembly.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/contents.md)
Follow the documentation style guide conventions when writing project documentation
Write official documentation with separate guides for 'Duplex Messaging & Pushes', 'Streaming Responses', and 'Message Fragmentation', each including runnable examples and explaining relevant concepts and APIs
docs/**/*.md: Use markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ('-ize' / '-yse' / '-our'), with the exception of the filenameLICENSE.
Reference the documentation style guide provided atdocs/documentation-style-guide.mdfor documentation writing standards.Documentation describing multi-packet streaming MUST reflect the tuple-based API and explain helper constructors that prepare the channel pair
Replace or retire documentation that refers to Cucumber, including guides and examples in docs/
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization', suffix -lyse in words like 'analyse' and 'paralyse', suffix -our in words like 'colour' and 'behaviour', suffix -re in words like 'calibre' and 'centre', double 'l' in words like 'cancelled' and 'counsellor', maintain the 'e' in words like 'likeable' and 'liveable', suffix -ogue in words like 'analogue' and 'catalogue'
The word 'outwith' is acceptable in documentation
Keep US spelling when used in an API, for example 'color'
Use the Oxford comma: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Lille Industries are expanding'
Write hea...
Files:
docs/users-guide.mddocs/execplans/8-2-3-message-key-support.mddocs/roadmap.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/users-guide.mddocs/execplans/8-2-3-message-key-support.mddocs/roadmap.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/users-guide.mddocs/execplans/8-2-3-message-key-support.mddocs/roadmap.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/users-guide.mddocs/execplans/8-2-3-message-key-support.mddocs/roadmap.md
docs/**/roadmap*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/roadmap*.md: Roadmap task writing: Define outcomes, not intentions. Phrase tasks in terms of the capability delivered (e.g. 'Implement role-based access control for API endpoints'), not aspirations
Roadmap task writing: Quantify completion criteria with measurable finish lines (e.g. '90% test coverage for new modules', 'response times under 200ms')
Roadmap task writing: Break into atomic increments. Ensure tasks can be completed in weeks, not quarters. Large goals should be decomposed into clear, deliverable units
Roadmap task writing: Tie to dependencies and sequencing. Document prerequisites so tasks can be scheduled realistically
Roadmap task writing: Bound scope explicitly. Note both in-scope and out-of-scope elements (e.g. 'Build analytics dashboard (excluding churn prediction)')
Roadmap formatting: Use dotted numbering for phases (1, 2, 3, …), steps (1.1, 1.2, 1.3, …), and headline tasks (1.1.1, 1.1.2, 1.1.3, …)
Roadmap formatting: Precede task and sub-task items with a GitHub-Flavoured Markdown (GFM) checkbox ([]) to track completion status
Roadmap formatting: Note non-linear dependencies explicitly using dotted notation (e.g. 'Requires 2.3.1')
Roadmap formatting: Include explicit success criteria only where not immediately obvious from the task description
Roadmap formatting: Where applicable, cite the relevant design document section for each task (e.g. 'See design-doc.md §3.2')
Files:
docs/roadmap.md
🧠 Learnings (4)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
tests/steps/mod.rstests/worlds/mod.rssrc/message_assembler/header.rstests/world.rstests/steps/message_assembly_steps.rstests/cucumber.rssrc/message_assembler/error.rssrc/message_assembler/mod.rssrc/message_assembler/tests.rssrc/message_assembler/series.rssrc/lib.rssrc/message_assembler/state.rstests/worlds/message_assembly.rs
📚 Learning: 2026-01-08T08:16:31.989Z
Learnt from: leynos
Repo: leynos/wireframe PR: 411
File: src/client/tests/lifecycle.rs:0-0
Timestamp: 2026-01-08T08:16:31.989Z
Learning: In Rust, apply #[expect(lint, reason = "...")] only to code that actually triggers the specified lint. If the code does not trigger the lint, Clippy will fail with 'unfulfilled lint expectation' errors, which is intentional to prevent stale suppressions. Do not add #[expect] attributes proactively or to code that doesn't trigger the lint, even if similar code elsewhere requires suppression. This guideline should be applied broadly to Rust source files across the repository.
Applied to files:
tests/steps/mod.rstests/worlds/mod.rssrc/message_assembler/header.rstests/world.rstests/steps/message_assembly_steps.rstests/cucumber.rssrc/message_assembler/error.rssrc/message_assembler/mod.rssrc/message_assembler/tests.rssrc/message_assembler/series.rssrc/lib.rssrc/message_assembler/state.rstests/worlds/message_assembly.rs
📚 Learning: 2026-01-08T20:33:24.570Z
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: src/codec/error_tests.rs:152-152
Timestamp: 2026-01-08T20:33:24.570Z
Learning: Guideline: In Rust, when using contains with a single-character pattern, prefer a char literal (e.g., 'a') over a string literal (e.g., "a") to satisfy the Clippy lint single_char_pattern, which flags single-character strings as less efficient. Apply this across Rust source files (all .rs) to improve efficiency and lint compliance. Example: use s.contains('a') instead of s.contains("a"); note that str::contains can take a char, making the char literal both correct and more efficient.
Applied to files:
tests/steps/mod.rstests/worlds/mod.rssrc/message_assembler/header.rstests/world.rstests/steps/message_assembly_steps.rstests/cucumber.rssrc/message_assembler/error.rssrc/message_assembler/mod.rssrc/message_assembler/tests.rssrc/message_assembler/series.rssrc/lib.rssrc/message_assembler/state.rstests/worlds/message_assembly.rs
📚 Learning: 2026-01-08T18:03:59.395Z
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: tests/steps/codec_error_steps.rs:58-62
Timestamp: 2026-01-08T18:03:59.395Z
Learning: In the wireframe repo, test step functions that always succeed should not return TestResult, because clippy::unnecessary_wraps will fire and cannot be suppressed in tests. Do not use #[allow(...)] or #[expect(clippy::unnecessary_wraps)] to silence it. Refactor such steps to return () or a type that does not trigger the lint, or move these steps to non-test code or a helper that isn’t linted the same way. This guidance applies to all files under tests/steps that contain similar patterns.
Applied to files:
tests/steps/mod.rstests/steps/message_assembly_steps.rs
🧬 Code graph analysis (4)
src/message_assembler/error.rs (1)
src/message_assembler/header.rs (4)
from(13-13)from(17-17)from(29-29)from(33-33)
src/message_assembler/tests.rs (2)
src/message_assembler/series.rs (5)
from_first_frame(96-104)is_complete(112-112)accept_continuation(164-193)message_key(108-108)expected_total(116-116)src/message_assembler/header.rs (1)
new(127-127)
src/message_assembler/series.rs (4)
src/message_assembler/header.rs (1)
header(152-152)src/message_assembler/state.rs (1)
message_key(122-122)tests/worlds/message_assembly.rs (1)
accept_continuation(250-269)tests/worlds/correlation.rs (1)
expected(45-45)
tests/worlds/message_assembly.rs (3)
src/message_assembler/header.rs (4)
fmt(21-21)fmt(37-37)new(127-127)header(152-152)src/message_assembler/state.rs (7)
new(53-59)new(72-79)new(112-118)new(198-204)body(130-130)metadata(126-126)accept_first_frame(218-223)src/message_assembler/series.rs (1)
accept_continuation(164-193)
🪛 LanguageTool
docs/users-guide.md
[uncategorized] ~444-~444: Possible missing comma found.
Context: ...ageKey`. This enables interleaved frame streams where frames from different logical mes...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/execplans/8-2-3-message-key-support.md
[uncategorized] ~13-~13: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...mbler` module needs multiplexing support so protocol implementations can process in...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~91-~91: Would you like to use the Oxford spelling “recognize”? The spelling ‘recognise’ is also correct.
Context: ...with transport-level fragmentation will recognise the analogous protocol-level structur...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~137-~137: Would you like to use the Oxford spelling “parameterized”? The spelling ‘parameterised’ is also correct.
Context: ... patterns: - Unit tests use rstest for parameterised cases (`src/message_assembler/tests.r...
(OXFORD_SPELLING_Z_NOT_S)
[style] ~216-~216: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mbly.rswithMessageAssemblyWorld. - Create tests/steps/message_assembly_steps.rs`...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~295-~295: Loose punctuation mark.
Context: ...leaved assembly pass. - make check-fmt, make lint, and make test exit with ...
(UNLIKELY_OPENING_PUNCTUATION)
🔍 Remote MCP
The repository structure is not available through Deepwiki at this moment. However, based on the detailed AI-generated summaries and PR objectives provided, I can synthesize the relevant review context:
Summary of Additional Review Context
Core Feature Architecture
New Modules Introduced:
src/message_assembler/error.rs: Defines error types (MessageSeriesError,MessageAssemblyError,MessageSeriesStatus) for protocol-level validation failures including key mismatches, sequence violations, duplicates, and size constraintssrc/message_assembler/series.rs: Per-message ordering tracker (MessageSeries) that handles optional sequence validation with two modes: Untracked (starts on first sequence) and Tracked (strict ordering enforcement)src/message_assembler/state.rs: Multiplexing state manager (MessageAssemblyState) that manages concurrent in-flight assemblies keyed byMessageKey, enforces size limits, handles timeouts, and returns completeAssembledMessageinstances
Key Design Aspects:
- Optional sequence tracking (skipped if sequence numbers absent) enables backward compatibility with unsequenced frames
- Per-key isolation prevents interleaved messages from interfering with one another
- Timeout-based purging removes stale assemblies; size enforcement prevents resource exhaustion
Test Coverage
Unit Tests (src/message_assembler/tests.rs):
- 488 new lines covering: sequential/non-sequential continuations, key mismatches, out-of-order sequences, duplicates, post-completion behavior, overflow detection, and single-frame completion
- Test helpers for frame header construction and payload building
Cucumber/BDD Tests (tests/features/message_assembly.feature):
- 78 lines of scenario-driven tests with 8 scenarios covering multiplexing, continuity, and error conditions
- Test world (
MessageAssemblyWorld) manages state transitions and error inspection
Public API Surface Changes
New exports at crate root (src/lib.rs): AssembledMessage, FirstFrameInput, MessageAssemblyState, MessageAssemblyError, MessageSeries, MessageSeriesError, MessageSeriesStatus
Related production type (FirstFrameInput): Groups header, metadata, and body as a cohesive parameter object—addressing the PR review feedback on argument count reduction
Code Review Findings
Duplication in Tests:
src/message_assembler/tests.rs: RepeatedFirstFrameHeader/ContinuationFrameHeaderconstruction across series teststests/steps/message_assembly_steps.rs: Repeated assertion patterns in Then steps- Mitigation in PR: Commit 2 introduces test helpers and
assert_conditionto reduce boilerplate
Primitive Obsession in Test Code:
tests/steps/message_assembly_steps.rsandtests/worlds/message_assembly.rsuse primitives (usize, u64) instead of domain types (MessageKey,FrameSequence)- Mitigation in PR: Commit 3 adds conversion helpers (
to_key,to_seq) at step boundary and introducesFirstFrameParams,ContinuationFrameParamsparameter objects
Function Argument Count:
MessageAssemblyState::accept_first_frame_at: 5 parameters- Test world methods originally exceeded threshold
- Mitigation:
FirstFrameInput<'a>in production (commit 3) and parameter objects in tests reduce counts and improve cohesion
[::Source: AI-generated summaries from pull request context::]
- Core feature definitions and error type hierarchies: src/message_assembler/{error,series,state}.rs summaries
- Test coverage details: src/message_assembler/tests.rs and tests/cucumber.rs summaries
- Public API and re-exports: src/lib.rs and module structure summaries
- Code quality recommendations and mitigations: PR comments and commit messages summaries
⏰ 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 (20)
src/message_assembler/header.rs (1)
40-56: LGTM! Well-implemented overflow-safe increment.The implementation correctly uses
checked_addfor overflow handling, includes comprehensive doctest examples, and follows the single-line function style guideline. The#[must_use]attribute ensures callers handle theOptionresult.tests/worlds/mod.rs (1)
39-39: LGTM! Module declaration follows conventions.The new
message_assemblymodule is declared consistently with the existing test world modules.tests/steps/mod.rs (1)
14-14: LGTM! Step module declaration is correct.The new
message_assembly_stepsmodule follows the established pattern for Cucumber step definitions.docs/roadmap.md (1)
284-286: LGTM! Roadmap accurately reflects completed work.The completion of tasks 8.2.3 (message key support) and 8.2.4 (continuity validation) aligns with the implemented
MessageSeriesandMessageAssemblyStatemodules in this PR.tests/cucumber.rs (3)
4-4: LGTM! Documentation updated correctly.The module-level documentation accurately reflects the addition of
MessageAssemblyWorldas the thirteenth test suite, with proper description and feature file mapping.Also applies to: 11-11, 29-29
54-54: LGTM! Import follows conventions.The
MessageAssemblyWorldimport is correctly positioned and consistent with the existing pattern.
69-69: LGTM! Test world execution wired correctly.The
MessageAssemblyWorld::runcall is properly sequenced and references the correct feature file.tests/features/message_assembly.feature (2)
1-7: Feature and Background are clear; scenarios comprehensively exercise multiplexing, ordering, and lifecycle management.The feature description correctly frames protocol-level multiplexing above transport fragmentation. The Background establishes shared state (max size 1024, timeout 30 seconds), and Scenario 8 overrides max size to 10 to test bounds. All 8 scenarios map directly to design goals: single-message assembly, interleaved assembly, sequence validation, duplicate detection, missing first frame, size limits, and timeout-based eviction.
35-42: Scenario 4 step phrasing could be clearer: "expecting 2 but found 3" assumes internal state visibility.The error assertion at line 41 references expected vs. found sequence. Ensure the step definition parses this format and the world provides adequate error inspection. If possible, extract the expected/found values in the step language to avoid brittle string matching (e.g., "the error reports a sequence mismatch expecting 2 but found 3").
docs/users-guide.md (1)
441-492: Section 8.2.3 clearly demonstrates per-key multiplexing with a well-designed example.The code example shows two interleaved first frames (keys 1 and 2) and continuation handling. The pattern is sound: construct state with size/timeout bounds, feed first and continuation frames, unwrap/inspect results. One minor point: the example uses
expect()for error handling; add a note that production code should propagate errors via?rather than panic on framework errors.Verify that the API signatures shown (e.g.,
state.accept_first_frame(&first1, vec![], b"hello")and return typeOption<AssembledMessage>) align with the actual production implementation insrc/message_assembler/state.rs.docs/execplans/8-2-3-message-key-support.md (4)
11-41: Purpose and Constraints are clearly defined; success criteria are observable and testable.The problem statement correctly frames protocol-level multiplexing above transport fragmentation. Success criteria enumerate new types, test coverage, documentation, and roadmap updates. Constraints are practical (400-line file limit, no new dependencies, integration with existing patterns). No issues detected.
144-224: Plan of Work stages are logically sequenced and deliverables are clearly specified.Stages A–H provide a clear progression from header extension through error types, series/state management, exports, tests, and documentation. Test scenarios (Stage F) cover single-message assembly, interleaving, ordering violations, duplicates, missing frames, size/timeout constraints—comprehensive. Cucumber integration wiring (cucumber.rs, worlds/mod.rs, steps/mod.rs) is mentioned. One note: the Progress checklist shows [x] only for the draft and [ ] for all subsequent tasks; ensure this is updated as work advances to provide visibility into completion status.
323-377: Formal API specifications are comprehensive and include time-variant methods for testability.The error taxonomy covers all foreseen failure modes (sequence mismatch, duplicates, missing frames, overflow, size limits). MessageSeries and MessageAssemblyState provide factories, lifecycle methods, and time-variant accessors (_at variants) for deterministic testing. AssembledMessage includes ownership-transfer (
into_body) and borrowing accessors. The API design is sound and mirrors transport-level fragmentation patterns for consistency. Ensure implementation signatures strictly follow these specs to avoid user breakage.
52-61: Risks section identifies sequence tracking ambiguity and test coverage gaps; mitigations are sound.Optional sequence tracking (when
ContinuationFrameHeader::sequenceisNone) is correctly identified as a risk, with mitigation to skip validation. Overflow detection andforce_next_sequence_for_tests()helper are specified to address underspecified edge cases. Both mitigations are implementable and reduce risk to acceptable levels.src/message_assembler/tests.rs (1)
199-226: Helper functions successfully reduce test duplication.The introduction of
first_header,continuation_header, andcontinuation_header_untrackedhelpers effectively addresses the code duplication concern raised in the PR comments. This improves test maintainability and readability.src/message_assembler/mod.rs (1)
1-22: Clear module documentation with feature references.The module-level documentation effectively explains the purpose of
MessageAssemblyState(multiplexing) andMessageSeries(continuity validation), with clear references to specific protocol features (8.2.3, 8.2.4). The structure and examples are well-organised.tests/steps/message_assembly_steps.rs (2)
8-21: Effective helper functions reduce primitive obsession.The
to_key,to_seq, andassert_conditionhelpers successfully address the concerns raised in PR comments:
- Primitive-to-domain conversion is pushed to the step boundary (lines 9, 12)
- Common assertion pattern is extracted (lines 14-21)
This improves maintainability and aligns with the refactoring objectives outlined in commit 2.
Based on PR objectives and commit messages.
129-132: Appropriate use of#[expect]attributes.The
#[expect(clippy::needless_pass_by_value)]attributes are correctly applied and justified. Cucumber hands{string}captures to step functions as ownedStringvalues, making the suppression necessary and the reason clear.As per coding guidelines and learnings regarding
#[expect]usage.Also applies to: 148-151
src/lib.rs (1)
80-95: Public API expansion is well-organised.The new re-exports (
AssembledMessage,FirstFrameInput,MessageAssemblyError,MessageAssemblyState,MessageSeries,MessageSeriesError,MessageSeriesStatus) are appropriately surfaced at the crate root. The alphabetical ordering within the list is maintained, and the additions align with the PR objectives for exposing message assembly functionality.tests/worlds/message_assembly.rs (1)
246-269: Keep #[expect] truthful; delete it if clippy stops emitting the lint.Run clippy and ensure the
clippy::needless_pass_by_valueexpectation is fulfilled; otherwise clippy fails with an unfulfilled expectation. Based on learnings, never leave stale#[expect]behind.#!/bin/bash # Run the same clippy invocation used by CI (adjust if your Makefile differs). make lint
leynos
changed the title
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/message_assembler/state.rs (1)
1-471: File exceeds 400-line limit; decompose into smaller modules.At 471 lines, this file exceeds the 400-line limit per coding guidelines. Extract
FirstFrameInputandFirstFrameInputErrorinto a separateinput.rsmodule, or moveAssembledMessagetooutput.rs.🔧 Suggested decomposition
Create
src/message_assembler/input.rs:Move lines 23-104 (FirstFrameInput, FirstFrameInputError) to a new input.rs module. Update mod.rs to re-export from input. Update state.rs to import from super::input.Alternatively, create
src/message_assembler/output.rs:Move lines 132-179 (AssembledMessage) to a new output.rs module. Update mod.rs to re-export from output. Update state.rs to import from super::output.
🤖 Fix all issues with AI agents
In @docs/execplans/8-2-3-message-key-support.md:
- Around line 74-77: Remove or retire the Cucumber-specific checklist entries
and replace them with the preferred testing guidance: remove references to
tests/features/message_assembly.feature, tests/worlds/message_assembly.rs,
tests/steps/message_assembly_steps.rs and the registration note for
tests/cucumber.rs and tests/worlds/mod.rs, and either mark these Cucumber
artifacts as retired or replace the items with instructions to create equivalent
integration tests using the project's standard test harness (e.g., Rust
integration tests or the approved framework) and link to the preferred testing
guide; ensure the checklist wording reflects retirement or the new testing
approach.
- Around line 99-102: The "Decision: Detect duplicates by comparing incoming
sequence against expected" log entry exceeds 80 columns; reflow that decision
sentence and any following rationale into lines wrapped at 80 characters or less
while preserving content and the Date/Author metadata (e.g., the line containing
"Decision: Detect duplicates..." and "Date/Author: 2026-01-07 (Codex)"). Ensure
each prose line breaks at word boundaries to maintain readability and keep the
original wording and intent unchanged.
- Around line 3-5: The introductory paragraph in the ExecPlan document exceeds
the 80-column wrap guideline; reflow that paragraph to maximum 80 characters per
line (soft-wrap) while leaving the surrounding headings and any code blocks
unwrapped, making sure line breaks occur at word boundaries so the paragraph
reads naturally.
In @src/message_assembler/state_tests.rs:
- Around line 164-189: Extract a parameterised rstest to consolidate the two
size-limit tests: replace the tests state_enforces_size_limit_on_first_frame and
state_enforces_size_limit_on_continuation with a single rstest that takes
parameters (case name, initial first-frame header/body sizes, optional
continuation size, expected attempted size, and expected buffered_count after
rejection); in the test instantiate MessageAssemblyState::new with the small
limit, construct inputs using FirstFrameInput::new and ContinuationInput::new as
needed, call accept_first_frame and optionally accept_continuation on the
MessageAssemblyState, assert the returned error matches
MessageAssemblyError::MessageTooLarge with the expected attempted size and that
the state's buffered_count is 0; reference the existing symbols
MessageAssemblyState, accept_first_frame, accept_continuation, FirstFrameInput,
ContinuationInput, and MessageAssemblyError when editing.
In @tests/steps/message_assembly_steps.rs:
- Around line 58-66: The two step functions when_first_frame_accepted and
when_first_frame_accepted_at_time_t are duplicates; consolidate by removing one
and annotating the remaining function with both step attributes (e.g., add a
second #[when("the first frame is accepted at time T")] above fn
when_first_frame_accepted) so a single function (when_first_frame_accepted)
handles both steps and keeps the body calling world.accept_first_frame().
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
docs/execplans/8-2-3-message-key-support.mdsrc/message_assembler/error.rssrc/message_assembler/series_tests.rssrc/message_assembler/state.rssrc/message_assembler/state_tests.rssrc/message_assembler/tests.rstests/features/message_assembly.featuretests/steps/message_assembly_steps.rstests/worlds/message_assembly.rstests/worlds/message_assembly_params.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/message_assembler/tests.rssrc/message_assembler/state_tests.rstests/steps/message_assembly_steps.rssrc/message_assembler/series_tests.rssrc/message_assembler/error.rstests/worlds/message_assembly_params.rstests/worlds/message_assembly.rssrc/message_assembler/state.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/message_assembler/tests.rssrc/message_assembler/state_tests.rstests/steps/message_assembly_steps.rssrc/message_assembler/series_tests.rssrc/message_assembler/error.rstests/worlds/message_assembly_params.rstests/worlds/message_assembly.rssrc/message_assembler/state.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/contents.md)
Follow the documentation style guide conventions when writing project documentation
Write official documentation with separate guides for 'Duplex Messaging & Pushes', 'Streaming Responses', and 'Message Fragmentation', each including runnable examples and explaining relevant concepts and APIs
docs/**/*.md: Use markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ('-ize' / '-yse' / '-our'), with the exception of the filenameLICENSE.
Reference the documentation style guide provided atdocs/documentation-style-guide.mdfor documentation writing standards.Documentation describing multi-packet streaming MUST reflect the tuple-based API and explain helper constructors that prepare the channel pair
Replace or retire documentation that refers to Cucumber, including guides and examples in docs/
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization', suffix -lyse in words like 'analyse' and 'paralyse', suffix -our in words like 'colour' and 'behaviour', suffix -re in words like 'calibre' and 'centre', double 'l' in words like 'cancelled' and 'counsellor', maintain the 'e' in words like 'likeable' and 'liveable', suffix -ogue in words like 'analogue' and 'catalogue'
The word 'outwith' is acceptable in documentation
Keep US spelling when used in an API, for example 'color'
Use the Oxford comma: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Lille Industries are expanding'
Write hea...
Files:
docs/execplans/8-2-3-message-key-support.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/execplans/8-2-3-message-key-support.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/execplans/8-2-3-message-key-support.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/execplans/8-2-3-message-key-support.md
🧠 Learnings (4)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
src/message_assembler/tests.rssrc/message_assembler/state_tests.rstests/steps/message_assembly_steps.rssrc/message_assembler/series_tests.rssrc/message_assembler/error.rstests/worlds/message_assembly_params.rstests/worlds/message_assembly.rssrc/message_assembler/state.rs
📚 Learning: 2026-01-08T08:16:31.989Z
Learnt from: leynos
Repo: leynos/wireframe PR: 411
File: src/client/tests/lifecycle.rs:0-0
Timestamp: 2026-01-08T08:16:31.989Z
Learning: In Rust, apply #[expect(lint, reason = "...")] only to code that actually triggers the specified lint. If the code does not trigger the lint, Clippy will fail with 'unfulfilled lint expectation' errors, which is intentional to prevent stale suppressions. Do not add #[expect] attributes proactively or to code that doesn't trigger the lint, even if similar code elsewhere requires suppression. This guideline should be applied broadly to Rust source files across the repository.
Applied to files:
src/message_assembler/tests.rssrc/message_assembler/state_tests.rstests/steps/message_assembly_steps.rssrc/message_assembler/series_tests.rssrc/message_assembler/error.rstests/worlds/message_assembly_params.rstests/worlds/message_assembly.rssrc/message_assembler/state.rs
📚 Learning: 2026-01-08T20:33:24.570Z
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: src/codec/error_tests.rs:152-152
Timestamp: 2026-01-08T20:33:24.570Z
Learning: Guideline: In Rust, when using contains with a single-character pattern, prefer a char literal (e.g., 'a') over a string literal (e.g., "a") to satisfy the Clippy lint single_char_pattern, which flags single-character strings as less efficient. Apply this across Rust source files (all .rs) to improve efficiency and lint compliance. Example: use s.contains('a') instead of s.contains("a"); note that str::contains can take a char, making the char literal both correct and more efficient.
Applied to files:
src/message_assembler/tests.rssrc/message_assembler/state_tests.rstests/steps/message_assembly_steps.rssrc/message_assembler/series_tests.rssrc/message_assembler/error.rstests/worlds/message_assembly_params.rstests/worlds/message_assembly.rssrc/message_assembler/state.rs
📚 Learning: 2026-01-08T18:03:59.395Z
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: tests/steps/codec_error_steps.rs:58-62
Timestamp: 2026-01-08T18:03:59.395Z
Learning: In the wireframe repo, test step functions that always succeed should not return TestResult, because clippy::unnecessary_wraps will fire and cannot be suppressed in tests. Do not use #[allow(...)] or #[expect(clippy::unnecessary_wraps)] to silence it. Refactor such steps to return () or a type that does not trigger the lint, or move these steps to non-test code or a helper that isn’t linted the same way. This guidance applies to all files under tests/steps that contain similar patterns.
Applied to files:
tests/steps/message_assembly_steps.rs
🔍 Remote MCP
Let me try a web search to understand the broader context of message assembly and multiplexing patterns:Based on the context provided and the PR content analysis, I have gathered sufficient information to provide focused review guidance. The PR introduces message assembly with multiplexing and continuity validation patterns similar to protocol standards like ISO-TP. Let me provide a summary of the key review insights:
Summary of Key Review Findings for PR #418
Core Design & Architecture
The PR introduces a three-layer message assembly system addressing interleaved multi-message scenarios:
- MessageAssemblyState (Orchestrator) – Manages per-key partial assemblies with constraints (max size, timeout-based purging)
- MessageSeries (Per-Message Tracker) – Handles frame sequencing, ordering validation, and completion detection for individual message streams
- Error Taxonomy – Comprehensive error types (
MessageSeriesError,MessageAssemblyError,FirstFrameInputError) with detailed context
Protocol Pattern Alignment
The implementation reflects established patterns from ISO-TP (ISO 15765-2), which is an international standard for sending data packets over a CAN bus allowing transport of messages that exceed the maximum payload of CAN frames by segmenting longer messages into multiple frames and adding metadata that allows reassembly. Key parallels:
- Frame sequencing – Sequence numbers start at 1 and increment with each frame sent, allowing detection of lost or discarded frames
- Continuation validation – Checks for key consistency, ordering, overflow, and duplicates
- Timeout-based cleanup – Purges incomplete assemblies after configurable duration
Critical Review Points
API Design Feedback (from comments):
- Excess parameters: Methods like
accept_first_frame_at()and world methodsadd_first_frame(),accept_continuation()exceed 4-argument thresholds. The PR already introducesFirstFrameInputas a parameter object for production code, which is appropriate for state.rs. Test APIs should follow similar patterns. - Primitive obsession: Cucumber step definitions accept primitives directly. While this is a framework artifact, converting primitives at step boundaries (e.g.,
to_message_key(),to_frame_sequence()) into domain types improves type safety and readability. - Test boilerplate: Both unit tests and step definitions have repeated header construction patterns. Extracting helpers (e.g.,
FirstFrameHeader::default_for_tests()) reduces duplication.
Validation Coverage:
- The PR validates continuation body length via
ContinuationBodyLengthMismatcherror, addressing a critical data integrity concern - Error handling distinguishes recoverable (assembly retained) vs. unrecoverable (assembly removed) failures
- Overflow protection on
FrameSequence::checked_increment()prevents wrap-around vulnerabilities
Test Completeness:
- Unit tests (
series_tests.rs,state_tests.rs) cover sequential, out-of-order, duplicate, and timeout scenarios - Cucumber feature file exercises multiplexing across interleaved streams and error conditions
- Single-frame completion optimization (immediate assembly) is validated
Public API Exports
Re-exports at crate root (src/lib.rs) appropriately surface MessageAssemblyState, MessageSeries, error types, and AssembledMessage. This reduces client boilerplate and aligns with ergonomic design goals.
Recommended Validation Steps
- Verify
purge_expired()correctness under concurrent timestamp scenarios - Confirm size limit enforcement applies consistently to both first frames and continuations
- Validate that assembly state is fully cleaned on unrecoverable errors (no dangling metadata or partial buffers)
- Test interleaved assembly with realistic frame timing variations
🔇 Additional comments (35)
docs/execplans/8-2-3-message-key-support.md (1)
1-382: Structure and content are sound.The execution plan comprehensively documents the feature scope, constraints, risks, and staged implementation approach. The decision log captures key architectural choices with rationale.
src/message_assembler/error.rs (3)
1-19: Well-structured error module with appropriate derives.The module doc comment explains the purpose clearly.
MessageSeriesStatusis correctly minimal withClone,Copy, and the equality traits. Usingthiserroraligns with project conventions.
21-83: Comprehensive error variants with contextual data.
MessageSeriesErrorcovers the full spectrum of continuity validation failures: key mismatches, sequence issues, completion state, overflow, and the newContinuationBodyLengthMismatch. Each variant carries sufficient context for diagnostics.
85-110: Assembly error hierarchy is well-designed.The
#[from]attribute onSeriesenables ergonomic?propagation from series-level errors.DuplicateFirstFrameandMessageTooLargecorrectly surface assembly-specific failures with actionable context.src/message_assembler/tests.rs (1)
190-203: Clean module organization for expanded test coverage.Using
#[path = "..."]to wire inseries_testsandstate_testsfrom dedicated files keeps this module under the 400-line limit whilst maintaining logical grouping. The banner comments aid navigation.tests/features/message_assembly.feature (3)
1-79: New Cucumber scaffolding conflicts with coding guidelines.The coding guidelines state: "Replace or retire documentation that refers to Cucumber, including guides and examples in docs/". Adding new Cucumber feature files contradicts this directive. Clarify whether Cucumber is being retained for this feature or migrate these scenarios to Rust-native tests using
rstestparameterisation.If Cucumber is intentionally retained for BDD coverage, update the coding guidelines to reflect the current policy. Otherwise, convert these scenarios to parameterised unit tests.
35-42: Scenario correctly validates recoverable error handling.The "Out-of-order continuation is rejected but assembly retained" scenario verifies that sequence mismatches do not discard the partial assembly, allowing recovery. This aligns with the error handling strategy described in the PR objectives.
64-70: Size limit scenario correctly overrides background state.Overriding the background's max size to 10 bytes in this scenario isolates the size-limit behaviour without affecting other scenarios. The assertion on
buffered count is 0confirms the assembly is removed on unrecoverable errors.src/message_assembler/state_tests.rs (7)
1-25: Module documentation and helper function are appropriate.The module doc comment identifies the test scope.
state_with_defaults()centralises common setup, reducing repetition across tests.
27-57: Single message assembly test is thorough.Validates the complete lifecycle: first frame acceptance, buffering, continuation acceptance, completion, and buffer cleanup. Assertions cover message key, metadata, body content, and buffered count.
59-116: Interleaved assembly test validates multiplexing.This test exercises the core multiplexing capability: two concurrent assemblies with interleaved frame acceptance and independent completion. The buffered count assertions at each stage confirm correct state management.
118-162: Error variant matching is clear and correct.The
matches!macro usage forMissingFirstFrameandDuplicateFirstFramevariants is idiomatic and readable.
231-260: Time-based purging test correctly usesaccept_first_frame_atandpurge_expired_at.Using explicit timestamps enables deterministic testing of expiry behaviour without relying on wall-clock time. The 31-second advance past a 30-second timeout correctly triggers eviction.
262-284: Single-frame completion test validates immediate return path.This confirms that messages with
is_last: trueon the first frame complete immediately without buffering, avoiding unnecessary state allocation.
286-291: ConfirmAssembledMessage::newis part of the public API.
AssembledMessage::newis called directly in this test. Establish whether this constructor is exported as public API or is internal-only. If internal, use an alternative construction path or mark it#[doc(hidden)].tests/steps/message_assembly_steps.rs (4)
1-21: Well-structured boundary adapters and assertion helper.The
to_keyandto_seqfunctions correctly convert primitives to domain types at the step boundary, addressing the primitive obsession concerns. Theassert_conditionhelper effectively reduces boilerplate in Then steps.
27-52: Given steps correctly use parameter objects and domain type conversion.The steps consistently use
FirstFrameParamswith its builder methods and convert primitives viato_keyat the boundary.
125-178: Then steps for result verification are well-implemented.The steps correctly use the
assert_conditionhelper with descriptive error messages. The#[expect]attributes include appropriate reason strings for the Cucumber string capture pattern.
180-245: Error condition steps are consistent and provide good diagnostics.All error-checking steps use
assert_conditionwith informative error messages that include the actual error viaworld.last_error()for debugging failed assertions.src/message_assembler/series_tests.rs (3)
1-40: Test helpers effectively reduce header construction boilerplate.The
first_header,continuation_header, andcontinuation_header_untrackedhelpers address the code duplication flagged in review comments whilst keeping test setup concise.
42-110: Sequential and ordering tests provide comprehensive coverage.The tests correctly exercise the happy path, untracked continuations, key mismatch, and out-of-order sequence scenarios. The use of
.expect()andmatches!aligns with coding guidelines.
112-200: Edge case tests are thorough and well-structured.The tests cover duplicate sequence detection, post-completion rejection, sequence overflow at
u32::MAX, expected total storage, and single-frame message completion. Theforce_next_sequence_for_testshelper enables controlled overflow testing.tests/worlds/message_assembly_params.rs (2)
1-44: FirstFrameParams builder is well-designed.The parameter object addresses the excess arguments concern with a fluent builder pattern. Public fields enable direct access in tests, and
#[must_use]correctly annotates builder methods.
46-84: ContinuationFrameParams builder is consistent and correct.The default sequence of 1 aligns with protocol expectations where the first continuation frame starts at sequence 1. The builder pattern is consistent with
FirstFrameParams.src/message_assembler/state.rs (6)
1-21: Module documentation is comprehensive and imports are well-organised.The module-level doc comment clearly explains the purpose of
MessageAssemblyStateand its role in managing in-flight assemblies.
23-104: FirstFrameInput validation and error handling are exemplary.The constructor validates header lengths against payloads, returning semantic errors via
thiserror. The rustdoc example demonstrates correct usage.
106-130: PartialAssembly is a clean internal helper.The internal struct encapsulates per-key assembly state with minimal surface area. Single-line method bodies follow coding guidelines.
132-179: AssembledMessage provides a clean API for completed messages.The struct correctly derives common traits, provides const accessors where possible, and offers
into_bodyfor zero-copy body extraction.
240-328: First-frame acceptance logic is well-structured.The
accept_first_frame_atmethod correctly validates size limits usingsaturating_add, handles single-frame messages with early return, and stores partial assemblies for multi-frame messages.
356-442: Continuation frame handling correctly distinguishes recoverable from unrecoverable errors.The error categorisation (lines 391-403) retains assemblies for potentially recoverable errors (duplicate frame, sequence mismatch, series complete) whilst removing assemblies for unrecoverable errors. The size limit enforcement uses
checked_addto prevent overflow.tests/worlds/message_assembly.rs (5)
1-66: World struct and custom Debug implementation are well-designed.The
VecDequefor pending frames enables FIFO semantics as documented. The customDebugimplementation avoids verbose state dumps whilst preserving diagnostic utility.
68-114: State initialisation and frame queueing are correct.The
create_statemethod appropriately panics on zeromax_size(documented in the doc comment) and resets all state for test isolation. Theadd_first_framemethod correctly constructs headers from parameter objects.
116-163: Frame acceptance methods handle errors gracefully.Both methods correctly validate state initialisation and use safe queue access via
pop_front(). Error mapping provides context for test diagnostics.
165-222: Continuation acceptance, time advancement, and purge methods are correctly implemented.The
#[expect]attribute provides a clear reason for accepting by value. Time manipulation and purge operations correctly interact with the underlying state.
224-317: Query and error inspection methods use safe access patterns throughout.The methods consistently use
map_or,matches!with guards, and iterator methods like.rev().find()- no direct indexing. Error inspection is precise and diagnostic-friendly.
|
@coderabbitai Have the following now been resolved? |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: src/message_assembler/state.rs Comment on lines +356 to +442 pub fn accept_continuation_frame_at(
&mut self,
header: &ContinuationFrameHeader,
body: &[u8],
now: Instant,
) -> Result<Option<AssembledMessage>, MessageAssemblyError> {
self.purge_expired_at(now);
let key = header.message_key;
// Validate header body_len matches actual payload
if header.body_len != body.len() {
return Err(MessageAssemblyError::Series(
MessageSeriesError::ContinuationBodyLengthMismatch {
key,
header_len: header.body_len,
actual_len: body.len(),
},
));
}
let Entry::Occupied(mut entry) = self.assemblies.entry(key) else {
return Err(MessageAssemblyError::Series(
MessageSeriesError::MissingFirstFrame { key },
));
};
// Validate continuity
let status = match entry.get_mut().series.accept_continuation(header) {
Ok(s) => s,
Err(e) => {
// Only remove assembly for unrecoverable errors.
// DuplicateFrame, SequenceMismatch, and SeriesComplete are potentially
// recoverable (retransmits, reordering) - retain assembly for recovery
// or timeout-based cleanup.
match &e {
MessageSeriesError::KeyMismatch { .. }
| MessageSeriesError::SequenceOverflow { .. }
| MessageSeriesError::MissingFirstFrame { .. }
| MessageSeriesError::MissingSequence { .. }
| MessageSeriesError::ContinuationBodyLengthMismatch { .. } => {
entry.remove();
}
MessageSeriesError::DuplicateFrame { .. }
| MessageSeriesError::SequenceMismatch { .. }
| MessageSeriesError::SeriesComplete => {
// Retain assembly for recovery or timeout-based cleanup
}
}
return Err(MessageAssemblyError::Series(e));
}
};
// Check size limit
let accumulated = entry.get().accumulated_len();
let Some(new_len) = accumulated.checked_add(body.len()) else {
entry.remove();
return Err(MessageAssemblyError::MessageTooLarge {
key,
attempted: usize::MAX,
limit: self.max_message_size,
});
};
if new_len > self.max_message_size.get() {
entry.remove();
return Err(MessageAssemblyError::MessageTooLarge {
key,
attempted: new_len,
limit: self.max_message_size,
});
}
entry.get_mut().push_body(body);
match status {
MessageSeriesStatus::Incomplete => Ok(None),
MessageSeriesStatus::Complete => {
let partial = entry.remove();
Ok(Some(AssembledMessage::new(
key,
partial.metadata,
partial.body_buffer,
)))
}
}
}❌ New issue: Large Method |
This comment was marked as resolved.
This comment was marked as resolved.
leynos
changed the title
… size checks - Introduced a method to identify unrecoverable continuity errors to better handle assembly removal. - Added size limit checks when appending message continuation frames to prevent oversized messages. - Improved error handling to remove assemblies only on unrecoverable errors. - Documented the multiplexing behavior of MessageAssemblyState for concurrent message keys with a detailed mermaid sequence diagram. - Enhanced message assembly state management to support multiplexed frame streams keyed by message keys, enabling interleaved processing and timeout cleanup. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @docs/execplans/8-2-3-message-key-support.md:
- Around line 13-17: Add a comma before "so" in the sentence describing
MessageAssembler: update the sentence referencing Wireframe's `MessageAssembler`
to read "Wireframe's `MessageAssembler` module needs multiplexing support, so
protocol implementations can process..." (edit the text containing
`MessageAssembler` to insert the comma).
- Around line 427-431: The snippet for the FrameSequence impl is using indented
code style instead of a fenced Rust code block; update the documentation so the
block is fenced with ```rust before and ``` after, and ensure the content shows
the exact impl FrameSequence { pub fn checked_increment(self) -> Option<Self>; }
declaration (referenced symbols: FrameSequence, checked_increment) so it renders
correctly.
- Around line 280-333: Replace all indented code blocks in the "Concrete Steps"
section with fenced code blocks that include a language identifier (use "shell"
for command lists and "plaintext" where appropriate); specifically, convert the
indented blocks under steps 1–10 (examples showing cat commands, “# Edit …”, and
the make commands) into fenced triple-backtick blocks (e.g., ```shell ... ```)
so the examples in lines mentioning src/message_assembler/header.rs,
src/fragment/series.rs, the checked_increment note, the created files
(message_assembler/error.rs, series.rs, state.rs,
tests/features/message_assembly.feature, etc.), and the quality gates commands
are all fenced and annotated to satisfy markdownlint MD046.
- Around line 378-425: The indented Rust interface examples should be converted
to fenced code blocks with language identifier "rust": replace each indented
block that defines types like MessageSeriesStatus, MessageSeriesError,
MessageSeries, AssembledMessage, MessageAssemblyError, and MessageAssemblyState
with a fenced block starting with ```rust and ending with ```; ensure each block
contains the exact original public signatures (e.g., pub enum
MessageSeriesStatus { Incomplete, Complete }, pub fn from_first_frame(header:
&FirstFrameHeader) -> Self, pub fn accept_continuation(&mut self, header:
&ContinuationFrameHeader) -> Result<MessageSeriesStatus, MessageSeriesError>,
pub fn into_body(self) -> Vec<u8>, pub enum MessageAssemblyError {
Series(MessageSeriesError), DuplicateFirstFrame { key: MessageKey },
MessageTooLarge { key: MessageKey, attempted: usize, limit: NonZeroUsize } },
and pub struct MessageAssemblyState with its listed methods) and apply this
fenced format to all remaining interface definition blocks in the section.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/execplans/8-2-3-message-key-support.mdsrc/message_assembler/state.rs
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit inference engine (docs/contents.md)
Follow the documentation style guide conventions when writing project documentation
Write official documentation with separate guides for 'Duplex Messaging & Pushes', 'Streaming Responses', and 'Message Fragmentation', each including runnable examples and explaining relevant concepts and APIs
docs/**/*.md: Use markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ('-ize' / '-yse' / '-our'), with the exception of the filenameLICENSE.
Reference the documentation style guide provided atdocs/documentation-style-guide.mdfor documentation writing standards.Documentation describing multi-packet streaming MUST reflect the tuple-based API and explain helper constructors that prepare the channel pair
Replace or retire documentation that refers to Cucumber, including guides and examples in docs/
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization', suffix -lyse in words like 'analyse' and 'paralyse', suffix -our in words like 'colour' and 'behaviour', suffix -re in words like 'calibre' and 'centre', double 'l' in words like 'cancelled' and 'counsellor', maintain the 'e' in words like 'likeable' and 'liveable', suffix -ogue in words like 'analogue' and 'catalogue'
The word 'outwith' is acceptable in documentation
Keep US spelling when used in an API, for example 'color'
Use the Oxford comma: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Lille Industries are expanding'
Write hea...
Files:
docs/execplans/8-2-3-message-key-support.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/execplans/8-2-3-message-key-support.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/execplans/8-2-3-message-key-support.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/execplans/8-2-3-message-key-support.md
**/*.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/message_assembler/state.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/message_assembler/state.rs
🧠 Learnings (3)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
src/message_assembler/state.rs
📚 Learning: 2026-01-08T08:16:31.989Z
Learnt from: leynos
Repo: leynos/wireframe PR: 411
File: src/client/tests/lifecycle.rs:0-0
Timestamp: 2026-01-08T08:16:31.989Z
Learning: In Rust, apply #[expect(lint, reason = "...")] only to code that actually triggers the specified lint. If the code does not trigger the lint, Clippy will fail with 'unfulfilled lint expectation' errors, which is intentional to prevent stale suppressions. Do not add #[expect] attributes proactively or to code that doesn't trigger the lint, even if similar code elsewhere requires suppression. This guideline should be applied broadly to Rust source files across the repository.
Applied to files:
src/message_assembler/state.rs
📚 Learning: 2026-01-08T20:33:24.570Z
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: src/codec/error_tests.rs:152-152
Timestamp: 2026-01-08T20:33:24.570Z
Learning: Guideline: In Rust, when using contains with a single-character pattern, prefer a char literal (e.g., 'a') over a string literal (e.g., "a") to satisfy the Clippy lint single_char_pattern, which flags single-character strings as less efficient. Apply this across Rust source files (all .rs) to improve efficiency and lint compliance. Example: use s.contains('a') instead of s.contains("a"); note that str::contains can take a char, making the char literal both correct and more efficient.
Applied to files:
src/message_assembler/state.rs
🧬 Code graph analysis (1)
src/message_assembler/state.rs (1)
src/message_assembler/series.rs (2)
message_key(108-108)from_first_frame(96-104)
🪛 LanguageTool
docs/execplans/8-2-3-message-key-support.md
[uncategorized] ~13-~13: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...mbler` module needs multiplexing support so protocol implementations can process in...
(COMMA_COMPOUND_SENTENCE_2)
[style] ~266-~266: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mbly.rswithMessageAssemblyWorld. - Create tests/steps/message_assembly_steps.rs`...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~345-~345: Loose punctuation mark.
Context: ...leaved assembly pass. - make check-fmt, make lint, and make test exit with ...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.18.1)
docs/execplans/8-2-3-message-key-support.md
280-280: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
286-286: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
291-291: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
296-296: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
301-301: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
307-307: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
312-312: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
316-316: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
323-323: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
328-328: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
378-378: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
429-429: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🔍 Remote MCP Deepwiki
Additional relevant facts to aid review
-
Repository context: wireframe is a Rust framework with actor-per-connection design, length-prefixed framing, bincode serializer, PushHandle/session registry, and a layered testing infra including rstest and Cucumber BDD.
-
Test harness & expectations:
- Project uses extensive automated checks (format, clippy, tests) in CI; PR must pass make fmt, make lint, cargo test. CI enforces zero Clippy warnings and formatting.
- wireframe_testing provides helpers (drive_with_bincode, duplex streams, logger fixture) used by existing tests and new Cucumber world/steps; new tests should follow these patterns.
-
Public API and design surface impacted by this PR:
- crate exposes WireframeServer/WireframeApp, PushHandle, SessionRegistry, FrameProcessor, Serializer; re-exports in lib.rs are central to consumer usage — new message_assembler types re-exported at crate root will be part of the public surface to review for compatibility.
-
Testing expectations for message-assembly changes:
- The project already exercises correlation IDs, streaming responses, fragmentation plans, and prioritized push queues in tests and Cucumber features; new MessageSeries/MessageAssemblyState additions must be covered by unit tests and BDD feature scenarios (the PR adds both). Use wireframe_testing helpers and Cucumber world patterns for consistent behavior.
-
Operational constraints and production concerns relevant to review:
- Wireframe enforces max message size, reassembly timeouts, and per-connection rate limiting (leaky-bucket). Confirm MessageAssemblyState respects and enforces these existing invariants (size/enforced limits, timeout-driven purge semantics) to match project resilience patterns.
Files/areas to focus review on (based on repo docs and PR summary):
- message_assembler/series.rs and state.rs: correctness of sequence tracking, overflow handling, completion transition, and error variants consistency with documented error taxonomy.
- message_assembler/error.rs: clarity and data carried by error enums, Display impls, and conversions used by state logic.
- lib.rs re-exports: ensure intended public API surface and documentation comments.
- Tests: series_tests.rs, state_tests.rs, and Cucumber feature/world/steps additions — verify they exercise edge cases (overflow, missing first frame, duplicate first frame, size limits, expiry) and integrate with wireframe_testing utilities.
References:
- Repository docs and testing/architecture overview used: Deepwiki pages for leynos/wireframe (Overview, Getting Started, Core Architecture, Testing, Roadmap).
⏰ 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 (11)
docs/execplans/8-2-3-message-key-support.md (1)
1-107: Well-structured exec plan with clear acceptance criteria.The document follows a sensible living-document format with constraints, tolerances, risks, progress tracking, and decision log. The Mermaid diagram effectively illustrates interleaved message handling.
src/message_assembler/state.rs (10)
23-104: Well-designed parameter object with validation.
FirstFrameInputcleanly groups frame components and validates header consistency. The error variants provide clear diagnostic information.
106-130: Clean private helper struct.
PartialAssemblyencapsulates assembly state appropriately. Theaccumulated_lenmethod correctly tracks body size, which integrates with the upfront metadata-inclusive size validation inaccept_first_frame_at.
132-179: Clean public API for assembled messages.
AssembledMessageprovides a well-documented interface with appropriate derives and#[must_use]annotations.
181-254: Solid state manager foundation.The struct design with
NonZeroUsizefor the size limit prevents misconfiguration. The doctest provides a clear end-to-end usage example.
284-328: Correct early size validation including declared total.The size check at lines 298–308 correctly prefers
total_body_lenwhen available and includes metadata, ensuring oversized assemblies are rejected upfront. The single-frame fast path is a sensible optimization.
364-390: Robust overflow handling in size check.The use of
checked_addwith a fallback tousize::MAXfor the error is a sound approach to prevent integer overflow exploits.
462-483: Safe timeout handling withsaturating_duration_since.The use of
saturating_duration_sinceguards against clock monotonicity issues. Returning evicted keys enables callers to observe and log timeouts.
485-488: Simple and correct.
347-362: No issues found. BothMissingSequenceandContinuationBodyLengthMismatchexist in theMessageSeriesErrorenum and are correctly referenced in this function. All unrecoverable variants are matched, with recoverable variants (DuplicateFrame,SequenceMismatch,SeriesComplete) correctly omitted.
401-460: Well-structured continuation handling with proper cleanup.The use of
Entry::Occupiedis idiomatic. Assembly removal on unrecoverable errors prevents resource leaks. The body length validation at lines 411–420 adds defensive protection against malformed frames.Sequence overflow on the final frame is handled correctly:
validate_and_advance_sequencetreats overflow as an error only when more frames are expected (!is_last), allowing the message to complete without error when overflow occurs on the final frame.
…acking and validations Introduce multiplexing in the MessageAssembler module to process interleaved frame streams from multiple logical messages. - Add MessageSeries and related status/error types for tracking sequences and continuity (ordering, duplicates, missing frames). - Add MessageAssemblyState to manage concurrent assemblies keyed by MessageKey. - Implement checked_increment() for FrameSequence. - Extract FirstFrameInput and AssembledMessage types into dedicated module to reduce file size. - Add thorough unit and integration tests covering error cases and size limits. - Update documentation to describe the new message key support and assembly process. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/message_assembler/state_tests.rs:
- Around line 29-164: Tests duplicate FirstFrameHeader and
ContinuationFrameHeader construction; add small helper factories to reduce
boilerplate and replace inline constructors with them. Implement two helpers
named first_frame_header(key: u64, metadata_len: usize, body_len: usize,
is_last: bool) -> FirstFrameHeader and continuation_header(key: u64, sequence:
u32, body_len: usize, is_last: bool) -> ContinuationFrameHeader, then update
tests (e.g., state_tracks_single_message_assembly,
state_tracks_multiple_interleaved_messages,
state_rejects_continuation_without_first_frame,
state_rejects_duplicate_first_frame) to call first_frame_header(...) and
continuation_header(...) instead of manually building the structs.
- Around line 22-27: Convert the helper function state_with_defaults() into an
rstest fixture by annotating it with #[fixture] and returning
MessageAssemblyState so tests can accept it as a parameter; update tests that
call state_with_defaults() to take a parameter named state_with_defaults:
MessageAssemblyState (or rename locally) and use that value instead of calling
the function; ensure you add the rstest import (use rstest::rstest and/or
rstest::fixture as needed) so the fixture compiles and retains the same
initialization (NonZeroUsize::new(1024) and Duration::from_secs(30)) used in
MessageAssemblyState::new.
- Around line 166-241: Add an extra optional total_body_len to SizeLimitCase and
a new rstest case that sets total_body_len: Some(value) larger than the
configured limit so the first frame is rejected early; update the construction
of FirstFrameHeader in state_enforces_size_limit to set total_body_len:
params.total_body_len and adjust expected_attempted for the new case to be the
declared total (or the actual attempted amount you expect), keeping the rest of
the test flow the same so accept_first_frame triggers
MessageAssemblyError::MessageTooLarge when total_body_len exceeds the
NonZeroUsize limit.
- Around line 1-303: Add a new unit test that verifies a final continuation
frame with sequence u32::MAX completes: create a MessageAssemblyState, accept a
FirstFrameInput for MessageKey(1) (use FirstFrameHeader/FirstFrameInput) for a
non-final first frame, then build a ContinuationFrameHeader with sequence
Some(FrameSequence(u32::MAX)) and is_last = true and call
accept_continuation_frame; assert it returns Ok(Some(AssembledMessage)) with the
expected key/body and that buffered_count() drops to 0. Use the same helper
types (MessageAssemblyState, FirstFrameHeader, FirstFrameInput,
ContinuationFrameHeader, FrameSequence, MessageKey, accept_first_frame,
accept_continuation_frame) as in existing tests and follow the pattern of
series_accepts_final_fragment_at_max_index().
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docs/execplans/8-2-3-message-key-support.mdsrc/message_assembler/mod.rssrc/message_assembler/state.rssrc/message_assembler/state_tests.rssrc/message_assembler/types.rstests/steps/message_assembly_steps.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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:
tests/steps/message_assembly_steps.rssrc/message_assembler/state_tests.rssrc/message_assembler/types.rssrc/message_assembler/state.rssrc/message_assembler/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:
tests/steps/message_assembly_steps.rssrc/message_assembler/state_tests.rssrc/message_assembler/types.rssrc/message_assembler/state.rssrc/message_assembler/mod.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/contents.md)
Follow the documentation style guide conventions when writing project documentation
Write official documentation with separate guides for 'Duplex Messaging & Pushes', 'Streaming Responses', and 'Message Fragmentation', each including runnable examples and explaining relevant concepts and APIs
docs/**/*.md: Use markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ('-ize' / '-yse' / '-our'), with the exception of the filenameLICENSE.
Reference the documentation style guide provided atdocs/documentation-style-guide.mdfor documentation writing standards.Documentation describing multi-packet streaming MUST reflect the tuple-based API and explain helper constructors that prepare the channel pair
Replace or retire documentation that refers to Cucumber, including guides and examples in docs/
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization', suffix -lyse in words like 'analyse' and 'paralyse', suffix -our in words like 'colour' and 'behaviour', suffix -re in words like 'calibre' and 'centre', double 'l' in words like 'cancelled' and 'counsellor', maintain the 'e' in words like 'likeable' and 'liveable', suffix -ogue in words like 'analogue' and 'catalogue'
The word 'outwith' is acceptable in documentation
Keep US spelling when used in an API, for example 'color'
Use the Oxford comma: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Lille Industries are expanding'
Write hea...
Files:
docs/execplans/8-2-3-message-key-support.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/execplans/8-2-3-message-key-support.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/execplans/8-2-3-message-key-support.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/execplans/8-2-3-message-key-support.md
🧠 Learnings (4)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
tests/steps/message_assembly_steps.rssrc/message_assembler/state_tests.rssrc/message_assembler/types.rssrc/message_assembler/state.rssrc/message_assembler/mod.rs
📚 Learning: 2026-01-08T08:16:31.989Z
Learnt from: leynos
Repo: leynos/wireframe PR: 411
File: src/client/tests/lifecycle.rs:0-0
Timestamp: 2026-01-08T08:16:31.989Z
Learning: In Rust, apply #[expect(lint, reason = "...")] only to code that actually triggers the specified lint. If the code does not trigger the lint, Clippy will fail with 'unfulfilled lint expectation' errors, which is intentional to prevent stale suppressions. Do not add #[expect] attributes proactively or to code that doesn't trigger the lint, even if similar code elsewhere requires suppression. This guideline should be applied broadly to Rust source files across the repository.
Applied to files:
tests/steps/message_assembly_steps.rssrc/message_assembler/state_tests.rssrc/message_assembler/types.rssrc/message_assembler/state.rssrc/message_assembler/mod.rs
📚 Learning: 2026-01-08T20:33:24.570Z
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: src/codec/error_tests.rs:152-152
Timestamp: 2026-01-08T20:33:24.570Z
Learning: Guideline: In Rust, when using contains with a single-character pattern, prefer a char literal (e.g., 'a') over a string literal (e.g., "a") to satisfy the Clippy lint single_char_pattern, which flags single-character strings as less efficient. Apply this across Rust source files (all .rs) to improve efficiency and lint compliance. Example: use s.contains('a') instead of s.contains("a"); note that str::contains can take a char, making the char literal both correct and more efficient.
Applied to files:
tests/steps/message_assembly_steps.rssrc/message_assembler/state_tests.rssrc/message_assembler/types.rssrc/message_assembler/state.rssrc/message_assembler/mod.rs
📚 Learning: 2026-01-08T18:03:59.395Z
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: tests/steps/codec_error_steps.rs:58-62
Timestamp: 2026-01-08T18:03:59.395Z
Learning: In the wireframe repo, test step functions that always succeed should not return TestResult, because clippy::unnecessary_wraps will fire and cannot be suppressed in tests. Do not use #[allow(...)] or #[expect(clippy::unnecessary_wraps)] to silence it. Refactor such steps to return () or a type that does not trigger the lint, or move these steps to non-test code or a helper that isn’t linted the same way. This guidance applies to all files under tests/steps that contain similar patterns.
Applied to files:
tests/steps/message_assembly_steps.rs
🧬 Code graph analysis (4)
src/message_assembler/state_tests.rs (2)
src/message_assembler/state.rs (3)
new(31-38)new(114-120)buffered_count(353-353)src/message_assembler/types.rs (5)
new(69-91)new(118-124)message_key(128-128)metadata(132-132)body(136-136)
src/message_assembler/types.rs (1)
src/message_assembler/state.rs (2)
new(31-38)new(114-120)
src/message_assembler/state.rs (3)
src/message_assembler/types.rs (3)
new(69-91)new(118-124)body(136-136)tests/worlds/message_assembly.rs (4)
new(69-78)accept_first_frame(121-138)purge_expired(213-222)buffered_count(226-230)src/message_assembler/series.rs (1)
from_first_frame(96-104)
src/message_assembler/mod.rs (3)
src/message_assembler/header.rs (1)
header(152-152)tests/worlds/fragment/mod.rs (1)
series(180-184)src/extractor.rs (1)
state(82-90)
🪛 LanguageTool
docs/execplans/8-2-3-message-key-support.md
[style] ~266-~266: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...mbly.rswithMessageAssemblyWorld. - Create tests/steps/message_assembly_steps.rs`...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[uncategorized] ~365-~365: Loose punctuation mark.
Context: ...leaved assembly pass. - make check-fmt, make lint, and make test exit with ...
(UNLIKELY_OPENING_PUNCTUATION)
🔍 Remote MCP Deepwiki
Additional relevant facts for reviewing this PR
-
Project architecture & invariants:
- Wireframe uses an actor-per-connection model; per-connection tasks must be panic-isolated and respect cancellation/timeout semantics ( Overview / Core Architecture).
- The framework already enforces max-message-size, reassembly timeouts, and per-connection rate limiting (leaky-bucket) as core DoS protections — new MessageAssemblyState must integrate with these (size/timeout) invariants ( Production Deployment / Resilience sections).
-
Public API and compatibility surface:
- lib.rs re-exports form the crate public surface used by consumers; adding MessageSeries, MessageAssemblyState, AssembledMessage and error enums makes these types part of the public API and requires stable docs/visibility consideration ( 7 API Reference and API Surface).
-
Testing & CI expectations:
- The repo uses layered testing with rstest unit tests and Cucumber BDD; behavioral features and worlds are executed in CI when the
advanced-testsfeature is enabled — the PR added both unit tests and Cucumber worlds/features which must follow existing test patterns and use wireframe_testing helpers (drive_with_bincode, duplex streams) ( Testing / Testing Utilities). - CI enforces formatting, clippy (zero-warning) and full test runs via make fmt / make lint / cargo test; new public types & docs should pass these gates ( CI/CD and Development Workflow).
- The repo uses layered testing with rstest unit tests and Cucumber BDD; behavioral features and worlds are executed in CI when the
-
Observability & production concerns:
- The project collects operational metrics (connection counts, reassembly errors, frame counts). Message assembly errors (MessageSeriesError / MessageAssemblyError) will be useful to map to existing metrics (e.g., reassembly_errors_total) or logging to preserve operational diagnosability ( Observability and Monitoring / Production Deployment).
-
Testing patterns to follow in review:
- Tests should use existing fixtures and wireframe_testing helpers, avoid duplicating header/frame builder boilerplate (the repo already prefers helpers / rstest fixtures) and use tokio time-pausing where appropriate for deterministic timeout tests ( Testing / Testing Utilities).
Sources:
- Repo docs & architecture (pages listed in wiki): and selected content: Overview, Core Architecture, Production Deployment, Testing, API Reference, CI/CD & Development Workflow ().
⏰ 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 (29)
docs/execplans/8-2-3-message-key-support.md (3)
1-107: Well-structured ExecPlan document.The document follows the expected living-document format with clear success criteria, constraints, tolerances, risks, progress tracking, and decision log. The status is marked COMPLETE, which aligns with the PR objectives.
144-192: Mermaid diagram effectively illustrates multiplexing flow.The sequence diagram clearly shows how
MessageAssemblyStatemanages interleaved frame streams from two concurrent message keys, including the cleanup path viapurge_expired(). The preceding description (lines 144-146) aids accessibility.
394-455: Interfaces section accurately documents the public API.The expected public interfaces match the implementation described in the PR. This serves as a useful reference for validation and acceptance criteria.
tests/steps/message_assembly_steps.rs (4)
8-21: Boundary adapters and assertion helper are well-designed.The
to_keyandto_seqfunctions convert primitives to domain types at the step boundary, addressing the primitive obsession concern raised in PR comments. Theassert_conditionhelper reduces duplication in Then steps whilst maintaining clear error messages.
27-52: Given steps correctly use parameter objects and domain types.The step definitions use
FirstFrameParamsbuilders with fluent API (with_metadata,final_frame), and domain type conversion occurs at the boundary viato_key. These steps appropriately return()since they always succeed, avoiding theclippy::unnecessary_wrapslint.
58-115: When steps correctly propagateTestResult.All When steps return
TestResult, ensuring errors surface immediately. Thewhen_another_first_framestep (lines 99-107) correctly returns the result fromworld.accept_first_frame(), addressing the concern raised in PR comments about discarded results.
129-165: Lint expectations correctly applied with reasons.The
#[expect(clippy::needless_pass_by_value, reason = "...")]attributes onthen_completes_with_bodyandthen_key_completesare correctly scoped with clear reasons explaining the Cucumber framework requirement. This follows the guideline to apply#[expect]only to code that triggers the specific lint.src/message_assembler/types.rs (4)
1-9: Module documentation clearly explains purpose.The module-level comment explains the purpose and notes the extraction from
state.rsto meet the 400-line file limit, providing useful context for maintainers.
31-92:FirstFrameInputparameter object is well-designed.The struct groups related first-frame parameters as suggested in PR comments. The validation in
new()correctly checks that header-declared lengths match actual payload sizes, returning typed errors on mismatch. The ownership model (borrowed header, owned metadata, borrowed body) is appropriate for the use case.
41-60: Error enum follows established patterns.
FirstFrameInputErrorusesthiserrorderive with clear error messages and carries diagnostic fields (header_len,actual_len) enabling callers to inspect mismatch details.
108-141:AssembledMessageprovides clean encapsulation.Private fields with public accessors follow good encapsulation practices. The
#[must_use]attributes on all methods encourage proper handling of return values. Theinto_body()method enables zero-copy ownership transfer when the caller needs the body bytes.src/message_assembler/state.rs (7)
1-19: Module documentation and imports are appropriate.The module-level comment clearly describes the purpose. The imports are well-organised with standard library types first, followed by local module imports.
21-45:PartialAssemblyprovides clean internal encapsulation.The private struct appropriately encapsulates the in-progress assembly state with focused helper methods. The separation from public API is well-designed.
106-120: Constructor uses appropriate type constraints.Using
NonZeroUsizeformax_message_sizeprevents misconfiguration with zero limits at compile time rather than runtime.
150-194: First-frame acceptance logic addresses PR feedback.The implementation prefers
header.total_body_lenwhen present (line 165) and includes metadata in size accounting (line 166), addressing the functional correctness suggestions from PR comments. The single-frame message path correctly returns immediately without buffering.
213-256: Helper extraction improves readability.The
is_unrecoverable_continuity_errorandcheck_size_limithelpers address the PR comment about method size. Theconst fnon the error classifier enables compile-time evaluation. The size check useschecked_addto prevent arithmetic overflow.
267-326: Continuation frame handling is robust.The method validates body length against header (lines 278-286), uses idiomatic Entry API, properly cleans up assemblies on unrecoverable errors, and correctly handles completion status. The helper extractions have reduced complexity compared to the concern raised in PR comments.
328-349: Expiration purging is efficient and safe.Using
retainfor bulk removal is efficient. Thesaturating_duration_sincehandles potential clock anomalies gracefully. Returning evicted keys enables operational logging and metrics.src/message_assembler/mod.rs (3)
7-21: Module documentation clearly describes new features.The updated module-level comments document message key multiplexing (8.2.3) and continuity validation (8.2.4) with references to the relevant types. The bullet points clearly enumerate what the
MessageSeriestype detects.
23-42: Module structure and re-exports are well-organised.The public modules (
error,series,state,types) expose the new functionality, whilstheaderremains private with selected re-exports. The flat re-exports at module level (lines 31-42) provide convenient access without requiring deep imports.
44-121:MessageAssemblertrait and examples remain stable.The trait definition is unchanged, satisfying the ExecPlan constraint that the existing trait signature must not change. The examples demonstrate realistic usage with the
bytescrate.src/message_assembler/state_tests.rs (8)
1-20: Module structure is sound.The module doc comment and imports are correctly organised. All imported types appear necessary for the tests.
29-59: Test logic is correct.The test properly validates the single-message assembly flow, checking completion, body concatenation, and buffer cleanup. The use of
.expect()with descriptive messages follows guidelines.
61-118: Interleaving test is well-constructed.This test validates the core multiplexing capability: two independent message streams processed concurrently with correct isolation and completion. The staged
buffered_countassertions provide good visibility into state transitions.
120-139: Error case coverage is appropriate.The test correctly validates rejection of orphan continuation frames and verifies the expected error variant with the correct message key.
141-164: Duplicate detection test is sound.Correctly validates that the state rejects a second first frame for the same message key, preserving assembly integrity.
243-272: Deterministic timeout test is well-designed.Using
accept_first_frame_atandpurge_expired_atwith explicitInstantvalues avoids real-time dependencies. This is the correct approach per project guidelines for timeout testing.
274-296: Single-frame immediate completion is correctly tested.Validates that
is_last: trueon a first frame returns the assembled message immediately without buffering.
298-303:into_bodyconversion test is sufficient.Simple but necessary coverage for the ownership-taking accessor.
- Add series_accepts_final_continuation_at_max_sequence test to verify that u32::MAX is accepted as a valid final sequence number - Convert state_with_defaults helper to an rstest fixture for better integration with the parameterized test framework - Extract header factory helpers (first_header, first_header_with_total, continuation_header) to reduce test setup duplication - Add state_enforces_size_limit_on_first_frame test to verify early rejection based on declared total_body_len - Refactor existing tests to use the new helpers and fixture pattern 🤖 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.
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/message_assembler/series_tests.rs:
- Around line 13-40: Convert the three helper factory functions (first_header,
continuation_header, continuation_header_untracked) into rstest fixtures to
match state_tests.rs patterns or move them into a shared test utility module if
they’ll be reused across files; replace direct calls to these functions in tests
with the fixture parameters (or import from the shared module), ensure fixture
signatures return the corresponding types (FirstFrameHeader and
ContinuationFrameHeader) and preserve the existing field values (message_key,
metadata_len/body_len/total_body_len, sequence, is_last) so tests continue to
behave identically.
In @src/message_assembler/state_tests.rs:
- Around line 253-276: Add this new scenario into the existing parameterized
test instead of a separate test: update the SizeLimitCase struct to include a
total_body_len field (optionally Option<usize>), extend the existing
state_enforces_size_limit test to build the FirstFrameInput using that
total_body_len when present, and remove the standalone
state_enforces_size_limit_on_first_frame test; use the same helpers
(first_header_with_total, FirstFrameInput::new) and the same assertion pattern
against MessageAssemblyState::accept_first_frame and
MessageAssemblyError::MessageTooLarge to keep behavior identical.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/message_assembler/series_tests.rssrc/message_assembler/state_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/message_assembler/state_tests.rssrc/message_assembler/series_tests.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/message_assembler/state_tests.rssrc/message_assembler/series_tests.rs
🧠 Learnings (3)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
src/message_assembler/state_tests.rssrc/message_assembler/series_tests.rs
📚 Learning: 2026-01-08T08:16:31.989Z
Learnt from: leynos
Repo: leynos/wireframe PR: 411
File: src/client/tests/lifecycle.rs:0-0
Timestamp: 2026-01-08T08:16:31.989Z
Learning: In Rust, apply #[expect(lint, reason = "...")] only to code that actually triggers the specified lint. If the code does not trigger the lint, Clippy will fail with 'unfulfilled lint expectation' errors, which is intentional to prevent stale suppressions. Do not add #[expect] attributes proactively or to code that doesn't trigger the lint, even if similar code elsewhere requires suppression. This guideline should be applied broadly to Rust source files across the repository.
Applied to files:
src/message_assembler/state_tests.rssrc/message_assembler/series_tests.rs
📚 Learning: 2026-01-08T20:33:24.570Z
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: src/codec/error_tests.rs:152-152
Timestamp: 2026-01-08T20:33:24.570Z
Learning: Guideline: In Rust, when using contains with a single-character pattern, prefer a char literal (e.g., 'a') over a string literal (e.g., "a") to satisfy the Clippy lint single_char_pattern, which flags single-character strings as less efficient. Apply this across Rust source files (all .rs) to improve efficiency and lint compliance. Example: use s.contains('a') instead of s.contains("a"); note that str::contains can take a char, making the char literal both correct and more efficient.
Applied to files:
src/message_assembler/state_tests.rssrc/message_assembler/series_tests.rs
🧬 Code graph analysis (2)
src/message_assembler/state_tests.rs (2)
src/message_assembler/types.rs (4)
new(69-91)new(118-124)metadata(132-132)body(136-136)src/message_assembler/state.rs (3)
new(31-38)new(114-120)buffered_count(353-353)
src/message_assembler/series_tests.rs (2)
src/message_assembler/series.rs (2)
from_first_frame(96-104)expected_total(116-116)tests/worlds/message_assembly.rs (1)
accept_continuation(174-193)
⏰ 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 (21)
src/message_assembler/series_tests.rs (11)
1-11: LGTM!Module documentation present and imports are well-organised. The imports cover all necessary types from the message assembler module.
42-60: LGTM!Good coverage of the happy path for sequential continuation frames. The test correctly verifies both intermediate
Incompletestatus and finalCompletestatus.
62-73: LGTM!Correctly validates that continuations without sequence numbers bypass ordering checks whilst still completing the series.
75-88: LGTM!Correctly validates rejection of continuations with mismatched message keys, including verification of both expected and found values in the error.
90-110: LGTM!Correctly validates that sequence gaps are detected and reported with accurate expected/found values.
112-138: LGTM!Good test for duplicate frame detection. The scenario of sending an old sequence number after advancing correctly triggers the
DuplicateFrameerror.
140-158: LGTM!Correctly validates that attempts to continue an already-completed series are rejected with
SeriesComplete.
160-179: LGTM!This test addresses the sequence overflow concern raised in PR feedback. Correctly validates that a non-final frame at
u32::MAXtriggersSequenceOverflowsince incrementing would overflow.
181-201: LGTM!Excellent complementary test to the overflow case. This validates that
u32::MAXis accepted as a valid final sequence number, addressing the PR feedback about the sequence overflow bug fix.
203-215: LGTM!Correctly validates that
expected_total()returns the declaredtotal_body_lenfrom the first frame header.
217-222: LGTM!Concise and correct test for single-frame message completion.
src/message_assembler/state_tests.rs (10)
1-29: LGTM!Good use of rstest fixtures for shared test setup. The
NonZeroUsize::new(1024).expect("non-zero")is acceptable here as 1024 is a compile-time constant that cannot fail the non-zero check.
31-70: LGTM!Header factory helpers are well-documented and reduce test setup duplication. The
first_header_with_totalvariant specifically supports testing the early size-rejection behaviour mentioned in PR feedback.
76-102: LGTM!Comprehensive happy-path test covering metadata handling, body assembly, and correct buffered count tracking through the assembly lifecycle.
104-139: LGTM!Good validation of interleaved message assembly. The test verifies correct buffered count tracking as messages complete independently.
Optionally, add a test case completing messages in reverse order (message 2 before message 1) to ensure ordering independence.
141-155: LGTM!Correctly validates rejection of orphan continuation frames and verifies the nested error structure.
157-174: LGTM!Correctly validates rejection of duplicate first frames for the same message key.
176-251: LGTM!Excellent use of parameterised testing with
rstestand a parameter object (SizeLimitCase) to reduce duplication whilst testing both first-frame and continuation rejection paths. The verification ofbuffered_count == 0confirms proper cleanup after size-limit violations.
278-301: LGTM!Correctly validates the expiration and purge mechanism. The test clearly demonstrates time advancement past the timeout threshold and verification of evicted keys.
303-326: LGTM!Correctly validates that single-frame messages bypass buffering and are returned immediately. Good verification that
buffered_countremains zero.
328-333: LGTM!Concise test for the consuming
into_body()accessor onAssembledMessage.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: src/message_assembler/series_tests.rs Comment on lines +161 to +179 fn series_detects_sequence_overflow() {
let first = first_header(1, false);
let mut series = MessageSeries::from_first_frame(&first);
// Force next_sequence to u32::MAX
series.force_next_sequence_for_tests(FrameSequence(u32::MAX));
// Send a non-final frame at MAX
let cont = ContinuationFrameHeader {
message_key: MessageKey(1),
sequence: Some(FrameSequence(u32::MAX)),
body_len: 5,
is_last: false,
};
assert!(matches!(
series.accept_continuation(&cont),
Err(MessageSeriesError::SequenceOverflow { .. })
));
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
…ate tests - Create test_helpers.rs module with shared header factory functions (first_header, first_header_with_total, continuation_header, continuation_header_untracked) to reduce duplication across test files - Update series_tests.rs to use shared helpers via local wrappers - Update state_tests.rs to use shared helpers directly - Merge state_enforces_size_limit_on_first_frame test case into the existing parameterized state_enforces_size_limit test by adding total_body_len field to SizeLimitCase struct 🤖 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.
Merge series_detects_sequence_overflow and series_accepts_final_continuation_at_max_sequence into a single rstest-parameterised test using SequenceOverflowExpectation enum to express expected outcomes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
leynos
changed the title
1 participant
Summary
Changes
src/message_assembler/error.rs,src/message_assembler/series.rs,src/message_assembler/state.rs, andsrc/message_assembler/types.rs.src/message_assembler/header.rsgainsFrameSequence::checked_increment()to support overflow handling in tests.src/message_assembler/mod.rsupdated to declare new submodules and re-export key types.src/lib.rsre-exports:AssembledMessage,MessageAssemblyError,MessageAssemblyState,MessageSeries,MessageSeriesError,MessageSeriesStatus.MessageSeries,MessageSeriesError,MessageSeriesStatus,MessageAssemblyState, andAssembledMessagefor ergonomic test usage.src/message_assembler/types.rs:FirstFrameInput,FirstFrameInputError, andAssembledMessageto simplify test interactions.src/message_assembler/series_tests.rssrc/message_assembler/state_tests.rstests/features/message_assembly.featuretests/worlds/message_assembly.rstests/steps/message_assembly_steps.rsdocs/execplans/8-2-3-message-key-support.md.docs/users-guide.mddescribingMessageAssemblyState,MessageSeries, and error handling.docs/roadmap.mdreflecting 8-2-3 and 8-2-4 progress.MessageSeries,MessageAssemblyState,MessageSeriesError,MessageSeriesStatus,AssembledMessage, andMessageAssemblyErrorre-exported at crate root for ergonomic usage.Testing & Validation
Compatibility & Migration
MessageSeries,MessageAssemblyState,MessageSeriesError,MessageSeriesStatus,AssembledMessage, andMessageAssemblyError; these are re-exported at the crate root for ergonomic usage.Release Notes
How to test locally
If you’d like, I can walk through the gates locally or adjust docs/examples to align with preferred protocol semantics.
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/4c74a299-56aa-4d2f-af6b-5f00d6b95539
📎 Task: https://www.terragonlabs.com/task/ac7c17c6-37de-4f27-90df-ab49f5dfe935