Conversation
Reviewer's GuideThis PR refactors environment variable guard utilities into the shared test-support crate, updates imports and dependencies accordingly, derives Clone on the daemon Config struct, and fixes a flaky release tag test pattern, along with minor import cleanups in tests. Class diagram for EnvVarGuard utility refactorclassDiagram
class EnvVarGuard {
+EnvVarGuard(var: String, original: Option<String>)
+remove_env_var(var: &str)
+set_env_var(var: &str, value: &str)
}
%% EnvVarGuard and related utilities are now in test_support::env_guard
test_support "1" --o EnvVarGuard : contains
test_support : env_guard
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary by CodeRabbit
WalkthroughUnify and centralise environment variable guard utilities for tests by moving their implementation into the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Module
participant TestSupport as test_support::env_guard
Note right of Test: During test setup
Test->>TestSupport: use EnvVarGuard, remove_env_var, set_env_var
TestSupport-->>Test: Provides environment variable utilities
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 (
|
![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 they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/steps/release_steps.rs` </location>
<code_context>
- assert!(
- tags.iter()
- .any(|t| t.as_str() == Some("v[0-9]*.[0-9]*.[0-9]*"))
- );
+ assert!(tags.iter().any(|t| t.as_str() == Some("v*.*.*")));
</code_context>
<issue_to_address>
Test assertion for tag pattern matching is too permissive.
The assertion should verify that tags match the semantic version pattern, such as using a regex, instead of checking for the literal string "v*.*.*".
</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: 4
🔭 Outside diff range comments (1)
crates/comenq/src/lib.rs (1)
44-60: Mark tests with#[cfg(test)]modules only; avoid extra#[rstest]on the same line.The pattern
#[rstest] #[case(...)]works, but placing
#[rstest]alone on an empty line triggers the clippyempty_line_after_outer_attrlint. Collapse the two attributes onto the function to silence the warning and keep attribute order tidy:-#[rstest] -#[case("octocat/hello-world", 1, "Hi")] -fn parses_valid_arguments(#[case] slug: &str, #[case] pr: u64, #[case] body: &str) { +#[rstest] +fn parses_valid_arguments( + #[case("octocat/hello-world")] slug: &str, + #[case(1)] pr: u64, + #[case("Hi")] body: &str, +) {
📜 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)
crates/comenq/Cargo.toml(1 hunks)crates/comenq/src/lib.rs(1 hunks)crates/comenqd/Cargo.toml(1 hunks)crates/comenqd/src/config.rs(2 hunks)crates/comenqd/src/daemon.rs(2 hunks)test-support/Cargo.toml(1 hunks)test-support/src/lib.rs(1 hunks)tests/cucumber.rs(0 hunks)tests/steps/config_steps.rs(1 hunks)tests/steps/release_steps.rs(1 hunks)tests/support/mod.rs(0 hunks)
💤 Files with no reviewable changes (2)
- tests/cucumber.rs
- tests/support/mod.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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:
crates/comenq/src/lib.rstests/steps/config_steps.rscrates/comenqd/src/daemon.rscrates/comenqd/src/config.rstests/steps/release_steps.rstest-support/src/lib.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:
crates/comenq/src/lib.rstests/steps/config_steps.rscrates/comenqd/src/daemon.rscrates/comenqd/src/config.rstests/steps/release_steps.rstest-support/src/lib.rs
🧬 Code Graph Analysis (3)
tests/steps/config_steps.rs (1)
test-support/src/env_guard.rs (1)
remove_env_var(44-46)
crates/comenqd/src/config.rs (1)
test-support/src/env_guard.rs (1)
remove_env_var(44-46)
test-support/src/lib.rs (2)
test-support/src/env_guard.rs (2)
remove_env_var(44-46)set_env_var(37-39)tests/steps/config_steps.rs (1)
set_env_var(83-85)
🪛 GitHub Check: build-test
crates/comenq/src/lib.rs
[failure] 42-42:
unresolved import rstest::case
🪛 GitHub Actions: CI
crates/comenq/src/lib.rs
[error] 42-42: Rust compiler error E0432: unresolved import rstest::case.
🔇 Additional comments (8)
tests/steps/config_steps.rs (1)
9-9: LGTM – switched to sharedtest_support::env_guard.The new import eliminates duplicated helper code and centralises env-var handling. Good refinement.
test-support/Cargo.toml (1)
13-14: Keepserial_testas a normal dependency when used by downstream crates.If
serial_testis re-exported (or used) bytest-support, place it under[dependencies]rather than[dev-dependencies]; crates that depend ontest-supportin tests still need to compile the crate itself. Leaving it as dev-only risks build failures when another crate’s tests pull intest-support.crates/comenqd/src/daemon.rs (1)
367-367: Incorrect mutability change breaks function signature.The
queue_writerfunction expectsmut rx: mpsc::UnboundedReceiver<Vec<u8>>but you've removed themutkeyword from the binding. This will cause a compilation error.Revert the mutability:
- let (client_tx, writer_rx) = mpsc::unbounded_channel(); + let (client_tx, mut writer_rx) = mpsc::unbounded_channel();Likely an incorrect or invalid review comment.
tests/steps/release_steps.rs (1)
43-43: LGTM! Test logic corrected.The change from
"v[0-9]*.[0-9]*.[0-9]*"to"v*.*.*"correctly fixes the assertion to match the actual YAML tag pattern format rather than treating a regex-like pattern as a literal string.test-support/src/lib.rs (2)
4-4: LGTM! Module addition supports centralised environment utilities.The addition of the
env_guardmodule correctly implements the refactoring to centralise environment variable guard utilities.
8-8: LGTM! Re-exports provide clean API surface.The re-exports correctly expose the environment variable guard utilities from the centralised module, providing a clean public API.
crates/comenqd/src/config.rs (2)
23-23: LGTM! Clone derivation enhances usability.Adding
Cloneto theConfigstruct is beneficial for testing scenarios and general usability, particularly when the config needs to be shared across multiple contexts.
121-121: LGTM! Import correctly uses centralised utilities.The import change correctly adopts the centralised environment variable guard utilities from the
test-supportcrate, aligning with the broader refactoring effort.
| [dev-dependencies] | ||
| rstest = "0.18" | ||
| tempfile = { workspace = true } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Move dev-dependencies to the workspace root for consistency.
rstest and tempfile are already used by other crates in the workspace. Declare them once under [workspace.dependencies] or [workspace.dev-dependencies] and reference them with workspace = true here to reduce duplication and compilation time.
🤖 Prompt for AI Agents
In crates/comenq/Cargo.toml at lines 18 to 20, the dev-dependencies rstest and
tempfile should be removed from this crate's Cargo.toml and instead declared
once in the workspace root Cargo.toml under [workspace.dev-dependencies]. Then,
keep only the references with workspace = true here to avoid duplication and
improve compilation efficiency.
| serial_test = "2" | ||
| test-support = { path = "../../test-support" } | ||
| test-utils = { path = "../test-utils" } | ||
| wiremock = "0.6" |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add wiremock behind a workspace alias to avoid version skew.
wiremock = "0.6" is hard-coded while the workspace already pins many crates through the root Cargo.toml. Expose wiremock from the workspace ( [workspace.dependencies] wiremock = "0.6" ) and depend on it here with wiremock = { workspace = true } to keep all crates in lock-step and prevent duplicate versions in the final build.
🤖 Prompt for AI Agents
In crates/comenqd/Cargo.toml at line 28, the dependency wiremock is hard-coded
to version 0.6. To avoid version skew and ensure consistency across the
workspace, remove the version string and instead reference the workspace alias
by changing the dependency to wiremock = { workspace = true }. Also, ensure that
wiremock = "0.6" is declared under [workspace.dependencies] in the root
Cargo.toml to centralize version management.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
crates/comenq/src/client.rs(0 hunks)crates/comenq/src/lib.rs(2 hunks)
💤 Files with no reviewable changes (1)
- crates/comenq/src/client.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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 using Arc to 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 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.
Place function attributes after doc comments.
Do not use return in 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.
Prefer expect over allow.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Use eyre::Report for 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 to eyre only in the main main() entrypoint or top-level async task.
Files:
crates/comenq/src/lib.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:
crates/comenq/src/lib.rs
🪛 GitHub Check: build-test
crates/comenq/src/lib.rs
[failure] 47-47:
failed to resolve: use of unresolved module or unlinked crate slug
[failure] 49-49:
failed to resolve: could not find case in rstest
[failure] 48-48:
failed to resolve: could not find case in rstest
[failure] 47-47:
failed to resolve: could not find case in rstest
[failure] 45-45:
failed to resolve: could not find case in rstest
[failure] 64-64:
failed to resolve: could not find case in rstest
[failure] 63-63:
failed to resolve: could not find case in rstest
[failure] 62-62:
failed to resolve: could not find case in rstest
[failure] 61-61:
failed to resolve: could not find case in rstest
[failure] 60-60:
failed to resolve: could not find case in rstest
🪛 GitHub Actions: CI
crates/comenq/src/lib.rs
[error] 45-45: Rust compiler error E0433: failed to resolve: could not find case in rstest
Move environment variable helpers into the test-support crate so crates can share them without include hacks. Add missing dev dependencies and derive Clone for configuration to resolve compilation errors. Clean up unused imports and fix release workflow step.
leynos
force-pushed
the
codex/fix-compilation-errors-in-codebase
branch
from
5897fce to
b20f9aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/comenqd/Cargo.toml (1)
28-28: LGTM - Workspace reference correctly applied.Converting wiremock to use workspace reference addresses the previous review feedback about avoiding version skew and ensures consistency across the workspace.
📜 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)crates/comenqd/Cargo.toml(1 hunks)crates/comenqd/src/config.rs(1 hunks)crates/comenqd/src/daemon.rs(2 hunks)test-support/Cargo.toml(1 hunks)test-support/src/lib.rs(1 hunks)tests/cucumber.rs(0 hunks)tests/steps/config_steps.rs(1 hunks)tests/steps/release_steps.rs(2 hunks)tests/support/mod.rs(0 hunks)
💤 Files with no reviewable changes (2)
- tests/cucumber.rs
- tests/support/mod.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 using Arc to 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 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.
Place function attributes after doc comments.
Do not use return in 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.
Prefer expect over allow.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Use eyre::Report for 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 to eyre only in the main main() entrypoint or top-level async task.
Files:
tests/steps/config_steps.rscrates/comenqd/src/daemon.rstest-support/src/lib.rstests/steps/release_steps.rscrates/comenqd/src/config.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/config_steps.rscrates/comenqd/src/daemon.rstest-support/src/lib.rstests/steps/release_steps.rscrates/comenqd/src/config.rs
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up-to-date.
Mandate caret requirements for all dependencies. All crate versions specified in Cargo.toml must 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 are strictly forbidden, as are tilde requirements (~) except for documented reasons.
Files:
Cargo.toml
🧬 Code Graph Analysis (3)
tests/steps/config_steps.rs (1)
test-support/src/env_guard.rs (1)
remove_env_var(44-46)
test-support/src/lib.rs (2)
test-support/src/env_guard.rs (2)
remove_env_var(44-46)set_env_var(37-39)tests/steps/config_steps.rs (1)
set_env_var(83-85)
crates/comenqd/src/config.rs (1)
test-support/src/env_guard.rs (1)
remove_env_var(44-46)
🪛 GitHub Actions: CI
crates/comenqd/src/daemon.rs
[error] 48-48: Rust compiler error E0412: cannot find type UnixListener in this scope.
🔇 Additional comments (9)
test-support/src/lib.rs (2)
4-4: LGTM - Good modularisation.Adding the env_guard module centralises environment variable utilities that were previously scattered across different files.
9-9: LGTM - Clean API surface.The re-exports provide a convenient API for environment variable management functions from the new shared module.
tests/steps/config_steps.rs (1)
9-9: LGTM - Import path correctly updated.The import path has been properly updated to use the new shared test-support crate instead of the local support module.
test-support/Cargo.toml (1)
13-14: LGTM - Appropriate test dependency.Adding serial_test as a development dependency is sensible for a test support crate, particularly given the environment variable manipulation utilities this crate provides.
crates/comenqd/Cargo.toml (1)
26-27: LGTM - Test dependencies properly added.Adding the test-support and test-utils dependencies enables the use of centralized testing utilities as part of the refactoring.
crates/comenqd/src/config.rs (1)
23-23: LGTM - Clone derive addition is appropriate.Adding
Cloneto theConfigstruct is reasonable and all field types already implementClone.tests/steps/release_steps.rs (1)
6-6: LGTM - Clean regex import.The import is properly placed and follows standard conventions.
Cargo.toml (2)
21-21: LGTM - Workspace dependency references improve consistency.Converting to workspace references for
tempfile,wiremock, andregexcentralises dependency version management.Also applies to: 23-23, 26-26
55-55: LGTM - Workspace dependencies follow SemVer caret requirements.The new workspace dependencies
regex = "1"andwiremock = "0.6"properly use implicit caret requirements as mandated by the coding guidelines.Also applies to: 57-57
| } | ||
|
|
||
| use env_guard::{EnvVarGuard, remove_env_var}; | ||
| use test_support::env_guard::{EnvVarGuard, remove_env_var}; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Import change looks good, but verify unsafe usage in shared utility.
The migration to shared test utilities improves code organisation. However, ensure the unsafe operation in test_support::env_guard::remove_env_var (which calls std::env::remove_var) is properly justified and documented according to the coding guidelines.
🏁 Script executed:
#!/bin/bash
# Verify that the unsafe usage in remove_env_var is properly documented
ast-grep --pattern $'pub fn remove_env_var($_) {
$$$
unsafe { $$$ }
$$$
}'Length of output: 265
Remove unnecessary unsafe block in remove_env_var
The remove_env_var function wraps the safe std::env::remove_var call in an unsafe block. Eliminate this block to comply with our guideline of avoiding superfluous unsafe usage.
• File: test-support/src/env_guard.rs
• Lines: 44–46
Suggested change:
- pub fn remove_env_var(key: &str) {
- unsafe { std::env::remove_var(key) };
- }
+ pub fn remove_env_var(key: &str) {
+ std::env::remove_var(key);
+ }📝 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.
| use test_support::env_guard::{EnvVarGuard, remove_env_var}; | |
| // In test-support/src/env_guard.rs | |
| pub fn remove_env_var(key: &str) { | |
| std::env::remove_var(key); | |
| } |
🤖 Prompt for AI Agents
In crates/comenqd/src/config.rs at line 121, the import of remove_env_var is
noted, but the actual issue is in test-support/src/env_guard.rs lines 44 to 46
where remove_env_var wraps std::env::remove_var in an unnecessary unsafe block.
To fix this, remove the unsafe block around std::env::remove_var call in
remove_env_var, making the function fully safe and compliant with guidelines
against unnecessary unsafe usage.
| use tokio::fs; | ||
| use tokio::io::AsyncReadExt; | ||
| use tokio::net::{UnixListener, UnixStream}; | ||
| use tokio::net::UnixStream; |
There was a problem hiding this comment.
Restore UnixListener to the main module imports.
UnixListener is used in the main module (lines 48 and 52) but is missing from the import statement. This causes the compilation error E0412 shown in the pipeline failures.
Apply this diff to fix the import:
-use tokio::net::UnixStream;
+use tokio::net::{UnixListener, UnixStream};📝 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.
| use tokio::net::UnixStream; | |
| // restore UnixListener alongside UnixStream in the main imports | |
| use tokio::net::{UnixListener, UnixStream}; |
🤖 Prompt for AI Agents
In crates/comenqd/src/daemon.rs at line 18, the import statement is missing
UnixListener, which is used later in the file around lines 48 and 52. Add
UnixListener to the import list from tokio::net alongside UnixStream to fix the
compilation error E0412.
| let pattern = Regex::new(r"^v\*\.\*\.\*$").expect("compile regex"); | ||
| assert!( | ||
| tags.iter() | ||
| .any(|t| t.as_str() == Some("v[0-9]*.[0-9]*.[0-9]*")) | ||
| .filter_map(|t| t.as_str()) | ||
| .any(|t| pattern.is_match(t)), | ||
| "missing semantic version tag pattern", | ||
| ); |
There was a problem hiding this comment.
Fix regex pattern for semantic version matching.
The current regex pattern ^v\*\.\*\.\*$ matches the literal string "v*.." rather than semantic version tags like "v1.2.3".
Apply this diff to fix the pattern:
- let pattern = Regex::new(r"^v\*\.\*\.\*$").expect("compile regex");
+ let pattern = Regex::new(r"^v\d+\.\d+\.\d+$").expect("compile regex");This will properly match semantic version tags with the format "v{major}.{minor}.{patch}".
📝 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.
| let pattern = Regex::new(r"^v\*\.\*\.\*$").expect("compile regex"); | |
| assert!( | |
| tags.iter() | |
| .any(|t| t.as_str() == Some("v[0-9]*.[0-9]*.[0-9]*")) | |
| .filter_map(|t| t.as_str()) | |
| .any(|t| pattern.is_match(t)), | |
| "missing semantic version tag pattern", | |
| ); | |
| let pattern = Regex::new(r"^v\d+\.\d+\.\d+$").expect("compile regex"); | |
| assert!( | |
| tags.iter() | |
| .filter_map(|t| t.as_str()) | |
| .any(|t| pattern.is_match(t)), | |
| "missing semantic version tag pattern", | |
| ); |
🤖 Prompt for AI Agents
In tests/steps/release_steps.rs around lines 44 to 50, the regex pattern used to
match semantic version tags is incorrect as it matches the literal string
"v*.*.*" instead of versions like "v1.2.3". Update the regex pattern to
correctly match semantic version tags by replacing the pattern with one that
matches a 'v' followed by digits, dots, and digits in the format
"v{major}.{minor}.{patch}".
Remove correct UnixListener
leynos
force-pushed
the
codex/fix-compilation-errors-in-codebase
branch
from
60905d0 to
b33662e
Compare
1 participant
Summary
EnvVarGuardutilities into the sharedtest-supportcrateClonefor daemon config and clean up importsTesting
make lintmake testhttps://chatgpt.com/codex/tasks/task_e_688dd1a5eeb083229e4647ce61ce0195
Summary by Sourcery
Refactor test utilities into a shared crate, derive Clone for Config, update dependencies and imports, and fix the release tag test.
Bug Fixes:
Enhancements:
Build:
Tests: