From 52e2490969310b868a37bfdc11556bdc19781366 Mon Sep 17 00:00:00 2001 From: Lifei Zhou Date: Tue, 27 Jan 2026 20:38:34 +1100 Subject: [PATCH] removed the line number in the error message and update the internal recipe template name for minininja --- crates/goose-server/src/routes/recipe.rs | 6 +- crates/goose/src/agents/types.rs | 65 +++++++++++++++++ crates/goose/src/recipe/build_recipe/tests.rs | 26 +++++++ crates/goose/src/recipe/mod.rs | 73 ++++++++++++++++--- crates/goose/src/recipe/template_recipe.rs | 2 +- crates/goose/src/recipe/validate_recipe.rs | 13 ++++ 6 files changed, 169 insertions(+), 16 deletions(-) diff --git a/crates/goose-server/src/routes/recipe.rs b/crates/goose-server/src/routes/recipe.rs index 7e2875c35d71..2696c2b5e128 100644 --- a/crates/goose-server/src/routes/recipe.rs +++ b/crates/goose-server/src/routes/recipe.rs @@ -8,7 +8,7 @@ use axum::routing::get; use axum::{extract::State, http::StatusCode, routing::post, Json, Router}; use goose::recipe::local_recipes; use goose::recipe::validate_recipe::validate_recipe_template_from_content; -use goose::recipe::Recipe; +use goose::recipe::{strip_error_location, Recipe}; use goose::{recipe_deeplink, slash_commands}; use serde::{Deserialize, Serialize}; @@ -443,7 +443,7 @@ async fn save_recipe( payload: Result, JsonRejection>, ) -> Result, ErrorResponse> { let Json(raw_json) = payload.map_err(json_rejection_to_error_response)?; - let request = deserialize_save_recipe_request(raw_json)?; + let request: SaveRecipeRequest = deserialize_save_recipe_request(raw_json)?; let has_security_warnings = request.recipe.check_for_security_warnings(); if has_security_warnings { return Err(ErrorResponse { @@ -492,7 +492,7 @@ fn deserialize_save_recipe_request(value: Value) -> Result = deserialize_with_path(&mut deserializer); result.map_err(|err| { let path = err.path().to_string(); - let inner = err.into_inner(); + let inner = strip_error_location(&err.into_inner().to_string()); let message = if path.is_empty() { format!("Save recipe validation failed: {}", inner) } else { diff --git a/crates/goose/src/agents/types.rs b/crates/goose/src/agents/types.rs index 13138a7fb732..beec0373c8ba 100644 --- a/crates/goose/src/agents/types.rs +++ b/crates/goose/src/agents/types.rs @@ -93,3 +93,68 @@ pub struct SessionConfig { #[serde(skip_serializing_if = "Option::is_none")] pub retry_config: Option, } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_retry_config_validate_success() { + let config = RetryConfig { + max_retries: 3, + checks: vec![], + on_failure: None, + timeout_seconds: Some(60), + on_failure_timeout_seconds: Some(120), + }; + assert!(config.validate().is_ok()); + } + + #[test] + fn test_retry_config_validate_max_retries_zero() { + let config = RetryConfig { + max_retries: 0, + checks: vec![], + on_failure: None, + timeout_seconds: None, + on_failure_timeout_seconds: None, + }; + let result = config.validate(); + assert!(result.is_err()); + assert_eq!(result.unwrap_err(), "max_retries must be greater than 0"); + } + + #[test] + fn test_retry_config_validate_timeout_zero() { + let config = RetryConfig { + max_retries: 3, + checks: vec![], + on_failure: None, + timeout_seconds: Some(0), + on_failure_timeout_seconds: None, + }; + let result = config.validate(); + assert!(result.is_err()); + assert_eq!( + result.unwrap_err(), + "timeout_seconds must be greater than 0 if specified" + ); + } + + #[test] + fn test_retry_config_validate_on_failure_timeout_zero() { + let config = RetryConfig { + max_retries: 3, + checks: vec![], + on_failure: None, + timeout_seconds: None, + on_failure_timeout_seconds: Some(0), + }; + let result = config.validate(); + assert!(result.is_err()); + assert_eq!( + result.unwrap_err(), + "on_failure_timeout_seconds must be greater than 0 if specified" + ); + } +} diff --git a/crates/goose/src/recipe/build_recipe/tests.rs b/crates/goose/src/recipe/build_recipe/tests.rs index c59dd4598094..6905314defb6 100644 --- a/crates/goose/src/recipe/build_recipe/tests.rs +++ b/crates/goose/src/recipe/build_recipe/tests.rs @@ -638,3 +638,29 @@ parameters: } } } + +#[test] +fn test_build_recipe_from_template_invalid_retry_config() { + let instructions_and_parameters = r#" + "instructions": "Test instructions", + "retry": { + "max_retries": 0, + "checks": [] + }"#; + let (_temp_dir, recipe_content, recipe_dir) = setup_recipe_file(instructions_and_parameters); + + let build_recipe_result = + build_recipe_from_template(recipe_content, &recipe_dir, Vec::new(), NO_USER_PROMPT); + assert!(build_recipe_result.is_err()); + let err = build_recipe_result.unwrap_err(); + + match err { + RecipeError::TemplateRendering { source } => { + assert_eq!( + source.to_string(), + "Invalid retry configuration: max_retries must be greater than 0" + ); + } + _ => panic!("Expected TemplateRendering error, got: {:?}", err), + } +} diff --git a/crates/goose/src/recipe/mod.rs b/crates/goose/src/recipe/mod.rs index 243b8731e982..57684e92eab8 100644 --- a/crates/goose/src/recipe/mod.rs +++ b/crates/goose/src/recipe/mod.rs @@ -28,6 +28,16 @@ fn default_version() -> String { "1.0.0".to_string() } +/// Strips location information (e.g., "at line X column Y") from error messages +/// to make them more user-friendly for UI display. +pub fn strip_error_location(error_msg: &str) -> String { + error_msg + .split(" at line") + .next() + .unwrap_or_default() + .to_string() +} + #[derive(Serialize, Deserialize, Debug, Clone, ToSchema)] pub struct Recipe { // Required fields @@ -268,25 +278,16 @@ impl Recipe { Ok(yaml_value) => { if let Some(nested_recipe) = yaml_value.get("recipe") { serde_yaml::from_value(nested_recipe.clone()) - .map_err(|e| anyhow::anyhow!("Failed to parse nested recipe: {}", e))? + .map_err(|e| anyhow::anyhow!("{}", strip_error_location(&e.to_string())))? } else { serde_yaml::from_str(content) - .map_err(|e| anyhow::anyhow!("Failed to parse recipe: {}", e))? + .map_err(|e| anyhow::anyhow!("{}", strip_error_location(&e.to_string())))? } } Err(_) => serde_yaml::from_str(content) - .map_err(|e| anyhow::anyhow!("Failed to parse recipe: {}", e))?, + .map_err(|e| anyhow::anyhow!("{}", strip_error_location(&e.to_string())))?, }; - if let Some(ref retry_config) = recipe.retry { - if let Err(validation_error) = retry_config.validate() { - return Err(anyhow::anyhow!( - "Invalid retry configuration: {}", - validation_error - )); - } - } - Ok(recipe) } } @@ -772,4 +773,52 @@ isGlobal: true"#; panic!("Expected Stdio extension"); } } + + #[test] + fn test_format_serde_error_removes_location() { + let content = r#"{"version": "1.0.0"}"#; + + let result = Recipe::from_content(content); + assert!(result.is_err()); + + let error_msg = result.unwrap_err().to_string(); + assert_eq!(error_msg, "missing field `title`"); + } + + #[test] + fn test_format_serde_error_missing_title() { + let content = r#"{ + "version": "1.0.0", + "description": "A test recipe", + "instructions": "Test instructions" + }"#; + + let result = Recipe::from_content(content); + assert!(result.is_err()); + + let error_msg = result.unwrap_err().to_string(); + assert_eq!(error_msg, "missing field `title`"); + } + + #[test] + fn test_format_serde_error_invalid_type() { + let content = r#"{ + "version": "1.0.0", + "title": "Test", + "description": "Test", + "instructions": "Test", + "settings": { + "temperature": "not_a_number" + } + }"#; + + let result = Recipe::from_content(content); + assert!(result.is_err()); + + let error_msg = result.unwrap_err().to_string(); + assert_eq!( + error_msg, + "settings.temperature: invalid type: string \"not_a_number\", expected f32" + ); + } } diff --git a/crates/goose/src/recipe/template_recipe.rs b/crates/goose/src/recipe/template_recipe.rs index 4490d2ffcd9f..8e651a88fc32 100644 --- a/crates/goose/src/recipe/template_recipe.rs +++ b/crates/goose/src/recipe/template_recipe.rs @@ -8,7 +8,7 @@ use anyhow::Result; use minijinja::{Environment, UndefinedBehavior}; use regex::Regex; -const CURRENT_TEMPLATE_NAME: &str = "current_template"; +const CURRENT_TEMPLATE_NAME: &str = "recipe_template"; const OPEN_BRACE: &str = "{{"; const CLOSE_BRACE: &str = "}}"; diff --git a/crates/goose/src/recipe/validate_recipe.rs b/crates/goose/src/recipe/validate_recipe.rs index 21c169b8babb..69af990dba49 100644 --- a/crates/goose/src/recipe/validate_recipe.rs +++ b/crates/goose/src/recipe/validate_recipe.rs @@ -44,6 +44,7 @@ pub fn validate_recipe_template_from_content( let (recipe, _) = parse_recipe_content(recipe_content, recipe_dir)?; validate_prompt_or_instructions(&recipe)?; + validate_retry_config(&recipe)?; if let Some(response) = &recipe.response { if let Some(json_schema) = &response.json_schema { validate_json_schema(json_schema)?; @@ -53,6 +54,18 @@ pub fn validate_recipe_template_from_content( Ok(recipe) } +fn validate_retry_config(recipe: &Recipe) -> Result<()> { + if let Some(ref retry_config) = recipe.retry { + if let Err(validation_error) = retry_config.validate() { + return Err(anyhow::anyhow!( + "Invalid retry configuration: {}", + validation_error + )); + } + } + Ok(()) +} + fn validate_prompt_or_instructions(recipe: &Recipe) -> Result<()> { let has_instructions = recipe .instructions