Conversation
Reviewer's GuideThis PR refactors the temporary configuration helper by introducing a parameterized Entity relationship diagram for Config usage in testserDiagram
TEMP_DIR ||--o{ CONFIG : creates
CONFIG {
string github_token
string socket_path
string queue_path
int cooldown_period_seconds
}
CONFIG ||--o{ DAEMON_TESTS : used_by
TEMP_DIR {
path path
}
Class diagram for temp_config and temp_config_with helpersclassDiagram
class TempDir
class Config {
github_token: String
socket_path: PathBuf
queue_path: PathBuf
cooldown_period_seconds: u64
}
class test_support {
+temp_config(dir: &TempDir) Config
+temp_config_with(dir: &TempDir, cooldown: u64) Config
}
TempDir <.. test_support : used by
test_support ..> Config : returns
test_support : temp_config_with is new
test_support : temp_config updated to use temp_config_with
Class diagram for env_guard module reexportsclassDiagram
class env_guard {
EnvVarGuard
remove_env_var()
set_env_var()
}
class support {
EnvVarGuard
remove_env_var()
}
env_guard <|-- support : reexports
support : set_env_var no longer reexported
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughUpdate the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Support as test-support
participant Daemon as Daemon Config
Test->>Support: temp_config_with(tmp, cooldown)
Support->>Daemon: Create Config with cooldown_period_seconds = cooldown
Support-->>Test: Return Config
Test->>Daemon: Use Config in tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit Inference Engine (AGENTS.md)
Files:
⚙️ CodeRabbit Configuration File
Files:
🔇 Additional comments (4)
✨ 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 (
|
![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 and found some issues that need to be addressed.
Blocking issues:
unsafeis used for setting an environment variable, which is not necessary here. (link)
General comments:
- Consider removing the redundant
cfg_with_cooldownhelper and usingtemp_config_withdirectly in daemon tests to keep configuration setup DRY. - Avoid using an unsafe
std::env::set_varin logging.rs; switch to a safe environment guard or wrapper to prevent potential race conditions or UB. - Review whether the
jsonfeature for tracing-subscriber needs to be enabled in release builds or could be scoped to dev-only to limit production dependencies.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider removing the redundant `cfg_with_cooldown` helper and using `temp_config_with` directly in daemon tests to keep configuration setup DRY.
- Avoid using an unsafe `std::env::set_var` in logging.rs; switch to a safe environment guard or wrapper to prevent potential race conditions or UB.
- Review whether the `json` feature for tracing-subscriber needs to be enabled in release builds or could be scoped to dev-only to limit production dependencies.
## Individual Comments
### Comment 1
<location> `crates/comenqd/src/logging.rs:69` </location>
<code_context>
#[test]
fn init_logging() {
let buf = Arc::new(Mutex::new(Vec::new()));
- std::env::set_var("RUST_LOG", "info");
+ unsafe { std::env::set_var("RUST_LOG", "info") };
init_with_writer(BufMakeWriter { buf: buf.clone() });
info!("captured");
</code_context>
<issue_to_address>
set_var is now called within an unsafe block, which is unnecessary.
std::env::set_var is safe and does not need to be called within an unsafe block. Please remove the unsafe block.
</issue_to_address>
### Comment 2
<location> `crates/comenqd/src/logging.rs:69` </location>
<code_context>
fn init_logging() {
let buf = Arc::new(Mutex::new(Vec::new()));
- std::env::set_var("RUST_LOG", "info");
+ unsafe { std::env::set_var("RUST_LOG", "info") };
init_with_writer(BufMakeWriter { buf: buf.clone() });
info!("captured");
</code_context>
<issue_to_address>
`unsafe` is used for setting an environment variable, which is not necessary here.
The use of `unsafe` here is not justified, as `std::env::set_var` is safe to call. Please remove the `unsafe` block.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| #[test] | ||
| fn init_logging() { | ||
| let buf = Arc::new(Mutex::new(Vec::new())); | ||
| std::env::set_var("RUST_LOG", "info"); |
There was a problem hiding this comment.
issue: set_var is now called within an unsafe block, which is unnecessary.
std::env::set_var is safe and does not need to be called within an unsafe block. Please remove the unsafe block.
| fn init_logging() { | ||
| let buf = Arc::new(Mutex::new(Vec::new())); | ||
| std::env::set_var("RUST_LOG", "info"); | ||
| unsafe { std::env::set_var("RUST_LOG", "info") }; |
There was a problem hiding this comment.
issue (review_instructions): unsafe is used for setting an environment variable, which is not necessary here.
The use of unsafe here is not justified, as std::env::set_var is safe to call. Please remove the unsafe block.
Review instructions:
Path patterns: **/*.rs
Instructions:
Avoid unsafe unless absolutely necessary.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
crates/comenqd/src/daemon.rs (1)
325-332: Drop superfluous local channel; reuse the one fromchannel()
handle_client_enqueues_requestcreates both a yaque channel and an unrelatedmpscpair only to forward bytes immediately. Spawnqueue_writerdirectly with the yaque sender to reduce resource usage and complexity.- let (sender, mut receiver) = channel(&queue_path).expect("channel"); - let (client_tx, writer_rx) = mpsc::unbounded_channel(); - let writer = tokio::spawn(queue_writer(sender, writer_rx)); + let (sender, mut receiver) = channel(&queue_path).expect("channel"); + let writer = tokio::spawn(queue_writer(sender, receiver_tx.clone()));Tighten the feedback loop and cut a whole channel hop.
📜 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(1 hunks)crates/comenqd/Cargo.toml(1 hunks)crates/comenqd/src/config.rs(1 hunks)crates/comenqd/src/daemon.rs(4 hunks)crates/comenqd/src/logging.rs(1 hunks)test-support/src/daemon.rs(1 hunks)test-support/src/lib.rs(1 hunks)tests/steps/listener_steps.rs(1 hunks)tests/steps/worker_steps.rs(2 hunks)tests/util/mod.rs(1 hunks)tests/util/test_helpers.rs(0 hunks)
💤 Files with no reviewable changes (1)
- tests/util/test_helpers.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
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 usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
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.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library. Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/steps/listener_steps.rscrates/comenqd/src/logging.rstests/util/mod.rstest-support/src/lib.rstests/steps/worker_steps.rscrates/comenqd/src/config.rstest-support/src/daemon.rscrates/comenqd/src/daemon.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/listener_steps.rscrates/comenqd/src/logging.rstests/util/mod.rstest-support/src/lib.rstests/steps/worker_steps.rscrates/comenqd/src/config.rstest-support/src/daemon.rscrates/comenqd/src/daemon.rs
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges inCargo.tomland keep dependencies up-to-date.
Mandate caret requirements for all dependencies: All crate versions specified inCargo.tomlmust use SemVer-compatible caret requirements (e.g.,some-crate = "1.2.3").
Prohibit unstable version specifiers: The use of wildcard (*) or open-ended inequality (>=) version requirements is strictly forbidden as they introduce unacceptable risk and unpredictability. Tilde requirements (~) should only be used where a dependency must be locked to patch-level updates for a specific, documented reason.
Files:
Cargo.toml
🔇 Additional comments (8)
Cargo.toml (1)
47-47: Good enhancement—jsonfeature added totracing-subscriberStructured logging support is now in place while retaining the existing
env-filterfunctionality. No further action required.tests/steps/listener_steps.rs (1)
40-40: LGTM! Helpful clarification.The comment clearly documents the implicit cooldown behaviour of
temp_config, improving test readability and maintainability.tests/util/mod.rs (1)
3-3: LGTM! Logical re-export addition.The re-export of
temp_config_withmaintains API consistency after the test helpers refactoring and follows the established pattern in this module.test-support/src/lib.rs (1)
6-6: LGTM! Consistent API extension.The re-export of
temp_config_withappropriately extends the public API of the test-support crate, maintaining consistency with the existing exports.tests/steps/worker_steps.rs (2)
10-10: LGTM! Clean import update.The import change correctly reflects the usage of
temp_config_within the updated test setup.
39-39: LGTM! Improved test clarity.The direct call to
temp_config_with(&dir, 0)with the explanatory comment is cleaner and more explicit than the previous approach of mutating a base config. The zero cooldown intent is now immediately clear.crates/comenqd/src/config.rs (1)
128-130: Drop the now-unusedset_env_varre-export or restore it consistently
pub use super::env_guard::{EnvVarGuard, remove_env_var};no longer re-exportsset_env_var, yet several test modules previously relied on it. Either:
- Remove all call-sites that still invoke
support::set_env_var, or- Re-export it here to avoid breakage.
Leaving the API half-changed will compile only if the old symbol is no longer referenced, masking accidental omissions. Run
cargo test --all --workspaceto ensure there are no hidden usages.crates/comenqd/src/daemon.rs (1)
299-306: Abort test after directory assertion to guarantee cleanup
run_creates_queue_directoryaborts the spawned daemon before asserting the directory exists, then asserts again afterwards. Swap the order to ensure the check happens while the task is still alive, or await the join handle for deterministic shutdown.Current ordering risks a race where the directory is created after
abort, causing a flaky failure.
| tempfile = "3.10" # latest 3.x at time of writing; update as new patch versions release | ||
| serial_test = "2" | ||
| test-support = { path = "../../test-support" } | ||
| wiremock = "0.6" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefix the version with an explicit caret to match dependency policy
The coding guidelines mandate explicit caret requirements. Cargo treats "0.6" as ^0.6, yet the caret should be written out for clarity and consistency with the rest of the manifest.
-wiremock = "0.6"
+wiremock = "^0.6"📝 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.
| wiremock = "0.6" | |
| -wiremock = "0.6" | |
| +wiremock = "^0.6" |
🤖 Prompt for AI Agents
In crates/comenqd/Cargo.toml at line 27, the dependency version for wiremock is
specified as "0.6" without an explicit caret. Update the version string to
"^0.6" to explicitly include the caret prefix, ensuring consistency with the
project's dependency versioning policy and clarity in the manifest.
| use tempfile::{TempDir, tempdir}; | ||
| use test_support::{octocrab_for, temp_config, temp_config_with, wait_for_file}; | ||
| use tokio::io::AsyncWriteExt; | ||
| use tokio::net::{UnixListener, UnixStream}; | ||
| use tokio::net::UnixStream; | ||
| use tokio::sync::{mpsc, watch}; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove unused TempDir import
TempDir is only referenced inside the now-redundant cfg_with_cooldown. Once that helper is dropped (see below) this import becomes dead code and will trigger a clippy warning.
🤖 Prompt for AI Agents
In crates/comenqd/src/daemon.rs around lines 242 to 246, the import of TempDir
is unused because it is only referenced in the cfg_with_cooldown helper which is
now removed. To fix this, remove the TempDir import from the use statement to
eliminate dead code and avoid clippy warnings.
| fn cfg_with_cooldown(dir: &TempDir, secs: u64) -> Config { | ||
| let other = temp_config_with(dir, secs); | ||
| Config { | ||
| cooldown_period_seconds: secs, | ||
| ..temp_config(dir) | ||
| github_token: other.github_token, | ||
| socket_path: other.socket_path, | ||
| queue_path: other.queue_path, | ||
| cooldown_period_seconds: other.cooldown_period_seconds, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Delete cfg_with_cooldown; call temp_config_with directly
cfg_with_cooldown is a thin wrapper that merely clones the output of temp_config_with, duplicating code and widening the API surface. Replace all invocations with temp_config_with and remove this function entirely.
🤖 Prompt for AI Agents
In crates/comenqd/src/daemon.rs around lines 250 to 258, the function
cfg_with_cooldown is redundant as it only clones the output of temp_config_with.
Remove the entire cfg_with_cooldown function and update all its call sites to
call temp_config_with directly instead, reducing code duplication and
simplifying the API.
There was a problem hiding this comment.
Has this now been resolved?
There was a problem hiding this comment.
@leynos No, this has not been resolved yet. The cfg_with_cooldown function is still present in the diff (lines 250-258). It's creating a Config by manually copying all fields from temp_config_with, which is the exact redundancy I flagged.
Remove the entire cfg_with_cooldown function and update all its call sites to call temp_config_with directly instead.
| let dir = tempdir().expect("tempdir"); | ||
| let cfg = Arc::new(cfg_with_cooldown(&dir, 1)); | ||
| let cfg = Arc::new(temp_config(&dir)); // Standard cooldown | ||
| let (sender, mut receiver) = channel(&cfg.queue_path).expect("channel"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use zero-cooldown configs in async tests to shorten wall-clock time
Tests such as run_listener_accepts_connections still rely on the one-second default. Supply temp_config_with(&dir, 0) to shave seconds off the suite without affecting behaviour.
🤖 Prompt for AI Agents
In crates/comenqd/src/daemon.rs around lines 348 to 350, the test uses
temp_config which applies a one-second cooldown, slowing down the test suite.
Replace temp_config(&dir) with temp_config_with(&dir, 0) to create a
zero-cooldown configuration, reducing test runtime without changing behavior.
There was a problem hiding this comment.
Has this now been resolved?
There was a problem hiding this comment.
🧩 Analysis chain
@leynos No, this has not been resolved. Looking at the current code in run_listener_accepts_connections, you're still using:
let cfg = Arc::new(temp_config(&dir)); // Standard cooldownThis still applies the default 1-second cooldown since temp_config delegates to temp_config_with(&dir, 1). To achieve the performance improvement, change it to:
let cfg = Arc::new(temp_config_with(&dir, 0)); // Zero cooldown for faster testsThe same optimization should be applied to other tests that don't specifically need to test cooldown behavior, such as run_creates_queue_directory which also uses temp_config(&dir).
🏁 Script executed:
#!/bin/bash
# Check the current implementation of the run_listener_accepts_connections test
ast-grep --pattern $'async fn run_listener_accepts_connections() {
$$$
}'
Length of output: 2338
@leynos No, this has not been resolved. The script confirms that line 340 in run_listener_accepts_connections still uses:
let cfg = Arc::new(temp_config(&dir)); // Standard cooldownSince temp_config(&dir) delegates to temp_config_with(&dir, 1), this test still applies a 1-second cooldown. The run_listener_accepts_connections test doesn't actually test cooldown behavior - it tests the listener's ability to accept connections and forward requests to the queue.
Change line 340 to:
let cfg = Arc::new(temp_config_with(&dir, 0)); // Zero cooldown for faster testsThis would eliminate the unnecessary 1-second wait and speed up the test without affecting its behavior or assertions.
🧬 Code Graph Analysis Results
Relevant snippets from test-support/src/daemon.rs (lines 62-71)
pub fn octocrab_for(server: &MockServer) -> Arc<Octocrab> {
Arc::new(
Octocrab::builder()
.personal_token("t".to_string())
.base_uri(server.uri())
.expect("base_uri")
.build()
.expect("build octocrab"),
)
}This function creates an Arc<Octocrab> client configured to use a mock server URI, useful for testing GitHub API interactions in the daemon worker.
Relevant snippets from test-support/src/daemon.rs (lines 28-30)
pub fn temp_config(tmp: &TempDir) -> Config {
temp_config_with(tmp, 1)
}This helper returns a default Config with a cooldown period of 1 second, used in tests.
Relevant snippets from test-support/src/daemon.rs (lines 49-56)
pub fn temp_config_with(tmp: &TempDir, cooldown_period_seconds: u64) -> Config {
Config {
github_token: "t".into(),
socket_path: tmp.path().join("sock"),
queue_path: tmp.path().join("q"),
cooldown_period_seconds,
}
}This helper returns a Config with a specified cooldown period, used to configure the daemon in tests.
Relevant snippets from test-support/src/util.rs (lines 22-30)
pub async fn wait_for_file(path: &Path, tries: u32, delay: Duration) -> bool {
for _ in 0..tries {
if path.exists() {
return true;
}
sleep(delay).await;
}
path.exists()
}This async utility waits for a file to appear on disk, retrying with a delay. Used in tests to wait for socket or queue directory creation.
Relevant snippets from crates/comenqd/src/daemon.rs (lines 230-260, test run_worker_commits_on_success)
#[tokio::test]
async fn run_worker_commits_on_success() {
let (server, cfg, rx, octo) = setup_run_worker(201).await;
let h = tokio::spawn(run_worker(cfg.clone(), rx, octo));
sleep(Duration::from_millis(50)).await;
h.abort();
assert_eq!(server.received_requests().await.unwrap().len(), 1);
assert_eq!(std::fs::read_dir(&cfg.queue_path).unwrap().count(), 0);
}This test verifies that the worker commits (removes) queue entries after successful GitHub API calls.
Relevant snippets from crates/comenqd/src/daemon.rs (lines 262-290, test run_worker_requeues_on_error)
#[tokio::test]
async fn run_worker_requeues_on_error() {
let (server, cfg, rx, octo) = setup_run_worker(500).await;
let h = tokio::spawn(run_worker(cfg.clone(), rx, octo));
sleep(Duration::from_millis(50)).await;
h.abort();
assert_eq!(server.received_requests().await.unwrap().len(), 1);
assert!(std::fs::read_dir(&cfg.queue_path).unwrap().count() > 0);
}This test verifies that the worker does not commit queue entries on GitHub API errors, leaving them for retry.
Relevant snippets from crates/comenqd/src/daemon.rs (lines 200-228, test run_listener_accepts_connections)
#[tokio::test]
async fn run_listener_accepts_connections() {
let dir = tempdir().expect("tempdir");
let cfg = Arc::new(temp_config(&dir)); // Standard cooldown
let (sender, mut receiver) = channel(&cfg.queue_path).expect("channel");
let (client_tx, writer_rx) = mpsc::unbounded_channel();
let (shutdown_tx, shutdown_rx) = watch::channel(());
let writer = tokio::spawn(queue_writer(sender, writer_rx));
let listener_task = tokio::spawn(run_listener(cfg.clone(), client_tx, shutdown_rx));
wait_for_file(&cfg.socket_path, 10, Duration::from_millis(10)).await;
let mut stream = UnixStream::connect(&cfg.socket_path)
.await
.expect("connect");
let req = CommentRequest {
owner: "o".into(),
repo: "r".into(),
pr_number: 1,
body: "b".into(),
};
let payload = serde_json::to_vec(&req).expect("serialize");
stream.write_all(&payload).await.expect("write");
stream.shutdown().await.expect("shutdown");
let guard = receiver.recv().await.expect("recv");
let stored: CommentRequest = serde_json::from_slice(&guard).expect("parse");
assert_eq!(stored, req);
listener_task.abort();
let _ = shutdown_tx.send(());
drop(writer);
}This test verifies that the listener accepts Unix socket connections, reads requests, and forwards them to the queue.
Relevant snippets from crates/comenqd/src/daemon.rs (lines 150-180, function run_worker)
pub async fn run_worker(
config: Arc<Config>,
mut rx: Receiver,
octocrab: Arc<Octocrab>,
) -> Result<()> {
loop {
let guard = rx.recv().await?;
let request: CommentRequest = serde_json::from_slice(&guard)?;
let issues = octocrab.issues(&request.owner, &request.repo);
let post = issues.create_comment(request.pr_number, &request.body);
match tokio::time::timeout(Duration::from_secs(10), post).await {
Ok(Ok(_)) => {
guard.commit()?;
}
Ok(Err(e)) => {
tracing::error!(
error = %e,
owner = %request.owner,
repo = %request.repo,
pr = request.pr_number,
"GitHub API call failed"
);
}
Err(_) => {
tracing::error!(
owner = %request.owner,
repo = %request.repo,
pr = request.pr_number,
"GitHub API call timed out"
);
}
}
tokio::time::sleep(Duration::from_secs(config.cooldown_period_seconds)).await;
}
}This function continuously dequeues requests and posts comments to GitHub, committing successful entries and retrying failed ones after a cooldown.
Relevant snippets from crates/comenqd/src/daemon.rs (lines 110-140, function run)
pub async fn run(config: Config) -> Result<()> {
ensure_queue_dir(&config.queue_path).await?;
tracing::info!(queue = %config.queue_path.display(), "Queue directory prepared");
let octocrab = Arc::new(build_octocrab(&config.github_token)?);
let (queue_tx, rx) = channel(&config.queue_path)?;
let (client_tx, client_rx) = mpsc::unbounded_channel();
let cfg = Arc::new(config);
let (shutdown_tx, shutdown_rx) = watch::channel(());
let writer = tokio::spawn(queue_writer(queue_tx, client_rx));
let listener = tokio::spawn(run_listener(cfg.clone(), client_tx, shutdown_rx));
let worker = tokio::spawn(run_worker(cfg.clone(), rx, octocrab));
tokio::select! {
res = listener => match res {
Ok(inner) => inner?,
Err(e) => return Err(e.into()),
},
res = worker => match res {
Ok(inner) => inner?,
Err(e) => return Err(e.into()),
},
}
let _ = shutdown_tx.send(());
writer.await??;
Ok(())
}This function starts the daemon by preparing the queue directory, creating the GitHub client, spawning the queue writer, listener, and worker tasks, and awaits their completion or errors.
Relevant snippets from crates/comenqd/src/daemon.rs (lines 180-200, function run_listener)
pub async fn run_listener(
config: Arc<Config>,
tx: mpsc::UnboundedSender<Vec<u8>>,
mut shutdown: watch::Receiver<()>,
) -> Result<()> {
let listener = prepare_listener(&config.socket_path)?;
loop {
tokio::select! {
res = listener.accept() => match res {
Ok((stream, _)) => {
let tx_clone = tx.clone();
tokio::spawn(async move {
if let Err(e) = handle_client(stream, tx_clone).await {
tracing::warn!(error = %e, "Client handling failed");
}
});
}
Err(e) => {
tracing::error!(error = %e, "Failed to accept client connection");
tokio::time::sleep(Duration::from_millis(100)).await;
}
},
_ = shutdown.changed() => {
break;
}
}
}
Ok(())
}This function listens on the Unix socket, spawning a handler for each client connection, and exits cleanly on shutdown signal.
Relevant snippets from crates/comenqd/src/daemon.rs (lines 140-150, function queue_writer)
pub async fn queue_writer(
mut sender: Sender,
mut rx: mpsc::UnboundedReceiver<Vec<u8>>,
) -> Result<()> {
while let Some(bytes) = rx.recv().await {
if let Err(e) = sender.send(bytes).await {
tracing::error!(error = %e, "Queue enqueue failed");
}
}
Ok(())
}This function forwards raw JSON payloads from client handlers into the persistent queue, logging errors but continuing operation.
These snippets provide comprehensive context on the daemon's operation, its testing, and the mechanisms for queueing and processing GitHub comment requests.
| fn init_logging() { | ||
| let buf = Arc::new(Mutex::new(Vec::new())); | ||
| std::env::set_var("RUST_LOG", "info"); | ||
| unsafe { std::env::set_var("RUST_LOG", "info") }; |
There was a problem hiding this comment.
Document the unsafe usage and consider safer alternatives.
The unsafe block lacks documentation explaining why it's necessary. Document the safety requirements clearly, as mandated by the coding guidelines.
Consider using a test-scoped environment variable approach instead of mutating global state:
- unsafe { std::env::set_var("RUST_LOG", "info") };
+ // SAFETY: Setting RUST_LOG in this test is safe as tests run sequentially
+ // and this doesn't affect other threads accessing environment variables
+ unsafe { std::env::set_var("RUST_LOG", "info") };Alternatively, use std::env::set_var within a test-specific scope or consider using EnvFilter::from_env with a test-specific filter string directly.
📝 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.
| unsafe { std::env::set_var("RUST_LOG", "info") }; | |
| // SAFETY: Setting RUST_LOG in this test is safe as tests run sequentially | |
| // and this doesn't affect other threads accessing environment variables | |
| unsafe { std::env::set_var("RUST_LOG", "info") }; |
🤖 Prompt for AI Agents
In crates/comenqd/src/logging.rs at line 69, the use of an unsafe block around
std::env::set_var lacks documentation explaining why unsafe is necessary. Add a
clear comment describing the safety rationale for using unsafe here.
Additionally, refactor the code to avoid mutating global environment variables
unsafely by either scoping the environment variable change to tests only or
using EnvFilter::from_env with a test-specific filter string to achieve the same
effect more safely.
There was a problem hiding this comment.
Has this now been resolved?
There was a problem hiding this comment.
@leynos No, the issue has not been resolved. The unsafe block still lacks the required safety documentation.
The current code shows:
unsafe { std::env::set_var("RUST_LOG", "info") };But it's missing the safety comment explaining why unsafe is necessary. According to your coding guidelines, unsafe usage must be properly documented. Add a comment like:
// SAFETY: Setting RUST_LOG in this test is safe as tests run sequentially
// and this doesn't affect other threads accessing environment variables
unsafe { std::env::set_var("RUST_LOG", "info") };The safety rationale should explain why the unsafe operation is sound in this specific context.
| /// Build a [`Config`] using paths inside `tmp` with a one-second cooldown. | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ``` | ||
| /// use tempfile::tempdir; | ||
| /// use comenqd::config::Config; | ||
| /// use test_support::temp_config; | ||
| /// | ||
| /// The configuration uses a dummy GitHub token and a one second cooldown. | ||
| /// let dir = tempdir().unwrap(); | ||
| /// let cfg: Config = temp_config(&dir); | ||
| /// assert_eq!(cfg.cooldown_period_seconds, 1); | ||
| /// ``` | ||
| pub fn temp_config(tmp: &TempDir) -> Config { | ||
| temp_config_with(tmp, 1) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mark helper return value as #[must_use]
temp_config creates a fully-initialised Config yet the caller can discard the result silently. Prevent accidental misuse:
- pub fn temp_config(tmp: &TempDir) -> Config {
+ #[must_use]
+ pub fn temp_config(tmp: &TempDir) -> Config {Do the same for temp_config_with.
📝 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.
| /// Build a [`Config`] using paths inside `tmp` with a one-second cooldown. | |
| /// | |
| /// # Examples | |
| /// | |
| /// ``` | |
| /// use tempfile::tempdir; | |
| /// use comenqd::config::Config; | |
| /// use test_support::temp_config; | |
| /// | |
| /// The configuration uses a dummy GitHub token and a one second cooldown. | |
| /// let dir = tempdir().unwrap(); | |
| /// let cfg: Config = temp_config(&dir); | |
| /// assert_eq!(cfg.cooldown_period_seconds, 1); | |
| /// ``` | |
| pub fn temp_config(tmp: &TempDir) -> Config { | |
| temp_config_with(tmp, 1) | |
| } | |
| /// Build a [`Config`] using paths inside `tmp` with a one-second cooldown. | |
| /// | |
| /// # Examples | |
| /// | |
| /// |
🤖 Prompt for AI Agents
In test-support/src/daemon.rs around lines 15 to 30, the functions temp_config
and temp_config_with return a fully-initialized Config but lack the #[must_use]
attribute, allowing callers to ignore the returned value unintentionally. Add
the #[must_use] attribute above both function definitions to enforce that their
return values are used and prevent accidental discarding.
There was a problem hiding this comment.
Has this now been resolved?
There was a problem hiding this comment.
@leynos No, the #[must_use] attribute has not been added to either function yet. Looking at the current file content, both temp_config and temp_config_with still lack the #[must_use] attribute.
Here's the corrected code with the #[must_use] attributes added:
/// Build a [`Config`] using paths inside `tmp` with a one-second cooldown.
///
/// # Examples
///
/// ```
/// use tempfile::tempdir;
/// use comenqd::config::Config;
/// use test_support::temp_config;
///
/// let dir = tempdir().unwrap();
/// let cfg: Config = temp_config(&dir);
/// assert_eq!(cfg.cooldown_period_seconds, 1);
/// ```
#[must_use]
pub fn temp_config(tmp: &TempDir) -> Config {
temp_config_with(tmp, 1)
}
/// Build a [`Config`] using paths inside `tmp`.
///
/// # Parameters
/// - `tmp`: temporary directory for socket and queue paths.
/// - `cooldown_period_seconds`: cooldown period between GitHub API calls.
///
/// # Examples
///
/// ```
/// use tempfile::tempdir;
/// use comenqd::config::Config;
/// use test_support::temp_config_with;
///
/// let dir = tempdir().unwrap();
/// let fast_cfg: Config = temp_config_with(&dir, 0);
/// assert_eq!(fast_cfg.cooldown_period_seconds, 0);
/// ```
#[must_use]
pub fn temp_config_with(tmp: &TempDir, cooldown_period_seconds: u64) -> Config {
Config {
github_token: "t".into(),
socket_path: tmp.path().join("sock"),
queue_path: tmp.path().join("q"),
cooldown_period_seconds,
}
}Both functions now have the #[must_use] attribute to prevent accidental discarding of their return values.
| pub fn temp_config_with(tmp: &TempDir, cooldown_period_seconds: u64) -> Config { | ||
| Config { | ||
| github_token: "t".into(), | ||
| socket_path: tmp.path().join("sock"), | ||
| queue_path: tmp.path().join("q"), | ||
| cooldown_period_seconds: 1, | ||
| cooldown_period_seconds, | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid duplication by delegating through ..Config::default()
Hard-coding every field increases drift risk if the struct grows. Build via struct-update syntax:
- Config {
- github_token: "t".into(),
- socket_path: tmp.path().join("sock"),
- queue_path: tmp.path().join("q"),
- cooldown_period_seconds,
- }
+ Config {
+ github_token: "t".into(),
+ socket_path: tmp.path().join("sock"),
+ queue_path: tmp.path().join("q"),
+ cooldown_period_seconds,
+ ..Default::default()
+ }Implement Default for Config inside the production crate if missing.
🤖 Prompt for AI Agents
In test-support/src/daemon.rs around lines 49 to 56, the temp_config_with
function manually sets all Config fields, risking duplication and drift if
Config changes. Refactor by implementing Default for Config in the production
crate if not already done, then construct the Config instance using struct
update syntax with ..Config::default() to fill in default values, overriding
only the necessary fields like github_token, socket_path, queue_path, and
cooldown_period_seconds.
There was a problem hiding this comment.
Has this now been resolved?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Config struct has Default implementation
ast-grep --pattern 'impl Default for Config {
$$$
}'
# Also check for derive Default on Config struct
ast-grep --pattern '#[derive($$$Default$$$)]
struct Config {
$$$
}'
# Look for the Config struct definition to understand its structure
ast-grep --pattern 'struct Config {
$$$
}'
Length of output: 175
🏁 Script executed:
#!/bin/bash
# Find the Config struct definition using ripgrep
rg -A 10 "struct Config" --type rust
# Search for Default implementation for Config
rg "impl.*Default.*Config" --type rust
# Look for derive macros on Config
rg "#\[derive.*Config" --type rust -B 2 -A 5
Length of output: 1529
🏁 Script executed:
#!/bin/bash
# Look for Config struct with derive macros
rg -B 3 -A 15 "pub struct Config" crates/comenqd/src/config.rs
# Check if there are any Default trait implementations in the config module
rg "impl.*Default" crates/comenqd/src/config.rs
# Look for any derive macros above the Config struct
rg -B 5 "pub struct Config" crates/comenqd/src/config.rs
Length of output: 1032
@leynos No, this has not been resolved. Looking at the current code, the temp_config_with function still manually sets all Config fields:
Config {
github_token: "t".into(),
socket_path: tmp.path().join("sock"),
queue_path: tmp.path().join("q"),
cooldown_period_seconds,
}Additionally, I found that the Config struct doesn't implement Default:
#[derive(Debug, Deserialize, Serialize, PartialEq, Eq)]
pub struct Config {
// fields...
}However, upon reflection, the original suggestion may not be appropriate here. Test helpers benefit from explicit field assignment because:
- Predictability: Tests need known, explicit values rather than potentially changing defaults
- Clarity: The manual approach makes it obvious what values are being set for tests
- No meaningful defaults: Fields like
github_tokendon't have sensible default values
The current implementation is actually more suitable for test code than using ..Config::default() would be.
|
Stale |
1 participant
Summary
Testing
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_688d12fbb4a88322b4b4720488862350
Summary by Sourcery
Refactor configuration helpers to support customizable cooldown intervals, update test-support exports and tests accordingly, add JSON tracing and wiremock test dependencies, and fix env_guard reexports and logging test warnings.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: