Conversation
…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.
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 Skillet, a new Rust-based idempotent host configuration tool. It establishes a modular architecture with 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 project, a new Rust-based host configuration tool. The initial structure with a multi-crate workspace, core primitives using traits, and a recorder for integration testing provides a solid foundation. However, there are several areas for improvement. I've identified a critical architectural issue with significant code duplication between the generic CLI and the host-specific binary, which should be addressed by refactoring common logic into a shared library. Additionally, there are some violations of the project's own mandates from AGENTS.md, such as using unwrap() in library code. I've also noted some dead code and opportunities to improve robustness and maintainability in the CLI implementation.
| 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.
This file is almost an identical copy of crates/cli/src/main.rs. This significant code duplication introduces a major maintenance burden and is not scalable, which goes against the modular design principle outlined in AGENTS.md.
A better approach would be to:
- Create a shared library crate (e.g.,
skillet_cli_lib) that contains all the common command-line parsing, logging, and execution logic. - The host-specific binaries (
skillet-beezelbot) and the generic CLI (skillet) would then become very thin wrappers around this shared library, only providing the specific configurations or modules to apply.
This would eliminate duplication and centralize the core CLI logic, making the project much easier to maintain and extend.
| 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 non-functional placeholder. The if condition is unlikely to be useful, and the comments indicate it's incomplete. This should be removed to avoid confusion and clean up the implementation.
| } | ||
|
|
||
| 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() result can lead to a panic if the lock is poisoned. This violates the project's mandate in AGENTS.md to avoid unwrap() and expect() in library code. The PoisonError should be propagated.
This will require changing the function signature to return a Result. A similar change is needed for record() on line 34, which will in turn affect the signatures of the FileResource and SystemResource trait methods implemented for Recorder.
For example:
// You would need a custom error type
#[derive(Error, Debug)]
pub enum RecorderError {
#[error("Mutex lock was poisoned")]
MutexPoisoned,
}
// ... in impl<T> Recorder<T>
pub fn get_ops(&self) -> Result<Vec<ResourceOp>, RecorderError> {
self.ops
.lock()
.map(|guard| guard.clone())
.map_err(|_| RecorderError::MutexPoisoned)
}| 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<()> { | ||
| info!("Executing skillet inside container..."); | ||
| // Use 'skillet apply' directly as it's the interface for all our binaries now | ||
| // We ensure /etc/sysctl.d exists because many minimal container images lack it. | ||
| let exec_status = Command::new("podman") | ||
| .args([ | ||
| "exec", | ||
| &container_name, | ||
| "sh", | ||
| "-c", | ||
| "mkdir -p /etc/sysctl.d && 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().unwrap(), | ||
| ]) | ||
| .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().unwrap(); | ||
|
|
||
| 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 function run_container_test is very long and handles multiple distinct responsibilities (building, starting container, executing command, processing results, cleanup). This reduces readability and makes it harder to maintain. Consider refactoring this into several smaller, more focused functions. For example:
build_binaries()prepare_container_environment()execute_in_container()process_test_results()cleanup_container()
This would make the logic clearer and easier to test or modify in the future.
| .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 a panic if the path contains non-UTF-8 characters. Since anyhow is used in this crate, it's better to handle this potential error by converting the Option to a Result and propagating it. This also applies to line 220.
| dest_file.to_str().unwrap(), | |
| dest_file.to_str().ok_or_else(|| anyhow::anyhow!("destination path is not valid UTF-8"))?, |
| let output = Command::new("groupadd") | ||
| .arg(name) | ||
| // .arg("-r") // System group? Maybe make it an option? | ||
| // For now, simple group creation. | ||
| .output()?; |
There was a problem hiding this comment.
This implementation shells out to the groupadd command. The AGENTS.md file states a preference for using Rust crates over shelling out. While there may not be a pure Rust crate for user/group creation that doesn't wrap the CLI tools, it's worth confirming if alternatives using libc directly were considered to better align with the project's stated goals. If shelling out is the only practical option, it might be good to add a comment here explaining the rationale.
1 participant
This PR introduces Skillet, a Rust-based idempotent host configuration tool with containerized integration tests.
Features: