Unify outbound codec path with FramePipeline & CodecDriver#461
Merged
Conversation
Contributor
Reviewer's GuideUnifies the server-side outbound path around a new FramePipeline and codec-aware helpers so that all handler responses, fragmentation, and metrics flow through a single pipeline, and adds end-to-end tests and docs to validate and describe the unified codec behavior. Sequence diagram for unified outbound response path via FramePipelinesequenceDiagram
actor Client
participant WireframeApp
participant HandlerService
participant ResponseContext
participant FramePipeline
participant Serializer
participant FrameCodec
participant Framed
participant Transport
Client->>WireframeApp: send_request
WireframeApp->>HandlerService: call_handler(env)
HandlerService-->>WireframeApp: Response
WireframeApp->>ResponseContext: forward_response(env, response)
ResponseContext->>ResponseContext: build_PacketParts
ResponseContext->>ResponseContext: Envelope::from_parts
ResponseContext->>FramePipeline: process(envelope)
FramePipeline->>FramePipeline: fragment_envelope
FramePipeline->>FramePipeline: push_frame(outbound_envelope)
ResponseContext->>FramePipeline: drain_output
FramePipeline-->>ResponseContext: Vec<Envelope>
ResponseContext->>Serializer: serialize(envelope)
Serializer-->>ResponseContext: bytes
ResponseContext->>FrameCodec: wrap_payload(bytes)
FrameCodec-->>ResponseContext: frame
ResponseContext->>Framed: send(frame)
Framed-->>Transport: write_bytes
Transport-->>Client: response_frames
Updated class diagram for FramePipeline and codec driver helpersclassDiagram
class FramePipeline {
-Option~FragmentationState~ fragmentation
-Vec~Envelope~ out
+FramePipeline new(fragmentation: Option~FragmentationConfig~)
+process(envelope: Envelope) void
+purge_expired() void
+drain_output() Vec~Envelope~
+fragmentation_mut() Option~&mut FragmentationState~
+has_fragmentation() bool
-fragment_envelope(envelope: Envelope) Result~Vec~Envelope~, FragmentationError~
-push_frame(envelope: Envelope) void
}
class Envelope {
+id: u32
+correlation_id: Option~u64~
+payload: Vec~u8~
+new(id: u32, correlation_id: Option~u64~, payload: Vec~u8~) Envelope
+from_parts(parts: PacketParts) Envelope
}
class FragmentationState {
+new(config: FragmentationConfig) FragmentationState
+fragment(envelope: Envelope) Result~Vec~Envelope~, FragmentationError~
+reassemble(envelope: Envelope) Result~Option~Envelope~, FragmentationError~
+purge_expired() void
}
class FragmentationConfig
class FragmentationError
class Serializer {
<<trait>>
+serialize(envelope: &Envelope) Result~Vec~u8~, SerializeError~
}
class FrameCodec {
<<trait>>
+wrap_payload(payload: Bytes) Frame
}
class ConnectionCodec {
}
class Framed {
+send(frame: Frame) Future~Result~(), io::Error~~
}
class Bytes
class ResponseContext {
+serializer: &Serializer
+framed: &mut Framed
+pipeline: &mut FramePipeline
+codec: &FrameCodec
}
class codec_driver_helpers {
+send_envelope(serializer: &Serializer, codec: &FrameCodec, framed: &mut Framed, envelope: &Envelope) Future~Result~(), io::Error~~
+flush_pipeline_output(serializer: &Serializer, codec: &FrameCodec, framed: &mut Framed, envelopes: &mut Vec~Envelope~) Future~Result~(), io::Error~~
}
FramePipeline --> FragmentationState : uses
FramePipeline --> Envelope : processes
FramePipeline --> FragmentationConfig : configured_by
FramePipeline --> FragmentationError : error_type
ResponseContext --> FramePipeline : holds_mut_ref
ResponseContext --> Serializer : uses
ResponseContext --> FrameCodec : uses
ResponseContext --> Framed : writes_to
codec_driver_helpers --> Serializer : uses
codec_driver_helpers --> FrameCodec : uses
codec_driver_helpers --> Framed : writes_to
codec_driver_helpers --> Envelope : sends
ConnectionCodec ..> FrameCodec : wraps
Framed ..> ConnectionCodec : parameterized_by
FrameCodec ..> Bytes : payload_type
Flow diagram for unified inbound and outbound fragmentation with FramePipelineflowchart LR
subgraph InboundPath
A[Inbound_frame_from_transport] --> B[ConnectionCodec_decode]
B --> C[Envelope]
C --> D[FramePipeline_fragmentation_mut]
D --> E[FragmentationState_reassemble]
E -->|Some_Envelope| F[Handler_dispatch]
E -->|None| G[Wait_for_more_fragments]
end
subgraph HandlerAndResponse
F --> H[HandlerService_execute]
H --> I[Response]
I --> J[build_PacketParts]
J --> K[Envelope::from_parts]
K --> L[FramePipeline_process]
L --> M[FramePipeline_drain_output]
end
subgraph OutboundPath
M --> N[Vec_Envelope_]
N --> O[flush_pipeline_output]
O --> P[send_envelope]
P --> Q[Serialize_with_Serializer]
Q --> R[FrameCodec_wrap_payload]
R --> S[Framed_send]
S --> T[Bytes_on_wire]
end
subgraph SharedFragmentationState
D --- L
L --- U[FragmentationState_shared_in_FramePipeline]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 Summary by CodeRabbit
WalkthroughAdd a FramePipeline and codec_driver module that centralises outbound fragmentation, buffering and metrics; replace prior FragmentationState usage across connection, reassembly and response flows; add envelope-based send helpers and extensive BDD and integration tests validating the unified outbound codec path. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Pipeline as FramePipeline
participant Fragmenter as Fragmenter
participant Serializer as Serializer
participant Wire as Wire
Client->>Pipeline: submit Envelope
alt fragmentation enabled
Pipeline->>Fragmenter: fragment_envelope(Envelope)
Fragmenter-->>Pipeline: fragments (Vec<Envelope>)
else fragmentation disabled
Pipeline-->>Pipeline: pass-through Envelope
end
Pipeline->>Pipeline: push_frame(envelope) / increment metrics
Pipeline->>Serializer: send_envelope(serializer, codec, framed, envelope)
alt send succeeds
Serializer->>Wire: write framed bytes
Wire-->>Client: deliver framed response
else send error
Serializer->>Pipeline: log error / increment handler errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@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/fixtures/unified_codec/mod.rs Comment on lines +107 to +127 pub fn verify_handler_payloads(&self) -> TestResult {
if self.handler_observed.len() != self.sent_payloads.len() {
return Err(format!(
"handler payload count mismatch: expected {}, got {}",
self.sent_payloads.len(),
self.handler_observed.len()
)
.into());
}
for (i, (observed, expected)) in self
.handler_observed
.iter()
.zip(self.sent_payloads.iter())
.enumerate()
{
if observed != expected {
return Err(format!("handler payload {i} mismatch").into());
}
}
Ok(())
}❌ New issue: Code Duplication |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 8 issues, and left some high level feedback:
- In
FramePipeline::processfragmentation errors are now logged and dropped rather than propagated (unlike the previousfragment_responsespath that returned anio::Error); if this change in failure semantics is intentional it might be worth double‑checking that callers are happy with fragmentation failures becoming non‑fatal and invisible to the response path. - The warning in
FramePipeline::process("failed to fragment outbound envelope") no longer includes the envelope id/correlation id that the oldfragment_responseslogging had; consider adding those fields back into the log context to make production debugging easier. - The new unified codec tests duplicate low‑level send/receive helpers (e.g.
send_one/recv_one) in bothtests/unified_codec.rsandtests/fixtures/unified_codec/transport.rs; factoring these into a single shared helper would reduce repetition and keep the transport behaviour consistent across tests.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `FramePipeline::process` fragmentation errors are now logged and dropped rather than propagated (unlike the previous `fragment_responses` path that returned an `io::Error`); if this change in failure semantics is intentional it might be worth double‑checking that callers are happy with fragmentation failures becoming non‑fatal and invisible to the response path.
- The warning in `FramePipeline::process` (`"failed to fragment outbound envelope"`) no longer includes the envelope id/correlation id that the old `fragment_responses` logging had; consider adding those fields back into the log context to make production debugging easier.
- The new unified codec tests duplicate low‑level send/receive helpers (e.g. `send_one`/`recv_one`) in both `tests/unified_codec.rs` and `tests/fixtures/unified_codec/transport.rs`; factoring these into a single shared helper would reduce repetition and keep the transport behaviour consistent across tests.
## Individual Comments
### Comment 1
<location> `src/app/codec_driver.rs:55-61` </location>
<code_context>
+ ///
+ /// Processed envelopes are buffered internally. Call
+ /// [`drain_output`](Self::drain_output) to retrieve them.
+ pub(crate) fn process(&mut self, envelope: Envelope) {
+ let frames = match self.fragment_envelope(envelope) {
+ Ok(frames) => frames,
+ Err(err) => {
+ warn!("failed to fragment outbound envelope: error={err:?}");
+ crate::metrics::inc_handler_errors();
+ return;
+ }
+ };
</code_context>
<issue_to_address>
**issue (bug_risk):** Outbound fragmentation errors are now swallowed, changing previous error-propagation semantics.
Previously, `fragment_responses` returned an `io::Result<Vec<Envelope>>` and `forward_response` propagated fragmentation failures as `io::Error`, so transport-level issues were surfaced to callers. In the new flow, `process` logs, bumps metrics, and returns `()` on `FragmentationError`, so `forward_response` still appears to succeed and the handler behaves as if the response was sent. That changes the intended separation between handler failures (`Ok(())`) and transport failures (`Err(io::Error)`). If this change isn’t deliberate, consider having `process` return a `Result` (or similar) so `forward_response` can propagate transport/protocol/serialization errors instead of silently dropping them.
</issue_to_address>
### Comment 2
<location> `docs/execplans/9-3-1-fragment-adapter-trait.md:150-153` </location>
<code_context>
+- [x] (2026-02-15 01:00Z) Stage B: created `src/app/codec_driver.rs` with
+ `FramePipeline` struct. Handles outbound fragmentation and metrics. All
+ existing tests pass.
+- [x] (2026-02-15 01:30Z) Stage C: updated `frame_handling/core.rs`,
+ `frame_handling/response.rs`, `frame_handling/reassembly.rs`, and
+ `frame_handling/tests.rs` to use `FramePipeline`. Responses now route through
+ the pipeline (fragment → metrics → serialize → send). All gates pass (fmt,
+ lint, test).
</code_context>
<issue_to_address>
**nitpick (typo):** Use en-GB spelling for “serialise” to match the stated spelling constraint.
In “Responses now route through the pipeline (fragment → metrics → serialize → send)”, please change “serialize” to “serialise” to match the en-GB-oxendict constraint and existing uses like “serialisation” elsewhere in the docs.
```suggestion
`frame_handling/response.rs`, `frame_handling/reassembly.rs`, and
`frame_handling/tests.rs` to use `FramePipeline`. Responses now route through
the pipeline (fragment → metrics → serialise → send). All gates pass (fmt,
lint, test).
```
</issue_to_address>
### Comment 3
<location> `docs/execplans/9-3-1-fragment-adapter-trait.md:134` </location>
<code_context>
+ codec-aware variant (or wrapper) for the server path. The standalone actor
+ remains useful for client scenarios and direct testing.
+
+- Risk: the app router currently handles deserialization failures with a
+ counter and threshold (`MAX_DESER_FAILURES`). Moving decode into a separate
+ loop task may complicate shared mutable state. Severity: low. Likelihood:
</code_context>
<issue_to_address>
**nitpick (typo):** Use en-GB spelling for “deserialisation” for consistency.
Since we’re standardising on en-GB-oxendict, please change “deserialization failures” to “deserialisation failures” to match nearby terms like “serialisation” and “behavioural”.
```suggestion
- Risk: the app router currently handles deserialisation failures with a
```
</issue_to_address>
### Comment 4
<location> `docs/asynchronous-outbound-messaging-design.md:840` </location>
<code_context>
+- **Unified Codec Pipeline:** On the server side, the `FramePipeline`
+ (`src/app/codec_driver.rs`) applies outbound fragmentation and metrics to
+ every `Envelope` before it reaches the wire. Handler responses pass through
+ the pipeline before serialisation and codec wrapping, ensuring the same
+ fragmentation logic applies to both handler responses and push traffic.
+ Protocol hooks are applied at the connection-actor level and are currently
</code_context>
<issue_to_address>
**suggestion (review_instructions):** "serialisation" should use an -ize form under en-GB-oxendict conventions ("serialization").
The style guide specifies en-GB-oxendict spelling, which uses -ize forms. Please change "serialisation" here to "serialization".
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 5
<location> `docs/users-guide.md:877` </location>
<code_context>
+On the server side, a unified `FramePipeline` applies the same fragmentation
+logic to all outbound `Envelope` values — handler responses, streaming frames,
+and multi-packet channels — before serialisation and codec wrapping. This
+guarantees that a single connection-scoped `FragmentationState` manages both
+outbound fragmentation and inbound reassembly.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** "serialisation" should be written with -ize spelling ("serialization") to match en-GB-oxendict.
To align with the en-GB-oxendict style (-ize / -yse / -our), please update "serialisation" to "serialization" here.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 6
<location> `docs/execplans/9-3-1-fragment-adapter-trait.md:162` </location>
<code_context>
+ `tests/unified_codec.rs`. Five tests: round-trip, fragmented response,
+ unfragmented small payload, multiple sequential requests, disabled
+ fragmentation. Committed in `1b2851a`.
+- [x] (2026-02-15 03:00Z) Stage F: BDD behavioural tests added. Five
+ rstest-bdd scenarios in `tests/features/unified_codec.feature` with fixture
+ `tests/fixtures/unified_codec/` and steps
</code_context>
<issue_to_address>
**suggestion (review_instructions):** "BDD" is introduced without being expanded on first use.
The instructions ask for uncommon acronyms to be defined on first use. Please expand this to something like "Behaviour-Driven Development (BDD) behavioural tests" here, or add an explicit definition immediately before this bullet.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 7
<location> `docs/execplans/9-3-1-fragment-adapter-trait.md:586` </location>
<code_context>
+The exact integration point (pre-wrapped frames vs. `Envelope` values) will be
+decided in Stage A and recorded in the decision log.
+
+### Fragmentation reconciliation
+
+Today path 1 fragments at the `Envelope` level (before serialisation) and path
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Within this section, "serialiser" should be "serializer" to follow the -ize/-izer convention.
In the sentence "The design documents specify outbound order as \"serialiser → fragmentation → codec framing\"", "serialiser" should be updated to "serializer" to follow en-GB-oxendict (-ize/-izer) spelling.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 8
<location> `docs/execplans/9-3-1-fragment-adapter-trait.md:421` </location>
<code_context>
+1. Construct the codec-aware driver from Stage B.
+2. Split the connection loop into an inbound decode task and an outbound
+ actor/writer task communicating via an internal bounded channel.
+3. When a handler returns `Response::Single(bytes)` or `Response::Vec(v)`,
+ send the serialised, wrapped frames through the actor's input channel so
+ hooks and fragmentation apply.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Later in this list item, "serialised" should be "serialized" to match en-GB-oxendict spelling.
This bullet refers to "the serialised, wrapped frames". To comply with the en-GB-oxendict (-ize) guidance, please change "serialised" to "serialized".
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…etween app router and Connection actor Introduce a comprehensive 626-line ExecPlan document outlining the design, constraints, risks, progress, and testing strategies for unifying codec handling between the app router and Connection actor in the Wireframe server runtime. This living document elaborates on the existing dual-path architecture, the need for uniform protocol hook invocation, fragmentation, and metrics handling by routing all outbound payloads through a codec-aware ConnectionActor wrapper. The plan defines stages from initial design, scaffolding a codec driver, refactoring response handling, integration and BDD tests, to documentation updates, ensuring source compatibility and no public API breaks. This foundational strategy aims to harmonize streaming, push, and multi-packet responses and enhance robustness and maintainability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Introduce `FramePipeline` in `src/app/codec_driver.rs` to centralise outbound fragmentation and metrics for handler responses. All outbound envelopes now pass through a single pipeline that applies optional fragmentation before serialisation and codec wrapping. Key changes: - Add `codec_driver` module with `FramePipeline`, `send_envelope`, and `flush_pipeline_output` helpers - Update `frame_handling/core.rs` `ResponseContext` to hold a `FramePipeline` reference instead of raw `FragmentationState` - Simplify `frame_handling/response.rs` to route through the pipeline instead of ad-hoc fragment → serialize → send - Update `frame_handling/reassembly.rs` to access fragmentation state via `FramePipeline::fragmentation_mut()` - Remove duplicate `fragment_responses()`, `serialize_response()`, and `send_response_payload()` helpers - Update exec plan with progress and design decisions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add `tests/unified_codec.rs` with five integration tests exercising the `FramePipeline`-based outbound path end-to-end: - handler_response_round_trips_through_pipeline - fragmented_response_passes_through_pipeline - small_payload_passes_unfragmented - multiple_sequential_requests_through_pipeline - pipeline_with_no_fragmentation_passes_large_payload Tests use duplex streams with `handle_connection_result` to validate that handler responses flow correctly through the unified pipeline, including both fragmented and unfragmented payloads. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Five rstest-bdd scenarios validate the FramePipeline end-to-end: handler round-trip, fragmented response, unfragmented small payload, multiple sequential requests, and disabled-fragmentation passthrough. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Record the FramePipeline unification in the outbound messaging design, multi-packet/streaming design, and users guide. Mark roadmap item 9.3.1 and its sub-items as done. Update exec plan progress to stage H. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
All stages complete. Quality gates pass. Protocol hooks deferred to follow-up stage due to F::Frame vs Envelope type constraint. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ion logic Extracted common verification logic from verify_handler_payloads and verify_response_payloads into a new verify_payloads_match helper method to reduce code duplication and improve maintainability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…458) * docs(execplans): add execplan for integrating MessageAssembler inbound Introduces a comprehensive execution plan document detailing the integration of the MessageAssembler trait into the inbound connection path. The plan covers constraints, tolerances, risks, progress stages, tests, and documentation updates corresponding to roadmap items 8.2.5 and 8.2.6. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * feat(app/frame_handling): integrate inbound message assembly in connection path - Added protocol-level message assembly on the inbound path after transport fragmentation reassembly and before handler dispatch. - Implemented parsing and continuity validation for message assembly frames with unified failure handling using deserialization failure policy. - Introduced new modules for assembly logic (`assembly.rs`) and envelope decoding (`decode.rs`) to streamline connection code. - Updated `WireframeApp` connection handling to create and maintain message assembly state alongside fragmentation state. - Added detailed unit tests and behavioral tests for interleaving, ordering violations, timeout purging, and integration scenarios. - Upgraded `rstest-bdd` dev-dependencies to v0.5.0 to support new behavioral tests. - Updated documentation, exec plans, and roadmap to reflect completed message assembly integration (items 8.2.5 and 8.2.6). This enhancement completes the roadmap for streaming requests and shared message assembly, improving protocol multiplexing and robustness on the inbound connection path. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): refactor continuation frame sending into single helper method Refactored the sending of continuation frames in message_assembly_inbound.rs by combining repeated code into a new helper method `send_continuation_frame_impl`. This reduces duplication and clarifies the distinction between final and non-final continuation frames while preserving existing functionality. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): use newtype wrappers for message key and frame sequence Introduce MessageKey and FrameSequence newtype structs to enhance type safety in the message assembly inbound test fixture. Update related methods to accept these types via Into conversions, improving code clarity and preventing misuse of raw integers. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(tests): use ContinuationFrameParams struct in message assembly tests Refactor send_continuation_frame_impl method to accept a ContinuationFrameParams struct instead of multiple parameters, improving code clarity and maintainability in the message assembly inbound test fixture. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(frame_handling): extract helper to simplify assembly testing code Introduced `process_assembly_frame` helper function to encapsulate calls to `assemble_if_needed` with assembly state and parameters. Replaced repetitive assembly invocation patterns in integration tests with this helper to improve code readability and maintainability without changing test logic. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * refactor(frame_handling): unify message assembly error handling and payload helpers - Introduced AssemblyContext::fail_invalid_none for streamlined error recording - Replaced envelope_from_assembled function with Envelope::from_assembled method - Moved test helpers for building first and continuation frame payloads to a common test_helpers module - Updated tests and fixtures to use shared payload builders instead of local copies - Adjusted failure counter reset in connection.rs to occur only after full inbound pipeline success - Minor docs wording improvements This refactor improves consistency in error handling, code reuse for test data generation, and overall maintainability of the message assembly logic. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * test(frame_handling): replace thread sleep with synthetic clock in assembly timeout test Replaced the use of thread::sleep with advancing a synthetic clock (Instant) in the inbound assembly timeout test. This change makes the test deterministic and independent of real wall-clock scheduling, improving reliability of timeout purging behavior verification. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * test(fixtures): deterministically control tokio time in message assembly tests Refactor MessageAssemblyInboundWorld tests to use a current-thread Tokio runtime with all features enabled. Pause the Tokio clock at test start and replace sleep calls with time advancements. This improves test determinism and makes time-dependent actions independent of wall-clock scheduling. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * feat(message_assembler): preserve envelope routing metadata from first frame - Introduce `EnvelopeRouting` struct to carry envelope_id and correlation_id. - Extend `FirstFrameInput` and `AssembledMessage` to include `EnvelopeRouting`. - Modify message assembly state to track and propagate routing metadata through assembly. - Update frame handling and assembly tests to preserve routing info on completed messages. - Ensure completed messages use envelope routing metadata from the initial first frame regardless of continuation frames. - Improves message dispatching and correlation by retaining original envelope identifiers throughout multi-frame message assembly. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * feat(message-assembler): introduce EnvelopeId and CorrelationId newtypes for routing Refactor routing metadata types by replacing raw u32 and u64 identifiers with strongly typed wrappers: EnvelopeId and CorrelationId. This improves type safety and clarity in message assembly and frame handling components. Update related code, tests, and documentation to use these newtypes accordingly. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> * docs(tests): enhance fixture documentation for inbound message assembly Improve the doc comment for the MessageAssemblyInboundWorld fixture used in BDD test scenarios. Clarifies its purpose for rstest injection and its role in exercising inbound message assembly integration within scenario tests. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com> --------- Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Deduplicate the message assembly local and context field bindings in src/app/connection.rs after the rebase conflict merge. This keeps the merged FramePipeline + message assembly behaviour while restoring a compilable FrameHandlingContext construction/destructuring path.
leynos
force-pushed
the
unify-codec-handling-9-3-1-mn7az7
branch
from
a3fae3e to
8735a08
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…per modules - Introduced a shared Tokio runtime for synchronous BDD test steps to avoid repetitive runtime creation and blocking. - Extracted common send/receive helpers into a shared module for reuse across integration and behavioral tests. - Refactored test fixtures and integration test harness to use shared helpers and runtime. - Improved error handling and test code clarity by eliminating redundant async runtime creations. This refactor improves test code maintainability and performance by centralizing common logic and resource usage. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Refactored the 'then_handler_receives_payload' and 'then_handler_receives_reassembled' test steps in `unified_codec_steps.rs` to use a new helper function `collect_and_verify_handler_payloads`. This reduces code duplication and improves test code clarity. Additionally, improved documentation clarity in `docs/execplans/9-3-1-fragment-adapter-trait.md` with expanded acronyms and consistent spelling, and added a doc comment in `unified_codec.rs` for the `setup_harness` function. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Owner
Author
|
@coderabbitai Have the following now been resolved? |
This comment was marked as resolved.
This comment was marked as resolved.
… return Results
Refactored `first_frame_payload` and `continuation_frame_payload` functions to return `Result<Vec<u8>, io::Error>` instead of unconditionally returning the payload vector. Added error handling for body length exceeding `u32::MAX`. Updated call sites in tests and related code to handle the Result with `.expect("valid test payload")` or propagating the error as appropriate. This improves robustness by explicitly handling potential payload construction failures.
Also includes minor documentation and style tweaks unrelated to the core fix.
Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/9-3-1-fragment-adapter-trait.md`:
- Around line 161-164: The documentation uses the British -ise spelling
"initialisation" in the text; update it to the en-GB-oxendict -ize form
"initialization" in this file (docs/execplans/9-3-1-fragment-adapter-trait.md)
and scan nearby prose for other -ise/-yse/-our variants (e.g., initialise,
emphasise, colour) and replace them with their -ize/-yze/-our forms per the
styleguide to ensure consistent en-GB-oxendict spelling.
In `@tests/steps/unified_codec_steps.rs`:
- Around line 13-19: The helper runtime() currently panics on Runtime::new()
failure; change it to return a TestResult (propagating errors) instead of
panicking and use a fallible OnceLock/OnceCell initializer (e.g.,
get_or_try_init) so initialization errors are returned; update all call sites of
runtime() to handle the Result via the ? operator rather than
unwrapping/panicking. Ensure you reference the runtime() function, the static
OnceLock/OnceCell, and Runtime::new() when making these changes so callers
return/propagate TestResult errors.
…ation Changed runtime() to return a Result to handle potential initialization errors. Updated callers to propagate these errors properly instead of panicking. This improves error handling and robustness in the unified_codec_steps test code. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Rationale
Plan of work (status)
How to review
Impact
Documentation updates
Validation & acceptance criteria
Artifacts
Task reference
Revision note (2026-02-15)
Summary by Sourcery
Unify the server-side outbound codec path by introducing a FramePipeline and codec driver utilities, routing all outbound envelopes through a single fragmentation- and metrics-aware pipeline and adding end-to-end tests and docs to cover the unified behaviour.
Enhancements:
Documentation:
Tests: