Implement Transport-Level Fragmentation And Reassembly#393
Conversation
Introduce domain types and logic for fragmentation and re-assembly layers. - Add MessageId, FragmentIndex, and FragmentHeader types - Implement FragmentSeries to track and validate fragment sequences - Provide detailed fragment error types and fragmentation status - Add tests for fragment ordering, completion, and error conditions - Export fragment module and types from the crate root Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
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 CodeRabbitRelease Notes
WalkthroughAdd a serialiser‑agnostic fragment metadata module exposing MessageId, FragmentIndex, FragmentHeader, FragmentSeries, FragmentStatus and FragmentError; re‑export them at crate root; add unit and BDD tests and new test worlds and feature files; update documentation and roadmap; add derive_more dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Series as FragmentSeries
participant Header as FragmentHeader
Tester->>Series: FragmentSeries::new(MessageId(5))
Note right of Series `#DDEBF7`: next_index = 0\ncomplete = false
Tester->>Header: FragmentHeader::new(MessageId(5), FragmentIndex(0), is_last=false)
Tester->>Series: accept(header)
alt valid, in-order
Series-->>Tester: Ok(FragmentStatus::Incomplete)
else message id mismatch
Series-->>Tester: Err(FragmentError::MessageMismatch)
else index mismatch
Series-->>Tester: Err(FragmentError::IndexMismatch)
else series already complete
Series-->>Tester: Err(FragmentError::SeriesComplete)
end
Tester->>Header: FragmentHeader::new(MessageId(5), FragmentIndex(1), is_last=true)
Tester->>Series: accept(header)
alt final fragment accepted
Series-->>Tester: Ok(FragmentStatus::Complete)
Note right of Series `#E8F5E9`: mark complete\nadvance next_index (checked)
else overflow on advance
Series-->>Tester: Err(FragmentError::IndexOverflow)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
… tests - Introduce FragmentHeader, MessageId, FragmentIndex, and FragmentSeries types to standardize fragment metadata. - Embed FragmentSeries logic to enforce fragment ordering and completion invariants. - Expose these new types in the crate root for easier access. - Add comprehensive behavioural tests and Cucumber scenarios to validate fragment sequencing and error cases. - Update documentation to describe the new canonical fragment header and design decisions related to fragment handling. This enables protocol implementers to uniformly handle message fragmentation and reassembly with consistent metadata across different codec strategies. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Added a test case that primes the FragmentSeries to expect the maximum fragment index, verifying that index overflow is detected correctly. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Reviewer's GuideThis PR implements transport-layer fragmentation by introducing a new Entity relationship diagram for fragmentation metadataerDiagram
MESSAGE {
u64 id
}
FRAGMENT {
u32 index
bool is_last_fragment
u64 message_id
}
FRAGMENT_SERIES {
u64 message_id
u32 next_index
bool complete
}
MESSAGE ||--o{ FRAGMENT : contains
MESSAGE ||--o{ FRAGMENT_SERIES : tracked_by
FRAGMENT_SERIES ||--|{ FRAGMENT : reassembles
Class diagram for new fragmentation primitivesclassDiagram
class MessageId {
+u64 value
+new(value: u64)
+get() u64
}
class FragmentIndex {
+u32 value
+new(value: u32)
+zero()
+get() u32
+checked_increment() Option<FragmentIndex>
}
class FragmentHeader {
+MessageId message_id
+FragmentIndex fragment_index
+bool is_last_fragment
+new(message_id, fragment_index, is_last_fragment)
+message_id() MessageId
+fragment_index() FragmentIndex
+is_last_fragment() bool
}
class FragmentStatus {
Incomplete
Complete
}
class FragmentError {
MessageMismatch(expected: MessageId, found: MessageId)
IndexMismatch(expected: FragmentIndex, found: FragmentIndex)
SeriesComplete
IndexOverflow(last: FragmentIndex)
}
class FragmentSeries {
+MessageId message_id
+FragmentIndex next_index
+bool complete
+new(message_id: MessageId)
+message_id() MessageId
+is_complete() bool
+accept(fragment: FragmentHeader) Result<FragmentStatus, FragmentError>
}
FragmentHeader --> MessageId
FragmentHeader --> FragmentIndex
FragmentSeries --> MessageId
FragmentSeries --> FragmentIndex
FragmentSeries --> FragmentStatus
FragmentSeries --> FragmentError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/features/fragment.feature:8` </location>
<code_context>
Cognitive Complexity is incremented based on three main rules.
1. **Breaks in Linear Flow:** Each time the code breaks the normal linear
- reading flow (e.g., loops, conditionals like
</code_context>
<issue_to_address>
**suggestion (testing):** Feature file is missing scenarios for index overflow and series completion error.
Please add scenarios for accepting a fragment after u32::MAX and after the series is marked complete to ensure all relevant error conditions are tested.
</issue_to_address>
### Comment 2
<location> `tests/world.rs:631-636` </location>
<code_context>
+ assert!(series.is_complete(), "series should be marked complete");
+ }
+
+ /// Assert that the latest fragment failed due to an index mismatch.
+ ///
+ /// # Panics
+ /// Panics if no fragment was processed or if the fragment failed for some
+ /// other reason.
+ pub fn assert_index_mismatch(&self) {
+ let err = match self.last_result() {
+ Err(err) => err,
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding assertion helpers for index overflow and series completion errors.
Adding these helpers will improve test coverage for error scenarios and align with existing assertion patterns.
</issue_to_address>
### Comment 3
<location> `src/fragment/mod.rs:1` </location>
<code_context>
+//! Fragment metadata primitives for transparent message splitting.
+//!
+//! This module introduces domain types required by the forthcoming
</code_context>
<issue_to_address>
**issue (complexity):** Consider splitting the large file into focused sub-modules and using derive macros to eliminate repetitive trait implementations.
```markdown
Consider peeling this 350 LOC file apart into focused sub-modules and cutting most of the trivial boilerplate with a `derive_more`-style helper. For example:
1. Split into sub-modules under `fragment/`:
- `id.rs` → `MessageId`
- `index.rs` → `FragmentIndex`
- `header.rs` → `FragmentHeader`
- `error.rs` → `FragmentError` & `FragmentStatus`
- `series.rs` → `FragmentSeries`
- `mod.rs`:
```rust
pub mod id;
pub mod index;
pub mod header;
pub mod error;
pub mod series;
pub use id::MessageId;
pub use index::FragmentIndex;
pub use header::FragmentHeader;
pub use error::{FragmentError, FragmentStatus};
pub use series::FragmentSeries;
```
2. In each newtype file you can collapse all of your `From`/`Display` impls:
```rust
// id.rs
use derive_more::{Display, From, Into};
use bincode::{Decode, Encode};
#[derive(
Clone, Copy, Debug, PartialEq, Eq, Hash, Encode, Decode,
Display, From, Into
)]
// Display("{0}") will print the inner u64
#[display(fmt = "{}", _0)]
pub struct MessageId(u64);
```
3. Simplify `checked_increment` in one line:
```rust
// index.rs
impl FragmentIndex {
#[must_use]
pub fn checked_increment(self) -> Option<Self> {
self.0.checked_add(1).map(FragmentIndex)
}
}
```
This removes dozens of lines of trivial trait impls, and the split‐up files keep each concept isolated.
</issue_to_address>
### Comment 4
<location> `src/fragment/mod.rs:279` </location>
<code_context>
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use rstest::rstest;
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural tests for the new fragmentation primitives; only unit tests are present.
The new fragmentation primitives (MessageId, FragmentIndex, FragmentHeader, FragmentSeries) are covered by unit tests in this module, but there are no behavioural tests present. Add behavioural tests that exercise these types in realistic protocol scenarios, ideally in the Cucumber test suite.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/rust-doctest-dry-guide.md (1)
640-649: Fix dangling “accessed on July 15, 2025” line in footnote [^11].Tidy the [^11] reference so the second URL has a proper label and does not start with a bare, indented “accessed on July 15, 2025,” line. Align it with the style of the other references.
-[^11]: Compile_fail doc test ignored in cfg(test) - help - The Rust Programming Language Forum, accessed on July 15, 2025, -<https://users.rust-lang.org/t/compile-fail-doc-test-ignored-in-cfg-test/124927> - accessed on July 15, 2025, -<https://users.rust-lang.org/t/test-setup-for-doctests/50426> +[^11]: Compile_fail doc test ignored in cfg(test) – help – The Rust Programming Language Forum, accessed on July 15, 2025, +<https://users.rust-lang.org/t/compile-fail-doc-test-ignored-in-cfg-test/124927> +Test setup for doctests – help – The Rust Programming Language Forum, accessed on July 15, 2025, +<https://users.rust-lang.org/t/test-setup-for-doctests/50426>tests/cucumber.rs (1)
1-19: Update module documentation to reflect all five test suites.The module-level documentation lists only three test suites (PanicWorld, CorrelationWorld, StreamEndWorld), but the implementation now includes five suites: the three listed plus MultiPacketWorld and FragmentWorld. Update the doc comment to enumerate all active test contexts.
Apply this diff:
//! Cucumber test runner for integration tests. //! -//! Orchestrates three distinct test suites: +//! Orchestrates five distinct test suites: //! - `PanicWorld`: Tests server resilience during connection panics //! - `CorrelationWorld`: Tests correlation ID propagation in multi-frame responses //! - `StreamEndWorld`: Verifies end-of-stream signalling +//! - `MultiPacketWorld`: Tests channel-backed multi-packet response delivery +//! - `FragmentWorld`: Tests fragment metadata enforcement and reassembly primitives //! //! # Example //! //! The runner executes feature files sequentially: //! ```text //! tests/features/connection_panic.feature -> PanicWorld context //! tests/features/correlation_id.feature -> CorrelationWorld context //! tests/features/stream_end.feature -> StreamEndWorld context +//! tests/features/multi_packet.feature -> MultiPacketWorld context +//! tests/features/fragment.feature -> FragmentWorld context //! ```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
docs/complexity-antipatterns-and-refactoring-strategies.md(1 hunks)docs/generic-message-fragmentation-and-re-assembly-design.md(2 hunks)docs/roadmap.md(1 hunks)docs/rust-binary-router-library-design.md(2 hunks)docs/rust-doctest-dry-guide.md(1 hunks)docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md(1 hunks)docs/users-guide.md(1 hunks)src/fragment/mod.rs(1 hunks)src/lib.rs(2 hunks)tests/cucumber.rs(1 hunks)tests/features/fragment.feature(1 hunks)tests/steps/fragment_steps.rs(1 hunks)tests/steps/mod.rs(1 hunks)tests/world.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (docs/message-versioning.md)
Document examples showing how to deprecate old message versions gracefully
When long lines are warranted in documentation, disable markdownlint rule MD013 inline using the comment:
docs/**/*.md: Use docs/ as the knowledge base and source of truth for requirements, dependencies, and architectural decisions
Proactively update docs/ when decisions, requirements, libraries, or architecture change to keep documentation accurate
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)Follow markdownlint recommendations
Files:
docs/roadmap.mddocs/rust-doctest-dry-guide.mddocs/complexity-antipatterns-and-refactoring-strategies.mddocs/users-guide.mddocs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/rust-binary-router-library-design.md
docs/**
📄 CodeRabbit inference engine (docs/wireframe-1-0-detailed-development-roadmap.md)
docs/**: Document async-stream as the canonical way to create streams imperatively
Write comprehensive user guides for Duplex Messaging & Pushes, Streaming Responses, and Message Fragmentation with runnable examples
Files:
docs/roadmap.mddocs/rust-doctest-dry-guide.mddocs/complexity-antipatterns-and-refactoring-strategies.mddocs/users-guide.mddocs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/rust-binary-router-library-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks in Markdown at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes
Files:
docs/roadmap.mddocs/rust-doctest-dry-guide.mddocs/complexity-antipatterns-and-refactoring-strategies.mddocs/users-guide.mddocs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/rust-binary-router-library-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/rust-doctest-dry-guide.mddocs/complexity-antipatterns-and-refactoring-strategies.mddocs/users-guide.mddocs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/rust-binary-router-library-design.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.{md,rs}: Use British English (en-GB-oxendict) spelling in documentation prose
Keep US spelling when it appears in API identifiers (e.g.,color)
The word “outwith” is acceptable in documentation
Use the Oxford comma where it aids comprehension
Treat company names as collective nouns (e.g., “Lille Industries are…”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Use standard Markdown syntax for code blocks and lists
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first-level bullet; renumber ordered lists when items change
Prefer inline links[text](url)or angle-bracket autolinks<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure Markdown tables include a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code at 120 columns
Do not wrap tables
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
When embedding images, use Markdownwith brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
Files:
docs/roadmap.mddocs/rust-doctest-dry-guide.mddocs/complexity-antipatterns-and-refactoring-strategies.mddocs/users-guide.mddocs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mddocs/generic-message-fragmentation-and-re-assembly-design.mddocs/rust-binary-router-library-design.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Name things precisely; use clear, descriptive names; booleans should use is/has/should prefixes
Use consistent en-GB-oxendict spelling and grammar in comments (except external API references)
Disallow Clippy warnings; fix warnings in code rather than silencing
Extract helpers when functions are too long; maintain separation of concerns and CQRS
Group many parameters into meaningful structs when a function has too many parameters
When returning a large error, consider using Arc to reduce data returned
Every Rust module must begin with a module-level (//! ) comment explaining its purpose and utility
Document public APIs with Rustdoc comments (///) to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Prefer single-line versions of trivial functions when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use NewTypes to model domain values; consider newt-hype, tuple structs, and the-newtype as appropriate; add shims like From<&str>/AsRef; implement AsRef for path wrappers and provide into_inner()/to_path_buf()
Keep file size manageable: no single code file longer than 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Files:
tests/steps/mod.rstests/cucumber.rssrc/lib.rstests/world.rstests/steps/fragment_steps.rssrc/fragment/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/mod.rstests/cucumber.rssrc/lib.rstests/world.rstests/steps/fragment_steps.rssrc/fragment/mod.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup in tests
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs in tests
Files:
tests/steps/mod.rstests/cucumber.rstests/world.rstests/steps/fragment_steps.rs
🧬 Code graph analysis (2)
tests/cucumber.rs (1)
tests/world.rs (1)
run(248-255)
tests/world.rs (1)
src/fragment/mod.rs (3)
message_id(141-141)message_id(209-209)is_complete(213-213)
🪛 LanguageTool
docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md
[uncategorized] ~183-~183: Possible missing comma found.
Context: ...hose headers, > keeping transport logic consistent while letting each protocol choose how ...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/generic-message-fragmentation-and-re-assembly-design.md
[typographical] ~100-~100: Consider using an em dash in dialogues and enumerations.
Context: ...ust surface: - message_id: MessageId – a unique identifier for the logical mes...
(DASH_RULE)
[typographical] ~101-~101: Consider using an em dash in dialogues and enumerations.
Context: ...sage. - fragment_index: FragmentIndex – a zero-based counter tracking fragment ...
(DASH_RULE)
[typographical] ~103-~103: Consider using an em dash in dialogues and enumerations.
Context: ... ordering. - is_last_fragment: bool – signals that the logical message is com...
(DASH_RULE)
[style] ~118-~118: Would you like to use the Oxford spelling “standardizing”? The spelling ‘standardising’ is also correct.
Context: ...series.accept(header).is_ok()); ``` By standardising this metadata, higher layers—behavioura...
(OXFORD_SPELLING_Z_NOT_S)
🔍 Remote MCP Deepwiki
Based on the comprehensive wiki documentation retrieved, here is the additional relevant context for reviewing this pull request:
Additional Context Found
This PR adds transport fragmentation primitives, which is Phase 7 in the project's development roadmap toward wireframe 1.0. Here are the key relevant facts:
Strategic Context
The fragmentation and reassembly feature is designed to transparently handle large messages by splitting them into smaller frames for transmission and reconstructing them at the receiving end, integrated into the message processing pipeline between raw socket I/O and the router/handlers as a FrameProcessor middleware called FragmentAdapter.
The core fragmentation implementation revolves around the FragmentAdapter and the FragmentStrategy trait, where FragmentAdapter manages state for multiplexing, allowing interleaved fragments from different logical messages on the same connection to be handled concurrently using a dashmap::DashMap keyed by msg_id to store PartialMessage instances.
Architecture Integration Points
The FragmentStrategy trait provides the protocol-specific rules for fragmentation and reassembly with methods including max_fragment_payload (determines maximum payload size), decode_header (parses fragment header from buffer), and encode_header (emits header for outbound fragment).
The FragmentMeta struct contains payload_len, total_message_len (for pre-allocation hints), is_final, msg_id (for multiplexing), and seq (for ordering).
Design Philosophy
The fragmentation layer is designed to be transparent to application logic, meaning handlers continue to send and receive complete Message objects, and works in conjunction with other wireframe capabilities like Streaming Responses and Asynchronous Pushes.
Testing Requirements
The PR demonstrates alignment with wireframe's four-layer testing strategy with foundational unit tests for fragment header exposure and series behavior in fragment/mod.rs, and Cucumber behavioral tests with feature file tests/features/fragment.feature, step definitions tests/steps/fragment_steps.rs, and test world tests/world.rs.
Production Maturity Concerns
The framework emphasizes DoS protection mechanisms in fragmentation including configurable max_message_size to prevent memory exhaustion, reassembly_timeout to purge incomplete messages, and back pressure mechanisms in push queues to prevent memory exhaustion. This PR establishes the foundational primitives that will support these protections in the full FragmentAdapter implementation.,
⏰ 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 (18)
docs/complexity-antipatterns-and-refactoring-strategies.md (1)
86-89: Keep reflowed Cognitive Complexity rule text as-is.Retain the updated wrapping of the “breaks in linear flow” rule; it improves readability without changing meaning and remains compliant with the documentation guidelines.
docs/rust-binary-router-library-design.md (1)
115-121: Accept clarified derive-macro emphasis.Keep the revised paragraph highlighting derive macros across
bincode,postcard, andbin-proto; it reads clearly, matches the surrounding discussion, and introduces no new technical risk.docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md (1)
180-184: Keep the 14 November 2025 fragmentation update; it aligns with the code.Retain this update block: the referenced types (
FragmentHeader,MessageId,FragmentIndex,FragmentSeries) and their role for codec implementers match the newsrc/fragment/mod.rsAPI and the intended serialiser-agnostic fragmentation design.src/fragment/mod.rs (4)
1-151: Approve fragment metadata newtypes and header API.Keep the
MessageId,FragmentIndex, andFragmentHeaderdefinitions as written. They:
- Use precise newtypes with
Encode/Decodederives,Displayimpls, and#[must_use]accessors.- Provide simple, const-friendly constructors (
new,zero) and getters that match the documented doctest behaviour.- Keep
FragmentHeadersmall, copyable, and serialiser-agnostic, which is exactly what codec implementations and tests need.
152-182: Approve FragmentStatus and FragmentError design.Retain the
FragmentStatusandFragmentErrorenums as-is. Variant naming andthiserror::Errormessages are clear, precise, and give downstream callers enough context to distinguish message mismatches, ordering issues, completion, and index overflow.
184-214: Approve lightweight FragmentSeries state and accessors.Keep the
FragmentSeriesstruct and its basic API (new,message_id,is_complete) unchanged. The type holds only minimal, copyable metadata (ID, next index, completion flag), which matches the intent to embed it in a future fragment adapter without extra allocation.
279-352: Approve rstest-backed coverage of fragment series behaviour.Keep the test module as written. The tests:
- Exercise header field access, sequential success paths, and each error case (message mismatch, index mismatch, post-completion rejection, and index overflow).
- Use
rstestparameterisation to avoid duplication while checking multiple message IDs.- Avoid
.unwrap()outside tests and useexpect_errwith descriptive messages, complying with the project’s Rust testing guidelines.tests/steps/mod.rs (1)
7-7: LGTM: Module wiring is clean.The fragment_steps module is correctly integrated alongside existing step modules.
docs/roadmap.md (1)
258-259: LGTM: Roadmap update accurately reflects completion of fragment metadata primitives.The checkbox correctly marks the completion of generic fragment header definition, aligning with the PR's introduction of
MessageId,FragmentIndex,FragmentHeader, and related types.tests/cucumber.rs (2)
25-25: LGTM: FragmentWorld import is correctly positioned.
33-33: LGTM: FragmentWorld test execution is properly integrated.src/lib.rs (2)
17-17: LGTM: Fragment module declaration follows established patterns.
34-41: LGTM: Fragment type re-exports provide clean public API surface.The re-exported types (
FragmentError,FragmentHeader,FragmentIndex,FragmentSeries,FragmentStatus,MessageId) establish a well-structured public interface for the fragmentation primitives.tests/features/fragment.feature (1)
1-17: LGTM: Feature scenarios comprehensively cover fragment sequencing behaviours.The three scenarios appropriately test sequential completion, out-of-order rejection, and message mismatch detection, providing strong BDD coverage for fragment metadata enforcement.
tests/world.rs (1)
25-32: LGTM: Fragment type imports are correctly positioned.tests/steps/fragment_steps.rs (1)
1-31: LGTM: Step definitions correctly wire feature scenarios to FragmentWorld.The seven step functions provide clean, single-line mappings from Gherkin steps to
FragmentWorldmethods, following established patterns in the codebase.docs/generic-message-fragmentation-and-re-assembly-design.md (2)
94-123: LGTM: Canonical fragment header documentation is comprehensive.Section 3.2 clearly describes the
FragmentHeaderdesign, its constituent fields, and the rationale for standardisation. The code example demonstrates practical usage of the new primitives.
331-342: LGTM: Design decisions are well-documented with appropriate context.Section 8 provides a dated record of key design decisions, establishing clear documentation of the
FragmentSeriesordering validation and the rationale for exposing fragment metadata as public API.
Introduce core domain types for fragmentation and re-assembly: - MessageId, FragmentIndex, FragmentHeader, FragmentSeries - FragmentStatus and FragmentError enums Refactor fragment module into submodules for improved organization. Add comprehensive unit and integration tests covering sequential fragment acceptance, error conditions (message mismatch, index mismatch, overflow, series completion), and scenario-based cucumber tests. Upgrade derive_more dependency to version 1.0.0 and add relevant features for display, from, and into implementations. This change enables robust handling and tracking of message fragmentation with strict metadata enforcement and improved test coverage to ensure protocol correctness. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
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: 15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.toml(1 hunks)src/fragment/error.rs(1 hunks)src/fragment/header.rs(1 hunks)src/fragment/id.rs(1 hunks)src/fragment/index.rs(1 hunks)src/fragment/mod.rs(1 hunks)src/fragment/series.rs(1 hunks)src/fragment/tests.rs(1 hunks)tests/features/fragment.feature(1 hunks)tests/steps/fragment_steps.rs(1 hunks)tests/world.rs(1 hunks)tests/worlds/correlation.rs(1 hunks)tests/worlds/fragment.rs(1 hunks)tests/worlds/mod.rs(1 hunks)tests/worlds/multi_packet.rs(1 hunks)tests/worlds/panic.rs(1 hunks)tests/worlds/stream_end.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Name things precisely; use clear, descriptive names; booleans should use is/has/should prefixes
Use consistent en-GB-oxendict spelling and grammar in comments (except external API references)
Disallow Clippy warnings; fix warnings in code rather than silencing
Extract helpers when functions are too long; maintain separation of concerns and CQRS
Group many parameters into meaningful structs when a function has too many parameters
When returning a large error, consider using Arc to reduce data returned
Every Rust module must begin with a module-level (//! ) comment explaining its purpose and utility
Document public APIs with Rustdoc comments (///) to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Prefer single-line versions of trivial functions when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use NewTypes to model domain values; consider newt-hype, tuple structs, and the-newtype as appropriate; add shims like From<&str>/AsRef; implement AsRef for path wrappers and provide into_inner()/to_path_buf()
Keep file size manageable: no single code file longer than 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Files:
src/fragment/error.rssrc/fragment/mod.rstests/worlds/correlation.rssrc/fragment/tests.rssrc/fragment/id.rstests/worlds/mod.rssrc/fragment/series.rssrc/fragment/header.rstests/steps/fragment_steps.rstests/worlds/panic.rstests/worlds/fragment.rstests/worlds/multi_packet.rstests/world.rssrc/fragment/index.rstests/worlds/stream_end.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/fragment/error.rssrc/fragment/mod.rstests/worlds/correlation.rssrc/fragment/tests.rssrc/fragment/id.rstests/worlds/mod.rssrc/fragment/series.rssrc/fragment/header.rstests/steps/fragment_steps.rstests/worlds/panic.rstests/worlds/fragment.rstests/worlds/multi_packet.rstests/world.rssrc/fragment/index.rstests/worlds/stream_end.rs
**/Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
**/Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up to date
Mandate caret requirements for all dependencies in Cargo.toml (e.g., "1.2.3")
Prohibit wildcard (*) and open-ended (>=) version specifiers; allow tilde (~) only with a documented reason
Files:
Cargo.toml
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup in tests
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs in tests
Files:
tests/worlds/correlation.rstests/worlds/mod.rstests/steps/fragment_steps.rstests/worlds/panic.rstests/worlds/fragment.rstests/worlds/multi_packet.rstests/world.rstests/worlds/stream_end.rs
🧬 Code graph analysis (11)
tests/worlds/correlation.rs (2)
tests/worlds/mod.rs (1)
build_small_queues(18-24)tests/worlds/stream_end.rs (6)
build_small_queues(107-107)build_small_queues(128-128)process(100-115)new(32-39)process_multi(121-136)drop(62-67)
src/fragment/tests.rs (4)
src/fragment/header.rs (4)
new(30-40)message_id(44-44)fragment_index(48-48)is_last_fragment(52-52)src/fragment/id.rs (1)
new(20-20)src/fragment/index.rs (2)
new(25-25)zero(29-29)src/fragment/series.rs (4)
new(18-24)message_id(28-28)accept(61-93)is_complete(32-32)
src/fragment/id.rs (2)
src/fragment/header.rs (1)
new(30-40)src/fragment/series.rs (1)
new(18-24)
tests/worlds/mod.rs (4)
tests/common/mod.rs (1)
unused_listener(12-15)tests/worlds/correlation.rs (2)
build_small_queues(39-39)build_small_queues(60-60)tests/worlds/multi_packet.rs (1)
build_small_queues(18-18)tests/worlds/stream_end.rs (2)
build_small_queues(107-107)build_small_queues(128-128)
src/fragment/series.rs (3)
src/fragment/header.rs (2)
new(30-40)message_id(44-44)src/fragment/id.rs (1)
new(20-20)src/fragment/index.rs (2)
new(25-25)zero(29-29)
src/fragment/header.rs (3)
src/fragment/id.rs (1)
new(20-20)src/fragment/index.rs (1)
new(25-25)src/fragment/series.rs (2)
new(18-24)message_id(28-28)
tests/worlds/panic.rs (2)
tests/common/mod.rs (1)
unused_listener(12-15)tests/steps/panic_steps.rs (1)
connect(15-15)
tests/worlds/fragment.rs (4)
src/fragment/header.rs (2)
message_id(44-44)new(30-40)src/fragment/series.rs (3)
message_id(28-28)new(18-24)is_complete(32-32)src/fragment/id.rs (1)
new(20-20)src/fragment/index.rs (1)
new(25-25)
tests/worlds/multi_packet.rs (2)
tests/worlds/mod.rs (1)
build_small_queues(18-24)src/response.rs (1)
with_channel(155-158)
src/fragment/index.rs (3)
src/fragment/header.rs (1)
new(30-40)src/fragment/id.rs (2)
new(20-20)get(24-24)src/fragment/series.rs (1)
new(18-24)
tests/worlds/stream_end.rs (5)
wireframe_testing/src/logging.rs (1)
logger(82-82)tests/worlds/mod.rs (1)
build_small_queues(18-24)src/hooks.rs (1)
from_protocol(140-176)src/connection.rs (1)
with_hooks(274-303)src/connection/test_support.rs (1)
new_with_state(57-68)
⏰ 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 (3)
src/fragment/tests.rs (1)
5-59: Keep these unit tests as the core regression suiteRetain these tests; they give solid coverage for header field access, sequential acceptance, wrong-message rejection, out-of-order rejection, and post-completion behaviour, and align well with the BDD scenarios.
src/fragment/mod.rs (1)
1-21: Retain this split module and re-export layoutKeep this small
mod.rsthat documents the fragment domain, wires submodules, and re-exports the key types; it matches the earlier recommendation to decompose the large monolithic module and yields a clean public surface (wireframe::fragment::*).tests/world.rs (1)
2-12: Keep this thin re-export module as-isRetain this short world hub; it restores compliance with the 400-line guideline by pushing each world into its own module while preserving a stable
tests::worldimport surface for the Cucumber runner.
The cucumber test runner documentation was updated to reflect the addition of two new test suites: MultiPacketWorld and FragmentWorld. This improves the accuracy and completeness of the docs describing the integration tests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationtests/worlds/fragment.rs: What lead to degradation?The module contains 4 functions with similar structure: FragmentWorld.assert_index_mismatch,FragmentWorld.assert_index_overflow,FragmentWorld.assert_message_mismatch,FragmentWorld.assert_series_complete_error 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 |
|
@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 Duplicationtests/worlds/stream_end.rs: What lead to degradation?The module contains 2 functions with similar structure: StreamEndWorld.process_multi_disconnect,StreamEndWorld.process_multi_shutdown 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 resolved.
This comment was marked as resolved.
The FragmentSeries now accepts a final fragment with the maximum allowed index (u32::MAX) as a valid completion event. Previously, this would be rejected as an index overflow error. This change updates the accept logic to treat the final fragment case specially when at max index, marking the fragment series as complete and avoiding overflow errors. Additional improvements: - Added From conversions from MessageId and FragmentIndex to their underlying integer types for ergonomic use. - Introduced doc comments for fragment module types for better documentation. - Enhanced fragment tests and cucumber integration to cover the new edge case. - Refactored some fragment world test assertions to reduce duplication. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (2)
src/fragment/id.rs (1)
19-19: Fix the Display attribute syntax for derive_more 1.0.0.The attribute syntax
#[display("{_0}")]is incorrect for derive_more 1.0.0, which requires thefmt =form. Change to#[display(fmt = "{}")]to match the crate's expected syntax.Apply this correction:
-#[display("{_0}")] +#[display(fmt = "{}")] pub struct MessageId(u64);🤖 Prompt for AI Agents
In src/fragment/id.rs at line 19, change the Display derive attribute from #[display("{_0}")] to #[display(fmt = "{}")] to match derive_more 1.0.0 syntax requirements; update only the attribute line without altering the struct definition or other derives.tests/worlds/multi_packet.rs (1)
1-3: Move the module doc comment before the cfg attribute.Ensure the module truly “begins” with a
//!comment per guidelines by placing the doc comment above the innercfgattribute.-#![cfg(not(loom))] -//! Test world for multi-packet channel behaviour and ergonomics. +//! Test world for multi-packet channel behaviour and ergonomics. +#![cfg(not(loom))]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
Cargo.toml(1 hunks)docs/users-guide.md(2 hunks)src/fragment/error.rs(1 hunks)src/fragment/header.rs(1 hunks)src/fragment/id.rs(1 hunks)src/fragment/index.rs(1 hunks)src/fragment/series.rs(1 hunks)src/fragment/tests.rs(1 hunks)tests/features/fragment.feature(1 hunks)tests/worlds/correlation.rs(1 hunks)tests/worlds/fragment.rs(1 hunks)tests/worlds/mod.rs(1 hunks)tests/worlds/multi_packet.rs(1 hunks)tests/worlds/panic.rs(1 hunks)tests/worlds/stream_end.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
**/Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up to date
Mandate caret requirements for all dependencies in Cargo.toml (e.g., "1.2.3")
Prohibit wildcard (*) and open-ended (>=) version specifiers; allow tilde (~) only with a documented reason
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Name things precisely; use clear, descriptive names; booleans should use is/has/should prefixes
Use consistent en-GB-oxendict spelling and grammar in comments (except external API references)
Disallow Clippy warnings; fix warnings in code rather than silencing
Extract helpers when functions are too long; maintain separation of concerns and CQRS
Group many parameters into meaningful structs when a function has too many parameters
When returning a large error, consider using Arc to reduce data returned
Every Rust module must begin with a module-level (//! ) comment explaining its purpose and utility
Document public APIs with Rustdoc comments (///) to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Prefer single-line versions of trivial functions when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use NewTypes to model domain values; consider newt-hype, tuple structs, and the-newtype as appropriate; add shims like From<&str>/AsRef; implement AsRef for path wrappers and provide into_inner()/to_path_buf()
Keep file size manageable: no single code file longer than 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Files:
src/fragment/error.rstests/worlds/fragment.rssrc/fragment/index.rstests/worlds/multi_packet.rssrc/fragment/header.rssrc/fragment/id.rstests/worlds/panic.rssrc/fragment/series.rstests/worlds/mod.rstests/worlds/stream_end.rssrc/fragment/tests.rstests/worlds/correlation.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/fragment/error.rstests/worlds/fragment.rssrc/fragment/index.rstests/worlds/multi_packet.rssrc/fragment/header.rssrc/fragment/id.rstests/worlds/panic.rssrc/fragment/series.rstests/worlds/mod.rstests/worlds/stream_end.rssrc/fragment/tests.rstests/worlds/correlation.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup in tests
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs in tests
Files:
tests/worlds/fragment.rstests/worlds/multi_packet.rstests/worlds/panic.rstests/worlds/mod.rstests/worlds/stream_end.rstests/worlds/correlation.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/message-versioning.md)
Document examples showing how to deprecate old message versions gracefully
When long lines are warranted in documentation, disable markdownlint rule MD013 inline using the comment:
docs/**/*.md: Use docs/ as the knowledge base and source of truth for requirements, dependencies, and architectural decisions
Proactively update docs/ when decisions, requirements, libraries, or architecture change to keep documentation accurate
Documentation must use en-GB-oxendict spelling and grammar (LICENSE name unchanged)Follow markdownlint recommendations
Files:
docs/users-guide.md
docs/**
📄 CodeRabbit inference engine (docs/wireframe-1-0-detailed-development-roadmap.md)
docs/**: Document async-stream as the canonical way to create streams imperatively
Write comprehensive user guides for Duplex Messaging & Pushes, Streaming Responses, and Message Fragmentation with runnable examples
Files:
docs/users-guide.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown with make markdownlint
Run make fmt after documentation changes to format Markdown and fix table markup
Validate Mermaid diagrams in Markdown using make nixie
Wrap Markdown paragraphs and bullet points at 80 columns
Wrap code blocks in Markdown at 120 columns
Do not wrap tables and headings in Markdown
Use dashes (-) for list bullets in Markdown
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes
Files:
docs/users-guide.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.md
docs/**/*.{md,rs}
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/*.{md,rs}: Use British English (en-GB-oxendict) spelling in documentation prose
Keep US spelling when it appears in API identifiers (e.g.,color)
The word “outwith” is acceptable in documentation
Use the Oxford comma where it aids comprehension
Treat company names as collective nouns (e.g., “Lille Industries are…”)
Write headings in sentence case
Use Markdown headings (#,##,###, …) in order without skipping levels
Use standard Markdown syntax for code blocks and lists
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text
Use-as the first-level bullet; renumber ordered lists when items change
Prefer inline links[text](url)or angle-bracket autolinks<url>
Ensure blank lines before and after bulleted lists and fenced code blocks
Ensure Markdown tables include a delimiter line below the header row
Expand uncommon acronyms on first use (e.g., Continuous Integration (CI))
Wrap paragraphs at 80 columns
Wrap code at 120 columns
Do not wrap tables
Use footnotes referenced with[^label]
Include Mermaid diagrams where they add clarity
When embedding images, use Markdownwith brief, descriptive alt text
Add a short descriptive sentence before each Mermaid diagram for screen readers
Files:
docs/users-guide.md
🧬 Code graph analysis (11)
tests/worlds/fragment.rs (4)
src/fragment/header.rs (2)
message_id(49-49)new(35-45)src/fragment/series.rs (3)
message_id(33-33)new(23-29)is_complete(37-37)src/fragment/id.rs (1)
new(25-25)src/fragment/index.rs (1)
new(30-30)
src/fragment/index.rs (3)
src/fragment/header.rs (1)
new(35-45)src/fragment/id.rs (3)
new(25-25)get(29-29)from(33-33)src/fragment/series.rs (1)
new(23-29)
tests/worlds/multi_packet.rs (2)
tests/worlds/mod.rs (1)
build_small_queues(22-28)src/response.rs (1)
with_channel(155-158)
src/fragment/header.rs (3)
src/fragment/id.rs (1)
new(25-25)src/fragment/index.rs (1)
new(30-30)src/fragment/series.rs (2)
new(23-29)message_id(33-33)
src/fragment/id.rs (2)
src/fragment/header.rs (1)
new(35-45)src/fragment/series.rs (1)
new(23-29)
tests/worlds/panic.rs (2)
tests/common/mod.rs (1)
unused_listener(12-15)tests/steps/panic_steps.rs (1)
connect(15-15)
src/fragment/series.rs (3)
src/fragment/header.rs (2)
new(35-45)message_id(49-49)src/fragment/id.rs (1)
new(25-25)src/fragment/index.rs (2)
new(30-30)zero(34-34)
tests/worlds/mod.rs (4)
tests/common/mod.rs (1)
unused_listener(12-15)tests/worlds/correlation.rs (2)
build_small_queues(40-40)build_small_queues(61-61)tests/worlds/multi_packet.rs (1)
build_small_queues(19-19)tests/worlds/stream_end.rs (2)
build_small_queues(114-114)build_small_queues(135-135)
tests/worlds/stream_end.rs (6)
wireframe_testing/src/logging.rs (1)
logger(82-82)tests/worlds/correlation.rs (5)
build_small_queues(40-40)build_small_queues(61-61)process(32-44)process_multi(50-67)expected(26-26)tests/worlds/mod.rs (1)
build_small_queues(22-28)src/hooks.rs (1)
from_protocol(140-176)src/connection.rs (1)
with_hooks(274-303)src/connection/test_support.rs (1)
new_with_state(57-68)
src/fragment/tests.rs (4)
src/fragment/header.rs (4)
new(35-45)message_id(49-49)fragment_index(53-53)is_last_fragment(57-57)src/fragment/id.rs (1)
new(25-25)src/fragment/index.rs (2)
new(30-30)zero(34-34)src/fragment/series.rs (4)
new(23-29)message_id(33-33)accept(66-102)is_complete(37-37)
tests/worlds/correlation.rs (1)
tests/worlds/mod.rs (1)
build_small_queues(22-28)
🪛 GitHub Actions: CI
tests/worlds/fragment.rs
[error] 106-106: Rust fmt check failed: diff detected for IndexMismatch formatting in fragment.rs. Run 'cargo fmt' to format.
[error] 127-127: Rust fmt check failed: diff detected for IndexOverflow formatting in fragment.rs. Run 'cargo fmt' to format.
[error] 135-135: Rust fmt check failed: diff detected for SeriesComplete formatting in fragment.rs. Run 'cargo fmt' to format.
tests/worlds/stream_end.rs
[error] 74-74: Rust fmt check failed: diff detected in prepare_test formatting in stream_end.rs. Run 'cargo fmt' to format.
🪛 GitHub Check: build-test
tests/worlds/fragment.rs
[warning] 135-135:
Diff in /home/runner/work/wireframe/wireframe/tests/worlds/fragment.rs
[warning] 127-127:
Diff in /home/runner/work/wireframe/wireframe/tests/worlds/fragment.rs
[warning] 106-106:
Diff in /home/runner/work/wireframe/wireframe/tests/worlds/fragment.rs
[warning] 135-135:
Diff in /home/runner/work/wireframe/wireframe/tests/worlds/fragment.rs
[warning] 127-127:
Diff in /home/runner/work/wireframe/wireframe/tests/worlds/fragment.rs
[warning] 106-106:
Diff in /home/runner/work/wireframe/wireframe/tests/worlds/fragment.rs
tests/worlds/stream_end.rs
[warning] 74-74:
Diff in /home/runner/work/wireframe/wireframe/tests/worlds/stream_end.rs
[warning] 74-74:
Diff in /home/runner/work/wireframe/wireframe/tests/worlds/stream_end.rs
🪛 LanguageTool
docs/users-guide.md
[misspelling] ~616-~616: This word is normally spelled as one.
Context: ...in src/fragment/mod.rs and supporting sub-modules. [^42]: Exercised in `tests/features/fr...
(EN_COMPOUNDS_SUB_MODULES)
🔍 Remote MCP Ref
I'll proceed without external docs. Provide a concise review-focused summary of additional relevant facts I can extract from the PR context itself, citing the internal tool I attempted to use.
Summary (concise, review-focused):
- New public crate surface:
- pub mod fragment added and re-exported from crate root; re-exports: FragmentError, FragmentHeader, FragmentIndex, FragmentSeries, FragmentStatus, MessageId.
- Types and behaviors to review:
- MessageId: newtype(u64), From for u64, const constructors, Encode/Decode derives — check Display/From derive usage and const fn correctness.
- FragmentIndex: zero-based u32 wrapper with checked_increment(), TryFrom, From for u32 — verify overflow handling and TryFrom error type.
- FragmentHeader: (MessageId, FragmentIndex, bool) with const constructor and accessor methods; derives Encode/Decode — ensure Encode/Decode derive config (bincode) compiles.
- FragmentSeries::accept(): enforces message id match, series-complete rejection, in-order index validation, increments next_index with overflow check and sets complete on final fragment — validate state transitions, IndexOverflow semantics, and tests covering edge cases (max u32).
- FragmentError and FragmentStatus enums: clear variants for test diagnostics and API consumers.
- Tests added:
- Unit tests in src/fragment/tests.rs covering header accessors, sequential acceptance, message mismatch, index mismatch, series-complete, index overflow, final-at-max-index. Pay attention to use of force_next_index_for_tests helper.
- Cucumber integration: feature file tests/features/fragment.feature + step definitions tests/steps/fragment_steps.rs + FragmentWorld in tests/worlds/fragment.rs wired into tests/world.rs and tests/steps/mod.rs — verify test harness wiring and that new worlds are gated appropriately (cfg not(loom) where needed).
- Test infra changes with higher review surface:
- tests/world.rs now re-exports multiple worlds from tests/worlds/mod.rs; many world implementations moved to tests/worlds/* — expect larger integration test surface and ensure no name collisions or unintended pub exposures.
- Dependency change:
- Added derive_more = "1.0.0" with features display, from in Cargo.toml — confirm no version conflicts and that new derives are used (MessageId Display/From).
- Suggested refactor comments in PR:
- Two test-world refactor suggestions: consolidate repeated assertion helpers in FragmentWorld and centralize multi-packet harness in StreamEndWorld — these are maintainability improvements; assess risk to tests if applied.
Recommendation checklist for reviewer (short):
- Run cargo test (unit + cucumber) to verify compilation and behavioral tests.
- Inspect Encode/Decode derives compile with chosen serializer (bincode) in workspace.
- Confirm FragmentSeries accept() edge cases: max index final fragment, overflow path, concurrency assumptions (Copy types used across actors).
- Verify public re-exports don't unintentionally break semver or crate API expectations.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (12)
tests/worlds/correlation.rs (1)
32-44: Retain current correlation world behaviour.Retain the streaming and multi-packet flows in
process,process_multi, andverify; they correctly driveConnectionActorwith aFrameStreamand multi-packet channel and then assert that all emitted frames respect the configured correlation expectation.Also applies to: 50-67, 69-82
tests/worlds/panic.rs (1)
12-53: Keep the panic world server lifecycle as implemented.Keep the
PanicServerandPanicWorldimplementation: the server factory, ready signal, shutdown signalling, abort-on-timeout inDrop, and the two connection attempts inPanicWorldcollectively exercise the panic-on-connection path and verify resilience without leaking resources.Also applies to: 55-69, 71-103
tests/worlds/stream_end.rs (1)
103-143: Keep the refactored stream-end and multi-packet harness logic.Retain the
process,process_multi,run_multi_packet_harness, and verification methods. The shared harness removes duplication between disconnect and shutdown tests, exercisesProtocolHooks::from_protocolandActorHarness::new_with_statecorrectly, and captures both frames and multi-packet closure logs for later assertions.Also applies to: 158-201, 203-250
tests/features/fragment.feature (1)
1-35: Retain the full set of fragment metadata scenarios.Retain this feature file as-is. The scenarios now cover ordered completion, out-of-order and cross-message rejection, index overflow, the final fragment at
4294967295completing the series, and rejection after completion, which together pin the intendedFragmentSeries::acceptsemantics.src/fragment/tests.rs (1)
1-79: Comprehensive test coverage established.The test suite now correctly covers all fragment state transitions, including the previously missing final-fragment-at-max-index scenario (lines 72-79). The parameterized tests using rstest reduce duplication whilst maintaining clarity.
src/fragment/header.rs (1)
1-58: Well-structured serializable metadata type.The module documentation is clear, the struct derives are appropriate for copyable metadata, and the const accessors enable efficient composition in fragmentation logic. Implementation is correct.
tests/worlds/fragment.rs (1)
77-86: Excellent refactoring to eliminate duplication.The
assert_errorhelper cleanly consolidates the repeated error-checking logic from the four assertion methods, reducing code size whilst maintaining clarity and correct panic behaviour.src/fragment/error.rs (1)
1-40: Clear error types with precise diagnostics.The error variants are well-named and provide structured diagnostic information. The use of
thiserror::Errorfor error messages and the Copy derives enable efficient error propagation throughout the fragmentation layer.src/fragment/series.rs (1)
66-102: Critical overflow handling correctly implemented.The
acceptmethod now properly handles the final fragment atu32::MAXby checkingis_last_fragment()before attempting to validate overflow (lines 86-92). This ensures the series accepts the final fragment at the index limit and marks completion correctly, whilst still detecting overflow for non-final fragments. Implementation is correct.tests/worlds/multi_packet.rs (3)
17-28: Approve connection harness and frame collection logic.Keep the
collect_frames_fromhelper as written; it cleanly encapsulates the ConnectionActor setup and frame collection for reuse across the world.
69-95: Approve overflow path and back-pressure exercise.Keep the overflow scenario as implemented; it correctly exercises the bounded channel, verifies the immediate
Fullerror viatry_send, and then confirms that the drained channel accepts a follow-up frame before collecting frames through the shared harness.
97-124: Approve verification helpers for received frames and overflow state.Retain the
verify_empty,verify, andverify_overflowhelpers; they provide clear, focused assertions on both the frames and the overflow flag, and they match the documented behaviour.
…nk lines Cleaned up the formatting of error assertion calls in fragment.rs for better readability by expanding the closure and message parameters over multiple lines. Also removed an extraneous blank line in stream_end.rs to maintain consistent code style. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
Updated documentation comments in src/fragment/index.rs and various test world modules to provide clearer explanations of the purpose and features of the respective modules and test worlds. This improves maintainability and clarity for contributors working with fragmentation, correlation, multi-packet, panic recovery, and stream termination test scenarios. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
tests/worlds/correlation.rs (2)
1-6: Move module doc comment before the cfg attributeStart the module with the
//!documentation and place#![cfg(not(loom))]immediately after it so the file literally begins with documentation, in line with the project’s Rust module guidelines.
31-47: Document all panic conditions inprocessExtend the
# Panicssection forprocessto state that it also panics whenself.expectedisNone(viaexpect("streaming scenario requires a correlation id")), not only whenactor.runfails, so the docs accurately describe all panic paths.tests/worlds/stream_end.rs (2)
1-8: Place module documentation before the cfg attributeMove the
//!module doc comment so it is the very first line in the file, with#![cfg(not(loom))]immediately after, to satisfy the requirement that modules begin with documentation.
7-77: Remove unnecessaryunsafeinStreamEndTestGuardand fixMultiPacketModematchingEliminate the raw pointer and
unsafeblocks inStreamEndTestGuardand fix the pattern match onMultiPacketModeso this file stays within the “no unsafe unless necessary” rule and compiles correctly.
- Replace the raw pointer guard with a simple mutable reference and drop
PhantomData/ptr:-use std::{future::Future, marker::PhantomData, ptr, sync::Arc}; +use std::{future::Future, sync::Arc}; @@ -struct StreamEndTestGuard<'a> { - world: *mut StreamEndWorld, - logger: LoggerHandle, - _marker: PhantomData<&'a mut StreamEndWorld>, -} +struct StreamEndTestGuard<'a> { + world: &'a mut StreamEndWorld, + logger: LoggerHandle, +} @@ impl<'a> StreamEndTestGuard<'a> { fn new(world: &'a mut StreamEndWorld) -> Self { let logger = world.prepare_test(); - Self { - world: ptr::from_mut(world), - logger, - _marker: PhantomData, - } + Self { world, logger } } @@ fn run<F, R>(mut self, f: F) -> R where F: FnOnce(&'a mut StreamEndWorld, &mut LoggerHandle) -> R, { - // Safety: the guard guarantees exclusive access to the world while the closure runs. - let world = unsafe { &mut *self.world }; - f(world, &mut self.logger) + f(self.world, &mut self.logger) } @@ async fn run_async<F, Fut, R>(mut self, f: F) -> R where F: FnOnce(&'a mut StreamEndWorld, &mut LoggerHandle) -> Fut, Fut: Future<Output = R> + 'a, { - // Safety: the guard guarantees exclusive access to the world while the future runs. - let world = unsafe { &mut *self.world }; - f(world, &mut self.logger).await + f(self.world, &mut self.logger).await } } @@ impl Drop for StreamEndTestGuard<'_> { fn drop(&mut self) { - // Safety: the guard created the pointer from a unique mutable reference. - unsafe { - (&mut *self.world).finalize_test(&mut self.logger); - } + self.world.finalize_test(&mut self.logger); } }This keeps the RAII semantics and exclusive access while staying entirely safe.
- Fix
run_multi_packet_harnessto work on the enum value rather than a reference and update the call sites; the currentmatch mode { MultiPacketMode::... }on&MultiPacketModedoes not type-check:- fn run_multi_packet_harness(&mut self, mode: &MultiPacketMode, correlation_id: u64) { + fn run_multi_packet_harness(&mut self, mode: MultiPacketMode, correlation_id: u64) { @@ - match mode { + match mode { MultiPacketMode::Disconnect { send_frames } => { @@ pub fn process_multi_disconnect(&mut self) { - self.run_multi_packet_harness(&MultiPacketMode::Disconnect { send_frames: true }, 42); + self.run_multi_packet_harness(MultiPacketMode::Disconnect { send_frames: true }, 42); @@ pub fn process_multi_shutdown(&mut self) { - self.run_multi_packet_harness(&MultiPacketMode::Shutdown, 77); + self.run_multi_packet_harness(MultiPacketMode::Shutdown, 77); }These changes remove unnecessary
unsafeusage and ensure the helper compiles and behaves as intended.Also applies to: 97-103, 160-187, 193-203
tests/worlds/panic.rs (1)
1-6: Ensure module documentation comes firstMove the
//!module-level doc comment above#![cfg(not(loom))]so the module formally begins with documentation, as required by the coding guidelines.tests/worlds/mod.rs (1)
1-8: Reorder module header so the doc comment is firstPlace the
//!documentation block before#![cfg(not(loom))]so this module literally begins with its documentation, matching the project’s module documentation rule.tests/worlds/multi_packet.rs (3)
1-6: Move the module doc comment before the cfg attribute.Place the
//!block at the very top of the file so the module truly begins with documentation as required by the coding guidelines, then keep#![cfg(not(loom))]immediately after it.-#![cfg(not(loom))] -//! Test world for multi-packet channel scenarios. -//! -//! -//! Provides [`MultiPacketWorld`] to verify message ordering, back-pressure -//! handling, and channel lifecycle in cucumber-based behaviour tests. +//! Test world for multi-packet channel scenarios. +//! +//! +//! Provides [`MultiPacketWorld`] to verify message ordering, back-pressure +//! handling, and channel lifecycle in cucumber-based behaviour tests. + +#![cfg(not(loom))]
14-18: Renameoverflow_errorto follow boolean naming guidelines.Rename the field (and all uses) to an is/has-style boolean such as
has_overflow_errorto align with the project’s boolean naming rule.#[derive(Debug, Default, World)] pub struct MultiPacketWorld { messages: Vec<u8>, - overflow_error: bool, + has_overflow_error: bool, } @@ - self.messages = frames; - self.overflow_error = false; + self.messages = frames; + self.has_overflow_error = false; @@ - self.messages = frames; - self.overflow_error = overflow_error; + self.messages = frames; + self.has_overflow_error = overflow_error; @@ - assert!( - self.overflow_error, + assert!( + self.has_overflow_error, "expected overflow error when channel capacity was exceeded", );
33-70: Align doc comments forprocess_messages,process, andprocess_emptywith actual panic behaviour.Align the doc comments with the shared helper behaviour so they all describe the same panic conditions (producer task spawn/join failure, and the invariant that the helper must return
MultiPacket):
- Stop documenting a panic “if sending to the channel fails” for
process; send failures inprocess_messagescause early return, not an explicit panic.- Stop claiming
process_empty“does not panic”; it panics under the same conditions asprocess_messages.- Optionally mention that the helper will panic if the
Responseis notMultiPacket(even thoughResponse::with_channelcurrently guarantees it).For example:
- /// Helper method to process messages through a multi-packet response built - /// via [`Response::with_channel`]. - /// - /// # Panics - /// Panics if spawning or joining the producer task fails. + /// Helper method to process messages through a multi-packet response built + /// via [`Response::with_channel`]. + /// + /// # Panics + /// Panics if spawning or joining the producer task fails, or if the + /// helper does not return a multi-packet response. @@ - /// Send messages through a multi-packet response and record them. - /// - /// # Panics - /// Panics if sending to the channel fails. + /// Send messages through a multi-packet response and record them. + /// + /// # Panics + /// Panics under the same conditions as [`Self::process_messages`]. @@ - /// Record zero messages from a closed channel. - /// - /// # Panics - /// Does not panic. + /// Record frames when no messages are sent. + /// + /// # Panics + /// Panics under the same conditions as [`Self::process_messages`].tests/worlds/fragment.rs (1)
1-6: Move the module doc comment before the cfg attribute.Move the
//!doc comment block to the top so the module genuinely begins with documentation, then place#![cfg(not(loom))]below it, in line with the coding guidelines.-#![cfg(not(loom))] -//! Test world for fragment reassembly scenarios. -//! -//! -//! Provides [`FragmentWorld`] to verify ordering, completion detection, and -//! error handling across the fragmentation behavioural tests. +//! Test world for fragment reassembly scenarios. +//! +//! +//! Provides [`FragmentWorld`] to verify ordering, completion detection, and +//! error handling across the fragmentation behavioural tests. + +#![cfg(not(loom))]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/fragment/index.rs(1 hunks)tests/worlds/correlation.rs(1 hunks)tests/worlds/fragment.rs(1 hunks)tests/worlds/mod.rs(1 hunks)tests/worlds/multi_packet.rs(1 hunks)tests/worlds/panic.rs(1 hunks)tests/worlds/stream_end.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Name things precisely; use clear, descriptive names; booleans should use is/has/should prefixes
Use consistent en-GB-oxendict spelling and grammar in comments (except external API references)
Disallow Clippy warnings; fix warnings in code rather than silencing
Extract helpers when functions are too long; maintain separation of concerns and CQRS
Group many parameters into meaningful structs when a function has too many parameters
When returning a large error, consider using Arc to reduce data returned
Every Rust module must begin with a module-level (//! ) comment explaining its purpose and utility
Document public APIs with Rustdoc comments (///) to enable cargo doc generation
Prefer immutable data; avoid unnecessary mut bindings
Use Result for error handling instead of panicking where feasible
Avoid unsafe code unless absolutely necessary and document any usage clearly
Place function attributes after doc comments
Do not use return in single-line functions
Use predicate functions for conditional criteria with more than two branches
Do not silence lints except as a last resort; suppressions must be tightly scoped and include a clear reason
Prefer #[expect] over #[allow] for lints
Prefer .expect() over .unwrap()
Use concat!() to combine long string literals rather than escaping newlines with a backslash
Prefer single-line versions of trivial functions when appropriate (e.g., pub fn new(id: u64) -> Self { Self(id) })
Use NewTypes to model domain values; consider newt-hype, tuple structs, and the-newtype as appropriate; add shims like From<&str>/AsRef; implement AsRef for path wrappers and provide into_inner()/to_path_buf()
Keep file size manageable: no single code file longer than 400 lines; split long switches/dispatch tables by feature; move large test data to external files
Files:
tests/worlds/correlation.rssrc/fragment/index.rstests/worlds/stream_end.rstests/worlds/panic.rstests/worlds/fragment.rstests/worlds/mod.rstests/worlds/multi_packet.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/worlds/correlation.rssrc/fragment/index.rstests/worlds/stream_end.rstests/worlds/panic.rstests/worlds/fragment.rstests/worlds/mod.rstests/worlds/multi_packet.rs
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
tests/**/*.rs: Use rstest fixtures for shared setup in tests
Replace duplicated tests with #[rstest(...)] parameterised cases
Prefer mockall for mocks/stubs in tests
Files:
tests/worlds/correlation.rstests/worlds/stream_end.rstests/worlds/panic.rstests/worlds/fragment.rstests/worlds/mod.rstests/worlds/multi_packet.rs
🧬 Code graph analysis (7)
tests/worlds/correlation.rs (1)
tests/worlds/mod.rs (1)
build_small_queues(24-30)
src/fragment/index.rs (3)
src/fragment/header.rs (1)
new(35-45)src/fragment/id.rs (3)
new(25-25)get(29-29)from(33-33)src/fragment/series.rs (1)
new(23-29)
tests/worlds/stream_end.rs (5)
wireframe_testing/src/logging.rs (1)
logger(82-82)tests/worlds/mod.rs (1)
build_small_queues(24-30)src/hooks.rs (1)
from_protocol(140-176)src/connection.rs (1)
with_hooks(274-303)src/connection/test_support.rs (1)
new_with_state(57-68)
tests/worlds/panic.rs (2)
tests/common/mod.rs (1)
unused_listener(12-15)tests/steps/panic_steps.rs (1)
connect(15-15)
tests/worlds/fragment.rs (4)
src/fragment/header.rs (2)
message_id(49-49)new(35-45)src/fragment/series.rs (3)
message_id(33-33)new(23-29)is_complete(37-37)src/fragment/index.rs (1)
new(30-30)src/fragment/id.rs (1)
new(25-25)
tests/worlds/mod.rs (4)
tests/common/mod.rs (1)
unused_listener(12-15)tests/worlds/correlation.rs (2)
build_small_queues(43-43)build_small_queues(64-64)tests/worlds/multi_packet.rs (1)
build_small_queues(22-22)tests/worlds/stream_end.rs (2)
build_small_queues(116-116)build_small_queues(137-137)
tests/worlds/multi_packet.rs (2)
tests/worlds/mod.rs (1)
build_small_queues(24-30)src/response.rs (1)
with_channel(155-158)
🔍 Remote MCP Deepwiki
Based on my search of the repository documentation, I've gathered comprehensive context about the wireframe project and this PR's fragmentation implementation. Here's the relevant factual information for your PR review:
Transport-Level Fragmentation & Reassembly Context
Project Background:
Wireframe is a Rust framework for building high-performance binary protocol servers using an actor-per-connection model with async/await patterns, comprehensive testing infrastructure, and production-ready observability features.
Design Philosophy for Fragmentation:
The fragmentation and reassembly feature (Phase 7 of the roadmap) is designed as a transparent layer that allows application handlers to continue working with complete messages while large frames are automatically fragmented/reassembled at the transport layer. The implementation uses:
- FragmentStrategy trait for protocol-specific fragmentation rules
- FragmentAdapter middleware for multiplexed reassembly using
dashmap::DashMapkeyed bymsg_id - Configurable DoS protections:
max_message_sizelimits andreassembly_timeoutfor incomplete messages
Types Added by This PR (from AI summary):
The PR introduces metadata-agnostic fragment types in wireframe::fragment:
FragmentHeader(message_id, fragment_index, is_last_fragment)MessageId(u64 newtype)FragmentIndex(zero-based, overflow-safe)FragmentSeries(ordered assembly state machine per message)FragmentStatus(Incomplete/Complete variants)FragmentError(specific diagnostics: MessageMismatch, IndexMismatch, SeriesComplete, IndexOverflow)
All types derive Encode/Decode for serialization compatibility.
Testing Strategy:
The framework uses a four-layer testing approach: (1) rstest fixtures for unit tests, (2) custom integration harnesses with wireframe_testing, (3) Cucumber BDD tests for behavioral specs, and (4) loom/proptest for advanced concurrency and property-based testing. The PR includes comprehensive Cucumber features and step definitions for fragment metadata enforcement.
Code Review Focus Areas
Based on the documentation and the AI summary, these are the key aspects to verify:
- Type Safety & Serialization: All fragment metadata types implement
Encode/Decodecorrectly for binary transport - OrderingValidation:
FragmentSeries::accept()enforces strict in-order fragment delivery with explicit error variants - Test Coverage: Unit tests, behavioral tests (Cucumber), and integration patterns match the framework's multi-layered testing strategy
- Public API Exposure: Re-exports in
src/lib.rscorrectly expose the fragment module to downstream codecs
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (6)
src/fragment/index.rs (1)
11-53: FragmentIndex newtype and conversions look correct and idiomaticKeep this implementation as-is: the derives, constructors,
checked_increment, andTryFrom<usize>/From<FragmentIndex> for u32all provide a clear, type-safe index abstraction that matches the fragment header/series usage and avoids overflow issues.tests/worlds/multi_packet.rs (3)
20-58: Approve multi-packet harness and frame collection logic.Keep the
collect_frames_fromandprocess_messagesstructure; it wiresConnectionActorwith a multi-packet receiver cleanly, uses a bounded channel, and awaits the producer task viaexpect("producer task panicked"), which is appropriate in tests.
72-98: Approve overflow scenario harness.Keep the overflow scenario: using
try_sendto detect the immediate full-buffer condition, recording the overflow flag, and then spawning an async sender that succeeds once the actor drains the channel accurately exercises back-pressure and produces the expected[1, 2]frame sequence.
100-127: Approve verification helpers for clarity of expectations.Keep the
verify_empty,verify, andverify_overflowhelpers; they clearly encode the expected outcomes for each scenario and produce straightforward assertion failures when behaviour diverges.tests/worlds/fragment.rs (2)
70-89: Approvelast_resultandassert_errorhelpers for clear failure diagnostics.Keep
last_resultandassert_error; they centralise the “no fragment processed yet” precondition and produce precise panic messages when the world expected an error but observed a status, or when the wrong error variant was returned.
91-151: Approve fragment assertion API built onassert_error.Keep the assertion methods (
assert_completion,assert_index_mismatch,assert_message_mismatch,assert_index_overflow,assert_series_complete_error) as thin wrappers aroundlast_resultandassert_error; they read well in Cucumber steps, align with the documented panic conditions, and exercise all keyFragmentError/FragmentStatusvariants.
|
✅ Resolved – The module-level doc comment has been added to |
Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…tency - Added missing newline after cfg attribute in multiple test modules - Updated variable name from `overflow_error` to `has_overflow_error` for clarity in multi_packet world - Adjusted documentation comments for clearer panic descriptions - Simplified unsafe usage in StreamEndTestGuard by using mutable references - Minor formatting improvements in docs and display macro syntax change in fragment index Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
- fix derive_more Display syntax and update bool and doc naming conventions - ensure world modules start with documentation and panic docs are accurate - refactor stream end world helpers to avoid unsafe patterns while keeping logging prep/finalize in sync Tests: make check-fmt, make lint, make test
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +49 to +67 pub async fn process(&mut self) {
let mut temp = StreamEndWorld::default();
mem::swap(self, &mut temp);
let mut logger = temp.prepare_test();
let stream: FrameStream<u8> = Box::pin(try_stream! {
yield 1u8;
yield 2u8;
});
let (queues, handle) = build_small_queues::<u8>();
let shutdown = CancellationToken::new();
let hooks = ProtocolHooks::from_protocol(&Arc::new(Terminator));
let mut actor = ConnectionActor::with_hooks(queues, handle, Some(stream), shutdown, hooks);
actor.run(&mut temp.frames).await.expect("actor run failed");
temp.finalize_test(&mut logger);
mem::swap(self, &mut temp);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
- introduce ActorMode and run_actor_test helper to share actor setup - simplify process/process_multi to delegate to the helper while keeping logging swap semantics Tests: CARGO_HOME=.cargo-home make lint && CARGO_HOME=.cargo-home make test
1 participant
Summary
wireframe::fragmentand a minimal reassembly series state machine.MessageId,FragmentIndex,FragmentHeader,FragmentSeries,FragmentStatus,FragmentError) with Encode/Decode support for binary codecs.Changes
Core types
MessageId: newtype aroundu64with helpers andDisplay.FragmentIndex: zero-based index with overflow-safe increment and conversions.FragmentHeader: lightweight per-fragment descriptor (message_id,fragment_index,is_last_fragment).FragmentStatus:IncompleteorComplete.FragmentError: error variants for mismatch, order, completion, and index overflow.FragmentSeries: tracks expectedmessage_id, next index, and completion state; providesaccept()to feed fragments and update state or error.Public API
lib.rs.FragmentError,FragmentHeader,FragmentIndex,FragmentSeries,FragmentStatus,MessageId.Serialization
Encode/Decodefor all metadata primitives to support binary codecs (viabincode).Tests
fragment/mod.rs).tests/features/fragment.feature.tests/steps/fragment_steps.rs.tests/worlds/fragment.rswires fragment state into behavioural tests.Test plan
cargo testto validate unit tests and the cucumber-based integration tests wired into the test suite.Notes
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/ccf68554-8e51-4a2c-9cac-69ef12e53a5e