From aa3366a797d9951adc835f390047b6aeee009fa2 Mon Sep 17 00:00:00 2001 From: bahdotsh Date: Wed, 1 Apr 2026 18:59:31 +0530 Subject: [PATCH] fix: correct multiple bugs found during full codebase review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It turns out that build_image_inner() in docker.rs was calling .elapsed() on a SystemTime to compute the tar mtime. That gives you "seconds since modification" — which is *not* what mtime means. Mtime is seconds since the Unix epoch. The fix is .duration_since(UNIX_EPOCH) like a normal person would use. While at it, the docker logs() call was passing None for options, which means it wasn't actually requesting stdout or stderr. So we were collecting logs from a stream that might not have any. Explicitly set stdout: true and stderr: true. The emulation runtime had a fun behavioral mismatch with Docker and Podman: it returned Err on non-zero exit codes, swallowing all stdout/stderr output. Docker and Podman return Ok with the exit code and let the caller decide what to do. The engine already handles non-zero exit codes in the Ok path, so the emulation was just silently eating useful output for no reason. The UI had a bounds check in next_job() that was mysteriously absent from previous_job() — the kind of inconsistency that waits patiently for someone to hit a stale workflow index and get a panic. Added the same .filter() guard. String slicing in the log processor wasn't checking char boundaries, which is fine until someone's log contains a multi-byte UTF-8 character before a bracket. Added is_char_boundary() checks. Step validation was accepting steps with only a 'name' field and no 'uses' or 'run', which is not a valid step in GitHub Actions. Fixed the validation to require at least one of the two fields that actually *do* something. Replaced .expect() calls on directory reads in main.rs with proper error handling. Panicking because a directory isn't readable is not great user experience. --- crates/executor/src/docker.rs | 11 ++++-- crates/runtime/src/emulation.rs | 60 +-------------------------------- crates/ui/src/app/state.rs | 6 ++-- crates/ui/src/log_processor.rs | 8 +++-- crates/validators/src/steps.rs | 6 ++-- crates/wrkflw/src/main.rs | 18 +++++++--- 6 files changed, 34 insertions(+), 75 deletions(-) diff --git a/crates/executor/src/docker.rs b/crates/executor/src/docker.rs index 435fc4b..089fad9 100644 --- a/crates/executor/src/docker.rs +++ b/crates/executor/src/docker.rs @@ -1009,10 +1009,15 @@ impl DockerRuntime { }; // Get logs with a timeout + let log_options = Some(bollard::container::LogsOptions:: { + stdout: true, + stderr: true, + ..Default::default() + }); let logs_result = tokio::time::timeout( std::time::Duration::from_secs(10), self.docker - .logs::(&container.id, None) + .logs(&container.id, log_options) .collect::>(), ) .await; @@ -1112,10 +1117,10 @@ impl DockerRuntime { e )) })? - .elapsed() + .duration_since(std::time::UNIX_EPOCH) .map_err(|e| { ContainerError::ContainerExecution(format!( - "Failed to get elapsed time since modification: {}", + "Failed to convert modification time to Unix timestamp: {}", e )) })? diff --git a/crates/runtime/src/emulation.rs b/crates/runtime/src/emulation.rs index ef649bb..064725c 100644 --- a/crates/runtime/src/emulation.rs +++ b/crates/runtime/src/emulation.rs @@ -271,23 +271,6 @@ impl ContainerRuntime for EmulationRuntime { exit_code )); - if exit_code != 0 { - let mut error_details = format!( - "Command failed with exit code: {}\nCommand: {}\n\nError output:\n{}", - exit_code, command_str, error - ); - - // Add environment variables to error details - error_details.push_str("\n\nEnvironment variables:\n"); - for (key, value) in env_vars { - if key.starts_with("GITHUB_") || key.starts_with("CI_") { - error_details.push_str(&format!("{}={}\n", key, value)); - } - } - - return Err(ContainerError::ContainerExecution(error_details)); - } - return Ok(ContainerOutput { stdout: output, stderr: error, @@ -354,27 +337,6 @@ impl ContainerRuntime for EmulationRuntime { wrkflw_logging::debug(&format!("Command exit code: {}", exit_code)); - if exit_code != 0 { - let mut error_details = format!( - "Command failed with exit code: {}\nCommand: {}\n\nError output:\n{}", - exit_code, command_str, error - ); - - // Add environment variables to error details - error_details.push_str("\n\nEnvironment variables:\n"); - for (key, value) in env_vars { - if key.starts_with("GITHUB_") - || key.starts_with("RUST") - || key.starts_with("CARGO") - || key.starts_with("CI_") - { - error_details.push_str(&format!("{}={}\n", key, value)); - } - } - - return Err(ContainerError::ContainerExecution(error_details)); - } - return Ok(ContainerOutput { stdout: output, stderr: error, @@ -409,28 +371,8 @@ impl ContainerRuntime for EmulationRuntime { wrkflw_logging::debug(&format!("Command completed with exit code: {}", exit_code)); - if exit_code != 0 { - let mut error_details = format!( - "Command failed with exit code: {}\nCommand: {}\n\nError output:\n{}", - exit_code, command_str, error - ); - - // Add environment variables to error details - error_details.push_str("\n\nEnvironment variables:\n"); - for (key, value) in env_vars { - if key.starts_with("GITHUB_") || key.starts_with("CI_") { - error_details.push_str(&format!("{}={}\n", key, value)); - } - } - - return Err(ContainerError::ContainerExecution(error_details)); - } - Ok(ContainerOutput { - stdout: format!( - "Emulated container execution with command: {}\n\nOutput:\n{}", - command_str, output - ), + stdout: output, stderr: error, exit_code, }) diff --git a/crates/ui/src/app/state.rs b/crates/ui/src/app/state.rs index 842b200..5850fd0 100644 --- a/crates/ui/src/app/state.rs +++ b/crates/ui/src/app/state.rs @@ -304,12 +304,10 @@ impl App { pub fn previous_job(&mut self) { let current_workflow_idx = self .current_execution - .or_else(|| self.workflow_list_state.selected()); + .or_else(|| self.workflow_list_state.selected()) + .filter(|&idx| idx < self.workflows.len()); if let Some(workflow_idx) = current_workflow_idx { - if workflow_idx >= self.workflows.len() { - return; - } if let Some(execution) = &self.workflows[workflow_idx].execution_details { if execution.jobs.is_empty() { diff --git a/crates/ui/src/log_processor.rs b/crates/ui/src/log_processor.rs index 15d12c5..f16fc90 100644 --- a/crates/ui/src/log_processor.rs +++ b/crates/ui/src/log_processor.rs @@ -202,7 +202,7 @@ impl LogProcessor { // Extract timestamp from log format [HH:MM:SS] let timestamp = if log_line.starts_with('[') && log_line.contains(']') { let end = log_line.find(']').unwrap_or(0); - if end > 1 { + if end > 1 && log_line.is_char_boundary(1) && log_line.is_char_boundary(end) { log_line[1..end].to_string() } else { "??:??:??".to_string() @@ -240,7 +240,11 @@ impl LogProcessor { // Extract content after timestamp let content = if log_line.starts_with('[') && log_line.contains(']') { let start = log_line.find(']').unwrap_or(0) + 1; - log_line[start..].trim() + if log_line.is_char_boundary(start) { + log_line[start..].trim() + } else { + log_line + } } else { log_line }; diff --git a/crates/validators/src/steps.rs b/crates/validators/src/steps.rs index 4b11433..941081c 100644 --- a/crates/validators/src/steps.rs +++ b/crates/validators/src/steps.rs @@ -8,12 +8,12 @@ pub fn validate_steps(steps: &[Value], job_name: &str, result: &mut ValidationRe for (i, step) in steps.iter().enumerate() { if let Some(step_map) = step.as_mapping() { - if !step_map.contains_key(Value::String("name".to_string())) - && !step_map.contains_key(Value::String("uses".to_string())) + // A step must have either 'uses' or 'run' (name alone is not sufficient) + if !step_map.contains_key(Value::String("uses".to_string())) && !step_map.contains_key(Value::String("run".to_string())) { result.add_issue(format!( - "Job '{}', step {}: Missing 'name', 'uses', or 'run' field", + "Job '{}', step {}: Missing required 'uses' or 'run' field", job_name, i + 1 )); diff --git a/crates/wrkflw/src/main.rs b/crates/wrkflw/src/main.rs index fbd8579..d5765d3 100644 --- a/crates/wrkflw/src/main.rs +++ b/crates/wrkflw/src/main.rs @@ -338,8 +338,13 @@ async fn main() { if validate_path.is_dir() { // Validate all workflow files in the directory - let entries = std::fs::read_dir(&validate_path) - .expect("Failed to read directory") + let entries = match std::fs::read_dir(&validate_path) { + Ok(rd) => rd, + Err(e) => { + eprintln!("Failed to read directory {}: {}", validate_path.display(), e); + std::process::exit(1); + } + } .filter_map(|entry| entry.ok()) .filter(|entry| { entry.path().is_file() @@ -648,8 +653,13 @@ fn list_workflows_and_pipelines(verbose: bool) { if github_path.exists() && github_path.is_dir() { println!("GitHub Workflows:"); - let entries = std::fs::read_dir(&github_path) - .expect("Failed to read directory") + let entries = match std::fs::read_dir(&github_path) { + Ok(rd) => rd, + Err(e) => { + eprintln!("Failed to read directory {}: {}", github_path.display(), e); + return; + } + } .filter_map(|entry| entry.ok()) .filter(|entry| { entry.path().is_file()