Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the initial implementation of Skillet, a new Rust-based idempotent host configuration tool. It establishes the core architecture, including modular crates for fundamental resource management and configuration logic. A key feature is the integrated testing suite, which uses containerization to record and validate configuration changes, ensuring that the tool maintains system state as expected. This foundational work sets the stage for a robust and reliable configuration management solution. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the skillet configuration tool, a well-structured Rust project with a clear separation of concerns into core, hardening, and CLI crates. The use of traits for dependency injection and a recorder for testing is excellent. My review focuses on improving robustness, reducing code duplication, and adhering to the project's own defined mandates. I've identified a few areas for improvement, including removing some placeholder code, handling potential panics more gracefully, simplifying error handling, and addressing a direct violation of the error handling mandate regarding unwrap() in library code. I've also noted an opportunity to reduce code duplication between the host-specific binaries.
| } | ||
|
|
||
| pub fn get_ops(&self) -> Vec<ResourceOp> { | ||
| self.ops.lock().unwrap().clone() |
There was a problem hiding this comment.
Using .unwrap() on a Mutex lock violates the project's mandate in AGENTS.md to never use unwrap() in library code. A poisoned mutex will cause a panic. To make this more robust, you can handle the PoisonError, for example by calling .unwrap_or_else(|e| e.into_inner()) to recover the inner data. This approach avoids a panic while still getting access to the data, which is generally safe for simple operations like clone or push. This also applies to the unwrap() on line 34.
| self.ops.lock().unwrap().clone() | |
| self.ops.lock().unwrap_or_else(|e| e.into_inner()).clone() |
ublue/skillet/crates/cli/src/main.rs
Outdated
| let recorder_system = Recorder::new(system); | ||
| let recorder_files = Recorder::with_ops(files, recorder_system.shared_ops()); | ||
|
|
||
| apply(&recorder_system, &recorder_files).map_err(|e| anyhow!(e))?; |
There was a problem hiding this comment.
The map_err(|e| anyhow!(e)) is redundant. HardeningError implements std::error::Error (via thiserror), and anyhow::Error has a From implementation for any such error. Therefore, the ? operator can perform the conversion automatically. Removing the explicit map_err makes the code cleaner. This also applies to line 91.
| apply(&recorder_system, &recorder_files).map_err(|e| anyhow!(e))?; | |
| apply(&recorder_system, &recorder_files)?; |
ublue/skillet/crates/cli/src/main.rs
Outdated
| .args([ | ||
| "cp", | ||
| &format!("{}:/tmp/ops.yaml", container_name), | ||
| dest_file.to_str().unwrap(), |
There was a problem hiding this comment.
Using .unwrap() on the result of to_str() can cause the program to panic if the path contains non-UTF-8 characters. While unlikely for this specific path, it's more robust to handle the Option and return a Result, especially since anyhow is used throughout this file. You can use .context() to provide a descriptive error message.
| dest_file.to_str().unwrap(), | |
| dest_file.to_str().context("Destination path is not valid UTF-8")?, |
ublue/skillet/crates/cli/src/main.rs
Outdated
| } else { | ||
| info!("Verifying recording..."); | ||
| let temp_dest = tempfile::Builder::new().suffix(".yaml").tempfile()?; | ||
| let temp_path = temp_dest.path().to_str().unwrap(); |
There was a problem hiding this comment.
Using .unwrap() on the result of to_str() can cause the program to panic if the path contains non-UTF-8 characters. It's safer to handle the Option and return a Result. Since anyhow is used, you can use .context() for a clean conversion to a Result.
| let temp_path = temp_dest.path().to_str().unwrap(); | |
| let temp_path = temp_dest.path().to_str().context("Temporary path is not valid UTF-8")?; |
| if let Some(desired_user) = owner { | ||
| let _user = get_user_by_name(desired_user) | ||
| .ok_or_else(|| FileError::UserNotFound(desired_user.to_string()))?; | ||
| if metadata.permissions().mode() & 0o777 != 0 { // Placeholder for real check | ||
| // For ownership we really need to check stat, not just permissions | ||
| // Let's use nix::sys::stat::stat or std::os::unix::fs::MetadataExt | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code appears to be a redundant placeholder. It contains a comment indicating it's a placeholder, an unused _user variable, and a check that doesn't seem to do anything useful for ownership. The correct ownership check is already implemented in the following lines (83-97). This block should be removed to improve clarity and remove dead code.
| use anyhow::{anyhow, Context, Result}; | ||
| use clap::{Parser, Subcommand}; | ||
| use skillet_core::files::LocalFileResource; | ||
| use skillet_core::recorder::Recorder; | ||
| use skillet_core::system::LinuxSystemResource; | ||
| use skillet_hardening::apply; | ||
| use std::fs; | ||
| use std::path::PathBuf; | ||
| use tracing::{info, Level}; | ||
| use tracing_subscriber::FmtSubscriber; | ||
|
|
||
| #[derive(Parser, Debug)] | ||
| #[command(author, version, about, long_about = None)] | ||
| struct Args { | ||
| #[command(subcommand)] | ||
| command: Commands, | ||
|
|
||
| /// Enable verbose logging | ||
| #[arg(short, long, global = true)] | ||
| verbose: bool, | ||
| } | ||
|
|
||
| #[derive(Subcommand, Debug)] | ||
| enum Commands { | ||
| /// Apply configuration | ||
| Apply { | ||
| /// Optional: Output recorded actions to this file path | ||
| #[arg(long)] | ||
| record: Option<PathBuf>, | ||
| }, | ||
| } | ||
|
|
||
| fn main() -> Result<()> { | ||
| let args = Args::parse(); | ||
|
|
||
| let subscriber = FmtSubscriber::builder() | ||
| .with_max_level(if args.verbose { | ||
| Level::DEBUG | ||
| } else { | ||
| Level::INFO | ||
| }) | ||
| .finish(); | ||
|
|
||
| tracing::subscriber::set_global_default(subscriber).expect("setting default subscriber failed"); | ||
|
|
||
| match args.command { | ||
| Commands::Apply { record } => handle_apply(record), | ||
| } | ||
| } | ||
|
|
||
| fn handle_apply(record_path: Option<PathBuf>) -> Result<()> { | ||
| info!("Starting Skillet configuration for beezelbot..."); | ||
|
|
||
| let system = LinuxSystemResource::new(); | ||
| let files = LocalFileResource::new(); | ||
|
|
||
| if let Some(path) = record_path { | ||
| let recorder_system = Recorder::new(system); | ||
| let recorder_files = Recorder::with_ops(files, recorder_system.shared_ops()); | ||
|
|
||
| apply(&recorder_system, &recorder_files).map_err(|e| anyhow!(e))?; | ||
|
|
||
| let ops = recorder_system.get_ops(); | ||
| let yaml = serde_yaml::to_string(&ops)?; | ||
| fs::write(&path, yaml).context("Failed to write recording")?; | ||
| info!("Recording saved to {}", path.display()); | ||
| } else { | ||
| apply(&system, &files).map_err(|e| anyhow!(e))?; | ||
| } | ||
|
|
||
| info!("Configuration applied successfully."); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
There is significant code duplication between this main.rs and the one in crates/cli. The argument parsing, logger setup, and handle_apply function are nearly identical. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider refactoring the common CLI logic into a shared crate or module. Each host-specific binary could then call into this shared logic, providing only the parts that are unique to it.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the skillet configuration tool, a well-structured Rust project with a clear separation of concerns into core, hardening, and CLI crates. The use of a workspace, traits for testability, and a recorder for integration testing are all excellent design choices.
My review focuses on improving robustness, test coverage, and maintainability. I've identified a few areas for improvement:
- Removing some placeholder code in the file resource implementation.
- Handling potential panics from
.unwrap()calls in the test runner. - Expanding test coverage for file metadata.
- Reducing code duplication in host-specific binaries and test mocks.
- A minor suggestion to improve the readability of recorded test data.
Overall, this is a very strong foundation for the project. The changes are well-thought-out and follow the principles outlined in AGENTS.md.
ublue/skillet/crates/cli/src/main.rs
Outdated
| .args([ | ||
| "cp", | ||
| &format!("{}:/tmp/ops.yaml", container_name), | ||
| dest_file.to_str().unwrap(), |
There was a problem hiding this comment.
The use of .unwrap() here can cause the program to panic if the dest_file path contains non-UTF-8 characters. While less common on some systems, file paths on Unix are not guaranteed to be valid UTF-8. It's safer to handle this potential error gracefully using ok_or_else or a similar method.
| dest_file.to_str().unwrap(), | |
| dest_file.to_str().ok_or_else(|| anyhow!("Destination path is not valid UTF-8"))?, |
ublue/skillet/crates/cli/src/main.rs
Outdated
| } else { | ||
| info!("Verifying recording..."); | ||
| let temp_dest = tempfile::Builder::new().suffix(".yaml").tempfile()?; | ||
| let temp_path = temp_dest.path().to_str().unwrap(); |
There was a problem hiding this comment.
Similar to the previous comment, .unwrap() here can cause a panic if the temporary path generated by tempfile is not valid UTF-8. This should be handled gracefully to prevent the test runner from crashing.
| let temp_path = temp_dest.path().to_str().unwrap(); | |
| let temp_path = temp_dest.path().to_str().ok_or_else(|| anyhow!("Temporary path is not valid UTF-8"))?; |
| if let Some(desired_user) = owner { | ||
| let _user = get_user_by_name(desired_user) | ||
| .ok_or_else(|| FileError::UserNotFound(desired_user.to_string()))?; | ||
| if metadata.permissions().mode() & 0o777 != 0 { // Placeholder for real check | ||
| // For ownership we really need to check stat, not just permissions | ||
| // Let's use nix::sys::stat::stat or std::os::unix::fs::MetadataExt | ||
| } | ||
| } |
There was a problem hiding this comment.
This if let block appears to be non-functional placeholder code. The check metadata.permissions().mode() & 0o777 != 0 is not a meaningful way to verify ownership, and the block has no effect. The comments inside even state it's a placeholder. The correct ownership check is already implemented in the subsequent block (lines 83-97). This placeholder code should be removed to avoid confusion and improve code clarity.
| @@ -0,0 +1,70 @@ | |||
| use super::*; | |||
There was a problem hiding this comment.
The existing tests cover file content changes and deletion well. However, there are no tests for ensuring file metadata (mode, owner, group) is correctly applied and idempotent. Since this is a core part of the ensure_file functionality, I recommend adding tests for these scenarios. For example, a test could:
- Create a file with default metadata.
- Call
ensure_filewith specificmode,owner, andgroup. - Assert that the metadata was changed correctly.
- Call
ensure_fileagain with the same parameters and assert that no change was reported (changed == false).
| EnsureFile { | ||
| path: String, | ||
| content_hash: String, | ||
| mode: Option<u32>, |
There was a problem hiding this comment.
The mode is currently stored as a u32. When this is serialized to YAML (as seen in integration_tests/recordings/beezelbot.yaml), an octal mode like 0o644 becomes the decimal 420, which is not intuitive for developers reading the recordings. To improve readability, consider storing the mode as a String in octal format (e.g., "0o644"). This would make the recorded YAML files easier to understand and debug. You would need to update the Recorder to format the u32 into a string before recording.
| mode: Option<u32>, | |
| mode: Option<String>, |
| use std::path::Path; | ||
| use std::sync::{Arc, Mutex}; | ||
|
|
||
| struct MockSystem { |
There was a problem hiding this comment.
The mock implementations for SystemResource (MockSystem) and FileResource (MockFiles) are very useful for testing. However, similar mocks are also needed and defined in other parts of the workspace (e.g., skillet_core::system::tests). To promote reuse and avoid duplication, consider moving these mock implementations to a central place. A good approach would be to define them within the skillet_core crate, perhaps in a skillet_core::test_utils module that is conditionally compiled with #[cfg(test)] or exposed as a feature for testing. This would allow all other crates in the workspace to use a consistent set of mocks for their tests.
| @@ -0,0 +1,73 @@ | |||
| use anyhow::{anyhow, Context, Result}; | |||
There was a problem hiding this comment.
This main.rs file contains a significant amount of boilerplate code for argument parsing and logging setup that is also present in crates/cli/src/main.rs. According to AGENTS.md, the plan is to have binaries per host. To improve maintainability and avoid code duplication as more hosts are added, consider refactoring the common CLI logic into a shared library or module. This would allow each host-specific binary to be much simpler, focusing only on calling the appropriate high-level functions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the initial implementation of Skillet, a Rust-based idempotent host configuration tool. The changes include setting up the Cargo workspace, defining core architectural mandates in AGENTS.md, and implementing basic file and system resource management. Integration tests are also set up using containerized environments. The overall structure is well-thought-out, and the use of traits for resource management promotes testability and modularity. The AGENTS.md document clearly outlines important development guidelines, such as error handling strategies and testing practices. One minor issue was identified regarding the use of expect in a library crate, which goes against the established guidelines.
| }) | ||
| .finish(); | ||
|
|
||
| tracing::subscriber::set_global_default(subscriber).expect("setting default subscriber failed"); |
There was a problem hiding this comment.
The AGENTS.md document explicitly states: "NEVER use unwrap() or expect() in library code. All errors must be propagated and handled." This expect call violates that mandate. As skillet_cli_common is a library crate, this error should be propagated using ? or handled more gracefully, rather than panicking.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the initial structure for the skillet project, a Rust-based idempotent host configuration tool. It sets up a Cargo workspace with core, hardening, cli, cli-common, and hosts/beezelbot crates, defining core architectural mandates in AGENTS.md. The skillet_core crate provides abstractions for file and system resources, including a Recorder for operation logging and test_utils for mocking. The skillet_hardening crate implements sysctl hardening by ensuring a configuration file and restarting the systemd-sysctl service. The main skillet CLI includes apply and test commands, with the test command managing containerized integration tests for recording and verifying configuration actions. A specific host binary, skillet-beezelbot, is also introduced. Review comments highlight issues with using yanked serde_yaml dependencies, code duplication in the handle_apply function, the excessive length and multiple responsibilities of the run_container_test function, incorrect placement of a use statement, and the inappropriate enabling of the test-utils feature in a regular dependency.
ublue/skillet/Cargo.lock
Outdated
|
|
||
| [[package]] | ||
| name = "serde_yaml" | ||
| version = "0.9.34+deprecated" |
There was a problem hiding this comment.
The serde_yaml dependency is locked to version 0.9.34+deprecated, which indicates it is a yanked version. Using yanked versions is risky as they might contain critical bugs or security vulnerabilities. Please run cargo update to resolve this to a non-yanked version.
| tracing-subscriber.workspace = true | ||
| thiserror.workspace = true | ||
| serde.workspace = true | ||
| serde_yaml = "0.9" |
There was a problem hiding this comment.
The specified version 0.9 for serde_yaml resolves to 0.9.34, which is a yanked version. Using yanked versions is risky. Please update your Cargo.lock file by running cargo update, or pin this dependency to a specific, non-yanked version like 0.9.33.
ublue/skillet/crates/cli/src/main.rs
Outdated
| fn handle_apply(record_path: Option<PathBuf>) -> Result<()> { | ||
| info!("Starting Skillet configuration (Agent Mode)..."); | ||
|
|
||
| let system = LinuxSystemResource::new(); | ||
| let files = LocalFileResource::new(); | ||
|
|
||
| if let Some(path) = record_path { | ||
| let recorder_system = Recorder::new(system); | ||
| let recorder_files = Recorder::with_ops(files, recorder_system.shared_ops()); | ||
|
|
||
| apply(&recorder_system, &recorder_files)?; | ||
|
|
||
| let ops = recorder_system.get_ops(); | ||
| let yaml = serde_yaml::to_string(&ops)?; | ||
| fs::write(&path, yaml).context("Failed to write recording")?; | ||
| info!("Recording saved to {}", path.display()); | ||
| } else { | ||
| apply(&system, &files)?; | ||
| } | ||
|
|
||
| info!("Configuration applied successfully."); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This handle_apply function is nearly identical to the handle_apply function in crates/cli-common/src/lib.rs. This code duplication makes maintenance harder and error-prone. Consider refactoring this logic into a shared function within the skillet_cli_common crate that both CLIs can use.
| fn run_container_test(hostname: &str, image: &str, is_record: bool) -> Result<()> { | ||
| // 1. Build binary | ||
| info!("Building skillet workspace..."); | ||
| let build_status = Command::new("cargo") | ||
| .args(["build"]) | ||
| .status() | ||
| .context("Failed to run cargo build")?; | ||
|
|
||
| if !build_status.success() { | ||
| return Err(anyhow!("Build failed")); | ||
| } | ||
|
|
||
| // 2. Locate binary (with fallback) | ||
| let host_binary_name = format!("skillet-{}", hostname); | ||
| let target_debug = PathBuf::from("target/debug"); | ||
|
|
||
| let binary_path = if target_debug.join(&host_binary_name).exists() { | ||
| info!("Found host-specific binary: {}", host_binary_name); | ||
| target_debug.join(&host_binary_name) | ||
| } else { | ||
| info!( | ||
| "Using generic skillet binary (host binary {} not found)", | ||
| host_binary_name | ||
| ); | ||
| target_debug.join("skillet") | ||
| }; | ||
|
|
||
| if !binary_path.exists() { | ||
| return Err(anyhow!( | ||
| "Binary not found at {}. Make sure you run this from workspace root.", | ||
| binary_path.display() | ||
| )); | ||
| } | ||
| let abs_binary_path = fs::canonicalize(&binary_path)?; | ||
|
|
||
| // 3. Start Container | ||
| let container_name = format!("skillet-test-{}", hostname); | ||
| info!( | ||
| "Starting container {} from image {}...", | ||
| container_name, image | ||
| ); | ||
|
|
||
| let _ = Command::new("podman") | ||
| .args(["rm", "-f", &container_name]) | ||
| .output(); | ||
|
|
||
| let run_status = Command::new("podman") | ||
| .args([ | ||
| "run", | ||
| "-d", | ||
| "--rm", | ||
| "--name", | ||
| &container_name, | ||
| "-v", | ||
| &format!("{}:/usr/bin/skillet:ro", abs_binary_path.display()), | ||
| image, | ||
| "sleep", | ||
| "infinity", | ||
| ]) | ||
| .status() | ||
| .context("Failed to start podman container")?; | ||
|
|
||
| if !run_status.success() { | ||
| return Err(anyhow!("Failed to start container")); | ||
| } | ||
|
|
||
| let result = (|| -> Result<()> { | ||
| // Prepare entrypoint script | ||
| let entrypoint_content = include_str!("test_entrypoint.sh"); | ||
| let mut temp_entrypoint = tempfile::Builder::new().suffix(".sh").tempfile()?; | ||
| use std::io::Write; | ||
| temp_entrypoint.write_all(entrypoint_content.as_bytes())?; | ||
| let temp_entrypoint_path = temp_entrypoint | ||
| .path() | ||
| .to_str() | ||
| .ok_or_else(|| anyhow!("Entrypoint path is not valid UTF-8"))?; | ||
|
|
||
| // Copy entrypoint to container | ||
| info!("Copying test entrypoint to container..."); | ||
| let cp_status = Command::new("podman") | ||
| .args([ | ||
| "cp", | ||
| temp_entrypoint_path, | ||
| &format!("{}:/tmp/test_entrypoint.sh", container_name), | ||
| ]) | ||
| .status() | ||
| .context("Failed to copy entrypoint")?; | ||
|
|
||
| if !cp_status.success() { | ||
| return Err(anyhow!("Failed to copy entrypoint to container")); | ||
| } | ||
|
|
||
| // Make executable | ||
| let chmod_status = Command::new("podman") | ||
| .args([ | ||
| "exec", | ||
| &container_name, | ||
| "chmod", | ||
| "+x", | ||
| "/tmp/test_entrypoint.sh", | ||
| ]) | ||
| .status() | ||
| .context("Failed to chmod entrypoint")?; | ||
|
|
||
| if !chmod_status.success() { | ||
| return Err(anyhow!("Failed to chmod entrypoint in container")); | ||
| } | ||
|
|
||
| info!("Executing skillet inside container..."); | ||
| let exec_status = Command::new("podman") | ||
| .args([ | ||
| "exec", | ||
| &container_name, | ||
| "/tmp/test_entrypoint.sh", | ||
| "skillet", | ||
| "apply", | ||
| "--record", | ||
| "/tmp/ops.yaml", | ||
| ]) | ||
| .status() | ||
| .context("Failed to exec skillet")?; | ||
|
|
||
| if !exec_status.success() { | ||
| return Err(anyhow!("skillet apply failed inside container")); | ||
| } | ||
|
|
||
| let dest_dir = PathBuf::from("integration_tests/recordings"); | ||
| fs::create_dir_all(&dest_dir)?; | ||
| let dest_file = dest_dir.join(format!("{}.yaml", hostname)); | ||
|
|
||
| if is_record { | ||
| info!("Copying recording to {}", dest_file.display()); | ||
| let cp_status = Command::new("podman") | ||
| .args([ | ||
| "cp", | ||
| &format!("{}:/tmp/ops.yaml", container_name), | ||
| dest_file | ||
| .to_str() | ||
| .ok_or_else(|| anyhow!("Destination path is not valid UTF-8"))?, | ||
| ]) | ||
| .status()?; | ||
|
|
||
| if !cp_status.success() { | ||
| return Err(anyhow!("Failed to copy recording from container")); | ||
| } | ||
| } else { | ||
| info!("Verifying recording..."); | ||
| let temp_dest = tempfile::Builder::new().suffix(".yaml").tempfile()?; | ||
| let temp_path = temp_dest | ||
| .path() | ||
| .to_str() | ||
| .ok_or_else(|| anyhow!("Temporary path is not valid UTF-8"))?; | ||
|
|
||
| let cp_status = Command::new("podman") | ||
| .args([ | ||
| "cp", | ||
| &format!("{}:/tmp/ops.yaml", container_name), | ||
| temp_path, | ||
| ]) | ||
| .status()?; | ||
| if !cp_status.success() { | ||
| return Err(anyhow!("Failed to copy recording from container")); | ||
| } | ||
|
|
||
| let recorded_content = fs::read_to_string(&dest_file).context(format!( | ||
| "Failed to read existing recording at {}", | ||
| dest_file.display() | ||
| ))?; | ||
| let new_content = fs::read_to_string(temp_path)?; | ||
|
|
||
| let recorded_ops: Vec<ResourceOp> = serde_yaml::from_str(&recorded_content)?; | ||
| let new_ops: Vec<ResourceOp> = serde_yaml::from_str(&new_content)?; | ||
|
|
||
| if recorded_ops != new_ops { | ||
| error!("Recording mismatch!"); | ||
| error!("Expected: {:?}", recorded_ops); | ||
| error!("Actual: {:?}", new_ops); | ||
| return Err(anyhow!( | ||
| "Integration test failed: Actions do not match recording." | ||
| )); | ||
| } else { | ||
| info!("Integration test passed!"); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| })(); | ||
|
|
||
| info!("Stopping container..."); | ||
| let _ = Command::new("podman") | ||
| .args(["kill", &container_name]) | ||
| .output(); | ||
|
|
||
| result | ||
| } |
There was a problem hiding this comment.
The run_container_test function is very long and handles multiple responsibilities (building, container management, test execution, result verification). To improve readability and maintainability, consider breaking it down into smaller, more focused functions. For example:
build_binary()prepare_container()execute_test_in_container()verify_or_record_results()cleanup_container()
The current use of an immediately-invoked closure for cleanup is a good pattern, and it can be retained around the new, smaller functions.
ublue/skillet/crates/cli/src/main.rs
Outdated
| // Prepare entrypoint script | ||
| let entrypoint_content = include_str!("test_entrypoint.sh"); | ||
| let mut temp_entrypoint = tempfile::Builder::new().suffix(".sh").tempfile()?; | ||
| use std::io::Write; |
There was a problem hiding this comment.
The use std::io::Write; statement is located inside the run_container_test function. According to Rust conventions, use statements should be placed at the top of the module to improve code readability and organization. Please move this line to the top of the file with the other use statements.
| edition = "2021" | ||
|
|
||
| [dependencies] | ||
| skillet_core = { workspace = true, features = ["test-utils"] } |
There was a problem hiding this comment.
The test-utils feature for skillet_core is enabled as a regular dependency. This causes test-only code and mocks to be included in the final library artifact, which is not ideal for production builds. This feature should only be enabled for development builds.
To fix this, remove the feature from this line and add skillet_core to the [dev-dependencies] section with the feature enabled, like this:
[dev-dependencies]
tempfile.workspace = true
skillet_core = { workspace = true, features = ["test-utils"] }| skillet_core = { workspace = true, features = ["test-utils"] } | |
| skillet_core.workspace = true |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces 'Skillet,' a Rust-based idempotent host configuration tool. It establishes a workspace structure with crates for core primitives, hardening logic, and a CLI. Key features include idempotent file and system resource management, a recording mechanism for operations, and an integration testing framework using Podman. Feedback focuses on improving security by checking special permission bits, enhancing robustness in binary location and file writing, and optimizing file change detection by comparing sizes before reading content.
| let mut changed = false; | ||
|
|
||
| if let Some(desired_mode) = mode { | ||
| if (metadata.permissions().mode() & 0o777) != desired_mode { |
There was a problem hiding this comment.
The octal mask 0o777 only checks the standard permission bits (rwx). It ignores special bits like setuid, setgid, and the sticky bit. For a security-focused configuration tool, it is safer to use 0o7777 to ensure that these special bits are also in the desired state, preventing unexpected privilege escalation vulnerabilities.
| if (metadata.permissions().mode() & 0o777) != desired_mode { | |
| if (metadata.permissions().mode() & 0o7777) != desired_mode { |
|
|
||
| let ops = recorder_system.get_ops(); | ||
| let yaml = serde_yml::to_string(&ops)?; | ||
| fs::write(&path, yaml)?; |
There was a problem hiding this comment.
fs::write will fail if the parent directory of the recording path does not exist. It is better to ensure the directory exists before attempting to write the file.
| fs::write(&path, yaml)?; | |
| if let Some(parent) = path.parent() { | |
| fs::create_dir_all(parent)?; | |
| } | |
| fs::write(&path, yaml)?; |
ublue/skillet/crates/cli/src/main.rs
Outdated
| let host_binary_name = format!("skillet-{hostname}"); | ||
| let target_debug = PathBuf::from("target/debug"); | ||
|
|
||
| let binary_path = if target_debug.join(&host_binary_name).exists() { | ||
| info!("Found host-specific binary: {host_binary_name}"); | ||
| target_debug.join(&host_binary_name) | ||
| } else { | ||
| info!( | ||
| "Using generic skillet binary (host binary {host_binary_name} not found)" | ||
| ); | ||
| target_debug.join("skillet") | ||
| }; | ||
|
|
||
| if !binary_path.exists() { | ||
| return Err(anyhow!( | ||
| "Binary not found at {}. Make sure you run this from workspace root.", | ||
| binary_path.display() | ||
| )); | ||
| } |
There was a problem hiding this comment.
The tool currently only looks for binaries in target/debug. This will cause it to fail if the project has been built in release mode. It should check both target/release and target/debug to be more robust. Additionally, the logic can be simplified using find_map to avoid redundant checks.
let host_binary_name = format!("skillet-{hostname}");
let binary_path = ["target/release", "target/debug"]
.iter()
.find_map(|dir| {
let p = PathBuf::from(dir).join(&host_binary_name);
if p.exists() { Some(p) } else { None }
})
.or_else(|| {
["target/release", "target/debug"]
.iter()
.find_map(|dir| {
let p = PathBuf::from(dir).join("skillet");
if p.exists() { Some(p) } else { None }
})
})
.ok_or_else(|| anyhow!("Binary not found in target/release or target/debug"))?;| let existing_content = | ||
| fs::read(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | ||
|
|
||
| let mut hasher = Sha256::new(); | ||
| hasher.update(&existing_content); | ||
| let existing_hash = hasher.finalize(); | ||
|
|
||
| let mut new_hasher = Sha256::new(); | ||
| new_hasher.update(content); | ||
| let new_hash = new_hasher.finalize(); | ||
|
|
||
| existing_hash != new_hash |
There was a problem hiding this comment.
Reading the entire existing file into memory to check for changes is inefficient and can lead to high memory usage or crashes on large files. Additionally, hashing the content which is already in memory is redundant. A more efficient approach would be to first compare the file sizes, and then compare the content directly if the sizes match.
| let existing_content = | |
| fs::read(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| let mut hasher = Sha256::new(); | |
| hasher.update(&existing_content); | |
| let existing_hash = hasher.finalize(); | |
| let mut new_hasher = Sha256::new(); | |
| new_hasher.update(content); | |
| let new_hash = new_hasher.finalize(); | |
| existing_hash != new_hash | |
| let metadata = fs::metadata(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| if metadata.len() != content.len() as u64 { | |
| true | |
| } else { | |
| let existing_content = fs::read(path).map_err(|e| FileError::Read(path.display().to_string(), e))?; | |
| existing_content != content | |
| } |
…tion tests - Added skillet_core with idempotent file and system primitives. - Implemented skillet_hardening with sysctl configuration. - Added skillet-beezelbot host binary. - Implemented Podman-based integration testing and recording. - Updated AGENTS.md with quality control mandates.
- Removed unsafe unwrap on Mutex locks in recorder. - Removed redundant map_err in CLI. - Handled potential non-UTF-8 paths safely in CLI. - Removed placeholder ownership code in core. - Refactored shared host CLI logic into skillet_cli_common. - Centralized test mocks in skillet_core::test_utils. - Added file metadata verification tests. - Improved readability of recorded modes by using octal strings.
- Removed expect() in skillet_cli_common and converted to thiserror. - Removed expect() in skillet_cli and used anyhow context. - Adheres to AGENTS.md mandate for library code.
- Runs clippy, unit tests, and integration tests. - Uses path filtering to run only when ublue/skillet/ is changed.
- Moved complex shell logic to 'test_entrypoint.sh'. - Embedded entrypoint script into the binary. - Updated test runner to use the embedded script.
- Added ServiceStart, ServiceStop, ServiceRestart ops. - Implemented systemctl resource in system.rs. - Updated hardening to restart systemd-sysctl on config change. - Updated beezelbot recording with service restart.
- Replace yanked serde_yaml with serde_yml - Refactor run_container_test into smaller, focused functions - Deduplicate handle_apply by moving it to cli-common - Move test-utils feature to dev-dependencies in skillet_hardening - Relocate use statement to top-level
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request initializes the skillet project, a Rust-based tool for idempotent host configuration. It establishes a workspace with core libraries for managing system resources, a hardening module for security baselines, and a CLI that supports both configuration application and containerized integration testing. Feedback focuses on improving security by using DirBuilder to avoid permission windows during directory creation, adhering to project mandates by replacing shell commands with native crates, and ensuring parent directories exist before file operations.
| let mut changed = false; | ||
|
|
||
| if !path.exists() { | ||
| fs::create_dir_all(path).map_err(FileError::Io)?; |
There was a problem hiding this comment.
Using fs::create_dir_all followed by apply_metadata creates a window where the directory exists with default umask permissions. This can be a security risk for sensitive directories. Consider using std::fs::DirBuilder with the mode extension to create the directory with the correct permissions from the start.
| fs::create_dir_all(path).map_err(FileError::Io)?; | |
| use std::os::unix::fs::DirBuilderExt; | |
| let mut builder = fs::DirBuilder::new(); | |
| builder.recursive(true); | |
| if let Some(m) = mode { | |
| builder.mode(m); | |
| } | |
| builder.create(path).map_err(FileError::Io)?; |
|
|
||
| fn run_systemctl(action: &str, name: &str) -> Result<(), SystemError> { | ||
| info!("Running systemctl {action} {name}"); | ||
| let output = Command::new("systemctl").arg(action).arg(name).output()?; |
There was a problem hiding this comment.
This implementation uses Command to shell out to systemctl, which violates the project mandate to 'Prioritize Crates over Shell-out' (AGENTS.md, line 11). Consider using a Rust crate to interact with systemd (e.g., via DBus) to improve safety and portability.
| let content = include_bytes!("../files/sysctl.boxy.conf"); | ||
| let path = Path::new("/etc/sysctl.d/99-hardening.conf"); | ||
|
|
||
| let changed = files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; |
There was a problem hiding this comment.
The /etc/sysctl.d directory may not exist on all systems (e.g., minimal containers). Since ensure_file fails if the parent directory is missing (as implemented in skillet_core), you should ensure the directory exists before attempting to write the configuration file.
| let changed = files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; | |
| files.ensure_directory(Path::new("/etc/sysctl.d"), Some(0o755), Some("root"), Some("root"))?; | |
| let changed = files.ensure_file(path, content, Some(0o644), Some("root"), Some("root"))?; |
1 participant
This PR introduces Skillet, a Rust-based idempotent host configuration tool with containerized integration tests. (Re-opened with target/ directory removed)