Implement async envelope-based client messaging API with correlation IDs#419
Implement async envelope-based client messaging API with correlation IDs#419
Conversation
Implemented new async client methods to support envelope-aware messaging with automatic correlation ID generation and validation. Added send_envelope, receive_envelope, and call_correlated methods on WireframeClient that work with the Packet trait. Introduced CorrelationMismatch error variant for detecting response mismatches. This feature preserves backward compatibility by adding new APIs alongside existing raw message methods. It includes a correlation counter per client instance and comprehensive unit and behavioral tests with Cucumber. Documentation has been updated with usage examples. The 10.2.1 roadmap item is now marked as done. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Reviewer's GuideAdds envelope-aware async client messaging APIs on WireframeClient with automatic, per-connection correlation ID generation/validation, a CorrelationMismatch error, and corresponding unit/BDD tests and docs, while refactoring receive() through a shared internal helper and preserving existing raw Message APIs. Updated class diagram for WireframeClient envelope messaging and correlation errorclassDiagram
class WireframeClient {
+connection_state: OptionConnectionState
+on_disconnect: OptionClientConnectionTeardownHandler
+on_error: OptionClientErrorHandler
+correlation_counter: AtomicU64
+framed: FramedTransport
+serializer: Serializer
+receive~M: Message~() Result~M, ClientError~
+call~Req: Message, Res: Message~(request: Req) Result~Res, ClientError~
+receive_internal~R: Message~() Result~R, ClientError~
+next_correlation_id() u64
+send_envelope~P: Packet~(envelope: P) Result~u64, ClientError~
+receive_envelope~P: Packet~() Result~P, ClientError~
+call_correlated~P: Packet~(request: P) Result~P, ClientError~
+invoke_error_hook(error: ClientError) void
}
class ClientError {
<<enum>>
+Io
+Serialize
+Deserialize
+Disconnected
+PreambleTimeout
+CorrelationMismatch
}
class CorrelationMismatchDetails {
+expected: Option~u64~
+received: Option~u64~
}
class Packet {
<<interface>>
+correlation_id() Option~u64~
+into_parts() PacketParts
+from_parts(parts: PacketParts) Packet
}
class Envelope {
+id: u32
+correlation_id: Option~u64~
+payload: Vec~u8~
+new(id: u32, correlation_id: Option~u64~, payload: Vec~u8~) Envelope
+correlation_id() Option~u64~
+into_parts() PacketParts
+from_parts(parts: PacketParts) Envelope
}
class PacketParts {
+id() u32
+correlation_id() Option~u64~
+payload() Vec~u8~
+new(id: u32, correlation_id: Option~u64~, payload: Vec~u8~) PacketParts
}
class Message {
<<interface>>
}
class Serializer {
+serialize<T>(value: T) Result~Vec~u8~, SerializeError~
+deserialize<T>(bytes: Bytes) Result~(T, usize), DeserializeError~
}
class FramedTransport {
+send(bytes: Bytes) Result~(), TransportError~
+next() Option~Result~Bytes, TransportError~~
}
WireframeClient o--> Serializer
WireframeClient o--> FramedTransport
WireframeClient ..> Packet
WireframeClient ..> Envelope
WireframeClient ..> ClientError
ClientError *-- CorrelationMismatchDetails : CorrelationMismatch
Envelope ..|> Packet
Envelope ..|> Message
PacketParts --> Envelope
Packet --> PacketParts
Serializer ..> Message
Serializer ..> Packet
Serializer ..> Envelope
FramedTransport ..> Envelope
FramedTransport ..> Message
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughImplement envelope-aware client messaging with automatic correlation ID stamping and validation: add send_envelope, receive_envelope, call_correlated; introduce an atomic correlation counter and CorrelationMismatch error; extract socket-option builder helpers; add tests, BDD features, test worlds, and documentation for the 10.2.1 client-message API. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as WireframeClient
participant Serializer as BincodeSerializer
participant Server as RemoteServer
rect rgba(200,200,255,0.5)
Client->>Client: next_correlation_id() -> id
end
rect rgba(200,255,200,0.5)
Client->>Serializer: serialize stamped envelope
Client->>Server: send bytes (request)
end
rect rgba(255,230,200,0.5)
Server->>Serializer: deserialize request
alt Echo
Server->>Server: prepare response with same correlation_id
else Mismatch
Server->>Server: prepare response with different correlation_id
end
Server->>Client: send response bytes
end
rect rgba(230,200,255,0.5)
Client->>Serializer: deserialize response -> Envelope
Client->>Client: compare response.correlation_id == request.id
alt Match
Client->>Client: return Ok(response)
else Mismatch
Client->>Client: return Err(CorrelationMismatch{expected, received})
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Comment |
![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: tests/worlds/client_messaging.rs Comment on lines +185 to +194 pub fn verify_auto_generated_correlation(&self) -> TestResult {
let id = self
.sent_correlation_ids
.first()
.ok_or("no correlation ID captured")?;
if *id == 0 {
return Err("correlation ID should be non-zero".into());
}
Ok(())
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +35 to +53 async fn spawn_envelope_echo_server() -> (std::net::SocketAddr, tokio::task::JoinHandle<()>) {
let listener = TcpListener::bind("127.0.0.1:0")
.await
.expect("bind listener");
let addr = listener.local_addr().expect("listener addr");
let handle = tokio::spawn(async move {
let (stream, _) = listener.accept().await.expect("accept client");
let mut framed = Framed::new(stream, LengthDelimitedCodec::new());
while let Some(Ok(bytes)) = framed.next().await {
if !echo_frame(&mut framed, &bytes).await {
break;
}
}
});
(addr, handle)
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +423 to +446 pub async fn receive_envelope<P: Packet>(&mut self) -> Result<P, ClientError> {
let Some(frame) = self.framed.next().await else {
let err = ClientError::Disconnected;
self.invoke_error_hook(&err).await;
return Err(err);
};
let bytes = match frame {
Ok(bytes) => bytes,
Err(e) => {
let err = ClientError::from(e);
self.invoke_error_hook(&err).await;
return Err(err);
}
};
let (envelope, _consumed) = match self.serializer.deserialize(&bytes) {
Ok(result) => result,
Err(e) => {
let err = ClientError::Deserialize(e);
self.invoke_error_hook(&err).await;
return Err(err);
}
};
Ok(envelope)
}❌ New issue: Code Duplication |
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.
…erver setup - Refactored `receive` and `receive_envelope` methods in client runtime to use a new internal helper `receive_internal` for receiving and deserializing frames, removing code duplication. - Updated client messaging tests by unifying echo and mismatched correlation ID servers into a single parameterized spawn_test_server function with different modes. - Added ServerMode enum to clarify server behavior in tests. - Simplified client messaging world test helpers with a method to get the first correlation ID. This cleanup improves code maintainability and test flexibility without changing external behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
![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 7 issues, and left some high level feedback:
- In
ClientMessagingWorld::await_serverthe name suggests waiting for completion but the implementation just aborts the task; consider either awaiting/joining the handle or renaming it to reflect that it cancels the server. - The
then 'the response contains the same message ID and payload'step currently derives both expected and actual values from the same response, making the assertion tautological; it would be more meaningful to persist the originally sent envelope (or its fields) and compare the response against that. - The correlation ID counter uses
AtomicU64withOrdering::Relaxed; if cross-thread access is expected at any point, consider documenting the intended concurrency guarantees and why relaxed ordering is sufficient, or tightening the ordering if needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ClientMessagingWorld::await_server` the name suggests waiting for completion but the implementation just aborts the task; consider either awaiting/joining the handle or renaming it to reflect that it cancels the server.
- The `then 'the response contains the same message ID and payload'` step currently derives both expected and actual values from the same response, making the assertion tautological; it would be more meaningful to persist the originally sent envelope (or its fields) and compare the response against that.
- The correlation ID counter uses `AtomicU64` with `Ordering::Relaxed`; if cross-thread access is expected at any point, consider documenting the intended concurrency guarantees and why relaxed ordering is sufficient, or tightening the ordering if needed.
## Individual Comments
### Comment 1
<location> `src/client/runtime.rs:368-375` </location>
<code_context>
+ /// # Ok(())
+ /// # }
+ /// ```
+ pub async fn send_envelope<P: Packet>(&mut self, envelope: P) -> Result<u64, ClientError> {
+ // Auto-generate correlation ID if not present.
+ let correlation_id = envelope
+ .correlation_id()
+ .unwrap_or_else(|| self.next_correlation_id());
+
+ // Rebuild envelope with correlation ID if it was auto-generated.
+ let envelope = if envelope.correlation_id().is_none() {
+ let parts = envelope.into_parts();
+ P::from_parts(crate::app::PacketParts::new(
</code_context>
<issue_to_address>
**suggestion:** Avoid calling `correlation_id()` twice and reconstructing `PacketParts` when not needed.
In `send_envelope`, `correlation_id()` is called twice: once to compute `correlation_id` and again in the `if envelope.correlation_id().is_none()` check. You can reuse the first result and avoid reconstructing `PacketParts` unnecessarily, e.g.:
```rust
let existing = envelope.correlation_id();
let correlation_id = existing.unwrap_or_else(|| self.next_correlation_id());
let envelope = if existing.is_none() {
let parts = envelope.into_parts();
P::from_parts(PacketParts::new(parts.id(), Some(correlation_id), parts.payload()))
} else {
envelope
};
```
This removes redundant work and simplifies the control flow.
</issue_to_address>
### Comment 2
<location> `tests/steps/client_messaging_steps.rs:101-106` </location>
<code_context>
+ world.verify_unique_correlation_ids()
+}
+
+#[then(expr = "the response contains the same message ID and payload")]
+async fn then_response_matches(world: &mut ClientMessagingWorld) -> TestResult {
+ // Get the values from the envelope that was sent (now stored in response).
+ let response = world.response.as_ref().ok_or("no response")?;
+ let message_id = response.id();
+ let payload = String::from_utf8_lossy(&response.clone().into_parts().payload()).to_string();
+ world.verify_response_matches(message_id, &payload)
+}
</code_context>
<issue_to_address>
**issue (testing):** The "response contains the same message ID and payload" step always passes because it derives expectations from the response itself.
This step currently re-derives `message_id` and `payload` from `world.response` and then calls `verify_response_matches(message_id, &payload)`, so the assertion is tautological. It will only fail if `verify_response_matches` is wrong, not if the server returns an incorrect ID or payload.
To make this step actually validate echo behavior, either:
- Pass the expected values from the scenario (e.g., captured when the request envelope is built), or
- Store the last sent envelope in the world and compare the response against that.
As written, the test never fails for a bad server response, only for a broken helper.
</issue_to_address>
### Comment 3
<location> `docs/execplans/10-2-1-client-message-api.md:215-217` </location>
<code_context>
+change needs to be backed out, revert only the specific files edited for this
+feature.
+
+## Artifacts and Notes
+
+Expected artefacts after completion:
+
+- Modified `src/client/runtime.rs` with new methods and correlation counter.
</code_context>
<issue_to_address>
**nitpick (typo):** Align spelling of "artifacts/artefacts" between the heading and the body for consistency.
The heading uses "Artifacts" while the bullet text uses "artefacts"; please choose one spelling and use it consistently in this section.
```suggestion
## Artifacts and Notes
Expected artifacts after completion:
```
</issue_to_address>
### Comment 4
<location> `src/client/runtime.rs:464` </location>
<code_context>
+ /// # Ok(())
+ /// # }
+ /// ```
+ pub async fn call_correlated<P: Packet>(&mut self, request: P) -> Result<P, ClientError> {
+ let correlation_id = self.send_envelope(request).await?;
+ let response: P = self.receive_envelope().await?;
</code_context>
<issue_to_address>
**issue (review_instructions):** Add both unit and behavioural tests covering the new correlation-aware client messaging APIs (including correlation ID generation, stamping, success path, and mismatch handling).
Treat `next_correlation_id`, `send_envelope`, `receive_envelope`, and `call_correlated` as a new feature surface. Add unit tests (e.g. in `src/client/tests/messaging.rs`) that verify sequential/unique ID generation, auto-stamping of correlation IDs, preservation of explicit IDs, and correct `CorrelationMismatch` behaviour when IDs differ. In addition, populate the newly added Cucumber artefacts with behavioural scenarios that exercise these flows end-to-end; do not leave the behavioural tests as empty shells.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `docs/execplans/10-2-1-client-message-api.md:3` </location>
<code_context>
+# Client Message API with Correlation Identifiers
+
+This ExecPlan is a living document. The sections `Progress`,
+`Surprises & Discoveries`, `Decision Log`, and `Outcomes & Retrospective` must
+be kept up to date as work proceeds.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** "ExecPlan" is an uncommon acronym and is used here without first expanding it (for example, "Execution Plan (ExecPlan)").
Consider expanding the term on first use, such as "This Execution Plan (ExecPlan) is a living document", so that the acronym is defined for readers.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 6
<location> `docs/execplans/10-2-1-client-message-api.md:223` </location>
<code_context>
+- Modified `src/client/error.rs` with `CorrelationMismatch` variant.
+- Modified `src/client/builder.rs` to initialize correlation counter.
+- New `src/client/tests/messaging.rs` with unit tests.
+- New `tests/features/client_messaging.feature` with BDD scenarios.
+- New `tests/worlds/client_messaging.rs` with test world.
+- New `tests/steps/client_messaging_steps.rs` with step definitions.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** "BDD" is an uncommon acronym and is used without being defined on first use.
Please expand "BDD" the first time it appears, for example "behaviour-driven development (BDD) scenarios)", to comply with the acronym definition guideline.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 7
<location> `docs/execplans/10-2-1-client-message-api.md:98` </location>
<code_context>
+- `id: u32` - Message type ID for routing
+- `correlation_id: Option<u64>` - Optional correlation ID for request/response
+ matching
+- `payload: Vec<u8>` - Serialized message payload
+
+The `CorrelatableFrame` trait (`src/correlation.rs`) provides generic access to
</code_context>
<issue_to_address>
**nitpick (review_instructions):** "Serialized" uses US spelling; en-GB spelling here should be "Serialised".
To follow the en-GB-oxendic spelling guideline, update "Serialized" to "Serialised" in this bullet point.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</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: 3e263fac8f
ℹ️ 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/builder.rs (1)
545-553: Correlation counter initialization is correct.Initializing
correlation_counterto1is sound — reserving0for "no correlation" follows the established convention in the codebase. TheAtomicU64provides thread-safe ID generation without requiring&mut self.Note: This file now stands at 555 lines, exceeding the 400-line guideline. Decompose the builder into smaller modules (e.g., extract socket option methods, preamble configuration, or lifecycle hooks into separate files) in a follow-up refactor.
🤖 Fix all issues with AI agents
In @docs/execplans/10-2-1-client-message-api.md:
- Around line 40-42: Update the checklist in
docs/execplans/10-2-1-client-message-api.md to reflect actual completion: change
the item for updating docs/users-guide.md (line ~40) from "[ ]" to "[x]" and
append the completion date, change the roadmap 10.2.1 item (line ~41) from "[ ]"
to "[x]" and append the completion date (since docs/roadmap.md shows it done),
and leave the validation item (line ~42) unchanged if CI validation hasn't run;
reference the users guide file name (docs/users-guide.md) and roadmap item
number (10.2.1) when making these edits so the entries match the PR summary and
roadmap state.
In @docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md:
- Around line 25-32: The PR unintentionally includes purely formatting edits to
the ExecPlan doc (docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md) —
wrapped bullet points for checklist items such as the entries referencing
src/codec/recovery.rs, src/response.rs, src/app/error.rs, and
LengthDelimitedDecoder — which are unrelated to the client message API changes;
remove those formatting-only edits from this branch (revert the wrapped bullet
changes in that file) or extract them into a separate housekeeping commit/PR so
this change set only contains the intended client message API work.
In @docs/users-guide.md:
- Around line 922-938: The Rust snippet is illustrative and won't compile as-is;
update the fenced code block containing the match on
client.call_correlated(request).await and the ClientError::CorrelationMismatch
arm to mark it as non-compiling by adding the "ignore" attribute to the Rust
fence (i.e., change the fence to ```rust,ignore) so the documentation build/test
ignores it while keeping the match, client.call_correlated, and ClientError
identifiers intact.
- Around line 905-917: The Rust example code block that uses Envelope, Packet
and a undefined client should be marked as non-executable to avoid doctest
failures; update the fenced code block opening token from ```rust to
```rust,no_run so the example (which references client and async calls like
client.send_envelope/receive_envelope) is not run as a doctest.
- Around line 869-876: Update the three method description lines to use an em
dash (—) as the separator between the signature and description instead of the
current hyphen-space; specifically change the lines for send_envelope<P:
Packet>(envelope: P), receive_envelope<P: Packet>(), and call_correlated<P:
Packet>(request: P) so each reads "signature — description" (preserving the
exact wording of each description and return details, e.g., the return of
correlation ID and the ClientError::CorrelationMismatch mention).
In @src/client/runtime.rs:
- Around line 368-384: In send_envelope, you're calling
envelope.correlation_id() twice; capture the original Option<u64> before
generating a new id (e.g., let original_correlation = envelope.correlation_id())
then compute correlation_id = original_correlation.unwrap_or_else(||
self.next_correlation_id()); rebuild the envelope only when
original_correlation.is_none() by using envelope.into_parts() and
P::from_parts(crate::app::PacketParts::new(..., Some(correlation_id), ...));
this removes the redundant call while preserving the existing value when
present.
In @tests/worlds/client_messaging.rs:
- Around line 278-283: Rename the method await_server to abort_server to reflect
its behavior: change the function name (keeping the body that does if let
Some(handle) = self.server.take() { handle.abort(); }) to pub fn
abort_server(&mut self) and update all callers to use abort_server() instead of
await_server(); ensure the method visibility and signature remain the same so
tests and step definitions compile.
- Around line 75-78: The method start_mismatch_server assigns self.server_mode =
ServerMode::Mismatch but that field is never read; remove that unused assignment
and leave the call to start_server_with_mode(ServerMode::Mismatch).
Specifically, edit the start_mismatch_server function to drop the line setting
server_mode and simply return
self.start_server_with_mode(ServerMode::Mismatch).await to avoid keeping
redundant state.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
docs/execplans/10-2-1-client-message-api.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/roadmap.mddocs/users-guide.mdsrc/client/builder.rssrc/client/error.rssrc/client/runtime.rssrc/client/tests/messaging.rssrc/client/tests/mod.rstests/cucumber.rstests/features/client_messaging.featuretests/steps/client_messaging_steps.rstests/steps/mod.rstests/world.rstests/worlds/client_messaging.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:
src/client/error.rstests/worlds/mod.rstests/steps/mod.rssrc/client/tests/mod.rssrc/client/builder.rstests/world.rstests/cucumber.rssrc/client/runtime.rssrc/client/tests/messaging.rstests/worlds/client_messaging.rstests/steps/client_messaging_steps.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/client/error.rstests/worlds/mod.rstests/steps/mod.rssrc/client/tests/mod.rssrc/client/builder.rstests/world.rstests/cucumber.rssrc/client/runtime.rssrc/client/tests/messaging.rstests/worlds/client_messaging.rstests/steps/client_messaging_steps.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/roadmap.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/users-guide.mddocs/execplans/10-2-1-client-message-api.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
Every doctest should validate the public API of a crate from the perspective of an external user, treating each documentation test as a separate temporary crate that imports the library as an external dependency
Files:
docs/roadmap.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/users-guide.mddocs/execplans/10-2-1-client-message-api.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes in Markdown.
Files:
docs/roadmap.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/users-guide.mddocs/execplans/10-2-1-client-message-api.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/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/users-guide.mddocs/execplans/10-2-1-client-message-api.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:
src/client/error.rstests/worlds/mod.rstests/steps/mod.rssrc/client/tests/mod.rssrc/client/builder.rstests/world.rstests/cucumber.rssrc/client/runtime.rssrc/client/tests/messaging.rstests/worlds/client_messaging.rstests/steps/client_messaging_steps.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/client/error.rstests/worlds/mod.rstests/steps/mod.rssrc/client/tests/mod.rssrc/client/builder.rstests/world.rstests/cucumber.rssrc/client/runtime.rssrc/client/tests/messaging.rstests/worlds/client_messaging.rstests/steps/client_messaging_steps.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/client/error.rstests/worlds/mod.rstests/steps/mod.rssrc/client/tests/mod.rssrc/client/builder.rstests/world.rstests/cucumber.rssrc/client/runtime.rssrc/client/tests/messaging.rstests/worlds/client_messaging.rstests/steps/client_messaging_steps.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/client_messaging_steps.rs
🧬 Code graph analysis (3)
src/client/tests/messaging.rs (1)
src/client/builder.rs (1)
new(107-115)
tests/worlds/client_messaging.rs (1)
src/client/runtime.rs (2)
send_envelope(368-400)call_correlated(464-480)
tests/steps/client_messaging_steps.rs (1)
tests/worlds/correlation.rs (1)
expected(45-45)
🪛 LanguageTool
docs/users-guide.md
[typographical] ~869-~869: To join two clauses or introduce examples, consider using an em dash.
Context: ... send_envelope<P: Packet>(envelope: P) - Sends an envelope, auto-generating a c...
(DASH_RULE)
[typographical] ~871-~871: To join two clauses or introduce examples, consider using an em dash.
Context: ... used. - receive_envelope<P: Packet>() - Receives and deserializes the next frame...
(DASH_RULE)
[typographical] ~873-~873: To join two clauses or introduce examples, consider using an em dash.
Context: ...call_correlated<P: Packet>(request: P) - Sends a request with auto- generated c...
(DASH_RULE)
docs/execplans/10-2-1-client-message-api.md
[uncategorized] ~67-~67: Possible missing article found.
Context: ...sures uniqueness within a connection. Counter starts at 1 so that 0 can be distinguis...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~68-~68: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...tinguished from auto-generated IDs if needed. Date/Author: 2026-01-09 (Codex). - De...
(EN_REPEATEDWORDS_NEED)
[uncategorized] ~89-~89: Possible missing comma found.
Context: ...gured Serializer for encoding/decoding but do not handle correlation identifiers. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~95-~95: To join two clauses or introduce examples, consider using an em dash.
Context: ...n. The Envelope contains: - id: u32 - Message type ID for routing - `correlati...
(DASH_RULE)
[typographical] ~96-~96: To join two clauses or introduce examples, consider using an em dash.
Context: ... routing - correlation_id: Option<u64> - Optional correlation ID for request/resp...
(DASH_RULE)
[typographical] ~98-~98: To join two clauses or introduce examples, consider using an em dash.
Context: ...response matching - payload: Vec<u8> - Serialized message payload The `Correla...
(DASH_RULE)
[typographical] ~105-~105: To join two clauses or introduce examples, consider using an em dash.
Context: ...y references: - src/client/runtime.rs - Client runtime implementation - `src/cli...
(DASH_RULE)
[typographical] ~106-~106: To join two clauses or introduce examples, consider using an em dash.
Context: ...e implementation - src/client/error.rs - Client error types - `src/app/envelope.r...
(DASH_RULE)
[typographical] ~107-~107: To join two clauses or introduce examples, consider using an em dash.
Context: ...ient error types - src/app/envelope.rs - Envelope and Packet trait definitions - ...
(DASH_RULE)
[typographical] ~108-~108: To join two clauses or introduce examples, consider using an em dash.
Context: ...trait definitions - src/correlation.rs - CorrelatableFrame trait - `docs/multi-pa...
(DASH_RULE)
[typographical] ~109-~109: To join two clauses or introduce examples, consider using an em dash.
Context: ...acket-and-streaming-responses-design.md` - Correlation ID requirements - Testing ...
(DASH_RULE)
[style] ~177-~177: Consider removing “of” to be more concise
Context: ...ion and Acceptance Acceptance requires all of the following: - New public methods compil...
(ALL_OF_THE)
[style] ~209-~209: Consider using a different verb for a more formal wording.
Context: ... can be re-run safely. If a step fails, fix the underlying issue and re-run only th...
(FIX_RESOLVE)
[style] ~221-~221: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...withCorrelationMismatchvariant. - Modifiedsrc/client/builder.rs` to initialize c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~224-~224: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...essaging.featurewith BDD scenarios. - Newtests/worlds/client_messaging.rs` with...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~225-~225: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...client_messaging.rswith test world. - Newtests/steps/client_messaging_steps.rs`...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[typographical] ~233-~233: To join two clauses or introduce examples, consider using an em dash.
Context: ...ient::next_correlation_id(&self) -> u64- Generate unique ID -WireframeClient::s...
(DASH_RULE)
[typographical] ~235-~235: To join two clauses or introduce examples, consider using an em dash.
Context: ...elope: P) -> Result<u64, ClientError>- Send with auto-correlation -WireframeC...
(DASH_RULE)
[typographical] ~237-~237: To join two clauses or introduce examples, consider using an em dash.
Context: ...(&mut self) -> Result<P, ClientError>- Receive typed envelope -WireframeClien...
(DASH_RULE)
[typographical] ~239-~239: To join two clauses or introduce examples, consider using an em dash.
Context: ...request: P) -> Result<P, ClientError>- Request-response with validation -Clie...
(DASH_RULE)
[typographical] ~241-~241: To join two clauses or introduce examples, consider using an em dash.
Context: ... Option, received: Option }` - Error variant for mismatched IDs The me...
(DASH_RULE)
[uncategorized] ~243-~243: Possible missing comma found.
Context: ...IDs The methods depend on the Packet trait which is implemented by Envelope.
(AI_HYDRA_LEO_MISSING_COMMA)
🔍 Remote MCP Deepwiki
Summary of Relevant Context for PR #419 Review
Foundation and Architecture
Packet Trait and Envelope Design:
The Packet trait defines the interface for message envelope types through four required methods: id() returning a u32 message identifier for routing, correlation_id() returning an optional u64 correlation identifier linking frames to requests, into_parts() consuming the packet and returning its identifier, correlation ID, and payload, and from_parts() constructing a packet from PacketParts. The Envelope struct serves as the default message envelope, implementing the Packet trait and encapsulating the message id, optional correlation_id, and raw byte payload.
Correlation ID Management Strategy:
Correlation IDs are crucial for tracking requests and responses, particularly in streaming scenarios. The wireframe framework automatically preserves correlation IDs from request to response, and handlers extract the correlation ID from ServiceRequest and include it in ServiceResponse. The PacketParts::inherit_correlation method ensures that a correlation ID is present, inheriting from a source if missing or overwriting mismatched values for downstream consistency.
Testing Patterns and Conventions
Integration Testing Framework:
The project employs a multi-layered testing strategy using rstest for parameterized unit/integration tests with fixtures and cucumber for behavioral-driven development scenarios. Step definitions are implemented as async fn functions, enabling asynchronous operations and parallel execution of I/O-bound tests. Tests often use in-memory duplex streams to feed length-prefixed frames without requiring a full server setup, and a World object is used to manage scenario state for isolation and parallel test execution.
Mock Server Patterns:
For testing network-bound scenarios, tokio-test::io::Builder is used to simulate network conditions deterministically with timing controls via #[tokio::test(start_paused)], enabling fast and flakiness-resistant tests.
Key Review Considerations
-
API Surface Alignment: The new envelope-aware APIs (
send_envelope,receive_envelope,call_correlated,next_correlation_id) align with the framework's establishedPackettrait design and correlation ID propagation patterns. -
Error Handling: The framework distinguishes error handling with
WireframeErrorvariants forIo(unrecoverable transport failures) andProtocol(protocol-specific logical errors), with protocol-specific error handling forwarded to callbacks allowing proper error frame serialization. The newCorrelationMismatchvariant follows this pattern. -
Atomic Counter Safety: While server-side
AtomicU64is used for connection tracking and message ID generation, the PR's use ofcorrelation_counter: AtomicU64initialized to 1 for per-connection ID generation is architecturally sound and follows established atomic operation conventions in the codebase. -
Test Coverage: The added unit tests in
src/client/tests/messaging.rsand Cucumber BDD scenarios follow project conventions for comprehensive integration and behavioral testing with in-process test servers and scenario isolation.
⏰ 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 (25)
docs/roadmap.md (1)
460-462: Roadmap completion correctly reflects delivered functionality.Marking item 10.2.1 as complete is appropriate. The PR delivers the async client messaging APIs with correlation identifier support as specified.
tests/steps/mod.rs (1)
7-7: LGTM!The new
client_messaging_stepsmodule declaration follows the established naming convention and maintains alphabetical ordering with the existing step modules.src/client/tests/mod.rs (1)
6-6: LGTM!The new
messagingtest module integrates correctly with the existing test module structure.tests/world.rs (1)
9-9: LGTM!The
ClientMessagingWorldre-export correctly follows the existing pattern and maintains alphabetical ordering.tests/worlds/mod.rs (1)
31-31: LGTM!The new
client_messagingworld module integrates correctly alongside existing world modules.src/client/builder.rs (1)
3-8: LGTM!The import reorganization correctly brings
AtomicU64into scope fromstd::sync::atomic.tests/features/client_messaging.feature (1)
1-3: LGTM!The feature file comprehensively covers the envelope-aware messaging API with correlation ID support. The scenarios align well with the documented behaviours in the PR.
tests/cucumber.rs (1)
4-70: LGTM!The
ClientMessagingWorldis properly integrated into the cucumber test runner with consistent documentation updates, imports, and execution scheduling.src/client/error.rs (1)
32-42: LGTM!The
CorrelationMismatchvariant is well-designed with appropriate documentation referencing the originating API method. The use ofOption<u64>for both fields correctly models scenarios where correlation IDs may be absent.src/client/runtime.rs (3)
71-72: LGTM!The atomic correlation counter with
Relaxedordering is appropriate for per-connection ID generation where only uniqueness within the client instance matters.
281-305: LGTM!The
receive_internalhelper correctly centralizes frame retrieval, error handling, and deserialization. Error hooks are consistently invoked before returning errors.
464-480: LGTM!The
call_correlatedmethod correctly validates that the response correlation ID matches the request. The comparisonresponse_correlation_id != Some(correlation_id)properly handles both missing and mismatched IDs.src/client/tests/messaging.rs (3)
84-111: LGTM!The test correctly verifies that correlation IDs are generated sequentially starting from 1. The server acceptance is properly awaited before assertions.
209-235: LGTM!The test correctly validates that
CorrelationMismatchis returned with differing expected and received values when the server returns a mismatched correlation ID.
304-326: LGTM!Good use of
rstestparameterized testing to verify round-trip integrity with various payload sizes including the empty payload edge case.tests/steps/client_messaging_steps.rs (3)
6-9: LGTM!The module-level
#[expect]with a clear reason correctly handles the cucumber framework requirement for async step functions.
101-107: LGTM!The step correctly extracts the message ID and payload from the stored response for verification. The clone is necessary since
into_parts()consumes the envelope.
47-54: LGTM!The step correctly transitions from the echo server to the mismatch server by first aborting the existing server before starting the new one.
tests/worlds/client_messaging.rs (7)
1-21: LGTM!Module-level documentation is present, and imports are appropriate for the test world functionality.
22-30: LGTM!The
ServerModeenum is well-documented with appropriate derives.
32-44: LGTM!The
ClientMessagingWorldstruct is well-designed for cucumber test scenarios with appropriate use ofOptionfor optional state.
99-179: LGTM!These methods follow proper error handling patterns with
ok_orforOptionhandling and?for propagation. The#[expect]attribute onsend_multiple_envelopescorrectly includes a reason.
181-187: LGTM!Excellent use of
.first().copied().ok_or_else()for safe access to the first correlation ID, avoiding potential panics from direct indexing. Based on learnings, this is the preferred pattern.
286-306: LGTM!The
run_frame_loopfunction is well-designed with proper generic trait bounds and clean error handling using pattern matching.
46-60: Consolidate duplicateprocess_framefunction.This function duplicates the implementation in
src/client/tests/messaging.rs(line 30). Extract the shared logic to a common test utilities module and import from both locations.♻️ Refactor approach
Create or update a shared test utilities module (e.g.,
tests/common/frame_processing.rsor add totests/support.rs) containing:pub fn process_frame(mode: ServerMode, bytes: &[u8]) -> Option<Vec<u8>> { let (envelope, _): (Envelope, usize) = BincodeSerializer.deserialize(bytes).ok()?; let response = match mode { ServerMode::Echo => envelope, ServerMode::Mismatch => { let wrong_id = envelope.correlation_id().map(|id| id.wrapping_add(999)); let parts = envelope.into_parts(); Envelope::new(parts.id(), wrong_id, parts.payload()) } }; BincodeSerializer.serialize(&response).ok() }Then import from both
tests/worlds/client_messaging.rsandsrc/client/tests/messaging.rs.Likely an incorrect or invalid review comment.
…lder - Move socket option convenience methods (nodelay, keepalive, linger, etc.) from builder.rs to a new socket_option_methods.rs module for code organization. - Implement these methods in WireframeClientBuilder to configure socket options via delegation to a SocketOptions struct. - Update client mod.rs to include the new socket_option_methods module. - Refactor builder.rs to remove socket options methods, reducing file size. This enables cleaner builder code and better separation of concerns for socket option configuration. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @docs/execplans/10-2-1-client-message-api.md:
- Line 99: Update the description for `payload: Vec<u8>` to use
Oxford/en-GB-oxendict spelling by replacing "Serialised message payload" with
"Serialized message payload" so the field comment conforms to the -ize/-yze
guideline.
- Around line 163-167: Replace the Cucumber-oriented Step 9 with instructions to
implement rstest-bdd-based behavioural tests per ADR-003: keep the existing
tests/features/client_messaging.feature file(s) but remove references to
Cucumber runner and world bindings (tests/cucumber.rs, tests/world.rs) and
replace tests/worlds/client_messaging.rs and
tests/steps/client_messaging_steps.rs with rstest-bdd fixtures and scenario
bindings (use rstest-bdd attributes and fixture functions to map Given/When/Then
steps to Rust test functions), document how to register and run the rstest-bdd
scenarios instead of Cucumber, and note the ADR-003 rationale for the change.
In @docs/users-guide.md:
- Around line 871-872: Update the documentation string for receive_envelope<P:
Packet>() to use Oxford en-GB oxendict spelling by replacing "deserialises" with
"deserializes" so the sentence reads "Receives and deserializes the next frame
as the specified packet type."; modify the text associated with the
receive_envelope<P: Packet>() entry accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
docs/execplans/10-2-1-client-message-api.mddocs/users-guide.mdsrc/client/builder.rssrc/client/mod.rssrc/client/runtime.rssrc/client/socket_option_methods.rstests/steps/client_messaging_steps.rstests/worlds/client_messaging.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/10-2-1-client-message-api.mddocs/users-guide.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/10-2-1-client-message-api.mddocs/users-guide.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/10-2-1-client-message-api.mddocs/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/execplans/10-2-1-client-message-api.mddocs/users-guide.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/client/mod.rssrc/client/builder.rstests/steps/client_messaging_steps.rssrc/client/runtime.rssrc/client/socket_option_methods.rstests/worlds/client_messaging.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/client/mod.rssrc/client/builder.rstests/steps/client_messaging_steps.rssrc/client/runtime.rssrc/client/socket_option_methods.rstests/worlds/client_messaging.rs
🧠 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/client/mod.rssrc/client/builder.rstests/steps/client_messaging_steps.rssrc/client/runtime.rssrc/client/socket_option_methods.rstests/worlds/client_messaging.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/client/mod.rssrc/client/builder.rstests/steps/client_messaging_steps.rssrc/client/runtime.rssrc/client/socket_option_methods.rstests/worlds/client_messaging.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/client/mod.rssrc/client/builder.rstests/steps/client_messaging_steps.rssrc/client/runtime.rssrc/client/socket_option_methods.rstests/worlds/client_messaging.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/client_messaging_steps.rs
🧬 Code graph analysis (4)
src/client/builder.rs (6)
tests/worlds/client_lifecycle.rs (1)
new(41-41)src/rewind_stream.rs (1)
new(25-31)src/client/preamble_exchange.rs (1)
new(29-36)tests/client_preamble.rs (1)
new(33-38)tests/worlds/client_preamble.rs (1)
new(37-42)tests/connection.rs (2)
new(49-54)new(100-105)
tests/steps/client_messaging_steps.rs (1)
tests/worlds/correlation.rs (1)
expected(45-45)
src/client/runtime.rs (2)
tests/worlds/client_messaging.rs (2)
send_envelope(133-139)call_correlated(145-160)tests/multi_packet_streaming.rs (1)
parts(111-111)
src/client/socket_option_methods.rs (1)
src/client/builder.rs (1)
serializer(136-141)
🪛 LanguageTool
docs/execplans/10-2-1-client-message-api.md
[uncategorized] ~68-~68: Possible missing article found.
Context: ...sures uniqueness within a connection. Counter starts at 1 so that 0 can be distinguis...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~69-~69: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...tinguished from auto-generated IDs if needed. Date/Author: 2026-01-09 (Codex). - De...
(EN_REPEATEDWORDS_NEED)
[uncategorized] ~90-~90: Possible missing comma found.
Context: ...gured Serializer for encoding/decoding but do not handle correlation identifiers. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~96-~96: To join two clauses or introduce examples, consider using an em dash.
Context: ...n. The Envelope contains: - id: u32 - Message type ID for routing - `correlati...
(DASH_RULE)
[typographical] ~97-~97: To join two clauses or introduce examples, consider using an em dash.
Context: ... routing - correlation_id: Option<u64> - Optional correlation ID for request/resp...
(DASH_RULE)
[typographical] ~99-~99: To join two clauses or introduce examples, consider using an em dash.
Context: ...response matching - payload: Vec<u8> - Serialised message payload The `Correla...
(DASH_RULE)
[style] ~99-~99: Would you like to use the Oxford spelling “Serialized”? The spelling ‘Serialised’ is also correct.
Context: ...ponse matching - payload: Vec<u8> - Serialised message payload The `CorrelatableFrame...
(OXFORD_SPELLING_Z_NOT_S)
[typographical] ~106-~106: To join two clauses or introduce examples, consider using an em dash.
Context: ...y references: - src/client/runtime.rs - Client runtime implementation - `src/cli...
(DASH_RULE)
[typographical] ~107-~107: To join two clauses or introduce examples, consider using an em dash.
Context: ...e implementation - src/client/error.rs - Client error types - `src/app/envelope.r...
(DASH_RULE)
[typographical] ~108-~108: To join two clauses or introduce examples, consider using an em dash.
Context: ...ient error types - src/app/envelope.rs - Envelope and Packet trait definitions - ...
(DASH_RULE)
[typographical] ~109-~109: To join two clauses or introduce examples, consider using an em dash.
Context: ...trait definitions - src/correlation.rs - CorrelatableFrame trait - `docs/multi-pa...
(DASH_RULE)
[typographical] ~110-~110: To join two clauses or introduce examples, consider using an em dash.
Context: ...acket-and-streaming-responses-design.md` - Correlation ID requirements - Testing ...
(DASH_RULE)
[uncategorized] ~141-~141: Do not mix variants of the same word (‘serialize’ and ‘serialise’) within a single text.
Context: ...rate correlation ID if not present - Serialize and send the envelope - Return the c...
(EN_WORD_COHERENCY)
[style] ~178-~178: Consider removing “of” to be more concise
Context: ...ion and Acceptance Acceptance requires all of the following: - New public methods compil...
(ALL_OF_THE)
[style] ~210-~210: Consider using a different verb for a more formal wording.
Context: ... can be re-run safely. If a step fails, fix the underlying issue and re-run only th...
(FIX_RESOLVE)
[style] ~222-~222: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...withCorrelationMismatchvariant. - Modifiedsrc/client/builder.rs` to initialize c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~228-~228: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...client_messaging.rswith test world. - Newtests/steps/client_messaging_steps.rs`...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[typographical] ~236-~236: To join two clauses or introduce examples, consider using an em dash.
Context: ...ient::next_correlation_id(&self) -> u64- Generate unique ID -WireframeClient::s...
(DASH_RULE)
[typographical] ~238-~238: To join two clauses or introduce examples, consider using an em dash.
Context: ...elope: P) -> Result<u64, ClientError>- Send with auto-correlation -WireframeC...
(DASH_RULE)
[typographical] ~240-~240: To join two clauses or introduce examples, consider using an em dash.
Context: ...(&mut self) -> Result<P, ClientError>- Receive typed envelope -WireframeClien...
(DASH_RULE)
[typographical] ~242-~242: To join two clauses or introduce examples, consider using an em dash.
Context: ...request: P) -> Result<P, ClientError>- Request-response with validation -Clie...
(DASH_RULE)
[typographical] ~244-~244: To join two clauses or introduce examples, consider using an em dash.
Context: ... Option, received: Option }` - Error variant for mismatched IDs The me...
(DASH_RULE)
[uncategorized] ~246-~246: Possible missing comma found.
Context: ...IDs The methods depend on the Packet trait which is implemented by Envelope.
(AI_HYDRA_LEO_MISSING_COMMA)
docs/users-guide.md
[style] ~871-~871: Would you like to use the Oxford spelling “deserializes”? The spelling ‘deserialises’ is also correct.
Context: ...e_envelope<P: Packet>()` — Receives and deserialises the next frame as the specified packe...
(OXFORD_SPELLING_Z_NOT_S)
⏰ 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). (3)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
- GitHub Check: build-test
🔇 Additional comments (51)
src/client/mod.rs (1)
21-21: LGTM!The private module declaration is correctly placed and follows the existing pattern of internal modules in this file.
src/client/builder.rs (3)
3-7: LGTM!Clean import grouping with
AtomicU64added alongside otherstd::synctypes.
192-193: LGTM!Clear rationale for the module extraction, aligned with the 400-line file limit guideline.
415-415: LGTM!Correlation counter correctly initialized to 1, allowing 0 to be distinguished from auto-generated IDs as documented in the execution plan.
docs/users-guide.md (2)
856-903: LGTM!Clear documentation of the envelope-aware client messaging API with a practical example demonstrating
call_correlatedusage and automatic correlation ID generation.
905-946: LGTM!Good separation of concerns showing manual correlation handling with
send_envelope/receive_envelopeand comprehensive error handling forCorrelationMismatch.docs/execplans/10-2-1-client-message-api.md (3)
1-43: LGTM!Well-structured execution plan with clear purpose, success criteria, and tracked progress items.
59-80: LGTM!Sound decision log documenting the rationale for additive APIs, per-client counters starting at 1, and explicit correlation validation with error variants.
176-206: LGTM!Comprehensive validation steps with appropriate timeouts and output capture for debugging.
src/client/socket_option_methods.rs (6)
1-10: LGTM!Good module-level documentation explaining the purpose, and clean imports.
12-31: LGTM!Clean implementation of
socket_optionsmethod with proper documentation and example.
33-65: LGTM!
nodelayandkeepalivemethods correctly delegate toSocketOptionswith clear documentation.
67-99: LGTM!
lingerandsend_buffer_sizemethods follow the established pattern with appropriate documentation.
101-131: LGTM!
recv_buffer_sizeandreuseaddrmethods correctly implemented with consistent documentation style.
133-153: Cfg attributes consistent with SocketOptions implementation.The
reuseportmethod is appropriately cfg-gated for platforms supportingSO_REUSEPORT, with identical conditional compilation directives in bothsocket_option_methods.rsand the underlyingSocketOptions::reuseportimplementation inconfig.rs.src/client/runtime.rs (8)
3-6: LGTM!Imports are appropriate for the new atomic correlation counter functionality.
71-72: LGTM!The
AtomicU64counter withpub(crate)visibility is appropriate for thread-safe correlation ID generation.
184-186: LGTM!Good refactor delegating to
receive_internal, addressing the duplication concern raised in PR comments.
281-305: LGTM!Well-structured internal helper that centralizes frame retrieval, error-hook invocation, and deserialization. This cleanly addresses the duplication between
receiveandreceive_envelope.
307-332: LGTM!The
Ordering::Relaxedis appropriate for a simple counter where only atomicity matters. Documentation accurately describes the behaviour.
368-399: LGTM!The conditional rebuild logic correctly handles both auto-generated and pre-existing correlation IDs. Error handling is consistent with other methods.
427-429: LGTM!Clean delegation to
receive_internal, maintaining consistency with thereceivemethod pattern.
463-479: LGTM!Correlation validation logic is correct. The error variant captures both expected and received values for debugging purposes.
tests/worlds/client_messaging.rs (15)
1-20: LGTM!Module documentation and imports are appropriate for the test world.
22-30: LGTM!Clean enum definition with appropriate derives and documentation.
32-47: LGTM!Well-structured test world with appropriate field visibility.
65-80: LGTM!Clean delegation to
start_server_with_modeaddresses the duplication concern.
82-99: LGTM!The server setup correctly binds to an ephemeral port and spawns a task for the frame loop.
101-110: LGTM!Client connection logic is straightforward and correctly propagates errors.
112-127: LGTM!Simple and clear envelope configuration methods.
129-160: LGTM!Both methods correctly delegate to the client and capture results for verification.
162-183: LGTM!The lint expectation is appropriately scoped with a clear reason. Draining echo responses prevents message backlog.
185-191: LGTM!Good use of
.first().copied()for safe access, addressing both the duplication concern and the learning about avoiding direct indexing.
193-215: LGTM!Both methods correctly delegate to
get_first_correlation_id, addressing the duplication flagged in PR comments.
217-264: LGTM!Verification methods are well-structured with clear error messages.
266-304: LGTM!Response verification is thorough, and
abort_servercorrectly uses the take-and-abort pattern.
307-327: LGTM!Clean frame loop implementation with appropriate error handling and logging.
49-63: Cannot substantiate duplication claim. The filetests/client/tests/messaging.rsreferenced in the review does not exist. A search across the test codebase found only oneprocess_framefunction matching this signature intests/worlds/client_messaging.rs(line 50). The otherprocess_frameintests/worlds/codec_stateful.rsis a struct method with a different signature and context.Likely an incorrect or invalid review comment.
tests/steps/client_messaging_steps.rs (13)
1-9: LGTM!Module-level lint expectation is appropriate here since Cucumber requires all step functions to be async.
11-13: LGTM!Imports are appropriate for the step definitions.
15-19: LGTM!Correctly chains server start and client connection with error propagation.
21-25: LGTM!The
Ok(())return is idiomatic for Cucumber steps that cannot fail but must return aResultfor framework compatibility.
27-34: LGTM!Parameter extraction and envelope configuration are correct.
36-44: LGTM!Parameter extraction and delegation are correct.
46-53: LGTM!Correctly aborts the existing server before starting the mismatch server.
55-58: LGTM!Simple delegation to the world method.
60-63: LGTM!Correct delegation to the world method.
65-68: LGTM!Parameter extraction and delegation are correct.
70-78: LGTM!Verification delegations are correct.
80-93: LGTM!All verification steps correctly delegate to their respective world methods.
95-103: LGTM!Final verification steps correctly delegate to their respective world methods.
- Introduce `wireframe_testing::echo_server` module with `ServerMode` enum and `process_frame` function. - Refactor client messaging test code to use shared echo server utilities. - Simplify tests by centralizing frame processing logic. - Improve test maintainability and reduce code duplication. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/worlds/client_messaging.rs`:
- Around line 258-269: The test unnecessarily clones the entire Envelope to get
its payload; instead, access the payload field directly from the referenced
Envelope to avoid cloning and consumption. Replace the use of
response.clone().into_parts().payload() with a direct borrow of the payload
field (e.g., use response.payload.as_slice()) so you compare the byte slice
against expected_payload.as_bytes() without cloning the Envelope or consuming
it; ensure you reference the Envelope struct's payload field and the response
variable when making this change.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
docs/execplans/10-2-1-client-message-api.mddocs/users-guide.mdsrc/client/tests/messaging.rstests/worlds/client_messaging.rswireframe_testing/src/echo_server.rswireframe_testing/src/lib.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/10-2-1-client-message-api.mddocs/users-guide.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/10-2-1-client-message-api.mddocs/users-guide.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/10-2-1-client-message-api.mddocs/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/execplans/10-2-1-client-message-api.mddocs/users-guide.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/client/tests/messaging.rswireframe_testing/src/echo_server.rswireframe_testing/src/lib.rstests/worlds/client_messaging.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/client/tests/messaging.rswireframe_testing/src/echo_server.rswireframe_testing/src/lib.rstests/worlds/client_messaging.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/client/tests/messaging.rswireframe_testing/src/echo_server.rswireframe_testing/src/lib.rstests/worlds/client_messaging.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/client/tests/messaging.rswireframe_testing/src/echo_server.rswireframe_testing/src/lib.rstests/worlds/client_messaging.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/client/tests/messaging.rswireframe_testing/src/echo_server.rswireframe_testing/src/lib.rstests/worlds/client_messaging.rs
🧬 Code graph analysis (3)
src/client/tests/messaging.rs (1)
wireframe_testing/src/echo_server.rs (1)
process_frame(30-43)
wireframe_testing/src/echo_server.rs (2)
src/frame/format.rs (1)
bytes(54-54)tests/multi_packet_streaming.rs (1)
parts(111-111)
wireframe_testing/src/lib.rs (1)
wireframe_testing/src/echo_server.rs (1)
process_frame(30-43)
🪛 LanguageTool
docs/execplans/10-2-1-client-message-api.md
[uncategorized] ~68-~68: Possible missing article found.
Context: ...sures uniqueness within a connection. Counter starts at 1 so that 0 can be distinguis...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~69-~69: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...tinguished from auto-generated IDs if needed. Date/Author: 2026-01-09 (Codex). - De...
(EN_REPEATEDWORDS_NEED)
[uncategorized] ~90-~90: Possible missing comma found.
Context: ...gured Serializer for encoding/decoding but do not handle correlation identifiers. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~96-~96: To join two clauses or introduce examples, consider using an em dash.
Context: ...n. The Envelope contains: - id: u32 - Message type ID for routing - `correlati...
(DASH_RULE)
[typographical] ~97-~97: To join two clauses or introduce examples, consider using an em dash.
Context: ... routing - correlation_id: Option<u64> - Optional correlation ID for request/resp...
(DASH_RULE)
[typographical] ~99-~99: To join two clauses or introduce examples, consider using an em dash.
Context: ...response matching - payload: Vec<u8> - Serialized message payload The `Correla...
(DASH_RULE)
[typographical] ~106-~106: To join two clauses or introduce examples, consider using an em dash.
Context: ...y references: - src/client/runtime.rs - Client runtime implementation - `src/cli...
(DASH_RULE)
[typographical] ~107-~107: To join two clauses or introduce examples, consider using an em dash.
Context: ...e implementation - src/client/error.rs - Client error types - `src/app/envelope.r...
(DASH_RULE)
[typographical] ~108-~108: To join two clauses or introduce examples, consider using an em dash.
Context: ...ient error types - src/app/envelope.rs - Envelope and Packet trait definitions - ...
(DASH_RULE)
[typographical] ~109-~109: To join two clauses or introduce examples, consider using an em dash.
Context: ...trait definitions - src/correlation.rs - CorrelatableFrame trait - `docs/multi-pa...
(DASH_RULE)
[typographical] ~110-~110: To join two clauses or introduce examples, consider using an em dash.
Context: ...acket-and-streaming-responses-design.md` - Correlation ID requirements - Testing ...
(DASH_RULE)
[style] ~180-~180: Consider removing “of” to be more concise
Context: ...ion and Acceptance Acceptance requires all of the following: - New public methods compil...
(ALL_OF_THE)
[style] ~212-~212: Consider using a different verb for a more formal wording.
Context: ... can be re-run safely. If a step fails, fix the underlying issue and re-run only th...
(FIX_RESOLVE)
[style] ~224-~224: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...withCorrelationMismatchvariant. - Modifiedsrc/client/builder.rs` to initialize c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~230-~230: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...client_messaging.rswith test world. - Newtests/steps/client_messaging_steps.rs`...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[typographical] ~238-~238: To join two clauses or introduce examples, consider using an em dash.
Context: ...ient::next_correlation_id(&self) -> u64- Generate unique ID -WireframeClient::s...
(DASH_RULE)
[typographical] ~240-~240: To join two clauses or introduce examples, consider using an em dash.
Context: ...elope: P) -> Result<u64, ClientError>- Send with auto-correlation -WireframeC...
(DASH_RULE)
[typographical] ~242-~242: To join two clauses or introduce examples, consider using an em dash.
Context: ...(&mut self) -> Result<P, ClientError>- Receive typed envelope -WireframeClien...
(DASH_RULE)
[typographical] ~244-~244: To join two clauses or introduce examples, consider using an em dash.
Context: ...request: P) -> Result<P, ClientError>- Request-response with validation -Clie...
(DASH_RULE)
[typographical] ~246-~246: To join two clauses or introduce examples, consider using an em dash.
Context: ... Option, received: Option }` - Error variant for mismatched IDs The me...
(DASH_RULE)
[uncategorized] ~248-~248: Possible missing comma found.
Context: ...IDs The methods depend on the Packet trait which is implemented by Envelope.
(AI_HYDRA_LEO_MISSING_COMMA)
🔍 Remote MCP Deepwiki
Summary of Additional Context Found
Based on my search of the wireframe repository documentation, here is the relevant context for reviewing this PR:
Envelope and Packet Architecture
The Envelope is the default implementation of the Packet trait, used to wrap incoming and outgoing messages, containing a message ID, an optional correlation ID, and the payload bytes. The Packet trait requires methods for id(), correlation_id(), into_parts(), and from_parts(), and Envelope provides From implementations to convert to and from PacketParts. This confirms that the PR's use of Option<u64> correlation_id in the Envelope structure aligns with the existing packet abstraction.
Serialization and Framing
Packets are serialized and deserialized using a Serializer implementation, such as BincodeSerializer, and then framed by a FrameProcessor like LengthPrefixedProcessor. The FrameProcessor trait has two main methods: decode() which attempts to extract a complete logical frame from a buffer, and encode() which takes a logical frame and encodes it into the destination buffer with framing information. The PR's use of LengthDelimitedCodec in test servers aligns with this architecture.
Test Patterns and Fixtures
The wireframe codebase utilizes rstest fixtures and a dedicated wireframe_testing crate to facilitate testing of client and server message handling. The wireframe_testing crate offers helpers like drive_with_bincode(), drive_with_frame(), run_app(), and a TestSerializer trait for test scenarios. This provides context for understanding why the PR introduces a wireframe_testing::echo_server module to consolidate test utilities.
WireframeClient Architecture
The WireframeClient is intended to provide an ergonomic Rust client library for interacting with wireframe servers with features like automatic correlation ID management for request/response matching. This directly aligns with the PR's objective of adding correlation ID support and envelope-aware methods.
⏰ 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 (31)
src/client/tests/messaging.rs (13)
1-19: LGTM! Well-structured module header with appropriate imports.The module-level doc comment explains the module's purpose, and the imports are cleanly organised. The use of shared utilities from
wireframe_testingaddresses previous duplication concerns.
21-45: LGTM! Clean test server abstraction.The
spawn_test_serverhelper properly centralises the TCP listener setup, accept loop, and frame processing. The use ofServerModeparameter enables reuse across echo and mismatch scenarios.
47-56: LGTM! Thin semantic wrappers provide clarity.These convenience functions delegate to the generic helper whilst providing descriptive names for test readability.
58-85: LGTM! Correctly validates sequential correlation ID generation.The test establishes a connection and verifies that
next_correlation_idreturns sequential values starting from 1, matching the design decision documented in the execution plan.
87-109: LGTM! Validates auto-generation of correlation IDs.The test correctly verifies that an envelope without a correlation ID receives an auto-generated positive ID upon sending.
111-131: LGTM! Validates preservation of explicit correlation IDs.The test correctly verifies that explicitly set correlation IDs are preserved rather than overwritten.
133-158: LGTM! Comprehensive round-trip envelope validation.The test verifies that the echoed response preserves the correlation ID, message ID, and payload correctly.
160-181: LGTM! Validates successful correlated call with matching IDs.The test confirms that
call_correlatedsucceeds when the server echoes the correct correlation ID.
183-209: LGTM! Validates error handling for correlation mismatches.The test correctly verifies that
call_correlatedreturnsCorrelationMismatchwith diagnostic information when the server returns a different correlation ID.
211-239: LGTM! Validates error hook invocation on correlation mismatch.The test correctly verifies that the error hook is invoked when a correlation mismatch occurs, using atomic counters for thread-safe counting.
241-276: LGTM! Validates uniqueness of correlation IDs across sequential calls.The test correctly verifies that each call receives a unique correlation ID by collecting them and checking for duplicates.
278-300: LGTM! Parameterised payload size testing.Good use of
rstestfor parameterised test cases covering empty, medium, and larger payloads. This aligns with the coding guideline to replace duplicated tests with#[rstest(...)]parameterised cases.
302-327: LGTM! Validates disconnection error handling.The test correctly verifies that
receive_envelopereturnsClientError::Disconnectedwhen the server closes the connection.wireframe_testing/src/lib.rs (1)
21-26: LGTM! Clean module addition and re-exports.The
echo_servermodule is properly declared and its public items (ServerMode,process_frame) are re-exported at the crate root, following the existing pattern.docs/execplans/10-2-1-client-message-api.md (3)
163-169: Acknowledge Cucumber references pending migration.Step 9 correctly documents the current Cucumber infrastructure with an explicit note referencing ADR-003 for the planned rstest-bdd migration. This provides accurate context for the transitional state.
15-43: LGTM! Clear success criteria and accurate progress tracking.The observable success criteria are well-defined, and the progress checklist accurately reflects the completed work with appropriate dates.
45-248: LGTM! Comprehensive execution plan structure.The document provides thorough context, decision rationale, validation commands, and interface documentation. The static analysis suggestions about em dashes are stylistic preferences; the current hyphen format is acceptable for technical documentation.
docs/users-guide.md (2)
856-877: LGTM! Clear API documentation with correct Oxford spelling.The section header and method descriptions are well-structured. The Oxford spelling "deserializes" is correctly used, and em dashes separate method signatures from descriptions as previously suggested.
878-946: LGTM! Well-structured code examples with appropriate fence attributes.The three examples progressively demonstrate basic usage, manual correlation control, and error handling. The
no_runandignoreattributes correctly prevent doctest failures for incomplete snippets.wireframe_testing/src/echo_server.rs (3)
1-11: LGTM! Well-documented shared test utility module.The module-level documentation clearly explains the purpose, and the imports are minimal and focused. This module addresses the duplication concern raised in past reviews by providing a single source of truth for
ServerModeandprocess_frame.
13-21: LGTM! Clean enum definition with sensible defaults.The
ServerModeenum is appropriately minimal withCopyfor cheap cloning andEchoas the default variant.
23-43: LGTM! Robust frame processing with proper error handling.The function uses
#[must_use]appropriately, handles errors viaOptionpropagation without panicking, and thewrapping_add(999)produces deterministic mismatches for testing. This follows the guideline to avoid.expect()and.unwrap()outside of tests.tests/worlds/client_messaging.rs (9)
1-21: LGTM!Module-level documentation is present, imports are well-organised, and the conditional compilation for non-loom builds is appropriate for this test world.
22-37: LGTM!The struct is well-documented with appropriate field visibility. The public
responsefield enables step definitions to access response data directly.
39-73: LGTM!Server startup methods are clean delegations with proper async error handling. The
start_server_with_modecorrectly binds to localhost, spawns the frame loop, and stores the handle for later cleanup.
75-101: LGTM!Client connection and envelope configuration methods are straightforward, properly propagate errors, and provide clear documentation.
103-157: LGTM!Envelope transmission methods correctly capture correlation IDs and handle both success and error cases. The
#[expect]attribute for truncation is properly scoped with a clear reason.
159-165: LGTM!Good use of
.first().copied()for safe access to the correlation ID vector, following the pattern from prior learnings to avoid panics from direct indexing.
167-238: LGTM!Verification methods are well-documented and use proper error propagation. The uniqueness check via sort-dedup is an efficient O(n log n) approach.
273-278: LGTM!The method has been renamed to
abort_serverper the prior review comment, accurately reflecting its behaviour of aborting rather than awaiting.
281-301: LGTM!The frame loop is cleanly implemented with appropriate error handling. It correctly delegates frame processing to the shared
wireframe_testing::process_frameutility, reducing duplication across test modules.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@coderabbitai Have the following now been resolved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1732 🏁 Script executed: Length of output: 230 🏁 Script executed: Length of output: 288 🏁 Script executed: Length of output: 743 🏁 Script executed: Length of output: 1276 🏁 Script executed: Length of output: 655 🏁 Script executed: Length of output: 69 🏁 Script executed: Length of output: 910
✅ Resolved IssuesComment 1 - Double let existing = envelope.correlation_id();
let correlation_id = existing.unwrap_or_else(|| self.next_correlation_id());
let envelope = if existing.is_none() {Comment 2 - Tautological test: Comment 3 - artifacts/artefacts spelling: Comment 4 - Add tests:
Comment 5 - ExecPlan acronym expansion: Overall comment - Overall comment - Tautological test: ❌ Outstanding IssuesComment 6 - BDD acronym expansion: Comment 7 - "Serialized" spelling: Overall comment - AtomicU64 ordering documentation: Summary: 7 out of 10 issues have been resolved. Three documentation issues remain: BDD acronym expansion, en-GB spelling of "Serialised", and AtomicU64 ordering rationale. |
Added a new method `payload_bytes` to the `Envelope` and `PacketParts` structs to provide access to raw payload bytes via a slice reference. This addition simplifies read-only access to payload data without consuming the structs. Corresponding tests updated to use the new method. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2340
✅ Resolved IssuesComment 6 - BDD acronym expansion: Comment 7 - "Serialized" spelling: ❌ Still OutstandingOverall comment - AtomicU64 ordering documentation:
The current documentation describes what the method does and provides a usage example, but doesn't justify the memory ordering choice or document the intended concurrency semantics. Recommendation: Add a comment above the // We use Relaxed ordering because we only need atomicity for the counter increment;
// there are no other memory operations that need to be ordered relative to this.
// Each correlation ID only needs to be unique within this client instance.Summary: 2 out of 3 issues have been resolved. Only the AtomicU64 ordering documentation remains outstanding. |
…tion support - Introduced a new `messaging` module providing envelope send/receive with automatic correlation ID generation and validation. - Moved correlation ID generation and envelope send/receive functionality from `runtime.rs` to the new module. - Added methods on `WireframeClient` for sending envelopes with auto-set correlation IDs, receiving envelopes, and correlated request-response pattern. - Updated documentation to specify use of relaxed atomic ordering for correlation ID increments. - Updated tests to use a shared test envelope supporting correlation IDs. This change modularizes the client messaging logic, improving code separation and robustness of correlation handling in client communications. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Clarified that the counter uses AtomicU64 with Ordering::Relaxed and is thread-safe. Added explanation about typical usage with a single task holding `&self` at a time to the concurrency section of the documentation. Also updated formatting in test common module allowing attribute for clarity but this is a minor style change. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/execplans/10-2-1-client-message-api.md`:
- Around line 70-72: The sentence mentioning Ordering::Relaxed uses the British
spelling "synchronisation"; update that word to the Oxford spelling
"synchronization" so the line reads "...not cross-thread ordering or
synchronization with other memory operations," ensuring the rest of the sentence
and punctuation remain unchanged and only the word "synchronisation" is
replaced.
In `@tests/common/mod.rs`:
- Line 42: Change the doc comment text "Serialised message payload." to use
Oxford spelling "Serialized" so it reads "Serialized message payload." — update
the doc comment above the relevant item in tests/common/mod.rs (the comment
currently containing "Serialised message payload.") to use "Serialized".
♻️ Duplicate comments (4)
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md (2)
25-32: Remove out-of-scope formatting changes from this PR.This PR implements roadmap item 10.2.1 (client message API with correlation IDs), yet modifies the ExecPlan for roadmap item 9.1.2 (CodecError taxonomy). The changes here are purely formatting (wrapping bullet points to comply with the 80-column guideline) and are unrelated to the PR's stated purpose. Revert these formatting-only edits or move them to a separate housekeeping PR to maintain clear change boundaries.
🤖 Prompt for AI Agents
In `@docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md` around lines 25-32, revert the formatting-only changes that wrap the bullet points for the checklist items referencing src/codec/recovery.rs, WireframeError extensions in src/response.rs and src/app/error.rs, and LengthDelimitedDecoder updates; these line-wrapping edits are unrelated to PR `#419`'s client message API work (roadmap 10.2.1) and should either be removed from this branch or extracted into a separate formatting/housekeeping commit or PR.
40-41: Remove out-of-scope formatting changes from this PR.This line-wrapping edit to the BDD tests checklist item is unrelated to PR
#419's client message API implementation (roadmap 10.2.1). Revert this formatting-only change or move it to a separate housekeeping PR.🤖 Prompt for AI Agents
In `@docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md` around lines 40-41, revert the formatting-only change that wraps the bullet point for the BDD behavioural tests checklist item; this line-wrapping edit is unrelated to PR `#419`'s client message API work (roadmap 10.2.1) and should either be removed from this branch or extracted into a separate formatting/housekeeping commit or PR.docs/execplans/10-2-1-client-message-api.md (2)
166-172: Cucumber references remain pending rstest-bdd migration.The note acknowledging ADR-003 and the pending migration is present, which documents the current state. Ensure the migration is tracked and these references are updated when rstest-bdd is adopted.
102-102: Use Oxford spelling "Serialized".Per en-GB-oxendict guidelines (-ize suffix), change "Serialised" to "Serialized".
Proposed fix
-- `payload: Vec<u8>` - Serialised message payload +- `payload: Vec<u8>` - Serialized message payload
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
docs/execplans/10-2-1-client-message-api.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mdsrc/client/messaging.rssrc/client/mod.rssrc/client/runtime.rstests/common/mod.rstests/lifecycle.rstests/routes.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/client/mod.rstests/common/mod.rstests/routes.rssrc/client/messaging.rstests/lifecycle.rssrc/client/runtime.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/client/mod.rstests/common/mod.rstests/routes.rssrc/client/messaging.rstests/lifecycle.rssrc/client/runtime.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/10-2-1-client-message-api.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.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/10-2-1-client-message-api.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.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/10-2-1-client-message-api.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.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/10-2-1-client-message-api.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
🧠 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/client/mod.rstests/common/mod.rstests/routes.rssrc/client/messaging.rstests/lifecycle.rssrc/client/runtime.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/client/mod.rstests/common/mod.rstests/routes.rssrc/client/messaging.rstests/lifecycle.rssrc/client/runtime.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/client/mod.rstests/common/mod.rstests/routes.rssrc/client/messaging.rstests/lifecycle.rssrc/client/runtime.rs
🧬 Code graph analysis (3)
tests/common/mod.rs (1)
src/app/envelope.rs (11)
correlation_id(126-126)correlation_id(168-168)set_correlation_id(128-130)id(62-62)id(118-118)id(155-155)into_parts(65-65)into_parts(120-120)new(94-100)new(136-142)payload(194-194)
tests/routes.rs (1)
tests/common/mod.rs (3)
id(56-56)correlation_id(47-47)from_parts(62-71)
src/client/messaging.rs (2)
src/frame/format.rs (1)
bytes(54-54)tests/common/mod.rs (1)
correlation_id(47-47)
🪛 LanguageTool
docs/execplans/10-2-1-client-message-api.md
[uncategorized] ~68-~68: Possible missing article found.
Context: ...ensures uniqueness within a connection. Counter starts at 1 so that 0 can be distingu...
(AI_HYDRA_LEO_MISSING_THE)
[style] ~69-~69: This word has been used in one of the immediately preceding sentences. Using a synonym could make your text more interesting to read, unless the repetition is intentional.
Context: ...istinguished from auto-generated IDs if needed. Ordering::Relaxed is sufficient be...
(EN_REPEATEDWORDS_NEED)
[style] ~71-~71: Would you like to use the Oxford spelling “synchronization”? The spelling ‘synchronisation’ is also correct.
Context: ...niqueness, not cross-thread ordering or synchronisation with other memory operations. Date/Au...
(OXFORD_SPELLING_Z_NOT_S)
[uncategorized] ~93-~93: Possible missing comma found.
Context: ...gured Serializer for encoding/decoding but do not handle correlation identifiers. ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~99-~99: To join two clauses or introduce examples, consider using an em dash.
Context: ...n. The Envelope contains: - id: u32 - Message type ID for routing - `correlati...
(DASH_RULE)
[typographical] ~100-~100: To join two clauses or introduce examples, consider using an em dash.
Context: ... routing - correlation_id: Option<u64> - Optional correlation ID for request/resp...
(DASH_RULE)
[typographical] ~102-~102: To join two clauses or introduce examples, consider using an em dash.
Context: ...response matching - payload: Vec<u8> - Serialised message payload The `Correla...
(DASH_RULE)
[style] ~102-~102: Would you like to use the Oxford spelling “Serialized”? The spelling ‘Serialised’ is also correct.
Context: ...ponse matching - payload: Vec<u8> - Serialised message payload The `CorrelatableFrame...
(OXFORD_SPELLING_Z_NOT_S)
[typographical] ~109-~109: To join two clauses or introduce examples, consider using an em dash.
Context: ...y references: - src/client/runtime.rs - Client runtime implementation - `src/cli...
(DASH_RULE)
[typographical] ~110-~110: To join two clauses or introduce examples, consider using an em dash.
Context: ...e implementation - src/client/error.rs - Client error types - `src/app/envelope.r...
(DASH_RULE)
[typographical] ~111-~111: To join two clauses or introduce examples, consider using an em dash.
Context: ...ient error types - src/app/envelope.rs - Envelope and Packet trait definitions - ...
(DASH_RULE)
[typographical] ~112-~112: To join two clauses or introduce examples, consider using an em dash.
Context: ...trait definitions - src/correlation.rs - CorrelatableFrame trait - `docs/multi-pa...
(DASH_RULE)
[typographical] ~113-~113: To join two clauses or introduce examples, consider using an em dash.
Context: ...acket-and-streaming-responses-design.md` - Correlation ID requirements - Testing ...
(DASH_RULE)
[uncategorized] ~144-~144: Do not mix variants of the same word (‘serialize’ and ‘serialise’) within a single text.
Context: ...rate correlation ID if not present - Serialize and send the envelope - Return the c...
(EN_WORD_COHERENCY)
[style] ~183-~183: Consider removing “of” to be more concise
Context: ...ion and Acceptance Acceptance requires all of the following: - New public methods compil...
(ALL_OF_THE)
[style] ~215-~215: Consider using a different verb for a more formal wording.
Context: ... can be re-run safely. If a step fails, fix the underlying issue and re-run only th...
(FIX_RESOLVE)
[style] ~227-~227: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...withCorrelationMismatchvariant. - Modifiedsrc/client/builder.rs` to initialize c...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~232-~232: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...client_messaging.rswith test world. - Newtests/steps/client_messaging_steps.rs`...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[typographical] ~240-~240: To join two clauses or introduce examples, consider using an em dash.
Context: ...ient::next_correlation_id(&self) -> u64- Generate unique ID -WireframeClient::s...
(DASH_RULE)
[typographical] ~242-~242: To join two clauses or introduce examples, consider using an em dash.
Context: ...elope: P) -> Result<u64, ClientError>- Send with auto-correlation -WireframeC...
(DASH_RULE)
[typographical] ~244-~244: To join two clauses or introduce examples, consider using an em dash.
Context: ...(&mut self) -> Result<P, ClientError>- Receive typed envelope -WireframeClien...
(DASH_RULE)
[typographical] ~246-~246: To join two clauses or introduce examples, consider using an em dash.
Context: ...request: P) -> Result<P, ClientError>- Request-response with validation -Clie...
(DASH_RULE)
[typographical] ~248-~248: To join two clauses or introduce examples, consider using an em dash.
Context: ... Option, received: Option }` - Error variant for mismatched IDs The me...
(DASH_RULE)
[uncategorized] ~250-~250: Possible missing comma found.
Context: ...IDs The methods depend on the Packet trait which is implemented by Envelope.
(AI_HYDRA_LEO_MISSING_COMMA)
🔍 Remote MCP Deepwiki
Summary of Relevant Context for PR Review
Based on my investigation of the wireframe codebase, here is the key context relevant to reviewing this PR:
1. Architectural Framework for Correlation IDs
The Packet trait is fundamental to message handling in wireframe, requiring implementations to provide an id for routing and an optional correlation_id. The trait includes methods for correlation_id(&self) -> Option<u64>, into_parts() to decompose packets, and from_parts() to reconstruct them.
The PacketParts struct is used to decompose and reconstruct packet data, including the correlation ID, and provides an inherit_correlation method which ensures a correlation identifier is present, inheriting from a source if missing, or overwriting mismatched values. This pattern directly aligns with the PR's goal of maintaining correlation ID consistency.
2. Atomic Counter Usage Patterns
Atomic counters like AtomicU64 are used in the framework for metrics tracking; for example, ACTIVE_CONNECTIONS is a global counter that is incremented when a connection opens and decremented when it closes, with Ordering::Relaxed being used when only the final count matters, not the precise interleaving of operations.
The PR's use of correlation_counter: AtomicU64 with Ordering::Relaxed (mentioned in commit messages) aligns with this established pattern for thread-safe, high-performance counters.
3. Serialization and Deserialization Framework
The framework uses a pluggable Serializer trait for message conversion, and application-specific types implement the Message trait which provides default to_bytes and from_bytes methods using bincode. The framework uses a FrameProcessor (such as LengthPrefixedProcessor) to convert raw byte streams into discrete logical units before serialization.
This context is important for the PR's implementation of send_envelope and receive_envelope, which must serialize/deserialize frames consistently with the framework's approach.
4. Error Handling Patterns
The framework uses a WireframeError enum with variants for both transport-level I/O errors (Io) and protocol-specific logical errors (Protocol). When a protocol error occurs, the framework invokes a handle_error callback on the installed WireframeProtocol trait before terminating the command.
The PR introduces ClientError::CorrelationMismatch following a similar pattern of domain-specific error variants, and implements error-hook invocation which aligns with this callback-based error handling philosophy.
5. Test Patterns for Async Code
The codebase uses rstest for fixture-based parameterized tests, tokio::test(start_paused = true) for deterministic async testing, and tokio::io::duplex to simulate network connections without actual network I/O.
The PR's addition of unit tests in src/client/tests/messaging.rs and Cucumber BDD tests follows these established patterns, using in-process TCP test servers to validate correlation ID semantics.
6. Backward Compatibility Strategy
Backward compatibility is maintained through careful API design; for example, deprecated methods are retained while newer explicit APIs are provided, and Result return types are used even when no errors are currently returned to allow for future error conditions without breaking the API.
The PR preserves all existing send/receive/call methods on WireframeClient, adding new envelope-aware methods alongside them—this aligns with the framework's backward compatibility approach.
7. Critical Implementation Concerns from Code Review
Based on the PR objectives and review comments summary:
- Duplication Reduction: Multiple helpers and methods duplicate logic for correlation ID retrieval and server spawning. The review identified opportunities to extract private helpers (
get_first_correlation_id,spawn_test_server) to improve maintainability. - Micro-optimizations: The
send_envelopemethod can be optimized to avoid callingenvelope.correlation_id()twice by caching the result. - Concurrency Documentation: While
AtomicU64withOrdering::Relaxedis used correctly, in-code comments justifying the ordering choice were identified as important for reviewer clarity. - Test Correctness: Tautological assertions (comparing responses to derived expected values) were identified; tests now properly store and compare against sent envelopes.
⏰ 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/client/mod.rs (1)
18-22: LGTM!The new
messagingandsocket_option_methodsmodules are wired correctly alongside existing siblings. The internal visibility is appropriate for implementation details.tests/lifecycle.rs (1)
152-172: LGTM!The migration from
StateEnvelopetoCommonTestEnvelopeconsolidates test envelope types appropriately. The test logic correctly verifies correlation ID preservation through the round-trip.src/client/runtime.rs (3)
67-68: LGTM!The
correlation_counter: AtomicU64field is appropriately placed with a clear doc comment. Thepub(crate)visibility allows the messaging module to access it whilst keeping it internal.
180-182: LGTM!The delegation to
receive_internal()consolidates frame-read/deserialize/error-hook logic, addressing the duplication concern from earlier reviews.
35-41: LGTM!The documentation clearly explains correlation identifier behaviour and correctly references the envelope-aware APIs using Rustdoc link syntax.
docs/execplans/10-2-1-client-message-api.md (1)
1-27: Document structure and content are comprehensive.The execution plan correctly documents the feature scope, success criteria, and progress tracking. The decision log properly captures the rationale for key design choices including the
AtomicU64ordering.tests/common/mod.rs (2)
46-72: LGTM!The
CorrelatableFrameandPackettrait implementations correctly mirror the patterns established insrc/app/envelope.rs. Thefrom_partsandinto_partsmethods properly handle the conversion betweenCommonTestEnvelopeandPacketParts.
7-11: Current#![allow]approach is correct for this shared test module.Items in
tests/common/mod.rsare used selectively by different test binaries (e.g.,unused_listenerby only 5 of 40 test binaries,factoryby 4). Each test binary compiles the common module independently, so unused items trigger the lint in non-consuming binaries but not in consuming ones. Using#[expect]would fail in binaries where items are actually used (lint doesn't fire), making#[allow]the correct choice here.Likely an incorrect or invalid review comment.
tests/routes.rs (6)
14-26: Keep the CommonTestEnvelope consolidation.
Retain the shared type to avoid duplicated envelope plumbing across the tests.
31-66: Keep the CommonTestEnvelope usage in the echo handler test.
Preserve this conversion to validate correlation ID propagation in the echo path.
74-104: Keep the None-correlation test using CommonTestEnvelope.
Maintain the shared envelope type to validate passthrough when no ID is set.
108-155: Keep the sequential multi-frame test with CommonTestEnvelope.
Maintain the shared envelope type to validate ordered correlation IDs across frames.
159-189: Keep the parameterised correlation propagation test with CommonTestEnvelope.
Maintain the shared envelope type while varying correlation IDs.
193-202: Keep the PacketParts round-trip using CommonTestEnvelope.
Retain this coverage to confirm the common packet type reconstructs cleanly.src/client/messaging.rs (7)
1-5: Keep the module summary.
Retain the high-level description so the envelope API purpose is explicit.
24-64: Keep the correlation ID generator.
Retain the concise method for sequential ID allocation.
100-125: Keep the in-place correlation stamping and send flow.
Preserve this logic to return the actual correlation ID used.
153-155: Keep the receive_envelope wrapper.
Retain the thin delegate to the shared receive path.
189-205: Keep the correlation mismatch validation.
Preserve the explicit mismatch error before returning the response.
207-231: Keep the shared receive/deserialize path.
Retain this helper to centralise error-hook invocation.
233-238: Keep the optional error-hook dispatch.
Retain the guard to avoid unnecessary awaits when no handler is set.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Update British spellings to Oxford English: - synchronisation → synchronization in execplan concurrency docs - Serialised → Serialized in test envelope doc comment 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/execplans/10-2-1-client-message-api.md`:
- Line 24: Replace all Cucumber mentions with rstest-bdd per ADR-003: change
line 24 to "rstest-bdd behavioural tests validate the messaging flow.", update
the progress item at line 39 to mention adding rstest-bdd behavioural tests with
a .feature file, swap the docs reference at line 116 from
docs/behavioural-testing-in-rust-with-cucumber.md to
docs/rust-testing-with-rstest-fixtures.md, change line 124 to "Add rstest-bdd
behavioural tests.", and rewrite Step 9 (lines 166-172) to instruct keeping
tests/features/client_messaging.feature, replacing Cucumber world/runner
registration (tests/cucumber.rs and tests/world.rs) with rstest-bdd fixture
functions using #[given], #[when], #[then] attributes, document running
scenarios via cargo test, and cite ADR-003 as the rationale for using
rstest-bdd.
In `@tests/common/mod.rs`:
- Around line 62-71: The from_parts function creates unnecessary temporary
bindings for id, correlation_id, and payload; replace them by constructing the
struct directly from the PacketParts accessors (call parts.id(),
parts.correlation_id(), parts.payload() inline) so the function simply returns
Self { id: parts.id(), correlation_id: parts.correlation_id(), payload:
parts.payload() } without the intermediate variables.
- Around line 7-11: Remove the file-level #![allow(dead_code, reason = "...")]
from tests/common/mod.rs and instead apply narrowly scoped solutions: for each
genuinely unused item in this module, add an item-level #[expect(dead_code,
reason = "...")] attribute on that function/struct/const (or gate the item
behind a cargo feature or cfg(test) so it isn't compiled into binaries that
don't use it); if you can't avoid a global suppression as a last resort, replace
the blanket allow with a documented FIXME comment linking a tracking issue and
explaining why a file-wide exception is necessary.
♻️ Duplicate comments (2)
docs/execplans/10-2-1-client-message-api.md (2)
68-69: Add article before "Counter" for better grammar.The sentence "Counter starts at 1" would read more naturally as "The counter starts at 1". This is a minor grammar improvement flagged by static analysis.
Proposed fix
Avoids global contention and ensures uniqueness within a connection. - Counter starts at 1 so that 0 can be distinguished from auto-generated IDs if needed. + The counter starts at 1 so that 0 can be distinguished from auto-generated IDs if needed.
102-102: Use Oxford spelling "Serialized" instead of "Serialised".The coding guideline mandates en-GB-oxendict spelling, which uses the -ize suffix. Line 102 uses "Serialised" (British -ise variant) whilst line 144 correctly uses "Serialize", creating word coherency issues.
Proposed fix
-- `payload: Vec<u8>` - Serialised message payload +- `payload: Vec<u8>` - Serialized message payload🤖 Prompt for AI agents
In `@docs/execplans/10-2-1-client-message-api.md` at line 102, replace "Serialised message payload" with "Serialized message payload" to conform to the en-GB-oxendict spelling guideline which mandates the -ize suffix.
- Add article "The" before "counter starts at 1" for better grammar - Change "Serialised" to "Serialized" (Oxford spelling) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…tionale - Inline PacketParts accessor calls in from_parts, removing temporary bindings - Replace blanket dead_code allow with documented file-level suppression explaining why expect cannot be used (inconsistent lint triggers across test binaries that share the common module) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace informal comment with FIXME(#333) linking to the tracking issue for forbidden #[allow] attributes. Document why conditional compilation and #[expect] are not viable alternatives: - Cargo features cannot coordinate across independent test binaries - Each integration test compiles as a separate crate with its own "used" view - #[cfg(test)] has no effect in tests/ directory - #[expect] fails with "unfulfilled expectation" in binaries that use the type This is a known Rust limitation when sharing utilities across multiple integration test binaries. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/common/mod.rs`:
- Around line 73-88: Remove the redundant #[inline] attribute on the id method
in the impl Packet for CommonTestEnvelope; locate the impl block for the Packet
trait on CommonTestEnvelope and delete the #[inline] placed above fn id(&self)
-> u32 { self.id } so the getter remains a simple trivial method without the
unnecessary attribute (other methods into_parts and from_parts can remain
unchanged).
The #[inline] attribute on the id() method in impl Packet for CommonTestEnvelope is unnecessary - the compiler will inline trivial single-expression getters automatically. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 participant
Summary
Changes
Why this is needed
How it works
Validation plan
Artifacts touched (highlights)
📎 Task: https://www.terragonlabs.com/task/9818bf98-08e1-44b1-972e-03b4897b9d42