Phase 8: Wireframe Client with per-connection state and lifecycle hooks#411
Phase 8: Wireframe Client with per-connection state and lifecycle hooks#411
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduce asynchronous client lifecycle hooks: add on_connection_setup, on_connection_teardown and on_error; add connection-state generic parameter C to builder and client types; wire lifecycle hooks through builder into runtime; update docs; add unit and BDD tests exercising setup, teardown, error and preamble interactions. (33 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Builder as WireframeClientBuilder
participant Client as WireframeClient
participant Transport as TcpStream
participant Server as Server
rect rgb(245,250,255)
Note over App,Builder: Configure builder with hooks
App->>Builder: on_connection_setup(setup_fn)
App->>Builder: on_connection_teardown(teardown_fn)
App->>Builder: on_error(error_fn)
end
rect rgb(240,255,240)
Note over App,Builder: Connect flow
App->>Builder: connect(addr)
Builder->>Transport: Establish TCP connection
Transport->>Server: TCP accept/handshake
alt Preamble exchange
Builder->>Transport: Send/receive preamble
Transport->>Server: Preamble messages
Server-->>Transport: Preamble ack
end
Builder->>Builder: Invoke setup_fn() -> Future[C]
Builder->>Builder: Await -> connection_state(C)
Builder->>Client: Construct WireframeClient with state & hooks
end
rect rgb(255,250,240)
Note over App,Client: Active messaging
loop
App->>Client: send(msg) / receive()
alt send/receive error
Client->>Client: invoke on_error(err)
Client-->>App: return Err
else success
Client->>Transport: I/O & framing
end
end
end
rect rgb(255,245,238)
Note over App,Client: Close / Teardown
App->>Client: close()
Client->>Transport: flush & close
Client->>Client: if connection_state present -> invoke on_disconnect(state)
Client->>Client: await teardown completion
Client-->>App: closed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (1)📚 Learning: 2026-01-03T17:44:28.792ZApplied to files:
⏰ 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)
🔇 Additional comments (2)
Comment |
Reviewer's GuideAdds client-side lifecycle hooks with type-safe per-connection state to WireframeClient, wires them through the builder, runtime, and preamble flow, and introduces tests and docs to validate setup/teardown/error hooks and their interaction with preamble callbacks. Class diagram for updated WireframeClient lifecycle hooksclassDiagram
class WireframeClientBuilder~S,P,C~ {
+S serializer
+ClientCodecConfig codec_config
+SocketOptions socket_options
+Option~PreambleConfig~P~~ preamble_config
+LifecycleHooks~C~ lifecycle_hooks
+new() WireframeClientBuilder~BincodeSerializer,(),()~
+serializer(serializer Ser) WireframeClientBuilder~Ser,P,C~
+with_preamble(preamble Q) WireframeClientBuilder~S,Q,C~
+on_connection_setup(f F) WireframeClientBuilder~S,P,C2~
+on_connection_teardown(f F) WireframeClientBuilder~S,P,C~
+on_error(f F) WireframeClientBuilder~S,P,C~
+connect(addr SocketAddr) WireframeClient~S,RewindStream~TcpStream~,C~
}
class WireframeClient~S,T,C~ {
+Framed~T,LengthDelimitedCodec~ framed
+S serializer
+ClientCodecConfig codec_config
+Option~C~ connection_state
+Option~ClientConnectionTeardownHandler~C~~ on_disconnect
+Option~ClientErrorHandler~ on_error
+builder() WireframeClientBuilder~BincodeSerializer,(),()~
+send(message M) Result~(),ClientError~
+receive() Result~M,ClientError~
+call(request R) Result~Response,ClientError~
+stream() &T
+rewind_stream() &RewindStream~TcpStream~
+close() Future~()~
-invoke_error_hook(error ClientError)
}
class LifecycleHooks~C~ {
+Option~ClientConnectionSetupHandler~C~~ on_connect
+Option~ClientConnectionTeardownHandler~C~~ on_disconnect
+Option~ClientErrorHandler~ on_error
+new() LifecycleHooks~C~
}
class ClientConnectionSetupHandler~C~ {
<<typealias>>
+Fn() -> Future~C~
}
class ClientConnectionTeardownHandler~C~ {
<<typealias>>
+Fn(C) -> Future~()~
}
class ClientErrorHandler {
<<typealias>>
+for_all_lifetimes Fn(&ClientError) -> Future~()~
}
WireframeClientBuilder "1" *-- "1" LifecycleHooks : has
WireframeClient "1" o-- "1" ClientConnectionTeardownHandler : uses
WireframeClient "1" o-- "1" ClientErrorHandler : uses
LifecycleHooks "1" o-- "0..1" ClientConnectionSetupHandler : holds
LifecycleHooks "1" o-- "0..1" ClientConnectionTeardownHandler : holds
LifecycleHooks "1" o-- "0..1" ClientErrorHandler : holds
WireframeClientBuilder ..> WireframeClient : builds
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +221 to +246 async fn setup_callback_invoked_on_connect() {
let setup_count = Arc::new(AtomicUsize::new(0));
let count = setup_count.clone();
let (addr, accept) = spawn_listener().await;
let _client = WireframeClient::builder()
.on_connection_setup(move || {
let count = count.clone();
async move {
count.fetch_add(1, Ordering::SeqCst);
42u32
}
})
.connect(addr)
.await
.expect("connect client");
let _server = accept.await.expect("join accept task");
assert_eq!(
setup_count.load(Ordering::SeqCst),
1,
"setup callback should be invoked exactly once on connect"
);
}❌ 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: tests/worlds/client_lifecycle.rs Comment on lines +72 to +84 pub async fn start_standard_server(&mut self) -> TestResult {
let listener = TcpListener::bind("127.0.0.1:0").await?;
let addr = listener.local_addr()?;
let handle = tokio::spawn(async move {
let (_stream, _) = listener.accept().await.expect("accept");
// Hold connection briefly
tokio::time::sleep(Duration::from_millis(100)).await;
});
self.addr = Some(addr);
self.server = Some(handle);
Ok(())
}❌ 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 outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +242 to +262 async fn setup_callback_invoked_on_connect() {
let setup_count = Arc::new(AtomicUsize::new(0));
let count = setup_count.clone();
let _client = test_with_client(|builder| {
builder.on_connection_setup(move || {
let count = count.clone();
async move {
count.fetch_add(1, Ordering::SeqCst);
42u32
}
})
})
.await;
assert_eq!(
setup_count.load(Ordering::SeqCst),
1,
"setup callback should be invoked exactly once on connect"
);
}❌ 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: tests/worlds/client_lifecycle.rs Comment on lines +168 to +185 pub async fn connect_with_setup(&mut self) -> TestResult {
let addr = self.addr.ok_or("server address missing")?;
let setup_count = Arc::clone(&self.setup_count);
let result = WireframeClient::builder()
.on_connection_setup(move || {
let count = setup_count.clone();
async move {
count.fetch_add(1, Ordering::SeqCst);
42u32
}
})
.connect(addr)
.await;
self.handle_connection_result(result);
Ok(())
}❌ 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.
leynos
force-pushed
the
terragon/wireframe-client-runtime-debo0u
branch
from
09a98df to
21629e4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationtests/worlds/client_lifecycle.rs: What lead to degradation?The module contains 2 functions with similar structure: ClientLifecycleWorld.connect_with_error_callback,ClientLifecycleWorld.connect_with_setup Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationtests/worlds/client_lifecycle.rs: What lead to degradation?The module contains 2 functions with similar structure: ClientLifecycleWorld.connect_with_error_callback,ClientLifecycleWorld.connect_with_setup Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11463 @leynos: The duplication between Current implementation:
The helper extracts the repeated pattern: retrieve address → apply builder configuration → connect → handle result. Remaining structural similarityThe methods share a pattern:
This is intentional and appropriate. Each method tests a different lifecycle hook (setup, error, teardown, preamble), so some structural similarity is expected. Further abstraction would:
RecommendationThe current abstraction level strikes the right balance. CodeScene may be detecting legitimate structural similarity, but it's at an acceptable level for test code where clarity and explicit configuration are valued. If you'd like to suppress this warning, consider:
The code already follows DRY principles by extracting the varying behaviour (builder configuration) into closures whilst reusing the common connection scaffolding. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +332 to +362 async fn error_callback_invoked_on_receive_error() {
let error_count = Arc::new(AtomicUsize::new(0));
let count = error_count.clone();
let (addr, accept) = spawn_listener().await;
let mut client = WireframeClient::builder()
.on_error(move |_err| {
let count = count.clone();
async move {
count.fetch_add(1, Ordering::SeqCst);
}
})
.connect(addr)
.await
.expect("connect client");
// Drop the server side to cause a disconnection
let server = accept.await.expect("join accept task");
drop(server);
// Try to receive - should fail and invoke error hook
let result: Result<Vec<u8>, ClientError> = client.receive().await;
assert!(result.is_err(), "receive should fail after disconnect");
assert_eq!(
error_count.load(Ordering::SeqCst),
1,
"error callback should be invoked on receive error"
);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
leynos
force-pushed
the
terragon/wireframe-client-runtime-debo0u
branch
from
a3bb423 to
cb7dbee
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/worlds/mod.rs (1)
1-8: Update module documentation to mention client lifecycle tests.The module doc comment lists specific scenarios (fragmentation, correlation, panic recovery, etc.) but omits the newly added
client_lifecycleworld. Add client lifecycle hooks to the list for completeness://! Cucumber test world implementations and shared helpers. //! -//! Provides world types for behaviour-driven tests covering fragmentation, -//! correlation, panic recovery, stream termination, multi-packet channels, -//! stateful codecs, request parts, and message assembler parsing. Shared +//! Provides world types for behaviour-driven tests covering client lifecycle +//! hooks, fragmentation, correlation, panic recovery, stream termination, +//! multi-packet channels, stateful codecs, request parts, and message assembler parsing. Shared //! utilities like `build_small_queues` keep individual worlds focused on their //! respective scenarios.src/client/preamble_builder.rs (1)
42-48: Consider struct update syntax for builder methods.The PR objectives note that builder methods could be simplified using struct update syntax (
..self) rather thanmut selffollowed by return. This would make the builder pattern more functional and align with Rust idioms for immutable data.🔎 Example refactor using struct update syntax
- pub fn preamble_timeout(mut self, duration: Duration) -> Self { + pub fn preamble_timeout(self, duration: Duration) -> Self { let normalised = duration.max(Duration::from_millis(1)); - if let Some(ref mut config) = self.preamble_config { - config.set_timeout(normalised); - } - self + Self { + preamble_config: self.preamble_config.map(|mut cfg| { + cfg.set_timeout(normalised); + cfg + }), + ..self + } }
🤖 Fix all issues with AI agents
In @src/client/tests/error_handling.rs:
- Around line 101-141: The fixed 10ms sleep in the test
error_callback_invoked_on_send_io_error is flaky; replace the single
tokio::time::sleep call with a small retry loop that polls until the server side
is observed closed (or client.send returns an Io error) with a bounded deadline
(e.g., total timeout) and short backoff between attempts so the test waits
deterministically for the disconnect instead of assuming OS timing; update the
wait before calling client.send (around where accept is dropped and before
client.send) to use this retry/polling approach, referencing the test function
name, the accept/ server drop, error_count, and client.send to locate the
change.
In @src/client/tests/helpers.rs:
- Around line 68-96: Extract a module-level type alias for the counting-hook
return type and update counting_hook to use it: add a generic alias like `type
CountingHook<T> = (Arc<AtomicUsize>, Arc<dyn Fn(T) -> Pin<Box<dyn Future<Output
= T> + Send>> + Send + Sync>);` then change `pub fn counting_hook<T>() -> (...)`
to `pub fn counting_hook<T>() -> CountingHook<T>` and wrap the `increment`
closure with `Arc::new(...)` before returning so the closure is a clonable
Arc<dyn Fn...> matching the alias.
In @src/client/tests/mod.rs:
- Around line 2-5: Remove the module-level #[expect(clippy::excessive_nesting,
...)] in src/client/tests/mod.rs and instead apply the expect annotation
narrowly to only the macro-generated socket option tests by moving the attribute
to the socket_option_test! macro definition in helpers.rs (or by applying it to
each socket_option_test! invocation such as builder_applies_nodelay_option) so
that only the async-closure builder tests are suppressed; keep the same reason
string and ensure no other tests in the module are covered by this lint
expectation.
In @tests/steps/client_lifecycle_steps.rs:
- Around line 43-44: The step function when_client_closes currently has a void
return but other when-steps return TestResult; change its signature to return
TestResult (e.g., async fn when_client_closes(world: &mut ClientLifecycleWorld)
-> TestResult) and propagate/convert any error from world.close_client().await
into a TestResult (return Ok(()) on success or bubble up the error using
?/map_err as appropriate) so its behavior and error reporting match the other
when-steps; update any imports or type aliases if needed to compile.
In @tests/worlds/client_lifecycle.rs:
- Around line 327-341: The two multi-line simple accessor methods
teardown_received_state and preamble_success_invoked should be reformatted as
single-line functions to match style guidelines; keep their existing doc
comments and #[must_use] attributes but collapse the body and signature into one
line each (similar to the existing single-line error_count accessor) so they
read e.g. pub fn teardown_received_state(&self) -> usize {
self.teardown_received_state.load(Ordering::SeqCst) } and similarly for
preamble_success_invoked.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
docs/roadmap.mddocs/users-guide.mddocs/wireframe-client-design.mdsrc/client/builder.rssrc/client/hooks.rssrc/client/mod.rssrc/client/preamble_builder.rssrc/client/runtime.rssrc/client/tests/error_handling.rssrc/client/tests/helpers.rssrc/client/tests/lifecycle.rssrc/client/tests/mod.rstests/client_lifecycle.rstests/cucumber.rstests/features/client_lifecycle.featuretests/steps/client_lifecycle_steps.rstests/steps/mod.rstests/world.rstests/worlds/client_lifecycle.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/preamble_builder.rssrc/client/tests/error_handling.rstests/steps/client_lifecycle_steps.rstests/worlds/mod.rssrc/client/tests/mod.rssrc/client/mod.rstests/steps/mod.rstests/client_lifecycle.rssrc/client/hooks.rstests/cucumber.rstests/world.rstests/worlds/client_lifecycle.rssrc/client/tests/lifecycle.rssrc/client/runtime.rssrc/client/tests/helpers.rssrc/client/builder.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/client/preamble_builder.rssrc/client/tests/error_handling.rstests/steps/client_lifecycle_steps.rstests/worlds/mod.rssrc/client/tests/mod.rssrc/client/mod.rstests/steps/mod.rstests/client_lifecycle.rssrc/client/hooks.rstests/cucumber.rstests/world.rstests/worlds/client_lifecycle.rssrc/client/tests/lifecycle.rssrc/client/runtime.rssrc/client/tests/helpers.rssrc/client/builder.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/users-guide.mddocs/wireframe-client-design.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
Every doctest should validate the public API of a crate from the perspective of an external user, treating each documentation test as a separate temporary crate that imports the library as an external dependency
Files:
docs/roadmap.mddocs/users-guide.mddocs/wireframe-client-design.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes in Markdown.
Files:
docs/roadmap.mddocs/users-guide.mddocs/wireframe-client-design.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/users-guide.mddocs/wireframe-client-design.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 (2)
📚 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/preamble_builder.rssrc/client/tests/error_handling.rstests/steps/client_lifecycle_steps.rstests/worlds/mod.rssrc/client/tests/mod.rssrc/client/mod.rstests/steps/mod.rstests/client_lifecycle.rssrc/client/hooks.rstests/cucumber.rstests/world.rstests/worlds/client_lifecycle.rssrc/client/tests/lifecycle.rssrc/client/runtime.rssrc/client/tests/helpers.rssrc/client/builder.rs
📚 Learning: 2026-01-06T18:53:16.788Z
Learnt from: CR
Repo: leynos/wireframe PR: 0
File: docs/users-guide.md:0-0
Timestamp: 2026-01-06T18:53:16.788Z
Learning: Applies to docs/**/*.rs : Use `with_preamble` on the client builder to send a preamble immediately after TCP connect with optional callbacks for success or failure
Applied to files:
docs/users-guide.md
🧬 Code graph analysis (7)
src/client/tests/error_handling.rs (1)
tests/worlds/client_lifecycle.rs (1)
error_count(335-335)
tests/client_lifecycle.rs (2)
tests/worlds/client_lifecycle.rs (4)
setup_count(321-321)new(41-41)teardown_count(325-325)error_count(335-335)src/client/builder.rs (1)
new(102-110)
src/client/hooks.rs (1)
src/client/builder.rs (1)
default(114-114)
tests/cucumber.rs (1)
src/connection.rs (1)
run(450-480)
tests/worlds/client_lifecycle.rs (4)
tests/client_lifecycle.rs (2)
read_preamble(157-157)read_preamble(172-172)src/preamble.rs (1)
write_preamble(175-193)src/client/builder.rs (1)
new(102-110)tests/worlds/client_preamble.rs (2)
version(46-46)accepted(58-58)
src/client/tests/lifecycle.rs (1)
src/client/tests/helpers.rs (4)
counting_hook(77-96)spawn_listener(21-31)test_error_hook_on_disconnect(102-127)test_with_client(52-66)
src/client/builder.rs (1)
src/client/hooks.rs (1)
default(106-112)
🪛 LanguageTool
docs/wireframe-client-design.md
[uncategorized] ~86-~86: Possible missing comma found.
Context: ...he setup callback must be an FnOnce closure returning a Future that produces C....
(AI_HYDRA_LEO_MISSING_COMMA)
🔍 Remote MCP Deepwiki
Based on my search of the leynos/wireframe repository documentation and codebase, I've gathered comprehensive context about the project. Here's the relevant information for reviewing PR #411:
Project Architecture Summary and
The Wireframe project is a Rust binary protocol server framework with:
- Server-side architecture centered on actor-per-connection model with
WireframeServer,WireframeApp, andConnectionActor - Protocol hooks through
WireframeProtocoltrait for connection lifecycle management - Push messaging system with
PushHandleandSessionRegistryfor server-initiated communication - Comprehensive testing infrastructure including unit, integration, behavioral, and advanced concurrency testing
PR #411 Context: Client Lifecycle Hooks Implementation
The PR adds client-side lifecycle hooks to the Wireframe client, mirroring the server-side architecture. Key observations:
Architectural Alignment
The implementation follows established Wireframe patterns:
- Type-changing builder pattern (similar to server's
on_connection_setupchanging state type) - Generic parameters on builder (
WireframeClientBuilder<S, P, C>) extending existingS, Pgenerics - Lifecycle hook trait implementations matching server-side
WireframeProtocolphilosophy - State management through typed connection state
Cpropagated through builder chain
Code Quality Standards
The repository emphasizes:
- Zero Clippy warnings enforcement in CI/CD
- Comprehensive testing with
rstestfixtures, Cucumber BDD, and behavioral testing - Documentation-first approach with roadmap-driven development
- Resilience patterns including panic isolation, graceful shutdown, and resource management
Critical Implementation Concerns from Review Comments
- Hook preservation: Reviewers noted
on_connection_setupcould drop existing teardown/error hooks - must preserve non-C hooks when changing state type - Async error handling: Non-blocking error-hook invocation is critical (avoid
block_onin async contexts) - Close behavior: Must flush transport before running teardown to maintain proper lifecycle semantics
- Testing coverage: Serialization/deserialization error paths need dedicated tests to ensure error hooks fire consistently
Verification Points for Review
- ✅ State type parameter
Cflows consistently through builder chain - ✅ Preamble wiring order:
preamble exchange → on_preamble_success → on_connection_setup - ✅ Error hook non-blocking invocation pattern
- ✅ Weak reference patterns prevent memory leaks (consistent with server SessionRegistry)
⚠️ Remaining gap: Deterministic serialization-failure test coverage
⏰ 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 (50)
docs/roadmap.md (1)
454-454: Status update aligns with PR implementation.Task 10.1.3 is correctly marked complete, reflecting the lifecycle hooks (setup, teardown, and error) now exposed in the client builder and runtime.
docs/users-guide.md (2)
700-751: Lifecycle hooks documentation is thorough and well-integrated.The new subsection clearly explains setup, teardown, and error hooks with a practical code example. Footnotes are properly defined and acronyms (BDD) are correctly expanded on first use. The content aligns with the client builder implementation and reinforces the mirrored architecture with server-side hooks.
1003-1008: Footnotes are correctly formatted and comprehensive.Both footnotes [^47] and [^48] provide precise implementation locations and test coverage references, enabling readers to find implementation details and test examples.
docs/wireframe-client-design.md (1)
78-124: Connection lifecycle section thoroughly documents the new hook system.The section clearly distinguishes setup (type-changing), teardown (state-consuming), and error (independent) hooks. The code example demonstrates practical state management through the session lifecycle. The type-changing builder pattern explanation at lines 121-124 correctly conveys the type-safety guarantees.
Static analysis hints at a potential comma after "closure" on line 86, though the current phrasing is grammatically acceptable. If clarity would benefit, reword to: "The setup callback must be an
FnOnceclosure, returning aFuturethat producesC."src/client/tests/helpers.rs (2)
20-31: LGTM: Clean server spawn pattern.The function correctly spawns a listener on an ephemeral port and returns both the address and an accept handle. The underscore-prefixed binding on line 27 correctly signals that the peer address is intentionally unused.
129-150: LGTM: Deterministic failure stub.The
FailingSerializerprovides a deterministic way to test error-handling paths without relying on I/O failures. This addresses the testing coverage requirements mentioned in the PR objectives.src/client/tests/mod.rs (1)
7-9: LGTM: Clean module decomposition.The reorganization into
error_handling,helpers, andlifecyclemodules follows separation of concerns and addresses the code duplication concerns from review feedback.tests/worlds/mod.rs (1)
30-30: LGTM: Public module addition.The
client_lifecyclemodule is correctly declared as public to enable re-export intests/world.rsand integration with the cucumber test infrastructure.tests/world.rs (1)
8-8: LGTM: Lifecycle world integration.The re-export correctly wires
ClientLifecycleWorldandEXPECTED_SETUP_STATEinto the cucumber test infrastructure, following the established pattern for world aggregation.tests/steps/mod.rs (1)
6-6: LGTM: Step definitions module added.The
client_lifecycle_stepsmodule is correctly positioned to load lifecycle-related Given-When-Then steps for the cucumber test suite.src/client/preamble_builder.rs (1)
14-18: Generic parameter C added correctly.The extension from
impl<S, P>toimpl<S, P, C>properly propagates the connection state type through the builder. The where clause correctly constrains only S and P, leaving C unconstrained at this level.src/client/mod.rs (2)
1-6: Module documentation accurately reflects new lifecycle hooks capability.The updated description clearly explains that the client now supports lifecycle hooks mirroring the server's system, enabling consistent instrumentation. Well done.
17-30: Hook module integration follows established patterns.The internal
mod hookswith selectivepub usere-exports matches the existing module structure (builder, config, error, runtime). Consistent and clean.tests/steps/client_lifecycle_steps.rs (3)
1-16: Helper function successfully eliminates duplication.The
assert_count_equalshelper consolidates the count-assertion pattern that was previously repeated across multiple steps. Clear logic and informative error messages.
18-31: Given steps correctly delegate to world methods.The server setup steps appropriately delegate to
ClientLifecycleWorldmethods, maintaining clean separation between step definitions and test logic.
61-113: Assertion steps correctly implement feedback from previous reviews.The then-steps now:
- Use the
assert_count_equalshelper to eliminate duplication- Reference
EXPECTED_SETUP_STATEinstead of the magic number 42- Assert specific
ClientError::Disconnectedvariant as requestedWell-structured and maintainable.
tests/features/client_lifecycle.feature (1)
1-28: Feature scenarios provide comprehensive lifecycle hook coverage.The four scenarios effectively test:
- Setup invocation on connection
- Teardown invocation with state propagation
- Error hook on disconnect with variant assertion
- Preamble and lifecycle hook integration
Clear, focused, and well-structured. The preamble integration scenario correctly validates the sequence described in the PR objectives.
tests/cucumber.rs (2)
4-15: Documentation correctly reflects the expanded test suite.The module doc accurately lists all eleven test worlds, including the new
ClientLifecycleWorld. Consistent formatting and clear descriptions.
42-67: ClientLifecycleWorld integration follows established patterns.The import and runner invocation match the existing structure. The world is correctly wired to its feature file and executes in sequence with other test suites.
src/client/tests/lifecycle.rs (6)
1-15: LGTM!Module structure is correct with appropriate doc comment and well-organised imports.
16-28: LGTM!Test correctly uses
counting_hookandtest_with_clienthelpers, verifying setup runs exactly once.
30-54: LGTM!Correctly verifies teardown receives the state value produced by setup.
56-72: LGTM!Correctly verifies teardown is not invoked when no setup hook produced state.
118-139: LGTM!Correctly validates that
on_errorconfigured beforeon_connection_setupis preserved. Good use of thetest_error_hook_on_disconnecthelper.
74-116: Reduce duplication withtest_with_clienthelper.The spawn/connect/accept pattern duplicates what
test_with_clientprovides. Since the assertion onsetup_countoccurs after connection is established (whichtest_with_clienthandles), refactor to use the helper.Likely an incorrect or invalid review comment.
tests/client_lifecycle.rs (4)
1-21: LGTM!Module structure is correct. Lint expectations are appropriately scoped with clear reasons.
22-78: LGTM!Well-structured integration test verifying both setup and teardown callbacks run exactly once with correct state propagation.
80-122: LGTM!Correctly verifies error hook invocation on server disconnect. Serialisation and deserialisation failure tests are now covered in
src/client/tests/error_handling.rs.
124-218: LGTM!Comprehensive integration test validating lifecycle hooks work correctly with preamble exchange. All three callback types are verified.
src/client/tests/error_handling.rs (5)
1-16: LGTM!Module structure is correct with appropriate imports for comprehensive error path testing.
18-35: LGTM!Concise test using
test_error_hook_on_disconnecthelper effectively.
37-56: LGTM!Important negative test ensuring the client handles errors gracefully when no error hook is configured.
58-99: LGTM!Good coverage of the deserialisation error path. Sending invalid bincode data correctly triggers
ClientError::Deserialize.
143-180: LGTM!Deterministic serialisation failure test using
FailingSerializer. This addresses the PR comment about missing serialisation failure coverage.src/client/hooks.rs (3)
1-14: LGTM!
BoxFuturealias reduces verbosity in handler type signatures as recommended.
16-85: LGTM!Handler types are well-documented with examples. The
ClientErrorHandlerretains the borrowed signature for efficiency, which is a reasonable trade-off.
87-113: LGTM!
LifecycleHooks<C>is correctly structured with appropriately scoped lint expectation and sensibleDefaultimplementation.src/client/runtime.rs (5)
13-59: LGTM!
WireframeClientcorrectly extends with lifecycle hook fields and generic connection state parameterC.
61-93: LGTM!
Debugimplementation andbuilder()method correctly handle the three-parameter generic form.
126-193: LGTM!Error hook invocation is correctly async and fires before each error is returned. This addresses the past review concern about
block_on.
280-287: LGTM!Clean async implementation of error hook invocation.
353-361: LGTM!
close()correctly flushes pending frames and sends EOF before invoking teardown hook. This addresses the past review concern about graceful shutdown.src/client/builder.rs (4)
23-65: LGTM!Macro documentation correctly explains why struct update syntax cannot be used when type parameters change. Each arm correctly propagates the
lifecycle_hooksfield.
67-115: LGTM!Builder struct correctly extended with
lifecycle_hooksfield and three-parameter generic form.
385-402: LGTM!
on_connection_setupcorrectly preserveson_errorwhilst clearingon_disconnect(since its type signature depends on the old state type). This addresses the past review concern about silently dropping hooks.
532-548: LGTM!Connection setup hook is correctly invoked after preamble exchange, and all lifecycle hooks are wired into the returned
WireframeClient.tests/worlds/client_lifecycle.rs (4)
1-59: LGTM!Module structure is correct.
TestClienttype alias addresses past review feedback and improves readability.
61-82: LGTM!World structure is well-organised with proper cleanup in
Dropimplementation.
84-137: LGTM!Helpers
spawn_server,handle_connection_result, andconnect_with_buildereffectively reduce duplication across test scenarios.
139-317: LGTM!Server and client methods consistently use the
spawn_serverandconnect_with_builderhelpers. Documentation includes appropriateErrorsandPanicssections.
Refactored the lifecycle.rs tests to replace manual setup of test server and client in favor of the test_with_client helper function. This simplifies test code and improves readability by removing redundant code like spawn_listener and manual WireframeClient setup. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/client/tests/lifecycle.rs:
- Around line 50-66: Two tests provide identical coverage:
teardown_without_setup_does_not_run and
close_without_setup_does_not_invoke_teardown both only register a teardown hook,
call client.close().await, and assert the teardown count is 0; remove the
duplicate by deleting the close_without_setup_does_not_invoke_teardown test
function so only teardown_without_setup_does_not_run remains, ensuring no other
tests or references depend on the removed function name.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/client/tests/lifecycle.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
src/client/tests/lifecycle.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/lifecycle.rs
🧠 Learnings (1)
📚 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/lifecycle.rs
🧬 Code graph analysis (1)
src/client/tests/lifecycle.rs (1)
src/client/tests/helpers.rs (3)
counting_hook(77-96)test_error_hook_on_disconnect(102-127)test_with_client(52-66)
🔍 Remote MCP Deepwiki
Summary of additional repository facts relevant to reviewing PR #411
-
Project CI and quality gates: PRs must pass formatting, zero Clippy warnings, unit+behavioral tests, and coverage; CI enforces make check-fmt, make lint, and coverage steps. This raises the importance of no warnings or test regressions from API changes in this PR.
-
Testing infra and helpers:
- wireframe_testing crate provides in-memory duplex helpers (drive_with_bincode, drive_with_frame) and LoggerHandle for deterministic tests; useful for verifying lifecycle tests and avoiding network flakiness.
- Existing test patterns favor rstest fixtures, tokio time control, and reuse via helper functions — aligns with refactors in this PR (test_with_client, counting_hook).
-
Hook and lifecycle conventions:
- Server-side WireframeProtocol and existing on_connection_setup/on_connection_teardown semantics are established; client-side hooks should mirror these lifecycle semantics (setup → preamble handling → teardown) and preserve non-state-dependent hooks when changing connection-state type C.
-
Preamble and hookup ordering:
- Docs and code emphasize preamble read → on_preamble_success handler → continue to app processing. PR’s ordering (preamble → on_preamble_success → on_connection_setup) matches repository conventions and should be validated against existing preamble handlers.
-
Error handling and async patterns:
- Project enforces non-blocking async patterns and panic isolation (connection tasks use catch_unwind). invoke_error_hook must be non-blocking and safe in async contexts — PR changes invoking error hooks on send/receive align with repository resilience patterns.
-
Metrics / observability impact:
- Framework exposes metrics (connection panics, frames processed, errors). New lifecycle hooks and added close() behavior may require metric updates or ensure no regressions in metrics emission.
Sources:
- Repository docs and design pages (overview, core architecture, testing, roadmap, observability) used to extract the above facts.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test
Address intermittent failures in `error_callback_invoked_on_send_io_error` test by adding retries with exponential backoff when sending a message after server disconnect. This ensures the OS has time to propagate connection closure signals, making the test more reliable. Also remove a redundant lifecycle test and fix async step return to use TestResult. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
|
@coderabbitai Have the following now been resolved?
Status: What's covered: What's missing: Gap: A test that uses a custom serialiser that always fails during encoding is still missing. Status: What's done: What's not done: pub type ClientErrorHandler = The suggestion was to take an owned error and return a 'static future: pub type ClientErrorHandler = ❌ Not Resolved (1/9) Status: ❌ NOT RESOLVED Issue 6a: The macro still manually lists all fields instead of using ..self struct update syntax: ($self:expr,serializer = $value:expr) => { Issue 6b: on_connection_teardown and on_error still mutate in place rather than consistently using the macro: pub fn on_connection_teardown<F, Fut>(mut self, f: F) -> Self { |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/client/tests/error_handling.rs:
- Around line 2-5: Remove the module-level #![expect(clippy::excessive_nesting,
...)] and instead add #[expect(clippy::excessive_nesting, reason = "async
closures within builder patterns are inherently nested")] immediately above each
of the four test functions error_callback_invoked_on_receive_error,
error_callback_invoked_on_deserialize_error,
error_callback_invoked_on_send_io_error, and
error_callback_invoked_on_serialize_error; leave no_error_hook_does_not_panic
without the suppression and ensure there are no other file- or module-wide
expect attributes remaining.
In @src/client/tests/lifecycle.rs:
- Around line 2-5: Remove the module-level#![expect(clippy::excessive_nesting,
reason = "...")] and instead add the attribute
#[expect(clippy::excessive_nesting, reason = "async closures within builder
patterns are inherently nested")] immediately above each test function
definition: setup_callback_invoked_on_connect,
teardown_callback_receives_setup_state, teardown_without_setup_does_not_run,
setup_and_teardown_callbacks_run, and on_connection_setup_preserves_error_hook
so the suppression is narrowly scoped to those tests only.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/client/tests/error_handling.rssrc/client/tests/lifecycle.rssrc/client/tests/mod.rstests/steps/client_lifecycle_steps.rstests/worlds/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
tests/steps/client_lifecycle_steps.rssrc/client/tests/lifecycle.rssrc/client/tests/error_handling.rstests/worlds/mod.rssrc/client/tests/mod.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/steps/client_lifecycle_steps.rssrc/client/tests/lifecycle.rssrc/client/tests/error_handling.rstests/worlds/mod.rssrc/client/tests/mod.rs
🧠 Learnings (1)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
tests/steps/client_lifecycle_steps.rssrc/client/tests/lifecycle.rssrc/client/tests/error_handling.rstests/worlds/mod.rssrc/client/tests/mod.rs
🧬 Code graph analysis (2)
tests/steps/client_lifecycle_steps.rs (4)
tests/worlds/correlation.rs (1)
expected(45-45)src/extractor.rs (1)
state(82-90)tests/multi_packet.rs (1)
count(71-76)tests/worlds/client_lifecycle.rs (1)
last_error(345-345)
src/client/tests/mod.rs (1)
src/frame/format.rs (1)
bytes(54-54)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (7)
tests/worlds/mod.rs (1)
3-7: LGTM: Module doc and export correctly updated.The doc comment accurately reflects the new client lifecycle world, and the module export maintains alphabetical ordering whilst following the existing pattern.
Also applies to: 30-30
tests/steps/client_lifecycle_steps.rs (5)
1-5: LGTM!Module documentation and imports are appropriate. The import of
EXPECTED_SETUP_STATEaddresses the previous magic-constant concern.
7-16: LGTM!The helper function cleanly extracts the repeated count-assertion pattern with a descriptive error message.
18-31: LGTM!Given steps are concise, consistent, and properly delegate to world methods.
33-62: LGTM!When steps are consistent and all return
TestResultas expected. Thewhen_client_closessignature now aligns with other steps.
106-116: LGTM!The error variant assertion correctly uses pattern matching and provides a descriptive error message on mismatch.
src/client/tests/mod.rs (1)
1-171: Test organisation is excellent.The modularisation into separate submodules (error_handling, helpers, lifecycle) improves maintainability whilst keeping the socket option and codec configuration tests cleanly organised in this main module. The consistent use of the
socket_option_test!macro reduces duplication effectively.
- Introduce helper functions `connect_with_server` and `test_with_serializer` to simplify client-server test setup and reuse common connection logic. - Replace repetitive listener and client setup code in error handling tests with the new helpers. - Add `CountingHookClosure` type alias for async hooks and improve `counting_hook` function signature for reusability. - Keep server streams alive during tests to allow precise control of server behavior. - Overall improves clarity and maintainability of client error handling tests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: src/client/tests/error_handling.rs Comment on lines +36 to +53 async fn connect_with_server<F, C>(
configure_builder: F,
) -> (
WireframeClient<BincodeSerializer, RewindStream<TcpStream>, C>,
TcpStream,
)
where
F: FnOnce(WireframeClientBuilder) -> WireframeClientBuilder<BincodeSerializer, (), C>,
C: Send + 'static,
{
let (addr, accept) = spawn_listener().await;
let client = configure_builder(WireframeClient::builder())
.connect(addr)
.await
.expect("connect client");
let server_stream = accept.await.expect("join accept task");
(client, server_stream)
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
Refactor the test helpers in error_handling.rs to reuse a generalized connect_with_server function that returns both the client and server stream. This reduces code duplication and improves clarity in tests that manipulate server streams or test error callbacks by keeping the server connection open for the duration of the test. 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 @src/client/tests/error_handling.rs:
- Around line 2-5: Remove the module-level #![expect(...)] attribute and instead
add the narrow #[expect(clippy::excessive_nesting, reason = "async closures
within builder patterns are inherently nested")] attribute directly above each
of the four affected async tests: error_callback_invoked_on_receive_error,
error_callback_invoked_on_deserialize_error,
error_callback_invoked_on_send_io_error, and
error_callback_invoked_on_serialize_error (place it alongside/above their
existing #[tokio::test] annotations). Do not add the attribute to
no_error_hook_does_not_panic; leave that test unsuppressed.
In @src/client/tests/helpers.rs:
- Around line 57-72: Update the docstring for the helper function
test_with_client to explicitly state that the spawned server task (bound to
_server) is intentionally dropped immediately after connecting the client to
simulate a disconnected/short-lived server for lifecycle/disconnect testing;
mention that the unused _server binding is deliberate to prevent future
maintainers from reattaching or awaiting it.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/client/tests/error_handling.rssrc/client/tests/helpers.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
src/client/tests/error_handling.rssrc/client/tests/helpers.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/error_handling.rssrc/client/tests/helpers.rs
🧠 Learnings (1)
📚 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/error_handling.rssrc/client/tests/helpers.rs
🧬 Code graph analysis (2)
src/client/tests/error_handling.rs (1)
src/client/tests/helpers.rs (3)
spawn_listener(27-37)test_error_hook_on_disconnect(100-125)test_with_client(58-72)
src/client/tests/helpers.rs (4)
src/client/runtime.rs (1)
rewind_stream(319-319)tests/multi_packet.rs (1)
count(71-76)src/request/mod.rs (1)
value(381-386)tests/worlds/client_lifecycle.rs (1)
error_count(335-335)
🔍 Remote MCP
The web search results aren't specific to this Wireframe client. Let me search for the actual repository to understand the existing codebase better.Great! I found a reference. Let me search for the specific repository mentioned in the crates.io entry:Based on my search findings and the comprehensive PR context you've provided, I can now provide a concise review summary with relevant facts to support your pull request review:
Summary of Key Review Findings
Architecture & Design Patterns
Generic State Management Approach:
The PR introduces a solid type-safe pattern for per-connection state by adding a generic C parameter to both WireframeClientBuilder<S, P, C> and WireframeClient<S, T, C>. This follows established Rust async patterns where state is propagated through the type system rather than runtime checks, improving compile-time safety.
Critical Implementation Details
Error Hook Non-Blocking Behavior:
The PR explicitly addresses a critical async/await pitfall: according to the review comments, the original implementation used futures::executor::block_on inside async runtime methods (noted as issue that "blocks the event loop"). The resolution was to make invoke_error_hook fully async and awaited, aligning with Tokio's threading model where blocking operations can stall other task execution.
Hook Preservation on Builder State Changes:
A key design decision: when on_connection_setup changes the C type parameter, the PR preserves any previously-configured on_error hook while resetting on_disconnect (which is state-dependent). This prevents silent loss of error handlers due to builder method ordering.
Explicit close() Semantics:
Rather than relying on Drop trait (which cannot be async), the PR implements an explicit close() method on WireframeClient<S, RewindStream<TcpStream>, C> that:
- Flushes and closes the framed transport
- Invokes teardown with the stored connection state
- Provides deterministic cleanup ordering
Test Coverage Implementation
Refactored Test Helpers:
The PR consolidates repeated test scaffolding into modularized helpers:
spawn_listener()- ephemeral TCP listenertest_with_client()- pre-connected client with running servercounting_hook()- Arc-based counter for invocation trackingFailingSerializer- stub serializer for error path testing
Comprehensive Error Path Coverage:
Added unit tests covering all error invocation points:
- Receive errors (send/framing/deserialization)
- Serialization failures
- I/O errors on disconnect
- Deserialize errors from invalid framed data
Documentation & Lifecycle Flow
Preamble → Setup → Operation → Teardown:
The documented flow is: preamble exchange → on_preamble_success → on_connection_setup → normal operation. This ordering ensures setup logic can access preamble results if needed.
Unresolved Considerations from Review
Based on the PR objectives summary, two refinement areas were noted:
- ClientErrorHandler Signature: Discussion ongoing about using owned error + 'static future vs. borrowed HRTB signature (current implementation uses borrowed)
- Builder Macro Simplification: Suggested consolidation of
builder_field_update!using struct update syntax—remains unimplemented
⏰ 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 (12)
src/client/tests/helpers.rs (6)
20-24: Type alias addresses prior feedback.The
CountingHookClosure<T>alias eliminates the#[expect(clippy::type_complexity)]annotation and improves readability. Well done.
26-55: Clean helper abstractions for TCP listener and builder option assertions.These helpers reduce boilerplate across tests. The ephemeral port binding and spawned accept task pattern is appropriate for parallel test execution.
74-94: LGTM!The
counting_hookfunction now uses theCountingHookClosure<T>type alias and wraps the closure withArc::new(...). This resolves the prior feedback.
96-125: Well-structured disconnect test helper.The explicit drop sequence and subsequent receive attempt cleanly verifies error hook invocation. The generic bounds allow flexible builder configuration.
127-148: LGTM!
FailingSerializerprovides a clean stub for exercising serialisation and deserialisation error paths. The descriptive error messages aid debugging.
150-160: LGTM!The macro correctly uses the absolute crate path and the
pub(crate)re-export restricts visibility appropriately.src/client/tests/error_handling.rs (6)
31-50: LGTM!This helper intentionally differs from
test_with_clientby returning the server stream for test manipulation (e.g., sending invalid data, controlled disconnection). The duplication is justified by the different return semantics.
52-69: LGTM!Clean use of the
test_error_hook_on_disconnecthelper with a clear assertion.
71-82: LGTM!Important edge case: verifies the client handles errors gracefully when no error hook is configured. Note this test has no nested closures, so it must not receive the
excessive_nestingsuppression when scoping is applied.
84-121: LGTM!Excellent test coverage for deserialisation errors. The use of
FramedwithLengthDelimitedCodecensures the invalid payload reaches the client's deserialiser rather than being rejected at the framing layer.
123-167: LGTM!The retry loop with exponential backoff (lines 146-155) properly addresses the prior flakiness concern. The bounded retry count prevents indefinite hangs whilst accommodating platform timing variations.
169-199: LGTM!Good design: keeping the server alive (
_server) ensures the error originates from serialisation rather than a broken pipe, providing a clean isolation of the error path under test.
Add #[expect(clippy::excessive_nesting)] annotations to async closure tests to acknowledge inherent nesting in builder patterns. Enhance test helpers to simulate disconnected server behavior by dropping server stream after connect. Add doc comments clarifying test behaviors and verify lifecycle hook ordering in client lifecycle steps. 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 @tests/steps/client_lifecycle_steps.rs:
- Around line 18-62: Add triple-slash doc comments to each Cucumber step
function to document their purpose and any constraints: add succinct ///
comments above given_standard_server, given_disconnecting_server,
given_ack_server, when_connect_with_setup, when_connect_with_setup_and_teardown,
when_client_closes, when_connect_with_error_callback,
when_client_attempts_receive, and when_connect_with_preamble_and_lifecycle
describing what the step does (e.g., starts specific test server variant or
performs client action), expected preconditions, and any important
postconditions or error behavior; keep comments short and consistent with
existing examples in the repo.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/client/tests/error_handling.rssrc/client/tests/helpers.rssrc/client/tests/lifecycle.rstests/steps/client_lifecycle_steps.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
src/client/tests/lifecycle.rssrc/client/tests/helpers.rssrc/client/tests/error_handling.rstests/steps/client_lifecycle_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/tests/lifecycle.rssrc/client/tests/helpers.rssrc/client/tests/error_handling.rstests/steps/client_lifecycle_steps.rs
🧠 Learnings (1)
📚 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/lifecycle.rssrc/client/tests/helpers.rssrc/client/tests/error_handling.rstests/steps/client_lifecycle_steps.rs
🧬 Code graph analysis (3)
src/client/tests/lifecycle.rs (1)
src/client/tests/helpers.rs (3)
counting_hook(82-98)test_error_hook_on_disconnect(104-129)test_with_client(61-76)
src/client/tests/error_handling.rs (1)
src/client/tests/helpers.rs (3)
spawn_listener(27-37)test_error_hook_on_disconnect(104-129)test_with_client(61-76)
tests/steps/client_lifecycle_steps.rs (2)
tests/worlds/correlation.rs (1)
expected(45-45)tests/worlds/client_lifecycle.rs (1)
last_error(345-345)
⏰ 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 (9)
src/client/tests/lifecycle.rs (1)
1-141: Excellent test coverage and guideline adherence.This test module correctly implements lifecycle hook tests with:
- Narrowly scoped lint suppressions on only the three tests requiring them
- Consistent use of test helpers (
test_with_client,test_error_hook_on_disconnect,counting_hook) to eliminate duplication- Clear test names and assertions
- Proper use of
.expect()for error handlingAll previous review concerns have been successfully addressed.
src/client/tests/error_handling.rs (3)
27-46: Well-designed test helper.The
connect_with_serverhelper appropriately exposes the server stream for tests that need to manipulate server behaviour (sending invalid data, dropping connection). The doc comment clearly explains this use case.
152-163: Excellent solution for OS-dependent timing.The retry loop with exponential backoff properly handles platform and load variance in connection termination propagation, eliminating the flakiness of a fixed sleep duration. This is a robust approach for testing I/O error paths.
1-211: Comprehensive error handling test coverage.This test module correctly exercises all major error paths:
- Receive errors (disconnect)
- Deserialisation errors (invalid data)
- Send I/O errors (broken pipe)
- Serialisation errors (failing serializer)
- Absence of error hook (no panic)
All tests follow guidelines with narrowly scoped lint suppressions, proper use of
.expect(), and appropriate use of thematches!macro for error variant checking. Previous flakiness concerns have been resolved with the retry loop.src/client/tests/helpers.rs (5)
20-24: Type alias successfully reduces cognitive load.The
CountingHookClosure<T>type alias eliminates the need for a#[expect(clippy::type_complexity)]annotation and significantly improves readability. The doc comment clearly explains its purpose and relationship tocounting_hook.
57-76: Clear documentation prevents maintenance confusion.The enhanced doc comment for
test_with_clientexplicitly documents the intentional server drop, preventing future maintainers from "fixing" this behaviour. The explanation of simulating disconnection is clear and valuable.
78-98: Type alias integration successfully implemented.The
counting_hookfunction correctly returnsCountingHookClosure<T>and wraps the closure inArc::newat line 97 to match the type alias. This eliminates the type complexity annotation and improves maintainability.
131-152: Clean test utility for error path coverage.The
FailingSerializerprovides a straightforward way to test serialisation and deserialisation error paths. The error messages are descriptive and the implementation is appropriate for test scenarios.
1-164: Excellent helper module addressing all previous feedback.This helpers module successfully implements all requested improvements:
- Type alias
CountingHookClosure<T>eliminates complexity warnings- Enhanced doc comment for
test_with_clientclarifies intentional server dropcounting_hookcorrectly uses the type alias withArc::newwrapping- All helpers are well-documented and follow coding guidelines
FailingSerializerprovides clean error path coverage- Macro implementation is straightforward and appropriate
The module provides a solid foundation for lifecycle and error handling tests.
…le callbacks - Updated lifecycle callback test steps with additional documentation comments. - Simplified feature scenario wording to clarify setup callback invocation count. - Added asserts for precise callback invocation counts and their sequencing. - Removed redundant or implicit test steps related to setup and preamble ordering. - Improved error callback and disconnect error assertion steps for clarity and accuracy. These improvements increase test readability and robustness for client lifecycle management. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
1 participant
Summary
Changes
API surface
Client hooks module
Runtime and builder integration
Documentation and roadmap
Tests and integration harness
Why
How to use
.with_preamble(MyPreamble { ... })
.on_preamble_success(|preamble, stream| async move {
// optional: examine preamble and read server preamble-ack
Ok(leftover_stream)
})
.on_connection_setup(|| async { SessionState { token: ... } })
.on_connection_teardown(|state: SessionState| async move { ... })
.on_error(|err| async move { ... })
.connect(addr)
Migration guide
Testing plan
Documentation
Checklist
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragonlabs.com/task/055976c7-bdba-40b2-bb5a-b130a17ba7a5