fix(agents): fail fast when a recipe's structured response can't reach an ACP-bridged provider (#11307)
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Lifei Zhou <lifei@squareup.com>
This commit is contained in:
@@ -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<MessageStream, ProviderError> {
|
||||
unreachable!()
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn builtin_tool_support_is_delegated_to_inner_provider() {
|
||||
let inner: Arc<dyn Provider> = Arc::new(TestProvider);
|
||||
let provider = CompactingProvider::new(inner);
|
||||
|
||||
assert!(provider.manages_own_context());
|
||||
assert!(provider.supports_builtin_tools());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<dyn Provider>,
|
||||
hook_manager: HookManager,
|
||||
}
|
||||
|
||||
impl RecipeOperation {
|
||||
pub fn new(hook_manager: HookManager) -> Self {
|
||||
Self { hook_manager }
|
||||
pub fn new(provider: Arc<dyn Provider>, hook_manager: HookManager) -> Self {
|
||||
Self {
|
||||
provider,
|
||||
hook_manager,
|
||||
}
|
||||
}
|
||||
|
||||
fn final_output(session: &Session) -> Result<Option<FinalOutputTool>> {
|
||||
@@ -282,6 +288,16 @@ impl Operation<Session, GooseEffect> 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()
|
||||
|
||||
@@ -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<dyn Provider>,
|
||||
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<goose_providers::base::ModelInfo, goose_providers::errors::ProviderError> {
|
||||
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<dyn Provider> = match provider_features.resolved_model {
|
||||
Some(resolved_model) => Arc::new(ResolvedModelProvider {
|
||||
inner: provider,
|
||||
resolved_model,
|
||||
}),
|
||||
None => provider,
|
||||
};
|
||||
let provider: Arc<dyn Provider> =
|
||||
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(),
|
||||
|
||||
@@ -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?;
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user