From 9b139607924a3eaf679b2946a4db5e19de25fe3a Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 26 Jul 2025 00:08:36 +0100 Subject: [PATCH 1/5] Add client run function and socket communication --- Cargo.lock | 67 +++++++++++++++-- Cargo.toml | 1 + crates/comenq/Cargo.toml | 1 + crates/comenq/src/lib.rs | 117 ++++++++++++++++++++++++++++- crates/comenq/src/main.rs | 15 ++-- docs/comenq-design.md | 6 ++ docs/roadmap.md | 6 +- tests/cucumber.rs | 5 +- tests/features/client_main.feature | 11 +++ tests/steps/client_main_steps.rs | 82 ++++++++++++++++++++ tests/steps/mod.rs | 2 + 11 files changed, 294 insertions(+), 19 deletions(-) create mode 100644 tests/features/client_main.feature create mode 100644 tests/steps/client_main_steps.rs diff --git a/Cargo.lock b/Cargo.lock index dd559d8..35701d0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -264,6 +264,7 @@ dependencies = [ "comenq-lib", "serde", "serde_json", + "thiserror 1.0.69", "tokio", ] @@ -276,6 +277,7 @@ dependencies = [ "cucumber", "serde", "serde_json", + "tempfile", "tokio", ] @@ -478,6 +480,12 @@ dependencies = [ "windows-sys 0.60.2", ] +[[package]] +name = "fastrand" +version = "2.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" + [[package]] name = "filetime" version = "0.2.25" @@ -612,10 +620,22 @@ dependencies = [ "cfg-if", "js-sys", "libc", - "wasi", + "wasi 0.11.1+wasi-snapshot-preview1", "wasm-bindgen", ] +[[package]] +name = "getrandom" +version = "0.3.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "26145e563e54f2cadc477553f1ec5ee650b00862f0a58bcd12cbdc5f0ea2d2f4" +dependencies = [ + "cfg-if", + "libc", + "r-efi", + "wasi 0.14.2+wasi-0.2.4", +] + [[package]] name = "gherkin" version = "0.14.0" @@ -1180,7 +1200,7 @@ checksum = "a4a650543ca06a924e8b371db273b2756685faae30f8487da1b56505a8f78b0c" dependencies = [ "libc", "log", - "wasi", + "wasi 0.11.1+wasi-snapshot-preview1", "windows-sys 0.48.0", ] @@ -1191,7 +1211,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "78bed444cc8a2160f01cbcf811ef18cac863ad68ae8ca62092e8db51d51c761c" dependencies = [ "libc", - "wasi", + "wasi 0.11.1+wasi-snapshot-preview1", "windows-sys 0.59.0", ] @@ -1499,6 +1519,12 @@ dependencies = [ "proc-macro2", ] +[[package]] +name = "r-efi" +version = "5.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "69cdb34c158ceb288df11e18b4bd39de994f6657d83847bdffdbd7f346754b0f" + [[package]] name = "rand" version = "0.8.5" @@ -1526,7 +1552,7 @@ version = "0.6.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ec0be4795e2f6a28069bec0b5ff3e2ac9bafc99e6a9a7dc3547996c5c816922c" dependencies = [ - "getrandom", + "getrandom 0.2.16", ] [[package]] @@ -1596,7 +1622,7 @@ checksum = "a4689e6c2294d81e88dc6261c768b63bc4fcdb852be6d1352498b114f61383b7" dependencies = [ "cc", "cfg-if", - "getrandom", + "getrandom 0.2.16", "libc", "untrusted", "windows-sys 0.52.0", @@ -2010,6 +2036,19 @@ dependencies = [ "winapi", ] +[[package]] +name = "tempfile" +version = "3.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e8a64e3985349f2441a1a9ef0b853f869006c3855f2cda6862a94d26ebb9d6a1" +dependencies = [ + "fastrand", + "getrandom 0.3.3", + "once_cell", + "rustix", + "windows-sys 0.59.0", +] + [[package]] name = "terminal_size" version = "0.4.2" @@ -2392,6 +2431,15 @@ version = "0.11.1+wasi-snapshot-preview1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ccf3ec651a847eb01de73ccad15eb7d99f80485de043efb2f370cd654f4ea44b" +[[package]] +name = "wasi" +version = "0.14.2+wasi-0.2.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9683f9a5a998d873c0d21fcbe3c083009670149a8fab228644b8bd36b2c48cb3" +dependencies = [ + "wit-bindgen-rt", +] + [[package]] name = "wasm-bindgen" version = "0.2.100" @@ -2827,6 +2875,15 @@ version = "0.53.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "271414315aff87387382ec3d271b52d7ae78726f5d44ac98b4f4030c91880486" +[[package]] +name = "wit-bindgen-rt" +version = "0.39.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6f42320e61fe2cfd34354ecb597f86f413484a798ba44a8ca1165c58d42da6c1" +dependencies = [ + "bitflags 2.9.1", +] + [[package]] name = "writeable" version = "0.6.1" diff --git a/Cargo.toml b/Cargo.toml index f22d734..fa1663a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,7 @@ cucumber = "0.20" tokio = { workspace = true } clap = { workspace = true } comenq = { path = "crates/comenq" } +tempfile = "3" [[test]] name = "cucumber" diff --git a/crates/comenq/Cargo.toml b/crates/comenq/Cargo.toml index dcba796..7f88890 100644 --- a/crates/comenq/Cargo.toml +++ b/crates/comenq/Cargo.toml @@ -12,3 +12,4 @@ clap = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } comenq-lib = { path = "../.." } +thiserror = { workspace = true } diff --git a/crates/comenq/src/lib.rs b/crates/comenq/src/lib.rs index 35a20e7..472ba1c 100644 --- a/crates/comenq/src/lib.rs +++ b/crates/comenq/src/lib.rs @@ -1,10 +1,14 @@ //! Library utilities for the `comenq` CLI. use clap::Parser; +use comenq_lib::CommentRequest; use std::path::PathBuf; +use thiserror::Error; +use tokio::io::AsyncWriteExt; +use tokio::net::UnixStream; /// Command line arguments for the `comenq` client. -#[derive(Debug, Parser)] +#[derive(Debug, Clone, Parser)] #[command(name = "comenq", about = "Enqueue a GitHub PR comment")] pub struct Args { /// The repository in 'owner/repo' format (e.g., "rust-lang/rust"). @@ -31,11 +35,76 @@ fn validate_repo_slug(s: &str) -> Result { } } +/// Errors that can occur when interacting with the daemon. +#[derive(Debug, Error)] +pub enum ClientError { + /// Connecting to the daemon failed. + #[error("failed to connect to daemon: {0}")] + Connect(#[from] std::io::Error), + /// Serialising the request failed. + #[error("failed to serialise request: {0}")] + Serialise(#[from] serde_json::Error), + /// Writing the request to the socket failed. + #[error("failed to write to daemon: {0}")] + Write(#[source] std::io::Error), +} + +/// Send a `CommentRequest` to the daemon. +/// +/// # Examples +/// +/// ```no_run +/// # use comenq::{Args, run}; +/// # use std::path::PathBuf; +/// # async fn try_run() -> Result<(), comenq::ClientError> { +/// let args = Args { +/// repo_slug: "owner/repo".into(), +/// pr_number: 1, +/// comment_body: String::from("Hi"), +/// socket: PathBuf::from("/run/comenq/socket"), +/// }; +/// run(args).await?; +/// # Ok(()) +/// # } +/// ``` +pub async fn run(args: Args) -> Result<(), ClientError> { + let (owner, repo) = parse_slug(&args.repo_slug); + let request = CommentRequest { + owner, + repo, + pr_number: args.pr_number, + body: args.comment_body, + }; + + let payload = serde_json::to_vec(&request)?; + + let mut stream = UnixStream::connect(&args.socket) + .await + .map_err(ClientError::Connect)?; + stream + .write_all(&payload) + .await + .map_err(ClientError::Write)?; + let _ = stream.shutdown().await; + Ok(()) +} + +fn parse_slug(slug: &str) -> (String, String) { + let mut parts = slug.splitn(2, '/'); + let owner = parts.next().unwrap_or_default().to_owned(); + let repo = parts.next().unwrap_or_default().to_owned(); + (owner, repo) +} + #[cfg(test)] mod tests { - use super::Args; + use super::{Args, ClientError, run}; use clap::Parser; + use comenq_lib::CommentRequest; use rstest::rstest; + use tempfile::tempdir; + use tokio::io::AsyncReadExt; + use tokio::net::UnixListener; #[rstest] #[case("octocat/hello-world", 1, "Hi")] @@ -57,4 +126,48 @@ mod tests { let result = Args::try_parse_from(["comenq", slug, "1", "Hi"]); assert!(result.is_err()); } + + #[tokio::test] + async fn run_sends_request() { + let dir = tempdir().expect("temp dir"); + let socket = dir.path().join("sock"); + let listener = UnixListener::bind(&socket).expect("bind socket"); + + let accept = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.expect("accept"); + let mut buf = Vec::new(); + stream.read_to_end(&mut buf).await.expect("read"); + serde_json::from_slice::(&buf).expect("deserialize") + }); + + let args = Args { + repo_slug: "octocat/hello-world".into(), + pr_number: 1, + comment_body: "Hi".into(), + socket: socket.clone(), + }; + + run(args).await.expect("run succeeds"); + let req = accept.await.expect("join"); + assert_eq!(req.owner, "octocat"); + assert_eq!(req.repo, "hello-world"); + assert_eq!(req.pr_number, 1); + assert_eq!(req.body, "Hi"); + } + + #[tokio::test] + async fn run_errors_when_socket_missing() { + let dir = tempdir().expect("temp dir"); + let socket = dir.path().join("nosock"); + + let args = Args { + repo_slug: "octocat/hello-world".into(), + pr_number: 1, + comment_body: "Hi".into(), + socket: socket.clone(), + }; + + let err = run(args).await.expect_err("should error"); + assert!(matches!(err, ClientError::Connect(_))); + } } diff --git a/crates/comenq/src/main.rs b/crates/comenq/src/main.rs index 74dcd40..95ac29e 100644 --- a/crates/comenq/src/main.rs +++ b/crates/comenq/src/main.rs @@ -2,13 +2,14 @@ //! Parses user input and forwards it to the daemon. use clap::Parser; -use comenq::Args; +use comenq::{Args, run}; +use std::process; -fn main() { +#[tokio::main] +async fn main() { let args = Args::parse(); - todo!( - "Connect to daemon at {} to enqueue comment for {}", - args.socket.display(), - args.repo_slug - ); + if let Err(e) = run(args).await { + eprintln!("{e}"); + process::exit(1); + } } diff --git a/docs/comenq-design.md b/docs/comenq-design.md index 6e3f141..d483138 100644 --- a/docs/comenq-design.md +++ b/docs/comenq-design.md @@ -336,6 +336,12 @@ This client is a self-contained, robust utility. It provides clear error messages for common failure modes, such as an invalid repository slug or the inability to connect to the daemon, guiding the user toward a resolution. +The production code exposes a `run` function in the `comenq` crate. The binary +parses the CLI arguments and delegates to this function, enabling the test +suite to exercise the network logic directly. Any failures to serialise the +request or communicate with the daemon are surfaced via a small `ClientError` +enumeration. + ## Section 3: Design of the `comenqd` Daemon The `comenqd` daemon is the heart of the system. It is a stateful, diff --git a/docs/roadmap.md b/docs/roadmap.md index 4433652..3d81cb2 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -19,13 +19,13 @@ - [x] Add validation for the `owner/repo` slug format. -- [ ] Implement the client's `main` function to connect to the daemon's Unix +- [x] Implement the client's `main` function to connect to the daemon's Unix Domain Socket using `tokio::net::UnixStream`. -- [ ] Serialize the `CommentRequest` payload to JSON and write it to the socket +- [x] Serialize the `CommentRequest` payload to JSON and write it to the socket stream. -- [ ] Implement robust error handling and user feedback for connection failures +- [x] Implement robust error handling and user feedback for connection failures or serialization errors. ## Milestone 3: `comenqd` Daemon Core diff --git a/tests/cucumber.rs b/tests/cucumber.rs index 4c2fb07..1d165c0 100644 --- a/tests/cucumber.rs +++ b/tests/cucumber.rs @@ -1,11 +1,12 @@ mod steps; use cucumber::World as _; -use steps::{CliWorld, CommentWorld}; +use steps::{CliWorld, ClientWorld, CommentWorld}; #[tokio::main] async fn main() { tokio::join!( CommentWorld::run("tests/features/comment_request.feature"), - CliWorld::run("tests/features/cli.feature") + CliWorld::run("tests/features/cli.feature"), + ClientWorld::run("tests/features/client_main.feature") ); } diff --git a/tests/features/client_main.feature b/tests/features/client_main.feature new file mode 100644 index 0000000..9e52512 --- /dev/null +++ b/tests/features/client_main.feature @@ -0,0 +1,11 @@ +Feature: Client main function + + Scenario: sending a comment request + Given a dummy daemon listening on a socket + When the client sends the request + Then the daemon receives the request + + Scenario: connection failure + Given no daemon is listening on a socket + When the client sends the request + Then an error occurs diff --git a/tests/steps/client_main_steps.rs b/tests/steps/client_main_steps.rs new file mode 100644 index 0000000..15ffdf8 --- /dev/null +++ b/tests/steps/client_main_steps.rs @@ -0,0 +1,82 @@ +#![allow( + clippy::expect_used, + clippy::unwrap_used, + reason = "simplify test failure output" +)] + +use comenq::{Args, ClientError, run}; +use comenq_lib::CommentRequest; +use cucumber::{World, given, then, when}; +use tempfile::TempDir; +use tokio::io::AsyncReadExt; +use tokio::net::UnixListener; + +#[derive(Debug, Default, World)] +pub struct ClientWorld { + args: Option, + tempdir: Option, + server: Option>>, + result: Option>, +} + +#[given("a dummy daemon listening on a socket")] +fn dummy_daemon(world: &mut ClientWorld) { + let dir = TempDir::new().expect("tempdir"); + let socket = dir.path().join("sock"); + let listener = UnixListener::bind(&socket).expect("bind"); + + let handle = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.expect("accept"); + let mut buf = Vec::new(); + stream.read_to_end(&mut buf).await.expect("read"); + buf + }); + + world.args = Some(Args { + repo_slug: "octocat/hello-world".into(), + pr_number: 1, + comment_body: "Hi".into(), + socket: socket.clone(), + }); + world.tempdir = Some(dir); + world.server = Some(handle); +} + +#[given("no daemon is listening on a socket")] +fn no_daemon(world: &mut ClientWorld) { + let dir = TempDir::new().expect("tempdir"); + let socket = dir.path().join("sock"); + world.args = Some(Args { + repo_slug: "octocat/hello-world".into(), + pr_number: 1, + comment_body: "Hi".into(), + socket, + }); + world.tempdir = Some(dir); +} + +#[when("the client sends the request")] +async fn send_request(world: &mut ClientWorld) { + let args = world.args.clone().expect("args"); + world.result = Some(run(args).await); +} + +#[then("the daemon receives the request")] +async fn daemon_receives(world: &mut ClientWorld) { + let handle = world.server.take().expect("server handle"); + let data = handle.await.expect("join"); + let req: CommentRequest = serde_json::from_slice(&data).expect("parse"); + assert_eq!(req.owner, "octocat"); + assert_eq!(req.repo, "hello-world"); + assert_eq!(req.pr_number, 1); + assert_eq!(req.body, "Hi"); + assert!(world.result.take().unwrap().is_ok()); +} + +#[then("an error occurs")] +fn an_error_occurs(world: &mut ClientWorld) { + match world.result.take() { + Some(Err(ClientError::Connect(_))) => {} + other => panic!("unexpected result: {other:?}"), + } +} diff --git a/tests/steps/mod.rs b/tests/steps/mod.rs index 8fe5feb..42e4852 100644 --- a/tests/steps/mod.rs +++ b/tests/steps/mod.rs @@ -2,3 +2,5 @@ pub mod comment_steps; pub use comment_steps::CommentWorld; pub mod cli_steps; pub use cli_steps::CliWorld; +pub mod client_main_steps; +pub use client_main_steps::ClientWorld; From db4126876aaf64d8a8b2431af4330bc7b2b6aa59 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 26 Jul 2025 00:28:57 +0100 Subject: [PATCH 2/5] Move client run logic to module --- crates/comenq/src/client.rs | 137 +++++++++++++++++++++++++++++ crates/comenq/src/lib.rs | 119 ++----------------------- docs/comenq-design.md | 10 +-- docs/roadmap.md | 8 +- tests/features/client_main.feature | 3 + tests/steps/mod.rs | 4 +- 6 files changed, 156 insertions(+), 125 deletions(-) create mode 100644 crates/comenq/src/client.rs diff --git a/crates/comenq/src/client.rs b/crates/comenq/src/client.rs new file mode 100644 index 0000000..3225c40 --- /dev/null +++ b/crates/comenq/src/client.rs @@ -0,0 +1,137 @@ +//! Client-side communication with the `comenqd` daemon. +//! +//! This module contains the logic to serialise a comment request and send it to +//! the daemon over its Unix Domain Socket. It is separated from `lib.rs` so +//! that argument parsing remains focused and the network logic is easily +//! testable. + +use comenq_lib::CommentRequest; +use thiserror::Error; +use tokio::{io::AsyncWriteExt, net::UnixStream}; + +use crate::Args; + +/// Errors that can occur when interacting with the daemon. +#[derive(Debug, Error)] +pub enum ClientError { + /// Connecting to the daemon failed. + #[error("failed to connect to daemon: {0}")] + Connect(#[from] std::io::Error), + /// Serialising the request failed. + #[error("failed to serialise request: {0}")] + Serialise(#[from] serde_json::Error), + /// Writing the request to the socket failed. + #[error("failed to write to daemon: {0}")] + Write(#[source] std::io::Error), + /// Shutting down the socket failed. + #[error("failed to close connection: {0}")] + Shutdown(#[source] std::io::Error), +} + +/// Send a `CommentRequest` to the daemon. +/// +/// # Examples +/// +/// ```no_run +/// # use comenq::{Args, run}; +/// # use std::path::PathBuf; +/// # async fn try_run() -> Result<(), comenq::ClientError> { +/// let args = Args { +/// repo_slug: "owner/repo".into(), +/// pr_number: 1, +/// comment_body: String::from("Hi"), +/// socket: PathBuf::from("/run/comenq/socket"), +/// }; +/// run(args).await?; +/// # Ok(()) +/// # } +/// ``` +pub async fn run(args: Args) -> Result<(), ClientError> { + let (owner, repo) = parse_slug(&args.repo_slug); + let request = CommentRequest { + owner, + repo, + pr_number: args.pr_number, + body: args.comment_body, + }; + + let payload = serde_json::to_vec(&request)?; + + let mut stream = UnixStream::connect(&args.socket) + .await + .map_err(ClientError::Connect)?; + stream + .write_all(&payload) + .await + .map_err(ClientError::Write)?; + stream.shutdown().await.map_err(ClientError::Shutdown)?; + Ok(()) +} + +fn parse_slug(slug: &str) -> (String, String) { + // safe unwrap: `validate_repo_slug` ensures two non-empty parts + let (owner, repo) = slug.split_once('/').unwrap(); + (owner.to_owned(), repo.to_owned()) +} + +#[cfg(test)] +mod tests { + use super::{ClientError, parse_slug, run}; + use crate::Args; + use comenq_lib::CommentRequest; + use rstest::rstest; + use tempfile::tempdir; + use tokio::io::AsyncReadExt; + use tokio::net::UnixListener; + + #[tokio::test] + async fn run_sends_request() { + let dir = tempdir().expect("temp dir"); + let socket = dir.path().join("sock"); + let listener = UnixListener::bind(&socket).expect("bind socket"); + + let accept = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.expect("accept"); + let mut buf = Vec::new(); + stream.read_to_end(&mut buf).await.expect("read"); + serde_json::from_slice::(&buf).expect("deserialize") + }); + + let args = Args { + repo_slug: "octocat/hello-world".into(), + pr_number: 1, + comment_body: "Hi".into(), + socket: socket.clone(), + }; + + run(args).await.expect("run succeeds"); + let req = accept.await.expect("join"); + assert_eq!(req.owner, "octocat"); + assert_eq!(req.repo, "hello-world"); + assert_eq!(req.pr_number, 1); + assert_eq!(req.body, "Hi"); + } + + #[tokio::test] + async fn run_errors_when_socket_missing() { + let dir = tempdir().expect("temp dir"); + let socket = dir.path().join("nosock"); + + let args = Args { + repo_slug: "octocat/hello-world".into(), + pr_number: 1, + comment_body: "Hi".into(), + socket: socket.clone(), + }; + + let err = run(args).await.expect_err("should error"); + assert!(matches!(err, ClientError::Connect(_))); + } + + #[test] + fn slug_is_split() { + let (owner, repo) = parse_slug("octocat/hello-world"); + assert_eq!(owner, "octocat"); + assert_eq!(repo, "hello-world"); + } +} diff --git a/crates/comenq/src/lib.rs b/crates/comenq/src/lib.rs index 472ba1c..f575521 100644 --- a/crates/comenq/src/lib.rs +++ b/crates/comenq/src/lib.rs @@ -1,11 +1,11 @@ //! Library utilities for the `comenq` CLI. use clap::Parser; -use comenq_lib::CommentRequest; use std::path::PathBuf; -use thiserror::Error; -use tokio::io::AsyncWriteExt; -use tokio::net::UnixStream; + +mod client; + +pub use client::{ClientError, run}; /// Command line arguments for the `comenq` client. #[derive(Debug, Clone, Parser)] @@ -35,76 +35,11 @@ fn validate_repo_slug(s: &str) -> Result { } } -/// Errors that can occur when interacting with the daemon. -#[derive(Debug, Error)] -pub enum ClientError { - /// Connecting to the daemon failed. - #[error("failed to connect to daemon: {0}")] - Connect(#[from] std::io::Error), - /// Serialising the request failed. - #[error("failed to serialise request: {0}")] - Serialise(#[from] serde_json::Error), - /// Writing the request to the socket failed. - #[error("failed to write to daemon: {0}")] - Write(#[source] std::io::Error), -} - -/// Send a `CommentRequest` to the daemon. -/// -/// # Examples -/// -/// ```no_run -/// # use comenq::{Args, run}; -/// # use std::path::PathBuf; -/// # async fn try_run() -> Result<(), comenq::ClientError> { -/// let args = Args { -/// repo_slug: "owner/repo".into(), -/// pr_number: 1, -/// comment_body: String::from("Hi"), -/// socket: PathBuf::from("/run/comenq/socket"), -/// }; -/// run(args).await?; -/// # Ok(()) -/// # } -/// ``` -pub async fn run(args: Args) -> Result<(), ClientError> { - let (owner, repo) = parse_slug(&args.repo_slug); - let request = CommentRequest { - owner, - repo, - pr_number: args.pr_number, - body: args.comment_body, - }; - - let payload = serde_json::to_vec(&request)?; - - let mut stream = UnixStream::connect(&args.socket) - .await - .map_err(ClientError::Connect)?; - stream - .write_all(&payload) - .await - .map_err(ClientError::Write)?; - let _ = stream.shutdown().await; - Ok(()) -} - -fn parse_slug(slug: &str) -> (String, String) { - let mut parts = slug.splitn(2, '/'); - let owner = parts.next().unwrap_or_default().to_owned(); - let repo = parts.next().unwrap_or_default().to_owned(); - (owner, repo) -} - #[cfg(test)] mod tests { - use super::{Args, ClientError, run}; + use super::Args; use clap::Parser; - use comenq_lib::CommentRequest; use rstest::rstest; - use tempfile::tempdir; - use tokio::io::AsyncReadExt; - use tokio::net::UnixListener; #[rstest] #[case("octocat/hello-world", 1, "Hi")] @@ -126,48 +61,4 @@ mod tests { let result = Args::try_parse_from(["comenq", slug, "1", "Hi"]); assert!(result.is_err()); } - - #[tokio::test] - async fn run_sends_request() { - let dir = tempdir().expect("temp dir"); - let socket = dir.path().join("sock"); - let listener = UnixListener::bind(&socket).expect("bind socket"); - - let accept = tokio::spawn(async move { - let (mut stream, _) = listener.accept().await.expect("accept"); - let mut buf = Vec::new(); - stream.read_to_end(&mut buf).await.expect("read"); - serde_json::from_slice::(&buf).expect("deserialize") - }); - - let args = Args { - repo_slug: "octocat/hello-world".into(), - pr_number: 1, - comment_body: "Hi".into(), - socket: socket.clone(), - }; - - run(args).await.expect("run succeeds"); - let req = accept.await.expect("join"); - assert_eq!(req.owner, "octocat"); - assert_eq!(req.repo, "hello-world"); - assert_eq!(req.pr_number, 1); - assert_eq!(req.body, "Hi"); - } - - #[tokio::test] - async fn run_errors_when_socket_missing() { - let dir = tempdir().expect("temp dir"); - let socket = dir.path().join("nosock"); - - let args = Args { - repo_slug: "octocat/hello-world".into(), - pr_number: 1, - comment_body: "Hi".into(), - socket: socket.clone(), - }; - - let err = run(args).await.expect_err("should error"); - assert!(matches!(err, ClientError::Connect(_))); - } } diff --git a/docs/comenq-design.md b/docs/comenq-design.md index d483138..c088ee0 100644 --- a/docs/comenq-design.md +++ b/docs/comenq-design.md @@ -336,11 +336,11 @@ This client is a self-contained, robust utility. It provides clear error messages for common failure modes, such as an invalid repository slug or the inability to connect to the daemon, guiding the user toward a resolution. -The production code exposes a `run` function in the `comenq` crate. The binary -parses the CLI arguments and delegates to this function, enabling the test -suite to exercise the network logic directly. Any failures to serialise the -request or communicate with the daemon are surfaced via a small `ClientError` -enumeration. +The production code exposes a `run` function in the `comenq` crate. This logic +resides in a dedicated `client` module to keep the argument parser focused. The +binary parses the CLI arguments and delegates to `run`, allowing the test suite +to exercise the network code directly. Any failures to serialise the request or +communicate with the daemon are surfaced via a small `ClientError` enumeration. ## Section 3: Design of the `comenqd` Daemon diff --git a/docs/roadmap.md b/docs/roadmap.md index 3d81cb2..4624226 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -22,11 +22,11 @@ - [x] Implement the client's `main` function to connect to the daemon's Unix Domain Socket using `tokio::net::UnixStream`. -- [x] Serialize the `CommentRequest` payload to JSON and write it to the socket - stream. +- [x] Serialize the `CommentRequest` payload to JSON and write it to the + socket stream. -- [x] Implement robust error handling and user feedback for connection failures - or serialization errors. +- [x] Implement robust error handling and user feedback for connection + failures or serialization errors. ## Milestone 3: `comenqd` Daemon Core diff --git a/tests/features/client_main.feature b/tests/features/client_main.feature index 9e52512..0c40145 100644 --- a/tests/features/client_main.feature +++ b/tests/features/client_main.feature @@ -1,10 +1,13 @@ +@client_main Feature: Client main function + @happy_path Scenario: sending a comment request Given a dummy daemon listening on a socket When the client sends the request Then the daemon receives the request + @unhappy_path Scenario: connection failure Given no daemon is listening on a socket When the client sends the request diff --git a/tests/steps/mod.rs b/tests/steps/mod.rs index 42e4852..c9c6912 100644 --- a/tests/steps/mod.rs +++ b/tests/steps/mod.rs @@ -1,6 +1,6 @@ -pub mod comment_steps; -pub use comment_steps::CommentWorld; pub mod cli_steps; pub use cli_steps::CliWorld; pub mod client_main_steps; pub use client_main_steps::ClientWorld; +pub mod comment_steps; +pub use comment_steps::CommentWorld; From e6497c6b1b0b1f7c922f300151147835ddd853c9 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 26 Jul 2025 14:45:11 +0100 Subject: [PATCH 3/5] Move tempfile to workspace --- Cargo.toml | 3 ++- crates/comenq/src/client.rs | 10 +++++----- docs/comenq-design.md | 2 +- tests/steps/mod.rs | 4 ++-- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index fa1663a..8b88b60 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,7 +15,7 @@ cucumber = "0.20" tokio = { workspace = true } clap = { workspace = true } comenq = { path = "crates/comenq" } -tempfile = "3" +tempfile = { workspace = true } [[test]] name = "cucumber" @@ -39,6 +39,7 @@ tracing = "0.1" tracing-subscriber = { version = "0.3", features = ["env-filter"] } anyhow = "1.0" thiserror = "1.0" +tempfile = "3" [lints.clippy] pedantic = { level = "warn", priority = -1 } diff --git a/crates/comenq/src/client.rs b/crates/comenq/src/client.rs index 3225c40..e7df31b 100644 --- a/crates/comenq/src/client.rs +++ b/crates/comenq/src/client.rs @@ -17,9 +17,9 @@ pub enum ClientError { /// Connecting to the daemon failed. #[error("failed to connect to daemon: {0}")] Connect(#[from] std::io::Error), - /// Serialising the request failed. - #[error("failed to serialise request: {0}")] - Serialise(#[from] serde_json::Error), + /// Serializing the request failed. + #[error("failed to serialize request: {0}")] + Serialize(#[from] serde_json::Error), /// Writing the request to the socket failed. #[error("failed to write to daemon: {0}")] Write(#[source] std::io::Error), @@ -69,8 +69,8 @@ pub async fn run(args: Args) -> Result<(), ClientError> { } fn parse_slug(slug: &str) -> (String, String) { - // safe unwrap: `validate_repo_slug` ensures two non-empty parts - let (owner, repo) = slug.split_once('/').unwrap(); + // safe expect: `validate_repo_slug` ensures two non-empty parts + let (owner, repo) = slug.split_once('/').expect("slug already validated"); (owner.to_owned(), repo.to_owned()) } diff --git a/docs/comenq-design.md b/docs/comenq-design.md index c088ee0..ee96ef5 100644 --- a/docs/comenq-design.md +++ b/docs/comenq-design.md @@ -339,7 +339,7 @@ inability to connect to the daemon, guiding the user toward a resolution. The production code exposes a `run` function in the `comenq` crate. This logic resides in a dedicated `client` module to keep the argument parser focused. The binary parses the CLI arguments and delegates to `run`, allowing the test suite -to exercise the network code directly. Any failures to serialise the request or +to exercise the network code directly. Any failures to serialize the request or communicate with the daemon are surfaced via a small `ClientError` enumeration. ## Section 3: Design of the `comenqd` Daemon diff --git a/tests/steps/mod.rs b/tests/steps/mod.rs index c9c6912..918d9ae 100644 --- a/tests/steps/mod.rs +++ b/tests/steps/mod.rs @@ -1,6 +1,6 @@ -pub mod cli_steps; -pub use cli_steps::CliWorld; pub mod client_main_steps; pub use client_main_steps::ClientWorld; +pub mod cli_steps; +pub use cli_steps::CliWorld; pub mod comment_steps; pub use comment_steps::CommentWorld; From 058342cd2449e3acc777ab69fd3dc25303ffbe2b Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 26 Jul 2025 16:47:22 +0100 Subject: [PATCH 4/5] Validate slug before sending request --- crates/comenq/src/client.rs | 29 +++++++++++++++++++++++++++-- docs/comenq-design.md | 3 +++ tests/features/client_main.feature | 7 +++++++ tests/steps/client_main_steps.rs | 15 +++++++++++++++ 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/crates/comenq/src/client.rs b/crates/comenq/src/client.rs index e7df31b..5eb783b 100644 --- a/crates/comenq/src/client.rs +++ b/crates/comenq/src/client.rs @@ -1,6 +1,6 @@ //! Client-side communication with the `comenqd` daemon. //! -//! This module contains the logic to serialise a comment request and send it to +//! This module contains the logic to serialize a comment request and send it to //! the daemon over its Unix Domain Socket. It is separated from `lib.rs` so //! that argument parsing remains focused and the network logic is easily //! testable. @@ -20,6 +20,9 @@ pub enum ClientError { /// Serializing the request failed. #[error("failed to serialize request: {0}")] Serialize(#[from] serde_json::Error), + /// The repository slug was invalid. + #[error("invalid repository format")] + BadSlug, /// Writing the request to the socket failed. #[error("failed to write to daemon: {0}")] Write(#[source] std::io::Error), @@ -47,6 +50,9 @@ pub enum ClientError { /// # } /// ``` pub async fn run(args: Args) -> Result<(), ClientError> { + if crate::validate_repo_slug(&args.repo_slug).is_err() { + return Err(ClientError::BadSlug); + } let (owner, repo) = parse_slug(&args.repo_slug); let request = CommentRequest { owner, @@ -70,7 +76,9 @@ pub async fn run(args: Args) -> Result<(), ClientError> { fn parse_slug(slug: &str) -> (String, String) { // safe expect: `validate_repo_slug` ensures two non-empty parts - let (owner, repo) = slug.split_once('/').expect("slug already validated"); + let (owner, repo) = slug + .split_once('/') + .expect("slug should have been validated by validate_repo_slug"); (owner.to_owned(), repo.to_owned()) } @@ -128,6 +136,23 @@ mod tests { assert!(matches!(err, ClientError::Connect(_))); } + #[tokio::test] + async fn run_errors_on_bad_slug() { + let dir = tempdir().expect("temp dir"); + let socket = dir.path().join("sock"); + let _listener = UnixListener::bind(&socket).expect("bind socket"); + + let args = Args { + repo_slug: "badslug".into(), + pr_number: 1, + comment_body: "Hi".into(), + socket, + }; + + let err = run(args).await.expect_err("should error"); + assert!(matches!(err, ClientError::BadSlug)); + } + #[test] fn slug_is_split() { let (owner, repo) = parse_slug("octocat/hello-world"); diff --git a/docs/comenq-design.md b/docs/comenq-design.md index ee96ef5..01dded4 100644 --- a/docs/comenq-design.md +++ b/docs/comenq-design.md @@ -341,6 +341,9 @@ resides in a dedicated `client` module to keep the argument parser focused. The binary parses the CLI arguments and delegates to `run`, allowing the test suite to exercise the network code directly. Any failures to serialize the request or communicate with the daemon are surfaced via a small `ClientError` enumeration. +The `run` function also verifies the repository slug at runtime to guard +against misuse in other contexts. An invalid slug results in a `BadSlug` +variant, keeping the code panic free while still surfacing helpful errors. ## Section 3: Design of the `comenqd` Daemon diff --git a/tests/features/client_main.feature b/tests/features/client_main.feature index 0c40145..95f1d59 100644 --- a/tests/features/client_main.feature +++ b/tests/features/client_main.feature @@ -12,3 +12,10 @@ Feature: Client main function Given no daemon is listening on a socket When the client sends the request Then an error occurs + + @edge_case + Scenario: invalid repository slug + Given a dummy daemon listening on a socket + And the arguments contain an invalid slug + When the client sends the request + Then a slug error occurs diff --git a/tests/steps/client_main_steps.rs b/tests/steps/client_main_steps.rs index 15ffdf8..e074b76 100644 --- a/tests/steps/client_main_steps.rs +++ b/tests/steps/client_main_steps.rs @@ -80,3 +80,18 @@ fn an_error_occurs(world: &mut ClientWorld) { other => panic!("unexpected result: {other:?}"), } } + +#[given("the arguments contain an invalid slug")] +fn invalid_slug(world: &mut ClientWorld) { + if let Some(args) = &mut world.args { + args.repo_slug = "bad".into(); + } +} + +#[then("a slug error occurs")] +fn slug_error_occurs(world: &mut ClientWorld) { + match world.result.take() { + Some(Err(ClientError::BadSlug)) => {} + other => panic!("unexpected result: {other:?}"), + } +} From eacdf3d80c4f7f7736e581ae36e184e691c32783 Mon Sep 17 00:00:00 2001 From: Leynos Date: Sat, 26 Jul 2025 17:08:38 +0100 Subject: [PATCH 5/5] Log shutdown errors --- Cargo.lock | 1 + crates/comenq/Cargo.toml | 1 + crates/comenq/src/client.rs | 6 +++++- docs/comenq-design.md | 35 +++++++++++++++++++---------------- 4 files changed, 26 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 35701d0..af97bd2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -266,6 +266,7 @@ dependencies = [ "serde_json", "thiserror 1.0.69", "tokio", + "tracing", ] [[package]] diff --git a/crates/comenq/Cargo.toml b/crates/comenq/Cargo.toml index 7f88890..1781a6e 100644 --- a/crates/comenq/Cargo.toml +++ b/crates/comenq/Cargo.toml @@ -13,3 +13,4 @@ serde = { workspace = true } serde_json = { workspace = true } comenq-lib = { path = "../.." } thiserror = { workspace = true } +tracing = { workspace = true } diff --git a/crates/comenq/src/client.rs b/crates/comenq/src/client.rs index 5eb783b..2c80570 100644 --- a/crates/comenq/src/client.rs +++ b/crates/comenq/src/client.rs @@ -8,6 +8,7 @@ use comenq_lib::CommentRequest; use thiserror::Error; use tokio::{io::AsyncWriteExt, net::UnixStream}; +use tracing::warn; use crate::Args; @@ -70,7 +71,10 @@ pub async fn run(args: Args) -> Result<(), ClientError> { .write_all(&payload) .await .map_err(ClientError::Write)?; - stream.shutdown().await.map_err(ClientError::Shutdown)?; + if let Err(e) = stream.shutdown().await { + warn!("failed to close connection: {e}"); + return Err(ClientError::Shutdown(e)); + } Ok(()) } diff --git a/docs/comenq-design.md b/docs/comenq-design.md index 01dded4..43fb0ea 100644 --- a/docs/comenq-design.md +++ b/docs/comenq-design.md @@ -238,6 +238,8 @@ use std::process; use tokio::io::AsyncWriteExt; use tokio::net::UnixStream; +use tracing::warn; + // Assume CommentRequest is in a shared library: `use comenq_lib::CommentRequest;` // For this example, we define it here. use serde::{Serialize, Deserialize}; @@ -277,13 +279,12 @@ async fn main() { let args = Args::parse(); // 2. Validate and parse the 'owner/repo' slug. - let parts: Vec<&str> = args.repo_slug.split('/').collect(); - if parts.len() != 2 || parts[0].is_empty() || parts[1].is_empty() { - eprintln!("Error: Invalid repository format. Please use 'owner/repo'."); - process::exit(1); - } - let owner = parts[0].to_string(); - let repo = parts[1].to_string(); + let (owner, repo) = args + .repo_slug + .split_once('/') + .expect("validated by clap value parser"); + let owner = owner.to_string(); + let repo = repo.to_string(); // Using a custom value parser keeps the error handling within `clap` // itself, providing immediate feedback if the slug is malformed. @@ -325,7 +326,7 @@ async fn main() { // 7. Gracefully shut down the write side of the stream. if let Err(e) = stream.shutdown().await { - eprintln!("Warning: Failed to gracefully shut down connection: {}", e); + warn!("failed to close connection: {e}"); } println!("Successfully enqueued comment for PR #{} on {}/{}.", request.pr_number, request.owner, request.repo); @@ -796,6 +797,7 @@ use std::process; use tokio::io::AsyncWriteExt; use tokio::net::UnixStream; use comenq_lib::CommentRequest; // Using the shared library +use tracing::warn; # # @@ -814,13 +816,12 @@ struct Args { async fn main() { let args = Args::parse(); - let parts: Vec<&str> = args.repo_slug.split('/').collect(); - if parts.len() != 2 || parts[0].is_empty() || parts[1].is_empty() { - eprintln!("Error: Invalid repository format. Please use 'owner/repo'."); - process::exit(1); - } - let owner = parts[0].to_string(); - let repo = parts[1].to_string(); + let (owner, repo) = args + .repo_slug + .split_once('/') + .expect("validated by clap value parser"); + let owner = owner.to_string(); + let repo = repo.to_string(); let request = CommentRequest { owner, @@ -851,7 +852,9 @@ async fn main() { process::exit(1); } - stream.shutdown().await.ok(); // Ignore error on shutdown + if let Err(e) = stream.shutdown().await { + warn!("failed to close connection: {e}"); + } println!("Successfully enqueued comment for PR #{} on {}/{}.", request.pr_number, request.owner, request.repo); }