Plan and Implement Wireframe Client Library

[leynos]

Summary

• Introduce a formal client preamble exchange with configurable callbacks and timeout.
• Extend the client runtime to support preamble-aware operation via generics and a rewindable stream that preserves leftover bytes for framing.
• Add encoding/decoding helpers for preambles and related error handling.
• Expand test coverage with integration tests and cucumber-based scenarios for preamble behavior.
• Update documentation across roadmap, user guide, and design docs to reflect preamble support.

Changes

API & Runtime

• Builder: WireframeClientBuilder gains preamble configuration:
  • with_preamble(preamble)
  • preamble_timeout(Duration)
  • on_preamble_success(callback)
  • on_preamble_failure(callback)
• Client runtime updated to generic form: WireframeClient<S, T> where T: ClientStream. Default behavior remains ergonomic via builder changes, but preamble support is fully wired into the runtime path.
• RewindStream: Introduced/extended so the framing layer can wrap leftover preamble bytes and expose inner streams when needed.
• Public preamble handlers:
  • ClientPreambleSuccessHandler
  • ClientPreambleFailureHandler
• Preamble write/read utilities: Added write_preamble in src/preamble.rs to complement existing read_preamble, enabling end-to-end preamble negotiation.
• Errors: Added dedicated ClientError variants for preamble flow:
  • PreambleEncode
  • PreambleWrite
  • PreambleRead
  • PreambleTimeout

Preamble Encoding/Decoding

• Implemented asynchronous preamble write path and integrated timeout handling for the preamble phase.
• Success callback can read server response (e.g., a preamble ack) and must return any leftover bytes for replay to the framing layer.
• Failure callback receives the error and the TCP stream for diagnostics or custom error handling.

Tests

• New integration tests under tests/client_preamble.rs exercising:
  • Client sends preamble and server acknowledges
  • Success callback receives preamble and leftover bytes
  • Timeout triggers failure callback and timeout error
  • Connection without preamble works as expected
  • Preamble write with no response scenario
• Test world and cucumber-based flows added under tests/worlds/client_preamble.rs, tests/features/client_preamble.feature, and related steps in tests/steps/client_preamble_steps.rs.
• Updated existing client runtime tests to accommodate the new generic client type and leftover-byte behavior.

Documentation

• docs/roadmap.md: Mark preamble integration as completed.
• docs/users-guide.md: Added section detailing client preamble exchange with code example.
• docs/wireframe-client-design.md: Documented preamble support and callback usage.

Tests & CI

• Added cucumber-based end-to-end tests for client preamble flows alongside existing test suites.
• Ensured tests cover success and failure callbacks, server acknowledgement, and normal operation without preamble.

Migration & Usage

• The client runtime now uses generics to thread a possible preamble and leftover stream bytes through the lifecycle of a connection. Existing code that relied on WireframeClient::builder() for default behavior will continue to work with the default typing, but you can opt into preamble support via the new with_preamble, preamble_timeout, and callbacks.
• Example snippet (conceptual):
  • let builder = WireframeClient::builder().with_preamble(MyPreamble { version: 1 }).preamble_timeout(Duration::from_secs(5)).on_preamble_success(|p, s| { async move { // read server response; return Ok(leftover) } .boxed() }).connect(addr).await?;

Why this matters

• Enables early negotiation and validation of protocol state before framing begins, reducing miscommunication and enabling richer error handling.
• Keeps payload framing decoupled from preamble logic by returning leftover bytes to replay before framing begins.
• Improves testability with dedicated preamble pathways and cucumber-driven scenarios.

Testing plan

• Unit tests around preamble encoding/decoding and error variants
• Integration tests for preamble success/failure paths
• Cucumber-based end-to-end scenarios for client preamble flows
• Regression tests ensuring normal operation without preamble remains functional

📎 Task: https://www.terragonlabs.com/task/3170fe00-36c0-4563-828c-f9b3a758978d

[sourcery-ai]

Sorry @leynos, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Walkthrough

This pull request adds optional preamble exchange to the WireframeClient, enabling clients to negotiate a typed preamble before frame exchange. It extends the builder with generic preamble support, async success/failure callbacks, timeout handling, stream wrapping via RewindStream, new error variants, and comprehensive integration tests.

Changes

Cohort / File(s)	Summary
Documentation updates docs/roadmap.md, docs/users-guide.md, docs/wireframe-client-design.md	Marked preamble integration task complete; added new "Client preamble exchange" section documenting builder API, callbacks, and control flow; updated design documentation with preamble support section and formatting fix.
Builder and client generics src/client/builder.rs, src/client/runtime.rs	Extended WireframeClientBuilder with generic parameter P for preamble type; added with_preamble, preamble_timeout, on_preamble_success, on_preamble_failure builder methods. Generalised WireframeClient over stream type T with ClientStream trait bound; added RewindStream specialisation with tcp_stream/rewind_stream accessors; updated connect flow to perform preamble exchange and wrap stream.
Error handling src/client/error.rs	Added four error variants: PreambleEncode, PreambleWrite, PreambleRead, PreambleTimeout for explicit preamble-related error reporting.
Handler and utility types src/client/mod.rs, src/preamble.rs, src/rewind_stream.rs	Introduced ClientPreambleSuccessHandler and ClientPreambleFailureHandler type aliases for async callbacks. Added write_preamble function for async preamble encoding/writing. Added RewindStream accessors: inner, inner_mut, into_inner.
Test infrastructure setup tests/steps/mod.rs, tests/world.rs, tests/worlds/mod.rs, tests/worlds/client_runtime.rs	Registered new test modules; added ClientPreambleWorld re-export; updated ClientRuntimeWorld client field to use full generic type WireframeClient<BincodeSerializer, RewindStream>.
BDD test suite tests/features/client_preamble.feature, tests/steps/client_preamble_steps.rs, tests/worlds/client_preamble.rs, tests/cucumber.rs	Created new feature file with four scenarios (preamble+ack, callback, timeout, no-preamble); implemented step definitions (given/when/then) and world state (TestPreamble, ServerAck, ClientPreambleWorld) with server simulacra and assertion methods; registered suite in cucumber runner.
Integration test module tests/client_preamble.rs	Added five integration test scenarios: basic preamble exchange, success callback reception, timeout handling, no-preamble connection, write-only scenario. Includes TestPreamble, ServerAck helpers and channel utilities.

Sequence Diagram

sequenceDiagram
    participant Client as Client Builder
    participant Conn as TCP Connection
    participant Handler as Success/Failure<br/>Handler
    participant RewindStream as RewindStream<br/>Wrapper
    participant Framing as Framing Layer

    Client->>Conn: TCP connect
    Note over Conn: Socket established

    alt Preamble configured
        Client->>Conn: write_preamble
        Note over Conn: Encode preamble<br/>transmit bytes
        
        opt Timeout triggered
            Client->>Handler: invoke_failure_handler
            Handler->>Conn: log/handle error
            Note over Client: Return ClientError
        end
        
        opt Success path
            Client->>Handler: on_preamble_success
            Handler->>Conn: bidirectional exchange<br/>(read response, etc.)
            Handler-->>Client: return leftover bytes
            Client->>RewindStream: wrap stream with leftovers
        end
        
        RewindStream->>Framing: stream ready
    else No preamble
        Conn->>Framing: stream ready
    end

    Note over Framing: Framing begins

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

• Implement WireframeClient with Configurable Runtime #407: Introduced the client runtime architecture that this PR extends with preamble support, adding new builder generics, handler types, error variants, and RewindStream integration to the same builder/runtime/error modules.

Poem

🎯 A preamble exchange begins,
Where clients send their opening moves,
Callbacks dance and leftover bytes rewind,
Timeouts catch the sluggish streams,
And frames at last commence!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title 'Plan and Implement Wireframe Client Library' is vague and does not accurately describe the specific changes in this pull request, which focuses on preamble support integration.	Revise the title to be specific and descriptive, e.g. 'Add client preamble exchange support with configurable callbacks and timeout' or 'Implement client preamble negotiation in WireframeClient runtime'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check	✅ Passed	The pull request description comprehensively describes the changes, objectives, and migration path for the preamble feature and is directly related to the changeset.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch terragon/implement-wireframe-client-runtime-icnjqy

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Actionable comments posted: 4

Caution

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

⚠️ Outside diff range comments (1)

src/client/builder.rs (1)

1-584: File exceeds 400-line limit.

At 584 lines, this file exceeds the 400-line guideline. Extract the preamble exchange logic (lines 514-584) into a dedicated module (e.g., src/client/preamble_exchange.rs or integrate into src/preamble.rs) to improve maintainability and adhere to file length constraints. As per coding guidelines, large modules must be decomposed.

📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3c285fa and db3cee8.

📒 Files selected for processing (19)

• docs/roadmap.md
• docs/users-guide.md
• docs/wireframe-client-design.md
• src/client/builder.rs
• src/client/error.rs
• src/client/mod.rs
• src/client/runtime.rs
• src/client/tests.rs
• src/preamble.rs
• src/rewind_stream.rs
• tests/client_preamble.rs
• tests/cucumber.rs
• tests/features/client_preamble.feature
• tests/steps/client_preamble_steps.rs
• tests/steps/mod.rs
• tests/world.rs
• tests/worlds/client_preamble.rs
• tests/worlds/client_runtime.rs
• tests/worlds/mod.rs

🧰 Additional context used

📓 Path-based instructions (4)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Run make check-fmt (cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Run make 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.
Run make 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.
Use make 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 using Arc to 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 unnecessary mut bindings in Rust code.
Avoid unsafe code 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 use return in single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Use concat!() to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...

Files:

• tests/worlds/mod.rs
• tests/worlds/client_runtime.rs
• tests/world.rs
• tests/cucumber.rs
• src/client/error.rs
• src/client/tests.rs
• src/rewind_stream.rs
• tests/steps/mod.rs
• src/client/mod.rs
• tests/client_preamble.rs
• src/client/runtime.rs
• src/client/builder.rs
• src/preamble.rs
• tests/steps/client_preamble_steps.rs
• tests/worlds/client_preamble.rs

---

⚙️ CodeRabbit configuration file

**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.

• Adhere to single responsibility and CQRS
• Place function attributes after doc comments.
• Do not use return in single-line functions.
• Move conditionals with >2 branches into a predicate function.
• Avoid unsafe unless absolutely necessary.
• Every module must begin with a //! doc comment that explains the module's purpose and utility.
• Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
• Lints must not be silenced except as a last resort.
  • #[allow] is forbidden.
  • Only narrowly scoped #[expect(lint, reason = "...")] is allowed.
  • No lint groups, no blanket or file-wide suppression.
  • Include FIXME: with link if a fix is expected.
• Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
• Use rstest fixtures for shared setup and to avoid repetition between tests.
• Replace duplicated tests with #[rstest(...)] parameterised cases.
• Prefer mockall for mocks/stubs.
• Prefer .expect() over .unwrap() in tests.
• .expect() and .unwrap() are forbidden outside of tests. Errors must be propagated.
• Ensure that any API or behavioural changes are reflected in the documentation in docs/
• Ensure that any completed roadmap steps are recorded in the appropriate roadmap in docs/
• Files must not exceed 400 lines in length
  • Large modules must be decomposed
  • Long match statements or dispatch tables should be decomposed by domain and collocated with targets
  • Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
• Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
  • For testing of functionality depending upon environment variables, dependency injection and...

Files:

• tests/worlds/mod.rs
• tests/worlds/client_runtime.rs
• tests/world.rs
• tests/cucumber.rs
• src/client/error.rs
• src/client/tests.rs
• src/rewind_stream.rs
• tests/steps/mod.rs
• src/client/mod.rs
• tests/client_preamble.rs
• src/client/runtime.rs
• src/client/builder.rs
• src/preamble.rs
• tests/steps/client_preamble_steps.rs
• tests/worlds/client_preamble.rs

docs/**/*.md

📄 CodeRabbit inference engine (docs/contents.md)

Follow the documentation style guide conventions when writing project documentation

docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: -ize suffix in words like 'realize' and 'organization', -lyse suffix in words like 'analyse', -our suffix in words like 'colour', -re suffix in words like 'centre', double 'l' in words like 'cancelled', maintain 'e' in words like 'likeable', -ogue suffix in words like 'catalogue'
The word 'outwith' is acceptable in British English documentation
Keep US spelling when used in an API (for example, 'color')
Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns: 'Lille Industries are expanding'
Write headings in sentence case in documentation
Use Markdown headings (#, ##, ###, and so on) in order without skipping levels
Follow markdownlint recommendations for Markdown files
Always provide a language identifier for fenced code blocks in documentation; use 'plaintext' for non-code text
Use '-' as the first level bullet and renumber lists when items change in documentation
Prefer inline links using 'text' or angle brackets around the URL in documentation
Ensure blank lines before and after bulleted lists and fenced blocks in documentation
Ensure tables have a delimiter line below the header row in documentation
Expand any uncommon acronym on first use in documentation (for example, Continuous Integration (CI))
Wrap paragraphs at 80 columns in documentation
Wrap code at 120 columns in documentation
Do not wrap tables in documentation
Use footnotes referenced with '[^label]' in documentation
Include Mermaid diagrams where they add clarity to documentation
When embedding figures in documentation, use '' and provide brief alt text describing the content
Add a short description before each Mermaid diagram so screen readers can understand it

Write official documentation with se...

Files:

• docs/roadmap.md
• docs/wireframe-client-design.md
• docs/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/roadmap.md
• docs/wireframe-client-design.md
• docs/users-guide.md

**/*.md

📄 CodeRabbit inference engine (AGENTS.md)

**/*.md: Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make 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.md
• docs/wireframe-client-design.md
• docs/users-guide.md

---

⚙️ CodeRabbit configuration file

**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")

• Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
• Headings must not be wrapped.
• Documents must start with a level 1 heading
• Headings must correctly increase or decrease by no more than one level at a time
• Use GitHub-flavoured Markdown style for footnotes and endnotes.
• Numbered footnotes must be numbered by order of appearance in the document.

Files:

• docs/roadmap.md
• docs/wireframe-client-design.md
• docs/users-guide.md

🧬 Code graph analysis (7)

tests/worlds/client_runtime.rs (1)

src/client/runtime.rs (1)

• rewind_stream (269-269)

src/client/tests.rs (1)

src/client/runtime.rs (1)

• rewind_stream (269-269)

src/rewind_stream.rs (2)

src/extractor.rs (1)

• into_inner (281-281)

src/middleware.rs (3)

• into_inner (34-34)
• into_inner (75-75)
• into_inner (116-116)

tests/client_preamble.rs (1)

src/preamble.rs (2)

• read_preamble (112-138)
• write_preamble (175-196)

src/client/runtime.rs (5)

src/client/builder.rs (2)

• serializer (92-102)
• new (63-70)

src/rewind_stream.rs (1)

• new (25-31)

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)

src/client/builder.rs (2)

src/preamble.rs (1)

• write_preamble (175-196)

src/rewind_stream.rs (1)

• new (25-31)

tests/worlds/client_preamble.rs (2)

src/preamble.rs (2)

• read_preamble (112-138)
• write_preamble (175-196)

src/client/builder.rs (1)

• new (63-70)

🪛 LanguageTool

docs/roadmap.md

[uncategorized] ~393-~393: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ... Integrate the existing preamble helpers so clients can emit and verify preambl...

(COMMA_COMPOUND_SENTENCE_2)

🔍 Remote MCP

I'll help you gather relevant context for reviewing this PR effectively. Let me search for information about the Wireframe client library architecture, preamble design patterns, and related implementation details.Based on the search results, I now have useful context about RewindStream and async callback patterns. Let me gather more specific information about the PR by checking if there's a GitHub repository reference and looking for documentation on the implementation details.Now let me compile the relevant context I've found:

Summary of Relevant Context for PR #410 Review

RewindStream Pattern (Leftover Bytes Management)

RewindStream wraps AsyncRead/AsyncWrite implementations, allowing bytes to be prepended before they are read again—useful when bytes have been read from a stream but need to "put them back" for reprocessing. This is directly applicable to the preamble use case: the PR's RewindStream integration captures leftover bytes from the preamble exchange and replays them before the framing layer begins decoding actual frames.

Async Callback Handler Pattern

The handler type signatures used in the PR (ClientPreambleSuccessHandler<T> and ClientPreambleFailureHandler) follow established Rust async patterns:

• BoxFuture is commonly used with Arc to wrap async closures, enabling them to be stored and executed later
• Arc is used to enable multiple ownership of the same data, allowing safe sharing across async contexts
• The pattern of Arc<Box<dyn Fn...>> + Send + Sync + 'static bounds are standard for passing async callbacks across task boundaries without blocking

Leftover Bytes in Framed Protocols

The tokio-util Decoder trait has a built-in check that raises an error ("bytes remaining on stream") when bytes are left in the buffer after decode returns None, which means the framing layer must properly consume all buffered data. This directly relates to the PR's careful handling of leftover bytes returned by the preamble success callback—those bytes must be prepended to the stream before framing begins, or the LengthDelimitedCodec will reject them.

Related Design: PR #407 Integration

The PR extends work from PR #407 (client runtime introduction). The new generics on WireframeClient<S, T> where T: ClientStream allow the client to work with any async stream type, not just TcpStream. This enables RewindStream<TcpStream> to be used as the concrete type, seamlessly wrapping the TCP stream while preserving the leftover preamble bytes.

Key Review Focus Areas

1. Timeout handling: The preamble exchange uses configurable timeouts; verify timeout errors properly propagate and don't leave connections in partial states
2. Error propagation: Four new error variants (PreambleEncode, PreambleWrite, PreambleRead, PreambleTimeout) must be properly mapped from underlying I/O and bincode errors
3. Callback semantics: The success callback receives mutable stream access and must return leftover bytes; the failure callback should not attempt recovery to avoid double-processing
4. Type ergonomics: Default generic parameters (WireframeClient<S = BincodeSerializer, T = TcpStream>) should preserve backward compatibility for non-preamble users

🔇 Additional comments (57)

docs/roadmap.md (1)

393-395: Roadmap update correctly reflects completed work.

The checkbox change accurately marks the preamble integration task as complete, consistent with the PR scope.

Optional: add a comma before 'so' to separate the independent clauses ("...preamble helpers, so clients can emit..."). Modern style permits omission for closely connected clauses, but the comma aids readability.

src/rewind_stream.rs (1)

33-42: LGTM: Accessor methods follow established patterns.

The three accessor methods (inner, inner_mut, into_inner) adhere to the single-line function style guideline and match patterns used elsewhere in the codebase. The doc comment on into_inner correctly warns that unread leftover bytes will be discarded, which is essential for correct usage in the preamble flow.

docs/wireframe-client-design.md (1)

81-122: LGTM: Preamble documentation is clear and well-formatted.

The new preamble support section correctly documents the builder API, callback semantics, and leftover byte handling. The code example demonstrates proper usage with boxed() futures and explains the success callback's responsibility to return leftover bytes for the framing layer. Formatting adheres to the documentation style guide (80-column paragraphs, 120-column code, proper language identifier).

src/client/mod.rs (1)

6-61: LGTM: Handler type aliases are well-designed.

The new ClientPreambleSuccessHandler<T> and ClientPreambleFailureHandler type aliases correctly use higher-rank trait bounds (for<'a>) to enable lifetime flexibility across invocations. The Arc wrapper enables Clone for the builder pattern, and Send + Sync + 'static bounds are appropriate for crossing async task boundaries. The doc comments clearly explain the handlers' responsibilities, and the ignore block is appropriate since the example types are not defined in the module.

tests/worlds/mod.rs (1)

29-29: LGTM: Module wiring for client preamble test world.

The module declaration correctly exposes the new client preamble test world, consistent with the existing world module structure.

tests/world.rs (1)

8-8: LGTM: World re-export for cucumber tests.

The re-export correctly exposes ClientPreambleWorld for the cucumber test runner, consistent with the existing world type pattern.

tests/steps/mod.rs (1)

6-6: LGTM: Step module wiring for preamble tests.

The module declaration correctly wires the client preamble step definitions into the cucumber test suite, consistent with the existing step module structure.

tests/features/client_preamble.feature (1)

1-24: LGTM: Comprehensive preamble test scenarios.

The feature file defines four distinct scenarios that comprehensively exercise the preamble exchange workflow: success path with version validation, server acknowledgement reading in the success callback, timeout handling with failure callback, and the no-preamble baseline. The Gherkin syntax is correct and the scenarios are clearly written.

src/client/tests.rs (1)

12-15: LGTM!

The import additions and the updated generic bound correctly reflect the new WireframeClient<BincodeSerializer, RewindStream<TcpStream>> type returned by the builder after preamble integration. The test helper now properly constrains against the concrete client type.

Also applies to: 41-41

docs/users-guide.md (2)

427-490: LGTM!

The new "Client preamble exchange" section provides clear documentation of the preamble API, including a comprehensive code example demonstrating success and failure callbacks. The explanation of leftover byte handling and the callback semantics aligns with the implementation.

---

738-739: LGTM!

The footnote correctly references the implementation locations for client preamble support.

tests/worlds/client_runtime.rs (1)

11-15: LGTM!

The import updates and field type change correctly reflect the new generic WireframeClient parameterisation. The test world now properly uses the concrete type returned by the builder.

Also applies to: 24-24

src/client/error.rs (1)

20-31: LGTM!

The four new error variants comprehensively cover the preamble exchange lifecycle: encoding, writing, reading/decoding, and timeout scenarios. The #[source] attributes correctly preserve the error chain for diagnostics.

One observation: PreambleRead wraps a DecodeError but the message says "failed to read preamble response". The actual I/O read failures during preamble reading would surface through the DecodeError::Io variant, so this is correct — but renaming to PreambleDecode might be marginally clearer. Not a blocker.

tests/cucumber.rs (1)

4-11: LGTM!

The cucumber test runner is correctly updated to include the new ClientPreambleWorld suite. The documentation comments accurately reflect the seven test suites and their feature file mappings.

Also applies to: 23-23, 34-34, 51-51

src/preamble.rs (2)

1-14: LGTM!

The module documentation and imports are correctly updated to reflect the new encoding capability alongside the existing decoding support.

---

118-121: LGTM!

Starting with a single byte read is a sensible improvement that avoids unnecessary blocking on small preambles where the initial chunk size might exceed the encoded preamble length.

tests/client_preamble.rs (6)

1-21: LGTM!

The module structure and imports are well-organised. The #[cfg(not(loom))] guard correctly excludes these tests from loom concurrency testing.

---

23-58: LGTM!

The test support types are well-designed:

• TestPreamble with magic validation provides a realistic preamble structure.
• ServerAck models the server response.
• The Holder<T> pattern with channel_holder and take_sender cleanly handles oneshot channel sharing across async boundaries.

---

60-99: LGTM!

This test exercises the complete preamble exchange flow: client sends preamble, server validates and responds with ServerAck, client reads and validates the acknowledgement. Good end-to-end coverage.

---

101-141: LGTM!

This test verifies that the success callback receives the original preamble value, using the oneshot channel pattern to extract and validate the preamble version outside the callback.

---

143-197: LGTM!

This test correctly exercises the timeout path by having the server hold the connection without responding. The assertions verify both the returned error type and that the failure callback receives ClientError::PreambleTimeout. Proper cleanup via server.abort() ensures the test does not hang.

---

199-259: LGTM!

These tests cover the edge cases well:

• client_without_preamble_connects_normally verifies backwards compatibility.
• client_preamble_write_only_no_response confirms the client can send a preamble without expecting a server response.

src/client/runtime.rs (6)

7-10: LGTM!

The import additions correctly support the new generic stream abstraction and RewindStream integration.

Also applies to: 16-16

---

20-22: LGTM!

The ClientStream trait alias cleanly encapsulates the AsyncRead + AsyncWrite + Unpin bound, improving readability across the module. The blanket implementation ensures any type meeting these bounds automatically implements ClientStream.

---

40-58: LGTM!

The generic parameterisation of WireframeClient<S, T> with default types preserves backwards compatibility whilst enabling the new RewindStream integration. The Debug implementation correctly constrains on T: ClientStream.

---

60-80: LGTM!

The builder() method remains on the concrete WireframeClient<BincodeSerializer, TcpStream> type, preserving the ergonomic entry point for users who do not need custom serializers or stream types.

---

83-86: LGTM!

The generic impl block correctly bounds on S: Serializer + Send + Sync and T: ClientStream. The new stream() accessor provides generic access to the underlying stream, complementing the existing codec_config() method.

Also applies to: 219-238

---

240-269: LGTM!

The specialised impl block for WireframeClient<S, RewindStream<TcpStream>> provides convenient accessors for both the underlying TcpStream and the RewindStream wrapper. This supports test inspection and advanced use cases requiring direct socket access.

tests/worlds/client_preamble.rs (13)

1-11: LGTM!

Module-level documentation and lint expectations are properly scoped with clear reasons, following the coding guidelines.

---

25-47: LGTM!

TestPreamble is well-structured with proper documentation, derives, and single-line accessor style.

---

49-59: LGTM!

ServerAck struct is appropriately minimal for its test purpose.

---

61-72: LGTM!

Signal channel helpers are appropriately simple for test orchestration. Silent failure handling is acceptable in this context.

---

74-86: LGTM!

ClientPreambleWorld struct is well-organised with appropriate derives for cucumber integration.

---

88-114: LGTM!

Server setup is clean with proper documentation of error and panic conditions.

---

116-140: LGTM!

Acknowledgement server implementation follows established patterns and is properly documented.

---

142-161: LGTM!

Slow server implementation is appropriate for timeout testing scenarios.

---

163-181: LGTM!

Standard server provides baseline for non-preamble testing.

---

183-228: LGTM!

Connection flow with preamble is well-structured. The use of let chains (Rust 2024 feature) is appropriate given the edition in use.

---

230-268: LGTM!

Acknowledgement reading flow is clean with appropriate error mapping.

---

270-317: LGTM!

Timeout testing flow properly exercises both success and failure callback paths.

---

319-336: LGTM!

Non-preamble connection provides baseline coverage.

tests/steps/client_preamble_steps.rs (5)

1-5: LGTM!

Module documentation and imports are clean and appropriate.

---

7-25: LGTM!

Given steps cleanly delegate to world methods with appropriate async handling.

---

27-45: LGTM!

When steps properly use cucumber expressions for parameterised scenarios.

---

47-77: LGTM!

Assertion steps provide clear error messages and properly clean up server resources.

---

86-102: LGTM!

Failure and success assertion steps follow established patterns with proper cleanup.

src/client/builder.rs (11)

1-32: LGTM!

Imports and PreambleConfig struct are well-organised with appropriate privacy and structure.

---

34-49: LGTM!

Builder struct definition with generic defaults maintains backward compatibility.

---

51-75: LGTM!

Constructor and Default implementation are clean with proper documentation.

---

77-102: LGTM!

Serializer method properly preserves preamble configuration across type transformations.

---

104-290: LGTM!

Configuration methods follow established builder patterns with proper documentation and platform-specific handling.

---

292-330: LGTM!

with_preamble method cleanly handles type transformation while preserving builder state.

---

368-450: LGTM!

Handler registration methods are well-documented with clear examples. The same silent no-op concern noted for preamble_timeout applies here.

---

452-512: LGTM!

connect method properly integrates preamble exchange with appropriate type changes and documentation.

---

514-537: LGTM!

perform_preamble_exchange cleanly orchestrates the exchange with proper failure handling.

---

573-584: LGTM!

Failure handler invocation is clean with appropriate error logging.

---

359-366: Review the permissiveness of the preamble builder API.

The silent no-op when preamble_timeout, on_preamble_success, or on_preamble_failure are called before with_preamble is intentional design—all documentation examples and tests consistently call with_preamble first. However, this permissive builder pattern allows accidental misuse without compile-time detection or runtime warnings.

Consider either: (1) documenting this requirement explicitly in module-level docs and the impl block; or (2) restructuring the API so these methods are only available after with_preamble is called (using type-state or builder patterns that enforce ordering through the type system). The current approach prioritises API flexibility over safety.

[leynos]

@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/builder.rs

Comment on file

    /// ```
    #[must_use]
    pub fn serializer<Ser>(self, serializer: Ser) -> WireframeClientBuilder<Ser>
    pub fn serializer<Ser>(self, serializer: Ser) -> WireframeClientBuilder<Ser, P>

❌ New issue: Code Duplication
The module contains 2 functions with similar structure: WireframeClientBuilder.serializer,WireframeClientBuilder.with_preamble

[codescene-delta-analysis]

Gates Passed
6 Quality Gates Passed

See analysis details in CodeScene

Absence of Expected Change Pattern

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

Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.