Prepare v0.9.0 release updates and fast coverage#147
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR bumps the package version to v0.9.0 with corresponding documentation updates and adds comprehensive test coverage across CLI and server modules. Version numbers are updated in Cargo.toml, OpenAPI specification, and CHANGELOG, while new unit and integration tests are introduced for error handling and feature validation without modifying production code logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/adapters/server/lifecycle.rs (1)
525-558: Harden global-state restoration against test panics.These tests restore global pointers/FDs manually; if assertions fail before restore, later tests can inherit corrupted global state. Consider a local
Drop-based restore guard forGLOBAL_DRAINING,SHUTDOWN_PIPE_WR, andGLOBAL_LOG_LEVEL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/server/lifecycle.rs` around lines 525 - 558, Wrap the manual global-state swaps in both tests (signal_handler_sets_draining_and_wakes_shutdown_pipe and sigusr1_handler_cycles_global_log_level) with a local Drop-based guard that captures the previous pointer/FD (previous_draining, previous_write_fd, previous) and restores GLOBAL_DRAINING, SHUTDOWN_PIPE_WR, and GLOBAL_LOG_LEVEL in its Drop impl so restoration always runs even if the test panics; create a small scoped helper type (e.g. TestGlobalRestore) instantiated after performing Box::into_raw/...swap and before calling signal_handler/sigusr1_handler, and ensure the guard also handles closing the pipe (close_shutdown_pipe) and dropping the boxed allocations in Drop to avoid leaks.src/adapters/cli/serve.rs (1)
208-247: Make env-var cleanup panic-safe in tests.Both tests mutate process-wide environment and clean up manually; if a test aborts early,
TRUSS_STORAGE_ROOTcan leak into other tests. Use aDropguard for deterministic cleanup.♻️ Suggested test-hardening patch
#[cfg(test)] mod tests { use super::*; use serial_test::serial; use std::io; + struct EnvVarGuard(&'static str); + impl Drop for EnvVarGuard { + fn drop(&mut self) { + // SAFETY: test-only cleanup of process env var. + unsafe { std::env::remove_var(self.0) }; + } + } + @@ fn execute_serve_returns_runtime_error_for_invalid_bind_addr() { let storage_root = tempfile::tempdir().expect("create tempdir"); // SAFETY: test-only env mutation guarded by serial execution. unsafe { std::env::set_var("TRUSS_STORAGE_ROOT", storage_root.path()) }; + let _env_guard = EnvVarGuard("TRUSS_STORAGE_ROOT"); @@ - // SAFETY: paired cleanup for the test-only env mutation above. - unsafe { std::env::remove_var("TRUSS_STORAGE_ROOT") }; @@ fn execute_validate_reports_writer_failures() { let storage_root = tempfile::tempdir().expect("create tempdir"); // SAFETY: test-only env mutation guarded by serial execution. unsafe { std::env::set_var("TRUSS_STORAGE_ROOT", storage_root.path()) }; + let _env_guard = EnvVarGuard("TRUSS_STORAGE_ROOT"); @@ - // SAFETY: paired cleanup for the test-only env mutation above. - unsafe { std::env::remove_var("TRUSS_STORAGE_ROOT") };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/cli/serve.rs` around lines 208 - 247, The tests execute_serve_returns_runtime_error_for_invalid_bind_addr and execute_validate_reports_writer_failures mutate TRUSS_STORAGE_ROOT and currently remove it manually; make cleanup panic-safe by introducing an RAII Drop guard (e.g., EnvVarGuard) used in each test: construct the guard which sets TRUSS_STORAGE_ROOT (for the duration of the test) and implements Drop to remove the var, then replace the unsafe set_var/remove_var pairs in those tests (which call execute_serve and execute_validate) with creating the guard so the env var is always removed even on panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/openapi.yaml`:
- Line 4: The top-level version was updated to 0.9.0 but two schema examples
still contain "0.7.1"; update the example values to "0.9.0" so examples match
the top-level version. Locate the example/version fields in the OpenAPI YAML
(the entries showing "0.7.1" near the schema examples referenced) and replace
them with "0.9.0", then scan the file for any remaining "0.7.1" strings
(example/version keys) and ensure all examples consistently reflect 0.9.0.
---
Nitpick comments:
In `@src/adapters/cli/serve.rs`:
- Around line 208-247: The tests
execute_serve_returns_runtime_error_for_invalid_bind_addr and
execute_validate_reports_writer_failures mutate TRUSS_STORAGE_ROOT and currently
remove it manually; make cleanup panic-safe by introducing an RAII Drop guard
(e.g., EnvVarGuard) used in each test: construct the guard which sets
TRUSS_STORAGE_ROOT (for the duration of the test) and implements Drop to remove
the var, then replace the unsafe set_var/remove_var pairs in those tests (which
call execute_serve and execute_validate) with creating the guard so the env var
is always removed even on panic.
In `@src/adapters/server/lifecycle.rs`:
- Around line 525-558: Wrap the manual global-state swaps in both tests
(signal_handler_sets_draining_and_wakes_shutdown_pipe and
sigusr1_handler_cycles_global_log_level) with a local Drop-based guard that
captures the previous pointer/FD (previous_draining, previous_write_fd,
previous) and restores GLOBAL_DRAINING, SHUTDOWN_PIPE_WR, and GLOBAL_LOG_LEVEL
in its Drop impl so restoration always runs even if the test panics; create a
small scoped helper type (e.g. TestGlobalRestore) instantiated after performing
Box::into_raw/...swap and before calling signal_handler/sigusr1_handler, and
ensure the guard also handles closing the pipe (close_shutdown_pipe) and
dropping the boxed allocations in Drop to avoid leaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ce01bed-d192-4e6e-9a22-3bb329470c31
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CHANGELOG.mdCargo.tomldocs/openapi.yamlsrc/adapters/cli/serve.rssrc/adapters/cli/sign.rssrc/adapters/server/lifecycle.rstests/server_head.rs
1 participant
Summary
Testing
Summary by CodeRabbit
New Features
Tests
Chores