CodecError taxonomy, EOF handling, recovery policies, hooks, metrics#417
CodecError taxonomy, EOF handling, recovery policies, hooks, metrics#417
Conversation
WalkthroughIntroduce a CodecError taxonomy (Framing, Protocol, IO, EOF), add a pluggable RecoveryPolicy (Drop, Quarantine, Disconnect) with CodecErrorContext and hooks, surface codec errors through wireframe/app error types, add WireframeProtocol::on_eof and CODEC_ERRORS metric, and add tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Decoder as LengthDelimitedDecoder
participant Protocol as WireframeProtocol
participant Recovery as RecoveryPolicyHook
participant Manager as ConnectionManager
participant Metrics as Metrics
Client->>Decoder: send bytes / close
Decoder-->>Decoder: parse header and payload
alt complete frame parsed
Decoder->>Protocol: deliver frame
else EOF or codec error detected
Decoder->>Protocol: on_eof(error, partial_data, ctx)
Decoder->>Recovery: recovery_policy(&CodecError, &CodecErrorContext)
Recovery-->>Manager: return policy (Drop/Quarantine/Disconnect)
Manager->>Metrics: inc_codec_error(error_type, policy)
Manager-->>Client: apply policy (drop/quarantine/disconnect)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideIntroduce a structured CodecError taxonomy with associated recovery policies, wire it through the codec, app, and response error surfaces, add EOF-specific handling and protocol hooks, expose metrics and public re-exports, and cover the behavior with integration, BDD, and documentation updates. Sequence diagram for EOF handling and on_eof protocol hooksequenceDiagram
actor Peer
participant TcpStream
participant LengthDelimitedDecoder
participant InnerLengthCodec as LengthDelimitedCodec
participant ConnectionActor
participant ProtocolHooks
participant WireframeProtocol
Peer->>TcpStream: Close TCP connection
TcpStream->>LengthDelimitedDecoder: decode_eof(src)
alt buffer_is_empty
LengthDelimitedDecoder-->>TcpStream: Ok(None)
TcpStream-->>ConnectionActor: No_frame_clean_close
ConnectionActor->>ProtocolHooks: on_eof(CleanClose, [], ctx)
ProtocolHooks->>WireframeProtocol: on_eof(CleanClose, [], ctx)
else buffer_not_empty
LengthDelimitedDecoder->>InnerLengthCodec: decode_eof(src)
alt inner_returns_frame
InnerLengthCodec-->>LengthDelimitedDecoder: Ok(Some(frame))
LengthDelimitedDecoder-->>TcpStream: Ok(Some(Bytes))
else inner_returns_none_or_err
InnerLengthCodec-->>LengthDelimitedDecoder: Ok(None) or Err
LengthDelimitedDecoder->>LengthDelimitedDecoder: build_eof_error(src)
LengthDelimitedDecoder-->>TcpStream: Err(io::Error from CodecError::Eof)
TcpStream-->>ConnectionActor: Decode_error(io::Error)
ConnectionActor->>ProtocolHooks: on_eof(EofError(MidHeader or MidFrame), partial_data, ctx)
ProtocolHooks->>WireframeProtocol: on_eof(EofError(...), partial_data, ctx)
end
end
Class diagram for codec error taxonomy and recovery infrastructureclassDiagram
class FramingError {
<<enumeration>>
+OversizedFrame(size: usize, max: usize)
+InvalidLengthEncoding
+IncompleteHeader(have: usize, need: usize)
+ChecksumMismatch(expected: u32, actual: u32)
+EmptyFrame
}
class ProtocolError {
<<enumeration>>
+MissingHeader(field: String)
+UnsupportedVersion(version: u32)
+UnknownMessageType(type_id: u32)
+SequenceViolation(expected: u64, actual: u64)
+InvalidStateTransition(from: String, to: String)
}
class EofError {
<<enumeration>>
+CleanClose
+MidFrame(bytes_received: usize, expected: usize)
+MidHeader(bytes_received: usize, header_size: usize)
}
class IoError {
}
class CodecError {
+Framing(err: FramingError)
+Protocol(err: ProtocolError)
+Io(err: IoError)
+Eof(err: EofError)
+default_recovery_policy() RecoveryPolicy
+is_clean_close() bool
+should_disconnect() bool
+error_type() str
}
class RecoveryPolicy {
<<enumeration>>
+Drop
+Quarantine
+Disconnect
}
class CodecErrorContext {
+connection_id: Option~u64~
+peer_address: Option~SocketAddr~
+correlation_id: Option~u64~
+codec_state: Option~String~
+new() CodecErrorContext
+with_connection_id(id: u64) CodecErrorContext
+with_peer_address(addr: SocketAddr) CodecErrorContext
+with_correlation_id(id: u64) CodecErrorContext
+with_codec_state(state: String) CodecErrorContext
}
class RecoveryPolicyHook {
<<interface>>
+recovery_policy(error: CodecError, ctx: CodecErrorContext) RecoveryPolicy
+quarantine_duration(error: CodecError, ctx: CodecErrorContext) Duration
+on_error(error: CodecError, ctx: CodecErrorContext, policy: RecoveryPolicy) void
}
class DefaultRecoveryPolicy {
+recovery_policy(error: CodecError, ctx: CodecErrorContext) RecoveryPolicy
+quarantine_duration(error: CodecError, ctx: CodecErrorContext) Duration
+on_error(error: CodecError, ctx: CodecErrorContext, policy: RecoveryPolicy) void
}
class RecoveryConfig {
+max_consecutive_drops: u32
+quarantine_duration: Duration
+log_dropped_frames: bool
+max_consecutive_drops(count: u32) RecoveryConfig
+quarantine_duration(duration: Duration) RecoveryConfig
+log_dropped_frames(enabled: bool) RecoveryConfig
}
class AppWireframeError {
<<enum_like>>
+DuplicateRoute(id: u32)
+Codec(err: CodecError)
}
class ResponseWireframeError {
<<enum_like>>
+Io(err: IoError)
+Protocol(err: E)
+Codec(err: CodecError)
+from_io(e: IoError) ResponseWireframeError
+from_codec(e: CodecError) ResponseWireframeError
+is_clean_close() bool
}
CodecError *-- FramingError
CodecError *-- ProtocolError
CodecError *-- EofError
CodecError *-- RecoveryPolicy
RecoveryPolicyHook <|.. DefaultRecoveryPolicy
RecoveryPolicyHook ..> CodecError
RecoveryPolicyHook ..> CodecErrorContext
RecoveryPolicyHook ..> RecoveryPolicy
RecoveryConfig ..> RecoveryPolicy
AppWireframeError o-- CodecError
ResponseWireframeError o-- CodecError
CodecErrorContext ..> SocketAddr
CodecError ..> IoError
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +299 to +309 fn decode_eof_with_empty_buffer_returns_clean_close() {
let codec = LengthDelimitedFrameCodec::new(128);
let mut decoder = codec.decoder();
let mut buf = BytesMut::new();
let err = decoder
.decode_eof(&mut buf)
.expect_err("expected clean close error");
assert_eq!(err.kind(), io::ErrorKind::UnexpectedEof);
assert!(err.to_string().contains("connection closed"));
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +170 to +180 fn decoder_eof_empty_buffer_returns_clean_close() {
let codec = LengthDelimitedFrameCodec::new(128);
let mut decoder = codec.decoder();
let mut buf = BytesMut::new();
let err = decoder
.decode_eof(&mut buf)
.expect_err("expected clean close");
assert_eq!(err.kind(), io::ErrorKind::UnexpectedEof);
assert!(err.to_string().contains("connection closed"));
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
leynos
force-pushed
the
terragon/introduce-codecerror-taxonomy-3abr1i
branch
from
6bc921e to
fd0a4a2
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
build_eof_erroryou callsrc.get(..LENGTH_HEADER_SIZE)and thenget_u32on the result, butBytesMut::getreturns a&[u8](not aBuf), so this either won’t compile or won’t do what you expect; consider wrapping the slice in aCursoror usingBytes::copy_from_sliceand thenBytes::get_u32to parse the length prefix safely. - There is quite a bit of duplicated EOF/oversized-frame behaviour testing between
src/codec.rstests andtests/codec_error.rs(e.g.assert_decode_eof_errorand related cases); you may want to consolidate these helpers/tests into one place to avoid divergence over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `build_eof_error` you call `src.get(..LENGTH_HEADER_SIZE)` and then `get_u32` on the result, but `BytesMut::get` returns a `&[u8]` (not a `Buf`), so this either won’t compile or won’t do what you expect; consider wrapping the slice in a `Cursor` or using `Bytes::copy_from_slice` and then `Bytes::get_u32` to parse the length prefix safely.
- There is quite a bit of duplicated EOF/oversized-frame behaviour testing between `src/codec.rs` tests and `tests/codec_error.rs` (e.g. `assert_decode_eof_error` and related cases); you may want to consolidate these helpers/tests into one place to avoid divergence over time.
## Individual Comments
### Comment 1
<location> `src/codec.rs:160-169` </location>
<code_context>
+fn build_eof_error(src: &BytesMut) -> io::Error {
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `Buf::get_u32` on a `&[u8]` slice will not compile; parse the length prefix without relying on `Buf` for slices.
`src.get(..LENGTH_HEADER_SIZE)` returns an `Option<&[u8]>`, so `let Some(mut header)` makes `header` a `&[u8]` that does not implement `Buf`; `header.get_u32()` will not compile. Instead, parse the length manually:
```rust
overflowed
let header = match src.get(..LENGTH_HEADER_SIZE) {
Some(h) => h,
None => {
return CodecError::Eof(EofError::MidHeader {
bytes_received,
header_size: LENGTH_HEADER_SIZE,
}).into();
}
};
let expected = u32::from_be_bytes(header.try_into().expect("slice is 4 bytes")) as usize;
```
Or, if you prefer `Buf`, wrap the slice in a `Cursor` before calling `get_u32()`.
</issue_to_address>
### Comment 2
<location> `src/app/error.rs:29-38` </location>
<code_context>
+ Codec(#[from] CodecError),
}
+impl PartialEq for WireframeError {
+ fn eq(&self, other: &Self) -> bool {
+ match (self, other) {
+ (Self::DuplicateRoute(a), Self::DuplicateRoute(b)) => a == b,
+ // CodecError cannot be compared for equality (contains io::Error)
+ // All other combinations are not equal
+ _ => false,
+ }
+ }
+}
+
+impl Eq for WireframeError {}
+
/// Errors produced when sending a handler response over a stream.
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The manual `PartialEq`/`Eq` impl only compares `DuplicateRoute`, making `Codec` variants never equal even when identical.
Because the manual impl now treats all non-`DuplicateRoute` variants as always-unequal (including identical `Codec` errors), any existing `assert_eq!` or equality-based logic on `WireframeError` will behave differently and potentially unexpectedly.
You could either:
- Remove `PartialEq`/`Eq` from `WireframeError` and rely on pattern matching, or
- Restrict equality to a test-only helper (e.g., feature-gated) if it’s primarily used in tests.
If you keep this impl, it’d be good to audit current equality usage to ensure this behavior is intentional.
Suggested implementation:
```rust
#[error("codec error: {0}")]
Codec(#[from] CodecError),
}
/// Errors produced when sending a handler response over a stream.
#[derive(Debug, Error)]
#[non_exhaustive]
```
1. Search for any usages of `WireframeError` in equality contexts (e.g., `assert_eq!(..., WireframeError::...)` or `if err1 == err2`).
2. Replace these with explicit pattern matching, for example:
- `matches!(err, WireframeError::DuplicateRoute(id) if id == expected_id)`
- or `matches!(err, WireframeError::Codec(_))` when only the variant matters.
3. If tests rely heavily on equality, consider adding a test-only helper under `#[cfg(test)]`, such as:
```rust
#[cfg(test)]
impl WireframeError {
pub fn same_variant(&self, other: &Self) -> bool {
use WireframeError::*;
match (self, other) {
(DuplicateRoute(a), DuplicateRoute(b)) => a == b,
(Codec(_), Codec(_)) => true,
_ => false,
}
}
}
```
and then use `assert!(err1.same_variant(&err2));` in tests instead of `assert_eq!`.
</issue_to_address>
### Comment 3
<location> `src/codec/error.rs:1` </location>
<code_context>
+//! Error types for the codec layer.
+//!
+//! This module provides a structured error taxonomy that distinguishes between
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural (Cucumber) tests that exercise the new codec error taxonomy and recovery behaviour end-to-end; the newly added feature files and step/world modules are currently empty.
Implement behavioural scenarios in `tests/features/codec_error.feature` and corresponding step/world code in `tests/steps/codec_error_steps.rs`, `tests/worlds/codec_error.rs`, and `tests/codec_error.rs` so they actually drive a Wireframe connection through framing, protocol, I/O, and EOF error cases.
You already have solid unit coverage in `src/codec/error.rs`, `src/codec/recovery.rs`, and `src/codec.rs::tests`, but the behavioural side is missing: the new files under `tests/` and `tests/features/` are empty, so no end-to-end assertions verify that recovery policies, hooks, and metrics are applied correctly in a running app.
Define concrete scenarios (e.g. oversized frame dropped, mid-frame EOF quarantine/disconnect, custom `RecoveryPolicyHook` override) and wire them into the existing Cucumber harness to satisfy the requirement for behavioural tests for this new feature.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural and unit tests are required.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd0a4a27ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Fix all issues with AI agents
In @docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md:
- Around line 21-40: The progress checklist is out of date: update the remaining
unchecked items to checked since the PR implements the CodecError taxonomy and
related features; specifically mark the entries for creating src/codec/error.rs
(CodecError), src/codec/recovery.rs (recovery policy types/traits), extending
WireframeError (in src/response.rs and src/app/error.rs) with a Codec variant,
updating LengthDelimitedDecoder to emit structured EOF errors, integrating
CodecErrorContext population, adding recovery policy evaluation in
src/app/connection.rs, adding structured logging and metrics for codec errors,
adding the on_eof hook to ProtocolHooks, and all unit/integration/Cucumber tests
as completed, then save the document so the checklist reflects the implemented
work.
- Around line 130-132: The three EOF sub-variant lines for CleanClose, MidFrame,
and MidHeader use inconsistent dash/parenthesis formatting; update each list
item to use a consistent explanatory separator (either an em-dash or a colon)
and consistent wording style so all three read uniformly (e.g., "CleanClose —
EOF at frame boundary (normal)", "MidFrame — EOF during payload read
(premature)", "MidHeader — EOF during header read (premature)"), keeping the
variant names exactly as CleanClose, MidFrame, and MidHeader.
- Line 142: Replace British "-isation" spelling with Oxford "-ization" in this
document: find any instance of "customisation" (and other "-isation" words) near
the RecoveryPolicyHook description and change to "customization" (and analogous
words to use the -ize suffix) so the text conforms to the en-GB-oxendict
guideline; update the sentence containing "RecoveryPolicyHook" accordingly to
use the corrected spelling.
In @src/codec.rs:
- Around line 143-156: The current decode_eof implementation drops the original
error from self.inner.decode_eof(src); instead, preserve that inner error for
diagnostics by mapping Err(e) to a new EOF error that wraps or chains e as its
source (e.g., construct the error from build_eof_error(src) but attach e as the
source/inner error or return a composed error type), so update the match arm
handling Err(_) to capture Err(inner_err) and return
Err(eof_err.with_source(inner_err) or otherwise chain inner_err into the
returned error while keeping use of build_eof_error(src) for context.
- Around line 159-189: The match arm in build_eof_error contains an unreachable
defensive else for src.get(..LENGTH_HEADER_SIZE) because bytes_received >=
LENGTH_HEADER_SIZE guarantees the slice exists; remove that else block and
simplify the code to directly obtain the header slice (e.g., bind header from
src.get(..LENGTH_HEADER_SIZE) without the Some/else) and parse the u32 length,
keeping the subsequent CodecError::Eof(EofError::MidFrame { ... }) logic intact.
In @src/codec/error.rs:
- Around line 312-441: The file is over the 400-line limit because the internal
test module #[cfg(test)] mod tests { ... } is included inline; extract that
entire tests module into a separate test file (e.g., create error_tests.rs with
the same module contents and either place it in the same directory as a sibling
or include it via #[path = "error_tests.rs"] mod tests;) and remove the inline
#[cfg(test)] mod tests block from the current file so the production code file
drops below 400 lines; ensure all referenced symbols (CodecError, FramingError,
ProtocolError, EofError, RecoveryPolicy, should_disconnect, is_clean_close) are
in scope in the new test file (use super::* or appropriate use statements) so
tests compile unchanged.
In @src/metrics.rs:
- Around line 141-149: The inc_codec_error function allocates unnecessarily by
calling .to_string() on its &str params; update the counter! invocation in
inc_codec_error to pass the &str labels directly (e.g., "error_type" =>
error_type, "recovery_policy" => recovery_policy) so CODEC_ERRORS uses &str
labels without heap allocations, matching the other metric functions.
In @src/response.rs:
- Around line 240-257: WireframeError::is_clean_close currently duplicates the
pattern match for CodecError; change it to delegate to codec's implementation by
matching only the WireframeError::Codec variant and returning the inner
crate::codec::CodecError::is_clean_close() result (return false for all other
variants). Update the is_clean_close method to: if let WireframeError::Codec(e)
= self { e.is_clean_close() } else { false }, keeping the from_codec constructor
unchanged.
In @tests/steps/codec_error_steps.rs:
- Around line 47-48: The literal 100 in when_client_closes_abrupt is a magic
number—define a named constant like const PARTIAL_BYTES: usize = 100 (near the
top of the test module) and replace the call world.record_mid_frame_eof(100)
with world.record_mid_frame_eof(PARTIAL_BYTES) so the intent is explicit;
reference the when_client_closes_abrupt function and the record_mid_frame_eof
method when making the change.
- Around line 67-72: The then_server_oversized step is redundantly mutating
state already set in when_client_sends_oversized; remove the calls to
world.set_error_type("framing") and world.set_framing_variant("oversized") from
then_server_oversized and leave only the verification call
(world.verify_recovery_policy("drop")) so that the step only asserts behavior
rather than re-establishing state set by when_client_sends_oversized; reference
functions: then_server_oversized, when_client_sends_oversized and methods
set_error_type, set_framing_variant, verify_recovery_policy on CodecErrorWorld.
- Around line 74-78: The code in then_recovery_policy unnecessarily converts the
incoming String to a Box<str>; change it to use as_str() and pass a &str to
verify_recovery_policy instead: in function then_recovery_policy replace the
into_boxed_str() usage with either let policy = policy.as_str(); or call
world.verify_recovery_policy(policy.as_str()) directly so the parameter to
CodecErrorWorld::verify_recovery_policy is a &str.
- Around line 17-32: The given_error_type_variant helper silently ignores
unknown error_type values and uses verbose into_boxed_str(); change the String
conversions to use as_str() and validate the match exhaustively: after setting
world.set_error_type(error_type.as_str()) and handling "framing" and "eof" via
world.set_framing_variant(variant.as_str()) /
world.set_eof_variant(variant.as_str()), return an Err(TestResult) with a clear
message including the unrecognized error_type in the match _ branch instead of
doing nothing so typos in feature files surface as test failures.
In @tests/worlds/codec_error.rs:
- Around line 42-50: The doc comment for CodecErrorWorld is placed after the
#[derive(..., World)] attribute which breaks rustdoc/attribute ordering; move
the triple-slash comment so it precedes the derive attribute: place the /// Test
world for codec error taxonomy scenarios. line immediately above the
#[derive(Debug, Default, World)] that applies to the struct named
CodecErrorWorld so the documentation is attached correctly.
- Around line 175-180: record_mid_frame_eof currently sets EofError::MidFrame
with expected: 0 which loses diagnostic info; change record_mid_frame_eof(&mut
self, bytes: usize) to accept an expected: usize parameter (e.g.,
record_mid_frame_eof(&mut self, bytes: usize, expected: usize)) and set
detected_eof = Some(EofError::MidFrame { bytes_received: bytes, expected });
update all call sites in tests to pass the correct expected value (or 0 where
unknown) so tests can inspect the real expected byte count; alternatively if you
prefer to keep the current signature, rename the method to
record_mid_frame_eof_with_unknown_expected to make the limitation explicit (but
prefer adding the expected parameter).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
docs/adr-004-pluggable-protocol-codecs.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/roadmap.mddocs/users-guide.mdsrc/app/error.rssrc/codec.rssrc/codec/error.rssrc/codec/recovery.rssrc/hooks.rssrc/lib.rssrc/metrics.rssrc/response.rstests/codec_error.rstests/cucumber.rstests/features/codec_error.featuretests/steps/codec_error_steps.rstests/steps/mod.rstests/world.rstests/worlds/codec_error.rstests/worlds/mod.rs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
tests/steps/mod.rssrc/metrics.rstests/steps/codec_error_steps.rssrc/codec/error.rstests/codec_error.rstests/worlds/mod.rssrc/response.rstests/worlds/codec_error.rssrc/hooks.rssrc/lib.rstests/cucumber.rstests/world.rssrc/codec/recovery.rssrc/app/error.rssrc/codec.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/steps/mod.rssrc/metrics.rstests/steps/codec_error_steps.rssrc/codec/error.rstests/codec_error.rstests/worlds/mod.rssrc/response.rstests/worlds/codec_error.rssrc/hooks.rssrc/lib.rstests/cucumber.rstests/world.rssrc/codec/recovery.rssrc/app/error.rssrc/codec.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/contents.md)
Follow the documentation style guide conventions when writing project documentation
Write official documentation with separate guides for 'Duplex Messaging & Pushes', 'Streaming Responses', and 'Message Fragmentation', each including runnable examples and explaining relevant concepts and APIs
docs/**/*.md: Use markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ('-ize' / '-yse' / '-our'), with the exception of the filenameLICENSE.
Reference the documentation style guide provided atdocs/documentation-style-guide.mdfor documentation writing standards.Documentation describing multi-packet streaming MUST reflect the tuple-based API and explain helper constructors that prepare the channel pair
Replace or retire documentation that refers to Cucumber, including guides and examples in docs/
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization', suffix -lyse in words like 'analyse' and 'paralyse', suffix -our in words like 'colour' and 'behaviour', suffix -re in words like 'calibre' and 'centre', double 'l' in words like 'cancelled' and 'counsellor', maintain the 'e' in words like 'likeable' and 'liveable', suffix -ogue in words like 'analogue' and 'catalogue'
The word 'outwith' is acceptable in documentation
Keep US spelling when used in an API, for example 'color'
Use the Oxford comma: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Lille Industries are expanding'
Write hea...
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/users-guide.mddocs/adr-004-pluggable-protocol-codecs.mddocs/roadmap.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
Every doctest should validate the public API of a crate from the perspective of an external user, treating each documentation test as a separate temporary crate that imports the library as an external dependency
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/users-guide.mddocs/adr-004-pluggable-protocol-codecs.mddocs/roadmap.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes in Markdown.
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/users-guide.mddocs/adr-004-pluggable-protocol-codecs.mddocs/roadmap.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/users-guide.mddocs/adr-004-pluggable-protocol-codecs.mddocs/roadmap.md
docs/adr-[0-9][0-9][0-9]-*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/adr-[0-9][0-9][0-9]-*.md: ADR naming convention: Name ADR files using the pattern 'adr-NNN-short-description.md', where NNN is a zero-padded sequence number (e.g. 'adr-001-async-fixtures-and-tests.md'). Place ADRs in the 'docs/' directory
ADR required sections: Include Status (one of 'Proposed', 'Accepted', 'Superseded', or 'Deprecated'; for 'Accepted' status include the date and a brief summary of what was decided), Date (format: YYYY-MM-DD), and Context and Problem Statement sections in order
ADR conditional sections: Include Decision Drivers, Requirements, Options Considered, Decision Outcome / Proposed Direction, Goals and Non-Goals, Migration Plan, Known Risks and Limitations, Outstanding Decisions, and Architectural Rationale sections as appropriate to the decision's complexity
ADR formatting: Use second-level headings (##) for major sections and third-level headings (###) for subsections (e.g. phases, option names)
ADR formatting: Use tables to compare options when multiple dimensions are relevant and include a caption below the table (e.g. 'Table 1: Trade-offs between X and Y.')
ADR formatting: Include code snippets with language identifiers when illustrating technical approaches. Use 'no_run' for illustrative Rust code that should not be executed
ADR formatting: Add screen reader descriptions before complex diagrams or code blocks
ADR formatting: Reference external sources using inline links or footnotes
Files:
docs/adr-004-pluggable-protocol-codecs.md
docs/**/roadmap*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/roadmap*.md: Roadmap task writing: Define outcomes, not intentions. Phrase tasks in terms of the capability delivered (e.g. 'Implement role-based access control for API endpoints'), not aspirations
Roadmap task writing: Quantify completion criteria with measurable finish lines (e.g. '90% test coverage for new modules', 'response times under 200ms')
Roadmap task writing: Break into atomic increments. Ensure tasks can be completed in weeks, not quarters. Large goals should be decomposed into clear, deliverable units
Roadmap task writing: Tie to dependencies and sequencing. Document prerequisites so tasks can be scheduled realistically
Roadmap task writing: Bound scope explicitly. Note both in-scope and out-of-scope elements (e.g. 'Build analytics dashboard (excluding churn prediction)')
Roadmap formatting: Use dotted numbering for phases (1, 2, 3, …), steps (1.1, 1.2, 1.3, …), and headline tasks (1.1.1, 1.1.2, 1.1.3, …)
Roadmap formatting: Precede task and sub-task items with a GitHub-Flavoured Markdown (GFM) checkbox ([]) to track completion status
Roadmap formatting: Note non-linear dependencies explicitly using dotted notation (e.g. 'Requires 2.3.1')
Roadmap formatting: Include explicit success criteria only where not immediately obvious from the task description
Roadmap formatting: Where applicable, cite the relevant design document section for each task (e.g. 'See design-doc.md §3.2')
Files:
docs/roadmap.md
🧠 Learnings (2)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
tests/steps/mod.rssrc/metrics.rstests/steps/codec_error_steps.rssrc/codec/error.rstests/codec_error.rstests/worlds/mod.rssrc/response.rstests/worlds/codec_error.rssrc/hooks.rssrc/lib.rstests/cucumber.rstests/world.rssrc/codec/recovery.rssrc/app/error.rssrc/codec.rs
📚 Learning: 2026-01-06T18:51:34.645Z
Learnt from: CR
Repo: leynos/wireframe PR: 0
File: docs/roadmap.md:0-0
Timestamp: 2026-01-06T18:51:34.645Z
Learning: Applies to docs/src/**/*.rs : Introduce a `CodecError` taxonomy separating framing, protocol, and IO failures
Applied to files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mdsrc/codec/error.rs
🧬 Code graph analysis (7)
src/metrics.rs (3)
src/codec/error.rs (1)
error_type(288-295)tests/codec_error.rs (1)
recovery_policy(147-149)src/codec/recovery.rs (1)
recovery_policy(208-211)
tests/steps/codec_error_steps.rs (1)
src/codec/error.rs (1)
error_type(288-295)
src/response.rs (1)
src/codec/error.rs (1)
is_clean_close(254-254)
tests/worlds/codec_error.rs (3)
src/codec.rs (1)
default(120-124)src/codec/recovery.rs (1)
default(277-283)src/codec/error.rs (1)
error_type(288-295)
src/codec/recovery.rs (5)
src/codec.rs (3)
default(120-124)new(102-106)correlation_id(84-84)src/hooks.rs (1)
default(141-150)src/extractor.rs (1)
state(82-90)tests/codec_error.rs (4)
recovery_policy(147-149)recovery_policy_default_is_drop(122-124)context_with_peer_address(109-115)recovery_config_builder(127-138)tests/multi_packet.rs (1)
count(71-76)
src/app/error.rs (2)
src/codec/error.rs (1)
from(299-309)src/response.rs (3)
from(110-110)from(114-114)from(233-233)
src/codec.rs (2)
tests/codec_error.rs (1)
assert_decode_eof_error(181-202)src/codec/error.rs (1)
from(299-309)
🪛 LanguageTool
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
[typographical] ~94-~94: To join two clauses or introduce examples, consider using an em dash.
Context: ... Codec implementation: - src/codec.rs - FrameCodec trait and `LengthDelimitedF...
(DASH_RULE)
[typographical] ~95-~95: To join two clauses or introduce examples, consider using an em dash.
Context: ...tedFrameCodec-src/codec/examples.rs- Example codec implementations -src/app...
(DASH_RULE)
[typographical] ~96-~96: To join two clauses or introduce examples, consider using an em dash.
Context: ...mentations - src/app/combined_codec.rs - Adapter for Tokio Framed Error handli...
(DASH_RULE)
[typographical] ~100-~100: To join two clauses or introduce examples, consider using an em dash.
Context: ...ndling paths: - src/app/connection.rs - Connection processing and error handling...
(DASH_RULE)
[typographical] ~101-~101: To join two clauses or introduce examples, consider using an em dash.
Context: ...r handling - src/app/frame_handling.rs - Frame deserialization and response forwa...
(DASH_RULE)
[typographical] ~102-~102: To join two clauses or introduce examples, consider using an em dash.
Context: ...and response forwarding - src/hooks.rs - Protocol hooks including error callbacks...
(DASH_RULE)
[typographical] ~106-~106: To join two clauses or introduce examples, consider using an em dash.
Context: ...nfrastructure: - tests/frame_codec.rs - Codec integration tests - `tests/feature...
(DASH_RULE)
[typographical] ~107-~107: To join two clauses or introduce examples, consider using an em dash.
Context: ...ec integration tests - tests/features/ - Cucumber feature files - tests/steps/ ...
(DASH_RULE)
[typographical] ~108-~108: To join two clauses or introduce examples, consider using an em dash.
Context: ...les - tests/steps/ and tests/worlds/ - Step definitions and world types Docume...
(DASH_RULE)
[typographical] ~112-~112: To join two clauses or introduce examples, consider using an em dash.
Context: ...entation to update: - docs/roadmap.md - Mark 9.1.2 done - docs/users-guide.md ...
(DASH_RULE)
[typographical] ~113-~113: To join two clauses or introduce examples, consider using an em dash.
Context: ... Mark 9.1.2 done - docs/users-guide.md - Public interface documentation - `docs/a...
(DASH_RULE)
[typographical] ~114-~114: To join two clauses or introduce examples, consider using an em dash.
Context: ...cs/adr-004-pluggable-protocol-codecs.md` - Design decisions ## Plan of Work ### P...
(DASH_RULE)
[typographical] ~123-~123: To join two clauses or introduce examples, consider using an em dash.
Context: ...r classification: 1. Framing errors - Wire-level frame boundary issues (e.g., ...
(DASH_RULE)
[typographical] ~125-~125: To join two clauses or introduce examples, consider using an em dash.
Context: ...hecksum mismatch) 2. Protocol errors - Semantic violations after frame extracti...
(DASH_RULE)
[typographical] ~128-~128: To join two clauses or introduce examples, consider using an em dash.
Context: ... sequence violation) 3. IO errors - Transport layer failures (wrapped `io::E...
(DASH_RULE)
[typographical] ~129-~129: To join two clauses or introduce examples, consider using an em dash.
Context: ... (wrapped io::Error) 4. EOF errors - End-of-stream handling with three sub-va...
(DASH_RULE)
[typographical] ~131-~131: To join two clauses or introduce examples, consider using an em dash.
Context: ... frame boundary (normal) - MidFrame - EOF during payload read (premature) -...
(DASH_RULE)
[typographical] ~132-~132: To join two clauses or introduce examples, consider using an em dash.
Context: ...ayload read (premature) - MidHeader - EOF during header read (premature) Each...
(DASH_RULE)
[typographical] ~136-~136: To join two clauses or introduce examples, consider using an em dash.
Context: ...cy()method returning one of: -Drop` - Discard the malformed frame and continue...
(DASH_RULE)
[typographical] ~137-~137: To join two clauses or introduce examples, consider using an em dash.
Context: ...inue (recoverable errors) - Quarantine - Pause the connection temporarily (rate l...
(DASH_RULE)
[typographical] ~138-~138: To join two clauses or introduce examples, consider using an em dash.
Context: ...mporarily (rate limiting) - Disconnect - Terminate the connection (fatal errors) ...
(DASH_RULE)
[style] ~142-~142: Would you like to use the Oxford spelling “customization”? The spelling ‘customisation’ is also correct.
Context: ... RecoveryPolicyHook trait that allows customisation of error handling behaviour: - `Recove...
(OXFORD_SPELLING_Z_NOT_S)
[typographical] ~171-~171: Two consecutive dots
Context: ...e)for EOF at frame boundary 3. ReturnCodecError::Eof(EofError::MidFrame{..})for EOF during payload 4. ReturnCo...
(DOUBLE_PUNCTUATION)
[typographical] ~172-~172: Two consecutive dots
Context: ...{..})for EOF during payload 4. ReturnCodecError::Eof(EofError::MidHeader{..})for EOF during header 5. Maintaini...
(DOUBLE_PUNCTUATION)
[uncategorized] ~232-~232: Loose punctuation mark.
Context: ...documentation: 1. docs/users-guide.md: - Add section on codec error handlin...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~238-~238: Loose punctuation mark.
Context: ...cs/adr-004-pluggable-protocol-codecs.md: - Document the CodecError` taxonomy...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~243-~243: Loose punctuation mark.
Context: ...OF handling design 3. docs/roadmap.md: - Mark all 9.1.2 sub-items as done ...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~255-~255: Possible typo: you repeated a word.
Context: ...CodecError::default_recovery_policy() implementation - From<CodecError> for io::Error implementation 2. Create src/codec/recovery.rs with...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~260-~260: Using four (or more) nouns in a row may decrease readability.
Context: ...ryPolicyenum -CodecErrorContextstruct -RecoveryPolicyHooktrait -DefaultRecoveryPolicyimplementation -RecoveryConfigbuilder config 3. Updatesrc/codec.rs`: - ...
(FOUR_NN)
docs/users-guide.md
[typographical] ~175-~175: To join two clauses or introduce examples, consider using an em dash.
Context: ... Error categories: - FramingError - Wire-level frame boundary issues (oversi...
(DASH_RULE)
[typographical] ~177-~177: To join two clauses or introduce examples, consider using an em dash.
Context: ...atches, empty frames). - ProtocolError - Semantic violations after frame extracti...
(DASH_RULE)
[typographical] ~179-~179: To join two clauses or introduce examples, consider using an em dash.
Context: ... types, invalid versions). - io::Error - Transport layer failures (connection res...
(DASH_RULE)
[typographical] ~180-~180: To join two clauses or introduce examples, consider using an em dash.
Context: ...nnection resets, timeouts). - EofError - End-of-stream conditions with clean/mid-...
(DASH_RULE)
🔍 Remote MCP Deepwiki
Summary of Relevant Context for PR #417 Review
Based on my search of the leynos/wireframe repository, here is the key context that applies to this CodecError taxonomy PR:
Existing Error Handling Architecture
The framework currently uses a comprehensive WireframeError<E> enum that distinguishes between unrecoverable transport-level I/O errors (Io) and protocol-specific logical errors (Protocol). The new CodecError taxonomy in PR #417 adds a third category specifically for codec-layer errors—introducing a more granular error classification below the protocol level. This aligns with the existing architecture's layered approach: Transport → Framing (Codec) → Protocol → Application.
Connection Context and Observability Integration
The ConnectionContext struct and peer_addr tracking are already established patterns in the codebase. The PR's CodecErrorContext (with fields for connection_id, peer_address, correlation_id, codec_state) follows the same design pattern and fits naturally into the existing connection lifecycle hooks. The ConnectionInfo extractor already provides peer address information to handlers, so the PR's use of peer_address in error context is consistent with current practices.
Metrics and Observability Implementation
The framework exposes metrics via the metrics crate with Prometheus integration, including counters like wireframe_errors_total (with kind labels) and wireframe_frames_processed_total (with direction labels). The PR's new CODEC_ERRORS metric (wireframe_codec_errors_total) with error_type and recovery_policy labels follows this established pattern of labeled counters and integrates cleanly with the existing conditional compilation pattern (#[cfg(feature = "metrics")]).
API Stability and Backward Compatibility
The project enforces strict API stability through comprehensive documentation, caret-locked dependencies, and phased development. The PR is backward-compatible—all additions are additive with new public exports and optional new code paths. The From<CodecError> for io::Error conversion ensures existing code accepting io::Error continues to work without modification.
LengthDelimitedDecoder EOF Semantics
The existing LengthPrefixedProcessor returns Ok(None) when insufficient data is available to form a complete frame. The PR's new LengthDelimitedDecoder::decode_eof() method and structured EofError types (CleanClose, MidFrame, MidHeader) provide semantic clarity around what constitutes a clean EOF versus a protocol violation—distinguishing between "no more data at a frame boundary" (CleanClose → Ok(None)) and "incomplete frame received" (MidFrame/MidHeader → error). Per the commit history, this behavior was refined to return Ok(None) for clean closes rather than an error.
Recovery Policies and Protocol Hooks
The PR introduces the RecoveryPolicyHook trait with recovery_policy(), quarantine_duration(), and on_error() methods, plus the on_eof hook in WireframeProtocol. These are new extension points that parallel the existing pattern of protocol lifecycle hooks (e.g., handle_error on WireframeProtocol), allowing applications to customize behavior at error boundaries.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
🔇 Additional comments (46)
docs/roadmap.md (1)
352-361: Roadmap status update accurately reflects delivered work.The completion markers for 9.1.2 align with the CodecError taxonomy, recovery policies, error surface extensions, EOF handling, and test coverage introduced in this PR.
src/metrics.rs (2)
36-44: LGTM: Metric constant follows established conventions.The CODEC_ERRORS constant is appropriately named and documented with clear Prometheus output examples demonstrating the label dimensions.
151-152: LGTM: No-op implementation follows established pattern.The disabled-feature version correctly ignores parameters and matches the pattern of other metric no-ops in this file.
tests/steps/mod.rs (1)
8-8: LGTM: Module declaration follows established pattern.The codec_error_steps module is wired consistently with the other step definitions in this aggregation module.
tests/world.rs (1)
10-10: LGTM: World re-export follows established pattern.CodecErrorWorld is appropriately positioned alphabetically and follows the same re-export pattern as other test worlds.
tests/worlds/mod.rs (1)
32-32: LGTM: Module declaration follows established pattern.The codec_error module is appropriately positioned alphabetically and follows the same public module pattern as the other test world declarations.
docs/users-guide.md (1)
168-270: LGTM: comprehensive codec error handling documentation.The new section clearly documents the CodecError taxonomy, recovery policies, context metadata, protocol hooks, and metrics integration. Code examples are properly tagged, the table is well-formatted, and the documentation aligns with the implementation described in the PR.
Note: The static analysis hints about em dashes are false positives—the dashes in lines 175-180 are list markers and inline separators, not prose dashes requiring typographical correction.
tests/cucumber.rs (1)
4-4: LGTM: CodecErrorWorld integration follows established patterns.The additions correctly extend the cucumber test suite to include codec error scenarios. Import order, documentation, and runner sequence all follow the existing conventions.
Also applies to: 15-15, 31-31, 44-44, 67-67
docs/adr-004-pluggable-protocol-codecs.md (1)
256-277: LGTM: resolved decision properly documented.The "Resolved Decisions" section clearly captures the CodecError taxonomy design outcome, including error classification, recovery policies, backward compatibility strategy, protocol hooks, and observability integration. The documentation aligns with the implementation and provides appropriate file references.
tests/features/codec_error.feature (1)
1-25: LGTM: clear BDD scenarios for codec error behaviour.The feature file defines four well-structured scenarios covering clean EOF, premature EOF, framing errors, and recovery policy defaults. The Gherkin syntax is correct and the scenarios align with the codec error taxonomy introduced in the PR.
tests/codec_error.rs (1)
1-327: LGTM: comprehensive integration tests for codec error taxonomy.The test suite thoroughly validates the CodecError taxonomy, recovery policies, error context, custom hooks, EOF handling, and integration with WireframeError and app-level error types. The helper function
assert_decode_eof_errorappropriately consolidates EOF testing logic, and all tests follow safe patterns without direct indexing or inappropriate.expect()usage.tests/steps/codec_error_steps.rs (1)
1-4: LGTM!Module-level doc comment and imports are appropriate.
src/response.rs (2)
200-229: LGTM!The new
Codecvariant is well-documented, and the doc comment accurately describes the error categorisation.
260-278: LGTM!
DisplayandErrorimplementations correctly handle the newCodecvariant.src/lib.rs (2)
17-31: LGTM!Re-exports are comprehensive and correctly expose the new codec error taxonomy and recovery infrastructure.
89-89: LGTM!
CODEC_ERRORSmetric correctly re-exported alongside existing metrics.src/codec/error.rs (7)
1-30: LGTM!Excellent module-level documentation explaining the error taxonomy and recovery policies.
32-72: LGTM!
FramingErrorvariants are well-documented with meaningful field names and appropriate#[error]messages.
74-118: LGTM!
ProtocolErrorvariants cover the key semantic violations with clear documentation.
120-155: LGTM!
EofErrorvariants correctly distinguish clean closure from premature disconnection scenarios.
157-192: LGTM!Top-level
CodecErrorenum with#[from]conversions and comprehensive doctests.
194-232: LGTM!Recovery policy defaults are well-documented and the match logic is clear. The table in the doc comment accurately reflects the implementation.
298-310: Verify thatUnexpectedEofis semantically correct forCleanClose.
CleanCloserepresents a graceful closure at a frame boundary, yet it maps toio::ErrorKind::UnexpectedEof. This seems contradictory—callers might interpretUnexpectedEofas an error condition when it is actually normal termination. Verify this is intentional, or use a differentErrorKind(e.g.,ConnectionAbortedorBrokenPipe) to distinguish clean closure from premature EOF.src/hooks.rs (5)
9-12: LGTM!Import correctly added for
EofError.
65-102: LGTM!The
on_eofhook is well-documented with a comprehensive example demonstrating all three EOF variants.
121-122: ExportOnEofHookfor external consumers.Other hook type aliases (
BeforeSendHook,OnConnectionSetupHook, etc.) are private (type), but downstream users implementing custom protocol hooks may benefit from a public type alias forOnEofHook. If external use is intended, addpubvisibility; otherwise, document the intent to keep it private.
136-137: LGTM!Field, default, and dispatcher method for
on_eoffollow the established pattern used by other hooks.Also applies to: 148-148, 186-191
223-236: LGTM!Hook wiring in
from_protocolcorrectly clones the protocolArcand delegates toprotocol_eof.on_eof.src/app/error.rs (3)
7-27: LGTM!The
Codecvariant is correctly added with comprehensive documentation explaining the error categorisation.
29-40: LGTM!Manual
PartialEqandEqimplementations are correctly implemented. The comment on lines 33–34 clearly explains whyCodecErrorcannot be compared for equality.
42-55: LGTM!
SendErrorcorrectly extended with theCodecvariant and#[from]conversion.tests/worlds/codec_error.rs (3)
52-102: LGTM!The setter methods correctly parse string inputs, return structured errors on unknown variants, and trigger
build_errorto keep internal state consistent. Error messages are clear and follow the expected pattern.
104-143: LGTM!
build_errorcleanly maps the configured type/variant combinations to concreteCodecErrorinstances. The use of hardcoded test values (e.g.,size: 2000,max: 1024) is appropriate for a test world.
145-206: LGTM!Verification methods correctly propagate errors via
TestResultand produce informative failure messages. The pattern-matching onOption<EofError>is idiomatic.src/codec.rs (5)
6-19: LGTM!The module documentation clearly explains the error taxonomy and recovery policy semantics, linking to the relevant submodules.
26-48: LGTM!Module declarations, re-exports, and public constants are well-documented. The clamping bounds (
MIN_FRAME_LENGTHandMAX_FRAME_LENGTH) are sensible defaults.
200-210: LGTM!The encoder now emits a structured
CodecError::Framing(FramingError::OversizedFrame { ... })which converts cleanly toio::Errorvia theFromimpl. This aligns with the new taxonomy.
297-322: LGTM!The
assert_decode_eof_errorhelper centralises EOF error assertions, reducing duplication across tests. The closure-based message validator is flexible.
324-375: LGTM!EOF tests cover the key scenarios: clean close returning
Ok(None), partial header, partial payload, and complete frame. Assertions are clear and use the new helper where appropriate.src/codec/recovery.rs (7)
1-36: LGTM!The module documentation is thorough, includes a working code example, and explains the three recovery policies clearly. The doc example uses the correct import paths.
42-91: LGTM!
RecoveryPolicyis well-documented with "When to Use" guidance for each variant. The#[default]attribute onDropis appropriate as it represents the least disruptive recovery action.
93-162: LGTM!
CodecErrorContextprovides a clean builder API with#[must_use]annotations. The fields cover the essential diagnostic context (connection ID, peer address, correlation ID, codec state).
203-226: LGTM!The
RecoveryPolicyHooktrait provides sensible defaults: delegating toCodecError::default_recovery_policy(), a 30-second quarantine duration, and a no-opon_errorhook. The design allows incremental customisation.
228-235: LGTM!
DefaultRecoveryPolicyis a zero-configuration implementation that inherits all trait defaults. The emptyimplblock is idiomatic Rust.
237-307: LGTM!
RecoveryConfiguses the builder pattern consistently with#[must_use]on all setters. The defaults (10 consecutive drops, 30-second quarantine, logging enabled) align with the trait defaults inRecoveryPolicyHook.
309-389: LGTM!Tests cover the key behaviours: default policy is
Drop, builder methods set fields correctly, andDefaultRecoveryPolicydelegates toCodecError::default_recovery_policy(). The use of.expect()in tests follows project guidelines.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/app/error.rssrc/codec.rstests/codec_error.rstests/steps/codec_error_steps.rstests/worlds/codec_error.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
tests/codec_error.rssrc/codec.rssrc/app/error.rstests/steps/codec_error_steps.rstests/worlds/codec_error.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/codec_error.rssrc/codec.rssrc/app/error.rstests/steps/codec_error_steps.rstests/worlds/codec_error.rs
🧠 Learnings (1)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
tests/codec_error.rssrc/codec.rssrc/app/error.rstests/steps/codec_error_steps.rstests/worlds/codec_error.rs
🧬 Code graph analysis (3)
src/codec.rs (2)
src/codec/recovery.rs (1)
new(133-133)src/codec/error.rs (1)
from(299-309)
src/app/error.rs (1)
src/codec/error.rs (1)
from(299-309)
tests/steps/codec_error_steps.rs (1)
src/codec/error.rs (1)
error_type(288-295)
🔍 Remote MCP Deepwiki
Summary of additional repository facts relevant to PR review (from repo docs/wiki):
-
Repo architecture confirms layered error placement: Transport → Framing (codec) → Protocol → Application; codec-layer taxonomy aligns with existing design pattern for layered errors and protocol hooks.
-
Public API and re-exports: crate already exposes many codec/metrics/public types; PR's re-exports (CodecError, RecoveryPolicy, CODEC_ERRORS, MAX_FRAME_LENGTH/MIN_FRAME_LENGTH, CodecErrorContext, RecoveryConfig, RecoveryPolicyHook, etc.) fit existing lib export pattern.
-
Frame/codec behavior & EOF semantics:
- Length-prefixed framing is primary frame processor; existing decode() returns Ok(None) for incomplete frames. PR's LengthDelimitedDecoder::decode_eof() and EofError variants (CleanClose, MidFrame, MidHeader) directly affect that decoder path and must preserve expected Ok(None) for clean closes as per prior behavior.
-
Protocol hooks and connection flow:
- WireframeProtocol already supplies lifecycle hooks (on_connection_setup, before_send, on_command_end, handle_error). PR adds on_eof hook; wiring pattern (ProtocolHooks::from_protocol) in repo shows how to integrate and where to verify hook registration.
-
Observability & metrics conventions:
- Repo uses metrics crate with conditional compilation feature "metrics" and Prometheus exporter; new CODEC_ERRORS counter name wireframe_codec_errors_total and inc_codec_error(error_type, recovery_policy) match existing labeled-counter patterns (e.g., wireframe_frames_processed_total). Ensure labels and feature-flagged no-op variant follow existing pattern.
-
Testing infrastructure & patterns:
- Project uses wireframe_testing helpers, rstest fixtures, and Cucumber BDD world files. PR adds unit/integration tests and a cucumber world (CodecErrorWorld) and feature file; ensure new tests use existing helpers (drive_with_*, logger fixtures) and follow rstest conventions. Duplicate EOF tests noted in PR should be consolidated per project testing patterns (fixtures/helpers preferred).
-
Recovery policy defaults & behavior integration points:
- Repo already applies recovery/behavior via protocol hooks and connection actor; new RecoveryPolicyHook, DefaultRecoveryPolicy, RecoveryConfig should be checked for where connection processing evaluates policies (connection actor error handling paths / WireframeProtocol::handle_error).
Files/areas to review closely (concrete locations suggested by repo docs):
- codec modules and public re-exports: src/codec.rs, src/codec/error.rs, src/codec/recovery.rs, src/lib.rs. Confirm API stability and docs.
- decoder/encoder EOF and oversized-frame changes: LengthDelimitedDecoder/LengthDelimitedEncoder in src/codec.rs — ensure decode_eof behavior preserves clean-close semantics and conversion to io::Error is safe.
- Wireframe error surfaces and conversions: src/response.rs, src/app/error.rs — verify From conversions and Display/Error::source behavior follow existing WireframeError patterns.
- Protocol hooks wiring: src/hooks.rs — validate on_eof hook registration in ProtocolHooks::from_protocol and usage in ConnectionActor/error paths.
- Metrics integration: src/metrics.rs and src/lib.rs exports — ensure conditional compilation and metric naming/labels are consistent with existing metrics conventions.
- Tests and BDD: tests/{codec_error.rs, features/codec_error.feature, worlds/codec_error.rs, steps/codec_error_steps.rs, cucumber.rs} — ensure they use wireframe_testing helpers/fixtures and avoid duplication (extract helper per repo testing style).
Sources:
- Repository wiki/docs and file index (used for above facts and integration patterns): Deepwiki read of leynos/wireframe documentation and API pages.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test
🔇 Additional comments (12)
src/app/error.rs (2)
7-27: LGTM – codec error integration is well-structured.The
Codecvariant with#[from]enables seamless error propagation from the codec layer. Documentation clearly explains the error categories and their origin. The removal ofPartialEq/Eqderives addresses the previous review concern about inconsistent equality semantics.
39-42: Consistent pattern applied toSendError.The
Codecvariant mirrors the pattern used inWireframeError, maintaining API consistency across the error surfaces.src/codec.rs (4)
1-36: Well-documented module with clear error taxonomy.The module-level documentation explains recovery policies concisely. Public re-exports make the error types and recovery infrastructure accessible to downstream consumers.
159-167: Good use of#[expect]for wire endianness.The lint expectation is appropriately scoped and clearly explains why big-endian is intentional for wire protocol compatibility.
215-224: Structured error emission for oversized frames.The encoder now emits
CodecError::Framing(OversizedFrame)instead of a plainio::Error, enabling recovery policy decisions upstream.
312-390: Test helper consolidates EOF assertions effectively.The
assert_decode_eof_errorhelper extracts the common test pattern, improving maintainability and making it straightforward to add new EOF test cases. Test coverage includes clean close, partial header, partial payload, and complete frame scenarios.tests/codec_error.rs (2)
1-14: Comprehensive integration test coverage for the error taxonomy.The tests exercise recovery policy behaviour, error conversions, context builders, and end-to-end integration with
WireframeErrorandSendError. Organisation with section comments aids readability.
138-157: Good demonstration of custom recovery hook override.The
StrictRecoveryexample clearly shows how consumers can implementRecoveryPolicyHookto override default policies.tests/steps/codec_error_steps.rs (1)
1-8: Clear module documentation for cucumber steps.The module comment explains the purpose and approach of these behavioural tests.
tests/worlds/codec_error.rs (3)
73-190: Well-structured test world with clear error handling.The
set_*methods returnTestResultand reject unknown values with descriptive error messages. Thebuild_errormethod correctly materialises all error type/variant combinations.
192-322: Real decoder operations provide end-to-end validation.The E2E methods (
send_complete_frame,decode_eof_clean_close,encode_oversized_frame, etc.) exercise actual codec paths rather than mocking, ensuring the error taxonomy integrates correctly with the decoder implementation.
324-369: Verification methods provide clear test assertions.The
verify_*methods produce descriptive error messages when assertions fail, aiding test debugging.
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @src/codec.rs:
- Around line 200-209: The slice-to-array conversion in the header parsing uses
an unreachable else branch; since bytes_received >= LENGTH_HEADER_SIZE is
already guaranteed, remove the conditional let Ok(...) = ... else { ... } block
and perform the conversion without the unreachable branch (e.g., directly unwrap
the result of <[u8; LENGTH_HEADER_SIZE]>::try_from(header_slice>), keeping
references to header_slice, LENGTH_HEADER_SIZE, and the existing error variant
CodecError::Eof(EofError::MidHeader) only removed in this location.
- Around line 155-167: The match arm handling Err(_) after calling
self.inner.decode_eof(src) discards the original error; preserve its context by
capturing the error (e) and return an EOF error that wraps or chains the
original error instead of throwing it away. Update either the match arm to pass
the captured error into build_eof_error (e.g., change build_eof_error to accept
an Option<io::Error> or &io::Error) or construct a new io::Error that includes
build_eof_error(src) as the message and sets the original error as the source so
diagnostics retain the inner error from self.inner.decode_eof.
In @src/codec/error_tests.rs:
- Around line 73-85: Extend the test
`codec_error_converts_to_io_error_with_correct_kind` to also assert that
`CodecError::Protocol(...)` converts to an `io::Error` with kind
`io::ErrorKind::InvalidData`, and that `CodecError::Io(inner_io_error)` is a
passthrough (i.e., convert a known `io::Error` via `CodecError::Io` to
`io::Error` and assert the resulting `.kind()` matches the original
`inner_io_error.kind()`); use the existing test patterns (creating the variant,
converting with `.into()`, and `assert_eq!(io_err.kind(), ...)`) and reference
the `CodecError::Protocol` and `CodecError::Io` variants when adding these two
assertions.
In @src/metrics.rs:
- Around line 141-152: The two feature-gated implementations of inc_codec_error
use mismatched parameter types; change the non-metrics (no-op) signature to
match the metrics-enabled one by accepting &'static str for both parameters
(error_type and recovery_policy) so the function signature is identical across
cfg branches; update the function definition for inc_codec_error in the
#[cfg(not(feature = "metrics"))] block to use &'static str for both parameters
to unify the API.
In @tests/worlds/codec_error.rs:
- Around line 196-210: setup_default_codec duplicates the reset logic in
setup_codec_with_max_length; change setup_default_codec to delegate to
setup_codec_with_max_length(1024) (i.e., call
self.setup_codec_with_max_length(1024)) and remove the duplicated assignments,
or alternately extract a private helper like reset_codec(&mut self) that sets
max_frame_length, buffer, decoder_error, and clean_close_detected and call it
from both setup_default_codec and setup_codec_with_max_length; update references
to the functions setup_default_codec and setup_codec_with_max_length
accordingly.
- Around line 80-89: The set_error_type method sets self.error_type but doesn’t
call build_error(), causing current_error to be stale; after assigning
self.error_type in set_error_type, call self.build_error() (like
set_framing_variant and set_eof_variant do) so current_error is rebuilt
immediately—update the pub fn set_error_type(&mut self, error_type: &str) ->
TestResult to invoke self.build_error() before returning Ok(()).
- Around line 57-71: The struct CodecErrorWorld has inconsistent documentation:
add doc comments for the fields error_type, framing_variant, eof_variant,
current_error, and detected_eof to match the existing style used for
max_frame_length, buffer, decoder_error, and clean_close_detected; update the
CodecErrorWorld declaration so each field (error_type, framing_variant,
eof_variant, current_error, detected_eof) has a brief /// comment describing its
purpose and type to maintain consistency across the struct.
- Around line 264-281: The classify_eof_error method currently infers header vs
frame EOF by string-matching the error text and sets EofError::MidFrame {
expected: 0 }, which is fragile and discards the true expected size; update
classify_eof_error (and the EofError usage) by adding a clear comment above the
function noting this fragility and the dependency on upstream error text, and
either (preferred) compute and set a meaningful expected value for
EofError::MidFrame using available context (e.g., parse the partial header from
self.buffer or read any stored expected frame length), or (if that data isn't
available today) change the MidFrame expected field to an Option and set it to
None so callers know the value is unknown instead of 0; ensure references to
detected_eof and EofError::MidHeader/EofError::MidFrame are updated accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mdsrc/codec.rssrc/codec/error.rssrc/codec/error_tests.rssrc/metrics.rssrc/response.rstests/steps/codec_error_steps.rstests/worlds/codec_error.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
src/metrics.rssrc/codec/error.rssrc/response.rssrc/codec/error_tests.rssrc/codec.rstests/worlds/codec_error.rstests/steps/codec_error_steps.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/metrics.rssrc/codec/error.rssrc/response.rssrc/codec/error_tests.rssrc/codec.rstests/worlds/codec_error.rstests/steps/codec_error_steps.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/contents.md)
Follow the documentation style guide conventions when writing project documentation
Write official documentation with separate guides for 'Duplex Messaging & Pushes', 'Streaming Responses', and 'Message Fragmentation', each including runnable examples and explaining relevant concepts and APIs
docs/**/*.md: Use markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ('-ize' / '-yse' / '-our'), with the exception of the filenameLICENSE.
Reference the documentation style guide provided atdocs/documentation-style-guide.mdfor documentation writing standards.Documentation describing multi-packet streaming MUST reflect the tuple-based API and explain helper constructors that prepare the channel pair
Replace or retire documentation that refers to Cucumber, including guides and examples in docs/
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization', suffix -lyse in words like 'analyse' and 'paralyse', suffix -our in words like 'colour' and 'behaviour', suffix -re in words like 'calibre' and 'centre', double 'l' in words like 'cancelled' and 'counsellor', maintain the 'e' in words like 'likeable' and 'liveable', suffix -ogue in words like 'analogue' and 'catalogue'
The word 'outwith' is acceptable in documentation
Keep US spelling when used in an API, for example 'color'
Use the Oxford comma: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Lille Industries are expanding'
Write hea...
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
Every doctest should validate the public API of a crate from the perspective of an external user, treating each documentation test as a separate temporary crate that imports the library as an external dependency
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes in Markdown.
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
🧠 Learnings (2)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
src/metrics.rssrc/codec/error.rssrc/response.rssrc/codec/error_tests.rssrc/codec.rstests/worlds/codec_error.rstests/steps/codec_error_steps.rs
📚 Learning: 2026-01-06T18:51:34.645Z
Learnt from: CR
Repo: leynos/wireframe PR: 0
File: docs/roadmap.md:0-0
Timestamp: 2026-01-06T18:51:34.645Z
Learning: Applies to docs/src/**/*.rs : Introduce a `CodecError` taxonomy separating framing, protocol, and IO failures
Applied to files:
src/codec/error.rsdocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
🧬 Code graph analysis (5)
src/metrics.rs (3)
src/codec/error.rs (1)
error_type(288-295)tests/codec_error.rs (1)
recovery_policy(141-143)src/codec/recovery.rs (1)
recovery_policy(208-211)
src/response.rs (1)
src/codec/error.rs (1)
is_clean_close(254-254)
src/codec/error_tests.rs (2)
src/codec/error.rs (4)
default_recovery_policy(224-232)should_disconnect(278-280)is_clean_close(254-254)error_type(288-295)src/response.rs (1)
is_clean_close(251-253)
tests/worlds/codec_error.rs (4)
src/frame/format.rs (1)
bytes(54-54)src/codec.rs (4)
default(120-124)new(102-106)decoder(70-70)decoder(244-248)src/codec/recovery.rs (2)
default(277-283)new(133-133)src/codec/error.rs (2)
error_type(288-295)from(299-309)
tests/steps/codec_error_steps.rs (1)
src/codec/error.rs (1)
error_type(288-295)
🪛 GitHub Actions: CI
src/codec.rs
[error] 142-146: Formatting check failed: cargo fmt --check detected formatting issues in src/codec.rs (diff reported around the expect attribute). Run 'cargo fmt --all' or 'cargo fmt --all -- --check' to fix.
🪛 LanguageTool
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
[typographical] ~94-~94: To join two clauses or introduce examples, consider using an em dash.
Context: ... Codec implementation: - src/codec.rs - FrameCodec trait and `LengthDelimitedF...
(DASH_RULE)
[typographical] ~95-~95: To join two clauses or introduce examples, consider using an em dash.
Context: ...tedFrameCodec-src/codec/examples.rs- Example codec implementations -src/app...
(DASH_RULE)
[typographical] ~96-~96: To join two clauses or introduce examples, consider using an em dash.
Context: ...mentations - src/app/combined_codec.rs - Adapter for Tokio Framed Error handli...
(DASH_RULE)
[typographical] ~100-~100: To join two clauses or introduce examples, consider using an em dash.
Context: ...ndling paths: - src/app/connection.rs - Connection processing and error handling...
(DASH_RULE)
[typographical] ~101-~101: To join two clauses or introduce examples, consider using an em dash.
Context: ...r handling - src/app/frame_handling.rs - Frame deserialization and response forwa...
(DASH_RULE)
[typographical] ~102-~102: To join two clauses or introduce examples, consider using an em dash.
Context: ...and response forwarding - src/hooks.rs - Protocol hooks including error callbacks...
(DASH_RULE)
[typographical] ~106-~106: To join two clauses or introduce examples, consider using an em dash.
Context: ...nfrastructure: - tests/frame_codec.rs - Codec integration tests - `tests/feature...
(DASH_RULE)
[typographical] ~107-~107: To join two clauses or introduce examples, consider using an em dash.
Context: ...ec integration tests - tests/features/ - Cucumber feature files - tests/steps/ ...
(DASH_RULE)
[typographical] ~108-~108: To join two clauses or introduce examples, consider using an em dash.
Context: ...les - tests/steps/ and tests/worlds/ - Step definitions and world types Docume...
(DASH_RULE)
[typographical] ~112-~112: To join two clauses or introduce examples, consider using an em dash.
Context: ...entation to update: - docs/roadmap.md - Mark 9.1.2 done - docs/users-guide.md ...
(DASH_RULE)
[typographical] ~113-~113: To join two clauses or introduce examples, consider using an em dash.
Context: ... Mark 9.1.2 done - docs/users-guide.md - Public interface documentation - `docs/a...
(DASH_RULE)
[typographical] ~114-~114: To join two clauses or introduce examples, consider using an em dash.
Context: ...cs/adr-004-pluggable-protocol-codecs.md` - Design decisions ## Plan of Work ### P...
(DASH_RULE)
[typographical] ~123-~123: To join two clauses or introduce examples, consider using an em dash.
Context: ...r classification: 1. Framing errors - Wire-level frame boundary issues (e.g., ...
(DASH_RULE)
[typographical] ~125-~125: To join two clauses or introduce examples, consider using an em dash.
Context: ...hecksum mismatch) 2. Protocol errors - Semantic violations after frame extracti...
(DASH_RULE)
[typographical] ~128-~128: To join two clauses or introduce examples, consider using an em dash.
Context: ... sequence violation) 3. IO errors - Transport layer failures (wrapped `io::E...
(DASH_RULE)
[typographical] ~129-~129: To join two clauses or introduce examples, consider using an em dash.
Context: ... (wrapped io::Error) 4. EOF errors - End-of-stream handling with three sub-va...
(DASH_RULE)
[typographical] ~136-~136: To join two clauses or introduce examples, consider using an em dash.
Context: ...cy()method returning one of: -Drop` - Discard the malformed frame and continue...
(DASH_RULE)
[typographical] ~137-~137: To join two clauses or introduce examples, consider using an em dash.
Context: ...inue (recoverable errors) - Quarantine - Pause the connection temporarily (rate l...
(DASH_RULE)
[typographical] ~138-~138: To join two clauses or introduce examples, consider using an em dash.
Context: ...mporarily (rate limiting) - Disconnect - Terminate the connection (fatal errors) ...
(DASH_RULE)
[typographical] ~171-~171: Two consecutive dots
Context: ...e)for EOF at frame boundary 3. ReturnCodecError::Eof(EofError::MidFrame{..})for EOF during payload 4. ReturnCo...
(DOUBLE_PUNCTUATION)
[typographical] ~172-~172: Two consecutive dots
Context: ...{..})for EOF during payload 4. ReturnCodecError::Eof(EofError::MidHeader{..})for EOF during header 5. Maintaini...
(DOUBLE_PUNCTUATION)
[uncategorized] ~232-~232: Loose punctuation mark.
Context: ...documentation: 1. docs/users-guide.md: - Add section on codec error handlin...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~238-~238: Loose punctuation mark.
Context: ...cs/adr-004-pluggable-protocol-codecs.md: - Document the CodecError` taxonomy...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~243-~243: Loose punctuation mark.
Context: ...OF handling design 3. docs/roadmap.md: - Mark all 9.1.2 sub-items as done ...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~255-~255: Possible typo: you repeated a word.
Context: ...CodecError::default_recovery_policy() implementation - From<CodecError> for io::Error implementation 2. Create src/codec/recovery.rs with...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~260-~260: Using four (or more) nouns in a row may decrease readability.
Context: ...ryPolicyenum -CodecErrorContextstruct -RecoveryPolicyHooktrait -DefaultRecoveryPolicyimplementation -RecoveryConfigbuilder config 3. Updatesrc/codec.rs`: - ...
(FOUR_NN)
🔍 Remote MCP Deepwiki
Summary of additional repository facts relevant to this PR (concise, review-focused):
-
Layered architecture & expected placement: repo uses Transport → Framing (codec) → Protocol → Application layering; codec-level errors and protocol hooks belong in the codec/module and WireframeProtocol hook surfaces (so adding CodecError, EofError, and on_eof is consistent).
-
Length-prefixed framing expectations: the project uses a LengthPrefixedProcessor that returns Ok(None) for incomplete frames; decode_eof must preserve prior semantics for clean close vs incomplete-frame detection (i.e., produce clean-close Ok(None) behavior where appropriate) — review LengthDelimitedDecoder changes carefully for backward compatibility.
-
Protocol hook wiring pattern: ProtocolHooks::from_protocol constructs boxed hooks (existing pattern used for before_send/on_command_end/etc.); adding on_eof must follow same registration/wiring pattern and be invoked by connection actor where EOF is detected.
-
Observability & metrics conventions: metrics are feature-gated via the "metrics" feature and use labeled Prometheus-style counters exported through metrics crate; new CODEC_ERRORS counter and inc_codec_error must match existing naming/label patterns and no-op when feature disabled.
-
Connection actor semantics & EOF handling location: ConnectionActor owns per-connection read/write loops and invokes protocol hooks; EOF detection/recovery/policy evaluation should be integrated where the connection read/decode_eof path runs (ensure recovery policies are consulted before terminating or quarantine actions).
-
Testing infra / preferred patterns: project favors rstest fixtures, wireframe_testing helpers (drive_with_bincode, drive_with_frame), and cucumber BDD worlds. Duplicate EOF tests should be consolidated into helper(s) per existing test utility patterns (wireframe_testing provides helpers and logger fixtures).
-
Session/registry & quarantine implications: RecoveryPolicy::Quarantine semantics (if used) will likely interact with SessionRegistry/PushHandle lifecycles — verify quarantine does not accidentally leak PushHandles/weak refs or prevent expected cleanup.
Files/areas to inspect closely in review (concrete targets):
- src/codec/{error.rs,recovery.rs} and src/codec.rs — public types, From for io::Error, MIN/MAX_FRAME_LENGTH re-export behavior
- LengthDelimitedDecoder/Encoder implementations in src/codec.rs — decode_eof behavior and encoder oversized-frame handling
- src/hooks.rs and ProtocolHooks::from_protocol — ensure on_eof hook is constructed/registered like other hooks
- Connection read/actor paths where EOF/errors are handled (ConnectionActor) — ensure recovery policy evaluation and on_eof invocation are placed correctly
- src/metrics.rs and src/lib.rs exports — feature gating and label names for CODEC_ERRORS and inc_codec_error consistency
- Tests and BDD additions (tests/, wireframe_testing helpers) — consolidate duplicated EOF tests using existing test helpers and follow rstest/drive_with_ patterns
Sources used: Deepwiki documentation pages for the leynos/wireframe repository: Overview, Core Architecture, Frame processing, Protocol hooks, Observability/metrics, ConnectionActor, and Testing pages.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (28)
src/metrics.rs (1)
36-44: LGTM!The
CODEC_ERRORSconstant follows established naming conventions and includes clear documentation with example Prometheus output.src/codec/error_tests.rs (1)
1-7: LGTM!Well-structured test module with comprehensive coverage of recovery policies, error conversions, categorisation, and Display implementations.
src/response.rs (2)
224-253: LGTM!The
Codecvariant integration is clean:from_codecmirrors the existingfrom_iopattern,is_clean_closecorrectly delegates toCodecError::is_clean_close(), and documentation references the detailed type.
256-277: LGTM!The
DisplayandErrortrait implementations for theCodecvariant are consistent with the existingIoandProtocolpatterns.src/codec/error.rs (6)
1-31: LGTM!Excellent module-level documentation explaining the error taxonomy, categories, and recovery policies with clear structure.
32-72: LGTM!The
FramingErrorenum provides comprehensive wire-level error coverage with clear, actionable messages and appropriate structured fields.
74-118: LGTM!The
ProtocolErrorenum covers semantic violations comprehensively. The use ofStringfields forMissingHeaderandInvalidStateTransitionprovides flexibility for protocol-specific contexts.
120-155: LGTM!The
EofErrorenum cleanly distinguishes EOF scenarios with appropriate byte count fields for diagnostics. TheCopyderive is efficient for these small variants.
194-296: LGTM!The
CodecErrormethods are well-implemented:default_recovery_policylogic matches the documented table,is_clean_closeis concise, anderror_typereturns consistent static strings for metrics.
298-310: LGTM!The
From<CodecError> for io::Errorimplementation correctly maps error categories to appropriateErrorKindvalues while preserving diagnostic context.src/codec.rs (3)
324-402: LGTM!The
assert_decode_eof_errorhelper consolidates test logic effectively. Test coverage for empty buffer (clean close), partial header, partial payload, and complete frame scenarios is comprehensive.
227-234: LGTM!The encoder correctly emits
CodecError::Framing(FramingError::OversizedFrame)with structured context for oversized payloads.
143-148: The lint suppression is valid and follows guidelines. No formatting issues exist.The code at lines 143–148 correctly uses a narrowly scoped
#[expect(lint, reason = "...")](not#[allow]) as required. The reason provided clearly explains the semantic distinction:Ok(None)andErr(_)represent different failure paths within the decoder's EOF handling, even though both arms returnErr(build_eof_error(src)). This justifies suppressing thematch_same_armswarning.docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md (3)
29-39: Update the progress checklist.Several items remain unchecked but may be partially or fully implemented based on the PR changes:
- Line 29:
CodecErrorContextpopulation - verify status- Line 30: Recovery policy evaluation in
src/app/connection.rs- verify status- Line 31: Structured logging - verify status
- Line 32:
on_eofhook toProtocolHooks- verify status- Lines 37-39: Documentation updates - verify status
Mark completed items as done to keep this living document accurate.
142-143: LGTM!The spelling "customization" at line 143 follows en-GB-oxendict conventions correctly.
1-68: LGTM!The execution plan is well-structured with clear purpose, progress tracking, decision log, and concrete steps. The living document format supports iterative development.
tests/steps/codec_error_steps.rs (3)
22-40: LGTM!The
given_error_type_variantstep correctly handles unknown error types with an explicit error at line 37, and the#[expect]attribute properly documents the cucumber macro requirement.
46-75: LGTM!The
whensteps clearly delegate to world methods with inline comments explaining the intent. Themax + 1at line 74 correctly ensures the frame exceeds the configured limit.
81-101: LGTM!The
thensteps cleanly delegate verification to world methods. The string handling at line 100 correctly passes&policywithout unnecessary allocations.tests/worlds/codec_error.rs (9)
1-24: LGTM!Module documentation is present and imports are appropriately scoped.
25-53: LGTM!Enum definitions are clean with appropriate derives and defaults.
125-164: LGTM!Error construction logic is comprehensive and covers all variant combinations.
166-190: LGTM!Policy verification with clear error messages for test diagnostics.
212-231: LGTM!Frame sending helpers are clean with appropriate error handling.
240-262: LGTM!EOF clean close detection is well-structured with appropriate state updates.
283-301: LGTM!Partial data EOF detection correctly expects and captures the error.
303-322: LGTM!Oversized frame encoding test helper is straightforward.
324-369: LGTM!Verification helpers provide clear diagnostics on assertion failures.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/codec.rs (1)
1-403: Decompose the file to meet the 400-line limit.The file contains 403 lines, exceeding the 400-line limit specified in the coding guidelines. Move the test module (lines 264-403) to a separate file such as
src/codec/tests.rsand conditionally compile it with#[cfg(test)].♻️ Proposed decomposition
In
src/codec.rs, replace the tests module:-#[cfg(test)] -mod tests { - // ... all test code ... -} +#[cfg(test)] +mod tests;Create
src/codec/tests.rs:use super::*; use bytes::{Bytes, BytesMut}; use std::io; #[test] fn length_delimited_codec_clamps_max_frame_length() { // ... move all tests here ... } // ... rest of tests ...As per coding guidelines, files must not exceed 400 lines; large modules must be decomposed.
🤖 Fix all issues with AI agents
In @src/codec.rs:
- Around line 196-209: The match arm using let Ok(header) = <[u8;
LENGTH_HEADER_SIZE]>::try_from(header_slice) else { ... } is dead code because
bytes_received >= LENGTH_HEADER_SIZE guarantees the slice is exactly
LENGTH_HEADER_SIZE; remove the unreachable else branch and replace that fallible
pattern with an infallible conversion (e.g., call try_from(...) and immediately
unwrap/expect) while keeping the variable name header and the same header_slice
source, then proceed with the existing u32_from_be_bytes call and the existing
CodecError::Eof(...) handling elsewhere.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/codec.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
src/codec.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/codec.rs
🧠 Learnings (1)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
src/codec.rs
🧬 Code graph analysis (1)
src/codec.rs (2)
src/codec/recovery.rs (1)
new(133-133)src/codec/error.rs (1)
from(299-309)
🔍 Remote MCP Deepwiki
Summary of additional repository facts relevant to reviewing this PR (concise, review-focused):
-
Overall architecture: actor-per-connection model with ConnectionActor owning read/write loops, framed by FrameProcessor (LengthPrefixedProcessor). Codec/Eof changes must preserve ConnectionActor semantics and framing expectations (Ok(None) for incomplete frames).
-
Frame/decoder expectations: LengthPrefixedProcessor.decode() returns Ok(None) when a frame is incomplete; decode_eof must distinguish clean-close vs mid-frame per existing framing semantics — verify LengthDelimitedDecoder changes maintain backward compatibility for complete-frame clean closes vs structured EOF errors for partial data.
-
Connection lifecycle and hooks: ConnectionActor wires ProtocolHooks and calls protocol lifecycle hooks (on_connection_setup, before_send, on_command_end). New on_eof must be registered in ProtocolHooks::from_protocol and invoked where EOF is detected in the connection read/decode path (connection actor). Ensure hook wiring matches existing hook registration patterns.
-
Observability/metrics conventions: metrics are feature-gated; counters and gauge naming follow existing patterns (prometheus-style labels). CODEC_ERRORS and inc_codec_error must follow existing conditional compilation/no-op pattern when metrics feature disabled and match naming/label conventions used elsewhere (wireframe_metrics exports).
-
Session/quarantine interactions: RecoveryPolicy::Quarantine may interact with SessionRegistry / PushHandle lifecycles; verify quarantine handling doesn't leak PushHandles or prevent connection cleanup via Weak/Arc patterns used by SessionRegistry.
-
Testing infra and duplication guidance: repository uses wireframe_testing helpers (drive_with_bincode, drive_with_frame) and rstest fixtures; preferred approach is extracting shared assertions into helpers. The PR comment about consolidating duplicate decode_eof tests aligns with existing testing patterns — implement a helper that uses wireframe_testing utilities and preserves test names/#[test]/rstest attributes.
Files/areas to inspect closely in review (concrete targets):
- src/codec/{error.rs,recovery.rs} and src/codec.rs — public types, From for io::Error, MIN/MAX_FRAME_LENGTH re-exports.
- LengthDelimitedDecoder/Encoder changes — ensure decode_eof preserves Ok(None)/clean-close semantics and encoder oversized-frame behavior.
- src/hooks.rs / ProtocolHooks::from_protocol — confirm on_eof hook constructed/registered analogously to other hooks.
- Connection read/actor code (ConnectionActor) — ensure on_eof invocation and recovery policy evaluation occur at the correct point and do not break shutdown/fairness behavior.
- src/metrics.rs and src/lib.rs exports — confirm feature-gating, label names, and export consistency with existing metrics.
- Tests and Cucumber additions — replace duplicated EOF tests by central helper using wireframe_testing functions to follow established test utilities and fixtures; keep test discoverability and names.
Sources used: Deepwiki repository documentation pages and code overview.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test
🔇 Additional comments (6)
src/codec.rs (6)
1-19: Clear documentation of the new error taxonomy.The module-level documentation appropriately describes the structured error handling and recovery policies introduced in this PR. The cross-references to the
errorandrecoverymodules help users navigate the new API surface.
26-36: Public API re-exports follow Rust conventions.The new error and recovery module exports provide a clean, discoverable API surface for the structured error taxonomy.
38-48: Frame length constants are well-documented.The public constants provide clear bounds for frame sizes with appropriate documentation explaining their purpose.
143-168: EOF handling correctly distinguishes clean closes from partial data.The decode_eof implementation properly handles:
- Clean closes (empty buffer) returning Ok(None)
- Complete frames at EOF
- Partial data synthesised into structured EOF errors
- Preservation of InvalidData errors (oversized frames) for correct recovery policy application
The
#[expect]forclippy::match_same_armsis appropriately justified.
171-179: Endianness helper is correctly implemented.The inline helper with explicit endianness handling and appropriately scoped lint suppression is clear and correct.
227-234: Oversized frame detection correctly uses structured errors.The encoder properly emits
CodecError::Framing(FramingError::OversizedFrame)for frames exceeding the limit, which will be converted toio::ErrorwithInvalidDatakind per theFromimplementation in src/codec/error.rs.
leynos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/codec/tests.rs:
- Around line 64-121: Remove the assert_decode_eof_error helper and the two
tests decode_eof_with_partial_header_returns_mid_header and
decode_eof_with_partial_payload_returns_mid_frame, add use rstest::rstest; to
the test module, and replace them with a single #[rstest] parameterized test
function decode_eof_with_incomplete_data_returns_error that takes a buffer case
and an expected_substr case; inside the test create a
LengthDelimitedFrameCodec::new(128), call codec.decoder(), initialize a BytesMut
from the buffer, call decoder.decode_eof(&mut buf).expect_err(...), assert
err.kind() == io::ErrorKind::UnexpectedEof and assert
err.to_string().contains(expected_substr) (use "header" for the partial-header
case and "4" for the partial-payload case).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/codec.rssrc/codec/tests.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
src/codec/tests.rssrc/codec.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/codec/tests.rssrc/codec.rs
🧠 Learnings (1)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
src/codec/tests.rssrc/codec.rs
🧬 Code graph analysis (2)
src/codec/tests.rs (3)
src/frame/format.rs (1)
bytes(54-54)src/codec.rs (10)
new(102-106)max_frame_length(87-87)max_frame_length(110-110)max_frame_length(261-261)encoder(73-73)encoder(250-255)decoder(70-70)decoder(244-248)frame_payload(76-76)frame_payload(257-257)src/extractor.rs (1)
as_ref(198-198)
src/codec.rs (6)
src/frame/format.rs (1)
bytes(54-54)tests/wireframe_protocol.rs (1)
decode_eof(91-95)tests/worlds/codec_stateful.rs (1)
decode_eof(109-111)src/app/combined_codec.rs (1)
decode_eof(31-33)src/fragment/index.rs (1)
try_from(48-48)tests/worlds/correlation.rs (1)
expected(45-45)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test
There was a problem hiding this comment.
This issue has been resolved in commit e9a4326.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/codec.rs:
- Around line 182-217: The build_eof_error function uses direct slicing
(&src[..LENGTH_HEADER_SIZE]) with an #[expect] suppression; change this to use
the safe accessor src.get(..LENGTH_HEADER_SIZE) and handle the None case by
returning the same CodecError::Eof(EofError::MidHeader { bytes_received,
header_size: LENGTH_HEADER_SIZE }) . If get(...) returns Some(slice) then
convert to an array with try_from(slice) as before and proceed to compute
expected via u32_from_be_bytes(header) and return the MidFrame error; remove the
#[expect] and any indexing-based assumptions so there is no potential panic.
In @src/codec/tests.rs:
- Around line 1-6: Add a module-level doc comment (starting with //! ) at the
very top of this test module describing its purpose and what it tests (e.g.,
that it contains unit/integration tests for the codec functions using
Bytes/BytesMut and rstest); place this comment above the existing use statements
and ensure it briefly explains the module's intent and any test utilities or
conventions used so it satisfies the coding guideline for module documentation.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/codec.rssrc/codec/error_tests.rssrc/codec/tests.rssrc/metrics.rstests/worlds/codec_error.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
src/codec/error_tests.rssrc/codec/tests.rssrc/metrics.rssrc/codec.rstests/worlds/codec_error.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
src/codec/error_tests.rssrc/codec/tests.rssrc/metrics.rssrc/codec.rstests/worlds/codec_error.rs
🧠 Learnings (1)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
src/codec/error_tests.rssrc/codec/tests.rssrc/metrics.rssrc/codec.rstests/worlds/codec_error.rs
🧬 Code graph analysis (5)
src/codec/error_tests.rs (2)
src/codec/error.rs (4)
default_recovery_policy(224-232)should_disconnect(278-280)is_clean_close(254-254)error_type(288-295)src/response.rs (1)
is_clean_close(251-253)
src/codec/tests.rs (1)
src/codec.rs (10)
new(102-106)max_frame_length(87-87)max_frame_length(110-110)max_frame_length(262-262)encoder(73-73)encoder(251-256)decoder(70-70)decoder(245-249)frame_payload(76-76)frame_payload(258-258)
src/metrics.rs (3)
src/codec/error.rs (1)
error_type(288-295)src/codec/recovery.rs (1)
recovery_policy(208-211)tests/codec_error.rs (1)
recovery_policy(141-143)
src/codec.rs (6)
src/frame/format.rs (1)
bytes(54-54)tests/wireframe_protocol.rs (1)
decode_eof(91-95)tests/worlds/codec_stateful.rs (1)
decode_eof(109-111)src/app/combined_codec.rs (1)
decode_eof(31-33)src/fragment/index.rs (1)
try_from(48-48)tests/worlds/correlation.rs (1)
expected(45-45)
tests/worlds/codec_error.rs (3)
src/codec.rs (6)
default(120-124)new(102-106)encoder(73-73)encoder(251-256)decoder(70-70)decoder(245-249)src/codec/recovery.rs (2)
default(277-283)new(133-133)src/codec/error.rs (2)
error_type(288-295)from(299-309)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: build-test
🔇 Additional comments (7)
src/metrics.rs (1)
36-152: LGTM!The metrics integration for codec errors is well-implemented. The function signature aligns with the error taxonomy (error_type() returns &'static str), documentation includes clear Prometheus examples, and the feature-gating pattern is consistent with existing metrics functions.
src/codec.rs (3)
1-52: LGTM!The module documentation clearly explains the error taxonomy and recovery policies. Public exports are well-organised, and the frame length constants are properly documented with clamping behaviour explained.
143-169: LGTM!The decode_eof implementation correctly differentiates clean close, complete frames, and EOF error conditions. The logic preserves InvalidData framing errors (like oversized frames) to maintain correct recovery policies, whilst synthesising structured EOF errors for incomplete data. The #[expect] for match_same_arms is well-justified.
219-266: LGTM!The encoder correctly emits structured
CodecError::Framingfor oversized frames, replacing the previous generic io::Error approach. The test module organisation maintains the file within the 400-line limit.src/codec/error_tests.rs (1)
1-144: LGTM!The unit tests comprehensively cover error taxonomy, recovery policies, io::Error conversions, and Display implementations. Each test is focused on a distinct aspect of the error types with clear assertions. The test structure is well-organised and maintainable.
src/codec/tests.rs (1)
79-123: LGTM!The parameterised EOF error tests using
#[rstest]eliminate code duplication whilst maintaining clear test case documentation. The test cases cover partial header and partial payload scenarios with precise assertions on error kind and message content. The complete frame test provides good complementary coverage.tests/worlds/codec_error.rs (1)
1-390: LGTM!The test world provides comprehensive BDD scaffolding for codec error scenarios with well-structured public APIs and end-to-end decoder operations. The classify_eof_error method's fragility is appropriately documented (lines 275-284), noting the string-matching approach is acceptable for test code where error messages are controlled. The implementation correctly balances pragmatism with clear documentation of trade-offs.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/worlds/mod.rs (1)
1-7: Update the module doc comment to include codec_error.The module-level documentation lists several world types but omits the newly added
codec_errorworld. Add it to the list for completeness.📝 Proposed fix
//! Cucumber test world implementations and shared helpers. //! //! Provides world types for behaviour-driven tests covering fragmentation, //! correlation, panic recovery, stream termination, multi-packet channels, -//! stateful codecs, request parts, and message assembler parsing. Shared +//! stateful codecs, request parts, message assembler parsing, and codec error +//! taxonomy. Shared //! utilities like `build_small_queues` keep individual worlds focused on their //! respective scenarios.
🤖 Fix all issues with AI agents
In @src/codec.rs:
- Around line 182-209: Add a doc comment to build_eof_error that explains the
EOF variants: when fewer than LENGTH_HEADER_SIZE bytes have been read it returns
CodecError::Eof(EofError::MidHeader { .. }) because the length prefix is
incomplete, and when at least LENGTH_HEADER_SIZE bytes exist but the payload is
truncated it returns CodecError::Eof(EofError::MidFrame { bytes_received:
src.len() - LENGTH_HEADER_SIZE, expected }) indicating a complete header but
incomplete frame; reference LENGTH_HEADER_SIZE, MidHeader, MidFrame, and
build_eof_error in the comment.
In @tests/steps/codec_error_steps.rs:
- Around line 58-62: Change the when step function when_client_sends_partial to
return TestResult rather than unit to match other steps; call
world.send_partial_frame_header_only() as before and return Ok(()) at the end so
the signature and return type are consistent and future failures can be
propagated. Ensure you update the function signature for
when_client_sends_partial and its usage of
CodecErrorWorld::send_partial_frame_header_only to reflect the TestResult
return.
In @tests/worlds/codec_error/decoder_ops.rs:
- Around line 87-99: In extract_expected_length(), replace the hard-coded array
type <[u8; 4]> with the constant-sized array using LENGTH_HEADER_SIZE (i.e., use
<[u8; LENGTH_HEADER_SIZE]>::try_from(slice).ok()) so the array length matches
the slice range and remains consistent; keep the rest of the map_or logic
unchanged to convert the big-endian bytes to a usize via u32::from_be_bytes.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/codec.rstests/steps/codec_error_steps.rstests/worlds/codec_error/decoder_ops.rstests/worlds/codec_error/mod.rstests/worlds/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
tests/steps/codec_error_steps.rstests/worlds/mod.rstests/worlds/codec_error/mod.rstests/worlds/codec_error/decoder_ops.rssrc/codec.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/steps/codec_error_steps.rstests/worlds/mod.rstests/worlds/codec_error/mod.rstests/worlds/codec_error/decoder_ops.rssrc/codec.rs
🧠 Learnings (2)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
tests/steps/codec_error_steps.rstests/worlds/mod.rstests/worlds/codec_error/mod.rstests/worlds/codec_error/decoder_ops.rssrc/codec.rs
📚 Learning: 2026-01-08T08:16:31.989Z
Learnt from: leynos
Repo: leynos/wireframe PR: 411
File: src/client/tests/lifecycle.rs:0-0
Timestamp: 2026-01-08T08:16:31.989Z
Learning: In Rust, apply #[expect(lint, reason = "...")] only to code that actually triggers the specified lint. If the code does not trigger the lint, Clippy will fail with 'unfulfilled lint expectation' errors, which is intentional to prevent stale suppressions. Do not add #[expect] attributes proactively or to code that doesn't trigger the lint, even if similar code elsewhere requires suppression. This guideline should be applied broadly to Rust source files across the repository.
Applied to files:
tests/steps/codec_error_steps.rstests/worlds/mod.rstests/worlds/codec_error/mod.rstests/worlds/codec_error/decoder_ops.rssrc/codec.rs
🧬 Code graph analysis (4)
tests/steps/codec_error_steps.rs (1)
src/codec/error.rs (1)
error_type(288-295)
tests/worlds/codec_error/mod.rs (2)
src/codec.rs (2)
default(120-124)build_eof_error(182-209)src/codec/error.rs (1)
error_type(288-295)
tests/worlds/codec_error/decoder_ops.rs (1)
src/codec.rs (5)
new(102-106)encoder(73-73)encoder(243-248)decoder(70-70)decoder(237-241)
src/codec.rs (4)
src/frame/format.rs (1)
bytes(54-54)tests/worlds/codec_error/mod.rs (1)
build_eof_error(152-164)tests/worlds/correlation.rs (1)
expected(45-45)src/fragment/index.rs (1)
try_from(48-48)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (25)
tests/worlds/mod.rs (1)
32-33: LGTM!The module declaration correctly follows the established pattern for path-based modules in this file.
tests/steps/codec_error_steps.rs (4)
1-8: LGTM!Clear module documentation and appropriate imports for cucumber step definitions.
14-20: LGTM!Setup steps are appropriately simple and delegate to the world methods.
22-40: LGTM!The lint expectation for
needless_pass_by_valueis correctly scoped and justified. The match arm handling for unknown error types provides clear error messages.
46-56: LGTM!The remaining steps correctly delegate to world methods and return
TestResultwhere appropriate. The lint expectations are properly scoped with clear reasons.Also applies to: 64-75, 81-101
tests/worlds/codec_error/mod.rs (6)
1-14: LGTM!Clear module documentation with appropriate imports. The
#[cfg(not(loom))]attribute correctly excludes this test world from loom testing.
16-44: LGTM!The enums are well-structured with sensible defaults. The variants align with the
FramingErrorandEofErrortypes from the codec module.
46-67: LGTM!The world struct appropriately uses
pub(crate)visibility for fields accessed by the siblingdecoder_opsmodule, whilst keepingerror_type,framing_variant, andeof_variantprivate to this module.
69-104: LGTM!The setter methods follow a consistent pattern: parse the string, update state, rebuild the error. Error messages are clear and actionable.
106-164: LGTM!The builder methods correctly construct error variants with representative test data. The helper methods are appropriately private.
166-191: LGTM!The verification method correctly maps string policy names to
RecoveryPolicyvariants and provides clear diagnostic messages on mismatch.tests/worlds/codec_error/decoder_ops.rs (7)
1-13: LGTM!Clear module documentation and appropriate imports for decoder operations.
15-33: LGTM!The setup methods correctly initialise state and delegate to
reset_codec_state. The default max frame length of 1024 matches the codec's default (persrc/codec.rs:119-123).
35-54: LGTM!Frame encoding and partial frame simulation are straightforward. The magic bytes
[0x00, 0x00, 0x00, 0x64]correctly encode 100 in big-endian.
56-85: LGTM!The clean-close detection logic correctly drains complete frames before calling
decode_eof, and distinguishes between the three possible outcomes with clear error messages.
101-131: Acknowledge the fragility and approve.The doc comment at lines 103-114 explicitly calls out that string-matching is fragile and explains why it's acceptable for test code. This is good defensive documentation.
133-172: LGTM!Both
decode_eof_with_partial_dataandencode_oversized_framecorrectly capture errors and return appropriate test results. The error classification is handled upstream.
174-219: LGTM!The verification methods provide clear diagnostic messages and correctly distinguish between clean closes, incomplete EOFs, and oversized frame errors. The check for
InvalidDataerror kind aligns with theOversizedFrame→io::Errorconversion in the codec.src/codec.rs (7)
1-19: Excellent module documentation.The expanded documentation clearly explains the codec layer's error taxonomy and recovery policies, providing a helpful overview for API consumers.
26-36: LGTM: Public API surface appropriately expanded.The module declarations and re-exports provide a clean, ergonomic API for error handling and recovery customisation.
38-48: LGTM: Frame-length and header constants properly exposed.The public constants are well-documented and provide necessary constraints for codec users. The 16 MiB maximum and 64-byte minimum are reasonable defaults.
Also applies to: 127-128
143-170: Robust EOF handling with proper error preservation.The enhanced
decode_eofcorrectly distinguishes clean closes from incomplete data and preserves framing errors (e.g., oversized frames) to maintain accurate recovery policies. The diagnostic logging at line 162 aids troubleshooting without impacting error semantics.
172-179: LGTM: Correct endianness handling with justified lint expectation.The
#[expect]attribute is appropriately scoped and includes a clear reason explaining that wire endianness is an explicit protocol requirement. The big-endian parsing aligns with the length-delimited codec specification.
220-227: LGTM: Proper oversized frame validation.The encoder correctly validates frame size before encoding and returns a structured
CodecError::Framingwith context (size, max), enabling appropriate recovery policy selection.
258-258: Incorrect file reference: test duplication is insrc/codec/error_tests.rsandtests/codec_error.rs, not insrc/codec.rsat line 258.Line 258 simply declares
mod tests;, which is correct. The actual duplicated EOF tests exist elsewhere:
src/codec/error_tests.rscontainsclean_eof_is_detectable()andmid_frame_eof_recommends_disconnect()tests/codec_error.rscontains equivalent tests:eof_clean_close_is_detectable()andeof_mid_frame_is_not_clean()Extract common test logic into a helper function accepting buffer data, expected error kind, and message validator. Test both files directly to remediate the duplication.
Likely an incorrect or invalid review comment.
leynos
changed the title
Introduce a structured CodecError taxonomy to classify codec-layer errors into framing, protocol, I/O, and EOF categories. Define default recovery policies (Drop, Quarantine, Disconnect) per error type and implement a RecoveryPolicyHook trait for customization. Extend WireframeError to include Codec variants and update the LengthDelimitedDecoder to produce structured EOF errors distinguishing clean and premature connection closures. Add comprehensive unit, integration, and behavioural tests covering error classification, recovery policies, and error propagation. Enhance documentation in users-guide, roadmap, and ADR to describe the error taxonomy, recovery policies, and EOF handling. Add metrics support for tracking codec errors by type and recovery policy. This commit improves error handling robustness, observability, and protocol extensibility in the codec layer. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…sertions Introduced a helper function to centralize and simplify testing of EOF error behavior in the length-delimited decoder. This helper replaces duplicated test code that asserts error kind and message content for various EOF edge cases (empty buffer, partial header, partial payload). Tests now call this helper with parameters describing the expected error conditions, improving maintainability and readability. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Previously, decode_eof returned an error on clean close when the buffer was empty. The fix changes it to return Ok(None) for a clean connection close, improving correctness and aligning with expected behavior. Updated tests to verify the new correct behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
- Fix `build_eof_error` to parse length prefix without relying on `Buf` trait - use `u32::from_be_bytes` with explicit clippy allow directives - Remove `PartialEq`/`Eq` implementations from `WireframeError` as the `Codec` variant made equality comparisons meaningless - Consolidate duplicate EOF and oversized-frame tests: removed redundant integration tests from `tests/codec_error.rs` that duplicated unit tests already present in `src/codec.rs` - Implement end-to-end behavioural tests for codec error taxonomy: replace simulated step implementations with real decoder operations that exercise the actual codec error paths 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Address review comments: - Preserve framing errors (InvalidData) at EOF instead of converting all errors to EOF errors; only convert incomplete data errors (Other kind) - Update execplan progress checklist with completed items - Use Oxford spelling "customization" instead of "customisation" - Use colons in EOF sub-variant list for consistent formatting - Extract tests from error.rs to error_tests.rs to stay under 400 lines - Remove unnecessary .to_string() allocations in inc_codec_error metrics - Delegate WireframeError::is_clean_close to CodecError::is_clean_close - Handle unknown error types explicitly in cucumber steps - Simplify String handling with as_str() instead of into_boxed_str() - Move doc comment before derive attributes in CodecErrorWorld - Add #[expect] attributes with reason for clippy suppressions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The multi-line string in the `#[expect]` attribute was reformatted to avoid trailing spaces and improve readability without changing semantics. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Extract tests from `src/codec.rs` into a new `src/codec/tests.rs` file to improve code organization and maintainability without changing test functionality. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
- Replace individual EOF error tests with parameterized tests using rstest. - Add detailed tests verifying correct io::ErrorKind conversion for CodecError variants. - Enhance CodecErrorWorld test helper with new fields and methods to manage error states. - Minor defensive comment and code improvements in codec.rs for EOF error handling. - Improve test code comments explaining EOF error classification fragility. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
This refactors the length-delimited codec's EOF error construction by: - Using safe buffer access to parse length header with `get()` and `try_from` instead of unchecked slicing. - Introducing a dedicated `parse_length_header` function for clarity. - Logging inner decoder errors on EOF before converting to structured EOF errors. Additionally, the EOF error classification in codec error tests now extracts the expected payload length from the buffer's length header instead of defaulting to zero, improving test robustness. These changes enhance safety, diagnostics, and test accuracy without altering external behavior. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
- Remove legacy codec_error test world implementation. - Introduce new modular codec_error test world with explicit error and EOF variants. - Add realistic end-to-end decoder operations for validating error handling and recovery policies. - Update test steps to reference new codec_error world implementation. - Make LENGTH_HEADER_SIZE constant public for test usage. This change improves the coverage and accuracy of codec error scenario tests by using actual decoding and encoding operations, facilitating robust validation of framing, protocol, IO, and EOF error handling with proper recovery policy checks. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
- Expanded `build_eof_error` doc to clarify EOF error cases (MidHeader vs MidFrame). - Corrected documentation in `extract_expected_length` test function about length header. - Updated module-level docs to mention codec error taxonomy world. These changes improve code clarity and documentation accuracy without behavior changes. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
force-pushed
the
terragon/introduce-codecerror-taxonomy-3abr1i
branch
from
e5ea54e to
2e71a3d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md:
- Line 36: The document still references Cucumber tests (e.g., the checklist
item "- [x] (2026-01-07) Add Cucumber behavioural tests for codec error
scenarios" and occurrences at lines 36, 218-226, 307-313, 441, 444); remove or
replace each Cucumber mention with the approved alternative testing approach
(e.g., "integration tests using Jest/Playwright" or "end-to-end tests using
<replacement framework>") and update any checklist items and test descriptions
to reflect the new test names, runners, and file paths so the ExecPlan
accurately describes the replacement testing strategy.
In @src/codec/error_tests.rs:
- Line 131: Replace the char literal in the assertion with a string literal to
match the rest of the file: change the assertion that calls
display.contains('5') to use display.contains("5") so all contains() checks
(e.g., the one using "10") use string literals consistently; update only the
assertion referencing display to use "5".
- Around line 99-111: Replace the four repetitive assertions in the test
function error_type_returns_correct_category with an rstest parameterized test:
add #[rstest] to the test, import rstest::rstest, and declare test parameters
(e.g. error: CodecError, expected: &str) with multiple #[case(...)] entries for
CodecError::Framing(FramingError::EmptyFrame) -> "framing",
CodecError::Protocol(ProtocolError::UnknownMessageType { type_id: 1 }) ->
"protocol", CodecError::Io(io::Error::other("test")) -> "io", and
CodecError::Eof(EofError::CleanClose) -> "eof", then assert that
error.error_type() == expected inside the test body.
- Around line 8-71: Consolidate the repetitive tests by adding rstest
(dev-dependency) and replacing the repeated assertions with a parameterized test
that calls CodecError::default_recovery_policy() and
CodecError::should_disconnect(); create one #[rstest] fn (e.g.
recovery_policy_cases) taking parameters (err, expected_policy: RecoveryPolicy,
expected_disconnect: bool) and supply tuples for OversizedFrame, EmptyFrame,
InvalidLengthEncoding, Protocol::UnknownMessageType,
Io(io::Error::other("test")), Eof::MidFrame and Eof::MidHeader;
assert_eq!(err.default_recovery_policy(), expected_policy) and
assert_eq!(err.should_disconnect(), expected_disconnect). Leave the clean EOF
case as its own small test asserting err.is_clean_close() and
should_disconnect() for Eof::CleanClose. Ensure you import rstest::rstest and
reference the exact symbols CodecError,
FramingError::OversizedFrame/EmptyFrame/InvalidLengthEncoding,
ProtocolError::UnknownMessageType, EofError::CleanClose/MidFrame/MidHeader,
RecoveryPolicy, default_recovery_policy, should_disconnect, and is_clean_close.
In @src/metrics.rs:
- Around line 122-152: The RecoveryPolicy enum lacks a conversion to the string
values expected by inc_codec_error; add an impl for RecoveryPolicy with a pub
const fn as_str(self) -> &'static str that returns "drop", "quarantine", or
"disconnect" for the Drop, Quarantine, and Disconnect variants respectively, and
update callers to pass recovery_policy.as_str() to inc_codec_error so metrics
use the canonical strings.
In @tests/worlds/codec_error/decoder_ops.rs:
- Around line 101-131: Add a FIXME comment above the classify_eof_error method
noting the fragility of string-matching on std::io::Error messages and link to
the tracking issue (e.g., FIXME: track replacing message-based classification
with downcasting to CodecError – issue #1234). Reference the relevant symbols in
the comment so future readers know what to change: classify_eof_error, EofError,
LENGTH_HEADER_SIZE, and extract_expected_length; mention that when an underlying
error type is exposed, the logic should switch from msg.contains("header") to
downcasting and using concrete error variants.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (23)
docs/adr-004-pluggable-protocol-codecs.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/roadmap.mddocs/users-guide.mdsrc/app/error.rssrc/codec.rssrc/codec/error.rssrc/codec/error_tests.rssrc/codec/recovery.rssrc/codec/tests.rssrc/hooks.rssrc/lib.rssrc/metrics.rssrc/response.rstests/codec_error.rstests/cucumber.rstests/features/codec_error.featuretests/steps/codec_error_steps.rstests/steps/mod.rstests/world.rstests/worlds/codec_error/decoder_ops.rstests/worlds/codec_error/mod.rstests/worlds/mod.rs
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runmake check-fmt(cargo fmt --workspace -- --check) before committing to validate Rust formatting across the entire workspace without modifying files.
Runmake lint(cargo clippy --workspace --all-targets --all-features -- -D warnings) before committing to lint every target with all features enabled and deny all Clippy warnings.
Runmake test(cargo test --workspace) before committing to run the full workspace test suite.
Clippy warnings MUST be disallowed. Fix any warnings emitted during tests in the code itself rather than silencing them.
Usemake fmt(cargo fmt --workspace) to apply formatting fixes reported by the formatter check.
Where a Rust function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a Rust function has too many parameters, group related parameters in meaningfully named structs.
Where a Rust function is returning a large error, consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new Rust functionality. Run both before and after making any change.
Every Rust module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public Rust APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings in Rust code.
Avoidunsafecode in Rust unless absolutely necessary, and document any usage clearly with a 'SAFETY' comment.
Place function attributes after doc comments in Rust code.
Do not usereturnin single-line Rust functions.
Use predicate functions for Rust conditional criteria with more than two branches.
Lints must not be silenced in Rust except as a last resort. Lint rule suppressions must be tightly scoped and include a clear reason.
Useconcat!()to combine long string literals in Rust rather than escaping newlines with a backslash.
Prefer single-line versions of Rust functions where...
Files:
tests/codec_error.rstests/world.rssrc/response.rssrc/metrics.rssrc/hooks.rssrc/codec/tests.rssrc/codec/error.rstests/worlds/mod.rssrc/app/error.rstests/steps/mod.rssrc/codec/error_tests.rstests/steps/codec_error_steps.rssrc/codec.rstests/cucumber.rssrc/lib.rstests/worlds/codec_error/decoder_ops.rstests/worlds/codec_error/mod.rssrc/codec/recovery.rs
⚙️ CodeRabbit configuration file
**/*.rs: * Seek to keep the cognitive complexity of functions no more than 9.
- Adhere to single responsibility and CQRS
- Place function attributes after doc comments.
- Do not use
returnin single-line functions.- Move conditionals with >2 branches into a predicate function.
- Avoid
unsafeunless absolutely necessary.- Every module must begin with a
//!doc comment that explains the module's purpose and utility.- Comments and docs must follow en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.- Where code is only used by specific features, it must be conditionally compiled or a conditional expectation for unused_code applied.
- Use
rstestfixtures for shared setup and to avoid repetition between tests.- Replace duplicated tests with
#[rstest(...)]parameterised cases.- Prefer
mockallfor mocks/stubs.- Prefer
.expect()over.unwrap()in tests..expect()and.unwrap()are forbidden outside of tests. Errors must be propagated.- Ensure that any API or behavioural changes are reflected in the documentation in
docs/- Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/- Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
- Environment access (env::set_var and env::remove_var) are always unsafe in Rust 2024 and MUST be marked as such
- For testing of functionality depending upon environment variables, dependency injection and...
Files:
tests/codec_error.rstests/world.rssrc/response.rssrc/metrics.rssrc/hooks.rssrc/codec/tests.rssrc/codec/error.rstests/worlds/mod.rssrc/app/error.rstests/steps/mod.rssrc/codec/error_tests.rstests/steps/codec_error_steps.rssrc/codec.rstests/cucumber.rssrc/lib.rstests/worlds/codec_error/decoder_ops.rstests/worlds/codec_error/mod.rssrc/codec/recovery.rs
docs/**/*.md
📄 CodeRabbit inference engine (docs/contents.md)
Follow the documentation style guide conventions when writing project documentation
Write official documentation with separate guides for 'Duplex Messaging & Pushes', 'Streaming Responses', and 'Message Fragmentation', each including runnable examples and explaining relevant concepts and APIs
docs/**/*.md: Use markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ('-ize' / '-yse' / '-our'), with the exception of the filenameLICENSE.
Reference the documentation style guide provided atdocs/documentation-style-guide.mdfor documentation writing standards.Documentation describing multi-packet streaming MUST reflect the tuple-based API and explain helper constructors that prepare the channel pair
Replace or retire documentation that refers to Cucumber, including guides and examples in docs/
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization', suffix -lyse in words like 'analyse' and 'paralyse', suffix -our in words like 'colour' and 'behaviour', suffix -re in words like 'calibre' and 'centre', double 'l' in words like 'cancelled' and 'counsellor', maintain the 'e' in words like 'likeable' and 'liveable', suffix -ogue in words like 'analogue' and 'catalogue'
The word 'outwith' is acceptable in documentation
Keep US spelling when used in an API, for example 'color'
Use the Oxford comma: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Lille Industries are expanding'
Write hea...
Files:
docs/users-guide.mddocs/adr-004-pluggable-protocol-codecs.mddocs/roadmap.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
Every doctest should validate the public API of a crate from the perspective of an external user, treating each documentation test as a separate temporary crate that imports the library as an external dependency
Files:
docs/users-guide.mddocs/adr-004-pluggable-protocol-codecs.mddocs/roadmap.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes in Markdown.
Files:
docs/users-guide.mddocs/adr-004-pluggable-protocol-codecs.mddocs/roadmap.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/users-guide.mddocs/adr-004-pluggable-protocol-codecs.mddocs/roadmap.mddocs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
docs/adr-[0-9][0-9][0-9]-*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/adr-[0-9][0-9][0-9]-*.md: ADR naming convention: Name ADR files using the pattern 'adr-NNN-short-description.md', where NNN is a zero-padded sequence number (e.g. 'adr-001-async-fixtures-and-tests.md'). Place ADRs in the 'docs/' directory
ADR required sections: Include Status (one of 'Proposed', 'Accepted', 'Superseded', or 'Deprecated'; for 'Accepted' status include the date and a brief summary of what was decided), Date (format: YYYY-MM-DD), and Context and Problem Statement sections in order
ADR conditional sections: Include Decision Drivers, Requirements, Options Considered, Decision Outcome / Proposed Direction, Goals and Non-Goals, Migration Plan, Known Risks and Limitations, Outstanding Decisions, and Architectural Rationale sections as appropriate to the decision's complexity
ADR formatting: Use second-level headings (##) for major sections and third-level headings (###) for subsections (e.g. phases, option names)
ADR formatting: Use tables to compare options when multiple dimensions are relevant and include a caption below the table (e.g. 'Table 1: Trade-offs between X and Y.')
ADR formatting: Include code snippets with language identifiers when illustrating technical approaches. Use 'no_run' for illustrative Rust code that should not be executed
ADR formatting: Add screen reader descriptions before complex diagrams or code blocks
ADR formatting: Reference external sources using inline links or footnotes
Files:
docs/adr-004-pluggable-protocol-codecs.md
docs/**/roadmap*.md
📄 CodeRabbit inference engine (docs/documentation-style-guide.md)
docs/**/roadmap*.md: Roadmap task writing: Define outcomes, not intentions. Phrase tasks in terms of the capability delivered (e.g. 'Implement role-based access control for API endpoints'), not aspirations
Roadmap task writing: Quantify completion criteria with measurable finish lines (e.g. '90% test coverage for new modules', 'response times under 200ms')
Roadmap task writing: Break into atomic increments. Ensure tasks can be completed in weeks, not quarters. Large goals should be decomposed into clear, deliverable units
Roadmap task writing: Tie to dependencies and sequencing. Document prerequisites so tasks can be scheduled realistically
Roadmap task writing: Bound scope explicitly. Note both in-scope and out-of-scope elements (e.g. 'Build analytics dashboard (excluding churn prediction)')
Roadmap formatting: Use dotted numbering for phases (1, 2, 3, …), steps (1.1, 1.2, 1.3, …), and headline tasks (1.1.1, 1.1.2, 1.1.3, …)
Roadmap formatting: Precede task and sub-task items with a GitHub-Flavoured Markdown (GFM) checkbox ([]) to track completion status
Roadmap formatting: Note non-linear dependencies explicitly using dotted notation (e.g. 'Requires 2.3.1')
Roadmap formatting: Include explicit success criteria only where not immediately obvious from the task description
Roadmap formatting: Where applicable, cite the relevant design document section for each task (e.g. 'See design-doc.md §3.2')
Files:
docs/roadmap.md
🧠 Learnings (5)
📚 Learning: 2026-01-03T17:44:28.792Z
Learnt from: leynos
Repo: leynos/wireframe PR: 412
File: tests/streaming_request.rs:85-107
Timestamp: 2026-01-03T17:44:28.792Z
Learning: In Rust code (e.g., the wireframe project), avoid direct indexing like vec[0] because it can panic. Prefer safe accessors that return Option, such as vec.get(0), vec.first(), or patterns that handle Option safely (e.g., match or if let Some). This should be applied across Rust files, including tests (e.g., tests/streaming_request.rs), regardless of prior length checks, to ensure safety and avoid panics from out-of-bounds access.
Applied to files:
tests/codec_error.rstests/world.rssrc/response.rssrc/metrics.rssrc/hooks.rssrc/codec/tests.rssrc/codec/error.rstests/worlds/mod.rssrc/app/error.rstests/steps/mod.rssrc/codec/error_tests.rstests/steps/codec_error_steps.rssrc/codec.rstests/cucumber.rssrc/lib.rstests/worlds/codec_error/decoder_ops.rstests/worlds/codec_error/mod.rssrc/codec/recovery.rs
📚 Learning: 2026-01-08T08:16:31.989Z
Learnt from: leynos
Repo: leynos/wireframe PR: 411
File: src/client/tests/lifecycle.rs:0-0
Timestamp: 2026-01-08T08:16:31.989Z
Learning: In Rust, apply #[expect(lint, reason = "...")] only to code that actually triggers the specified lint. If the code does not trigger the lint, Clippy will fail with 'unfulfilled lint expectation' errors, which is intentional to prevent stale suppressions. Do not add #[expect] attributes proactively or to code that doesn't trigger the lint, even if similar code elsewhere requires suppression. This guideline should be applied broadly to Rust source files across the repository.
Applied to files:
tests/codec_error.rstests/world.rssrc/response.rssrc/metrics.rssrc/hooks.rssrc/codec/tests.rssrc/codec/error.rstests/worlds/mod.rssrc/app/error.rstests/steps/mod.rssrc/codec/error_tests.rstests/steps/codec_error_steps.rssrc/codec.rstests/cucumber.rssrc/lib.rstests/worlds/codec_error/decoder_ops.rstests/worlds/codec_error/mod.rssrc/codec/recovery.rs
📚 Learning: 2026-01-05T20:11:49.999Z
Learnt from: CR
Repo: leynos/wireframe PR: 0
File: docs/adr-004-pluggable-protocol-codecs.md:0-0
Timestamp: 2026-01-05T20:11:49.999Z
Learning: Outstanding decisions pending: codec error taxonomy and recovery policy (drop frame vs. close connection), realignment of fragmentation with the FragmentAdapter design, and unification of codec handling between app router and Connection actor
Applied to files:
docs/adr-004-pluggable-protocol-codecs.md
📚 Learning: 2026-01-05T20:11:49.999Z
Learnt from: CR
Repo: leynos/wireframe PR: 0
File: docs/adr-004-pluggable-protocol-codecs.md:0-0
Timestamp: 2026-01-05T20:11:49.999Z
Learning: Applies to docs/**/*_test.rs,**/tests/**/*.rs : Add codec-specific tests that exercise encoder/decoder round-trips, payload extraction, correlation identifiers, and maximum frame length enforcement
Applied to files:
src/codec/tests.rssrc/codec.rs
📚 Learning: 2026-01-08T18:03:59.395Z
Learnt from: leynos
Repo: leynos/wireframe PR: 417
File: tests/steps/codec_error_steps.rs:58-62
Timestamp: 2026-01-08T18:03:59.395Z
Learning: In the wireframe repo, test step functions that always succeed should not return TestResult, because clippy::unnecessary_wraps will fire and cannot be suppressed in tests. Do not use #[allow(...)] or #[expect(clippy::unnecessary_wraps)] to silence it. Refactor such steps to return () or a type that does not trigger the lint, or move these steps to non-test code or a helper that isn’t linted the same way. This guidance applies to all files under tests/steps that contain similar patterns.
Applied to files:
tests/steps/mod.rstests/steps/codec_error_steps.rs
🧬 Code graph analysis (11)
tests/codec_error.rs (5)
src/codec/error.rs (4)
default_recovery_policy(224-232)should_disconnect(278-280)error_type(288-295)is_clean_close(254-254)src/response.rs (2)
is_clean_close(251-253)from_codec(243-243)src/codec.rs (3)
new(102-106)correlation_id(84-84)default(120-124)src/codec/recovery.rs (10)
new(133-133)context_with_peer_address(333-337)recovery_policy_default_is_drop(316-318)recovery_config_builder(370-379)default(277-283)max_consecutive_drops(289-292)quarantine_duration(217-219)quarantine_duration(296-299)log_dropped_frames(303-306)recovery_policy(208-211)src/hooks.rs (1)
default(141-150)
src/response.rs (1)
src/codec/error.rs (1)
is_clean_close(254-254)
src/metrics.rs (3)
src/codec/error.rs (1)
error_type(288-295)tests/codec_error.rs (1)
recovery_policy(141-143)src/codec/recovery.rs (1)
recovery_policy(208-211)
src/codec/error.rs (3)
src/response.rs (4)
from(110-110)from(114-114)from(233-233)is_clean_close(251-253)src/codec.rs (1)
new(102-106)src/codec/recovery.rs (1)
new(133-133)
src/app/error.rs (2)
src/codec/error.rs (1)
from(299-309)src/response.rs (3)
from(110-110)from(114-114)from(233-233)
src/codec/error_tests.rs (1)
src/codec/error.rs (4)
default_recovery_policy(224-232)should_disconnect(278-280)is_clean_close(254-254)error_type(288-295)
tests/steps/codec_error_steps.rs (1)
src/codec/error.rs (1)
error_type(288-295)
src/codec.rs (4)
src/frame/format.rs (1)
bytes(54-54)tests/worlds/codec_error/mod.rs (1)
build_eof_error(152-164)tests/worlds/correlation.rs (1)
expected(45-45)src/fragment/index.rs (1)
try_from(48-48)
tests/worlds/codec_error/decoder_ops.rs (3)
src/frame/format.rs (1)
bytes(54-54)src/codec.rs (5)
new(102-106)encoder(73-73)encoder(250-255)decoder(70-70)decoder(244-248)src/fragment/index.rs (1)
try_from(48-48)
tests/worlds/codec_error/mod.rs (2)
src/codec.rs (2)
default(120-124)build_eof_error(189-216)src/codec/error.rs (1)
error_type(288-295)
src/codec/recovery.rs (1)
tests/codec_error.rs (4)
recovery_policy(141-143)recovery_policy_default_is_drop(116-118)context_with_peer_address(103-109)recovery_config_builder(121-132)
🪛 LanguageTool
docs/users-guide.md
[typographical] ~175-~175: To join two clauses or introduce examples, consider using an em dash.
Context: ... Error categories: - FramingError - Wire-level frame boundary issues (oversi...
(DASH_RULE)
[typographical] ~177-~177: To join two clauses or introduce examples, consider using an em dash.
Context: ...atches, empty frames). - ProtocolError - Semantic violations after frame extracti...
(DASH_RULE)
[typographical] ~179-~179: To join two clauses or introduce examples, consider using an em dash.
Context: ... types, invalid versions). - io::Error - Transport layer failures (connection res...
(DASH_RULE)
[typographical] ~180-~180: To join two clauses or introduce examples, consider using an em dash.
Context: ...nnection resets, timeouts). - EofError - End-of-stream conditions with clean/mid-...
(DASH_RULE)
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md
[typographical] ~94-~94: To join two clauses or introduce examples, consider using an em dash.
Context: ... Codec implementation: - src/codec.rs - FrameCodec trait and `LengthDelimitedF...
(DASH_RULE)
[typographical] ~95-~95: To join two clauses or introduce examples, consider using an em dash.
Context: ...tedFrameCodec-src/codec/examples.rs- Example codec implementations -src/app...
(DASH_RULE)
[typographical] ~96-~96: To join two clauses or introduce examples, consider using an em dash.
Context: ...mentations - src/app/combined_codec.rs - Adapter for Tokio Framed Error handli...
(DASH_RULE)
[typographical] ~100-~100: To join two clauses or introduce examples, consider using an em dash.
Context: ...ndling paths: - src/app/connection.rs - Connection processing and error handling...
(DASH_RULE)
[typographical] ~101-~101: To join two clauses or introduce examples, consider using an em dash.
Context: ...r handling - src/app/frame_handling.rs - Frame deserialization and response forwa...
(DASH_RULE)
[typographical] ~102-~102: To join two clauses or introduce examples, consider using an em dash.
Context: ...and response forwarding - src/hooks.rs - Protocol hooks including error callbacks...
(DASH_RULE)
[typographical] ~106-~106: To join two clauses or introduce examples, consider using an em dash.
Context: ...nfrastructure: - tests/frame_codec.rs - Codec integration tests - `tests/feature...
(DASH_RULE)
[typographical] ~107-~107: To join two clauses or introduce examples, consider using an em dash.
Context: ...ec integration tests - tests/features/ - Cucumber feature files - tests/steps/ ...
(DASH_RULE)
[typographical] ~108-~108: To join two clauses or introduce examples, consider using an em dash.
Context: ...les - tests/steps/ and tests/worlds/ - Step definitions and world types Docume...
(DASH_RULE)
[typographical] ~112-~112: To join two clauses or introduce examples, consider using an em dash.
Context: ...entation to update: - docs/roadmap.md - Mark 9.1.2 done - docs/users-guide.md ...
(DASH_RULE)
[typographical] ~113-~113: To join two clauses or introduce examples, consider using an em dash.
Context: ... Mark 9.1.2 done - docs/users-guide.md - Public interface documentation - `docs/a...
(DASH_RULE)
[typographical] ~114-~114: To join two clauses or introduce examples, consider using an em dash.
Context: ...cs/adr-004-pluggable-protocol-codecs.md` - Design decisions ## Plan of Work ### P...
(DASH_RULE)
[typographical] ~123-~123: To join two clauses or introduce examples, consider using an em dash.
Context: ...r classification: 1. Framing errors - Wire-level frame boundary issues (e.g., ...
(DASH_RULE)
[typographical] ~125-~125: To join two clauses or introduce examples, consider using an em dash.
Context: ...hecksum mismatch) 2. Protocol errors - Semantic violations after frame extracti...
(DASH_RULE)
[typographical] ~128-~128: To join two clauses or introduce examples, consider using an em dash.
Context: ... sequence violation) 3. IO errors - Transport layer failures (wrapped `io::E...
(DASH_RULE)
[typographical] ~129-~129: To join two clauses or introduce examples, consider using an em dash.
Context: ... (wrapped io::Error) 4. EOF errors - End-of-stream handling with three sub-va...
(DASH_RULE)
[typographical] ~136-~136: To join two clauses or introduce examples, consider using an em dash.
Context: ...cy()method returning one of: -Drop` - Discard the malformed frame and continue...
(DASH_RULE)
[typographical] ~137-~137: To join two clauses or introduce examples, consider using an em dash.
Context: ...inue (recoverable errors) - Quarantine - Pause the connection temporarily (rate l...
(DASH_RULE)
[typographical] ~138-~138: To join two clauses or introduce examples, consider using an em dash.
Context: ...mporarily (rate limiting) - Disconnect - Terminate the connection (fatal errors) ...
(DASH_RULE)
[typographical] ~171-~171: Two consecutive dots
Context: ...e)for EOF at frame boundary 3. ReturnCodecError::Eof(EofError::MidFrame{..})for EOF during payload 4. ReturnCo...
(DOUBLE_PUNCTUATION)
[typographical] ~172-~172: Two consecutive dots
Context: ...{..})for EOF during payload 4. ReturnCodecError::Eof(EofError::MidHeader{..})for EOF during header 5. Maintaini...
(DOUBLE_PUNCTUATION)
[uncategorized] ~232-~232: Loose punctuation mark.
Context: ...documentation: 1. docs/users-guide.md: - Add section on codec error handlin...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~238-~238: Loose punctuation mark.
Context: ...cs/adr-004-pluggable-protocol-codecs.md: - Document the CodecError` taxonomy...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~243-~243: Loose punctuation mark.
Context: ...OF handling design 3. docs/roadmap.md: - Mark all 9.1.2 sub-items as done ...
(UNLIKELY_OPENING_PUNCTUATION)
[duplication] ~255-~255: Possible typo: you repeated a word.
Context: ...CodecError::default_recovery_policy() implementation - From<CodecError> for io::Error implementation 2. Create src/codec/recovery.rs with...
(ENGLISH_WORD_REPEAT_RULE)
[style] ~260-~260: Using four (or more) nouns in a row may decrease readability.
Context: ...ryPolicyenum -CodecErrorContextstruct -RecoveryPolicyHooktrait -DefaultRecoveryPolicyimplementation -RecoveryConfigbuilder config 3. Updatesrc/codec.rs`: - ...
(FOUR_NN)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (78)
tests/steps/mod.rs (1)
9-9: LGTM!The module declaration cleanly extends the test step definitions to include codec error scenarios, following the existing pattern.
tests/features/codec_error.feature (1)
1-25: Well-structured BDD scenarios.The feature file provides clear, comprehensive coverage of the codec error taxonomy and recovery policies. The scenarios are well-organised and follow proper Gherkin syntax.
src/codec/tests.rs (8)
1-11: LGTM!Proper module-level documentation and clean imports.
13-17: LGTM!Proper boundary test for maximum frame length clamping with safe arithmetic.
19-42: LGTM!Thorough round-trip test validating encode/decode cycle with proper error handling.
44-58: LGTM!Proper validation of oversized payload rejection with appropriate error kind checking.
60-68: LGTM!Valid test for payload reuse optimisation using pointer comparison to verify zero-copy behaviour.
70-82: LGTM!Proper handling of the clean close case where an empty buffer at EOF returns
Ok(None).
84-108: Excellent use of parameterised tests.The rstest parameterisation eliminates code duplication whilst maintaining clear test intent. Each case is well-documented with buffer contents, expected error kind, and message validation. This aligns perfectly with the coding guidelines' recommendation to use parameterised tests.
Based on coding guidelines: "Replace duplicated tests with
#[rstest(...)]parameterised cases."
110-128: LGTM!Proper success path test for EOF handling when a complete frame is present in the buffer.
tests/world.rs (1)
11-11: LGTM!Clean addition of the codec error test world to the public exports, following the established pattern.
tests/worlds/mod.rs (2)
1-7: LGTM!Documentation properly updated to reflect the addition of codec error taxonomy coverage.
33-34: LGTM!Proper module declaration with path attribute, following the established pattern in the file.
docs/adr-004-pluggable-protocol-codecs.md (1)
256-276: Well-documented resolution of the outstanding decision.The new "Resolved Decisions" section clearly captures the CodecError taxonomy, recovery policies, and observability hooks. This aligns with the learning that codec error taxonomy and recovery policy were pending decisions. The cross-references to
src/codec/error.rsandsrc/codec/recovery.rsprovide useful navigation for implementers.docs/users-guide.md (1)
168-270: Comprehensive codec error handling documentation.The new section thoroughly documents the error taxonomy, recovery policies, and integration points. The code examples are clear and demonstrate practical usage patterns. The table format for recovery policies aids quick reference.
tests/codec_error.rs (7)
1-14: Module documentation and imports are correct.The module-level doc comment explains the file's purpose. Imports cover the full public API surface needed for testing the CodecError taxonomy.
20-84: Thorough taxonomy coverage.The tests exercise all CodecError variants and verify their recovery policies, disconnect behaviour, and error type categorization. The assertions align with the implementation in
src/codec/error.rs.
90-109: Context builder tests are adequate.The tests verify the builder pattern for
CodecErrorContext, including all fields and peer address handling.
115-132: Recovery policy and config tests are correct.The tests verify the default policy and builder pattern for
RecoveryConfig, aligning with the implementation.
138-157: Custom hook override test demonstrates the extension point.The
StrictRecoveryimplementation and test show how custom recovery policies override defaults, which is a key use case for the hook system.
159-190: Clear scope delineation and WireframeError integration tests.The NOTE comment clarifies that decoder EOF tests reside in
src/codec.rs, avoiding confusion about test duplication. The WireframeError integration tests verify thefrom_codecconstructor and display formatting.
196-216: App error integration tests are appropriate.The tests verify
From<CodecError>conversions for bothWireframeErrorandSendErrorin the app error surface, ensuring consistent error propagation.docs/roadmap.md (1)
352-361: Roadmap accurately reflects completed work.The 9.1.2 tasks are correctly marked complete. The descriptions align with the implemented CodecError taxonomy, recovery policies, EOF handling, and test coverage delivered in this PR.
src/metrics.rs (1)
36-44: Metric constant follows established conventions.The
CODEC_ERRORSconstant uses thewireframe_prefix and_totalsuffix consistent with other counters in this module. The documentation clearly describes the label structure.tests/cucumber.rs (1)
1-72: Cucumber runner integration is complete and consistent.All touchpoints are updated: module doc comment, import, and runner call. The
CodecErrorWorldfollows the established pattern for test world integration.src/response.rs (3)
198-230: WireframeError Codec variant is well-integrated.The new
Codecvariant follows the established pattern forIoandProtocol. The documentation explains the error categorisation clearly.
240-254: Helper methods follow established patterns.The
from_codecconstructor mirrorsfrom_io, andis_clean_closecorrectly delegates toCodecError::is_clean_close()as shown in the relevant code snippet fromsrc/codec/error.rs.
256-277: Display and Error implementations are complete.The
Displayformatting uses "codec error:" prefix consistent with the other variants' style. Thesource()implementation correctly exposes the innerCodecError.src/app/error.rs (2)
7-27: Codec error integration correctly implemented.The addition of
Codec(#[from] CodecError)variant properly extends WireframeError to propagate codec-layer failures. The removal ofPartialEqandEqderives is necessary becauseCodecErrorcontainsio::Error, which doesn't implement these traits. The expanded documentation appropriately describes error categorisation by origin (framing, protocol, I/O, EOF).
39-41: SendError codec integration mirrors WireframeError pattern.The
Codec(#[from] CodecError)variant enables codec errors to propagate through send operations, maintaining consistency with the WireframeError integration.tests/steps/codec_error_steps.rs (3)
1-40: Given steps correctly handle success and failure cases.The distinction between infallible setup steps (returning
()) and fallible configuration steps (returningTestResult) aligns with the repository's lint policy. The#[expect(clippy::needless_pass_by_value)]attribute on lines 23-26 is correctly scoped and reasoned, as Cucumber step macros require ownedStringfor{word}captures.
42-75: When steps appropriately distinguish setup from operations that can fail.The
when_client_sends_partialstep (lines 58-62) correctly returns()as it only sets up state without error paths, whilst other When steps that invoke codec operations returnTestResultto propagate potential failures.
77-101: Then steps correctly return TestResult for verification operations.All verification steps appropriately return
TestResultto propagate assertion failures. The#[expect(clippy::needless_pass_by_value)]attribute on lines 95-98 is correctly scoped with a clear reason, matching the pattern established in the Given steps.src/codec/error.rs (6)
1-25: Comprehensive module-level documentation.The module documentation clearly describes the error taxonomy (framing, protocol, I/O, EOF) and recovery policies (Drop, Quarantine, Disconnect), providing context for the structured error handling introduced by this PR.
32-72: FramingError variants comprehensively cover wire-level failures.The enum design appropriately distinguishes between oversized frames (size enforcement), corrupt length encoding (framing state corruption), incomplete headers (need more data), checksum failures (data integrity), and empty frames (protocol constraint). Field names are clear, and error messages are descriptive. The use of
{:#x}for checksum formatting (line 61) improves diagnostic readability.
74-118: ProtocolError variants appropriately capture semantic violations.The enum design correctly distinguishes between header issues (missing fields), version mismatches, unknown message types, sequence violations, and state machine errors. The use of
Stringfor field and state names provides clear diagnostic context, whilst numeric types (u32 for version/type_id, u64 for sequences) are appropriately chosen.
120-155: EofError variants effectively distinguish clean from premature closure.The enum design correctly separates clean EOF (at frame boundary, no data lost) from premature closure scenarios (mid-frame, mid-header). The inclusion of
Copyin the derives (line 124) is appropriate, as all fields areusizewith no heap allocations. Error messages provide clear diagnostic context about bytes received versus expected.
194-232: Recovery policy logic appropriately distinguishes recoverable from unrecoverable errors.The
default_recovery_policymethod correctly categorises errors:
- Drop policy (lines 227-228):
OversizedFrame,EmptyFrame, and allProtocolerrors are recoverable—discard the malformed frame and continue.- Disconnect policy (line 230): Other
Framingerrors (corrupt framing state),Ioerrors (transport failure), and allEoferrors (connection lost) require termination.This logic aligns with the principle that framing state corruption and transport failures cannot be safely recovered, whilst oversized or malformed individual frames can be dropped without affecting subsequent frames.
298-310: CodecError to io::Error conversion preserves error semantics.The
Fromimplementation correctly maps codec errors toio::ErrorKind:
- InvalidData (lines 302-303): Framing and protocol errors represent data validation failures.
- UnexpectedEof (lines 304-307): EOF errors represent premature connection closure during reads.
- Special case (lines 304-306):
CleanClosereceives a simplified message whilst preserving its distinct semantic meaning (graceful shutdown).Error sources are preserved in all cases, maintaining diagnostic context.
src/hooks.rs (3)
66-102: on_eof hook extends WireframeProtocol with backward compatibility.The new
on_eofmethod correctly provides a default no-op implementation, allowing existing protocol implementations to continue working without modification. The signature appropriately borrows&EofErrorand&[u8](no ownership transfer needed) whilst providing mutable access toConnectionContext. The comprehensive documentation includes a practical example demonstrating EOF variant handling.
121-149: OnEofHook type alias and field follow established hook patterns.The
OnEofHooktype alias (lines 121-122) correctly usesFnMut(allowing hook state mutation), withSend + 'staticbounds for cross-thread usage. Theon_eoffield addition toProtocolHooks(lines 136-137) and itsNoneinitialization inDefault(line 148) match the patterns established by existing hooks (before_send,on_command_end, etc.).
186-237: on_eof implementation and protocol wiring correctly follow established patterns.The
on_eofmethod (lines 186-191) appropriately invokes the registered hook when present, matching the conditional pattern used by other hook methods. Thefrom_protocolwiring (lines 223-228, 236) correctly clones theArc, creates a boxed closure that delegates to the protocol'son_eofmethod, and includes the hook in the returnedProtocolHooks. This implementation mirrors the patterns established bybefore_send,on_command_end, and other hooks.src/lib.rs (2)
89-89: CODEC_ERRORS metric export is correctly defined.The metric is properly declared in the metrics module with the naming pattern
wireframe_codec_errors_total, enabling codec-layer failure observability consistent with the existing instrumentation.
17-31: Well-structured consolidation of codec exports.Unify the codec module surface with this expanded
pub use codec::block. The re-exports correctly include the error taxonomy (CodecError, FramingError, ProtocolError, EofError), recovery infrastructure (RecoveryPolicy, RecoveryConfig, DefaultRecoveryPolicy, RecoveryPolicyHook, CodecErrorContext), codec types (FrameCodec, LengthDelimitedFrameCodec), and frame length constants (MAX_FRAME_LENGTH, MIN_FRAME_LENGTH). All items are publicly accessible from the codec module and properly aggregated for consumer convenience.src/codec.rs (9)
6-19: LGTM!The module-level documentation extension clearly explains the error taxonomy and recovery policies. The en-GB spelling ("customising") is correct and the doc references are properly linked.
26-36: LGTM!Public module declarations and re-exports are well-organised. The re-exported types provide a clean public API surface for consumers.
38-48: LGTM!Constants are appropriately documented with Rustdoc comments explaining their purpose and usage constraints.
127-128: LGTM!The
LENGTH_HEADER_SIZEconstant is correctly defined and documented.
143-170: LGTM!The
decode_eofimplementation correctly handles the three EOF scenarios:
- Clean close when buffer is empty.
- Preserves
InvalidDataframing errors (e.g., oversized frames).- Converts other errors to structured EOF errors with diagnostic logging.
The tracing diagnostic at line 162-165 aids debugging without leaking sensitive data.
172-179: LGTM!The
#[expect]attribute is correctly scoped with an appropriate reason. The cast fromu32tousizeis safe on all supported platforms.
189-216: LGTM!The
build_eof_errorfunction uses safe accessors (.get()) and correctly distinguishes between mid-header and mid-frame EOF based on whether a complete length header is present. Thesaturating_subprevents underflow.
227-236: LGTM!The encoder now returns structured
CodecError::Framing(FramingError::OversizedFrame)instead of a genericio::Error, enabling proper recovery policy selection.
264-265: LGTM!Tests extracted to a separate module keeps the main file under the 400-line limit and improves organisation.
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md (4)
1-68: LGTM!The ExecPlan structure is well-organised with clear purpose, progress tracking, and decision log. The three-tier error taxonomy plus EOF distinction is well-reasoned.
116-139: LGTM!The plan of work clearly defines the error taxonomy with appropriate recovery policy mappings for each category.
246-325: LGTM!Concrete steps are actionable and well-sequenced. The validation gate commands are comprehensive.
348-364: LGTM!Acceptance criteria are observable and measurable, covering error taxonomy, recovery policies, logging, metrics, and test gates.
tests/worlds/codec_error/mod.rs (6)
1-6: LGTM!Module-level documentation explains the purpose. The
#![cfg(not(loom))]attribute correctly excludes this module from loom testing.
16-44: LGTM!The enum definitions are clean with appropriate
#[derive]attributes. Using#[default]on specific variants makes intent explicit.
46-67: LGTM!The
CodecErrorWorldstruct appropriately usespub(crate)visibility for fields accessed by sibling modules (likedecoder_ops) while keeping internal state private.
69-104: LGTM!The setter methods follow the pattern of matching string input to enum variants and rebuilding the error. Error messages are clear and include the invalid input.
122-164: LGTM!The error builder methods are well-structured and use representative test values. The
build_eof_errormethod matches the pattern in the relevant code snippet fromsrc/tests/worlds/codec_error/mod.rs.
166-191: LGTM!The
verify_recovery_policymethod correctly usesok_orfor theOptioncheck and provides clear error messages for policy mismatches.tests/worlds/codec_error/decoder_ops.rs (8)
1-4: LGTM!Module-level documentation clearly describes the purpose of the module.
15-33: LGTM!The reset and setup methods provide clean state management for test scenarios.
35-54: LGTM!Frame sending helpers correctly use the codec's encoder. The partial frame helper writes an explicit big-endian length prefix with a clear comment explaining the expected payload size.
56-85: LGTM!The
decode_eof_clean_closemethod correctly drains complete frames before checking EOF behaviour. The match arms handle all possible outcomes appropriately.
87-99: LGTM!The
#[expect]attribute is correctly scoped with appropriate reason. The safe accessor chain (.get(),.and_then(),.map_or()) follows the learnings about avoiding direct indexing.
133-151: LGTM!The
decode_eof_with_partial_datamethod correctly captures the error and classifies it. Error paths return appropriate test failures.
153-172: LGTM!The oversized frame test helper correctly expects an error and captures it for verification.
174-219: LGTM!Verification helpers provide clear assertions with descriptive error messages. The
verify_oversized_errorcorrectly checks forInvalidDatakind which is the mapped error kind forOversizedFrame.src/codec/recovery.rs (7)
1-36: LGTM!Comprehensive module documentation with clear examples demonstrating custom recovery hook implementation. The en-GB spelling ("customising") is correct.
42-91: LGTM!The
RecoveryPolicyenum is well-documented with clear guidance on when to use each variant. The#[default]attribute onDropaligns with the recoverable-by-default principle.
93-162: LGTM!The
CodecErrorContextstruct provides a clean builder pattern with#[must_use]on all builder methods. Public fields allow direct access for structured logging while builders enable fluent construction.
164-226: LGTM!The
RecoveryPolicyHooktrait provides sensible defaults while allowing full customisation. TheSend + Syncbounds ensure thread safety for async contexts. The example demonstrates a realistic use case.
228-235: LGTM!The
DefaultRecoveryPolicystruct correctly implements the trait with an empty impl block, inheriting all default behaviour.
237-307: LGTM!The
RecoveryConfigstruct provides sensible defaults (10 consecutive drops, 30s quarantine, logging enabled) and a fluent builder API. Documentation clearly explains each field's purpose and default value.
309-389: LGTM!Tests cover:
- Default policy value
- Context builder methods
- Peer address setting
- Policy delegation from DefaultRecoveryPolicy
- Quarantine duration default
- RecoveryConfig builder and defaults
The tests use
.expect()appropriately for test code per guidelines.
- Converted multiple individual tests in `src/codec/error_tests.rs` to parameterized rstest tests, improving test readability and maintenance. - Added parameterized tests for recovery policy defaults, io::Error conversions, and error type category strings. - Updated clean EOF testing functions to clarify behaviours. - No behaviour changes; all tests pass successfully. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
changed the title
|
@coderabbitai Have the following now been resolved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1257 🏁 Script executed: Length of output: 5083 🏁 Script executed: Length of output: 192 🏁 Script executed: Length of output: 4575 🏁 Script executed: Length of output: 1853 🏁 Script executed: Length of output: 165 @leynos: Yes, all the issues from the code review have been resolved! Here's the status of each: ✅ Comment 1 -
|
1 participant
Summary
Changes
Why this is needed
Testing plan
Backwards compatibility and migrations
How to test locally
🌿 Generated by Terry
ℹ️ Tag @terragon-labs to ask questions and address PR feedback
📎 Task: https://www.terragon-labs.com/task/58f26ea3-eb37-43d9-8805-f2bb4680cc01
Summary by Sourcery
Introduce a structured codec error taxonomy with recovery policies, EOF handling, and observability, and surface these errors consistently through public APIs.
New Features:
Enhancements:
Documentation:
Tests:
📎 Task: https://www.terragon-labs.com/task/58f26ea3-eb37-43d9-8805-f2bb4680cc01
📎 Task: https://www.terragon-labs.com/task/58f26ea3-eb37-43d9-8805-f2bb4680cc01
📎 Task: https://www.terragonlabs.com/task/58f26ea3-eb37-43d9-8805-f2bb4680cc01