Conversation
Reviewer's GuideThis PR bolsters server stability by wrapping each connection task in a panic-safe boundary that logs peer addresses on failure, refactors and expands Cucumber-based behavioural tests with a PanicWorld helper API, updates the cucumber dependency and test harness config, and completes and clarifies panic-handling docs across multiple guides. Sequence diagram for panic handling in connection taskssequenceDiagram
participant Server
participant TaskTracker
participant ConnectionTask
participant Logger
Server->>TaskTracker: spawn connection task
TaskTracker->>ConnectionTask: run process_stream (wrapped in catch_unwind)
alt process_stream panics
ConnectionTask->>Logger: log panic with peer_addr
end
Class diagram for PanicWorld helper in Cucumber testsclassDiagram
class PanicWorld {
+setup_connection()
+trigger_panic()
+verify_panic_logged()
}
class StepDefinitions {
+Given()
+When()
+Then()
}
StepDefinitions --> PanicWorld : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughUpdate the dependency and test configuration in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ConnectionTask
participant Logger
Client->>Server: Connect
Server->>ConnectionTask: Spawn process_stream (wrapped in catch_unwind)
ConnectionTask->>ConnectionTask: Panic occurs
ConnectionTask->>Logger: Log panic info and peer address
ConnectionTask-->>Server: Task ends (panic caught, server continues)
Client->>Server: Connect again
Server->>ConnectionTask: Spawn process_stream (wrapped in catch_unwind)
ConnectionTask-->>Server: Task ends (panic caught, server continues)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- Move the cucumber dependency and related test harness entries to dev-dependencies so they aren’t compiled into production builds.
- In the
connection_panic_is_caughttest, assert on the captured panic log (e.g. using a log test harness) to verify that the peer address context is actually emitted. - Consider exposing a metrics counter or hook for caught panics in connection tasks to help monitor how often user callbacks are failing in production.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Move the cucumber dependency and related test harness entries to dev-dependencies so they aren’t compiled into production builds.
- In the `connection_panic_is_caught` test, assert on the captured panic log (e.g. using a log test harness) to verify that the peer address context is actually emitted.
- Consider exposing a metrics counter or hook for caught panics in connection tasks to help monitor how often user callbacks are failing in production.
## Individual Comments
### Comment 1
<location> `src/server.rs:383` </location>
<code_context>
+ .await
+ {
+ tracing::error!(
+ ?panic,
+ ?peer_addr,
+ "connection task panicked"
+ );
+ }
</code_context>
<issue_to_address>
Logging the panic object directly may not always yield useful information.
Consider converting the panic payload to a string or formatting it for clearer log output, as logging it directly may be unclear if it's not a string.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
tracing::error!(
?panic,
?peer_addr,
"connection task panicked"
);
=======
let panic_msg = if let Some(s) = panic.downcast_ref::<&str>() {
s.to_string()
} else if let Some(s) = panic.downcast_ref::<String>() {
s.clone()
} else {
format!("{:?}", panic)
};
tracing::error!(
panic = %panic_msg,
?peer_addr,
"connection task panicked"
);
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `tests/cucumber.rs:1` </location>
<code_context>
+mod steps;
+mod world;
+
</code_context>
<issue_to_address>
Consider replacing the Cucumber harness with a simple async test using #[tokio::test] to reduce boilerplate and complexity.
Consider dropping the full Cucumber harness for a single‐scenario test and instead using a simple `#[tokio::test]` in your `tests/` directory. This removes the extra `world`/`steps` modules and boilerplate.
Example `tests/panic_integration.rs`:
```rust
use tokio::process::Command;
use assert_cmd::prelude::*;
#[tokio::test]
async fn panic_world_should_run_and_fail() -> Result<(), Box<dyn std::error::Error>> {
// Invokes the same binary as `cucumber::World::run`
let mut cmd = Command::cargo_bin("your_binary_name")?;
cmd.arg("--run-features").arg("tests/features");
let output = cmd.output().await?;
assert!(!output.status.success(), "expected panic scenario to fail");
let stdout = String::from_utf8_lossy(&output.stdout);
assert!(
stdout.contains("Expected panic message"),
"got stdout: {}",
stdout
);
Ok(())
}
```
Steps to migrate:
1. Remove `mod steps; mod world;` and the `main` entry in your repo root.
2. Add `assert_cmd = "2.0"` to your `[dev-dependencies]`.
3. Create `tests/panic_integration.rs` as above.
4. Adjust assertions to match your feature’s expected output.
This keeps the same behavior, but cuts out the Cucumber layer and its boilerplate.
</issue_to_address>
### Comment 3
<location> `tests/features/connection_panic.feature:1` </location>
<code_context>
+Feature: Connection panic resilience
+ Scenario: connection panic does not crash server
+ Given a running wireframe server with a panic in connection setup
</code_context>
<issue_to_address>
Consider replacing the BDD feature file with a focused async Rust test to streamline testing.
```markdown
It looks like this BDD feature is duplicating what you can achieve with a single async Rust test, and adds unnecessary indirection. Instead of keeping the `.feature` file, you can:
1. Remove the new `connection_panic_resilience.feature`
2. Add a focused `#[tokio::test]` in your Rust test suite
For example, in `tests/connection_panic_resilience.rs`:
```rust
use wireframe::ServerBuilder;
use tokio::net::TcpStream;
#[tokio::test]
async fn connection_panic_resilience() {
// Start the server with a panic in the first connection setup
let server = ServerBuilder::new()
.panic_on_setup(true)
.spawn()
.await;
// First connection should recover from the panic
let conn1 = TcpStream::connect(server.addr()).await;
assert!(conn1.is_ok(), "first connection failed after panic");
// Second connection should still succeed
let conn2 = TcpStream::connect(server.addr()).await;
assert!(conn2.is_ok(), "second connection failed");
}
```
This keeps all behavior in one place, removes the extra indirection, and keeps your test suite fully in Rust.
</issue_to_address>
### Comment 4
<location> `tests/world.rs:16` </location>
<code_context>
+use wireframe::{app::WireframeApp, server::WireframeServer};
+
+#[derive(Debug, Default, World)]
+pub struct PanicWorld {
+ pub addr: Option<SocketAddr>,
+ pub attempts: usize,
</code_context>
<issue_to_address>
Consider replacing the custom world state struct and Cucumber integration with a single self-contained async test function.
Here’s a way to collapse all of that “world” machinery into a single, self-contained `#[tokio::test]`, remove the global state entirely, and keep *exactly* the same panic-resilience check:
```rust
use std::net::SocketAddr;
use tokio::{
net::TcpStream,
sync::oneshot,
task,
};
use wireframe::{app::WireframeApp, server::WireframeServer};
async fn start_panic_server() -> (SocketAddr, oneshot::Sender<()>) {
// build a server that panics on each connection
let factory = || {
WireframeApp::new()
.expect("WireframeApp::new failed")
.on_connection_setup(|| async { panic!("boom") })
.expect("set callback failed")
};
let server = WireframeServer::new(factory)
.workers(1)
.bind("127.0.0.1:0".parse().unwrap())
.expect("bind failed");
let addr = server.local_addr().unwrap();
let (tx, rx) = oneshot::channel();
task::spawn(async move {
server
.run_with_shutdown(async { let _ = rx.await; })
.await
.expect("server task failed");
});
// give the task a tick to start up
task::yield_now().await;
(addr, tx)
}
#[tokio::test]
async fn panic_resilience() {
let (addr, shutdown) = start_panic_server().await;
// both connections should panic the server, but still accept
for _ in 0..2 {
TcpStream::connect(addr)
.await
.expect("connect failed");
}
// now shut it down gracefully
let _ = shutdown.send(());
task::yield_now().await;
}
```
– removes the `#[derive(World)]` + global struct
– keeps the same panic-in-accept loop & shutdown logic
– no more `Option` state or Cucumber overhead
</issue_to_address>
### Comment 5
<location> `docs/hardening-wireframe-a-guide-to-production-resilience.md:166` </location>
<code_context>
Initialization) pattern for safety.
+Connection tasks are wrapped with `catch_unwind` to log and discard panics.
+Each panicking connection is isolated so it cannot terminate the entire server.
+
### 3.2 Leak-Proof Registries with `Weak`/`Arc`
</code_context>
<issue_to_address>
This line is part of a paragraph that exceeds 80 columns and should be wrapped.
Ensure that this paragraph is wrapped to 80 columns for consistency with the documentation standards.
</issue_to_address>
### Comment 6
<location> `docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md:243` </location>
<code_context>
system's overall resilience.
+- **Panic Handling:** Connection tasks are wrapped in a panic handler using
+ `catch_unwind`. A misbehaving connection can no longer terminate the entire
+ server.
### B. First-Class Developer Ergonomics
</code_context>
<issue_to_address>
This line is part of a bullet point that is not wrapped to 80 columns.
Ensure the entire bullet point is wrapped to 80 columns for consistency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
tests/cucumber.rs (1)
1-9: Add required module-level documentation.Every module must begin with a
//!comment explaining the module's purpose and utility per the coding guidelines.Add this at the top of the file:
+//! Cucumber test runner for panic resilience integration tests. +//! +//! Runs behavioral tests defined in `tests/features/` using the +//! `PanicWorld` test context to verify server panic handling. + mod steps;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
Cargo.toml(2 hunks)docs/hardening-wireframe-a-guide-to-production-resilience.md(1 hunks)docs/roadmap.md(1 hunks)docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md(1 hunks)src/server.rs(3 hunks)tests/cucumber.rs(1 hunks)tests/features/connection_panic.feature(1 hunks)tests/steps/mod.rs(1 hunks)tests/steps/panic_steps.rs(1 hunks)tests/world.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Each file should encapsulate a coherent module. Group related code (e.g., models + utilities + fixtures) close together.
Group by feature, not layer. Colocate views, logic, fixtures, and helpers related to a domain concept rather than splitting by type.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Prefer .expect() over .unwrap().
Use predicate functions for conditional criteria with more than two branches.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Files:
tests/steps/mod.rstests/cucumber.rstests/steps/panic_steps.rssrc/server.rstests/world.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
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 / -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.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()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.
Files:
tests/steps/mod.rstests/cucumber.rstests/steps/panic_steps.rssrc/server.rstests/world.rs
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Reference: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Update: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in the docs/ directory to reflect the latest state. Ensure the documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).Conventions for writing project documentation should be followed as described in the documentation style guide.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation lists and sentences.
Company names are treated as collective nouns (e.g., "Lille Industries are expanding").
Write headings in sentence case.
Use Markdown headings (#,##,###, etc.) in order without skipping levels.
Follow markdownlint recommendations.
Provide code blocks and lists using standard Markdown syntax.
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text.
Use-as the first level bullet and renumber lists when items change.
Prefer inline links using[text](url)or angle brackets around the URL.
Ensure blank lines before and after bulleted lists and fenced blocks.
Ensure tables have a delimiter line below the header row.
Expand any uncommon acronym on first use, for example, Continuous Integration (CI).
Wrap paragraphs at 80 columns.
Wrap code at 120 columns.
Do not wrap tables.
Use footnotes referenced with[^label].
Include Mermaid diagrams where they add clarity in documentation.
When embedding figures, usean...
Files:
docs/hardening-wireframe-a-guide-to-production-resilience.mddocs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mddocs/roadmap.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
Use dashes (-) for list bullets.
Use GitHub-flavoured Markdown footnotes ([^1]) for references.
Files:
docs/hardening-wireframe-a-guide-to-production-resilience.mddocs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mddocs/roadmap.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/hardening-wireframe-a-guide-to-production-resilience.mddocs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mddocs/roadmap.md
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Use explicit version ranges in Cargo.toml and keep dependencies up-to-date.
Files:
Cargo.toml
🧠 Learnings (1)
tests/cucumber.rs (1)
Learnt from: CR
PR: leynos/netsuke#0
File: docs/behavioural-testing-in-rust-with-cucumber.md:0-0
Timestamp: 2025-07-20T02:48:36.265Z
Learning: Applies to docs/tests/cucumber.rs : The main test runner file (e.g., tests/cucumber.rs) must define an async fn main() annotated with #[tokio::main].
🔇 Additional comments (9)
Cargo.toml (2)
9-18: Avoid shipping test-only Tokio code in production buildsThe
test-utilfeature is now enabled under[dependencies]. This drags test helpers such assync::Barrierinto the release binary and increases compile times. Keep test-only features under[dev-dependencies]or behind an explicit crate feature.-[dependencies] -tokio = { version = "1", default-features = false, features = [ - "net", - "signal", - "rt-multi-thread", - "macros", - "sync", - "time", - "io-util", - "test-util", -] } +[dependencies] +tokio = { version = "1", default-features = false, features = [ + "net", + "signal", + "rt-multi-thread", + "macros", + "sync", + "time", + "io-util", +] } + +[dev-dependencies] +tokio = { version = "1", default-features = false, features = ["test-util"] }[ suggest_essential_refactor ]
36-38: Good: use an explicit, non-breaking version range for CucumberThe bounded range
>=0.20, <0.21satisfies the guideline on explicit versions while permitting compatible patch updates.docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md (1)
241-243: Wrap bullet text to 80 columnsThe added bullet exceeds the mandated line length. Re-wrap to satisfy markdownlint.
- - **Panic Handling:** Connection tasks are wrapped in a panic handler using - `catch_unwind`. A misbehaving connection can no longer terminate the entire - server. +- **Panic handling:** Connection tasks are wrapped in a panic handler using + `catch_unwind`. A misbehaving connection can no longer terminate the entire + server.⛔ Skipped due to learnings
Learnt from: CR PR: leynos/wireframe#0 File: docs/roadmap.md:0-0 Timestamp: 2025-07-21T13:21:45.715Z Learning: Applies to docs/src/connection.rs : Implement panic handlers in connection tasks to prevent a single connection from crashing the serverdocs/roadmap.md (1)
161-165: Maintain line-length discipline in roadmap bulletsWrap the updated checklist items at 80 columns to conform with the documentation style guide.
- - [x] Implement panic handlers in connection tasks to prevent a single - connection from crashing the server. + - [x] Implement panic handlers in connection tasks to prevent a single + connection from crashing the server.Likely an incorrect or invalid review comment.
tests/cucumber.rs (1)
8-8: Format single-line function correctly.The coding guidelines state "Do not use
returnin single-line functions." Apply proper formatting for the async main function.-async fn main() { PanicWorld::run("tests/features").await; } +async fn main() { + PanicWorld::run("tests/features").await; +}Likely an incorrect or invalid review comment.
tests/features/connection_panic.feature (1)
1-7: Well-structured behavioral test specification.The feature file clearly defines the panic resilience scenario with appropriate Given-When-Then steps that align with the PR objectives.
src/server.rs (2)
372-388: Excellent panic handling implementation.The panic catching logic is well-implemented with proper error context. Capturing the peer address before spawning the task provides valuable debugging information, and the use of
AssertUnwindSafewithcatch_unwindcorrectly prevents panics from crashing the server.
472-474: Leave test imports as-isTest-only imports for
TcpStreamandoneshotreside inside the#[cfg(test)]module (lines 459–460) and don’t affect non-test builds. No changes required.tests/steps/panic_steps.rs (1)
1-19: Well-implemented Cucumber step definitions.The step implementations are clean, properly documented, and follow the coding guidelines. The duplicate
#[when]attribute on line 14 correctly allows the same step to handle repeated connection attempts.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Cargo.toml (1)
45-48: Previous nitpick resolved – thank you.
The inline comment clarifies whyharness = falseis required, satisfying the earlier review.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
Cargo.toml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Use explicit version ranges in Cargo.toml and keep dependencies up-to-date.
Files:
Cargo.toml
🔇 Additional comments (1)
Cargo.toml (1)
36-37: LGTM – version range follows the guideline.
The explanatory comment is helpful and the<0.21upper bound guards against breaking changes.
|
@coderabbitai please create GitHub issues for the following, providing as much context as possible and potential solutions:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos I'll create GitHub issues for both enhancement requests with relevant context from the current implementation. Now I'll create the GitHub issues: Both GitHub issues have been created with comprehensive context from the current implementation and detailed potential solutions. The issues include:
Each issue includes implementation examples, benefits, and acceptance criteria to guide future development. 🏁 Script executed: Length of output: 2430 🏁 Script executed: Length of output: 5014 Perfect! I've successfully created both GitHub issues with comprehensive context and potential solutions: Created IssuesIssue #216: Enhance connection_panic_is_caught test to assert on panic log output
Issue #217: Add metrics/monitoring for caught panics in connection tasks
Both issues reference the current PR #215 and include relevant code snippets from the actual implementation. The issues provide actionable enhancement paths to improve both testing robustness and production observability of the panic handling feature. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/world.rs (1)
28-52: Fix server startup synchronisation.The server startup lacks proper synchronisation, creating potential race conditions in tests. Replace the implicit startup timing with an explicit ready signal.
Add a ready channel to ensure the server is listening before returning:
pub fn start_panic_server(&mut self) { + let (ready_tx, ready_rx) = oneshot::channel(); let factory = || { WireframeApp::new() .expect("Failed to create WireframeApp") .on_connection_setup(|| async { panic!("boom") }) .expect("Failed to set connection setup callback") }; let server = WireframeServer::new(factory) .workers(1) .bind("127.0.0.1:0".parse().expect("Failed to parse address")) .expect("bind"); self.addr = Some(server.local_addr().expect("Failed to get server address")); let (tx, rx) = oneshot::channel(); self.shutdown = Some(tx); - self.handle = Some(tokio::spawn(async move { + self.handle = Some(tokio::spawn(async move { + let _ = ready_tx.send(()); server .run_with_shutdown(async { let _ = rx.await; }) .await .expect("Server task failed"); })); + + // This should be awaited in an async version of this method + // Consider making this method async or adding a separate wait method }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)tests/steps/panic_steps.rs(1 hunks)tests/world.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
Use dashes (-) for list bullets.
Use GitHub-flavoured Markdown footnotes ([^1]) for references.
Files:
README.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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:
README.md
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Each file should encapsulate a coherent module. Group related code (e.g., models + utilities + fixtures) close together.
Group by feature, not layer. Colocate views, logic, fixtures, and helpers related to a domain concept rather than splitting by type.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Prefer .expect() over .unwrap().
Use predicate functions for conditional criteria with more than two branches.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Files:
tests/steps/panic_steps.rstests/world.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
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 / -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.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()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.
Files:
tests/steps/panic_steps.rstests/world.rs
🪛 LanguageTool
README.md
[grammar] ~87-~87: Please add a punctuation mark at the end of paragraph.
Context: ...a binary protocol server. See the [full example](docs/rust-binary-router-library-design...
(PUNCTUATION_PARAGRAPH_END)
🔇 Additional comments (8)
tests/steps/panic_steps.rs (3)
1-5: LGTM - Clear module documentation.The module documentation effectively explains the purpose and follows the required format.
6-8: LGTM - Clean imports.The imports are minimal and appropriate for the module's functionality.
16-21: LGTM - Well-structured step implementations.The step functions properly delegate to world methods and use appropriate Cucumber patterns.
tests/world.rs (5)
1-5: LGTM - Clear module documentation.The module documentation effectively explains the test world's purpose.
6-13: LGTM - Well-organised imports.The imports are appropriate and cleanly organised.
15-21: LGTM - Appropriate struct design.The
PanicWorldstruct correctly models the test state with appropriate optional fields.
54-63: LGTM - Well-implemented connection method.The method properly handles connections and tracks attempts with appropriate error handling.
65-78: LGTM - Proper verification and cleanup.The method correctly verifies connection attempts and performs proper server shutdown with task completion awaiting.
- add ready channel support in WireframeServer- synchronise PanicWorld server start- document cucumber modules and steps- clarify panic test assertions
leynos
force-pushed
the
26xd2p-codex/implement-advanced-error-handling
branch
from
3b3039a to
c4dbd74
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
README.md (1)
86-88: Inline the long-URL directive and finish the sentence.Keep the suppression comment on the same line as the long URL and close the paragraph with a full stop. This keeps markdownlint happy and removes the stray line break.
-binary protocol server. See the -[full example](docs/rust-binary-router-library-design.md#5-6-illustrative-api-usage-examples) - <!-- markdownlint-disable-line MD013 --> in the design document for further -details. +binary protocol server. See the [full example](docs/rust-binary-router-library-design.md#5-6-illustrative-api-usage-examples) <!-- markdownlint-disable-line MD013 --> in the design document for further details.Cargo.toml (1)
9-18: Repeat: pin Tokio and gatetest-utilbehindadvanced-tests.The earlier feedback remains unresolved. Specify an explicit semver range (e.g.,
>=1.37, <2) and movetest-utilto a test-only feature or[dev-dependencies]to avoid bloating release builds.src/server.rs (2)
387-406: Improve panic message extraction for clarity.The current implementation extracts panic messages but could benefit from cleaner formatting as suggested in the previous review.
908-944: Enhance test documentation and clarity.The test would benefit from the improvements suggested in the previous review, including better documentation and explicit assertions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
Cargo.toml(3 hunks)README.md(1 hunks)docs/hardening-wireframe-a-guide-to-production-resilience.md(1 hunks)docs/roadmap.md(1 hunks)docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md(1 hunks)src/server.rs(9 hunks)tests/cucumber.rs(1 hunks)tests/features/connection_panic.feature(1 hunks)tests/steps/mod.rs(1 hunks)tests/steps/panic_steps.rs(1 hunks)tests/world.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Reference: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
Update: When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in the docs/ directory to reflect the latest state. Ensure the documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar (with the exception of 'license' which is to be left unchanged for community consistency).Conventions for writing project documentation should be followed as described in the documentation style guide.
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-oxendict) for documentation.
The word "outwith" is acceptable in documentation.
Keep US spelling when used in an API, for examplecolor.
Use the Oxford comma in documentation lists and sentences.
Company names are treated as collective nouns (e.g., "Lille Industries are expanding").
Write headings in sentence case.
Use Markdown headings (#,##,###, etc.) in order without skipping levels.
Follow markdownlint recommendations.
Provide code blocks and lists using standard Markdown syntax.
Always provide a language identifier for fenced code blocks; useplaintextfor non-code text.
Use-as the first level bullet and renumber lists when items change.
Prefer inline links using[text](url)or angle brackets around the URL.
Ensure blank lines before and after bulleted lists and fenced blocks.
Ensure tables have a delimiter line below the header row.
Expand any uncommon acronym on first use, for example, Continuous Integration (CI).
Wrap paragraphs at 80 columns.
Wrap code at 120 columns.
Do not wrap tables.
Use footnotes referenced with[^label].
Include Mermaid diagrams where they add clarity in documentation.
When embedding figures, usean...
Files:
docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mddocs/hardening-wireframe-a-guide-to-production-resilience.mddocs/roadmap.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
Use dashes (-) for list bullets.
Use GitHub-flavoured Markdown footnotes ([^1]) for references.
Files:
docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mdREADME.mddocs/hardening-wireframe-a-guide-to-production-resilience.mddocs/roadmap.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- 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/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.mdREADME.mddocs/hardening-wireframe-a-guide-to-production-resilience.mddocs/roadmap.md
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Comment why, not what. Explain assumptions, edge cases, trade-offs, or complexity. Don't echo the obvious.
Comments must use en-GB-oxendict spelling and grammar.
Function documentation must include clear examples.
Name things precisely. Use clear, descriptive variable and function names. For booleans, prefer names with is, has, or should.
Each file should encapsulate a coherent module. Group related code (e.g., models + utilities + fixtures) close together.
Group by feature, not layer. Colocate views, logic, fixtures, and helpers related to a domain concept rather than splitting by type.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Place function attributes after doc comments.
Do not use return in single-line functions.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Clippy warnings MUST be disallowed.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Prefer .expect() over .unwrap().
Use predicate functions for conditional criteria with more than two branches.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Files:
tests/steps/mod.rstests/cucumber.rstests/steps/panic_steps.rstests/world.rssrc/server.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
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 / -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.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()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.
Files:
tests/steps/mod.rstests/cucumber.rstests/steps/panic_steps.rstests/world.rssrc/server.rs
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Use explicit version ranges in Cargo.toml and keep dependencies up-to-date.
Files:
Cargo.toml
🪛 LanguageTool
README.md
[grammar] ~87-~87: Please add a punctuation mark at the end of paragraph.
Context: ...a binary protocol server. See the [full example](docs/rust-binary-router-library-design...
(PUNCTUATION_PARAGRAPH_END)
🔇 Additional comments (13)
Cargo.toml (1)
51-56: Good explanatory comment.The note clearly justifies disabling the standard test harness.
No further action required.docs/hardening-wireframe-a-guide-to-production-resilience.md (1)
165-167: Panic-handling note looks good.The lines now respect the 80-column limit.
No further changes needed.docs/roadmap.md (1)
161-165: Roadmap tick-box update acknowledged.The completion status correctly reflects the new panic-handling work.
tests/steps/mod.rs (1)
1-7: Module structure and documentation look good.The module properly aggregates step definitions with clear documentation explaining its purpose. The module-level doc comment correctly describes the module's role in exposing Given-When-Then steps for behaviour-driven tests.
tests/cucumber.rs (1)
1-13: Cucumber test runner implementation is correct.The module provides clear documentation and a concise async entry point for running behavioural tests. The implementation correctly uses the
PanicWorldcontext for test execution.tests/features/connection_panic.feature (1)
1-7: Feature specification is clear and well-structured.The Gherkin scenario effectively describes the panic resilience behaviour being tested. The two connection attempts properly verify that the server remains operational after a panic.
tests/steps/panic_steps.rs (1)
1-18: Step definitions are well-implemented.The module provides clear documentation and concise step implementations. The reuse of the
connectfunction for both "I connect to the server" and "I connect to the server again" steps through multiple#[when]attributes is good practice.tests/world.rs (4)
1-22: Test world structure is well-designed.The
PanicWorldstruct properly encapsulates test state with appropriate use ofOptiontypes for lifecycle management. The module documentation clearly explains its purpose in managing behavioural test state.
23-56: Server startup implementation is robust.The method properly implements server startup with panic injection and includes proper synchronisation using the ready signal pattern. Error handling with descriptive messages aids debugging, and storing the task handle ensures proper cleanup.
58-67: Connection method is simple and effective.The implementation correctly handles connection attempts with proper error reporting and attempt tracking.
69-82: Shutdown logic properly ensures task completion.The method correctly validates connection attempts and implements graceful shutdown by awaiting the server task completion, which is more reliable than the previous
yield_nowapproach.src/server.rs (2)
196-202: Method implementation follows established patterns.The
ready_signalmethod correctly implements the builder pattern and provides clear documentation.
327-329: Ready signal correctly sent after listener setup.The implementation properly signals readiness after binding but before accepting connections.
| - **Panic Handling:** Connection tasks are wrapped in a panic handler using | ||
| `catch_unwind`. A misbehaving connection can no longer terminate the entire | ||
| server. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Hard-wrap the bullet to ≤ 80 columns.
Break the long sentence to satisfy the documentation wrap rule.
-- **Panic Handling:** Connection tasks are wrapped in a panic handler using
- `catch_unwind`. A misbehaving connection can no longer terminate the entire
- server.
+- **Panic Handling:** Connection tasks are wrapped in a panic handler using
+ `catch_unwind`. A misbehaving connection can no longer terminate the
+ entire server.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Panic Handling:** Connection tasks are wrapped in a panic handler using | |
| `catch_unwind`. A misbehaving connection can no longer terminate the entire | |
| server. | |
| - **Panic Handling:** Connection tasks are wrapped in a panic handler using | |
| `catch_unwind`. A misbehaving connection can no longer terminate the | |
| entire server. |
🤖 Prompt for AI Agents
In
docs/the-road-to-wireframe-1-0-feature-set-philosophy-and-capability-maturity.md
around lines 241 to 243, the bullet point about Panic Handling exceeds 80
columns. Reformat the text to hard-wrap the lines so that no line exceeds 80
characters, breaking the sentence appropriately while preserving its meaning.
| workers: usize, | ||
| on_preamble_success: Option<PreambleCallback<T>>, | ||
| on_preamble_failure: Option<PreambleErrorCallback>, | ||
| ready_tx: Option<oneshot::Sender<()>>, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add documentation for the ready_tx field.
Document the purpose of this field to maintain consistency with other struct fields.
+ /// Optional channel to signal when the server is ready to accept connections.
ready_tx: Option<oneshot::Sender<()>>,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ready_tx: Option<oneshot::Sender<()>>, | |
| /// Optional channel to signal when the server is ready to accept connections. | |
| ready_tx: Option<oneshot::Sender<()>>, |
🤖 Prompt for AI Agents
In src/server.rs at line 61, the field `ready_tx` lacks documentation. Add a
comment above this field explaining its purpose, such as indicating that it is
an optional oneshot sender used to signal readiness, to maintain consistency
with the documentation style of other struct fields.
|
Superseded |
1 participant
Summary
Testing
make fmtmake lintmake testmake markdownlintmake nixie(fails: ENOENT reading puppeteer-core)https://chatgpt.com/codex/tasks/task_e_6881736067a4832299a85aacc59fd724
Summary by Sourcery
Improve server resilience by wrapping connection tasks in panic handlers that log errors with peer addresses and prevent a single panic from terminating the server. Refactor panic test helpers and behavioural steps, add unit and cucumber tests to verify panic isolation, update dependencies, and complete related documentation.
New Features:
Enhancements:
Build:
Documentation:
Tests: