fix(recipe): reject non-object response schemas (#11478)
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -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?;
|
||||
|
||||
@@ -313,15 +313,21 @@ impl GooseAcpAgent {
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) async fn apply_recipe(&self, agent: &Arc<Agent>, recipe: &Recipe) {
|
||||
pub(super) async fn apply_recipe(
|
||||
&self,
|
||||
agent: &Arc<Agent>,
|
||||
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(())
|
||||
|
||||
@@ -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<Response>,
|
||||
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();
|
||||
|
||||
@@ -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<Option<Message>> {
|
||||
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<String>) -> 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());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<Self, String> {
|
||||
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": {
|
||||
|
||||
@@ -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(())
|
||||
}
|
||||
|
||||
@@ -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?;
|
||||
|
||||
@@ -229,7 +229,7 @@ impl AgentManager {
|
||||
if let Some(recipe) = &session.recipe {
|
||||
agent
|
||||
.apply_recipe_components(recipe.response.clone(), true)
|
||||
.await;
|
||||
.await?;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<Recipe> {
|
||||
@@ -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"));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user