diff --git a/crates/goose-context-management/src/provider.rs b/crates/goose-context-management/src/provider.rs index a4301e5b8..3744a99fc 100644 --- a/crates/goose-context-management/src/provider.rs +++ b/crates/goose-context-management/src/provider.rs @@ -103,4 +103,41 @@ impl Provider for CompactingProvider { fn manages_own_context(&self) -> bool { true } + + fn supports_builtin_tools(&self) -> bool { + self.inner.supports_builtin_tools() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + struct TestProvider; + + #[async_trait] + impl Provider for TestProvider { + fn get_name(&self) -> &str { + "test" + } + + async fn stream( + &self, + _model_config: &ModelConfig, + _system: &str, + _messages: &[Message], + _tools: &[rmcp::model::Tool], + ) -> Result { + unreachable!() + } + } + + #[test] + fn builtin_tool_support_is_delegated_to_inner_provider() { + let inner: Arc = Arc::new(TestProvider); + let provider = CompactingProvider::new(inner); + + assert!(provider.manages_own_context()); + assert!(provider.supports_builtin_tools()); + } } diff --git a/crates/goose-provider-types/src/base.rs b/crates/goose-provider-types/src/base.rs index 2d8ad302d..55321c495 100644 --- a/crates/goose-provider-types/src/base.rs +++ b/crates/goose-provider-types/src/base.rs @@ -646,6 +646,10 @@ pub trait Provider: Send + Sync { false } + fn supports_builtin_tools(&self) -> bool { + !self.manages_own_context() + } + /// Configure OAuth authentication for this provider /// /// This method is called when a provider has configuration keys marked with oauth_flow = true. diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index d9c2f9b7c..87baeb92b 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -22,7 +22,10 @@ use crate::agents::extension::{ExtensionConfig, ExtensionResult, ToolInfo}; use crate::agents::extension_manager::{ get_parameter_names, ExtensionManager, ExtensionManagerCapabilities, }; -use crate::agents::final_output_tool::{FINAL_OUTPUT_CONTINUATION_MESSAGE, FINAL_OUTPUT_TOOL_NAME}; +use crate::agents::final_output_tool::{ + structured_output_unsupported_message, FINAL_OUTPUT_CONTINUATION_MESSAGE, + FINAL_OUTPUT_TOOL_NAME, +}; use crate::agents::platform_extensions::MANAGE_EXTENSIONS_TOOL_NAME_COMPLETE; use crate::agents::prompt_manager::PromptManager; use crate::agents::retry::{RetryManager, RetryResult}; @@ -1724,7 +1727,10 @@ impl Agent { Arc::new(DoctorOperation), Arc::new(ProjectOperation), Arc::new(SkillOperation::new(self.hook_manager.clone())), - Arc::new(RecipeOperation::new(self.hook_manager.clone())), + Arc::new(RecipeOperation::new( + provider.clone(), + self.hook_manager.clone(), + )), Arc::new(ToolExecutionOperation::new( &self.current_goose_mode, self.extension_manager.clone(), @@ -2146,6 +2152,30 @@ impl Agent { .clone() .ok_or_else(|| anyhow::anyhow!("Session {} has no conversation", session_config.id))?; + if self.final_output_tool.lock().await.is_some() { + let provider = self.provider().await?; + if !provider.supports_builtin_tools() { + let provider_name = provider.get_name(); + warn!( + provider = %provider_name, + "Recipe declares structured response, but this provider can't receive the final_output tool; failing before inference" + ); + let message = Message::assistant() + .with_text(structured_output_unsupported_message(provider_name)) + .with_generated_id_if_missing(); + session_manager + .add_message(&session_config.id, &message) + .await?; + + return Ok(Box::pin(async_stream::try_stream! { + for event in command_preamble { + yield event; + } + yield AgentEvent::Message(message); + })); + } + } + let needs_auto_compact = check_if_compaction_needed( self.provider().await?.as_ref(), &conversation, diff --git a/crates/goose/src/agents/final_output_tool.rs b/crates/goose/src/agents/final_output_tool.rs index f4b2e749b..70f71ccc2 100644 --- a/crates/goose/src/agents/final_output_tool.rs +++ b/crates/goose/src/agents/final_output_tool.rs @@ -12,6 +12,15 @@ pub const FINAL_OUTPUT_SUCCESS_MESSAGE: &str = "Final output successfully collec pub const FINAL_OUTPUT_CONTINUATION_MESSAGE: &str = "You MUST call the `final_output` tool NOW with the final output for the user."; +pub(crate) fn structured_output_unsupported_message(provider_name: &str) -> String { + format!( + "This recipe declares a structured `response`, but provider `{provider_name}` can't \ + support it because it never receives goose's built-in `final_output` tool, so the \ + model can never satisfy this recipe. Remove the entire `response` block from the recipe \ + or run it with a different provider." + ) +} + pub struct FinalOutputTool { pub response: Response, /// The final output collected for the user. It will be a single line string for easy script extraction from output. diff --git a/crates/goose/src/agents/state_machine/ops_recipe.rs b/crates/goose/src/agents/state_machine/ops_recipe.rs index d80d4da18..d4683450a 100644 --- a/crates/goose/src/agents/state_machine/ops_recipe.rs +++ b/crates/goose/src/agents/state_machine/ops_recipe.rs @@ -1,6 +1,7 @@ //! Applies recipe commands and enforces their structured final output. use std::collections::HashSet; +use std::sync::Arc; use anyhow::{anyhow, Result}; use async_trait::async_trait; @@ -8,8 +9,8 @@ use rmcp::model::{CallToolResult, ContentBlock, Tool}; use tracing_futures::Instrument; use crate::agents::final_output_tool::{ - FinalOutputTool, FINAL_OUTPUT_CONTINUATION_MESSAGE, FINAL_OUTPUT_SUCCESS_MESSAGE, - FINAL_OUTPUT_TOOL_NAME, + structured_output_unsupported_message, FinalOutputTool, FINAL_OUTPUT_CONTINUATION_MESSAGE, + FINAL_OUTPUT_SUCCESS_MESSAGE, FINAL_OUTPUT_TOOL_NAME, }; use crate::agents::state_machine::ops_toolcalling::{ emit_post_tool_use, pending_tool_requests, run_pre_tool_hooks, tool_span, ToolDisposition, @@ -23,15 +24,20 @@ use crate::config::GooseMode; use crate::conversation::message::{Message, MessageContent}; use crate::conversation::{Conversation, EffectiveRole}; use crate::hooks::HookManager; +use crate::providers::base::Provider; use crate::session::Session; pub struct RecipeOperation { + provider: Arc, hook_manager: HookManager, } impl RecipeOperation { - pub fn new(hook_manager: HookManager) -> Self { - Self { hook_manager } + pub fn new(provider: Arc, hook_manager: HookManager) -> Self { + Self { + provider, + hook_manager, + } } fn final_output(session: &Session) -> Result> { @@ -282,6 +288,16 @@ impl Operation for RecipeOperation { return not_applicable(); }; + if !self.provider.supports_builtin_tools() { + return self + .command_error( + conversation, + structured_output_unsupported_message(self.provider.get_name()), + emit, + ) + .await; + } + let messages = messages_since_kickoff(conversation)?; let pending = pending_tool_requests(messages) .into_iter() diff --git a/crates/goose/src/agents/state_machine/tests/pipeline.rs b/crates/goose/src/agents/state_machine/tests/pipeline.rs index 8c0e80083..9aaf22bde 100644 --- a/crates/goose/src/agents/state_machine/tests/pipeline.rs +++ b/crates/goose/src/agents/state_machine/tests/pipeline.rs @@ -36,13 +36,13 @@ use crate::session::{Session, SessionManager, SessionType}; use crate::tool_inspection::ToolInspectionManager; use goose_providers::model::ModelConfig; -struct ResolvedModelProvider { +struct FeatureProvider { inner: Arc, - resolved_model: &'static str, + features: ProviderFeatures, } #[async_trait::async_trait] -impl Provider for ResolvedModelProvider { +impl Provider for FeatureProvider { fn get_name(&self) -> &str { self.inner.get_name() } @@ -66,12 +66,18 @@ impl Provider for ResolvedModelProvider { self.inner.get_context_limit(model_config).await } + fn manages_own_context(&self) -> bool { + self.features.manages_own_context + } + async fn fetch_model_info( &self, model_name: &str, ) -> Result { let mut model_info = self.inner.fetch_model_info(model_name).await?; - model_info.resolved_model = Some(self.resolved_model.to_string()); + if let Some(resolved_model) = self.features.resolved_model { + model_info.resolved_model = Some(resolved_model.to_string()); + } Ok(model_info) } } @@ -144,7 +150,10 @@ impl TestPipeline { Arc::new(DoctorOperation), Arc::new(ProjectOperation), Arc::new(SkillOperation::new(self.hook_manager.clone())), - Arc::new(RecipeOperation::new(self.hook_manager.clone())), + Arc::new(RecipeOperation::new( + provider.clone(), + self.hook_manager.clone(), + )), Arc::new(ToolExecutionOperation::new( &self.goose_mode, self.extension_manager.clone(), @@ -745,13 +754,15 @@ async fn build_test_pipeline( .preserve_thinking_context(provider_features.preserves_thinking) .build(), ); - let provider: Arc = match provider_features.resolved_model { - Some(resolved_model) => Arc::new(ResolvedModelProvider { - inner: provider, - resolved_model, - }), - None => provider, - }; + let provider: Arc = + if provider_features.resolved_model.is_some() || provider_features.manages_own_context { + Arc::new(FeatureProvider { + inner: provider, + features: provider_features, + }) + } else { + provider + }; let shared_provider = Arc::new(TokioMutex::new(Some(provider.clone()))); let extension_manager = Arc::new(ExtensionManager::new( shared_provider.clone(), 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 50747d8fd..e3b0845de 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 @@ -1,9 +1,10 @@ use anyhow::Result; use serde_json::json; +use super::dummy_api::ProviderFeatures; use super::pipeline::{ - test_pipeline, test_pipeline_with_scheduler, MessageKind::Agent, MessageKind::Error, - MessageKind::ToolResponse, + test_pipeline, test_pipeline_with, test_pipeline_with_scheduler, MessageKind::Agent, + MessageKind::Error, MessageKind::ToolResponse, }; use crate::agents::extension::ExtensionConfig; use crate::agents::final_output_tool::{FINAL_OUTPUT_CONTINUATION_MESSAGE, FINAL_OUTPUT_TOOL_NAME}; @@ -214,6 +215,44 @@ async fn recipe_retry_and_final_output_run_to_completion() -> Result<()> { Ok(()) } +#[tokio::test] +async fn structured_output_fails_fast_when_provider_manages_own_context() -> Result<()> { + let (pipeline, api) = test_pipeline_with(ProviderFeatures { + manages_own_context: true, + ..ProviderFeatures::default() + }) + .await?; + let pipeline = pipeline.with_provider_name("context-owning-test").await?; + api.on("compute the answer").reply("thinking about it"); + api.on(FINAL_OUTPUT_CONTINUATION_MESSAGE) + .call(FINAL_OUTPUT_TOOL_NAME, json!({ "result": "42" })); + let recipe = Recipe::builder() + .title("Structured output") + .description("Return structured output") + .instructions("Compute the answer") + .response(Response { + json_schema: Some(json!({ + "type": "object", + "properties": { "result": { "type": "string" } }, + "required": ["result"] + })), + }) + .build() + .expect("valid recipe"); + pipeline.set_recipe(recipe).await?; + + let result = pipeline.run(["compute the answer"]).await?; + result.assert_message(-1, Agent, "provider `context-owning-test` can't support it"); + assert!( + api.calls() + .iter() + .all(|call| !call.input_contains(FINAL_OUTPUT_CONTINUATION_MESSAGE)), + "must fail fast without ever entering the continuation-nudge loop" + ); + + Ok(()) +} + #[tokio::test] async fn scheduler_is_advertised_only_when_configured_and_manages_jobs() -> Result<()> { let (pipeline, api) = test_pipeline().await?; diff --git a/crates/goose/tests/agent.rs b/crates/goose/tests/agent.rs index 70b5a3808..7ed5ec902 100644 --- a/crates/goose/tests/agent.rs +++ b/crates/goose/tests/agent.rs @@ -3146,6 +3146,7 @@ mod tests { call_count: AtomicUsize, empty_count: usize, wrap_empty_text: bool, + manages_own_context: bool, } struct AssistantOnlyProvider; @@ -3214,6 +3215,7 @@ mod tests { call_count: AtomicUsize::new(0), empty_count, wrap_empty_text: false, + manages_own_context: false, } } @@ -3222,6 +3224,14 @@ mod tests { call_count: AtomicUsize::new(0), empty_count, wrap_empty_text: true, + manages_own_context: false, + } + } + + fn with_own_context() -> Self { + Self { + manages_own_context: true, + ..Self::new(usize::MAX) } } } @@ -3293,6 +3303,10 @@ mod tests { fn get_name(&self) -> &str { "empty-then-text-mock" } + + fn manages_own_context(&self) -> bool { + self.manages_own_context + } } #[async_trait] @@ -3580,6 +3594,70 @@ mod tests { Ok(()) } + #[tokio::test] + async fn legacy_structured_output_fails_before_provider_inference() -> Result<()> { + use goose::recipe::Response; + + let _guard = env_lock::lock_env([("GOOSE_STATE_MACHINE", None::<&str>)]); + let agent = Agent::new(); + let session = agent + .config + .session_manager + .create_session( + PathBuf::default(), + "unsupported-structured-output".to_string(), + SessionType::Hidden, + GooseMode::default(), + ) + .await?; + let provider = Arc::new(EmptyThenTextProvider::with_own_context()); + agent + .update_provider( + provider.clone(), + ModelConfig::new("mock-model"), + &session.id, + ) + .await?; + agent + .add_final_output_tool(Response { + json_schema: Some(serde_json::json!({ + "type": "object", + "properties": { "result": { "type": "string" } } + })), + }) + .await; + + let reply_stream = agent + .reply( + Message::user().with_text("Hi"), + SessionConfig { + id: session.id, + schedule_id: None, + max_turns: Some(3), + retry_config: None, + }, + None, + ) + .await?; + tokio::pin!(reply_stream); + + let mut messages = Vec::new(); + while let Some(event) = reply_stream.next().await { + if let AgentEvent::Message(message) = event? { + messages.push(message); + } + } + + let text = concat_text(&messages); + assert!( + text.contains("empty-then-text-mock") && text.contains("final_output"), + "expected the unsupported structured-output error, got: {text:?}" + ); + assert_eq!(provider.call_count.load(Ordering::SeqCst), 0); + + Ok(()) + } + /// When a final-output tool is installed and the model stops without /// calling it, the empty turn must yield the mandatory final-output nudge /// — not the generic empty-response fallback — so structured-output