From 3dd0762f96ba0afe1c881d92a93b489c61e2d7b5 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 20 Aug 2026 12:50:47 +0000 Subject: [PATCH] feat(acp): derive and forward thinking effort from the ACP harness (#10949) Signed-off-by: Matt Toohey Co-authored-by: Claude Co-authored-by: Jasper Hugo --- crates/goose-provider-types/Cargo.toml | 2 +- crates/goose-provider-types/src/base.rs | 36 + crates/goose-provider-types/src/errors.rs | 6 + crates/goose-provider-types/src/model.rs | 30 +- crates/goose-provider-types/src/thinking.rs | 28 + crates/goose/src/acp/provider.rs | 1038 ++++++++++++++++- crates/goose/src/acp/response_builder.rs | 204 +++- crates/goose/src/acp/server.rs | 346 +++++- crates/goose/src/acp/server/dispatch.rs | 1 + crates/goose/src/acp/server/fork_session.rs | 15 +- crates/goose/src/acp/server/load_session.rs | 86 +- crates/goose/src/acp/server/new_session.rs | 11 +- crates/goose/src/agents/agent.rs | 323 ++++- crates/goose/src/agents/mod.rs | 2 +- .../goose/tests/acp_bootstrap_effort_test.rs | 329 ++++++ crates/goose/tests/providers.rs | 62 + goose-self-test.yaml | 21 +- 17 files changed, 2444 insertions(+), 96 deletions(-) create mode 100644 crates/goose/tests/acp_bootstrap_effort_test.rs diff --git a/crates/goose-provider-types/Cargo.toml b/crates/goose-provider-types/Cargo.toml index 4c5b4aaa4..9ced28d34 100644 --- a/crates/goose-provider-types/Cargo.toml +++ b/crates/goose-provider-types/Cargo.toml @@ -27,7 +27,7 @@ serde = { workspace = true } serde_json = { workspace = true } strum = { workspace = true } thiserror = { workspace = true } -tokio = { workspace = true, features = ["time"] } +tokio = { workspace = true, features = ["sync", "time"] } tracing = { workspace = true } unicode-normalization = { version = "0.1.22", default-features = false, features = ["std"] } uuid = { workspace = true, features = ["v4", "std"] } diff --git a/crates/goose-provider-types/src/base.rs b/crates/goose-provider-types/src/base.rs index b44254349..f29cfb3c4 100644 --- a/crates/goose-provider-types/src/base.rs +++ b/crates/goose-provider-types/src/base.rs @@ -5,6 +5,7 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use std::collections::HashMap; use std::pin::Pin; +use tokio::sync::watch; use crate::{ canonical::{catalog::ProviderSetupMetadata, map_to_canonical_model, CanonicalModelRegistry}, @@ -17,6 +18,7 @@ use crate::{ model::ModelConfig, permission::PermissionConfirmation, retry::RetryConfig, + thinking::ThinkingEffortSupport, }; /// Metadata about a provider's configuration requirements and capabilities @@ -646,6 +648,40 @@ pub trait Provider: Send + Sync { Ok(()) } + /// How this provider participates in thinking-effort selection. Providers + /// that manage reasoning through an external harness report the harness's + /// advertised capability; the default keeps the model-name-based path. + fn thinking_effort_support(&self) -> ThinkingEffortSupport { + ThinkingEffortSupport::Unspecified + } + + /// Subscribe to provider-managed thinking-effort capability changes. + /// Providers without an asynchronous capability source return `None`. + fn subscribe_thinking_effort_support(&self) -> Option> { + None + } + + /// Forward a thinking-effort selection to the provider. Returns `Ok(true)` + /// when the provider applied the value itself (no provider recreation + /// needed); `Ok(false)` when the caller should use the legacy path. + async fn set_thinking_effort( + &self, + _session_id: &str, + _value: &str, + ) -> Result { + Ok(false) + } + + /// Apply a session's model selection after the provider is installed. + /// Providers that manage their own model (e.g. ACP harnesses) override + /// this to sync the selection before the first prompt. + async fn apply_model_selection( + &self, + _model_config: &ModelConfig, + ) -> Result<(), ProviderError> { + Ok(()) + } + fn permission_routing(&self) -> PermissionRouting { PermissionRouting::Noop } diff --git a/crates/goose-provider-types/src/errors.rs b/crates/goose-provider-types/src/errors.rs index ed441738b..5170a5ea4 100644 --- a/crates/goose-provider-types/src/errors.rs +++ b/crates/goose-provider-types/src/errors.rs @@ -30,6 +30,11 @@ pub enum ProviderError { #[error("Request failed: {0}")] RequestFailed(String), + /// Bad input rather than an operational failure: retrying is pointless, but + /// a different value may succeed. + #[error("Invalid value: {0}")] + InvalidValue(String), + #[error("Execution error: {0}")] ExecutionError(String), @@ -69,6 +74,7 @@ impl ProviderError { ProviderError::ServerError(_) => "server", ProviderError::NetworkError(_) => "network", ProviderError::RequestFailed(_) => "request", + ProviderError::InvalidValue(_) => "invalid_value", ProviderError::ExecutionError(_) => "execution", ProviderError::UsageError(_) => "usage", ProviderError::NotImplemented(_) => "not_implemented", diff --git a/crates/goose-provider-types/src/model.rs b/crates/goose-provider-types/src/model.rs index 757034a24..7c2290555 100644 --- a/crates/goose-provider-types/src/model.rs +++ b/crates/goose-provider-types/src/model.rs @@ -213,7 +213,10 @@ impl ModelConfig { } pub fn with_default_thinking_effort(mut self, effort: Option) -> Self { - if self.thinking_effort().is_none() { + // Guard on raw-param presence rather than parseability: a persisted + // harness value like "default" doesn't parse into ThinkingEffort but + // is still an explicit user pick that must not be overwritten. + if self.request_param::("thinking_effort").is_none() { if let Some(effort) = effort { self = self.with_thinking_effort(effort); } @@ -387,6 +390,31 @@ mod tests { ); } + #[test] + fn with_default_thinking_effort_preserves_unparseable_raw_param() { + let config = config_with_params( + "test", + HashMap::from([("thinking_effort".to_string(), serde_json::json!("default"))]), + ) + .with_default_thinking_effort(Some(ThinkingEffort::High)); + + assert_eq!( + config + .request_params + .as_ref() + .and_then(|params| params.get("thinking_effort")), + Some(&serde_json::json!("default")) + ); + } + + #[test] + fn with_default_thinking_effort_applies_when_absent() { + let config = + ModelConfig::new("test").with_default_thinking_effort(Some(ThinkingEffort::High)); + + assert_eq!(config.thinking_effort(), Some(ThinkingEffort::High)); + } + #[test] fn preserves_explicit_thinking_effort() { let previous = config_with_params( diff --git a/crates/goose-provider-types/src/thinking.rs b/crates/goose-provider-types/src/thinking.rs index 20041b2ed..17cea9f97 100644 --- a/crates/goose-provider-types/src/thinking.rs +++ b/crates/goose-provider-types/src/thinking.rs @@ -338,6 +338,34 @@ impl fmt::Display for ThinkingEffort { } } +/// A single selectable effort value advertised by a provider-managed harness. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ThinkingEffortOption { + pub value: String, + pub label: String, +} + +/// A harness-advertised effort config option mirrored verbatim into goose's +/// `thinking_effort` session option. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ThinkingEffortCapability { + /// The harness's config option id, e.g. "effort". + pub option_id: String, + pub values: Vec, + pub current: Option, +} + +/// How a provider participates in thinking-effort selection. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ThinkingEffortSupport { + /// The provider doesn't manage effort; callers keep the model-name-based path. + Unspecified, + /// The provider manages reasoning itself but has no effort knob for the current model. + Unsupported, + /// The provider passes through a harness-advertised effort option. + Options(ThinkingEffortCapability), +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/goose/src/acp/provider.rs b/crates/goose/src/acp/provider.rs index 2e37ff5b7..d9ecf9698 100644 --- a/crates/goose/src/acp/provider.rs +++ b/crates/goose/src/acp/provider.rs @@ -4,10 +4,10 @@ use agent_client_protocol::schema::v1::{ LoadSessionRequest, McpCapabilities, McpServer, McpServerHttp, McpServerStdio, NewSessionRequest, NewSessionResponse, PromptRequest, PromptResponse, RequestPermissionOutcome, RequestPermissionRequest, RequestPermissionResponse, Role as AcpRole, SessionConfigKind, - SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOptions, SessionId, - SessionModeState, SessionNotification, SessionUpdate, SetSessionConfigOptionRequest, - SetSessionModeRequest, SetSessionModeResponse, StopReason, TextContent, ToolCallContent, - ToolCallStatus, ToolKind, + SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption, + SessionConfigSelectOptions, SessionId, SessionModeState, SessionNotification, SessionUpdate, + SetSessionConfigOptionRequest, SetSessionModeRequest, SetSessionModeResponse, StopReason, + TextContent, ToolCallContent, ToolCallStatus, ToolKind, }; use agent_client_protocol::schema::ProtocolVersion; use agent_client_protocol::{Agent, Client, ConnectionTo}; @@ -29,12 +29,12 @@ use std::sync::{ use std::thread::JoinHandle; use tokio::io::AsyncReadExt; use tokio::process::{Child, Command}; -use tokio::sync::{mpsc, oneshot, Mutex as TokioMutex}; +use tokio::sync::{mpsc, oneshot, watch, Mutex as TokioMutex}; use tokio_util::compat::{TokioAsyncReadCompatExt as _, TokioAsyncWriteCompatExt as _}; use crate::acp::handoff::{build_handoff_context_memo, memo_token_budget, prompt_token_cost}; use crate::acp::{map_permission_response, PermissionDecision}; -use crate::config::{ExtensionConfig, GooseMode}; +use crate::config::{Config, ExtensionConfig, GooseMode}; use crate::conversation::message::{Message, MessageContent, TOOL_META_EXTERNAL_DISPATCH_KEY}; use crate::permission::permission_confirmation::PrincipalType; use crate::permission::{Permission, PermissionConfirmation}; @@ -44,10 +44,20 @@ use crate::token_counter::create_token_counter; use crate::utils::sanitize_unicode_tags; use goose_providers::errors::ProviderError; use goose_providers::model::ModelConfig; +use goose_providers::thinking::{ + ThinkingEffortCapability, ThinkingEffortOption, ThinkingEffortSupport, +}; /// Sentinel: resolved to the actual model name during connect(). pub const ACP_CURRENT_MODEL: &str = "current"; +/// Config option id used by agents that advertise a thinking-effort selector +/// without categorizing it as `thought_level`. +const EFFORT_CONFIG_OPTION_ID: &str = "effort"; + +/// Session request param holding the selected thinking effort. +pub(super) const THINKING_EFFORT_PARAM: &str = "thinking_effort"; + pub struct AcpProviderConfig { pub command: PathBuf, pub args: Vec, @@ -230,6 +240,37 @@ impl Drop for HandoffContextClaimGuard { } } +#[derive(Clone)] +struct AcpEffortState { + capability: Arc>>, + updates: watch::Sender, +} + +impl AcpEffortState { + fn new() -> Self { + let (updates, _) = watch::channel(ThinkingEffortSupport::Unsupported); + Self { + capability: Arc::new(Mutex::new(None)), + updates, + } + } +} + +#[derive(Clone)] +struct AcpSessionState { + active_id: Arc>>, + effort: AcpEffortState, +} + +impl AcpSessionState { + fn new(effort: AcpEffortState) -> Self { + Self { + active_id: Arc::new(Mutex::new(None)), + effort, + } + } +} + pub struct AcpProvider { name: String, goose_mode: Arc>, @@ -255,6 +296,14 @@ pub struct AcpProvider { /// redundant `SetConfigOption` calls. applied_model: Arc>>, + /// The agent's thinking-effort config option, mirrored from every + /// config-options payload it sends. `None` means the agent offers no effort + /// selector for its current model. Its `current` value doubles as the + /// redundant-send guard: unlike a goose-side cache of the last applied + /// value, it tracks the agent resetting its own effort (e.g. on a model + /// switch), so the persisted value is re-applied when that happens. + effort: AcpEffortState, + tx: Option>, cancel_tx: Option>, loop_thread: Option>, @@ -350,11 +399,13 @@ impl AcpProvider { let pending_tool_updates: Arc>> = Arc::new(Mutex::new(HashMap::new())); let context_size = Arc::new(AtomicU64::new(0)); + let effort = AcpEffortState::new(); let client_loop = AcpClientLoop::new( config, goose_mode_shared.clone(), pending_tool_updates.clone(), context_size.clone(), + effort.clone(), ); let (cancel_tx, cancel_rx) = oneshot::channel(); let loop_thread = spawn_client_loop(run(client_loop, rx, init_tx, cancel_rx)); @@ -399,6 +450,7 @@ impl AcpProvider { context_size, model_config_option_id, applied_model: Arc::new(Mutex::new(applied_model)), + effort, tx: client_loop_guard.tx.take(), cancel_tx: client_loop_guard.cancel_tx.take(), loop_thread: client_loop_guard.thread.take(), @@ -498,6 +550,61 @@ impl AcpProvider { Ok(()) } + fn effort_capability(&self) -> Result> { + Ok(self + .effort + .capability + .lock() + .map_err(|_| anyhow::anyhow!("effort_state lock poisoned"))? + .clone()) + } + + /// The redundant-send guard is the mirrored capability's `current`, not a + /// goose-side record of the last sent value: the agent's `SetConfigOption` + /// response refreshes `effort_state` before this call returns, and later + /// refreshes track the agent resetting its own effort. + async fn set_effort_option( + &self, + goose_id: &str, + option_id: &str, + value: String, + ) -> Result<()> { + { + let state = self + .effort + .capability + .lock() + .map_err(|_| anyhow::anyhow!("effort_state lock poisoned"))?; + let current = state + .as_ref() + .and_then(|capability| capability.current.as_deref()); + if current == Some(value.as_str()) { + return Ok(()); + } + } + + self.send_set_config_option(goose_id, option_id.to_string(), value) + .await + } + + /// Forward the session's thinking effort to the agent when it differs from + /// the agent's mirrored current value. A recreated provider (model switch, + /// provider switch, session reload) starts from the agent's own default, so + /// the value has to be re-applied rather than assumed. It comes from + /// `resolve_effort_value`, the same resolver the config menu advertises + /// from, so the selection ACP clients see is the one that gets sent. + async fn apply_effort_if_changed(&self, model_config: &ModelConfig) -> Result<()> { + let Some(capability) = self.effort_capability()? else { + return Ok(()); + }; + let Some(mapped) = resolve_effort_value(&capability, model_config) else { + return Ok(()); + }; + + self.set_effort_option("", &capability.option_id, mapped) + .await + } + async fn prompt( &self, session_id: SessionId, @@ -638,6 +745,56 @@ impl Provider for AcpProvider { Ok(()) } + fn thinking_effort_support(&self) -> ThinkingEffortSupport { + match self.effort_capability().ok().flatten() { + Some(capability) => ThinkingEffortSupport::Options(capability), + None => ThinkingEffortSupport::Unsupported, + } + } + + fn subscribe_thinking_effort_support(&self) -> Option> { + Some(self.effort.updates.subscribe()) + } + + async fn set_thinking_effort( + &self, + session_id: &str, + value: &str, + ) -> Result { + let capability = self + .effort_capability() + .map_err(|e| ProviderError::RequestFailed(e.to_string()))?; + // No effort selector: the only advertised choice is `off`, which is a + // no-op. Falling back to the caller's legacy path would respawn the + // agent, while accepting any other value would persist a setting that + // was never applied. + let Some(capability) = capability else { + return if value.eq_ignore_ascii_case("off") { + Ok(true) + } else { + Err(ProviderError::InvalidValue(format!( + "Agent offers no thinking effort '{value}'" + ))) + }; + }; + let mapped = map_effort_value(&capability, value).ok_or_else(|| { + ProviderError::InvalidValue(format!("Agent offers no thinking effort '{value}'")) + })?; + + self.set_effort_option(session_id, &capability.option_id, mapped) + .await + .map_err(|e| effort_option_error(value, e))?; + Ok(true) + } + + async fn apply_model_selection(&self, model_config: &ModelConfig) -> Result<(), ProviderError> { + self.apply_model_if_changed(&model_config.model_name) + .await + .map_err(|e| { + ProviderError::RequestFailed(format!("Failed to set ACP model option: {e}")) + }) + } + fn permission_routing(&self) -> PermissionRouting { PermissionRouting::ActionRequired } @@ -674,6 +831,12 @@ impl Provider for AcpProvider { ProviderError::RequestFailed(format!("Failed to set ACP model option: {e}")) })?; + self.apply_effort_if_changed(model_config) + .await + .map_err(|e| { + ProviderError::RequestFailed(format!("Failed to set ACP effort option: {e}")) + })?; + let current_prompt_blocks = messages_to_prompt(messages, None); if current_prompt_blocks.is_empty() { return Ok(Box::pin(futures::stream::empty())); @@ -960,6 +1123,7 @@ struct AcpClientLoop { prompt_response_tx: Arc>>>, pending_tool_updates: Arc>>, context_size: Arc, + effort: AcpEffortState, } impl AcpClientLoop { @@ -968,6 +1132,7 @@ impl AcpClientLoop { goose_mode: Arc>, pending_tool_updates: Arc>>, context_size: Arc, + effort: AcpEffortState, ) -> Self { Self { config, @@ -975,6 +1140,7 @@ impl AcpClientLoop { prompt_response_tx: Arc::new(Mutex::new(None)), pending_tool_updates, context_size, + effort, } } @@ -1029,9 +1195,11 @@ impl AcpClientLoop { prompt_response_tx, pending_tool_updates, context_size, + effort, } = self; let notification_callback = config.notification_callback.clone(); let reverse_modes = reverse_mode_mapping(&config.mode_mapping); + let session_state = AcpSessionState::new(effort); Client .builder() @@ -1042,7 +1210,17 @@ impl AcpClientLoop { let goose_mode = goose_mode.clone(); let pending_tool_updates = pending_tool_updates.clone(); let context_size = context_size.clone(); + let session_state = session_state.clone(); async move |notification: SessionNotification, _cx| { + let is_active_session = + session_state.active_id.lock().is_ok_and(|active| { + active + .as_ref() + .is_none_or(|id| id == ¬ification.session_id) + }); + if !is_active_session { + return Ok(()); + } if let Some(ref cb) = notification_callback { cb(notification.clone()); } @@ -1059,6 +1237,7 @@ impl AcpClientLoop { } } SessionUpdate::ConfigOptionUpdate(update) => { + publish_effort_state(&session_state.effort, &update.config_options); for opt in &update.config_options { if opt.category == Some(SessionConfigOptionCategory::Mode) { if let SessionConfigKind::Select(sel) = &opt.kind { @@ -1236,7 +1415,16 @@ impl AcpClientLoop { agent_client_protocol::on_receive_request!(), ) .connect_with(transport, async move |cx: ConnectionTo| { - handle_requests(config, goose_mode, cx, rx, prompt_response_tx, init_tx).await + handle_requests( + config, + goose_mode, + cx, + rx, + prompt_response_tx, + session_state, + init_tx, + ) + .await }) .await?; @@ -1339,6 +1527,7 @@ async fn handle_requests( cx: ConnectionTo, rx: &mut mpsc::Receiver, prompt_response_tx: Arc>>>, + session_state: AcpSessionState, init_tx: oneshot::Sender>, ) -> Result<(), agent_client_protocol::Error> { let mut init_tx = Some(init_tx); @@ -1383,9 +1572,18 @@ async fn handle_requests( .await; let result = match session { Ok(session) => { + *session_state.active_id.lock().unwrap() = Some(session.session_id.clone()); session_ids.push(session.session_id.clone()); - apply_session_config_options(&config, &cx, session.session_id.clone()) - .await?; + if let Some(config_options) = session.config_options.as_deref() { + publish_effort_state(&session_state.effort, config_options); + } + apply_session_config_options( + &config, + &cx, + session.session_id.clone(), + &session_state.effort, + ) + .await?; apply_session_mode(&config, &goose_mode, &cx, session).await } Err(error) => Err(acp_method_error(AGENT_METHOD_NAMES.session_new, error)), @@ -1396,6 +1594,12 @@ async fn handle_requests( session_id, response_tx, } => { + let previous_session_id = session_state + .active_id + .lock() + .unwrap() + .replace(session_id.clone()); + let previous_effort_state = replace_effort_state(&session_state.effort, None); let result = if supports_load { let mcp_servers = filter_supported_servers(&config.mcp_servers, &mcp_capabilities); @@ -1418,11 +1622,23 @@ async fn handle_requests( let result = match result { Ok(session) => { session_ids.push(session.session_id.clone()); - apply_session_config_options(&config, &cx, session.session_id.clone()) - .await?; + if let Some(config_options) = session.config_options.as_deref() { + publish_effort_state(&session_state.effort, config_options); + } + apply_session_config_options( + &config, + &cx, + session.session_id.clone(), + &session_state.effort, + ) + .await?; apply_session_mode(&config, &goose_mode, &cx, session).await } - Err(error) => Err(error), + Err(error) => { + *session_state.active_id.lock().unwrap() = previous_session_id; + replace_effort_state(&session_state.effort, previous_effort_state); + Err(error) + } }; log_undelivered(response_tx.send(result), AGENT_METHOD_NAMES.session_load); } @@ -1462,11 +1678,15 @@ async fn handle_requests( } => { let value_id = agent_client_protocol::schema::v1::SessionConfigValueId::new(value); let req = SetSessionConfigOptionRequest::new(session_id, config_id, value_id); + // The agent rebuilds per-model effort levels in this response, + // so it is the freshest source after a goose-initiated switch. let result: Result<()> = cx .send_request(req) .block_task() .await - .map(|_| ()) + .map(|response| { + publish_effort_state(&session_state.effort, &response.config_options) + }) .map_err(anyhow::Error::from); log_undelivered( response_tx.send(result), @@ -1524,23 +1744,28 @@ async fn apply_session_config_options( config: &AcpProviderConfig, cx: &ConnectionTo, session_id: SessionId, + effort: &AcpEffortState, ) -> Result<()> { for (config_id, value) in &config.session_config_options { let value_id = agent_client_protocol::schema::v1::SessionConfigValueId::new(value.clone()); - cx.send_request(SetSessionConfigOptionRequest::new( - session_id.clone(), - config_id.clone(), - value_id, - )) - .block_task() - .await - .map_err(|err| { - anyhow::anyhow!( - "ACP agent rejected {} for '{}': {err}", - AGENT_METHOD_NAMES.session_set_config_option, - config_id - ) - })?; + let response = cx + .send_request(SetSessionConfigOptionRequest::new( + session_id.clone(), + config_id.clone(), + value_id, + )) + .block_task() + .await + .map_err(|err| { + anyhow::anyhow!( + "ACP agent rejected {} for '{}': {err}", + AGENT_METHOD_NAMES.session_set_config_option, + config_id + ) + })?; + // Pinning the model here makes the agent rebuild its per-model effort + // levels, so this response supersedes the session/new snapshot. + publish_effort_state(effort, &response.config_options); } Ok(()) } @@ -1917,22 +2142,10 @@ fn extract_model_info_from_config_options( })?; let current = select.current_value.0.to_string(); - let available = match &select.options { - SessionConfigSelectOptions::Ungrouped(options) => options - .iter() - .map(|option| option.value.0.to_string()) - .collect(), - SessionConfigSelectOptions::Grouped(groups) => groups - .iter() - .flat_map(|group| { - group - .options - .iter() - .map(|option| option.value.0.to_string()) - }) - .collect(), - _ => Vec::new(), - }; + let available = select_option_values(&select.options) + .into_iter() + .map(|option| option.value) + .collect(); Some((current, available)) } @@ -1951,6 +2164,166 @@ fn resolve_model_info( ))) } +fn select_option_values(options: &SessionConfigSelectOptions) -> Vec { + let effort_option = |option: &SessionConfigSelectOption| ThinkingEffortOption { + value: option.value.0.to_string(), + label: option.name.clone(), + }; + match options { + SessionConfigSelectOptions::Ungrouped(options) => { + options.iter().map(effort_option).collect() + } + SessionConfigSelectOptions::Grouped(groups) => groups + .iter() + .flat_map(|group| group.options.iter().map(effort_option)) + .collect(), + _ => Vec::new(), + } +} + +/// Find the agent's thinking-effort selector. Detection is by category so any +/// ACP agent advertising `thought_level` is supported without per-provider +/// config, with the well-known option id as a fallback. +fn extract_effort_capability( + config_options: &[SessionConfigOption], +) -> Option { + let option = config_options + .iter() + .find(|opt| opt.category.as_ref() == Some(&SessionConfigOptionCategory::ThoughtLevel)) + .or_else(|| { + config_options + .iter() + .find(|opt| opt.id.0.as_ref() == EFFORT_CONFIG_OPTION_ID) + })?; + let SessionConfigKind::Select(select) = &option.kind else { + return None; + }; + + let values = select_option_values(&select.options); + if values.is_empty() { + return None; + } + Some(ThinkingEffortCapability { + option_id: option.id.0.to_string(), + values, + current: Some(select.current_value.0.to_string()), + }) +} + +/// Config-options payloads carry the agent's full set, so a payload without an +/// effort selector means the current model has none and the mirrored capability +/// must be dropped. +#[cfg(test)] +fn refresh_effort_state( + effort_state: &Arc>>, + config_options: &[SessionConfigOption], +) { + if let Ok(mut state) = effort_state.lock() { + *state = extract_effort_capability(config_options); + } +} + +fn publish_effort_support( + effort_updates: &watch::Sender, + support: ThinkingEffortSupport, +) { + effort_updates.send_if_modified(|current| { + if *current == support { + false + } else { + *current = support; + true + } + }); +} + +fn publish_effort_state(effort: &AcpEffortState, config_options: &[SessionConfigOption]) { + let capability = extract_effort_capability(config_options); + replace_effort_state(effort, capability); +} + +fn replace_effort_state( + effort: &AcpEffortState, + capability: Option, +) -> Option { + let mut state = effort.capability.lock().unwrap(); + let previous = std::mem::replace(&mut *state, capability.clone()); + publish_effort_support( + &effort.updates, + capability.map_or( + ThinkingEffortSupport::Unsupported, + ThinkingEffortSupport::Options, + ), + ); + previous +} + +/// Map a goose effort value onto the agent's advertised vocabulary. Values goose +/// and the agent share pass through; goose's own enum values map onto their +/// closest agent equivalent. Anything else yields `None` so we never send a +/// value the agent would reject. +pub(super) fn map_effort_value( + capability: &ThinkingEffortCapability, + value: &str, +) -> Option { + let offered = |candidate: &str| { + capability + .values + .iter() + .find(|option| option.value.eq_ignore_ascii_case(candidate)) + .map(|option| option.value.clone()) + }; + + offered(value).or_else(|| { + let synonyms: &[&str] = match value.to_lowercase().as_str() { + // Harnesses that always reason have no "off"; their default is the + // closest thing to not forcing an effort level. + "off" => &["default"], + "max" => &["xhigh"], + "xhigh" => &["max"], + _ => &[], + }; + synonyms.iter().find_map(|candidate| offered(candidate)) + }) +} + +/// Resolve the goose-side thinking effort to send to the agent: the session's +/// persisted pick wins, then the global default, each mapped into the agent's +/// vocabulary. `None` leaves the agent on its own current value. Shared with the +/// config menu so the advertised selection is the applied one — a persisted pick +/// the agent no longer offers (its selector was rebuilt by a model switch) falls +/// back to the global on both sides. The unmappable pick is deliberately left +/// persisted: it becomes honorable again if the user switches back. +pub(super) fn resolve_effort_value( + capability: &ThinkingEffortCapability, + model_config: &ModelConfig, +) -> Option { + model_config + .request_param::(THINKING_EFFORT_PARAM) + .and_then(|value| map_effort_value(capability, &value)) + .or_else(|| { + Config::global() + .get_goose_thinking_effort() + .and_then(|effort| map_effort_value(capability, &effort.to_string())) + }) +} + +/// Separate a value the agent evaluated and refused from an operational failure. +/// Only an agent that actually processed the request answers with the JSON-RPC +/// `invalid_params` code; the client library synthesizes `internal_error` for a +/// dead subprocess or dropped connection, and goose's own send failures carry no +/// ACP error at all. +fn effort_option_error(value: &str, error: anyhow::Error) -> ProviderError { + match error.downcast_ref::() { + Some(acp_error) if acp_error.code == agent_client_protocol::ErrorCode::InvalidParams => { + ProviderError::InvalidValue(format!( + "Agent rejected thinking effort '{value}': {acp_error}" + )) + } + _ => ProviderError::RequestFailed(format!("Failed to set ACP effort option: {error}")), + } +} + fn reverse_mode_mapping( mode_mapping: &HashMap>, ) -> HashMap> { @@ -1993,7 +2366,8 @@ mod tests { use super::*; use crate::agents::extension::Envs; use agent_client_protocol::schema::v1::{ - ErrorCode, SessionConfigSelectOption, SessionMode, SessionModeId, + ConfigOptionUpdate, ErrorCode, SessionConfigSelectGroup, SessionConfigSelectOption, + SessionMode, SessionModeId, }; use test_case::test_case; @@ -2145,6 +2519,7 @@ mod tests { context_size: Arc::new(AtomicU64::new(0)), model_config_option_id: None, applied_model: Arc::new(Mutex::new(None)), + effort: AcpEffortState::new(), tx, cancel_tx: None, loop_thread: None, @@ -2996,6 +3371,581 @@ mod tests { assert!(rx.try_recv().is_err()); } + fn effort_select_options(values: &[&str]) -> Vec { + values + .iter() + .map(|value| SessionConfigSelectOption::new(value.to_string(), *value)) + .collect() + } + + fn effort_capability(values: &[&str], current: &str) -> ThinkingEffortCapability { + ThinkingEffortCapability { + option_id: EFFORT_CONFIG_OPTION_ID.to_string(), + values: values + .iter() + .map(|value| ThinkingEffortOption { + value: value.to_string(), + label: value.to_string(), + }) + .collect(), + current: Some(current.to_string()), + } + } + + fn test_provider_with_effort( + tx: mpsc::Sender, + capability: Option, + ) -> AcpProvider { + let (provider, _) = test_provider_with_tx(Some(tx)); + *provider.effort.capability.lock().unwrap() = capability; + provider + } + + fn model_with_effort(value: &str) -> ModelConfig { + ModelConfig::new(ACP_CURRENT_MODEL).with_merged_request_params(HashMap::from([( + THINKING_EFFORT_PARAM.to_string(), + serde_json::json!(value), + )])) + } + + async fn expect_set_config_option(rx: &mut mpsc::Receiver) -> (String, String) { + match rx.recv().await.expect("expected a SetConfigOption request") { + ClientRequest::SetConfigOption { + config_id, + value, + response_tx, + .. + } => { + let _ = response_tx.send(Ok(())); + (config_id, value) + } + _ => panic!("unexpected request kind"), + } + } + + async fn fail_set_config_option( + rx: &mut mpsc::Receiver, + error: agent_client_protocol::Error, + ) { + match rx.recv().await.expect("expected a SetConfigOption request") { + ClientRequest::SetConfigOption { response_tx, .. } => { + let _ = response_tx.send(Err(anyhow::Error::from(error))); + } + _ => panic!("unexpected request kind"), + } + } + + #[test] + fn extract_effort_capability_reads_thought_level_option() { + let options = vec![ + SessionConfigOption::select( + "model", + "Model", + "sonnet", + effort_select_options(&["sonnet"]), + ) + .category(SessionConfigOptionCategory::Model), + SessionConfigOption::select( + "effort", + "Thinking", + "high", + effort_select_options(&["default", "low", "high", "xhigh"]), + ) + .category(SessionConfigOptionCategory::ThoughtLevel), + ]; + + let capability = + extract_effort_capability(&options).expect("effort option should be found"); + + assert_eq!(capability.option_id, "effort"); + assert_eq!(capability.current.as_deref(), Some("high")); + assert_eq!( + capability + .values + .iter() + .map(|option| option.value.as_str()) + .collect::>(), + vec!["default", "low", "high", "xhigh"] + ); + } + + #[test] + fn extract_effort_capability_falls_back_to_effort_option_id() { + let options = vec![SessionConfigOption::select( + "effort", + "Reasoning", + "low", + effort_select_options(&["low", "high"]), + )]; + + let capability = + extract_effort_capability(&options).expect("effort option should be found"); + + assert_eq!(capability.option_id, "effort"); + assert_eq!(capability.current.as_deref(), Some("low")); + } + + #[test] + fn extract_effort_capability_keeps_agent_labels() { + let options = vec![SessionConfigOption::select( + "effort", + "Thinking", + "default", + vec![ + SessionConfigSelectOption::new("default", "Default"), + SessionConfigSelectOption::new("xhigh", "Extra high"), + ], + ) + .category(SessionConfigOptionCategory::ThoughtLevel)]; + + let capability = + extract_effort_capability(&options).expect("effort option should be found"); + + assert_eq!( + capability + .values + .iter() + .map(|option| (option.value.as_str(), option.label.as_str())) + .collect::>(), + vec![("default", "Default"), ("xhigh", "Extra high")] + ); + } + + #[test] + fn extract_effort_capability_flattens_grouped_values() { + let options = vec![SessionConfigOption::select( + "effort", + "Thinking", + "medium", + vec![ + SessionConfigSelectGroup::new( + "standard", + "Standard", + effort_select_options(&["low", "medium"]), + ), + SessionConfigSelectGroup::new( + "extended", + "Extended", + effort_select_options(&["high", "max"]), + ), + ], + ) + .category(SessionConfigOptionCategory::ThoughtLevel)]; + + let capability = + extract_effort_capability(&options).expect("effort option should be found"); + + assert_eq!( + capability + .values + .iter() + .map(|option| option.value.as_str()) + .collect::>(), + vec!["low", "medium", "high", "max"] + ); + } + + #[test] + fn extract_effort_capability_returns_none_without_effort_option() { + let options = vec![SessionConfigOption::select( + "model", + "Model", + "sonnet", + effort_select_options(&["sonnet"]), + ) + .category(SessionConfigOptionCategory::Model)]; + + assert!(extract_effort_capability(&options).is_none()); + } + + #[test] + fn config_option_update_replaces_effort_state() { + let effort_state = Arc::new(Mutex::new(Some(effort_capability(&["low"], "low")))); + let update = ConfigOptionUpdate::new(vec![SessionConfigOption::select( + "effort", + "Thinking", + "xhigh", + effort_select_options(&["default", "xhigh"]), + ) + .category(SessionConfigOptionCategory::ThoughtLevel)]); + + refresh_effort_state(&effort_state, &update.config_options); + + let state = effort_state + .lock() + .unwrap() + .clone() + .expect("state replaced"); + assert_eq!(state.current.as_deref(), Some("xhigh")); + assert_eq!( + state + .values + .iter() + .map(|option| option.value.as_str()) + .collect::>(), + vec!["default", "xhigh"] + ); + } + + #[test] + fn refresh_effort_state_clears_when_agent_drops_the_option() { + let effort_state = Arc::new(Mutex::new(Some(effort_capability(&["low", "high"], "low")))); + + refresh_effort_state( + &effort_state, + &[SessionConfigOption::select( + "model", + "Model", + "sonnet", + effort_select_options(&["sonnet"]), + ) + .category(SessionConfigOptionCategory::Model)], + ); + + assert!(effort_state.lock().unwrap().is_none()); + } + + #[test] + fn publish_effort_state_notifies_subscribers() { + let effort = AcpEffortState::new(); + let mut subscriber = effort.updates.subscribe(); + + publish_effort_state( + &effort, + &[SessionConfigOption::select( + "effort", + "Thinking", + "high", + effort_select_options(&["default", "high"]), + ) + .category(SessionConfigOptionCategory::ThoughtLevel)], + ); + + assert!(subscriber.has_changed().unwrap()); + assert_eq!( + subscriber.borrow_and_update().clone(), + ThinkingEffortSupport::Options(effort_capability(&["default", "high"], "high")) + ); + } + + #[test] + fn clearing_effort_state_allows_same_options_to_be_republished() { + let effort = AcpEffortState::new(); + let mut subscriber = effort.updates.subscribe(); + let capability = effort_capability(&["default", "high"], "high"); + + replace_effort_state(&effort, Some(capability.clone())); + assert!(subscriber.has_changed().unwrap()); + subscriber.borrow_and_update(); + + assert_eq!( + replace_effort_state(&effort, None), + Some(capability.clone()) + ); + assert!(subscriber.has_changed().unwrap()); + assert_eq!( + subscriber.borrow_and_update().clone(), + ThinkingEffortSupport::Unsupported + ); + + replace_effort_state(&effort, Some(capability.clone())); + assert!(subscriber.has_changed().unwrap()); + assert_eq!( + subscriber.borrow_and_update().clone(), + ThinkingEffortSupport::Options(capability) + ); + } + + #[test_case("high" => Some("high".to_string()) ; "exact match passes through")] + #[test_case("HIGH" => Some("high".to_string()) ; "match ignores case")] + #[test_case("off" => Some("default".to_string()) ; "off maps to the agent default")] + #[test_case("max" => Some("xhigh".to_string()) ; "max maps to xhigh")] + #[test_case("medium" => None ; "value the agent does not offer is not forwarded")] + fn map_effort_value_maps_goose_values_onto_agent_values(value: &str) -> Option { + map_effort_value( + &effort_capability(&["default", "low", "high", "xhigh"], "default"), + value, + ) + } + + #[test] + fn map_effort_value_prefers_max_when_agent_offers_it() { + let capability = effort_capability(&["default", "max", "xhigh"], "default"); + + assert_eq!( + map_effort_value(&capability, "max"), + Some("max".to_string()) + ); + assert_eq!( + map_effort_value(&capability, "xhigh"), + Some("xhigh".to_string()) + ); + } + + #[test] + fn thinking_effort_support_reports_agent_options() { + let (tx, _rx) = mpsc::channel(1); + let capability = effort_capability(&["default", "high"], "default"); + let provider = test_provider_with_effort(tx, Some(capability.clone())); + + assert_eq!( + provider.thinking_effort_support(), + ThinkingEffortSupport::Options(capability) + ); + } + + #[test] + fn thinking_effort_support_is_unsupported_without_an_effort_option() { + let (provider, _) = test_provider(); + + assert_eq!( + provider.thinking_effort_support(), + ThinkingEffortSupport::Unsupported + ); + } + + #[tokio::test] + async fn set_thinking_effort_forwards_the_pick_to_the_agent() { + let (tx, mut rx) = mpsc::channel(1); + let provider = + test_provider_with_effort(tx, Some(effort_capability(&["default", "high"], "default"))); + + let handle = tokio::spawn(async move { + provider + .set_thinking_effort("session", "high") + .await + .unwrap() + }); + + assert_eq!( + expect_set_config_option(&mut rx).await, + ("effort".to_string(), "high".to_string()) + ); + + assert!(handle.await.unwrap()); + } + + #[tokio::test] + async fn set_thinking_effort_is_applied_as_a_no_op_without_an_effort_option() { + let (tx, mut rx) = mpsc::channel(1); + let provider = test_provider_with_effort(tx, None); + + assert!(provider + .set_thinking_effort("session", "off") + .await + .unwrap()); + assert!(rx.try_recv().is_err()); + } + + #[tokio::test] + async fn set_thinking_effort_rejects_non_off_value_without_an_effort_option() { + let (tx, mut rx) = mpsc::channel(1); + let provider = test_provider_with_effort(tx, None); + + let result = provider.set_thinking_effort("session", "high").await; + + assert!(matches!(result, Err(ProviderError::InvalidValue(_)))); + assert!(rx.try_recv().is_err()); + } + + #[tokio::test] + async fn set_thinking_effort_rejects_values_the_agent_does_not_offer() { + let (tx, mut rx) = mpsc::channel(1); + let provider = + test_provider_with_effort(tx, Some(effort_capability(&["default", "high"], "default"))); + + let result = provider.set_thinking_effort("session", "medium").await; + + assert!(matches!(result, Err(ProviderError::InvalidValue(_)))); + assert!(rx.try_recv().is_err()); + } + + #[tokio::test] + async fn set_thinking_effort_reports_an_agent_value_rejection_as_invalid() { + let (tx, mut rx) = mpsc::channel(1); + let provider = + test_provider_with_effort(tx, Some(effort_capability(&["default", "high"], "default"))); + + let handle = + tokio::spawn(async move { provider.set_thinking_effort("session", "high").await }); + fail_set_config_option(&mut rx, agent_client_protocol::Error::invalid_params()).await; + + assert!(matches!( + handle.await.unwrap(), + Err(ProviderError::InvalidValue(_)) + )); + } + + /// A dead subprocess or dropped connection surfaces as `internal_error` from + /// the client library, which says nothing about the value the client picked. + #[tokio::test] + async fn set_thinking_effort_reports_a_transport_failure_as_a_request_failure() { + let (tx, mut rx) = mpsc::channel(1); + let provider = + test_provider_with_effort(tx, Some(effort_capability(&["default", "high"], "default"))); + + let handle = + tokio::spawn(async move { provider.set_thinking_effort("session", "high").await }); + fail_set_config_option(&mut rx, agent_client_protocol::Error::internal_error()).await; + + assert!(matches!( + handle.await.unwrap(), + Err(ProviderError::RequestFailed(_)) + )); + } + + #[tokio::test] + async fn set_thinking_effort_reports_a_dropped_response_as_a_request_failure() { + let (tx, mut rx) = mpsc::channel(1); + let provider = + test_provider_with_effort(tx, Some(effort_capability(&["default", "high"], "default"))); + + let handle = + tokio::spawn(async move { provider.set_thinking_effort("session", "high").await }); + drop(rx.recv().await.expect("expected a SetConfigOption request")); + + assert!(matches!( + handle.await.unwrap(), + Err(ProviderError::RequestFailed(_)) + )); + } + + #[tokio::test] + async fn apply_effort_if_changed_forwards_the_persisted_value() { + let (tx, mut rx) = mpsc::channel(1); + let provider = test_provider_with_effort( + tx, + Some(effort_capability(&["default", "low", "xhigh"], "default")), + ); + let model = model_with_effort("max"); + + let handle = + tokio::spawn(async move { provider.apply_effort_if_changed(&model).await.unwrap() }); + + assert_eq!( + expect_set_config_option(&mut rx).await, + ("effort".to_string(), "xhigh".to_string()) + ); + + handle.await.unwrap(); + } + + #[tokio::test] + async fn apply_effort_if_changed_skips_when_agent_already_has_the_value() { + let (tx, mut rx) = mpsc::channel(1); + let provider = + test_provider_with_effort(tx, Some(effort_capability(&["default", "high"], "high"))); + + provider + .apply_effort_if_changed(&model_with_effort("high")) + .await + .unwrap(); + + assert!(rx.try_recv().is_err()); + } + + /// A refresh that reverts the agent's own effort (e.g. a model switch + /// rebuilding per-model levels) must not be masked by a stale record of + /// what goose last sent: the persisted value gets re-applied. + #[tokio::test] + async fn apply_effort_if_changed_resends_after_the_agent_resets_its_effort() { + let (tx, mut rx) = mpsc::channel(1); + let provider = + test_provider_with_effort(tx, Some(effort_capability(&["default", "high"], "high"))); + refresh_effort_state( + &provider.effort.capability, + &[SessionConfigOption::select( + "effort", + "Thinking", + "default", + effort_select_options(&["default", "high"]), + ) + .category(SessionConfigOptionCategory::ThoughtLevel)], + ); + + let handle = tokio::spawn(async move { + provider + .apply_effort_if_changed(&model_with_effort("high")) + .await + .unwrap() + }); + + assert_eq!( + expect_set_config_option(&mut rx).await, + ("effort".to_string(), "high".to_string()) + ); + + handle.await.unwrap(); + } + + /// The menu advertises the global default once the persisted pick stops + /// being offered (a model switch rebuilt the agent's selector), so the send + /// path has to apply it too — otherwise the agent silently runs at its own + /// current while the client shows the global. + #[tokio::test] + async fn apply_effort_if_changed_falls_back_to_the_global_default() { + let _guard = env_lock::lock_env([("GOOSE_THINKING_EFFORT", Some("high"))]); + let (tx, mut rx) = mpsc::channel(1); + let provider = + test_provider_with_effort(tx, Some(effort_capability(&["default", "high"], "default"))); + let model = model_with_effort("medium"); + + let handle = + tokio::spawn(async move { provider.apply_effort_if_changed(&model).await.unwrap() }); + + assert_eq!( + expect_set_config_option(&mut rx).await, + ("effort".to_string(), "high".to_string()) + ); + + handle.await.unwrap(); + } + + #[tokio::test] + async fn apply_effort_if_changed_skips_unmapped_value() { + // Unoffered by the capability below, so the global default never wins + // and the test doesn't depend on the machine's configured value. + let _guard = env_lock::lock_env([("GOOSE_THINKING_EFFORT", Some("medium"))]); + let (tx, mut rx) = mpsc::channel(1); + let provider = + test_provider_with_effort(tx, Some(effort_capability(&["default", "high"], "default"))); + + provider + .apply_effort_if_changed(&model_with_effort("medium")) + .await + .unwrap(); + + assert!(rx.try_recv().is_err()); + } + + #[tokio::test] + async fn apply_effort_if_changed_skips_without_an_effort_option() { + let (tx, mut rx) = mpsc::channel(1); + let provider = test_provider_with_effort(tx, None); + + provider + .apply_effort_if_changed(&model_with_effort("high")) + .await + .unwrap(); + + assert!(rx.try_recv().is_err()); + } + + #[tokio::test] + async fn apply_effort_if_changed_skips_session_without_a_persisted_value() { + let _guard = env_lock::lock_env([("GOOSE_THINKING_EFFORT", Some("medium"))]); + let (tx, mut rx) = mpsc::channel(1); + let provider = + test_provider_with_effort(tx, Some(effort_capability(&["default", "high"], "default"))); + + provider + .apply_effort_if_changed(&ModelConfig::new(ACP_CURRENT_MODEL)) + .await + .unwrap(); + + assert!(rx.try_recv().is_err()); + } + fn test_acp_config( mode_mapping: HashMap>, session_mode_id: Option, diff --git a/crates/goose/src/acp/response_builder.rs b/crates/goose/src/acp/response_builder.rs index 2eb9ad6a6..0a8dae16a 100644 --- a/crates/goose/src/acp/response_builder.rs +++ b/crates/goose/src/acp/response_builder.rs @@ -1,4 +1,4 @@ -use crate::agents::ExtensionLoadResult; +use crate::agents::{Agent, ExtensionLoadResult}; use crate::config::{Config, GooseMode}; use crate::providers::inventory::{ProviderInventoryEntry, ProviderInventoryService}; use crate::session::session_manager::SessionUsageTotals; @@ -11,10 +11,11 @@ use agent_client_protocol::schema::v1::{ }; use agent_client_protocol::{Client, ConnectionTo}; use goose_providers::model::ModelConfig; -use goose_providers::thinking::ThinkingEffort; +use goose_providers::thinking::{ThinkingEffort, ThinkingEffortCapability, ThinkingEffortSupport}; use serde::Serialize; use strum::{EnumMessage, VariantNames}; +use super::provider::resolve_effort_value; use super::server::{build_usage_updates, DEFAULT_PROVIDER_ID, DEFAULT_PROVIDER_LABEL}; pub(super) fn session_provider_selection(session: &Session) -> &str { @@ -232,9 +233,19 @@ pub(super) fn build_mode_state( )) } +/// The provider decides whether goose owns the effort menu; a session without a +/// live provider keeps the model-name based path. +pub(super) async fn agent_thinking_effort_support(agent: &Agent) -> ThinkingEffortSupport { + match agent.provider().await { + Ok(provider) => provider.thinking_effort_support(), + Err(_) => ThinkingEffortSupport::Unspecified, + } +} + pub(super) async fn build_session_setup_config( provider_inventory: &ProviderInventoryService, session: &Session, + effort_support: &ThinkingEffortSupport, ) -> Result<(SessionModeState, Option>), agent_client_protocol::Error> { let mode_state = build_mode_state(session.goose_mode)?; @@ -259,6 +270,7 @@ pub(super) async fn build_session_setup_config( model_config, provider_selection, provider_options, + effort_support, ); Ok((mode_state, Some(config_options))) } @@ -269,6 +281,7 @@ pub(super) fn build_config_options( model_config: &ModelConfig, provider_selection: &str, provider_options: Vec, + effort_support: &ThinkingEffortSupport, ) -> Vec { let mode_options: Vec = mode_state .available_modes @@ -283,14 +296,8 @@ pub(super) fn build_config_options( .iter() .map(|m| SessionConfigSelectOption::new(m.id.clone(), m.name.clone())) .collect(); - let thinking_effort_options = thinking_effort_values(model_config) - .iter() - .map(|effort| { - let effort = effort.to_string(); - SessionConfigSelectOption::new(effort.clone(), effort) - }) - .collect::>(); - let current_thinking_effort = current_thinking_effort_value(model_config); + let (thinking_effort_options, current_thinking_effort) = + build_thinking_effort_choices(model_config, effort_support); vec![ SessionConfigOption::select( "provider", @@ -323,6 +330,59 @@ pub(super) fn build_config_options( ] } +/// A provider that manages its own reasoning decides the menu: it mirrors the +/// agent's effort selector verbatim, or honestly offers nothing when the agent +/// has no such knob for the current model. Only providers that leave effort to +/// goose fall back to the model-name based values, which the ACP sentinel model +/// name can never satisfy. +fn build_thinking_effort_choices( + model_config: &ModelConfig, + effort_support: &ThinkingEffortSupport, +) -> (Vec, String) { + match effort_support { + ThinkingEffortSupport::Options(capability) => ( + capability + .values + .iter() + .map(|option| { + SessionConfigSelectOption::new(option.value.clone(), option.label.clone()) + }) + .collect(), + capability_thinking_effort_value(capability, model_config), + ), + ThinkingEffortSupport::Unsupported => { + let off = ThinkingEffort::Off.to_string(); + ( + vec![SessionConfigSelectOption::new(off.clone(), off.clone())], + off, + ) + } + ThinkingEffortSupport::Unspecified => ( + thinking_effort_values(model_config) + .iter() + .map(|effort| { + let effort = effort.to_string(); + SessionConfigSelectOption::new(effort.clone(), effort) + }) + .collect(), + current_thinking_effort_value(model_config), + ), + } +} + +/// The goose-side value that will actually be sent wins — `resolve_effort_value` +/// is shared with the provider's send path — then whatever the agent currently +/// has, which is what it keeps when goose sends nothing. +fn capability_thinking_effort_value( + capability: &ThinkingEffortCapability, + model_config: &ModelConfig, +) -> String { + resolve_effort_value(capability, model_config) + .or_else(|| capability.current.clone()) + .or_else(|| capability.values.first().map(|option| option.value.clone())) + .unwrap_or_default() +} + fn thinking_effort_values(model_config: &ModelConfig) -> &'static [ThinkingEffort] { if model_config.is_reasoning_model() { &[ @@ -423,8 +483,10 @@ pub(super) fn send_session_setup_notifications( #[cfg(test)] mod tests { + use super::super::provider::THINKING_EFFORT_PARAM; use super::*; use agent_client_protocol::schema::v1::SessionConfigKind; + use goose_providers::thinking::ThinkingEffortOption; use test_case::test_case; fn model_selection(current: &str, models: &[&str]) -> ModelSelection { @@ -660,6 +722,7 @@ mod tests { &model_config, provider_name, provider_options, + &ThinkingEffortSupport::Unspecified, ) } @@ -680,6 +743,7 @@ mod tests { &model_config, "openai", vec![SessionConfigSelectOption::new("openai", "openai")], + &ThinkingEffortSupport::Unspecified, ); let option = options .iter() @@ -709,6 +773,7 @@ mod tests { &model_config, "openai", vec![SessionConfigSelectOption::new("openai", "openai")], + &ThinkingEffortSupport::Unspecified, ); let option = options .iter() @@ -727,4 +792,123 @@ mod tests { ]) ); } + + fn effort_capability(values: &[&str], current: &str) -> ThinkingEffortCapability { + ThinkingEffortCapability { + option_id: "effort".to_string(), + values: values + .iter() + .map(|value| ThinkingEffortOption { + value: value.to_string(), + label: value.to_string(), + }) + .collect(), + current: Some(current.to_string()), + } + } + + fn build_effort_option( + model_name: &str, + persisted_effort: Option<&str>, + effort_support: &ThinkingEffortSupport, + ) -> (String, Vec) { + let mode_state = build_mode_state(GooseMode::Auto).unwrap(); + let model_state = model_selection(model_name, &[model_name]); + let mut model_config = ModelConfig::new(model_name); + if let Some(effort) = persisted_effort { + model_config = + model_config.with_merged_request_params(std::collections::HashMap::from([( + THINKING_EFFORT_PARAM.to_string(), + serde_json::json!(effort), + )])); + } + + let options = build_config_options( + &mode_state, + &model_state, + &model_config, + "claude-acp", + vec![SessionConfigSelectOption::new("claude-acp", "claude-acp")], + effort_support, + ); + let option = options + .into_iter() + .find(|option| option.id.0.as_ref() == "thinking_effort") + .expect("thinking_effort option"); + let SessionConfigKind::Select(select) = option.kind else { + panic!("thinking_effort should be a select option"); + }; + let values = match select.options { + agent_client_protocol::schema::v1::SessionConfigSelectOptions::Ungrouped(values) => { + values + } + grouped => panic!("unexpected thinking_effort options: {grouped:?}"), + }; + (select.current_value.0.to_string(), values) + } + + #[test] + fn test_build_config_options_mirrors_agent_effort_menu_on_the_model_sentinel() { + let model_name = crate::acp::ACP_CURRENT_MODEL; + assert!( + !ModelConfig::new(model_name).is_reasoning_model(), + "the sentinel is the model name that made the old sniffing path collapse the menu" + ); + let support = ThinkingEffortSupport::Options(effort_capability( + &["default", "high", "xhigh"], + "high", + )); + + let (current, values) = build_effort_option(model_name, Some("high"), &support); + + assert_eq!(current, "high"); + assert_eq!( + values, + vec![ + SessionConfigSelectOption::new("default", "default"), + SessionConfigSelectOption::new("high", "high"), + SessionConfigSelectOption::new("xhigh", "xhigh"), + ] + ); + } + + #[test_case(Some("high") => "high".to_string() ; "persisted value")] + #[test_case(Some("off") => "default".to_string() ; "persisted off maps onto the agent default")] + #[test_case(Some("max") => "xhigh".to_string() ; "persisted max maps onto the agent's xhigh")] + #[test_case(Some("unknown") => "low".to_string() ; "unmappable persisted value falls back to the agent")] + #[test_case(None => "low".to_string() ; "no persisted value falls back to the agent")] + fn test_build_config_options_effort_current_value(persisted: Option<&str>) -> String { + // Unoffered by the capability below, so the global default never wins + // and the test doesn't depend on the machine's configured value. + let _guard = env_lock::lock_env([("GOOSE_THINKING_EFFORT", Some("medium"))]); + let support = ThinkingEffortSupport::Options(effort_capability( + &["default", "low", "high", "xhigh"], + "low", + )); + + build_effort_option(crate::acp::ACP_CURRENT_MODEL, persisted, &support).0 + } + + #[test] + fn test_build_config_options_effort_falls_back_to_the_global_default() { + let _guard = env_lock::lock_env([("GOOSE_THINKING_EFFORT", Some("high"))]); + let support = + ThinkingEffortSupport::Options(effort_capability(&["default", "high"], "default")); + + let (current, _) = build_effort_option(crate::acp::ACP_CURRENT_MODEL, None, &support); + + assert_eq!(current, "high"); + } + + #[test] + fn test_build_config_options_offers_no_effort_when_the_agent_has_none() { + let (current, values) = build_effort_option( + "claude-sonnet-4", + Some("high"), + &ThinkingEffortSupport::Unsupported, + ); + + assert_eq!(current, "off"); + assert_eq!(values, vec![SessionConfigSelectOption::new("off", "off")]); + } } diff --git a/crates/goose/src/acp/server.rs b/crates/goose/src/acp/server.rs index 80f90c50f..7f3735743 100644 --- a/crates/goose/src/acp/server.rs +++ b/crates/goose/src/acp/server.rs @@ -2,9 +2,10 @@ use crate::acp::custom_notifications::*; use crate::acp::custom_requests::*; use crate::acp::fs::AcpTools; pub(super) use crate::acp::response_builder::{ - build_config_options, build_mode_state, build_model_state, build_provider_options, - build_session_info, build_session_setup_config, send_session_setup_notifications, session_meta, - session_provider_selection, session_response_meta, should_refresh_inventory_for_session_init, + agent_thinking_effort_support, build_config_options, build_mode_state, build_model_state, + build_provider_options, build_session_info, build_session_setup_config, + send_session_setup_notifications, session_meta, session_provider_selection, + session_response_meta, should_refresh_inventory_for_session_init, }; use crate::acp::tool_call_notifier::ToolCallNotifier; use crate::acp::{PermissionDecision, ACP_CURRENT_MODEL}; @@ -23,6 +24,7 @@ use crate::conversation::message::{ ActionRequiredData, Message, MessageContent, SystemNotificationContent, SystemNotificationType, ToolRequest, ToolResponse, }; +use crate::conversation::Conversation; use crate::execution::manager::{AgentManager, AgentManagerGetResult, RuntimeContext}; use crate::permission::permission_confirmation::PrincipalType; use crate::permission::{Permission, PermissionConfirmation}; @@ -63,6 +65,7 @@ use anyhow::Result; use fs_err as fs; use futures::future::{BoxFuture, FutureExt}; use futures::stream::{self, StreamExt}; +use goose_providers::errors::ProviderError; use rmcp::model::{ Annotations as RmcpAnnotations, ImageContent as RmcpImageContent, Role, TextContent as RmcpTextContent, @@ -72,7 +75,7 @@ use std::collections::{HashMap, HashSet}; use std::panic::AssertUnwindSafe; use std::path::{Path, PathBuf}; use std::sync::Arc; -use tokio::sync::{Mutex, OnceCell}; +use tokio::sync::{mpsc, Mutex, OnceCell}; use tokio_util::compat::{TokioAsyncReadCompatExt as _, TokioAsyncWriteCompatExt as _}; use tokio_util::sync::CancellationToken; use tracing::{debug, error, info, warn}; @@ -170,6 +173,41 @@ fn agent_creation_error(error: anyhow::Error, context: &str) -> agent_client_pro } } +/// Only a value the client could usefully change is `invalid_params`; everything +/// else (a dead agent subprocess, a failed persist, a failed provider respawn) is +/// an operational failure the client cannot fix by picking differently. +fn thinking_effort_error(error: anyhow::Error) -> agent_client_protocol::Error { + let base = match error.downcast_ref::() { + Some(ProviderError::InvalidValue(_)) => agent_client_protocol::Error::invalid_params(), + _ => agent_client_protocol::Error::internal_error(), + }; + // `{error:#}` rather than `{error}`: context layering hides the cause chain, + // including the variant this mapping branched on. + base.data(format!("Failed to update thinking effort: {error:#}")) +} + +async fn resume_saved_provider_session( + provider: &Arc, + conversation: Option<&Conversation>, +) { + let Some(conversation) = conversation else { + return; + }; + let provider_name = provider.get_name(); + let Some(session_id) = + crate::agents::latest_provider_session_id(conversation.messages(), provider_name) + else { + return; + }; + if let Err(error) = provider.resume(session_id).await { + warn!( + provider = provider_name, + %error, + "Could not resume provider session during ACP session setup" + ); + } +} + pub(super) const DEFAULT_PROVIDER_ID: &str = "goose"; pub(super) const DEFAULT_PROVIDER_LABEL: &str = "Goose (Default)"; const PROVIDER_CONFIG_STATUS_CHECK_CONCURRENCY: usize = 16; @@ -243,6 +281,8 @@ pub struct GooseAcpAgent { client_requests_tool_call_label_enrichment: OnceCell, use_login_shell_path: OnceCell, client_cx: OnceCell>, + thinking_effort_update_tx: mpsc::UnboundedSender, + thinking_effort_update_rx: Mutex>>, config_dir: std::path::PathBuf, session_manager: Arc, permission_manager: Arc, @@ -779,6 +819,7 @@ impl GooseAcpAgent { options.goose_platform.clone(), ); let agent_manager = Arc::new(AgentManager::new(agent_config, None).await?); + let (thinking_effort_update_tx, thinking_effort_update_rx) = mpsc::unbounded_channel(); Ok(Self { sessions: Arc::new(Mutex::new(HashMap::new())), @@ -796,6 +837,8 @@ impl GooseAcpAgent { client_requests_tool_call_label_enrichment: OnceCell::new(), use_login_shell_path: OnceCell::new(), client_cx: OnceCell::new(), + thinking_effort_update_tx, + thinking_effort_update_rx: Mutex::new(Some(thinking_effort_update_rx)), config_dir: options.config_dir, session_manager, permission_manager, @@ -1038,8 +1081,70 @@ impl GooseAcpAgent { } async fn register_acp_session(&self, session_id: String, agent: Arc) { - let acp_session = GooseAcpSession { agent }; - self.sessions.lock().await.insert(session_id, acp_session); + let acp_session = GooseAcpSession { + agent: agent.clone(), + }; + self.sessions + .lock() + .await + .insert(session_id.clone(), acp_session); + self.subscribe_thinking_effort_updates(&session_id, &agent) + .await; + } + + async fn subscribe_thinking_effort_updates(&self, session_id: &str, agent: &Arc) { + let Ok(provider) = agent.provider().await else { + return; + }; + let Some(mut updates) = provider.subscribe_thinking_effort_support() else { + return; + }; + let session_id = session_id.to_string(); + let tx = self.thinking_effort_update_tx.clone(); + tokio::spawn(async move { + while updates.changed().await.is_ok() { + if tx.send(session_id.clone()).is_err() { + break; + } + } + }); + } + + async fn start_thinking_effort_update_forwarder(self: &Arc, cx: &ConnectionTo) { + let Some(mut updates) = self.thinking_effort_update_rx.lock().await.take() else { + return; + }; + let agent = Arc::downgrade(self); + let cx = cx.clone(); + tokio::spawn(async move { + while let Some(session_id) = updates.recv().await { + let Some(agent) = agent.upgrade() else { + break; + }; + if agent.closed_session_ids.lock().await.contains(&session_id) { + continue; + } + let session_id = SessionId::new(session_id); + match agent.build_config_update(&session_id).await { + Ok((notification, _)) => { + if let Err(error) = cx.send_notification(notification) { + warn!( + session_id = %session_id, + %error, + "Failed to forward thinking-effort config update" + ); + } + } + Err(error) => { + warn!( + session_id = %session_id, + ?error, + "Failed to build thinking-effort config update" + ); + } + } + } + }); } async fn activate_acp_session( @@ -2128,6 +2233,8 @@ impl GooseAcpAgent { .recreate_provider_for_session(session_id, &provider_name, model_config) .await .internal_err_ctx("Failed to recreate provider")?; + self.subscribe_thinking_effort_updates(session_id, &agent) + .await; // model_config is already updated on the session by the agent's update_provider call. Ok(()) } @@ -2171,6 +2278,7 @@ impl GooseAcpAgent { ¤t_model_config, session_provider_selection(&session), provider_options, + &provider.thinking_effort_support(), ); let notification = SessionNotification::new( session_id.clone(), @@ -2205,17 +2313,11 @@ impl GooseAcpAgent { session_id: &str, effort_id: &str, ) -> Result<(), agent_client_protocol::Error> { - let effort = effort_id - .parse::() - .map_err(|_| { - agent_client_protocol::Error::invalid_params() - .data(format!("Invalid thinking effort: {}", effort_id)) - })?; let agent = self.get_session_agent(session_id).await?; agent - .update_thinking_effort(session_id, effort) + .update_thinking_effort(session_id, effort_id) .await - .internal_err_ctx("Failed to update thinking effort")?; + .map_err(thinking_effort_error)?; Ok(()) } @@ -2279,6 +2381,8 @@ impl GooseAcpAgent { .recreate_provider_for_session(session_id, &resolved_provider_name, model_config) .await .internal_err_ctx("Failed to recreate provider")?; + self.subscribe_thinking_effort_updates(session_id, &agent) + .await; // provider_name is already updated on the session by the agent's update_provider call. Ok(()) @@ -2415,11 +2519,71 @@ mod tests { SelectedPermissionOutcome, TextResourceContents, }; use goose_providers::conversation::token_usage::Usage as TokenUsage; + use goose_providers::thinking::{ + ThinkingEffortCapability, ThinkingEffortOption, ThinkingEffortSupport, + }; use std::io::Write; use std::path::PathBuf; use tempfile::NamedTempFile; use test_case::test_case; + #[derive(Debug)] + struct AsyncEffortProvider { + updates: tokio::sync::watch::Sender, + } + + impl AsyncEffortProvider { + fn new() -> Self { + let (updates, _) = tokio::sync::watch::channel(Self::support("low", &["low", "high"])); + Self { updates } + } + + fn support(current: &str, values: &[&str]) -> ThinkingEffortSupport { + ThinkingEffortSupport::Options(ThinkingEffortCapability { + option_id: "effort".to_string(), + values: values + .iter() + .map(|value| ThinkingEffortOption { + value: value.to_string(), + label: value.to_string(), + }) + .collect(), + current: Some(current.to_string()), + }) + } + + fn update(&self, current: &str, values: &[&str]) { + self.updates.send_replace(Self::support(current, values)); + } + } + + #[async_trait::async_trait] + impl Provider for AsyncEffortProvider { + fn get_name(&self) -> &str { + "openai" + } + + fn thinking_effort_support(&self) -> ThinkingEffortSupport { + self.updates.borrow().clone() + } + + fn subscribe_thinking_effort_support( + &self, + ) -> Option> { + Some(self.updates.subscribe()) + } + + async fn stream( + &self, + _model_config: &goose_providers::model::ModelConfig, + _system: &str, + _messages: &[Message], + _tools: &[rmcp::model::Tool], + ) -> Result { + Ok(Box::pin(futures::stream::empty())) + } + } + #[test] fn agent_creation_auth_error_maps_to_auth_required() { let error = anyhow::Error::new(agent_client_protocol::Error::auth_required()); @@ -3143,4 +3307,158 @@ print(\"hello, world\") .and_then(|goose| goose.tool_call_label_enrichment) .unwrap_or(false)); } + + #[test] + fn thinking_effort_error_maps_a_rejected_value_to_invalid_params() { + let error = thinking_effort_error( + anyhow::Error::new(ProviderError::InvalidValue( + "Agent offers no thinking effort 'medium'".to_string(), + )) + .context("Provider rejected thinking effort update"), + ); + + assert_eq!(error.code, agent_client_protocol::ErrorCode::InvalidParams); + // The cause chain, not just the outermost context, reaches the client. + let data = error.data.unwrap().to_string(); + assert!(data.contains("Provider rejected thinking effort update")); + assert!(data.contains("Agent offers no thinking effort 'medium'")); + } + + #[test] + fn thinking_effort_error_maps_an_operational_failure_to_internal_error() { + let error = thinking_effort_error( + anyhow::Error::new(ProviderError::RequestFailed( + "Failed to set ACP effort option: agent is gone".to_string(), + )) + .context("Provider rejected thinking effort update"), + ); + + assert_eq!(error.code, agent_client_protocol::ErrorCode::InternalError); + } + + #[test] + fn thinking_effort_error_maps_an_untyped_failure_to_internal_error() { + let error = thinking_effort_error(anyhow::anyhow!("Failed to persist thinking effort")); + + assert_eq!(error.code, agent_client_protocol::ErrorCode::InternalError); + } + + #[tokio::test] + async fn asynchronous_provider_effort_update_is_forwarded_to_client() { + let root = tempfile::tempdir().unwrap(); + let provider_factory: AcpProviderFactory = Arc::new( + |_provider_name, _extensions, _working_dir, _use_default_model| { + Box::pin(async { Err(anyhow::anyhow!("unused provider factory")) }) + }, + ); + let server = Arc::new( + GooseAcpAgent::new(GooseAcpAgentOptions { + provider_factory, + builtin_selection: AcpBuiltinSelection::default(), + data_dir: root.path().to_path_buf(), + config_dir: root.path().to_path_buf(), + disable_session_naming: true, + goose_platform: GoosePlatform::GooseCli, + additional_source_roots: Vec::new(), + scheduler: None, + }) + .await + .unwrap(), + ); + let session = server + .session_manager + .create_session( + root.path().to_path_buf(), + "Effort update test".to_string(), + SessionType::Acp, + GooseMode::Auto, + ) + .await + .unwrap(); + let session_agent = Arc::new(Agent::with_config(AgentConfig::new( + server.session_manager.clone(), + server.permission_manager.clone(), + None, + GooseMode::Auto, + true, + GoosePlatform::GooseCli, + ))); + let provider = Arc::new(AsyncEffortProvider::new()); + session_agent + .update_provider( + provider.clone(), + goose_providers::model::ModelConfig::new("gpt-4o").with_merged_request_params( + HashMap::from([("thinking_effort".to_string(), serde_json::json!("xhigh"))]), + ), + &session.id, + ) + .await + .unwrap(); + server + .register_acp_session(session.id.clone(), session_agent) + .await; + + let (client_read, server_write) = tokio::io::duplex(64 * 1024); + let (server_read, client_write) = tokio::io::duplex(64 * 1024); + let (notification_tx, mut notification_rx) = + mpsc::unbounded_channel::(); + let client = tokio::spawn(async move { + Client + .builder() + .on_receive_notification( + async move |notification: SessionNotification, _cx| { + let _ = notification_tx.send(notification); + Ok(()) + }, + agent_client_protocol::on_receive_notification!(), + ) + .connect_to(ByteStreams::new( + client_write.compat_write(), + client_read.compat(), + )) + .await + }); + + let session_id = SessionId::new(session.id); + let server_for_connection = server.clone(); + SacpAgent + .builder() + .name("effort-update-test") + .connect_with( + ByteStreams::new(server_write.compat_write(), server_read.compat()), + async move |cx: ConnectionTo| { + server_for_connection + .start_thinking_effort_update_forwarder(&cx) + .await; + provider.update("xhigh", &["default", "high", "xhigh"]); + + let notification = tokio::time::timeout( + std::time::Duration::from_secs(1), + notification_rx.recv(), + ) + .await + .expect("timed out waiting for effort update") + .expect("client notification channel closed"); + assert_eq!(notification.session_id, session_id); + let SessionUpdate::ConfigOptionUpdate(update) = notification.update else { + panic!("expected config option update"); + }; + let option = update + .config_options + .iter() + .find(|option| option.id.0.as_ref() == "thinking_effort") + .expect("thinking_effort option"); + let agent_client_protocol::schema::v1::SessionConfigKind::Select(select) = + &option.kind + else { + panic!("thinking_effort should be a select option"); + }; + assert_eq!(select.current_value.0.as_ref(), "xhigh"); + Ok(()) + }, + ) + .await + .unwrap(); + client.await.unwrap().unwrap(); + } } diff --git a/crates/goose/src/acp/server/dispatch.rs b/crates/goose/src/acp/server/dispatch.rs index ce369ad41..86ed61476 100644 --- a/crates/goose/src/acp/server/dispatch.rs +++ b/crates/goose/src/acp/server/dispatch.rs @@ -22,6 +22,7 @@ impl HandleDispatchFrom for GooseAcpHandler { // new_session/load_session on this connection. Set-once per // connection; the result is ignored on later requests. let _ = agent.client_cx.set(cx.clone()); + agent.start_thinking_effort_update_forwarder(&cx).await; // InitializeRequest runs inline: it sets connection-scoped state // (client fs/terminal capabilities) that later handlers read with diff --git a/crates/goose/src/acp/server/fork_session.rs b/crates/goose/src/acp/server/fork_session.rs index 46202445f..b8dfcca67 100644 --- a/crates/goose/src/acp/server/fork_session.rs +++ b/crates/goose/src/acp/server/fork_session.rs @@ -38,7 +38,7 @@ impl GooseAcpAgent { let new_session = self .session_manager - .get_session(&new_session_id, false) + .get_session(&new_session_id, true) .await .internal_err()?; @@ -47,14 +47,20 @@ impl GooseAcpAgent { new_session.clone(), args.cwd.clone(), args.mcp_servers, - false, + true, ) .await?; let (agent, extension_results) = self.prepare_acp_session_agent(cx, &goose_session).await?; self.apply_session_recipe(&agent, &goose_session).await?; - self.register_acp_session(goose_session.id.clone(), agent) + self.register_acp_session(goose_session.id.clone(), agent.clone()) .await; + let provider = agent + .provider() + .await + .internal_err_ctx("Failed to get provider while forking ACP session")?; + resume_saved_provider_session(&provider, goose_session.conversation.as_ref()).await; + let effort_support = agent_thinking_effort_support(&agent).await; let acp_session_id = SessionId::new(new_session_id.clone()); let mut meta = session_meta(&goose_session); @@ -63,7 +69,8 @@ impl GooseAcpAgent { } let (mode_state, config_options) = - build_session_setup_config(&self.provider_inventory, &goose_session).await?; + build_session_setup_config(&self.provider_inventory, &goose_session, &effort_support) + .await?; let mut response = ForkSessionResponse::new(acp_session_id.clone()) .modes(mode_state) diff --git a/crates/goose/src/acp/server/load_session.rs b/crates/goose/src/acp/server/load_session.rs index f11bcfbfc..4c4aa8dbe 100644 --- a/crates/goose/src/acp/server/load_session.rs +++ b/crates/goose/src/acp/server/load_session.rs @@ -7,7 +7,6 @@ use super::tool_calls::conversion::{ }; use super::tool_calls::enrichment::tool_chain_summary; use super::*; -use crate::conversation::Conversation; use agent_client_protocol::schema::v1::ToolCall; fn replay_audience_annotations(audience: &[Role]) -> Annotations { @@ -299,6 +298,11 @@ impl GooseAcpAgent { self.apply_session_recipe(&agent, &session).await?; self.register_acp_session(session_id_str.clone(), agent.clone()) .await; + let provider = agent + .provider() + .await + .internal_err_ctx("Failed to get provider while loading ACP session")?; + resume_saved_provider_session(&provider, session.conversation.as_ref()).await; self.resend_pending_tool_permissions(cx, &agent, &session)?; session = self @@ -312,8 +316,12 @@ impl GooseAcpAgent { .update_working_dir(&session.working_dir) .await; - let (mode_state, config_options) = - build_session_setup_config(&self.provider_inventory, &session).await?; + let (mode_state, config_options) = build_session_setup_config( + &self.provider_inventory, + &session, + &agent_thinking_effort_support(&agent).await, + ) + .await?; self.notify_session_setup(cx, &session).await?; @@ -332,7 +340,79 @@ impl GooseAcpAgent { #[cfg(test)] mod tests { use super::*; + use crate::conversation::message::InferenceMetadata; + use goose_providers::thinking::{ + ThinkingEffortCapability, ThinkingEffortOption, ThinkingEffortSupport, + }; use rmcp::model::CallToolRequestParams; + use std::sync::atomic::{AtomicBool, Ordering}; + + #[derive(Debug)] + struct ResumeEffortProvider { + resumed: AtomicBool, + } + + #[async_trait::async_trait] + impl Provider for ResumeEffortProvider { + fn get_name(&self) -> &str { + "claude-acp" + } + + async fn resume(&self, session_id: &str) -> std::result::Result<(), ProviderError> { + assert_eq!(session_id, "saved-inner-session"); + self.resumed.store(true, Ordering::Release); + Ok(()) + } + + fn thinking_effort_support(&self) -> ThinkingEffortSupport { + let value = if self.resumed.load(Ordering::Acquire) { + "high" + } else { + "low" + }; + ThinkingEffortSupport::Options(ThinkingEffortCapability { + option_id: "effort".to_string(), + values: vec![ThinkingEffortOption { + value: value.to_string(), + label: value.to_string(), + }], + current: Some(value.to_string()), + }) + } + + async fn stream( + &self, + _model_config: &goose_providers::model::ModelConfig, + _system: &str, + _messages: &[Message], + _tools: &[rmcp::model::Tool], + ) -> std::result::Result { + Ok(Box::pin(futures::stream::empty())) + } + } + + #[tokio::test] + async fn saved_provider_session_is_resumed_before_effort_snapshot() { + let provider = Arc::new(ResumeEffortProvider { + resumed: AtomicBool::new(false), + }); + let conversation = Conversation::new_unvalidated([Message::assistant().with_inference( + InferenceMetadata { + provider: "claude-acp".to_string(), + requested_model: "current".to_string(), + resolved_model: None, + provider_session_id: Some("saved-inner-session".to_string()), + }, + )]); + + let provider_dyn: Arc = provider.clone(); + resume_saved_provider_session(&provider_dyn, Some(&conversation)).await; + + let ThinkingEffortSupport::Options(capability) = provider.thinking_effort_support() else { + panic!("expected resumed effort capability"); + }; + assert_eq!(capability.current.as_deref(), Some("high")); + } #[test] fn acp_replay_populates_only_empty_marked_assistant_messages() { diff --git a/crates/goose/src/acp/server/new_session.rs b/crates/goose/src/acp/server/new_session.rs index 768dfb17a..a75e5a366 100644 --- a/crates/goose/src/acp/server/new_session.rs +++ b/crates/goose/src/acp/server/new_session.rs @@ -9,6 +9,7 @@ use super::GooseAcpAgent; use agent_client_protocol::schema::v1::{Meta, NewSessionRequest, NewSessionResponse, SessionId}; use agent_client_protocol::{Client, ConnectionTo}; use goose_providers::model::ModelConfig; +use goose_providers::thinking::ThinkingEffortSupport; use std::collections::HashMap; use std::path::PathBuf; use tracing::warn; @@ -86,7 +87,11 @@ impl GooseAcpAgent { let reloaded_session = self.reload_session(&session.id).await?; let response = self - .build_new_session_response(&reloaded_session, &extension_results) + .build_new_session_response( + &reloaded_session, + &extension_results, + &super::agent_thinking_effort_support(&agent).await, + ) .await?; Ok(response) } @@ -243,9 +248,11 @@ impl GooseAcpAgent { &self, session: &Session, extension_results: &[ExtensionLoadResult], + effort_support: &ThinkingEffortSupport, ) -> Result { let (mode_state, config_options) = - super::build_session_setup_config(&self.provider_inventory, session).await?; + super::build_session_setup_config(&self.provider_inventory, session, effort_support) + .await?; let mut response = NewSessionResponse::new(SessionId::new(session.id.clone())).modes(mode_state); diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs index 950e86305..64628a4a7 100644 --- a/crates/goose/src/agents/agent.rs +++ b/crates/goose/src/agents/agent.rs @@ -69,7 +69,7 @@ use crate::tool_monitor::RepetitionInspector; use crate::utils::is_token_cancelled; use goose_providers::conversation::token_usage::{ProviderUsage, Usage}; use goose_providers::errors::ProviderError; -use goose_providers::thinking::ThinkingEffort; +use goose_providers::thinking::{ThinkingEffort, ThinkingEffortSupport}; use regex::Regex; use rmcp::model::{ CallToolRequestParams, CallToolResult, ContentBlock, ElicitationAction, ErrorCode, ErrorData, @@ -93,6 +93,27 @@ fn provider_creation_error(error: anyhow::Error, context: impl fmt::Display) -> error.context(message) } +fn normalize_legacy_provider_thinking_effort( + mut model_config: goose_providers::model::ModelConfig, + effort_support: &ThinkingEffortSupport, +) -> goose_providers::model::ModelConfig { + let has_raw_effort = model_config + .request_params + .as_ref() + .is_some_and(|params| params.contains_key("thinking_effort")); + if !matches!(effort_support, ThinkingEffortSupport::Unspecified) + || !has_raw_effort + || model_config.thinking_effort().is_some() + { + return model_config; + } + + if let Some(params) = model_config.request_params.as_mut() { + params.remove("thinking_effort"); + } + model_config.with_default_thinking_effort(Config::global().get_goose_thinking_effort()) +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum ToolCategory { Shell, @@ -3460,9 +3481,21 @@ impl Agent { .unwrap_or(model_config), Err(_) => model_config, }; + let effort_support = provider.thinking_effort_support(); + let model_config = normalize_legacy_provider_thinking_effort(model_config, &effort_support); - let mut current_provider = self.provider.lock().await; - *current_provider = Some(provider); + { + let mut current_provider = self.provider.lock().await; + *current_provider = Some(Arc::clone(&provider)); + } + + // A freshly created provider that manages its own model starts on its + // own default, so the session's selection has to be pushed to it before + // the next config snapshot is built. Failures are not fatal here: the + // selection is re-applied at stream time. + if let Err(e) = provider.apply_model_selection(&model_config).await { + warn!("Failed to apply model selection to provider: {e}"); + } self.config .session_manager @@ -3530,20 +3563,51 @@ impl Agent { self.update_goose_mode(mode, session_id).await } - pub async fn update_thinking_effort( - &self, - session_id: &str, - effort: ThinkingEffort, - ) -> Result<()> { + /// Apply a thinking-effort selection. `effort` is the raw option value: a + /// provider that manages effort through a harness has its own vocabulary, + /// which is not always a `ThinkingEffort` member. + pub async fn update_thinking_effort(&self, session_id: &str, effort: &str) -> Result<()> { let current_provider = self.provider().await?; - let provider_name = current_provider.get_name().to_string(); - let model_config = self - .model_config_for_session(session_id) - .await? - .with_thinking_effort(effort); - - self.recreate_provider_for_session(session_id, &provider_name, model_config) + // Context rather than a formatted string: the caller distinguishes a + // value rejection from an operational failure by downcasting to + // `ProviderError`, which stringifying would destroy. + let provider_handled = current_provider + .set_thinking_effort(session_id, effort) .await + .context("Provider rejected thinking effort update")?; + + let model_config = self.model_config_for_session(session_id).await?; + + if provider_handled { + // The provider applied the value live; recreating it would discard + // the very session state we just configured. + let model_config = model_config.with_merged_request_params(HashMap::from([( + "thinking_effort".to_string(), + Value::String(effort.to_string()), + )])); + return self + .config + .session_manager + .clone() + .update(session_id) + .model_config(model_config) + .apply() + .await + .context("Failed to persist thinking effort to session"); + } + + let effort = effort.parse::().map_err(|_| { + anyhow::Error::new(ProviderError::InvalidValue(format!( + "Invalid thinking effort: {effort}" + ))) + })?; + let provider_name = current_provider.get_name().to_string(); + self.recreate_provider_for_session( + session_id, + &provider_name, + model_config.with_thinking_effort(effort), + ) + .await } /// Restore the provider from session data or fall back to global config @@ -4365,6 +4429,235 @@ mod tests { assert_eq!(conf.permission, crate::permission::Permission::AllowOnce); } + enum EffortOutcome { + Applied, + Unhandled, + Rejected, + } + + #[derive(Debug)] + struct EffortProvider { + applies_effort: bool, + rejects_effort: bool, + effort_calls: std::sync::Mutex>, + model_selections: std::sync::Mutex>, + } + + impl EffortProvider { + fn new(outcome: EffortOutcome) -> Self { + Self { + applies_effort: matches!(outcome, EffortOutcome::Applied), + rejects_effort: matches!(outcome, EffortOutcome::Rejected), + effort_calls: std::sync::Mutex::new(Vec::new()), + model_selections: std::sync::Mutex::new(Vec::new()), + } + } + + fn effort_calls(&self) -> Vec { + self.effort_calls.lock().unwrap().clone() + } + + fn model_selections(&self) -> Vec { + self.model_selections.lock().unwrap().clone() + } + } + + #[async_trait::async_trait] + impl crate::providers::base::Provider for EffortProvider { + fn get_name(&self) -> &str { + "test-effort" + } + fn thinking_effort_support(&self) -> ThinkingEffortSupport { + if self.applies_effort { + ThinkingEffortSupport::Options( + goose_providers::thinking::ThinkingEffortCapability { + option_id: "effort".to_string(), + values: vec![goose_providers::thinking::ThinkingEffortOption { + value: "default".to_string(), + label: "Default".to_string(), + }], + current: Some("default".to_string()), + }, + ) + } else { + ThinkingEffortSupport::Unspecified + } + } + async fn stream( + &self, + _: &goose_providers::model::ModelConfig, + _: &str, + _: &[crate::conversation::message::Message], + _: &[rmcp::model::Tool], + ) -> Result { + unimplemented!() + } + async fn set_thinking_effort( + &self, + _session_id: &str, + value: &str, + ) -> Result { + self.effort_calls.lock().unwrap().push(value.to_string()); + if self.rejects_effort { + return Err(ProviderError::RequestFailed("no such effort".to_string())); + } + Ok(self.applies_effort) + } + async fn apply_model_selection( + &self, + model_config: &goose_providers::model::ModelConfig, + ) -> Result<(), ProviderError> { + self.model_selections + .lock() + .unwrap() + .push(model_config.model_name.clone()); + Ok(()) + } + } + + async fn effort_test_agent( + outcome: EffortOutcome, + ) -> (Agent, String, Arc, TempDir) { + let (agent, session, data_dir) = tracing_test_agent_and_session().await; + let provider = Arc::new(EffortProvider::new(outcome)); + agent + .update_provider( + provider.clone(), + goose_providers::model::ModelConfig::new("mock-model"), + &session.id, + ) + .await + .unwrap(); + (agent, session.id, provider, data_dir) + } + + async fn persisted_thinking_effort(agent: &Agent, session_id: &str) -> Option { + agent + .model_config_for_session(session_id) + .await + .unwrap() + .request_param::("thinking_effort") + } + + #[tokio::test] + async fn update_provider_applies_the_model_selection() { + let (_agent, _session_id, provider, _data_dir) = + effort_test_agent(EffortOutcome::Applied).await; + + assert_eq!(provider.model_selections(), ["mock-model"]); + } + + #[tokio::test] + async fn update_provider_replaces_harness_only_effort_for_legacy_provider() { + let _guard = env_lock::lock_env([("GOOSE_THINKING_EFFORT", Some("high"))]); + let (agent, session, _data_dir) = tracing_test_agent_and_session().await; + let provider = Arc::new(EffortProvider::new(EffortOutcome::Unhandled)); + let model_config = + goose_providers::model::ModelConfig::new("mock-model").with_merged_request_params( + HashMap::from([("thinking_effort".to_string(), serde_json::json!("default"))]), + ); + + agent + .update_provider(provider, model_config, &session.id) + .await + .unwrap(); + + assert_eq!( + persisted_thinking_effort(&agent, &session.id) + .await + .as_deref(), + Some("high") + ); + } + + #[tokio::test] + async fn update_provider_preserves_harness_only_effort_for_managed_provider() { + let _guard = env_lock::lock_env([("GOOSE_THINKING_EFFORT", Some("high"))]); + let (agent, session, _data_dir) = tracing_test_agent_and_session().await; + let provider = Arc::new(EffortProvider::new(EffortOutcome::Applied)); + let model_config = + goose_providers::model::ModelConfig::new("mock-model").with_merged_request_params( + HashMap::from([("thinking_effort".to_string(), serde_json::json!("default"))]), + ); + + agent + .update_provider(provider, model_config, &session.id) + .await + .unwrap(); + + assert_eq!( + persisted_thinking_effort(&agent, &session.id) + .await + .as_deref(), + Some("default") + ); + } + + #[tokio::test] + async fn update_thinking_effort_persists_the_raw_value_when_the_provider_applies_it() { + let (agent, session_id, provider, _data_dir) = + effort_test_agent(EffortOutcome::Applied).await; + + // "xhigh" is a harness value, not a ThinkingEffort member spelling. + agent + .update_thinking_effort(&session_id, "xhigh") + .await + .unwrap(); + + assert_eq!(provider.effort_calls(), ["xhigh"]); + assert_eq!( + persisted_thinking_effort(&agent, &session_id) + .await + .as_deref(), + Some("xhigh") + ); + // The unregistered test provider was not respawned. + assert_eq!(agent.provider().await.unwrap().get_name(), "test-effort"); + } + + #[tokio::test] + async fn update_thinking_effort_rejects_an_unparseable_value_on_the_legacy_path() { + let (agent, session_id, provider, _data_dir) = + effort_test_agent(EffortOutcome::Unhandled).await; + + let err = agent + .update_thinking_effort(&session_id, "bogus") + .await + .unwrap_err(); + + assert!(matches!( + err.downcast_ref::(), + Some(ProviderError::InvalidValue(_)) + )); + assert!(err.to_string().contains("Invalid thinking effort")); + assert_eq!(provider.effort_calls(), ["bogus"]); + assert!(persisted_thinking_effort(&agent, &session_id) + .await + .is_none()); + } + + #[tokio::test] + async fn update_thinking_effort_surfaces_a_provider_rejection() { + let (agent, session_id, _provider, _data_dir) = + effort_test_agent(EffortOutcome::Rejected).await; + + let err = agent + .update_thinking_effort(&session_id, "high") + .await + .unwrap_err(); + + assert!(err.to_string().contains("Provider rejected")); + // The caller classifies the failure by variant, so the provider's typed + // error has to survive the trip up. + assert!(matches!( + err.downcast_ref::(), + Some(ProviderError::RequestFailed(_)) + )); + assert!(persisted_thinking_effort(&agent, &session_id) + .await + .is_none()); + } + const ALWAYS_BLOCK_SCRIPT: &str = r#"#!/bin/sh echo blocked >> "$PLUGIN_ROOT/hook.log" echo "always block" >&2 diff --git a/crates/goose/src/agents/mod.rs b/crates/goose/src/agents/mod.rs index 52947dda2..909b2899e 100644 --- a/crates/goose/src/agents/mod.rs +++ b/crates/goose/src/agents/mod.rs @@ -39,7 +39,7 @@ pub use subagent_task_config::TaskConfig; pub use tool_execution::ToolCallContext; pub use types::{FrontendTool, RetryConfig, SessionConfig, SuccessCheck}; -fn latest_provider_session_id<'a>( +pub(crate) fn latest_provider_session_id<'a>( messages: &'a [crate::conversation::message::Message], provider: &str, ) -> Option<&'a str> { diff --git a/crates/goose/tests/acp_bootstrap_effort_test.rs b/crates/goose/tests/acp_bootstrap_effort_test.rs new file mode 100644 index 000000000..cda8cbb07 --- /dev/null +++ b/crates/goose/tests/acp_bootstrap_effort_test.rs @@ -0,0 +1,329 @@ +use agent_client_protocol::schema::v1::{ + AgentCapabilities, ConfigOptionUpdate, InitializeRequest, InitializeResponse, + LoadSessionRequest, LoadSessionResponse, NewSessionRequest, NewSessionResponse, + SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption, SessionId, + SessionNotification, SessionUpdate, SetSessionConfigOptionRequest, + SetSessionConfigOptionResponse, UsageUpdate, +}; +use agent_client_protocol::schema::ProtocolVersion; +use agent_client_protocol::{on_receive_request, Agent as SacpAgent, ByteStreams}; +use goose::acp::{AcpProvider, AcpProviderConfig}; +use goose::config::GooseMode; +use goose::providers::base::Provider; +use goose_providers::thinking::ThinkingEffortSupport; +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; +use tokio::sync::Notify; +use tokio::time::{timeout, Duration}; +use tokio_util::compat::{TokioAsyncReadCompatExt, TokioAsyncWriteCompatExt}; + +fn effort_option(current: &str, values: &[&str]) -> SessionConfigOption { + SessionConfigOption::select( + "effort", + "Thinking", + current.to_string(), + values + .iter() + .map(|value| SessionConfigSelectOption::new(value.to_string(), *value)) + .collect::>(), + ) + .category(SessionConfigOptionCategory::ThoughtLevel) +} + +/// Agents that pin a model during session bootstrap rebuild their per-model +/// effort levels in that response, so it supersedes the `session/new` snapshot +/// the mirrored capability was first built from. +#[tokio::test] +async fn bootstrap_config_option_response_refreshes_the_effort_mirror() { + let (client_read, agent_write) = tokio::io::duplex(64 * 1024); + let (agent_read, client_write) = tokio::io::duplex(64 * 1024); + + let bootstrap_picks: Arc>> = Arc::new(Mutex::new(Vec::new())); + let recorded_picks = bootstrap_picks.clone(); + + let agent = tokio::spawn(async move { + SacpAgent + .builder() + .name("scripted-agent") + .on_receive_request( + async |_req: InitializeRequest, responder, _cx| { + responder.respond(InitializeResponse::new(ProtocolVersion::LATEST)) + }, + on_receive_request!(), + ) + .on_receive_request( + async |_req: NewSessionRequest, responder, _cx| { + responder.respond( + NewSessionResponse::new(SessionId::new("scripted-session")).config_options( + vec![effort_option("medium", &["low", "medium", "high"])], + ), + ) + }, + on_receive_request!(), + ) + .on_receive_request( + async |req: SetSessionConfigOptionRequest, responder, _cx| { + let value = req + .value + .as_value_id() + .expect("select option value") + .0 + .to_string(); + recorded_picks + .lock() + .unwrap() + .push((req.config_id.0.to_string(), value)); + responder.respond(SetSessionConfigOptionResponse::new(vec![effort_option( + "high", + &["minimal", "high"], + )])) + }, + on_receive_request!(), + ) + .connect_to(ByteStreams::new( + agent_write.compat_write(), + agent_read.compat(), + )) + .await + }); + + let config = AcpProviderConfig { + command: "unused".into(), + args: vec![], + env: vec![], + env_remove: vec![], + work_dir: std::env::temp_dir(), + mcp_servers: vec![], + session_mode_id: None, + session_config_options: vec![("model".to_string(), "gpt-5".to_string())], + model_config_option_id: Some("model".to_string()), + mode_mapping: HashMap::new(), + notification_callback: None, + }; + + let provider = AcpProvider::connect_with_transport( + "scripted-acp".to_string(), + GooseMode::default(), + config, + ByteStreams::new(client_write.compat_write(), client_read.compat()), + ) + .await + .expect("provider should connect to the scripted agent"); + + assert_eq!( + *bootstrap_picks.lock().unwrap(), + vec![("model".to_string(), "gpt-5".to_string())] + ); + + match provider.thinking_effort_support() { + ThinkingEffortSupport::Options(capability) => { + assert_eq!(capability.current.as_deref(), Some("high")); + assert_eq!( + capability + .values + .iter() + .map(|option| option.value.as_str()) + .collect::>(), + vec!["minimal", "high"] + ); + } + other => panic!("expected the agent's rebuilt effort options, got {other:?}"), + } + + drop(provider); + agent.abort(); +} + +#[tokio::test] +async fn new_session_preserves_pre_response_effort_update() { + let (client_read, agent_write) = tokio::io::duplex(64 * 1024); + let (agent_read, client_write) = tokio::io::duplex(64 * 1024); + + let agent = tokio::spawn(async move { + SacpAgent + .builder() + .name("scripted-agent") + .on_receive_request( + async |_req: InitializeRequest, responder, _cx| { + responder.respond(InitializeResponse::new(ProtocolVersion::LATEST)) + }, + on_receive_request!(), + ) + .on_receive_request( + async |_req: NewSessionRequest, responder, cx| { + cx.send_notification(SessionNotification::new( + SessionId::new("scripted-session"), + SessionUpdate::ConfigOptionUpdate(ConfigOptionUpdate::new(vec![ + effort_option("high", &["default", "high", "xhigh"]), + ])), + ))?; + responder.respond(NewSessionResponse::new(SessionId::new("scripted-session"))) + }, + on_receive_request!(), + ) + .connect_to(ByteStreams::new( + agent_write.compat_write(), + agent_read.compat(), + )) + .await + }); + + let provider = AcpProvider::connect_with_transport( + "scripted-acp".to_string(), + GooseMode::default(), + AcpProviderConfig { + command: "unused".into(), + args: vec![], + env: vec![], + env_remove: vec![], + work_dir: std::env::temp_dir(), + mcp_servers: vec![], + session_mode_id: None, + session_config_options: vec![], + model_config_option_id: None, + mode_mapping: HashMap::new(), + notification_callback: None, + }, + ByteStreams::new(client_write.compat_write(), client_read.compat()), + ) + .await + .expect("provider should preserve the pre-response update"); + + match provider.thinking_effort_support() { + ThinkingEffortSupport::Options(capability) => { + assert_eq!(capability.current.as_deref(), Some("high")); + assert_eq!( + capability + .values + .iter() + .map(|option| option.value.as_str()) + .collect::>(), + vec!["default", "high", "xhigh"] + ); + } + other => panic!("expected the pre-response effort options, got {other:?}"), + } + + drop(provider); + agent.abort(); +} + +#[tokio::test] +async fn loaded_session_refreshes_the_effort_mirror() { + let (client_read, agent_write) = tokio::io::duplex(64 * 1024); + let (agent_read, client_write) = tokio::io::duplex(64 * 1024); + + let emit_late_update = Arc::new(Notify::new()); + let agent_emit_late_update = emit_late_update.clone(); + let active_update_received = Arc::new(Notify::new()); + let callback_active_update_received = active_update_received.clone(); + + let agent = tokio::spawn(async move { + SacpAgent + .builder() + .name("scripted-agent") + .on_receive_request( + async |_req: InitializeRequest, responder, _cx| { + responder.respond( + InitializeResponse::new(ProtocolVersion::LATEST) + .agent_capabilities(AgentCapabilities::new().load_session(true)), + ) + }, + on_receive_request!(), + ) + .on_receive_request( + async |_req: NewSessionRequest, responder, _cx| { + responder.respond( + NewSessionResponse::new(SessionId::new("temporary-session")) + .config_options(vec![effort_option( + "medium", + &["low", "medium", "high"], + )]), + ) + }, + on_receive_request!(), + ) + .on_receive_request( + async move |req: LoadSessionRequest, responder, cx| { + assert_eq!(req.session_id.0.as_ref(), "saved-session"); + responder.respond(LoadSessionResponse::new().config_options(vec![ + effort_option("xhigh", &["default", "high", "xhigh"]), + ]))?; + agent_emit_late_update.notified().await; + cx.send_notification(SessionNotification::new( + SessionId::new("temporary-session"), + SessionUpdate::ConfigOptionUpdate(ConfigOptionUpdate::new(vec![ + effort_option("medium", &["low", "medium", "high"]), + ])), + ))?; + cx.send_notification(SessionNotification::new( + SessionId::new("saved-session"), + SessionUpdate::UsageUpdate(UsageUpdate::new(1, 100)), + )) + }, + on_receive_request!(), + ) + .connect_to(ByteStreams::new( + agent_write.compat_write(), + agent_read.compat(), + )) + .await + }); + + let config = AcpProviderConfig { + command: "unused".into(), + args: vec![], + env: vec![], + env_remove: vec![], + work_dir: std::env::temp_dir(), + mcp_servers: vec![], + session_mode_id: None, + session_config_options: vec![], + model_config_option_id: None, + mode_mapping: HashMap::new(), + notification_callback: Some(Arc::new(move |notification| { + if notification.session_id.0.as_ref() == "saved-session" + && matches!(notification.update, SessionUpdate::UsageUpdate(_)) + { + callback_active_update_received.notify_one(); + } + })), + }; + + let provider = AcpProvider::connect_with_transport( + "scripted-acp".to_string(), + GooseMode::default(), + config, + ByteStreams::new(client_write.compat_write(), client_read.compat()), + ) + .await + .expect("provider should connect to the scripted agent"); + + provider + .resume("saved-session") + .await + .expect("provider should load the saved session"); + + emit_late_update.notify_one(); + timeout(Duration::from_secs(1), active_update_received.notified()) + .await + .expect("active-session barrier notification should be received"); + + match provider.thinking_effort_support() { + ThinkingEffortSupport::Options(capability) => { + assert_eq!(capability.current.as_deref(), Some("xhigh")); + assert_eq!( + capability + .values + .iter() + .map(|option| option.value.as_str()) + .collect::>(), + vec!["default", "high", "xhigh"] + ); + } + other => panic!("expected the loaded session's effort options, got {other:?}"), + } + + drop(provider); + agent.abort(); +} diff --git a/crates/goose/tests/providers.rs b/crates/goose/tests/providers.rs index 36e080f35..4ed94c0b7 100644 --- a/crates/goose/tests/providers.rs +++ b/crates/goose/tests/providers.rs @@ -25,6 +25,7 @@ use goose::providers::xai::XAI_DEFAULT_MODEL; use goose::session::{SessionManager, SessionType}; use goose_providers::databricks::DATABRICKS_DEFAULT_MODEL; use goose_providers::errors::ProviderError; +use goose_providers::thinking::ThinkingEffortSupport; use goose_test_support::{ EnforceSessionId, ExpectedSessionId, IgnoreSessionId, McpFixture, FAKE_CODE, }; @@ -123,6 +124,7 @@ struct ProviderTestConfig { test_mode_update: bool, test_mcp_tools: bool, test_context_length_exceeded: bool, + test_thinking_effort: bool, expect_context_length_exceeded: bool, context_length_exceeded: usize, } @@ -147,6 +149,7 @@ impl ProviderTestConfig { test_mode_update: true, test_mcp_tools: true, test_context_length_exceeded: true, + test_thinking_effort: false, expect_context_length_exceeded: true, context_length_exceeded: 600_000, } @@ -177,6 +180,11 @@ impl ProviderTestConfig { self } + fn test_thinking_effort(mut self, v: bool) -> Self { + self.test_thinking_effort = v; + self + } + fn expect_context_length_exceeded(mut self, v: bool) -> Self { self.expect_context_length_exceeded = v; self @@ -497,6 +505,53 @@ impl ProviderFixture { Ok(()) } + async fn test_thinking_effort(&self) -> Result<()> { + let ThinkingEffortSupport::Options(capability) = self.provider.thinking_effort_support() + else { + anyhow::bail!("{} should mirror the agent's effort option", self.name); + }; + assert!(!capability.values.is_empty()); + let target = capability.values.last().unwrap().value.clone(); + println!( + "=== {}::thinking_effort ({} -> {}) === {:?}", + self.name, + capability.current.as_deref().unwrap_or("unset"), + target, + capability.values + ); + + assert!( + self.provider + .set_thinking_effort(&self.session_id, &target) + .await? + ); + + let effort_config = self + .model_config + .clone() + .with_merged_request_params(HashMap::from([( + "thinking_effort".to_string(), + serde_json::json!(target), + )])); + let message = Message::user().with_text("Just say hello!"); + let (response, _) = goose::session_context::with_session_id( + Some(self.session_id.clone()), + self.provider.complete( + &effort_config, + "You are a helpful assistant.", + &[message], + &[], + ), + ) + .await?; + + assert!(response + .content + .iter() + .any(|c| matches!(c, MessageContent::Text(_)))); + Ok(()) + } + async fn test_model_listing(&self) -> Result<()> { let models = self.provider.fetch_supported_models().await?; @@ -687,6 +742,12 @@ async fn test_provider(config: ProviderTestConfig) -> Result<()> { .test_context_length_exceeded_error() .await?; } + if config.test_thinking_effort { + run_test(GooseMode::Auto) + .await? + .test_thinking_effort() + .await?; + } if config.test_permissions { run_test(GooseMode::Approve) .await? @@ -895,6 +956,7 @@ async fn test_codex_provider() -> Result<()> { async fn test_claude_acp_provider() -> Result<()> { ProviderTestConfig::with_agentic_provider("claude-acp", ACP_CURRENT_MODEL, "claude-agent-acp") .model_switch_name("sonnet") + .test_thinking_effort(true) .run() .await } diff --git a/goose-self-test.yaml b/goose-self-test.yaml index 177e23b84..800328781 100644 --- a/goose-self-test.yaml +++ b/goose-self-test.yaml @@ -14,6 +14,7 @@ activities: - Test load tool for knowledge injection and discovery - Test delegate tool for task delegation (sync and async) - Test multi-turn thinking preservation for providers that reject replayed reasoning_content + - Validate ACP thinking-effort discovery, forwarding, and provider switching - Test error boundaries including nested delegation prevention - Generate comprehensive test report @@ -22,7 +23,7 @@ parameters: input_type: string requirement: optional default: "all" - description: "Which test phases to run: all, basic, extensions, delegation, reasoning, advanced" + description: "Which test phases to run: all, basic, extensions, delegation, reasoning, acp-effort, advanced" - key: test_depth input_type: string @@ -391,6 +392,24 @@ prompt: | Log results to: {{ workspace_dir }}/phase3c_reasoning.md {% endif %} + {% if test_phases == "all" or "acp-effort" in test_phases %} + ## 🧠 PHASE 3D: ACP Thinking-Effort Testing + + **Prerequisites**: Run this phase from a goose source checkout with the Rust toolchain + available. Skip this phase and record it as SKIPPED otherwise. + + ### ACP Effort Integration Test + 1. Run `cargo test -p goose effort`. + 2. Verify ACP agents' advertised effort menus and current values are mirrored after new, + loaded, and reconfigured sessions. + 3. Verify supported effort selections are forwarded to ACP agents and unsupported values + are rejected without changing the session. + 4. Verify ACP-only effort values are removed when switching to a legacy provider while + managed providers preserve values they support. + + Log results to: {{ workspace_dir }}/phase3d_acp_effort.md + {% endif %} + {% if test_phases == "all" or "advanced" in test_phases %} ## 🔬 PHASE 4: Advanced Testing