Decouple Message Encoding from Bincode: Adapters, Context & SerdeBridge#475
Merged
Decouple Message Encoding from Bincode: Adapters, Context & SerdeBridge#475
Conversation
Contributor
Reviewer's GuideIntroduce serializer-agnostic message adapters and an explicit deserialization context, refactor all serializer/client/server boundaries to use them while keeping bincode compatibility via shims, and add an optional Serde bridge plus tests and docs to validate and document the new design. Sequence diagram for inbound frame parsing and context-aware deserializationsequenceDiagram
actor Client
participant Network
participant ServerConnection as Server_connection
participant Serializer as Serializer_S
participant Envelope
Client->>Network: send frame bytes
Network->>ServerConnection: incoming frame bytes
ServerConnection->>Serializer: parse(payload)
alt metadata parse succeeds
Serializer-->>ServerConnection: (parsed_envelope, metadata_bytes_consumed)
ServerConnection->>ServerConnection: build DeserializeContext
ServerConnection->>Serializer: deserialize_with_context~Envelope~(payload, context)
alt context-aware decode succeeds
Serializer-->>ServerConnection: (decoded_envelope, consumed)
ServerConnection->>ServerConnection: use decoded_envelope
else context-aware decode fails
ServerConnection->>ServerConnection: fall back to parsed_envelope
end
else metadata parse fails
ServerConnection->>Serializer: deserialize_with_context~Envelope~(payload, empty_context)
Serializer-->>ServerConnection: (decoded_envelope, consumed)
end
ServerConnection-->>Client: application handler processes Envelope
Sequence diagram for Serde bridge encode/decode via adapterssequenceDiagram
actor App as Application_code
participant Ser as BincodeSerializer
participant SM as SerdeMessage_T
participant Serde as SerdeSerializerBridge_impl
App->>App: payload: T implements Serialize + DeserializeOwned
App->>SM: into_serde_message(payload)
SM-->>App: serde_message
App->>Ser: serialize(serde_message)
activate Ser
Ser->>SM: encode_with(self)
activate SM
SM->>Serde: serialize_serde(self.as_ref())
Serde-->>SM: Vec<u8>
SM-->>Ser: Vec<u8>
deactivate SM
Ser-->>App: Vec<u8> bytes
deactivate Ser
App->>Ser: deserialize~SerdeMessage~T~~(bytes)
activate Ser
Ser->>SM: decode_with(self, bytes, empty_context)
activate SM
SM->>Serde: deserialize_serde(bytes, context)
Serde-->>SM: (T, consumed)
SM-->>Ser: (SerdeMessage~T~, consumed)
deactivate SM
Ser-->>App: (SerdeMessage~T~, consumed)
deactivate Ser
App->>SM: into_inner()
SM-->>App: T value
Class diagram for serializer adapters, context, and Serde bridgeclassDiagram
direction LR
class DeserializeContext~'a~ {
+Option~&'a u8[]~ frame_metadata
+Option~u32~ message_id
+Option~u64~ correlation_id
+Option~usize~ metadata_bytes_consumed
+empty() DeserializeContext
}
class EncodeWith~S~ {
<<interface>>
+encode_with(serializer S) Result~Vec~u8~~
}
class DecodeWith~S~ {
<<interface>>
+decode_with(serializer S, bytes u8[], context DeserializeContext) Result~(Self, usize)~
}
class Message {
<<interface>>
+to_bytes() Result~Vec~u8~~
+from_bytes(bytes u8[]) Result~Self~
}
class Serializer {
<<interface>>
+serialize~M~(value M) Result~Vec~u8~~
+deserialize~M~(bytes u8[]) Result~(M, usize)~
+deserialize_with_context~M~(bytes u8[], context DeserializeContext) Result~(M, usize)~
}
class BincodeSerializer {
+serialize~M~(value M) Result~Vec~u8~~
+deserialize~M~(bytes u8[]) Result~(M, usize)~
+deserialize_with_context~M~(bytes u8[], context DeserializeContext) Result~(M, usize)~
}
class SerdeSerializerBridge {
<<interface>>
+serialize_serde~T~(value T) Result~Vec~u8~~
+deserialize_serde~T~(bytes u8[], context DeserializeContext) Result~(T, usize)~
}
class SerdeMessage~T~ {
-T value
+new(value T) SerdeMessage~T~
+as_ref() &T
+into_inner() T
}
class IntoSerdeMessage {
<<interface>>
+into_serde_message() SerdeMessage~Self~
}
class Packet {
<<interface>>
+id() u32
+correlation_id() Option~u64~
+set_correlation_id(cid Option~u64~)
+into_parts() PacketParts
+from_parts(parts PacketParts) Self
}
class Envelope {
+u32 id
+Option~u64~ correlation_id
+payload
}
%% Relationships
Serializer <|.. BincodeSerializer
%% Bincode compatibility: Message types get adapter impls
Message <|.. EncodeWith
Message <|.. DecodeWith
%% BincodeSerializer uses adapters, not Message directly
BincodeSerializer ..> EncodeWith : serialize
BincodeSerializer ..> DecodeWith : deserialize*
%% Context flows into DecodeWith and Serializer
DeserializeContext ..> DecodeWith : passed_to
Serializer ..> DeserializeContext : uses
%% Serde bridge
SerdeSerializerBridge <|.. BincodeSerializer
EncodeWith <|.. SerdeMessage
DecodeWith <|.. SerdeMessage
IntoSerdeMessage <|.. SerdeMessage
IntoSerdeMessage <|.. SerdeMessage~T~
%% Packet no longer extends Message
Packet ..> Envelope : wraps
Packet ..> CorrelatableFrame
class CorrelatableFrame {
<<interface>>
+correlation_id() Option~u64~
+set_correlation_id(cid Option~u64~)
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary by CodeRabbit
WalkthroughIntroduce serializer-agnostic EncodeWith/DecodeWith traits and DeserializeContext, add an optional Serde bridge feature, update ADRs and docs, change many public API bounds from Message to EncodeWith/DecodeWith, add test fixtures and harness helpers; no runtime behavioural changes beyond trait-bound requirements. Changes
Sequence Diagram(s)sequenceDiagram
participant Frame as "Frame (LengthDelimited)"
participant Parser as "Frame Parser"
participant App as "App / Envelope"
participant Serializer as "Serializer (Bincode / Bridge)"
participant Decoder as "DecodeWith impl"
Frame->>Parser: receive frame bytes
Parser->>App: parse envelope and expose frame metadata
alt serializer.should_deserialize_after_parse() == true
App->>Serializer: deserialize_with_context(bytes, DeserializeContext{frame metadata,...})
Serializer->>Decoder: Decoder::decode_with(serializer, bytes, context)
Decoder-->>Serializer: (value, consumed)
Serializer-->>App: deserialised value
else
App->>Serializer: deserialize(bytes)
Serializer->>Decoder: Decoder::decode_with(serializer, bytes, DeserializeContext::empty())
Decoder-->>Serializer: (value, consumed)
Serializer-->>App: deserialised value
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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}
![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 4 issues, and left some high level feedback:
- The
DeserializeContextexample at the end hard-codes fields likemessage_id: Option<u32>andcorrelation_id: Option<u64>; consider aligning these types and names directly withFrameMetadata(or referencing that type) to avoid drift between the plan and the actual metadata model. - There is a lot of overlap between this ExecPlan and
docs/adr-005-serializer-abstraction.md/docs/users-guide.md; you might want to clearly mark which document is the single source of truth for the adapter traits and bridge behaviour to reduce the risk of inconsistent guidance later. - Since this file is intended as a living plan, it may help to add a short note near
Status: DRAFTexplicitly describing how and when theProgress,Decision Log, andOutcomes & Retrospectivesections should be updated (e.g., per stage or per PR) so that future edits follow a consistent process.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `DeserializeContext` example at the end hard-codes fields like `message_id: Option<u32>` and `correlation_id: Option<u64>`; consider aligning these types and names directly with `FrameMetadata` (or referencing that type) to avoid drift between the plan and the actual metadata model.
- There is a lot of overlap between this ExecPlan and `docs/adr-005-serializer-abstraction.md`/`docs/users-guide.md`; you might want to clearly mark which document is the single source of truth for the adapter traits and bridge behaviour to reduce the risk of inconsistent guidance later.
- Since this file is intended as a living plan, it may help to add a short note near `Status: DRAFT` explicitly describing how and when the `Progress`, `Decision Log`, and `Outcomes & Retrospective` sections should be updated (e.g., per stage or per PR) so that future edits follow a consistent process.
## Individual Comments
### Comment 1
<location> `docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md:3` </location>
<code_context>
+# 9.5.1 Decouple message encoding from bincode-specific traits
+
+This ExecPlan is a living document. The sections `Constraints`, `Tolerances`,
+`Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, and
+`Outcomes & Retrospective` must be kept up to date as work proceeds.
</code_context>
<issue_to_address>
**issue (review_instructions):** ExecPlan appears as an acronym without being expanded on first use.
To comply with the guideline on defining uncommon acronyms, expand ExecPlan the first time it appears, for example “This Execution Plan (ExecPlan) is a living document.”
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 2
<location> `docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md:26` </location>
<code_context>
+ serializer-agnostic adapter contract instead of direct `bincode` trait bounds.
+- Existing bincode message types continue to work with minimal or zero source
+ changes through compatibility shims.
+- A feature-gated Serde bridge is available as an optional path to reduce
+ manual adapter implementations.
+- Deserializers can inspect frame metadata through an explicit context object,
</code_context>
<issue_to_address>
**issue (review_instructions):** Serde is introduced as an acronym/name without being defined on first use.
The instructions require uncommon acronyms to be defined on first use. Consider expanding this to something like “a feature-gated Serde (serialization/deserialization) bridge” the first time it is mentioned.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 3
<location> `docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md:91` </location>
<code_context>
+
+- Risk: behavioural tests may assert implementation details rather than user
+ observable outcomes. Severity: medium. Likelihood: medium. Mitigation: keep
+ BDD scenarios focused on round-trip compatibility, migration-visible
+ behaviour, and version negotiation outcomes.
+
</code_context>
<issue_to_address>
**issue (review_instructions):** BDD is used without being expanded on first use.
To align with the requirement to define uncommon acronyms on first use, please expand BDD here, for example “behaviour-driven development (BDD) scenarios”.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 4
<location> `docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md:141` </location>
<code_context>
+
+## Decision Log
+
+- Decision: implement ADR-005 Option B (serializer-agnostic boundary plus
+ adapters) using compatibility-first layering. Rationale: satisfies roadmap
+ goals while minimizing disruption for current users. Date/Author: 2026-02-21
</code_context>
<issue_to_address>
**issue (review_instructions):** ADR is referenced in “ADR-005” without explaining the acronym on first use in prose.
Although the file name contains `adr-005`, the prose reference to “ADR-005 Option B” should still expand the acronym on first use, for example “Architecture Decision Record (ADR-005) Option B…”.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-5-1-decouple-message-encoding-from-bincode.md`:
- Around line 423-449: The long trait/type signatures in the Markdown code block
exceed 120 columns; reformat the signatures for EncodeWith::encode_with,
DecodeWith::decode_with, and Serializer::{serialize, deserialize_with_context}
by breaking parameter and return types across lines to wrap at ≤120 columns
(e.g., place generic bounds and long return types on their own indented lines),
preserving the same types (Box<dyn std::error::Error + Send + Sync>,
DeserializeContext<'_>, S: Serializer, M: EncodeWith<Self>, M: DecodeWith<Self>,
Sized) and method names so the semantics are unchanged while keeping each line
under the 120‑column limit.
- Around line 39-40: The sentence mentioning WireframeApp and WireframeClient
needs a comma inserted before "so" to fix the compound sentence: update the line
that reads "Keep `WireframeApp` and `WireframeClient` defaults on
bincode-compatible behaviour so existing applications do not need immediate
serializer rewrites." to include a comma after "behaviour" so it reads
"...behaviour, so existing applications...".
- Around line 13-33: Reflow the prose for roadmap item `9.5.1` so every
paragraph and each bullet point is wrapped to a maximum of 80 columns; update
the long sentences describing the goals (preserve bincode flows, add
serializer-agnostic adapter surface, optional Serde bridge, frame metadata for
deserialization, testing and docs requirements) into shorter lines and rewrap
the bullet list entries to 80 columns per line while preserving the original
wording and list order.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-5-1-decouple-message-encoding-from-bincode.md`:
- Around line 417-450: Add the missing Rust language identifier to the fenced
code block (change the opening ``` to ```rust) and reflow the long function
signature for the EncodeWith trait's method encode_with so it doesn't exceed 120
columns; specifically, locate the pub trait EncodeWith<S: Serializer> and break
fn encode_with(&self, serializer: &S) -> Result<Vec<u8>, Box<dyn
std::error::Error + Send + Sync>>; across multiple lines (e.g., each parameter
and the return type on its own line) to keep lines <=120 chars while preserving
types and bounds.
- Line 16: Update the prose in
docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md to use British
English (en-GB-oxendict) spelling: change occurrences of "serializer" in
descriptive text to "serialiser" (but do not change code identifiers such as the
trait name `Serializer`), change "serialization/deserialization" to
"serialisation/deserialisation", and convert -ize/-yze suffixes to -ise/-yse
(e.g., "realize"→"realise", "analyze"→"analyse") across the noted lines
(including 16, 23, 27, 41, 152, 203, 224, 278, 328, 378); ensure only prose is
modified and leave filenames, feature flags, and code identifiers unchanged.
- Line 348: Replace the three fenced code blocks that contain the shell commands
with language-tagged fences: locate the blocks containing the exact command
lines "set -o pipefail && cargo check --all-targets --all-features 2>&1 | tee
/tmp/9-5-1-check-baseline.log", "set -o pipefail && cargo test message
--all-features 2>&1 | tee /tmp/9-5-1-message-tests.log", and "set -o pipefail &&
make fmt 2>&1 | tee /tmp/9-5-1-fmt.log" and change their opening fence from ```
to ```shell so each block is fenced as shell code (leave the closing fence
as-is).
---
Duplicate comments:
In `@docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md`:
- Around line 23-34: Reflow the bullet list in
docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md (the five bullets
describing adapter contract, compatibility shims, Serde bridge, deserializer
context, and testing/docs/roadmap) so every line in each bullet wraps at or
below 80 columns; edit the paragraph text for each bullet (not semantics) to
insert line breaks and preserve words and punctuation, ensuring no line exceeds
80 characters and the bullets remain in the same order and content.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20618
Status SummaryDeserializeContext alignment ✓ ResolvedThe document now includes explicit alignment guidance before the code example (lines ~440-445):
Plus an inline comment within the struct definition referencing Document overlap/source of truth ✓ ResolvedLines 32-38 now define clear authority boundaries:
Status update process ✓ ResolvedLines 9-11 now specify the maintenance cadence:
All original review concerns have been addressed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-5-1-decouple-message-encoding-from-bincode.md`:
- Line 278: Replace the ambiguous phrase "`src/frame/metadata.rs` and/or
`src/serializer.rs`" with the clearer "`src/frame/metadata.rs` or
`src/serializer.rs`" in the document so it reads unambiguously; update that
exact phrase wherever it appears in the file
docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md (referencing the
snippet containing `src/frame/metadata.rs` and/or `src/serializer.rs`) to use
"or" instead of "and/or".
- Line 394: Update the sentence "Acceptance is complete only when all of the
following are true:" by removing the extraneous word "of" so it reads
"Acceptance is complete only when all the following are true:"; locate this
exact sentence in the document (the heading/paragraph containing "Acceptance is
complete only when all of the following are true:") and replace it accordingly
for conciseness.
- Around line 18-495: Change American English prose occurrences of "serializer",
"serialization" and "deserialization" to British English "serialiser",
"serialisation" and "deserialisation" throughout the document text (paragraphs,
bullets and inline prose) while explicitly preserving code identifiers and
literals (e.g. trait/struct names like Serializer, BincodeSerializer,
DeserializeContext, FrameMetadata::parse, FrameMetadata::Frame), file paths,
feature flags, and everything inside fenced code blocks; update the specific
prose instances flagged (e.g. "serializer-agnostic message adapter" →
"serialiser-agnostic message adapter", "Serde (serialization/deserialization)" →
"Serde (serialisation/deserialisation)", "immediate serializer rewrites" →
"immediate serialiser rewrites", and the other listed occurrences) and run a
quick pass to catch any other descriptive occurrences of those three words so
only natural-language text is altered.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
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/serializer_boundaries.rs Comment on file pub fn assert_captured_message_id(&self, expected: u32) -> TestResult {
let captured = self
.captured_context
.lock()
.ok()
.and_then(|state| *state)
.ok_or("captured context not available")?;
if captured.message_id != Some(expected) {
return Err(format!(
"expected captured message id {:?}, got {:?}",
Some(expected),
captured.message_id
)
.into());
}
Ok(())
}❌ New issue: Code Duplication |
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: Comment on file let deserialize_calls = Arc::new(AtomicUsize::new(0));
let serializer = CountingSerializer(counter.clone(), deserialize_calls.clone());❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
…ode traits Introduce a comprehensive ExecPlan document for roadmap item 9.5.1 that outlines the strategy to remove bincode-specific trait coupling from message encoding boundaries. This plan details adapter trait design, compatibility layers, deserialization context with metadata exposure, optional Serde bridge, testing approaches, progress checkpoints, risks, decisions, and documentation updates to ensure a serializer-agnostic interface while preserving existing bincode workflows. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…cument - Expanded acronyms on first mention for clarity. - Improved line breaks and punctuation for readability. - Reworded several risk and decision log items for better comprehension. - Updated terminology to avoid ambiguity and enhance professionalism. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Applied Rust syntax highlighting to the code snippets in the 9-5-1-decouple-message-encoding-from-bincode.md document to improve readability and clarity of the message encoding and decoding interface definitions. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
…ity boundaries Added guidance on maintenance cadence including updates to Progress, Decision Log, and Outcomes & Retrospective. Clarified authority boundaries for documentation artifacts, specifying roles of ExecPlan, ADR-005, and users-guide documents. Also included details on DeserializeContext fields alignment with metadata model to prevent drift. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
- Introduce serializer-agnostic encoding and decoding adapter traits `EncodeWith` and `DecodeWith` to replace the previous bincode-only `Message` trait dependency. - Add `DeserializeContext` struct to pass frame metadata and context to deserializers supporting version negotiation. - Enhance `Serializer` trait with `deserialize_with_context` method, defaulting to `deserialize` for backward compatibility. - Update core components (app, client, server, serializer implementations) to use the new serializer-agnostic traits. - Provide an optional Serde bridge behind the `serializer-serde` feature via `SerdeMessage`, `IntoSerdeMessage`, and `SerdeSerializerBridge` trait to reduce boilerplate for Serde serializers. - Update documentation, ADR, execution plans, user guides, examples, and roadmap to reflect the new serializer abstraction and migration guidance. - Introduce comprehensive tests, fixtures, and scenarios verifying legacy bincode compatibility, metadata context propagation, and serializer boundary behavior. This change enables greater flexibility for supporting alternative serializers and version-aware message decoding without breaking existing bincode-compatible code. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
devboxerhub
Bot
force-pushed
the
decouple-message-encoding-bincode-cpjk3l
branch
from
2398c1f to
925c373
Compare
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: Code Duplicationtests/fixtures/serializer_boundaries.rs: What lead to degradation?The module contains 2 functions with similar structure: SerializerBoundariesWorld.assert_captured_correlation_id,SerializerBoundariesWorld.assert_captured_message_id 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 |
Refactored assertion methods in serializer_boundaries test fixture to use a new generic helper method `assert_captured_field`. This removes repetitive code for checking captured context fields like message_id and correlation_id, improving code maintainability and clarity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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.
… Message support Introduce the marker trait `MessageCompatibilitySerializer` to explicitly opt-in serializers to legacy `Message` compatibility. This allows `T: Message` values to automatically satisfy `EncodeWith<Self>` and `DecodeWith<Self>`, enabling back-compatibility with existing `Message`-based encoding/decoding. Update existing serializers and tests to implement this trait as appropriate. Adjust inbound handling logic to optimize deserialization based on serializers' capabilities. Update documentation and examples to reflect this new explicit opt-in pattern and migration guidance. This change paves the way for more robust and serializer-specific encoding/decoding boundaries, improving flexibility and future extensibility. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
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.
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 5007 🏁 Script executed: Length of output: 4189
✓ Resolved (3/6)Individual Comment 1: adapter/adaptor spellingStatus: ✓ Resolved The ADR now uses "adaptor" consistently throughout (lines 52, 59, 79, 80, 88, 97, 120, 121, 125, 145). No instances of "adapter" remain. Individual Comment 3: BDD test coverageStatus: ✓ Resolved The BDD tests are fully implemented:
Individual Comment 4: "your" pronounStatus: ✓ Resolved Line 391 of ✗ Not Resolved (3/6)Overall Comment 1: Self: Sized object safety documentationStatus: ✗ Not resolved The
Consider adding a trait-level doc comment such as: /// # Object Safety
///
/// This trait is not object-safe due to `Self: Sized` bounds on serialization methods.
/// Code requiring `dyn Serializer` should migrate to concrete types or use a type-erased wrapper.
Overall Comment 2: ContextCapturingSerializer duplicationStatus: ✗ Not resolved
Both share similar logic for capturing Individual Comment 2: SerdeMessage wrapper complexityStatus: ✗ Not resolved The Serde bridge still requires explicit wrapping with
The suggested blanket impls: impl<S, T> EncodeWith<S> for T
where S: SerdeSerializerBridge + ?Sized, T: serde::Serializeare not present, so callers must still wrap/unwrap with Would you like me to help draft the object safety documentation or create an issue for the |
…s, runtime, and tests - Extract framing codec helpers into helpers/codec.rs - Move in-memory frame driving helpers to helpers/drive.rs - Move payload-oriented helpers to helpers/payloads.rs - Move runtime-level helpers for app execution to helpers/runtime.rs - Add comprehensive tests in helpers/tests/ - Expose helper functions via re-exports in helpers/mod.rs - Remove redundant code and macros from helpers.rs to the new modules - Add macros for push_expect! and recv_expect! in macros.rs - Update docs and tests accordingly This improves code modularity, readability, and maintainability of the wireframe_testing crate. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…coverage - Introduce ContextCapturingSerializer to capture DeserializeContext metadata during deserialization in tests. - Replace duplicate context capturing implementations with a shared module. - Update existing tests to use the new context capturing serializer for better consistency and metadata verification. - Add detailed docs and usage for SerdeMessage wrapper to clarify Serde bridge pattern. - Document non-object safe nature of Serializer trait and recommend usage patterns. This enhances the ability to test and verify context propagation through the serialization framework. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…anicking - Changed `push_expect!` and `recv_expect!` macros to return contextualized `io::Error` results rather than panicking on failure. - Updated test code to propagate errors using `?` operator instead of unwrapping. - Improved error diagnostics with file and line information in macro-generated errors. - Modified helper functions in `wireframe_testing` to return `io::Result` for decode and encode operations, improving error handling. - Updated various test files to handle new error returns consistently. This refactoring enhances robustness of test code by avoiding panics, providing better error handling and diagnostics. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2790 🏁 Script executed: Length of output: 761
✓ Resolved (5/6)Overall Comment 1: Self: Sized object safety documentationStatus: ✓ Resolved Lines 52-60 of /// # Object Safety
///
/// This trait is not object-safe. Its core methods include `Self: Sized`
/// bounds that prevent use with `dyn Serializer`. Code requiring dynamic
/// dispatch should use a concrete type-erased wrapper that provides
/// object-safe forwarding methods.
Overall Comment 2: ContextCapturingSerializer duplicationStatus: ✓ Resolved
Individual Comment 1: adapter/adaptor spellingStatus: ✓ Resolved (previously resolved) All instances in Individual Comment 3: BDD test coverageStatus: ✓ Resolved (previously resolved) BDD tests are fully implemented with 15 lines in the feature file, 60 lines of step definitions, and 21 lines of scenario wiring. Individual Comment 4: "your" pronounStatus: ✓ Resolved (previously resolved) Line 390 of ✗ Not Resolved (1/6)Individual Comment 2: SerdeMessage blanket implsStatus: ✗ Not resolved The blanket impl at line 118 of impl<S, T> EncodeWith<S> for T
where
S: crate::serializer::MessageCompatibilitySerializer + ?Sized,
T: Message, // ← Still requires Message traitThe requested feature-gated blanket impls for Would you like me to open an issue to track the Serde blanket impl enhancement? |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md (1)
18-59:⚠️ Potential issue | 🟡 MinorAlign remaining American spellings with en-GB-oxendict.
Replace remaining prose occurrences such as “serializer-boundary”, “deserialization”, and “Deserializers” with “serialiser-boundary”, “deserialisation”, and “Deserialisers” (and audit the rest of the document for the same pattern).
As per coding guidelines: “Use British English based on the Oxford English Dictionary (en-GB-oxendict)”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md` around lines 18 - 59, Change American English spellings to en-GB-oxendict variants throughout this document: replace "serializer-boundary" with "serialiser-boundary", "deserialization" with "serialisation", "Deserializers" (and any capitalised forms) with "Serialisers", and similarly convert "serializer"/"deserializer" occurrences to "serialiser"/"serialiser" as appropriate; audit the entire text for other US spellings and update them consistently (e.g., "serialize"/"serialize" → "serialise"/"serialise") while preserving punctuation and meaning.
🤖 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/users-guide.md`:
- Around line 381-387: The prose uses US spelling
"deserialization"/"deserializing" but our guideline requires British English;
update the occurrences in this paragraph (the sentence referencing
FrameMetadata::parse and the fallback to full deserialization, and the phrase
"without deserializing the payload") to British spelling: change "full
deserialization" to "full deserialisation" and "deserializing" to
"deserialising"; the surrounding symbols to check for context are
FrameMetadata::parse, EncodeWith<S>, DecodeWith<S>, Message, and the helper
names to_bytes/from_bytes to ensure only prose is altered.
In `@wireframe_testing/src/helpers/tests/helper_tests.rs`:
- Around line 35-73: The test drive_with_payloads_wraps_frames currently uses
expect() and panics; replace each expect() on WireframeApp::new, the .route(...)
call, serializer.serialize(&env), and serializer.deserialize::<Envelope>(first)
with map_err(|e| io::Error::new(io::ErrorKind::InvalidData, format!("...: {}",
e)))? (matching the pattern used elsewhere in the file) so each Result is
converted to an io::Error and propagated with the ? operator; ensure the error
messages include context (e.g., "failed to create app", "route registration
should succeed", "failed to serialize envelope", "failed to deserialise
envelope") to mirror the prior expect strings.
---
Duplicate comments:
In `@docs/execplans/9-5-1-decouple-message-encoding-from-bincode.md`:
- Around line 18-59: Change American English spellings to en-GB-oxendict
variants throughout this document: replace "serializer-boundary" with
"serialiser-boundary", "deserialization" with "serialisation", "Deserializers"
(and any capitalised forms) with "Serialisers", and similarly convert
"serializer"/"deserializer" occurrences to "serialiser"/"serialiser" as
appropriate; audit the entire text for other US spellings and update them
consistently (e.g., "serialize"/"serialize" → "serialise"/"serialise") while
preserving punctuation and meaning.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
docs/execplans/9-5-1-decouple-message-encoding-from-bincode.mddocs/users-guide.mdsrc/serializer.rstests/connection_actor_errors.rstests/connection_actor_fairness.rstests/connection_actor_shutdown.rstests/lifecycle.rstests/middleware_order.rstests/push.rstests/response.rstests/routes.rswireframe_testing/README.mdwireframe_testing/src/helpers/codec.rswireframe_testing/src/helpers/payloads.rswireframe_testing/src/helpers/tests/helper_tests.rswireframe_testing/src/helpers/tests/mod.rswireframe_testing/src/macros.rs
Owner
Author
|
@coderabbitai the engineer writes:
|
1 similar comment
Owner
Author
|
@coderabbitai the engineer writes:
|
Changed unwraps and expects to return io::Result with descriptive io::Error errors in helper_tests.rs tests. This improves error reporting by mapping creation, routing, serialization, and deserialization failures into io::Errors instead of panicking. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/message.rs is usually changed with: wireframe/src/preamble.rs
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Owner
Author
|
@coderabbitai the engineer writes:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Why the Original Suggestion Was FlawedMy earlier suggestion to add blanket The Current Design Is CorrectThe The wrapper:
RecommendationDo not implement either breaking alternative. The current design with I'm marking Individual Comment 2 (SerdeMessage wrapper complexity) as resolved — no code change is needed, and the original review suggestion was based on incomplete analysis of Rust's trait coherence constraints. |
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
Core functionality
API surface
Documentation & ADRs
Tests
ExecPlan & Validation
References
Validation & acceptance criteria
Validation plan (commands)
Risks & mitigations
Artifacts & notes
Task
Summary by Sourcery
Introduce serializer-agnostic message encoding/decoding adapters and deserialization context while preserving existing bincode flows and adding an optional Serde bridge.
New Features:
EncodeWithandDecodeWithadapter traits plusDeserializeContextto decouple message APIs from bincode-specific traits.serializer-serdefeature, includingSerdeMessageand helper traits for ergonomic integration.Enhancements:
Serializertrait to support context-aware deserialization and use adapter-based bounds, keepingBincodeSerializeras the default implementation via compatibility shims.Packettrait bounds to remove directMessagerequirements and update client/server/runtime APIs to depend on the new adapter traits instead.Build:
serdefeature and add aserializer-serdecrate feature to gate the optional Serde bridge implementation.Documentation:
Tests: