Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,11 @@ project:
- Use dashes (`-`) for list bullets.
- Use GitHub-flavoured Markdown footnotes (`[^1]`) for references and
footnotes.
- Exception: roadmap-shaped Markdown edited with `mapsplice` (for example
`docs/roadmap.md`) must use inline links instead, because the `mapsplice`
roadmap grammar rejects footnote references (`unsupported inline node
footnoteReference`). Cite references inline, either as a parenthetical
`(see [target](path))` or by linking an existing phrase.
Comment on lines +310 to +314

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Remove the comma before the essential “because” clauses.

Apply the same punctuation correction in both guidance documents.

  • AGENTS.md#L310-L314: change instead, because to instead because.
  • docs/documentation-style-guide.md#L65-L68: change footnotes, because to footnotes because.

Triage: [type:grammar]

As per coding guidelines, keep Markdown guidance consistent with the documented formatting rules. As per path instructions, grammar findings require a Triage: paragraph.

🧰 Tools
🪛 LanguageTool

[formatting] ~311-~311: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...admap.md) must use inline links instead, because the mapsplice` roadmap grammar rej...

(COMMA_BEFORE_BECAUSE)

📍 Affects 2 files
  • AGENTS.md#L310-L314 (this comment)
  • docs/documentation-style-guide.md#L65-L68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@AGENTS.md` around lines 310 - 314, Remove the comma before the essential
“because” clause in AGENTS.md lines 310-314, changing “instead, because” to
“instead because”; make the corresponding punctuation correction in
docs/documentation-style-guide.md lines 65-68, changing “footnotes, because” to
“footnotes because,” and preserve the required Triage paragraph.

Sources: Coding guidelines, Path instructions, Linters/SAST tools


## Additional tooling

Expand Down
4 changes: 4 additions & 0 deletions docs/documentation-style-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ consistent, and easy to maintain across projects.
- Do not wrap tables.
- Use GitHub-flavoured numeric footnotes referenced as `[^1]`.
- Footnotes must be numbered in order of appearance in the document.
- Exception: roadmap-shaped Markdown edited with `mapsplice` (for example
`docs/roadmap.md`) must cite references with inline links instead of
footnotes, because the `mapsplice` roadmap grammar rejects footnote
references.
- Caption every table, and caption every diagram.

## Standard document types
Expand Down
84 changes: 37 additions & 47 deletions docs/roadmap.md
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,12 @@ stream.

- [x] 6.3.1. Provide a helper (for example `Response::with_channel`) that
returns a bounded channel sender alongside a `Response::MultiPacket` so
handlers can opt into streaming ergonomically.[^adr-0001]
handlers can opt into streaming ergonomically (see
[ADR 0001](adr-001-multi-packet-streaming-response-api.md)).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reconcile inline links with the required footnote format

This converts the roadmap citation to an inline link, and the same change removes every footnote definition, even though repository instructions require GitHub-flavoured [^1] references. Since the stated parser constraint makes that representation impossible, document a scoped roadmap exception or extend the parser before converting the canonical roadmap; otherwise this file is knowingly left non-conformant.

AGENTS.md reference: AGENTS.md:L308-L309

Useful? React with 👍 / 👎.

- [x] 6.3.2. Update the multi-packet design documentation and user guide with
tuple return examples that explain initial-frame handling, back-pressure, and
graceful termination.[^adr-0001]
graceful termination (see
[ADR 0001](adr-001-multi-packet-streaming-response-api.md)).
- [x] 6.3.3. Add an example handler (or test fixture) demonstrating spawning a
background task that pushes frames through the returned sender while the
connection actor manages delivery.
Expand Down Expand Up @@ -258,7 +260,8 @@ into a single frame, making the process transparent to the application logic.

## 8. Streaming requests and shared message assembly

This phase implements the decisions from ADR 0002,[^adr-0002] adding
This phase implements the decisions from
[ADR 0002](adr-002-streaming-requests-and-shared-message-assembly.md), adding
first-class streaming request bodies, a generic message assembly abstraction,
and standardized per-connection memory budgets.

Expand Down Expand Up @@ -371,9 +374,9 @@ integration boundaries.
### 9.2. Fragment adaptor alignment

- [x] 9.2.1. Introduce a `FragmentAdapter` trait as described in the
fragmentation design.[^fragmentation-design] Fragmentation behaviour must
explicitly define duplicate handling, out-of-order policies, and ownership of
purge scheduling.
[fragmentation](generic-message-fragmentation-and-re-assembly-design.md)
design. Fragmentation behaviour must explicitly define duplicate handling,
out-of-order policies, and ownership of purge scheduling.
- [x] Make fragmentation opt-in by requiring explicit configuration on the
`WireframeApp` builder.
- [x] Expose a public purge API, so callers can drive timeout eviction.
Expand All @@ -389,7 +392,8 @@ integration boundaries.
### 9.3. Unified codec handling

- [x] 9.3.1. Unify codec handling between the app router and the `Connection`
actor.[^outbound-design]
actor (see
[outbound messaging design](asynchronous-outbound-messaging-design.md)).
- [x] Route app-level request and response handling through the
`FramePipeline` so fragmentation and metrics apply consistently.
- [x] Remove duplicate codec construction in `src/app/inbound_handler.rs`; the
Expand All @@ -398,8 +402,9 @@ integration boundaries.
fragmentation, sequential requests, disabled fragmentation).
- [x] Add BDD behavioural tests exercising the unified codec path.
- [x] Note: protocol hooks (`before_send`) are deferred to a follow-up
stage because `F::Frame` and `Envelope` types may
differ.[^streaming-design]
stage because `F::Frame` and `Envelope` types may differ; see the
[streaming responses](multi-packet-and-streaming-responses-design.md)
design.

### 9.4. Property-based codec tests

Expand All @@ -412,12 +417,15 @@ integration boundaries.
### 9.5. Serializer boundaries and protocol metadata

- [x] 9.5.1. Decouple message encoding from `bincode`-specific traits to
support alternative serializers.[^router-design][^adr-005]
support alternative serializers (see the
[binary router library design](rust-binary-router-library-design.md) and
[ADR 0005](adr-005-serializer-abstraction.md)).
- [x] Introduce a serializer-agnostic message trait or adaptor layer for
`Message` types.
- [x] Provide optional wire-rs or Serde bridges to reduce manual boilerplate.
- [x] Define how frame metadata is exposed to the deserialization context to
enable version negotiation.[^message-versioning]
enable version negotiation (see the
[message versioning design](message-versioning.md)).
- [x] Add migration guidance covering existing `bincode` users.

### 9.6. Codec performance benchmarks
Expand All @@ -436,7 +444,7 @@ integration boundaries.
invalid frames, including oversized payloads and correlation metadata.
- [x] 9.7.3. Introduce a test observability harness in `wireframe_testing` that
captures logs and metrics per test run for asserting codec failures and
recovery policies.[^adr-006]
recovery policies (see [ADR 0006](adr-006-test-observability.md)).
- [x] 9.7.4. Add regression tests backed by `wireframe_testing` for the
`CodecError`
taxonomy and recovery policy behaviours defined in 9.1.2. Requires 9.1.2.
Expand Down Expand Up @@ -614,26 +622,29 @@ Wireframe's protocol, framing, and message assembly layers.

### 15.2. Protocol contract decisions

Each decision below is analysed in the
[formal verification guide](formal-verification-methods-in-wireframe.md); the
cited section names the specific question it answers.

- [ ] 15.2.1. Support a determined set of length-prefix widths (either `1`,
`2`, `4`, and `8`, or the full `1..=8` range) and enforce them in
constructors, conversions, and tests; record the decision in an ADR. Requires
15.1.1. See the formal verification guide[^fv-guide] §"What widths does
Wireframe actually support for length prefixes?". Success criteria: an ADR
records the decision, constructors enforce the chosen set, and existing tests
cover rejected widths.
15.1.1. See §"What widths does Wireframe actually support for length
prefixes?". Success criteria: an ADR records the decision, constructors
enforce the chosen set, and existing tests cover rejected widths.
- [ ] 15.2.2. Treat `total_body_len` as either authoritative or advisory and
enforce or rename it consistently across the message assembly path; record
the decision in an ADR and add tests for both conforming and violating
inputs. Requires 15.1.1. See the formal verification guide[^fv-guide] §"Is
`total_body_len` authoritative or advisory?". Success criteria: an ADR
records the decision, runtime code enforces the chosen semantics, and tests
verify both conforming and violating inputs.
inputs. Requires 15.1.1. See §"Is `total_body_len` authoritative or
advisory?". Success criteria: an ADR records the decision, runtime code
enforces the chosen semantics, and tests verify both conforming and violating
inputs.
- [ ] 15.2.3. Publish named fairness and priority guarantees for
`ConnectionActor` and encode them as model properties for Stateright checks.
Requires 15.1.1. See the formal verification guide[^fv-guide] §"What fairness
guarantee does `ConnectionActor` actually make?". Success criteria: the
design document enumerates each guarantee as a named property that can be
referenced by Stateright model checks.
Requires 15.1.1. See §"What fairness guarantee does `ConnectionActor`
actually make?". Success criteria: the design document enumerates each
guarantee as a named property that can be referenced by Stateright model
checks.

### 15.3. Kani bounded model checks

Expand Down Expand Up @@ -801,7 +812,8 @@ ecosystem.
- [ ] 18.2.1. Implement a formal message versioning system to allow for
protocol evolution.
- [ ] 18.2.2. Ensure version negotiation can consume codec metadata without
leaking framing details into handlers.[^message-versioning]
leaking framing details into handlers (see the
[message versioning design](message-versioning.md)).

### 18.3. Security

Expand Down Expand Up @@ -832,25 +844,3 @@ and usability.
- [ ] 19.4.1. Ensure all public items have clear, useful documentation
examples.
- [ ] 19.4.2. Publish documentation to `docs.rs`.

[^adr-0001]: Refer to
[ADR 0001](adr-001-multi-packet-streaming-response-api.md).
[^adr-0002]: Refer to
[ADR 0002](adr-002-streaming-requests-and-shared-message-assembly.md).
[^fragmentation-design]: See
[fragmentation doc](generic-message-fragmentation-and-re-assembly-design.md).
[^outbound-design]: See
[outbound messaging design](asynchronous-outbound-messaging-design.md).
[^streaming-design]: See
[streaming responses design](multi-packet-and-streaming-responses-design.md).
[^router-design]: See
[rust-binary-router-library-design.md](rust-binary-router-library-design.md).
[^message-versioning]: See
[message-versioning.md](message-versioning.md).
[^adr-005]: See
[adr-005-serializer-abstraction.md](adr-005-serializer-abstraction.md).
[^adr-006]: See
[adr-006-test-observability.md](adr-006-test-observability.md).

[^fv-guide]: See
[formal-verification-methods-in-wireframe.md](formal-verification-methods-in-wireframe.md).
24 changes: 16 additions & 8 deletions src/client/tests/error_handling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,15 @@ use crate::{
///
/// This helper is used by tests that need to manipulate the server stream after connection,
/// such as sending invalid data or dropping the connection at specific times.
///
/// # Errors
/// Returns an error if the client fails to connect or the accept task cannot be joined.
async fn connect_with_server<S, F, C>(
configure_builder: F,
) -> (WireframeClient<S, RewindStream<TcpStream>, C>, TcpStream)
) -> Result<
(WireframeClient<S, RewindStream<TcpStream>, C>, TcpStream),
Box<dyn std::error::Error + Send + Sync>,
>
where
S: Serializer + Send + Sync + 'static,
F: FnOnce(WireframeClientBuilder) -> WireframeClientBuilder<S, (), C>,
Expand All @@ -41,10 +47,9 @@ where
let (addr, accept) = spawn_listener().await;
let client = configure_builder(WireframeClient::builder())
.connect(addr)
.await
.expect("connect client");
let server_stream = accept.await.expect("join accept task");
(client, server_stream)
.await?;
let server_stream = accept.await?;
Ok((client, server_stream))
}

#[tokio::test]
Expand Down Expand Up @@ -100,7 +105,8 @@ async fn error_callback_invoked_on_deserialize_error() {
}
})
})
.await;
.await
.expect("connect with server");

// Send invalid bincode data via the server stream
let mut framed = Framed::new(server_stream, LengthDelimitedCodec::new());
Expand Down Expand Up @@ -151,7 +157,8 @@ async fn error_callback_invoked_on_send_io_error() {
}
})
})
.await;
.await
.expect("connect with server");

// Drop the server side to cause a broken pipe on send
drop(server_stream);
Expand Down Expand Up @@ -201,7 +208,8 @@ async fn error_callback_invoked_on_serialize_error() {
}
})
})
.await;
.await
.expect("connect with server");

// Try to send - should fail with serialization error and invoke error hook
let result = client.send(&TestMessage(42)).await;
Expand Down
58 changes: 42 additions & 16 deletions src/client/tests/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,36 @@ pub type CountingHookClosure<T> =
Arc<dyn Fn(T) -> Pin<Box<dyn Future<Output = T> + Send>> + Send + Sync>;

/// Spawns a TCP listener and returns the address and a handle to accept a connection.
///

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ New issue: Code Duplication
The module contains 2 functions with similar structure: FailingSerializer.deserialize,FailingSerializer.serialize

Suppress

/// This helper feeds signature-locked helpers (used from `lifecycle.rs`), so it
/// keeps returning plain values and fails loudly via `let ... else { panic! }`
/// rather than the bare `expect`/`unwrap`/`unwrap_or_else` panics the test-only
/// lints forbid.
pub async fn spawn_listener() -> (SocketAddr, tokio::task::JoinHandle<TcpStream>) {
let listener = TcpListener::bind("127.0.0.1:0")
.await
.expect("bind listener");
let addr = listener.local_addr().expect("listener addr");
let Ok(listener) = TcpListener::bind("127.0.0.1:0").await else {
panic!("bind listener");
};
let Ok(addr) = listener.local_addr() else {
panic!("listener addr");
};
let accept = tokio::spawn(async move {
let (stream, _) = listener.accept().await.expect("accept client");
let Ok((stream, _)) = listener.accept().await else {
panic!("accept client");
};
stream
});
(addr, accept)
Comment on lines 32 to 45

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Return listener failures from the shared fixture.

Line 32 exports this helper for other client tests, but Lines 33-42 replace
fallible networking with panic!. Return a Result from spawn_listener,
including the accept task's inner result, then propagate it through callers.
This preserves the fallible contract already introduced in
assert_builder_option and connect_with_server.

As per coding guidelines, “In production code and shared fixtures, return
Result and propagate errors with ?; avoid .expect() and panics.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/client/tests/helpers.rs` around lines 32 - 45, Update the shared fixture
helper spawn_listener to return a Result instead of panicking on
TcpListener::bind, local_addr, or accept failures. Preserve the accepted stream
and address on success, make the spawned accept task expose its inner Result,
and propagate the revised result type through every caller using ? alongside the
existing fallible helpers assert_builder_option and connect_with_server.

Source: Coding guidelines

}

/// Helper function to test that a builder option is correctly applied to the TCP socket.
pub async fn assert_builder_option<F, A>(configure_builder: F, assert_option: A)
///
/// # Errors
/// Returns an error if the client fails to connect or the accept task cannot be
/// joined.
pub async fn assert_builder_option<F, A>(
configure_builder: F,
assert_option: A,
) -> Result<(), Box<dyn std::error::Error + Send + Sync>>
where
F: FnOnce(WireframeClientBuilder) -> WireframeClientBuilder,
A: FnOnce(&WireframeClient<BincodeSerializer, crate::rewind_stream::RewindStream<TcpStream>>),
Expand All @@ -46,12 +62,12 @@ where

let client = configure_builder(WireframeClient::builder())
.connect(addr)
.await
.expect("connect client");
.await?;

assert_option(&client);

let _server_stream = accept.await.expect("join accept task");
let _server_stream = accept.await?;
Ok(())
}

/// Helper function to test lifecycle hooks with a connected client.
Expand All @@ -66,12 +82,16 @@ where
C: Send + 'static,
{
let (addr, accept) = spawn_listener().await;
let client = configure_builder(WireframeClient::builder())
let Ok(client) = configure_builder(WireframeClient::builder())
.connect(addr)
.await
.expect("connect client");
else {
panic!("connect client");
};
// Server stream is dropped here, simulating a disconnected server.
let _server = accept.await.expect("join accept task");
let Ok(_server) = accept.await else {
panic!("join accept task");
};
client
}

Expand Down Expand Up @@ -112,13 +132,17 @@ where
let error_count = Arc::new(AtomicUsize::new(0));
let (addr, accept) = spawn_listener().await;

let mut client = configure_builder(WireframeClient::builder(), error_count.clone())
let Ok(mut client) = configure_builder(WireframeClient::builder(), error_count.clone())
.connect(addr)
.await
.expect("connect client");
else {
panic!("connect client");
};

// Drop the server side to cause a disconnection
let server = accept.await.expect("join accept task");
let Ok(server) = accept.await else {
panic!("join accept task");
};
drop(server);

// Try to receive - should fail and invoke error hook
Expand Down Expand Up @@ -161,7 +185,9 @@ macro_rules! socket_option_test {
($name:ident, $configure:expr, $assert:expr $(,)?) => {
#[tokio::test]
async fn $name() {
$crate::client::tests::helpers::assert_builder_option($configure, $assert).await;
$crate::client::tests::helpers::assert_builder_option($configure, $assert)
.await
.expect("assert builder option");
}
};
}
Expand Down
Loading