From c5e5929a641ffc0edbccad476e18f726e9024f70 Mon Sep 17 00:00:00 2001 From: Jasper Date: Sun, 23 Aug 2026 23:27:17 +0000 Subject: [PATCH] fix(recipe): reject non-object response schemas (#11478) --- crates/goose-cli/src/session/builder.rs | 6 +- crates/goose/src/acp/server/new_session.rs | 2 +- crates/goose/src/acp/server/recipe/mod.rs | 14 +++- crates/goose/src/agents/agent.rs | 31 ++++++-- crates/goose/src/agents/execute_commands.rs | 77 ++++++++++++++++--- crates/goose/src/agents/final_output_tool.rs | 49 ++++++++---- .../tests/recipe_scheduling_lifecycle.rs | 24 ++++++ crates/goose/src/agents/subagent_handler.rs | 2 +- crates/goose/src/execution/manager.rs | 2 +- crates/goose/src/recipe/validate_recipe.rs | 66 +++++++++++++++- crates/goose/tests/agent.rs | 6 +- 11 files changed, 234 insertions(+), 45 deletions(-) diff --git a/crates/goose-cli/src/session/builder.rs b/crates/goose-cli/src/session/builder.rs index 36b44e2bb..eaa1766c8 100644 --- a/crates/goose-cli/src/session/builder.rs +++ b/crates/goose-cli/src/session/builder.rs @@ -747,7 +747,11 @@ pub async fn build_session(session_config: SessionBuilderConfig) -> CliSession { agent .apply_recipe_components(recipe.and_then(|r| r.response.clone()), true) - .await; + .await + .unwrap_or_else(|error| { + output::render_error(&format!("Invalid recipe response: {error}")); + process::exit(1); + }); let session_id = resolve_session_id(&session_config, &session_manager, agent.config.goose_mode).await; diff --git a/crates/goose/src/acp/server/new_session.rs b/crates/goose/src/acp/server/new_session.rs index a75e5a366..6fb81eb96 100644 --- a/crates/goose/src/acp/server/new_session.rs +++ b/crates/goose/src/acp/server/new_session.rs @@ -82,7 +82,7 @@ impl GooseAcpAgent { let reloaded_session = self.reload_session(&session.id).await?; let (agent, extension_results) = self.activate_acp_session(cx, &reloaded_session).await?; if let Some(recipe) = &rendered_recipe { - self.apply_recipe(&agent, recipe).await; + self.apply_recipe(&agent, recipe).await?; } let reloaded_session = self.reload_session(&session.id).await?; diff --git a/crates/goose/src/acp/server/recipe/mod.rs b/crates/goose/src/acp/server/recipe/mod.rs index 75304a50b..06100bff6 100644 --- a/crates/goose/src/acp/server/recipe/mod.rs +++ b/crates/goose/src/acp/server/recipe/mod.rs @@ -313,15 +313,21 @@ impl GooseAcpAgent { } } - pub(super) async fn apply_recipe(&self, agent: &Arc, recipe: &Recipe) { + pub(super) async fn apply_recipe( + &self, + agent: &Arc, + recipe: &Recipe, + ) -> Result<(), agent_client_protocol::Error> { agent .apply_recipe_components(recipe.response.clone(), true) - .await; + .await + .invalid_params_err()?; if let Some(instructions) = recipe.instructions.clone() { agent .extend_system_prompt("recipe".to_string(), instructions) .await; } + Ok(()) } pub(super) async fn apply_session_recipe( @@ -334,7 +340,7 @@ impl GooseAcpAgent { }; if session.session_type == SessionType::Scheduled { - self.apply_recipe(agent, recipe).await; + self.apply_recipe(agent, recipe).await?; return Ok(()); } @@ -344,7 +350,7 @@ impl GooseAcpAgent { &recipe_dir, session.user_recipe_values.clone().unwrap_or_default(), )? { - self.apply_recipe(agent, &rendered).await; + self.apply_recipe(agent, &rendered).await?; } Ok(()) diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 87baeb92b..84a06aa43 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -1140,25 +1140,28 @@ impl Agent { extension_configs } - pub async fn add_final_output_tool(&self, response: Response) { + pub async fn add_final_output_tool(&self, response: Response) -> Result<()> { let mut final_output_tool = self.final_output_tool.lock().await; - let created_final_output_tool = FinalOutputTool::new(response); + let created_final_output_tool = + FinalOutputTool::try_new(response).map_err(anyhow::Error::msg)?; let final_output_system_prompt = created_final_output_tool.system_prompt(); *final_output_tool = Some(created_final_output_tool); self.extend_system_prompt("final_output".to_string(), final_output_system_prompt) .await; + Ok(()) } pub async fn apply_recipe_components( &self, response: Option, include_final_output: bool, - ) { + ) -> Result<()> { if include_final_output { if let Some(response) = response { - self.add_final_output_tool(response).await; + self.add_final_output_tool(response).await?; } } + Ok(()) } /// Dispatch a single tool call to the appropriate client @@ -5520,7 +5523,7 @@ echo start >> "$PLUGIN_ROOT/hook.log" })), }; - agent.add_final_output_tool(response).await; + agent.add_final_output_tool(response).await?; let tools = agent.list_tools("test-session-id", None).await; let final_output_tool = tools @@ -5545,6 +5548,24 @@ echo start >> "$PLUGIN_ROOT/hook.log" Ok(()) } + #[tokio::test] + async fn boolean_final_output_schema_returns_error() { + let agent = Agent::new(); + + let error = agent + .apply_recipe_components( + Some(Response { + json_schema: Some(serde_json::json!(true)), + }), + true, + ) + .await + .unwrap_err(); + + assert_eq!(error.to_string(), "json_schema must be an object"); + assert!(agent.final_output_tool.lock().await.is_none()); + } + #[tokio::test] async fn test_tool_inspection_manager_has_all_inspectors() -> Result<()> { let agent = Agent::new(); diff --git a/crates/goose/src/agents/execute_commands.rs b/crates/goose/src/agents/execute_commands.rs index 5f99420e5..c4e923266 100644 --- a/crates/goose/src/agents/execute_commands.rs +++ b/crates/goose/src/agents/execute_commands.rs @@ -5,6 +5,7 @@ use anyhow::{anyhow, Result}; use crate::context_mgmt::compact_messages; use crate::conversation::message::Message; +use crate::recipe::Recipe; use crate::slash_commands::{recipe_slash_command, skill_slash_command}; use super::Agent; @@ -466,20 +467,37 @@ impl Agent { match recipe_slash_command::resolve_command(command, params_str) { Ok(None) => Ok(None), Ok(Some((recipe, prompt))) => { - self.apply_recipe_components(recipe.response.clone(), true) - .await; - self.config - .session_manager - .update(session_id) - .recipe(Some(recipe)) - .apply() - .await?; - Ok(Some(Message::user().with_text(prompt))) + self.apply_resolved_recipe_command(command, recipe, prompt, session_id) + .await } Err(text) => Ok(Some(Message::assistant().with_text(text))), } } + async fn apply_resolved_recipe_command( + &self, + command: &str, + recipe: Recipe, + prompt: String, + session_id: &str, + ) -> Result> { + if let Err(error) = self + .apply_recipe_components(recipe.response.clone(), true) + .await + { + return Ok(Some( + Message::assistant().with_text(format!("Recipe /{command} is not valid: {error}")), + )); + } + self.config + .session_manager + .update(session_id) + .recipe(Some(recipe)) + .apply() + .await?; + Ok(Some(Message::user().with_text(prompt))) + } + async fn handle_skill_command( &self, command: &str, @@ -558,6 +576,8 @@ fn user_only_assistant_text(text: impl Into) -> Message { mod tests { use super::*; use crate::conversation::message::MessageContent; + use crate::recipe::Response; + use serde_json::json; #[test] fn parse_slash_command_splits_on_literal_space() { @@ -615,4 +635,43 @@ mod tests { .iter() .any(|command| command.name == "status")); } + + #[tokio::test] + async fn invalid_rendered_recipe_schema_returns_assistant_response() { + let agent = Agent::new(); + let recipe = Recipe::builder() + .title("Invalid rendered schema") + .description("Invalid rendered schema") + .instructions("Return structured output") + .response(Response { + json_schema: Some(json!({ + "type": "object", + "properties": { + "result": { + "type": "string", + "pattern": "[" + } + } + })), + }) + .build() + .expect("recipe shape is otherwise valid"); + + let response = agent + .apply_resolved_recipe_command( + "invalid-rendered-schema", + recipe, + "Return structured output".to_string(), + "unused-session", + ) + .await + .unwrap() + .unwrap(); + + assert_eq!(response.role, rmcp::model::Role::Assistant); + assert!(response + .as_concat_text() + .contains("Recipe /invalid-rendered-schema is not valid")); + assert!(agent.final_output_tool.lock().await.is_none()); + } } diff --git a/crates/goose/src/agents/final_output_tool.rs b/crates/goose/src/agents/final_output_tool.rs index 70f71ccc2..4b399ca60 100644 --- a/crates/goose/src/agents/final_output_tool.rs +++ b/crates/goose/src/agents/final_output_tool.rs @@ -28,11 +28,6 @@ pub struct FinalOutputTool { } impl FinalOutputTool { - pub fn new(response: Response) -> Self { - Self::try_new(response) - .unwrap_or_else(|error| panic!("Cannot create FinalOutputTool: {error}")) - } - pub fn try_new(response: Response) -> Result { let schema_value = response .json_schema @@ -44,7 +39,7 @@ impl FinalOutputTool { if schema.is_empty() { return Err("empty json_schema is not allowed".to_string()); } - jsonschema::meta::validate(schema_value).map_err(|error| error.to_string())?; + jsonschema::validator_for(schema_value).map_err(|error| error.to_string())?; Ok(Self { response, @@ -195,24 +190,27 @@ mod tests { } #[test] - #[should_panic(expected = "Cannot create FinalOutputTool: json_schema is required")] - fn test_new_with_missing_schema() { + fn test_try_new_with_missing_schema() { let response = Response { json_schema: None }; - FinalOutputTool::new(response); + assert_eq!( + FinalOutputTool::try_new(response).err().unwrap(), + "json_schema is required" + ); } #[test] - #[should_panic(expected = "Cannot create FinalOutputTool: empty json_schema is not allowed")] - fn test_new_with_empty_schema() { + fn test_try_new_with_empty_schema() { let response = Response { json_schema: Some(json!({})), }; - FinalOutputTool::new(response); + assert_eq!( + FinalOutputTool::try_new(response).err().unwrap(), + "empty json_schema is not allowed" + ); } #[test] - #[should_panic] - fn test_new_with_invalid_schema() { + fn test_try_new_with_invalid_schema() { let response = Response { json_schema: Some(json!({ "type": "invalid_type", @@ -223,7 +221,24 @@ mod tests { } })), }; - FinalOutputTool::new(response); + assert!(FinalOutputTool::try_new(response).is_err()); + } + + #[test] + fn test_try_new_with_invalid_pattern() { + let response = Response { + json_schema: Some(json!({ + "type": "object", + "properties": { + "message": { + "type": "string", + "pattern": "[" + } + } + })), + }; + + assert!(FinalOutputTool::try_new(response).is_err()); } #[tokio::test] @@ -243,7 +258,7 @@ mod tests { })), }; - let mut tool = FinalOutputTool::new(response); + let mut tool = FinalOutputTool::try_new(response).unwrap(); let tool_call = CallToolRequestParams::new(FINAL_OUTPUT_TOOL_NAME).with_arguments(object!({ "message": "Hello" // Missing required "count" field @@ -263,7 +278,7 @@ mod tests { json_schema: Some(create_complex_test_schema()), }; - let mut tool = FinalOutputTool::new(response); + let mut tool = FinalOutputTool::try_new(response).unwrap(); let tool_call = CallToolRequestParams::new(FINAL_OUTPUT_TOOL_NAME).with_arguments(object!({ "user": { diff --git a/crates/goose/src/agents/state_machine/tests/recipe_scheduling_lifecycle.rs b/crates/goose/src/agents/state_machine/tests/recipe_scheduling_lifecycle.rs index e3b0845de..35b23bc9c 100644 --- a/crates/goose/src/agents/state_machine/tests/recipe_scheduling_lifecycle.rs +++ b/crates/goose/src/agents/state_machine/tests/recipe_scheduling_lifecycle.rs @@ -384,3 +384,27 @@ async fn invalid_final_output_schema_stops_before_inference() -> Result<()> { Ok(()) } + +#[tokio::test] +async fn boolean_final_output_schema_stops_before_inference() -> Result<()> { + let (pipeline, api) = test_pipeline().await?; + let recipe = Recipe::builder() + .title("Boolean output schema") + .description("Boolean output schema") + .instructions("This must not reach inference") + .response(Response { + json_schema: Some(json!(true)), + }) + .build() + .expect("recipe shape is otherwise valid"); + pipeline.set_recipe(recipe).await?; + + let error = match pipeline.run(["start"]).await { + Ok(_) => panic!("boolean schema must be rejected"), + Err(error) => error, + }; + assert!(error.to_string().contains("json_schema must be an object")); + assert_eq!(api.call_count(), 0); + + Ok(()) +} diff --git a/crates/goose/src/agents/subagent_handler.rs b/crates/goose/src/agents/subagent_handler.rs index 27a7037ab..34efb677f 100644 --- a/crates/goose/src/agents/subagent_handler.rs +++ b/crates/goose/src/agents/subagent_handler.rs @@ -161,7 +161,7 @@ fn get_agent_messages(params: SubagentRunParams) -> AgentMessagesFuture { let has_response_schema = recipe.response.is_some(); agent .apply_recipe_components(recipe.response.clone(), true) - .await; + .await?; let subagent_prompt = build_subagent_prompt(&agent, &task_config, &session_id, system_instructions).await?; diff --git a/crates/goose/src/execution/manager.rs b/crates/goose/src/execution/manager.rs index 96d929861..876824116 100644 --- a/crates/goose/src/execution/manager.rs +++ b/crates/goose/src/execution/manager.rs @@ -229,7 +229,7 @@ impl AgentManager { if let Some(recipe) = &session.recipe { agent .apply_recipe_components(recipe.response.clone(), true) - .await; + .await?; } } diff --git a/crates/goose/src/recipe/validate_recipe.rs b/crates/goose/src/recipe/validate_recipe.rs index b669ac059..c27f58ef5 100644 --- a/crates/goose/src/recipe/validate_recipe.rs +++ b/crates/goose/src/recipe/validate_recipe.rs @@ -20,10 +20,15 @@ pub fn parse_and_validate_parameters( } fn validate_json_schema(schema: &serde_json::Value) -> Result<()> { - match jsonschema::validator_for(schema) { - Ok(_) => Ok(()), - Err(err) => Err(anyhow::anyhow!("JSON schema validation failed: {}", err)), + let schema_object = schema + .as_object() + .ok_or_else(|| anyhow::anyhow!("JSON schema must be an object"))?; + if schema_object.is_empty() { + return Err(anyhow::anyhow!("Empty JSON schema is not allowed")); } + jsonschema::validator_for(schema) + .map(|_| ()) + .map_err(|error| anyhow::anyhow!("JSON schema validation failed: {error}")) } pub fn validate_recipe_template_from_file(recipe_file: &RecipeFile) -> Result { @@ -264,4 +269,59 @@ parameters: assert!(recipe.instructions.is_some()); println!("Recipe: {:?}", recipe.prompt); } + + #[test] + fn response_json_schema_must_be_an_object() { + let recipe_content = r#" +version: 1.0.0 +title: Boolean schema +description: Boolean schema +instructions: Return structured output +response: + json_schema: true +"#; + + let error = validate_recipe_template_from_content(recipe_content, None).unwrap_err(); + + assert_eq!(error.to_string(), "JSON schema must be an object"); + } + + #[test] + fn response_json_schema_accepts_an_object_schema() { + let recipe_content = r#" +version: 1.0.0 +title: Object schema +description: Object schema +instructions: Return structured output +response: + json_schema: + type: object + properties: + result: + type: string +"#; + + validate_recipe_template_from_content(recipe_content, None).unwrap(); + } + + #[test] + fn response_json_schema_must_compile() { + let recipe_content = r#" +version: 1.0.0 +title: Invalid pattern +description: Invalid pattern +instructions: Return structured output +response: + json_schema: + type: object + properties: + result: + type: string + pattern: "[" +"#; + + let error = validate_recipe_template_from_content(recipe_content, None).unwrap_err(); + + assert!(error.to_string().contains("JSON schema validation failed")); + } } diff --git a/crates/goose/tests/agent.rs b/crates/goose/tests/agent.rs index 7ed5ec902..a9a733190 100644 --- a/crates/goose/tests/agent.rs +++ b/crates/goose/tests/agent.rs @@ -3625,7 +3625,7 @@ mod tests { "properties": { "result": { "type": "string" } } })), }) - .await; + .await?; let reply_stream = agent .reply( @@ -3692,7 +3692,7 @@ mod tests { "properties": { "result": { "type": "string" } } })), }) - .await; + .await?; let session_config = SessionConfig { id: session.id.clone(), @@ -3810,7 +3810,7 @@ mod tests { "required": ["result"] })), }) - .await; + .await?; let session_config = SessionConfig { id: session.id,