feat: adopt mise conventions - build info, edition 2024, lints, tooling - #38
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds project tooling configuration, updates Cargo to edition 2024 with lint and dependency settings, generates build metadata at compile time, uses that metadata to format the CLI version string, and wraps test environment variable mutations in unsafe blocks. ChangesBuild tooling and metadata
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/build_time.rs (1)
8-9: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePanic on startup if
BUILT_TIME_UTCfails to parse.
BUILD_TIMEis aLazyLockreferenced frommain.rs'sAGENTFLARE_VERSION, which is itself embedded in the clap#[command(version = ...)]attribute — meaning it is evaluated on essentially every CLI invocation (not just--version), since building theCommandis part ofCli::parse(). Thebuiltcrate's docs guaranteeBUILT_TIME_UTCparses via RFC2822, but it also supports aBUILT_OVERRIDE_BUILT_TIME_UTCenv-var override that must independently satisfy that contract — if it's ever misconfigured, this.unwrap()will crash the entire CLI on startup, not just when--versionis requested.Using
.expect(...)with a clear message would at least make the failure mode self-diagnosing.🔧 Proposed fix
pub static BUILD_TIME: Lazy<DateTime<FixedOffset>> = - Lazy::new(|| DateTime::parse_from_rfc2822(built_info::BUILT_TIME_UTC).unwrap()); + Lazy::new(|| { + DateTime::parse_from_rfc2822(built_info::BUILT_TIME_UTC) + .expect("built_info::BUILT_TIME_UTC should always be a valid RFC2822 timestamp") + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/build_time.rs` around lines 8 - 9, The BUILD_TIME Lazy initialization currently uses unwrap on built_info::BUILT_TIME_UTC, which can crash Cli::parse() startup if the override is malformed. Update the BUILD_TIME initializer in build_time.rs to use expect with a clear, self-diagnosing message instead of unwrap, so failures in AGENTFLARE_VERSION/version evaluation are reported explicitly when main.rs or clap::command(version) triggers the parse.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/build_time.rs`:
- Around line 8-9: The BUILD_TIME Lazy initialization currently uses unwrap on
built_info::BUILT_TIME_UTC, which can crash Cli::parse() startup if the override
is malformed. Update the BUILD_TIME initializer in build_time.rs to use expect
with a clear, self-diagnosing message instead of unwrap, so failures in
AGENTFLARE_VERSION/version evaluation are reported explicitly when main.rs or
clap::command(version) triggers the parse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5fc84dae-f344-4c29-acbd-a383507cc93c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.editorconfigCargo.tomlbuild.rscliff.tomlsrc/build_time.rssrc/main.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agent_detect.rs (1)
46-66: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
PATH_LOCKstill leaves other PATH reads unsynchronized.find_binary()readsPATHdirectly, and Cargo runs this test binary in parallel by default. Any other test that touchesPATH—directly or throughfind_binary()—can race with theseset_var/remove_varcalls unless the whole suite is serialized with--test-threads=1or every PATH-sensitive test path takes the same lock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_detect.rs` around lines 46 - 66, `PATH_LOCK` only protects the mutation block in `with_temp_path_dir`, but `find_binary()` still reads `PATH` without synchronization, so PATH-sensitive tests can race. Update the PATH-handling test support around `find_binary_tests::with_temp_path_dir` and `find_binary()` so every PATH read/write uses the same `PATH_LOCK` (or otherwise serialize the relevant tests), ensuring no test touches PATH outside the shared lock.
🧹 Nitpick comments (2)
src/paths.rs (1)
25-34: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRestoration is skipped if
f()panics.If the closure passed to
with_temp_homepanics,unsafe { std::env::remove_var("AGENTFLARE_HOME_OVERRIDE") }on Line 32 never runs, leaving the override pointed at a temp dir that gets cleaned up by nothing, for the remainder of the test process. Sincehome()consumers (e.g.init.rstests) rely on this override, a single panicking test can cause unrelated subsequent tests to silently read/write the wrong home directory.Consider a
Drop-based guard (same pattern as suggested forsrc/agent_detect.rs's PATH helpers) so restoration happens even on unwind.♻️ Suggested fix
pub(crate) fn with_temp_home<T>(f: impl FnOnce() -> T) -> T { let _guard = GLOBAL_STATE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let dir = std::env::temp_dir().join("agentflare-test-home"); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).unwrap(); - unsafe { std::env::set_var("AGENTFLARE_HOME_OVERRIDE", &dir) }; - let result = f(); - unsafe { std::env::remove_var("AGENTFLARE_HOME_OVERRIDE") }; - result + struct Restore; + impl Drop for Restore { + fn drop(&mut self) { + unsafe { std::env::remove_var("AGENTFLARE_HOME_OVERRIDE") }; + } + } + unsafe { std::env::set_var("AGENTFLARE_HOME_OVERRIDE", &dir) }; + let _restore = Restore; + f() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/paths.rs` around lines 25 - 34, `with_temp_home` leaves AGENTFLARE_HOME_OVERRIDE set if the closure panics, because the cleanup runs only after `f()` returns normally. Update the helper to use a `Drop`-based guard or equivalent RAII cleanup so `std::env::remove_var("AGENTFLARE_HOME_OVERRIDE")` always runs on unwind as well; keep the fix localized to `with_temp_home` and preserve the existing GLOBAL_STATE_LOCK and temp dir setup.src/agent_detect.rs (1)
53-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate and make the PATH restore panic-safe.
This
with_temp_path_diris duplicated almost verbatim indetect_all_tests(Lines 452-465) and again insrc/agents.rs(Lines 209-223). Additionally, iff(&dir)panics (e.g. an assertion fails), the restoration code on Lines 61-64 is skipped, leavingPATHpointed at a since-deleted temp dir for the rest of the test binary — causing unrelated, hard-to-diagnose cascading test failures.Both issues are addressed by extracting a small
Drop-based guard that restores the variable even on unwind, and sharing it across all three call sites.♻️ Suggested RAII guard (shared, e.g. in a small internal test-support module)
+struct EnvVarGuard { + key: &'static str, + original: Option<std::ffi::OsString>, +} + +impl EnvVarGuard { + fn set(key: &'static str, value: impl AsRef<std::ffi::OsStr>) -> Self { + let original = std::env::var_os(key); + unsafe { std::env::set_var(key, value) }; + Self { key, original } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + match self.original.take() { + Some(v) => unsafe { std::env::set_var(self.key, v) }, + None => unsafe { std::env::remove_var(self.key) }, + } + } +} + fn with_temp_path_dir(f: impl FnOnce(&Path)) { let _guard = super::PATH_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let dir = std::env::temp_dir().join(format!("agentflare-test-path-{}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).unwrap(); - let original = std::env::var_os("PATH"); - unsafe { std::env::set_var("PATH", &dir) }; - f(&dir); - match original { - Some(p) => unsafe { std::env::set_var("PATH", p) }, - None => unsafe { std::env::remove_var("PATH") }, - } + let _env_guard = EnvVarGuard::set("PATH", &dir); + f(&dir); let _ = std::fs::remove_dir_all(&dir); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/agent_detect.rs` around lines 53 - 66, `with_temp_path_dir` needs two fixes: it is duplicated in `detect_all_tests` and `src/agents.rs`, and its PATH restoration is not panic-safe. Extract the PATH swap/restore logic into a shared internal RAII guard (used by `with_temp_path_dir` and the other two call sites) so restoration happens in `Drop` even if `f(&dir)` panics. Keep the existing behavior of creating the temp dir, setting PATH to it, running the closure, and cleaning up the temp directory afterward.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/agent_detect.rs`:
- Around line 46-66: `PATH_LOCK` only protects the mutation block in
`with_temp_path_dir`, but `find_binary()` still reads `PATH` without
synchronization, so PATH-sensitive tests can race. Update the PATH-handling test
support around `find_binary_tests::with_temp_path_dir` and `find_binary()` so
every PATH read/write uses the same `PATH_LOCK` (or otherwise serialize the
relevant tests), ensuring no test touches PATH outside the shared lock.
---
Nitpick comments:
In `@src/agent_detect.rs`:
- Around line 53-66: `with_temp_path_dir` needs two fixes: it is duplicated in
`detect_all_tests` and `src/agents.rs`, and its PATH restoration is not
panic-safe. Extract the PATH swap/restore logic into a shared internal RAII
guard (used by `with_temp_path_dir` and the other two call sites) so restoration
happens in `Drop` even if `f(&dir)` panics. Keep the existing behavior of
creating the temp dir, setting PATH to it, running the closure, and cleaning up
the temp directory afterward.
In `@src/paths.rs`:
- Around line 25-34: `with_temp_home` leaves AGENTFLARE_HOME_OVERRIDE set if the
closure panics, because the cleanup runs only after `f()` returns normally.
Update the helper to use a `Drop`-based guard or equivalent RAII cleanup so
`std::env::remove_var("AGENTFLARE_HOME_OVERRIDE")` always runs on unwind as
well; keep the fix localized to `with_temp_home` and preserve the existing
GLOBAL_STATE_LOCK and temp dir setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a33fe0e-11dc-44cd-a4b4-ce93c03196e4
📒 Files selected for processing (3)
src/agent_detect.rssrc/agents.rssrc/paths.rs
6461cf9 to
641bb26
Compare
- build.rs with built crate embeds git hash + build date into version output - Cargo.toml: edition 2024, rust-version 1.91, [lints] clippy pedantic - Cargo.toml: profile.dev debug=1 for faster dev compile - Cargo.toml: insta + pretty_assertions dev-deps - .editorconfig: consistent indent/charset across editors - cliff.toml: auto-generate changelog from conventional commits
Mise only has targeted clippy allows, not pedantic at crate level. Pedantic lints on test code cause 11 compile errors.
E0133: std::env::set_var and remove_var are unsafe in Rust 2024. Fixes 11 compile errors across paths.rs, agent_detect.rs, agents.rs
641bb26 to
6b057ba
Compare
1 participant
Closes #37 (partial)
Summary
Adopt mise conventions: build.rs with built crate embeds git hash + build date into --version, edition 2024, rust-version 1.91, clippy pedantic lints, profile.dev debug=1, insta + pretty_assertions dev-deps, .editorconfig, cliff.toml.
Test plan
Notes for reviewers
Summary by CodeRabbit