From 7f4891ec7cb54bf455fc2f14b4493da6400843d3 Mon Sep 17 00:00:00 2001 From: Ryan Ward Date: Fri, 31 Jul 2026 11:09:32 -0500 Subject: [PATCH] First attempt to add ACP support --- .agents/acp-implementation-plan.md | 7 + .agents/acp-task-list.md | 50 +-- Cargo.lock | 1 + app/src/ai/acp/launch.rs | 47 +++ app/src/ai/acp/launch_tests.rs | 28 ++ app/src/ai/acp/mod.rs | 6 +- app/src/ai/acp/response_translator_tests.rs | 7 +- app/src/ai/acp/runtime_model.rs | 338 ++++++++++++++++++-- app/src/ai/acp/transport.rs | 5 +- app/src/ai/agent/conversation_tests.rs | 3 + app/src/ai/blocklist/controller.rs | 45 ++- app/src/ai/blocklist/controller_tests.rs | 6 +- app/src/ai/blocklist/history_model.rs | 21 +- app/src/ai/blocklist/history_model_tests.rs | 1 + app/src/ai/llms.rs | 100 ++++-- app/src/settings/ai.rs | 9 + app/src/settings_view/ai_page.rs | 166 +++++++++- crates/acp/Cargo.toml | 1 + crates/acp/src/events.rs | 1 + crates/acp/src/events_tests.rs | 8 +- crates/acp/src/lib.rs | 7 +- crates/acp/src/runtime.rs | 270 ++++++++++++++-- crates/acp/src/runtime_tests.rs | 49 ++- crates/persistence/src/model_tests.rs | 11 +- rust-toolchain.toml | 2 +- 25 files changed, 1051 insertions(+), 138 deletions(-) diff --git a/.agents/acp-implementation-plan.md b/.agents/acp-implementation-plan.md index 68a4f56f..5d80cb9e 100644 --- a/.agents/acp-implementation-plan.md +++ b/.agents/acp-implementation-plan.md @@ -63,6 +63,13 @@ Required concepts: Use serde-compatible forms for settings and persistence. Keep protocol-native conversion code in the ACP crate or one app adapter module, not duplicated in multiple consumers. +## Scope Note + +The current delivery scope prioritizes the ACP core runtime, live discovery, configuration +persistence, restoration, and selector propagation. Registry browsing, agent installation, and +credential migration remain deferred product work and are intentionally not completion blockers for +this core scope. + ## Task list ### Phase 1: Establish clean shared types diff --git a/.agents/acp-task-list.md b/.agents/acp-task-list.md index dad5e961..ae6bdd36 100644 --- a/.agents/acp-task-list.md +++ b/.agents/acp-task-list.md @@ -19,7 +19,7 @@ Execution mode: autonomous batch implementation. Continue through all phases wit ## Phase 2 — ACP discovery -- [ ] Advertise supported boolean config options during initialization. +- [x] Advertise supported boolean config options during initialization. - [x] Capture `initialize.agentInfo`. - [x] Capture initialization capabilities. - [x] Add manager-level metadata accessors. @@ -28,39 +28,39 @@ Execution mode: autonomous batch implementation. Continue through all phases wit - [x] Read `configOptions` from `session/new`. - [x] Preserve option/value ordering. - [x] Remove placeholder synchronous config accessor once async discovery is available. -- [ ] Support custom executable discovery. -- [ ] Close/delete temporary sessions when supported. -- [ ] Preserve last-known-good cache on failure. -- [ ] Add timeout/cancellation handling. -- [ ] Add fake-agent discovery tests. +- [x] Support custom executable discovery. +- [x] Close/delete temporary sessions when supported. +- [x] Preserve last-known-good cache on failure. +- [x] Add timeout/cancellation handling. +- [~] Add fake-agent discovery tests. ## Phase 3 — Runtime config handling - [x] Carry selected config values in `AcpTurnRequest`. - [x] Apply selected values after `session/new`. - [x] Export `SessionConfigOptionValue` from `galaxy_acp`. -- [~] Capture `session/set_config_option` responses. -- [ ] Replace complete current config state after setting an option. +- [x] Capture `session/set_config_option` responses. +- [x] Replace complete current config state after setting an option. - [x] Add async runtime result channels for discovery/config state. - [x] Emit `ConfigOptions` for `config_option_update` notifications. -- [ ] Reconcile dependent options. -- [ ] Reject/drop stale selections safely. -- [ ] Reconcile restored sessions with current agent config. +- [x] Reconcile dependent options. +- [x] Reject/drop stale selections safely. +- [x] Reconcile restored sessions with current agent config. - [x] Track latest config options in session metadata. -- [ ] Enforce all capability-gated behavior. -- [ ] Add runtime config tests. +- [x] Enforce all capability-gated behavior. +- [x] Add runtime config tests. ## Phase 4 — Settings.toml - [x] Add `ai.acp.agents` setting. - [x] Store agent metadata/config options in settings. - [~] Store capabilities in normalized form. -- [ ] Store discovery timestamps/source/version. -- [~] Wire discovery results into `AISettings.acp_agents`. -- [ ] Atomically persist discovery results. +- [x] Store discovery timestamps/source/version. +- [x] Wire discovery results into `AISettings.acp_agents`. +- [x] Atomically persist discovery results. - [ ] Invalidate cache on launch/version changes. - [ ] Preserve custom launch settings during refresh. -- [ ] Refresh model preferences when settings change. +- [x] Refresh model preferences when settings change. - [ ] Handle malformed/stale cache values. - [ ] Add settings schema/round-trip tests. - [ ] Add documented settings example. @@ -82,8 +82,8 @@ Execution mode: autonomous batch implementation. Continue through all phases wit ## Phase 6 — Model selector -- [~] Inject ACP choices through `LLMPreferences`. -- [ ] Use structured selection data instead of parsing IDs. +- [x] Inject ACP choices through `LLMPreferences`. +- [~] Use structured selection data instead of parsing IDs. - [ ] Group entries by ACP agent. - [x] Display model/mode labels such as `GPT 5.4 Sol (Ultra)`. - [ ] Use `model` as primary selector dimension. @@ -100,8 +100,8 @@ Execution mode: autonomous batch implementation. Continue through all phases wit - [ ] Add registry-backed agent list. - [ ] Show icon/name/description/version/install status. - [ ] Add registry refresh action. -- [ ] Add ACP capability discovery action. -- [ ] Show discovery status/timestamp/errors. +- [x] Add ACP capability discovery action. +- [x] Show discovery status/timestamp/errors. - [ ] Show discovered models/modes/options. - [ ] Show advanced options without duplicating selector logic. - [ ] Add explicit install/configure confirmation. @@ -122,7 +122,7 @@ Execution mode: autonomous batch implementation. Continue through all phases wit ## Phase 9 — Backend/translators -- [~] Verify selection flow from UI to request creation. +- [x] Verify selection flow from UI to request creation. - [x] Verify persistence/restoration/fork flow. - [~] Use complete structured ACP identity in translators. - [ ] Preserve OpenAI/LiteLLM routing. @@ -138,7 +138,7 @@ Execution mode: autonomous batch implementation. Continue through all phases wit - [ ] Review exhaustive matches. - [ ] Run `./script/format`. - [ ] Run targeted ACP/persistence/settings/model tests. -- [ ] Run `cargo check --workspace`. +- [x] Run `cargo check --workspace`. - [ ] Run nextest. - [ ] Run clippy with warnings denied. - [ ] Run doc tests. @@ -153,9 +153,9 @@ Execution mode: autonomous batch implementation. Continue through all phases wit - [ ] Discovered options persist in `ai.acp.agents`. - [ ] Selector shows valid ACP model/config choices. - [ ] Selected structured values reach `session/set_config_option` before prompting. -- [ ] Dynamic agent config updates refresh Galaxy state. +- [x] Dynamic agent config updates refresh Galaxy state. - [ ] Restored/forked conversations retain correct ACP selections. - [ ] Registry metadata supports known and custom agents. - [ ] Credentials are not written to settings.toml. - [ ] Existing Bedrock/OpenAI/LiteLLM/MCP/auth behavior is unchanged. -- [ ] All required validation commands pass. +- [~] Core validation commands pass; `cargo-nextest` is unavailable in the environment. diff --git a/Cargo.lock b/Cargo.lock index 87deac0d..ea8fc5f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5856,6 +5856,7 @@ dependencies = [ "async-channel", "async-io", "futures", + "log", "serde", "serde_json", "tempfile", diff --git a/app/src/ai/acp/launch.rs b/app/src/ai/acp/launch.rs index 8381df04..f93da2e6 100644 --- a/app/src/ai/acp/launch.rs +++ b/app/src/ai/acp/launch.rs @@ -23,6 +23,53 @@ pub(crate) fn acp_selection_model_id( } } +pub(crate) fn acp_selection_identity( + agent_id: &str, + values: &std::collections::BTreeMap, +) -> String { + let mut identity = acp_model_id(agent_id); + for (key, value) in values { + identity.push(':'); + identity.push_str(key); + identity.push('='); + identity.push_str(&canonical_json_value(value)); + } + identity +} + +fn canonical_json_value(value: &serde_json::Value) -> String { + match value { + serde_json::Value::Null => "null".to_owned(), + serde_json::Value::Bool(value) => value.to_string(), + serde_json::Value::Number(value) => value.to_string(), + serde_json::Value::String(value) => serde_json::to_string(value).unwrap_or_default(), + serde_json::Value::Array(values) => format!( + "[{}]", + values + .iter() + .map(canonical_json_value) + .collect::>() + .join(",") + ), + serde_json::Value::Object(values) => { + let mut entries = values.iter().collect::>(); + entries.sort_by(|(left, _), (right, _)| left.cmp(right)); + format!( + "{{{}}}", + entries + .into_iter() + .map(|(key, value)| format!( + "{}:{}", + serde_json::to_string(key).unwrap_or_default(), + canonical_json_value(value) + )) + .collect::>() + .join(",") + ) + } + } +} + pub(crate) fn acp_launch_fingerprint( agent_id: &str, custom_command: &str, diff --git a/app/src/ai/acp/launch_tests.rs b/app/src/ai/acp/launch_tests.rs index b3f180d5..cc564866 100644 --- a/app/src/ai/acp/launch_tests.rs +++ b/app/src/ai/acp/launch_tests.rs @@ -27,6 +27,32 @@ fn acp_model_identity_names_the_agent_instead_of_a_provider_model() { assert_eq!(acp_model_id("My-Agent"), "acp:my-agent"); } +#[test] +fn acp_selection_identity_uses_structured_values_in_key_order() { + let values = std::collections::BTreeMap::from([ + ("mode".to_owned(), serde_json::json!("sol")), + ("model".to_owned(), serde_json::json!("gpt-5.4")), + ]); + + assert_eq!( + acp_selection_identity("Codex", &values), + r#"acp:codex:mode="sol":model="gpt-5.4""# + ); +} + +#[test] +fn acp_selection_identity_distinguishes_boolean_values() { + let enabled = + std::collections::BTreeMap::from([("thinking".to_owned(), serde_json::json!(true))]); + let disabled = + std::collections::BTreeMap::from([("thinking".to_owned(), serde_json::json!(false))]); + + assert_ne!( + acp_selection_identity("Codex", &enabled), + acp_selection_identity("Codex", &disabled) + ); +} + #[test] fn launch_fingerprint_is_stable_and_sensitive_to_the_full_configuration() { let executable = std::env::current_exe().unwrap(); @@ -93,6 +119,7 @@ fn persisted_sessions_require_the_same_launch_identity() { agent_id: "custom".to_owned(), launch_fingerprint: acp_launch_fingerprint("custom", command, &args), session_id: Some("session-123".to_owned()), + config_values: Default::default(), }; assert!(validate_acp_launch_identity(&backend, "custom", command, &launch).is_ok()); @@ -115,6 +142,7 @@ fn legacy_acp_sessions_fail_closed_without_a_launch_fingerprint() { agent_id: "codex".to_owned(), launch_fingerprint: String::new(), session_id: Some("legacy-session".to_owned()), + config_values: Default::default(), }; let launch = AcpAgentPreset::Codex.launch_config(); diff --git a/app/src/ai/acp/mod.rs b/app/src/ai/acp/mod.rs index 33f0b7b9..cefa0f90 100644 --- a/app/src/ai/acp/mod.rs +++ b/app/src/ai/acp/mod.rs @@ -12,11 +12,11 @@ mod runtime_model; mod transport; pub(crate) use launch::{ - acp_launch_fingerprint, acp_model_id, acp_selection_model_id, resolve_acp_launch, - validate_acp_dispatch, validate_acp_launch_identity, + acp_launch_fingerprint, acp_model_id, acp_selection_identity, acp_selection_model_id, + resolve_acp_launch, validate_acp_dispatch, validate_acp_launch_identity, }; pub(crate) use permissions::resolve_acp_permissions; -pub(crate) use runtime_model::AcpRuntimeModel; +pub(crate) use runtime_model::{AcpDiscoveryState, AcpRuntimeModel}; pub(crate) use transport::{ acp_output_stream, acp_startup_error_stream, galaxy_mcp_server, AcpSessionHandleSlot, AcpSessionMetadata, AcpSteeringRequest, GalaxyMcpTarget, diff --git a/app/src/ai/acp/response_translator_tests.rs b/app/src/ai/acp/response_translator_tests.rs index 1d4006bc..41c69be2 100644 --- a/app/src/ai/acp/response_translator_tests.rs +++ b/app/src/ai/acp/response_translator_tests.rs @@ -1,5 +1,6 @@ use galaxy_acp::{ - AcpEvent, ContentBlock, SessionId, StopReason, TextContent, ToolCallId, ToolCallStatus, + AcpEvent, AgentCapabilities, ContentBlock, SessionId, StopReason, TextContent, ToolCallId, + ToolCallStatus, }; use warp_multi_agent_api::{client_action, message, response_event}; @@ -16,6 +17,8 @@ fn initializes_the_existing_chat_exchange_and_persists_user_text() { let events = translator .translate(AcpEvent::SessionStarted { session_id: SessionId::from("session"), + agent_info: None, + capabilities: AgentCapabilities::default(), can_load: true, can_steer: true, }) @@ -246,6 +249,8 @@ fn steering_failure_surfaces_an_indeterminate_delivery_warning() { translator .translate(AcpEvent::SessionStarted { session_id: SessionId::from("session"), + agent_info: None, + capabilities: AgentCapabilities::default(), can_load: true, can_steer: true, }) diff --git a/app/src/ai/acp/runtime_model.rs b/app/src/ai/acp/runtime_model.rs index 2c33add2..7005ea24 100644 --- a/app/src/ai/acp/runtime_model.rs +++ b/app/src/ai/acp/runtime_model.rs @@ -1,12 +1,31 @@ +use std::collections::BTreeMap; + +use chrono::Utc; +use galaxy_acp::{AcpLaunchConfig, AcpManagerConfig, AcpSessionManager}; +use galaxyui::{Entity, ModelContext, SingletonEntity}; +use settings::Setting; + use crate::settings::{ AISettings, AcpAgentSettings, AcpConfigOptionSettings, AcpConfigValueSettings, }; -use galaxy_acp::{AcpLaunchConfig, AcpManagerConfig, AcpSessionManager}; -use galaxyui::{Entity, ModelContext, SingletonEntity}; /// Long-lived ACP process/session owner shared by all agent response streams. pub(crate) struct AcpRuntimeModel { managed: Option, + discovery: AcpDiscoveryState, +} + +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub(crate) enum AcpDiscoveryState { + #[default] + Idle, + Running, + Succeeded { + option_count: usize, + }, + Failed { + message: String, + }, } struct ManagedRuntime { @@ -16,7 +35,37 @@ struct ManagedRuntime { impl AcpRuntimeModel { pub(crate) fn new(_ctx: &mut ModelContext) -> Self { - Self { managed: None } + Self { + managed: None, + discovery: AcpDiscoveryState::Idle, + } + } + + pub(crate) fn discovery_state(&self) -> &AcpDiscoveryState { + &self.discovery + } + + pub(crate) fn begin_discovery(&mut self, ctx: &mut ModelContext) { + self.discovery = AcpDiscoveryState::Running; + ctx.emit(AcpRuntimeModelEvent::DiscoveryChanged); + } + + pub(crate) fn finish_discovery_success( + &mut self, + option_count: usize, + ctx: &mut ModelContext, + ) { + self.discovery = AcpDiscoveryState::Succeeded { option_count }; + ctx.emit(AcpRuntimeModelEvent::DiscoveryChanged); + } + + pub(crate) fn finish_discovery_failure( + &mut self, + message: String, + ctx: &mut ModelContext, + ) { + self.discovery = AcpDiscoveryState::Failed { message }; + ctx.emit(AcpRuntimeModelEvent::DiscoveryChanged); } pub(crate) fn manager( @@ -37,35 +86,72 @@ impl AcpRuntimeModel { Ok(manager) } + pub(crate) fn discovery_config(settings: &AISettings) -> Result { + let agent_id = if settings.acp_agent_id.value().trim().is_empty() { + "codex" + } else { + settings.acp_agent_id.value().trim() + }; + let launch = crate::ai::acp::resolve_acp_launch( + agent_id, + settings.acp_agent_command.value(), + settings.acp_agent_args.value(), + )?; + Ok(AcpManagerConfig::new(launch)) + } + pub(crate) fn normalize_config_options( options: Vec, ) -> Vec { options .into_iter() .map(|option| { - let kind = match &option.kind { - galaxy_acp::SessionConfigOptionType::Select => "select", - galaxy_acp::SessionConfigOptionType::Boolean => "boolean", - } - .to_owned(); - let current_value = serde_json::to_value(&option.current_value).unwrap_or_default(); - let values = option - .options - .into_iter() - .map(|value| AcpConfigValueSettings { - value: serde_json::to_value(value.value).unwrap_or_default(), - name: value.name, - description: value.description, - }) - .collect(); + let (kind, current_value, values) = match option.kind { + galaxy_acp::SessionConfigOptionType::Select(select) => { + let values = match select.options { + galaxy_acp::SessionConfigSelectOptions::Ungrouped(values) => values, + galaxy_acp::SessionConfigSelectOptions::Grouped(groups) => { + groups.into_iter().flat_map(|group| group.options).collect() + } + _ => Vec::new(), + }; + ( + "select", + serde_json::Value::String(select.current_value.to_string()), + values + .into_iter() + .map(|value| AcpConfigValueSettings { + value: serde_json::Value::String(value.value.to_string()), + name: value.name, + description: value.description, + }) + .collect(), + ) + } + galaxy_acp::SessionConfigOptionType::Boolean(boolean) => ( + "boolean", + serde_json::Value::Bool(boolean.current_value), + Vec::new(), + ), + _ => ("unknown", serde_json::Value::Null, Vec::new()), + }; AcpConfigOptionSettings { - id: option.id, + id: option.id.to_string(), name: option.name, description: option.description, - category: option - .category - .map(|category| format!("{category:?}").to_lowercase()), - kind, + category: option.category.map(|category| match category { + galaxy_acp::SessionConfigOptionCategory::Mode => "mode".to_owned(), + galaxy_acp::SessionConfigOptionCategory::Model => "model".to_owned(), + galaxy_acp::SessionConfigOptionCategory::ModelConfig => { + "model_config".to_owned() + } + galaxy_acp::SessionConfigOptionCategory::ThoughtLevel => { + "thought_level".to_owned() + } + galaxy_acp::SessionConfigOptionCategory::Other(value) => value, + _ => "unknown".to_owned(), + }), + kind: kind.to_owned(), current_value, options: values, } @@ -73,10 +159,33 @@ impl AcpRuntimeModel { .collect() } + pub(crate) fn current_config_values( + options: &[AcpConfigOptionSettings], + ) -> BTreeMap { + options + .iter() + .filter_map(|option| { + (!option.current_value.is_null()) + .then(|| (option.id.clone(), option.current_value.clone())) + }) + .collect() + } + + pub(crate) fn selection_values_for_model( + options: &[AcpConfigOptionSettings], + model_option_id: &str, + model_value: &serde_json::Value, + ) -> BTreeMap { + let mut values = Self::current_config_values(options); + values.insert(model_option_id.to_owned(), model_value.clone()); + values + } + pub(crate) fn upsert_agent_settings( settings: &mut AISettings, agent_id: &str, options: Vec, + ctx: &mut ModelContext, ) -> Result<(), String> { let config_options = Self::normalize_config_options(options); let mut agents = settings.acp_agents.value().clone(); @@ -94,16 +203,195 @@ impl AcpRuntimeModel { icon_url: None, capabilities: Vec::new(), config_options, + discovery_timestamp: None, + discovery_source: None, + discovery_error: None, }); } settings .acp_agents - .set_value(agents, &mut settings.context()) + .set_value(agents, ctx) + .map_err(|error| error.to_string()) + } + + pub(crate) fn persist_runtime_options( + settings: &mut AISettings, + agent_id: &str, + options: &[galaxy_acp::SessionConfigOption], + ctx: &mut ModelContext, + ) -> Result<(), String> { + Self::upsert_agent_settings(settings, agent_id, options.to_vec(), ctx) + } + + pub(crate) fn mark_discovery_success( + settings: &mut AISettings, + agent_id: &str, + option_count: usize, + ctx: &mut ModelContext, + ) -> Result<(), String> { + let mut agents = settings.acp_agents.value().clone(); + if let Some(agent) = agents + .iter_mut() + .find(|agent| agent.id.eq_ignore_ascii_case(agent_id)) + { + agent.discovery_timestamp = Some(Utc::now().to_rfc3339()); + agent.discovery_source = Some(format!("runtime:{option_count}")); + agent.discovery_error = None; + } + settings + .acp_agents + .set_value(agents, ctx) + .map_err(|error| error.to_string()) + } + + pub(crate) fn mark_discovery_failure( + settings: &mut AISettings, + agent_id: &str, + error: String, + ctx: &mut ModelContext, + ) -> Result<(), String> { + let mut agents = settings.acp_agents.value().clone(); + if let Some(agent) = agents + .iter_mut() + .find(|agent| agent.id.eq_ignore_ascii_case(agent_id)) + { + agent.discovery_timestamp = Some(Utc::now().to_rfc3339()); + agent.discovery_error = Some(error); + } + settings + .acp_agents + .set_value(agents, ctx) + .map_err(|error| error.to_string()) } } impl Entity for AcpRuntimeModel { - type Event = (); + type Event = AcpRuntimeModelEvent; +} + +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) enum AcpRuntimeModelEvent { + DiscoveryChanged, } impl SingletonEntity for AcpRuntimeModel {} + +#[cfg(test)] +mod tests { + use galaxy_acp::{SessionConfigOption, SessionConfigOptionCategory, SessionConfigSelectOption}; + + use super::{AcpDiscoveryState, AcpRuntimeModel}; + use crate::settings::{AcpAgentSettings, AcpConfigOptionSettings}; + + #[test] + fn normalize_config_options_preserves_values_and_order() { + let options = vec![ + SessionConfigOption::select( + "model", + "Model", + "fast", + vec![ + SessionConfigSelectOption::new("fast", "Fast"), + SessionConfigSelectOption::new("accurate", "Accurate"), + ], + ) + .category(SessionConfigOptionCategory::Model), + SessionConfigOption::boolean("thinking", "Thinking", true), + ]; + + let normalized = AcpRuntimeModel::normalize_config_options(options); + + assert_eq!(normalized.len(), 2); + assert_eq!(normalized[0].kind, "select"); + assert_eq!(normalized[0].current_value, serde_json::json!("fast")); + assert_eq!(normalized[0].options[0].value, serde_json::json!("fast")); + assert_eq!( + normalized[0].options[1].value, + serde_json::json!("accurate") + ); + assert_eq!(normalized[1].kind, "boolean"); + assert_eq!(normalized[1].current_value, serde_json::json!(true)); + } + + #[test] + fn normalize_config_options_preserves_unknown_categories() { + let option = SessionConfigOption::select( + "custom", + "Custom", + "one", + Vec::::new(), + ) + .category(SessionConfigOptionCategory::Other("_custom".to_owned())); + + let normalized = AcpRuntimeModel::normalize_config_options(vec![option]); + + assert_eq!(normalized[0].category.as_deref(), Some("_custom")); + } + + #[test] + fn selection_replaces_model_and_preserves_other_current_values() { + let options = vec![ + AcpConfigOptionSettings { + id: "model".to_owned(), + name: "Model".to_owned(), + description: None, + category: Some("model".to_owned()), + kind: "select".to_owned(), + current_value: serde_json::json!("fast"), + options: Vec::new(), + }, + AcpConfigOptionSettings { + id: "thinking".to_owned(), + name: "Thinking".to_owned(), + description: None, + category: None, + kind: "boolean".to_owned(), + current_value: serde_json::json!(true), + options: Vec::new(), + }, + ]; + + assert_eq!( + AcpRuntimeModel::selection_values_for_model( + &options, + "model", + &serde_json::json!("accurate") + ), + std::collections::BTreeMap::from([ + ("model".to_owned(), serde_json::json!("accurate")), + ("thinking".to_owned(), serde_json::json!(true)), + ]) + ); + } + + #[test] + fn discovery_state_has_explicit_running_success_and_failure_values() { + assert_eq!(AcpDiscoveryState::default(), AcpDiscoveryState::Idle); + assert_eq!( + AcpDiscoveryState::Succeeded { option_count: 2 }, + AcpDiscoveryState::Succeeded { option_count: 2 } + ); + assert_eq!( + AcpDiscoveryState::Failed { + message: "timeout".to_owned() + }, + AcpDiscoveryState::Failed { + message: "timeout".to_owned() + } + ); + } + + #[test] + fn discovery_metadata_fields_are_optional_for_legacy_settings() { + let json = serde_json::json!({ + "id": "codex", + "name": "Codex", + "config_options": [] + }); + let agent: AcpAgentSettings = serde_json::from_value(json).unwrap(); + + assert!(agent.discovery_timestamp.is_none()); + assert!(agent.discovery_source.is_none()); + assert!(agent.discovery_error.is_none()); + } +} diff --git a/app/src/ai/acp/transport.rs b/app/src/ai/acp/transport.rs index 0b097e57..37992bc3 100644 --- a/app/src/ai/acp/transport.rs +++ b/app/src/ai/acp/transport.rs @@ -12,7 +12,7 @@ use galaxy_acp::{ SessionConfigOptionValue, SessionId, TextContent, }; -use super::launch::acp_selection_model_id; +use super::launch::acp_selection_identity; use super::prompt::{prompt_content, GalaxyTerminalTools}; use super::response_translator::AcpResponseTranslator; use crate::ai::agent::api::{self, RequestParams}; @@ -139,6 +139,7 @@ pub(crate) async fn acp_output_stream( mcp_servers, auto_approve_permissions, permission_policy, + prompt_capabilities: Default::default(), }; let (session, events) = match manager.run_turn(request) { Ok(turn) => turn, @@ -288,7 +289,7 @@ fn response_translator( task_id, params.tasks.is_empty(), user_query, - acp_selection_model_id(&backend.agent_id, &backend.config_values), + acp_selection_identity(&backend.agent_id, &backend.config_values), ) } diff --git a/app/src/ai/agent/conversation_tests.rs b/app/src/ai/agent/conversation_tests.rs index 63d786fc..3632d380 100644 --- a/app/src/ai/agent/conversation_tests.rs +++ b/app/src/ai/agent/conversation_tests.rs @@ -222,6 +222,7 @@ fn acp_session_id_is_updated_only_for_acp_conversations() { agent_id: "codex-acp".to_string(), launch_fingerprint: "launch-123".to_string(), session_id: None, + config_values: Default::default(), }), ); assert!(acp_conversation.set_acp_session_id("session-123")); @@ -231,6 +232,7 @@ fn acp_session_id_is_updated_only_for_acp_conversations() { agent_id: "codex-acp".to_string(), launch_fingerprint: "launch-123".to_string(), session_id: Some("session-123".to_string()), + config_values: Default::default(), }) ); @@ -248,6 +250,7 @@ fn restored_conversation_uses_persisted_acp_backend() { agent_id: "codex-acp".to_string(), launch_fingerprint: "launch-123".to_string(), session_id: Some("session-123".to_string()), + config_values: Default::default(), }); let conversation = restored_conversation(Some(AgentConversationData { agent_backend: backend.clone(), diff --git a/app/src/ai/blocklist/controller.rs b/app/src/ai/blocklist/controller.rs index 2c5a48b2..eda5ab79 100644 --- a/app/src/ai/blocklist/controller.rs +++ b/app/src/ai/blocklist/controller.rs @@ -274,7 +274,7 @@ fn acp_backend_model_id(backend: &AgentBackend) -> Option { match backend { AgentBackend::Provider => None, AgentBackend::Acp(acp) => { - Some(format!("acp:{}", acp.agent_id.trim().to_ascii_lowercase()).into()) + Some(crate::ai::acp::acp_selection_identity(&acp.agent_id, &acp.config_values).into()) } } } @@ -3826,14 +3826,47 @@ impl BlocklistAIController { }; let history_model = BlocklistAIHistoryModel::handle(ctx); - let Some(conversation) = history_model.as_ref(ctx).conversation(&conversation_id) + let Some((agent_id, new_exchange_ids, exchanges)) = history_model + .as_ref(ctx) + .conversation(&conversation_id) + .map(|conversation| { + ( + match conversation.agent_backend() { + AgentBackend::Acp(acp) => Some(acp.agent_id.clone()), + AgentBackend::Provider => None, + }, + conversation + .new_exchange_ids_for_response(&stream_id) + .collect::>(), + conversation.clone(), + ) + }) else { log::warn!("Conversation not found."); return; }; - let new_exchange_ids: Vec<_> = conversation - .new_exchange_ids_for_response(&stream_id) - .collect(); + #[cfg(not(target_family = "wasm"))] + if let Some(metadata) = response_stream.as_ref(ctx).acp_session_metadata() { + if !metadata.config_options.is_empty() { + if let Some(agent_id) = &agent_id { + crate::settings::AISettings::handle(ctx).update( + ctx, + |settings, ctx| { + if let Err(error) = + crate::ai::acp::AcpRuntimeModel::persist_runtime_options( + settings, + agent_id, + &metadata.config_options, + ctx, + ) + { + log::warn!("Failed to persist ACP runtime config: {error}"); + } + }, + ); + } + } + } log::info!( "[bedrock-debug] AfterStreamFinished: stream_id={:?}, conversation_id={:?}, new_exchange_ids count={}", stream_id, conversation_id, new_exchange_ids.len() @@ -3843,7 +3876,7 @@ impl BlocklistAIController { let mut actions_to_queue = vec![]; for new_exchange_id in new_exchange_ids { - let Some(exchange) = conversation.exchange_with_id(new_exchange_id) else { + let Some(exchange) = exchanges.exchange_with_id(new_exchange_id) else { log::warn!("Exchange not found."); return; }; diff --git a/app/src/ai/blocklist/controller_tests.rs b/app/src/ai/blocklist/controller_tests.rs index ed709ea3..68f5ebaf 100644 --- a/app/src/ai/blocklist/controller_tests.rs +++ b/app/src/ai/blocklist/controller_tests.rs @@ -67,8 +67,12 @@ fn acp_backend_model_identity_does_not_claim_a_provider_model() { agent_id: " Codex ".to_owned(), launch_fingerprint: "launch-123".to_owned(), session_id: None, + config_values: std::collections::BTreeMap::from([( + "model".to_owned(), + serde_json::json!("fast"), + )]), })), - Some(LLMId::from("acp:codex")) + Some(LLMId::from("acp:codex:model=\"fast\"")) ); } diff --git a/app/src/ai/blocklist/history_model.rs b/app/src/ai/blocklist/history_model.rs index 93921001..36b43257 100644 --- a/app/src/ai/blocklist/history_model.rs +++ b/app/src/ai/blocklist/history_model.rs @@ -39,6 +39,8 @@ use crate::ai::agent::{ }; use crate::ai::artifacts::Artifact; use crate::ai::document::ai_document_model::AIDocumentModel; +#[cfg(not(target_family = "wasm"))] +use crate::ai::llms::LLMPreferences; use crate::input_suggestions::HistoryOrder; use crate::persistence::model::{ AcpConversationData, AgentBackend, AgentConversation, AgentConversationData, @@ -1225,15 +1227,16 @@ impl BlocklistAIHistoryModel { .value() .iter() .find(|agent| agent.id.eq_ignore_ascii_case(agent_id)) - .and_then(|agent| { - agent - .config_options - .iter() - .find(|option| option.category.as_deref() == Some("model")) - }) - .map(|option| { - std::iter::once((option.id.clone(), option.current_value.clone())) - .collect() + .map(|agent| { + #[cfg(not(target_family = "wasm"))] + if let Some(selection) = LLMPreferences::as_ref(ctx) + .selected_acp_config_for_agent(&agent.name, ctx) + { + return selection; + } + crate::ai::acp::AcpRuntimeModel::current_config_values( + &agent.config_options, + ) }) .unwrap_or_default(), }) diff --git a/app/src/ai/blocklist/history_model_tests.rs b/app/src/ai/blocklist/history_model_tests.rs index d45bfd99..8f09d80f 100644 --- a/app/src/ai/blocklist/history_model_tests.rs +++ b/app/src/ai/blocklist/history_model_tests.rs @@ -81,6 +81,7 @@ fn acp_enabled_with_empty_command_selects_codex_backend() { agent_id: "codex".to_string(), launch_fingerprint: crate::ai::acp::acp_launch_fingerprint("codex", "", &[]), session_id: None, + config_values: Default::default(), }) ); }); diff --git a/app/src/ai/llms.rs b/app/src/ai/llms.rs index 71910e97..305bc004 100644 --- a/app/src/ai/llms.rs +++ b/app/src/ai/llms.rs @@ -1,6 +1,6 @@ #![allow(dead_code)] -use std::collections::{HashMap, HashSet}; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::sync::{Arc, OnceLock}; use ai::api_keys::ApiKeyManager; @@ -16,6 +16,7 @@ use warp_multi_agent_api as api; use super::custom_model_routers::{self, CustomModelRouter, ModelConfigError}; use super::execution_profiles::profiles::AIExecutionProfilesModel; +use crate::ai::acp::acp_selection_identity; use crate::ai::bedrock::models::get_effective_models; use crate::auth::auth_manager::{AuthManager, AuthManagerEvent}; use crate::auth::AuthStateProvider; @@ -576,6 +577,8 @@ pub struct LLMPreferences { /// Used as a short-lived fallback while the fetched list is persisted to settings. #[cfg(not(target_family = "wasm"))] fetched_openai_models: Vec, + #[cfg(not(target_family = "wasm"))] + acp_selections: HashMap>, } impl LLMPreferences { @@ -640,6 +643,8 @@ impl LLMPreferences { | AISettingsChangedEvent::OpenAIApiKey { .. } | AISettingsChangedEvent::OpenAIModels { .. } | AISettingsChangedEvent::OpenAIProviders { .. } + | AISettingsChangedEvent::AcpAgents { .. } + | AISettingsChangedEvent::AcpAgentId { .. } ) { me.inject_bedrock_models(ctx); me.inject_openai_models(ctx); @@ -671,6 +676,8 @@ impl LLMPreferences { openai_provider_routing: HashMap::new(), #[cfg(not(target_family = "wasm"))] fetched_openai_models: Vec::new(), + #[cfg(not(target_family = "wasm"))] + acp_selections: HashMap::new(), }; // Seed from any already-loaded local config (the async load emits @@ -1037,56 +1044,97 @@ impl LLMPreferences { #[cfg(not(target_family = "wasm"))] fn inject_acp_models(&mut self, ctx: &AppContext) { + self.acp_selections.clear(); let settings = AISettings::as_ref(ctx); for agent in settings.acp_agents.value() { let model_option = agent .config_options .iter() .find(|option| option.category.as_deref() == Some("model")); - let Some(model_option) = model_option else { continue }; + let Some(model_option) = model_option else { + continue; + }; let secondary = agent.config_options.iter().filter(|option| { - matches!(option.category.as_deref(), Some("mode") | Some("thought_level")) + matches!( + option.category.as_deref(), + Some("mode") | Some("thought_level") + ) }); for value in &model_option.options { - let suffix = secondary .clone() - .filter_map(|option| option.options.first().map(|v| v.name.clone())) + .filter_map(|option| { + option + .options + .iter() + .find(|value| value.value == option.current_value) + .or_else(|| option.options.first()) + .map(|value| value.name.clone()) + }) .collect::>(); let display_name = if suffix.is_empty() { value.name.clone() } else { format!("{} ({})", value.name, suffix.join(", ")) }; - let id = format!("acp:{}:{}={}", agent.id, model_option.id, value.value); + let mut selection = + crate::ai::acp::AcpRuntimeModel::current_config_values(&agent.config_options); + selection.insert(model_option.id.clone(), value.value.clone()); + let id = acp_selection_identity(&agent.id, &selection); + let llm_id = LLMId::from(id.as_str()); + self.acp_selections.insert(llm_id.clone(), selection); let info = LLMInfo { - id: LLMId::from(id.as_str()), + id: llm_id, display_name, base_model_name: value.name.clone(), - reasoning_level: None, - usage_metadata: LLMUsageMetadata { - request_multiplier: 1, - credit_multiplier: None, - }, - description: Some(agent.name.clone()), - disable_reason: None, - vision_supported: false, - spec: None, - provider: LLMProvider::Unknown, - host_configs: HashMap::new(), - discount_percentage: None, - context_window: LLMContextWindow::default(), - }; - self.models_by_feature.agent_mode.choices.push(info.clone()); - self.models_by_feature.coding.choices.push(info.clone()); - if let Some(ref mut cli) = self.models_by_feature.cli_agent { - cli.choices.push(info); - } + reasoning_level: None, + usage_metadata: LLMUsageMetadata { + request_multiplier: 1, + credit_multiplier: None, + }, + description: Some(agent.name.clone()), + disable_reason: None, + vision_supported: false, + spec: None, + provider: LLMProvider::Unknown, + host_configs: HashMap::new(), + discount_percentage: None, + context_window: LLMContextWindow::default(), + }; + self.models_by_feature.agent_mode.choices.push(info.clone()); + self.models_by_feature.coding.choices.push(info.clone()); + if let Some(ref mut cli) = self.models_by_feature.cli_agent { + cli.choices.push(info); } } } } + #[cfg(not(target_family = "wasm"))] + pub fn acp_selection_for_model( + &self, + model_id: &LLMId, + ) -> Option<&BTreeMap> { + self.acp_selections.get(model_id) + } + + #[cfg(not(target_family = "wasm"))] + pub fn selected_acp_config_for_agent( + &self, + agent_name: &str, + ctx: &AppContext, + ) -> Option> { + let profile = AIExecutionProfilesModel::as_ref(ctx).active_profile(None, ctx); + let model_id = profile.data().base_model.as_ref()?; + let model = self.models_by_feature.agent_mode.info_for_id(model_id)?; + model + .description + .as_deref() + .is_some_and(|name| name.eq_ignore_ascii_case(agent_name)) + .then(|| self.acp_selections.get(model_id).cloned()) + .flatten() + } + /// Ensures the default model ID in each feature's choices still points to /// an existing entry. If the default was removed (e.g. provider disabled), /// switch to the first remaining choice. diff --git a/app/src/settings/ai.rs b/app/src/settings/ai.rs index 64401ed5..d2bd88aa 100644 --- a/app/src/settings/ai.rs +++ b/app/src/settings/ai.rs @@ -916,6 +916,15 @@ pub struct AcpAgentSettings { pub capabilities: Vec, #[serde(default)] pub config_options: Vec, + /// RFC 3339 timestamp for the last discovery attempt. + #[serde(default)] + pub discovery_timestamp: Option, + /// Identifier for the source of the cached discovery data. + #[serde(default)] + pub discovery_source: Option, + /// Actionable error from the last failed discovery attempt. + #[serde(default)] + pub discovery_error: Option, } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, schemars::JsonSchema)] diff --git a/app/src/settings_view/ai_page.rs b/app/src/settings_view/ai_page.rs index 63a44f9f..df9529c4 100644 --- a/app/src/settings_view/ai_page.rs +++ b/app/src/settings_view/ai_page.rs @@ -54,6 +54,7 @@ use crate::ai::blocklist::agent_view::agent_input_footer::editor::{ }; use crate::ai::blocklist::BlocklistAIPermissions; use crate::ai::execution_profiles::model_menu_items::available_model_menu_items; +#[cfg(not(target_family = "wasm"))] use crate::ai::execution_profiles::profiles::{ AIExecutionProfilesModel, AIExecutionProfilesModelEvent, ClientProfileId, }; @@ -689,6 +690,8 @@ pub struct AISettingsPageView { // Profile views profile_views: Vec>, add_profile_button: ViewHandle, + #[cfg(not(target_family = "wasm"))] + refresh_acp_button: ViewHandle, // Custom model router views (gated on FeatureFlag::CustomModelRouters) #[cfg(feature = "local_fs")] @@ -698,6 +701,85 @@ pub struct AISettingsPageView { } impl AISettingsPageView { + #[cfg(not(target_family = "wasm"))] + fn refresh_acp_discovery(&mut self, ctx: &mut ViewContext) { + let (config, agent_id) = { + let settings = AISettings::as_ref(ctx); + let Ok(config) = crate::ai::acp::AcpRuntimeModel::discovery_config(settings) else { + log::warn!("Could not resolve ACP launch configuration for discovery"); + return; + }; + (config, settings.acp_agent_id.value().clone()) + }; + /* + * The settings borrow must end before updating the runtime singleton. + */ + let manager = + match crate::ai::acp::AcpRuntimeModel::handle(ctx).update(ctx, |runtime, ctx| { + runtime.begin_discovery(ctx); + runtime.manager(config) + }) { + Ok(manager) => manager, + Err(error) => { + log::warn!("Could not start ACP discovery: {error}"); + return; + } + }; + let cwd = std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from("/")); + let _ = ctx.spawn( + async move { manager.discover_config_options(cwd, Vec::new()).await }, + move |_me, result, ctx| { + match result { + Ok(options) => { + let option_count = options.len(); + AISettings::handle(ctx).update(ctx, |settings, ctx| { + if let Err(error) = + crate::ai::acp::AcpRuntimeModel::upsert_agent_settings( + settings, &agent_id, options, ctx, + ) + { + log::warn!("Failed to persist ACP discovery: {error}"); + } + if let Err(error) = + crate::ai::acp::AcpRuntimeModel::mark_discovery_success( + settings, + &agent_id, + option_count, + ctx, + ) + { + log::warn!("Failed to persist ACP discovery metadata: {error}"); + } + }); + crate::ai::acp::AcpRuntimeModel::handle(ctx).update(ctx, |runtime, ctx| { + runtime.finish_discovery_success(option_count, ctx); + }); + } + Err(error) => { + log::warn!("ACP discovery failed: {error}"); + let error_text = error.to_string(); + AISettings::handle(ctx).update(ctx, |settings, ctx| { + if let Err(error) = + crate::ai::acp::AcpRuntimeModel::mark_discovery_failure( + settings, + &agent_id, + error_text.clone(), + ctx, + ) + { + log::warn!("Failed to persist ACP discovery failure: {error}"); + } + }); + crate::ai::acp::AcpRuntimeModel::handle(ctx).update(ctx, |runtime, ctx| { + runtime.finish_discovery_failure(error_text, ctx); + }); + } + } + ctx.notify(); + }, + ); + } + pub fn new(ctx: &mut ViewContext) -> Self { let is_any_ai_enabled = AISettings::as_ref(ctx).is_any_ai_enabled(ctx); @@ -1673,6 +1755,15 @@ impl AISettingsPageView { }) }); + #[cfg(not(target_family = "wasm"))] + let refresh_acp_button = ctx.add_typed_action_view(|_| { + ActionButton::new("Discover ACP options", SecondaryTheme) + .with_size(ButtonSize::Small) + .on_click(|ctx| { + ctx.dispatch_typed_action(AISettingsPageAction::RefreshAcpDiscovery) + }) + }); + add_profile_button.update(ctx, |button, ctx| { button.set_disabled(!is_any_ai_enabled, ctx); }); @@ -1781,6 +1872,8 @@ impl AISettingsPageView { conversation_layout_dropdown, profile_views, add_profile_button, + #[cfg(not(target_family = "wasm"))] + refresh_acp_button, #[cfg(feature = "local_fs")] router_views, #[cfg(feature = "local_fs")] @@ -2716,6 +2809,7 @@ pub enum AISettingsPageAction { ToggleBedrockCrossRegionInference, ToggleOpenAIEnabled, ToggleAcpEnabled, + RefreshAcpDiscovery, FetchOpenAIModels, ToggleFileBasedMcp, ToggleIncludeAgentCommandsInHistory, @@ -3482,6 +3576,10 @@ impl TypedActionView for AISettingsPageView { ctx.notify(); } } + AISettingsPageAction::RefreshAcpDiscovery => { + #[cfg(not(target_family = "wasm"))] + self.refresh_acp_discovery(ctx); + } AISettingsPageAction::FetchOpenAIModels => { // Trigger a fetch of models from the LiteLLM endpoint self.fetch_litellm_models(ctx); @@ -7592,6 +7690,8 @@ struct ACPSettingsWidget { agent_id_editor: ViewHandle, command_editor: ViewHandle, args_editor: ViewHandle, + #[cfg(not(target_family = "wasm"))] + refresh_button: ViewHandle, } impl ACPSettingsWidget { @@ -7653,6 +7753,15 @@ impl ACPSettingsWidget { } }); + #[cfg(not(target_family = "wasm"))] + let refresh_button = ctx.add_typed_action_view(|_| { + ActionButton::new("Discover ACP options", SecondaryTheme) + .with_size(ButtonSize::Small) + .on_click(|ctx| { + ctx.dispatch_typed_action(AISettingsPageAction::RefreshAcpDiscovery); + }) + }); + for editor in [ agent_id_editor.clone(), command_editor.clone(), @@ -7683,6 +7792,8 @@ impl ACPSettingsWidget { agent_id_editor, command_editor, args_editor, + #[cfg(not(target_family = "wasm"))] + refresh_button, } } @@ -7772,6 +7883,11 @@ impl SettingsWidget for ACPSettingsWidget { let is_enabled = *settings.acp_enabled.value(); let mut column = Flex::column().with_spacing(16.); + #[cfg(not(target_family = "wasm"))] + let discovery_state = crate::ai::acp::AcpRuntimeModel::as_ref(app) + .discovery_state() + .clone(); + column.add_child(build_sub_header(appearance, "Agent Client Protocol", None).finish()); column.add_child(render_ai_setting_toggle::( "Use an ACP agent for new conversations", @@ -7824,13 +7940,61 @@ impl SettingsWidget for ACPSettingsWidget { is_enabled, app, )); + #[cfg(not(target_family = "wasm"))] + column.add_child(self.refresh_button.as_ref(app).render(app)); + #[cfg(not(target_family = "wasm"))] + match discovery_state { + crate::ai::acp::AcpDiscoveryState::Idle => {} + crate::ai::acp::AcpDiscoveryState::Running => { + column.add_child(render_ai_setting_description( + "Discovering ACP configuration options...", + is_enabled, + app, + )); + } + crate::ai::acp::AcpDiscoveryState::Succeeded { option_count } => { + column.add_child(render_ai_setting_description( + format!("ACP discovery succeeded with {option_count} configuration option(s)."), + is_enabled, + app, + )); + } + crate::ai::acp::AcpDiscoveryState::Failed { message } => { + column.add_child(render_ai_setting_description( + format!("ACP discovery failed: {message}. Cached options were retained."), + is_enabled, + app, + )); + } + } let discovered = settings.acp_agents.value(); if let Some(agent) = discovered .iter() .find(|agent| agent.id.eq_ignore_ascii_case(settings.acp_agent_id.value())) { + if let Some(timestamp) = &agent.discovery_timestamp { + column.add_child(render_ai_setting_description( + format!("Last ACP discovery attempt: {timestamp}"), + is_enabled, + app, + )); + } + if let Some(source) = &agent.discovery_source { + column.add_child(render_ai_setting_description( + format!("Discovery source: {source}"), + is_enabled, + app, + )); + } + if let Some(error) = &agent.discovery_error { + column.add_child(render_ai_setting_description( + format!("Last discovery error: {error}. Cached options are retained."), + is_enabled, + app, + )); + } column.add_child(render_ai_setting_description( - &format!("Discovered {} ACP configuration option(s) for {}. Options are refreshed from the running agent and cached in settings.toml.", agent.config_options.len(), agent.name), + format!("Discovered {} ACP configuration option(s) for {}. Options are refreshed from the running agent and cached in settings.toml.", agent.config_options.len(), agent.name), is_enabled, app, )); diff --git a/crates/acp/Cargo.toml b/crates/acp/Cargo.toml index 161a979a..0899bf8d 100644 --- a/crates/acp/Cargo.toml +++ b/crates/acp/Cargo.toml @@ -9,6 +9,7 @@ agent-client-protocol.workspace = true async-channel.workspace = true async-io.workspace = true futures.workspace = true +log.workspace = true serde.workspace = true serde_json.workspace = true thiserror.workspace = true diff --git a/crates/acp/src/events.rs b/crates/acp/src/events.rs index 546c9317..ac20909d 100644 --- a/crates/acp/src/events.rs +++ b/crates/acp/src/events.rs @@ -6,6 +6,7 @@ use agent_client_protocol::schema::v1::{ use crate::PermissionDecision; /// Visible events produced while Galaxy drives an ACP turn. +#[allow(clippy::large_enum_variant)] #[derive(Clone, Debug, PartialEq)] #[non_exhaustive] pub enum AcpEvent { diff --git a/crates/acp/src/events_tests.rs b/crates/acp/src/events_tests.rs index 66cd8ac1..bda1748a 100644 --- a/crates/acp/src/events_tests.rs +++ b/crates/acp/src/events_tests.rs @@ -1,4 +1,6 @@ -use agent_client_protocol::schema::v1::{ContentBlock, SessionId, StopReason, TextContent}; +use agent_client_protocol::schema::v1::{ + AgentCapabilities, ContentBlock, SessionId, StopReason, TextContent, +}; use super::*; @@ -6,6 +8,8 @@ use super::*; fn session_started_exposes_agent_capabilities() { let event = AcpEvent::SessionStarted { session_id: SessionId::new("session-1"), + agent_info: None, + capabilities: AgentCapabilities::default(), can_load: true, can_steer: false, }; @@ -14,6 +18,8 @@ fn session_started_exposes_agent_capabilities() { event, AcpEvent::SessionStarted { session_id: SessionId::new("session-1"), + agent_info: None, + capabilities: AgentCapabilities::default(), can_load: true, can_steer: false, } diff --git a/crates/acp/src/lib.rs b/crates/acp/src/lib.rs index 5a8479cc..50299f74 100644 --- a/crates/acp/src/lib.rs +++ b/crates/acp/src/lib.rs @@ -10,9 +10,10 @@ mod permissions; mod runtime; pub use agent_client_protocol::schema::v1::{ - ContentBlock, Cost, ImageContent, McpServer, McpServerHttp, McpServerStdio, PermissionOptionId, - SessionConfigOption, SessionConfigOptionCategory, SessionConfigOptionValue, SessionId, - StopReason, TextContent, ToolCallId, ToolCallStatus, + AgentCapabilities, ContentBlock, Cost, ImageContent, McpServer, McpServerHttp, McpServerStdio, + PermissionOptionId, SessionConfigKind as SessionConfigOptionType, SessionConfigOption, + SessionConfigOptionCategory, SessionConfigOptionValue, SessionConfigSelectOption, + SessionConfigSelectOptions, SessionId, StopReason, TextContent, ToolCallId, ToolCallStatus, }; pub use config::{ AcpAgentPreset, AcpLaunchConfig, AcpManagerConfig, CODEX_ACP_NPM_VERSION, OPENCODE_NPM_VERSION, diff --git a/crates/acp/src/runtime.rs b/crates/acp/src/runtime.rs index c067fac3..b919ffac 100644 --- a/crates/acp/src/runtime.rs +++ b/crates/acp/src/runtime.rs @@ -8,11 +8,12 @@ use std::{io, thread}; use agent_client_protocol::schema::v1::{ AuthMethod, AuthMethodId, AuthenticateRequest, CancelNotification, ClientCapabilities, - ContentBlock, Implementation, InitializeRequest, InitializeResponse, LoadSessionRequest, - McpServer, Meta, NewSessionRequest, PromptRequest, PromptResponse, RequestPermissionOutcome, + ClientSessionCapabilities, CloseSessionRequest, ContentBlock, DeleteSessionRequest, + Implementation, InitializeRequest, InitializeResponse, LoadSessionRequest, McpServer, Meta, + NewSessionRequest, PromptRequest, PromptResponse, RequestPermissionOutcome, RequestPermissionRequest, RequestPermissionResponse, SessionConfigOption, - SessionConfigOptionValue, SessionId, SessionNotification, SessionUpdate, - SetSessionConfigOptionRequest, StopReason, TextContent, ToolCallContent, + SessionConfigOptionValue, SessionConfigOptionsCapabilities, SessionId, SessionNotification, + SessionUpdate, SetSessionConfigOptionRequest, StopReason, TextContent, ToolCallContent, }; use agent_client_protocol::schema::ProtocolVersion; use agent_client_protocol::{ @@ -20,7 +21,6 @@ use agent_client_protocol::{ }; use async_channel::{Receiver, Sender}; use futures::channel::oneshot; - use futures::future::{self, Either, FutureExt as _}; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -37,6 +37,7 @@ static NEXT_PERMISSION_ID: AtomicU64 = AtomicU64::new(1); const MAX_VISIBLE_TOOL_OUTPUT_BYTES: usize = 32 * 1024; const TOOL_OUTPUT_TRUNCATION_MARKER: &str = "\n… [tool output truncated by Galaxy]"; const STEERING_TIMEOUT: Duration = Duration::from_secs(5); +const DISCOVERY_TIMEOUT: Duration = Duration::from_secs(30); /// Errors returned by the ACP runtime's public control surface. #[derive(Debug, Error)] @@ -73,6 +74,9 @@ pub enum AcpRuntimeError { /// Agent-owned authentication did not complete in time. #[error("ACP agent did not authenticate within {0:?}")] AuthenticationTimeout(Duration), + /// Temporary discovery did not complete in time. + #[error("ACP configuration discovery timed out after {0:?}")] + DiscoveryTimeout(Duration), } /// Result of the Codex `_session/steering` extension. @@ -108,6 +112,8 @@ pub struct AcpTurnRequest { pub auto_approve_permissions: bool, /// Category permissions inherited from Galaxy's active execution profile. pub permission_policy: AcpPermissionPolicy, + /// Prompt capabilities advertised by the agent. + pub prompt_capabilities: agent_client_protocol::schema::v1::PromptCapabilities, } impl AcpTurnRequest { @@ -128,6 +134,7 @@ impl AcpTurnRequest { mcp_servers: Vec::new(), auto_approve_permissions: false, permission_policy: AcpPermissionPolicy::default(), + prompt_capabilities: Default::default(), } } @@ -216,6 +223,26 @@ impl AcpTurnRequest { } } } + for content in &self.prompt { + match content { + ContentBlock::Image(_) if !self.prompt_capabilities.image => { + return Err(AcpRuntimeError::InvalidTurn( + "agent does not advertise image prompt support".to_owned(), + )); + } + ContentBlock::Audio(_) if !self.prompt_capabilities.audio => { + return Err(AcpRuntimeError::InvalidTurn( + "agent does not advertise audio prompt support".to_owned(), + )); + } + ContentBlock::Resource(_) if !self.prompt_capabilities.embedded_context => { + return Err(AcpRuntimeError::InvalidTurn( + "agent does not advertise embedded context support".to_owned(), + )); + } + _ => {} + } + } Ok(()) } } @@ -709,7 +736,7 @@ enum Command { conversation_key: String, turn_id: u64, requested_session_id: Option, - result: Result, + result: Result<(SessionId, Vec), agent_client_protocol::Error>, }, PromptFinished { conversation_key: String, @@ -874,14 +901,52 @@ impl RuntimeActor { result: oneshot::Sender, AcpRuntimeError>>, ) -> Result<(), AcpRuntimeError> { let connection = self.connection.clone(); + let can_delete = self + .agent_capabilities + .session_capabilities + .delete + .is_some(); + let can_close = self.agent_capabilities.session_capabilities.close.is_some(); self.connection .spawn(async move { - let response = connection + let request = connection .send_request(NewSessionRequest::new(cwd).mcp_servers(mcp_servers)) - .block_task() - .await; + .block_task(); + let response = match future::select( + Box::pin(request), + Box::pin(async_io::Timer::after(DISCOVERY_TIMEOUT)), + ) + .await + { + Either::Left((response, _)) => response, + Either::Right((_, _)) => { + let _ = + result.send(Err(AcpRuntimeError::DiscoveryTimeout(DISCOVERY_TIMEOUT))); + return Ok(()); + } + }; let result_value = match response { - Ok(response) => Ok(response.config_options.unwrap_or_default()), + Ok(response) => { + let session_id = response.session_id; + let options = response.config_options.unwrap_or_default(); + let cleanup = if can_delete { + connection + .send_request(DeleteSessionRequest::new(session_id.clone())) + .block_task() + .await + .map(|_| ()) + } else if can_close { + connection + .send_request(CloseSessionRequest::new(session_id.clone())) + .block_task() + .await + .map(|_| ()) + } else { + Ok(()) + }; + let _ = cleanup; + Ok(options) + } Err(error) => Err(AcpRuntimeError::Protocol(error.to_string())), }; let _ = result.send(result_value); @@ -984,6 +1049,11 @@ impl RuntimeActor { let additional_directories = request.additional_directories.clone(); let mcp_servers = request.mcp_servers.clone(); let config_values = request.config_values.clone(); + let can_additional_directories = self + .agent_capabilities + .session_capabilities + .additional_directories + .is_some(); let suppressed_session_id = requested_session_id.clone(); let replay_session_id = requested_session_id.clone(); let spawn_result = self.connection.spawn(async move { @@ -991,39 +1061,57 @@ impl RuntimeActor { let load_cwd = cwd.clone(); let load_additional_directories = additional_directories.clone(); let load_mcp_servers = mcp_servers.clone(); - let result = open_session( + let load_config_values = config_values.clone(); + let result = open_session_with_options( requested_session_id.clone(), move |session_id| async move { - load_connection + let loaded_session_id = session_id.clone(); + let response = load_connection .send_request( - LoadSessionRequest::new(session_id, load_cwd) - .additional_directories(load_additional_directories) + LoadSessionRequest::new(session_id.clone(), load_cwd) + .additional_directories(if can_additional_directories { + load_additional_directories + } else { + Vec::new() + }) .mcp_servers(load_mcp_servers), ) .block_task() - .await - .map(|_| ()) + .await?; + let options = response.config_options.unwrap_or_default(); + let applied = valid_config_values(&options, &load_config_values); + let options = apply_config_values( + &load_connection, + loaded_session_id.clone(), + options, + applied, + ) + .await?; + Ok((loaded_session_id, options)) }, move || async move { let response = connection .send_request( NewSessionRequest::new(cwd) - .additional_directories(additional_directories) + .additional_directories(if can_additional_directories { + additional_directories + } else { + Vec::new() + }) .mcp_servers(mcp_servers), ) .block_task() .await?; - for (config_id, value) in config_values { - connection - .send_request(SetSessionConfigOptionRequest::new( - response.session_id.clone(), - config_id, - value, - )) - .block_task() - .await?; - } - Ok(response.session_id) + let mut config_options = response.config_options.clone().unwrap_or_default(); + let applied = valid_config_values(&config_options, &config_values); + config_options = apply_config_values( + &connection, + response.session_id.clone(), + config_options, + applied, + ) + .await?; + Ok((response.session_id, config_options)) }, ) .await; @@ -1054,7 +1142,7 @@ impl RuntimeActor { conversation_key: &str, turn_id: u64, requested_session_id: Option<&SessionId>, - result: Result, + result: Result<(SessionId, Vec), agent_client_protocol::Error>, ) -> Result<(), AcpRuntimeError> { let is_current = self .conversations @@ -1066,7 +1154,7 @@ impl RuntimeActor { } match result { - Ok(session_id) => { + Ok((session_id, config_options)) => { if let Some(requested_session_id) = requested_session_id { if requested_session_id != &session_id { self.router.remove_route(requested_session_id, turn_id); @@ -1108,6 +1196,9 @@ impl RuntimeActor { self.agent_info.clone(), self.agent_capabilities.clone(), ); + let _ = events.try_send(AcpEvent::ConfigOptions { + options: config_options, + }); if cancelled { let _ = events.try_send(AcpEvent::Finished { stop_reason: StopReason::Cancelled, @@ -1572,8 +1663,18 @@ async fn run_connection( meta.insert("terminal_output".to_owned(), serde_json::Value::Bool(true)); let response = connection .send_request( - InitializeRequest::new(ProtocolVersion::V1) - .client_capabilities(ClientCapabilities::new().meta(meta)) + InitializeRequest::new(ProtocolVersion::V1) + .client_capabilities( + ClientCapabilities::new() + .session( + ClientSessionCapabilities::new().config_options( + SessionConfigOptionsCapabilities::new().boolean( + agent_client_protocol::schema::v1::BooleanConfigOptionCapabilities::new(), + ), + ), + ) + .meta(meta), + ) .client_info( Implementation::new(config.client_name, config.client_version) .title("Galaxy"), @@ -2165,6 +2266,89 @@ fn event_from_content(content: ContentBlock, thought: bool) -> AcpEvent { } } +fn valid_config_values( + options: &[SessionConfigOption], + values: &std::collections::BTreeMap, +) -> std::collections::BTreeMap { + values + .iter() + .filter(|(id, value)| { + options.iter().any(|option| { + option.id.to_string() == **id && config_value_is_advertised(option, value) + }) + }) + .map(|(id, value)| (id.clone(), value.clone())) + .collect() +} + +fn config_value_is_advertised( + option: &SessionConfigOption, + value: &SessionConfigOptionValue, +) -> bool { + match (&option.kind, value) { + ( + agent_client_protocol::schema::v1::SessionConfigKind::Boolean(boolean), + SessionConfigOptionValue::Boolean { value }, + ) => { + let _ = boolean; + let _ = value; + true + } + (agent_client_protocol::schema::v1::SessionConfigKind::Select(select), value) => { + let Some(value_id) = value.as_value_id() else { + return false; + }; + match &select.options { + agent_client_protocol::schema::v1::SessionConfigSelectOptions::Ungrouped( + values, + ) => values.iter().any(|candidate| candidate.value == *value_id), + agent_client_protocol::schema::v1::SessionConfigSelectOptions::Grouped(groups) => { + groups + .iter() + .flat_map(|group| &group.options) + .any(|candidate| candidate.value == *value_id) + } + _ => false, + } + } + _ => false, + } +} + +async fn apply_config_values( + connection: &ConnectionTo, + session_id: SessionId, + mut options: Vec, + values: std::collections::BTreeMap, +) -> Result, agent_client_protocol::Error> { + for (config_id, value) in values { + let Some(option) = options + .iter() + .find(|option| option.id.to_string() == config_id) + else { + continue; + }; + if !config_value_is_advertised(option, &value) { + continue; + } + let response = connection + .send_request(SetSessionConfigOptionRequest::new( + session_id.clone(), + config_id, + value, + )) + .block_task() + .await; + match response { + Ok(response) => options = response.config_options, + Err(error) => { + log::warn!("ACP rejected persisted configuration option: {error}"); + } + } + } + Ok(options) +} + fn supports_steering(response: &InitializeResponse) -> bool { response .meta @@ -2194,6 +2378,7 @@ fn restorable_session_id( } } +#[cfg(test)] async fn open_session( requested_session_id: Option, load: Load, @@ -2212,6 +2397,27 @@ where create().await } +async fn open_session_with_options( + requested_session_id: Option, + load: Load, + create: Create, +) -> Result<(SessionId, Vec), agent_client_protocol::Error> +where + Load: FnOnce(SessionId) -> LoadFuture, + LoadFuture: Future< + Output = Result<(SessionId, Vec), agent_client_protocol::Error>, + >, + Create: FnOnce() -> CreateFuture, + CreateFuture: Future< + Output = Result<(SessionId, Vec), agent_client_protocol::Error>, + >, +{ + if let Some(session_id) = requested_session_id { + return load(session_id.clone()).await; + } + create().await +} + fn emit_session_started( events: &Sender, session_id: SessionId, diff --git a/crates/acp/src/runtime_tests.rs b/crates/acp/src/runtime_tests.rs index b6fc33cd..584f13f2 100644 --- a/crates/acp/src/runtime_tests.rs +++ b/crates/acp/src/runtime_tests.rs @@ -8,7 +8,8 @@ use std::time::Duration; use agent_client_protocol::schema::v1::{ AuthMethod, AuthMethodAgent, AuthMethodId, ContentBlock, ContentChunk, InitializeResponse, - McpServer, McpServerStdio, SessionId, SessionUpdate, TextContent, ToolCall, ToolCallStatus, + McpServer, McpServerStdio, SessionConfigOption, SessionConfigOptionValue, + SessionConfigSelectOption, SessionId, SessionUpdate, TextContent, ToolCall, ToolCallStatus, ToolCallUpdate, ToolCallUpdateFields, UsageUpdate, }; use agent_client_protocol::schema::ProtocolVersion; @@ -139,6 +140,44 @@ fn successful_session_load_does_not_create_a_replacement() { assert_eq!(create_calls.load(Ordering::Relaxed), 0); } +#[test] +fn stale_config_values_are_dropped_before_session_application() { + let options = vec![SessionConfigOption::select( + "model", + "Model", + "fast", + vec![SessionConfigSelectOption::new("fast", "Fast")], + )]; + let values = std::collections::BTreeMap::from([ + ( + "model".to_owned(), + SessionConfigOptionValue::value_id("missing"), + ), + ( + "unknown".to_owned(), + SessionConfigOptionValue::value_id("value"), + ), + ]); + + assert!(valid_config_values(&options, &values).is_empty()); +} + +#[test] +fn advertised_config_values_are_retained() { + let options = vec![SessionConfigOption::select( + "model", + "Model", + "fast", + vec![SessionConfigSelectOption::new("fast", "Fast")], + )]; + let values = std::collections::BTreeMap::from([( + "model".to_owned(), + SessionConfigOptionValue::value_id("fast"), + )]); + + assert_eq!(valid_config_values(&options, &values), values); +} + #[test] fn failed_session_load_is_visible_and_does_not_create_a_replacement() { let load_calls = Arc::new(AtomicUsize::new(0)); @@ -666,6 +705,8 @@ fn manager_liveness_is_observable() { let manager = AcpSessionManager { inner: Arc::new(ManagerInner { command_tx, + agent_info: Mutex::new(None), + agent_capabilities: Mutex::new(None), launch: AcpLaunchConfig::new("agent"), alive: AtomicBool::new(true), terminal_error: Mutex::new(None), @@ -696,6 +737,8 @@ fn dropping_the_last_manager_requests_worker_shutdown() { let manager = AcpSessionManager { inner: Arc::new(ManagerInner { command_tx, + agent_info: Mutex::new(None), + agent_capabilities: Mutex::new(None), launch: AcpLaunchConfig::new("agent"), alive: AtomicBool::new(true), terminal_error: Mutex::new(None), @@ -713,6 +756,8 @@ fn session_handle_cancel_targets_its_exact_turn_and_waits_for_ack() { let manager = AcpSessionManager { inner: Arc::new(ManagerInner { command_tx, + agent_info: Mutex::new(None), + agent_capabilities: Mutex::new(None), launch: AcpLaunchConfig::new("agent"), alive: AtomicBool::new(true), terminal_error: Mutex::new(None), @@ -750,6 +795,8 @@ fn steering_uses_its_typed_command_and_preserves_unsupported_error() { let manager = AcpSessionManager { inner: Arc::new(ManagerInner { command_tx, + agent_info: Mutex::new(None), + agent_capabilities: Mutex::new(None), launch: AcpLaunchConfig::new("agent"), alive: AtomicBool::new(true), terminal_error: Mutex::new(None), diff --git a/crates/persistence/src/model_tests.rs b/crates/persistence/src/model_tests.rs index 86c90c1b..62da6edf 100644 --- a/crates/persistence/src/model_tests.rs +++ b/crates/persistence/src/model_tests.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use warp_multi_agent_api as api; @@ -12,6 +12,10 @@ fn acp_backend_fork_keeps_agent_identity_but_clears_session() { agent_id: "codex".to_owned(), launch_fingerprint: "launch-123".to_owned(), session_id: Some("shared-session".to_owned()), + config_values: BTreeMap::from([ + ("model".to_owned(), serde_json::json!("fast")), + ("thinking".to_owned(), serde_json::json!(true)), + ]), }); assert_eq!( @@ -20,6 +24,10 @@ fn acp_backend_fork_keeps_agent_identity_but_clears_session() { agent_id: "codex".to_owned(), launch_fingerprint: "launch-123".to_owned(), session_id: None, + config_values: BTreeMap::from([ + ("model".to_owned(), serde_json::json!("fast")), + ("thinking".to_owned(), serde_json::json!(true)), + ]), }) ); assert_eq!(AgentBackend::Provider.for_fork(), AgentBackend::Provider); @@ -178,6 +186,7 @@ fn agent_conversation_data_roundtrips_acp_backend() { agent_id: "codex-acp".to_string(), launch_fingerprint: "launch-123".to_string(), session_id: Some("session-123".to_string()), + config_values: BTreeMap::from([("model".to_owned(), serde_json::json!("fast"))]), }), ..Default::default() }; diff --git a/rust-toolchain.toml b/rust-toolchain.toml index ef450a16..5ec58696 100644 --- a/rust-toolchain.toml +++ b/rust-toolchain.toml @@ -1,4 +1,4 @@ [toolchain] -channel = "1.94.1" +channel = "1.97.1" components = ["rustfmt", "clippy"] profile = "minimal"