From fde1ef5c58f5120b49d8884249fa457846060299 Mon Sep 17 00:00:00 2001 From: Solomon Date: Mon, 14 Jul 2025 16:15:29 +0100 Subject: [PATCH] Setup script error output (#166) * Improved the setup script error reporting UX Show stdout/stderr output from setup script even when it exits with non-zero error code. * UI: don't display the internal maker "---STDERR_CHUNK_BOUNDARY---" * fmt * Address review feedback --- backend/Cargo.toml | 4 +- backend/src/executor.rs | 17 ++++- backend/src/executors/setup_script.rs | 77 +++++++++++++++++++++++ backend/src/models/execution_process.rs | 19 +++++- backend/src/routes/task_attempts.rs | 83 ++++++++++++++++--------- backend/src/services/process_service.rs | 6 ++ 6 files changed, 172 insertions(+), 34 deletions(-) diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 3cc05e2f..b3ca80a0 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -24,7 +24,7 @@ tracing-subscriber = { workspace = true } sqlx = { version = "0.8.6", features = ["runtime-tokio-rustls", "sqlite", "chrono", "uuid"] } chrono = { version = "0.4", features = ["serde"] } uuid = { version = "1.0", features = ["v4", "serde"] } -ts-rs = { version = "9.0", features = ["uuid-impl", "chrono-impl"] } +ts-rs = { version = "9.0", features = ["uuid-impl", "chrono-impl", "no-serde-warnings"] } dirs = "5.0" git2 = "0.18" async-trait = "0.1" @@ -56,7 +56,7 @@ tempfile = "3.8" [build-dependencies] dotenv = "0.15" -ts-rs = { version = "9.0", features = ["uuid-impl", "chrono-impl"] } +ts-rs = { version = "9.0", features = ["uuid-impl", "chrono-impl", "no-serde-warnings"] } [profile.release] debug = true diff --git a/backend/src/executor.rs b/backend/src/executor.rs index 262ade85..39c052ea 100644 --- a/backend/src/executor.rs +++ b/backend/src/executor.rs @@ -8,6 +8,7 @@ use uuid::Uuid; use crate::executors::{ AmpExecutor, ClaudeExecutor, EchoExecutor, GeminiExecutor, OpencodeExecutor, + SetupScriptExecutor, }; // Constants for database streaming @@ -345,6 +346,7 @@ pub enum ExecutorConfig { Amp, Gemini, Opencode, + SetupScript { script: String }, // Future executors can be added here // Shell { command: String }, // Docker { image: String, command: String }, @@ -368,6 +370,9 @@ impl FromStr for ExecutorConfig { "amp" => Ok(ExecutorConfig::Amp), "gemini" => Ok(ExecutorConfig::Gemini), "opencode" => Ok(ExecutorConfig::Opencode), + "setup_script" => Ok(ExecutorConfig::SetupScript { + script: "setup script".to_string(), + }), _ => Err(format!("Unknown executor type: {}", s)), } } @@ -381,6 +386,9 @@ impl ExecutorConfig { ExecutorConfig::Amp => Box::new(AmpExecutor), ExecutorConfig::Gemini => Box::new(GeminiExecutor), ExecutorConfig::Opencode => Box::new(OpencodeExecutor), + ExecutorConfig::SetupScript { script } => { + Box::new(SetupScriptExecutor::new(script.clone())) + } } } @@ -395,6 +403,7 @@ impl ExecutorConfig { ExecutorConfig::Gemini => { dirs::home_dir().map(|home| home.join(".gemini").join("settings.json")) } + ExecutorConfig::SetupScript { .. } => None, } } @@ -406,12 +415,16 @@ impl ExecutorConfig { ExecutorConfig::Claude => Some(vec!["mcpServers"]), ExecutorConfig::Amp => Some(vec!["amp", "mcpServers"]), // Nested path for Amp ExecutorConfig::Gemini => Some(vec!["mcpServers"]), + ExecutorConfig::SetupScript { .. } => None, // Setup scripts don't support MCP } } /// Check if this executor supports MCP configuration pub fn supports_mcp(&self) -> bool { - !matches!(self, ExecutorConfig::Echo) + !matches!( + self, + ExecutorConfig::Echo | ExecutorConfig::SetupScript { .. } + ) } /// Get the display name for this executor @@ -422,6 +435,7 @@ impl ExecutorConfig { ExecutorConfig::Claude => "Claude", ExecutorConfig::Amp => "Amp", ExecutorConfig::Gemini => "Gemini", + ExecutorConfig::SetupScript { .. } => "Setup Script", } } } @@ -434,6 +448,7 @@ impl std::fmt::Display for ExecutorConfig { ExecutorConfig::Amp => "amp", ExecutorConfig::Gemini => "gemini", ExecutorConfig::Opencode => "opencode", + ExecutorConfig::SetupScript { .. } => "setup_script", }; write!(f, "{}", s) } diff --git a/backend/src/executors/setup_script.rs b/backend/src/executors/setup_script.rs index fd632d03..e1b5b238 100644 --- a/backend/src/executors/setup_script.rs +++ b/backend/src/executors/setup_script.rs @@ -14,6 +14,12 @@ pub struct SetupScriptExecutor { pub script: String, } +impl SetupScriptExecutor { + pub fn new(script: String) -> Self { + Self { script } + } +} + #[async_trait] impl Executor for SetupScriptExecutor { async fn spawn( @@ -50,4 +56,75 @@ impl Executor for SetupScriptExecutor { Ok(child) } + + /// Normalize setup script logs into a readable format + fn normalize_logs( + &self, + logs: &str, + _worktree_path: &str, + ) -> Result { + let mut entries = Vec::new(); + + // Add script command as first entry + entries.push(crate::executor::NormalizedEntry { + timestamp: None, + entry_type: crate::executor::NormalizedEntryType::SystemMessage, + content: format!("Executing setup script:\n{}", self.script), + metadata: None, + }); + + // Process the logs - split by lines and create entries + if !logs.trim().is_empty() { + let lines: Vec<&str> = logs.lines().collect(); + let mut current_chunk = String::new(); + + for line in lines { + current_chunk.push_str(line); + current_chunk.push('\n'); + + // Create entry for every 10 lines or when we encounter an error-like line + if current_chunk.lines().count() >= 10 + || line.to_lowercase().contains("error") + || line.to_lowercase().contains("failed") + || line.to_lowercase().contains("exception") + { + let entry_type = if line.to_lowercase().contains("error") + || line.to_lowercase().contains("failed") + || line.to_lowercase().contains("exception") + { + crate::executor::NormalizedEntryType::ErrorMessage + } else { + crate::executor::NormalizedEntryType::SystemMessage + }; + + entries.push(crate::executor::NormalizedEntry { + timestamp: Some(chrono::Utc::now().to_rfc3339()), + entry_type, + content: current_chunk.trim().to_string(), + metadata: None, + }); + + current_chunk.clear(); + } + } + + // Add any remaining content + if !current_chunk.trim().is_empty() { + entries.push(crate::executor::NormalizedEntry { + timestamp: Some(chrono::Utc::now().to_rfc3339()), + entry_type: crate::executor::NormalizedEntryType::SystemMessage, + content: current_chunk.trim().to_string(), + metadata: None, + }); + } + } + + Ok(crate::executor::NormalizedConversation { + entries, + session_id: None, + executor_type: "setup_script".to_string(), + prompt: Some(self.script.clone()), + summary: None, + }) + } } diff --git a/backend/src/models/execution_process.rs b/backend/src/models/execution_process.rs index 1289e70f..de2d6a0e 100644 --- a/backend/src/models/execution_process.rs +++ b/backend/src/models/execution_process.rs @@ -1,11 +1,27 @@ use chrono::{DateTime, Utc}; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Serialize, Serializer}; use sqlx::{FromRow, SqlitePool, Type}; use ts_rs::TS; use uuid::Uuid; use crate::app_state::ExecutionType; +/// Filter out stderr boundary markers from output +fn filter_stderr_boundary_markers(stderr: &Option) -> Option { + stderr + .as_ref() + .map(|s| s.replace("---STDERR_CHUNK_BOUNDARY---", "")) +} + +/// Custom serializer for stderr field that filters out boundary markers +fn serialize_filtered_stderr(stderr: &Option, serializer: S) -> Result +where + S: Serializer, +{ + let filtered = filter_stderr_boundary_markers(stderr); + filtered.serialize(serializer) +} + #[derive(Debug, Clone, Type, Serialize, Deserialize, PartialEq, TS)] #[sqlx(type_name = "execution_process_status", rename_all = "lowercase")] #[serde(rename_all = "lowercase")] @@ -59,6 +75,7 @@ pub struct ExecutionProcess { pub args: Option, // JSON array of arguments pub working_directory: String, pub stdout: Option, + #[serde(serialize_with = "serialize_filtered_stderr")] pub stderr: Option, pub exit_code: Option, pub started_at: DateTime, diff --git a/backend/src/routes/task_attempts.rs b/backend/src/routes/task_attempts.rs index 7bcfd0ec..6227556a 100644 --- a/backend/src/routes/task_attempts.rs +++ b/backend/src/routes/task_attempts.rs @@ -13,7 +13,9 @@ use crate::{ executor::{ExecutorConfig, NormalizedConversation, NormalizedEntry, NormalizedEntryType}, models::{ config::Config, - execution_process::{ExecutionProcess, ExecutionProcessStatus, ExecutionProcessSummary}, + execution_process::{ + ExecutionProcess, ExecutionProcessStatus, ExecutionProcessSummary, ExecutionProcessType, + }, executor_session::ExecutorSession, task::Task, task_attempt::{ @@ -1215,22 +1217,33 @@ pub async fn get_execution_process_normalized_logs( if !stdout.trim().is_empty() { // Determine executor type and create appropriate executor for normalization let executor_type = process.executor_type.as_deref().unwrap_or("unknown"); - let executor_config = match executor_type { - "amp" => ExecutorConfig::Amp, - "claude" => ExecutorConfig::Claude, - "echo" => ExecutorConfig::Echo, - "gemini" => ExecutorConfig::Gemini, - "opencode" => ExecutorConfig::Opencode, - _ => { - tracing::warn!( - "Unsupported executor type: {}, cannot normalize logs properly", - executor_type - ); - return Ok(ResponseJson(ApiResponse { - success: false, - data: None, - message: Some(format!("Unsupported executor type: {}", executor_type)), - })); + + let executor_config = if process.process_type == ExecutionProcessType::SetupScript { + // For setup scripts, use the setup script executor + ExecutorConfig::SetupScript { + script: executor_session + .as_ref() + .and_then(|s| s.prompt.clone()) + .unwrap_or_else(|| "setup script".to_string()), + } + } else { + match executor_type { + "amp" => ExecutorConfig::Amp, + "claude" => ExecutorConfig::Claude, + "echo" => ExecutorConfig::Echo, + "gemini" => ExecutorConfig::Gemini, + "opencode" => ExecutorConfig::Opencode, + _ => { + tracing::warn!( + "Unsupported executor type: {}, cannot normalize logs properly", + executor_type + ); + return Ok(ResponseJson(ApiResponse { + success: false, + data: None, + message: Some(format!("Unsupported executor type: {}", executor_type)), + })); + } } }; @@ -1248,9 +1261,9 @@ pub async fn get_execution_process_normalized_logs( } Err(_) => { tracing::debug!( - "Working directory {} no longer exists, using stored path for normalization", - process.working_directory - ); + "Working directory {} no longer exists, using stored path for normalization", + process.working_directory + ); process.working_directory.clone() } }; @@ -1292,12 +1305,16 @@ pub async fn get_execution_process_normalized_logs( for chunk in chunks { let chunk_trimmed = chunk.trim(); if !chunk_trimmed.is_empty() { - stderr_entries.push(NormalizedEntry { - timestamp: Some(chrono::Utc::now().to_rfc3339()), - entry_type: NormalizedEntryType::ErrorMessage, - content: chunk_trimmed.to_string(), - metadata: None, - }); + // Filter out any remaining boundary markers from the chunk content + let filtered_content = chunk_trimmed.replace("---STDERR_CHUNK_BOUNDARY---", ""); + if !filtered_content.trim().is_empty() { + stderr_entries.push(NormalizedEntry { + timestamp: Some(chrono::Utc::now().to_rfc3339()), + entry_type: NormalizedEntryType::ErrorMessage, + content: filtered_content.trim().to_string(), + metadata: None, + }); + } } } @@ -1323,13 +1340,19 @@ pub async fn get_execution_process_normalized_logs( }); // Create final normalized conversation + let executor_type = if process.process_type == ExecutionProcessType::SetupScript { + "setup_script".to_string() + } else { + process + .executor_type + .clone() + .unwrap_or("unknown".to_string()) + }; + let normalized_conversation = NormalizedConversation { entries: all_entries, session_id: None, - executor_type: process - .executor_type - .clone() - .unwrap_or("unknown".to_string()), + executor_type, prompt: executor_session.as_ref().and_then(|s| s.prompt.clone()), summary: executor_session.as_ref().and_then(|s| s.summary.clone()), }; diff --git a/backend/src/services/process_service.rs b/backend/src/services/process_service.rs index 8d1cbbb0..39387c04 100644 --- a/backend/src/services/process_service.rs +++ b/backend/src/services/process_service.rs @@ -673,6 +673,7 @@ impl ProcessService { crate::executor::ExecutorConfig::Amp => "amp", crate::executor::ExecutorConfig::Gemini => "gemini", crate::executor::ExecutorConfig::Opencode => "opencode", + crate::executor::ExecutorConfig::SetupScript { .. } => "setup_script", }; ( "executor".to_string(), @@ -687,6 +688,7 @@ impl ProcessService { crate::executor::ExecutorConfig::Amp => "amp", crate::executor::ExecutorConfig::Gemini => "gemini", crate::executor::ExecutorConfig::Opencode => "opencode", + crate::executor::ExecutorConfig::SetupScript { .. } => "setup_script", }; ( "followup_executor".to_string(), @@ -847,6 +849,10 @@ impl ProcessService { return Err(TaskAttemptError::TaskNotFound); // No session ID for followup } } + crate::executor::ExecutorConfig::SetupScript { .. } => { + // Setup scripts don't support followup, use regular setup script + config.create_executor() + } }; executor